imx: imximage: add new CHECK/CLR BIT command

Since rom code supports the following commands, add new commands support in imximage.

1. CHECK_BITS_SET 4 [address] [mask bit]
means:

   while ((*address & mask) != mask);

2. CHECK_BITS_CLR 4 [address] [mask bit]
means:

   while ((*address & ~mask) != 0);

2. CLR_BIT 4 [address] [mask bit]
means:

   *address = *address & ~mask;

dcd_v2_t is redefined, because there may not be only one write data command, there may be many different commands like CHECK_BITS_SET, CHECK_BITS_CLR and CLR_BIT.

dcd_len is still leaved there, since changing it needs changes for dcd v1. For v2, we check whether dcd size is larger than MAX_DCD_SIZE_V2, but not dcd_len.

Signed-off-by Peng Fan <[email protected]>

Quoted 19 lines · Peng Fan

Since rom code supports the following commands, add new commands support in imximage.

1. CHECK_BITS_SET 4 [address] [mask bit]
means:

   while ((*address & mask) != mask);

2. CHECK_BITS_CLR 4 [address] [mask bit]
means:

   while ((*address & ~mask) != 0);

2. CLR_BIT 4 [address] [mask bit]
means:

   *address = *address & ~mask;

dcd_v2_t is redefined, because there may not be only one write data command, there may be many different commands like CHECK_BITS_SET, CHECK_BITS_CLR and CLR_BIT.

Quoted 5 lines

dcd_len is still leaved there, since changing it needs changes for dcd v1. For v2, we check whether dcd size is larger than MAX_DCD_SIZE_V2, but not dcd_len.

Signed-off-by Peng Fan <[email protected]>

What about just using SPL mechanism instead?

Quoted 1 line

What about just using SPL mechanism instead?

While I agree we ought to use SPL as much as possible I also believe the DCD support should be as complete as possible; some people might have reasons to avoid the SPL and rely on DCD for it.

Do you believe SPL can be assumed to be a viable option for everyone?

While I agree we ought to use SPL as much as possible I also believe the DCD support should be as complete as possible; some people might have reasons to avoid the SPL and rely on DCD for it.

Do you believe SPL can be assumed to be a viable option for everyone?

Quoted 5 lines · Otavio Salvador

While I agree we ought to use SPL as much as possible I also believe the DCD support should be as complete as possible; some people might have reasons to avoid the SPL and rely on DCD for it.

Do you believe SPL can be assumed to be a viable option for everyone?

What about boards that don't use (and don't need) SPL right now?

Hi Peng,

Quoted 19 lines · Peng Fan

Since rom code supports the following commands, add new commands support in imximage.

1. CHECK_BITS_SET 4 [address] [mask bit]
means:

   while ((*address & mask) != mask);

2. CHECK_BITS_CLR 4 [address] [mask bit]
means:

   while ((*address & ~mask) != 0);

2. CLR_BIT 4 [address] [mask bit]
means:

   *address = *address & ~mask;

dcd_v2_t is redefined, because there may not be only one write data command, there may be many different commands like CHECK_BITS_SET, CHECK_BITS_CLR and CLR_BIT.

Quoted 5 lines

dcd_len is still leaved there, since changing it needs changes for dcd v1. For v2, we check whether dcd size is larger than MAX_DCD_SIZE_V2, but not dcd_len.

Signed-off-by Peng Fan <[email protected]>

What about just using SPL mechanism instead?

Hi Peng,

Quoted 29 lines · Fabio Estevam

Hi Peng,

Quoted 19 lines · Peng Fan

Since rom code supports the following commands, add new commands support in imximage.

1. CHECK_BITS_SET 4 [address] [mask bit]
means:

   while ((*address & mask) != mask);

2. CHECK_BITS_CLR 4 [address] [mask bit]
means:

   while ((*address & ~mask) != 0);

2. CLR_BIT 4 [address] [mask bit]
means:

   *address = *address & ~mask;

dcd_v2_t is redefined, because there may not be only one write data command, there may be many different commands like CHECK_BITS_SET, CHECK_BITS_CLR and CLR_BIT.

Quoted 5 lines

dcd_len is still leaved there, since changing it needs changes for dcd v1. For v2, we check whether dcd size is larger than MAX_DCD_SIZE_V2, but not dcd_len.

Signed-off-by Peng Fan <[email protected]>

What about just using SPL mechanism instead?

Thanks, Fabio. I can only join to your proposal.

Best regards,
Stefano Babic

Signature · 5 lines

=====================================================================
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
=====================================================================

Quoted 2 lines

Quoted 1 line

What about just using SPL mechanism instead?

Hi Fabio,
I agree that SPL can give many benifits, but we default not support it. CHECK_BITS_SET/CLR is supported by i.MX6, but current no user use this feature. This patch is actually for i.MX7 which needs these command. Since this feature is also supported by i.MX6, I make it a single patch, but not with i.MX7 patch set.
As Otavio said, we should make it complete, since not everyone use SPL.

Quoted 2 lines

Thanks, Fabio. I can only join to your proposal.

Hi Stefano,
I hope you can review this patch.

Since rom code supports the following commands, add new commands support in imximage.

1. CHECK_BITS_SET 4 [address] [mask bit]
means:

   while ((*address & mask) != mask);

2. CHECK_BITS_CLR 4 [address] [mask bit]
means:

   while ((*address & ~mask) != 0);

2. CLR_BIT 4 [address] [mask bit]
means:

   *address = *address & ~mask;

dcd_v2_t is redefined, because there may not be only one write data command, there may be many different commands like CHECK_BITS_SET, CHECK_BITS_CLR and CLR_BIT.

dcd_len is still leaved there, since changing it needs changes for dcd v1. For v2, we check whether dcd size is larger than MAX_DCD_SIZE_V2, but not dcd_len.

Signed-off-by Peng Fan <[email protected]>

---
 tools/imximage.c | 129 ++++++++++++++++++++++++++++++++++++++++++-------------
 tools/imximage.h |  24 +++++++++--
 2 files changed, 119 insertions(+), 34 deletions(-)

tools/imximage.c +0 -0

index 6f469ae..1c0225d 100644

tools/imximage.c +1 -0

 	{CMD_CSF,               "CSF",           "Command Sequence File", },
 	{CMD_IMAGE_VERSION,     "IMAGE_VERSION",        "image version",  },
 	{-1,                    "",                     "",	          },

@@ -84,6 +87,7 @@ static set_imx_hdr_t set_imx_hdr;
 static uint32_t max_dcd_entries;
 static uint32_t *header_size_ptr;
 static uint32_t *csf_ptr;
