Zundel,
Thanks for your valuable comments. My reply inline.
-Nag
On Fri, Jul 29, 2011 at 13:54:30, Detlev Zundel wrote:
> Hi,
>
> > From: Nagabhushana Netagunte <nagabhushana.netagu...@ti.com>
> >
> > Add an option to use NOR boot mode in configuration file and
> > correspanding pin-mux support in board file.
> >
> > Signed-off-by: Sudhakar Rajashekhara <sudhakar....@ti.com>
> > Signed-off-by: Nagabhushana Netagunte <nagabhushana.netagu...@ti.com>
> > ---
> > board/davinci/da8xxevm/da850evm.c | 50
> > +++++++++++++++++++++++++++++++++++++
> > include/configs/da850evm.h | 21 ++++++++++++++-
> > 2 files changed, 70 insertions(+), 1 deletions(-)
> >
> > diff --git a/board/davinci/da8xxevm/da850evm.c
> > b/board/davinci/da8xxevm/da850evm.c
> > index 73eaa48..a77e438 100644
> > --- a/board/davinci/da8xxevm/da850evm.c
> > +++ b/board/davinci/da8xxevm/da850evm.c
> > @@ -105,6 +105,54 @@ const struct pinmux_config nand_pins[] = {
> > { pinmux(12), 1, 5 },
> > { pinmux(12), 1, 6 }
> > };
> > +#elif defined(CONFIG_SYS_USE_NOR)
>
> Can we have at least a little comment explaining what this long magic list
> does? I for one don't have the slightest clue to why it has to be like this.
>
Will add comments.
> > +const struct pinmux_config nor_pins[] = {
> > + { pinmux(5), 1, 6 },
> > + { pinmux(6), 1, 6 },
> > + { pinmux(7), 1, 0 },
> > + { pinmux(7), 1, 4 },
> > + { pinmux(7), 1, 5 },
> > + { pinmux(8), 1, 0 },
> > + { pinmux(8), 1, 1 },
> > + { pinmux(8), 1, 2 },
> > + { pinmux(8), 1, 3 },
> > + { pinmux(8), 1, 4 },
> > + { pinmux(8), 1, 5 },
> > + { pinmux(8), 1, 6 },
> > + { pinmux(8), 1, 7 },
> > + { pinmux(9), 1, 0 },
> > + { pinmux(9), 1, 1 },
> > + { pinmux(9), 1, 2 },
> > + { pinmux(9), 1, 3 },
> > + { pinmux(9), 1, 4 },
> > + { pinmux(9), 1, 5 },
> > + { pinmux(9), 1, 6 },
> > + { pinmux(9), 1, 7 },
> > + { pinmux(10), 1, 0 },
> > + { pinmux(10), 1, 1 },
> > + { pinmux(10), 1, 2 },
> > + { pinmux(10), 1, 3 },
> > + { pinmux(10), 1, 4 },
> > + { pinmux(10), 1, 5 },
> > + { pinmux(10), 1, 6 },
> > + { pinmux(10), 1, 7 },
> > + { pinmux(11), 1, 0 },
> > + { pinmux(11), 1, 1 },
> > + { pinmux(11), 1, 2 },
> > + { pinmux(11), 1, 3 },
> > + { pinmux(11), 1, 4 },
> > + { pinmux(11), 1, 5 },
> > + { pinmux(11), 1, 6 },
> > + { pinmux(11), 1, 7 },
> > + { pinmux(12), 1, 0 },
> > + { pinmux(12), 1, 1 },
> > + { pinmux(12), 1, 2 },
> > + { pinmux(12), 1, 3 },
> > + { pinmux(12), 1, 4 },
> > + { pinmux(12), 1, 5 },
> > + { pinmux(12), 1, 6 },
> > + { pinmux(12), 1, 7 }
> > +};
> > #endif
> >
> > #ifdef CONFIG_DRIVER_TI_EMAC_USE_RMII @@ -122,6 +170,8 @@ static
> > const struct pinmux_resource pinmuxes[] = {
> > PINMUX_ITEM(i2c_pins),
> > #ifdef CONFIG_NAND_DAVINCI
> > PINMUX_ITEM(nand_pins),
> > +#elif defined(CONFIG_USE_NOR)
> > + PINMUX_ITEM(nor_pins),
> > #endif
> > };
> >
> > diff --git a/include/configs/da850evm.h b/include/configs/da850evm.h
> > index fdcc6e3..f0015e4 100644
> > --- a/include/configs/da850evm.h
> > +++ b/include/configs/da850evm.h
> > @@ -28,6 +28,8 @@
> > */
> > #define CONFIG_DRIVER_TI_EMAC
> > #define CONFIG_USE_SPIFLASH
> > +#undef CONFIG_USE_NAND
> > +#undef CONFIG_SYS_USE_NOR
> >
> > /*
> > * SoC Configuration
> > @@ -129,6 +131,23 @@
> > #define CONFIG_NET_MULTI
> > #endif
> >
> > +#ifdef CONFIG_SYS_USE_NOR
> > +#define CONFIG_ENV_IS_IN_FLASH
> > +#undef CONFIG_SYS_NO_FLASH
> > +#define CONFIG_FLASH_CFI_DRIVER
> > +#define CONFIG_SYS_FLASH_CFI
> > +#define CONFIG_SYS_FLASH_PROTECTION
> > +#define CONFIG_SYS_MAX_FLASH_BANKS 1 /* max number of flash banks */
> > +#define CONFIG_SYS_FLASH_SECT_SZ (128 << 10) /* 128KB */
> > +#define CONFIG_ENV_OFFSET (CONFIG_SYS_FLASH_SECT_SZ * 3)
> > +#define CONFIG_ENV_SIZE (128 << 10)
> > +#define CONFIG_SYS_FLASH_BASE DAVINCI_ASYNC_EMIF_DATA_CE2_BASE
> > +#define PHYS_FLASH_SIZE (8 << 20) /* Flash size 8MB */
> > +#define CONFIG_SYS_MAX_FLASH_SECT
> > ((PHYS_FLASH_SIZE/CONFIG_SYS_FLASH_SECT_SZ)\
> > + + 3)
> > +#define CONFIG_ENV_SECT_SIZE CONFIG_SYS_FLASH_SECT_SZ
> > +#endif
> > +
> > #ifdef CONFIG_USE_SPIFLASH
> > #undef CONFIG_ENV_IS_IN_FLASH
> > #undef CONFIG_ENV_IS_IN_NAND
> > @@ -212,7 +231,7 @@
> > #endif
> >
> > #if !defined(CONFIG_USE_NAND) && \
> > - !defined(CONFIG_USE_NOR) && \
> > + !defined(CONFIG_SYS_USE_NOR) && \
> > !defined(CONFIG_USE_SPIFLASH)
> > #define CONFIG_ENV_IS_NOWHERE
> > #define CONFIG_SYS_NO_FLASH
>
> Also I am somewhat sceptical about the names of the defines in this board
> config - there is for example the new CONFIG_SYS_USE_NOR and the "old"
> CONFIG_USE_NAND and CONFIG_USE_SPIFLASH. Looking into the README, I see this:
>
> ,----[ README ]
> | There are two classes of configuration variables:
> |
> | * Configuration _OPTIONS_:
> | These are selectable by the user and have names beginning with
> | "CONFIG_".
> |
> | * Configuration _SETTINGS_:
> | These depend on the hardware etc. and should not be meddled with if
> | you don't know what you're doing; they have names beginning with
> | "CONFIG_SYS_".
> `----
>
> If the names are chosen correctly, then CONFIG_SYS_USE_NOR is coupled to the
> actual hardware, so either the hardware _has_ NOR flash, then it must be set,
> or it _doesn't_ have NOR flash, then we don't set it. But it seems the
> options are there to give the user a choice what medium he wants to use (for
> e.g. the environment). So if this is correct, then please strip the _SYS_ in
> them.
>
> Cheers
> Detlev
>
Will take care CONFIG name.
> --
> 14474011154664524427946373126085988481573677491474835889066354349131199152128
> If you know why this number is perfect - you're probably a mathematician...
> --
> DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: d...@denx.de
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
