| Message ID | 1293626967-18740-1-git-send-email-r64343@freescale.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
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
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 >
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 >> >
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
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
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
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 >
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
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 >
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
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
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
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
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
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
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
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_ */
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(-)