[U-Boot] packed attribute problem

[U-Boot] packed attribute problem

[Vipin Kumar]

Hi All,

I encountered a problem something like

struct xyz {
	int	x;
	int	y;
	int	z[CONST];
} __attribute__ ((packed));

struct xyz *abc;
u32 * status_reg = (u32 *)&abc->z[0];

writel(status, status_reg);

This writel results in writing byte by byte on the address pointed to by status_reg.
This problem is visible with both gcc version 4.4.1 as well as 4.5.0

Incidently, the same code works well with 4.2.4

The problem is visible in the usb host driver which uses the packed structures for 
accessing  device registers.

Regards
Vipin

[U-Boot] packed attribute problem

[Wolfgang Denk]

Dear Vipin Kumar,

In message <4CA9A095.9000900@st.com> you wrote:
> 
> I encountered a problem something like
> 
> struct xyz {
> 	int	x;
> 	int	y;
> 	int	z[CONST];
> } __attribute__ ((packed));
> 
> struct xyz *abc;
> u32 * status_reg = (u32 *)&abc->z[0];
> 
> writel(status, status_reg);
> 
> This writel results in writing byte by byte on the address pointed to by status_reg.
> This problem is visible with both gcc version 4.4.1 as well as 4.5.0

I bet this is on some ARM system?

> Incidently, the same code works well with 4.2.4

...which surprises me. I thought this has always been an ARM
"feature".

> The problem is visible in the usb host driver which uses the packed structures for 
> accessing  device registers.

Hm... Why do these structs have any "__attribute__ ((packed))" at all?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If I had to live my life again,  I'd  make  the  same  mistakes, only
sooner.                                          -- Tallulah Bankhead

[U-Boot] packed attribute problem

[Vipin Kumar]

On 10/4/2010 3:47 PM, Wolfgang Denk wrote:
> Dear Vipin Kumar,
> 

Dear Wolfgang

> In message <4CA9A095.9000900@st.com> you wrote:
>>
>> I encountered a problem something like
>>
>> struct xyz {
>> 	int	x;
>> 	int	y;
>> 	int	z[CONST];
>> } __attribute__ ((packed));
>>
>> struct xyz *abc;
>> u32 * status_reg = (u32 *)&abc->z[0];
>>
>> writel(status, status_reg);
>>
>> This writel results in writing byte by byte on the address pointed to by status_reg.
>> This problem is visible with both gcc version 4.4.1 as well as 4.5.0
> 
> I bet this is on some ARM system?
> 

Yes, it is on an ARM system (CortexA9). But I still feel that since I am creating 
a new u32 * status_reg, the code should not use any intelligence and use the pointer 
only to produce an str instruction in the form
	str r0, [r1]

But it retains the packed property of the structure even with a new u32 ponter 
typecasted to u32 *
	u32 * status_reg = (u32 *)xyz->x;

A writel to status_reg results in byte by byte writing

>> Incidently, the same code works well with 4.2.4
> 
> ...which surprises me. I thought this has always been an ARM
> "feature".
> 
>> The problem is visible in the usb host driver which uses the packed structures for 
>> accessing  device registers.
> 
> Hm... Why do these structs have any "__attribute__ ((packed))" at all?
> 

Even I could not understand that very well

Regards
Vipin
> Best regards,
> 
> Wolfgang Denk
> 

[U-Boot] packed attribute problem

[Wolfgang Denk]

Dear Vipin Kumar,

In message <4CA9ACAA.2020001@st.com> you wrote:
>
> >> This writel results in writing byte by byte on the address pointed to by status_reg.
> >> This problem is visible with both gcc version 4.4.1 as well as 4.5.0
> > 
> > I bet this is on some ARM system?
> 
> Yes, it is on an ARM system (CortexA9). But I still feel that since I am creating 
> a new u32 * status_reg, the code should not use any intelligence and use the pointer 
> only to produce an str instruction in the form
> 	str r0, [r1]
> 
> But it retains the packed property of the structure even with a new u32 ponter 
> typecasted to u32 *
> 	u32 * status_reg = (u32 *)xyz->x;
> 
> A writel to status_reg results in byte by byte writing

I agree with you. I always considered such behaviour of the ARM C
compiler a bug, and still do.  However, people with better knowledge
of the ARm architecture than me might be able to explain why the
responsible PTB consider this to be a good and necessary "feature" of
th compiler.

> > Hm... Why do these structs have any "__attribute__ ((packed))" at all?
> 
> Even I could not understand that very well

Eventually alignment of these structs cannot be guaranteed?



Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
PLEASE NOTE: Some Quantum Physics Theories Suggest That When the Con-
sumer Is Not Directly Observing This Product, It May Cease  to  Exist
or Will Exist Only in a Vague and Undetermined State.

[U-Boot] packed attribute problem

[Reinhard Meyer]

Dear Wolfgang Denk, Vipin Kumar,
> 
> In message <4CA9ACAA.2020001@st.com> you wrote:
>>>> This writel results in writing byte by byte on the address pointed to by status_reg.
>>>> This problem is visible with both gcc version 4.4.1 as well as 4.5.0
>>> I bet this is on some ARM system?
>> Yes, it is on an ARM system (CortexA9). But I still feel that since I am creating 
>> a new u32 * status_reg, the code should not use any intelligence and use the pointer 
>> only to produce an str instruction in the form
>> 	str r0, [r1]
>>
>> But it retains the packed property of the structure even with a new u32 ponter 
>> typecasted to u32 *
>> 	u32 * status_reg = (u32 *)xyz->x;
>>
>> A writel to status_reg results in byte by byte writing

How do you know that? Disassembly? Bus snooping?

> 
> I agree with you. I always considered such behaviour of the ARM C
> compiler a bug, and still do.  However, people with better knowledge
> of the ARm architecture than me might be able to explain why the
> responsible PTB consider this to be a good and necessary "feature" of
> th compiler.

Maybe because writel(v,a) is ultimately defined as

(*(volatile unsigned int *)(a) = (v))

