linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Stephan Gerhold <stephan@gerhold.net>
To: Niklas Cassel <nks@flawful.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>,
	Loic Poulain <loic.poulain@linaro.org>,
	agross@kernel.org, linux-arm-msm@vger.kernel.org,
	Viresh Kumar <vireshk@kernel.org>
Subject: Re: [PATCH] arch: arm64: dts: apq8016-dbc: Add missing cpu opps
Date: Wed, 27 May 2020 22:56:07 +0200	[thread overview]
Message-ID: <20200527205518.GA8254@gerhold.net> (raw)
In-Reply-To: <20200527125919.GA16363@flawful.org>

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

  reply	other threads:[~2020-05-27 20:56 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-01 17:50 [PATCH] arch: arm64: dts: apq8016-dbc: Add missing cpu opps Loic Poulain
2020-04-01 23:46 ` Bjorn Andersson
2020-04-02  8:13 ` Stephan Gerhold
2020-04-02  9:58   ` Loic Poulain
2020-04-03  1:31   ` Bjorn Andersson
2020-04-03 10:09     ` Stephan Gerhold
2020-04-03 18:00       ` Stephan Gerhold
2020-04-23  4:55         ` Bjorn Andersson
2020-04-26 12:31           ` Stephan Gerhold
2020-05-06 21:18             ` Stephan Gerhold
2020-05-07  5:34               ` Bjorn Andersson
2020-05-08 12:08                 ` Ulf Hansson
2020-05-08 13:42                   ` Stephan Gerhold
2020-05-11  5:29                   ` Viresh Kumar
2020-05-07 10:46               ` Stephan Gerhold
2020-05-21 19:18                 ` Stephan Gerhold
2020-05-23 12:08                   ` Stephan Gerhold
2020-05-27 20:47                     ` Georgi Djakov
2020-05-25 15:32           ` Niklas Cassel
2020-05-25 16:36             ` Stephan Gerhold
2020-05-25 19:44               ` Niklas Cassel
2020-05-26  8:59                 ` Stephan Gerhold
2020-05-26 15:54                   ` Niklas Cassel
2020-05-26 20:54                     ` Stephan Gerhold
2020-05-27 10:39                       ` Niklas Cassel
2020-05-27 12:04                         ` Stephan Gerhold
2020-05-27 12:59                           ` Niklas Cassel
2020-05-27 20:56                             ` Stephan Gerhold [this message]
2020-05-27 23:10                               ` Niklas Cassel
2020-05-28 13:32                                 ` Stephan Gerhold
2020-05-28  4:44                           ` Viresh Kumar
2020-04-28 20:04 ` Amit Kucheria

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200527205518.GA8254@gerhold.net \
    --to=stephan@gerhold.net \
    --cc=agross@kernel.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=loic.poulain@linaro.org \
    --cc=nks@flawful.org \
    --cc=vireshk@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).