On Wed, May 27, 2020 at 02:59:19PM +0200, Niklas Cassel wrote: > 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. > I agree. > > > > 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. > Would this actually work if you update the voltage of the current OPP? Looking at the qcom-cpr code the voltage is dynamically changed when receiving IRQs. I would say with that function you can only change the voltage when switching to that OPP the next time. Maybe I'm wrong though. Otherwise that's pretty nice, because the setup is very similar with or without CPR (since the regulator is still managed by the OPP core). > > > > 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)? > I mean, CPR is arguably the better solution on long term. I doubt it will be me who will enable it for MSM8916 (mainly for lack of understanding of the whole scaling formulas etc), but IMO it would be worth to at least consider how to switch in the future. > 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. > I think my concern is essentially that I would duplicate the MEMACC code into a new driver utilizing .set_opp() - and to keep backwards compatibility we would need to keep that duplication forever. The MEMACC scaling isn't all that complicated, but overall I would prefer to avoid introducing duplication in the first place. Also: If full CPR ever happens we would be basically back to one part of the current discussion: specifying two "required-opps" MX and CPR (= APC + MEMACC) would result in the wrong order when scaling up/down. But maybe I just worry too much? Thanks, Stephan
