[U-Boot] [PATCH V2 01/21] imximage: make header variable length
Troy Kisky
troy.kisky at boundarydevices.com
Mon Sep 24 22:30:26 CEST 2012
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.
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.
>> 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.
>
> 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?
>
>> /* 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.
>
>> /* 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->entry = entry_point;
>> 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->self = hdr_base = entry_point - header_length;
>>
>> - 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);
>> + fhdr_v2->dcd_ptr = (dcd_len) ? hdr_base
>> + + offsetof(imx_header_v2_t, dcd_table) : 0;
>> + fhdr_v2->boot_data_ptr = hdr_base
>> + + offsetof(imx_header_v2_t, boot_data);
>> + hdr_v2->boot_data.start = hdr_base - flash_offset;
>>
>> /* Security feature are not supported */
>> fhdr_v2->csf = 0;
>> + return header_length;
>> +}
>> +
>> +static void set_imx_size_v2(struct imx_header *imxhdr, uint32_t file_size,
>> + uint32_t flash_offset)
>> +{
>> + imx_header_v2_t *hdr_v2 = &imxhdr->header.hdr_v2;
>> + /* file_size includes header */
>> + hdr_v2->boot_data.size = file_size + flash_offset;
>> }
>>
>> static void set_hdr_func(struct imx_header *imxhdr)
>> @@ -295,11 +292,13 @@ static void set_hdr_func(struct imx_header *imxhdr)
>> set_dcd_val = set_dcd_val_v1;
>> set_dcd_rst = set_dcd_rst_v1;
>> set_imx_hdr = set_imx_hdr_v1;
>> + set_imx_size = set_imx_size_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;
>> + set_imx_size = set_imx_size_v2;
>> break;
>> default:
>> err_imximage_version(imximage_version);
>> @@ -381,9 +380,9 @@ static void parse_cfg_cmd(struct imx_header *imxhdr, int32_t cmd, char *token,
>> set_hdr_func(imxhdr);
>> break;
>> case CMD_BOOT_FROM:
>> - imxhdr->flash_offset = get_table_entry_id(imximage_bootops,
>> + g_flash_offset = get_table_entry_id(imximage_bootops,
>> "imximage boot option", token);
>> - if (imxhdr->flash_offset == -1) {
> Why do we need a global when we have already a way to not use a global ?
Because that flash_offset is unnecessarily taking up space in the output
file, and with
variable length headers, I might overwrite it with other data.
>
>> + 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.
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.
>
>
> Best regards,
> Stefano Babic
>
More information about the U-Boot
mailing list