USB:host: Attribute packed removed from usb structures

PATCH New Amit Virdi 7 participants 20 messages Feb 24, 2012 – Mar 7, 2012

Subsystems (2)

USB USB ehci driver

lore

Share & tools

lore

Amit Virdi

author

2012-02-24 11:58 UTC #0 lore raw

PATCH 1/1 USB:host: Attribute packed removed from usb structures

From: Vipin Kumar <[email protected]>

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 <[email protected]>

Signed-off-by Amit Virdi <[email protected]>

---
 drivers/usb/host/ehci.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/usb/host/ehci.h +2 -2

index 3d0ad0c..df9f055 100644
@@ -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 */

Signature · 1 line

1.7.2.2

Mike Frysinger

2012-02-24 19:18 UTC #1 lore raw

PATCH 1/1 USB:host: Attribute packed removed from usb structures

Quoted 2 lines · 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>

Amit Virdi

author

2012-02-27 10:02 UTC #2 lore raw

PATCH 1/1 USB:host: Attribute packed removed from usb structures

Hello Mike,

Quoted 5 lines · Mike Frysinger

Quoted 2 lines · 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

Mike Frysinger

2012-02-27 18:25 UTC #3 lore raw

PATCH 1/1 USB:host: Attribute packed removed from usb structures

Quoted 8 lines · On Monday 27 February 2012 05:02:14 Amit Virdi wrote:

Quoted 5 lines · Mike Frysinger
Quoted 2 lines · 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>

Albert ARIBAUD

2012-02-25 10:12 UTC #4 lore raw

PATCH 1/1 USB:host: Attribute packed removed from usb structures

In reply to Amit Virdi

Hi Amit,

Le 24/02/2012 12:58, Amit Virdi a ?crit :

Quoted 5 lines

From: Vipin Kumar<[email protected]>
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,

Signature · 1 line

Albert.

Vipin Kumar

2012-02-27 07:16 UTC #5 lore raw

PATCH 1/1 USB:host: Attribute packed removed from usb structures

Quoted 2 lines · Albert ARIBAUD

Hi Amit,

Hello Albert,

Quoted 11 lines

Le 24/02/2012 12:58, Amit Virdi a ?crit :
Quoted 5 lines
From: Vipin Kumar<[email protected]>
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

Quoted 1 line

Amicalement,

Marek Vasut

2012-02-27 13:14 UTC #6 lore raw

PATCH 1/1 USB:host: Attribute packed removed from usb structures

Quoted 20 lines · Albert ARIBAUD

Quoted 1 line
Hi Amit,
Hello Albert,
Quoted 10 lines
Le 24/02/2012 12:58, Amit Virdi a ?crit :
Quoted 5 lines
From: Vipin Kumar<[email protected]>
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

Quoted 7 lines

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?

Mike Frysinger

2012-02-27 18:26 UTC #7 lore raw

PATCH 1/1 USB:host: Attribute packed removed from usb structures

Quoted 14 lines · On Monday 27 February 2012 08:14:26 Marek Vasut wrote:

Quoted 14 lines · Albert ARIBAUD
Quoted 8 lines
Le 24/02/2012 12:58, Amit Virdi a ?crit :
Quoted 3 lines
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

Quoted 8 lines

Quoted 6 lines
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>

Marek Vasut

2012-02-27 20:53 UTC #8 lore raw

PATCH 1/1 USB:host: Attribute packed removed from usb structures

Quoted 22 lines · On Monday 27 February 2012 08:14:26 Marek Vasut wrote:

Quoted 21 lines
Quoted 20 lines · Albert ARIBAUD
Quoted 8 lines
Le 24/02/2012 12:58, Amit Virdi a ?crit :
Quoted 3 lines
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

Quoted 5 lines

Quoted 1 line
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 ?

Mike Frysinger

2012-02-28 00:56 UTC #9 lore raw

PATCH 1/1 USB:host: Attribute packed removed from usb structures

Quoted 22 lines · On Monday 27 February 2012 15:53:29 Marek Vasut wrote:

Quoted 22 lines · On Monday 27 February 2012 08:14:26 Marek Vasut wrote:
Quoted 21 lines
Quoted 20 lines · Albert ARIBAUD
Quoted 8 lines
Le 24/02/2012 12:58, Amit Virdi a ?crit :
Quoted 3 lines
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

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
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>

Amit Virdi

author

2012-02-29 10:25 UTC #10 lore raw

PATCH 1/1 USB:host: Attribute packed removed from usb structures

Hello Mike,

Quoted 31 lines · Mike Frysinger

Quoted 22 lines · On Monday 27 February 2012 15:53:29 Marek Vasut wrote:
Quoted 22 lines · On Monday 27 February 2012 08:14:26 Marek Vasut wrote:
Quoted 21 lines
Quoted 20 lines · Albert ARIBAUD
Quoted 8 lines
Le 24/02/2012 12:58, Amit Virdi a ?crit :
Quoted 3 lines
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
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
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 ?

