[U-Boot] [PATCH 2/4] mpc832x: add support for the mpc8321 based suvd3 board

[Wolfgang Denk]

Dear Heiko Schocher,

In message <4BC2CCBE.6060509 at denx.de> you wrote:
> - serial console on UART1
> - Ethernet RMII over UCC4
> - PHY SMSC LAN8700
> - 64MB Flash
> - 128 MB DDR2 RAM
> - I2C
> - bootcount
> 
> This board is similiar to the kmeter1 (8360) board,
> so common config options are extracted into the
> include/configs/km83xx-common.h file.

> --- a/board/keymile/kmeter1/kmeter1.c
> +++ b/board/keymile/km83xx/km83xx.c
> @@ -11,6 +11,9 @@
>   * (C) Copyright 2008
>   * Heiko Schocher, DENX Software Engineering, hs at denx.de.
>   *
> + * (C) Copyright 2009
> + * Heiko Schocher, DENX Software Engineering, hs at denx.de.
> + *

Please use a single entry and use "2008-2009" instead. Please fix
globally.

Or is this 2010 actually?


> +#if defined(CONFIG_SUVD3)
> +const uint upma_table[] =
> +{ 0x1ffedc00, 0x0ffcdc80, 0x0ffcdc80, 0x0ffcdc04, //Words 0 to 3
> +  0x0ffcdc00, 0xffffcc00, 0xffffcc01, 0xfffffc01, //Words 4 to 7
> +  0xfffffc01, 0xfffffc01, 0xfffffc01, 0xfffffc01, //Words 8 to 11
> +  0xfffffc01, 0xfffffc01, 0xfffffc01, 0xfffffc01, //Words 12 to 15
> +  0xfffffc01, 0xfffffc01, 0xfffffc01, 0xfffffc01, //Words 16 to 19
> +  0xfffffc01, 0xfffffc01, 0xfffffc01, 0xfffffc01, //Words 20 to 23
> +  0x9cfffc00, 0x00fffc80, 0x00fffc80, 0x00fffc00, //Words 24 to 27
> +  0xffffec04, 0xffffec01, 0xfffffc01, 0xfffffc01, //Words 28 to 31
> +  0xfffffc01, 0xfffffc01, 0xfffffc01, 0xfffffc01, //Words 32 to 35
> +  0xfffffc01, 0xfffffc01, 0xfffffc01, 0xfffffc01, //Words 36 to 39
> +  0xfffffc01, 0xfffffc01, 0xfffffc01, 0xfffffc01, //Words 40 to 43
> +  0xfffffc01, 0xfffffc01, 0xfffffc01, 0xfffffc01, //Words 44 to 47
> +  0xfffffc01, 0xfffffc01, 0xfffffc01, 0xfffffc01, //Words 48 to 51
> +  0xfffffc01, 0xfffffc01, 0xfffffc01, 0xfffffc01, //Words 52 to 55
> +  0xfffffc01, 0xfffffc01, 0xfffffc01, 0xfffffc01, //Words 56 to 59
> +  0xfffffc01, 0xfffffc01, 0xfffffc01, 0xfffffc01  //Words 60 to 63
> +};
> +#endif

No C++ comments, please. Please fix globally.

>  int checkboard (void)
>  {
> -	puts ("Board: Keymile kmeter1");
> +	puts ("Board: Keymile ");
> +	puts(CONFIG_KM_BOARD_NAME);

Please write:

	puts("Board: Keymile " CONFIG_KM_BOARD_NAME);



> --- a/cpu/mpc83xx/cpu.c
> +++ b/cpu/mpc83xx/cpu.c
> @@ -305,7 +305,7 @@ int cpu_mmc_init(bd_t *bis)
> 
>  #ifdef CONFIG_BOOTCOUNT_LIMIT
> 
> -#if !defined(CONFIG_MPC8360)
> +#if !defined(CONFIG_MPC8360) && !defined(CONFIG_MPC832x)
>  #error "CONFIG_BOOTCOUNT_LIMIT only for MPC8360 implemented"

Code and comments don't agree.

And please keep list sorted (832x < 8360). Please fix globally.

...
> +#undef CONFIG_DDR_ECC
> +
> +/*
> + * DDRCDR - DDR Control Driver Register
> + */
> +
> +#undef CONFIG_SPD_EEPROM	/* Do not use SPD EEPROM for DDR setup */

No need to undefine what is not defined anyway.

> +/*
> + * Manually set up DDR parameters
> + */
> +#define CONFIG_DDR_II
> +#define CONFIG_SYS_DDR_SIZE		2048 /* MB */

Do you use get_ram_size() for auto-sizing?

> +#if (CONFIG_SYS_MONITOR_BASE < CONFIG_SYS_FLASH_BASE)
> +#define CONFIG_SYS_RAMBOOT
> +#else
> +#undef	CONFIG_SYS_RAMBOOT
> +#endif

Please do not #undef what is not defined - please fix globally.

> +/*
> + * Initial RAM Base Address Setup
> + */
> +#define CONFIG_SYS_INIT_RAM_LOCK	1
> +#define CONFIG_SYS_INIT_RAM_ADDR	0xE6000000 /* Initial RAM address */
> +#define CONFIG_SYS_INIT_RAM_END	0x1000 /* End of used area in RAM */
> +#define CONFIG_SYS_GBL_DATA_SIZE	0x100 /* num bytes initial data */
> +#define CONFIG_SYS_GBL_DATA_OFFSET	(CONFIG_SYS_INIT_RAM_END - CONFIG_SYS_GBL_DATA_SIZE)

Line too long. please fix globally.

...
> +#ifndef CONFIG_SYS_RAMBOOT
...
> +#else /* CFG_RAMBOOT */
...
> +#endif /* CFG_RAMBOOT */

CFG_RAMBOOT ??? Seems this needs cleanup.

> +#if defined(CFG_RAMBOOT)
> +#undef CONFIG_CMD_SAVEENV
> +#undef CONFIG_CMD_LOADS

Why?

> +#define CONFIG_EXTRA_ENV_SETTINGS \
> +       CONFIG_KM_DEF_ENV						\
> +       CONFIG_KM_DEF_ROOTPATH						\
> +	"dtt_bus=pca9547:70:a\0"					\
> +	"EEprom_ivm=pca9547:70:9\0"					\
> +	"newenv="							\
> +		"prot off 0xF00C0000 +0x40000 && "			\
> +		"era 0xF00C0000 +0x40000\0" 				\
> +	"unlock=yes\0"							\
> +	""

Indentation by TAB only. Please fix globally.


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
Nobody goes to that restaurant anymore. It's too crowded.