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

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

[Peng Fan]

Add plugin support for imximage.

Define CONFIG_USE_IMXIMG_PLUGIN in defconfig to enable using plugin.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Eric Nelson <eric@nelint.com>
Cc: Ye Li <ye.li@nxp.com>
Reviewed-by: Tom Rini <trini@konsulko.com>
---

V3:
 Fix compile error.

V2:
 Drop the CONFIG_USE_PLUGIN, make plugin always support in imximage.

 tools/imximage.c | 282 +++++++++++++++++++++++++++++++++++++++++++------------
 tools/imximage.h |   7 +-
 2 files changed, 229 insertions(+), 60 deletions(-)

diff --git a/tools/imximage.c b/tools/imximage.c
index 092d550..7fa601e 100644
--- a/tools/imximage.c
+++ b/tools/imximage.c
@@ -27,6 +27,7 @@ static table_entry_t imximage_cmds[] = {
 	{CMD_CHECK_BITS_CLR,    "CHECK_BITS_CLR",   "Reg Check bits clr", },
 	{CMD_CSF,               "CSF",           "Command Sequence File", },
 	{CMD_IMAGE_VERSION,     "IMAGE_VERSION",        "image version",  },
+	{CMD_PLUGIN,            "PLUGIN",               "file plugin_addr",  },
 	{-1,                    "",                     "",	          },
 };
 
@@ -80,6 +81,9 @@ static uint32_t imximage_ivt_offset = UNDEFINED;
 static uint32_t imximage_csf_size = UNDEFINED;
 /* Initial Load Region Size */
 static uint32_t imximage_init_loadsize;
+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)
 {
-	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 */
 
-	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);
+		}
+
+		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.
+		 */
+		hdr_v2->boot_data.size = MAX_PLUGIN_CODE_SIZE;
+
+		/* Security feature are not supported */
+		fhdr_v2->csf = 0;
+
+		next_hdr_v2 = (imx_header_v2_t *)((char *)hdr_v2 +
+			       imximage_plugin_size);
+
+		next_fhdr_v2 = &next_hdr_v2->fhdr;
+
+		next_fhdr_v2->header.tag = IVT_HEADER_TAG; /* 0xD1 */
+		next_fhdr_v2->header.length =
+			cpu_to_be16(sizeof(flash_header_v2_t));
+		next_fhdr_v2->header.version = IVT_VERSION; /* 0x40 */
+
+		next_fhdr_v2->entry = entry_point;
+		hdr_base = entry_point - sizeof(struct imx_header);
+		next_fhdr_v2->reserved1 = 0;
+		next_fhdr_v2->reserved2 = 0;
+		next_fhdr_v2->self = hdr_base + imximage_plugin_size;
+
+		next_fhdr_v2->dcd_ptr = 0;
+		next_fhdr_v2->boot_data_ptr = next_fhdr_v2->self +
+				offsetof(imx_header_v2_t, boot_data);
+
+		next_hdr_v2->boot_data.start = hdr_base - flash_offset;
+
+		header_size_ptr = &next_hdr_v2->boot_data.size;
 
-	header_size_ptr = &hdr_v2->boot_data.size;
-	csf_ptr = &fhdr_v2->csf;
+		next_hdr_v2->boot_data.plugin = 0;
+
+		next_fhdr_v2->csf = 0;
+	}
 }
 
 static void set_hdr_func(void)
@@ -393,16 +472,19 @@ static void print_hdr_v2(struct imx_header *imx_hdr)
 {
 	imx_header_v2_t *hdr_v2 = &imx_hdr->header.hdr_v2;
 	flash_header_v2_t *fhdr_v2 = &hdr_v2->fhdr;
-	dcd_v2_t *dcd_v2 = &hdr_v2->dcd_table;
-	uint32_t size, version;
+	dcd_v2_t *dcd_v2 = &hdr_v2->data.dcd_table;
+	uint32_t size, version, plugin;
 
-	size = be16_to_cpu(dcd_v2->header.length);
-	if (size > (MAX_HW_CFG_SIZE_V2 * sizeof(dcd_addr_data_t)) + 8) {
-		fprintf(stderr,
-			"Error: Image corrupt DCD size %d exceed maximum %d\n",
-			(uint32_t)(size / sizeof(dcd_addr_data_t)),
-			MAX_HW_CFG_SIZE_V2);
-		exit(EXIT_FAILURE);
+	plugin = hdr_v2->boot_data.plugin;
+	if (!plugin) {
+		size = be16_to_cpu(dcd_v2->header.length);
+		if (size > (MAX_HW_CFG_SIZE_V2 * sizeof(dcd_addr_data_t))) {
+			fprintf(stderr,
+				"Error: Image corrupt DCD size %d exceed maximum %d\n",
+				(uint32_t)(size / sizeof(dcd_addr_data_t)),
+				MAX_HW_CFG_SIZE_V2);
+			exit(EXIT_FAILURE);
+		}
 	}
 
 	version = detect_imximage_version(imx_hdr);
@@ -410,19 +492,81 @@ static void print_hdr_v2(struct imx_header *imx_hdr)
 	printf("Image Type:   Freescale IMX Boot Image\n");
 	printf("Image Ver:    %x", version);
 	printf("%s\n", get_table_entry_name(imximage_versions, NULL, version));
-	printf("Data Size:    ");
-	genimg_print_size(hdr_v2->boot_data.size);
-	printf("Load Address: %08x\n", (uint32_t)fhdr_v2->boot_data_ptr);
-	printf("Entry Point:  %08x\n", (uint32_t)fhdr_v2->entry);
-	if (fhdr_v2->csf && (imximage_ivt_offset != UNDEFINED) &&
-	    (imximage_csf_size != UNDEFINED)) {
-		printf("HAB Blocks:   %08x %08x %08x\n",
-		       (uint32_t)fhdr_v2->self, 0,
-		       hdr_v2->boot_data.size - imximage_ivt_offset -
-		       imximage_csf_size);
+	printf("Mode:         %s\n", plugin ? "PLUGIN" : "DCD");
+	if (!plugin) {
+		printf("Data Size:    ");
+		genimg_print_size(hdr_v2->boot_data.size);
+		printf("Load Address: %08x\n", (uint32_t)fhdr_v2->boot_data_ptr);
+		printf("Entry Point:  %08x\n", (uint32_t)fhdr_v2->entry);
+		if (fhdr_v2->csf && (imximage_ivt_offset != UNDEFINED) &&
+		    (imximage_csf_size != UNDEFINED)) {
+			printf("HAB Blocks:   %08x %08x %08x\n",
+			       (uint32_t)fhdr_v2->self, 0,
+			       hdr_v2->boot_data.size - imximage_ivt_offset -
+			       imximage_csf_size);
+		}
+	} else {
+		imx_header_v2_t *next_hdr_v2;
+		flash_header_v2_t *next_fhdr_v2;
+
+		/*First Header*/
+		printf("Plugin Data Size:     ");
+		genimg_print_size(hdr_v2->boot_data.size);
+		printf("Plugin Code Size:     ");
+		genimg_print_size(imximage_plugin_size);
+		printf("Plugin Load Address:  %08x\n", hdr_v2->boot_data.start);
+		printf("Plugin Entry Point:   %08x\n", (uint32_t)fhdr_v2->entry);
+
+		/*Second Header*/
+		next_hdr_v2 = (imx_header_v2_t *)((char *)hdr_v2 +
+				imximage_plugin_size);
+		next_fhdr_v2 = &next_hdr_v2->fhdr;
+		printf("U-Boot Data Size:     ");
+		genimg_print_size(next_hdr_v2->boot_data.size);
+		printf("U-Boot Load Address:  %08x\n",
+		       next_hdr_v2->boot_data.start);
+		printf("U-Boot Entry Point:   %08x\n",
+		       (uint32_t)next_fhdr_v2->entry);
 	}
 }
 
+static void copy_plugin_code(struct imx_header *imxhdr, char *plugin_file)
+{
+	int ifd = -1;
+	struct stat sbuf;
+	char *plugin_buf = imxhdr->header.hdr_v2.data.plugin_code;
+	char *ptr;
+
+	ifd = open(plugin_file, O_RDONLY|O_BINARY);
+	if (fstat(ifd, &sbuf) < 0) {
+		fprintf(stderr, "Can't stat %s: %s\n",
+			plugin_file,
+			strerror(errno));
+		exit(EXIT_FAILURE);
+	}
+
+	ptr = mmap(0, sbuf.st_size, PROT_READ, MAP_SHARED, ifd, 0);
+	if (ptr == MAP_FAILED) {
+		fprintf(stderr, "Can't read %s: %s\n",
+			plugin_file,
+			strerror(errno));
+		exit(EXIT_FAILURE);
+	}
+
+	if (sbuf.st_size > MAX_PLUGIN_CODE_SIZE) {
+		printf("plugin binary size too large\n");
+		exit(EXIT_FAILURE);
+	}
+
+	memcpy(plugin_buf, ptr, sbuf.st_size);
+	imximage_plugin_size = sbuf.st_size;
+
+	(void) munmap((void *)ptr, sbuf.st_size);
+	(void) close(ifd);
+
+	imxhdr->header.hdr_v2.boot_data.plugin = 1;
+}
+
 static void parse_cfg_cmd(struct imx_header *imxhdr, int32_t cmd, char *token,
 				char *name, int lineno, int fld, int dcd_len)
 {
@@ -497,6 +641,10 @@ static void parse_cfg_cmd(struct imx_header *imxhdr, int32_t cmd, char *token,
 		if (unlikely(cmd_ver_first != 1))
 			cmd_ver_first = 0;
 		break;
+	case CMD_PLUGIN:
+		plugin_image = 1;
+		copy_plugin_code(imxhdr, token);
+		break;
 	}
 }
 
@@ -542,6 +690,10 @@ static void parse_cfg_fld(struct imx_header *imxhdr, int32_t *cmd,
 				}
 			}
 			break;
+		case CMD_PLUGIN:
+			value = get_cfg_value(token, name, lineno);
+			imximage_iram_free_start = value;
+			break;
 		default:
 			break;
 		}
