Re: [U-Boot] [PATCH V3 1/8] tools: imximage: add plugin support

On Sat, Oct 08, 2016 at 05:26:18PM +0200, Eric Nelson wrote:

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:

Yeah. The host tool always support plugin code now. But need to define CONFIG_USE_IMXIMG_PLUGIN to compile plugin.S.

...

Define CONFIG_USE_IMXIMG_PLUGIN in defconfig to enable using plugin.

Signed-off-by: Peng Fan peng.fan@nxp.com Cc: Stefano Babic sbabic@denx.de Cc: Eric Nelson eric@nelint.com Cc: Ye Li ye.li@nxp.com Reviewed-by: Tom Rini trini@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.

I try to follow, are you talking about chained plugin images? Now we only support two IVT headers when using plugin. The first IVT header is for the plugin image, the second ivt header is for uboot image.

...

+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.

Confirmed with ROM team. DCD and plugin are exclusive,

You can not have both at the same time.

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.

There are two IVT headers when using plugin, the 0x907000 is for the first IVT header, 0x87800000 in command line is for the second IVT header.

...

• 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.

There are two IVT headers for plugin. In this block, the second IVT also needs to be handled.

...

• 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.

Confirmed with ROM team, there is no limitation. But SPL code needs to be small to be in OCRAM. I'll rework this piece code to drop this limitation.

...

• 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.

parse_cfg_cmd is to parse the CMD, parse_cfg_fld is to handle the second argument. This is the design of imximage.c to parse xx.cfg file. I do not want to break this.

...

• 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

some more work needed to use SPL as a plugin image, I think, directly use "PLUGIN xxx/SPL" may not work.

mx6_plugin.S is needed.

Regards, Peng.

(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