[U-Boot] [PATCH] omap3: allow slection of NAND GPMC settings based on board config

[U-Boot] [PATCH] omap3: allow slection of NAND GPMC settings based on board config

[Mike Rapoport]

There are several presets for GPMC registers defined in
include/asm-arm/arch-omap3/mem.h. Allow selection between SMNAND and
M_NAND presets based on OMAP34XX_GPMC_NAND_<PART> defines

Signed-off-by: Mike Rapoport <mike@compulab.co.il>
---
 cpu/arm_cortexa8/omap3/mem.c |   25 ++++++++++++++++---------
 1 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/cpu/arm_cortexa8/omap3/mem.c b/cpu/arm_cortexa8/omap3/mem.c
index 8b8cd6d..2bd7e1c 100644
--- a/cpu/arm_cortexa8/omap3/mem.c
+++ b/cpu/arm_cortexa8/omap3/mem.c
@@ -44,14 +44,21 @@ volatile unsigned int boot_flash_env_addr;
 struct gpmc *gpmc_cfg;
 
 #if defined(CONFIG_CMD_NAND)
-static const u32 gpmc_m_nand[GPMC_MAX_REG] = {
-	M_NAND_GPMC_CONFIG1,
-	M_NAND_GPMC_CONFIG2,
-	M_NAND_GPMC_CONFIG3,
-	M_NAND_GPMC_CONFIG4,
-	M_NAND_GPMC_CONFIG5,
-	M_NAND_GPMC_CONFIG6, 0
-};
+#define GPMC_NAND(PART)					\
+	static const u32 gpmc_nand[GPMC_MAX_REG] = {	\
+		PART##_GPMC_CONFIG1,			\
+		PART##_GPMC_CONFIG2,			\
+		PART##_GPMC_CONFIG3,			\
+		PART##_GPMC_CONFIG4,			\
+		PART##_GPMC_CONFIG5,			\
+		PART##_GPMC_CONFIG6, 0			\
+	};
+
+#ifdef OMAP34XX_GPMC_NAND_SMNAND
+GPMC_NAND(SMNAND)
+#else
+GPMC_NAND(M_NAND)
+#endif
 
 #if defined(CONFIG_ENV_IS_IN_NAND)
 #define GPMC_CS 0
@@ -246,7 +253,7 @@ void gpmc_init(void)
 	sdelay(1000);
 
 #if defined(CONFIG_CMD_NAND)	/* CS 0 */
-	gpmc_config = gpmc_m_nand;
+	gpmc_config = gpmc_nand;
 
 	base = PISMO1_NAND_BASE;
 	size = PISMO1_NAND_SIZE;
-- 
1.6.0.6

[U-Boot] [PATCH] omap3: allow slection of NAND GPMC settings based on board config

[Mike Rapoport]

Any comments on this?

Mike Rapoport wrote:
> There are several presets for GPMC registers defined in
> include/asm-arm/arch-omap3/mem.h. Allow selection between SMNAND and
> M_NAND presets based on OMAP34XX_GPMC_NAND_<PART> defines
> 
> Signed-off-by: Mike Rapoport <mike@compulab.co.il>
> ---
>  cpu/arm_cortexa8/omap3/mem.c |   25 ++++++++++++++++---------
>  1 files changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/cpu/arm_cortexa8/omap3/mem.c b/cpu/arm_cortexa8/omap3/mem.c
> index 8b8cd6d..2bd7e1c 100644
> --- a/cpu/arm_cortexa8/omap3/mem.c
> +++ b/cpu/arm_cortexa8/omap3/mem.c
> @@ -44,14 +44,21 @@ volatile unsigned int boot_flash_env_addr;
>  struct gpmc *gpmc_cfg;
>  
>  #if defined(CONFIG_CMD_NAND)
> -static const u32 gpmc_m_nand[GPMC_MAX_REG] = {
> -	M_NAND_GPMC_CONFIG1,
> -	M_NAND_GPMC_CONFIG2,
> -	M_NAND_GPMC_CONFIG3,
> -	M_NAND_GPMC_CONFIG4,
> -	M_NAND_GPMC_CONFIG5,
> -	M_NAND_GPMC_CONFIG6, 0
> -};
> +#define GPMC_NAND(PART)					\
> +	static const u32 gpmc_nand[GPMC_MAX_REG] = {	\
> +		PART##_GPMC_CONFIG1,			\
> +		PART##_GPMC_CONFIG2,			\
> +		PART##_GPMC_CONFIG3,			\
> +		PART##_GPMC_CONFIG4,			\
> +		PART##_GPMC_CONFIG5,			\
> +		PART##_GPMC_CONFIG6, 0			\
> +	};
> +
> +#ifdef OMAP34XX_GPMC_NAND_SMNAND
> +GPMC_NAND(SMNAND)
> +#else
> +GPMC_NAND(M_NAND)
> +#endif
>  
>  #if defined(CONFIG_ENV_IS_IN_NAND)
>  #define GPMC_CS 0
> @@ -246,7 +253,7 @@ void gpmc_init(void)
>  	sdelay(1000);
>  
>  #if defined(CONFIG_CMD_NAND)	/* CS 0 */
> -	gpmc_config = gpmc_m_nand;
> +	gpmc_config = gpmc_nand;
>  
>  	base = PISMO1_NAND_BASE;
>  	size = PISMO1_NAND_SIZE;

