Hi Wolfgang,
On Wednesday 10 December 2008, Wolfgang Denk wrote:
Sorry for the late review...
In message 1226493138-28470-1-git-send-email-sr@denx.de 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: office@denx.de =====================================================================