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

Anatolij Gustschin
2010-04-14T10:39:25Z

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