[PATCH] OPP: Fix support for required OPPs for multiple PM domains

[Ulf Hansson]

On Tue, 25 Jun 2024 at 12:54, Viresh Kumar <viresh.kumar at linaro.org> wrote:
>
> On 18-06-24, 17:50, Ulf Hansson wrote:
> > In _set_opp() we are normally bailing out when trying to set an OPP that is
> > the current one. This make perfect sense, but becomes a problem when
> > _set_required_opps() calls it recursively.
> >
> > More precisely, when a required OPP is being shared by multiple PM domains,
> > we end up skipping to request the corresponding performance-state for all
> > of the PM domains, but the first one. Let's fix the problem, by calling
> > _set_opp_level() from _set_required_opps() instead.
> >
> > Fixes: e37440e7e2c2 ("OPP: Call dev_pm_opp_set_opp() for required OPPs")
> > Cc: stable at vger.kernel.org
> > Signed-off-by: Ulf Hansson <ulf.hansson at linaro.org>
> > ---
> >  drivers/opp/core.c | 47 +++++++++++++++++++++++-----------------------
> >  1 file changed, 24 insertions(+), 23 deletions(-)
>
> >  /* This is only called for PM domain for now */
> >  static int _set_required_opps(struct device *dev, struct opp_table *opp_table,
> >                             struct dev_pm_opp *opp, bool up)
> > @@ -1091,7 +1113,8 @@ static int _set_required_opps(struct device *dev, struct opp_table *opp_table,
> >               if (devs[index]) {
> >                       required_opp = opp ? opp->required_opps[index] : NULL;
> >
> > -                     ret = dev_pm_opp_set_opp(devs[index], required_opp);
> > +                     ret = _set_opp_level(devs[index], opp_table,
> > +                                          required_opp);
>
> No, we won't be doing this I guess. Its like going back instead of
> moving forward :)
>
> The required OPPs is not just a performance domain thing, but
> specially with devs[] here, it can be used to set OPP for any device
> type and so dev_pm_opp_set_opp() is the right call here.
>
> Coming back to the problem you are pointing to, I am not very clear of
> the whole picture here. Can you please help me get some details on
> that ?

I get your point, but I am not sure I agree with it.

For the required-opps, the only existing use case is power/perf
domains with performance-states, so why make the code more complicated
than it needs to be?

>
> From what I understand, you have a device which has multiple power
> domains. Now all these power domains share the same OPP table in the
> device tree (i.e. to avoid duplication of tables only). Is that right
> ?

No, that's not correct. Let me try to elaborate on my setup, which is
very similar to a use case on a Tegra platform.

...

pd_perf0: pd-perf0 {
    #power-domain-cells = <0>;
    operating-points-v2 = <&opp_table_pd_perf0>;
};

//Note: no opp-table
pd_power4: pd-power4 {
    #power-domain-cells = <0>;
     power-domains = <&pd_perf0>;
};

//Note: no opp-table
pd_power5: pd-power5 {
     #power-domain-cells = <0>;
     power-domains = <&pd_perf0>;
};

//Note: The opp_table_pm_test10 are having required-opps pointing to
pd_perf0's opp-table.
pm_test10 {
    ...
    power-domains = <&pd_power4>, <&pd_power5>;
    power-domain-names = "perf4", "perf5";
    operating-points-v2 = <&opp_table_pm_test10>;
};

...

>
> Even if in DT we have the same OPP table for all the domains, the OPP
> core will have separate OPP tables structures (as the domains aren't
> connected). And these OPP tables will have their own `current_opp`
> fields and so we shouldn't really bail out earlier.

In the use case above, we end up never voting on pd_power5.

>
> Maybe there is a bug somewhere that is causing it. Maybe I can look at
> the DT to find the issue ? (Hint: The OPP table shouldn't have the
> `shared` flag set).
>
> Maybe I completely misunderstood the whole thing :)

The DT parsing of the required-opps is already complicated and there
seems to be endless new corner-cases showing up. Maybe we can fix this
too, but perhaps we should simply take a step back and go for
simplifications instead?

Kind regards
Uffe