[U-Boot] [PATCH 0/8] AM3517 EVM / Crane enhancements

[U-Boot] [PATCH 0/8] AM3517 EVM / Crane enhancements

[Tom Rini]

Hey all,

The following series enhances AM3517EVM support as follows:
- Make use of uEnv.txt on MMC, when possible (#4).
- Add build targets for supporting NOR flash and booting from NOR flash
  (#5).
  * As the commit messages say, this requires hardware modifications for
    NOR to be seen at all, which is why this isn't part of the standard
    config.
- Enable ethernet (#7)

While enabling ethernet for the EVM, also enable it on Crane (#8).

To do all of this we needed the following general bugfixes or
enhancements:
#1: SYSBOOT_MASK wasn't including the peripheral/memory flag.
#2: gpmc_init had an extra delay that we didn't need.
#3: gpmc_init needs to know what to do (and not do) when NOR is present.
#6: We need to read the ethaddr for the EMAC and put that into the
    env when possible.

Patch #5 is large but it's a lot of shuffling of the contents
around in order to be able to say "setup this way for NOR" and "setup
this way for NAND".

-- 
Tom

[U-Boot] [PATCH 1/8] OMAP3: Correct SYSBOOT_MASK

[Tom Rini]

The comment in get_boot_type says that bit 5 indicates if we are
peripheral or memory booting, but the mask wasn't including this.  Fix.

Signed-off-by: Tom Rini <trini@ti.com>
---
 arch/arm/include/asm/arch-omap3/cpu.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/include/asm/arch-omap3/cpu.h b/arch/arm/include/asm/arch-omap3/cpu.h
index 84308e0..ea95a1e 100644
--- a/arch/arm/include/asm/arch-omap3/cpu.h
+++ b/arch/arm/include/asm/arch-omap3/cpu.h
@@ -78,7 +78,7 @@ struct ctrl_id {
 
 /* device type */
 #define DEVICE_MASK		(0x7 << 8)
-#define SYSBOOT_MASK		0x1F
+#define SYSBOOT_MASK		0x3F
 #define TST_DEVICE		0x0
 #define EMU_DEVICE		0x1
 #define HS_DEVICE		0x2
-- 
1.7.0.4

[U-Boot] [PATCH 2/8] OMAP3: Drop extra delay in gpmc_init

[Tom Rini]

gpmc_init() was disabling chip select 0 and then waiting the required
amount of time for the change to settle before calling
enable_gpmc_cs_config on CS 0 which starts by disabling and then
sleeping.  Remove this redundancy.

Signed-off-by: Tom Rini <trini@ti.com>
---
 arch/arm/cpu/armv7/omap3/mem.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/arch/arm/cpu/armv7/omap3/mem.c b/arch/arm/cpu/armv7/omap3/mem.c
index 2fe5ac7..4a8c025 100644
--- a/arch/arm/cpu/armv7/omap3/mem.c
+++ b/arch/arm/cpu/armv7/omap3/mem.c
@@ -142,13 +142,6 @@ void gpmc_init(void)
 	config &= (~0xf00);
 	writel(config, &gpmc_cfg->config);
 
-	/*
-	 * Disable the GPMC0 config set by ROM code
-	 * It conflicts with our MPDB (both at 0x08000000)
-	 */
-	writel(0, &gpmc_cfg->cs[0].config7);
-	sdelay(1000);
-
 #if defined(CONFIG_CMD_NAND)	/* CS 0 */
 	gpmc_config = gpmc_m_nand;
 
-- 
1.7.0.4

[U-Boot] [PATCH 3/8] AM3517: Add NOR Flash boot mode Support

[Tom Rini]

From: Vaibhav Hiremath <hvaibhav@ti.com>

Please note that NOR Flash is located on Application board and requires
hardware modification to get NOR boot mode working.

NOR Flash boot mode configuration -

        - From baseboard remove R235 resistor.
	- Set switch S11.3 position to "ON"
	- Set S7 switch position to
		 1  2   3   4   5
		 -----------------
		on off off off off

Please note that, once you remove R235 resistor from the baseboard, you
will not be able to boot from NAND without Application board.
The GPMC_nCS0 is now routed through Application board.

Please note that, <Rev4 revision of Application board doesn't
work with NOR Flash due to HW issue.

Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
Signed-off-by: Tom Rini <trini@ti.com>
---
 arch/arm/cpu/armv7/omap3/mem.c        |   61 +++++++++++++++++++++++++++-----
 arch/arm/include/asm/arch-omap3/mem.h |   15 +++++---
 board/logicpd/am3517evm/am3517evm.h   |    2 +-
 3 files changed, 61 insertions(+), 17 deletions(-)

diff --git a/arch/arm/cpu/armv7/omap3/mem.c b/arch/arm/cpu/armv7/omap3/mem.c
index 4a8c025..4ad3d12 100644
--- a/arch/arm/cpu/armv7/omap3/mem.c
+++ b/arch/arm/cpu/armv7/omap3/mem.c
@@ -51,6 +51,17 @@ static const u32 gpmc_m_nand[GPMC_MAX_REG] = {
 
 #endif
 
+#if defined (CONFIG_CMD_FLASH)
+static const u32 gpmc_nor[GPMC_MAX_REG] = {
+	STNOR_GPMC_CONFIG1,
+	STNOR_GPMC_CONFIG2,
+	STNOR_GPMC_CONFIG3,
+	STNOR_GPMC_CONFIG4,
+	STNOR_GPMC_CONFIG5,
+	STNOR_GPMC_CONFIG6, 0
+};
+#endif
+
 #if defined(CONFIG_CMD_ONENAND)
 static const u32 gpmc_onenand[GPMC_MAX_REG] = {
 	ONENAND_GPMC_CONFIG1,
@@ -118,21 +129,34 @@ void enable_gpmc_cs_config(const u32 *gpmc_config, struct gpmc_cs *cs, u32 base,
 	sdelay(2000);
 }
 
-/*****************************************************
+/*
  * gpmc_init(): init gpmc bus
- * Init GPMC for x16, MuxMode (SDRAM in x32).
- * This code can only be executed from SRAM or SDRAM.
- *****************************************************/
+ * Init GPMC for x16, MuxMode (SDRAM in x32).  This code can only be
+ * executed from SRAM or SDRAM.  In the common case, we will disable
+ * and then configure chip select 0 for our needs (NAND or OneNAND).
+ * However, on the AM3517 EVM the way that NOR can be exposed requires us
+ * to rethink this.  When NOR is enabled, it will be chip select 0 if
+ * we are booting from NOR or chip select 2 otherwise.  This requires us
+ * to check the value we get from the SYSBOOT pins.
+ */
 void gpmc_init(void)
 {
 	/* putting a blanket check on GPMC based on ZeBu for now */
 	gpmc_cfg = (struct gpmc *)GPMC_BASE;
-#if defined(CONFIG_CMD_NAND) || defined(CONFIG_CMD_ONENAND)
+#if defined(CONFIG_CMD_NAND) || defined(CONFIG_CMD_ONENAND) || \
+	(defined(CONFIG_OMAP3_AM3517EVM) && defined(CONFIG_SYS_HAS_NORFLASH))
 	const u32 *gpmc_config = NULL;
 	u32 base = 0;
-	u32 size = 0;
+	u32 sz = 0;
 #endif
 	u32 config = 0;
+	u32 nor_boot = 0;
+
+#if defined(CONFIG_OMAP3_AM3517EVM) && defined(CONFIG_SYS_HAS_NORFLASH)
+	/* 0xD means NOR boot on AM35x */
+	if (get_boot_type() == 0xD)
+		nor_boot = 1;
+#endif
 
 	/* global settings */
 	writel(0, &gpmc_cfg->irqenable); /* isr's sources masked */
@@ -146,14 +170,31 @@ void gpmc_init(void)
 	gpmc_config = gpmc_m_nand;
 
 	base = PISMO1_NAND_BASE;
-	size = PISMO1_NAND_SIZE;
-	enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, size);
+	sz = PISMO1_NAND_SIZE;
+	if (!nor_boot)
+		enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, sz);
+	else
+		enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[2], base, sz);
+
 #endif
 
 #if defined(CONFIG_CMD_ONENAND)
 	gpmc_config = gpmc_onenand;
 	base = PISMO1_ONEN_BASE;
-	size = PISMO1_ONEN_SIZE;
-	enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, size);
+	sz = PISMO1_ONEN_SIZE;
+	if (!nor_boot)
+		enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, sz);
+	else
+		enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[2], base, sz);
+
+#endif
+
+#if defined(CONFIG_OMAP3_AM3517EVM) && defined(CONFIG_SYS_HAS_NORFLASH)
+	/* NOR - CS2 */
+	gpmc_config = gpmc_nor;
+	base = DEBUG_BASE;
+	sz = GPMC_SIZE_64M;
+	if (!nor_boot)
+		enable_gpmc_cs_config(gpmc_nor, &gpmc_cfg->cs[2], base, sz);
 #endif
 }
diff --git a/arch/arm/include/asm/arch-omap3/mem.h b/arch/arm/include/asm/arch-omap3/mem.h
index 5fd02d4..2f52481 100644
--- a/arch/arm/include/asm/arch-omap3/mem.h
+++ b/arch/arm/include/asm/arch-omap3/mem.h
@@ -329,12 +329,15 @@ enum {
 #define M_NAND_GPMC_CONFIG6	0x1f0f0A80
 #define M_NAND_GPMC_CONFIG7	0x00000C44
 
-#define STNOR_GPMC_CONFIG1	0x3
-#define STNOR_GPMC_CONFIG2	0x00151501
-#define STNOR_GPMC_CONFIG3	0x00060602
-#define STNOR_GPMC_CONFIG4	0x11091109
-#define STNOR_GPMC_CONFIG5	0x01141F1F
-#define STNOR_GPMC_CONFIG6	0x000004c4
+/*
+ * Configuration required for AM3517EVM PC28F640P30B85 Flash
+ */
+#define STNOR_GPMC_CONFIG1	0x00001210
+#define STNOR_GPMC_CONFIG2	0x00101001
+#define STNOR_GPMC_CONFIG3	0x00020201
+#define STNOR_GPMC_CONFIG4	0x0f031003
+#define STNOR_GPMC_CONFIG5	0x000f1111
+#define STNOR_GPMC_CONFIG6	0x0f030080
 
 #define SIBNOR_GPMC_CONFIG1	0x1200
 #define SIBNOR_GPMC_CONFIG2	0x001f1f00
diff --git a/board/logicpd/am3517evm/am3517evm.h b/board/logicpd/am3517evm/am3517evm.h
index 68d746c..17bb78d 100644
--- a/board/logicpd/am3517evm/am3517evm.h
+++ b/board/logicpd/am3517evm/am3517evm.h
@@ -327,7 +327,7 @@ const omap3_sysinfo sysinfo = {
 	MUX_VAL(CP(SYS_CLKREQ),		(IEN  | PTD | DIS | M0)) \
 	MUX_VAL(CP(SYS_NIRQ),		(IEN  | PTU | EN  | M0)) \
 	/*SYS_nRESWARM */\
-	MUX_VAL(CP(SYS_NRESWARM),     	(IDIS | PTU | DIS | M4)) \
+	MUX_VAL(CP(SYS_NRESWARM),     	(IDIS | PTU | EN  | M4)) \
 							/* - GPIO30 */\
 	MUX_VAL(CP(SYS_BOOT0),		(IEN  | PTD | DIS | M4)) /*GPIO_2*/\
 							 /* - PEN_IRQ */\
-- 
1.7.0.4

[U-Boot] [PATCH 3/8] AM3517: Add NOR Flash boot mode Support

[Igor Grinberg]

Hi Tom,

On 12/06/11 17:49, Tom Rini wrote:
> From: Vaibhav Hiremath <hvaibhav@ti.com>
> 
> Please note that NOR Flash is located on Application board and requires
> hardware modification to get NOR boot mode working.
> 
> NOR Flash boot mode configuration -
> 
>         - From baseboard remove R235 resistor.
> 	- Set switch S11.3 position to "ON"
> 	- Set S7 switch position to
> 		 1  2   3   4   5
> 		 -----------------
> 		on off off off off
> 
> Please note that, once you remove R235 resistor from the baseboard, you
> will not be able to boot from NAND without Application board.
> The GPMC_nCS0 is now routed through Application board.
> 
> Please note that, <Rev4 revision of Application board doesn't
> work with NOR Flash due to HW issue.
> 
> Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
> Signed-off-by: Tom Rini <trini@ti.com>
> ---
>  arch/arm/cpu/armv7/omap3/mem.c        |   61 +++++++++++++++++++++++++++-----
>  arch/arm/include/asm/arch-omap3/mem.h |   15 +++++---
>  board/logicpd/am3517evm/am3517evm.h   |    2 +-
>  3 files changed, 61 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv7/omap3/mem.c b/arch/arm/cpu/armv7/omap3/mem.c
> index 4a8c025..4ad3d12 100644
> --- a/arch/arm/cpu/armv7/omap3/mem.c
> +++ b/arch/arm/cpu/armv7/omap3/mem.c
> @@ -51,6 +51,17 @@ static const u32 gpmc_m_nand[GPMC_MAX_REG] = {
>  
>  #endif
>  
> +#if defined (CONFIG_CMD_FLASH)
> +static const u32 gpmc_nor[GPMC_MAX_REG] = {
> +	STNOR_GPMC_CONFIG1,
> +	STNOR_GPMC_CONFIG2,
> +	STNOR_GPMC_CONFIG3,
> +	STNOR_GPMC_CONFIG4,
> +	STNOR_GPMC_CONFIG5,
> +	STNOR_GPMC_CONFIG6, 0
> +};
> +#endif

This does not seem to be the right place for these settings...
I think those must be board specific.

> +
>  #if defined(CONFIG_CMD_ONENAND)
>  static const u32 gpmc_onenand[GPMC_MAX_REG] = {
>  	ONENAND_GPMC_CONFIG1,
> @@ -118,21 +129,34 @@ void enable_gpmc_cs_config(const u32 *gpmc_config, struct gpmc_cs *cs, u32 base,
>  	sdelay(2000);
>  }
>  
> -/*****************************************************
> +/*
>   * gpmc_init(): init gpmc bus
> - * Init GPMC for x16, MuxMode (SDRAM in x32).
> - * This code can only be executed from SRAM or SDRAM.
> - *****************************************************/
> + * Init GPMC for x16, MuxMode (SDRAM in x32).  This code can only be
> + * executed from SRAM or SDRAM.  In the common case, we will disable
> + * and then configure chip select 0 for our needs (NAND or OneNAND).
> + * However, on the AM3517 EVM the way that NOR can be exposed requires us
> + * to rethink this.  When NOR is enabled, it will be chip select 0 if
> + * we are booting from NOR or chip select 2 otherwise.  This requires us
> + * to check the value we get from the SYSBOOT pins.

All the above looks like board specific...

> + */
>  void gpmc_init(void)
>  {
>  	/* putting a blanket check on GPMC based on ZeBu for now */
>  	gpmc_cfg = (struct gpmc *)GPMC_BASE;
> -#if defined(CONFIG_CMD_NAND) || defined(CONFIG_CMD_ONENAND)
> +#if defined(CONFIG_CMD_NAND) || defined(CONFIG_CMD_ONENAND) || \
> +	(defined(CONFIG_OMAP3_AM3517EVM) && defined(CONFIG_SYS_HAS_NORFLASH))
>  	const u32 *gpmc_config = NULL;
>  	u32 base = 0;
> -	u32 size = 0;
> +	u32 sz = 0;
>  #endif
>  	u32 config = 0;
> +	u32 nor_boot = 0;
> +
> +#if defined(CONFIG_OMAP3_AM3517EVM) && defined(CONFIG_SYS_HAS_NORFLASH)
> +	/* 0xD means NOR boot on AM35x */
> +	if (get_boot_type() == 0xD)
> +		nor_boot = 1;
> +#endif
>  
>  	/* global settings */
>  	writel(0, &gpmc_cfg->irqenable); /* isr's sources masked */
> @@ -146,14 +170,31 @@ void gpmc_init(void)
>  	gpmc_config = gpmc_m_nand;
>  
>  	base = PISMO1_NAND_BASE;
> -	size = PISMO1_NAND_SIZE;
> -	enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, size);
> +	sz = PISMO1_NAND_SIZE;
> +	if (!nor_boot)
> +		enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, sz);
> +	else
> +		enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[2], base, sz);
> +
>  #endif
>  
>  #if defined(CONFIG_CMD_ONENAND)
>  	gpmc_config = gpmc_onenand;
>  	base = PISMO1_ONEN_BASE;
> -	size = PISMO1_ONEN_SIZE;
> -	enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, size);
> +	sz = PISMO1_ONEN_SIZE;
> +	if (!nor_boot)
> +		enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, sz);
> +	else
> +		enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[2], base, sz);
> +
> +#endif
> +
> +#if defined(CONFIG_OMAP3_AM3517EVM) && defined(CONFIG_SYS_HAS_NORFLASH)
> +	/* NOR - CS2 */
> +	gpmc_config = gpmc_nor;
> +	base = DEBUG_BASE;
> +	sz = GPMC_SIZE_64M;
> +	if (!nor_boot)
> +		enable_gpmc_cs_config(gpmc_nor, &gpmc_cfg->cs[2], base, sz);
>  #endif
>  }

All the above look very hackish...
You just bring board specific code into a common location.
I don't think we should be doing it this way.
Instead, change the gpmc_init() function signature to get a parameter(s),
so it will be a generic function, that will initialize the right stuff
according to the parameters and will not have this board specific
ifdeffery crap.

> diff --git a/arch/arm/include/asm/arch-omap3/mem.h b/arch/arm/include/asm/arch-omap3/mem.h
> index 5fd02d4..2f52481 100644
> --- a/arch/arm/include/asm/arch-omap3/mem.h
> +++ b/arch/arm/include/asm/arch-omap3/mem.h
> @@ -329,12 +329,15 @@ enum {
>  #define M_NAND_GPMC_CONFIG6	0x1f0f0A80
>  #define M_NAND_GPMC_CONFIG7	0x00000C44
>  
> -#define STNOR_GPMC_CONFIG1	0x3
> -#define STNOR_GPMC_CONFIG2	0x00151501
> -#define STNOR_GPMC_CONFIG3	0x00060602
> -#define STNOR_GPMC_CONFIG4	0x11091109
> -#define STNOR_GPMC_CONFIG5	0x01141F1F
> -#define STNOR_GPMC_CONFIG6	0x000004c4
> +/*
> + * Configuration required for AM3517EVM PC28F640P30B85 Flash
> + */
> +#define STNOR_GPMC_CONFIG1	0x00001210
> +#define STNOR_GPMC_CONFIG2	0x00101001
> +#define STNOR_GPMC_CONFIG3	0x00020201
> +#define STNOR_GPMC_CONFIG4	0x0f031003
> +#define STNOR_GPMC_CONFIG5	0x000f1111
> +#define STNOR_GPMC_CONFIG6	0x0f030080

I see also:
arch/arm/cpu/armv7/omap3/lowlevel_init.S:184:   .word STNOR_GPMC_CONFIG3
arch/arm/cpu/armv7/omap3/lowlevel_init.S:188:   .word STNOR_GPMC_CONFIG4
arch/arm/cpu/armv7/omap3/lowlevel_init.S:190:   .word STNOR_GPMC_CONFIG5

I haven't looked into this, but will your change break anything else?

>  
>  #define SIBNOR_GPMC_CONFIG1	0x1200
>  #define SIBNOR_GPMC_CONFIG2	0x001f1f00
> diff --git a/board/logicpd/am3517evm/am3517evm.h b/board/logicpd/am3517evm/am3517evm.h
> index 68d746c..17bb78d 100644
> --- a/board/logicpd/am3517evm/am3517evm.h
> +++ b/board/logicpd/am3517evm/am3517evm.h
> @@ -327,7 +327,7 @@ const omap3_sysinfo sysinfo = {
>  	MUX_VAL(CP(SYS_CLKREQ),		(IEN  | PTD | DIS | M0)) \
>  	MUX_VAL(CP(SYS_NIRQ),		(IEN  | PTU | EN  | M0)) \
>  	/*SYS_nRESWARM */\
> -	MUX_VAL(CP(SYS_NRESWARM),     	(IDIS | PTU | DIS | M4)) \
> +	MUX_VAL(CP(SYS_NRESWARM),     	(IDIS | PTU | EN  | M4)) \
>  							/* - GPIO30 */\
>  	MUX_VAL(CP(SYS_BOOT0),		(IEN  | PTD | DIS | M4)) /*GPIO_2*/\
>  							 /* - PEN_IRQ */\

-- 
Regards,
Igor.

[U-Boot] [PATCH 3/8] AM3517: Add NOR Flash boot mode Support

[Tom Rini]

On Wed, Dec 7, 2011 at 7:25 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:
> Hi Tom,
>
> On 12/06/11 17:49, Tom Rini wrote:
>> From: Vaibhav Hiremath <hvaibhav@ti.com>
>>
>> Please note that NOR Flash is located on Application board and requires
>> hardware modification to get NOR boot mode working.
>>
>> NOR Flash boot mode configuration -
>>
>> ? ? ? ? - From baseboard remove R235 resistor.
>> ? ? ? - Set switch S11.3 position to "ON"
>> ? ? ? - Set S7 switch position to
>> ? ? ? ? ? ? ? ?1 ?2 ? 3 ? 4 ? 5
>> ? ? ? ? ? ? ? ?-----------------
>> ? ? ? ? ? ? ? on off off off off
>>
>> Please note that, once you remove R235 resistor from the baseboard, you
>> will not be able to boot from NAND without Application board.
>> The GPMC_nCS0 is now routed through Application board.
>>
>> Please note that, <Rev4 revision of Application board doesn't
>> work with NOR Flash due to HW issue.
>>
>> Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
>> Signed-off-by: Tom Rini <trini@ti.com>
>> ---
>> ?arch/arm/cpu/armv7/omap3/mem.c ? ? ? ?| ? 61 +++++++++++++++++++++++++++-----
>> ?arch/arm/include/asm/arch-omap3/mem.h | ? 15 +++++---
>> ?board/logicpd/am3517evm/am3517evm.h ? | ? ?2 +-
>> ?3 files changed, 61 insertions(+), 17 deletions(-)
>>
>> diff --git a/arch/arm/cpu/armv7/omap3/mem.c b/arch/arm/cpu/armv7/omap3/mem.c
>> index 4a8c025..4ad3d12 100644
>> --- a/arch/arm/cpu/armv7/omap3/mem.c
>> +++ b/arch/arm/cpu/armv7/omap3/mem.c
>> @@ -51,6 +51,17 @@ static const u32 gpmc_m_nand[GPMC_MAX_REG] = {
>>
>> ?#endif
>>
>> +#if defined (CONFIG_CMD_FLASH)
>> +static const u32 gpmc_nor[GPMC_MAX_REG] = {
>> + ? ? STNOR_GPMC_CONFIG1,
>> + ? ? STNOR_GPMC_CONFIG2,
>> + ? ? STNOR_GPMC_CONFIG3,
>> + ? ? STNOR_GPMC_CONFIG4,
>> + ? ? STNOR_GPMC_CONFIG5,
>> + ? ? STNOR_GPMC_CONFIG6, 0
>> +};
>> +#endif
>
> This does not seem to be the right place for these settings...
> I think those must be board specific.

I'm not sure, it's hard to tell with just one board having NOR and
using this code (and I don't have access to custom boards with NOR on
them, but I know they exist).  In NAND/OneNAND land, this apparently
isn't board-specific settings we're shoving in here.  Another argument
in favor of not just shoving magic values behind a define.

>> ?#if defined(CONFIG_CMD_ONENAND)
>> ?static const u32 gpmc_onenand[GPMC_MAX_REG] = {
>> ? ? ? ONENAND_GPMC_CONFIG1,
>> @@ -118,21 +129,34 @@ void enable_gpmc_cs_config(const u32 *gpmc_config, struct gpmc_cs *cs, u32 base,
>> ? ? ? sdelay(2000);
>> ?}
>>
>> -/*****************************************************
>> +/*
>> ? * gpmc_init(): init gpmc bus
>> - * Init GPMC for x16, MuxMode (SDRAM in x32).
>> - * This code can only be executed from SRAM or SDRAM.
>> - *****************************************************/
>> + * Init GPMC for x16, MuxMode (SDRAM in x32). ?This code can only be
>> + * executed from SRAM or SDRAM. ?In the common case, we will disable
>> + * and then configure chip select 0 for our needs (NAND or OneNAND).
>> + * However, on the AM3517 EVM the way that NOR can be exposed requires us
>> + * to rethink this. ?When NOR is enabled, it will be chip select 0 if
>> + * we are booting from NOR or chip select 2 otherwise. ?This requires us
>> + * to check the value we get from the SYSBOOT pins.
>
> All the above looks like board specific...
>
>> + */
>> ?void gpmc_init(void)
>> ?{
>> ? ? ? /* putting a blanket check on GPMC based on ZeBu for now */
>> ? ? ? gpmc_cfg = (struct gpmc *)GPMC_BASE;
>> -#if defined(CONFIG_CMD_NAND) || defined(CONFIG_CMD_ONENAND)
>> +#if defined(CONFIG_CMD_NAND) || defined(CONFIG_CMD_ONENAND) || \
>> + ? ? (defined(CONFIG_OMAP3_AM3517EVM) && defined(CONFIG_SYS_HAS_NORFLASH))
>> ? ? ? const u32 *gpmc_config = NULL;
>> ? ? ? u32 base = 0;
>> - ? ? u32 size = 0;
>> + ? ? u32 sz = 0;
>> ?#endif
>> ? ? ? u32 config = 0;
>> + ? ? u32 nor_boot = 0;
>> +
>> +#if defined(CONFIG_OMAP3_AM3517EVM) && defined(CONFIG_SYS_HAS_NORFLASH)
>> + ? ? /* 0xD means NOR boot on AM35x */
>> + ? ? if (get_boot_type() == 0xD)
>> + ? ? ? ? ? ? nor_boot = 1;
>> +#endif
>>
>> ? ? ? /* global settings */
>> ? ? ? writel(0, &gpmc_cfg->irqenable); /* isr's sources masked */
>> @@ -146,14 +170,31 @@ void gpmc_init(void)
>> ? ? ? gpmc_config = gpmc_m_nand;
>>
>> ? ? ? base = PISMO1_NAND_BASE;
>> - ? ? size = PISMO1_NAND_SIZE;
>> - ? ? enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, size);
>> + ? ? sz = PISMO1_NAND_SIZE;
>> + ? ? if (!nor_boot)
>> + ? ? ? ? ? ? enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, sz);
>> + ? ? else
>> + ? ? ? ? ? ? enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[2], base, sz);
>> +
>> ?#endif
>>
>> ?#if defined(CONFIG_CMD_ONENAND)
>> ? ? ? gpmc_config = gpmc_onenand;
>> ? ? ? base = PISMO1_ONEN_BASE;
>> - ? ? size = PISMO1_ONEN_SIZE;
>> - ? ? enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, size);
>> + ? ? sz = PISMO1_ONEN_SIZE;
>> + ? ? if (!nor_boot)
>> + ? ? ? ? ? ? enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, sz);
>> + ? ? else
>> + ? ? ? ? ? ? enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[2], base, sz);
>> +
>> +#endif
>> +
>> +#if defined(CONFIG_OMAP3_AM3517EVM) && defined(CONFIG_SYS_HAS_NORFLASH)
>> + ? ? /* NOR - CS2 */
>> + ? ? gpmc_config = gpmc_nor;
>> + ? ? base = DEBUG_BASE;
>> + ? ? sz = GPMC_SIZE_64M;
>> + ? ? if (!nor_boot)
>> + ? ? ? ? ? ? enable_gpmc_cs_config(gpmc_nor, &gpmc_cfg->cs[2], base, sz);
>> ?#endif
>> ?}
>
> All the above look very hackish...
> You just bring board specific code into a common location.

I'm not sure it is board-specific, but I don't have another NOR using
example around...

> I don't think we should be doing it this way.
> Instead, change the gpmc_init() function signature to get a parameter(s),
> so it will be a generic function, that will initialize the right stuff
> according to the parameters and will not have this board specific
> ifdeffery crap.

This I think is right, gpmc_init needs a re-think.

