[U-Boot] [PATCH V2 06/21] imximage: add plugin commands

[Troy Kisky]

On 9/23/2012 8:38 AM, Stefano Babic wrote:
> On 22/09/2012 04:39, Troy Kisky wrote:
>> Add commands
>> plugin address filename
>> iomux_entry addr, data1 [, data2, [, data3]]
>> write_entry addr, data1 [, data2, [, data3]]
> Why do we need explicitely an IOMUX command ? As far as I can see, the
> program image defined in V2 defines a plugin, but not an iomux.
> I am expecting that the imximage generates a iMX header only, without
> moving some code from the initialization code directly here. In the
> manula there is a "Write Data" (what we have always had), a "Check data"
> and an "Unlock" commands.

The table built by iomux_entry and write_entry are not used by the ROM.
The plugin.S file that I add will interpret these entries.
I could have repurposed the "DATA" command  if I didn't mind bloating 
the table.
Having separate commands made it easy to generate small tables.

>
> If we start to add special commands, maybe we are staring again to
> reimplement U-Boot. We could have some SET_CLK, SET_CPU_FREQ, and so on.
> What I am really mising in this series is why you are moving a lot of
> things from U-Boot into the iMX header.
The cfg file after this patch series does no more setup/initialization 
than before
this series. I don't know what "moving" you are referring to.


>
> It seems to me we want to put much more code in the iMX header as what
> it is really required to boot the device.
>
> Adding / modifying the syntax requires to update doc/README.imximage, too.

Yes, if leaning toward acceptance I will add this.

>
>> Signed-off-by: Troy Kisky <troy.kisky at boundarydevices.com>
>> ---
>>   tools/imximage.c |  334 ++++++++++++++++++++++++++++++++++++++++++++----------
>>   tools/imximage.h |   11 +-
>>   2 files changed, 283 insertions(+), 62 deletions(-)
>>
>> diff --git a/tools/imximage.c b/tools/imximage.c
>> index 2c5a622..fae786a 100644
>> --- a/tools/imximage.c
>> +++ b/tools/imximage.c
>> @@ -31,7 +31,6 @@
>>   #include "mkimage.h"
>>   #include <image.h>
>>   #include "imximage.h"
>> -
>>   /*
>>    * Supported commands for configuration file
>>    */
>> @@ -39,6 +38,9 @@ static table_entry_t imximage_cmds[] = {
>>   	{CMD_BOOT_FROM,         "BOOT_FROM",            "boot command",	  },
>>   	{CMD_DATA,              "DATA",                 "Reg Write Data", },
>>   	{CMD_IMAGE_VERSION,     "IMAGE_VERSION",        "image version",  },
>> +	{CMD_PLUGIN,		"plugin",		"plugin addr,file",  },
>> +	{CMD_IOMUX_ENTRY,	"iomux_entry",		"Write iomux reg",  },
>> +	{CMD_WRITE_ENTRY,	"write_entry",		"Write register",  },
>>   	{-1,                    "",                     "",	          },
>>   };
>>   
>> @@ -69,8 +71,8 @@ static uint32_t imximage_version;
>>   
>>   static set_dcd_val_t set_dcd_val;
>>   static set_imx_hdr_t set_imx_hdr;
>> -static set_imx_size_t set_imx_size;
>>   static uint32_t *p_max_dcd;
>> +static uint32_t *header_size_ptr;
>>   static uint32_t g_flash_offset;
>>   
>>   static struct image_type_params imximage_params;
>> @@ -88,8 +90,7 @@ static uint32_t detect_imximage_version(struct imx_header *imx_hdr)
>>   		return IMXIMAGE_V1;
>>   
>>   	/* Try to detect V2 */
>> -	if ((fhdr_v2->header.tag == IVT_HEADER_TAG) &&
>> -		(hdr_v2->dcd_table.header.tag == DCD_HEADER_TAG))
>> +	if ((fhdr_v2->header.tag == IVT_HEADER_TAG))
>>   		return IMXIMAGE_V2;
> Help me to understand. I am reading i.MX6 manual and, even if the number
> of DCD entries could be variable, I do not see why the header tag of DCD
> is moving. At least, this is what I can see on picture 7-19, Image
> Vector table.

If the DCD table is missing, there is no DCD_HEADER_TAG.
DCD table is not required, so there is no need to check for it.

>>   
>>   	return IMXIMAGE_VER_INVALID;
>> @@ -160,7 +161,7 @@ static int set_dcd_val_v2(struct imx_header *imxhdr, uint32_t *data)
>>   }
>>   
>>   static int set_imx_hdr_v1(struct imx_header *imxhdr,
>> -		uint32_t entry_point, uint32_t flash_offset)
>> +		uint32_t entry_point, uint32_t flash_offset, int plugin)
>>   {
>>   	imx_header_v1_t *hdr_v1 = &imxhdr->header.hdr_v1;
>>   	flash_header_v1_t *fhdr_v1 = &hdr_v1->fhdr;
>> @@ -180,22 +181,12 @@ static int set_imx_hdr_v1(struct imx_header *imxhdr,
>>   	/* Security feature are not supported */
>>   	fhdr_v1->app_code_csf = 0;
>>   	fhdr_v1->super_root_key = 0;
>> +	header_size_ptr = (uint32_t *)(((char *)imxhdr) + header_length - 4);
>>   	return header_length;
>>   }
>>   
>> -static void set_imx_size_v1(struct imx_header *imxhdr, uint32_t file_size,
>> -		uint32_t flash_offset)
>> -{
>> -	uint32_t *p = (uint32_t *)(((char *)imxhdr)
>> -			+ imximage_params.header_size);
>> -
>> -	/* The external flash header must be at the end of the DCD table */
>> -	/* file_size includes header */
>> -	p[-1] = file_size + flash_offset;
>> -}
> I think you have to squash some of your patches or to defines them in
> another way. You added this code previously, and you drop now. This
> makes more difficult to review your patches.

I though this is what you were referring to in patch 4/21 response. So my
agreement there, should have been here.  Now I don't  know what in 4/21
you were referring to, but I'll reexamine before next version.