On Tue, Aug 25, 2020 at 02:42:54PM +0200, Ulf Hansson wrote: > On Tue, 25 Aug 2020 at 09:34, Stephan Gerhold <step...@gerhold.net> wrote: > > > > On Tue, Aug 25, 2020 at 08:43:42AM +0200, Ulf Hansson wrote: > > > On Tue, 25 Aug 2020 at 06:43, Viresh Kumar <viresh.ku...@linaro.org> > > > wrote: > > > > > > > > On 24-08-20, 17:08, Stephan Gerhold wrote: > > > > > On Mon, Aug 24, 2020 at 04:36:57PM +0200, Ulf Hansson wrote: > > > > > > 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. > > > > > > > > > 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). > > > > > > > > > > > > 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. > > > > > 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. > > > > > 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