-- 
Sincerely yours,
Mike.

[U-Boot] [PATCH] omap3: allow slection of NAND GPMC settings based on board config

[Tom]

Mike Rapoport wrote:
> Any comments on this?
> 

This is a good idea but..

These defines are used in the TI codebase but
it doesn't look like the samsung nand is used here.

As a test I removed the samsung defines
and reran MAKEALL arm.
No failures.
Also grepping the code shows that it isn't used.

Do you plan on using the samsung nand ?

If not, I think a better patch  would be to remove the smnand defines.

Also I think the nand and onnand initialization could be generalized.  You
may want to look at that.

Thanks
Tom

> Mike Rapoport wrote:
>> There are several presets for GPMC registers defined in
>> include/asm-arm/arch-omap3/mem.h. Allow selection between SMNAND and
>> M_NAND presets based on OMAP34XX_GPMC_NAND_<PART> defines
>>
>> Signed-off-by: Mike Rapoport <mike@compulab.co.il>
>> ---
>>  cpu/arm_cortexa8/omap3/mem.c |   25 ++++++++++++++++---------
>>  1 files changed, 16 insertions(+), 9 deletions(-)
>>
>> diff --git a/cpu/arm_cortexa8/omap3/mem.c b/cpu/arm_cortexa8/omap3/mem.c
>> index 8b8cd6d..2bd7e1c 100644
>> --- a/cpu/arm_cortexa8/omap3/mem.c
>> +++ b/cpu/arm_cortexa8/omap3/mem.c
>> @@ -44,14 +44,21 @@ volatile unsigned int boot_flash_env_addr;
>>  struct gpmc *gpmc_cfg;
>>  
>>  #if defined(CONFIG_CMD_NAND)
>> -static const u32 gpmc_m_nand[GPMC_MAX_REG] = {
>> -	M_NAND_GPMC_CONFIG1,
>> -	M_NAND_GPMC_CONFIG2,
>> -	M_NAND_GPMC_CONFIG3,
>> -	M_NAND_GPMC_CONFIG4,
>> -	M_NAND_GPMC_CONFIG5,
>> -	M_NAND_GPMC_CONFIG6, 0
>> -};
>> +#define GPMC_NAND(PART)					\
>> +	static const u32 gpmc_nand[GPMC_MAX_REG] = {	\
>> +		PART##_GPMC_CONFIG1,			\
>> +		PART##_GPMC_CONFIG2,			\
>> +		PART##_GPMC_CONFIG3,			\
>> +		PART##_GPMC_CONFIG4,			\
>> +		PART##_GPMC_CONFIG5,			\
>> +		PART##_GPMC_CONFIG6, 0			\
>> +	};
>> +
>> +#ifdef OMAP34XX_GPMC_NAND_SMNAND
>> +GPMC_NAND(SMNAND)
>> +#else
>> +GPMC_NAND(M_NAND)
>> +#endif
>>  
>>  #if defined(CONFIG_ENV_IS_IN_NAND)
>>  #define GPMC_CS 0
>> @@ -246,7 +253,7 @@ void gpmc_init(void)
>>  	sdelay(1000);
>>  
>>  #if defined(CONFIG_CMD_NAND)	/* CS 0 */
>> -	gpmc_config = gpmc_m_nand;
>> +	gpmc_config = gpmc_nand;
>>  
>>  	base = PISMO1_NAND_BASE;
>>  	size = PISMO1_NAND_SIZE;
> 

[U-Boot] [PATCH] omap3: allow slection of NAND GPMC settings based on board config

