Hi Wolfgang, On Wednesday 10 December 2008, Wolfgang Denk wrote: > Sorry for the late review... > > In message <[EMAIL PROTECTED]> you wrote: > > This patch adds the CONFIG_FLASH_NOT_MEM_MAPPED define which can be > > used on boards where the NOR FLASH is not memory-mapped and > > special accessor functions are needed to access the NOR FLASH. > > This affects the memory commands (cmd_mem.c) and the environment > > (env_flash.c). > > > > Boards using CONFIG_FLASH_NOT_MEM_MAPPED need to additionally specify > > CONFIG_FLASH_BASE and CONFIG_FLASH_END so that the NOR FLASH region > > can be detected. > > You have to document this (at least in the README).
No, I missed it. I'll send an updated patch later this week. > But there is a general problem with this approach: U-Boot is based on > the design that the actual flash size is auto-detected, i. e. it is > always assumed to be unknown at compile time. Therefore it is > impossible to set something like CONFIG_FLASH_END at compile time. Yes, this approach is not optimal but I found no other way to work with the CFI driver and especially the environment in NOR without such an "extension". But CONFIG_FLASH_END doesn't have point to the real end of the NOR FLASH but the highest possible address. So auto-detection should still be possible in such an area. Here an example from the VCTH board port: /* * For the non-memory-mapped NOR FLASH, we need to define the * NOR FLASH area. This can't be detected via the addr2info() * function, since we check for flash access in the very early * U-Boot code, before the NOR FLASH is detected. */ #define CONFIG_FLASH_BASE 0xb0000000 #define CONFIG_FLASH_END 0xbfffffff So the FLASH size could be a maximum of 256MB in this case. Not optimal but at least not totally fixed. > > +#if defined(CONFIG_FLASH_NOT_MEM_MAPPED) > > + if (((u32)addr >= CONFIG_FLASH_BASE) && ((u32)addr < CONFIG_FLASH_END)) > > And this looks as if CONFIG_FLASH_END was not the "FLASH_END" address, > i. e. the last address in flash, but "FLASH_END" + 1 ? Good catch. I'll fix this to "((u32)addr <= CONFIG_FLASH_END). > > +#if defined(CONFIG_FLASH_NOT_MEM_MAPPED) > > +#define FLASH_READ_MEMCPY(d, s, n) board_flash_read_memcpy(d, s, n) > > +#else > > +#define FLASH_READ_MEMCPY(d, s, n) memcpy(d, s, n); > > +#endif /* CONFIG_FLASH_NOT_MEM_MAPPED */ > > This is really, really ugly - and error prone, as you must be > extremely careful which of the functions you may call might use > memcpy() or similar internally. > > You know that I know of the specific problems of this hardware, but > anyway - I really dislike having to add such code. Yes, we agree that this is not a "nice" solution. But this is the best I could come up after several approaches. I'm open to better suggestions. > > +#if defined(CONFIG_FLASH_NOT_MEM_MAPPED) > > +u8 flash_read8(void *addr); > > +#define DO1(buf) \ > > + if (((u32)buf >= CONFIG_FLASH_BASE) && ((u32)buf < CONFIG_FLASH_END)) { > > \ + crc = crc_table[((int)crc ^ (flash_read8((void *)buf++))) & > > 0xff] ^ > > \ + (crc >> 8); \ > > + } else { \ > > + crc = crc_table[((int)crc ^ (*buf++)) & 0xff] ^ (crc >> 8); \ > > + } > > +#else > > #define DO1(buf) crc = crc_table[((int)crc ^ (*buf++)) & 0xff] ^ (crc >> > > 8); +#endif > > #define DO2(buf) DO1(buf); DO1(buf); > > #define DO4(buf) DO2(buf); DO2(buf); > > #define DO8(buf) DO4(buf); DO4(buf); > > Please wrap all such macros in the usual "do { ... } while (0)" > wrappers. OK, will do in the next version. Thanks for the review. Best regards, Stefan ===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: [EMAIL PROTECTED] ===================================================================== _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot