[U-Boot] [PATCH v4 7/8] imximage: Add MX53 boot image support

[Jason Liu]

Hi, Wolfgang,

2011/1/18 Wolfgang Denk <wd at denx.de>:
> Dear Jason Liu,
>
> In message <1294129662-680-7-git-send-email-r64343 at freescale.com> you 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>
> ...
>>  static table_entry_t imximage_cmds[] = {
>> +     {CMD_IMAGE_VERSION,     "IMAGE_VERSION",        "image version", },
>>       {CMD_BOOT_FROM,         "BOOT_FROM",            "boot command", },
>>       {CMD_DATA,              "DATA",                 "Reg Write Data", },
>>       {-1,            "",                     "",     },
>
> Can we please keep the table sorted?

what does the "keep the table sorted" mean?

>
>>       {-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;
>> +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 uint32_t get_cfg_value(char *token, char *name,  int linenr)
>>  {
>> @@ -71,58 +85,264 @@ static uint32_t get_cfg_value(char *token, char *name,  int linenr)
>>       return value;
>> +     /* Try to detect V1 */
>> +     if (fhdr_v1->app_code_barker == APP_CODE_BARKER &&
>> +             imx_hdr->header.hdr_v1.dcd_table.preamble.barker == DCD_BARKER)
>> +
>> +             return IMXIMAGE_V1;
>
> This needs braces.  Please fix globally.
>
>> +static void set_dcd_rst_v1(struct imx_header *imxhdr, uint32_t dcd_len,
>> +                                             char *name, int lineno)
>
> Could you please add a comment what the somewhat cryptic name
> "set_dcd_rst_v1" is supposed to mean?  Similar for the other functions
> like set_dcd_rst_v2() etc.?

OK, I will add some comments for this function.

>
>> +     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);
>> +}
>
> It seems these functions can run into error situations - yet they are
> of type void, i. e. they don't return any information about such
> errors to their callers.  In the end, you cannot test the result oif
> running the command to find out if it had worked or not.
>
> This must be fixed (globally).

I think this function does not need return something, I will fix as
following, is it OK,

static void set_dcd_rst_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);
               + exit(EXIT_FAILURE);
        }

        dcd_v1->preamble.barker = DCD_BARKER;
        dcd_v1->preamble.length = dcd_len * sizeof(dcd_type_addr_data_t);
}


>
>
>> +                             case CMD_IMAGE_VERSION:
>> +                                     imximage_version =
>> +                                             get_cfg_value(token,
>> +                                                     name, lineno);
>> +                                     if (cmd_ver_first == 0) {
>> +                                             fprintf(stderr,
>> +                                                     "Error: %s[%d] -  "
>> +                                                     "IMAGE_VERSION command "
>> +                                                     "need appear the first "
>> +                                                     "before other valid "
>> +                                                     "command in the file\n",
>> +                                                     name, lineno);
>> +                                             exit(EXIT_FAILURE);
>> +                                     }
>
> Your nesting is too deep. Consider reorganizing the code.

In fact, I did not change the nesting deep, which is the same deep as
the original code,
I will reorganizing it.

>
>
> Stefano, Albert - I apologize for the late review.  Please don't pull
> this into mainline as is.
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>