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

From: Ulf Hansson <ulf.hansson@linaro.org>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>,
	Cristian Marussi <cristian.marussi@arm.com>,
	Viresh Kumar <vireshk@kernel.org>, Nishanth Menon <nm@ti.com>,
	Stephen Boyd <sboyd@kernel.org>, Nikunj Kela <nkela@quicinc.com>,
	Prasad Sodagudi <psodagud@quicinc.com>,
	Alexandre Torgue <alexandre.torgue@foss.st.com>,
	linux-pm@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 13/16] OPP: Extend dev_pm_opp_data with OPP provider support
Date: Thu, 8 Jun 2023 13:45:42 +0200	[thread overview]
Message-ID: <CAPDyKFrbpm0W1Hdv+85MqMAC2-UbPOE6qp26L0UvVF0sSL2ayA@mail.gmail.com> (raw)
In-Reply-To: <20230608104540.tykxtvwhoyogthw5@vireshk-i7>

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