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

Peng Fan van.freenix at gmail.com
Sun Oct 9 08:12:09 CEST 2016


Hi Eric,
On Sun, Oct 09, 2016 at 07:42:34AM +0200, Eric Nelson wrote:
>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).

I saw the patches.
If using plugin, there is no need to use DCD.

all the things in DCD can be done in plugin code.

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

CSF is supported  and code in nxp community. But that piece code is not included
in the nxp released uboot.

https://community.nxp.com/docs/DOC-332725

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

SPL is designed to run in OCRAM for i.MX6, so it's still ok to let SPL as plugin image.

Regards,
Peng.

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


More information about the U-Boot mailing list