arm: dts: Fix OPP and cooling device properties

It should be "#cooling-cells" instead of "cooling-cells". Fix it.

Signed-off-by Viresh Kumar <[email protected]>

---
 arch/arm/boot/dts/armada-385-synology-ds116.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/boot/dts/armada-385-synology-ds116.dts +1 -1

index 6782ce481ac9..d8769956cbfc 100644
@@ -139,7 +139,7 @@
 					      3700 5
 					      3900 6
 					      4000 7>;
-			cooling-cells = <2>;
+			#cooling-cells = <2>;
 		};
 
 		gpio-leds {

Signature · 1 line

2.15.0.194.g9af6a3dea062

The cooling device properties, like "#cooling-cells" and "dynamic-power-coefficient", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can register a cooling device.

Add such missing properties.

Signed-off-by Viresh Kumar <[email protected]>

The cooling device properties, like "#cooling-cells" and "dynamic-power-coefficient", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can register a cooling device.

Add such missing properties.

Signed-off-by Viresh Kumar <[email protected]>

---
 arch/arm/boot/dts/mt7623.dtsi | 3 +++
 1 file changed, 3 insertions(+)

arch/arm/boot/dts/mt7623.dtsi +3 -0

index d1eb123bc73b..1cdc346a05e8 100644
@@ -92,6 +92,7 @@
 				 <&apmixedsys CLK_APMIXED_MAINPLL>;
 			clock-names = "cpu", "intermediate";
 			operating-points-v2 = <&cpu_opp_table>;
+			#cooling-cells = <2>;
 			clock-frequency = <1300000000>;
 		};
 

@@ -103,6 +104,7 @@
 				 <&apmixedsys CLK_APMIXED_MAINPLL>;
 			clock-names = "cpu", "intermediate";
 			operating-points-v2 = <&cpu_opp_table>;
+			#cooling-cells = <2>;
 			clock-frequency = <1300000000>;
 		};
 

@@ -114,6 +116,7 @@
 				 <&apmixedsys CLK_APMIXED_MAINPLL>;
 			clock-names = "cpu", "intermediate";
 			operating-points-v2 = <&cpu_opp_table>;
+			#cooling-cells = <2>;
 			clock-frequency = <1300000000>;
 		};
 	};

The cooling device properties, like "#cooling-cells" and "dynamic-power-coefficient", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can register a cooling device.

Add such missing properties.

Signed-off-by Viresh Kumar <[email protected]>

The cooling device properties, like "#cooling-cells" and "dynamic-power-coefficient", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can register a cooling device.

Add such missing properties.

Signed-off-by Viresh Kumar <[email protected]>

---
 arch/arm/boot/dts/uniphier-pxs2.dtsi | 3 +++
 1 file changed, 3 insertions(+)

arch/arm/boot/dts/uniphier-pxs2.dtsi +3 -1

index debcbd15c24b..40ed15465095 100644
@@ -36,6 +36,7 @@
                        enable-method = "psci";
                        next-level-cache = <&l2>;
                        operating-points-v2 = <&cpu_opp>;
+                       #cooling-cells = <2>;
                };
 
                cpu2: cpu at 2 {

@@ -46,6 +47,7 @@
                        enable-method = "psci";
                        next-level-cache = <&l2>;
                        operating-points-v2 = <&cpu_opp>;
+                       #cooling-cells = <2>;
                };
 
                cpu3: cpu at 3 {

@@ -56,6 +58,7 @@
                        enable-method = "psci";
                        next-level-cache = <&l2>;
                        operating-points-v2 = <&cpu_opp>;
+                       #cooling-cells = <2>;
                };
        };
 
--

2.15.0.194.g9af6a3dea062

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

The cooling device properties, like "#cooling-cells" and "dynamic-power-coefficient", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can register a cooling device.

Add such missing properties.

Fix other missing properties (clocks, OPP, clock latency) as well to make it all work.

Signed-off-by Viresh Kumar <[email protected]>

Hi,

Quoted 13 lines · Viresh Kumar

The cooling device properties, like "#cooling-cells" and "dynamic-power-coefficient", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can register a cooling device.

Add such missing properties.

Fix other missing properties (clocks, OPP, clock latency) as well to make it all work.

Quoted 1 line

Signed-off-by Viresh Kumar <[email protected]>

The prefix should be sunxi and not sun, but it looks good to me otherwise.

Let me know what your preferred merge method is.

Quoted 18 lines · Maxime Ripard

Hi,

Quoted 14 lines · Viresh Kumar

The cooling device properties, like "#cooling-cells" and "dynamic-power-coefficient", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can register a cooling device.

Add such missing properties.

Fix other missing properties (clocks, OPP, clock latency) as well to make it all work.

Signed-off-by Viresh Kumar <[email protected]>

Quoted 4 lines

The prefix should be sunxi and not sun, but it looks good to me otherwise.

Let me know what your preferred merge method is.

Please pick it up directly and send it as part of your pull request. Do you want me to resend or can you fix the $subject ?

The cooling device properties, like "#cooling-cells" and "dynamic-power-coefficient", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can register a cooling device.

Add such missing properties.

Fix other missing properties (clocks, OPP, clock latency) as well to make it all work.

Signed-off-by Viresh Kumar <[email protected]>

Quoted 8 lines · Viresh Kumar

The cooling device properties, like "#cooling-cells" and "dynamic-power-coefficient", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can register a cooling device.

Quoted 6 lines

Add such missing properties.

Fix other missing properties (clocks, OPP, clock latency) as well to make it all work.

Signed-off-by Viresh Kumar <[email protected]>

Applied both, thanks!

Quoted 11 lines · Maxime Ripard

Quoted 10 lines · Viresh Kumar

The cooling device properties, like "#cooling-cells" and "dynamic-power-coefficient", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can register a cooling device.

Add such missing properties.

Quoted 6 lines

Quoted 4 lines

Fix other missing properties (clocks, OPP, clock latency) as well to make it all work.

Signed-off-by Viresh Kumar <[email protected]>

Applied both, thanks!

Please fix the "ARM" prefix when applying. :)

The cooling device properties, like "#cooling-cells" and "dynamic-power-coefficient", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order.

For example, this will happen
because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can register a cooling device.

Add such missing properties.

Fix other missing properties (clocks, OPP, clock latency) as well to make it all work.

Signed-off-by Viresh Kumar <[email protected]>

---
 arch/arm/boot/dts/exynos3250.dtsi | 16 ++++++++++++++++
 arch/arm/boot/dts/exynos4210.dtsi | 13 +++++++++++++
 arch/arm/boot/dts/exynos4412.dtsi |  9 +++++++++
 arch/arm/boot/dts/exynos5250.dtsi | 23 +++++++++++++++++++++++
 4 files changed, 61 insertions(+)

Thanks for the patch.

In case of Exynos, the booting CPU always has these information in DT and the booting CPU cannot be changed (chosen by firmware/hardware configuration).

Therefore there is no real risk of falling although for correctness of DT your change makes sense.

It is too late for this cycle for me so I'll pick it up after merge window. Alternatively, arm-soc guys can pick it up directly with my tag:

Reviewed-by Krzysztof Kozlowski <[email protected]>

Quoted 5 lines · Krzysztof Kozlowski

Thanks for the patch.

In case of Exynos, the booting CPU always has these information in DT and the booting CPU cannot be changed (chosen by firmware/hardware configuration).

But can the booting CPU be offlined ?

If yes, then this is how things are broken right now.

Build cpufreq driver as module, boot kernel, hotplug out CPU0, insert cpufreq driver and that will try to find these properties in CPU1.

OK, I see the possibility although it is still far away from use cases. You cannot hotplug booting CPU (CPU0) on Exynos kernels. It never worked. Strictly speaking - offlining will work. But bringing it online will likely hang the system.

The cooling device properties, like "#cooling-cells" and "dynamic-power-coefficient", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can register a cooling device.

Add such missing properties.

Fix other missing properties (clocks, OPP, clock latency) as well to make it all work.

Signed-off-by Viresh Kumar <[email protected]>

