linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Niklas Cassel <nks@flawful.org>
To: Stephan Gerhold <stephan@gerhold.net>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>,
	Loic Poulain <loic.poulain@linaro.org>,
	agross@kernel.org, linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH] arch: arm64: dts: apq8016-dbc: Add missing cpu opps
Date: Mon, 25 May 2020 21:44:43 +0200	[thread overview]
Message-ID: <20200525194443.GA11851@flawful.org> (raw)
In-Reply-To: <20200525163638.GA41001@gerhold.net>

On Mon, May 25, 2020 at 06:36:38PM +0200, Stephan Gerhold wrote:
> On Mon, May 25, 2020 at 05:32:47PM +0200, Niklas Cassel wrote:
> > On Wed, Apr 22, 2020 at 09:55:06PM -0700, Bjorn Andersson wrote:
> > > > Based on the available downstream sources I guessed the defines to add
> > > > for MSM8916 to the rpmpd driver. Then I added the VDD_MX OPPs as
> > > > "required-opps" to the CPU OPP table so it would vote for the appopriate
> > > > corners (with the mapping you mentioned above).
> > > > 
> > > 
> > > I was not aware it was possible to describe the dependency between the
> > > CPU opp table and MX in this fashion. If that's the case then this looks
> > > really good and it should be straight forward to add MSM8916 support to
> > > the CPR driver as well.
> > > 
> > > > I haven't tested it yet, maybe I can get some feedback first if the code
> > > > seems reasonable or if I'm missing something obvious? :)
> > > > 
> > > 
> > > Have you tested this yet?
> > > 
> > > > Also: Is there a good way to validate these changes?
> > > > I suppose I could check the genpd state but that wouldn't tell me if the
> > > > corner was applied correctly. Maybe I can check the actual voltage
> > > > through the SPMI interface, hm...
> > > > 
> > > 
> > > Validating that S2 and VDD_MX changes appropriately in Linux would be a
> > > pretty good test.
> > 
> > Like Bjorn says,
> > 
> > Downstream CPR on MSM8916 controls 3 things; VDD_APC, VDD_MX and MEMACC.
> > 
> > On QCS404 we don't have to adjust VDD_MX, therefore this is no code for
> > this in the upstream CPR driver. It just scales VPP_APC and MEMACC.
> > 
> > I like Stephan's idea of scaling VDD_APC and VDD_MEM to the maximum
> > necessary for the selected CPU frequency, until there is full CPR
> > support for MSM8916 (if ever).
> > 
> > 
> > The patch suggested so far looks good, however, I'm slightly worried
> > that this might lead to unstable boards, since MEMACC is never scaled
> > in the suggested patch.
> > 
> 
> Yeah, I was recently looking at that. I have no idea if it's needed.
> 
> If I understand this correctly, on downstream this is implemented
> separately as "mem-acc-regulator", although it is controlled by the
> "cpr-regulator" driver.
> 
> The mapping seems to be fairly static:
> Essentially it is just set to Nominal (1), SVS (2) or Turbo (3),
> depending on the CPU frequency. (On downstream this is specified in the
> device tree as qcom,cpr-corner-map = <1 1 2 2 3 3 3 3 3>; where each
> value is one CPU frequency.)
> 
> Additionally there seem to be some fuses to eventually override
> that behavior slightly (qcom,override-corner-acc-map).
> See: https://source.codeaurora.org/quic/la/kernel/msm-3.10/tree/arch/arm/boot/dts/qcom/msm8916-regulator.dtsi?h=LA.BR.1.2.9.1-02310-8x16.0#n29
> 
> On mainline this is currently entirely handled by the CPR driver,
> and the register sequence for QCS404 actually looks a bit more
> complicated... Hmm.

I agree:
https://source.codeaurora.org/quic/la/kernel/msm-4.14/tree/arch/arm64/boot/dts/qcom/qcs405-regulator.dtsi?h=msm-4.14#n294

> 
> The reason I mention all this: At least as I understand it,
> this isn't much different from the VDD_MX scaling. Essentially it
> doesn't strictly have something to do with the voltage scaling
> we do for CPR (VDD_APC), but rather it seems to be just another
> requirement when scaling the CPU frequency.

I agree.

> 
> In other words, I wonder if we should separate this into yet another
> power domain driver, and then reference it independently from CPR
> as additional required-opps for both MSM8916 and QCS404.

While this sounds very nice, by simply referencing these as
required-opps, I don't see how the OPP library will be able
to set APC, MX and MEMACC in the correct order:

https://source.codeaurora.org/quic/la/kernel/msm-4.14/tree/drivers/regulator/cpr-regulator.c?h=msm-4.14#n774

If scaling down, first MEMACC is set, then APC is set, then MX is set.

If scaling up, first MX is set, then APC is set, then MEMACC is set.



Perhaps the easiest way is to write a minimalistic cpufreq driver,
very similar to drivers/cpufreq/omap-cpufreq.c, that implements
the .target_index callback, and just like omap_target(), simply
gets the OPP using dev_pm_opp_find_freq_ceil(), and then calls
regulator_set_voltage() (APC), clk_set_rate() (to change the clock
frequency), dev_pm_genpd_set_performance_state() (to set MX),
and then do that little code that needs to be done for memacc.

Considering that CPR is not an actual power domain, CPR gives
adjustments to VDD_APC, but I don't know of any other device
connected to VDD_APC, other than the CPU, so in hindsight the CPR
driver probably should have been implemented using .target_index(),
rather than as a power domain provider using performance states.


Kind regards,
Niklas

  reply	other threads:[~2020-05-25 19:53 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 [this message]
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
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=20200525194443.GA11851@flawful.org \
    --to=nks@flawful.org \
    --cc=agross@kernel.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=loic.poulain@linaro.org \
    --cc=stephan@gerhold.net \
    /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).