[PATCH v3 00/13] remoteproc: add fixed memory region support

[PATCH v3 00/13] remoteproc: add fixed memory region support

[Loic Pallardy]

The aim of the series is to implement carveout memory management as
discussed during OpenAMP weekly call and defined in proposed document [1]

This first series focus only on adding support of the different types of
carveout memories (dynamic, fixed, platform driver depend...).
64bit resource table will be addressed in a next series.

[1]: http://openamp.github.io/docs/mca/coprocessor-memory-definition-v6.pdf

---
Changes sine V2:
Reshuffle the series to: 
- Take into account Bjorn's comments.
- Add patch to check consistency between carveout resource request and IOMMU
  support.
- Introduce platform specific prepare and unprepare ops to enable HW like
  clock, bus, regulator, memory region... before loading co-processor firmware.
- Rely on memory carveout management for all remoteproc memory allocations.
- Lookup pre-registered carveout by name first.
- Create a subdevice for each vdev declared in firmware resource table that
  will be used by virtio based driver to retrieve specific memory pool.

This series takes some assumptions for carveout names associated to vdev:
- For vring: "vdev%xvring%x" with vdev index from resource table and vring index
  in vdev.
- For vdev buffer: "vdev%xbuffer" with vdev index from resource table

This will be changed in the future, adding names field in vdev resource in
next resource table version.


Changes since V1:
- Minor corrections on first 7 patches (error management)
- Add "memory device" support on the top of first 7 patches.
  Goal is to answer use case reported during OpenAMP weekly discussion:
  - "Be able to specify memory region for vring and buffer allocation, even
    if no specific request defined in firmware resource table."
  Patches offer the capability to create a "memory device" associated to a
  carveout with a dedicated DMA memory pool. Different resource handlers are
  modified to look-up for specific carveout by name. If match found and associated
  "memory device" present, device is used instead of rproc platform device for
  allocation.


Loic Pallardy (13):
  remoteproc: configure IOMMU only if device address requested
  remoteproc: add rproc_va_to_pa function
  remoteproc: add release ops in rproc_mem_entry struct
  remoteproc: add name in rproc_mem_entry struct
  remoteproc: add helper function to allocate and init rproc_mem_entry
    struct
  remoteproc: introduce rproc_add_carveout function
  remoteproc: introduce rproc_find_carveout_by_name function
  remoteproc: add prepare and unprepare ops
  remoteproc: modify rproc_handle_carveout to support pre-registered
    region
  remoteproc: modify vring allocation to support pre-registered region
  remoteproc: create vdev subdevice with specific dma memory pool
  rpmsg: virtio: allocate buffer from parent
  remoteproc: st: add reserved memory support

 drivers/remoteproc/remoteproc_core.c    | 332 ++++++++++++++++++++++++++++----
 drivers/remoteproc/remoteproc_debugfs.c |   1 +
 drivers/remoteproc/remoteproc_virtio.c  |   2 +-
 drivers/remoteproc/st_remoteproc.c      |  68 ++++++-
 drivers/rpmsg/virtio_rpmsg_bus.c        |   2 +-
 include/linux/remoteproc.h              |  22 ++-
 6 files changed, 379 insertions(+), 48 deletions(-)

-- 
1.9.1

[PATCH v3 01/13] remoteproc: configure IOMMU only if device address requested

[Loic Pallardy]

If there is no IOMMU associate to remote processor device,
remoteproc_core won't be able to satisfy device address requested
in firmware resource table.
Return an error as configuration won't be coherent.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 4170dfb..56b33e4 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -654,7 +654,15 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	 * to use the iommu-based DMA API: we expect 'dma' to contain the
 	 * physical address in this case.
 	 */
