[U-Boot] [PATCH V2 01/21] imximage: make header variable length

[Stefano Babic <sbabic@denx.de>]

On 22/09/2012 04:38, Troy Kisky wrote:

Hi Troy,

> Also, the header offset is no longer
> right before the code starts.

Comment and subject of the patch do not match. Can you better explain it
? What have "making header variable length", that is, a new feature,
with " the header offset is no longer rigth", that is, a bug ?

Do we already have a variable header the we add a vrec_header function
to  image_type_params ?

> Series tested on an mx51 and mx6q
> ---
>  tools/imximage.c |  142 +++++++++++++++++++++++++++++++-----------------------
>  tools/imximage.h |   10 ++--
>  2 files changed, 87 insertions(+), 65 deletions(-)
> 
> diff --git a/tools/imximage.c b/tools/imximage.c
> index 03a7716..25d3b74 100644
> --- a/tools/imximage.c
> +++ b/tools/imximage.c
> @@ -65,12 +65,15 @@ static table_entry_t imximage_versions[] = {
>  	{-1,            "",     " (Invalid)",                 },
>  };
>  
> -static struct imx_header imximage_header;
>  static uint32_t imximage_version;
>  
>  static set_dcd_val_t set_dcd_val;
>  static set_dcd_rst_t set_dcd_rst;
>  static set_imx_hdr_t set_imx_hdr;
> +static set_imx_size_t set_imx_size;
> +static uint32_t g_flash_offset;
> +
> +static struct image_type_params imximage_params;
>  
>  static uint32_t get_cfg_value(char *token, char *name,  int linenr)
>  {
> @@ -207,85 +210,79 @@ static void set_dcd_rst_v2(struct imx_header *imxhdr, uint32_t dcd_len,
>  	dcd_v2->write_dcd_command.param = DCD_COMMAND_PARAM;
>  }
>  
> -static void set_imx_hdr_v1(struct imx_header *imxhdr, uint32_t dcd_len,
> -					struct stat *sbuf,
> -					struct mkimage_params *params)
> +static int set_imx_hdr_v1(struct imx_header *imxhdr, uint32_t dcd_len,
> +		uint32_t entry_point, uint32_t flash_offset)
>  {
>  	imx_header_v1_t *hdr_v1 = &imxhdr->header.hdr_v1;
>  	flash_header_v1_t *fhdr_v1 = &hdr_v1->fhdr;
>  	dcd_v1_t *dcd_v1 = &hdr_v1->dcd_table;
> -	uint32_t base_offset;
> -
> -	/* Exit if there is no BOOT_FROM field specifying the flash_offset */
> -	if(imxhdr->flash_offset == FLASH_OFFSET_UNDEFINED) {
> -		fprintf(stderr, "Error: Header v1: No BOOT_FROM tag in %s\n",
> -			params->imagename);
> -		exit(EXIT_FAILURE);
> -	}

Do you drop BOOT_FROM ? Then this should be documented. Is this to allow
that the same image can be loaded from different media, that share the
same flash offset ? Then, instead of drop it, I suggest to add more
entries in the imximage file, one for each media that is allow.

Something like:
	BOOT_FROM	sd, nand, spi

and maybe a check in the code if all entries do not share the same start
address.

> +	uint32_t hdr_base;
> +	uint32_t header_length = (((char *)&dcd_v1->addr_data[dcd_len].addr)
> +			- ((char *)imxhdr));
>  

For V1, the header is preallocated with the maximum size, that is the
maximum number of DCD entries the SOC in V1 can support. Why do we need
a dynamic length for V1 processors ? As far as I know, the number of
entries and fields for theses SOCs (i.MX25, i.MX35, i.MX51) is fixed.

>  	/* Set magic number */
>  	fhdr_v1->app_code_barker = APP_CODE_BARKER;
>  
> -	fhdr_v1->app_dest_ptr = params->addr;
> -	fhdr_v1->app_dest_ptr = params->ep - imxhdr->flash_offset -
> -		sizeof(struct imx_header);
> -	fhdr_v1->app_code_jump_vector = params->ep;
> +	hdr_base = entry_point - header_length;
> +	fhdr_v1->app_dest_ptr = hdr_base - flash_offset;
> +	fhdr_v1->app_code_jump_vector = entry_point;
>  
> -	base_offset = fhdr_v1->app_dest_ptr + imxhdr->flash_offset ;
> -	fhdr_v1->dcd_ptr_ptr =
> -		(uint32_t) (offsetof(flash_header_v1_t, dcd_ptr) -
> -		offsetof(flash_header_v1_t, app_code_jump_vector) +
> -		base_offset);
> -
> -	fhdr_v1->dcd_ptr = base_offset +
> -			offsetof(imx_header_v1_t, dcd_table);
> -
> -	/* The external flash header must be at the end of the DCD table */
> -	dcd_v1->addr_data[dcd_len].type = sbuf->st_size +
> -				imxhdr->flash_offset +
> -				sizeof(struct imx_header);
> +	fhdr_v1->dcd_ptr_ptr = hdr_base + offsetof(flash_header_v1_t, dcd_ptr);
> +	fhdr_v1->dcd_ptr = hdr_base + offsetof(imx_header_v1_t, dcd_table);
>  
>  	/* Security feature are not supported */
>  	fhdr_v1->app_code_csf = 0;
>  	fhdr_v1->super_root_key = 0;
> +	return header_length;
> +}
> +

Ok, I skip review of this part - it depends on your answer on the
previous question.

>  
> -static void set_imx_hdr_v2(struct imx_header *imxhdr, uint32_t dcd_len,
> -					struct stat *sbuf,
> -					struct mkimage_params *params)
> +static int set_imx_hdr_v2(struct imx_header *imxhdr, uint32_t dcd_len,
> +		uint32_t entry_point, uint32_t flash_offset)
>  {
>  	imx_header_v2_t *hdr_v2 = &imxhdr->header.hdr_v2;
>  	flash_header_v2_t *fhdr_v2 = &hdr_v2->fhdr;
> -
> -	/* Exit if there is no BOOT_FROM field specifying the flash_offset */
> -	if(imxhdr->flash_offset == FLASH_OFFSET_UNDEFINED) {
> -		fprintf(stderr, "Error: Header v2: No BOOT_FROM tag in %s\n",
> -			params->imagename);
> -		exit(EXIT_FAILURE);
> -	}
> +	uint32_t hdr_base;
> +	uint32_t header_length = (dcd_len) ?
> +		(char *)&hdr_v2->dcd_table.addr_data[dcd_len] - ((char*)imxhdr)
> +		: offsetof(imx_header_v2_t, dcd_table);

So you add a case where there is no DCD table at all. Apart that this
van be a use case, but does it happen in the real life ?

>  	/* Set magic number */
>  	fhdr_v2->header.tag = IVT_HEADER_TAG; /* 0xD1 */
>  	fhdr_v2->header.length = cpu_to_be16(sizeof(flash_header_v2_t));
>  	fhdr_v2->header.version = IVT_VERSION; /* 0x40 */
>  
> -	fhdr_v2->entry = params->ep;
> +	fhdr_v2->entry = entry_point;
>  	fhdr_v2->reserved1 = fhdr_v2->reserved2 = 0;
> -	fhdr_v2->self = params->ep - sizeof(struct imx_header);
> -
> -	fhdr_v2->dcd_ptr = fhdr_v2->self +
> -			offsetof(imx_header_v2_t, dcd_table);
> +	fhdr_v2->self = hdr_base = entry_point - header_length;
>  
> -	fhdr_v2->boot_data_ptr = fhdr_v2->self +
> -			offsetof(imx_header_v2_t, boot_data);
> -
> -	hdr_v2->boot_data.start = fhdr_v2->self - imxhdr->flash_offset;
> -	hdr_v2->boot_data.size = sbuf->st_size +
> -			imxhdr->flash_offset +
> -			sizeof(struct imx_header);
> +	fhdr_v2->dcd_ptr = (dcd_len) ? hdr_base
> +			+ offsetof(imx_header_v2_t, dcd_table) : 0;
> +	fhdr_v2->boot_data_ptr = hdr_base
> +			+ offsetof(imx_header_v2_t, boot_data);
> +	hdr_v2->boot_data.start = hdr_base - flash_offset;
>  
>  	/* Security feature are not supported */
>  	fhdr_v2->csf = 0;
> +	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)
> @@ -295,11 +292,13 @@ static void set_hdr_func(struct imx_header *imxhdr)
>  		set_dcd_val = set_dcd_val_v1;
>  		set_dcd_rst = set_dcd_rst_v1;
>  		set_imx_hdr = set_imx_hdr_v1;
> +		set_imx_size = set_imx_size_v1;
>  		break;
>  	case IMXIMAGE_V2:
>  		set_dcd_val = set_dcd_val_v2;
>  		set_dcd_rst = set_dcd_rst_v2;
>  		set_imx_hdr = set_imx_hdr_v2;
> +		set_imx_size = set_imx_size_v2;
>  		break;
>  	default:
>  		err_imximage_version(imximage_version);
> @@ -381,9 +380,9 @@ static void parse_cfg_cmd(struct imx_header *imxhdr, int32_t cmd, char *token,
>  		set_hdr_func(imxhdr);
>  		break;
>  	case CMD_BOOT_FROM:
> -		imxhdr->flash_offset = get_table_entry_id(imximage_bootops,
> +		g_flash_offset = get_table_entry_id(imximage_bootops,
>  					"imximage boot option", token);
> -		if (imxhdr->flash_offset == -1) {

Why do we need a global when we have already a way to not use a global ?

> +		if (g_flash_offset == -1) {
>  			fprintf(stderr, "Error: %s[%d] -Invalid boot device"
>  				"(%s)\n", name, lineno, token);
>  			exit(EXIT_FAILURE);
> @@ -521,12 +520,17 @@ static void imximage_print_header(const void *ptr)
>  	}
>  }
>  
> -static void imximage_set_header(void *ptr, struct stat *sbuf, int ifd,
> -				struct mkimage_params *params)
> +int imximage_vrec_header(struct mkimage_params *params,
> +		struct image_type_params *tparams)
>  {
> -	struct imx_header *imxhdr = (struct imx_header *)ptr;
> +	struct imx_header *imxhdr;
>  	uint32_t dcd_len;
>  
> +	imxhdr = calloc(1, MAX_HEADER_SIZE);
> +	if (!imxhdr) {
> +		fprintf(stderr, "Error: out of memory\n");
> +		exit(EXIT_FAILURE);
> +	}
>  	/*
>  	 * In order to not change the old imx cfg file
>  	 * by adding VERSION command into it, here need
> @@ -534,14 +538,31 @@ static void imximage_set_header(void *ptr, struct stat *sbuf, int ifd,
>  	 */
>  	imximage_version = IMXIMAGE_V1;
>  	/* Be able to detect if the cfg file has no BOOT_FROM tag */
> -	imxhdr->flash_offset = FLASH_OFFSET_UNDEFINED;
> +	g_flash_offset = FLASH_OFFSET_UNDEFINED;
>  	set_hdr_func(imxhdr);
>  
>  	/* Parse dcd configuration file */
>  	dcd_len = parse_cfg_file(imxhdr, params->imagename);
>  
> +	/* Exit if there is no BOOT_FROM field specifying the flash_offset */
> +	if (g_flash_offset == FLASH_OFFSET_UNDEFINED) {
> +		fprintf(stderr, "Error: No BOOT_FROM tag in %s\n",
> +				params->imagename);
> +		exit(EXIT_FAILURE);
> +	}
>  	/* Set the imx header */
> -	(*set_imx_hdr)(imxhdr, dcd_len, sbuf, params);
> +	imximage_params.header_size = (*set_imx_hdr)(imxhdr, dcd_len,
> +			params->ep, g_flash_offset);
> +	imximage_params.hdr = imxhdr;
> +	return 0;
> +}
> +
> +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);
>  }
>  
>  int imximage_check_params(struct mkimage_params *params)
> @@ -571,8 +592,9 @@ int imximage_check_params(struct mkimage_params *params)
>   */
>  static struct image_type_params imximage_params = {
>  	.name		= "Freescale i.MX 5x Boot Image support",
> -	.header_size	= sizeof(struct imx_header),
> -	.hdr		= (void *)&imximage_header,
> +	.header_size	= 0,
> +	.hdr		= NULL,
> +	.vrec_header	= imximage_vrec_header,
>  	.check_image_type = imximage_check_image_types,
>  	.verify_header	= imximage_verify_header,
>  	.print_header	= imximage_print_header,
> diff --git a/tools/imximage.h b/tools/imximage.h
> index 34f293d..5fe3a8a 100644


> --- a/tools/imximage.h
> +++ b/tools/imximage.h
> @@ -30,6 +30,7 @@
>  #define DCD_BARKER	0xB17219E9
>  
>  #define HEADER_OFFSET	0x400
> +#define MAX_HEADER_SIZE	(16 << 10)
>  
>  #define CMD_DATA_STR	"DATA"
>  #define FLASH_OFFSET_UNDEFINED	0xFFFFFFFF
> @@ -156,7 +157,6 @@ struct imx_header {
>  		imx_header_v1_t hdr_v1;
>  		imx_header_v2_t hdr_v2;
>  	} header;
> -	uint32_t flash_offset;
>  };
>  
>  typedef void (*set_dcd_val_t)(struct imx_header *imxhdr,
> @@ -168,9 +168,9 @@ typedef void (*set_dcd_rst_t)(struct imx_header *imxhdr,
>  					uint32_t dcd_len,
>  					char *name, int lineno);
>  
> -typedef void (*set_imx_hdr_t)(struct imx_header *imxhdr,
> -					uint32_t dcd_len,
> -					struct stat *sbuf,
> -					struct mkimage_params *params);
> +typedef int (*set_imx_hdr_t)(struct imx_header *imxhdr, uint32_t dcd_len,
> +		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);

I disagree here. mkimage is valid for all architecture, we must not have
special entries here for a SOC or SOC family. For all other SOCs, DCD,
iMX header have no sense.
Anyway, why do you need to add set_imx_size_t when you call it only in
imximage.c ?

>  
>  #endif /* _IMXIMAGE_H_ */

You should split changes in image.h, that are valid for all
architecture, from changes to imximage.c, that are only for i.MX, into
different patches.

In my understanding you add additional entry points to have a variable
header lenght, but this feature is already used on TI with the AIS
image. You use also vrec_header. What am I missing here ?


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
=====================================================================