Hi John,
On Tuesday, December 28, 2010 6:17 AM, John Rigby wrote:
> Signed-off-by: John Rigby <john.ri...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
