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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fdf58f3e9fcb4c672a4bb114fbdab60d@manjaro.org>
Date: Tue, 12 Nov 2024 15:36:41 +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 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@...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;