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

[Niklas Cassel <nks@flawful.org>]

On Wed, May 27, 2020 at 02:04:52PM +0200, Stephan Gerhold wrote:
> > 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.
> > 
> > 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.
> > 
> > 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...

You are going to need performance states where it is needed,
e.g. for controlling MX (as it is a power domain).
For performance states, you using the required-opps DT property.

My 50 cents is just to not use it for something that is not
a power domain, e.g. MEMACC.

> 
> 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

Yes, and there are more ways to implement AVS, see e.g.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=25cb20a212a1f989385dfe23230817e69c62bee5

Which probably would have been a better fit for CPR,
if it were to be designed today.

> 
> 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.

You care about how to do DVFS without AVS (CPR).

Why do you worry about how it will look when, if ever,
you transition to full AVS (CPR)?

If the CPR driver is not changed, which I doubt, you will simply change
the device tree to remove the cpu-supply regulator and move it into the
new CPR DT node.

Old device trees will still use your DVFS solution, new device
trees will use the CPR DT node and will use the AVS solution.


Kind regards,
Niklas