[U-Boot] [PATCH 01/11][v2] imx: imximage: add new CHECK/CLR BIT command

[Stefano Babic <sbabic@denx.de>]

Hi Adrian, hi Peng,

On 16/07/2015 00:49, Adrian Alonso wrote:
> * Extend imximage DCD version 2 to support DCD commands
>   CMD_WRITE_CLR_BIT 4 [address] [mask bit] means:
>     while ((*address & ~mask) != 0);
>   CMD_CHECK_BITS_SET 4 [address] [mask bit] means:
>     while ((*address & mask) != mask);
>   CMD_CHECK_BITS_CLR 4 [address] [mask bit] means:
>     *address = *address & ~mask;
> * Add set_dcd_param_v2 helper function to set DCD
>   command parameters
> 
> Signed-off-by: Adrian Alonso <aalonso@freescale.com>
> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
> ---
> Changes for V2
> - Add set_dcd_param_v2 helper function to set DCD command tag
>   and parameters
> 
>  tools/imximage.c | 99 ++++++++++++++++++++++++++++++++++++++++++++------------
>  tools/imximage.h | 25 ++++++++++----
>  2 files changed, 97 insertions(+), 27 deletions(-)
> 
> diff --git a/tools/imximage.c b/tools/imximage.c
> index 6f469ae..cc0392f 100644
> --- a/tools/imximage.c
> +++ b/tools/imximage.c
> @@ -21,7 +21,10 @@
>  static table_entry_t imximage_cmds[] = {
>  	{CMD_BOOT_FROM,         "BOOT_FROM",            "boot command",	  },
>  	{CMD_BOOT_OFFSET,       "BOOT_OFFSET",          "Boot offset",	  },
> -	{CMD_DATA,              "DATA",                 "Reg Write Data", },
> +	{CMD_WRITE_DATA,        "DATA",                 "Reg Write Data", },
> +	{CMD_WRITE_CLR_BIT,     "CLR_BIT",              "Reg clear bit",  },
> +	{CMD_CHECK_BITS_SET,    "CHECK_BITS_SET",   "Reg Check bits set", },
> +	{CMD_CHECK_BITS_CLR,    "CHECK_BITS_CLR",   "Reg Check bits clr", },
>  	{CMD_CSF,               "CSF",           "Command Sequence File", },
>  	{CMD_IMAGE_VERSION,     "IMAGE_VERSION",        "image version",  },
>  	{-1,                    "",                     "",	          },
> @@ -62,7 +65,7 @@ static table_entry_t imximage_boot_loadsize[] = {
>   */
>  static table_entry_t imximage_versions[] = {
>  	{IMXIMAGE_V1,	"",	" (i.MX25/35/51 compatible)", },
> -	{IMXIMAGE_V2,	"",	" (i.MX53/6 compatible)",     },
> +	{IMXIMAGE_V2,	"",	" (i.MX53/6/7 compatible)",   },
>  	{-1,            "",     " (Invalid)",                 },
>  };
>  
> @@ -79,6 +82,7 @@ static uint32_t imximage_csf_size = UNDEFINED;
>  static uint32_t imximage_init_loadsize;
>  
>  static set_dcd_val_t set_dcd_val;
> +static set_dcd_param_t set_dcd_param;
>  static set_dcd_rst_t set_dcd_rst;
>  static set_imx_hdr_t set_imx_hdr;
>  static uint32_t max_dcd_entries;
> @@ -128,6 +132,12 @@ static void err_imximage_version(int version)
>  	exit(EXIT_FAILURE);
>  }
>  
> +static void set_dcd_param_v1(struct imx_header *imxhdr, uint32_t dcd_len,
> +		int32_t cmd)
> +{
> +	/* DCD V1 no parameter settings */
> +}

It is better you check if you drop this and you check if the pointer is
set to NULL before calling.

> +
>  static void set_dcd_val_v1(struct imx_header *imxhdr, char *name, int lineno,
>  					int fld, uint32_t value, uint32_t off)
>  {
> @@ -156,6 +166,43 @@ static void set_dcd_val_v1(struct imx_header *imxhdr, char *name, int lineno,
>  	}
>  }
>  
> +static void set_dcd_param_v2(struct imx_header *imxhdr, uint32_t dcd_len,
> +		int32_t cmd)
> +{
> +	dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.dcd_table;
> +
> +	switch (cmd) {
> +	case CMD_WRITE_DATA:
> +		dcd_v2->write_dcd_command.tag = DCD_WRITE_DATA_COMMAND_TAG;
> +		dcd_v2->write_dcd_command.length = cpu_to_be16(
> +				dcd_len * sizeof(dcd_addr_data_t) + 4);
> +		dcd_v2->write_dcd_command.param = DCD_WRITE_DATA_PARAM;
> +		break;
> +	case CMD_WRITE_CLR_BIT:
> +		dcd_v2->write_dcd_command.tag = DCD_WRITE_DATA_COMMAND_TAG;
> +		dcd_v2->write_dcd_command.length = cpu_to_be16(
> +				dcd_len * sizeof(dcd_addr_data_t) + 4);
> +		dcd_v2->write_dcd_command.param = DCD_WRITE_CLR_BIT_PARAM;
> +		break;
> +	case CMD_CHECK_BITS_SET:
> +		dcd_v2->write_dcd_command.tag = DCD_CHECK_DATA_COMMAND_TAG;
> +		/*
> +		 * Check data command only supports one entry,
> +		 * so use 0xC = size(address + value + command).
> +		 */

Comment applied to both CHECK_BITS, you could move it before (or put it
also in CMD_CHECK_BITS_CLR case).

> +		dcd_v2->write_dcd_command.length = cpu_to_be16(0xC);
> +		dcd_v2->write_dcd_command.param = DCD_CHECK_BITS_SET_PARAM;
> +		break;
> +	case CMD_CHECK_BITS_CLR:
> +		dcd_v2->write_dcd_command.tag = DCD_CHECK_DATA_COMMAND_TAG;
> +		dcd_v2->write_dcd_command.length = cpu_to_be16(0xC);
> +		dcd_v2->write_dcd_command.param = DCD_CHECK_BITS_SET_PARAM;
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
>  static void set_dcd_val_v2(struct imx_header *imxhdr, char *name, int lineno,
>  					int fld, uint32_t value, uint32_t off)
>  {
> @@ -200,10 +247,7 @@ static void set_dcd_rst_v2(struct imx_header *imxhdr, uint32_t dcd_len,
>  	dcd_v2->header.length = cpu_to_be16(
>  			dcd_len * sizeof(dcd_addr_data_t) + 8);
>  	dcd_v2->header.version = DCD_VERSION;
> -	dcd_v2->write_dcd_command.tag = DCD_COMMAND_TAG;
> -	dcd_v2->write_dcd_command.length = cpu_to_be16(
> -			dcd_len * sizeof(dcd_addr_data_t) + 4);
> -	dcd_v2->write_dcd_command.param = DCD_COMMAND_PARAM;
> +	set_dcd_param_v2(imxhdr, dcd_len, CMD_WRITE_DATA);
>  }
>  
>  static void set_imx_hdr_v1(struct imx_header *imxhdr, uint32_t dcd_len,
> @@ -266,12 +310,14 @@ static void set_hdr_func(void)
>  	switch (imximage_version) {
>  	case IMXIMAGE_V1:
>  		set_dcd_val = set_dcd_val_v1;
> +		set_dcd_param = set_dcd_param_v1;
>  		set_dcd_rst = set_dcd_rst_v1;
>  		set_imx_hdr = set_imx_hdr_v1;
>  		max_dcd_entries = MAX_HW_CFG_SIZE_V1;
>  		break;
>  	case IMXIMAGE_V2:
>  		set_dcd_val = set_dcd_val_v2;
> +		set_dcd_param = set_dcd_param_v2;
>  		set_dcd_rst = set_dcd_rst_v2;
>  		set_imx_hdr = set_imx_hdr_v2;
>  		max_dcd_entries = MAX_HW_CFG_SIZE_V2;
> @@ -396,8 +442,12 @@ static void parse_cfg_cmd(struct imx_header *imxhdr, int32_t cmd, char *token,
>  		if (unlikely(cmd_ver_first != 1))
>  			cmd_ver_first = 0;
>  		break;
> -	case CMD_DATA:
> +	case CMD_WRITE_DATA:
> +	case CMD_WRITE_CLR_BIT:
> +	case CMD_CHECK_BITS_SET:
> +	case CMD_CHECK_BITS_CLR:
>  		value = get_cfg_value(token, name, lineno);
> +		(*set_dcd_param)(imxhdr, dcd_len, cmd);
>  		(*set_dcd_val)(imxhdr, name, lineno, fld, value, dcd_len);
>  		if (unlikely(cmd_ver_first != 1))
>  			cmd_ver_first = 0;
> @@ -436,20 +486,29 @@ static void parse_cfg_fld(struct imx_header *imxhdr, int32_t *cmd,
>  		break;
>  	case CFG_REG_ADDRESS:
>  	case CFG_REG_VALUE:
> -		if (*cmd != CMD_DATA)
> -			return;
> -
> -		value = get_cfg_value(token, name, lineno);
> -		(*set_dcd_val)(imxhdr, name, lineno, fld, value, *dcd_len);
> -
> -		if (fld == CFG_REG_VALUE) {
> -			(*dcd_len)++;
> -			if (*dcd_len > max_dcd_entries) {
> -				fprintf(stderr, "Error: %s[%d] -"
> -					"DCD table exceeds maximum size(%d)\n",
> -					name, lineno, max_dcd_entries);
> -				exit(EXIT_FAILURE);
> +		switch(*cmd) {
> +		case CMD_WRITE_DATA:
> +		case CMD_WRITE_CLR_BIT:
> +		case CMD_CHECK_BITS_SET:
> +		case CMD_CHECK_BITS_CLR:
> +
> +			value = get_cfg_value(token, name, lineno);
> +			(*set_dcd_param)(imxhdr, *dcd_len, *cmd);

What I meant before was:

	if (set_dcd_param)
		(*set_dcd_param)(imxhdr, *dcd_len, *cmd);

and then
  	case IMXIMAGE_V1:
  		set_dcd_val = set_dcd_val_v1;
 +		set_dcd_param = NULL;

> +			(*set_dcd_val)(imxhdr, name, lineno, fld, value,
> +					*dcd_len);
> +
> +			if (fld == CFG_REG_VALUE) {
> +				(*dcd_len)++;
> +				if (*dcd_len > max_dcd_entries) {
> +					fprintf(stderr, "Error: %s[%d] -"
> +						"DCD table exceeds maximum size(%d)\n",
> +						name, lineno, max_dcd_entries);
> +					exit(EXIT_FAILURE);
> +				}
>  			}
> +			break;
> +		default:
> +			break;
>  		}
>  		break;
>  	default:
> diff --git a/tools/imximage.h b/tools/imximage.h
> index 36fe095..c199814 100644
> --- a/tools/imximage.h
> +++ b/tools/imximage.h
> @@ -42,19 +42,27 @@
>  #define FLASH_LOADSIZE_SATA		FLASH_LOADSIZE_STANDARD
>  #define FLASH_LOADSIZE_QSPI		0x0 /* entire image */
>  
> -#define IVT_HEADER_TAG 0xD1
> -#define IVT_VERSION 0x40
> -#define DCD_HEADER_TAG 0xD2
> -#define DCD_COMMAND_TAG 0xCC
> -#define DCD_VERSION 0x40
> -#define DCD_COMMAND_PARAM 0x4
> +/* Command tags and parameters */
> +#define IVT_HEADER_TAG			0xD1
> +#define IVT_VERSION			0x40
> +#define DCD_HEADER_TAG			0xD2
> +#define DCD_VERSION			0x40
> +#define DCD_WRITE_DATA_COMMAND_TAG	0xCC
> +#define DCD_WRITE_DATA_PARAM		0x4
> +#define DCD_WRITE_CLR_BIT_PARAM	0xC
> +#define DCD_CHECK_DATA_COMMAND_TAG	0xCF
> +#define DCD_CHECK_BITS_SET_PARAM	0x14
> +#define DCD_CHECK_BITS_CLR_PARAM	0x04
>  
>  enum imximage_cmd {
>  	CMD_INVALID,
>  	CMD_IMAGE_VERSION,
>  	CMD_BOOT_FROM,
>  	CMD_BOOT_OFFSET,
> -	CMD_DATA,
> +	CMD_WRITE_DATA,
> +	CMD_WRITE_CLR_BIT,
> +	CMD_CHECK_BITS_SET,
> +	CMD_CHECK_BITS_CLR,
>  	CMD_CSF,
>  };
>  

> @@ -167,6 +175,9 @@ typedef void (*set_dcd_val_t)(struct imx_header *imxhdr,
>  					int fld, uint32_t value,
>  					uint32_t off);
>  
> +typedef void (*set_dcd_param_t)(struct imx_header *imxhdr, uint32_t dcd_len,
> +					int32_t cmd);
> +
>  typedef void (*set_dcd_rst_t)(struct imx_header *imxhdr,
>  					uint32_t dcd_len,
>  					char *name, int lineno);
> 

V2 looks like much more easier to understand - apart the very small
comments, it is ok for me.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================