>> diff --git a/arch/arm/include/asm/arch-omap3/mem.h b/arch/arm/include/asm/arch-omap3/mem.h
>> index 5fd02d4..2f52481 100644
>> --- a/arch/arm/include/asm/arch-omap3/mem.h
>> +++ b/arch/arm/include/asm/arch-omap3/mem.h
>> @@ -329,12 +329,15 @@ enum {
>> ?#define M_NAND_GPMC_CONFIG6 ?0x1f0f0A80
>> ?#define M_NAND_GPMC_CONFIG7 ?0x00000C44
>>
>> -#define STNOR_GPMC_CONFIG1 ? 0x3
>> -#define STNOR_GPMC_CONFIG2 ? 0x00151501
>> -#define STNOR_GPMC_CONFIG3 ? 0x00060602
>> -#define STNOR_GPMC_CONFIG4 ? 0x11091109
>> -#define STNOR_GPMC_CONFIG5 ? 0x01141F1F
>> -#define STNOR_GPMC_CONFIG6 ? 0x000004c4
>> +/*
>> + * Configuration required for AM3517EVM PC28F640P30B85 Flash
>> + */
>> +#define STNOR_GPMC_CONFIG1 ? 0x00001210
>> +#define STNOR_GPMC_CONFIG2 ? 0x00101001
>> +#define STNOR_GPMC_CONFIG3 ? 0x00020201
>> +#define STNOR_GPMC_CONFIG4 ? 0x0f031003
>> +#define STNOR_GPMC_CONFIG5 ? 0x000f1111
>> +#define STNOR_GPMC_CONFIG6 ? 0x0f030080
>
> I see also:
> arch/arm/cpu/armv7/omap3/lowlevel_init.S:184: ? .word STNOR_GPMC_CONFIG3
> arch/arm/cpu/armv7/omap3/lowlevel_init.S:188: ? .word STNOR_GPMC_CONFIG4
> arch/arm/cpu/armv7/omap3/lowlevel_init.S:190: ? .word STNOR_GPMC_CONFIG5
>
> I haven't looked into this, but will your change break anything else?

I think the lowlevel_init.S bits need stepping around with a JTAG on a
few boards, but I believe no since no other boards have NOR.

-- 
Tom

[U-Boot] [PATCH 3/8] AM3517: Add NOR Flash boot mode Support

[Igor Grinberg]

On 12/08/11 03:07, Tom Rini wrote:
> On Wed, Dec 7, 2011 at 7:25 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:
>> Hi Tom,
>>
>> On 12/06/11 17:49, Tom Rini wrote:
>>> From: Vaibhav Hiremath <hvaibhav@ti.com>
>>>
>>> Please note that NOR Flash is located on Application board and requires
>>> hardware modification to get NOR boot mode working.
>>>
>>> NOR Flash boot mode configuration -
>>>
>>>         - From baseboard remove R235 resistor.
>>>       - Set switch S11.3 position to "ON"
>>>       - Set S7 switch position to
>>>                1  2   3   4   5
>>>                -----------------
>>>               on off off off off
>>>
>>> Please note that, once you remove R235 resistor from the baseboard, you
>>> will not be able to boot from NAND without Application board.
>>> The GPMC_nCS0 is now routed through Application board.
>>>
>>> Please note that, <Rev4 revision of Application board doesn't
>>> work with NOR Flash due to HW issue.
>>>
>>> Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
>>> Signed-off-by: Tom Rini <trini@ti.com>
>>> ---
>>>  arch/arm/cpu/armv7/omap3/mem.c        |   61 +++++++++++++++++++++++++++-----
>>>  arch/arm/include/asm/arch-omap3/mem.h |   15 +++++---
>>>  board/logicpd/am3517evm/am3517evm.h   |    2 +-
>>>  3 files changed, 61 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/arch/arm/cpu/armv7/omap3/mem.c b/arch/arm/cpu/armv7/omap3/mem.c
>>> index 4a8c025..4ad3d12 100644
>>> --- a/arch/arm/cpu/armv7/omap3/mem.c
>>> +++ b/arch/arm/cpu/armv7/omap3/mem.c
>>> @@ -51,6 +51,17 @@ static const u32 gpmc_m_nand[GPMC_MAX_REG] = {
>>>
>>>  #endif
>>>
>>> +#if defined (CONFIG_CMD_FLASH)
>>> +static const u32 gpmc_nor[GPMC_MAX_REG] = {
>>> +     STNOR_GPMC_CONFIG1,
>>> +     STNOR_GPMC_CONFIG2,
>>> +     STNOR_GPMC_CONFIG3,
>>> +     STNOR_GPMC_CONFIG4,
>>> +     STNOR_GPMC_CONFIG5,
>>> +     STNOR_GPMC_CONFIG6, 0
>>> +};
>>> +#endif
>>
>> This does not seem to be the right place for these settings...
>> I think those must be board specific.
> 
> I'm not sure, it's hard to tell with just one board having NOR and
> using this code (and I don't have access to custom boards with NOR on
> them, but I know they exist).

You don't have to have an access, it is software we are on here and
it should be generic and flexible enough to allow any custom boards
use the common implementation by just providing the information
about the custom part of it.

> In NAND/OneNAND land, this apparently
> isn't board-specific settings we're shoving in here.  Another argument
> in favor of not just shoving magic values behind a define.

No, it is also wrong, look in the
arch/arm/include/asm/arch-omap3/mem.h file, it has at least
two configurations for NAND and NOR each.
I haven't checked the usage for NOR configurations, but
both NAND configurations are in use.
Probably, not every person, who adds board support,
has the confidence to change generic functions in a way
that will serve everybody, so they use it for general init
and then reprogram the chip select to new values.

> 
>>>  #if defined(CONFIG_CMD_ONENAND)
>>>  static const u32 gpmc_onenand[GPMC_MAX_REG] = {
>>>       ONENAND_GPMC_CONFIG1,
>>> @@ -118,21 +129,34 @@ void enable_gpmc_cs_config(const u32 *gpmc_config, struct gpmc_cs *cs, u32 base,
>>>       sdelay(2000);
>>>  }
>>>
>>> -/*****************************************************
>>> +/*
>>>   * gpmc_init(): init gpmc bus
>>> - * Init GPMC for x16, MuxMode (SDRAM in x32).
>>> - * This code can only be executed from SRAM or SDRAM.
>>> - *****************************************************/
>>> + * Init GPMC for x16, MuxMode (SDRAM in x32).  This code can only be
>>> + * executed from SRAM or SDRAM.  In the common case, we will disable
>>> + * and then configure chip select 0 for our needs (NAND or OneNAND).
>>> + * However, on the AM3517 EVM the way that NOR can be exposed requires us
>>> + * to rethink this.  When NOR is enabled, it will be chip select 0 if
>>> + * we are booting from NOR or chip select 2 otherwise.  This requires us
>>> + * to check the value we get from the SYSBOOT pins.
>>
>> All the above looks like board specific...
>>
>>> + */
>>>  void gpmc_init(void)
>>>  {
>>>       /* putting a blanket check on GPMC based on ZeBu for now */
>>>       gpmc_cfg = (struct gpmc *)GPMC_BASE;
>>> -#if defined(CONFIG_CMD_NAND) || defined(CONFIG_CMD_ONENAND)
>>> +#if defined(CONFIG_CMD_NAND) || defined(CONFIG_CMD_ONENAND) || \
>>> +     (defined(CONFIG_OMAP3_AM3517EVM) && defined(CONFIG_SYS_HAS_NORFLASH))
>>>       const u32 *gpmc_config = NULL;
>>>       u32 base = 0;
>>> -     u32 size = 0;
>>> +     u32 sz = 0;
>>>  #endif
>>>       u32 config = 0;
>>> +     u32 nor_boot = 0;
>>> +
>>> +#if defined(CONFIG_OMAP3_AM3517EVM) && defined(CONFIG_SYS_HAS_NORFLASH)
>>> +     /* 0xD means NOR boot on AM35x */
>>> +     if (get_boot_type() == 0xD)
>>> +             nor_boot = 1;
>>> +#endif
>>>
>>>       /* global settings */
>>>       writel(0, &gpmc_cfg->irqenable); /* isr's sources masked */
>>> @@ -146,14 +170,31 @@ void gpmc_init(void)
>>>       gpmc_config = gpmc_m_nand;
>>>
>>>       base = PISMO1_NAND_BASE;
>>> -     size = PISMO1_NAND_SIZE;
>>> -     enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, size);
>>> +     sz = PISMO1_NAND_SIZE;
>>> +     if (!nor_boot)
>>> +             enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, sz);
>>> +     else
>>> +             enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[2], base, sz);
>>> +
>>>  #endif
>>>
>>>  #if defined(CONFIG_CMD_ONENAND)
>>>       gpmc_config = gpmc_onenand;
>>>       base = PISMO1_ONEN_BASE;
>>> -     size = PISMO1_ONEN_SIZE;
>>> -     enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, size);
>>> +     sz = PISMO1_ONEN_SIZE;
>>> +     if (!nor_boot)
>>> +             enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, sz);
>>> +     else
>>> +             enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[2], base, sz);
>>> +
>>> +#endif
>>> +
>>> +#if defined(CONFIG_OMAP3_AM3517EVM) && defined(CONFIG_SYS_HAS_NORFLASH)
>>> +     /* NOR - CS2 */
>>> +     gpmc_config = gpmc_nor;
>>> +     base = DEBUG_BASE;
>>> +     sz = GPMC_SIZE_64M;
>>> +     if (!nor_boot)
>>> +             enable_gpmc_cs_config(gpmc_nor, &gpmc_cfg->cs[2], base, sz);
>>>  #endif
>>>  }
>>
>> All the above look very hackish...
>> You just bring board specific code into a common location.
> 
> I'm not sure it is board-specific, but I don't have another NOR using
> example around...

When you use CONFIG_OMAP3_AM3517EVM - you make it
very board specific...

> 
>> I don't think we should be doing it this way.
>> Instead, change the gpmc_init() function signature to get a parameter(s),
>> so it will be a generic function, that will initialize the right stuff
>> according to the parameters and will not have this board specific
>> ifdeffery crap.
> 
> This I think is right, gpmc_init needs a re-think.

Well, at least we agree on this.
You are the ti tree maintainer now, right?
I think you should initiate such rethinking ask people to do
the right thing and even more, you should not let people merge crap
(at least without a good reason for this).

> 
>>> diff --git a/arch/arm/include/asm/arch-omap3/mem.h b/arch/arm/include/asm/arch-omap3/mem.h
>>> index 5fd02d4..2f52481 100644
>>> --- a/arch/arm/include/asm/arch-omap3/mem.h
>>> +++ b/arch/arm/include/asm/arch-omap3/mem.h
>>> @@ -329,12 +329,15 @@ enum {
>>>  #define M_NAND_GPMC_CONFIG6  0x1f0f0A80
>>>  #define M_NAND_GPMC_CONFIG7  0x00000C44
>>>
>>> -#define STNOR_GPMC_CONFIG1   0x3
>>> -#define STNOR_GPMC_CONFIG2   0x00151501
>>> -#define STNOR_GPMC_CONFIG3   0x00060602
>>> -#define STNOR_GPMC_CONFIG4   0x11091109
>>> -#define STNOR_GPMC_CONFIG5   0x01141F1F
>>> -#define STNOR_GPMC_CONFIG6   0x000004c4
>>> +/*
>>> + * Configuration required for AM3517EVM PC28F640P30B85 Flash
>>> + */
>>> +#define STNOR_GPMC_CONFIG1   0x00001210
>>> +#define STNOR_GPMC_CONFIG2   0x00101001
>>> +#define STNOR_GPMC_CONFIG3   0x00020201
>>> +#define STNOR_GPMC_CONFIG4   0x0f031003
>>> +#define STNOR_GPMC_CONFIG5   0x000f1111
>>> +#define STNOR_GPMC_CONFIG6   0x0f030080
>>
>> I see also:
>> arch/arm/cpu/armv7/omap3/lowlevel_init.S:184:   .word STNOR_GPMC_CONFIG3
>> arch/arm/cpu/armv7/omap3/lowlevel_init.S:188:   .word STNOR_GPMC_CONFIG4
>> arch/arm/cpu/armv7/omap3/lowlevel_init.S:190:   .word STNOR_GPMC_CONFIG5
>>
>> I haven't looked into this, but will your change break anything else?
> 
> I think the lowlevel_init.S bits need stepping around with a JTAG on a
> few boards, but I believe no since no other boards have NOR.
> 

-- 
Regards,
Igor.

[U-Boot] [PATCH 3/8] AM3517: Add NOR Flash boot mode Support

[Tom Rini]

On 12/08/2011 07:12 AM, Igor Grinberg wrote:
> On 12/08/11 03:07, Tom Rini wrote:
>> On Wed, Dec 7, 2011 at 7:25 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>> Hi Tom,
>>>
>>> On 12/06/11 17:49, Tom Rini wrote:
>>>> From: Vaibhav Hiremath <hvaibhav@ti.com>
>>>>
>>>> Please note that NOR Flash is located on Application board and requires
>>>> hardware modification to get NOR boot mode working.
>>>>
>>>> NOR Flash boot mode configuration -
>>>>
>>>>         - From baseboard remove R235 resistor.
>>>>       - Set switch S11.3 position to "ON"
>>>>       - Set S7 switch position to
>>>>                1  2   3   4   5
>>>>                -----------------
>>>>               on off off off off
>>>>
>>>> Please note that, once you remove R235 resistor from the baseboard, you
>>>> will not be able to boot from NAND without Application board.
>>>> The GPMC_nCS0 is now routed through Application board.
>>>>
>>>> Please note that, <Rev4 revision of Application board doesn't
>>>> work with NOR Flash due to HW issue.
>>>>
>>>> Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
>>>> Signed-off-by: Tom Rini <trini@ti.com>
>>>> ---
>>>>  arch/arm/cpu/armv7/omap3/mem.c        |   61 +++++++++++++++++++++++++++-----
>>>>  arch/arm/include/asm/arch-omap3/mem.h |   15 +++++---
>>>>  board/logicpd/am3517evm/am3517evm.h   |    2 +-
>>>>  3 files changed, 61 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/arch/arm/cpu/armv7/omap3/mem.c b/arch/arm/cpu/armv7/omap3/mem.c
>>>> index 4a8c025..4ad3d12 100644
>>>> --- a/arch/arm/cpu/armv7/omap3/mem.c
>>>> +++ b/arch/arm/cpu/armv7/omap3/mem.c
>>>> @@ -51,6 +51,17 @@ static const u32 gpmc_m_nand[GPMC_MAX_REG] = {
>>>>
>>>>  #endif
>>>>
>>>> +#if defined (CONFIG_CMD_FLASH)
>>>> +static const u32 gpmc_nor[GPMC_MAX_REG] = {
>>>> +     STNOR_GPMC_CONFIG1,
>>>> +     STNOR_GPMC_CONFIG2,
>>>> +     STNOR_GPMC_CONFIG3,
>>>> +     STNOR_GPMC_CONFIG4,
>>>> +     STNOR_GPMC_CONFIG5,
>>>> +     STNOR_GPMC_CONFIG6, 0
>>>> +};
>>>> +#endif
>>>
>>> This does not seem to be the right place for these settings...
>>> I think those must be board specific.
>>
>> I'm not sure, it's hard to tell with just one board having NOR and
>> using this code (and I don't have access to custom boards with NOR on
>> them, but I know they exist).
> 
> You don't have to have an access, it is software we are on here and
> it should be generic and flexible enough to allow any custom boards
> use the common implementation by just providing the information
> about the custom part of it.
> 
>> In NAND/OneNAND land, this apparently
>> isn't board-specific settings we're shoving in here.  Another argument
>> in favor of not just shoving magic values behind a define.
> 
> No, it is also wrong, look in the
> arch/arm/include/asm/arch-omap3/mem.h file, it has at least
> two configurations for NAND and NOR each.
> I haven't checked the usage for NOR configurations, but
> both NAND configurations are in use.
> Probably, not every person, who adds board support,
> has the confidence to change generic functions in a way
> that will serve everybody, so they use it for general init
> and then reprogram the chip select to new values.
> 
>>
>>>>  #if defined(CONFIG_CMD_ONENAND)
>>>>  static const u32 gpmc_onenand[GPMC_MAX_REG] = {
>>>>       ONENAND_GPMC_CONFIG1,
>>>> @@ -118,21 +129,34 @@ void enable_gpmc_cs_config(const u32 *gpmc_config, struct gpmc_cs *cs, u32 base,
>>>>       sdelay(2000);
>>>>  }
>>>>
>>>> -/*****************************************************
>>>> +/*
>>>>   * gpmc_init(): init gpmc bus
>>>> - * Init GPMC for x16, MuxMode (SDRAM in x32).
>>>> - * This code can only be executed from SRAM or SDRAM.
>>>> - *****************************************************/
>>>> + * Init GPMC for x16, MuxMode (SDRAM in x32).  This code can only be
>>>> + * executed from SRAM or SDRAM.  In the common case, we will disable
>>>> + * and then configure chip select 0 for our needs (NAND or OneNAND).
>>>> + * However, on the AM3517 EVM the way that NOR can be exposed requires us
>>>> + * to rethink this.  When NOR is enabled, it will be chip select 0 if
>>>> + * we are booting from NOR or chip select 2 otherwise.  This requires us
>>>> + * to check the value we get from the SYSBOOT pins.
>>>
>>> All the above looks like board specific...
>>>
>>>> + */
>>>>  void gpmc_init(void)
>>>>  {
>>>>       /* putting a blanket check on GPMC based on ZeBu for now */
>>>>       gpmc_cfg = (struct gpmc *)GPMC_BASE;
>>>> -#if defined(CONFIG_CMD_NAND) || defined(CONFIG_CMD_ONENAND)
>>>> +#if defined(CONFIG_CMD_NAND) || defined(CONFIG_CMD_ONENAND) || \
>>>> +     (defined(CONFIG_OMAP3_AM3517EVM) && defined(CONFIG_SYS_HAS_NORFLASH))
>>>>       const u32 *gpmc_config = NULL;
>>>>       u32 base = 0;
>>>> -     u32 size = 0;
>>>> +     u32 sz = 0;
>>>>  #endif
>>>>       u32 config = 0;
>>>> +     u32 nor_boot = 0;
>>>> +
>>>> +#if defined(CONFIG_OMAP3_AM3517EVM) && defined(CONFIG_SYS_HAS_NORFLASH)
>>>> +     /* 0xD means NOR boot on AM35x */
>>>> +     if (get_boot_type() == 0xD)
>>>> +             nor_boot = 1;
>>>> +#endif
>>>>
>>>>       /* global settings */
>>>>       writel(0, &gpmc_cfg->irqenable); /* isr's sources masked */
>>>> @@ -146,14 +170,31 @@ void gpmc_init(void)
>>>>       gpmc_config = gpmc_m_nand;
>>>>
>>>>       base = PISMO1_NAND_BASE;
>>>> -     size = PISMO1_NAND_SIZE;
>>>> -     enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, size);
>>>> +     sz = PISMO1_NAND_SIZE;
>>>> +     if (!nor_boot)
>>>> +             enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, sz);
>>>> +     else
>>>> +             enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[2], base, sz);
>>>> +
>>>>  #endif
>>>>
>>>>  #if defined(CONFIG_CMD_ONENAND)
>>>>       gpmc_config = gpmc_onenand;
>>>>       base = PISMO1_ONEN_BASE;
>>>> -     size = PISMO1_ONEN_SIZE;
>>>> -     enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, size);
>>>> +     sz = PISMO1_ONEN_SIZE;
>>>> +     if (!nor_boot)
>>>> +             enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, sz);
>>>> +     else
>>>> +             enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[2], base, sz);
>>>> +
>>>> +#endif
>>>> +
>>>> +#if defined(CONFIG_OMAP3_AM3517EVM) && defined(CONFIG_SYS_HAS_NORFLASH)
>>>> +     /* NOR - CS2 */
>>>> +     gpmc_config = gpmc_nor;
>>>> +     base = DEBUG_BASE;
>>>> +     sz = GPMC_SIZE_64M;
>>>> +     if (!nor_boot)
>>>> +             enable_gpmc_cs_config(gpmc_nor, &gpmc_cfg->cs[2], base, sz);
>>>>  #endif
>>>>  }
>>>
>>> All the above look very hackish...
>>> You just bring board specific code into a common location.
>>
>> I'm not sure it is board-specific, but I don't have another NOR using
>> example around...is 
> 
> When you use CONFIG_OMAP3_AM3517EVM - you make it
> very board specific...

