[U-Boot] [PATCH v3] ARM: Avoid compiler optimization for usages of readb, writeb and friends.

[Dirk Behme <dirk.behme@googlemail.com>]

On 02.01.2011 13:43, Alexander Holler wrote:
> Am 01.01.2011 20:21, schrieb Dirk Behme:
>> On 01.01.2011 19:47, Alexander Holler wrote:
>>> Am 01.01.2011 19:25, schrieb Dirk Behme:
>>>> On 01.01.2011 18:52, Alexander Holler wrote:
>>>>> Hello,
>>>>>
>>>>> Am 01.01.2011 13:04, schrieb Dirk Behme:
>>>>>> On 22.12.2010 12:04, Alexander Holler wrote:
>>>>>>> gcc 4.5.1 seems to ignore (at least some) volatile definitions,
>>>>>>> avoid that as done in the kernel.
>>>>>>>
>>>>>>> Reading C99 6.7.3 8 and the comment 114) there, I think it is a
>>>>>>> bug of
>>>>>>> that
>>>>>>> gcc version to ignore the volatile type qualifier used e.g. in
>>>>>>> __arch_getl().
>>>>>>> Anyway, using a definition as in the kernel headers avoids such
>>>>>>> optimizations when
>>>>>>> gcc 4.5.1 is used.
>>>>>>>
>>>>>>> Maybe the headers as used in the current linux-kernel should be
>>>>>>> used,
>>>>>>> but to avoid large changes, I've just added a small change to the
>>>>>>> current headers.
>>>>>
>>>>>> Do you like to test the patch in the attachment? I named it 'v4'.
>>>>>>
>>>>>> After some thinking and testing, it seems to me that the volatile
>>>>>> optimization issue this patch shall fix is only with the readx()
>>>>>> macros.
>>>>>> So the idea is to drop all writex() changes done in the v3
>>>>>> version of
>>>>>> this patch. With dropping the writex() changes, we would drop all
>>>>>> issues
>>>>>> we discussed with e.g. the GCC statement-expression and the do
>>>>>> while
>>>>>> workaround, too.
>>>>>
>>>>> I've come across a bug which reads as the problem might be fixed in
>>>>> gcc 4.5.2:
>>>>>
>>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45052
>>>>>
>>>>> I will test gcc 4.5.2 in the next days.
>>>>
>>>> Have you been able to test v4 of the patch I sent with gcc 4.5.1?
>>>
>>> No, sorry, I don't have a test case for consequent write* and I will
>>> have to write one.
>>
>> ?
>>
>> If I remember correctly, the test case for this patch was compiling
>> U-Boot with 4.5.1 and then check
>>
>> a) if it boots at Beagle (correct clock.c)
>> b) if NAND works ok (correct omap_gpmc.c)
>>
>> ?
>
> No. None of those must fail when the compiler optimizes consequent
> write* to one write* because the compiler ignores the volatile keyword.
> I've only found the problem with consequent read* (in clock.c), but
> there might be problems with consequent write* somewhere else too. So
> if you remove the change for those write* some other problems might
> arise and just through booting a kernel those might not be found. So I
> think it would be dangerous to remove the change for write* when using
> gcc 4.5.x
>
> And because the patch fixes only write* and read* some stuff in u-boot
> which uses volatile in another context might still fail, therefore I
> vote to use the current kernel headers where other things besides
> read* and write* are using those barriers too.

Just to understand correctly: Do you want to say that we should ignore 
your v3 patch

http://lists.denx.de/pipermail/u-boot/2010-December/084132.html

?

And that you didn't test the v4 patch

http://lists.denx.de/pipermail/u-boot/2011-January/084481.html

with the test you did in

http://lists.denx.de/pipermail/u-boot/2010-December/084134.html

("tested with both gcc 4.3.5 and gcc 4.5.1 using binutils 2.20.1") 
because you now think this test isn't sufficient?

Thanks

Dirk