[PATCH] arm64: dts: rockchip: Fix vdd_gpu voltage constraints on PinePhone Pro

[PATCH] arm64: dts: rockchip: Fix vdd_gpu voltage constraints on PinePhone Pro

[Dragan Simic]

The regulator-{min,max}-microvolt values for the vdd_gpu regulator in the
PinePhone Pro device dts file are too restrictive, which prevents the highest
GPU OPP from being used, slowing the GPU down unnecessarily.  Let's fix that
by making the regulator-{min,max}-microvolt values less strict, using the
voltage range that the Silergy SYR838 chip used for the vdd_gpu regulator is
actually capable of producing. [1][2]

This also eliminates the following error messages from the kernel log:

  core: _opp_supported_by_regulators: OPP minuV: 1100000 maxuV: 1150000, not supported by regulator
  panfrost ff9a0000.gpu: _opp_add: OPP not supported by regulators (800000000)

These changes to the regulator-{min,max}-microvolt values make the PinePhone
Pro device dts consistent with the dts files for other Rockchip RK3399-based
boards and devices.  It's possible to be more strict here, by specifying the
regulator-{min,max}-microvolt values that don't go outside of what the GPU
actually may use, as the consumer of the vdd_gpu regulator, but those changes
are left for a later directory-wide regulator cleanup.

[1] https://files.pine64.org/doc/PinePhonePro/PinephonePro-Schematic-V1.0-20211127.pdf
[2] https://www.t-firefly.com/download/Firefly-RK3399/docs/Chip%20Specifications/DC-DC_SYR837_838.pdf

