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

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

[Guilherme G. Piccoli]

Commit 2317b87a2a6f ("arm64: dts: qcom: db820c: Add vdd_gfx and tie it into mmcc")
introduced this voltage regulator which seems to be essential for the GPU,
according to the board schematics [0]. The problem is that such commit sets
the regulator min/max voltage range to a static value, which is a bit lower than
the range supported to such regulator [1]. With that, the GPU is not stable
as per my experiments (in a Dragonboard 820c-based board) - I've observed
sudden reboots into a FW bad state.

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].
With the voltage range updated plus set as always-on, the GPU is working
correctly, in a stable fashion.

[0] See page 9 (VDD_GFX), at
https://www.96boards.org/documentation/consumer/dragonboard/dragonboard820c/hardware-docs/files/db820c-schematics.pdf

[1] See section 3.5.3 (FT-SMPS) in the "PMI8994/PMI8996 Power Management IC",
at https://developer.qualcomm.com/download/sd820e/pmi8994pmi8996-power-management-ic-device-specification.pdf

[2] https://git.linaro.org/landing-teams/working/qualcomm/kernel.git/commit/?h=release/qcomlt-4.14&id=75fb43f3a62

Cc: Rajendra Nayak <rnayak@codeaurora.org>
Cc: Vinod Koul <vkoul@kernel.org>
Fixes: 2317b87a2a6f ("arm64: dts: qcom: db820c: Add vdd_gfx and tie it into mmcc")
Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---

Hi Andy/Bjorn/all, this patch was tested in 5.14, but I've tested it
in the linux-next tree as well and was able to apply there cleanly.
I'm new in the DTS world, so my apologies in advance for any rookie
mistake - suggestions are appreciated! Thanks in advance for the review,


Guilherme


 arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
index 51e17094d7b1..977842068619 100644
--- a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
+++ b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
@@ -699,8 +699,11 @@ &pmi8994_spmi_regulators {
 	vdd_gfx: s2@1700 {
 		reg = <0x1700 0x100>;
 		regulator-name = "VDD_GFX";
-		regulator-min-microvolt = <980000>;
-		regulator-max-microvolt = <980000>;
+		regulator-min-microvolt = <350000>;
+		regulator-max-microvolt = <1350000>;
+		regulator-always-on;
+		status = "okay";
+
 	};
 };
 
-- 
2.33.0

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

[Guilherme G. Piccoli]

On 27/09/2021 13:37, Guilherme G. Piccoli wrote:
> Commit 2317b87a2a6f ("arm64: dts: qcom: db820c: Add vdd_gfx and tie it into mmcc")
> introduced this voltage regulator which seems to be essential for the GPU,
> according to the board schematics [0]. The problem is that such commit sets
> the regulator min/max voltage range to a static value, which is a bit lower than
> the range supported to such regulator [1]. With that, the GPU is not stable
> as per my experiments (in a Dragonboard 820c-based board) - I've observed
> sudden reboots into a FW bad state.
> 
> 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].
> With the voltage range updated plus set as always-on, the GPU is working
> correctly, in a stable fashion.
> 
> [0] See page 9 (VDD_GFX), at
> https://www.96boards.org/documentation/consumer/dragonboard/dragonboard820c/hardware-docs/files/db820c-schematics.pdf
> 
> [1] See section 3.5.3 (FT-SMPS) in the "PMI8994/PMI8996 Power Management IC",
> at https://developer.qualcomm.com/download/sd820e/pmi8994pmi8996-power-management-ic-device-specification.pdf
> 
> [2] https://git.linaro.org/landing-teams/working/qualcomm/kernel.git/commit/?h=release/qcomlt-4.14&id=75fb43f3a62
> 
> Cc: Rajendra Nayak <rnayak@codeaurora.org>
> Cc: Vinod Koul <vkoul@kernel.org>
> Fixes: 2317b87a2a6f ("arm64: dts: qcom: db820c: Add vdd_gfx and tie it into mmcc")
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> ---
> 
> Hi Andy/Bjorn/all, this patch was tested in 5.14, but I've tested it
> in the linux-next tree as well and was able to apply there cleanly.
> I'm new in the DTS world, so my apologies in advance for any rookie
> mistake - suggestions are appreciated! Thanks in advance for the review,
> 
> 
> Guilherme
> 
> 
>  arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
> index 51e17094d7b1..977842068619 100644
> --- a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
> +++ b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
> @@ -699,8 +699,11 @@ &pmi8994_spmi_regulators {
>  	vdd_gfx: s2@1700 {
>  		reg = <0x1700 0x100>;
>  		regulator-name = "VDD_GFX";
> -		regulator-min-microvolt = <980000>;
> -		regulator-max-microvolt = <980000>;
> +		regulator-min-microvolt = <350000>;
> +		regulator-max-microvolt = <1350000>;
> +		regulator-always-on;
> +		status = "okay";
> +
>  	};
>  };
>  
> 


Sorry, due to a git bad config, it supressed all CCs - adding them now!

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

[Bjorn Andersson]

On Mon 27 Sep 11:37 CDT 2021, Guilherme G. Piccoli wrote:

> Commit 2317b87a2a6f ("arm64: dts: qcom: db820c: Add vdd_gfx and tie it into mmcc")
> introduced this voltage regulator which seems to be essential for the GPU,
> according to the board schematics [0]. The problem is that such commit sets
> the regulator min/max voltage range to a static value, which is a bit lower than
> the range supported to such regulator [1]. With that, the GPU is not stable
> as per my experiments (in a Dragonboard 820c-based board) - I've observed
> sudden reboots into a FW bad state.
> 

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.

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

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

> [0] See page 9 (VDD_GFX), at
> https://www.96boards.org/documentation/consumer/dragonboard/dragonboard820c/hardware-docs/files/db820c-schematics.pdf
> 
> [1] See section 3.5.3 (FT-SMPS) in the "PMI8994/PMI8996 Power Management IC",
> at https://developer.qualcomm.com/download/sd820e/pmi8994pmi8996-power-management-ic-device-specification.pdf
> 
> [2] https://git.linaro.org/landing-teams/working/qualcomm/kernel.git/commit/?h=release/qcomlt-4.14&id=75fb43f3a62
> 
> Cc: Rajendra Nayak <rnayak@codeaurora.org>
> Cc: Vinod Koul <vkoul@kernel.org>
> Fixes: 2317b87a2a6f ("arm64: dts: qcom: db820c: Add vdd_gfx and tie it into mmcc")
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> ---
> 
> Hi Andy/Bjorn/all, this patch was tested in 5.14, but I've tested it
> in the linux-next tree as well and was able to apply there cleanly.
> I'm new in the DTS world, so my apologies in advance for any rookie
> mistake - suggestions are appreciated! Thanks in advance for the review,
> 

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!

> 
> Guilherme
> 
> 
>  arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
> index 51e17094d7b1..977842068619 100644
> --- a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
> +++ b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
> @@ -699,8 +699,11 @@ &pmi8994_spmi_regulators {
>  	vdd_gfx: s2@1700 {
>  		reg = <0x1700 0x100>;
>  		regulator-name = "VDD_GFX";
> -		regulator-min-microvolt = <980000>;
> -		regulator-max-microvolt = <980000>;
> +		regulator-min-microvolt = <350000>;
> +		regulator-max-microvolt = <1350000>;
> +		regulator-always-on;
> +		status = "okay";

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?

Regards,
Bjorn

>  	};
>  };
>  
> -- 
> 2.33.0
> 

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

[Guilherme G. Piccoli]

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

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

[Bjorn Andersson]

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