---
 arch/arm/boot/dts/exynos3250.dtsi | 16 ++++++++++++++++
 arch/arm/boot/dts/exynos4210.dtsi | 13 +++++++++++++
 arch/arm/boot/dts/exynos4412.dtsi |  9 +++++++++
 arch/arm/boot/dts/exynos5250.dtsi | 23 +++++++++++++++++++++++
 4 files changed, 61 insertions(+)

The cooling device properties, like "#cooling-cells" and "dynamic-power-coefficient", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can register a cooling device.

Add such missing properties.

Fix other missing properties (clocks, supply, clock latency) as well to make it all work.

The cooling device properties, like "#cooling-cells" and "dynamic-power-coefficient", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can register a cooling device.

Add such missing properties.

Fix other missing properties (clocks, supply, clock latency) as well to make it all work.

The cooling device properties, like "#cooling-cells" and "dynamic-power-coefficient", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can register a cooling device.

Add such missing properties.

Fix other missing properties (clocks, OPP, clock latency) as well to make it all work.

Signed-off-by Viresh Kumar <[email protected]>

The OPP properties, like "operating-points", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can create an OPP table.

Add such missing properties.

Fix other missing property (clock latency) as well to make it all work.

Signed-off-by Viresh Kumar <[email protected]>

---
 arch/arm/boot/dts/highbank.dts | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

Quoted 14 lines · Viresh Kumar

The OPP properties, like "operating-points", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can create an OPP table.

Add such missing properties.

Fix other missing property (clock latency) as well to make it all work.

Quoted 4 lines

Signed-off-by Viresh Kumar <[email protected]>

---
 arch/arm/boot/dts/highbank.dts | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

Acked-by Rob Herring <[email protected]>

ARM-SOC maintainers, Please apply this.

Quoted 20 lines · Rob Herring

Quoted 18 lines · Viresh Kumar

The OPP properties, like "operating-points", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can create an OPP table.

Add such missing properties.

Fix other missing property (clock latency) as well to make it all work.

Signed-off-by Viresh Kumar <[email protected]>

---
 arch/arm/boot/dts/highbank.dts | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

Quoted 3 lines

Acked-by Rob Herring <[email protected]>

ARM-SOC maintainers, Please apply this.

Applied to next/dt.

Quoted 23 lines · Olof Johansson

Quoted 22 lines · Rob Herring

Quoted 11 lines · Viresh Kumar

The OPP properties, like "operating-points", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can create an OPP table.

Add such missing properties.

Quoted 7 lines

Fix other missing property (clock latency) as well to make it all work.

Signed-off-by Viresh Kumar <[email protected]>

---
 arch/arm/boot/dts/highbank.dts | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

Acked-by Rob Herring <[email protected]>

Quoted 3 lines

Quoted 1 line

ARM-SOC maintainers, Please apply this.

Applied to next/dt.

Thanks Olof.

Can you also apply other patches in this series, that haven't been applied yet by SoC maintainers ? And same for the arm64 series..

The OPP properties, like "operating-points", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can create an OPP table.

Add such missing properties.

Fix other missing property (clock latency) as well to make it all work.

Signed-off-by Viresh Kumar <[email protected]>

---
 arch/arm/boot/dts/r8a7743.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

arch/arm/boot/dts/r8a7743.dtsi +9 -0

index 142949d7066f..e4fb31c4f0ee 100644
@@ -98,8 +98,17 @@
 			reg = <1>;
 			clock-frequency = <1500000000>;
 			clocks = <&cpg CPG_CORE R8A7743_CLK_Z>;
+			clock-latency = <300000>; /* 300 us */
 			power-domains = <&sysc R8A7743_PD_CA15_CPU1>;
 			next-level-cache = <&L2_CA15>;
+
+			/* kHz - uV - OPPs unknown yet */
+			operating-points = <1500000 1000000>,
+					   <1312500 1000000>,
+					   <1125000 1000000>,
+					   < 937500 1000000>,
+					   < 750000 1000000>,
+					   < 375000 1000000>;
 		};
 
 		L2_CA15: cache-controller-0 {

Signature · 1 line

2.15.0.194.g9af6a3dea062

[Cc Biju Das]

Quoted 10 lines · Viresh Kumar

The OPP properties, like "operating-points", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can create an OPP table.

Add such missing properties.

Quoted 4 lines

Fix other missing property (clock latency) as well to make it all work.

Signed-off-by Viresh Kumar <[email protected]>

Thanks, this looks good to me and it looks like it should have:

Fixes 0417814ea140 ("ARM: dts: r8a7743: Add OPP table for frequency scaling")

Quoted 18 lines · Simon Horman

[Cc Biju Das]

Quoted 14 lines · Viresh Kumar

The OPP properties, like "operating-points", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can create an OPP table.

Add such missing properties.

Fix other missing property (clock latency) as well to make it all work.

Signed-off-by Viresh Kumar <[email protected]>

Quoted 3 lines

Thanks, this looks good to me and it looks like it should have:

Fixes 0417814ea140 ("ARM: dts: r8a7743: Add OPP table for frequency scaling")

Sure.

Will you be picking this patch directly and send it part of your pull request ? Maybe add Fixes tag then only ?

-----Original Message-----
From: Simon Horman [mailto:[email protected]]
Sent: 28 May 2018 12:59
To: Viresh Kumar <[email protected]>
Cc: [email protected]; Magnus Damm <[email protected]>; Rob Herring <[email protected]>; Mark Rutland <[email protected]>; Vincent Guittot <[email protected]>; [email protected]; Daniel Lezcano <[email protected]>; [email protected]; linux- [email protected]; [email protected]; linux- [email protected]; Biju Das <[email protected]>
Subject: Re: [PATCH 13/15] arm: dts: r8a7743: Add missing OPP properties for CPUs

Quoted 24 lines · Viresh Kumar

Quoted 22 lines · Simon Horman

[Cc Biju Das]

Quoted 8 lines · Viresh Kumar

The OPP properties, like "operating-points", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can create an OPP table.

Quoted 6 lines

Add such missing properties.

Fix other missing property (clock latency) as well to make it all work.

Signed-off-by Viresh Kumar <[email protected]>

Thanks, this looks good to me and it looks like it should have:

Fixes 0417814ea140 ("ARM: dts: r8a7743: Add OPP table for frequency

scaling")

Quoted 4 lines

Sure.

Will you be picking this patch directly and send it part of your pull request ? Maybe add Fixes tag then only ?

Yes, that is my plan. I can handle adding the Fixes tag. But I'll wait to see if Bjiu has an feedback first.

Hi All,

I have tested this patch on RZ/G1M and I didn't find any issues. r8a7743 is similar to r8a7791. So I assume you will apply the same patch for other R-SoC devices as well.

Apart from this, maybe we need to update the OPP binding documentation. i.e., extend the operating- point usage to other cores in the cluster (Binding 1: operating-points).

The OPP properties, like "operating-points", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can create an OPP table.

Add such missing properties.

Fix other missing properties (like, clock latency, voltage tolerance, etc) as well to make it all work.

Signed-off-by Viresh Kumar <[email protected]>

The OPP properties, like "operating-points", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can create an OPP table.

Add such missing properties.

Fix other missing properties (like, clock latency, voltage tolerance, etc) as well to make it all work.

Signed-off-by Viresh Kumar <[email protected]>

The OPP properties, like "operating-points", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can create an OPP table.

Add such missing properties.

Fix other missing properties (like, clock latency, voltage tolerance, etc) as well to make it all work.

Signed-off-by Viresh Kumar <[email protected]>

The OPP properties, like "operating-points", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can create an OPP table.

Add such missing properties.

Fix other missing property (clock latency) as well to make it all work.

Signed-off-by Viresh Kumar <[email protected]>

---
 arch/arm/boot/dts/qcom-ipq4019.dtsi | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

The OPP properties, like "operating-points", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can create an OPP table.

Add such missing properties.

Fix other missing property (clock latency) as well to make it all work.

Signed-off-by Viresh Kumar <[email protected]>

---
 arch/arm/boot/dts/qcom-ipq4019.dtsi | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

arch/arm/boot/dts/qcom-ipq4019.dtsi +24 -1

index b25daf368012..8c202b359270 100644
@@ -75,6 +75,14 @@
                        reg = <0x1>;
                        clocks = <&gcc GCC_APPS_CLK_SRC>;
                        clock-frequency = <0>;
+                       operating-points = <
+                               /* kHz  uV (fixed) */
+                               48000   1100000
+                               200000  1100000
+                               500000  1100000
+                               666000  1100000
+                       >;
+                       clock-latency = <256000>;
                };
 
                cpu@2 {

@@ -86,6 +94,14 @@
                        reg = <0x2>;
                        clocks = <&gcc GCC_APPS_CLK_SRC>;
                        clock-frequency = <0>;
+                       operating-points = <
+                               /* kHz  uV (fixed) */
+                               48000   1100000
+                               200000  1100000
+                               500000  1100000
+                               666000  1100000
+                       >;
+                       clock-latency = <256000>;
                };
 
                cpu@3 {

@@ -97,6 +113,14 @@
                        reg = <0x3>;
                        clocks = <&gcc GCC_APPS_CLK_SRC>;
                        clock-frequency = <0>;
+                       operating-points = <
+                               /* kHz  uV (fixed) */
+                               48000   1100000
+                               200000  1100000
+                               500000  1100000
+                               666000  1100000
+                       >;
+                       clock-latency = <256000>;
                };
        };
 
--

2.15.0.194.g9af6a3dea062

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 10 lines · Viresh Kumar

The OPP properties, like "operating-points", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can create an OPP table.

Add such missing properties.

Quoted 4 lines

Fix other missing property (clock latency) as well to make it all work.

Signed-off-by Viresh Kumar <[email protected]>

FWIW,

Reviewed-by Amit Kucheria <[email protected]>

The OPP properties, like "operating-points", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can create an OPP table.

Add such missing properties.

Fix other missing properties (like clocks, supply, clock latency) as well to make it all work.

Signed-off-by Viresh Kumar <[email protected]>

---

?arch/arm/boot/dts/imx6dl.dtsi??????| 23 ++++++++++
?arch/arm/boot/dts/imx6q-cm-fx6.dts | 66 +++++++++++++++++++++++++++++
?arch/arm/boot/dts/imx6q.dtsi???????| 87 ++++++++++++++++++++++++++++++++++++--
?arch/arm/boot/dts/imx7d.dtsi???????|??5 +++

?4 files changed, 178 insertions(+), 3 deletions(-)

arch/arm/boot/dts/imx6dl.dtsi +0 -0

index b384913c34dd..cc8ffc42d128 100644
@@ -50,6 +50,29 @@
 > ?			device_type = "cpu";
 > ?			reg = <1>;
 > ?			next-level-cache = <&L2>;

Quoted 23 lines

(quoted diff) +8 -0

+			operating-points = <
+				/* kHz????uV */
+				996000??1250000
+				792000??1175000
+				396000??1150000
+			>;
+			fsl,soc-operating-points = <
+				/* ARM kHz??SOC-PU uV */

Quoted 3 lines

arch/arm/boot/dts/imx6dl.dtsi +3 -0

+				996000	1175000
+				792000	1175000
+				396000	1175000

arch/arm/boot/dts/imx6dl.dtsi +12 -0

+			>;
+			clock-latency = <61036>; /* two CLK32 periods */
+			clocks = <&clks IMX6QDL_CLK_ARM>,
+				?<&clks IMX6QDL_CLK_PLL2_PFD2_396M>,
+				?<&clks IMX6QDL_CLK_STEP>,
+				?<&clks IMX6QDL_CLK_PLL1_SW>,
+				?<&clks IMX6QDL_CLK_PLL1_SYS>;
+			clock-names = "arm", "pll2_pfd2_396m", "step",
+				??????"pll1_sw", "pll1_sys";
+			arm-supply = <&reg_arm>;
+			pu-supply = <&reg_pu>;
+			soc-supply = <&reg_soc>;

Quoted 2 lines

? };
? };

?

arch/arm/boot/dts/imx6q-cm-fx6.dts +0 -0

index 65ef4cacbc71..18ae4f3be6e3 100644
@@ -187,6 +187,72 @@
 > ?	>;
 ?};

