[U-Boot] [PATCH 2/3] mtd: nand: atmel: prepare for nand spl boot support

[Bo Shen]

Hi Scott,

On 02/28/2014 08:35 AM, Scott Wood wrote:
> On Mon, 2013-12-02 at 11:24 +0800, Bo Shen wrote:
>> Prepare for nand spl boot support. It supports nand software ECC and
>> hardware PMECC.
>> This patch is take <drivers/mtd/nand/nand_spl_simple.c> as reference.
>>
>> Signed-off-by: Bo Shen <voice.shen at atmel.com>
>> ---
>>   drivers/mtd/nand/atmel_nand.c |  206 +++++++++++++++++++++++++++++++++++++++++
>>   include/nand.h                |    6 ++
>>   2 files changed, 212 insertions(+)
>
> Looks OK but some style nits:
>
>> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
>> index da83f06..64e11e1 100644
>> --- a/drivers/mtd/nand/atmel_nand.c
>> +++ b/drivers/mtd/nand/atmel_nand.c
>> @@ -32,6 +32,12 @@
>>
>>   #ifdef CONFIG_ATMEL_NAND_HW_PMECC
>>
>> +#ifdef CONFIG_SPL_BUILD
>> +#ifdef CONFIG_SYS_NAND_ONFI_DETECTION
>> +#undef CONFIG_SYS_NAND_ONFI_DETECTION
>> +#endif
>> +#endif
>
> There's no need to ifdef before undeffing.

I will remove the ifdef.

>>   struct atmel_nand_host {
>>   	struct pmecc_regs __iomem *pmecc;
>>   	struct pmecc_errloc_regs __iomem *pmerrloc;
>> @@ -1163,6 +1169,8 @@ static int at91_nand_ready(struct mtd_info *mtd)
>>   }
>>   #endif
>>
>> +#ifndef CONFIG_SPL_BUILD
>
> Use ifdef rather than ifndef for if/else.

OK, I will use ifdef instead of ifndef.

>> +#ifdef CONFIG_SPL_NAND_SOFTECC
>
> This symbol needs to be documented (I realize it isn't new).

OK, I will document it.

>> +static int nand_ecc_pos[] = CONFIG_SYS_NAND_ECCPOS;
>> +#define ECCSTEPS (CONFIG_SYS_NAND_PAGE_SIZE / \
>> +		  CONFIG_SYS_NAND_ECCSIZE)
>> +#define ECCTOTAL (ECCSTEPS * CONFIG_SYS_NAND_ECCBYTES)
>> +#endif
>
> Is this stuff used anywhere but nand_read_page()?  If not, move it
> there (as local variables, not #defines).

OK, I will move where  use it.

>> +static int nand_read_page(int block, int page, void *dst)
>> +{
>> +	struct nand_chip *this = mtd.priv;
>> +#ifdef CONFIG_SPL_NAND_SOFTECC
>> +	u_char ecc_calc[ECCTOTAL];
>> +	u_char ecc_code[ECCTOTAL];
>> +	u_char oob_data[CONFIG_SYS_NAND_OOBSIZE];
>> +	int eccsize = CONFIG_SYS_NAND_ECCSIZE;
>> +	int eccbytes = CONFIG_SYS_NAND_ECCBYTES;
>> +	int eccsteps = ECCSTEPS;
>> +	int i;
>> +	uint8_t *p = dst;
>> +#endif
>> +	nand_command(block, page, 0, NAND_CMD_READ0);
>> +
>> +#ifdef CONFIG_SPL_NAND_SOFTECC
>> +	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
>> +		if (this->ecc.mode != NAND_ECC_SOFT)
>> +			this->ecc.hwctl(&mtd, NAND_ECC_READ);
>> +		this->read_buf(&mtd, p, eccsize);
>> +		this->ecc.calculate(&mtd, p, &ecc_calc[i]);
>> +	}
>> +	this->read_buf(&mtd, oob_data, CONFIG_SYS_NAND_OOBSIZE);
>> +
>> +	for (i = 0; i < ECCTOTAL; i++)
>> +		ecc_code[i] = oob_data[nand_ecc_pos[i]];
>> +
>> +	eccsteps = ECCSTEPS;
>> +	p = dst;
>> +
>> +	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize)
>> +		this->ecc.correct(&mtd, p, &ecc_code[i], &ecc_calc[i]);
>> +#else
>> +	atmel_nand_pmecc_read_page(&mtd, this, dst, 0, page);
>> +#endif
>> +
>> +	return 0;
>> +}
>
> These don't share enough code to warrant interleaving like this -- just
> have one big ifdef/else.  It will be more readable.

I will change in next version.

> -Scott
>
>

Best Regards,
Bo Shen