[PATCH v3 00/10] Add support for loading main_r5fss0_core0

[PATCH v3 00/10] Add support for loading main_r5fss0_core0

[Keerthy]

This patch series enables mcu_r5fss0_core0 & main_r5fss0_core0.
Tested for firmware loading and execution on J721e.

Changes in v3:

  * Removed saving env in MMC and fixed env saving in SPL when nowhere
    option is set.

Changes in v2:

  * Factored out all the generic elf handling functions under lib/elf.c 

Keerthy (10):
  lib: elf: Move the generic elf loading/validating functions to lib
  arm: k3: Add support for loading non linux remote cores
  armv7R: K3: r5_mpu: Enable execute permission for MCU0 BTCM
  armv7R: K3: Add support for jumping to firmware
  arm: dts: k3-j721e-r5: Add fs_loader node
  arm: dts: k3-j721e-r5: Enable r5fss0 cluster in SPL
  include: configs: j721e_evm: Add env variables for mcu_r5fss0_core0 &
    main_r5fss0_core0
  configs: j721e_evm_r5: Enable R5F remoteproc support
  configs: j721e_evm_r5_defconfig: Remove saving ENV in eMMC
  env: nowhere: set default enviroment

 .../arm/dts/k3-j721e-r5-common-proc-board.dts |  20 ++
 arch/arm/mach-k3/common.c                     | 106 +++++++-
 arch/arm/mach-k3/common.h                     |   2 +
 arch/arm/mach-k3/j721e_init.c                 |  34 +++
 arch/arm/mach-k3/r5_mpu.c                     |   4 +-
 cmd/Kconfig                                   |   1 +
 cmd/elf.c                                     | 229 ----------------
 configs/j721e_evm_r5_defconfig                |   6 +-
 env/nowhere.c                                 |   1 +
 include/configs/j721e_evm.h                   |   4 +
 include/elf.h                                 |   4 +
 lib/Kconfig                                   |   3 +
 lib/Makefile                                  |   1 +
 lib/elf.c                                     | 256 ++++++++++++++++++
 14 files changed, 426 insertions(+), 245 deletions(-)
 create mode 100644 lib/elf.c

-- 
2.17.1

[PATCH v3 01/10] lib: elf: Move the generic elf loading/validating functions to lib

[Keerthy]

Move the generic elf loading/validating functions to lib/
so that they can be re-used and accessed by code existing
outside cmd.

Signed-off-by: Keerthy <j-keerthy@ti.com>
Suggested-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
---
 cmd/Kconfig   |   1 +
 cmd/elf.c     | 229 --------------------------------------------
 include/elf.h |   4 +
 lib/Kconfig   |   3 +
 lib/Makefile  |   1 +
 lib/elf.c     | 256 ++++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 265 insertions(+), 229 deletions(-)
 create mode 100644 lib/elf.c

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 298feae24d..6f4f08d02a 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -375,6 +375,7 @@ config CMD_ADTIMG
 config CMD_ELF
 	bool "bootelf, bootvx"
 	default y
+	select ELF
 	help
 	  Boot an ELF/vxWorks image from the memory.
 
diff --git a/cmd/elf.c b/cmd/elf.c
index 32f12a72b9..23cc17aebc 100644
--- a/cmd/elf.c
+++ b/cmd/elf.c
@@ -26,211 +26,6 @@
 #include <linux/linkage.h>
 #endif
 
-/*
- * A very simple ELF64 loader, assumes the image is valid, returns the
- * entry point address.
- *
- * Note if U-Boot is 32-bit, the loader assumes the to segment's
- * physical address and size is within the lower 32-bit address space.
- */
-static unsigned long load_elf64_image_phdr(unsigned long addr)
-{
-	Elf64_Ehdr *ehdr; /* Elf header structure pointer */
-	Elf64_Phdr *phdr; /* Program header structure pointer */
-	int i;
-
-	ehdr = (Elf64_Ehdr *)addr;
-	phdr = (Elf64_Phdr *)(addr + (ulong)ehdr->e_phoff);
-
-	/* Load each program header */
-	for (i = 0; i < ehdr->e_phnum; ++i) {
-		void *dst = (void *)(ulong)phdr->p_paddr;
-		void *src = (void *)addr + phdr->p_offset;
-
-		debug("Loading phdr %i to 0x%p (%lu bytes)\n",
-		      i, dst, (ulong)phdr->p_filesz);
-		if (phdr->p_filesz)
-			memcpy(dst, src, phdr->p_filesz);
-		if (phdr->p_filesz != phdr->p_memsz)
-			memset(dst + phdr->p_filesz, 0x00,
-			       phdr->p_memsz - phdr->p_filesz);
-		flush_cache(rounddown((unsigned long)dst, ARCH_DMA_MINALIGN),
-			    roundup(phdr->p_memsz, ARCH_DMA_MINALIGN));
-		++phdr;
-	}
-
-	if (ehdr->e_machine == EM_PPC64 && (ehdr->e_flags &
-					    EF_PPC64_ELFV1_ABI)) {
-		/*
-		 * For the 64-bit PowerPC ELF V1 ABI, e_entry is a function
-		 * descriptor pointer with the first double word being the
-		 * address of the entry point of the function.
-		 */
-		uintptr_t addr = ehdr->e_entry;
-
-		return *(Elf64_Addr *)addr;
-	}
-
-	return ehdr->e_entry;
-}
-
-static unsigned long load_elf64_image_shdr(unsigned long addr)
-{
-	Elf64_Ehdr *ehdr; /* Elf header structure pointer */
-	Elf64_Shdr *shdr; /* Section header structure pointer */
-	unsigned char *strtab = 0; /* String table pointer */
-	unsigned char *image; /* Binary image pointer */
-	int i; /* Loop counter */
-
-	ehdr = (Elf64_Ehdr *)addr;
-
-	/* Find the section header string table for output info */
-	shdr = (Elf64_Shdr *)(addr + (ulong)ehdr->e_shoff +
-			     (ehdr->e_shstrndx * sizeof(Elf64_Shdr)));
-
-	if (shdr->sh_type == SHT_STRTAB)
-		strtab = (unsigned char *)(addr + (ulong)shdr->sh_offset);
-
-	/* Load each appropriate section */
-	for (i = 0; i < ehdr->e_shnum; ++i) {
-		shdr = (Elf64_Shdr *)(addr + (ulong)ehdr->e_shoff +
-				     (i * sizeof(Elf64_Shdr)));
-
-		if (!(shdr->sh_flags & SHF_ALLOC) ||
-		    shdr->sh_addr == 0 || shdr->sh_size == 0) {
-			continue;
-		}
-
-		if (strtab) {
-			debug("%sing %s @ 0x%08lx (%ld bytes)\n",
-			      (shdr->sh_type == SHT_NOBITS) ? "Clear" : "Load",
-			       &strtab[shdr->sh_name],
-			       (unsigned long)shdr->sh_addr,
-			       (long)shdr->sh_size);
-		}
-
-		if (shdr->sh_type == SHT_NOBITS) {
-			memset((void *)(uintptr_t)shdr->sh_addr, 0,
-			       shdr->sh_size);
-		} else {
-			image = (unsigned char *)addr + (ulong)shdr->sh_offset;
-			memcpy((void *)(uintptr_t)shdr->sh_addr,
-			       (const void *)image, shdr->sh_size);
-		}
-		flush_cache(rounddown(shdr->sh_addr, ARCH_DMA_MINALIGN),
-			    roundup((shdr->sh_addr + shdr->sh_size),
-				     ARCH_DMA_MINALIGN) -
-			            rounddown(shdr->sh_addr, ARCH_DMA_MINALIGN));
-	}
-
-	if (ehdr->e_machine == EM_PPC64 && (ehdr->e_flags &
-					    EF_PPC64_ELFV1_ABI)) {
-		/*
-		 * For the 64-bit PowerPC ELF V1 ABI, e_entry is a function
-		 * descriptor pointer with the first double word being the
-		 * address of the entry point of the function.
-		 */
-		uintptr_t addr = ehdr->e_entry;
-
-		return *(Elf64_Addr *)addr;
-	}
-
-	return ehdr->e_entry;
-}
-
-/*
- * A very simple ELF loader, assumes the image is valid, returns the
- * entry point address.
- *
- * The loader firstly reads the EFI class to see if it's a 64-bit image.
- * If yes, call the ELF64 loader. Otherwise continue with the ELF32 loader.
- */
-static unsigned long load_elf_image_phdr(unsigned long addr)
-{
-	Elf32_Ehdr *ehdr; /* Elf header structure pointer */
-	Elf32_Phdr *phdr; /* Program header structure pointer */
-	int i;
-
-	ehdr = (Elf32_Ehdr *)addr;
-	if (ehdr->e_ident[EI_CLASS] == ELFCLASS64)
-		return load_elf64_image_phdr(addr);
-
-	phdr = (Elf32_Phdr *)(addr + ehdr->e_phoff);
-
-	/* Load each program header */
-	for (i = 0; i < ehdr->e_phnum; ++i) {
-		void *dst = (void *)(uintptr_t)phdr->p_paddr;
-		void *src = (void *)addr + phdr->p_offset;
-
-		debug("Loading phdr %i to 0x%p (%i bytes)\n",
-		      i, dst, phdr->p_filesz);
-		if (phdr->p_filesz)
-			memcpy(dst, src, phdr->p_filesz);
-		if (phdr->p_filesz != phdr->p_memsz)
-			memset(dst + phdr->p_filesz, 0x00,
-			       phdr->p_memsz - phdr->p_filesz);
-		flush_cache(rounddown((unsigned long)dst, ARCH_DMA_MINALIGN),
-			    roundup(phdr->p_memsz, ARCH_DMA_MINALIGN));
-		++phdr;
-	}
-
-	return ehdr->e_entry;
-}
-
-static unsigned long load_elf_image_shdr(unsigned long addr)
-{
-	Elf32_Ehdr *ehdr; /* Elf header structure pointer */
-	Elf32_Shdr *shdr; /* Section header structure pointer */
-	unsigned char *strtab = 0; /* String table pointer */
-	unsigned char *image; /* Binary image pointer */
-	int i; /* Loop counter */
-
-	ehdr = (Elf32_Ehdr *)addr;
-	if (ehdr->e_ident[EI_CLASS] == ELFCLASS64)
-		return load_elf64_image_shdr(addr);
-
-	/* Find the section header string table for output info */
-	shdr = (Elf32_Shdr *)(addr + ehdr->e_shoff +
-			     (ehdr->e_shstrndx * sizeof(Elf32_Shdr)));
-
-	if (shdr->sh_type == SHT_STRTAB)
-		strtab = (unsigned char *)(addr + shdr->sh_offset);
-
-	/* Load each appropriate section */
-	for (i = 0; i < ehdr->e_shnum; ++i) {
-		shdr = (Elf32_Shdr *)(addr + ehdr->e_shoff +
-				     (i * sizeof(Elf32_Shdr)));
-
-		if (!(shdr->sh_flags & SHF_ALLOC) ||
-		    shdr->sh_addr == 0 || shdr->sh_size == 0) {
-			continue;
-		}
-
-		if (strtab) {
-			debug("%sing %s @ 0x%08lx (%ld bytes)\n",
-			      (shdr->sh_type == SHT_NOBITS) ? "Clear" : "Load",
-			       &strtab[shdr->sh_name],
-			       (unsigned long)shdr->sh_addr,
-			       (long)shdr->sh_size);
-		}
-
-		if (shdr->sh_type == SHT_NOBITS) {
-			memset((void *)(uintptr_t)shdr->sh_addr, 0,
-			       shdr->sh_size);
-		} else {
-			image = (unsigned char *)addr + shdr->sh_offset;
-			memcpy((void *)(uintptr_t)shdr->sh_addr,
-			       (const void *)image, shdr->sh_size);
-		}
-		flush_cache(rounddown(shdr->sh_addr, ARCH_DMA_MINALIGN),
-			    roundup((shdr->sh_addr + shdr->sh_size),
-				    ARCH_DMA_MINALIGN) -
-			    rounddown(shdr->sh_addr, ARCH_DMA_MINALIGN));
-	}
-
-	return ehdr->e_entry;
-}
-
 /* Allow ports to override the default behavior */
 static unsigned long do_bootelf_exec(ulong (*entry)(int, char * const[]),
 				     int argc, char * const argv[])
@@ -246,30 +41,6 @@ static unsigned long do_bootelf_exec(ulong (*entry)(int, char * const[]),
 	return ret;
 }
 
-/*
- * Determine if a valid ELF image exists at the given memory location.
- * First look at the ELF header magic field, then make sure that it is
- * executable.
- */
-int valid_elf_image(unsigned long addr)
-{
-	Elf32_Ehdr *ehdr; /* Elf header structure pointer */
-
-	ehdr = (Elf32_Ehdr *)addr;
-
-	if (!IS_ELF(*ehdr)) {
-		printf("## No elf image at address 0x%08lx\n", addr);
-		return 0;
-	}
-
-	if (ehdr->e_type != ET_EXEC) {
-		printf("## Not a 32-bit elf image at address 0x%08lx\n", addr);
-		return 0;
-	}
-
-	return 1;
-}
-
 /* Interpreter command to boot an arbitrary ELF image from memory */
 int do_bootelf(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
diff --git a/include/elf.h b/include/elf.h
index 81f40191d7..e7c51986df 100644
--- a/include/elf.h
+++ b/include/elf.h
@@ -692,6 +692,10 @@ unsigned long elf_hash(const unsigned char *name);
 
 #ifndef __ASSEMBLER__
 int valid_elf_image(unsigned long addr);
+unsigned long load_elf64_image_phdr(unsigned long addr);
+unsigned long load_elf64_image_shdr(unsigned long addr);
+unsigned long load_elf_image_phdr(unsigned long addr);
+unsigned long load_elf_image_shdr(unsigned long addr);
 #endif
 
 #endif /* _ELF_H */
diff --git a/lib/Kconfig b/lib/Kconfig
index d040a87d26..b155ced4b2 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -601,4 +601,7 @@ config TEST_FDTDEC
 config LIB_DATE
 	bool
 
+config ELF
+	bool "enable basic elf loading/validating functions"
+
 endmenu
diff --git a/lib/Makefile b/lib/Makefile
index 51eba80b89..cef6db1eb8 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -122,6 +122,7 @@ obj-y += vsprintf.o strto.o
 endif
 
 obj-y += date.o
+obj-$(CONFIG_ELF) += elf.o
 
 #
 # Build a fast OID lookup registry from include/linux/oid_registry.h
diff --git a/lib/elf.c b/lib/elf.c
new file mode 100644
index 0000000000..54ac4ee502
--- /dev/null
+++ b/lib/elf.c
@@ -0,0 +1,256 @@
+/*
+ * Copyright (c) 2001 William L. Pitts
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms are freely
+ * permitted provided that the above copyright notice and this
+ * paragraph and the following disclaimer are duplicated in all
+ * such forms.
+ *
+ * This software is provided "AS IS" and without any express or
+ * implied warranties, including, without limitation, the implied
+ * warranties of merchantability and fitness for a particular
+ * purpose.
+ */
+
+#include <common.h>
+#include <command.h>
+#include <cpu_func.h>
+#include <elf.h>
+#include <env.h>
+#include <net.h>
+#include <vxworks.h>
+#ifdef CONFIG_X86
+#include <vbe.h>
+#include <asm/e820.h>
+#include <linux/linkage.h>
+#endif
+
+/*
+ * A very simple ELF64 loader, assumes the image is valid, returns the
+ * entry point address.
+ *
+ * Note if U-Boot is 32-bit, the loader assumes the to segment's
+ * physical address and size is within the lower 32-bit address space.
+ */
+unsigned long load_elf64_image_phdr(unsigned long addr)
+{
+	Elf64_Ehdr *ehdr; /* Elf header structure pointer */
+	Elf64_Phdr *phdr; /* Program header structure pointer */
+	int i;
+
+	ehdr = (Elf64_Ehdr *)addr;
+	phdr = (Elf64_Phdr *)(addr + (ulong)ehdr->e_phoff);
+
+	/* Load each program header */
+	for (i = 0; i < ehdr->e_phnum; ++i) {
+		void *dst = (void *)(ulong)phdr->p_paddr;
+		void *src = (void *)addr + phdr->p_offset;
+
+		debug("Loading phdr %i to 0x%p (%lu bytes)\n",
+		      i, dst, (ulong)phdr->p_filesz);
+		if (phdr->p_filesz)
+			memcpy(dst, src, phdr->p_filesz);
+		if (phdr->p_filesz != phdr->p_memsz)
+			memset(dst + phdr->p_filesz, 0x00,
+			       phdr->p_memsz - phdr->p_filesz);
+		flush_cache(rounddown((unsigned long)dst, ARCH_DMA_MINALIGN),
+			    roundup(phdr->p_memsz, ARCH_DMA_MINALIGN));
+		++phdr;
+	}
+
+	if (ehdr->e_machine == EM_PPC64 && (ehdr->e_flags &
+					    EF_PPC64_ELFV1_ABI)) {
+		/*
+		 * For the 64-bit PowerPC ELF V1 ABI, e_entry is a function
+		 * descriptor pointer with the first double word being the
+		 * address of the entry point of the function.
+		 */
+		uintptr_t addr = ehdr->e_entry;
+
+		return *(Elf64_Addr *)addr;
+	}
+
+	return ehdr->e_entry;
+}
+
+unsigned long load_elf64_image_shdr(unsigned long addr)
+{
+	Elf64_Ehdr *ehdr; /* Elf header structure pointer */
+	Elf64_Shdr *shdr; /* Section header structure pointer */
+	unsigned char *strtab = 0; /* String table pointer */
+	unsigned char *image; /* Binary image pointer */
+	int i; /* Loop counter */
+
+	ehdr = (Elf64_Ehdr *)addr;
+
+	/* Find the section header string table for output info */
+	shdr = (Elf64_Shdr *)(addr + (ulong)ehdr->e_shoff +
+			     (ehdr->e_shstrndx * sizeof(Elf64_Shdr)));
+
+	if (shdr->sh_type == SHT_STRTAB)
+		strtab = (unsigned char *)(addr + (ulong)shdr->sh_offset);
+
+	/* Load each appropriate section */
+	for (i = 0; i < ehdr->e_shnum; ++i) {
+		shdr = (Elf64_Shdr *)(addr + (ulong)ehdr->e_shoff +
+				     (i * sizeof(Elf64_Shdr)));
+
+		if (!(shdr->sh_flags & SHF_ALLOC) ||
+		    shdr->sh_addr == 0 || shdr->sh_size == 0) {
+			continue;
+		}
+
+		if (strtab) {
+			debug("%sing %s @ 0x%08lx (%ld bytes)\n",
+			      (shdr->sh_type == SHT_NOBITS) ? "Clear" : "Load",
+			       &strtab[shdr->sh_name],
+			       (unsigned long)shdr->sh_addr,
+			       (long)shdr->sh_size);
+		}
+
+		if (shdr->sh_type == SHT_NOBITS) {
+			memset((void *)(uintptr_t)shdr->sh_addr, 0,
+			       shdr->sh_size);
+		} else {
+			image = (unsigned char *)addr + (ulong)shdr->sh_offset;
+			memcpy((void *)(uintptr_t)shdr->sh_addr,
+			       (const void *)image, shdr->sh_size);
+		}
+		flush_cache(rounddown(shdr->sh_addr, ARCH_DMA_MINALIGN),
+			    roundup((shdr->sh_addr + shdr->sh_size),
+				     ARCH_DMA_MINALIGN) -
+			            rounddown(shdr->sh_addr, ARCH_DMA_MINALIGN));
+	}
+
+	if (ehdr->e_machine == EM_PPC64 && (ehdr->e_flags &
+					    EF_PPC64_ELFV1_ABI)) {
+		/*
+		 * For the 64-bit PowerPC ELF V1 ABI, e_entry is a function
+		 * descriptor pointer with the first double word being the
+		 * address of the entry point of the function.
+		 */
+		uintptr_t addr = ehdr->e_entry;
+
+		return *(Elf64_Addr *)addr;
+	}
+
+	return ehdr->e_entry;
+}
+
+/*
+ * A very simple ELF loader, assumes the image is valid, returns the
+ * entry point address.
+ *
+ * The loader firstly reads the EFI class to see if it's a 64-bit image.
+ * If yes, call the ELF64 loader. Otherwise continue with the ELF32 loader.
+ */
+unsigned long load_elf_image_phdr(unsigned long addr)
+{
+	Elf32_Ehdr *ehdr; /* Elf header structure pointer */
+	Elf32_Phdr *phdr; /* Program header structure pointer */
+	int i;
+
+	ehdr = (Elf32_Ehdr *)addr;
+	if (ehdr->e_ident[EI_CLASS] == ELFCLASS64)
+		return load_elf64_image_phdr(addr);
+
+	phdr = (Elf32_Phdr *)(addr + ehdr->e_phoff);
+
+	/* Load each program header */
+	for (i = 0; i < ehdr->e_phnum; ++i) {
+		void *dst = (void *)(uintptr_t)phdr->p_paddr;
+		void *src = (void *)addr + phdr->p_offset;
+
+		debug("Loading phdr %i to 0x%p (%i bytes)\n",
+		      i, dst, phdr->p_filesz);
+		if (phdr->p_filesz)
+			memcpy(dst, src, phdr->p_filesz);
+		if (phdr->p_filesz != phdr->p_memsz)
+			memset(dst + phdr->p_filesz, 0x00,
+			       phdr->p_memsz - phdr->p_filesz);
+		flush_cache(rounddown((unsigned long)dst, ARCH_DMA_MINALIGN),
+			    roundup(phdr->p_memsz, ARCH_DMA_MINALIGN));
+		++phdr;
+	}
+
+	return ehdr->e_entry;
+}
+
+unsigned long load_elf_image_shdr(unsigned long addr)
+{
+	Elf32_Ehdr *ehdr; /* Elf header structure pointer */
+	Elf32_Shdr *shdr; /* Section header structure pointer */
+	unsigned char *strtab = 0; /* String table pointer */
+	unsigned char *image; /* Binary image pointer */
+	int i; /* Loop counter */
+
+	ehdr = (Elf32_Ehdr *)addr;
+	if (ehdr->e_ident[EI_CLASS] == ELFCLASS64)
+		return load_elf64_image_shdr(addr);
+
+	/* Find the section header string table for output info */
+	shdr = (Elf32_Shdr *)(addr + ehdr->e_shoff +
+			     (ehdr->e_shstrndx * sizeof(Elf32_Shdr)));
+
+	if (shdr->sh_type == SHT_STRTAB)
+		strtab = (unsigned char *)(addr + shdr->sh_offset);
+
+	/* Load each appropriate section */
+	for (i = 0; i < ehdr->e_shnum; ++i) {
+		shdr = (Elf32_Shdr *)(addr + ehdr->e_shoff +
+				     (i * sizeof(Elf32_Shdr)));
+
+		if (!(shdr->sh_flags & SHF_ALLOC) ||
+		    shdr->sh_addr == 0 || shdr->sh_size == 0) {
+			continue;
+		}
+
+		if (strtab) {
+			debug("%sing %s @ 0x%08lx (%ld bytes)\n",
+			      (shdr->sh_type == SHT_NOBITS) ? "Clear" : "Load",
+			       &strtab[shdr->sh_name],
+			       (unsigned long)shdr->sh_addr,
+			       (long)shdr->sh_size);
+		}
+
+		if (shdr->sh_type == SHT_NOBITS) {
+			memset((void *)(uintptr_t)shdr->sh_addr, 0,
+			       shdr->sh_size);
+		} else {
+			image = (unsigned char *)addr + shdr->sh_offset;
+			memcpy((void *)(uintptr_t)shdr->sh_addr,
+			       (const void *)image, shdr->sh_size);
+		}
+		flush_cache(rounddown(shdr->sh_addr, ARCH_DMA_MINALIGN),
+			    roundup((shdr->sh_addr + shdr->sh_size),
+				    ARCH_DMA_MINALIGN) -
+			    rounddown(shdr->sh_addr, ARCH_DMA_MINALIGN));
+	}
+
+	return ehdr->e_entry;
+}
+
+/*
+ * Determine if a valid ELF image exists at the given memory location.
+ * First look at the ELF header magic field, then make sure that it is
+ * executable.
+ */
+int valid_elf_image(unsigned long addr)
+{
+	Elf32_Ehdr *ehdr; /* Elf header structure pointer */
+
+	ehdr = (Elf32_Ehdr *)addr;
+
+	if (!IS_ELF(*ehdr)) {
+		printf("## No elf image at address 0x%08lx\n", addr);
+		return 0;
+	}
+
+	if (ehdr->e_type != ET_EXEC) {
+		printf("## Not a 32-bit elf image at address 0x%08lx\n", addr);
+		return 0;
+	}
+
+	return 1;
+}
-- 
2.17.1

[PATCH v3 02/10] arm: k3: Add support for loading non linux remote cores

[Keerthy]

Add MAIN domain R5FSS0 remoteproc support from spl. This enables
loading the elf firmware in SPL and starting the remotecore.

In order to start the core, there should be a file with path
"/lib/firmware/j7-main-r5f0_0-fw" under filesystem
of respective boot mode.

Signed-off-by: Keerthy <j-keerthy@ti.com>
Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
[Guard start_non_linux_remote_cores under CONFIG_FS_LOADER]
Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
 arch/arm/mach-k3/common.c     | 84 ++++++++++++++++++++++++++++++++---
 arch/arm/mach-k3/common.h     |  2 +
 arch/arm/mach-k3/j721e_init.c | 34 ++++++++++++++
 3 files changed, 115 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
index 8d1529062d..f0ac0c39f1 100644
--- a/arch/arm/mach-k3/common.c
+++ b/arch/arm/mach-k3/common.c
@@ -16,6 +16,10 @@
 #include <asm/arch/sys_proto.h>
 #include <asm/hardware.h>
 #include <asm/io.h>
+#include <fs_loader.h>
+#include <fs.h>
+#include <env.h>
+#include <elf.h>
 
 struct ti_sci_handle *get_ti_sci_handle(void)
 {
@@ -57,6 +61,74 @@ int early_console_init(void)
 #endif
 
 #ifdef CONFIG_SYS_K3_SPL_ATF
+
+void init_env(void)
+{
+#ifdef CONFIG_SPL_ENV_SUPPORT
+	char *part;
+
+	env_init();
+	env_load();
+	switch (spl_boot_device()) {
+	case BOOT_DEVICE_MMC2:
+		part = env_get("bootpart");
+		env_set("storage_interface", "mmc");
+		env_set("fw_dev_part", part);
+		break;
+	case BOOT_DEVICE_SPI:
+		env_set("storage_interface", "ubi");
+		env_set("fw_ubi_mtdpart", "UBI");
+		env_set("fw_ubi_volume", "UBI0");
+		break;
+	default:
+		printf("%s from device %u not supported!\n",
+		       __func__, spl_boot_device());
+		return;
+	}
+#endif
+}
+
+#ifdef CONFIG_FS_LOADER
+int load_firmware(char *name_fw, char *name_loadaddr, u32 *loadaddr)
+{
+	struct udevice *fsdev;
+	char *name = NULL;
+	int size = 0;
+
+	*loadaddr = 0;
+#ifdef CONFIG_SPL_ENV_SUPPORT
+	switch (spl_boot_device()) {
+	case BOOT_DEVICE_MMC2:
+		name = env_get(name_fw);
+		*loadaddr = env_get_hex(name_loadaddr, *loadaddr);
+		break;
+	default:
+		printf("Loading rproc fw image from device %u not supported!\n",
+		       spl_boot_device());
+		return 0;
+	}
+#endif
+	if (!*loadaddr)
+		return 0;
+
+	if (!uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0, &fsdev)) {
+		size = request_firmware_into_buf(fsdev, name, (void *)*loadaddr,
+						 0, 0);
+	}
+
+	return size;
+}
+#else
+int load_firmware(char *name_fw, char *name_loadaddr, u32 *loadaddr)
+{
+	return 0;
+}
+#endif
+
+__weak void start_non_linux_remote_cores(void)
+{
+}
+
 void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
 {
 	struct ti_sci_handle *ti_sci = get_ti_sci_handle();
@@ -65,15 +137,17 @@ void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
 	/* Release all the exclusive devices held by SPL before starting ATF */
 	ti_sci->ops.dev_ops.release_exclusive_devices(ti_sci);
 
+	ret = rproc_init();
+	if (ret)
+		panic("rproc failed to be initialized (%d)\n", ret);
+
+	init_env();
+	start_non_linux_remote_cores();
+
 	/*
 	 * It is assumed that remoteproc device 1 is the corresponding
 	 * Cortex-A core which runs ATF. Make sure DT reflects the same.
 	 */
-	ret = rproc_dev_init(1);
-	if (ret)
-		panic("%s: ATF failed to initialize on rproc (%d)\n", __func__,
-		      ret);
-
 	ret = rproc_load(1, spl_image->entry_point, 0x200);
 	if (ret)
 		panic("%s: ATF failed to load on rproc (%d)\n", __func__, ret);
diff --git a/arch/arm/mach-k3/common.h b/arch/arm/mach-k3/common.h
index d8b34fe060..42fb8ee6e7 100644
--- a/arch/arm/mach-k3/common.h
+++ b/arch/arm/mach-k3/common.h
@@ -24,3 +24,5 @@ void setup_k3_mpu_regions(void);
 int early_console_init(void);
 void disable_linefill_optimization(void);
 void remove_fwl_configs(struct fwl_data *fwl_data, size_t fwl_data_size);
+void start_non_linux_remote_cores(void);
+int load_firmware(char *name_fw, char *name_loadaddr, u32 *loadaddr);
diff --git a/arch/arm/mach-k3/j721e_init.c b/arch/arm/mach-k3/j721e_init.c
index f7f7398081..c5f8ede1a0 100644
--- a/arch/arm/mach-k3/j721e_init.c
+++ b/arch/arm/mach-k3/j721e_init.c
@@ -18,6 +18,7 @@
 #include <dm.h>
 #include <dm/uclass-internal.h>
 #include <dm/pinctrl.h>
+#include <remoteproc.h>
 
 #ifdef CONFIG_SPL_BUILD
 #ifdef CONFIG_K3_LOAD_SYSFW
@@ -295,3 +296,36 @@ void release_resources_for_core_shutdown(void)
 	}
 }
 #endif
