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

To allow a dynamically added OPP to be coupled with a specific OPP provider
type, let's add a new enum variable in the struct dev_pm_opp_data.
Moreover, let's add support for a DEV_PM_OPP_TYPE_GENPD type, corresponding
to genpd's performance states support.

More precisely, this allows a genpd provider to dynamically add OPPs when a
device gets attached to it, that later can be used by a consumer driver
when it needs to change the performance level for its corresponding device.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/opp/core.c     | 19 +++++++++++++++++++
 drivers/opp/opp.h      |  1 +
 include/linux/pm_opp.h |  7 +++++++
 3 files changed, 27 insertions(+)

On Thu, 8 Jun 2023 at 07:34, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 07-06-23, 14:46, Ulf Hansson wrote:
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index 79b4b44ced3e..81a3418e2eaf 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -1112,6 +1112,15 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
> >                       return ret;
> >               }
> >
> > +             if (opp->provider == DEV_PM_OPP_TYPE_GENPD) {
> > +                     ret = dev_pm_genpd_set_performance_state(dev, opp->level);
> > +                     if (ret) {
> > +                             dev_err(dev, "Failed to set performance level: %d\n",
> > +                                     ret);
> > +                             return ret;
> > +                     }
> > +             }
> > +
>
> I don't like this :)
>
> We already have these calls in place from within _set_required_opps(), and we
> should try to get this done in a way that those calls themselves get the
> performance state configured.

I was looking at that, but wanted to keep things as simple as possible
in the $subject series.

The required opps are also different, as it's getting parsed from DT
both for the genpd provider and the consumer. The point is, there are
more code involved but just _set_required_opps().

For example, _set_performance_state() (which is the one that calls
dev_pm_genpd_set_performance_state()) is designed to be used for
required opps. Does it really make sense to rework
_set_performance_state() so it can be used for this case too, just to
avoid another call to dev_pm_genpd_set_performance_state() somewhere
in the code?

One improvement we can make though, is to add a helper function,
"_set_opp_level()", which we call from _set_opp(). This can then
replace the call to _set_required_opps() and the code above that I am
adding for DEV_PM_OPP_TYPE_GENPD. At least that should keep the code
_set_opp() a bit more readable.

What do you think?

Kind regards
Uffe

On 08-06-23, 11:37, Ulf Hansson wrote:
> The required opps are also different, as it's getting parsed from DT
> both for the genpd provider and the consumer. The point is, there are
> more code involved but just _set_required_opps().
> 
> For example, _set_performance_state() (which is the one that calls
> dev_pm_genpd_set_performance_state()) is designed to be used for
> required opps. Does it really make sense to rework
> _set_performance_state() so it can be used for this case too, just to
> avoid another call to dev_pm_genpd_set_performance_state() somewhere
> in the code?

What we need here, in you case, is really the required-opp thing, without the
DT parsing. The genpd will have an OPP table here, and devices (you are adding
OPP table dynamically for) shall have the genpd's OPPs as their required OPPs,
since for setting OPPs of the device, it is *required* to have OPP of the genpd
set too. Just like how it happens with DT. No special handling will be required
in dev_pm_opp_set_opp() path in this case and existing code will just work. You
just need to set the required-opp tables properly.

On Thu, 8 Jun 2023 at 12:45, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 08-06-23, 11:37, Ulf Hansson wrote:
> > The required opps are also different, as it's getting parsed from DT
> > both for the genpd provider and the consumer. The point is, there are
> > more code involved but just _set_required_opps().
> >
> > For example, _set_performance_state() (which is the one that calls
> > dev_pm_genpd_set_performance_state()) is designed to be used for
> > required opps. Does it really make sense to rework
> > _set_performance_state() so it can be used for this case too, just to
> > avoid another call to dev_pm_genpd_set_performance_state() somewhere
> > in the code?
>
> What we need here, in you case, is really the required-opp thing, without the
> DT parsing. The genpd will have an OPP table here, and devices (you are adding
> OPP table dynamically for) shall have the genpd's OPPs as their required OPPs,
> since for setting OPPs of the device, it is *required* to have OPP of the genpd
> set too. Just like how it happens with DT. No special handling will be required
> in dev_pm_opp_set_opp() path in this case and existing code will just work. You
> just need to set the required-opp tables properly.

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?

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?

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.

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.

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.

Kind regards
Uffe