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

[Stefano Babic]

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

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


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