-	if (rproc->domain) {
+
+	if (rsc->da != FW_RSC_ADDR_ANY && !rproc->domain) {
+		dev_err(dev->parent,
+			"Bad carveout rsc configuration\n");
+		ret = -ENOMEM;
+		goto dma_free;
+	}
+
+	if (rsc->da != FW_RSC_ADDR_ANY && rproc->domain) {
 		mapping = kzalloc(sizeof(*mapping), GFP_KERNEL);
 		if (!mapping) {
 			ret = -ENOMEM;
-- 
1.9.1

[PATCH v3 02/13] remoteproc: add rproc_va_to_pa function

[Loic Pallardy]

This new function translates CPU virtual address in
CPU physical one according to virtual address location.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 56b33e4..eb1b779 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -139,6 +139,22 @@ static void rproc_disable_iommu(struct rproc *rproc)
 	iommu_domain_free(domain);
 }
 
+static phys_addr_t rproc_va_to_pa(void *cpu_addr)
+{
+	/*
+	 * Return physical address according to virtual address location
+	 * - in vmalloc: if region ioremapped or defined as dma_alloc_coherent
+	 * - in kernel: if region allocated in generic dma memory pool
+	 */
+	if (is_vmalloc_addr(cpu_addr)) {
+		return page_to_phys(vmalloc_to_page(cpu_addr)) +
+				    offset_in_page(cpu_addr);
+	}
+
+	WARN_ON(!virt_addr_valid(cpu_addr));
+	return virt_to_phys(cpu_addr);
+}
+
 /**
  * rproc_da_to_va() - lookup the kernel virtual address for a remoteproc address
  * @rproc: handle of a remote processor
@@ -708,7 +724,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	 * In this case, the device address and the physical address
 	 * are the same.
 	 */
-	rsc->pa = dma;
+	rsc->pa = (u32)rproc_va_to_pa(va);
 
 	carveout->va = va;
 	carveout->len = rsc->len;
-- 
1.9.1

[PATCH v3 03/13] remoteproc: add release ops in rproc_mem_entry struct

[Loic Pallardy]

Memory entry could be allocated in different ways (ioremap,
dma_alloc_coherent, internal RAM allocator...).
This patch introduces a release ops in rproc_mem_entry structure
to associate dedicated release mechanism to each memory entry descriptor
in order to keep remoteproc core generic.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 23 +++++++++++++++++++++--
 include/linux/remoteproc.h           |  5 ++++-
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index eb1b779..5f11536 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -597,6 +597,24 @@ static int rproc_handle_devmem(struct rproc *rproc, struct fw_rsc_devmem *rsc,
 }
 
 /**
+ * rproc_release_carveout() - release acquired carveout
+ * @rproc: rproc handle
+ * @mem: the memory entry to release
+ *
+ * This function releases specified memory entry @mem allocated via
+ * dma_alloc_coherent() function by @rproc.
+ */
+static int rproc_release_carveout(struct rproc *rproc,
+				  struct rproc_mem_entry *mem)
+{
+	struct device *dev = &rproc->dev;
+
+	/* clean up carveout allocations */
+	dma_free_coherent(dev->parent, mem->len, mem->va, mem->dma);
+	return 0;
+}
+
+/**
  * rproc_handle_carveout() - handle phys contig memory allocation requests
  * @rproc: rproc handle
  * @rsc: the resource entry
@@ -730,6 +748,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	carveout->len = rsc->len;
 	carveout->dma = dma;
 	carveout->da = rsc->da;
+	carveout->release = rproc_release_carveout;
 
 	list_add_tail(&carveout->node, &rproc->carveouts);
 
@@ -863,8 +882,8 @@ static void rproc_resource_cleanup(struct rproc *rproc)
 
 	/* clean up carveout allocations */
 	list_for_each_entry_safe(entry, tmp, &rproc->carveouts, node) {
-		dma_free_coherent(dev->parent, entry->len, entry->va,
-				  entry->dma);
+		if (entry->release)
+			entry->release(rproc, entry);
 		list_del(&entry->node);
 		kfree(entry);
 	}
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 728d421..dedc138 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -305,12 +305,15 @@ struct fw_rsc_vdev {
 	struct fw_rsc_vdev_vring vring[0];
 } __packed;
 
+struct rproc;
+
 /**
  * struct rproc_mem_entry - memory entry descriptor
  * @va:	virtual address
  * @dma: dma address
  * @len: length, in bytes
  * @da: device address
+ * @release: release associated memory
  * @priv: associated data
  * @node: list node
  */
@@ -321,9 +324,9 @@ struct rproc_mem_entry {
 	u32 da;
 	void *priv;
 	struct list_head node;
+	int (*release)(struct rproc *rproc, struct rproc_mem_entry *mem);
 };
 
-struct rproc;
 struct firmware;
 
 /**
-- 
1.9.1

[PATCH v3 04/13] remoteproc: add name in rproc_mem_entry struct

[Loic Pallardy]

Add name field in struct rproc_mem_entry.
This new field will be used to match memory area
requested in resource table with pre-registered carveout.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c    | 1 +
 drivers/remoteproc/remoteproc_debugfs.c | 1 +
 include/linux/remoteproc.h              | 2 ++
 3 files changed, 4 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 5f11536..f73a0cf 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -749,6 +749,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	carveout->dma = dma;
 	carveout->da = rsc->da;
 	carveout->release = rproc_release_carveout;
+	strlcpy(carveout->name, rsc->name, sizeof(carveout->name));
 
 	list_add_tail(&carveout->node, &rproc->carveouts);
 
diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
index a204883..fc0e570 100644
--- a/drivers/remoteproc/remoteproc_debugfs.c
+++ b/drivers/remoteproc/remoteproc_debugfs.c
@@ -260,6 +260,7 @@ static int rproc_carveouts_show(struct seq_file *seq, void *p)
 
 	list_for_each_entry(carveout, &rproc->carveouts, node) {
 		seq_puts(seq, "Carveout memory entry:\n");
+		seq_printf(seq, "\tName: %s\n", carveout->name);
 		seq_printf(seq, "\tVirtual address: %p\n", carveout->va);
 		seq_printf(seq, "\tDMA address: %pad\n", &carveout->dma);
 		seq_printf(seq, "\tDevice address: 0x%x\n", carveout->da);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index dedc138..2202c08 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -315,6 +315,7 @@ struct fw_rsc_vdev {
  * @da: device address
  * @release: release associated memory
  * @priv: associated data
+ * @name: associated memory region name (optional)
  * @node: list node
  */
 struct rproc_mem_entry {
@@ -323,6 +324,7 @@ struct rproc_mem_entry {
 	int len;
 	u32 da;
 	void *priv;
+	char name[32];
 	struct list_head node;
 	int (*release)(struct rproc *rproc, struct rproc_mem_entry *mem);
 };
-- 
1.9.1

[PATCH v3 05/13] remoteproc: add helper function to allocate and init rproc_mem_entry struct

[Loic Pallardy]

This patch introduces rproc_mem_entry_init helper function to
simplify rproc_mem_entry structure allocation and filling by
client.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 64 +++++++++++++++++++++++++++---------
 include/linux/remoteproc.h           |  5 +++
 2 files changed, 53 insertions(+), 16 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index f73a0cf..4c92b7d 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -636,7 +636,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
 				 struct fw_rsc_carveout *rsc,
 				 int offset, int avail)
 {
-	struct rproc_mem_entry *carveout, *mapping;
+	struct rproc_mem_entry *carveout, *mapping = NULL;
 	struct device *dev = &rproc->dev;
 	dma_addr_t dma;
 	void *va;
@@ -656,16 +656,11 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	dev_dbg(dev, "carveout rsc: name: %s, da 0x%x, pa 0x%x, len 0x%x, flags 0x%x\n",
 		rsc->name, rsc->da, rsc->pa, rsc->len, rsc->flags);
 
-	carveout = kzalloc(sizeof(*carveout), GFP_KERNEL);
-	if (!carveout)
-		return -ENOMEM;
-
 	va = dma_alloc_coherent(dev->parent, rsc->len, &dma, GFP_KERNEL);
 	if (!va) {
 		dev_err(dev->parent,
 			"failed to allocate dma memory: len 0x%x\n", rsc->len);
-		ret = -ENOMEM;
-		goto free_carv;
+		return -ENOMEM;
 	}
 
 	dev_dbg(dev, "carveout va %p, dma %pad, len 0x%x\n",
@@ -744,27 +739,64 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	 */
 	rsc->pa = (u32)rproc_va_to_pa(va);
 
-	carveout->va = va;
-	carveout->len = rsc->len;
-	carveout->dma = dma;
-	carveout->da = rsc->da;
-	carveout->release = rproc_release_carveout;
-	strlcpy(carveout->name, rsc->name, sizeof(carveout->name));
+	carveout = rproc_mem_entry_init(dev, va, dma, rsc->len, rsc->da,
+					rproc_release_carveout, rsc->name);
+	if (!carveout)
+		goto free_carv;
 
 	list_add_tail(&carveout->node, &rproc->carveouts);
 
 	return 0;
 
+free_carv:
+	kfree(carveout);
 free_mapping:
 	kfree(mapping);
 dma_free:
 	dma_free_coherent(dev->parent, rsc->len, va, dma);
-free_carv:
-	kfree(carveout);
 	return ret;
 }
 
-/*
+/**
+ * rproc_mem_entry_init() - allocate and initialize rproc_mem_entry struct
+ * @dev: pointer on device struct
+ * @va: virtual address
+ * @dma: dma address
+ * @len: memory carveout length
+ * @da: device address
+ * @release: memory carveout function
+ * @name: carveout name
+ *
+ * This function allocates a rproc_mem_entry struct and fill it with parameters
+ * provided by client.
+ */
+struct rproc_mem_entry *rproc_mem_entry_init(struct device *dev,
+		void *va, dma_addr_t dma, int len, u32 da,
+		int (*release)(struct rproc *, struct rproc_mem_entry *),
+		const char *name, ...)
+{
+	struct rproc_mem_entry *mem;
+	va_list args;
+
+	mem = devm_kzalloc(dev, sizeof(*mem), GFP_KERNEL);
+	if (!mem)
+		return ERR_PTR(-ENOMEM);
+
+	mem->va = va;
+	mem->dma = dma;
+	mem->da = da;
+	mem->len = len;
+	mem->release = release;
+
+	va_start(args, name);
+	snprintf(mem->name, sizeof(mem->name), name, args);
+	va_end(args);
+
+	return mem;
+}
+EXPORT_SYMBOL(rproc_mem_entry_init);
+
+/**
  * A lookup table for resource handlers. The indices are defined in
  * enum fw_resource_type.
  */
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 2202c08..59b60f1 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -536,6 +536,11 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 int rproc_del(struct rproc *rproc);
 void rproc_free(struct rproc *rproc);
 
+struct rproc_mem_entry *rproc_mem_entry_init(struct device *dev,
+		void *va, dma_addr_t dma, int len, u32 da,
+		int (*release)(struct rproc *, struct rproc_mem_entry *),
+		const char *name, ...);
+
 int rproc_boot(struct rproc *rproc);
 void rproc_shutdown(struct rproc *rproc);
 void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type);
-- 
1.9.1

[PATCH v3 06/13] remoteproc: introduce rproc_add_carveout function

[Loic Pallardy]

This patch introduces a new API to allow platform driver to register
platform specific carveout regions.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 14 ++++++++++++++
 include/linux/remoteproc.h           |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 4c92b7d..91aa22b 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -758,6 +758,20 @@ static int rproc_handle_carveout(struct rproc *rproc,
 }
 
 /**
+ * rproc_add_carveout() - register an allocated carveout region
+ * @rproc: rproc handle
+ * @mem: memory entry to register
+ *
+ * This function registers specified memory entry in @rproc carveouts list.
+ * Specified carveout should have been allocated before registering.
+ */
+void rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry *mem)
+{
+	list_add_tail(&mem->node, &rproc->carveouts);
+}
+EXPORT_SYMBOL(rproc_add_carveout);
+
+/**
  * rproc_mem_entry_init() - allocate and initialize rproc_mem_entry struct
  * @dev: pointer on device struct
  * @va: virtual address
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 59b60f1..4aa30bd 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -536,6 +536,8 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 int rproc_del(struct rproc *rproc);
 void rproc_free(struct rproc *rproc);
 
+void rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry *mem);
+
 struct rproc_mem_entry *rproc_mem_entry_init(struct device *dev,
 		void *va, dma_addr_t dma, int len, u32 da,
 		int (*release)(struct rproc *, struct rproc_mem_entry *),
-- 
1.9.1

[PATCH v3 07/13] remoteproc: introduce rproc_find_carveout_by_name function

[Loic Pallardy]

This patch provides a new function to find a carveout according
to a name.
If match found, this function returns a pointer on the corresponding
carveout (rproc_mem_entry structure).

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 43 ++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 91aa22b..7a500cb 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -216,6 +216,49 @@ void *rproc_da_to_va(struct rproc *rproc, u64 da, int len)
 }
 EXPORT_SYMBOL(rproc_da_to_va);
 
+/**
+ * rproc_find_carveout_by_name() - lookup the carveout region by a name
+ * @rproc: handle of a remote processor
+ * @name,..: carveout name to find (standard printf format)
+ *
+ * Platform driver has the capability to register some pre-allacoted carveout
+ * (physically contiguous memory regions) before rproc firmware loading and
+ * associated resource table analysis. These regions may be dedicated memory
+ * regions internal to the coprocessor or specified DDR region with specific
+ * attributes
+ *
+ * This function is a helper function with which we can go over the
+ * allocated carveouts and return associated region characteristics like
+ * coprocessor address, length or processor virtual address.
+ *
+ * The function returns a valid pointer on carveout entry on success
+ * or NULL on failure.
+ */
+struct rproc_mem_entry *
+rproc_find_carveout_by_name(struct rproc *rproc, const char *name, ...)
+{
+	va_list args;
+	char _name[32];
+	struct rproc_mem_entry *carveout, *mem = NULL;
+
+	va_start(args, name);
+	snprintf(_name, sizeof(_name), name, args);
+	va_end(args);
+
+	if (!name)
+		return NULL;
+
+	list_for_each_entry(carveout, &rproc->carveouts, node) {
+		/* Compare carveout and requested names */
+		if (!strcmp(carveout->name, name)) {
+			mem = carveout;
+			break;
+		}
+	}
+
+	return mem;
+}
+
 int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 {
 	struct rproc *rproc = rvdev->rproc;
-- 
1.9.1

[PATCH v3 08/13] remoteproc: add prepare and unprepare ops

[Loic Pallardy]

On some SoC architecture, it is needed to enable HW like
clock, bus, regulator, memory region... before loading
co-processor firmware.

This patch introduces prepare and unprepare ops to execute
platform specific function before firmware loading and after
stop execution.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 20 +++++++++++++++++++-
 include/linux/remoteproc.h           |  4 ++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 7a500cb..0ebbc4f 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1058,12 +1058,22 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 		return ret;
 	}
 
+	/* Prepare rproc for firmware loading if needed */
+	if (rproc->ops->prepare) {
+		ret = rproc->ops->prepare(rproc);
+		if (ret) {
+			dev_err(dev, "can't prepare rproc %s: %d\n",
+				rproc->name, ret);
+			goto disable_iommu;
+		}
+	}
+
 	rproc->bootaddr = rproc_get_boot_addr(rproc, fw);
 
 	/* load resource table */
 	ret = rproc_load_rsc_table(rproc, fw);
 	if (ret)
-		goto disable_iommu;
+		goto unprepare_rproc;
 
 	/* reset max_notifyid */
 	rproc->max_notifyid = -1;
@@ -1086,6 +1096,10 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	kfree(rproc->cached_table);
 	rproc->cached_table = NULL;
 	rproc->table_ptr = NULL;
+unprepare_rproc:
+	/* release HW resources if needed */
+	if (rproc->ops->unprepare)
+		rproc->ops->unprepare(rproc);
 disable_iommu:
 	rproc_disable_iommu(rproc);
 	return ret;
@@ -1331,6 +1345,10 @@ void rproc_shutdown(struct rproc *rproc)
 	/* clean up all acquired resources */
 	rproc_resource_cleanup(rproc);
 
+	/* release HW resources if needed */
+	if (rproc->ops->unprepare)
+		rproc->ops->unprepare(rproc);
+
 	rproc_disable_iommu(rproc);
 
 	/* Free the copy of the resource table */
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 4aa30bd..dcfa601 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -333,6 +333,8 @@ struct rproc_mem_entry {
 
 /**
  * struct rproc_ops - platform-specific device handlers
+ * @prepare:	prepare device for code loading
+ * @unprepare:	unprepare device after stop
  * @start:	power on the device and boot it
  * @stop:	power off the device
  * @kick:	kick a virtqueue (virtqueue id given as a parameter)
@@ -345,6 +347,8 @@ struct rproc_mem_entry {
  * @get_boot_addr:	get boot address to entry point specified in firmware
  */
 struct rproc_ops {
+	int (*prepare)(struct rproc *rproc);
+	int (*unprepare)(struct rproc *rproc);
 	int (*start)(struct rproc *rproc);
 	int (*stop)(struct rproc *rproc);
 	void (*kick)(struct rproc *rproc, int vqid);
-- 
1.9.1

[PATCH v3 09/13] remoteproc: modify rproc_handle_carveout to support pre-registered region

[Loic Pallardy]

In current version rproc_handle_carveout function support only dynamic
region allocation.
This patch extends rproc_handle_carveout function to support pre-registered
region. Match is done on region name, then requested device address and
length are checked.
If no name match found, original allocation is used.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 49 +++++++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 0ebbc4f..49b28a0 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -679,7 +679,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
 				 struct fw_rsc_carveout *rsc,
 				 int offset, int avail)
 {
-	struct rproc_mem_entry *carveout, *mapping = NULL;
+	struct rproc_mem_entry *carveout, *mapping = NULL, *mem;
 	struct device *dev = &rproc->dev;
 	dma_addr_t dma;
 	void *va;
@@ -699,6 +699,51 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	dev_dbg(dev, "carveout rsc: name: %s, da 0x%x, pa 0x%x, len 0x%x, flags 0x%x\n",
 		rsc->name, rsc->da, rsc->pa, rsc->len, rsc->flags);
 
+	/* Check carveout rsc already part of a registered carveout */
+	/* Search by name */
+	mem = rproc_find_carveout_by_name(rproc, rsc->name);
+	if (mem) {
+		int delta = 0;
+
+		if (rsc->da != FW_RSC_ADDR_ANY) {
+			delta = rsc->da - mem->da;
+
+			/* Check requested resource belongs to registered carveout */
+			if (delta < 0) {
+				dev_err(dev->parent,
+					"Registered carveout doesn't fit da request\n");
+				return -ENOMEM;
+			}
+
+			if (delta + rsc->len > mem->len) {
+				dev_err(dev->parent,
+					"Registered carveout doesn't fit len request\n");
+				return -ENOMEM;
+			}
+		} else {
+			/* Check requested resource length */
+			if (rsc->len > mem->len) {
+				dev_err(dev->parent,
+					"Registered carveout doesn't fit len request\n");
+				return -ENOMEM;
+			}
+			/* Update resource da with registered resource value */
+			rsc->da = mem->da;
+		}
+
+		/*
+		 * Update resource pa
+		 * Use va if defined else dma to generate pa
+		 */
+		if (mem->va)
+			rsc->pa = rproc_va_to_pa(mem->va) + delta;
+		else
+			rsc->pa = (u32)mem->dma + delta;
+
+		return 0;
+	}
+
+	/* No registered carveout found, allocate a new one */
 	va = dma_alloc_coherent(dev->parent, rsc->len, &dma, GFP_KERNEL);
 	if (!va) {
 		dev_err(dev->parent,
@@ -761,6 +806,8 @@ static int rproc_handle_carveout(struct rproc *rproc,
 
 		dev_dbg(dev, "carveout mapped 0x%x to %pad\n",
 			rsc->da, &dma);
+	} else {
+		rsc->da = (u32)dma;
 	}
 
 	/*
-- 
1.9.1

[PATCH v3 10/13] remoteproc: modify vring allocation to support pre-registered region

[Loic Pallardy]

Current version of rproc_alloc_vring function supports only dynamic vring
allocation.
This patch proposes to manage vrings like memory carveouts to commonize
memory management codeand to rely on rproc_handle_carveout and
rproc_find_carveout_by_name functions for vrings allocation.

This patch sets vrings names to vdev"x"vring"y" as no name defined in
firmware resource table.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 58 +++++++++++++++++++++++++-----------
 include/linux/remoteproc.h           |  3 +-
 2 files changed, 42 insertions(+), 19 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 49b28a0..3041772 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -52,6 +52,10 @@ typedef int (*rproc_handle_resources_t)(struct rproc *rproc,
 typedef int (*rproc_handle_resource_t)(struct rproc *rproc,
 				 void *, int offset, int avail);
 
+static int rproc_handle_carveout(struct rproc *rproc,
+				 struct fw_rsc_carveout *rsc,
+				 int offset, int avail);
+
 /* Unique indices for remoteproc devices */
 static DEFINE_IDA(rproc_dev_index);
 
@@ -265,23 +269,45 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 	struct device *dev = &rproc->dev;
 	struct rproc_vring *rvring = &rvdev->vring[i];
 	struct fw_rsc_vdev *rsc;
-	dma_addr_t dma;
-	void *va;
 	int ret, size, notifyid;
+	struct fw_rsc_carveout rsc_carveout;
+	struct rproc_mem_entry *mem;
 
 	/* actual size of vring (in bytes) */
 	size = PAGE_ALIGN(vring_size(rvring->len, rvring->align));
 
-	/*
-	 * Allocate non-cacheable memory for the vring. In the future
-	 * this call will also configure the IOMMU for us
-	 */
-	va = dma_alloc_coherent(dev->parent, size, &dma, GFP_KERNEL);
-	if (!va) {
-		dev_err(dev->parent, "dma_alloc_coherent failed\n");
+	rsc = (void *)rproc->table_ptr + rvdev->rsc_offset;
+
+	/* Create virtual firmware carveout resource */
+	rsc_carveout.da = rsc->vring[i].da;
+	rsc_carveout.pa = FW_RSC_ADDR_ANY;
+	rsc_carveout.len = size;
+	rsc_carveout.flags = 0;
+	rsc_carveout.reserved = 0;
+	snprintf(rsc_carveout.name, sizeof(rsc_carveout.name), "vdev%dvring%d",
+		 rvdev->index, i);
+
+	/* Do vring carveout allocation */
+	ret = rproc_handle_carveout(rproc, &rsc_carveout, 0,
+				    sizeof(rsc_carveout));
+	if (ret) {
+		dev_err(dev->parent, "Failled to process vring carveout\n");
 		return -EINVAL;
 	}
 
+	/* Retrieve memory entry */
+	mem = rproc_find_carveout_by_name(rproc, rsc_carveout.name);
+	if (!mem) {
+		dev_err(dev->parent, "Failled to find vring carveout\n");
+		return -ENOMEM;
+	}
+
+	/* Verify carveout is well mapped */
+	if (!mem->va) {
+		dev_err(dev->parent, "Invalid vring carveout\n");
+		return -ENOMEM;
+	}
+
 	/*
 	 * Assign an rproc-wide unique index for this vring
 	 * TODO: assign a notifyid for rvdev updates as well
@@ -290,7 +316,6 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 	ret = idr_alloc(&rproc->notifyids, rvring, 0, 0, GFP_KERNEL);
 	if (ret < 0) {
 		dev_err(dev, "idr_alloc failed: %d\n", ret);
-		dma_free_coherent(dev->parent, size, va, dma);
 		return ret;
 	}
 	notifyid = ret;
@@ -300,10 +325,9 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 		rproc->max_notifyid = notifyid;
 
 	dev_dbg(dev, "vring%d: va %p dma %pad size 0x%x idr %d\n",
-		i, va, &dma, size, notifyid);
+		i, mem->va, &mem->dma, size, notifyid);
 
-	rvring->va = va;
-	rvring->dma = dma;
+	rvring->va = mem->va;
 	rvring->notifyid = notifyid;
 
 	/*
@@ -312,8 +336,8 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 	 * set up the iommu. In this case the device address (da) will
 	 * hold the physical address and not the device address.
 	 */
-	rsc = (void *)rproc->table_ptr + rvdev->rsc_offset;
-	rsc->vring[i].da = dma;
+	if (rsc->vring[i].da == FW_RSC_ADDR_ANY)
+		rsc->vring[i].da = mem->da;
 	rsc->vring[i].notifyid = notifyid;
 	return 0;
 }
@@ -345,12 +369,10 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 
 void rproc_free_vring(struct rproc_vring *rvring)
 {
-	int size = PAGE_ALIGN(vring_size(rvring->len, rvring->align));
 	struct rproc *rproc = rvring->rvdev->rproc;
 	int idx = rvring->rvdev->vring - rvring;
 	struct fw_rsc_vdev *rsc;
 
-	dma_free_coherent(rproc->dev.parent, size, rvring->va, rvring->dma);
 	idr_remove(&rproc->notifyids, rvring->notifyid);
 
 	/* reset resource entry info */
@@ -406,6 +428,7 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
 	struct device *dev = &rproc->dev;
 	struct rproc_vdev *rvdev;
 	int i, ret;
+	static int index;
 
 	/* make sure resource isn't truncated */
 	if (sizeof(*rsc) + rsc->num_of_vrings * sizeof(struct fw_rsc_vdev_vring)
@@ -437,6 +460,7 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
 
 	rvdev->id = rsc->id;
 	rvdev->rproc = rproc;
+	rvdev->index = index++;
 
 	/* parse the vrings */
 	for (i = 0; i < rsc->num_of_vrings; i++) {
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index dcfa601..0c9d0f6 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -486,7 +486,6 @@ struct rproc_subdev {
 /**
  * struct rproc_vring - remoteproc vring state
  * @va:	virtual address
- * @dma: dma address
  * @len: length, in bytes
  * @da: device address
  * @align: vring alignment
@@ -496,7 +495,6 @@ struct rproc_subdev {
  */
 struct rproc_vring {
 	void *va;
-	dma_addr_t dma;
 	int len;
 	u32 da;
 	u32 align;
@@ -522,6 +520,7 @@ struct rproc_vdev {
 	struct rproc_subdev subdev;
 
 	unsigned int id;
+	unsigned int index;
 	struct list_head node;
 	struct rproc *rproc;
 	struct virtio_device vdev;
-- 
1.9.1

[PATCH v3 11/13] remoteproc: create vdev subdevice with specific dma memory pool

[Loic Pallardy]

This patch creates a dedicated vdev subdevice for each vdev declared
in firmware resource table and associates carveout named "vdev%dbuffer"
(with %d vdev index in resource table) if any as dma coherent memory pool.

Then vdev subdevice is used as parent for virtio device.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c   | 40 ++++++++++++++++++++++++++++++++++
 drivers/remoteproc/remoteproc_virtio.c |  2 +-
 include/linux/remoteproc.h             |  1 +
 3 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 3041772..61c9927 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -427,8 +427,10 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
 {
 	struct device *dev = &rproc->dev;
 	struct rproc_vdev *rvdev;
+	struct rproc_mem_entry *carveout;
 	int i, ret;
 	static int index;
+	char name[16];
 
 	/* make sure resource isn't truncated */
 	if (sizeof(*rsc) + rsc->num_of_vrings * sizeof(struct fw_rsc_vdev_vring)
@@ -479,6 +481,41 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
 			goto unwind_vring_allocations;
 	}
 
+	/* Initialise vdev subdevice */
+	snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index);
+	rvdev->dev.parent = rproc->dev.parent;
+	dev_set_name(&rvdev->dev, "%s#%s", dev_name(rvdev->dev.parent), name);
+	dev_set_drvdata(&rvdev->dev, rvdev);
+	dma_set_coherent_mask(&rvdev->dev, DMA_BIT_MASK(32));
+
+	ret = device_register(&rvdev->dev);
+	if (ret)
+		goto unwind_vring_allocations;
+
+	/* Try to find dedicated vdev buffer carveout */
+	carveout = rproc_find_carveout_by_name(rproc, name);
+
+	if (carveout) {
+		phys_addr_t pa;
+
+		if (carveout->va) {
+			dev_warn(dev, "vdev %d buffer carveout already mapped\n",
+				 rvdev->index);
+			pa = rproc_va_to_pa(carveout->va);
+		} else {
+			/* Use dma address as carveout no memmapped yet */
+			pa = (phys_addr_t)carveout->dma;
+		}
+
+		/* Associate vdev buffer memory pool to vdev subdevice */
+		ret = dmam_declare_coherent_memory(&rvdev->dev, pa,
+						   carveout->da,
+						   carveout->len,
+						   DMA_MEMORY_EXCLUSIVE);
+		if (ret < 0)
+			goto unregister_device;
+	}
+
 	list_add_tail(&rvdev->node, &rproc->rvdevs);
 
 	rproc_add_subdev(rproc, &rvdev->subdev,
@@ -486,6 +523,9 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
 
 	return 0;
 
+unregister_device:
+	put_device(&rvdev->dev);
+
 unwind_vring_allocations:
 	for (i--; i >= 0; i--)
 		rproc_free_vring(&rvdev->vring[i]);
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index b0633fd..f80014f 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -303,7 +303,7 @@ static void rproc_virtio_dev_release(struct device *dev)
 int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
 {
 	struct rproc *rproc = rvdev->rproc;
-	struct device *dev = &rproc->dev;
+	struct device *dev = &rvdev->dev;
 	struct virtio_device *vdev = &rvdev->vdev;
 	int ret;
 
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 0c9d0f6..dfc44af7 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -518,6 +518,7 @@ struct rproc_vdev {
 	struct kref refcount;
 
 	struct rproc_subdev subdev;
+	struct device dev;
 
 	unsigned int id;
 	unsigned int index;
-- 
1.9.1

[PATCH v3 12/13] rpmsg: virtio: allocate buffer from parent

[Loic Pallardy]

Remoteproc is now capable to create one specific sub-device per
virtio link to associate a dedicated memory pool.
This implies to change device used by virtio_rpmsg for
buffer allocation from grand-parent to parent.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 82b8300..6dd25c6 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -920,7 +920,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	total_buf_space = vrp->num_bufs * vrp->buf_size;
 
 	/* allocate coherent memory for the buffers */
-	bufs_va = dma_alloc_coherent(vdev->dev.parent->parent,
+	bufs_va = dma_alloc_coherent(vdev->dev.parent,
 				     total_buf_space, &vrp->bufs_dma,
 				     GFP_KERNEL);
 	if (!bufs_va) {
-- 
1.9.1

[PATCH v3 13/13] remoteproc: st: add reserved memory support

[Loic Pallardy]

ST remote processor needs some specified memory regions for
firmware and IPC.
Memory regions are defined as reserved memory and should
be registered in remoteproc core thanks to rproc_add_carveout
function before rproc_start. For this, st rproc driver implements
prepare ops.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/st_remoteproc.c | 68 +++++++++++++++++++++++++++++++++-----
 1 file changed, 60 insertions(+), 8 deletions(-)

diff --git a/drivers/remoteproc/st_remoteproc.c b/drivers/remoteproc/st_remoteproc.c
index aacef0e..72a19a0 100644
--- a/drivers/remoteproc/st_remoteproc.c
+++ b/drivers/remoteproc/st_remoteproc.c
@@ -19,6 +19,7 @@
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/of_device.h>
 #include <linux/of_reserved_mem.h>
 #include <linux/platform_device.h>
@@ -91,6 +92,64 @@ static void st_rproc_kick(struct rproc *rproc, int vqid)
 		dev_err(dev, "failed to send message via mbox: %d\n", ret);
 }
 
+static int st_rproc_mem_release(struct rproc *rproc,
+				struct rproc_mem_entry *mem)
+{
+	struct device *dev = rproc->dev.parent;
+
+	devm_iounmap(dev, mem->va);
+
+	return 0;
+}
+
+static int st_rproc_prepare(struct rproc *rproc)
+{
+	struct device *dev = rproc->dev.parent;
+	struct device_node *np = dev->of_node;
+	struct rproc_mem_entry *mem;
+	void *va;
+	struct reserved_mem *rmem;
+	struct of_phandle_iterator it;
+	int err;
+
+	of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
+	while ((err = of_phandle_iterator_next(&it)) == 0) {
+		va = NULL;
+		rmem = of_reserved_mem_lookup(it.node);
+
+		/*  No need to map vdev buffer */
+		if (strcmp(it.node->name, "vdev0buffer")) {
+			va = devm_ioremap_wc(dev, rmem->base, rmem->size);
+			if (!va) {
+				dev_err(dev, "Unable to map memory region: %pa+%zx\n",
+					&rmem->base, rmem->size);
+				return -EBUSY;
+			}
+
+			/* Register memory region */
+			mem = rproc_mem_entry_init(dev, va,
+						   (dma_addr_t)rmem->base,
+						   rmem->size, rmem->base,
+						   st_rproc_mem_release,
+						   it.node->name);
+		} else {
+			/* Register vdev buffer */
+			mem = rproc_mem_entry_init(dev, NULL,
+						   (dma_addr_t)rmem->base,
+						   rmem->size, rmem->base,
+						   NULL,
+						   it.node->name);
+		}
+
+		if (!mem)
+			return -ENOMEM;
+
+		rproc_add_carveout(rproc, mem);
+	}
+
+	return 0;
+}
+
 static int st_rproc_start(struct rproc *rproc)
 {
 	struct st_rproc *ddata = rproc->priv;
@@ -161,6 +220,7 @@ static int st_rproc_stop(struct rproc *rproc)
 	.kick		= st_rproc_kick,
 	.start		= st_rproc_start,
 	.stop		= st_rproc_stop,
+	.prepare	= st_rproc_prepare,
 };
 
 /*
@@ -254,12 +314,6 @@ static int st_rproc_parse_dt(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	err = of_reserved_mem_device_init(dev);
-	if (err) {
-		dev_err(dev, "Failed to obtain shared memory\n");
-		return err;
-	}
-
 	err = clk_prepare(ddata->clk);
 	if (err)
 		dev_err(dev, "failed to get clock\n");
@@ -387,8 +441,6 @@ static int st_rproc_remove(struct platform_device *pdev)
 
 	clk_disable_unprepare(ddata->clk);
 
-	of_reserved_mem_device_release(&pdev->dev);
-
 	for (i = 0; i < ST_RPROC_MAX_VRING * MBOX_MAX; i++)
 		mbox_free_channel(ddata->mbox_chan[i]);
 
-- 
1.9.1

RE: [PATCH v3 00/13] remoteproc: add fixed memory region support

[Loic PALLARDY]

Hi Bjorn, 

Just a gentle ping...
As discussed during Linaro connect, I'll appreciate if you can share me your comments...

Regards,
Loic

> -----Original Message-----
> From: Loic PALLARDY
> Sent: Thursday, March 01, 2018 5:24 PM
> To: bjorn.andersson@linaro.org; ohad@wizery.com
> Cc: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org;
> Arnaud POULIQUEN <arnaud.pouliquen@st.com>;
> benjamin.gaignard@linaro.org; Loic PALLARDY <loic.pallardy@st.com>
> Subject: [PATCH v3 00/13] remoteproc: add fixed memory region support
> 
> The aim of the series is to implement carveout memory management as
> discussed during OpenAMP weekly call and defined in proposed document
> [1]
> 
> This first series focus only on adding support of the different types of
> carveout memories (dynamic, fixed, platform driver depend...).
> 64bit resource table will be addressed in a next series.
> 
> [1]: http://openamp.github.io/docs/mca/coprocessor-memory-definition-
> v6.pdf
> 
> ---
> Changes sine V2:
> Reshuffle the series to:
> - Take into account Bjorn's comments.
> - Add patch to check consistency between carveout resource request and
> IOMMU
>   support.
> - Introduce platform specific prepare and unprepare ops to enable HW like
>   clock, bus, regulator, memory region... before loading co-processor
> firmware.
> - Rely on memory carveout management for all remoteproc memory
> allocations.
> - Lookup pre-registered carveout by name first.
> - Create a subdevice for each vdev declared in firmware resource table that
>   will be used by virtio based driver to retrieve specific memory pool.
> 
> This series takes some assumptions for carveout names associated to vdev:
> - For vring: "vdev%xvring%x" with vdev index from resource table and vring
> index
>   in vdev.
> - For vdev buffer: "vdev%xbuffer" with vdev index from resource table
> 
> This will be changed in the future, adding names field in vdev resource in
> next resource table version.
> 
> 
> Changes since V1:
> - Minor corrections on first 7 patches (error management)
> - Add "memory device" support on the top of first 7 patches.
>   Goal is to answer use case reported during OpenAMP weekly discussion:
>   - "Be able to specify memory region for vring and buffer allocation, even
>     if no specific request defined in firmware resource table."
>   Patches offer the capability to create a "memory device" associated to a
>   carveout with a dedicated DMA memory pool. Different resource handlers
> are
>   modified to look-up for specific carveout by name. If match found and
> associated
>   "memory device" present, device is used instead of rproc platform device
> for
>   allocation.
> 
> 
> Loic Pallardy (13):
>   remoteproc: configure IOMMU only if device address requested
>   remoteproc: add rproc_va_to_pa function
>   remoteproc: add release ops in rproc_mem_entry struct
>   remoteproc: add name in rproc_mem_entry struct
>   remoteproc: add helper function to allocate and init rproc_mem_entry
>     struct
>   remoteproc: introduce rproc_add_carveout function
>   remoteproc: introduce rproc_find_carveout_by_name function
>   remoteproc: add prepare and unprepare ops
>   remoteproc: modify rproc_handle_carveout to support pre-registered
>     region
>   remoteproc: modify vring allocation to support pre-registered region
>   remoteproc: create vdev subdevice with specific dma memory pool
>   rpmsg: virtio: allocate buffer from parent
>   remoteproc: st: add reserved memory support
> 
>  drivers/remoteproc/remoteproc_core.c    | 332
> ++++++++++++++++++++++++++++----
>  drivers/remoteproc/remoteproc_debugfs.c |   1 +
>  drivers/remoteproc/remoteproc_virtio.c  |   2 +-
>  drivers/remoteproc/st_remoteproc.c      |  68 ++++++-
>  drivers/rpmsg/virtio_rpmsg_bus.c        |   2 +-
>  include/linux/remoteproc.h              |  22 ++-
>  6 files changed, 379 insertions(+), 48 deletions(-)
> 
> --
> 1.9.1

Re: [PATCH v3 02/13] remoteproc: add rproc_va_to_pa function

[Bjorn Andersson]

On Thu 01 Mar 08:23 PST 2018, Loic Pallardy wrote:

> This new function translates CPU virtual address in
> CPU physical one according to virtual address location.
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>

Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  drivers/remoteproc/remoteproc_core.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 56b33e4..eb1b779 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -139,6 +139,22 @@ static void rproc_disable_iommu(struct rproc *rproc)
>  	iommu_domain_free(domain);
>  }
>  
> +static phys_addr_t rproc_va_to_pa(void *cpu_addr)
> +{
> +	/*
> +	 * Return physical address according to virtual address location
> +	 * - in vmalloc: if region ioremapped or defined as dma_alloc_coherent
> +	 * - in kernel: if region allocated in generic dma memory pool
> +	 */
> +	if (is_vmalloc_addr(cpu_addr)) {
> +		return page_to_phys(vmalloc_to_page(cpu_addr)) +
> +				    offset_in_page(cpu_addr);
> +	}
> +
> +	WARN_ON(!virt_addr_valid(cpu_addr));
> +	return virt_to_phys(cpu_addr);
> +}
> +
>  /**
>   * rproc_da_to_va() - lookup the kernel virtual address for a remoteproc address
>   * @rproc: handle of a remote processor
> @@ -708,7 +724,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
>  	 * In this case, the device address and the physical address
>  	 * are the same.
>  	 */
> -	rsc->pa = dma;
> +	rsc->pa = (u32)rproc_va_to_pa(va);
>  
>  	carveout->va = va;
>  	carveout->len = rsc->len;
> -- 
> 1.9.1
> 

Re: [PATCH v3 04/13] remoteproc: add name in rproc_mem_entry struct

[Bjorn Andersson]

On Thu 01 Mar 08:23 PST 2018, Loic Pallardy wrote:

> Add name field in struct rproc_mem_entry.
> This new field will be used to match memory area
> requested in resource table with pre-registered carveout.
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>

Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  drivers/remoteproc/remoteproc_core.c    | 1 +
>  drivers/remoteproc/remoteproc_debugfs.c | 1 +
>  include/linux/remoteproc.h              | 2 ++
>  3 files changed, 4 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 5f11536..f73a0cf 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -749,6 +749,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
>  	carveout->dma = dma;
>  	carveout->da = rsc->da;
>  	carveout->release = rproc_release_carveout;
> +	strlcpy(carveout->name, rsc->name, sizeof(carveout->name));
>  
>  	list_add_tail(&carveout->node, &rproc->carveouts);
>  
> diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
> index a204883..fc0e570 100644
> --- a/drivers/remoteproc/remoteproc_debugfs.c
> +++ b/drivers/remoteproc/remoteproc_debugfs.c
> @@ -260,6 +260,7 @@ static int rproc_carveouts_show(struct seq_file *seq, void *p)
>  
>  	list_for_each_entry(carveout, &rproc->carveouts, node) {
>  		seq_puts(seq, "Carveout memory entry:\n");
> +		seq_printf(seq, "\tName: %s\n", carveout->name);
>  		seq_printf(seq, "\tVirtual address: %p\n", carveout->va);
>  		seq_printf(seq, "\tDMA address: %pad\n", &carveout->dma);
>  		seq_printf(seq, "\tDevice address: 0x%x\n", carveout->da);
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index dedc138..2202c08 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -315,6 +315,7 @@ struct fw_rsc_vdev {
>   * @da: device address
>   * @release: release associated memory
>   * @priv: associated data
> + * @name: associated memory region name (optional)
>   * @node: list node
>   */
>  struct rproc_mem_entry {
> @@ -323,6 +324,7 @@ struct rproc_mem_entry {
>  	int len;
>  	u32 da;
>  	void *priv;
> +	char name[32];
>  	struct list_head node;
>  	int (*release)(struct rproc *rproc, struct rproc_mem_entry *mem);
>  };
> -- 
> 1.9.1
> 

Re: [PATCH v3 07/13] remoteproc: introduce rproc_find_carveout_by_name function

[Bjorn Andersson]

On Thu 01 Mar 08:23 PST 2018, Loic Pallardy wrote:

> This patch provides a new function to find a carveout according
> to a name.
> If match found, this function returns a pointer on the corresponding
> carveout (rproc_mem_entry structure).
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> ---
>  drivers/remoteproc/remoteproc_core.c | 43 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 91aa22b..7a500cb 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -216,6 +216,49 @@ void *rproc_da_to_va(struct rproc *rproc, u64 da, int len)
>  }
>  EXPORT_SYMBOL(rproc_da_to_va);
>  
> +/**
> + * rproc_find_carveout_by_name() - lookup the carveout region by a name
> + * @rproc: handle of a remote processor
> + * @name,..: carveout name to find (standard printf format)
> + *
> + * Platform driver has the capability to register some pre-allacoted carveout
> + * (physically contiguous memory regions) before rproc firmware loading and
> + * associated resource table analysis. These regions may be dedicated memory
> + * regions internal to the coprocessor or specified DDR region with specific
> + * attributes
> + *
> + * This function is a helper function with which we can go over the
> + * allocated carveouts and return associated region characteristics like
> + * coprocessor address, length or processor virtual address.
> + *
> + * The function returns a valid pointer on carveout entry on success
> + * or NULL on failure.

The kerneldoc format for describing the return value is

 * Return: description

> + */
> +struct rproc_mem_entry *
> +rproc_find_carveout_by_name(struct rproc *rproc, const char *name, ...)
> +{
> +	va_list args;
> +	char _name[32];
> +	struct rproc_mem_entry *carveout, *mem = NULL;
> +
> +	va_start(args, name);
> +	snprintf(_name, sizeof(_name), name, args);
> +	va_end(args);
> +
> +	if (!name)
> +		return NULL;

If you would like for it to actually be valid to pass NULL for name,
then I think you should move this check to the top of the function.

But I suspect that you're looking for (name[0] == '\0'), to check if we
where passed an empty string, e.g. from a resource table without
resource names.

> +
> +	list_for_each_entry(carveout, &rproc->carveouts, node) {
> +		/* Compare carveout and requested names */
> +		if (!strcmp(carveout->name, name)) {
> +			mem = carveout;
> +			break;

Just return carveout when you find it.

> +		}
> +	}
> +
> +	return mem;
> +}
> +

Regards,
Bjorn

Re: [PATCH v3 09/13] remoteproc: modify rproc_handle_carveout to support pre-registered region

[Bjorn Andersson]

On Thu 01 Mar 08:23 PST 2018, Loic Pallardy wrote:

> In current version rproc_handle_carveout function support only dynamic
> region allocation.
> This patch extends rproc_handle_carveout function to support pre-registered
> region. Match is done on region name, then requested device address and
> length are checked.
> If no name match found, original allocation is used.
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> ---
>  drivers/remoteproc/remoteproc_core.c | 49 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 0ebbc4f..49b28a0 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -679,7 +679,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
>  				 struct fw_rsc_carveout *rsc,
>  				 int offset, int avail)
>  {
> -	struct rproc_mem_entry *carveout, *mapping = NULL;
> +	struct rproc_mem_entry *carveout, *mapping = NULL, *mem;
>  	struct device *dev = &rproc->dev;
>  	dma_addr_t dma;
>  	void *va;
> @@ -699,6 +699,51 @@ static int rproc_handle_carveout(struct rproc *rproc,
>  	dev_dbg(dev, "carveout rsc: name: %s, da 0x%x, pa 0x%x, len 0x%x, flags 0x%x\n",
>  		rsc->name, rsc->da, rsc->pa, rsc->len, rsc->flags);
>  
> +	/* Check carveout rsc already part of a registered carveout */
> +	/* Search by name */
> +	mem = rproc_find_carveout_by_name(rproc, rsc->name);
> +	if (mem) {

I don't fancy the concept of "check if there is another registered
carveout and if so update this carveouts data based on that one and then
skip the bottom half of this function but keep them both on the
carveouts list".

It's unfortunately not very easy to follow and it doesn't allow us to
reuse the carveout-handler for allocations in remoteprocs without a
resource table.

How about splitting the handling of the resource table in two parts; one
that creates or updates a carveout on the carvouts list and a second
part that runs through all carveouts and "allocate" (similar to your
specific release function) them.


The first part of this function would then attempt to find a carveout
entry matching the one we're trying to "handle";

* if one is found we check if it's compatible (as you do here), update a
  rsc_offset (as we do with vrings) and return.

* if no match is found we create a new rproc_mem_entry, fill it out
  based on the fw_rsc_carveout information and stash it at the end of
  the carveouts list.

We do the same in the other resource handlers (just allocate entries
onto the lists).


As that is done the second step is to loop over all carveouts, devmem,
trace and vdev resources and actually "allocate" the resources, by
calling a "alloc" function pointer next to your proposed release one.

For memremap() memory this could be as simple as filling out the
resource table at the associated rsc_offset or simply doing the
memremap().

The default alloc (filled out in step 1, if not already specified) would
be what's today found in rproc_handle_carveout().


This allows carveout resources not specified in the resource table to be
allocated as well. Which comes in handy for the handling of vdev
resources:

In rproc_parse_vdev() we do a similar operation to the parser of a
fw_rsc_carveout and try to find an existing carveout by name and if not
create a new one on the list.

As the actual allocation of carveouts is done before the "allocation" of
vrings there will be an allocated carveout ready when we hit
rproc_alloc_vring() - and we don't care if it came from
dma_alloc_coherent() or a driver defined region.


Does this sound reasonable?

Regards,
Bjorn

Re: [PATCH v3 08/13] remoteproc: add prepare and unprepare ops

[Bjorn Andersson]

On Thu 01 Mar 08:23 PST 2018, Loic Pallardy wrote:

> On some SoC architecture, it is needed to enable HW like
> clock, bus, regulator, memory region... before loading
> co-processor firmware.
> 
> This patch introduces prepare and unprepare ops to execute
> platform specific function before firmware loading and after
> stop execution.
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> ---
>  drivers/remoteproc/remoteproc_core.c | 20 +++++++++++++++++++-
>  include/linux/remoteproc.h           |  4 ++++
>  2 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 7a500cb..0ebbc4f 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1058,12 +1058,22 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>  		return ret;
>  	}
>  
> +	/* Prepare rproc for firmware loading if needed */
> +	if (rproc->ops->prepare) {
> +		ret = rproc->ops->prepare(rproc);
> +		if (ret) {
> +			dev_err(dev, "can't prepare rproc %s: %d\n",
> +				rproc->name, ret);
> +			goto disable_iommu;
> +		}
> +	}

We do allow drivers to implement custom versions of parse_fw() - and
they can call the resource-table-parse-fw from the custom function.

So with the proposed refactoring in patch 9 I would like for parse_fw()
to call back into the core to fill out the resource lists and then
before jumping to rproc_start() we loop over the allocator functions.

> +
>  	rproc->bootaddr = rproc_get_boot_addr(rproc, fw);
>  
>  	/* load resource table */
>  	ret = rproc_load_rsc_table(rproc, fw);
>  	if (ret)
> -		goto disable_iommu;
> +		goto unprepare_rproc;
>  
>  	/* reset max_notifyid */
>  	rproc->max_notifyid = -1;
> @@ -1086,6 +1096,10 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>  	kfree(rproc->cached_table);
>  	rproc->cached_table = NULL;
>  	rproc->table_ptr = NULL;
> +unprepare_rproc:
> +	/* release HW resources if needed */
> +	if (rproc->ops->unprepare)
> +		rproc->ops->unprepare(rproc);
>  disable_iommu:
>  	rproc_disable_iommu(rproc);
>  	return ret;
> @@ -1331,6 +1345,10 @@ void rproc_shutdown(struct rproc *rproc)
>  	/* clean up all acquired resources */
>  	rproc_resource_cleanup(rproc);
>  
> +	/* release HW resources if needed */
> +	if (rproc->ops->unprepare)
> +		rproc->ops->unprepare(rproc);

And this would then be handled by the rproc_resource_cleanup() function,
looping over all resources and calling release().

Regards,
Bjorn

Re: [PATCH v3 03/13] remoteproc: add release ops in rproc_mem_entry struct

[Bjorn Andersson]

On Thu 01 Mar 08:23 PST 2018, Loic Pallardy wrote:

> Memory entry could be allocated in different ways (ioremap,
> dma_alloc_coherent, internal RAM allocator...).
> This patch introduces a release ops in rproc_mem_entry structure
> to associate dedicated release mechanism to each memory entry descriptor
> in order to keep remoteproc core generic.
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>

Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  drivers/remoteproc/remoteproc_core.c | 23 +++++++++++++++++++++--
>  include/linux/remoteproc.h           |  5 ++++-
>  2 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index eb1b779..5f11536 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -597,6 +597,24 @@ static int rproc_handle_devmem(struct rproc *rproc, struct fw_rsc_devmem *rsc,
>  }
>  
>  /**
> + * rproc_release_carveout() - release acquired carveout
> + * @rproc: rproc handle
> + * @mem: the memory entry to release
> + *
> + * This function releases specified memory entry @mem allocated via
> + * dma_alloc_coherent() function by @rproc.
> + */
> +static int rproc_release_carveout(struct rproc *rproc,
> +				  struct rproc_mem_entry *mem)
> +{
> +	struct device *dev = &rproc->dev;
> +
> +	/* clean up carveout allocations */
> +	dma_free_coherent(dev->parent, mem->len, mem->va, mem->dma);
> +	return 0;
> +}
> +
> +/**
>   * rproc_handle_carveout() - handle phys contig memory allocation requests
>   * @rproc: rproc handle
>   * @rsc: the resource entry
> @@ -730,6 +748,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
>  	carveout->len = rsc->len;
>  	carveout->dma = dma;
>  	carveout->da = rsc->da;
> +	carveout->release = rproc_release_carveout;
>  
>  	list_add_tail(&carveout->node, &rproc->carveouts);
>  
> @@ -863,8 +882,8 @@ static void rproc_resource_cleanup(struct rproc *rproc)
>  
>  	/* clean up carveout allocations */
>  	list_for_each_entry_safe(entry, tmp, &rproc->carveouts, node) {
> -		dma_free_coherent(dev->parent, entry->len, entry->va,
> -				  entry->dma);
> +		if (entry->release)
> +			entry->release(rproc, entry);
>  		list_del(&entry->node);
>  		kfree(entry);
>  	}
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 728d421..dedc138 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -305,12 +305,15 @@ struct fw_rsc_vdev {
>  	struct fw_rsc_vdev_vring vring[0];
>  } __packed;
>  
> +struct rproc;
> +
>  /**
>   * struct rproc_mem_entry - memory entry descriptor
>   * @va:	virtual address
>   * @dma: dma address
>   * @len: length, in bytes
>   * @da: device address
> + * @release: release associated memory
>   * @priv: associated data
>   * @node: list node
>   */
> @@ -321,9 +324,9 @@ struct rproc_mem_entry {
>  	u32 da;
>  	void *priv;
>  	struct list_head node;
> +	int (*release)(struct rproc *rproc, struct rproc_mem_entry *mem);
>  };
>  
> -struct rproc;
>  struct firmware;
>  
>  /**
> -- 
> 1.9.1
> 

Re: [PATCH v3 06/13] remoteproc: introduce rproc_add_carveout function

[Bjorn Andersson]

On Thu 01 Mar 08:23 PST 2018, Loic Pallardy wrote:

> This patch introduces a new API to allow platform driver to register
> platform specific carveout regions.
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>

Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  drivers/remoteproc/remoteproc_core.c | 14 ++++++++++++++
>  include/linux/remoteproc.h           |  2 ++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 4c92b7d..91aa22b 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -758,6 +758,20 @@ static int rproc_handle_carveout(struct rproc *rproc,
>  }
>  
>  /**
> + * rproc_add_carveout() - register an allocated carveout region
> + * @rproc: rproc handle
> + * @mem: memory entry to register
> + *
> + * This function registers specified memory entry in @rproc carveouts list.
> + * Specified carveout should have been allocated before registering.
> + */
> +void rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry *mem)
> +{
> +	list_add_tail(&mem->node, &rproc->carveouts);
> +}
> +EXPORT_SYMBOL(rproc_add_carveout);
> +
> +/**
>   * rproc_mem_entry_init() - allocate and initialize rproc_mem_entry struct
>   * @dev: pointer on device struct
>   * @va: virtual address
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 59b60f1..4aa30bd 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -536,6 +536,8 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>  int rproc_del(struct rproc *rproc);
>  void rproc_free(struct rproc *rproc);
>  
> +void rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry *mem);
> +
>  struct rproc_mem_entry *rproc_mem_entry_init(struct device *dev,
>  		void *va, dma_addr_t dma, int len, u32 da,
>  		int (*release)(struct rproc *, struct rproc_mem_entry *),
> -- 
> 1.9.1
> 

Re: [PATCH v3 10/13] remoteproc: modify vring allocation to support pre-registered region

[Bjorn Andersson]

On Thu 01 Mar 08:23 PST 2018, Loic Pallardy wrote:
[..]
> @@ -265,23 +269,45 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
>  	struct device *dev = &rproc->dev;
>  	struct rproc_vring *rvring = &rvdev->vring[i];
>  	struct fw_rsc_vdev *rsc;
> -	dma_addr_t dma;
> -	void *va;
>  	int ret, size, notifyid;
> +	struct fw_rsc_carveout rsc_carveout;
> +	struct rproc_mem_entry *mem;
>  
>  	/* actual size of vring (in bytes) */
>  	size = PAGE_ALIGN(vring_size(rvring->len, rvring->align));
>  
> -	/*
> -	 * Allocate non-cacheable memory for the vring. In the future
> -	 * this call will also configure the IOMMU for us
> -	 */
> -	va = dma_alloc_coherent(dev->parent, size, &dma, GFP_KERNEL);
> -	if (!va) {
> -		dev_err(dev->parent, "dma_alloc_coherent failed\n");
> +	rsc = (void *)rproc->table_ptr + rvdev->rsc_offset;
> +
> +	/* Create virtual firmware carveout resource */
> +	rsc_carveout.da = rsc->vring[i].da;
> +	rsc_carveout.pa = FW_RSC_ADDR_ANY;
> +	rsc_carveout.len = size;
> +	rsc_carveout.flags = 0;
> +	rsc_carveout.reserved = 0;
> +	snprintf(rsc_carveout.name, sizeof(rsc_carveout.name), "vdev%dvring%d",
> +		 rvdev->index, i);
[..]
> @@ -437,6 +460,7 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
>  
>  	rvdev->id = rsc->id;
>  	rvdev->rproc = rproc;
> +	rvdev->index = index++;

This index isn't deterministic over multiple remoteproc instances and
multiple restarts of the remoteproc. It probably needs to be based
generated based on the ordering in the resource table.

Regards,
Bjorn

Re: [PATCH v3 11/13] remoteproc: create vdev subdevice with specific dma memory pool

[Bjorn Andersson]

On Thu 01 Mar 08:23 PST 2018, Loic Pallardy wrote:
> @@ -479,6 +481,41 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
>  			goto unwind_vring_allocations;
>  	}
>  
> +	/* Initialise vdev subdevice */
> +	snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index);
> +	rvdev->dev.parent = rproc->dev.parent;
> +	dev_set_name(&rvdev->dev, "%s#%s", dev_name(rvdev->dev.parent), name);
> +	dev_set_drvdata(&rvdev->dev, rvdev);
> +	dma_set_coherent_mask(&rvdev->dev, DMA_BIT_MASK(32));
> +
> +	ret = device_register(&rvdev->dev);
> +	if (ret)
> +		goto unwind_vring_allocations;
> +
> +	/* Try to find dedicated vdev buffer carveout */
> +	carveout = rproc_find_carveout_by_name(rproc, name);
> +
> +	if (carveout) {
> +		phys_addr_t pa;
> +
> +		if (carveout->va) {
> +			dev_warn(dev, "vdev %d buffer carveout already mapped\n",
> +				 rvdev->index);
> +			pa = rproc_va_to_pa(carveout->va);
> +		} else {
> +			/* Use dma address as carveout no memmapped yet */
> +			pa = (phys_addr_t)carveout->dma;
> +		}
> +
> +		/* Associate vdev buffer memory pool to vdev subdevice */
> +		ret = dmam_declare_coherent_memory(&rvdev->dev, pa,
> +						   carveout->da,
> +						   carveout->len,
> +						   DMA_MEMORY_EXCLUSIVE);
> +		if (ret < 0)
> +			goto unregister_device;
> +	}
> +

So with this there will be one more device between rproc->dev and the
virtio dev, for the sake of memory management. So e.g. a rpmsg device
will still need to allocate memory from dev->parent->parent; which now
possibly has a specific dma_mem.

Is it not possible to assign the memory to the vdev->dev and allow the
virtio devices can allocate memory from their parent device?

Regards,
Bjorn

RE: [PATCH v3 07/13] remoteproc: introduce rproc_find_carveout_by_name function

[Loic PALLARDY]

Hi Bjorn,

Thanks for the review

> -----Original Message-----
> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]
> Sent: Thursday, May 10, 2018 2:19 AM
> To: Loic PALLARDY <loic.pallardy@st.com>
> Cc: ohad@wizery.com; linux-remoteproc@vger.kernel.org; linux-
> kernel@vger.kernel.org; Arnaud POULIQUEN <arnaud.pouliquen@st.com>;
> benjamin.gaignard@linaro.org
> Subject: Re: [PATCH v3 07/13] remoteproc: introduce
> rproc_find_carveout_by_name function
> 
> On Thu 01 Mar 08:23 PST 2018, Loic Pallardy wrote:
> 
> > This patch provides a new function to find a carveout according
> > to a name.
> > If match found, this function returns a pointer on the corresponding
> > carveout (rproc_mem_entry structure).
> >
> > Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> > ---
> >  drivers/remoteproc/remoteproc_core.c | 43
> ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 43 insertions(+)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> > index 91aa22b..7a500cb 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -216,6 +216,49 @@ void *rproc_da_to_va(struct rproc *rproc, u64 da,
> int len)
> >  }
> >  EXPORT_SYMBOL(rproc_da_to_va);
> >
> > +/**
> > + * rproc_find_carveout_by_name() - lookup the carveout region by a
> name
> > + * @rproc: handle of a remote processor
> > + * @name,..: carveout name to find (standard printf format)
> > + *
> > + * Platform driver has the capability to register some pre-allacoted
> carveout
> > + * (physically contiguous memory regions) before rproc firmware loading
> and
> > + * associated resource table analysis. These regions may be dedicated
> memory
> > + * regions internal to the coprocessor or specified DDR region with specific
> > + * attributes
> > + *
> > + * This function is a helper function with which we can go over the
> > + * allocated carveouts and return associated region characteristics like
> > + * coprocessor address, length or processor virtual address.
> > + *
> > + * The function returns a valid pointer on carveout entry on success
> > + * or NULL on failure.
> 
> The kerneldoc format for describing the return value is
> 
>  * Return: description
Yes I'll update

> 
> > + */
> > +struct rproc_mem_entry *
> > +rproc_find_carveout_by_name(struct rproc *rproc, const char *name, ...)
> > +{
> > +	va_list args;
> > +	char _name[32];
> > +	struct rproc_mem_entry *carveout, *mem = NULL;
> > +
> > +	va_start(args, name);
> > +	snprintf(_name, sizeof(_name), name, args);
> > +	va_end(args);
> > +
> > +	if (!name)
> > +		return NULL;
> 
> If you would like for it to actually be valid to pass NULL for name,
> then I think you should move this check to the top of the function.
> 
> But I suspect that you're looking for (name[0] == '\0'), to check if we
> where passed an empty string, e.g. from a resource table without
> resource names.

Good catch, I updated function prototype without changing associated test
 
> 
> > +
> > +	list_for_each_entry(carveout, &rproc->carveouts, node) {
> > +		/* Compare carveout and requested names */
> > +		if (!strcmp(carveout->name, name)) {
> > +			mem = carveout;
> > +			break;
> 
> Just return carveout when you find it.
Sure

> 
> > +		}
> > +	}
> > +
> > +	return mem;
> > +}
> > +
> 
> Regards,
> Bjorn

RE: [PATCH v3 09/13] remoteproc: modify rproc_handle_carveout to support pre-registered region

[Loic PALLARDY]

> -----Original Message-----
> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]
> Sent: Thursday, May 10, 2018 2:43 AM
> To: Loic PALLARDY <loic.pallardy@st.com>
> Cc: ohad@wizery.com; linux-remoteproc@vger.kernel.org; linux-
> kernel@vger.kernel.org; Arnaud POULIQUEN <arnaud.pouliquen@st.com>;
> benjamin.gaignard@linaro.org
> Subject: Re: [PATCH v3 09/13] remoteproc: modify rproc_handle_carveout to
> support pre-registered region
> 
> On Thu 01 Mar 08:23 PST 2018, Loic Pallardy wrote:
> 
> > In current version rproc_handle_carveout function support only dynamic
> > region allocation.
> > This patch extends rproc_handle_carveout function to support pre-
> registered
> > region. Match is done on region name, then requested device address and
> > length are checked.
> > If no name match found, original allocation is used.
> >
> > Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> > ---
> >  drivers/remoteproc/remoteproc_core.c | 49
> +++++++++++++++++++++++++++++++++++-
> >  1 file changed, 48 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> > index 0ebbc4f..49b28a0 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -679,7 +679,7 @@ static int rproc_handle_carveout(struct rproc
> *rproc,
> >  				 struct fw_rsc_carveout *rsc,
> >  				 int offset, int avail)
> >  {
> > -	struct rproc_mem_entry *carveout, *mapping = NULL;
> > +	struct rproc_mem_entry *carveout, *mapping = NULL, *mem;
> >  	struct device *dev = &rproc->dev;
> >  	dma_addr_t dma;
> >  	void *va;
> > @@ -699,6 +699,51 @@ static int rproc_handle_carveout(struct rproc
> *rproc,
> >  	dev_dbg(dev, "carveout rsc: name: %s, da 0x%x, pa 0x%x, len 0x%x,
> flags 0x%x\n",
> >  		rsc->name, rsc->da, rsc->pa, rsc->len, rsc->flags);
> >
> > +	/* Check carveout rsc already part of a registered carveout */
> > +	/* Search by name */
> > +	mem = rproc_find_carveout_by_name(rproc, rsc->name);
> > +	if (mem) {
> 
> I don't fancy the concept of "check if there is another registered
> carveout and if so update this carveouts data based on that one and then
> skip the bottom half of this function but keep them both on the
> carveouts list".
> 
> It's unfortunately not very easy to follow and it doesn't allow us to
> reuse the carveout-handler for allocations in remoteprocs without a
> resource table.
> 
> How about splitting the handling of the resource table in two parts; one
> that creates or updates a carveout on the carvouts list and a second
> part that runs through all carveouts and "allocate" (similar to your
> specific release function) them.
> 
> 
> The first part of this function would then attempt to find a carveout
> entry matching the one we're trying to "handle";
> 
> * if one is found we check if it's compatible (as you do here), update a
>   rsc_offset (as we do with vrings) and return.
> 
> * if no match is found we create a new rproc_mem_entry, fill it out
>   based on the fw_rsc_carveout information and stash it at the end of
>   the carveouts list.
> 
> We do the same in the other resource handlers (just allocate entries
> onto the lists).
> 
> 
> As that is done the second step is to loop over all carveouts, devmem,
> trace and vdev resources and actually "allocate" the resources, by
> calling a "alloc" function pointer next to your proposed release one.
> 
> For memremap() memory this could be as simple as filling out the
> resource table at the associated rsc_offset or simply doing the
> memremap().
> 
> The default alloc (filled out in step 1, if not already specified) would
> be what's today found in rproc_handle_carveout().
> 
> 
> This allows carveout resources not specified in the resource table to be
> allocated as well. Which comes in handy for the handling of vdev
> resources:
> 
> In rproc_parse_vdev() we do a similar operation to the parser of a
> fw_rsc_carveout and try to find an existing carveout by name and if not
> create a new one on the list.
> 
> As the actual allocation of carveouts is done before the "allocation" of
> vrings there will be an allocated carveout ready when we hit
> rproc_alloc_vring() - and we don't care if it came from
> dma_alloc_coherent() or a driver defined region.
> 
> 
> Does this sound reasonable?
Yes, better to separate resource table parsing and memory carveout allocation.
I'll update series in that way

Regards,
Loic


> 
> Regards,
> Bjorn

RE: [PATCH v3 08/13] remoteproc: add prepare and unprepare ops

[Loic PALLARDY]

> -----Original Message-----
> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]
> Sent: Thursday, May 10, 2018 2:53 AM
> To: Loic PALLARDY <loic.pallardy@st.com>
> Cc: ohad@wizery.com; linux-remoteproc@vger.kernel.org; linux-
> kernel@vger.kernel.org; Arnaud POULIQUEN <arnaud.pouliquen@st.com>;
> benjamin.gaignard@linaro.org
> Subject: Re: [PATCH v3 08/13] remoteproc: add prepare and unprepare ops
> 
> On Thu 01 Mar 08:23 PST 2018, Loic Pallardy wrote:
> 
> > On some SoC architecture, it is needed to enable HW like
> > clock, bus, regulator, memory region... before loading
> > co-processor firmware.
> >
> > This patch introduces prepare and unprepare ops to execute
> > platform specific function before firmware loading and after
> > stop execution.
> >
> > Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> > ---
> >  drivers/remoteproc/remoteproc_core.c | 20 +++++++++++++++++++-
> >  include/linux/remoteproc.h           |  4 ++++
> >  2 files changed, 23 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> > index 7a500cb..0ebbc4f 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -1058,12 +1058,22 @@ static int rproc_fw_boot(struct rproc *rproc,
> const struct firmware *fw)
> >  		return ret;
> >  	}
> >
> > +	/* Prepare rproc for firmware loading if needed */
> > +	if (rproc->ops->prepare) {
> > +		ret = rproc->ops->prepare(rproc);
> > +		if (ret) {
> > +			dev_err(dev, "can't prepare rproc %s: %d\n",
> > +				rproc->name, ret);
> > +			goto disable_iommu;
> > +		}
> > +	}
> 
> We do allow drivers to implement custom versions of parse_fw() - and
> they can call the resource-table-parse-fw from the custom function.
> 
> So with the proposed refactoring in patch 9 I would like for parse_fw()
> to call back into the core to fill out the resource lists and then
> before jumping to rproc_start() we loop over the allocator functions.

Agree
Regards,
Loic
> 
> > +
> >  	rproc->bootaddr = rproc_get_boot_addr(rproc, fw);
> >
> >  	/* load resource table */
> >  	ret = rproc_load_rsc_table(rproc, fw);
> >  	if (ret)
> > -		goto disable_iommu;
> > +		goto unprepare_rproc;
> >
> >  	/* reset max_notifyid */
> >  	rproc->max_notifyid = -1;
> > @@ -1086,6 +1096,10 @@ static int rproc_fw_boot(struct rproc *rproc,
> const struct firmware *fw)
> >  	kfree(rproc->cached_table);
> >  	rproc->cached_table = NULL;
> >  	rproc->table_ptr = NULL;
> > +unprepare_rproc:
> > +	/* release HW resources if needed */
> > +	if (rproc->ops->unprepare)
> > +		rproc->ops->unprepare(rproc);
> >  disable_iommu:
> >  	rproc_disable_iommu(rproc);
> >  	return ret;
> > @@ -1331,6 +1345,10 @@ void rproc_shutdown(struct rproc *rproc)
> >  	/* clean up all acquired resources */
> >  	rproc_resource_cleanup(rproc);
> >
> > +	/* release HW resources if needed */
> > +	if (rproc->ops->unprepare)
> > +		rproc->ops->unprepare(rproc);
> 
> And this would then be handled by the rproc_resource_cleanup() function,
> looping over all resources and calling release().
> 
> Regards,
> Bjorn

RE: [PATCH v3 10/13] remoteproc: modify vring allocation to support pre-registered region

[Loic PALLARDY]

> -----Original Message-----
> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]
> Sent: Thursday, May 10, 2018 2:59 AM
> To: Loic PALLARDY <loic.pallardy@st.com>
> Cc: ohad@wizery.com; linux-remoteproc@vger.kernel.org; linux-
> kernel@vger.kernel.org; Arnaud POULIQUEN <arnaud.pouliquen@st.com>;
> benjamin.gaignard@linaro.org
> Subject: Re: [PATCH v3 10/13] remoteproc: modify vring allocation to support
> pre-registered region
> 
> On Thu 01 Mar 08:23 PST 2018, Loic Pallardy wrote:
> [..]
> > @@ -265,23 +269,45 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev,
> int i)
> >  	struct device *dev = &rproc->dev;
> >  	struct rproc_vring *rvring = &rvdev->vring[i];
> >  	struct fw_rsc_vdev *rsc;
> > -	dma_addr_t dma;
> > -	void *va;
> >  	int ret, size, notifyid;
> > +	struct fw_rsc_carveout rsc_carveout;
> > +	struct rproc_mem_entry *mem;
> >
> >  	/* actual size of vring (in bytes) */
> >  	size = PAGE_ALIGN(vring_size(rvring->len, rvring->align));
> >
> > -	/*
> > -	 * Allocate non-cacheable memory for the vring. In the future
> > -	 * this call will also configure the IOMMU for us
> > -	 */
> > -	va = dma_alloc_coherent(dev->parent, size, &dma, GFP_KERNEL);
> > -	if (!va) {
> > -		dev_err(dev->parent, "dma_alloc_coherent failed\n");
> > +	rsc = (void *)rproc->table_ptr + rvdev->rsc_offset;
> > +
> > +	/* Create virtual firmware carveout resource */
> > +	rsc_carveout.da = rsc->vring[i].da;
> > +	rsc_carveout.pa = FW_RSC_ADDR_ANY;
> > +	rsc_carveout.len = size;
> > +	rsc_carveout.flags = 0;
> > +	rsc_carveout.reserved = 0;
> > +	snprintf(rsc_carveout.name, sizeof(rsc_carveout.name),
> "vdev%dvring%d",
> > +		 rvdev->index, i);
> [..]
> > @@ -437,6 +460,7 @@ static int rproc_handle_vdev(struct rproc *rproc,
> struct fw_rsc_vdev *rsc,
> >
> >  	rvdev->id = rsc->id;
> >  	rvdev->rproc = rproc;
> > +	rvdev->index = index++;
> 
> This index isn't deterministic over multiple remoteproc instances and
> multiple restarts of the remoteproc. It probably needs to be based
> generated based on the ordering in the resource table.
Yes it was my intention, but static use make it wrong.
I'll revisit this point

Regards,
Loic
> 
> Regards,
> Bjorn

RE: [PATCH v3 11/13] remoteproc: create vdev subdevice with specific dma memory pool

[Loic PALLARDY]

> -----Original Message-----
> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]
> Sent: Thursday, May 10, 2018 3:06 AM
> To: Loic PALLARDY <loic.pallardy@st.com>
> Cc: ohad@wizery.com; linux-remoteproc@vger.kernel.org; linux-
> kernel@vger.kernel.org; Arnaud POULIQUEN <arnaud.pouliquen@st.com>;
> benjamin.gaignard@linaro.org
> Subject: Re: [PATCH v3 11/13] remoteproc: create vdev subdevice with
> specific dma memory pool
> 
> On Thu 01 Mar 08:23 PST 2018, Loic Pallardy wrote:
> > @@ -479,6 +481,41 @@ static int rproc_handle_vdev(struct rproc *rproc,
> struct fw_rsc_vdev *rsc,
> >  			goto unwind_vring_allocations;
> >  	}
> >
> > +	/* Initialise vdev subdevice */
> > +	snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index);
> > +	rvdev->dev.parent = rproc->dev.parent;
> > +	dev_set_name(&rvdev->dev, "%s#%s", dev_name(rvdev-
> >dev.parent), name);
> > +	dev_set_drvdata(&rvdev->dev, rvdev);
> > +	dma_set_coherent_mask(&rvdev->dev, DMA_BIT_MASK(32));
> > +
> > +	ret = device_register(&rvdev->dev);
> > +	if (ret)
> > +		goto unwind_vring_allocations;
> > +
> > +	/* Try to find dedicated vdev buffer carveout */
> > +	carveout = rproc_find_carveout_by_name(rproc, name);
> > +
> > +	if (carveout) {
> > +		phys_addr_t pa;
> > +
> > +		if (carveout->va) {
> > +			dev_warn(dev, "vdev %d buffer carveout already
> mapped\n",
> > +				 rvdev->index);
> > +			pa = rproc_va_to_pa(carveout->va);
> > +		} else {
> > +			/* Use dma address as carveout no memmapped yet
> */
> > +			pa = (phys_addr_t)carveout->dma;
> > +		}
> > +
> > +		/* Associate vdev buffer memory pool to vdev subdevice */
> > +		ret = dmam_declare_coherent_memory(&rvdev->dev, pa,
> > +						   carveout->da,
> > +						   carveout->len,
> > +
> DMA_MEMORY_EXCLUSIVE);
> > +		if (ret < 0)
> > +			goto unregister_device;
> > +	}
> > +
> 
> So with this there will be one more device between rproc->dev and the
> virtio dev, for the sake of memory management. So e.g. a rpmsg device
> will still need to allocate memory from dev->parent->parent; which now
> possibly has a specific dma_mem.

Yes it is a new sub device between virtio dev and rproc dev, but this device owns memory region dedicated to this vdev
So, rpmsg device will allocate from dev->parent, not from dev->parent->parent.

> 
> Is it not possible to assign the memory to the vdev->dev and allow the
> virtio devices can allocate memory from their parent device?
No sure to catch your point.
Who should be the parent device? Rproc dev itself? Doesn't fit well if you want to handle 2 different vdev with specific memory region for each of them.

Regards,
Loic
> 
> Regards,
> Bjorn

Re: [PATCH v3 00/13] remoteproc: add fixed memory region support

[Anup Patel]

Hi Loic Pallardy, Bjorn Andersson,

This patchset also helps in getting VirtIO RPMSG driver working
with VirtIO MMIO transport.

Is it possible to have this patchset part of Linux-4.19 or Linux-4.20?

Regards,
Anup

RE: [PATCH v3 00/13] remoteproc: add fixed memory region support

[Loic PALLARDY]

Hi Anup,

Sorry busy on ST internal stuff. I'll send V4 integrating Bjorn's comments in the coming days.

Regards,
Loic

> -----Original Message-----
> From: Anup Patel [mailto:anup@brainfault.org]
> Sent: Monday, June 25, 2018 5:23 AM
> To: Loic PALLARDY <loic.pallardy@st.com>; Bjorn Andersson
> <bjorn.andersson@linaro.org>
> Cc: Ohad Ben-Cohen <ohad@wizery.com>; linux-
> remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org List <linux-
> kernel@vger.kernel.org>; Arnaud POULIQUEN <arnaud.pouliquen@st.com>;
> benjamin.gaignard@linaro.org
> Subject: Re: [PATCH v3 00/13] remoteproc: add fixed memory region support
> 
> Hi Loic Pallardy, Bjorn Andersson,
> 
> This patchset also helps in getting VirtIO RPMSG driver working
> with VirtIO MMIO transport.
> 
> Is it possible to have this patchset part of Linux-4.19 or Linux-4.20?
> 
> Regards,
> Anup

Re: [PATCH v3 08/13] remoteproc: add prepare and unprepare ops

[Suman Anna]

Hi Bjorn,

> >> -----Original Message-----
>> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]
>> Sent: Thursday, May 10, 2018 2:53 AM
>> To: Loic PALLARDY <loic.pallardy@st.com>
>> Cc: ohad@wizery.com; linux-remoteproc@vger.kernel.org; linux-
>> kernel@vger.kernel.org; Arnaud POULIQUEN <arnaud.pouliquen@st.com>;
>> benjamin.gaignard@linaro.org
>> Subject: Re: [PATCH v3 08/13] remoteproc: add prepare and unprepare ops
>>
>> On Thu 01 Mar 08:23 PST 2018, Loic Pallardy wrote:
>>
>>> On some SoC architecture, it is needed to enable HW like
>>> clock, bus, regulator, memory region... before loading
>>> co-processor firmware.
>>>
>>> This patch introduces prepare and unprepare ops to execute
>>> platform specific function before firmware loading and after
>>> stop execution.
>>>
>>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>>> ---
>>>  drivers/remoteproc/remoteproc_core.c | 20 +++++++++++++++++++-
>>>  include/linux/remoteproc.h           |  4 ++++
>>>  2 files changed, 23 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/remoteproc/remoteproc_core.c
>> b/drivers/remoteproc/remoteproc_core.c
>>> index 7a500cb..0ebbc4f 100644
>>> --- a/drivers/remoteproc/remoteproc_core.c
>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>> @@ -1058,12 +1058,22 @@ static int rproc_fw_boot(struct rproc *rproc,
>> const struct firmware *fw)
>>>  		return ret;
>>>  	}
>>>
>>> +	/* Prepare rproc for firmware loading if needed */
>>> +	if (rproc->ops->prepare) {
>>> +		ret = rproc->ops->prepare(rproc);
>>> +		if (ret) {
>>> +			dev_err(dev, "can't prepare rproc %s: %d\n",
>>> +				rproc->name, ret);
>>> +			goto disable_iommu;
>>> +		}
>>> +	}
>>
>> We do allow drivers to implement custom versions of parse_fw() - and
>> they can call the resource-table-parse-fw from the custom function.
>>
>> So with the proposed refactoring in patch 9 I would like for parse_fw()
>> to call back into the core to fill out the resource lists and then
>> before jumping to rproc_start() we loop over the allocator functions.

I do like this patch and actually have a need for something similar for
supporting loading into internal memories for some R5F remote processors
on the latest TI SoCs. R5Fs have both resets and halt signals, and the
reset needs to be released to allow loading into TCMs, with the start
performing the halt. I am forced to do this between rproc_alloc() and
rproc_start() at the moment, but this really creates a mismatch between
my probe, start, stop and remove.

That said, I do agree with you that the way this was used with carveouts
should not have been used. Overloading the parse_fw to achieve above is
not right either. Please see my comments on Loic's v4 ST remoteproc
patch [1].

[1] https://patchwork.kernel.org/patch/10547153/

> 
> Agree
> Regards,
> Loic
>>
>>> +
>>>  	rproc->bootaddr = rproc_get_boot_addr(rproc, fw);
>>>
>>>  	/* load resource table */
>>>  	ret = rproc_load_rsc_table(rproc, fw);
>>>  	if (ret)
>>> -		goto disable_iommu;
>>> +		goto unprepare_rproc;
>>>
>>>  	/* reset max_notifyid */
>>>  	rproc->max_notifyid = -1;
>>> @@ -1086,6 +1096,10 @@ static int rproc_fw_boot(struct rproc *rproc,
>> const struct firmware *fw)
>>>  	kfree(rproc->cached_table);
>>>  	rproc->cached_table = NULL;
>>>  	rproc->table_ptr = NULL;
>>> +unprepare_rproc:
>>> +	/* release HW resources if needed */
>>> +	if (rproc->ops->unprepare)
>>> +		rproc->ops->unprepare(rproc);
>>>  disable_iommu:
>>>  	rproc_disable_iommu(rproc);
>>>  	return ret;
>>> @@ -1331,6 +1345,10 @@ void rproc_shutdown(struct rproc *rproc)
>>>  	/* clean up all acquired resources */
>>>  	rproc_resource_cleanup(rproc);
>>>
>>> +	/* release HW resources if needed */
>>> +	if (rproc->ops->unprepare)
>>> +		rproc->ops->unprepare(rproc);
>>
>> And this would then be handled by the rproc_resource_cleanup() function,
>> looping over all resources and calling release().
>>
>> Regards,
>> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>