Message-ID: <1663957A755BE820+acc57e45-954f-4b33-90fb-47f97815db96@radxa.com>
Date: Wed, 23 Oct 2024 08:30:28 +0900
From: FUKAUMI Naoki <naoki@...xa.com>
To: Dragan Simic <dsimic@...jaro.org>
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, TL Lim <tllim@...e64.org>,
 Marek Kraus <gamiee@...e64.org>, Tom Cubie <tom@...xa.com>,
 Nicolas Frattaroli <frattaroli.nicolas@...il.com>,
 Jonas Karlman <jonas@...boo.se>
Subject: Re: [PATCH 3/3] arm64: dts: rockchip: Add new SoC dtsi for the
 RK3566T variant

Hi Dragan Simic,

On 10/23/24 05:13, Dragan Simic wrote:
> Hello Fukaumi and Tom,
> 
> On 2024-10-14 07:16, Dragan Simic wrote:
>> On 2024-10-14 06:38, FUKAUMI Naoki wrote:
>>> On 10/13/24 02:04, Dragan Simic wrote:
>>>> Add new SoC dtsi file for the RK3566T variant of the Rockchip RK3566 
>>>> SoC.
>>>> The difference between the RK3566T variant and the "full-fat" RK3566 
>>>> variant
>>>> is in fewer supported CPU and GPU OPPs on the RK3566T, and in the 
>>>> absence of
>>>> a functional NPU, which we currently don't have to worry about.
>>>>
>>>> Examples of the boards based on the RK3566T include the Pine64 
>>>> Quartz64 Zero
>>>> SBC, [2] the Radxa ROCK 3C and the Radxa ZERO 3E/3W SBCs. 
>>>> Unfortunately,
>>>> Radxa doesn't mention the use of RK3566T officially, but its 
>>>> official SBC
>>>> specifications do state that the maximum frequency for the 
>>>> Cortex-A55 cores
>>>> on those SBCs is lower than the "full-fat" RK3566's 1.8 GHz, which 
>>>> makes
>>>> spotting the presence of the RK3566T SoC variant rather easy. 
>>>> [3][4][5]  An
>>>> additional, helpful cue is that Radxa handles the CPU and GPU OPPs 
>>>> for the
>>>> RK3566T variant separately in its downstream kernel. [6]
>>>>
>>>> The CPU and GPU OPPs supported on the RK3566T SoC variant are taken 
>>>> from the
>>>> vendor kernel source, [1] which uses the values of the 
>>>> "opp-supported-hw" OPP
>>>> properties to determine which ones are supported on a particular SoC 
>>>> variant.
>>>> The actual values of the "opp-supported-hw" properties make it 
>>>> rather easy
>>>> to see what OPPs are supported on the RK3566T SoC variant, but that, 
>>>> rather
>>>> unfortunately, clashes with the maximum frequencies advertised 
>>>> officially
>>>> for the Cortex-A55 CPU cores on the above-mentioned SBCs. 
>>>> [2][3][4][5]  The
>>>> vendor kernel source indicates that the maximum frequency for the 
>>>> CPU cores
>>>> is 1.4 GHz, while the SBC specifications state that to be 1.6 GHz. 
>>>> Unless
>>>> that discrepancy is resolved somehow, let's take the safe approach 
>>>> and use
>>>> the lower maximum frequency for the CPU cores.
>>>>
>>>> Update the dts files of the currently supported RK3566T-based boards 
>>>> to use
>>>> the new SoC dtsi for the RK3566T variant.  This actually takes the 
>>>> CPU cores
>>>> and the GPUs found on these boards out of their earlier overclocks, 
>>>> but it
>>>> also means that the officially advertised specifications 
>>>> [2][3][4][5] of the
>>>> highest supported frequencies for the Cortex-A55 CPU cores on these 
>>>> boards
>>>> may actually be wrong, as already explained above.
>>>>
>>>> The correctness of the introduced changes was validated by 
>>>> decompiling and
>>>> comparing all affected board dtb files before and after these changes.
>>>>
>>>> [1] 
>>>> https://raw.githubusercontent.com/rockchip-linux/kernel/f8b9431ee38ed561650be7092ab93f564598daa9/arch/arm64/boot/dts/rockchip/rk3568.dtsi
>>>> [2] https://wiki.pine64.org/wiki/Quartz64
>>>> [3] 
>>>> https://dl.radxa.com/rock3/docs/hw/3c/radxa_rock3c_product_brief.pdf
>>>> [4] 
>>>> https://dl.radxa.com/zero3/docs/hw/3e/radxa_zero_3e_product_brief.pdf
>>>> [5] 
>>>> https://dl.radxa.com/zero3/docs/hw/3w/radxa_zero_3w_product_brief.pdf
>>>> [6] 
>>>> https://github.com/radxa/kernel/commit/2dfd51da472e7ebb5ef0d3db78f902454af826b8
>>>>
>>>> Cc: TL Lim <tllim@...e64.org>
>>>> Cc: Marek Kraus <gamiee@...e64.org>
>>>> Cc: Tom Cubie <tom@...xa.com>
>>>> Cc: FUKAUMI Naoki <naoki@...xa.com>
>>>> Helped-by: Nicolas Frattaroli <frattaroli.nicolas@...il.com>
>>>> Helped-by: Jonas Karlman <jonas@...boo.se>
>>>> Signed-off-by: Dragan Simic <dsimic@...jaro.org>
>>>> ---
>>>>   .../dts/rockchip/rk3566-radxa-zero-3.dtsi     |  2 +-
>>>>   .../boot/dts/rockchip/rk3566-rock-3c.dts      |  2 +-
>>>>   arch/arm64/boot/dts/rockchip/rk3566t.dtsi     | 90 
>>>> +++++++++++++++++++
>>>>   3 files changed, 92 insertions(+), 2 deletions(-)
>>>>   create mode 100644 arch/arm64/boot/dts/rockchip/rk3566t.dtsi
>>>>
>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3566-radxa-zero-3.dtsi 
>>>> b/arch/arm64/boot/dts/rockchip/rk3566-radxa-zero-3.dtsi
>>>> index de390d92c35e..1ee5d96a46a1 100644
>>>> --- a/arch/arm64/boot/dts/rockchip/rk3566-radxa-zero-3.dtsi
>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3566-radxa-zero-3.dtsi
>>>> @@ -3,7 +3,7 @@
>>>>   #include <dt-bindings/gpio/gpio.h>
>>>>   #include <dt-bindings/leds/common.h>
>>>>   #include <dt-bindings/soc/rockchip,vop2.h>
>>>> -#include "rk3566.dtsi"
>>>> +#include "rk3566t.dtsi"
>>>
>>> could you drop this change for now?
>>
>> This patch is also going to be used for the upcoming board dts
>> for the Pine64 Quartz64 Zero, so there's no need for dropping it.
>> The Quartz64 Zero definitely uses the RK3566T.
>>
>>> We (Radxa) think we use RK3566.
>>
>> Well, the available documentation for the Radxa ROCK 3C and ZERO
>> 3E/3W boards doesn't say so; instead, everything points to the
>> RK3566T being used.  The referenced commit in the Radxa downstream
>> kernel also indicates that RK3566T is used at least on some boards.
>>
>> Also, some independent testing, by reading the efuses, has showed
>> that at least some ROCK 3C and ZERO 3E/3W boards actually have the
>> RK3566T, which means that we should handle them all as having the
>> RK3566T, to avoid overclocking the CPU cores and the GPU.  I'll
>> get back to this below.
>>
>>> and vendor kernel[6] refers efuse value to determine it's -T or not.
>>> can you do similar way?
>>
>> Unfortunately not at the moment, because that would require major
>> changes to the way OPPs are handled in the upstream kernel.  Maybe
>> we can do that at some point in the future, as part of my planned
>> work on supporting SoC binning.
>>
>> With that in place, hopefully, we could handle any ROCK 3C and ZERO
>> 3E/3W boards that actually have the "full-fat" RK3566 variant as
>> such, but until then, it's much safer to treat them all as having
>> the RK3566T, and avoid the possible overclocking.
> 
> Just checking, and having the subsequent discussion on IRC in mind,
> are you fine with the above-proposed approach?  Please let me know
> if some further clarification is needed.

