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

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

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

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 <Peng.Fan@freescale.com>
---
 tools/imximage.c | 129 ++++++++++++++++++++++++++++++++++++++++++-------------
 tools/imximage.h |  24 +++++++++--
 2 files changed, 119 insertions(+), 34 deletions(-)

diff --git a/tools/imximage.c b/tools/imximage.c
index 6f469ae..1c0225d 100644
--- a/tools/imximage.c
+++ b/tools/imximage.c
@@ -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",  },
+	{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,                    "",                     "",	          },
@@ -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)
 {
@@ -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)
 {
 	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,
 }
 
 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:
+		/* 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.
+	 */
 	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;
 }
 
 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;
+			}
+		}
+
+		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);
+		}
+
+		*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);
 		}
 
 	}
diff --git a/tools/imximage.h b/tools/imximage.h
index 36fe095..2ac5bbd 100644
--- a/tools/imximage.h
+++ b/tools/imximage.h
@@ -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);
 
 typedef void (*set_dcd_rst_t)(struct imx_header *imxhdr,
-- 
1.8.4

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

[Fabio Estevam]

Hi Peng,

On Sun, Jun 14, 2015 at 6:38 AM, Peng Fan <Peng.Fan@freescale.com> wrote:
> 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 <Peng.Fan@freescale.com>

What about just using SPL mechanism instead?

Regards,

Fabio Estevam

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

[Otavio Salvador]

On Mon, Jun 15, 2015 at 12:17 PM, Fabio Estevam <festevam@gmail.com> wrote:
> On Sun, Jun 14, 2015 at 6:38 AM, Peng Fan <Peng.Fan@freescale.com> wrote:
>> 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 <Peng.Fan@freescale.com>
>
> 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?

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

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

[Fabio Estevam]

On Mon, Jun 15, 2015 at 12:20 PM, Otavio Salvador
<otavio@ossystems.com.br> wrote:

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

It would be nice to see a user of the new proposed feature. This patch
as is only adds dead code.

Regards,

Fabio Estevam

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

[Stefano Babic]

Hi Peng,

On 15/06/2015 17:17, Fabio Estevam wrote:
> Hi Peng,
> 
> On Sun, Jun 14, 2015 at 6:38 AM, Peng Fan <Peng.Fan@freescale.com> wrote:
>> 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 <Peng.Fan@freescale.com>
> 
> What about just using SPL mechanism instead?


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

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

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

[Chris Kuethe]

On Mon, Jun 15, 2015 at 8:20 AM, Otavio Salvador <otavio@ossystems.com.br>
wrote:
> 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?

-- 
GDB has a 'break' feature; why doesn't it have 'fix' too?

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

[Fabio Estevam]

On Mon, Jun 15, 2015 at 2:55 PM, Chris Kuethe <chris.kuethe@gmail.com> wrote:
> On Mon, Jun 15, 2015 at 8:20 AM, Otavio Salvador <otavio@ossystems.com.br>
> wrote:
>> 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?

I am not against the patch. My main point is that we do not have any
user for the proposed commands, so we can better understand its
motivation.

Regards,

Fabio Estevam

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

[Peng Fan]

Hi All,
On Mon, Jun 15, 2015 at 05:53:19PM +0200, Stefano Babic wrote:
>Hi Peng,
>
>On 15/06/2015 17:17, Fabio Estevam wrote:
>> Hi Peng,
>> 
>> On Sun, Jun 14, 2015 at 6:38 AM, Peng Fan <Peng.Fan@freescale.com> wrote:
>>> 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 <Peng.Fan@freescale.com>
>> 
>> 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.
>
>Thanks, Fabio. I can only join to your proposal.
Hi Stefano,
I hope you can review this patch. I agree Otavio's idea to make feature complete,
and future i.MX7 needs this feature.
>
>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
>=====================================================================

Regards,
Peng.
-- 

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

[Stefano Babic]

Hi Peng,

On 16/06/2015 03:48, Peng Fan wrote:

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

The point is not to block this feature, but, as you are starting with a
new SOC (i.MX7), to think about how to better support this new
processor. We get a while to have SPL support for i.MX6, and the same
benefits we can have from the beginning with i.MX7.

>>
>> Thanks, Fabio. I can only join to your proposal.
> Hi Stefano,
> I hope you can review this patch.

Of course, this will be done - anyway, what I and Fabio try to point out
is to give i.MX7 a chance to be supported as well as i.MX6 is.

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

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

[Stefano Babic]

Hi Peng,

On 14/06/2015 11:38, Peng Fan wrote:
> 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).


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

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.

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

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

> 
> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
> ---
>  tools/imximage.c | 129 ++++++++++++++++++++++++++++++++++++++++++-------------
>  tools/imximage.h |  24 +++++++++--
>  2 files changed, 119 insertions(+), 34 deletions(-)
> 
> diff --git a/tools/imximage.c b/tools/imximage.c
> index 6f469ae..1c0225d 100644
> --- a/tools/imximage.c
> +++ b/tools/imximage.c
> @@ -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",  },
> +	{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.

>  	{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)
>  {
> @@ -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)


>  	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,
>  }
>  
>  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.

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