[Mike Rapoport]

On Sun, Nov 15, 2009 at 6:36 PM, Tom <Tom.Rix@windriver.com> wrote:
> Mike Rapoport wrote:
>> Any comments on this?
>>
>
> This is a good idea but..
>
> These defines are used in the TI codebase but
> it doesn't look like the samsung nand is used here.
>
> As a test I removed the samsung defines
> and reran MAKEALL arm.
> No failures.
> Also grepping the code shows that it isn't used.
>
> Do you plan on using the samsung nand ?

That's why I enabled the choice between samsung and Micron parts :)
The CM-T35 board I'm submitting uses the samsung NAND flashes

> If not, I think a better patch ?would be to remove the smnand defines.
>
> Also I think the nand and onnand initialization could be generalized. ?You
> may want to look at that.
>
> Thanks
> Tom
>
>> Mike Rapoport wrote:
>>> There are several presets for GPMC registers defined in
>>> include/asm-arm/arch-omap3/mem.h. Allow selection between SMNAND and
>>> M_NAND presets based on OMAP34XX_GPMC_NAND_<PART> defines
>>>
>>> Signed-off-by: Mike Rapoport <mike@compulab.co.il>
>>> ---
>>> ?cpu/arm_cortexa8/omap3/mem.c | ? 25 ++++++++++++++++---------
>>> ?1 files changed, 16 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/cpu/arm_cortexa8/omap3/mem.c b/cpu/arm_cortexa8/omap3/mem.c
>>> index 8b8cd6d..2bd7e1c 100644
>>> --- a/cpu/arm_cortexa8/omap3/mem.c
>>> +++ b/cpu/arm_cortexa8/omap3/mem.c
>>> @@ -44,14 +44,21 @@ volatile unsigned int boot_flash_env_addr;
>>> ?struct gpmc *gpmc_cfg;
>>>
>>> ?#if defined(CONFIG_CMD_NAND)
>>> -static const u32 gpmc_m_nand[GPMC_MAX_REG] = {
>>> - ? ?M_NAND_GPMC_CONFIG1,
>>> - ? ?M_NAND_GPMC_CONFIG2,
>>> - ? ?M_NAND_GPMC_CONFIG3,
>>> - ? ?M_NAND_GPMC_CONFIG4,
>>> - ? ?M_NAND_GPMC_CONFIG5,
>>> - ? ?M_NAND_GPMC_CONFIG6, 0
>>> -};
>>> +#define GPMC_NAND(PART) ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
>>> + ? ?static const u32 gpmc_nand[GPMC_MAX_REG] = { ? ?\
>>> + ? ? ? ? ? ?PART##_GPMC_CONFIG1, ? ? ? ? ? ? ? ? ? ?\
>>> + ? ? ? ? ? ?PART##_GPMC_CONFIG2, ? ? ? ? ? ? ? ? ? ?\
>>> + ? ? ? ? ? ?PART##_GPMC_CONFIG3, ? ? ? ? ? ? ? ? ? ?\
>>> + ? ? ? ? ? ?PART##_GPMC_CONFIG4, ? ? ? ? ? ? ? ? ? ?\
>>> + ? ? ? ? ? ?PART##_GPMC_CONFIG5, ? ? ? ? ? ? ? ? ? ?\
>>> + ? ? ? ? ? ?PART##_GPMC_CONFIG6, 0 ? ? ? ? ? ? ? ? ?\
>>> + ? ?};
>>> +
>>> +#ifdef OMAP34XX_GPMC_NAND_SMNAND
>>> +GPMC_NAND(SMNAND)
>>> +#else
>>> +GPMC_NAND(M_NAND)
>>> +#endif
>>>
>>> ?#if defined(CONFIG_ENV_IS_IN_NAND)
>>> ?#define GPMC_CS 0
>>> @@ -246,7 +253,7 @@ void gpmc_init(void)
>>> ? ? ?sdelay(1000);
>>>
>>> ?#if defined(CONFIG_CMD_NAND) ? ? ? ?/* CS 0 */
>>> - ? ?gpmc_config = gpmc_m_nand;
>>> + ? ?gpmc_config = gpmc_nand;
>>>
>>> ? ? ?base = PISMO1_NAND_BASE;
>>> ? ? ?size = PISMO1_NAND_SIZE;
>>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>



-- 
	Sincerely Yours,
		Mike.

[U-Boot] [PATCH] omap3: allow slection of NAND GPMC settings based on board config

