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

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

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.