[U-Boot] [PATCH] imx: imximage: add new CHECK/CLR BIT command
Stefano Babic
sbabic at denx.de
Sun Jun 28 13:00:07 CEST 2015
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 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.
> {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
=====================================================================
More information about the U-Boot
mailing list