Fixes: 78a21c7d5952 ("arm64: dts: rockchip: Add initial support for Pine64 PinePhone Pro")
Cc: stable@vger.kernel.org
Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
index 1a44582a49fb..956d64f5b271 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
@@ -410,8 +410,8 @@ vdd_gpu: regulator@41 {
 		pinctrl-names = "default";
 		pinctrl-0 = <&vsel2_pin>;
 		regulator-name = "vdd_gpu";
-		regulator-min-microvolt = <875000>;
-		regulator-max-microvolt = <975000>;
+		regulator-min-microvolt = <712500>;
+		regulator-max-microvolt = <1500000>;
 		regulator-ramp-delay = <1000>;
 		regulator-always-on;
 		regulator-boot-on;

Re: [PATCH] arm64: dts: rockchip: Fix vdd_gpu voltage constraints on PinePhone Pro

[Heiko Stübner]

Am Sonntag, 10. November 2024, 19:44:31 CET schrieb Dragan Simic:
> The regulator-{min,max}-microvolt values for the vdd_gpu regulator in the
> PinePhone Pro device dts file are too restrictive, which prevents the highest
> GPU OPP from being used, slowing the GPU down unnecessarily.  Let's fix that
> by making the regulator-{min,max}-microvolt values less strict, using the
> voltage range that the Silergy SYR838 chip used for the vdd_gpu regulator is
> actually capable of producing. [1][2]
> 
> This also eliminates the following error messages from the kernel log:
> 
>   core: _opp_supported_by_regulators: OPP minuV: 1100000 maxuV: 1150000, not supported by regulator
>   panfrost ff9a0000.gpu: _opp_add: OPP not supported by regulators (800000000)
> 
> These changes to the regulator-{min,max}-microvolt values make the PinePhone
> Pro device dts consistent with the dts files for other Rockchip RK3399-based
> boards and devices.  It's possible to be more strict here, by specifying the
> regulator-{min,max}-microvolt values that don't go outside of what the GPU
> actually may use, as the consumer of the vdd_gpu regulator, but those changes
> are left for a later directory-wide regulator cleanup.

With the Pinephone Pro using some sort of special-rk3399, how much of
"the soc variant cannot use the highest gpu opp" is in there, and just the
original implementation is wrong?

Did you run this on actual hardware?


Heiko


> Fixes: 78a21c7d5952 ("arm64: dts: rockchip: Add initial support for Pine64 PinePhone Pro")
> Cc: stable@vger.kernel.org
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---
>  arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> index 1a44582a49fb..956d64f5b271 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> @@ -410,8 +410,8 @@ vdd_gpu: regulator@41 {
>  		pinctrl-names = "default";
>  		pinctrl-0 = <&vsel2_pin>;
>  		regulator-name = "vdd_gpu";
> -		regulator-min-microvolt = <875000>;
> -		regulator-max-microvolt = <975000>;
> +		regulator-min-microvolt = <712500>;
> +		regulator-max-microvolt = <1500000>;
>  		regulator-ramp-delay = <1000>;
>  		regulator-always-on;
>  		regulator-boot-on;
> 

Re: [PATCH] arm64: dts: rockchip: Fix vdd_gpu voltage constraints on PinePhone Pro

[Dragan Simic]

Hello Heiko,

On 2024-11-10 21:08, Heiko Stübner wrote:
> Am Sonntag, 10. November 2024, 19:44:31 CET schrieb Dragan Simic:
>> The regulator-{min,max}-microvolt values for the vdd_gpu regulator in 
>> the
>> PinePhone Pro device dts file are too restrictive, which prevents the 
>> highest
>> GPU OPP from being used, slowing the GPU down unnecessarily.  Let's 
>> fix that
>> by making the regulator-{min,max}-microvolt values less strict, using 
>> the
>> voltage range that the Silergy SYR838 chip used for the vdd_gpu 
>> regulator is
>> actually capable of producing. [1][2]
>> 
>> This also eliminates the following error messages from the kernel log:
>> 
>>   core: _opp_supported_by_regulators: OPP minuV: 1100000 maxuV: 
>> 1150000, not supported by regulator
>>   panfrost ff9a0000.gpu: _opp_add: OPP not supported by regulators 
>> (800000000)
>> 
>> These changes to the regulator-{min,max}-microvolt values make the 
>> PinePhone
>> Pro device dts consistent with the dts files for other Rockchip 
>> RK3399-based
>> boards and devices.  It's possible to be more strict here, by 
>> specifying the
>> regulator-{min,max}-microvolt values that don't go outside of what the 
>> GPU
>> actually may use, as the consumer of the vdd_gpu regulator, but those 
>> changes
>> are left for a later directory-wide regulator cleanup.
> 
> With the Pinephone Pro using some sort of special-rk3399, how much of
> "the soc variant cannot use the highest gpu opp" is in there, and just 
> the
> original implementation is wrong?

Good question, I already asked it myself.  I'm unaware of any kind of
GPU-OPP-related restrictions when it comes to the PinePhone-Pro-specific
RK3399S.  Furthermore, "the word on the street" is that the RK3399S can
work perfectly fine even at the couple of "full-fat" RK3399 CPU OPPs
that are not defined for the RK3399S, and the only result would be the
expected higher power consumption and a bit more heat generated.

This just reaffirms that no known GPU OPP restrictions exist.  Even
if they existed, enforcing them _primarily_ through the constraints of
the associated voltage regulator would be the wrong approach.  Instead,
the restrictions should be defined primarily through the per-SoC-variant
GPU OPPs, which are, to my best knowledge, not known to be existing for
the RK3399S SoC variant.

> Did you run this on actual hardware?

I rushed a bit to submit the patch before being able to test in on the
actual hardware, but there's already one person willing to test the 
patch
on their PinePhone Pro and provide their Tested-By.  I see no reasons 
why
it shouldn't work as expected, as explained above, which is why I 
decided
it's safe to submit the patch before detailed testing.

I'm very careful when it comes to changes like this one, but I'm quite
confident there should be no issues, just a nice performance boost. :)
I also checked and compared the schematics of the PinePhone Pro and
a couple of other Pine64 RK3399-based boards and devices, to make sure
there are no differences in the GPU regulators that would make the
PinePhone Pro an exception.  I saw no such differences.

>> Fixes: 78a21c7d5952 ("arm64: dts: rockchip: Add initial support for 
>> Pine64 PinePhone Pro")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>> ---
>>  arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts 
>> b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
>> index 1a44582a49fb..956d64f5b271 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
>> @@ -410,8 +410,8 @@ vdd_gpu: regulator@41 {
>>  		pinctrl-names = "default";
>>  		pinctrl-0 = <&vsel2_pin>;
>>  		regulator-name = "vdd_gpu";
>> -		regulator-min-microvolt = <875000>;
>> -		regulator-max-microvolt = <975000>;
>> +		regulator-min-microvolt = <712500>;
>> +		regulator-max-microvolt = <1500000>;
>>  		regulator-ramp-delay = <1000>;
>>  		regulator-always-on;
>>  		regulator-boot-on;

Re: [PATCH] arm64: dts: rockchip: Fix vdd_gpu voltage constraints on PinePhone Pro

[Heiko Stübner]

Am Sonntag, 10. November 2024, 21:47:15 CET schrieb Dragan Simic:
> Hello Heiko,
> 
> On 2024-11-10 21:08, Heiko Stübner wrote:
> > Am Sonntag, 10. November 2024, 19:44:31 CET schrieb Dragan Simic:
> >> The regulator-{min,max}-microvolt values for the vdd_gpu regulator in 
> >> the
> >> PinePhone Pro device dts file are too restrictive, which prevents the 
> >> highest
> >> GPU OPP from being used, slowing the GPU down unnecessarily.  Let's 
> >> fix that
> >> by making the regulator-{min,max}-microvolt values less strict, using 
> >> the
> >> voltage range that the Silergy SYR838 chip used for the vdd_gpu 
> >> regulator is
> >> actually capable of producing. [1][2]
> >> 
> >> This also eliminates the following error messages from the kernel log:
> >> 
> >>   core: _opp_supported_by_regulators: OPP minuV: 1100000 maxuV: 
> >> 1150000, not supported by regulator
> >>   panfrost ff9a0000.gpu: _opp_add: OPP not supported by regulators 
> >> (800000000)
> >> 
> >> These changes to the regulator-{min,max}-microvolt values make the 
> >> PinePhone
> >> Pro device dts consistent with the dts files for other Rockchip 
> >> RK3399-based
> >> boards and devices.  It's possible to be more strict here, by 
> >> specifying the
> >> regulator-{min,max}-microvolt values that don't go outside of what the 
> >> GPU
> >> actually may use, as the consumer of the vdd_gpu regulator, but those 
> >> changes
> >> are left for a later directory-wide regulator cleanup.
> > 
> > With the Pinephone Pro using some sort of special-rk3399, how much of
> > "the soc variant cannot use the highest gpu opp" is in there, and just 
> > the
> > original implementation is wrong?
> 
> Good question, I already asked it myself.  I'm unaware of any kind of
> GPU-OPP-related restrictions when it comes to the PinePhone-Pro-specific
> RK3399S.  Furthermore, "the word on the street" is that the RK3399S can
> work perfectly fine even at the couple of "full-fat" RK3399 CPU OPPs
> that are not defined for the RK3399S, and the only result would be the
> expected higher power consumption and a bit more heat generated.

In the past we already had people submit higher cpu OPPs with the
reasoning "the cpu runs fine with it", but which where outside of the
officially specified frequencies and were essentially overclocking the
CPU cores and thus possibly reducing its lifetime.

So "it runs fine" is a bit of thin argument ;-) . I guess for the gpu it
might not matter too much, compared to the cpu cores, but I still like
the safe sides - especially for the mainline sources.

I guess we'll wait for people to test the change and go from there ;-) .


> This just reaffirms that no known GPU OPP restrictions exist.  Even
> if they existed, enforcing them _primarily_ through the constraints of
> the associated voltage regulator would be the wrong approach.  Instead,
> the restrictions should be defined primarily through the per-SoC-variant
> GPU OPPs, which are, to my best knowledge, not known to be existing for
> the RK3399S SoC variant.

Yes, that is what I was getting at, if that is a limiting implementation
it is of course not done correctly, but I'd like to make sure.

Of course Pine's development model doesn't help at all in that regard.
There isn't even a "vendor" kernel source it seems. [0]


Heiko


[0] https://wiki.pine64.org/wiki/PinePhone_Pro_Development#Kernel
states "There's no canonical location for Pinephone Pro Linux kernel development,"

Re: [PATCH] arm64: dts: rockchip: Fix vdd_gpu voltage constraints on PinePhone Pro

[Dragan Simic]

On 2024-11-10 22:16, Heiko Stübner wrote:
> Am Sonntag, 10. November 2024, 21:47:15 CET schrieb Dragan Simic:
>> On 2024-11-10 21:08, Heiko Stübner wrote:
>> > Am Sonntag, 10. November 2024, 19:44:31 CET schrieb Dragan Simic:
>> >> The regulator-{min,max}-microvolt values for the vdd_gpu regulator in
>> >> the
>> >> PinePhone Pro device dts file are too restrictive, which prevents the
>> >> highest
>> >> GPU OPP from being used, slowing the GPU down unnecessarily.  Let's
>> >> fix that
>> >> by making the regulator-{min,max}-microvolt values less strict, using
>> >> the
>> >> voltage range that the Silergy SYR838 chip used for the vdd_gpu
>> >> regulator is
>> >> actually capable of producing. [1][2]
>> >>
>> >> This also eliminates the following error messages from the kernel log:
>> >>
>> >>   core: _opp_supported_by_regulators: OPP minuV: 1100000 maxuV:
>> >> 1150000, not supported by regulator
>> >>   panfrost ff9a0000.gpu: _opp_add: OPP not supported by regulators
>> >> (800000000)
>> >>
>> >> These changes to the regulator-{min,max}-microvolt values make the
>> >> PinePhone
>> >> Pro device dts consistent with the dts files for other Rockchip
>> >> RK3399-based
>> >> boards and devices.  It's possible to be more strict here, by
>> >> specifying the
>> >> regulator-{min,max}-microvolt values that don't go outside of what the
>> >> GPU
>> >> actually may use, as the consumer of the vdd_gpu regulator, but those
>> >> changes
>> >> are left for a later directory-wide regulator cleanup.
>> >
>> > With the Pinephone Pro using some sort of special-rk3399, how much of
>> > "the soc variant cannot use the highest gpu opp" is in there, and just
>> > the
>> > original implementation is wrong?
>> 
>> Good question, I already asked it myself.  I'm unaware of any kind of
>> GPU-OPP-related restrictions when it comes to the 
>> PinePhone-Pro-specific
>> RK3399S.  Furthermore, "the word on the street" is that the RK3399S 
>> can
>> work perfectly fine even at the couple of "full-fat" RK3399 CPU OPPs
>> that are not defined for the RK3399S, and the only result would be the
>> expected higher power consumption and a bit more heat generated.
> 
> In the past we already had people submit higher cpu OPPs with the
> reasoning "the cpu runs fine with it", but which where outside of the
> officially specified frequencies and were essentially overclocking the
> CPU cores and thus possibly reducing its lifetime.

Sure, having higher-frequency OPPs working doesn't mean that's the way
the SoC is intended to be used.  It also doesn't mean that all samples
of the same SoC would work reliably with higher-frequency OPPs.

> So "it runs fine" is a bit of thin argument ;-) . I guess for the gpu 
> it
> might not matter too much, compared to the cpu cores, but I still like
> the safe sides - especially for the mainline sources.

Just to clarify, in this particular case the above-mentioned "word
on the street" came straight from TL Lim, the founder of Pine64, back
when we recently discussed what actually makes the RK3399S a special
variant of the RK3399.  He basically forwarded what Rockchip said him
about the RK3399S as a special variant.

One of the troubles, in this particular case, is there's no official
datasheet that describes the RK3399S, so it's all a bit up to "the
word on the street", I'm afraid.

> I guess we'll wait for people to test the change and go from there ;-) 
> .

Sure, but even with a few "tested, works for me" reports, we still
won't be able to stop relying on the above-described "word on the
street", simply because e.g. even CPU core overclocks, which would
of course be wrong, perhaps would work just fine for some people.
I hope I'm conveying this in an understandable way. :)

>> This just reaffirms that no known GPU OPP restrictions exist.  Even
>> if they existed, enforcing them _primarily_ through the constraints of
>> the associated voltage regulator would be the wrong approach.  
>> Instead,
>> the restrictions should be defined primarily through the 
>> per-SoC-variant
>> GPU OPPs, which are, to my best knowledge, not known to be existing 
>> for
>> the RK3399S SoC variant.
> 
> Yes, that is what I was getting at, if that is a limiting 
> implementation
> it is of course not done correctly, but I'd like to make sure.

Indeed, I'd also like to have it all checked as much as possible.
I'll try to extract the device dts from the test Android image that
was supposedly provided directly by Rockchip for the PinePhone Pro,
and check what's actually defined inside it.

> Of course Pine's development model doesn't help at all in that regard.
> There isn't even a "vendor" kernel source it seems. [0]

I see, it's a bit confusing, so I'll try to explain.  See, Pine64,
as an SBC and device manufacturer, basically has no official software
development model or an associated team.  Instead, the entire software
development, be it low-level or high-level software, is left to the
broader community made primarily of various individuals, who all have
different approaches to their work.

That's why I referred to "the word on the street" originally.  I hope
it all makes more sense now. :)

