[U-Boot] [PATCH] USB:host: Attribute packed removed from usb structures

[Mike Frysinger]

On Tuesday 06 March 2012 07:06:22 Amit Virdi wrote:
> On 2/29/2012 3:55 PM, Amit Virdi wrote:
> > On 2/28/2012 6:26 AM, Mike Frysinger wrote:
> >> On Monday 27 February 2012 15:53:29 Marek Vasut wrote:
> >>>> On Monday 27 February 2012 08:14:26 Marek Vasut wrote:
> >>>>>> On 2/25/2012 3:42 PM, Albert ARIBAUD wrote:
> >>>>>>> Le 24/02/2012 12:58, Amit Virdi a écrit :
> >>>>>>>> Packed attribute is forcing a bytewise write on device registers,
> >>>>>>>> there by, resulting in a misbehavior on gcc-4.4.1.
> >>>>>>>> Reverting the structures to non-packed
> >>>>>>> 
> >>>>>>> If (just asking, not asserting) the issue is caused by fields being
> >>>>>>> u8 where u8 access is not possible, then should you not make the
> >>>>>>> fields u16 / u32 according to access requirements?
> >>>>>> 
> >>>>>> The problem is not with the fields being of a different width.
> >>>>>> AFAIK, the packed attribute changes the generated code to access
> >>>>>> even the word field elements in a byte by byte manner
> >>>>>> 
> >>>>>> Infact, there is a discussion on lkml that I can point
> >>>>>> https://lkml.org/lkml/2011/4/27/278
> >>>>>> 
> >>>>>> It seems that the discussion did not lead to a conclusion but it was
> >>>>>> sensible (at least for ARM) to remove the packed attribute from this
> >>>>>> structure
> >>>>> 
> >>>>> What does the USB spec say ? It might be a HW bug?
> >>>> 
> >>>> it isn't covered by the USB spec. these are structs for hardware
> >>>> registers in the EHCI usb host controller.
> >>> 
> >>> I see ... so replacing them with unions of accessors where it colides
> >>> might
> >>> work ?
> >> 
> >> i'm not entirely sure what you're asking, but i think you're pointing
> >> to the
> >> right answer: drivers/usb/host/ehci.h:ehci_{read,write}l() should
> >> *not* be
> >> casting/derferencing the pointers directly. they should instead be using
> >> standard {read,write}l() funcs from asm/io.h.
> >> 
> >> Amit: can you post a new patch that does that instead ? don't touch the
> >> packed attribute, but change ehci_readl() to use readl() and
> >> ehci_writel() to
> >> use writel() ?
> > 
> > I'll make the changes and post the patch after testing successfully.
> 
> I did the changes suggested by you and tested the build. The issue
> didn't come up. Then I reverted the code to the original (attributes
> retained and ehci directly de-referencing the pointers. The issue didn't
> come here too.
> 
> Today, I used armv7-linux-gcc (GCC) v4.6.2
> So I suspect there has been some fix done in the GCC.
> 
> Now, even with the packed attributes, the word fields are accessed
> word-by-word in contrast to the earlier observed behavior
> (byte-by-byte). I could see ldr and str in the disassembly.
> 
> May be, we can discard this patch and keep drivers/usb/host/ehci.h intact.

not sure ... in general, device registers should be accessed with io.h helpers 
instead of dereferenced by volatile pointers.  i can think of cases on Blackfin 
where the io.h helpers would be required, and even using volatile pointers 
could result in misbehavior.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120306/01f16bf8/attachment.pgp>