[U-Boot] [PATCH] ARM DaVinci: Adding CONFIG options for NAND ALE and CLE

[U-Boot] [PATCH] ARM DaVinci: Adding CONFIG options for NAND ALE and CLE

[s-paulraj at ti.com]

The CLE and ALE for DaVinci DM644x is not the same as DM646x. This patch
adds 2 CONFIG_ options to the config.h header files to all the DM6446 based
boards. These values are then used by the driver. This has been tested on the
DM644x, DM355, DM357 and DM365. Support for the latter 3 will be added soon.

Signed-off-by: Sandeep Paulraj <s-paulraj@ti.com>
---
 drivers/mtd/nand/davinci_nand.c          |    6 +++---
 include/asm-arm/arch-davinci/nand_defs.h |    9 ++++-----
 include/configs/davinci_dvevm.h          |    2 ++
 include/configs/davinci_schmoogie.h      |    2 ++
 include/configs/davinci_sffsdr.h         |    2 ++
 include/configs/davinci_sonata.h         |    2 ++
 6 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
index a974667..dcc0f39 100644
--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -54,13 +54,13 @@ static void nand_davinci_hwcontrol(struct mtd_info *mtd, int cmd, unsigned int c
 	struct		nand_chip *this = mtd->priv;
 	u_int32_t	IO_ADDR_W = (u_int32_t)this->IO_ADDR_W;
 
-	IO_ADDR_W &= ~(MASK_ALE|MASK_CLE);
+	IO_ADDR_W &= ~(CONFIG_MASK_ALE | CONFIG_MASK_CLE);
 
 	if (ctrl & NAND_CTRL_CHANGE) {
 		if ( ctrl & NAND_CLE )
-			IO_ADDR_W |= MASK_CLE;
+			IO_ADDR_W |= CONFIG_MASK_CLE;
 		if ( ctrl & NAND_ALE )
-			IO_ADDR_W |= MASK_ALE;
+			IO_ADDR_W |= CONFIG_MASK_ALE;
 		this->IO_ADDR_W = (void __iomem *) IO_ADDR_W;
 	}
 
diff --git a/include/asm-arm/arch-davinci/nand_defs.h b/include/asm-arm/arch-davinci/nand_defs.h
index 187d3c3..a2d01b0 100644
--- a/include/asm-arm/arch-davinci/nand_defs.h
+++ b/include/asm-arm/arch-davinci/nand_defs.h
@@ -28,11 +28,10 @@
 
 #include <asm/arch/hardware.h>
 
-#define	MASK_CLE	0x10
-#define	MASK_ALE	0x0a
-
-#define NAND_CE0CLE	((volatile u_int8_t *)(CONFIG_SYS_NAND_BASE + 0x10))
-#define NAND_CE0ALE	((volatile u_int8_t *)(CONFIG_SYS_NAND_BASE + 0x0a))
+#define NAND_CE0CLE	((volatile u_int8_t *)(CONFIG_SYS_NAND_BASE + \
+				CONFIG_MASK_CLE))
+#define NAND_CE0ALE	((volatile u_int8_t *)(CONFIG_SYS_NAND_BASE + \
+				CONFIG_MASK_ALE))
 #define NAND_CE0DATA	((volatile u_int8_t *)CONFIG_SYS_NAND_BASE)
 
 typedef struct  {
diff --git a/include/configs/davinci_dvevm.h b/include/configs/davinci_dvevm.h
index fae430b..1fe3e19 100644
--- a/include/configs/davinci_dvevm.h
+++ b/include/configs/davinci_dvevm.h
@@ -128,6 +128,8 @@
 #define CONFIG_SYS_NAND_BASE		0x02000000
 #define CONFIG_SYS_NAND_HW_ECC
 #define CONFIG_SYS_MAX_NAND_DEVICE	1	/* Max number of NAND devices */
+#define	CONFIG_MASK_CLE			0x10
+#define	CONFIG_MASK_ALE			0x0a
 #define CONFIG_ENV_OFFSET		0x0	/* Block 0--not used by bootcode */
 #define DEF_BOOTM		""
 #elif defined(CONFIG_SYS_USE_NOR)
diff --git a/include/configs/davinci_schmoogie.h b/include/configs/davinci_schmoogie.h
index 923e477..40df3c3 100644
--- a/include/configs/davinci_schmoogie.h
+++ b/include/configs/davinci_schmoogie.h
@@ -90,6 +90,8 @@
 #define CONFIG_SYS_NAND_BASE		0x02000000
 #define CONFIG_SYS_NAND_HW_ECC
 #define CONFIG_SYS_MAX_NAND_DEVICE	1	/* Max number of NAND devices */
+#define	CONFIG_MASK_CLE			0x10
+#define	CONFIG_MASK_ALE			0x0a
 #define CONFIG_ENV_OFFSET		0x0	/* Block 0--not used by bootcode */
 /*=====================*/
 /* Board related stuff */
diff --git a/include/configs/davinci_sffsdr.h b/include/configs/davinci_sffsdr.h
index 73a59db..5d693f8 100644
--- a/include/configs/davinci_sffsdr.h
+++ b/include/configs/davinci_sffsdr.h
@@ -86,6 +86,8 @@
 #define CONFIG_SYS_NAND_BASE		0x02000000
 #define CONFIG_SYS_NAND_HW_ECC
 #define CONFIG_SYS_MAX_NAND_DEVICE	1	/* Max number of NAND devices */
+#define	CONFIG_MASK_CLE			0x10
+#define	CONFIG_MASK_ALE			0x0a
 #define CONFIG_ENV_OFFSET		0x0	/* Block 0--not used by bootcode */
 /* I2C switch definitions for PCA9543 chip */
 #define CONFIG_SYS_I2C_PCA9543_ADDR		0x70
diff --git a/include/configs/davinci_sonata.h b/include/configs/davinci_sonata.h
index 70d2c7d..6699b3a 100644
--- a/include/configs/davinci_sonata.h
+++ b/include/configs/davinci_sonata.h
@@ -123,6 +123,8 @@
 #define CONFIG_SYS_NAND_BASE		0x02000000
 #define CONFIG_SYS_NAND_HW_ECC
 #define CONFIG_SYS_MAX_NAND_DEVICE	1	/* Max number of NAND devices */
+#define	CONFIG_MASK_CLE			0x10
+#define	CONFIG_MASK_ALE			0x0a
 #define CONFIG_ENV_OFFSET		0x0	/* Block 0--not used by bootcode */
 #define DEF_BOOTM		""
 #elif defined(CONFIG_SYS_USE_NOR)
-- 
1.6.0.4

[U-Boot] [PATCH] ARM DaVinci: Adding CONFIG options for NAND ALE and CLE

[Steve Chen]

On Tue, 2009-04-28 at 16:33 -0400, s-paulraj at ti.com wrote:
> The CLE and ALE for DaVinci DM644x is not the same as DM646x. This patch
> adds 2 CONFIG_ options to the config.h header files to all the DM6446 based
> boards. These values are then used by the driver. This has been tested on the
> DM644x, DM355, DM357 and DM365. Support for the latter 3 will be added soon.
> 
> Signed-off-by: Sandeep Paulraj <s-paulraj@ti.com>
> ---
>  drivers/mtd/nand/davinci_nand.c          |    6 +++---
>  include/asm-arm/arch-davinci/nand_defs.h |    9 ++++-----
>  include/configs/davinci_dvevm.h          |    2 ++
>  include/configs/davinci_schmoogie.h      |    2 ++
>  include/configs/davinci_sffsdr.h         |    2 ++
>  include/configs/davinci_sonata.h         |    2 ++
>  6 files changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
> index a974667..dcc0f39 100644
> --- a/drivers/mtd/nand/davinci_nand.c
> +++ b/drivers/mtd/nand/davinci_nand.c
> @@ -54,13 +54,13 @@ static void nand_davinci_hwcontrol(struct mtd_info *mtd, int cmd, unsigned int c
>  	struct		nand_chip *this = mtd->priv;
>  	u_int32_t	IO_ADDR_W = (u_int32_t)this->IO_ADDR_W;
>  
> -	IO_ADDR_W &= ~(MASK_ALE|MASK_CLE);
> +	IO_ADDR_W &= ~(CONFIG_MASK_ALE | CONFIG_MASK_CLE);
>  
>  	if (ctrl & NAND_CTRL_CHANGE) {
>  		if ( ctrl & NAND_CLE )
> -			IO_ADDR_W |= MASK_CLE;
> +			IO_ADDR_W |= CONFIG_MASK_CLE;
>  		if ( ctrl & NAND_ALE )
> -			IO_ADDR_W |= MASK_ALE;
> +			IO_ADDR_W |= CONFIG_MASK_ALE;

When I modified this code in the MV kernel, I remember having to use +=
and -= instead of |= and &=.  The reason is tha DM6467 CLE/ALE mask has
large enough that the bits overlaps IO_ADDR_W.

[U-Boot] [PATCH] ARM DaVinci: Adding CONFIG options for NAND ALE and CLE

[Paulraj, Sandeep]

Steve,
      My intention was to change that when I add the DM6467 Support which I am working on even as we speak.

I am aware of it.

Thanks,
Sandeep

> -----Original Message-----
> From: Steve Chen [mailto:schen at mvista.com]
> Sent: Tuesday, April 28, 2009 4:54 PM
> To: Paulraj, Sandeep
> Cc: u-boot at lists.denx.de; davinci-linux-open-source at linux.davincidsp.com
> Subject: Re: [PATCH] ARM DaVinci: Adding CONFIG options for NAND ALE and
> CLE
> 
> On Tue, 2009-04-28 at 16:33 -0400, s-paulraj at ti.com wrote:
> > The CLE and ALE for DaVinci DM644x is not the same as DM646x. This patch
> > adds 2 CONFIG_ options to the config.h header files to all the DM6446
> based
> > boards. These values are then used by the driver. This has been tested
> on the
> > DM644x, DM355, DM357 and DM365. Support for the latter 3 will be added
> soon.
> >
> > Signed-off-by: Sandeep Paulraj <s-paulraj@ti.com>
> > ---
> >  drivers/mtd/nand/davinci_nand.c          |    6 +++---
> >  include/asm-arm/arch-davinci/nand_defs.h |    9 ++++-----
> >  include/configs/davinci_dvevm.h          |    2 ++
> >  include/configs/davinci_schmoogie.h      |    2 ++
> >  include/configs/davinci_sffsdr.h         |    2 ++
> >  include/configs/davinci_sonata.h         |    2 ++
> >  6 files changed, 15 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/davinci_nand.c
> b/drivers/mtd/nand/davinci_nand.c
> > index a974667..dcc0f39 100644
> > --- a/drivers/mtd/nand/davinci_nand.c
> > +++ b/drivers/mtd/nand/davinci_nand.c
> > @@ -54,13 +54,13 @@ static void nand_davinci_hwcontrol(struct mtd_info
> *mtd, int cmd, unsigned int c
> >  	struct		nand_chip *this = mtd->priv;
> >  	u_int32_t	IO_ADDR_W = (u_int32_t)this->IO_ADDR_W;
> >
> > -	IO_ADDR_W &= ~(MASK_ALE|MASK_CLE);
> > +	IO_ADDR_W &= ~(CONFIG_MASK_ALE | CONFIG_MASK_CLE);
> >
> >  	if (ctrl & NAND_CTRL_CHANGE) {
> >  		if ( ctrl & NAND_CLE )
> > -			IO_ADDR_W |= MASK_CLE;
> > +			IO_ADDR_W |= CONFIG_MASK_CLE;
> >  		if ( ctrl & NAND_ALE )
> > -			IO_ADDR_W |= MASK_ALE;
> > +			IO_ADDR_W |= CONFIG_MASK_ALE;
> 
> When I modified this code in the MV kernel, I remember having to use +=
> and -= instead of |= and &=.  The reason is tha DM6467 CLE/ALE mask has
> large enough that the bits overlaps IO_ADDR_W.
> 
> 
> 

[U-Boot] [PATCH] ARM DaVinci: Adding CONFIG options for NAND ALE and CLE

[Scott Wood]

On Tue, Apr 28, 2009 at 04:33:14PM -0400, s-paulraj at ti.com wrote:
> diff --git a/include/configs/davinci_dvevm.h b/include/configs/davinci_dvevm.h
> index fae430b..1fe3e19 100644
> --- a/include/configs/davinci_dvevm.h
> +++ b/include/configs/davinci_dvevm.h
> @@ -128,6 +128,8 @@
>  #define CONFIG_SYS_NAND_BASE		0x02000000
>  #define CONFIG_SYS_NAND_HW_ECC
>  #define CONFIG_SYS_MAX_NAND_DEVICE	1	/* Max number of NAND devices */
> +#define	CONFIG_MASK_CLE			0x10
> +#define	CONFIG_MASK_ALE			0x0a

Make it CONFIG_SYS_DAVINCI_NAND_CLE/ALE.

Also, no tab after #define.

-Scott

[U-Boot] [PATCH] ARM DaVinci: Adding CONFIG options for NAND ALE and CLE

[Paulraj, Sandeep]

Scott,
       I will resubmit after i take care of your comments.

Thanks,
Sandeep
________________________________________
From: Scott Wood [scottwood at freescale.com]
Sent: Tuesday, April 28, 2009 5:22 PM
To: Paulraj, Sandeep
Cc: u-boot at lists.denx.de; davinci-linux-open-source at linux.davincidsp.com
Subject: Re: [U-Boot] [PATCH] ARM DaVinci: Adding CONFIG options for NAND ALE and CLE

On Tue, Apr 28, 2009 at 04:33:14PM -0400, s-paulraj at ti.com wrote:
> diff --git a/include/configs/davinci_dvevm.h b/include/configs/davinci_dvevm.h
> index fae430b..1fe3e19 100644
> --- a/include/configs/davinci_dvevm.h
> +++ b/include/configs/davinci_dvevm.h
> @@ -128,6 +128,8 @@
>  #define CONFIG_SYS_NAND_BASE         0x02000000
>  #define CONFIG_SYS_NAND_HW_ECC
>  #define CONFIG_SYS_MAX_NAND_DEVICE   1       /* Max number of NAND devices */
> +#define      CONFIG_MASK_CLE                 0x10
> +#define      CONFIG_MASK_ALE                 0x0a

Make it CONFIG_SYS_DAVINCI_NAND_CLE/ALE.

Also, no tab after #define.

-Scott

[U-Boot] [PATCH] ARM DaVinci: Adding CONFIG options for NAND ALE and CLE

[David Brownell]

On Tuesday 28 April 2009, s-paulraj at ti.com wrote:
> The CLE and ALE for DaVinci DM644x is not the same as DM646x. This patch
> adds 2 CONFIG_ options to the config.h header files to all the DM6446 based
> boards. These values are then used by the driver.

I had been thinking that the davinci_nand driver should
probably change to let board_nand_init() really become a
board-specific function...

It would call a driver routine to init the callback functions
and set up private data so that for example the 2GByte NAND
could be properly treated as a single device with two chips,
instead of two separate devices.  (As Linux does; I'm just
assuming that mechanism works right in U-Boot.)

Other private data could include CLE/ALE masks, if needed.
I may well have missed something, but I thought that the
ROM on all chips used the same mask values when booting
from NAND flash... but that non-boot chips might end up
using different values, if they wanted.  (Burst memory
access would work better if toggling low address bits
didn't affect ALE/CLE, etc.)


Alternatively ... how about just letting those values be
defined in the headers if the boot-from-NAND defaults
shouldn't be used?

That is, the u-boot davinci_nand.c code could

	#ifndef CONFIG_SYS_DAVINCI_NAND_CLE
	... #define it to the default 0x10 ...
	#endif

I think the convention for these things (toplevel README) is
that since they're hardware-specific, CONFIG_SYS_* is the
prefix not CONFIG_* for those symbols.


Related: MASK_ALE should not be 0x0a by default, but 0x08;
right?  That's what newer docs say; I think the old stuff
was just a bug.  In Linux, 0x08 works just fine.

And for that matter, include/asm-arm/arch-davinci/nand_defs.h
is starting to look almost un-necessary after those cleanup
patches I sent.  :)


> This has been tested on the 
> DM644x, DM355, DM357 and DM365. Support for the latter 3 will be
> added soon. 

I'm glad to see TI's latest U-Boot work becoming public...

However, this won't apply on top of the cleanup patches I just
sent, which remove the NAND_CE0{ALE,CLE,DATA} symbols in favor
of the relevant nand_chip pointer.  As you know, that change
is important for support of the EVM boards which include the
socketed 2 GByte NAND chips, with multiple chip select lines.

- Dave

[U-Boot] [PATCH] ARM DaVinci: Adding CONFIG options for NAND ALE and CLE

[Paulraj, Sandeep]

Dave,

Please see inline.

> -----Original Message-----
> From: David Brownell [mailto:david-b at pacbell.net]
> Sent: Tuesday, April 28, 2009 5:51 PM
> To: Paulraj, Sandeep; u-boot at lists.denx.de
> Cc: davinci-linux-open-source at linux.davincidsp.com
> Subject: Re: [PATCH] ARM DaVinci: Adding CONFIG options for NAND ALE and
> CLE
> 
> On Tuesday 28 April 2009, s-paulraj at ti.com wrote:
> > The CLE and ALE for DaVinci DM644x is not the same as DM646x. This patch
> > adds 2 CONFIG_ options to the config.h header files to all the DM6446
> based
> > boards. These values are then used by the driver.
> 
> I had been thinking that the davinci_nand driver should
> probably change to let board_nand_init() really become a
> board-specific function...
> 
> It would call a driver routine to init the callback functions
> and set up private data so that for example the 2GByte NAND
> could be properly treated as a single device with two chips,
> instead of two separate devices.  (As Linux does; I'm just
> assuming that mechanism works right in U-Boot.)
[Sandeep] At the U-Boot level do we really need this mechanism?
> 
> Other private data could include CLE/ALE masks, if needed.
[Sandeep] This is the way we do it in the kernel. Again does U-boot really need this much sophistication.
> I may well have missed something, but I thought that the
> ROM on all chips used the same mask values when booting
> from NAND flash... but that non-boot chips might end up
> using different values, if they wanted.  (Burst memory
> access would work better if toggling low address bits
> didn't affect ALE/CLE, etc.)
> 
> 
> Alternatively ... how about just letting those values be
> defined in the headers if the boot-from-NAND defaults
> shouldn't be used?
> 
> That is, the u-boot davinci_nand.c code could
> 
> 	#ifndef CONFIG_SYS_DAVINCI_NAND_CLE
> 	... #define it to the default 0x10 ...
> 	#endif
[Sandeep] It is my personal opinion that we should try to avoid such #ifdefs in the driver. I would just define all these in the config.h file. Let me know your opinion. We will need such a #ifndef for the CLE as well so I would just put both the values in the config.h rather than have 2 #ifndefs in the NAND driver
> 
> I think the convention for these things (toplevel README) is
> that since they're hardware-specific, CONFIG_SYS_* is the
> prefix not CONFIG_* for those symbols.
[Sandeep] will do
> 
> 
> Related: MASK_ALE should not be 0x0a by default, but 0x08;
> right? 
[Sandeep] That is what I use in the internal LSP releases
 That's what newer docs say; I think the old stuff
> was just a bug.  In Linux, 0x08 works just fine.
[Sandeep] Yes that is correct. I was going to send an e-mail tomorrow to see if anyone had any objections to me changing this value
> 
> And for that matter, include/asm-arm/arch-davinci/nand_defs.h
> is starting to look almost un-necessary after those cleanup
> patches I sent.  :)
[Sandeep] yes I noticed that.
> 
> 
> > This has been tested on the
> > DM644x, DM355, DM357 and DM365. Support for the latter 3 will be
> > added soon.
[Sandeep] Its ready, it just a matter of getting the patches to apply over other patches in a form acceptable to everybody.
> 
> I'm glad to see TI's latest U-Boot work becoming public...
[Sandeep] I don't know why version 1.3.4 has not become public yet but it will be soon.
> 
> However, this won't apply on top of the cleanup patches I just
> sent, which remove the NAND_CE0{ALE,CLE,DATA} symbols in favor
> of the relevant nand_chip pointer.  As you know, that change
> is important for support of the EVM boards which include the
> socketed 2 GByte NAND chips, with multiple chip select lines.
[Sandeep] yes and I will send an updated patch tomorrow.
> 
> - Dave
> 

[U-Boot] [PATCH] ARM DaVinci: Adding CONFIG options for NAND ALE and CLE

[David Brownell]

Hi Sandeep,

On Tuesday 28 April 2009, Paulraj, Sandeep wrote:
> > 
> > I had been thinking that the davinci_nand driver should
> > probably change to let board_nand_init() really become a
> > board-specific function...
> > 
> > It would call a driver routine to init the callback functions
> > and set up private data so that for example the 2GByte NAND
> > could be properly treated as a single device with two chips,
> > instead of two separate devices.  (As Linux does; I'm just
> > assuming that mechanism works right in U-Boot.)
> 
> [Sandeep] At the U-Boot level do we really need this mechanism?

I think it will improve Linux interop.

For example, the MTDPART support in U-Boot provides a string
that can be passed on the kernel command line to ensure that
U-Boot and Linux use the same partitioning.

But ... that won't work very well if Linux sees one big device,
while U-Boot sees two smaller ones.  That "mtdpart=..." command
line string won't work for Linux.

 
> > Other private data could include CLE/ALE masks, if needed.
>
> [Sandeep] This is the way we do it in the kernel. Again does
> U-boot really need this much sophistication.

Erm, the 2.6.30 code (and davinci-git) passes the masks in
through platform_data; they're not fixed at compile time.


> > That is, the u-boot davinci_nand.c code could
> > 
> > 	#ifndef CONFIG_SYS_DAVINCI_NAND_CLE
> > 	... #define it to the default 0x10 ...
> > 	#endif
>
> [Sandeep] It is my personal opinion that we should try to
> avoid such #ifdefs in the driver. I would just define all
> these in the config.h file. Let me know your opinion.

The place to *really* avoid #ifdefs is inside C functions.
I could agree that putting such defaults into the DaVinci
"nand.h" header might be prettier.

But I'd like to keep the config.h files free of such stuff.
Especially, free of stuff that doesn't change from board
to board (except in unusual cases).


> We will need such a #ifndef for the CLE as well so I would
> just put both the values in the config.h rather than have
> 2 #ifndefs in the NAND driver

Well, "nand.h" would be better.  Especially considering
that board using a non-default values are uncommon.


> > Related: MASK_ALE should not be 0x0a by default, but 0x08;
> > right? 
>
> [Sandeep] That is what I use in the internal LSP releases
>
> >	 That's what newer docs say; I think the old stuff
> > was just a bug.  In Linux, 0x08 works just fine.
>
> [Sandeep] Yes that is correct. I was going to send an e-mail
> tomorrow to see if anyone had any objections to me changing this value 

Nah ... just fixit.  ;)

- Dave