On Thu, 19 Aug 2021 at 08:17, Viresh Kumar [off-list ref] wrote:

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

quoted

18.08.2021 12:41, Ulf Hansson пишет:

Either way gives the equal result. The new callback allows to remove the
boilerplate dev_pm_opp_set_rate(clk_get_rate() code from the rpm-resume
of consumer devices, that's it.

It may not be equal, as dev_pm_opp_set_rate() may do additional stuff,
now or in a later implementation. Currently it only does
regulator_enable() as a special case, but it can be clk_enable() as
well. Also, this tries to solve the problem in a tricky/hacky way,
while all you wanted was to make the genpd aware of what the
performance state should be.

Your driver can break tomorrow if we started to do more stuff from
this API at another time.

quoted

quoted

dev_pm_opp_set_rate() is best called from consumer drivers, as they
need to be in control.

quoted

What we need here is just configure. So something like this then:

The intent wasn't to use dev_pm_opp_set_rate() from
__genpd_dev_pm_attach(), but to set genpd->rpm_pstate in accordance to
the h/w configuration.

Right.

quoted

On Tegra we have a chain of PDs and it's not trivial to convert the
device's OPP into pstate because only the parent domain can translate
the required OPP.

The driver should just be required to make a call, and OPP/genpd core
should return it a value. This is already done today while setting the
pstate for a device. The same frameworks must be able to supply a
value to be used for the device.

Right, that sounds reasonable.

We already have pm_genpd_opp_to_performance_state() which translates
an OPP to a performance state. This function invokes the
->opp_to_performance_state() for a genpd. Maybe we need to allow a
genpd to not have ->opp_to_performance_state() callback assigned
though, but continue up in the hierarchy to see if the parent has the
callback assigned, to make this work for Tegra?

Perhaps we should add an API dev_pm_genpd_opp_to_performance_state(),
allowing us to pass the device instead of the genpd. But that's a
minor thing.

Finally, the precondition to use the above, is to first get a handle
to an OPP table. This is where I am struggling to find a generic
solution, because I guess that would be platform or even consumer
driver specific for how to do this. And at what point should we do
this?

quoted

Viresh, please take a look at what I did in [1]. Maybe it could be done
in another way.

I looked into this and looked like too much trouble. The
implementation needs to be simple. I am not sure I understand all the
problems you faced while doing that, would be better to start with a
simpler implementation of get_performance_state() kind of API for
genpd, after the domain is attached and its OPP table is initialized.

Note, that the OPP table isn't required to be fully initialized for
the device at this point, we can parse the DT as well if needed be.

Sure, but as I indicated above, you need some kind of input data to
figure out what OPP table to pick, before you can translate that into
a performance state. Is that always the clock rate, for example?

Perhaps, we should start with adding a dev_pm_opp_get_from_rate() or
what do you think? Do you have other suggestions?

--
viresh

Kind regards
Uffe