+
+#ifdef CONFIG_SYS_K3_SPL_ATF
+void start_non_linux_remote_cores(void)
+{
+	int size = 0, ret;
+	u32 loadaddr = 0;
+
+	size = load_firmware("mainr5f0_0fwname", "mainr5f0_0loadaddr",
+			     &loadaddr);
+	if (size <= 0)
+		goto err_load;
+
+	/* assuming remoteproc 2 is aliased for the needed remotecore */
+	ret = rproc_load(2, loadaddr, size);
+	if (ret) {
+		printf("Firmware failed to start on rproc (%d)\n", ret);
+		goto err_load;
+	}
+
+	ret = rproc_start(2);
+	if (ret) {
+		printf("Firmware init failed on rproc (%d)\n", ret);
+		goto err_load;
+	}
+
+	printf("Remoteproc 2 started successfully\n");
+
+	return;
+
+err_load:
+	rproc_reset(2);
+}
+#endif
-- 
2.17.1

[PATCH v3 03/10] armv7R: K3: r5_mpu: Enable execute permission for MCU0 BTCM

[Keerthy]

Enable execute permission for mcu_r5fss0_core0 BTCM so that we can jump
to a firmware directly from SPL.

Signed-off-by: Keerthy <j-keerthy@ti.com>
---
 arch/arm/mach-k3/r5_mpu.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-k3/r5_mpu.c b/arch/arm/mach-k3/r5_mpu.c
index ee076ed877..3d2ff6775a 100644
--- a/arch/arm/mach-k3/r5_mpu.c
+++ b/arch/arm/mach-k3/r5_mpu.c
@@ -26,7 +26,9 @@ struct mpu_region_config k3_mpu_regions[16] = {
 	/* U-Boot's code area marking it as WB and Write allocate */
 	{CONFIG_SYS_SDRAM_BASE, REGION_2, XN_DIS, PRIV_RW_USR_RW,
 	 O_I_WB_RD_WR_ALLOC, REGION_2GB},
-	{0x0, 3, 0x0, 0x0, 0x0, 0x0},
+	/* mcu_r5fss0_core0 BTCM area marking it as WB and Write allocate. */
+	{0x41010000, 3, XN_DIS, PRIV_RW_USR_RW, O_I_WB_RD_WR_ALLOC,
+	 REGION_8MB},
 	{0x0, 4, 0x0, 0x0, 0x0, 0x0},
 	{0x0, 5, 0x0, 0x0, 0x0, 0x0},
 	{0x0, 6, 0x0, 0x0, 0x0, 0x0},
-- 
2.17.1

[PATCH v3 04/10] armv7R: K3: Add support for jumping to firmware

[Keerthy]

MCU Domain rf50 is currently shutting down after loading the ATF.
Load elf firmware and jump to firmware post loading ATF.

ROM doesn't enable ATCM memory, so make sure that firmware that
is being loaded doesn't use ATCM memory or override SPL.

Signed-off-by: Keerthy <j-keerthy@ti.com>
Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 arch/arm/mach-k3/common.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
index f0ac0c39f1..809e23bb86 100644
--- a/arch/arm/mach-k3/common.c
+++ b/arch/arm/mach-k3/common.c
@@ -131,8 +131,10 @@ __weak void start_non_linux_remote_cores(void)
 
 void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
 {
+	typedef void __noreturn (*image_entry_noargs_t)(void);
 	struct ti_sci_handle *ti_sci = get_ti_sci_handle();
-	int ret;
+	u32 loadaddr = 0;
+	int ret, size;
 
 	/* Release all the exclusive devices held by SPL before starting ATF */
 	ti_sci->ops.dev_ops.release_exclusive_devices(ti_sci);
@@ -143,6 +145,9 @@ void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
 
 	init_env();
 	start_non_linux_remote_cores();
+	size = load_firmware("mcur5f0_0fwname", "mcur5f0_0loadaddr",
+			     &loadaddr);
+
 
 	/*
 	 * It is assumed that remoteproc device 1 is the corresponding
@@ -158,13 +163,18 @@ void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
 	ret = rproc_start(1);
 	if (ret)
 		panic("%s: ATF failed to start on rproc (%d)\n", __func__, ret);
+	if (!(size > 0 && valid_elf_image(loadaddr))) {
+		debug("Shutting down...\n");
+		release_resources_for_core_shutdown();
+
+		while (1)
+			asm volatile("wfe");
+	}
 
-	debug("Releasing resources...\n");
-	release_resources_for_core_shutdown();
+	image_entry_noargs_t image_entry =
+		(image_entry_noargs_t)load_elf_image_phdr(loadaddr);
 
-	debug("Finalizing core shutdown...\n");
-	while (1)
-		asm volatile("wfe");
+	image_entry();
 }
 #endif
 
-- 
2.17.1

[PATCH v3 05/10] arm: dts: k3-j721e-r5: Add fs_loader node

[Keerthy]

Add fs_loader node which will be needed for loading firmwares
from the boot media/filesystem.

Signed-off-by: Keerthy <j-keerthy@ti.com>
Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 arch/arm/dts/k3-j721e-r5-common-proc-board.dts | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/dts/k3-j721e-r5-common-proc-board.dts b/arch/arm/dts/k3-j721e-r5-common-proc-board.dts
index 28a355d49c..caeee8defe 100644
--- a/arch/arm/dts/k3-j721e-r5-common-proc-board.dts
+++ b/arch/arm/dts/k3-j721e-r5-common-proc-board.dts
@@ -18,6 +18,12 @@
 	chosen {
 		stdout-path = "serial2:115200n8";
 		tick-timer = &timer1;
+		firmware-loader = &fs_loader0;
+	};
+
+	fs_loader0: fs_loader at 0 {
+		u-boot,dm-pre-reloc;
+		compatible = "u-boot,fs-loader";
 	};
 
 	a72_0: a72 at 0 {
-- 
2.17.1

[PATCH v3 06/10] arm: dts: k3-j721e-r5: Enable r5fss0 cluster in SPL

[Keerthy]

Enable MAIN domain r5fss0 cluster and its core0 in R5 spl.

Signed-off-by: Keerthy <j-keerthy@ti.com>
Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 arch/arm/dts/k3-j721e-r5-common-proc-board.dts | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm/dts/k3-j721e-r5-common-proc-board.dts b/arch/arm/dts/k3-j721e-r5-common-proc-board.dts
index caeee8defe..d9f33bf4a4 100644
--- a/arch/arm/dts/k3-j721e-r5-common-proc-board.dts
+++ b/arch/arm/dts/k3-j721e-r5-common-proc-board.dts
@@ -13,6 +13,8 @@
 	aliases {
 		remoteproc0 = &sysctrler;
 		remoteproc1 = &a72_0;
+		remoteproc2 = &main_r5fss0_core0;
+		remoteproc3 = &main_r5fss0_core1;
 	};
 
 	chosen {
@@ -213,4 +215,16 @@
 	u-boot,dm-spl;
 };
 
+&main_r5fss0 {
+	u-boot,dm-spl;
+};
+
+&main_r5fss0_core0 {
+	u-boot,dm-spl;
+};
+
+&main_r5fss0_core1 {
+	u-boot,dm-spl;
+};
+
 #include "k3-j721e-common-proc-board-u-boot.dtsi"
-- 
2.17.1

[PATCH v3 07/10] include: configs: j721e_evm: Add env variables for mcu_r5fss0_core0 & main_r5fss0_core0

[Keerthy]

Add env variables for mcu_r5fss0_core0 & main_r5fss0_core0 firmware
loadaddr and name.

Signed-off-by: Keerthy <j-keerthy@ti.com>
---
 include/configs/j721e_evm.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/configs/j721e_evm.h b/include/configs/j721e_evm.h
index eaed520e6b..265239e32a 100644
--- a/include/configs/j721e_evm.h
+++ b/include/configs/j721e_evm.h
@@ -84,6 +84,10 @@
 	"mmcdev=1\0"							\
 	"bootpart=1:2\0"						\
 	"bootdir=/boot\0"						\
+	"mainr5f0_0loadaddr=88000000\0"					\
+	"mainr5f0_0fwname=/lib/firmware/j7-main-r5f0_0-fw\0"		\
+	"mcur5f0_0loadaddr=89000000\0"					\
+	"mcur5f0_0fwname=/lib/firmware/j7-mcu-r5f0_0-fw\0"		\
 	"rd_spec=-\0"							\
 	"init_mmc=run args_all args_mmc\0"				\
 	"get_fdt_mmc=load mmc ${bootpart} ${fdtaddr} ${bootdir}/${name_fdt}\0" \
-- 
2.17.1

[PATCH v3 08/10] configs: j721e_evm_r5: Enable R5F remoteproc support

[Keerthy]

Enable r5f remoteproc support in r5 defconfig so that r5s can
be started in spl.

Signed-off-by: Keerthy <j-keerthy@ti.com>
Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 configs/j721e_evm_r5_defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/configs/j721e_evm_r5_defconfig b/configs/j721e_evm_r5_defconfig
index cb6c74d7bf..0925690e10 100644
--- a/configs/j721e_evm_r5_defconfig
+++ b/configs/j721e_evm_r5_defconfig
@@ -25,6 +25,7 @@ CONFIG_SPL_STACK_R=y
 CONFIG_SPL_SEPARATE_BSS=y
 CONFIG_SPL_EARLY_BSS=y
 CONFIG_SPL_ENV_SUPPORT=y
+CONFIG_SPL_FS_EXT4=y
 CONFIG_SPL_I2C_SUPPORT=y
 CONFIG_SPL_DM_MAILBOX=y
 CONFIG_SPL_DM_RESET=y
@@ -92,6 +93,7 @@ CONFIG_SPL_DM_REGULATOR=y
 CONFIG_DM_REGULATOR_TPS65941=y
 CONFIG_K3_SYSTEM_CONTROLLER=y
 CONFIG_REMOTEPROC_TI_K3_ARM64=y
+CONFIG_REMOTEPROC_TI_K3_R5F=y
 CONFIG_DM_RESET=y
 CONFIG_RESET_TI_SCI=y
 CONFIG_DM_SERIAL=y
-- 
2.17.1

[PATCH v3 09/10] configs: j721e_evm_r5_defconfig: Remove saving ENV in eMMC

[Keerthy]

Remove saving ENV in eMMC in r5 as the power domains are not
setup. Environment in eMMC cannot be read if we do not boot from
eMMC.

Signed-off-by: Keerthy <j-keerthy@ti.com>
---
 configs/j721e_evm_r5_defconfig | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/configs/j721e_evm_r5_defconfig b/configs/j721e_evm_r5_defconfig
index 0925690e10..0f391ff98e 100644
--- a/configs/j721e_evm_r5_defconfig
+++ b/configs/j721e_evm_r5_defconfig
@@ -7,7 +7,6 @@ CONFIG_SYS_MALLOC_F_LEN=0x55000
 CONFIG_SOC_K3_J721E=y
 CONFIG_TARGET_J721E_R5_EVM=y
 CONFIG_ENV_SIZE=0x20000
-CONFIG_ENV_OFFSET=0x680000
 CONFIG_SPL_MMC_SUPPORT=y
 CONFIG_SPL_SERIAL_SUPPORT=y
 CONFIG_SPL_DRIVERS_MISC_SUPPORT=y
@@ -48,9 +47,6 @@ CONFIG_CMD_FAT=y
 CONFIG_OF_CONTROL=y
 CONFIG_SPL_OF_CONTROL=y
 CONFIG_DEFAULT_DEVICE_TREE="k3-j721e-r5-common-proc-board"
-CONFIG_ENV_IS_IN_MMC=y
-CONFIG_SYS_REDUNDAND_ENVIRONMENT=y
-CONFIG_ENV_OFFSET_REDUND=0x700000
 CONFIG_SYS_RELOC_GD_ENV_ADDR=y
 CONFIG_DM=y
 CONFIG_SPL_DM=y
-- 
2.17.1

[PATCH v3 10/10] env: nowhere: set default enviroment

[Keerthy]

set default enviroment so that set_env calls succeed
when ENV_IS_NOWHERE is alone set.

Signed-off-by: Keerthy <j-keerthy@ti.com>
---
 env/nowhere.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/env/nowhere.c b/env/nowhere.c
index f5b0a17652..70c3b3e011 100644
--- a/env/nowhere.c
+++ b/env/nowhere.c
@@ -23,6 +23,7 @@ static int env_nowhere_init(void)
 {
 	gd->env_addr	= (ulong)&default_environment[0];
 	gd->env_valid	= ENV_INVALID;
+	env_set_default(NULL, 0);
 
 	return 0;
 }
-- 
2.17.1

[PATCH v3 01/10] lib: elf: Move the generic elf loading/validating functions to lib

[Andrew F. Davis]

On 1/21/20 6:07 AM, Keerthy wrote:
> Move the generic elf loading/validating functions to lib/
> so that they can be re-used and accessed by code existing
> outside cmd.
> 
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> Suggested-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> ---
> diff --git a/include/elf.h b/include/elf.h
> index 81f40191d7..e7c51986df 100644
> --- a/include/elf.h
> +++ b/include/elf.h
> @@ -692,6 +692,10 @@ unsigned long elf_hash(const unsigned char *name);
>  
>  #ifndef __ASSEMBLER__
>  int valid_elf_image(unsigned long addr);
> +unsigned long load_elf64_image_phdr(unsigned long addr);
> +unsigned long load_elf64_image_shdr(unsigned long addr);
> +unsigned long load_elf_image_phdr(unsigned long addr);
> +unsigned long load_elf_image_shdr(unsigned long addr);
>  #endif
>  
>  #endif /* _ELF_H */
> diff --git a/lib/Kconfig b/lib/Kconfig
> index d040a87d26..b155ced4b2 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -601,4 +601,7 @@ config TEST_FDTDEC
>  config LIB_DATE
>  	bool
>  
> +config ELF


LIB_ELF?

Andrew

[PATCH v3 02/10] arm: k3: Add support for loading non linux remote cores

[Andrew F. Davis]

On 1/21/20 6:07 AM, Keerthy wrote:
> Add MAIN domain R5FSS0 remoteproc support from spl. This enables
> loading the elf firmware in SPL and starting the remotecore.
> 
> In order to start the core, there should be a file with path
> "/lib/firmware/j7-main-r5f0_0-fw" under filesystem
> of respective boot mode.
> 
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> [Guard start_non_linux_remote_cores under CONFIG_FS_LOADER]
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> ---
>  arch/arm/mach-k3/common.c     | 84 ++++++++++++++++++++++++++++++++---
>  arch/arm/mach-k3/common.h     |  2 +
>  arch/arm/mach-k3/j721e_init.c | 34 ++++++++++++++
>  3 files changed, 115 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
> index 8d1529062d..f0ac0c39f1 100644
> --- a/arch/arm/mach-k3/common.c
> +++ b/arch/arm/mach-k3/common.c
> @@ -16,6 +16,10 @@
>  #include <asm/arch/sys_proto.h>
>  #include <asm/hardware.h>
>  #include <asm/io.h>
> +#include <fs_loader.h>
> +#include <fs.h>
> +#include <env.h>
> +#include <elf.h>
>  
>  struct ti_sci_handle *get_ti_sci_handle(void)
>  {
> @@ -57,6 +61,74 @@ int early_console_init(void)
>  #endif
>  
>  #ifdef CONFIG_SYS_K3_SPL_ATF
> +
> +void init_env(void)
> +{
> +#ifdef CONFIG_SPL_ENV_SUPPORT
> +	char *part;
> +
> +	env_init();
> +	env_load();
> +	switch (spl_boot_device()) {
> +	case BOOT_DEVICE_MMC2:
> +		part = env_get("bootpart");
> +		env_set("storage_interface", "mmc");
> +		env_set("fw_dev_part", part);
> +		break;
> +	case BOOT_DEVICE_SPI:
> +		env_set("storage_interface", "ubi");
> +		env_set("fw_ubi_mtdpart", "UBI");
> +		env_set("fw_ubi_volume", "UBI0");
> +		break;
> +	default:
> +		printf("%s from device %u not supported!\n",
> +		       __func__, spl_boot_device());


This will print for almost every boot mode..


> +		return;
> +	}
> +#endif
> +}
> +
> +#ifdef CONFIG_FS_LOADER
> +int load_firmware(char *name_fw, char *name_loadaddr, u32 *loadaddr)
> +{
> +	struct udevice *fsdev;
> +	char *name = NULL;
> +	int size = 0;
> +
> +	*loadaddr = 0;
> +#ifdef CONFIG_SPL_ENV_SUPPORT
> +	switch (spl_boot_device()) {
> +	case BOOT_DEVICE_MMC2:
> +		name = env_get(name_fw);
> +		*loadaddr = env_get_hex(name_loadaddr, *loadaddr);
> +		break;
> +	default:
> +		printf("Loading rproc fw image from device %u not supported!\n",
> +		       spl_boot_device());


This whole thing seems very MMC specific, if early firmware loading is
important it should work for all boot modes. Find a way to include it in
the next boot stage FIT image (tispl.bin) so it works for all modes.


> +		return 0;
> +	}
> +#endif
> +	if (!*loadaddr)
> +		return 0;
> +
> +	if (!uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0, &fsdev)) {
> +		size = request_firmware_into_buf(fsdev, name, (void *)*loadaddr,
> +						 0, 0);
> +	}
> +
> +	return size;
> +}
> +#else
> +int load_firmware(char *name_fw, char *name_loadaddr, u32 *loadaddr)
> +{
> +	return 0;
> +}
> +#endif
> +
> +__weak void start_non_linux_remote_cores(void)
> +{
> +}
> +
>  void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
>  {
>  	struct ti_sci_handle *ti_sci = get_ti_sci_handle();
> @@ -65,15 +137,17 @@ void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
>  	/* Release all the exclusive devices held by SPL before starting ATF */
>  	ti_sci->ops.dev_ops.release_exclusive_devices(ti_sci);
>  
> +	ret = rproc_init();
> +	if (ret)
> +		panic("rproc failed to be initialized (%d)\n", ret);
> +
> +	init_env();
> +	start_non_linux_remote_cores();
> +
>  	/*
>  	 * It is assumed that remoteproc device 1 is the corresponding
>  	 * Cortex-A core which runs ATF. Make sure DT reflects the same.
>  	 */
> -	ret = rproc_dev_init(1);
> -	if (ret)
> -		panic("%s: ATF failed to initialize on rproc (%d)\n", __func__,
> -		      ret);
> -


Where did this code go?


>  	ret = rproc_load(1, spl_image->entry_point, 0x200);
>  	if (ret)
>  		panic("%s: ATF failed to load on rproc (%d)\n", __func__, ret);
> diff --git a/arch/arm/mach-k3/common.h b/arch/arm/mach-k3/common.h
> index d8b34fe060..42fb8ee6e7 100644
> --- a/arch/arm/mach-k3/common.h
> +++ b/arch/arm/mach-k3/common.h
> @@ -24,3 +24,5 @@ void setup_k3_mpu_regions(void);
>  int early_console_init(void);
>  void disable_linefill_optimization(void);
>  void remove_fwl_configs(struct fwl_data *fwl_data, size_t fwl_data_size);
> +void start_non_linux_remote_cores(void);
> +int load_firmware(char *name_fw, char *name_loadaddr, u32 *loadaddr);
> diff --git a/arch/arm/mach-k3/j721e_init.c b/arch/arm/mach-k3/j721e_init.c
> index f7f7398081..c5f8ede1a0 100644
> --- a/arch/arm/mach-k3/j721e_init.c
> +++ b/arch/arm/mach-k3/j721e_init.c
> @@ -18,6 +18,7 @@
>  #include <dm.h>
>  #include <dm/uclass-internal.h>
>  #include <dm/pinctrl.h>
> +#include <remoteproc.h>
>  
>  #ifdef CONFIG_SPL_BUILD
>  #ifdef CONFIG_K3_LOAD_SYSFW
> @@ -295,3 +296,36 @@ void release_resources_for_core_shutdown(void)
>  	}
>  }
>  #endif
> +
> +#ifdef CONFIG_SYS_K3_SPL_ATF
> +void start_non_linux_remote_cores(void)
> +{
> +	int size = 0, ret;
> +	u32 loadaddr = 0;
> +
> +	size = load_firmware("mainr5f0_0fwname", "mainr5f0_0loadaddr",
> +			     &loadaddr);
> +	if (size <= 0)
> +		goto err_load;
> +
> +	/*  remoteproc 2 is aliased for the needed remotecore */


Assuming the big-arm core to boot is remoteproc 1 was reasonable, but
there needs to be a better what than assuming the number for every other
remote core.


> +	ret = rproc_load(2, loadaddr, size);
> +	if (ret) {
> +		printf("Firmware failed to start on rproc (%d)\n", ret);
> +		goto err_load;
> +	}
> +
> +	ret = rproc_start(2);
> +	if (ret) {
> +		printf("Firmware init failed on rproc (%d)\n", ret);
> +		goto err_load;
> +	}
> +
> +	printf("Remoteproc 2 started successfully\n");


That's useful..

Andrew


> +
> +	return;
> +
> +err_load:
> +	rproc_reset(2);
> +}
> +#endif
> 

[PATCH v3 07/10] include: configs: j721e_evm: Add env variables for mcu_r5fss0_core0 & main_r5fss0_core0

[Andrew F. Davis]

On 1/21/20 6:07 AM, Keerthy wrote:
> Add env variables for mcu_r5fss0_core0 & main_r5fss0_core0 firmware
> loadaddr and name.
> 
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---
>  include/configs/j721e_evm.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/configs/j721e_evm.h b/include/configs/j721e_evm.h
> index eaed520e6b..265239e32a 100644
> --- a/include/configs/j721e_evm.h
> +++ b/include/configs/j721e_evm.h
> @@ -84,6 +84,10 @@
>  	"mmcdev=1\0"							\
>  	"bootpart=1:2\0"						\
>  	"bootdir=/boot\0"						\
> +	"mainr5f0_0loadaddr=88000000\0"					\
> +	"mainr5f0_0fwname=/lib/firmware/j7-main-r5f0_0-fw\0"		\
> +	"mcur5f0_0loadaddr=89000000\0"					\
> +	"mcur5f0_0fwname=/lib/firmware/j7-mcu-r5f0_0-fw\0"		\


New convention for env vars I'm pushing is: names start with "name_" and
addresses start with "addr_". Makes for easier sorting and for figuring
out what each of the large list of env vars does at a glance.

Andrew


>  	"rd_spec=-\0"							\
>  	"init_mmc=run args_all args_mmc\0"				\
>  	"get_fdt_mmc=load mmc ${bootpart} ${fdtaddr} ${bootdir}/${name_fdt}\0" \
> 

[PATCH v3 08/10] configs: j721e_evm_r5: Enable R5F remoteproc support

[Andrew F. Davis]

On 1/21/20 6:07 AM, Keerthy wrote:
> Enable r5f remoteproc support in r5 defconfig so that r5s can
> be started in spl.
> 


s/r5f/R5F
s/r5/R5
s/spl/SPL


> Signed-off-by: Keerthy <j-keerthy@ti.com>
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> ---
>  configs/j721e_evm_r5_defconfig | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/configs/j721e_evm_r5_defconfig b/configs/j721e_evm_r5_defconfig
> index cb6c74d7bf..0925690e10 100644
> --- a/configs/j721e_evm_r5_defconfig
> +++ b/configs/j721e_evm_r5_defconfig
> @@ -25,6 +25,7 @@ CONFIG_SPL_STACK_R=y
>  CONFIG_SPL_SEPARATE_BSS=y
>  CONFIG_SPL_EARLY_BSS=y
>  CONFIG_SPL_ENV_SUPPORT=y
> +CONFIG_SPL_FS_EXT4=y


? Not in commit message.

Andrew


>  CONFIG_SPL_I2C_SUPPORT=y
>  CONFIG_SPL_DM_MAILBOX=y
>  CONFIG_SPL_DM_RESET=y
> @@ -92,6 +93,7 @@ CONFIG_SPL_DM_REGULATOR=y
>  CONFIG_DM_REGULATOR_TPS65941=y
>  CONFIG_K3_SYSTEM_CONTROLLER=y
>  CONFIG_REMOTEPROC_TI_K3_ARM64=y
> +CONFIG_REMOTEPROC_TI_K3_R5F=y
>  CONFIG_DM_RESET=y
>  CONFIG_RESET_TI_SCI=y
>  CONFIG_DM_SERIAL=y
> 

[PATCH v3 10/10] env: nowhere: set default enviroment

[Andrew F. Davis]

On 1/21/20 6:07 AM, Keerthy wrote:
> set default enviroment so that set_env calls succeed
> when ENV_IS_NOWHERE is alone set.
> 


Capitalize first letter of a sentence.

Also no need to line wrap at 50 chars..

For last line:
"when only ENV_IS_NOWHERE is set."
reads better.

Andrew


> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---
>  env/nowhere.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/env/nowhere.c b/env/nowhere.c
> index f5b0a17652..70c3b3e011 100644
> --- a/env/nowhere.c
> +++ b/env/nowhere.c
> @@ -23,6 +23,7 @@ static int env_nowhere_init(void)
>  {
>  	gd->env_addr	= (ulong)&default_environment[0];
>  	gd->env_valid	= ENV_INVALID;
> +	env_set_default(NULL, 0);
>  
>  	return 0;
>  }
> 

[PATCH v3 02/10] arm: k3: Add support for loading non linux remote cores

[keerthy]

On 1/21/2020 6:26 PM, Andrew F. Davis wrote:
> On 1/21/20 6:07 AM, Keerthy wrote:
>> Add MAIN domain R5FSS0 remoteproc support from spl. This enables
>> loading the elf firmware in SPL and starting the remotecore.
>>
>> In order to start the core, there should be a file with path
>> "/lib/firmware/j7-main-r5f0_0-fw" under filesystem
>> of respective boot mode.
>>
>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>> [Guard start_non_linux_remote_cores under CONFIG_FS_LOADER]
>> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
>> ---
>>   arch/arm/mach-k3/common.c     | 84 ++++++++++++++++++++++++++++++++---
>>   arch/arm/mach-k3/common.h     |  2 +
>>   arch/arm/mach-k3/j721e_init.c | 34 ++++++++++++++
>>   3 files changed, 115 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
>> index 8d1529062d..f0ac0c39f1 100644
>> --- a/arch/arm/mach-k3/common.c
>> +++ b/arch/arm/mach-k3/common.c
>> @@ -16,6 +16,10 @@
>>   #include <asm/arch/sys_proto.h>
>>   #include <asm/hardware.h>
>>   #include <asm/io.h>
>> +#include <fs_loader.h>
>> +#include <fs.h>
>> +#include <env.h>
>> +#include <elf.h>
>>   
>>   struct ti_sci_handle *get_ti_sci_handle(void)
>>   {
>> @@ -57,6 +61,74 @@ int early_console_init(void)
>>   #endif
>>   
>>   #ifdef CONFIG_SYS_K3_SPL_ATF
>> +
>> +void init_env(void)
>> +{
>> +#ifdef CONFIG_SPL_ENV_SUPPORT
>> +	char *part;
>> +
>> +	env_init();
>> +	env_load();
>> +	switch (spl_boot_device()) {
>> +	case BOOT_DEVICE_MMC2:
>> +		part = env_get("bootpart");
>> +		env_set("storage_interface", "mmc");
>> +		env_set("fw_dev_part", part);
>> +		break;
>> +	case BOOT_DEVICE_SPI:
>> +		env_set("storage_interface", "ubi");
>> +		env_set("fw_ubi_mtdpart", "UBI");
>> +		env_set("fw_ubi_volume", "UBI0");
>> +		break;
>> +	default:
>> +		printf("%s from device %u not supported!\n",
>> +		       __func__, spl_boot_device());
> 
> 
> This will print for almost every boot mode..

I can keep this under debug.

> 
> 
>> +		return;
>> +	}
>> +#endif
>> +}
>> +
>> +#ifdef CONFIG_FS_LOADER
>> +int load_firmware(char *name_fw, char *name_loadaddr, u32 *loadaddr)
>> +{
>> +	struct udevice *fsdev;
>> +	char *name = NULL;
>> +	int size = 0;
>> +
>> +	*loadaddr = 0;
>> +#ifdef CONFIG_SPL_ENV_SUPPORT
>> +	switch (spl_boot_device()) {
>> +	case BOOT_DEVICE_MMC2:
>> +		name = env_get(name_fw);
>> +		*loadaddr = env_get_hex(name_loadaddr, *loadaddr);
>> +		break;
>> +	default:
>> +		printf("Loading rproc fw image from device %u not supported!\n",
>> +		       spl_boot_device());
> 
> 
> This whole thing seems very MMC specific, if early firmware loading is
> important it should work for all boot modes. Find a way to include it in
> the next boot stage FIT image (tispl.bin) so it works for all modes.

That was not NAKd. We are going with fs_loader approach.

> 
> 
>> +		return 0;
>> +	}
>> +#endif
>> +	if (!*loadaddr)
>> +		return 0;
>> +
>> +	if (!uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0, &fsdev)) {
>> +		size = request_firmware_into_buf(fsdev, name, (void *)*loadaddr,
>> +						 0, 0);
>> +	}
>> +
>> +	return size;
>> +}
>> +#else
>> +int load_firmware(char *name_fw, char *name_loadaddr, u32 *loadaddr)
>> +{
>> +	return 0;
>> +}
>> +#endif
>> +
>> +__weak void start_non_linux_remote_cores(void)
>> +{
>> +}
>> +
>>   void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
>>   {
>>   	struct ti_sci_handle *ti_sci = get_ti_sci_handle();
>> @@ -65,15 +137,17 @@ void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
>>   	/* Release all the exclusive devices held by SPL before starting ATF */
>>   	ti_sci->ops.dev_ops.release_exclusive_devices(ti_sci);
>>   
>> +	ret = rproc_init();
>> +	if (ret)
>> +		panic("rproc failed to be initialized (%d)\n", ret);
>> +
>> +	init_env();
>> +	start_non_linux_remote_cores();
>> +
>>   	/*
>>   	 * It is assumed that remoteproc device 1 is the corresponding
>>   	 * Cortex-A core which runs ATF. Make sure DT reflects the same.
>>   	 */
>> -	ret = rproc_dev_init(1);
>> -	if (ret)
>> -		panic("%s: ATF failed to initialize on rproc (%d)\n", __func__,
>> -		      ret);
>> -
> 
> 
> Where did this code go?

rproc_init takes care of that.

> 
> 
>>   	ret = rproc_load(1, spl_image->entry_point, 0x200);
>>   	if (ret)
>>   		panic("%s: ATF failed to load on rproc (%d)\n", __func__, ret);
>> diff --git a/arch/arm/mach-k3/common.h b/arch/arm/mach-k3/common.h
>> index d8b34fe060..42fb8ee6e7 100644
>> --- a/arch/arm/mach-k3/common.h
>> +++ b/arch/arm/mach-k3/common.h
>> @@ -24,3 +24,5 @@ void setup_k3_mpu_regions(void);
>>   int early_console_init(void);
>>   void disable_linefill_optimization(void);
>>   void remove_fwl_configs(struct fwl_data *fwl_data, size_t fwl_data_size);
>> +void start_non_linux_remote_cores(void);
>> +int load_firmware(char *name_fw, char *name_loadaddr, u32 *loadaddr);
>> diff --git a/arch/arm/mach-k3/j721e_init.c b/arch/arm/mach-k3/j721e_init.c
>> index f7f7398081..c5f8ede1a0 100644
>> --- a/arch/arm/mach-k3/j721e_init.c
>> +++ b/arch/arm/mach-k3/j721e_init.c
>> @@ -18,6 +18,7 @@
>>   #include <dm.h>
>>   #include <dm/uclass-internal.h>
>>   #include <dm/pinctrl.h>
>> +#include <remoteproc.h>
>>   
>>   #ifdef CONFIG_SPL_BUILD
>>   #ifdef CONFIG_K3_LOAD_SYSFW
>> @@ -295,3 +296,36 @@ void release_resources_for_core_shutdown(void)
>>   	}
>>   }
>>   #endif
>> +
>> +#ifdef CONFIG_SYS_K3_SPL_ATF
>> +void start_non_linux_remote_cores(void)
>> +{
>> +	int size = 0, ret;
>> +	u32 loadaddr = 0;
>> +
>> +	size = load_firmware("mainr5f0_0fwname", "mainr5f0_0loadaddr",
>> +			     &loadaddr);
>> +	if (size <= 0)
>> +		goto err_load;
>> +
>> +	/*  remoteproc 2 is aliased for the needed remotecore */
> 
> 
> Assuming the big-arm core to boot is remoteproc 1 was reasonable, but
> there needs to be a better what than assuming the number for every other
> remote core.
> 
> 
>> +	ret = rproc_load(2, loadaddr, size);
>> +	if (ret) {
>> +		printf("Firmware failed to start on rproc (%d)\n", ret);
>> +		goto err_load;
>> +	}
>> +
>> +	ret = rproc_start(2);
>> +	if (ret) {
>> +		printf("Firmware init failed on rproc (%d)\n", ret);
>> +		goto err_load;
>> +	}
>> +
>> +	printf("Remoteproc 2 started successfully\n");
> 
> 
> That's useful..

That is a print that tells everything went well with rproc 2. Otherwise 
one has to really find other ways to see if it succeeded or not.

> 
> Andrew
> 
> 
>> +
>> +	return;
>> +
>> +err_load:
>> +	rproc_reset(2);
>> +}
>> +#endif
>>

