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