?
+&cpu1 {

Quoted 19 lines

(quoted diff) +6 -0

+	/*
+	?* Although the imx6q fuse indicates that 1.2GHz operation is possible,
+	?* the module behaves unstable at this frequency. Hence, remove the
+	?* 1.2GHz operation point here.
+	?*/
+	operating-points = <

Quoted 5 lines

arch/arm/boot/dts/imx6q-cm-fx6.dts +5 -0

+		/* kHz	uV */
+		996000	1250000
+		852000	1250000
+		792000	1175000
+		396000	975000

(quoted diff) +2 -0

+	>;
+	fsl,soc-operating-points = <

Quoted 5 lines

arch/arm/boot/dts/imx6q-cm-fx6.dts +5 -0

+		/* ARM kHz	SOC-PU uV */
+		996000		1250000
+		852000		1250000
+		792000		1175000
+		396000		1175000

(quoted diff) +1 -0

+	>;

arch/arm/boot/dts/imx6q-cm-fx6.dts +3 -0

+};
+
+&cpu2 {

Quoted 19 lines

(quoted diff) +6 -0

+	/*
+	?* Although the imx6q fuse indicates that 1.2GHz operation is possible,
+	?* the module behaves unstable at this frequency. Hence, remove the
+	?* 1.2GHz operation point here.
+	?*/
+	operating-points = <

Quoted 5 lines

arch/arm/boot/dts/imx6q-cm-fx6.dts +5 -0

+		/* kHz	uV */
+		996000	1250000
+		852000	1250000
+		792000	1175000
+		396000	975000

(quoted diff) +2 -0

+	>;
+	fsl,soc-operating-points = <

Quoted 5 lines

arch/arm/boot/dts/imx6q-cm-fx6.dts +5 -0

+		/* ARM kHz	SOC-PU uV */
+		996000		1250000
+		852000		1250000
+		792000		1175000
+		396000		1175000

(quoted diff) +1 -0

+	>;

arch/arm/boot/dts/imx6q-cm-fx6.dts +3 -0

+};
+
+&cpu3 {

Quoted 19 lines

(quoted diff) +6 -0

+	/*
+	?* Although the imx6q fuse indicates that 1.2GHz operation is possible,
+	?* the module behaves unstable at this frequency. Hence, remove the
+	?* 1.2GHz operation point here.
+	?*/
+	operating-points = <

Quoted 5 lines

arch/arm/boot/dts/imx6q-cm-fx6.dts +5 -0

+		/* kHz	uV */
+		996000	1250000
+		852000	1250000
+		792000	1175000
+		396000	975000

(quoted diff) +2 -0

+	>;
+	fsl,soc-operating-points = <

Quoted 5 lines

arch/arm/boot/dts/imx6q-cm-fx6.dts +5 -0

+		/* ARM kHz	SOC-PU uV */
+		996000		1250000
+		852000		1250000
+		792000		1175000
+		396000		1175000

(quoted diff) +1 -0

+	>;

arch/arm/boot/dts/imx6q-cm-fx6.dts +2 -0

+};
+

?&ecspi1 {

Quoted 2 lines

? cs-gpios = <&gpio2 30 GPIO_ACTIVE_HIGH>, <&gpio3 19 GPIO_ACTIVE_HIGH>;
? pinctrl-names = "default";

arch/arm/boot/dts/imx6q.dtsi +0 -0

index 70483ce72ba6..78b89bb1bfed 100644
@@ -50,25 +50,106 @@
 > ?			soc-supply = <&reg_soc>;
 > ?		};

?

Quoted 33 lines

arch/arm/boot/dts/imx6q.dtsi +0 -1

