[RFC 0/6] remoteproc: custom fw handling

[RFC 0/6] remoteproc: custom fw handling

[sjur.brandeland]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

Hi Ohad.

I have cooked a couple of patches for remoteproc. I was planning to
test and polish them some more, but figured that I might as well send
them and find out what you think.

First patch is a trivial fix for print formatting warnings (gcc 4.6.1)

The next five patches are about custom firmware handlers.
I have done this in small steps to facilitate review, but I
suspect you might want to squash them all together.

The three first patches are just preparation. The fourth patch
is the most interesting one, doing firmware loaders customizable.
The last is optional, it makes the ELF loader into a separate module.

Note that the ELF loaders are not tested. I have no environment to test
this at the moment.

Regards,
Sjur


Sjur Brændeland (6):
  remoteproc: fix print format warnings
  remoteproc: Pass struct fw to load_segments and find_rsc_table.
  remoteproc: Move fw sanity check to find_rsc_table.
  remoteproc: Move Elf related functions to separate file
  remoteproc: Support custom firmware handlers
  remoteproc: Make REMOTEPROC_ELF a sparate Kconfig

 drivers/remoteproc/Kconfig                 |    6 +
 drivers/remoteproc/Makefile                |    1 +
 drivers/remoteproc/omap_remoteproc.c       |    1 +
 drivers/remoteproc/remoteproc_core.c       |  278 +++------------------------
 drivers/remoteproc/remoteproc_elf_loader.c |  282 ++++++++++++++++++++++++++++
 include/linux/remoteproc.h                 |   21 +++
 6 files changed, 334 insertions(+), 255 deletions(-)
 create mode 100644 drivers/remoteproc/remoteproc_elf_loader.c

-- 
1.7.9.5

[RFC 1/6] remoteproc: fix print format warnings

[sjur.brandeland]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

