Hello Wolfgang,
Wolfgang Denk wrote:
> In message <[email protected]> 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
_______________________________________________
U-Boot mailing list
[email protected]
http://lists.denx.de/mailman/listinfo/u-boot
