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

[Wolfgang Denk <wd@denx.de>]

Dear Anatolij Gustschin,

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