[PATCH v3 02/10] arm: k3: Add support for loading non linux remote cores

[Andrew F. Davis]

On 1/21/20 8:10 PM, keerthy wrote:
> 
> 
> On 1/21/2020 6:26 PM, Andrew F. Davis wrote:
>> On 1/21/20 6:07 AM, Keerthy wrote:
>>> Add MAIN domain R5FSS0 remoteproc support from spl. This enables
>>> loading the elf firmware in SPL and starting the remotecore.
>>>
>>> In order to start the core, there should be a file with path
>>> "/lib/firmware/j7-main-r5f0_0-fw" under filesystem
>>> of respective boot mode.
>>>
>>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>>> [Guard start_non_linux_remote_cores under CONFIG_FS_LOADER]
>>> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
>>> ---
>>>   arch/arm/mach-k3/common.c     | 84 ++++++++++++++++++++++++++++++++---
>>>   arch/arm/mach-k3/common.h     |  2 +
>>>   arch/arm/mach-k3/j721e_init.c | 34 ++++++++++++++
>>>   3 files changed, 115 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
>>> index 8d1529062d..f0ac0c39f1 100644
>>> --- a/arch/arm/mach-k3/common.c
>>> +++ b/arch/arm/mach-k3/common.c
>>> @@ -16,6 +16,10 @@
>>>   #include <asm/arch/sys_proto.h>
>>>   #include <asm/hardware.h>
>>>   #include <asm/io.h>
>>> +#include <fs_loader.h>
>>> +#include <fs.h>
>>> +#include <env.h>
>>> +#include <elf.h>
>>>     struct ti_sci_handle *get_ti_sci_handle(void)
>>>   {
>>> @@ -57,6 +61,74 @@ int early_console_init(void)
>>>   #endif
>>>     #ifdef CONFIG_SYS_K3_SPL_ATF
>>> +
>>> +void init_env(void)
>>> +{
>>> +#ifdef CONFIG_SPL_ENV_SUPPORT
>>> +    char *part;
>>> +
>>> +    env_init();
>>> +    env_load();
>>> +    switch (spl_boot_device()) {
>>> +    case BOOT_DEVICE_MMC2:
>>> +        part = env_get("bootpart");
>>> +        env_set("storage_interface", "mmc");
>>> +        env_set("fw_dev_part", part);
>>> +        break;
>>> +    case BOOT_DEVICE_SPI:
>>> +        env_set("storage_interface", "ubi");
>>> +        env_set("fw_ubi_mtdpart", "UBI");
>>> +        env_set("fw_ubi_volume", "UBI0");
>>> +        break;
>>> +    default:
>>> +        printf("%s from device %u not supported!\n",
>>> +               __func__, spl_boot_device());
>>
>>
>> This will print for almost every boot mode..
> 
> I can keep this under debug.
> 
>>
>>
>>> +        return;
>>> +    }
>>> +#endif
>>> +}
>>> +
>>> +#ifdef CONFIG_FS_LOADER
>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32 *loadaddr)
>>> +{
>>> +    struct udevice *fsdev;
>>> +    char *name = NULL;
>>> +    int size = 0;
>>> +
>>> +    *loadaddr = 0;
>>> +#ifdef CONFIG_SPL_ENV_SUPPORT
>>> +    switch (spl_boot_device()) {
>>> +    case BOOT_DEVICE_MMC2:
>>> +        name = env_get(name_fw);
>>> +        *loadaddr = env_get_hex(name_loadaddr, *loadaddr);
>>> +        break;
>>> +    default:
>>> +        printf("Loading rproc fw image from device %u not
>>> supported!\n",
>>> +               spl_boot_device());
>>
>>
>> This whole thing seems very MMC specific, if early firmware loading is
>> important it should work for all boot modes. Find a way to include it in
>> the next boot stage FIT image (tispl.bin) so it works for all modes.
> 
> That was not NAKd. We are going with fs_loader approach.
> 


When, where, link?


>>
>>
>>> +        return 0;
>>> +    }
>>> +#endif
>>> +    if (!*loadaddr)
>>> +        return 0;
>>> +
>>> +    if (!uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0, &fsdev)) {
>>> +        size = request_firmware_into_buf(fsdev, name, (void
>>> *)*loadaddr,
>>> +                         0, 0);
>>> +    }
>>> +
>>> +    return size;
>>> +}
>>> +#else
>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32 *loadaddr)
>>> +{
>>> +    return 0;
>>> +}
>>> +#endif
>>> +
>>> +__weak void start_non_linux_remote_cores(void)
>>> +{
>>> +}
>>> +
>>>   void __noreturn jump_to_image_no_args(struct spl_image_info
>>> *spl_image)
>>>   {
>>>       struct ti_sci_handle *ti_sci = get_ti_sci_handle();
>>> @@ -65,15 +137,17 @@ void __noreturn jump_to_image_no_args(struct
>>> spl_image_info *spl_image)
>>>       /* Release all the exclusive devices held by SPL before
>>> starting ATF */
>>>       ti_sci->ops.dev_ops.release_exclusive_devices(ti_sci);
>>>   +    ret = rproc_init();
>>> +    if (ret)
>>> +        panic("rproc failed to be initialized (%d)\n", ret);
>>> +
>>> +    init_env();
>>> +    start_non_linux_remote_cores();
>>> +
>>>       /*
>>>        * It is assumed that remoteproc device 1 is the corresponding
>>>        * Cortex-A core which runs ATF. Make sure DT reflects the same.
>>>        */
>>> -    ret = rproc_dev_init(1);
>>> -    if (ret)
>>> -        panic("%s: ATF failed to initialize on rproc (%d)\n", __func__,
>>> -              ret);
>>> -
>>
>>
>> Where did this code go?
> 
> rproc_init takes care of that.
> 


Is that new behavior then? It should be it's own patch with a commit
message about that.


>>
>>
>>>       ret = rproc_load(1, spl_image->entry_point, 0x200);
>>>       if (ret)
>>>           panic("%s: ATF failed to load on rproc (%d)\n", __func__,
>>> ret);
>>> diff --git a/arch/arm/mach-k3/common.h b/arch/arm/mach-k3/common.h
>>> index d8b34fe060..42fb8ee6e7 100644
>>> --- a/arch/arm/mach-k3/common.h
>>> +++ b/arch/arm/mach-k3/common.h
>>> @@ -24,3 +24,5 @@ void setup_k3_mpu_regions(void);
>>>   int early_console_init(void);
>>>   void disable_linefill_optimization(void);
>>>   void remove_fwl_configs(struct fwl_data *fwl_data, size_t
>>> fwl_data_size);
>>> +void start_non_linux_remote_cores(void);
>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32 *loadaddr);
>>> diff --git a/arch/arm/mach-k3/j721e_init.c
>>> b/arch/arm/mach-k3/j721e_init.c
>>> index f7f7398081..c5f8ede1a0 100644
>>> --- a/arch/arm/mach-k3/j721e_init.c
>>> +++ b/arch/arm/mach-k3/j721e_init.c
>>> @@ -18,6 +18,7 @@
>>>   #include <dm.h>
>>>   #include <dm/uclass-internal.h>
>>>   #include <dm/pinctrl.h>
>>> +#include <remoteproc.h>
>>>     #ifdef CONFIG_SPL_BUILD
>>>   #ifdef CONFIG_K3_LOAD_SYSFW
>>> @@ -295,3 +296,36 @@ void release_resources_for_core_shutdown(void)
>>>       }
>>>   }
>>>   #endif
>>> +
>>> +#ifdef CONFIG_SYS_K3_SPL_ATF
>>> +void start_non_linux_remote_cores(void)
>>> +{
>>> +    int size = 0, ret;
>>> +    u32 loadaddr = 0;
>>> +
>>> +    size = load_firmware("mainr5f0_0fwname", "mainr5f0_0loadaddr",
>>> +                 &loadaddr);
>>> +    if (size <= 0)
>>> +        goto err_load;
>>> +
>>> +    /*  remoteproc 2 is aliased for the needed remotecore */
>>
>>
>> Assuming the big-arm core to boot is remoteproc 1 was reasonable, but
>> there needs to be a better what than assuming the number for every other
>> remote core.
>>
>>
>>> +    ret = rproc_load(2, loadaddr, size);
>>> +    if (ret) {
>>> +        printf("Firmware failed to start on rproc (%d)\n", ret);
>>> +        goto err_load;
>>> +    }
>>> +
>>> +    ret = rproc_start(2);
>>> +    if (ret) {
>>> +        printf("Firmware init failed on rproc (%d)\n", ret);
>>> +        goto err_load;
>>> +    }
>>> +
>>> +    printf("Remoteproc 2 started successfully\n");
>>
>>
>> That's useful..
> 
> That is a print that tells everything went well with rproc 2. Otherwise
> one has to really find other ways to see if it succeeded or not.
> 


I'm just a customer booting my board, I have no idea what a "Remoteproc
2" is. I'm saying make the message describe what has happened.

Andrew


>>
>> Andrew
>>
>>
>>> +
>>> +    return;
>>> +
>>> +err_load:
>>> +    rproc_reset(2);
>>> +}
>>> +#endif
>>>

[PATCH v3 02/10] arm: k3: Add support for loading non linux remote cores

[Keerthy]

On 22/01/20 9:55 pm, Andrew F. Davis wrote:
> On 1/21/20 8:10 PM, keerthy wrote:
>>
>>
>> On 1/21/2020 6:26 PM, Andrew F. Davis wrote:
>>> On 1/21/20 6:07 AM, Keerthy wrote:
>>>> Add MAIN domain R5FSS0 remoteproc support from spl. This enables
>>>> loading the elf firmware in SPL and starting the remotecore.
>>>>
>>>> In order to start the core, there should be a file with path
>>>> "/lib/firmware/j7-main-r5f0_0-fw" under filesystem
>>>> of respective boot mode.
>>>>
>>>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>>>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>>>> [Guard start_non_linux_remote_cores under CONFIG_FS_LOADER]
>>>> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
>>>> ---
>>>>    arch/arm/mach-k3/common.c     | 84 ++++++++++++++++++++++++++++++++---
>>>>    arch/arm/mach-k3/common.h     |  2 +
>>>>    arch/arm/mach-k3/j721e_init.c | 34 ++++++++++++++
>>>>    3 files changed, 115 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
>>>> index 8d1529062d..f0ac0c39f1 100644
>>>> --- a/arch/arm/mach-k3/common.c
>>>> +++ b/arch/arm/mach-k3/common.c
>>>> @@ -16,6 +16,10 @@
>>>>    #include <asm/arch/sys_proto.h>
>>>>    #include <asm/hardware.h>
>>>>    #include <asm/io.h>
>>>> +#include <fs_loader.h>
>>>> +#include <fs.h>
>>>> +#include <env.h>
>>>> +#include <elf.h>
>>>>      struct ti_sci_handle *get_ti_sci_handle(void)
>>>>    {
>>>> @@ -57,6 +61,74 @@ int early_console_init(void)
>>>>    #endif
>>>>      #ifdef CONFIG_SYS_K3_SPL_ATF
>>>> +
>>>> +void init_env(void)
>>>> +{
>>>> +#ifdef CONFIG_SPL_ENV_SUPPORT
>>>> +    char *part;
>>>> +
>>>> +    env_init();
>>>> +    env_load();
>>>> +    switch (spl_boot_device()) {
>>>> +    case BOOT_DEVICE_MMC2:
>>>> +        part = env_get("bootpart");
>>>> +        env_set("storage_interface", "mmc");
>>>> +        env_set("fw_dev_part", part);
>>>> +        break;
>>>> +    case BOOT_DEVICE_SPI:
>>>> +        env_set("storage_interface", "ubi");
>>>> +        env_set("fw_ubi_mtdpart", "UBI");
>>>> +        env_set("fw_ubi_volume", "UBI0");
>>>> +        break;
>>>> +    default:
>>>> +        printf("%s from device %u not supported!\n",
>>>> +               __func__, spl_boot_device());
>>>
>>>
>>> This will print for almost every boot mode..
>>
>> I can keep this under debug.
>>
>>>
>>>
>>>> +        return;
>>>> +    }
>>>> +#endif
>>>> +}
>>>> +
>>>> +#ifdef CONFIG_FS_LOADER
>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32 *loadaddr)
>>>> +{
>>>> +    struct udevice *fsdev;
>>>> +    char *name = NULL;
>>>> +    int size = 0;
>>>> +
>>>> +    *loadaddr = 0;
>>>> +#ifdef CONFIG_SPL_ENV_SUPPORT
>>>> +    switch (spl_boot_device()) {
>>>> +    case BOOT_DEVICE_MMC2:
>>>> +        name = env_get(name_fw);
>>>> +        *loadaddr = env_get_hex(name_loadaddr, *loadaddr);
>>>> +        break;
>>>> +    default:
>>>> +        printf("Loading rproc fw image from device %u not
>>>> supported!\n",
>>>> +               spl_boot_device());
>>>
>>>
>>> This whole thing seems very MMC specific, if early firmware loading is
>>> important it should work for all boot modes. Find a way to include it in
>>> the next boot stage FIT image (tispl.bin) so it works for all modes.
>>
>> That was not NAKd. We are going with fs_loader approach.
>>
> 
> 
> When, where, link?

I had implemented that way internally. That was rejected for multiple 
right reasons:

1) SPL size would bloat based on the size of the firmware.
2) There are multiple cores that need to be loaded and hence adding all 
the firmwares under a fit can be really painful.
3) Changing firmware means building the tispl.bin again.

The FIT solution can not scale well.

