From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peng Fan Date: Sun, 9 Oct 2016 14:12:09 +0800 Subject: [U-Boot] [PATCH V3 1/8] tools: imximage: add plugin support In-Reply-To: <77357148-7557-5324-fa8b-0a0e04b3a65f@nelint.com> References: <1475909892-8827-1-git-send-email-peng.fan@nxp.com> <2c634318-4a3f-9b2d-cfd9-7b4b211df78c@nelint.com> <20161009022056.GA29414@linux-7smt.suse> <77357148-7557-5324-fa8b-0a0e04b3a65f@nelint.com> Message-ID: <20161009061207.GA9511@linux-7smt.suse> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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: > > > >>> >>>>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. >> > > > >>> >>> 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. > > > >>>> >>>> /* 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