Yes, and what I'm saying is that without seeing some of the other AM35x
boards with NOR, I'm unsure if the check should simply be on
CONFIG_SYS_AM35X_HAS_NORFLASH or if it is indeed am3517 evm specific due
to the hardware changes required to expose NOR.  This is moot however as...

>>> I don't think we should be doing it this way.
>>> Instead, change the gpmc_init() function signature to get a parameter(s),
>>> so it will be a generic function, that will initialize the right stuff
>>> according to the parameters and will not have this board specific
>>> ifdeffery crap.
>>
>> This I think is right, gpmc_init needs a re-think.
> 
> Well, at least we agree on this.
> You are the ti tree maintainer now, right?
> I think you should initiate such rethinking ask people to do
> the right thing and even more, you should not let people merge crap
> (at least without a good reason for this).

To be clear, I was saying that gpmc_init needs a re-think, and that does
fall to me to go and do.  And in general, I'm deciding to post the work
other folks have done at various points in the past as I take "TI
trees", figure out what hasn't been re-done by the community (or
eventually posted by us) and get it ready for merging.

And yes, gpmc_init should either end up taking the CSs we need to call
enable_gpmc_cs_init() on, or maybe turned into a weak function that does
the normal case of just CS0 always needs setting to whichever of NAND or
OneNAND is set and let am3517evm provide its own more complicated one,
I'll have to play around.

And, thanks for the review.  More eyes means we'll get all of this code
into better shape.

-- 
Tom

[U-Boot] [PATCH 3/8] AM3517: Add NOR Flash boot mode Support

[Igor Grinberg]

On 12/08/11 16:38, Tom Rini wrote:
> On 12/08/2011 07:12 AM, Igor Grinberg wrote:
>> On 12/08/11 03:07, Tom Rini wrote:
>>> On Wed, Dec 7, 2011 at 7:25 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>>> Hi Tom,
>>>>
>>>> On 12/06/11 17:49, Tom Rini wrote:
>>>>> From: Vaibhav Hiremath <hvaibhav@ti.com>
>>>>>
>>>>> Please note that NOR Flash is located on Application board and requires
>>>>> hardware modification to get NOR boot mode working.
>>>>>
>>>>> NOR Flash boot mode configuration -
>>>>>
>>>>>         - From baseboard remove R235 resistor.
>>>>>       - Set switch S11.3 position to "ON"
>>>>>       - Set S7 switch position to
>>>>>                1  2   3   4   5
>>>>>                -----------------
>>>>>               on off off off off
>>>>>
>>>>> Please note that, once you remove R235 resistor from the baseboard, you
>>>>> will not be able to boot from NAND without Application board.
>>>>> The GPMC_nCS0 is now routed through Application board.
>>>>>
>>>>> Please note that, <Rev4 revision of Application board doesn't
>>>>> work with NOR Flash due to HW issue.
>>>>>
>>>>> Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
>>>>> Signed-off-by: Tom Rini <trini@ti.com>
>>>>> ---
>>>>>  arch/arm/cpu/armv7/omap3/mem.c        |   61 +++++++++++++++++++++++++++-----
>>>>>  arch/arm/include/asm/arch-omap3/mem.h |   15 +++++---
>>>>>  board/logicpd/am3517evm/am3517evm.h   |    2 +-
>>>>>  3 files changed, 61 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/cpu/armv7/omap3/mem.c b/arch/arm/cpu/armv7/omap3/mem.c
>>>>> index 4a8c025..4ad3d12 100644
>>>>> --- a/arch/arm/cpu/armv7/omap3/mem.c
>>>>> +++ b/arch/arm/cpu/armv7/omap3/mem.c
>>>>> @@ -51,6 +51,17 @@ static const u32 gpmc_m_nand[GPMC_MAX_REG] = {
>>>>>
>>>>>  #endif
>>>>>
>>>>> +#if defined (CONFIG_CMD_FLASH)
>>>>> +static const u32 gpmc_nor[GPMC_MAX_REG] = {
>>>>> +     STNOR_GPMC_CONFIG1,
>>>>> +     STNOR_GPMC_CONFIG2,
>>>>> +     STNOR_GPMC_CONFIG3,
>>>>> +     STNOR_GPMC_CONFIG4,
>>>>> +     STNOR_GPMC_CONFIG5,
>>>>> +     STNOR_GPMC_CONFIG6, 0
>>>>> +};
>>>>> +#endif
>>>>
>>>> This does not seem to be the right place for these settings...
>>>> I think those must be board specific.
>>>
>>> I'm not sure, it's hard to tell with just one board having NOR and
>>> using this code (and I don't have access to custom boards with NOR on
>>> them, but I know they exist).
>>
>> You don't have to have an access, it is software we are on here and
>> it should be generic and flexible enough to allow any custom boards
>> use the common implementation by just providing the information
>> about the custom part of it.
>>
>>> In NAND/OneNAND land, this apparently
>>> isn't board-specific settings we're shoving in here.  Another argument
>>> in favor of not just shoving magic values behind a define.
>>
>> No, it is also wrong, look in the
>> arch/arm/include/asm/arch-omap3/mem.h file, it has at least
>> two configurations for NAND and NOR each.
>> I haven't checked the usage for NOR configurations, but
>> both NAND configurations are in use.
>> Probably, not every person, who adds board support,
>> has the confidence to change generic functions in a way
>> that will serve everybody, so they use it for general init
>> and then reprogram the chip select to new values.
>>
>>>
>>>>>  #if defined(CONFIG_CMD_ONENAND)
>>>>>  static const u32 gpmc_onenand[GPMC_MAX_REG] = {
>>>>>       ONENAND_GPMC_CONFIG1,
>>>>> @@ -118,21 +129,34 @@ void enable_gpmc_cs_config(const u32 *gpmc_config, struct gpmc_cs *cs, u32 base,
>>>>>       sdelay(2000);
>>>>>  }
>>>>>
>>>>> -/*****************************************************
>>>>> +/*
>>>>>   * gpmc_init(): init gpmc bus
>>>>> - * Init GPMC for x16, MuxMode (SDRAM in x32).
>>>>> - * This code can only be executed from SRAM or SDRAM.
>>>>> - *****************************************************/
>>>>> + * Init GPMC for x16, MuxMode (SDRAM in x32).  This code can only be
>>>>> + * executed from SRAM or SDRAM.  In the common case, we will disable
>>>>> + * and then configure chip select 0 for our needs (NAND or OneNAND).
>>>>> + * However, on the AM3517 EVM the way that NOR can be exposed requires us
>>>>> + * to rethink this.  When NOR is enabled, it will be chip select 0 if
>>>>> + * we are booting from NOR or chip select 2 otherwise.  This requires us
>>>>> + * to check the value we get from the SYSBOOT pins.
>>>>
>>>> All the above looks like board specific...
>>>>
>>>>> + */
>>>>>  void gpmc_init(void)
>>>>>  {
>>>>>       /* putting a blanket check on GPMC based on ZeBu for now */
>>>>>       gpmc_cfg = (struct gpmc *)GPMC_BASE;
>>>>> -#if defined(CONFIG_CMD_NAND) || defined(CONFIG_CMD_ONENAND)
>>>>> +#if defined(CONFIG_CMD_NAND) || defined(CONFIG_CMD_ONENAND) || \
>>>>> +     (defined(CONFIG_OMAP3_AM3517EVM) && defined(CONFIG_SYS_HAS_NORFLASH))
>>>>>       const u32 *gpmc_config = NULL;
>>>>>       u32 base = 0;
>>>>> -     u32 size = 0;
>>>>> +     u32 sz = 0;
>>>>>  #endif
>>>>>       u32 config = 0;
>>>>> +     u32 nor_boot = 0;
>>>>> +
>>>>> +#if defined(CONFIG_OMAP3_AM3517EVM) && defined(CONFIG_SYS_HAS_NORFLASH)
>>>>> +     /* 0xD means NOR boot on AM35x */
>>>>> +     if (get_boot_type() == 0xD)
>>>>> +             nor_boot = 1;
>>>>> +#endif
>>>>>
>>>>>       /* global settings */
>>>>>       writel(0, &gpmc_cfg->irqenable); /* isr's sources masked */
>>>>> @@ -146,14 +170,31 @@ void gpmc_init(void)
>>>>>       gpmc_config = gpmc_m_nand;
>>>>>
>>>>>       base = PISMO1_NAND_BASE;
>>>>> -     size = PISMO1_NAND_SIZE;
>>>>> -     enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, size);
>>>>> +     sz = PISMO1_NAND_SIZE;
>>>>> +     if (!nor_boot)
>>>>> +             enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, sz);
>>>>> +     else
>>>>> +             enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[2], base, sz);
>>>>> +
>>>>>  #endif
>>>>>
>>>>>  #if defined(CONFIG_CMD_ONENAND)
>>>>>       gpmc_config = gpmc_onenand;
>>>>>       base = PISMO1_ONEN_BASE;
>>>>> -     size = PISMO1_ONEN_SIZE;
>>>>> -     enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, size);
>>>>> +     sz = PISMO1_ONEN_SIZE;
>>>>> +     if (!nor_boot)
>>>>> +             enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, sz);
>>>>> +     else
>>>>> +             enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[2], base, sz);
>>>>> +
>>>>> +#endif
>>>>> +
>>>>> +#if defined(CONFIG_OMAP3_AM3517EVM) && defined(CONFIG_SYS_HAS_NORFLASH)
>>>>> +     /* NOR - CS2 */
>>>>> +     gpmc_config = gpmc_nor;
>>>>> +     base = DEBUG_BASE;
>>>>> +     sz = GPMC_SIZE_64M;
>>>>> +     if (!nor_boot)
>>>>> +             enable_gpmc_cs_config(gpmc_nor, &gpmc_cfg->cs[2], base, sz);
>>>>>  #endif
>>>>>  }
>>>>
>>>> All the above look very hackish...
>>>> You just bring board specific code into a common location.
>>>
>>> I'm not sure it is board-specific, but I don't have another NOR using
>>> example around...is 
>>
>> When you use CONFIG_OMAP3_AM3517EVM - you make it
>> very board specific...
> 
> Yes, and what I'm saying is that without seeing some of the other AM35x
> boards with NOR, I'm unsure if the check should simply be on
> CONFIG_SYS_AM35X_HAS_NORFLASH or if it is indeed am3517 evm specific due
> to the hardware changes required to expose NOR.  This is moot however as...
> 
>>>> I don't think we should be doing it this way.
>>>> Instead, change the gpmc_init() function signature to get a parameter(s),
>>>> so it will be a generic function, that will initialize the right stuff
>>>> according to the parameters and will not have this board specific
>>>> ifdeffery crap.
>>>
>>> This I think is right, gpmc_init needs a re-think.
>>
>> Well, at least we agree on this.
>> You are the ti tree maintainer now, right?
>> I think you should initiate such rethinking ask people to do
>> the right thing and even more, you should not let people merge crap
>> (at least without a good reason for this).
> 
> To be clear, I was saying that gpmc_init needs a re-think, and that does
> fall to me to go and do.  And in general, I'm deciding to post the work
> other folks have done at various points in the past as I take "TI
> trees", figure out what hasn't been re-done by the community (or
> eventually posted by us) and get it ready for merging.

That would be great!

> 
> And yes, gpmc_init should either end up taking the CSs we need to call
> enable_gpmc_cs_init() on, or maybe turned into a weak function that does
> the normal case of just CS0 always needs setting to whichever of NAND or
> OneNAND is set and let am3517evm provide its own more complicated one,
> I'll have to play around.

Good.
I vote for the same function taking different routes
depending on the parameters passed, but probably,
it can be best seen while working on this.

Thanks


-- 
Regards,
Igor.

[U-Boot] [PATCH 3/8] AM3517: Add NOR Flash boot mode Support

[Hiremath, Vaibhav]

