[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