[U-Boot] [PATCH v3 7/8] imximage: Add MX53 boot image support

[Stefano Babic <sbabic@denx.de>]

On 12/29/2010 01:49 PM, Jason Liu wrote:
> This patch add the MX53 boot image support.
> 
> This patch has been tested on Freescale MX53EVK board
> and MX51EVK board.
> 
> Signed-off-by: Jason Liu <r64343@freescale.com>

Hi Jason,

> diff --git a/board/freescale/mx51evk/imximage.cfg b/board/freescale/mx51evk/imximage.cfg
> index a875e8f..11825fb 100644
> --- a/board/freescale/mx51evk/imximage.cfg
> +++ b/board/freescale/mx51evk/imximage.cfg
> @@ -25,6 +25,10 @@
>  #
>  # The syntax is taken as close as possible with the kwbimage
>  
> +# Imximage version
> +
> +IMXIMAGE_VERSION 1

The name is not consistent with the other commands, as they do not use
the IMXIMAGE_ prefix (we do not have such as IMXIMAGE_BOOT_FROM). Please
replace with VERSION.

Not change the mx51evk file. The code should take VERSION=1 as default,
and we do not need to change the actual boards.

> diff --git a/board/ttcontrol/vision2/imximage_hynix.cfg b/board/ttcontrol/vision2/imximage_hynix.cfg
> index ed531db..cdc533d 100644
> --- a/board/ttcontrol/vision2/imximage_hynix.cfg
> +++ b/board/ttcontrol/vision2/imximage_hynix.cfg
> @@ -28,6 +28,10 @@
>  #
>  # The syntax is taken as close as possible with the kwbimage
>  
> +# Imximage version
> +
> +IMXIMAGE_VERSION 2

...and this is wrong, as vision is a MX51 board.

> diff --git a/doc/README.imximage b/doc/README.imximage
> index 3378f7e..48ac466 100644
> --- a/doc/README.imximage
> +++ b/doc/README.imximage
> @@ -57,6 +57,11 @@ Configuration command line syntax:
>  2. Following are the valid command strings and associated data strings:-
>  	Command string		data string
>  	--------------		-----------
> +	IMXIMAGE_VERSION        1/2
> +				1 is for mx25/mx35/mx51 compatible,
> +				2 is for mx53 compatible,
> +				others is invalid and error is generated.

As the VERSION selects the type of the header, I do not think it could
be set in any place of the file, or the code must be aware of it. Please
add a note in the documentation and raise an error in code if the
VERSION command is read after any other suitable commands.

What happens for example if VERSION is set at the end of imximage.cfg file ?

> diff --git a/tools/imximage.c b/tools/imximage.c
> index 39f89c2..2bbc4a6 100644
> --- a/tools/imximage.c
> +++ b/tools/imximage.c
> @@ -36,6 +36,7 @@
>   * Supported commands for configuration file
>   */
>  static table_entry_t imximage_cmds[] = {
> +	{CMD_IMXIMAGE_VERSION,  "IMXIMAGE_VERSION",     "imximage version", },

Change IMXIMAGE_VERSION simply to VERSION

> +static void err_imximage_version(int version)
> +{
> +	fprintf(stderr,
> +		"Error: Unsuported imximage version:%d\n", version);
                            ^--typo, unsupported


> +static void set_dcd_value_v1(struct imx_header *imxhdr, char *name, int lineno,
> +					int fld, uint32_t value, uint32_t off)
> +{
> +	dcd_v1_t *dcd_v1 = &imxhdr->header.hdr_v1.dcd_table;
> +
> +	switch (fld) {
> +	case CFG_REG_SIZE:
> +		/* Byte, halfword, word */
> +		if ((value != 1) && (value != 2) && (value != 4)) {
> +			fprintf(stderr, "Error: %s[%d] - "
> +				"Invalid register size " "(%d)\n",
> +				name, lineno, value);
> +			exit(EXIT_FAILURE);
> +		}
> +		dcd_v1->addr_data[off].type = value;
> +		break;
> +	case CFG_REG_ADDRESS:
> +		dcd_v1->addr_data[off].addr = value;
> +		break;
> +	case CFG_REG_VALUE:
> +		dcd_v1->addr_data[off].value = value;
> +		break;
> +	default:
> +		break;
> +
> +	}
> +}
> +
> +static void set_dcd_value_v2(struct imx_header *imxhdr, char *name, int lineno,
> +					int fld, uint32_t value, uint32_t off)
> +{
> +	dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.dcd_table;
> +
> +	switch (fld) {
> +	case CFG_REG_ADDRESS:
> +		dcd_v2->addr_data[off].addr = cpu_to_be32(value);
> +		break;
> +	case CFG_REG_VALUE:
> +		dcd_v2->addr_data[off].value = cpu_to_be32(value);
> +		break;
> +	default:
> +		break;
> +
> +	}
> +}
> +
> +static void set_dcd_value(struct imx_header *imxhdr, char *name, int lineno,
> +					int fld, uint32_t value, uint32_t off)
> +{
> +	switch (imxhdr->imximage_version) {
> +	case IMXIMAGE_V1:
> +		set_dcd_value_v1(imxhdr, name, lineno, fld, value, off);
> +		break;
> +	case IMXIMAGE_V2:
> +		set_dcd_value_v2(imxhdr, name, lineno, fld, value, off);
> +		break;
> +	default:
> +		err_imximage_version(imxhdr->imximage_version);
> +		break;
> +	}
> +}

You inserted several help functions (set_dcd_value, print_...,..) and
all of them implement a switch case to reindirect to the specific
function. What about to drop them and to use function pointers, set when
the version of header is recognized ?

> @@ -82,44 +213,91 @@ static int imximage_check_image_types(uint8_t type)
>  static int imximage_verify_header(unsigned char *ptr, int image_size,
>  			struct mkimage_params *params)
>  {
> -
>  	struct imx_header *imx_hdr = (struct imx_header *) ptr;
> -	flash_header_t *hdr = &imx_hdr->fhdr;
> +	flash_header_v1_t *fhdr = &imx_hdr->header.hdr_v1.fhdr;
> +
> +	if (imx_hdr->imximage_version != IMXIMAGE_V1)
> +		return 0;

I think this is not correct. mkimage can be called with "-l" option to
check the correctness of the produced image without an imximage.cfg
file, and from your code it seems to me that actual images are not
recognized anymore. In fact, you recognize the header from a version
field that you add in the header, but it is not actually provided.

You should find the version of the header checking its structure, for
example recognizing APP_CODE_BARKER and DCD_BARKER for V1, and for
example DCD_HEADER_TAG for V2.

Please note that print_header and verify_header are part of the mkimage
API (in imximage_params table), and they are called by mkimage
independently without parsing any configuration file.

> +static void imximage_print_header(const void *ptr)
> +{
> +	struct imx_header *imx_hdr = (struct imx_header *) ptr;
> +
> +	switch (imx_hdr->imximage_version) {

As I said, this does not work with actual images. The tool should be
able to recognize the version directly from its structure (we have
enough magic numbers, or better, barkers, to do it), and not storing the
version into the header.

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-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================