diff mbox

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

Message ID 1294129662-680-7-git-send-email-r64343@freescale.com
State Accepted
Delegated to: Stefano Babic
Headers show

Commit Message

Liu Hui-R64343 Jan. 4, 2011, 8:27 a.m. UTC
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.

Changes for v3:
- Address the following comments from Stefano,
 - Not change the mx51evk file. The code should take VERSION=1 as default,
   and we do not need to change the actual boards.
 - add a note in the documentation and raise an error in code if the
   VERSION command is read after any other suitable commands.
 - Change command IMXIMAGE_VERSION simply to IMAGE_VERSION
 - Need recognize the version directly from its structure and not storing the
   version into the header when do header verify and print.
 - Use function pointer to simpliy the code when the version of header is recognized
---
 doc/README.imximage |   12 ++-
 tools/imximage.c    |  421 ++++++++++++++++++++++++++++++++++++++++-----------
 tools/imximage.h    |  110 +++++++++++---
 3 files changed, 433 insertions(+), 110 deletions(-)

Comments

Stefano Babic Jan. 12, 2011, 9:38 a.m. UTC | #1
On 01/04/2011 09:27 AM, 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>
> 

Tested on vision2 board, too.

Tested-by: Stefano Babic <sbabic@denx.de>

Best regards,
Stefano Babic
Stefano Babic Jan. 12, 2011, 9:48 a.m. UTC | #2
On 01/04/2011 09:27 AM, 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>
> 
> ---
> 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.
> 
> Changes for v3:
> - Address the following comments from Stefano,
>  - Not change the mx51evk file. The code should take VERSION=1 as default,
>    and we do not need to change the actual boards.
>  - add a note in the documentation and raise an error in code if the
>    VERSION command is read after any other suitable commands.
>  - Change command IMXIMAGE_VERSION simply to IMAGE_VERSION
>  - Need recognize the version directly from its structure and not storing the
>    version into the header when do header verify and print.
>  - Use function pointer to simpliy the code when the version of header is recognized
> ---
>  doc/README.imximage |   12 ++-
>  tools/imximage.c    |  421 ++++++++++++++++++++++++++++++++++++++++-----------
>  tools/imximage.h    |  110 +++++++++++---
>  3 files changed, 433 insertions(+), 110 deletions(-)

Applied to u-boot-imx, thanks.

Best regards,
Stefano Babic
Wolfgang Denk Jan. 17, 2011, 10:54 p.m. UTC | #3
Dear Jason Liu,

In message <1294129662-680-7-git-send-email-r64343@freescale.com> you 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>
...
>  static table_entry_t imximage_cmds[] = {
> +	{CMD_IMAGE_VERSION,     "IMAGE_VERSION",        "image version", },
>  	{CMD_BOOT_FROM,		"BOOT_FROM",		"boot command",	},
>  	{CMD_DATA,		"DATA",			"Reg Write Data", },
>  	{-1,		"",			"",	},

Can we please keep the table sorted?

>  	{-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;
> +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 uint32_t get_cfg_value(char *token, char *name,  int linenr)
>  {
> @@ -71,58 +85,264 @@ static uint32_t get_cfg_value(char *token, char *name,  int linenr)
>  	return value;
> +	/* Try to detect V1 */
> +	if (fhdr_v1->app_code_barker == APP_CODE_BARKER &&
> +		imx_hdr->header.hdr_v1.dcd_table.preamble.barker == DCD_BARKER)
> +
> +		return IMXIMAGE_V1;

This needs braces.  Please fix globally.

> +static void set_dcd_rst_v1(struct imx_header *imxhdr, uint32_t dcd_len,
> +						char *name, int lineno)

Could you please add a comment what the somewhat cryptic name
"set_dcd_rst_v1" is supposed to mean?  Similar for the other functions
like set_dcd_rst_v2() etc.?

> +	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);
> +}

It seems these functions can run into error situations - yet they are
of type void, i. e. they don't return any information about such
errors to their callers.  In the end, you cannot test the result oif
running the command to find out if it had worked or not.

This must be fixed (globally).


> +				case CMD_IMAGE_VERSION:
> +					imximage_version =
> +						get_cfg_value(token,
> +							name, lineno);
> +					if (cmd_ver_first == 0) {
> +						fprintf(stderr,
> +							"Error: %s[%d] -  "
> +							"IMAGE_VERSION command "
> +							"need appear the first "
> +							"before other valid "
> +							"command in the file\n",
> +							name, lineno);
> +						exit(EXIT_FAILURE);
> +					}

Your nesting is too deep. Consider reorganizing the code.


Stefano, Albert - I apologize for the late review.  Please don't pull
this into mainline as is.
Stefano Babic Jan. 18, 2011, 7:12 a.m. UTC | #4
On 01/17/2011 11:54 PM, Wolfgang Denk wrote:
> 
> Stefano, Albert - I apologize for the late review.  Please don't pull
> this into mainline as is.

Wolfgang,

it is too late,  it was already pulled. We need now a patch on top of
this. Jason, can you check Wolfgang's comments and send a patch to fix
the issues he reported ?

Best regards,
Stefano Babic
Wolfgang Denk Jan. 18, 2011, 9:24 a.m. UTC | #5
Dear Stefano Babic,

In message <4D353D72.10208@denx.de> you wrote:
>
> it is too late,  it was already pulled. We need now a patch on top of
> this. Jason, can you check Wolfgang's comments and send a patch to fix
> the issues he reported ?

It is not too late.  I will not pull this into mainline as is.  You
can rebase your tree.

Best regards,

Wolfgang Denk
Stefano Babic Jan. 18, 2011, 10:20 a.m. UTC | #6
On 01/18/2011 10:24 AM, Wolfgang Denk wrote:

> It is not too late.  I will not pull this into mainline as is.  You
> can rebase your tree.

No problem on my side. Albert should also drop this patch, because it is
already merged into u-boot-arm. I will wait for updates from Jason.

Best regards,
Stefano Babic
Jason Liu Jan. 18, 2011, 2:53 p.m. UTC | #7
Hi, Wolfgang and Stefano,

2011/1/18 Wolfgang Denk <wd@denx.de>:
> Dear Stefano Babic,
>
> In message <4D353D72.10208@denx.de> you wrote:
>>
>> it is too late,  it was already pulled. We need now a patch on top of
>> this. Jason, can you check Wolfgang's comments and send a patch to fix
>> the issues he reported ?
>
> It is not too late.  I will not pull this into mainline as is.  You
> can rebase your tree.

Do I need send out the new version patch for imximage to address
Wolfgang's comments
or send out the whole patchset(8patches)  include the new version patch?

What's the process for it?

>
> 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
> "...one of the main causes of the fall of the Roman Empire was  that,
> lacking  zero,  they had no way to indicate successful termination of
> their C programs."                                     - Robert Firth
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Wolfgang Denk Jan. 18, 2011, 2:56 p.m. UTC | #8
Dear Jason Liu,

In message <AANLkTingvggSztUuHQCzrxaOn04ZO=k07xFCZkFciTjP@mail.gmail.com> you wrote:
> 
> Do I need send out the new version patch for imximage to address
> Wolfgang's comments
> or send out the whole patchset(8patches)  include the new version patch?
> 
> What's the process for it?

If the changes affect only this one patch, then you can resubmit only
this single patch.

Best regards,

Wolfgang Denk
Jason Liu Jan. 18, 2011, 2:59 p.m. UTC | #9
Hi, Wolfgang,

2011/1/18 Wolfgang Denk <wd@denx.de>:
> Dear Jason Liu,
>
> In message <AANLkTingvggSztUuHQCzrxaOn04ZO=k07xFCZkFciTjP@mail.gmail.com> you wrote:
>>
>> Do I need send out the new version patch for imximage to address
>> Wolfgang's comments
>> or send out the whole patchset(8patches)  include the new version patch?
>>
>> What's the process for it?
>
> If the changes affect only this one patch, then you can resubmit only
> this single patch.

This is the only one patch from the whole pathset(8patches) need get updated
So, I just resubmit it with one newer version for only this patch, is it ok?

>
> 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
> How many hardware guys does it take to change a light bulb? "Well the
> diagnostics say it's fine buddy, so it's a software problem."
>
Jason Liu Jan. 18, 2011, 3:15 p.m. UTC | #10
Hi, Wolfgang,

2011/1/18 Wolfgang Denk <wd@denx.de>:
> Dear Jason Liu,
>
> In message <1294129662-680-7-git-send-email-r64343@freescale.com> you 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>
> ...
>>  static table_entry_t imximage_cmds[] = {
>> +     {CMD_IMAGE_VERSION,     "IMAGE_VERSION",        "image version", },
>>       {CMD_BOOT_FROM,         "BOOT_FROM",            "boot command", },
>>       {CMD_DATA,              "DATA",                 "Reg Write Data", },
>>       {-1,            "",                     "",     },
>
> Can we please keep the table sorted?

what does the "keep the table sorted" mean?

>
>>       {-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;
>> +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 uint32_t get_cfg_value(char *token, char *name,  int linenr)
>>  {
>> @@ -71,58 +85,264 @@ static uint32_t get_cfg_value(char *token, char *name,  int linenr)
>>       return value;
>> +     /* Try to detect V1 */
>> +     if (fhdr_v1->app_code_barker == APP_CODE_BARKER &&
>> +             imx_hdr->header.hdr_v1.dcd_table.preamble.barker == DCD_BARKER)
>> +
>> +             return IMXIMAGE_V1;
>
> This needs braces.  Please fix globally.
>
>> +static void set_dcd_rst_v1(struct imx_header *imxhdr, uint32_t dcd_len,
>> +                                             char *name, int lineno)
>
> Could you please add a comment what the somewhat cryptic name
> "set_dcd_rst_v1" is supposed to mean?  Similar for the other functions
> like set_dcd_rst_v2() etc.?

OK, I will add some comments for this function.

>
>> +     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);
>> +}
>
> It seems these functions can run into error situations - yet they are
> of type void, i. e. they don't return any information about such
> errors to their callers.  In the end, you cannot test the result oif
> running the command to find out if it had worked or not.
>
> This must be fixed (globally).

I think this function does not need return something, I will fix as
following, is it OK,

static void set_dcd_rst_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);
               + exit(EXIT_FAILURE);
        }

        dcd_v1->preamble.barker = DCD_BARKER;
        dcd_v1->preamble.length = dcd_len * sizeof(dcd_type_addr_data_t);
}


