Dear Anatolij Gustschin,
In message 1266964630-7754-7-git-send-email-agust@denx.de you wrote:
--===============1816892533==
PDM360NG is a MPC5121E based board by ifm ecomatic gmbh.
Signed-off-by: Michael Weiss michael.weiss@ifm.com
Incorrect mail address format.
Signed-off-by: Anatolij Gustschin agust@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