From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Fri, 23 Apr 2010 13:39:46 +0200 Subject: [U-Boot] [PATCH 2/4] mpc832x: add support for the mpc8321 based suvd3 board In-Reply-To: <20100422112008.EE0D9C36DD0@gemini.denx.de> References: <4BC2CCBE.6060509@denx.de> <20100422112008.EE0D9C36DD0@gemini.denx.de> Message-ID: <4BD18702.40104@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello Wolfgang, Wolfgang Denk wrote: > In message <4BC2CCBE.6060509@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