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

[Wolfgang Denk]

Dear Anatolij Gustschin,

In message <1266964630-7754-7-git-send-email-agust at denx.de> you wrote:
> --===============1816892533==
> 
> PDM360NG is a MPC5121E based board by ifm ecomatic gmbh.
> 
> Signed-off-by: Michael Weiss michael.weiss at ifm.com

Incorrect mail address format.

> Signed-off-by: Anatolij Gustschin <agust at 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: wd at denx.de
Prediction is very difficult, especially of the future.  - Niels Bohr