[PATCH 0/2] Remoteproc: Add predefined coprocessor memory mapping support

[PATCH 0/2] Remoteproc: Add predefined coprocessor memory mapping support

[Loic Pallardy]

Hi all,

Due to diverse constraints like SoC design, coprocessor characteristics,
security context, etc, coprocessor memory mapping may be predefined/fixed at system
level.
In that case remoteproc should respect the different memory regions allocated
to coprocessor in order to ensure proper operations.
The following patches implement support of fixed memory region predefined in
firmware resource table.
During resource table handling, if resource physical address is defined, i.e
different from 0x0 or 0xFFFFFFFF (-1), remoteproc enables memory region access
using memremap function.
If resource physical address is not defined, current behavior is preserved.

Best regards,
Loic

Loic Pallardy (2):
  remoteproc: Modify FW_RSC_ADDR_ANY definition
  remoteproc: core: Add fixed memory region support

 drivers/remoteproc/remoteproc_core.c | 61 ++++++++++++++++++++++++++----------
 include/linux/remoteproc.h           |  6 +++-
 2 files changed, 50 insertions(+), 17 deletions(-)

-- 
1.9.1

[PATCH 0/2] Remoteproc: Add predefined coprocessor memory mapping support

[Loic Pallardy]

Hi all,

Due to diverse constraints like SoC design, coprocessor characteristics,
security context, etc, coprocessor memory mapping may be predefined/fixed at system
level.
In that case remoteproc should respect the different memory regions allocated
to coprocessor in order to ensure proper operations.
The following patches implement support of fixed memory region predefined in
firmware resource table.
During resource table handling, if resource physical address is defined, i.e
different from 0x0 or 0xFFFFFFFF (-1), remoteproc enables memory region access
using memremap function.
If resource physical address is not defined, current behavior is preserved.

Best regards,
Loic

Loic Pallardy (2):
  remoteproc: Modify FW_RSC_ADDR_ANY definition
  remoteproc: core: Add fixed memory region support

 drivers/remoteproc/remoteproc_core.c | 61 ++++++++++++++++++++++++++----------
 include/linux/remoteproc.h           |  6 +++-
 2 files changed, 50 insertions(+), 17 deletions(-)

-- 
1.9.1

[PATCH 1/2] remoteproc: Modify FW_RSC_ADDR_ANY definition

[Loic Pallardy]

Replace 0xFFFFFFFFFFFFFFFF by -1 to fit any types in comparison operations.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 include/linux/remoteproc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index d488f9e..80e1cba 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -118,7 +118,7 @@ enum fw_resource_type {
 	RSC_LAST	= 4,
 };
 
-#define FW_RSC_ADDR_ANY (0xFFFFFFFFFFFFFFFF)
+#define FW_RSC_ADDR_ANY (-1)
 
 /**
  * struct fw_rsc_carveout - physically contiguous memory request
-- 
1.9.1

[PATCH 1/2] remoteproc: Modify FW_RSC_ADDR_ANY definition

[Loic Pallardy]

Replace 0xFFFFFFFFFFFFFFFF by -1 to fit any types in comparison operations.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 include/linux/remoteproc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index d488f9e..80e1cba 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -118,7 +118,7 @@ enum fw_resource_type {
 	RSC_LAST	= 4,
 };
 
-#define FW_RSC_ADDR_ANY (0xFFFFFFFFFFFFFFFF)
+#define FW_RSC_ADDR_ANY (-1)
 
 /**
  * struct fw_rsc_carveout - physically contiguous memory request
-- 
1.9.1

[PATCH 2/2] remoteproc: core: Add fixed memory region support

[Loic Pallardy]

Some coprocessors request fixed memory mapping for firmware execution
and associated communication linked.
Memory resources are defined in firmware resource table.
Resource address different from 0x0 and 0xFFFFFFFF is considered as predefined
and already reserved at system level.
In that case, remoteproc core doesn't need to perform any allocation.
Memory region access can be managed using memremap/memunmap functions

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

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 18f4286..0ddbb92 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -213,13 +213,25 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 	/* 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);
+	rsc = (void *)rproc->table_ptr + rvdev->rsc_offset;
+
+	/* check if specific memory region requested by firmware */
+	if (rsc->vring[i].da != 0 && rsc->vring[i].da != FW_RSC_ADDR_ANY) {
+		va = memremap(rsc->vring[i].da, size, MEMREMAP_WC);
+		rvring->dma = rsc->vring[i].da;
+		rvring->memmap = true;
+	} 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);
+		rvring->dma = dma;
+		rsc->vring[i].da = dma;
+	}
+
 	if (!va) {
-		dev_err(dev->parent, "dma_alloc_coherent failed\n");
+		dev_err(dev->parent, "Failed to get valid ving[%d] va\n", i);
 		return -EINVAL;
 	}
 
@@ -231,7 +243,10 @@ 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 (rvring->memmap)
+			memunmap(rvring->va);
+		else
+			dma_free_coherent(dev->parent, size, va, dma);
 		return ret;
 	}
 	notifyid = ret;
@@ -240,7 +255,6 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 		i, va, &dma, size, notifyid);
 
 	rvring->va = va;
-	rvring->dma = dma;
 	rvring->notifyid = notifyid;
 
 	/*
@@ -249,8 +263,6 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 	 * set up the iommu. In this case the device address (da) will
 	 * hold the physical address and not the device address.
 	 */
-	rsc = (void *)rproc->table_ptr + rvdev->rsc_offset;
-	rsc->vring[i].da = dma;
 	rsc->vring[i].notifyid = notifyid;
 	return 0;
 }
@@ -293,7 +305,11 @@ 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->memmap)
+		memunmap(rvring->va);
+	else
+		dma_free_coherent(rproc->dev.parent, size, rvring->va,
+				  rvring->dma);
 	idr_remove(&rproc->notifyids, rvring->notifyid);
 
 	/* reset resource entry info */
@@ -585,7 +601,15 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	if (!carveout)
 		return -ENOMEM;
 
-	va = dma_alloc_coherent(dev->parent, rsc->len, &dma, GFP_KERNEL);
+	/* check if specific memory region requested by firmware */
+	if (rsc->pa != 0 && rsc->pa != FW_RSC_ADDR_ANY) {
+		va = memremap(rsc->pa, rsc->len, MEMREMAP_WC);
+		carveout->memmap = true;
+		dma = rsc->pa;
+	} else {
+		va = dma_alloc_coherent(dev->parent, rsc->len, &dma, GFP_KERNEL);
+		rsc->pa = dma;
+	}
 	if (!va) {
 		dev_err(dev->parent,
 			"failed to allocate dma memory: len 0x%x\n", rsc->len);
@@ -659,7 +683,6 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	 * In this case, the device address and the physical address
 	 * are the same.
 	 */
-	rsc->pa = dma;
 
 	carveout->va = va;
 	carveout->len = rsc->len;
@@ -673,7 +696,10 @@ static int rproc_handle_carveout(struct rproc *rproc,
 free_mapping:
 	kfree(mapping);
 dma_free:
-	dma_free_coherent(dev->parent, rsc->len, va, dma);
+	if (carveout->memmap)
+		memunmap(va);
+	else
+		dma_free_coherent(dev->parent, rsc->len, va, dma);
 free_carv:
 	kfree(carveout);
 	return ret;
@@ -780,8 +806,11 @@ static void rproc_resource_cleanup(struct rproc *rproc)
 
 	/* clean up carveout allocations */
 	list_for_each_entry_safe(entry, tmp, &rproc->carveouts, node) {
-		dma_free_coherent(dev->parent, entry->len, entry->va,
-				  entry->dma);
+		if (entry->memmap)
+			memunmap(entry->va);
+		else
+			dma_free_coherent(dev->parent, entry->len, entry->va,
+					  entry->dma);
 		list_del(&entry->node);
 		kfree(entry);
 	}
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 80e1cba..ff1fb59 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -312,6 +312,7 @@ struct fw_rsc_vdev {
  * @len: length, in bytes
  * @da: device address
  * @priv: associated data
+ * @memmap: true if memory is memremapped
  * @node: list node
  */
 struct rproc_mem_entry {
@@ -320,6 +321,7 @@ struct rproc_mem_entry {
 	int len;
 	u32 da;
 	void *priv;
+	bool memmap;
 	struct list_head node;
 };
 
@@ -458,6 +460,7 @@ struct rproc {
  * @notifyid: rproc-specific unique vring index
  * @rvdev: remote vdev
  * @vq: the virtqueue of this vring
+ * @memmap: true if memory is memremapped
  */
 struct rproc_vring {
 	void *va;
@@ -468,6 +471,7 @@ struct rproc_vring {
 	int notifyid;
 	struct rproc_vdev *rvdev;
 	struct virtqueue *vq;
+	bool memmap;
 };
 
 /**
-- 
1.9.1

[PATCH 2/2] remoteproc: core: Add fixed memory region support

[Loic Pallardy]

Some coprocessors request fixed memory mapping for firmware execution
and associated communication linked.
Memory resources are defined in firmware resource table.
Resource address different from 0x0 and 0xFFFFFFFF is considered as predefined
and already reserved at system level.
In that case, remoteproc core doesn't need to perform any allocation.
Memory region access can be managed using memremap/memunmap functions

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

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 18f4286..0ddbb92 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -213,13 +213,25 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 	/* 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);
+	rsc = (void *)rproc->table_ptr + rvdev->rsc_offset;
+
+	/* check if specific memory region requested by firmware */
+	if (rsc->vring[i].da != 0 && rsc->vring[i].da != FW_RSC_ADDR_ANY) {
+		va = memremap(rsc->vring[i].da, size, MEMREMAP_WC);
+		rvring->dma = rsc->vring[i].da;
+		rvring->memmap = true;
+	} 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);
+		rvring->dma = dma;
+		rsc->vring[i].da = dma;
+	}
+
 	if (!va) {
-		dev_err(dev->parent, "dma_alloc_coherent failed\n");
+		dev_err(dev->parent, "Failed to get valid ving[%d] va\n", i);
 		return -EINVAL;
 	}
 
@@ -231,7 +243,10 @@ 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 (rvring->memmap)
+			memunmap(rvring->va);
+		else
+			dma_free_coherent(dev->parent, size, va, dma);
 		return ret;
 	}
 	notifyid = ret;
@@ -240,7 +255,6 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 		i, va, &dma, size, notifyid);
 
 	rvring->va = va;
-	rvring->dma = dma;
 	rvring->notifyid = notifyid;
 
 	/*
@@ -249,8 +263,6 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 	 * set up the iommu. In this case the device address (da) will
 	 * hold the physical address and not the device address.
 	 */
-	rsc = (void *)rproc->table_ptr + rvdev->rsc_offset;
-	rsc->vring[i].da = dma;
 	rsc->vring[i].notifyid = notifyid;
 	return 0;
 }
@@ -293,7 +305,11 @@ 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->memmap)
+		memunmap(rvring->va);
+	else
+		dma_free_coherent(rproc->dev.parent, size, rvring->va,
+				  rvring->dma);
 	idr_remove(&rproc->notifyids, rvring->notifyid);
 
 	/* reset resource entry info */
