[U-Boot] [PATCH V3 1/8] tools: imximage: add plugin support

[Eric Nelson]

Hi Peng,

On 10/09/2016 04:20 AM, Peng Fan wrote:
> On Sat, Oct 08, 2016 at 05:26:18PM +0200, Eric Nelson wrote:
>> On 10/08/2016 08:58 AM, Peng Fan wrote:

<snip>

>>
>>From what I can tell, there's no reason that you can't have multiple
>> plugins, and the use of these variables isn't really needed.
> 
> I try to follow, are you talking about chained plugin images?
> Now we only support two IVT headers when using plugin.
> The first IVT header is for the plugin image, the second ivt header is for
> uboot image.
> 

I understand, though the API seems to allow it (chained plugins)
and I have a suspicion that the code would be cleaner without
the union.

That said, I don't have a use case in mind.

>>
>>> +static uint32_t imximage_iram_free_start;
>>> +static uint32_t imximage_plugin_size;
>>> +static uint32_t plugin_image;
>>>  
>>>  static set_dcd_val_t set_dcd_val;
>>>  static set_dcd_param_t set_dcd_param;
>>> @@ -118,7 +122,11 @@ static uint32_t detect_imximage_version(struct imx_header *imx_hdr)
>>>  
>>>  	/* Try to detect V2 */
>>>  	if ((fhdr_v2->header.tag == IVT_HEADER_TAG) &&
>>> -		(hdr_v2->dcd_table.header.tag == DCD_HEADER_TAG))
>>> +		(hdr_v2->data.dcd_table.header.tag == DCD_HEADER_TAG))
>>> +		return IMXIMAGE_V2;
>>> +
>>> +	if ((fhdr_v2->header.tag == IVT_HEADER_TAG) &&
>>> +	    hdr_v2->boot_data.plugin)
>>>  		return IMXIMAGE_V2;
>>>  
>>>  	return IMXIMAGE_VER_INVALID;
>>> @@ -165,7 +173,7 @@ static struct dcd_v2_cmd *gd_last_cmd;
>>>  static void set_dcd_param_v2(struct imx_header *imxhdr, uint32_t dcd_len,
>>>  		int32_t cmd)
>>>  {
>>
>> I also don't understand why the choice to make the union
>> with either a DCD table or a plugin.
> 
> Confirmed with ROM team. DCD and plugin are exclusive,
> 
> You can not have both at the same time.
>

That's too bad, since porting from DCD-style to SPL can be
useful (as Fabio has seen trying to keep some boards up-to-date).

If we get SPL-as-plugin working, then this would form a
dividing line where you need to get rid of the DCD altogether.

What about the CSF test? Your patches say that CSF and plugins
are also not supported.

>>
>> We should be able to have both, and this doesn't make
>> the code any easier.
>>
>>> -	dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.dcd_table;
>>> +	dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.data.dcd_table;
>>>  	struct dcd_v2_cmd *d = gd_last_cmd;
>>>  	struct dcd_v2_cmd *d2;
>>>  	int len;
>>> @@ -261,21 +269,23 @@ static void set_dcd_rst_v1(struct imx_header *imxhdr, uint32_t dcd_len,
>>>  static void set_dcd_rst_v2(struct imx_header *imxhdr, uint32_t dcd_len,
>>>  						char *name, int lineno)
>>>  {
>>> -	dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.dcd_table;
>>> -	struct dcd_v2_cmd *d = gd_last_cmd;
>>> -	int len;
>>> -
>>> -	if (!d)
>>> -		d = &dcd_v2->dcd_cmd;
>>> -	len = be16_to_cpu(d->write_dcd_command.length);
>>> -	if (len > 4)
>>> -		d = (struct dcd_v2_cmd *)(((char *)d) + len);
>>> -
>>> -	len = (char *)d - (char *)&dcd_v2->header;
>>> -
>>> -	dcd_v2->header.tag = DCD_HEADER_TAG;
>>> -	dcd_v2->header.length = cpu_to_be16(len);
>>> -	dcd_v2->header.version = DCD_VERSION;
>>> +	if (!imxhdr->header.hdr_v2.boot_data.plugin) {
>>> +		dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.data.dcd_table;
>>> +		struct dcd_v2_cmd *d = gd_last_cmd;
>>> +		int len;
>>> +
>>> +		if (!d)
>>> +			d = &dcd_v2->dcd_cmd;
>>> +		len = be16_to_cpu(d->write_dcd_command.length);
>>> +		if (len > 4)
>>> +			d = (struct dcd_v2_cmd *)(((char *)d) + len);
>>> +
>>> +		len = (char *)d - (char *)&dcd_v2->header;
>>> +
>>> +		dcd_v2->header.tag = DCD_HEADER_TAG;
>>> +		dcd_v2->header.length = cpu_to_be16(len);
>>> +		dcd_v2->header.version = DCD_VERSION;
>>> +	}
>>>  }
>>>  
>>>  static void set_imx_hdr_v1(struct imx_header *imxhdr, uint32_t dcd_len,
>>> @@ -317,24 +327,93 @@ static void set_imx_hdr_v2(struct imx_header *imxhdr, uint32_t dcd_len,
>>>  	fhdr_v2->header.length = cpu_to_be16(sizeof(flash_header_v2_t));
>>>  	fhdr_v2->header.version = IVT_VERSION; /* 0x40 */
>>>  
>>
>> It seems that the reason for a lot of this special-purpose code is to add
>> support for the entry address.
>>
>> If mkimage is invoked with an entrypoint from the command-line:
>>
>> ~/$ ./tools/mkimage -n my.cfg -T imximage -e 0xentrypoint u-boot.img
>> u-boot.imx
>>
>> This entry point is normally placed into the header, but if we have a
>> plugin in the .cfg file like this:
>>
>> ~/$ grep PLUGIN my.cfg
>> PLUGIN path/to/board/plugin.bin 0x00907000
>>
>> Then we need to use the plugin's address instead of the command
>> line address, and use the command-line address for the file which
>> follows.
> 
> There are two IVT headers when using plugin, the 0x907000 is for the first IVT
> header, 0x87800000 in command line is for the second IVT header.
> 
>>
>>> -	fhdr_v2->entry = entry_point;
>>> -	fhdr_v2->reserved1 = fhdr_v2->reserved2 = 0;
>>> -	hdr_base = entry_point - imximage_init_loadsize +
>>> -		flash_offset;
>>> -	fhdr_v2->self = hdr_base;
>>> -	if (dcd_len > 0)
>>> -		fhdr_v2->dcd_ptr = hdr_base
>>> -			+ offsetof(imx_header_v2_t, dcd_table);
>>> -	else
>>> +	if (!hdr_v2->boot_data.plugin) {
>>> +		fhdr_v2->entry = entry_point;
>>> +		fhdr_v2->reserved1 = 0;
>>> +		fhdr_v2->reserved1 = 0;
>>> +		hdr_base = entry_point - imximage_init_loadsize +
>>> +			flash_offset;
>>> +		fhdr_v2->self = hdr_base;
>>> +		if (dcd_len > 0)
>>> +			fhdr_v2->dcd_ptr = hdr_base +
>>> +				offsetof(imx_header_v2_t, data);
>>> +		else
>>> +			fhdr_v2->dcd_ptr = 0;
>>> +		fhdr_v2->boot_data_ptr = hdr_base
>>> +				+ offsetof(imx_header_v2_t, boot_data);
>>> +		hdr_v2->boot_data.start = entry_point - imximage_init_loadsize;
>>> +
>>> +		fhdr_v2->csf = 0;
>>> +
>>> +		header_size_ptr = &hdr_v2->boot_data.size;
>>> +		csf_ptr = &fhdr_v2->csf;
>>> +	} else {
>>> +		imx_header_v2_t *next_hdr_v2;
>>> +		flash_header_v2_t *next_fhdr_v2;
>>> +
>>> +		if (imximage_csf_size != 0) {
>>> +			fprintf(stderr, "Error: Header v2: SECURE_BOOT is only supported in DCD mode!");
>>> +			exit(EXIT_FAILURE);
>>> +		}
>>> +
>>
>> I think that only ->entry needs to be different between these
>> two blocks.
> 
> There are two IVT headers for plugin. In this block, the second IVT also
> needs to be handled.
> 

I understand. It just appears to me that almost all of the code
that manipulates fhdr_v2 in the two blocks of code could be
done before the test.

Only the processing of fhdr_v2->entry and next_fhdr_v2 needs
to be different.

>>
>>> +		fhdr_v2->entry = imximage_iram_free_start +
>>> +			flash_offset + sizeof(flash_header_v2_t) +
>>> +			sizeof(boot_data_t);
>>> +
>>> +		fhdr_v2->reserved1 = 0;
>>> +		fhdr_v2->reserved2 = 0;
>>> +		fhdr_v2->self = imximage_iram_free_start + flash_offset;
>>> +
>>>  		fhdr_v2->dcd_ptr = 0;
>>> -	fhdr_v2->boot_data_ptr = hdr_base
>>> -			+ offsetof(imx_header_v2_t, boot_data);
>>> -	hdr_v2->boot_data.start = entry_point - imximage_init_loadsize;
>>>  
>>> -	fhdr_v2->csf = 0;
>>> +		fhdr_v2->boot_data_ptr = fhdr_v2->self +
>>> +				offsetof(imx_header_v2_t, boot_data);
>>> +
>>> +		hdr_v2->boot_data.start = imximage_iram_free_start;
>>> +		/*
>>> +		 * The actural size of plugin image is "imximage_plugin_size +
>>> +		 * sizeof(flash_header_v2_t) + sizeof(boot_data_t)", plus the
>>> +		 * flash_offset space.The ROM code only need to copy this size
>>> +		 * to run the plugin code. However, later when copy the whole
>>> +		 * U-Boot image to DDR, the ROM code use memcpy to copy the
>>> +		 * first part of the image, and use the storage read function
>>> +		 * to get the remaining part. This requires the dividing point
>>> +		 * must be multiple of storage sector size. Here we set the
>>> +		 * first section to be 16KB for this purpose.
>>> +		 */
>>
>> Where is the 16k limit coming from? I don't think this is necessary,
>> and if it is, we won't be able to load SPL using a plugin.
> 
> Confirmed with ROM team, there is no limitation. But SPL code needs to be small
> to be in OCRAM.
>

Whew! This would have killed the idea of SPL-as-plugin.

> I'll rework this piece code to drop this limitation.
> 

<snip>

>>
>> This structure of parse_cfg_cmd to handle the first
>> argument and parse_cfg_fld to handle the second
>> argument is unfortunate.
> 
> parse_cfg_cmd is to parse the CMD, parse_cfg_fld is to handle the second argument.
> This is the design of imximage.c to parse xx.cfg file. I do not want to break this.
> 

I'd say that it's a bit broken already, but that has nothing to
do with your patch.

The parsing model seems to have outlived its' usefulness and the
use of the CFG_COMMAND, CFG_REG_SIZE, et cetera to mean
parameter numbers doesn't fit for many (any?) commands.

<snip>

>>>  
>>>  /* The header must be aligned to 4k on MX53 for NAND boot */
>>>
>>
>> Again, I apologize for being slow to review this, but I'm
>> hoping to use this to build a package of SPL (as a plugin)
>> along with U-Boot and I still think it's doable.
>>
>> A quick proof-of-concept shows that we can load SPL
>> directly as a plugin just by setting byte 0x28 to 1
> 
> some more work needed to use SPL as a plugin image, I think,
> directly use "PLUGIN xxx/SPL" may not work.
> 
> mx6_plugin.S is needed.
> 

Well, some modification to the startup is needed to save
the context for an eventual return to ROM, but if we're
trying to use SPL to perform SoC and DDR initialization,
it can't be a stand-alone plugin.S.

Regards,


Eric