[PATCH v2 00/16] remoteproc: add fixed memory region support

[PATCH v2 00/16] 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 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 (16):
  remoteproc: add rproc_va_to_pa function
  remoteproc: add release ops in rproc_mem_entry struct
  remoteproc: introduce rproc_add_carveout function
  remoteproc: introduce rproc_find_carveout_by_da
  remoteproc: modify rproc_handle_carveout to support preallocated
    region
  remoteproc: modify vring allocation to support preallocated region
  remoteproc: st: add reserved memory support
  remoteproc: add name in rproc_mem_entry struct
  remoteproc: add memory device management support
  remoteproc: add memory device registering in rproc_add_carveout
  remoteproc: introduce rproc_find_carveout_by_name function
  remoteproc: look-up memory-device for vring allocation
  remoteproc: look-up memory-device for virtio device allocation
  remoteproc: look-up pre-registered carveout by name for carveout
    allocation
  remoteproc: st: associate memory device to memory regions
  rpmsg: virtio: allocate buffer from parent

 drivers/remoteproc/remoteproc_core.c     | 408 +++++++++++++++++++++++++++++--
 drivers/remoteproc/remoteproc_debugfs.c  |   1 +
 drivers/remoteproc/remoteproc_internal.h |   7 +
 drivers/remoteproc/remoteproc_virtio.c   |   2 +-
 drivers/remoteproc/st_remoteproc.c       |  44 +++-
 drivers/rpmsg/virtio_rpmsg_bus.c         |   2 +-
 include/linux/remoteproc.h               |  14 +-
 7 files changed, 441 insertions(+), 37 deletions(-)

-- 
1.9.1

[PATCH v2 00/16] 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 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 (16):
  remoteproc: add rproc_va_to_pa function
  remoteproc: add release ops in rproc_mem_entry struct
  remoteproc: introduce rproc_add_carveout function
  remoteproc: introduce rproc_find_carveout_by_da
  remoteproc: modify rproc_handle_carveout to support preallocated
    region
  remoteproc: modify vring allocation to support preallocated region
  remoteproc: st: add reserved memory support
  remoteproc: add name in rproc_mem_entry struct
  remoteproc: add memory device management support
  remoteproc: add memory device registering in rproc_add_carveout
  remoteproc: introduce rproc_find_carveout_by_name function
  remoteproc: look-up memory-device for vring allocation
  remoteproc: look-up memory-device for virtio device allocation
  remoteproc: look-up pre-registered carveout by name for carveout
    allocation
  remoteproc: st: associate memory device to memory regions
  rpmsg: virtio: allocate buffer from parent

 drivers/remoteproc/remoteproc_core.c     | 408 +++++++++++++++++++++++++++++--
 drivers/remoteproc/remoteproc_debugfs.c  |   1 +
 drivers/remoteproc/remoteproc_internal.h |   7 +
 drivers/remoteproc/remoteproc_virtio.c   |   2 +-
 drivers/remoteproc/st_remoteproc.c       |  44 +++-
 drivers/rpmsg/virtio_rpmsg_bus.c         |   2 +-
 include/linux/remoteproc.h               |  14 +-
 7 files changed, 441 insertions(+), 37 deletions(-)

-- 
1.9.1

[PATCH v2 01/16] 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 | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index eab14b4..faa18a7 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -139,6 +139,17 @@ static void rproc_disable_iommu(struct rproc *rproc)
 	iommu_domain_free(domain);
 }
 
+static phys_addr_t rproc_va_to_pa(void *cpu_addr)
+{
+	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
@@ -700,7 +711,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 v2 01/16] 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 | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index eab14b4..faa18a7 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -139,6 +139,17 @@ static void rproc_disable_iommu(struct rproc *rproc)
 	iommu_domain_free(domain);
 }
 
+static phys_addr_t rproc_va_to_pa(void *cpu_addr)
+{
+	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
@@ -700,7 +711,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 v2 01/16] remoteproc: add rproc_va_to_pa function

[Bjorn Andersson]

On Thu 30 Nov 08:46 PST 2017, 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>
> ---
>  drivers/remoteproc/remoteproc_core.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index eab14b4..faa18a7 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -139,6 +139,17 @@ static void rproc_disable_iommu(struct rproc *rproc)
>  	iommu_domain_free(domain);
>  }
>  
> +static phys_addr_t rproc_va_to_pa(void *cpu_addr)
> +{
> +	if (is_vmalloc_addr(cpu_addr)) {

Please add a comment describing when is_vmalloc_addr() would be true.

> +		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
> @@ -700,7 +711,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);

This is more correct than using "dma", so this is good.

Regards,
Bjorn

RE: [PATCH v2 01/16] remoteproc: add rproc_va_to_pa function

[Loic PALLARDY]

Hi Bjorn,

Thanks for the review of this series.

> -----Original Message-----
> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]
> Sent: Thursday, December 14, 2017 1:31 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 v2 01/16] remoteproc: add rproc_va_to_pa function
> 
> On Thu 30 Nov 08:46 PST 2017, 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>
> > ---
> >  drivers/remoteproc/remoteproc_core.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> > index eab14b4..faa18a7 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -139,6 +139,17 @@ static void rproc_disable_iommu(struct rproc
> *rproc)
> >  	iommu_domain_free(domain);
> >  }
> >
> > +static phys_addr_t rproc_va_to_pa(void *cpu_addr)
> > +{
> > +	if (is_vmalloc_addr(cpu_addr)) {
> 
> Please add a comment describing when is_vmalloc_addr() would be true.
Yes sure.
Regards,
Loic
> 
> > +		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
> > @@ -700,7 +711,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);
> 
> This is more correct than using "dma", so this is good.
> 
> Regards,
> Bjorn

[PATCH v2 02/16] 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 | 26 ++++++++++++++++++++++----
 include/linux/remoteproc.h           |  6 ++++--
 2 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index faa18a7..f23daf9 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -592,6 +592,25 @@ 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);
+	list_del(&mem->node);
+	kfree(mem);
+	return 0;
+}
+
+/**
  * rproc_handle_carveout() - handle phys contig memory allocation requests
  * @rproc: rproc handle
  * @rsc: the resource entry
@@ -717,6 +736,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);
 
@@ -847,10 +867,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);
-		list_del(&entry->node);
-		kfree(entry);
+		if (entry->release)
+			entry->release(rproc, entry);
 	}
 
 	/* clean up remote vdev entries */
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 44e630e..8780f2e 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
  */
@@ -319,12 +322,11 @@ struct rproc_mem_entry {
 	dma_addr_t dma;
 	int len;
 	u32 da;
+	int (*release)(struct rproc *rproc, struct rproc_mem_entry *mem);
 	void *priv;
 	struct list_head node;
 };
 
-struct rproc;
-
 /**
  * struct rproc_ops - platform-specific device handlers
  * @start:	power on the device and boot it
-- 
1.9.1

[PATCH v2 02/16] 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 | 26 ++++++++++++++++++++++----
 include/linux/remoteproc.h           |  6 ++++--
 2 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index faa18a7..f23daf9 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -592,6 +592,25 @@ 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);
+	list_del(&mem->node);
+	kfree(mem);
+	return 0;
+}
+
+/**
  * rproc_handle_carveout() - handle phys contig memory allocation requests
  * @rproc: rproc handle
  * @rsc: the resource entry
@@ -717,6 +736,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);
 
@@ -847,10 +867,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);
-		list_del(&entry->node);
-		kfree(entry);
+		if (entry->release)
+			entry->release(rproc, entry);
 	}
 
 	/* clean up remote vdev entries */
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 44e630e..8780f2e 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
  */
@@ -319,12 +322,11 @@ struct rproc_mem_entry {
 	dma_addr_t dma;
 	int len;
 	u32 da;
+	int (*release)(struct rproc *rproc, struct rproc_mem_entry *mem);
 	void *priv;
 	struct list_head node;
 };
 
-struct rproc;
-
 /**
  * struct rproc_ops - platform-specific device handlers
  * @start:	power on the device and boot it
-- 
1.9.1

Re: [PATCH v2 02/16] remoteproc: add release ops in rproc_mem_entry struct

[Bjorn Andersson]

On Thu 30 Nov 08:46 PST 2017, Loic Pallardy wrote:

> +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);
> +	list_del(&mem->node);

The core is responsible for putting the node on a list, so let the
cleanup take if off the list.

> +	kfree(mem);
> +	return 0;
> +}
> +
[..]
> @@ -319,12 +322,11 @@ struct rproc_mem_entry {
>  	dma_addr_t dma;
>  	int len;
>  	u32 da;
> +	int (*release)(struct rproc *rproc, struct rproc_mem_entry *mem);

The placement here seems random, please move it last in the struct.

>  	void *priv;
>  	struct list_head node;
>  };
>  

Regards,
Bjorn

RE: [PATCH v2 02/16] remoteproc: add release ops in rproc_mem_entry struct

[Loic PALLARDY]

> -----Original Message-----
> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]
> Sent: Thursday, December 14, 2017 1:34 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 v2 02/16] remoteproc: add release ops in
> rproc_mem_entry struct
> 
> On Thu 30 Nov 08:46 PST 2017, Loic Pallardy wrote:
> 
> > +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);
> > +	list_del(&mem->node);
> 
> The core is responsible for putting the node on a list, so let the
> cleanup take if off the list.
ok
> 
> > +	kfree(mem);
> > +	return 0;
> > +}
> > +
> [..]
> > @@ -319,12 +322,11 @@ struct rproc_mem_entry {
> >  	dma_addr_t dma;
> >  	int len;
> >  	u32 da;
> > +	int (*release)(struct rproc *rproc, struct rproc_mem_entry *mem);
> 
> The placement here seems random, please move it last in the struct.
ok
> 
> >  	void *priv;
> >  	struct list_head node;
> >  };
> >
> 
> Regards,
> Bjorn

[PATCH v2 03/16] 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     | 22 ++++++++++++++++++++++
 drivers/remoteproc/remoteproc_internal.h |  7 +++++++
 include/linux/remoteproc.h               |  2 ++
 3 files changed, 31 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index f23daf9..279320a 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -737,6 +737,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	carveout->dma = dma;
 	carveout->da = rsc->da;
 	carveout->release = rproc_release_carveout;
+	carveout->priv = (void *)CARVEOUT_RSC_ALLOCATED;
 
 	list_add_tail(&carveout->node, &rproc->carveouts);
 
@@ -751,6 +752,27 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	return ret;
 }
 
+/**
+ * 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.
+ */
+int rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry *mem)
+{
+	if (!rproc || !mem)
+		return -EINVAL;
+
+	mem->priv = (void *)CARVEOUT_EXTERNAL;
+
+	list_add_tail(&mem->node, &rproc->carveouts);
+
+	return 0;
+}
+EXPORT_SYMBOL(rproc_add_carveout);
+
 /*
  * A lookup table for resource handlers. The indices are defined in
  * enum fw_resource_type.
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index c1077be..69b22ac 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -23,6 +23,13 @@
 #include <linux/irqreturn.h>
 #include <linux/firmware.h>
 
+/* Indicate carveout origin */
+enum carveout_src {
+	CARVEOUT_RSC		= 0,
+	CARVEOUT_RSC_ALLOCATED	= 1,
+	CARVEOUT_EXTERNAL	= 2,
+};
+
 struct rproc;
 
 /**
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 8780f2e..5bd5175 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -522,6 +522,8 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 int rproc_del(struct rproc *rproc);
 void rproc_free(struct rproc *rproc);
 
+int rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry *mem);
+
 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 v2 03/16] 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     | 22 ++++++++++++++++++++++
 drivers/remoteproc/remoteproc_internal.h |  7 +++++++
 include/linux/remoteproc.h               |  2 ++
 3 files changed, 31 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index f23daf9..279320a 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -737,6 +737,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	carveout->dma = dma;
 	carveout->da = rsc->da;
 	carveout->release = rproc_release_carveout;
+	carveout->priv = (void *)CARVEOUT_RSC_ALLOCATED;
 
 	list_add_tail(&carveout->node, &rproc->carveouts);
 
@@ -751,6 +752,27 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	return ret;
 }
 
+/**
+ * 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.
+ */
+int rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry *mem)
+{
+	if (!rproc || !mem)
+		return -EINVAL;
+
+	mem->priv = (void *)CARVEOUT_EXTERNAL;
+
+	list_add_tail(&mem->node, &rproc->carveouts);
+
+	return 0;
+}
+EXPORT_SYMBOL(rproc_add_carveout);
+
 /*
  * A lookup table for resource handlers. The indices are defined in
  * enum fw_resource_type.
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index c1077be..69b22ac 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -23,6 +23,13 @@
 #include <linux/irqreturn.h>
 #include <linux/firmware.h>
 
+/* Indicate carveout origin */
+enum carveout_src {
+	CARVEOUT_RSC		= 0,
+	CARVEOUT_RSC_ALLOCATED	= 1,
+	CARVEOUT_EXTERNAL	= 2,
+};
+
 struct rproc;
 
 /**
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 8780f2e..5bd5175 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -522,6 +522,8 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 int rproc_del(struct rproc *rproc);
 void rproc_free(struct rproc *rproc);
 
+int rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry *mem);
+
 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

Re: [PATCH v2 03/16] remoteproc: introduce rproc_add_carveout function

[Bjorn Andersson]

On Thu 30 Nov 08:46 PST 2017, Loic Pallardy wrote:
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index f23daf9..279320a 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -737,6 +737,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
>  	carveout->dma = dma;
>  	carveout->da = rsc->da;
>  	carveout->release = rproc_release_carveout;
> +	carveout->priv = (void *)CARVEOUT_RSC_ALLOCATED;

I don't fancy the (ab)use of priv to keep track of this, I also don't
see that it's ever used. Please drop it.

[..]
> +int rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry *mem)
> +{
> +	if (!rproc || !mem)
> +		return -EINVAL;

I don't see this function doing more than adding the item to the list of
carveouts, which can't fail. So let's just rely on the user calling it
with valid references and make it return void.

> +
> +	mem->priv = (void *)CARVEOUT_EXTERNAL;
> +
> +	list_add_tail(&mem->node, &rproc->carveouts);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(rproc_add_carveout);

Regards,
Bjorn

RE: [PATCH v2 03/16] remoteproc: introduce rproc_add_carveout function

[Loic PALLARDY]

> -----Original Message-----
> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]
> Sent: Thursday, December 14, 2017 1:37 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 v2 03/16] remoteproc: introduce rproc_add_carveout
> function
> 
> On Thu 30 Nov 08:46 PST 2017, Loic Pallardy wrote:
> > diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> > index f23daf9..279320a 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -737,6 +737,7 @@ static int rproc_handle_carveout(struct rproc
> *rproc,
> >  	carveout->dma = dma;
> >  	carveout->da = rsc->da;
> >  	carveout->release = rproc_release_carveout;
> > +	carveout->priv = (void *)CARVEOUT_RSC_ALLOCATED;
> 
> I don't fancy the (ab)use of priv to keep track of this, I also don't
> see that it's ever used. Please drop it.
It was to distinguish carveout defined from resource table and carveout registered by driver.
But agree about priv field usage
> 
> [..]
> > +int rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry
> *mem)
> > +{
> > +	if (!rproc || !mem)
> > +		return -EINVAL;
> 
> I don't see this function doing more than adding the item to the list of
> carveouts, which can't fail. So let's just rely on the user calling it
> with valid references and make it return void.
Ok

> 
> > +
> > +	mem->priv = (void *)CARVEOUT_EXTERNAL;
> > +
> > +	list_add_tail(&mem->node, &rproc->carveouts);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(rproc_add_carveout);
> 
> Regards,
> Bjorn

[PATCH v2 04/16] remoteproc: introduce rproc_find_carveout_by_da

[Loic Pallardy]

This patch provides a new function to find a carveout according
to a device address (da).
If match found, this function returns CPU virtual address corresponding
to specified da.

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

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 279320a..78525d1 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -211,6 +211,48 @@ void *rproc_da_to_va(struct rproc *rproc, u64 da, int len)
 }
 EXPORT_SYMBOL(rproc_da_to_va);
 
+/**
+ * rproc_find_carveout_by_da() - lookup the carveout region for a remoteproc address
+ * @rproc: handle of a remote processor
+ * @da: remoteproc device address to find
+ * @len: length of the memory region @da is pointing to
+ *
+ * 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 translate specific device addresse to virtual
+ * addresse so we can fill firmware resource table.
+ *
+ * The function returns a valid virtual address on success or NULL on failure.
+ */
+void *rproc_find_carveout_by_da(struct rproc *rproc, u64 da, int len)
+{
+	struct rproc_mem_entry *carveout;
+	void *va = NULL;
+
+	list_for_each_entry(carveout, &rproc->carveouts, node) {
+		int offset = da - carveout->da;
+
+		/* try next carveout if da is too small */
+		if (offset < 0)
+			continue;
+
+		/* try next carveout if da is too large */
+		if (offset + len > carveout->len)
+			continue;
+
+		va = carveout->va + offset;
+
+		break;
+	}
+
+	return va;
+}
+
 int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 {
 	struct rproc *rproc = rvdev->rproc;
-- 
1.9.1

[PATCH v2 04/16] remoteproc: introduce rproc_find_carveout_by_da

[Loic Pallardy]

This patch provides a new function to find a carveout according
to a device address (da).
If match found, this function returns CPU virtual address corresponding
to specified da.

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

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 279320a..78525d1 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -211,6 +211,48 @@ void *rproc_da_to_va(struct rproc *rproc, u64 da, int len)
 }
 EXPORT_SYMBOL(rproc_da_to_va);
 
