[U-Boot] [PATCH] arm: enable unaligned access on ARMv7

[Aneesh V]

On 06/22/2012 03:11 PM, Aneesh V wrote:
> +Tom
>
> Hi Lucas,
>
> On 06/22/2012 04:47 AM, Lucas Stach wrote:
>> Hi Albert,
>>
>> Am Freitag, den 22.06.2012, 13:16 +0200 schrieb Albert ARIBAUD:
>>> Hi Lucas,
>>>
>>>>>> Linux in particular does reinitialize this state and I expect any
>>>>>> reasonable OS to do so.
>>>>>
>>>>> Then what is the point of enabling it on U-Boot? Does it fix some
>>>>> issue whereby some mis-aligned piece of data cannot be properly
>>>>> aligned?
>>>>>
>>>> Yes, it fixes U-Boot USB on Tegra, when built with a recent toolchain.
>>>> Fixing the alignment of some of the structures in the USB code should
>>>> also be done, but this is a whole lot more invasive and requires some
>>>> more thought, as the discussion about this on LKML shows. The issue
>>>> doesn't show for older toolchains, as they by default emit code to
>>>> work around unaligned accesses.
>>>>
>>>> This patch fixes all unaligned issues, that may appear with recent
>>>> toolchains. We avoid having to instruct the toolchain to work around
>>>> unaligned accesses and gain better performance in cases where it is
>>>> needed.
>>>
>>> I am not too happy with enabling a lax behavior only to avoid an
>>> issue which apparently is diagnosed and could / should be fixed at
>>> its root. Can you point me to the relevant LKML thread
>>> so that I get the details and understand what prevents fixing this by
>>> 'simply' aligning the USB structures?
>>
>> I'm with you that the issue for this particular fault that I ran into
>> should be fixed at it's root and I will do so as soon as I have enough
>> time to do so, i.e. within the next three weeks.
>> You can find a thread about this here:
>> https://lkml.org/lkml/2011/4/27/278
>> The problem here is that simply removing the packed attribute is not the
>> right thing to do for structures that are used for accessing hardware
>> registers. I have to read a bit more of the USB code to come up with the
>> right thing to do for every structure there.
>>
>> But apart from this, we certainly have situations where we have
>> unaligned accesses that are justified and could not be removed.
>> Activating the aligned access hardware check is crippling a feature of
>> the ARMv7 architecture that's apparently useful enough that all recent
>> toolchains default to using it and not using compiler side workarounds.
>> Furthermore setting the check bit doesn't even deactivate unaligned
>> access (there is also a bit for this, which is forced to 1 by all v7
>> implementations), but just traps the processor in case you care about
>> such unaligned accesses. Linux for example only sets this check bit if
>> you choose to install a trap handler for this.
>>
>> I cannot see how enabling a hardware feature can be seen as allowing of
>> lax behaviour. As some of the USB structs are used to access hardware
>> registers, we can not align every struct there. Our options are either:
>> 1. instruct the toolchain to insert workaround code or
>> 2. allow v7 hardware to do the unaligned access on it's own
>> My comment about fixing the USB code applies only to part of the structs
>> used there as some of them generate _unnecessary_ unaligned accesses,
>> the issue that all unaligned accesses fail with current U-Boot built
>> with a recent toolchain has to be fixed either way and is exactly what
>> this patch does.
>
> I think this issue was discussed before here(I haven't gone through all
> the details of your problem, but it looks similar). And I think Tom
> fixed this by wrapping the problematic accesses with get/set_unaligned().
>
> Please look at this thread, especially starting from my post reporting
> the OMAP4 regression:
> http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/113347/

BTW, I agree that enabling un-aligned access is not a bad idea.

br,
Aneeesh