[PATCH 0/3] of: Common "memory-region" parsing

[PATCH 0/3] of: Common "memory-region" parsing

[Rob Herring (Arm)]

While there's a common function to parse "memory-region" properties for
DMA pool regions, there's not anything for driver private regions. As a
result, drivers have resorted to parsing "memory-region" properties
themselves repeating the same pattern over and over. To fix this, this
series adds 2 functions to handle those cases:
of_reserved_mem_region_to_resource() and of_reserved_mem_region_count().

I've converted the whole tree, but just including remoteproc here as
it has the most cases. I intend to apply the first 2 patches for 6.15
so the driver conversions can be applied for 6.16.

A git tree with all the drivers converted is here[1].

[1] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git dt/memory-region

Rob Herring (Arm) (3):
  of: reserved_mem: Add functions to parse "memory-region"
  of: Simplify of_dma_set_restricted_buffer() to use
    of_for_each_phandle()
  remoteproc: Use of_reserved_mem_region_* functions for "memory-region"

 drivers/of/device.c                       | 34 ++++------
 drivers/of/of_reserved_mem.c              | 77 +++++++++++++++++++++++
 drivers/remoteproc/imx_dsp_rproc.c        | 44 +++++--------
 drivers/remoteproc/imx_rproc.c            | 65 ++++++++-----------
 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        | 39 +++++-------
 drivers/remoteproc/stm32_rproc.c          | 42 ++++++-------
 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   | 49 +++++----------
 include/linux/of_reserved_mem.h           | 26 ++++++++
 17 files changed, 329 insertions(+), 368 deletions(-)

--
2.47.2

[PATCH 1/3] of: reserved_mem: Add functions to parse "memory-region"

[Rob Herring (Arm)]

Drivers with "memory-region" properties currently have to do their own
parsing of "memory-region" properties. The result is all the drivers
have similar patterns of a call to parse "memory-region" and then get
the region's address and size. As this is a standard property, it should
have common functions for drivers to use. Add new functions to count the
number of regions and retrieve the region's address as a resource.

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
 drivers/of/of_reserved_mem.c    | 77 +++++++++++++++++++++++++++++++++
 include/linux/of_reserved_mem.h | 26 +++++++++++
 2 files changed, 103 insertions(+)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 75e819f66a56..fd50038dff76 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -12,6 +12,7 @@
 #define pr_fmt(fmt)	"OF: reserved mem: " fmt
 
 #include <linux/err.h>
+#include <linux/ioport.h>
 #include <linux/libfdt.h>
 #include <linux/of.h>
 #include <linux/of_fdt.h>
@@ -740,3 +741,79 @@ struct reserved_mem *of_reserved_mem_lookup(struct device_node *np)
 	return NULL;
 }
 EXPORT_SYMBOL_GPL(of_reserved_mem_lookup);
+
+/**
+ * of_reserved_mem_region_to_resource() - Get a reserved memory region as a resource
+ * @np:		node containing 'memory-region' property
+ * @idx:	index of 'memory-region' property to lookup
+ * @res:	Pointer to a struct resource to fill in with reserved region
+ *
+ * This function allows drivers to lookup a node's 'memory-region' property
+ * entries by index and return a struct resource for the entry.
+ *
+ * Returns 0 on success with @res filled in. Returns -ENODEV if 'memory-region'
+ * is missing or unavailable, -EINVAL for any other error.
+ */
+int of_reserved_mem_region_to_resource(const struct device_node *np, unsigned int idx, struct resource *res)
+{
+	struct reserved_mem *rmem;
+
+	if (!np)
+		return -EINVAL;
+
+	struct device_node __free(device_node) *target = of_parse_phandle(np, "memory-region", idx);
+	if (!target || !of_device_is_available(target))
+		return -ENODEV;
+
+	rmem = of_reserved_mem_lookup(target);
+	if (!rmem)
+		return -EINVAL;
+
+	resource_set_range(res, rmem->base, rmem->size);
+	res->name = rmem->name;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_reserved_mem_region_to_resource);
+
+/**
+ * of_reserved_mem_region_to_resource_byname() - Get a reserved memory region as a resource
+ * @np:		node containing 'memory-region' property
+ * @name:	name of 'memory-region' property entry to lookup
+ * @res:	Pointer to a struct resource to fill in with reserved region
+ *
+ * This function allows drivers to lookup a node's 'memory-region' property
+ * entries by name and return a struct resource for the entry.
+ *
+ * Returns 0 on success with @res filled in. Returns -ENODEV if 'memory-region'
+ * is missing or unavailable, -EINVAL for any other error.
+ */
+int of_reserved_mem_region_to_resource_byname(const struct device_node *np, const char *name, struct resource *res)
+{
+	int idx;
+
+	if (!name)
+		return -EINVAL;
+
+	idx = of_property_match_string(np, "memory-region-names", name);
+	if (idx < 0)
+		return idx;
+
+	return of_reserved_mem_region_to_resource(np, idx, res);
+}
+EXPORT_SYMBOL_GPL(of_reserved_mem_region_to_resource_byname);
+
+/**
+ * of_reserved_mem_region_count() - Return the number of 'memory-region' entries
+ * @np:		node containing 'memory-region' property
+ *
+ * This function allows drivers to retrieve the number of entries for a node's
+ * 'memory-region' property.
+ *
+ * Returns the number of entries on success, or negative error code on a
+ * malformed property.
+ */
+int of_reserved_mem_region_count(const struct device_node *np)
+{
+	return of_count_phandle_with_args(np, "memory-region", NULL);
+}
+EXPORT_SYMBOL_GPL(of_reserved_mem_region_count);
diff --git a/include/linux/of_reserved_mem.h b/include/linux/of_reserved_mem.h
index e338282da652..f573423359f4 100644
--- a/include/linux/of_reserved_mem.h
+++ b/include/linux/of_reserved_mem.h
@@ -7,6 +7,7 @@
 
 struct of_phandle_args;
 struct reserved_mem_ops;
+struct resource;
 
 struct reserved_mem {
 	const char			*name;
@@ -39,6 +40,12 @@ int of_reserved_mem_device_init_by_name(struct device *dev,
 void of_reserved_mem_device_release(struct device *dev);
 
 struct reserved_mem *of_reserved_mem_lookup(struct device_node *np);
+int of_reserved_mem_region_to_resource(const struct device_node *np,
+				       unsigned int idx, struct resource *res);
+int of_reserved_mem_region_to_resource_byname(const struct device_node *np,
+					      const char *name, struct resource *res);
+int of_reserved_mem_region_count(const struct device_node *np);
+
 #else
 
 #define RESERVEDMEM_OF_DECLARE(name, compat, init)			\
@@ -63,6 +70,25 @@ static inline struct reserved_mem *of_reserved_mem_lookup(struct device_node *np
 {
 	return NULL;
 }
+
+static inline int of_reserved_mem_region_to_resource(const struct device_node *np,
+						     unsigned int idx,
+						     struct resource *res)
+{
+	return -ENOSYS;
+}
+
+static inline int of_reserved_mem_region_to_resource_byname(const struct device_node *np,
+							    const char *name,
+							    struct resource *res)
+{
+	return -ENOSYS;
+}
+
+static inline int of_reserved_mem_region_count(const struct device_node *np)
+{
+	return 0;
+}
 #endif
 
 /**
-- 
2.47.2

Re: [PATCH 1/3] of: reserved_mem: Add functions to parse "memory-region"

[Daniel Baluta]

On Tue, Mar 18, 2025 at 1:24 AM Rob Herring (Arm) <robh@kernel.org> wrote:
>
> Drivers with "memory-region" properties currently have to do their own
> parsing of "memory-region" properties. The result is all the drivers
> have similar patterns of a call to parse "memory-region" and then get
> the region's address and size. As this is a standard property, it should
> have common functions for drivers to use. Add new functions to count the
> number of regions and retrieve the region's address as a resource.
>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>

Reviewed-by: Daniel Baluta <daniel.baluta@nxp.com>

Re: [PATCH 1/3] of: reserved_mem: Add functions to parse "memory-region"

[Jonathan Cameron]

On Mon, 17 Mar 2025 18:24:21 -0500
"Rob Herring (Arm)" <robh@kernel.org> wrote:

> Drivers with "memory-region" properties currently have to do their own
> parsing of "memory-region" properties. The result is all the drivers
> have similar patterns of a call to parse "memory-region" and then get
> the region's address and size. As this is a standard property, it should
> have common functions for drivers to use. Add new functions to count the
> number of regions and retrieve the region's address as a resource.
> 
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
>  drivers/of/of_reserved_mem.c    | 77 +++++++++++++++++++++++++++++++++
>  include/linux/of_reserved_mem.h | 26 +++++++++++
>  2 files changed, 103 insertions(+)
> 
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 75e819f66a56..fd50038dff76 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -12,6 +12,7 @@
>  #define pr_fmt(fmt)	"OF: reserved mem: " fmt
>  
>  #include <linux/err.h>
> +#include <linux/ioport.h>
>  #include <linux/libfdt.h>
>  #include <linux/of.h>
>  #include <linux/of_fdt.h>
> @@ -740,3 +741,79 @@ struct reserved_mem *of_reserved_mem_lookup(struct device_node *np)
>  	return NULL;
>  }
>  EXPORT_SYMBOL_GPL(of_reserved_mem_lookup);
> +
> +/**
> + * of_reserved_mem_region_to_resource() - Get a reserved memory region as a resource
> + * @np:		node containing 'memory-region' property
> + * @idx:	index of 'memory-region' property to lookup
> + * @res:	Pointer to a struct resource to fill in with reserved region
> + *
> + * This function allows drivers to lookup a node's 'memory-region' property
> + * entries by index and return a struct resource for the entry.
> + *
> + * Returns 0 on success with @res filled in. Returns -ENODEV if 'memory-region'
> + * is missing or unavailable, -EINVAL for any other error.
> + */
> +int of_reserved_mem_region_to_resource(const struct device_node *np, unsigned int idx, struct resource *res)

Maybe wrap?
 
> +{

[PATCH 2/3] of: Simplify of_dma_set_restricted_buffer() to use of_for_each_phandle()

[Rob Herring (Arm)]

Simplify of_dma_set_restricted_buffer() by using of_property_present()
and of_for_each_phandle() iterator.

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
 drivers/of/device.c | 34 +++++++++++++---------------------
 1 file changed, 13 insertions(+), 21 deletions(-)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index edf3be197265..bb4a47d58249 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -35,44 +35,36 @@ EXPORT_SYMBOL(of_match_device);
 static void
 of_dma_set_restricted_buffer(struct device *dev, struct device_node *np)
 {
-	struct device_node *node, *of_node = dev->of_node;
-	int count, i;
+	struct device_node *of_node = dev->of_node;
+	struct of_phandle_iterator it;
+	int rc, i = 0;
 
 	if (!IS_ENABLED(CONFIG_DMA_RESTRICTED_POOL))
 		return;
 
-	count = of_property_count_elems_of_size(of_node, "memory-region",
-						sizeof(u32));
 	/*
 	 * If dev->of_node doesn't exist or doesn't contain memory-region, try
 	 * the OF node having DMA configuration.
 	 */
-	if (count <= 0) {
+	if (!of_property_present(of_node, "memory-region"))
 		of_node = np;
-		count = of_property_count_elems_of_size(
-			of_node, "memory-region", sizeof(u32));
-	}
 
-	for (i = 0; i < count; i++) {
-		node = of_parse_phandle(of_node, "memory-region", i);
+	of_for_each_phandle(&it, rc, of_node, "memory-region", NULL, 0) {
 		/*
 		 * There might be multiple memory regions, but only one
 		 * restricted-dma-pool region is allowed.
 		 */
-		if (of_device_is_compatible(node, "restricted-dma-pool") &&
-		    of_device_is_available(node)) {
-			of_node_put(node);
-			break;
+		if (of_device_is_compatible(it.node, "restricted-dma-pool") &&
+		    of_device_is_available(it.node)) {
+			if (!of_reserved_mem_device_init_by_idx(dev, of_node, i)) {
+				of_node_put(it.node);
+				return;
+			}
 		}
-		of_node_put(node);
+		i++;
 	}
 
-	/*
-	 * Attempt to initialize a restricted-dma-pool region if one was found.
-	 * Note that count can hold a negative error code.
-	 */
-	if (i < count && of_reserved_mem_device_init_by_idx(dev, of_node, i))
-		dev_warn(dev, "failed to initialise \"restricted-dma-pool\" memory node\n");
+	dev_warn(dev, "failed to initialise \"restricted-dma-pool\" memory node\n");
 }
 
 /**
-- 
2.47.2

Re: [PATCH 2/3] of: Simplify of_dma_set_restricted_buffer() to use of_for_each_phandle()

[Chen-Yu Tsai]

Hi,

On Tue, Mar 18, 2025 at 7:29 AM Rob Herring (Arm) <robh@kernel.org> wrote:
>
> Simplify of_dma_set_restricted_buffer() by using of_property_present()
> and of_for_each_phandle() iterator.
>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
>  drivers/of/device.c | 34 +++++++++++++---------------------
>  1 file changed, 13 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index edf3be197265..bb4a47d58249 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -35,44 +35,36 @@ EXPORT_SYMBOL(of_match_device);
>  static void
>  of_dma_set_restricted_buffer(struct device *dev, struct device_node *np)
>  {
> -       struct device_node *node, *of_node = dev->of_node;
> -       int count, i;
> +       struct device_node *of_node = dev->of_node;
> +       struct of_phandle_iterator it;
> +       int rc, i = 0;
>
>         if (!IS_ENABLED(CONFIG_DMA_RESTRICTED_POOL))
>                 return;
>
> -       count = of_property_count_elems_of_size(of_node, "memory-region",
> -                                               sizeof(u32));
>         /*
>          * If dev->of_node doesn't exist or doesn't contain memory-region, try
>          * the OF node having DMA configuration.
>          */
> -       if (count <= 0) {
> +       if (!of_property_present(of_node, "memory-region"))
>                 of_node = np;
> -               count = of_property_count_elems_of_size(
> -                       of_node, "memory-region", sizeof(u32));
> -       }
>
> -       for (i = 0; i < count; i++) {
> -               node = of_parse_phandle(of_node, "memory-region", i);
> +       of_for_each_phandle(&it, rc, of_node, "memory-region", NULL, 0) {
>                 /*
>                  * There might be multiple memory regions, but only one
>                  * restricted-dma-pool region is allowed.
>                  */
> -               if (of_device_is_compatible(node, "restricted-dma-pool") &&
> -                   of_device_is_available(node)) {
> -                       of_node_put(node);
> -                       break;
> +               if (of_device_is_compatible(it.node, "restricted-dma-pool") &&
> +                   of_device_is_available(it.node)) {
> +                       if (!of_reserved_mem_device_init_by_idx(dev, of_node, i)) {
> +                               of_node_put(it.node);
> +                               return;
> +                       }
>                 }
> -               of_node_put(node);
> +               i++;
>         }
>
> -       /*
> -        * Attempt to initialize a restricted-dma-pool region if one was found.
> -        * Note that count can hold a negative error code.
> -        */
> -       if (i < count && of_reserved_mem_device_init_by_idx(dev, of_node, i))
> -               dev_warn(dev, "failed to initialise \"restricted-dma-pool\" memory node\n");
> +       dev_warn(dev, "failed to initialise \"restricted-dma-pool\" memory node\n");

This changes the behavior. Before this patch, it was:

    if a restricted dma pool was found, but initializing it failed, print
    a warning.

Whereas now it has become:

     print a warning unless a restricted dma pool was found and successfully
     initialized.

This change causes the kernel to print out the warning for devices that
don't even do DMA:

simple-pm-bus soc: failed to initialise "restricted-dma-pool" memory node
simple-pm-bus 10006000.syscon: failed to initialise
"restricted-dma-pool" memory node
mtk-tphy soc:t-phy@11c80000: failed to initialise
"restricted-dma-pool" memory node
mtk-tphy soc:t-phy@11ca0000: failed to initialise
"restricted-dma-pool" memory node
mediatek-mipi-tx 11cc0000.dsi-phy: failed to initialise
"restricted-dma-pool" memory node
mediatek-mipi-tx 11cc0000.dsi-phy: can't get nvmem_cell_get, ignore it
clk-mt8186-apmixed 1000c000.syscon: failed to initialise
"restricted-dma-pool" memory node
clk-mt8186-topck 10000000.syscon: failed to initialise
"restricted-dma-pool" memory node
clk-mt8186-infra-ao 10001000.syscon: failed to initialise
"restricted-dma-pool" memory node
clk-mt8186-cam 1a000000.clock-controller: failed to initialise
"restricted-dma-pool" memory node
clk-mt8186-cam 1a04f000.clock-controller: failed to initialise
"restricted-dma-pool" memory node
clk-mt8186-cam 1a06f000.clock-controller: failed to initialise
"restricted-dma-pool" memory node
clk-mt8186-img 15020000.clock-controller: failed to initialise
"restricted-dma-pool" memory node
clk-mt8186-img 15820000.clock-controller: failed to initialise
"restricted-dma-pool" memory node
clk-mt8186-imp_iic_wrap 11017000.clock-controller: failed to
initialise "restricted-dma-pool" memory node
clk-mt8186-ipe 1c000000.clock-controller: failed to initialise
"restricted-dma-pool" memory node
clk-mt8186-mcu c53a000.syscon: failed to initialise
"restricted-dma-pool" memory node
clk-mt8186-mdp 1b000000.clock-controller: failed to initialise
"restricted-dma-pool" memory node
clk-mt8186-mfg 13000000.clock-controller: failed to initialise
"restricted-dma-pool" memory node
clk-mt8186-vdec 1602f000.clock-controller: failed to initialise
"restricted-dma-pool" memory node
clk-mt8186-venc 17000000.clock-controller: failed to initialise
"restricted-dma-pool" memory node
clk-mt8186-wpe 14020000.clock-controller: failed to initialise
"restricted-dma-pool" memory node
mt-pmic-pwrap 1000d000.pwrap: failed to initialise
"restricted-dma-pool" memory node
platform 1000d000.pwrap:pmic: failed to initialise
"restricted-dma-pool" memory node
mtk-svs 1100bc00.svs: failed to initialise "restricted-dma-pool" memory node


ChenYu
>  }
>
>  /**
> --
> 2.47.2
>
>

Re: [PATCH 2/3] of: Simplify of_dma_set_restricted_buffer() to use of_for_each_phandle()

[Rob Herring]

On Wed, Mar 26, 2025 at 1:44 AM Chen-Yu Tsai <wens@kernel.org> wrote:
>
> Hi,
>
> On Tue, Mar 18, 2025 at 7:29 AM Rob Herring (Arm) <robh@kernel.org> wrote:
> >
> > Simplify of_dma_set_restricted_buffer() by using of_property_present()
> > and of_for_each_phandle() iterator.
> >
> > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > ---
> >  drivers/of/device.c | 34 +++++++++++++---------------------
> >  1 file changed, 13 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/of/device.c b/drivers/of/device.c
> > index edf3be197265..bb4a47d58249 100644
> > --- a/drivers/of/device.c
> > +++ b/drivers/of/device.c
> > @@ -35,44 +35,36 @@ EXPORT_SYMBOL(of_match_device);
> >  static void
> >  of_dma_set_restricted_buffer(struct device *dev, struct device_node *np)
> >  {
> > -       struct device_node *node, *of_node = dev->of_node;
> > -       int count, i;
> > +       struct device_node *of_node = dev->of_node;
> > +       struct of_phandle_iterator it;
> > +       int rc, i = 0;
> >
> >         if (!IS_ENABLED(CONFIG_DMA_RESTRICTED_POOL))
> >                 return;
> >
> > -       count = of_property_count_elems_of_size(of_node, "memory-region",
> > -                                               sizeof(u32));
> >         /*
> >          * If dev->of_node doesn't exist or doesn't contain memory-region, try
> >          * the OF node having DMA configuration.
> >          */
> > -       if (count <= 0) {
> > +       if (!of_property_present(of_node, "memory-region"))
> >                 of_node = np;
> > -               count = of_property_count_elems_of_size(
> > -                       of_node, "memory-region", sizeof(u32));
> > -       }
> >
> > -       for (i = 0; i < count; i++) {
> > -               node = of_parse_phandle(of_node, "memory-region", i);
> > +       of_for_each_phandle(&it, rc, of_node, "memory-region", NULL, 0) {
> >                 /*
> >                  * There might be multiple memory regions, but only one
> >                  * restricted-dma-pool region is allowed.
> >                  */
> > -               if (of_device_is_compatible(node, "restricted-dma-pool") &&
> > -                   of_device_is_available(node)) {
> > -                       of_node_put(node);
> > -                       break;
> > +               if (of_device_is_compatible(it.node, "restricted-dma-pool") &&
> > +                   of_device_is_available(it.node)) {
> > +                       if (!of_reserved_mem_device_init_by_idx(dev, of_node, i)) {
> > +                               of_node_put(it.node);
> > +                               return;
> > +                       }
> >                 }
> > -               of_node_put(node);
> > +               i++;
> >         }
> >
> > -       /*
> > -        * Attempt to initialize a restricted-dma-pool region if one was found.
> > -        * Note that count can hold a negative error code.
> > -        */
> > -       if (i < count && of_reserved_mem_device_init_by_idx(dev, of_node, i))
> > -               dev_warn(dev, "failed to initialise \"restricted-dma-pool\" memory node\n");
> > +       dev_warn(dev, "failed to initialise \"restricted-dma-pool\" memory node\n");
>
> This changes the behavior. Before this patch, it was:
>
>     if a restricted dma pool was found, but initializing it failed, print
>     a warning.
>
> Whereas now it has become:
>
>      print a warning unless a restricted dma pool was found and successfully
>      initialized.
>
> This change causes the kernel to print out the warning for devices that
> don't even do DMA:

Thanks. I fixed it up to only warn if i is non-zero.

Rob

Re: [PATCH 2/3] of: Simplify of_dma_set_restricted_buffer() to use of_for_each_phandle()

[Chen-Yu Tsai]

On Thu, Mar 27, 2025 at 2:53 AM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Mar 26, 2025 at 1:44 AM Chen-Yu Tsai <wens@kernel.org> wrote:
> >
> > Hi,
> >
> > On Tue, Mar 18, 2025 at 7:29 AM Rob Herring (Arm) <robh@kernel.org> wrote:
> > >
> > > Simplify of_dma_set_restricted_buffer() by using of_property_present()
> > > and of_for_each_phandle() iterator.
> > >
> > > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > > ---
> > >  drivers/of/device.c | 34 +++++++++++++---------------------
> > >  1 file changed, 13 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/of/device.c b/drivers/of/device.c
> > > index edf3be197265..bb4a47d58249 100644
> > > --- a/drivers/of/device.c
> > > +++ b/drivers/of/device.c
> > > @@ -35,44 +35,36 @@ EXPORT_SYMBOL(of_match_device);
> > >  static void
> > >  of_dma_set_restricted_buffer(struct device *dev, struct device_node *np)
> > >  {
> > > -       struct device_node *node, *of_node = dev->of_node;
> > > -       int count, i;
> > > +       struct device_node *of_node = dev->of_node;
> > > +       struct of_phandle_iterator it;
> > > +       int rc, i = 0;
> > >
> > >         if (!IS_ENABLED(CONFIG_DMA_RESTRICTED_POOL))
> > >                 return;
> > >
> > > -       count = of_property_count_elems_of_size(of_node, "memory-region",
> > > -                                               sizeof(u32));
> > >         /*
> > >          * If dev->of_node doesn't exist or doesn't contain memory-region, try
> > >          * the OF node having DMA configuration.
> > >          */
> > > -       if (count <= 0) {
> > > +       if (!of_property_present(of_node, "memory-region"))
> > >                 of_node = np;
> > > -               count = of_property_count_elems_of_size(
> > > -                       of_node, "memory-region", sizeof(u32));
> > > -       }
> > >
> > > -       for (i = 0; i < count; i++) {
> > > -               node = of_parse_phandle(of_node, "memory-region", i);
> > > +       of_for_each_phandle(&it, rc, of_node, "memory-region", NULL, 0) {
> > >                 /*
> > >                  * There might be multiple memory regions, but only one
> > >                  * restricted-dma-pool region is allowed.
> > >                  */
> > > -               if (of_device_is_compatible(node, "restricted-dma-pool") &&
> > > -                   of_device_is_available(node)) {
> > > -                       of_node_put(node);
> > > -                       break;
> > > +               if (of_device_is_compatible(it.node, "restricted-dma-pool") &&
> > > +                   of_device_is_available(it.node)) {
> > > +                       if (!of_reserved_mem_device_init_by_idx(dev, of_node, i)) {
> > > +                               of_node_put(it.node);
> > > +                               return;
> > > +                       }
> > >                 }
> > > -               of_node_put(node);
> > > +               i++;
> > >         }
> > >
> > > -       /*
> > > -        * Attempt to initialize a restricted-dma-pool region if one was found.
> > > -        * Note that count can hold a negative error code.
> > > -        */
> > > -       if (i < count && of_reserved_mem_device_init_by_idx(dev, of_node, i))
> > > -               dev_warn(dev, "failed to initialise \"restricted-dma-pool\" memory node\n");
> > > +       dev_warn(dev, "failed to initialise \"restricted-dma-pool\" memory node\n");
> >
> > This changes the behavior. Before this patch, it was:
> >
> >     if a restricted dma pool was found, but initializing it failed, print
> >     a warning.
> >
> > Whereas now it has become:
> >
> >      print a warning unless a restricted dma pool was found and successfully
> >      initialized.
> >
> > This change causes the kernel to print out the warning for devices that
> > don't even do DMA:
>
> Thanks. I fixed it up to only warn if i is non-zero.

Not sure if that matches the old behavior though? A node could have
memory-regions for shared dma pools but not restricted dma pools,
and i would be non-zero.

IMO the warning should be in the "else" branch of

    if (!of_reserved_mem_device_init_by_idx(dev, of_node, i))


ChenYu

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

[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) <robh@kernel.org>
---
For v6.16

 drivers/remoteproc/imx_dsp_rproc.c        | 44 ++++++---------
 drivers/remoteproc/imx_rproc.c            | 65 ++++++++-------------
 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        | 39 ++++++-------
 drivers/remoteproc/stm32_rproc.c          | 42 ++++++--------
 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   | 49 ++++++----------
 14 files changed, 213 insertions(+), 347 deletions(-)

diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
index ea5024919c2f..f3f341f4a262 100644
--- a/drivers/remoteproc/imx_dsp_rproc.c
+++ b/drivers/remoteproc/imx_dsp_rproc.c
@@ -595,11 +595,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 */
@@ -630,45 +628,37 @@ 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 (!strcmp(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, res.name);
+		if (!mem)
 			return -ENOMEM;
-		}

+		rproc_coredump_add_segment(rproc, da, resource_size(&res));
 		rproc_add_carveout(rproc, mem);
 	}

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 74299af1d7f1..fba95507b6cf 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -549,46 +549,41 @@ static int imx_rproc_prepare(struct rproc *rproc)
 {
 	struct imx_rproc *priv = rproc->priv;
 	struct device_node *np = priv->dev->of_node;
-	struct of_phandle_iterator it;
 	struct rproc_mem_entry *mem;
-	struct reserved_mem *rmem;
+	int i = 0;
 	u32 da;

 	/* Register associated reserved memory regions */
-	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 (!strcmp(res.name, "vdev0buffer"))
 			continue;

-		if (!strcmp(it.node->name, "rsc-table"))
+		if (!strcmp(res.name, "rsc-table"))
 			continue;

-		rmem = of_reserved_mem_lookup(it.node);
-		if (!rmem) {
-			of_node_put(it.node);
-			dev_err(priv->dev, "unable to acquire memory-region\n");
-			return -EINVAL;
-		}
-
 		/* No need to translate pa to da, i.MX use same map */
-		da = rmem->base;
+		da = res.start;

 		/* Register memory region */
-		mem = rproc_mem_entry_init(priv->dev, NULL, (dma_addr_t)rmem->base, rmem->size, da,
+		mem = rproc_mem_entry_init(priv->dev, NULL, (dma_addr_t)res.start, resource_size(&res), da,
 					   imx_rproc_mem_alloc, imx_rproc_mem_release,
-					   it.node->name);
+					   res.name);

-		if (mem) {
-			rproc_coredump_add_segment(rproc, da, rmem->size);
-		} else {
-			of_node_put(it.node);
+		if (!mem)
 			return -ENOMEM;
-		}

+		rproc_coredump_add_segment(rproc, da, resource_size(&res));
 		rproc_add_carveout(rproc, mem);
 	}

@@ -723,47 +718,37 @@ static int imx_rproc_addr_init(struct imx_rproc *priv,
 	}

 	/* memory-region is optional property */
-	nph = of_count_phandle_with_args(np, "memory-region", NULL);
+	nph = of_reserved_mem_region_count(np);
 	if (nph <= 0)
 		return 0;

 	/* remap optional addresses */
 	for (a = 0; a < nph; a++) {
-		struct device_node *node;
 		struct resource res;

-		node = of_parse_phandle(np, "memory-region", a);
-		if (!node)
-			continue;
-		/* Not map vdevbuffer, vdevring region */
-		if (!strncmp(node->name, "vdev", strlen("vdev"))) {
-			of_node_put(node);
-			continue;
-		}
-		err = of_address_to_resource(node, 0, &res);
+		err = of_reserved_mem_region_to_resource(np, a, &res);
 		if (err) {
 			dev_err(dev, "unable to resolve memory region\n");
-			of_node_put(node);
 			return err;
 		}

-		if (b >= IMX_RPROC_MEM_MAX) {
-			of_node_put(node);
+		/* Not map vdevbuffer, vdevring region */
+		if (!strncmp(res.name, "vdev", strlen("vdev")))
+			continue;
+
+		if (b >= IMX_RPROC_MEM_MAX)
 			break;
-		}

 		/* Not use resource version, because we might share region */
-		priv->mem[b].cpu_addr = devm_ioremap_wc(&pdev->dev, res.start, resource_size(&res));
+		priv->mem[b].cpu_addr = devm_ioremap_resource_wc(&pdev->dev, &res);
 		if (!priv->mem[b].cpu_addr) {
 			dev_err(dev, "failed to remap %pr\n", &res);
-			of_node_put(node);
 			return -ENOMEM;
 		}
 		priv->mem[b].sys_addr = res.start;
 		priv->mem[b].size = resource_size(&res);
-		if (!strcmp(node->name, "rsc-table"))
+		if (!strcmp(res.name, "rsc-table"))
 			priv->rsc_table = priv->mem[b].cpu_addr;
-		of_node_put(node);
 		b++;
 	}

diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c b/drivers/remoteproc/qcom_q6v5_adsp.c
index 94af77baa7a1..a5b7cbb8fe07 100644
--- a/drivers/remoteproc/qcom_q6v5_adsp.c
+++ b/drivers/remoteproc/qcom_q6v5_adsp.c
@@ -625,26 +625,20 @@ static int adsp_init_mmio(struct qcom_adsp *adsp,

 static int adsp_alloc_memory_region(struct qcom_adsp *adsp)
 {
-	struct reserved_mem *rmem = NULL;
-	struct device_node *node;
-
-	node = of_parse_phandle(adsp->dev->of_node, "memory-region", 0);
-	if (node)
-		rmem = of_reserved_mem_lookup(node);
-	of_node_put(node);
+	int ret;
+	struct resource res;

-	if (!rmem) {
+	ret = of_reserved_mem_region_to_resource(adsp->dev->of_node, 0, &res);
+	if (!ret) {
 		dev_err(adsp->dev, "unable to resolve memory-region\n");
-		return -EINVAL;
+		return ret;
 	}

-	adsp->mem_phys = adsp->mem_reloc = rmem->base;
-	adsp->mem_size = rmem->size;
-	adsp->mem_region = devm_ioremap_wc(adsp->dev,
-				adsp->mem_phys, adsp->mem_size);
+	adsp->mem_phys = adsp->mem_reloc = res.start;
+	adsp->mem_size = resource_size(&res);
+	adsp->mem_region = devm_ioremap_resource_wc(adsp->dev, &res);
 	if (!adsp->mem_region) {
-		dev_err(adsp->dev, "unable to map memory region: %pa+%zx\n",
-			&rmem->base, adsp->mem_size);
+		dev_err(adsp->dev, "unable to map memory region: %pR\n", &res);
 		return -EBUSY;
 	}

diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index e78bd986dc3f..905a142bd65d 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -1881,8 +1881,8 @@ static int q6v5_init_reset(struct q6v5 *qproc)
 static int q6v5_alloc_memory_region(struct q6v5 *qproc)
 {
 	struct device_node *child;
-	struct reserved_mem *rmem;
-	struct device_node *node;
+	struct resource res;
+	int ret;

 	/*
 	 * In the absence of mba/mpss sub-child, extract the mba and mpss
@@ -1890,71 +1890,49 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc)
 	 */
 	child = of_get_child_by_name(qproc->dev->of_node, "mba");
 	if (!child) {
-		node = of_parse_phandle(qproc->dev->of_node,
-					"memory-region", 0);
+		ret = of_reserved_mem_region_to_resource(qproc->dev->of_node, 0, &res);
 	} else {
-		node = of_parse_phandle(child, "memory-region", 0);
+		ret = of_reserved_mem_region_to_resource(child, 0, &res);
 		of_node_put(child);
 	}

-	if (!node) {
-		dev_err(qproc->dev, "no mba memory-region specified\n");
-		return -EINVAL;
-	}
-
-	rmem = of_reserved_mem_lookup(node);
-	of_node_put(node);
-	if (!rmem) {
+	if (ret) {
 		dev_err(qproc->dev, "unable to resolve mba region\n");
-		return -EINVAL;
+		return ret;
 	}

-	qproc->mba_phys = rmem->base;
-	qproc->mba_size = rmem->size;
+	qproc->mba_phys = res.start;
+	qproc->mba_size = resource_size(&res);

 	if (!child) {
-		node = of_parse_phandle(qproc->dev->of_node,
-					"memory-region", 1);
+		ret = of_reserved_mem_region_to_resource(qproc->dev->of_node, 1, &res);
 	} else {
 		child = of_get_child_by_name(qproc->dev->of_node, "mpss");
-		node = of_parse_phandle(child, "memory-region", 0);
+		ret = of_reserved_mem_region_to_resource(child, 0, &res);
 		of_node_put(child);
 	}

-	if (!node) {
-		dev_err(qproc->dev, "no mpss memory-region specified\n");
-		return -EINVAL;
-	}
-
-	rmem = of_reserved_mem_lookup(node);
-	of_node_put(node);
-	if (!rmem) {
+	if (ret) {
 		dev_err(qproc->dev, "unable to resolve mpss region\n");
-		return -EINVAL;
+		return ret;
 	}

-	qproc->mpss_phys = qproc->mpss_reloc = rmem->base;
-	qproc->mpss_size = rmem->size;
+	qproc->mpss_phys = qproc->mpss_reloc = res.start;
+	qproc->mpss_size = resource_size(&res);

 	if (!child) {
-		node = of_parse_phandle(qproc->dev->of_node, "memory-region", 2);
+		ret = of_reserved_mem_region_to_resource(qproc->dev->of_node, 2, &res);
 	} else {
 		child = of_get_child_by_name(qproc->dev->of_node, "metadata");
-		node = of_parse_phandle(child, "memory-region", 0);
+		ret = of_reserved_mem_region_to_resource(child, 0, &res);
 		of_node_put(child);
 	}

-	if (!node)
+	if (ret)
 		return 0;

-	rmem = of_reserved_mem_lookup(node);
-	if (!rmem) {
-		dev_err(qproc->dev, "unable to resolve metadata region\n");
-		return -EINVAL;
-	}
-
-	qproc->mdata_phys = rmem->base;
-	qproc->mdata_size = rmem->size;
+	qproc->mdata_phys = res.start;
+	qproc->mdata_size = resource_size(&res);

 	return 0;
 }
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
index 97c4bdd9222a..0ebd2ce0477b 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -546,53 +546,37 @@ static void adsp_pds_detach(struct qcom_adsp *adsp, struct device **pds,

 static int adsp_alloc_memory_region(struct qcom_adsp *adsp)
 {
-	struct reserved_mem *rmem;
-	struct device_node *node;
-
-	node = of_parse_phandle(adsp->dev->of_node, "memory-region", 0);
-	if (!node) {
-		dev_err(adsp->dev, "no memory-region specified\n");
-		return -EINVAL;
-	}
+	struct resource res;
+	int ret;

-	rmem = of_reserved_mem_lookup(node);
-	of_node_put(node);
-	if (!rmem) {
+	ret = of_reserved_mem_region_to_resource(adsp->dev->of_node, 0, &res);
+	if (ret) {
 		dev_err(adsp->dev, "unable to resolve memory-region\n");
-		return -EINVAL;
+		return ret;
 	}

-	adsp->mem_phys = adsp->mem_reloc = rmem->base;
-	adsp->mem_size = rmem->size;
-	adsp->mem_region = devm_ioremap_wc(adsp->dev, adsp->mem_phys, adsp->mem_size);
+	adsp->mem_phys = adsp->mem_reloc = res.start;
+	adsp->mem_size = resource_size(&res);
+	adsp->mem_region = devm_ioremap_resource_wc(adsp->dev, &res);
 	if (!adsp->mem_region) {
-		dev_err(adsp->dev, "unable to map memory region: %pa+%zx\n",
-			&rmem->base, adsp->mem_size);
+		dev_err(adsp->dev, "unable to map memory region: %pR\n", &res);
 		return -EBUSY;
 	}

 	if (!adsp->dtb_pas_id)
 		return 0;

-	node = of_parse_phandle(adsp->dev->of_node, "memory-region", 1);
-	if (!node) {
-		dev_err(adsp->dev, "no dtb memory-region specified\n");
-		return -EINVAL;
-	}
-
-	rmem = of_reserved_mem_lookup(node);
-	of_node_put(node);
-	if (!rmem) {
+	ret = of_reserved_mem_region_to_resource(adsp->dev->of_node, 1, &res);
+	if (ret) {
 		dev_err(adsp->dev, "unable to resolve dtb memory-region\n");
-		return -EINVAL;
+		return ret;
 	}

-	adsp->dtb_mem_phys = adsp->dtb_mem_reloc = rmem->base;
-	adsp->dtb_mem_size = rmem->size;
-	adsp->dtb_mem_region = devm_ioremap_wc(adsp->dev, adsp->dtb_mem_phys, adsp->dtb_mem_size);
+	adsp->dtb_mem_phys = adsp->dtb_mem_reloc = res.start;
+	adsp->dtb_mem_size = resource_size(&res);
+	adsp->dtb_mem_region = devm_ioremap_resource_wc(adsp->dev, &res);
 	if (!adsp->dtb_mem_region) {
-		dev_err(adsp->dev, "unable to map dtb memory region: %pa+%zx\n",
-			&rmem->base, adsp->dtb_mem_size);
+		dev_err(adsp->dev, "unable to map dtb memory region: %pR\n", &res);
 		return -EBUSY;
 	}

@@ -602,7 +586,6 @@ static int adsp_alloc_memory_region(struct qcom_adsp *adsp)
 static int adsp_assign_memory_region(struct qcom_adsp *adsp)
 {
 	struct qcom_scm_vmperm perm[MAX_ASSIGN_COUNT];
-	struct device_node *node;
 	unsigned int perm_size;
 	int offset;
 	int ret;
@@ -611,17 +594,15 @@ static int adsp_assign_memory_region(struct qcom_adsp *adsp)
 		return 0;

 	for (offset = 0; offset < adsp->region_assign_count; ++offset) {
-		struct reserved_mem *rmem = NULL;
-
-		node = of_parse_phandle(adsp->dev->of_node, "memory-region",
-					adsp->region_assign_idx + offset);
-		if (node)
-			rmem = of_reserved_mem_lookup(node);
-		of_node_put(node);
-		if (!rmem) {
+		struct resource res;
+
+		ret = of_reserved_mem_region_to_resource(adsp->dev->of_node,
+							 adsp->region_assign_idx + offset,
+							 &res);
+		if (ret) {
 			dev_err(adsp->dev, "unable to resolve shareable memory-region index %d\n",
 				offset);
-			return -EINVAL;
+			return ret;
 		}

 		if (adsp->region_assign_shared)  {
@@ -636,8 +617,8 @@ static int adsp_assign_memory_region(struct qcom_adsp *adsp)
 			perm_size = 1;
 		}

-		adsp->region_assign_phys[offset] = rmem->base;
-		adsp->region_assign_size[offset] = rmem->size;
+		adsp->region_assign_phys[offset] = res.start;
+		adsp->region_assign_size[offset] = resource_size(&res);
 		adsp->region_assign_owners[offset] = BIT(QCOM_SCM_VMID_HLOS);

 		ret = qcom_scm_assign_mem(adsp->region_assign_phys[offset],
diff --git a/drivers/remoteproc/qcom_q6v5_wcss.c b/drivers/remoteproc/qcom_q6v5_wcss.c
index 93648734a2f2..4a3235ee0963 100644
--- a/drivers/remoteproc/qcom_q6v5_wcss.c
+++ b/drivers/remoteproc/qcom_q6v5_wcss.c
@@ -873,27 +873,22 @@ static int q6v5_wcss_init_mmio(struct q6v5_wcss *wcss,

 static int q6v5_alloc_memory_region(struct q6v5_wcss *wcss)
 {
-	struct reserved_mem *rmem = NULL;
-	struct device_node *node;
 	struct device *dev = wcss->dev;
+	struct resource res;
+	int ret;

-	node = of_parse_phandle(dev->of_node, "memory-region", 0);
-	if (node)
-		rmem = of_reserved_mem_lookup(node);
-	of_node_put(node);
-
-	if (!rmem) {
+	ret = of_reserved_mem_region_to_resource(dev->of_node, 0, &res);
+	if (ret) {
 		dev_err(dev, "unable to acquire memory-region\n");
-		return -EINVAL;
+		return ret;
 	}

-	wcss->mem_phys = rmem->base;
-	wcss->mem_reloc = rmem->base;
-	wcss->mem_size = rmem->size;
-	wcss->mem_region = devm_ioremap_wc(dev, wcss->mem_phys, wcss->mem_size);
+	wcss->mem_phys = res.start;
+	wcss->mem_reloc = res.start;
+	wcss->mem_size = resource_size(&res);
+	wcss->mem_region = devm_ioremap_resource_wc(dev, &res);
 	if (!wcss->mem_region) {
-		dev_err(dev, "unable to map memory region: %pa+%pa\n",
-			&rmem->base, &rmem->size);
+		dev_err(dev, "unable to map memory region: %pR\n", &res);
 		return -EBUSY;
 	}

diff --git a/drivers/remoteproc/qcom_wcnss.c b/drivers/remoteproc/qcom_wcnss.c
index 5b5664603eed..d99a324bd532 100644
--- a/drivers/remoteproc/qcom_wcnss.c
+++ b/drivers/remoteproc/qcom_wcnss.c
@@ -506,25 +506,20 @@ static int wcnss_request_irq(struct qcom_wcnss *wcnss,

 static int wcnss_alloc_memory_region(struct qcom_wcnss *wcnss)
 {
-	struct reserved_mem *rmem = NULL;
-	struct device_node *node;
-
-	node = of_parse_phandle(wcnss->dev->of_node, "memory-region", 0);
-	if (node)
-		rmem = of_reserved_mem_lookup(node);
-	of_node_put(node);
+	struct resource res;
+	int ret;

-	if (!rmem) {
+	ret = of_reserved_mem_region_to_resource(wcnss->dev->of_node, 0, &res);
+	if (ret) {
 		dev_err(wcnss->dev, "unable to resolve memory-region\n");
-		return -EINVAL;
+		return ret;
 	}

-	wcnss->mem_phys = wcnss->mem_reloc = rmem->base;
-	wcnss->mem_size = rmem->size;
-	wcnss->mem_region = devm_ioremap_wc(wcnss->dev, wcnss->mem_phys, wcnss->mem_size);
+	wcnss->mem_phys = wcnss->mem_reloc = res.start;
+	wcnss->mem_size = resource_size(&res);
+	wcnss->mem_region = devm_ioremap_resource_wc(wcnss->dev, &res);
 	if (!wcnss->mem_region) {
-		dev_err(wcnss->dev, "unable to map memory region: %pa+%zx\n",
-			&rmem->base, wcnss->mem_size);
+		dev_err(wcnss->dev, "unable to map memory region: %pR\n", &res);
 		return -EBUSY;
 	}

diff --git a/drivers/remoteproc/rcar_rproc.c b/drivers/remoteproc/rcar_rproc.c
index 921d853594f4..0be1a4073a94 100644
--- a/drivers/remoteproc/rcar_rproc.c
+++ b/drivers/remoteproc/rcar_rproc.c
@@ -52,41 +52,33 @@ static int rcar_rproc_prepare(struct rproc *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;
+	int i = 0;
 	u32 da;

 	/* Register associated reserved memory regions */
-	of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
-	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;
-		}
+	while (1) {
+		struct resource res;
+		int ret;
+
+		ret = of_reserved_mem_region_to_resource(np, i++, &res);
+		if (ret)
+			return 0;

-		if (rmem->base > U32_MAX) {
-			of_node_put(it.node);
+		if (res.start > U32_MAX)
 			return -EINVAL;
-		}

 		/* No need to translate pa to da, R-Car use same map */
-		da = rmem->base;
+		da = res.start;
 		mem = rproc_mem_entry_init(dev, NULL,
-					   rmem->base,
-					   rmem->size, da,
+					   res.start,
+					   resource_size(&res), da,
 					   rcar_rproc_mem_alloc,
 					   rcar_rproc_mem_release,
-					   it.node->name);
+					   res.name);

-		if (!mem) {
-			of_node_put(it.node);
+		if (!mem)
 			return -ENOMEM;
-		}

 		rproc_add_carveout(rproc, mem);
 	}
diff --git a/drivers/remoteproc/st_remoteproc.c b/drivers/remoteproc/st_remoteproc.c
index e6566a9839dc..901b90de4953 100644
--- a/drivers/remoteproc/st_remoteproc.c
+++ b/drivers/remoteproc/st_remoteproc.c
@@ -120,40 +120,35 @@ static int st_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
 	struct device *dev = rproc->dev.parent;
 	struct device_node *np = dev->of_node;
 	struct rproc_mem_entry *mem;
-	struct reserved_mem *rmem;
-	struct of_phandle_iterator it;
-	int index = 0;
-
-	of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
-	while (of_phandle_iterator_next(&it) == 0) {
-		rmem = of_reserved_mem_lookup(it.node);
-		if (!rmem) {
-			of_node_put(it.node);
-			dev_err(dev, "unable to acquire memory-region\n");
-			return -EINVAL;
-		}
+	int index = 0, mr = 0;
+
+	while (1) {
+		struct resource res;
+		int ret;
+
+		ret = of_reserved_mem_region_to_resource(np, mr++, &res);
+		if (ret)
+			return 0;

 		/*  No need to map vdev buffer */
-		if (strcmp(it.node->name, "vdev0buffer")) {
+		if (strcmp(res.name, "vdev0buffer")) {
 			/* Register memory region */
 			mem = rproc_mem_entry_init(dev, NULL,
-						   (dma_addr_t)rmem->base,
-						   rmem->size, rmem->base,
+						   (dma_addr_t)res.start,
+						   resource_size(&res), res.start,
 						   st_rproc_mem_alloc,
 						   st_rproc_mem_release,
-						   it.node->name);
+						   res.name);
 		} else {
 			/* Register reserved memory for vdev buffer allocation */
 			mem = rproc_of_resm_mem_entry_init(dev, index,
-							   rmem->size,
-							   rmem->base,
-							   it.node->name);
+							   resource_size(&res),
+							   res.start,
+							   res.name);
 		}

-		if (!mem) {
-			of_node_put(it.node);
+		if (!mem)
 			return -ENOMEM;
-		}

 		rproc_add_carveout(rproc, mem);
 		index++;
diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
index b02b36a3f515..9d2bd8904c49 100644
--- a/drivers/remoteproc/stm32_rproc.c
+++ b/drivers/remoteproc/stm32_rproc.c
@@ -213,52 +213,46 @@ static int stm32_rproc_prepare(struct rproc *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;
 	u64 da;
-	int index = 0;
+	int index = 0, mr = 0;

 	/* Register associated reserved memory regions */
-	of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
-	while (of_phandle_iterator_next(&it) == 0) {
-		rmem = of_reserved_mem_lookup(it.node);
-		if (!rmem) {
-			of_node_put(it.node);
-			dev_err(dev, "unable to acquire memory-region\n");
-			return -EINVAL;
-		}
+	while (1) {
+		struct resource res;
+		int ret;
+
+		ret = of_reserved_mem_region_to_resource(np, mr++, &res);
+		if (ret)
+			return 0;

-		if (stm32_rproc_pa_to_da(rproc, rmem->base, &da) < 0) {
-			of_node_put(it.node);
-			dev_err(dev, "memory region not valid %pa\n",
-				&rmem->base);
+		if (stm32_rproc_pa_to_da(rproc, res.start, &da) < 0) {
+			dev_err(dev, "memory region not valid %pR\n", &res);
 			return -EINVAL;
 		}

 		/*  No need to map vdev buffer */
-		if (strcmp(it.node->name, "vdev0buffer")) {
+		if (strcmp(res.name, "vdev0buffer")) {
 			/* Register memory region */
 			mem = rproc_mem_entry_init(dev, NULL,
-						   (dma_addr_t)rmem->base,
-						   rmem->size, da,
+						   (dma_addr_t)res.start,
+						   resource_size(&res), da,
 						   stm32_rproc_mem_alloc,
 						   stm32_rproc_mem_release,
-						   it.node->name);
+						   res.name);

 			if (mem)
 				rproc_coredump_add_segment(rproc, da,
-							   rmem->size);
+							   resource_size(&res));
 		} else {
 			/* Register reserved memory for vdev buffer alloc */
 			mem = rproc_of_resm_mem_entry_init(dev, index,
-							   rmem->size,
-							   rmem->base,
-							   it.node->name);
+							   resource_size(&res),
+							   res.start,
+							   res.name);
 		}

 		if (!mem) {
-			of_node_put(it.node);
 			return -ENOMEM;
 		}

diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
index a695890254ff..f02c835535bc 100644
--- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
@@ -535,13 +535,10 @@ static int k3_dsp_reserved_mem_init(struct k3_dsp_rproc *kproc)
 {
 	struct device *dev = kproc->dev;
 	struct device_node *np = dev->of_node;
-	struct device_node *rmem_np;
-	struct reserved_mem *rmem;
 	int num_rmems;
 	int ret, i;

-	num_rmems = of_property_count_elems_of_size(np, "memory-region",
-						    sizeof(phandle));
+	num_rmems = of_reserved_mem_region_count(np);
 	if (num_rmems < 0) {
 		dev_err(dev, "device does not reserved memory regions (%pe)\n",
 			ERR_PTR(num_rmems));
@@ -571,23 +568,20 @@ static int k3_dsp_reserved_mem_init(struct k3_dsp_rproc *kproc)

 	/* use remaining reserved memory regions for static carveouts */
 	for (i = 0; i < num_rmems; i++) {
-		rmem_np = of_parse_phandle(np, "memory-region", i + 1);
-		if (!rmem_np)
-			return -EINVAL;
+		struct resource res;

-		rmem = of_reserved_mem_lookup(rmem_np);
-		of_node_put(rmem_np);
-		if (!rmem)
-			return -EINVAL;
+		ret = of_reserved_mem_region_to_resource(np, i + 1, &res);
+		if (ret)
+			return ret;

-		kproc->rmem[i].bus_addr = rmem->base;
+		kproc->rmem[i].bus_addr = res.start;
 		/* 64-bit address regions currently not supported */
-		kproc->rmem[i].dev_addr = (u32)rmem->base;
-		kproc->rmem[i].size = rmem->size;
-		kproc->rmem[i].cpu_addr = devm_ioremap_wc(dev, rmem->base, rmem->size);
+		kproc->rmem[i].dev_addr = (u32)res.start;
+		kproc->rmem[i].size = resource_size(&res);
+		kproc->rmem[i].cpu_addr = devm_ioremap_resource_wc(dev, &res);
 		if (!kproc->rmem[i].cpu_addr) {
-			dev_err(dev, "failed to map reserved memory#%d at %pa of size %pa\n",
-				i + 1, &rmem->base, &rmem->size);
+			dev_err(dev, "failed to map reserved memory#%d at %pR\n",
+				i + 1, &res);
 			return -ENOMEM;
 		}

diff --git a/drivers/remoteproc/ti_k3_m4_remoteproc.c b/drivers/remoteproc/ti_k3_m4_remoteproc.c
index a16fb165fced..8c7772cd6baf 100644
--- a/drivers/remoteproc/ti_k3_m4_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_m4_remoteproc.c
@@ -393,13 +393,10 @@ static int k3_m4_reserved_mem_init(struct k3_m4_rproc *kproc)
 {
 	struct device *dev = kproc->dev;
 	struct device_node *np = dev->of_node;
-	struct device_node *rmem_np;
-	struct reserved_mem *rmem;
 	int num_rmems;
 	int ret, i;

-	num_rmems = of_property_count_elems_of_size(np, "memory-region",
-						    sizeof(phandle));
+	num_rmems = of_reserved_mem_region_count(np);
 	if (num_rmems < 0) {
 		dev_err(dev, "device does not reserved memory regions (%d)\n",
 			num_rmems);
@@ -428,23 +425,20 @@ static int k3_m4_reserved_mem_init(struct k3_m4_rproc *kproc)

 	/* use remaining reserved memory regions for static carveouts */
 	for (i = 0; i < num_rmems; i++) {
-		rmem_np = of_parse_phandle(np, "memory-region", i + 1);
-		if (!rmem_np)
-			return -EINVAL;
+		struct resource res;

-		rmem = of_reserved_mem_lookup(rmem_np);
-		of_node_put(rmem_np);
-		if (!rmem)
-			return -EINVAL;
+		ret = of_reserved_mem_region_to_resource(np, i + 1, &res);
+		if (ret)
+			return ret;

-		kproc->rmem[i].bus_addr = rmem->base;
+		kproc->rmem[i].bus_addr = res.start;
 		/* 64-bit address regions currently not supported */
-		kproc->rmem[i].dev_addr = (u32)rmem->base;
-		kproc->rmem[i].size = rmem->size;
-		kproc->rmem[i].cpu_addr = devm_ioremap_wc(dev, rmem->base, rmem->size);
+		kproc->rmem[i].dev_addr = (u32)res.start;
+		kproc->rmem[i].size = resource_size(&res);
+		kproc->rmem[i].cpu_addr = devm_ioremap_resource_wc(dev, &res);
 		if (!kproc->rmem[i].cpu_addr) {
-			dev_err(dev, "failed to map reserved memory#%d at %pa of size %pa\n",
-				i + 1, &rmem->base, &rmem->size);
+			dev_err(dev, "failed to map reserved memory#%d at %pR\n",
+				i + 1, &res);
 			return -ENOMEM;
 		}

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index dbc513c5569c..9a7a61e0ecb8 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -966,13 +966,10 @@ static int k3_r5_reserved_mem_init(struct k3_r5_rproc *kproc)
 {
 	struct device *dev = kproc->dev;
 	struct device_node *np = dev_of_node(dev);
-	struct device_node *rmem_np;
-	struct reserved_mem *rmem;
 	int num_rmems;
 	int ret, i;

-	num_rmems = of_property_count_elems_of_size(np, "memory-region",
-						    sizeof(phandle));
+	num_rmems = of_reserved_mem_region_count(np);
 	if (num_rmems <= 0) {
 		dev_err(dev, "device does not have reserved memory regions, ret = %d\n",
 			num_rmems);
@@ -1003,16 +1000,13 @@ static int k3_r5_reserved_mem_init(struct k3_r5_rproc *kproc)

 	/* use remaining reserved memory regions for static carveouts */
 	for (i = 0; i < num_rmems; i++) {
-		rmem_np = of_parse_phandle(np, "memory-region", i + 1);
-		if (!rmem_np)
-			return -EINVAL;
+		struct resource res;

-		rmem = of_reserved_mem_lookup(rmem_np);
-		of_node_put(rmem_np);
-		if (!rmem)
-			return -EINVAL;
+		ret = of_reserved_mem_region_to_resource(np, i + 1, &res);
+		if (ret)
+			return ret;

-		kproc->rmem[i].bus_addr = rmem->base;
+		kproc->rmem[i].bus_addr = res.start;
 		/*
 		 * R5Fs do not have an MMU, but have a Region Address Translator
 		 * (RAT) module that provides a fixed entry translation between
@@ -1023,12 +1017,12 @@ static int k3_r5_reserved_mem_init(struct k3_r5_rproc *kproc)
 		 * addresses/supported memory regions are restricted to 32-bit
 		 * bus addresses, and are identical
 		 */
-		kproc->rmem[i].dev_addr = (u32)rmem->base;
-		kproc->rmem[i].size = rmem->size;
-		kproc->rmem[i].cpu_addr = devm_ioremap_wc(dev, rmem->base, rmem->size);
+		kproc->rmem[i].dev_addr = (u32)res.start;
+		kproc->rmem[i].size = resource_size(&res);
+		kproc->rmem[i].cpu_addr = devm_ioremap_resource_wc(dev, &res);
 		if (!kproc->rmem[i].cpu_addr) {
-			dev_err(dev, "failed to map reserved memory#%d at %pa of size %pa\n",
-				i + 1, &rmem->base, &rmem->size);
+			dev_err(dev, "failed to map reserved memory#%d at %pR\n",
+				i + 1, &res);
 			return -ENOMEM;
 		}

diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
index 5aeedeaf3c41..000eeadd5a01 100644
--- a/drivers/remoteproc/xlnx_r5_remoteproc.c
+++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
@@ -460,49 +460,42 @@ 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);
+		if (err)
+			return 0;

-		if (!strcmp(it.node->name, "vdev0buffer")) {
+		if (!strcmp(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,
+								 res.name);
 		} 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);
+							 res.name);
 		}

-		if (!rproc_mem) {
-			of_node_put(it.node);
+		if (!rproc_mem)
 			return -ENOMEM;
-		}

 		rproc_add_carveout(rproc, rproc_mem);
-		rproc_coredump_add_segment(rproc, rmem->base, rmem->size);
+		rproc_coredump_add_segment(rproc, res.start, resource_size(&res));

-		dev_dbg(&rproc->dev, "reserved mem carveout %s addr=%llx, size=0x%llx",
-			it.node->name, rmem->base, rmem->size);
+		dev_dbg(&rproc->dev, "reserved mem carveout %pR\n", &res);
 		i++;
 	}

@@ -776,7 +769,6 @@ static int zynqmp_r5_get_rsc_table_va(struct zynqmp_r5_core *r5_core)
 	struct device *dev = r5_core->dev;
 	struct rsc_tbl_data *rsc_data_va;
 	struct resource res_mem;
-	struct device_node *np;
 	int ret;

 	/*
@@ -786,14 +778,7 @@ static int zynqmp_r5_get_rsc_table_va(struct zynqmp_r5_core *r5_core)
 	 * contains that data structure which holds resource table address, size
 	 * and some magic number to validate correct resource table entry.
 	 */
-	np = of_parse_phandle(r5_core->np, "memory-region", 0);
-	if (!np) {
-		dev_err(dev, "failed to get memory region dev node\n");
-		return -EINVAL;
-	}
-
-	ret = of_address_to_resource(np, 0, &res_mem);
-	of_node_put(np);
+	ret = of_reserved_mem_region_to_resource(r5_core->np, 0, &res_mem);
 	if (ret) {
 		dev_err(dev, "failed to get memory-region resource addr\n");
 		return -EINVAL;
--
2.47.2

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

[Daniel Baluta]

On Tue, Mar 18, 2025 at 1:25 AM Rob Herring (Arm) <robh@kernel.org> 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) <robh@kernel.org>

For IMX part:

Reviewed-by: Daniel Baluta <daniel.baluta@nxp.com>

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

[neil.armstrong]

Hi,

On 18/03/2025 00:24, 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) <robh@kernel.org>
> ---
> For v6.16
> 
>   drivers/remoteproc/imx_dsp_rproc.c        | 44 ++++++---------
>   drivers/remoteproc/imx_rproc.c            | 65 ++++++++-------------
>   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        | 39 ++++++-------
>   drivers/remoteproc/stm32_rproc.c          | 42 ++++++--------
>   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   | 49 ++++++----------
>   14 files changed, 213 insertions(+), 347 deletions(-)
> 
<snip>

I get:
ERROR: modpost: "devm_ioremap_resource_wc" [drivers/remoteproc/qcom_q6v5_adsp.ko] undefined!
ERROR: modpost: "devm_ioremap_resource_wc" [drivers/remoteproc/qcom_q6v5_pas.ko] undefined!
ERROR: modpost: "devm_ioremap_resource_wc" [drivers/remoteproc/qcom_wcnss_pil.ko] undefined!
ERROR: modpost: "devm_ioremap_resource_wc" [drivers/remoteproc/ti_k3_dsp_remoteproc.ko] undefined!
ERROR: modpost: "devm_ioremap_resource_wc" [drivers/remoteproc/ti_k3_m4_remoteproc.ko] undefined!
ERROR: modpost: "devm_ioremap_resource_wc" [drivers/remoteproc/ti_k3_r5_remoteproc.ko] undefined!

when building with arm64 defconfig. I guess a proper:
EXPORT_SYMBOL(devm_ioremap_resource_wc)
is missing.

Neil

Re: [Linux-stm32] [PATCH 3/3] remoteproc: Use of_reserved_mem_region_* functions for "memory-region"

[Arnaud POULIQUEN]

Hello Rob,

On 3/18/25 00:24, 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) <robh@kernel.org>
> ---
> For v6.16
> 
>  drivers/remoteproc/imx_dsp_rproc.c        | 44 ++++++---------
>  drivers/remoteproc/imx_rproc.c            | 65 ++++++++-------------
>  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        | 39 ++++++-------
>  drivers/remoteproc/stm32_rproc.c          | 42 ++++++--------
>  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   | 49 ++++++----------
>  14 files changed, 213 insertions(+), 347 deletions(-)
> 
> diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
> index ea5024919c2f..f3f341f4a262 100644
> --- a/drivers/remoteproc/imx_dsp_rproc.c
> +++ b/drivers/remoteproc/imx_dsp_rproc.c
> @@ -595,11 +595,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 */
> @@ -630,45 +628,37 @@ 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 (!strcmp(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, res.name);
> +		if (!mem)
>  			return -ENOMEM;
> -		}
> 
> +		rproc_coredump_add_segment(rproc, da, resource_size(&res));
>  		rproc_add_carveout(rproc, mem);
>  	}
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 74299af1d7f1..fba95507b6cf 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -549,46 +549,41 @@ static int imx_rproc_prepare(struct rproc *rproc)
>  {
>  	struct imx_rproc *priv = rproc->priv;
>  	struct device_node *np = priv->dev->of_node;
> -	struct of_phandle_iterator it;
>  	struct rproc_mem_entry *mem;
> -	struct reserved_mem *rmem;
> +	int i = 0;
>  	u32 da;
> 
>  	/* Register associated reserved memory regions */
> -	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 (!strcmp(res.name, "vdev0buffer"))
>  			continue;
> 
> -		if (!strcmp(it.node->name, "rsc-table"))
> +		if (!strcmp(res.name, "rsc-table"))
>  			continue;
> 
> -		rmem = of_reserved_mem_lookup(it.node);
> -		if (!rmem) {
> -			of_node_put(it.node);
> -			dev_err(priv->dev, "unable to acquire memory-region\n");
> -			return -EINVAL;
> -		}
> -
>  		/* No need to translate pa to da, i.MX use same map */
> -		da = rmem->base;
> +		da = res.start;
> 
>  		/* Register memory region */
> -		mem = rproc_mem_entry_init(priv->dev, NULL, (dma_addr_t)rmem->base, rmem->size, da,
> +		mem = rproc_mem_entry_init(priv->dev, NULL, (dma_addr_t)res.start, resource_size(&res), da,
>  					   imx_rproc_mem_alloc, imx_rproc_mem_release,
> -					   it.node->name);
> +					   res.name);
> 
> -		if (mem) {
> -			rproc_coredump_add_segment(rproc, da, rmem->size);
> -		} else {
> -			of_node_put(it.node);
> +		if (!mem)
>  			return -ENOMEM;
> -		}
> 
> +		rproc_coredump_add_segment(rproc, da, resource_size(&res));
>  		rproc_add_carveout(rproc, mem);
>  	}
> 
> @@ -723,47 +718,37 @@ static int imx_rproc_addr_init(struct imx_rproc *priv,
>  	}
> 
>  	/* memory-region is optional property */
> -	nph = of_count_phandle_with_args(np, "memory-region", NULL);
> +	nph = of_reserved_mem_region_count(np);
>  	if (nph <= 0)
>  		return 0;
> 
>  	/* remap optional addresses */
>  	for (a = 0; a < nph; a++) {
> -		struct device_node *node;
>  		struct resource res;
> 
> -		node = of_parse_phandle(np, "memory-region", a);
> -		if (!node)
> -			continue;
> -		/* Not map vdevbuffer, vdevring region */
> -		if (!strncmp(node->name, "vdev", strlen("vdev"))) {
> -			of_node_put(node);
> -			continue;
> -		}
> -		err = of_address_to_resource(node, 0, &res);
> +		err = of_reserved_mem_region_to_resource(np, a, &res);
>  		if (err) {
>  			dev_err(dev, "unable to resolve memory region\n");
> -			of_node_put(node);
>  			return err;
>  		}
> 
> -		if (b >= IMX_RPROC_MEM_MAX) {
> -			of_node_put(node);
> +		/* Not map vdevbuffer, vdevring region */
> +		if (!strncmp(res.name, "vdev", strlen("vdev")))
> +			continue;
> +
> +		if (b >= IMX_RPROC_MEM_MAX)
>  			break;
> -		}
> 
>  		/* Not use resource version, because we might share region */
> -		priv->mem[b].cpu_addr = devm_ioremap_wc(&pdev->dev, res.start, resource_size(&res));
> +		priv->mem[b].cpu_addr = devm_ioremap_resource_wc(&pdev->dev, &res);
>  		if (!priv->mem[b].cpu_addr) {
>  			dev_err(dev, "failed to remap %pr\n", &res);
> -			of_node_put(node);
>  			return -ENOMEM;
>  		}
>  		priv->mem[b].sys_addr = res.start;
>  		priv->mem[b].size = resource_size(&res);
> -		if (!strcmp(node->name, "rsc-table"))
> +		if (!strcmp(res.name, "rsc-table"))
>  			priv->rsc_table = priv->mem[b].cpu_addr;
> -		of_node_put(node);
>  		b++;
>  	}
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c b/drivers/remoteproc/qcom_q6v5_adsp.c
> index 94af77baa7a1..a5b7cbb8fe07 100644
> --- a/drivers/remoteproc/qcom_q6v5_adsp.c
> +++ b/drivers/remoteproc/qcom_q6v5_adsp.c
> @@ -625,26 +625,20 @@ static int adsp_init_mmio(struct qcom_adsp *adsp,
> 
>  static int adsp_alloc_memory_region(struct qcom_adsp *adsp)
>  {
> -	struct reserved_mem *rmem = NULL;
> -	struct device_node *node;
> -
> -	node = of_parse_phandle(adsp->dev->of_node, "memory-region", 0);
> -	if (node)
> -		rmem = of_reserved_mem_lookup(node);
> -	of_node_put(node);
> +	int ret;
> +	struct resource res;
> 
> -	if (!rmem) {
> +	ret = of_reserved_mem_region_to_resource(adsp->dev->of_node, 0, &res);
> +	if (!ret) {
>  		dev_err(adsp->dev, "unable to resolve memory-region\n");
> -		return -EINVAL;
> +		return ret;
>  	}
> 
> -	adsp->mem_phys = adsp->mem_reloc = rmem->base;
> -	adsp->mem_size = rmem->size;
> -	adsp->mem_region = devm_ioremap_wc(adsp->dev,
> -				adsp->mem_phys, adsp->mem_size);
> +	adsp->mem_phys = adsp->mem_reloc = res.start;
> +	adsp->mem_size = resource_size(&res);
> +	adsp->mem_region = devm_ioremap_resource_wc(adsp->dev, &res);
>  	if (!adsp->mem_region) {
> -		dev_err(adsp->dev, "unable to map memory region: %pa+%zx\n",
> -			&rmem->base, adsp->mem_size);
> +		dev_err(adsp->dev, "unable to map memory region: %pR\n", &res);
>  		return -EBUSY;
>  	}
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index e78bd986dc3f..905a142bd65d 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -1881,8 +1881,8 @@ static int q6v5_init_reset(struct q6v5 *qproc)
>  static int q6v5_alloc_memory_region(struct q6v5 *qproc)
>  {
>  	struct device_node *child;
> -	struct reserved_mem *rmem;
> -	struct device_node *node;
> +	struct resource res;
> +	int ret;
> 
>  	/*
>  	 * In the absence of mba/mpss sub-child, extract the mba and mpss
> @@ -1890,71 +1890,49 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc)
>  	 */
>  	child = of_get_child_by_name(qproc->dev->of_node, "mba");
>  	if (!child) {
> -		node = of_parse_phandle(qproc->dev->of_node,
> -					"memory-region", 0);
> +		ret = of_reserved_mem_region_to_resource(qproc->dev->of_node, 0, &res);
>  	} else {
> -		node = of_parse_phandle(child, "memory-region", 0);
> +		ret = of_reserved_mem_region_to_resource(child, 0, &res);
>  		of_node_put(child);
>  	}
> 
> -	if (!node) {
> -		dev_err(qproc->dev, "no mba memory-region specified\n");
> -		return -EINVAL;
> -	}
> -
> -	rmem = of_reserved_mem_lookup(node);
> -	of_node_put(node);
> -	if (!rmem) {
> +	if (ret) {
>  		dev_err(qproc->dev, "unable to resolve mba region\n");
> -		return -EINVAL;
> +		return ret;
>  	}
> 
> -	qproc->mba_phys = rmem->base;
> -	qproc->mba_size = rmem->size;
> +	qproc->mba_phys = res.start;
> +	qproc->mba_size = resource_size(&res);
> 
>  	if (!child) {
> -		node = of_parse_phandle(qproc->dev->of_node,
> -					"memory-region", 1);
> +		ret = of_reserved_mem_region_to_resource(qproc->dev->of_node, 1, &res);
>  	} else {
>  		child = of_get_child_by_name(qproc->dev->of_node, "mpss");
> -		node = of_parse_phandle(child, "memory-region", 0);
> +		ret = of_reserved_mem_region_to_resource(child, 0, &res);
>  		of_node_put(child);
>  	}
> 
> -	if (!node) {
> -		dev_err(qproc->dev, "no mpss memory-region specified\n");
> -		return -EINVAL;
> -	}
> -
> -	rmem = of_reserved_mem_lookup(node);
> -	of_node_put(node);
> -	if (!rmem) {
> +	if (ret) {
>  		dev_err(qproc->dev, "unable to resolve mpss region\n");
> -		return -EINVAL;
> +		return ret;
>  	}
> 
> -	qproc->mpss_phys = qproc->mpss_reloc = rmem->base;
> -	qproc->mpss_size = rmem->size;
> +	qproc->mpss_phys = qproc->mpss_reloc = res.start;
> +	qproc->mpss_size = resource_size(&res);
> 
>  	if (!child) {
> -		node = of_parse_phandle(qproc->dev->of_node, "memory-region", 2);
> +		ret = of_reserved_mem_region_to_resource(qproc->dev->of_node, 2, &res);
>  	} else {
>  		child = of_get_child_by_name(qproc->dev->of_node, "metadata");
> -		node = of_parse_phandle(child, "memory-region", 0);
> +		ret = of_reserved_mem_region_to_resource(child, 0, &res);
>  		of_node_put(child);
>  	}
> 
> -	if (!node)
> +	if (ret)
>  		return 0;
> 
> -	rmem = of_reserved_mem_lookup(node);
> -	if (!rmem) {
> -		dev_err(qproc->dev, "unable to resolve metadata region\n");
> -		return -EINVAL;
> -	}
> -
> -	qproc->mdata_phys = rmem->base;
> -	qproc->mdata_size = rmem->size;
> +	qproc->mdata_phys = res.start;
> +	qproc->mdata_size = resource_size(&res);
> 
>  	return 0;
>  }
> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index 97c4bdd9222a..0ebd2ce0477b 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -546,53 +546,37 @@ static void adsp_pds_detach(struct qcom_adsp *adsp, struct device **pds,
> 
>  static int adsp_alloc_memory_region(struct qcom_adsp *adsp)
>  {
> -	struct reserved_mem *rmem;
> -	struct device_node *node;
> -
> -	node = of_parse_phandle(adsp->dev->of_node, "memory-region", 0);
> -	if (!node) {
> -		dev_err(adsp->dev, "no memory-region specified\n");
> -		return -EINVAL;
> -	}
> +	struct resource res;
> +	int ret;
> 
> -	rmem = of_reserved_mem_lookup(node);
> -	of_node_put(node);
> -	if (!rmem) {
> +	ret = of_reserved_mem_region_to_resource(adsp->dev->of_node, 0, &res);
> +	if (ret) {
>  		dev_err(adsp->dev, "unable to resolve memory-region\n");
> -		return -EINVAL;
> +		return ret;
>  	}
> 
> -	adsp->mem_phys = adsp->mem_reloc = rmem->base;
> -	adsp->mem_size = rmem->size;
> -	adsp->mem_region = devm_ioremap_wc(adsp->dev, adsp->mem_phys, adsp->mem_size);
> +	adsp->mem_phys = adsp->mem_reloc = res.start;
> +	adsp->mem_size = resource_size(&res);
> +	adsp->mem_region = devm_ioremap_resource_wc(adsp->dev, &res);
>  	if (!adsp->mem_region) {
> -		dev_err(adsp->dev, "unable to map memory region: %pa+%zx\n",
> -			&rmem->base, adsp->mem_size);
> +		dev_err(adsp->dev, "unable to map memory region: %pR\n", &res);
>  		return -EBUSY;
>  	}
> 
>  	if (!adsp->dtb_pas_id)
>  		return 0;
> 
> -	node = of_parse_phandle(adsp->dev->of_node, "memory-region", 1);
> -	if (!node) {
> -		dev_err(adsp->dev, "no dtb memory-region specified\n");
> -		return -EINVAL;
> -	}
> -
> -	rmem = of_reserved_mem_lookup(node);
> -	of_node_put(node);
> -	if (!rmem) {
> +	ret = of_reserved_mem_region_to_resource(adsp->dev->of_node, 1, &res);
> +	if (ret) {
>  		dev_err(adsp->dev, "unable to resolve dtb memory-region\n");
> -		return -EINVAL;
> +		return ret;
>  	}
> 
> -	adsp->dtb_mem_phys = adsp->dtb_mem_reloc = rmem->base;
> -	adsp->dtb_mem_size = rmem->size;
> -	adsp->dtb_mem_region = devm_ioremap_wc(adsp->dev, adsp->dtb_mem_phys, adsp->dtb_mem_size);
> +	adsp->dtb_mem_phys = adsp->dtb_mem_reloc = res.start;
> +	adsp->dtb_mem_size = resource_size(&res);
> +	adsp->dtb_mem_region = devm_ioremap_resource_wc(adsp->dev, &res);
>  	if (!adsp->dtb_mem_region) {
> -		dev_err(adsp->dev, "unable to map dtb memory region: %pa+%zx\n",
> -			&rmem->base, adsp->dtb_mem_size);
> +		dev_err(adsp->dev, "unable to map dtb memory region: %pR\n", &res);
>  		return -EBUSY;
>  	}
> 
> @@ -602,7 +586,6 @@ static int adsp_alloc_memory_region(struct qcom_adsp *adsp)
>  static int adsp_assign_memory_region(struct qcom_adsp *adsp)
>  {
>  	struct qcom_scm_vmperm perm[MAX_ASSIGN_COUNT];
> -	struct device_node *node;
>  	unsigned int perm_size;
>  	int offset;
>  	int ret;
> @@ -611,17 +594,15 @@ static int adsp_assign_memory_region(struct qcom_adsp *adsp)
>  		return 0;
> 
>  	for (offset = 0; offset < adsp->region_assign_count; ++offset) {
> -		struct reserved_mem *rmem = NULL;
> -
> -		node = of_parse_phandle(adsp->dev->of_node, "memory-region",
> -					adsp->region_assign_idx + offset);
> -		if (node)
> -			rmem = of_reserved_mem_lookup(node);
> -		of_node_put(node);
> -		if (!rmem) {
> +		struct resource res;
> +
> +		ret = of_reserved_mem_region_to_resource(adsp->dev->of_node,
> +							 adsp->region_assign_idx + offset,
> +							 &res);
> +		if (ret) {
>  			dev_err(adsp->dev, "unable to resolve shareable memory-region index %d\n",
>  				offset);
> -			return -EINVAL;
> +			return ret;
>  		}
> 
>  		if (adsp->region_assign_shared)  {
> @@ -636,8 +617,8 @@ static int adsp_assign_memory_region(struct qcom_adsp *adsp)
>  			perm_size = 1;
>  		}
> 
> -		adsp->region_assign_phys[offset] = rmem->base;
> -		adsp->region_assign_size[offset] = rmem->size;
> +		adsp->region_assign_phys[offset] = res.start;
> +		adsp->region_assign_size[offset] = resource_size(&res);
>  		adsp->region_assign_owners[offset] = BIT(QCOM_SCM_VMID_HLOS);
> 
>  		ret = qcom_scm_assign_mem(adsp->region_assign_phys[offset],
> diff --git a/drivers/remoteproc/qcom_q6v5_wcss.c b/drivers/remoteproc/qcom_q6v5_wcss.c
> index 93648734a2f2..4a3235ee0963 100644
> --- a/drivers/remoteproc/qcom_q6v5_wcss.c
> +++ b/drivers/remoteproc/qcom_q6v5_wcss.c
> @@ -873,27 +873,22 @@ static int q6v5_wcss_init_mmio(struct q6v5_wcss *wcss,
> 
>  static int q6v5_alloc_memory_region(struct q6v5_wcss *wcss)
>  {
> -	struct reserved_mem *rmem = NULL;
> -	struct device_node *node;
>  	struct device *dev = wcss->dev;
> +	struct resource res;
> +	int ret;
> 
> -	node = of_parse_phandle(dev->of_node, "memory-region", 0);
> -	if (node)
> -		rmem = of_reserved_mem_lookup(node);
> -	of_node_put(node);
> -
> -	if (!rmem) {
> +	ret = of_reserved_mem_region_to_resource(dev->of_node, 0, &res);
> +	if (ret) {
>  		dev_err(dev, "unable to acquire memory-region\n");
> -		return -EINVAL;
> +		return ret;
>  	}
> 
> -	wcss->mem_phys = rmem->base;
> -	wcss->mem_reloc = rmem->base;
> -	wcss->mem_size = rmem->size;
> -	wcss->mem_region = devm_ioremap_wc(dev, wcss->mem_phys, wcss->mem_size);
> +	wcss->mem_phys = res.start;
> +	wcss->mem_reloc = res.start;
> +	wcss->mem_size = resource_size(&res);
> +	wcss->mem_region = devm_ioremap_resource_wc(dev, &res);
>  	if (!wcss->mem_region) {
> -		dev_err(dev, "unable to map memory region: %pa+%pa\n",
> -			&rmem->base, &rmem->size);
> +		dev_err(dev, "unable to map memory region: %pR\n", &res);
>  		return -EBUSY;
>  	}
> 
> diff --git a/drivers/remoteproc/qcom_wcnss.c b/drivers/remoteproc/qcom_wcnss.c
> index 5b5664603eed..d99a324bd532 100644
> --- a/drivers/remoteproc/qcom_wcnss.c
> +++ b/drivers/remoteproc/qcom_wcnss.c
> @@ -506,25 +506,20 @@ static int wcnss_request_irq(struct qcom_wcnss *wcnss,
> 
>  static int wcnss_alloc_memory_region(struct qcom_wcnss *wcnss)
>  {
> -	struct reserved_mem *rmem = NULL;
> -	struct device_node *node;
> -
> -	node = of_parse_phandle(wcnss->dev->of_node, "memory-region", 0);
> -	if (node)
> -		rmem = of_reserved_mem_lookup(node);
> -	of_node_put(node);
> +	struct resource res;
> +	int ret;
> 
> -	if (!rmem) {
> +	ret = of_reserved_mem_region_to_resource(wcnss->dev->of_node, 0, &res);
> +	if (ret) {
>  		dev_err(wcnss->dev, "unable to resolve memory-region\n");
> -		return -EINVAL;
> +		return ret;
>  	}
> 
> -	wcnss->mem_phys = wcnss->mem_reloc = rmem->base;
> -	wcnss->mem_size = rmem->size;
> -	wcnss->mem_region = devm_ioremap_wc(wcnss->dev, wcnss->mem_phys, wcnss->mem_size);
> +	wcnss->mem_phys = wcnss->mem_reloc = res.start;
> +	wcnss->mem_size = resource_size(&res);
> +	wcnss->mem_region = devm_ioremap_resource_wc(wcnss->dev, &res);
>  	if (!wcnss->mem_region) {
> -		dev_err(wcnss->dev, "unable to map memory region: %pa+%zx\n",
> -			&rmem->base, wcnss->mem_size);
> +		dev_err(wcnss->dev, "unable to map memory region: %pR\n", &res);
>  		return -EBUSY;
>  	}
> 
> diff --git a/drivers/remoteproc/rcar_rproc.c b/drivers/remoteproc/rcar_rproc.c
> index 921d853594f4..0be1a4073a94 100644
> --- a/drivers/remoteproc/rcar_rproc.c
> +++ b/drivers/remoteproc/rcar_rproc.c
> @@ -52,41 +52,33 @@ static int rcar_rproc_prepare(struct rproc *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;
> +	int i = 0;
>  	u32 da;
> 
>  	/* Register associated reserved memory regions */
> -	of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
> -	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;
> -		}
> +	while (1) {
> +		struct resource res;
> +		int ret;
> +
> +		ret = of_reserved_mem_region_to_resource(np, i++, &res);
> +		if (ret)
> +			return 0;
> 
> -		if (rmem->base > U32_MAX) {
> -			of_node_put(it.node);
> +		if (res.start > U32_MAX)
>  			return -EINVAL;
> -		}
> 
>  		/* No need to translate pa to da, R-Car use same map */
> -		da = rmem->base;
> +		da = res.start;
>  		mem = rproc_mem_entry_init(dev, NULL,
> -					   rmem->base,
> -					   rmem->size, da,
> +					   res.start,
> +					   resource_size(&res), da,
>  					   rcar_rproc_mem_alloc,
>  					   rcar_rproc_mem_release,
> -					   it.node->name);
> +					   res.name);
> 
> -		if (!mem) {
> -			of_node_put(it.node);
> +		if (!mem)
>  			return -ENOMEM;
> -		}
> 
>  		rproc_add_carveout(rproc, mem);
>  	}
> diff --git a/drivers/remoteproc/st_remoteproc.c b/drivers/remoteproc/st_remoteproc.c
> index e6566a9839dc..901b90de4953 100644
> --- a/drivers/remoteproc/st_remoteproc.c
> +++ b/drivers/remoteproc/st_remoteproc.c
> @@ -120,40 +120,35 @@ static int st_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
>  	struct device *dev = rproc->dev.parent;
>  	struct device_node *np = dev->of_node;
>  	struct rproc_mem_entry *mem;
> -	struct reserved_mem *rmem;
> -	struct of_phandle_iterator it;
> -	int index = 0;
> -
> -	of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
> -	while (of_phandle_iterator_next(&it) == 0) {
> -		rmem = of_reserved_mem_lookup(it.node);
> -		if (!rmem) {
> -			of_node_put(it.node);
> -			dev_err(dev, "unable to acquire memory-region\n");
> -			return -EINVAL;
> -		}
> +	int index = 0, mr = 0;
> +
> +	while (1) {
> +		struct resource res;
> +		int ret;
> +
> +		ret = of_reserved_mem_region_to_resource(np, mr++, &res);
> +		if (ret)
> +			return 0;
> 
>  		/*  No need to map vdev buffer */
> -		if (strcmp(it.node->name, "vdev0buffer")) {
> +		if (strcmp(res.name, "vdev0buffer")) {
>  			/* Register memory region */
>  			mem = rproc_mem_entry_init(dev, NULL,
> -						   (dma_addr_t)rmem->base,
> -						   rmem->size, rmem->base,
> +						   (dma_addr_t)res.start,
> +						   resource_size(&res), res.start,
>  						   st_rproc_mem_alloc,
>  						   st_rproc_mem_release,
> -						   it.node->name);
> +						   res.name);
>  		} else {
>  			/* Register reserved memory for vdev buffer allocation */
>  			mem = rproc_of_resm_mem_entry_init(dev, index,
> -							   rmem->size,
> -							   rmem->base,
> -							   it.node->name);
> +							   resource_size(&res),
> +							   res.start,
> +							   res.name);
>  		}
> 
> -		if (!mem) {
> -			of_node_put(it.node);
> +		if (!mem)
>  			return -ENOMEM;
> -		}
> 
>  		rproc_add_carveout(rproc, mem);
>  		index++;
> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
> index b02b36a3f515..9d2bd8904c49 100644
> --- a/drivers/remoteproc/stm32_rproc.c
> +++ b/drivers/remoteproc/stm32_rproc.c
> @@ -213,52 +213,46 @@ static int stm32_rproc_prepare(struct rproc *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;
>  	u64 da;
> -	int index = 0;
> +	int index = 0, mr = 0;
> 
>  	/* Register associated reserved memory regions */
> -	of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
> -	while (of_phandle_iterator_next(&it) == 0) {
> -		rmem = of_reserved_mem_lookup(it.node);
> -		if (!rmem) {
> -			of_node_put(it.node);
> -			dev_err(dev, "unable to acquire memory-region\n");
> -			return -EINVAL;
> -		}
> +	while (1) {
> +		struct resource res;
> +		int ret;
> +
> +		ret = of_reserved_mem_region_to_resource(np, mr++, &res);
> +		if (ret)
> +			return 0;
> 
> -		if (stm32_rproc_pa_to_da(rproc, rmem->base, &da) < 0) {
> -			of_node_put(it.node);
> -			dev_err(dev, "memory region not valid %pa\n",
> -				&rmem->base);
> +		if (stm32_rproc_pa_to_da(rproc, res.start, &da) < 0) {
> +			dev_err(dev, "memory region not valid %pR\n", &res);
>  			return -EINVAL;
>  		}
> 
>  		/*  No need to map vdev buffer */
> -		if (strcmp(it.node->name, "vdev0buffer")) {
> +		if (strcmp(res.name, "vdev0buffer")) {

I tested your patches

The update introduces a regression here. The strcmp function never returns 0.
Indeed, it.node->name stores the memory region label "vdev0buffer," while
res.name stores the memory region name "vdev0buffer@10042000."

Several remoteproc drivers may face the same issue as they embed similar code.

Regards,
Arnaud

>  			/* Register memory region */
>  			mem = rproc_mem_entry_init(dev, NULL,
> -						   (dma_addr_t)rmem->base,
> -						   rmem->size, da,
> +						   (dma_addr_t)res.start,
> +						   resource_size(&res), da,
>  						   stm32_rproc_mem_alloc,
>  						   stm32_rproc_mem_release,
> -						   it.node->name);
> +						   res.name);
> 
>  			if (mem)
>  				rproc_coredump_add_segment(rproc, da,
> -							   rmem->size);
> +							   resource_size(&res));
>  		} else {
>  			/* Register reserved memory for vdev buffer alloc */
>  			mem = rproc_of_resm_mem_entry_init(dev, index,
> -							   rmem->size,
> -							   rmem->base,
> -							   it.node->name);
> +							   resource_size(&res),
> +							   res.start,
> +							   res.name);
>  		}
> 
>  		if (!mem) {
> -			of_node_put(it.node);
>  			return -ENOMEM;
>  		}
> 
> diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> index a695890254ff..f02c835535bc 100644
> --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> @@ -535,13 +535,10 @@ static int k3_dsp_reserved_mem_init(struct k3_dsp_rproc *kproc)
>  {
>  	struct device *dev = kproc->dev;
>  	struct device_node *np = dev->of_node;
> -	struct device_node *rmem_np;
> -	struct reserved_mem *rmem;
>  	int num_rmems;
>  	int ret, i;
> 
> -	num_rmems = of_property_count_elems_of_size(np, "memory-region",
> -						    sizeof(phandle));
> +	num_rmems = of_reserved_mem_region_count(np);
>  	if (num_rmems < 0) {
>  		dev_err(dev, "device does not reserved memory regions (%pe)\n",
>  			ERR_PTR(num_rmems));
> @@ -571,23 +568,20 @@ static int k3_dsp_reserved_mem_init(struct k3_dsp_rproc *kproc)
> 
>  	/* use remaining reserved memory regions for static carveouts */
>  	for (i = 0; i < num_rmems; i++) {
> -		rmem_np = of_parse_phandle(np, "memory-region", i + 1);
> -		if (!rmem_np)
> -			return -EINVAL;
> +		struct resource res;
> 
> -		rmem = of_reserved_mem_lookup(rmem_np);
> -		of_node_put(rmem_np);
> -		if (!rmem)
> -			return -EINVAL;
> +		ret = of_reserved_mem_region_to_resource(np, i + 1, &res);
> +		if (ret)
> +			return ret;
> 
> -		kproc->rmem[i].bus_addr = rmem->base;
> +		kproc->rmem[i].bus_addr = res.start;
>  		/* 64-bit address regions currently not supported */
> -		kproc->rmem[i].dev_addr = (u32)rmem->base;
> -		kproc->rmem[i].size = rmem->size;
> -		kproc->rmem[i].cpu_addr = devm_ioremap_wc(dev, rmem->base, rmem->size);
> +		kproc->rmem[i].dev_addr = (u32)res.start;
> +		kproc->rmem[i].size = resource_size(&res);
> +		kproc->rmem[i].cpu_addr = devm_ioremap_resource_wc(dev, &res);
>  		if (!kproc->rmem[i].cpu_addr) {
> -			dev_err(dev, "failed to map reserved memory#%d at %pa of size %pa\n",
> -				i + 1, &rmem->base, &rmem->size);
> +			dev_err(dev, "failed to map reserved memory#%d at %pR\n",
> +				i + 1, &res);
>  			return -ENOMEM;
>  		}
> 
> diff --git a/drivers/remoteproc/ti_k3_m4_remoteproc.c b/drivers/remoteproc/ti_k3_m4_remoteproc.c
> index a16fb165fced..8c7772cd6baf 100644
> --- a/drivers/remoteproc/ti_k3_m4_remoteproc.c
> +++ b/drivers/remoteproc/ti_k3_m4_remoteproc.c
> @@ -393,13 +393,10 @@ static int k3_m4_reserved_mem_init(struct k3_m4_rproc *kproc)
>  {
>  	struct device *dev = kproc->dev;
>  	struct device_node *np = dev->of_node;
> -	struct device_node *rmem_np;
> -	struct reserved_mem *rmem;
>  	int num_rmems;
>  	int ret, i;
> 
> -	num_rmems = of_property_count_elems_of_size(np, "memory-region",
> -						    sizeof(phandle));
> +	num_rmems = of_reserved_mem_region_count(np);
>  	if (num_rmems < 0) {
>  		dev_err(dev, "device does not reserved memory regions (%d)\n",
>  			num_rmems);
> @@ -428,23 +425,20 @@ static int k3_m4_reserved_mem_init(struct k3_m4_rproc *kproc)
> 
>  	/* use remaining reserved memory regions for static carveouts */
>  	for (i = 0; i < num_rmems; i++) {
> -		rmem_np = of_parse_phandle(np, "memory-region", i + 1);
> -		if (!rmem_np)
> -			return -EINVAL;
> +		struct resource res;
> 
> -		rmem = of_reserved_mem_lookup(rmem_np);
> -		of_node_put(rmem_np);
> -		if (!rmem)
> -			return -EINVAL;
> +		ret = of_reserved_mem_region_to_resource(np, i + 1, &res);
> +		if (ret)
> +			return ret;
> 
> -		kproc->rmem[i].bus_addr = rmem->base;
> +		kproc->rmem[i].bus_addr = res.start;
>  		/* 64-bit address regions currently not supported */
> -		kproc->rmem[i].dev_addr = (u32)rmem->base;
> -		kproc->rmem[i].size = rmem->size;
> -		kproc->rmem[i].cpu_addr = devm_ioremap_wc(dev, rmem->base, rmem->size);
> +		kproc->rmem[i].dev_addr = (u32)res.start;
> +		kproc->rmem[i].size = resource_size(&res);
> +		kproc->rmem[i].cpu_addr = devm_ioremap_resource_wc(dev, &res);
>  		if (!kproc->rmem[i].cpu_addr) {
> -			dev_err(dev, "failed to map reserved memory#%d at %pa of size %pa\n",
> -				i + 1, &rmem->base, &rmem->size);
> +			dev_err(dev, "failed to map reserved memory#%d at %pR\n",
> +				i + 1, &res);
>  			return -ENOMEM;
>  		}
> 
> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> index dbc513c5569c..9a7a61e0ecb8 100644
> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> @@ -966,13 +966,10 @@ static int k3_r5_reserved_mem_init(struct k3_r5_rproc *kproc)
>  {
>  	struct device *dev = kproc->dev;
>  	struct device_node *np = dev_of_node(dev);
> -	struct device_node *rmem_np;
> -	struct reserved_mem *rmem;
>  	int num_rmems;
>  	int ret, i;
> 
> -	num_rmems = of_property_count_elems_of_size(np, "memory-region",
> -						    sizeof(phandle));
> +	num_rmems = of_reserved_mem_region_count(np);
>  	if (num_rmems <= 0) {
>  		dev_err(dev, "device does not have reserved memory regions, ret = %d\n",
>  			num_rmems);
> @@ -1003,16 +1000,13 @@ static int k3_r5_reserved_mem_init(struct k3_r5_rproc *kproc)
> 
>  	/* use remaining reserved memory regions for static carveouts */
>  	for (i = 0; i < num_rmems; i++) {
> -		rmem_np = of_parse_phandle(np, "memory-region", i + 1);
> -		if (!rmem_np)
> -			return -EINVAL;
> +		struct resource res;
> 
> -		rmem = of_reserved_mem_lookup(rmem_np);
> -		of_node_put(rmem_np);
> -		if (!rmem)
> -			return -EINVAL;
> +		ret = of_reserved_mem_region_to_resource(np, i + 1, &res);
> +		if (ret)
> +			return ret;
> 
> -		kproc->rmem[i].bus_addr = rmem->base;
> +		kproc->rmem[i].bus_addr = res.start;
>  		/*
>  		 * R5Fs do not have an MMU, but have a Region Address Translator
>  		 * (RAT) module that provides a fixed entry translation between
> @@ -1023,12 +1017,12 @@ static int k3_r5_reserved_mem_init(struct k3_r5_rproc *kproc)
>  		 * addresses/supported memory regions are restricted to 32-bit
>  		 * bus addresses, and are identical
>  		 */
> -		kproc->rmem[i].dev_addr = (u32)rmem->base;
> -		kproc->rmem[i].size = rmem->size;
> -		kproc->rmem[i].cpu_addr = devm_ioremap_wc(dev, rmem->base, rmem->size);
> +		kproc->rmem[i].dev_addr = (u32)res.start;
> +		kproc->rmem[i].size = resource_size(&res);
> +		kproc->rmem[i].cpu_addr = devm_ioremap_resource_wc(dev, &res);
>  		if (!kproc->rmem[i].cpu_addr) {
> -			dev_err(dev, "failed to map reserved memory#%d at %pa of size %pa\n",
> -				i + 1, &rmem->base, &rmem->size);
> +			dev_err(dev, "failed to map reserved memory#%d at %pR\n",
> +				i + 1, &res);
>  			return -ENOMEM;
>  		}
> 
> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
> index 5aeedeaf3c41..000eeadd5a01 100644
> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
> @@ -460,49 +460,42 @@ 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);
> +		if (err)
> +			return 0;
> 
> -		if (!strcmp(it.node->name, "vdev0buffer")) {
> +		if (!strcmp(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,
> +								 res.name);
>  		} 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);
> +							 res.name);
>  		}
> 
> -		if (!rproc_mem) {
> -			of_node_put(it.node);
> +		if (!rproc_mem)
>  			return -ENOMEM;
> -		}
> 
>  		rproc_add_carveout(rproc, rproc_mem);
> -		rproc_coredump_add_segment(rproc, rmem->base, rmem->size);
> +		rproc_coredump_add_segment(rproc, res.start, resource_size(&res));
> 
> -		dev_dbg(&rproc->dev, "reserved mem carveout %s addr=%llx, size=0x%llx",
> -			it.node->name, rmem->base, rmem->size);
> +		dev_dbg(&rproc->dev, "reserved mem carveout %pR\n", &res);
>  		i++;
>  	}
> 
> @@ -776,7 +769,6 @@ static int zynqmp_r5_get_rsc_table_va(struct zynqmp_r5_core *r5_core)
>  	struct device *dev = r5_core->dev;
>  	struct rsc_tbl_data *rsc_data_va;
>  	struct resource res_mem;
> -	struct device_node *np;
>  	int ret;
> 
>  	/*
> @@ -786,14 +778,7 @@ static int zynqmp_r5_get_rsc_table_va(struct zynqmp_r5_core *r5_core)
>  	 * contains that data structure which holds resource table address, size
>  	 * and some magic number to validate correct resource table entry.
>  	 */
> -	np = of_parse_phandle(r5_core->np, "memory-region", 0);
> -	if (!np) {
> -		dev_err(dev, "failed to get memory region dev node\n");
> -		return -EINVAL;
> -	}
> -
> -	ret = of_address_to_resource(np, 0, &res_mem);
> -	of_node_put(np);
> +	ret = of_reserved_mem_region_to_resource(r5_core->np, 0, &res_mem);
>  	if (ret) {
>  		dev_err(dev, "failed to get memory-region resource addr\n");
>  		return -EINVAL;
> --
> 2.47.2
> 
> _______________________________________________
> Linux-stm32 mailing list
> Linux-stm32@st-md-mailman.stormreply.com
> https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32

Re: [Linux-stm32] [PATCH 3/3] remoteproc: Use of_reserved_mem_region_* functions for "memory-region"

[Rob Herring]

On Wed, Mar 19, 2025 at 10:26 AM Arnaud POULIQUEN
<arnaud.pouliquen@foss.st.com> wrote:
>
> Hello Rob,
>
> On 3/18/25 00:24, 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) <robh@kernel.org>
> > ---
> > For v6.16
> >

[...]

> > diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
> > index b02b36a3f515..9d2bd8904c49 100644
> > --- a/drivers/remoteproc/stm32_rproc.c
> > +++ b/drivers/remoteproc/stm32_rproc.c
> > @@ -213,52 +213,46 @@ static int stm32_rproc_prepare(struct rproc *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;
> >       u64 da;
> > -     int index = 0;
> > +     int index = 0, mr = 0;
> >
> >       /* Register associated reserved memory regions */
> > -     of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
> > -     while (of_phandle_iterator_next(&it) == 0) {
> > -             rmem = of_reserved_mem_lookup(it.node);
> > -             if (!rmem) {
> > -                     of_node_put(it.node);
> > -                     dev_err(dev, "unable to acquire memory-region\n");
> > -                     return -EINVAL;
> > -             }
> > +     while (1) {
> > +             struct resource res;
> > +             int ret;
> > +
> > +             ret = of_reserved_mem_region_to_resource(np, mr++, &res);
> > +             if (ret)
> > +                     return 0;
> >
> > -             if (stm32_rproc_pa_to_da(rproc, rmem->base, &da) < 0) {
> > -                     of_node_put(it.node);
> > -                     dev_err(dev, "memory region not valid %pa\n",
> > -                             &rmem->base);
> > +             if (stm32_rproc_pa_to_da(rproc, res.start, &da) < 0) {
> > +                     dev_err(dev, "memory region not valid %pR\n", &res);
> >                       return -EINVAL;
> >               }
> >
> >               /*  No need to map vdev buffer */
> > -             if (strcmp(it.node->name, "vdev0buffer")) {
> > +             if (strcmp(res.name, "vdev0buffer")) {
>
> I tested your patches

Thank you.

> The update introduces a regression here. The strcmp function never returns 0.
> Indeed, it.node->name stores the memory region label "vdev0buffer," while
> res.name stores the memory region name "vdev0buffer@10042000."
>
> Several remoteproc drivers may face the same issue as they embed similar code.

Indeed. I confused myself because node 'name' is without the
unit-address, but this is using the full name. I've replaced the
strcmp's with strstarts() to address this. I've updated my branch with
the changes.

Rob

Re: [Linux-stm32] [PATCH 3/3] remoteproc: Use of_reserved_mem_region_* functions for "memory-region"

[Arnaud POULIQUEN]

On 3/20/25 00:04, Rob Herring wrote:
> On Wed, Mar 19, 2025 at 10:26 AM Arnaud POULIQUEN
> <arnaud.pouliquen@foss.st.com> wrote:
>>
>> Hello Rob,
>>
>> On 3/18/25 00:24, 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) <robh@kernel.org>
>>> ---
>>> For v6.16
>>>
> 
> [...]
> 
>>> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
>>> index b02b36a3f515..9d2bd8904c49 100644
>>> --- a/drivers/remoteproc/stm32_rproc.c
>>> +++ b/drivers/remoteproc/stm32_rproc.c
>>> @@ -213,52 +213,46 @@ static int stm32_rproc_prepare(struct rproc *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;
>>>       u64 da;
>>> -     int index = 0;
>>> +     int index = 0, mr = 0;
>>>
>>>       /* Register associated reserved memory regions */
>>> -     of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
>>> -     while (of_phandle_iterator_next(&it) == 0) {
>>> -             rmem = of_reserved_mem_lookup(it.node);
>>> -             if (!rmem) {
>>> -                     of_node_put(it.node);
>>> -                     dev_err(dev, "unable to acquire memory-region\n");
>>> -                     return -EINVAL;
>>> -             }
>>> +     while (1) {
>>> +             struct resource res;
>>> +             int ret;
>>> +
>>> +             ret = of_reserved_mem_region_to_resource(np, mr++, &res);
>>> +             if (ret)
>>> +                     return 0;
>>>
>>> -             if (stm32_rproc_pa_to_da(rproc, rmem->base, &da) < 0) {
>>> -                     of_node_put(it.node);
>>> -                     dev_err(dev, "memory region not valid %pa\n",
>>> -                             &rmem->base);
>>> +             if (stm32_rproc_pa_to_da(rproc, res.start, &da) < 0) {
>>> +                     dev_err(dev, "memory region not valid %pR\n", &res);
>>>                       return -EINVAL;
>>>               }
>>>
>>>               /*  No need to map vdev buffer */
>>> -             if (strcmp(it.node->name, "vdev0buffer")) {
>>> +             if (strcmp(res.name, "vdev0buffer")) {
>>
>> I tested your patches
> 
> Thank you.
> 
>> The update introduces a regression here. The strcmp function never returns 0.
>> Indeed, it.node->name stores the memory region label "vdev0buffer," while
>> res.name stores the memory region name "vdev0buffer@10042000."
>>
>> Several remoteproc drivers may face the same issue as they embed similar code.
> 
> Indeed. I confused myself because node 'name' is without the
> unit-address, but this is using the full name. I've replaced the
> strcmp's with strstarts() to address this. I've updated my branch with
> the changes.

This is not enough as the remoteproc core function rproc_find_carveout_by_name()
also compares the memory names. With the following additional fix, it is working
on my STM32MP15-DK board.

@@ -309,11 +309,11 @@ rproc_find_carveout_by_name(struct rproc *rproc, const
char *name, ...)
 	vsnprintf(_name, sizeof(_name), name, args);
 	va_end(args);

 	list_for_each_entry(carveout, &rproc->carveouts, node) {
 		/* Compare carveout and requested names */
-		if (!strcmp(carveout->name, _name)) {
+		if (strstarts(carveout->name, _name)) {
 			mem = carveout;
 			break;
 		}
 	}

I just wonder if would not be more suitable to address this using the
"memory-region-names" field.

The drawback is that we would break compatibility with legacy boards...

I let Mathieu and Bjorn review and comment


Else with the fix in rproc_find_carveout_by_name(),

-for the stm32_rproc:
reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
tested-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>

- for the st_remoteproc
reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>

Thanks,
Arnaud


> 
> Rob

Re: [Linux-stm32] [PATCH 3/3] remoteproc: Use of_reserved_mem_region_* functions for "memory-region"

[Arnaud POULIQUEN]

On 3/20/25 10:21, Arnaud POULIQUEN wrote:
> 
> 
> On 3/20/25 00:04, Rob Herring wrote:
>> On Wed, Mar 19, 2025 at 10:26 AM Arnaud POULIQUEN
>> <arnaud.pouliquen@foss.st.com> wrote:
>>>
>>> Hello Rob,
>>>
>>> On 3/18/25 00:24, 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) <robh@kernel.org>
>>>> ---
>>>> For v6.16
>>>>
>>
>> [...]
>>
>>>> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
>>>> index b02b36a3f515..9d2bd8904c49 100644
>>>> --- a/drivers/remoteproc/stm32_rproc.c
>>>> +++ b/drivers/remoteproc/stm32_rproc.c
>>>> @@ -213,52 +213,46 @@ static int stm32_rproc_prepare(struct rproc *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;
>>>>       u64 da;
>>>> -     int index = 0;
>>>> +     int index = 0, mr = 0;
>>>>
>>>>       /* Register associated reserved memory regions */
>>>> -     of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
>>>> -     while (of_phandle_iterator_next(&it) == 0) {
>>>> -             rmem = of_reserved_mem_lookup(it.node);
>>>> -             if (!rmem) {
>>>> -                     of_node_put(it.node);
>>>> -                     dev_err(dev, "unable to acquire memory-region\n");
>>>> -                     return -EINVAL;
>>>> -             }
>>>> +     while (1) {
>>>> +             struct resource res;
>>>> +             int ret;
>>>> +
>>>> +             ret = of_reserved_mem_region_to_resource(np, mr++, &res);
>>>> +             if (ret)
>>>> +                     return 0;
>>>>
>>>> -             if (stm32_rproc_pa_to_da(rproc, rmem->base, &da) < 0) {
>>>> -                     of_node_put(it.node);
>>>> -                     dev_err(dev, "memory region not valid %pa\n",
>>>> -                             &rmem->base);
>>>> +             if (stm32_rproc_pa_to_da(rproc, res.start, &da) < 0) {
>>>> +                     dev_err(dev, "memory region not valid %pR\n", &res);
>>>>                       return -EINVAL;
>>>>               }
>>>>
>>>>               /*  No need to map vdev buffer */
>>>> -             if (strcmp(it.node->name, "vdev0buffer")) {
>>>> +             if (strcmp(res.name, "vdev0buffer")) {
>>>
>>> I tested your patches
>>
>> Thank you.
>>
>>> The update introduces a regression here. The strcmp function never returns 0.
>>> Indeed, it.node->name stores the memory region label "vdev0buffer," while
>>> res.name stores the memory region name "vdev0buffer@10042000."
>>>
>>> Several remoteproc drivers may face the same issue as they embed similar code.
>>
>> Indeed. I confused myself because node 'name' is without the
>> unit-address, but this is using the full name. I've replaced the
>> strcmp's with strstarts() to address this. I've updated my branch with
>> the changes.
> 
> This is not enough as the remoteproc core function rproc_find_carveout_by_name()
> also compares the memory names. With the following additional fix, it is working
> on my STM32MP15-DK board.
> 
> @@ -309,11 +309,11 @@ rproc_find_carveout_by_name(struct rproc *rproc, const
> char *name, ...)
>  	vsnprintf(_name, sizeof(_name), name, args);
>  	va_end(args);
> 
>  	list_for_each_entry(carveout, &rproc->carveouts, node) {
>  		/* Compare carveout and requested names */
> -		if (!strcmp(carveout->name, _name)) {
> +		if (strstarts(carveout->name, _name)) {
>  			mem = carveout;
>  			break;
>  		}
>  	}
> 
> I just wonder if would not be more suitable to address this using the
> "memory-region-names" field.
> 
> The drawback is that we would break compatibility with legacy boards...

Errata:
The drawback is that we would break compatibility with legacy DTs...

> 
> I let Mathieu and Bjorn review and comment
> 
> 
> Else with the fix in rproc_find_carveout_by_name(),
> 
> -for the stm32_rproc:
> reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> tested-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> 
> - for the st_remoteproc
> reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> 
> Thanks,
> Arnaud
> 
> 
>>
>> Rob
> _______________________________________________
> Linux-stm32 mailing list
> Linux-stm32@st-md-mailman.stormreply.com
> https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32

Re: [Linux-stm32] [PATCH 3/3] remoteproc: Use of_reserved_mem_region_* functions for "memory-region"

[Rob Herring]

On Thu, Mar 20, 2025 at 4:23 AM Arnaud POULIQUEN
<arnaud.pouliquen@foss.st.com> wrote:
>
>
>
> On 3/20/25 00:04, Rob Herring wrote:
> > On Wed, Mar 19, 2025 at 10:26 AM Arnaud POULIQUEN
> > <arnaud.pouliquen@foss.st.com> wrote:
> >>
> >> Hello Rob,
> >>
> >> On 3/18/25 00:24, 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) <robh@kernel.org>
> >>> ---
> >>> For v6.16
> >>>
> >
> > [...]
> >
> >>> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
> >>> index b02b36a3f515..9d2bd8904c49 100644
> >>> --- a/drivers/remoteproc/stm32_rproc.c
> >>> +++ b/drivers/remoteproc/stm32_rproc.c
> >>> @@ -213,52 +213,46 @@ static int stm32_rproc_prepare(struct rproc *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;
> >>>       u64 da;
> >>> -     int index = 0;
> >>> +     int index = 0, mr = 0;
> >>>
> >>>       /* Register associated reserved memory regions */
> >>> -     of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
> >>> -     while (of_phandle_iterator_next(&it) == 0) {
> >>> -             rmem = of_reserved_mem_lookup(it.node);
> >>> -             if (!rmem) {
> >>> -                     of_node_put(it.node);
> >>> -                     dev_err(dev, "unable to acquire memory-region\n");
> >>> -                     return -EINVAL;
> >>> -             }
> >>> +     while (1) {
> >>> +             struct resource res;
> >>> +             int ret;
> >>> +
> >>> +             ret = of_reserved_mem_region_to_resource(np, mr++, &res);
> >>> +             if (ret)
> >>> +                     return 0;
> >>>
> >>> -             if (stm32_rproc_pa_to_da(rproc, rmem->base, &da) < 0) {
> >>> -                     of_node_put(it.node);
> >>> -                     dev_err(dev, "memory region not valid %pa\n",
> >>> -                             &rmem->base);
> >>> +             if (stm32_rproc_pa_to_da(rproc, res.start, &da) < 0) {
> >>> +                     dev_err(dev, "memory region not valid %pR\n", &res);
> >>>                       return -EINVAL;
> >>>               }
> >>>
> >>>               /*  No need to map vdev buffer */
> >>> -             if (strcmp(it.node->name, "vdev0buffer")) {
> >>> +             if (strcmp(res.name, "vdev0buffer")) {
> >>
> >> I tested your patches
> >
> > Thank you.
> >
> >> The update introduces a regression here. The strcmp function never returns 0.
> >> Indeed, it.node->name stores the memory region label "vdev0buffer," while
> >> res.name stores the memory region name "vdev0buffer@10042000."
> >>
> >> Several remoteproc drivers may face the same issue as they embed similar code.
> >
> > Indeed. I confused myself because node 'name' is without the
> > unit-address, but this is using the full name. I've replaced the
> > strcmp's with strstarts() to address this. I've updated my branch with
> > the changes.
>
> This is not enough as the remoteproc core function rproc_find_carveout_by_name()
> also compares the memory names. With the following additional fix, it is working
> on my STM32MP15-DK board.
>
> @@ -309,11 +309,11 @@ rproc_find_carveout_by_name(struct rproc *rproc, const
> char *name, ...)
>         vsnprintf(_name, sizeof(_name), name, args);
>         va_end(args);
>
>         list_for_each_entry(carveout, &rproc->carveouts, node) {
>                 /* Compare carveout and requested names */
> -               if (!strcmp(carveout->name, _name)) {
> +               if (strstarts(carveout->name, _name)) {
>                         mem = carveout;
>                         break;
>                 }
>         }
>
> I just wonder if would not be more suitable to address this using the
> "memory-region-names" field.

That would be better as you shouldn't really care what a provider node
name is where-as "memory-region-names" is meaningful to the driver.

>
> The drawback is that we would break compatibility with legacy boards...

So not an option.

I think I'll have to fix this within the reserved mem code storing the
name or do something like the diff below. I'd like to avoid the
former. Using the original device_node.name is also problematic
because I want to get rid of it. We redundantly store the node name
with and without the unit-address. There's a lot of places like this
one where we hand out the pointer with no lifetime.

diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
index 1e949694d365..cdee87c6ffe0 100644
--- a/drivers/remoteproc/stm32_rproc.c
+++ b/drivers/remoteproc/stm32_rproc.c
@@ -239,7 +239,7 @@ static int stm32_rproc_prepare(struct rproc *rproc)
                                                   resource_size(&res), da,
                                                   stm32_rproc_mem_alloc,
                                                   stm32_rproc_mem_release,
-                                                  res.name);
+                                                  "%.*s",
strchrnul(res.name, '@') - res.name, res.name);

                        if (mem)
                                rproc_coredump_add_segment(rproc, da,
@@ -249,7 +249,7 @@ static int stm32_rproc_prepare(struct rproc *rproc)
                        mem = rproc_of_resm_mem_entry_init(dev, index,
                                                           resource_size(&res),
                                                           res.start,
-                                                          res.name);
+                                                          "vdev0buffer");
                }

                if (!mem) {

Re: [Linux-stm32] [PATCH 3/3] remoteproc: Use of_reserved_mem_region_* functions for "memory-region"

[Arnaud POULIQUEN]

On 3/20/25 19:02, Rob Herring wrote:
> On Thu, Mar 20, 2025 at 4:23 AM Arnaud POULIQUEN
> <arnaud.pouliquen@foss.st.com> wrote:
>>
>>
>>
>> On 3/20/25 00:04, Rob Herring wrote:
>>> On Wed, Mar 19, 2025 at 10:26 AM Arnaud POULIQUEN
>>> <arnaud.pouliquen@foss.st.com> wrote:
>>>>
>>>> Hello Rob,
>>>>
>>>> On 3/18/25 00:24, 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) <robh@kernel.org>
>>>>> ---
>>>>> For v6.16
>>>>>
>>>
>>> [...]
>>>
>>>>> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
>>>>> index b02b36a3f515..9d2bd8904c49 100644
>>>>> --- a/drivers/remoteproc/stm32_rproc.c
>>>>> +++ b/drivers/remoteproc/stm32_rproc.c
>>>>> @@ -213,52 +213,46 @@ static int stm32_rproc_prepare(struct rproc *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;
>>>>>       u64 da;
>>>>> -     int index = 0;
>>>>> +     int index = 0, mr = 0;
>>>>>
>>>>>       /* Register associated reserved memory regions */
>>>>> -     of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
>>>>> -     while (of_phandle_iterator_next(&it) == 0) {
>>>>> -             rmem = of_reserved_mem_lookup(it.node);
>>>>> -             if (!rmem) {
>>>>> -                     of_node_put(it.node);
>>>>> -                     dev_err(dev, "unable to acquire memory-region\n");
>>>>> -                     return -EINVAL;
>>>>> -             }
>>>>> +     while (1) {
>>>>> +             struct resource res;
>>>>> +             int ret;
>>>>> +
>>>>> +             ret = of_reserved_mem_region_to_resource(np, mr++, &res);
>>>>> +             if (ret)
>>>>> +                     return 0;
>>>>>
>>>>> -             if (stm32_rproc_pa_to_da(rproc, rmem->base, &da) < 0) {
>>>>> -                     of_node_put(it.node);
>>>>> -                     dev_err(dev, "memory region not valid %pa\n",
>>>>> -                             &rmem->base);
>>>>> +             if (stm32_rproc_pa_to_da(rproc, res.start, &da) < 0) {
>>>>> +                     dev_err(dev, "memory region not valid %pR\n", &res);
>>>>>                       return -EINVAL;
>>>>>               }
>>>>>
>>>>>               /*  No need to map vdev buffer */
>>>>> -             if (strcmp(it.node->name, "vdev0buffer")) {
>>>>> +             if (strcmp(res.name, "vdev0buffer")) {
>>>>
>>>> I tested your patches
>>>
>>> Thank you.
>>>
>>>> The update introduces a regression here. The strcmp function never returns 0.
>>>> Indeed, it.node->name stores the memory region label "vdev0buffer," while
>>>> res.name stores the memory region name "vdev0buffer@10042000."
>>>>
>>>> Several remoteproc drivers may face the same issue as they embed similar code.
>>>
>>> Indeed. I confused myself because node 'name' is without the
>>> unit-address, but this is using the full name. I've replaced the
>>> strcmp's with strstarts() to address this. I've updated my branch with
>>> the changes.
>>
>> This is not enough as the remoteproc core function rproc_find_carveout_by_name()
>> also compares the memory names. With the following additional fix, it is working
>> on my STM32MP15-DK board.
>>
>> @@ -309,11 +309,11 @@ rproc_find_carveout_by_name(struct rproc *rproc, const
>> char *name, ...)
>>         vsnprintf(_name, sizeof(_name), name, args);
>>         va_end(args);
>>
>>         list_for_each_entry(carveout, &rproc->carveouts, node) {
>>                 /* Compare carveout and requested names */
>> -               if (!strcmp(carveout->name, _name)) {
>> +               if (strstarts(carveout->name, _name)) {
>>                         mem = carveout;
>>                         break;
>>                 }
>>         }
>>
>> I just wonder if would not be more suitable to address this using the
>> "memory-region-names" field.
> 
> That would be better as you shouldn't really care what a provider node
> name is where-as "memory-region-names" is meaningful to the driver.
> 
>>
>> The drawback is that we would break compatibility with legacy boards...
> 
> So not an option.

> 
> I think I'll have to fix this within the reserved mem code storing the
> name or do something like the diff below. I'd like to avoid the
> former. Using the original device_node.name is also problematic
> because I want to get rid of it. We redundantly store the node name
> with and without the unit-address. There's a lot of places like this
> one where we hand out the pointer with no lifetime.
> 
> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
> index 1e949694d365..cdee87c6ffe0 100644
> --- a/drivers/remoteproc/stm32_rproc.c
> +++ b/drivers/remoteproc/stm32_rproc.c
> @@ -239,7 +239,7 @@ static int stm32_rproc_prepare(struct rproc *rproc)
>                                                    resource_size(&res), da,
>                                                    stm32_rproc_mem_alloc,
>                                                    stm32_rproc_mem_release,
> -                                                  res.name);
> +                                                  "%.*s",
> strchrnul(res.name, '@') - res.name, res.name);
> 
>                         if (mem)
>                                 rproc_coredump_add_segment(rproc, da,
> @@ -249,7 +249,7 @@ static int stm32_rproc_prepare(struct rproc *rproc)
>                         mem = rproc_of_resm_mem_entry_init(dev, index,
>                                                            resource_size(&res),
>                                                            res.start,
> -                                                          res.name);
> +                                                          "vdev0buffer");
>                 }
> 
>                 if (!mem) {


That's work on my side.
Could we have an OF helper to retrieve the name from the full name?

Thanks,
Arnaud

Re: [Linux-stm32] [PATCH 3/3] remoteproc: Use of_reserved_mem_region_* functions for "memory-region"

[Rob Herring]

On Fri, Mar 21, 2025 at 3:25 AM Arnaud POULIQUEN
<arnaud.pouliquen@foss.st.com> wrote:
>
>
>
> On 3/20/25 19:02, Rob Herring wrote:
> > On Thu, Mar 20, 2025 at 4:23 AM Arnaud POULIQUEN
> > <arnaud.pouliquen@foss.st.com> wrote:
> >>
> >>
> >>
> >> On 3/20/25 00:04, Rob Herring wrote:
> >>> On Wed, Mar 19, 2025 at 10:26 AM Arnaud POULIQUEN
> >>> <arnaud.pouliquen@foss.st.com> wrote:
> >>>>
> >>>> Hello Rob,
> >>>>
> >>>> On 3/18/25 00:24, 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) <robh@kernel.org>
> >>>>> ---
> >>>>> For v6.16
> >>>>>
> >>>
> >>> [...]
> >>>
> >>>>> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
> >>>>> index b02b36a3f515..9d2bd8904c49 100644
> >>>>> --- a/drivers/remoteproc/stm32_rproc.c
> >>>>> +++ b/drivers/remoteproc/stm32_rproc.c
> >>>>> @@ -213,52 +213,46 @@ static int stm32_rproc_prepare(struct rproc *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;
> >>>>>       u64 da;
> >>>>> -     int index = 0;
> >>>>> +     int index = 0, mr = 0;
> >>>>>
> >>>>>       /* Register associated reserved memory regions */
> >>>>> -     of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
> >>>>> -     while (of_phandle_iterator_next(&it) == 0) {
> >>>>> -             rmem = of_reserved_mem_lookup(it.node);
> >>>>> -             if (!rmem) {
> >>>>> -                     of_node_put(it.node);
> >>>>> -                     dev_err(dev, "unable to acquire memory-region\n");
> >>>>> -                     return -EINVAL;
> >>>>> -             }
> >>>>> +     while (1) {
> >>>>> +             struct resource res;
> >>>>> +             int ret;
> >>>>> +
> >>>>> +             ret = of_reserved_mem_region_to_resource(np, mr++, &res);
> >>>>> +             if (ret)
> >>>>> +                     return 0;
> >>>>>
> >>>>> -             if (stm32_rproc_pa_to_da(rproc, rmem->base, &da) < 0) {
> >>>>> -                     of_node_put(it.node);
> >>>>> -                     dev_err(dev, "memory region not valid %pa\n",
> >>>>> -                             &rmem->base);
> >>>>> +             if (stm32_rproc_pa_to_da(rproc, res.start, &da) < 0) {
> >>>>> +                     dev_err(dev, "memory region not valid %pR\n", &res);
> >>>>>                       return -EINVAL;
> >>>>>               }
> >>>>>
> >>>>>               /*  No need to map vdev buffer */
> >>>>> -             if (strcmp(it.node->name, "vdev0buffer")) {
> >>>>> +             if (strcmp(res.name, "vdev0buffer")) {
> >>>>
> >>>> I tested your patches
> >>>
> >>> Thank you.
> >>>
> >>>> The update introduces a regression here. The strcmp function never returns 0.
> >>>> Indeed, it.node->name stores the memory region label "vdev0buffer," while
> >>>> res.name stores the memory region name "vdev0buffer@10042000."
> >>>>
> >>>> Several remoteproc drivers may face the same issue as they embed similar code.
> >>>
> >>> Indeed. I confused myself because node 'name' is without the
> >>> unit-address, but this is using the full name. I've replaced the
> >>> strcmp's with strstarts() to address this. I've updated my branch with
> >>> the changes.
> >>
> >> This is not enough as the remoteproc core function rproc_find_carveout_by_name()
> >> also compares the memory names. With the following additional fix, it is working
> >> on my STM32MP15-DK board.
> >>
> >> @@ -309,11 +309,11 @@ rproc_find_carveout_by_name(struct rproc *rproc, const
> >> char *name, ...)
> >>         vsnprintf(_name, sizeof(_name), name, args);
> >>         va_end(args);
> >>
> >>         list_for_each_entry(carveout, &rproc->carveouts, node) {
> >>                 /* Compare carveout and requested names */
> >> -               if (!strcmp(carveout->name, _name)) {
> >> +               if (strstarts(carveout->name, _name)) {
> >>                         mem = carveout;
> >>                         break;
> >>                 }
> >>         }
> >>
> >> I just wonder if would not be more suitable to address this using the
> >> "memory-region-names" field.
> >
> > That would be better as you shouldn't really care what a provider node
> > name is where-as "memory-region-names" is meaningful to the driver.
> >
> >>
> >> The drawback is that we would break compatibility with legacy boards...
> >
> > So not an option.
>
> >
> > I think I'll have to fix this within the reserved mem code storing the
> > name or do something like the diff below. I'd like to avoid the
> > former. Using the original device_node.name is also problematic
> > because I want to get rid of it. We redundantly store the node name
> > with and without the unit-address. There's a lot of places like this
> > one where we hand out the pointer with no lifetime.
> >
> > diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
> > index 1e949694d365..cdee87c6ffe0 100644
> > --- a/drivers/remoteproc/stm32_rproc.c
> > +++ b/drivers/remoteproc/stm32_rproc.c
> > @@ -239,7 +239,7 @@ static int stm32_rproc_prepare(struct rproc *rproc)
> >                                                    resource_size(&res), da,
> >                                                    stm32_rproc_mem_alloc,
> >                                                    stm32_rproc_mem_release,
> > -                                                  res.name);
> > +                                                  "%.*s",
> > strchrnul(res.name, '@') - res.name, res.name);
> >
> >                         if (mem)
> >                                 rproc_coredump_add_segment(rproc, da,
> > @@ -249,7 +249,7 @@ static int stm32_rproc_prepare(struct rproc *rproc)
> >                         mem = rproc_of_resm_mem_entry_init(dev, index,
> >                                                            resource_size(&res),
> >                                                            res.start,
> > -                                                          res.name);
> > +                                                          "vdev0buffer");
> >                 }
> >
> >                 if (!mem) {
>
>
> That's work on my side.
> Could we have an OF helper to retrieve the name from the full name?

That would be: sprintf(buf, "%pOFn", node);

The problem here is we don't have the device_node pointer. The only
way I see to make the above prettier is perhaps a define.

Rob