[U-Boot] [PATCH 3/8] AM3517: Add NOR Flash boot mode Support
Tom Rini
trini at ti.com
Thu Dec 8 15:38:01 CET 2011
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 at compulab.co.il> wrote:
>>> Hi Tom,
>>>
>>> On 12/06/11 17:49, Tom Rini wrote:
>>>> From: Vaibhav Hiremath <hvaibhav at 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 at ti.com>
>>>> Signed-off-by: Tom Rini <trini at 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
More information about the U-Boot
mailing list