[U-Boot] [PATCH V2 01/21] imximage: make header variable length

[Stefano Babic <sbabic@denx.de>]

On 24/09/2012 22:30, Troy Kisky wrote:
> On 9/23/2012 3:57 AM, Stefano Babic wrote:
>> On 22/09/2012 04:38, Troy Kisky wrote:
>>

Hi Troy,
>>> Also, the header offset is no longer
>>> right before the code starts.
>> Comment and subject of the patch do not match. Can you better explain it
>> ? What have "making header variable length", that is, a new feature,
>> with " the header offset is no longer rigth", that is, a bug ?
>>
>> Do we already have a variable header the we add a vrec_header function
>> to  image_type_params ?
> Before this patch we have
> 000000 402000d1 17800000 00000000 177ffc2c
> 000010 177ffc20 177ffc00 00000000 00000000
> 000020 177ff800 00042b58 00000000 402803d2
> 000030 042403cc a8050e02 30000000 b0050e02
> ... more DCD table
> 000340 cf0000f0 18000e02 7f007f00 1c000e02
> 000350 7f007f00 00000000 00000000 00000000
> 000360 00000000 00000000 00000000 00000000
> *
> 0003f0 00000000 00000000 00000000 00000400
> 000400 ea000014 e59ff014 e59ff014 e59ff014
> 
> 
> Notice offset 3fc contains 0x400. This
> is the header offset. There is no reason
> for this to be in the file, and I have
> removed it.

Right - I do not know anymore the reason why it was explicitely set. It
is not useful and not required by the SOC, no reason to have it, agree.

> 
> 
> After this patch we have
> 000000 402000d1 17800000 00000000 177ffcd8
> 000010 177ffccc 177ffcac 00000000 00000000
> 000020 177ff8ac 000426ac 00000000 402803d2
> 000030 042403cc a8050e02 30000000 b0050e02
> ... more DCD table
> 000340 cf0000f0 18000e02 7f007f00 1c000e02
> 000350 7f007f00 ea000014 e59ff014 e59ff014
> 000360 e59ff014 e59ff014 e59ff014 e59ff014
> 
> Notice the zeros between 0x354 and 0x3fb have
> been removed. That is what I mean by making it
> a variable length header.

Ok - I understan that the whole header have a different lenght, but
really it is the DCD table that has now a variable length. Please change
commit header and comment to explain it. Feel free to add this example

