All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support
@ 2017-03-29 19:58 Stefan Agner
  2017-03-29 19:58 ` [U-Boot] [PATCH 1/3] imx: imx-common: move aux core image parsing to common code Stefan Agner
                   ` (3 more replies)
  0 siblings, 4 replies; 25+ messages in thread
From: Stefan Agner @ 2017-03-29 19:58 UTC (permalink / raw)
  To: u-boot

From: Stefan Agner <stefan.agner@toradex.com>

This patchset enables to boot elf binaries on secondary Cortex-M
class cores available on i.MX 6SoloX/7Solo/7Dual. This makes
handling and loading firmwares much more convinient since all
information where the firmware has to be loaded to is contained in
the elf headers. A typical usage looks like this:

  Colibri iMX7 # tftp ${loadaddr} firmware.elf && bootaux ${loadaddr}
  Using FEC0 device
  TFTP from server 192.168.10.1; our IP address is 192.168.10.2
  Filename 'firmware.elf'.
  Load address: 0x80800000
  Loading: ##################################################  88.3 KiB
           5.4 MiB/s
  done
  Bytes transferred = 90424 (16138 hex)
  ## Starting auxiliary core at 0x1FFF8311 ...
  Colibri iMX7 #

Note that the bootaux command retrieved the entry point (PC) from
the elf binary. Also all sections are translated to A7 addresses
in order to properly load the firmware sections into the appropriate
locations. Also cache flushes is taken care of, so that there is no
manual dcache flush necessary anymore.

The NXP FreeRTOS BSP already generates elf binaries which can be
directly used with this elf binary support.

The last patch adds bootaux support for Vybrid.


Stefan Agner (3):
  imx: imx-common: move aux core image parsing to common code
  imx: imx-common: add elf firmware support
  ARM: vf610: add auxiliary core boot support

 arch/arm/cpu/armv7/mx6/soc.c                |  30 +++++---
 arch/arm/cpu/armv7/mx7/soc.c                |  34 ++++++---
 arch/arm/cpu/armv7/vf610/generic.c          |  42 +++++++++++
 arch/arm/imx-common/Kconfig                 |   2 +-
 arch/arm/imx-common/Makefile                |   4 +-
 arch/arm/imx-common/imx_bootaux.c           | 105 +++++++++++++++++++++++-----
 arch/arm/include/asm/arch-vf610/sys_proto.h |   8 +++
 arch/arm/include/asm/imx-common/sys_proto.h |   6 ++
 configs/colibri_vf_defconfig                |   1 +
 9 files changed, 198 insertions(+), 34 deletions(-)
 create mode 100644 arch/arm/include/asm/arch-vf610/sys_proto.h

-- 
2.12.1

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH 1/3] imx: imx-common: move aux core image parsing to common code
  2017-03-29 19:58 [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support Stefan Agner
@ 2017-03-29 19:58 ` Stefan Agner
  2017-03-29 19:58 ` [U-Boot] [PATCH 2/3] imx: imx-common: add elf firmware support Stefan Agner
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 25+ messages in thread
From: Stefan Agner @ 2017-03-29 19:58 UTC (permalink / raw)
  To: u-boot

From: Stefan Agner <stefan.agner@toradex.com>

For i.MX 6SoloX/i.MX 7 simple binary files are used to boot the
auxiliary CPU core (Cortex-M4). This patch moves the "parsing" of
this binary firmwares to the SoC independent code. This allows to
add different binary formats more easily.

While at it, also move the comment about the inner workings how
to boot the Cortex-M4 core to a more appropriate location.

Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
---

 arch/arm/cpu/armv7/mx6/soc.c      | 18 ++++++++++--------
 arch/arm/cpu/armv7/mx7/soc.c      | 19 +++++++++++--------
 arch/arm/imx-common/imx_bootaux.c | 21 +++++++++------------
 3 files changed, 30 insertions(+), 28 deletions(-)

diff --git a/arch/arm/cpu/armv7/mx6/soc.c b/arch/arm/cpu/armv7/mx6/soc.c
index dd94797514..642195b97c 100644
--- a/arch/arm/cpu/armv7/mx6/soc.c
+++ b/arch/arm/cpu/armv7/mx6/soc.c
@@ -666,16 +666,18 @@ void imx_setup_hdmi(void)
 #endif
 
 #ifdef CONFIG_IMX_BOOTAUX
-int arch_auxiliary_core_up(u32 core_id, u32 boot_private_data)
+/*
+ * Per the cortex-M reference manual, the reset vector of M4 needs
+ * to exist at 0x0 (TCMUL). The PC and SP are the first two addresses
+ * of that vector.  So to boot M4, the A core must build the M4's reset
+ * vector with getting the PC and SP from image and filling them to
+ * TCMUL. When M4 is kicked, it will load the PC and SP by itself.
+ * The TCMUL is mapped to (M4_BOOTROM_BASE_ADDR) at A core side for
+ * accessing the M4 TCMUL.
+ */
+int arch_auxiliary_core_up(u32 core_id, u32 stack, u32 pc)
 {
 	struct src *src_reg;
-	u32 stack, pc;
-
-	if (!boot_private_data)
-		return -EINVAL;
-
-	stack = *(u32 *)boot_private_data;
-	pc = *(u32 *)(boot_private_data + 4);
 
 	/* Set the stack and pc to M4 bootROM */
 	writel(stack, M4_BOOTROM_BASE_ADDR);
diff --git a/arch/arm/cpu/armv7/mx7/soc.c b/arch/arm/cpu/armv7/mx7/soc.c
index 8422f24573..e949b8e557 100644
--- a/arch/arm/cpu/armv7/mx7/soc.c
+++ b/arch/arm/cpu/armv7/mx7/soc.c
@@ -311,17 +311,20 @@ void imx_get_mac_from_fuse(int dev_id, unsigned char *mac)
 #endif
 
 #ifdef CONFIG_IMX_BOOTAUX
-int arch_auxiliary_core_up(u32 core_id, u32 boot_private_data)
+
+/*
+ * Per the cortex-M reference manual, the reset vector of M4 needs
+ * to exist at 0x0 (TCMUL). The PC and SP are the first two addresses
+ * of that vector.  So to boot M4, the A core must build the M4's reset
+ * vector with getting the PC and SP from image and filling them to
+ * TCMUL. When M4 is kicked, it will load the PC and SP by itself.
+ * The TCMUL is mapped to (M4_BOOTROM_BASE_ADDR) at A core side for
+ * accessing the M4 TCMUL.
+ */
+int arch_auxiliary_core_up(u32 core_id, u32 stack, u32 pc)
 {
-	u32 stack, pc;
 	struct src *src_reg = (struct src *)SRC_BASE_ADDR;
 
-	if (!boot_private_data)
-		return 1;
-
-	stack = *(u32 *)boot_private_data;
-	pc = *(u32 *)(boot_private_data + 4);
-
 	/* Set the stack and pc to M4 bootROM */
 	writel(stack, M4_BOOTROM_BASE_ADDR);
 	writel(pc, M4_BOOTROM_BASE_ADDR + 4);
diff --git a/arch/arm/imx-common/imx_bootaux.c b/arch/arm/imx-common/imx_bootaux.c
index 69026df763..4d697b0660 100644
--- a/arch/arm/imx-common/imx_bootaux.c
+++ b/arch/arm/imx-common/imx_bootaux.c
@@ -8,13 +8,13 @@
 #include <command.h>
 
 /* Allow for arch specific config before we boot */
-static int __arch_auxiliary_core_up(u32 core_id, u32 boot_private_data)
+static int __arch_auxiliary_core_up(u32 core_id, u32 stack, u32 pc)
 {
 	/* please define platform specific arch_auxiliary_core_up() */
 	return CMD_RET_FAILURE;
 }
 
-int arch_auxiliary_core_up(u32 core_id, u32 boot_private_data)
+int arch_auxiliary_core_up(u32 core_id, u32 stack, u32 pc)
 	__attribute__((weak, alias("__arch_auxiliary_core_up")));
 
 /* Allow for arch specific config before we boot */
@@ -31,17 +31,10 @@ int arch_auxiliary_core_check_up(u32 core_id)
  * To i.MX6SX and i.MX7D, the image supported by bootaux needs
  * the reset vector at the head for the image, with SP and PC
  * as the first two words.
- *
- * Per the cortex-M reference manual, the reset vector of M4 needs
- * to exist at 0x0 (TCMUL). The PC and SP are the first two addresses
- * of that vector.  So to boot M4, the A core must build the M4's reset
- * vector with getting the PC and SP from image and filling them to
- * TCMUL. When M4 is kicked, it will load the PC and SP by itself.
- * The TCMUL is mapped to (M4_BOOTROM_BASE_ADDR) at A core side for
- * accessing the M4 TCMUL.
  */
 int do_bootaux(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
+	u32 stack, pc;
 	ulong addr;
 	int ret, up;
 
@@ -56,9 +49,13 @@ int do_bootaux(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 
 	addr = simple_strtoul(argv[1], NULL, 16);
 
-	printf("## Starting auxiliary core at 0x%08lX ...\n", addr);
+	/* Assume binary file with vector table at the beginning */
+	stack = *(u32 *)addr;
+	pc = *(u32 *)(addr + 4);
+
+	printf("## Starting auxiliary core at 0x%08X ...\n", pc);
 
-	ret = arch_auxiliary_core_up(0, addr);
+	ret = arch_auxiliary_core_up(0, stack, pc);
 	if (ret)
 		return CMD_RET_FAILURE;
 
-- 
2.12.1

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH 2/3] imx: imx-common: add elf firmware support
  2017-03-29 19:58 [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support Stefan Agner
  2017-03-29 19:58 ` [U-Boot] [PATCH 1/3] imx: imx-common: move aux core image parsing to common code Stefan Agner
@ 2017-03-29 19:58 ` Stefan Agner
  2017-03-29 19:58 ` [U-Boot] [PATCH 3/3] ARM: vf610: add auxiliary core boot support Stefan Agner
  2017-04-03 11:20 ` [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support Lukasz Majewski
  3 siblings, 0 replies; 25+ messages in thread
From: Stefan Agner @ 2017-03-29 19:58 UTC (permalink / raw)
  To: u-boot

From: Stefan Agner <stefan.agner@toradex.com>

Support elf firmware files for the auxiliary Cortex-M4 core. This
has the advantage that the user does not need to know to which
address the binary has been linked to. However, in order to load
the elf sections to the right address, we need to translate the
Cortex-M4 core memory addresses to primary/host CPU memory
addresses (U-Boot is typically running on the A7/A9 core). This
allows to boot firmwares from any location with just using
bootaux, e.g.:

  tftp ${loadaddr} low_power_demo.elf && bootaux ${loadaddr}

Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
---

 arch/arm/cpu/armv7/mx6/soc.c                | 12 ++++
 arch/arm/cpu/armv7/mx7/soc.c                | 15 +++++
 arch/arm/imx-common/imx_bootaux.c           | 90 ++++++++++++++++++++++++++---
 arch/arm/include/asm/imx-common/sys_proto.h |  6 ++
 4 files changed, 116 insertions(+), 7 deletions(-)

diff --git a/arch/arm/cpu/armv7/mx6/soc.c b/arch/arm/cpu/armv7/mx6/soc.c
index 642195b97c..9b89c87e04 100644
--- a/arch/arm/cpu/armv7/mx6/soc.c
+++ b/arch/arm/cpu/armv7/mx6/soc.c
@@ -666,6 +666,18 @@ void imx_setup_hdmi(void)
 #endif
 
 #ifdef CONFIG_IMX_BOOTAUX
+#ifdef CONFIG_MX6SX
+const struct memorymap hostmap[] = {
+	{ .auxcore = 0x00000000, .host = 0x007f8000, .size = 0x8000 },
+	{ .auxcore = 0x1fff8000, .host = 0x007f8000, .size = 0x8000 },
+	{ .auxcore = 0x20000000, .host = 0x00800000, .size = 0x8000 },
+	{ .auxcore = 0x00900000, .host = 0x00900000, .size = 0x20000 },
+	{ .auxcore = 0x20900000, .host = 0x00900000, .size = 0x20000 },
+	{ .auxcore = 0x10000000, .host = 0x80000000, .size = 0x0fff0000 },
+	{ .auxcore = 0x80000000, .host = 0x80000000, .size = 0xe0000000 },
+	{ /* sentinel */ }
+#endif
+
 /*
  * Per the cortex-M reference manual, the reset vector of M4 needs
  * to exist at 0x0 (TCMUL). The PC and SP are the first two addresses
diff --git a/arch/arm/cpu/armv7/mx7/soc.c b/arch/arm/cpu/armv7/mx7/soc.c
index e949b8e557..6491778f5b 100644
--- a/arch/arm/cpu/armv7/mx7/soc.c
+++ b/arch/arm/cpu/armv7/mx7/soc.c
@@ -311,6 +311,21 @@ void imx_get_mac_from_fuse(int dev_id, unsigned char *mac)
 #endif
 
 #ifdef CONFIG_IMX_BOOTAUX
+const struct memorymap hostmap[] = {
+	{ .auxcore = 0x00000000, .host = 0x00180000, .size = 0x8000 },
+	{ .auxcore = 0x00180000, .host = 0x00180000, .size = 0x8000 },
+	{ .auxcore = 0x1fff8000, .host = 0x007f8000, .size = 0x8000 },
+	{ .auxcore = 0x20000000, .host = 0x00800000, .size = 0x8000 },
+	{ .auxcore = 0x00900000, .host = 0x00900000, .size = 0x20000 },
+	{ .auxcore = 0x20200000, .host = 0x00900000, .size = 0x20000 },
+	{ .auxcore = 0x00920000, .host = 0x00920000, .size = 0x20000 },
+	{ .auxcore = 0x20220000, .host = 0x00920000, .size = 0x20000 },
+	{ .auxcore = 0x00940000, .host = 0x00940000, .size = 0x20000 },
+	{ .auxcore = 0x20240000, .host = 0x00940000, .size = 0x20000 },
+	{ .auxcore = 0x10000000, .host = 0x80000000, .size = 0x0fff0000 },
+	{ .auxcore = 0x80000000, .host = 0x80000000, .size = 0xe0000000 },
+	{ /* sentinel */ }
+};
 
 /*
  * Per the cortex-M reference manual, the reset vector of M4 needs
diff --git a/arch/arm/imx-common/imx_bootaux.c b/arch/arm/imx-common/imx_bootaux.c
index 4d697b0660..365c32f3dd 100644
--- a/arch/arm/imx-common/imx_bootaux.c
+++ b/arch/arm/imx-common/imx_bootaux.c
@@ -4,8 +4,10 @@
  * SPDX-License-Identifier:	GPL-2.0+
  */
 
+#include <asm/arch/sys_proto.h>
 #include <common.h>
 #include <command.h>
+#include <elf.h>
 
 /* Allow for arch specific config before we boot */
 static int __arch_auxiliary_core_up(u32 core_id, u32 stack, u32 pc)
@@ -27,11 +29,68 @@ static int __arch_auxiliary_core_check_up(u32 core_id)
 int arch_auxiliary_core_check_up(u32 core_id)
 	__attribute__((weak, alias("__arch_auxiliary_core_check_up")));
 
+const __weak struct memorymap hostmap[] = { };
+
 /*
- * To i.MX6SX and i.MX7D, the image supported by bootaux needs
- * the reset vector at the head for the image, with SP and PC
- * as the first two words.
+ * Get memory map by auxiliary core memory address
  */
+static const struct memorymap *get_host_mapping(unsigned long auxcore)
+{
+	const struct memorymap *mmap = hostmap;
+
+	while (mmap) {
+		if (mmap->auxcore <= auxcore &&
+		    mmap->auxcore + mmap->size > auxcore)
+			return mmap;
+		mmap++;
+	}
+
+	return NULL;
+}
+
+/*
+ * A very simple elf loader, assumes the image is valid, returns the
+ * entry point address.
+ */
+static unsigned long load_elf_image_phdr(unsigned long addr)
+{
+	Elf32_Ehdr *ehdr; /* Elf header structure pointer */
+	Elf32_Phdr *phdr; /* Program header structure pointer */
+	int i;
+
+	ehdr = (Elf32_Ehdr *)addr;
+	phdr = (Elf32_Phdr *)(addr + ehdr->e_phoff);
+
+	/* Load each program header */
+	for (i = 0; i < ehdr->e_phnum; ++i, ++phdr) {
+		const struct memorymap *mmap = get_host_mapping(phdr->p_paddr);
+		void *dst, *src;
+
+		if (phdr->p_type != PT_LOAD)
+			continue;
+
+		if (!mmap) {
+			error("Invalid aux core address: %08x", phdr->p_paddr);
+			return 0;
+		}
+
+		dst = (void *)(phdr->p_paddr - mmap->auxcore) + mmap->host;
+		src = (void *)addr + phdr->p_offset;
+		debug("Loading phdr %i to 0x%p (%i bytes)\n",
+		      i, dst, phdr->p_filesz);
+		if (phdr->p_filesz)
+			memcpy(dst, src, phdr->p_filesz);
+		if (phdr->p_filesz != phdr->p_memsz)
+			memset(dst + phdr->p_filesz, 0x00,
+			       phdr->p_memsz - phdr->p_filesz);
+		flush_cache((unsigned long)dst & ~(CONFIG_SYS_CACHELINE_SIZE-1),
+			    ALIGN(phdr->p_filesz, CONFIG_SYS_CACHELINE_SIZE));
+	}
+
+	return ehdr->e_entry;
+}
+
+/* Supports firmware files in binary and elf format (using autodetection) */
 int do_bootaux(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	u32 stack, pc;
@@ -48,10 +107,24 @@ int do_bootaux(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	}
 
 	addr = simple_strtoul(argv[1], NULL, 16);
+	if (!addr)
+		return CMD_RET_FAILURE;
 
-	/* Assume binary file with vector table at the beginning */
-	stack = *(u32 *)addr;
-	pc = *(u32 *)(addr + 4);
+	if (valid_elf_image(addr)) {
+		stack = 0x0;
+		pc = load_elf_image_phdr(addr);
+		if (!pc)
+			return CMD_RET_FAILURE;
+
+	} else {
+		/*
+		 * Assume binary file with vector table at the beginning.
+		 * Cortex-M4 vector tables start with the stack pointer (SP)
+		 * and reset vector (initial PC).
+		 */
+		stack = *(u32 *)addr;
+		pc = *(u32 *)(addr + 4);
+	}
 
 	printf("## Starting auxiliary core at 0x%08X ...\n", pc);
 
@@ -65,5 +138,8 @@ int do_bootaux(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 U_BOOT_CMD(
 	bootaux, CONFIG_SYS_MAXARGS, 1,	do_bootaux,
 	"Start auxiliary core",
-	""
+	"<addr>\n"
+	"Boot firmware at 'addr' on auxiliary core. Firmware formats:\n"
+	"  - bin: 'addr' must be the address the fw has been linked to\n"
+	"  - elf: 'addr' can be anywhere, relocating according to elf headers\n"
 );
diff --git a/arch/arm/include/asm/imx-common/sys_proto.h b/arch/arm/include/asm/imx-common/sys_proto.h
index a07061bc9b..dfd736ae50 100644
--- a/arch/arm/include/asm/imx-common/sys_proto.h
+++ b/arch/arm/include/asm/imx-common/sys_proto.h
@@ -87,6 +87,12 @@ static inline u8 imx6_is_bmode_from_gpr9(void)
 u32 imx6_src_get_boot_mode(void);
 #endif /* CONFIG_MX6 */
 
+struct memorymap {
+	unsigned long auxcore;
+	unsigned long host;
+	unsigned long size;
+};
+
 u32 get_nr_cpus(void);
 u32 get_cpu_rev(void);
 u32 get_cpu_speed_grade_hz(void);
-- 
2.12.1

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH 3/3] ARM: vf610: add auxiliary core boot support
  2017-03-29 19:58 [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support Stefan Agner
  2017-03-29 19:58 ` [U-Boot] [PATCH 1/3] imx: imx-common: move aux core image parsing to common code Stefan Agner
  2017-03-29 19:58 ` [U-Boot] [PATCH 2/3] imx: imx-common: add elf firmware support Stefan Agner
@ 2017-03-29 19:58 ` Stefan Agner
  2017-04-03 11:20 ` [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support Lukasz Majewski
  3 siblings, 0 replies; 25+ messages in thread
From: Stefan Agner @ 2017-03-29 19:58 UTC (permalink / raw)
  To: u-boot

From: Stefan Agner <stefan.agner@toradex.com>

Use i.MX bootaux support introduced for i.MX 6SoloX/i.MX 7 for
Vybrid too. Starting the Cortex-M4 core on Vybrid works a bit
differently, naemly it uses a GPR register to define the initial
PC. There is no way to define the initial stack (the stack is
set up in a boot ROM). This is not a problem for most firmwares
since the firmwares startup code reinitialize the stack as part
of the firmware startup code anyway.

Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
---

 arch/arm/cpu/armv7/vf610/generic.c          | 42 +++++++++++++++++++++++++++++
 arch/arm/imx-common/Kconfig                 |  2 +-
 arch/arm/imx-common/Makefile                |  4 ++-
 arch/arm/include/asm/arch-vf610/sys_proto.h |  8 ++++++
 configs/colibri_vf_defconfig                |  1 +
 5 files changed, 55 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm/include/asm/arch-vf610/sys_proto.h

diff --git a/arch/arm/cpu/armv7/vf610/generic.c b/arch/arm/cpu/armv7/vf610/generic.c
index 0328096afd..59c5b1adca 100644
--- a/arch/arm/cpu/armv7/vf610/generic.c
+++ b/arch/arm/cpu/armv7/vf610/generic.c
@@ -376,3 +376,45 @@ void enable_caches(void)
 	mmu_set_region_dcache_behaviour(IRAM_BASE_ADDR, IRAM_SIZE, option);
 }
 #endif
+
+#ifdef CONFIG_IMX_BOOTAUX
+#define CCM_CCOWR_START 0x00015a5a
+
+const struct memorymap hostmap[] = {
+	{ .auxcore = 0x00000000, .host = 0x00000000, .size = 0x18000 },
+	{ .auxcore = 0x1f000000, .host = 0x3f000000, .size = 0x80000 },
+	{ .auxcore = 0x1f800000, .host = 0x1f800000, .size = 0x8000 },
+	{ .auxcore = 0x3f000000, .host = 0x3f000000, .size = 0x80000 },
+	{ .auxcore = 0x3f800000, .host = 0x3f800000, .size = 0x8000 },
+	{ .auxcore = 0x00800000, .host = 0x80800000, .size = 0x0f800000 },
+	{ .auxcore = 0x80000000, .host = 0x80000000, .size = 0xe0000000 },
+	{ /* sentinel */ }
+};
+
+/* The Cortex-M4 starts from the address present in SRC_GPR2 */
+int arch_auxiliary_core_up(u32 core_id, u32 stack, u32 pc)
+{
+	struct src *src = (struct src *)SRC_BASE_ADDR;
+	struct ccm_reg *ccm = (struct ccm_reg *)CCM_BASE_ADDR;
+
+	/* Set the PC for the Cortex-M4 */
+	writel(pc, &src->gpr2);
+
+	/* Enable M4 */
+	writel(CCM_CCOWR_START, &ccm->ccowr);
+
+	return 0;
+}
+
+int arch_auxiliary_core_check_up(u32 core_id)
+{
+	uint32_t val;
+	struct ccm_reg *ccm = (struct ccm_reg *)CCM_BASE_ADDR;
+
+	val = readl(&ccm->ccowr);
+	if (val & 0x00010000)
+		return 1;
+
+	return 0;
+}
+#endif
diff --git a/arch/arm/imx-common/Kconfig b/arch/arm/imx-common/Kconfig
index 7ee74d59a8..0b68202b46 100644
--- a/arch/arm/imx-common/Kconfig
+++ b/arch/arm/imx-common/Kconfig
@@ -14,7 +14,7 @@ config IMX_RDC
 
 config IMX_BOOTAUX
 	bool "Support boot auxiliary core"
-	depends on ARCH_MX7 || ARCH_MX6
+	depends on ARCH_MX7 || ARCH_MX6 || ARCH_VF610
 	help
 	  bootaux [addr] to boot auxiliary core.
 
diff --git a/arch/arm/imx-common/Makefile b/arch/arm/imx-common/Makefile
index b7cb434bd7..8c21c8a3e8 100644
--- a/arch/arm/imx-common/Makefile
+++ b/arch/arm/imx-common/Makefile
@@ -28,13 +28,15 @@ obj-y 	+= cache.o init.o
 obj-$(CONFIG_CMD_SATA) += sata.o
 obj-$(CONFIG_IMX_VIDEO_SKIP) += video.o
 obj-$(CONFIG_IMX_RDC) += rdc-sema.o
-obj-$(CONFIG_IMX_BOOTAUX) += imx_bootaux.o
 obj-$(CONFIG_SECURE_BOOT)    += hab.o
 endif
 ifeq ($(SOC),$(filter $(SOC),mx7ulp))
 obj-y  += cache.o
 obj-$(CONFIG_SECURE_BOOT) += hab.o
 endif
+ifeq ($(SOC),$(filter $(SOC),mx6 mx7 vf610))
+obj-$(CONFIG_IMX_BOOTAUX) += imx_bootaux.o
+endif
 ifeq ($(SOC),$(filter $(SOC),vf610))
 obj-y += ddrmc-vf610.o
 endif
diff --git a/arch/arm/include/asm/arch-vf610/sys_proto.h b/arch/arm/include/asm/arch-vf610/sys_proto.h
new file mode 100644
index 0000000000..2a39816c7b
--- /dev/null
+++ b/arch/arm/include/asm/arch-vf610/sys_proto.h
@@ -0,0 +1,8 @@
+/*
+ * (C) Copyright 2017
+ * Stefan Agner, Toradex
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <asm/imx-common/sys_proto.h>
diff --git a/configs/colibri_vf_defconfig b/configs/colibri_vf_defconfig
index 0474abc3c5..08c243923c 100644
--- a/configs/colibri_vf_defconfig
+++ b/configs/colibri_vf_defconfig
@@ -2,6 +2,7 @@ CONFIG_ARM=y
 CONFIG_SYS_THUMB_BUILD=y
 CONFIG_ARCH_VF610=y
 CONFIG_TARGET_COLIBRI_VF=y
+CONFIG_IMX_BOOTAUX=y
 CONFIG_DEFAULT_DEVICE_TREE="vf610-colibri"
 CONFIG_SYS_EXTRA_OPTIONS="IMX_CONFIG=board/toradex/colibri_vf/imximage.cfg,ENV_IS_IN_NAND,IMX_NAND"
 CONFIG_BOOTDELAY=1
-- 
2.12.1

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support
  2017-03-29 19:58 [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support Stefan Agner
                   ` (2 preceding siblings ...)
  2017-03-29 19:58 ` [U-Boot] [PATCH 3/3] ARM: vf610: add auxiliary core boot support Stefan Agner
@ 2017-04-03 11:20 ` Lukasz Majewski
  2017-04-03 21:36   ` Stefan Agner
  3 siblings, 1 reply; 25+ messages in thread
From: Lukasz Majewski @ 2017-04-03 11:20 UTC (permalink / raw)
  To: u-boot

Hi Stefan,

Thanks for your patch. Please allow me to share some ideas for
improvements.

> From: Stefan Agner <stefan.agner@toradex.com>
> 
> This patchset enables to boot elf binaries on secondary Cortex-M
> class cores available on i.MX 6SoloX/7Solo/7Dual. This makes
> handling and loading firmwares much more convinient since all
> information where the firmware has to be loaded to is contained in
> the elf headers. A typical usage looks like this:
> 
>   Colibri iMX7 # tftp ${loadaddr} firmware.elf && bootaux ${loadaddr}
>   Using FEC0 device
>   TFTP from server 192.168.10.1; our IP address is 192.168.10.2
>   Filename 'firmware.elf'.
>   Load address: 0x80800000
>   Loading: ##################################################  88.3
> KiB 5.4 MiB/s
>   done
>   Bytes transferred = 90424 (16138 hex)
>   ## Starting auxiliary core at 0x1FFF8311 ...
>   Colibri iMX7 #

I can find some other platforms (not only IMX), which would benefit
from this code - the generic 'bootaux' command.

One good example would to allow multiple binaries for different SoC
Cores (e.g. 2x Cortex-A8) to be loaded and started by u-boot.

Hence, I'm wondering if you could make those patches usable for other
platforms as well?

> 
> Note that the bootaux command retrieved the entry point (PC) from
> the elf binary.

Could you make this code flexible enough to handle not only elf
binaries, but also other data (e.g. FPGA bitstreams)?

Maybe it would better to:
-------------------------

Embrace those binaries into FIT file (*.itb)? And allow multiple
binaries loading? I'm thinking of work similar to one already did by
Andre Przywara for SPL:

"[PATCH v3 00/19] SPL: extend FIT loading support" posted on 31.03.2017?

In that way we would "open" many new use cases, and other platforms
(e.g. FPGA's, TI, etc) could benefit from it. 
One solid use case is to load different Linux binaries (or/and bare
metal programs) to different SoC cores.

The "meta data" (i.e. load address, data type, description), could be
extracted from the FIT format (the code is already in u-boot).

IMHO, this is very generic approach.

> Also all sections are translated to A7 addresses
> in order to properly load the firmware sections into the appropriate
> locations. 

This would require some tiny arch specific code.

> Also cache flushes is taken care of, so that there is no
> manual dcache flush necessary anymore.
> 
> The NXP FreeRTOS BSP already generates elf binaries which can be
> directly used with this elf binary support.
> 
> The last patch adds bootaux support for Vybrid.
> 
> 
> Stefan Agner (3):
>   imx: imx-common: move aux core image parsing to common code
>   imx: imx-common: add elf firmware support
>   ARM: vf610: add auxiliary core boot support
> 
>  arch/arm/cpu/armv7/mx6/soc.c                |  30 +++++---
>  arch/arm/cpu/armv7/mx7/soc.c                |  34 ++++++---
>  arch/arm/cpu/armv7/vf610/generic.c          |  42 +++++++++++
>  arch/arm/imx-common/Kconfig                 |   2 +-
>  arch/arm/imx-common/Makefile                |   4 +-
>  arch/arm/imx-common/imx_bootaux.c           | 105
> +++++++++++++++++++++++-----
> arch/arm/include/asm/arch-vf610/sys_proto.h |   8 +++
> arch/arm/include/asm/imx-common/sys_proto.h |   6 ++
> configs/colibri_vf_defconfig                |   1 + 9 files changed,
> 198 insertions(+), 34 deletions(-) create mode 100644
> arch/arm/include/asm/arch-vf610/sys_proto.h
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support
  2017-04-03 11:20 ` [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support Lukasz Majewski
@ 2017-04-03 21:36   ` Stefan Agner
  2017-04-03 22:07     ` Marek Vasut
  2017-04-04  8:23     ` Lukasz Majewski
  0 siblings, 2 replies; 25+ messages in thread
From: Stefan Agner @ 2017-04-03 21:36 UTC (permalink / raw)
  To: u-boot

Hi Lukasz,

On 2017-04-03 04:20, Lukasz Majewski wrote:
> Hi Stefan,
> 
> Thanks for your patch. Please allow me to share some ideas for
> improvements.
> 
>> From: Stefan Agner <stefan.agner@toradex.com>
>>
>> This patchset enables to boot elf binaries on secondary Cortex-M
>> class cores available on i.MX 6SoloX/7Solo/7Dual. This makes
>> handling and loading firmwares much more convinient since all
>> information where the firmware has to be loaded to is contained in
>> the elf headers. A typical usage looks like this:
>>
>>   Colibri iMX7 # tftp ${loadaddr} firmware.elf && bootaux ${loadaddr}
>>   Using FEC0 device
>>   TFTP from server 192.168.10.1; our IP address is 192.168.10.2
>>   Filename 'firmware.elf'.
>>   Load address: 0x80800000
>>   Loading: ##################################################  88.3
>> KiB 5.4 MiB/s
>>   done
>>   Bytes transferred = 90424 (16138 hex)
>>   ## Starting auxiliary core at 0x1FFF8311 ...
>>   Colibri iMX7 #
> 
> I can find some other platforms (not only IMX), which would benefit
> from this code - the generic 'bootaux' command.
> 
> One good example would to allow multiple binaries for different SoC
> Cores (e.g. 2x Cortex-A8) to be loaded and started by u-boot.
> 
> Hence, I'm wondering if you could make those patches usable for other
> platforms as well?

I don't think that this is a good idea. bootaux is meant for auxiliary
cores, which often use a different architecture and are not cache
coherent (hence the cache flushes).

On SMP systems the main operating system normally starts the secondary
core. Otherwise, if you want to run them separately using U-Boot, maybe
a new command such as bootsmp would be more suited.

> 
>>
>> Note that the bootaux command retrieved the entry point (PC) from
>> the elf binary.
> 
> Could you make this code flexible enough to handle not only elf
> binaries, but also other data (e.g. FPGA bitstreams)?

The code of bootaux is rather small, I don't see much value into stuff
boot code for other peripherals into it. I don't know how FPGA bistream
loading typically works, but I guess it is quite different from loading
a firmware into RAM/SRAM and flush caches...

I am not against reuse and unification, I just feel that this is not
really a case where we gain much.

> 
> Maybe it would better to:
> -------------------------
> 
> Embrace those binaries into FIT file (*.itb)? And allow multiple
> binaries loading? I'm thinking of work similar to one already did by
> Andre Przywara for SPL:
> 
> "[PATCH v3 00/19] SPL: extend FIT loading support" posted on 31.03.2017?
> 
> In that way we would "open" many new use cases, and other platforms
> (e.g. FPGA's, TI, etc) could benefit from it. 
> One solid use case is to load different Linux binaries (or/and bare
> metal programs) to different SoC cores.
> 
> The "meta data" (i.e. load address, data type, description), could be
> extracted from the FIT format (the code is already in u-boot).
> 
> IMHO, this is very generic approach.

I did some experiments with using FIT images for auxiliary core
firmware, however, it did not add a lot of advantage over using elf:
http://git.toradex.com/cgit/u-boot-toradex.git/commit/?h=2015.04-toradex-next&id=d1d416f272e840e8139aec911f89a70fe5523eb2

Firmwares are already built and available in the elf file format. The
Linux remoteproc framework, which is meant to handle this kind of cores
too, supports elf firmware loading too, so supporting elf in U-Boot too
is a nice alignment. Also using FIT adds an additional step when
building firm wares...


> 
>> Also all sections are translated to A7 addresses
>> in order to properly load the firmware sections into the appropriate
>> locations.
> 
> This would require some tiny arch specific code.
> 

Yes, but in my rather small patchset (164 additional lines) this is
actually almost the bulk of changes :-)

--
Stefan

>> Also cache flushes is taken care of, so that there is no
>> manual dcache flush necessary anymore.
>>
>> The NXP FreeRTOS BSP already generates elf binaries which can be
>> directly used with this elf binary support.
>>
>> The last patch adds bootaux support for Vybrid.
>>
>>
>> Stefan Agner (3):
>>   imx: imx-common: move aux core image parsing to common code
>>   imx: imx-common: add elf firmware support
>>   ARM: vf610: add auxiliary core boot support
>>
>>  arch/arm/cpu/armv7/mx6/soc.c                |  30 +++++---
>>  arch/arm/cpu/armv7/mx7/soc.c                |  34 ++++++---
>>  arch/arm/cpu/armv7/vf610/generic.c          |  42 +++++++++++
>>  arch/arm/imx-common/Kconfig                 |   2 +-
>>  arch/arm/imx-common/Makefile                |   4 +-
>>  arch/arm/imx-common/imx_bootaux.c           | 105
>> +++++++++++++++++++++++-----
>> arch/arm/include/asm/arch-vf610/sys_proto.h |   8 +++
>> arch/arm/include/asm/imx-common/sys_proto.h |   6 ++
>> configs/colibri_vf_defconfig                |   1 + 9 files changed,
>> 198 insertions(+), 34 deletions(-) create mode 100644
>> arch/arm/include/asm/arch-vf610/sys_proto.h
>>
> 
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support
  2017-04-03 21:36   ` Stefan Agner
@ 2017-04-03 22:07     ` Marek Vasut
  2017-04-03 22:42       ` Stefan Agner
  2017-04-04  8:25       ` Lukasz Majewski
  2017-04-04  8:23     ` Lukasz Majewski
  1 sibling, 2 replies; 25+ messages in thread
From: Marek Vasut @ 2017-04-03 22:07 UTC (permalink / raw)
  To: u-boot

On 04/03/2017 11:36 PM, Stefan Agner wrote:
> Hi Lukasz,
> 
> On 2017-04-03 04:20, Lukasz Majewski wrote:
>> Hi Stefan,
>>
>> Thanks for your patch. Please allow me to share some ideas for
>> improvements.
>>
>>> From: Stefan Agner <stefan.agner@toradex.com>
>>>
>>> This patchset enables to boot elf binaries on secondary Cortex-M
>>> class cores available on i.MX 6SoloX/7Solo/7Dual. This makes
>>> handling and loading firmwares much more convinient since all
>>> information where the firmware has to be loaded to is contained in
>>> the elf headers. A typical usage looks like this:
>>>
>>>   Colibri iMX7 # tftp ${loadaddr} firmware.elf && bootaux ${loadaddr}
>>>   Using FEC0 device
>>>   TFTP from server 192.168.10.1; our IP address is 192.168.10.2
>>>   Filename 'firmware.elf'.
>>>   Load address: 0x80800000
>>>   Loading: ##################################################  88.3
>>> KiB 5.4 MiB/s
>>>   done
>>>   Bytes transferred = 90424 (16138 hex)
>>>   ## Starting auxiliary core at 0x1FFF8311 ...
>>>   Colibri iMX7 #
>>
>> I can find some other platforms (not only IMX), which would benefit
>> from this code - the generic 'bootaux' command.
>>
>> One good example would to allow multiple binaries for different SoC
>> Cores (e.g. 2x Cortex-A8) to be loaded and started by u-boot.
>>
>> Hence, I'm wondering if you could make those patches usable for other
>> platforms as well?
> 
> I don't think that this is a good idea. bootaux is meant for auxiliary
> cores, which often use a different architecture and are not cache
> coherent (hence the cache flushes).
> 
> On SMP systems the main operating system normally starts the secondary
> core. Otherwise, if you want to run them separately using U-Boot, maybe
> a new command such as bootsmp would be more suited.
> 
Admitedly, I didn't look at the patch, but if you want to boot ad-hoc
cores, you can very well also boot secondary cores on the current CPU
complex with the same command. Why not ?

Also, I think this might come useful when booting stuff like "Altera
Sparrow" ...

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support
  2017-04-03 22:07     ` Marek Vasut
@ 2017-04-03 22:42       ` Stefan Agner
  2017-04-03 23:34         ` Marek Vasut
  2017-04-04  8:25       ` Lukasz Majewski
  1 sibling, 1 reply; 25+ messages in thread
From: Stefan Agner @ 2017-04-03 22:42 UTC (permalink / raw)
  To: u-boot

On 2017-04-03 15:07, Marek Vasut wrote:
> On 04/03/2017 11:36 PM, Stefan Agner wrote:
>> Hi Lukasz,
>>
>> On 2017-04-03 04:20, Lukasz Majewski wrote:
>>> Hi Stefan,
>>>
>>> Thanks for your patch. Please allow me to share some ideas for
>>> improvements.
>>>
>>>> From: Stefan Agner <stefan.agner@toradex.com>
>>>>
>>>> This patchset enables to boot elf binaries on secondary Cortex-M
>>>> class cores available on i.MX 6SoloX/7Solo/7Dual. This makes
>>>> handling and loading firmwares much more convinient since all
>>>> information where the firmware has to be loaded to is contained in
>>>> the elf headers. A typical usage looks like this:
>>>>
>>>>   Colibri iMX7 # tftp ${loadaddr} firmware.elf && bootaux ${loadaddr}
>>>>   Using FEC0 device
>>>>   TFTP from server 192.168.10.1; our IP address is 192.168.10.2
>>>>   Filename 'firmware.elf'.
>>>>   Load address: 0x80800000
>>>>   Loading: ##################################################  88.3
>>>> KiB 5.4 MiB/s
>>>>   done
>>>>   Bytes transferred = 90424 (16138 hex)
>>>>   ## Starting auxiliary core at 0x1FFF8311 ...
>>>>   Colibri iMX7 #
>>>
>>> I can find some other platforms (not only IMX), which would benefit
>>> from this code - the generic 'bootaux' command.
>>>
>>> One good example would to allow multiple binaries for different SoC
>>> Cores (e.g. 2x Cortex-A8) to be loaded and started by u-boot.
>>>
>>> Hence, I'm wondering if you could make those patches usable for other
>>> platforms as well?
>>
>> I don't think that this is a good idea. bootaux is meant for auxiliary
>> cores, which often use a different architecture and are not cache
>> coherent (hence the cache flushes).
>>
>> On SMP systems the main operating system normally starts the secondary
>> core. Otherwise, if you want to run them separately using U-Boot, maybe
>> a new command such as bootsmp would be more suited.
>>
> Admitedly, I didn't look at the patch, but if you want to boot ad-hoc
> cores, you can very well also boot secondary cores on the current CPU
> complex with the same command. Why not ?

Sure, it could be done. I just feel it is not the right design.

Auxiliary cores have usually a different view to memory, this is why I
had to add the get_host_mapping callback in the elf loader code to let
architecture dependent code translate to host addresses. SMP systems
don't need that.

Also flush caches is not necessary on some cache coherent CPU's
(depending on how your cache coherence between I and D cache looks
like).

Creating a new command like bootaux comes with very few overhead.

This are the reasons why I feel creating a new command for a SMP boot
case makes more sense. We can still reuse functions which are very
similar by moving them into some common location, where it makes sense.

> 
> Also, I think this might come useful when booting stuff like "Altera
> Sparrow" ...

I am not familiar with that architecture, what kind of core does it
provide which needs to be booted by U-Boot?

--
Stefan

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support
  2017-04-03 22:42       ` Stefan Agner
@ 2017-04-03 23:34         ` Marek Vasut
  2017-04-04  0:02           ` Stefan Agner
  0 siblings, 1 reply; 25+ messages in thread
From: Marek Vasut @ 2017-04-03 23:34 UTC (permalink / raw)
  To: u-boot

On 04/04/2017 12:42 AM, Stefan Agner wrote:
> On 2017-04-03 15:07, Marek Vasut wrote:
>> On 04/03/2017 11:36 PM, Stefan Agner wrote:
>>> Hi Lukasz,
>>>
>>> On 2017-04-03 04:20, Lukasz Majewski wrote:
>>>> Hi Stefan,
>>>>
>>>> Thanks for your patch. Please allow me to share some ideas for
>>>> improvements.
>>>>
>>>>> From: Stefan Agner <stefan.agner@toradex.com>
>>>>>
>>>>> This patchset enables to boot elf binaries on secondary Cortex-M
>>>>> class cores available on i.MX 6SoloX/7Solo/7Dual. This makes
>>>>> handling and loading firmwares much more convinient since all
>>>>> information where the firmware has to be loaded to is contained in
>>>>> the elf headers. A typical usage looks like this:
>>>>>
>>>>>   Colibri iMX7 # tftp ${loadaddr} firmware.elf && bootaux ${loadaddr}
>>>>>   Using FEC0 device
>>>>>   TFTP from server 192.168.10.1; our IP address is 192.168.10.2
>>>>>   Filename 'firmware.elf'.
>>>>>   Load address: 0x80800000
>>>>>   Loading: ##################################################  88.3
>>>>> KiB 5.4 MiB/s
>>>>>   done
>>>>>   Bytes transferred = 90424 (16138 hex)
>>>>>   ## Starting auxiliary core at 0x1FFF8311 ...
>>>>>   Colibri iMX7 #
>>>>
>>>> I can find some other platforms (not only IMX), which would benefit
>>>> from this code - the generic 'bootaux' command.
>>>>
>>>> One good example would to allow multiple binaries for different SoC
>>>> Cores (e.g. 2x Cortex-A8) to be loaded and started by u-boot.
>>>>
>>>> Hence, I'm wondering if you could make those patches usable for other
>>>> platforms as well?
>>>
>>> I don't think that this is a good idea. bootaux is meant for auxiliary
>>> cores, which often use a different architecture and are not cache
>>> coherent (hence the cache flushes).
>>>
>>> On SMP systems the main operating system normally starts the secondary
>>> core. Otherwise, if you want to run them separately using U-Boot, maybe
>>> a new command such as bootsmp would be more suited.
>>>
>> Admitedly, I didn't look at the patch, but if you want to boot ad-hoc
>> cores, you can very well also boot secondary cores on the current CPU
>> complex with the same command. Why not ?
> 
> Sure, it could be done. I just feel it is not the right design.
> 
> Auxiliary cores have usually a different view to memory, this is why I
> had to add the get_host_mapping callback in the elf loader code to let
> architecture dependent code translate to host addresses. SMP systems
> don't need that.
> 
> Also flush caches is not necessary on some cache coherent CPU's
> (depending on how your cache coherence between I and D cache looks
> like).

So SMP is just a reduced special-case of this , yes ?

> Creating a new command like bootaux comes with very few overhead.

The overhead is the new command, we already have many ad-hoc commands.

> This are the reasons why I feel creating a new command for a SMP boot
> case makes more sense. We can still reuse functions which are very
> similar by moving them into some common location, where it makes sense.
> 
>>
>> Also, I think this might come useful when booting stuff like "Altera
>> Sparrow" ...
> 
> I am not familiar with that architecture, what kind of core does it
> provide which needs to be booted by U-Boot?

The secondary ARM core in the SoCFPGA C-A9 complex or Nios2 core in the
FPGA.

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support
  2017-04-03 23:34         ` Marek Vasut
@ 2017-04-04  0:02           ` Stefan Agner
  2017-04-04  8:46             ` Lukasz Majewski
  2017-04-04  9:22             ` Marek Vasut
  0 siblings, 2 replies; 25+ messages in thread
From: Stefan Agner @ 2017-04-04  0:02 UTC (permalink / raw)
  To: u-boot

On 2017-04-03 16:34, Marek Vasut wrote:
> On 04/04/2017 12:42 AM, Stefan Agner wrote:
>> On 2017-04-03 15:07, Marek Vasut wrote:
>>> On 04/03/2017 11:36 PM, Stefan Agner wrote:
>>>> Hi Lukasz,
>>>>
>>>> On 2017-04-03 04:20, Lukasz Majewski wrote:
>>>>> Hi Stefan,
>>>>>
>>>>> Thanks for your patch. Please allow me to share some ideas for
>>>>> improvements.
>>>>>
>>>>>> From: Stefan Agner <stefan.agner@toradex.com>
>>>>>>
>>>>>> This patchset enables to boot elf binaries on secondary Cortex-M
>>>>>> class cores available on i.MX 6SoloX/7Solo/7Dual. This makes
>>>>>> handling and loading firmwares much more convinient since all
>>>>>> information where the firmware has to be loaded to is contained in
>>>>>> the elf headers. A typical usage looks like this:
>>>>>>
>>>>>>   Colibri iMX7 # tftp ${loadaddr} firmware.elf && bootaux ${loadaddr}
>>>>>>   Using FEC0 device
>>>>>>   TFTP from server 192.168.10.1; our IP address is 192.168.10.2
>>>>>>   Filename 'firmware.elf'.
>>>>>>   Load address: 0x80800000
>>>>>>   Loading: ##################################################  88.3
>>>>>> KiB 5.4 MiB/s
>>>>>>   done
>>>>>>   Bytes transferred = 90424 (16138 hex)
>>>>>>   ## Starting auxiliary core at 0x1FFF8311 ...
>>>>>>   Colibri iMX7 #
>>>>>
>>>>> I can find some other platforms (not only IMX), which would benefit
>>>>> from this code - the generic 'bootaux' command.
>>>>>
>>>>> One good example would to allow multiple binaries for different SoC
>>>>> Cores (e.g. 2x Cortex-A8) to be loaded and started by u-boot.
>>>>>
>>>>> Hence, I'm wondering if you could make those patches usable for other
>>>>> platforms as well?
>>>>
>>>> I don't think that this is a good idea. bootaux is meant for auxiliary
>>>> cores, which often use a different architecture and are not cache
>>>> coherent (hence the cache flushes).
>>>>
>>>> On SMP systems the main operating system normally starts the secondary
>>>> core. Otherwise, if you want to run them separately using U-Boot, maybe
>>>> a new command such as bootsmp would be more suited.
>>>>
>>> Admitedly, I didn't look at the patch, but if you want to boot ad-hoc
>>> cores, you can very well also boot secondary cores on the current CPU
>>> complex with the same command. Why not ?
>>
>> Sure, it could be done. I just feel it is not the right design.
>>
>> Auxiliary cores have usually a different view to memory, this is why I
>> had to add the get_host_mapping callback in the elf loader code to let
>> architecture dependent code translate to host addresses. SMP systems
>> don't need that.
>>
>> Also flush caches is not necessary on some cache coherent CPU's
>> (depending on how your cache coherence between I and D cache looks
>> like).
> 
> So SMP is just a reduced special-case of this , yes ?

Yeah, I guess you can get away with dummy callback implementation and a
performance hit due to cash flushes.

> 
>> Creating a new command like bootaux comes with very few overhead.
> 
> The overhead is the new command, we already have many ad-hoc commands.
> 

Agreed, and I really wished that this got discussed when that command
initially got added. I brought it up back then...
https://lists.denx.de/pipermail/u-boot/2016-January/240323.html

It seemed to be acceptable to just add this ad hoc command, with some
"random binary format" support back then... Ok, it is not entirely
random, since it is the format of a binary how it ends up in
microcontrollers execute in place flash (stack pointer and reset vector
are the two first words).... However, making this ad hoc command now
generic really feels weird to me, since we would end up supporting that
format for A class CPUs etc... bootaux is really suited for auxiliary
M-class cores on ARM, as it is right now. Maybe we should have named it
bootm ;-)

>> This are the reasons why I feel creating a new command for a SMP boot
>> case makes more sense. We can still reuse functions which are very
>> similar by moving them into some common location, where it makes sense.
>>
>>>
>>> Also, I think this might come useful when booting stuff like "Altera
>>> Sparrow" ...
>>
>> I am not familiar with that architecture, what kind of core does it
>> provide which needs to be booted by U-Boot?
> 
> The secondary ARM core in the SoCFPGA C-A9 complex or Nios2 core in the
> FPGA.

In my thinking, the Nios2 core seems like such a remote processor well
suited for the bootaux command. For the secondary A9, I would create a
new command.

If we want to support the two with the same command, we already have a
first problem: How do we address them? Of course, we could add just a
index or something, but this would already break backward compatibility
of the bootaux command.

--
Stefan

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support
  2017-04-03 21:36   ` Stefan Agner
  2017-04-03 22:07     ` Marek Vasut
@ 2017-04-04  8:23     ` Lukasz Majewski
  2017-04-04 18:59       ` Stefan Agner
  1 sibling, 1 reply; 25+ messages in thread
From: Lukasz Majewski @ 2017-04-04  8:23 UTC (permalink / raw)
  To: u-boot

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="windows-1254", Size: 8144 bytes --]

Hi Stefan,

> Hi Lukasz,
> 
> On 2017-04-03 04:20, Lukasz Majewski wrote:
> > Hi Stefan,
> > 
> > Thanks for your patch. Please allow me to share some ideas for
> > improvements.
> > 
> >> From: Stefan Agner <stefan.agner@toradex.com>
> >>
> >> This patchset enables to boot elf binaries on secondary Cortex-M
> >> class cores available on i.MX 6SoloX/7Solo/7Dual. This makes
> >> handling and loading firmwares much more convinient since all
> >> information where the firmware has to be loaded to is contained in
> >> the elf headers. A typical usage looks like this:
> >>
> >>   Colibri iMX7 # tftp ${loadaddr} firmware.elf && bootaux
> >> ${loadaddr} Using FEC0 device
> >>   TFTP from server 192.168.10.1; our IP address is 192.168.10.2
> >>   Filename 'firmware.elf'.
> >>   Load address: 0x80800000
> >>   Loading: ##################################################  88.3
> >> KiB 5.4 MiB/s
> >>   done
> >>   Bytes transferred = 90424 (16138 hex)
> >>   ## Starting auxiliary core at 0x1FFF8311 ...
> >>   Colibri iMX7 #
> > 
> > I can find some other platforms (not only IMX), which would benefit
> > from this code - the generic 'bootaux' command.
> > 
> > One good example would to allow multiple binaries for different SoC
> > Cores (e.g. 2x Cortex-A8) to be loaded and started by u-boot.
> > 
> > Hence, I'm wondering if you could make those patches usable for
> > other platforms as well?
> 
> I don't think that this is a good idea. bootaux is meant for auxiliary
> cores, which often use a different architecture and are not cache
> coherent (hence the cache flushes).

I do remember, that I saw similar "tiny" patch to add "just one" ad-hoc
command to do the same (conceptually) for TI SoC floating on the u-boot
mailing list.

Please correct me if I'm wrong, but bootaux is IMX specific and does
work, which very likely, will be also required by other SoC vendors
(TI's Sitara is also equipped with Cortex-M4 and PRUSS).

Unification of such effort can save us all a lot of trouble in a very
near future.

> 
> On SMP systems the main operating system normally starts the secondary
> core. 

I think that there are some conceptual similarities between loading code
to SMP core and Cortex M3. Of course some "tweaks" would be needed.

> Otherwise, if you want to run them separately using U-Boot,
> maybe a new command such as bootsmp would be more suited.

Hmm - I do think that it would be too much:

- bootm for generic single core
- bootaux for IMX
- bootsmp for SMP (on IMX/TI/RK?)
- boot?? for ??

I would like to avoid adding new commands for doing conceptually the
same work.

Even better, we could extend bootm to support the "multi binary" case
too, but IMHO it would be also correct to have "bootaux" to handle
generic binaries loading.

> 
> > 
> >>
> >> Note that the bootaux command retrieved the entry point (PC) from
> >> the elf binary.
> > 
> > Could you make this code flexible enough to handle not only elf
> > binaries, but also other data (e.g. FPGA bitstreams)?
> 
> The code of bootaux is rather small, I don't see much value into stuff
> boot code for other peripherals into it.

But I'm not asking to write support for other vendor's SoC/use cases.

I'm just wondering if you could write generic command (framework) to
support this use case and as an example add support for your IMX's
Cortex-M3/4.

We would kill two birds with one stone - IMX is supported from the very
beginning and we do have generic code to extend it by other vendors.

> I don't know how FPGA
> bistream loading typically works, but I guess it is quite different
> from loading a firmware into RAM/SRAM and flush caches...

FPGA quirks would be handled in arch/SoC specific code.

> 
> I am not against reuse and unification, I just feel that this is not
> really a case where we gain much.

With generic "bootaux/bootm" command we can point other developers who
would like to add such booting code to the already available framework.

Also, we would prevent other "ad-hoc" commands adding to u-boot.

> 
> > 
> > Maybe it would better to:
> > -------------------------
> > 
> > Embrace those binaries into FIT file (*.itb)? And allow multiple
> > binaries loading? I'm thinking of work similar to one already did by
> > Andre Przywara for SPL:
> > 
> > "[PATCH v3 00/19] SPL: extend FIT loading support" posted on
> > 31.03.2017?
> > 
> > In that way we would "open" many new use cases, and other platforms
> > (e.g. FPGA's, TI, etc) could benefit from it. 
> > One solid use case is to load different Linux binaries (or/and bare
> > metal programs) to different SoC cores.
> > 
> > The "meta data" (i.e. load address, data type, description), could
> > be extracted from the FIT format (the code is already in u-boot).
> > 
> > IMHO, this is very generic approach.
> 
> I did some experiments with using FIT images for auxiliary core
> firmware, however, it did not add a lot of advantage over using elf:
> http://git.toradex.com/cgit/u-boot-toradex.git/commit/?h=2015.04-toradex-next&id=d1d416f272e840e8139aec911f89a70fe5523eb2

Unfortunately, not all use cases allow using ELF format. FIT gives more
flexibility:

-  ./doc/README.dfutftp -> different images loading

-  Andre's patch to load multiple binaries in SPL - [PATCH v3 00/19]
  SPL: extend FIT loading support"

> 
> Firmwares are already built and available in the elf file format. The
> Linux remoteproc framework, which is meant to handle this kind of
> cores too, supports elf firmware loading too, so supporting elf in
> U-Boot too is a nice alignment. Also using FIT adds an additional
> step when building firm wares...

But this is all OK.

The ELF binary would be wrapped in FIT (with e.g. "elf" property, even
1 to 1 mapping). Then the 'bootaux/bootm' could parse ELF (which is also
generic). And then some "IMX specific" (arch/SoC) code would be
executed.


> 
> 
> > 
> >> Also all sections are translated to A7 addresses
> >> in order to properly load the firmware sections into the
> >> appropriate locations.
> > 
> > This would require some tiny arch specific code.
> > 
> 
> Yes, but in my rather small patchset (164 additional lines) this is
> actually almost the bulk of changes :-)

Yes, I fully understand you :-) - I do remember when I wanted to add 3
lines (polarity inversion) to IMX PWM subsystem some time ago ... :-) 

Best regards,
Łukasz Majewski

> 
> --
> Stefan
> 
> >> Also cache flushes is taken care of, so that there is no
> >> manual dcache flush necessary anymore.
> >>
> >> The NXP FreeRTOS BSP already generates elf binaries which can be
> >> directly used with this elf binary support.
> >>
> >> The last patch adds bootaux support for Vybrid.
> >>
> >>
> >> Stefan Agner (3):
> >>   imx: imx-common: move aux core image parsing to common code
> >>   imx: imx-common: add elf firmware support
> >>   ARM: vf610: add auxiliary core boot support
> >>
> >>  arch/arm/cpu/armv7/mx6/soc.c                |  30 +++++---
> >>  arch/arm/cpu/armv7/mx7/soc.c                |  34 ++++++---
> >>  arch/arm/cpu/armv7/vf610/generic.c          |  42 +++++++++++
> >>  arch/arm/imx-common/Kconfig                 |   2 +-
> >>  arch/arm/imx-common/Makefile                |   4 +-
> >>  arch/arm/imx-common/imx_bootaux.c           | 105
> >> +++++++++++++++++++++++-----
> >> arch/arm/include/asm/arch-vf610/sys_proto.h |   8 +++
> >> arch/arm/include/asm/imx-common/sys_proto.h |   6 ++
> >> configs/colibri_vf_defconfig                |   1 + 9 files
> >> changed, 198 insertions(+), 34 deletions(-) create mode 100644
> >> arch/arm/include/asm/arch-vf610/sys_proto.h
> >>
> > 
> > 
> > 
> > 
> > Best regards,
> > 
> > Lukasz Majewski
> > 
> > --
> > 
> > DENX Software Engineering GmbH,      Managing Director: Wolfgang
> > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
> > Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email:
> > wd at denx.de




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support
  2017-04-03 22:07     ` Marek Vasut
  2017-04-03 22:42       ` Stefan Agner
@ 2017-04-04  8:25       ` Lukasz Majewski
  1 sibling, 0 replies; 25+ messages in thread
From: Lukasz Majewski @ 2017-04-04  8:25 UTC (permalink / raw)
  To: u-boot

Hi Marek, Stefan,

> On 04/03/2017 11:36 PM, Stefan Agner wrote:
> > Hi Lukasz,
> > 
> > On 2017-04-03 04:20, Lukasz Majewski wrote:
> >> Hi Stefan,
> >>
> >> Thanks for your patch. Please allow me to share some ideas for
> >> improvements.
> >>
> >>> From: Stefan Agner <stefan.agner@toradex.com>
> >>>
> >>> This patchset enables to boot elf binaries on secondary Cortex-M
> >>> class cores available on i.MX 6SoloX/7Solo/7Dual. This makes
> >>> handling and loading firmwares much more convinient since all
> >>> information where the firmware has to be loaded to is contained in
> >>> the elf headers. A typical usage looks like this:
> >>>
> >>>   Colibri iMX7 # tftp ${loadaddr} firmware.elf && bootaux
> >>> ${loadaddr} Using FEC0 device
> >>>   TFTP from server 192.168.10.1; our IP address is 192.168.10.2
> >>>   Filename 'firmware.elf'.
> >>>   Load address: 0x80800000
> >>>   Loading: ##################################################
> >>> 88.3 KiB 5.4 MiB/s
> >>>   done
> >>>   Bytes transferred = 90424 (16138 hex)
> >>>   ## Starting auxiliary core at 0x1FFF8311 ...
> >>>   Colibri iMX7 #
> >>
> >> I can find some other platforms (not only IMX), which would benefit
> >> from this code - the generic 'bootaux' command.
> >>
> >> One good example would to allow multiple binaries for different SoC
> >> Cores (e.g. 2x Cortex-A8) to be loaded and started by u-boot.
> >>
> >> Hence, I'm wondering if you could make those patches usable for
> >> other platforms as well?
> > 
> > I don't think that this is a good idea. bootaux is meant for
> > auxiliary cores, which often use a different architecture and are
> > not cache coherent (hence the cache flushes).
> > 
> > On SMP systems the main operating system normally starts the
> > secondary core. Otherwise, if you want to run them separately using
> > U-Boot, maybe a new command such as bootsmp would be more suited.
> > 
> Admitedly, I didn't look at the patch, but if you want to boot ad-hoc
> cores, you can very well also boot secondary cores on the current CPU
> complex with the same command. Why not ?
> 
> Also, I think this might come useful when booting stuff like "Altera
> Sparrow" ...

This is one of already available use cases in my mind. That is why I do
see the need to add generic approach here.



Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support
  2017-04-04  0:02           ` Stefan Agner
@ 2017-04-04  8:46             ` Lukasz Majewski
  2017-04-04  9:22             ` Marek Vasut
  1 sibling, 0 replies; 25+ messages in thread
From: Lukasz Majewski @ 2017-04-04  8:46 UTC (permalink / raw)
  To: u-boot

Hi Stefan,

> On 2017-04-03 16:34, Marek Vasut wrote:
> > On 04/04/2017 12:42 AM, Stefan Agner wrote:
> >> On 2017-04-03 15:07, Marek Vasut wrote:
> >>> On 04/03/2017 11:36 PM, Stefan Agner wrote:
> >>>> Hi Lukasz,
> >>>>
> >>>> On 2017-04-03 04:20, Lukasz Majewski wrote:
> >>>>> Hi Stefan,
> >>>>>
> >>>>> Thanks for your patch. Please allow me to share some ideas for
> >>>>> improvements.
> >>>>>
> >>>>>> From: Stefan Agner <stefan.agner@toradex.com>
> >>>>>>
> >>>>>> This patchset enables to boot elf binaries on secondary
> >>>>>> Cortex-M class cores available on i.MX 6SoloX/7Solo/7Dual.
> >>>>>> This makes handling and loading firmwares much more convinient
> >>>>>> since all information where the firmware has to be loaded to
> >>>>>> is contained in the elf headers. A typical usage looks like
> >>>>>> this:
> >>>>>>
> >>>>>>   Colibri iMX7 # tftp ${loadaddr} firmware.elf && bootaux
> >>>>>> ${loadaddr} Using FEC0 device
> >>>>>>   TFTP from server 192.168.10.1; our IP address is 192.168.10.2
> >>>>>>   Filename 'firmware.elf'.
> >>>>>>   Load address: 0x80800000
> >>>>>>   Loading: ##################################################
> >>>>>> 88.3 KiB 5.4 MiB/s
> >>>>>>   done
> >>>>>>   Bytes transferred = 90424 (16138 hex)
> >>>>>>   ## Starting auxiliary core at 0x1FFF8311 ...
> >>>>>>   Colibri iMX7 #
> >>>>>
> >>>>> I can find some other platforms (not only IMX), which would
> >>>>> benefit from this code - the generic 'bootaux' command.
> >>>>>
> >>>>> One good example would to allow multiple binaries for different
> >>>>> SoC Cores (e.g. 2x Cortex-A8) to be loaded and started by
> >>>>> u-boot.
> >>>>>
> >>>>> Hence, I'm wondering if you could make those patches usable for
> >>>>> other platforms as well?
> >>>>
> >>>> I don't think that this is a good idea. bootaux is meant for
> >>>> auxiliary cores, which often use a different architecture and
> >>>> are not cache coherent (hence the cache flushes).
> >>>>
> >>>> On SMP systems the main operating system normally starts the
> >>>> secondary core. Otherwise, if you want to run them separately
> >>>> using U-Boot, maybe a new command such as bootsmp would be more
> >>>> suited.
> >>>>
> >>> Admitedly, I didn't look at the patch, but if you want to boot
> >>> ad-hoc cores, you can very well also boot secondary cores on the
> >>> current CPU complex with the same command. Why not ?
> >>
> >> Sure, it could be done. I just feel it is not the right design.
> >>
> >> Auxiliary cores have usually a different view to memory, this is
> >> why I had to add the get_host_mapping callback in the elf loader
> >> code to let architecture dependent code translate to host
> >> addresses. SMP systems don't need that.
> >>
> >> Also flush caches is not necessary on some cache coherent CPU's
> >> (depending on how your cache coherence between I and D cache looks
> >> like).
> > 
> > So SMP is just a reduced special-case of this , yes ?
> 
> Yeah, I guess you can get away with dummy callback implementation and
> a performance hit due to cash flushes.
> 
> > 
> >> Creating a new command like bootaux comes with very few overhead.
> > 
> > The overhead is the new command, we already have many ad-hoc
> > commands.
> > 
> 
> Agreed, and I really wished that this got discussed when that command
> initially got added. I brought it up back then...
> https://lists.denx.de/pipermail/u-boot/2016-January/240323.html

I can only regret that this discussion did not happened then...

> 
> It seemed to be acceptable to just add this ad hoc command, with some
> "random binary format" support back then... Ok, it is not entirely
> random, since it is the format of a binary how it ends up in
> microcontrollers execute in place flash (stack pointer and reset
> vector are the two first words).... However, making this ad hoc
> command now generic really feels weird to me, since we would end up
> supporting that format for A class CPUs etc... bootaux is really
> suited for auxiliary M-class cores on ARM, as it is right now. Maybe
> we should have named it bootm ;-)

Please refer to my reply to an earlier mail in this thread.

> 
> >> This are the reasons why I feel creating a new command for a SMP
> >> boot case makes more sense. We can still reuse functions which are
> >> very similar by moving them into some common location, where it
> >> makes sense.
> >>
> >>>
> >>> Also, I think this might come useful when booting stuff like
> >>> "Altera Sparrow" ...
> >>
> >> I am not familiar with that architecture, what kind of core does it
> >> provide which needs to be booted by U-Boot?
> > 
> > The secondary ARM core in the SoCFPGA C-A9 complex or Nios2 core in
> > the FPGA.
> 
> In my thinking, the Nios2 core seems like such a remote processor well
> suited for the bootaux command. For the secondary A9, I would create a
> new command.
> 
> If we want to support the two with the same command, we already have a
> first problem: How do we address them? Of course, we could add just a
> index or something, but this would already break backward
> compatibility of the bootaux command.
> 
> --
> Stefan
> 
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support
  2017-04-04  0:02           ` Stefan Agner
  2017-04-04  8:46             ` Lukasz Majewski
@ 2017-04-04  9:22             ` Marek Vasut
  2017-04-04 17:57               ` Stefan Agner
  1 sibling, 1 reply; 25+ messages in thread
From: Marek Vasut @ 2017-04-04  9:22 UTC (permalink / raw)
  To: u-boot

On 04/04/2017 02:02 AM, Stefan Agner wrote:
[...]
>>>> Admitedly, I didn't look at the patch, but if you want to boot ad-hoc
>>>> cores, you can very well also boot secondary cores on the current CPU
>>>> complex with the same command. Why not ?
>>>
>>> Sure, it could be done. I just feel it is not the right design.
>>>
>>> Auxiliary cores have usually a different view to memory, this is why I
>>> had to add the get_host_mapping callback in the elf loader code to let
>>> architecture dependent code translate to host addresses. SMP systems
>>> don't need that.
>>>
>>> Also flush caches is not necessary on some cache coherent CPU's
>>> (depending on how your cache coherence between I and D cache looks
>>> like).
>>
>> So SMP is just a reduced special-case of this , yes ?
> 
> Yeah, I guess you can get away with dummy callback implementation and a
> performance hit due to cash flushes.

Or you can abstract things out ?

>>> Creating a new command like bootaux comes with very few overhead.
>>
>> The overhead is the new command, we already have many ad-hoc commands.
>>
> 
> Agreed, and I really wished that this got discussed when that command
> initially got added. I brought it up back then...
> https://lists.denx.de/pipermail/u-boot/2016-January/240323.html
> 
> It seemed to be acceptable to just add this ad hoc command, with some
> "random binary format" support back then...

Based on that discussion, I only see that noone opposed, but I don't see
any agreement.

> Ok, it is not entirely
> random, since it is the format of a binary how it ends up in
> microcontrollers execute in place flash (stack pointer and reset vector
> are the two first words)....

I thought this command was starting the core by loading data to RAM ,
not flash ?

> However, making this ad hoc command now
> generic really feels weird to me, since we would end up supporting that
> format for A class CPUs etc... bootaux is really suited for auxiliary
> M-class cores on ARM, as it is right now. Maybe we should have named it
> bootm ;-)

We always try to avoid one-off hacks, so it's not weird. I still don't
quite understand how it is a problem to abstract things out . I am not
asking you to support CA, but to avoid CM specific implementation which
cannot be extended to support CA or even MIPS/Nios2/etc .

>>> This are the reasons why I feel creating a new command for a SMP boot
>>> case makes more sense. We can still reuse functions which are very
>>> similar by moving them into some common location, where it makes sense.
>>>
>>>>
>>>> Also, I think this might come useful when booting stuff like "Altera
>>>> Sparrow" ...
>>>
>>> I am not familiar with that architecture, what kind of core does it
>>> provide which needs to be booted by U-Boot?
>>
>> The secondary ARM core in the SoCFPGA C-A9 complex or Nios2 core in the
>> FPGA.
> 
> In my thinking, the Nios2 core seems like such a remote processor well
> suited for the bootaux command. For the secondary A9, I would create a
> new command.

Uh, why, that does not make any sense to me. Both the random core in
FPGA and the secondary CA9 core have almost the same bringup sequence.
What is so different about this stuff ?

> If we want to support the two with the same command, we already have a
> first problem: How do we address them? Of course, we could add just a
> index or something, but this would already break backward compatibility
> of the bootaux command.

So we already accepted a command which has shit commandline semantics
and we're stuck with it , hrm. You can always specify a default core
index for those two iMX boards and it's done. Or just fix the semantics
in their default environment, which is not awesome, but I doubt this is
wildly used yet.

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support
  2017-04-04  9:22             ` Marek Vasut
@ 2017-04-04 17:57               ` Stefan Agner
  2017-04-04 18:38                 ` Marek Vasut
  0 siblings, 1 reply; 25+ messages in thread
From: Stefan Agner @ 2017-04-04 17:57 UTC (permalink / raw)
  To: u-boot

On 2017-04-04 02:22, Marek Vasut wrote:
> On 04/04/2017 02:02 AM, Stefan Agner wrote:
> [...]
>>>>> Admitedly, I didn't look at the patch, but if you want to boot ad-hoc
>>>>> cores, you can very well also boot secondary cores on the current CPU
>>>>> complex with the same command. Why not ?
>>>>
>>>> Sure, it could be done. I just feel it is not the right design.
>>>>
>>>> Auxiliary cores have usually a different view to memory, this is why I
>>>> had to add the get_host_mapping callback in the elf loader code to let
>>>> architecture dependent code translate to host addresses. SMP systems
>>>> don't need that.
>>>>
>>>> Also flush caches is not necessary on some cache coherent CPU's
>>>> (depending on how your cache coherence between I and D cache looks
>>>> like).
>>>
>>> So SMP is just a reduced special-case of this , yes ?
>>
>> Yeah, I guess you can get away with dummy callback implementation and a
>> performance hit due to cash flushes.
> 
> Or you can abstract things out ?
> 

There is one callback to arch for translation and one for cache flush,
what more can I abstract out?

>>>> Creating a new command like bootaux comes with very few overhead.
>>>
>>> The overhead is the new command, we already have many ad-hoc commands.
>>>
>>
>> Agreed, and I really wished that this got discussed when that command
>> initially got added. I brought it up back then...
>> https://lists.denx.de/pipermail/u-boot/2016-January/240323.html
>>
>> It seemed to be acceptable to just add this ad hoc command, with some
>> "random binary format" support back then...
> 
> Based on that discussion, I only see that noone opposed, but I don't see
> any agreement.
> 

Maybe I need to word my emails a bit stronger, but with that email I
actually tried to oppose:
https://lists.denx.de/pipermail/u-boot/2016-January/240989.html

>> Ok, it is not entirely
>> random, since it is the format of a binary how it ends up in
>> microcontrollers execute in place flash (stack pointer and reset vector
>> are the two first words)....
> 
> I thought this command was starting the core by loading data to RAM ,
> not flash ?
> 

Ok, maybe I am a bit unclear here:

bootaux currently supports a Cortex-M specific binary format only. The
binary format starts with a Cortex-M class vector table. The vector
tables first vector is the "reset vector".

In a regular microcontroller, that binary gets flashed on a NOR flash
which is mapped at 0x0 for the CPU. The CPU has no "boot ROM", the CPU
starts by calling the reset vector. So when NXP defined how the bootaux
command should look like, they just took that binary format which was
laying around and implemented a U-Boot command around it.

So this is the history of the binary format. And my point here is, that
the binary format supported by bootaux is _very_ Cortex-M class
specific.



>> However, making this ad hoc command now
>> generic really feels weird to me, since we would end up supporting that
>> format for A class CPUs etc... bootaux is really suited for auxiliary
>> M-class cores on ARM, as it is right now. Maybe we should have named it
>> bootm ;-)
> 
> We always try to avoid one-off hacks, so it's not weird. I still don't
> quite understand how it is a problem to abstract things out . I am not
> asking you to support CA, but to avoid CM specific implementation which
> cannot be extended to support CA or even MIPS/Nios2/etc .
> 

Again, why would you support a Cortex-M class specific file format for
MIPS/Nios2 etc...?

bootaux is M4 specific. We can through it away, and start over, but then
we should call the command differently.


>>>> This are the reasons why I feel creating a new command for a SMP boot
>>>> case makes more sense. We can still reuse functions which are very
>>>> similar by moving them into some common location, where it makes sense.
>>>>
>>>>>
>>>>> Also, I think this might come useful when booting stuff like "Altera
>>>>> Sparrow" ...
>>>>
>>>> I am not familiar with that architecture, what kind of core does it
>>>> provide which needs to be booted by U-Boot?
>>>
>>> The secondary ARM core in the SoCFPGA C-A9 complex or Nios2 core in the
>>> FPGA.
>>
>> In my thinking, the Nios2 core seems like such a remote processor well
>> suited for the bootaux command. For the secondary A9, I would create a
>> new command.
> 
> Uh, why, that does not make any sense to me. Both the random core in
> FPGA and the secondary CA9 core have almost the same bringup sequence.
> What is so different about this stuff ?
> 

A cache coherent core and a non-cache coherent core remote core
somewhere in the SoC is very much different IMHO.

Lets compare how you bring up a A class CPU, or cache coherent SMP style
CPU in general:

1. Load code into RAM
2. Some cache flushing might be necessary due to I/D-cache incoherency
3. Write entry point into some architecture specific register
4. kick off the CPU by writing another architecture specific register

M class/remote CPU

1. Load code in RAM, depending on image format translate core specific
memory map to host memory map
2. Flush D-cache
3. (Potentially set up IOMMU, not in the NXP case though)
4. Write entry point into some architecture specific register
5. kick off the CPU by writing another architecture specific register

From what I can see, everything is different other than "Load code in
RAM" which is image specific. If the image format is complex, we
certainly should factor that out to avoid code duplication.

>> If we want to support the two with the same command, we already have a
>> first problem: How do we address them? Of course, we could add just a
>> index or something, but this would already break backward compatibility
>> of the bootaux command.
> 
> So we already accepted a command which has shit commandline semantics
> and we're stuck with it , hrm. You can always specify a default core

Add shitty binary file format to the list.

> index for those two iMX boards and it's done. Or just fix the semantics
> in their default environment, which is not awesome, but I doubt this is
> wildly used yet.

I guess my general point is, bootaux is already broken in two ways:
Command line interface does not allow for extensibility, the only format
supported right now is a binary format which is not generic.

My patchset was an attempt to improve the situation to give it at least
a decent elf format support.

Note that there is also bootelf, where the elf headers have been
introduced. Reusing that command seems impractical since it is meant to
boot on the primary CPU. My first attempt was reusing at least the load
function, but it seemed impractical due to memory map translation and
since the load function is rather short. If it helps for the acceptance
of this patchset, I still could try to make that happen.

Making the bootaux something completely new, e.g. a generic command
under cmd/*, with CPU enumeration capabilities and (pluggable?) image
support is something I am not willing to build.

--
Stefan

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support
  2017-04-04 17:57               ` Stefan Agner
@ 2017-04-04 18:38                 ` Marek Vasut
  2017-04-04 19:45                   ` Stefan Agner
  0 siblings, 1 reply; 25+ messages in thread
From: Marek Vasut @ 2017-04-04 18:38 UTC (permalink / raw)
  To: u-boot

On 04/04/2017 07:57 PM, Stefan Agner wrote:
> On 2017-04-04 02:22, Marek Vasut wrote:
>> On 04/04/2017 02:02 AM, Stefan Agner wrote:
>> [...]
>>>>>> Admitedly, I didn't look at the patch, but if you want to boot ad-hoc
>>>>>> cores, you can very well also boot secondary cores on the current CPU
>>>>>> complex with the same command. Why not ?
>>>>>
>>>>> Sure, it could be done. I just feel it is not the right design.
>>>>>
>>>>> Auxiliary cores have usually a different view to memory, this is why I
>>>>> had to add the get_host_mapping callback in the elf loader code to let
>>>>> architecture dependent code translate to host addresses. SMP systems
>>>>> don't need that.
>>>>>
>>>>> Also flush caches is not necessary on some cache coherent CPU's
>>>>> (depending on how your cache coherence between I and D cache looks
>>>>> like).
>>>>
>>>> So SMP is just a reduced special-case of this , yes ?
>>>
>>> Yeah, I guess you can get away with dummy callback implementation and a
>>> performance hit due to cash flushes.
>>
>> Or you can abstract things out ?
>>
> 
> There is one callback to arch for translation and one for cache flush,
> what more can I abstract out?

Well then I don't think I understand your concerns about cache flushing
in SMP .

>>>>> Creating a new command like bootaux comes with very few overhead.
>>>>
>>>> The overhead is the new command, we already have many ad-hoc commands.
>>>>
>>>
>>> Agreed, and I really wished that this got discussed when that command
>>> initially got added. I brought it up back then...
>>> https://lists.denx.de/pipermail/u-boot/2016-January/240323.html
>>>
>>> It seemed to be acceptable to just add this ad hoc command, with some
>>> "random binary format" support back then...
>>
>> Based on that discussion, I only see that noone opposed, but I don't see
>> any agreement.
>>
> 
> Maybe I need to word my emails a bit stronger, but with that email I
> actually tried to oppose:
> https://lists.denx.de/pipermail/u-boot/2016-January/240989.html

Well, I do not like adding ad-hoc commands as it's not managable in the
long run.

>>> Ok, it is not entirely
>>> random, since it is the format of a binary how it ends up in
>>> microcontrollers execute in place flash (stack pointer and reset vector
>>> are the two first words)....
>>
>> I thought this command was starting the core by loading data to RAM ,
>> not flash ?
>>
> 
> Ok, maybe I am a bit unclear here:
> 
> bootaux currently supports a Cortex-M specific binary format only. The
> binary format starts with a Cortex-M class vector table. The vector
> tables first vector is the "reset vector".
> 
> In a regular microcontroller, that binary gets flashed on a NOR flash
> which is mapped at 0x0 for the CPU. The CPU has no "boot ROM", the CPU
> starts by calling the reset vector. So when NXP defined how the bootaux
> command should look like, they just took that binary format which was
> laying around and implemented a U-Boot command around it.
> 
> So this is the history of the binary format. And my point here is, that
> the binary format supported by bootaux is _very_ Cortex-M class
> specific.

Aha, so I now totally don't understand why this command cannot be
fixed/extended to support other/generic cores or SMP systems etc.
But looking at the initial proposal, I think maybe the intention of this
patchset was not to add that support, but to fix the command to
support loading ELF files ? We already have bootelf for that though ...

>>> However, making this ad hoc command now
>>> generic really feels weird to me, since we would end up supporting that
>>> format for A class CPUs etc... bootaux is really suited for auxiliary
>>> M-class cores on ARM, as it is right now. Maybe we should have named it
>>> bootm ;-)
>>
>> We always try to avoid one-off hacks, so it's not weird. I still don't
>> quite understand how it is a problem to abstract things out . I am not
>> asking you to support CA, but to avoid CM specific implementation which
>> cannot be extended to support CA or even MIPS/Nios2/etc .
>>
> 
> Again, why would you support a Cortex-M class specific file format for
> MIPS/Nios2 etc...?
> 
> bootaux is M4 specific. We can through it away, and start over, but then
> we should call the command differently.

Unless you look very carefully, it is not clear that it is cortex M4
specific at all based on this discussion. I was under the impression
that the goal here was to support all sorts of secondary cores ...

>>>>> This are the reasons why I feel creating a new command for a SMP boot
>>>>> case makes more sense. We can still reuse functions which are very
>>>>> similar by moving them into some common location, where it makes sense.
>>>>>
>>>>>>
>>>>>> Also, I think this might come useful when booting stuff like "Altera
>>>>>> Sparrow" ...
>>>>>
>>>>> I am not familiar with that architecture, what kind of core does it
>>>>> provide which needs to be booted by U-Boot?
>>>>
>>>> The secondary ARM core in the SoCFPGA C-A9 complex or Nios2 core in the
>>>> FPGA.
>>>
>>> In my thinking, the Nios2 core seems like such a remote processor well
>>> suited for the bootaux command. For the secondary A9, I would create a
>>> new command.
>>
>> Uh, why, that does not make any sense to me. Both the random core in
>> FPGA and the secondary CA9 core have almost the same bringup sequence.
>> What is so different about this stuff ?
>>
> 
> A cache coherent core and a non-cache coherent core remote core
> somewhere in the SoC is very much different IMHO.
> 
> Lets compare how you bring up a A class CPU, or cache coherent SMP style
> CPU in general:
> 
> 1. Load code into RAM
> 2. Some cache flushing might be necessary due to I/D-cache incoherency
> 3. Write entry point into some architecture specific register
> 4. kick off the CPU by writing another architecture specific register
> 
> M class/remote CPU
> 
> 1. Load code in RAM, depending on image format translate core specific
> memory map to host memory map
> 2. Flush D-cache
> 3. (Potentially set up IOMMU, not in the NXP case though)
> 4. Write entry point into some architecture specific register
> 5. kick off the CPU by writing another architecture specific register
> 
> From what I can see, everything is different other than "Load code in
> RAM" which is image specific. If the image format is complex, we
> certainly should factor that out to avoid code duplication.

Image format is usually binary blob or elf file, we support both.
The rest is "load file to RAM, do magic setup (can be CPU specific),
unreset CPU".

>>> If we want to support the two with the same command, we already have a
>>> first problem: How do we address them? Of course, we could add just a
>>> index or something, but this would already break backward compatibility
>>> of the bootaux command.
>>
>> So we already accepted a command which has shit commandline semantics
>> and we're stuck with it , hrm. You can always specify a default core
> 
> Add shitty binary file format to the list.

Why do we care about this ? The CM just needs a vectoring table, so set
it up (in the elf file or whatever) and point the core at the reset vector.

>> index for those two iMX boards and it's done. Or just fix the semantics
>> in their default environment, which is not awesome, but I doubt this is
>> wildly used yet.
> 
> I guess my general point is, bootaux is already broken in two ways:
> Command line interface does not allow for extensibility, the only format
> supported right now is a binary format which is not generic.
> 
> My patchset was an attempt to improve the situation to give it at least
> a decent elf format support.

Maybe this information was lost ... Lukasz ?!

> Note that there is also bootelf, where the elf headers have been
> introduced. Reusing that command seems impractical since it is meant to
> boot on the primary CPU. My first attempt was reusing at least the load
> function, but it seemed impractical due to memory map translation and
> since the load function is rather short. If it helps for the acceptance
> of this patchset, I still could try to make that happen.

Extending bootelf might indeed make more sense IMO. But that needs
discussion.

> Making the bootaux something completely new, e.g. a generic command
> under cmd/*, with CPU enumeration capabilities and (pluggable?) image
> support is something I am not willing to build.
> 
> --
> Stefan
> 


-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support
  2017-04-04  8:23     ` Lukasz Majewski
@ 2017-04-04 18:59       ` Stefan Agner
  2017-04-05 15:15         ` Lukasz Majewski
  0 siblings, 1 reply; 25+ messages in thread
From: Stefan Agner @ 2017-04-04 18:59 UTC (permalink / raw)
  To: u-boot

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="windows-1254", Size: 11032 bytes --]

On 2017-04-04 01:23, Lukasz Majewski wrote:
> Hi Stefan,
> 
>> Hi Lukasz,
>>
>> On 2017-04-03 04:20, Lukasz Majewski wrote:
>> > Hi Stefan,
>> >
>> > Thanks for your patch. Please allow me to share some ideas for
>> > improvements.
>> >
>> >> From: Stefan Agner <stefan.agner@toradex.com>
>> >>
>> >> This patchset enables to boot elf binaries on secondary Cortex-M
>> >> class cores available on i.MX 6SoloX/7Solo/7Dual. This makes
>> >> handling and loading firmwares much more convinient since all
>> >> information where the firmware has to be loaded to is contained in
>> >> the elf headers. A typical usage looks like this:
>> >>
>> >>   Colibri iMX7 # tftp ${loadaddr} firmware.elf && bootaux
>> >> ${loadaddr} Using FEC0 device
>> >>   TFTP from server 192.168.10.1; our IP address is 192.168.10.2
>> >>   Filename 'firmware.elf'.
>> >>   Load address: 0x80800000
>> >>   Loading: ##################################################  88.3
>> >> KiB 5.4 MiB/s
>> >>   done
>> >>   Bytes transferred = 90424 (16138 hex)
>> >>   ## Starting auxiliary core at 0x1FFF8311 ...
>> >>   Colibri iMX7 #
>> >
>> > I can find some other platforms (not only IMX), which would benefit
>> > from this code - the generic 'bootaux' command.
>> >
>> > One good example would to allow multiple binaries for different SoC
>> > Cores (e.g. 2x Cortex-A8) to be loaded and started by u-boot.
>> >
>> > Hence, I'm wondering if you could make those patches usable for
>> > other platforms as well?
>>
>> I don't think that this is a good idea. bootaux is meant for auxiliary
>> cores, which often use a different architecture and are not cache
>> coherent (hence the cache flushes).
> 
> I do remember, that I saw similar "tiny" patch to add "just one" ad-hoc
> command to do the same (conceptually) for TI SoC floating on the u-boot
> mailing list.
> 
> Please correct me if I'm wrong, but bootaux is IMX specific and does
> work, which very likely, will be also required by other SoC vendors
> (TI's Sitara is also equipped with Cortex-M4 and PRUSS).

bootaux is currently IMX specific, and its currently supported binary
format is M-class specific.

> 
> Unification of such effort can save us all a lot of trouble in a very
> near future.

In OSS, you do not develop for the future. It gets developed when its
here.

> 
>>
>> On SMP systems the main operating system normally starts the secondary
>> core.
> 
> I think that there are some conceptual similarities between loading code
> to SMP core and Cortex M3. Of course some "tweaks" would be needed.
>

There are conceptual similarities between a car and a truck, still, they
are likely manufactured in a different assembly line, probably by a
different producer.

I guess in the end it really depends on where you draw the line: There
are differences between loading code which will get executed by the
primary code, loading code to execute explicitly on a SMP core, and
loading code to execute on a remote processor.

The first case is already well supported, and we need to keep support
backward compatibility.

The second case, IMHO, is a somewhat silly case: A SMP system usually
gets driven by a single OS image, and that OS makes sure to initialize
the cores (maybe with the help of firmware, such as the PSCI interface
on ARM). There is no need for a boot loader command to execute a image
on a secondary core explicitly...

The third case is probably a case where we could start think about
unification efforts.


>> Otherwise, if you want to run them separately using U-Boot,
>> maybe a new command such as bootsmp would be more suited.
> 
> Hmm - I do think that it would be too much:
> 
> - bootm for generic single core
> - bootaux for IMX
> - bootsmp for SMP (on IMX/TI/RK?)
> - boot?? for ??

There is at least also bootz and bootelf.

> 
> I would like to avoid adding new commands for doing conceptually the
> same work.
> 
> Even better, we could extend bootm to support the "multi binary" case
> too, but IMHO it would be also correct to have "bootaux" to handle
> generic binaries loading.
> 
>>
>> >
>> >>
>> >> Note that the bootaux command retrieved the entry point (PC) from
>> >> the elf binary.
>> >
>> > Could you make this code flexible enough to handle not only elf
>> > binaries, but also other data (e.g. FPGA bitstreams)?
>>
>> The code of bootaux is rather small, I don't see much value into stuff
>> boot code for other peripherals into it.
> 
> But I'm not asking to write support for other vendor's SoC/use cases.
> 
> I'm just wondering if you could write generic command (framework) to
> support this use case and as an example add support for your IMX's
> Cortex-M3/4.
> 

Sure, mv arch/arm/imx-common/imx_bootaux.c cmd/, there is the framework
:-)

But this promotes the M class specific binary format to a generic format
supported for all cores.

> We would kill two birds with one stone - IMX is supported from the very
> beginning and we do have generic code to extend it by other vendors.
> 
>> I don't know how FPGA
>> bistream loading typically works, but I guess it is quite different
>> from loading a firmware into RAM/SRAM and flush caches...
> 
> FPGA quirks would be handled in arch/SoC specific code.
> 
>>
>> I am not against reuse and unification, I just feel that this is not
>> really a case where we gain much.
> 
> With generic "bootaux/bootm" command we can point other developers who
> would like to add such booting code to the already available framework.
> 
> Also, we would prevent other "ad-hoc" commands adding to u-boot.
> 

Extending bootm does not seem like a good idea. bootm is already rather
complex, making it even more complex by supporting the auxiliary core
case seems really not work well.

Also, bootm supports lots of features which you probably never would use
or test on a remote core (e.g. initramfs etc...)

>>
>> >
>> > Maybe it would better to:
>> > -------------------------
>> >
>> > Embrace those binaries into FIT file (*.itb)? And allow multiple
>> > binaries loading? I'm thinking of work similar to one already did by
>> > Andre Przywara for SPL:
>> >
>> > "[PATCH v3 00/19] SPL: extend FIT loading support" posted on
>> > 31.03.2017?
>> >
>> > In that way we would "open" many new use cases, and other platforms
>> > (e.g. FPGA's, TI, etc) could benefit from it.
>> > One solid use case is to load different Linux binaries (or/and bare
>> > metal programs) to different SoC cores.
>> >
>> > The "meta data" (i.e. load address, data type, description), could
>> > be extracted from the FIT format (the code is already in u-boot).
>> >
>> > IMHO, this is very generic approach.
>>
>> I did some experiments with using FIT images for auxiliary core
>> firmware, however, it did not add a lot of advantage over using elf:
>> http://git.toradex.com/cgit/u-boot-toradex.git/commit/?h=2015.04-toradex-next&id=d1d416f272e840e8139aec911f89a70fe5523eb2
> 
> Unfortunately, not all use cases allow using ELF format. FIT gives more
> flexibility:
> 
> -  ./doc/README.dfutftp -> different images loading
> 
> -  Andre's patch to load multiple binaries in SPL - [PATCH v3 00/19]
>   SPL: extend FIT loading support"
> 

Are flexible, but very much U-Boot specific. And much more complex to
support.

>>
>> Firmwares are already built and available in the elf file format. The
>> Linux remoteproc framework, which is meant to handle this kind of
>> cores too, supports elf firmware loading too, so supporting elf in
>> U-Boot too is a nice alignment. Also using FIT adds an additional
>> step when building firm wares...
> 
> But this is all OK.
> 
> The ELF binary would be wrapped in FIT (with e.g. "elf" property, even
> 1 to 1 mapping). Then the 'bootaux/bootm' could parse ELF (which is also
> generic). And then some "IMX specific" (arch/SoC) code would be
> executed.
> 

So we go from a nacked binary loaded directly to the place it has been
linked to, to a double wrapped firmware file...?

Not sure if user will appriciate that extra work and boot time. And
developers the extra code.

Maybe in a perfect world that just works, because you know, FIT is
generic, and elf is generic... But that is just not how it works in
practice. The existing FIT code is rather complex, supports lots of
corner cases. The elf code supports different header types... Loading
the elf sections (which use M4 specific addressing) needs address
translation to get to suitable host addresses...

> 
>>
>>
>> >
>> >> Also all sections are translated to A7 addresses
>> >> in order to properly load the firmware sections into the
>> >> appropriate locations.
>> >
>> > This would require some tiny arch specific code.
>> >
>>
>> Yes, but in my rather small patchset (164 additional lines) this is
>> actually almost the bulk of changes :-)
> 
> Yes, I fully understand you :-) - I do remember when I wanted to add 3
> lines (polarity inversion) to IMX PWM subsystem some time ago ... :-) 
> 

Hehe, yeah that was a rather long episode.

It was a bit different, though, the core framework was already there, it
was mostly about adopting a particular driver to that framework.
Building a new framework is a bit more involved...

--
Stefan

> Best regards,
> Łukasz Majewski
> 
>>
>> --
>> Stefan
>>
>> >> Also cache flushes is taken care of, so that there is no
>> >> manual dcache flush necessary anymore.
>> >>
>> >> The NXP FreeRTOS BSP already generates elf binaries which can be
>> >> directly used with this elf binary support.
>> >>
>> >> The last patch adds bootaux support for Vybrid.
>> >>
>> >>
>> >> Stefan Agner (3):
>> >>   imx: imx-common: move aux core image parsing to common code
>> >>   imx: imx-common: add elf firmware support
>> >>   ARM: vf610: add auxiliary core boot support
>> >>
>> >>  arch/arm/cpu/armv7/mx6/soc.c                |  30 +++++---
>> >>  arch/arm/cpu/armv7/mx7/soc.c                |  34 ++++++---
>> >>  arch/arm/cpu/armv7/vf610/generic.c          |  42 +++++++++++
>> >>  arch/arm/imx-common/Kconfig                 |   2 +-
>> >>  arch/arm/imx-common/Makefile                |   4 +-
>> >>  arch/arm/imx-common/imx_bootaux.c           | 105
>> >> +++++++++++++++++++++++-----
>> >> arch/arm/include/asm/arch-vf610/sys_proto.h |   8 +++
>> >> arch/arm/include/asm/imx-common/sys_proto.h |   6 ++
>> >> configs/colibri_vf_defconfig                |   1 + 9 files
>> >> changed, 198 insertions(+), 34 deletions(-) create mode 100644
>> >> arch/arm/include/asm/arch-vf610/sys_proto.h
>> >>
>> >
>> >
>> >
>> >
>> > Best regards,
>> >
>> > Lukasz Majewski
>> >
>> > --
>> >
>> > DENX Software Engineering GmbH,      Managing Director: Wolfgang
>> > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
>> > Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email:
>> > wd at denx.de
> 
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support
  2017-04-04 18:38                 ` Marek Vasut
@ 2017-04-04 19:45                   ` Stefan Agner
  2017-04-04 20:17                     ` Marek Vasut
  0 siblings, 1 reply; 25+ messages in thread
From: Stefan Agner @ 2017-04-04 19:45 UTC (permalink / raw)
  To: u-boot

On 2017-04-04 11:38, Marek Vasut wrote:
> On 04/04/2017 07:57 PM, Stefan Agner wrote:
>> On 2017-04-04 02:22, Marek Vasut wrote:
>>> On 04/04/2017 02:02 AM, Stefan Agner wrote:
>>> [...]
>>>>>>> Admitedly, I didn't look at the patch, but if you want to boot ad-hoc
>>>>>>> cores, you can very well also boot secondary cores on the current CPU
>>>>>>> complex with the same command. Why not ?
>>>>>>
>>>>>> Sure, it could be done. I just feel it is not the right design.
>>>>>>
>>>>>> Auxiliary cores have usually a different view to memory, this is why I
>>>>>> had to add the get_host_mapping callback in the elf loader code to let
>>>>>> architecture dependent code translate to host addresses. SMP systems
>>>>>> don't need that.
>>>>>>
>>>>>> Also flush caches is not necessary on some cache coherent CPU's
>>>>>> (depending on how your cache coherence between I and D cache looks
>>>>>> like).
>>>>>
>>>>> So SMP is just a reduced special-case of this , yes ?
>>>>
>>>> Yeah, I guess you can get away with dummy callback implementation and a
>>>> performance hit due to cash flushes.
>>>
>>> Or you can abstract things out ?
>>>
>>
>> There is one callback to arch for translation and one for cache flush,
>> what more can I abstract out?
> 
> Well then I don't think I understand your concerns about cache flushing
> in SMP .
> 

It makes things unnecessary slower.

>>>>>> Creating a new command like bootaux comes with very few overhead.
>>>>>
>>>>> The overhead is the new command, we already have many ad-hoc commands.
>>>>>
>>>>
>>>> Agreed, and I really wished that this got discussed when that command
>>>> initially got added. I brought it up back then...
>>>> https://lists.denx.de/pipermail/u-boot/2016-January/240323.html
>>>>
>>>> It seemed to be acceptable to just add this ad hoc command, with some
>>>> "random binary format" support back then...
>>>
>>> Based on that discussion, I only see that noone opposed, but I don't see
>>> any agreement.
>>>
>>
>> Maybe I need to word my emails a bit stronger, but with that email I
>> actually tried to oppose:
>> https://lists.denx.de/pipermail/u-boot/2016-January/240989.html
> 
> Well, I do not like adding ad-hoc commands as it's not managable in the
> long run.
> 

I argue that a remote core loading command is _much_ more manageable
than making the bootm command even more complex by support
SMP/remoteproc and whatnot usecases... I would even argue that a bunch
of those commands are more manageable than a single ifdef/if hell....

That said, I still would push for keeping the image processing code
generic, whenever it makes sense.

>>>> Ok, it is not entirely
>>>> random, since it is the format of a binary how it ends up in
>>>> microcontrollers execute in place flash (stack pointer and reset vector
>>>> are the two first words)....
>>>
>>> I thought this command was starting the core by loading data to RAM ,
>>> not flash ?
>>>
>>
>> Ok, maybe I am a bit unclear here:
>>
>> bootaux currently supports a Cortex-M specific binary format only. The
>> binary format starts with a Cortex-M class vector table. The vector
>> tables first vector is the "reset vector".
>>
>> In a regular microcontroller, that binary gets flashed on a NOR flash
>> which is mapped at 0x0 for the CPU. The CPU has no "boot ROM", the CPU
>> starts by calling the reset vector. So when NXP defined how the bootaux
>> command should look like, they just took that binary format which was
>> laying around and implemented a U-Boot command around it.
>>
>> So this is the history of the binary format. And my point here is, that
>> the binary format supported by bootaux is _very_ Cortex-M class
>> specific.
> 
> Aha, so I now totally don't understand why this command cannot be
> fixed/extended to support other/generic cores or SMP systems etc.
> But looking at the initial proposal, I think maybe the intention of this
> patchset was not to add that support, but to fix the command to
> support loading ELF files ? We already have bootelf for that though ...
> 

Yes, that is pretty much it. I would like to teach that command a more
generic format, which would be at least a step towards something more
generic/standardized.

bootelf is really meant for the primary CPU. That would be an entirely
different direction: Make all common boot commands "aux core" capable.

But I would strongly vote against that. First, those commands have
already complex arguments and argument handling (e.g. bootm), and their
implementation supports use cases which we hardly would ever use on aux
cores (initramfs..).

>>>> However, making this ad hoc command now
>>>> generic really feels weird to me, since we would end up supporting that
>>>> format for A class CPUs etc... bootaux is really suited for auxiliary
>>>> M-class cores on ARM, as it is right now. Maybe we should have named it
>>>> bootm ;-)
>>>
>>> We always try to avoid one-off hacks, so it's not weird. I still don't
>>> quite understand how it is a problem to abstract things out . I am not
>>> asking you to support CA, but to avoid CM specific implementation which
>>> cannot be extended to support CA or even MIPS/Nios2/etc .
>>>
>>
>> Again, why would you support a Cortex-M class specific file format for
>> MIPS/Nios2 etc...?
>>
>> bootaux is M4 specific. We can through it away, and start over, but then
>> we should call the command differently.
> 
> Unless you look very carefully, it is not clear that it is cortex M4
> specific at all based on this discussion. I was under the impression
> that the goal here was to support all sorts of secondary cores ...
> 

Yeah it's also only that binary format handling which is M4 specific.
And frankly, that is just 2 lines of code, basically dereferencing the
first two words:

+    if (valid_elf_image(addr)) {
+        stack = 0x0;
+        pc = load_elf_image_phdr(addr);
+        if (!pc)
+            return CMD_RET_FAILURE;
+
+    } else {
+        /*
+         * Assume binary file with vector table at the beginning.
+         * Cortex-M4 vector tables start with the stack pointer (SP)
+         * and reset vector (initial PC).
+         */
+        stack = *(u32 *)addr;
+        pc = *(u32 *)(addr + 4);
+    }

>>>>>> This are the reasons why I feel creating a new command for a SMP boot
>>>>>> case makes more sense. We can still reuse functions which are very
>>>>>> similar by moving them into some common location, where it makes sense.
>>>>>>
>>>>>>>
>>>>>>> Also, I think this might come useful when booting stuff like "Altera
>>>>>>> Sparrow" ...
>>>>>>
>>>>>> I am not familiar with that architecture, what kind of core does it
>>>>>> provide which needs to be booted by U-Boot?
>>>>>
>>>>> The secondary ARM core in the SoCFPGA C-A9 complex or Nios2 core in the
>>>>> FPGA.
>>>>
>>>> In my thinking, the Nios2 core seems like such a remote processor well
>>>> suited for the bootaux command. For the secondary A9, I would create a
>>>> new command.
>>>
>>> Uh, why, that does not make any sense to me. Both the random core in
>>> FPGA and the secondary CA9 core have almost the same bringup sequence.
>>> What is so different about this stuff ?
>>>
>>
>> A cache coherent core and a non-cache coherent core remote core
>> somewhere in the SoC is very much different IMHO.
>>
>> Lets compare how you bring up a A class CPU, or cache coherent SMP style
>> CPU in general:
>>
>> 1. Load code into RAM
>> 2. Some cache flushing might be necessary due to I/D-cache incoherency
>> 3. Write entry point into some architecture specific register
>> 4. kick off the CPU by writing another architecture specific register
>>
>> M class/remote CPU
>>
>> 1. Load code in RAM, depending on image format translate core specific
>> memory map to host memory map
>> 2. Flush D-cache
>> 3. (Potentially set up IOMMU, not in the NXP case though)
>> 4. Write entry point into some architecture specific register
>> 5. kick off the CPU by writing another architecture specific register
>>
>> From what I can see, everything is different other than "Load code in
>> RAM" which is image specific. If the image format is complex, we
>> certainly should factor that out to avoid code duplication.
> 
> Image format is usually binary blob or elf file, we support both.
> The rest is "load file to RAM, do magic setup (can be CPU specific),
> unreset CPU".
> 

Loading files to RAM is already generic since it is handled by separate
commands.

It is really the image handling which has the potential of reusability.

>>>> If we want to support the two with the same command, we already have a
>>>> first problem: How do we address them? Of course, we could add just a
>>>> index or something, but this would already break backward compatibility
>>>> of the bootaux command.
>>>
>>> So we already accepted a command which has shit commandline semantics
>>> and we're stuck with it , hrm. You can always specify a default core
>>
>> Add shitty binary file format to the list.
> 
> Why do we care about this ? The CM just needs a vectoring table, so set
> it up (in the elf file or whatever) and point the core at the reset vector.
> 

Not sure what you want to say here, but I guess we need to keep support
for this file format for backward compatibility?

>>> index for those two iMX boards and it's done. Or just fix the semantics
>>> in their default environment, which is not awesome, but I doubt this is
>>> wildly used yet.
>>
>> I guess my general point is, bootaux is already broken in two ways:
>> Command line interface does not allow for extensibility, the only format
>> supported right now is a binary format which is not generic.
>>
>> My patchset was an attempt to improve the situation to give it at least
>> a decent elf format support.
> 
> Maybe this information was lost ... Lukasz ?!
> 
>> Note that there is also bootelf, where the elf headers have been
>> introduced. Reusing that command seems impractical since it is meant to
>> boot on the primary CPU. My first attempt was reusing at least the load
>> function, but it seemed impractical due to memory map translation and
>> since the load function is rather short. If it helps for the acceptance
>> of this patchset, I still could try to make that happen.
> 
> Extending bootelf might indeed make more sense IMO. But that needs
> discussion.
> 

There are two variants of reuse here:
1. Reuse the elf parsing and loading function (function
load_elf_image_phdr in this case).
2. Reuse the bootelf command. This would need extending the command with
some parameter to tell it to not boot the primary CPU but boot some
other auxiliary processor.


Let's get a bit more concrete here, take bootelf as a case study:

1 would introduce the architecture specific translation callback
load_elf_image_phdr (see my load_elf_image_phdr implementation in this
patchset). For regular/primary boot we can implement a dummy 1:1
translation.



2 would need 1, and changes to do_bootelf. Since the command has already
optional arguments adding core selection as an optional argument is not
possible. But we could add a parameter, e.g. -c (core):

bootelf -c aux

or

bootelf -c 1

The exact enumeration would have to be discussed. I guess this would
also be architecture specific.

Adding the new parameter handling plus callbacks into architecture
dependent code probably makes do_bootelf double in length. Also, half of
the existing code cannot be reused: The call do_bootelf_exec is primary
CPU specific, passing the arguments (this is likely not supported by
remote cores) and the return code handling (since a remote core would
not return), autostart handling, not sure?

2 would also need to either disable the section header functionality for
the remote core, or we add another if in do_bootelf and avoid parsing
section header even if requested...


Again, I strongly disfavor solution 2. That makes boot commands with
image types we want to support even more complex. bootelf already starts
to look ugly when I start implementing that, and bootelf is really one
of the cleanest and simplest boot commands I have seen... I probably
would give it a shot if this is the only way to bring elf format support
for aux cores upstream... Good luck for the guy implementing FIT image
support (bootm) for auxiliary cores :-D

1 we could do, and I would be willing to do it. I personally feel that
the small code duplication is cleaner than reusing that function from
cmd/elf.c and implementing the dummy 1:1 translation, but both works for
me....

IMHO, commands are really lightweight and nice in U-Boot, I think we
should make use of them, even when it means some namespace pollution.

--
Stefan

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support
  2017-04-04 19:45                   ` Stefan Agner
@ 2017-04-04 20:17                     ` Marek Vasut
  2017-04-04 21:39                       ` Stefan Agner
  0 siblings, 1 reply; 25+ messages in thread
From: Marek Vasut @ 2017-04-04 20:17 UTC (permalink / raw)
  To: u-boot

On 04/04/2017 09:45 PM, Stefan Agner wrote:
> On 2017-04-04 11:38, Marek Vasut wrote:
>> On 04/04/2017 07:57 PM, Stefan Agner wrote:
>>> On 2017-04-04 02:22, Marek Vasut wrote:
>>>> On 04/04/2017 02:02 AM, Stefan Agner wrote:
>>>> [...]
>>>>>>>> Admitedly, I didn't look at the patch, but if you want to boot ad-hoc
>>>>>>>> cores, you can very well also boot secondary cores on the current CPU
>>>>>>>> complex with the same command. Why not ?
>>>>>>>
>>>>>>> Sure, it could be done. I just feel it is not the right design.
>>>>>>>
>>>>>>> Auxiliary cores have usually a different view to memory, this is why I
>>>>>>> had to add the get_host_mapping callback in the elf loader code to let
>>>>>>> architecture dependent code translate to host addresses. SMP systems
>>>>>>> don't need that.
>>>>>>>
>>>>>>> Also flush caches is not necessary on some cache coherent CPU's
>>>>>>> (depending on how your cache coherence between I and D cache looks
>>>>>>> like).
>>>>>>
>>>>>> So SMP is just a reduced special-case of this , yes ?
>>>>>
>>>>> Yeah, I guess you can get away with dummy callback implementation and a
>>>>> performance hit due to cash flushes.
>>>>
>>>> Or you can abstract things out ?
>>>>
>>>
>>> There is one callback to arch for translation and one for cache flush,
>>> what more can I abstract out?
>>
>> Well then I don't think I understand your concerns about cache flushing
>> in SMP .
>>
> 
> It makes things unnecessary slower.

You always have to flush cache if you expect the peripheral to read
cached memory , so I don't think I understand your remark ...

>>>>>>> Creating a new command like bootaux comes with very few overhead.
>>>>>>
>>>>>> The overhead is the new command, we already have many ad-hoc commands.
>>>>>>
>>>>>
>>>>> Agreed, and I really wished that this got discussed when that command
>>>>> initially got added. I brought it up back then...
>>>>> https://lists.denx.de/pipermail/u-boot/2016-January/240323.html
>>>>>
>>>>> It seemed to be acceptable to just add this ad hoc command, with some
>>>>> "random binary format" support back then...
>>>>
>>>> Based on that discussion, I only see that noone opposed, but I don't see
>>>> any agreement.
>>>>
>>>
>>> Maybe I need to word my emails a bit stronger, but with that email I
>>> actually tried to oppose:
>>> https://lists.denx.de/pipermail/u-boot/2016-January/240989.html
>>
>> Well, I do not like adding ad-hoc commands as it's not managable in the
>> long run.
>>
> 
> I argue that a remote core loading command is _much_ more manageable
> than making the bootm command even more complex by support
> SMP/remoteproc and whatnot usecases... I would even argue that a bunch
> of those commands are more manageable than a single ifdef/if hell....

I guess a boot* option to start remote core would do ?

> That said, I still would push for keeping the image processing code
> generic, whenever it makes sense.

Agreed

>>>>> Ok, it is not entirely
>>>>> random, since it is the format of a binary how it ends up in
>>>>> microcontrollers execute in place flash (stack pointer and reset vector
>>>>> are the two first words)....
>>>>
>>>> I thought this command was starting the core by loading data to RAM ,
>>>> not flash ?
>>>>
>>>
>>> Ok, maybe I am a bit unclear here:
>>>
>>> bootaux currently supports a Cortex-M specific binary format only. The
>>> binary format starts with a Cortex-M class vector table. The vector
>>> tables first vector is the "reset vector".
>>>
>>> In a regular microcontroller, that binary gets flashed on a NOR flash
>>> which is mapped at 0x0 for the CPU. The CPU has no "boot ROM", the CPU
>>> starts by calling the reset vector. So when NXP defined how the bootaux
>>> command should look like, they just took that binary format which was
>>> laying around and implemented a U-Boot command around it.
>>>
>>> So this is the history of the binary format. And my point here is, that
>>> the binary format supported by bootaux is _very_ Cortex-M class
>>> specific.
>>
>> Aha, so I now totally don't understand why this command cannot be
>> fixed/extended to support other/generic cores or SMP systems etc.
>> But looking at the initial proposal, I think maybe the intention of this
>> patchset was not to add that support, but to fix the command to
>> support loading ELF files ? We already have bootelf for that though ...
>>
> 
> Yes, that is pretty much it. I would like to teach that command a more
> generic format, which would be at least a step towards something more
> generic/standardized.
> 
> bootelf is really meant for the primary CPU. That would be an entirely
> different direction: Make all common boot commands "aux core" capable.
> 
> But I would strongly vote against that. First, those commands have
> already complex arguments and argument handling (e.g. bootm), and their
> implementation supports use cases which we hardly would ever use on aux
> cores (initramfs..).

That might need some further thinking/consolidation . IMO out of scope
of this discussion ...

>>>>> However, making this ad hoc command now
>>>>> generic really feels weird to me, since we would end up supporting that
>>>>> format for A class CPUs etc... bootaux is really suited for auxiliary
>>>>> M-class cores on ARM, as it is right now. Maybe we should have named it
>>>>> bootm ;-)
>>>>
>>>> We always try to avoid one-off hacks, so it's not weird. I still don't
>>>> quite understand how it is a problem to abstract things out . I am not
>>>> asking you to support CA, but to avoid CM specific implementation which
>>>> cannot be extended to support CA or even MIPS/Nios2/etc .
>>>>
>>>
>>> Again, why would you support a Cortex-M class specific file format for
>>> MIPS/Nios2 etc...?
>>>
>>> bootaux is M4 specific. We can through it away, and start over, but then
>>> we should call the command differently.
>>
>> Unless you look very carefully, it is not clear that it is cortex M4
>> specific at all based on this discussion. I was under the impression
>> that the goal here was to support all sorts of secondary cores ...
>>
> 
> Yeah it's also only that binary format handling which is M4 specific.
> And frankly, that is just 2 lines of code, basically dereferencing the
> first two words:
> 
> +    if (valid_elf_image(addr)) {
> +        stack = 0x0;
> +        pc = load_elf_image_phdr(addr);
> +        if (!pc)
> +            return CMD_RET_FAILURE;
> +
> +    } else {
> +        /*
> +         * Assume binary file with vector table at the beginning.
> +         * Cortex-M4 vector tables start with the stack pointer (SP)
> +         * and reset vector (initial PC).
> +         */
> +        stack = *(u32 *)addr;
> +        pc = *(u32 *)(addr + 4);
> +    }
> 
>>>>>>> This are the reasons why I feel creating a new command for a SMP boot
>>>>>>> case makes more sense. We can still reuse functions which are very
>>>>>>> similar by moving them into some common location, where it makes sense.
>>>>>>>
>>>>>>>>
>>>>>>>> Also, I think this might come useful when booting stuff like "Altera
>>>>>>>> Sparrow" ...
>>>>>>>
>>>>>>> I am not familiar with that architecture, what kind of core does it
>>>>>>> provide which needs to be booted by U-Boot?
>>>>>>
>>>>>> The secondary ARM core in the SoCFPGA C-A9 complex or Nios2 core in the
>>>>>> FPGA.
>>>>>
>>>>> In my thinking, the Nios2 core seems like such a remote processor well
>>>>> suited for the bootaux command. For the secondary A9, I would create a
>>>>> new command.
>>>>
>>>> Uh, why, that does not make any sense to me. Both the random core in
>>>> FPGA and the secondary CA9 core have almost the same bringup sequence.
>>>> What is so different about this stuff ?
>>>>
>>>
>>> A cache coherent core and a non-cache coherent core remote core
>>> somewhere in the SoC is very much different IMHO.
>>>
>>> Lets compare how you bring up a A class CPU, or cache coherent SMP style
>>> CPU in general:
>>>
>>> 1. Load code into RAM
>>> 2. Some cache flushing might be necessary due to I/D-cache incoherency
>>> 3. Write entry point into some architecture specific register
>>> 4. kick off the CPU by writing another architecture specific register
>>>
>>> M class/remote CPU
>>>
>>> 1. Load code in RAM, depending on image format translate core specific
>>> memory map to host memory map
>>> 2. Flush D-cache
>>> 3. (Potentially set up IOMMU, not in the NXP case though)
>>> 4. Write entry point into some architecture specific register
>>> 5. kick off the CPU by writing another architecture specific register
>>>
>>> From what I can see, everything is different other than "Load code in
>>> RAM" which is image specific. If the image format is complex, we
>>> certainly should factor that out to avoid code duplication.
>>
>> Image format is usually binary blob or elf file, we support both.
>> The rest is "load file to RAM, do magic setup (can be CPU specific),
>> unreset CPU".
>>
> 
> Loading files to RAM is already generic since it is handled by separate
> commands.
> 
> It is really the image handling which has the potential of reusability.

OK

>>>>> If we want to support the two with the same command, we already have a
>>>>> first problem: How do we address them? Of course, we could add just a
>>>>> index or something, but this would already break backward compatibility
>>>>> of the bootaux command.
>>>>
>>>> So we already accepted a command which has shit commandline semantics
>>>> and we're stuck with it , hrm. You can always specify a default core
>>>
>>> Add shitty binary file format to the list.
>>
>> Why do we care about this ? The CM just needs a vectoring table, so set
>> it up (in the elf file or whatever) and point the core at the reset vector.
>>
> 
> Not sure what you want to say here, but I guess we need to keep support
> for this file format for backward compatibility?

I guess so, I didn't look into the requirements for the CM4 core on MX6.

>>>> index for those two iMX boards and it's done. Or just fix the semantics
>>>> in their default environment, which is not awesome, but I doubt this is
>>>> wildly used yet.
>>>
>>> I guess my general point is, bootaux is already broken in two ways:
>>> Command line interface does not allow for extensibility, the only format
>>> supported right now is a binary format which is not generic.
>>>
>>> My patchset was an attempt to improve the situation to give it at least
>>> a decent elf format support.
>>
>> Maybe this information was lost ... Lukasz ?!
>>
>>> Note that there is also bootelf, where the elf headers have been
>>> introduced. Reusing that command seems impractical since it is meant to
>>> boot on the primary CPU. My first attempt was reusing at least the load
>>> function, but it seemed impractical due to memory map translation and
>>> since the load function is rather short. If it helps for the acceptance
>>> of this patchset, I still could try to make that happen.
>>
>> Extending bootelf might indeed make more sense IMO. But that needs
>> discussion.
>>
> 
> There are two variants of reuse here:
> 1. Reuse the elf parsing and loading function (function
> load_elf_image_phdr in this case).
> 2. Reuse the bootelf command. This would need extending the command with
> some parameter to tell it to not boot the primary CPU but boot some
> other auxiliary processor.
> 
> 
> Let's get a bit more concrete here, take bootelf as a case study:
> 
> 1 would introduce the architecture specific translation callback
> load_elf_image_phdr (see my load_elf_image_phdr implementation in this
> patchset). For regular/primary boot we can implement a dummy 1:1
> translation.
> 
> 
> 
> 2 would need 1, and changes to do_bootelf. Since the command has already
> optional arguments adding core selection as an optional argument is not
> possible. But we could add a parameter, e.g. -c (core):
> 
> bootelf -c aux
> 
> or
> 
> bootelf -c 1

Works for me ...

> The exact enumeration would have to be discussed. I guess this would
> also be architecture specific.
> 
> Adding the new parameter handling plus callbacks into architecture
> dependent code probably makes do_bootelf double in length. Also, half of
> the existing code cannot be reused: The call do_bootelf_exec is primary
> CPU specific, passing the arguments (this is likely not supported by
> remote cores) and the return code handling (since a remote core would
> not return), autostart handling, not sure?
> 
> 2 would also need to either disable the section header functionality for
> the remote core, or we add another if in do_bootelf and avoid parsing
> section header even if requested...
> 
> 
> Again, I strongly disfavor solution 2. That makes boot commands with
> image types we want to support even more complex. bootelf already starts
> to look ugly when I start implementing that, and bootelf is really one
> of the cleanest and simplest boot commands I have seen... I probably
> would give it a shot if this is the only way to bring elf format support
> for aux cores upstream... Good luck for the guy implementing FIT image
> support (bootm) for auxiliary cores :-D

So what is the problem with this ? Load the payload somewhere and ungate
the secondary core , why is that so hard ?

> 1 we could do, and I would be willing to do it. I personally feel that
> the small code duplication is cleaner than reusing that function from
> cmd/elf.c and implementing the dummy 1:1 translation, but both works for
> me....
> 
> IMHO, commands are really lightweight and nice in U-Boot, I think we
> should make use of them, even when it means some namespace pollution.
> 
> --
> Stefan
> 
> 


-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support
  2017-04-04 20:17                     ` Marek Vasut
@ 2017-04-04 21:39                       ` Stefan Agner
  0 siblings, 0 replies; 25+ messages in thread
From: Stefan Agner @ 2017-04-04 21:39 UTC (permalink / raw)
  To: u-boot

On 2017-04-04 13:17, Marek Vasut wrote:
> On 04/04/2017 09:45 PM, Stefan Agner wrote:
>> On 2017-04-04 11:38, Marek Vasut wrote:
>>> On 04/04/2017 07:57 PM, Stefan Agner wrote:
>>>> On 2017-04-04 02:22, Marek Vasut wrote:
>>>>> On 04/04/2017 02:02 AM, Stefan Agner wrote:
>>>>> [...]
>>>>>>>>> Admitedly, I didn't look at the patch, but if you want to boot ad-hoc
>>>>>>>>> cores, you can very well also boot secondary cores on the current CPU
>>>>>>>>> complex with the same command. Why not ?
>>>>>>>>
>>>>>>>> Sure, it could be done. I just feel it is not the right design.
>>>>>>>>
>>>>>>>> Auxiliary cores have usually a different view to memory, this is why I
>>>>>>>> had to add the get_host_mapping callback in the elf loader code to let
>>>>>>>> architecture dependent code translate to host addresses. SMP systems
>>>>>>>> don't need that.
>>>>>>>>
>>>>>>>> Also flush caches is not necessary on some cache coherent CPU's
>>>>>>>> (depending on how your cache coherence between I and D cache looks
>>>>>>>> like).
>>>>>>>
>>>>>>> So SMP is just a reduced special-case of this , yes ?
>>>>>>
>>>>>> Yeah, I guess you can get away with dummy callback implementation and a
>>>>>> performance hit due to cash flushes.
>>>>>
>>>>> Or you can abstract things out ?
>>>>>
>>>>
>>>> There is one callback to arch for translation and one for cache flush,
>>>> what more can I abstract out?
>>>
>>> Well then I don't think I understand your concerns about cache flushing
>>> in SMP .
>>>
>>
>> It makes things unnecessary slower.
> 
> You always have to flush cache if you expect the peripheral to read
> cached memory , so I don't think I understand your remark ...
> 

Which peripheral? The cache coherent CPU is going to read that memory...

>>>>>>>> Creating a new command like bootaux comes with very few overhead.
>>>>>>>
>>>>>>> The overhead is the new command, we already have many ad-hoc commands.
>>>>>>>
>>>>>>
>>>>>> Agreed, and I really wished that this got discussed when that command
>>>>>> initially got added. I brought it up back then...
>>>>>> https://lists.denx.de/pipermail/u-boot/2016-January/240323.html
>>>>>>
>>>>>> It seemed to be acceptable to just add this ad hoc command, with some
>>>>>> "random binary format" support back then...
>>>>>
>>>>> Based on that discussion, I only see that noone opposed, but I don't see
>>>>> any agreement.
>>>>>
>>>>
>>>> Maybe I need to word my emails a bit stronger, but with that email I
>>>> actually tried to oppose:
>>>> https://lists.denx.de/pipermail/u-boot/2016-January/240989.html
>>>
>>> Well, I do not like adding ad-hoc commands as it's not managable in the
>>> long run.
>>>
>>
>> I argue that a remote core loading command is _much_ more manageable
>> than making the bootm command even more complex by support
>> SMP/remoteproc and whatnot usecases... I would even argue that a bunch
>> of those commands are more manageable than a single ifdef/if hell....
> 
> I guess a boot* option to start remote core would do ?
> 
>> That said, I still would push for keeping the image processing code
>> generic, whenever it makes sense.
> 
> Agreed
> 
>>>>>> Ok, it is not entirely
>>>>>> random, since it is the format of a binary how it ends up in
>>>>>> microcontrollers execute in place flash (stack pointer and reset vector
>>>>>> are the two first words)....
>>>>>
>>>>> I thought this command was starting the core by loading data to RAM ,
>>>>> not flash ?
>>>>>
>>>>
>>>> Ok, maybe I am a bit unclear here:
>>>>
>>>> bootaux currently supports a Cortex-M specific binary format only. The
>>>> binary format starts with a Cortex-M class vector table. The vector
>>>> tables first vector is the "reset vector".
>>>>
>>>> In a regular microcontroller, that binary gets flashed on a NOR flash
>>>> which is mapped at 0x0 for the CPU. The CPU has no "boot ROM", the CPU
>>>> starts by calling the reset vector. So when NXP defined how the bootaux
>>>> command should look like, they just took that binary format which was
>>>> laying around and implemented a U-Boot command around it.
>>>>
>>>> So this is the history of the binary format. And my point here is, that
>>>> the binary format supported by bootaux is _very_ Cortex-M class
>>>> specific.
>>>
>>> Aha, so I now totally don't understand why this command cannot be
>>> fixed/extended to support other/generic cores or SMP systems etc.
>>> But looking at the initial proposal, I think maybe the intention of this
>>> patchset was not to add that support, but to fix the command to
>>> support loading ELF files ? We already have bootelf for that though ...
>>>
>>
>> Yes, that is pretty much it. I would like to teach that command a more
>> generic format, which would be at least a step towards something more
>> generic/standardized.
>>
>> bootelf is really meant for the primary CPU. That would be an entirely
>> different direction: Make all common boot commands "aux core" capable.
>>
>> But I would strongly vote against that. First, those commands have
>> already complex arguments and argument handling (e.g. bootm), and their
>> implementation supports use cases which we hardly would ever use on aux
>> cores (initramfs..).
> 
> That might need some further thinking/consolidation . IMO out of scope
> of this discussion ...
> 

Outside of scope, but you need to take it in consideration when
desciding which way to go with remote proc boot commands...

>>>>>> However, making this ad hoc command now
>>>>>> generic really feels weird to me, since we would end up supporting that
>>>>>> format for A class CPUs etc... bootaux is really suited for auxiliary
>>>>>> M-class cores on ARM, as it is right now. Maybe we should have named it
>>>>>> bootm ;-)
>>>>>
>>>>> We always try to avoid one-off hacks, so it's not weird. I still don't
>>>>> quite understand how it is a problem to abstract things out . I am not
>>>>> asking you to support CA, but to avoid CM specific implementation which
>>>>> cannot be extended to support CA or even MIPS/Nios2/etc .
>>>>>
>>>>
>>>> Again, why would you support a Cortex-M class specific file format for
>>>> MIPS/Nios2 etc...?
>>>>
>>>> bootaux is M4 specific. We can through it away, and start over, but then
>>>> we should call the command differently.
>>>
>>> Unless you look very carefully, it is not clear that it is cortex M4
>>> specific at all based on this discussion. I was under the impression
>>> that the goal here was to support all sorts of secondary cores ...
>>>
>>
>> Yeah it's also only that binary format handling which is M4 specific.
>> And frankly, that is just 2 lines of code, basically dereferencing the
>> first two words:
>>
>> +    if (valid_elf_image(addr)) {
>> +        stack = 0x0;
>> +        pc = load_elf_image_phdr(addr);
>> +        if (!pc)
>> +            return CMD_RET_FAILURE;
>> +
>> +    } else {
>> +        /*
>> +         * Assume binary file with vector table at the beginning.
>> +         * Cortex-M4 vector tables start with the stack pointer (SP)
>> +         * and reset vector (initial PC).
>> +         */
>> +        stack = *(u32 *)addr;
>> +        pc = *(u32 *)(addr + 4);
>> +    }
>>
>>>>>>>> This are the reasons why I feel creating a new command for a SMP boot
>>>>>>>> case makes more sense. We can still reuse functions which are very
>>>>>>>> similar by moving them into some common location, where it makes sense.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Also, I think this might come useful when booting stuff like "Altera
>>>>>>>>> Sparrow" ...
>>>>>>>>
>>>>>>>> I am not familiar with that architecture, what kind of core does it
>>>>>>>> provide which needs to be booted by U-Boot?
>>>>>>>
>>>>>>> The secondary ARM core in the SoCFPGA C-A9 complex or Nios2 core in the
>>>>>>> FPGA.
>>>>>>
>>>>>> In my thinking, the Nios2 core seems like such a remote processor well
>>>>>> suited for the bootaux command. For the secondary A9, I would create a
>>>>>> new command.
>>>>>
>>>>> Uh, why, that does not make any sense to me. Both the random core in
>>>>> FPGA and the secondary CA9 core have almost the same bringup sequence.
>>>>> What is so different about this stuff ?
>>>>>
>>>>
>>>> A cache coherent core and a non-cache coherent core remote core
>>>> somewhere in the SoC is very much different IMHO.
>>>>
>>>> Lets compare how you bring up a A class CPU, or cache coherent SMP style
>>>> CPU in general:
>>>>
>>>> 1. Load code into RAM
>>>> 2. Some cache flushing might be necessary due to I/D-cache incoherency
>>>> 3. Write entry point into some architecture specific register
>>>> 4. kick off the CPU by writing another architecture specific register
>>>>
>>>> M class/remote CPU
>>>>
>>>> 1. Load code in RAM, depending on image format translate core specific
>>>> memory map to host memory map
>>>> 2. Flush D-cache
>>>> 3. (Potentially set up IOMMU, not in the NXP case though)
>>>> 4. Write entry point into some architecture specific register
>>>> 5. kick off the CPU by writing another architecture specific register
>>>>
>>>> From what I can see, everything is different other than "Load code in
>>>> RAM" which is image specific. If the image format is complex, we
>>>> certainly should factor that out to avoid code duplication.
>>>
>>> Image format is usually binary blob or elf file, we support both.
>>> The rest is "load file to RAM, do magic setup (can be CPU specific),
>>> unreset CPU".
>>>
>>
>> Loading files to RAM is already generic since it is handled by separate
>> commands.
>>
>> It is really the image handling which has the potential of reusability.
> 
> OK
> 
>>>>>> If we want to support the two with the same command, we already have a
>>>>>> first problem: How do we address them? Of course, we could add just a
>>>>>> index or something, but this would already break backward compatibility
>>>>>> of the bootaux command.
>>>>>
>>>>> So we already accepted a command which has shit commandline semantics
>>>>> and we're stuck with it , hrm. You can always specify a default core
>>>>
>>>> Add shitty binary file format to the list.
>>>
>>> Why do we care about this ? The CM just needs a vectoring table, so set
>>> it up (in the elf file or whatever) and point the core at the reset vector.
>>>
>>
>> Not sure what you want to say here, but I guess we need to keep support
>> for this file format for backward compatibility?
> 
> I guess so, I didn't look into the requirements for the CM4 core on MX6.
> 

The only hard requirement the CPU has it needs a valid entry point, and
of course, code to run. The binary format is otherwise pretty much
independent of the hardware.

So the only reason to keep backward compatibility is because there are
already users using the command. There is documentation etc...

That said, if your firmware relies on stack pointer being setup
correctly, then your binary format should also be able to provide a
valid stack pointer. The binary format has that since that is the way
the initial reset vector on M class cores work. The elf and FIT image do
not have these capabilities. However, setting up the SP as part of the
firmware is rather easy, and the FreeRTOS BSP as offered by NXP does not
make use of the preinitialized stack pointer and sets it up anyway...


>>>>> index for those two iMX boards and it's done. Or just fix the semantics
>>>>> in their default environment, which is not awesome, but I doubt this is
>>>>> wildly used yet.
>>>>
>>>> I guess my general point is, bootaux is already broken in two ways:
>>>> Command line interface does not allow for extensibility, the only format
>>>> supported right now is a binary format which is not generic.
>>>>
>>>> My patchset was an attempt to improve the situation to give it at least
>>>> a decent elf format support.
>>>
>>> Maybe this information was lost ... Lukasz ?!
>>>
>>>> Note that there is also bootelf, where the elf headers have been
>>>> introduced. Reusing that command seems impractical since it is meant to
>>>> boot on the primary CPU. My first attempt was reusing at least the load
>>>> function, but it seemed impractical due to memory map translation and
>>>> since the load function is rather short. If it helps for the acceptance
>>>> of this patchset, I still could try to make that happen.
>>>
>>> Extending bootelf might indeed make more sense IMO. But that needs
>>> discussion.
>>>
>>
>> There are two variants of reuse here:
>> 1. Reuse the elf parsing and loading function (function
>> load_elf_image_phdr in this case).
>> 2. Reuse the bootelf command. This would need extending the command with
>> some parameter to tell it to not boot the primary CPU but boot some
>> other auxiliary processor.
>>
>>
>> Let's get a bit more concrete here, take bootelf as a case study:
>>
>> 1 would introduce the architecture specific translation callback
>> load_elf_image_phdr (see my load_elf_image_phdr implementation in this
>> patchset). For regular/primary boot we can implement a dummy 1:1
>> translation.
>>
>>
>>
>> 2 would need 1, and changes to do_bootelf. Since the command has already
>> optional arguments adding core selection as an optional argument is not
>> possible. But we could add a parameter, e.g. -c (core):
>>
>> bootelf -c aux
>>
>> or
>>
>> bootelf -c 1
> 
> Works for me ...
> 
>> The exact enumeration would have to be discussed. I guess this would
>> also be architecture specific.
>>
>> Adding the new parameter handling plus callbacks into architecture
>> dependent code probably makes do_bootelf double in length. Also, half of
>> the existing code cannot be reused: The call do_bootelf_exec is primary
>> CPU specific, passing the arguments (this is likely not supported by
>> remote cores) and the return code handling (since a remote core would
>> not return), autostart handling, not sure?

Here...  see below

>>
>> 2 would also need to either disable the section header functionality for
>> the remote core, or we add another if in do_bootelf and avoid parsing
>> section header even if requested...
>>
>>
>> Again, I strongly disfavor solution 2. That makes boot commands with
>> image types we want to support even more complex. bootelf already starts
>> to look ugly when I start implementing that, and bootelf is really one
>> of the cleanest and simplest boot commands I have seen... I probably
>> would give it a shot if this is the only way to bring elf format support
>> for aux cores upstream... Good luck for the guy implementing FIT image
>> support (bootm) for auxiliary cores :-D
> 
> So what is the problem with this ? Load the payload somewhere and ungate
> the secondary core , why is that so hard ?
> 

Are you ignoring my arguments above on purpose? As I clearly showed,
even bootelf has differences between a primary and a secondary/remote
core... I don't expect that for bootm to be any different.

In fact, almost certainly there will be a lot more primary core/remote
core distinctions to be made during the whole bootm command. bootm also
supports cases which probably never make sense for a remote core
(Android images, initramfs). Sure, hacking something up which works in a
certain use-case is probably not a big deal, but to do it properly, you
would have to check every image format, every corner case, and properly
disambiguate between primary/auxilary core scenarios (otherwise you end
up with "for me, bootm -c aux my.fit works, I don't know why your bootm
-c your.uimg does not work!", sure you can just ignore that, but IMHO
that is broken design, and just not user-friendly).


But then, I really couldn't care less since I am not interested in FIT
image support for remote processors. My point is just that in bootm will
be ugly and hard to properly support auxiliary cores. And I feel when
bootm does not work well, and bootelf also is a bit bumpy, we probably
should just not mix up primary core boot commands and auxilary core boot
commands...

--
Stefan


>> 1 we could do, and I would be willing to do it. I personally feel that
>> the small code duplication is cleaner than reusing that function from
>> cmd/elf.c and implementing the dummy 1:1 translation, but both works for
>> me....
>>
>> IMHO, commands are really lightweight and nice in U-Boot, I think we
>> should make use of them, even when it means some namespace pollution.
>>
>> --
>> Stefan
>>
>>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support
  2017-04-04 18:59       ` Stefan Agner
@ 2017-04-05 15:15         ` Lukasz Majewski
  2017-04-05 18:20           ` Stefan Agner
  0 siblings, 1 reply; 25+ messages in thread
From: Lukasz Majewski @ 2017-04-05 15:15 UTC (permalink / raw)
  To: u-boot

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="windows-1254", Size: 12850 bytes --]

Hi Stefan,

> On 2017-04-04 01:23, Lukasz Majewski wrote:
> > Hi Stefan,
> > 
> >> Hi Lukasz,
> >>
> >> On 2017-04-03 04:20, Lukasz Majewski wrote:
> >> > Hi Stefan,
> >> >
> >> > Thanks for your patch. Please allow me to share some ideas for
> >> > improvements.
> >> >
> >> >> From: Stefan Agner <stefan.agner@toradex.com>
> >> >>
> >> >> This patchset enables to boot elf binaries on secondary Cortex-M
> >> >> class cores available on i.MX 6SoloX/7Solo/7Dual. This makes
> >> >> handling and loading firmwares much more convinient since all
> >> >> information where the firmware has to be loaded to is contained
> >> >> in the elf headers. A typical usage looks like this:
> >> >>
> >> >>   Colibri iMX7 # tftp ${loadaddr} firmware.elf && bootaux
> >> >> ${loadaddr} Using FEC0 device
> >> >>   TFTP from server 192.168.10.1; our IP address is 192.168.10.2
> >> >>   Filename 'firmware.elf'.
> >> >>   Load address: 0x80800000
> >> >>   Loading: ##################################################
> >> >> 88.3 KiB 5.4 MiB/s
> >> >>   done
> >> >>   Bytes transferred = 90424 (16138 hex)
> >> >>   ## Starting auxiliary core at 0x1FFF8311 ...
> >> >>   Colibri iMX7 #
> >> >
> >> > I can find some other platforms (not only IMX), which would
> >> > benefit from this code - the generic 'bootaux' command.
> >> >
> >> > One good example would to allow multiple binaries for different
> >> > SoC Cores (e.g. 2x Cortex-A8) to be loaded and started by u-boot.
> >> >
> >> > Hence, I'm wondering if you could make those patches usable for
> >> > other platforms as well?
> >>
> >> I don't think that this is a good idea. bootaux is meant for
> >> auxiliary cores, which often use a different architecture and are
> >> not cache coherent (hence the cache flushes).
> > 
> > I do remember, that I saw similar "tiny" patch to add "just one"
> > ad-hoc command to do the same (conceptually) for TI SoC floating on
> > the u-boot mailing list.
> > 
> > Please correct me if I'm wrong, but bootaux is IMX specific and does
> > work, which very likely, will be also required by other SoC vendors
> > (TI's Sitara is also equipped with Cortex-M4 and PRUSS).
> 
> bootaux is currently IMX specific, and its currently supported binary
> format is M-class specific.
> 
> > 
> > Unification of such effort can save us all a lot of trouble in a
> > very near future.
> 
> In OSS, you do not develop for the future. It gets developed when its
> here.

I cannot agree here. When you perceive threat then you prepare for it.

> 
> > 
> >>
> >> On SMP systems the main operating system normally starts the
> >> secondary core.
> > 
> > I think that there are some conceptual similarities between loading
> > code to SMP core and Cortex M3. Of course some "tweaks" would be
> > needed.
> >
> 
> There are conceptual similarities between a car and a truck, still,
> they are likely manufactured in a different assembly line, probably
> by a different producer.
> 
> I guess in the end it really depends on where you draw the line: There
> are differences between loading code which will get executed by the
> primary code, loading code to execute explicitly on a SMP core, and
> loading code to execute on a remote processor.
> 
> The first case is already well supported, and we need to keep support
> backward compatibility.
> 
> The second case, IMHO, is a somewhat silly case: A SMP system usually
> gets driven by a single OS image, and that OS makes sure to initialize
> the cores (maybe with the help of firmware, such as the PSCI interface
> on ARM). There is no need for a boot loader command to execute a image
> on a secondary core explicitly...

I do understand (and agree) with your point with SMP.

But, I do know at least one use case when somebody would like to start
two binaries (those are bare metal programs, taking care of SoC setup,
IPC, etc).

And maybe Linux with some FPGA based IP block already configured in
u-boot.

> 
> The third case is probably a case where we could start think about
> unification efforts.
> 
> 
> >> Otherwise, if you want to run them separately using U-Boot,
> >> maybe a new command such as bootsmp would be more suited.
> > 
> > Hmm - I do think that it would be too much:
> > 
> > - bootm for generic single core
> > - bootaux for IMX
> > - bootsmp for SMP (on IMX/TI/RK?)
> > - boot?? for ??
> 
> There is at least also bootz and bootelf.

I will reply to the other thread regarding bootelf.

> 
> > 
> > I would like to avoid adding new commands for doing conceptually the
> > same work.
> > 
> > Even better, we could extend bootm to support the "multi binary"
> > case too, but IMHO it would be also correct to have "bootaux" to
> > handle generic binaries loading.
> > 
> >>
> >> >
> >> >>
> >> >> Note that the bootaux command retrieved the entry point (PC)
> >> >> from the elf binary.
> >> >
> >> > Could you make this code flexible enough to handle not only elf
> >> > binaries, but also other data (e.g. FPGA bitstreams)?
> >>
> >> The code of bootaux is rather small, I don't see much value into
> >> stuff boot code for other peripherals into it.
> > 
> > But I'm not asking to write support for other vendor's SoC/use
> > cases.
> > 
> > I'm just wondering if you could write generic command (framework) to
> > support this use case and as an example add support for your IMX's
> > Cortex-M3/4.
> > 
> 
> Sure, mv arch/arm/imx-common/imx_bootaux.c cmd/, there is the
> framework :-)
> 
> But this promotes the M class specific binary format to a generic
> format supported for all cores.
> 
> > We would kill two birds with one stone - IMX is supported from the
> > very beginning and we do have generic code to extend it by other
> > vendors.
> > 
> >> I don't know how FPGA
> >> bistream loading typically works, but I guess it is quite different
> >> from loading a firmware into RAM/SRAM and flush caches...
> > 
> > FPGA quirks would be handled in arch/SoC specific code.
> > 
> >>
> >> I am not against reuse and unification, I just feel that this is
> >> not really a case where we gain much.
> > 
> > With generic "bootaux/bootm" command we can point other developers
> > who would like to add such booting code to the already available
> > framework.
> > 
> > Also, we would prevent other "ad-hoc" commands adding to u-boot.
> > 
> 
> Extending bootm does not seem like a good idea. bootm is already
> rather complex, making it even more complex by supporting the
> auxiliary core case seems really not work well.
> 
> Also, bootm supports lots of features which you probably never would
> use or test on a remote core (e.g. initramfs etc...)
> 
> >>
> >> >
> >> > Maybe it would better to:
> >> > -------------------------
> >> >
> >> > Embrace those binaries into FIT file (*.itb)? And allow multiple
> >> > binaries loading? I'm thinking of work similar to one already
> >> > did by Andre Przywara for SPL:
> >> >
> >> > "[PATCH v3 00/19] SPL: extend FIT loading support" posted on
> >> > 31.03.2017?
> >> >
> >> > In that way we would "open" many new use cases, and other
> >> > platforms (e.g. FPGA's, TI, etc) could benefit from it.
> >> > One solid use case is to load different Linux binaries (or/and
> >> > bare metal programs) to different SoC cores.
> >> >
> >> > The "meta data" (i.e. load address, data type, description),
> >> > could be extracted from the FIT format (the code is already in
> >> > u-boot).
> >> >
> >> > IMHO, this is very generic approach.
> >>
> >> I did some experiments with using FIT images for auxiliary core
> >> firmware, however, it did not add a lot of advantage over using
> >> elf:
> >> http://git.toradex.com/cgit/u-boot-toradex.git/commit/?h=2015.04-toradex-next&id=d1d416f272e840e8139aec911f89a70fe5523eb2
> > 
> > Unfortunately, not all use cases allow using ELF format. FIT gives
> > more flexibility:
> > 
> > -  ./doc/README.dfutftp -> different images loading
> > 
> > -  Andre's patch to load multiple binaries in SPL - [PATCH v3 00/19]
> >   SPL: extend FIT loading support"
> > 
> 
> Are flexible, but very much U-Boot specific. And much more complex to
> support.
> 
> >>
> >> Firmwares are already built and available in the elf file format.
> >> The Linux remoteproc framework, which is meant to handle this kind
> >> of cores too, supports elf firmware loading too, so supporting elf
> >> in U-Boot too is a nice alignment. Also using FIT adds an
> >> additional step when building firm wares...
> > 
> > But this is all OK.
> > 
> > The ELF binary would be wrapped in FIT (with e.g. "elf" property,
> > even 1 to 1 mapping). Then the 'bootaux/bootm' could parse ELF
> > (which is also generic). And then some "IMX specific" (arch/SoC)
> > code would be executed.
> > 
> 
> So we go from a nacked binary loaded directly to the place it has been
> linked to, to a double wrapped firmware file...?

We would have elf binary file embedded into FIT file, these would bring
flexibility.

FIT support is in place (u-boot/spl). 

In such a way you can use any binary in any format.

But I must admit that we are going off-topic here.....

> 
> Not sure if user will appriciate that extra work and boot time. And
> developers the extra code.
> 
> Maybe in a perfect world that just works, because you know, FIT is
> generic, and elf is generic...  But that is just not how it works in
> practice. The existing FIT code is rather complex, supports lots of
> corner cases. The elf code supports different header types... Loading
> the elf sections (which use M4 specific addressing) needs address
> translation to get to suitable host addresses...

Maybe I'm an idealist :-)

One analogy comes to my mind between linux and u-boot.

The proliferation of u-boot commands and linux board files. Why Linux
spend tremendous resources to remove board files and switch to device
tree?

> 
> > 
> >>
> >>
> >> >
> >> >> Also all sections are translated to A7 addresses
> >> >> in order to properly load the firmware sections into the
> >> >> appropriate locations.
> >> >
> >> > This would require some tiny arch specific code.
> >> >
> >>
> >> Yes, but in my rather small patchset (164 additional lines) this is
> >> actually almost the bulk of changes :-)
> > 
> > Yes, I fully understand you :-) - I do remember when I wanted to
> > add 3 lines (polarity inversion) to IMX PWM subsystem some time
> > ago ... :-) 
> > 
> 
> Hehe, yeah that was a rather long episode.
> 
> It was a bit different, though, the core framework was already there,
> it was mostly about adopting a particular driver to that framework.
> Building a new framework is a bit more involved...
> 
> --
> Stefan
> 
> > Best regards,
> > Łukasz Majewski
> > 
> >>
> >> --
> >> Stefan
> >>
> >> >> Also cache flushes is taken care of, so that there is no
> >> >> manual dcache flush necessary anymore.
> >> >>
> >> >> The NXP FreeRTOS BSP already generates elf binaries which can be
> >> >> directly used with this elf binary support.
> >> >>
> >> >> The last patch adds bootaux support for Vybrid.
> >> >>
> >> >>
> >> >> Stefan Agner (3):
> >> >>   imx: imx-common: move aux core image parsing to common code
> >> >>   imx: imx-common: add elf firmware support
> >> >>   ARM: vf610: add auxiliary core boot support
> >> >>
> >> >>  arch/arm/cpu/armv7/mx6/soc.c                |  30 +++++---
> >> >>  arch/arm/cpu/armv7/mx7/soc.c                |  34 ++++++---
> >> >>  arch/arm/cpu/armv7/vf610/generic.c          |  42 +++++++++++
> >> >>  arch/arm/imx-common/Kconfig                 |   2 +-
> >> >>  arch/arm/imx-common/Makefile                |   4 +-
> >> >>  arch/arm/imx-common/imx_bootaux.c           | 105
> >> >> +++++++++++++++++++++++-----
> >> >> arch/arm/include/asm/arch-vf610/sys_proto.h |   8 +++
> >> >> arch/arm/include/asm/imx-common/sys_proto.h |   6 ++
> >> >> configs/colibri_vf_defconfig                |   1 + 9 files
> >> >> changed, 198 insertions(+), 34 deletions(-) create mode 100644
> >> >> arch/arm/include/asm/arch-vf610/sys_proto.h
> >> >>
> >> >
> >> >
> >> >
> >> >
> >> > Best regards,
> >> >
> >> > Lukasz Majewski
> >> >
> >> > --
> >> >
> >> > DENX Software Engineering GmbH,      Managing Director: Wolfgang
> >> > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194
> >> > Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax:
> >> > (+49)-8142-66989-80 Email: wd at denx.de
> > 
> > 
> > 
> > 
> > Best regards,
> > 
> > Lukasz Majewski
> > 
> > --
> > 
> > DENX Software Engineering GmbH,      Managing Director: Wolfgang
> > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
> > Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email:
> > wd at denx.de




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support
  2017-04-05 15:15         ` Lukasz Majewski
@ 2017-04-05 18:20           ` Stefan Agner
  2017-04-05 19:10             ` Tom Rini
  0 siblings, 1 reply; 25+ messages in thread
From: Stefan Agner @ 2017-04-05 18:20 UTC (permalink / raw)
  To: u-boot

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="windows-1254", Size: 13843 bytes --]

On 2017-04-05 08:15, Lukasz Majewski wrote:
> Hi Stefan,
> 
>> On 2017-04-04 01:23, Lukasz Majewski wrote:
>> > Hi Stefan,
>> >
>> >> Hi Lukasz,
>> >>
>> >> On 2017-04-03 04:20, Lukasz Majewski wrote:
>> >> > Hi Stefan,
>> >> >
>> >> > Thanks for your patch. Please allow me to share some ideas for
>> >> > improvements.
>> >> >
>> >> >> From: Stefan Agner <stefan.agner@toradex.com>
>> >> >>
>> >> >> This patchset enables to boot elf binaries on secondary Cortex-M
>> >> >> class cores available on i.MX 6SoloX/7Solo/7Dual. This makes
>> >> >> handling and loading firmwares much more convinient since all
>> >> >> information where the firmware has to be loaded to is contained
>> >> >> in the elf headers. A typical usage looks like this:
>> >> >>
>> >> >>   Colibri iMX7 # tftp ${loadaddr} firmware.elf && bootaux
>> >> >> ${loadaddr} Using FEC0 device
>> >> >>   TFTP from server 192.168.10.1; our IP address is 192.168.10.2
>> >> >>   Filename 'firmware.elf'.
>> >> >>   Load address: 0x80800000
>> >> >>   Loading: ##################################################
>> >> >> 88.3 KiB 5.4 MiB/s
>> >> >>   done
>> >> >>   Bytes transferred = 90424 (16138 hex)
>> >> >>   ## Starting auxiliary core at 0x1FFF8311 ...
>> >> >>   Colibri iMX7 #
>> >> >
>> >> > I can find some other platforms (not only IMX), which would
>> >> > benefit from this code - the generic 'bootaux' command.
>> >> >
>> >> > One good example would to allow multiple binaries for different
>> >> > SoC Cores (e.g. 2x Cortex-A8) to be loaded and started by u-boot.
>> >> >
>> >> > Hence, I'm wondering if you could make those patches usable for
>> >> > other platforms as well?
>> >>
>> >> I don't think that this is a good idea. bootaux is meant for
>> >> auxiliary cores, which often use a different architecture and are
>> >> not cache coherent (hence the cache flushes).
>> >
>> > I do remember, that I saw similar "tiny" patch to add "just one"
>> > ad-hoc command to do the same (conceptually) for TI SoC floating on
>> > the u-boot mailing list.
>> >
>> > Please correct me if I'm wrong, but bootaux is IMX specific and does
>> > work, which very likely, will be also required by other SoC vendors
>> > (TI's Sitara is also equipped with Cortex-M4 and PRUSS).
>>
>> bootaux is currently IMX specific, and its currently supported binary
>> format is M-class specific.
>>
>> >
>> > Unification of such effort can save us all a lot of trouble in a
>> > very near future.
>>
>> In OSS, you do not develop for the future. It gets developed when its
>> here.
> 
> I cannot agree here. When you perceive threat then you prepare for it.
> 
>>
>> >
>> >>
>> >> On SMP systems the main operating system normally starts the
>> >> secondary core.
>> >
>> > I think that there are some conceptual similarities between loading
>> > code to SMP core and Cortex M3. Of course some "tweaks" would be
>> > needed.
>> >
>>
>> There are conceptual similarities between a car and a truck, still,
>> they are likely manufactured in a different assembly line, probably
>> by a different producer.
>>
>> I guess in the end it really depends on where you draw the line: There
>> are differences between loading code which will get executed by the
>> primary code, loading code to execute explicitly on a SMP core, and
>> loading code to execute on a remote processor.
>>
>> The first case is already well supported, and we need to keep support
>> backward compatibility.
>>
>> The second case, IMHO, is a somewhat silly case: A SMP system usually
>> gets driven by a single OS image, and that OS makes sure to initialize
>> the cores (maybe with the help of firmware, such as the PSCI interface
>> on ARM). There is no need for a boot loader command to execute a image
>> on a secondary core explicitly...
> 
> I do understand (and agree) with your point with SMP.
> 
> But, I do know at least one use case when somebody would like to start
> two binaries (those are bare metal programs, taking care of SoC setup,
> IPC, etc).
> 
> And maybe Linux with some FPGA based IP block already configured in
> u-boot.
> 
>>
>> The third case is probably a case where we could start think about
>> unification efforts.
>>
>>
>> >> Otherwise, if you want to run them separately using U-Boot,
>> >> maybe a new command such as bootsmp would be more suited.
>> >
>> > Hmm - I do think that it would be too much:
>> >
>> > - bootm for generic single core
>> > - bootaux for IMX
>> > - bootsmp for SMP (on IMX/TI/RK?)
>> > - boot?? for ??
>>
>> There is at least also bootz and bootelf.
> 
> I will reply to the other thread regarding bootelf.
> 
>>
>> >
>> > I would like to avoid adding new commands for doing conceptually the
>> > same work.
>> >
>> > Even better, we could extend bootm to support the "multi binary"
>> > case too, but IMHO it would be also correct to have "bootaux" to
>> > handle generic binaries loading.
>> >
>> >>
>> >> >
>> >> >>
>> >> >> Note that the bootaux command retrieved the entry point (PC)
>> >> >> from the elf binary.
>> >> >
>> >> > Could you make this code flexible enough to handle not only elf
>> >> > binaries, but also other data (e.g. FPGA bitstreams)?
>> >>
>> >> The code of bootaux is rather small, I don't see much value into
>> >> stuff boot code for other peripherals into it.
>> >
>> > But I'm not asking to write support for other vendor's SoC/use
>> > cases.
>> >
>> > I'm just wondering if you could write generic command (framework) to
>> > support this use case and as an example add support for your IMX's
>> > Cortex-M3/4.
>> >
>>
>> Sure, mv arch/arm/imx-common/imx_bootaux.c cmd/, there is the
>> framework :-)
>>
>> But this promotes the M class specific binary format to a generic
>> format supported for all cores.
>>
>> > We would kill two birds with one stone - IMX is supported from the
>> > very beginning and we do have generic code to extend it by other
>> > vendors.
>> >
>> >> I don't know how FPGA
>> >> bistream loading typically works, but I guess it is quite different
>> >> from loading a firmware into RAM/SRAM and flush caches...
>> >
>> > FPGA quirks would be handled in arch/SoC specific code.
>> >
>> >>
>> >> I am not against reuse and unification, I just feel that this is
>> >> not really a case where we gain much.
>> >
>> > With generic "bootaux/bootm" command we can point other developers
>> > who would like to add such booting code to the already available
>> > framework.
>> >
>> > Also, we would prevent other "ad-hoc" commands adding to u-boot.
>> >
>>
>> Extending bootm does not seem like a good idea. bootm is already
>> rather complex, making it even more complex by supporting the
>> auxiliary core case seems really not work well.
>>
>> Also, bootm supports lots of features which you probably never would
>> use or test on a remote core (e.g. initramfs etc...)
>>
>> >>
>> >> >
>> >> > Maybe it would better to:
>> >> > -------------------------
>> >> >
>> >> > Embrace those binaries into FIT file (*.itb)? And allow multiple
>> >> > binaries loading? I'm thinking of work similar to one already
>> >> > did by Andre Przywara for SPL:
>> >> >
>> >> > "[PATCH v3 00/19] SPL: extend FIT loading support" posted on
>> >> > 31.03.2017?
>> >> >
>> >> > In that way we would "open" many new use cases, and other
>> >> > platforms (e.g. FPGA's, TI, etc) could benefit from it.
>> >> > One solid use case is to load different Linux binaries (or/and
>> >> > bare metal programs) to different SoC cores.
>> >> >
>> >> > The "meta data" (i.e. load address, data type, description),
>> >> > could be extracted from the FIT format (the code is already in
>> >> > u-boot).
>> >> >
>> >> > IMHO, this is very generic approach.
>> >>
>> >> I did some experiments with using FIT images for auxiliary core
>> >> firmware, however, it did not add a lot of advantage over using
>> >> elf:
>> >> http://git.toradex.com/cgit/u-boot-toradex.git/commit/?h=2015.04-toradex-next&id=d1d416f272e840e8139aec911f89a70fe5523eb2
>> >
>> > Unfortunately, not all use cases allow using ELF format. FIT gives
>> > more flexibility:
>> >
>> > -  ./doc/README.dfutftp -> different images loading
>> >
>> > -  Andre's patch to load multiple binaries in SPL - [PATCH v3 00/19]
>> >   SPL: extend FIT loading support"
>> >
>>
>> Are flexible, but very much U-Boot specific. And much more complex to
>> support.
>>
>> >>
>> >> Firmwares are already built and available in the elf file format.
>> >> The Linux remoteproc framework, which is meant to handle this kind
>> >> of cores too, supports elf firmware loading too, so supporting elf
>> >> in U-Boot too is a nice alignment. Also using FIT adds an
>> >> additional step when building firm wares...
>> >
>> > But this is all OK.
>> >
>> > The ELF binary would be wrapped in FIT (with e.g. "elf" property,
>> > even 1 to 1 mapping). Then the 'bootaux/bootm' could parse ELF
>> > (which is also generic). And then some "IMX specific" (arch/SoC)
>> > code would be executed.
>> >
>>
>> So we go from a nacked binary loaded directly to the place it has been
>> linked to, to a double wrapped firmware file...?
> 
> We would have elf binary file embedded into FIT file, these would bring
> flexibility.
> 
> FIT support is in place (u-boot/spl). 
> 
> In such a way you can use any binary in any format.
> 
> But I must admit that we are going off-topic here.....
> 
>>
>> Not sure if user will appriciate that extra work and boot time. And
>> developers the extra code.
>>
>> Maybe in a perfect world that just works, because you know, FIT is
>> generic, and elf is generic...  But that is just not how it works in
>> practice. The existing FIT code is rather complex, supports lots of
>> corner cases. The elf code supports different header types... Loading
>> the elf sections (which use M4 specific addressing) needs address
>> translation to get to suitable host addresses...
> 
> Maybe I'm an idealist :-)
> 
> One analogy comes to my mind between linux and u-boot.
> 
> The proliferation of u-boot commands and linux board files. Why Linux
> spend tremendous resources to remove board files and switch to device
> tree?
> 

I argue that is the right way of doing it: Do ad-hoc solutions, whatever
makes sense, and once you understand the full breath of design space it
is much easier to build a suitable framework. At that point, do the
refactoring and build that suitable framework.

If you design a framework without the understanding of the whole design
space, you will end up with something which does not work well and needs
lots of work-arounds etc...

Of course, it is a bit different since both examples have outside facing
impact (change to device tree as well as changes to U-Boot commands).

--
Stefan

>>
>> >
>> >>
>> >>
>> >> >
>> >> >> Also all sections are translated to A7 addresses
>> >> >> in order to properly load the firmware sections into the
>> >> >> appropriate locations.
>> >> >
>> >> > This would require some tiny arch specific code.
>> >> >
>> >>
>> >> Yes, but in my rather small patchset (164 additional lines) this is
>> >> actually almost the bulk of changes :-)
>> >
>> > Yes, I fully understand you :-) - I do remember when I wanted to
>> > add 3 lines (polarity inversion) to IMX PWM subsystem some time
>> > ago ... :-)
>> >
>>
>> Hehe, yeah that was a rather long episode.
>>
>> It was a bit different, though, the core framework was already there,
>> it was mostly about adopting a particular driver to that framework.
>> Building a new framework is a bit more involved...
>>
>> --
>> Stefan
>>
>> > Best regards,
>> > Łukasz Majewski
>> >
>> >>
>> >> --
>> >> Stefan
>> >>
>> >> >> Also cache flushes is taken care of, so that there is no
>> >> >> manual dcache flush necessary anymore.
>> >> >>
>> >> >> The NXP FreeRTOS BSP already generates elf binaries which can be
>> >> >> directly used with this elf binary support.
>> >> >>
>> >> >> The last patch adds bootaux support for Vybrid.
>> >> >>
>> >> >>
>> >> >> Stefan Agner (3):
>> >> >>   imx: imx-common: move aux core image parsing to common code
>> >> >>   imx: imx-common: add elf firmware support
>> >> >>   ARM: vf610: add auxiliary core boot support
>> >> >>
>> >> >>  arch/arm/cpu/armv7/mx6/soc.c                |  30 +++++---
>> >> >>  arch/arm/cpu/armv7/mx7/soc.c                |  34 ++++++---
>> >> >>  arch/arm/cpu/armv7/vf610/generic.c          |  42 +++++++++++
>> >> >>  arch/arm/imx-common/Kconfig                 |   2 +-
>> >> >>  arch/arm/imx-common/Makefile                |   4 +-
>> >> >>  arch/arm/imx-common/imx_bootaux.c           | 105
>> >> >> +++++++++++++++++++++++-----
>> >> >> arch/arm/include/asm/arch-vf610/sys_proto.h |   8 +++
>> >> >> arch/arm/include/asm/imx-common/sys_proto.h |   6 ++
>> >> >> configs/colibri_vf_defconfig                |   1 + 9 files
>> >> >> changed, 198 insertions(+), 34 deletions(-) create mode 100644
>> >> >> arch/arm/include/asm/arch-vf610/sys_proto.h
>> >> >>
>> >> >
>> >> >
>> >> >
>> >> >
>> >> > Best regards,
>> >> >
>> >> > Lukasz Majewski
>> >> >
>> >> > --
>> >> >
>> >> > DENX Software Engineering GmbH,      Managing Director: Wolfgang
>> >> > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194
>> >> > Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax:
>> >> > (+49)-8142-66989-80 Email: wd at denx.de
>> >
>> >
>> >
>> >
>> > Best regards,
>> >
>> > Lukasz Majewski
>> >
>> > --
>> >
>> > DENX Software Engineering GmbH,      Managing Director: Wolfgang
>> > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
>> > Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email:
>> > wd at denx.de
> 
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support
  2017-04-05 18:20           ` Stefan Agner
@ 2017-04-05 19:10             ` Tom Rini
  2017-04-05 19:54               ` Lukasz Majewski
  2017-04-05 19:56               ` Stefan Agner
  0 siblings, 2 replies; 25+ messages in thread
From: Tom Rini @ 2017-04-05 19:10 UTC (permalink / raw)
  To: u-boot

On Wed, Apr 05, 2017 at 11:20:43AM -0700, Stefan Agner wrote:
> On 2017-04-05 08:15, Lukasz Majewski wrote:
> > Hi Stefan,
> > 
> >> On 2017-04-04 01:23, Lukasz Majewski wrote:
> >> > Hi Stefan,
> >> >
> >> >> Hi Lukasz,
> >> >>
> >> >> On 2017-04-03 04:20, Lukasz Majewski wrote:
> >> >> > Hi Stefan,
> >> >> >
> >> >> > Thanks for your patch. Please allow me to share some ideas for
> >> >> > improvements.
> >> >> >
> >> >> >> From: Stefan Agner <stefan.agner@toradex.com>
> >> >> >>
> >> >> >> This patchset enables to boot elf binaries on secondary Cortex-M
> >> >> >> class cores available on i.MX 6SoloX/7Solo/7Dual. This makes
> >> >> >> handling and loading firmwares much more convinient since all
> >> >> >> information where the firmware has to be loaded to is contained
> >> >> >> in the elf headers. A typical usage looks like this:
> >> >> >>
> >> >> >>   Colibri iMX7 # tftp ${loadaddr} firmware.elf && bootaux
> >> >> >> ${loadaddr} Using FEC0 device
> >> >> >>   TFTP from server 192.168.10.1; our IP address is 192.168.10.2
> >> >> >>   Filename 'firmware.elf'.
> >> >> >>   Load address: 0x80800000
> >> >> >>   Loading: ##################################################
> >> >> >> 88.3 KiB 5.4 MiB/s
> >> >> >>   done
> >> >> >>   Bytes transferred = 90424 (16138 hex)
> >> >> >>   ## Starting auxiliary core at 0x1FFF8311 ...
> >> >> >>   Colibri iMX7 #
> >> >> >
> >> >> > I can find some other platforms (not only IMX), which would
> >> >> > benefit from this code - the generic 'bootaux' command.
> >> >> >
> >> >> > One good example would to allow multiple binaries for different
> >> >> > SoC Cores (e.g. 2x Cortex-A8) to be loaded and started by u-boot.
> >> >> >
> >> >> > Hence, I'm wondering if you could make those patches usable for
> >> >> > other platforms as well?
> >> >>
> >> >> I don't think that this is a good idea. bootaux is meant for
> >> >> auxiliary cores, which often use a different architecture and are
> >> >> not cache coherent (hence the cache flushes).
> >> >
> >> > I do remember, that I saw similar "tiny" patch to add "just one"
> >> > ad-hoc command to do the same (conceptually) for TI SoC floating on
> >> > the u-boot mailing list.
> >> >
> >> > Please correct me if I'm wrong, but bootaux is IMX specific and does
> >> > work, which very likely, will be also required by other SoC vendors
> >> > (TI's Sitara is also equipped with Cortex-M4 and PRUSS).
> >>
> >> bootaux is currently IMX specific, and its currently supported binary
> >> format is M-class specific.
> >>
> >> >
> >> > Unification of such effort can save us all a lot of trouble in a
> >> > very near future.
> >>
> >> In OSS, you do not develop for the future. It gets developed when its
> >> here.
> > 
> > I cannot agree here. When you perceive threat then you prepare for it.
> > 
> >>
> >> >
> >> >>
> >> >> On SMP systems the main operating system normally starts the
> >> >> secondary core.
> >> >
> >> > I think that there are some conceptual similarities between loading
> >> > code to SMP core and Cortex M3. Of course some "tweaks" would be
> >> > needed.
> >> >
> >>
> >> There are conceptual similarities between a car and a truck, still,
> >> they are likely manufactured in a different assembly line, probably
> >> by a different producer.
> >>
> >> I guess in the end it really depends on where you draw the line: There
> >> are differences between loading code which will get executed by the
> >> primary code, loading code to execute explicitly on a SMP core, and
> >> loading code to execute on a remote processor.
> >>
> >> The first case is already well supported, and we need to keep support
> >> backward compatibility.
> >>
> >> The second case, IMHO, is a somewhat silly case: A SMP system usually
> >> gets driven by a single OS image, and that OS makes sure to initialize
> >> the cores (maybe with the help of firmware, such as the PSCI interface
> >> on ARM). There is no need for a boot loader command to execute a image
> >> on a secondary core explicitly...
> > 
> > I do understand (and agree) with your point with SMP.
> > 
> > But, I do know at least one use case when somebody would like to start
> > two binaries (those are bare metal programs, taking care of SoC setup,
> > IPC, etc).
> > 
> > And maybe Linux with some FPGA based IP block already configured in
> > u-boot.
> > 
> >>
> >> The third case is probably a case where we could start think about
> >> unification efforts.
> >>
> >>
> >> >> Otherwise, if you want to run them separately using U-Boot,
> >> >> maybe a new command such as bootsmp would be more suited.
> >> >
> >> > Hmm - I do think that it would be too much:
> >> >
> >> > - bootm for generic single core
> >> > - bootaux for IMX
> >> > - bootsmp for SMP (on IMX/TI/RK?)
> >> > - boot?? for ??
> >>
> >> There is at least also bootz and bootelf.
> > 
> > I will reply to the other thread regarding bootelf.
> > 
> >>
> >> >
> >> > I would like to avoid adding new commands for doing conceptually the
> >> > same work.
> >> >
> >> > Even better, we could extend bootm to support the "multi binary"
> >> > case too, but IMHO it would be also correct to have "bootaux" to
> >> > handle generic binaries loading.
> >> >
> >> >>
> >> >> >
> >> >> >>
> >> >> >> Note that the bootaux command retrieved the entry point (PC)
> >> >> >> from the elf binary.
> >> >> >
> >> >> > Could you make this code flexible enough to handle not only elf
> >> >> > binaries, but also other data (e.g. FPGA bitstreams)?
> >> >>
> >> >> The code of bootaux is rather small, I don't see much value into
> >> >> stuff boot code for other peripherals into it.
> >> >
> >> > But I'm not asking to write support for other vendor's SoC/use
> >> > cases.
> >> >
> >> > I'm just wondering if you could write generic command (framework) to
> >> > support this use case and as an example add support for your IMX's
> >> > Cortex-M3/4.
> >> >
> >>
> >> Sure, mv arch/arm/imx-common/imx_bootaux.c cmd/, there is the
> >> framework :-)
> >>
> >> But this promotes the M class specific binary format to a generic
> >> format supported for all cores.
> >>
> >> > We would kill two birds with one stone - IMX is supported from the
> >> > very beginning and we do have generic code to extend it by other
> >> > vendors.
> >> >
> >> >> I don't know how FPGA
> >> >> bistream loading typically works, but I guess it is quite different
> >> >> from loading a firmware into RAM/SRAM and flush caches...
> >> >
> >> > FPGA quirks would be handled in arch/SoC specific code.
> >> >
> >> >>
> >> >> I am not against reuse and unification, I just feel that this is
> >> >> not really a case where we gain much.
> >> >
> >> > With generic "bootaux/bootm" command we can point other developers
> >> > who would like to add such booting code to the already available
> >> > framework.
> >> >
> >> > Also, we would prevent other "ad-hoc" commands adding to u-boot.
> >> >
> >>
> >> Extending bootm does not seem like a good idea. bootm is already
> >> rather complex, making it even more complex by supporting the
> >> auxiliary core case seems really not work well.
> >>
> >> Also, bootm supports lots of features which you probably never would
> >> use or test on a remote core (e.g. initramfs etc...)
> >>
> >> >>
> >> >> >
> >> >> > Maybe it would better to:
> >> >> > -------------------------
> >> >> >
> >> >> > Embrace those binaries into FIT file (*.itb)? And allow multiple
> >> >> > binaries loading? I'm thinking of work similar to one already
> >> >> > did by Andre Przywara for SPL:
> >> >> >
> >> >> > "[PATCH v3 00/19] SPL: extend FIT loading support" posted on
> >> >> > 31.03.2017?
> >> >> >
> >> >> > In that way we would "open" many new use cases, and other
> >> >> > platforms (e.g. FPGA's, TI, etc) could benefit from it.
> >> >> > One solid use case is to load different Linux binaries (or/and
> >> >> > bare metal programs) to different SoC cores.
> >> >> >
> >> >> > The "meta data" (i.e. load address, data type, description),
> >> >> > could be extracted from the FIT format (the code is already in
> >> >> > u-boot).
> >> >> >
> >> >> > IMHO, this is very generic approach.
> >> >>
> >> >> I did some experiments with using FIT images for auxiliary core
> >> >> firmware, however, it did not add a lot of advantage over using
> >> >> elf:
> >> >> http://git.toradex.com/cgit/u-boot-toradex.git/commit/?h=2015.04-toradex-next&id=d1d416f272e840e8139aec911f89a70fe5523eb2
> >> >
> >> > Unfortunately, not all use cases allow using ELF format. FIT gives
> >> > more flexibility:
> >> >
> >> > -  ./doc/README.dfutftp -> different images loading
> >> >
> >> > -  Andre's patch to load multiple binaries in SPL - [PATCH v3 00/19]
> >> >   SPL: extend FIT loading support"
> >> >
> >>
> >> Are flexible, but very much U-Boot specific. And much more complex to
> >> support.
> >>
> >> >>
> >> >> Firmwares are already built and available in the elf file format.
> >> >> The Linux remoteproc framework, which is meant to handle this kind
> >> >> of cores too, supports elf firmware loading too, so supporting elf
> >> >> in U-Boot too is a nice alignment. Also using FIT adds an
> >> >> additional step when building firm wares...
> >> >
> >> > But this is all OK.
> >> >
> >> > The ELF binary would be wrapped in FIT (with e.g. "elf" property,
> >> > even 1 to 1 mapping). Then the 'bootaux/bootm' could parse ELF
> >> > (which is also generic). And then some "IMX specific" (arch/SoC)
> >> > code would be executed.
> >> >
> >>
> >> So we go from a nacked binary loaded directly to the place it has been
> >> linked to, to a double wrapped firmware file...?
> > 
> > We would have elf binary file embedded into FIT file, these would bring
> > flexibility.
> > 
> > FIT support is in place (u-boot/spl). 
> > 
> > In such a way you can use any binary in any format.
> > 
> > But I must admit that we are going off-topic here.....
> > 
> >>
> >> Not sure if user will appriciate that extra work and boot time. And
> >> developers the extra code.
> >>
> >> Maybe in a perfect world that just works, because you know, FIT is
> >> generic, and elf is generic...  But that is just not how it works in
> >> practice. The existing FIT code is rather complex, supports lots of
> >> corner cases. The elf code supports different header types... Loading
> >> the elf sections (which use M4 specific addressing) needs address
> >> translation to get to suitable host addresses...
> > 
> > Maybe I'm an idealist :-)
> > 
> > One analogy comes to my mind between linux and u-boot.
> > 
> > The proliferation of u-boot commands and linux board files. Why Linux
> > spend tremendous resources to remove board files and switch to device
> > tree?
> > 
> 
> I argue that is the right way of doing it: Do ad-hoc solutions, whatever
> makes sense, and once you understand the full breath of design space it
> is much easier to build a suitable framework. At that point, do the
> refactoring and build that suitable framework.
> 
> If you design a framework without the understanding of the whole design
> space, you will end up with something which does not work well and needs
> lots of work-arounds etc...
> 
> Of course, it is a bit different since both examples have outside facing
> impact (change to device tree as well as changes to U-Boot commands).

To ask the silly question, isn't cmd/remoteproc.c part of the proper
framework for a solution here?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170405/ffde4016/attachment.sig>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support
  2017-04-05 19:10             ` Tom Rini
@ 2017-04-05 19:54               ` Lukasz Majewski
  2017-04-05 19:56               ` Stefan Agner
  1 sibling, 0 replies; 25+ messages in thread
From: Lukasz Majewski @ 2017-04-05 19:54 UTC (permalink / raw)
  To: u-boot

On Wed, 5 Apr 2017 15:10:23 -0400
Tom Rini <trini@konsulko.com> wrote:

> On Wed, Apr 05, 2017 at 11:20:43AM -0700, Stefan Agner wrote:
> > On 2017-04-05 08:15, Lukasz Majewski wrote:
> > > Hi Stefan,
> > > 
> > >> On 2017-04-04 01:23, Lukasz Majewski wrote:
> > >> > Hi Stefan,
> > >> >
> > >> >> Hi Lukasz,
> > >> >>
> > >> >> On 2017-04-03 04:20, Lukasz Majewski wrote:
> > >> >> > Hi Stefan,
> > >> >> >
> > >> >> > Thanks for your patch. Please allow me to share some ideas
> > >> >> > for improvements.
> > >> >> >
> > >> >> >> From: Stefan Agner <stefan.agner@toradex.com>
> > >> >> >>
> > >> >> >> This patchset enables to boot elf binaries on secondary
> > >> >> >> Cortex-M class cores available on i.MX 6SoloX/7Solo/7Dual.
> > >> >> >> This makes handling and loading firmwares much more
> > >> >> >> convinient since all information where the firmware has to
> > >> >> >> be loaded to is contained in the elf headers. A typical
> > >> >> >> usage looks like this:
> > >> >> >>
> > >> >> >>   Colibri iMX7 # tftp ${loadaddr} firmware.elf && bootaux
> > >> >> >> ${loadaddr} Using FEC0 device
> > >> >> >>   TFTP from server 192.168.10.1; our IP address is
> > >> >> >> 192.168.10.2 Filename 'firmware.elf'.
> > >> >> >>   Load address: 0x80800000
> > >> >> >>   Loading:
> > >> >> >> ################################################## 88.3
> > >> >> >> KiB 5.4 MiB/s done
> > >> >> >>   Bytes transferred = 90424 (16138 hex)
> > >> >> >>   ## Starting auxiliary core at 0x1FFF8311 ...
> > >> >> >>   Colibri iMX7 #
> > >> >> >
> > >> >> > I can find some other platforms (not only IMX), which would
> > >> >> > benefit from this code - the generic 'bootaux' command.
> > >> >> >
> > >> >> > One good example would to allow multiple binaries for
> > >> >> > different SoC Cores (e.g. 2x Cortex-A8) to be loaded and
> > >> >> > started by u-boot.
> > >> >> >
> > >> >> > Hence, I'm wondering if you could make those patches usable
> > >> >> > for other platforms as well?
> > >> >>
> > >> >> I don't think that this is a good idea. bootaux is meant for
> > >> >> auxiliary cores, which often use a different architecture and
> > >> >> are not cache coherent (hence the cache flushes).
> > >> >
> > >> > I do remember, that I saw similar "tiny" patch to add "just
> > >> > one" ad-hoc command to do the same (conceptually) for TI SoC
> > >> > floating on the u-boot mailing list.
> > >> >
> > >> > Please correct me if I'm wrong, but bootaux is IMX specific
> > >> > and does work, which very likely, will be also required by
> > >> > other SoC vendors (TI's Sitara is also equipped with Cortex-M4
> > >> > and PRUSS).
> > >>
> > >> bootaux is currently IMX specific, and its currently supported
> > >> binary format is M-class specific.
> > >>
> > >> >
> > >> > Unification of such effort can save us all a lot of trouble in
> > >> > a very near future.
> > >>
> > >> In OSS, you do not develop for the future. It gets developed
> > >> when its here.
> > > 
> > > I cannot agree here. When you perceive threat then you prepare
> > > for it.
> > > 
> > >>
> > >> >
> > >> >>
> > >> >> On SMP systems the main operating system normally starts the
> > >> >> secondary core.
> > >> >
> > >> > I think that there are some conceptual similarities between
> > >> > loading code to SMP core and Cortex M3. Of course some
> > >> > "tweaks" would be needed.
> > >> >
> > >>
> > >> There are conceptual similarities between a car and a truck,
> > >> still, they are likely manufactured in a different assembly
> > >> line, probably by a different producer.
> > >>
> > >> I guess in the end it really depends on where you draw the line:
> > >> There are differences between loading code which will get
> > >> executed by the primary code, loading code to execute explicitly
> > >> on a SMP core, and loading code to execute on a remote processor.
> > >>
> > >> The first case is already well supported, and we need to keep
> > >> support backward compatibility.
> > >>
> > >> The second case, IMHO, is a somewhat silly case: A SMP system
> > >> usually gets driven by a single OS image, and that OS makes sure
> > >> to initialize the cores (maybe with the help of firmware, such
> > >> as the PSCI interface on ARM). There is no need for a boot
> > >> loader command to execute a image on a secondary core
> > >> explicitly...
> > > 
> > > I do understand (and agree) with your point with SMP.
> > > 
> > > But, I do know at least one use case when somebody would like to
> > > start two binaries (those are bare metal programs, taking care of
> > > SoC setup, IPC, etc).
> > > 
> > > And maybe Linux with some FPGA based IP block already configured
> > > in u-boot.
> > > 
> > >>
> > >> The third case is probably a case where we could start think
> > >> about unification efforts.
> > >>
> > >>
> > >> >> Otherwise, if you want to run them separately using U-Boot,
> > >> >> maybe a new command such as bootsmp would be more suited.
> > >> >
> > >> > Hmm - I do think that it would be too much:
> > >> >
> > >> > - bootm for generic single core
> > >> > - bootaux for IMX
> > >> > - bootsmp for SMP (on IMX/TI/RK?)
> > >> > - boot?? for ??
> > >>
> > >> There is at least also bootz and bootelf.
> > > 
> > > I will reply to the other thread regarding bootelf.
> > > 
> > >>
> > >> >
> > >> > I would like to avoid adding new commands for doing
> > >> > conceptually the same work.
> > >> >
> > >> > Even better, we could extend bootm to support the "multi
> > >> > binary" case too, but IMHO it would be also correct to have
> > >> > "bootaux" to handle generic binaries loading.
> > >> >
> > >> >>
> > >> >> >
> > >> >> >>
> > >> >> >> Note that the bootaux command retrieved the entry point
> > >> >> >> (PC) from the elf binary.
> > >> >> >
> > >> >> > Could you make this code flexible enough to handle not only
> > >> >> > elf binaries, but also other data (e.g. FPGA bitstreams)?
> > >> >>
> > >> >> The code of bootaux is rather small, I don't see much value
> > >> >> into stuff boot code for other peripherals into it.
> > >> >
> > >> > But I'm not asking to write support for other vendor's SoC/use
> > >> > cases.
> > >> >
> > >> > I'm just wondering if you could write generic command
> > >> > (framework) to support this use case and as an example add
> > >> > support for your IMX's Cortex-M3/4.
> > >> >
> > >>
> > >> Sure, mv arch/arm/imx-common/imx_bootaux.c cmd/, there is the
> > >> framework :-)
> > >>
> > >> But this promotes the M class specific binary format to a generic
> > >> format supported for all cores.
> > >>
> > >> > We would kill two birds with one stone - IMX is supported from
> > >> > the very beginning and we do have generic code to extend it by
> > >> > other vendors.
> > >> >
> > >> >> I don't know how FPGA
> > >> >> bistream loading typically works, but I guess it is quite
> > >> >> different from loading a firmware into RAM/SRAM and flush
> > >> >> caches...
> > >> >
> > >> > FPGA quirks would be handled in arch/SoC specific code.
> > >> >
> > >> >>
> > >> >> I am not against reuse and unification, I just feel that this
> > >> >> is not really a case where we gain much.
> > >> >
> > >> > With generic "bootaux/bootm" command we can point other
> > >> > developers who would like to add such booting code to the
> > >> > already available framework.
> > >> >
> > >> > Also, we would prevent other "ad-hoc" commands adding to
> > >> > u-boot.
> > >> >
> > >>
> > >> Extending bootm does not seem like a good idea. bootm is already
> > >> rather complex, making it even more complex by supporting the
> > >> auxiliary core case seems really not work well.
> > >>
> > >> Also, bootm supports lots of features which you probably never
> > >> would use or test on a remote core (e.g. initramfs etc...)
> > >>
> > >> >>
> > >> >> >
> > >> >> > Maybe it would better to:
> > >> >> > -------------------------
> > >> >> >
> > >> >> > Embrace those binaries into FIT file (*.itb)? And allow
> > >> >> > multiple binaries loading? I'm thinking of work similar to
> > >> >> > one already did by Andre Przywara for SPL:
> > >> >> >
> > >> >> > "[PATCH v3 00/19] SPL: extend FIT loading support" posted on
> > >> >> > 31.03.2017?
> > >> >> >
> > >> >> > In that way we would "open" many new use cases, and other
> > >> >> > platforms (e.g. FPGA's, TI, etc) could benefit from it.
> > >> >> > One solid use case is to load different Linux binaries
> > >> >> > (or/and bare metal programs) to different SoC cores.
> > >> >> >
> > >> >> > The "meta data" (i.e. load address, data type, description),
> > >> >> > could be extracted from the FIT format (the code is already
> > >> >> > in u-boot).
> > >> >> >
> > >> >> > IMHO, this is very generic approach.
> > >> >>
> > >> >> I did some experiments with using FIT images for auxiliary
> > >> >> core firmware, however, it did not add a lot of advantage
> > >> >> over using elf:
> > >> >> http://git.toradex.com/cgit/u-boot-toradex.git/commit/?h=2015.04-toradex-next&id=d1d416f272e840e8139aec911f89a70fe5523eb2
> > >> >
> > >> > Unfortunately, not all use cases allow using ELF format. FIT
> > >> > gives more flexibility:
> > >> >
> > >> > -  ./doc/README.dfutftp -> different images loading
> > >> >
> > >> > -  Andre's patch to load multiple binaries in SPL - [PATCH v3
> > >> > 00/19] SPL: extend FIT loading support"
> > >> >
> > >>
> > >> Are flexible, but very much U-Boot specific. And much more
> > >> complex to support.
> > >>
> > >> >>
> > >> >> Firmwares are already built and available in the elf file
> > >> >> format. The Linux remoteproc framework, which is meant to
> > >> >> handle this kind of cores too, supports elf firmware loading
> > >> >> too, so supporting elf in U-Boot too is a nice alignment.
> > >> >> Also using FIT adds an additional step when building firm
> > >> >> wares...
> > >> >
> > >> > But this is all OK.
> > >> >
> > >> > The ELF binary would be wrapped in FIT (with e.g. "elf"
> > >> > property, even 1 to 1 mapping). Then the 'bootaux/bootm' could
> > >> > parse ELF (which is also generic). And then some "IMX
> > >> > specific" (arch/SoC) code would be executed.
> > >> >
> > >>
> > >> So we go from a nacked binary loaded directly to the place it
> > >> has been linked to, to a double wrapped firmware file...?
> > > 
> > > We would have elf binary file embedded into FIT file, these would
> > > bring flexibility.
> > > 
> > > FIT support is in place (u-boot/spl). 
> > > 
> > > In such a way you can use any binary in any format.
> > > 
> > > But I must admit that we are going off-topic here.....
> > > 
> > >>
> > >> Not sure if user will appriciate that extra work and boot time.
> > >> And developers the extra code.
> > >>
> > >> Maybe in a perfect world that just works, because you know, FIT
> > >> is generic, and elf is generic...  But that is just not how it
> > >> works in practice. The existing FIT code is rather complex,
> > >> supports lots of corner cases. The elf code supports different
> > >> header types... Loading the elf sections (which use M4 specific
> > >> addressing) needs address translation to get to suitable host
> > >> addresses...
> > > 
> > > Maybe I'm an idealist :-)
> > > 
> > > One analogy comes to my mind between linux and u-boot.
> > > 
> > > The proliferation of u-boot commands and linux board files. Why
> > > Linux spend tremendous resources to remove board files and switch
> > > to device tree?
> > > 
> > 
> > I argue that is the right way of doing it: Do ad-hoc solutions,
> > whatever makes sense, and once you understand the full breath of
> > design space it is much easier to build a suitable framework. At
> > that point, do the refactoring and build that suitable framework.
> > 
> > If you design a framework without the understanding of the whole
> > design space, you will end up with something which does not work
> > well and needs lots of work-arounds etc...
> > 
> > Of course, it is a bit different since both examples have outside
> > facing impact (change to device tree as well as changes to U-Boot
> > commands).
> 
> To ask the silly question, isn't cmd/remoteproc.c part of the proper
> framework for a solution here?
> 

Yes, this seems like a solution :-). Thanks for pointing out.


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170405/47b11407/attachment.sig>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support
  2017-04-05 19:10             ` Tom Rini
  2017-04-05 19:54               ` Lukasz Majewski
@ 2017-04-05 19:56               ` Stefan Agner
  1 sibling, 0 replies; 25+ messages in thread
From: Stefan Agner @ 2017-04-05 19:56 UTC (permalink / raw)
  To: u-boot

On 2017-04-05 12:10, Tom Rini wrote:
> On Wed, Apr 05, 2017 at 11:20:43AM -0700, Stefan Agner wrote:
>> On 2017-04-05 08:15, Lukasz Majewski wrote:
>> > Hi Stefan,
>> >
>> >> On 2017-04-04 01:23, Lukasz Majewski wrote:
>> >> > Hi Stefan,
>> >> >
>> >> >> Hi Lukasz,
>> >> >>
>> >> >> On 2017-04-03 04:20, Lukasz Majewski wrote:
>> >> >> > Hi Stefan,
>> >> >> >
>> >> >> > Thanks for your patch. Please allow me to share some ideas for
>> >> >> > improvements.
>> >> >> >
>> >> >> >> From: Stefan Agner <stefan.agner@toradex.com>
>> >> >> >>
>> >> >> >> This patchset enables to boot elf binaries on secondary Cortex-M
>> >> >> >> class cores available on i.MX 6SoloX/7Solo/7Dual. This makes
>> >> >> >> handling and loading firmwares much more convinient since all
>> >> >> >> information where the firmware has to be loaded to is contained
>> >> >> >> in the elf headers. A typical usage looks like this:
>> >> >> >>
>> >> >> >>   Colibri iMX7 # tftp ${loadaddr} firmware.elf && bootaux
>> >> >> >> ${loadaddr} Using FEC0 device
>> >> >> >>   TFTP from server 192.168.10.1; our IP address is 192.168.10.2
>> >> >> >>   Filename 'firmware.elf'.
>> >> >> >>   Load address: 0x80800000
>> >> >> >>   Loading: ##################################################
>> >> >> >> 88.3 KiB 5.4 MiB/s
>> >> >> >>   done
>> >> >> >>   Bytes transferred = 90424 (16138 hex)
>> >> >> >>   ## Starting auxiliary core at 0x1FFF8311 ...
>> >> >> >>   Colibri iMX7 #
>> >> >> >
>> >> >> > I can find some other platforms (not only IMX), which would
>> >> >> > benefit from this code - the generic 'bootaux' command.
>> >> >> >
>> >> >> > One good example would to allow multiple binaries for different
>> >> >> > SoC Cores (e.g. 2x Cortex-A8) to be loaded and started by u-boot.
>> >> >> >
>> >> >> > Hence, I'm wondering if you could make those patches usable for
>> >> >> > other platforms as well?
>> >> >>
>> >> >> I don't think that this is a good idea. bootaux is meant for
>> >> >> auxiliary cores, which often use a different architecture and are
>> >> >> not cache coherent (hence the cache flushes).
>> >> >
>> >> > I do remember, that I saw similar "tiny" patch to add "just one"
>> >> > ad-hoc command to do the same (conceptually) for TI SoC floating on
>> >> > the u-boot mailing list.
>> >> >
>> >> > Please correct me if I'm wrong, but bootaux is IMX specific and does
>> >> > work, which very likely, will be also required by other SoC vendors
>> >> > (TI's Sitara is also equipped with Cortex-M4 and PRUSS).
>> >>
>> >> bootaux is currently IMX specific, and its currently supported binary
>> >> format is M-class specific.
>> >>
>> >> >
>> >> > Unification of such effort can save us all a lot of trouble in a
>> >> > very near future.
>> >>
>> >> In OSS, you do not develop for the future. It gets developed when its
>> >> here.
>> >
>> > I cannot agree here. When you perceive threat then you prepare for it.
>> >
>> >>
>> >> >
>> >> >>
>> >> >> On SMP systems the main operating system normally starts the
>> >> >> secondary core.
>> >> >
>> >> > I think that there are some conceptual similarities between loading
>> >> > code to SMP core and Cortex M3. Of course some "tweaks" would be
>> >> > needed.
>> >> >
>> >>
>> >> There are conceptual similarities between a car and a truck, still,
>> >> they are likely manufactured in a different assembly line, probably
>> >> by a different producer.
>> >>
>> >> I guess in the end it really depends on where you draw the line: There
>> >> are differences between loading code which will get executed by the
>> >> primary code, loading code to execute explicitly on a SMP core, and
>> >> loading code to execute on a remote processor.
>> >>
>> >> The first case is already well supported, and we need to keep support
>> >> backward compatibility.
>> >>
>> >> The second case, IMHO, is a somewhat silly case: A SMP system usually
>> >> gets driven by a single OS image, and that OS makes sure to initialize
>> >> the cores (maybe with the help of firmware, such as the PSCI interface
>> >> on ARM). There is no need for a boot loader command to execute a image
>> >> on a secondary core explicitly...
>> >
>> > I do understand (and agree) with your point with SMP.
>> >
>> > But, I do know at least one use case when somebody would like to start
>> > two binaries (those are bare metal programs, taking care of SoC setup,
>> > IPC, etc).
>> >
>> > And maybe Linux with some FPGA based IP block already configured in
>> > u-boot.
>> >
>> >>
>> >> The third case is probably a case where we could start think about
>> >> unification efforts.
>> >>
>> >>
>> >> >> Otherwise, if you want to run them separately using U-Boot,
>> >> >> maybe a new command such as bootsmp would be more suited.
>> >> >
>> >> > Hmm - I do think that it would be too much:
>> >> >
>> >> > - bootm for generic single core
>> >> > - bootaux for IMX
>> >> > - bootsmp for SMP (on IMX/TI/RK?)
>> >> > - boot?? for ??
>> >>
>> >> There is at least also bootz and bootelf.
>> >
>> > I will reply to the other thread regarding bootelf.
>> >
>> >>
>> >> >
>> >> > I would like to avoid adding new commands for doing conceptually the
>> >> > same work.
>> >> >
>> >> > Even better, we could extend bootm to support the "multi binary"
>> >> > case too, but IMHO it would be also correct to have "bootaux" to
>> >> > handle generic binaries loading.
>> >> >
>> >> >>
>> >> >> >
>> >> >> >>
>> >> >> >> Note that the bootaux command retrieved the entry point (PC)
>> >> >> >> from the elf binary.
>> >> >> >
>> >> >> > Could you make this code flexible enough to handle not only elf
>> >> >> > binaries, but also other data (e.g. FPGA bitstreams)?
>> >> >>
>> >> >> The code of bootaux is rather small, I don't see much value into
>> >> >> stuff boot code for other peripherals into it.
>> >> >
>> >> > But I'm not asking to write support for other vendor's SoC/use
>> >> > cases.
>> >> >
>> >> > I'm just wondering if you could write generic command (framework) to
>> >> > support this use case and as an example add support for your IMX's
>> >> > Cortex-M3/4.
>> >> >
>> >>
>> >> Sure, mv arch/arm/imx-common/imx_bootaux.c cmd/, there is the
>> >> framework :-)
>> >>
>> >> But this promotes the M class specific binary format to a generic
>> >> format supported for all cores.
>> >>
>> >> > We would kill two birds with one stone - IMX is supported from the
>> >> > very beginning and we do have generic code to extend it by other
>> >> > vendors.
>> >> >
>> >> >> I don't know how FPGA
>> >> >> bistream loading typically works, but I guess it is quite different
>> >> >> from loading a firmware into RAM/SRAM and flush caches...
>> >> >
>> >> > FPGA quirks would be handled in arch/SoC specific code.
>> >> >
>> >> >>
>> >> >> I am not against reuse and unification, I just feel that this is
>> >> >> not really a case where we gain much.
>> >> >
>> >> > With generic "bootaux/bootm" command we can point other developers
>> >> > who would like to add such booting code to the already available
>> >> > framework.
>> >> >
>> >> > Also, we would prevent other "ad-hoc" commands adding to u-boot.
>> >> >
>> >>
>> >> Extending bootm does not seem like a good idea. bootm is already
>> >> rather complex, making it even more complex by supporting the
>> >> auxiliary core case seems really not work well.
>> >>
>> >> Also, bootm supports lots of features which you probably never would
>> >> use or test on a remote core (e.g. initramfs etc...)
>> >>
>> >> >>
>> >> >> >
>> >> >> > Maybe it would better to:
>> >> >> > -------------------------
>> >> >> >
>> >> >> > Embrace those binaries into FIT file (*.itb)? And allow multiple
>> >> >> > binaries loading? I'm thinking of work similar to one already
>> >> >> > did by Andre Przywara for SPL:
>> >> >> >
>> >> >> > "[PATCH v3 00/19] SPL: extend FIT loading support" posted on
>> >> >> > 31.03.2017?
>> >> >> >
>> >> >> > In that way we would "open" many new use cases, and other
>> >> >> > platforms (e.g. FPGA's, TI, etc) could benefit from it.
>> >> >> > One solid use case is to load different Linux binaries (or/and
>> >> >> > bare metal programs) to different SoC cores.
>> >> >> >
>> >> >> > The "meta data" (i.e. load address, data type, description),
>> >> >> > could be extracted from the FIT format (the code is already in
>> >> >> > u-boot).
>> >> >> >
>> >> >> > IMHO, this is very generic approach.
>> >> >>
>> >> >> I did some experiments with using FIT images for auxiliary core
>> >> >> firmware, however, it did not add a lot of advantage over using
>> >> >> elf:
>> >> >> http://git.toradex.com/cgit/u-boot-toradex.git/commit/?h=2015.04-toradex-next&id=d1d416f272e840e8139aec911f89a70fe5523eb2
>> >> >
>> >> > Unfortunately, not all use cases allow using ELF format. FIT gives
>> >> > more flexibility:
>> >> >
>> >> > -  ./doc/README.dfutftp -> different images loading
>> >> >
>> >> > -  Andre's patch to load multiple binaries in SPL - [PATCH v3 00/19]
>> >> >   SPL: extend FIT loading support"
>> >> >
>> >>
>> >> Are flexible, but very much U-Boot specific. And much more complex to
>> >> support.
>> >>
>> >> >>
>> >> >> Firmwares are already built and available in the elf file format.
>> >> >> The Linux remoteproc framework, which is meant to handle this kind
>> >> >> of cores too, supports elf firmware loading too, so supporting elf
>> >> >> in U-Boot too is a nice alignment. Also using FIT adds an
>> >> >> additional step when building firm wares...
>> >> >
>> >> > But this is all OK.
>> >> >
>> >> > The ELF binary would be wrapped in FIT (with e.g. "elf" property,
>> >> > even 1 to 1 mapping). Then the 'bootaux/bootm' could parse ELF
>> >> > (which is also generic). And then some "IMX specific" (arch/SoC)
>> >> > code would be executed.
>> >> >
>> >>
>> >> So we go from a nacked binary loaded directly to the place it has been
>> >> linked to, to a double wrapped firmware file...?
>> >
>> > We would have elf binary file embedded into FIT file, these would bring
>> > flexibility.
>> >
>> > FIT support is in place (u-boot/spl).
>> >
>> > In such a way you can use any binary in any format.
>> >
>> > But I must admit that we are going off-topic here.....
>> >
>> >>
>> >> Not sure if user will appriciate that extra work and boot time. And
>> >> developers the extra code.
>> >>
>> >> Maybe in a perfect world that just works, because you know, FIT is
>> >> generic, and elf is generic...  But that is just not how it works in
>> >> practice. The existing FIT code is rather complex, supports lots of
>> >> corner cases. The elf code supports different header types... Loading
>> >> the elf sections (which use M4 specific addressing) needs address
>> >> translation to get to suitable host addresses...
>> >
>> > Maybe I'm an idealist :-)
>> >
>> > One analogy comes to my mind between linux and u-boot.
>> >
>> > The proliferation of u-boot commands and linux board files. Why Linux
>> > spend tremendous resources to remove board files and switch to device
>> > tree?
>> >
>>
>> I argue that is the right way of doing it: Do ad-hoc solutions, whatever
>> makes sense, and once you understand the full breath of design space it
>> is much easier to build a suitable framework. At that point, do the
>> refactoring and build that suitable framework.
>>
>> If you design a framework without the understanding of the whole design
>> space, you will end up with something which does not work well and needs
>> lots of work-arounds etc...
>>
>> Of course, it is a bit different since both examples have outside facing
>> impact (change to device tree as well as changes to U-Boot commands).
> 
> To ask the silly question, isn't cmd/remoteproc.c part of the proper
> framework for a solution here?

Oh wow, wasn't aware of that.

Interestingly, this even got merged before bootaux! (remoteproc has been
added September 2015, bootaux January 2016)...

The framework seems somewhat TI specific, e.g. there is no notion of
load address. Not sure how well that will fit with the NXP requirements,
but I will give it a shot.

--
Stefan

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2017-04-05 19:56 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-29 19:58 [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support Stefan Agner
2017-03-29 19:58 ` [U-Boot] [PATCH 1/3] imx: imx-common: move aux core image parsing to common code Stefan Agner
2017-03-29 19:58 ` [U-Boot] [PATCH 2/3] imx: imx-common: add elf firmware support Stefan Agner
2017-03-29 19:58 ` [U-Boot] [PATCH 3/3] ARM: vf610: add auxiliary core boot support Stefan Agner
2017-04-03 11:20 ` [U-Boot] [PATCH 0/3] imx: bootaux elf firmware support Lukasz Majewski
2017-04-03 21:36   ` Stefan Agner
2017-04-03 22:07     ` Marek Vasut
2017-04-03 22:42       ` Stefan Agner
2017-04-03 23:34         ` Marek Vasut
2017-04-04  0:02           ` Stefan Agner
2017-04-04  8:46             ` Lukasz Majewski
2017-04-04  9:22             ` Marek Vasut
2017-04-04 17:57               ` Stefan Agner
2017-04-04 18:38                 ` Marek Vasut
2017-04-04 19:45                   ` Stefan Agner
2017-04-04 20:17                     ` Marek Vasut
2017-04-04 21:39                       ` Stefan Agner
2017-04-04  8:25       ` Lukasz Majewski
2017-04-04  8:23     ` Lukasz Majewski
2017-04-04 18:59       ` Stefan Agner
2017-04-05 15:15         ` Lukasz Majewski
2017-04-05 18:20           ` Stefan Agner
2017-04-05 19:10             ` Tom Rini
2017-04-05 19:54               ` Lukasz Majewski
2017-04-05 19:56               ` Stefan Agner

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.