From mboxrd@z Thu Jan  1 00:00:00 1970
From: Tolunay Orkun <listmember@orkun.us>
Date: Sat, 13 Jan 2007 01:11:56 -0600
Subject: [U-Boot-Users] [PATCH] Fixed cfi flash read uchar bug.
In-Reply-To: <20070107104008.C0083352636@atlas.denx.de>
References: <20070107104008.C0083352636@atlas.denx.de>
Message-ID: <45A8863C.1040303@orkun.us>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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