[U-Boot] [PATCH V2 06/21] imximage: add plugin commands

[Stefano Babic <sbabic@denx.de>]

On 22/09/2012 04:39, Troy Kisky wrote:
> Add commands
> plugin address filename
> iomux_entry addr, data1 [, data2, [, data3]]
> write_entry addr, data1 [, data2, [, data3]]

Why do we need explicitely an IOMUX command ? As far as I can see, the
program image defined in V2 defines a plugin, but not an iomux.
I am expecting that the imximage generates a iMX header only, without
moving some code from the initialization code directly here. In the
manula there is a "Write Data" (what we have always had), a "Check data"
and an "Unlock" commands.

If we start to add special commands, maybe we are staring again to
reimplement U-Boot. We could have some SET_CLK, SET_CPU_FREQ, and so on.
What I am really mising in this series is why you are moving a lot of
things from U-Boot into the iMX header.

It seems to me we want to put much more code in the iMX header as what
it is really required to boot the device.

Adding / modifying the syntax requires to update doc/README.imximage, too.

> 
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> ---
>  tools/imximage.c |  334 ++++++++++++++++++++++++++++++++++++++++++++----------
>  tools/imximage.h |   11 +-
>  2 files changed, 283 insertions(+), 62 deletions(-)
> 
> diff --git a/tools/imximage.c b/tools/imximage.c
> index 2c5a622..fae786a 100644
> --- a/tools/imximage.c
> +++ b/tools/imximage.c
> @@ -31,7 +31,6 @@
>  #include "mkimage.h"
>  #include <image.h>
>  #include "imximage.h"
> -
>  /*
>   * Supported commands for configuration file
>   */
> @@ -39,6 +38,9 @@ static table_entry_t imximage_cmds[] = {
>  	{CMD_BOOT_FROM,         "BOOT_FROM",            "boot command",	  },
>  	{CMD_DATA,              "DATA",                 "Reg Write Data", },
>  	{CMD_IMAGE_VERSION,     "IMAGE_VERSION",        "image version",  },
> +	{CMD_PLUGIN,		"plugin",		"plugin addr,file",  },
> +	{CMD_IOMUX_ENTRY,	"iomux_entry",		"Write iomux reg",  },
> +	{CMD_WRITE_ENTRY,	"write_entry",		"Write register",  },
>  	{-1,                    "",                     "",	          },
>  };
>  
> @@ -69,8 +71,8 @@ static uint32_t imximage_version;
>  
>  static set_dcd_val_t set_dcd_val;
>  static set_imx_hdr_t set_imx_hdr;
> -static set_imx_size_t set_imx_size;
>  static uint32_t *p_max_dcd;
> +static uint32_t *header_size_ptr;
>  static uint32_t g_flash_offset;
>  
>  static struct image_type_params imximage_params;
> @@ -88,8 +90,7 @@ static uint32_t detect_imximage_version(struct imx_header *imx_hdr)
>  		return IMXIMAGE_V1;
>  
>  	/* Try to detect V2 */
> -	if ((fhdr_v2->header.tag == IVT_HEADER_TAG) &&
> -		(hdr_v2->dcd_table.header.tag == DCD_HEADER_TAG))
> +	if ((fhdr_v2->header.tag == IVT_HEADER_TAG))
>  		return IMXIMAGE_V2;

Help me to understand. I am reading i.MX6 manual and, even if the number
of DCD entries could be variable, I do not see why the header tag of DCD
is moving. At least, this is what I can see on picture 7-19, Image
Vector table.

