[U-Boot] [PATCH] Add support for boards where the NOR FLASH is not memory-mapped

[Wolfgang Denk]

Dear Stefan Roese,

Sorry for the late review...

In message <1226493138-28470-1-git-send-email-sr at 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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
There is nothing in this world constant but inconstancy.      - Swift