Messages in this thread | | | Subject | Re: [PATCH v7 3/5] remoteproc: Add support for runtime PM | From | Suman Anna <> | Date | Mon, 8 Jun 2020 18:10:39 -0500 |
| |
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 >>>> has broken the pm-runtime autosuspend feature we have in the >>>> OMAP remoteproc driver. See commit 5f31b232c674 ("remoteproc/omap: >>>> Add support for runtime auto-suspend/resume"). >>>> >>>> What was the original purpose of this patch, because there can be >>>> differing backends across different SoCs. >>> >>> Did you try pm_suspend_ignore_children()? It looks like it was made >>> for your use-case. >> >> Sorry for the delay in getting back. So, using >> pm_suspend_ignore_children() does fix my current issue. >> >> But I still fail to see the original purpose of this patch in the >> remoteproc core especially given that the core itself does not have >> any callbacks. If the sole intention was to call the parent pdev's >> callbacks, then I feel that state-machine is better managed within >> that particular platform driver itself, as the sequencing/device >> management can vary with different platform drivers. > > The problem is that with Ingenic SoCs some clocks must be enabled in > order to load the firmware, and the core doesn't give you an option to > register 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 > my patchset added .prepare/.unprepare callbacks to the struct rproc_ops, > but the feedback from the maintainers was that I should do it via > runtime PM. However, it was not possible to keep it contained in the > driver, since again the core doesn't provide a "prepare" callback, so no > place 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.
So we settled with having runtime > PM in the core without callbacks, which will trigger the runtime PM > callbacks 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 >>>>> rproc_fw_boot/rproc_shutdown >>>>> v6: Instead of prepare/unprepare callbacks, use PM runtime >>>>> callbacks >>>>> 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 >>>>> b/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 >>>>> *rproc, const struct firmware *fw) >>>>> if (ret) >>>>> return ret; >>>>> + 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, >>>>> fw->size); >>>>> /* >>>>> @@ -1391,7 +1398,7 @@ static int rproc_fw_boot(struct rproc >>>>> *rproc, const 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; >>>>> } >>>>> rproc->bootaddr = rproc_get_boot_addr(rproc, fw); >>>>> @@ -1435,6 +1442,8 @@ static int rproc_fw_boot(struct rproc >>>>> *rproc, const struct firmware *fw) >>>>> rproc->table_ptr = NULL; >>>>> disable_iommu: >>>>> rproc_disable_iommu(rproc); >>>>> +put_pm_runtime: >>>>> + pm_runtime_put(dev); >>>>> return ret; >>>>> } >>>>> @@ -1840,6 +1849,8 @@ void rproc_shutdown(struct rproc *rproc) >>>>> rproc_disable_iommu(rproc); >>>>> + 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 >>>>> *dev, const char *name, >>>>> rproc->state = RPROC_OFFLINE; >>>>> + 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); >>>>> >>>> > >
| |