>  
>  	return IMXIMAGE_VER_INVALID;
> @@ -160,7 +161,7 @@ static int set_dcd_val_v2(struct imx_header *imxhdr, uint32_t *data)
>  }
>  
>  static int set_imx_hdr_v1(struct imx_header *imxhdr,
> -		uint32_t entry_point, uint32_t flash_offset)
> +		uint32_t entry_point, uint32_t flash_offset, int plugin)
>  {
>  	imx_header_v1_t *hdr_v1 = &imxhdr->header.hdr_v1;
>  	flash_header_v1_t *fhdr_v1 = &hdr_v1->fhdr;
> @@ -180,22 +181,12 @@ static int set_imx_hdr_v1(struct imx_header *imxhdr,
>  	/* Security feature are not supported */
>  	fhdr_v1->app_code_csf = 0;
>  	fhdr_v1->super_root_key = 0;
> +	header_size_ptr = (uint32_t *)(((char *)imxhdr) + header_length - 4);
>  	return header_length;
>  }
>  
> -static void set_imx_size_v1(struct imx_header *imxhdr, uint32_t file_size,
> -		uint32_t flash_offset)
> -{
> -	uint32_t *p = (uint32_t *)(((char *)imxhdr)
> -			+ imximage_params.header_size);
> -
> -	/* The external flash header must be at the end of the DCD table */
> -	/* file_size includes header */
> -	p[-1] = file_size + flash_offset;
> -}

I think you have to squash some of your patches or to defines them in
another way. You added this code previously, and you drop now. This
makes more difficult to review your patches.

> -
>  static int set_imx_hdr_v2(struct imx_header *imxhdr,
> -		uint32_t entry_point, uint32_t flash_offset)
> +		uint32_t entry_point, uint32_t flash_offset, int plugin)
>  {
>  	imx_header_v2_t *hdr_v2 = &imxhdr->header.hdr_v2;
>  	flash_header_v2_t *fhdr_v2 = &hdr_v2->fhdr;
> @@ -216,27 +207,20 @@ static int set_imx_hdr_v2(struct imx_header *imxhdr,
>  	fhdr_v2->boot_data_ptr = hdr_base
>  			+ offsetof(imx_header_v2_t, boot_data);
>  	hdr_v2->boot_data.start = hdr_base - flash_offset;
> +	hdr_v2->boot_data.plugin = plugin;
>  
>  	/* Security feature are not supported */
>  	fhdr_v2->csf = 0;
> +	header_size_ptr = &hdr_v2->boot_data.size;
>  	return header_length;
>  }
>  
> -static void set_imx_size_v2(struct imx_header *imxhdr, uint32_t file_size,
> -		uint32_t flash_offset)
> -{
> -	imx_header_v2_t *hdr_v2 = &imxhdr->header.hdr_v2;
> -	/* file_size includes header */
> -	hdr_v2->boot_data.size = file_size + flash_offset;
> -}
> -
>  static void set_hdr_func(struct imx_header *imxhdr)
>  {
>  	switch (imximage_version) {
>  	case IMXIMAGE_V1:
>  		set_dcd_val = set_dcd_val_v1;
>  		set_imx_hdr = set_imx_hdr_v1;
> -		set_imx_size = set_imx_size_v1;
>  		p_entry = &imxhdr->header.hdr_v1.dcd_table.addr_data[0].type;
>  		p_max_dcd = &imxhdr->header.hdr_v1.dcd_table
>  				.addr_data[MAX_HW_CFG_SIZE_V1].type;
> @@ -245,7 +229,6 @@ static void set_hdr_func(struct imx_header *imxhdr)
>  	case IMXIMAGE_V2:
>  		set_dcd_val = set_dcd_val_v2;
>  		set_imx_hdr = set_imx_hdr_v2;
> -		set_imx_size = set_imx_size_v2;
>  		p_entry = (uint32_t *)&imxhdr->header.hdr_v2.dcd_table;
>  		p_max_dcd = (uint32_t *)((char *)imxhdr + MAX_HEADER_SIZE);
>  		break;
> @@ -283,31 +266,49 @@ static void print_hdr_v1(struct imx_header *imx_hdr)
>  	printf("Entry Point:  %08x\n", (uint32_t)fhdr_v1->app_code_jump_vector);
>  }
>  
> -static void print_hdr_v2(struct imx_header *imx_hdr)
> +static void print_header_info2(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;
> +
> +	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);
> +}
> +
> +static void print_hdr_v2(struct imx_header *imxhdr)
> +{
> +	imx_header_v2_t *hdr_v2 = &imxhdr->header.hdr_v2;
>  	dcd_v2_t *dcd_v2 = &hdr_v2->dcd_table;
>  	uint32_t size, version;
>  
> -	size = be16_to_cpu(dcd_v2->header.length) - 8;
> -	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);
> +	if (hdr_v2->fhdr.dcd_ptr) {
> +		size = be16_to_cpu(dcd_v2->header.length) - 8;
> +		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);
> +	version = detect_imximage_version(imxhdr);
>  
>  	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);
> +	print_header_info2(imxhdr);
> +	if (hdr_v2->boot_data.plugin) {
> +		uint32_t flash_offset =
> +				hdr_v2->fhdr.self - hdr_v2->boot_data.start;
> +		/* The 1st size includes flash offset and the next header */
> +		uint32_t plugin_length = hdr_v2->boot_data.size - flash_offset
> +				- offsetof(imx_header_v2_t, dcd_table);
> +
> +		imxhdr = (struct imx_header *)((char *)imxhdr + plugin_length);
> +		print_header_info2(imxhdr);
> +	}
>  }
>  
>  static int cmd_cnt;
> @@ -363,6 +364,24 @@ int skip_separators(struct data_src *ds)
>  	}
>  }
>  
> +int skip_comma(struct data_src *ds)
> +{
> +	char *p = ds->p;
> +
> +	for (;;) {
> +		char c = *p++;
> +		if ((c == '#') || (c == '\r') || (c == '\n') || !c)
> +			return 0;
> +		if (c == ',') {
> +			ds->p = p;
> +			skip_separators(ds);
> +			return 1;
> +		}
> +		if ((c != ' ') && (c == '\t'))
> +			return 0;
> +	}
> +}
> +
>  char *grab_token(char *dest, int size, char *src)
>  {
>  	while (size) {
> @@ -551,16 +570,18 @@ do_unary:
>  static int parse_cmd_data(struct data_src *ds)
>  {
>  	uint32_t data[3];
> -	int ret = get_cfg_value(ds, &data[0]);
> +	int ret, i;
>  
> -	if (ret)
> -		return ret;
> -	ret = get_cfg_value(ds, &data[1]);
> -	if (ret)
> -		return ret;
> -	ret = get_cfg_value(ds, &data[2]);
> -	if (ret)
> -		return ret;
> +	if (ds->plugin) {
> +		fprintf(stderr, "DATA should be before plug command\n");
> +		return -1;
> +	}
> +	for (i = 0; i < 3; i++) {
> +		int ret = get_cfg_value(ds, &data[i]);
> +		if (ret)
> +			return ret;
> +		skip_comma(ds);		/* comma is optional */
> +	}
>  	ret = (*set_dcd_val)(ds->imxhdr, data);
>  	if (ret)
>  		return ret;
> @@ -573,6 +594,99 @@ static int parse_cmd_data(struct data_src *ds)
>  	return 0;
>  }
>  
> +static int get_data(struct data_src *ds, uint32_t *data, int cnt)
> +{
> +	int i = 0;
> +
> +	if (!ds->plugin) {
> +		fprintf(stderr, "missing plug command\n");
> +		return -1;
> +	}
> +	for (;;) {
> +		int ret = get_cfg_value(ds, &data[i++]);
> +		if (ret)
> +			return ret;
> +		if (i >= cnt)
> +			break;
> +		if (!skip_comma(ds))
> +			break;
> +	}
> +	if (i < 2) {
> +		fprintf(stderr, "missing ','\n");
> +		return -1;
> +	}
> +	while (i < cnt) {
> +		data[i] = data[i - 1];
> +		i++;
> +	}
> +	if ((data[1] == ds->prev[1]) && (data[2] == ds->prev[2])
> +			&& (data[3] == ds->prev[3])
> +			&& (data[4] == ds->prev[4]))
> +		i = 0;
> +	else if ((data[1] == data[2]) && (data[2] == data[3])
> +			&& (data[3] == data[4]))
> +		i = 1;
> +	else if ((data[2] == data[3]) && (data[3] == data[4]))
> +		i = 2;
> +	else
> +		i = 3;
> +	return i;
> +}
> +
> +static int store_data(struct data_src *ds, uint32_t *data, int cnt)
> +{
> +	int i;
> +
> +	for (i = 0; i < cnt; i++)
> +		*p_entry++ = data[i];
> +
> +	ds->prev[1] = data[1];
> +	ds->prev[2] = data[2];
> +	ds->prev[3] = data[3];
> +	ds->prev[4] = data[4];
> +	if (p_entry > p_max_dcd) {
> +		uint32_t size = (char *)p_max_dcd - (char *)ds->imxhdr;
> +		fprintf(stderr, "Error: header exceeds maximum size(%d)\n",
> +				size);
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +static int parse_iomux_entry(struct data_src *ds)
> +{
> +	uint32_t data[5];
> +	int i;
> +
> +	i = get_data(ds, data, 5);
> +	if (i < 0)
> +		return i;
> +	if (data[0] & (3 << 30)) {
> +		fprintf(stderr, "bad 1st value\n");
> +		return -1;
> +	}
> +	if (i < 3)
> +		i++;
> +	data[0] |= (i << 30);
> +	return store_data(ds, data, (i == 3) ? 5 : i);
> +}
> +
> +static int parse_write_entry(struct data_src *ds)
> +{
> +	uint32_t data[5];
> +	int i;
> +
> +	i = get_data(ds, data, 5);
> +	if (i < 0)
> +		return i;
> +	if (data[0] & 3) {
> +		fprintf(stderr, "Address must be aligned on word boundary\n");
> +		return -1;
> +	}
> +	data[0] |= i;
> +	return store_data(ds, data, (i == 3) ? 5 : (i + 1));
> +}
> +
>  static int parse_image_version(struct data_src *ds)
>  {
>  	int ret;
> @@ -618,8 +732,82 @@ static int parse_boot_from(struct data_src *ds)
>  	return 0;
>  }
>  
> +static int parse_plugin(struct data_src *ds)
> +{
> +	struct stat sbuf;
> +	int plug_file;
> +	unsigned char *ptr;
> +	char *p;
> +	char c;
> +	int ret;
> +	uint32_t plug_base;
> +	uint32_t header_length;
> +
> +	if (g_flash_offset == FLASH_OFFSET_UNDEFINED) {
> +		fprintf(stderr, "Error: Place BOOT_FROM before plugin\n");
> +		return -1;
> +	}
> +	ret = get_cfg_value(ds, &plug_base);
> +	if (ret)
> +		return ret;
> +
> +	if (skip_separators(ds))
> +		return -1;
> +	p = ds->p;
> +	for (;;) {
> +		c = *p;
> +		if (!c)
> +			break;
> +		if ((c == ' ') || (c == '\t') || (c == ';') || (c == '#')
> +				|| (c == '\r') || (c == '\n')) {
> +			*p = 0;
> +			break;
> +		}
> +		p++;
> +	}
> +	plug_file = open(ds->p, O_RDONLY|O_BINARY);
> +	if (plug_file < 0) {
> +		fprintf(stderr, "Can't open plugin file %s: %s\n",
> +				ds->p, strerror(errno));
> +		*p = c;
> +		return -1;
> +	}
> +	if (fstat(plug_file, &sbuf) < 0) {
> +		fprintf(stderr, "Can't stat %s: %s\n",
> +			ds->p, strerror(errno));
> +		close(plug_file);
> +		return -1;
> +	}
> +	ptr = mmap(0, sbuf.st_size, PROT_READ, MAP_SHARED, plug_file, 0);
> +	if (ptr == MAP_FAILED) {
> +		fprintf(stderr, "Can't read %s: %s\n",
> +			ds->p, strerror(errno));
> +		return -1;
> +	}
> +	*p = c;
> +	ds->p = p;
> +	/* Set the plugin header */
> +	header_length = (*set_imx_hdr)(ds->imxhdr, plug_base,
> +			g_flash_offset, 1);
> +
> +	p = ((char *)ds->imxhdr) + header_length;
> +	if ((p + sbuf.st_size) >= (char *)p_max_dcd) {
> +		fprintf(stderr, "Out of space\n");
> +		return -1;
> +	}
> +
> +	ds->plugin = 1;
> +	memcpy(p, ptr, sbuf.st_size);
> +	munmap((void *)ptr, sbuf.st_size);
> +	close(plug_file);
> +
> +	p_entry = (uint32_t *)(p + sbuf.st_size);
> +	return 0;
> +}
> +
>  parse_fld_t cmd_table[] = {
> -	parse_image_version, parse_boot_from, parse_cmd_data
> +	parse_image_version, parse_boot_from, parse_cmd_data, parse_plugin,
> +	parse_iomux_entry, parse_write_entry
>  };
>  
>  static int parse_command(struct data_src *ds)
> @@ -630,9 +818,11 @@ static int parse_command(struct data_src *ds)
>  	return cmd_table[cmd](ds);
>  }
>  
> -static void parse_cfg_file(struct imx_header *imxhdr, char *name)
> +static void parse_cfg_file(struct imx_header *imxhdr, char *name,
> +		uint32_t entry_point)
>  {
>  	struct data_src ds;
> +	uint32_t plugin_length = 0;
>  
>  	ds.line = NULL;
>  	ds.len = 0;
> @@ -662,6 +852,35 @@ static void parse_cfg_file(struct imx_header *imxhdr, char *name)
>  		cmd_cnt++;
>  	}
>  	fclose(ds.fd);
> +
> +	if (ds.plugin) {
> +		uint32_t header_length, more;
> +		struct imx_header *next_imxhdr;
> +
> +		*p_entry++ = 0;
> +		header_length = ((char *)p_entry) - ((char *)imxhdr);
> +		plugin_length = ((header_length - 1) | 0x3f) + 1;
> +		more = plugin_length - header_length;
> +		if (more)
> +			memset(p_entry, 0, more);
> +		next_imxhdr = (struct imx_header *)
> +				(((char *)imxhdr) + plugin_length);
> +		p_entry = (imximage_version == IMXIMAGE_V1) ? (uint32_t *)
> +			&next_imxhdr->header.hdr_v1.dcd_table.addr_data[0].type
> +			: (uint32_t *)&next_imxhdr->header.hdr_v2.dcd_table;
> +		if (p_entry > p_max_dcd) {
> +			fprintf(stderr, "Out of space\n");
> +			exit(EXIT_FAILURE);
> +		}
> +
> +		/* Set the plugin size in header to include next header */
> +		*header_size_ptr = ((char *)p_entry) - ((char *)imxhdr)
> +				+ g_flash_offset;
> +		imxhdr = next_imxhdr;
> +	}
> +	/* Set the imx header */
> +	imximage_params.header_size = (*set_imx_hdr)(imxhdr, entry_point,
> +			g_flash_offset + plugin_length, 0) + plugin_length;
>  	return;
>  }
>  
> @@ -727,7 +946,7 @@ int imximage_vrec_header(struct mkimage_params *params,
>  	set_hdr_func(imxhdr);
>  
>  	/* Parse dcd configuration file */
> -	parse_cfg_file(imxhdr, params->imagename);
> +	parse_cfg_file(imxhdr, params->imagename, params->ep);
>  
>  	/* Exit if there is no BOOT_FROM field specifying the flash_offset */
>  	if (g_flash_offset == FLASH_OFFSET_UNDEFINED) {
> @@ -735,9 +954,6 @@ int imximage_vrec_header(struct mkimage_params *params,
>  				params->imagename);
>  		exit(EXIT_FAILURE);
>  	}
> -	/* Set the imx header */
---> -	imximage_params.header_size = (*set_imx_hdr)(imxhdr, params->ep,
> -			g_flash_offset);
>  	imximage_params.hdr = imxhdr;
>  	return 0;
>  }
> @@ -746,8 +962,10 @@ static void imximage_set_header(void *ptr, struct stat *sbuf, int ifd,
>  				struct mkimage_params *params)
>  {
>  	/* Set the size in header */
> -	(*set_imx_size)((struct imx_header *)ptr, sbuf->st_size,
> -			g_flash_offset);
> +	uint32_t offset = (char *)header_size_ptr - (char *)imximage_params.hdr;
> +	uint32_t *p = (uint32_t *)((char *)ptr + offset);
> +
> +	*p = sbuf->st_size + g_flash_offset;
>  }
>  
>  int imximage_check_params(struct mkimage_params *params)
> diff --git a/tools/imximage.h b/tools/imximage.h
> index efd249b..7613386 100644
> --- a/tools/imximage.h
> +++ b/tools/imximage.h
> @@ -51,7 +51,10 @@
>  enum imximage_cmd {
>  	CMD_IMAGE_VERSION,
>  	CMD_BOOT_FROM,
> -	CMD_DATA
> +	CMD_DATA,
> +	CMD_PLUGIN,
> +	CMD_IOMUX_ENTRY,
> +	CMD_WRITE_ENTRY,
>  };
>  
>  enum imximage_version {
> @@ -158,6 +161,8 @@ struct data_src {
>  	char *filename;
>  	struct imx_header *imxhdr;
>  	char *p;
> +	int plugin;
> +	uint32_t prev[5];
>  };
>  
>  typedef int (*parse_fld_t)(struct data_src *ds);
> @@ -165,8 +170,6 @@ typedef int (*parse_fld_t)(struct data_src *ds);
>  typedef int (*set_dcd_val_t)(struct imx_header *imxhdr, uint32_t *data);
>  
>  typedef int (*set_imx_hdr_t)(struct imx_header *imxhdr, uint32_t entry_point,
> -		uint32_t flash_offset);
> -typedef void (*set_imx_size_t)(struct imx_header *imxhdr, uint32_t file_size,
> -		uint32_t flash_offset);
> +		uint32_t flash_offset, int plugin);
>  
>  #endif /* _IMXIMAGE_H_ */
> 

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================