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

[Igor Grinberg <grinberg@compulab.co.il>]

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.