On Wed, 18 Aug 2021 at 08:27, Viresh Kumar viresh.kumar@linaro.org wrote:
On 18-08-21, 09:22, Dmitry Osipenko wrote:
18.08.2021 08:58, Viresh Kumar пишет:
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().
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.
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