Hello Wolfgang,
Wolfgang Denk wrote:
In message 4C340265.3020302@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