-		cpu at 1 {

arch/arm/boot/dts/imx6q.dtsi line 53

Quoted 1 line

arch/arm/boot/dts/imx6q.dtsi +1 -0

+		cpu1: cpu at 1 {

? compatible = "arm,cortex-a9";
? device_type = "cpu";
? reg = <1>;
? next-level-cache = <&L2>;

(quoted diff) +11 -0

+			operating-points = <
+				/* kHz????uV */
+				1200000 1275000
+				996000??1250000
+				852000??1250000
+				792000??1175000
+				396000??975000
+			>;
+			fsl,soc-operating-points = <
+				/* ARM kHz??SOC-PU uV */
+				1200000 1275000

Quoted 4 lines

arch/arm/boot/dts/imx6q-cm-fx6.dts +4 -0

+				996000	1250000
+				852000	1250000
+				792000	1175000
+				396000	1175000

arch/arm/boot/dts/imx6dl.dtsi +12 -0

+			>;
+			clock-latency = <61036>; /* two CLK32 periods */
+			clocks = <&clks IMX6QDL_CLK_ARM>,
+				?<&clks IMX6QDL_CLK_PLL2_PFD2_396M>,
+				?<&clks IMX6QDL_CLK_STEP>,
+				?<&clks IMX6QDL_CLK_PLL1_SW>,
+				?<&clks IMX6QDL_CLK_PLL1_SYS>;
+			clock-names = "arm", "pll2_pfd2_396m", "step",
+				??????"pll1_sw", "pll1_sys";
+			arm-supply = <&reg_arm>;
+			pu-supply = <&reg_pu>;
+			soc-supply = <&reg_soc>;

Quoted 1 line

? };

?

Quoted 33 lines

arch/arm/boot/dts/imx6q.dtsi +0 -1

-		cpu at 2 {

arch/arm/boot/dts/imx6q.dtsi line 87

Quoted 1 line

arch/arm/boot/dts/imx6q.dtsi +1 -0

+		cpu2: cpu at 2 {

? compatible = "arm,cortex-a9";
? device_type = "cpu";
? reg = <2>;
? next-level-cache = <&L2>;

(quoted diff) +11 -0

+			operating-points = <
+				/* kHz????uV */
+				1200000 1275000
+				996000??1250000
+				852000??1250000
+				792000??1175000
+				396000??975000
+			>;
+			fsl,soc-operating-points = <
+				/* ARM kHz??SOC-PU uV */
+				1200000 1275000

Quoted 4 lines

arch/arm/boot/dts/imx6q-cm-fx6.dts +4 -0

+				996000	1250000
+				852000	1250000
+				792000	1175000
+				396000	1175000

arch/arm/boot/dts/imx6dl.dtsi +12 -0

+			>;
+			clock-latency = <61036>; /* two CLK32 periods */
+			clocks = <&clks IMX6QDL_CLK_ARM>,
+				?<&clks IMX6QDL_CLK_PLL2_PFD2_396M>,
+				?<&clks IMX6QDL_CLK_STEP>,
+				?<&clks IMX6QDL_CLK_PLL1_SW>,
+				?<&clks IMX6QDL_CLK_PLL1_SYS>;
+			clock-names = "arm", "pll2_pfd2_396m", "step",
+				??????"pll1_sw", "pll1_sys";
+			arm-supply = <&reg_arm>;
+			pu-supply = <&reg_pu>;
+			soc-supply = <&reg_soc>;

Quoted 1 line

? };

?

Quoted 33 lines

arch/arm/boot/dts/imx6q.dtsi +0 -1

-		cpu at 3 {

arch/arm/boot/dts/imx6q.dtsi line 121

Quoted 1 line

arch/arm/boot/dts/imx6q.dtsi +1 -0

+		cpu3: cpu at 3 {

? compatible = "arm,cortex-a9";
? device_type = "cpu";
? reg = <3>;
? next-level-cache = <&L2>;

(quoted diff) +11 -0

+			operating-points = <
+				/* kHz????uV */
+				1200000 1275000
+				996000??1250000
+				852000??1250000
+				792000??1175000
+				396000??975000
+			>;
+			fsl,soc-operating-points = <
+				/* ARM kHz??SOC-PU uV */
+				1200000 1275000

Quoted 4 lines

arch/arm/boot/dts/imx6q-cm-fx6.dts +4 -0

+				996000	1250000
+				852000	1250000
+				792000	1175000
+				396000	1175000

arch/arm/boot/dts/imx6dl.dtsi +12 -0

+			>;
+			clock-latency = <61036>; /* two CLK32 periods */
+			clocks = <&clks IMX6QDL_CLK_ARM>,
+				?<&clks IMX6QDL_CLK_PLL2_PFD2_396M>,
+				?<&clks IMX6QDL_CLK_STEP>,
+				?<&clks IMX6QDL_CLK_PLL1_SW>,
+				?<&clks IMX6QDL_CLK_PLL1_SYS>;
+			clock-names = "arm", "pll2_pfd2_396m", "step",
+				??????"pll1_sw", "pll1_sys";
+			arm-supply = <&reg_arm>;
+			pu-supply = <&reg_pu>;
+			soc-supply = <&reg_soc>;

Quoted 2 lines

? };
? };

?

arch/arm/boot/dts/imx7d.dtsi +0 -0

index 4c9877ec29f2..5434a8aa5602 100644
@@ -21,6 +21,11 @@
 > ?			compatible = "arm,cortex-a7";
 > ?			device_type = "cpu";
 > ?			reg = <1>;

Quoted 8 lines

(quoted diff) +1 -0

+			operating-points = <

Quoted 3 lines

arch/arm/boot/dts/imx7d.dtsi +3 -0

+				/* KHz	uV */
+				996000	1075000
+				792000	975000

(quoted diff) +1 -0

+			>;

? clock-frequency = <996000000>;
? };
? };