Quoted 8 lines

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

Amit Virdi

author

2012-03-06 12:06 UTC #11 lore raw

PATCH 1/1 USB:host: Attribute packed removed from usb structures

Hello Mike and Marek,

Quoted 42 lines · Amit Virdi

Hello Mike,
Quoted 39 lines · Mike Frysinger
Quoted 22 lines · On Monday 27 February 2012 15:53:29 Marek Vasut wrote:
Quoted 22 lines · On Monday 27 February 2012 08:14:26 Marek Vasut wrote:
Quoted 21 lines
Quoted 20 lines · Albert ARIBAUD
Quoted 8 lines
Le 24/02/2012 12:58, Amit Virdi a ?crit :
Quoted 3 lines
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
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
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.

Quoted 7 lines

Quoted 4 lines
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

Marek Vasut

2012-03-06 12:51 UTC #12 lore raw

PATCH 1/1 USB:host: Attribute packed removed from usb structures

Dear Amit Virdi,

Quoted 57 lines

Hello Mike and Marek,
Quoted 42 lines · Amit Virdi
Hello Mike,
Quoted 39 lines · Mike Frysinger
Quoted 22 lines · On Monday 27 February 2012 15:53:29 Marek Vasut wrote:
Quoted 22 lines · On Monday 27 February 2012 08:14:26 Marek Vasut wrote:
Quoted 21 lines
Quoted 20 lines · Albert ARIBAUD
Quoted 8 lines
Le 24/02/2012 12:58, Amit Virdi a ?crit :
Quoted 3 lines
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
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
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.
Quoted 6 lines
Quoted 4 lines
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.

Quoted 8 lines

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

Amit Virdi

author

2012-03-07 08:30 UTC #13 lore raw

PATCH 1/1 USB:host: Attribute packed removed from usb structures

Dear Marek,

Quoted 8 lines

Quoted 8 lines
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.

Quoted 8 lines

Quoted 5 lines
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

Marek Vasut

2012-03-07 11:38 UTC #14 lore raw

PATCH 1/1 USB:host: Attribute packed removed from usb structures

Dear Amit Virdi,

Quoted 17 lines

Dear Marek,
Quoted 15 lines
Quoted 14 lines
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.

Quoted 6 lines

Quoted 1 line
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 :-)

Quoted 4 lines

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.

Quoted 3 lines

Thanks
Amit Virdi

Best regards,
Marek Vasut

Amit Virdi

author

2012-03-07 12:12 UTC #15 lore raw

PATCH 1/1 USB:host: Attribute packed removed from usb structures

Dear Marek,

Quoted 19 lines · Marek Vasut

Dear Amit Virdi,
Quoted 17 lines
Dear Marek,
Quoted 15 lines
Quoted 14 lines
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.

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
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.

Quoted 8 lines

Quoted 4 lines
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

Mike Frysinger

2012-03-06 16:11 UTC #16 lore raw

PATCH 1/1 USB:host: Attribute packed removed from usb structures

In reply to Amit Virdi

Quoted 53 lines · On Tuesday 06 March 2012 07:06:22 Amit Virdi wrote:

Quoted 40 lines · Amit Virdi
Quoted 39 lines · Mike Frysinger
Quoted 22 lines · On Monday 27 February 2012 15:53:29 Marek Vasut wrote:
Quoted 22 lines · On Monday 27 February 2012 08:14:26 Marek Vasut wrote:
Quoted 21 lines
Quoted 20 lines · Albert ARIBAUD
Quoted 8 lines
Le 24/02/2012 12:58, Amit Virdi a ?crit :
Quoted 3 lines
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
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
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.
Quoted 6 lines
Quoted 4 lines
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.

Quoted 8 lines

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>

Amit Virdi

author

2012-03-07 08:23 UTC #17 lore raw

PATCH 1/1 USB:host: Attribute packed removed from usb structures

Mike,

Quoted 13 lines

Quoted 13 lines
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.

Quoted 6 lines

Quoted 1 line
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

Mike Frysinger

2012-03-07 13:33 UTC #18 lore raw

PATCH 1/1 USB:host: Attribute packed removed from usb structures

Quoted 15 lines · On Wednesday 07 March 2012 03:23:42 Amit Virdi wrote:

Quoted 15 lines
Quoted 14 lines
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.

Quoted 7 lines

Quoted 4 lines
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>

Amit Virdi

2012-03-07 13:49 UTC #19 lore raw

PATCH 1/1 USB:host: Attribute packed removed from usb structures

Quoted 21 lines · Mike Frysinger

Quoted 20 lines · On Wednesday 07 March 2012 03:23:42 Amit Virdi wrote:
Quoted 19 lines
Quoted 8 lines
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.
Quoted 6 lines
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.

Quoted 7 lines

Quoted 2 lines
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