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

On Wed, 18 Aug 2021 at 08:27, Viresh Kumar [off-list ref] wrote:

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

quoted

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

quoted

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.

quoted

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().

Okay, please use dev_pm_opp_set_rate() instead then. New helper just
adds to the confusion and isn't doing anything special apart from
doing clk_get_rate() for you.

quoted

quoted

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.

Hmm, I wonder if that would be a problem since only genpd core is
going to call that routine anyway.

Me and Dmitry discussed adding a new genpd callback for this. I agreed
that it seems like a reasonable thing to add, if he insists.

The intent was to invoke the new callback from __genpd_dev_pm_attach()
when the device has been attached to its genpd. This allows the
callback, to invoke clk_get_rate() and then dev_pm_opp_set_rate(), to
update the vote according to the current state of the HW.

I am not sure if/why that approach seemed insufficient?

Another option to solve the problem, I think, is simply to patch
drivers to let them call dev_pm_opp_set_rate() during ->probe(), this
should synchronize the HW state too.

Dmitry, can you please elaborate on this?

Kind regards
Uffe