On Wed, Nov 12, 2025 at 10:59:42AM -0600, Rob Herring wrote:
> On Wed, Nov 12, 2025 at 9:43 AM Mathieu Poirier
> <[email protected]> wrote:
> >
> > On Tue, 11 Nov 2025 at 12:59, Rob Herring <[email protected]> wrote:
> > >
> > > On Tue, Nov 11, 2025 at 10:38:05AM -0700, Mathieu Poirier wrote:
> > > > Hi Rob,
> > > >
> > > > Please see may comment for st_remoteproc.c
> > > >
> > > > On Fri, Oct 31, 2025 at 12:59:22PM -0500, 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.
>
> [...]
>
> > > > > diff --git a/drivers/remoteproc/st_remoteproc.c
> > > > > b/drivers/remoteproc/st_remoteproc.c
> > > > > index e6566a9839dc..043348366926 100644
> > > > > --- a/drivers/remoteproc/st_remoteproc.c
> > > > > +++ b/drivers/remoteproc/st_remoteproc.c
> > > > > @@ -120,40 +120,37 @@ 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;
> > > >
> > > > The original code calls rproc_elf_load_rsc_table() [1] after iterating
> > > > through
> > > > the memory region, something that won't happen with the above.
> > >
> > > Indeed. it needs the following incremental change. It is slightly
> > > different in that rproc_elf_load_rsc_table() is not called if
> > > 'memory-region' is missing, but the binding says that's required.
> > >
> > > 8<--------------------------------------------------
> > >
> > > diff --git a/drivers/remoteproc/st_remoteproc.c
> > > b/drivers/remoteproc/st_remoteproc.c
> > > index 043348366926..cb09c244fdb5 100644
> > > --- a/drivers/remoteproc/st_remoteproc.c
> > > +++ b/drivers/remoteproc/st_remoteproc.c
> > > @@ -120,15 +120,19 @@ 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;
> > > - int index = 0, mr = 0;
> > > + int index = 0;
> > >
> > > while (1) {
> > > struct resource res;
> > > int ret;
> > >
> > > - ret = of_reserved_mem_region_to_resource(np, mr++, &res);
> > > - if (ret)
> > > - return 0;
> > > + ret = of_reserved_mem_region_to_resource(np, index, &res);
> > > + if (ret) {
> > > + if (index)
> > > + break;
> > > + else
> > > + return ret;
> > > + }
> >
> > This looks brittle and I'm not sure it would work.
> >
> > Going back to the original implementation, the only time we want to
> > "break" is when @index is equal to the amount of memory regions _and_
> > ret is -EINVAL. Any other condition should return.
>
> @index equal to number of entries returns -ENODEV, so that condition
> is impossible. We can simply it to this:
>
> if (ret == -ENODEV && index)
> break;
> else
> return ret;
To me this needs to be:
entries = of_reserved_mem_region_count(np);
...
...
if (ret == -ENODEV && index == entries)
break;
else
return ret;
But taking a step back, it might even be easier to go from a while() to a for(),
the same way you did in imx_rproc_addr_init().
>
> If you want to keep the prior behavior when 'memory-region' is
> missing, then '&& index' can be removed, but I think that was wrong
> behavior.
>
> Rob
