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: sba...@denx.de
=====================================================================
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