[Nishanth Menon]

On Sun, Nov 15, 2009 at 10:36 AM, Tom <Tom.Rix@windriver.com> wrote:
> Mike Rapoport wrote:
>> Any comments on this?
>>
>
> This is a good idea but..
>
> These defines are used in the TI codebase but
> it doesn't look like the samsung nand is used here.
>
> As a test I removed the samsung defines
> and reran MAKEALL arm.
> No failures.
> Also grepping the code shows that it isn't used.
>
> Do you plan on using the samsung nand ?
SDP3430 uses Samsung NAND.

>
> If not, I think a better patch ?would be to remove the smnand defines.
NAK.

>
> Also I think the nand and onnand initialization could be generalized. ?You
> may want to look at that.
>
> Thanks
> Tom
>
>> Mike Rapoport wrote:
>>> There are several presets for GPMC registers defined in
>>> include/asm-arm/arch-omap3/mem.h. Allow selection between SMNAND and
>>> M_NAND presets based on OMAP34XX_GPMC_NAND_<PART> defines
>>>
>>> Signed-off-by: Mike Rapoport <mike@compulab.co.il>
>>> ---
>>> ?cpu/arm_cortexa8/omap3/mem.c | ? 25 ++++++++++++++++---------
>>> ?1 files changed, 16 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/cpu/arm_cortexa8/omap3/mem.c b/cpu/arm_cortexa8/omap3/mem.c
>>> index 8b8cd6d..2bd7e1c 100644
>>> --- a/cpu/arm_cortexa8/omap3/mem.c
>>> +++ b/cpu/arm_cortexa8/omap3/mem.c
>>> @@ -44,14 +44,21 @@ volatile unsigned int boot_flash_env_addr;
>>> ?struct gpmc *gpmc_cfg;
>>>
>>> ?#if defined(CONFIG_CMD_NAND)
>>> -static const u32 gpmc_m_nand[GPMC_MAX_REG] = {
>>> - ? ?M_NAND_GPMC_CONFIG1,
>>> - ? ?M_NAND_GPMC_CONFIG2,
>>> - ? ?M_NAND_GPMC_CONFIG3,
>>> - ? ?M_NAND_GPMC_CONFIG4,
>>> - ? ?M_NAND_GPMC_CONFIG5,
>>> - ? ?M_NAND_GPMC_CONFIG6, 0
>>> -};
>>> +#define GPMC_NAND(PART) ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
>>> + ? ?static const u32 gpmc_nand[GPMC_MAX_REG] = { ? ?\
>>> + ? ? ? ? ? ?PART##_GPMC_CONFIG1, ? ? ? ? ? ? ? ? ? ?\
>>> + ? ? ? ? ? ?PART##_GPMC_CONFIG2, ? ? ? ? ? ? ? ? ? ?\
>>> + ? ? ? ? ? ?PART##_GPMC_CONFIG3, ? ? ? ? ? ? ? ? ? ?\
>>> + ? ? ? ? ? ?PART##_GPMC_CONFIG4, ? ? ? ? ? ? ? ? ? ?\
>>> + ? ? ? ? ? ?PART##_GPMC_CONFIG5, ? ? ? ? ? ? ? ? ? ?\
>>> + ? ? ? ? ? ?PART##_GPMC_CONFIG6, 0 ? ? ? ? ? ? ? ? ?\

This is completely unnecessary implementation IMHO, where required,
the board files can register
their own NAND device timings on a need basis.
NOTE: timing values change based on L3 clk -> e..g 3630 means that ALL
the above timing values
are invalid as L3 is at 200Mhz!

>>> + ? ?};
>>> +
>>> +#ifdef OMAP34XX_GPMC_NAND_SMNAND
>>> +GPMC_NAND(SMNAND)
>>> +#else
>>> +GPMC_NAND(M_NAND)
>>> +#endif
>>>
>>> ?#if defined(CONFIG_ENV_IS_IN_NAND)
>>> ?#define GPMC_CS 0
>>> @@ -246,7 +253,7 @@ void gpmc_init(void)
>>> ? ? ?sdelay(1000);
>>>
>>> ?#if defined(CONFIG_CMD_NAND) ? ? ? ?/* CS 0 */
>>> - ? ?gpmc_config = gpmc_m_nand;
>>> + ? ?gpmc_config = gpmc_nand;
>>>
>>> ? ? ?base = PISMO1_NAND_BASE;
>>> ? ? ?size = PISMO1_NAND_SIZE;
>>

