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

login
register
mail settings
Submitter Liu Hui-R64343
Date Dec. 29, 2010, 12:49 p.m.
Message ID <1293626967-18740-1-git-send-email-r64343@freescale.com>
Download mbox | patch
Permalink /patch/76895/
State Changes Requested
Delegated to: Stefano Babic
Headers show

Comments

Liu Hui-R64343 - Dec. 29, 2010, 12:49 p.m.
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>

---
Changes for v2:
- Address the following comments from Stefano,
  - Get rid of  #ifdef in the imximage.h and .c file and use
    the runtime check for imximage version
  - Document the IMXIMAGE_VERSION definiton in doc/README.imximage
  - Move mx53evk/config.mk and mx53evk/imximage.cfg to MX53EVK board
    support patch.
---
 board/freescale/mx51evk/imximage.cfg       |    4 +
 board/ttcontrol/vision2/imximage_hynix.cfg |    4 +
 doc/README.imximage                        |   10 +-
 tools/imximage.c                           |  357 ++++++++++++++++++++++------
 tools/imximage.h                           |   98 ++++++--
 5 files changed, 382 insertions(+), 91 deletions(-)
Stefano Babic - Dec. 30, 2010, 12:46 p.m.
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
Jason Liu - Dec. 31, 2010, 3:29 a.m.
Hi, Stefano,

2010/12/30 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.

OK, make sense.

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

Do you mean that if can't find the VERSION command in the cfg file, it
will be treated
as V1, right?

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

My bad. Sorry for typo.

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

Yes, I will add this note to tell that VERSION need be set before
other suitable commands.

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

Will fix it.

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

Sounds good, I think I can do the change.

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

OK,  make sense. I get your mean. I will do the change.

Thanks for the review.
Happy New Year!

>
> 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@denx.de
> =====================================================================
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Jason Liu - Dec. 31, 2010, 6:06 a.m.
Hi, Stefano,

2010/12/31 Jason Liu <liu.h.jason@gmail.com>:
> Hi, Stefano,
>
> 2010/12/30 Stefano Babic <sbabic@denx.de>:
>> On 12/29/2010 01:49 PM, Jason Liu wrote:
>>> This patch add the MX53 boot image support.
>>> @@ -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.
>
> OK,  make sense. I get your mean. I will do the change.

But, I did think again about your comments. If we store the version
into the header which
will make the version check more easier, the side-effect is that we
store some boot
ROM useless information into the header, but which should not bring some issues.

mkimage -l can work with the actual images if the version info stored
into the head.

Here is the log I get:

r64343@r64343-desktop:~/work_space/u-boot-upstream/u-boot$
./tools/mkimage -l u-boot.imx
Image Type:   Freescale IMX Boot Image
Image Ver:    1 (i.MX25/35/51 compatible)
Data Size:    170936 Bytes = 166.93 kB = 0.16 MB
Load Address: 977ff7fc
Entry Point:  97800000

What's you comments?

>
> Thanks for the review.
> Happy New Year!
>
>>
>> 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@denx.de
>> =====================================================================
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot@lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>>
>
Wolfgang Denk - Dec. 31, 2010, 8:10 a.m.
Dear Stefano Babic,

In message <4D1C7F08.1010400@denx.de> you wrote:
>
> 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.

No, plain "VERSION" is way too generic.  We need some prefix here.


Best regards,

Wolfgang Denk
Wolfgang Denk - Dec. 31, 2010, 8:12 a.m.
Dear Jason Liu,

In message <AANLkTimcHbOvNNTW9EnqaFrp3WvW3epgB7S7cvtKs0vH@mail.gmail.com> you wrote:
> 
> > Not change the mx51evk file. The code should take VERSION=1 as default,
> > and we do not need to change the actual boards.
>
> Do you mean that if can't find the VERSION command in the cfg file, it
> will be treated
> as V1, right?

No. You must not rely on variable setting from the config file. You
must be able to recognize existing images by reading their headers
without access to any specific board config.


Best regards,

Wolfgang Denk
Wolfgang Denk - Dec. 31, 2010, 8:14 a.m.
Dear Jason Liu,

In message <AANLkTinnfXd26YRCiwfNbT5Lx9aY5KXiHBOLSc8jGse4@mail.gmail.com> you wrote:
> 
> But, I did think again about your comments. If we store the version
> into the header which
> will make the version check more easier, the side-effect is that we
> store some boot
> ROM useless information into the header, but which should not bring some issues.
>
> mkimage -l can work with the actual images if the version info stored
> into the head.

