[U-Boot] [PATCH v2] Add support for IDS8313 boards

[Wolfgang Denk]

Dear Sergej.Stepanov at ids.de,

In message <4206182445660643B9AEB8D4E55BBD0A15399F40A6 at HERMES2> you wrote:
> This patch adds support for IDS8313 boards based on MPC8313
> It contains the following components:
> - both of TSEC's, NOR- and NAND flash, I2C, SPI
> 
> Signed-off-by: Sergej Stepanov <ste at ids.de>
> Signed-off-by: Rolf Riehle <rl at ids.de>
> --
>  board/ids/ids8313/Makefile  |   52 ++++
>  board/ids/ids8313/ids8313.c |  170 +++++++++++++
>  boards.cfg                  |    1 +
>  include/configs/IDS8313.h   |  575 +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 798 insertions(+), 0 deletions(-)

Entry to MAINTAINERS missing.

Please make sure to put the MPC83xx custodian on Cc:


> +/*************************************************************************
> + *  fixed sdram init -- doesn't use serial presence detect.
> + ************************************************************************/

Incorrect multiline comment style.  Please fix globally.

...
> +phys_size_t initdram(int board_type)
> +{
> +       immap_t *im = (immap_t *)CONFIG_SYS_IMMR;
> +       fsl_lbc_t *lbc = &im->im_lbc;
> +       u32 msize = 0;
> +
> +       if ((im->sysconf.immrbar & IMMRBAR_BASE_ADDR) != (u32)im)

Please ALWAYS use I/O accessors.  Please fix globally.

...
> +int misc_init_r(void)
> +{
> +       gpio83xx_t *iopd = &((immap_t *)CONFIG_SYS_IMMR)->gpio[0];
> +       u8 *spi_base = (u8 *)IDSCPLD_SPI_CS_BASE;
> +
> +       *spi_base = 0;
> +       iopd->dir |= IDSCPLD_SPI_CS_MASK;

See above - please ALWAYS use I/O accessors.

...
> +#define CONFIG_E300                    1
> +#define CONFIG_MPC83xx                 1
> +#define CONFIG_MPC831x                 1
> +#define CONFIG_MPC8313                 1
> +#define CONFIG_IDS8313                 1

Please remove the values ('1') from all macro definitions that select
features only.

> +#define BOOTFLAG_COLD                  0x01
> +#define BOOTFLAG_WARM                  0x02

I guess these are bogus remnants from ancient times?  Drop these...

...
> +#define CONFIG_BOOTDELAY               5

Already defined further above.  Please clean up.

> +/* watchdog disabled */
> +#undef CONFIG_WATCHDOG

Please do not undef what is not defined anyway.


...
> +/* mtdparts command line support */
> +#define CONFIG_CMD_MTDPARTS
> +#define CONFIG_MTD_DEVICE
> +#define MTDIDS_DEFAULT         "nor0=cu73x-0,nand0=cu73x-nand"
> +#define MTDPARTS_DEFAULT       "mtdparts=cu73x-0:2M(kernel),5m(cramfs)," \
> +                                       "384k(u-boot),128k(env);" \
> +                               "cu78d-nand:96m(jffs2),32m(jffs2)"

This looks inconsistent.  Is it "cu73x-nand" or "cu78d-nand" ?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I don't know if it's what you want, but it's what you get.  :-)
                      - Larry Wall in <10502 at jpl-devvax.JPL.NASA.GOV>