Re: [PATCH] arm64: dts: qcom: db820c: Improve regulator voltage range and mark it as always-on

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Tue 28 Sep 14:54 PDT 2021, Guilherme G. Piccoli wrote:

> Bjorn, first of all, thanks a *lot* for your great/informative response!
> Much appreciated =)
> 
> I'll respond more inline, below:
> 
> 
> On 28/09/2021 00:01, Bjorn Andersson wrote:
> > 
> > The regulator range described in the datasheet and in the regulator
> > driver defines what the PMIC can do, the dts refines this to a range
> > that it suitable for this particular board. So it's expected to be more
> > narrow.
> > 
> > The problem with vdd_gfx is that we don't have anything voting for an
> > actual voltage, so you will either continue to run with whatever voltage
> > we got from the power-on state (or bootloader), or
> > regulator-min-microvolt. Until someone could come up with this support
> > 0.98V seems to have been picked as some good enough number...
> > 
> > 
> > The right thing would be to ensure that the voltage is scaled with the
> > GPU clock, presumably with some reference from gpu_opp_table. This can
> > perhaps be done using static voltages, but should in the long run be
> > done by votes against the performance states exposed by the CPR block
> > attached to the vdd_gfx - which will then ensure that the voltage level
> > is adjusted as needed on a per-device basis.
> > 
> 
> Very interesting...thanks for the details. Just confirming: CPR is Core
> Power Reduction, right?
> Found its (DTS) documentation at
> devicetree/bindings/power/avs/qcom,cpr.txt, I'll study more and also the
> driver counter-part.
> 

I expect that we should be able to add MSM8996 support on top of:
https://lore.kernel.org/linux-arm-msm/20210901155735.629282-1-angelogioacchino.delregno@somainline.org/

> 
> >> More than that, my experiment showed that this regulator must be set to
> >> always-on - this idea came from a commit in Linaro's tree, from Rajendra [2].
> > 
> > The regulator should be enabled whenever someone is voting for the
> > GPU_GX_GDSC power domain of mmcc. Question is why this isn't enough.
> > 
> 
> This is very interesting, I'll investigate more! I just tested and
> indeed, without that setting, the board reboots suddenly.
> 
> 
> >> With the voltage range updated plus set as always-on, the GPU is working
> >> correctly, in a stable fashion.
> >>
> > 
> > Could you perhaps check /sys/kernel/debug/regulator/regulator_summary to
> > see the voltage level you get for your VDD_GFX when it works?
> > 
> 
> Sure! This is the output:
> 
> VDD_GFX  1    1      0    fast  1000mV     0mA   350mV  1350mV
>    8c0000.clock-controller-vdd-gfx   0 0mA     0mV     0mV
> 
> So, 1000mV is enough it seems.
> 

980mV is quite close to 1000mV, but I guess if 1V is just barely enough
then 980mV might be too much.

> 
> > [...]
> > 
> > No need to apologize, the patch itself looks really good and nice to see
> > that you tested it on both v5.14 and linux-next!
> > 
> 
> Thank you =)
> 
> >>
> > status = "okay" is the default value, so unless you want to disable a
> > node in the dtsi by default, or override it to "okay" when it was
> > previously disabled, there's no need to provide "status" here.
> > 
> >> +
> > 
> > And this empty line is undesirable.
> > 
> > 
> > So to summarize, I do think there might be cases where your patch
> > lowers the GPU voltage from 0.98V to 0.35V, which would result in a sad
> > outcome. I think we should try to plug some voltages into gpu_opp_table,
> > but perhaps we need to look into CPR to figure out what those voltages
> > should be?
> > 
> 
> OK cool, removed the okay status, worked fine as you suggested.
> 
> Now, regarding this approach of plugging the voltages on gpu_opp, I can
> study more and try to come up with something. But it'll take some days
> at least - for now, do you think that'd be interesting to adjust the
> regulator voltage with min == 980mV and max == 1000mV? I tried here, and
> it worked!
> 

I don't know where 980mV comes from, so I don't know if 1V would be good
enough or if it just happens to work today.

I think if we could figure out how to wire up the gpu_opp_table to scale
the voltage, even statically (without CPR), that would be a good next
step.

Regards,
Bjorn

> Let me know your thoughts!
> Cheers,
> 
> 
> Guilherme