[U-Boot] [RFC PATCH 7/8] mkimage: Add OMAP boot image support

[Bedia, Vaibhav]

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