[U-Boot] [PATCH v2 1/2] mtd: nand: Introduce CONFIG_SYS_NAND_BUSWIDTH_16BIT

[U-Boot] [PATCH v2 1/2] mtd: nand: Introduce CONFIG_SYS_NAND_BUSWIDTH_16BIT

[Fabio Estevam]

Introduce CONFIG_SYS_NAND_BUSWIDTH_16BIT option so that other NAND controller 
drivers could use it when a 16-bit NAND is deployed.

drivers/mtd/nand/ndfc has CONFIG_SYS_NDFC_16BIT, so just rename it, so that
other NAND drivers could reuse the same symbol.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Changes since v1:
- Improve README

 README                  |    9 ++++++---
 drivers/mtd/nand/ndfc.c |    4 ++--
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/README b/README
index d8cb394..bdd2c81 100644
--- a/README
+++ b/README
@@ -3713,9 +3713,12 @@ Low Level (hardware related) configuration options:
 - CONFIG_SYS_SRIOn_MEM_SIZE:
 		Size of SRIO port 'n' memory region
 
-- CONFIG_SYS_NDFC_16
-		Defined to tell the NDFC that the NAND chip is using a
-		16 bit bus.
+- CONFIG_SYS_NAND_BUSWIDTH_16BIT
+		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:
+		- drivers/mtd/nand/ndfc.c
 
 - CONFIG_SYS_NDFC_EBC0_CFG
 		Sets the EBC0_CFG register for the NDFC. If not defined
diff --git a/drivers/mtd/nand/ndfc.c b/drivers/mtd/nand/ndfc.c
index 6ebbb5e..213d2c9 100644
--- a/drivers/mtd/nand/ndfc.c
+++ b/drivers/mtd/nand/ndfc.c
@@ -156,7 +156,7 @@ static uint8_t ndfc_read_byte(struct mtd_info *mtd)
 
 	struct nand_chip *chip = mtd->priv;
 
-#ifdef CONFIG_SYS_NDFC_16BIT
+#ifdef CONFIG_SYS_NAND_BUSWIDTH_16BIT
 	return (uint8_t) readw(chip->IO_ADDR_R);
 #else
 	return readb(chip->IO_ADDR_R);
@@ -218,7 +218,7 @@ int board_nand_init(struct nand_chip *nand)
 	nand->ecc.bytes = 3;
 	nand->select_chip = ndfc_select_chip;
 
-#ifdef CONFIG_SYS_NDFC_16BIT
+#ifdef CONFIG_SYS_NAND_BUSWIDTH_16BIT
 	nand->options |= NAND_BUSWIDTH_16;
 #endif
 
-- 
1.7.9.5

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

[Fabio Estevam]

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@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)
 {
-- 
1.7.9.5

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

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

[Fabio Estevam]

On Tue, Feb 26, 2013 at 3:47 PM, Beno?t Th?baudeau
<benoit.thebaudeau@advansee.com> wrote:

> 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

Yes, this could be done.

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

My main goal with this patch series was to fix a bug, and I think I
can do as you suggest, and get rid of is_16bit_nand() in a future
patch.

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

I would say that you could send this series as is at the beginning of
your next version in order to avoid conflicts.

Thanks,

Fabio Estevam

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

[Eric Bénard]

Hi Fabio,

Le Tue, 26 Feb 2013 15:35:20 -0300,
Fabio Estevam <fabio.estevam@freescale.com> a ?crit :

> 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
> 
are you sure that your patch won't break current boards booting on nand
flash (and so which can use the current functions to detect the nand
width) ?

Eric

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

[Benoît Thébaudeau]

Hi Eric,

On Wednesday, February 27, 2013 12:11:04 PM, Eric B?nard wrote:
> Hi Fabio,
> 
> Le Tue, 26 Feb 2013 15:35:20 -0300,
> Fabio Estevam <fabio.estevam@freescale.com> a ?crit :
> 
> > 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
> > 
> are you sure that your patch won't break current boards booting on nand
> flash (and so which can use the current functions to detect the nand
> width) ?

This code is not used for NAND boot, for which the SPL version of this driver
and CONFIG_SYS_NAND_BUSWIDTH_16 are used.

Best regards,
Beno?t

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

[Eric Bénard]

Hi Beno?t,

Le Wed, 27 Feb 2013 13:53:10 +0100 (CET),
Beno?t Th?baudeau <benoit.thebaudeau@advansee.com> a ?crit :
> 
> This code is not used for NAND boot, for which the SPL version of this driver
> and CONFIG_SYS_NAND_BUSWIDTH_16 are used.
> 
I didn't follow SPL migration (I have to come back to it so the next
question may be stupid sorry in advance) but once u-boot is running
isn't it using the "standard" driver and no more the SPL one ?

