From owner-freebsd-arch@FreeBSD.ORG Wed Jun 10 16:22:32 2009 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id EDF1C106566C; Wed, 10 Jun 2009 16:22:32 +0000 (UTC) (envelope-from imp@bsdimp.com) Received: from harmony.bsdimp.com (bsdimp.com [199.45.160.85]) by mx1.freebsd.org (Postfix) with ESMTP id 859258FC26; Wed, 10 Jun 2009 16:22:32 +0000 (UTC) (envelope-from imp@bsdimp.com) Received: from localhost (localhost [127.0.0.1]) by harmony.bsdimp.com (8.14.3/8.14.1) with ESMTP id n5AGLO8o065400; Wed, 10 Jun 2009 10:21:24 -0600 (MDT) (envelope-from imp@bsdimp.com) Date: Wed, 10 Jun 2009 10:21:44 -0600 (MDT) Message-Id: <20090610.102144.324381338.imp@bsdimp.com> To: jhb@freebsd.org From: "M. Warner Losh" In-Reply-To: <200906100822.15516.jhb@freebsd.org> References: <20090609.174249.-1435625969.imp@bsdimp.com> <200906100822.15516.jhb@freebsd.org> X-Mailer: Mew version 5.2 on Emacs 21.3 / Mule 5.0 (SAKAKI) Mime-Version: 1.0 Content-Type: Multipart/Mixed; boundary="--Next_Part(Wed_Jun_10_10_21_44_2009_872)--" Content-Transfer-Encoding: 7bit Cc: freebsd-arch@freebsd.org Subject: Re: devclass_find_free_unit X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 10 Jun 2009 16:22:33 -0000 ----Next_Part(Wed_Jun_10_10_21_44_2009_872)-- Content-Type: Text/Plain; charset=us-ascii Content-Transfer-Encoding: 7bit In message: <200906100822.15516.jhb@freebsd.org> John Baldwin writes: : On Tuesday 09 June 2009 7:42:49 pm M. Warner Losh wrote: : > What purpose does devclass_find_free_unit serve? I think it can safely be : > eliminated from the tree. The current design is racy. : > : > Comments? : > : > It is currently used: : > : > ./arm/xscale/ixp425/.svn/text-base/avila_ata.c.svn-base: : device_add_child(dev, "ata", devclass_find_free_unit(ata_devclass, 0)); : > ./arm/xscale/ixp425/avila_ata.c: device_add_child(dev, "ata", : devclass_find_free_unit(ata_devclass, 0)); : > ./arm/at91/.svn/text-base/at91_cfata.c.svn-base: : device_add_child(dev, "ata", devclass_find_free_unit(ata_devclass, 0)); : > ./arm/at91/at91_cfata.c: device_add_child(dev, "ata", : devclass_find_free_unit(ata_devclass, 0)); : > ./powerpc/psim/.svn/text-base/ata_iobus.c.svn-base: : devclass_find_free_unit(ata_devclass, 0)); : > : > # All the above can be replaced with a simple '-1'. : > : > ata/ata-pci.c: unit : devclass_find_free_unit(ata_devclass, 2)); : > ata/ata-usb.c: devclass_find_free_unit(ata_devclass, 2))) == : NULL) { : > : > These can likely be replaced by '2', but that may result in a warning : > message being printed that likely can be eliminated... : : ata does this so it can reserve ata0 and ata1 for the "legacy" ATA channels on : legacy ATA PCI adapters. That is, if you have both SATA controllers and a : PATA controller, this allows the two PATA channels to always be ata0 and ata1 : and the PATA drivers to always be ad0 - ad3. You could perhaps implement : this in 8.x now by a really horrendous hack of having ISA hints for ata0 and : ata1 and letting bus_hint_device_unit() in the atapci driver claim those : hints for the channels on PATA controllers. I think it already does something akin to this: /* attach all channels on this controller */ for (unit = 0; unit < ctlr->channels; unit++) { if ((ctlr->ichannels & (1 << unit)) == 0) continue; child = device_add_child(dev, "ata", ((unit == 0 || unit == 1) && ctlr->legacy) ? unit : devclass_find_free_unit(ata_devclass, 2)); if (child == NULL) device_printf(dev, "failed to add ata child device\n"); else device_set_ivars(child, (void *)(intptr_t)unit); } Why not just replace devclass_find_free_unit with '2'? All the other users in the tree aer bogus and should be replaced by -1. Well, I'm not 100% sure about the ata-usb.c patch, since that would also be necessary to avoid collision. And the above code really only applies to x86-based machine, right? There's no need to do that for non-intel boxes. Or is the assumption on those boxes the controller would never be in legacy. Warner ----Next_Part(Wed_Jun_10_10_21_44_2009_872)-- Content-Type: Text/Plain; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="devclass_find_free_fix.diff" Index: arm/xscale/ixp425/avila_ata.c =================================================================== --- arm/xscale/ixp425/avila_ata.c (revision 193873) +++ arm/xscale/ixp425/avila_ata.c (working copy) @@ -248,7 +248,7 @@ NULL, ata_avila_intr, sc, &sc->sc_ih); /* attach channel on this controller */ - device_add_child(dev, "ata", devclass_find_free_unit(ata_devclass, 0)); + device_add_child(dev, "ata", -1); bus_generic_attach(dev); return 0; Index: arm/at91/at91_cfata.c =================================================================== --- arm/at91/at91_cfata.c (revision 193873) +++ arm/at91/at91_cfata.c (working copy) @@ -94,7 +94,7 @@ /* XXX: init CF controller? */ callout_init(&sc->tick, 1); /* Callout to poll the device. */ - device_add_child(dev, "ata", devclass_find_free_unit(ata_devclass, 0)); + device_add_child(dev, "ata", -1); bus_generic_attach(dev); return (0); } Index: arm/at91/files.at91 =================================================================== --- arm/at91/files.at91 (revision 193873) +++ arm/at91/files.at91 (working copy) @@ -18,6 +18,11 @@ arm/at91/uart_bus_at91usart.c optional uart arm/at91/uart_cpu_at91rm9200usart.c optional uart arm/at91/uart_dev_at91usart.c optional uart + +dev/cfi/cfi_bus_lbc.c optional cfi +dev/cfi/cfi_core.c optional cfi +dev/cfi/cfi_dev.c optional cfi + # # All the boards we support # Index: arm/at91/at91_machdep.c =================================================================== --- arm/at91/at91_machdep.c (revision 193873) +++ arm/at91/at91_machdep.c (working copy) @@ -179,6 +179,14 @@ PTE_NOCACHE, }, { + AT91RM92_FLS_BASE, + AT91RM92_FLS_PA_BASE, + AT91RM92_FLS_SIZE, + VM_PROT_READ|VM_PROT_WRITE, + PTE_NOCACHE, + }, + + { /* CompactFlash controller. */ AT91RM92_CF_BASE, AT91RM92_CF_PA_BASE, Index: arm/at91/at91_mci.c =================================================================== --- arm/at91/at91_mci.c (revision 193873) +++ arm/at91/at91_mci.c (working copy) @@ -62,8 +62,15 @@ #include "mmcbr_if.h" -#define BBSZ 512 +// #define AT91_MCI_DEBUG +#define MAX_SIZE 1 +#define BBSZ 512 * MAX_SIZE +/* + * Note: This driver only supports the SlotA card. No attempt has been made + * to support SlotB. + */ + struct at91_mci_softc { void *intrhand; /* Interrupt handle */ device_t dev; @@ -204,10 +211,16 @@ sc->host.f_min = 375000; sc->host.f_max = at91_master_clock / 2; /* Typically 30MHz */ sc->host.host_ocr = MMC_OCR_320_330 | MMC_OCR_330_340; + sc->host.caps = 0; + /* + * The in-tree Linux driver doesn't allow 4-wire operation for the + * at91rm9200, but does for other members of the family. The atmel + * patches to this do allow it, or have in the past. It is unclear + * that the hardware even works, but my boot loader uses 4-bit bus + * in polling mode successfully. + */ if (sc->sc_cap & CAP_HAS_4WIRE) - sc->host.caps = MMC_CAP_4_BIT_DATA; - else - sc->host.caps = 0; + sc->host.caps |= MMC_CAP_4_BIT_DATA; child = device_add_child(dev, "mmc", 0); device_set_ivars(dev, &sc->host); err = bus_generic_attach(dev); @@ -300,9 +313,9 @@ clkdiv = (at91_master_clock / ios->clock) / 2; } if (ios->bus_width == bus_width_4) - WR4(sc, MCI_SDCR, RD4(sc, MCI_SDCR) | MCI_SDCR_SDCBUS); + WR4(sc, MCI_SDCR, MCI_SDCR_SDCBUS); else - WR4(sc, MCI_SDCR, RD4(sc, MCI_SDCR) & ~MCI_SDCR_SDCBUS); + WR4(sc, MCI_SDCR, 0); WR4(sc, MCI_MR, (RD4(sc, MCI_MR) & ~MCI_MR_CLKDIV) | clkdiv); /* Do we need a settle time here? */ /* XXX We need to turn the device on/off here with a GPIO pin */ @@ -341,7 +354,9 @@ if (!data) { // The no data case is fairly simple at91_mci_pdc_disable(sc); -// printf("CMDR %x ARGR %x\n", cmdr, cmd->arg); +#ifdef AT91_MCI_DEBUG + printf("CMDR %x ARGR %x\n", cmdr, cmd->arg); +#endif WR4(sc, MCI_ARGR, cmd->arg); WR4(sc, MCI_CMDR, cmdr); WR4(sc, MCI_IER, MCI_SR_ERROR | MCI_SR_CMDRDY); @@ -399,7 +414,9 @@ ier = MCI_SR_TXBUFE; } } -// printf("CMDR %x ARGR %x with data\n", cmdr, cmd->arg); +#ifdef AT91_MCI_DEBUG + printf("CMDR %x ARGR %x with data\n", cmdr, cmd->arg); +#endif WR4(sc, MCI_ARGR, cmd->arg); if (cmdr & MCI_CMDR_TRCMD_START) { if (cmdr & MCI_CMDR_TRDIR) { @@ -438,6 +455,14 @@ sc->req = NULL; sc->curcmd = NULL; req->done(req); + /* + * Attempted hack-a-round for the DMA bug for multiple reads. + */ + if (req->cmd->opcode == MMC_READ_MULTIPLE_BLOCK) { + at91_mci_fini(sc->dev); + at91_mci_init(sc->dev); + at91_mci_update_ios(sc->dev, NULL); + } } static int @@ -498,7 +523,9 @@ uint32_t *walker; struct mmc_command *cmd; int i, len; - +#ifdef AT91_MCI_DEBUG + char *w2; +#endif cmd = sc->curcmd; bus_dmamap_sync(sc->dmatag, sc->map, BUS_DMASYNC_POSTREAD); bus_dmamap_unload(sc->dmatag, sc->map); @@ -509,6 +536,15 @@ for (i = 0; i < len; i++) walker[i] = bswap32(walker[i]); } +#ifdef AT91_MCI_DEBUG + printf("Read data\n"); + for (i = 0, w2 = cmd->data->data; i < cmd->data->len; i++) { + if (i % 16 == 0) + printf("%08x ", cmd->arg + i); + printf("%02x%s", w2[i], (i + 1) % 16 ? " " : "\n"); + } + printf("\n"); +#endif // Finish up the sequence... WR4(sc, MCI_IDR, MCI_SR_ENDRX); WR4(sc, MCI_IER, MCI_SR_RXBUFF); @@ -544,14 +580,19 @@ if ((sr & MCI_SR_RCRCE) && (cmd->opcode == MMC_SEND_OP_COND || cmd->opcode == ACMD_SD_SEND_OP_COND)) cmd->error = MMC_ERR_NONE; - else if (sr & (MCI_SR_RTOE | MCI_SR_DTOE)) + else if (sr & (MCI_SR_RTOE | MCI_SR_DTOE)) { + printf("TIMEOUT %#x\n", sr); cmd->error = MMC_ERR_TIMEOUT; - else if (sr & (MCI_SR_RCRCE | MCI_SR_DCRCE)) + } else if (sr & (MCI_SR_RCRCE | MCI_SR_DCRCE)) { + printf("CRC %#x\n", sr); cmd->error = MMC_ERR_BADCRC; - else if (sr & (MCI_SR_OVRE | MCI_SR_UNRE)) + } else if (sr & (MCI_SR_OVRE | MCI_SR_UNRE)) { + printf("FIFO %#x\n", sr); cmd->error = MMC_ERR_FIFO; - else + } else { + printf("FAILED %#x\n", sr); cmd->error = MMC_ERR_FAILED; + } done = 1; if (sc->mapped && cmd->error) { bus_dmamap_unload(sc->dmatag, sc->map); @@ -656,7 +697,7 @@ *(int *)result = sc->host.caps; break; case MMCBR_IVAR_MAX_DATA: - *(int *)result = 1; + *(int *)result = MAX_SIZE; break; } return (0); Index: arm/at91/at91rm92reg.h =================================================================== --- arm/at91/at91rm92reg.h (revision 193873) +++ arm/at91/at91rm92reg.h (working copy) @@ -341,6 +341,10 @@ #define AT91RM92_OHCI_PA_BASE 0x00300000 #define AT91RM92_OHCI_SIZE 0x00100000 +#define AT91RM92_FLS_BASE 0xdf000000 +#define AT91RM92_FLS_PA_BASE 0x10000000 +#define AT91RM92_FLS_SIZE 0x02000000 /* Support up to 32MB flash */ + #define AT91RM92_CF_BASE 0xdfd00000 #define AT91RM92_CF_PA_BASE 0x51400000 #define AT91RM92_CF_SIZE 0x00100000 Index: powerpc/psim/ata_iobus.c =================================================================== --- powerpc/psim/ata_iobus.c (revision 193873) +++ powerpc/psim/ata_iobus.c (working copy) @@ -114,9 +114,7 @@ * Add a single child per controller. Should be able * to add two */ - device_add_child(dev, "ata", - devclass_find_free_unit(ata_devclass, 0)); - + device_add_child(dev, "ata", -1); return (bus_generic_attach(dev)); } Index: dev/ata/ata-all.c =================================================================== --- dev/ata/ata-all.c (revision 193873) +++ dev/ata/ata-all.c (working copy) @@ -663,7 +663,7 @@ btrim(atacap->serial, sizeof(atacap->serial)); bpack(atacap->serial, atacap->serial, sizeof(atacap->serial)); - if (bootverbose) + if (bootverbose || 1) printf("ata%d-%s: pio=%s wdma=%s udma=%s cable=%s wire\n", device_get_unit(ch->dev), ata_unit2str(atadev), Index: dev/ata/ata-usb.c =================================================================== --- dev/ata/ata-usb.c (revision 193873) +++ dev/ata/ata-usb.c (working copy) @@ -414,11 +414,10 @@ /* ata channels are children to this USB control device */ for (i = 0; i <= sc->maxlun; i++) { - if ((child = device_add_child(sc->dev, "ata", - devclass_find_free_unit(ata_devclass, 2))) == NULL) { + if ((child = device_add_child(sc->dev, "ata", -1)) == NULL) device_printf(sc->dev, "failed to add ata child device\n"); - } else - device_set_ivars(child, (void *)(intptr_t)i); + else + device_set_ivars(child, (void *)(intptr_t)i); } bus_generic_attach(sc->dev); ----Next_Part(Wed_Jun_10_10_21_44_2009_872)----