[U-Boot] [PATCH 1/4] spl: nand: simple: replace readb() with chip specific read_buf()

[Albert ARIBAUD <albert.u.boot@aribaud.net>]

Hello Vladimir,

On Thu, 16 Jul 2015 14:31:26 +0300, Vladimir Zapolskiy <vz@mleia.com>
wrote:
> Hello Albert,
> 
> On 16.07.2015 11:02, Albert ARIBAUD wrote:
> > Hello Vladimir,
> > 
> > On Thu, 16 Jul 2015 02:33:45 +0300, Vladimir Zapolskiy <vz@mleia.com>
> > wrote:
> >> Some NAND controllers define custom functions to read data out,
> >> respect this in order to correctly support bad block handling in
> >> simple SPL NAND framework.
> >>
> >> NAND controller specific read_buf() is used even to read 1 byte in
> >> case of connected 8-bit NAND device, it turns out that read_byte()
> >> may become outdated.
> >>
> >> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
> >> ---
> >>  drivers/mtd/nand/nand_spl_simple.c | 7 +++++--
> >>  1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/nand_spl_simple.c b/drivers/mtd/nand/nand_spl_simple.c
> >> index 700ca32..e69f662 100644
> >> --- a/drivers/mtd/nand/nand_spl_simple.c
> >> +++ b/drivers/mtd/nand/nand_spl_simple.c
> >> @@ -115,6 +115,7 @@ static int nand_command(int block, int page, uint32_t offs,
> >>  static int nand_is_bad_block(int block)
> >>  {
> >>  	struct nand_chip *this = mtd.priv;
> >> +	u_char bb_data[2];
> >>  
> >>  	nand_command(block, 0, CONFIG_SYS_NAND_BAD_BLOCK_POS,
> >>  		NAND_CMD_READOOB);
> >> @@ -123,10 +124,12 @@ static int nand_is_bad_block(int block)
> >>  	 * Read one byte (or two if it's a 16 bit chip).
> >>  	 */
> >>  	if (this->options & NAND_BUSWIDTH_16) {
> >> -		if (readw(this->IO_ADDR_R) != 0xffff)
> >> +		this->read_buf(&mtd, bb_data, 2);
> >> +		if (bb_data[0] != 0xff || bb_data[1] != 0xff)
> >>  			return 1;
> >>  	} else {
> >> -		if (readb(this->IO_ADDR_R) != 0xff)
> >> +		this->read_buf(&mtd, bb_data, 1);
> >> +		if (bb_data[0] != 0xff)
> >>  			return 1;
> >>  	}
> >>  
> >> -- 
> >> 2.1.4
> >>
> > 
> > The way you describe this patch, it looks like a bug that should have
> > bitten way more boards than lpc32xx. Did you have a look at some other
> > boards to see why this did not affect them?
> 
> Yes, it is a bug IMHO.

If it is, then it has hit no board which defines CONFIG_SPL_NAND_SIMPLE
and we should understand why.

> Grepping for CONFIG_SPL_NAND_SIMPLE I see that TI and Tegra boards may
> be impacted (positively or negatively):
> 
> * CONFIG_NAND_OMAP_GPMC --- own .read_buf(), default .read_byte()
> * CONFIG_NAND_DAVINCI   --- own .read_buf(), default .read_byte()
> * CONFIG_TEGRA_NAND     --- own .read_byte(), own .read_buf()

They may be impacted by your change, but they are working now -- they
are not obscure models. Let's dig a bit...

> > Conversively, what is the actual failure scenario that led you to 
> > writing this patch?
> 
> The failure scenario is quite simple, the ARM core gets stuck on first
> attempt to access SLC NAND data register -- traced with JTAG.
> 
> The same happens, if I remove custom .read_byte()/.read_buf() from SLC
> NAND driver. The only difference between custom .read_byte() and shared
> read_byte() is in readb()/readl() access to the data register, it is
> essential to have only 32-bit wide access to SLC NAND registers.

README describes CONFIG_SPL_NAND_SIMPLE as "Support for NAND boot using
simple NAND drivers that expose the cmd_ctrl() interface". The cmd_ctrl
interface actually contains the cmd_ctrl() function *and* the
IO_ADDR_[RW] fields. IOW, a simple NAND driver provides byte or word
access to the NAND's I/O lines on which command, address and data are
passed. If the NAND is 8 bits, then there are 8 lines; if it is 16
bit, then there are 16 lines. 

I reckon that the OMAP/DAVINCI and TEGRA simple drivers just do that:
set IO_ADDR_[RW] to the IP register through which direct access to the
NAND's I/O lines can be performed, byte or word depending on the chip
width.

As such, there is no bug in the way nand_simple.c uses IO_ADDR_[RW].

OTOH, the SLC IP does not provide direct access to the NAND I/O lines
through a general register; what it provides is a set of three
specialized registers one for commands, one for addresses and one
for data. Plus, even though only 8 bit NANDs are supported, the data
register does not physically support 8-bit wide accesses (NXP: I am
still struggling to understand what you were trying to achieve there).

All this basically makes the SLC controller a 'not simple NAND IP', and
its driver a 'not simple NAND driver' incompatible with nand_simple.c.

Your solution to this incompatibility is to change nand_simple.c to
support other types of drivers; but by doing that, you're changing the
API between nand_simple.c and all simple drivers, possibly changing the
established behavior.

I personally don't think this is the right way; nand_simple.c should be
left unchanged and the board should simply not use it since it does not
have a simple NAND controller, and instead it should provide its own
nand_spl_load_image().

But hey, I'm not then NAND custodian. Scott: your call. :)

> --
> With best wishes,
> Vladimir

Amicalement,
-- 
Albert.