>
>
>> +                             case CMD_IMAGE_VERSION:
>> +                                     imximage_version =
>> +                                             get_cfg_value(token,
>> +                                                     name, lineno);
>> +                                     if (cmd_ver_first == 0) {
>> +                                             fprintf(stderr,
>> +                                                     "Error: %s[%d] -  "
>> +                                                     "IMAGE_VERSION command "
>> +                                                     "need appear the first "
>> +                                                     "before other valid "
>> +                                                     "command in the file\n",
>> +                                                     name, lineno);
>> +                                             exit(EXIT_FAILURE);
>> +                                     }
>
> Your nesting is too deep. Consider reorganizing the code.

In fact, I did not change the nesting deep, which is the same deep as
the original code,
I will reorganizing it.

>
>
> Stefano, Albert - I apologize for the late review.  Please don't pull
> this into mainline as is.
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Stefano Babic Jan. 18, 2011, 3:28 p.m. UTC | #11
Am 18.01.2011 15:59, schrieb Jason Liu:

Hi Jason,

> 
> This is the only one patch from the whole pathset(8patches) need get updated
> So, I just resubmit it with one newer version for only this patch, is it ok?

Yes, I agree. The other patches have received formal ACKs or there are
no more comments for them, and I have integrated in u-boot-imx.

Best regards,
Stefano Babic
Wolfgang Denk Jan. 18, 2011, 7:34 p.m. UTC | #12
Dear Jason Liu,

