Hi, Wolfgang,
2011/1/18 Wolfgang Denk <w...@denx.de>:
> Dear Jason Liu,
>
> In message <1294129662-680-7-git-send-email-r64...@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 <r64...@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@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
