* [PATCH v2 0/3] Remoteproc: Add predefined coprocessor memory mapping support @ 2016-09-06 7:39 Loic Pallardy 2016-09-06 7:39 ` [PATCH v2 1/3] remoteproc: Modify FW_RSC_ADDR_ANY definition Loic Pallardy ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Loic Pallardy @ 2016-09-06 7:39 UTC (permalink / raw) To: bjorn.andersson, ohad, lee.jones Cc: loic.pallardy, linux-remoteproc, linux-kernel, kernel 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 V2 introduces new specific-platform ops for memory allocation. This new ops is called before standard dma_alloc_coherent to let platform-specific driver handle specific use cases like internal SRAM or fixed memory mapping. Moreover to have the capability to fix vring location and have homogenous resource management, reserved vring resource field is transformed in physical address. To preserve compatibility with existing firmware for which reserved field is equal to 0, physical address set to 0 is interpreted as any like -1. Loic Pallardy (3): remoteproc: Modify FW_RSC_ADDR_ANY definition remoteproc: core: transform struct fw_rsc_vdev_vring reserved field in pa remoteproc: core: add rproc ops for memory allocation drivers/remoteproc/remoteproc_core.c | 73 +++++++++++++++++++++++++++--------- include/linux/remoteproc.h | 10 +++-- 2 files changed, 63 insertions(+), 20 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/3] remoteproc: Modify FW_RSC_ADDR_ANY definition 2016-09-06 7:39 [PATCH v2 0/3] Remoteproc: Add predefined coprocessor memory mapping support Loic Pallardy @ 2016-09-06 7:39 ` Loic Pallardy 2016-09-06 18:10 ` Bjorn Andersson 2016-09-06 7:39 ` [PATCH v2 2/3] remoteproc: core: transform struct fw_rsc_vdev_vring reserved field in pa Loic Pallardy 2016-09-06 7:39 ` [PATCH v2 3/3] remoteproc: core: add rproc ops for memory allocation Loic Pallardy 2 siblings, 1 reply; 10+ messages in thread From: Loic Pallardy @ 2016-09-06 7:39 UTC (permalink / raw) To: bjorn.andersson, ohad, lee.jones Cc: loic.pallardy, linux-remoteproc, linux-kernel, kernel Replace 0xFFFFFFFFFFFFFFFF by -1 to fit any type. 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/3] remoteproc: Modify FW_RSC_ADDR_ANY definition 2016-09-06 7:39 ` [PATCH v2 1/3] remoteproc: Modify FW_RSC_ADDR_ANY definition Loic Pallardy @ 2016-09-06 18:10 ` Bjorn Andersson 0 siblings, 0 replies; 10+ messages in thread From: Bjorn Andersson @ 2016-09-06 18:10 UTC (permalink / raw) To: Loic Pallardy; +Cc: ohad, lee.jones, linux-remoteproc, linux-kernel, kernel On Tue 06 Sep 00:39 PDT 2016, Loic Pallardy wrote: > Replace 0xFFFFFFFFFFFFFFFF by -1 to fit any type. > > Signed-off-by: Loic Pallardy <loic.pallardy@st.com> Applied, thanks > --- > 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/3] remoteproc: core: transform struct fw_rsc_vdev_vring reserved field in pa 2016-09-06 7:39 [PATCH v2 0/3] Remoteproc: Add predefined coprocessor memory mapping support Loic Pallardy 2016-09-06 7:39 ` [PATCH v2 1/3] remoteproc: Modify FW_RSC_ADDR_ANY definition Loic Pallardy @ 2016-09-06 7:39 ` Loic Pallardy 2016-09-06 18:10 ` Bjorn Andersson 2016-09-06 7:39 ` [PATCH v2 3/3] remoteproc: core: add rproc ops for memory allocation Loic Pallardy 2 siblings, 1 reply; 10+ messages in thread From: Loic Pallardy @ 2016-09-06 7:39 UTC (permalink / raw) To: bjorn.andersson, ohad, lee.jones Cc: loic.pallardy, linux-remoteproc, linux-kernel, kernel In current implementation, struct fw_rsc_vdev_vring which describes vring resource in firmware resource table owns only device address, because it assumes that host is responsible of vring allocation and only device address is needed by coprocessor. But if vrings need to be fixe in system memory map for any reasons (security, SoC charactieristics...), physical address is needed exatly identified the memory chunck by host. For that let's transform reserved field of struct fw_rsc_vdev_vring to pa (physical address). Signed-off-by: Loic Pallardy <loic.pallardy@st.com> --- drivers/remoteproc/remoteproc_core.c | 6 ------ include/linux/remoteproc.h | 4 ++-- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index 18f4286..0d3c191 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -266,12 +266,6 @@ rproc_parse_vring(struct rproc_vdev *rvdev, struct fw_rsc_vdev *rsc, int i) dev_dbg(dev, "vdev rsc: vring%d: da 0x%x, qsz %d, align %d\n", i, vring->da, vring->num, vring->align); - /* make sure reserved bytes are zeroes */ - if (vring->reserved) { - dev_err(dev, "vring rsc has non zero reserved bytes\n"); - return -EINVAL; - } - /* verify queue size and vring alignment are sane */ if (!vring->num || !vring->align) { dev_err(dev, "invalid qsz (%d) or alignment (%d)\n", diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h index 80e1cba..c321eab 100644 --- a/include/linux/remoteproc.h +++ b/include/linux/remoteproc.h @@ -241,7 +241,7 @@ struct fw_rsc_trace { * @notifyid is a unique rproc-wide notify index for this vring. This notify * index is used when kicking a remote processor, to let it know that this * vring is triggered. - * @reserved: reserved (must be zero) + * @pa: physical address * * This descriptor is not a resource entry by itself; it is part of the * vdev resource type (see below). @@ -255,7 +255,7 @@ struct fw_rsc_vdev_vring { u32 align; u32 num; u32 notifyid; - u32 reserved; + u32 pa; } __packed; /** -- 1.9.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/3] remoteproc: core: transform struct fw_rsc_vdev_vring reserved field in pa 2016-09-06 7:39 ` [PATCH v2 2/3] remoteproc: core: transform struct fw_rsc_vdev_vring reserved field in pa Loic Pallardy @ 2016-09-06 18:10 ` Bjorn Andersson 0 siblings, 0 replies; 10+ messages in thread From: Bjorn Andersson @ 2016-09-06 18:10 UTC (permalink / raw) To: Loic Pallardy; +Cc: ohad, lee.jones, linux-remoteproc, linux-kernel, kernel On Tue 06 Sep 00:39 PDT 2016, Loic Pallardy wrote: > In current implementation, struct fw_rsc_vdev_vring which describes > vring resource in firmware resource table owns only device address, > because it assumes that host is responsible of vring allocation and > only device address is needed by coprocessor. > But if vrings need to be fixe in system memory map for any reasons > (security, SoC charactieristics...), physical address is needed exatly > identified the memory chunck by host. > > For that let's transform reserved field of struct fw_rsc_vdev_vring > to pa (physical address). > > Signed-off-by: Loic Pallardy <loic.pallardy@st.com> Applied, thanks > --- > drivers/remoteproc/remoteproc_core.c | 6 ------ > include/linux/remoteproc.h | 4 ++-- > 2 files changed, 2 insertions(+), 8 deletions(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index 18f4286..0d3c191 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -266,12 +266,6 @@ rproc_parse_vring(struct rproc_vdev *rvdev, struct fw_rsc_vdev *rsc, int i) > dev_dbg(dev, "vdev rsc: vring%d: da 0x%x, qsz %d, align %d\n", > i, vring->da, vring->num, vring->align); > > - /* make sure reserved bytes are zeroes */ > - if (vring->reserved) { > - dev_err(dev, "vring rsc has non zero reserved bytes\n"); > - return -EINVAL; > - } > - > /* verify queue size and vring alignment are sane */ > if (!vring->num || !vring->align) { > dev_err(dev, "invalid qsz (%d) or alignment (%d)\n", > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > index 80e1cba..c321eab 100644 > --- a/include/linux/remoteproc.h > +++ b/include/linux/remoteproc.h > @@ -241,7 +241,7 @@ struct fw_rsc_trace { > * @notifyid is a unique rproc-wide notify index for this vring. This notify > * index is used when kicking a remote processor, to let it know that this > * vring is triggered. > - * @reserved: reserved (must be zero) > + * @pa: physical address > * > * This descriptor is not a resource entry by itself; it is part of the > * vdev resource type (see below). > @@ -255,7 +255,7 @@ struct fw_rsc_vdev_vring { > u32 align; > u32 num; > u32 notifyid; > - u32 reserved; > + u32 pa; > } __packed; > > /** > -- > 1.9.1 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 3/3] remoteproc: core: add rproc ops for memory allocation 2016-09-06 7:39 [PATCH v2 0/3] Remoteproc: Add predefined coprocessor memory mapping support Loic Pallardy 2016-09-06 7:39 ` [PATCH v2 1/3] remoteproc: Modify FW_RSC_ADDR_ANY definition Loic Pallardy 2016-09-06 7:39 ` [PATCH v2 2/3] remoteproc: core: transform struct fw_rsc_vdev_vring reserved field in pa Loic Pallardy @ 2016-09-06 7:39 ` Loic Pallardy 2016-09-08 14:06 ` Lee Jones 2016-09-15 17:27 ` Bjorn Andersson 2 siblings, 2 replies; 10+ messages in thread From: Loic Pallardy @ 2016-09-06 7:39 UTC (permalink / raw) To: bjorn.andersson, ohad, lee.jones Cc: loic.pallardy, linux-remoteproc, linux-kernel, kernel Remoteproc core is currently using dma_alloc_coherent for carveout and vring allocation. It doesn't allow to support specific use cases like fixed memory region or internal RAM support. Two new rproc ops (alloc and free) is added to provide flexibility to platform implementation to provide specific memory allocator taking into account coprocessor characteristics. rproc_handle_carveout and rproc_alloc_vring functions are modified to invoke these ops if present, and fallback to regular processing if platform specific allocation failed and if resquested memory is not fixed (physical address == FW_RSC_ADDR_ANY) Signed-off-by: Loic Pallardy <loic.pallardy@st.com> --- drivers/remoteproc/remoteproc_core.c | 67 ++++++++++++++++++++++++++++++------ include/linux/remoteproc.h | 4 +++ 2 files changed, 60 insertions(+), 11 deletions(-) diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index 0d3c191..7493b08 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -207,19 +207,29 @@ 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; - void *va; + void *va = NULL; int ret, size, notifyid; /* actual size of vring (in bytes) */ size = PAGE_ALIGN(vring_size(rvring->len, rvring->align)); + rsc = (void *)rproc->table_ptr + rvdev->rsc_offset; + /* * 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); + + dma = rsc->vring[i].pa; + + if (rproc->ops->alloc) + va = rproc->ops->alloc(rproc, size, &dma); + + if (!va && rsc->vring[i].pa == FW_RSC_ADDR_ANY) + va = dma_alloc_coherent(dev->parent, size, &dma, GFP_KERNEL); + 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 +241,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 (rproc->ops->free) + ret = rproc->ops->free(rproc, size, va, dma); + if (!ret) + dma_free_coherent(dev->parent, size, va, dma); return ret; } notifyid = ret; @@ -249,8 +262,8 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i) * set up the iommu. In this case the device address (da) will * hold the physical address and not the device address. */ - rsc = (void *)rproc->table_ptr + rvdev->rsc_offset; rsc->vring[i].da = dma; + rsc->vring[i].pa = dma; rsc->vring[i].notifyid = notifyid; return 0; } @@ -273,6 +286,15 @@ rproc_parse_vring(struct rproc_vdev *rvdev, struct fw_rsc_vdev *rsc, int i) return -EINVAL; } + /* + * pa field was previously reserved and fixed to 0 + * used FW_RSC_ADDR_ANY as default value if 0 detected + * to keep backward compatibility and have vring allocated + * by dma_alloc_coherent + */ + if (vring->pa == 0) + vring->pa = FW_RSC_ADDR_ANY; + rvring->len = vring->num; rvring->align = vring->align; rvring->rvdev = rvdev; @@ -286,8 +308,15 @@ void rproc_free_vring(struct rproc_vring *rvring) struct rproc *rproc = rvring->rvdev->rproc; int idx = rvring->rvdev->vring - rvring; struct fw_rsc_vdev *rsc; + int ret = 0; + + if (rproc->ops->free) + ret = rproc->ops->free(rproc, size, rvring->va, rvring->dma); + + if (!ret) + dma_free_coherent(rproc->dev.parent, size, rvring->va, + rvring->dma); - dma_free_coherent(rproc->dev.parent, size, rvring->va, rvring->dma); idr_remove(&rproc->notifyids, rvring->notifyid); /* reset resource entry info */ @@ -558,7 +587,7 @@ static int rproc_handle_carveout(struct rproc *rproc, struct rproc_mem_entry *carveout, *mapping; struct device *dev = &rproc->dev; dma_addr_t dma; - void *va; + void *va = NULL; int ret; if (sizeof(*rsc) > avail) { @@ -579,7 +608,15 @@ static int rproc_handle_carveout(struct rproc *rproc, if (!carveout) return -ENOMEM; - va = dma_alloc_coherent(dev->parent, rsc->len, &dma, GFP_KERNEL); + dma = rsc->pa; + /* first try platform-specific allocator */ + if (rproc->ops->alloc) + va = rproc->ops->alloc(rproc, rsc->len, &dma); + + /* use standad method only if region not fixed */ + if (!va && rsc->pa == FW_RSC_ADDR_ANY) + va = dma_alloc_coherent(dev->parent, rsc->len, &dma, GFP_KERNEL); + if (!va) { dev_err(dev->parent, "failed to allocate dma memory: len 0x%x\n", rsc->len); @@ -667,7 +704,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 (rproc->ops->free) + ret = rproc->ops->free(rproc, rsc->len, va, dma); + if (!ret) + dma_free_coherent(dev->parent, rsc->len, va, dma); free_carv: kfree(carveout); return ret; @@ -748,6 +788,7 @@ static void rproc_resource_cleanup(struct rproc *rproc) struct rproc_mem_entry *entry, *tmp; struct rproc_vdev *rvdev, *rvtmp; struct device *dev = &rproc->dev; + int ret = 0; /* clean up debugfs trace entries */ list_for_each_entry_safe(entry, tmp, &rproc->traces, node) { @@ -774,8 +815,12 @@ 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 (rproc->ops->free) + ret = rproc->ops->free(rproc, entry->len, entry->va, + entry->dma); + if (!ret) + 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 c321eab..b2f8227 100644 --- a/include/linux/remoteproc.h +++ b/include/linux/remoteproc.h @@ -331,12 +331,16 @@ struct rproc; * @stop: power off the device * @kick: kick a virtqueue (virtqueue id given as a parameter) * @da_to_va: optional platform hook to perform address translations + * @alloc: alloc requested memory chunck + * @free: release specified memory chunck */ struct rproc_ops { int (*start)(struct rproc *rproc); int (*stop)(struct rproc *rproc); void (*kick)(struct rproc *rproc, int vqid); void * (*da_to_va)(struct rproc *rproc, u64 da, int len); + void * (*alloc)(struct rproc *rproc, int size, dma_addr_t *dma_handle); + int (*free)(struct rproc *rproc, size_t size, void *cpu_addr, dma_addr_t dma_handle); }; /** -- 1.9.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/3] remoteproc: core: add rproc ops for memory allocation 2016-09-06 7:39 ` [PATCH v2 3/3] remoteproc: core: add rproc ops for memory allocation Loic Pallardy @ 2016-09-08 14:06 ` Lee Jones 2016-09-15 17:27 ` Bjorn Andersson 1 sibling, 0 replies; 10+ messages in thread From: Lee Jones @ 2016-09-08 14:06 UTC (permalink / raw) To: Loic Pallardy Cc: bjorn.andersson, ohad, linux-remoteproc, linux-kernel, kernel On Tue, 06 Sep 2016, Loic Pallardy wrote: > Remoteproc core is currently using dma_alloc_coherent for > carveout and vring allocation. > It doesn't allow to support specific use cases like fixed memory > region or internal RAM support. > > Two new rproc ops (alloc and free) is added to provide flexibility > to platform implementation to provide specific memory allocator > taking into account coprocessor characteristics. > rproc_handle_carveout and rproc_alloc_vring functions are modified > to invoke these ops if present, and fallback to regular processing > if platform specific allocation failed and if resquested memory is > not fixed (physical address == FW_RSC_ADDR_ANY) > > Signed-off-by: Loic Pallardy <loic.pallardy@st.com> > --- > drivers/remoteproc/remoteproc_core.c | 67 ++++++++++++++++++++++++++++++------ > include/linux/remoteproc.h | 4 +++ > 2 files changed, 60 insertions(+), 11 deletions(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index 0d3c191..7493b08 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -207,19 +207,29 @@ 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; > - void *va; > + void *va = NULL; > int ret, size, notifyid; > > /* actual size of vring (in bytes) */ > size = PAGE_ALIGN(vring_size(rvring->len, rvring->align)); > > + rsc = (void *)rproc->table_ptr + rvdev->rsc_offset; > + > /* > * 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); > + > + dma = rsc->vring[i].pa; > + > + if (rproc->ops->alloc) > + va = rproc->ops->alloc(rproc, size, &dma); > + > + if (!va && rsc->vring[i].pa == FW_RSC_ADDR_ANY) > + va = dma_alloc_coherent(dev->parent, size, &dma, GFP_KERNEL); > + > if (!va) { > - dev_err(dev->parent, "dma_alloc_coherent failed\n"); > + dev_err(dev->parent, "Failed to get valid ving[%d] va\n", i); Error messages isn't the place for abbreviations IMO. "Failed to allocate memory for ... XXX" > return -EINVAL; > } > > @@ -231,7 +241,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 (rproc->ops->free) > + ret = rproc->ops->free(rproc, size, va, dma); > + if (!ret) > + dma_free_coherent(dev->parent, size, va, dma); Are you sure this is what you want to do? Won't this free the memory twice? Looking at this *very* briefly, shouldn't this be something like: else if (va) dma_free_coherent(dev->parent, size, va, dma); > return ret; > } > notifyid = ret; > @@ -249,8 +262,8 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i) > * set up the iommu. In this case the device address (da) will > * hold the physical address and not the device address. > */ > - rsc = (void *)rproc->table_ptr + rvdev->rsc_offset; > rsc->vring[i].da = dma; > + rsc->vring[i].pa = dma; > rsc->vring[i].notifyid = notifyid; > return 0; > } > @@ -273,6 +286,15 @@ rproc_parse_vring(struct rproc_vdev *rvdev, struct fw_rsc_vdev *rsc, int i) > return -EINVAL; > } > > + /* > + * pa field was previously reserved and fixed to 0 > + * used FW_RSC_ADDR_ANY as default value if 0 detected > + * to keep backward compatibility and have vring allocated > + * by dma_alloc_coherent > + */ > + if (vring->pa == 0) > + vring->pa = FW_RSC_ADDR_ANY; > + > rvring->len = vring->num; > rvring->align = vring->align; > rvring->rvdev = rvdev; > @@ -286,8 +308,15 @@ void rproc_free_vring(struct rproc_vring *rvring) > struct rproc *rproc = rvring->rvdev->rproc; > int idx = rvring->rvdev->vring - rvring; > struct fw_rsc_vdev *rsc; > + int ret = 0; > + > + if (rproc->ops->free) > + ret = rproc->ops->free(rproc, size, rvring->va, rvring->dma); > + > + if (!ret) > + dma_free_coherent(rproc->dev.parent, size, rvring->va, > + rvring->dma); Same here. > - dma_free_coherent(rproc->dev.parent, size, rvring->va, rvring->dma); > idr_remove(&rproc->notifyids, rvring->notifyid); > > /* reset resource entry info */ > @@ -558,7 +587,7 @@ static int rproc_handle_carveout(struct rproc *rproc, > struct rproc_mem_entry *carveout, *mapping; > struct device *dev = &rproc->dev; > dma_addr_t dma; > - void *va; > + void *va = NULL; > int ret; > > if (sizeof(*rsc) > avail) { > @@ -579,7 +608,15 @@ static int rproc_handle_carveout(struct rproc *rproc, > if (!carveout) > return -ENOMEM; > > - va = dma_alloc_coherent(dev->parent, rsc->len, &dma, GFP_KERNEL); > + dma = rsc->pa; > + /* first try platform-specific allocator */ Same comment throughout about comments. > + if (rproc->ops->alloc) > + va = rproc->ops->alloc(rproc, rsc->len, &dma); > + > + /* use standad method only if region not fixed */ > + if (!va && rsc->pa == FW_RSC_ADDR_ANY) > + va = dma_alloc_coherent(dev->parent, rsc->len, &dma, GFP_KERNEL); > + > if (!va) { > dev_err(dev->parent, > "failed to allocate dma memory: len 0x%x\n", rsc->len); > @@ -667,7 +704,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 (rproc->ops->free) > + ret = rproc->ops->free(rproc, rsc->len, va, dma); > + if (!ret) > + dma_free_coherent(dev->parent, rsc->len, va, dma); > free_carv: > kfree(carveout); > return ret; > @@ -748,6 +788,7 @@ static void rproc_resource_cleanup(struct rproc *rproc) > struct rproc_mem_entry *entry, *tmp; > struct rproc_vdev *rvdev, *rvtmp; > struct device *dev = &rproc->dev; > + int ret = 0; > > /* clean up debugfs trace entries */ > list_for_each_entry_safe(entry, tmp, &rproc->traces, node) { > @@ -774,8 +815,12 @@ 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 (rproc->ops->free) > + ret = rproc->ops->free(rproc, entry->len, entry->va, > + entry->dma); > + if (!ret) > + dma_free_coherent(dev->parent, entry->len, entry->va, > + entry->dma); And here I guess. > list_del(&entry->node); > kfree(entry); > } > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > index c321eab..b2f8227 100644 > --- a/include/linux/remoteproc.h > +++ b/include/linux/remoteproc.h > @@ -331,12 +331,16 @@ struct rproc; > * @stop: power off the device > * @kick: kick a virtqueue (virtqueue id given as a parameter) > * @da_to_va: optional platform hook to perform address translations > + * @alloc: alloc requested memory chunck > + * @free: release specified memory chunck > */ > struct rproc_ops { > int (*start)(struct rproc *rproc); > int (*stop)(struct rproc *rproc); > void (*kick)(struct rproc *rproc, int vqid); > void * (*da_to_va)(struct rproc *rproc, u64 da, int len); > + void * (*alloc)(struct rproc *rproc, int size, dma_addr_t *dma_handle); > + int (*free)(struct rproc *rproc, size_t size, void *cpu_addr, dma_addr_t dma_handle); > }; > > /** -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/3] remoteproc: core: add rproc ops for memory allocation 2016-09-06 7:39 ` [PATCH v2 3/3] remoteproc: core: add rproc ops for memory allocation Loic Pallardy 2016-09-08 14:06 ` Lee Jones @ 2016-09-15 17:27 ` Bjorn Andersson 2016-09-16 7:47 ` loic pallardy 1 sibling, 1 reply; 10+ messages in thread From: Bjorn Andersson @ 2016-09-15 17:27 UTC (permalink / raw) To: Loic Pallardy; +Cc: ohad, lee.jones, linux-remoteproc, linux-kernel, kernel On Tue 06 Sep 00:39 PDT 2016, Loic Pallardy wrote: > Remoteproc core is currently using dma_alloc_coherent for > carveout and vring allocation. > It doesn't allow to support specific use cases like fixed memory > region or internal RAM support. > > Two new rproc ops (alloc and free) is added to provide flexibility > to platform implementation to provide specific memory allocator > taking into account coprocessor characteristics. > rproc_handle_carveout and rproc_alloc_vring functions are modified > to invoke these ops if present, and fallback to regular processing > if platform specific allocation failed and if resquested memory is > not fixed (physical address == FW_RSC_ADDR_ANY) > > Signed-off-by: Loic Pallardy <loic.pallardy@st.com> > --- > drivers/remoteproc/remoteproc_core.c | 67 ++++++++++++++++++++++++++++++------ > include/linux/remoteproc.h | 4 +++ > 2 files changed, 60 insertions(+), 11 deletions(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index 0d3c191..7493b08 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -207,19 +207,29 @@ 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; > - void *va; > + void *va = NULL; > int ret, size, notifyid; > > /* actual size of vring (in bytes) */ > size = PAGE_ALIGN(vring_size(rvring->len, rvring->align)); > > + rsc = (void *)rproc->table_ptr + rvdev->rsc_offset; > + > /* > * 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); > + > + dma = rsc->vring[i].pa; > + > + if (rproc->ops->alloc) > + va = rproc->ops->alloc(rproc, size, &dma); I believe this will be awkward for the remoteproc drivers to implement. Imagine a driver that programmatically register some fixed positioned carveouts and ioremapped vring buffers, it would then need internal book keeping to figure out which type of allocation each call is related to. Rather then deferring the allocation until this point I think we should tie a rproc_mem_entry to each vring and once we reach rproc_alloc_vring() we simply use "va" and "dma" from that. We would get this from rproc_parse_vring() checking to find an existing mem_entry matching the vring requirements (da, then pa) and falling back to allocating a new carveout mem_entry. By then making the current "carveouts" list heterogeneous we would allow for arbitrary memory types to be used for backing vrings, as well as trace devices, code and data segments. Regards, Bjorn ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/3] remoteproc: core: add rproc ops for memory allocation 2016-09-15 17:27 ` Bjorn Andersson @ 2016-09-16 7:47 ` loic pallardy 2016-09-16 18:12 ` Bjorn Andersson 0 siblings, 1 reply; 10+ messages in thread From: loic pallardy @ 2016-09-16 7:47 UTC (permalink / raw) To: Bjorn Andersson Cc: ohad, lee.jones, linux-remoteproc, linux-kernel, kernel, Suman Anna On 09/15/2016 07:27 PM, Bjorn Andersson wrote: > On Tue 06 Sep 00:39 PDT 2016, Loic Pallardy wrote: > >> Remoteproc core is currently using dma_alloc_coherent for >> carveout and vring allocation. >> It doesn't allow to support specific use cases like fixed memory >> region or internal RAM support. >> >> Two new rproc ops (alloc and free) is added to provide flexibility >> to platform implementation to provide specific memory allocator >> taking into account coprocessor characteristics. >> rproc_handle_carveout and rproc_alloc_vring functions are modified >> to invoke these ops if present, and fallback to regular processing >> if platform specific allocation failed and if resquested memory is >> not fixed (physical address == FW_RSC_ADDR_ANY) >> >> Signed-off-by: Loic Pallardy <loic.pallardy@st.com> >> --- >> drivers/remoteproc/remoteproc_core.c | 67 ++++++++++++++++++++++++++++++------ >> include/linux/remoteproc.h | 4 +++ >> 2 files changed, 60 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c >> index 0d3c191..7493b08 100644 >> --- a/drivers/remoteproc/remoteproc_core.c >> +++ b/drivers/remoteproc/remoteproc_core.c >> @@ -207,19 +207,29 @@ 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; >> - void *va; >> + void *va = NULL; >> int ret, size, notifyid; >> >> /* actual size of vring (in bytes) */ >> size = PAGE_ALIGN(vring_size(rvring->len, rvring->align)); >> >> + rsc = (void *)rproc->table_ptr + rvdev->rsc_offset; >> + >> /* >> * 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); >> + >> + dma = rsc->vring[i].pa; >> + >> + if (rproc->ops->alloc) >> + va = rproc->ops->alloc(rproc, size, &dma); > > I believe this will be awkward for the remoteproc drivers to implement. > > Imagine a driver that programmatically register some fixed positioned > carveouts and ioremapped vring buffers, it would then need internal book > keeping to figure out which type of allocation each call is related to. Yes true like any allocator does. And it is needed to manage region overlap. > > > Rather then deferring the allocation until this point I think we should > tie a rproc_mem_entry to each vring and once we reach > rproc_alloc_vring() we simply use "va" and "dma" from that. > > We would get this from rproc_parse_vring() checking to find an existing > mem_entry matching the vring requirements (da, then pa) and falling back > to allocating a new carveout mem_entry. > This doesn't answer to use case described by Suman. What if no specific address are requested in firmware resource table, but buffers need to be allocated in internal RAM for example. Only rproc driver will know on which allocator to rely. By memremaping a complete memory area and offering va to dma (pa) conversion, you don't verify possible overlap between requested regions. This is done today by allocator. The idea from ST pov, was to rely on memory region, to declare subdev associated to rproc driver and to rely on dma_alloc_coherent. I think TI wants to rely on its internal RAM memory allocator. Regards, Loic > > By then making the current "carveouts" list heterogeneous we would allow > for arbitrary memory types to be used for backing vrings, as well as > trace devices, code and data segments. > > Regards, > Bjorn > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/3] remoteproc: core: add rproc ops for memory allocation 2016-09-16 7:47 ` loic pallardy @ 2016-09-16 18:12 ` Bjorn Andersson 0 siblings, 0 replies; 10+ messages in thread From: Bjorn Andersson @ 2016-09-16 18:12 UTC (permalink / raw) To: loic pallardy Cc: ohad, lee.jones, linux-remoteproc, linux-kernel, kernel, Suman Anna On Fri 16 Sep 00:47 PDT 2016, loic pallardy wrote: > > > On 09/15/2016 07:27 PM, Bjorn Andersson wrote: > >On Tue 06 Sep 00:39 PDT 2016, Loic Pallardy wrote: > > > >>Remoteproc core is currently using dma_alloc_coherent for > >>carveout and vring allocation. > >>It doesn't allow to support specific use cases like fixed memory > >>region or internal RAM support. > >> > >>Two new rproc ops (alloc and free) is added to provide flexibility > >>to platform implementation to provide specific memory allocator > >>taking into account coprocessor characteristics. > >>rproc_handle_carveout and rproc_alloc_vring functions are modified > >>to invoke these ops if present, and fallback to regular processing > >>if platform specific allocation failed and if resquested memory is > >>not fixed (physical address == FW_RSC_ADDR_ANY) > >> > >>Signed-off-by: Loic Pallardy <loic.pallardy@st.com> > >>--- > >> drivers/remoteproc/remoteproc_core.c | 67 ++++++++++++++++++++++++++++++------ > >> include/linux/remoteproc.h | 4 +++ > >> 2 files changed, 60 insertions(+), 11 deletions(-) > >> > >>diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > >>index 0d3c191..7493b08 100644 > >>--- a/drivers/remoteproc/remoteproc_core.c > >>+++ b/drivers/remoteproc/remoteproc_core.c > >>@@ -207,19 +207,29 @@ 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; > >>- void *va; > >>+ void *va = NULL; > >> int ret, size, notifyid; > >> > >> /* actual size of vring (in bytes) */ > >> size = PAGE_ALIGN(vring_size(rvring->len, rvring->align)); > >> > >>+ rsc = (void *)rproc->table_ptr + rvdev->rsc_offset; > >>+ > >> /* > >> * 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); > >>+ > >>+ dma = rsc->vring[i].pa; > >>+ > >>+ if (rproc->ops->alloc) > >>+ va = rproc->ops->alloc(rproc, size, &dma); > > > >I believe this will be awkward for the remoteproc drivers to implement. > > > >Imagine a driver that programmatically register some fixed positioned > >carveouts and ioremapped vring buffers, it would then need internal book > >keeping to figure out which type of allocation each call is related to. > > Yes true like any allocator does. And it is needed to manage region overlap. Right, but I'm hoping we don't have to make each remoteproc driver an allocator - that we rather just have the drivers register a set of regions with the core and then that's matched with the resource table. Otherwise there will be a lot of duplicated boilerplate code in the drivers. > > > > > >Rather then deferring the allocation until this point I think we should > >tie a rproc_mem_entry to each vring and once we reach > >rproc_alloc_vring() we simply use "va" and "dma" from that. > > > >We would get this from rproc_parse_vring() checking to find an existing > >mem_entry matching the vring requirements (da, then pa) and falling back > >to allocating a new carveout mem_entry. > > > This doesn't answer to use case described by Suman. What if no specific > address are requested in firmware resource table, but buffers need to be > allocated in internal RAM for example. Only rproc driver will know on which > allocator to rely. > If the vrings are listed with FW_RSC_ADDR_ANY as both da and pa, then we have no way to match it towards an allocation and the same would go for your proposed API. The alloc() function would not know if the request is for a carveout or vring - or for which vring it is. For the case of the vrings residing in some device memory that we ioremap it makes sense to specify the "pa" for this in the resource table and we can match this towards a rproc_mem_entry. For the case of the vrings being allocated from sram using a dynamic allocator, we're out of luck with the current resource table - we have nothing to match this on. By associating a rproc_mem_entry to the vring a driver could programmatically register a vdev with a set of vrings with oddly allocated memory. But there is no standard way of communicating these addresses to the remote. > By memremaping a complete memory area and offering va to dma (pa) > conversion, you don't verify possible overlap between requested regions. > This is done today by allocator. > I share this view, I don't think we should rely on da_to_va() here, but rather only match whole rproc_mem_entries. > The idea from ST pov, was to rely on memory region, to declare subdev > associated to rproc driver and to rely on dma_alloc_coherent. > I think TI wants to rely on its internal RAM memory allocator. > There are additional constraints, beyond using a fixed "pa" that makes the ST suggestion of creating a dma-dev worth while - e.g. hardware that can only address parts of RAM. But as we've concluded we have an issue with dma_alloc_coherent() not dealing with non-power-of-two sized memory regions. So this is something I hope to discuss with people during Linaro Connect. The issue with the setup of registering subdevices and then separately registering a carveout that matches up is that in the Qualcomm driver I want to be able to reuse the subdevice thing, but I don't want to create a resource table as well, just to "trigger" the allocation. In addition to that we still have the request of allowing ioremapped regions used instead of dma_alloc_coherent() and in an extension other types of allocators. If we can represent all these types of regions in a single list of rproc_mem_entries then the users (firmware, vrings, trace...) will be oblivious to what type of allocation they are residing in. I'll write up a few patches to show what I'm suggesting. Regards, Bjorn ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-09-16 18:12 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-09-06 7:39 [PATCH v2 0/3] Remoteproc: Add predefined coprocessor memory mapping support Loic Pallardy 2016-09-06 7:39 ` [PATCH v2 1/3] remoteproc: Modify FW_RSC_ADDR_ANY definition Loic Pallardy 2016-09-06 18:10 ` Bjorn Andersson 2016-09-06 7:39 ` [PATCH v2 2/3] remoteproc: core: transform struct fw_rsc_vdev_vring reserved field in pa Loic Pallardy 2016-09-06 18:10 ` Bjorn Andersson 2016-09-06 7:39 ` [PATCH v2 3/3] remoteproc: core: add rproc ops for memory allocation Loic Pallardy 2016-09-08 14:06 ` Lee Jones 2016-09-15 17:27 ` Bjorn Andersson 2016-09-16 7:47 ` loic pallardy 2016-09-16 18:12 ` Bjorn Andersson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).