On Tue, Aug 25, 2020 at 02:42:54PM +0200, Ulf Hansson wrote:

On Tue, 25 Aug 2020 at 09:34, Stephan Gerhold [off-list ref] wrote:

quoted

On Tue, Aug 25, 2020 at 08:43:42AM +0200, Ulf Hansson wrote:

quoted

On Tue, 25 Aug 2020 at 06:43, Viresh Kumar [off-list ref] wrote:

quoted

On 24-08-20, 17:08, Stephan Gerhold wrote:

quoted

On Mon, Aug 24, 2020 at 04:36:57PM +0200, Ulf Hansson wrote:

quoted

That said, perhaps should rely on the consumer to deploy runtime PM
support, but let the OPP core to set up the device links for the genpd
virtual devices!?

Yes, that would be the alternative option.

That is the right option IMO.

quoted

I would be fine with it as long as it also works for the CPUfreq case.

I don't think anything manages runtime PM for the CPU device, just
like no-one calls dev_pm_opp_set_rate(cpu_dev, 0). So with my patch the
power domain is essentially kept always-on (except for system suspend).
At least in my case this is intended.

If device links also keep the power domains on if the consumer device
does not make use of runtime PM it should work fine for my case.

With device link, you only need to do rpm enable/disable on the consumer device
and it will get propagated by itself.

Note that the default state for the genpd virtual device(s) is that
runtime PM has been enabled for them. This means it's left in runtime
suspended state, which allows its PM domain to be powered off (if all
other devices and child domains for it allow that too, of course).

quoted

quoted

Personally, I think my original patch (without device links) fits better
into the OPP API, for the following two reasons.

With device links:

  1. Unlike regulators/interconnects, attached power domains would be
     controlled by runtime PM instead of dev_pm_opp_set_rate(opp_dev, 0).

  2. ... some driver using OPP tables might not make use of runtime PM.
     In that case, the power domains would stay on the whole time,
     even if dev_pm_opp_set_rate(opp_dev, 0) was called.

With my patch, the power domain state is directly related to the
dev_pm_opp_set_rate(opp_dev, 0) call, which is more intuitive than
relying on the runtime PM state in my opinion.

So opp-set-rate isn't in the best of shape TBH, some things are left for the
drivers while other are done by it. Regulator-enable/disable was moved to it
some time back as people needed something like that. While on the other hand,
clk_enable/disable doesn't happen there, nor does rpm enable/disable.

Maybe one day we may want to do that, but lets make sure someone wants to do
that first.

Anyway, even in that case both of the changes would be required. We must make
device links nevertheless first. And later on if required, we may want to do rpm
enable/disable on the consumer device itself.

This sounds like a reasonable step-by-step approach.

Then, to create the device links, we should use DL_FLAG_PM_RUNTIME,
DL_FLAG_STATELESS.

OK, I will give this a try later this week.

quoted

But whether we should use DL_FLAG_RPM_ACTIVE as well, to initially
runtime resume the supplier (the genpd virtual device), is harder to
know - as that kind of depends on expectations by the consumer device
driver.

I'm not sure I understand the purpose of that flag. I thought we want to
link the PM state of the virtual genpd device (supplier) to the PM state
of the device of the OPP table (consumer).

Correct, but this is about synchronizing the initial runtime PM state
of the consumer and supplier.

quoted

Shouldn't it just determine the initial state based on the state of the
consumer device?

We could do that. Then something along the lines of the below, should work:

pm_runtime_get_noresume(consumer) - to prevent runtime suspend, temporarily.

if(pm_runtime_active(consumer))
    create links with DL_FLAG_RPM_ACTIVE
else
    create links without DL_FLAG_RPM_ACTIVE

pm_runtime_put(consumer)

This seems to work as expected, thanks for the suggestion!
I will submit a v2 with that shortly.

Thanks!
Stephan