> [0] https://wiki.pine64.org/wiki/PinePhone_Pro_Development#Kernel
> states "There's no canonical location for Pinephone Pro Linux kernel
> development,"

Re: [PATCH] arm64: dts: rockchip: Fix vdd_gpu voltage constraints on PinePhone Pro

[Adam Pigg]

[-- Attachment #1: Type: text/plain, Size: 4211 bytes --]

On Sunday 10 November 2024 18:44:31 Greenwich Mean Time Dragan Simic wrote:
> The regulator-{min,max}-microvolt values for the vdd_gpu regulator in the
> PinePhone Pro device dts file are too restrictive, which prevents the
> highest GPU OPP from being used, slowing the GPU down unnecessarily.  Let's
> fix that by making the regulator-{min,max}-microvolt values less strict,
> using the voltage range that the Silergy SYR838 chip used for the vdd_gpu
> regulator is actually capable of producing. [1][2]
> 
> This also eliminates the following error messages from the kernel log:
> 
>   core: _opp_supported_by_regulators: OPP minuV: 1100000 maxuV: 1150000, not
> supported by regulator panfrost ff9a0000.gpu: _opp_add: OPP not supported
> by regulators (800000000)
> 
> These changes to the regulator-{min,max}-microvolt values make the PinePhone
> Pro device dts consistent with the dts files for other Rockchip
> RK3399-based boards and devices.  It's possible to be more strict here, by
> specifying the regulator-{min,max}-microvolt values that don't go outside
> of what the GPU actually may use, as the consumer of the vdd_gpu regulator,
> but those changes are left for a later directory-wide regulator cleanup.
> 
Ive tested this on my PPP and can provide the following outputs.

On a device without the patch:
# cat /sys/class/devfreq/ff9a0000.gpu/trans_stat
From  :   To
: 200000000 297000000 400000000 500000000 600000000   time(ms)
200000000:         0         0         0         0     12203   5387782
297000000:      8208         0         0         0      2973   2337382
400000000:       784      6283         0         0       439   1859857
500000000:        67       287      1093         0       284    389599
600000000:      3145      4611      6413      1730         0   1748889
Total transition : 48520

And with the patch:
[root@PinePhonePro defaultuser]# cat /sys/class/devfreq/ff9a0000.gpu/trans_stat 
     From  :   To
           : 200000000 297000000 400000000 500000000 600000000 800000000   
time(ms)
  200000000:         0         0         0         0         0       364    
188911
  297000000:       120         0         0         0         0       234     
31652
  400000000:        77       182         0         0         0        82     
32287
  500000000:        10        57        56         0         0        57     
13376
  600000000:        21        14        35        31         0        22      
9463
  800000000:       137       101       250       148       123         0     
97310
Total transition : 2121
[root@PinePhonePro defaultuser]# uptime
 11:56:24 up  3:34,  1 users,  load average: 2.77, 2.24, 1.70

I havnt noticed any issues, though I havnt done anything more in-depth than 
run the compositor and play a youtube video in the browser

> [1]
> https://files.pine64.org/doc/PinePhonePro/PinephonePro-Schematic-V1.0-20211
> 127.pdf [2]
> https://www.t-firefly.com/download/Firefly-RK3399/docs/Chip%20Specification
> s/DC-DC_SYR837_838.pdf
> 
> Fixes: 78a21c7d5952 ("arm64: dts: rockchip: Add initial support for Pine64
> PinePhone Pro") Cc: stable@vger.kernel.org
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
Tested-By: Adam Pigg <adam@piggz.co.uk>
> ---
>  arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts index
> 1a44582a49fb..956d64f5b271 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> @@ -410,8 +410,8 @@ vdd_gpu: regulator@41 {
>  		pinctrl-names = "default";
>  		pinctrl-0 = <&vsel2_pin>;
>  		regulator-name = "vdd_gpu";
> -		regulator-min-microvolt = <875000>;
> -		regulator-max-microvolt = <975000>;
> +		regulator-min-microvolt = <712500>;
> +		regulator-max-microvolt = <1500000>;
>  		regulator-ramp-delay = <1000>;
>  		regulator-always-on;
>  		regulator-boot-on;
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] arm64: dts: rockchip: Fix vdd_gpu voltage constraints on PinePhone Pro

[Robin Murphy]

On 10/11/2024 6:44 pm, Dragan Simic wrote:
> The regulator-{min,max}-microvolt values for the vdd_gpu regulator in the
> PinePhone Pro device dts file are too restrictive, which prevents the highest
> GPU OPP from being used, slowing the GPU down unnecessarily.  Let's fix that
> by making the regulator-{min,max}-microvolt values less strict, using the
> voltage range that the Silergy SYR838 chip used for the vdd_gpu regulator is
> actually capable of producing. [1][2]

Specifying the absolute limits which the regulator driver necessarily 
already knows doesn't seem particularly useful... Moreover, the RK3399 
datasheet specifies the operating range for GPU_VDD as 0.80-1.20V, so at 
the very least, allowing the regulator to go outside that range seems 
inadvisable. However there's a separate datasheet for the RK3399-T 
variant, which does specify this 875-975mV range and a maximum GPU clock 
of 600MHz, along with the same 1.5GHz max. Cortex-A72 clock as 
advertised for RK3399S, so it seems quite possible that these GPU 
constraints here are in fact intentional as well. Obviously users are 
free to overclock and overvolt if they wish - I do for my 
actively-cooled RK3399 board :) - but it's a different matter for 
mainline to force it upon them.

Thanks,
Robin.

> This also eliminates the following error messages from the kernel log:
> 
>    core: _opp_supported_by_regulators: OPP minuV: 1100000 maxuV: 1150000, not supported by regulator
>    panfrost ff9a0000.gpu: _opp_add: OPP not supported by regulators (800000000)
> 
> These changes to the regulator-{min,max}-microvolt values make the PinePhone
> Pro device dts consistent with the dts files for other Rockchip RK3399-based
> boards and devices.  It's possible to be more strict here, by specifying the
> regulator-{min,max}-microvolt values that don't go outside of what the GPU
> actually may use, as the consumer of the vdd_gpu regulator, but those changes
> are left for a later directory-wide regulator cleanup.
> 
> [1] https://files.pine64.org/doc/PinePhonePro/PinephonePro-Schematic-V1.0-20211127.pdf
> [2] https://www.t-firefly.com/download/Firefly-RK3399/docs/Chip%20Specifications/DC-DC_SYR837_838.pdf
> 
> Fixes: 78a21c7d5952 ("arm64: dts: rockchip: Add initial support for Pine64 PinePhone Pro")
> Cc: stable@vger.kernel.org
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---
>   arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> index 1a44582a49fb..956d64f5b271 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> @@ -410,8 +410,8 @@ vdd_gpu: regulator@41 {
>   		pinctrl-names = "default";
>   		pinctrl-0 = <&vsel2_pin>;
>   		regulator-name = "vdd_gpu";
> -		regulator-min-microvolt = <875000>;
> -		regulator-max-microvolt = <975000>;
> +		regulator-min-microvolt = <712500>;
> +		regulator-max-microvolt = <1500000>;
>   		regulator-ramp-delay = <1000>;
>   		regulator-always-on;
>   		regulator-boot-on;
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] arm64: dts: rockchip: Fix vdd_gpu voltage constraints on PinePhone Pro

[Dragan Simic]

Hello Robin,

On 2024-11-12 15:19, Robin Murphy wrote:
> On 10/11/2024 6:44 pm, Dragan Simic wrote:
>> The regulator-{min,max}-microvolt values for the vdd_gpu regulator in 
>> the
>> PinePhone Pro device dts file are too restrictive, which prevents the 
>> highest
>> GPU OPP from being used, slowing the GPU down unnecessarily.  Let's 
>> fix that
>> by making the regulator-{min,max}-microvolt values less strict, using 
>> the
>> voltage range that the Silergy SYR838 chip used for the vdd_gpu 
>> regulator is
>> actually capable of producing. [1][2]
> 
> Specifying the absolute limits which the regulator driver necessarily
> already knows doesn't seem particularly useful... Moreover, the RK3399
> datasheet specifies the operating range for GPU_VDD as 0.80-1.20V, so
> at the very least, allowing the regulator to go outside that range
> seems inadvisable.

Indeed, which is why I already mentioned in the patch description
that I do plan to update the constraints of all regulators to match
the summary of the constraints of their consumers.  Though, I plan
to do that later, as a separate directory-wide cleanup, for which
I must find and allocate a substantial amount of time, to make sure
there will be no mistakes.

> However there's a separate datasheet for the
> RK3399-T variant, which does specify this 875-975mV range and a
> maximum GPU clock of 600MHz, along with the same 1.5GHz max.
> Cortex-A72 clock as advertised for RK3399S, so it seems quite possible
> that these GPU constraints here are in fact intentional as well.
> Obviously users are free to overclock and overvolt if they wish - I do
> for my actively-cooled RK3399 board :) - but it's a different matter
> for mainline to force it upon them.