- Keerthy
> 
> 
>>>
>>>
>>>> +        return 0;
>>>> +    }
>>>> +#endif
>>>> +    if (!*loadaddr)
>>>> +        return 0;
>>>> +
>>>> +    if (!uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0, &fsdev)) {
>>>> +        size = request_firmware_into_buf(fsdev, name, (void
>>>> *)*loadaddr,
>>>> +                         0, 0);
>>>> +    }
>>>> +
>>>> +    return size;
>>>> +}
>>>> +#else
>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32 *loadaddr)
>>>> +{
>>>> +    return 0;
>>>> +}
>>>> +#endif
>>>> +
>>>> +__weak void start_non_linux_remote_cores(void)
>>>> +{
>>>> +}
>>>> +
>>>>    void __noreturn jump_to_image_no_args(struct spl_image_info
>>>> *spl_image)
>>>>    {
>>>>        struct ti_sci_handle *ti_sci = get_ti_sci_handle();
>>>> @@ -65,15 +137,17 @@ void __noreturn jump_to_image_no_args(struct
>>>> spl_image_info *spl_image)
>>>>        /* Release all the exclusive devices held by SPL before
>>>> starting ATF */
>>>>        ti_sci->ops.dev_ops.release_exclusive_devices(ti_sci);
>>>>    +    ret = rproc_init();
>>>> +    if (ret)
>>>> +        panic("rproc failed to be initialized (%d)\n", ret);
>>>> +
>>>> +    init_env();
>>>> +    start_non_linux_remote_cores();
>>>> +
>>>>        /*
>>>>         * It is assumed that remoteproc device 1 is the corresponding
>>>>         * Cortex-A core which runs ATF. Make sure DT reflects the same.
>>>>         */
>>>> -    ret = rproc_dev_init(1);
>>>> -    if (ret)
>>>> -        panic("%s: ATF failed to initialize on rproc (%d)\n", __func__,
>>>> -              ret);
>>>> -
>>>
>>>
>>> Where did this code go?
>>
>> rproc_init takes care of that.
>>
> 
> 
> Is that new behavior then? It should be it's own patch with a commit
> message about that.
> 
> 
>>>
>>>
>>>>        ret = rproc_load(1, spl_image->entry_point, 0x200);
>>>>        if (ret)
>>>>            panic("%s: ATF failed to load on rproc (%d)\n", __func__,
>>>> ret);
>>>> diff --git a/arch/arm/mach-k3/common.h b/arch/arm/mach-k3/common.h
>>>> index d8b34fe060..42fb8ee6e7 100644
>>>> --- a/arch/arm/mach-k3/common.h
>>>> +++ b/arch/arm/mach-k3/common.h
>>>> @@ -24,3 +24,5 @@ void setup_k3_mpu_regions(void);
>>>>    int early_console_init(void);
>>>>    void disable_linefill_optimization(void);
>>>>    void remove_fwl_configs(struct fwl_data *fwl_data, size_t
>>>> fwl_data_size);
>>>> +void start_non_linux_remote_cores(void);
>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32 *loadaddr);
>>>> diff --git a/arch/arm/mach-k3/j721e_init.c
>>>> b/arch/arm/mach-k3/j721e_init.c
>>>> index f7f7398081..c5f8ede1a0 100644
>>>> --- a/arch/arm/mach-k3/j721e_init.c
>>>> +++ b/arch/arm/mach-k3/j721e_init.c
>>>> @@ -18,6 +18,7 @@
>>>>    #include <dm.h>
>>>>    #include <dm/uclass-internal.h>
>>>>    #include <dm/pinctrl.h>
>>>> +#include <remoteproc.h>
>>>>      #ifdef CONFIG_SPL_BUILD
>>>>    #ifdef CONFIG_K3_LOAD_SYSFW
>>>> @@ -295,3 +296,36 @@ void release_resources_for_core_shutdown(void)
>>>>        }
>>>>    }
>>>>    #endif
>>>> +
>>>> +#ifdef CONFIG_SYS_K3_SPL_ATF
>>>> +void start_non_linux_remote_cores(void)
>>>> +{
>>>> +    int size = 0, ret;
>>>> +    u32 loadaddr = 0;
>>>> +
>>>> +    size = load_firmware("mainr5f0_0fwname", "mainr5f0_0loadaddr",
>>>> +                 &loadaddr);
>>>> +    if (size <= 0)
>>>> +        goto err_load;
>>>> +
>>>> +    /*  remoteproc 2 is aliased for the needed remotecore */
>>>
>>>
>>> Assuming the big-arm core to boot is remoteproc 1 was reasonable, but
>>> there needs to be a better what than assuming the number for every other
>>> remote core.
>>>
>>>
>>>> +    ret = rproc_load(2, loadaddr, size);
>>>> +    if (ret) {
>>>> +        printf("Firmware failed to start on rproc (%d)\n", ret);
>>>> +        goto err_load;
>>>> +    }
>>>> +
>>>> +    ret = rproc_start(2);
>>>> +    if (ret) {
>>>> +        printf("Firmware init failed on rproc (%d)\n", ret);
>>>> +        goto err_load;
>>>> +    }
>>>> +
>>>> +    printf("Remoteproc 2 started successfully\n");
>>>
>>>
>>> That's useful..
>>
>> That is a print that tells everything went well with rproc 2. Otherwise
>> one has to really find other ways to see if it succeeded or not.
>>
> 
> 
> I'm just a customer booting my board, I have no idea what a "Remoteproc
> 2" is. I'm saying make the message describe what has happened.
> 
> Andrew
> 
> 
>>>
>>> Andrew
>>>
>>>
>>>> +
>>>> +    return;
>>>> +
>>>> +err_load:
>>>> +    rproc_reset(2);
>>>> +}
>>>> +#endif
>>>>

[PATCH v3 02/10] arm: k3: Add support for loading non linux remote cores

[Andrew F. Davis]

On 1/22/20 11:10 PM, Keerthy wrote:
> 
> 
> On 22/01/20 9:55 pm, Andrew F. Davis wrote:
>> On 1/21/20 8:10 PM, keerthy wrote:
>>>
>>>
>>> On 1/21/2020 6:26 PM, Andrew F. Davis wrote:
>>>> On 1/21/20 6:07 AM, Keerthy wrote:
>>>>> Add MAIN domain R5FSS0 remoteproc support from spl. This enables
>>>>> loading the elf firmware in SPL and starting the remotecore.
>>>>>
>>>>> In order to start the core, there should be a file with path
>>>>> "/lib/firmware/j7-main-r5f0_0-fw" under filesystem
>>>>> of respective boot mode.
>>>>>
>>>>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>>>>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>>>>> [Guard start_non_linux_remote_cores under CONFIG_FS_LOADER]
>>>>> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
>>>>> ---
>>>>>    arch/arm/mach-k3/common.c     | 84
>>>>> ++++++++++++++++++++++++++++++++---
>>>>>    arch/arm/mach-k3/common.h     |  2 +
>>>>>    arch/arm/mach-k3/j721e_init.c | 34 ++++++++++++++
>>>>>    3 files changed, 115 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
>>>>> index 8d1529062d..f0ac0c39f1 100644
>>>>> --- a/arch/arm/mach-k3/common.c
>>>>> +++ b/arch/arm/mach-k3/common.c
>>>>> @@ -16,6 +16,10 @@
>>>>>    #include <asm/arch/sys_proto.h>
>>>>>    #include <asm/hardware.h>
>>>>>    #include <asm/io.h>
>>>>> +#include <fs_loader.h>
>>>>> +#include <fs.h>
>>>>> +#include <env.h>
>>>>> +#include <elf.h>
>>>>>      struct ti_sci_handle *get_ti_sci_handle(void)
>>>>>    {
>>>>> @@ -57,6 +61,74 @@ int early_console_init(void)
>>>>>    #endif
>>>>>      #ifdef CONFIG_SYS_K3_SPL_ATF
>>>>> +
>>>>> +void init_env(void)
>>>>> +{
>>>>> +#ifdef CONFIG_SPL_ENV_SUPPORT
>>>>> +    char *part;
>>>>> +
>>>>> +    env_init();
>>>>> +    env_load();
>>>>> +    switch (spl_boot_device()) {
>>>>> +    case BOOT_DEVICE_MMC2:
>>>>> +        part = env_get("bootpart");
>>>>> +        env_set("storage_interface", "mmc");
>>>>> +        env_set("fw_dev_part", part);
>>>>> +        break;
>>>>> +    case BOOT_DEVICE_SPI:
>>>>> +        env_set("storage_interface", "ubi");
>>>>> +        env_set("fw_ubi_mtdpart", "UBI");
>>>>> +        env_set("fw_ubi_volume", "UBI0");
>>>>> +        break;
>>>>> +    default:
>>>>> +        printf("%s from device %u not supported!\n",
>>>>> +               __func__, spl_boot_device());
>>>>
>>>>
>>>> This will print for almost every boot mode..
>>>
>>> I can keep this under debug.
>>>
>>>>
>>>>
>>>>> +        return;
>>>>> +    }
>>>>> +#endif
>>>>> +}
>>>>> +
>>>>> +#ifdef CONFIG_FS_LOADER
>>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32 *loadaddr)
>>>>> +{
>>>>> +    struct udevice *fsdev;
>>>>> +    char *name = NULL;
>>>>> +    int size = 0;
>>>>> +
>>>>> +    *loadaddr = 0;
>>>>> +#ifdef CONFIG_SPL_ENV_SUPPORT
>>>>> +    switch (spl_boot_device()) {
>>>>> +    case BOOT_DEVICE_MMC2:
>>>>> +        name = env_get(name_fw);
>>>>> +        *loadaddr = env_get_hex(name_loadaddr, *loadaddr);
>>>>> +        break;
>>>>> +    default:
>>>>> +        printf("Loading rproc fw image from device %u not
>>>>> supported!\n",
>>>>> +               spl_boot_device());
>>>>
>>>>
>>>> This whole thing seems very MMC specific, if early firmware loading is
>>>> important it should work for all boot modes. Find a way to include
>>>> it in
>>>> the next boot stage FIT image (tispl.bin) so it works for all modes.
>>>
>>> That was not NAKd. We are going with fs_loader approach.
>>>
>>
>>
>> When, where, link?
> 
> I had implemented that way internally. That was rejected for multiple
> right reasons:
> 


I must have missed the internal reviews for this, anyway this is posted
upstream so lets discus it here.


> 1) SPL size would bloat based on the size of the firmware.


SPL size would remain constant, the combined FIT (tispl.bin) would grow,
but that is okay as DRAM is enabled at this point so we have no hard
memory constraints.


> 2) There are multiple cores that need to be loaded and hence adding all
> the firmwares under a fit can be really painful.


Bundling images is what FIT is for, are you saying the better solution
is to hard-code each firmware starting like done here?


> 3) Changing firmware means building the tispl.bin again.
> 


FIT images can be disassembled and reassembled with a script around
tools/dumpimage.

SPL should be simple and load the one next stage.


> The FIT solution can not scale well.
> 


How does this current series scale at all? At least with FIT you can add
more images without adding code for
request_firmware(<hard-coded-firmware-name>) and
rproc_load(<hard-coded-number>). That all could be encoded in the FIT data.

Andrew


> - Keerthy
>>
>>
>>>>
>>>>
>>>>> +        return 0;
>>>>> +    }
>>>>> +#endif
>>>>> +    if (!*loadaddr)
>>>>> +        return 0;
>>>>> +
>>>>> +    if (!uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0, &fsdev)) {
>>>>> +        size = request_firmware_into_buf(fsdev, name, (void
>>>>> *)*loadaddr,
>>>>> +                         0, 0);
>>>>> +    }
>>>>> +
>>>>> +    return size;
>>>>> +}
>>>>> +#else
>>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32 *loadaddr)
>>>>> +{
>>>>> +    return 0;
>>>>> +}
>>>>> +#endif
>>>>> +
>>>>> +__weak void start_non_linux_remote_cores(void)
>>>>> +{
>>>>> +}
>>>>> +
>>>>>    void __noreturn jump_to_image_no_args(struct spl_image_info
>>>>> *spl_image)
>>>>>    {
>>>>>        struct ti_sci_handle *ti_sci = get_ti_sci_handle();
>>>>> @@ -65,15 +137,17 @@ void __noreturn jump_to_image_no_args(struct
>>>>> spl_image_info *spl_image)
>>>>>        /* Release all the exclusive devices held by SPL before
>>>>> starting ATF */
>>>>>        ti_sci->ops.dev_ops.release_exclusive_devices(ti_sci);
>>>>>    +    ret = rproc_init();
>>>>> +    if (ret)
>>>>> +        panic("rproc failed to be initialized (%d)\n", ret);
>>>>> +
>>>>> +    init_env();
>>>>> +    start_non_linux_remote_cores();
>>>>> +
>>>>>        /*
>>>>>         * It is assumed that remoteproc device 1 is the corresponding
>>>>>         * Cortex-A core which runs ATF. Make sure DT reflects the
>>>>> same.
>>>>>         */
>>>>> -    ret = rproc_dev_init(1);
>>>>> -    if (ret)
>>>>> -        panic("%s: ATF failed to initialize on rproc (%d)\n",
>>>>> __func__,
>>>>> -              ret);
>>>>> -
>>>>
>>>>
>>>> Where did this code go?
>>>
>>> rproc_init takes care of that.
>>>
>>
>>
>> Is that new behavior then? It should be it's own patch with a commit
>> message about that.
>>
>>
>>>>
>>>>
>>>>>        ret = rproc_load(1, spl_image->entry_point, 0x200);
>>>>>        if (ret)
>>>>>            panic("%s: ATF failed to load on rproc (%d)\n", __func__,
>>>>> ret);
>>>>> diff --git a/arch/arm/mach-k3/common.h b/arch/arm/mach-k3/common.h
>>>>> index d8b34fe060..42fb8ee6e7 100644
>>>>> --- a/arch/arm/mach-k3/common.h
>>>>> +++ b/arch/arm/mach-k3/common.h
>>>>> @@ -24,3 +24,5 @@ void setup_k3_mpu_regions(void);
>>>>>    int early_console_init(void);
>>>>>    void disable_linefill_optimization(void);
>>>>>    void remove_fwl_configs(struct fwl_data *fwl_data, size_t
>>>>> fwl_data_size);
>>>>> +void start_non_linux_remote_cores(void);
>>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32 *loadaddr);
>>>>> diff --git a/arch/arm/mach-k3/j721e_init.c
>>>>> b/arch/arm/mach-k3/j721e_init.c
>>>>> index f7f7398081..c5f8ede1a0 100644
>>>>> --- a/arch/arm/mach-k3/j721e_init.c
>>>>> +++ b/arch/arm/mach-k3/j721e_init.c
>>>>> @@ -18,6 +18,7 @@
>>>>>    #include <dm.h>
>>>>>    #include <dm/uclass-internal.h>
>>>>>    #include <dm/pinctrl.h>
>>>>> +#include <remoteproc.h>
>>>>>      #ifdef CONFIG_SPL_BUILD
>>>>>    #ifdef CONFIG_K3_LOAD_SYSFW
>>>>> @@ -295,3 +296,36 @@ void release_resources_for_core_shutdown(void)
>>>>>        }
>>>>>    }
>>>>>    #endif
>>>>> +
>>>>> +#ifdef CONFIG_SYS_K3_SPL_ATF
>>>>> +void start_non_linux_remote_cores(void)
>>>>> +{
>>>>> +    int size = 0, ret;
>>>>> +    u32 loadaddr = 0;
>>>>> +
>>>>> +    size = load_firmware("mainr5f0_0fwname", "mainr5f0_0loadaddr",
>>>>> +                 &loadaddr);
>>>>> +    if (size <= 0)
>>>>> +        goto err_load;
>>>>> +
>>>>> +    /*  remoteproc 2 is aliased for the needed remotecore */
>>>>
>>>>
>>>> Assuming the big-arm core to boot is remoteproc 1 was reasonable, but
>>>> there needs to be a better what than assuming the number for every
>>>> other
>>>> remote core.
>>>>
>>>>
>>>>> +    ret = rproc_load(2, loadaddr, size);
>>>>> +    if (ret) {
>>>>> +        printf("Firmware failed to start on rproc (%d)\n", ret);
>>>>> +        goto err_load;
>>>>> +    }
>>>>> +
>>>>> +    ret = rproc_start(2);
>>>>> +    if (ret) {
>>>>> +        printf("Firmware init failed on rproc (%d)\n", ret);
>>>>> +        goto err_load;
>>>>> +    }
>>>>> +
>>>>> +    printf("Remoteproc 2 started successfully\n");
>>>>
>>>>
>>>> That's useful..
>>>
>>> That is a print that tells everything went well with rproc 2. Otherwise
>>> one has to really find other ways to see if it succeeded or not.
>>>
>>
>>
>> I'm just a customer booting my board, I have no idea what a "Remoteproc
>> 2" is. I'm saying make the message describe what has happened.
>>
>> Andrew
>>
>>
>>>>
>>>> Andrew
>>>>
>>>>
>>>>> +
>>>>> +    return;
>>>>> +
>>>>> +err_load:
>>>>> +    rproc_reset(2);
>>>>> +}
>>>>> +#endif
>>>>>

[PATCH v3 02/10] arm: k3: Add support for loading non linux remote cores

[Keerthy]

On 23/01/20 6:54 pm, Andrew F. Davis wrote:
> On 1/22/20 11:10 PM, Keerthy wrote:
>>
>>
>> On 22/01/20 9:55 pm, Andrew F. Davis wrote:
>>> On 1/21/20 8:10 PM, keerthy wrote:
>>>>
>>>>
>>>> On 1/21/2020 6:26 PM, Andrew F. Davis wrote:
>>>>> On 1/21/20 6:07 AM, Keerthy wrote:
>>>>>> Add MAIN domain R5FSS0 remoteproc support from spl. This enables
>>>>>> loading the elf firmware in SPL and starting the remotecore.
>>>>>>
>>>>>> In order to start the core, there should be a file with path
>>>>>> "/lib/firmware/j7-main-r5f0_0-fw" under filesystem
>>>>>> of respective boot mode.
>>>>>>
>>>>>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>>>>>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>>>>>> [Guard start_non_linux_remote_cores under CONFIG_FS_LOADER]
>>>>>> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
>>>>>> ---
>>>>>>     arch/arm/mach-k3/common.c     | 84
>>>>>> ++++++++++++++++++++++++++++++++---
>>>>>>     arch/arm/mach-k3/common.h     |  2 +
>>>>>>     arch/arm/mach-k3/j721e_init.c | 34 ++++++++++++++
>>>>>>     3 files changed, 115 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
>>>>>> index 8d1529062d..f0ac0c39f1 100644
>>>>>> --- a/arch/arm/mach-k3/common.c
>>>>>> +++ b/arch/arm/mach-k3/common.c
>>>>>> @@ -16,6 +16,10 @@
>>>>>>     #include <asm/arch/sys_proto.h>
>>>>>>     #include <asm/hardware.h>
>>>>>>     #include <asm/io.h>
>>>>>> +#include <fs_loader.h>
>>>>>> +#include <fs.h>
>>>>>> +#include <env.h>
>>>>>> +#include <elf.h>
>>>>>>       struct ti_sci_handle *get_ti_sci_handle(void)
>>>>>>     {
>>>>>> @@ -57,6 +61,74 @@ int early_console_init(void)
>>>>>>     #endif
>>>>>>       #ifdef CONFIG_SYS_K3_SPL_ATF
>>>>>> +
>>>>>> +void init_env(void)
>>>>>> +{
>>>>>> +#ifdef CONFIG_SPL_ENV_SUPPORT
>>>>>> +    char *part;
>>>>>> +
>>>>>> +    env_init();
>>>>>> +    env_load();
>>>>>> +    switch (spl_boot_device()) {
>>>>>> +    case BOOT_DEVICE_MMC2:
>>>>>> +        part = env_get("bootpart");
>>>>>> +        env_set("storage_interface", "mmc");
>>>>>> +        env_set("fw_dev_part", part);
>>>>>> +        break;
>>>>>> +    case BOOT_DEVICE_SPI:
>>>>>> +        env_set("storage_interface", "ubi");
>>>>>> +        env_set("fw_ubi_mtdpart", "UBI");
>>>>>> +        env_set("fw_ubi_volume", "UBI0");
>>>>>> +        break;
>>>>>> +    default:
>>>>>> +        printf("%s from device %u not supported!\n",
>>>>>> +               __func__, spl_boot_device());
>>>>>
>>>>>
>>>>> This will print for almost every boot mode..
>>>>
>>>> I can keep this under debug.
>>>>
>>>>>
>>>>>
>>>>>> +        return;
>>>>>> +    }
>>>>>> +#endif
>>>>>> +}
>>>>>> +
>>>>>> +#ifdef CONFIG_FS_LOADER
>>>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32 *loadaddr)
>>>>>> +{
>>>>>> +    struct udevice *fsdev;
>>>>>> +    char *name = NULL;
>>>>>> +    int size = 0;
>>>>>> +
>>>>>> +    *loadaddr = 0;
>>>>>> +#ifdef CONFIG_SPL_ENV_SUPPORT
>>>>>> +    switch (spl_boot_device()) {
>>>>>> +    case BOOT_DEVICE_MMC2:
>>>>>> +        name = env_get(name_fw);
>>>>>> +        *loadaddr = env_get_hex(name_loadaddr, *loadaddr);
>>>>>> +        break;
>>>>>> +    default:
>>>>>> +        printf("Loading rproc fw image from device %u not
>>>>>> supported!\n",
>>>>>> +               spl_boot_device());
>>>>>
>>>>>
>>>>> This whole thing seems very MMC specific, if early firmware loading is
>>>>> important it should work for all boot modes. Find a way to include
>>>>> it in
>>>>> the next boot stage FIT image (tispl.bin) so it works for all modes.
>>>>
>>>> That was not NAKd. We are going with fs_loader approach.
>>>>
>>>
>>>
>>> When, where, link?
>>
>> I had implemented that way internally. That was rejected for multiple
>> right reasons:
>>
> 
> 
> I must have missed the internal reviews for this, anyway this is posted
> upstream so lets discus it here.
> 
> 
>> 1) SPL size would bloat based on the size of the firmware.
> 
> 
> SPL size would remain constant, the combined FIT (tispl.bin) would grow,
> but that is okay as DRAM is enabled at this point so we have no hard
> memory constraints.

I meant the FIT image containing the SPL will bloat.

> 
> 
>> 2) There are multiple cores that need to be loaded and hence adding all
>> the firmwares under a fit can be really painful.
> 
> 
> Bundling images is what FIT is for, are you saying the better solution
> is to hard-code each firmware starting like done here?

How many firmwares will you go on bundling. Firmwares are already kept 
in file system. It is a matter of reading them from there.

> 
> 
>> 3) Changing firmware means building the tispl.bin again.
>>
> 
> 
> FIT images can be disassembled and reassembled with a script around
> tools/dumpimage.

And you expect everyone to master that instead of looking at couple of 
aliases in DT to figure out which core corresponds to which ID?

> 
> SPL should be simple and load the one next stage.
> 
> 
>> The FIT solution can not scale well.
>>
> 
> 
> How does this current series scale at all? At least with FIT you can add
> more images without adding code for
> request_firmware(<hard-coded-firmware-name>) and
> rproc_load(<hard-coded-number>). That all could be encoded in the FIT data.

I understand and as explained earlier i have even implemented that once 
before. fs_loader was meant to address the exact use case we are 
discussing about.

Even in u-boot remotecores are started/loaded by indices. Users need to 
know them. This is no different than that.

I am not convinced about FIT approach. I would let Lokesh take a call on 
this.

Thanks,
Keerthy
> 
> Andrew
> 
> 
>> - Keerthy
>>>
>>>
>>>>>
>>>>>
>>>>>> +        return 0;
>>>>>> +    }
>>>>>> +#endif
>>>>>> +    if (!*loadaddr)
>>>>>> +        return 0;
>>>>>> +
>>>>>> +    if (!uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0, &fsdev)) {
>>>>>> +        size = request_firmware_into_buf(fsdev, name, (void
>>>>>> *)*loadaddr,
>>>>>> +                         0, 0);
>>>>>> +    }
>>>>>> +
>>>>>> +    return size;
>>>>>> +}
>>>>>> +#else
>>>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32 *loadaddr)
>>>>>> +{
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +#endif
>>>>>> +
>>>>>> +__weak void start_non_linux_remote_cores(void)
>>>>>> +{
>>>>>> +}
>>>>>> +
>>>>>>     void __noreturn jump_to_image_no_args(struct spl_image_info
>>>>>> *spl_image)
>>>>>>     {
>>>>>>         struct ti_sci_handle *ti_sci = get_ti_sci_handle();
>>>>>> @@ -65,15 +137,17 @@ void __noreturn jump_to_image_no_args(struct
>>>>>> spl_image_info *spl_image)
>>>>>>         /* Release all the exclusive devices held by SPL before
>>>>>> starting ATF */
>>>>>>         ti_sci->ops.dev_ops.release_exclusive_devices(ti_sci);
>>>>>>     +    ret = rproc_init();
>>>>>> +    if (ret)
>>>>>> +        panic("rproc failed to be initialized (%d)\n", ret);
>>>>>> +
>>>>>> +    init_env();
>>>>>> +    start_non_linux_remote_cores();
>>>>>> +
>>>>>>         /*
>>>>>>          * It is assumed that remoteproc device 1 is the corresponding
>>>>>>          * Cortex-A core which runs ATF. Make sure DT reflects the
>>>>>> same.
>>>>>>          */
>>>>>> -    ret = rproc_dev_init(1);
>>>>>> -    if (ret)
>>>>>> -        panic("%s: ATF failed to initialize on rproc (%d)\n",
>>>>>> __func__,
>>>>>> -              ret);
>>>>>> -
>>>>>
>>>>>
>>>>> Where did this code go?
>>>>
>>>> rproc_init takes care of that.
>>>>
>>>
>>>
>>> Is that new behavior then? It should be it's own patch with a commit
>>> message about that.
>>>
>>>
>>>>>
>>>>>
>>>>>>         ret = rproc_load(1, spl_image->entry_point, 0x200);
>>>>>>         if (ret)
>>>>>>             panic("%s: ATF failed to load on rproc (%d)\n", __func__,
>>>>>> ret);
>>>>>> diff --git a/arch/arm/mach-k3/common.h b/arch/arm/mach-k3/common.h
>>>>>> index d8b34fe060..42fb8ee6e7 100644
>>>>>> --- a/arch/arm/mach-k3/common.h
>>>>>> +++ b/arch/arm/mach-k3/common.h
>>>>>> @@ -24,3 +24,5 @@ void setup_k3_mpu_regions(void);
>>>>>>     int early_console_init(void);
>>>>>>     void disable_linefill_optimization(void);
>>>>>>     void remove_fwl_configs(struct fwl_data *fwl_data, size_t
>>>>>> fwl_data_size);
>>>>>> +void start_non_linux_remote_cores(void);
>>>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32 *loadaddr);
>>>>>> diff --git a/arch/arm/mach-k3/j721e_init.c
>>>>>> b/arch/arm/mach-k3/j721e_init.c
>>>>>> index f7f7398081..c5f8ede1a0 100644
>>>>>> --- a/arch/arm/mach-k3/j721e_init.c
>>>>>> +++ b/arch/arm/mach-k3/j721e_init.c
>>>>>> @@ -18,6 +18,7 @@
>>>>>>     #include <dm.h>
>>>>>>     #include <dm/uclass-internal.h>
>>>>>>     #include <dm/pinctrl.h>
>>>>>> +#include <remoteproc.h>
>>>>>>       #ifdef CONFIG_SPL_BUILD
>>>>>>     #ifdef CONFIG_K3_LOAD_SYSFW
>>>>>> @@ -295,3 +296,36 @@ void release_resources_for_core_shutdown(void)
>>>>>>         }
>>>>>>     }
>>>>>>     #endif
>>>>>> +
>>>>>> +#ifdef CONFIG_SYS_K3_SPL_ATF
>>>>>> +void start_non_linux_remote_cores(void)
>>>>>> +{
>>>>>> +    int size = 0, ret;
>>>>>> +    u32 loadaddr = 0;
>>>>>> +
>>>>>> +    size = load_firmware("mainr5f0_0fwname", "mainr5f0_0loadaddr",
>>>>>> +                 &loadaddr);
>>>>>> +    if (size <= 0)
>>>>>> +        goto err_load;
>>>>>> +
>>>>>> +    /*  remoteproc 2 is aliased for the needed remotecore */
>>>>>
>>>>>
>>>>> Assuming the big-arm core to boot is remoteproc 1 was reasonable, but
>>>>> there needs to be a better what than assuming the number for every
>>>>> other
>>>>> remote core.
>>>>>
>>>>>
>>>>>> +    ret = rproc_load(2, loadaddr, size);
>>>>>> +    if (ret) {
>>>>>> +        printf("Firmware failed to start on rproc (%d)\n", ret);
>>>>>> +        goto err_load;
>>>>>> +    }
>>>>>> +
>>>>>> +    ret = rproc_start(2);
>>>>>> +    if (ret) {
>>>>>> +        printf("Firmware init failed on rproc (%d)\n", ret);
>>>>>> +        goto err_load;
>>>>>> +    }
>>>>>> +
>>>>>> +    printf("Remoteproc 2 started successfully\n");
>>>>>
>>>>>
>>>>> That's useful..
>>>>
>>>> That is a print that tells everything went well with rproc 2. Otherwise
>>>> one has to really find other ways to see if it succeeded or not.
>>>>
>>>
>>>
>>> I'm just a customer booting my board, I have no idea what a "Remoteproc
>>> 2" is. I'm saying make the message describe what has happened.
>>>
>>> Andrew
>>>
>>>
>>>>>
>>>>> Andrew
>>>>>
>>>>>
>>>>>> +
>>>>>> +    return;
>>>>>> +
>>>>>> +err_load:
>>>>>> +    rproc_reset(2);
>>>>>> +}
>>>>>> +#endif
>>>>>>

