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

[Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>]

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