[U-Boot] [PATCH v3 7/8] imximage: Add MX53 boot image support
Jason Liu
liu.h.jason at gmail.com
Fri Dec 31 04:29:56 CET 2010
Hi, Stefano,
2010/12/30 Stefano Babic <sbabic at 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 at 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 at denx.de
> =====================================================================
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
More information about the U-Boot
mailing list