[PATCH v3 02/10] arm: k3: Add support for loading non linux remote cores

[Andrew F. Davis]

On 1/23/20 11:44 AM, Keerthy wrote:
> 
> 
> On 23/01/20 6:54 pm, Andrew F. Davis wrote:
>> On 1/22/20 11:10 PM, Keerthy wrote:
>>>
>>>
>>> On 22/01/20 9:55 pm, Andrew F. Davis wrote:
>>>> On 1/21/20 8:10 PM, keerthy wrote:
>>>>>
>>>>>
>>>>> On 1/21/2020 6:26 PM, Andrew F. Davis wrote:
>>>>>> On 1/21/20 6:07 AM, Keerthy wrote:
>>>>>>> Add MAIN domain R5FSS0 remoteproc support from spl. This enables
>>>>>>> loading the elf firmware in SPL and starting the remotecore.
>>>>>>>
>>>>>>> In order to start the core, there should be a file with path
>>>>>>> "/lib/firmware/j7-main-r5f0_0-fw" under filesystem
>>>>>>> of respective boot mode.
>>>>>>>
>>>>>>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>>>>>>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>>>>>>> [Guard start_non_linux_remote_cores under CONFIG_FS_LOADER]
>>>>>>> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
>>>>>>> ---
>>>>>>>     arch/arm/mach-k3/common.c     | 84
>>>>>>> ++++++++++++++++++++++++++++++++---
>>>>>>>     arch/arm/mach-k3/common.h     |  2 +
>>>>>>>     arch/arm/mach-k3/j721e_init.c | 34 ++++++++++++++
>>>>>>>     3 files changed, 115 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
>>>>>>> index 8d1529062d..f0ac0c39f1 100644
>>>>>>> --- a/arch/arm/mach-k3/common.c
>>>>>>> +++ b/arch/arm/mach-k3/common.c
>>>>>>> @@ -16,6 +16,10 @@
>>>>>>>     #include <asm/arch/sys_proto.h>
>>>>>>>     #include <asm/hardware.h>
>>>>>>>     #include <asm/io.h>
>>>>>>> +#include <fs_loader.h>
>>>>>>> +#include <fs.h>
>>>>>>> +#include <env.h>
>>>>>>> +#include <elf.h>
>>>>>>>       struct ti_sci_handle *get_ti_sci_handle(void)
>>>>>>>     {
>>>>>>> @@ -57,6 +61,74 @@ int early_console_init(void)
>>>>>>>     #endif
>>>>>>>       #ifdef CONFIG_SYS_K3_SPL_ATF
>>>>>>> +
>>>>>>> +void init_env(void)
>>>>>>> +{
>>>>>>> +#ifdef CONFIG_SPL_ENV_SUPPORT
>>>>>>> +    char *part;
>>>>>>> +
>>>>>>> +    env_init();
>>>>>>> +    env_load();
>>>>>>> +    switch (spl_boot_device()) {
>>>>>>> +    case BOOT_DEVICE_MMC2:
>>>>>>> +        part = env_get("bootpart");
>>>>>>> +        env_set("storage_interface", "mmc");
>>>>>>> +        env_set("fw_dev_part", part);
>>>>>>> +        break;
>>>>>>> +    case BOOT_DEVICE_SPI:
>>>>>>> +        env_set("storage_interface", "ubi");
>>>>>>> +        env_set("fw_ubi_mtdpart", "UBI");
>>>>>>> +        env_set("fw_ubi_volume", "UBI0");
>>>>>>> +        break;
>>>>>>> +    default:
>>>>>>> +        printf("%s from device %u not supported!\n",
>>>>>>> +               __func__, spl_boot_device());
>>>>>>
>>>>>>
>>>>>> This will print for almost every boot mode..
>>>>>
>>>>> I can keep this under debug.
>>>>>
>>>>>>
>>>>>>
>>>>>>> +        return;
>>>>>>> +    }
>>>>>>> +#endif
>>>>>>> +}
>>>>>>> +
>>>>>>> +#ifdef CONFIG_FS_LOADER
>>>>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32
>>>>>>> *loadaddr)
>>>>>>> +{
>>>>>>> +    struct udevice *fsdev;
>>>>>>> +    char *name = NULL;
>>>>>>> +    int size = 0;
>>>>>>> +
>>>>>>> +    *loadaddr = 0;
>>>>>>> +#ifdef CONFIG_SPL_ENV_SUPPORT
>>>>>>> +    switch (spl_boot_device()) {
>>>>>>> +    case BOOT_DEVICE_MMC2:
>>>>>>> +        name = env_get(name_fw);
>>>>>>> +        *loadaddr = env_get_hex(name_loadaddr, *loadaddr);
>>>>>>> +        break;
>>>>>>> +    default:
>>>>>>> +        printf("Loading rproc fw image from device %u not
>>>>>>> supported!\n",
>>>>>>> +               spl_boot_device());
>>>>>>
>>>>>>
>>>>>> This whole thing seems very MMC specific, if early firmware
>>>>>> loading is
>>>>>> important it should work for all boot modes. Find a way to include
>>>>>> it in
>>>>>> the next boot stage FIT image (tispl.bin) so it works for all modes.
>>>>>
>>>>> That was not NAKd. We are going with fs_loader approach.
>>>>>
>>>>
>>>>
>>>> When, where, link?
>>>
>>> I had implemented that way internally. That was rejected for multiple
>>> right reasons:
>>>
>>
>>
>> I must have missed the internal reviews for this, anyway this is posted
>> upstream so lets discus it here.
>>
>>
>>> 1) SPL size would bloat based on the size of the firmware.
>>
>>
>> SPL size would remain constant, the combined FIT (tispl.bin) would grow,
>> but that is okay as DRAM is enabled at this point so we have no hard
>> memory constraints.
> 
> I meant the FIT image containing the SPL will bloat.


Exactly what I said, and so size is not a huge deal.


> 
>>
>>
>>> 2) There are multiple cores that need to be loaded and hence adding all
>>> the firmwares under a fit can be really painful.
>>
>>
>> Bundling images is what FIT is for, are you saying the better solution
>> is to hard-code each firmware starting like done here?
> 
> How many firmwares will you go on bundling. Firmwares are already kept
> in file system. It is a matter of reading them from there.
> 


If we are early booting them from SPL then they don't really need to be
on the filesystem.


>>
>>
>>> 3) Changing firmware means building the tispl.bin again.
>>>
>>
>>
>> FIT images can be disassembled and reassembled with a script around
>> tools/dumpimage.
> 
> And you expect everyone to master that instead of looking at couple of
> aliases in DT to figure out which core corresponds to which ID?
> 


Your patches do more than add DT aliases to add a firmware image. I
think you are responding to the wrong comment here, the ID part is below.


>>
>> SPL should be simple and load the one next stage.
>>
>>
>>> The FIT solution can not scale well.
>>>
>>
>>
>> How does this current series scale at all? At least with FIT you can add
>> more images without adding code for
>> request_firmware(<hard-coded-firmware-name>) and
>> rproc_load(<hard-coded-number>). That all could be encoded in the FIT
>> data.
> 
> I understand and as explained earlier i have even implemented that once
> before. fs_loader was meant to address the exact use case we are
> discussing about.
> 
> Even in u-boot remotecores are started/loaded by indices. Users need to
> know them. This is no different than that.
> 
> I am not convinced about FIT approach. I would let Lokesh take a call on
> this.
> 


Lokesh is not the whole U-Boot community, I get you two aligned
internally on this, but I'd be much more interested in Tom or Simon's
opinion here. I was doing the same thing when loading PMMC firmware for
HS and was pushed to make a FIT friendly version, I'm glad that I did,
it ended up much less hacky in the long run.

Andrew


