[PATCH v2] arm64: dts: rockchip: Add dtsi file for RK3399S SoC variant

[PATCH v2] arm64: dts: rockchip: Add dtsi file for RK3399S SoC variant

[Dragan Simic]

Following the hierarchical representation of the SoC data that's been already
established in the commit 296602b8e5f7 ("arm64: dts: rockchip: Move RK3399
OPPs to dtsi files for SoC variants"), add new SoC dtsi file for the Rockchip
RK3399S SoC, which is yet another variant of the Rockchip RK3399 SoC.

The only perceivable differences between the RK3399S and the RK3399 are in
the supported CPU DVFS OPPs, which result from the RK3399S being binned for
lower maximum CPU frequencies than the regular RK3399 variant.

The RK3399S variant is used in the Pine64 PinePhone Pro only, [1] whose board
dts file included the necessary adjustments to the CPU DVFS OPPs.  This commit
effectively moves those adjustments into the separate RK3399S SoC dtsi file,
following the above-mentioned "encapsulation" approach.

No functional changes are introduced, which was validated by decompiling and
comparing the affected dtb file before and after these changes.

[1] https://wiki.pine64.org/index.php/PinePhone_Pro

Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---

Notes:
    Changes in v2:
      - Renamed the new RK3399S SoC variant dtsi file to rk3399-s.dtsi,
        as suggested by Heiko, [2] which has bothered me too, as a rather
        unnecessary file naming inconsistency
      - Adjusted the patch description accordingly, by removing the note
        about the file naming inconsistency...  yay! :)
      - Validated the introduced changes again, in the same way
    
    Link to v1: https://lore.kernel.org/linux-rockchip/59c524a9a12465c21e01b779b42749fae148c41d.1728482151.git.dsimic@manjaro.org/T/#u
    
    [2] https://lore.kernel.org/linux-rockchip/46729153.fMDQidcC6G@diego/

 .../dts/rockchip/rk3399-pinephone-pro.dts     |  23 +---
 arch/arm64/boot/dts/rockchip/rk3399-s.dtsi    | 123 ++++++++++++++++++
 2 files changed, 124 insertions(+), 22 deletions(-)
 create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-s.dtsi

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
index 1a44582a49fb..eee6cfb6de01 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
@@ -13,7 +13,7 @@
 #include <dt-bindings/input/gpio-keys.h>
 #include <dt-bindings/input/linux-event-codes.h>
 #include <dt-bindings/leds/common.h>
-#include "rk3399.dtsi"
+#include "rk3399-s.dtsi"
 
 / {
 	model = "Pine64 PinePhone Pro";
@@ -456,27 +456,6 @@ mpu6500@68 {
 	};
 };
 