> 
>>> Series tested on an mx51 and mx6q
>>> ---
>>>   tools/imximage.c |  142
>>> +++++++++++++++++++++++++++++++-----------------------
>>>   tools/imximage.h |   10 ++--
>>>   2 files changed, 87 insertions(+), 65 deletions(-)
>>>
>>> diff --git a/tools/imximage.c b/tools/imximage.c
>>> index 03a7716..25d3b74 100644
>>> --- a/tools/imximage.c
>>> +++ b/tools/imximage.c
>>> @@ -65,12 +65,15 @@ static table_entry_t imximage_versions[] = {
>>>       {-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 set_imx_size_t set_imx_size;
>>> +static uint32_t g_flash_offset;
>>> +
>>> +static struct image_type_params imximage_params;
>>>     static uint32_t get_cfg_value(char *token, char *name,  int linenr)
>>>   {
>>> @@ -207,85 +210,79 @@ static void set_dcd_rst_v2(struct imx_header
>>> *imxhdr, uint32_t dcd_len,
>>>       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)
>>> +static int set_imx_hdr_v1(struct imx_header *imxhdr, uint32_t dcd_len,
>>> +        uint32_t entry_point, uint32_t flash_offset)
>>>   {
>>>       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;
>>> -
>>> -    /* Exit if there is no BOOT_FROM field specifying the
>>> flash_offset */
>>> -    if(imxhdr->flash_offset == FLASH_OFFSET_UNDEFINED) {
>>> -        fprintf(stderr, "Error: Header v1: No BOOT_FROM tag in %s\n",
>>> -            params->imagename);
>>> -        exit(EXIT_FAILURE);
>>> -    }
>> Do you drop BOOT_FROM ? Then this should be documented. Is this to allow
>> that the same image can be loaded from different media, that share the
>> same flash offset ? Then, instead of drop it, I suggest to add more
>> entries in the imximage file, one for each media that is allow.
> 
> No I did not drop the BOOT_FROM command. I merely moved this check before
> the function call, as it was common to both set_imx_hdr_v1 and
> set_imx_hdr_v2
> 
> I can make it a separate patch to make it obvious.

Yes, please.

>>
>> Something like:
>>     BOOT_FROM    sd, nand, spi
>>
>> and maybe a check in the code if all entries do not share the same start
>> address.
>>
>>> +    uint32_t hdr_base;
>>> +    uint32_t header_length = (((char
>>> *)&dcd_v1->addr_data[dcd_len].addr)
>>> +            - ((char *)imxhdr));
>>>   
>> For V1, the header is preallocated with the maximum size, that is the
>> maximum number of DCD entries the SOC in V1 can support. Why do we need
>> a dynamic length for V1 processors ? As far as I know, the number of
>> entries and fields for theses SOCs (i.MX25, i.MX35, i.MX51) is fixed.
> 
> You right the DCD table maximum size is fixed. But why should we be
> forced to use
> the maximum size? Why make V1 headers a special case?

No, it should not be - I want only be sure that changes are clear
enough. It is absolutely ok that there is no padding after the DCD
table. What I see, you fix several different issues with this single
patch, and this can make confusion. Split into several patches or at
least add a complete and exhaurient commit message.

>>>       /* Set magic number */
>>>       fhdr_v1->app_code_barker = APP_CODE_BARKER;
>>>   -    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;
>>> +    hdr_base = entry_point - header_length;
>>> +    fhdr_v1->app_dest_ptr = hdr_base - flash_offset;
>>> +    fhdr_v1->app_code_jump_vector = entry_point;
>>>   -    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);
>>> +    fhdr_v1->dcd_ptr_ptr = hdr_base + offsetof(flash_header_v1_t,
>>> dcd_ptr);
>>> +    fhdr_v1->dcd_ptr = hdr_base + offsetof(imx_header_v1_t, dcd_table);
>>>         /* Security feature are not supported */
>>>       fhdr_v1->app_code_csf = 0;
>>>       fhdr_v1->super_root_key = 0;
>>> +    return header_length;
>>> +}
>>> +
>> Ok, I skip review of this part - it depends on your answer on the
>> previous question.
>>
>>>   -static void set_imx_hdr_v2(struct imx_header *imxhdr, uint32_t
>>> dcd_len,
>>> -                    struct stat *sbuf,
>>> -                    struct mkimage_params *params)
>>> +static int set_imx_hdr_v2(struct imx_header *imxhdr, uint32_t dcd_len,
>>> +        uint32_t entry_point, uint32_t flash_offset)
>>>   {
>>>       imx_header_v2_t *hdr_v2 = &imxhdr->header.hdr_v2;
>>>       flash_header_v2_t *fhdr_v2 = &hdr_v2->fhdr;
>>> -
>>> -    /* Exit if there is no BOOT_FROM field specifying the
>>> flash_offset */
>>> -    if(imxhdr->flash_offset == FLASH_OFFSET_UNDEFINED) {
>>> -        fprintf(stderr, "Error: Header v2: No BOOT_FROM tag in %s\n",
>>> -            params->imagename);
>>> -        exit(EXIT_FAILURE);
>>> -    }
>>> +    uint32_t hdr_base;
>>> +    uint32_t header_length = (dcd_len) ?
>>> +        (char *)&hdr_v2->dcd_table.addr_data[dcd_len] - ((char*)imxhdr)
>>> +        : offsetof(imx_header_v2_t, dcd_table);
>> So you add a case where there is no DCD table at all. Apart that this
>> van be a use case, but does it happen in the real life ?
> 
> Yes, after this patch series, sabrelite with use a plugin with no DCD
> table at all.
> Even your SPL route could use no DCD table.

Ok - then this is also a bug fix. The SOCs support an empty DCD table,
and the current imximage does not. Add also this fix in the commit
message, or move it into a separate patch if you can.


> 
>>
>>> +        if (g_flash_offset == -1) {
>>>               fprintf(stderr, "Error: %s[%d] -Invalid boot device"
>>>                   "(%s)\n", name, lineno, token);
>>>               exit(EXIT_FAILURE);
>>> @@ -521,12 +520,17 @@ static void imximage_print_header(const void *ptr)
>>>       }
>>>   }
>>>   -static void imximage_set_header(void *ptr, struct stat *sbuf, int
>>> ifd,
>>> -                struct mkimage_params *params)
>>> +int imximage_vrec_header(struct mkimage_params *params,
>>> +        struct image_type_params *tparams)
>>>   {
>>> -    struct imx_header *imxhdr = (struct imx_header *)ptr;
>>> +    struct imx_header *imxhdr;
>>>       uint32_t dcd_len;
>>>   +    imxhdr = calloc(1, MAX_HEADER_SIZE);
>>> +    if (!imxhdr) {
>>> +        fprintf(stderr, "Error: out of memory\n");
>>> +        exit(EXIT_FAILURE);
>>> +    }
>>>       /*
>>>        * In order to not change the old imx cfg file
>>>        * by adding VERSION command into it, here need
>>> @@ -534,14 +538,31 @@ static void imximage_set_header(void *ptr,
>>> struct stat *sbuf, int ifd,
>>>        */
>>>       imximage_version = IMXIMAGE_V1;
>>>       /* Be able to detect if the cfg file has no BOOT_FROM tag */
>>> -    imxhdr->flash_offset = FLASH_OFFSET_UNDEFINED;
>>> +    g_flash_offset = FLASH_OFFSET_UNDEFINED;
>>>       set_hdr_func(imxhdr);
>>>         /* Parse dcd configuration file */
>>>       dcd_len = parse_cfg_file(imxhdr, params->imagename);
>>>   +    /* Exit if there is no BOOT_FROM field specifying the
>>> flash_offset */
>>> +    if (g_flash_offset == FLASH_OFFSET_UNDEFINED) {
>>> +        fprintf(stderr, "Error: No BOOT_FROM tag in %s\n",
>>> +                params->imagename);
>>> +        exit(EXIT_FAILURE);
>>> +    }
>>>       /* Set the imx header */
>>> -    (*set_imx_hdr)(imxhdr, dcd_len, sbuf, params);
>>> +    imximage_params.header_size = (*set_imx_hdr)(imxhdr, dcd_len,
>>> +            params->ep, g_flash_offset);
>>> +    imximage_params.hdr = imxhdr;
>>> +    return 0;
>>> +}
>>> +
>>> +static void imximage_set_header(void *ptr, struct stat *sbuf, int ifd,
>>> +                struct mkimage_params *params)
>>> +{
>>> +    /* Set the size in header */
>>> +    (*set_imx_size)((struct imx_header *)ptr, sbuf->st_size,
>>> +            g_flash_offset);
>>>   }
>>>     int imximage_check_params(struct mkimage_params *params)
>>> @@ -571,8 +592,9 @@ int imximage_check_params(struct mkimage_params
>>> *params)
>>>    */
>>>   static struct image_type_params imximage_params = {
>>>       .name        = "Freescale i.MX 5x Boot Image support",
>>> -    .header_size    = sizeof(struct imx_header),
>>> -    .hdr        = (void *)&imximage_header,
>>> +    .header_size    = 0,
>>> +    .hdr        = NULL,
>>> +    .vrec_header    = imximage_vrec_header,
>>>       .check_image_type = imximage_check_image_types,
>>>       .verify_header    = imximage_verify_header,
>>>       .print_header    = imximage_print_header,
>>> diff --git a/tools/imximage.h b/tools/imximage.h
>>> index 34f293d..5fe3a8a 100644
>>
>>> --- a/tools/imximage.h
>>> +++ b/tools/imximage.h
>>> @@ -30,6 +30,7 @@
>>>   #define DCD_BARKER    0xB17219E9
>>>     #define HEADER_OFFSET    0x400
>>> +#define MAX_HEADER_SIZE    (16 << 10)
>>>     #define CMD_DATA_STR    "DATA"
>>>   #define FLASH_OFFSET_UNDEFINED    0xFFFFFFFF
>>> @@ -156,7 +157,6 @@ struct imx_header {
>>>           imx_header_v1_t hdr_v1;
>>>           imx_header_v2_t hdr_v2;
>>>       } header;
>>> -    uint32_t flash_offset;
>>>   };
>>>     typedef void (*set_dcd_val_t)(struct imx_header *imxhdr,
>>> @@ -168,9 +168,9 @@ 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);
>>> +typedef int (*set_imx_hdr_t)(struct imx_header *imxhdr, uint32_t
>>> dcd_len,
>>> +        uint32_t entry_point, uint32_t flash_offset);
>>> +typedef void (*set_imx_size_t)(struct imx_header *imxhdr, uint32_t
>>> file_size,
>>> +        uint32_t flash_offset);
>> I disagree here. mkimage is valid for all architecture, we must not have
>> special entries here for a SOC or SOC family. For all other SOCs, DCD,
>> iMX header have no sense.
>> Anyway, why do you need to add set_imx_size_t when you call it only in
>> imximage.c ?
> 
> I think you misread the file name. This is imximage.h.

Sorry, my fault here.

> But I will squash the removal of set_imx_size_t done in a later
> patch with this one to make it easier to review.
> 
>>
>>>     #endif /* _IMXIMAGE_H_ */
>> You should split changes in image.h, that are valid for all
>> architecture, from changes to imximage.c, that are only for i.MX, into
>> different patches.
>>
>> In my understanding you add additional entry points to have a variable
>> header lenght, but this feature is already used on TI with the AIS
>> image. You use also vrec_header. What am I missing here ?
> 
> I did not change image.h at all.

That is fine, then.

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