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