> -----Original Message-----
> From: Igor Grinberg [mailto:grinberg at compulab.co.il]
> Sent: Wednesday, December 07, 2011 7:55 PM
> To: Rini, Tom
> Cc: u-boot at lists.denx.de; Hiremath, Vaibhav
> Subject: Re: [U-Boot] [PATCH 3/8] AM3517: Add NOR Flash boot mode Support
> 
> Hi Tom,
> 
> On 12/06/11 17:49, Tom Rini wrote:
> > From: Vaibhav Hiremath <hvaibhav@ti.com>
> >
> > Please note that NOR Flash is located on Application board and requires
> > hardware modification to get NOR boot mode working.
> >
> > NOR Flash boot mode configuration -
> >
> >         - From baseboard remove R235 resistor.
> > 	- Set switch S11.3 position to "ON"
> > 	- Set S7 switch position to
> > 		 1  2   3   4   5
> > 		 -----------------
> > 		on off off off off
> >
> > Please note that, once you remove R235 resistor from the baseboard, you
> > will not be able to boot from NAND without Application board.
> > The GPMC_nCS0 is now routed through Application board.
> >
> > Please note that, <Rev4 revision of Application board doesn't
> > work with NOR Flash due to HW issue.
> >
> > Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
> > Signed-off-by: Tom Rini <trini@ti.com>
> > ---
> >  arch/arm/cpu/armv7/omap3/mem.c        |   61
> +++++++++++++++++++++++++++-----
> >  arch/arm/include/asm/arch-omap3/mem.h |   15 +++++---
> >  board/logicpd/am3517evm/am3517evm.h   |    2 +-
> >  3 files changed, 61 insertions(+), 17 deletions(-)
> >
> > diff --git a/arch/arm/cpu/armv7/omap3/mem.c
> b/arch/arm/cpu/armv7/omap3/mem.c
> > index 4a8c025..4ad3d12 100644
> > --- a/arch/arm/cpu/armv7/omap3/mem.c
> > +++ b/arch/arm/cpu/armv7/omap3/mem.c
> > @@ -51,6 +51,17 @@ static const u32 gpmc_m_nand[GPMC_MAX_REG] = {
> >
> >  #endif
> >
> > +#if defined (CONFIG_CMD_FLASH)
> > +static const u32 gpmc_nor[GPMC_MAX_REG] = {
> > +	STNOR_GPMC_CONFIG1,
> > +	STNOR_GPMC_CONFIG2,
> > +	STNOR_GPMC_CONFIG3,
> > +	STNOR_GPMC_CONFIG4,
> > +	STNOR_GPMC_CONFIG5,
> > +	STNOR_GPMC_CONFIG6, 0
> > +};
> > +#endif
> 
> This does not seem to be the right place for these settings...
> I think those must be board specific.
> 
[Hiremath, Vaibhav] I think, yes this information is board specific, since NOR flash is mounted on board.

Actually we have followed existing NAND/OneNAND implementation here, and that's where we have this here.
Also note that, in case of OMAP3/AM37x we have POP package where NAND/OneNAND part is fixed, so it makes sense to bring here.

> > +
> >  #if defined(CONFIG_CMD_ONENAND)
> >  static const u32 gpmc_onenand[GPMC_MAX_REG] = {
> >  	ONENAND_GPMC_CONFIG1,
> > @@ -118,21 +129,34 @@ void enable_gpmc_cs_config(const u32 *gpmc_config,
> struct gpmc_cs *cs, u32 base,
> >  	sdelay(2000);
> >  }
> >
> > -/*****************************************************
> > +/*
> >   * gpmc_init(): init gpmc bus
> > - * Init GPMC for x16, MuxMode (SDRAM in x32).
> > - * This code can only be executed from SRAM or SDRAM.
> > - *****************************************************/
> > + * Init GPMC for x16, MuxMode (SDRAM in x32).  This code can only be
> > + * executed from SRAM or SDRAM.  In the common case, we will disable
> > + * and then configure chip select 0 for our needs (NAND or OneNAND).
> > + * However, on the AM3517 EVM the way that NOR can be exposed requires
> us
> > + * to rethink this.  When NOR is enabled, it will be chip select 0 if
> > + * we are booting from NOR or chip select 2 otherwise.  This requires
> us
> > + * to check the value we get from the SYSBOOT pins.
> 
> All the above looks like board specific...
> 
> > + */
> >  void gpmc_init(void)
> >  {
> >  	/* putting a blanket check on GPMC based on ZeBu for now */
> >  	gpmc_cfg = (struct gpmc *)GPMC_BASE;
> > -#if defined(CONFIG_CMD_NAND) || defined(CONFIG_CMD_ONENAND)
> > +#if defined(CONFIG_CMD_NAND) || defined(CONFIG_CMD_ONENAND) || \
> > +	(defined(CONFIG_OMAP3_AM3517EVM) &&
> defined(CONFIG_SYS_HAS_NORFLASH))
> >  	const u32 *gpmc_config = NULL;
> >  	u32 base = 0;
> > -	u32 size = 0;
> > +	u32 sz = 0;
> >  #endif
> >  	u32 config = 0;
> > +	u32 nor_boot = 0;
> > +
> > +#if defined(CONFIG_OMAP3_AM3517EVM) && defined(CONFIG_SYS_HAS_NORFLASH)
> > +	/* 0xD means NOR boot on AM35x */
> > +	if (get_boot_type() == 0xD)
> > +		nor_boot = 1;
> > +#endif
> >
> >  	/* global settings */
> >  	writel(0, &gpmc_cfg->irqenable); /* isr's sources masked */
> > @@ -146,14 +170,31 @@ void gpmc_init(void)
> >  	gpmc_config = gpmc_m_nand;
> >
> >  	base = PISMO1_NAND_BASE;
> > -	size = PISMO1_NAND_SIZE;
> > -	enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, size);
> > +	sz = PISMO1_NAND_SIZE;
> > +	if (!nor_boot)
> > +		enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base,
> sz);
> > +	else
> > +		enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[2], base,
> sz);
> > +
> >  #endif
> >
> >  #if defined(CONFIG_CMD_ONENAND)
> >  	gpmc_config = gpmc_onenand;
> >  	base = PISMO1_ONEN_BASE;
> > -	size = PISMO1_ONEN_SIZE;
> > -	enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, size);
> > +	sz = PISMO1_ONEN_SIZE;
> > +	if (!nor_boot)
> > +		enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base,
> sz);
> > +	else
> > +		enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[2], base,
> sz);
> > +
> > +#endif
> > +
> > +#if defined(CONFIG_OMAP3_AM3517EVM) && defined(CONFIG_SYS_HAS_NORFLASH)
> > +	/* NOR - CS2 */
> > +	gpmc_config = gpmc_nor;
> > +	base = DEBUG_BASE;
> > +	sz = GPMC_SIZE_64M;
> > +	if (!nor_boot)
> > +		enable_gpmc_cs_config(gpmc_nor, &gpmc_cfg->cs[2], base, sz);
> >  #endif
> >  }
> 
> All the above look very hackish...
> You just bring board specific code into a common location.
> I don't think we should be doing it this way.
> Instead, change the gpmc_init() function signature to get a parameter(s),
> so it will be a generic function, that will initialize the right stuff
> according to the parameters and will not have this board specific
> ifdeffery crap.
> 
[Hiremath, Vaibhav] Agreed, gpmc_init needs some cleanup to make it generic interface. And probably all data should get passed from board file.

> > diff --git a/arch/arm/include/asm/arch-omap3/mem.h
> b/arch/arm/include/asm/arch-omap3/mem.h
> > index 5fd02d4..2f52481 100644
> > --- a/arch/arm/include/asm/arch-omap3/mem.h
> > +++ b/arch/arm/include/asm/arch-omap3/mem.h
> > @@ -329,12 +329,15 @@ enum {
> >  #define M_NAND_GPMC_CONFIG6	0x1f0f0A80
> >  #define M_NAND_GPMC_CONFIG7	0x00000C44
> >
> > -#define STNOR_GPMC_CONFIG1	0x3
> > -#define STNOR_GPMC_CONFIG2	0x00151501
> > -#define STNOR_GPMC_CONFIG3	0x00060602
> > -#define STNOR_GPMC_CONFIG4	0x11091109
> > -#define STNOR_GPMC_CONFIG5	0x01141F1F
> > -#define STNOR_GPMC_CONFIG6	0x000004c4
> > +/*
> > + * Configuration required for AM3517EVM PC28F640P30B85 Flash
> > + */
> > +#define STNOR_GPMC_CONFIG1	0x00001210
> > +#define STNOR_GPMC_CONFIG2	0x00101001
> > +#define STNOR_GPMC_CONFIG3	0x00020201
> > +#define STNOR_GPMC_CONFIG4	0x0f031003
> > +#define STNOR_GPMC_CONFIG5	0x000f1111
> > +#define STNOR_GPMC_CONFIG6	0x0f030080
> 
> I see also:
> arch/arm/cpu/armv7/omap3/lowlevel_init.S:184:   .word STNOR_GPMC_CONFIG3
> arch/arm/cpu/armv7/omap3/lowlevel_init.S:188:   .word STNOR_GPMC_CONFIG4
> arch/arm/cpu/armv7/omap3/lowlevel_init.S:190:   .word STNOR_GPMC_CONFIG5
> 
> I haven't looked into this, but will your change break anything else?
> 
[Hiremath, Vaibhav] I do not think it breaks anything.

> >
> >  #define SIBNOR_GPMC_CONFIG1	0x1200
> >  #define SIBNOR_GPMC_CONFIG2	0x001f1f00
> > diff --git a/board/logicpd/am3517evm/am3517evm.h
> b/board/logicpd/am3517evm/am3517evm.h
> > index 68d746c..17bb78d 100644
> > --- a/board/logicpd/am3517evm/am3517evm.h
> > +++ b/board/logicpd/am3517evm/am3517evm.h
> > @@ -327,7 +327,7 @@ const omap3_sysinfo sysinfo = {
> >  	MUX_VAL(CP(SYS_CLKREQ),		(IEN  | PTD | DIS | M0)) \
> >  	MUX_VAL(CP(SYS_NIRQ),		(IEN  | PTU | EN  | M0)) \
> >  	/*SYS_nRESWARM */\
> > -	MUX_VAL(CP(SYS_NRESWARM),     	(IDIS | PTU | DIS | M4)) \
> > +	MUX_VAL(CP(SYS_NRESWARM),     	(IDIS | PTU | EN  | M4)) \
> >  							/* - GPIO30 */\
> >  	MUX_VAL(CP(SYS_BOOT0),		(IEN  | PTD | DIS | M4)) /*GPIO_2*/\
> >  							 /* - PEN_IRQ */\
> 
> --
> Regards,
> Igor.

[U-Boot] [PATCH 3/8] AM3517: Add NOR Flash boot mode Support

[Igor Grinberg]

Hi Vaibhav,

On 12/08/11 13:11, Hiremath, Vaibhav wrote:
> 
>> -----Original Message-----
>> From: Igor Grinberg [mailto:grinberg at compulab.co.il]
>> Sent: Wednesday, December 07, 2011 7:55 PM
>> To: Rini, Tom
>> Cc: u-boot at lists.denx.de; Hiremath, Vaibhav
>> Subject: Re: [U-Boot] [PATCH 3/8] AM3517: Add NOR Flash boot mode Support
>>
>> Hi Tom,
>>
>> On 12/06/11 17:49, Tom Rini wrote:
>>> From: Vaibhav Hiremath <hvaibhav@ti.com>
>>>
>>> Please note that NOR Flash is located on Application board and requires
>>> hardware modification to get NOR boot mode working.
>>>
>>> NOR Flash boot mode configuration -
>>>
>>>         - From baseboard remove R235 resistor.
>>> 	- Set switch S11.3 position to "ON"
>>> 	- Set S7 switch position to
>>> 		 1  2   3   4   5
>>> 		 -----------------
>>> 		on off off off off
>>>
>>> Please note that, once you remove R235 resistor from the baseboard, you
>>> will not be able to boot from NAND without Application board.
>>> The GPMC_nCS0 is now routed through Application board.
>>>
>>> Please note that, <Rev4 revision of Application board doesn't
>>> work with NOR Flash due to HW issue.
>>>
>>> Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
>>> Signed-off-by: Tom Rini <trini@ti.com>
>>> ---
>>>  arch/arm/cpu/armv7/omap3/mem.c        |   61
>> +++++++++++++++++++++++++++-----
>>>  arch/arm/include/asm/arch-omap3/mem.h |   15 +++++---
>>>  board/logicpd/am3517evm/am3517evm.h   |    2 +-
>>>  3 files changed, 61 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/arch/arm/cpu/armv7/omap3/mem.c
>> b/arch/arm/cpu/armv7/omap3/mem.c
>>> index 4a8c025..4ad3d12 100644
>>> --- a/arch/arm/cpu/armv7/omap3/mem.c
>>> +++ b/arch/arm/cpu/armv7/omap3/mem.c
>>> @@ -51,6 +51,17 @@ static const u32 gpmc_m_nand[GPMC_MAX_REG] = {
>>>
>>>  #endif
>>>
>>> +#if defined (CONFIG_CMD_FLASH)
>>> +static const u32 gpmc_nor[GPMC_MAX_REG] = {
>>> +	STNOR_GPMC_CONFIG1,
>>> +	STNOR_GPMC_CONFIG2,
>>> +	STNOR_GPMC_CONFIG3,
>>> +	STNOR_GPMC_CONFIG4,
>>> +	STNOR_GPMC_CONFIG5,
>>> +	STNOR_GPMC_CONFIG6, 0
>>> +};
>>> +#endif
>>
>> This does not seem to be the right place for these settings...
>> I think those must be board specific.
>>
> [Hiremath, Vaibhav] I think, yes this information is board specific, since NOR flash is mounted on board.
> 
> Actually we have followed existing NAND/OneNAND implementation here, and that's where we have this here.

Yeah, I know you did.
The thing is we can continue adding crap code, just because
it was like that before, but that is not a good argument.
Instead, IMO, if you need to touch that code, make an effort and
clean this at least a bit and then add a clean version of the code.
I think it worked like this at least last year.

> Also note that, in case of OMAP3/AM37x we have POP package where NAND/OneNAND part is fixed, so it makes sense to bring here.

No, not really, because, there are other packages besides POP,
and information on them should come from the board.

What can be done is, move the NOR information to a separate function/file
so it can be reused by all boards, but the board gets to decide
whether to use this or may be some other setting.

Also, are you certain that POP package will always have the same
parts on top of the SoC, because the pin out can stay the same, but
timing information can vary and that's where you need that separation.

> 
>>> +
>>>  #if defined(CONFIG_CMD_ONENAND)
>>>  static const u32 gpmc_onenand[GPMC_MAX_REG] = {
>>>  	ONENAND_GPMC_CONFIG1,
>>> @@ -118,21 +129,34 @@ void enable_gpmc_cs_config(const u32 *gpmc_config,
>> struct gpmc_cs *cs, u32 base,
>>>  	sdelay(2000);
>>>  }
>>>
>>> -/*****************************************************
>>> +/*
>>>   * gpmc_init(): init gpmc bus
>>> - * Init GPMC for x16, MuxMode (SDRAM in x32).
>>> - * This code can only be executed from SRAM or SDRAM.
>>> - *****************************************************/
>>> + * Init GPMC for x16, MuxMode (SDRAM in x32).  This code can only be
>>> + * executed from SRAM or SDRAM.  In the common case, we will disable
>>> + * and then configure chip select 0 for our needs (NAND or OneNAND).
>>> + * However, on the AM3517 EVM the way that NOR can be exposed requires
>> us
>>> + * to rethink this.  When NOR is enabled, it will be chip select 0 if
>>> + * we are booting from NOR or chip select 2 otherwise.  This requires
>> us
>>> + * to check the value we get from the SYSBOOT pins.
>>
>> All the above looks like board specific...
>>
>>> + */
>>>  void gpmc_init(void)
>>>  {
>>>  	/* putting a blanket check on GPMC based on ZeBu for now */
>>>  	gpmc_cfg = (struct gpmc *)GPMC_BASE;
>>> -#if defined(CONFIG_CMD_NAND) || defined(CONFIG_CMD_ONENAND)
>>> +#if defined(CONFIG_CMD_NAND) || defined(CONFIG_CMD_ONENAND) || \
>>> +	(defined(CONFIG_OMAP3_AM3517EVM) &&
>> defined(CONFIG_SYS_HAS_NORFLASH))
>>>  	const u32 *gpmc_config = NULL;
>>>  	u32 base = 0;
>>> -	u32 size = 0;
>>> +	u32 sz = 0;
>>>  #endif
>>>  	u32 config = 0;
>>> +	u32 nor_boot = 0;
>>> +
>>> +#if defined(CONFIG_OMAP3_AM3517EVM) && defined(CONFIG_SYS_HAS_NORFLASH)
>>> +	/* 0xD means NOR boot on AM35x */
>>> +	if (get_boot_type() == 0xD)
>>> +		nor_boot = 1;
>>> +#endif
>>>
>>>  	/* global settings */
>>>  	writel(0, &gpmc_cfg->irqenable); /* isr's sources masked */
>>> @@ -146,14 +170,31 @@ void gpmc_init(void)
>>>  	gpmc_config = gpmc_m_nand;
>>>
>>>  	base = PISMO1_NAND_BASE;
>>> -	size = PISMO1_NAND_SIZE;
>>> -	enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, size);
>>> +	sz = PISMO1_NAND_SIZE;
>>> +	if (!nor_boot)
>>> +		enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base,
>> sz);
>>> +	else
>>> +		enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[2], base,
>> sz);
>>> +
>>>  #endif
>>>
>>>  #if defined(CONFIG_CMD_ONENAND)
>>>  	gpmc_config = gpmc_onenand;
>>>  	base = PISMO1_ONEN_BASE;
>>> -	size = PISMO1_ONEN_SIZE;
>>> -	enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, size);
>>> +	sz = PISMO1_ONEN_SIZE;
>>> +	if (!nor_boot)
>>> +		enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base,
>> sz);
>>> +	else
>>> +		enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[2], base,
>> sz);
>>> +
>>> +#endif
>>> +
>>> +#if defined(CONFIG_OMAP3_AM3517EVM) && defined(CONFIG_SYS_HAS_NORFLASH)
>>> +	/* NOR - CS2 */
>>> +	gpmc_config = gpmc_nor;
>>> +	base = DEBUG_BASE;
>>> +	sz = GPMC_SIZE_64M;
>>> +	if (!nor_boot)
>>> +		enable_gpmc_cs_config(gpmc_nor, &gpmc_cfg->cs[2], base, sz);
>>>  #endif
>>>  }
>>
>> All the above look very hackish...
>> You just bring board specific code into a common location.
>> I don't think we should be doing it this way.
>> Instead, change the gpmc_init() function signature to get a parameter(s),
>> so it will be a generic function, that will initialize the right stuff
>> according to the parameters and will not have this board specific
>> ifdeffery crap.
>>
> [Hiremath, Vaibhav] Agreed, gpmc_init needs some cleanup to make it generic interface. And probably all data should get passed from board file.

