[U-Boot,v3,5/6] omap4+: Avoid using __attribute__ ((__packed__))

Avoid using __attribute__ ((__packed__)) unless it's
absolutely necessary. "packed" will remove alignment
requirements for the respective objects and may cause
alignment issues unless alignment is also enforced
using a pragma.

Here, these packed attributes were causing alignment
faults in Thumb build.

Signed-off-by: Aneesh V <aneesh@ti.com>
---
 arch/arm/include/asm/arch-omap4/mux_omap4.h |    2 +-
 arch/arm/include/asm/arch-omap5/mux_omap5.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

On Thu, Feb 23, 2012 at 7:06 AM, Aneesh V <aneesh@ti.com> wrote:
> Avoid using __attribute__ ((__packed__)) unless it's
> absolutely necessary. "packed" will remove alignment
> requirements for the respective objects and may cause
> alignment issues unless alignment is also enforced
> using a pragma.
>
> Here, these packed attributes were causing alignment
> faults in Thumb build.
>
> Signed-off-by: Aneesh V <aneesh@ti.com>

Why did we pack these to start with?  Otherwise seems fine (and I see
the rest of the TI parts don't have this particular packing).

On Thursday 23 February 2012 07:51 PM, Tom Rini wrote:
> On Thu, Feb 23, 2012 at 7:06 AM, Aneesh V<aneesh@ti.com>  wrote:
>> Avoid using __attribute__ ((__packed__)) unless it's
>> absolutely necessary. "packed" will remove alignment
>> requirements for the respective objects and may cause
>> alignment issues unless alignment is also enforced
>> using a pragma.
>>
>> Here, these packed attributes were causing alignment
>> faults in Thumb build.
>>
>> Signed-off-by: Aneesh V<aneesh@ti.com>
>
> Why did we pack these to start with?  Otherwise seems fine (and I see
> the rest of the TI parts don't have this particular packing).
>

I think that was to save some space - to make sure that the compiler
didn't pad the structure to have the u16 fields at word boundary. But
even without "packed" the complier is not padding it. I checked that
today, the size of the mux arrays remain the same even after removing
the "packed". So, I guess the "packed" didn't have any impact.

On Thursday 23 February 2012 09:21:00 Tom Rini wrote:
> On Thu, Feb 23, 2012 at 7:06 AM, Aneesh V <aneesh@ti.com> wrote:
> > Avoid using __attribute__ ((__packed__)) unless it's
> > absolutely necessary. "packed" will remove alignment
> > requirements for the respective objects and may cause
> > alignment issues unless alignment is also enforced
> > using a pragma.
> > 
> > Here, these packed attributes were causing alignment
> > faults in Thumb build.
> 
> Why did we pack these to start with?  Otherwise seems fine (and I see
> the rest of the TI parts don't have this particular packing).

because these represent hardware register blocks which get used with writew() 
and typically those blocks get marked packed.

if the arch won't introduce padding with the members, then this change should 
be ok, and looking at the structs which are just 2 16bit members, that should 
be the case here.

if you really want to be pedantic, i think the alternative would be:
	struct pad_conf_entry {} __packed __aligned(2);
-mike

On Thursday 23 February 2012 09:56:24 Aneesh V wrote:
> On Thursday 23 February 2012 07:51 PM, Tom Rini wrote:
> > On Thu, Feb 23, 2012 at 7:06 AM, Aneesh V<aneesh@ti.com>  wrote:
> >> Avoid using __attribute__ ((__packed__)) unless it's
> >> absolutely necessary. "packed" will remove alignment
> >> requirements for the respective objects and may cause
> >> alignment issues unless alignment is also enforced
> >> using a pragma.
> >> 
> >> Here, these packed attributes were causing alignment
> >> faults in Thumb build.
> > 
> > Why did we pack these to start with?  Otherwise seems fine (and I see
> > the rest of the TI parts don't have this particular packing).
> 
> I think that was to save some space - to make sure that the compiler
> didn't pad the structure to have the u16 fields at word boundary. But
> even without "packed" the complier is not padding it. I checked that
> today, the size of the mux arrays remain the same even after removing
> the "packed". So, I guess the "packed" didn't have any impact.

i don't think so.  see my other reply.
-mike

On Thursday 23 February 2012 08:33 PM, Mike Frysinger wrote:
> On Thursday 23 February 2012 09:21:00 Tom Rini wrote:
>> On Thu, Feb 23, 2012 at 7:06 AM, Aneesh V<aneesh@ti.com>  wrote:
>>> Avoid using __attribute__ ((__packed__)) unless it's
>>> absolutely necessary. "packed" will remove alignment
>>> requirements for the respective objects and may cause
>>> alignment issues unless alignment is also enforced
>>> using a pragma.
>>>
>>> Here, these packed attributes were causing alignment
>>> faults in Thumb build.
>>
>> Why did we pack these to start with?  Otherwise seems fine (and I see
>> the rest of the TI parts don't have this particular packing).
>
> because these represent hardware register blocks which get used with writew()
> and typically those blocks get marked packed.

No. That's not the case. This is just a table that has the mux data for
the board. It's not directly mapped to hardware. The hardware register
offset is part of this table.

>
> if the arch won't introduce padding with the members, then this change should
> be ok, and looking at the structs which are just 2 16bit members, that should
> be the case here.
>
> if you really want to be pedantic, i think the alternative would be:
> 	struct pad_conf_entry {} __packed __aligned(2);

I had tried that and it had solved the problem too, but the "packed" is
not needed at all. That's purely a software table.

regards,
Aneesh