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

[Andreas Bießmann]

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.

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

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

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

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