Dear Anatolij Gustschin,
In message <1266964630-7754-7-git-send-email-ag...@denx.de> you wrote:
> --===============1816892533==
>
> PDM360NG is a MPC5121E based board by ifm ecomatic gmbh.
>
> Signed-off-by: Michael Weiss michael.we...@ifm.com
Incorrect mail address format.
> Signed-off-by: Anatolij Gustschin <ag...@denx.de>
> ---
> MAKEALL | 1 +
> Makefile | 3 +
> board/freescale/common/fsl_diu_fb.c | 29 ++-
> board/pdm360ng/Makefile | 51 +++
> board/pdm360ng/config.mk | 24 ++
> board/pdm360ng/pdm360ng.c | 618
> +++++++++++++++++++++++++++++++++++
> cpu/mpc512x/diu.c | 14 +-
> include/configs/pdm360ng.h | 525 +++++++++++++++++++++++++++++
> include/post.h | 1 +
> post/tests.c | 4 +
> 10 files changed, 1264 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 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,
> + 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]);
If you really want to do this, then please use
sprintf(mac, "%pM", buf);
instead.
> + /* Owned by IFM ? */
> + if (strstr(mac, "00:02:01") != mac) {
> + printf("Illegal MAC address in EEPROM: %s\n",
> + mac);
Why not simply a memcmp() or a strncmp() ?
> + } 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,
> + CONFIG_SYS_I2C_EEPROM_MAC_OFFSET);
> + }
All this seems overly complicated to me. All these string conversions
are not really needed. Why not like this:
uchar ifm_oui[3] = { 0, 2, 1, };
ret = i2c_read(...);
if (ret != 0) {
printf("Error: Unable to read MAC from I2C"
" EEPROM at address %02X:%02X\n",
CONFIG_SYS_I2C_EEPROM,
CONFIG_SYS_I2C_EEPROM_MAC_OFFSET);
return 0;
/* or rather return 1; ??? */
}
if (memcmp(buf, ifm_oui, sizeof(ifm_oui)) {
printf("Illegal MAC address in EEPROM: %pM\n",
buf):
}
eth_setenv_enetaddr("ethaddr", buf);
> +#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?
> +#ifdef CONFIG_PDM360NG
> + xres = 800;
> + yres = 480;
> +#else
> xres = 1024;
> yres = 768;
> +#endif
Please avoid the board specific #define here. 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?
> +/*
> + * 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 */
Please use auto-sizing with get_ram_size() instead, so a single
U-Bootimage can be used with both configurations.
> +/* DDR Controller Configuration for Micron DDR2 SDRAM MT47H128M8-3
Do comment and code match?
> +#ifdef CONFIG_PDM360NG_BIG
> +#define CONFIG_SYS_FLASH_BASE 0xF0000000 /* start of
> FLASH-Bank0 */
> +#define CONFIG_SYS_FLASH1_BASE 0xF8000000 /* start of
> FLASH-Bank1 */
> +#define CONFIG_SYS_FLASH_SIZE 0x08000000 /* max flash size of
> FLASH-Bank0 in bytes */
> +#define CONFIG_SYS_FLASH1_SIZE 0x08000000 /* max flash size of
> FLASH-Bank1 in bytes */
> +#define CONFIG_SYS_MAX_FLASH_SECT 512 /* max sectors per device */
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. Remove dead code.
> +#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.
> +/* POST support */
> +#define CONFIG_POST (CONFIG_SYS_POST_COPROC)
> +
> +#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: w...@denx.de
Prediction is very difficult, especially of the future. - Niels Bohr
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
