Re: [PATCH v7 3/5] remoteproc: Add support for runtime PM
From: Paul Cercueil
Date: Wed Jun 10 2020 - 05:40:23 EST
Hi,
Le lun. 8 juin 2020 à 18:10, Suman Anna <s-anna@xxxxxx> 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
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.
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
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@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);