Good, we have an agreement ;)

> 
>>> diff --git a/arch/arm/include/asm/arch-omap3/mem.h
>> b/arch/arm/include/asm/arch-omap3/mem.h
>>> index 5fd02d4..2f52481 100644
>>> --- a/arch/arm/include/asm/arch-omap3/mem.h
>>> +++ b/arch/arm/include/asm/arch-omap3/mem.h
>>> @@ -329,12 +329,15 @@ enum {
>>>  #define M_NAND_GPMC_CONFIG6	0x1f0f0A80
>>>  #define M_NAND_GPMC_CONFIG7	0x00000C44
>>>
>>> -#define STNOR_GPMC_CONFIG1	0x3
>>> -#define STNOR_GPMC_CONFIG2	0x00151501
>>> -#define STNOR_GPMC_CONFIG3	0x00060602
>>> -#define STNOR_GPMC_CONFIG4	0x11091109
>>> -#define STNOR_GPMC_CONFIG5	0x01141F1F
>>> -#define STNOR_GPMC_CONFIG6	0x000004c4
>>> +/*
>>> + * Configuration required for AM3517EVM PC28F640P30B85 Flash
>>> + */
>>> +#define STNOR_GPMC_CONFIG1	0x00001210
>>> +#define STNOR_GPMC_CONFIG2	0x00101001
>>> +#define STNOR_GPMC_CONFIG3	0x00020201
>>> +#define STNOR_GPMC_CONFIG4	0x0f031003
>>> +#define STNOR_GPMC_CONFIG5	0x000f1111
>>> +#define STNOR_GPMC_CONFIG6	0x0f030080
>>
>> I see also:
>> arch/arm/cpu/armv7/omap3/lowlevel_init.S:184:   .word STNOR_GPMC_CONFIG3
>> arch/arm/cpu/armv7/omap3/lowlevel_init.S:188:   .word STNOR_GPMC_CONFIG4
>> arch/arm/cpu/armv7/omap3/lowlevel_init.S:190:   .word STNOR_GPMC_CONFIG5
>>
>> I haven't looked into this, but will your change break anything else?
>>
> [Hiremath, Vaibhav] I do not think it breaks anything.
> 
>>>
>>>  #define SIBNOR_GPMC_CONFIG1	0x1200
>>>  #define SIBNOR_GPMC_CONFIG2	0x001f1f00
>>> diff --git a/board/logicpd/am3517evm/am3517evm.h
>> b/board/logicpd/am3517evm/am3517evm.h
>>> index 68d746c..17bb78d 100644
>>> --- a/board/logicpd/am3517evm/am3517evm.h
>>> +++ b/board/logicpd/am3517evm/am3517evm.h
>>> @@ -327,7 +327,7 @@ const omap3_sysinfo sysinfo = {
>>>  	MUX_VAL(CP(SYS_CLKREQ),		(IEN  | PTD | DIS | M0)) \
>>>  	MUX_VAL(CP(SYS_NIRQ),		(IEN  | PTU | EN  | M0)) \
>>>  	/*SYS_nRESWARM */\
>>> -	MUX_VAL(CP(SYS_NRESWARM),     	(IDIS | PTU | DIS | M4)) \
>>> +	MUX_VAL(CP(SYS_NRESWARM),     	(IDIS | PTU | EN  | M4)) \
>>>  							/* - GPIO30 */\
>>>  	MUX_VAL(CP(SYS_BOOT0),		(IEN  | PTD | DIS | M4)) /*GPIO_2*/\
>>>  							 /* - PEN_IRQ */\


-- 
Regards,
Igor.

[U-Boot] [PATCH 3/8] AM3517: Add NOR Flash boot mode Support

[Hiremath, Vaibhav]

> -----Original Message-----
> From: Igor Grinberg [mailto:grinberg at compulab.co.il]
> Sent: Thursday, December 08, 2011 7:18 PM
> To: Hiremath, Vaibhav
> Cc: Rini, Tom; u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH 3/8] AM3517: Add NOR Flash boot mode Support
> 
> Hi Vaibhav,
> 
> On 12/08/11 13:11, Hiremath, Vaibhav wrote:
> >
> >> -----Original Message-----
> >> From: Igor Grinberg [mailto:grinberg at compulab.co.il]
> >> Sent: Wednesday, December 07, 2011 7:55 PM
> >> To: Rini, Tom
> >> Cc: u-boot at lists.denx.de; Hiremath, Vaibhav
> >> Subject: Re: [U-Boot] [PATCH 3/8] AM3517: Add NOR Flash boot mode
> Support
> >>
> >> Hi Tom,
> >>
> >> On 12/06/11 17:49, Tom Rini wrote:
> >>> From: Vaibhav Hiremath <hvaibhav@ti.com>
> >>>
> >>> Please note that NOR Flash is located on Application board and
> requires
> >>> hardware modification to get NOR boot mode working.
> >>>
> >>> NOR Flash boot mode configuration -
> >>>
> >>>         - From baseboard remove R235 resistor.
> >>> 	- Set switch S11.3 position to "ON"
> >>> 	- Set S7 switch position to
> >>> 		 1  2   3   4   5
> >>> 		 -----------------
> >>> 		on off off off off
> >>>
> >>> Please note that, once you remove R235 resistor from the baseboard,
> you
> >>> will not be able to boot from NAND without Application board.
> >>> The GPMC_nCS0 is now routed through Application board.
> >>>
> >>> Please note that, <Rev4 revision of Application board doesn't
> >>> work with NOR Flash due to HW issue.
> >>>
> >>> Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
> >>> Signed-off-by: Tom Rini <trini@ti.com>
> >>> ---
> >>>  arch/arm/cpu/armv7/omap3/mem.c        |   61
> >> +++++++++++++++++++++++++++-----
> >>>  arch/arm/include/asm/arch-omap3/mem.h |   15 +++++---
> >>>  board/logicpd/am3517evm/am3517evm.h   |    2 +-
> >>>  3 files changed, 61 insertions(+), 17 deletions(-)
> >>>
> >>> diff --git a/arch/arm/cpu/armv7/omap3/mem.c
> >> b/arch/arm/cpu/armv7/omap3/mem.c
> >>> index 4a8c025..4ad3d12 100644
> >>> --- a/arch/arm/cpu/armv7/omap3/mem.c
> >>> +++ b/arch/arm/cpu/armv7/omap3/mem.c
> >>> @@ -51,6 +51,17 @@ static const u32 gpmc_m_nand[GPMC_MAX_REG] = {
> >>>
> >>>  #endif
> >>>
> >>> +#if defined (CONFIG_CMD_FLASH)
> >>> +static const u32 gpmc_nor[GPMC_MAX_REG] = {
> >>> +	STNOR_GPMC_CONFIG1,
> >>> +	STNOR_GPMC_CONFIG2,
> >>> +	STNOR_GPMC_CONFIG3,
> >>> +	STNOR_GPMC_CONFIG4,
> >>> +	STNOR_GPMC_CONFIG5,
> >>> +	STNOR_GPMC_CONFIG6, 0
> >>> +};
> >>> +#endif
> >>
> >> This does not seem to be the right place for these settings...
> >> I think those must be board specific.
> >>
> > [Hiremath, Vaibhav] I think, yes this information is board specific,
> since NOR flash is mounted on board.
> >
> > Actually we have followed existing NAND/OneNAND implementation here, and
> that's where we have this here.
> 
> Yeah, I know you did.
> The thing is we can continue adding crap code, just because
> it was like that before, but that is not a good argument.
> Instead, IMO, if you need to touch that code, make an effort and
> clean this at least a bit and then add a clean version of the code.
> I think it worked like this at least last year.
> 
> > Also note that, in case of OMAP3/AM37x we have POP package where
> NAND/OneNAND part is fixed, so it makes sense to bring here.
> 
> No, not really, because, there are other packages besides POP,
> and information on them should come from the board.
> 
> What can be done is, move the NOR information to a separate function/file
> so it can be reused by all boards, but the board gets to decide
> whether to use this or may be some other setting.
> 
> Also, are you certain that POP package will always have the same
> parts on top of the SoC, because the pin out can stay the same, but
> timing information can vary and that's where you need that separation.
> 
Fair enough,

I was just trying to say, this could be the reason behind why this definitions kept here in this file.

Thanks,
Vaibhav

> >
> >>> +
> >>>  #if defined(CONFIG_CMD_ONENAND)
> >>>  static const u32 gpmc_onenand[GPMC_MAX_REG] = {
> >>>  	ONENAND_GPMC_CONFIG1,
> >>> @@ -118,21 +129,34 @@ void enable_gpmc_cs_config(const u32
> *gpmc_config,
> >> struct gpmc_cs *cs, u32 base,
> >>>  	sdelay(2000);
> >>>  }
> >>>
> >>> -/*****************************************************
> >>> +/*
> >>>   * gpmc_init(): init gpmc bus
> >>> - * Init GPMC for x16, MuxMode (SDRAM in x32).
> >>> - * This code can only be executed from SRAM or SDRAM.
> >>> - *****************************************************/
> >>> + * Init GPMC for x16, MuxMode (SDRAM in x32).  This code can only be
> >>> + * executed from SRAM or SDRAM.  In the common case, we will disable
> >>> + * and then configure chip select 0 for our needs (NAND or OneNAND).
> >>> + * However, on the AM3517 EVM the way that NOR can be exposed
> requires
> >> us
> >>> + * to rethink this.  When NOR is enabled, it will be chip select 0 if
> >>> + * we are booting from NOR or chip select 2 otherwise.  This requires
> >> us
> >>> + * to check the value we get from the SYSBOOT pins.
> >>
> >> All the above looks like board specific...
> >>
> >>> + */
> >>>  void gpmc_init(void)
> >>>  {
> >>>  	/* putting a blanket check on GPMC based on ZeBu for now */
> >>>  	gpmc_cfg = (struct gpmc *)GPMC_BASE;
> >>> -#if defined(CONFIG_CMD_NAND) || defined(CONFIG_CMD_ONENAND)
> >>> +#if defined(CONFIG_CMD_NAND) || defined(CONFIG_CMD_ONENAND) || \
> >>> +	(defined(CONFIG_OMAP3_AM3517EVM) &&
> >> defined(CONFIG_SYS_HAS_NORFLASH))
> >>>  	const u32 *gpmc_config = NULL;
> >>>  	u32 base = 0;
> >>> -	u32 size = 0;
> >>> +	u32 sz = 0;
> >>>  #endif
> >>>  	u32 config = 0;
> >>> +	u32 nor_boot = 0;
> >>> +
> >>> +#if defined(CONFIG_OMAP3_AM3517EVM) &&
> defined(CONFIG_SYS_HAS_NORFLASH)
> >>> +	/* 0xD means NOR boot on AM35x */
> >>> +	if (get_boot_type() == 0xD)
> >>> +		nor_boot = 1;
> >>> +#endif
> >>>
> >>>  	/* global settings */
> >>>  	writel(0, &gpmc_cfg->irqenable); /* isr's sources masked */
> >>> @@ -146,14 +170,31 @@ void gpmc_init(void)
> >>>  	gpmc_config = gpmc_m_nand;
> >>>
> >>>  	base = PISMO1_NAND_BASE;
> >>> -	size = PISMO1_NAND_SIZE;
> >>> -	enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, size);
> >>> +	sz = PISMO1_NAND_SIZE;
> >>> +	if (!nor_boot)
> >>> +		enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base,
> >> sz);
> >>> +	else
> >>> +		enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[2], base,
> >> sz);
> >>> +
> >>>  #endif
> >>>
> >>>  #if defined(CONFIG_CMD_ONENAND)
> >>>  	gpmc_config = gpmc_onenand;
> >>>  	base = PISMO1_ONEN_BASE;
> >>> -	size = PISMO1_ONEN_SIZE;
> >>> -	enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base, size);
> >>> +	sz = PISMO1_ONEN_SIZE;
> >>> +	if (!nor_boot)
> >>> +		enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[0], base,
> >> sz);
> >>> +	else
> >>> +		enable_gpmc_cs_config(gpmc_config, &gpmc_cfg->cs[2], base,
> >> sz);
> >>> +
> >>> +#endif
> >>> +
> >>> +#if defined(CONFIG_OMAP3_AM3517EVM) &&
> defined(CONFIG_SYS_HAS_NORFLASH)
> >>> +	/* NOR - CS2 */
> >>> +	gpmc_config = gpmc_nor;
> >>> +	base = DEBUG_BASE;
> >>> +	sz = GPMC_SIZE_64M;
> >>> +	if (!nor_boot)
> >>> +		enable_gpmc_cs_config(gpmc_nor, &gpmc_cfg->cs[2], base, sz);
> >>>  #endif
> >>>  }
> >>
> >> All the above look very hackish...
> >> You just bring board specific code into a common location.
> >> I don't think we should be doing it this way.
> >> Instead, change the gpmc_init() function signature to get a
> parameter(s),
> >> so it will be a generic function, that will initialize the right stuff
> >> according to the parameters and will not have this board specific
> >> ifdeffery crap.
> >>
> > [Hiremath, Vaibhav] Agreed, gpmc_init needs some cleanup to make it
> generic interface. And probably all data should get passed from board file.
> 
> Good, we have an agreement ;)
> 
> >
> >>> diff --git a/arch/arm/include/asm/arch-omap3/mem.h
> >> b/arch/arm/include/asm/arch-omap3/mem.h
> >>> index 5fd02d4..2f52481 100644
> >>> --- a/arch/arm/include/asm/arch-omap3/mem.h
> >>> +++ b/arch/arm/include/asm/arch-omap3/mem.h
> >>> @@ -329,12 +329,15 @@ enum {
> >>>  #define M_NAND_GPMC_CONFIG6	0x1f0f0A80
> >>>  #define M_NAND_GPMC_CONFIG7	0x00000C44
> >>>
> >>> -#define STNOR_GPMC_CONFIG1	0x3
> >>> -#define STNOR_GPMC_CONFIG2	0x00151501
> >>> -#define STNOR_GPMC_CONFIG3	0x00060602
> >>> -#define STNOR_GPMC_CONFIG4	0x11091109
> >>> -#define STNOR_GPMC_CONFIG5	0x01141F1F
> >>> -#define STNOR_GPMC_CONFIG6	0x000004c4
> >>> +/*
> >>> + * Configuration required for AM3517EVM PC28F640P30B85 Flash
> >>> + */
> >>> +#define STNOR_GPMC_CONFIG1	0x00001210
> >>> +#define STNOR_GPMC_CONFIG2	0x00101001
> >>> +#define STNOR_GPMC_CONFIG3	0x00020201
> >>> +#define STNOR_GPMC_CONFIG4	0x0f031003
> >>> +#define STNOR_GPMC_CONFIG5	0x000f1111
> >>> +#define STNOR_GPMC_CONFIG6	0x0f030080
> >>
> >> I see also:
> >> arch/arm/cpu/armv7/omap3/lowlevel_init.S:184:   .word
> STNOR_GPMC_CONFIG3
> >> arch/arm/cpu/armv7/omap3/lowlevel_init.S:188:   .word
> STNOR_GPMC_CONFIG4
> >> arch/arm/cpu/armv7/omap3/lowlevel_init.S:190:   .word
> STNOR_GPMC_CONFIG5
> >>
> >> I haven't looked into this, but will your change break anything else?
> >>
> > [Hiremath, Vaibhav] I do not think it breaks anything.
> >
> >>>
> >>>  #define SIBNOR_GPMC_CONFIG1	0x1200
> >>>  #define SIBNOR_GPMC_CONFIG2	0x001f1f00
> >>> diff --git a/board/logicpd/am3517evm/am3517evm.h
> >> b/board/logicpd/am3517evm/am3517evm.h
> >>> index 68d746c..17bb78d 100644
> >>> --- a/board/logicpd/am3517evm/am3517evm.h
> >>> +++ b/board/logicpd/am3517evm/am3517evm.h
> >>> @@ -327,7 +327,7 @@ const omap3_sysinfo sysinfo = {
> >>>  	MUX_VAL(CP(SYS_CLKREQ),		(IEN  | PTD | DIS | M0)) \
> >>>  	MUX_VAL(CP(SYS_NIRQ),		(IEN  | PTU | EN  | M0)) \
> >>>  	/*SYS_nRESWARM */\
> >>> -	MUX_VAL(CP(SYS_NRESWARM),     	(IDIS | PTU | DIS | M4)) \
> >>> +	MUX_VAL(CP(SYS_NRESWARM),     	(IDIS | PTU | EN  | M4)) \
> >>>  							/* - GPIO30 */\
> >>>  	MUX_VAL(CP(SYS_BOOT0),		(IEN  | PTD | DIS | M4)) /*GPIO_2*/\
> >>>  							 /* - PEN_IRQ */\
> 
> 
> --
> Regards,
> Igor.

[U-Boot] [PATCH 4/8] AM3517 EVM: Add uEnv.txt to the default bootcmd

[Tom Rini]

This adds uEnv.txt (and optargs/uenvcmd) support to the MMC boot section

Signed-off-by: Tom Rini <trini@ti.com>
---
 include/configs/am3517_evm.h |   22 ++++++++++++++++++++--
 1 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/include/configs/am3517_evm.h b/include/configs/am3517_evm.h
index d44eeec..39f1a82 100644
--- a/include/configs/am3517_evm.h
+++ b/include/configs/am3517_evm.h
@@ -191,10 +191,20 @@
 	"loadaddr=0x82000000\0" \
 	"console=ttyO2,115200n8\0" \
 	"mmcdev=0\0" \
-	"mmcargs=setenv bootargs console=${console} " \
+	"bootenv=uEnv.txt\0" \
+	"loadbootenv=fatload mmc ${mmc_dev} ${loadaddr} ${bootenv}\0" \
+	"importbootenv=echo Importing environment from mmc ...; " \
+		"env import -t $loadaddr $filesize\0" \
+	"optargs=\0" \
+	"bootargs_defaults=setenv bootargs " \
+		"console=${console} " \
+		"${optargs}\0" \
+	"mmcargs=run bootargs_defaults; " \
+		"setenv bootargs ${bootargs} " \
 		"root=/dev/mmcblk0p2 rw " \
 		"rootfstype=ext3 rootwait\0" \
-	"nandargs=setenv bootargs console=${console} " \
+	"nandargs=run bootargs_defaults; " \
+		"setenv bootargs ${bootargs} " \
 		"root=/dev/mtdblock4 rw " \
 		"rootfstype=jffs2\0" \
 	"loadbootscript=fatload mmc ${mmcdev} ${loadaddr} boot.scr\0" \
@@ -214,6 +224,14 @@
 		"if run loadbootscript; then " \
 			"run bootscript; " \
 		"else " \
+			"if run loadbootenv; then " \
+				"echo Loaded environment from ${bootenv};" \
+				"run importbootenv;" \
+			"fi;" \
+			"if test -n $uenvcmd; then " \
+				"echo Running uenvcmd ...;" \
+				"run uenvcmd;" \
+			"fi;" \
 			"if run loaduimage; then " \
 				"run mmcboot; " \
 			"else run nandboot; " \
-- 
1.7.0.4

[U-Boot] [PATCH 5/8] AM3517 EVM: Add am3517_evm_norflash and _norflash_boot targets

[Tom Rini]

These build targets will make a U-Boot that knows about the NOR
flash that can be enabled on this board (see previous commits for
details) and a U-Boot that can run from NOR flash (u-boot.bin).

When we are building to run from NOR flash we makes changes to
the default environment in order to run the system fully from
NOR flash.  This results in a lot of 'noise' in terms of moving
NAND-specific config options to be in one place that we can
key off of.  We also modify the bootcmd to use the flash the
user has chosen (by building for NOR, or NAND).

Signed-off-by: Tom Rini <trini@ti.com>
---
 boards.cfg                   |    2 +
 include/configs/am3517_evm.h |  142 +++++++++++++++++++++++++----------------
 2 files changed, 88 insertions(+), 56 deletions(-)

diff --git a/boards.cfg b/boards.cfg
index 50f2e5f..7b0cfc5 100644
--- a/boards.cfg
+++ b/boards.cfg
@@ -187,6 +187,8 @@ igep0020                     arm         armv7       igep0020            isee
 igep0030                     arm         armv7       igep0030            isee           omap3
 am3517_crane                 arm         armv7       am3517crane         ti             omap3
 am3517_evm                   arm         armv7       am3517evm           logicpd        omap3
+am3517_evm_norflash          arm         armv7       am3517evm           logicpd        omap3       am3517_evm:SYS_HAS_NORFLASH
+am3517_evm_norflash_boot     arm         armv7       am3517evm           logicpd        omap3       am3517_evm:SYS_HAS_NORFLASH,SYS_BOOT_NORFLASH
 dig297                       arm         armv7       dig297              comelit        omap3
 omap3_zoom1                  arm         armv7       zoom1               logicpd        omap3
 omap3_zoom2                  arm         armv7       zoom2               logicpd        omap3
diff --git a/include/configs/am3517_evm.h b/include/configs/am3517_evm.h
index 39f1a82..8eb693a 100644
--- a/include/configs/am3517_evm.h
+++ b/include/configs/am3517_evm.h
@@ -147,12 +147,10 @@
 #define CONFIG_CMD_DHCP
 #define CONFIG_CMD_PING
 
-#undef CONFIG_CMD_FLASH		/* flinfo, erase, protect	*/
 #undef CONFIG_CMD_FPGA		/* FPGA configuration Support	*/
 #undef CONFIG_CMD_IMI		/* iminfo			*/
 #undef CONFIG_CMD_IMLS		/* List all found images	*/
 
-#define CONFIG_SYS_NO_FLASH
 #define CONFIG_HARD_I2C			1
 #define CONFIG_SYS_I2C_SPEED		100000
 #define CONFIG_SYS_I2C_SLAVE		1
@@ -175,19 +173,16 @@
 							/* NAND devices */
 #define CONFIG_SYS_64BIT_VSPRINTF		/* needed for nand_util.c */
 
-#define CONFIG_JFFS2_NAND
-/* nand device jffs2 lives on */
-#define CONFIG_JFFS2_DEV		"nand0"
-/* start of jffs2 partition */
-#define CONFIG_JFFS2_PART_OFFSET	0x680000
-#define CONFIG_JFFS2_PART_SIZE		0xf980000	/* sz of jffs2 part */
+#define CONFIG_NAND_OMAP_GPMC
+#define GPMC_NAND_ECC_LP_x16_LAYOUT	1
 
 /* Environment information */
 #define CONFIG_BOOTDELAY	10
 
 #define CONFIG_BOOTFILE		"uImage"
 
-#define CONFIG_EXTRA_ENV_SETTINGS \
+/* Basic 'extra' env variables */
+#define EXTRA_ENV_SETTINGS \
 	"loadaddr=0x82000000\0" \
 	"console=ttyO2,115200n8\0" \
 	"mmcdev=0\0" \
@@ -203,10 +198,6 @@
 		"setenv bootargs ${bootargs} " \
 		"root=/dev/mmcblk0p2 rw " \
 		"rootfstype=ext3 rootwait\0" \
-	"nandargs=run bootargs_defaults; " \
-		"setenv bootargs ${bootargs} " \
-		"root=/dev/mtdblock4 rw " \
-		"rootfstype=jffs2\0" \
 	"loadbootscript=fatload mmc ${mmcdev} ${loadaddr} boot.scr\0" \
 	"bootscript=echo Running bootscript from mmc ...; " \
 		"source ${loadaddr}\0" \
@@ -214,10 +205,6 @@
 	"mmcboot=echo Booting from mmc ...; " \
 		"run mmcargs; " \
 		"bootm ${loadaddr}\0" \
-	"nandboot=echo Booting from nand ...; " \
-		"run nandargs; " \
-		"nand read ${loadaddr} 280000 400000; " \
-		"bootm ${loadaddr}\0" \
 
 #define CONFIG_BOOTCOMMAND \
 	"if mmc rescan ${mmcdev}; then " \
@@ -234,10 +221,10 @@
 			"fi;" \
 			"if run loaduimage; then " \
 				"run mmcboot; " \
-			"else run nandboot; " \
+			"else run " FLASHBOOT "; " \
 			"fi; " \
 		"fi; " \
-	"else run nandboot; fi"
+	"else run " FLASHBOOT "; fi"
 
 #define CONFIG_AUTO_COMPLETE	1
 /*
@@ -292,47 +279,81 @@
  * FLASH and environment organization
  */
 
-/* **** PISMO SUPPORT *** */
-
 /* Configure the PISMO */
 #define PISMO1_NAND_SIZE		GPMC_SIZE_128M
 #define PISMO1_ONEN_SIZE		GPMC_SIZE_128M
 
-#define CONFIG_SYS_MAX_FLASH_SECT	520	/* max number of sectors */
-						/* on one chip */
-#define CONFIG_SYS_MAX_FLASH_BANKS	2	/* max number of flash banks */
-#define CONFIG_SYS_MONITOR_LEN		(256 << 10)	/* Reserve 2 sectors */
-
-#if defined(CONFIG_CMD_NAND)
-#define CONFIG_SYS_FLASH_BASE		PISMO1_NAND_BASE
-#endif
+/* General */
+#define CONFIG_SYS_MONITOR_LEN		(256 << 10)	/* Reserve 256 KiB */
 
+/*
+ * CFI FLASH driver setup.  Please note that, first 4 blocks are of 32K and
+ * rest all blocks are 128K.
+ */
+#if defined (CONFIG_SYS_HAS_NORFLASH)
+#define CONFIG_CMD_FLASH			/* flinfo, erase, protect */
+#define CONFIG_SYS_FLASH_BASE		DEBUG_BASE
+#define CONFIG_SYS_FLASH_CFI			/* use CFI geometry data */
+#define CONFIG_FLASH_CFI_DRIVER
+#define CONFIG_SYS_FLASH_USE_BUFFER_WRITE 	/* ~10x faster writes */
+#define CONFIG_SYS_FLASH_PROTECTION		/* hardware sector protection */
+#define CONFIG_SYS_FLASH_EMPTY_INFO		/* flinfo 'E' for empty */
+#define CONFIG_SYS_FLASH_BANKS_LIST	{CONFIG_SYS_FLASH_BASE}
+#define CONFIG_SYS_MAX_FLASH_BANKS	1	/* max number of flash banks */
+#define CONFIG_SYS_MAX_FLASH_SECT	512	/* max sectors on one chip */
+#define CONFIG_SYS_FLASH_CFI_WIDTH	2
+#define PHYS_FLASH_SIZE			(8 << 20)
 /* Monitor at start of flash */
 #define CONFIG_SYS_MONITOR_BASE		CONFIG_SYS_FLASH_BASE
+#else
+/* No support for CFI flash. */
+#undef CONFIG_CMD_FLASH
+#define CONFIG_SYS_NO_FLASH
+#define CONFIG_SYS_MAX_FLASH_BANKS	0
+#endif
 
-#define CONFIG_NAND_OMAP_GPMC
-#define GPMC_NAND_ECC_LP_x16_LAYOUT	1
-#define CONFIG_ENV_IS_IN_NAND		1
+/* Environment location */
+#ifdef CONFIG_SYS_BOOT_NORFLASH
+#define CONFIG_ENV_IS_IN_FLASH
+#define CONFIG_ENV_OFFSET		0x80000	/* environment starts here */
+/* NOR related env and boot */
+#define FLASHBOOT			"norboot"
+#define CONFIG_EXTRA_ENV_SETTINGS	EXTRA_ENV_SETTINGS \
+	"norargs=run bootargs_defaults; " \
+		"setenv bootargs ${bootargs} " \
+		"root=/dev/mtdblock3 rw " \
+		"rootfstype=jffs2\0" \
+	"norboot=echo Booting from nor ...; " \
+		"run norargs; " \
+		"bootm 0x080A0000; \0"
+/* JFFS2 */
+/* start of jffs2 partition */
+#define CONFIG_JFFS2_PART_OFFSET	0x400000
+#define CONFIG_JFFS2_PART_SIZE		0x400000	/* sz of jffs2 part */
+#else
+/* ENV resides in NAND */
+#define CONFIG_ENV_IS_IN_NAND
 #define SMNAND_ENV_OFFSET		0x260000 /* environment starts here */
-
-#define CONFIG_SYS_ENV_SECT_SIZE	(128 << 10)	/* 128 KiB */
 #define CONFIG_ENV_OFFSET		SMNAND_ENV_OFFSET
-#define CONFIG_ENV_ADDR			SMNAND_ENV_OFFSET
-
-/*-----------------------------------------------------------------------
- * CFI FLASH driver setup
- */
-/* timeout values are in ticks */
-#define CONFIG_SYS_FLASH_ERASE_TOUT	(100 * CONFIG_SYS_HZ)
-#define CONFIG_SYS_FLASH_WRITE_TOUT	(100 * CONFIG_SYS_HZ)
-
-/* Flash banks JFFS2 should use */
-#define CONFIG_SYS_MAX_MTD_BANKS	(CONFIG_SYS_MAX_FLASH_BANKS + \
-					CONFIG_SYS_MAX_NAND_DEVICE)
-#define CONFIG_SYS_JFFS2_MEM_NAND
-/* use flash_info[2] */
-#define CONFIG_SYS_JFFS2_FIRST_BANK	CONFIG_SYS_MAX_FLASH_BANKS
-#define CONFIG_SYS_JFFS2_NUM_BANKS	1
+/* NAND related env and boot */
+#define FLASHBOOT			"nandboot"
+#define CONFIG_EXTRA_ENV_SETTINGS	EXTRA_ENV_SETTINGS \
+	"nandargs=run bootargs_defaults; " \
+		"setenv bootargs ${bootargs} " \
+		"root=/dev/mtdblock4 rw " \
+		"rootfstype=jffs2\0" \
+	"nandboot=echo Booting from nand ...; " \
+		"run nandargs; " \
+		"nand read ${loadaddr} 280000 500000; " \
+		"bootm ${loadaddr}\0"
+/* JFFS2 */
+#define CONFIG_JFFS2_NAND
+/* nand device jffs2 lives on */
+#define CONFIG_JFFS2_DEV		"nand0"
+/* start of jffs2 partition */
+#define CONFIG_JFFS2_PART_OFFSET	0x780000
+#define CONFIG_JFFS2_PART_SIZE		0x1f880000	/* sz of jffs2 part */
+#endif
 
 #define CONFIG_SYS_SDRAM_BASE		PHYS_SDRAM_1
 #define CONFIG_SYS_INIT_RAM_ADDR	0x4020f800
@@ -341,9 +362,12 @@
 					 CONFIG_SYS_INIT_RAM_SIZE - \
 					 GENERATED_GBL_DATA_SIZE)
 
-/* Defines for SPL */
+/*
+ * SPL support.  No need to build all of this if we are going to be
+ * booting from NOR instead.
+ */
+#ifndef CONFIG_SYS_BOOT_NORFLASH
 #define CONFIG_SPL
-#define CONFIG_SPL_NAND_SIMPLE
 #define CONFIG_SPL_TEXT_BASE		0x40200800
 #define CONFIG_SPL_MAX_SIZE		(45 * 1024)
 #define CONFIG_SPL_STACK		LOW_LEVEL_SRAM_STACK
@@ -363,11 +387,12 @@
 #define CONFIG_SPL_MMC_SUPPORT
 #define CONFIG_SPL_FAT_SUPPORT
 #define CONFIG_SPL_SERIAL_SUPPORT
-#define CONFIG_SPL_NAND_SUPPORT
 #define CONFIG_SPL_POWER_SUPPORT
 #define CONFIG_SPL_LDSCRIPT		"$(CPUDIR)/omap-common/u-boot-spl.lds"
 
 /* NAND boot config */
+#define CONFIG_SPL_NAND_SUPPORT
+#define CONFIG_SPL_NAND_SIMPLE
 #define CONFIG_SYS_NAND_5_ADDR_CYCLE
 #define CONFIG_SYS_NAND_PAGE_COUNT	64
 #define CONFIG_SYS_NAND_PAGE_SIZE	2048
