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:
- instruct the toolchain to insert workaround code or
- 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