From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wolfgang Denk Date: Sun, 21 Mar 2010 20:23:06 +0100 Subject: [U-Boot] [PATCH v2 6/7] mpc5121: add support for PDM360NG board In-Reply-To: <1268755808-24931-7-git-send-email-agust@denx.de> References: <1268755808-24931-1-git-send-email-agust@denx.de> <1268755808-24931-2-git-send-email-agust@denx.de> <1268755808-24931-3-git-send-email-agust@denx.de> <1268755808-24931-4-git-send-email-agust@denx.de> <1268755808-24931-5-git-send-email-agust@denx.de> <1268755808-24931-6-git-send-email-agust@denx.de> <1268755808-24931-7-git-send-email-agust@denx.de> Message-ID: <20100321192306.BFE104C022@gemini.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 Dear Anatolij Gustschin, In message <1268755808-24931-7-git-send-email-agust@denx.de> you wrote: > --===============0837153488== > > PDM360NG is a MPC5121E based board by ifm ecomatic gmbh. > > Signed-off-by: Michael Weiss > Signed-off-by: Anatolij Gustschin > --- > Changes since first version: > - don't include RLE8 bitmap support in DIU code, > now it is in common code submitted to U-Boot ML > as separate patch. It is also extended to support > more video formats. > - fixed e-mail format > - keep the list of targets sorted in the Makefile > - move POST code into separate file > - move post_word_load/store code to common file for > all mpc512x boards > - create a file with common board config options > for mpc512x boards which can be included in the > board config file. > - use cfb_console driver for frame buffer support > - allow configuration of coprocessor communication > parameters (baudrate, wait delay) in the board config > file I see you already addressed a number of issues I mentioned in my previous review of this patch. Some others are still open: > MAKEALL | 1 + > Makefile | 3 + > board/freescale/common/fsl_diu_fb.c | 29 ++- > board/pdm360ng/Makefile | 52 ++++ > board/pdm360ng/config.mk | 24 ++ > board/pdm360ng/pdm360ng.c | 525 +++++++++++++++++++++++++++++++++++ > board/pdm360ng/post.c | 75 +++++ > cpu/mpc512x/diu.c | 14 +- > include/configs/mpc5121-common.h | 52 ++++ > include/configs/pdm360ng.h | 520 ++++++++++++++++++++++++++++++++++ > include/post.h | 1 + > post/tests.c | 4 + > 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. > +#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. > +#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 ??? > 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. > 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? 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.] > } else if ((*(char *)(h_addr) != 'B') || (*(char *)(h_addr+1) != 'M')) { > @@ -85,8 +90,13 @@ int mpc5121_diu_init(void) > char *bmp = NULL; > char *bmp_env; > > +#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). > +#ifndef __CONFIG_H > +#define __CONFIG_H > + > +#undef DEBUG Please remove dead code. > +#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. > +/* 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") ? > +#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. > + * 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. > +#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. > +#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. > 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. 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 There's an old proverb that says just about whatever you want it to.