Re: [U-Boot] [PATCH v2 6/7] mpc5121: add support for PDM360NG board

2010-04-14 Thread Anatolij Gustschin 
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

Re: [U-Boot] [PATCH v2 6/7] mpc5121: add support for PDM360NG board

2010-03-21 Thread Wolfgang Denk 
Dear Anatolij Gustschin,

In message 1268755808-24931-7-git-send-email-ag...@denx.de you wrote:
 --===0837153488==
 
 PDM360NG is a MPC5121E based board by ifm ecomatic gmbh.
 
 Signed-off-by: Michael Weiss michael.we...@ifm.com
 Signed-off-by: Anatolij Gustschin ag...@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

[U-Boot] [PATCH v2 6/7] mpc5121: add support for PDM360NG board

2010-03-16 Thread Anatolij Gustschin 
PDM360NG is a MPC5121E based board by ifm ecomatic gmbh.

Signed-off-by: Michael Weiss michael.we...@ifm.com
Signed-off-by: Anatolij Gustschin ag...@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

 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

diff --git a/MAKEALL b/MAKEALL
index beacb5f..deda7e1 100755
--- a/MAKEALL
+++ b/MAKEALL
@@ -93,6 +93,7 @@ LIST_512x=   \
aria\
mecp5123\
mpc5121ads  \
+   pdm360ng\
 
 
 #
diff --git a/Makefile b/Makefile
index d801e25..1068e6a 100644
--- a/Makefile
+++ b/Makefile
@@ -828,6 +828,9 @@ mpc5121ads_rev2_config  \
fi
@$(MKCONFIG) -a mpc5121ads ppc mpc512x mpc5121ads freescale
 
+pdm360ng_config:   unconfig
+   @$(MKCONFIG) -a pdm360ng ppc mpc512x pdm360ng
+
 #
 ## MPC8xx Systems
 #
diff --git a/board/freescale/common/fsl_diu_fb.c 
b/board/freescale/common/fsl_diu_fb.c
index 2fc878b..ae5e7a7 100644
--- a/board/freescale/common/fsl_diu_fb.c
+++ b/board/freescale/common/fsl_diu_fb.c
@@ -50,6 +50,22 @@ struct fb_videomode {
 #define FB_SYNC_COMP_HIGH_ACT  8   /* composite sync high active   */
 #define FB_VMODE_NONINTERLACED  0  /* non interlaced */
 
+/* This setting is used for the ifm pdm360ng with PRIMEVIEW PM070WL3 */
+static struct fb_videomode fsl_diu_mode_800 = {
+   .refresh= 60,
+   .xres   = 800,
+   .yres   = 480,
+   .pixclock   = 31250,
+   .left_margin= 86,
+   .right_margin   = 42,
+   .upper_margin   = 33,
+   .lower_margin   = 10,
+   .hsync_len  = 128,
+   .vsync_len  = 2,
+   .sync   = FB_SYNC_COMP_HIGH_ACT | FB_SYNC_VERT_HIGH_ACT,
+   .vmode  = FB_VMODE_NONINTERLACED
+};
+
 /*
  * These parameters give default parameters
  * for video output 1024x768,
@@ -210,9 +226,14 @@ int fsl_diu_init(int xres,
 
disable_lcdc();
 
-   if (xres == 1280) {
+   switch (xres) {
+   case 800:
+   fsl_diu_mode_db = fsl_diu_mode_800;
+   break;
+   case 1280:
fsl_diu_mode_db = fsl_diu_mode_1280;
-   } else {
+   break;
+   default:
fsl_diu_mode_db = fsl_diu_mode_1024;
}
 
@@ -519,9 +540,9 @@ int fsl_diu_display_bmp(unsigned char *bmp,
b = *bitmap++;
for (k = 0; k  8; k++) {
if (b  0x80)
-   *fb_t = palette[1];
+   *fb_t++ = palette[1];
else
-   *fb_t = palette[0];
+   *fb_t++ = palette[0];
b = b  1;
}
}
diff --git a/board/pdm360ng/Makefile b/board/pdm360ng/Makefile
new file mode 100644
index 000..113ac0a
--- /dev/null
+++ b/board/pdm360ng/Makefile
@@ -0,0 +1,52 @@
+#
+# (C) Copyright 2007
+# Wolfgang Denk, DENX Software Engineering, w...@denx.de.
+#
+# See file CREDITS for list of people who contributed