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

[Wolfgang Denk]

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?

>  	{-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.?

> +	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).


> +				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.


Stefano, Albert - I apologize for the late review.  Please don't pull
this into mainline as is.