we have no objection. please do safer way.

Best regards,

--
FUKAUMI Naoki
Radxa Computer (Shenzhen) Co., Ltd.

>>>>   / {
>>>>       chosen {
>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3566-rock-3c.dts 
>>>> b/arch/arm64/boot/dts/rockchip/rk3566-rock-3c.dts
>>>> index f2cc086e5001..9a8f4f774dbc 100644
>>>> --- a/arch/arm64/boot/dts/rockchip/rk3566-rock-3c.dts
>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3566-rock-3c.dts
>>>> @@ -5,7 +5,7 @@
>>>>   #include <dt-bindings/leds/common.h>
>>>>   #include <dt-bindings/pinctrl/rockchip.h>
>>>>   #include <dt-bindings/soc/rockchip,vop2.h>
>>>> -#include "rk3566.dtsi"
>>>> +#include "rk3566t.dtsi"
>>>
>>> same here.
>>>
>>>>   / {
>>>>       model = "Radxa ROCK 3C";
>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3566t.dtsi 
>>>> b/arch/arm64/boot/dts/rockchip/rk3566t.dtsi
>>>> new file mode 100644
>>>> index 000000000000..cd89bd3b125b
>>>> --- /dev/null
>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3566t.dtsi
>>>> @@ -0,0 +1,90 @@
>>>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>>>> +
>>>> +#include "rk3566-base.dtsi"
>>>> +
>>>> +/ {
>>>> +    cpu0_opp_table: opp-table-0 {
>>>> +        compatible = "operating-points-v2";
>>>> +        opp-shared;
>>>> +
>>>> +        opp-408000000 {
>>>> +            opp-hz = /bits/ 64 <408000000>;
>>>> +            opp-microvolt = <850000 850000 1150000>;
>>>> +            clock-latency-ns = <40000>;
>>>> +        };
>>>> +
>>>> +        opp-600000000 {
>>>> +            opp-hz = /bits/ 64 <600000000>;
>>>> +            opp-microvolt = <850000 850000 1150000>;
>>>> +            clock-latency-ns = <40000>;
>>>> +        };
>>>> +
>>>> +        opp-816000000 {
>>>> +            opp-hz = /bits/ 64 <816000000>;
>>>> +            opp-microvolt = <850000 850000 1150000>;
>>>> +            clock-latency-ns = <40000>;
>>>> +            opp-suspend;
>>>> +        };
>>>> +
>>>> +        opp-1104000000 {
>>>> +            opp-hz = /bits/ 64 <1104000000>;
>>>> +            opp-microvolt = <900000 900000 1150000>;
>>>> +            clock-latency-ns = <40000>;
>>>> +        };
>>>> +
>>>> +        opp-1416000000 {
>>>> +            opp-hz = /bits/ 64 <1416000000>;
>>>> +            opp-microvolt = <1025000 1025000 1150000>;
>>>> +            clock-latency-ns = <40000>;
>>>> +        };
>>>> +    };
>>>> +
>>>> +    gpu_opp_table: opp-table-1 {
>>>> +        compatible = "operating-points-v2";
>>>> +
>>>> +        opp-200000000 {
>>>> +            opp-hz = /bits/ 64 <200000000>;
>>>> +            opp-microvolt = <850000 850000 1000000>;
>>>> +        };
>>>> +
>>>> +        opp-300000000 {
>>>> +            opp-hz = /bits/ 64 <300000000>;
>>>> +            opp-microvolt = <850000 850000 1000000>;
>>>> +        };
>>>> +
>>>> +        opp-400000000 {
>>>> +            opp-hz = /bits/ 64 <400000000>;
>>>> +            opp-microvolt = <850000 850000 1000000>;
>>>> +        };
>>>> +
>>>> +        opp-600000000 {
>>>> +            opp-hz = /bits/ 64 <600000000>;
>>>> +            opp-microvolt = <900000 900000 1000000>;
>>>> +        };
>>>> +
>>>> +        opp-700000000 {
>>>> +            opp-hz = /bits/ 64 <700000000>;
>>>> +            opp-microvolt = <950000 950000 1000000>;
>>>> +        };
>>>> +    };
>>>> +};
>>>> +
>>>> +&cpu0 {
>>>> +    operating-points-v2 = <&cpu0_opp_table>;
>>>> +};
>>>> +
>>>> +&cpu1 {
>>>> +    operating-points-v2 = <&cpu0_opp_table>;
>>>> +};
>>>> +
>>>> +&cpu2 {
>>>> +    operating-points-v2 = <&cpu0_opp_table>;
>>>> +};
>>>> +
>>>> +&cpu3 {
>>>> +    operating-points-v2 = <&cpu0_opp_table>;
>>>> +};
>>>> +
>>>> +&gpu {
>>>> +    operating-points-v2 = <&gpu_opp_table>;
>>>> +};
> 

Powered by blists - more mailing lists