@@ -585,7 +601,15 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	if (!carveout)
 		return -ENOMEM;
 
-	va = dma_alloc_coherent(dev->parent, rsc->len, &dma, GFP_KERNEL);
+	/* check if specific memory region requested by firmware */
+	if (rsc->pa != 0 && rsc->pa != FW_RSC_ADDR_ANY) {
+		va = memremap(rsc->pa, rsc->len, MEMREMAP_WC);
+		carveout->memmap = true;
+		dma = rsc->pa;
+	} else {
+		va = dma_alloc_coherent(dev->parent, rsc->len, &dma, GFP_KERNEL);
+		rsc->pa = dma;
+	}
 	if (!va) {
 		dev_err(dev->parent,
 			"failed to allocate dma memory: len 0x%x\n", rsc->len);
@@ -659,7 +683,6 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	 * In this case, the device address and the physical address
 	 * are the same.
 	 */
-	rsc->pa = dma;
 
 	carveout->va = va;
 	carveout->len = rsc->len;
@@ -673,7 +696,10 @@ static int rproc_handle_carveout(struct rproc *rproc,
 free_mapping:
 	kfree(mapping);
 dma_free:
-	dma_free_coherent(dev->parent, rsc->len, va, dma);
+	if (carveout->memmap)
+		memunmap(va);
+	else
+		dma_free_coherent(dev->parent, rsc->len, va, dma);
 free_carv:
 	kfree(carveout);
 	return ret;
@@ -780,8 +806,11 @@ static void rproc_resource_cleanup(struct rproc *rproc)
 
 	/* clean up carveout allocations */
 	list_for_each_entry_safe(entry, tmp, &rproc->carveouts, node) {
-		dma_free_coherent(dev->parent, entry->len, entry->va,
-				  entry->dma);
+		if (entry->memmap)
+			memunmap(entry->va);
+		else
+			dma_free_coherent(dev->parent, entry->len, entry->va,
+					  entry->dma);
 		list_del(&entry->node);
 		kfree(entry);
 	}
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 80e1cba..ff1fb59 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -312,6 +312,7 @@ struct fw_rsc_vdev {
  * @len: length, in bytes
  * @da: device address
  * @priv: associated data
+ * @memmap: true if memory is memremapped
  * @node: list node
  */
 struct rproc_mem_entry {
@@ -320,6 +321,7 @@ struct rproc_mem_entry {
 	int len;
 	u32 da;
 	void *priv;
+	bool memmap;
 	struct list_head node;
 };
 
@@ -458,6 +460,7 @@ struct rproc {
  * @notifyid: rproc-specific unique vring index
  * @rvdev: remote vdev
  * @vq: the virtqueue of this vring
+ * @memmap: true if memory is memremapped
  */
 struct rproc_vring {
 	void *va;
@@ -468,6 +471,7 @@ struct rproc_vring {
 	int notifyid;
 	struct rproc_vdev *rvdev;
 	struct virtqueue *vq;
+	bool memmap;
 };
 
 /**
-- 
1.9.1

Re: [PATCH 2/2] remoteproc: core: Add fixed memory region support

[Bjorn Andersson]

On Fri 26 Aug 13:19 PDT 2016, Loic Pallardy wrote:

> Some coprocessors request fixed memory mapping for firmware execution
> and associated communication linked.
> Memory resources are defined in firmware resource table.
> Resource address different from 0x0 and 0xFFFFFFFF is considered as predefined

Do you think we're required to support both 0 and -1 for this?

> and already reserved at system level.
> In that case, remoteproc core doesn't need to perform any allocation.
> Memory region access can be managed using memremap/memunmap functions
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> ---
>  drivers/remoteproc/remoteproc_core.c | 61 ++++++++++++++++++++++++++----------
>  include/linux/remoteproc.h           |  4 +++
>  2 files changed, 49 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 18f4286..0ddbb92 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -213,13 +213,25 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
>  	/* 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);
> +	rsc = (void *)rproc->table_ptr + rvdev->rsc_offset;
> +
> +	/* check if specific memory region requested by firmware */
> +	if (rsc->vring[i].da != 0 && rsc->vring[i].da != FW_RSC_ADDR_ANY) {

I think we should convert that reserved field in the vring to a "pa";
allowing this resource to not be 1:1 mapped into the remote. And if
nothing else just to be consistent with the carveouts and devmem.


@Suman, do you have any input on this?

Regards,
Bjorn

Re: [PATCH 2/2] remoteproc: core: Add fixed memory region support

[loic pallardy]

On 08/27/2016 02:32 AM, Bjorn Andersson wrote:
> On Fri 26 Aug 13:19 PDT 2016, Loic Pallardy wrote:
>
>> Some coprocessors request fixed memory mapping for firmware execution
>> and associated communication linked.
>> Memory resources are defined in firmware resource table.
>> Resource address different from 0x0 and 0xFFFFFFFF is considered as predefined
>
> Do you think we're required to support both 0 and -1 for this?
Hi Bjorn,
You're right, only -1 is needed. SoC can have internal RAM in 0x0 for 
example.
I'll update in a V2.
>
>> and already reserved at system level.
>> In that case, remoteproc core doesn't need to perform any allocation.
>> Memory region access can be managed using memremap/memunmap functions
>>
>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>> ---
>>  drivers/remoteproc/remoteproc_core.c | 61 ++++++++++++++++++++++++++----------
>>  include/linux/remoteproc.h           |  4 +++
>>  2 files changed, 49 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index 18f4286..0ddbb92 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -213,13 +213,25 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
>>  	/* 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);
>> +	rsc = (void *)rproc->table_ptr + rvdev->rsc_offset;
>> +
>> +	/* check if specific memory region requested by firmware */
>> +	if (rsc->vring[i].da != 0 && rsc->vring[i].da != FW_RSC_ADDR_ANY) {
>
> I think we should convert that reserved field in the vring to a "pa";
> allowing this resource to not be 1:1 mapped into the remote. And if
> nothing else just to be consistent with the carveouts and devmem.
In fact vring doesn't have pa because coprocessor diretly access it 
without help of hardware accelerator. On both carveout and devmem, 
hardware accelerators may be used.
That's true having pa field will be more consistent from host pov.

Regards,
Loic

>
>
> @Suman, do you have any input on this?
>
> Regards,
> Bjorn
>

Re: [PATCH 2/2] remoteproc: core: Add fixed memory region support

[loic pallardy]

On 08/27/2016 02:32 AM, Bjorn Andersson wrote:
> On Fri 26 Aug 13:19 PDT 2016, Loic Pallardy wrote:
>
>> Some coprocessors request fixed memory mapping for firmware execution
>> and associated communication linked.
>> Memory resources are defined in firmware resource table.
>> Resource address different from 0x0 and 0xFFFFFFFF is considered as predefined
>
> Do you think we're required to support both 0 and -1 for this?
Hi Bjorn,
You're right, only -1 is needed. SoC can have internal RAM in 0x0 for 
example.
I'll update in a V2.
>
>> and already reserved at system level.
>> In that case, remoteproc core doesn't need to perform any allocation.
>> Memory region access can be managed using memremap/memunmap functions
>>
>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>> ---
>>  drivers/remoteproc/remoteproc_core.c | 61 ++++++++++++++++++++++++++----------
>>  include/linux/remoteproc.h           |  4 +++
>>  2 files changed, 49 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index 18f4286..0ddbb92 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -213,13 +213,25 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
>>  	/* 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);
>> +	rsc = (void *)rproc->table_ptr + rvdev->rsc_offset;
>> +
>> +	/* check if specific memory region requested by firmware */
>> +	if (rsc->vring[i].da != 0 && rsc->vring[i].da != FW_RSC_ADDR_ANY) {
>
> I think we should convert that reserved field in the vring to a "pa";
> allowing this resource to not be 1:1 mapped into the remote. And if
> nothing else just to be consistent with the carveouts and devmem.
In fact vring doesn't have pa because coprocessor diretly access it 
without help of hardware accelerator. On both carveout and devmem, 
hardware accelerators may be used.
That's true having pa field will be more consistent from host pov.

Regards,
Loic

>
>
> @Suman, do you have any input on this?
>
> Regards,
> Bjorn
>

Re: [PATCH 2/2] remoteproc: core: Add fixed memory region support

[Suman Anna]

Hi Loic, Bjorn,

On 08/29/2016 03:09 AM, loic pallardy wrote:
> 
> 
> On 08/27/2016 02:32 AM, Bjorn Andersson wrote:
>> On Fri 26 Aug 13:19 PDT 2016, Loic Pallardy wrote:
>>
>>> Some coprocessors request fixed memory mapping for firmware execution
>>> and associated communication linked.
>>> Memory resources are defined in firmware resource table.
>>> Resource address different from 0x0 and 0xFFFFFFFF is considered as
>>> predefined
>>
>> Do you think we're required to support both 0 and -1 for this?
> Hi Bjorn,
> You're right, only -1 is needed. SoC can have internal RAM in 0x0 for
> example.
> I'll update in a V2.
>>
>>> and already reserved at system level.
>>> In that case, remoteproc core doesn't need to perform any allocation.
>>> Memory region access can be managed using memremap/memunmap functions
>>>
>>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>>> ---
>>>  drivers/remoteproc/remoteproc_core.c | 61
>>> ++++++++++++++++++++++++++----------
>>>  include/linux/remoteproc.h           |  4 +++
>>>  2 files changed, 49 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/remoteproc/remoteproc_core.c
>>> b/drivers/remoteproc/remoteproc_core.c
>>> index 18f4286..0ddbb92 100644
>>> --- a/drivers/remoteproc/remoteproc_core.c
>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>> @@ -213,13 +213,25 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev,
>>> int i)
>>>      /* 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);
>>> +    rsc = (void *)rproc->table_ptr + rvdev->rsc_offset;
>>> +
>>> +    /* check if specific memory region requested by firmware */
>>> +    if (rsc->vring[i].da != 0 && rsc->vring[i].da != FW_RSC_ADDR_ANY) {
>>
>> I think we should convert that reserved field in the vring to a "pa";
>> allowing this resource to not be 1:1 mapped into the remote. And if
>> nothing else just to be consistent with the carveouts and devmem.
> In fact vring doesn't have pa because coprocessor diretly access it
> without help of hardware accelerator. On both carveout and devmem,
> hardware accelerators may be used.
> That's true having pa field will be more consistent from host pov.

I agree, and I actually have a need for the pa/dma address without
disturbing the da as well.
> 
> Regards,
> Loic
> 
>>
>> @Suman, do you have any input on this?

I was thinking about this as well, and the way I actually envisioned
this is to add additional rproc_ops with the default behavior falling
back to the dma_alloc API. I had two use-cases in mind for that - one is
the same as what Loic is trying to resolve here, and the other is a case
where I want to allocate these memories not through DMA API, but like
say allocate from an remote processor internal RAM or an on-chip
internal memory. This is the case atleast for vrings and vring buffers.
I think these decisions are best made in the individual platform drivers
as the integration can definitely vary from one SoC to another.

The other thing this series makes an assumption is that with a fixed da,
it is assuming the device is not behind an MMU, and whatever da is
pointing to is a bus accessible address. We have traditional meant the
da as "device address" so it translated as bus address on devices that
are not behind an MMU, or actual virtual addresses as seen by the device
if behind an MMU. On TI SoCs on some devices, we do have an MMU and so
we have a non (-1) da, but it is not valid for memremapping.
At the same time, we would also need any allocated address to be filled in.

regards
Suman

Re: [PATCH 2/2] remoteproc: core: Add fixed memory region support

[Suman Anna]

Hi Loic, Bjorn,

On 08/29/2016 03:09 AM, loic pallardy wrote:
> 
> 
> On 08/27/2016 02:32 AM, Bjorn Andersson wrote:
>> On Fri 26 Aug 13:19 PDT 2016, Loic Pallardy wrote:
>>
>>> Some coprocessors request fixed memory mapping for firmware execution
>>> and associated communication linked.
>>> Memory resources are defined in firmware resource table.
>>> Resource address different from 0x0 and 0xFFFFFFFF is considered as
>>> predefined
>>
>> Do you think we're required to support both 0 and -1 for this?
> Hi Bjorn,
> You're right, only -1 is needed. SoC can have internal RAM in 0x0 for
> example.
> I'll update in a V2.
>>
>>> and already reserved at system level.
>>> In that case, remoteproc core doesn't need to perform any allocation.
>>> Memory region access can be managed using memremap/memunmap functions
>>>
>>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>>> ---
>>>  drivers/remoteproc/remoteproc_core.c | 61
>>> ++++++++++++++++++++++++++----------
>>>  include/linux/remoteproc.h           |  4 +++
>>>  2 files changed, 49 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/remoteproc/remoteproc_core.c
>>> b/drivers/remoteproc/remoteproc_core.c
>>> index 18f4286..0ddbb92 100644
>>> --- a/drivers/remoteproc/remoteproc_core.c
>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>> @@ -213,13 +213,25 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev,
>>> int i)
>>>      /* 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);
>>> +    rsc = (void *)rproc->table_ptr + rvdev->rsc_offset;
>>> +
>>> +    /* check if specific memory region requested by firmware */
>>> +    if (rsc->vring[i].da != 0 && rsc->vring[i].da != FW_RSC_ADDR_ANY) {
>>
>> I think we should convert that reserved field in the vring to a "pa";
>> allowing this resource to not be 1:1 mapped into the remote. And if
>> nothing else just to be consistent with the carveouts and devmem.
> In fact vring doesn't have pa because coprocessor diretly access it
> without help of hardware accelerator. On both carveout and devmem,
> hardware accelerators may be used.
> That's true having pa field will be more consistent from host pov.

I agree, and I actually have a need for the pa/dma address without
disturbing the da as well.
> 
> Regards,
> Loic
> 
>>
>> @Suman, do you have any input on this?

I was thinking about this as well, and the way I actually envisioned
this is to add additional rproc_ops with the default behavior falling
back to the dma_alloc API. I had two use-cases in mind for that - one is
the same as what Loic is trying to resolve here, and the other is a case
where I want to allocate these memories not through DMA API, but like
say allocate from an remote processor internal RAM or an on-chip
internal memory. This is the case atleast for vrings and vring buffers.
I think these decisions are best made in the individual platform drivers
as the integration can definitely vary from one SoC to another.

The other thing this series makes an assumption is that with a fixed da,
it is assuming the device is not behind an MMU, and whatever da is
pointing to is a bus accessible address. We have traditional meant the
da as "device address" so it translated as bus address on devices that
are not behind an MMU, or actual virtual addresses as seen by the device
if behind an MMU. On TI SoCs on some devices, we do have an MMU and so
we have a non (-1) da, but it is not valid for memremapping.
At the same time, we would also need any allocated address to be filled in.

regards
Suman

Re: [PATCH 2/2] remoteproc: core: Add fixed memory region support

[loic pallardy]

On 08/31/2016 01:13 AM, Suman Anna wrote:
>
Hi Suman,
Hi Loic, Bjorn,
>
> On 08/29/2016 03:09 AM, loic pallardy wrote:
>>
>>
>> On 08/27/2016 02:32 AM, Bjorn Andersson wrote:
>>> On Fri 26 Aug 13:19 PDT 2016, Loic Pallardy wrote:
>>>
>>>> Some coprocessors request fixed memory mapping for firmware execution
>>>> and associated communication linked.
>>>> Memory resources are defined in firmware resource table.
>>>> Resource address different from 0x0 and 0xFFFFFFFF is considered as
>>>> predefined
>>>
>>> Do you think we're required to support both 0 and -1 for this?
>> Hi Bjorn,
>> You're right, only -1 is needed. SoC can have internal RAM in 0x0 for
>> example.
>> I'll update in a V2.
>>>
>>>> and already reserved at system level.
>>>> In that case, remoteproc core doesn't need to perform any allocation.
>>>> Memory region access can be managed using memremap/memunmap functions
>>>>
>>>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>>>> ---
>>>>  drivers/remoteproc/remoteproc_core.c | 61
>>>> ++++++++++++++++++++++++++----------
>>>>  include/linux/remoteproc.h           |  4 +++
>>>>  2 files changed, 49 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/drivers/remoteproc/remoteproc_core.c
>>>> b/drivers/remoteproc/remoteproc_core.c
>>>> index 18f4286..0ddbb92 100644
>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>> @@ -213,13 +213,25 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev,
>>>> int i)
>>>>      /* 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);
>>>> +    rsc = (void *)rproc->table_ptr + rvdev->rsc_offset;
>>>> +
>>>> +    /* check if specific memory region requested by firmware */
>>>> +    if (rsc->vring[i].da != 0 && rsc->vring[i].da != FW_RSC_ADDR_ANY) {
>>>
>>> I think we should convert that reserved field in the vring to a "pa";
>>> allowing this resource to not be 1:1 mapped into the remote. And if
>>> nothing else just to be consistent with the carveouts and devmem.
>> In fact vring doesn't have pa because coprocessor diretly access it
>> without help of hardware accelerator. On both carveout and devmem,
>> hardware accelerators may be used.
>> That's true having pa field will be more consistent from host pov.
>
> I agree, and I actually have a need for the pa/dma address without
> disturbing the da as well.
>>
>> Regards,
>> Loic
>>
>>>
>>> @Suman, do you have any input on this?
>
> I was thinking about this as well, and the way I actually envisioned
> this is to add additional rproc_ops with the default behavior falling
> back to the dma_alloc API. I had two use-cases in mind for that - one is
> the same as what Loic is trying to resolve here, and the other is a case
> where I want to allocate these memories not through DMA API, but like
> say allocate from an remote processor internal RAM or an on-chip
> internal memory. This is the case atleast for vrings and vring buffers.
> I think these decisions are best made in the individual platform drivers
> as the integration can definitely vary from one SoC to another.
>
Same use cases on ST side.
Indeed we can create some new ops to alloc and release memory chunk at 
driver level.
Driver will be free to use reserved memeory framework or simply memremap 
chunks.
I'll propose a patch for that.

> The other thing this series makes an assumption is that with a fixed da,
> it is assuming the device is not behind an MMU, and whatever da is
> pointing to is a bus accessible address. We have traditional meant the
> da as "device address" so it translated as bus address on devices that
> are not behind an MMU, or actual virtual addresses as seen by the device
> if behind an MMU. On TI SoCs on some devices, we do have an MMU and so
> we have a non (-1) da, but it is not valid for memremapping.
> At the same time, we would also need any allocated address to be filled in.
>
Today da is used because there is no other field which can be used to 
fix this resource. But agree da is only for device address and pa is 
missing.
If all agree I'll convert reserved field in pa.

Thanks for your review.
Regards,
Loic
> regards
> Suman
>

Re: [PATCH 2/2] remoteproc: core: Add fixed memory region support

[loic pallardy]

On 08/31/2016 01:13 AM, Suman Anna wrote:
>
Hi Suman,
Hi Loic, Bjorn,
>
> On 08/29/2016 03:09 AM, loic pallardy wrote:
>>
>>
>> On 08/27/2016 02:32 AM, Bjorn Andersson wrote:
>>> On Fri 26 Aug 13:19 PDT 2016, Loic Pallardy wrote:
>>>
>>>> Some coprocessors request fixed memory mapping for firmware execution
>>>> and associated communication linked.
>>>> Memory resources are defined in firmware resource table.
>>>> Resource address different from 0x0 and 0xFFFFFFFF is considered as
>>>> predefined
>>>
>>> Do you think we're required to support both 0 and -1 for this?
>> Hi Bjorn,
>> You're right, only -1 is needed. SoC can have internal RAM in 0x0 for
>> example.
>> I'll update in a V2.
>>>
>>>> and already reserved at system level.
>>>> In that case, remoteproc core doesn't need to perform any allocation.
>>>> Memory region access can be managed using memremap/memunmap functions
>>>>
>>>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>>>> ---
>>>>  drivers/remoteproc/remoteproc_core.c | 61
>>>> ++++++++++++++++++++++++++----------
>>>>  include/linux/remoteproc.h           |  4 +++
>>>>  2 files changed, 49 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/drivers/remoteproc/remoteproc_core.c
>>>> b/drivers/remoteproc/remoteproc_core.c
>>>> index 18f4286..0ddbb92 100644
>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>> @@ -213,13 +213,25 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev,
>>>> int i)
>>>>      /* 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);
>>>> +    rsc = (void *)rproc->table_ptr + rvdev->rsc_offset;
>>>> +
>>>> +    /* check if specific memory region requested by firmware */
>>>> +    if (rsc->vring[i].da != 0 && rsc->vring[i].da != FW_RSC_ADDR_ANY) {
>>>
>>> I think we should convert that reserved field in the vring to a "pa";
>>> allowing this resource to not be 1:1 mapped into the remote. And if
>>> nothing else just to be consistent with the carveouts and devmem.
>> In fact vring doesn't have pa because coprocessor diretly access it
>> without help of hardware accelerator. On both carveout and devmem,
>> hardware accelerators may be used.
>> That's true having pa field will be more consistent from host pov.
>
> I agree, and I actually have a need for the pa/dma address without
> disturbing the da as well.
>>
>> Regards,
>> Loic
>>
>>>
>>> @Suman, do you have any input on this?
>
> I was thinking about this as well, and the way I actually envisioned
> this is to add additional rproc_ops with the default behavior falling
> back to the dma_alloc API. I had two use-cases in mind for that - one is
> the same as what Loic is trying to resolve here, and the other is a case
> where I want to allocate these memories not through DMA API, but like
> say allocate from an remote processor internal RAM or an on-chip
> internal memory. This is the case atleast for vrings and vring buffers.
> I think these decisions are best made in the individual platform drivers
> as the integration can definitely vary from one SoC to another.
>
Same use cases on ST side.
Indeed we can create some new ops to alloc and release memory chunk at 
driver level.
Driver will be free to use reserved memeory framework or simply memremap 
chunks.
I'll propose a patch for that.

> The other thing this series makes an assumption is that with a fixed da,
> it is assuming the device is not behind an MMU, and whatever da is
> pointing to is a bus accessible address. We have traditional meant the
> da as "device address" so it translated as bus address on devices that
> are not behind an MMU, or actual virtual addresses as seen by the device
> if behind an MMU. On TI SoCs on some devices, we do have an MMU and so
> we have a non (-1) da, but it is not valid for memremapping.
> At the same time, we would also need any allocated address to be filled in.
>
Today da is used because there is no other field which can be used to 
fix this resource. But agree da is only for device address and pa is 
missing.
If all agree I'll convert reserved field in pa.

Thanks for your review.
Regards,
Loic
> regards
> Suman
>

Re: [PATCH 2/2] remoteproc: core: Add fixed memory region support

[Bjorn Andersson]

On Tue 30 Aug 16:13 PDT 2016, Suman Anna wrote:

> >>> +    if (rsc->vring[i].da != 0 && rsc->vring[i].da != FW_RSC_ADDR_ANY) {
[..]
> >> @Suman, do you have any input on this?
> 

Thanks Suman

> I was thinking about this as well, and the way I actually envisioned
> this is to add additional rproc_ops with the default behavior falling
> back to the dma_alloc API. I had two use-cases in mind for that - one is
> the same as what Loic is trying to resolve here, and the other is a case
> where I want to allocate these memories not through DMA API, but like
> say allocate from an remote processor internal RAM or an on-chip
> internal memory.

Are these cases a matter of mapping the chunks with ioremap, or are
there more fancy setups that would affect how we load the data into them
as well?

Also, in the case of you mapping vrings in on-chip memory, would you use
the resource table to communicate these addresses or are they simply
hard coded in the loaded firmware?

> This is the case atleast for vrings and vring buffers.
> I think these decisions are best made in the individual platform drivers
> as the integration can definitely vary from one SoC to another.
> 

This touches upon the discussion related to how to support fixed
position vring buffers.

> The other thing this series makes an assumption is that with a fixed da,
> it is assuming the device is not behind an MMU, and whatever da is
> pointing to is a bus accessible address.

But doesn't the current code do the same?
Isn't the "dma" that we assign "da" the physical address of the memory?

> We have traditional meant the
> da as "device address" so it translated as bus address on devices that
> are not behind an MMU, or actual virtual addresses as seen by the device
> if behind an MMU.

I like the idea of making this the uniform design among the various
resource types.

> On TI SoCs on some devices, we do have an MMU and so
> we have a non (-1) da, but it is not valid for memremapping.
> At the same time, we would also need any allocated address to be filled in.

Right, so analog to the carveout case we need to allocate memory and
potentially map the memory in the iommu.

As this case then repeats itself for the vring (rpmsg) buffers I think
we should strive for representing and handling all these memory
allocations in a more uniform way.

Regards,
Bjorn

Re: [PATCH 2/2] remoteproc: core: Add fixed memory region support

[Suman Anna]

On 08/31/2016 11:37 AM, Bjorn Andersson wrote:
> On Tue 30 Aug 16:13 PDT 2016, Suman Anna wrote:
> 
>>>>> +    if (rsc->vring[i].da != 0 && rsc->vring[i].da != FW_RSC_ADDR_ANY) {
> [..]
>>>> @Suman, do you have any input on this?
>>
> 
> Thanks Suman
> 
>> I was thinking about this as well, and the way I actually envisioned
>> this is to add additional rproc_ops with the default behavior falling
>> back to the dma_alloc API. I had two use-cases in mind for that - one is
>> the same as what Loic is trying to resolve here, and the other is a case
>> where I want to allocate these memories not through DMA API, but like
>> say allocate from an remote processor internal RAM or an on-chip
>> internal memory.
> 
> Are these cases a matter of mapping the chunks with ioremap, or are
> there more fancy setups that would affect how we load the data into them
> as well?

The loading can be handled automatically as we do provide the
.da_to_va() ops for individual platform drivers to provide a translation
mechanism. The default ELF loader only needs the kernel va to be able to
copy the data over. In the case of fixed addresses, it is just a matter
of ioremap, but if using the mmio-sram driver, the platform drivers are
responsible for providing you the va and dma.

> 
> Also, in the case of you mapping vrings in on-chip memory, would you use
> the resource table to communicate these addresses or are they simply
> hard coded in the loaded firmware?

It really depends on how the on-chip memory get used. Unless there is a
limitation to use a fixed address location, the normal usage would be
used the mmio-sram driver and the gen_pool API to allocate on-chip
memory. We do fill in the resource table to communicate these addresses
to loaded firmware.

> 
>> This is the case atleast for vrings and vring buffers.
>> I think these decisions are best made in the individual platform drivers
>> as the integration can definitely vary from one SoC to another.
>>
> 
> This touches upon the discussion related to how to support fixed
> position vring buffers.

Indeed, in one of the usage patterns.

> 
>> The other thing this series makes an assumption is that with a fixed da,
>> it is assuming the device is not behind an MMU, and whatever da is
>> pointing to is a bus accessible address.
> 
> But doesn't the current code do the same?
> Isn't the "dma" that we assign "da" the physical address of the memory?

I meant this series is making the assumption. Previously, we were
ignoring the da and overwriting it with the allocated physical address
field, right.

> 
>> We have traditional meant the
>> da as "device address" so it translated as bus address on devices that
>> are not behind an MMU, or actual virtual addresses as seen by the device
>> if behind an MMU.
> 
> I like the idea of making this the uniform design among the various
> resource types.
> 
>> On TI SoCs on some devices, we do have an MMU and so
>> we have a non (-1) da, but it is not valid for memremapping.
>> At the same time, we would also need any allocated address to be filled in.
> 
> Right, so analog to the carveout case we need to allocate memory and
> potentially map the memory in the iommu.
> 
> As this case then repeats itself for the vring (rpmsg) buffers I think
> we should strive for representing and handling all these memory
> allocations in a more uniform way.

Yes agreed. Though there are currently some gaps w.r.t the vrings and
the vring buffers mapping, as the current code doesn't have an
associated iommu_map calls around the allocation. It might be that the
remoteproc would require these to be allocated/mapped in a specific
region for properly configuring the cacheability property around this.
We are using a work-around for the moment to get around this.

regards
Suman

Re: [PATCH 2/2] remoteproc: core: Add fixed memory region support

[Suman Anna]

On 08/31/2016 11:37 AM, Bjorn Andersson wrote:
> On Tue 30 Aug 16:13 PDT 2016, Suman Anna wrote:
> 
>>>>> +    if (rsc->vring[i].da != 0 && rsc->vring[i].da != FW_RSC_ADDR_ANY) {
> [..]
>>>> @Suman, do you have any input on this?
>>
> 
> Thanks Suman
> 
>> I was thinking about this as well, and the way I actually envisioned
>> this is to add additional rproc_ops with the default behavior falling
>> back to the dma_alloc API. I had two use-cases in mind for that - one is
>> the same as what Loic is trying to resolve here, and the other is a case
>> where I want to allocate these memories not through DMA API, but like
>> say allocate from an remote processor internal RAM or an on-chip
>> internal memory.
> 
> Are these cases a matter of mapping the chunks with ioremap, or are
> there more fancy setups that would affect how we load the data into them
> as well?

The loading can be handled automatically as we do provide the
.da_to_va() ops for individual platform drivers to provide a translation
mechanism. The default ELF loader only needs the kernel va to be able to
copy the data over. In the case of fixed addresses, it is just a matter
of ioremap, but if using the mmio-sram driver, the platform drivers are
responsible for providing you the va and dma.

> 
> Also, in the case of you mapping vrings in on-chip memory, would you use
> the resource table to communicate these addresses or are they simply
> hard coded in the loaded firmware?

It really depends on how the on-chip memory get used. Unless there is a
limitation to use a fixed address location, the normal usage would be
used the mmio-sram driver and the gen_pool API to allocate on-chip
memory. We do fill in the resource table to communicate these addresses
to loaded firmware.

> 
>> This is the case atleast for vrings and vring buffers.
>> I think these decisions are best made in the individual platform drivers
>> as the integration can definitely vary from one SoC to another.
>>
> 
> This touches upon the discussion related to how to support fixed
> position vring buffers.

Indeed, in one of the usage patterns.

> 
>> The other thing this series makes an assumption is that with a fixed da,
>> it is assuming the device is not behind an MMU, and whatever da is
>> pointing to is a bus accessible address.
> 
> But doesn't the current code do the same?
> Isn't the "dma" that we assign "da" the physical address of the memory?

I meant this series is making the assumption. Previously, we were
ignoring the da and overwriting it with the allocated physical address
field, right.

> 
>> We have traditional meant the
>> da as "device address" so it translated as bus address on devices that
>> are not behind an MMU, or actual virtual addresses as seen by the device
>> if behind an MMU.
> 
> I like the idea of making this the uniform design among the various
> resource types.
> 
>> On TI SoCs on some devices, we do have an MMU and so
>> we have a non (-1) da, but it is not valid for memremapping.
>> At the same time, we would also need any allocated address to be filled in.
> 
> Right, so analog to the carveout case we need to allocate memory and
> potentially map the memory in the iommu.
> 
> As this case then repeats itself for the vring (rpmsg) buffers I think
> we should strive for representing and handling all these memory
> allocations in a more uniform way.

Yes agreed. Though there are currently some gaps w.r.t the vrings and
the vring buffers mapping, as the current code doesn't have an
associated iommu_map calls around the allocation. It might be that the
remoteproc would require these to be allocated/mapped in a specific
region for properly configuring the cacheability property around this.
We are using a work-around for the moment to get around this.

regards
Suman