@@ -380,19 +405,24 @@
 #define CONFIG_SYS_NAND_ECCBYTES	3
 #define CONFIG_SYS_NAND_ECCSTEPS	(CONFIG_SYS_NAND_PAGE_SIZE / \
 						CONFIG_SYS_NAND_ECCSIZE)
-#define CONFIG_SYS_NAND_ECCTOTAL	(CONFIG_SYS_NAND_ECCBYTES * \
+#define CONFIG_SYS_NAND_ECCTOTAL       (CONFIG_SYS_NAND_ECCBYTES * \
 						CONFIG_SYS_NAND_ECCSTEPS)
-#define CONFIG_SYS_NAND_U_BOOT_START	CONFIG_SYS_TEXT_BASE
+#define CONFIG_SYS_NAND_U_BOOT_START   CONFIG_SYS_TEXT_BASE
 #define CONFIG_SYS_NAND_U_BOOT_OFFS	0x80000
+#endif
 
 /*
+ * For NOR we need to link at where NOR is mapped at.  Otherwise, we are
  * 1MB into the SDRAM to allow for SPL's bss@the beginning of SDRAM
  * 64 bytes before this address should be set aside for u-boot.img's
  * header. That is 0x800FFFC0--0x80100000 should not be used for any
  * other needs.
  */
+#ifdef CONFIG_SYS_BOOT_NORFLASH
+#define CONFIG_SYS_TEXT_BASE		0x08000000
+#else
 #define CONFIG_SYS_TEXT_BASE		0x80100000
+#endif
 #define CONFIG_SYS_SPL_MALLOC_START	0x80208000
 #define CONFIG_SYS_SPL_MALLOC_SIZE	0x100000
-
 #endif /* __CONFIG_H */
-- 
1.7.0.4

[U-Boot] [PATCH 6/8] AM35xx: Read and set ethaddr for EMAC

[Tom Rini]

From: Steve Kipisz <s-kipisz2@ti.com>

Signed-off-by: Steve Kipisz <s-kipisz2@ti.com>
Signed-off-by: Tom Rini <trini@ti.com>
---
 arch/arm/cpu/armv7/omap3/emac.c             |   20 +++++++++++++++++++-
 arch/arm/include/asm/arch-omap3/emac_defs.h |    3 +++
 2 files changed, 22 insertions(+), 1 deletions(-)

diff --git a/arch/arm/cpu/armv7/omap3/emac.c b/arch/arm/cpu/armv7/omap3/emac.c
index 14667f1..d400bef 100644
--- a/arch/arm/cpu/armv7/omap3/emac.c
+++ b/arch/arm/cpu/armv7/omap3/emac.c
@@ -26,6 +26,7 @@
 #include <netdev.h>
 #include <asm/io.h>
 #include <asm/arch/am35x_def.h>
+#include <asm/arch/emac_defs.h>
 
 /*
  * Initializes on-chip ethernet controllers.
@@ -33,12 +34,29 @@
  */
 int cpu_eth_init(bd_t *bis)
 {
-	u32 reset;
+	u32 reset, msb, lsb;
+	u_int8_t macaddr[6];
+	int i;
 
 	/* ensure that the module is out of reset */
 	reset = readl(&am35x_scm_general_regs->ip_sw_reset);
 	reset &= ~CPGMACSS_SW_RST;
 	writel(reset, &am35x_scm_general_regs->ip_sw_reset);
 
+	/* Read MAC address */
+	msb = readl(EMAC_MACADDR_MSB);
+	lsb = readl(EMAC_MACADDR_LSB);
+
+	for (i = 0; i < 3; i++) {
+		macaddr[5 - i] = lsb & 0xFF;
+		lsb >>= 8;
+	}
+
+	for (i = 0; i < 3; i++) {
+		macaddr[2 - i] = msb & 0xFF;
+		msb >>= 8;
+	}
+	eth_setenv_enetaddr("ethaddr", macaddr);
+
 	return davinci_emac_initialize();
 }
diff --git a/arch/arm/include/asm/arch-omap3/emac_defs.h b/arch/arm/include/asm/arch-omap3/emac_defs.h
index 8506c55..c3c96c0 100644
--- a/arch/arm/include/asm/arch-omap3/emac_defs.h
+++ b/arch/arm/include/asm/arch-omap3/emac_defs.h
@@ -42,6 +42,9 @@
 #define EMAC_MDIO_BASE_ADDR            0x5C030000
 #define EMAC_HW_RAM_ADDR               0x01E20000
 
+#define EMAC_MACADDR_LSB	       0x48002380
+#define EMAC_MACADDR_MSB	       0x48002384
+
 #define EMAC_MDIO_BUS_FREQ             166000000       /* 166 MHZ check */
 #define EMAC_MDIO_CLOCK_FREQ           1000000         /* 2.0 MHz */
 
-- 
1.7.0.4

[U-Boot] [PATCH 6/8] AM35xx: Read and set ethaddr for EMAC

[Igor Grinberg]

Hi Steve, Tom,

On 12/06/11 17:49, Tom Rini wrote:
> From: Steve Kipisz <s-kipisz2@ti.com>
> 
> Signed-off-by: Steve Kipisz <s-kipisz2@ti.com>
> Signed-off-by: Tom Rini <trini@ti.com>
> ---
>  arch/arm/cpu/armv7/omap3/emac.c             |   20 +++++++++++++++++++-
>  arch/arm/include/asm/arch-omap3/emac_defs.h |    3 +++
>  2 files changed, 22 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv7/omap3/emac.c b/arch/arm/cpu/armv7/omap3/emac.c
> index 14667f1..d400bef 100644
> --- a/arch/arm/cpu/armv7/omap3/emac.c
> +++ b/arch/arm/cpu/armv7/omap3/emac.c
> @@ -26,6 +26,7 @@
>  #include <netdev.h>
>  #include <asm/io.h>
>  #include <asm/arch/am35x_def.h>
> +#include <asm/arch/emac_defs.h>
>  
>  /*
>   * Initializes on-chip ethernet controllers.
> @@ -33,12 +34,29 @@
>   */
>  int cpu_eth_init(bd_t *bis)
>  {
> -	u32 reset;
> +	u32 reset, msb, lsb;
> +	u_int8_t macaddr[6];
> +	int i;
>  
>  	/* ensure that the module is out of reset */
>  	reset = readl(&am35x_scm_general_regs->ip_sw_reset);
>  	reset &= ~CPGMACSS_SW_RST;
>  	writel(reset, &am35x_scm_general_regs->ip_sw_reset);
>  
> +	/* Read MAC address */
> +	msb = readl(EMAC_MACADDR_MSB);
> +	lsb = readl(EMAC_MACADDR_LSB);
> +
> +	for (i = 0; i < 3; i++) {
> +		macaddr[5 - i] = lsb & 0xFF;
> +		lsb >>= 8;
> +	}
> +
> +	for (i = 0; i < 3; i++) {
> +		macaddr[2 - i] = msb & 0xFF;
> +		msb >>= 8;
> +	}
> +	eth_setenv_enetaddr("ethaddr", macaddr);
> +

This is a wrong place for this code...
You force every board to use the EFUSE'd MAC address - this is wrong.
The board must have a freedom to choose what MAC address it wants to use.
You don't even check if ethaddr variable is already set...

What you can do is put this implementation into a separate function
and let board to make a decision if it wants to call it or use another
MAC address.
Something like:
int am3517_get_efuse_enetaddr(u8 *enetaddr)
{
	/* read the address and shift or whatever */
	...
	return is_valid_ether_addr(enetaddr);
}

>  	return davinci_emac_initialize();
>  }
> diff --git a/arch/arm/include/asm/arch-omap3/emac_defs.h b/arch/arm/include/asm/arch-omap3/emac_defs.h
> index 8506c55..c3c96c0 100644
> --- a/arch/arm/include/asm/arch-omap3/emac_defs.h
> +++ b/arch/arm/include/asm/arch-omap3/emac_defs.h
> @@ -42,6 +42,9 @@
>  #define EMAC_MDIO_BASE_ADDR            0x5C030000
>  #define EMAC_HW_RAM_ADDR               0x01E20000
>  
> +#define EMAC_MACADDR_LSB	       0x48002380
> +#define EMAC_MACADDR_MSB	       0x48002384
> +
>  #define EMAC_MDIO_BUS_FREQ             166000000       /* 166 MHZ check */
>  #define EMAC_MDIO_CLOCK_FREQ           1000000         /* 2.0 MHz */
>  

-- 
Regards,
Igor.

[U-Boot] [PATCH 6/8] AM35xx: Read and set ethaddr for EMAC

[Tom Rini]

On 12/07/2011 07:48 AM, Igor Grinberg wrote:
> Hi Steve, Tom,
> 
> On 12/06/11 17:49, Tom Rini wrote:
>> From: Steve Kipisz <s-kipisz2@ti.com>
>>
>> Signed-off-by: Steve Kipisz <s-kipisz2@ti.com>
>> Signed-off-by: Tom Rini <trini@ti.com>
>> ---
>>  arch/arm/cpu/armv7/omap3/emac.c             |   20 +++++++++++++++++++-
>>  arch/arm/include/asm/arch-omap3/emac_defs.h |    3 +++
>>  2 files changed, 22 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/cpu/armv7/omap3/emac.c b/arch/arm/cpu/armv7/omap3/emac.c
>> index 14667f1..d400bef 100644
>> --- a/arch/arm/cpu/armv7/omap3/emac.c
>> +++ b/arch/arm/cpu/armv7/omap3/emac.c
>> @@ -26,6 +26,7 @@
>>  #include <netdev.h>
>>  #include <asm/io.h>
>>  #include <asm/arch/am35x_def.h>
>> +#include <asm/arch/emac_defs.h>
>>  
>>  /*
>>   * Initializes on-chip ethernet controllers.
>> @@ -33,12 +34,29 @@
>>   */
>>  int cpu_eth_init(bd_t *bis)
>>  {
>> -	u32 reset;
>> +	u32 reset, msb, lsb;
>> +	u_int8_t macaddr[6];
>> +	int i;
>>  
>>  	/* ensure that the module is out of reset */
>>  	reset = readl(&am35x_scm_general_regs->ip_sw_reset);
>>  	reset &= ~CPGMACSS_SW_RST;
>>  	writel(reset, &am35x_scm_general_regs->ip_sw_reset);
>>  
>> +	/* Read MAC address */
>> +	msb = readl(EMAC_MACADDR_MSB);
>> +	lsb = readl(EMAC_MACADDR_LSB);
>> +
>> +	for (i = 0; i < 3; i++) {
>> +		macaddr[5 - i] = lsb & 0xFF;
>> +		lsb >>= 8;
>> +	}
>> +
>> +	for (i = 0; i < 3; i++) {
>> +		macaddr[2 - i] = msb & 0xFF;
>> +		msb >>= 8;
>> +	}
>> +	eth_setenv_enetaddr("ethaddr", macaddr);
>> +
> 
> This is a wrong place for this code...
> You force every board to use the EFUSE'd MAC address - this is wrong.
> The board must have a freedom to choose what MAC address it wants to use.
> You don't even check if ethaddr variable is already set...
> 
> What you can do is put this implementation into a separate function
> and let board to make a decision if it wants to call it or use another
> MAC address.
> Something like:
> int am3517_get_efuse_enetaddr(u8 *enetaddr)
> {
> 	/* read the address and shift or whatever */
> 	...
> 	return is_valid_ether_addr(enetaddr);
> }

That's reasonable enough.  I wasn't sure where the custom boards were
getting their MAC as the EVM and Crane both need this change.

-- 
Tom

[U-Boot] [PATCH 6/8] AM35xx: Read and set ethaddr for EMAC

[Wolfgang Denk]

Dear Tom Rini,

In message <4EDF7E34.1070007@ti.com> you wrote:
>
> > This is a wrong place for this code...
> > You force every board to use the EFUSE'd MAC address - this is wrong.
> > The board must have a freedom to choose what MAC address it wants to use.
> > You don't even check if ethaddr variable is already set...
> > 
> > What you can do is put this implementation into a separate function
> > and let board to make a decision if it wants to call it or use another
> > MAC address.
> > Something like:
> > int am3517_get_efuse_enetaddr(u8 *enetaddr)
> > {
> > 	/* read the address and shift or whatever */
> > 	...
> > 	return is_valid_ether_addr(enetaddr);
> > }
> 
> That's reasonable enough.  I wasn't sure where the custom boards were
> getting their MAC as the EVM and Crane both need this change.

The rule is that the settings in the environment always have
precedence, and you must never overwrite an existing "eth*addr" in
the environment.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"They that can give up essential liberty to obtain a little temporary
saftey deserve neither liberty not saftey." - Benjamin Franklin, 1759

[U-Boot] [PATCH 7/8] AM3517 EVM: Enable ethernet

[Tom Rini]

Signed-off-by: Tom Rini <trini@ti.com>
---
 include/configs/am3517_evm.h |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/include/configs/am3517_evm.h b/include/configs/am3517_evm.h
index 8eb693a..ed6a143 100644
--- a/include/configs/am3517_evm.h
+++ b/include/configs/am3517_evm.h
@@ -158,8 +158,18 @@
 #define CONFIG_SYS_I2C_BUS_SELECT	1
 #define CONFIG_DRIVER_OMAP34XX_I2C	1
 
-#undef CONFIG_CMD_NET
-#undef CONFIG_CMD_NFS
+/*
+ * Ethernet
+ */
+#define CONFIG_DRIVER_TI_EMAC
+#define CONFIG_DRIVER_TI_EMAC_USE_RMII
+#define CONFIG_MII
+#define CONFIG_BOOTP_DEFAULT
+#define CONFIG_BOOTP_DNS
+#define CONFIG_BOOTP_DNS2
+#define CONFIG_BOOTP_SEND_HOSTNAME
+#define CONFIG_NET_RETRY_COUNT		10
+
 /*
  * Board NAND Info.
  */
-- 
1.7.0.4

[U-Boot] [PATCH 8/8] AM3517 Crane: Enable ethernet

[Tom Rini]

Tested-by: Koen Kooi <k-kooi@ti.com>
Signed-off-by: Tom Rini <trini@ti.com>
---
 include/configs/am3517_crane.h |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/include/configs/am3517_crane.h b/include/configs/am3517_crane.h
index 0a0c261..ea862fd 100644
--- a/include/configs/am3517_crane.h
+++ b/include/configs/am3517_crane.h
@@ -161,8 +161,18 @@
 #define CONFIG_SYS_I2C_BUS_SELECT	1
 #define CONFIG_DRIVER_OMAP34XX_I2C	1
 
-#undef CONFIG_CMD_NET
-#undef CONFIG_CMD_NFS
+/*
+ * Ethernet
+ */
+#define CONFIG_DRIVER_TI_EMAC
+#define CONFIG_DRIVER_TI_EMAC_USE_RMII
+#define CONFIG_MII
+#define CONFIG_BOOTP_DEFAULT
+#define CONFIG_BOOTP_DNS
+#define CONFIG_BOOTP_DNS2
+#define CONFIG_BOOTP_SEND_HOSTNAME
+#define CONFIG_NET_RETRY_COUNT		10
+
 /*
  * Board NAND Info.
  */
-- 
1.7.0.4