> From: Gabbasov, Andrew > Sent: Tuesday, May 14, 2013 21:27 > To: Albert ARIBAUD; Marek Vasut > Cc: u-boot@lists.denx.de; Masahiro Yamada; tr...@ti.com > Subject: RE: [U-Boot] ARM: cfi_flash: Fix unaligned accesses to cfi_qry > structure > > Hello Albert, > > > From: Albert ARIBAUD [albert.u.b...@aribaud.net] > > Sent: Monday, May 13, 2013 10:48 > > To: Marek Vasut > > Cc: u-boot@lists.denx.de; Masahiro Yamada; tr...@ti.com; Gabbasov, Andrew > > Subject: Re: [U-Boot] ARM: cfi_flash: Fix unaligned accesses to cfi_qry > > structure > > > > Hi Marek, > > > > On Fri, 10 May 2013 14:36:00 +0200, Marek Vasut <ma...@denx.de> wrote: > > > > > Hello Masahiro-san, > > > > > > By the way, I also had this unalignment access problem for my board. > > > > Before finding your patch, I was thinking another way to fix this > > > > problem. > > > > > > > > My idea is to just use 'get_unaligned' and 'put_unaligned' functions > > > > instead of introducing special macros. > > > > With this way, we don't need to change the fields of struct cfi_qry. > > > > > > I think we should make sure to use natural alignment as much as possible, > > > really. I'm keeping Albert in CC because this is his turf. > > > > Marek, you invoked me; next time be careful what you wish for. :) > > > > My rules (not 'of thumb', as they also apply to ARM) regarding > > alignment are as follows (yes, it's a more general answer than your > > question called for, but the last rules are more directly related): > > > > 0) Never assume that U-Boot can use native unaligned accesses. Yes, > > ARMv7+ can do native unaligned accesses with almost no performance > > cost, but U-Boot runs on all sorts of targets (not only ARM), some > > allowing unaligned access but with a penalty, and some unable to > > perform unaligned access at all. We always run the risk that some code > > in U-Boot ends up running on target which will die horribly on some > > unaligned access, so to avoid this we must assume and enforce strict > > alignment, even for architectures which do not require it, such as > > ARMv7+. > > > > 1) As per rule 0, always enable alignment check -- again, even on > > targets which could do without it. This allows catching any unaligned > > access, be they accidental (bad pointer arithmetic) or by design > > (misaligned field in an otherwise aligned struct). > > > > 2) Despite rules 0 and 1, always enable native unaligned accesses (IOW, > > always use option -munaligned-accesses). That is because without this > > option (or more precisely, when -mno-unaligned-accesses is in effect), > > the ARM compiler may silently detect misaligned accesses and fix them > > using smaller aligned accesses, thus hiding a potential alignment > > issue until it bites on some later and unrelated target run. > > > > 3) always size fields in a structure to their natural size, i.e., if a > > field is 16-bits, make it u16 or s16. > > > > 4) always align fields in a structure to their natural boundaries, > > i.e., if a field is 16-bits, align it to an even address. > > > > 5) if a field absolutely has to be unaligned because of hardware or > > standard, then a) document that! and b) access it with explicit > > unaligned access macros. > > > > So in essence, I am opposed to changing fields from 16-bit to 2 x 8-bit > > just 'because unaligned'. Either fix the fields' alignment, if at all > > possible; and if not, then apply rule 5: document the issue and fix it > > using explicit unaligned access macros. > > > > > Best regards, > > > Marek Vasut > > > > Amicalement, > > -- > > Albert. > > Thank you for your review and the very useful list of rules. > > Although theoretically the cfi_qry structure can be made non-packed and > with properly aligned members (and filled in by individual members assignments > when reading, rather than just copying byte-to-byte, as it is now), > it may make more sense to keep it packed and replicating the actual flash > layout > as much as possible. This also makes it more similar to what is used > in Linux kernel (although it seems to rely on native unaligned access ;-)). > > So, it seems reasonable to choose to apply rule 5: add comments to cfi_qry > strcuture > definition and use get_unaligned/put_unaligned for the necessary fields (only > for really > unaligned members, since some 16-bit fields are properly aligned). > > I'm sending the version 2 of the patch with this change as a separate message > with > Subject: [U-Boot][PATCH v2] cfi_flash: Fix unaligned accesses to cfi_qry > structure > > Please, let me know if somebody still prefers making non-packed aligned > structure, > as described above. > > Thanks. > > Best regards, > Andrew
Does anybody have any comments on the mentioned updated patch? Thanks. Best regards, Andrew _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
