[U-Boot] [PATCH 0/5] remoteproc: add elf resource table loader
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(-)
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,
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
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
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
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
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
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
Btw, I sent a v2 for this patch:
https://www.mail-archive.com/u-boot@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
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)
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; }
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;
}
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
*/ struct stm32_copro_privdata { struct reset_ctl reset_ctl;
- @rsc_table_addr: resource table address
@@ -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; }
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@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@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);
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@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@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);
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@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@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);
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 = {
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 = {
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 = {
participants (4)
-
Fabien DESSENNE -
Fabien Dessenne
-
Simon Glass -
Suman Anna