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

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