@@ -649,6 +801,7 @@ static void imximage_set_header(void *ptr, struct stat *sbuf, int ifd,
 {
 	struct imx_header *imxhdr = (struct imx_header *)ptr;
 	uint32_t dcd_len;
+	uint32_t header_size;
 
 	/*
 	 * In order to not change the old imx cfg file
@@ -665,10 +818,15 @@ static void imximage_set_header(void *ptr, struct stat *sbuf, int ifd,
 	dcd_len = parse_cfg_file(imxhdr, params->imagename);
 
 	if (imximage_version == IMXIMAGE_V2) {
-		if (imximage_init_loadsize < imximage_ivt_offset +
-			sizeof(imx_header_v2_t))
+		header_size = sizeof(flash_header_v2_t) + sizeof(boot_data_t);
+		if (!plugin_image)
+			header_size += sizeof(dcd_v2_t);
+		else
+			header_size += MAX_PLUGIN_CODE_SIZE;
+
+		if (imximage_init_loadsize < imximage_ivt_offset + header_size)
 				imximage_init_loadsize = imximage_ivt_offset +
-					sizeof(imx_header_v2_t);
+					header_size;
 	}
 
 	/* Set the imx header */
@@ -721,7 +879,7 @@ static int imximage_generate(struct image_tool_params *params,
 	size_t alloc_len;
 	struct stat sbuf;
 	char *datafile = params->datafile;
-	uint32_t pad_len;
+	uint32_t pad_len, header_size;
 
 	memset(&imximage_header, 0, sizeof(imximage_header));
 
@@ -742,15 +900,21 @@ static int imximage_generate(struct image_tool_params *params,
 	/* TODO: check i.MX image V1 handling, for now use 'old' style */
 	if (imximage_version == IMXIMAGE_V1) {
 		alloc_len = 4096;
+		header_size = 4096;
 	} else {
-		if (imximage_init_loadsize < imximage_ivt_offset +
-			sizeof(imx_header_v2_t))
+		header_size = sizeof(flash_header_v2_t) + sizeof(boot_data_t);
+		if (!plugin_image)
+			header_size += sizeof(dcd_v2_t);
+		else
+			header_size += MAX_PLUGIN_CODE_SIZE;
+
+		if (imximage_init_loadsize < imximage_ivt_offset + header_size)
 				imximage_init_loadsize = imximage_ivt_offset +
-					sizeof(imx_header_v2_t);
+					header_size;
 		alloc_len = imximage_init_loadsize - imximage_ivt_offset;
 	}
 
-	if (alloc_len < sizeof(struct imx_header)) {
+	if (alloc_len < header_size) {
 		fprintf(stderr, "%s: header error\n",
 			params->cmdname);
 		exit(EXIT_FAILURE);
diff --git a/tools/imximage.h b/tools/imximage.h
index c7b9b5c..fe4dd89 100644
--- a/tools/imximage.h
+++ b/tools/imximage.h
@@ -9,6 +9,7 @@
 #define _IMXIMAGE_H_
 
 #define MAX_HW_CFG_SIZE_V2 220 /* Max number of registers imx can set for v2 */
+#define MAX_PLUGIN_CODE_SIZE (16*1024)
 #define MAX_HW_CFG_SIZE_V1 60  /* Max number of registers imx can set for v1 */
 #define APP_CODE_BARKER	0xB1
 #define DCD_BARKER	0xB17219E9
@@ -64,6 +65,7 @@ enum imximage_cmd {
 	CMD_CHECK_BITS_SET,
 	CMD_CHECK_BITS_CLR,
 	CMD_CSF,
+	CMD_PLUGIN,
 };
 
 enum imximage_fld_types {
@@ -164,7 +166,10 @@ typedef struct {
 typedef struct {
 	flash_header_v2_t fhdr;
 	boot_data_t boot_data;
-	dcd_v2_t dcd_table;
+	union {
+		dcd_v2_t dcd_table;
+		char plugin_code[MAX_PLUGIN_CODE_SIZE];
+	} data;
 } imx_header_v2_t;
 
 /* The header must be aligned to 4k on MX53 for NAND boot */
-- 
2.6.2

[U-Boot] [PATCH V3 2/8] imx: mx6: Add plugin support

[Peng Fan]

Add mx6_plugin.S which calls boot rom setup function, generate the second ivt,
and jump back to boot rom.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
Signed-off-by: Ye Li <ye.li@nxp.com>
Signed-off-by: Utkarsh Gupta <utkarsh.gupta@nxp.com>
Reviewed-by: Tom Rini <trini@konsulko.com>
Acked-by: Utkarsh Gupta <utkarsh.gupta@nxp.com>
---

V3:
 None

V2:
 None

 arch/arm/include/asm/arch-mx6/mx6_plugin.S | 159 +++++++++++++++++++++++++++++
 1 file changed, 159 insertions(+)
 create mode 100644 arch/arm/include/asm/arch-mx6/mx6_plugin.S

diff --git a/arch/arm/include/asm/arch-mx6/mx6_plugin.S b/arch/arm/include/asm/arch-mx6/mx6_plugin.S
new file mode 100644
index 0000000..b7d1b20
--- /dev/null
+++ b/arch/arm/include/asm/arch-mx6/mx6_plugin.S
@@ -0,0 +1,159 @@
+/*
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <config.h>
+
+#ifdef CONFIG_ROM_UNIFIED_SECTIONS
+#define ROM_API_TABLE_BASE_ADDR_LEGACY		0x180
+#define ROM_VERSION_OFFSET               	0x80
+#else
+#define ROM_API_TABLE_BASE_ADDR_LEGACY		0xC0
+#define ROM_VERSION_OFFSET               	0x48
+#endif
+#define ROM_API_TABLE_BASE_ADDR_MX6DQ_TO15	0xC4
+#define ROM_API_TABLE_BASE_ADDR_MX6DL_TO12	0xC4
+#define ROM_API_HWCNFG_SETUP_OFFSET		0x08
+#define ROM_VERSION_TO10			0x10
+#define ROM_VERSION_TO12			0x12
+#define ROM_VERSION_TO15			0x15
+
+plugin_start:
+
+	push    {r0-r4, lr}
+
+	imx6_ddr_setting
+	imx6_clock_gating
+	imx6_qos_setting
+
+/*
+ * The following is to fill in those arguments for this ROM function
+ * pu_irom_hwcnfg_setup(void **start, size_t *bytes, const void *boot_data)
+ * This function is used to copy data from the storage media into DDR.
+ * start - Initial (possibly partial) image load address on entry.
+ *         Final image load address on exit.
+ * bytes - Initial (possibly partial) image size on entry.
+ *         Final image size on exit.
+ * boot_data - Initial @ref ivt Boot Data load address.
+ */
+	adr r0, boot_data2
+	adr r1, image_len2
+	adr r2, boot_data2
+
+#ifdef CONFIG_NOR_BOOT
+#ifdef CONFIG_MX6SX
+	ldr r3, =ROM_VERSION_OFFSET
+	ldr r4, [r3]
+	cmp r4, #ROM_VERSION_TO10
+	bgt before_calling_rom___pu_irom_hwcnfg_setup
+	ldr r3, =0x00900b00
+	ldr r4, =0x50000000
+	str r4, [r3, #0x5c]
+#else
+	ldr r3, =0x00900800
+	ldr r4, =0x08000000
+	str r4, [r3, #0xc0]
+#endif
+#endif
+
+/*
+ * check the _pu_irom_api_table for the address
+ */
+before_calling_rom___pu_irom_hwcnfg_setup:
+	ldr r3, =ROM_VERSION_OFFSET
+	ldr r4, [r3]
+#if defined(CONFIG_MX6SOLO) || defined(CONFIG_MX6DL)
+	ldr r3, =ROM_VERSION_TO12
+	cmp r4, r3
+	ldrge r3, =ROM_API_TABLE_BASE_ADDR_MX6DL_TO12
+	ldrlt r3, =ROM_API_TABLE_BASE_ADDR_LEGACY
+#elif defined(CONFIG_MX6Q)
+	ldr r3, =ROM_VERSION_TO15
+	cmp r4, r3
+	ldrge r3, =ROM_API_TABLE_BASE_ADDR_MX6DQ_TO15
+	ldrlt r3, =ROM_API_TABLE_BASE_ADDR_LEGACY
+#else
+	ldr r3, =ROM_API_TABLE_BASE_ADDR_LEGACY
+#endif
+	ldr r4, [r3, #ROM_API_HWCNFG_SETUP_OFFSET]
+	blx r4
+after_calling_rom___pu_irom_hwcnfg_setup:
+
+/*
+ * ROM_API_HWCNFG_SETUP function enables MMU & Caches.
+ * Thus disable MMU & Caches.
+ */
+
+	mrc     p15, 0, r0, c1, c0, 0   /* read CP15 register 1 into r0*/
+	ands    r0, r0, #0x1            /* check if MMU is enabled */
+	beq     mmu_disable_notreq      /* exit if MMU is already disabled */
+
+	/* Disable caches, MMU */
+	mrc     p15, 0, r0, c1, c0, 0	/* read CP15 register 1 into r0 */
+	bic     r0, r0, #(1 << 2)	/* disable D Cache */
+	bic     r0, r0, #0x1		/* clear bit 0 ; MMU off */
+
+	bic     r0, r0, #(0x1 << 11)	/* disable Z, branch prediction */
+	bic     r0, r0, #(0x1 << 1)	/* disable A, Strict alignment */
+					/* check enabled. */
+	mcr     p15, 0, r0, c1, c0, 0	/* write CP15 register 1 */
+	mov     r0, r0
+	mov     r0, r0
+	mov     r0, r0
+	mov     r0, r0
+
+mmu_disable_notreq:
+    NOP
+
+/* To return to ROM from plugin, we need to fill in these argument.
+ * Here is what need to do:
+ * Need to construct the paramters for this function before return to ROM:
+ * plugin_download(void **start, size_t *bytes, UINT32 *ivt_offset)
+ */
+	pop {r0-r4, lr}
+	push {r5}
+	ldr r5, boot_data2
+	str r5, [r0]
+	ldr r5, image_len2
+	str r5, [r1]
+	ldr r5, second_ivt_offset
+	str r5, [r2]
+	mov r0, #1
+	pop {r5}
+
+	/* return back to ROM code */
+	bx lr
+
+/* make the following data right in the end of the output*/
+.ltorg
+
+#if (defined(CONFIG_NOR_BOOT) || defined(CONFIG_QSPI_BOOT))
+#define FLASH_OFFSET 0x1000
+#else
+#define FLASH_OFFSET 0x400
+#endif
+
+/*
+ * second_ivt_offset is the offset from the "second_ivt_header" to
+ * "image_copy_start", which involves FLASH_OFFSET, plus the first
+ * ivt_header, the plugin code size itself recorded by "ivt2_header"
+ */
+
+second_ivt_offset:      .long (ivt2_header + 0x2C + FLASH_OFFSET)
+
+/*
+ * The following is the second IVT header plus the second boot data
+ */
+ivt2_header:            .long 0x0
+app2_code_jump_v:       .long 0x0
+reserv3:                .long 0x0
+dcd2_ptr:               .long 0x0
+boot_data2_ptr:         .long 0x0
+self_ptr2:              .long 0x0
+app_code_csf2:          .long 0x0
+reserv4:                .long 0x0
+boot_data2:             .long 0x0
+image_len2:             .long 0x0
+plugin2:                .long 0x0
-- 
2.6.2

[U-Boot] [PATCH V3 3/8] imx: mx7: Add plugin support

[Peng Fan]

Add mx7_plugin.S which calls boot rom setup function, generate the second ivt,
and jump back to boot rom.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
Signed-off-by: Ye Li <ye.li@nxp.com>
Cc: Stefano Babic <sbabic@denx.de>
Reviewed-by: Tom Rini <trini@konsulko.com>
---

V3:
 None

V2:
 None

 arch/arm/include/asm/arch-mx7/mx7_plugin.S | 111 +++++++++++++++++++++++++++++
 1 file changed, 111 insertions(+)
 create mode 100644 arch/arm/include/asm/arch-mx7/mx7_plugin.S

diff --git a/arch/arm/include/asm/arch-mx7/mx7_plugin.S b/arch/arm/include/asm/arch-mx7/mx7_plugin.S
new file mode 100644
index 0000000..41336b4
--- /dev/null
+++ b/arch/arm/include/asm/arch-mx7/mx7_plugin.S
@@ -0,0 +1,111 @@
+/*
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <config.h>
+
+#define ROM_API_TABLE_BASE_ADDR_LEGACY		0x180
+#define ROM_VERSION_OFFSET               	0x80
+#define ROM_API_HWCNFG_SETUP_OFFSET		0x08
+
+plugin_start:
+
+	push    {r0-r4, lr}
+
+	imx7_ddr_setting
+	imx7_clock_gating
+	imx7_qos_setting
+
+/*
+ * Check if we are in USB serial download mode and immediately return to ROM
+ * Need to check USB CTRL clock firstly, then check the USBx_nASYNCLISTADDR
+ */
+	ldr r0, =0x30384680
+	ldr r1, [r0]
+	cmp r1, #0
+	beq normal_boot
+
+	ldr r0, =0x30B10158
+	ldr r1, [r0]
+	cmp r1, #0
+	beq normal_boot
+
+	pop {r0-r4, lr}
+	bx lr
+
+normal_boot:
+
+/*
+ * The following is to fill in those arguments for this ROM function
+ * pu_irom_hwcnfg_setup(void **start, size_t *bytes, const void *boot_data)
+ * This function is used to copy data from the storage media into DDR.
+ * start - Initial (possibly partial) image load address on entry.
+ *         Final image load address on exit.
+ * bytes - Initial (possibly partial) image size on entry.
+ *         Final image size on exit.
+ * boot_data - Initial @ref ivt Boot Data load address.
+ */
+	adr r0, boot_data2
+	adr r1, image_len2
+	adr r2, boot_data2
+
+/*
+ * check the _pu_irom_api_table for the address
+ */
+before_calling_rom___pu_irom_hwcnfg_setup:
+	ldr r3, =ROM_VERSION_OFFSET
+	ldr r4, [r3]
+	ldr r3, =ROM_API_TABLE_BASE_ADDR_LEGACY
+	ldr r4, [r3, #ROM_API_HWCNFG_SETUP_OFFSET]
+	blx r4
+after_calling_rom___pu_irom_hwcnfg_setup:
+
+
+/* To return to ROM from plugin, we need to fill in these argument.
+ * Here is what need to do:
+ * Need to construct the paramters for this function before return to ROM:
+ * plugin_download(void **start, size_t *bytes, UINT32 *ivt_offset)
+ */
+	pop {r0-r4, lr}
+	push {r5}
+	ldr r5, boot_data2
+	str r5, [r0]
+	ldr r5, image_len2
+	str r5, [r1]
+	ldr r5, second_ivt_offset
+	str r5, [r2]
+	mov r0, #1
+	pop {r5}
+
+	/* return back to ROM code */
+	bx lr
+
+/* make the following data right in the end of the output*/
+.ltorg
+
+#define FLASH_OFFSET 0x400
+
+/*
+ * second_ivt_offset is the offset from the "second_ivt_header" to
+ * "image_copy_start", which involves FLASH_OFFSET, plus the first
+ * ivt_header, the plugin code size itself recorded by "ivt2_header"
+ */
+
+second_ivt_offset:      .long (ivt2_header + 0x2C + FLASH_OFFSET)
+
+/*
+ * The following is the second IVT header plus the second boot data
+ */
+ivt2_header:            .long 0x0
+app2_code_jump_v:       .long 0x0
+reserv3:                .long 0x0
+dcd2_ptr:               .long 0x0
+boot_data2_ptr:         .long 0x0
+self_ptr2:              .long 0x0
+app_code_csf2:          .long 0x0
+reserv4:                .long 0x0
+boot_data2:             .long 0x0
+image_len2:             .long 0x0
+plugin2:                .long 0x0
-- 
2.6.2

[U-Boot] [PATCH V3 4/8] imx-common: inctroude USE_IMXIMG_PLUGIN Kconfig

[Peng Fan]

Introduce USE_IMXIMG_PLUGIN Kconfig

Signed-off-by: Peng Fan <peng.fan@nxp.com>
Cc: Stefano Babic <sbabic@denx.de>
Reviewed-by: Tom Rini <trini@konsulko.com>
---

V3:
 None

V2:
 New

 arch/arm/imx-common/Kconfig | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/imx-common/Kconfig b/arch/arm/imx-common/Kconfig
index 1b7da5a..85eac57 100644
--- a/arch/arm/imx-common/Kconfig
+++ b/arch/arm/imx-common/Kconfig
@@ -17,3 +17,10 @@ config IMX_BOOTAUX
 	depends on ARCH_MX7 || ARCH_MX6
 	help
 	  bootaux [addr] to boot auxiliary core.
+
+config USE_IMXIMG_PLUGIN
+	bool "Using imximage plugin code"
+	depends on ARCH_MX7 || ARCH_MX6
+	help
+	  i.MX6/7 supports DCD and Plugin. Enable this configuration
+	  to use Plugin, otherwise DCD will be used.
-- 
2.6.2

[U-Boot] [PATCH V3 5/8] imx-common: compile plugin code

[Peng Fan]

If CONFIG_USE_IMXIMG_PLUGIN is selected, plugin.bin will be
generated under board/$(BOARDDIR)/.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
Cc: Stefano Babic <sbabic@denx.de>
Reviewed-by: Tom Rini <trini@konsulko.com>
---

V3:
 None

V2:
  New. Make this common to all i.MX6/7, but not duplicated in board makefile.

 arch/arm/imx-common/Makefile | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/arch/arm/imx-common/Makefile b/arch/arm/imx-common/Makefile
index d34a784..1873185 100644
--- a/arch/arm/imx-common/Makefile
+++ b/arch/arm/imx-common/Makefile
@@ -38,6 +38,23 @@ obj-$(CONFIG_CMD_BMODE) += cmd_bmode.o
 obj-$(CONFIG_CMD_HDMIDETECT) += cmd_hdmidet.o
 obj-$(CONFIG_CMD_DEKBLOB) += cmd_dek.o
 
+PLUGIN = board/$(BOARDDIR)/plugin
+
+ifeq ($(CONFIG_USE_IMXIMG_PLUGIN),y)
+
+$(PLUGIN).o: $(PLUGIN).S FORCE
+	$(Q)mkdir -p $(dir $@)
+	$(call if_changed_dep,as_o_S)
+
+$(PLUGIN).bin: $(PLUGIN).o FORCE
+	$(Q)mkdir -p $(dir $@)
+	$(OBJCOPY) -O binary --gap-fill 0xff $< $@
+else
+
+$(PLUGIN).bin:
+
+endif
+
 quiet_cmd_cpp_cfg = CFGS    $@
       cmd_cpp_cfg = $(CPP) $(cpp_flags) -x c -o $@ $<
 
@@ -47,24 +64,24 @@ $(IMX_CONFIG): %.cfgtmp: % FORCE
 	$(Q)mkdir -p $(dir $@)
 	$(call if_changed_dep,cpp_cfg)
 
-MKIMAGEFLAGS_u-boot.imx = -n $(filter-out $< $(PHONY),$^) -T imximage \
+MKIMAGEFLAGS_u-boot.imx = -n $(filter-out $(PLUGIN).bin $< $(PHONY),$^) -T imximage \
 	-e $(CONFIG_SYS_TEXT_BASE)
 
-u-boot.imx: u-boot.bin $(IMX_CONFIG) FORCE
+u-boot.imx: u-boot.bin $(IMX_CONFIG) $(PLUGIN).bin FORCE
 	$(call if_changed,mkimage)
 
 ifeq ($(CONFIG_OF_SEPARATE),y)
-MKIMAGEFLAGS_u-boot-dtb.imx = -n $(filter-out $< $(PHONY),$^) -T imximage \
+MKIMAGEFLAGS_u-boot-dtb.imx = -n $(filter-out $(PLUGIN).bin $< $(PHONY),$^) -T imximage \
 	-e $(CONFIG_SYS_TEXT_BASE)
 
-u-boot-dtb.imx: u-boot-dtb.bin $(IMX_CONFIG) FORCE
+u-boot-dtb.imx: u-boot-dtb.bin $(IMX_CONFIG) $(PLUGIN).bin FORCE
 	$(call if_changed,mkimage)
 endif
 
-MKIMAGEFLAGS_SPL = -n $(filter-out $< $(PHONY),$^) -T imximage \
+MKIMAGEFLAGS_SPL = -n $(filter-out $(PLUGIN).bin $< $(PHONY),$^) -T imximage \
 	-e $(CONFIG_SPL_TEXT_BASE)
 
-SPL: spl/u-boot-spl.bin $(IMX_CONFIG) FORCE
+SPL: spl/u-boot-spl.bin $(IMX_CONFIG) $(PLUGIN).bin FORCE
 	$(call if_changed,mkimage)
 
 MKIMAGEFLAGS_u-boot.uim = -A arm -O U-Boot -a $(CONFIG_SYS_TEXT_BASE) \
-- 
2.6.2

[U-Boot] [PATCH V3 6/8] imx: mx6ullevk: support plugin

[Peng Fan]

Add plugin code for mx6ullevk.
Define CONFIG_USE_IMXIMG_PLUGIN in defconfig file to use plugin code.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
Cc: Stefano Babic <sbabic@denx.de>
---

V3:
 Update commit log

V2:
 None

 board/freescale/mx6ullevk/imximage.cfg |   2 +-
 board/freescale/mx6ullevk/plugin.S     | 139 +++++++++++++++++++++++++++++++++
 2 files changed, 140 insertions(+), 1 deletion(-)
 create mode 100644 board/freescale/mx6ullevk/plugin.S

diff --git a/board/freescale/mx6ullevk/imximage.cfg b/board/freescale/mx6ullevk/imximage.cfg
index 4604b62..1a21b49 100644
--- a/board/freescale/mx6ullevk/imximage.cfg
+++ b/board/freescale/mx6ullevk/imximage.cfg
@@ -29,7 +29,7 @@ BOOT_FROM	nor
 BOOT_FROM	sd
 #endif
 
-#ifdef CONFIG_USE_PLUGIN
+#ifdef CONFIG_USE_IMXIMG_PLUGIN
 /*PLUGIN    plugin-binary-file    IRAM_FREE_START_ADDR*/
 PLUGIN	board/freescale/mx6ullevk/plugin.bin 0x00907000
 #else
diff --git a/board/freescale/mx6ullevk/plugin.S b/board/freescale/mx6ullevk/plugin.S
new file mode 100644
index 0000000..65a3c45
--- /dev/null
+++ b/board/freescale/mx6ullevk/plugin.S
@@ -0,0 +1,139 @@
+/*
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <config.h>
+
+/* DDR script */
+.macro imx6ull_ddr3_evk_setting
+	ldr r0, =IOMUXC_BASE_ADDR
+	ldr r1, =0x000C0000
+	str r1, [r0, #0x4B4]
+	ldr r1, =0x00000000
+	str r1, [r0, #0x4AC]
+	ldr r1, =0x00000030
+	str r1, [r0, #0x27C]
+	ldr r1, =0x00000030
+	str r1, [r0, #0x250]
+	str r1, [r0, #0x24C]
+	str r1, [r0, #0x490]
+	ldr r1, =0x000C0030
+	str r1, [r0, #0x288]
+
+	ldr r1, =0x00000000
+	str r1, [r0, #0x270]
+
+	ldr r1, =0x00000030
+	str r1, [r0, #0x260]
+	str r1, [r0, #0x264]
+	str r1, [r0, #0x4A0]
+
+	ldr r1, =0x00020000
+	str r1, [r0, #0x494]
+
+	ldr r1, =0x00000030
+	str r1, [r0, #0x280]
+	ldr r1, =0x00000030
+	str r1, [r0, #0x284]
+
+	ldr r1, =0x00020000
+	str r1, [r0, #0x4B0]
+
+	ldr r1, =0x00000030
+	str r1, [r0, #0x498]
+	str r1, [r0, #0x4A4]
+	str r1, [r0, #0x244]
+	str r1, [r0, #0x248]
+
+	ldr r0, =MMDC_P0_BASE_ADDR
+	ldr r1, =0x00008000
+	str r1, [r0, #0x1C]
+	ldr r1, =0xA1390003
+	str r1, [r0, #0x800]
+	ldr r1, =0x00000004
+	str r1, [r0, #0x80C]
+	ldr r1, =0x41640158
+	str r1, [r0, #0x83C]
+	ldr r1, =0x40403237
+	str r1, [r0, #0x848]
+	ldr r1, =0x40403C33
+	str r1, [r0, #0x850]
+	ldr r1, =0x33333333
+	str r1, [r0, #0x81C]
+	str r1, [r0, #0x820]
+	ldr r1, =0xF3333333
+	str r1, [r0, #0x82C]
+	str r1, [r0, #0x830]
+	ldr r1, =0x00944009
+	str r1, [r0, #0x8C0]
+	ldr r1, =0x00000800
+	str r1, [r0, #0x8B8]
+	ldr r1, =0x0002002D
+	str r1, [r0, #0x004]
+	ldr r1, =0x1B333030
+	str r1, [r0, #0x008]
+	ldr r1, =0x676B52F3
+	str r1, [r0, #0x00C]
+	ldr r1, =0xB66D0B63
+	str r1, [r0, #0x010]
+	ldr r1, =0x01FF00DB
+	str r1, [r0, #0x014]
+	ldr r1, =0x00201740
+	str r1, [r0, #0x018]
+	ldr r1, =0x00008000
+	str r1, [r0, #0x01C]
+	ldr r1, =0x000026D2
+	str r1, [r0, #0x02C]
+	ldr r1, =0x006B1023
+	str r1, [r0, #0x030]
+	ldr r1, =0x0000004F
+	str r1, [r0, #0x040]
+	ldr r1, =0x84180000
+	str r1, [r0, #0x000]
+	ldr r1, =0x00400000
+	str r1, [r0, #0x890]
+	ldr r1, =0x02008032
+	str r1, [r0, #0x01C]
+	ldr r1, =0x00008033
+	str r1, [r0, #0x01C]
+	ldr r1, =0x00048031
+	str r1, [r0, #0x01C]
+	ldr r1, =0x15208030
+	str r1, [r0, #0x01C]
+	ldr r1, =0x04008040
+	str r1, [r0, #0x01C]
+	ldr r1, =0x00000800
+	str r1, [r0, #0x020]
+	ldr r1, =0x00000227
+	str r1, [r0, #0x818]
+	ldr r1, =0x0002552D
+	str r1, [r0, #0x004]
+	ldr r1, =0x00011006
+	str r1, [r0, #0x404]
+	ldr r1, =0x00000000
+	str r1, [r0, #0x01C]
+.endm
+
+.macro imx6_clock_gating
+	ldr r0, =CCM_BASE_ADDR
+	ldr r1, =0xFFFFFFFF
+	str r1, [r0, #0x68]
+	str r1, [r0, #0x6C]
+	str r1, [r0, #0x70]
+	str r1, [r0, #0x74]
+	str r1, [r0, #0x78]
+	str r1, [r0, #0x7C]
+	str r1, [r0, #0x80]
+.endm
+
+.macro imx6_qos_setting
+.endm
+
+.macro imx6_ddr_setting
+	imx6ull_ddr3_evk_setting
+.endm
+
+/* include the common plugin code here */
+#include <asm/arch/mx6_plugin.S>
-- 
2.6.2

[U-Boot] [PATCH V3 7/8] imx: mx6ullevk: correct boot device macro

[Peng Fan]

Correct boot device macro according to kconfig entry
in common/Kconfig

Signed-off-by: Peng Fan <peng.fan@nxp.com>
Cc: Stefano Babic <sbabic@denx.de>
---

V3:
 None

V2:
 None

 board/freescale/mx6ullevk/imximage.cfg | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/board/freescale/mx6ullevk/imximage.cfg b/board/freescale/mx6ullevk/imximage.cfg
index 1a21b49..3c219fa 100644
--- a/board/freescale/mx6ullevk/imximage.cfg
+++ b/board/freescale/mx6ullevk/imximage.cfg
@@ -21,9 +21,9 @@ IMAGE_VERSION 2
  * spi/sd/nand/onenand, qspi/nor
  */
 
-#ifdef CONFIG_SYS_BOOT_QSPI
+#ifdef CONFIG_QSPI_BOOT
 BOOT_FROM	qspi
-#elif defined(CONFIG_SYS_BOOT_EIMNOR)
+#elif defined(CONFIG_NOR_BOOT)
 BOOT_FROM	nor
 #else
 BOOT_FROM	sd
-- 
2.6.2

[U-Boot] [PATCH V3 8/8] imx: mx6ull_14x14_evk: add plugin defconfig

[Peng Fan]

Add defconfig file to use plugin code.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
Cc: Stefano Babic <sbabic@denx.de>
---

V3:
 None

V2:
 New

 configs/mx6ull_14x14_evk_plugin_defconfig | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 configs/mx6ull_14x14_evk_plugin_defconfig

diff --git a/configs/mx6ull_14x14_evk_plugin_defconfig b/configs/mx6ull_14x14_evk_plugin_defconfig
new file mode 100644
index 0000000..ff15c8e
--- /dev/null
+++ b/configs/mx6ull_14x14_evk_plugin_defconfig
@@ -0,0 +1,31 @@
+CONFIG_ARM=y
+CONFIG_ARCH_MX6=y
+CONFIG_TARGET_MX6ULL_14X14_EVK=y
+CONFIG_USE_IMXIMG_PLUGIN=y
+CONFIG_DEFAULT_DEVICE_TREE="imx6ull-14x14-evk"
+CONFIG_SYS_EXTRA_OPTIONS="IMX_CONFIG=board/freescale/mx6ullevk/imximage.cfg"
+CONFIG_BOOTDELAY=3
+CONFIG_HUSH_PARSER=y
+CONFIG_CMD_BOOTZ=y
+# CONFIG_CMD_IMLS is not set
+CONFIG_CMD_MEMTEST=y
+CONFIG_CMD_MMC=y
+CONFIG_CMD_I2C=y
+CONFIG_CMD_GPIO=y
+CONFIG_CMD_DHCP=y
+CONFIG_CMD_PING=y
+CONFIG_CMD_CACHE=y
+CONFIG_CMD_EXT2=y
+CONFIG_CMD_EXT4=y
+CONFIG_CMD_EXT4_WRITE=y
+CONFIG_CMD_FAT=y
+CONFIG_CMD_FS_GENERIC=y
+CONFIG_OF_CONTROL=y
+CONFIG_DM_GPIO=y
+CONFIG_DM_74X164=y
+CONFIG_DM_I2C=y
+CONFIG_DM_MMC=y
+CONFIG_PINCTRL=y
+CONFIG_PINCTRL_IMX6=y
+CONFIG_DM_REGULATOR=y
+CONFIG_DM_SPI=y
-- 
2.6.2

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

[Eric Nelson]

Hi Peng,

I'm sorry for taking so long to go though this.

On 10/08/2016 08:58 AM, Peng Fan wrote:
> Add plugin support for imximage.
> 

This CONFIG setting doesn't actually affect mkimage or imximage:

> Define CONFIG_USE_IMXIMG_PLUGIN in defconfig to enable using plugin.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Eric Nelson <eric@nelint.com>
> Cc: Ye Li <ye.li@nxp.com>
> Reviewed-by: Tom Rini <trini@konsulko.com>
> ---
> 
> V3:
>  Fix compile error.
> 
> V2:
>  Drop the CONFIG_USE_PLUGIN, make plugin always support in imximage.
> 
>  tools/imximage.c | 282 +++++++++++++++++++++++++++++++++++++++++++------------
>  tools/imximage.h |   7 +-
>  2 files changed, 229 insertions(+), 60 deletions(-)
> 
> diff --git a/tools/imximage.c b/tools/imximage.c
> index 092d550..7fa601e 100644
> --- a/tools/imximage.c
> +++ b/tools/imximage.c
> @@ -27,6 +27,7 @@ static table_entry_t imximage_cmds[] = {
>  	{CMD_CHECK_BITS_CLR,    "CHECK_BITS_CLR",   "Reg Check bits clr", },
>  	{CMD_CSF,               "CSF",           "Command Sequence File", },
>  	{CMD_IMAGE_VERSION,     "IMAGE_VERSION",        "image version",  },
> +	{CMD_PLUGIN,            "PLUGIN",               "file plugin_addr",  },
>  	{-1,                    "",                     "",	          },
>  };
>  
> @@ -80,6 +81,9 @@ static uint32_t imximage_ivt_offset = UNDEFINED;
>  static uint32_t imximage_csf_size = UNDEFINED;
>  /* Initial Load Region Size */
>  static uint32_t imximage_init_loadsize;

These seem very limiting.

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.

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

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.

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

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

> +		hdr_v2->boot_data.size = MAX_PLUGIN_CODE_SIZE;
> +
> +		/* Security feature are not supported */
> +		fhdr_v2->csf = 0;
> +
> +		next_hdr_v2 = (imx_header_v2_t *)((char *)hdr_v2 +
> +			       imximage_plugin_size);
> +
> +		next_fhdr_v2 = &next_hdr_v2->fhdr;
> +
> +		next_fhdr_v2->header.tag = IVT_HEADER_TAG; /* 0xD1 */
> +		next_fhdr_v2->header.length =
> +			cpu_to_be16(sizeof(flash_header_v2_t));
> +		next_fhdr_v2->header.version = IVT_VERSION; /* 0x40 */
> +
> +		next_fhdr_v2->entry = entry_point;
> +		hdr_base = entry_point - sizeof(struct imx_header);
> +		next_fhdr_v2->reserved1 = 0;
> +		next_fhdr_v2->reserved2 = 0;
> +		next_fhdr_v2->self = hdr_base + imximage_plugin_size;
> +
> +		next_fhdr_v2->dcd_ptr = 0;
> +		next_fhdr_v2->boot_data_ptr = next_fhdr_v2->self +
> +				offsetof(imx_header_v2_t, boot_data);
> +
> +		next_hdr_v2->boot_data.start = hdr_base - flash_offset;
> +
> +		header_size_ptr = &next_hdr_v2->boot_data.size;
>  
> -	header_size_ptr = &hdr_v2->boot_data.size;
> -	csf_ptr = &fhdr_v2->csf;
> +		next_hdr_v2->boot_data.plugin = 0;
> +
> +		next_fhdr_v2->csf = 0;
> +	}
>  }
>  
>  static void set_hdr_func(void)
> @@ -393,16 +472,19 @@ static void print_hdr_v2(struct imx_header *imx_hdr)
>  {
>  	imx_header_v2_t *hdr_v2 = &imx_hdr->header.hdr_v2;
>  	flash_header_v2_t *fhdr_v2 = &hdr_v2->fhdr;
> -	dcd_v2_t *dcd_v2 = &hdr_v2->dcd_table;
> -	uint32_t size, version;
> +	dcd_v2_t *dcd_v2 = &hdr_v2->data.dcd_table;
> +	uint32_t size, version, plugin;
>  
> -	size = be16_to_cpu(dcd_v2->header.length);
> -	if (size > (MAX_HW_CFG_SIZE_V2 * sizeof(dcd_addr_data_t)) + 8) {
> -		fprintf(stderr,
> -			"Error: Image corrupt DCD size %d exceed maximum %d\n",
> -			(uint32_t)(size / sizeof(dcd_addr_data_t)),
> -			MAX_HW_CFG_SIZE_V2);
> -		exit(EXIT_FAILURE);
> +	plugin = hdr_v2->boot_data.plugin;
> +	if (!plugin) {
> +		size = be16_to_cpu(dcd_v2->header.length);
> +		if (size > (MAX_HW_CFG_SIZE_V2 * sizeof(dcd_addr_data_t))) {
> +			fprintf(stderr,
> +				"Error: Image corrupt DCD size %d exceed maximum %d\n",
> +				(uint32_t)(size / sizeof(dcd_addr_data_t)),
> +				MAX_HW_CFG_SIZE_V2);
> +			exit(EXIT_FAILURE);
> +		}
>  	}
>  
>  	version = detect_imximage_version(imx_hdr);
> @@ -410,19 +492,81 @@ static void print_hdr_v2(struct imx_header *imx_hdr)
>  	printf("Image Type:   Freescale IMX Boot Image\n");
>  	printf("Image Ver:    %x", version);
>  	printf("%s\n", get_table_entry_name(imximage_versions, NULL, version));
> -	printf("Data Size:    ");
> -	genimg_print_size(hdr_v2->boot_data.size);
> -	printf("Load Address: %08x\n", (uint32_t)fhdr_v2->boot_data_ptr);
> -	printf("Entry Point:  %08x\n", (uint32_t)fhdr_v2->entry);
> -	if (fhdr_v2->csf && (imximage_ivt_offset != UNDEFINED) &&
> -	    (imximage_csf_size != UNDEFINED)) {
> -		printf("HAB Blocks:   %08x %08x %08x\n",
> -		       (uint32_t)fhdr_v2->self, 0,
> -		       hdr_v2->boot_data.size - imximage_ivt_offset -
> -		       imximage_csf_size);
> +	printf("Mode:         %s\n", plugin ? "PLUGIN" : "DCD");
> +	if (!plugin) {
> +		printf("Data Size:    ");
> +		genimg_print_size(hdr_v2->boot_data.size);
> +		printf("Load Address: %08x\n", (uint32_t)fhdr_v2->boot_data_ptr);
> +		printf("Entry Point:  %08x\n", (uint32_t)fhdr_v2->entry);
> +		if (fhdr_v2->csf && (imximage_ivt_offset != UNDEFINED) &&
> +		    (imximage_csf_size != UNDEFINED)) {
> +			printf("HAB Blocks:   %08x %08x %08x\n",
> +			       (uint32_t)fhdr_v2->self, 0,
> +			       hdr_v2->boot_data.size - imximage_ivt_offset -
> +			       imximage_csf_size);
> +		}
> +	} else {
> +		imx_header_v2_t *next_hdr_v2;
> +		flash_header_v2_t *next_fhdr_v2;
> +
> +		/*First Header*/
> +		printf("Plugin Data Size:     ");
> +		genimg_print_size(hdr_v2->boot_data.size);
> +		printf("Plugin Code Size:     ");
> +		genimg_print_size(imximage_plugin_size);
> +		printf("Plugin Load Address:  %08x\n", hdr_v2->boot_data.start);
> +		printf("Plugin Entry Point:   %08x\n", (uint32_t)fhdr_v2->entry);
> +
> +		/*Second Header*/
> +		next_hdr_v2 = (imx_header_v2_t *)((char *)hdr_v2 +
> +				imximage_plugin_size);
> +		next_fhdr_v2 = &next_hdr_v2->fhdr;
> +		printf("U-Boot Data Size:     ");
> +		genimg_print_size(next_hdr_v2->boot_data.size);
> +		printf("U-Boot Load Address:  %08x\n",
> +		       next_hdr_v2->boot_data.start);
> +		printf("U-Boot Entry Point:   %08x\n",
> +		       (uint32_t)next_fhdr_v2->entry);
>  	}
>  }
>  
> +static void copy_plugin_code(struct imx_header *imxhdr, char *plugin_file)
> +{
> +	int ifd = -1;
> +	struct stat sbuf;
> +	char *plugin_buf = imxhdr->header.hdr_v2.data.plugin_code;
> +	char *ptr;
> +
> +	ifd = open(plugin_file, O_RDONLY|O_BINARY);
> +	if (fstat(ifd, &sbuf) < 0) {
> +		fprintf(stderr, "Can't stat %s: %s\n",
> +			plugin_file,
> +			strerror(errno));
> +		exit(EXIT_FAILURE);
> +	}
> +
> +	ptr = mmap(0, sbuf.st_size, PROT_READ, MAP_SHARED, ifd, 0);
> +	if (ptr == MAP_FAILED) {
> +		fprintf(stderr, "Can't read %s: %s\n",
> +			plugin_file,
> +			strerror(errno));
> +		exit(EXIT_FAILURE);
> +	}
> +
> +	if (sbuf.st_size > MAX_PLUGIN_CODE_SIZE) {
> +		printf("plugin binary size too large\n");
> +		exit(EXIT_FAILURE);
> +	}
> +
> +	memcpy(plugin_buf, ptr, sbuf.st_size);
> +	imximage_plugin_size = sbuf.st_size;
> +
> +	(void) munmap((void *)ptr, sbuf.st_size);
> +	(void) close(ifd);
> +
> +	imxhdr->header.hdr_v2.boot_data.plugin = 1;
> +}
> +
>  static void parse_cfg_cmd(struct imx_header *imxhdr, int32_t cmd, char *token,
>  				char *name, int lineno, int fld, int dcd_len)
>  {
> @@ -497,6 +641,10 @@ static void parse_cfg_cmd(struct imx_header *imxhdr, int32_t cmd, char *token,
>  		if (unlikely(cmd_ver_first != 1))
>  			cmd_ver_first = 0;
>  		break;

This structure of parse_cfg_cmd to handle the first
argument and parse_cfg_fld to handle the second
argument is unfortunate.

> +	case CMD_PLUGIN:
> +		plugin_image = 1;
> +		copy_plugin_code(imxhdr, token);
> +		break;
>  	}
>  }
>  
> @@ -542,6 +690,10 @@ static void parse_cfg_fld(struct imx_header *imxhdr, int32_t *cmd,
>  				}
>  			}
>  			break;
> +		case CMD_PLUGIN:
> +			value = get_cfg_value(token, name, lineno);
> +			imximage_iram_free_start = value;
> +			break;
>  		default:
>  			break;
>  		}
> @@ -649,6 +801,7 @@ static void imximage_set_header(void *ptr, struct stat *sbuf, int ifd,
>  {
>  	struct imx_header *imxhdr = (struct imx_header *)ptr;
>  	uint32_t dcd_len;
> +	uint32_t header_size;
>  
>  	/*
>  	 * In order to not change the old imx cfg file
> @@ -665,10 +818,15 @@ static void imximage_set_header(void *ptr, struct stat *sbuf, int ifd,
>  	dcd_len = parse_cfg_file(imxhdr, params->imagename);
>  
>  	if (imximage_version == IMXIMAGE_V2) {
> -		if (imximage_init_loadsize < imximage_ivt_offset +
> -			sizeof(imx_header_v2_t))
> +		header_size = sizeof(flash_header_v2_t) + sizeof(boot_data_t);
> +		if (!plugin_image)
> +			header_size += sizeof(dcd_v2_t);
> +		else
> +			header_size += MAX_PLUGIN_CODE_SIZE;
> +
> +		if (imximage_init_loadsize < imximage_ivt_offset + header_size)
>  				imximage_init_loadsize = imximage_ivt_offset +
> -					sizeof(imx_header_v2_t);
> +					header_size;
>  	}
>  
>  	/* Set the imx header */
> @@ -721,7 +879,7 @@ static int imximage_generate(struct image_tool_params *params,
>  	size_t alloc_len;
>  	struct stat sbuf;
>  	char *datafile = params->datafile;
> -	uint32_t pad_len;
> +	uint32_t pad_len, header_size;
>  
>  	memset(&imximage_header, 0, sizeof(imximage_header));
>  
> @@ -742,15 +900,21 @@ static int imximage_generate(struct image_tool_params *params,
>  	/* TODO: check i.MX image V1 handling, for now use 'old' style */
>  	if (imximage_version == IMXIMAGE_V1) {
>  		alloc_len = 4096;
> +		header_size = 4096;
>  	} else {
> -		if (imximage_init_loadsize < imximage_ivt_offset +
> -			sizeof(imx_header_v2_t))
> +		header_size = sizeof(flash_header_v2_t) + sizeof(boot_data_t);
> +		if (!plugin_image)
> +			header_size += sizeof(dcd_v2_t);
> +		else
> +			header_size += MAX_PLUGIN_CODE_SIZE;
> +
> +		if (imximage_init_loadsize < imximage_ivt_offset + header_size)
>  				imximage_init_loadsize = imximage_ivt_offset +
> -					sizeof(imx_header_v2_t);
> +					header_size;
>  		alloc_len = imximage_init_loadsize - imximage_ivt_offset;
>  	}
>  
> -	if (alloc_len < sizeof(struct imx_header)) {
> +	if (alloc_len < header_size) {
>  		fprintf(stderr, "%s: header error\n",
>  			params->cmdname);
>  		exit(EXIT_FAILURE);
> diff --git a/tools/imximage.h b/tools/imximage.h
> index c7b9b5c..fe4dd89 100644
> --- a/tools/imximage.h
> +++ b/tools/imximage.h
> @@ -9,6 +9,7 @@
>  #define _IMXIMAGE_H_
>  
>  #define MAX_HW_CFG_SIZE_V2 220 /* Max number of registers imx can set for v2 */
> +#define MAX_PLUGIN_CODE_SIZE (16*1024)
>  #define MAX_HW_CFG_SIZE_V1 60  /* Max number of registers imx can set for v1 */
>  #define APP_CODE_BARKER	0xB1
>  #define DCD_BARKER	0xB17219E9
> @@ -64,6 +65,7 @@ enum imximage_cmd {
>  	CMD_CHECK_BITS_SET,
>  	CMD_CHECK_BITS_CLR,
>  	CMD_CSF,
> +	CMD_PLUGIN,
>  };
>  
>  enum imximage_fld_types {
> @@ -164,7 +166,10 @@ typedef struct {
>  typedef struct {
>  	flash_header_v2_t fhdr;
>  	boot_data_t boot_data;
> -	dcd_v2_t dcd_table;
> +	union {
> +		dcd_v2_t dcd_table;
> +		char plugin_code[MAX_PLUGIN_CODE_SIZE];
> +	} data;
>  } imx_header_v2_t;
>  
>  /* 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
(the plugin flag), and if we can test for serial download
mode as Stefano does in this patch, we can return
to the boot loader.

http://lists.denx.de/pipermail/u-boot/2015-December/237555.html

The subsequent patches to require plugin.S seem to get
in the way of that.

Regards,


Eric

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

[Peng Fan]

On Sat, Oct 08, 2016 at 05:26:18PM +0200, Eric Nelson wrote:
>Hi Peng,
>
>I'm sorry for taking so long to go though this.
>
>On 10/08/2016 08:58 AM, Peng Fan wrote:
>> Add plugin support for imximage.
>> 
>
>This CONFIG setting doesn't actually affect mkimage or imximage:

Yeah. The host tool always support plugin code now. But need to define
CONFIG_USE_IMXIMG_PLUGIN to compile plugin.S.

>
>> Define CONFIG_USE_IMXIMG_PLUGIN in defconfig to enable using plugin.
>> 
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> Cc: Stefano Babic <sbabic@denx.de>
>> Cc: Eric Nelson <eric@nelint.com>
>> Cc: Ye Li <ye.li@nxp.com>
>> Reviewed-by: Tom Rini <trini@konsulko.com>
>> ---
>> 
>> V3:
>>  Fix compile error.
>> 
>> V2:
>>  Drop the CONFIG_USE_PLUGIN, make plugin always support in imximage.
>> 
>>  tools/imximage.c | 282 +++++++++++++++++++++++++++++++++++++++++++------------
>>  tools/imximage.h |   7 +-
>>  2 files changed, 229 insertions(+), 60 deletions(-)
>> 
>> diff --git a/tools/imximage.c b/tools/imximage.c
>> index 092d550..7fa601e 100644
>> --- a/tools/imximage.c
>> +++ b/tools/imximage.c
>> @@ -27,6 +27,7 @@ static table_entry_t imximage_cmds[] = {
>>  	{CMD_CHECK_BITS_CLR,    "CHECK_BITS_CLR",   "Reg Check bits clr", },
>>  	{CMD_CSF,               "CSF",           "Command Sequence File", },
>>  	{CMD_IMAGE_VERSION,     "IMAGE_VERSION",        "image version",  },
>> +	{CMD_PLUGIN,            "PLUGIN",               "file plugin_addr",  },
>>  	{-1,                    "",                     "",	          },
>>  };
>>  
>> @@ -80,6 +81,9 @@ static uint32_t imximage_ivt_offset = UNDEFINED;
>>  static uint32_t imximage_csf_size = UNDEFINED;
>>  /* Initial Load Region Size */
>>  static uint32_t imximage_init_loadsize;
>
>These seem very limiting.
>
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.

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

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

>
>> +		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.
I'll rework this piece code to drop this limitation.

>
>> +		hdr_v2->boot_data.size = MAX_PLUGIN_CODE_SIZE;
>> +
>> +		/* Security feature are not supported */
>> +		fhdr_v2->csf = 0;
>> +
>> +		next_hdr_v2 = (imx_header_v2_t *)((char *)hdr_v2 +
>> +			       imximage_plugin_size);
>> +
>> +		next_fhdr_v2 = &next_hdr_v2->fhdr;
>> +
>> +		next_fhdr_v2->header.tag = IVT_HEADER_TAG; /* 0xD1 */
>> +		next_fhdr_v2->header.length =
>> +			cpu_to_be16(sizeof(flash_header_v2_t));
>> +		next_fhdr_v2->header.version = IVT_VERSION; /* 0x40 */
>> +
>> +		next_fhdr_v2->entry = entry_point;
>> +		hdr_base = entry_point - sizeof(struct imx_header);
>> +		next_fhdr_v2->reserved1 = 0;
>> +		next_fhdr_v2->reserved2 = 0;
>> +		next_fhdr_v2->self = hdr_base + imximage_plugin_size;
>> +
>> +		next_fhdr_v2->dcd_ptr = 0;
>> +		next_fhdr_v2->boot_data_ptr = next_fhdr_v2->self +
>> +				offsetof(imx_header_v2_t, boot_data);
>> +
>> +		next_hdr_v2->boot_data.start = hdr_base - flash_offset;
>> +
>> +		header_size_ptr = &next_hdr_v2->boot_data.size;
>>  
>> -	header_size_ptr = &hdr_v2->boot_data.size;
>> -	csf_ptr = &fhdr_v2->csf;
>> +		next_hdr_v2->boot_data.plugin = 0;
>> +
>> +		next_fhdr_v2->csf = 0;
>> +	}
>>  }
>>  
>>  static void set_hdr_func(void)
>> @@ -393,16 +472,19 @@ static void print_hdr_v2(struct imx_header *imx_hdr)
>>  {
>>  	imx_header_v2_t *hdr_v2 = &imx_hdr->header.hdr_v2;
>>  	flash_header_v2_t *fhdr_v2 = &hdr_v2->fhdr;
>> -	dcd_v2_t *dcd_v2 = &hdr_v2->dcd_table;
>> -	uint32_t size, version;
>> +	dcd_v2_t *dcd_v2 = &hdr_v2->data.dcd_table;
>> +	uint32_t size, version, plugin;
>>  
>> -	size = be16_to_cpu(dcd_v2->header.length);
>> -	if (size > (MAX_HW_CFG_SIZE_V2 * sizeof(dcd_addr_data_t)) + 8) {
>> -		fprintf(stderr,
>> -			"Error: Image corrupt DCD size %d exceed maximum %d\n",
>> -			(uint32_t)(size / sizeof(dcd_addr_data_t)),
>> -			MAX_HW_CFG_SIZE_V2);
>> -		exit(EXIT_FAILURE);
>> +	plugin = hdr_v2->boot_data.plugin;
>> +	if (!plugin) {
>> +		size = be16_to_cpu(dcd_v2->header.length);
>> +		if (size > (MAX_HW_CFG_SIZE_V2 * sizeof(dcd_addr_data_t))) {
>> +			fprintf(stderr,
>> +				"Error: Image corrupt DCD size %d exceed maximum %d\n",
>> +				(uint32_t)(size / sizeof(dcd_addr_data_t)),
>> +				MAX_HW_CFG_SIZE_V2);
>> +			exit(EXIT_FAILURE);
>> +		}
>>  	}
>>  
>>  	version = detect_imximage_version(imx_hdr);
>> @@ -410,19 +492,81 @@ static void print_hdr_v2(struct imx_header *imx_hdr)
>>  	printf("Image Type:   Freescale IMX Boot Image\n");
>>  	printf("Image Ver:    %x", version);
>>  	printf("%s\n", get_table_entry_name(imximage_versions, NULL, version));
>> -	printf("Data Size:    ");
>> -	genimg_print_size(hdr_v2->boot_data.size);
>> -	printf("Load Address: %08x\n", (uint32_t)fhdr_v2->boot_data_ptr);
>> -	printf("Entry Point:  %08x\n", (uint32_t)fhdr_v2->entry);
>> -	if (fhdr_v2->csf && (imximage_ivt_offset != UNDEFINED) &&
>> -	    (imximage_csf_size != UNDEFINED)) {
>> -		printf("HAB Blocks:   %08x %08x %08x\n",
>> -		       (uint32_t)fhdr_v2->self, 0,
>> -		       hdr_v2->boot_data.size - imximage_ivt_offset -
>> -		       imximage_csf_size);
>> +	printf("Mode:         %s\n", plugin ? "PLUGIN" : "DCD");
>> +	if (!plugin) {
>> +		printf("Data Size:    ");
>> +		genimg_print_size(hdr_v2->boot_data.size);
>> +		printf("Load Address: %08x\n", (uint32_t)fhdr_v2->boot_data_ptr);
>> +		printf("Entry Point:  %08x\n", (uint32_t)fhdr_v2->entry);
>> +		if (fhdr_v2->csf && (imximage_ivt_offset != UNDEFINED) &&
>> +		    (imximage_csf_size != UNDEFINED)) {
>> +			printf("HAB Blocks:   %08x %08x %08x\n",
>> +			       (uint32_t)fhdr_v2->self, 0,
>> +			       hdr_v2->boot_data.size - imximage_ivt_offset -
>> +			       imximage_csf_size);
>> +		}
>> +	} else {
>> +		imx_header_v2_t *next_hdr_v2;
>> +		flash_header_v2_t *next_fhdr_v2;
>> +
>> +		/*First Header*/
>> +		printf("Plugin Data Size:     ");
>> +		genimg_print_size(hdr_v2->boot_data.size);
>> +		printf("Plugin Code Size:     ");
>> +		genimg_print_size(imximage_plugin_size);
>> +		printf("Plugin Load Address:  %08x\n", hdr_v2->boot_data.start);
>> +		printf("Plugin Entry Point:   %08x\n", (uint32_t)fhdr_v2->entry);
>> +
>> +		/*Second Header*/
>> +		next_hdr_v2 = (imx_header_v2_t *)((char *)hdr_v2 +
>> +				imximage_plugin_size);
>> +		next_fhdr_v2 = &next_hdr_v2->fhdr;
>> +		printf("U-Boot Data Size:     ");
>> +		genimg_print_size(next_hdr_v2->boot_data.size);
>> +		printf("U-Boot Load Address:  %08x\n",
>> +		       next_hdr_v2->boot_data.start);
>> +		printf("U-Boot Entry Point:   %08x\n",
>> +		       (uint32_t)next_fhdr_v2->entry);
>>  	}
>>  }
>>  
>> +static void copy_plugin_code(struct imx_header *imxhdr, char *plugin_file)
>> +{
>> +	int ifd = -1;
>> +	struct stat sbuf;
>> +	char *plugin_buf = imxhdr->header.hdr_v2.data.plugin_code;
>> +	char *ptr;
>> +
>> +	ifd = open(plugin_file, O_RDONLY|O_BINARY);
>> +	if (fstat(ifd, &sbuf) < 0) {
>> +		fprintf(stderr, "Can't stat %s: %s\n",
>> +			plugin_file,
>> +			strerror(errno));
>> +		exit(EXIT_FAILURE);
>> +	}
>> +
>> +	ptr = mmap(0, sbuf.st_size, PROT_READ, MAP_SHARED, ifd, 0);
>> +	if (ptr == MAP_FAILED) {
>> +		fprintf(stderr, "Can't read %s: %s\n",
>> +			plugin_file,
>> +			strerror(errno));
>> +		exit(EXIT_FAILURE);
>> +	}
>> +
>> +	if (sbuf.st_size > MAX_PLUGIN_CODE_SIZE) {
>> +		printf("plugin binary size too large\n");
>> +		exit(EXIT_FAILURE);
>> +	}
>> +
>> +	memcpy(plugin_buf, ptr, sbuf.st_size);
>> +	imximage_plugin_size = sbuf.st_size;
>> +
>> +	(void) munmap((void *)ptr, sbuf.st_size);
>> +	(void) close(ifd);
>> +
>> +	imxhdr->header.hdr_v2.boot_data.plugin = 1;
>> +}
>> +
>>  static void parse_cfg_cmd(struct imx_header *imxhdr, int32_t cmd, char *token,
>>  				char *name, int lineno, int fld, int dcd_len)
>>  {
>> @@ -497,6 +641,10 @@ static void parse_cfg_cmd(struct imx_header *imxhdr, int32_t cmd, char *token,
>>  		if (unlikely(cmd_ver_first != 1))
>>  			cmd_ver_first = 0;
>>  		break;
>
>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.

>
>> +	case CMD_PLUGIN:
>> +		plugin_image = 1;
>> +		copy_plugin_code(imxhdr, token);
>> +		break;
>>  	}
>>  }
>>  
>> @@ -542,6 +690,10 @@ static void parse_cfg_fld(struct imx_header *imxhdr, int32_t *cmd,
>>  				}
>>  			}
>>  			break;
>> +		case CMD_PLUGIN:
>> +			value = get_cfg_value(token, name, lineno);
>> +			imximage_iram_free_start = value;
>> +			break;
>>  		default:
>>  			break;
>>  		}
>> @@ -649,6 +801,7 @@ static void imximage_set_header(void *ptr, struct stat *sbuf, int ifd,
>>  {
>>  	struct imx_header *imxhdr = (struct imx_header *)ptr;
>>  	uint32_t dcd_len;
>> +	uint32_t header_size;
>>  
>>  	/*
>>  	 * In order to not change the old imx cfg file
>> @@ -665,10 +818,15 @@ static void imximage_set_header(void *ptr, struct stat *sbuf, int ifd,
>>  	dcd_len = parse_cfg_file(imxhdr, params->imagename);
>>  
>>  	if (imximage_version == IMXIMAGE_V2) {
>> -		if (imximage_init_loadsize < imximage_ivt_offset +
>> -			sizeof(imx_header_v2_t))
>> +		header_size = sizeof(flash_header_v2_t) + sizeof(boot_data_t);
>> +		if (!plugin_image)
>> +			header_size += sizeof(dcd_v2_t);
>> +		else
>> +			header_size += MAX_PLUGIN_CODE_SIZE;
>> +
>> +		if (imximage_init_loadsize < imximage_ivt_offset + header_size)
>>  				imximage_init_loadsize = imximage_ivt_offset +
>> -					sizeof(imx_header_v2_t);
>> +					header_size;
>>  	}
>>  
>>  	/* Set the imx header */
>> @@ -721,7 +879,7 @@ static int imximage_generate(struct image_tool_params *params,
>>  	size_t alloc_len;
>>  	struct stat sbuf;
>>  	char *datafile = params->datafile;
>> -	uint32_t pad_len;
>> +	uint32_t pad_len, header_size;
>>  
>>  	memset(&imximage_header, 0, sizeof(imximage_header));
>>  
>> @@ -742,15 +900,21 @@ static int imximage_generate(struct image_tool_params *params,
>>  	/* TODO: check i.MX image V1 handling, for now use 'old' style */
>>  	if (imximage_version == IMXIMAGE_V1) {
>>  		alloc_len = 4096;
>> +		header_size = 4096;
>>  	} else {
>> -		if (imximage_init_loadsize < imximage_ivt_offset +
>> -			sizeof(imx_header_v2_t))
>> +		header_size = sizeof(flash_header_v2_t) + sizeof(boot_data_t);
>> +		if (!plugin_image)
>> +			header_size += sizeof(dcd_v2_t);
>> +		else
>> +			header_size += MAX_PLUGIN_CODE_SIZE;
>> +
>> +		if (imximage_init_loadsize < imximage_ivt_offset + header_size)
>>  				imximage_init_loadsize = imximage_ivt_offset +
>> -					sizeof(imx_header_v2_t);
>> +					header_size;
>>  		alloc_len = imximage_init_loadsize - imximage_ivt_offset;
>>  	}
>>  
>> -	if (alloc_len < sizeof(struct imx_header)) {
>> +	if (alloc_len < header_size) {
>>  		fprintf(stderr, "%s: header error\n",
>>  			params->cmdname);
>>  		exit(EXIT_FAILURE);
>> diff --git a/tools/imximage.h b/tools/imximage.h
>> index c7b9b5c..fe4dd89 100644
>> --- a/tools/imximage.h
>> +++ b/tools/imximage.h
>> @@ -9,6 +9,7 @@
>>  #define _IMXIMAGE_H_
>>  
>>  #define MAX_HW_CFG_SIZE_V2 220 /* Max number of registers imx can set for v2 */
>> +#define MAX_PLUGIN_CODE_SIZE (16*1024)
>>  #define MAX_HW_CFG_SIZE_V1 60  /* Max number of registers imx can set for v1 */
>>  #define APP_CODE_BARKER	0xB1
>>  #define DCD_BARKER	0xB17219E9
>> @@ -64,6 +65,7 @@ enum imximage_cmd {
>>  	CMD_CHECK_BITS_SET,
>>  	CMD_CHECK_BITS_CLR,
>>  	CMD_CSF,
>> +	CMD_PLUGIN,
>>  };
>>  
>>  enum imximage_fld_types {
>> @@ -164,7 +166,10 @@ typedef struct {
>>  typedef struct {
>>  	flash_header_v2_t fhdr;
>>  	boot_data_t boot_data;
>> -	dcd_v2_t dcd_table;
>> +	union {
>> +		dcd_v2_t dcd_table;
>> +		char plugin_code[MAX_PLUGIN_CODE_SIZE];
>> +	} data;
>>  } imx_header_v2_t;
>>  
>>  /* 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.

Regards,
Peng.
>(the plugin flag), and if we can test for serial download
>mode as Stefano does in this patch, we can return
>to the boot loader.
>
>http://lists.denx.de/pipermail/u-boot/2015-December/237555.html
>
>The subsequent patches to require plugin.S seem to get
>in the way of that.
>
>Regards,
>
>
>Eric

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

[Eric Nelson]

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

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.

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

> 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

[U-Boot] [PATCH V3 4/8] imx-common: inctroude USE_IMXIMG_PLUGIN Kconfig

[Eric Nelson]

Hi Peng,

Note the typo in the subject header.

On 10/08/2016 08:58 AM, Peng Fan wrote:
> Introduce USE_IMXIMG_PLUGIN Kconfig
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> Cc: Stefano Babic <sbabic@denx.de>
> Reviewed-by: Tom Rini <trini@konsulko.com>
> ---
> 
> V3:
>  None
> 
> V2:
>  New
> 
>  arch/arm/imx-common/Kconfig | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm/imx-common/Kconfig b/arch/arm/imx-common/Kconfig
> index 1b7da5a..85eac57 100644
> --- a/arch/arm/imx-common/Kconfig
> +++ b/arch/arm/imx-common/Kconfig
> @@ -17,3 +17,10 @@ config IMX_BOOTAUX
>  	depends on ARCH_MX7 || ARCH_MX6
>  	help
>  	  bootaux [addr] to boot auxiliary core.
> +
> +config USE_IMXIMG_PLUGIN
> +	bool "Using imximage plugin code"

s/Using/Use/

> +	depends on ARCH_MX7 || ARCH_MX6
> +	help
> +	  i.MX6/7 supports DCD and Plugin. Enable this configuration
> +	  to use Plugin, otherwise DCD will be used.
> 

MX5x also supports plugins, right?

Also, the alternative isn't really DCD is it? Most (all) of the boards
using SPL now are using the essentially empty config file
in arch/arm/imx-common/spl_sd.cfg.

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

[Peng Fan]

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

[U-Boot] [PATCH V3 4/8] imx-common: inctroude USE_IMXIMG_PLUGIN Kconfig

[Peng Fan]

On Sun, Oct 09, 2016 at 07:45:41AM +0200, Eric Nelson wrote:
>Hi Peng,
>
>Note the typo in the subject header.

Thanks.

>
>On 10/08/2016 08:58 AM, Peng Fan wrote:
>> Introduce USE_IMXIMG_PLUGIN Kconfig
>> 
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> Cc: Stefano Babic <sbabic@denx.de>
>> Reviewed-by: Tom Rini <trini@konsulko.com>
>> ---
>> 
>> V3:
>>  None
>> 
>> V2:
>>  New
>> 
>>  arch/arm/imx-common/Kconfig | 7 +++++++
>>  1 file changed, 7 insertions(+)
>> 
>> diff --git a/arch/arm/imx-common/Kconfig b/arch/arm/imx-common/Kconfig
>> index 1b7da5a..85eac57 100644
>> --- a/arch/arm/imx-common/Kconfig
>> +++ b/arch/arm/imx-common/Kconfig
>> @@ -17,3 +17,10 @@ config IMX_BOOTAUX
>>  	depends on ARCH_MX7 || ARCH_MX6
>>  	help
>>  	  bootaux [addr] to boot auxiliary core.
>> +
>> +config USE_IMXIMG_PLUGIN
>> +	bool "Using imximage plugin code"
>
>s/Using/Use/

Will fix this in V4.

>
>> +	depends on ARCH_MX7 || ARCH_MX6
>> +	help
>> +	  i.MX6/7 supports DCD and Plugin. Enable this configuration
>> +	  to use Plugin, otherwise DCD will be used.
>> 
>
>MX5x also supports plugins, right?

Yeah. I do not have the hardware to test, so not add ARCH_MX5.
If later, someone can test ARCH_MX5, then he/she could add the support.

>
>Also, the alternative isn't really DCD is it? Most (all) of the boards
>using SPL now are using the essentially empty config file
>in arch/arm/imx-common/spl_sd.cfg.

The help msg is just to tell use plugin or DCD. spl_sd.cfg surely
can have DCD DATA lines or not :)

Regards,
Peng.
>
>
>_______________________________________________
>U-Boot mailing list
>U-Boot at lists.denx.de
>http://lists.denx.de/mailman/listinfo/u-boot

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

[Eric Nelson]

Hi Peng,

On 10/09/2016 08:12 AM, Peng Fan wrote:
>> 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>

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

Cool. Otherwise

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

This page is telling me that "Access is restricted".

>>

<snip>

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

Cool.

I'll take a stab at instrumenting the startup code and let you know
what I find.

Regards,


Eric

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

[Eric Nelson]

Hi Peng,

On 10/09/2016 06:48 PM, Eric Nelson wrote:
> On 10/09/2016 08:12 AM, Peng Fan wrote:
>>> 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>

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

Sorry. I fat-fingered the <enter> key while I went in search for a
link.

What I meant to say was

"Otherwise, we couldn't use HAB and LPSR mode on i.MX7", which
is another reason for getting plugins to work."

https://community.nxp.com/thread/434057?sr=inbox&ru=206296