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