* [U-Boot] [PATCH] USB:host: Attribute packed removed from usb structures
@ 2012-02-24 11:58 Amit Virdi
2012-02-24 19:18 ` Mike Frysinger
2012-02-25 10:12 ` Albert ARIBAUD
0 siblings, 2 replies; 20+ messages in thread
From: Amit Virdi @ 2012-02-24 11:58 UTC (permalink / raw)
To: u-boot
From: Vipin Kumar <vipin.kumar@st.com>
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
Signed-off-by: Vipin Kumar <vipin.kumar@st.com>
Signed-off-by: Amit Virdi <amit.virdi@st.com>
---
drivers/usb/host/ehci.h | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index 3d0ad0c..df9f055 100644
--- a/drivers/usb/host/ehci.h
+++ b/drivers/usb/host/ehci.h
@@ -55,7 +55,7 @@ struct ehci_hccr {
#define HCS_N_PORTS(p) (((p) >> 0) & 0xf)
uint32_t cr_hccparams;
uint8_t cr_hcsp_portrt[8];
-} __attribute__ ((packed, aligned(4)));
+};
struct ehci_hcor {
uint32_t or_usbcmd;
@@ -85,7 +85,7 @@ struct ehci_hcor {
#define FLAG_CF (1 << 0) /* true: we'll support "high speed" */
uint32_t or_portsc[CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS];
uint32_t or_systune;
-} __attribute__ ((packed, aligned(4)));
+};
#define USBMODE 0x68 /* USB Device mode */
#define USBMODE_SDIS (1 << 3) /* Stream disable */
--
1.7.2.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [U-Boot] [PATCH] USB:host: Attribute packed removed from usb structures
2012-02-24 11:58 [U-Boot] [PATCH] USB:host: Attribute packed removed from usb structures Amit Virdi
@ 2012-02-24 19:18 ` Mike Frysinger
2012-02-27 10:02 ` Amit Virdi
2012-02-25 10:12 ` Albert ARIBAUD
1 sibling, 1 reply; 20+ messages in thread
From: Mike Frysinger @ 2012-02-24 19:18 UTC (permalink / raw)
To: u-boot
On Friday 24 February 2012 06:58:40 Amit Virdi wrote:
> Packed attribute is forcing a bytewise write on device registers,
> there by, resulting in a misbehavior on gcc-4.4.1.
so use a compiler that isn't broken ?
-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/20120224/bcf52a06/attachment.pgp>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [U-Boot] [PATCH] USB:host: Attribute packed removed from usb structures
2012-02-24 11:58 [U-Boot] [PATCH] USB:host: Attribute packed removed from usb structures Amit Virdi
2012-02-24 19:18 ` Mike Frysinger
@ 2012-02-25 10:12 ` Albert ARIBAUD
2012-02-27 7:16 ` Vipin Kumar
1 sibling, 1 reply; 20+ messages in thread
From: Albert ARIBAUD @ 2012-02-25 10:12 UTC (permalink / raw)
To: u-boot
Hi Amit,
Le 24/02/2012 12:58, Amit Virdi a ?crit :
> From: Vipin Kumar<vipin.kumar@st.com>
>
> 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?
Amicalement,
--
Albert.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [U-Boot] [PATCH] USB:host: Attribute packed removed from usb structures
2012-02-25 10:12 ` Albert ARIBAUD
@ 2012-02-27 7:16 ` Vipin Kumar
2012-02-27 13:14 ` Marek Vasut
0 siblings, 1 reply; 20+ messages in thread
From: Vipin Kumar @ 2012-02-27 7:16 UTC (permalink / raw)
To: u-boot
On 2/25/2012 3:42 PM, Albert ARIBAUD wrote:
> Hi Amit,
>
Hello Albert,
> Le 24/02/2012 12:58, Amit Virdi a ?crit :
>> From: Vipin Kumar<vipin.kumar@st.com>
>>
>> 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
Regards
Vipin
> Amicalement,
^ permalink raw reply [flat|nested] 20+ messages in thread
* [U-Boot] [PATCH] USB:host: Attribute packed removed from usb structures
2012-02-24 19:18 ` Mike Frysinger
@ 2012-02-27 10:02 ` Amit Virdi
2012-02-27 18:25 ` Mike Frysinger
0 siblings, 1 reply; 20+ messages in thread
From: Amit Virdi @ 2012-02-27 10:02 UTC (permalink / raw)
To: u-boot
Hello Mike,
On 2/25/2012 12:48 AM, Mike Frysinger wrote:
> On Friday 24 February 2012 06:58:40 Amit Virdi wrote:
>> Packed attribute is forcing a bytewise write on device registers,
>> there by, resulting in a misbehavior on gcc-4.4.1.
>
> so use a compiler that isn't broken ?
It doesn't seem like compiler issue (on ARM).
Thanks
Amit Virdi
^ permalink raw reply [flat|nested] 20+ messages in thread
* [U-Boot] [PATCH] USB:host: Attribute packed removed from usb structures
2012-02-27 7:16 ` Vipin Kumar
@ 2012-02-27 13:14 ` Marek Vasut
2012-02-27 18:26 ` Mike Frysinger
0 siblings, 1 reply; 20+ messages in thread
From: Marek Vasut @ 2012-02-27 13:14 UTC (permalink / raw)
To: u-boot
> On 2/25/2012 3:42 PM, Albert ARIBAUD wrote:
> > Hi Amit,
>
> Hello Albert,
>
> > Le 24/02/2012 12:58, Amit Virdi a ?crit :
> >> From: Vipin Kumar<vipin.kumar@st.com>
> >>
> >> 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?
M
^ permalink raw reply [flat|nested] 20+ messages in thread
* [U-Boot] [PATCH] USB:host: Attribute packed removed from usb structures
2012-02-27 10:02 ` Amit Virdi
@ 2012-02-27 18:25 ` Mike Frysinger
0 siblings, 0 replies; 20+ messages in thread
From: Mike Frysinger @ 2012-02-27 18:25 UTC (permalink / raw)
To: u-boot
On Monday 27 February 2012 05:02:14 Amit Virdi wrote:
> On 2/25/2012 12:48 AM, Mike Frysinger wrote:
> > On Friday 24 February 2012 06:58:40 Amit Virdi wrote:
> >> Packed attribute is forcing a bytewise write on device registers,
> >> there by, resulting in a misbehavior on gcc-4.4.1.
> >
> > so use a compiler that isn't broken ?
>
> It doesn't seem like compiler issue (on ARM).
i think it is ... you don't see this behavior on other architectures. the
recent thread indicates that ARM has been broken for a long time though.
http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/85629
-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/20120227/318004e3/attachment.pgp>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [U-Boot] [PATCH] USB:host: Attribute packed removed from usb structures
2012-02-27 13:14 ` Marek Vasut
@ 2012-02-27 18:26 ` Mike Frysinger
2012-02-27 20:53 ` Marek Vasut
0 siblings, 1 reply; 20+ messages in thread
From: Mike Frysinger @ 2012-02-27 18:26 UTC (permalink / raw)
To: u-boot
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.
-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/20120227/894ffefd/attachment.pgp>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [U-Boot] [PATCH] USB:host: Attribute packed removed from usb structures
2012-02-27 18:26 ` Mike Frysinger
@ 2012-02-27 20:53 ` Marek Vasut
2012-02-28 0:56 ` Mike Frysinger
0 siblings, 1 reply; 20+ messages in thread
From: Marek Vasut @ 2012-02-27 20:53 UTC (permalink / raw)
To: u-boot
> 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.
> -mike
I see ... so replacing them with unions of accessors where it colides might work
?
^ permalink raw reply [flat|nested] 20+ messages in thread
* [U-Boot] [PATCH] USB:host: Attribute packed removed from usb structures
2012-02-27 20:53 ` Marek Vasut
@ 2012-02-28 0:56 ` Mike Frysinger
2012-02-29 10:25 ` Amit Virdi
0 siblings, 1 reply; 20+ messages in thread
From: Mike Frysinger @ 2012-02-28 0:56 UTC (permalink / raw)
To: u-boot
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() ?
-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/20120227/79bb511a/attachment.pgp>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [U-Boot] [PATCH] USB:host: Attribute packed removed from usb structures
2012-02-28 0:56 ` Mike Frysinger
@ 2012-02-29 10:25 ` Amit Virdi
2012-03-06 12:06 ` Amit Virdi
0 siblings, 1 reply; 20+ messages in thread
From: Amit Virdi @ 2012-02-29 10:25 UTC (permalink / raw)
To: u-boot
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.
Thanks
Amit Virdi
^ permalink raw reply [flat|nested] 20+ messages in thread
* [U-Boot] [PATCH] USB:host: Attribute packed removed from usb structures
2012-02-29 10:25 ` Amit Virdi
@ 2012-03-06 12:06 ` Amit Virdi
2012-03-06 12:51 ` Marek Vasut
2012-03-06 16:11 ` Mike Frysinger
0 siblings, 2 replies; 20+ messages in thread
From: Amit Virdi @ 2012-03-06 12:06 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [U-Boot] [PATCH] USB:host: Attribute packed removed from usb structures
2012-03-06 12:06 ` Amit Virdi
@ 2012-03-06 12:51 ` Marek Vasut
2012-03-07 8:30 ` Amit Virdi
2012-03-06 16:11 ` Mike Frysinger
1 sibling, 1 reply; 20+ messages in thread
From: Marek Vasut @ 2012-03-06 12:51 UTC (permalink / raw)
To: u-boot
Dear 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.
Can you check with different toolchain please?
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 20+ messages in thread
* [U-Boot] [PATCH] USB:host: Attribute packed removed from usb structures
2012-03-06 12:06 ` Amit Virdi
2012-03-06 12:51 ` Marek Vasut
@ 2012-03-06 16:11 ` Mike Frysinger
2012-03-07 8:23 ` Amit Virdi
1 sibling, 1 reply; 20+ messages in thread
From: Mike Frysinger @ 2012-03-06 16:11 UTC (permalink / raw)
To: u-boot
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>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [U-Boot] [PATCH] USB:host: Attribute packed removed from usb structures
2012-03-06 16:11 ` Mike Frysinger
@ 2012-03-07 8:23 ` Amit Virdi
2012-03-07 13:33 ` Mike Frysinger
0 siblings, 1 reply; 20+ messages in thread
From: Amit Virdi @ 2012-03-07 8:23 UTC (permalink / raw)
To: u-boot
Mike,
>>
>> 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.
If you guys agree, I can float the patch but how can we distribute the
responsibility for testing on other architectures?
Thanks
Amit Virdi
^ permalink raw reply [flat|nested] 20+ messages in thread
* [U-Boot] [PATCH] USB:host: Attribute packed removed from usb structures
2012-03-06 12:51 ` Marek Vasut
@ 2012-03-07 8:30 ` Amit Virdi
2012-03-07 11:38 ` Marek Vasut
0 siblings, 1 reply; 20+ messages in thread
From: Amit Virdi @ 2012-03-07 8:30 UTC (permalink / raw)
To: u-boot
Dear Marek,
>> 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.
>
> Can you check with different toolchain please?
>
Do you mean different toolchains for ARM? I can surely check if you can
help me. I have used ST's internal distribution till date and I guess I
need to download and install other ARM toolchains in order to verify
this issue.
Or, we can work this way that my source code is compiled by someone
already installed/using other tool chains. I can test it on my board and
report the results?
Thanks
Amit Virdi
^ permalink raw reply [flat|nested] 20+ messages in thread
* [U-Boot] [PATCH] USB:host: Attribute packed removed from usb structures
2012-03-07 8:30 ` Amit Virdi
@ 2012-03-07 11:38 ` Marek Vasut
2012-03-07 12:12 ` Amit Virdi
0 siblings, 1 reply; 20+ messages in thread
From: Marek Vasut @ 2012-03-07 11:38 UTC (permalink / raw)
To: u-boot
Dear Amit Virdi,
> Dear Marek,
>
> >> 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.
> >
> > Can you check with different toolchain please?
>
> Do you mean different toolchains for ARM? I can surely check if you can
> help me. I have used ST's internal distribution till date and I guess I
> need to download and install other ARM toolchains in order to verify
> this issue.
Sure, you can download the ELDK installer for ELDK 5.1 :-)
>
> Or, we can work this way that my source code is compiled by someone
> already installed/using other tool chains. I can test it on my board and
> report the results?
Yes please. I'll most likely apply your other patchset, but let's keep an eye on
this patch, it might come handy later if the issue reappears.
>
> Thanks
> Amit Virdi
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 20+ messages in thread
* [U-Boot] [PATCH] USB:host: Attribute packed removed from usb structures
2012-03-07 11:38 ` Marek Vasut
@ 2012-03-07 12:12 ` Amit Virdi
0 siblings, 0 replies; 20+ messages in thread
From: Amit Virdi @ 2012-03-07 12:12 UTC (permalink / raw)
To: u-boot
Dear Marek,
On 3/7/2012 5:08 PM, Marek Vasut wrote:
> Dear Amit Virdi,
>
>> Dear Marek,
>>
>>>> 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.
>>>
>>> Can you check with different toolchain please?
>>
>> Do you mean different toolchains for ARM? I can surely check if you can
>> help me. I have used ST's internal distribution till date and I guess I
>> need to download and install other ARM toolchains in order to verify
>> this issue.
>
> Sure, you can download the ELDK installer for ELDK 5.1 :-)
Ok. I keep this in my "To do" list. As soon as I get some spare time,
I'll install the toolchain and test the build on my SPEAr320 eval board.
>>
>> Or, we can work this way that my source code is compiled by someone
>> already installed/using other tool chains. I can test it on my board and
>> report the results?
>
> Yes please. I'll most likely apply your other patchset, but let's keep an eye on
> this patch, it might come handy later if the issue reappears.
>
Ok, thanks.
Regards
Amit Virdi
^ permalink raw reply [flat|nested] 20+ messages in thread
* [U-Boot] [PATCH] USB:host: Attribute packed removed from usb structures
2012-03-07 8:23 ` Amit Virdi
@ 2012-03-07 13:33 ` Mike Frysinger
2012-03-07 13:49 ` Amit Virdi
0 siblings, 1 reply; 20+ messages in thread
From: Mike Frysinger @ 2012-03-07 13:33 UTC (permalink / raw)
To: u-boot
On Wednesday 07 March 2012 03:23:42 Amit Virdi wrote:
> >> 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.
>
> If you guys agree, I can float the patch but how can we distribute the
> responsibility for testing on other architectures?
if it works on your platform, and you compile it test it for others, and we
agree it looks correct, and no one else wants to runtime test it to verify, i
think it's fine to merge. if there's a problem, someone will notice and let us
know ;).
-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/20120307/1dc19ae2/attachment.pgp>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [U-Boot] [PATCH] USB:host: Attribute packed removed from usb structures
2012-03-07 13:33 ` Mike Frysinger
@ 2012-03-07 13:49 ` Amit Virdi
0 siblings, 0 replies; 20+ messages in thread
From: Amit Virdi @ 2012-03-07 13:49 UTC (permalink / raw)
To: u-boot
On Wed, Mar 7, 2012 at 7:03 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Wednesday 07 March 2012 03:23:42 Amit Virdi wrote:
>> >> 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.
>>
>> If you guys agree, I can float the patch but how can we distribute the
>> responsibility for testing on other architectures?
>
> if it works on your platform, and you compile it test it for others, and we
> agree it looks correct, and no one else wants to runtime test it to verify, i
> think it's fine to merge. ?if there's a problem, someone will notice and let us
> know ;).
Ok Mike, agreed. Once I test it with ELDK 5.1, i'll inform you guys.
Thanks
Amit Virdi
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2012-03-07 13:49 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-02-24 11:58 [U-Boot] [PATCH] USB:host: Attribute packed removed from usb structures Amit Virdi
2012-02-24 19:18 ` Mike Frysinger
2012-02-27 10:02 ` Amit Virdi
2012-02-27 18:25 ` Mike Frysinger
2012-02-25 10:12 ` Albert ARIBAUD
2012-02-27 7:16 ` Vipin Kumar
2012-02-27 13:14 ` Marek Vasut
2012-02-27 18:26 ` Mike Frysinger
2012-02-27 20:53 ` Marek Vasut
2012-02-28 0:56 ` Mike Frysinger
2012-02-29 10:25 ` Amit Virdi
2012-03-06 12:06 ` Amit Virdi
2012-03-06 12:51 ` Marek Vasut
2012-03-07 8:30 ` Amit Virdi
2012-03-07 11:38 ` Marek Vasut
2012-03-07 12:12 ` Amit Virdi
2012-03-06 16:11 ` Mike Frysinger
2012-03-07 8:23 ` Amit Virdi
2012-03-07 13:33 ` Mike Frysinger
2012-03-07 13:49 ` Amit Virdi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.