Re: [PATCH v7 3/5] remoteproc: Add support for runtime PM

[Paul Cercueil <paul@crapouillou.net>]

Hi,

Le lun. 8 juin 2020 à 18:10, Suman Anna <s-anna@ti.com> a écrit :
> Hi Paul,
> 
> On 6/8/20 5:46 PM, Paul Cercueil wrote:
>> Hi Suman,
>> 
>>>>> On 5/15/20 5:43 AM, Paul Cercueil wrote:
>>>>>> Call pm_runtime_get_sync() before the firmware is loaded, and
>>>>>> pm_runtime_put() after the remote processor has been stopped.
>>>>>> 
>>>>>> Even though the remoteproc device has no PM callbacks, this 
>>>>>> allows the
>>>>>> parent device's PM callbacks to be properly called.
>>>>> 
>>>>> I see this patch staged now for 5.8, and the latest -next branch 
>>>>> \x7f\x7f\x7f\x7fhas \x7f\x7fbroken the pm-runtime autosuspend feature we have in 
>>>>> the \x7f\x7f\x7f\x7fOMAP \x7f\x7fremoteproc driver. See commit 5f31b232c674 
>>>>> ("remoteproc/omap: \x7f\x7f\x7f\x7fAdd \x7f\x7fsupport for runtime 
>>>>> auto-suspend/resume").
>>>>> 
>>>>> What was the original purpose of this patch, because there can be 
>>>>> \x7f\x7f\x7f\x7f\x7f\x7fdiffering backends across different SoCs.
>>>> 
>>>> Did you try pm_suspend_ignore_children()? It looks like it was 
>>>> made \x7f\x7f\x7ffor \x7fyour use-case.
>>> 
>>> Sorry for the delay in getting back. So, using 
>>> \x7f\x7fpm_suspend_ignore_children() does fix my current issue.
>>> 
>>> But I still fail to see the original purpose of this patch in the 
>>> \x7f\x7fremoteproc core especially given that the core itself does not 
>>> have \x7f\x7fany callbacks. If the sole intention was to call the parent 
>>> pdev's \x7f\x7fcallbacks, then I feel that state-machine is better 
>>> managed within \x7f\x7fthat particular platform driver itself, as the 
>>> sequencing/device \x7f\x7fmanagement can vary with different platform 
>>> drivers.
>> 
>> The problem is that with Ingenic SoCs some clocks must be enabled in 
>> \x7forder to load the firmware, and the core doesn't give you an option 
>> to \x7fregister a callback to be called before loading it.
> 
> Yep, I have similar usage in one of my remoteproc drivers (see 
> keystone_remoteproc.c), and I think this all stems from the need to 
> use/support loading into a processor's internal memories. My driver 
> does leverage the pm-clks backend plugged into pm_runtime, so you 
> won't see explicit calls on the clocks.
> 
> I guess the question is what exact PM features you are looking for 
> with the Ingenic SoC. I do see you are using pm_runtime autosuspend, 
> and your callbacks are managing the clocks, but reset is managed only 
> in start/stop.
> 
>> The first version of \x7fmy patchset added .prepare/.unprepare 
>> callbacks to the struct rproc_ops, \x7fbut the feedback from the 
>> maintainers was that I should do it via \x7fruntime PM. However, it was 
>> not possible to keep it contained in the \x7fdriver, since again the 
>> core doesn't provide a "prepare" callback, so no \x7fplace to call 
>> pm_runtime_get_sync().
> FWIW, the .prepare/.unprepare callbacks is actually now part of the 
> rproc core. Looks like multiple developers had a need for this, and 
> this functionality went in at the same time as your driver :). Not 
> sure if you looked up the prior patches, I leveraged the patch that 
> Loic had submitted a long-time ago, and a revised version of it is 
> now part of 5.8-rc1.

WTF maintainers, you refuse my patchset for adding a 
.prepare/.unprepare, ask me to do it via runtime PM, then merge another 
patchset that adds these callback. At least be constant in your 
decisions.

Anyway, now we have two methods added to linux-next for doing the exact 
same thing. What should we do about it?

-Paul

