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

[Vladimir Zapolskiy]

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 at 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 at 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.

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()

> 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.

--
With best wishes,
Vladimir