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

[Anatolij Gustschin <agust@denx.de>]

Hello Wolfgang,

On Sun, 21 Mar 2010 20:23:06 +0100
Wolfgang Denk <wd@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 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.

I will use get_ram_size() in fixed_sdram() to determine the
size, but we still need to differentiate between "big" and
"small" configuration as some parameters are different for
128 MB configuration and cannot be determined using SPD.

> > +/* 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") ?

Will fix and move the comment to "big" configuration.

> > +#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.

Will fix. Thanks.

> > + * 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.

Ok, fixed.

> > +#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.

Will do.

> > +#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.

Ok, I will move it to post/tests.c 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.

OK.

Thanks!
Anatolij

--
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de