[U-Boot] [PATCH v2 2/2] mtd: nand: mxc_nand: Fix is_16bit_nand()

[Benoît Thébaudeau]

Hi Fabio,

On Tuesday, February 26, 2013 7:35:20 PM, Fabio Estevam wrote:
> Currently is_16bit_nand() is a per SoC function and it decides the bus nand
> width by reading some boot related registers.
> 
> This method works when NAND is the boot medium, but does not work if another
> boot medium is used. For example: booting from a SD card and then using NAND
> to store the environment variables, would lead to the following error:
> 
> NAND bus width 16 instead 8 bit
> No NAND device found!!!
> 0 MiB
> 
> Use CONFIG_SYS_NAND_BUSWIDTH_16BIT symbol to decide the bus width.
> 
> If it is defined in the board file, then consider 16-bit NAND bus-width,
> otherwise assume 8-bit NAND is used.
> 
> This also aligns with Documentation/devicetree/bindings/mtd/nand.txt, which
> states:
> 
> nand-bus-width : 8 or 16 bus width if not present 82
> 
> Signed-off-by: Fabio Estevam <fabio.estevam at freescale.com>
> ---
> Changes since v1:
> - Add an entry into README
>  README                      |    3 ++-
>  drivers/mtd/nand/mxc_nand.c |   37 +++----------------------------------
>  2 files changed, 5 insertions(+), 35 deletions(-)
> 
> diff --git a/README b/README
> index bdd2c81..1a6a7a5 100644
> --- a/README
> +++ b/README
> @@ -3717,8 +3717,9 @@ Low Level (hardware related) configuration options:
>  		Defined to tell the NAND controller that the NAND chip is using
>   		a 16 bit bus.
>  		Not all NAND drivers use this symbol.
> -		Example of driver that uses it:
> +		Example of drivers that use it:
>  		- drivers/mtd/nand/ndfc.c
> +		- drivers/mtd/nand/mxc_nand.c
>  
>  - CONFIG_SYS_NDFC_EBC0_CFG
>  		Sets the EBC0_CFG register for the NDFC. If not defined
> diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> index d0ded48..bb475f2 100644
> --- a/drivers/mtd/nand/mxc_nand.c
> +++ b/drivers/mtd/nand/mxc_nand.c
> @@ -98,45 +98,14 @@ static struct nand_ecclayout nand_hw_eccoob2k = {
>  #endif
>  #endif
>  
> -#ifdef CONFIG_MX27
>  static int is_16bit_nand(void)
>  {
> -	struct system_control_regs *sc_regs =
> -		(struct system_control_regs *)IMX_SYSTEM_CTL_BASE;
> -
> -	if (readl(&sc_regs->fmcr) & NF_16BIT_SEL)
> -		return 1;
> -	else
> -		return 0;
> -}
> -#elif defined(CONFIG_MX31)
> -static int is_16bit_nand(void)
> -{
> -	struct clock_control_regs *sc_regs =
> -		(struct clock_control_regs *)CCM_BASE;
> -
> -	if (readl(&sc_regs->rcsr) & CCM_RCSR_NF16B)
> -		return 1;
> -	else
> -		return 0;
> -}
> -#elif defined(CONFIG_MX25) || defined(CONFIG_MX35)
> -static int is_16bit_nand(void)
> -{
> -	struct ccm_regs *ccm = (struct ccm_regs *)IMX_CCM_BASE;
> -
> -	if (readl(&ccm->rcsr) & CCM_RCSR_NF_16BIT_SEL)
> -		return 1;
> -	else
> -		return 0;
> -}
> +#if defined(CONFIG_SYS_NAND_BUSWIDTH_16BIT)
> +	return 1;
>  #else
> -#warning "8/16 bit NAND autodetection not supported"
> -static int is_16bit_nand(void)
> -{
>  	return 0;
> -}
>  #endif
> +}
>  
>  static uint32_t *mxc_nand_memcpy32(uint32_t *dest, uint32_t *source, size_t
>  size)
>  {

That's correct.

is_16bit_nand() is only used to conditionally set NAND_BUSWIDTH_16 in
this->options, then this flag is tested everywhere else, so we could also
completely drop is_16bit_nand() and just do:
#ifdef CONFIG_SYS_NAND_BUSWIDTH_16BIT
	this->options |= NAND_BUSWIDTH_16;
#endif

What do you think is better? I'm fine with your implementation.

Should I rebase my series on that, keeping both series separate, or should I
integrate these 2 patches as is at the beginning of my series?

Best regards,
Benoît