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@xxxxxxxxxxxxxx/ > > >> 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
