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

[Heiko Schocher]

Hello Wolfgang,

Wolfgang Denk wrote:
> 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 ?

Ok, done.

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

removed.

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

Yup, fix this.

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

I check this.

>> --- 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).

fixed.

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

fixed

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

fixed.

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

Because this is so in all other board configs I looked.

Kim? Can you comment this?
(I think it is because to be in sync with "eth0" ...)

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

fixed.

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

Argh, this is a leftover from the first hardware version, where the
flash didn;t worked. Good catch remove this.

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

yep, fixed.

>> +/*
>> + * Environment Configuration
>> + */
>> +#define CONFIG_ENV_OVERWRITE
> 
> Please remove.

done.

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

removed this here and fixed CONFIG_SYS_LOAD_ADDR to 0x100000

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

done

Thanks for the review

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany