[U-Boot] [PATCH 1/1] am33xx: add CONFIG_SYS_NAND_DEVICE_WIDTH to determine NAND device bus-width

[Gupta, Pekon]

> From: Scott Wood [mailto:scottwood at freescale.com]
> 
> On Fri, 2013-09-27 at 04:18 +0000, Gupta, Pekon wrote:
> > > From: Scott Wood [mailto:scottwood at freescale.com]
> > >
> > > On Thu, 2013-09-26 at 13:14 +0000, Gupta, Pekon wrote:
> > > > > > From: Gupta, Pekon <pekon at ti.com>
> > > > > >
[snip]
> 
> 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.
> 
Oh  I missed referring to nand_spl_simple.c.
Thanks for pointing out. Yes, we already have a generic SPL driver.


> > > > 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).
> 
(1) drivers/mtd/nand/fsl_ifc_spl.c
They are doing same way as OMAP used to. They are also using controller
configurations to tell driver about the]NAND bus-width 
"port_size = (cspr & CSPR_PORT_SIZE_16) ? 16 : 8;"

(2) drivers/mtd/nand/fsl_elbc_spl.c
They are doing incomplete check. Rather they are not caring for x16 device
For a x16 device, badblock marker should be 16-bit (2 bytes) but here they
are checking of only 1st byte.
"if (i++ < 2 && buf[page_offs + bad_marker] != 0xff)"

So CONFIG_SYS_NAND_DEVICE_WIDTH should help them also. right ?
So can this new CONFIG_xx be accepted ?


> > > 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?
> 
Yes, I'm not changing the default configs for GPMC in gpmc_init(), 
because they are ok for x8 device. I'm just overriding them again during
board_nand_init() if CONFIG_SYS_NAND_DEVICE_WIDTH == x16 device.

+	/* reconfigure GPMC.CONFIG1 register with correct device-width */
+	gpmc_config = readl(&gpmc_cfg->cs[cs].config1);
+	if (nand->options & NAND_BUSWIDTH_16)
+		gpmc_config |= (0x1 << 12);
+	else
+		gpmc_config &= ~(0x1 << 12);
+	writel(gpmc_config, &gpmc_cfg->cs[cs].config1);
+
This way im not breaking any backward compatibility, just avoiding
the need to hack the board file for x16 devices.

with regards, pekon