Hi Wolfgang,
On Monday 16 May 2011 05:18 PM, Wolfgang Denk wrote:
> Dear Aneesh V,
>
> In message<4dd0f98a.2040...@ti.com> you wrote:
>>
>>>> @@ -141,6 +141,7 @@ static const table_entry_t uimage_type[] = {
>>>> { IH_TYPE_FLATDT, "flat_dt", "Flat Device Tree",
>>>> },
>>>> { IH_TYPE_KWBIMAGE, "kwbimage", "Kirkwood Boot
>>>> Image",},
>>>> { IH_TYPE_IMXIMAGE, "imximage", "Freescale i.MX Boot
>>>> Image",},
>>>> + { IH_TYPE_OMAPIMAGE, "omapimage", "TI OMAP CH/GP Boot Image",},
>>>> { -1, "", "",
>>>> },
>>>
>>> Please keep list sorted / sort list.
>>
>> Sort by the second field(kwbimage, omapimage etc), right?
>
> First field, but the result is the same.
>
>>>> +struct ch_toc {
>>>> + uint32_t section_offset;
>>>> + uint32_t section_size;
>>>> + uint8_t unused[12];
>>>> + uint8_t section_name[12];
>>>> +} __attribute__ ((__packed__));
>>>> +
>>>> +struct ch_settings {
>>>> + uint32_t section_key;
>>>> + uint8_t valid;
>>>> + uint8_t version;
>>>> + uint16_t reserved;
>>>> + uint32_t flags;
>>>> +} __attribute__ ((__packed__));
>>>> +
>>>> +struct gp_header {
>>>> + uint32_t size;
>>>> + uint32_t load_addr;
>>>> +} __attribute__ ((__packed__));
>>>
>>> Is there any good reason to have these "__attribute__ ((__packed__))"
>>> here? In general, these are only known to cause trouble and pain, and
>>> I cannot see a need here.
>>
>> ROM code expects the header in a precise format. I think it will not be
>> safe to leave it to the compiler to decide the field layout. For
>> instance, the compiler may align the uint8_t or uint16_t to 32 bit
>> boundary and that will break the Configuration Header.
>
> No. Not in the structs listed above.
Why do you think it will not create any problems. For instance, what if
the field "uint8_t version" in "struct ch_settings" is aligned to a 32
bit boundary by the compiler for faster access? That is not the
intended layout.
best regards,
Aneesh
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
