Dear Stefan Roese,
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).
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.
+#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 ?
+#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.
+#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.
Best regards,
Wolfgang Denk