On Fri, Mar 21, 2025 at 3:25 AM Arnaud POULIQUEN
<arnaud.pouliquen@xxxxxxxxxxx> wrote:
>
>
>
> On 3/20/25 19:02, Rob Herring wrote:
> > On Thu, Mar 20, 2025 at 4:23 AM Arnaud POULIQUEN
> > <arnaud.pouliquen@xxxxxxxxxxx> wrote:
> >>
> >>
> >>
> >> On 3/20/25 00:04, Rob Herring wrote:
> >>> On Wed, Mar 19, 2025 at 10:26 AM Arnaud POULIQUEN
> >>> <arnaud.pouliquen@xxxxxxxxxxx> 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@xxxxxxxxxx>
> >>>>> ---
> >>>>> 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
