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@xxxxxxxxxxxxxx/
[**]
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@xxxxxxxxxxxxxxx
Signed-off-by: Dragan Simic <dsimic@xxxxxxxxxxx>
---
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;