and the compiler has to assume the worst, i.e. that (a) is not word
aligned? Maybe there is a new compiler switch to turn that
"pessimisation" off?

On the other hand, if what you say were true, all code for AT91 that
uses writel() to access 32 bit only peripheral registers would not
work with those newer toolchains (still 4.2.4 here).

Best Regards,
Reinhard

[U-Boot] packed attribute problem

[Vipin Kumar]

On 10/4/2010 4:26 PM, Reinhard Meyer wrote:
> Dear Wolfgang Denk, Vipin Kumar,
>>
>> In message <4CA9ACAA.2020001@st.com> you wrote:
>>>>> This writel results in writing byte by byte on the address pointed to by status_reg.
>>>>> This problem is visible with both gcc version 4.4.1 as well as 4.5.0
>>>> I bet this is on some ARM system?
>>> Yes, it is on an ARM system (CortexA9). But I still feel that since I am creating 
>>> a new u32 * status_reg, the code should not use any intelligence and use the pointer 
>>> only to produce an str instruction in the form
>>> 	str r0, [r1]
>>>
>>> But it retains the packed property of the structure even with a new u32 ponter 
>>> typecasted to u32 *
>>> 	u32 * status_reg = (u32 *)xyz->x;
>>>
>>> A writel to status_reg results in byte by byte writing
> 
> How do you know that? Disassembly? Bus snooping?
> 

Disassembly

>>
>> I agree with you. I always considered such behaviour of the ARM C
>> compiler a bug, and still do.  However, people with better knowledge
>> of the ARm architecture than me might be able to explain why the
>> responsible PTB consider this to be a good and necessary "feature" of
>> th compiler.
> 
> Maybe because writel(v,a) is ultimately defined as
> 
> (*(volatile unsigned int *)(a) = (v))
> 
> and the compiler has to assume the worst, i.e. that (a) is not word
> aligned? Maybe there is a new compiler switch to turn that
> "pessimisation" off?
> 

I agree with you on this. But if I create a new u32 pointer and point to 
an element with in the packed structure, then what should be the behavior ?

> On the other hand, if what you say were true, all code for AT91 that
> uses writel() to access 32 bit only peripheral registers would not
> work with those newer toolchains (still 4.2.4 here).
> 

Yes, if the at91 code is using packed attribute for device structures

Regards
Vipin

[U-Boot] packed attribute problem

[Reinhard Meyer]

Dear Vipin Kumar,
>>>> A writel to status_reg results in byte by byte writing
>> How do you know that? Disassembly? Bus snooping?
>>
> 
> Disassembly
> 
>>> I agree with you. I always considered such behaviour of the ARM C
>>> compiler a bug, and still do.  However, people with better knowledge
>>> of the ARm architecture than me might be able to explain why the
>>> responsible PTB consider this to be a good and necessary "feature" of
>>> th compiler.
>> Maybe because writel(v,a) is ultimately defined as
>>
>> (*(volatile unsigned int *)(a) = (v))
>>
>> and the compiler has to assume the worst, i.e. that (a) is not word
>> aligned? Maybe there is a new compiler switch to turn that
>> "pessimisation" off?
>>
> 
> I agree with you on this. But if I create a new u32 pointer and point to 
> an element with in the packed structure, then what should be the behavior ?

Do you imply that the code is really different when the pointer gets
its value by assigning it NOT to a packed entity? Hard to believe.

My assumptions:
1. it has to do NOTHING with packed attributes, once the pointer is in a
new variable, that "packed" information should be lost. Just try
int *ptr = (int *) 0x10000;
a)   *ptr = 5;
b)   writel (5, ptr);

do a) and b) produce different code?

2. the culprit on ARM is that unaligned accesses do not raise any signal
but silently IGNORE the "unused" adress lines which leads to very
undesirable effects that are hard to find.

*((int *)0x10000) = 5 is the same as *((int *)0x10003) = 5 !
Both write 5 to the word at adress 0x10000 !

So, MAYBE, in newer toolchains it was decided to circumvent that problem
by always assuming unaligned pointers unless clearly instructed otherwise.

> 
>> On the other hand, if what you say were true, all code for AT91 that
>> uses writel() to access 32 bit only peripheral registers would not
>> work with those newer toolchains (still 4.2.4 here).
>>
> 
> Yes, if the at91 code is using packed attribute for device structures


Nope. It does not have to do with packed. It would have to do with the fact
that I/O registers cannot be safely written byte-wise.

Reinhard

[U-Boot] packed attribute problem

[Wolfgang Denk]

Dear Reinhard Meyer,

In message <4CA9BE94.6000401@emk-elektronik.de> you wrote:
>
> Do you imply that the code is really different when the pointer gets
> its value by assigning it NOT to a packed entity? Hard to believe.

This is a special "feature" of GCC on ARM.

-> cat foo.c
#define writel(v,a)              (*(volatile unsigned int *)(a) = (v))

struct p {
        int n;
} __attribute__ ((packed));

struct q {
        int n;
};

void foo()
{
        struct p *pp = (struct p *)0x1000;

        pp->n = 5;
}

void bar()
{
        struct q *qq = (struct q *)0x1000;

        qq->n = 5;
}
-> arm-linux-gcc -O -S foo.c
-> cat foo.s
        .file   "foo.c"
        .text
        .align  2
        .global foo
        .type   foo, %function
foo:
        @ Function supports interworking.
        @ args = 0, pretend = 0, frame = 0
        @ frame_needed = 0, uses_anonymous_args = 0
        @ link register save eliminated.
        @ lr needed for prologue
        mov     r3, #4096
        mov     r2, #0
        orr     r1, r2, #5
        strb    r1, [r3, #0]
        strb    r2, [r3, #1]
        strb    r2, [r3, #2]
        strb    r2, [r3, #3]
        bx      lr
        .size   foo, .-foo
        .align  2
        .global bar
        .type   bar, %function
bar:
        @ Function supports interworking.
        @ args = 0, pretend = 0, frame = 0
        @ frame_needed = 0, uses_anonymous_args = 0
        @ link register save eliminated.
        @ lr needed for prologue
        mov     r2, #5
        mov     r3, #4096
        str     r2, [r3, #0]
        bx      lr
        .size   bar, .-bar
        .ident  "GCC: (GNU) 4.2.2"


Note that this is with GCC 4.2.2. Even GCC 4.0.0 behaves the same, so
this is *not* an issue with very recent tool chains.


> 2. the culprit on ARM is that unaligned accesses do not raise any signal
> but silently IGNORE the "unused" adress lines which leads to very
> undesirable effects that are hard to find.

Well, the compiler turning a single 32 bit access silently into 4 x 8
bit accesses can also lead to very undesirable effects that are hard
to find, especially when accessing hardware that performs
auto-incrementing or any kind of (IRQ or similar) ACK for each
access.

> *((int *)0x10000) = 5 is the same as *((int *)0x10003) = 5 !
> Both write 5 to the word at adress 0x10000 !
> 
> So, MAYBE, in newer toolchains it was decided to circumvent that problem
> by always assuming unaligned pointers unless clearly instructed otherwise.

No, AFAICT this behaviour has not changed in the last few months / years.

> Nope. It does not have to do with packed. It would have to do with the fact
> that I/O registers cannot be safely written byte-wise.

Yes, it has to do with pcked. See above.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If a person (a) is poorly, (b) receives treatment  intended  to  make
him  better, and (c) gets better, then no power of reasoning known to
medical science can convince him  that  it  may  not  have  been  the
treatment that restored his health.
- Sir Peter Medawar, The Art of the Soluble

[U-Boot] packed attribute problem

[Reinhard Meyer]

Dear Wolfgang Denk,

> Dear Reinhard Meyer,
> 
> In message <4CA9BE94.6000401@emk-elektronik.de> you wrote:
>> Do you imply that the code is really different when the pointer gets
>> its value by assigning it NOT to a packed entity? Hard to believe.
> 
> This is a special "feature" of GCC on ARM.
> 
> -> cat foo.c
> #define writel(v,a)              (*(volatile unsigned int *)(a) = (v))
> 
> struct p {
>         int n;
> } __attribute__ ((packed));
> 
> struct q {
>         int n;
> };
> 
> void foo()
> {
>         struct p *pp = (struct p *)0x1000;
> 
>         pp->n = 5;
> }
> 
> void bar()
> {
>         struct q *qq = (struct q *)0x1000;
> 
>         qq->n = 5;
> }
> -> arm-linux-gcc -O -S foo.c
> -> cat foo.s
>         .file   "foo.c"
>         .text
>         .align  2
>         .global foo
>         .type   foo, %function
> foo:
>         @ Function supports interworking.
>         @ args = 0, pretend = 0, frame = 0
>         @ frame_needed = 0, uses_anonymous_args = 0
>         @ link register save eliminated.
>         @ lr needed for prologue
>         mov     r3, #4096
>         mov     r2, #0
>         orr     r1, r2, #5
>         strb    r1, [r3, #0]
>         strb    r2, [r3, #1]
>         strb    r2, [r3, #2]
>         strb    r2, [r3, #3]
>         bx      lr
>         .size   foo, .-foo
>         .align  2
>         .global bar
>         .type   bar, %function

In a non-packed struct an int will never be unaligned
(unless you use an unaligned pointer to the whole struct)

In a packed struct an int might be unaligned, so it
_might_ make sense for the compiler to handle that
differently on ARM. Assume you overlay (bad idea anyway)
a packed structure over some communication data stream
thats is byte oriented. On most architectures that would
work (besides obvious endianess issues) but on ARM it would
(without raising an exception) malfunction.
[remember the "display_buffer issue"]

> bar:
>         @ Function supports interworking.
>         @ args = 0, pretend = 0, frame = 0
>         @ frame_needed = 0, uses_anonymous_args = 0
>         @ link register save eliminated.
>         @ lr needed for prologue
>         mov     r2, #5
>         mov     r3, #4096
>         str     r2, [r3, #0]
>         bx      lr
>         .size   bar, .-bar
>         .ident  "GCC: (GNU) 4.2.2"
> 
> 
> Note that this is with GCC 4.2.2. Even GCC 4.0.0 behaves the same, so
> this is *not* an issue with very recent tool chains.

OK, for directly adressing elements inside a packed struct;
but the original post said:

"struct xyz {
	int	x;
	int	y;
	int	z[CONST];
} __attribute__ ((packed));

struct xyz *abc;
u32 * status_reg = (u32 *)&abc->z[0];

writel(status, status_reg);"

So the "status_reg" pointer is in a completely unrelated (to the packed struct)
"u32 *" and still the access is done like it was packed. If the
compiler silently drags that attribute along into the "u32 *"
THAT is really sick!

Reinhard

[U-Boot] packed attribute problem

[Balau]

Dear Wolfgang Denk and Reinhard Meyer,

The compiler (4.4.1) generates the expected 32bit store instruction when using:

struct p {
        int n;
} __attribute__ ((packed, aligned(4)));

In case of hardware registers, I have yet to see a case where this is not true.

Regards,
Francesco

On Mon, Oct 4, 2010 at 2:43 PM, Reinhard Meyer <u-boot@emk-elektronik.de> wrote:
> Dear Wolfgang Denk,
>
>> Dear Reinhard Meyer,
>>
>> In message <4CA9BE94.6000401@emk-elektronik.de> you wrote:
>>> Do you imply that the code is really different when the pointer gets
>>> its value by assigning it NOT to a packed entity? Hard to believe.
>>
>> This is a special "feature" of GCC on ARM.
>>
>> -> cat foo.c
>> #define writel(v,a) ? ? ? ? ? ? ?(*(volatile unsigned int *)(a) = (v))
>>
>> struct p {
>> ? ? ? ? int n;
>> } __attribute__ ((packed));
>>
>> struct q {
>> ? ? ? ? int n;
>> };
>>
>> void foo()
>> {
>> ? ? ? ? struct p *pp = (struct p *)0x1000;
>>
>> ? ? ? ? pp->n = 5;
>> }
>>
>> void bar()
>> {
>> ? ? ? ? struct q *qq = (struct q *)0x1000;
>>
>> ? ? ? ? qq->n = 5;
>> }
>> -> arm-linux-gcc -O -S foo.c
>> -> cat foo.s
>> ? ? ? ? .file ? "foo.c"
>> ? ? ? ? .text
>> ? ? ? ? .align ?2
>> ? ? ? ? .global foo
>> ? ? ? ? .type ? foo, %function
>> foo:
>> ? ? ? ? @ Function supports interworking.
>> ? ? ? ? @ args = 0, pretend = 0, frame = 0
>> ? ? ? ? @ frame_needed = 0, uses_anonymous_args = 0
>> ? ? ? ? @ link register save eliminated.
>> ? ? ? ? @ lr needed for prologue
>> ? ? ? ? mov ? ? r3, #4096
>> ? ? ? ? mov ? ? r2, #0
>> ? ? ? ? orr ? ? r1, r2, #5
>> ? ? ? ? strb ? ?r1, [r3, #0]
>> ? ? ? ? strb ? ?r2, [r3, #1]
>> ? ? ? ? strb ? ?r2, [r3, #2]
>> ? ? ? ? strb ? ?r2, [r3, #3]
>> ? ? ? ? bx ? ? ?lr
>> ? ? ? ? .size ? foo, .-foo
>> ? ? ? ? .align ?2
>> ? ? ? ? .global bar
>> ? ? ? ? .type ? bar, %function
>
> In a non-packed struct an int will never be unaligned
> (unless you use an unaligned pointer to the whole struct)
>
> In a packed struct an int might be unaligned, so it
> _might_ make sense for the compiler to handle that
> differently on ARM. Assume you overlay (bad idea anyway)
> a packed structure over some communication data stream
> thats is byte oriented. On most architectures that would
> work (besides obvious endianess issues) but on ARM it would
> (without raising an exception) malfunction.
> [remember the "display_buffer issue"]
>
>> bar:
>> ? ? ? ? @ Function supports interworking.
>> ? ? ? ? @ args = 0, pretend = 0, frame = 0
>> ? ? ? ? @ frame_needed = 0, uses_anonymous_args = 0
>> ? ? ? ? @ link register save eliminated.
>> ? ? ? ? @ lr needed for prologue
>> ? ? ? ? mov ? ? r2, #5
>> ? ? ? ? mov ? ? r3, #4096
>> ? ? ? ? str ? ? r2, [r3, #0]
>> ? ? ? ? bx ? ? ?lr
>> ? ? ? ? .size ? bar, .-bar
>> ? ? ? ? .ident ?"GCC: (GNU) 4.2.2"
>>
>>
>> Note that this is with GCC 4.2.2. Even GCC 4.0.0 behaves the same, so
>> this is *not* an issue with very recent tool chains.
>
> OK, for directly adressing elements inside a packed struct;
> but the original post said:
>
> "struct xyz {
> ? ? ? ?int ? ? x;
> ? ? ? ?int ? ? y;
> ? ? ? ?int ? ? z[CONST];
> } __attribute__ ((packed));
>
> struct xyz *abc;
> u32 * status_reg = (u32 *)&abc->z[0];
>
> writel(status, status_reg);"
>
> So the "status_reg" pointer is in a completely unrelated (to the packed struct)
> "u32 *" and still the access is done like it was packed. If the
> compiler silently drags that attribute along into the "u32 *"
> THAT is really sick!
>
> Reinhard
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>

[U-Boot] packed attribute problem

[Wolfgang Denk]

Dear Balau,

In message <AANLkTik6FcN-n+4iG0VV67T7rvSaTB16=27QyA8D6=7z@mail.gmail.com> you wrote:
> 
> The compiler (4.4.1) generates the expected 32bit store instruction when using:
> 
> struct p {
>         int n;
> } __attribute__ ((packed, aligned(4)));

Confirmed.  Thanks for pointing this out.

> In case of hardware registers, I have yet to see a case where this is not true.

Indeed, sounds as if this was a solution in case the "packed" should
really be needed - which I seriously doubt.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Objects in mirror are closer than they appear.

[U-Boot] DDR SPD table

[sywang]

Do you have the general materials about DDR programming in u-boot?  

Thanks!
Shuyou

[U-Boot] DDR SPD table

[Mike Frysinger]

On Monday, October 04, 2010 09:02:26 sywang wrote:
> Do you have the general materials about DDR programming in u-boot?

do not hijack threads.  start your own.
-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/20101004/28c1c0b5/attachment.pgp 

[U-Boot] packed attribute problem

[Wolfgang Denk]

Dear Reinhard Meyer,

In message <4CA9CC04.5020800@emk-elektronik.de> you wrote:
>
> In a non-packed struct an int will never be unaligned
> (unless you use an unaligned pointer to the whole struct)

Which may happen for example when overlaying such a struct on top of
some I/O buffer.

> In a packed struct an int might be unaligned, so it
> _might_ make sense for the compiler to handle that
> differently on ARM. Assume you overlay (bad idea anyway)

In a packed struct, where the struct itself is located with
sufficient alignment, and where all previous elements in the struct
are sufficiently aligned (like in the example I gave), it is NOT
possible that such unaligment happens.

> a packed structure over some communication data stream
> thats is byte oriented. On most architectures that would
> work (besides obvious endianess issues) but on ARM it would
> (without raising an exception) malfunction.

Keep in mind that the compiler knows the start address of the struct,
and the alignment of the elements. It could easily determine that no
unaligned accesses can result.

With the same logic you would have to enforce byte accesses for the
not-packed structs as well, as theese might be accessed through a not
sufficiently aligned pointer.

> So the "status_reg" pointer is in a completely unrelated (to the packed struct)
> "u32 *" and still the access is done like it was packed. If the
> compiler silently drags that attribute along into the "u32 *"
> THAT is really sick!

We do agree on this statement. GCC on ARM is a strange beast.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"The whole problem with the world is  that  fools  and  fanatics  are
always so certain of themselves, but wiser people so full of doubts."
- Bertrand Russell

[U-Boot] packed attribute problem

[Vipin Kumar]

On 10/4/2010 6:13 PM, Reinhard Meyer wrote:
>> > bar:
>> >         @ Function supports interworking.
>> >         @ args = 0, pretend = 0, frame = 0
>> >         @ frame_needed = 0, uses_anonymous_args = 0
>> >         @ link register save eliminated.
>> >         @ lr needed for prologue
>> >         mov     r2, #5
>> >         mov     r3, #4096
>> >         str     r2, [r3, #0]
>> >         bx      lr
>> >         .size   bar, .-bar
>> >         .ident  "GCC: (GNU) 4.2.2"
>> > 
>> > 
>> > Note that this is with GCC 4.2.2. Even GCC 4.0.0 behaves the same, so
>> > this is *not* an issue with very recent tool chains.
> OK, for directly adressing elements inside a packed struct;
> but the original post said:
> 
> "struct xyz {
> 	int	x;
> 	int	y;
> 	int	z[CONST];
> } __attribute__ ((packed));
> 
> struct xyz *abc;
> u32 * status_reg = (u32 *)&abc->z[0];
> 
> writel(status, status_reg);"
> 
> So the "status_reg" pointer is in a completely unrelated (to the packed struct)
> "u32 *" and still the access is done like it was packed. If the
> compiler silently drags that attribute along into the "u32 *"
> THAT is really sick!
> 

Yes, This is exactly what I wanted to point out. I can see this behavior with 
gcc-4.5.0 but the same code works well with gcc-4.2.4

Vipin

[U-Boot] packed attribute problem

[Detlev Zundel]

Hi Reinhard,

[...]

>> Note that this is with GCC 4.2.2. Even GCC 4.0.0 behaves the same, so
>> this is *not* an issue with very recent tool chains.
>
> OK, for directly adressing elements inside a packed struct;
> but the original post said:
>
> "struct xyz {
> 	int	x;
> 	int	y;
> 	int	z[CONST];
> } __attribute__ ((packed));
>
> struct xyz *abc;
> u32 * status_reg = (u32 *)&abc->z[0];
>
> writel(status, status_reg);"
>
> So the "status_reg" pointer is in a completely unrelated (to the packed struct)
> "u32 *" and still the access is done like it was packed. If the
> compiler silently drags that attribute along into the "u32 *"
> THAT is really sick!

I cannot follow why you think this is sick - actually I think this is
clever.  GCC "knows" that abc points to a packed struct and it has no
clue on actual values, so it _has to assume_ it is not aligned.
status_reg is initialized as a pointer to a member of this unaligned
pointer and is thus also considered unaligned.  Seems perfectly fine to
me.

Cheers
  Detlev

-- 
BUGS:
  It is not yet possible to change operating system by writing to
  /proc/sys/kernel/ostype.
                                     -- Linux sysctl(2) man page
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] packed attribute problem

[Scott Wood]

On Tue, 5 Oct 2010 13:43:01 +0200
Detlev Zundel <dzu@denx.de> wrote:

> Hi Reinhard,
> 
> [...]
> 
> >> Note that this is with GCC 4.2.2. Even GCC 4.0.0 behaves the same, so
> >> this is *not* an issue with very recent tool chains.
> >
> > OK, for directly adressing elements inside a packed struct;
> > but the original post said:
> >
> > "struct xyz {
> > 	int	x;
> > 	int	y;
> > 	int	z[CONST];
> > } __attribute__ ((packed));
> >
> > struct xyz *abc;
> > u32 * status_reg = (u32 *)&abc->z[0];
> >
> > writel(status, status_reg);"
> >
> > So the "status_reg" pointer is in a completely unrelated (to the packed struct)
> > "u32 *" and still the access is done like it was packed. If the
> > compiler silently drags that attribute along into the "u32 *"
> > THAT is really sick!
> 
> I cannot follow why you think this is sick - actually I think this is
> clever.  GCC "knows" that abc points to a packed struct and it has no
> clue on actual values, so it _has to assume_ it is not aligned.

So instead of the code breaking in the obvious situation where the
unaligned address is assigned to a pointer type that implies alignment,
it breaks when sufficient barriers to optimization are added that the
compiler can no longer keep track of where the value came from?

Seems like the right thing is for GCC to warn about the assignment to a
plain "u32 *", and allow a warningless assignment to something like
"u32 __attribute__((packed)) *".

-Scott

[U-Boot] packed attribute problem

[Detlev Zundel]

Hi Scott,

> On Tue, 5 Oct 2010 13:43:01 +0200
> Detlev Zundel <dzu@denx.de> wrote:
>
>> Hi Reinhard,
>> 
>> [...]
>> 
>> >> Note that this is with GCC 4.2.2. Even GCC 4.0.0 behaves the same, so
>> >> this is *not* an issue with very recent tool chains.
>> >
>> > OK, for directly adressing elements inside a packed struct;
>> > but the original post said:
>> >
>> > "struct xyz {
>> > 	int	x;
>> > 	int	y;
>> > 	int	z[CONST];
>> > } __attribute__ ((packed));
>> >
>> > struct xyz *abc;
>> > u32 * status_reg = (u32 *)&abc->z[0];
>> >
>> > writel(status, status_reg);"
>> >
>> > So the "status_reg" pointer is in a completely unrelated (to the packed struct)
>> > "u32 *" and still the access is done like it was packed. If the
>> > compiler silently drags that attribute along into the "u32 *"
>> > THAT is really sick!
>> 
>> I cannot follow why you think this is sick - actually I think this is
>> clever.  GCC "knows" that abc points to a packed struct and it has no
>> clue on actual values, so it _has to assume_ it is not aligned.
>
> So instead of the code breaking in the obvious situation where the
> unaligned address is assigned to a pointer type that implies alignment,
> it breaks when sufficient barriers to optimization are added that the
> compiler can no longer keep track of where the value came from?

This is getting somewhat theoretical here, but I do not believe that "a
pointer type implies alignment" in C.  I may be wrong here, but I cannot
think of any statement in the standards in this regard - it's only data
types that imply alignments.  So if this is true, then for pointers gcc
will always have to reason from actual values assigned to them.

> Seems like the right thing is for GCC to warn about the assignment to a
> plain "u32 *", and allow a warningless assignment to something like
> "u32 __attribute__((packed)) *".

Somehow I still feel that gcc should _always_ generate 32-bit accesses
when I dereference a u32 pointer in code.  Silently substituting 4 byte
accesses is simply not the right thing to do (maybe with some
-i-know-what-i-am-doing option, but certainly not by default).  

If such an access traps the machine, then so be it.  Then the code has
to be changed to reflect this in the source code level which will _make
programmers ware of it_.  Doing such a "silent fixup" which extends to
other situations thereby changing general semantics of the C language is
not a good thing IMHO.

Cheers
  Detlev

-- 
Modern technique has made it possible for leisure, within limits, to be not
the prerogative of small privileged classes, but a right evenly distributed
throughout the community.  The morality of  work is the morality of slaves,
and the modern world has no need of slavery.            -- Bertrand Russell
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] packed attribute problem

[Scott Wood]

On Thu, 7 Oct 2010 11:59:07 +0200
Detlev Zundel <dzu@denx.de> wrote:

> Hi Scott,
> 
> > So instead of the code breaking in the obvious situation where the
> > unaligned address is assigned to a pointer type that implies alignment,
> > it breaks when sufficient barriers to optimization are added that the
> > compiler can no longer keep track of where the value came from?
> 
> This is getting somewhat theoretical here, but I do not believe that "a
> pointer type implies alignment" in C.

I think this gets down to how such pointers are generated -- if you
stick with well-defined C, the compiler/ABI should be able to avoid
generating an unaligned pointer.

> I may be wrong here, but I cannot
> think of any statement in the standards in this regard - it's only data
> types that imply alignments.

Pointers point to typed data...

>  So if this is true, then for pointers gcc
> will always have to reason from actual values assigned to them.

That's not always possible.  How will GCC reason about this pointer,
where foo is called from a different compilation unit?

int foo(int *ptr)
{
	return *ptr;
}

> 
> > Seems like the right thing is for GCC to warn about the assignment to a
> > plain "u32 *", and allow a warningless assignment to something like
> > "u32 __attribute__((packed)) *".
> 
> Somehow I still feel that gcc should _always_ generate 32-bit accesses
> when I dereference a u32 pointer in code.

C doesn't guarantee this, and it would be broken on certain chips
when unaligned, e.g. old ARM chips that do weird rotaty things rather
than trap when you do an unaligned access.

This is why MMIO accessors should be done with inline assembly rather
than a volatile pointer, although it's usually just a theoretical
concern.

> If such an access traps the machine, then so be it.  Then the code has
> to be changed to reflect this in the source code level which will _make
> programmers ware of it_.

There is something at the source code level triggering this:
__attribute__((packed)).

That said, I don't think a lack of alignment of certain fields within
the struct ought to imply a lack of alignment of the start of the
struct.  There should have been two separate attributes for that, but
changing it now would probably not be a good thing (unless two entirely
new names are introduced).

-Scott

[U-Boot] packed attribute problem

[Wolfgang Denk]

Dear Scott Wood,

In message <20101007114634.70e0f197@udp111988uds.am.freescale.net> you wrote:
>
> I think this gets down to how such pointers are generated -- if you
> stick with well-defined C, the compiler/ABI should be able to avoid
> generating an unaligned pointer.

It is a pretty common method to use a pointer to some struct (for
example, some form of PDU) and make it point to some I/O buffer.
Depending on I/O subsystem, pre-pended protocol headerss and such the
resulting pointer may or may not be aligned. This is something that
the compiler has zero influence on.  [Yes, of course the application
could always copy the data to a properly aligned memory region. But I
bet you would quickly complain about the poor throughput if the
network stack on your systems were implemented that way.]

> > I may be wrong here, but I cannot
> > think of any statement in the standards in this regard - it's only data
> > types that imply alignments.
> 
> Pointers point to typed data...

Yes, and these typed date can be anywhere, even on addresses that
don't match the natural alignment of these data.

> C doesn't guarantee this, and it would be broken on certain chips

Actually I consider this a deficiency in the C specification. I've
been using C since Unix Version 6, and to me it is still mostly a
high-level assembler language. I definitly do not want the compiler
to do different things than what I tell him.

> when unaligned, e.g. old ARM chips that do weird rotaty things rather
> than trap when you do an unaligned access.

I think it was acceptable for such systems to enforce the compiler (by
setting special compiler switches) to implement such extra access modes.
But doing it by default and unconditionally for all systems of that
architecture seems broken to me. It's unfair collective punishment.

But alas. That's how it is in ARM land...


Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Perl already has an IDE.  It's called Unix.
                      -- Tom Christiansen in 375bd509 at cs.colorado.edu

[U-Boot] packed attribute problem

[Scott Wood]

On Thu, 7 Oct 2010 19:57:01 +0200
Wolfgang Denk <wd@denx.de> wrote:

> Dear Scott Wood,
> 
> In message <20101007114634.70e0f197@udp111988uds.am.freescale.net> you wrote:
> >
> > I think this gets down to how such pointers are generated -- if you
> > stick with well-defined C, the compiler/ABI should be able to avoid
> > generating an unaligned pointer.
> 
> It is a pretty common method to use a pointer to some struct (for
> example, some form of PDU) and make it point to some I/O buffer.

Yes, but at that point we are not talking about well-defined C, but
rather implementation-specific behavior.  There's nothing wrong with
it, but the C standard is no longer authoritative on what happens in
such cases.

> Depending on I/O subsystem, pre-pended protocol headerss and such the
> resulting pointer may or may not be aligned. This is something that
> the compiler has zero influence on.  [Yes, of course the application
> could always copy the data to a properly aligned memory region. But I
> bet you would quickly complain about the poor throughput if the
> network stack on your systems were implemented that way.]

Yes.  And there would also be performance complaints if each of those
accesses were to trap and be emulated (even ignoring weird stuff like
old ARM).  Thus it's nice to have some sort of pointer or data type
annotation to tell the compiler to be careful.

> > when unaligned, e.g. old ARM chips that do weird rotaty things rather
> > than trap when you do an unaligned access.
> 
> I think it was acceptable for such systems to enforce the compiler (by
> setting special compiler switches) to implement such extra access modes.
> But doing it by default and unconditionally for all systems of that
> architecture seems broken to me. It's unfair collective punishment.

Yes, it ought to be configurable -- but this is only happening when
you use __attribute__((packed)) without explicit alignment, right?

BTW, I see GCC splitting accesses to bitfields in a packed
struct into bytes on powerpc, even with -mno-strict-align.

-Scott

[U-Boot] packed attribute problem

[Wolfgang Denk]

Dear Scott Wood,

In message <20101007135257.05a93975@udp111988uds.am.freescale.net> you wrote:
>
> > It is a pretty common method to use a pointer to some struct (for
> > example, some form of PDU) and make it point to some I/O buffer.
> 
> Yes, but at that point we are not talking about well-defined C, but
> rather implementation-specific behavior.  There's nothing wrong with
> it, but the C standard is no longer authoritative on what happens in
> such cases.

Huch?  Which part of that is not well-defined (or even not
standard-conforming) C?

> Yes.  And there would also be performance complaints if each of those
> accesses were to trap and be emulated (even ignoring weird stuff like
> old ARM).  Thus it's nice to have some sort of pointer or data type
> annotation to tell the compiler to be careful.

I also complain about poor performance when instead of a single
instruction (a 32 bit load) at least 4 (8 bit) instructions need to be
executed.

> BTW, I see GCC splitting accesses to bitfields in a packed
> struct into bytes on powerpc, even with -mno-strict-align.

Indeed. Bitfields have always been evil.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"And it should be the law: If you use  the  word  `paradigm'  without
knowing  what  the  dictionary  says  it  means,  you  go to jail. No
exceptions."                     - David Jones @ Megatest Corporation

[U-Boot] packed attribute problem

[Scott Wood]

On Thu, 7 Oct 2010 21:31:43 +0200
Wolfgang Denk <wd@denx.de> wrote:

> Dear Scott Wood,
> 
> In message <20101007135257.05a93975@udp111988uds.am.freescale.net> you wrote:
> >
> > > It is a pretty common method to use a pointer to some struct (for
> > > example, some form of PDU) and make it point to some I/O buffer.
> > 
> > Yes, but at that point we are not talking about well-defined C, but
> > rather implementation-specific behavior.  There's nothing wrong with
> > it, but the C standard is no longer authoritative on what happens in
> > such cases.
> 
> Huch?  Which part of that is not well-defined (or even not
> standard-conforming) C?

Well, how do you obtain the unaligned pointer?

Casting an integer to a pointer?  6.3.2.3: "An integer may be converted
to any pointer type. Except as previously specified, the result is
implementation-defined, might not be properly aligned, and might not
point to an entity of the referenced type."

Casting a pointer to a pointer of a different type?  6.3.2.3: "A pointer
to an object or incomplete type may be converted to a pointer to a
different object or incomplete type. If the resulting pointer is not
correctly aligned for the pointed-to type, the behavior is undefined."

I was expecting to find something stating that it was not well-defined
behavior if you dereference a pointer that points to data of a
different type, but couldn't (maybe I wasn't searching for the right
terms) -- though a similar limitation is present in 6.5.2.3 for unions:
"With one exception, if the value of a member of a union object is used
when the most recent store to the object was to a different member, the
behavior is implementation-defined."

Extensions like __attribute__((packed)) are obviously not
well-defined C.

> > Yes.  And there would also be performance complaints if each of those
> > accesses were to trap and be emulated (even ignoring weird stuff like
> > old ARM).  Thus it's nice to have some sort of pointer or data type
> > annotation to tell the compiler to be careful.
> 
> I also complain about poor performance when instead of a single
> instruction (a 32 bit load) at least 4 (8 bit) instructions need to be
> executed.

So only use the unaligned annotation when there's a significant chance
of it really being unaligned -- and make sure the compiler knows
whether the cost of an unaligned access is significantly worse than the
cost of its workaround.

> > BTW, I see GCC splitting accesses to bitfields in a packed
> > struct into bytes on powerpc, even with -mno-strict-align.
> 
> Indeed. Bitfields have always been evil.

Evil or not, I don't see why GCC feels the need to do this.

-Scott

[U-Boot] packed attribute problem

[Detlev Zundel]

Hi Scott,

> On Thu, 7 Oct 2010 21:31:43 +0200
> Wolfgang Denk <wd@denx.de> wrote:
>
>> Dear Scott Wood,
>> 
>> In message <20101007135257.05a93975@udp111988uds.am.freescale.net> you wrote:
>> >
>> > > It is a pretty common method to use a pointer to some struct (for
>> > > example, some form of PDU) and make it point to some I/O buffer.
>> > 
>> > Yes, but at that point we are not talking about well-defined C, but
>> > rather implementation-specific behavior.  There's nothing wrong with
>> > it, but the C standard is no longer authoritative on what happens in
>> > such cases.
>> 
>> Huch?  Which part of that is not well-defined (or even not
>> standard-conforming) C?
>
> Well, how do you obtain the unaligned pointer?

[...]

> Extensions like __attribute__((packed)) are obviously not
> well-defined C.

In ISO-IEC9899:TC3[1] I find this about dereferencing a pointer:

,----[ 6.5.3.2 footnote 87 ]
| [...]
| Among the invalid values for dereferencing a pointer by the unary *
| operator are a null pointer, an address inappropriately aligned for the
| type of object pointed to, and the address of an object after the end of
| its lifetime.
`----

So dereferencing such a pointer - however we arrive at it - is simply
invalid.  The same sentence is in the ANSI spec[2] also.  The C FAQ[3]
notes in section 16.8 that using unaligned pointers may very well lead
to segmentation faults, bus errors or general protection faults.  Not
much documentation actually...

Cheers
  Detlev

[1] http://www.open-std.org/JTC1/SC22/WG14/www/docs/n1256.pdf
[2] http://flash-gordon.me.uk/ansi.c.txt
[3] http://www.c-faq.com/versions.html


-- 
... the tools we are trying to use and the language or notation we are using to
express or record our thoughts,  are the major factors  determining what we can
think or express at all!
                                        -- Edsger W. Dijkstra
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] packed attribute problem

[Balau]

Dear all,

I wanted to point out clues about why GCC manages the packed attribute
in this way.

The behavior is suspiciously similar to what the ARM RVDS compiler
(armcc) does with the __packed qualifier:

> [...]
> Thus if you wish to define a pointer to a word that can be at any address (i.e. that can be at a non-natural
> alignment) then you must specify this using the __packed qualifier when defining the pointer:
>
>    __packed int *pi; // pointer to unaligned int
>
> The ARM compilers will not then use an LDR, but instead generate code which correctly accesses the value
> regardless of the alignment of the pointer. This code generated will be a sequence of byte accesses, or
> variable alignment-dependent shifting and masking (depending on the compile options) and will therefore
> incur a performance and code size penalty.
> [...]
( From http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.faqs/ka3544.html
)

I am starting to suspect that the "feature" was copied from the armcc
compiler, even though there shouldn't be any reason to do that.

I can picture someone translating C code from armcc to GCC and
changing the __packed qualifier with the packed attribute, and then
obtaining undesired 32-bit accesses when he wanted 8-bit accesses.
Maybe the fact that GCC packed attribute did not map globals to
unaligned addresses was listed as a "bug" by someone who was more
familiar with armcc, and the developers who "fixed" it were uncertain
about GCC specifications. Maybe the modification was part of a big
changeset and went overlooked, and changing it back now would break so
much code it's better to leave it so. But these are only speculations.

The file "gcc/config/arm/arm.h" inside GCC source contains possibly
useful information:
> /* Setting STRUCTURE_SIZE_BOUNDARY to 32 produces more efficient code, but the
> value set in previous versions of this toolchain was 8, which produces more
> compact structures.  The command line option -mstructure_size_boundary=<n>
> can be used to change this value.  For compatibility with the ARM SDK
> however the value should be left at 32.  ARM SDT Reference Manual (ARM DUI
> 0020D) page 2-20 says "Structures are aligned on word boundaries".
> The AAPCS specifies a value of 8.  */

It seems, though, that using -mstructure-size-boundary=8 or 32 with a
4.4.1-based toolchain has no difference on the managing of packed
structs.

Another interesting scenario is the access of arrays of packed
structs. If a struct is packed but not aligned(4), its sizeof() could
be a number that is not a multiple of the alignment (in the case of
ARM targets). Even if the first element of the array is aligned, the
second element has an unaligned address.

Hope it rings some bells.

Regards,
Francesco

[U-Boot] packed attribute problem

[Vipin Kumar]

On 10/4/2010 4:14 PM, Wolfgang Denk wrote:
> Dear Vipin Kumar,
> 
> In message <4CA9ACAA.2020001@st.com> you wrote:
>>
>>>> This writel results in writing byte by byte on the address pointed to by status_reg.
>>>> This problem is visible with both gcc version 4.4.1 as well as 4.5.0
>>>
>>> I bet this is on some ARM system?
>>
>> Yes, it is on an ARM system (CortexA9). But I still feel that since I am creating 
>> a new u32 * status_reg, the code should not use any intelligence and use the pointer 
>> only to produce an str instruction in the form
>> 	str r0, [r1]
>>
>> But it retains the packed property of the structure even with a new u32 ponter 
>> typecasted to u32 *
>> 	u32 * status_reg = (u32 *)xyz->x;
>>
>> A writel to status_reg results in byte by byte writing
> 
> I agree with you. I always considered such behaviour of the ARM C
> compiler a bug, and still do.  However, people with better knowledge
> of the ARm architecture than me might be able to explain why the
> responsible PTB consider this to be a good and necessary "feature" of
> th compiler.
> 
>>> Hm... Why do these structs have any "__attribute__ ((packed))" at all?
>>
>> Even I could not understand that very well
> 
> Eventually alignment of these structs cannot be guaranteed?
> 

In my opinion it can be guaranteed. 
btw, I am talking about ehci_hcor structure in include/usb/host/ehci.h
The only reason I am confused is that a lot many platforms would have faced a
similar problem (or is it only me). 

Please confirm if I should remove the packed attribute and send a patch

This also raises one doubt. Since u-boot code now contains structures to access 
device registers, using packed attribute with these structures can be lethal

Regards
Vipin

[U-Boot] packed attribute problem

[Wolfgang Denk]

Dear Vipin Kumar,

In message <4CA9B316.3050802@st.com> you wrote:
>
> >>> Hm... Why do these structs have any "__attribute__ ((packed))" at all?
> >>
> >> Even I could not understand that very well
> > 
> > Eventually alignment of these structs cannot be guaranteed?
> 
> In my opinion it can be guaranteed. 
> btw, I am talking about ehci_hcor structure in include/usb/host/ehci.h
> The only reason I am confused is that a lot many platforms would have faced a
> similar problem (or is it only me). 
> 
> Please confirm if I should remove the packed attribute and send a patch

This is mostly a decision Remy has to make (on cc:)

> This also raises one doubt. Since u-boot code now contains structures to access 
> device registers, using packed attribute with these structures can be lethal

On ARM, indeed. Such structures must not use any packed attributes.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
About the use of language: it is impossible to sharpen a pencil  with
a  blunt  ax.  It is equally vain to try to do it with ten blunt axes
instead.                                           -- Edsger Dijkstra