From mboxrd@z Thu Jan 1 00:00:00 1970 From: Troy Kisky Date: Mon, 24 Sep 2012 14:46:19 -0700 Subject: [U-Boot] [PATCH V2 06/21] imximage: add plugin commands In-Reply-To: <505F2CDE.3060402@denx.de> References: <1348012989-19674-1-git-send-email-troy.kisky@boundarydevices.com> <1348281558-19520-1-git-send-email-troy.kisky@boundarydevices.com> <1348281558-19520-7-git-send-email-troy.kisky@boundarydevices.com> <505F2CDE.3060402@denx.de> Message-ID: <5060D4AB.2020409@boundarydevices.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >> --- >> 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 >> #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.