+/**
+ * rproc_find_carveout_by_da() - lookup the carveout region for a remoteproc address
+ * @rproc: handle of a remote processor
+ * @da: remoteproc device address to find
+ * @len: length of the memory region @da is pointing to
+ *
+ * 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 translate specific device addresse to virtual
+ * addresse so we can fill firmware resource table.
+ *
+ * The function returns a valid virtual address on success or NULL on failure.
+ */
+void *rproc_find_carveout_by_da(struct rproc *rproc, u64 da, int len)
+{
+	struct rproc_mem_entry *carveout;
+	void *va = NULL;
+
+	list_for_each_entry(carveout, &rproc->carveouts, node) {
+		int offset = da - carveout->da;
+
+		/* try next carveout if da is too small */
+		if (offset < 0)
+			continue;
+
+		/* try next carveout if da is too large */
+		if (offset + len > carveout->len)
+			continue;
+
+		va = carveout->va + offset;
+
+		break;
+	}
+
+	return va;
+}
+
 int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 {
 	struct rproc *rproc = rvdev->rproc;
-- 
1.9.1

Re: [PATCH v2 04/16] remoteproc: introduce rproc_find_carveout_by_da

[Bjorn Andersson]

On Thu 30 Nov 08:46 PST 2017, Loic Pallardy wrote:

> This patch provides a new function to find a carveout according
> to a device address (da).
> If match found, this function returns CPU virtual address corresponding
> to specified da.
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> ---
>  drivers/remoteproc/remoteproc_core.c | 42 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 279320a..78525d1 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -211,6 +211,48 @@ void *rproc_da_to_va(struct rproc *rproc, u64 da, int len)
>  }
>  EXPORT_SYMBOL(rproc_da_to_va);
>  
> +/**
> + * rproc_find_carveout_by_da() - lookup the carveout region for a remoteproc address
> + * @rproc: handle of a remote processor
> + * @da: remoteproc device address to find
> + * @len: length of the memory region @da is pointing to
> + *
> + * 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 translate specific device addresse to virtual
> + * addresse so we can fill firmware resource table.
> + *
> + * The function returns a valid virtual address on success or NULL on failure.
> + */
> +void *rproc_find_carveout_by_da(struct rproc *rproc, u64 da, int len)

The name suggest that this returns a struct rproc_mem_entry *, but the
implementation is just a duplicate of rproc_da_to_va.

I think I prefer that we just use the name based lookup in the
subsequent patch, alternatively I think this should be made to return
the carveout and one could then use da_to_va if you need a reference
within that carveout.

> +{
> +	struct rproc_mem_entry *carveout;
> +	void *va = NULL;
> +
> +	list_for_each_entry(carveout, &rproc->carveouts, node) {
> +		int offset = da - carveout->da;
> +
> +		/* try next carveout if da is too small */
> +		if (offset < 0)
> +			continue;
> +
> +		/* try next carveout if da is too large */
> +		if (offset + len > carveout->len)
> +			continue;
> +
> +		va = carveout->va + offset;
> +
> +		break;
> +	}
> +
> +	return va;
> +}

Regards,
Bjorn

RE: [PATCH v2 04/16] remoteproc: introduce rproc_find_carveout_by_da

[Loic PALLARDY]

> -----Original Message-----
> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]
> Sent: Thursday, December 14, 2017 1:46 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 v2 04/16] remoteproc: introduce
> rproc_find_carveout_by_da
> 
> On Thu 30 Nov 08:46 PST 2017, Loic Pallardy wrote:
> 
> > This patch provides a new function to find a carveout according
> > to a device address (da).
> > If match found, this function returns CPU virtual address corresponding
> > to specified da.
> >
> > Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> > ---
> >  drivers/remoteproc/remoteproc_core.c | 42
> ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> > index 279320a..78525d1 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -211,6 +211,48 @@ void *rproc_da_to_va(struct rproc *rproc, u64 da,
> int len)
> >  }
> >  EXPORT_SYMBOL(rproc_da_to_va);
> >
> > +/**
> > + * rproc_find_carveout_by_da() - lookup the carveout region for a
> remoteproc address
> > + * @rproc: handle of a remote processor
> > + * @da: remoteproc device address to find
> > + * @len: length of the memory region @da is pointing to
> > + *
> > + * 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 translate specific device addresse to virtual
> > + * addresse so we can fill firmware resource table.
> > + *
> > + * The function returns a valid virtual address on success or NULL on
> failure.
> > + */
> > +void *rproc_find_carveout_by_da(struct rproc *rproc, u64 da, int len)
> 
> The name suggest that this returns a struct rproc_mem_entry *, but the
> implementation is just a duplicate of rproc_da_to_va.
> 
> I think I prefer that we just use the name based lookup in the
> subsequent patch, alternatively I think this should be made to return
> the carveout and one could then use da_to_va if you need a reference
> within that carveout.
Ok, agree to have more coherent API. And as discussed we can first lookup by name and then check if requested da is matching.

/Loic
> 
> > +{
> > +	struct rproc_mem_entry *carveout;
> > +	void *va = NULL;
> > +
> > +	list_for_each_entry(carveout, &rproc->carveouts, node) {
> > +		int offset = da - carveout->da;
> > +
> > +		/* try next carveout if da is too small */
> > +		if (offset < 0)
> > +			continue;
> > +
> > +		/* try next carveout if da is too large */
> > +		if (offset + len > carveout->len)
> > +			continue;
> > +
> > +		va = carveout->va + offset;
> > +
> > +		break;
> > +	}
> > +
> > +	return va;
> > +}
> 
> Regards,
> Bjorn

[PATCH v2 05/16] remoteproc: modify rproc_handle_carveout to support preallocated region

[Loic Pallardy]

In current version rproc_handle_carveout function support only dynamic
region allocation.
This patch extends rproc_handle_carveout function to support different carveout
configurations:
- fixed DA and fixed PA: check if already part of pre-registered carveouts
(platform driver). If no, return error.
- fixed DA and any PA: check if already part of pre-allocated carveouts
(platform driver). If not found and rproc supports iommu, continue with
dynamic allocation (DA will be used for iommu programming), else return
error as no way to force DA.
- any DA and any PA: use original dynamic allocation

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

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 78525d1..515a17a 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -184,6 +184,10 @@ void *rproc_da_to_va(struct rproc *rproc, u64 da, int len)
 	struct rproc_mem_entry *carveout;
 	void *ptr = NULL;
 
