| 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
| ||
|
Message-ID: <e03fec2aa3bedb4710b27717cb2394df@manjaro.org>
Date: Sat, 12 Oct 2024 22:01:27 +0200
From: Dragan Simic <dsimic@...jaro.org>
To: Diederik de Haas <didi.debian@...ow.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
Subject: Re: [PATCH 2/3] arm64: dts: rockchip: Prepare RK356x SoC dtsi files
for per-variant OPPs
Hello Diederik,
On 2024-10-12 21:41, Diederik de Haas wrote:
> On Sat Oct 12, 2024 at 7:04 PM CEST, Dragan Simic wrote:
>> Rename the Rockchip RK356x SoC dtsi files and, consequently, adjust
>> their
>> contents appropriately, to prepare them for the ability to specify
>> different
>> CPU and GPU OPPs for each of the supported RK356x SoC variants.
>>
>> The first new RK356x SoC variant to be introduced is the RK3566T,
>> which the
>> Pine64 Quartz64 Zero SBC is officially based on. [1] Some other SBCs
>> are
>> also based on the RK3566T variant, including Radxa ROCK 3C and ZERO
>> 3E/3W,
>> but the slight trouble is that Radxa doesn't state that officially.
>> Though,
>> it's rather easy to spot the RK3566T on such boards, because their
>> official
>> specifications state that the maximum frequency for the Cortex-A55
>> cores is
>> lower than the "full-fat" RK3566's 1.8 GHz. [2][3][4]
>
> I think we changed terminology from "full-fat" to something else in the
> rk3588 variant? Would be nice to be consisten.
Back then, it was about the naming of one of the dtsi files, [*] not
about using "full-fat" in the commit description. Using "full-fat"
in one of the file names was just part of the RFC, as a temporary
solution. OTOH, frankly, I don't feel like using "full-fat" in this
commit description is inappropriate or inconsistent.
[*]
https://lore.kernel.org/linux-rockchip/673dcf47596e7bc8ba065034e339bb1bbf9cdcb0.1716948159.git.dsimic@manjaro.org/T/#u
>> These changes follow the approach used for the Rockchip RK3588 SoC
>> variants,
>> which was introduced and described further in commit def88eb4d836
>> ("arm64:
>> dts: rockchip: Prepare RK3588 SoC dtsi files for per-variant OPPs").
>> Please
>> see that commit for a more detailed explanation.
>>
>> No functional changes are introduced, which was validated by
>> decompiling and
>
> No functional changes ...
This will be covered later in my response...
>> comparing all affected board dtb files before and after these changes.
>> In
>> more detail, the affected dtb files have some of their blocks shuffled
>> around
>> a bit and some of their phandles have different values, as a result of
>> the
>> changes to the order in which the building blocks from the parent dtsi
>> files
>> are included, but they effectively remain the same as the originals.
>>
>> [1] https://wiki.pine64.org/wiki/Quartz64
>> [2]
>> https://dl.radxa.com/rock3/docs/hw/3c/radxa_rock3c_product_brief.pdf
>> [3]
>> https://dl.radxa.com/zero3/docs/hw/3e/radxa_zero_3e_product_brief.pdf
>> [4]
>> https://dl.radxa.com/zero3/docs/hw/3w/radxa_zero_3w_product_brief.pdf
>>
>> Related-to: def88eb4d836 ("arm64: dts: rockchip: Prepare RK3588 SoC
>> dtsi files for per-variant OPPs")
>> Signed-off-by: Dragan Simic <dsimic@...jaro.org>
>> ---
>> .../{rk3566.dtsi => rk3566-base.dtsi} | 2 +-
>> arch/arm64/boot/dts/rockchip/rk3566.dtsi | 116
>> ++++++++++++++----
>> arch/arm64/boot/dts/rockchip/rk3568.dtsi | 114 +++++++++++++++--
>> .../{rk356x.dtsi => rk356x-base.dtsi} | 87 -------------
>> 4 files changed, 202 insertions(+), 117 deletions(-)
>> copy arch/arm64/boot/dts/rockchip/{rk3566.dtsi => rk3566-base.dtsi}
>> (95%)
>> rename arch/arm64/boot/dts/rockchip/{rk356x.dtsi => rk356x-base.dtsi}
>> (96%)
>>
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3566.dtsi
>> b/arch/arm64/boot/dts/rockchip/rk3566-base.dtsi
>> similarity index 95%
>> copy from arch/arm64/boot/dts/rockchip/rk3566.dtsi
>> copy to arch/arm64/boot/dts/rockchip/rk3566-base.dtsi
>> index 6c4b17d27bdc..e56e0b6ba941 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3566.dtsi
>> +++ b/arch/arm64/boot/dts/rockchip/rk3566-base.dtsi
>> @@ -1,6 +1,6 @@
>> // SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>>
>> -#include "rk356x.dtsi"
>> +#include "rk356x-base.dtsi"
>>
>> / {
>> compatible = "rockchip,rk3566";
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3566.dtsi
>> b/arch/arm64/boot/dts/rockchip/rk3566.dtsi
>> index 6c4b17d27bdc..3fcca79279f7 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3566.dtsi
>> +++ b/arch/arm64/boot/dts/rockchip/rk3566.dtsi
>> @@ -1,35 +1,107 @@
>> // SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>>
>> -#include "rk356x.dtsi"
>> +#include "rk3566-base.dtsi"
>>
>> / {
>> - compatible = "rockchip,rk3566";
>> + 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;
>> + };
>
> Just like with patch 1 of this series, drop the blank line?
I believe I've already explained the reasoning behind that. [**]
[**]
https://lore.kernel.org/linux-rockchip/0a1f13d06ec3668c136997e72d0aea44@manjaro.org/
>> +
>> + 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>;
>> + };
>> +
>> + opp-1608000000 {
>> + opp-hz = /bits/ 64 <1608000000>;
>> + opp-microvolt = <1100000 1100000 1150000>;
>> + clock-latency-ns = <40000>;
>> + };
>> +
>> + opp-1800000000 {
>> + opp-hz = /bits/ 64 <1800000000>;
>> + opp-microvolt = <1150000 1150000 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>;
>> + };
>> +
>> + opp-800000000 {
>> + opp-hz = /bits/ 64 <800000000>;
>> + opp-microvolt = <1000000 1000000 1000000>;
>> + };
>> + };
>> };
>>
>> -&pipegrf {
>> - compatible = "rockchip,rk3566-pipe-grf", "syscon";
>
> This seems unrelated?
Yes, it looks completely out of place, but it's just the way this
diff ended up looking like. It's actually fine.
>> +&cpu0 {
>> + operating-points-v2 = <&cpu0_opp_table>;
>> };
>>
>> -&power {
>> - power-domain@...568_PD_PIPE {
>> - reg = <RK3568_PD_PIPE>;
>> - clocks = <&cru PCLK_PIPE>;
>> - pm_qos = <&qos_pcie2x1>,
>> - <&qos_sata1>,
>> - <&qos_sata2>,
>> - <&qos_usb3_0>,
>> - <&qos_usb3_1>;
>> - #power-domain-cells = <0>;
>> - };
>
> This seems unrelated to me and possibly a functional change?
> If this was intended, then a description in the commit message would be
> nice why this is appropriate and possibly moved to a separate patch?
Just another instance of the diff ending up looking strange,
while there are actually no such changes.
>> +&cpu1 {
>> + operating-points-v2 = <&cpu0_opp_table>;
>> +};
>> +
>> +&cpu2 {
>> + operating-points-v2 = <&cpu0_opp_table>;
>> };
>>
>> -&usb_host0_xhci {
>> - phys = <&usb2phy0_otg>;
>> - phy-names = "usb2-phy";
>> - extcon = <&usb2phy0>;
>> - maximum-speed = "high-speed";
>
> This also looks unrelated and a functional change?
Already explained above.
>> +&cpu3 {
>> + operating-points-v2 = <&cpu0_opp_table>;
>> };
>>
>> -&vop {
>> - compatible = "rockchip,rk3566-vop";
>
> This also looks unrelated?
Already explained above.
Powered by blists - more mailing lists