Eric

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

[Fabio Estevam]

Hi Eric,

On Wed, Feb 27, 2013 at 8:11 AM, Eric B?nard <eric@eukrea.com> wrote:

> are you sure that your patch won't break current boards booting on nand
> flash (and so which can use the current functions to detect the nand
> width) ?

If a board is booting from NAND, then it is required that the boot
jumpers are correctly set to specify NAND boot mode and the NAND type
(8-bit versus 16-bit width, for example).

I revisited all the i.mx boards that use MXC_NAND driver and could not
see anyone using 16-bit NAND (ie, no 16-bit NAND IOMUX setting), so it
is safe to assume that all the current boards are using 8-bit width
NAND.

If a board has 16-bit width NAND, then the boot jumpers need to
reflect that and CONFIG_SYS_NAND_BUSWIDTH_16BIT must be specified in
the config file.

So I don't envision any breakage here.

Regards,

Fabio Estevam

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

[Benoît Thébaudeau]

Hi Eric,

On Wednesday, February 27, 2013 2:15:21 PM, Eric B?nard wrote:
> Hi Beno?t,
> 
> Le Wed, 27 Feb 2013 13:53:10 +0100 (CET),
> Beno?t Th?baudeau <benoit.thebaudeau@advansee.com> a ?crit :
> > 
> > This code is not used for NAND boot, for which the SPL version of this
> > driver
> > and CONFIG_SYS_NAND_BUSWIDTH_16 are used.
> > 
> I didn't follow SPL migration (I have to come back to it so the next
> question may be stupid sorry in advance) but once u-boot is running
> isn't it using the "standard" driver and no more the SPL one ?

Yes, that's correct (unless you need to access NAND only from SPL). But if any
board booting from NAND had a 16-bit NAND, CONFIG_SYS_NAND_BUSWIDTH_16 would
have to be defined, which no board does, so none of these boards needs
CONFIG_SYS_NAND_BUSWIDTH_16BIT either. And the rest of the explanation is what
Fabio has just said.

Best regards,
Beno?t

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

[Eric Bénard]

Hi Beno?t and Fabio,

Le Wed, 27 Feb 2013 14:40:51 +0100 (CET),
Beno?t Th?baudeau <benoit.thebaudeau@advansee.com> a ?crit :
> On Wednesday, February 27, 2013 2:15:21 PM, Eric B?nard wrote:
> > Hi Beno?t,
> > 
> > Le Wed, 27 Feb 2013 13:53:10 +0100 (CET),
> > Beno?t Th?baudeau <benoit.thebaudeau@advansee.com> a ?crit :
> > > 
> > > This code is not used for NAND boot, for which the SPL version of this
> > > driver
> > > and CONFIG_SYS_NAND_BUSWIDTH_16 are used.
> > > 
> > I didn't follow SPL migration (I have to come back to it so the next
> > question may be stupid sorry in advance) but once u-boot is running
> > isn't it using the "standard" driver and no more the SPL one ?
> 
> Yes, that's correct (unless you need to access NAND only from SPL). But if any
> board booting from NAND had a 16-bit NAND, CONFIG_SYS_NAND_BUSWIDTH_16 would
> have to be defined, which no board does, so none of these boards needs
> CONFIG_SYS_NAND_BUSWIDTH_16BIT either. And the rest of the explanation is what
> Fabio has just said.
> 
thanks to both of you for the details.

Eric