18.08.2021 08:58, Viresh Kumar пишет:

On 18-08-21, 08:21, Dmitry Osipenko wrote:

quoted

Yes, GENPD will cache the perf state across suspend/resume and initially
cached value is out of sync with h/w.

Nothing else. But let me clarify it all again.

Thanks for your explanation.

quoted

Initially the performance state of all GENPDs is 0 for all devices.

The clock rate is preinitialized for all devices to a some default rate
by clk driver, or by bootloader or by assigned-clocks in DT.

When device is rpm-resumed, the resume callback of a device driver
enables the clock.

Before clock is enabled, the voltage needs to be configured in
accordance to the clk rate.

So now we have a GENPD with pstate=0 on a first rpm-resume, which
doesn't match the h/w configuration. Calling dev_pm_opp_sync() sets the
pstate in accordance to the h/w config.

What about calling dev_pm_opp_set_rate(dev, clk_get_rate(dev)) here
instead ? That will work, right ? The advantage is it works without
any special routine to do so.

It will work, but a dedicated helper is nicer.

I also wonder looking at your gr3d.c changes, you set a set-opp
helper, but the driver doesn't call set_opp_rate at all. Who calls it
?

dev_pm_opp_sync() calls it from _set_opp().

And if it is all about just syncing the genpd core, then can the genpd
core do something like what clk framework does? i.e. allow a new
optional genpd callback, get_performance_state() (just like
set_performance_state()), which can be called initially by the core to
get the performance to something other than zero. opp-set-rate is
there to set the performance state and enable the stuff as well.
That's why it looks incorrect in your case, where the function was
only required to be called once, and you are ending up calling it on
each resume. Limiting that with another local variable is bad as well.

We discussed variant with get_performance_state() previously and Ulf
didn't like it either since it still requires to touch 'internals' of GENPD.