Re: [RFC PATCH v2 4/6] PM: opp: allow control of multiple clocks

[Viresh Kumar <viresh.kumar@linaro.org>]

On 09-05-22, 12:38, Krzysztof Kozlowski wrote:
> On 25/04/2022 09:27, Viresh Kumar wrote:
> > This is tricky as the OPP core can't really assume the order in which the clocks
> > needs to be programmed. We had the same problem with multiple regulators and the
> > same is left for drivers to do via the custom-api.
> > 
> > Either we can take the same route here, and let platforms add their own OPP
> > drivers which can handle this, Or hide this all behind a basic device clock's
> > driver, which you get with clk_get(dev, NULL).
> 
> For my use case, the order of scaling will be the same as in previous
> implementation, because UFS drivers just got bunch of clocks with
> freq-table-hz property and were scaling in DT order.
> 
> If drivers need something better, they can always provide custom-opp
> thus replacing my method. My implementation here does not restrict them.
> 
> For the drivers where the order does not matter, why forcing each driver
> to provide its own implementation of clock scaling? Isn't shared generic
> PM OPP code a way to remove code duplication?

Code duplication is a good argument and I am in favor of avoiding it,
but nevertheless this shouldn't be something which platforms can pick
by mistake, just because they didn't go through core code. In other
words, this shouldn't be the default behavior of the core.

If we want, core can provide a helper to get rid of the duplication
though, but the user explicitly needs to use it.

> >> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > 
> >> +static int _generic_set_opp_clks_only(struct device *dev,
> >> +				      struct opp_table *opp_table,
> >> +				      struct dev_pm_opp *opp)
> >> +{
> >> +	int i, ret;
> >> +
> >> +	if (!opp_table->clks)
> >> +		return 0;
> >> +
> >> +	for (i = 0; i < opp_table->clk_count; i++) {
> >> +		if (opp->rates[i]) {
> > 
> > This should mean that we can disable that clock and it isn't required.
> 
> No, it does not mean that. The DT might provide several clocks which
> only some are important for frequency scaling. All others just need to
> be enabled.
> 
> Maybe you prefer to skip getting such clocks in PM OPP?

They shouldn't reach the OPP core then. What will the OPP core do if a
clock has a value for one OPP and not the other ?

> >> @@ -969,8 +1008,8 @@ static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
> > 
> > I think this routine breaks as soon as we add support for multiple clocks.
> > clks[0]'s frequency can be same for multiple OPPs and this won't get you the
> > right OPP then.
> 
> I don't think so and this was raised also by Stephen - only the first
> clock is considered the one used for all PM OPP frequency operations,
> like get ceil/floor.

IMHO, this is broken by design. I can easily see that someone wants to
have few variants of all other frequencies for the same frequency of
the so called "main" clock, i.e. multiple OPPs with same "main" freq
value.  I don't think we can mark the clocks "main" or otherwise as
easily for every platform.

Stephen, any inputs on this ?

> The assumption (which might need better documentation) is that first
> clock frequency is the main one:
> 1. It is still in opp->rate field, so it is used everywhere when OPPs
> are compared/checked for rates.
> 1. Usually is used also in opp-table nodes names.
> 
> The logical explanation is that devices has some main operating
> frequency, e.g. the core clock, and this determines the performance. In
> the same time such device might not be able to scale this one core clock
> independently from others, therefore this set of patches.

I understand what you are saying, but I can feel that it will break or
will force bad bug-fixes into the core at a later point of time.

I think it would be better to take it slowly and see how it goes. Lets
first add support for the OPP core to parse and store this data and
then we can add support to use it, or at least do all this in separate
patches so they are easier to review/apply.

-- 
viresh