On 6/10/20 11:39 PM, Bjorn Andersson wrote:
On Wed 10 Jun 02:40 PDT 2020, Paul Cercueil wrote:
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.
Sorry, I missed this when applying the two patches, but you're of course
right.
Anyway, now we have two methods added to linux-next for doing the exact same
thing. What should we do about it?
I like the pm_runtime approach and as it was Arnaud that asked you to
change it, perhaps he and Loic can agree on updating the ST driver so we
can drop the prepare/unprepare ops again?
These callbacks were added primarily in preparation for the TI K3 rproc
drivers, not just ST (the patch was resurrected from a very old patch
from Loic).
I still think prepare/unprepare is actually better suited to scale well
for the long term. This pm_runtime logic will now make the early-boot
scenarios complicated, as you would have to match its status, but all
actual operations are on the actual parent remoteproc platform device
and not the child remoteproc device. I think it serves to mess up the
state-machines of different platform drivers due to additional refcounts
acquired and maybe performing some operations out of sequence to what a
platform driver wants esp. if there is automated backend usage like
genpd, pm_clks etc. I am yet to review Mathieu's latest MCU sync series,
but the concept of different sync_ops already scales w.r.t the
prepare/unprepare.
As for my K3 drivers, the callbacks are doing more than just turning on
clocks, as the R5Fs in general as a complex power-on sequence. I do not
have remoteproc auto-suspend atm on the K3 drivers, but that typically
means shutting down and restoring the core and would involve all the
hardware-specific sequences, so the rpm callback implementations will be
more than just clocks.
I looked through the patch history on the Ingenic remoteproc driver, and
the only reason for either of runtime pm usage or prepare/unprepare ops
usage is to ensure that clocks do not stay enabled in the case the
processor is not loaded/started. The driver is using auto-boot, so when
it probes, in general we expect the remoteproc to be running. So, the
only failure case is if there is no firmware. Otherwise, Paul could have
just used clk_bulk API in probe and remove.
Anyway, I will provide some additional review comments on the pm_runtime
usage within the Ingenic rproc driver.
regards
Suman