IMHO, NAK.
Regards,
Nishanth Menon

[U-Boot] [PATCH] omap3: allow slection of NAND GPMC settings based on board config

[Tom]

Nishanth Menon wrote:
> On Sun, Nov 15, 2009 at 10:36 AM, Tom <Tom.Rix@windriver.com> wrote:
>> Mike Rapoport wrote:
>>> Any comments on this?
>>>
>> This is a good idea but..
>>
>> These defines are used in the TI codebase but
>> it doesn't look like the samsung nand is used here.
>>
>> As a test I removed the samsung defines
>> and reran MAKEALL arm.
>> No failures.
>> Also grepping the code shows that it isn't used.
>>
>> Do you plan on using the samsung nand ?
> SDP3430 uses Samsung NAND.
> 
>> If not, I think a better patch  would be to remove the smnand defines.
> NAK.
> 
>> Also I think the nand and onnand initialization could be generalized.  You
>> may want to look at that.
>>
>> Thanks
>> Tom
>>
>>> Mike Rapoport wrote:
>>>> There are several presets for GPMC registers defined in
>>>> include/asm-arm/arch-omap3/mem.h. Allow selection between SMNAND and
>>>> M_NAND presets based on OMAP34XX_GPMC_NAND_<PART> defines
>>>>
>>>> Signed-off-by: Mike Rapoport <mike@compulab.co.il>
>>>> ---
>>>>  cpu/arm_cortexa8/omap3/mem.c |   25 ++++++++++++++++---------
>>>>  1 files changed, 16 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/cpu/arm_cortexa8/omap3/mem.c b/cpu/arm_cortexa8/omap3/mem.c
>>>> index 8b8cd6d..2bd7e1c 100644
>>>> --- a/cpu/arm_cortexa8/omap3/mem.c
>>>> +++ b/cpu/arm_cortexa8/omap3/mem.c
>>>> @@ -44,14 +44,21 @@ volatile unsigned int boot_flash_env_addr;
>>>>  struct gpmc *gpmc_cfg;
>>>>
>>>>  #if defined(CONFIG_CMD_NAND)
>>>> -static const u32 gpmc_m_nand[GPMC_MAX_REG] = {
>>>> -    M_NAND_GPMC_CONFIG1,
>>>> -    M_NAND_GPMC_CONFIG2,
>>>> -    M_NAND_GPMC_CONFIG3,
>>>> -    M_NAND_GPMC_CONFIG4,
>>>> -    M_NAND_GPMC_CONFIG5,
>>>> -    M_NAND_GPMC_CONFIG6, 0
>>>> -};
>>>> +#define GPMC_NAND(PART)                                     \
>>>> +    static const u32 gpmc_nand[GPMC_MAX_REG] = {    \
>>>> +            PART##_GPMC_CONFIG1,                    \
>>>> +            PART##_GPMC_CONFIG2,                    \
>>>> +            PART##_GPMC_CONFIG3,                    \
>>>> +            PART##_GPMC_CONFIG4,                    \
>>>> +            PART##_GPMC_CONFIG5,                    \
>>>> +            PART##_GPMC_CONFIG6, 0                  \
> 
> This is completely unnecessary implementation IMHO, where required,
> the board files can register
> their own NAND device timings on a need basis.
> NOTE: timing values change based on L3 clk -> e..g 3630 means that ALL
> the above timing values
> are invalid as L3 is at 200Mhz!
> 

You are correct!
Not that this is a surprise :P

Agreeing with Nishanth..
Tom

>>>> +    };
>>>> +
>>>> +#ifdef OMAP34XX_GPMC_NAND_SMNAND
>>>> +GPMC_NAND(SMNAND)
>>>> +#else
>>>> +GPMC_NAND(M_NAND)
>>>> +#endif
>>>>
>>>>  #if defined(CONFIG_ENV_IS_IN_NAND)
>>>>  #define GPMC_CS 0
>>>> @@ -246,7 +253,7 @@ void gpmc_init(void)
>>>>      sdelay(1000);
>>>>
>>>>  #if defined(CONFIG_CMD_NAND)        /* CS 0 */
>>>> -    gpmc_config = gpmc_m_nand;
>>>> +    gpmc_config = gpmc_nand;
>>>>
>>>>      base = PISMO1_NAND_BASE;
>>>>      size = PISMO1_NAND_SIZE;
> 
> IMHO, NAK.
> Regards,
> Nishanth Menon