Be careful.

> Here is the log I get:
>
> r64343@r64343-desktop:~/work_space/u-boot-upstream/u-boot$
> ./tools/mkimage -l u-boot.imx
> Image Type:   Freescale IMX Boot Image
> Image Ver:    1 (i.MX25/35/51 compatible)
> Data Size:    170936 Bytes = 166.93 kB = 0.16 MB
> Load Address: 977ff7fc
> Entry Point:  97800000
>
> What's you comments?

This looks as if you added something to the standard 64 byte image
header.  You must not modify this.  It is fixed for all architectures
and cannot be extended.

Best regards,

Wolfgang Denk
Jason Liu - Dec. 31, 2010, 8:36 a.m.
Hi, Wolfgang,

2010/12/31 Wolfgang Denk <wd@denx.de>:
> Dear Jason Liu,
>
> In message <AANLkTinnfXd26YRCiwfNbT5Lx9aY5KXiHBOLSc8jGse4@mail.gmail.com> you wrote:
>>
>> But, I did think again about your comments. If we store the version
>> into the header which
>> will make the version check more easier, the side-effect is that we
>> store some boot
>> ROM useless information into the header, but which should not bring some issues.
>>
>> mkimage -l can work with the actual images if the version info stored
>> into the head.
>
> Be careful.
>
>> Here is the log I get:
>>
>> r64343@r64343-desktop:~/work_space/u-boot-upstream/u-boot$
>> ./tools/mkimage -l u-boot.imx
>> Image Type:   Freescale IMX Boot Image
>> Image Ver:    1 (i.MX25/35/51 compatible)
>> Data Size:    170936 Bytes = 166.93 kB = 0.16 MB
>> Load Address: 977ff7fc
>> Entry Point:  97800000
>>
>> What's you comments?
>
> This looks as if you added something to the standard 64 byte image
> header.  You must not modify this.  It is fixed for all architectures
> and cannot be extended.

This change is specific for I.MX chip and will not affect other architectures.

Best Regards,
Jason Liu
Happy New Year!

>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> In the beginning, I was made. I didn't ask to be made. No one consul-
> ted with me or considered my feelings  in  this  matter.  But  if  it
> brought  some  passing fancy to some lowly humans as they haphazardly
> pranced their way through life's mournful jungle, then so be it.
> - Marvin the Paranoid Android
>
Wolfgang Denk - Dec. 31, 2010, 8:50 a.m.
Dear Jason Liu,

In message <AANLkTik1uxZK+7LL6cHtQ465Z4x+pm-kfQeaLi=D7-9S@mail.gmail.com> you wrote:
> 
> > This looks as if you added something to the standard 64 byte image
> > header. You must not modify this. It is fixed for all architectures
> > and cannot be extended.
>
> This change is specific for I.MX chip and will not affect other architectur> es.

All architectures use exactly the same binary image header.  This
header has no version field, and such a field cannot be added.

Best regards,

Wolfgang Denk
Jason Liu - Dec. 31, 2010, 9:25 a.m.
Hi, Wolfgang,

2010/12/31 Wolfgang Denk <wd@denx.de>:
> Dear Jason Liu,
>
> In message <AANLkTik1uxZK+7LL6cHtQ465Z4x+pm-kfQeaLi=D7-9S@mail.gmail.com> you wrote:
>>
>> > This looks as if you added something to the standard 64 byte image
>> > header. You must not modify this. It is fixed for all architectures
>> > and cannot be extended.
>>
>> This change is specific for I.MX chip and will not affect other architectur> es.
>
> All architectures use exactly the same binary image header.  This
> header has no version field, and such a field cannot be added.

Here, the imx header is the data structure we defined in imximage.c
and it will be used to generate
the boot-able image u-boot.imx of i.mx soc together with u-boot.bin by
.set_header function call of struct
image_type_params according to FSLi.MX boot ROM requirement.

When run the command "mkimage -l u-boot.imx", the imx header will be
fetched from the u-boot.imx and
the version information will be decoded according to data structure
definition, and then it do the verify by
checking some magic number or tag according to different version, and
in the end, it will print the header
information if it pass the verification.

So, I don't think it will affect other architectures.

