Re: [PATCH] arch: arm64: dts: apq8016-dbc: Add missing cpu opps

[Viresh Kumar <viresh.kumar@linaro.org>]

On 27-05-20, 14:04, Stephan Gerhold wrote:
> +Cc Viresh (should have already done this earlier :) )
> 
> On Wed, May 27, 2020 at 12:39:21PM +0200, Niklas Cassel wrote:
> > On Tue, May 26, 2020 at 10:54:03PM +0200, Stephan Gerhold wrote:
> > > Speaking of the current solution, I also have to say that (IMO) the
> > > device tree binding for "required-opps" is rather confusing
> > > and potentially misleading.
> > > 
> > > e.g. for VDD_MX scaling I use
> > > 
> > > 	required-opps = <&rpmpd_opp_nom>;
> > > 
> > > but looking at just the OPP table absolutely nothing tells me this is
> > > supposed to apply to VDD_MX. You actually need to go search for the cpu@
> > > device tree node and then know that some of the power domains there
> > > (in some order) are eventually going to be used for the required-opps
> > > there. The order is only defined by the qcom-nvmem-cpufreq driver.
> > > 
> > > It took me a few hours to get that right... :)

I agree, we need a way to figure out devices as well for which the
required-opp works. And yes that's missing.

> > > Nevertheless I guess we need a solution for scaling MEMACC without CPR
> > > for now. :) I'm not sure if rewriting all this is very realistic
> > > (if even possible). So I guess we might be stuck with the genpd approach?
> > 
> > I agree, the CPR driver will most likely not be changed now, since we
> > need to be compatible with the existing device tree.

A driver can be changed as much as you want, just that you need to
honor both new and old DTs.

> > 
> > For DVFS without CPR:
> > 
> > You need to scale APC, MX, MEMACC.
> > 
> > If we don't care about MEMACC, then the existing code in the OPP library
> > satisfies all our needs.
> > The problem here is if we need to do MEMACC as well.
> > 
> > I don't think it is proper to implement MEMACC as a power domain
> > (because it is not). Thus, we can't add it as a required-opp.

Required-opps can be extended if there is a real need. It isn't just
about power domains.

> > Another problem is that MEMACC should be done after regulator_set_voltage()
> > when scaling up, and before regulator_set_voltage() when scaling down.
> > 
> > So even if MEMACC was a power domain, currently the OPP library does
> > the _set_required_opps() call in the wrong order needed for MEMACC.
> > 
> > Like you said, the OPP library almost does everything already,
> > so it probably makes most sense to extend it to your needs,
> > rather than duplicating most of the code inside dev_pm_opp_set_rate().
> > 
> > 
> > I guess what you really want is two new optional callbacks in
> > dev_pm_opp_set_rate(), one before _generic_set_opp_regulator() and one
> > after, where you could do the MEMACC thing.
> > 
> > The callbacks need to have a parameter that tells if we are scaling down
> > or up.

NAK :)

> > Or, if Viresh doesn't like new function pointers, create a new
> > OPP_EVENT_* that you can register for, and in that callback you do what
> > you need.

NAK :)

> > Or, maybe you can even use the existing CPUFREQ_TRANSITION_NOTIFIER,
> > with CPUFREQ_PRECHANGE / CPUFREQ_POSTCHANGE, however, I'm not sure
> > how nicely they play when you are using the OPP library.
> > 
> 
> I'm not sure. Overall all of this doesn't really sound like it is going
> to make all this easier to understand (from looking at the device tree).
> We then have required-opps for VDD_MX, and CPR (which isn't really a
> power domain), and something entirely different for MEMACC (which like
> CPR, isn't really a power domain).
> 
> I don't know, right now this mixture of different approaches sounds
> rather complicated (and confusing) to me...
> 
> Just to throw another idea in the room: there seems to be a set_opp()
> callback already in the OPP table, which bypasses the code that
> updates clock and regulators (see ti-opp-supply.c). Actually if I'm
> reading this correctly ti-opp-supply seems to implement adaptive voltage
> scaling similar to CPR with it. Seemingly we have two different solutions
> for the same concept now:
> 
>   - CPR implements a power domain provider (even though it's not really
>     a power domain since it has only one consumer)
>   - ti-opp-supply implements this with the set_opp() callback
> 
> In general I think this looks pretty nice - we don't duplicate the full
> cpufreq driver, but have control about the order
> regulators/clocks/power domains etc are changed.
> 
> I think something like this would fit quite well for my case
> (scaling MX, APC and MEMACC without CPR). However, not sure how it would
> integrate with the existing CPR driver at some point.
> 
> Adding Viresh to Cc in case he has some opinion for all this.

OPP core broadly is a place where we store/parse some data from the DT
and keep, so others can use it. opp_set_rate() was added to it to
avoid duplicating the same thing across drivers. As you have figured
out, the right way for you to solve it is by using your set_opp()
callback along with required-opps thing.

-- 
viresh