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

[Alexander Holler]

On 02.01.2011 14:29, Dirk Behme wrote:
> 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?

Sorry, but I don't understand why you are assuming that the compiler 
will only use those (wrong) optimizations on reads and not writes.

If the compiler does the same wrong optimizations for writes (why not, 
if it ignores volatile), your v4 would'nt fix that.

Regards,

Alexander