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