[U-Boot] [PATCH v2 1/5] at91: atmel_nand: extract HWECC initialization code into one function: atmel_hw_nand_init_param().

[Josh Wu]

Hi, Andreas

On 8/16/2012 4:56 PM, Andreas Bießmann wrote:
> Dear Josh Wu,
>
> On 16.08.2012 07:05, Josh Wu wrote:
>> Extract the hwecc initialization code into one function. It is a preparation for adding atmel PMECC support.
>>
>> Signed-off-by: Josh Wu <josh.wu at atmel.com>
>> ---
>>   drivers/mtd/nand/atmel_nand.c |  108 ++++++++++++++++++++++-------------------
>>   1 file changed, 57 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
>> index de66382..113da93 100644
>> --- a/drivers/mtd/nand/atmel_nand.c
>> +++ b/drivers/mtd/nand/atmel_nand.c
>> @@ -232,68 +232,19 @@ static int atmel_nand_correct(struct mtd_info *mtd, u_char *dat,
>>   static void atmel_nand_hwctl(struct mtd_info *mtd, int mode)
>>   {
>>   }
>> -#endif
>> -
>> -static void at91_nand_hwcontrol(struct mtd_info *mtd,
>> -					 int cmd, unsigned int ctrl)
>> -{
>> -	struct nand_chip *this = mtd->priv;
>> -
>> -	if (ctrl & NAND_CTRL_CHANGE) {
>> -		ulong IO_ADDR_W = (ulong) this->IO_ADDR_W;
>> -		IO_ADDR_W &= ~(CONFIG_SYS_NAND_MASK_ALE
>> -			     | CONFIG_SYS_NAND_MASK_CLE);
>> -
>> -		if (ctrl & NAND_CLE)
>> -			IO_ADDR_W |= CONFIG_SYS_NAND_MASK_CLE;
>> -		if (ctrl & NAND_ALE)
>> -			IO_ADDR_W |= CONFIG_SYS_NAND_MASK_ALE;
>> -
>> -#ifdef CONFIG_SYS_NAND_ENABLE_PIN
>> -		at91_set_gpio_value(CONFIG_SYS_NAND_ENABLE_PIN,
>> -				    !(ctrl & NAND_NCE));
>> -#endif
>> -		this->IO_ADDR_W = (void *) IO_ADDR_W;
>> -	}
>> -
>> -	if (cmd != NAND_CMD_NONE)
>> -		writeb(cmd, this->IO_ADDR_W);
>> -}
>>   
>> -#ifdef CONFIG_SYS_NAND_READY_PIN
>> -static int at91_nand_ready(struct mtd_info *mtd)
>> +int atmel_hw_nand_init_param(struct nand_chip *nand)
> Grr ... just realized your kernel patch has the same named function. I
> would have named it with 'ecc' in ... nevertheless I would accept this.

I think I can change the name to 'atmel_hwecc_nand_init_param'. it 
sounds better.

>
>>   {
>> -	return at91_get_gpio_value(CONFIG_SYS_NAND_READY_PIN);
>> -}
>> -#endif
>> -
>> -int board_nand_init(struct nand_chip *nand)
>> -{
>> -#ifdef CONFIG_ATMEL_NAND_HWECC
>>   	static int chip_nr = 0;
> This seems to be an remnant. It seems this is a mixture between
> 'SELF_INIT' and older initialization by common nand code. Can you please
> adopt to the CONFIG_SYS_NAND_SELFINIT? -> please read doc/REDME.nand
> If I got this correctly you just need to move this static chip_nr plus
> mtd detection (nand_scan_ident()) into the board_nand_init() and call it
> _always_. Next step is to define CONFIG_SYS_NAND_SELFINIT in
> include/nand.h and fix compiler issues (board_nand_init then has no
> parameter).

Thank you for point it out. I was not pay much attention in this part.
According to your suggestion and read the code and README.nand file, I 
think I will add a new function 'board_nand_init(void)',
which use to initialize hwecc & pmecc parameters.

>
> I dunno if this is ok to call the nand_scan_ident twice when hwecc is
> enabled. Scott, can you please comment?

With the above change, I can make sure only call nand_scan_ident once.

>>   	struct mtd_info *mtd;
>> -#endif
>> -
>> -	nand->ecc.mode = NAND_ECC_SOFT;
>> -#ifdef CONFIG_SYS_NAND_DBW_16
>> -	nand->options = NAND_BUSWIDTH_16;
>> -#endif
>> -	nand->cmd_ctrl = at91_nand_hwcontrol;
>> -#ifdef CONFIG_SYS_NAND_READY_PIN
>> -	nand->dev_ready = at91_nand_ready;
>> -#endif
>> -	nand->chip_delay = 20;
>>   
>> -#ifdef CONFIG_ATMEL_NAND_HWECC
>>   	nand->ecc.mode = NAND_ECC_HW;
>>   	nand->ecc.calculate = atmel_nand_calculate;
>>   	nand->ecc.correct = atmel_nand_correct;
>>   	nand->ecc.hwctl = atmel_nand_hwctl;
>>   	nand->ecc.read_page = atmel_nand_read_page;
>>   	nand->ecc.bytes = 4;
>> -#endif
>>   
>> -#ifdef CONFIG_ATMEL_NAND_HWECC
>>   	mtd = &nand_info[chip_nr++];
>>   	mtd->priv = nand;
> I think this is the most problematic part. mtd->priv was setup in
> nand_init_chip(int) before, so why rewrite it here? I know it was this
> way before ... I wonder if anybody is using atmel_nand with hwecc.

I agree. It seems those code should only for CONFIG_SYS_NAND_SELFINIT is 
defined. I will move this part of code to 'board_nand_init(void)'.

>
>> @@ -339,7 +290,62 @@ int board_nand_init(struct nand_chip *nand)
>>   			break;
>>   		}
>>   	}
>> -#endif
>>   
>>   	return 0;
>>   }
>> +
>> +#endif
>> +
>> +static void at91_nand_hwcontrol(struct mtd_info *mtd,
>> +					 int cmd, unsigned int ctrl)
>> +{
>> +	struct nand_chip *this = mtd->priv;
>> +
>> +	if (ctrl & NAND_CTRL_CHANGE) {
>> +		ulong IO_ADDR_W = (ulong) this->IO_ADDR_W;
>> +		IO_ADDR_W &= ~(CONFIG_SYS_NAND_MASK_ALE
>> +			     | CONFIG_SYS_NAND_MASK_CLE);
>> +
>> +		if (ctrl & NAND_CLE)
>> +			IO_ADDR_W |= CONFIG_SYS_NAND_MASK_CLE;
>> +		if (ctrl & NAND_ALE)
>> +			IO_ADDR_W |= CONFIG_SYS_NAND_MASK_ALE;
>> +
>> +#ifdef CONFIG_SYS_NAND_ENABLE_PIN
>> +		at91_set_gpio_value(CONFIG_SYS_NAND_ENABLE_PIN,
>> +				    !(ctrl & NAND_NCE));
>> +#endif
>> +		this->IO_ADDR_W = (void *) IO_ADDR_W;
>> +	}
>> +
>> +	if (cmd != NAND_CMD_NONE)
>> +		writeb(cmd, this->IO_ADDR_W);
>> +}
>> +
>> +#ifdef CONFIG_SYS_NAND_READY_PIN
>> +static int at91_nand_ready(struct mtd_info *mtd)
>> +{
>> +	return at91_get_gpio_value(CONFIG_SYS_NAND_READY_PIN);
>> +}
>> +#endif
>> +
>> +int board_nand_init(struct nand_chip *nand)
>> +{
>> +	int res = 0;
>> +
>> +	nand->ecc.mode = NAND_ECC_SOFT;
>> +#ifdef CONFIG_SYS_NAND_DBW_16
>> +	nand->options = NAND_BUSWIDTH_16;
>> +#endif
>> +	nand->cmd_ctrl = at91_nand_hwcontrol;
>> +#ifdef CONFIG_SYS_NAND_READY_PIN
>> +	nand->dev_ready = at91_nand_ready;
>> +#endif
>> +	nand->chip_delay = 20;
>> +
>> +#ifdef CONFIG_ATMEL_NAND_HWECC
>> +	res = atmel_hw_nand_init_param(nand);
>> +#endif
>> +
>> +	return res;
>> +}
>>
> Best regards
>
> Andreas Bießmann

Best Regards,
Josh Wu