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

[Robin Murphy <robin.murphy@arm.com>]

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;