Well, maybe the RK3399S is the same in that regard as the RK3399-T,
but maybe it actually isn't -- unfortunately, we don't have some
official RK3399S datasheet that would provide us with the required
information.  As another, somewhat unrelated example, we don't have
some official documentation to tell us is the RK3399S supposed not
to have working PCI Express interface, which officially isn't present
in the RK3399-T variant.

However, I fully agree that forcing any kind of an overclock is not
what we want to do.  Thus, I'll do my best, as I already noted in this
thread, to extract the dtb from the "reference" Android build that
Rockchip itself provided for the RK3399S-based PinePhone Pro.  That's
closest to the official documentation for the RK3399S variant that we
can get our hands on.

>> This also eliminates the following error messages from the kernel log:
>> 
>>    core: _opp_supported_by_regulators: OPP minuV: 1100000 maxuV: 
>> 1150000, not supported by regulator
>>    panfrost ff9a0000.gpu: _opp_add: OPP not supported by regulators 
>> (800000000)
>> 
>> These changes to the regulator-{min,max}-microvolt values make the 
>> PinePhone
>> Pro device dts consistent with the dts files for other Rockchip 
>> RK3399-based
>> boards and devices.  It's possible to be more strict here, by 
>> specifying the
>> regulator-{min,max}-microvolt values that don't go outside of what the 
>> GPU
>> actually may use, as the consumer of the vdd_gpu regulator, but those 
>> changes
>> are left for a later directory-wide regulator cleanup.
>> 
>> [1] 
>> https://files.pine64.org/doc/PinePhonePro/PinephonePro-Schematic-V1.0-20211127.pdf
>> [2] 
>> https://www.t-firefly.com/download/Firefly-RK3399/docs/Chip%20Specifications/DC-DC_SYR837_838.pdf
>> 
>> Fixes: 78a21c7d5952 ("arm64: dts: rockchip: Add initial support for 
>> Pine64 PinePhone Pro")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>> ---
>>   arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts 
>> b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
>> index 1a44582a49fb..956d64f5b271 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
>> @@ -410,8 +410,8 @@ vdd_gpu: regulator@41 {
>>   		pinctrl-names = "default";
>>   		pinctrl-0 = <&vsel2_pin>;
>>   		regulator-name = "vdd_gpu";
>> -		regulator-min-microvolt = <875000>;
>> -		regulator-max-microvolt = <975000>;
>> +		regulator-min-microvolt = <712500>;
>> +		regulator-max-microvolt = <1500000>;
>>   		regulator-ramp-delay = <1000>;
>>   		regulator-always-on;
>>   		regulator-boot-on;

Re: [PATCH] arm64: dts: rockchip: Fix vdd_gpu voltage constraints on PinePhone Pro

[Robin Murphy]

On 12/11/2024 2:36 pm, Dragan Simic wrote:
> Hello Robin,
> 
> On 2024-11-12 15:19, Robin Murphy wrote:
>> On 10/11/2024 6:44 pm, Dragan Simic wrote:
>>> The regulator-{min,max}-microvolt values for the vdd_gpu regulator in 
>>> the
>>> PinePhone Pro device dts file are too restrictive, which prevents the 
>>> highest
>>> GPU OPP from being used, slowing the GPU down unnecessarily.  Let's 
>>> fix that
>>> by making the regulator-{min,max}-microvolt values less strict, using 
>>> the
>>> voltage range that the Silergy SYR838 chip used for the vdd_gpu 
>>> regulator is
>>> actually capable of producing. [1][2]
>>
>> Specifying the absolute limits which the regulator driver necessarily
>> already knows doesn't seem particularly useful... Moreover, the RK3399
>> datasheet specifies the operating range for GPU_VDD as 0.80-1.20V, so
>> at the very least, allowing the regulator to go outside that range
>> seems inadvisable.
> 
> Indeed, which is why I already mentioned in the patch description
> that I do plan to update the constraints of all regulators to match
> the summary of the constraints of their consumers.  Though, I plan
> to do that later, as a separate directory-wide cleanup, for which
> I must find and allocate a substantial amount of time, to make sure
> there will be no mistakes.

Sure, but even if every other DT needs fixing, that still doesn't make 
it a good idea to deliberately introduce the same mistake to *this* DT 
and thus create even more work to fix it again. There's no value in 
being consistently wrong over inconsistently wrong - if there's 
justification for changing this DT at all, change it to be right.

>> However there's a separate datasheet for the
>> RK3399-T variant, which does specify this 875-975mV range and a
>> maximum GPU clock of 600MHz, along with the same 1.5GHz max.
>> Cortex-A72 clock as advertised for RK3399S, so it seems quite possible
>> that these GPU constraints here are in fact intentional as well.
>> Obviously users are free to overclock and overvolt if they wish - I do
>> for my actively-cooled RK3399 board :) - but it's a different matter
>> for mainline to force it upon them.
> 
> Well, maybe the RK3399S is the same in that regard as the RK3399-T,
> but maybe it actually isn't -- unfortunately, we don't have some
> official RK3399S datasheet that would provide us with the required
> information.  As another, somewhat unrelated example, we don't have
> some official documentation to tell us is the RK3399S supposed not
> to have working PCI Express interface, which officially isn't present
> in the RK3399-T variant.

Looking back at the original submission, v2 *was* proposing the RK3399-T 
OPPs, with the GPU capped at 600MHz, and it was said that those are what 
PPP *should* be using[1]. It seems there was a semantic objection to 
having a separate rk3399-t-opp.dtsi at the time, and when the main DTS 
was reworked for v3 the 800MHz GPU OPP seems to have been overlooked. 
However, since rk3399-t.dtsi does now exist anyway, it would seem more 
logical to just use that instead of including rk3399.dtsi and then 
overriding it to be pretty much equivalent to the T variant anyway.

Thanks,
Robin.

[1] 
https://lore.kernel.org/linux-rockchip/CAN1fySWVVTeGHAD=_hFH+ZdcR_AEiBc0wqes9Y4VRzB=zcdvSw@mail.gmail.com/

> However, I fully agree that forcing any kind of an overclock is not
> what we want to do.  Thus, I'll do my best, as I already noted in this
> thread, to extract the dtb from the "reference" Android build that
> Rockchip itself provided for the RK3399S-based PinePhone Pro.  That's
> closest to the official documentation for the RK3399S variant that we
> can get our hands on.
> 
>>> This also eliminates the following error messages from the kernel log:
>>>
>>>    core: _opp_supported_by_regulators: OPP minuV: 1100000 maxuV: 
>>> 1150000, not supported by regulator
>>>    panfrost ff9a0000.gpu: _opp_add: OPP not supported by regulators 
>>> (800000000)
>>>
>>> These changes to the regulator-{min,max}-microvolt values make the 
>>> PinePhone
>>> Pro device dts consistent with the dts files for other Rockchip 
>>> RK3399-based
>>> boards and devices.  It's possible to be more strict here, by 
>>> specifying the
>>> regulator-{min,max}-microvolt values that don't go outside of what 
>>> the GPU
>>> actually may use, as the consumer of the vdd_gpu regulator, but those 
>>> changes
>>> are left for a later directory-wide regulator cleanup.
>>>
>>> [1] 
>>> https://files.pine64.org/doc/PinePhonePro/PinephonePro-Schematic-V1.0-20211127.pdf
>>> [2] 
>>> https://www.t-firefly.com/download/Firefly-RK3399/docs/Chip%20Specifications/DC-DC_SYR837_838.pdf
>>>
>>> Fixes: 78a21c7d5952 ("arm64: dts: rockchip: Add initial support for 
>>> Pine64 PinePhone Pro")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>>> ---
>>>   arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts 
>>> b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
>>> index 1a44582a49fb..956d64f5b271 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
>>> @@ -410,8 +410,8 @@ vdd_gpu: regulator@41 {
>>>           pinctrl-names = "default";
>>>           pinctrl-0 = <&vsel2_pin>;
>>>           regulator-name = "vdd_gpu";
>>> -        regulator-min-microvolt = <875000>;
>>> -        regulator-max-microvolt = <975000>;
>>> +        regulator-min-microvolt = <712500>;
>>> +        regulator-max-microvolt = <1500000>;
>>>           regulator-ramp-delay = <1000>;
>>>           regulator-always-on;
>>>           regulator-boot-on;

Re: [PATCH] arm64: dts: rockchip: Fix vdd_gpu voltage constraints on PinePhone Pro

[Dragan Simic]

Hello Robin,

On 2024-11-12 19:51, Robin Murphy wrote:
> On 12/11/2024 2:36 pm, Dragan Simic wrote:
>> On 2024-11-12 15:19, Robin Murphy wrote:
>>> On 10/11/2024 6:44 pm, Dragan Simic wrote:
>>>> The regulator-{min,max}-microvolt values for the vdd_gpu regulator 
>>>> in the
>>>> PinePhone Pro device dts file are too restrictive, which prevents 
>>>> the highest
>>>> GPU OPP from being used, slowing the GPU down unnecessarily.  Let's 
>>>> fix that
>>>> by making the regulator-{min,max}-microvolt values less strict, 
>>>> using the
>>>> voltage range that the Silergy SYR838 chip used for the vdd_gpu 
>>>> regulator is
>>>> actually capable of producing. [1][2]
>>> 
>>> Specifying the absolute limits which the regulator driver necessarily
>>> already knows doesn't seem particularly useful... Moreover, the 
>>> RK3399
>>> datasheet specifies the operating range for GPU_VDD as 0.80-1.20V, so
>>> at the very least, allowing the regulator to go outside that range
>>> seems inadvisable.
>> 
>> Indeed, which is why I already mentioned in the patch description
>> that I do plan to update the constraints of all regulators to match
>> the summary of the constraints of their consumers.  Though, I plan
>> to do that later, as a separate directory-wide cleanup, for which
>> I must find and allocate a substantial amount of time, to make sure
>> there will be no mistakes.
> 
> Sure, but even if every other DT needs fixing, that still doesn't make
> it a good idea to deliberately introduce the same mistake to *this* DT
> and thus create even more work to fix it again. There's no value in
> being consistently wrong over inconsistently wrong - if there's
> justification for changing this DT at all, change it to be right.

After thinking a bit more about it, I agree.  At least, setting the
voltage regulator constraints according to the constraints of its
consumer(s) in one place sets an example for what's to be done in
the future for the other voltage regulators.

>>> However there's a separate datasheet for the
>>> RK3399-T variant, which does specify this 875-975mV range and a
>>> maximum GPU clock of 600MHz, along with the same 1.5GHz max.
>>> Cortex-A72 clock as advertised for RK3399S, so it seems quite 
>>> possible
>>> that these GPU constraints here are in fact intentional as well.
>>> Obviously users are free to overclock and overvolt if they wish - I 
>>> do
>>> for my actively-cooled RK3399 board :) - but it's a different matter
>>> for mainline to force it upon them.
>> 
>> Well, maybe the RK3399S is the same in that regard as the RK3399-T,
>> but maybe it actually isn't -- unfortunately, we don't have some
>> official RK3399S datasheet that would provide us with the required
>> information.  As another, somewhat unrelated example, we don't have
>> some official documentation to tell us is the RK3399S supposed not
>> to have working PCI Express interface, which officially isn't present
>> in the RK3399-T variant.
> 
> Looking back at the original submission, v2 *was* proposing the
> RK3399-T OPPs, with the GPU capped at 600MHz, and it was said that
> those are what PPP *should* be using[*]. It seems there was a semantic
> objection to having a separate rk3399-t-opp.dtsi at the time, and when
> the main DTS was reworked for v3 the 800MHz GPU OPP seems to have been
> overlooked. However, since rk3399-t.dtsi does now exist anyway, it
> would seem more logical to just use that instead of including
> rk3399.dtsi and then overriding it to be pretty much equivalent to the
> T variant anyway.

Ah, I see, thanks for pointing this out.  With this in mind, I think
that the RK3399S is actually just the RK3399-T binned specifically
for lower leakage values and, as a result, lower power consumption.
I've already assumed so, but this reaffirms it.

Actually, there's now also the rk3399-s.dtsi, [**] in which I just
spotted a rather small, non-critical mistake that I made, for which
I'll send a separate patch later.

Anyway, the rk3399-t.dtsi, originally known as rk3399-t-opp.dtsi and
added in the commit 9176ba910ba0 (arm64: dts: rockchip: Add RK3399-T
OPP table, 2022-09-02) specifies a bit higher voltages for the OPPs
than those found in the rk3399-s.dtsi, which fits well together with
the above-described assumption that the RK3399S is actually just the
RK3399-T specifically binned for lower leakage values...

... which also means that the RK3399S's GPU is supposed to run at
the GPU OPPs _below_ 800 MHz, just like the RK3399-T, but at slightly
lower voltages specified in the rk3399-s.dtsi.

Let me dig out that Rockchip Android dtb for the PinePhone Pro that
I mentioned already, to provide the last piece of evidence, and I'll
come back with the v2 of this patch.

[*] 
https://lore.kernel.org/linux-rockchip/CAN1fySWVVTeGHAD=_hFH+ZdcR_AEiBc0wqes9Y4VRzB=zcdvSw@mail.gmail.com/
[**] 
https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git/commit/?h=for-next&id=f7f8ec7d8cef4cf62ee13b526d59438c23bbb34f

>> However, I fully agree that forcing any kind of an overclock is not
>> what we want to do.  Thus, I'll do my best, as I already noted in this
>> thread, to extract the dtb from the "reference" Android build that
>> Rockchip itself provided for the RK3399S-based PinePhone Pro.  That's
>> closest to the official documentation for the RK3399S variant that we
>> can get our hands on.
>> 
>>>> This also eliminates the following error messages from the kernel 
>>>> log:
>>>> 
>>>>    core: _opp_supported_by_regulators: OPP minuV: 1100000 maxuV: 
>>>> 1150000, not supported by regulator
>>>>    panfrost ff9a0000.gpu: _opp_add: OPP not supported by regulators 
>>>> (800000000)
>>>> 
>>>> These changes to the regulator-{min,max}-microvolt values make the 
>>>> PinePhone
>>>> Pro device dts consistent with the dts files for other Rockchip 
>>>> RK3399-based
>>>> boards and devices.  It's possible to be more strict here, by 
>>>> specifying the
>>>> regulator-{min,max}-microvolt values that don't go outside of what 
>>>> the GPU
>>>> actually may use, as the consumer of the vdd_gpu regulator, but 
>>>> those changes
>>>> are left for a later directory-wide regulator cleanup.
>>>> 
>>>> [1] 
>>>> https://files.pine64.org/doc/PinePhonePro/PinephonePro-Schematic-V1.0-20211127.pdf
>>>> [2] 
>>>> https://www.t-firefly.com/download/Firefly-RK3399/docs/Chip%20Specifications/DC-DC_SYR837_838.pdf
>>>> 
>>>> Fixes: 78a21c7d5952 ("arm64: dts: rockchip: Add initial support for 
>>>> Pine64 PinePhone Pro")
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>>>> ---
>>>>   arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts | 4 ++--
>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts 
>>>> b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
>>>> index 1a44582a49fb..956d64f5b271 100644
>>>> --- a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
>>>> @@ -410,8 +410,8 @@ vdd_gpu: regulator@41 {
>>>>           pinctrl-names = "default";
>>>>           pinctrl-0 = <&vsel2_pin>;
>>>>           regulator-name = "vdd_gpu";
>>>> -        regulator-min-microvolt = <875000>;
>>>> -        regulator-max-microvolt = <975000>;
>>>> +        regulator-min-microvolt = <712500>;
>>>> +        regulator-max-microvolt = <1500000>;
>>>>           regulator-ramp-delay = <1000>;
>>>>           regulator-always-on;
>>>>           regulator-boot-on;