[U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.

Tolunay Orkun
2007-01-13T07:11:56Z

Wolfgang Denk wrote:
> Dear Tolunay,
>
> in message <45A0C777.1000508@orkun.us> you wrote:
>>> Please find attached a small patch that adds fixes this potential problem for
>>> the 3 functions flash_read_uchar/ushort/long. Please give it a try and let me
>>> know if this changed the behavior somehow.
>> I think this is a good idea given GCC 4.x is agressive in optimizations.
>
> I already discussed this internally with Stefan. I *don't* think it's
> a good idea. I really hate to change bits and pieces of code without
> really understanding why we are doing this.
>
> In the current situation we are accessing flash memory which is
> supposed to be in read mode, i. e. it behaves like normal system
> memory. It should be no problem even if the flash memory content was
> cached. So why would "volatile" improve anything?

These functions are used to read data from flash tables. I agree most
tables do not change but I think (need to verify) we also use these
functions to read the status registers to determine if programming is OK
etc. We might have been OK since we probably access these registers once
in a function context. When the function returns the compiler
optimization context is gone so no state data is used. So, it might not
be necessary.

Perhaps, more important issue would be if these areas were cached. In
that case, every time we change the flash from read mode to special
query modes, we should probably invalidate the cache. In PowerPC 405
platform our flash is uncached so this is not necessary. I do not know
if any platform caches its flash areas.

> As long as I don't see (for example in the generated assembler code)
> how a problem might exist, and how the suggested patch fixes exactly
> this problem, I'd like to continue researching this problem.
>
>> I do not think converting these to use memcpy() is a good idea. I am
>> with Wolfgang on this.
>
> Actually I might have been wrong in my assessment here, when I stated
> that memcpy() performs a character-wise copy, too. The simple code
> from lib_generic/string.c is used only if __HAVE_ARCH_MEMCPY is
> undefined, and especially on PPC we define this (in
> include/asm-ppc/string.h). So we might use an optimized versions
> where it *does* make a difference.

Memcopy might be OK for flash tables but I am not sure if we use the
same function to access status registers. I would rather access flash
using bus wide accesses instead of byte by byte. Maybe it is safe but I
do not know how it behaves in various platforms and bus interface units
of various processors. I would take the safe route.

Best regards,
Tolunay