Re: [PATCH v8 01/34] opp: Add dev_pm_opp_sync() helper

[Viresh Kumar <viresh.kumar@xxxxxxxxxx>]

On 19-08-21, 22:35, Dmitry Osipenko wrote:
> 19.08.2021 16:07, Ulf Hansson пишет:
> > In the other scenario where a consumer driver prefers to *not* call
> > pm_runtime_resume_and_get() in its ->probe(), because it doesn't need
> > to power on the device to complete probing, then we don't want to vote
> > for an OPP at all - and we also want the performance state for the
> > device in genpd to be set to zero. Correct?
> 
> Yes
> 
> > Is this the main problem you are trying to solve, because I think this
> > doesn't work out of the box as of today?
> 
> The main problem is that the restored performance state is zero for the
> first genpd_runtime_resume(), while it's not zero from the h/w perspective.

This is exactly why I have been advocating that the genpd needs to
sync up with the hardware before any calls are made to it from the
consumer driver. Just what clock framework does to get the clock rate.

> > There is another concern though, but perhaps it's not a problem after
> > all. Viresh told us that dev_pm_opp_set_rate() may turn on resources
> > like clock/regulators. That could certainly be problematic, in
> > particular if the device and its genpd have OPP tables associated with
> > it and the consumer driver wants to follow the above sequence in
> > probe.
> 
> dev_pm_opp_set_rate() won't enable clocks and regulators, but it may

It does enable regulators right now, it may choose to enable clocks
later on, no guarantees.

> change the clock rate and voltage. This is also platform/driver specific
> because it's up to OPP user how to configure OPP table. On Tegra we only
> assign clock to OPP table, regulators are unused.

Right, over that platforms can set their own version of set-opp
callback, where all this is done from a platform specific callback.

> > Viresh, can you please chime in here and elaborate on some of the
> > magic happening behind dev_pm_opp_set_rate() API - is there a problem
> > here or not?

It configures clock, regulators, genpds, any required OPPs, + it
enables regulators right now.

-- 
viresh