Message-ID: <05adb008-c4f0-4003-aef4-6ce6dbbfd392@foss.st.com>
Date: Thu, 20 Mar 2025 10:37:13 +0100
From: Arnaud POULIQUEN <arnaud.pouliquen@...s.st.com>
To: Rob Herring <robh@...nel.org>
CC: Maxime Coquelin <mcoquelin.stm32@...il.com>, <imx@...ts.linux.dev>,
        Saravana Kannan <saravanak@...gle.com>,
        Mathieu Poirier
	<mathieu.poirier@...aro.org>,
        <devicetree@...r.kernel.org>, Fabio Estevam
	<festevam@...il.com>,
        Bjorn Andersson <andersson@...nel.org>,
        <linux-remoteproc@...r.kernel.org>,
        <linux-stm32@...md-mailman.stormreply.com>,
        <linux-arm-kernel@...ts.infradead.org>,
        Pengutronix Kernel Team
	<kernel@...gutronix.de>,
        <linux-arm-msm@...r.kernel.org>, Shawn Guo
	<shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>, <linux-kernel@...r.kernel.org>
Subject: Re: [Linux-stm32] [PATCH 3/3] remoteproc: Use
 of_reserved_mem_region_* functions for "memory-region"



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@...s.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@...nel.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@...42000."
>>>
>>> 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@...s.st.com>
> tested-by: Arnaud Pouliquen <arnaud.pouliquen@...s.st.com>
> 
> - for the st_remoteproc
> reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@...s.st.com>
> 
> Thanks,
> Arnaud
> 
> 
>>
>> Rob
> _______________________________________________
> Linux-stm32 mailing list
> Linux-stm32@...md-mailman.stormreply.com
> https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32

Powered by blists - more mailing lists