| Message ID | 0718feb8e95344a0b615f61e6d909f6e105e3bf9.1731264205.git.dsimic@manjaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: rockchip: Fix vdd_gpu voltage constraints on PinePhone Pro | expand |
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; >
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;
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,"
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,"
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
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
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;
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;
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;
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;
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(-)