> So we settled with having runtime
>> PM in the core without callbacks, which will trigger the runtime PM 
>> \x7fcallbacks of the driver at the right moment.
> 
> Looks like we can do some cleanup on the Ingenic SoC driver depending 
> on the features you want.
> 
> regards
> Suman
> 
>> 
>> Sorry if that caused you trouble.
>> 
>> Cheers,
>> -Paul
>>>>> 
>> 
>>>>> 
>>>>>> 
>>>>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>>>>> ---
>>>>>> 
>>>>>> Notes:
>>>>>>      v2-v4: No change
>>>>>>      v5: Move calls to prepare/unprepare to 
>>>>>> \x7f\x7f\x7f\x7f\x7frproc_fw_boot/rproc_shutdown
>>>>>>      v6: Instead of prepare/unprepare callbacks, use PM runtime 
>>>>>> \x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7fcallbacks
>>>>>>      v7: Check return value of pm_runtime_get_sync()
>>>>>> 
>>>>>>   drivers/remoteproc/remoteproc_core.c | 17 ++++++++++++++++-
>>>>>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>>>>> 
>>>>>> diff --git a/drivers/remoteproc/remoteproc_core.c 
>>>>>> \x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7fb/drivers/remoteproc/remoteproc_core.c
>>>>>> index a7f96bc98406..e33d1ef27981 100644
>>>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>>>> @@ -29,6 +29,7 @@
>>>>>>   #include <linux/devcoredump.h>
>>>>>>   #include <linux/rculist.h>
>>>>>>   #include <linux/remoteproc.h>
>>>>>> +#include <linux/pm_runtime.h>
>>>>>>   #include <linux/iommu.h>
>>>>>>   #include <linux/idr.h>
>>>>>>   #include <linux/elf.h>
>>>>>> @@ -1382,6 +1383,12 @@ static int rproc_fw_boot(struct rproc 
>>>>>> \x7f\x7f\x7f\x7f\x7f*rproc, \x7f\x7f\x7fconst struct firmware *fw)
>>>>>>       if (ret)
>>>>>>           return ret;
>>>>>>   \x7f+    ret = pm_runtime_get_sync(dev);
>>>>>> +    if (ret < 0) {
>>>>>> +        dev_err(dev, "pm_runtime_get_sync failed: %d\n", ret);
>>>>>> +        return ret;
>>>>>> +    }
>>>>>> +
>>>>>>       dev_info(dev, "Booting fw image %s, size %zd\n", name, 
>>>>>> \x7f\x7f\x7f\x7f\x7ffw->size);
>>>>>>   \x7f      /*
>>>>>> @@ -1391,7 +1398,7 @@ static int rproc_fw_boot(struct rproc 
>>>>>> \x7f\x7f\x7f\x7f\x7f*rproc, \x7f\x7f\x7fconst struct firmware *fw)
>>>>>>       ret = rproc_enable_iommu(rproc);
>>>>>>       if (ret) {
>>>>>>           dev_err(dev, "can't enable iommu: %d\n", ret);
>>>>>> -        return ret;
>>>>>> +        goto put_pm_runtime;
>>>>>>       }
>>>>>>   \x7f      rproc->bootaddr = rproc_get_boot_addr(rproc, fw);
>>>>>> @@ -1435,6 +1442,8 @@ static int rproc_fw_boot(struct rproc 
>>>>>> \x7f\x7f\x7f\x7f\x7f*rproc, \x7f\x7f\x7fconst struct firmware *fw)
>>>>>>       rproc->table_ptr = NULL;
>>>>>>   disable_iommu:
>>>>>>       rproc_disable_iommu(rproc);
>>>>>> +put_pm_runtime:
>>>>>> +    pm_runtime_put(dev);
>>>>>>       return ret;
>>>>>>   }
>>>>>>   \x7f@@ -1840,6 +1849,8 @@ void rproc_shutdown(struct rproc *rproc)
>>>>>>   \x7f      rproc_disable_iommu(rproc);
>>>>>>   \x7f+    pm_runtime_put(dev);
>>>>>> +
>>>>>>       /* Free the copy of the resource table */
>>>>>>       kfree(rproc->cached_table);
>>>>>>       rproc->cached_table = NULL;
>>>>>> @@ -2118,6 +2129,9 @@ struct rproc *rproc_alloc(struct device 
>>>>>> \x7f\x7f\x7f\x7f\x7f*dev, \x7f\x7f\x7fconst char *name,
>>>>>>   \x7f      rproc->state = RPROC_OFFLINE;
>>>>>>   \x7f+    pm_runtime_no_callbacks(&rproc->dev);
>>>>>> +    pm_runtime_enable(&rproc->dev);
>>>>>> +
>>>>>>       return rproc;
>>>>>>   }
>>>>>>   EXPORT_SYMBOL(rproc_alloc);
>>>>>> @@ -2133,6 +2147,7 @@ EXPORT_SYMBOL(rproc_alloc);
>>>>>>    */
>>>>>>   void rproc_free(struct rproc *rproc)
>>>>>>   {
>>>>>> +    pm_runtime_disable(&rproc->dev);
>>>>>>       put_device(&rproc->dev);
>>>>>>   }
>>>>>>   EXPORT_SYMBOL(rproc_free);
>>>>>> 
>>>>> 
>> 
>> 
>