From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Wed, 07 Jul 2010 10:29:23 +0200 Subject: [U-Boot] [PATCH v2] 83xx: add support for ve8313 board In-Reply-To: <20100707072620.8D082160691@gemini.denx.de> References: <4C31B28E.3060209@denx.de> <20100706182403.9ced634f.kim.phillips@freescale.com> <4C340265.3020302@invitel.hu> <20100707072620.8D082160691@gemini.denx.de> Message-ID: <4C343AE3.7060804@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany