[U-Boot-Users] cfi_flash.c and lost volatile qualifier

[Jerry Van Baren <gerald.vanbaren@ge.com>]

Adrian Filipi wrote:
> On Tue, 29 Apr 2008, Jerry Van Baren wrote:
> 
>> Adrian Filipi wrote:
>>> On Tue, 29 Apr 2008, Wolfgang Denk wrote:
>>>
>>>> In message <alpine.DEB.1.10.0804291604250.32753@pmy.adscville> you 
>>>> wrote:
>>>>>      I narrowed down the source of the problem to the loss of the
>>>>> volatile qualifier on the addr pointer in flash_write_cmd().  
>>>>> Adding the
>>>>> qualifier gets rid of the corruption.  In the older 1.2.0 sources, 
>>>>> addr was
>>>>> declared as "volatile cfiptr_t addr;".
>>>> The volatile should not be needed - the CFI driver should use correct
>>>> accessor macros instead. See
>>>> Documentation/volatile-considered-harmful.txt in your Linux kernel
>>>> source tree...
>>>>
>>>> Best regards,
>>>>
>>>> Wolfgang Denk
>>>
>>>      Sure, reducing the reliance on volatile is a good idea, but I'm 
>>> at a loss for anything better to do.
>>>
>>>      I'm seeing a real problem that is only fixed by qualifying the 
>>> container of the pointer as volatile, i.e. "void *volatile".  
>>> "volatile void *" has no effect as expected given that the read/write 
>>> accessors are used now.
>>>
>>>      The old data type was essentially "volatile void *volatile addr" 
>>> and the new type is simply "void *addr".  I seem to need at least 
>>> "void *volatile addr" for things to work.
>>>
>>>      Note, I'm only seeing this problem on our PXA250 boards.
>>>
>>>      Adrian
>>
>> Hi Adrian,
>>
>> Please bottom post.
>>
>> It may be useful to disassemble cfi_flash.o file (objdump -S) and see 
>> what the two different configurations of flash_write_cmd() get turned 
>> into in assembly.
>>
>> Best regards,
>> gvb
>>
> 
>     I've attached the assembler output as well as the diff to the 
> version with volatile added.  I don't see anything incriminating.
> Instead of using a register, the variable is read from its location on 
> the stack.
> 
>     I'm having a hard time seeing how flash could get corrupted by this 
> code.  It's happening during initialization.
> 
>     Adrian

I'm not an ARMexpert, but if that were a PowerPC CPU, I would *strongly* 
suspect that the bus interface unit is rearranging the bus operations 
going to memory (e.g. moving a read ahead of a write or re-ordering the 
writes).  The result is that the write command/data sequence to the 
flash gets buggered by the re-ordering.

Per my theory, by putting in the "volatile", you force extra reads on 
the bus which forces the bus interface unit to flush out the flash write 
sequence in the right order.  The fix is totally unrelated the the 
problem, other than as an unrecognized side effect it "fixes" the problem.

The solution in the PowerPC world is to add a "eieio" or "sync" 
instruction[1] appropriately[2], which prevents the bus interface unit 
from reordering the memory operations.

Your task is to dig into the PXA250 manual to (a) figure out what syncs 
you need and (b) figure out why the bus interface is doing you in. 
Alternatively, (c) show that I don't know what I'm talking about. 
Obviously, (a) gets you the best results, so lets root for that. ;-)

HTH,
gvb

[1] Sync is a big hammer, eieio is a medium size hammer.  Don't use 
both, PPC people will know you don't know what you are doing.  ;-)

[2] The flash command sequence order is critical (e.g. the 55s and AAs). 
  The actual data sequence is not critical.  The observed problem is in 
flash_write_cmd() that is writing the command sequence.  Hmmmmm.