Hi John,
On Tuesday, December 28, 2010 6:17 AM, John Rigby wrote:
Signed-off-by: John Rigby john.rigby@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