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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux