> -----Original Message-----
> From: Igor Grinberg [mailto:grinb...@compulab.co.il]
> Sent: Thursday, December 08, 2011 7:18 PM
> To: Hiremath, Vaibhav
> Cc: Rini, Tom; u-boot@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:grinb...@compulab.co.il]
> >> Sent: Wednesday, December 07, 2011 7:55 PM
> >> To: Rini, Tom
> >> Cc: u-boot@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 <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.
> >>
> > [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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
