| Message ID | 1338918451-10126-1-git-send-email-dev@lynxeye.de |
|---|---|
| State | Rejected |
| Delegated to: | Albert ARIBAUD |
| Headers | show |
On 06/05/2012 11:47 AM, Lucas Stach wrote: > Recent toolchains default to using the hardware feature for unaligned access on > ARM v7, rather than doing the software fallback. According to ARM this is safe > as all v7 implementations have to support this feature. > (http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0471c/BABJFFAE.html) > > To avoid CPU hangs when doing unaligned memory access, we have to turn off > alignment checking in our CPU initialisation code. > (http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0491c/CIHCGCFD.html) Does this behavior change trickle down to Linux/... too, or would an OS completely re-initialize this state, and hence not be affected?
Hi Stephen, Am Dienstag, den 05.06.2012, 12:42 -0600 schrieb Stephen Warren: > On 06/05/2012 11:47 AM, Lucas Stach wrote: > > Recent toolchains default to using the hardware feature for unaligned access on > > ARM v7, rather than doing the software fallback. According to ARM this is safe > > as all v7 implementations have to support this feature. > > (http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0471c/BABJFFAE.html) > > > > To avoid CPU hangs when doing unaligned memory access, we have to turn off > > alignment checking in our CPU initialisation code. > > (http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0491c/CIHCGCFD.html) > > Does this behavior change trickle down to Linux/... too, or would an OS > completely re-initialize this state, and hence not be affected? > Linux in particular does reinitialize this state and I expect any reasonable OS to do so. -- Lucas
Hi Lucas, On Tue, 05 Jun 2012 21:06:20 +0200, Lucas Stach <dev@lynxeye.de> wrote: > Hi Stephen, > > Am Dienstag, den 05.06.2012, 12:42 -0600 schrieb Stephen Warren: > > On 06/05/2012 11:47 AM, Lucas Stach wrote: > > > Recent toolchains default to using the hardware feature for > > > unaligned access on ARM v7, rather than doing the software > > > fallback. According to ARM this is safe as all v7 implementations > > > have to support this feature. > > > (http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0471c/BABJFFAE.html) > > > > > > To avoid CPU hangs when doing unaligned memory access, we have to > > > turn off alignment checking in our CPU initialisation code. > > > (http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0491c/CIHCGCFD.html) > > > > Does this behavior change trickle down to Linux/... too, or would > > an OS completely re-initialize this state, and hence not be > > affected? > > > > 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? Amicalement,
Am Freitag, den 22.06.2012, 11:15 +0200 schrieb Albert ARIBAUD: > Hi Lucas, > > On Tue, 05 Jun 2012 21:06:20 +0200, Lucas Stach <dev@lynxeye.de> wrote: > > Hi Stephen, > > > > Am Dienstag, den 05.06.2012, 12:42 -0600 schrieb Stephen Warren: > > > On 06/05/2012 11:47 AM, Lucas Stach wrote: > > > > Recent toolchains default to using the hardware feature for > > > > unaligned access on ARM v7, rather than doing the software > > > > fallback. According to ARM this is safe as all v7 implementations > > > > have to support this feature. > > > > (http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0471c/BABJFFAE.html) > > > > > > > > To avoid CPU hangs when doing unaligned memory access, we have to > > > > turn off alignment checking in our CPU initialisation code. > > > > (http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0491c/CIHCGCFD.html) > > > > > > Does this behavior change trickle down to Linux/... too, or would > > > an OS completely re-initialize this state, and hence not be > > > affected? > > > > > > > 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. Thanks, Lucas
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? > Thanks, > Lucas Amicalement,
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. Thanks, Lucas
+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/ best regards, Aneesh
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
Hi Aneesh, On Fri, 22 Jun 2012 15:13:39 -0700, Aneesh V <aneesh@ti.com> wrote: > 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: > >>> 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 From what I understand, the issue was not to allow unaligned access at the hardware level, but to modify the attributes of the structure, first by removing the packed attribute, then by reinstating the packed attribute along with align(4). > >> But apart from this, we certainly have situations where we have > >> unaligned accesses that are justified and could not be removed. > >> [...] > >> 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. If the access is in true RAM, then we can always realign the data; and I don't know of memory-mapped registers which would be unaligned wrt their width. If some USB controller is designed so, then the fix should only and explicitly affect that controller, because we don't know it it will always be used with an ARM implementation that can do unaligned accesses. > > 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(). Could be a solution if the structures themselves cannot be fixed. > > 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/ Thanks for the reference. There seems to have been no confirmation that the structures involved needed packing in the first place, and my general opinion on packing structures is "DO NOT" -- if packing a structure has an effect, it can alway be to de-align some field, which barely makes sense as far as HW prgramming is concerned (I can only see some point in packing a struct when you deal with network layer 7 data in very special cases). > BTW, I agree that enabling un-aligned access is not a bad idea. Just being "not a bad idea" is not enough for me to accept this. It will have to be the sole sound solution to a problem, and at this point, I do not think it is as far as USB structure mis-alignement issues are concerned. > br, > Aneeesh Amicalement,
Hi Albert, On Sat, Jun 23, 2012 at 2:01 AM, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote: > Hi Aneesh, > > On Fri, 22 Jun 2012 15:13:39 -0700, Aneesh V <aneesh@ti.com> wrote: >> 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: > >> >>> 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 > > From what I understand, the issue was not to allow unaligned access at > the hardware level, but to modify the attributes of the structure, > first by removing the packed attribute, then by reinstating the packed > attribute along with align(4). > >> >> But apart from this, we certainly have situations where we have >> >> unaligned accesses that are justified and could not be removed. >> >> [...] >> >> 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. > > If the access is in true RAM, then we can always realign the data; and > I don't know of memory-mapped registers which would be unaligned wrt > their width. If some USB controller is designed so, then the fix should > only and explicitly affect that controller, because we don't know it it > will always be used with an ARM implementation that can do unaligned > accesses. > >> > 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(). > > Could be a solution if the structures themselves cannot be fixed. > >> > 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/ > > Thanks for the reference. There seems to have been no confirmation that > the structures involved needed packing in the first place, and my > general opinion on packing structures is "DO NOT" -- if packing a > structure has an effect, it can alway be to de-align some field, which > barely makes sense as far as HW prgramming is concerned (I can only see > some point in packing a struct when you deal with network layer 7 data > in very special cases). > >> BTW, I agree that enabling un-aligned access is not a bad idea. > > Just being "not a bad idea" is not enough for me to accept this. It will > have to be the sole sound solution to a problem, and at this point, I > do not think it is as far as USB structure mis-alignement issues are > concerned. My point is that enabling un-aligned accesses in itsown merit is not a bad idea, not as a solution to this problem. I have seen it being enabled in HLOS environment. TI's Symbian port for instance had it enabled for OMAP3. I don't know why Linux too shoudln't take advantage of such hw features. Perhaps you don't want to take it at this point of time to force the real solution to the USB problem, which is reasonable.
Albert ARIBAUD <albert.u.boot@aribaud.net> writes: >> >> 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. > > If the access is in true RAM, then we can always realign the data; and > I don't know of memory-mapped registers which would be unaligned wrt > their width. If some USB controller is designed so, then the fix should > only and explicitly affect that controller, because we don't know it it > will always be used with an ARM implementation that can do unaligned > accesses. The ARM architecture does not permit unaligned accesses to strongly ordered or device memory, so MMIO register accesses are always aligned.
Hi Albert, Am Samstag, den 23.06.2012, 11:01 +0200 schrieb Albert ARIBAUD: [snip] > > >> But apart from this, we certainly have situations where we have > > >> unaligned accesses that are justified and could not be removed. > > >> [...] > > >> 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. > > If the access is in true RAM, then we can always realign the data; and > I don't know of memory-mapped registers which would be unaligned wrt > their width. If some USB controller is designed so, then the fix should > only and explicitly affect that controller, because we don't know it it > will always be used with an ARM implementation that can do unaligned > accesses. My point is: on ARM platforms that can't do unaligned access in hardware the toolchain will automaticly emit helper code to work around this. On an ARMv7 platform hardware assisted unaligned access is mandatory, so toolchains will not emit helper code and rather let the hardware do it's job. Now the situation is as follows: hardware platforms without the ability to do unaligned access in hardware will just work even though the code is suboptimal, but users of an ARMv7 platform will encounter unexplained system hangs, which is bad imho. This patch just makes behaviour consistent across ARMv5 and ARMv7 platforms. If we really want to disallow the use of unaligned memory operations in U-Boot we should make all platforms fail at compile time, not make one platform fail randomly at runtime. Do you think this is the way to go? I'm fine either way, I'm just not okay with the current situation where one platform fails while another just works. Thanks, Lucas
Hi Aneesh, > >> BTW, I agree that enabling un-aligned access is not a bad idea. > > > > Just being "not a bad idea" is not enough for me to accept this. It > > will have to be the sole sound solution to a problem, and at this > > point, I do not think it is as far as USB structure mis-alignement > > issues are concerned. > > My point is that enabling un-aligned accesses in itsown merit > is not a bad idea, not as a solution to this problem. I have seen > it being enabled in HLOS environment. TI's Symbian port for > instance had it enabled for OMAP3. I don't > know why Linux too shoudln't take advantage of such hw > features. Perhaps you don't want to take it at this point of time to > force the real solution to the USB problem, which is reasonable. What is the (non-contrived) problem to which allowing mis-aligned accesses would be a solution? Amicalement,
Hi Albert, Am Montag, den 25.06.2012, 22:17 +0200 schrieb Albert ARIBAUD: > Hi Lucas, > > On Sun, 24 Jun 2012 08:30:19 +0200, Lucas Stach <dev@lynxeye.de> wrote: > > Hi Albert, > > > > Am Samstag, den 23.06.2012, 11:01 +0200 schrieb Albert ARIBAUD: > > [snip] > > > > >> But apart from this, we certainly have situations where we have > > > > >> unaligned accesses that are justified and could not be removed. > > > > >> [...] > > > > >> 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. > > > > > > If the access is in true RAM, then we can always realign the data; > > > and I don't know of memory-mapped registers which would be > > > unaligned wrt their width. If some USB controller is designed so, > > > then the fix should only and explicitly affect that controller, > > > because we don't know it it will always be used with an ARM > > > implementation that can do unaligned accesses. > > > > My point is: on ARM platforms that can't do unaligned access in > > hardware the toolchain will automaticly emit helper code to work > > around this. On an ARMv7 platform hardware assisted unaligned access > > is mandatory, so toolchains will not emit helper code and rather let > > the hardware do it's job. > > My point is that there should be a reason for doing unaligned access to > boot (pun half-intended) and I don't see any. > I see your point here. > > Now the situation is as follows: hardware platforms without the > > ability to do unaligned access in hardware will just work even though > > the code is suboptimal, but users of an ARMv7 platform will encounter > > unexplained system hangs, which is bad imho. This patch just makes > > behaviour consistent across ARMv5 and ARMv7 platforms. > > Wouldn't aligning data properly make behaviour consistent as well? > Consistent in the way of "it works on all platforms", but not in the way of programming practice. ARMv7 people will have to care about alignment to not encounter runtime errors, while on ARMv5 it just doesn't matter. (Which certainly does not make it the right thing to do on ARMv5, but as the existing USB code shows people just haven't cared about this.) > > If we really want to disallow the use of unaligned memory operations > > in U-Boot we should make all platforms fail at compile time, not make > > one platform fail randomly at runtime. Do you think this is the way > > to go? I'm fine either way, I'm just not okay with the current > > situation where one platform fails while another just works. > > I do not want to disallow unaligned accesses, because this assumes > they were allowed in the first place; I want to keep not allowing them, > just like they were unallowed far, and just like they can keep on > being. If a misaligned access causes one platform to fail while another > just works, then we should avoid the misaligned access by re-aligning > it properly so as to make both platforms work. > The thing I wanted to say is: if we generally consider unaligned access a no-go for ARMv7 (which I'm fine with), we should equally consider it a no-go for ARMv5. So I would rather want to see both platforms fail at compile time if we have unaligned data, to avoid the unintended introduction of such code in the codebase as has happened with the usb code. Sadly I have not found a way to tell gcc to error out if encounters an unaligned access. Currently ARMv5 "allows" unaligned data, as toolchains insert helper code which makes it work. Also it is really unfortunate that coding errors (which the unjustified use of unaligned data certainly is) manifest as a runtime error, rather than a compiletime failure, which make them a lot harder to spot. > IOW, things will not change until I am shown some actual situation where > a misaligned access is necessary and the core prevents it. > I'm okay with you dropping this patch and will try to come up with something that make programming constraints more consistent across both ARM platforms. Thanks, Lucas
Hi Albert, On 06/25/2012 01:34 PM, Albert ARIBAUD wrote: > Hi Aneesh, > >>>> BTW, I agree that enabling un-aligned access is not a bad idea. >>> >>> Just being "not a bad idea" is not enough for me to accept this. It >>> will have to be the sole sound solution to a problem, and at this >>> point, I do not think it is as far as USB structure mis-alignement >>> issues are concerned. >> >> My point is that enabling un-aligned accesses in itsown merit >> is not a bad idea, not as a solution to this problem. I have seen >> it being enabled in HLOS environment. TI's Symbian port for >> instance had it enabled for OMAP3. I don't >> know why Linux too shoudln't take advantage of such hw >> features. Perhaps you don't want to take it at this point of time to >> force the real solution to the USB problem, which is reasonable. > > What is the (non-contrived) problem to which allowing mis-aligned > accesses would be a solution? memcpy() when there is a mismatch in the alignment of source and destination buffers. Let's say the source buffer is 4 byte aligned but the destination buffer is only 2 byte aligned. I believe relaxed alignment requirements will help in writing an accelerated memcpy routine for this case. Again, my point is that as a platform software provider I would like to enable such features to make maximum things work on my platform, where as the developer of a generic sw module should probably avoid depending on such features to ensure maximum portability. br, Aneesh
Dear Aneesh V, In message <4FE8DCE7.7090700@ti.com> you wrote: > > > What is the (non-contrived) problem to which allowing mis-aligned > > accesses would be a solution? > > memcpy() when there is a mismatch in the alignment of source and > destination buffers. Let's say the source buffer is 4 byte aligned > but the destination buffer is only 2 byte aligned. I believe relaxed > alignment requirements will help in writing an accelerated memcpy > routine for this case. If memcpy() has problems with handling such a situation, then clearly memcpy() needs fixing. Best regards, Wolfgang Denk
On 06/22/2012 04:15 AM, Albert ARIBAUD wrote: > Hi Lucas, > > On Tue, 05 Jun 2012 21:06:20 +0200, Lucas Stach <dev@lynxeye.de> wrote: >> Hi Stephen, >> >> Am Dienstag, den 05.06.2012, 12:42 -0600 schrieb Stephen Warren: >>> On 06/05/2012 11:47 AM, Lucas Stach wrote: >>>> Recent toolchains default to using the hardware feature for >>>> unaligned access on ARM v7, rather than doing the software >>>> fallback. According to ARM this is safe as all v7 implementations >>>> have to support this feature. >>>> (http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0471c/BABJFFAE.html) >>>> >>>> To avoid CPU hangs when doing unaligned memory access, we have to >>>> turn off alignment checking in our CPU initialisation code. >>>> (http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0491c/CIHCGCFD.html) >>> >>> Does this behavior change trickle down to Linux/... too, or would >>> an OS completely re-initialize this state, and hence not be >>> affected? >>> >> >> 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? > This is a new optimization feature in gcc 4.7 (and backported to some 4.6 versions like the ubuntu 12.04 arm cross compiler (4.6.3)): http://lists.linaro.org/pipermail/linaro-dev/2012-June/012360.html http://seabright.co.nz/2012/06/11/kernel-not-booting-with-linaro-gcc/ If you don't want to enable unaligned accesses, then "-mno-unaligned-access" needs to be added. Regards, Rob
Hi Rob and all, >>> >>> Am Dienstag, den 05.06.2012, 12:42 -0600 schrieb Stephen Warren: >>>> On 06/05/2012 11:47 AM, Lucas Stach wrote: >>>>> Recent toolchains default to using the hardware feature for >>>>> unaligned access on ARM v7, rather than doing the software >>>>> fallback. According to ARM this is safe as all v7 implementations >>>>> have to support this feature. >>>>> (http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0471c/BABJFFAE.html) >>>>> >>>>> To avoid CPU hangs when doing unaligned memory access, we have to >>>>> turn off alignment checking in our CPU initialisation code. >>>>> (http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0491c/CIHCGCFD.html) >>>> >>>> Does this behavior change trickle down to Linux/... too, or would >>>> an OS completely re-initialize this state, and hence not be >>>> affected? >>>> >>> >>> 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? >> > > This is a new optimization feature in gcc 4.7 (and backported to some > 4.6 versions like the ubuntu 12.04 arm cross compiler (4.6.3)): > > http://lists.linaro.org/pipermail/linaro-dev/2012-June/012360.html > > http://seabright.co.nz/2012/06/11/kernel-not-booting-with-linaro-gcc/ > > If you don't want to enable unaligned accesses, then > "-mno-unaligned-access" needs to be added. > I verified it. Option "-mno-unaligned-access" works good. include/mtd/cfi_flash.h /* CFI standard query structure */ struct cfi_qry { u8 qry[3]; u16 p_id; <-- unaligned! ... } __attribute__((packed)); $ ${CROSS_COMPILE}gcc --version arm-none-eabi-gcc (GNU Tools for ARM Embedded Processors) 4.6.2 20110921 (release) [ARM/embedded-4_6-branch revision 182083] Copyright (C) 2011 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. ===================================================================== Compiled without --mno-unaligned-access $ ${CROSS_COMPILE}objdump -d -S u-boot info->vendor = le16_to_cpu(qry.p_id); cc88: e3003a1c movw r3, #2588 ; 0xa1c cc8c: e1dd11bb ldrh r1, [sp, #27] <-- this is unaligned access cc90: ... cc94: e18410b3 strh r1, [r4, r3] ===================================================================== Compiled with --mno-unaligned-access $ ${CROSS_COMPILE}objdump -d -S u-boot info->vendor = le16_to_cpu(qry.p_id); cce8: e5dd101c ldrb r1, [sp, #28] <-- ccec: e5dd301b ldrb r3, [sp, #27] <-- separated 2 byte accesses ccf0: ... ccf4: e1831401 orr r1, r3, r1, lsl #8 ccf8: e3003a1c movw r3, #2588 ; 0xa1c ccfc: e18410b3 strh r1, [r4, r3]
diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S index 261835b..52f7f6e 100644 --- a/arch/arm/cpu/armv7/start.S +++ b/arch/arm/cpu/armv7/start.S @@ -316,7 +316,7 @@ ENTRY(cpu_init_cp15) mrc p15, 0, r0, c1, c0, 0 bic r0, r0, #0x00002000 @ clear bits 13 (--V-) bic r0, r0, #0x00000007 @ clear bits 2:0 (-CAM) - orr r0, r0, #0x00000002 @ set bit 1 (--A-) Align + bic r0, r0, #0x00000002 @ clear bit 1 (--A-) Align orr r0, r0, #0x00000800 @ set bit 11 (Z---) BTB #ifdef CONFIG_SYS_ICACHE_OFF bic r0, r0, #0x00001000 @ clear bit 12 (I) I-cache
Recent toolchains default to using the hardware feature for unaligned access on ARM v7, rather than doing the software fallback. According to ARM this is safe as all v7 implementations have to support this feature. (http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0471c/BABJFFAE.html) To avoid CPU hangs when doing unaligned memory access, we have to turn off alignment checking in our CPU initialisation code. (http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0491c/CIHCGCFD.html) Signed-off-by: Lucas Stach <dev@lynxeye.de> CC: Albert ARIBAUD <albert.u.boot@aribaud.net> --- arch/arm/cpu/armv7/start.S | 2 +- 1 Datei geändert, 1 Zeile hinzugefügt(+), 1 Zeile entfernt(-)