+	/*
+	 * da_to_va platform driver is deprecated. Driver should register
+	 * carveout thanks to rproc_add_carveout function
+	 */
 	if (rproc->ops->da_to_va) {
 		ptr = rproc->ops->da_to_va(rproc, da, len);
 		if (ptr)
@@ -677,6 +681,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	struct rproc_mem_entry *carveout, *mapping;
 	struct device *dev = &rproc->dev;
 	dma_addr_t dma;
+	phys_addr_t pa;
 	void *va;
 	int ret;
 
@@ -698,6 +703,41 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	if (!carveout)
 		return -ENOMEM;
 
+	/* Check carveout rsc already part of a registered carveout */
+	if (rsc->da != FW_RSC_ADDR_ANY) {
+		va = rproc_find_carveout_by_da(rproc, rsc->da, rsc->len);
+
+		if (va) {
+			/* Registered region found */
+			pa = rproc_va_to_pa(va);
+			if (rsc->pa != FW_RSC_ADDR_ANY && rsc->pa != (u32)pa) {
+				/* Carveout doesn't match request */
+				dev_err(dev->parent,
+					"Failed to find carveout fitting da and pa\n");
+				return -ENOMEM;
+			}
+
+			/* Update rsc table with physical address */
+			rsc->pa = (u32)pa;
+
+			/* Update carveouts list */
+			carveout->va = va;
+			carveout->len = rsc->len;
+			carveout->da = rsc->da;
+			carveout->priv = (void *)CARVEOUT_RSC;
+
+			list_add_tail(&carveout->node, &rproc->carveouts);
+
+			return 0;
+		}
+
+		if (!rproc->domain) {
+			dev_err(dev->parent,
+				"Bad carveout rsc configuration\n");
+			return -ENOMEM;
+		}
+	}
+
 	va = dma_alloc_coherent(dev->parent, rsc->len, &dma, GFP_KERNEL);
 	if (!va) {
 		dev_err(dev->parent,
-- 
1.9.1

[PATCH v2 05/16] remoteproc: modify rproc_handle_carveout to support preallocated region

[Loic Pallardy]

In current version rproc_handle_carveout function support only dynamic
region allocation.
This patch extends rproc_handle_carveout function to support different carveout
configurations:
- fixed DA and fixed PA: check if already part of pre-registered carveouts
(platform driver). If no, return error.
- fixed DA and any PA: check if already part of pre-allocated carveouts
(platform driver). If not found and rproc supports iommu, continue with
dynamic allocation (DA will be used for iommu programming), else return
error as no way to force DA.
- any DA and any PA: use original dynamic allocation

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

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 78525d1..515a17a 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -184,6 +184,10 @@ void *rproc_da_to_va(struct rproc *rproc, u64 da, int len)
 	struct rproc_mem_entry *carveout;
 	void *ptr = NULL;
 
+	/*
+	 * da_to_va platform driver is deprecated. Driver should register
+	 * carveout thanks to rproc_add_carveout function
+	 */
 	if (rproc->ops->da_to_va) {
 		ptr = rproc->ops->da_to_va(rproc, da, len);
 		if (ptr)
@@ -677,6 +681,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	struct rproc_mem_entry *carveout, *mapping;
 	struct device *dev = &rproc->dev;
 	dma_addr_t dma;
+	phys_addr_t pa;
 	void *va;
 	int ret;
 
@@ -698,6 +703,41 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	if (!carveout)
 		return -ENOMEM;
 
+	/* Check carveout rsc already part of a registered carveout */
+	if (rsc->da != FW_RSC_ADDR_ANY) {
+		va = rproc_find_carveout_by_da(rproc, rsc->da, rsc->len);
+
+		if (va) {
+			/* Registered region found */
+			pa = rproc_va_to_pa(va);
+			if (rsc->pa != FW_RSC_ADDR_ANY && rsc->pa != (u32)pa) {
+				/* Carveout doesn't match request */
+				dev_err(dev->parent,
+					"Failed to find carveout fitting da and pa\n");
+				return -ENOMEM;
+			}
+
+			/* Update rsc table with physical address */
+			rsc->pa = (u32)pa;
+
+			/* Update carveouts list */
+			carveout->va = va;
+			carveout->len = rsc->len;
+			carveout->da = rsc->da;
+			carveout->priv = (void *)CARVEOUT_RSC;
+
+			list_add_tail(&carveout->node, &rproc->carveouts);
+
+			return 0;
+		}
+
+		if (!rproc->domain) {
+			dev_err(dev->parent,
+				"Bad carveout rsc configuration\n");
+			return -ENOMEM;
+		}
+	}
+
 	va = dma_alloc_coherent(dev->parent, rsc->len, &dma, GFP_KERNEL);
 	if (!va) {
 		dev_err(dev->parent,
-- 
1.9.1

Re: [PATCH v2 05/16] remoteproc: modify rproc_handle_carveout to support preallocated region

[Bjorn Andersson]

On Thu 30 Nov 08:46 PST 2017, Loic Pallardy wrote:

> In current version rproc_handle_carveout function support only dynamic
> region allocation.
> This patch extends rproc_handle_carveout function to support different carveout
> configurations:
> - fixed DA and fixed PA: check if already part of pre-registered carveouts
> (platform driver). If no, return error.
> - fixed DA and any PA: check if already part of pre-allocated carveouts
> (platform driver). If not found and rproc supports iommu, continue with
> dynamic allocation (DA will be used for iommu programming), else return
> error as no way to force DA.
> - any DA and any PA: use original dynamic allocation
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> ---
>  drivers/remoteproc/remoteproc_core.c | 40 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 78525d1..515a17a 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -184,6 +184,10 @@ void *rproc_da_to_va(struct rproc *rproc, u64 da, int len)
>  	struct rproc_mem_entry *carveout;
>  	void *ptr = NULL;
>  
> +	/*
> +	 * da_to_va platform driver is deprecated. Driver should register
> +	 * carveout thanks to rproc_add_carveout function
> +	 */

I think this comment is unrelated to the rest of this patch. I also
think that at the end of the carveout-rework we should have a patch
removing this ops.

>  	if (rproc->ops->da_to_va) {
>  		ptr = rproc->ops->da_to_va(rproc, da, len);
>  		if (ptr)
> @@ -677,6 +681,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
>  	struct rproc_mem_entry *carveout, *mapping;
>  	struct device *dev = &rproc->dev;
>  	dma_addr_t dma;
> +	phys_addr_t pa;
>  	void *va;
>  	int ret;
>  
> @@ -698,6 +703,41 @@ static int rproc_handle_carveout(struct rproc *rproc,
>  	if (!carveout)
>  		return -ENOMEM;
>  
> +	/* Check carveout rsc already part of a registered carveout */
> +	if (rsc->da != FW_RSC_ADDR_ANY) {

As mentioned before, I consider it perfectly viable for rsc->da to be
ANY and the driver providing a fixed carveout.

> +		va = rproc_find_carveout_by_da(rproc, rsc->da, rsc->len);
> +
> +		if (va) {

In a system with an iommu it's possible that rsc->len is larger than
some carveout->len and va is NULL here so we fall through, allocate some
memory and remap a segment of the carveout. (Or hopefully fails
attempting).

> +			/* Registered region found */
> +			pa = rproc_va_to_pa(va);
> +			if (rsc->pa != FW_RSC_ADDR_ANY && rsc->pa != (u32)pa) {
> +				/* Carveout doesn't match request */
> +				dev_err(dev->parent,
> +					"Failed to find carveout fitting da and pa\n");
> +				return -ENOMEM;
> +			}
> +
> +			/* Update rsc table with physical address */
> +			rsc->pa = (u32)pa;
> +
> +			/* Update carveouts list */
> +			carveout->va = va;
> +			carveout->len = rsc->len;
> +			carveout->da = rsc->da;
> +			carveout->priv = (void *)CARVEOUT_RSC;
> +
> +			list_add_tail(&carveout->node, &rproc->carveouts);

rproc_find_carveout_by_da() will return a reference into a carveout, now
we add another overlapping carveout into the same list.


I think it would be saner to not allow the resource table to describe
subsets of carveouts registered by the driver.

In which case this would better find a carveout by name or exact da,
then check that the pa, da, len and rsc->flags are adequate.

> +
> +			return 0;
> +		}
> +
> +		if (!rproc->domain) {

Currently this function ignore invalid values of da when !domain, so I
think it would be good you can submit this sanity check in it's own
patch so that anyone bisecting this would know why their broken firmware
suddenly isn't loadable.

> +			dev_err(dev->parent,
> +				"Bad carveout rsc configuration\n");
> +			return -ENOMEM;
> +		}
> +	}
> +

Regards,
Bjorn

RE: [PATCH v2 05/16] remoteproc: modify rproc_handle_carveout to support preallocated region

[Loic PALLARDY]

> -----Original Message-----
> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]
> Sent: Thursday, December 14, 2017 1: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 v2 05/16] remoteproc: modify rproc_handle_carveout to
> support preallocated region
> 
> On Thu 30 Nov 08:46 PST 2017, Loic Pallardy wrote:
> 
> > In current version rproc_handle_carveout function support only dynamic
> > region allocation.
> > This patch extends rproc_handle_carveout function to support different
> carveout
> > configurations:
> > - fixed DA and fixed PA: check if already part of pre-registered carveouts
> > (platform driver). If no, return error.
> > - fixed DA and any PA: check if already part of pre-allocated carveouts
> > (platform driver). If not found and rproc supports iommu, continue with
> > dynamic allocation (DA will be used for iommu programming), else return
> > error as no way to force DA.
> > - any DA and any PA: use original dynamic allocation
> >
> > Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> > ---
> >  drivers/remoteproc/remoteproc_core.c | 40
> ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 40 insertions(+)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> > index 78525d1..515a17a 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -184,6 +184,10 @@ void *rproc_da_to_va(struct rproc *rproc, u64 da,
> int len)
> >  	struct rproc_mem_entry *carveout;
> >  	void *ptr = NULL;
> >
> > +	/*
> > +	 * da_to_va platform driver is deprecated. Driver should register
> > +	 * carveout thanks to rproc_add_carveout function
> > +	 */
> 
> I think this comment is unrelated to the rest of this patch. I also
> think that at the end of the carveout-rework we should have a patch
> removing this ops.

I'll remove this comment and add a da_to_va clean-up patch at the end of the series

> 
> >  	if (rproc->ops->da_to_va) {
> >  		ptr = rproc->ops->da_to_va(rproc, da, len);
> >  		if (ptr)
> > @@ -677,6 +681,7 @@ static int rproc_handle_carveout(struct rproc
> *rproc,
> >  	struct rproc_mem_entry *carveout, *mapping;
> >  	struct device *dev = &rproc->dev;
> >  	dma_addr_t dma;
> > +	phys_addr_t pa;
> >  	void *va;
> >  	int ret;
> >
> > @@ -698,6 +703,41 @@ static int rproc_handle_carveout(struct rproc
> *rproc,
> >  	if (!carveout)
> >  		return -ENOMEM;
> >
> > +	/* Check carveout rsc already part of a registered carveout */
> > +	if (rsc->da != FW_RSC_ADDR_ANY) {
> 
> As mentioned before, I consider it perfectly viable for rsc->da to be
> ANY and the driver providing a fixed carveout.

Yes I'll change sequence to lookup by name first and then verify exact parameters matching , not only da definition.

> 
> > +		va = rproc_find_carveout_by_da(rproc, rsc->da, rsc->len);
> > +
> > +		if (va) {
> 
> In a system with an iommu it's possible that rsc->len is larger than
> some carveout->len and va is NULL here so we fall through, allocate some
> memory and remap a segment of the carveout. (Or hopefully fails
> attempting).
> 
> > +			/* Registered region found */
> > +			pa = rproc_va_to_pa(va);
> > +			if (rsc->pa != FW_RSC_ADDR_ANY && rsc->pa !=
> (u32)pa) {
> > +				/* Carveout doesn't match request */
> > +				dev_err(dev->parent,
> > +					"Failed to find carveout fitting da and
> pa\n");
> > +				return -ENOMEM;
> > +			}
> > +
> > +			/* Update rsc table with physical address */
> > +			rsc->pa = (u32)pa;
> > +
> > +			/* Update carveouts list */
> > +			carveout->va = va;
> > +			carveout->len = rsc->len;
> > +			carveout->da = rsc->da;
> > +			carveout->priv = (void *)CARVEOUT_RSC;
> > +
> > +			list_add_tail(&carveout->node, &rproc->carveouts);
> 
> rproc_find_carveout_by_da() will return a reference into a carveout, now
> we add another overlapping carveout into the same list.
> 
> 
> I think it would be saner to not allow the resource table to describe
> subsets of carveouts registered by the driver.
> 
> In which case this would better find a carveout by name or exact da,
> then check that the pa, da, len and rsc->flags are adequate.

Agree
/Loic
> 
> > +
> > +			return 0;
> > +		}
> > +
> > +		if (!rproc->domain) {
> 
> Currently this function ignore invalid values of da when !domain, so I
> think it would be good you can submit this sanity check in it's own
> patch so that anyone bisecting this would know why their broken firmware
> suddenly isn't loadable.
> 
> > +			dev_err(dev->parent,
> > +				"Bad carveout rsc configuration\n");
> > +			return -ENOMEM;
> > +		}
> > +	}
> > +
> 
> Regards,
> Bjorn

Re: [PATCH v2 05/16] remoteproc: modify rproc_handle_carveout to support preallocated region

[Suman Anna]

>>
>> On Thu 30 Nov 08:46 PST 2017, Loic Pallardy wrote:
>>
>>> In current version rproc_handle_carveout function support only dynamic
>>> region allocation.
>>> This patch extends rproc_handle_carveout function to support different
>> carveout
>>> configurations:
>>> - fixed DA and fixed PA: check if already part of pre-registered carveouts
>>> (platform driver). If no, return error.
>>> - fixed DA and any PA: check if already part of pre-allocated carveouts
>>> (platform driver). If not found and rproc supports iommu, continue with
>>> dynamic allocation (DA will be used for iommu programming), else return
>>> error as no way to force DA.
>>> - any DA and any PA: use original dynamic allocation
>>>
>>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>>> ---
>>>  drivers/remoteproc/remoteproc_core.c | 40
>> ++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 40 insertions(+)
>>>
>>> diff --git a/drivers/remoteproc/remoteproc_core.c
>> b/drivers/remoteproc/remoteproc_core.c
>>> index 78525d1..515a17a 100644
>>> --- a/drivers/remoteproc/remoteproc_core.c
>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>> @@ -184,6 +184,10 @@ void *rproc_da_to_va(struct rproc *rproc, u64 da,
>> int len)
>>>  	struct rproc_mem_entry *carveout;
>>>  	void *ptr = NULL;
>>>
>>> +	/*
>>> +	 * da_to_va platform driver is deprecated. Driver should register
>>> +	 * carveout thanks to rproc_add_carveout function
>>> +	 */
>>
>> I think this comment is unrelated to the rest of this patch. I also
>> think that at the end of the carveout-rework we should have a patch
>> removing this ops.
> 
> I'll remove this comment and add a da_to_va clean-up patch at the end of the series

da_to_va platform ops is actually used to provide the remoteproc
internal memory translations for the most part, not restricted just to
fixed carveouts. Also, typically these do have multiple address-views -
one the regular bus-address view, and another a remote processor address
view.

regards
Suman

> 
>>
>>>  	if (rproc->ops->da_to_va) {
>>>  		ptr = rproc->ops->da_to_va(rproc, da, len);
>>>  		if (ptr)
>>> @@ -677,6 +681,7 @@ static int rproc_handle_carveout(struct rproc
>> *rproc,
>>>  	struct rproc_mem_entry *carveout, *mapping;
>>>  	struct device *dev = &rproc->dev;
>>>  	dma_addr_t dma;
>>> +	phys_addr_t pa;
>>>  	void *va;
>>>  	int ret;
>>>
>>> @@ -698,6 +703,41 @@ static int rproc_handle_carveout(struct rproc
>> *rproc,
>>>  	if (!carveout)
>>>  		return -ENOMEM;
>>>
>>> +	/* Check carveout rsc already part of a registered carveout */
>>> +	if (rsc->da != FW_RSC_ADDR_ANY) {
>>
>> As mentioned before, I consider it perfectly viable for rsc->da to be
>> ANY and the driver providing a fixed carveout.
> 
> Yes I'll change sequence to lookup by name first and then verify exact parameters matching , not only da definition.
> 
>>
>>> +		va = rproc_find_carveout_by_da(rproc, rsc->da, rsc->len);
>>> +
>>> +		if (va) {
>>
>> In a system with an iommu it's possible that rsc->len is larger than
>> some carveout->len and va is NULL here so we fall through, allocate some
>> memory and remap a segment of the carveout. (Or hopefully fails
>> attempting).
>>
>>> +			/* Registered region found */
>>> +			pa = rproc_va_to_pa(va);
>>> +			if (rsc->pa != FW_RSC_ADDR_ANY && rsc->pa !=
>> (u32)pa) {
>>> +				/* Carveout doesn't match request */
>>> +				dev_err(dev->parent,
>>> +					"Failed to find carveout fitting da and
>> pa\n");
>>> +				return -ENOMEM;
>>> +			}
>>> +
>>> +			/* Update rsc table with physical address */
>>> +			rsc->pa = (u32)pa;
>>> +
>>> +			/* Update carveouts list */
>>> +			carveout->va = va;
>>> +			carveout->len = rsc->len;
>>> +			carveout->da = rsc->da;
>>> +			carveout->priv = (void *)CARVEOUT_RSC;
>>> +
>>> +			list_add_tail(&carveout->node, &rproc->carveouts);
>>
>> rproc_find_carveout_by_da() will return a reference into a carveout, now
>> we add another overlapping carveout into the same list.
>>
>>
>> I think it would be saner to not allow the resource table to describe
>> subsets of carveouts registered by the driver.
>>
>> In which case this would better find a carveout by name or exact da,
>> then check that the pa, da, len and rsc->flags are adequate.
> 
> Agree
> /Loic
>>
>>> +
>>> +			return 0;
>>> +		}
>>> +
>>> +		if (!rproc->domain) {
>>
>> Currently this function ignore invalid values of da when !domain, so I
>> think it would be good you can submit this sanity check in it's own
>> patch so that anyone bisecting this would know why their broken firmware
>> suddenly isn't loadable.
>>
>>> +			dev_err(dev->parent,
>>> +				"Bad carveout rsc configuration\n");
>>> +			return -ENOMEM;
>>> +		}
>>> +	}
>>> +
>>
>> 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
> 

RE: [PATCH v2 05/16] remoteproc: modify rproc_handle_carveout to support preallocated region

[Loic PALLARDY]

> -----Original Message-----
> From: Suman Anna <s-anna@ti.com>
> Sent: mardi 23 octobre 2018 19:40
> To: Loic PALLARDY <loic.pallardy@st.com>; Bjorn Andersson
> <bjorn.andersson@linaro.org>
> 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 v2 05/16] remoteproc: modify rproc_handle_carveout to
> support preallocated region
> 
> >>
> >> On Thu 30 Nov 08:46 PST 2017, Loic Pallardy wrote:
> >>
> >>> In current version rproc_handle_carveout function support only dynamic
> >>> region allocation.
> >>> This patch extends rproc_handle_carveout function to support different
> >> carveout
> >>> configurations:
> >>> - fixed DA and fixed PA: check if already part of pre-registered carveouts
> >>> (platform driver). If no, return error.
> >>> - fixed DA and any PA: check if already part of pre-allocated carveouts
> >>> (platform driver). If not found and rproc supports iommu, continue with
> >>> dynamic allocation (DA will be used for iommu programming), else
> return
> >>> error as no way to force DA.
> >>> - any DA and any PA: use original dynamic allocation
> >>>
> >>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> >>> ---
> >>>  drivers/remoteproc/remoteproc_core.c | 40
> >> ++++++++++++++++++++++++++++++++++++
> >>>  1 file changed, 40 insertions(+)
> >>>
> >>> diff --git a/drivers/remoteproc/remoteproc_core.c
> >> b/drivers/remoteproc/remoteproc_core.c
> >>> index 78525d1..515a17a 100644
> >>> --- a/drivers/remoteproc/remoteproc_core.c
> >>> +++ b/drivers/remoteproc/remoteproc_core.c
> >>> @@ -184,6 +184,10 @@ void *rproc_da_to_va(struct rproc *rproc, u64
> da,
> >> int len)
> >>>  	struct rproc_mem_entry *carveout;
> >>>  	void *ptr = NULL;
> >>>
> >>> +	/*
> >>> +	 * da_to_va platform driver is deprecated. Driver should register
> >>> +	 * carveout thanks to rproc_add_carveout function
> >>> +	 */
> >>
> >> I think this comment is unrelated to the rest of this patch. I also
> >> think that at the end of the carveout-rework we should have a patch
> >> removing this ops.
> >
> > I'll remove this comment and add a da_to_va clean-up patch at the end of
> the series
> 
> da_to_va platform ops is actually used to provide the remoteproc
> internal memory translations for the most part, not restricted just to
> fixed carveouts. Also, typically these do have multiple address-views -
> one the regular bus-address view, and another a remote processor address
> view.

da_to_va op sis still there. I was proposing to remove this ops as we were discussing to register all carveouts accessed by coprocessor in rproc core carveout list.
This will allow to centralize all carveout definitions and to see all memory resources viewed by coprocessor (va, pa and da) via debugfs...

Regards,
Loic
> 
> regards
> Suman
> 
> >
> >>
> >>>  	if (rproc->ops->da_to_va) {
> >>>  		ptr = rproc->ops->da_to_va(rproc, da, len);
> >>>  		if (ptr)
> >>> @@ -677,6 +681,7 @@ static int rproc_handle_carveout(struct rproc
> >> *rproc,
> >>>  	struct rproc_mem_entry *carveout, *mapping;
> >>>  	struct device *dev = &rproc->dev;
> >>>  	dma_addr_t dma;
> >>> +	phys_addr_t pa;
> >>>  	void *va;
> >>>  	int ret;
> >>>
> >>> @@ -698,6 +703,41 @@ static int rproc_handle_carveout(struct rproc
> >> *rproc,
> >>>  	if (!carveout)
> >>>  		return -ENOMEM;
> >>>
> >>> +	/* Check carveout rsc already part of a registered carveout */
> >>> +	if (rsc->da != FW_RSC_ADDR_ANY) {
> >>
> >> As mentioned before, I consider it perfectly viable for rsc->da to be
> >> ANY and the driver providing a fixed carveout.
> >
> > Yes I'll change sequence to lookup by name first and then verify exact
> parameters matching , not only da definition.
> >
> >>
> >>> +		va = rproc_find_carveout_by_da(rproc, rsc->da, rsc->len);
> >>> +
> >>> +		if (va) {
> >>
> >> In a system with an iommu it's possible that rsc->len is larger than
> >> some carveout->len and va is NULL here so we fall through, allocate some
> >> memory and remap a segment of the carveout. (Or hopefully fails
> >> attempting).
> >>
> >>> +			/* Registered region found */
> >>> +			pa = rproc_va_to_pa(va);
> >>> +			if (rsc->pa != FW_RSC_ADDR_ANY && rsc->pa !=
> >> (u32)pa) {
> >>> +				/* Carveout doesn't match request */
> >>> +				dev_err(dev->parent,
> >>> +					"Failed to find carveout fitting da and
> >> pa\n");
> >>> +				return -ENOMEM;
> >>> +			}
> >>> +
> >>> +			/* Update rsc table with physical address */
> >>> +			rsc->pa = (u32)pa;
> >>> +
> >>> +			/* Update carveouts list */
> >>> +			carveout->va = va;
> >>> +			carveout->len = rsc->len;
> >>> +			carveout->da = rsc->da;
> >>> +			carveout->priv = (void *)CARVEOUT_RSC;
> >>> +
> >>> +			list_add_tail(&carveout->node, &rproc->carveouts);
> >>
> >> rproc_find_carveout_by_da() will return a reference into a carveout, now
> >> we add another overlapping carveout into the same list.
> >>
> >>
> >> I think it would be saner to not allow the resource table to describe
> >> subsets of carveouts registered by the driver.
> >>
> >> In which case this would better find a carveout by name or exact da,
> >> then check that the pa, da, len and rsc->flags are adequate.
> >
> > Agree
> > /Loic
> >>
> >>> +
> >>> +			return 0;
> >>> +		}
> >>> +
> >>> +		if (!rproc->domain) {
> >>
> >> Currently this function ignore invalid values of da when !domain, so I
> >> think it would be good you can submit this sanity check in it's own
> >> patch so that anyone bisecting this would know why their broken
> firmware
> >> suddenly isn't loadable.
> >>
> >>> +			dev_err(dev->parent,
> >>> +				"Bad carveout rsc configuration\n");
> >>> +			return -ENOMEM;
> >>> +		}
> >>> +	}
> >>> +
> >>
> >> 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
> >

Re: [PATCH v2 05/16] remoteproc: modify rproc_handle_carveout to support preallocated region

[Suman Anna]

On 10/23/18 2:09 PM, Loic PALLARDY wrote:
> 
> 
>> -----Original Message-----
>> From: Suman Anna <s-anna@ti.com>
>> Sent: mardi 23 octobre 2018 19:40
>> To: Loic PALLARDY <loic.pallardy@st.com>; Bjorn Andersson
>> <bjorn.andersson@linaro.org>
>> 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 v2 05/16] remoteproc: modify rproc_handle_carveout to
>> support preallocated region
>>
>>>>
>>>> On Thu 30 Nov 08:46 PST 2017, Loic Pallardy wrote:
>>>>
>>>>> In current version rproc_handle_carveout function support only dynamic
>>>>> region allocation.
>>>>> This patch extends rproc_handle_carveout function to support different
>>>> carveout
>>>>> configurations:
>>>>> - fixed DA and fixed PA: check if already part of pre-registered carveouts
>>>>> (platform driver). If no, return error.
>>>>> - fixed DA and any PA: check if already part of pre-allocated carveouts
>>>>> (platform driver). If not found and rproc supports iommu, continue with
>>>>> dynamic allocation (DA will be used for iommu programming), else
>> return
>>>>> error as no way to force DA.
>>>>> - any DA and any PA: use original dynamic allocation
>>>>>
>>>>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>>>>> ---
>>>>>  drivers/remoteproc/remoteproc_core.c | 40
>>>> ++++++++++++++++++++++++++++++++++++
>>>>>  1 file changed, 40 insertions(+)
>>>>>
>>>>> diff --git a/drivers/remoteproc/remoteproc_core.c
>>>> b/drivers/remoteproc/remoteproc_core.c
>>>>> index 78525d1..515a17a 100644
>>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>>> @@ -184,6 +184,10 @@ void *rproc_da_to_va(struct rproc *rproc, u64
>> da,
>>>> int len)
>>>>>  	struct rproc_mem_entry *carveout;
>>>>>  	void *ptr = NULL;
>>>>>
>>>>> +	/*
>>>>> +	 * da_to_va platform driver is deprecated. Driver should register
>>>>> +	 * carveout thanks to rproc_add_carveout function
>>>>> +	 */
>>>>
>>>> I think this comment is unrelated to the rest of this patch. I also
>>>> think that at the end of the carveout-rework we should have a patch
>>>> removing this ops.
>>>
>>> I'll remove this comment and add a da_to_va clean-up patch at the end of
>> the series
>>
>> da_to_va platform ops is actually used to provide the remoteproc
>> internal memory translations for the most part, not restricted just to
>> fixed carveouts. Also, typically these do have multiple address-views -
>> one the regular bus-address view, and another a remote processor address
>> view.
> 
> da_to_va op sis still there. I was proposing to remove this ops as we were discussing to register all carveouts accessed by coprocessor in rproc core carveout list.
> This will allow to centralize all carveout definitions and to see all memory resources viewed by coprocessor (va, pa and da) via debugfs...

Yes, understood. I was commenting only on the future removal part, and
if it is really judicious to do that.

regards
Suman

> 
> Regards,
> Loic
>>
>> regards
>> Suman
>>
>>>
>>>>
>>>>>  	if (rproc->ops->da_to_va) {
>>>>>  		ptr = rproc->ops->da_to_va(rproc, da, len);
>>>>>  		if (ptr)
>>>>> @@ -677,6 +681,7 @@ static int rproc_handle_carveout(struct rproc
>>>> *rproc,
>>>>>  	struct rproc_mem_entry *carveout, *mapping;
>>>>>  	struct device *dev = &rproc->dev;
>>>>>  	dma_addr_t dma;
>>>>> +	phys_addr_t pa;
>>>>>  	void *va;
>>>>>  	int ret;
>>>>>
>>>>> @@ -698,6 +703,41 @@ static int rproc_handle_carveout(struct rproc
>>>> *rproc,
>>>>>  	if (!carveout)
>>>>>  		return -ENOMEM;
>>>>>
>>>>> +	/* Check carveout rsc already part of a registered carveout */
>>>>> +	if (rsc->da != FW_RSC_ADDR_ANY) {
>>>>
>>>> As mentioned before, I consider it perfectly viable for rsc->da to be
>>>> ANY and the driver providing a fixed carveout.
>>>
>>> Yes I'll change sequence to lookup by name first and then verify exact
>> parameters matching , not only da definition.
>>>
>>>>
>>>>> +		va = rproc_find_carveout_by_da(rproc, rsc->da, rsc->len);
>>>>> +
>>>>> +		if (va) {
>>>>
>>>> In a system with an iommu it's possible that rsc->len is larger than
>>>> some carveout->len and va is NULL here so we fall through, allocate some
>>>> memory and remap a segment of the carveout. (Or hopefully fails
>>>> attempting).
>>>>
>>>>> +			/* Registered region found */
>>>>> +			pa = rproc_va_to_pa(va);
>>>>> +			if (rsc->pa != FW_RSC_ADDR_ANY && rsc->pa !=
>>>> (u32)pa) {
>>>>> +				/* Carveout doesn't match request */
>>>>> +				dev_err(dev->parent,
>>>>> +					"Failed to find carveout fitting da and
>>>> pa\n");
>>>>> +				return -ENOMEM;
>>>>> +			}
>>>>> +
>>>>> +			/* Update rsc table with physical address */
>>>>> +			rsc->pa = (u32)pa;
>>>>> +
>>>>> +			/* Update carveouts list */
>>>>> +			carveout->va = va;
>>>>> +			carveout->len = rsc->len;
>>>>> +			carveout->da = rsc->da;
>>>>> +			carveout->priv = (void *)CARVEOUT_RSC;
>>>>> +
>>>>> +			list_add_tail(&carveout->node, &rproc->carveouts);
>>>>
>>>> rproc_find_carveout_by_da() will return a reference into a carveout, now
>>>> we add another overlapping carveout into the same list.
>>>>
>>>>
>>>> I think it would be saner to not allow the resource table to describe
>>>> subsets of carveouts registered by the driver.
>>>>
>>>> In which case this would better find a carveout by name or exact da,
>>>> then check that the pa, da, len and rsc->flags are adequate.
>>>
>>> Agree
>>> /Loic
>>>>
>>>>> +
>>>>> +			return 0;
>>>>> +		}
>>>>> +
>>>>> +		if (!rproc->domain) {
>>>>
>>>> Currently this function ignore invalid values of da when !domain, so I
>>>> think it would be good you can submit this sanity check in it's own
>>>> patch so that anyone bisecting this would know why their broken
>> firmware
>>>> suddenly isn't loadable.
>>>>
>>>>> +			dev_err(dev->parent,
>>>>> +				"Bad carveout rsc configuration\n");
>>>>> +			return -ENOMEM;
>>>>> +		}
>>>>> +	}
>>>>> +
>>>>
>>>> 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
>>>
> 

[PATCH v2 06/16] remoteproc: modify vring allocation to support preallocated region

[Loic Pallardy]

Current version of rproc_alloc_vring function supports only dynamic vring
allocation.
This patch extends rproc_alloc_vring to verify if requested vring DA is
already part or not of a registered carveout. If true, nothing to do, else
just allocate vring as before.

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

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 515a17a..bdc99cd 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -263,21 +263,41 @@ 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;
+	dma_addr_t dma = -1;
 	void *va;
 	int ret, size, notifyid;
 
 	/* 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");
-		return -EINVAL;
+	/* get vring resource table pointer */
+	rsc = (void *)rproc->table_ptr + rvdev->rsc_offset;
+
+	if (rsc->vring[i].da != FW_RSC_ADDR_ANY) {
+		va = rproc_find_carveout_by_da(rproc, rsc->vring[i].da, size);
+
+		if (!va) {
+			/* No region not found */
+			dev_err(dev->parent, "Pre-allocated vring not found\n");
+			return -ENOMEM;
+		}
+	} else {
+		/*
+		 * 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");
+			return -EINVAL;
+		}
+		/*
+		 * Let the rproc know the da of this vring.
+		 * Not all platforms use dma_alloc_coherent to automatically
+		 * set up the iommu. In this case the device address (da) will
+		 * hold the physical address and not the device address.
+		 */
+		rsc->vring[i].da = dma;
 	}
 
 	/*
@@ -288,7 +308,8 @@ 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);
+		if (dma != -1)
+			dma_free_coherent(dev->parent, size, va, dma);
 		return ret;
 	}
 	notifyid = ret;
@@ -304,14 +325,7 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 	rvring->dma = dma;
 	rvring->notifyid = notifyid;
 
-	/*
-	 * Let the rproc know the notifyid and da of this vring.
-	 * Not all platforms use dma_alloc_coherent to automatically
-	 * 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;
+	/* Let the rproc know the notifyid of this vring. */
 	rsc->vring[i].notifyid = notifyid;
 	return 0;
 }
@@ -348,7 +362,8 @@ void rproc_free_vring(struct rproc_vring *rvring)
 	int idx = rvring->rvdev->vring - rvring;
 	struct fw_rsc_vdev *rsc;
 
-	dma_free_coherent(rproc->dev.parent, size, rvring->va, rvring->dma);
+	if (rvring->dma != -1)
+		dma_free_coherent(rproc->dev.parent, size, rvring->va, rvring->dma);
 	idr_remove(&rproc->notifyids, rvring->notifyid);
 
 	/* reset resource entry info */
-- 
1.9.1

[PATCH v2 06/16] remoteproc: modify vring allocation to support preallocated region

[Loic Pallardy]

Current version of rproc_alloc_vring function supports only dynamic vring
allocation.
This patch extends rproc_alloc_vring to verify if requested vring DA is
already part or not of a registered carveout. If true, nothing to do, else
just allocate vring as before.

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

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 515a17a..bdc99cd 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -263,21 +263,41 @@ 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;
+	dma_addr_t dma = -1;
 	void *va;
 	int ret, size, notifyid;
 
 	/* 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");
-		return -EINVAL;
+	/* get vring resource table pointer */
+	rsc = (void *)rproc->table_ptr + rvdev->rsc_offset;
+
+	if (rsc->vring[i].da != FW_RSC_ADDR_ANY) {
+		va = rproc_find_carveout_by_da(rproc, rsc->vring[i].da, size);
+
+		if (!va) {
+			/* No region not found */
+			dev_err(dev->parent, "Pre-allocated vring not found\n");
+			return -ENOMEM;
+		}
+	} else {
+		/*
+		 * 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");
+			return -EINVAL;
+		}
+		/*
+		 * Let the rproc know the da of this vring.
+		 * Not all platforms use dma_alloc_coherent to automatically
+		 * set up the iommu. In this case the device address (da) will
+		 * hold the physical address and not the device address.
+		 */
+		rsc->vring[i].da = dma;
 	}
 
 	/*
@@ -288,7 +308,8 @@ 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);
+		if (dma != -1)
+			dma_free_coherent(dev->parent, size, va, dma);
 		return ret;
 	}
 	notifyid = ret;
@@ -304,14 +325,7 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 	rvring->dma = dma;
 	rvring->notifyid = notifyid;
 
-	/*
-	 * Let the rproc know the notifyid and da of this vring.
-	 * Not all platforms use dma_alloc_coherent to automatically
-	 * 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;
+	/* Let the rproc know the notifyid of this vring. */
 	rsc->vring[i].notifyid = notifyid;
 	return 0;
 }
@@ -348,7 +362,8 @@ void rproc_free_vring(struct rproc_vring *rvring)
 	int idx = rvring->rvdev->vring - rvring;
 	struct fw_rsc_vdev *rsc;
 
-	dma_free_coherent(rproc->dev.parent, size, rvring->va, rvring->dma);
+	if (rvring->dma != -1)
+		dma_free_coherent(rproc->dev.parent, size, rvring->va, rvring->dma);
 	idr_remove(&rproc->notifyids, rvring->notifyid);
 
 	/* reset resource entry info */
-- 
1.9.1

Re: [PATCH v2 06/16] remoteproc: modify vring allocation to support preallocated region

[Bjorn Andersson]

On Thu 30 Nov 08:46 PST 2017, Loic Pallardy wrote:

> Current version of rproc_alloc_vring function supports only dynamic vring
> allocation.
> This patch extends rproc_alloc_vring to verify if requested vring DA is
> already part or not of a registered carveout. If true, nothing to do, else
> just allocate vring as before.
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> ---
>  drivers/remoteproc/remoteproc_core.c | 53 +++++++++++++++++++++++-------------
>  1 file changed, 34 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 515a17a..bdc99cd 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -263,21 +263,41 @@ 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;
> +	dma_addr_t dma = -1;
>  	void *va;
>  	int ret, size, notifyid;
>  
>  	/* 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);

This dma_alloc_coherent() should have been a full
rproc_handle_carveout(), so that we don't duplicate the effort of
allocation and setting up the iommu mapping.

> -	if (!va) {
> -		dev_err(dev->parent, "dma_alloc_coherent failed\n");
> -		return -EINVAL;
> +	/* get vring resource table pointer */
> +	rsc = (void *)rproc->table_ptr + rvdev->rsc_offset;
> +
> +	if (rsc->vring[i].da != FW_RSC_ADDR_ANY) {

I think it's reasonable in a system with iommu to specify da, rely on
dynamic allocation and expect the iommu to bet configured.

> +		va = rproc_find_carveout_by_da(rproc, rsc->vring[i].da, size);
> +
> +		if (!va) {
> +			/* No region not found */
> +			dev_err(dev->parent, "Pre-allocated vring not found\n");
> +			return -ENOMEM;
> +		}
[..]
> @@ -304,14 +325,7 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
>  	rvring->dma = dma;
>  	rvring->notifyid = notifyid;
>  
> -	/*
> -	 * Let the rproc know the notifyid and da of this vring.
> -	 * Not all platforms use dma_alloc_coherent to automatically
> -	 * 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;

I prefer that we keep the rsc assignments in a single place.

> +	/* Let the rproc know the notifyid of this vring. */
>  	rsc->vring[i].notifyid = notifyid;
>  	return 0;
>  }
> @@ -348,7 +362,8 @@ void rproc_free_vring(struct rproc_vring *rvring)
>  	int idx = rvring->rvdev->vring - rvring;
>  	struct fw_rsc_vdev *rsc;
>  
> -	dma_free_coherent(rproc->dev.parent, size, rvring->va, rvring->dma);
> +	if (rvring->dma != -1)

This doesn't feel well designed.

> +		dma_free_coherent(rproc->dev.parent, size, rvring->va, rvring->dma);

How about we start by reworking rproc_alloc_vring() to utilize the
carveout handler logic, i.e. when we parse the vring we create (or from
your other patches find an existing) carveout and assign this to rvring.
Then rproc_alloc_vring() becomes a matter of copying the information off
the associated carveout to the rvring and rsc.

This would also simplify the cleanup path as the carveout would be taken
care of by rproc_resource_cleanup(), both in the successful and
unsuccessful cases.

Regards,
Bjorn

RE: [PATCH v2 06/16] remoteproc: modify vring allocation to support preallocated region

[Loic PALLARDY]

> -----Original Message-----
> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]
> Sent: Thursday, December 14, 2017 2:09 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 v2 06/16] remoteproc: modify vring allocation to support
> preallocated region
> 
> On Thu 30 Nov 08:46 PST 2017, Loic Pallardy wrote:
> 
> > Current version of rproc_alloc_vring function supports only dynamic vring
> > allocation.
> > This patch extends rproc_alloc_vring to verify if requested vring DA is
> > already part or not of a registered carveout. If true, nothing to do, else
> > just allocate vring as before.
> >
> > Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> > ---
> >  drivers/remoteproc/remoteproc_core.c | 53
> +++++++++++++++++++++++-------------
> >  1 file changed, 34 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> > index 515a17a..bdc99cd 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -263,21 +263,41 @@ 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;
> > +	dma_addr_t dma = -1;
> >  	void *va;
> >  	int ret, size, notifyid;
> >
> >  	/* 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);
> 
> This dma_alloc_coherent() should have been a full
> rproc_handle_carveout(), so that we don't duplicate the effort of
> allocation and setting up the iommu mapping.

If all memory region are defined as carveout, in that case all allocations will be done before by rproc_handle_carveout
and here we just get access to the area thanks to the carveout name...
> 
> > -	if (!va) {
> > -		dev_err(dev->parent, "dma_alloc_coherent failed\n");
> > -		return -EINVAL;
> > +	/* get vring resource table pointer */
> > +	rsc = (void *)rproc->table_ptr + rvdev->rsc_offset;
> > +
> > +	if (rsc->vring[i].da != FW_RSC_ADDR_ANY) {
> 
> I think it's reasonable in a system with iommu to specify da, rely on
> dynamic allocation and expect the iommu to bet configured.

It is the same as for carveout. Agree to first lookup by name and then check requested resource parameters.
If no region found, in that case we rely on default dynamic allocation.

> 
> > +		va = rproc_find_carveout_by_da(rproc, rsc->vring[i].da,
> size);
> > +
> > +		if (!va) {
> > +			/* No region not found */
> > +			dev_err(dev->parent, "Pre-allocated vring not
> found\n");
> > +			return -ENOMEM;
> > +		}
> [..]
> > @@ -304,14 +325,7 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int
> i)
> >  	rvring->dma = dma;
> >  	rvring->notifyid = notifyid;
> >
> > -	/*
> > -	 * Let the rproc know the notifyid and da of this vring.
> > -	 * Not all platforms use dma_alloc_coherent to automatically
> > -	 * 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;
> 
> I prefer that we keep the rsc assignments in a single place.

Ok

> 
> > +	/* Let the rproc know the notifyid of this vring. */
> >  	rsc->vring[i].notifyid = notifyid;
> >  	return 0;
> >  }
> > @@ -348,7 +362,8 @@ void rproc_free_vring(struct rproc_vring *rvring)
> >  	int idx = rvring->rvdev->vring - rvring;
> >  	struct fw_rsc_vdev *rsc;
> >
> > -	dma_free_coherent(rproc->dev.parent, size, rvring->va, rvring-
> >dma);
> > +	if (rvring->dma != -1)
> 
> This doesn't feel well designed.
> 
> > +		dma_free_coherent(rproc->dev.parent, size, rvring->va,
> rvring->dma);
> 
> How about we start by reworking rproc_alloc_vring() to utilize the
> carveout handler logic, i.e. when we parse the vring we create (or from
> your other patches find an existing) carveout and assign this to rvring.
> Then rproc_alloc_vring() becomes a matter of copying the information off
> the associated carveout to the rvring and rsc.

Yes good idea, if no name match on a registered carveout, rproc_alloc_vring can create one. Like that we are sure to have all the carveout handled at the same location, in the same way.

/Loic
> 
> This would also simplify the cleanup path as the carveout would be taken
> care of by rproc_resource_cleanup(), both in the successful and
> unsuccessful cases.
> 
> Regards,
> Bjorn

[PATCH v2 07/16] 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.
Memory region release is handled by ST driver itself on remove operation.

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

diff --git a/drivers/remoteproc/st_remoteproc.c b/drivers/remoteproc/st_remoteproc.c
index aacef0e..1549ce8 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>
@@ -208,8 +209,10 @@ static int st_rproc_parse_dt(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct rproc *rproc = platform_get_drvdata(pdev);
 	struct st_rproc *ddata = rproc->priv;
-	struct device_node *np = dev->of_node;
-	int err;
+	struct device_node *node, *np = dev->of_node;
+	struct resource res;
+	struct rproc_mem_entry *mem;
+	int err, count, i;
 
 	if (ddata->config->sw_reset) {
 		ddata->sw_reset = devm_reset_control_get_exclusive(dev,
@@ -254,10 +257,36 @@ 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;
+	count = of_count_phandle_with_args(np, "memory-region", NULL);
+
+	for (i = 0; i < count; i++) {
+		node = of_parse_phandle(np, "memory-region", i);
+		if (!node) {
+			dev_err(dev, "No memory-region specified\n");
+			return -EINVAL;
+		}
+
+		err = of_address_to_resource(node, 0, &res);
+		if (err) {
+			dev_err(dev, "Bad memory-region definition\n");
+			return err;
+		}
+
+		mem = devm_kzalloc(dev, sizeof(*mem), GFP_KERNEL);
+		if (!mem)
+			return -ENOMEM;
+
+		mem->dma = res.start;
+		mem->da = res.start;
+		mem->len = resource_size(&res);
+		mem->va = devm_ioremap_wc(dev, mem->dma, mem->len);
+		if (!mem->va) {
+			dev_err(dev, "Unable to map memory region: %pa+%zx\n",
+				&res.start, mem->len);
+			return -EBUSY;
+		}
+
+		rproc_add_carveout(rproc, mem);
 	}
 
 	err = clk_prepare(ddata->clk);
@@ -387,8 +416,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

[PATCH v2 07/16] 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.
Memory region release is handled by ST driver itself on remove operation.

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

diff --git a/drivers/remoteproc/st_remoteproc.c b/drivers/remoteproc/st_remoteproc.c
index aacef0e..1549ce8 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>
@@ -208,8 +209,10 @@ static int st_rproc_parse_dt(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct rproc *rproc = platform_get_drvdata(pdev);
 	struct st_rproc *ddata = rproc->priv;
-	struct device_node *np = dev->of_node;
-	int err;
+	struct device_node *node, *np = dev->of_node;
+	struct resource res;
+	struct rproc_mem_entry *mem;
+	int err, count, i;
 
 	if (ddata->config->sw_reset) {
 		ddata->sw_reset = devm_reset_control_get_exclusive(dev,
@@ -254,10 +257,36 @@ 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;
+	count = of_count_phandle_with_args(np, "memory-region", NULL);
+
+	for (i = 0; i < count; i++) {
+		node = of_parse_phandle(np, "memory-region", i);
+		if (!node) {
+			dev_err(dev, "No memory-region specified\n");
+			return -EINVAL;
+		}
+
+		err = of_address_to_resource(node, 0, &res);
+		if (err) {
+			dev_err(dev, "Bad memory-region definition\n");
+			return err;
+		}
+
+		mem = devm_kzalloc(dev, sizeof(*mem), GFP_KERNEL);
+		if (!mem)
+			return -ENOMEM;
+
+		mem->dma = res.start;
+		mem->da = res.start;
+		mem->len = resource_size(&res);
+		mem->va = devm_ioremap_wc(dev, mem->dma, mem->len);
+		if (!mem->va) {
+			dev_err(dev, "Unable to map memory region: %pa+%zx\n",
+				&res.start, mem->len);
+			return -EBUSY;
+		}
+
+		rproc_add_carveout(rproc, mem);
 	}
 
 	err = clk_prepare(ddata->clk);
@@ -387,8 +416,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 v2 07/16] remoteproc: st: add reserved memory support

[Bjorn Andersson]

On Thu 30 Nov 08:46 PST 2017, Loic Pallardy wrote:

> 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.
> Memory region release is handled by ST driver itself on remove operation.
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> ---
>  drivers/remoteproc/st_remoteproc.c | 43 +++++++++++++++++++++++++++++++-------
>  1 file changed, 35 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/remoteproc/st_remoteproc.c b/drivers/remoteproc/st_remoteproc.c
> index aacef0e..1549ce8 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>
> @@ -208,8 +209,10 @@ static int st_rproc_parse_dt(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct rproc *rproc = platform_get_drvdata(pdev);
>  	struct st_rproc *ddata = rproc->priv;
> -	struct device_node *np = dev->of_node;
> -	int err;
> +	struct device_node *node, *np = dev->of_node;
> +	struct resource res;
> +	struct rproc_mem_entry *mem;
> +	int err, count, i;
>  
>  	if (ddata->config->sw_reset) {
>  		ddata->sw_reset = devm_reset_control_get_exclusive(dev,
> @@ -254,10 +257,36 @@ 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;
> +	count = of_count_phandle_with_args(np, "memory-region", NULL);
> +
> +	for (i = 0; i < count; i++) {

If you use of_phandle_iterator this becomes a little bit more compact
and using of_reserved_mem_lookup() gives you the flexibility of
specifying the reserved-memory using size= in addition to a reg=.

of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
while ((err = of_phandle_iterator_next(&it)) == 0) {
	rmem = of_reserved_mem_lookup(it->node);

	// Memory is rmem->base, rmem->size;
}

> +		node = of_parse_phandle(np, "memory-region", i);
> +		if (!node) {
> +			dev_err(dev, "No memory-region specified\n");
> +			return -EINVAL;
> +		}
> +
> +		err = of_address_to_resource(node, 0, &res);
> +		if (err) {
> +			dev_err(dev, "Bad memory-region definition\n");
> +			return err;
> +		}
> +
> +		mem = devm_kzalloc(dev, sizeof(*mem), GFP_KERNEL);
> +		if (!mem)
> +			return -ENOMEM;
> +
> +		mem->dma = res.start;
> +		mem->da = res.start;
> +		mem->len = resource_size(&res);
> +		mem->va = devm_ioremap_wc(dev, mem->dma, mem->len);
> +		if (!mem->va) {
> +			dev_err(dev, "Unable to map memory region: %pa+%zx\n",
> +				&res.start, mem->len);
> +			return -EBUSY;
> +		}
> +
> +		rproc_add_carveout(rproc, mem);
>  	}

We have a few copies of this logic, how about we move this into a helper
function in the core?

Regards,
Bjorn

RE: [PATCH v2 07/16] remoteproc: st: add reserved memory support

[Loic PALLARDY]

> -----Original Message-----
> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]
> Sent: Thursday, December 14, 2017 2:16 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 v2 07/16] remoteproc: st: add reserved memory support
> 
> On Thu 30 Nov 08:46 PST 2017, Loic Pallardy wrote:
> 
> > 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.
> > Memory region release is handled by ST driver itself on remove operation.
> >
> > Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> > ---
> >  drivers/remoteproc/st_remoteproc.c | 43
> +++++++++++++++++++++++++++++++-------
> >  1 file changed, 35 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/remoteproc/st_remoteproc.c
> b/drivers/remoteproc/st_remoteproc.c
> > index aacef0e..1549ce8 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>
> > @@ -208,8 +209,10 @@ static int st_rproc_parse_dt(struct
> platform_device *pdev)
> >  	struct device *dev = &pdev->dev;
> >  	struct rproc *rproc = platform_get_drvdata(pdev);
> >  	struct st_rproc *ddata = rproc->priv;
> > -	struct device_node *np = dev->of_node;
> > -	int err;
> > +	struct device_node *node, *np = dev->of_node;
> > +	struct resource res;
> > +	struct rproc_mem_entry *mem;
> > +	int err, count, i;
> >
> >  	if (ddata->config->sw_reset) {
> >  		ddata->sw_reset = devm_reset_control_get_exclusive(dev,
> > @@ -254,10 +257,36 @@ 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;
> > +	count = of_count_phandle_with_args(np, "memory-region", NULL);
> > +
> > +	for (i = 0; i < count; i++) {
> 
> If you use of_phandle_iterator this becomes a little bit more compact
> and using of_reserved_mem_lookup() gives you the flexibility of
> specifying the reserved-memory using size= in addition to a reg=.
> 
> of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
> while ((err = of_phandle_iterator_next(&it)) == 0) {
> 	rmem = of_reserved_mem_lookup(it->node);
> 
> 	// Memory is rmem->base, rmem->size;
> }
> 

Good point, thanks

> > +		node = of_parse_phandle(np, "memory-region", i);
> > +		if (!node) {
> > +			dev_err(dev, "No memory-region specified\n");
> > +			return -EINVAL;
> > +		}
> > +
> > +		err = of_address_to_resource(node, 0, &res);
> > +		if (err) {
> > +			dev_err(dev, "Bad memory-region definition\n");
> > +			return err;
> > +		}
> > +
> > +		mem = devm_kzalloc(dev, sizeof(*mem), GFP_KERNEL);
> > +		if (!mem)
> > +			return -ENOMEM;
> > +
> > +		mem->dma = res.start;
> > +		mem->da = res.start;
> > +		mem->len = resource_size(&res);
> > +		mem->va = devm_ioremap_wc(dev, mem->dma, mem-
> >len);
> > +		if (!mem->va) {
> > +			dev_err(dev, "Unable to map memory region:
> %pa+%zx\n",
> > +				&res.start, mem->len);
> > +			return -EBUSY;
> > +		}
> > +
> > +		rproc_add_carveout(rproc, mem);
> >  	}
> 
> We have a few copies of this logic, how about we move this into a helper
> function in the core?
>
Yes, helper function to abstract rproc_mem_entry structure would be nice

Regards,
Loic
 
> Regards,
> Bjorn

[PATCH v2 08/16] remoteproc: add name in rproc_mem_entry struct

[Loic Pallardy]

Add name field in struc 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 bdc99cd..cc53247 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -835,6 +835,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	carveout->da = rsc->da;
 	carveout->release = rproc_release_carveout;
 	carveout->priv = (void *)CARVEOUT_RSC_ALLOCATED;
+	strncpy(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 5bd5175..66e6863 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 {
@@ -324,6 +325,7 @@ struct rproc_mem_entry {
 	u32 da;
 	int (*release)(struct rproc *rproc, struct rproc_mem_entry *mem);
 	void *priv;
+	char name[32];
 	struct list_head node;
 };
 
-- 
1.9.1

[PATCH v2 08/16] remoteproc: add name in rproc_mem_entry struct

[Loic Pallardy]

Add name field in struc 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 bdc99cd..cc53247 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -835,6 +835,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	carveout->da = rsc->da;
 	carveout->release = rproc_release_carveout;
 	carveout->priv = (void *)CARVEOUT_RSC_ALLOCATED;
+	strncpy(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 5bd5175..66e6863 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 {
@@ -324,6 +325,7 @@ struct rproc_mem_entry {
 	u32 da;
 	int (*release)(struct rproc *rproc, struct rproc_mem_entry *mem);
 	void *priv;
+	char name[32];
 	struct list_head node;
 };
 
-- 
1.9.1

Re: [PATCH v2 08/16] remoteproc: add name in rproc_mem_entry struct

[Bjorn Andersson]

On Thu 30 Nov 08:46 PST 2017, Loic Pallardy wrote:

> Add name field in struc 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 bdc99cd..cc53247 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -835,6 +835,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
>  	carveout->da = rsc->da;
>  	carveout->release = rproc_release_carveout;
>  	carveout->priv = (void *)CARVEOUT_RSC_ALLOCATED;
> +	strncpy(carveout->name, rsc->name, sizeof(carveout->name));

Please ensure that this string is NUL terminated.

Regards,
Bjorn

RE: [PATCH v2 08/16] remoteproc: add name in rproc_mem_entry struct

[Loic PALLARDY]

> -----Original Message-----
> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]
> Sent: Thursday, December 14, 2017 2:21 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 v2 08/16] remoteproc: add name in rproc_mem_entry
> struct
> 
> On Thu 30 Nov 08:46 PST 2017, Loic Pallardy wrote:
> 
> > Add name field in struc 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 bdc99cd..cc53247 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -835,6 +835,7 @@ static int rproc_handle_carveout(struct rproc
> *rproc,
> >  	carveout->da = rsc->da;
> >  	carveout->release = rproc_release_carveout;
> >  	carveout->priv = (void *)CARVEOUT_RSC_ALLOCATED;
> > +	strncpy(carveout->name, rsc->name, sizeof(carveout->name));
> 
> Please ensure that this string is NUL terminated.

I'll use strlcpy instead

Regards,
Loic
> 
> Regards,
> Bjorn

[PATCH v2 09/16] remoteproc: add memory device management support

[Loic Pallardy]

This patch add functions to create and delete some
remoteproc sub-devices with dedicated dma coherent region.

These "memory devices" are identified by their name and
will be used for carveout, vring, buffers allocation
in specific memory region.

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

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index cc53247..76d54bf 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -69,6 +69,127 @@ static const char *rproc_crash_to_string(enum rproc_crash_type type)
 	return "unknown";
 }
 
+static phys_addr_t rproc_va_to_pa(void *cpu_addr)
+{
+	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);
+}
+
+struct rproc_memdev {
+	struct device dev;
+	struct rproc *rproc;
+	struct rproc_mem_entry *mem;
+};
+
+#define to_memdevice(d) container_of(d, struct rproc_memdev, dev)
+
+/**
+ * rproc_memdev_release() - release the existence of a memdevice
+ *
+ * @dev: the subdevice's dev
+ */
+static void rproc_memdev_release(struct device *dev)
+{
+	struct rproc_memdev *memd = to_memdevice(dev);
+
+	kfree(memd);
+}
+
+/**
+ * rproc_memdev_add() - add a memory-device on remote processor
+ *
+ * @rproc: the parent remote processor
+ * @mem: memory resource entry allow to define the dma coherent memory of memory-device
+ *
+ * This function add a memory-device child on rproc parent. This memory-device allow
+ * to define a new dma coherent memory area. When the rproc would alloc a
+ * dma coherent memory it's find the memory-device that match with physical memory
+ * asked (if there is no children that match, the rproc is the default device)
+ *
+ * Returns the memory-device handle on success, and error on failure.
+ */
+static struct rproc_memdev *rproc_memdev_add(struct rproc *rproc,
+					     struct rproc_mem_entry *mem)
+{
+	struct rproc_memdev *memd;
+	int ret;
+
+	if (!mem || strlen(mem->name) == 0) {
+		ret = -EINVAL;
+		goto err;
+	}
+
+	memd = kzalloc(sizeof(*memd), GFP_KERNEL);
+	if (!memd) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	memd->rproc = rproc;
+	memd->mem = mem;
+	memd->dev.parent = rproc->dev.parent;
+	memd->dev.release = rproc_memdev_release;
+	dev_set_name(&memd->dev, "%s#%s", dev_name(memd->dev.parent), mem->name);
+	dev_set_drvdata(&memd->dev, memd);
+
+	ret = device_register(&memd->dev);
+	if (ret)
+		goto err_dev;
+
+	ret = dmam_declare_coherent_memory(&memd->dev,
+					   rproc_va_to_pa(mem->va), mem->da,
+					   mem->len,
+					   DMA_MEMORY_EXCLUSIVE);
+	if (ret < 0)
+		goto err_dev;
+
+	return memd;
+
+err_dev:
+	put_device(&memd->dev);
+err:
+	dev_err(&rproc->dev, "unable to register subdev %s, err = %d\n",
+		(mem && strlen(mem->name)) ? mem->name : "unnamed", ret);
+	return ERR_PTR(ret);
+}
+
+/**
+ * rproc_memdev_del() - delete a memory-device of remote processor
+ *
+ * @memdev: rproc memory-device
+ */
+static void rproc_memdev_del(struct rproc_memdev *memdev)
+{
+	if (get_device(&memdev->dev)) {
+		device_unregister(&memdev->dev);
+		put_device(&memdev->dev);
+	}
+}
+
+/**
+ * rproc_memdev_unregister() - unregister memory-device of remote processor
+ *
+ * @dev: rproc memory-device
+ * @data: Not use (just to be compliant with device_for_each_child)
+ *
+ * This function is called by device_for_each_child function when unregister
+ * remote processor.
+ */
+static int rproc_memdev_unregister(struct device *dev, void *data)
+{
+	struct rproc_memdev *memd = to_memdevice(dev);
+	struct rproc *rproc = data;
+
+	if (dev != &rproc->dev)
+		rproc_memdev_del(memd);
+	return 0;
+}
+
 /*
  * This is the IOMMU fault handler we register with the IOMMU API
  * (when relevant; not all remote processors access memory through
@@ -139,17 +260,6 @@ static void rproc_disable_iommu(struct rproc *rproc)
 	iommu_domain_free(domain);
 }
 
-static phys_addr_t rproc_va_to_pa(void *cpu_addr)
-{
-	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
@@ -1678,6 +1788,8 @@ int rproc_del(struct rproc *rproc)
 
 	rproc_delete_debug_dir(rproc);
 
+	device_for_each_child(rproc->dev.parent, rproc,
+			      rproc_memdev_unregister);
 	/* the rproc is downref'ed as soon as it's removed from the klist */
 	mutex_lock(&rproc_list_mutex);
 	list_del(&rproc->node);
-- 
1.9.1

[PATCH v2 09/16] remoteproc: add memory device management support

[Loic Pallardy]

This patch add functions to create and delete some
remoteproc sub-devices with dedicated dma coherent region.

These "memory devices" are identified by their name and
will be used for carveout, vring, buffers allocation
in specific memory region.

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

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index cc53247..76d54bf 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -69,6 +69,127 @@ static const char *rproc_crash_to_string(enum rproc_crash_type type)
 	return "unknown";
 }
 
+static phys_addr_t rproc_va_to_pa(void *cpu_addr)
+{
+	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);
+}
+
+struct rproc_memdev {
+	struct device dev;
+	struct rproc *rproc;
+	struct rproc_mem_entry *mem;
+};
+
+#define to_memdevice(d) container_of(d, struct rproc_memdev, dev)
+
+/**
+ * rproc_memdev_release() - release the existence of a memdevice
+ *
+ * @dev: the subdevice's dev
+ */
+static void rproc_memdev_release(struct device *dev)
+{
+	struct rproc_memdev *memd = to_memdevice(dev);
+
+	kfree(memd);
+}
+
+/**
+ * rproc_memdev_add() - add a memory-device on remote processor
+ *
+ * @rproc: the parent remote processor
+ * @mem: memory resource entry allow to define the dma coherent memory of memory-device
+ *
+ * This function add a memory-device child on rproc parent. This memory-device allow
+ * to define a new dma coherent memory area. When the rproc would alloc a
+ * dma coherent memory it's find the memory-device that match with physical memory
+ * asked (if there is no children that match, the rproc is the default device)
+ *
+ * Returns the memory-device handle on success, and error on failure.
+ */
+static struct rproc_memdev *rproc_memdev_add(struct rproc *rproc,
+					     struct rproc_mem_entry *mem)
+{
+	struct rproc_memdev *memd;
+	int ret;
+
+	if (!mem || strlen(mem->name) == 0) {
+		ret = -EINVAL;
+		goto err;
+	}
+
+	memd = kzalloc(sizeof(*memd), GFP_KERNEL);
+	if (!memd) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	memd->rproc = rproc;
+	memd->mem = mem;
+	memd->dev.parent = rproc->dev.parent;
+	memd->dev.release = rproc_memdev_release;
+	dev_set_name(&memd->dev, "%s#%s", dev_name(memd->dev.parent), mem->name);
+	dev_set_drvdata(&memd->dev, memd);
+
+	ret = device_register(&memd->dev);
+	if (ret)
+		goto err_dev;
+
+	ret = dmam_declare_coherent_memory(&memd->dev,
+					   rproc_va_to_pa(mem->va), mem->da,
+					   mem->len,
+					   DMA_MEMORY_EXCLUSIVE);
+	if (ret < 0)
+		goto err_dev;
+
+	return memd;
+
+err_dev:
+	put_device(&memd->dev);
+err:
+	dev_err(&rproc->dev, "unable to register subdev %s, err = %d\n",
+		(mem && strlen(mem->name)) ? mem->name : "unnamed", ret);
+	return ERR_PTR(ret);
+}
+
+/**
+ * rproc_memdev_del() - delete a memory-device of remote processor
+ *
+ * @memdev: rproc memory-device
+ */
+static void rproc_memdev_del(struct rproc_memdev *memdev)
+{
+	if (get_device(&memdev->dev)) {
+		device_unregister(&memdev->dev);
+		put_device(&memdev->dev);
+	}
+}
+
+/**
+ * rproc_memdev_unregister() - unregister memory-device of remote processor
+ *
+ * @dev: rproc memory-device
+ * @data: Not use (just to be compliant with device_for_each_child)
+ *
+ * This function is called by device_for_each_child function when unregister
+ * remote processor.
+ */
+static int rproc_memdev_unregister(struct device *dev, void *data)
+{
+	struct rproc_memdev *memd = to_memdevice(dev);
+	struct rproc *rproc = data;
+
+	if (dev != &rproc->dev)
+		rproc_memdev_del(memd);
+	return 0;
+}
+
 /*
  * This is the IOMMU fault handler we register with the IOMMU API
  * (when relevant; not all remote processors access memory through
@@ -139,17 +260,6 @@ static void rproc_disable_iommu(struct rproc *rproc)
 	iommu_domain_free(domain);
 }
 
-static phys_addr_t rproc_va_to_pa(void *cpu_addr)
-{
-	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
@@ -1678,6 +1788,8 @@ int rproc_del(struct rproc *rproc)
 
 	rproc_delete_debug_dir(rproc);
 
+	device_for_each_child(rproc->dev.parent, rproc,
+			      rproc_memdev_unregister);
 	/* the rproc is downref'ed as soon as it's removed from the klist */
 	mutex_lock(&rproc_list_mutex);
 	list_del(&rproc->node);
-- 
1.9.1

[PATCH v2 10/16] remoteproc: add memory device registering in rproc_add_carveout

[Loic Pallardy]

Add the possibility to associate a memory device to
carveout.

Due to some memory mapping constraints, remoteproc related memory
allocations should be done in a specific memory region.
Constraint is not coming from remoteproc firmware (with defined
device address), but from remoteproc platform driver itself.

In that case, platform driver has to register a carveout region with
memory device. Memory device will be used for carveout, vring or buffer
allocation accorfing to its name.

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

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 76d54bf..2b7effb 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -964,17 +964,29 @@ static int rproc_handle_carveout(struct rproc *rproc,
  * rproc_add_carveout() - register an allocated carveout region
  * @rproc: rproc handle
  * @mem: memory entry to register
+ * @memdev: true if carveout shoult be associated to a memory device
  *
  * This function registers specified memory entry in @rproc carveouts list.
  * Specified carveout should have been allocated before registering.
  */
-int rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry *mem)
+int rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry *mem, bool memdev)
 {
+	struct rproc_memdev *memd;
+
 	if (!rproc || !mem)
 		return -EINVAL;
 
 	mem->priv = (void *)CARVEOUT_EXTERNAL;
 
+	if (memdev) {
+		memd = rproc_memdev_add(rproc, mem);
+		if (IS_ERR(memd))
+			return -ENOMEM;
+		mem->memdev = memd;
+	} else {
+		mem->memdev = NULL;
+	}
+
 	list_add_tail(&mem->node, &rproc->carveouts);
 
 	return 0;
diff --git a/drivers/remoteproc/st_remoteproc.c b/drivers/remoteproc/st_remoteproc.c
index 1549ce8..da42ec9 100644
--- a/drivers/remoteproc/st_remoteproc.c
+++ b/drivers/remoteproc/st_remoteproc.c
@@ -286,7 +286,7 @@ static int st_rproc_parse_dt(struct platform_device *pdev)
 			return -EBUSY;
 		}
 
-		rproc_add_carveout(rproc, mem);
+		rproc_add_carveout(rproc, mem, false);
 	}
 
 	err = clk_prepare(ddata->clk);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 66e6863..d7e7485 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -325,6 +325,7 @@ struct rproc_mem_entry {
 	u32 da;
 	int (*release)(struct rproc *rproc, struct rproc_mem_entry *mem);
 	void *priv;
+	struct rproc_memdev *memdev;
 	char name[32];
 	struct list_head node;
 };
@@ -524,7 +525,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 int rproc_del(struct rproc *rproc);
 void rproc_free(struct rproc *rproc);
 
-int rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry *mem);
+int rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry *mem, bool memdev);
 
 int rproc_boot(struct rproc *rproc);
 void rproc_shutdown(struct rproc *rproc);
-- 
1.9.1

[PATCH v2 10/16] remoteproc: add memory device registering in rproc_add_carveout

[Loic Pallardy]

Add the possibility to associate a memory device to
carveout.

Due to some memory mapping constraints, remoteproc related memory
allocations should be done in a specific memory region.
Constraint is not coming from remoteproc firmware (with defined
device address), but from remoteproc platform driver itself.

In that case, platform driver has to register a carveout region with
memory device. Memory device will be used for carveout, vring or buffer
allocation accorfing to its name.

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

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 76d54bf..2b7effb 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -964,17 +964,29 @@ static int rproc_handle_carveout(struct rproc *rproc,
  * rproc_add_carveout() - register an allocated carveout region
  * @rproc: rproc handle
  * @mem: memory entry to register
+ * @memdev: true if carveout shoult be associated to a memory device
  *
  * This function registers specified memory entry in @rproc carveouts list.
  * Specified carveout should have been allocated before registering.
  */
-int rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry *mem)
+int rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry *mem, bool memdev)
 {
+	struct rproc_memdev *memd;
+
 	if (!rproc || !mem)
 		return -EINVAL;
 
 	mem->priv = (void *)CARVEOUT_EXTERNAL;
 
+	if (memdev) {
+		memd = rproc_memdev_add(rproc, mem);
+		if (IS_ERR(memd))
+			return -ENOMEM;
+		mem->memdev = memd;
+	} else {
+		mem->memdev = NULL;
+	}
+
 	list_add_tail(&mem->node, &rproc->carveouts);
 
 	return 0;
diff --git a/drivers/remoteproc/st_remoteproc.c b/drivers/remoteproc/st_remoteproc.c
index 1549ce8..da42ec9 100644
--- a/drivers/remoteproc/st_remoteproc.c
+++ b/drivers/remoteproc/st_remoteproc.c
@@ -286,7 +286,7 @@ static int st_rproc_parse_dt(struct platform_device *pdev)
 			return -EBUSY;
 		}
 
-		rproc_add_carveout(rproc, mem);
+		rproc_add_carveout(rproc, mem, false);
 	}
 
 	err = clk_prepare(ddata->clk);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 66e6863..d7e7485 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -325,6 +325,7 @@ struct rproc_mem_entry {
 	u32 da;
 	int (*release)(struct rproc *rproc, struct rproc_mem_entry *mem);
 	void *priv;
+	struct rproc_memdev *memdev;
 	char name[32];
 	struct list_head node;
 };
@@ -524,7 +525,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 int rproc_del(struct rproc *rproc);
 void rproc_free(struct rproc *rproc);
 
-int rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry *mem);
+int rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry *mem, bool memdev);
 
 int rproc_boot(struct rproc *rproc);
 void rproc_shutdown(struct rproc *rproc);
-- 
1.9.1

Re: [PATCH v2 10/16] remoteproc: add memory device registering in rproc_add_carveout

[Bjorn Andersson]

On Thu 30 Nov 08:46 PST 2017, Loic Pallardy wrote:

> Add the possibility to associate a memory device to
> carveout.
> 
> Due to some memory mapping constraints, remoteproc related memory
> allocations should be done in a specific memory region.
> Constraint is not coming from remoteproc firmware (with defined
> device address), but from remoteproc platform driver itself.
> 
> In that case, platform driver has to register a carveout region with
> memory device. Memory device will be used for carveout, vring or buffer
> allocation accorfing to its name.
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> ---
>  drivers/remoteproc/remoteproc_core.c | 14 +++++++++++++-
>  drivers/remoteproc/st_remoteproc.c   |  2 +-
>  include/linux/remoteproc.h           |  3 ++-
>  3 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 76d54bf..2b7effb 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -964,17 +964,29 @@ static int rproc_handle_carveout(struct rproc *rproc,
>   * rproc_add_carveout() - register an allocated carveout region
>   * @rproc: rproc handle
>   * @mem: memory entry to register
> + * @memdev: true if carveout shoult be associated to a memory device
>   *
>   * This function registers specified memory entry in @rproc carveouts list.
>   * Specified carveout should have been allocated before registering.
>   */
> -int rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry *mem)
> +int rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry *mem, bool memdev)
>  {
> +	struct rproc_memdev *memd;
> +
>  	if (!rproc || !mem)
>  		return -EINVAL;
>  
>  	mem->priv = (void *)CARVEOUT_EXTERNAL;
>  
> +	if (memdev) {
> +		memd = rproc_memdev_add(rproc, mem);

But this would likely cause the memory-region to be remapped twice, once by the
caller and once by the dmam_declare_coherent_memory().

> +		if (IS_ERR(memd))
> +			return -ENOMEM;
> +		mem->memdev = memd;
> +	} else {
> +		mem->memdev = NULL;
> +	}
> +
>  	list_add_tail(&mem->node, &rproc->carveouts);
>  
>  	return 0;
> diff --git a/drivers/remoteproc/st_remoteproc.c b/drivers/remoteproc/st_remoteproc.c
> index 1549ce8..da42ec9 100644
> --- a/drivers/remoteproc/st_remoteproc.c
> +++ b/drivers/remoteproc/st_remoteproc.c
> @@ -286,7 +286,7 @@ static int st_rproc_parse_dt(struct platform_device *pdev)
>  			return -EBUSY;
>  		}
>  
> -		rproc_add_carveout(rproc, mem);
> +		rproc_add_carveout(rproc, mem, false);

So when memdev is false this should imply that "mem" has not been
remapped already. Which I think would be better captured by not
overloading the add_carveout function.

Regards,
Bjorn

RE: [PATCH v2 10/16] remoteproc: add memory device registering in rproc_add_carveout

[Loic PALLARDY]

> -----Original Message-----
> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]
> Sent: Thursday, December 14, 2017 2:30 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 v2 10/16] remoteproc: add memory device registering in
> rproc_add_carveout
> 
> On Thu 30 Nov 08:46 PST 2017, Loic Pallardy wrote:
> 
> > Add the possibility to associate a memory device to
> > carveout.
> >
> > Due to some memory mapping constraints, remoteproc related memory
> > allocations should be done in a specific memory region.
> > Constraint is not coming from remoteproc firmware (with defined
> > device address), but from remoteproc platform driver itself.
> >
> > In that case, platform driver has to register a carveout region with
> > memory device. Memory device will be used for carveout, vring or buffer
> > allocation accorfing to its name.
> >
> > Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> > ---
> >  drivers/remoteproc/remoteproc_core.c | 14 +++++++++++++-
> >  drivers/remoteproc/st_remoteproc.c   |  2 +-
> >  include/linux/remoteproc.h           |  3 ++-
> >  3 files changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> > index 76d54bf..2b7effb 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -964,17 +964,29 @@ static int rproc_handle_carveout(struct rproc
> *rproc,
> >   * rproc_add_carveout() - register an allocated carveout region
> >   * @rproc: rproc handle
> >   * @mem: memory entry to register
> > + * @memdev: true if carveout shoult be associated to a memory device
> >   *
> >   * This function registers specified memory entry in @rproc carveouts list.
> >   * Specified carveout should have been allocated before registering.
> >   */
> > -int rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry
> *mem)
> > +int rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry
> *mem, bool memdev)
> >  {
> > +	struct rproc_memdev *memd;
> > +
> >  	if (!rproc || !mem)
> >  		return -EINVAL;
> >
> >  	mem->priv = (void *)CARVEOUT_EXTERNAL;
> >
> > +	if (memdev) {
> > +		memd = rproc_memdev_add(rproc, mem);
> 
> But this would likely cause the memory-region to be remapped twice, once
> by the
> caller and once by the dmam_declare_coherent_memory().

Yes if already mapped by driver

> 
> > +		if (IS_ERR(memd))
> > +			return -ENOMEM;
> > +		mem->memdev = memd;
> > +	} else {
> > +		mem->memdev = NULL;
> > +	}
> > +
> >  	list_add_tail(&mem->node, &rproc->carveouts);
> >
> >  	return 0;
> > diff --git a/drivers/remoteproc/st_remoteproc.c
> b/drivers/remoteproc/st_remoteproc.c
> > index 1549ce8..da42ec9 100644
> > --- a/drivers/remoteproc/st_remoteproc.c
> > +++ b/drivers/remoteproc/st_remoteproc.c
> > @@ -286,7 +286,7 @@ static int st_rproc_parse_dt(struct platform_device
> *pdev)
> >  			return -EBUSY;
> >  		}
> >
> > -		rproc_add_carveout(rproc, mem);
> > +		rproc_add_carveout(rproc, mem, false);
> 
> So when memdev is false this should imply that "mem" has not been
> remapped already. Which I think would be better captured by not
> overloading the add_carveout function.
> 
So you propose to have to different interfaces: one for memory device registry and a second for carveout registry?
In that case user will do memory mapping either by its own or by using memory device registration.

Regards,
Loic

> Regards,
> Bjorn

[PATCH v2 11/16] 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 point on the corresponding
carveout.

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

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 2b7effb..8d990b1 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -367,6 +367,42 @@ void *rproc_find_carveout_by_da(struct rproc *rproc, u64 da, int len)
 	return va;
 }
 
+/**
+ * rproc_find_carveout_by_name() - lookup the carveout region by a name
+ * @rproc: handle of a remote processor
+ * @name: carveout name to find
+ *
+ * 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, char *name)
+{
+	struct rproc_mem_entry *carveout, *mem = NULL;
+
+	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 v2 11/16] 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 point on the corresponding
carveout.

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

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 2b7effb..8d990b1 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -367,6 +367,42 @@ void *rproc_find_carveout_by_da(struct rproc *rproc, u64 da, int len)
 	return va;
 }
 
+/**
+ * rproc_find_carveout_by_name() - lookup the carveout region by a name
+ * @rproc: handle of a remote processor
+ * @name: carveout name to find
+ *
+ * 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, char *name)
+{
+	struct rproc_mem_entry *carveout, *mem = NULL;
+
+	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

Re: [PATCH v2 11/16] remoteproc: introduce rproc_find_carveout_by_name function

[Bjorn Andersson]

On Thu 30 Nov 08:46 PST 2017, Loic Pallardy wrote:
> +struct rproc_mem_entry *
> +rproc_find_carveout_by_name(struct rproc *rproc, char *name)

In almost all cases after this patch you have to do a snprintf(), so it
would be better to make this function format the name based on a format
string and variable arguments.

> +{
> +	struct rproc_mem_entry *carveout, *mem = NULL;
> +
> +	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;
> +}
> +

Regards,
Bjorn

RE: [PATCH v2 11/16] remoteproc: introduce rproc_find_carveout_by_name function

[Loic PALLARDY]

> -----Original Message-----
> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]
> Sent: Thursday, December 14, 2017 2:33 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 v2 11/16] remoteproc: introduce
> rproc_find_carveout_by_name function
> 
> On Thu 30 Nov 08:46 PST 2017, Loic Pallardy wrote:
> > +struct rproc_mem_entry *
> > +rproc_find_carveout_by_name(struct rproc *rproc, char *name)
> 
> In almost all cases after this patch you have to do a snprintf(), so it
> would be better to make this function format the name based on a format
> string and variable arguments.

Good point
/Loic
> 
> > +{
> > +	struct rproc_mem_entry *carveout, *mem = NULL;
> > +
> > +	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;
> > +}
> > +
> 
> Regards,
> Bjorn

[PATCH v2 12/16] remoteproc: look-up memory-device for vring allocation

[Loic Pallardy]

This patch parse existing carveout list to find a memory area
matching on name.

Naming rule for search is the following one:
- "vdev<vdev_id>vring<vring_id>" to find a memory pool dedicated to
one vring belonging to one specific vdev (vdev_id).
- "vdev<vdev_id>vrings> to find common memory pool for allocation of
all vrings belonging to one specific vdev (vdev_id).

This allows to cover different SoC requirements like allocating vrings
at best location taking into account access performance in read and
write for master and slave processors.

If memory area found, memory device will be used as device for vring
allocation, else rproc platform device will be used as today.

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

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 8d990b1..6b5e2b2 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -410,8 +410,11 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 	struct rproc_vring *rvring = &rvdev->vring[i];
 	struct fw_rsc_vdev *rsc;
 	dma_addr_t dma = -1;
+	struct device *memdev = dev->parent;
+	struct rproc_mem_entry *carveout;
 	void *va;
 	int ret, size, notifyid;
+	char name[16];
 
 	/* actual size of vring (in bytes) */
 	size = PAGE_ALIGN(vring_size(rvring->len, rvring->align));
@@ -428,11 +431,27 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 			return -ENOMEM;
 		}
 	} else {
+		/* Find any carveout matching vring */
+		/* Try dedicated vdev j vring i pool. */
+		snprintf(name, sizeof(name), "vdev%dvring%d", rvdev->index, i);
+		carveout = rproc_find_carveout_by_name(rproc, name);
+
+		if (!carveout) {
+			/* Try dedicated vdev j vrings pool. */
+			snprintf(name, sizeof(name), "vdev%dvring", rvdev->index);
+			carveout = rproc_find_carveout_by_name(rproc, name);
+		}
+
+		if (carveout && carveout->memdev)
+			memdev = &carveout->memdev->dev;
+
+		rvring->dev = memdev;
+
 		/*
 		 * 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);
+		va = dma_alloc_coherent(memdev, size, &dma, GFP_KERNEL);
 		if (!va) {
 			dev_err(dev->parent, "dma_alloc_coherent failed\n");
 			return -EINVAL;
@@ -455,7 +474,7 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 	if (ret < 0) {
 		dev_err(dev, "idr_alloc failed: %d\n", ret);
 		if (dma != -1)
-			dma_free_coherent(dev->parent, size, va, dma);
+			dma_free_coherent(memdev, size, va, dma);
 		return ret;
 	}
 	notifyid = ret;
@@ -565,6 +584,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)
@@ -596,6 +616,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 d7e7485..fb293d3 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -480,6 +480,7 @@ struct rproc_subdev {
  * @vq: the virtqueue of this vring
  */
 struct rproc_vring {
+	struct device *dev;
 	void *va;
 	dma_addr_t dma;
 	int len;
@@ -507,6 +508,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 v2 12/16] remoteproc: look-up memory-device for vring allocation

[Loic Pallardy]

This patch parse existing carveout list to find a memory area
matching on name.

Naming rule for search is the following one:
- "vdev<vdev_id>vring<vring_id>" to find a memory pool dedicated to
one vring belonging to one specific vdev (vdev_id).
- "vdev<vdev_id>vrings> to find common memory pool for allocation of
all vrings belonging to one specific vdev (vdev_id).

This allows to cover different SoC requirements like allocating vrings
at best location taking into account access performance in read and
write for master and slave processors.

If memory area found, memory device will be used as device for vring
allocation, else rproc platform device will be used as today.

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

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 8d990b1..6b5e2b2 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -410,8 +410,11 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 	struct rproc_vring *rvring = &rvdev->vring[i];
 	struct fw_rsc_vdev *rsc;
 	dma_addr_t dma = -1;
+	struct device *memdev = dev->parent;
+	struct rproc_mem_entry *carveout;
 	void *va;
 	int ret, size, notifyid;
+	char name[16];
 
 	/* actual size of vring (in bytes) */
 	size = PAGE_ALIGN(vring_size(rvring->len, rvring->align));
@@ -428,11 +431,27 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 			return -ENOMEM;
 		}
 	} else {
+		/* Find any carveout matching vring */
+		/* Try dedicated vdev j vring i pool. */
+		snprintf(name, sizeof(name), "vdev%dvring%d", rvdev->index, i);
+		carveout = rproc_find_carveout_by_name(rproc, name);
+
+		if (!carveout) {
+			/* Try dedicated vdev j vrings pool. */
+			snprintf(name, sizeof(name), "vdev%dvring", rvdev->index);
+			carveout = rproc_find_carveout_by_name(rproc, name);
+		}
+
+		if (carveout && carveout->memdev)
+			memdev = &carveout->memdev->dev;
+
+		rvring->dev = memdev;
+
 		/*
 		 * 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);
+		va = dma_alloc_coherent(memdev, size, &dma, GFP_KERNEL);
 		if (!va) {
 			dev_err(dev->parent, "dma_alloc_coherent failed\n");
 			return -EINVAL;
@@ -455,7 +474,7 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 	if (ret < 0) {
 		dev_err(dev, "idr_alloc failed: %d\n", ret);
 		if (dma != -1)
-			dma_free_coherent(dev->parent, size, va, dma);
+			dma_free_coherent(memdev, size, va, dma);
 		return ret;
 	}
 	notifyid = ret;
@@ -565,6 +584,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)
@@ -596,6 +616,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 d7e7485..fb293d3 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -480,6 +480,7 @@ struct rproc_subdev {
  * @vq: the virtqueue of this vring
  */
 struct rproc_vring {
+	struct device *dev;
 	void *va;
 	dma_addr_t dma;
 	int len;
@@ -507,6 +508,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

Re: [PATCH v2 12/16] remoteproc: look-up memory-device for vring allocation

[Bjorn Andersson]

On Thu 30 Nov 08:46 PST 2017, Loic Pallardy wrote:
>  	} else {
> +		/* Find any carveout matching vring */
> +		/* Try dedicated vdev j vring i pool. */
> +		snprintf(name, sizeof(name), "vdev%dvring%d", rvdev->index, i);
> +		carveout = rproc_find_carveout_by_name(rproc, name);

This might match a carveout representing remapped device memory, which
wouldn't have a memdev so the logic below would silently allocate from
the parent dma_mem instead.

I don't think this is user friendly and would be better to just use the
information in the already mapped carveout.

> +
> +		if (!carveout) {
> +			/* Try dedicated vdev j vrings pool. */
> +			snprintf(name, sizeof(name), "vdev%dvring", rvdev->index);
> +			carveout = rproc_find_carveout_by_name(rproc, name);
> +		}
> +
> +		if (carveout && carveout->memdev)
> +			memdev = &carveout->memdev->dev;
> +
> +		rvring->dev = memdev;
> +
>  		/*
>  		 * 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);
> +		va = dma_alloc_coherent(memdev, size, &dma, GFP_KERNEL);

It's possible that you have fulfilled a resource_table carveout request
with this memory, making the dynamic allocation likely to cause issues.

>  		if (!va) {
>  			dev_err(dev->parent, "dma_alloc_coherent failed\n");
>  			return -EINVAL;

Regards,
Bjorn

RE: [PATCH v2 12/16] remoteproc: look-up memory-device for vring allocation

[Loic PALLARDY]

> -----Original Message-----
> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]
> Sent: Thursday, December 14, 2017 2:44 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 v2 12/16] remoteproc: look-up memory-device for vring
> allocation
> 
> On Thu 30 Nov 08:46 PST 2017, Loic Pallardy wrote:
> >  	} else {
> > +		/* Find any carveout matching vring */
> > +		/* Try dedicated vdev j vring i pool. */
> > +		snprintf(name, sizeof(name), "vdev%dvring%d", rvdev-
> >index, i);
> > +		carveout = rproc_find_carveout_by_name(rproc, name);
> 
> This might match a carveout representing remapped device memory, which
> wouldn't have a memdev so the logic below would silently allocate from
> the parent dma_mem instead.
> 
> I don't think this is user friendly and would be better to just use the
> information in the already mapped carveout.
 
Yes best will be to have a "real" device. If over memory config, dma allocation will rely on dedicated memory pool, else on generic one.
> 
> > +
> > +		if (!carveout) {
> > +			/* Try dedicated vdev j vrings pool. */
> > +			snprintf(name, sizeof(name), "vdev%dvring", rvdev-
> >index);
> > +			carveout = rproc_find_carveout_by_name(rproc,
> name);
> > +		}
> > +
> > +		if (carveout && carveout->memdev)
> > +			memdev = &carveout->memdev->dev;
> > +
> > +		rvring->dev = memdev;
> > +
> >  		/*
> >  		 * 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);
> > +		va = dma_alloc_coherent(memdev, size, &dma,
> GFP_KERNEL);
> 
> It's possible that you have fulfilled a resource_table carveout request
> with this memory, making the dynamic allocation likely to cause issues.
> 
Yes if memory carveout is not dedicated to vring, indeed some other allocations could have been done before...
In that case it means the region is not well sized and firmware and associated resources can't be supported

/Loic
> >  		if (!va) {
> >  			dev_err(dev->parent, "dma_alloc_coherent
> failed\n");
> >  			return -EINVAL;
> 
> Regards,
> Bjorn

[PATCH v2 13/16] remoteproc: look-up memory-device for virtio device allocation

[Loic Pallardy]

This patch parse existing carveout list to find a memory area
matching on "vdev<vdev_id>buffer" name.
If found, memory device will be used as parent for vdev creation, else
rproc platform device will be used as today.

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

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 6b5e2b2..9c12319 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -583,8 +583,11 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
 {
 	struct device *dev = &rproc->dev;
 	struct rproc_vdev *rvdev;
+	struct device *memdev = dev->parent;
+	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)
@@ -637,6 +640,16 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
 
 	list_add_tail(&rvdev->node, &rproc->rvdevs);
 
+	/* Find associated registered carveout. */
+	/* Try dedicated vdev buffer pool. */
+	snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index);
+	carveout = rproc_find_carveout_by_name(rproc, name);
+
+	if (carveout && carveout->memdev)
+		memdev = &carveout->memdev->dev;
+
+	rvdev->dev = memdev;
+
 	rproc_add_subdev(rproc, &rvdev->subdev,
 			 rproc_vdev_do_probe, rproc_vdev_do_remove);
 
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index 2946348..1f7a444 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 fb293d3..b3d6656 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -506,6 +506,7 @@ struct rproc_vdev {
 	struct kref refcount;
 
 	struct rproc_subdev subdev;
+	struct device *dev;
 
 	unsigned int id;
 	unsigned int index;
-- 
1.9.1

[PATCH v2 13/16] remoteproc: look-up memory-device for virtio device allocation

[Loic Pallardy]

This patch parse existing carveout list to find a memory area
matching on "vdev<vdev_id>buffer" name.
If found, memory device will be used as parent for vdev creation, else
rproc platform device will be used as today.

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

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 6b5e2b2..9c12319 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -583,8 +583,11 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
 {
 	struct device *dev = &rproc->dev;
 	struct rproc_vdev *rvdev;
+	struct device *memdev = dev->parent;
+	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)
@@ -637,6 +640,16 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
 
 	list_add_tail(&rvdev->node, &rproc->rvdevs);
 
+	/* Find associated registered carveout. */
+	/* Try dedicated vdev buffer pool. */
+	snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index);
+	carveout = rproc_find_carveout_by_name(rproc, name);
+
+	if (carveout && carveout->memdev)
+		memdev = &carveout->memdev->dev;
+
+	rvdev->dev = memdev;
+
 	rproc_add_subdev(rproc, &rvdev->subdev,
 			 rproc_vdev_do_probe, rproc_vdev_do_remove);
 
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index 2946348..1f7a444 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 fb293d3..b3d6656 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -506,6 +506,7 @@ struct rproc_vdev {
 	struct kref refcount;
 
 	struct rproc_subdev subdev;
+	struct device *dev;
 
 	unsigned int id;
 	unsigned int index;
-- 
1.9.1

Re: [PATCH v2 13/16] remoteproc: look-up memory-device for virtio device allocation

[Bjorn Andersson]

On Thu 30 Nov 08:46 PST 2017, Loic Pallardy wrote:

> This patch parse existing carveout list to find a memory area
> matching on "vdev<vdev_id>buffer" name.
> If found, memory device will be used as parent for vdev creation, else
> rproc platform device will be used as today.
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> ---
>  drivers/remoteproc/remoteproc_core.c   | 13 +++++++++++++
>  drivers/remoteproc/remoteproc_virtio.c |  2 +-
>  include/linux/remoteproc.h             |  1 +
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 6b5e2b2..9c12319 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -583,8 +583,11 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
>  {
>  	struct device *dev = &rproc->dev;
>  	struct rproc_vdev *rvdev;
> +	struct device *memdev = dev->parent;
> +	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)
> @@ -637,6 +640,16 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
>  
>  	list_add_tail(&rvdev->node, &rproc->rvdevs);
>  
> +	/* Find associated registered carveout. */
> +	/* Try dedicated vdev buffer pool. */
> +	snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index);
> +	carveout = rproc_find_carveout_by_name(rproc, name);
> +
> +	if (carveout && carveout->memdev)
> +		memdev = &carveout->memdev->dev;
> +
> +	rvdev->dev = memdev;
> +
>  	rproc_add_subdev(rproc, &rvdev->subdev,
>  			 rproc_vdev_do_probe, rproc_vdev_do_remove);
>  
> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
> index 2946348..1f7a444 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;

This will cause the device structure to change shape based on there
being a match of a carveout or not.


I also think it's preferable to limit the life cycle of the allocations
in this memory region to a single start->stop cycle, rather than
boot->shutdown.

So I think it makes more sense to use the vdev->dev and
dmam_declare_coherent_memory on this. But as in the previous patch this
can't be a carveout that has been remapped already.


A somewhat unrelated topic to this is the ability to associate DT nodes
to rpmsg devices (I do this for the Qualcomm children), in this case we
would have a DT node per vdev under the remoteproc, perhaps it would
make more sense to introduce that and put the memory-region in that
node. Only thin that comes to mind is that we still need to match a
carveout in the resource table, in order to communicate the buffer
region to the remote side for your memory protection purposes.

Regards,
Bjorn

RE: [PATCH v2 13/16] remoteproc: look-up memory-device for virtio device allocation

[Loic PALLARDY]

> -----Original Message-----
> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]
> Sent: Thursday, December 14, 2017 6:33 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 v2 13/16] remoteproc: look-up memory-device for virtio
> device allocation
> 
> On Thu 30 Nov 08:46 PST 2017, Loic Pallardy wrote:
> 
> > This patch parse existing carveout list to find a memory area
> > matching on "vdev<vdev_id>buffer" name.
> > If found, memory device will be used as parent for vdev creation, else
> > rproc platform device will be used as today.
> >
> > Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> > ---
> >  drivers/remoteproc/remoteproc_core.c   | 13 +++++++++++++
> >  drivers/remoteproc/remoteproc_virtio.c |  2 +-
> >  include/linux/remoteproc.h             |  1 +
> >  3 files changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> > index 6b5e2b2..9c12319 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -583,8 +583,11 @@ static int rproc_handle_vdev(struct rproc *rproc,
> struct fw_rsc_vdev *rsc,
> >  {
> >  	struct device *dev = &rproc->dev;
> >  	struct rproc_vdev *rvdev;
> > +	struct device *memdev = dev->parent;
> > +	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)
> > @@ -637,6 +640,16 @@ static int rproc_handle_vdev(struct rproc *rproc,
> struct fw_rsc_vdev *rsc,
> >
> >  	list_add_tail(&rvdev->node, &rproc->rvdevs);
> >
> > +	/* Find associated registered carveout. */
> > +	/* Try dedicated vdev buffer pool. */
> > +	snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index);
> > +	carveout = rproc_find_carveout_by_name(rproc, name);
> > +
> > +	if (carveout && carveout->memdev)
> > +		memdev = &carveout->memdev->dev;
> > +
> > +	rvdev->dev = memdev;
> > +
> >  	rproc_add_subdev(rproc, &rvdev->subdev,
> >  			 rproc_vdev_do_probe, rproc_vdev_do_remove);
> >
> > diff --git a/drivers/remoteproc/remoteproc_virtio.c
> b/drivers/remoteproc/remoteproc_virtio.c
> > index 2946348..1f7a444 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;
> 
> This will cause the device structure to change shape based on there
> being a match of a carveout or not.
> 
> 
> I also think it's preferable to limit the life cycle of the allocations
> in this memory region to a single start->stop cycle, rather than
> boot->shutdown.
> 
> So I think it makes more sense to use the vdev->dev and
> dmam_declare_coherent_memory on this. But as in the previous patch this
> can't be a carveout that has been remapped already.
> 
Yes it is the main issue. Rproc_handle_carveout function will process it before, except if we create a new status to not allocate a carveout and provide function to update carevout in resource table...
> 
> A somewhat unrelated topic to this is the ability to associate DT nodes
> to rpmsg devices (I do this for the Qualcomm children), in this case we
> would have a DT node per vdev under the remoteproc, perhaps it would
> make more sense to introduce that and put the memory-region in that
> node. Only thin that comes to mind is that we still need to match a
> carveout in the resource table, in order to communicate the buffer
> region to the remote side for your memory protection purposes.
> 
OK I'll have a look to Qualcom DT nodes. But as it is SW, is DT the right place?
But it will help to decide if there is a dedicated memory region or not for virtio device.
I'll investigate...

Regards,
Loic

> Regards,
> Bjorn

[PATCH v2 14/16] remoteproc: look-up pre-registered carveout by name for carveout allocation

[Loic Pallardy]

Look-up for a pre-registred carveout having the same
name as requested one.
If match found, pre-registed carevout is used and resource
table updated.

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

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 9c12319..8436185 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -873,7 +873,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, *mem;
 	struct device *dev = &rproc->dev;
 	dma_addr_t dma;
 	phys_addr_t pa;
@@ -899,6 +899,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
 		return -ENOMEM;
 
 	/* Check carveout rsc already part of a registered carveout */
+	/* By device address if any */
 	if (rsc->da != FW_RSC_ADDR_ANY) {
 		va = rproc_find_carveout_by_da(rproc, rsc->da, rsc->len);
 
@@ -933,6 +934,20 @@ static int rproc_handle_carveout(struct rproc *rproc,
 		}
 	}
 
+	/* By name */
+	mem = rproc_find_carveout_by_name(rproc, rsc->name);
+	if (mem) {
+		/*
+		 * Update resource table with registered carevout information
+		 */
+		rsc->len = mem->len;
+		rsc->da = mem->da;
+		rsc->pa = rproc_va_to_pa(mem->va);
+		/* no need to register as already match one for one */
+		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,
-- 
1.9.1

[PATCH v2 14/16] remoteproc: look-up pre-registered carveout by name for carveout allocation

[Loic Pallardy]

Look-up for a pre-registred carveout having the same
name as requested one.
If match found, pre-registed carevout is used and resource
table updated.

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

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 9c12319..8436185 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -873,7 +873,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, *mem;
 	struct device *dev = &rproc->dev;
 	dma_addr_t dma;
 	phys_addr_t pa;
@@ -899,6 +899,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
 		return -ENOMEM;
 
 	/* Check carveout rsc already part of a registered carveout */
+	/* By device address if any */
 	if (rsc->da != FW_RSC_ADDR_ANY) {
 		va = rproc_find_carveout_by_da(rproc, rsc->da, rsc->len);
 
@@ -933,6 +934,20 @@ static int rproc_handle_carveout(struct rproc *rproc,
 		}
 	}
 
+	/* By name */
+	mem = rproc_find_carveout_by_name(rproc, rsc->name);
+	if (mem) {
+		/*
+		 * Update resource table with registered carevout information
+		 */
+		rsc->len = mem->len;
+		rsc->da = mem->da;
+		rsc->pa = rproc_va_to_pa(mem->va);
+		/* no need to register as already match one for one */
+		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,
-- 
1.9.1

Re: [PATCH v2 14/16] remoteproc: look-up pre-registered carveout by name for carveout allocation

[Bjorn Andersson]

On Thu 30 Nov 08:46 PST 2017, Loic Pallardy wrote:
> +	/* By name */
> +	mem = rproc_find_carveout_by_name(rproc, rsc->name);
> +	if (mem) {
> +		/*
> +		 * Update resource table with registered carevout information
> +		 */
> +		rsc->len = mem->len;

You should validate that len, da and pa are compatible.

> +		rsc->da = mem->da;
> +		rsc->pa = rproc_va_to_pa(mem->va);
> +		/* no need to register as already match one for one */
> +		return 0;
> +	}

Regards,
Bjorn

RE: [PATCH v2 14/16] remoteproc: look-up pre-registered carveout by name for carveout allocation

[Loic PALLARDY]

> -----Original Message-----
> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]
> Sent: Thursday, December 14, 2017 6:35 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 v2 14/16] remoteproc: look-up pre-registered carveout
> by name for carveout allocation
> 
> On Thu 30 Nov 08:46 PST 2017, Loic Pallardy wrote:
> > +	/* By name */
> > +	mem = rproc_find_carveout_by_name(rproc, rsc->name);
> > +	if (mem) {
> > +		/*
> > +		 * Update resource table with registered carevout
> information
> > +		 */
> > +		rsc->len = mem->len;
> 
> You should validate that len, da and pa are compatible.
Yes you're right, complete definition must be checked

Regards,
Loic
> 
> > +		rsc->da = mem->da;
> > +		rsc->pa = rproc_va_to_pa(mem->va);
> > +		/* no need to register as already match one for one */
> > +		return 0;
> > +	}
> 
> Regards,
> Bjorn

[PATCH v2 15/16] remoteproc: st: associate memory device to memory regions

[Loic Pallardy]

Enable memory device creation for each memory region added
by rproc_add_carveout function.

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

diff --git a/drivers/remoteproc/st_remoteproc.c b/drivers/remoteproc/st_remoteproc.c
index da42ec9..bf83d82 100644
--- a/drivers/remoteproc/st_remoteproc.c
+++ b/drivers/remoteproc/st_remoteproc.c
@@ -279,6 +279,7 @@ static int st_rproc_parse_dt(struct platform_device *pdev)
 		mem->dma = res.start;
 		mem->da = res.start;
 		mem->len = resource_size(&res);
+		strncpy(mem->name, node->name, sizeof(mem->name));
 		mem->va = devm_ioremap_wc(dev, mem->dma, mem->len);
 		if (!mem->va) {
 			dev_err(dev, "Unable to map memory region: %pa+%zx\n",
@@ -286,7 +287,7 @@ static int st_rproc_parse_dt(struct platform_device *pdev)
 			return -EBUSY;
 		}
 
-		rproc_add_carveout(rproc, mem, false);
+		rproc_add_carveout(rproc, mem, true);
 	}
 
 	err = clk_prepare(ddata->clk);
-- 
1.9.1

[PATCH v2 15/16] remoteproc: st: associate memory device to memory regions

[Loic Pallardy]

Enable memory device creation for each memory region added
by rproc_add_carveout function.

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

diff --git a/drivers/remoteproc/st_remoteproc.c b/drivers/remoteproc/st_remoteproc.c
index da42ec9..bf83d82 100644
--- a/drivers/remoteproc/st_remoteproc.c
+++ b/drivers/remoteproc/st_remoteproc.c
@@ -279,6 +279,7 @@ static int st_rproc_parse_dt(struct platform_device *pdev)
 		mem->dma = res.start;
 		mem->da = res.start;
 		mem->len = resource_size(&res);
+		strncpy(mem->name, node->name, sizeof(mem->name));
 		mem->va = devm_ioremap_wc(dev, mem->dma, mem->len);
 		if (!mem->va) {
 			dev_err(dev, "Unable to map memory region: %pa+%zx\n",
@@ -286,7 +287,7 @@ static int st_rproc_parse_dt(struct platform_device *pdev)
 			return -EBUSY;
 		}
 
-		rproc_add_carveout(rproc, mem, false);
+		rproc_add_carveout(rproc, mem, true);
 	}
 
 	err = clk_prepare(ddata->clk);
-- 
1.9.1

Re: [PATCH v2 15/16] remoteproc: st: associate memory device to memory regions

[Bjorn Andersson]

On Thu 30 Nov 08:46 PST 2017, Loic Pallardy wrote:

> Enable memory device creation for each memory region added
> by rproc_add_carveout function.
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> ---
>  drivers/remoteproc/st_remoteproc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/st_remoteproc.c b/drivers/remoteproc/st_remoteproc.c
> index da42ec9..bf83d82 100644
> --- a/drivers/remoteproc/st_remoteproc.c
> +++ b/drivers/remoteproc/st_remoteproc.c
> @@ -279,6 +279,7 @@ static int st_rproc_parse_dt(struct platform_device *pdev)
>  		mem->dma = res.start;
>  		mem->da = res.start;
>  		mem->len = resource_size(&res);
> +		strncpy(mem->name, node->name, sizeof(mem->name));

Please add this to the previous patch.

>  		mem->va = devm_ioremap_wc(dev, mem->dma, mem->len);
>  		if (!mem->va) {
>  			dev_err(dev, "Unable to map memory region: %pa+%zx\n",
> @@ -286,7 +287,7 @@ static int st_rproc_parse_dt(struct platform_device *pdev)
>  			return -EBUSY;
>  		}
>  
> -		rproc_add_carveout(rproc, mem, false);
> +		rproc_add_carveout(rproc, mem, true);

Here you can see how easy it is to accidentally double-map the memory
region.

Regards,
Bjorn

RE: [PATCH v2 15/16] remoteproc: st: associate memory device to memory regions

[Loic PALLARDY]

> -----Original Message-----
> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]
> Sent: Thursday, December 14, 2017 6:37 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 v2 15/16] remoteproc: st: associate memory device to
> memory regions
> 
> On Thu 30 Nov 08:46 PST 2017, Loic Pallardy wrote:
> 
> > Enable memory device creation for each memory region added
> > by rproc_add_carveout function.
> >
> > Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> > ---
> >  drivers/remoteproc/st_remoteproc.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/remoteproc/st_remoteproc.c
> b/drivers/remoteproc/st_remoteproc.c
> > index da42ec9..bf83d82 100644
> > --- a/drivers/remoteproc/st_remoteproc.c
> > +++ b/drivers/remoteproc/st_remoteproc.c
> > @@ -279,6 +279,7 @@ static int st_rproc_parse_dt(struct platform_device
> *pdev)
> >  		mem->dma = res.start;
> >  		mem->da = res.start;
> >  		mem->len = resource_size(&res);
> > +		strncpy(mem->name, node->name, sizeof(mem->name));
> 
> Please add this to the previous patch.

As indicated in cover letter, I'll some patches to have a coherent series.

/Loic

> 
> >  		mem->va = devm_ioremap_wc(dev, mem->dma, mem-
> >len);
> >  		if (!mem->va) {
> >  			dev_err(dev, "Unable to map memory region:
> %pa+%zx\n",
> > @@ -286,7 +287,7 @@ static int st_rproc_parse_dt(struct platform_device
> *pdev)
> >  			return -EBUSY;
> >  		}
> >
> > -		rproc_add_carveout(rproc, mem, false);
> > +		rproc_add_carveout(rproc, mem, true);
> 
> Here you can see how easy it is to accidentally double-map the memory
> region.
>
> Regards,
> Bjorn

[PATCH v2 16/16] 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 v2 16/16] 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