[U-Boot] [PATCH 0/5] remoteproc: add elf resource table loader

[U-Boot] [PATCH 0/5] remoteproc: add elf resource table loader

[Fabien Dessenne]

Add some helpers that can be called by the drivers to load the firmware
resource table from an elf32 / elf64 image.
The stm32 remoteproc driver makes use of it, to load the resource table before
the elf image itself.

This series applies on top of the "remoteproc: Add support for R5F and DSP
processors" series [1] proposed by Lokesh Vutla which introduces the elf64
support.

[1]: https://patchwork.ozlabs.org/project/uboot/list/?series=128946

Fabien Dessenne (5):
  remoteproc: elf_loader: Add elf resource table load support
  stm32mp1: declare backup register for copro resource table address
  remoteproc: stm32: load resource table from firmware
  stm32mp1: Fixup the Linux DeviceTree with coprocessor information
  remoteproc: stm32: invert the is_running() return value

 arch/arm/mach-stm32mp/include/mach/stm32.h |   1 +
 board/st/stm32mp1/stm32mp1.c               |  16 +-
 drivers/remoteproc/rproc-elf-loader.c      | 269 +++++++++++++++++++++++++++++
 drivers/remoteproc/stm32_copro.c           |  17 +-
 include/remoteproc.h                       |  70 ++++++++
 test/dm/remoteproc.c                       |  91 ++++++++--
 6 files changed, 448 insertions(+), 16 deletions(-)

-- 
2.7.4

[U-Boot] [PATCH 1/5] remoteproc: elf_loader: Add elf resource table load support

[Fabien Dessenne]

Add rproc_elf_load_rsc_table(), which searches for a resource table in
an elf64/elf32 image, and if found, copies it to device memory.
Add also the elf32 and elf64 variants of this API.
Add a test for this.

Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
---
 drivers/remoteproc/rproc-elf-loader.c | 269 ++++++++++++++++++++++++++++++++++
 include/remoteproc.h                  |  70 +++++++++
 test/dm/remoteproc.c                  |  91 ++++++++++--
 3 files changed, 419 insertions(+), 11 deletions(-)

diff --git a/drivers/remoteproc/rproc-elf-loader.c b/drivers/remoteproc/rproc-elf-loader.c
index b38a226..9127ea5 100644
--- a/drivers/remoteproc/rproc-elf-loader.c
+++ b/drivers/remoteproc/rproc-elf-loader.c
@@ -7,6 +7,39 @@
 #include <elf.h>
 #include <remoteproc.h>
 
+/**
+ * struct resource_table - firmware resource table header
+ * @ver: version number
+ * @num: number of resource entries
+ * @reserved: reserved (must be zero)
+ * @offset: array of offsets pointing at the various resource entries
+ *
+ * A resource table is essentially a list of system resources required
+ * by the remote processor. It may also include configuration entries.
+ * If needed, the remote processor firmware should contain this table
+ * as a dedicated ".resource_table" ELF section.
+ *
+ * Some resources entries are mere announcements, where the host is informed
+ * of specific remoteproc configuration. Other entries require the host to
+ * do something (e.g. allocate a system resource). Sometimes a negotiation
+ * is expected, where the firmware requests a resource, and once allocated,
+ * the host should provide back its details (e.g. address of an allocated
+ * memory region).
+ *
+ * The header of the resource table, as expressed by this structure,
+ * contains a version number (should we need to change this format in the
+ * future), the number of available resource entries, and their offsets
+ * in the table.
+ *
+ * Immediately following this header are the resource entries themselves.
+ */
+struct resource_table {
+	u32 ver;
+	u32 num;
+	u32 reserved[2];
+	u32 offset[0];
+} __packed;
+
 /* Basic function to verify ELF32 image format */
 int rproc_elf32_sanity_check(ulong addr, ulong size)
 {
@@ -275,3 +308,239 @@ ulong rproc_elf_get_boot_addr(struct udevice *dev, ulong addr)
 	else
 		return rproc_elf32_get_boot_addr(addr);
 }
+
+/*
+ * Search for the resource table in an ELF32 image.
+ * Returns the address of the resource table section if found, NULL if there is
+ * no resource table section, or error pointer.
+ */
+static Elf32_Shdr *rproc_elf32_find_rsc_table(struct udevice *dev,
+					      ulong fw_addr, ulong fw_size)
+{
+	int ret;
+	unsigned int i;
+	const char *name_table;
+	struct resource_table *table;
+	const u8 *elf_data = (void *)fw_addr;
+	Elf32_Ehdr *ehdr = (Elf32_Ehdr *)fw_addr;
+	Elf32_Shdr *shdr;
+
+	ret = rproc_elf32_sanity_check(fw_addr, fw_size);
+	if (ret) {
+		pr_debug("Invalid ELF32 Image %d\n", ret);
+		return ERR_PTR(ret);
+	}
+
+	/* look for the resource table and handle it */
+	shdr = (Elf32_Shdr *)(elf_data + ehdr->e_shoff);
+	name_table = (const char *)(elf_data +
+				    shdr[ehdr->e_shstrndx].sh_offset);
+
+	for (i = 0; i < ehdr->e_shnum; i++, shdr++) {
+		u32 size = shdr->sh_size;
+		u32 offset = shdr->sh_offset;
+
+		if (strcmp(name_table + shdr->sh_name, ".resource_table"))
+			continue;
+
+		table = (struct resource_table *)(elf_data + offset);
+
+		/* make sure we have the entire table */
+		if (offset + size > fw_size) {
+			pr_debug("resource table truncated\n");
+			return ERR_PTR(-ENOSPC);
+		}
+
+		/* make sure table has at least the header */
+		if (sizeof(*table) > size) {
+			pr_debug("header-less resource table\n");
+			return ERR_PTR(-ENOSPC);
+		}
+
+		/* we don't support any version beyond the first */
+		if (table->ver != 1) {
+			pr_debug("unsupported fw ver: %d\n", table->ver);
+			return ERR_PTR(-EPROTONOSUPPORT);
+		}
+
+		/* make sure reserved bytes are zeroes */
+		if (table->reserved[0] || table->reserved[1]) {
+			pr_debug("non zero reserved bytes\n");
+			return ERR_PTR(-EBADF);
+		}
+
+		/* make sure the offsets array isn't truncated */
+		if (table->num * sizeof(table->offset[0]) +
+				 sizeof(*table) > size) {
+			pr_debug("resource table incomplete\n");
+			return ERR_PTR(-ENOSPC);
+		}
+
+		return shdr;
+	}
+
+	return NULL;
+}
+
+/* Load the resource table from an ELF32 image */
+int rproc_elf32_load_rsc_table(struct udevice *dev, ulong fw_addr,
+			       ulong fw_size, ulong *rsc_addr, ulong *rsc_size)
+{
+	const struct dm_rproc_ops *ops;
+	Elf32_Shdr *shdr;
+	void *src, *dst;
+
+	shdr = rproc_elf32_find_rsc_table(dev, fw_addr, fw_size);
+	if (!shdr)
+		return -ENODATA;
+	if (IS_ERR(shdr))
+		return PTR_ERR(shdr);
+
+	ops = rproc_get_ops(dev);
+	*rsc_addr = (ulong)shdr->sh_addr;
+	*rsc_size = (ulong)shdr->sh_size;
+
+	src = (void *)fw_addr + shdr->sh_offset;
+	if (ops->device_to_virt)
+		dst = (void *)ops->device_to_virt(dev, *rsc_addr, *rsc_size);
+	else
+		dst = (void *)rsc_addr;
+
+	dev_dbg(dev, "Loading resource table to 0x%8lx (%ld bytes)\n",
+		(ulong)dst, *rsc_size);
+
+	memcpy(dst, src, *rsc_size);
+	flush_cache(rounddown((unsigned long)dst, ARCH_DMA_MINALIGN),
+		    roundup((unsigned long)dst + *rsc_size,
+			    ARCH_DMA_MINALIGN) -
+		    rounddown((unsigned long)dst, ARCH_DMA_MINALIGN));
+
+	return 0;
+}
+
+/*
+ * Search for the resource table in an ELF64 image.
+ * Returns the address of the resource table section if found, NULL if there is
+ * no resource table section, or error pointer.
+ */
+static Elf64_Shdr *rproc_elf64_find_rsc_table(struct udevice *dev,
+					      ulong fw_addr, ulong fw_size)
+{
+	int ret;
+	unsigned int i;
+	const char *name_table;
+	struct resource_table *table;
+	const u8 *elf_data = (void *)fw_addr;
+	Elf64_Ehdr *ehdr = (Elf64_Ehdr *)fw_addr;
+	Elf64_Shdr *shdr;
+
+	ret = rproc_elf64_sanity_check(fw_addr, fw_size);
+	if (ret) {
+		pr_debug("Invalid ELF64 Image %d\n", ret);
+		return ERR_PTR(ret);
+	}
+
+	/* look for the resource table and handle it */
+	shdr = (Elf64_Shdr *)(elf_data + ehdr->e_shoff);
+	name_table = (const char *)(elf_data +
+				    shdr[ehdr->e_shstrndx].sh_offset);
+
+	for (i = 0; i < ehdr->e_shnum; i++, shdr++) {
+		u64 size = shdr->sh_size;
+		u64 offset = shdr->sh_offset;
+
+		if (strcmp(name_table + shdr->sh_name, ".resource_table"))
+			continue;
+
+		table = (struct resource_table *)(elf_data + offset);
+
+		/* make sure we have the entire table */
+		if (offset + size > fw_size) {
+			pr_debug("resource table truncated\n");
+			return ERR_PTR(-ENOSPC);
+		}
+
+		/* make sure table has at least the header */
+		if (sizeof(*table) > size) {
+			pr_debug("header-less resource table\n");
+			return ERR_PTR(-ENOSPC);
+		}
+
+		/* we don't support any version beyond the first */
+		if (table->ver != 1) {
+			pr_debug("unsupported fw ver: %d\n", table->ver);
+			return ERR_PTR(-EPROTONOSUPPORT);
+		}
+
+		/* make sure reserved bytes are zeroes */
+		if (table->reserved[0] || table->reserved[1]) {
+			pr_debug("non zero reserved bytes\n");
+			return ERR_PTR(-EBADF);
+		}
+
+		/* make sure the offsets array isn't truncated */
+		if (table->num * sizeof(table->offset[0]) +
+				 sizeof(*table) > size) {
+			pr_debug("resource table incomplete\n");
+			return ERR_PTR(-ENOSPC);
+		}
+
+		return shdr;
+	}
+
+	return NULL;
+}
+
+/* Load the resource table from an ELF64 image */
+int rproc_elf64_load_rsc_table(struct udevice *dev, ulong fw_addr,
+			       ulong fw_size, ulong *rsc_addr, ulong *rsc_size)
+{
+	const struct dm_rproc_ops *ops;
+	Elf64_Shdr *shdr;
+	void *src, *dst;
+
+	shdr = rproc_elf64_find_rsc_table(dev, fw_addr, fw_size);
+	if (!shdr)
+		return -ENODATA;
+	if (IS_ERR(shdr))
+		return PTR_ERR(shdr);
+
+	ops = rproc_get_ops(dev);
+	*rsc_addr = (ulong)shdr->sh_addr;
+	*rsc_size = (ulong)shdr->sh_size;
+
+	src = (void *)fw_addr + shdr->sh_offset;
+	if (ops->device_to_virt)
+		dst = (void *)ops->device_to_virt(dev, *rsc_addr, *rsc_size);
+	else
+		dst = (void *)rsc_addr;
+
+	dev_dbg(dev, "Loading resource table to 0x%8lx (%ld bytes)\n",
+		(ulong)dst, *rsc_size);
+
+	memcpy(dst, src, *rsc_size);
+	flush_cache(rounddown((unsigned long)dst, ARCH_DMA_MINALIGN),
+		    roundup((unsigned long)dst + *rsc_size,
+			    ARCH_DMA_MINALIGN) -
+		    rounddown((unsigned long)dst, ARCH_DMA_MINALIGN));
+
+	return 0;
+}
+
+/* Load the resource table from an ELF32 or ELF64 image */
+int rproc_elf_load_rsc_table(struct udevice *dev, ulong fw_addr,
+			     ulong fw_size, ulong *rsc_addr, ulong *rsc_size)
+
+{
+	Elf32_Ehdr *ehdr = (Elf32_Ehdr *)fw_addr;
+
+	if (!fw_addr)
+		return -EFAULT;
+
+	if (ehdr->e_ident[EI_CLASS] == ELFCLASS64)
+		return rproc_elf64_load_rsc_table(dev, fw_addr, fw_size,
+						  rsc_addr, rsc_size);
+	else
+		return rproc_elf32_load_rsc_table(dev, fw_addr, fw_size,
+						  rsc_addr, rsc_size);
+}
diff --git a/include/remoteproc.h b/include/remoteproc.h
index 046cd9e..a903acb 100644
--- a/include/remoteproc.h
+++ b/include/remoteproc.h
@@ -277,6 +277,64 @@ int rproc_elf_load_image(struct udevice *dev, unsigned long addr, ulong size);
  * image.
  */
 ulong rproc_elf_get_boot_addr(struct udevice *dev, ulong addr);
+
+/**
+ * rproc_elf32_load_rsc_table() - load the resource table from an ELF32 image
+ *
+ * Search for the resource table in an ELF32 image, and if found, copy it to
+ * device memory.
+ *
+ * @dev:	device loading the resource table
+ * @fw_addr:	ELF image address
+ * @fw_size:	size of the ELF image
+ * @rsc_addr:	pointer to the found resource table address. Updated on
+ *		operation success
+ * @rsc_size:	pointer to the found resource table size. Updated on operation
+ *		success
+ *
+ * @return 0 if a valid resource table is successfully loaded, -ENODATA if there
+ * is no resource table (which is optional), or another appropriate error value.
+ */
+int rproc_elf32_load_rsc_table(struct udevice *dev, ulong fw_addr,
+			       ulong fw_size, ulong *rsc_addr, ulong *rsc_size);
+/**
+ * rproc_elf64_load_rsc_table() - load the resource table from an ELF64 image
+ *
+ * Search for the resource table in an ELF64 image, and if found, copy it to
+ * device memory.
+ *
+ * @dev:	device loading the resource table
+ * @fw_addr:	ELF image address
+ * @fw_size:	size of the ELF image
+ * @rsc_addr:	pointer to the found resource table address. Updated on
+ *		operation success
+ * @rsc_size:	pointer to the found resource table size. Updated on operation
+ *		success
+ *
+ * @return 0 if a valid resource table is successfully loaded, -ENODATA if there
+ * is no resource table (which is optional), or another appropriate error value.
+ */
+int rproc_elf64_load_rsc_table(struct udevice *dev, ulong fw_addr,
+			       ulong fw_size, ulong *rsc_addr, ulong *rsc_size);
+/**
+ * rproc_elf_load_rsc_table() - load the resource table from an ELF image
+ *
+ * Auto detects if the image is ELF32 or ELF64 image and search accordingly for
+ * the resource table, and if found, copy it to device memory.
+ *
+ * @dev:	device loading the resource table
+ * @fw_addr:	ELF image address
+ * @fw_size:	size of the ELF image
+ * @rsc_addr:	pointer to the found resource table address. Updated on
+ *		operation success
+ * @rsc_size:	pointer to the found resource table size. Updated on operation
+ *		success
+ *
+ * @return 0 if a valid resource table is successfully loaded, -ENODATA if there
+ * is no resource table (which is optional), or another appropriate error value.
+ */
+int rproc_elf_load_rsc_table(struct udevice *dev, ulong fw_addr,
+			     ulong fw_size, ulong *rsc_addr, ulong *rsc_size);
 #else
 static inline int rproc_init(void) { return -ENOSYS; }
 static inline int rproc_dev_init(int id) { return -ENOSYS; }
@@ -304,6 +362,18 @@ static inline int rproc_elf_load_image(struct udevice *dev, ulong addr,
 { return -ENOSYS; }
 static inline ulong rproc_elf_get_boot_addr(struct udevice *dev, ulong addr)
 { return 0; }
+static inline int rproc_elf32_load_rsc_table(struct udevice *dev, ulong fw_addr,
+					     ulong fw_size, ulong *rsc_addr,
+					     ulong *rsc_size)
+{ return -ENOSYS; }
+static inline int rproc_elf64_load_rsc_table(struct udevice *dev, ulong fw_addr,
+					     ulong fw_size, ulong *rsc_addr,
+					     ulong *rsc_size)
+{ return -ENOSYS; }
+static inline int rproc_elf_load_rsc_table(struct udevice *dev, ulong fw_addr,
+					   ulong fw_size, ulong *rsc_addr,
+					   ulong *rsc_size)
+{ return -ENOSYS; }
 #endif
 
 #endif	/* _RPROC_H_ */
diff --git a/test/dm/remoteproc.c b/test/dm/remoteproc.c
index 1d9a9b3..4067596 100644
--- a/test/dm/remoteproc.c
+++ b/test/dm/remoteproc.c
@@ -103,8 +103,8 @@ static int dm_test_remoteproc_elf(struct unit_test_state *uts)
 		0x00, 0x00, 0x00, 0x08,
 		/* phoff (program header offset @ 0x40)*/
 		0x40, 0x00, 0x00, 0x00,
-		/* shoff (section header offset : none) */
-		0x00, 0x00, 0x00, 0x00,
+		/* shoff (section header offset @ 0x90) */
+		0x90, 0x00, 0x00, 0x00,
 		/* flags */
 		0x00, 0x00, 0x00, 0x00,
 		/* ehsize (elf header size = 0x34) */
@@ -113,16 +113,17 @@ static int dm_test_remoteproc_elf(struct unit_test_state *uts)
 		0x20, 0x00,
 		/* phnum (program header number : 1) */
 		0x01, 0x00,
-		/* shentsize (section heade size : none) */
-		0x00, 0x00,
-		/* shnum (section header number: none) */
-		0x00, 0x00,
-		/* shstrndx (section header name section index: none) */
-		0x00, 0x00,
+		/* shentsize (section header size : 40 bytes) */
+		0x28, 0x00,
+		/* shnum (section header number: 3) */
+		0x02, 0x00,
+		/* shstrndx (section header name section index: 1) */
+		0x01, 0x00,
 		/* padding */
 		0x00, 0x00, 0x00, 0x00,
 		0x00, 0x00, 0x00, 0x00,
 		0x00, 0x00, 0x00, 0x00,
+
 		/* @0x40 - PROGRAM HEADER TABLE - */
 		/* type : PT_LOAD */
 		0x01, 0x00, 0x00, 0x00,
@@ -140,14 +141,63 @@ static int dm_test_remoteproc_elf(struct unit_test_state *uts)
 		0x05, 0x00, 0x00, 0x00,
 		/* padding */
 		0x00, 0x00, 0x00, 0x00,
+
+		/* @0x60 - RESOURCE TABLE SECTION - */
+		/* version */
+		0x01, 0x00, 0x00, 0x00,
+		/* num (0, no entries) */
+		0x00, 0x00, 0x00, 0x00,
+		/* Reserved */
+		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+
+		/* @0x70 - SECTION'S NAMES SECTION - */
+		/* section 0 name (".shrtrtab") */
+		0x2e, 0x73, 0x68, 0x73, 0x74, 0x72, 0x74, 0x61, 0x62, 0x00,
+		/* section 1 name (".resource_table") */
+		0x2e, 0x72, 0x65, 0x73, 0x6f, 0x75, 0x72, 0x63, 0x65, 0x5f,
+		0x74, 0x61, 0x62, 0x6c, 0x65, 0x00,
+		/* padding */
+		0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+
+		/* @0x90 - SECTION HEADER TABLE - */
+		/* Section 0 : resource table header */
+		/* sh_name - index into section header string table section */
+		0x0a, 0x00, 0x00, 0x00,
+		/* sh_type and sh_flags */
+		0x01, 0x00, 0x00, 0x00, 0x03, 0x00, 0x00, 0x00,
+		/* sh_addr = where the resource table has to be copied to */
+		0x00, 0x00, 0x00, 0x00,
+		/* sh_offset = 0x60 */
+		0x60, 0x00, 0x00, 0x00,
+		/* sh_size = 16 bytes */
+		0x10, 0x00, 0x00, 0x00,
+		/* sh_link, sh_info, sh_addralign, sh_entsize */
+		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+		/* Section 1 : section's names section header */
+		/* sh_name - index into section header string table section */
+		0x00, 0x00, 0x00, 0x00,
+		/* sh_type and sh_flags */
+		0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+		/* sh_addr  */
+		0x00, 0x00, 0x00, 0x00,
+		/* sh_offset = 0x70 */
+		0x70, 0x00, 0x00, 0x00,
+		/* sh_size = 27 bytes */
+		0x1b, 0x00, 0x00, 0x00,
+		/* sh_link, sh_info, sh_addralign, sh_entsize */
+		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
 	};
 	unsigned int size = ARRAY_SIZE(valid_elf32);
 	struct udevice *dev;
-	phys_addr_t loaded_firmware_paddr;
-	void *loaded_firmware;
-	u32 loaded_firmware_size;
+	phys_addr_t loaded_firmware_paddr, loaded_rsc_table_paddr;
+	void *loaded_firmware, *loaded_rsc_table;
+	u32 loaded_firmware_size, rsc_table_size;
+	ulong rsc_addr, rsc_size;
 	Elf32_Ehdr *ehdr = (Elf32_Ehdr *)valid_elf32;
 	Elf32_Phdr *phdr = (Elf32_Phdr *)(valid_elf32 + ehdr->e_phoff);
+	Elf32_Shdr *shdr = (Elf32_Shdr *)(valid_elf32 + ehdr->e_shoff);
 
 	ut_assertok(uclass_get_device(UCLASS_REMOTEPROC, 0, &dev));
 
@@ -178,6 +228,25 @@ static int dm_test_remoteproc_elf(struct unit_test_state *uts)
 		    0x08000000);
 	unmap_physmem(loaded_firmware, MAP_NOCACHE);
 