> Thanks,
> Keerthy
>>
>> Andrew
>>
>>
>>> - Keerthy
>>>>
>>>>
>>>>>>
>>>>>>
>>>>>>> +        return 0;
>>>>>>> +    }
>>>>>>> +#endif
>>>>>>> +    if (!*loadaddr)
>>>>>>> +        return 0;
>>>>>>> +
>>>>>>> +    if (!uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0, &fsdev)) {
>>>>>>> +        size = request_firmware_into_buf(fsdev, name, (void
>>>>>>> *)*loadaddr,
>>>>>>> +                         0, 0);
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    return size;
>>>>>>> +}
>>>>>>> +#else
>>>>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32
>>>>>>> *loadaddr)
>>>>>>> +{
>>>>>>> +    return 0;
>>>>>>> +}
>>>>>>> +#endif
>>>>>>> +
>>>>>>> +__weak void start_non_linux_remote_cores(void)
>>>>>>> +{
>>>>>>> +}
>>>>>>> +
>>>>>>>     void __noreturn jump_to_image_no_args(struct spl_image_info
>>>>>>> *spl_image)
>>>>>>>     {
>>>>>>>         struct ti_sci_handle *ti_sci = get_ti_sci_handle();
>>>>>>> @@ -65,15 +137,17 @@ void __noreturn jump_to_image_no_args(struct
>>>>>>> spl_image_info *spl_image)
>>>>>>>         /* Release all the exclusive devices held by SPL before
>>>>>>> starting ATF */
>>>>>>>         ti_sci->ops.dev_ops.release_exclusive_devices(ti_sci);
>>>>>>>     +    ret = rproc_init();
>>>>>>> +    if (ret)
>>>>>>> +        panic("rproc failed to be initialized (%d)\n", ret);
>>>>>>> +
>>>>>>> +    init_env();
>>>>>>> +    start_non_linux_remote_cores();
>>>>>>> +
>>>>>>>         /*
>>>>>>>          * It is assumed that remoteproc device 1 is the
>>>>>>> corresponding
>>>>>>>          * Cortex-A core which runs ATF. Make sure DT reflects the
>>>>>>> same.
>>>>>>>          */
>>>>>>> -    ret = rproc_dev_init(1);
>>>>>>> -    if (ret)
>>>>>>> -        panic("%s: ATF failed to initialize on rproc (%d)\n",
>>>>>>> __func__,
>>>>>>> -              ret);
>>>>>>> -
>>>>>>
>>>>>>
>>>>>> Where did this code go?
>>>>>
>>>>> rproc_init takes care of that.
>>>>>
>>>>
>>>>
>>>> Is that new behavior then? It should be it's own patch with a commit
>>>> message about that.
>>>>
>>>>
>>>>>>
>>>>>>
>>>>>>>         ret = rproc_load(1, spl_image->entry_point, 0x200);
>>>>>>>         if (ret)
>>>>>>>             panic("%s: ATF failed to load on rproc (%d)\n",
>>>>>>> __func__,
>>>>>>> ret);
>>>>>>> diff --git a/arch/arm/mach-k3/common.h b/arch/arm/mach-k3/common.h
>>>>>>> index d8b34fe060..42fb8ee6e7 100644
>>>>>>> --- a/arch/arm/mach-k3/common.h
>>>>>>> +++ b/arch/arm/mach-k3/common.h
>>>>>>> @@ -24,3 +24,5 @@ void setup_k3_mpu_regions(void);
>>>>>>>     int early_console_init(void);
>>>>>>>     void disable_linefill_optimization(void);
>>>>>>>     void remove_fwl_configs(struct fwl_data *fwl_data, size_t
>>>>>>> fwl_data_size);
>>>>>>> +void start_non_linux_remote_cores(void);
>>>>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32
>>>>>>> *loadaddr);
>>>>>>> diff --git a/arch/arm/mach-k3/j721e_init.c
>>>>>>> b/arch/arm/mach-k3/j721e_init.c
>>>>>>> index f7f7398081..c5f8ede1a0 100644
>>>>>>> --- a/arch/arm/mach-k3/j721e_init.c
>>>>>>> +++ b/arch/arm/mach-k3/j721e_init.c
>>>>>>> @@ -18,6 +18,7 @@
>>>>>>>     #include <dm.h>
>>>>>>>     #include <dm/uclass-internal.h>
>>>>>>>     #include <dm/pinctrl.h>
>>>>>>> +#include <remoteproc.h>
>>>>>>>       #ifdef CONFIG_SPL_BUILD
>>>>>>>     #ifdef CONFIG_K3_LOAD_SYSFW
>>>>>>> @@ -295,3 +296,36 @@ void release_resources_for_core_shutdown(void)
>>>>>>>         }
>>>>>>>     }
>>>>>>>     #endif
>>>>>>> +
>>>>>>> +#ifdef CONFIG_SYS_K3_SPL_ATF
>>>>>>> +void start_non_linux_remote_cores(void)
>>>>>>> +{
>>>>>>> +    int size = 0, ret;
>>>>>>> +    u32 loadaddr = 0;
>>>>>>> +
>>>>>>> +    size = load_firmware("mainr5f0_0fwname", "mainr5f0_0loadaddr",
>>>>>>> +                 &loadaddr);
>>>>>>> +    if (size <= 0)
>>>>>>> +        goto err_load;
>>>>>>> +
>>>>>>> +    /*  remoteproc 2 is aliased for the needed remotecore */
>>>>>>
>>>>>>
>>>>>> Assuming the big-arm core to boot is remoteproc 1 was reasonable, but
>>>>>> there needs to be a better what than assuming the number for every
>>>>>> other
>>>>>> remote core.
>>>>>>
>>>>>>
>>>>>>> +    ret = rproc_load(2, loadaddr, size);
>>>>>>> +    if (ret) {
>>>>>>> +        printf("Firmware failed to start on rproc (%d)\n", ret);
>>>>>>> +        goto err_load;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    ret = rproc_start(2);
>>>>>>> +    if (ret) {
>>>>>>> +        printf("Firmware init failed on rproc (%d)\n", ret);
>>>>>>> +        goto err_load;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    printf("Remoteproc 2 started successfully\n");
>>>>>>
>>>>>>
>>>>>> That's useful..
>>>>>
>>>>> That is a print that tells everything went well with rproc 2.
>>>>> Otherwise
>>>>> one has to really find other ways to see if it succeeded or not.
>>>>>
>>>>
>>>>
>>>> I'm just a customer booting my board, I have no idea what a "Remoteproc
>>>> 2" is. I'm saying make the message describe what has happened.
>>>>
>>>> Andrew
>>>>
>>>>
>>>>>>
>>>>>> Andrew
>>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +    return;
>>>>>>> +
>>>>>>> +err_load:
>>>>>>> +    rproc_reset(2);
>>>>>>> +}
>>>>>>> +#endif
>>>>>>>

[PATCH v3 02/10] arm: k3: Add support for loading non linux remote cores

[Keerthy]

On 23/01/20 10:35 pm, Andrew F. Davis wrote:
> On 1/23/20 11:44 AM, Keerthy wrote:
>>
>>
>> On 23/01/20 6:54 pm, Andrew F. Davis wrote:
>>> On 1/22/20 11:10 PM, Keerthy wrote:
>>>>
>>>>
>>>> On 22/01/20 9:55 pm, Andrew F. Davis wrote:
>>>>> On 1/21/20 8:10 PM, keerthy wrote:
>>>>>>
>>>>>>
>>>>>> On 1/21/2020 6:26 PM, Andrew F. Davis wrote:
>>>>>>> On 1/21/20 6:07 AM, Keerthy wrote:
>>>>>>>> Add MAIN domain R5FSS0 remoteproc support from spl. This enables
>>>>>>>> loading the elf firmware in SPL and starting the remotecore.
>>>>>>>>
>>>>>>>> In order to start the core, there should be a file with path
>>>>>>>> "/lib/firmware/j7-main-r5f0_0-fw" under filesystem
>>>>>>>> of respective boot mode.
>>>>>>>>
>>>>>>>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>>>>>>>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>>>>>>>> [Guard start_non_linux_remote_cores under CONFIG_FS_LOADER]
>>>>>>>> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
>>>>>>>> ---
>>>>>>>>      arch/arm/mach-k3/common.c     | 84
>>>>>>>> ++++++++++++++++++++++++++++++++---
>>>>>>>>      arch/arm/mach-k3/common.h     |  2 +
>>>>>>>>      arch/arm/mach-k3/j721e_init.c | 34 ++++++++++++++
>>>>>>>>      3 files changed, 115 insertions(+), 5 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
>>>>>>>> index 8d1529062d..f0ac0c39f1 100644
>>>>>>>> --- a/arch/arm/mach-k3/common.c
>>>>>>>> +++ b/arch/arm/mach-k3/common.c
>>>>>>>> @@ -16,6 +16,10 @@
>>>>>>>>      #include <asm/arch/sys_proto.h>
>>>>>>>>      #include <asm/hardware.h>
>>>>>>>>      #include <asm/io.h>
>>>>>>>> +#include <fs_loader.h>
>>>>>>>> +#include <fs.h>
>>>>>>>> +#include <env.h>
>>>>>>>> +#include <elf.h>
>>>>>>>>        struct ti_sci_handle *get_ti_sci_handle(void)
>>>>>>>>      {
>>>>>>>> @@ -57,6 +61,74 @@ int early_console_init(void)
>>>>>>>>      #endif
>>>>>>>>        #ifdef CONFIG_SYS_K3_SPL_ATF
>>>>>>>> +
>>>>>>>> +void init_env(void)
>>>>>>>> +{
>>>>>>>> +#ifdef CONFIG_SPL_ENV_SUPPORT
>>>>>>>> +    char *part;
>>>>>>>> +
>>>>>>>> +    env_init();
>>>>>>>> +    env_load();
>>>>>>>> +    switch (spl_boot_device()) {
>>>>>>>> +    case BOOT_DEVICE_MMC2:
>>>>>>>> +        part = env_get("bootpart");
>>>>>>>> +        env_set("storage_interface", "mmc");
>>>>>>>> +        env_set("fw_dev_part", part);
>>>>>>>> +        break;
>>>>>>>> +    case BOOT_DEVICE_SPI:
>>>>>>>> +        env_set("storage_interface", "ubi");
>>>>>>>> +        env_set("fw_ubi_mtdpart", "UBI");
>>>>>>>> +        env_set("fw_ubi_volume", "UBI0");
>>>>>>>> +        break;
>>>>>>>> +    default:
>>>>>>>> +        printf("%s from device %u not supported!\n",
>>>>>>>> +               __func__, spl_boot_device());
>>>>>>>
>>>>>>>
>>>>>>> This will print for almost every boot mode..
>>>>>>
>>>>>> I can keep this under debug.
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> +        return;
>>>>>>>> +    }
>>>>>>>> +#endif
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +#ifdef CONFIG_FS_LOADER
>>>>>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32
>>>>>>>> *loadaddr)
>>>>>>>> +{
>>>>>>>> +    struct udevice *fsdev;
>>>>>>>> +    char *name = NULL;
>>>>>>>> +    int size = 0;
>>>>>>>> +
>>>>>>>> +    *loadaddr = 0;
>>>>>>>> +#ifdef CONFIG_SPL_ENV_SUPPORT
>>>>>>>> +    switch (spl_boot_device()) {
>>>>>>>> +    case BOOT_DEVICE_MMC2:
>>>>>>>> +        name = env_get(name_fw);
>>>>>>>> +        *loadaddr = env_get_hex(name_loadaddr, *loadaddr);
>>>>>>>> +        break;
>>>>>>>> +    default:
>>>>>>>> +        printf("Loading rproc fw image from device %u not
>>>>>>>> supported!\n",
>>>>>>>> +               spl_boot_device());
>>>>>>>
>>>>>>>
>>>>>>> This whole thing seems very MMC specific, if early firmware
>>>>>>> loading is
>>>>>>> important it should work for all boot modes. Find a way to include
>>>>>>> it in
>>>>>>> the next boot stage FIT image (tispl.bin) so it works for all modes.
>>>>>>
>>>>>> That was not NAKd. We are going with fs_loader approach.
>>>>>>
>>>>>
>>>>>
>>>>> When, where, link?
>>>>
>>>> I had implemented that way internally. That was rejected for multiple
>>>> right reasons:
>>>>
>>>
>>>
>>> I must have missed the internal reviews for this, anyway this is posted
>>> upstream so lets discus it here.
>>>
>>>
>>>> 1) SPL size would bloat based on the size of the firmware.
>>>
>>>
>>> SPL size would remain constant, the combined FIT (tispl.bin) would grow,
>>> but that is okay as DRAM is enabled at this point so we have no hard
>>> memory constraints.
>>
>> I meant the FIT image containing the SPL will bloat.
> 
> 
> Exactly what I said, and so size is not a huge deal.
> 
> 
>>
>>>
>>>
>>>> 2) There are multiple cores that need to be loaded and hence adding all
>>>> the firmwares under a fit can be really painful.
>>>
>>>
>>> Bundling images is what FIT is for, are you saying the better solution
>>> is to hard-code each firmware starting like done here?
>>
>> How many firmwares will you go on bundling. Firmwares are already kept
>> in file system. It is a matter of reading them from there.
>>
> 
> 
> If we are early booting them from SPL then they don't really need to be
> on the filesystem.
> 
> 
>>>
>>>
>>>> 3) Changing firmware means building the tispl.bin again.
>>>>
>>>
>>>
>>> FIT images can be disassembled and reassembled with a script around
>>> tools/dumpimage.
>>
>> And you expect everyone to master that instead of looking at couple of
>> aliases in DT to figure out which core corresponds to which ID?
>>
> 
> 
> Your patches do more than add DT aliases to add a firmware image. I
> think you are responding to the wrong comment here, the ID part is below.
> 
> 
>>>
>>> SPL should be simple and load the one next stage.
>>>
>>>
>>>> The FIT solution can not scale well.
>>>>
>>>
>>>
>>> How does this current series scale at all? At least with FIT you can add
>>> more images without adding code for
>>> request_firmware(<hard-coded-firmware-name>) and
>>> rproc_load(<hard-coded-number>). That all could be encoded in the FIT
>>> data.
>>
>> I understand and as explained earlier i have even implemented that once
>> before. fs_loader was meant to address the exact use case we are
>> discussing about.
>>
>> Even in u-boot remotecores are started/loaded by indices. Users need to
>> know them. This is no different than that.
>>
>> I am not convinced about FIT approach. I would let Lokesh take a call on
>> this.
>>
> 
> 
> Lokesh is not the whole U-Boot community, I get you two aligned
> internally on this, but I'd be much more interested in Tom or Simon's
> opinion here. I was doing the same thing when loading PMMC firmware for
> HS and was pushed to make a FIT friendly version, I'm glad that I did,
> it ended up much less hacky in the long run.

I would love to hear Tom & simon's opinions. Before we jump on to PMMC 
fimrware example. I believe there is one such firmware that gets 
appended to the FIT image. In case of remoteprocs there can be multiple 
such firmware. File system is the most generic and scaleble place to 
access firmware.

Lokesh/Sekhar/Tero,

You can share your viewpoints as well.

Thanks,
Keerthy
> 
> Andrew
> 
> 
>> Thanks,
>> Keerthy
>>>
>>> Andrew
>>>
>>>
>>>> - Keerthy
>>>>>
>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> +        return 0;
>>>>>>>> +    }
>>>>>>>> +#endif
>>>>>>>> +    if (!*loadaddr)
>>>>>>>> +        return 0;
>>>>>>>> +
>>>>>>>> +    if (!uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0, &fsdev)) {
>>>>>>>> +        size = request_firmware_into_buf(fsdev, name, (void
>>>>>>>> *)*loadaddr,
>>>>>>>> +                         0, 0);
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    return size;
>>>>>>>> +}
>>>>>>>> +#else
>>>>>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32
>>>>>>>> *loadaddr)
>>>>>>>> +{
>>>>>>>> +    return 0;
>>>>>>>> +}
>>>>>>>> +#endif
>>>>>>>> +
>>>>>>>> +__weak void start_non_linux_remote_cores(void)
>>>>>>>> +{
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>      void __noreturn jump_to_image_no_args(struct spl_image_info
>>>>>>>> *spl_image)
>>>>>>>>      {
>>>>>>>>          struct ti_sci_handle *ti_sci = get_ti_sci_handle();
>>>>>>>> @@ -65,15 +137,17 @@ void __noreturn jump_to_image_no_args(struct
>>>>>>>> spl_image_info *spl_image)
>>>>>>>>          /* Release all the exclusive devices held by SPL before
>>>>>>>> starting ATF */
>>>>>>>>          ti_sci->ops.dev_ops.release_exclusive_devices(ti_sci);
>>>>>>>>      +    ret = rproc_init();
>>>>>>>> +    if (ret)
>>>>>>>> +        panic("rproc failed to be initialized (%d)\n", ret);
>>>>>>>> +
>>>>>>>> +    init_env();
>>>>>>>> +    start_non_linux_remote_cores();
>>>>>>>> +
>>>>>>>>          /*
>>>>>>>>           * It is assumed that remoteproc device 1 is the
>>>>>>>> corresponding
>>>>>>>>           * Cortex-A core which runs ATF. Make sure DT reflects the
>>>>>>>> same.
>>>>>>>>           */
>>>>>>>> -    ret = rproc_dev_init(1);
>>>>>>>> -    if (ret)
>>>>>>>> -        panic("%s: ATF failed to initialize on rproc (%d)\n",
>>>>>>>> __func__,
>>>>>>>> -              ret);
>>>>>>>> -
>>>>>>>
>>>>>>>
>>>>>>> Where did this code go?
>>>>>>
>>>>>> rproc_init takes care of that.
>>>>>>
>>>>>
>>>>>
>>>>> Is that new behavior then? It should be it's own patch with a commit
>>>>> message about that.
>>>>>
>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>          ret = rproc_load(1, spl_image->entry_point, 0x200);
>>>>>>>>          if (ret)
>>>>>>>>              panic("%s: ATF failed to load on rproc (%d)\n",
>>>>>>>> __func__,
>>>>>>>> ret);
>>>>>>>> diff --git a/arch/arm/mach-k3/common.h b/arch/arm/mach-k3/common.h
>>>>>>>> index d8b34fe060..42fb8ee6e7 100644
>>>>>>>> --- a/arch/arm/mach-k3/common.h
>>>>>>>> +++ b/arch/arm/mach-k3/common.h
>>>>>>>> @@ -24,3 +24,5 @@ void setup_k3_mpu_regions(void);
>>>>>>>>      int early_console_init(void);
>>>>>>>>      void disable_linefill_optimization(void);
>>>>>>>>      void remove_fwl_configs(struct fwl_data *fwl_data, size_t
>>>>>>>> fwl_data_size);
>>>>>>>> +void start_non_linux_remote_cores(void);
>>>>>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32
>>>>>>>> *loadaddr);
>>>>>>>> diff --git a/arch/arm/mach-k3/j721e_init.c
>>>>>>>> b/arch/arm/mach-k3/j721e_init.c
>>>>>>>> index f7f7398081..c5f8ede1a0 100644
>>>>>>>> --- a/arch/arm/mach-k3/j721e_init.c
>>>>>>>> +++ b/arch/arm/mach-k3/j721e_init.c
>>>>>>>> @@ -18,6 +18,7 @@
>>>>>>>>      #include <dm.h>
>>>>>>>>      #include <dm/uclass-internal.h>
>>>>>>>>      #include <dm/pinctrl.h>
>>>>>>>> +#include <remoteproc.h>
>>>>>>>>        #ifdef CONFIG_SPL_BUILD
>>>>>>>>      #ifdef CONFIG_K3_LOAD_SYSFW
>>>>>>>> @@ -295,3 +296,36 @@ void release_resources_for_core_shutdown(void)
>>>>>>>>          }
>>>>>>>>      }
>>>>>>>>      #endif
>>>>>>>> +
>>>>>>>> +#ifdef CONFIG_SYS_K3_SPL_ATF
>>>>>>>> +void start_non_linux_remote_cores(void)
>>>>>>>> +{
>>>>>>>> +    int size = 0, ret;
>>>>>>>> +    u32 loadaddr = 0;
>>>>>>>> +
>>>>>>>> +    size = load_firmware("mainr5f0_0fwname", "mainr5f0_0loadaddr",
>>>>>>>> +                 &loadaddr);
>>>>>>>> +    if (size <= 0)
>>>>>>>> +        goto err_load;
>>>>>>>> +
>>>>>>>> +    /*  remoteproc 2 is aliased for the needed remotecore */
>>>>>>>
>>>>>>>
>>>>>>> Assuming the big-arm core to boot is remoteproc 1 was reasonable, but
>>>>>>> there needs to be a better what than assuming the number for every
>>>>>>> other
>>>>>>> remote core.
>>>>>>>
>>>>>>>
>>>>>>>> +    ret = rproc_load(2, loadaddr, size);
>>>>>>>> +    if (ret) {
>>>>>>>> +        printf("Firmware failed to start on rproc (%d)\n", ret);
>>>>>>>> +        goto err_load;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    ret = rproc_start(2);
>>>>>>>> +    if (ret) {
>>>>>>>> +        printf("Firmware init failed on rproc (%d)\n", ret);
>>>>>>>> +        goto err_load;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    printf("Remoteproc 2 started successfully\n");
>>>>>>>
>>>>>>>
>>>>>>> That's useful..
>>>>>>
>>>>>> That is a print that tells everything went well with rproc 2.
>>>>>> Otherwise
>>>>>> one has to really find other ways to see if it succeeded or not.
>>>>>>
>>>>>
>>>>>
>>>>> I'm just a customer booting my board, I have no idea what a "Remoteproc
>>>>> 2" is. I'm saying make the message describe what has happened.
>>>>>
>>>>> Andrew
>>>>>
>>>>>
>>>>>>>
>>>>>>> Andrew
>>>>>>>
>>>>>>>
>>>>>>>> +
>>>>>>>> +    return;
>>>>>>>> +
>>>>>>>> +err_load:
>>>>>>>> +    rproc_reset(2);
>>>>>>>> +}
>>>>>>>> +#endif
>>>>>>>>

[PATCH v3 02/10] arm: k3: Add support for loading non linux remote cores

[Tero Kristo]

On 23/01/2020 19:19, Keerthy wrote:
> 
> 
> On 23/01/20 10:35 pm, Andrew F. Davis wrote:
>> On 1/23/20 11:44 AM, Keerthy wrote:
>>>
>>>
>>> On 23/01/20 6:54 pm, Andrew F. Davis wrote:
>>>> On 1/22/20 11:10 PM, Keerthy wrote:
>>>>>
>>>>>
>>>>> On 22/01/20 9:55 pm, Andrew F. Davis wrote:
>>>>>> On 1/21/20 8:10 PM, keerthy wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 1/21/2020 6:26 PM, Andrew F. Davis wrote:
>>>>>>>> On 1/21/20 6:07 AM, Keerthy wrote:
>>>>>>>>> Add MAIN domain R5FSS0 remoteproc support from spl. This enables
>>>>>>>>> loading the elf firmware in SPL and starting the remotecore.
>>>>>>>>>
>>>>>>>>> In order to start the core, there should be a file with path
>>>>>>>>> "/lib/firmware/j7-main-r5f0_0-fw" under filesystem
>>>>>>>>> of respective boot mode.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>>>>>>>>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>>>>>>>>> [Guard start_non_linux_remote_cores under CONFIG_FS_LOADER]
>>>>>>>>> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
>>>>>>>>> ---
>>>>>>>>>      arch/arm/mach-k3/common.c     | 84
>>>>>>>>> ++++++++++++++++++++++++++++++++---
>>>>>>>>>      arch/arm/mach-k3/common.h     |  2 +
>>>>>>>>>      arch/arm/mach-k3/j721e_init.c | 34 ++++++++++++++
>>>>>>>>>      3 files changed, 115 insertions(+), 5 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
>>>>>>>>> index 8d1529062d..f0ac0c39f1 100644
>>>>>>>>> --- a/arch/arm/mach-k3/common.c
>>>>>>>>> +++ b/arch/arm/mach-k3/common.c
>>>>>>>>> @@ -16,6 +16,10 @@
>>>>>>>>>      #include <asm/arch/sys_proto.h>
>>>>>>>>>      #include <asm/hardware.h>
>>>>>>>>>      #include <asm/io.h>
>>>>>>>>> +#include <fs_loader.h>
>>>>>>>>> +#include <fs.h>
>>>>>>>>> +#include <env.h>
>>>>>>>>> +#include <elf.h>
>>>>>>>>>        struct ti_sci_handle *get_ti_sci_handle(void)
>>>>>>>>>      {
>>>>>>>>> @@ -57,6 +61,74 @@ int early_console_init(void)
>>>>>>>>>      #endif
>>>>>>>>>        #ifdef CONFIG_SYS_K3_SPL_ATF
>>>>>>>>> +
>>>>>>>>> +void init_env(void)
>>>>>>>>> +{
>>>>>>>>> +#ifdef CONFIG_SPL_ENV_SUPPORT
>>>>>>>>> +    char *part;
>>>>>>>>> +
>>>>>>>>> +    env_init();
>>>>>>>>> +    env_load();
>>>>>>>>> +    switch (spl_boot_device()) {
>>>>>>>>> +    case BOOT_DEVICE_MMC2:
>>>>>>>>> +        part = env_get("bootpart");
>>>>>>>>> +        env_set("storage_interface", "mmc");
>>>>>>>>> +        env_set("fw_dev_part", part);
>>>>>>>>> +        break;
>>>>>>>>> +    case BOOT_DEVICE_SPI:
>>>>>>>>> +        env_set("storage_interface", "ubi");
>>>>>>>>> +        env_set("fw_ubi_mtdpart", "UBI");
>>>>>>>>> +        env_set("fw_ubi_volume", "UBI0");
>>>>>>>>> +        break;
>>>>>>>>> +    default:
>>>>>>>>> +        printf("%s from device %u not supported!\n",
>>>>>>>>> +               __func__, spl_boot_device());
>>>>>>>>
>>>>>>>>
>>>>>>>> This will print for almost every boot mode..
>>>>>>>
>>>>>>> I can keep this under debug.
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> +        return;
>>>>>>>>> +    }
>>>>>>>>> +#endif
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +#ifdef CONFIG_FS_LOADER
>>>>>>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32
>>>>>>>>> *loadaddr)
>>>>>>>>> +{
>>>>>>>>> +    struct udevice *fsdev;
>>>>>>>>> +    char *name = NULL;
>>>>>>>>> +    int size = 0;
>>>>>>>>> +
>>>>>>>>> +    *loadaddr = 0;
>>>>>>>>> +#ifdef CONFIG_SPL_ENV_SUPPORT
>>>>>>>>> +    switch (spl_boot_device()) {
>>>>>>>>> +    case BOOT_DEVICE_MMC2:
>>>>>>>>> +        name = env_get(name_fw);
>>>>>>>>> +        *loadaddr = env_get_hex(name_loadaddr, *loadaddr);
>>>>>>>>> +        break;
>>>>>>>>> +    default:
>>>>>>>>> +        printf("Loading rproc fw image from device %u not
>>>>>>>>> supported!\n",
>>>>>>>>> +               spl_boot_device());
>>>>>>>>
>>>>>>>>
>>>>>>>> This whole thing seems very MMC specific, if early firmware
>>>>>>>> loading is
>>>>>>>> important it should work for all boot modes. Find a way to include
>>>>>>>> it in
>>>>>>>> the next boot stage FIT image (tispl.bin) so it works for all 
>>>>>>>> modes.
>>>>>>>
>>>>>>> That was not NAKd. We are going with fs_loader approach.
>>>>>>>
>>>>>>
>>>>>>
>>>>>> When, where, link?
>>>>>
>>>>> I had implemented that way internally. That was rejected for multiple
>>>>> right reasons:
>>>>>
>>>>
>>>>
>>>> I must have missed the internal reviews for this, anyway this is posted
>>>> upstream so lets discus it here.
>>>>
>>>>
>>>>> 1) SPL size would bloat based on the size of the firmware.
>>>>
>>>>
>>>> SPL size would remain constant, the combined FIT (tispl.bin) would 
>>>> grow,
>>>> but that is okay as DRAM is enabled at this point so we have no hard
>>>> memory constraints.
>>>
>>> I meant the FIT image containing the SPL will bloat.
>>
>>
>> Exactly what I said, and so size is not a huge deal.
>>
>>
>>>
>>>>
>>>>
>>>>> 2) There are multiple cores that need to be loaded and hence adding 
>>>>> all
>>>>> the firmwares under a fit can be really painful.
>>>>
>>>>
>>>> Bundling images is what FIT is for, are you saying the better solution
>>>> is to hard-code each firmware starting like done here?
>>>
>>> How many firmwares will you go on bundling. Firmwares are already kept
>>> in file system. It is a matter of reading them from there.
>>>
>>
>>
>> If we are early booting them from SPL then they don't really need to be
>> on the filesystem.
>>
>>
>>>>
>>>>
>>>>> 3) Changing firmware means building the tispl.bin again.
>>>>>
>>>>
>>>>
>>>> FIT images can be disassembled and reassembled with a script around
>>>> tools/dumpimage.
>>>
>>> And you expect everyone to master that instead of looking at couple of
>>> aliases in DT to figure out which core corresponds to which ID?
>>>
>>
>>
>> Your patches do more than add DT aliases to add a firmware image. I
>> think you are responding to the wrong comment here, the ID part is below.
>>
>>
>>>>
>>>> SPL should be simple and load the one next stage.
>>>>
>>>>
>>>>> The FIT solution can not scale well.
>>>>>
>>>>
>>>>
>>>> How does this current series scale at all? At least with FIT you can 
>>>> add
>>>> more images without adding code for
>>>> request_firmware(<hard-coded-firmware-name>) and
>>>> rproc_load(<hard-coded-number>). That all could be encoded in the FIT
>>>> data.
>>>
>>> I understand and as explained earlier i have even implemented that once
>>> before. fs_loader was meant to address the exact use case we are
>>> discussing about.
>>>
>>> Even in u-boot remotecores are started/loaded by indices. Users need to
>>> know them. This is no different than that.
>>>
>>> I am not convinced about FIT approach. I would let Lokesh take a call on
>>> this.
>>>
>>
>>
>> Lokesh is not the whole U-Boot community, I get you two aligned
>> internally on this, but I'd be much more interested in Tom or Simon's
>> opinion here. I was doing the same thing when loading PMMC firmware for
>> HS and was pushed to make a FIT friendly version, I'm glad that I did,
>> it ended up much less hacky in the long run.
> 
> I would love to hear Tom & simon's opinions. Before we jump on to PMMC 
> fimrware example. I believe there is one such firmware that gets 
> appended to the FIT image. In case of remoteprocs there can be multiple 
> such firmware. File system is the most generic and scaleble place to 
> access firmware.
> 
> Lokesh/Sekhar/Tero,
> 
> You can share your viewpoints as well.

I think the initial reason to split the firmwares away from the FIT was 
that we didn't want to have ROM load everything for us, and so that SPL 
would come up earlier. Couple of reasons behind that, the size of the 
FIT image directly transforms into time spent loading it, and also ROM 
might not be loading things at optimal speed... There are couple of 
critical firmwares that need to be booted up as soon as possible.

-Tero

> 
> Thanks,
> Keerthy
>>
>> Andrew
>>
>>
>>> Thanks,
>>> Keerthy
>>>>
>>>> Andrew
>>>>
>>>>
>>>>> - Keerthy
>>>>>>
>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> +        return 0;
>>>>>>>>> +    }
>>>>>>>>> +#endif
>>>>>>>>> +    if (!*loadaddr)
>>>>>>>>> +        return 0;
>>>>>>>>> +
>>>>>>>>> +    if (!uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0, 
>>>>>>>>> &fsdev)) {
>>>>>>>>> +        size = request_firmware_into_buf(fsdev, name, (void
>>>>>>>>> *)*loadaddr,
>>>>>>>>> +                         0, 0);
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    return size;
>>>>>>>>> +}
>>>>>>>>> +#else
>>>>>>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32
>>>>>>>>> *loadaddr)
>>>>>>>>> +{
>>>>>>>>> +    return 0;
>>>>>>>>> +}
>>>>>>>>> +#endif
>>>>>>>>> +
>>>>>>>>> +__weak void start_non_linux_remote_cores(void)
>>>>>>>>> +{
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>>      void __noreturn jump_to_image_no_args(struct spl_image_info
>>>>>>>>> *spl_image)
>>>>>>>>>      {
>>>>>>>>>          struct ti_sci_handle *ti_sci = get_ti_sci_handle();
>>>>>>>>> @@ -65,15 +137,17 @@ void __noreturn jump_to_image_no_args(struct
>>>>>>>>> spl_image_info *spl_image)
>>>>>>>>>          /* Release all the exclusive devices held by SPL before
>>>>>>>>> starting ATF */
>>>>>>>>>          ti_sci->ops.dev_ops.release_exclusive_devices(ti_sci);
>>>>>>>>>      +    ret = rproc_init();
>>>>>>>>> +    if (ret)
>>>>>>>>> +        panic("rproc failed to be initialized (%d)\n", ret);
>>>>>>>>> +
>>>>>>>>> +    init_env();
>>>>>>>>> +    start_non_linux_remote_cores();
>>>>>>>>> +
>>>>>>>>>          /*
>>>>>>>>>           * It is assumed that remoteproc device 1 is the
>>>>>>>>> corresponding
>>>>>>>>>           * Cortex-A core which runs ATF. Make sure DT reflects 
>>>>>>>>> the
>>>>>>>>> same.
>>>>>>>>>           */
>>>>>>>>> -    ret = rproc_dev_init(1);
>>>>>>>>> -    if (ret)
>>>>>>>>> -        panic("%s: ATF failed to initialize on rproc (%d)\n",
>>>>>>>>> __func__,
>>>>>>>>> -              ret);
>>>>>>>>> -
>>>>>>>>
>>>>>>>>
>>>>>>>> Where did this code go?
>>>>>>>
>>>>>>> rproc_init takes care of that.
>>>>>>>
>>>>>>
>>>>>>
>>>>>> Is that new behavior then? It should be it's own patch with a commit
>>>>>> message about that.
>>>>>>
>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>          ret = rproc_load(1, spl_image->entry_point, 0x200);
>>>>>>>>>          if (ret)
>>>>>>>>>              panic("%s: ATF failed to load on rproc (%d)\n",
>>>>>>>>> __func__,
>>>>>>>>> ret);
>>>>>>>>> diff --git a/arch/arm/mach-k3/common.h b/arch/arm/mach-k3/common.h
>>>>>>>>> index d8b34fe060..42fb8ee6e7 100644
>>>>>>>>> --- a/arch/arm/mach-k3/common.h
>>>>>>>>> +++ b/arch/arm/mach-k3/common.h
>>>>>>>>> @@ -24,3 +24,5 @@ void setup_k3_mpu_regions(void);
>>>>>>>>>      int early_console_init(void);
>>>>>>>>>      void disable_linefill_optimization(void);
>>>>>>>>>      void remove_fwl_configs(struct fwl_data *fwl_data, size_t
>>>>>>>>> fwl_data_size);
>>>>>>>>> +void start_non_linux_remote_cores(void);
>>>>>>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32
>>>>>>>>> *loadaddr);
>>>>>>>>> diff --git a/arch/arm/mach-k3/j721e_init.c
>>>>>>>>> b/arch/arm/mach-k3/j721e_init.c
>>>>>>>>> index f7f7398081..c5f8ede1a0 100644
>>>>>>>>> --- a/arch/arm/mach-k3/j721e_init.c
>>>>>>>>> +++ b/arch/arm/mach-k3/j721e_init.c
>>>>>>>>> @@ -18,6 +18,7 @@
>>>>>>>>>      #include <dm.h>
>>>>>>>>>      #include <dm/uclass-internal.h>
>>>>>>>>>      #include <dm/pinctrl.h>
>>>>>>>>> +#include <remoteproc.h>
>>>>>>>>>        #ifdef CONFIG_SPL_BUILD
>>>>>>>>>      #ifdef CONFIG_K3_LOAD_SYSFW
>>>>>>>>> @@ -295,3 +296,36 @@ void 
>>>>>>>>> release_resources_for_core_shutdown(void)
>>>>>>>>>          }
>>>>>>>>>      }
>>>>>>>>>      #endif
>>>>>>>>> +
>>>>>>>>> +#ifdef CONFIG_SYS_K3_SPL_ATF
>>>>>>>>> +void start_non_linux_remote_cores(void)
>>>>>>>>> +{
>>>>>>>>> +    int size = 0, ret;
>>>>>>>>> +    u32 loadaddr = 0;
>>>>>>>>> +
>>>>>>>>> +    size = load_firmware("mainr5f0_0fwname", 
>>>>>>>>> "mainr5f0_0loadaddr",
>>>>>>>>> +                 &loadaddr);
>>>>>>>>> +    if (size <= 0)
>>>>>>>>> +        goto err_load;
>>>>>>>>> +
>>>>>>>>> +    /*  remoteproc 2 is aliased for the needed remotecore */
>>>>>>>>
>>>>>>>>
>>>>>>>> Assuming the big-arm core to boot is remoteproc 1 was 
>>>>>>>> reasonable, but
>>>>>>>> there needs to be a better what than assuming the number for every
>>>>>>>> other
>>>>>>>> remote core.
>>>>>>>>
>>>>>>>>
>>>>>>>>> +    ret = rproc_load(2, loadaddr, size);
>>>>>>>>> +    if (ret) {
>>>>>>>>> +        printf("Firmware failed to start on rproc (%d)\n", ret);
>>>>>>>>> +        goto err_load;
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    ret = rproc_start(2);
>>>>>>>>> +    if (ret) {
>>>>>>>>> +        printf("Firmware init failed on rproc (%d)\n", ret);
>>>>>>>>> +        goto err_load;
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    printf("Remoteproc 2 started successfully\n");
>>>>>>>>
>>>>>>>>
>>>>>>>> That's useful..
>>>>>>>
>>>>>>> That is a print that tells everything went well with rproc 2.
>>>>>>> Otherwise
>>>>>>> one has to really find other ways to see if it succeeded or not.
>>>>>>>
>>>>>>
>>>>>>
>>>>>> I'm just a customer booting my board, I have no idea what a 
>>>>>> "Remoteproc
>>>>>> 2" is. I'm saying make the message describe what has happened.
>>>>>>
>>>>>> Andrew
>>>>>>
>>>>>>
>>>>>>>>
>>>>>>>> Andrew
>>>>>>>>
>>>>>>>>
>>>>>>>>> +
>>>>>>>>> +    return;
>>>>>>>>> +
>>>>>>>>> +err_load:
>>>>>>>>> +    rproc_reset(2);
>>>>>>>>> +}
>>>>>>>>> +#endif
>>>>>>>>>

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH v3 02/10] arm: k3: Add support for loading non linux remote cores

[Keerthy]

On 23/01/20 10:49 pm, Keerthy wrote:
> 
> 
> On 23/01/20 10:35 pm, Andrew F. Davis wrote:
>> On 1/23/20 11:44 AM, Keerthy wrote:
>>>
>>>
>>> On 23/01/20 6:54 pm, Andrew F. Davis wrote:
>>>> On 1/22/20 11:10 PM, Keerthy wrote:
>>>>>
>>>>>
>>>>> On 22/01/20 9:55 pm, Andrew F. Davis wrote:
>>>>>> On 1/21/20 8:10 PM, keerthy wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 1/21/2020 6:26 PM, Andrew F. Davis wrote:
>>>>>>>> On 1/21/20 6:07 AM, Keerthy wrote:
>>>>>>>>> Add MAIN domain R5FSS0 remoteproc support from spl. This enables
>>>>>>>>> loading the elf firmware in SPL and starting the remotecore.
>>>>>>>>>
>>>>>>>>> In order to start the core, there should be a file with path
>>>>>>>>> "/lib/firmware/j7-main-r5f0_0-fw" under filesystem
>>>>>>>>> of respective boot mode.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>>>>>>>>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>>>>>>>>> [Guard start_non_linux_remote_cores under CONFIG_FS_LOADER]
>>>>>>>>> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
>>>>>>>>> ---
>>>>>>>>>      arch/arm/mach-k3/common.c     | 84
>>>>>>>>> ++++++++++++++++++++++++++++++++---
>>>>>>>>>      arch/arm/mach-k3/common.h     |  2 +
>>>>>>>>>      arch/arm/mach-k3/j721e_init.c | 34 ++++++++++++++
>>>>>>>>>      3 files changed, 115 insertions(+), 5 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
>>>>>>>>> index 8d1529062d..f0ac0c39f1 100644
>>>>>>>>> --- a/arch/arm/mach-k3/common.c
>>>>>>>>> +++ b/arch/arm/mach-k3/common.c
>>>>>>>>> @@ -16,6 +16,10 @@
>>>>>>>>>      #include <asm/arch/sys_proto.h>
>>>>>>>>>      #include <asm/hardware.h>
>>>>>>>>>      #include <asm/io.h>
>>>>>>>>> +#include <fs_loader.h>
>>>>>>>>> +#include <fs.h>
>>>>>>>>> +#include <env.h>
>>>>>>>>> +#include <elf.h>
>>>>>>>>>        struct ti_sci_handle *get_ti_sci_handle(void)
>>>>>>>>>      {
>>>>>>>>> @@ -57,6 +61,74 @@ int early_console_init(void)
>>>>>>>>>      #endif
>>>>>>>>>        #ifdef CONFIG_SYS_K3_SPL_ATF
>>>>>>>>> +
>>>>>>>>> +void init_env(void)
>>>>>>>>> +{
>>>>>>>>> +#ifdef CONFIG_SPL_ENV_SUPPORT
>>>>>>>>> +    char *part;
>>>>>>>>> +
>>>>>>>>> +    env_init();
>>>>>>>>> +    env_load();
>>>>>>>>> +    switch (spl_boot_device()) {
>>>>>>>>> +    case BOOT_DEVICE_MMC2:
>>>>>>>>> +        part = env_get("bootpart");
>>>>>>>>> +        env_set("storage_interface", "mmc");
>>>>>>>>> +        env_set("fw_dev_part", part);
>>>>>>>>> +        break;
>>>>>>>>> +    case BOOT_DEVICE_SPI:
>>>>>>>>> +        env_set("storage_interface", "ubi");
>>>>>>>>> +        env_set("fw_ubi_mtdpart", "UBI");
>>>>>>>>> +        env_set("fw_ubi_volume", "UBI0");
>>>>>>>>> +        break;
>>>>>>>>> +    default:
>>>>>>>>> +        printf("%s from device %u not supported!\n",
>>>>>>>>> +               __func__, spl_boot_device());
>>>>>>>>
>>>>>>>>
>>>>>>>> This will print for almost every boot mode..
>>>>>>>
>>>>>>> I can keep this under debug.
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> +        return;
>>>>>>>>> +    }
>>>>>>>>> +#endif
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +#ifdef CONFIG_FS_LOADER
>>>>>>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32
>>>>>>>>> *loadaddr)
>>>>>>>>> +{
>>>>>>>>> +    struct udevice *fsdev;
>>>>>>>>> +    char *name = NULL;
>>>>>>>>> +    int size = 0;
>>>>>>>>> +
>>>>>>>>> +    *loadaddr = 0;
>>>>>>>>> +#ifdef CONFIG_SPL_ENV_SUPPORT
>>>>>>>>> +    switch (spl_boot_device()) {
>>>>>>>>> +    case BOOT_DEVICE_MMC2:
>>>>>>>>> +        name = env_get(name_fw);
>>>>>>>>> +        *loadaddr = env_get_hex(name_loadaddr, *loadaddr);
>>>>>>>>> +        break;
>>>>>>>>> +    default:
>>>>>>>>> +        printf("Loading rproc fw image from device %u not
>>>>>>>>> supported!\n",
>>>>>>>>> +               spl_boot_device());
>>>>>>>>
>>>>>>>>
>>>>>>>> This whole thing seems very MMC specific, if early firmware
>>>>>>>> loading is
>>>>>>>> important it should work for all boot modes. Find a way to include
>>>>>>>> it in
>>>>>>>> the next boot stage FIT image (tispl.bin) so it works for all 
>>>>>>>> modes.
>>>>>>>
>>>>>>> That was not NAKd. We are going with fs_loader approach.
>>>>>>>
>>>>>>
>>>>>>
>>>>>> When, where, link?
>>>>>
>>>>> I had implemented that way internally. That was rejected for multiple
>>>>> right reasons:
>>>>>
>>>>
>>>>
>>>> I must have missed the internal reviews for this, anyway this is posted
>>>> upstream so lets discus it here.
>>>>
>>>>
>>>>> 1) SPL size would bloat based on the size of the firmware.
>>>>
>>>>
>>>> SPL size would remain constant, the combined FIT (tispl.bin) would 
>>>> grow,
>>>> but that is okay as DRAM is enabled at this point so we have no hard
>>>> memory constraints.
>>>
>>> I meant the FIT image containing the SPL will bloat.
>>
>>
>> Exactly what I said, and so size is not a huge deal.
>>
>>
>>>
>>>>
>>>>
>>>>> 2) There are multiple cores that need to be loaded and hence adding 
>>>>> all
>>>>> the firmwares under a fit can be really painful.
>>>>
>>>>
>>>> Bundling images is what FIT is for, are you saying the better solution
>>>> is to hard-code each firmware starting like done here?
>>>
>>> How many firmwares will you go on bundling. Firmwares are already kept
>>> in file system. It is a matter of reading them from there.
>>>
>>
>>
>> If we are early booting them from SPL then they don't really need to be
>> on the filesystem.
>>
>>
>>>>
>>>>
>>>>> 3) Changing firmware means building the tispl.bin again.
>>>>>
>>>>
>>>>
>>>> FIT images can be disassembled and reassembled with a script around
>>>> tools/dumpimage.
>>>
>>> And you expect everyone to master that instead of looking at couple of
>>> aliases in DT to figure out which core corresponds to which ID?
>>>
>>
>>
>> Your patches do more than add DT aliases to add a firmware image. I
>> think you are responding to the wrong comment here, the ID part is below.
>>
>>
>>>>
>>>> SPL should be simple and load the one next stage.
>>>>
>>>>
>>>>> The FIT solution can not scale well.
>>>>>
>>>>
>>>>
>>>> How does this current series scale at all? At least with FIT you can 
>>>> add
>>>> more images without adding code for
>>>> request_firmware(<hard-coded-firmware-name>) and
>>>> rproc_load(<hard-coded-number>). That all could be encoded in the FIT
>>>> data.
>>>
>>> I understand and as explained earlier i have even implemented that once
>>> before. fs_loader was meant to address the exact use case we are
>>> discussing about.
>>>
>>> Even in u-boot remotecores are started/loaded by indices. Users need to
>>> know them. This is no different than that.
>>>
>>> I am not convinced about FIT approach. I would let Lokesh take a call on
>>> this.
>>>
>>
>>
>> Lokesh is not the whole U-Boot community, I get you two aligned
>> internally on this, but I'd be much more interested in Tom or Simon's
>> opinion here. I was doing the same thing when loading PMMC firmware for
>> HS and was pushed to make a FIT friendly version, I'm glad that I did,
>> it ended up much less hacky in the long run.
> 
> I would love to hear Tom & simon's opinions. Before we jump on to PMMC 
> fimrware example. I believe there is one such firmware that gets 
> appended to the FIT image. In case of remoteprocs there can be multiple 
> such firmware. File system is the most generic and scaleble place to 
> access firmware.
> 

Andrew,

PMMC in K2G is a case where the firmware image is targeted for a 
specific core and in K2G U-Boot only takes care of loading that 
particular core. Where as this series targets multiple cores which can 
cater to multiple use cases resulting in different combinations. To give 
more details, below are the approaches evaluated(prototyped):

Approach 1: Pack all the firmwares into a fit Image along with A72 SPL
----------------------------------------------------------------------
Pros:
-----
a) Boot media independent: No need to load firmware via file
system present in different boot media.