>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> It is your destiny.                                     - Darth Vader
>
Stefano Babic - Dec. 31, 2010, 9:35 a.m.
On 12/31/2010 09:10 AM, Wolfgang Denk wrote:
> Dear Stefano Babic,
> 
> In message <4D1C7F08.1010400@denx.de> you wrote:
>>
>> 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.
> 
> No, plain "VERSION" is way too generic.  We need some prefix here.

As the setting is only inside the imximage configuration file, I
supposed that it is redundant to remember we are talking about IMX
images and VERSION is self explaininig related to the file itself.

Jason has already set a prefix (IMXIMAGE_VERSION), so we can take it.

Best regards,
Stefano Babic
Stefano Babic - Dec. 31, 2010, 9:48 a.m.
On 12/31/2010 07:06 AM, Jason Liu wrote:
> But, I did think again about your comments. If we store the version
> into the header which
> will make the version check more easier, the side-effect is that we
> store some boot
> ROM useless information into the header, but which should not bring some issues.

Exactly. And you introduce an incompatibility with actual images. If you
run your patched mkimage on actual u-boot images, you should not get a
valid value because they do not contain the version field. It works only
with images generated with the updated mkimage.

Tendentially I do not like to introduce some hidden and at the end
bad-documented fields if they are not strictly needed. The IMX header
supplied in u-boot is documented in Freescale's manuals, without adding
spare fields.

As I can see, the IMX header have enough "barker" and magic number to
make recognition easy. V1 rely on two magic numbers (barker for app code
and for dcd), and in V2 reading your patch I see at least IVT_HEADER_TAG
(0xD1) and DCD_HEADER_TAG. Checking these values at the specific offsets
you can find in a more reliable way if an image is a V1 or a V2 or no
imx image at all.

Best regards,
Stefano Babic
Stefano Babic - Dec. 31, 2010, 9:55 a.m.
On 12/31/2010 09:12 AM, Wolfgang Denk wrote:
> Dear Jason Liu,
> 
> In message <AANLkTimcHbOvNNTW9EnqaFrp3WvW3epgB7S7cvtKs0vH@mail.gmail.com> you wrote:
>>
>>> Not change the mx51evk file. The code should take VERSION=1 as default,
>>> and we do not need to change the actual boards.
>>
>> Do you mean that if can't find the VERSION command in the cfg file, it
>> will be treated
>> as V1, right?
> 
> No. You must not rely on variable setting from the config file. You
> must be able to recognize existing images by reading their headers
> without access to any specific board config.

Agree, but I think Jason is speaking here about the generation of the
imx image and not its detection. To make things clearer:

- to check the validity of an imximage file, mkimage must rely only on
the header, without any supplied configuration file, if any.

- to generate the imximage, I agree for compatibility reasons that the
default version should be V1. In other words, if the version is not
supplied in the file, the image is generated for an MX35/MX51 processor.

Best regards,
Stefano Babic
Stefano Babic - Dec. 31, 2010, 10:03 a.m.
On 12/31/2010 10:25 AM, Jason Liu wrote:
> Hi, Wolfgang,
> 
> 2010/12/31 Wolfgang Denk <wd@denx.de>:
>> Dear Jason Liu,
>>
>> In message <AANLkTik1uxZK+7LL6cHtQ465Z4x+pm-kfQeaLi=D7-9S@mail.gmail.com> you wrote:
>>>
>>>> This looks as if you added something to the standard 64 byte image
>>>> header. You must not modify this. It is fixed for all architectures
>>>> and cannot be extended.
>>>
>>> This change is specific for I.MX chip and will not affect other architectur> es.
>>
>> All architectures use exactly the same binary image header.  This
>> header has no version field, and such a field cannot be added.
> 
> Here, the imx header is the data structure we defined in imximage.c
> and it will be used to generate
> the boot-able image u-boot.imx of i.mx soc together with u-boot.bin by
> .set_header function call of struct
> image_type_params according to FSLi.MX boot ROM requirement.

Yes, the field is inside the imx header and not in the 64 byte structure.

> 
> When run the command "mkimage -l u-boot.imx", the imx header will be
> fetched from the u-boot.imx and
> the version information will be decoded according to data structure
> definition, and then it do the verify by
> checking some magic number or tag according to different version, and
> in the end, it will print the header
> information if it pass the verification.
> 
> So, I don't think it will affect other architectures.

Yes, but we rely on a single undocumented byte when on the other hand
Freescale has provided at least two well-documented magic numbers for
each version to detect if an image is valid or not. Ans as I stated,
introducing this version field generates an unneeded incompatibility
with atual images.

Best regards,
Stefano Babic
Wolfgang Denk - Dec. 31, 2010, 11:37 a.m.
Dear Jason Liu,

In message <AANLkTimUduwb8ktCPZw1RnJHAzbNiLcdr8-9vheee8DM@mail.gmail.com> you wrote:
> 
> So, I don't think it will affect other architectures.

I haven't seen your code yet, but your description does not convince
me.  Please keep in mind that not only "mkimage -l", but also the
U-Boot equivalent "iminfo" must be able to intepret this information -
on all boards.  Even when I load this image on a PowerPC board the
image detection must be working.

Best regards,

Wolfgang Denk
Wolfgang Denk - Dec. 31, 2010, 11:39 a.m.
Dear Stefano Babic,

In message <4D1DA3EA.4060208@denx.de> you wrote:
>
> > No, plain "VERSION" is way too generic.  We need some prefix here.
> 
> As the setting is only inside the imximage configuration file, I
> supposed that it is redundant to remember we are talking about IMX
> images and VERSION is self explaininig related to the file itself.
> 
> Jason has already set a prefix (IMXIMAGE_VERSION), so we can take it.

This seems the other extreme. How about a comprimize like
"IMG_VERSION" or "IMAGE_VERSION" or the like, so it's clear what sort
of version we are talking about, while at the same time allowing to
reuse this on other architectures if that should be needed one day?

Best regards,

Wolfgang Denk
Stefano Babic - Dec. 31, 2010, 12:47 p.m.
On 12/31/2010 12:39 PM, Wolfgang Denk wrote:

> This seems the other extreme. How about a comprimize like
> "IMG_VERSION" or "IMAGE_VERSION" or the like, so it's clear what sort
> of version we are talking about, while at the same time allowing to
> reuse this on other architectures if that should be needed one day?

Agree, my choice goes to IMAGE_VERSION ;-)

Best regards,
Stefano Babic

Patch

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
+
 # Boot Device : one of
 # spi, sd (the board has no nand neither onenand)
 
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
+
 # Boot Device : one of
 # spi, nand, onenand, sd
 
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.
+
 	BOOT_FROM		nand/spi/sd/onenand
 				Example:
 				BOOT_FROM spi
@@ -69,8 +74,9 @@  Configuration command line syntax:
 				Example (write to IOMUXC):
 				DATA 4 0x73FA88a0 0x200
 
-The processor support up to 60 register programming commands. An error
-is generated if more commands are found in the configuration file.
+The processor support up to 60 register programming commands for IMXIMAGE_VERSION 1
+and 121 register programming commands for IMXIMAGE_VERSION 2.
+An error is generated if more commands are found in the configuration file.
 
 3. All commands are optional to program.
 
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", },
 	{CMD_BOOT_FROM,		"BOOT_FROM",		"boot command",	},
 	{CMD_DATA,		"DATA",			"Reg Write Data", },
 	{-1,		"",			"",	},
@@ -53,6 +54,14 @@  static table_entry_t imximage_bootops[] = {
 	{-1,			"",		"Invalid",	},
 };
 
+/*
+ * IMXIMAGE version definition for i.MX chips
+ */
+static table_entry_t imximage_versions[] = {
+	{IMXIMAGE_V1,	"",	" (i.MX25/35/51 compatible)", },
+	{IMXIMAGE_V2,	"",	" (i.MX53 compatible)",       },
+	{-1,	        "",     " (Invalid)",                 },
+};
 
 static struct imx_header imximage_header;
 
@@ -71,6 +80,128 @@  static uint32_t get_cfg_value(char *token, char *name,  int linenr)
 	return value;
 }
 
+static void err_imximage_version(int version)
+{
+	fprintf(stderr,
+		"Error: Unsuported imximage version:%d\n", version);
+
+	exit(EXIT_FAILURE);
+}
+
+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;
+	}
+}
+
+static void set_dcd_rest_v1(struct imx_header *imxhdr, uint32_t dcd_len,
+						char *name, int lineno)
+{
+	dcd_v1_t *dcd_v1 = &imxhdr->header.hdr_v1.dcd_table;
+
+	if (dcd_len > MAX_HW_CFG_SIZE_V1) {
+		fprintf(stderr, "Error: %s[%d] -"
+			"DCD table exceeds maximum size(%d)\n",
+			name, lineno, MAX_HW_CFG_SIZE_V1);
+	}
+
+	dcd_v1->preamble.barker = DCD_BARKER;
+	dcd_v1->preamble.length = dcd_len * sizeof(dcd_type_addr_data_t);
+}
+
+static void set_dcd_rest_v2(struct imx_header *imxhdr, uint32_t dcd_len,
+						char *name, int lineno)
+{
+	dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.dcd_table;
+
+	if (dcd_len > MAX_HW_CFG_SIZE_V2) {
+		fprintf(stderr, "Error: %s[%d] -"
+			"DCD table exceeds maximum size(%d)\n",
+			name, lineno, MAX_HW_CFG_SIZE_V2);
+	}
+
+	dcd_v2->header.tag = DCD_HEADER_TAG;
+	dcd_v2->header.length = cpu_to_be16(
+			dcd_len * sizeof(dcd_addr_data_t) + 8);
+	dcd_v2->header.version = DCD_VERSION;
+	dcd_v2->write_dcd_command.tag = DCD_COMMAND_TAG;
+	dcd_v2->write_dcd_command.length = cpu_to_be16(
+			dcd_len * sizeof(dcd_addr_data_t) + 4);
+	dcd_v2->write_dcd_command.param = DCD_COMMAND_PARAM;
+}
+
+static void set_dcd_rest(struct imx_header *imxhdr, uint32_t dcd_len,
+						char *name, int lineno)
+{
+	switch (imxhdr->imximage_version) {
+	case IMXIMAGE_V1:
+		set_dcd_rest_v1(imxhdr, dcd_len, name, lineno);
+		break;
+	case IMXIMAGE_V2:
+		set_dcd_rest_v2(imxhdr, dcd_len, name, lineno);
+		break;
+	default:
+		err_imximage_version(imxhdr->imximage_version);
+		break;
+	}
+}
+
 static int imximage_check_image_types(uint8_t type)
 {
 	if (type == IH_TYPE_IMXIMAGE)
@@ -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;
 
 	/* Only a few checks can be done: search for magic numbers */
-	if (hdr->app_code_barker != APP_CODE_BARKER)
+	if (fhdr->app_code_barker != APP_CODE_BARKER)
 		return -FDT_ERR_BADSTRUCTURE;
 
-	if (imx_hdr->dcd_table.preamble.barker != DCD_BARKER)
+	if (imx_hdr->header.hdr_v1.dcd_table.preamble.barker != DCD_BARKER)
 		return -FDT_ERR_BADSTRUCTURE;
 
 	return 0;
 }
 
-static void imximage_print_header(const void *ptr)
+static void imximage_print_header_v1(struct imx_header *imx_hdr)
 {
-	struct imx_header *imx_hdr = (struct imx_header *) ptr;
-	flash_header_t *hdr = &imx_hdr->fhdr;
-	uint32_t size;
-	uint32_t length;
-	dcd_t *dcd = &imx_hdr->dcd_table;
+	imx_header_v1_t *hdr_v1 = &imx_hdr->header.hdr_v1;
+	flash_header_v1_t *fhdr_v1 = &hdr_v1->fhdr;
+	dcd_v1_t *dcd_v1 = &hdr_v1->dcd_table;
+	uint32_t size, length;
 
-	size = imx_hdr->dcd_table.preamble.length;
-	if (size > (MAX_HW_CFG_SIZE * sizeof(dcd_type_addr_data_t))) {
+	size = dcd_v1->preamble.length;
+	if (size > (MAX_HW_CFG_SIZE_V1 * sizeof(dcd_type_addr_data_t))) {
 		fprintf(stderr,
 			"Error: Image corrupt DCD size %d exceed maximum %d\n",
 			(uint32_t)(size / sizeof(dcd_type_addr_data_t)),
-			MAX_HW_CFG_SIZE);
+			MAX_HW_CFG_SIZE_V1);
 		exit(EXIT_FAILURE);
 	}
 
-	length =  dcd->preamble.length / sizeof(dcd_type_addr_data_t);
+	length = dcd_v1->preamble.length / sizeof(dcd_type_addr_data_t);
+
+	printf("Image Type:   Freescale IMX Boot Image\n");
+	printf("Image Ver:    %x", (uint32_t)imx_hdr->imximage_version);
+	printf("%s\n", get_table_entry_name(imximage_versions,
+				NULL, imx_hdr->imximage_version));
+	printf("Data Size:    ");
+	genimg_print_size(dcd_v1->addr_data[length].type);
+	printf("Load Address: %08x\n", (uint32_t)fhdr_v1->app_dest_ptr);
+	printf("Entry Point:  %08x\n", (uint32_t)fhdr_v1->app_code_jump_vector);
+}
+
+static void imximage_print_header_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;
+
+	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);
+	}
 
 	printf("Image Type:   Freescale IMX Boot Image\n");
+	printf("Image Ver:    %x", (uint32_t)imx_hdr->imximage_version);
+	printf("%s\n", get_table_entry_name(imximage_versions,
+				NULL, imx_hdr->imximage_version));
 	printf("Data Size:    ");
-	genimg_print_size(dcd->addr_data[length].type);
-	printf("Load Address: %08x\n", (unsigned int)hdr->app_dest_ptr);
-	printf("Entry Point:  %08x\n", (unsigned int)hdr->app_code_jump_vector);
+	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 imximage_print_header(const void *ptr)
+{
+	struct imx_header *imx_hdr = (struct imx_header *) ptr;
+
+	switch (imx_hdr->imximage_version) {
+	case IMXIMAGE_V1:
+		imximage_print_header_v1(imx_hdr);
+		break;
+	case IMXIMAGE_V2:
+		imximage_print_header_v2(imx_hdr);
+		break;
+	default:
+		err_imximage_version(imx_hdr->imximage_version);
+		break;
+	}
 }
 
 static uint32_t imximage_parse_cfg_file(struct imx_header *imxhdr, char *name)
@@ -131,7 +309,6 @@  static uint32_t imximage_parse_cfg_file(struct imx_header *imxhdr, char *name)
 	int fld, value;
 	size_t len;
 	int dcd_len = 0;
-	dcd_t *dcd = &imxhdr->dcd_table;
 	int32_t cmd;
 
 	fd = fopen(name, "r");
@@ -176,6 +353,11 @@  static uint32_t imximage_parse_cfg_file(struct imx_header *imxhdr, char *name)
 				break;
 			case CFG_REG_SIZE:
 				switch (cmd) {
+				case CMD_IMXIMAGE_VERSION:
+					imxhdr->imximage_version =
+						get_cfg_value(token,
+							name, lineno);
+					break;
 				case CMD_BOOT_FROM:
 					/* Get flash header offset */
 					imxhdr->flash_offset =
@@ -195,92 +377,131 @@  static uint32_t imximage_parse_cfg_file(struct imx_header *imxhdr, char *name)
 				case CMD_DATA:
 					value = get_cfg_value(token,
 							name, lineno);
-
-					/* 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->addr_data[dcd_len].type = value;
+					set_dcd_value(imxhdr, name, lineno,
+							fld, value, dcd_len);
 					break;
 				}
 
 			case CFG_REG_ADDRESS:
-				if (cmd == CMD_DATA)
-					dcd->addr_data[dcd_len].addr =
-						get_cfg_value(token,
+				if (cmd == CMD_DATA) {
+					value = get_cfg_value(token,
 							name, lineno);
+					set_dcd_value(imxhdr, name, lineno,
+							fld, value, dcd_len);
+				}
 				break;
 			case CFG_REG_VALUE:
 				if (cmd == CMD_DATA) {
-					dcd->addr_data[dcd_len].value =
-						get_cfg_value(token,
+					value = get_cfg_value(token,
 							name, lineno);
+					set_dcd_value(imxhdr, name, lineno,
+							fld, value, dcd_len);
 					dcd_len++;
 				}
 				break;
 			}
 		}
 
-		if (dcd_len > MAX_HW_CFG_SIZE) {
-			fprintf(stderr,
-				"Error: %s[%d] -"
-				"DCD table exceeds maximum size(%d)\n",
-				name, lineno, MAX_HW_CFG_SIZE);
-		}
 	}
-	dcd->preamble.barker = DCD_BARKER;
-	dcd->preamble.length = dcd_len * sizeof(dcd_type_addr_data_t);
-	fclose(fd);
 
+	set_dcd_rest(imxhdr, dcd_len, name, lineno);
+
+	fclose(fd);
 	return dcd_len;
 }
 
-static void imximage_set_header(void *ptr, struct stat *sbuf, int ifd,
-				struct mkimage_params *params)
+static void imximage_set_header_v1(struct imx_header *imxhdr, uint32_t dcd_len,
+					struct stat *sbuf,
+					struct mkimage_params *params)
 {
-	struct imx_header *hdr = (struct imx_header *)ptr;
-	flash_header_t *fhdr = &hdr->fhdr;
-	int dcd_len;
-	dcd_t *dcd = &hdr->dcd_table;
+	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;
 
 	/* Set default offset */
-	hdr->flash_offset = FLASH_OFFSET_STANDARD;
+	imxhdr->flash_offset = FLASH_OFFSET_STANDARD;
 
 	/* Set magic number */
-	fhdr->app_code_barker = APP_CODE_BARKER;
+	fhdr_v1->app_code_barker = APP_CODE_BARKER;
 
-	/* Parse dcd configuration file */
-	dcd_len = imximage_parse_cfg_file(hdr, params->imagename);
-
-	fhdr->app_dest_ptr = params->addr;
-	fhdr->app_dest_ptr = params->ep - hdr->flash_offset -
+	fhdr_v1->app_dest_ptr = params->addr;
+	fhdr_v1->app_dest_ptr = params->ep - imxhdr->flash_offset -
 		sizeof(struct imx_header);
-	fhdr->app_code_jump_vector = params->ep;
+	fhdr_v1->app_code_jump_vector = params->ep;
 
-	base_offset = fhdr->app_dest_ptr + hdr->flash_offset ;
-	fhdr->dcd_ptr_ptr = (uint32_t) (offsetof(flash_header_t, dcd_ptr) -
-		offsetof(flash_header_t, app_code_jump_vector) +
+	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->dcd_ptr = base_offset +
-			offsetof(struct imx_header, dcd_table);
+	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->addr_data[dcd_len].type = sbuf->st_size +
-				hdr->flash_offset +
+	dcd_v1->addr_data[dcd_len].type = sbuf->st_size +
+				imxhdr->flash_offset +
 				sizeof(struct imx_header);
 
 	/* Security feature are not supported */
-	fhdr->app_code_csf = 0;
-	fhdr->super_root_key = 0;
+	fhdr_v1->app_code_csf = 0;
+	fhdr_v1->super_root_key = 0;
+}
+
+static void imximage_set_header_v2(struct imx_header *imxhdr,
+					struct stat *sbuf,
+					struct mkimage_params *params)
+{
+	imx_header_v2_t *hdr_v2 = &imxhdr->header.hdr_v2;
+	flash_header_v2_t *fhdr_v2 = &hdr_v2->fhdr;
+
+	/* Set default offset */
+	imxhdr->flash_offset = FLASH_OFFSET_STANDARD;
+
+	/* 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->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->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);
+
+	/* Security feature are not supported */
+	fhdr_v2->csf = 0;
+}
+static void imximage_set_header(void *ptr, struct stat *sbuf, int ifd,
+				struct mkimage_params *params)
+{
+	struct imx_header *imxhdr = (struct imx_header *)ptr;
+	uint32_t dcd_len;
+
+	/* Parse dcd configuration file */
+	dcd_len = imximage_parse_cfg_file(imxhdr, params->imagename);
+
+	switch (imxhdr->imximage_version) {
+	case IMXIMAGE_V1:
+		imximage_set_header_v1(imxhdr, dcd_len, sbuf, params);
+		break;
+	case IMXIMAGE_V2:
+		imximage_set_header_v2(imxhdr, sbuf, params);
+		break;
+	default:
+		err_imximage_version(imxhdr->imximage_version);
+		break;
+	}
 }
 
 int imximage_check_params(struct mkimage_params *params)
@@ -309,7 +530,7 @@  int imximage_check_params(struct mkimage_params *params)
  * imximage parameters
  */
 static struct image_type_params imximage_params = {
-	.name		= "Freescale i.MX 51 Boot Image support",
+	.name		= "Freescale i.MX 5x Boot Image support",
 	.header_size	= sizeof(struct imx_header),
 	.hdr		= (void *)&imximage_header,
 	.check_image_type = imximage_check_image_types,
diff --git a/tools/imximage.h b/tools/imximage.h
index b4d926d..67b9022 100644
--- a/tools/imximage.h
+++ b/tools/imximage.h
@@ -24,12 +24,14 @@ 
 #ifndef _IMXIMAGE_H_
 #define _IMXIMAGE_H_
 
-#define MAX_HW_CFG_SIZE 60	/* Max number of registers imx can set */
-#define MAX_EXP_SIZE	4
+#include <config.h>
+
+#define MAX_HW_CFG_SIZE_V2 121 /* Max number of registers imx can set for v2 */
+#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
-#define HEADER_OFFSET	0x400
 
+#define HEADER_OFFSET	0x400
 
 #define CMD_DATA_STR	"DATA"
 #define FLASH_OFFSET_STANDARD	0x400
@@ -38,8 +40,16 @@ 
 #define FLASH_OFFSET_SPI	FLASH_OFFSET_STANDARD
 #define FLASH_OFFSET_ONENAND	0x100
 
+#define IVT_HEADER_TAG 0xD1
+#define IVT_VERSION 0x40
+#define DCD_HEADER_TAG 0xD2
+#define DCD_COMMAND_TAG 0xCC
+#define DCD_VERSION 0x40
+#define DCD_COMMAND_PARAM 0x4
+
 enum imximage_cmd {
 	CMD_INVALID,
+	CMD_IMXIMAGE_VERSION,
 	CMD_BOOT_FROM,
 	CMD_DATA
 };
@@ -52,13 +62,10 @@  enum imximage_fld_types {
 	CFG_REG_VALUE
 };
 
-typedef struct {
-	uint8_t rsa_exponent[MAX_EXP_SIZE];	 /* RSA public exponent */
-	uint8_t *rsa_modulus;			 /* RSA modulus pointer */
-	uint16_t exponent_size;			 /* Exponent size (bytes) */
-	uint16_t modulus_size;			 /* Modulus size (bytes) */
-	uint8_t init_flag;			 /* key initialized */
-} hab_rsa_public_key;
+enum imximage_version {
+	IMXIMAGE_V1 = 1,
+	IMXIMAGE_V2
+};
 
 typedef struct {
 	uint32_t type; /* Type of pointer (byte, halfword, word, wait/read) */
@@ -73,8 +80,8 @@  typedef struct {
 
 typedef struct {
 	dcd_preamble_t preamble;
-	dcd_type_addr_data_t addr_data[MAX_HW_CFG_SIZE];
-} dcd_t;
+	dcd_type_addr_data_t addr_data[MAX_HW_CFG_SIZE_V1];
+} dcd_v1_t;
 
 typedef struct {
 	uint32_t app_code_jump_vector;
@@ -84,22 +91,71 @@  typedef struct {
 	uint32_t super_root_key;
 	uint32_t dcd_ptr;
 	uint32_t app_dest_ptr;
-} flash_header_t;
+} flash_header_v1_t;
 
 typedef struct {
 	uint32_t length; 	/* Length of data to be read from flash */
 } flash_cfg_parms_t;
 
-struct imx_header {
-	flash_header_t fhdr;
-	dcd_t dcd_table;
+typedef struct {
+	flash_header_v1_t fhdr;
+	dcd_v1_t dcd_table;
 	flash_cfg_parms_t ext_header;
-	uint32_t flash_offset;
-};
+} imx_header_v1_t;
+
+typedef struct {
+	uint32_t addr;
+	uint32_t value;
+} dcd_addr_data_t;
+
+typedef struct {
+	uint8_t tag;
+	uint16_t length;
+	uint8_t version;
+} __attribute__((packed)) ivt_header_t;
 
-struct reg_config {
-	uint32_t raddr;
-	uint32_t rdata;
+typedef struct {
+	uint8_t tag;
+	uint16_t length;
+	uint8_t param;
+} __attribute__((packed)) write_dcd_command_t;
+
+typedef struct {
+	ivt_header_t header;
+	write_dcd_command_t write_dcd_command;
+	dcd_addr_data_t addr_data[MAX_HW_CFG_SIZE_V2];
+} dcd_v2_t;
+
+typedef struct {
+	uint32_t start;
+	uint32_t size;
+	uint32_t plugin;
+} boot_data_t;
+
+typedef struct {
+	ivt_header_t header;
+	uint32_t entry;
+	uint32_t reserved1;
+	uint32_t dcd_ptr;
+	uint32_t boot_data_ptr;
+	uint32_t self;
+	uint32_t csf;
+	uint32_t reserved2;
+} flash_header_v2_t;
+
+typedef struct {
+	flash_header_v2_t fhdr;
+	boot_data_t boot_data;
+	dcd_v2_t dcd_table;
+} imx_header_v2_t;
+
+struct imx_header {
+	union {
+		imx_header_v1_t hdr_v1;
+		imx_header_v2_t hdr_v2;
+	} header;
+	uint32_t flash_offset;
+	uint32_t imximage_version;
 };
 
 #endif /* _IMXIMAGE_H_ */