>  }
>  
>  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;
> +			}
> +		}
> +
> +		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.
> +		}
> +
> +		*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);
>  		}
>  
>  	}
> diff --git a/tools/imximage.h b/tools/imximage.h
> index 36fe095..2ac5bbd 100644
> --- a/tools/imximage.h
> +++ b/tools/imximage.h
> @@ -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);
>  
>  typedef void (*set_dcd_rst_t)(struct imx_header *imxhdr,
> 

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

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

[Peng Fan]

Hi Stefano,

On Sun, Jun 28, 2015 at 01:00:07PM +0200, Stefano Babic wrote:
>Hi Peng,
>
>On 14/06/2015 11:38, Peng Fan wrote:
>> 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.

>
>
>> 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;
>
>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"?

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

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

>
>> 
>> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
>> ---
>>  tools/imximage.c | 129 ++++++++++++++++++++++++++++++++++++++++++-------------
>>  tools/imximage.h |  24 +++++++++--
>>  2 files changed, 119 insertions(+), 34 deletions(-)
>> 
>> diff --git a/tools/imximage.c b/tools/imximage.c
>> index 6f469ae..1c0225d 100644
>> --- a/tools/imximage.c
>> +++ b/tools/imximage.c
>> @@ -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",  },
>> +	{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.

>
>>  	{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)
>>  {
>> @@ -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)
>

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.

>
>>  	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,
>>  }
>>  
>>  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.
>
>> +		/* 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.
>> +	 */
>>  	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.
>
>>  }
>>  
>>  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;
>> +			}
>> +		}
>> +
>> +		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.
>> +		}
>> +
>> +		*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);
>>  		}
>>  
>>  	}
>> diff --git a/tools/imximage.h b/tools/imximage.h
>> index 36fe095..2ac5bbd 100644
>> --- a/tools/imximage.h
>> +++ b/tools/imximage.h
>> @@ -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);
>>  
>>  typedef void (*set_dcd_rst_t)(struct imx_header *imxhdr,
>> 
>
>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
>=====================================================================

Regards,
Peng
-- 

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

[Nitin Garg]

On 06/30/2015 03:02 AM, Peng Fan wrote:
> Hi Stefano,
> 
> On Sun, Jun 28, 2015 at 01:00:07PM +0200, Stefano Babic wrote:
>> Hi Peng,
>>
>> On 14/06/2015 11:38, Peng Fan wrote:
>>> 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.
> 
Just to make sure; i.MX6 and i.MX7 support these commands.

>>
>>
>>> 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;
>>
>> 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"?
> 
>>
>>>
>>> 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.
> 
>>
>>>
>>> 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.
> 
>>
>>>
>>> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
>>> ---
>>>  tools/imximage.c | 129 ++++++++++++++++++++++++++++++++++++++++++-------------
>>>  tools/imximage.h |  24 +++++++++--
>>>  2 files changed, 119 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/tools/imximage.c b/tools/imximage.c
>>> index 6f469ae..1c0225d 100644
>>> --- a/tools/imximage.c
>>> +++ b/tools/imximage.c
>>> @@ -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",  },
>>> +	{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.
> 
>>
>>>  	{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)
>>>  {
>>> @@ -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)
>>
> 
> 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.
> 
>>
>>>  	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,
>>>  }
>>>  
>>>  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.
>>
>>> +		/* 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.
>>> +	 */
>>>  	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.
>>
>>>  }
>>>  
>>>  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;
>>> +			}
>>> +		}
>>> +
>>> +		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.
>>> +		}
>>> +
>>> +		*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);
>>>  		}
>>>  
>>>  	}
>>> diff --git a/tools/imximage.h b/tools/imximage.h
>>> index 36fe095..2ac5bbd 100644
>>> --- a/tools/imximage.h
>>> +++ b/tools/imximage.h
>>> @@ -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);
>>>  
>>>  typedef void (*set_dcd_rst_t)(struct imx_header *imxhdr,
>>>
>>
>> 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
>> =====================================================================
> 
> Regards,
> Peng
> 

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

[Stefano Babic]

Hi Peng,

On 30/06/2015 10:02, Peng Fan wrote:

> 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"?
> 
>>
>>>
>>> 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.
> 
>>
>>>
>>> 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.
> 
>>
>>>
>>> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
>>> ---
>>>  tools/imximage.c | 129 ++++++++++++++++++++++++++++++++++++++++++-------------
>>>  tools/imximage.h |  24 +++++++++--
>>>  2 files changed, 119 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/tools/imximage.c b/tools/imximage.c
>>> index 6f469ae..1c0225d 100644
>>> --- a/tools/imximage.c
>>> +++ b/tools/imximage.c
>>> @@ -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",  },
>>> +	{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.

Yes, I understand, but if we check the current implementation you'll
agree that it follows the schema to have an entry in the command table
for each ROM command.

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

Yes, this is my biggest concern in your implementation. This makes IMHO
also more confusing the code later when it must be detected if it was a
CMD_DATA or CMD_CLR_BIT or...
This is automatically solved by switching to function pointers.

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