Cons:
-----
a) Flash image: In case of raw flash devices. We need to upfront
predict the partition that needs to be allocated for tispl.bin.
Complex SoCs like J721e has ~9 remote cores and each firmware can be 
~2-3MB resulting in ~20-30MB of tispl.bin. In case one has to add
a bigger firmware that would mean flash partition resizing. Also not all 
boards has flashes > 32MB.

In fact a prototype of this approach was tried. It failed as soon as
a larger firmware was included in the FIT image as the memory allocated
was not sufficient.

b) Dependency on U-Boot repo for changing firmwares: The firmwares need 
to be built as part of the FIT Image that contains the SPL. Firmware 
change means repacking the FIT image. Any customer/developer who intends 
to change the remote core firmware needs to be using the u-boot 
repository to pack the image into the FIT Image.

c) Mapping the firmware to remote cores: J7 has around 9 remote
cores(Mostly R5Fs & DSPs). Each firmware is position dependent.
There is no good way to map which firmware
corresponds to which remote core other than hard coding.

d) Use case support: Different use cases result in different combination 
of remote cores. Every different combination will result in a new binary.

e) Example build command would be:
make ARCH=arm R50=<> R51=<> R52=<>.........C7x=<> C6x0=<>

You can see it :)

Approach 2: Loading the firmware using fs_loader from file system

Pros:
a) No restriction on the firmware image sizes.
b) Need not re build u-boot for changing firmware
c) U-Boot build is un-touched for enabling various combination of use cases.

Cons:

1) Boot media dependent: fs_loader as of today supports loading firmware 
from the most common boot media and can be extended to any boot media.

2) Hard coding of remote core numbers to the firmware. This cannot be 
avoided any case with the current remoteproc framework in u-boot.

Rationale on choosing fs_loader approach:

Considering these options, and to scale the development process as well,
fs_loader approach has been preferred.

Regards,
Keerthy
> 
> Thanks,
> Keerthy
>>
>> Andrew
>>
>>
>>> Thanks,
>>> Keerthy
>>>>
>>>> Andrew
>>>>
>>>>
>>>>> - Keerthy
>>>>>>
>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> +        return 0;
>>>>>>>>> +    }
>>>>>>>>> +#endif
>>>>>>>>> +    if (!*loadaddr)
>>>>>>>>> +        return 0;
>>>>>>>>> +
>>>>>>>>> +    if (!uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0, 
>>>>>>>>> &fsdev)) {
>>>>>>>>> +        size = request_firmware_into_buf(fsdev, name, (void
>>>>>>>>> *)*loadaddr,
>>>>>>>>> +                         0, 0);
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    return size;
>>>>>>>>> +}
>>>>>>>>> +#else
>>>>>>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32
>>>>>>>>> *loadaddr)
>>>>>>>>> +{
>>>>>>>>> +    return 0;
>>>>>>>>> +}
>>>>>>>>> +#endif
>>>>>>>>> +
>>>>>>>>> +__weak void start_non_linux_remote_cores(void)
>>>>>>>>> +{
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>>      void __noreturn jump_to_image_no_args(struct spl_image_info
>>>>>>>>> *spl_image)
>>>>>>>>>      {
>>>>>>>>>          struct ti_sci_handle *ti_sci = get_ti_sci_handle();
>>>>>>>>> @@ -65,15 +137,17 @@ void __noreturn jump_to_image_no_args(struct
>>>>>>>>> spl_image_info *spl_image)
>>>>>>>>>          /* Release all the exclusive devices held by SPL before
>>>>>>>>> starting ATF */
>>>>>>>>>          ti_sci->ops.dev_ops.release_exclusive_devices(ti_sci);
>>>>>>>>>      +    ret = rproc_init();
>>>>>>>>> +    if (ret)
>>>>>>>>> +        panic("rproc failed to be initialized (%d)\n", ret);
>>>>>>>>> +
>>>>>>>>> +    init_env();
>>>>>>>>> +    start_non_linux_remote_cores();
>>>>>>>>> +
>>>>>>>>>          /*
>>>>>>>>>           * It is assumed that remoteproc device 1 is the
>>>>>>>>> corresponding
>>>>>>>>>           * Cortex-A core which runs ATF. Make sure DT reflects 
>>>>>>>>> the
>>>>>>>>> same.
>>>>>>>>>           */
>>>>>>>>> -    ret = rproc_dev_init(1);
>>>>>>>>> -    if (ret)
>>>>>>>>> -        panic("%s: ATF failed to initialize on rproc (%d)\n",
>>>>>>>>> __func__,
>>>>>>>>> -              ret);
>>>>>>>>> -
>>>>>>>>
>>>>>>>>
>>>>>>>> Where did this code go?
>>>>>>>
>>>>>>> rproc_init takes care of that.
>>>>>>>
>>>>>>
>>>>>>
>>>>>> Is that new behavior then? It should be it's own patch with a commit
>>>>>> message about that.
>>>>>>
>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>          ret = rproc_load(1, spl_image->entry_point, 0x200);
>>>>>>>>>          if (ret)
>>>>>>>>>              panic("%s: ATF failed to load on rproc (%d)\n",
>>>>>>>>> __func__,
>>>>>>>>> ret);
>>>>>>>>> diff --git a/arch/arm/mach-k3/common.h b/arch/arm/mach-k3/common.h
>>>>>>>>> index d8b34fe060..42fb8ee6e7 100644
>>>>>>>>> --- a/arch/arm/mach-k3/common.h
>>>>>>>>> +++ b/arch/arm/mach-k3/common.h
>>>>>>>>> @@ -24,3 +24,5 @@ void setup_k3_mpu_regions(void);
>>>>>>>>>      int early_console_init(void);
>>>>>>>>>      void disable_linefill_optimization(void);
>>>>>>>>>      void remove_fwl_configs(struct fwl_data *fwl_data, size_t
>>>>>>>>> fwl_data_size);
>>>>>>>>> +void start_non_linux_remote_cores(void);
>>>>>>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32
>>>>>>>>> *loadaddr);
>>>>>>>>> diff --git a/arch/arm/mach-k3/j721e_init.c
>>>>>>>>> b/arch/arm/mach-k3/j721e_init.c
>>>>>>>>> index f7f7398081..c5f8ede1a0 100644
>>>>>>>>> --- a/arch/arm/mach-k3/j721e_init.c
>>>>>>>>> +++ b/arch/arm/mach-k3/j721e_init.c
>>>>>>>>> @@ -18,6 +18,7 @@
>>>>>>>>>      #include <dm.h>
>>>>>>>>>      #include <dm/uclass-internal.h>
>>>>>>>>>      #include <dm/pinctrl.h>
>>>>>>>>> +#include <remoteproc.h>
>>>>>>>>>        #ifdef CONFIG_SPL_BUILD
>>>>>>>>>      #ifdef CONFIG_K3_LOAD_SYSFW
>>>>>>>>> @@ -295,3 +296,36 @@ void 
>>>>>>>>> release_resources_for_core_shutdown(void)
>>>>>>>>>          }
>>>>>>>>>      }
>>>>>>>>>      #endif
>>>>>>>>> +
>>>>>>>>> +#ifdef CONFIG_SYS_K3_SPL_ATF
>>>>>>>>> +void start_non_linux_remote_cores(void)
>>>>>>>>> +{
>>>>>>>>> +    int size = 0, ret;
>>>>>>>>> +    u32 loadaddr = 0;
>>>>>>>>> +
>>>>>>>>> +    size = load_firmware("mainr5f0_0fwname", 
>>>>>>>>> "mainr5f0_0loadaddr",
>>>>>>>>> +                 &loadaddr);
>>>>>>>>> +    if (size <= 0)
>>>>>>>>> +        goto err_load;
>>>>>>>>> +
>>>>>>>>> +    /*  remoteproc 2 is aliased for the needed remotecore */
>>>>>>>>
>>>>>>>>
>>>>>>>> Assuming the big-arm core to boot is remoteproc 1 was 
>>>>>>>> reasonable, but
>>>>>>>> there needs to be a better what than assuming the number for every
>>>>>>>> other
>>>>>>>> remote core.
>>>>>>>>
>>>>>>>>
>>>>>>>>> +    ret = rproc_load(2, loadaddr, size);
>>>>>>>>> +    if (ret) {
>>>>>>>>> +        printf("Firmware failed to start on rproc (%d)\n", ret);
>>>>>>>>> +        goto err_load;
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    ret = rproc_start(2);
>>>>>>>>> +    if (ret) {
>>>>>>>>> +        printf("Firmware init failed on rproc (%d)\n", ret);
>>>>>>>>> +        goto err_load;
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    printf("Remoteproc 2 started successfully\n");
>>>>>>>>
>>>>>>>>
>>>>>>>> That's useful..
>>>>>>>
>>>>>>> That is a print that tells everything went well with rproc 2.
>>>>>>> Otherwise
>>>>>>> one has to really find other ways to see if it succeeded or not.
>>>>>>>
>>>>>>
>>>>>>
>>>>>> I'm just a customer booting my board, I have no idea what a 
>>>>>> "Remoteproc
>>>>>> 2" is. I'm saying make the message describe what has happened.
>>>>>>
>>>>>> Andrew
>>>>>>
>>>>>>
>>>>>>>>
>>>>>>>> Andrew
>>>>>>>>
>>>>>>>>
>>>>>>>>> +
>>>>>>>>> +    return;
>>>>>>>>> +
>>>>>>>>> +err_load:
>>>>>>>>> +    rproc_reset(2);
>>>>>>>>> +}
>>>>>>>>> +#endif
>>>>>>>>>

[PATCH v3 02/10] arm: k3: Add support for loading non linux remote cores

[Andrew F. Davis]

On 1/24/20 3:42 AM, Tero Kristo wrote:
> On 23/01/2020 19:19, Keerthy wrote:
>>
>>
>> On 23/01/20 10:35 pm, Andrew F. Davis wrote:
>>> On 1/23/20 11:44 AM, Keerthy wrote:
>>>>
>>>>
>>>> On 23/01/20 6:54 pm, Andrew F. Davis wrote:
>>>>> On 1/22/20 11:10 PM, Keerthy wrote:
>>>>>>
>>>>>>
>>>>>> On 22/01/20 9:55 pm, Andrew F. Davis wrote:
>>>>>>> On 1/21/20 8:10 PM, keerthy wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 1/21/2020 6:26 PM, Andrew F. Davis wrote:
>>>>>>>>> On 1/21/20 6:07 AM, Keerthy wrote:
>>>>>>>>>> Add MAIN domain R5FSS0 remoteproc support from spl. This enables
>>>>>>>>>> loading the elf firmware in SPL and starting the remotecore.
>>>>>>>>>>
>>>>>>>>>> In order to start the core, there should be a file with path
>>>>>>>>>> "/lib/firmware/j7-main-r5f0_0-fw" under filesystem
>>>>>>>>>> of respective boot mode.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>>>>>>>>>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>>>>>>>>>> [Guard start_non_linux_remote_cores under CONFIG_FS_LOADER]
>>>>>>>>>> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
>>>>>>>>>> ---
>>>>>>>>>>      arch/arm/mach-k3/common.c     | 84
>>>>>>>>>> ++++++++++++++++++++++++++++++++---
>>>>>>>>>>      arch/arm/mach-k3/common.h     |  2 +
>>>>>>>>>>      arch/arm/mach-k3/j721e_init.c | 34 ++++++++++++++
>>>>>>>>>>      3 files changed, 115 insertions(+), 5 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/arch/arm/mach-k3/common.c
>>>>>>>>>> b/arch/arm/mach-k3/common.c
>>>>>>>>>> index 8d1529062d..f0ac0c39f1 100644
>>>>>>>>>> --- a/arch/arm/mach-k3/common.c
>>>>>>>>>> +++ b/arch/arm/mach-k3/common.c
>>>>>>>>>> @@ -16,6 +16,10 @@
>>>>>>>>>>      #include <asm/arch/sys_proto.h>
>>>>>>>>>>      #include <asm/hardware.h>
>>>>>>>>>>      #include <asm/io.h>
>>>>>>>>>> +#include <fs_loader.h>
>>>>>>>>>> +#include <fs.h>
>>>>>>>>>> +#include <env.h>
>>>>>>>>>> +#include <elf.h>
>>>>>>>>>>        struct ti_sci_handle *get_ti_sci_handle(void)
>>>>>>>>>>      {
>>>>>>>>>> @@ -57,6 +61,74 @@ int early_console_init(void)
>>>>>>>>>>      #endif
>>>>>>>>>>        #ifdef CONFIG_SYS_K3_SPL_ATF
>>>>>>>>>> +
>>>>>>>>>> +void init_env(void)
>>>>>>>>>> +{
>>>>>>>>>> +#ifdef CONFIG_SPL_ENV_SUPPORT
>>>>>>>>>> +    char *part;
>>>>>>>>>> +
>>>>>>>>>> +    env_init();
>>>>>>>>>> +    env_load();
>>>>>>>>>> +    switch (spl_boot_device()) {
>>>>>>>>>> +    case BOOT_DEVICE_MMC2:
>>>>>>>>>> +        part = env_get("bootpart");
>>>>>>>>>> +        env_set("storage_interface", "mmc");
>>>>>>>>>> +        env_set("fw_dev_part", part);
>>>>>>>>>> +        break;
>>>>>>>>>> +    case BOOT_DEVICE_SPI:
>>>>>>>>>> +        env_set("storage_interface", "ubi");
>>>>>>>>>> +        env_set("fw_ubi_mtdpart", "UBI");
>>>>>>>>>> +        env_set("fw_ubi_volume", "UBI0");
>>>>>>>>>> +        break;
>>>>>>>>>> +    default:
>>>>>>>>>> +        printf("%s from device %u not supported!\n",
>>>>>>>>>> +               __func__, spl_boot_device());
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This will print for almost every boot mode..
>>>>>>>>
>>>>>>>> I can keep this under debug.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> +        return;
>>>>>>>>>> +    }
>>>>>>>>>> +#endif
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +#ifdef CONFIG_FS_LOADER
>>>>>>>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32
>>>>>>>>>> *loadaddr)
>>>>>>>>>> +{
>>>>>>>>>> +    struct udevice *fsdev;
>>>>>>>>>> +    char *name = NULL;
>>>>>>>>>> +    int size = 0;
>>>>>>>>>> +
>>>>>>>>>> +    *loadaddr = 0;
>>>>>>>>>> +#ifdef CONFIG_SPL_ENV_SUPPORT
>>>>>>>>>> +    switch (spl_boot_device()) {
>>>>>>>>>> +    case BOOT_DEVICE_MMC2:
>>>>>>>>>> +        name = env_get(name_fw);
>>>>>>>>>> +        *loadaddr = env_get_hex(name_loadaddr, *loadaddr);
>>>>>>>>>> +        break;
>>>>>>>>>> +    default:
>>>>>>>>>> +        printf("Loading rproc fw image from device %u not
>>>>>>>>>> supported!\n",
>>>>>>>>>> +               spl_boot_device());
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This whole thing seems very MMC specific, if early firmware
>>>>>>>>> loading is
>>>>>>>>> important it should work for all boot modes. Find a way to include
>>>>>>>>> it in
>>>>>>>>> the next boot stage FIT image (tispl.bin) so it works for all
>>>>>>>>> modes.
>>>>>>>>
>>>>>>>> That was not NAKd. We are going with fs_loader approach.
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> When, where, link?
>>>>>>
>>>>>> I had implemented that way internally. That was rejected for multiple
>>>>>> right reasons:
>>>>>>
>>>>>
>>>>>
>>>>> I must have missed the internal reviews for this, anyway this is
>>>>> posted
>>>>> upstream so lets discus it here.
>>>>>
>>>>>
>>>>>> 1) SPL size would bloat based on the size of the firmware.
>>>>>
>>>>>
>>>>> SPL size would remain constant, the combined FIT (tispl.bin) would
>>>>> grow,
>>>>> but that is okay as DRAM is enabled at this point so we have no hard
>>>>> memory constraints.
>>>>
>>>> I meant the FIT image containing the SPL will bloat.
>>>
>>>
>>> Exactly what I said, and so size is not a huge deal.
>>>
>>>
>>>>
>>>>>
>>>>>
>>>>>> 2) There are multiple cores that need to be loaded and hence
>>>>>> adding all
>>>>>> the firmwares under a fit can be really painful.
>>>>>
>>>>>
>>>>> Bundling images is what FIT is for, are you saying the better solution
>>>>> is to hard-code each firmware starting like done here?
>>>>
>>>> How many firmwares will you go on bundling. Firmwares are already kept
>>>> in file system. It is a matter of reading them from there.
>>>>
>>>
>>>
>>> If we are early booting them from SPL then they don't really need to be
>>> on the filesystem.
>>>
>>>
>>>>>
>>>>>
>>>>>> 3) Changing firmware means building the tispl.bin again.
>>>>>>
>>>>>
>>>>>
>>>>> FIT images can be disassembled and reassembled with a script around
>>>>> tools/dumpimage.
>>>>
>>>> And you expect everyone to master that instead of looking at couple of
>>>> aliases in DT to figure out which core corresponds to which ID?
>>>>
>>>
>>>
>>> Your patches do more than add DT aliases to add a firmware image. I
>>> think you are responding to the wrong comment here, the ID part is
>>> below.
>>>
>>>
>>>>>
>>>>> SPL should be simple and load the one next stage.
>>>>>
>>>>>
>>>>>> The FIT solution can not scale well.
>>>>>>
>>>>>
>>>>>
>>>>> How does this current series scale at all? At least with FIT you
>>>>> can add
>>>>> more images without adding code for
>>>>> request_firmware(<hard-coded-firmware-name>) and
>>>>> rproc_load(<hard-coded-number>). That all could be encoded in the FIT
>>>>> data.
>>>>
>>>> I understand and as explained earlier i have even implemented that once
>>>> before. fs_loader was meant to address the exact use case we are
>>>> discussing about.
>>>>
>>>> Even in u-boot remotecores are started/loaded by indices. Users need to
>>>> know them. This is no different than that.
>>>>
>>>> I am not convinced about FIT approach. I would let Lokesh take a
>>>> call on
>>>> this.
>>>>
>>>
>>>
>>> Lokesh is not the whole U-Boot community, I get you two aligned
>>> internally on this, but I'd be much more interested in Tom or Simon's
>>> opinion here. I was doing the same thing when loading PMMC firmware for
>>> HS and was pushed to make a FIT friendly version, I'm glad that I did,
>>> it ended up much less hacky in the long run.
>>
>> I would love to hear Tom & simon's opinions. Before we jump on to PMMC
>> fimrware example. I believe there is one such firmware that gets
>> appended to the FIT image. In case of remoteprocs there can be
>> multiple such firmware. File system is the most generic and scaleble
>> place to access firmware.
>>
>> Lokesh/Sekhar/Tero,
>>
>> You can share your viewpoints as well.
> 
> I think the initial reason to split the firmwares away from the FIT was
> that we didn't want to have ROM load everything for us, and so that SPL
> would come up earlier. Couple of reasons behind that, the size of the
> FIT image directly transforms into time spent loading it, and also ROM
> might not be loading things at optimal speed... There are couple of
> critical firmwares that need to be booted up as soon as possible.
> 


The firmware is not going into the ROM loaded SPL (tiboot3.bin) I'm
saying the A53/A72 SPL (tispl.bin) image (load done by R5 SPL). The R5
SPL is loading the firmware in either case so no time penalty based on
how the firmware is packaged.

Andrew


> -Tero
> 
>>
>> Thanks,
>> Keerthy
>>>
>>> Andrew
>>>
>>>
>>>> Thanks,
>>>> Keerthy
>>>>>
>>>>> Andrew
>>>>>
>>>>>
>>>>>> - Keerthy
>>>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> +        return 0;
>>>>>>>>>> +    }
>>>>>>>>>> +#endif
>>>>>>>>>> +    if (!*loadaddr)
>>>>>>>>>> +        return 0;
>>>>>>>>>> +
>>>>>>>>>> +    if (!uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0,
>>>>>>>>>> &fsdev)) {
>>>>>>>>>> +        size = request_firmware_into_buf(fsdev, name, (void
>>>>>>>>>> *)*loadaddr,
>>>>>>>>>> +                         0, 0);
>>>>>>>>>> +    }
>>>>>>>>>> +
>>>>>>>>>> +    return size;
>>>>>>>>>> +}
>>>>>>>>>> +#else
>>>>>>>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32
>>>>>>>>>> *loadaddr)
>>>>>>>>>> +{
>>>>>>>>>> +    return 0;
>>>>>>>>>> +}
>>>>>>>>>> +#endif
>>>>>>>>>> +
>>>>>>>>>> +__weak void start_non_linux_remote_cores(void)
>>>>>>>>>> +{
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>>      void __noreturn jump_to_image_no_args(struct spl_image_info
>>>>>>>>>> *spl_image)
>>>>>>>>>>      {
>>>>>>>>>>          struct ti_sci_handle *ti_sci = get_ti_sci_handle();
>>>>>>>>>> @@ -65,15 +137,17 @@ void __noreturn jump_to_image_no_args(struct
>>>>>>>>>> spl_image_info *spl_image)
>>>>>>>>>>          /* Release all the exclusive devices held by SPL before
>>>>>>>>>> starting ATF */
>>>>>>>>>>          ti_sci->ops.dev_ops.release_exclusive_devices(ti_sci);
>>>>>>>>>>      +    ret = rproc_init();
>>>>>>>>>> +    if (ret)
>>>>>>>>>> +        panic("rproc failed to be initialized (%d)\n", ret);
>>>>>>>>>> +
>>>>>>>>>> +    init_env();
>>>>>>>>>> +    start_non_linux_remote_cores();
>>>>>>>>>> +
>>>>>>>>>>          /*
>>>>>>>>>>           * It is assumed that remoteproc device 1 is the
>>>>>>>>>> corresponding
>>>>>>>>>>           * Cortex-A core which runs ATF. Make sure DT
>>>>>>>>>> reflects the
>>>>>>>>>> same.
>>>>>>>>>>           */
>>>>>>>>>> -    ret = rproc_dev_init(1);
>>>>>>>>>> -    if (ret)
>>>>>>>>>> -        panic("%s: ATF failed to initialize on rproc (%d)\n",
>>>>>>>>>> __func__,
>>>>>>>>>> -              ret);
>>>>>>>>>> -
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Where did this code go?
>>>>>>>>
>>>>>>>> rproc_init takes care of that.
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Is that new behavior then? It should be it's own patch with a commit
>>>>>>> message about that.
>>>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>          ret = rproc_load(1, spl_image->entry_point, 0x200);
>>>>>>>>>>          if (ret)
>>>>>>>>>>              panic("%s: ATF failed to load on rproc (%d)\n",
>>>>>>>>>> __func__,
>>>>>>>>>> ret);
>>>>>>>>>> diff --git a/arch/arm/mach-k3/common.h
>>>>>>>>>> b/arch/arm/mach-k3/common.h
>>>>>>>>>> index d8b34fe060..42fb8ee6e7 100644
>>>>>>>>>> --- a/arch/arm/mach-k3/common.h
>>>>>>>>>> +++ b/arch/arm/mach-k3/common.h
>>>>>>>>>> @@ -24,3 +24,5 @@ void setup_k3_mpu_regions(void);
>>>>>>>>>>      int early_console_init(void);
>>>>>>>>>>      void disable_linefill_optimization(void);
>>>>>>>>>>      void remove_fwl_configs(struct fwl_data *fwl_data, size_t
>>>>>>>>>> fwl_data_size);
>>>>>>>>>> +void start_non_linux_remote_cores(void);
>>>>>>>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32
>>>>>>>>>> *loadaddr);
>>>>>>>>>> diff --git a/arch/arm/mach-k3/j721e_init.c
>>>>>>>>>> b/arch/arm/mach-k3/j721e_init.c
>>>>>>>>>> index f7f7398081..c5f8ede1a0 100644
>>>>>>>>>> --- a/arch/arm/mach-k3/j721e_init.c
>>>>>>>>>> +++ b/arch/arm/mach-k3/j721e_init.c
>>>>>>>>>> @@ -18,6 +18,7 @@
>>>>>>>>>>      #include <dm.h>
>>>>>>>>>>      #include <dm/uclass-internal.h>
>>>>>>>>>>      #include <dm/pinctrl.h>
>>>>>>>>>> +#include <remoteproc.h>
>>>>>>>>>>        #ifdef CONFIG_SPL_BUILD
>>>>>>>>>>      #ifdef CONFIG_K3_LOAD_SYSFW
>>>>>>>>>> @@ -295,3 +296,36 @@ void
>>>>>>>>>> release_resources_for_core_shutdown(void)
>>>>>>>>>>          }
>>>>>>>>>>      }
>>>>>>>>>>      #endif
>>>>>>>>>> +
>>>>>>>>>> +#ifdef CONFIG_SYS_K3_SPL_ATF
>>>>>>>>>> +void start_non_linux_remote_cores(void)
>>>>>>>>>> +{
>>>>>>>>>> +    int size = 0, ret;
>>>>>>>>>> +    u32 loadaddr = 0;
>>>>>>>>>> +
>>>>>>>>>> +    size = load_firmware("mainr5f0_0fwname",
>>>>>>>>>> "mainr5f0_0loadaddr",
>>>>>>>>>> +                 &loadaddr);
>>>>>>>>>> +    if (size <= 0)
>>>>>>>>>> +        goto err_load;
>>>>>>>>>> +
>>>>>>>>>> +    /*  remoteproc 2 is aliased for the needed remotecore */
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Assuming the big-arm core to boot is remoteproc 1 was
>>>>>>>>> reasonable, but
>>>>>>>>> there needs to be a better what than assuming the number for every
>>>>>>>>> other
>>>>>>>>> remote core.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> +    ret = rproc_load(2, loadaddr, size);
>>>>>>>>>> +    if (ret) {
>>>>>>>>>> +        printf("Firmware failed to start on rproc (%d)\n", ret);
>>>>>>>>>> +        goto err_load;
>>>>>>>>>> +    }
>>>>>>>>>> +
>>>>>>>>>> +    ret = rproc_start(2);
>>>>>>>>>> +    if (ret) {
>>>>>>>>>> +        printf("Firmware init failed on rproc (%d)\n", ret);
>>>>>>>>>> +        goto err_load;
>>>>>>>>>> +    }
>>>>>>>>>> +
>>>>>>>>>> +    printf("Remoteproc 2 started successfully\n");
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> That's useful..
>>>>>>>>
>>>>>>>> That is a print that tells everything went well with rproc 2.
>>>>>>>> Otherwise
>>>>>>>> one has to really find other ways to see if it succeeded or not.
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> I'm just a customer booting my board, I have no idea what a
>>>>>>> "Remoteproc
>>>>>>> 2" is. I'm saying make the message describe what has happened.
>>>>>>>
>>>>>>> Andrew
>>>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>> Andrew
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +    return;
>>>>>>>>>> +
>>>>>>>>>> +err_load:
>>>>>>>>>> +    rproc_reset(2);
>>>>>>>>>> +}
>>>>>>>>>> +#endif
>>>>>>>>>>
> 
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH v3 02/10] arm: k3: Add support for loading non linux remote cores

