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
Subject: Re: [PATCH] arch: arm64: dts: apq8016-dbc: Add missing cpu opps
Date: Mon, 25 May 2020 18:36:38 +0200 [thread overview]
Message-ID: <20200525163638.GA41001@gerhold.net> (raw)
In-Reply-To: <20200525153246.GA9224@flawful.org>
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.
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.
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.
CPR would then be only responsible for the actual adaptive voltage
scaling of VDD_APC.
Does that make sense?
Thanks,
Stephan
next prev parent reply other threads:[~2020-05-25 16:36 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 [this message]
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
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=20200525163638.GA41001@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 \
/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).