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

3. 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@denx.de =====================================================================

Regards, Peng --

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