On Fri, 2013-09-27 at 04:18 +0000, Gupta, Pekon wrote: > > From: Scott Wood [mailto:scottw...@freescale.com] > > > > On Thu, 2013-09-26 at 13:14 +0000, Gupta, Pekon wrote: > > > > > From: Gupta, Pekon <pe...@ti.com> > > > > > > > > > > NAND driver needs to know bus-width of the connected NAND device, > > in > > > > order to perform proper I/O and initialize itself. Currently there is no > > CONFIG > > > > option to provide this information to NAND driver. > > > > > - SPL NAND driver does not have framework to parse ONFI parameter > > > > page. > > > > > > > > Is this about SPL? It looks like a more general change. > > > > > > > Yes, actually I would have loved to see a generic SPL driver for all > > > platforms, > > > > How could that possibly work? > > > NAND SPL (as in drivers/mtd/nand/am335x_spl_bch.c) had following limitation > (a) depends on CONFIG_SYS_NAND_xx (for NAND device parameters like > erasesize, pagesize, oobsize, etc). > (b) can only do NAND read access. > (c) calls nand_chip->ecc.hwctl() for doing controller specific > configurations, which is populated while doing board_nand_init(). > > Above (a), (b), and (c) do not have any SoC specific dependency. And can be > put into a generic framework which can be used for all SPL drivers.
Not all NAND controllers expose a low-level interface that matches hwctl() (e.g. fsl_ifc and fsl_elbc). For those that do, we do have a common nand_spl_simple.c that works for many drivers, without needing to hardcode the bus width (though perhaps a few bytes could be shaved off by hardcoding it). BTW, referring to "driver" in NAND context is ambiguous... I prefer to use it to refer to the controller drivers when not qualified as "high level driver" or similar. > All SoC specific configurations are done in either: > - board_nand_init(): initializations based on device. > - nand_chip->ecc.hwctl(): configurations for ECC scheme. > which is *shared* code between SPL & u-boot driver, and is present in > u-boot driver file (like drivers/mtd/nand/omap_gpmc.c), not in SPL driver > file. > > The missing piece was device bus-width detection which I addresses in > this patch by adding CONFIG_SYS_NAND_DEVICE_WIDTH. The changelog that introduced am335x_spl_bch.c says that "custom read_page implementation is required for proper syndrome generation." Other than that ECC mode, AFAICT you're already using nand_spl_simple.c. > > > because SPL is mostly statically configured, and it just does plain NAND > > > read > > > (it doesn’t even support NAND write, etc). > > > But I do not know the hardware configurations tweaks of other SoC, > > > So at-least have common CONFIG_SYS_NAND_xx which all SPL drivers can > > use. > > > > Again, are you introducing this symbol for use only in SPL? > Apart from SPL, CONFIG_SYS_NAND_DEVICE_WIDTH also be useful for > (1) drivers which do not use CONFIG_SYS_NAND_ONFI_DETECTION, where > code for reading on-chip ONFI parameters is not enabled in nand_base.c > (2) non ONFI compatible NAND devices. Unlikely, given that they've all managed to work without this so far. E.g. eLBC and IFC hardcode this information on a per-chip basis in the #defines that hold values for config registers, and prior to this patch omap_gpmc had code to read a config register (regardless of where it originally got set). > > It looked > > like you were removing the code that does dynamic detection, which would > > also affect non-SPL. > > > > > - /* If we are 16 bit dev, our gpmc config tells us that */ > > > - if ((readl(&gpmc_cfg->cs[cs].config1) & 0x3000) == 0x1000) > > omap_gpmc.c never had dynamic detection support. Above gpmc_config bit > which is used to tell whether device is x16 or x8, gets actually hard-coded in > gpmc_init(). Thus it was actually a mechanism to pass hard-coded bus-width > information to nand driver. > Refer: arch/arm/cpu/armv7/am33xx/mem.c : gpmc_init() > > So, instead of hacking the gpmc_init() everytime for different devices, > this patch introduces a generic CONFIG which can be used everywhere. It looks like you do more NAND config in gpmc_init() than just setting this one bit, so I don't think you save anything here. BTW, do you not need to set this bit in the config register for the hardware to work in the SPL case? -Scott _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot