[U-Boot] [PATCH v2 01/22] mkimage: Add OMAP boot image support

[Aneesh V]

Hi Wolfgang,

On Monday 16 May 2011 12:36 AM, Wolfgang Denk wrote:
> Dear Aneesh V,
>
> In message<1305472900-4004-2-git-send-email-aneesh at ti.com>  you wrote:
>> From: John Rigby<john.rigby at linaro.org>
>>
>> Signed-off-by: John Rigby<john.rigby at linaro.org>
>> ---
>>   common/image.c    |    1 +
>>   include/image.h   |    1 +
>>   tools/Makefile    |    2 +
>>   tools/mkimage.c   |    2 +
>>   tools/mkimage.h   |    1 +
>>   tools/omapimage.c |  229 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   tools/omapimage.h |   50 ++++++++++++
>>   7 files changed, 286 insertions(+), 0 deletions(-)
>>   create mode 100644 tools/omapimage.c
>>   create mode 100644 tools/omapimage.h
>>
>> diff --git a/common/image.c b/common/image.c
>> index e542a57..7f6fe1c 100644
>> --- a/common/image.c
>> +++ b/common/image.c
>> @@ -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?

>
>> diff --git a/tools/Makefile b/tools/Makefile
>> index 623f908..a1c4ed7 100644
>> --- a/tools/Makefile
>> +++ b/tools/Makefile
>> @@ -84,6 +84,7 @@ OBJ_FILES-$(CONFIG_CMD_LOADS) += img2srec.o
>>   OBJ_FILES-$(CONFIG_INCA_IP) += inca-swap-bytes.o
>>   NOPED_OBJ_FILES-y += kwbimage.o
>>   NOPED_OBJ_FILES-y += imximage.o
>> +NOPED_OBJ_FILES-y += omapimage.o
>>   NOPED_OBJ_FILES-y += mkimage.o
>>   OBJ_FILES-$(CONFIG_NETCONSOLE) += ncb.o
>>   NOPED_OBJ_FILES-y += os_support.o
>> @@ -180,6 +181,7 @@ $(obj)mkimage$(SFX):	$(obj)crc32.o \
>>   			$(obj)fit_image.o \
>>   			$(obj)image.o \
>>   			$(obj)imximage.o \
>> +			$(obj)omapimage.o \
>>   			$(obj)kwbimage.o \
>>   			$(obj)md5.o \
>>   			$(obj)mkimage.o \
>
> Please keep lists sorted.

Ok.

>
>> --- /dev/null
>> +++ b/tools/omapimage.c
>> @@ -0,0 +1,229 @@
...

>> +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.

Just curious, what are the issues caused by "__packed__"?

>
> Best regards,
>
> Wolfgang Denk
>