[Lokesh Vutla]

On 24/01/20 5:33 PM, Keerthy wrote:
> 
> 
> On 23/01/20 10:49 pm, Keerthy wrote:
>>
>>
>> On 23/01/20 10:35 pm, Andrew F. Davis wrote:
>>> On 1/23/20 11:44 AM, Keerthy wrote:
>>>>
>>>>
>>>> On 23/01/20 6:54 pm, Andrew F. Davis wrote:
>>>>> On 1/22/20 11:10 PM, Keerthy wrote:
>>>>>>
>>>>>>
>>>>>> On 22/01/20 9:55 pm, Andrew F. Davis wrote:
>>>>>>> On 1/21/20 8:10 PM, keerthy wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 1/21/2020 6:26 PM, Andrew F. Davis wrote:
>>>>>>>>> On 1/21/20 6:07 AM, Keerthy wrote:
>>>>>>>>>> Add MAIN domain R5FSS0 remoteproc support from spl. This enables
>>>>>>>>>> loading the elf firmware in SPL and starting the remotecore.
>>>>>>>>>>
>>>>>>>>>> In order to start the core, there should be a file with path
>>>>>>>>>> "/lib/firmware/j7-main-r5f0_0-fw" under filesystem
>>>>>>>>>> of respective boot mode.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>>>>>>>>>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>>>>>>>>>> [Guard start_non_linux_remote_cores under CONFIG_FS_LOADER]
>>>>>>>>>> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
>>>>>>>>>> ---
>>>>>>>>>>      arch/arm/mach-k3/common.c     | 84
>>>>>>>>>> ++++++++++++++++++++++++++++++++---
>>>>>>>>>>      arch/arm/mach-k3/common.h     |  2 +
>>>>>>>>>>      arch/arm/mach-k3/j721e_init.c | 34 ++++++++++++++
>>>>>>>>>>      3 files changed, 115 insertions(+), 5 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
>>>>>>>>>> index 8d1529062d..f0ac0c39f1 100644
>>>>>>>>>> --- a/arch/arm/mach-k3/common.c
>>>>>>>>>> +++ b/arch/arm/mach-k3/common.c
>>>>>>>>>> @@ -16,6 +16,10 @@
>>>>>>>>>>      #include <asm/arch/sys_proto.h>
>>>>>>>>>>      #include <asm/hardware.h>
>>>>>>>>>>      #include <asm/io.h>
>>>>>>>>>> +#include <fs_loader.h>
>>>>>>>>>> +#include <fs.h>
>>>>>>>>>> +#include <env.h>
>>>>>>>>>> +#include <elf.h>
>>>>>>>>>>        struct ti_sci_handle *get_ti_sci_handle(void)
>>>>>>>>>>      {
>>>>>>>>>> @@ -57,6 +61,74 @@ int early_console_init(void)
>>>>>>>>>>      #endif
>>>>>>>>>>        #ifdef CONFIG_SYS_K3_SPL_ATF
>>>>>>>>>> +
>>>>>>>>>> +void init_env(void)
>>>>>>>>>> +{
>>>>>>>>>> +#ifdef CONFIG_SPL_ENV_SUPPORT
>>>>>>>>>> +    char *part;
>>>>>>>>>> +
>>>>>>>>>> +    env_init();
>>>>>>>>>> +    env_load();
>>>>>>>>>> +    switch (spl_boot_device()) {
>>>>>>>>>> +    case BOOT_DEVICE_MMC2:
>>>>>>>>>> +        part = env_get("bootpart");
>>>>>>>>>> +        env_set("storage_interface", "mmc");
>>>>>>>>>> +        env_set("fw_dev_part", part);
>>>>>>>>>> +        break;
>>>>>>>>>> +    case BOOT_DEVICE_SPI:
>>>>>>>>>> +        env_set("storage_interface", "ubi");
>>>>>>>>>> +        env_set("fw_ubi_mtdpart", "UBI");
>>>>>>>>>> +        env_set("fw_ubi_volume", "UBI0");
>>>>>>>>>> +        break;
>>>>>>>>>> +    default:
>>>>>>>>>> +        printf("%s from device %u not supported!\n",
>>>>>>>>>> +               __func__, spl_boot_device());
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This will print for almost every boot mode..
>>>>>>>>
>>>>>>>> I can keep this under debug.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> +        return;
>>>>>>>>>> +    }
>>>>>>>>>> +#endif
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +#ifdef CONFIG_FS_LOADER
>>>>>>>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32
>>>>>>>>>> *loadaddr)
>>>>>>>>>> +{
>>>>>>>>>> +    struct udevice *fsdev;
>>>>>>>>>> +    char *name = NULL;
>>>>>>>>>> +    int size = 0;
>>>>>>>>>> +
>>>>>>>>>> +    *loadaddr = 0;
>>>>>>>>>> +#ifdef CONFIG_SPL_ENV_SUPPORT
>>>>>>>>>> +    switch (spl_boot_device()) {
>>>>>>>>>> +    case BOOT_DEVICE_MMC2:
>>>>>>>>>> +        name = env_get(name_fw);
>>>>>>>>>> +        *loadaddr = env_get_hex(name_loadaddr, *loadaddr);
>>>>>>>>>> +        break;
>>>>>>>>>> +    default:
>>>>>>>>>> +        printf("Loading rproc fw image from device %u not
>>>>>>>>>> supported!\n",
>>>>>>>>>> +               spl_boot_device());
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This whole thing seems very MMC specific, if early firmware
>>>>>>>>> loading is
>>>>>>>>> important it should work for all boot modes. Find a way to include
>>>>>>>>> it in
>>>>>>>>> the next boot stage FIT image (tispl.bin) so it works for all modes.
>>>>>>>>
>>>>>>>> That was not NAKd. We are going with fs_loader approach.
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> When, where, link?
>>>>>>
>>>>>> I had implemented that way internally. That was rejected for multiple
>>>>>> right reasons:
>>>>>>
>>>>>
>>>>>
>>>>> I must have missed the internal reviews for this, anyway this is posted
>>>>> upstream so lets discus it here.
>>>>>
>>>>>
>>>>>> 1) SPL size would bloat based on the size of the firmware.
>>>>>
>>>>>
>>>>> SPL size would remain constant, the combined FIT (tispl.bin) would grow,
>>>>> but that is okay as DRAM is enabled at this point so we have no hard
>>>>> memory constraints.
>>>>
>>>> I meant the FIT image containing the SPL will bloat.
>>>
>>>
>>> Exactly what I said, and so size is not a huge deal.
>>>
>>>
>>>>
>>>>>
>>>>>
>>>>>> 2) There are multiple cores that need to be loaded and hence adding all
>>>>>> the firmwares under a fit can be really painful.
>>>>>
>>>>>
>>>>> Bundling images is what FIT is for, are you saying the better solution
>>>>> is to hard-code each firmware starting like done here?
>>>>
>>>> How many firmwares will you go on bundling. Firmwares are already kept
>>>> in file system. It is a matter of reading them from there.
>>>>
>>>
>>>
>>> If we are early booting them from SPL then they don't really need to be
>>> on the filesystem.
>>>
>>>
>>>>>
>>>>>
>>>>>> 3) Changing firmware means building the tispl.bin again.
>>>>>>
>>>>>
>>>>>
>>>>> FIT images can be disassembled and reassembled with a script around
>>>>> tools/dumpimage.
>>>>
>>>> And you expect everyone to master that instead of looking at couple of
>>>> aliases in DT to figure out which core corresponds to which ID?
>>>>
>>>
>>>
>>> Your patches do more than add DT aliases to add a firmware image. I
>>> think you are responding to the wrong comment here, the ID part is below.
>>>
>>>
>>>>>
>>>>> SPL should be simple and load the one next stage.
>>>>>
>>>>>
>>>>>> The FIT solution can not scale well.
>>>>>>
>>>>>
>>>>>
>>>>> How does this current series scale at all? At least with FIT you can add
>>>>> more images without adding code for
>>>>> request_firmware(<hard-coded-firmware-name>) and
>>>>> rproc_load(<hard-coded-number>). That all could be encoded in the FIT
>>>>> data.
>>>>
>>>> I understand and as explained earlier i have even implemented that once
>>>> before. fs_loader was meant to address the exact use case we are
>>>> discussing about.
>>>>
>>>> Even in u-boot remotecores are started/loaded by indices. Users need to
>>>> know them. This is no different than that.
>>>>
>>>> I am not convinced about FIT approach. I would let Lokesh take a call on
>>>> this.
>>>>
>>>
>>>
>>> Lokesh is not the whole U-Boot community, I get you two aligned
>>> internally on this, but I'd be much more interested in Tom or Simon's
>>> opinion here. I was doing the same thing when loading PMMC firmware for
>>> HS and was pushed to make a FIT friendly version, I'm glad that I did,
>>> it ended up much less hacky in the long run.
>>
>> I would love to hear Tom & simon's opinions. Before we jump on to PMMC
>> fimrware example. I believe there is one such firmware that gets appended to
>> the FIT image. In case of remoteprocs there can be multiple such firmware.
>> File system is the most generic and scaleble place to access firmware.
>>
> 
> Andrew,
> 
> PMMC in K2G is a case where the firmware image is targeted for a specific core
> and in K2G U-Boot only takes care of loading that particular core. Where as this
> series targets multiple cores which can cater to multiple use cases resulting in
> different combinations. To give more details, below are the approaches
> evaluated(prototyped):
> 
> Approach 1: Pack all the firmwares into a fit Image along with A72 SPL
> ----------------------------------------------------------------------
> Pros:
> -----
> a) Boot media independent: No need to load firmware via file
> system present in different boot media.
> 
> Cons:
> -----
> a) Flash image: In case of raw flash devices. We need to upfront
> predict the partition that needs to be allocated for tispl.bin.
> Complex SoCs like J721e has ~9 remote cores and each firmware can be ~2-3MB
> resulting in ~20-30MB of tispl.bin. In case one has to add
> a bigger firmware that would mean flash partition resizing. Also not all boards
> has flashes > 32MB.
> 
> In fact a prototype of this approach was tried. It failed as soon as
> a larger firmware was included in the FIT image as the memory allocated
> was not sufficient.
> 
> b) Dependency on U-Boot repo for changing firmwares: The firmwares need to be
> built as part of the FIT Image that contains the SPL. Firmware change means
> repacking the FIT image. Any customer/developer who intends to change the remote
> core firmware needs to be using the u-boot repository to pack the image into the
> FIT Image.
> 
> c) Mapping the firmware to remote cores: J7 has around 9 remote
> cores(Mostly R5Fs & DSPs). Each firmware is position dependent.
> There is no good way to map which firmware
> corresponds to which remote core other than hard coding.
> 
> d) Use case support: Different use cases result in different combination of
> remote cores. Every different combination will result in a new binary.
> 
> e) Example build command would be:
> make ARCH=arm R50=<> R51=<> R52=<>.........C7x=<> C6x0=<>
> 
> You can see it :)
> 
> Approach 2: Loading the firmware using fs_loader from file system
> 
> Pros:
> a) No restriction on the firmware image sizes.
> b) Need not re build u-boot for changing firmware
> c) U-Boot build is un-touched for enabling various combination of use cases.
> 
> Cons:
> 
> 1) Boot media dependent: fs_loader as of today supports loading firmware from
> the most common boot media and can be extended to any boot media.
> 
> 2) Hard coding of remote core numbers to the firmware. This cannot be avoided
> any case with the current remoteproc framework in u-boot.
> 
> Rationale on choosing fs_loader approach:
> 
> Considering these options, and to scale the development process as well,
> fs_loader approach has been preferred.
> 


Its been more than a week and there are no responses for the above
justification. I assume there are no objections for the current approach.

Thanks and regards,
Lokesh


> Regards,
> Keerthy
>>
>> Thanks,
>> Keerthy
>>>
>>> Andrew
>>>
>>>
>>>> Thanks,
>>>> Keerthy
>>>>>
>>>>> Andrew
>>>>>
>>>>>
>>>>>> - Keerthy
>>>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> +        return 0;
>>>>>>>>>> +    }
>>>>>>>>>> +#endif
>>>>>>>>>> +    if (!*loadaddr)
>>>>>>>>>> +        return 0;
>>>>>>>>>> +
>>>>>>>>>> +    if (!uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0, &fsdev)) {
>>>>>>>>>> +        size = request_firmware_into_buf(fsdev, name, (void
>>>>>>>>>> *)*loadaddr,
>>>>>>>>>> +                         0, 0);
>>>>>>>>>> +    }
>>>>>>>>>> +
>>>>>>>>>> +    return size;
>>>>>>>>>> +}
>>>>>>>>>> +#else
>>>>>>>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32
>>>>>>>>>> *loadaddr)
>>>>>>>>>> +{
>>>>>>>>>> +    return 0;
>>>>>>>>>> +}
>>>>>>>>>> +#endif
>>>>>>>>>> +
>>>>>>>>>> +__weak void start_non_linux_remote_cores(void)
>>>>>>>>>> +{
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>>      void __noreturn jump_to_image_no_args(struct spl_image_info
>>>>>>>>>> *spl_image)
>>>>>>>>>>      {
>>>>>>>>>>          struct ti_sci_handle *ti_sci = get_ti_sci_handle();
>>>>>>>>>> @@ -65,15 +137,17 @@ void __noreturn jump_to_image_no_args(struct
>>>>>>>>>> spl_image_info *spl_image)
>>>>>>>>>>          /* Release all the exclusive devices held by SPL before
>>>>>>>>>> starting ATF */
>>>>>>>>>>          ti_sci->ops.dev_ops.release_exclusive_devices(ti_sci);
>>>>>>>>>>      +    ret = rproc_init();
>>>>>>>>>> +    if (ret)
>>>>>>>>>> +        panic("rproc failed to be initialized (%d)\n", ret);
>>>>>>>>>> +
>>>>>>>>>> +    init_env();
>>>>>>>>>> +    start_non_linux_remote_cores();
>>>>>>>>>> +
>>>>>>>>>>          /*
>>>>>>>>>>           * It is assumed that remoteproc device 1 is the
>>>>>>>>>> corresponding
>>>>>>>>>>           * Cortex-A core which runs ATF. Make sure DT reflects the
>>>>>>>>>> same.
>>>>>>>>>>           */
>>>>>>>>>> -    ret = rproc_dev_init(1);
>>>>>>>>>> -    if (ret)
>>>>>>>>>> -        panic("%s: ATF failed to initialize on rproc (%d)\n",
>>>>>>>>>> __func__,
>>>>>>>>>> -              ret);
>>>>>>>>>> -
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Where did this code go?
>>>>>>>>
>>>>>>>> rproc_init takes care of that.
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Is that new behavior then? It should be it's own patch with a commit
>>>>>>> message about that.
>>>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>          ret = rproc_load(1, spl_image->entry_point, 0x200);
>>>>>>>>>>          if (ret)
>>>>>>>>>>              panic("%s: ATF failed to load on rproc (%d)\n",
>>>>>>>>>> __func__,
>>>>>>>>>> ret);
>>>>>>>>>> diff --git a/arch/arm/mach-k3/common.h b/arch/arm/mach-k3/common.h
>>>>>>>>>> index d8b34fe060..42fb8ee6e7 100644
>>>>>>>>>> --- a/arch/arm/mach-k3/common.h
>>>>>>>>>> +++ b/arch/arm/mach-k3/common.h
>>>>>>>>>> @@ -24,3 +24,5 @@ void setup_k3_mpu_regions(void);
>>>>>>>>>>      int early_console_init(void);
>>>>>>>>>>      void disable_linefill_optimization(void);
>>>>>>>>>>      void remove_fwl_configs(struct fwl_data *fwl_data, size_t
>>>>>>>>>> fwl_data_size);
>>>>>>>>>> +void start_non_linux_remote_cores(void);
>>>>>>>>>> +int load_firmware(char *name_fw, char *name_loadaddr, u32
>>>>>>>>>> *loadaddr);
>>>>>>>>>> diff --git a/arch/arm/mach-k3/j721e_init.c
>>>>>>>>>> b/arch/arm/mach-k3/j721e_init.c
>>>>>>>>>> index f7f7398081..c5f8ede1a0 100644
>>>>>>>>>> --- a/arch/arm/mach-k3/j721e_init.c
>>>>>>>>>> +++ b/arch/arm/mach-k3/j721e_init.c
>>>>>>>>>> @@ -18,6 +18,7 @@
>>>>>>>>>>      #include <dm.h>
>>>>>>>>>>      #include <dm/uclass-internal.h>
>>>>>>>>>>      #include <dm/pinctrl.h>
>>>>>>>>>> +#include <remoteproc.h>
>>>>>>>>>>        #ifdef CONFIG_SPL_BUILD
>>>>>>>>>>      #ifdef CONFIG_K3_LOAD_SYSFW
>>>>>>>>>> @@ -295,3 +296,36 @@ void release_resources_for_core_shutdown(void)
>>>>>>>>>>          }
>>>>>>>>>>      }
>>>>>>>>>>      #endif
>>>>>>>>>> +
>>>>>>>>>> +#ifdef CONFIG_SYS_K3_SPL_ATF
>>>>>>>>>> +void start_non_linux_remote_cores(void)
>>>>>>>>>> +{
>>>>>>>>>> +    int size = 0, ret;
>>>>>>>>>> +    u32 loadaddr = 0;
>>>>>>>>>> +
>>>>>>>>>> +    size = load_firmware("mainr5f0_0fwname", "mainr5f0_0loadaddr",
>>>>>>>>>> +                 &loadaddr);
>>>>>>>>>> +    if (size <= 0)
>>>>>>>>>> +        goto err_load;
>>>>>>>>>> +
>>>>>>>>>> +    /*  remoteproc 2 is aliased for the needed remotecore */
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Assuming the big-arm core to boot is remoteproc 1 was reasonable, but
>>>>>>>>> there needs to be a better what than assuming the number for every
>>>>>>>>> other
>>>>>>>>> remote core.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> +    ret = rproc_load(2, loadaddr, size);
>>>>>>>>>> +    if (ret) {
>>>>>>>>>> +        printf("Firmware failed to start on rproc (%d)\n", ret);
>>>>>>>>>> +        goto err_load;
>>>>>>>>>> +    }
>>>>>>>>>> +
>>>>>>>>>> +    ret = rproc_start(2);
>>>>>>>>>> +    if (ret) {
>>>>>>>>>> +        printf("Firmware init failed on rproc (%d)\n", ret);
>>>>>>>>>> +        goto err_load;
>>>>>>>>>> +    }
>>>>>>>>>> +
>>>>>>>>>> +    printf("Remoteproc 2 started successfully\n");
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> That's useful..
>>>>>>>>
>>>>>>>> That is a print that tells everything went well with rproc 2.
>>>>>>>> Otherwise
>>>>>>>> one has to really find other ways to see if it succeeded or not.
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> I'm just a customer booting my board, I have no idea what a "Remoteproc
>>>>>>> 2" is. I'm saying make the message describe what has happened.
>>>>>>>
>>>>>>> Andrew
>>>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>> Andrew
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +    return;
>>>>>>>>>> +
>>>>>>>>>> +err_load:
>>>>>>>>>> +    rproc_reset(2);
>>>>>>>>>> +}
>>>>>>>>>> +#endif
>>>>>>>>>>