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

[Stefan Roese]

Hi Wolfgang,

On Wednesday 10 December 2008, Wolfgang Denk wrote:
> 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).

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 at denx.de
=====================================================================