This is a lot of duplicate information for what is effectively a shared
cluster wide thing. This does absolutely not _feel_ right.

What problem are you solving here? Why do we need all this duplicate information? Why can't we fix it by falling back to looking at cpu0 if needed?

Hi Lucas,

Quoted 2 lines · Lucas Stach

This is a lot of duplicate information for what is effectively a shared
cluster wide thing. This does absolutely not _feel_ right.

I cannot agree more :)

Quoted 3 lines

What problem are you solving here? Why do we need all this duplicate information? Why can't we fix it by falling back to looking at cpu0 if needed?

Let me try explaining one of the problem scenarios to you as your platform is a single cluster one. Make cpufreq driver as module, don't insert it, hotplug out CPU0 and now insert the cpufreq driver. The cpufreq core will try adding the cpufreq policy for CPU1 but wouldn't find the required information in the DT node of CPU1 and so will fail or behave incorrectly.

We can't look at CPU0 as we don't know they are related at all. Nothing tells that to us. The right solution to fix the duplication is to move to OPP-v2 bindings, which allow us to create a single OPP table node and refer to it from all the CPU nodes. Because in case of imx platforms getting updated here, we use the old and some platforms specific frequency tables, we have to duplicate it everywhere.

But looking from DT otherwise, all the device should anyway have all the information required right in their node. That can be simplified with things like phandle to opp-v2 node, but still everything needs to be there. We shouldn't really rely on other CPU nodes to make it work. That would be an incomplete definition of the hardware IMHO.

Quoted 26 lines

Hi Lucas,

Quoted 2 lines · Lucas Stach

This is a lot of duplicate information for what is effectively a shared ?cluster wide thing. This does absolutely not _feel_ right.

I cannot agree more :)

Quoted 3 lines

What problem are you solving here? Why do we need all this duplicate information? Why can't we fix it by falling back to looking at cpu0 if needed?

Let me try explaining one of the problem scenarios to you as your platform is a single cluster one. Make cpufreq driver as module, don't insert it, hotplug out CPU0 and now insert the cpufreq driver. The cpufreq core will try adding the cpufreq policy for CPU1 but wouldn't find the required information in the DT node of CPU1 and so will fail or behave incorrectly.

We can't look at CPU0 as we don't know they are related at all. Nothing tells that to us. The right solution to fix the duplication is to move to OPP-v2 bindings, which allow us to create a single OPP table node and refer to it from all the CPU nodes. Because in case of imx platforms getting updated here, we use the old and some platforms specific frequency tables, we have to duplicate it everywhere.

Quoted 5 lines

But looking from DT otherwise, all the device should anyway have all the information required right in their node. That can be simplified with things like phandle to opp-v2 node, but still everything needs to be there. We shouldn't really rely on other CPU nodes to make it work. That would be an incomplete definition of the hardware IMHO.

Lucas,

Are you fine with the patch now considering the respond from Viresh?

The OPP properties, like "operating-points", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can create an OPP table.

Add such missing properties.

Fix other missing properties (like clocks, supply, clock latency) as well to make it all work.

Signed-off-by Viresh Kumar <[email protected]>