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 <[email protected]> >>> --- >>> 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: [email protected] >> ===================================================================== > > Regards, > Peng > _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
