[U-Boot] [PATCH V2 06/21] imximage: add plugin commands

Troy Kisky
2012-09-24T21:46:19Z

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 <troy.kisky@boundarydevices.com>
>> ---
>> 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 <image.h>
>> #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.