[U-Boot] [PATCH v2] 83xx: add support for ve8313 board

[Wolfgang Denk]

Dear Heiko Schocher,

In message <4C340265.3020302 at invitel.hu> you wrote:
>
> changes since v1
> - environment size = sector size

Argh.  While the previous environment size of 8 KiB was indeed a bit
smallish, using 128 KiB is overkill. Keep in mind that we then have to
compute the checksum over 18 KiB as well, which just costs time - at
booting, and with every setenv you run.

I suggest to change this to a more resaobale size - say 16 KiB ?


> +/* Fixed sdram init -- doesn't use serial presence detect.
> + *
> + * This is useful for faster booting in configs where the RAM is unlikely
> + * to be changed, or for things like NAND booting where space is tight.
> + */

Incorrect multiline comment style.

The first line of the comment is misleading as one might think there
was SPD information available but we decide not to use it - actually
there is none and we cannot use it. Please fix. And since there are
no other RAM configurations on this board, I recommend to remove the
last 2 lines of the comment as well.


> +	/* set WDT pins as output */
> +	setbits_be32(&gpio->dir, VE8313_WDT_EN | VE8313_WDT_TRIG);
> +#if defined(CONFIG_HW_WATCHDOG)
> +	/* enable WDT */
> +	clrbits_be32(&gpio->dat, VE8313_WDT_EN | VE8313_WDT_TRIG);
> +#else
> +	/* disable WDT */
> +	setbits_be32(&gpio->dat, VE8313_WDT_EN | VE8313_WDT_TRIG);
> +#endif
> +	return 0;

Is this the correct order? Would it not make sense first to define
the state of the data bit before you enable the output? Otherwise
this might result in short spikes at the wrong signal level, which
here might start the watchdog by accident?

> +#if defined(CONFIG_HW_WATCHDOG)
> +void hw_watchdog_reset(void)
> +{
> +	volatile immap_t *im = (volatile immap_t *)CONFIG_SYS_IMMR;
> +	volatile gpio83xx_t *gpio = (volatile gpio83xx_t *)im->gpio;
> +
> +	setbits_be32(&gpio->dat, VE8313_WDT_TRIG);
> +	clrbits_be32(&gpio->dat, VE8313_WDT_TRIG);
> +}

Is there a minimal length of the trigger pulse defined for this
watchdog? Maybe some udelay() is needed here?


> --- a/boards.cfg
> +++ b/boards.cfg
> @@ -135,6 +135,7 @@ TQM8272		powerpc	mpc8260		tqm8272		tqc
>  kmeter1		powerpc	mpc83xx		kmeter1		keymile
>  MVBLM7		powerpc	mpc83xx		mvblm7		matrix_vision
>  TQM834x		powerpc	mpc83xx		tqm834x		tqc
> +ve8313		powerpc	mpc83xx		ve8313
>  PM854		powerpc	mpc85xx		pm854
>  PM856		powerpc	mpc85xx		pm856
>  stxgp3		powerpc	mpc85xx		stxgp3		stx

This is not sorted correctly. It should go here (because of the empty
vendor field).

 SCM            powerpc mpc8260         -               siemens
 TQM8272        powerpc mpc8260         tqm8272         tqc
+ve8313         powerpc mpc83xx         ve8313
 kmeter1        powerpc mpc83xx         kmeter1         keymile
 MVBLM7         powerpc mpc83xx         mvblm7          matrix_vision

The comment in the header of the file describes how to correctly sort
this file; if you cannot parse this, just run the command in vi :-)


> +/*
> + * Manually set up DDR parameters, as this board does not
> + * seem to have the SPD connected to I2C.
> + */

"does not seem to have the SPD connected to I2C"? You know this for
sure, so please fix the comment.

> +#if !defined(CONFIG_SYS_NO_FLASH)
...
> +#else
> +#define CONFIG_SYS_FLASH_BASE		0xFE000000
> +#define CONFIG_SYS_FLASH_SIZE		32	/* size in MB */
> +#endif

I think there are no configurations of this board without NOR flash
either; please drop this as well.

> +/*
> + * TSEC
> + */
> +#define CONFIG_TSEC_ENET		/* TSEC ethernet support */
> +
> +#define CONFIG_NET_MULTI
> +
> +#define CONFIG_TSEC1

Please drop these empty lines.

> +#ifdef CONFIG_TSEC1
> +#define CONFIG_HAS_ETH0
> +#define CONFIG_TSEC1_NAME      "TSEC0"

Oops? This is TSEC1, so why do you name it "TSEC0" ?



> +/*
> + * Environment
> + */
> +#if !defined(CONFIG_SYS_RAMBOOT)
> +	#define CONFIG_ENV_IS_IN_FLASH	1
> +	#define CONFIG_ENV_ADDR		(CONFIG_SYS_MONITOR_BASE + \
> +						CONFIG_SYS_MONITOR_LEN)
> +	#define CONFIG_ENV_SECT_SIZE	0x20000	/* 64K(one sector) for env */

Comment and code don't match - the comment is wrong - actual sector
size is 128 KiB.

> +	/* Address and size of Redundant Environment Sector */
> +	#define CONFIG_ENV_OFFSET_REDUND	(CONFIG_ENV_OFFSET + \
> +						CONFIG_ENV_SECT_SIZE)
> +	#define CONFIG_ENV_SIZE_REDUND	(CONFIG_ENV_SIZE)
> +#else
> +	#define CONFIG_SYS_NO_FLASH	1	/* Flash is not usable now */

Why would flash not be usable when booting from RAM?

> +	#define CONFIG_ENV_IS_NOWHERE	1	/* Store ENV in mem only */
> +	#define CONFIG_ENV_ADDR		(CONFIG_SYS_MONITOR_BASE - 0x1000)
> +	#define CONFIG_ENV_SIZE		0x2000

I recommend to use the same CONFIG_ENV_SIZE setting for both
configurations (suggestion: use (16 << 10)).

> +#if defined(CONFIG_SYS_RAMBOOT) && !defined(CONFIG_NAND_U_BOOT)
> +    #undef CONFIG_CMD_SAVEENV
> +    #undef CONFIG_CMD_LOADS
> +#endif

Why do you disable the loads command here? This makes no sense to me.


> +/*
> + * Environment Configuration
> + */
> +#define CONFIG_ENV_OVERWRITE

Please remove.

> +#define CONFIG_SYS_LOAD_ADDR   0x2000000       /* default load address */
...
> +#define CONFIG_LOADADDR		800000

Is this 800000 (= 0xc3500) or 0x800000?
and why is it different from the CONFIG_SYS_LOAD_ADDR definition above?

> +#define CONFIG_BOOTDELAY	6	/* -1 disables auto-boot */
> +#define CONFIG_BAUDRATE		115200
> +
> +#define XMK_STR(x)	#x
> +#define MK_STR(x)	XMK_STR(x)
> +
> +#define CONFIG_EXTRA_ENV_SETTINGS \
> +	"netdev=" MK_STR(CONFIG_NETDEV) "\0"				\
> +	"ethprime=TSEC0\0"						\

Use CONFIG_TSEC1_NAME here?


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
Severe culture shock results when experts from another protocol suite
[...] try to read OSI documents. The term "osified" is used to  refer
to  such  documents. [...] Any relationship to the word "ossified" is
purely intentional.                                - Marshall T. Rose