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

[Suman Anna]

Hi Paul,

On 5/22/20 12:11 PM, Paul Cercueil wrote:
> Hi Suman,
>
> Le ven. 22 mai 2020 Ã 11:47, Suman Anna <s-anna@xxxxxx> a Ãcrit :
> > Hi Paul,
> >
> > 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.

regards
Suman

> Cheers,
> -Paul
>
> > regards
> > Suman
> >
> > > Signed-off-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx>
> > > ---
> > >
> > > 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);