Hello Wolfgang, On Sun, 21 Mar 2010 20:23:06 +0100 Wolfgang Denk <w...@denx.de> wrote:
... > > 12 files changed, 1294 insertions(+), 6 deletions(-) > > create mode 100644 board/pdm360ng/Makefile > > create mode 100644 board/pdm360ng/config.mk > > create mode 100644 board/pdm360ng/pdm360ng.c > > create mode 100644 board/pdm360ng/post.c > > create mode 100644 include/configs/mpc5121-common.h > > create mode 100644 include/configs/pdm360ng.h > > Entry to MAINTAINERS file is missing. Ok, will fix it. > > +#if defined(CONFIG_HARD_I2C) > > + if (!getenv("ethaddr")) { > > + uchar buf[6]; > > + char mac[18]; > > + int ret; > > + > > + /* I2C-0 for on-board eeprom */ > > + i2c_set_bus_num(CONFIG_SYS_I2C_EEPROM_BUS_NUM); > > + > > + /* Read ethaddr from EEPROM */ > > + ret = i2c_read(CONFIG_SYS_I2C_EEPROM_ADDR, > > + CONFIG_SYS_I2C_EEPROM_MAC_OFFSET, 1, buf, 6); > > + if (!ret) { > > + sprintf(mac, "%02X:%02X:%02X:%02X:%02X:%02X", > > + buf[0], buf[1], buf[2], buf[3], buf[4], buf[5]); > > + /* Owned by IFM ? */ > > + if (strstr(mac, "00:02:01") != mac) { > > + printf("Illegal MAC address in EEPROM: %s\n", > > + mac); > > + } else { > > + debug("Using MAC from I2C EEPROM: %s\n", mac); > > + setenv("ethaddr", mac); > > + } > > + } else { > > + printf("Error: Unable to read MAC from I2C" > > + " EEPROM at address %02X:%02X\n", > > + CONFIG_SYS_I2C_EEPROM_ADDR, > > + CONFIG_SYS_I2C_EEPROM_MAC_OFFSET); > > + } > > Please see comments in previous message. IMHO this needs to be > rewritten. Ok, I will rewrite it as suggested. Thanks. > > > +#if defined(CONFIG_SERIAL_MULTI) > > +/* > > + * If argument is NULL, set the LCD brightness to the > > + * value from "brightness" environment variable. Set > > + * the LCD brightness to the value specified by the > > + * argument otherwise. Default brightness is zero. > > + */ > > +#define MAX_BRIGHTNESS 99 > > +static int set_lcd_brightness(char *brightness) > > This seems wrong to me. Why does LCD related code depend on > CONFIG_SERIAL_MULTI ??? The reason for this is that LCD brightness is controlled by sending commands to the I/O Coprocessor over serial line (PSC4). If CONFIG_SERIAL_MULTI is not selected, there is no possibility to set the brightness, so it doesn't make sense to include this code then. > > diff --git a/cpu/mpc512x/diu.c b/cpu/mpc512x/diu.c > > index a24f395..fc43a9d 100644 > > --- a/cpu/mpc512x/diu.c > > +++ b/cpu/mpc512x/diu.c > > @@ -68,8 +68,13 @@ char *valid_bmp(char *addr) > > unsigned long h_addr; > > > > h_addr = simple_strtoul(addr, NULL, 16); > > - if (h_addr < CONFIG_SYS_FLASH_BASE || > > - h_addr >= (CONFIG_SYS_FLASH_BASE + > > CONFIG_SYS_FLASH_SIZE - 1)) { > > + if ((h_addr < CONFIG_SYS_FLASH_BASE || > > + h_addr >= (CONFIG_SYS_FLASH_BASE + CONFIG_SYS_FLASH_SIZE - 1)) > > +#if defined(CONFIG_SYS_FLASH1_BASE) && defined(CONFIG_SYS_FLASH1_SIZE) > > + && (h_addr < CONFIG_SYS_FLASH1_BASE || > > + h_addr >= (CONFIG_SYS_FLASH1_BASE + CONFIG_SYS_FLASH1_SIZE - 1)) > > +#endif > > + ) { > > I don't like this. Why do we see hardcoded values here and not the > data from the CFI driver's auto-sizing? I assume both flash banks are > maped into one big contiguous array of NOR flash, right? Then there > should be just a single test for "< base || >(base+size)" here, using > the total size of the combined flash banks. Will fix it using a single test as suggested. Thanks! > > printf("bmp addr %lx is not a valid flash address\n", h_addr); > > return 0; > > And why would it be necessary that the BMP resides in NOR flash in the > first place? This check is only done while first DIU init. It checks the BMP address in "diu_bmp_addr" environment variable which is supposed to point to a bitmap in flash. > Why cannot - for example - a "preboot" command be used to read it from > SDCard into RAM? > > [I am aware that you did not create this code, but anyways - it seems > wrong to me.] Displaying a bitmap form RAM can be done later by running "diufb addr" and this command doesn't perform the above check. > > +#ifdef CONFIG_PDM360NG > > + xres = 800; > > + yres = 480; > > +#else > > xres = 1024; > > yres = 768; > > +#endif > > Please avoid the board specific #define here. Please check for a > feature instead (resolution = 800x400). I will fix it using CONFIG macros for resolution. ... > > +#undef DEBUG > > Please remove dead code. Ok, will fix. Thanks! > > +#define CONFIG_PDM360NG 1 > > +#define CONFIG_PDM360NG_BIG /* PDM360NG with big memory */ > > +#undef CONFIG_PDM360NG_SMALL /* PDM360NG with small memory */ > > This makes no sense. If you want to toggle between "small" and "big" > configurations, then please use a single #define which is either set > or not set. > > I don't see why this is needed at all. Does not U-Boot auto-detect the > RAM size, so you can auto-adjust all code where needed? If not, then > this should be fixed. > > > +/* > > + * DDR Setup - manually set all parameters as there's no SPD etc. > > + */ > > +#ifdef CONFIG_PDM360NG_BIG > > +#define CONFIG_SYS_DDR_SIZE 512 /* MB */ > > +#elif defined CONFIG_PDM360NG_SMALL > > +#define CONFIG_SYS_DDR_SIZE 128 /* MB */ > > +#else > > +#error No memory configuration level defined! > > +#endif > > Please use auto-sizing with get_ram_size() instead, so a single > U-Boot image can be used with both configurations. I will use get_ram_size() in fixed_sdram() to determine the size, but we still need to differentiate between "big" and "small" configuration as some parameters are different for 128 MB configuration and cannot be determined using SPD. > > +/* DDR Controller Configuration for Micron DDR2 SDRAM MT47H128M8-3 > > + * > > Incorrect multiline comment format. > > And: comment and code seem to differ - the comment explains one > configuration, but the code has 2 ("small" and "big") ? Will fix and move the comment to "big" configuration. > > +#define CONFIG_SYS_DDRCMD_NOP 0x01380000 > > +#define CONFIG_SYS_DDRCMD_PCHG_ALL 0x01100400 > > +#define CONFIG_SYS_DDRCMD_EM2 0x01020000 /* EMR2 */ > > +#define CONFIG_SYS_DDRCMD_EM3 0x01030000 /* EMR3 */ > > +#define CONFIG_SYS_DDRCMD_EN_DLL 0x01010040 /* EMR with 150 ohm ODT > > todo: verify*/ > > +#define CONFIG_SYS_DDRCMD_RES_DLL 0x01000100 > > +#define CONFIG_SYS_DDRCMD_RFSH 0x01080000 > > +#define CONFIG_SYS_MICRON_INIT_DEV_OP 0x01000432 > > +#define CONFIG_SYS_DDRCMD_OCD_DEFAULT 0x010107C0 /* EMR with 150 > > ohm ODT todo: verify*/ > > +#define CONFIG_SYS_DDRCMD_OCD_EXIT 0x01010440 /* EMR new command with 150 > > ohm ODT todo: verify*/ > > Lines way too long. Please fix globally. Will fix. Thanks. > > + * Serial Port > > + */ > > +#define CONFIG_CONS_INDEX 1 > > +#undef CONFIG_SERIAL_SOFTWARE_FIFO > > Please do not #undef what is not defined anyway. Please remove dead > code globally. Ok, fixed. > > +#define CONFIG_CMD_ASKENV > > +#define CONFIG_CMD_DHCP > > +#define CONFIG_CMD_I2C > > +#define CONFIG_CMD_MII > > +#define CONFIG_CMD_NFS > > +#define CONFIG_CMD_PING > > +#define CONFIG_CMD_REGINFO > > +#define CONFIG_CMD_EEPROM > > +#define CONFIG_CMD_DATE > > +#undef CONFIG_CMD_FUSE > > You may want to sort that list. Will do. > > +#define CONFIG_POST_COPROC {\ > > + "Coprocessors communication test", \ > > + "coproc_com", \ > > + "This test checks communication with coprocessors.", \ > > + POST_RAM | POST_ALWAYS | POST_CRITICAL, \ > > + &pdm360ng_coprocessor_post_test, \ > > + NULL, \ > > + NULL, \ > > + CONFIG_SYS_POST_COPROC \ > > + } > > +#endif > > This does not belong into the board config file. Ok, I will move it to post/tests.c file. > > diff --git a/include/post.h b/include/post.h > > index 9fcd3ce..d147103 100644 > > --- a/include/post.h > > +++ b/include/post.h > > @@ -119,6 +119,7 @@ extern int post_hotkeys_pressed(void); > > #define CONFIG_SYS_POST_BSPEC4 0x00080000 > > #define CONFIG_SYS_POST_BSPEC5 0x00100000 > > #define CONFIG_SYS_POST_CODEC 0x00200000 > > +#define CONFIG_SYS_POST_COPROC 0x00400000 > > Please split the POST specific code out into a separate commit. OK. Thanks! Anatolij -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot