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
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot