Re: [PATCH v2 4/4] remoteproc: Use of_reserved_mem_region_* functions for "memory-region"

2025-05-02 Thread Rob Herring 
On Fri, May 2, 2025 at 10:40 AM Tanmay Shah  wrote:
>
>
> Hello Rob,
>
> Thanks for the patch. Please find my comments below.
>
>
> On 4/23/25 2:42 PM, Rob Herring (Arm) wrote:
> > Use the newly added of_reserved_mem_region_to_resource() and
> > of_reserved_mem_region_count() functions to handle "memory-region"
> > properties.
> >
> > The error handling is a bit different in some cases. Often
> > "memory-region" is optional, so failed lookup is not an error. But then
> > an error in of_reserved_mem_lookup() is treated as an error. However,
> > that distinction is not really important. Either the region is available
> > and usable or it is not. So now, it is just
> > of_reserved_mem_region_to_resource() which is checked for an error.
> >
> > Signed-off-by: Rob Herring (Arm) 
> > ---
> > v2:
> >   - Use strstarts instead of strcmp for resource names as they include
> > the unit-address.
> >   - Drop the unit-address from resource name for imx and st drivers
> > ---
> >   drivers/remoteproc/imx_dsp_rproc.c| 45 
> >   drivers/remoteproc/imx_rproc.c| 68 
> > --
> >   drivers/remoteproc/qcom_q6v5_adsp.c   | 24 ---
> >   drivers/remoteproc/qcom_q6v5_mss.c| 60 +--
> >   drivers/remoteproc/qcom_q6v5_pas.c| 69 
> > +++
> >   drivers/remoteproc/qcom_q6v5_wcss.c   | 25 +--
> >   drivers/remoteproc/qcom_wcnss.c   | 23 ---
> >   drivers/remoteproc/rcar_rproc.c   | 36 +++-
> >   drivers/remoteproc/st_remoteproc.c| 41 +-
> >   drivers/remoteproc/stm32_rproc.c  | 44 +---
> >   drivers/remoteproc/ti_k3_dsp_remoteproc.c | 28 +
> >   drivers/remoteproc/ti_k3_m4_remoteproc.c  | 28 +
> >   drivers/remoteproc/ti_k3_r5_remoteproc.c  | 28 +
> >   drivers/remoteproc/xlnx_r5_remoteproc.c   | 51 +--
> >   14 files changed, 221 insertions(+), 349 deletions(-)
> >
> > diff --git a/drivers/remoteproc/imx_dsp_rproc.c 
> > b/drivers/remoteproc/imx_dsp_rproc.c
> > index 90cb1fc13e71..fffae6ff4a5c 100644
>
> [ ... ]
>
> > diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c 
> > b/drivers/remoteproc/xlnx_r5_remoteproc.c
> > index 5aeedeaf3c41..b73e97074c01 100644
> > --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
> > +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
> > @@ -460,49 +460,44 @@ static int add_mem_regions_carveout(struct rproc 
> > *rproc)
> >   {
> >   struct rproc_mem_entry *rproc_mem;
> >   struct zynqmp_r5_core *r5_core;
> > - struct of_phandle_iterator it;
> > - struct reserved_mem *rmem;
> >   int i = 0;
> >
> >   r5_core = rproc->priv;
> >
> >   /* Register associated reserved memory regions */
> > - of_phandle_iterator_init(&it, r5_core->np, "memory-region", NULL, 0);
> > + while (1) {
> > + int err;
> > + struct resource res;
> >
> > - while (of_phandle_iterator_next(&it) == 0) {
> > - rmem = of_reserved_mem_lookup(it.node);
> > - if (!rmem) {
> > - of_node_put(it.node);
> > - dev_err(&rproc->dev, "unable to acquire 
> > memory-region\n");
> > - return -EINVAL;
> > - }
> > + err = of_reserved_mem_region_to_resource(r5_core->np, i++, 
> > &res);
>
> Here i++ is not needed as it's done at the end of the loop.
> This bug breaks RPMsg communication on zynqmp platform.

Thanks for debugging it. I'll fix that up.

Rob

Re: [PATCH v2 4/4] remoteproc: Use of_reserved_mem_region_* functions for "memory-region"

2025-05-02 Thread Tanmay Shah 



Hello Rob,

Thanks for the patch. Please find my comments below.


On 4/23/25 2:42 PM, Rob Herring (Arm) wrote:

Use the newly added of_reserved_mem_region_to_resource() and
of_reserved_mem_region_count() functions to handle "memory-region"
properties.

The error handling is a bit different in some cases. Often
"memory-region" is optional, so failed lookup is not an error. But then
an error in of_reserved_mem_lookup() is treated as an error. However,
that distinction is not really important. Either the region is available
and usable or it is not. So now, it is just
of_reserved_mem_region_to_resource() which is checked for an error.

Signed-off-by: Rob Herring (Arm) 
---
v2:
  - Use strstarts instead of strcmp for resource names as they include
the unit-address.
  - Drop the unit-address from resource name for imx and st drivers
---
  drivers/remoteproc/imx_dsp_rproc.c| 45 
  drivers/remoteproc/imx_rproc.c| 68 --
  drivers/remoteproc/qcom_q6v5_adsp.c   | 24 ---
  drivers/remoteproc/qcom_q6v5_mss.c| 60 +--
  drivers/remoteproc/qcom_q6v5_pas.c| 69 +++
  drivers/remoteproc/qcom_q6v5_wcss.c   | 25 +--
  drivers/remoteproc/qcom_wcnss.c   | 23 ---
  drivers/remoteproc/rcar_rproc.c   | 36 +++-
  drivers/remoteproc/st_remoteproc.c| 41 +-
  drivers/remoteproc/stm32_rproc.c  | 44 +---
  drivers/remoteproc/ti_k3_dsp_remoteproc.c | 28 +
  drivers/remoteproc/ti_k3_m4_remoteproc.c  | 28 +
  drivers/remoteproc/ti_k3_r5_remoteproc.c  | 28 +
  drivers/remoteproc/xlnx_r5_remoteproc.c   | 51 +--
  14 files changed, 221 insertions(+), 349 deletions(-)

diff --git a/drivers/remoteproc/imx_dsp_rproc.c 
b/drivers/remoteproc/imx_dsp_rproc.c
index 90cb1fc13e71..fffae6ff4a5c 100644


[ ... ]


diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c 
b/drivers/remoteproc/xlnx_r5_remoteproc.c
index 5aeedeaf3c41..b73e97074c01 100644
--- a/drivers/remoteproc/xlnx_r5_remoteproc.c
+++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
@@ -460,49 +460,44 @@ static int add_mem_regions_carveout(struct rproc *rproc)
  {
struct rproc_mem_entry *rproc_mem;
struct zynqmp_r5_core *r5_core;
-   struct of_phandle_iterator it;
-   struct reserved_mem *rmem;
int i = 0;
  
  	r5_core = rproc->priv;
  
  	/* Register associated reserved memory regions */

-   of_phandle_iterator_init(&it, r5_core->np, "memory-region", NULL, 0);
+   while (1) {
+   int err;
+   struct resource res;
  
-	while (of_phandle_iterator_next(&it) == 0) {

-   rmem = of_reserved_mem_lookup(it.node);
-   if (!rmem) {
-   of_node_put(it.node);
-   dev_err(&rproc->dev, "unable to acquire 
memory-region\n");
-   return -EINVAL;
-   }
+   err = of_reserved_mem_region_to_resource(r5_core->np, i++, 
&res);


Here i++ is not needed as it's done at the end of the loop.
This bug breaks RPMsg communication on zynqmp platform.

Thanks,
Tanmay


+   if (err)
+   return 0;
  
-		if (!strcmp(it.node->name, "vdev0buffer")) {

+   if (strstarts(res.name, "vdev0buffer")) {
/* Init reserved memory for vdev buffer */
rproc_mem = rproc_of_resm_mem_entry_init(&rproc->dev, i,
-rmem->size,
-rmem->base,
-it.node->name);
+
resource_size(&res),
+res.start,
+"vdev0buffer");
} else {
/* Register associated reserved memory regions */
rproc_mem = rproc_mem_entry_init(&rproc->dev, NULL,
-(dma_addr_t)rmem->base,
-rmem->size, rmem->base,
+(dma_addr_t)res.start,
+resource_size(&res), 
res.start,
 
zynqmp_r5_mem_region_map,
 
zynqmp_r5_mem_region_unmap,
-it.node->name);
+"%.*s",
+strchrnul(res.name, 
'@') - res.name,
+

[PATCH v2 4/4] remoteproc: Use of_reserved_mem_region_* functions for "memory-region"

2025-04-23 Thread Rob Herring (Arm) 
Use the newly added of_reserved_mem_region_to_resource() and
of_reserved_mem_region_count() functions to handle "memory-region"
properties.

The error handling is a bit different in some cases. Often
"memory-region" is optional, so failed lookup is not an error. But then
an error in of_reserved_mem_lookup() is treated as an error. However,
that distinction is not really important. Either the region is available
and usable or it is not. So now, it is just
of_reserved_mem_region_to_resource() which is checked for an error.

Signed-off-by: Rob Herring (Arm) 
---
v2:
 - Use strstarts instead of strcmp for resource names as they include
   the unit-address.
 - Drop the unit-address from resource name for imx and st drivers
---
 drivers/remoteproc/imx_dsp_rproc.c| 45 
 drivers/remoteproc/imx_rproc.c| 68 --
 drivers/remoteproc/qcom_q6v5_adsp.c   | 24 ---
 drivers/remoteproc/qcom_q6v5_mss.c| 60 +--
 drivers/remoteproc/qcom_q6v5_pas.c| 69 +++
 drivers/remoteproc/qcom_q6v5_wcss.c   | 25 +--
 drivers/remoteproc/qcom_wcnss.c   | 23 ---
 drivers/remoteproc/rcar_rproc.c   | 36 +++-
 drivers/remoteproc/st_remoteproc.c| 41 +-
 drivers/remoteproc/stm32_rproc.c  | 44 +---
 drivers/remoteproc/ti_k3_dsp_remoteproc.c | 28 +
 drivers/remoteproc/ti_k3_m4_remoteproc.c  | 28 +
 drivers/remoteproc/ti_k3_r5_remoteproc.c  | 28 +
 drivers/remoteproc/xlnx_r5_remoteproc.c   | 51 +--
 14 files changed, 221 insertions(+), 349 deletions(-)

diff --git a/drivers/remoteproc/imx_dsp_rproc.c 
b/drivers/remoteproc/imx_dsp_rproc.c
index 90cb1fc13e71..fffae6ff4a5c 100644
--- a/drivers/remoteproc/imx_dsp_rproc.c
+++ b/drivers/remoteproc/imx_dsp_rproc.c
@@ -598,11 +598,9 @@ static int imx_dsp_rproc_add_carveout(struct imx_dsp_rproc 
*priv)
struct rproc *rproc = priv->rproc;
struct device *dev = rproc->dev.parent;
struct device_node *np = dev->of_node;
-   struct of_phandle_iterator it;
struct rproc_mem_entry *mem;
-   struct reserved_mem *rmem;
void __iomem *cpu_addr;
-   int a;
+   int a, i = 0;
u64 da;
 
/* Remap required addresses */
@@ -633,45 +631,38 @@ static int imx_dsp_rproc_add_carveout(struct 
imx_dsp_rproc *priv)
rproc_add_carveout(rproc, mem);
}
 
-   of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
-   while (of_phandle_iterator_next(&it) == 0) {
+   while (1) {
+   int err;
+   struct resource res;
+
+   err = of_reserved_mem_region_to_resource(np, i++, &res);
+   if (err)
+   return 0;
+
/*
 * Ignore the first memory region which will be used vdev 
buffer.
 * No need to do extra handlings, rproc_add_virtio_dev will 
handle it.
 */
-   if (!strcmp(it.node->name, "vdev0buffer"))
+   if (strstarts(res.name, "vdev0buffer"))
continue;
 
-   rmem = of_reserved_mem_lookup(it.node);
-   if (!rmem) {
-   of_node_put(it.node);
-   dev_err(dev, "unable to acquire memory-region\n");
+   if (imx_dsp_rproc_sys_to_da(priv, res.start, 
resource_size(&res), &da))
return -EINVAL;
-   }
 
-   if (imx_dsp_rproc_sys_to_da(priv, rmem->base, rmem->size, &da)) 
{
-   of_node_put(it.node);
-   return -EINVAL;
-   }
-
-   cpu_addr = devm_ioremap_wc(dev, rmem->base, rmem->size);
+   cpu_addr = devm_ioremap_resource_wc(dev, &res);
if (!cpu_addr) {
-   of_node_put(it.node);
-   dev_err(dev, "failed to map memory %p\n", &rmem->base);
+   dev_err(dev, "failed to map memory %pR\n", &res);
return -ENOMEM;
}
 
/* Register memory region */
-   mem = rproc_mem_entry_init(dev, (void __force *)cpu_addr, 
(dma_addr_t)rmem->base,
-  rmem->size, da, NULL, NULL, 
it.node->name);
-
-   if (mem) {
-   rproc_coredump_add_segment(rproc, da, rmem->size);
-   } else {
-   of_node_put(it.node);
+   mem = rproc_mem_entry_init(dev, (void __force *)cpu_addr, 
(dma_addr_t)res.start,
+  resource_size(&res), da, NULL, NULL,
+  "%.*s", strchrnul(res.name, '@') - 
res.name, res.name);
+   if (!mem)
return -ENOMEM;
-   }
 
+