Re: [PATCH 13/16] OPP: Extend dev_pm_opp_data with OPP provider support

[Ulf Hansson]

On Fri, 9 Jun 2023 at 07:10, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
>
> On 08-06-23, 13:45, Ulf Hansson wrote:
> > Okay, if I understand your point you want to avoid creating OPPs for
> > each device, but rather coupling them with the genpd provider's OPP
> > table. Right?
>
> Not exactly :)
>
> > Note that, there is no such thing as a "required opp" in the SCMI
> > performance protocol case. A device is compatible to use all of the
> > OPPs that its corresponding SCMI performance domain provides. Should
> > we rename the required opp things in the OPP core to better reflect
> > this too?
>
> Not really :)

I think it may be confusing, but okay, let's leave it as is.

>
> > That said, we still need to be able to add OPPs dynamically when not
> > based on DT. The difference would be that we add the OPPs when
> > initializing the genpd provider instead of when attaching the devices.
> > In other words, we still need something along the lines of the new
> > dev_pm_opp_add_dynamic() API that $subject series is introducing, I
> > think.
>
> That's fine.
>
> > Moreover, to tie the consumer device's OPP table to their genpd
> > provider's OPP table (call it required-opp or whatever), we need
> > another OPP helper function that we can call from the genpd provider's
> > ->attach_dev() callback. Similarly, we need to be able to remove this
> > connection when genpd's ->detach_dev() callback is invoked. I will
> > think of something here.
>
> Something like set/link/config_required_opp()..
>
> > Finally, I want to point out that there is work going on in parallel
> > with this, that is adding performance state support for the ACPI PM
> > domain. The ACPI PM domain, isn't a genpd provider but implements it's
> > own PM domain. The important point is, that it will have its own
> > variant of the dev_pm_genpd_set_performance_state() that we may need
> > to call from the OPP library.
>
> Okay.
>
> Let me explain how structures are linked currently with help of below (badly
> made) drawing. The 'level' fields are only set for Genpd's OPP entries and not
> devices. The genpd OPP table normally has only this information, where it
> presents all the possible level values, separate OPP for each of them.
>
> Now the devices don't have `level` set in their OPP table, DT or in C
> structures. All they have are links for required OPPs, which help us find the
> required level or performance state for a particular genpd.
>
>   +-----------------+                    +------------------+
>   |  Device A       |                    | GENPD OPP Table  |
>   +-----------------+  required-opps     +------------------+   required-opps
>   |OPP1: freq: x1   +--------------------+ OPP1:            +--------------+
>   +-----------------+                    |      level: 1    |              |
>   |OPP2: freq: y1   +-----------+        +------------------+              |
>   +-----------------+           |        | OPP2:            +---------+    |
>   |OPP3: freq: z1   +--------+  +--------+      level: 2    |         |    |
>   +-----------------+        |           +------------------+         |    |
>                              |           | OPP3:            +--+      |    |
>                              |           |      level: 3    |  |      |    |
>                              |           +------------------+  |      |    |
>   +-----------------+        |           | OPP4:            |  |      |    |
>   |  Device B       |        +-----------+      level: 4    |  |      |    |
>   +-----------------+                    +------------------+  |      |    |
>   |OPP1: freq: x2   +------------------------------------------+      |    |
>   +-----------------+                                                 |    |
>   |OPP2: freq: y2   +-------------------------------------------------+    |
>   +-----------------+                                                      |
>   |OPP3: freq: z2   +------------------------------------------------------+
>   +-----------------+
>

Thanks for taking the time to explain things further! It's not
entirely easy to follow all the things in the OPP library.

However, I think you are mixing up the "level" field with the "pstate"
field in the struct dev_pm_opp. The pstate field is solely for genpd
and the required opps, while level is *generic* for all types of
devices. Right?

More precisely, _read_opp_key() parses for the "opp-level" property
from DT to set the ->level field. Additionally, the generic OPP
helpers functions, like dev_pm_opp_get_level(),
dev_pm_opp_find_level_exact(), dev_pm_opp_find_level_ceil() allows any
type of consumer driver to operate on the level for an OPP for its
device. Please have a look at the apple-soc-cpufreq, for example.

>
> What I am asking you to do now is, create an OPP table for the Genpd first, with
> OPPs for each possible level. Now the Genpd layer creates the OPP table for
> Device A, where it won't fill the levels, but all other fields and then use a
> new API to add required OPPs for the device's OPP, which will point into Genpd's
> OPP entries. And with that the existing code will work fine without any other
> modifications.
>
> Does this help ?

Well, I think what you propose may be doable, but I fail to understand
the benefit of implementing it like that.

As I said earlier, there is no such thing as a required OPP for the
SCMI performance domain and neither are the OPP tables described in
DT.

Moreover, as I understand it, we already have the generic "level" to
use, why not use it here?

Kind regards
Uffe