[U-Boot] [PATCH v2 6/7] mpc5121: add support for PDM360NG board
Wolfgang Denk
wd at denx.de
Sun Mar 21 20:23:06 CET 2010
Dear Anatolij Gustschin,
In message <1268755808-24931-7-git-send-email-agust at denx.de> you wrote:
> --===============0837153488==
>
> PDM360NG is a MPC5121E based board by ifm ecomatic gmbh.
>
> Signed-off-by: Michael Weiss <michael.weiss at ifm.com>
> Signed-off-by: Anatolij Gustschin <agust at denx.de>
> ---
> 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.
More information about the U-Boot
mailing list