Re: [PATCH] arch: arm64: dts: apq8016-dbc: Add missing cpu opps

[Stephan Gerhold <stephan@gerhold.net>]

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