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

[Heiko Schocher]

Hello Wolfgang,

Wolfgang Denk wrote:
> 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?

It is 2008-2010, fixed.

>> +#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.

fixed.

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

fixed.

>> --- 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.

fixed.

> ...
>> +#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.

Yep.

>> +/*
>> + * 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?

Yes, this is the maximum size ...

>> +#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.

OK, fixed.

>> +/*
>> + * 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.

Ok.

> ...
>> +#ifndef CONFIG_SYS_RAMBOOT
> ...
>> +#else /* CFG_RAMBOOT */
> ...
>> +#endif /* CFG_RAMBOOT */
> 
> CFG_RAMBOOT ??? Seems this needs cleanup.

Yep.

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

Good question ... I delete this.

>> +#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.

fixed.

Thanks for the review.

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany