Re: [PATCH 2/2] OPP/pmdomain: Fix the assignment of the required-devs

[Ulf Hansson]

On Wed, 4 Sept 2024 at 08:40, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
>
> On 03-09-24, 13:43, Ulf Hansson wrote:
> > On Tue, 3 Sept 2024 at 12:53, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> > > On 03-09-24, 11:54, Ulf Hansson wrote:
> > > > In the current code, genpd_find_opp_table() tries to find an OPP table
> > > > for the genpd that the device is getting attached to. Then genpd
> > > > passes that OPP table via devm_pm_opp_set_config(), to let the OPP
> > > > core to hook up a required-dev for it. This was a naive approach, as
> > > > that OPP table may not be the one that actually corresponds to a
> > > > required-opps for the required-dev. Consider the below in DT.
> > > >
> > > >         opp_table_devA: opp-table-devA {
> > > >                 compatible = "operating-points-v2";
> > > >
> > > >                 opp-devA-50 {
> > > >                         opp-hz = /bits/ 64 <2500>;
> > > >                         required-opps = <&opp_pd_50>; //corresponds to
> > > > pd_perf1's OPP table
> > > >                 };
> > > >                ....
> > > >
> > > >         devA {
> > > >                 compatible = "foo,bar";
> > > >                 power-domains = <&pd_perf0>, <&pd_perf1>; //both
> > > > pd_perf0 and pd_perf1 has OPP tables.
> > > >                 power-domain-names = "perf0", "perf1";
> > > >                 operating-points-v2 = <&opp_table_devA>;
> > > >         };
> > >
> > > I think another way forward would be to send an index along with
> > > required-dev information (now that you do it one by one). That index
> > > would be the index of the genpd in the genpd-list for the device. That
> > > would make it work, isn't it ?
> >
> > I am not sure how that index will be much helpful, but maybe I am not
> > fully understanding what you propose.
> >
> > Please note that the index of the power-domain doesn't need to match
> > the index of the required-opps.
>
> Yeah, I missed that, it doesn't happen via DT but by platform code. I
> do see problems where situation would be a bit ambiguous. Your example
> with a minor change to your code:
>
>         opp_table_devA: opp-table-devA {
>                 compatible = "operating-points-v2";
>
>                 opp-devA-50 {
>                         opp-hz = /bits/ 64 <2500>;
>                         required-opps = <&opp_pd_50, &opp_pd_51>; //corresponds to pd_perf1 and pd_perf0 (in reverse order)
>                 };
>                ....
>
>         devA {
>                 compatible = "foo,bar";
>                 power-domains = <&pd_perf0>, <&pd_perf1>; //both
> pd_perf0 and pd_perf1 has OPP tables.
>                 power-domain-names = "perf0", "perf1";
>                 operating-points-v2 = <&opp_table_devA>;
>         };
>
> Here, I don't think there is a way for us to know which genpd does
> opp_pd_50 belongs to and to which one opp_pd_51 does.
>
> We solve this by sending clock_names and regulator_names in OPP
> config structure. That gives the ordering in which required_opps are
> present. The same needs to be done for genpd, and then genpd core
> would be able to attach the right genpd with right required opp.

No, we don't need this for gend as $subject patch is addressing this
problem too. Let me elaborate.

The OPP core holds the information about the devA's required-opps and
to what OPP table each required-opps belongs to
(opp_table->required_opp_tables[n]).

The genpd core holds the information about the allocated virtual
devices that it creates when it attached devA to its power-domains.
The virtual device(s) gets a genpd attached to it and that genpd also
has an OPP table associated with it (genpd->opp_table).

By asking the OPP core to walk through the array of allocated
required-opps for devA and to match it against a *one* of the virtual
devices' genpd->opp_table, we can figure out at what index we should
assign the virtual device to in the opp_table->required_devs[index].

Kind regards
Uffe