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

Nitin Garg nitin.garg at freescale.com
Tue Jul 7 22:02:52 CEST 2015


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



More information about the U-Boot mailing list