Fix compile warnings when printing values of type size_t

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/remoteproc/remoteproc_core.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index ee15c68..1bba479 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -247,7 +247,7 @@ rproc_load_segments(struct rproc *rproc, const u8 *elf_data, size_t len)
 		}
 
 		if (offset + filesz > len) {
-			dev_err(dev, "truncated fw: need 0x%x avail 0x%x\n",
+			dev_err(dev, "truncated fw: need 0x%x avail 0x%zx\n",
 					offset + filesz, len);
 			ret = -EINVAL;
 			break;
@@ -934,7 +934,7 @@ static void rproc_resource_cleanup(struct rproc *rproc)
 		unmapped = iommu_unmap(rproc->domain, entry->da, entry->len);
 		if (unmapped != entry->len) {
 			/* nothing much to do besides complaining */
-			dev_err(dev, "failed to unmap %u/%u\n", entry->len,
+			dev_err(dev, "failed to unmap %u/%zu\n", entry->len,
 								unmapped);
 		}
 
@@ -1020,7 +1020,7 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 
 	ehdr = (struct elf32_hdr *)fw->data;
 
-	dev_info(dev, "Booting fw image %s, size %d\n", name, fw->size);
+	dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
 
 	/*
 	 * if enabling an IOMMU isn't relevant for this rproc, this is
-- 
1.7.9.5

[RFC 2/6] remoteproc: Pass struct fw to load_segments and find_rsc_table.

[sjur.brandeland]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

Use struct firmware as arguments to functions rproc_find_rcs_table()
and rproc_load_segments().

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/remoteproc/remoteproc_core.c |   38 +++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 1bba479..3d980ea 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -192,8 +192,7 @@ static void *rproc_da_to_va(struct rproc *rproc, u64 da, int len)
 /**
  * rproc_load_segments() - load firmware segments to memory
  * @rproc: remote processor which will be booted using these fw segments
- * @elf_data: the content of the ELF firmware image
- * @len: firmware size (in bytes)
+ * @fw: the  the ELF firmware image
  *
  * This function loads the firmware segments to memory, where the remote
  * processor expects them.
@@ -215,12 +214,13 @@ static void *rproc_da_to_va(struct rproc *rproc, u64 da, int len)
  * supported, though.
  */
 static int
-rproc_load_segments(struct rproc *rproc, const u8 *elf_data, size_t len)
+rproc_load_segments(struct rproc *rproc, const struct firmware *fw)
 {
 	struct device *dev = rproc->dev;
 	struct elf32_hdr *ehdr;
 	struct elf32_phdr *phdr;
 	int i, ret = 0;
+	const u8 *elf_data = fw->data;
 
 	ehdr = (struct elf32_hdr *)elf_data;
 	phdr = (struct elf32_phdr *)(elf_data + ehdr->e_phoff);
@@ -246,9 +246,9 @@ rproc_load_segments(struct rproc *rproc, const u8 *elf_data, size_t len)
 			break;
 		}
 
-		if (offset + filesz > len) {
+		if (offset + filesz > fw->size) {
 			dev_err(dev, "truncated fw: need 0x%x avail 0x%zx\n",
-					offset + filesz, len);
+					offset + filesz, fw->size);
 			ret = -EINVAL;
 			break;
 		}
@@ -824,8 +824,7 @@ rproc_handle_virtio_rsc(struct rproc *rproc, struct resource_table *table, int l
 /**
  * rproc_find_rsc_table() - find the resource table
  * @rproc: the rproc handle
- * @elf_data: the content of the ELF firmware image
- * @len: firmware size (in bytes)
+ * @fw: the ELF firmware image
  * @tablesz: place holder for providing back the table size
  *
  * This function finds the resource table inside the remote processor's
@@ -838,7 +837,7 @@ rproc_handle_virtio_rsc(struct rproc *rproc, struct resource_table *table, int l
  * (and @tablesz isn't set).
  */
 static struct resource_table *
-rproc_find_rsc_table(struct rproc *rproc, const u8 *elf_data, size_t len,
+rproc_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
 							int *tablesz)
 {
 	struct elf32_hdr *ehdr;
@@ -847,11 +846,19 @@ rproc_find_rsc_table(struct rproc *rproc, const u8 *elf_data, size_t len,
 	struct device *dev = rproc->dev;
 	struct resource_table *table = NULL;
 	int i;
+	const u8 *elf_data = fw->data;
 
 	ehdr = (struct elf32_hdr *)elf_data;
 	shdr = (struct elf32_shdr *)(elf_data + ehdr->e_shoff);
 	name_table = elf_data + shdr[ehdr->e_shstrndx].sh_offset;
 
+	/*
+	 * The ELF entry point is the rproc's boot addr (though this is not
+	 * a configurable property of all remote processors: some will always
+	 * boot at a specific hardcoded address).
+	 */
+	rproc->bootaddr = ehdr->e_entry;
+
 	/* look for the resource table and handle it */
 	for (i = 0; i < ehdr->e_shnum; i++, shdr++) {
 		int size = shdr->sh_size;
@@ -863,7 +870,7 @@ rproc_find_rsc_table(struct rproc *rproc, const u8 *elf_data, size_t len,
 		table = (struct resource_table *)(elf_data + offset);
 
 		/* make sure we have the entire table */
-		if (offset + size > len) {
+		if (offset + size > fw->size) {
 			dev_err(dev, "resource table truncated\n");
 			return NULL;
 		}
@@ -1032,15 +1039,8 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 		return ret;
 	}
 
-	/*
-	 * The ELF entry point is the rproc's boot addr (though this is not
-	 * a configurable property of all remote processors: some will always
-	 * boot at a specific hardcoded address).
-	 */
-	rproc->bootaddr = ehdr->e_entry;
-
 	/* look for the resource table */
-	table = rproc_find_rsc_table(rproc, fw->data, fw->size, &tablesz);
+	table = rproc_find_rsc_table(rproc, fw, &tablesz);
 	if (!table)
 		goto clean_up;
 
@@ -1052,7 +1052,7 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	}
 
 	/* load the ELF segments to memory */
-	ret = rproc_load_segments(rproc, fw->data, fw->size);
+	ret = rproc_load_segments(rproc, fw);
 	if (ret) {
 		dev_err(dev, "Failed to load program segments: %d\n", ret);
 		goto clean_up;
@@ -1095,7 +1095,7 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
 		goto out;
 
 	/* look for the resource table */
-	table = rproc_find_rsc_table(rproc, fw->data, fw->size, &tablesz);
+	table = rproc_find_rsc_table(rproc, fw,  &tablesz);
 	if (!table)
 		goto out;
 
-- 
1.7.9.5

[RFC 3/6] remoteproc: Move fw sanity check to find_rsc_table.

[sjur.brandeland]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

Prepare for pluggable firmware handling by calling firmware
sanity check from function find_rsc_table.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/remoteproc/remoteproc_core.c |   15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 3d980ea..785d66b 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -43,6 +43,7 @@
 
 #include "remoteproc_internal.h"
 
+static int rproc_fw_sanity_check(struct rproc *rproc, const struct firmware *fw);
 static void klist_rproc_get(struct klist_node *n);
 static void klist_rproc_put(struct klist_node *n);
 
@@ -848,6 +849,9 @@ rproc_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
 	int i;
 	const u8 *elf_data = fw->data;
 
+	if (rproc_fw_sanity_check(rproc, fw) < 0)
+		return NULL;
+
 	ehdr = (struct elf32_hdr *)elf_data;
 	shdr = (struct elf32_shdr *)(elf_data + ehdr->e_shoff);
 	name_table = elf_data + shdr[ehdr->e_shstrndx].sh_offset;
@@ -1021,10 +1025,6 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	struct resource_table *table;
 	int ret, tablesz;
 
-	ret = rproc_fw_sanity_check(rproc, fw);
-	if (ret)
-		return ret;
-
 	ehdr = (struct elf32_hdr *)fw->data;
 
 	dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
@@ -1041,8 +1041,10 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 
 	/* look for the resource table */
 	table = rproc_find_rsc_table(rproc, fw, &tablesz);
-	if (!table)
+	if (!table) {
+		ret = -EINVAL;
 		goto clean_up;
+	}
 
 	/* handle fw resources which are required to boot rproc */
 	ret = rproc_handle_boot_rsc(rproc, table, tablesz);
@@ -1091,9 +1093,6 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
 	struct resource_table *table;
 	int ret, tablesz;
 
-	if (rproc_fw_sanity_check(rproc, fw) < 0)
-		goto out;
-
 	/* look for the resource table */
 	table = rproc_find_rsc_table(rproc, fw,  &tablesz);
 	if (!table)
-- 
1.7.9.5

[RFC 4/6] remoteproc: Move Elf related functions to separate file

[sjur.brandeland]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

Move the ELF loaders to separate file. Move the functions
rproc_find_rsc_table(), rproc_fw_sanity_check() and rproc_find_rsc_table()
to the file remoteproc_elf_loader.c

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/remoteproc/Makefile                |    1 +
 drivers/remoteproc/remoteproc_core.c       |  242 +-----------------------
 drivers/remoteproc/remoteproc_elf_loader.c |  273 ++++++++++++++++++++++++++++
 include/linux/remoteproc.h                 |    8 +
 4 files changed, 283 insertions(+), 241 deletions(-)
 create mode 100644 drivers/remoteproc/remoteproc_elf_loader.c

diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index 5445d9b..934ce6e 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -6,4 +6,5 @@ obj-$(CONFIG_REMOTEPROC)		+= remoteproc.o
 remoteproc-y				:= remoteproc_core.o
 remoteproc-y				+= remoteproc_debugfs.o
 remoteproc-y				+= remoteproc_virtio.o
+remoteproc-y				+= remoteproc_elf_loader.o
 obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 785d66b..33384e6 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -43,7 +43,6 @@
 
 #include "remoteproc_internal.h"
 
-static int rproc_fw_sanity_check(struct rproc *rproc, const struct firmware *fw);
 static void klist_rproc_get(struct klist_node *n);
 static void klist_rproc_put(struct klist_node *n);
 
@@ -166,7 +165,7 @@ static void rproc_disable_iommu(struct rproc *rproc)
  * but only on kernel direct mapped RAM memory. Instead, we're just using
  * here the output of the DMA API, which should be more correct.
  */
-static void *rproc_da_to_va(struct rproc *rproc, u64 da, int len)
+void *rproc_da_to_va(struct rproc *rproc, u64 da, int len)
 {
 	struct rproc_mem_entry *carveout;
 	void *ptr = NULL;
@@ -190,96 +189,6 @@ static void *rproc_da_to_va(struct rproc *rproc, u64 da, int len)
 	return ptr;
 }
 
-/**
- * rproc_load_segments() - load firmware segments to memory
- * @rproc: remote processor which will be booted using these fw segments
- * @fw: the  the ELF firmware image
- *
- * This function loads the firmware segments to memory, where the remote
- * processor expects them.
- *
- * Some remote processors will expect their code and data to be placed
- * in specific device addresses, and can't have them dynamically assigned.
- *
- * We currently support only those kind of remote processors, and expect
- * the program header's paddr member to contain those addresses. We then go
- * through the physically contiguous "carveout" memory regions which we
- * allocated (and mapped) earlier on behalf of the remote processor,
- * and "translate" device address to kernel addresses, so we can copy the
- * segments where they are expected.
- *
- * Currently we only support remote processors that required carveout
- * allocations and got them mapped onto their iommus. Some processors
- * might be different: they might not have iommus, and would prefer to
- * directly allocate memory for every segment/resource. This is not yet
- * supported, though.
- */
-static int
-rproc_load_segments(struct rproc *rproc, const struct firmware *fw)
-{
-	struct device *dev = rproc->dev;
-	struct elf32_hdr *ehdr;
-	struct elf32_phdr *phdr;
-	int i, ret = 0;
-	const u8 *elf_data = fw->data;
-
-	ehdr = (struct elf32_hdr *)elf_data;
-	phdr = (struct elf32_phdr *)(elf_data + ehdr->e_phoff);
-
-	/* go through the available ELF segments */
-	for (i = 0; i < ehdr->e_phnum; i++, phdr++) {
-		u32 da = phdr->p_paddr;
-		u32 memsz = phdr->p_memsz;
-		u32 filesz = phdr->p_filesz;
-		u32 offset = phdr->p_offset;
-		void *ptr;
-
-		if (phdr->p_type != PT_LOAD)
-			continue;
-
-		dev_dbg(dev, "phdr: type %d da 0x%x memsz 0x%x filesz 0x%x\n",
-					phdr->p_type, da, memsz, filesz);
-
-		if (filesz > memsz) {
-			dev_err(dev, "bad phdr filesz 0x%x memsz 0x%x\n",
-							filesz, memsz);
-			ret = -EINVAL;
-			break;
-		}
-
-		if (offset + filesz > fw->size) {
-			dev_err(dev, "truncated fw: need 0x%x avail 0x%zx\n",
-					offset + filesz, fw->size);
-			ret = -EINVAL;
-			break;
-		}
-
-		/* grab the kernel address for this device address */
-		ptr = rproc_da_to_va(rproc, da, memsz);
-		if (!ptr) {
-			dev_err(dev, "bad phdr da 0x%x mem 0x%x\n", da, memsz);
-			ret = -EINVAL;
-			break;
-		}
-
-		/* put the segment where the remote processor expects it */
-		if (phdr->p_filesz)
-			memcpy(ptr, elf_data + phdr->p_offset, filesz);
-
-		/*
-		 * Zero out remaining memory for this segment.
-		 *
-		 * This isn't strictly required since dma_alloc_coherent already
-		 * did this for us. albeit harmless, we may consider removing
-		 * this.
-		 */
-		if (memsz > filesz)
-			memset(ptr + filesz, 0, memsz - filesz);
-	}
-
-	return ret;
-}
-
 static int
 __rproc_handle_vring(struct rproc_vdev *rvdev, struct fw_rsc_vdev *rsc, int i)
 {
@@ -823,95 +732,6 @@ rproc_handle_virtio_rsc(struct rproc *rproc, struct resource_table *table, int l
 }
 
 /**
- * rproc_find_rsc_table() - find the resource table
- * @rproc: the rproc handle
- * @fw: the ELF firmware image
- * @tablesz: place holder for providing back the table size
- *
- * This function finds the resource table inside the remote processor's
- * firmware. It is used both upon the registration of @rproc (in order
- * to look for and register the supported virito devices), and when the
- * @rproc is booted.
- *
- * Returns the pointer to the resource table if it is found, and write its
- * size into @tablesz. If a valid table isn't found, NULL is returned
- * (and @tablesz isn't set).
- */
-static struct resource_table *
-rproc_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
-							int *tablesz)
-{
-	struct elf32_hdr *ehdr;
-	struct elf32_shdr *shdr;
-	const char *name_table;
-	struct device *dev = rproc->dev;
-	struct resource_table *table = NULL;
-	int i;
-	const u8 *elf_data = fw->data;
-
-	if (rproc_fw_sanity_check(rproc, fw) < 0)
-		return NULL;
-
-	ehdr = (struct elf32_hdr *)elf_data;
-	shdr = (struct elf32_shdr *)(elf_data + ehdr->e_shoff);
-	name_table = elf_data + shdr[ehdr->e_shstrndx].sh_offset;
-
-	/*
-	 * The ELF entry point is the rproc's boot addr (though this is not
-	 * a configurable property of all remote processors: some will always
-	 * boot at a specific hardcoded address).
-	 */
-	rproc->bootaddr = ehdr->e_entry;
-
-	/* look for the resource table and handle it */
-	for (i = 0; i < ehdr->e_shnum; i++, shdr++) {
-		int size = shdr->sh_size;
-		int offset = shdr->sh_offset;
-
-		if (strcmp(name_table + shdr->sh_name, ".resource_table"))
-			continue;
-
-		table = (struct resource_table *)(elf_data + offset);
-
-		/* make sure we have the entire table */
-		if (offset + size > fw->size) {
-			dev_err(dev, "resource table truncated\n");
-			return NULL;
-		}
-
-		/* make sure table has at least the header */
-		if (sizeof(struct resource_table) > size) {
-			dev_err(dev, "header-less resource table\n");
-			return NULL;
-		}
-
-		/* we don't support any version beyond the first */
-		if (table->ver != 1) {
-			dev_err(dev, "unsupported fw ver: %d\n", table->ver);
-			return NULL;
-		}
-
-		/* make sure reserved bytes are zeroes */
-		if (table->reserved[0] || table->reserved[1]) {
-			dev_err(dev, "non zero reserved bytes\n");
-			return NULL;
-		}
-
-		/* make sure the offsets array isn't truncated */
-		if (table->num * sizeof(table->offset[0]) +
-				sizeof(struct resource_table) > size) {
-			dev_err(dev, "resource table incomplete\n");
-			return NULL;
-		}
-
-		*tablesz = shdr->sh_size;
-		break;
-	}
-
-	return table;
-}
-
-/**
  * rproc_resource_cleanup() - clean up and free all acquired resources
  * @rproc: rproc handle
  *
@@ -954,66 +774,6 @@ static void rproc_resource_cleanup(struct rproc *rproc)
 	}
 }
 
-/* make sure this fw image is sane */
-static int rproc_fw_sanity_check(struct rproc *rproc, const struct firmware *fw)
-{
-	const char *name = rproc->firmware;
-	struct device *dev = rproc->dev;
-	struct elf32_hdr *ehdr;
-	char class;
-
-	if (!fw) {
-		dev_err(dev, "failed to load %s\n", name);
-		return -EINVAL;
-	}
-
-	if (fw->size < sizeof(struct elf32_hdr)) {
-		dev_err(dev, "Image is too small\n");
-		return -EINVAL;
-	}
-
-	ehdr = (struct elf32_hdr *)fw->data;
-
-	/* We only support ELF32 at this point */
-	class = ehdr->e_ident[EI_CLASS];
-	if (class != ELFCLASS32) {
-		dev_err(dev, "Unsupported class: %d\n", class);
-		return -EINVAL;
-	}
-
-	/* We assume the firmware has the same endianess as the host */
-# ifdef __LITTLE_ENDIAN
-	if (ehdr->e_ident[EI_DATA] != ELFDATA2LSB) {
-# else /* BIG ENDIAN */
-	if (ehdr->e_ident[EI_DATA] != ELFDATA2MSB) {
-# endif
-		dev_err(dev, "Unsupported firmware endianess\n");
-		return -EINVAL;
-	}
-
-	if (fw->size < ehdr->e_shoff + sizeof(struct elf32_shdr)) {
-		dev_err(dev, "Image is too small\n");
-		return -EINVAL;
-	}
-
-	if (memcmp(ehdr->e_ident, ELFMAG, SELFMAG)) {
-		dev_err(dev, "Image is corrupted (bad magic)\n");
-		return -EINVAL;
-	}
-
-	if (ehdr->e_phnum == 0) {
-		dev_err(dev, "No loadable segments\n");
-		return -EINVAL;
-	}
-
-	if (ehdr->e_phoff > fw->size) {
-		dev_err(dev, "Firmware size is too small\n");
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 /*
  * take a firmware and boot a remote processor with it.
  */
diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
new file mode 100644
index 0000000..3a1ae83
--- /dev/null
+++ b/drivers/remoteproc/remoteproc_elf_loader.c
@@ -0,0 +1,273 @@
+/*
+ * Remote Processor Framework Elf loader
+ *
+ * Copyright (C) 2011 Texas Instruments, Inc.
+ * Copyright (C) 2011 Google, Inc.
+ *
+ * Ohad Ben-Cohen <ohad@wizery.com>
+ * Brian Swetland <swetland@google.com>
+ * Mark Grosen <mgrosen@ti.com>
+ * Fernando Guzman Lugo <fernando.lugo@ti.com>
+ * Suman Anna <s-anna@ti.com>
+ * Robert Tivy <rtivy@ti.com>
+ * Armando Uribe De Leon <x0095078@ti.com>
+ * Sjur Brændeland <sjur.brandeland@stericsson.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#define pr_fmt(fmt)    "%s: " fmt, __func__
+
+#include <linux/module.h>
+#include <linux/firmware.h>
+#include <linux/remoteproc.h>
+#include <linux/elf.h>
+
+#include "remoteproc_internal.h"
+
+/* make sure this fw image is sane */
+static int
+rproc_fw_sanity_check(struct rproc *rproc, const struct firmware *fw)
+{
+	const char *name = rproc->firmware;
+	struct device *dev = rproc->dev;
+	struct elf32_hdr *ehdr;
+	char class;
+
+	if (!fw) {
+		dev_err(dev, "failed to load %s\n", name);
+		return -EINVAL;
+	}
+
+	if (fw->size < sizeof(struct elf32_hdr)) {
+		dev_err(dev, "Image is too small\n");
+		return -EINVAL;
+	}
+
+	ehdr = (struct elf32_hdr *)fw->data;
+
+	/* We only support ELF32 at this point */
+	class = ehdr->e_ident[EI_CLASS];
+	if (class != ELFCLASS32) {
+		dev_err(dev, "Unsupported class: %d\n", class);
+		return -EINVAL;
+	}
+
+	/* We assume the firmware has the same endianess as the host */
+# ifdef __LITTLE_ENDIAN
+	if (ehdr->e_ident[EI_DATA] != ELFDATA2LSB) {
+# else /* BIG ENDIAN */
+	if (ehdr->e_ident[EI_DATA] != ELFDATA2MSB) {
+# endif
+		dev_err(dev, "Unsupported firmware endianess\n");
+		return -EINVAL;
+	}
+
+	if (fw->size < ehdr->e_shoff + sizeof(struct elf32_shdr)) {
+		dev_err(dev, "Image is too small\n");
+		return -EINVAL;
+	}
+
+	if (memcmp(ehdr->e_ident, ELFMAG, SELFMAG)) {
+		dev_err(dev, "Image is corrupted (bad magic)\n");
+		return -EINVAL;
+	}
+
+	if (ehdr->e_phnum == 0) {
+		dev_err(dev, "No loadable segments\n");
+		return -EINVAL;
+	}
+
+	if (ehdr->e_phoff > fw->size) {
+		dev_err(dev, "Firmware size is too small\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/**
+ * rproc_load_segments() - load firmware segments to memory
+ * @rproc: remote processor which will be booted using these fw segments
+ * @fw: the  the ELF firmware image
+ *
+ * This function loads the firmware segments to memory, where the remote
+ * processor expects them.
+ *
+ * Some remote processors will expect their code and data to be placed
+ * in specific device addresses, and can't have them dynamically assigned.
+ *
+ * We currently support only those kind of remote processors, and expect
+ * the program header's paddr member to contain those addresses. We then go
+ * through the physically contiguous "carveout" memory regions which we
+ * allocated (and mapped) earlier on behalf of the remote processor,
+ * and "translate" device address to kernel addresses, so we can copy the
+ * segments where they are expected.
+ *
+ * Currently we only support remote processors that required carveout
+ * allocations and got them mapped onto their iommus. Some processors
+ * might be different: they might not have iommus, and would prefer to
+ * directly allocate memory for every segment/resource. This is not yet
+ * supported, though.
+ */
+int
+rproc_load_segments(struct rproc *rproc, const struct firmware *fw)
+{
+	struct device *dev = rproc->dev;
+	struct elf32_hdr *ehdr;
+	struct elf32_phdr *phdr;
+	int i, ret = 0;
+	const u8 *elf_data = fw->data;
+
+	ehdr = (struct elf32_hdr *)elf_data;
+	phdr = (struct elf32_phdr *)(elf_data + ehdr->e_phoff);
+
+	/* go through the available ELF segments */
+	for (i = 0; i < ehdr->e_phnum; i++, phdr++) {
+		u32 da = phdr->p_paddr;
+		u32 memsz = phdr->p_memsz;
+		u32 filesz = phdr->p_filesz;
+		u32 offset = phdr->p_offset;
+		void *ptr;
+
+		if (phdr->p_type != PT_LOAD)
+			continue;
+
+		dev_dbg(dev, "phdr: type %d da 0x%x memsz 0x%x filesz 0x%x\n",
+					phdr->p_type, da, memsz, filesz);
+
+		if (filesz > memsz) {
+			dev_err(dev, "bad phdr filesz 0x%x memsz 0x%x\n",
+							filesz, memsz);
+			ret = -EINVAL;
+			break;
+		}
+
+		if (offset + filesz > fw->size) {
+			dev_err(dev, "truncated fw: need 0x%x avail 0x%zx\n",
+					offset + filesz, fw->size);
+			ret = -EINVAL;
+			break;
+		}
+
+		/* grab the kernel address for this device address */
+		ptr = rproc_da_to_va(rproc, da, memsz);
+		if (!ptr) {
+			dev_err(dev, "bad phdr da 0x%x mem 0x%x\n", da, memsz);
+			ret = -EINVAL;
+			break;
+		}
+
+		/* put the segment where the remote processor expects it */
+		if (phdr->p_filesz)
+			memcpy(ptr, elf_data + phdr->p_offset, filesz);
+
+		/*
+		 * Zero out remaining memory for this segment.
+		 *
+		 * This isn't strictly required since dma_alloc_coherent already
+		 * did this for us. albeit harmless, we may consider removing
+		 * this.
+		 */
+		if (memsz > filesz)
+			memset(ptr + filesz, 0, memsz - filesz);
+	}
+
+	return ret;
+}
+
+/**
+ * rproc_find_rsc_table() - find the resource table
+ * @rproc: the rproc handle
+ * @fw: the ELF firmware image
+ * @tablesz: place holder for providing back the table size
+ *
+ * This function finds the resource table inside the remote processor's
+ * firmware. It is used both upon the registration of @rproc (in order
+ * to look for and register the supported virito devices), and when the
+ * @rproc is booted.
+ *
+ * Returns the pointer to the resource table if it is found, and write its
+ * size into @tablesz. If a valid table isn't found, NULL is returned
+ * (and @tablesz isn't set).
+ */
+struct resource_table *
+rproc_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
+							int *tablesz)
+{
+	struct elf32_hdr *ehdr;
+	struct elf32_shdr *shdr;
+	const char *name_table;
+	struct device *dev = rproc->dev;
+	struct resource_table *table = NULL;
+	int i;
+	const u8 *elf_data = fw->data;
+
+	if (rproc_fw_sanity_check(rproc, fw) < 0)
+		return NULL;
+
+	ehdr = (struct elf32_hdr *)elf_data;
+	shdr = (struct elf32_shdr *)(elf_data + ehdr->e_shoff);
+	name_table = elf_data + shdr[ehdr->e_shstrndx].sh_offset;
+
+	/*
+	 * The ELF entry point is the rproc's boot addr (though this is not
+	 * a configurable property of all remote processors: some will always
+	 * boot at a specific hardcoded address).
+	 */
+	rproc->bootaddr = ehdr->e_entry;
+
+	/* look for the resource table and handle it */
+	for (i = 0; i < ehdr->e_shnum; i++, shdr++) {
+		int size = shdr->sh_size;
+		int offset = shdr->sh_offset;
+
+		if (strcmp(name_table + shdr->sh_name, ".resource_table"))
+			continue;
+
+		table = (struct resource_table *)(elf_data + offset);
+
+		/* make sure we have the entire table */
+		if (offset + size > fw->size) {
+			dev_err(dev, "resource table truncated\n");
+			return NULL;
+		}
+
+		/* make sure table has at least the header */
+		if (sizeof(struct resource_table) > size) {
+			dev_err(dev, "header-less resource table\n");
+			return NULL;
+		}
+
+		/* we don't support any version beyond the first */
+		if (table->ver != 1) {
+			dev_err(dev, "unsupported fw ver: %d\n", table->ver);
+			return NULL;
+		}
+
+		/* make sure reserved bytes are zeroes */
+		if (table->reserved[0] || table->reserved[1]) {
+			dev_err(dev, "non zero reserved bytes\n");
+			return NULL;
+		}
+
+		/* make sure the offsets array isn't truncated */
+		if (table->num * sizeof(table->offset[0]) +
+				sizeof(struct resource_table) > size) {
+			dev_err(dev, "resource table incomplete\n");
+			return NULL;
+		}
+
+		*tablesz = shdr->sh_size;
+		break;
+	}
+
+	return table;
+}
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index f1ffabb..372d1a8 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -42,6 +42,7 @@
 #include <linux/virtio.h>
 #include <linux/completion.h>
 #include <linux/idr.h>
+#include <linux/firmware.h>
 
 /**
  * struct resource_table - firmware resource table header
@@ -326,6 +327,12 @@ struct rproc_mem_entry {
 
 struct rproc;
 
+int
+rproc_load_segments(struct rproc *rproc, const struct firmware *fw);
+struct resource_table *
+rproc_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
+								int *tablesz);
+
 /**
  * struct rproc_ops - platform-specific device handlers
  * @start:	power on the device and boot it
@@ -462,6 +469,7 @@ int rproc_unregister(struct rproc *rproc);
 
 int rproc_boot(struct rproc *rproc);
 void rproc_shutdown(struct rproc *rproc);
+void *rproc_da_to_va(struct rproc *rproc, u64 da, int len);
 
 static inline struct rproc_vdev *vdev_to_rvdev(struct virtio_device *vdev)
 {
-- 
1.7.9.5

[RFC 5/6] remoteproc: Support custom firmware handlers

[sjur.brandeland]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

Make the firmware handling customizable by creating
a rproc_fw_ops structure. Expose the existing
Elf firmware handling in rproc_elf_fw_ops.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/remoteproc/omap_remoteproc.c       |    1 +
 drivers/remoteproc/remoteproc_core.c       |   26 +++++++++++++++++---------
 drivers/remoteproc/remoteproc_elf_loader.c |   11 +++++++++--
 include/linux/remoteproc.h                 |   23 ++++++++++++++++++-----
 4 files changed, 45 insertions(+), 16 deletions(-)

diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c
index 69425c4..53ceaab 100644
--- a/drivers/remoteproc/omap_remoteproc.c
+++ b/drivers/remoteproc/omap_remoteproc.c
@@ -171,6 +171,7 @@ static struct rproc_ops omap_rproc_ops = {
 	.start		= omap_rproc_start,
 	.stop		= omap_rproc_stop,
 	.kick		= omap_rproc_kick,
+	.fw		= &rproc_elf_fw_ops
 };
 
 static int __devinit omap_rproc_probe(struct platform_device *pdev)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 33384e6..75984ad 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -36,7 +36,6 @@
 #include <linux/remoteproc.h>
 #include <linux/iommu.h>
 #include <linux/klist.h>
-#include <linux/elf.h>
 #include <linux/virtio_ids.h>
 #include <linux/virtio_ring.h>
 #include <asm/byteorder.h>
@@ -781,12 +780,13 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 {
 	struct device *dev = rproc->dev;
 	const char *name = rproc->firmware;
-	struct elf32_hdr *ehdr;
 	struct resource_table *table;
 	int ret, tablesz;
 
-	ehdr = (struct elf32_hdr *)fw->data;
-
+	if (!rproc->ops->fw_ops || !try_module_get(rproc->ops->fw_ops->owner)) {
+		dev_err(dev, "%s: can't get fw_ops->owner\n", __func__);
+		ret = -EINVAL;
+	}
 	dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
 
 	/*
@@ -796,11 +796,11 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	ret = rproc_enable_iommu(rproc);
 	if (ret) {
 		dev_err(dev, "can't enable iommu: %d\n", ret);
-		return ret;
+		goto out;
 	}
 
 	/* look for the resource table */
-	table = rproc_find_rsc_table(rproc, fw, &tablesz);
+	table = rproc->ops->fw_ops->find_rsc_table(rproc, fw, &tablesz);
 	if (!table) {
 		ret = -EINVAL;
 		goto clean_up;
@@ -813,8 +813,8 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 		goto clean_up;
 	}
 
-	/* load the ELF segments to memory */
-	ret = rproc_load_segments(rproc, fw);
+	/* load the firmware to memory */
+	ret = rproc->ops->fw_ops->load_fw(rproc, fw);
 	if (ret) {
 		dev_err(dev, "Failed to load program segments: %d\n", ret);
 		goto clean_up;
@@ -836,6 +836,8 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 clean_up:
 	rproc_resource_cleanup(rproc);
 	rproc_disable_iommu(rproc);
+out:
+	module_put(rproc->ops->fw_ops->owner);
 	return ret;
 }
 
@@ -853,8 +855,13 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
 	struct resource_table *table;
 	int ret, tablesz;
 
+	if (!rproc->ops->fw_ops || !try_module_get(rproc->ops->fw_ops->owner)) {
+		dev_err(rproc->dev, "%s: can't get fw_ops->owner\n", __func__);
+		return;
+	}
+
 	/* look for the resource table */
-	table = rproc_find_rsc_table(rproc, fw,  &tablesz);
+	table = rproc->ops->fw_ops->find_rsc_table(rproc, fw,  &tablesz);
 	if (!table)
 		goto out;
 
@@ -864,6 +871,7 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
 		goto out;
 
 out:
+	module_put(rproc->ops->fw_ops->owner);
 	if (fw)
 		release_firmware(fw);
 	/* allow rproc_unregister() contexts, if any, to proceed */
diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
index 3a1ae83..b90aeff 100644
--- a/drivers/remoteproc/remoteproc_elf_loader.c
+++ b/drivers/remoteproc/remoteproc_elf_loader.c
@@ -117,7 +117,7 @@ rproc_fw_sanity_check(struct rproc *rproc, const struct firmware *fw)
  * directly allocate memory for every segment/resource. This is not yet
  * supported, though.
  */
-int
+static int
 rproc_load_segments(struct rproc *rproc, const struct firmware *fw)
 {
 	struct device *dev = rproc->dev;
@@ -198,7 +198,7 @@ rproc_load_segments(struct rproc *rproc, const struct firmware *fw)
  * size into @tablesz. If a valid table isn't found, NULL is returned
  * (and @tablesz isn't set).
  */
-struct resource_table *
+static struct resource_table *
 rproc_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
 							int *tablesz)
 {
@@ -271,3 +271,10 @@ rproc_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
 
 	return table;
 }
+
+struct rproc_fw_ops rproc_elf_fw_ops = {
+	.load_fw = rproc_load_segments,
+	.find_rsc_table = rproc_find_rsc_table,
+	.owner = THIS_MODULE
+};
+EXPORT_SYMBOL(rproc_elf_fw_ops);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 372d1a8..7ebb1ef 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -327,22 +327,33 @@ struct rproc_mem_entry {
 
 struct rproc;
 
-int
-rproc_load_segments(struct rproc *rproc, const struct firmware *fw);
-struct resource_table *
-rproc_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
-								int *tablesz);
+/**
+ * struct rproc_fw_ops - firmware format specific operations.
+ * @find_rsc_table:	finds the resource table inside the firmware image.
+ * @load_fw:		load firmeware to memory, where the remote processor
+ *			expects to find it.
+ * @owner:		module owning the operations.
+ */
+struct rproc_fw_ops {
+	struct resource_table *(*find_rsc_table) (struct rproc *rproc,
+						const struct firmware *fw,
+						int *tablesz);
+	int (*load_fw)(struct rproc *rproc, const struct firmware *fw);
+	struct module *owner;
+};
 
 /**
  * struct rproc_ops - platform-specific device handlers
  * @start:	power on the device and boot it
  * @stop:	power off the device
  * @kick:	kick a virtqueue (virtqueue id given as a parameter)
+ * @fw_ops:	firmware operations for the remote processor
  */
 struct rproc_ops {
 	int (*start)(struct rproc *rproc);
 	int (*stop)(struct rproc *rproc);
 	void (*kick)(struct rproc *rproc, int vqid);
+	struct rproc_fw_ops *fw_ops;
 };
 
 /**
@@ -483,4 +494,6 @@ static inline struct rproc *vdev_to_rproc(struct virtio_device *vdev)
 	return rvdev->rproc;
 }
 
+extern struct rproc_fw_ops rproc_elf_fw_ops;
+
 #endif /* REMOTEPROC_H */
-- 
1.7.9.5

[RFC 6/6] remoteproc: Make REMOTEPROC_ELF a sparate Kconfig

[sjur.brandeland]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

Make REMOTEPROC_ELF a separate Kconfig entry.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/remoteproc/Kconfig                 |    6 ++++++
 drivers/remoteproc/Makefile                |    2 +-
 drivers/remoteproc/remoteproc_core.c       |    1 +
 drivers/remoteproc/remoteproc_elf_loader.c |    2 ++
 4 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 24d880e..316a726 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -5,11 +5,17 @@ config REMOTEPROC
 	tristate
 	depends on EXPERIMENTAL
 
+config REMOTEPROC_ELF
+	tristate
+	depends on REMOTEPROC && EXPERIMENTAL
+	default m
+
 config OMAP_REMOTEPROC
 	tristate "OMAP remoteproc support"
 	depends on ARCH_OMAP4
 	depends on OMAP_IOMMU
 	select REMOTEPROC
+	select REMOTEPROC_ELF
 	select OMAP_MBOX_FWK
 	select RPMSG
 	help
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index 934ce6e..047d286 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -6,5 +6,5 @@ obj-$(CONFIG_REMOTEPROC)		+= remoteproc.o
 remoteproc-y				:= remoteproc_core.o
 remoteproc-y				+= remoteproc_debugfs.o
 remoteproc-y				+= remoteproc_virtio.o
-remoteproc-y				+= remoteproc_elf_loader.o
+obj-$(CONFIG_REMOTEPROC_ELF)		+= remoteproc_elf_loader.o
 obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 75984ad..7aca0d7 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -187,6 +187,7 @@ void *rproc_da_to_va(struct rproc *rproc, u64 da, int len)
 
 	return ptr;
 }
+EXPORT_SYMBOL(rproc_da_to_va);
 
 static int
 __rproc_handle_vring(struct rproc_vdev *rvdev, struct fw_rsc_vdev *rsc, int i)
diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
index b90aeff..b726bfe 100644
--- a/drivers/remoteproc/remoteproc_elf_loader.c
+++ b/drivers/remoteproc/remoteproc_elf_loader.c
@@ -278,3 +278,5 @@ struct rproc_fw_ops rproc_elf_fw_ops = {
 	.owner = THIS_MODULE
 };
 EXPORT_SYMBOL(rproc_elf_fw_ops);
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Generic Remote Processor ELF Firmware handling");
-- 
1.7.9.5

Re: [RFC 0/6] remoteproc: custom fw handling

[Ohad Ben-Cohen]

Hi Sjur,

On Tue, May 22, 2012 at 4:26 PM,  <sjur.brandeland@stericsson.com> wrote:
> I have cooked a couple of patches for remoteproc. I was planning to
> test and polish them some more, but figured that I might as well send
> them and find out what you think.

Thanks! I'll give it a good look this week.

> The next five patches are about custom firmware handlers.
> I have done this in small steps to facilitate review, but I
> suspect you might want to squash them all together.

Any chance you have the customer loader patch ready ? Can you submit it too ?

Ideally we should review (and merge) the custom fw groundwork together
with the new fw loader.

> Note that the ELF loaders are not tested. I have no environment to test
> this at the moment.

That's ok, I'll test the ELF part before merging the patch set.

Thanks,
Ohad.

Re: [RFC 1/6] remoteproc: fix print format warnings

[Ohad Ben-Cohen]

On Tue, May 22, 2012 at 4:26 PM,  <sjur.brandeland@stericsson.com> wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Fix compile warnings when printing values of type size_t

Looks good, thanks.

Might be nice to add the actual warning in the commit log, and maybe
also specify the version of your gcc (I think you're the first who
reported about these warnings).

Thanks,
Ohad.

Re: [RFC 2/6] remoteproc: Pass struct fw to load_segments and find_rsc_table.

[Ohad Ben-Cohen]

On Tue, May 22, 2012 at 4:26 PM,  <sjur.brandeland@stericsson.com> wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Use struct firmware as arguments to functions rproc_find_rcs_table()
> and rproc_load_segments().

I'll keep reading through the patches to get the big picture, but in
general, it's best just to provide the incentive behind every change
in the commit log.

> +rproc_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
>                                                        int *tablesz)
>  {
...
> +       /*
> +        * The ELF entry point is the rproc's boot addr (though this is not
> +        * a configurable property of all remote processors: some will always
> +        * boot at a specific hardcoded address).
> +        */
> +       rproc->bootaddr = ehdr->e_entry;
> +
...
> @@ -1032,15 +1039,8 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
> -       /*
> -        * The ELF entry point is the rproc's boot addr (though this is not
> -        * a configurable property of all remote processors: some will always
> -        * boot at a specific hardcoded address).
> -        */
> -       rproc->bootaddr = ehdr->e_entry;

Is this change related?

Thanks,
Ohad.

Re: [RFC 5/6] remoteproc: Support custom firmware handlers

[Ohad Ben-Cohen]

On Tue, May 22, 2012 at 4:26 PM,  <sjur.brandeland@stericsson.com> wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Make the firmware handling customizable by creating
> a rproc_fw_ops structure. Expose the existing
> Elf firmware handling in rproc_elf_fw_ops.
>
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
..
> @@ -171,6 +171,7 @@ static struct rproc_ops omap_rproc_ops = {
>        .start          = omap_rproc_start,
>        .stop           = omap_rproc_stop,
>        .kick           = omap_rproc_kick,
> +       .fw             = &rproc_elf_fw_ops
>  };

Can we instead make these ops dynamically assigned, without mandating
them in the low level driver ?

E.g. with ELF it's easy (first four bytes of the binary), but I'm not
sure how feasible it is with proprietary binary formats (such as
yours).

> @@ -781,12 +780,13 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>  {
>        struct device *dev = rproc->dev;
>        const char *name = rproc->firmware;
> -       struct elf32_hdr *ehdr;
>        struct resource_table *table;
>        int ret, tablesz;
>
> -       ehdr = (struct elf32_hdr *)fw->data;
> -
> +       if (!rproc->ops->fw_ops || !try_module_get(rproc->ops->fw_ops->owner)) {

I'm not sure we need loaders' ops to be full-fledged modules of their
own. For simplicity, we might just have them statically linked with
the remoteproc module.

>        /* look for the resource table */
> -       table = rproc_find_rsc_table(rproc, fw, &tablesz);
> +       table = rproc->ops->fw_ops->find_rsc_table(rproc, fw, &tablesz);

Might be easier on the eyes if we turn rproc_find_rsc_table() (and
friends) into static inline "macros" which does all this
de-referencing.

Thanks,
Ohad.

Re: [RFC 3/6] remoteproc: Move fw sanity check to find_rsc_table.

[Ohad Ben-Cohen]

On Tue, May 22, 2012 at 4:26 PM,  <sjur.brandeland@stericsson.com> wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Prepare for pluggable firmware handling by calling firmware
> sanity check from function find_rsc_table.

Can we keep the present location of rproc_fw_sanity_check(), and later
just invoke the relevant ops handler instead?

> @@ -1041,8 +1041,10 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>
>        /* look for the resource table */
>        table = rproc_find_rsc_table(rproc, fw, &tablesz);
> -       if (!table)
> +       if (!table) {
> +               ret = -EINVAL;

This looks like an unrelated error-path fix which we might want to
merge regardless of this work.

Thanks,
Ohad.

Re: [RFC 2/6] remoteproc: Pass struct fw to load_segments and find_rsc_table.

[Ohad Ben-Cohen]

On Sun, Jun 3, 2012 at 3:54 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>> +rproc_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
>>                                                        int *tablesz)
>>  {
> ...
>> +       /*
>> +        * The ELF entry point is the rproc's boot addr (though this is not
>> +        * a configurable property of all remote processors: some will always
>> +        * boot at a specific hardcoded address).
>> +        */
>> +       rproc->bootaddr = ehdr->e_entry;

We might want to dedicate this (humble) hunk its own handler, because
I'm not sure it fits very well into rproc_find_rsc_table().

Thanks,
Ohad.

Re: [RFC 0/6] remoteproc: custom fw handling

[Ohad Ben-Cohen]

Hi Sjur,

On Sat, May 26, 2012 at 6:03 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> On Tue, May 22, 2012 at 4:26 PM,  <sjur.brandeland@stericsson.com> wrote:
>> I have cooked a couple of patches for remoteproc. I was planning to
>> test and polish them some more, but figured that I might as well send
>> them and find out what you think.
>
> Thanks! I'll give it a good look this week.

I think that generally this looks very good, thanks for sending this!

There's some small stuff we need to hash out but it looks like we can
nail this pretty quickly.

Thanks,
Ohad.

RE: [RFC 3/6] remoteproc: Move fw sanity check to find_rsc_table.

[Sjur BRENDELAND]

>> Prepare for pluggable firmware handling by calling firmware
>> sanity check from function find_rsc_table.
> 
> Can we keep the present location of rproc_fw_sanity_check(), and later
> just invoke the relevant ops handler instead?

The sanity_check() is working on the ELF structures. And all ELF
processing needs to be moved to a separate file. So if you want
to keep the call to sanity_check() here, I need to make the
sanity_check() function a separate handler. That could make sense,
but will also bloat the struct rproc_fw_ops with one more function
pointer.

Regards,
Sjur

RE: [RFC 2/6] remoteproc: Pass struct fw to load_segments and find_rsc_table.

[Sjur BRENDELAND]

> >> +       rproc->bootaddr = ehdr->e_entry;
> 
> We might want to dedicate this (humble) hunk its own handler, because
> I'm not sure it fits very well into rproc_find_rsc_table().

Sure, I can do this, but this whole thing is a bit obscure and I
didn't quite know what to do with it.

This value is not used anywhere in the upstream code. Couldn't we
just delete this, and you can add it when you upstream the code
where this is used?

Regards,
Sjur

Re: [RFC 2/6] remoteproc: Pass struct fw to load_segments and find_rsc_table.

[Ohad Ben-Cohen]

On Mon, Jun 4, 2012 at 12:58 PM, Sjur BRENDELAND
<sjur.brandeland@stericsson.com> wrote:
> This value is not used anywhere in the upstream code. Couldn't we
> just delete this, and you can add it when you upstream the code
> where this is used?

We must have it for OMAP's and DaVinci's DSPs - the relevant patch is
coming up very soon now, so let's please keep this intact.

Thanks,
Ohad.

Re: [RFC 3/6] remoteproc: Move fw sanity check to find_rsc_table.

[Ohad Ben-Cohen]

On Mon, Jun 4, 2012 at 12:56 PM, Sjur BRENDELAND
<sjur.brandeland@stericsson.com> wrote:
> The sanity_check() is working on the ELF structures. And all ELF
> processing needs to be moved to a separate file. So if you want
> to keep the call to sanity_check() here, I need to make the
> sanity_check() function a separate handler. That could make sense,
> but will also bloat the struct rproc_fw_ops with one more function
> pointer.

I think it's OK.

Adding another handler is cheap, and if by doing so the code becomes
even slightly easier to read or maintain, it's probably worth it.

Thanks,
Ohad.