[U-Boot] [PATCH V3 1/8] tools: imximage: add plugin support
Eric Nelson
eric at nelint.com
Sat Oct 8 17:26:18 CEST 2016
Hi Peng,
I'm sorry for taking so long to go though this.
On 10/08/2016 08:58 AM, Peng Fan wrote:
> Add plugin support for imximage.
>
This CONFIG setting doesn't actually affect mkimage or imximage:
> Define CONFIG_USE_IMXIMG_PLUGIN in defconfig to enable using plugin.
>
> Signed-off-by: Peng Fan <peng.fan at nxp.com>
> Cc: Stefano Babic <sbabic at denx.de>
> Cc: Eric Nelson <eric at nelint.com>
> Cc: Ye Li <ye.li at nxp.com>
> Reviewed-by: Tom Rini <trini at konsulko.com>
> ---
>
> V3:
> Fix compile error.
>
> V2:
> Drop the CONFIG_USE_PLUGIN, make plugin always support in imximage.
>
> tools/imximage.c | 282 +++++++++++++++++++++++++++++++++++++++++++------------
> tools/imximage.h | 7 +-
> 2 files changed, 229 insertions(+), 60 deletions(-)
>
> diff --git a/tools/imximage.c b/tools/imximage.c
> index 092d550..7fa601e 100644
> --- a/tools/imximage.c
> +++ b/tools/imximage.c
> @@ -27,6 +27,7 @@ static table_entry_t imximage_cmds[] = {
> {CMD_CHECK_BITS_CLR, "CHECK_BITS_CLR", "Reg Check bits clr", },
> {CMD_CSF, "CSF", "Command Sequence File", },
> {CMD_IMAGE_VERSION, "IMAGE_VERSION", "image version", },
> + {CMD_PLUGIN, "PLUGIN", "file plugin_addr", },
> {-1, "", "", },
> };
>
> @@ -80,6 +81,9 @@ static uint32_t imximage_ivt_offset = UNDEFINED;
> static uint32_t imximage_csf_size = UNDEFINED;
> /* Initial Load Region Size */
> static uint32_t imximage_init_loadsize;
These seem very limiting.
>From what I can tell, there's no reason that you can't have multiple
plugins, and the use of these variables isn't really needed.
> +static uint32_t imximage_iram_free_start;
> +static uint32_t imximage_plugin_size;
> +static uint32_t plugin_image;
>
> static set_dcd_val_t set_dcd_val;
> static set_dcd_param_t set_dcd_param;
> @@ -118,7 +122,11 @@ static uint32_t detect_imximage_version(struct imx_header *imx_hdr)
>
> /* Try to detect V2 */
> if ((fhdr_v2->header.tag == IVT_HEADER_TAG) &&
> - (hdr_v2->dcd_table.header.tag == DCD_HEADER_TAG))
> + (hdr_v2->data.dcd_table.header.tag == DCD_HEADER_TAG))
> + return IMXIMAGE_V2;
> +
> + if ((fhdr_v2->header.tag == IVT_HEADER_TAG) &&
> + hdr_v2->boot_data.plugin)
> return IMXIMAGE_V2;
>
> return IMXIMAGE_VER_INVALID;
> @@ -165,7 +173,7 @@ static struct dcd_v2_cmd *gd_last_cmd;
> static void set_dcd_param_v2(struct imx_header *imxhdr, uint32_t dcd_len,
> int32_t cmd)
> {
I also don't understand why the choice to make the union
with either a DCD table or a plugin.
We should be able to have both, and this doesn't make
the code any easier.
> - dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.dcd_table;
> + dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.data.dcd_table;
> struct dcd_v2_cmd *d = gd_last_cmd;
> struct dcd_v2_cmd *d2;
> int len;
> @@ -261,21 +269,23 @@ static void set_dcd_rst_v1(struct imx_header *imxhdr, uint32_t dcd_len,
> static void set_dcd_rst_v2(struct imx_header *imxhdr, uint32_t dcd_len,
> char *name, int lineno)
> {
> - dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.dcd_table;
> - struct dcd_v2_cmd *d = gd_last_cmd;
> - int len;
> -
> - if (!d)
> - d = &dcd_v2->dcd_cmd;
> - len = be16_to_cpu(d->write_dcd_command.length);
> - if (len > 4)
> - d = (struct dcd_v2_cmd *)(((char *)d) + len);
> -
> - len = (char *)d - (char *)&dcd_v2->header;
> -
> - dcd_v2->header.tag = DCD_HEADER_TAG;
> - dcd_v2->header.length = cpu_to_be16(len);
> - dcd_v2->header.version = DCD_VERSION;
> + if (!imxhdr->header.hdr_v2.boot_data.plugin) {
> + dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.data.dcd_table;
> + struct dcd_v2_cmd *d = gd_last_cmd;
> + int len;
> +
> + if (!d)
> + d = &dcd_v2->dcd_cmd;
> + len = be16_to_cpu(d->write_dcd_command.length);
> + if (len > 4)
> + d = (struct dcd_v2_cmd *)(((char *)d) + len);
> +
> + len = (char *)d - (char *)&dcd_v2->header;
> +
> + dcd_v2->header.tag = DCD_HEADER_TAG;
> + dcd_v2->header.length = cpu_to_be16(len);
> + dcd_v2->header.version = DCD_VERSION;
> + }
> }
>
> static void set_imx_hdr_v1(struct imx_header *imxhdr, uint32_t dcd_len,
> @@ -317,24 +327,93 @@ static void set_imx_hdr_v2(struct imx_header *imxhdr, uint32_t dcd_len,
> fhdr_v2->header.length = cpu_to_be16(sizeof(flash_header_v2_t));
> fhdr_v2->header.version = IVT_VERSION; /* 0x40 */
>
It seems that the reason for a lot of this special-purpose code is to add
support for the entry address.
If mkimage is invoked with an entrypoint from the command-line:
~/$ ./tools/mkimage -n my.cfg -T imximage -e 0xentrypoint u-boot.img
u-boot.imx
This entry point is normally placed into the header, but if we have a
plugin in the .cfg file like this:
~/$ grep PLUGIN my.cfg
PLUGIN path/to/board/plugin.bin 0x00907000
Then we need to use the plugin's address instead of the command
line address, and use the command-line address for the file which
follows.
> - fhdr_v2->entry = entry_point;
> - fhdr_v2->reserved1 = fhdr_v2->reserved2 = 0;
> - hdr_base = entry_point - imximage_init_loadsize +
> - flash_offset;
> - fhdr_v2->self = hdr_base;
> - if (dcd_len > 0)
> - fhdr_v2->dcd_ptr = hdr_base
> - + offsetof(imx_header_v2_t, dcd_table);
> - else
> + if (!hdr_v2->boot_data.plugin) {
> + fhdr_v2->entry = entry_point;
> + fhdr_v2->reserved1 = 0;
> + fhdr_v2->reserved1 = 0;
> + hdr_base = entry_point - imximage_init_loadsize +
> + flash_offset;
> + fhdr_v2->self = hdr_base;
> + if (dcd_len > 0)
> + fhdr_v2->dcd_ptr = hdr_base +
> + offsetof(imx_header_v2_t, data);
> + else
> + fhdr_v2->dcd_ptr = 0;
> + fhdr_v2->boot_data_ptr = hdr_base
> + + offsetof(imx_header_v2_t, boot_data);
> + hdr_v2->boot_data.start = entry_point - imximage_init_loadsize;
> +
> + fhdr_v2->csf = 0;
> +
> + header_size_ptr = &hdr_v2->boot_data.size;
> + csf_ptr = &fhdr_v2->csf;
> + } else {
> + imx_header_v2_t *next_hdr_v2;
> + flash_header_v2_t *next_fhdr_v2;
> +
> + if (imximage_csf_size != 0) {
> + fprintf(stderr, "Error: Header v2: SECURE_BOOT is only supported in DCD mode!");
> + exit(EXIT_FAILURE);
> + }
> +
I think that only ->entry needs to be different between these
two blocks.
> + fhdr_v2->entry = imximage_iram_free_start +
> + flash_offset + sizeof(flash_header_v2_t) +
> + sizeof(boot_data_t);
> +
> + fhdr_v2->reserved1 = 0;
> + fhdr_v2->reserved2 = 0;
> + fhdr_v2->self = imximage_iram_free_start + flash_offset;
> +
> fhdr_v2->dcd_ptr = 0;
> - fhdr_v2->boot_data_ptr = hdr_base
> - + offsetof(imx_header_v2_t, boot_data);
> - hdr_v2->boot_data.start = entry_point - imximage_init_loadsize;
>
> - fhdr_v2->csf = 0;
> + fhdr_v2->boot_data_ptr = fhdr_v2->self +
> + offsetof(imx_header_v2_t, boot_data);
> +
> + hdr_v2->boot_data.start = imximage_iram_free_start;
> + /*
> + * The actural size of plugin image is "imximage_plugin_size +
> + * sizeof(flash_header_v2_t) + sizeof(boot_data_t)", plus the
> + * flash_offset space.The ROM code only need to copy this size
> + * to run the plugin code. However, later when copy the whole
> + * U-Boot image to DDR, the ROM code use memcpy to copy the
> + * first part of the image, and use the storage read function
> + * to get the remaining part. This requires the dividing point
> + * must be multiple of storage sector size. Here we set the
> + * first section to be 16KB for this purpose.
> + */
Where is the 16k limit coming from? I don't think this is necessary,
and if it is, we won't be able to load SPL using a plugin.
> + hdr_v2->boot_data.size = MAX_PLUGIN_CODE_SIZE;
> +
> + /* Security feature are not supported */
> + fhdr_v2->csf = 0;
> +
> + next_hdr_v2 = (imx_header_v2_t *)((char *)hdr_v2 +
> + imximage_plugin_size);
> +
> + next_fhdr_v2 = &next_hdr_v2->fhdr;
> +
> + next_fhdr_v2->header.tag = IVT_HEADER_TAG; /* 0xD1 */
> + next_fhdr_v2->header.length =
> + cpu_to_be16(sizeof(flash_header_v2_t));
> + next_fhdr_v2->header.version = IVT_VERSION; /* 0x40 */
> +
> + next_fhdr_v2->entry = entry_point;
> + hdr_base = entry_point - sizeof(struct imx_header);
> + next_fhdr_v2->reserved1 = 0;
> + next_fhdr_v2->reserved2 = 0;
> + next_fhdr_v2->self = hdr_base + imximage_plugin_size;
> +
> + next_fhdr_v2->dcd_ptr = 0;
> + next_fhdr_v2->boot_data_ptr = next_fhdr_v2->self +
> + offsetof(imx_header_v2_t, boot_data);
> +
> + next_hdr_v2->boot_data.start = hdr_base - flash_offset;
> +
> + header_size_ptr = &next_hdr_v2->boot_data.size;
>
> - header_size_ptr = &hdr_v2->boot_data.size;
> - csf_ptr = &fhdr_v2->csf;
> + next_hdr_v2->boot_data.plugin = 0;
> +
> + next_fhdr_v2->csf = 0;
> + }
> }
>
> static void set_hdr_func(void)
> @@ -393,16 +472,19 @@ static void print_hdr_v2(struct imx_header *imx_hdr)
> {
> imx_header_v2_t *hdr_v2 = &imx_hdr->header.hdr_v2;
> flash_header_v2_t *fhdr_v2 = &hdr_v2->fhdr;
> - dcd_v2_t *dcd_v2 = &hdr_v2->dcd_table;
> - uint32_t size, version;
> + dcd_v2_t *dcd_v2 = &hdr_v2->data.dcd_table;
> + uint32_t size, version, plugin;
>
> - size = be16_to_cpu(dcd_v2->header.length);
> - if (size > (MAX_HW_CFG_SIZE_V2 * sizeof(dcd_addr_data_t)) + 8) {
> - fprintf(stderr,
> - "Error: Image corrupt DCD size %d exceed maximum %d\n",
> - (uint32_t)(size / sizeof(dcd_addr_data_t)),
> - MAX_HW_CFG_SIZE_V2);
> - exit(EXIT_FAILURE);
> + plugin = hdr_v2->boot_data.plugin;
> + if (!plugin) {
> + size = be16_to_cpu(dcd_v2->header.length);
> + if (size > (MAX_HW_CFG_SIZE_V2 * sizeof(dcd_addr_data_t))) {
> + fprintf(stderr,
> + "Error: Image corrupt DCD size %d exceed maximum %d\n",
> + (uint32_t)(size / sizeof(dcd_addr_data_t)),
> + MAX_HW_CFG_SIZE_V2);
> + exit(EXIT_FAILURE);
> + }
> }
>
> version = detect_imximage_version(imx_hdr);
> @@ -410,19 +492,81 @@ static void print_hdr_v2(struct imx_header *imx_hdr)
> printf("Image Type: Freescale IMX Boot Image\n");
> printf("Image Ver: %x", version);
> printf("%s\n", get_table_entry_name(imximage_versions, NULL, version));
> - printf("Data Size: ");
> - genimg_print_size(hdr_v2->boot_data.size);
> - printf("Load Address: %08x\n", (uint32_t)fhdr_v2->boot_data_ptr);
> - printf("Entry Point: %08x\n", (uint32_t)fhdr_v2->entry);
> - if (fhdr_v2->csf && (imximage_ivt_offset != UNDEFINED) &&
> - (imximage_csf_size != UNDEFINED)) {
> - printf("HAB Blocks: %08x %08x %08x\n",
> - (uint32_t)fhdr_v2->self, 0,
> - hdr_v2->boot_data.size - imximage_ivt_offset -
> - imximage_csf_size);
> + printf("Mode: %s\n", plugin ? "PLUGIN" : "DCD");
> + if (!plugin) {
> + printf("Data Size: ");
> + genimg_print_size(hdr_v2->boot_data.size);
> + printf("Load Address: %08x\n", (uint32_t)fhdr_v2->boot_data_ptr);
> + printf("Entry Point: %08x\n", (uint32_t)fhdr_v2->entry);
> + if (fhdr_v2->csf && (imximage_ivt_offset != UNDEFINED) &&
> + (imximage_csf_size != UNDEFINED)) {
> + printf("HAB Blocks: %08x %08x %08x\n",
> + (uint32_t)fhdr_v2->self, 0,
> + hdr_v2->boot_data.size - imximage_ivt_offset -
> + imximage_csf_size);
> + }
> + } else {
> + imx_header_v2_t *next_hdr_v2;
> + flash_header_v2_t *next_fhdr_v2;
> +
> + /*First Header*/
> + printf("Plugin Data Size: ");
> + genimg_print_size(hdr_v2->boot_data.size);
> + printf("Plugin Code Size: ");
> + genimg_print_size(imximage_plugin_size);
> + printf("Plugin Load Address: %08x\n", hdr_v2->boot_data.start);
> + printf("Plugin Entry Point: %08x\n", (uint32_t)fhdr_v2->entry);
> +
> + /*Second Header*/
> + next_hdr_v2 = (imx_header_v2_t *)((char *)hdr_v2 +
> + imximage_plugin_size);
> + next_fhdr_v2 = &next_hdr_v2->fhdr;
> + printf("U-Boot Data Size: ");
> + genimg_print_size(next_hdr_v2->boot_data.size);
> + printf("U-Boot Load Address: %08x\n",
> + next_hdr_v2->boot_data.start);
> + printf("U-Boot Entry Point: %08x\n",
> + (uint32_t)next_fhdr_v2->entry);
> }
> }
>
> +static void copy_plugin_code(struct imx_header *imxhdr, char *plugin_file)
> +{
> + int ifd = -1;
> + struct stat sbuf;
> + char *plugin_buf = imxhdr->header.hdr_v2.data.plugin_code;
> + char *ptr;
> +
> + ifd = open(plugin_file, O_RDONLY|O_BINARY);
> + if (fstat(ifd, &sbuf) < 0) {
> + fprintf(stderr, "Can't stat %s: %s\n",
> + plugin_file,
> + strerror(errno));
> + exit(EXIT_FAILURE);
> + }
> +
> + ptr = mmap(0, sbuf.st_size, PROT_READ, MAP_SHARED, ifd, 0);
> + if (ptr == MAP_FAILED) {
> + fprintf(stderr, "Can't read %s: %s\n",
> + plugin_file,
> + strerror(errno));
> + exit(EXIT_FAILURE);
> + }
> +
> + if (sbuf.st_size > MAX_PLUGIN_CODE_SIZE) {
> + printf("plugin binary size too large\n");
> + exit(EXIT_FAILURE);
> + }
> +
> + memcpy(plugin_buf, ptr, sbuf.st_size);
> + imximage_plugin_size = sbuf.st_size;
> +
> + (void) munmap((void *)ptr, sbuf.st_size);
> + (void) close(ifd);
> +
> + imxhdr->header.hdr_v2.boot_data.plugin = 1;
> +}
> +
> static void parse_cfg_cmd(struct imx_header *imxhdr, int32_t cmd, char *token,
> char *name, int lineno, int fld, int dcd_len)
> {
> @@ -497,6 +641,10 @@ static void parse_cfg_cmd(struct imx_header *imxhdr, int32_t cmd, char *token,
> if (unlikely(cmd_ver_first != 1))
> cmd_ver_first = 0;
> break;
This structure of parse_cfg_cmd to handle the first
argument and parse_cfg_fld to handle the second
argument is unfortunate.
> + case CMD_PLUGIN:
> + plugin_image = 1;
> + copy_plugin_code(imxhdr, token);
> + break;
> }
> }
>
> @@ -542,6 +690,10 @@ static void parse_cfg_fld(struct imx_header *imxhdr, int32_t *cmd,
> }
> }
> break;
> + case CMD_PLUGIN:
> + value = get_cfg_value(token, name, lineno);
> + imximage_iram_free_start = value;
> + break;
> default:
> break;
> }
> @@ -649,6 +801,7 @@ static void imximage_set_header(void *ptr, struct stat *sbuf, int ifd,
> {
> struct imx_header *imxhdr = (struct imx_header *)ptr;
> uint32_t dcd_len;
> + uint32_t header_size;
>
> /*
> * In order to not change the old imx cfg file
> @@ -665,10 +818,15 @@ static void imximage_set_header(void *ptr, struct stat *sbuf, int ifd,
> dcd_len = parse_cfg_file(imxhdr, params->imagename);
>
> if (imximage_version == IMXIMAGE_V2) {
> - if (imximage_init_loadsize < imximage_ivt_offset +
> - sizeof(imx_header_v2_t))
> + header_size = sizeof(flash_header_v2_t) + sizeof(boot_data_t);
> + if (!plugin_image)
> + header_size += sizeof(dcd_v2_t);
> + else
> + header_size += MAX_PLUGIN_CODE_SIZE;
> +
> + if (imximage_init_loadsize < imximage_ivt_offset + header_size)
> imximage_init_loadsize = imximage_ivt_offset +
> - sizeof(imx_header_v2_t);
> + header_size;
> }
>
> /* Set the imx header */
> @@ -721,7 +879,7 @@ static int imximage_generate(struct image_tool_params *params,
> size_t alloc_len;
> struct stat sbuf;
> char *datafile = params->datafile;
> - uint32_t pad_len;
> + uint32_t pad_len, header_size;
>
> memset(&imximage_header, 0, sizeof(imximage_header));
>
> @@ -742,15 +900,21 @@ static int imximage_generate(struct image_tool_params *params,
> /* TODO: check i.MX image V1 handling, for now use 'old' style */
> if (imximage_version == IMXIMAGE_V1) {
> alloc_len = 4096;
> + header_size = 4096;
> } else {
> - if (imximage_init_loadsize < imximage_ivt_offset +
> - sizeof(imx_header_v2_t))
> + header_size = sizeof(flash_header_v2_t) + sizeof(boot_data_t);
> + if (!plugin_image)
> + header_size += sizeof(dcd_v2_t);
> + else
> + header_size += MAX_PLUGIN_CODE_SIZE;
> +
> + if (imximage_init_loadsize < imximage_ivt_offset + header_size)
> imximage_init_loadsize = imximage_ivt_offset +
> - sizeof(imx_header_v2_t);
> + header_size;
> alloc_len = imximage_init_loadsize - imximage_ivt_offset;
> }
>
> - if (alloc_len < sizeof(struct imx_header)) {
> + if (alloc_len < header_size) {
> fprintf(stderr, "%s: header error\n",
> params->cmdname);
> exit(EXIT_FAILURE);
> diff --git a/tools/imximage.h b/tools/imximage.h
> index c7b9b5c..fe4dd89 100644
> --- a/tools/imximage.h
> +++ b/tools/imximage.h
> @@ -9,6 +9,7 @@
> #define _IMXIMAGE_H_
>
> #define MAX_HW_CFG_SIZE_V2 220 /* Max number of registers imx can set for v2 */
> +#define MAX_PLUGIN_CODE_SIZE (16*1024)
> #define MAX_HW_CFG_SIZE_V1 60 /* Max number of registers imx can set for v1 */
> #define APP_CODE_BARKER 0xB1
> #define DCD_BARKER 0xB17219E9
> @@ -64,6 +65,7 @@ enum imximage_cmd {
> CMD_CHECK_BITS_SET,
> CMD_CHECK_BITS_CLR,
> CMD_CSF,
> + CMD_PLUGIN,
> };
>
> enum imximage_fld_types {
> @@ -164,7 +166,10 @@ typedef struct {
> typedef struct {
> flash_header_v2_t fhdr;
> boot_data_t boot_data;
> - dcd_v2_t dcd_table;
> + union {
> + dcd_v2_t dcd_table;
> + char plugin_code[MAX_PLUGIN_CODE_SIZE];
> + } data;
> } imx_header_v2_t;
>
> /* The header must be aligned to 4k on MX53 for NAND boot */
>
Again, I apologize for being slow to review this, but I'm
hoping to use this to build a package of SPL (as a plugin)
along with U-Boot and I still think it's doable.
A quick proof-of-concept shows that we can load SPL
directly as a plugin just by setting byte 0x28 to 1
(the plugin flag), and if we can test for serial download
mode as Stefano does in this patch, we can return
to the boot loader.
http://lists.denx.de/pipermail/u-boot/2015-December/237555.html
The subsequent patches to require plugin.S seem to get
in the way of that.
Regards,
Eric
More information about the U-Boot
mailing list