From: Scott Wood [mailto:scottwood@freescale.com]
On Fri, 2013-09-27 at 04:18 +0000, Gupta, Pekon wrote:
From: Scott Wood [mailto:scottwood@freescale.com]
On Thu, 2013-09-26 at 13:14 +0000, Gupta, Pekon wrote:
From: Gupta, Pekon pekon@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