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

[Amit Virdi]

Hello Mike and Marek,

On 2/29/2012 3:55 PM, Amit Virdi wrote:
> Hello Mike,
>
> 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.

Thanks
Amit Virdi