In message <AANLkTiny2u9cvFwjURnMOeWAzQxLceyQLpW__Dgupx6e@mail.gmail.com> you wrote:
> > ...
> >>  static table_entry_t imximage_cmds[] = {
> >> +     {CMD_IMAGE_VERSION,     "IMAGE_VERSION",        "i> mage version", },
> >>       {CMD_BOOT_FROM,         "BOOT_FROM",        >     "boot command", },
> >>       {CMD_DATA,              "DATA",       >           "Reg Write Data", },
> >>       {-1,            "",               >       "",     },
> >
> > Can we please keep the table sorted?
>
> what does the "keep the table sorted" mean?

Sorted means that the entries should be sorted, and sort order is "B",
"D", "I".

> I think this function does not need return something, I will fix as
> following, is it OK,
>
> static void set_dcd_rst_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);
>                + exit(EXIT_FAILURE);
>         }

That's fine with me.

Best regards,

Wolfgang Denk
Albert ARIBAUD Jan. 20, 2011, 9:28 p.m. UTC | #13
Le 18/01/2011 11:20, Stefano Babic a écrit :
> On 01/18/2011 10:24 AM, Wolfgang Denk wrote:
>
>> It is not too late.  I will not pull this into mainline as is.  You
>> can rebase your tree.
>
> No problem on my side. Albert should also drop this patch, because it is
> already merged into u-boot-arm. I will wait for updates from Jason.

Of course I was late to the dance... Stefano, did you rebase your tree? 
I still see this patch in your current master.

> Best regards,
> Stefano Babic

Amicalement,
Stefano Babic Jan. 21, 2011, 9:16 a.m. UTC | #14
On 01/20/2011 10:28 PM, Albert ARIBAUD wrote:
> Of course I was late to the dance... Stefano, did you rebase your tree?
> I still see this patch in your current master.

Hi Albert,

I have rebased my tree but not pushed on git.denx because I would like
to replace the patch with the new version sent by Jason. However, it
requires probably some more time for the review. I have removed the
patch from the tree, but a couple of other patches were added. I send
you a pull request.

Stefano
diff mbox

Patch

diff --git a/doc/README.imximage b/doc/README.imximage
index 3378f7e..c74239d 100644
--- a/doc/README.imximage
+++ b/doc/README.imximage
@@ -57,6 +57,13 @@  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.
+				This command need appear the fist before
+				other valid commands in configuration file.
+
 	BOOT_FROM		nand/spi/sd/onenand
 				Example:
 				BOOT_FROM spi
@@ -69,8 +76,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..e09dd8c 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_IMAGE_VERSION,     "IMAGE_VERSION",        "image version", },
 	{CMD_BOOT_FROM,		"BOOT_FROM",		"boot command",	},
 	{CMD_DATA,		"DATA",			"Reg Write Data", },
 	{-1,		"",			"",	},
@@ -53,8 +54,21 @@  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;
+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 uint32_t get_cfg_value(char *token, char *name,  int linenr)
 {
@@ -71,58 +85,264 @@  static uint32_t get_cfg_value(char *token, char *name,  int linenr)
 	return value;
 }
 
-static int imximage_check_image_types(uint8_t type)
+static uint32_t detect_imximage_version(struct imx_header *imx_hdr)
 {
-	if (type == IH_TYPE_IMXIMAGE)
-		return EXIT_SUCCESS;
-	else
-		return EXIT_FAILURE;
+	flash_header_v1_t *fhdr_v1 = &imx_hdr->header.hdr_v1.fhdr;
+	flash_header_v2_t *fhdr_v2 = &imx_hdr->header.hdr_v2.fhdr;
+
+	/* Try to detect V1 */
+	if (fhdr_v1->app_code_barker == APP_CODE_BARKER &&
+		imx_hdr->header.hdr_v1.dcd_table.preamble.barker == DCD_BARKER)
+
+		return IMXIMAGE_V1;
+
+	/* Try to detect V2 */
+	if (fhdr_v2->header.tag == IVT_HEADER_TAG &&
+		imx_hdr->header.hdr_v2.dcd_table.header.tag == DCD_HEADER_TAG)
+
+		return IMXIMAGE_V2;
+
+	return IMXIMAGE_VER_INVALID;
 }
 
-static int imximage_verify_header(unsigned char *ptr, int image_size,
-			struct mkimage_params *params)
+static void err_imximage_version(int version)
 {
+	fprintf(stderr,
+		"Error: Unsupported imximage version:%d\n", version);
 
-	struct imx_header *imx_hdr = (struct imx_header *) ptr;
-	flash_header_t *hdr = &imx_hdr->fhdr;
+	exit(EXIT_FAILURE);
+}
 
-	/* Only a few checks can be done: search for magic numbers */
-	if (hdr->app_code_barker != APP_CODE_BARKER)
-		return -FDT_ERR_BADSTRUCTURE;
+static void set_dcd_val_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;
 
-	if (imx_hdr->dcd_table.preamble.barker != DCD_BARKER)
-		return -FDT_ERR_BADSTRUCTURE;
+	}
+}
 
-	return 0;
+static void set_dcd_val_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 imximage_print_header(const void *ptr)
+static void set_dcd_rst_v1(struct imx_header *imxhdr, uint32_t dcd_len,
+						char *name, int lineno)
 {
-	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;
+	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_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;
+
+	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_imx_hdr_v1(struct imx_header *imxhdr, uint32_t dcd_len,
+					struct stat *sbuf,
+					struct mkimage_params *params)
+{
+	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 */
+	imxhdr->flash_offset = FLASH_OFFSET_STANDARD;
+
+	/* Set magic number */
+	fhdr_v1->app_code_barker = APP_CODE_BARKER;
 
-	size = imx_hdr->dcd_table.preamble.length;
-	if (size > (MAX_HW_CFG_SIZE * sizeof(dcd_type_addr_data_t))) {
+	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;
+
+	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);
+
+	/* Security feature are not supported */
+	fhdr_v1->app_code_csf = 0;
+	fhdr_v1->super_root_key = 0;
+}
+
+static void set_imx_hdr_v2(struct imx_header *imxhdr, uint32_t dcd_len,
+					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 set_hdr_func(struct imx_header *imxhdr)
+{
+	switch (imximage_version) {
+	case IMXIMAGE_V1:
+		set_dcd_val = set_dcd_val_v1;
+		set_dcd_rst = set_dcd_rst_v1;
+		set_imx_hdr = set_imx_hdr_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;
+		break;
+	default:
+		err_imximage_version(imximage_version);
+		break;
+	}
+}
+
+static void print_hdr_v1(struct imx_header *imx_hdr)
+{
+	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, ver;
+
+	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);
+	ver = detect_imximage_version(imx_hdr);
 
 	printf("Image Type:   Freescale IMX Boot Image\n");
+	printf("Image Ver:    %x", ver);
+	printf("%s\n", get_table_entry_name(imximage_versions, NULL, ver));
 	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(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 uint32_t imximage_parse_cfg_file(struct imx_header *imxhdr, char *name)
+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;
+
+	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);
+
+	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);
+}
+
+static uint32_t parse_cfg_file(struct imx_header *imxhdr, char *name)
 {
 	FILE *fd = NULL;
 	char *line = NULL;
@@ -131,8 +351,8 @@  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;
+	int cmd_ver_first = ~0;
 
 	fd = fopen(name, "r");
 	if (fd == 0) {
@@ -176,6 +396,23 @@  static uint32_t imximage_parse_cfg_file(struct imx_header *imxhdr, char *name)
 				break;
 			case CFG_REG_SIZE:
 				switch (cmd) {
+				case CMD_IMAGE_VERSION:
+					imximage_version =
+						get_cfg_value(token,
+							name, lineno);
+					if (cmd_ver_first == 0) {
+						fprintf(stderr,
+							"Error: %s[%d] -  "
+							"IMAGE_VERSION command "
+							"need appear the first "
+							"before other valid "
+							"command in the file\n",
+							name, lineno);
+						exit(EXIT_FAILURE);
+					}
+					cmd_ver_first = 1;
+					set_hdr_func(imxhdr);
+					break;
 				case CMD_BOOT_FROM:
 					/* Get flash header offset */
 					imxhdr->flash_offset =
@@ -191,96 +428,104 @@  static uint32_t imximage_parse_cfg_file(struct imx_header *imxhdr, char *name)
 							name, lineno, token);
 						exit(EXIT_FAILURE);
 					}
+					if (unlikely(cmd_ver_first != 1))
+						cmd_ver_first = 0;
 					break;
 				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_val)(imxhdr, name, lineno,
+							fld, value, dcd_len);
+					if (unlikely(cmd_ver_first != 1))
+						cmd_ver_first = 0;
 					break;
 				}
-
+				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_val)(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_val)(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);
+
+	(*set_dcd_rst)(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)
-{
-	struct imx_header *hdr = (struct imx_header *)ptr;
-	flash_header_t *fhdr = &hdr->fhdr;
-	int dcd_len;
-	dcd_t *dcd = &hdr->dcd_table;
-	uint32_t base_offset;
 
-	/* Set default offset */
-	hdr->flash_offset = FLASH_OFFSET_STANDARD;
+static int imximage_check_image_types(uint8_t type)
+{
+	if (type == IH_TYPE_IMXIMAGE)
+		return EXIT_SUCCESS;
+	else
+		return EXIT_FAILURE;
+}
 
-	/* Set magic number */
-	fhdr->app_code_barker = APP_CODE_BARKER;
+static int imximage_verify_header(unsigned char *ptr, int image_size,
+			struct mkimage_params *params)
+{
+	struct imx_header *imx_hdr = (struct imx_header *) ptr;
 
-	/* Parse dcd configuration file */
-	dcd_len = imximage_parse_cfg_file(hdr, params->imagename);
+	if (detect_imximage_version(imx_hdr) == IMXIMAGE_VER_INVALID)
+		return -FDT_ERR_BADSTRUCTURE;
 
-	fhdr->app_dest_ptr = params->addr;
-	fhdr->app_dest_ptr = params->ep - hdr->flash_offset -
-		sizeof(struct imx_header);
-	fhdr->app_code_jump_vector = params->ep;
+	return 0;
+}
 
-	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);
+static void imximage_print_header(const void *ptr)
+{
+	struct imx_header *imx_hdr = (struct imx_header *) ptr;
+	uint32_t version = detect_imximage_version(imx_hdr);
+
+	switch (version) {
+	case IMXIMAGE_V1:
+		print_hdr_v1(imx_hdr);
+		break;
+	case IMXIMAGE_V2:
+		print_hdr_v2(imx_hdr);
+		break;
+	default:
+		err_imximage_version(version);
+		break;
+	}
+}
 
-	fhdr->dcd_ptr = base_offset +
-			offsetof(struct imx_header, dcd_table);
+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;
 
-	/* 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 +
-				sizeof(struct imx_header);
+	/*
+	 * In order to not change the old imx cfg file
+	 * by adding VERSION command into it, here need
+	 * set up function ptr group to V1 by default.
+	 */
+	imximage_version = IMXIMAGE_V1;
+	set_hdr_func(imxhdr);
 
-	/* Security feature are not supported */
-	fhdr->app_code_csf = 0;
-	fhdr->super_root_key = 0;
+	/* Parse dcd configuration file */
+	dcd_len = parse_cfg_file(imxhdr, params->imagename);
 
+	/* Set the imx header */
+	(*set_imx_hdr)(imxhdr, dcd_len, sbuf, params);
 }
 
 int imximage_check_params(struct mkimage_params *params)
@@ -309,7 +554,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..38ca6be 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_IMAGE_VERSION,
 	CMD_BOOT_FROM,
 	CMD_DATA
 };
@@ -52,13 +62,11 @@  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_VER_INVALID = -1,
+	IMXIMAGE_V1 = 1,
+	IMXIMAGE_V2
+};
 
 typedef struct {
 	uint32_t type; /* Type of pointer (byte, halfword, word, wait/read) */
@@ -73,8 +81,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 +92,84 @@  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;
+} 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;
+
+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;
 };
 
-struct reg_config {
-	uint32_t raddr;
-	uint32_t rdata;
-};
+typedef void (*set_dcd_val_t)(struct imx_header *imxhdr,
+					char *name, int lineno,
+					int fld, uint32_t value,
+					uint32_t off);
+
+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);
 
 #endif /* _IMXIMAGE_H_ */