Message-ID: <4ba2d58cf1da5b90930b630ba0978e2c@manjaro.org>
Date: Wed, 13 Nov 2024 00:12:12 +0100
From: Dragan Simic <dsimic@...jaro.org>
To: Robin Murphy <robin.murphy@....com>
Cc: linux-rockchip@...ts.infradead.org, heiko@...ech.de,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, robh@...nel.org, krzk+dt@...nel.org,
 conor+dt@...nel.org, stable@...r.kernel.org
Subject: Re: [PATCH] arm64: dts: rockchip: Fix vdd_gpu voltage constraints on
 PinePhone Pro

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@...r.kernel.org
>>>> Signed-off-by: Dragan Simic <dsimic@...jaro.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;

Powered by blists - more mailing lists