-&cluster0_opp {
-	opp04 {
-		status = "disabled";
-	};
-
-	opp05 {
-		status = "disabled";
-	};
-};
-
-&cluster1_opp {
-	opp06 {
-		opp-hz = /bits/ 64 <1500000000>;
-		opp-microvolt = <1100000 1100000 1150000>;
-	};
-
-	opp07 {
-		status = "disabled";
-	};
-};
-
 &io_domains {
 	bt656-supply = <&vcc1v8_dvp>;
 	audio-supply = <&vcca1v8_codec>;
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-s.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-s.dtsi
new file mode 100644
index 000000000000..e54f451af9f3
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3399-s.dtsi
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (c) 2016-2017 Fuzhou Rockchip Electronics Co., Ltd
+ */
+
+#include "rk3399-base.dtsi"
+
+/ {
+	cluster0_opp: opp-table-0 {
+		compatible = "operating-points-v2";
+		opp-shared;
+
+		opp00 {
+			opp-hz = /bits/ 64 <408000000>;
+			opp-microvolt = <825000 825000 1250000>;
+			clock-latency-ns = <40000>;
+		};
+		opp01 {
+			opp-hz = /bits/ 64 <600000000>;
+			opp-microvolt = <825000 825000 1250000>;
+		};
+		opp02 {
+			opp-hz = /bits/ 64 <816000000>;
+			opp-microvolt = <850000 850000 1250000>;
+		};
+		opp03 {
+			opp-hz = /bits/ 64 <1008000000>;
+			opp-microvolt = <925000 925000 1250000>;
+		};
+	};
+
+	cluster1_opp: opp-table-1 {
+		compatible = "operating-points-v2";
+		opp-shared;
+
+		opp00 {
+			opp-hz = /bits/ 64 <408000000>;
+			opp-microvolt = <825000 825000 1250000>;
+			clock-latency-ns = <40000>;
+		};
+		opp01 {
+			opp-hz = /bits/ 64 <600000000>;
+			opp-microvolt = <825000 825000 1250000>;
+		};
+		opp02 {
+			opp-hz = /bits/ 64 <816000000>;
+			opp-microvolt = <825000 825000 1250000>;
+		};
+		opp03 {
+			opp-hz = /bits/ 64 <1008000000>;
+			opp-microvolt = <875000 875000 1250000>;
+		};
+		opp04 {
+			opp-hz = /bits/ 64 <1200000000>;
+			opp-microvolt = <950000 950000 1250000>;
+		};
+		opp05 {
+			opp-hz = /bits/ 64 <1416000000>;
+			opp-microvolt = <1025000 1025000 1250000>;
+		};
+		opp06 {
+			opp-hz = /bits/ 64 <1500000000>;
+			opp-microvolt = <1100000 1100000 1150000>;
+		};
+	};
+
+	gpu_opp_table: opp-table-2 {
+		compatible = "operating-points-v2";
+
+		opp00 {
+			opp-hz = /bits/ 64 <200000000>;
+			opp-microvolt = <825000 825000 1150000>;
+		};
+		opp01 {
+			opp-hz = /bits/ 64 <297000000>;
+			opp-microvolt = <825000 825000 1150000>;
+		};
+		opp02 {
+			opp-hz = /bits/ 64 <400000000>;
+			opp-microvolt = <825000 825000 1150000>;
+		};
+		opp03 {
+			opp-hz = /bits/ 64 <500000000>;
+			opp-microvolt = <875000 875000 1150000>;
+		};
+		opp04 {
+			opp-hz = /bits/ 64 <600000000>;
+			opp-microvolt = <925000 925000 1150000>;
+		};
+		opp05 {
+			opp-hz = /bits/ 64 <800000000>;
+			opp-microvolt = <1100000 1100000 1150000>;
+		};
+	};
+};
+
+&cpu_l0 {
+	operating-points-v2 = <&cluster0_opp>;
+};
+
+&cpu_l1 {
+	operating-points-v2 = <&cluster0_opp>;
+};
+
+&cpu_l2 {
+	operating-points-v2 = <&cluster0_opp>;
+};
+
+&cpu_l3 {
+	operating-points-v2 = <&cluster0_opp>;
+};
+
+&cpu_b0 {
+	operating-points-v2 = <&cluster1_opp>;
+};
+
+&cpu_b1 {
+	operating-points-v2 = <&cluster1_opp>;
+};
+
+&gpu {
+	operating-points-v2 = <&gpu_opp_table>;
+};

Re: [PATCH v2] arm64: dts: rockchip: Add dtsi file for RK3399S SoC variant

[Diederik de Haas]

[-- Attachment #1: Type: text/plain, Size: 3228 bytes --]

On Fri Oct 11, 2024 at 9:40 AM CEST, Dragan Simic wrote:
> Following the hierarchical representation of the SoC data that's been already
> established in the commit 296602b8e5f7 ("arm64: dts: rockchip: Move RK3399
> OPPs to dtsi files for SoC variants"), add new SoC dtsi file for the Rockchip
> RK3399S SoC, which is yet another variant of the Rockchip RK3399 SoC.
> ...
> The RK3399S variant is used in the Pine64 PinePhone Pro only, [1] whose board
> dts file included the necessary adjustments to the CPU DVFS OPPs.  This commit
> effectively moves those adjustments into the separate RK3399S SoC dtsi file,
> following the above-mentioned "encapsulation" approach.
> ...
> ---
> ...
>  .../dts/rockchip/rk3399-pinephone-pro.dts     |  23 +---
>  arch/arm64/boot/dts/rockchip/rk3399-s.dtsi    | 123 ++++++++++++++++++
>  2 files changed, 124 insertions(+), 22 deletions(-)
>  create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-s.dtsi
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> index 1a44582a49fb..eee6cfb6de01 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> @@ -13,7 +13,7 @@
>  #include <dt-bindings/input/gpio-keys.h>
>  #include <dt-bindings/input/linux-event-codes.h>
>  #include <dt-bindings/leds/common.h>
> -#include "rk3399.dtsi"
> +#include "rk3399-s.dtsi"
>  
>  / {
>  	model = "Pine64 PinePhone Pro";
> @@ -456,27 +456,6 @@ mpu6500@68 {
>  	};
>  };
>  
> -&cluster0_opp {
> -	opp04 {
> -		status = "disabled";
> -	};
> -
> -	opp05 {
> -		status = "disabled";
> -	};
> -};
> -
> -&cluster1_opp {
> -	opp06 {
> -		opp-hz = /bits/ 64 <1500000000>;
> -		opp-microvolt = <1100000 1100000 1150000>;
> -	};
> -
> -	opp07 {
> -		status = "disabled";
> -	};
> -};
> -
>  &io_domains {
>  	bt656-supply = <&vcc1v8_dvp>;
>  	audio-supply = <&vcca1v8_codec>;
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-s.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-s.dtsi
> new file mode 100644
> index 000000000000..e54f451af9f3
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-s.dtsi
> @@ -0,0 +1,123 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (c) 2016-2017 Fuzhou Rockchip Electronics Co., Ltd
> + */
> +
> +#include "rk3399-base.dtsi"
> +
> +/ {
> +	cluster0_opp: opp-table-0 {
> +		compatible = "operating-points-v2";
> +		opp-shared;
> +
> +		opp00 {
> +			opp-hz = /bits/ 64 <408000000>;
> +			opp-microvolt = <825000 825000 1250000>;
> +			clock-latency-ns = <40000>;
> +		};
> +		opp01 {
> +			opp-hz = /bits/ 64 <600000000>;
> +			opp-microvolt = <825000 825000 1250000>;
> +		};
> +		opp02 {
> +			opp-hz = /bits/ 64 <816000000>;
> +			opp-microvolt = <850000 850000 1250000>;
> +		};

Is there a reason why there isn't a line separator between the various
opp nodes? Normally there is one between nodes.
Note that in rk3588-opp.dtsi there are no separator lines between the
opp nodes, while they do exist between other nodes.
And in rk356x.dtsi the opp nodes do have a separator line.

Cheers,
  Diederik

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2] arm64: dts: rockchip: Add dtsi file for RK3399S SoC variant

[Dragan Simic]

Hello Diederik,

On 2024-10-11 10:00, Diederik de Haas wrote:
> On Fri Oct 11, 2024 at 9:40 AM CEST, Dragan Simic wrote:
>> Following the hierarchical representation of the SoC data that's been 
>> already
>> established in the commit 296602b8e5f7 ("arm64: dts: rockchip: Move 
>> RK3399
>> OPPs to dtsi files for SoC variants"), add new SoC dtsi file for the 
>> Rockchip
>> RK3399S SoC, which is yet another variant of the Rockchip RK3399 SoC.
>> ...
>> The RK3399S variant is used in the Pine64 PinePhone Pro only, [1] 
>> whose board
>> dts file included the necessary adjustments to the CPU DVFS OPPs.  
>> This commit
>> effectively moves those adjustments into the separate RK3399S SoC dtsi 
>> file,
>> following the above-mentioned "encapsulation" approach.
>> ...
>> ---
>> ...
>>  .../dts/rockchip/rk3399-pinephone-pro.dts     |  23 +---
>>  arch/arm64/boot/dts/rockchip/rk3399-s.dtsi    | 123 
>> ++++++++++++++++++
>>  2 files changed, 124 insertions(+), 22 deletions(-)
>>  create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-s.dtsi
>> 
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts 
>> b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
>> index 1a44582a49fb..eee6cfb6de01 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
>> @@ -13,7 +13,7 @@
>>  #include <dt-bindings/input/gpio-keys.h>
>>  #include <dt-bindings/input/linux-event-codes.h>
>>  #include <dt-bindings/leds/common.h>
>> -#include "rk3399.dtsi"
>> +#include "rk3399-s.dtsi"
>> 
>>  / {
>>  	model = "Pine64 PinePhone Pro";
>> @@ -456,27 +456,6 @@ mpu6500@68 {
>>  	};
>>  };
>> 
>> -&cluster0_opp {
>> -	opp04 {
>> -		status = "disabled";
>> -	};
>> -
>> -	opp05 {
>> -		status = "disabled";
>> -	};
>> -};
>> -
>> -&cluster1_opp {
>> -	opp06 {
>> -		opp-hz = /bits/ 64 <1500000000>;
>> -		opp-microvolt = <1100000 1100000 1150000>;
>> -	};
>> -
>> -	opp07 {
>> -		status = "disabled";
>> -	};
>> -};
>> -
>>  &io_domains {
>>  	bt656-supply = <&vcc1v8_dvp>;
>>  	audio-supply = <&vcca1v8_codec>;
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-s.dtsi 
>> b/arch/arm64/boot/dts/rockchip/rk3399-s.dtsi
>> new file mode 100644
>> index 000000000000..e54f451af9f3
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-s.dtsi
>> @@ -0,0 +1,123 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Copyright (c) 2016-2017 Fuzhou Rockchip Electronics Co., Ltd
>> + */
>> +
>> +#include "rk3399-base.dtsi"
>> +
>> +/ {
>> +	cluster0_opp: opp-table-0 {
>> +		compatible = "operating-points-v2";
>> +		opp-shared;
>> +
>> +		opp00 {
>> +			opp-hz = /bits/ 64 <408000000>;
>> +			opp-microvolt = <825000 825000 1250000>;
>> +			clock-latency-ns = <40000>;
>> +		};
>> +		opp01 {
>> +			opp-hz = /bits/ 64 <600000000>;
>> +			opp-microvolt = <825000 825000 1250000>;
>> +		};
>> +		opp02 {
>> +			opp-hz = /bits/ 64 <816000000>;
>> +			opp-microvolt = <850000 850000 1250000>;
>> +		};
> 
> Is there a reason why there isn't a line separator between the various
> opp nodes? Normally there is one between nodes.
> Note that in rk3588-opp.dtsi there are no separator lines between the
> opp nodes, while they do exist between other nodes.
> And in rk356x.dtsi the opp nodes do have a separator line.

That has also bothered me. :)  I already had a look around in various
dts(i) files long time ago and there seems to be no preferred layout.

In this particular case, it's better to have no separator lines because
that's what we already have lacking in rk3399.dtsi, rk3399-t.dtsi, etc.,
so running something like "diff rk3399.dtsi rk3399-s.dtsi" makes it easy
to see what actually differs in the RK3399 SoC variants, without having
to filter out any whitespace differences.

Re: [PATCH v2] arm64: dts: rockchip: Add dtsi file for RK3399S SoC variant

[Diederik de Haas]

[-- Attachment #1: Type: text/plain, Size: 4428 bytes --]

Hi Dragan,

On Fri Oct 11, 2024 at 10:23 AM CEST, Dragan Simic wrote:
> On 2024-10-11 10:00, Diederik de Haas wrote:
> > On Fri Oct 11, 2024 at 9:40 AM CEST, Dragan Simic wrote:
> >> Following the hierarchical representation of the SoC data that's been 
> >> already
> >> established in the commit 296602b8e5f7 ("arm64: dts: rockchip: Move 
> >> RK3399
> >> OPPs to dtsi files for SoC variants"), add new SoC dtsi file for the 
> >> Rockchip
> >> RK3399S SoC, which is yet another variant of the Rockchip RK3399 SoC.
> >> ...
> >> The RK3399S variant is used in the Pine64 PinePhone Pro only, [1] 
> >> whose board
> >> dts file included the necessary adjustments to the CPU DVFS OPPs.  
> >> This commit
> >> effectively moves those adjustments into the separate RK3399S SoC dtsi 
> >> file,
> >> following the above-mentioned "encapsulation" approach.
> >> ...
> >> ---
> >> ...
> >>  .../dts/rockchip/rk3399-pinephone-pro.dts     |  23 +---
> >>  arch/arm64/boot/dts/rockchip/rk3399-s.dtsi    | 123 
> >> ++++++++++++++++++
> >>  2 files changed, 124 insertions(+), 22 deletions(-)
> >>  create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-s.dtsi
> >> 
> >> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts 
> >> b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> >> index 1a44582a49fb..eee6cfb6de01 100644
> >> --- a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> >> +++ b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> >> @@ -13,7 +13,7 @@
> >>  #include <dt-bindings/input/gpio-keys.h>
> >>  #include <dt-bindings/input/linux-event-codes.h>
> >>  #include <dt-bindings/leds/common.h>
> >> -#include "rk3399.dtsi"
> >> +#include "rk3399-s.dtsi"
> >> 
> >>  / {
> >>  	model = "Pine64 PinePhone Pro";
> >> @@ -456,27 +456,6 @@ mpu6500@68 {
> >>  	};
> >>  };
> >> 
> >> -&cluster0_opp {
> >> -	opp04 {
> >> -		status = "disabled";
> >> -	};
> >> -
> >> -	opp05 {
> >> -		status = "disabled";
> >> -	};
> >> -};
> >> -
> >> -&cluster1_opp {
> >> -	opp06 {
> >> -		opp-hz = /bits/ 64 <1500000000>;
> >> -		opp-microvolt = <1100000 1100000 1150000>;
> >> -	};
> >> -
> >> -	opp07 {
> >> -		status = "disabled";
> >> -	};
> >> -};
> >> -
> >>  &io_domains {
> >>  	bt656-supply = <&vcc1v8_dvp>;
> >>  	audio-supply = <&vcca1v8_codec>;
> >> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-s.dtsi 
> >> b/arch/arm64/boot/dts/rockchip/rk3399-s.dtsi
> >> new file mode 100644
> >> index 000000000000..e54f451af9f3
> >> --- /dev/null
> >> +++ b/arch/arm64/boot/dts/rockchip/rk3399-s.dtsi
> >> @@ -0,0 +1,123 @@
> >> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> >> +/*
> >> + * Copyright (c) 2016-2017 Fuzhou Rockchip Electronics Co., Ltd
> >> + */
> >> +
> >> +#include "rk3399-base.dtsi"
> >> +
> >> +/ {
> >> +	cluster0_opp: opp-table-0 {
> >> +		compatible = "operating-points-v2";
> >> +		opp-shared;
> >> +
> >> +		opp00 {
> >> +			opp-hz = /bits/ 64 <408000000>;
> >> +			opp-microvolt = <825000 825000 1250000>;
> >> +			clock-latency-ns = <40000>;
> >> +		};
> >> +		opp01 {
> >> +			opp-hz = /bits/ 64 <600000000>;
> >> +			opp-microvolt = <825000 825000 1250000>;
> >> +		};
> >> +		opp02 {
> >> +			opp-hz = /bits/ 64 <816000000>;
> >> +			opp-microvolt = <850000 850000 1250000>;
> >> +		};
> > 
> > Is there a reason why there isn't a line separator between the various
> > opp nodes? Normally there is one between nodes.
> > Note that in rk3588-opp.dtsi there are no separator lines between the
> > opp nodes, while they do exist between other nodes.
> > And in rk356x.dtsi the opp nodes do have a separator line.
>
> That has also bothered me. :)  I already had a look around in various
> dts(i) files long time ago and there seems to be no preferred layout.

I'm inclined to say the opp ones are the odd ones.

> In this particular case, it's better to have no separator lines because
> that's what we already have lacking in rk3399.dtsi, rk3399-t.dtsi, etc.,
> so running something like "diff rk3399.dtsi rk3399-s.dtsi" makes it easy
> to see what actually differs in the RK3399 SoC variants, without having
> to filter out any whitespace differences.

Besides that inconsistencies always seem to 'trigger' me, I especially
noticed it as this patch changed it from having separator lines to
having no separator lines.

Cheers,
  Diederik

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2] arm64: dts: rockchip: Add dtsi file for RK3399S SoC variant

[Heiko Stübner]

Am Freitag, 11. Oktober 2024, 10:33:56 CEST schrieb Diederik de Haas:
> Hi Dragan,
> 
> On Fri Oct 11, 2024 at 10:23 AM CEST, Dragan Simic wrote:
> > On 2024-10-11 10:00, Diederik de Haas wrote:
> > > On Fri Oct 11, 2024 at 9:40 AM CEST, Dragan Simic wrote:
> > >> Following the hierarchical representation of the SoC data that's been 
> > >> already
> > >> established in the commit 296602b8e5f7 ("arm64: dts: rockchip: Move 
> > >> RK3399
> > >> OPPs to dtsi files for SoC variants"), add new SoC dtsi file for the 
> > >> Rockchip
> > >> RK3399S SoC, which is yet another variant of the Rockchip RK3399 SoC.
> > >> ...
> > >> The RK3399S variant is used in the Pine64 PinePhone Pro only, [1] 
> > >> whose board
> > >> dts file included the necessary adjustments to the CPU DVFS OPPs.  
> > >> This commit
> > >> effectively moves those adjustments into the separate RK3399S SoC dtsi 
> > >> file,
> > >> following the above-mentioned "encapsulation" approach.
> > >> ...
> > >> ---
> > >> ...
> > >>  .../dts/rockchip/rk3399-pinephone-pro.dts     |  23 +---
> > >>  arch/arm64/boot/dts/rockchip/rk3399-s.dtsi    | 123 
> > >> ++++++++++++++++++
> > >>  2 files changed, 124 insertions(+), 22 deletions(-)
> > >>  create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-s.dtsi
> > >> 
> > >> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts 
> > >> b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> > >> index 1a44582a49fb..eee6cfb6de01 100644
> > >> --- a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> > >> +++ b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> > >> @@ -13,7 +13,7 @@
> > >>  #include <dt-bindings/input/gpio-keys.h>
> > >>  #include <dt-bindings/input/linux-event-codes.h>
> > >>  #include <dt-bindings/leds/common.h>
> > >> -#include "rk3399.dtsi"
> > >> +#include "rk3399-s.dtsi"
> > >> 
> > >>  / {
> > >>  	model = "Pine64 PinePhone Pro";
> > >> @@ -456,27 +456,6 @@ mpu6500@68 {
> > >>  	};
> > >>  };
> > >> 
> > >> -&cluster0_opp {
> > >> -	opp04 {
> > >> -		status = "disabled";
> > >> -	};
> > >> -
> > >> -	opp05 {
> > >> -		status = "disabled";
> > >> -	};
> > >> -};
> > >> -
> > >> -&cluster1_opp {
> > >> -	opp06 {
> > >> -		opp-hz = /bits/ 64 <1500000000>;
> > >> -		opp-microvolt = <1100000 1100000 1150000>;
> > >> -	};
> > >> -
> > >> -	opp07 {
> > >> -		status = "disabled";
> > >> -	};
> > >> -};
> > >> -
> > >>  &io_domains {
> > >>  	bt656-supply = <&vcc1v8_dvp>;
> > >>  	audio-supply = <&vcca1v8_codec>;
> > >> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-s.dtsi 
> > >> b/arch/arm64/boot/dts/rockchip/rk3399-s.dtsi
> > >> new file mode 100644
> > >> index 000000000000..e54f451af9f3
> > >> --- /dev/null
> > >> +++ b/arch/arm64/boot/dts/rockchip/rk3399-s.dtsi
> > >> @@ -0,0 +1,123 @@
> > >> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > >> +/*
> > >> + * Copyright (c) 2016-2017 Fuzhou Rockchip Electronics Co., Ltd
> > >> + */
> > >> +
> > >> +#include "rk3399-base.dtsi"
> > >> +
> > >> +/ {
> > >> +	cluster0_opp: opp-table-0 {
> > >> +		compatible = "operating-points-v2";
> > >> +		opp-shared;
> > >> +
> > >> +		opp00 {
> > >> +			opp-hz = /bits/ 64 <408000000>;
> > >> +			opp-microvolt = <825000 825000 1250000>;
> > >> +			clock-latency-ns = <40000>;
> > >> +		};
> > >> +		opp01 {
> > >> +			opp-hz = /bits/ 64 <600000000>;
> > >> +			opp-microvolt = <825000 825000 1250000>;
> > >> +		};
> > >> +		opp02 {
> > >> +			opp-hz = /bits/ 64 <816000000>;
> > >> +			opp-microvolt = <850000 850000 1250000>;
> > >> +		};
> > > 
> > > Is there a reason why there isn't a line separator between the various
> > > opp nodes? Normally there is one between nodes.
> > > Note that in rk3588-opp.dtsi there are no separator lines between the
> > > opp nodes, while they do exist between other nodes.
> > > And in rk356x.dtsi the opp nodes do have a separator line.
> >
> > That has also bothered me. :)  I already had a look around in various
> > dts(i) files long time ago and there seems to be no preferred layout.

I guess "with" lines in between is sort-of preferred in general.
I sometime add them in new board-dts when applying and noticing them,
but also sometimes miss them.

I guess empty lines are helpful when the nodes are "not the same",
but I guess for OPPs it doesn't matter so much, as the individual nodes
are all the same.


But in the end, I guess just follow the other OPPs in rk3399 for now ;-)
[as this patch does]

> I'm inclined to say the opp ones are the odd ones.
> 
> > In this particular case, it's better to have no separator lines because
> > that's what we already have lacking in rk3399.dtsi, rk3399-t.dtsi, etc.,
> > so running something like "diff rk3399.dtsi rk3399-s.dtsi" makes it easy
> > to see what actually differs in the RK3399 SoC variants, without having
> > to filter out any whitespace differences.
> 
> Besides that inconsistencies always seem to 'trigger' me, I especially
> noticed it as this patch changed it from having separator lines to
> having no separator lines.
> 
> Cheers,
>   Diederik
> 

Re: [PATCH v2] arm64: dts: rockchip: Add dtsi file for RK3399S SoC variant

[Dragan Simic]

Hello Heiko and Diederik,

On 2024-10-11 10:43, Heiko Stübner wrote:
> Am Freitag, 11. Oktober 2024, 10:33:56 CEST schrieb Diederik de Haas:
>> On Fri Oct 11, 2024 at 10:23 AM CEST, Dragan Simic wrote:
>> > On 2024-10-11 10:00, Diederik de Haas wrote:
>> > > On Fri Oct 11, 2024 at 9:40 AM CEST, Dragan Simic wrote:
>> > >> Following the hierarchical representation of the SoC data that's been
>> > >> already
>> > >> established in the commit 296602b8e5f7 ("arm64: dts: rockchip: Move
>> > >> RK3399
>> > >> OPPs to dtsi files for SoC variants"), add new SoC dtsi file for the
>> > >> Rockchip
>> > >> RK3399S SoC, which is yet another variant of the Rockchip RK3399 SoC.
>> > >> ...
>> > >> The RK3399S variant is used in the Pine64 PinePhone Pro only, [1]
>> > >> whose board
>> > >> dts file included the necessary adjustments to the CPU DVFS OPPs.
>> > >> This commit
>> > >> effectively moves those adjustments into the separate RK3399S SoC dtsi
>> > >> file,
>> > >> following the above-mentioned "encapsulation" approach.
>> > >> ...
>> > >> ---
>> > >> ...
>> > >>  .../dts/rockchip/rk3399-pinephone-pro.dts     |  23 +---
>> > >>  arch/arm64/boot/dts/rockchip/rk3399-s.dtsi    | 123
>> > >> ++++++++++++++++++
>> > >>  2 files changed, 124 insertions(+), 22 deletions(-)
>> > >>  create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-s.dtsi
>> > >>
>> > >> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
>> > >> b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
>> > >> index 1a44582a49fb..eee6cfb6de01 100644
>> > >> --- a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
>> > >> +++ b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
>> > >> @@ -13,7 +13,7 @@
>> > >>  #include <dt-bindings/input/gpio-keys.h>
>> > >>  #include <dt-bindings/input/linux-event-codes.h>
>> > >>  #include <dt-bindings/leds/common.h>
>> > >> -#include "rk3399.dtsi"
>> > >> +#include "rk3399-s.dtsi"
>> > >>
>> > >>  / {
>> > >>  	model = "Pine64 PinePhone Pro";
>> > >> @@ -456,27 +456,6 @@ mpu6500@68 {
>> > >>  	};
>> > >>  };
>> > >>
>> > >> -&cluster0_opp {
>> > >> -	opp04 {
>> > >> -		status = "disabled";
>> > >> -	};
>> > >> -
>> > >> -	opp05 {
>> > >> -		status = "disabled";
>> > >> -	};
>> > >> -};
>> > >> -
>> > >> -&cluster1_opp {
>> > >> -	opp06 {
>> > >> -		opp-hz = /bits/ 64 <1500000000>;
>> > >> -		opp-microvolt = <1100000 1100000 1150000>;
>> > >> -	};
>> > >> -
>> > >> -	opp07 {
>> > >> -		status = "disabled";
>> > >> -	};
>> > >> -};
>> > >> -
>> > >>  &io_domains {
>> > >>  	bt656-supply = <&vcc1v8_dvp>;
>> > >>  	audio-supply = <&vcca1v8_codec>;
>> > >> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-s.dtsi
>> > >> b/arch/arm64/boot/dts/rockchip/rk3399-s.dtsi
>> > >> new file mode 100644
>> > >> index 000000000000..e54f451af9f3
>> > >> --- /dev/null
>> > >> +++ b/arch/arm64/boot/dts/rockchip/rk3399-s.dtsi
>> > >> @@ -0,0 +1,123 @@
>> > >> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> > >> +/*
>> > >> + * Copyright (c) 2016-2017 Fuzhou Rockchip Electronics Co., Ltd
>> > >> + */
>> > >> +
>> > >> +#include "rk3399-base.dtsi"
>> > >> +
>> > >> +/ {
>> > >> +	cluster0_opp: opp-table-0 {
>> > >> +		compatible = "operating-points-v2";
>> > >> +		opp-shared;
>> > >> +
>> > >> +		opp00 {
>> > >> +			opp-hz = /bits/ 64 <408000000>;
>> > >> +			opp-microvolt = <825000 825000 1250000>;
>> > >> +			clock-latency-ns = <40000>;
>> > >> +		};
>> > >> +		opp01 {
>> > >> +			opp-hz = /bits/ 64 <600000000>;
>> > >> +			opp-microvolt = <825000 825000 1250000>;
>> > >> +		};
>> > >> +		opp02 {
>> > >> +			opp-hz = /bits/ 64 <816000000>;
>> > >> +			opp-microvolt = <850000 850000 1250000>;
>> > >> +		};
>> > >
>> > > Is there a reason why there isn't a line separator between the various
>> > > opp nodes? Normally there is one between nodes.
>> > > Note that in rk3588-opp.dtsi there are no separator lines between the
>> > > opp nodes, while they do exist between other nodes.
>> > > And in rk356x.dtsi the opp nodes do have a separator line.
>> >
>> > That has also bothered me. :)  I already had a look around in various
>> > dts(i) files long time ago and there seems to be no preferred layout.
> 
> I guess "with" lines in between is sort-of preferred in general.
> I sometime add them in new board-dts when applying and noticing them,
> but also sometimes miss them.
> 
> I guess empty lines are helpful when the nodes are "not the same",
> but I guess for OPPs it doesn't matter so much, as the individual nodes
> are all the same.

Ah, sorry, I wasn't precise enough in my earlier response to
Diederik...  My research that I referred to was about the OPP nodes
in various dts(i) files, for which there seems to be no preferred
or commonly used layout.

For other nodes, in most cases it's much better to have separator
lines, because they represent different things, which also seems to
be the preferred layout used in most places.

> But in the end, I guess just follow the other OPPs in rk3399 for now 
> ;-)
> [as this patch does]

Agreed.  We'd need to patch a few additional RK3399 files otherwise,
because we'd then need to add separator lines into other RK3399 files
as well...  Inconsistency is also not so great. :)

>> I'm inclined to say the opp ones are the odd ones.
>> 
>> > In this particular case, it's better to have no separator lines because
>> > that's what we already have lacking in rk3399.dtsi, rk3399-t.dtsi, etc.,
>> > so running something like "diff rk3399.dtsi rk3399-s.dtsi" makes it easy
>> > to see what actually differs in the RK3399 SoC variants, without having
>> > to filter out any whitespace differences.
>> 
>> Besides that inconsistencies always seem to 'trigger' me, I especially
>> noticed it as this patch changed it from having separator lines to
>> having no separator lines.

Ah, totally understood. :)

Re: [PATCH v2] arm64: dts: rockchip: Add dtsi file for RK3399S SoC variant

[Heiko Stuebner]

On Fri, 11 Oct 2024 09:40:51 +0200, Dragan Simic wrote:
> Following the hierarchical representation of the SoC data that's been already
> established in the commit 296602b8e5f7 ("arm64: dts: rockchip: Move RK3399
> OPPs to dtsi files for SoC variants"), add new SoC dtsi file for the Rockchip
> RK3399S SoC, which is yet another variant of the Rockchip RK3399 SoC.
> 
> The only perceivable differences between the RK3399S and the RK3399 are in
> the supported CPU DVFS OPPs, which result from the RK3399S being binned for
> lower maximum CPU frequencies than the regular RK3399 variant.
> 
> [...]

Applied, thanks!

[1/1] arm64: dts: rockchip: Add dtsi file for RK3399S SoC variant
      commit: f7f8ec7d8cef4cf62ee13b526d59438c23bbb34f

Best regards,
-- 
Heiko Stuebner <heiko@sntech.de>