+	/* Resource table */
+	shdr->sh_addr = CONFIG_SYS_SDRAM_BASE;
+	rsc_table_size = shdr->sh_size;
+
+	loaded_rsc_table_paddr = shdr->sh_addr + DEVICE_TO_PHYSICAL_OFFSET;
+	loaded_rsc_table = map_physmem(loaded_rsc_table_paddr,
+				       rsc_table_size, MAP_NOCACHE);
+	ut_assertnonnull(loaded_rsc_table);
+	memset(loaded_rsc_table, 0, rsc_table_size);
+
+	/* Load and verify */
+	ut_assertok(rproc_elf32_load_rsc_table(dev, (ulong)valid_elf32, size,
+					       &rsc_addr, &rsc_size));
+	ut_asserteq(rsc_addr, CONFIG_SYS_SDRAM_BASE);
+	ut_asserteq(rsc_size, rsc_table_size);
+	ut_assertok(memcmp(loaded_firmware, valid_elf32 + shdr->sh_offset,
+			   shdr->sh_size));
+	unmap_physmem(loaded_firmware, MAP_NOCACHE);
+
 	/* Invalid ELF Magic */
 	valid_elf32[0] = 0;
 	ut_asserteq(-EPROTONOSUPPORT,
-- 
2.7.4

[U-Boot] [PATCH 2/5] stm32mp1: declare backup register for copro resource table address

[Fabien Dessenne]

Use the backup register #17 as coprocessor resource table address.

Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
---
 arch/arm/mach-stm32mp/include/mach/stm32.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-stm32mp/include/mach/stm32.h b/arch/arm/mach-stm32mp/include/mach/stm32.h
index b3e9ccc..4392096 100644
--- a/arch/arm/mach-stm32mp/include/mach/stm32.h
+++ b/arch/arm/mach-stm32mp/include/mach/stm32.h
@@ -86,6 +86,7 @@ enum boot_device {
 #define TAMP_BACKUP_REGISTER(x)		(STM32_TAMP_BASE + 0x100 + 4 * x)
 #define TAMP_BACKUP_MAGIC_NUMBER	TAMP_BACKUP_REGISTER(4)
 #define TAMP_BACKUP_BRANCH_ADDRESS	TAMP_BACKUP_REGISTER(5)
+#define TAMP_COPRO_RSC_TBL_ADDRESS	TAMP_BACKUP_REGISTER(17)
 #define TAMP_BOOT_CONTEXT		TAMP_BACKUP_REGISTER(20)
 #define TAMP_BOOTCOUNT			TAMP_BACKUP_REGISTER(21)
 
-- 
2.7.4

[U-Boot] [PATCH 3/5] remoteproc: stm32: load resource table from firmware

[Fabien Dessenne]

Load the optional resource table from the firmware, and write its
address in the dedicated backup register.

Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
---
 drivers/remoteproc/stm32_copro.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/remoteproc/stm32_copro.c b/drivers/remoteproc/stm32_copro.c
index 40bba37..eef3416 100644
--- a/drivers/remoteproc/stm32_copro.c
+++ b/drivers/remoteproc/stm32_copro.c
@@ -23,6 +23,7 @@
  * @hold_boot_offset:	offset of the register controlling the hold boot setting
  * @hold_boot_mask:	bitmask of the register for the hold boot field
  * @is_running:		is the remote processor running
+ * @rsc_table_addr:	resource table address
  */
 struct stm32_copro_privdata {
 	struct reset_ctl reset_ctl;
@@ -30,6 +31,7 @@ struct stm32_copro_privdata {
 	uint hold_boot_offset;
 	uint hold_boot_mask;
 	bool is_running;
+	ulong rsc_table_addr;
 };
 
 /**
@@ -141,6 +143,7 @@ static void *stm32_copro_device_to_virt(struct udevice *dev, ulong da,
 static int stm32_copro_load(struct udevice *dev, ulong addr, ulong size)
 {
 	struct stm32_copro_privdata *priv;
+	ulong rsc_table_size;
 	int ret;
 
 	priv = dev_get_priv(dev);
@@ -155,6 +158,12 @@ static int stm32_copro_load(struct udevice *dev, ulong addr, ulong size)
 		return ret;
 	}
 
+	if (rproc_elf32_load_rsc_table(dev, addr, size, &priv->rsc_table_addr,
+				       &rsc_table_size)) {
+		priv->rsc_table_addr = 0;
+		dev_warn(dev, "No valid resource table for this firmware\n");
+	}
+
 	return rproc_elf32_load_image(dev, addr, size);
 }
 
@@ -180,6 +189,10 @@ static int stm32_copro_start(struct udevice *dev)
 	 * rebooting autonomously
 	 */
 	ret = stm32_copro_set_hold_boot(dev, true);
+	if (!ret)
+		/* Store rsc_address in bkp register */
+		writel(priv->rsc_table_addr, TAMP_COPRO_RSC_TBL_ADDRESS);
+
 	priv->is_running = !ret;
 	return ret;
 }
-- 
2.7.4

[U-Boot] [PATCH 4/5] stm32mp1: Fixup the Linux DeviceTree with coprocessor information

[Fabien Dessenne]

When the coprocessor has been started, provide the context to Linux
kernel so it can handle it:
- update the coprocessor node of kernel DeviceTree with the
  "early-booted" property.
- write the resource table address in a dedicated backup register.

Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
---
 board/st/stm32mp1/stm32mp1.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c
index 18f9b84..8c669d0 100644
--- a/board/st/stm32mp1/stm32mp1.c
+++ b/board/st/stm32mp1/stm32mp1.c
@@ -891,6 +891,7 @@ void board_mtdparts_default(const char **mtdids, const char **mtdparts)
 #if defined(CONFIG_OF_BOARD_SETUP)
 int ft_board_setup(void *blob, bd_t *bd)
 {
+	int off, id = 0; /* Copro id fixed to 0 as only one coproc on mp1 */
 #ifdef CONFIG_FDT_FIXUP_PARTITIONS
 	struct node_info nodes[] = {
 		{ "st,stm32f469-qspi",		MTD_DEV_TYPE_NOR,  },
@@ -899,6 +900,17 @@ int ft_board_setup(void *blob, bd_t *bd)
 	fdt_fixup_mtdparts(blob, nodes, ARRAY_SIZE(nodes));
 #endif
 
+	/* Update DT if coprocessor started */
+	off = fdt_path_offset(blob, "/mlahb/m4 at 10000000");
+	if (off > 0) {
+		if (!rproc_is_running(id)) {
+			fdt_setprop_empty(blob, off, "early-booted");
+		} else {
+			fdt_delprop(blob, off, "early-booted");
+			writel(0, TAMP_COPRO_RSC_TBL_ADDRESS);
+		}
+	}
+
 	return 0;
 }
 #endif
@@ -918,10 +930,8 @@ static void board_copro_image_process(ulong fw_image, size_t fw_size)
 	printf("Load Remote Processor %d with data at addr=0x%08lx %u bytes:%s\n",
 	       id, fw_image, fw_size, ret ? " Failed!" : " Success!");
 
-	if (!ret) {
+	if (!ret)
 		rproc_start(id);
-		env_set("copro_state", "booted");
-	}
 }
 
 U_BOOT_FIT_LOADABLE_HANDLER(IH_TYPE_COPRO, board_copro_image_process);
-- 
2.7.4

[U-Boot] [PATCH 5/5] remoteproc: stm32: invert the is_running() return value

[Fabien Dessenne]

The .is_running() ops expects a return value of 0 if the processor is
running, 1 if not running : align to this.

Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
---
 drivers/remoteproc/stm32_copro.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/stm32_copro.c b/drivers/remoteproc/stm32_copro.c
index eef3416..fce9653 100644
--- a/drivers/remoteproc/stm32_copro.c
+++ b/drivers/remoteproc/stm32_copro.c
@@ -237,14 +237,14 @@ static int stm32_copro_stop(struct udevice *dev)
 /**
  * stm32_copro_is_running() - Is the STM32 remote processor running
  * @dev:	corresponding STM32 remote processor device
- * @return 1 if the remote processor is running, 0 otherwise
+ * @return 0 if the remote processor is running, 1 otherwise
  */
 static int stm32_copro_is_running(struct udevice *dev)
 {
 	struct stm32_copro_privdata *priv;
 
 	priv = dev_get_priv(dev);
-	return priv->is_running;
+	return !priv->is_running;
 }
 
 static const struct dm_rproc_ops stm32_copro_ops = {
-- 
2.7.4

[U-Boot] [PATCH 4/5] stm32mp1: Fixup the Linux DeviceTree with coprocessor information

[Suman Anna]

Hi Fabien,

On 10/9/19 10:36 AM, Fabien Dessenne wrote:
> When the coprocessor has been started, provide the context to Linux
> kernel so it can handle it:
> - update the coprocessor node of kernel DeviceTree with the
>   "early-booted" property.

Has this property been acked by DT maintainers at the kernel-level?
We have used something similar but moving away from it and instead just
relying on reading the hardware reset status in the kernel remoteproc
driver, and configuring the driver accordingly.

> - write the resource table address in a dedicated backup register.

Is this an actual register on the SoC or some block of memory dedicated
for sharing information from U-Boot to kernel?

We have the same problem and I am currently going with a
design-by-contract approach for early-booted usecases where I am
expecting the resource table to be placed at a specific location in
memory regions given to remoteproc.

regards
Suman

> 
> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
> ---
>  board/st/stm32mp1/stm32mp1.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c
> index 18f9b84..8c669d0 100644
> --- a/board/st/stm32mp1/stm32mp1.c
> +++ b/board/st/stm32mp1/stm32mp1.c
> @@ -891,6 +891,7 @@ void board_mtdparts_default(const char **mtdids, const char **mtdparts)
>  #if defined(CONFIG_OF_BOARD_SETUP)
>  int ft_board_setup(void *blob, bd_t *bd)
>  {
> +	int off, id = 0; /* Copro id fixed to 0 as only one coproc on mp1 */
>  #ifdef CONFIG_FDT_FIXUP_PARTITIONS
>  	struct node_info nodes[] = {
>  		{ "st,stm32f469-qspi",		MTD_DEV_TYPE_NOR,  },
> @@ -899,6 +900,17 @@ int ft_board_setup(void *blob, bd_t *bd)
>  	fdt_fixup_mtdparts(blob, nodes, ARRAY_SIZE(nodes));
>  #endif
>  
> +	/* Update DT if coprocessor started */
> +	off = fdt_path_offset(blob, "/mlahb/m4 at 10000000");
> +	if (off > 0) {
> +		if (!rproc_is_running(id)) {
> +			fdt_setprop_empty(blob, off, "early-booted");
> +		} else {
> +			fdt_delprop(blob, off, "early-booted");
> +			writel(0, TAMP_COPRO_RSC_TBL_ADDRESS);
> +		}
> +	}
> +
>  	return 0;
>  }
>  #endif
> @@ -918,10 +930,8 @@ static void board_copro_image_process(ulong fw_image, size_t fw_size)
>  	printf("Load Remote Processor %d with data at addr=0x%08lx %u bytes:%s\n",
>  	       id, fw_image, fw_size, ret ? " Failed!" : " Success!");
>  
> -	if (!ret) {
> +	if (!ret)
>  		rproc_start(id);
> -		env_set("copro_state", "booted");
> -	}
>  }
>  
>  U_BOOT_FIT_LOADABLE_HANDLER(IH_TYPE_COPRO, board_copro_image_process);
> 

[U-Boot] [PATCH 5/5] remoteproc: stm32: invert the is_running() return value

[Suman Anna]

On 10/9/19 10:36 AM, Fabien Dessenne wrote:
> The .is_running() ops expects a return value of 0 if the processor is
> running, 1 if not running : align to this.
> 
> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>

This patch should be earlier than patch4, right?

Reviewed-by: Suman Anna <s-anna@ti.com>

> ---
>  drivers/remoteproc/stm32_copro.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/stm32_copro.c b/drivers/remoteproc/stm32_copro.c
> index eef3416..fce9653 100644
> --- a/drivers/remoteproc/stm32_copro.c
> +++ b/drivers/remoteproc/stm32_copro.c
> @@ -237,14 +237,14 @@ static int stm32_copro_stop(struct udevice *dev)
>  /**
>   * stm32_copro_is_running() - Is the STM32 remote processor running
>   * @dev:	corresponding STM32 remote processor device
> - * @return 1 if the remote processor is running, 0 otherwise
> + * @return 0 if the remote processor is running, 1 otherwise
>   */
>  static int stm32_copro_is_running(struct udevice *dev)
>  {
>  	struct stm32_copro_privdata *priv;
>  
>  	priv = dev_get_priv(dev);
> -	return priv->is_running;
> +	return !priv->is_running;
>  }
>  
>  static const struct dm_rproc_ops stm32_copro_ops = {
> 

[U-Boot] [PATCH 3/5] remoteproc: stm32: load resource table from firmware

[Suman Anna]

Hi Fabien,

On 10/9/19 10:36 AM, Fabien Dessenne wrote:
> Load the optional resource table from the firmware, and write its
> address in the dedicated backup register.

What processor is this? Reason I ask is that you are using 0 as a no
resource table address, and if it is a valid address for that processor?

regards
Suman

> 
> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
> ---
>  drivers/remoteproc/stm32_copro.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/remoteproc/stm32_copro.c b/drivers/remoteproc/stm32_copro.c
> index 40bba37..eef3416 100644
> --- a/drivers/remoteproc/stm32_copro.c
> +++ b/drivers/remoteproc/stm32_copro.c
> @@ -23,6 +23,7 @@
>   * @hold_boot_offset:	offset of the register controlling the hold boot setting
>   * @hold_boot_mask:	bitmask of the register for the hold boot field
>   * @is_running:		is the remote processor running
> + * @rsc_table_addr:	resource table address
>   */
>  struct stm32_copro_privdata {
>  	struct reset_ctl reset_ctl;
> @@ -30,6 +31,7 @@ struct stm32_copro_privdata {
>  	uint hold_boot_offset;
>  	uint hold_boot_mask;
>  	bool is_running;
> +	ulong rsc_table_addr;
>  };
>  
>  /**
> @@ -141,6 +143,7 @@ static void *stm32_copro_device_to_virt(struct udevice *dev, ulong da,
>  static int stm32_copro_load(struct udevice *dev, ulong addr, ulong size)
>  {
>  	struct stm32_copro_privdata *priv;
> +	ulong rsc_table_size;
>  	int ret;
>  
>  	priv = dev_get_priv(dev);
> @@ -155,6 +158,12 @@ static int stm32_copro_load(struct udevice *dev, ulong addr, ulong size)
>  		return ret;
>  	}
>  
> +	if (rproc_elf32_load_rsc_table(dev, addr, size, &priv->rsc_table_addr,
> +				       &rsc_table_size)) {
> +		priv->rsc_table_addr = 0;
> +		dev_warn(dev, "No valid resource table for this firmware\n");
> +	}
> +
>  	return rproc_elf32_load_image(dev, addr, size);
>  }
>  
> @@ -180,6 +189,10 @@ static int stm32_copro_start(struct udevice *dev)
>  	 * rebooting autonomously
>  	 */
>  	ret = stm32_copro_set_hold_boot(dev, true);
> +	if (!ret)
> +		/* Store rsc_address in bkp register */
> +		writel(priv->rsc_table_addr, TAMP_COPRO_RSC_TBL_ADDRESS);
> +
>  	priv->is_running = !ret;
>  	return ret;
>  }
> 

[U-Boot] [PATCH 4/5] stm32mp1: Fixup the Linux DeviceTree with coprocessor information

[Fabien DESSENNE]

Hi Suman,


Thank you for your comments.


On 11/10/2019 9:57 PM, Suman Anna wrote:
> Hi Fabien,
>
> On 10/9/19 10:36 AM, Fabien Dessenne wrote:
>> When the coprocessor has been started, provide the context to Linux
>> kernel so it can handle it:
>> - update the coprocessor node of kernel DeviceTree with the
>>    "early-booted" property.
> Has this property been acked by DT maintainers at the kernel-level?
> We have used something similar but moving away from it and instead just
> relying on reading the hardware reset status in the kernel remoteproc
> driver, and configuring the driver accordingly.

Good point, that "early-booted" property has not been reviewed by the 
kernel DT maintainers.
This property shall be read by some kernel remoteproc drivers, together 
with the kernel "remoteproc: add support for preloaded firmware" 
patchset [1] which is still under review/rework (I expect Loic to send 
an update version (very) soon).

I will nevertheless rework this u-boot patchset in order to get rid of 
that property. Instead of, I plan to use some additional HW register to 
track the processor state.

[1] https://lkml.org/lkml/2018/11/30/157


>
>> - write the resource table address in a dedicated backup register.
> Is this an actual register on the SoC or some block of memory dedicated
> for sharing information from U-Boot to kernel?
>
> We have the same problem and I am currently going with a
> design-by-contract approach for early-booted usecases where I am
> expecting the resource table to be placed at a specific location in
> memory regions given to remoteproc.

I am interested in having the resource table placed in a specific 
remoteproc memory region.
For the time being I write the RscTable in a backup register which is an 
actual SoC register. When the "specific memory region" dev is available, 
I may consider moving to that alternate implementation.


BR


Fabien

>
> regards
> Suman
>
>> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
>> ---
>>   board/st/stm32mp1/stm32mp1.c | 16 +++++++++++++---
>>   1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c
>> index 18f9b84..8c669d0 100644
>> --- a/board/st/stm32mp1/stm32mp1.c
>> +++ b/board/st/stm32mp1/stm32mp1.c
>> @@ -891,6 +891,7 @@ void board_mtdparts_default(const char **mtdids, const char **mtdparts)
>>   #if defined(CONFIG_OF_BOARD_SETUP)
>>   int ft_board_setup(void *blob, bd_t *bd)
>>   {
>> +	int off, id = 0; /* Copro id fixed to 0 as only one coproc on mp1 */
>>   #ifdef CONFIG_FDT_FIXUP_PARTITIONS
>>   	struct node_info nodes[] = {
>>   		{ "st,stm32f469-qspi",		MTD_DEV_TYPE_NOR,  },
>> @@ -899,6 +900,17 @@ int ft_board_setup(void *blob, bd_t *bd)
>>   	fdt_fixup_mtdparts(blob, nodes, ARRAY_SIZE(nodes));
>>   #endif
>>   
>> +	/* Update DT if coprocessor started */
>> +	off = fdt_path_offset(blob, "/mlahb/m4 at 10000000");
>> +	if (off > 0) {
>> +		if (!rproc_is_running(id)) {
>> +			fdt_setprop_empty(blob, off, "early-booted");
>> +		} else {
>> +			fdt_delprop(blob, off, "early-booted");
>> +			writel(0, TAMP_COPRO_RSC_TBL_ADDRESS);
>> +		}
>> +	}
>> +
>>   	return 0;
>>   }
>>   #endif
>> @@ -918,10 +930,8 @@ static void board_copro_image_process(ulong fw_image, size_t fw_size)
>>   	printf("Load Remote Processor %d with data at addr=0x%08lx %u bytes:%s\n",
>>   	       id, fw_image, fw_size, ret ? " Failed!" : " Success!");
>>   
>> -	if (!ret) {
>> +	if (!ret)
>>   		rproc_start(id);
>> -		env_set("copro_state", "booted");
>> -	}
>>   }
>>   
>>   U_BOOT_FIT_LOADABLE_HANDLER(IH_TYPE_COPRO, board_copro_image_process);
>>

[U-Boot] [PATCH 5/5] remoteproc: stm32: invert the is_running() return value

[Fabien DESSENNE]

On 11/10/2019 10:00 PM, Suman Anna wrote:
> On 10/9/19 10:36 AM, Fabien Dessenne wrote:
>> The .is_running() ops expects a return value of 0 if the processor is
>> running, 1 if not running : align to this.
>>
>> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
> This patch should be earlier than patch4, right?


You're probably right. I will double check this in v2.


>
> Reviewed-by: Suman Anna <s-anna@ti.com>
>
>> ---
>>   drivers/remoteproc/stm32_copro.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/remoteproc/stm32_copro.c b/drivers/remoteproc/stm32_copro.c
>> index eef3416..fce9653 100644
>> --- a/drivers/remoteproc/stm32_copro.c
>> +++ b/drivers/remoteproc/stm32_copro.c
>> @@ -237,14 +237,14 @@ static int stm32_copro_stop(struct udevice *dev)
>>   /**
>>    * stm32_copro_is_running() - Is the STM32 remote processor running
>>    * @dev:	corresponding STM32 remote processor device
>> - * @return 1 if the remote processor is running, 0 otherwise
>> + * @return 0 if the remote processor is running, 1 otherwise
>>    */
>>   static int stm32_copro_is_running(struct udevice *dev)
>>   {
>>   	struct stm32_copro_privdata *priv;
>>   
>>   	priv = dev_get_priv(dev);
>> -	return priv->is_running;
>> +	return !priv->is_running;
>>   }
>>   
>>   static const struct dm_rproc_ops stm32_copro_ops = {
>>

[U-Boot] [PATCH 3/5] remoteproc: stm32: load resource table from firmware

[Fabien DESSENNE]

On 11/10/2019 10:09 PM, Suman Anna wrote:
> Hi Fabien,
>
> On 10/9/19 10:36 AM, Fabien Dessenne wrote:
>> Load the optional resource table from the firmware, and write its
>> address in the dedicated backup register.
> What processor is this? Reason I ask is that you are using 0 as a no
> resource table address, and if it is a valid address for that processor?


Since on system reset, the vector table of the STM32MP Cortex-M4 
co-processor is fixed at address 0, the ResourceTable can't be located here.

ResourceTable address = 0 can be safely interpreted as "No resource table".

BR

Fabien


>
> regards
> Suman
>
>> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
>> ---
>>   drivers/remoteproc/stm32_copro.c | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/remoteproc/stm32_copro.c b/drivers/remoteproc/stm32_copro.c
>> index 40bba37..eef3416 100644
>> --- a/drivers/remoteproc/stm32_copro.c
>> +++ b/drivers/remoteproc/stm32_copro.c
>> @@ -23,6 +23,7 @@
>>    * @hold_boot_offset:	offset of the register controlling the hold boot setting
>>    * @hold_boot_mask:	bitmask of the register for the hold boot field
>>    * @is_running:		is the remote processor running
>> + * @rsc_table_addr:	resource table address
>>    */
>>   struct stm32_copro_privdata {
>>   	struct reset_ctl reset_ctl;
>> @@ -30,6 +31,7 @@ struct stm32_copro_privdata {
>>   	uint hold_boot_offset;
>>   	uint hold_boot_mask;
>>   	bool is_running;
>> +	ulong rsc_table_addr;
>>   };
>>   
>>   /**
>> @@ -141,6 +143,7 @@ static void *stm32_copro_device_to_virt(struct udevice *dev, ulong da,
>>   static int stm32_copro_load(struct udevice *dev, ulong addr, ulong size)
>>   {
>>   	struct stm32_copro_privdata *priv;
>> +	ulong rsc_table_size;
>>   	int ret;
>>   
>>   	priv = dev_get_priv(dev);
>> @@ -155,6 +158,12 @@ static int stm32_copro_load(struct udevice *dev, ulong addr, ulong size)
>>   		return ret;
>>   	}
>>   
>> +	if (rproc_elf32_load_rsc_table(dev, addr, size, &priv->rsc_table_addr,
>> +				       &rsc_table_size)) {
>> +		priv->rsc_table_addr = 0;
>> +		dev_warn(dev, "No valid resource table for this firmware\n");
>> +	}
>> +
>>   	return rproc_elf32_load_image(dev, addr, size);
>>   }
>>   
>> @@ -180,6 +189,10 @@ static int stm32_copro_start(struct udevice *dev)
>>   	 * rebooting autonomously
>>   	 */
>>   	ret = stm32_copro_set_hold_boot(dev, true);
>> +	if (!ret)
>> +		/* Store rsc_address in bkp register */
>> +		writel(priv->rsc_table_addr, TAMP_COPRO_RSC_TBL_ADDRESS);
>> +
>>   	priv->is_running = !ret;
>>   	return ret;
>>   }
>>

[U-Boot] [PATCH 1/5] remoteproc: elf_loader: Add elf resource table load support

[Simon Glass]

Hi Fabien,

On Wed, 9 Oct 2019 at 09:36, Fabien Dessenne <fabien.dessenne@st.com> wrote:
>
> Add rproc_elf_load_rsc_table(), which searches for a resource table in
> an elf64/elf32 image, and if found, copies it to device memory.
> Add also the elf32 and elf64 variants of this API.
> Add a test for this.
>
> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
> ---
>  drivers/remoteproc/rproc-elf-loader.c | 269 ++++++++++++++++++++++++++++++++++
>  include/remoteproc.h                  |  70 +++++++++
>  test/dm/remoteproc.c                  |  91 ++++++++++--
>  3 files changed, 419 insertions(+), 11 deletions(-)
>

If you are putting stuff in the image, should you use binman to build
the image, then find the contents using the binman tables?

Scanning the image for a table seems a bit horrible.

Regards,
Simon

[U-Boot] [PATCH 1/5] remoteproc: elf_loader: Add elf resource table load support

[Fabien DESSENNE]

Hi Simon,


On 22/10/2019 1:47 AM, Simon Glass wrote:
> Hi Fabien,
>
> On Wed, 9 Oct 2019 at 09:36, Fabien Dessenne <fabien.dessenne@st.com> wrote:
>> Add rproc_elf_load_rsc_table(), which searches for a resource table in
>> an elf64/elf32 image, and if found, copies it to device memory.
>> Add also the elf32 and elf64 variants of this API.
>> Add a test for this.
>>
>> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
>> ---
>>   drivers/remoteproc/rproc-elf-loader.c | 269 ++++++++++++++++++++++++++++++++++
>>   include/remoteproc.h                  |  70 +++++++++
>>   test/dm/remoteproc.c                  |  91 ++++++++++--
>>   3 files changed, 419 insertions(+), 11 deletions(-)
>>
> If you are putting stuff in the image, should you use binman to build
> the image, then find the contents using the binman tables?


The "resource table" may be located anywhere, there is no strict rule 
defining where it is expected to be.

Nevertheless the Linux remoteproc[1] and OpenAmp (running RTOS) [2] 
frameworks expect the resource table to be stored in a dedicated ELF 
section. Both of them run some ELF scanning to find out this section.

The proposed patch is for the "ELF section" variant of the resource table.
Other variants like binman packing may be proposed as well, both 
implementations can coexist alongside.

BR

Fabien

[1] https://www.kernel.org/doc/Documentation/remoteproc.txt
[2] 
https://github.com/OpenAMP/open-amp/blob/master/lib/remoteproc/elf_loader.c

>
> Scanning the image for a table seems a bit horrible.
>
> Regards,
> Simon

[U-Boot] [PATCH 1/5] remoteproc: elf_loader: Add elf resource table load support

[Simon Glass]

Hi Fabien,

On Tue, 22 Oct 2019 at 03:08, Fabien DESSENNE <fabien.dessenne@st.com> wrote:
>
> Hi Simon,
>
>
> On 22/10/2019 1:47 AM, Simon Glass wrote:
> > Hi Fabien,
> >
> > On Wed, 9 Oct 2019 at 09:36, Fabien Dessenne <fabien.dessenne@st.com> wrote:
> >> Add rproc_elf_load_rsc_table(), which searches for a resource table in
> >> an elf64/elf32 image, and if found, copies it to device memory.
> >> Add also the elf32 and elf64 variants of this API.
> >> Add a test for this.
> >>
> >> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
> >> ---
> >>   drivers/remoteproc/rproc-elf-loader.c | 269 ++++++++++++++++++++++++++++++++++
> >>   include/remoteproc.h                  |  70 +++++++++
> >>   test/dm/remoteproc.c                  |  91 ++++++++++--
> >>   3 files changed, 419 insertions(+), 11 deletions(-)
> >>
> > If you are putting stuff in the image, should you use binman to build
> > the image, then find the contents using the binman tables?
>
>
> The "resource table" may be located anywhere, there is no strict rule
> defining where it is expected to be.
>
> Nevertheless the Linux remoteproc[1] and OpenAmp (running RTOS) [2]
> frameworks expect the resource table to be stored in a dedicated ELF
> section. Both of them run some ELF scanning to find out this section.
>
> The proposed patch is for the "ELF section" variant of the resource table.
> Other variants like binman packing may be proposed as well, both
> implementations can coexist alongside.

So why not use binman to pack the image and find the components? This
is U-Boot, after all.


>
> BR
>
> Fabien
>
> [1] https://www.kernel.org/doc/Documentation/remoteproc.txt
> [2]
> https://github.com/OpenAMP/open-amp/blob/master/lib/remoteproc/elf_loader.c
>
> >
> > Scanning the image for a table seems a bit horrible.

Regards,
Simon

[U-Boot] [PATCH 1/5] remoteproc: elf_loader: Add elf resource table load support

[Fabien DESSENNE]

Hi Simon

On 30/10/2019 2:49 AM, Simon Glass wrote:
> Hi Fabien,
>
> On Tue, 22 Oct 2019 at 03:08, Fabien DESSENNE <fabien.dessenne@st.com> wrote:
>> Hi Simon,
>>
>>
>> On 22/10/2019 1:47 AM, Simon Glass wrote:
>>> Hi Fabien,
>>>
>>> On Wed, 9 Oct 2019 at 09:36, Fabien Dessenne <fabien.dessenne@st.com> wrote:
>>>> Add rproc_elf_load_rsc_table(), which searches for a resource table in
>>>> an elf64/elf32 image, and if found, copies it to device memory.
>>>> Add also the elf32 and elf64 variants of this API.
>>>> Add a test for this.
>>>>
>>>> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
>>>> ---
>>>>    drivers/remoteproc/rproc-elf-loader.c | 269 ++++++++++++++++++++++++++++++++++
>>>>    include/remoteproc.h                  |  70 +++++++++
>>>>    test/dm/remoteproc.c                  |  91 ++++++++++--
>>>>    3 files changed, 419 insertions(+), 11 deletions(-)
>>>>
>>> If you are putting stuff in the image, should you use binman to build
>>> the image, then find the contents using the binman tables?
>>
>> The "resource table" may be located anywhere, there is no strict rule
>> defining where it is expected to be.
>>
>> Nevertheless the Linux remoteproc[1] and OpenAmp (running RTOS) [2]
>> frameworks expect the resource table to be stored in a dedicated ELF
>> section. Both of them run some ELF scanning to find out this section.
>>
>> The proposed patch is for the "ELF section" variant of the resource table.
>> Other variants like binman packing may be proposed as well, both
>> implementations can coexist alongside.
> So why not use binman to pack the image and find the components? This
> is U-Boot, after all.
>

Packing the firmware together with the other U-Boot components is 
acceptable if the firmware is controlled only by U-Boot.
My requirement is that the coprocessor firmware shall be loaded by 
U-Boot or by Linux.

You can have a look at [1] for more details on the way this is handled 
on STM32 MPU. In that case, the .elf firmware is stored in a in File 
System that can be read by both U-Boot and Linux.

If we have the firmware packed in the image (for U-Boot), we need to 
have a copy in the FileSystem (for Linux) which would not be a good idea.

BR
Fabien

[1] https://wiki.st.com/stm32mpu/index.php/Boot_chains_overview


>> BR
>>
>> Fabien
>>
>> [1] https://www.kernel.org/doc/Documentation/remoteproc.txt
>> [2]
>> https://github.com/OpenAMP/open-amp/blob/master/lib/remoteproc/elf_loader.c
>>
>>> Scanning the image for a table seems a bit horrible.
> Regards,
> Simon

[PATCH 1/5] remoteproc: elf_loader: Add elf resource table load support

[Simon Glass]

Hi Fabien,

On Wed, 30 Oct 2019 at 03:50, Fabien DESSENNE <fabien.dessenne@st.com> wrote:
>
> Hi Simon
>
> On 30/10/2019 2:49 AM, Simon Glass wrote:
> > Hi Fabien,
> >
> > On Tue, 22 Oct 2019 at 03:08, Fabien DESSENNE <fabien.dessenne@st.com> wrote:
> >> Hi Simon,
> >>
> >>
> >> On 22/10/2019 1:47 AM, Simon Glass wrote:
> >>> Hi Fabien,
> >>>
> >>> On Wed, 9 Oct 2019 at 09:36, Fabien Dessenne <fabien.dessenne@st.com> wrote:
> >>>> Add rproc_elf_load_rsc_table(), which searches for a resource table in
> >>>> an elf64/elf32 image, and if found, copies it to device memory.
> >>>> Add also the elf32 and elf64 variants of this API.
> >>>> Add a test for this.
> >>>>
> >>>> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
> >>>> ---
> >>>>    drivers/remoteproc/rproc-elf-loader.c | 269 ++++++++++++++++++++++++++++++++++
> >>>>    include/remoteproc.h                  |  70 +++++++++
> >>>>    test/dm/remoteproc.c                  |  91 ++++++++++--
> >>>>    3 files changed, 419 insertions(+), 11 deletions(-)
> >>>>
> >>> If you are putting stuff in the image, should you use binman to build
> >>> the image, then find the contents using the binman tables?
> >>
> >> The "resource table" may be located anywhere, there is no strict rule
> >> defining where it is expected to be.
> >>
> >> Nevertheless the Linux remoteproc[1] and OpenAmp (running RTOS) [2]
> >> frameworks expect the resource table to be stored in a dedicated ELF
> >> section. Both of them run some ELF scanning to find out this section.
> >>
> >> The proposed patch is for the "ELF section" variant of the resource table.
> >> Other variants like binman packing may be proposed as well, both
> >> implementations can coexist alongside.
> > So why not use binman to pack the image and find the components? This
> > is U-Boot, after all.
> >
>
> Packing the firmware together with the other U-Boot components is
> acceptable if the firmware is controlled only by U-Boot.
> My requirement is that the coprocessor firmware shall be loaded by
> U-Boot or by Linux.
>
> You can have a look at [1] for more details on the way this is handled
> on STM32 MPU. In that case, the .elf firmware is stored in a in File
> System that can be read by both U-Boot and Linux.
>

Where is the coprocessor firmware stored, then?

> If we have the firmware packed in the image (for U-Boot), we need to
> have a copy in the FileSystem (for Linux) which would not be a good idea.

What type of filesystem do you use? I don't see any filesystem access
in this patch though.

>
> BR
> Fabien
>
> [1] https://wiki.st.com/stm32mpu/index.php/Boot_chains_overview

Regards,
Simon

[PATCH 1/5] remoteproc: elf_loader: Add elf resource table load support

[Fabien DESSENNE]

Hi Simon,


On 10/12/2019 4:18 PM, Simon Glass wrote:
> Hi Fabien,
>
> On Wed, 30 Oct 2019 at 03:50, Fabien DESSENNE <fabien.dessenne@st.com> wrote:
>> Hi Simon
>>
>> On 30/10/2019 2:49 AM, Simon Glass wrote:
>>> Hi Fabien,
>>>
>>> On Tue, 22 Oct 2019 at 03:08, Fabien DESSENNE <fabien.dessenne@st.com> wrote:
>>>> Hi Simon,
>>>>
>>>>
>>>> On 22/10/2019 1:47 AM, Simon Glass wrote:
>>>>> Hi Fabien,
>>>>>
>>>>> On Wed, 9 Oct 2019 at 09:36, Fabien Dessenne <fabien.dessenne@st.com> wrote:
>>>>>> Add rproc_elf_load_rsc_table(), which searches for a resource table in
>>>>>> an elf64/elf32 image, and if found, copies it to device memory.
>>>>>> Add also the elf32 and elf64 variants of this API.
>>>>>> Add a test for this.
>>>>>>
>>>>>> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
>>>>>> ---
>>>>>>     drivers/remoteproc/rproc-elf-loader.c | 269 ++++++++++++++++++++++++++++++++++
>>>>>>     include/remoteproc.h                  |  70 +++++++++
>>>>>>     test/dm/remoteproc.c                  |  91 ++++++++++--
>>>>>>     3 files changed, 419 insertions(+), 11 deletions(-)
>>>>>>
>>>>> If you are putting stuff in the image, should you use binman to build
>>>>> the image, then find the contents using the binman tables?
>>>> The "resource table" may be located anywhere, there is no strict rule
>>>> defining where it is expected to be.
>>>>
>>>> Nevertheless the Linux remoteproc[1] and OpenAmp (running RTOS) [2]
>>>> frameworks expect the resource table to be stored in a dedicated ELF
>>>> section. Both of them run some ELF scanning to find out this section.
>>>>
>>>> The proposed patch is for the "ELF section" variant of the resource table.
>>>> Other variants like binman packing may be proposed as well, both
>>>> implementations can coexist alongside.
>>> So why not use binman to pack the image and find the components? This
>>> is U-Boot, after all.
>>>
>> Packing the firmware together with the other U-Boot components is
>> acceptable if the firmware is controlled only by U-Boot.
>> My requirement is that the coprocessor firmware shall be loaded by
>> U-Boot or by Linux.
>>
>> You can have a look at [1] for more details on the way this is handled
>> on STM32 MPU. In that case, the .elf firmware is stored in a in File
>> System that can be read by both U-Boot and Linux.
>>
> Where is the coprocessor firmware stored, then?

Please have a look to [STM32MP15_Flash_mapping] which describes how 
u-boot, linux and the coprocessor firmwares can be stored in flash memory.

Both U-boot and Linux use an EXT4 filesystem (eg bootfs partition) to 
read the coprocessor .elf firmware file.


When it is read (it is then in DDR), U-boot or Linux parses it, in order 
to copy the resource table and the different ELF segments into 
appropriate memory addresses.

[STM32MP15_Flash_mapping] 
https://wiki.st.com/stm32mpu/wiki/STM32MP15_Flash_mapping


>
>> If we have the firmware packed in the image (for U-Boot), we need to
>> have a copy in the FileSystem (for Linux) which would not be a good idea.
> What type of filesystem do you use? I don't see any filesystem access
> in this patch though.


As described above, an EXT4 filesystem is a good example.

This patch is not about filesystem but about how the .elf contents 
available in DDR memory (after is has been loaded from somewhere) is parsed.


>
>> BR
>> Fabien
>>
>> [1] https://wiki.st.com/stm32mpu/index.php/Boot_chains_overview
> Regards,
> Simon

[PATCH 1/5] remoteproc: elf_loader: Add elf resource table load support

[Fabien DESSENNE]

Btw, I sent a v2 for this patch:

https://www.mail-archive.com/u-boot at lists.denx.de/msg346085.html


On 10/12/2019 4:18 PM, Simon Glass wrote:
> Hi Fabien,
>
> On Wed, 30 Oct 2019 at 03:50, Fabien DESSENNE <fabien.dessenne@st.com> wrote:
>> Hi Simon
>>
>> On 30/10/2019 2:49 AM, Simon Glass wrote:
>>> Hi Fabien,
>>>
>>> On Tue, 22 Oct 2019 at 03:08, Fabien DESSENNE <fabien.dessenne@st.com> wrote:
>>>> Hi Simon,
>>>>
>>>>
>>>> On 22/10/2019 1:47 AM, Simon Glass wrote:
>>>>> Hi Fabien,
>>>>>
>>>>> On Wed, 9 Oct 2019 at 09:36, Fabien Dessenne <fabien.dessenne@st.com> wrote:
>>>>>> Add rproc_elf_load_rsc_table(), which searches for a resource table in
>>>>>> an elf64/elf32 image, and if found, copies it to device memory.
>>>>>> Add also the elf32 and elf64 variants of this API.
>>>>>> Add a test for this.
>>>>>>
>>>>>> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
>>>>>> ---
>>>>>>     drivers/remoteproc/rproc-elf-loader.c | 269 ++++++++++++++++++++++++++++++++++
>>>>>>     include/remoteproc.h                  |  70 +++++++++
>>>>>>     test/dm/remoteproc.c                  |  91 ++++++++++--
>>>>>>     3 files changed, 419 insertions(+), 11 deletions(-)
>>>>>>
>>>>> If you are putting stuff in the image, should you use binman to build
>>>>> the image, then find the contents using the binman tables?
>>>> The "resource table" may be located anywhere, there is no strict rule
>>>> defining where it is expected to be.
>>>>
>>>> Nevertheless the Linux remoteproc[1] and OpenAmp (running RTOS) [2]
>>>> frameworks expect the resource table to be stored in a dedicated ELF
>>>> section. Both of them run some ELF scanning to find out this section.
>>>>
>>>> The proposed patch is for the "ELF section" variant of the resource table.
>>>> Other variants like binman packing may be proposed as well, both
>>>> implementations can coexist alongside.
>>> So why not use binman to pack the image and find the components? This
>>> is U-Boot, after all.
>>>
>> Packing the firmware together with the other U-Boot components is
>> acceptable if the firmware is controlled only by U-Boot.
>> My requirement is that the coprocessor firmware shall be loaded by
>> U-Boot or by Linux.
>>
>> You can have a look at [1] for more details on the way this is handled
>> on STM32 MPU. In that case, the .elf firmware is stored in a in File
>> System that can be read by both U-Boot and Linux.
>>
> Where is the coprocessor firmware stored, then?
>
>> If we have the firmware packed in the image (for U-Boot), we need to
>> have a copy in the FileSystem (for Linux) which would not be a good idea.
> What type of filesystem do you use? I don't see any filesystem access
> in this patch though.
>
>> BR
>> Fabien
>>
>> [1] https://wiki.st.com/stm32mpu/index.php/Boot_chains_overview
> Regards,
> Simon