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 <grinb...@compulab.co.il>
>>> wrote:
>>>> Hi Tom,
>>>>
>>>> On 12/06/11 17:49, Tom Rini wrote:
>>>>> From: Vaibhav Hiremath <hvaib...@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 <hvaib...@ti.com>
>>>>> Signed-off-by: Tom Rini <tr...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
