[U-Boot] [RFC PATCH 7/8] mkimage: Add OMAP boot image support
John Rigby
john.rigby at linaro.org
Tue Jan 4 16:37:02 CET 2011
On Tue, Jan 4, 2011 at 6:43 AM, Bedia, Vaibhav <vaibhav.bedia at ti.com> wrote:
> Hi John,
>
> On Tuesday, December 28, 2010 6:17 AM, John Rigby wrote:
>> 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/omapimage.c | 226
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>> tools/omapimage.h | 50 ++++++++++++ 6 files changed, 282
>> 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 f63a2ff..4198d76
>> 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, "", "", },
>> };
>>
> [...]
>
> We are working on patch sets to add support for TI816X and TI814X processor series from Texas Instruments. This series includes DM8168/8148, C6A816x and AM389x devices.
>
> We were also in the process of extending mkimage to attach a header to the u-boot binary for TI816X and TI814X. We could build upon the mkimage extension that you proposed, so please consider making it more generic.
>
>> diff --git a/include/image.h b/include/image.h index
>> 005e0d2..f74e2b9 100644 --- a/include/image.h
>> +++ b/include/image.h
>> @@ -157,6 +157,7 @@
>> #define IH_TYPE_FLATDT 8 /* Binary Flat Device Tree Blob */
>> #define IH_TYPE_KWBIMAGE 9 /* Kirkwood Boot Image */
>> #define IH_TYPE_IMXIMAGE 10 /* Freescale IMXBoot Image */
>> +#define IH_TYPE_OMAPIMAGE 11 /* TI OMAP Config Header Image */
>>
> [...]
>
> TIIMAGE instead of OMAPIMAGE sounds more generic.
>
> [...]
>> $(obj)image.o \
>> $(obj)imximage.o \
>> + $(obj)omapimage.o \
> Same here. This change could be done globally in the patch.
>
>> $(obj)kwbimage.o \
>> $(obj)md5.o \
>> $(obj)mkimage.o \
> [...]
>
>> +/* Header size is CH header rounded up to 512 bytes plus GP header */
>> +#define OMAP_CH_HDR_SIZE 512 #define OMAP_GP_HDR_SIZE
>> (sizeof(struct +gp_header)) #define OMAP_FILE_HDR_SIZE
>> +(OMAP_CH_HDR_SIZE+OMAP_GP_HDR_SIZE)
>> +
>> +static uint8_t omapimage_header[OMAP_FILE_HDR_SIZE];
>> +
>
> TI816X and TI814X only have GP_HDR. How about adding a config option like CONFIG_OMAP_TIIMAGE to decide upon the final size of the header over here? That config option can also help in conditional compilation of the code which deals with the configuration header.
>
> [...]
>
>> +static int omapimage_verify_header(unsigned char *ptr, int
>> image_size, + struct mkimage_params *params)
>> +{
>> + struct ch_toc *toc = (struct ch_toc *)ptr;
>> + struct gp_header *gph = (struct gp_header
>> *)(ptr+OMAP_CH_HDR_SIZE); + uint32_t offset, size;
>> +
>> + while (toc->section_offset != 0xffffffff
>> + && toc->section_size != 0xffffffff) {
>> + offset = toc->section_offset;
>> + size = toc->section_size;
>> + if (!offset || !size)
>> + return -1;
>> + if (offset >= OMAP_CH_HDR_SIZE || offset+size >=
>> OMAP_CH_HDR_SIZE) + return -1;
>> + toc++;
>> + }
>> + if (!valid_gph_size(gph->size))
>> + return -1;
>> + if (!valid_gph_load_addr(gph->load_addr))
>> + return -1;
>> +
>> + return 0;
>> +}
>
> Please consider splitting the various functions/adding checks for CH_HDR and GP_HDR.
>
> [...]
>
>> +
>> +/*
>> + * omapimage parameters
>> + */
>> +static struct image_type_params omapimage_params = {
>> + .name = "TI OMAP CH/GP Boot Image support",
>> + .header_size = OMAP_FILE_HDR_SIZE,
>> + .hdr = (void *)&omapimage_header,
>> + .check_image_type = omapimage_check_image_types,
>> + .verify_header = omapimage_verify_header,
>> + .print_header = omapimage_print_header,
>> + .set_header = omapimage_set_header,
>> + .check_params = omapimage_check_params,
>> +};
>> +
>
> The set_header and print_header implementations will vary for TI816X and TI814X so protecting them with a macro like CONFIG_OMAP_TIIMAGE will be needed. I think you'll need to add dummy functions also to avoid compilation errors.
>
> Adding dummy function also has one more advantage in case 2 different binaries are built from the same tree.
>
> Without dummy functions for the spl stage and the full-fledged u-boot binary there will be different commands.
> Eg: "make u-boot.ti" for the spl stage and just "make" in the second stage.
>
> But with dummy functions in place the user can invoke "make u-boot.ti" and not get confused about which command is to be used.
>
> [...]
>
>> +
>> +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__));
>> +
> [...]
>
> TI8168 and TI8148 will require only the gp_header struct. So please consider protecting ch_toc and ch_settings structures with a macro.
>
> If it helps, I can share the current code we have for the mkimage extension for TI816X and TI814X.
>
> Regards,
> Vaibhav
I agree with your suggestions however there is one problem with the
suggested implementation. A mkimage binary must be able to produce
any kind of image. See the recent conversation on extending mkimage
for i.mx53 for example. So a mkimage that supports OMAP and other TI
SOCs with only the GP header must make the selection at run time not
compile time.
I'll will try to add GP header only support in the next revsion.
Thanks
John
More information about the U-Boot
mailing list