+static uint32_t dataindex;
 
 static uint32_t get_cfg_value(char *token, char *name,  int linenr)
 {

tools/imximage.c +0 -0

 	dcd_v1_t *dcd_v1 = &imxhdr->header.hdr_v1.dcd_table;
 

@@ -157,16 +161,47 @@ static void set_dcd_val_v1(struct imx_header *imxhdr, char *name, int lineno,
 }
 

tools/imximage.c +33 -2

+		/* update header */
+		if (cmd == CMD_DATA) {
+			dcd_command_block->tag = DCD_WRITE_DATA_COMMAND_TAG;
+			dcd_command_block->length = cpu_to_be16(off *
+					sizeof(dcd_addr_data_t) + 4);
+			dcd_command_block->param = DCD_WRITE_DATA_PARAM;
+		} else if (cmd == CMD_CLR_BIT) {
+			dcd_command_block->tag = DCD_WRITE_DATA_COMMAND_TAG;
+			dcd_command_block->length = cpu_to_be16(off *
+					sizeof(dcd_addr_data_t) + 4);
+			dcd_command_block->param = DCD_CLR_BIT_PARAM;
+		} else if (cmd == CMD_CHECK_BITS_SET) {
+			dcd_command_block->tag = DCD_CHECK_DATA_COMMAND_TAG;
+			/*
+			 * check data command only supports one entry,
+			 * so use 0xC = size(address + value + command).
+			 */
+			dcd_command_block->length = cpu_to_be16(0xC);
+			dcd_command_block->param = DCD_CHECK_BITS_SET_PARAM;
+		} else if (cmd == CMD_CHECK_BITS_CLR) {
+			dcd_command_block->tag = DCD_CHECK_DATA_COMMAND_TAG;
+			/*
+			 * check data command only supports one entry,
+			 * so use 0xC = size(address + value + command).
+			 */
+			dcd_command_block->length = cpu_to_be16(0xC);
+			dcd_command_block->param = DCD_CHECK_BITS_CLR_PARAM;
+		}
 	case CFG_REG_ADDRESS:
-		dcd_v2->addr_data[off].addr = cpu_to_be32(value);
+		dcd_command_block->addr_data[off].addr = cpu_to_be32(value);
 		break;
 	case CFG_REG_VALUE:
-		dcd_v2->addr_data[off].value = cpu_to_be32(value);
+		dcd_command_block->addr_data[off].value = cpu_to_be32(value);
 		break;
 	default:
 		break;

@@ -197,13 +232,14 @@ static void set_dcd_rst_v2(struct imx_header *imxhdr, uint32_t dcd_len,
 	dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.dcd_table;
 
 	dcd_v2->header.tag = DCD_HEADER_TAG;
+	/*
+	 * dataindex does not contain the last dcd block,
+	 * see how dataindex is updated.

tools/imximage.c +16 -6

 }
 
 static void set_imx_hdr_v1(struct imx_header *imxhdr, uint32_t dcd_len,

@@ -317,12 +353,11 @@ static void print_hdr_v2(struct imx_header *imx_hdr)
 	dcd_v2_t *dcd_v2 = &hdr_v2->dcd_table;
 	uint32_t size, version;
 
-	size = be16_to_cpu(dcd_v2->header.length) - 8;
-	if (size > (MAX_HW_CFG_SIZE_V2 * sizeof(dcd_addr_data_t))) {
+	size = be16_to_cpu(dcd_v2->header.length);
+	if (size > MAX_DCD_SIZE_V2) {
 		fprintf(stderr,
 			"Error: Image corrupt DCD size %d exceed maximum %d\n",
-			(uint32_t)(size / sizeof(dcd_addr_data_t)),
-			MAX_HW_CFG_SIZE_V2);
+			size, MAX_DCD_SIZE_V2);
 		exit(EXIT_FAILURE);
 	}
 

@@ -398,7 +433,7 @@ static void parse_cfg_cmd(struct imx_header *imxhdr, int32_t cmd, char *token,
 		break;
 	case CMD_DATA:
 		value = get_cfg_value(token, name, lineno);
-		(*set_dcd_val)(imxhdr, name, lineno, fld, value, dcd_len);
+		(*set_dcd_val)(imxhdr, name, lineno, fld, cmd, value, dcd_len);
 		if (unlikely(cmd_ver_first != 1))
 			cmd_ver_first = 0;
 		break;

@@ -417,7 +452,8 @@ static void parse_cfg_cmd(struct imx_header *imxhdr, int32_t cmd, char *token,
 }
 
 static void parse_cfg_fld(struct imx_header *imxhdr, int32_t *cmd,
-		char *token, char *name, int lineno, int fld, int *dcd_len)
+			  int32_t *precmd, char *token, char *name,
+			  int lineno, int fld, int *dcd_len)
 {
 	int value;
 

@@ -430,26 +466,56 @@ static void parse_cfg_fld(struct imx_header *imxhdr, int32_t *cmd,
 			"(%s)\n", name, lineno, token);
 			exit(EXIT_FAILURE);
 		}
+
+		if ((*precmd == CMD_DATA) || (*precmd == CMD_CLR_BIT) ||
+		    (*precmd == CMD_CHECK_BITS_SET) ||
+		    (*precmd == CMD_CHECK_BITS_CLR)) {
+			if (*cmd != *precmd) {
+				dataindex += ((*dcd_len) *
+					      sizeof(dcd_addr_data_t) + 4) >> 2;
+				*dcd_len = 0;
+			}
+		}

tools/imximage.c +27 -16

+		}
+
+		*precmd = *cmd;
+
 		break;
 	case CFG_REG_SIZE:
 		parse_cfg_cmd(imxhdr, *cmd, token, name, lineno, fld, *dcd_len);
 		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_CHECK_BITS_SET:
+		case CMD_CHECK_BITS_CLR:
+		case CMD_DATA:
+		case CMD_CLR_BIT:
+			value = get_cfg_value(token, name, lineno);
+			(*set_dcd_val)(imxhdr, name, lineno, fld, *cmd, 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:
+			return;
 		}
 		break;
 	default:

@@ -465,7 +531,7 @@ static uint32_t parse_cfg_file(struct imx_header *imxhdr, char *name)
 	int fld;
 	size_t len;
 	int dcd_len = 0;
-	int32_t cmd;
+	int32_t cmd, precmd = CMD_INVALID;
 
 	fd = fopen(name, "r");
 	if (fd == 0) {

@@ -473,6 +539,7 @@ static uint32_t parse_cfg_file(struct imx_header *imxhdr, char *name)
 		exit(EXIT_FAILURE);
 	}
 
+	dataindex = 0;
 	/*
 	 * Very simple parsing, line starting with # are comments
 	 * and are dropped

@@ -495,8 +562,8 @@ static uint32_t parse_cfg_file(struct imx_header *imxhdr, char *name)
 			if (token[0] == '#')
 				break;
 
-			parse_cfg_fld(imxhdr, &cmd, token, name,
-					lineno, fld, &dcd_len);
+			parse_cfg_fld(imxhdr, &cmd, &precmd, token, name,
+				      lineno, fld, &dcd_len);
 		}
 
 	}

tools/imximage.h +20 -2

index 36fe095..2ac5bbd 100644
@@ -8,6 +8,8 @@
 #ifndef _IMXIMAGE_H_
 #define _IMXIMAGE_H_
 
+#include <linux/sizes.h>
+#define MAX_DCD_SIZE_V2 1768 /* The DCD size limited to 1768 bytes for v2 */
 #define MAX_HW_CFG_SIZE_V2 220 /* Max number of registers imx can set for v2 */
 #define MAX_HW_CFG_SIZE_V1 60  /* Max number of registers imx can set for v1 */
 #define APP_CODE_BARKER	0xB1

@@ -49,12 +51,22 @@
 #define DCD_VERSION 0x40
 #define DCD_COMMAND_PARAM 0x4
 
+#define DCD_WRITE_DATA_COMMAND_TAG	0xCC
+#define DCD_WRITE_DATA_PARAM		0x4
+#define DCD_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_CLR_BIT,
+	CMD_CHECK_BITS_SET,
+	CMD_CHECK_BITS_CLR,
 	CMD_CSF,
 };
 

@@ -126,9 +138,15 @@ typedef struct {
 } __attribute__((packed)) write_dcd_command_t;
 
 typedef struct {
+	uint8_t tag;
+	uint16_t length;
+	uint8_t param;
+	dcd_addr_data_t addr_data[0];
+} __attribute__((packed)) dcd_command_t;
+
+typedef struct {
 	ivt_header_t header;
-	write_dcd_command_t write_dcd_command;
-	dcd_addr_data_t addr_data[MAX_HW_CFG_SIZE_V2];
+	uint8_t dcd_data[MAX_DCD_SIZE_V2 - sizeof(ivt_header_t)];
 } dcd_v2_t;
 
 typedef struct {

Hi Peng,

Quoted 3 lines · Peng Fan

Since rom code supports the following commands, add new commands support in imximage.

It is better to explain here which i.MX are supporting this ROM (i.MX6 and i.MX7).

Quoted 6 lines

1. CHECK_BITS_SET 4 [address] [mask bit]
means:

   while ((*address & mask) != mask);

Quoted 7 lines

2. CHECK_BITS_CLR 4 [address] [mask bit]
means:

   while ((*address & ~mask) != 0);

2. CLR_BIT 4 [address] [mask bit]
means:

   *address = *address & ~mask;

I understand that the command to be added is CHECK_DATA_COMMAND, as reported by manual. The TAG for the command is the same (0xCF), that means we have a single command with different parameters.
It is better to follow the same approach in the code, because it is easy to find the relatd documentation in manual. In this patch, it looks like we have different commands, but this is not true: there is one command with different parameters.

Quoted 5 lines

dcd_v2_t is redefined, because there may not be only one write data command, there may be many different commands like CHECK_BITS_SET, CHECK_BITS_CLR and CLR_BIT.

I disagree or maybe this is related to i.MX7, where I could not check the documentation. Please explain here: I see only one command (CHECK_DATA_COMMAND, Chapter 8.7.2.2 in i.MX6Q).

Quoted 5 lines

dcd_len is still leaved there, since changing it needs changes for dcd v1. For v2, we check whether dcd size is larger than MAX_DCD_SIZE_V2, but not dcd_len.

It is just a bit confusing, and these are details in implementation - they are not related to the commit where you explain the new feature. Move these comments inside the code where they belong to.

Quoted 17 lines

Signed-off-by Peng Fan <[email protected]>

---
 tools/imximage.c | 129 ++++++++++++++++++++++++++++++++++++++++++-------------
 tools/imximage.h |  24 +++++++++--
 2 files changed, 119 insertions(+), 34 deletions(-)

tools/imximage.c +1 -0

index 6f469ae..1c0225d 100644
@@ -22,6 +22,9 @@ 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_CLR_BIT,           "CLR_BIT",              "Reg clear bit",  },

tools/imximage.c static table_entry_t imximage_cmds[] = { · line 27

Quoted 2 lines

tools/imximage.c +2 -0

+	{CMD_CHECK_BITS_SET,    "CHECK_BITS_SET",       "Reg Check bits set", },
+	{CMD_CHECK_BITS_CLR,    "CHECK_BITS_CLR",       "Reg Check bits clr", },

The table reflects Freescale's documentation. This has the big advantage because there is no need to explain which command is supposed to do because this is really well done by Freescale in the manuals. Here we should have only an additional entry due to CHECK_DATA_COMMAND, exactly what the SOC is able to do.

Quoted 13 lines

tools/imximage.c +1 -0

 	{CMD_CSF,               "CSF",           "Command Sequence File", },
 	{CMD_IMAGE_VERSION,     "IMAGE_VERSION",        "image version",  },
 	{-1,                    "",                     "",	          },

@@ -84,6 +87,7 @@ static set_imx_hdr_t set_imx_hdr;
 static uint32_t max_dcd_entries;
 static uint32_t *header_size_ptr;
 static uint32_t *csf_ptr;
+static uint32_t dataindex;
 
 static uint32_t get_cfg_value(char *token, char *name,  int linenr)
 {

tools/imximage.c static void err_imximage_version(int version) · line 137

Quoted 6 lines

tools/imximage.c +1 -1

@@ -129,7 +133,7 @@ static void err_imximage_version(int version)
 }
 
 static void set_dcd_val_v1(struct imx_header *imxhdr, char *name, int lineno,
-					int fld, uint32_t value, uint32_t off)
+			   int fld, int cmd, uint32_t value, uint32_t off)
 {

Parameter list is becoming very long compared to first implementation.

Let's see what we can do. We have function pointers to override a specific behaviour for a SOC (V1 or V2). The set_dcd_val_vX corresponds to set an entry in the DCD, but it was thought as WRITE_DATA_COMMAND.

Instead of trying to extend this function, that has a different goal, it should be better to add a function pointer for CHECK_DATA_COMMAND, that becomes independent from WRITE_DATA. This solves also the nasty check later to understand if it is a WRITE_DATA or CHECK_DATA (in your patch,a CHECK_BITS_SET / CHECK_BITS_CLR / CLR_BIT)

Quoted 6 lines

tools/imximage.c +0 -0

 	dcd_v1_t *dcd_v1 = &imxhdr->header.hdr_v1.dcd_table;
 

@@ -157,16 +161,47 @@ static void set_dcd_val_v1(struct imx_header *imxhdr, char *name, int lineno,
 }
 

tools/imximage.c static void set_dcd_val_v1(struct imx_header *imxhdr, char *name, int lineno, · line 171

Quoted 10 lines

tools/imximage.c +4 -1

 static void set_dcd_val_v2(struct imx_header *imxhdr, char *name, int lineno,
-					int fld, uint32_t value, uint32_t off)
+			   int fld, int cmd, uint32_t value, uint32_t off)
 {
 	dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.dcd_table;
+	dcd_command_t *dcd_command_block = (dcd_command_t *)&
+					    dcd_v2->dcd_data[dataindex];
 
 	switch (fld) {
+	case CFG_COMMAND:

...that means it should be not solved here. Instead of that, provide a dcd_check_data() function. Then we do not need at all this code.

Quoted 45 lines

tools/imximage.c +33 -2

+		/* update header */
+		if (cmd == CMD_DATA) {
+			dcd_command_block->tag = DCD_WRITE_DATA_COMMAND_TAG;
+			dcd_command_block->length = cpu_to_be16(off *
+					sizeof(dcd_addr_data_t) + 4);
+			dcd_command_block->param = DCD_WRITE_DATA_PARAM;
+		} else if (cmd == CMD_CLR_BIT) {
+			dcd_command_block->tag = DCD_WRITE_DATA_COMMAND_TAG;
+			dcd_command_block->length = cpu_to_be16(off *
+					sizeof(dcd_addr_data_t) + 4);
+			dcd_command_block->param = DCD_CLR_BIT_PARAM;
+		} else if (cmd == CMD_CHECK_BITS_SET) {
+			dcd_command_block->tag = DCD_CHECK_DATA_COMMAND_TAG;
+			/*
+			 * check data command only supports one entry,
+			 * so use 0xC = size(address + value + command).
+			 */
+			dcd_command_block->length = cpu_to_be16(0xC);
+			dcd_command_block->param = DCD_CHECK_BITS_SET_PARAM;
+		} else if (cmd == CMD_CHECK_BITS_CLR) {
+			dcd_command_block->tag = DCD_CHECK_DATA_COMMAND_TAG;
+			/*
+			 * check data command only supports one entry,
+			 * so use 0xC = size(address + value + command).
+			 */
+			dcd_command_block->length = cpu_to_be16(0xC);
+			dcd_command_block->param = DCD_CHECK_BITS_CLR_PARAM;
+		}
 	case CFG_REG_ADDRESS:
-		dcd_v2->addr_data[off].addr = cpu_to_be32(value);
+		dcd_command_block->addr_data[off].addr = cpu_to_be32(value);
 		break;
 	case CFG_REG_VALUE:
-		dcd_v2->addr_data[off].value = cpu_to_be32(value);
+		dcd_command_block->addr_data[off].value = cpu_to_be32(value);
 		break;
 	default:
 		break;

@@ -197,13 +232,14 @@ static void set_dcd_rst_v2(struct imx_header *imxhdr, uint32_t dcd_len,
 	dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.dcd_table;
 
 	dcd_v2->header.tag = DCD_HEADER_TAG;
+	/*
+	 * dataindex does not contain the last dcd block,
+	 * see how dataindex is updated.

tools/imximage.c static void set_dcd_rst_v2(struct imx_header *imxhdr, uint32_t dcd_len, · line 206

Quoted 10 lines

tools/imximage.c +3 -5

+	 */
 	dcd_v2->header.length = cpu_to_be16(
-			dcd_len * sizeof(dcd_addr_data_t) + 8);
+			(dataindex + 1) * 4 + dcd_len *
+			sizeof(dcd_addr_data_t) + 4);
 	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;

That means the code must be rearranged.

Quoted 52 lines

tools/imximage.c +16 -6

 }
 
 static void set_imx_hdr_v1(struct imx_header *imxhdr, uint32_t dcd_len,

@@ -317,12 +353,11 @@ static void print_hdr_v2(struct imx_header *imx_hdr)
 	dcd_v2_t *dcd_v2 = &hdr_v2->dcd_table;
 	uint32_t size, version;
 
-	size = be16_to_cpu(dcd_v2->header.length) - 8;
-	if (size > (MAX_HW_CFG_SIZE_V2 * sizeof(dcd_addr_data_t))) {
+	size = be16_to_cpu(dcd_v2->header.length);
+	if (size > MAX_DCD_SIZE_V2) {
 		fprintf(stderr,
 			"Error: Image corrupt DCD size %d exceed maximum %d\n",
-			(uint32_t)(size / sizeof(dcd_addr_data_t)),
-			MAX_HW_CFG_SIZE_V2);
+			size, MAX_DCD_SIZE_V2);
 		exit(EXIT_FAILURE);
 	}
 

@@ -398,7 +433,7 @@ static void parse_cfg_cmd(struct imx_header *imxhdr, int32_t cmd, char *token,
 		break;
 	case CMD_DATA:
 		value = get_cfg_value(token, name, lineno);
-		(*set_dcd_val)(imxhdr, name, lineno, fld, value, dcd_len);
+		(*set_dcd_val)(imxhdr, name, lineno, fld, cmd, value, dcd_len);
 		if (unlikely(cmd_ver_first != 1))
 			cmd_ver_first = 0;
 		break;

@@ -417,7 +452,8 @@ static void parse_cfg_cmd(struct imx_header *imxhdr, int32_t cmd, char *token,
 }
 
 static void parse_cfg_fld(struct imx_header *imxhdr, int32_t *cmd,
-		char *token, char *name, int lineno, int fld, int *dcd_len)
+			  int32_t *precmd, char *token, char *name,
+			  int lineno, int fld, int *dcd_len)
 {
 	int value;
 

@@ -430,26 +466,56 @@ static void parse_cfg_fld(struct imx_header *imxhdr, int32_t *cmd,
 			"(%s)\n", name, lineno, token);
 			exit(EXIT_FAILURE);
 		}
+
+		if ((*precmd == CMD_DATA) || (*precmd == CMD_CLR_BIT) ||
+		    (*precmd == CMD_CHECK_BITS_SET) ||
+		    (*precmd == CMD_CHECK_BITS_CLR)) {
+			if (*cmd != *precmd) {
+				dataindex += ((*dcd_len) *
+					      sizeof(dcd_addr_data_t) + 4) >> 2;
+				*dcd_len = 0;
+			}
+		}

tools/imximage.c static void parse_cfg_fld(struct imx_header *imxhdr, int32_t *cmd, · line 488

Quoted 10 lines

tools/imximage.c +10 -0

+
+		if ((*cmd == CMD_DATA) || (*cmd == CMD_CLR_BIT) ||
+		    (*cmd == CMD_CHECK_BITS_SET) ||
+		    (*cmd == CMD_CHECK_BITS_CLR)) {
+			/*
+			 * Reserve the first entry for command header,
+			 * So use *dcd_len + 1 as the off.
+			 */
+			(*set_dcd_val)(imxhdr, name, lineno, fld,
+				       *cmd, 0, *dcd_len + 1);

You see here: mixing the two commands make code more confused.

Quoted 136 lines

tools/imximage.c +27 -16

+		}
+
+		*precmd = *cmd;
+
 		break;
 	case CFG_REG_SIZE:
 		parse_cfg_cmd(imxhdr, *cmd, token, name, lineno, fld, *dcd_len);
 		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_CHECK_BITS_SET:
+		case CMD_CHECK_BITS_CLR:
+		case CMD_DATA:
+		case CMD_CLR_BIT:
+			value = get_cfg_value(token, name, lineno);
+			(*set_dcd_val)(imxhdr, name, lineno, fld, *cmd, 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:
+			return;
 		}
 		break;
 	default:

@@ -465,7 +531,7 @@ static uint32_t parse_cfg_file(struct imx_header *imxhdr, char *name)
 	int fld;
 	size_t len;
 	int dcd_len = 0;
-	int32_t cmd;
+	int32_t cmd, precmd = CMD_INVALID;
 
 	fd = fopen(name, "r");
 	if (fd == 0) {

@@ -473,6 +539,7 @@ static uint32_t parse_cfg_file(struct imx_header *imxhdr, char *name)
 		exit(EXIT_FAILURE);
 	}
 
+	dataindex = 0;
 	/*
 	 * Very simple parsing, line starting with # are comments
 	 * and are dropped

@@ -495,8 +562,8 @@ static uint32_t parse_cfg_file(struct imx_header *imxhdr, char *name)
 			if (token[0] == '#')
 				break;
 
-			parse_cfg_fld(imxhdr, &cmd, token, name,
-					lineno, fld, &dcd_len);
+			parse_cfg_fld(imxhdr, &cmd, &precmd, token, name,
+				      lineno, fld, &dcd_len);
 		}
 
 	}

tools/imximage.h +21 -3

index 36fe095..2ac5bbd 100644
@@ -8,6 +8,8 @@
 #ifndef _IMXIMAGE_H_
 #define _IMXIMAGE_H_
 
+#include <linux/sizes.h>
+#define MAX_DCD_SIZE_V2 1768 /* The DCD size limited to 1768 bytes for v2 */
 #define MAX_HW_CFG_SIZE_V2 220 /* Max number of registers imx can set for v2 */
 #define MAX_HW_CFG_SIZE_V1 60  /* Max number of registers imx can set for v1 */
 #define APP_CODE_BARKER	0xB1

@@ -49,12 +51,22 @@
 #define DCD_VERSION 0x40
 #define DCD_COMMAND_PARAM 0x4
 
+#define DCD_WRITE_DATA_COMMAND_TAG	0xCC
+#define DCD_WRITE_DATA_PARAM		0x4
+#define DCD_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_CLR_BIT,
+	CMD_CHECK_BITS_SET,
+	CMD_CHECK_BITS_CLR,
 	CMD_CSF,
 };
 

@@ -126,9 +138,15 @@ typedef struct {
 } __attribute__((packed)) write_dcd_command_t;
 
 typedef struct {
+	uint8_t tag;
+	uint16_t length;
+	uint8_t param;
+	dcd_addr_data_t addr_data[0];
+} __attribute__((packed)) dcd_command_t;
+
+typedef struct {
 	ivt_header_t header;
-	write_dcd_command_t write_dcd_command;
-	dcd_addr_data_t addr_data[MAX_HW_CFG_SIZE_V2];
+	uint8_t dcd_data[MAX_DCD_SIZE_V2 - sizeof(ivt_header_t)];
 } dcd_v2_t;
 
 typedef struct {

@@ -164,7 +182,7 @@ struct imx_header {
 
 typedef void (*set_dcd_val_t)(struct imx_header *imxhdr,
 					char *name, int lineno,
-					int fld, uint32_t value,
+					int fld, int cmd, uint32_t value,
 					uint32_t off);
 

tools/imximage.h struct imx_header { · line 188

Quoted 2 lines

tools/imximage.h +0 -0

 typedef void (*set_dcd_rst_t)(struct imx_header *imxhdr,

Best regards,
Stefano Babic

Signature · 5 lines

=====================================================================
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
=====================================================================

Hi Stefano,

Quoted 3 lines · Stefano Babic

Hi Peng,

On 14/06/2015 11:38, Peng Fan wrote:

Quoted 6 lines

Quoted 3 lines

Since rom code supports the following commands, add new commands support in imximage.

It is better to explain here which i.MX are supporting this ROM (i.MX6 and i.MX7).

Ok. Will fix this.

Quoted 14 lines

Quoted 6 lines

1. CHECK_BITS_SET 4 [address] [mask bit]
means:

   while ((*address & mask) != mask);

Quoted 7 lines

2. CHECK_BITS_CLR 4 [address] [mask bit]
means:

   while ((*address & ~mask) != 0);

2. CLR_BIT 4 [address] [mask bit]
means:

   *address = *address & ~mask;

Quoted 7 lines

I understand that the command to be added is CHECK_DATA_COMMAND, as reported by manual. The TAG for the command is the same (0xCF), that means we have a single command with different parameters.
It is better to follow the same approach in the code, because it is easy to find the relatd documentation in manual. In this patch, it looks like we have different commands, but this is not true: there is one command with different parameters.

Yeah, you are right. CHECK_BITS_SET/CLR corresponds to CHECK_DATA_COMMAND. CLR_BIT corresponds to WRITE_DATA_COMMAND, with mask=1, set=0.

The reason to add different commands but not one CHECK_DATA_COMMAND is that compatible with current implementation.
Current DCD supports "DATA 4 addr value". If want to use one CHECK_DATA_COMMAND to cover CHECK_BITS_SET/CLR, we need another entry, then commands like this:
"CHECK_DATA 4 addr mask [SET or CLR]", which will make code more complex. So, I choose to make CHECK_BITS_SET/CLR different commands, but not one CHECK_DATA command.

Since "4" is not used for V2, how about "CHECK_DATA SET addr mask" and "CHECK_DATA CLR addr mask"?

Quoted 9 lines

Quoted 5 lines

dcd_v2_t is redefined, because there may not be only one write data command, there may be many different commands like CHECK_BITS_SET, CHECK_BITS_CLR and CLR_BIT.

I disagree or maybe this is related to i.MX7, where I could not check the documentation. Please explain here: I see only one command (CHECK_DATA_COMMAND, Chapter 8.7.2.2 in i.MX6Q).

i.MX7 needs CHECK_DATA_COMMAND to wait some bits set, because CHECK_BITS_SET is implemented, so I call it a command:)
Yeah, there is only one CHECK_DATA_COMMAND. I'll refine this piece commit msg.

Quoted 9 lines

Quoted 5 lines

dcd_len is still leaved there, since changing it needs changes for dcd v1. For v2, we check whether dcd size is larger than MAX_DCD_SIZE_V2, but not dcd_len.

It is just a bit confusing, and these are details in implementation - they are not related to the commit where you explain the new feature. Move these comments inside the code where they belong to.

Ok. Will do.

Quoted 17 lines

Quoted 17 lines

Signed-off-by Peng Fan <[email protected]>

---
 tools/imximage.c | 129 ++++++++++++++++++++++++++++++++++++++++++-------------
 tools/imximage.h |  24 +++++++++--
 2 files changed, 119 insertions(+), 34 deletions(-)

tools/imximage.c +1 -0

index 6f469ae..1c0225d 100644
@@ -22,6 +22,9 @@ 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_CLR_BIT,           "CLR_BIT",              "Reg clear bit",  },

Quoted 8 lines

tools/imximage.c static table_entry_t imximage_cmds[] = { · line 27

Quoted 2 lines

tools/imximage.c +2 -0

+	{CMD_CHECK_BITS_SET,    "CHECK_BITS_SET",       "Reg Check bits set", },
+	{CMD_CHECK_BITS_CLR,    "CHECK_BITS_CLR",       "Reg Check bits clr", },

The table reflects Freescale's documentation. This has the big advantage because there is no need to explain which command is supposed to do because this is really well done by Freescale in the manuals. Here we should have only an additional entry due to CHECK_DATA_COMMAND, exactly what the SOC is able to do.

Back to the question, why I implemented CHECK_BITS_SET/CLR, but not CHECK_DATA_COMMAND. I do not want to introduce more entry in one DCD entry such as "CHECK_DATA 4 addr mask [CLR/SET]", so I use two software commands to cover CHECK_DATA_COMMAND.

Quoted 26 lines

Quoted 13 lines

tools/imximage.c +1 -0

 	{CMD_CSF,               "CSF",           "Command Sequence File", },
 	{CMD_IMAGE_VERSION,     "IMAGE_VERSION",        "image version",  },
 	{-1,                    "",                     "",	          },

@@ -84,6 +87,7 @@ static set_imx_hdr_t set_imx_hdr;
 static uint32_t max_dcd_entries;
 static uint32_t *header_size_ptr;
 static uint32_t *csf_ptr;
+static uint32_t dataindex;
 
 static uint32_t get_cfg_value(char *token, char *name,  int linenr)
 {

tools/imximage.c static void err_imximage_version(int version) · line 137

Quoted 6 lines

tools/imximage.c +1 -1

@@ -129,7 +133,7 @@ static void err_imximage_version(int version)
 }
 
 static void set_dcd_val_v1(struct imx_header *imxhdr, char *name, int lineno,
-					int fld, uint32_t value, uint32_t off)
+			   int fld, int cmd, uint32_t value, uint32_t off)
 {

Parameter list is becoming very long compared to first implementation.

Let's see what we can do. We have function pointers to override a specific behaviour for a SOC (V1 or V2). The set_dcd_val_vX corresponds to set an entry in the DCD, but it was thought as WRITE_DATA_COMMAND.

Quoted 6 lines

Instead of trying to extend this function, that has a different goal, it should be better to add a function pointer for CHECK_DATA_COMMAND, that becomes independent from WRITE_DATA. This solves also the nasty check later to understand if it is a WRITE_DATA or CHECK_DATA (in your patch,a CHECK_BITS_SET / CHECK_BITS_CLR / CLR_BIT)

Ok. more work need to be done to use this way, seems need to refactor the current patch.

The main point about your comments is my way implementation should follow Reference mannual, but not different commands implemented in imximage.c for only one command in reference mannual.

Quoted 278 lines

Quoted 6 lines

tools/imximage.c +0 -0

 	dcd_v1_t *dcd_v1 = &imxhdr->header.hdr_v1.dcd_table;
 

@@ -157,16 +161,47 @@ static void set_dcd_val_v1(struct imx_header *imxhdr, char *name, int lineno,
 }
 

tools/imximage.c static void set_dcd_val_v1(struct imx_header *imxhdr, char *name, int lineno, · line 171

Quoted 10 lines

tools/imximage.c +4 -1

 static void set_dcd_val_v2(struct imx_header *imxhdr, char *name, int lineno,
-					int fld, uint32_t value, uint32_t off)
+			   int fld, int cmd, uint32_t value, uint32_t off)
 {
 	dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.dcd_table;
+	dcd_command_t *dcd_command_block = (dcd_command_t *)&
+					    dcd_v2->dcd_data[dataindex];
 
 	switch (fld) {
+	case CFG_COMMAND:

...that means it should be not solved here. Instead of that, provide a dcd_check_data() function. Then we do not need at all this code.

Quoted 45 lines

tools/imximage.c +33 -2

+		/* update header */
+		if (cmd == CMD_DATA) {
+			dcd_command_block->tag = DCD_WRITE_DATA_COMMAND_TAG;
+			dcd_command_block->length = cpu_to_be16(off *
+					sizeof(dcd_addr_data_t) + 4);
+			dcd_command_block->param = DCD_WRITE_DATA_PARAM;
+		} else if (cmd == CMD_CLR_BIT) {
+			dcd_command_block->tag = DCD_WRITE_DATA_COMMAND_TAG;
+			dcd_command_block->length = cpu_to_be16(off *
+					sizeof(dcd_addr_data_t) + 4);
+			dcd_command_block->param = DCD_CLR_BIT_PARAM;
+		} else if (cmd == CMD_CHECK_BITS_SET) {
+			dcd_command_block->tag = DCD_CHECK_DATA_COMMAND_TAG;
+			/*
+			 * check data command only supports one entry,
+			 * so use 0xC = size(address + value + command).
+			 */
+			dcd_command_block->length = cpu_to_be16(0xC);
+			dcd_command_block->param = DCD_CHECK_BITS_SET_PARAM;
+		} else if (cmd == CMD_CHECK_BITS_CLR) {
+			dcd_command_block->tag = DCD_CHECK_DATA_COMMAND_TAG;
+			/*
+			 * check data command only supports one entry,
+			 * so use 0xC = size(address + value + command).
+			 */
+			dcd_command_block->length = cpu_to_be16(0xC);
+			dcd_command_block->param = DCD_CHECK_BITS_CLR_PARAM;
+		}
 	case CFG_REG_ADDRESS:
-		dcd_v2->addr_data[off].addr = cpu_to_be32(value);
+		dcd_command_block->addr_data[off].addr = cpu_to_be32(value);
 		break;
 	case CFG_REG_VALUE:
-		dcd_v2->addr_data[off].value = cpu_to_be32(value);
+		dcd_command_block->addr_data[off].value = cpu_to_be32(value);
 		break;
 	default:
 		break;

@@ -197,13 +232,14 @@ static void set_dcd_rst_v2(struct imx_header *imxhdr, uint32_t dcd_len,
 	dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.dcd_table;
 
 	dcd_v2->header.tag = DCD_HEADER_TAG;
+	/*
+	 * dataindex does not contain the last dcd block,
+	 * see how dataindex is updated.

tools/imximage.c static void set_dcd_rst_v2(struct imx_header *imxhdr, uint32_t dcd_len, · line 206

Quoted 10 lines

tools/imximage.c +3 -5

+	 */
 	dcd_v2->header.length = cpu_to_be16(
-			dcd_len * sizeof(dcd_addr_data_t) + 8);
+			(dataindex + 1) * 4 + dcd_len *
+			sizeof(dcd_addr_data_t) + 4);
 	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;

That means the code must be rearranged.

Quoted 52 lines

tools/imximage.c +16 -6

 }
 
 static void set_imx_hdr_v1(struct imx_header *imxhdr, uint32_t dcd_len,

@@ -317,12 +353,11 @@ static void print_hdr_v2(struct imx_header *imx_hdr)
 	dcd_v2_t *dcd_v2 = &hdr_v2->dcd_table;
 	uint32_t size, version;
 
-	size = be16_to_cpu(dcd_v2->header.length) - 8;
-	if (size > (MAX_HW_CFG_SIZE_V2 * sizeof(dcd_addr_data_t))) {
+	size = be16_to_cpu(dcd_v2->header.length);
+	if (size > MAX_DCD_SIZE_V2) {
 		fprintf(stderr,
 			"Error: Image corrupt DCD size %d exceed maximum %d\n",
-			(uint32_t)(size / sizeof(dcd_addr_data_t)),
-			MAX_HW_CFG_SIZE_V2);
+			size, MAX_DCD_SIZE_V2);
 		exit(EXIT_FAILURE);
 	}
 

@@ -398,7 +433,7 @@ static void parse_cfg_cmd(struct imx_header *imxhdr, int32_t cmd, char *token,
 		break;
 	case CMD_DATA:
 		value = get_cfg_value(token, name, lineno);
-		(*set_dcd_val)(imxhdr, name, lineno, fld, value, dcd_len);
+		(*set_dcd_val)(imxhdr, name, lineno, fld, cmd, value, dcd_len);
 		if (unlikely(cmd_ver_first != 1))
 			cmd_ver_first = 0;
 		break;

@@ -417,7 +452,8 @@ static void parse_cfg_cmd(struct imx_header *imxhdr, int32_t cmd, char *token,
 }
 
 static void parse_cfg_fld(struct imx_header *imxhdr, int32_t *cmd,
-		char *token, char *name, int lineno, int fld, int *dcd_len)
+			  int32_t *precmd, char *token, char *name,
+			  int lineno, int fld, int *dcd_len)
 {
 	int value;
 

@@ -430,26 +466,56 @@ static void parse_cfg_fld(struct imx_header *imxhdr, int32_t *cmd,
 			"(%s)\n", name, lineno, token);
 			exit(EXIT_FAILURE);
 		}
+
+		if ((*precmd == CMD_DATA) || (*precmd == CMD_CLR_BIT) ||
+		    (*precmd == CMD_CHECK_BITS_SET) ||
+		    (*precmd == CMD_CHECK_BITS_CLR)) {
+			if (*cmd != *precmd) {
+				dataindex += ((*dcd_len) *
+					      sizeof(dcd_addr_data_t) + 4) >> 2;
+				*dcd_len = 0;
+			}
+		}

tools/imximage.c static void parse_cfg_fld(struct imx_header *imxhdr, int32_t *cmd, · line 488

Quoted 10 lines

tools/imximage.c +10 -0

+
+		if ((*cmd == CMD_DATA) || (*cmd == CMD_CLR_BIT) ||
+		    (*cmd == CMD_CHECK_BITS_SET) ||
+		    (*cmd == CMD_CHECK_BITS_CLR)) {
+			/*
+			 * Reserve the first entry for command header,
+			 * So use *dcd_len + 1 as the off.
+			 */
+			(*set_dcd_val)(imxhdr, name, lineno, fld,
+				       *cmd, 0, *dcd_len + 1);

You see here: mixing the two commands make code more confused.

Quoted 136 lines

tools/imximage.c +27 -16

+		}
+
+		*precmd = *cmd;
+
 		break;
 	case CFG_REG_SIZE:
 		parse_cfg_cmd(imxhdr, *cmd, token, name, lineno, fld, *dcd_len);
 		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_CHECK_BITS_SET:
+		case CMD_CHECK_BITS_CLR:
+		case CMD_DATA:
+		case CMD_CLR_BIT:
+			value = get_cfg_value(token, name, lineno);
+			(*set_dcd_val)(imxhdr, name, lineno, fld, *cmd, 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:
+			return;
 		}
 		break;
 	default:

@@ -465,7 +531,7 @@ static uint32_t parse_cfg_file(struct imx_header *imxhdr, char *name)
 	int fld;
 	size_t len;
 	int dcd_len = 0;
-	int32_t cmd;
+	int32_t cmd, precmd = CMD_INVALID;
 
 	fd = fopen(name, "r");
 	if (fd == 0) {

@@ -473,6 +539,7 @@ static uint32_t parse_cfg_file(struct imx_header *imxhdr, char *name)
 		exit(EXIT_FAILURE);
 	}
 
+	dataindex = 0;
 	/*
 	 * Very simple parsing, line starting with # are comments
 	 * and are dropped

@@ -495,8 +562,8 @@ static uint32_t parse_cfg_file(struct imx_header *imxhdr, char *name)
 			if (token[0] == '#')
 				break;
 
-			parse_cfg_fld(imxhdr, &cmd, token, name,
-					lineno, fld, &dcd_len);
+			parse_cfg_fld(imxhdr, &cmd, &precmd, token, name,
+				      lineno, fld, &dcd_len);
 		}
 
 	}

tools/imximage.h +21 -3

index 36fe095..2ac5bbd 100644
@@ -8,6 +8,8 @@
 #ifndef _IMXIMAGE_H_
 #define _IMXIMAGE_H_
 
+#include <linux/sizes.h>
+#define MAX_DCD_SIZE_V2 1768 /* The DCD size limited to 1768 bytes for v2 */
 #define MAX_HW_CFG_SIZE_V2 220 /* Max number of registers imx can set for v2 */
 #define MAX_HW_CFG_SIZE_V1 60  /* Max number of registers imx can set for v1 */
 #define APP_CODE_BARKER	0xB1

@@ -49,12 +51,22 @@
 #define DCD_VERSION 0x40
 #define DCD_COMMAND_PARAM 0x4
 
+#define DCD_WRITE_DATA_COMMAND_TAG	0xCC
+#define DCD_WRITE_DATA_PARAM		0x4
+#define DCD_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_CLR_BIT,
+	CMD_CHECK_BITS_SET,
+	CMD_CHECK_BITS_CLR,
 	CMD_CSF,
 };
 

@@ -126,9 +138,15 @@ typedef struct {
 } __attribute__((packed)) write_dcd_command_t;
 
 typedef struct {
+	uint8_t tag;
+	uint16_t length;
+	uint8_t param;
+	dcd_addr_data_t addr_data[0];
+} __attribute__((packed)) dcd_command_t;
+
+typedef struct {
 	ivt_header_t header;
-	write_dcd_command_t write_dcd_command;
-	dcd_addr_data_t addr_data[MAX_HW_CFG_SIZE_V2];
+	uint8_t dcd_data[MAX_DCD_SIZE_V2 - sizeof(ivt_header_t)];
 } dcd_v2_t;
 
 typedef struct {

@@ -164,7 +182,7 @@ struct imx_header {
 
 typedef void (*set_dcd_val_t)(struct imx_header *imxhdr,
 					char *name, int lineno,
-					int fld, uint32_t value,
+					int fld, int cmd, uint32_t value,
 					uint32_t off);
 

Quoted 12 lines

tools/imximage.h struct imx_header { · line 188

Quoted 2 lines

tools/imximage.h +0 -0

 typedef void (*set_dcd_rst_t)(struct imx_header *imxhdr,

Best regards,
Stefano Babic

Signature · 5 lines

=====================================================================
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
=====================================================================

Regards,
Peng

The reason to add different commands but not one CHECK_DATA_COMMAND is that compatible with current implementation.
Current DCD supports "DATA 4 addr value". If want to use one CHECK_DATA_COMMAND to cover CHECK_BITS_SET/CLR, we need another entry, then commands like this:
"CHECK_DATA 4 addr mask [SET or CLR]", which will make code more complex. So, I choose to make CHECK_BITS_SET/CLR different commands, but not one CHECK_DATA command.

Since "4" is not used for V2, how about "CHECK_DATA SET addr mask" and "CHECK_DATA CLR addr mask"?

Quoted 9 lines

Quoted 5 lines

dcd_v2_t is redefined, because there may not be only one write data command, there may be many different commands like CHECK_BITS_SET, CHECK_BITS_CLR and CLR_BIT.

I disagree or maybe this is related to i.MX7, where I could not check the documentation. Please explain here: I see only one command (CHECK_DATA_COMMAND, Chapter 8.7.2.2 in i.MX6Q).

i.MX7 needs CHECK_DATA_COMMAND to wait some bits set, because CHECK_BITS_SET is implemented, so I call it a command:)
Yeah, there is only one CHECK_DATA_COMMAND. I'll refine this piece commit msg.

Quoted 9 lines

Quoted 5 lines

dcd_len is still leaved there, since changing it needs changes for dcd v1. For v2, we check whether dcd size is larger than MAX_DCD_SIZE_V2, but not dcd_len.

It is just a bit confusing, and these are details in implementation - they are not related to the commit where you explain the new feature. Move these comments inside the code where they belong to.

Ok. Will do.

Quoted 17 lines

Quoted 17 lines

Signed-off-by Peng Fan <[email protected]>

---
 tools/imximage.c | 129 ++++++++++++++++++++++++++++++++++++++++++-------------
 tools/imximage.h |  24 +++++++++--
 2 files changed, 119 insertions(+), 34 deletions(-)

tools/imximage.c +1 -0

index 6f469ae..1c0225d 100644
@@ -22,6 +22,9 @@ 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_CLR_BIT,           "CLR_BIT",              "Reg clear bit",  },

Quoted 8 lines

tools/imximage.c static table_entry_t imximage_cmds[] = { · line 27

Quoted 2 lines

tools/imximage.c +2 -0

+	{CMD_CHECK_BITS_SET,    "CHECK_BITS_SET",       "Reg Check bits set", },
+	{CMD_CHECK_BITS_CLR,    "CHECK_BITS_CLR",       "Reg Check bits clr", },

The table reflects Freescale's documentation. This has the big advantage because there is no need to explain which command is supposed to do because this is really well done by Freescale in the manuals. Here we should have only an additional entry due to CHECK_DATA_COMMAND, exactly what the SOC is able to do.

Back to the question, why I implemented CHECK_BITS_SET/CLR, but not CHECK_DATA_COMMAND. I do not want to introduce more entry in one DCD entry such as "CHECK_DATA 4 addr mask [CLR/SET]", so I use two software commands to cover CHECK_DATA_COMMAND.

Quoted 1 line

Ok. more work need to be done to use this way, seems need to refactor the current patch.

The main point about your comments is my way implementation should follow Reference mannual, but not different commands implemented in imximage.c for only one command in reference mannual.