| Message ID | 1323186582-2811-4-git-send-email-trini@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
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. > + > #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. > 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? > > #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 */\
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). 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. >> #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... > 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. >> 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.
> -----Original Message----- > From: Igor Grinberg [mailto:grinberg@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 <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. 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. > > + > > #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. > > 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.
Hi Vaibhav, On 12/08/11 13:11, Hiremath, Vaibhav wrote: > >> -----Original Message----- >> From: Igor Grinberg [mailto:grinberg@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 <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 */\
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. >
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@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...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.
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 <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...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
> -----Original Message----- > From: Igor Grinberg [mailto:grinberg@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:grinberg@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 <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. > 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.
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 + #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. + */ 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 } 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 #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 */\