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

["Guilherme G. Piccoli" <gpiccoli@igalia.com>]

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.


>> 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.


> [...]
> 
> 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!

Let me know your thoughts!
Cheers,


Guilherme