LinuxLists.cc - [PATCH 0/6] arm64: dts: Add missing cooling device properties for CPUs

2018-05-25 05:42:42

Subject: [PATCH 0/6] arm64: dts: Add missing cooling device properties for CPUs

Hello,

This fixes missing cooling device properties for CPUs for the ARM64
platforms. This is build tested by the zero day testing infrastructure
as well.

Individual maintainers can pick the patches to their SoC trees or I will
ask ARM SoC maintainers to pick them up later.

--
viresh

Viresh Kumar (6):
arm64: dts: amlogic: Add missing cooling device properties for CPUs
arm64: dts: freescale: Add missing cooling device properties for CPUs
arm64: dts: hisilicon: Add missing cooling device properties for CPUs
arm64: dts: mediatek: Add missing cooling device properties for CPUs
arm64: dts: rockchip: Add missing cooling device properties for CPUs
arm64: dts: socionext: Add missing cooling device properties for CPUs

.../boot/dts/amlogic/meson-gxm-khadas-vim2.dts | 24 ++++++++++++++++++++++
arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi | 5 ++++-
arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi | 3 +++
arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi | 6 ++++++
arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi | 4 ++++
arch/arm64/boot/dts/freescale/fsl-ls2088a.dtsi | 4 ++++
arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 16 ++++++++++++++-
arch/arm64/boot/dts/mediatek/mt7622.dtsi | 1 +
arch/arm64/boot/dts/mediatek/mt8173.dtsi | 2 ++
arch/arm64/boot/dts/rockchip/rk3328.dtsi | 3 +++
arch/arm64/boot/dts/rockchip/rk3368.dtsi | 12 +++++++++++
arch/arm64/boot/dts/rockchip/rk3399.dtsi | 8 ++++++--
arch/arm64/boot/dts/socionext/uniphier-ld20.dtsi | 2 ++
13 files changed, 86 insertions(+), 4 deletions(-)

--
2.15.0.194.g9af6a3dea062

2018-05-25 05:41:14

by Viresh Kumar

[permalink] [raw]

Subject: [PATCH 2/6] arm64: dts: freescale: Add missing cooling device properties for CPUs

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.

Do minor rearrangement as well to keep ordering consistent.

Signed-off-by: Viresh Kumar <[email protected]>
---
arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi | 5 ++++-
arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi | 3 +++
arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi | 6 ++++++
arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi | 4 ++++
arch/arm64/boot/dts/freescale/fsl-ls2088a.dtsi | 4 ++++
5 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
index 1109f22bda5e..630ee47441f2 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
@@ -80,8 +80,8 @@
reg = <0x0>;
clocks = <&clockgen 1 0>;
next-level-cache = <&l2>;
- #cooling-cells = <2>;
cpu-idle-states = <&CPU_PH20>;
+ #cooling-cells = <2>;
};

cpu1: cpu@1 {
@@ -91,6 +91,7 @@
clocks = <&clockgen 1 0>;
next-level-cache = <&l2>;
cpu-idle-states = <&CPU_PH20>;
+ #cooling-cells = <2>;
};

cpu2: cpu@2 {
@@ -100,6 +101,7 @@
clocks = <&clockgen 1 0>;
next-level-cache = <&l2>;
cpu-idle-states = <&CPU_PH20>;
+ #cooling-cells = <2>;
};

cpu3: cpu@3 {
@@ -109,6 +111,7 @@
clocks = <&clockgen 1 0>;
next-level-cache = <&l2>;
cpu-idle-states = <&CPU_PH20>;
+ #cooling-cells = <2>;
};

l2: l2-cache {
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi
index 136ebfa9b333..ee7beab8bfae 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi
@@ -87,6 +87,7 @@
clocks = <&clockgen 1 0>;
next-level-cache = <&l2>;
cpu-idle-states = <&CPU_PH20>;
+ #cooling-cells = <2>;
};

cpu2: cpu@2 {
@@ -96,6 +97,7 @@
clocks = <&clockgen 1 0>;
next-level-cache = <&l2>;
cpu-idle-states = <&CPU_PH20>;
+ #cooling-cells = <2>;
};

cpu3: cpu@3 {
@@ -105,6 +107,7 @@
clocks = <&clockgen 1 0>;
next-level-cache = <&l2>;
cpu-idle-states = <&CPU_PH20>;
+ #cooling-cells = <2>;
};

l2: l2-cache {
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi
index 1c6556bcfddf..e64823a25158 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi
@@ -76,6 +76,7 @@
reg = <0x1>;
clocks = <&clockgen 1 0>;
cpu-idle-states = <&CPU_PH20>;
+ #cooling-cells = <2>;
};

cpu2: cpu@2 {
@@ -84,6 +85,7 @@
reg = <0x2>;
clocks = <&clockgen 1 0>;
cpu-idle-states = <&CPU_PH20>;
+ #cooling-cells = <2>;
};

cpu3: cpu@3 {
@@ -92,6 +94,7 @@
reg = <0x3>;
clocks = <&clockgen 1 0>;
cpu-idle-states = <&CPU_PH20>;
+ #cooling-cells = <2>;
};

cpu4: cpu@100 {
@@ -109,6 +112,7 @@
reg = <0x101>;
clocks = <&clockgen 1 1>;
cpu-idle-states = <&CPU_PH20>;
+ #cooling-cells = <2>;
};

cpu6: cpu@102 {
@@ -117,6 +121,7 @@
reg = <0x102>;
clocks = <&clockgen 1 1>;
cpu-idle-states = <&CPU_PH20>;
+ #cooling-cells = <2>;
};

cpu7: cpu@103 {
@@ -125,6 +130,7 @@
reg = <0x103>;
clocks = <&clockgen 1 1>;
cpu-idle-states = <&CPU_PH20>;
+ #cooling-cells = <2>;
};

CPU_PH20: cpu-ph20 {
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi
index 8d739301e7b8..c264b6d1bd7f 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi
@@ -65,6 +65,7 @@
clocks = <&clockgen 1 0>;
cpu-idle-states = <&CPU_PW20>;
next-level-cache = <&cluster0_l2>;
+ #cooling-cells = <2>;
};

cpu2: cpu@100 {
@@ -84,6 +85,7 @@
clocks = <&clockgen 1 1>;
cpu-idle-states = <&CPU_PW20>;
next-level-cache = <&cluster1_l2>;
+ #cooling-cells = <2>;
};

cpu4: cpu@200 {
@@ -103,6 +105,7 @@
clocks = <&clockgen 1 2>;
cpu-idle-states = <&CPU_PW20>;
next-level-cache = <&cluster2_l2>;
+ #cooling-cells = <2>;
};

cpu6: cpu@300 {
@@ -122,6 +125,7 @@
clocks = <&clockgen 1 3>;
cpu-idle-states = <&CPU_PW20>;
next-level-cache = <&cluster3_l2>;
+ #cooling-cells = <2>;
};

cluster0_l2: l2-cache0 {
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls2088a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls2088a.dtsi
index 0884e1a77901..b6ea9e96c866 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls2088a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls2088a.dtsi
@@ -65,6 +65,7 @@
clocks = <&clockgen 1 0>;
cpu-idle-states = <&CPU_PW20>;
next-level-cache = <&cluster0_l2>;
+ #cooling-cells = <2>;
};

cpu2: cpu@100 {
@@ -84,6 +85,7 @@
clocks = <&clockgen 1 1>;
cpu-idle-states = <&CPU_PW20>;
next-level-cache = <&cluster1_l2>;
+ #cooling-cells = <2>;
};

cpu4: cpu@200 {
@@ -103,6 +105,7 @@
clocks = <&clockgen 1 2>;
cpu-idle-states = <&CPU_PW20>;
next-level-cache = <&cluster2_l2>;
+ #cooling-cells = <2>;
};

cpu6: cpu@300 {
@@ -122,6 +125,7 @@
clocks = <&clockgen 1 3>;
cpu-idle-states = <&CPU_PW20>;
next-level-cache = <&cluster3_l2>;
+ #cooling-cells = <2>;
};

cluster0_l2: l2-cache0 {
--
2.15.0.194.g9af6a3dea062

2018-05-25 05:41:21

by Viresh Kumar

[permalink] [raw]

Subject: [PATCH 4/6] arm64: dts: mediatek: Add missing cooling device properties for CPUs

2018-05-25 05:41:25

by Viresh Kumar

[permalink] [raw]

Subject: [PATCH 6/6] arm64: dts: socionext: Add missing cooling device properties for CPUs

2018-05-25 05:42:04

by Viresh Kumar

[permalink] [raw]

Subject: [PATCH 5/6] arm64: dts: rockchip: Add missing cooling device properties for CPUs

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.

Do minor rearrangement as well to keep ordering consistent.

Signed-off-by: Viresh Kumar <[email protected]>
---
arch/arm64/boot/dts/rockchip/rk3328.dtsi | 3 +++
arch/arm64/boot/dts/rockchip/rk3368.dtsi | 12 ++++++++++++
arch/arm64/boot/dts/rockchip/rk3399.dtsi | 8 ++++++--
3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
index b8e9da15e00c..902a0907ad34 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
@@ -89,6 +89,7 @@
compatible = "arm,cortex-a53", "arm,armv8";
reg = <0x0 0x1>;
clocks = <&cru ARMCLK>;
+ #cooling-cells = <2>;
dynamic-power-coefficient = <120>;
enable-method = "psci";
next-level-cache = <&l2>;
@@ -100,6 +101,7 @@
compatible = "arm,cortex-a53", "arm,armv8";
reg = <0x0 0x2>;
clocks = <&cru ARMCLK>;
+ #cooling-cells = <2>;
dynamic-power-coefficient = <120>;
enable-method = "psci";
next-level-cache = <&l2>;
@@ -111,6 +113,7 @@
compatible = "arm,cortex-a53", "arm,armv8";
reg = <0x0 0x3>;
clocks = <&cru ARMCLK>;
+ #cooling-cells = <2>;
dynamic-power-coefficient = <120>;
enable-method = "psci";
next-level-cache = <&l2>;
diff --git a/arch/arm64/boot/dts/rockchip/rk3368.dtsi b/arch/arm64/boot/dts/rockchip/rk3368.dtsi
index ad91ced78649..c32f2a551a1f 100644
--- a/arch/arm64/boot/dts/rockchip/rk3368.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3368.dtsi
@@ -122,6 +122,8 @@
compatible = "arm,cortex-a53", "arm,armv8";
reg = <0x0 0x1>;
enable-method = "psci";
+
+ #cooling-cells = <2>; /* min followed by max */
};

cpu_l2: cpu@2 {
@@ -129,6 +131,8 @@
compatible = "arm,cortex-a53", "arm,armv8";
reg = <0x0 0x2>;
enable-method = "psci";
+
+ #cooling-cells = <2>; /* min followed by max */
};

cpu_l3: cpu@3 {
@@ -136,6 +140,8 @@
compatible = "arm,cortex-a53", "arm,armv8";
reg = <0x0 0x3>;
enable-method = "psci";
+
+ #cooling-cells = <2>; /* min followed by max */
};

cpu_b0: cpu@100 {
@@ -152,6 +158,8 @@
compatible = "arm,cortex-a53", "arm,armv8";
reg = <0x0 0x101>;
enable-method = "psci";
+
+ #cooling-cells = <2>; /* min followed by max */
};

cpu_b2: cpu@102 {
@@ -159,6 +167,8 @@
compatible = "arm,cortex-a53", "arm,armv8";
reg = <0x0 0x102>;
enable-method = "psci";
+
+ #cooling-cells = <2>; /* min followed by max */
};

cpu_b3: cpu@103 {
@@ -166,6 +176,8 @@
compatible = "arm,cortex-a53", "arm,armv8";
reg = <0x0 0x103>;
enable-method = "psci";
+
+ #cooling-cells = <2>; /* min followed by max */
};
};

diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index e0040b648f43..da935383a8f2 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -108,8 +108,8 @@
compatible = "arm,cortex-a53", "arm,armv8";
reg = <0x0 0x0>;
enable-method = "psci";
- #cooling-cells = <2>; /* min followed by max */
clocks = <&cru ARMCLKL>;
+ #cooling-cells = <2>; /* min followed by max */
dynamic-power-coefficient = <100>;
};

@@ -119,6 +119,7 @@
reg = <0x0 0x1>;
enable-method = "psci";
clocks = <&cru ARMCLKL>;
+ #cooling-cells = <2>; /* min followed by max */
dynamic-power-coefficient = <100>;
};

@@ -128,6 +129,7 @@
reg = <0x0 0x2>;
enable-method = "psci";
clocks = <&cru ARMCLKL>;
+ #cooling-cells = <2>; /* min followed by max */
dynamic-power-coefficient = <100>;
};

@@ -137,6 +139,7 @@
reg = <0x0 0x3>;
enable-method = "psci";
clocks = <&cru ARMCLKL>;
+ #cooling-cells = <2>; /* min followed by max */
dynamic-power-coefficient = <100>;
};

@@ -145,8 +148,8 @@
compatible = "arm,cortex-a72", "arm,armv8";
reg = <0x0 0x100>;
enable-method = "psci";
- #cooling-cells = <2>; /* min followed by max */
clocks = <&cru ARMCLKB>;
+ #cooling-cells = <2>; /* min followed by max */
dynamic-power-coefficient = <436>;
};

@@ -156,6 +159,7 @@
reg = <0x0 0x101>;
enable-method = "psci";
clocks = <&cru ARMCLKB>;
+ #cooling-cells = <2>; /* min followed by max */
dynamic-power-coefficient = <436>;
};
};
--
2.15.0.194.g9af6a3dea062

2018-05-25 05:42:21

by Viresh Kumar

[permalink] [raw]

Subject: [PATCH 3/6] arm64: dts: hisilicon: Add missing cooling device properties for CPUs

2018-05-25 05:43:37

by Viresh Kumar

[permalink] [raw]

Subject: [PATCH 1/6] arm64: dts: amlogic: Add missing cooling device properties for CPUs

2018-05-25 21:37:22

by Olof Johansson

[permalink] [raw]

Subject: Re: [PATCH 1/6] arm64: dts: amlogic: Add missing cooling device properties for CPUs

On Fri, May 25, 2018 at 11:10:01AM +0530, Viresh Kumar wrote:
> 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.

This seems awkward compared to just having one cooling-cells in the /cpus node
instead.

What's it used for? I don't see any properties in the device nodes on meson-gxm
that have any cooling-foo cells in them? So why should #cooling-cells be
needed?

-Olof

2018-05-26 08:38:37

by Neil Armstrong

[permalink] [raw]

Subject: Re: [PATCH 1/6] arm64: dts: amlogic: Add missing cooling device properties for CPUs

Hi,

On 25/05/2018 23:10, Olof Johansson wrote:
> On Fri, May 25, 2018 at 11:10:01AM +0530, Viresh Kumar wrote:
>> 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.
>
> This seems awkward compared to just having one cooling-cells in the /cpus node
> instead.
>
> What's it used for? I don't see any properties in the device nodes on meson-gxm
> that have any cooling-foo cells in them? So why should #cooling-cells be
> needed?

There is no reason to have the cooling-cells on these other CPUs, the DVFS is
controlled on the first CPU of each cluster, here cpu0 and cpu4 and only
cpu0 and cpu4 are used as cooling-cells.

Neil

>
>
> -Olof
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

2018-05-26 18:01:05

by Wei Xu

[permalink] [raw]

Subject: Re: [PATCH 3/6] arm64: dts: hisilicon: Add missing cooling device properties for CPUs

Hi Viresh,

On 2018/5/25 6:40, Viresh Kumar wrote:
> 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.
>
> Do minor rearrangement as well to keep ordering consistent.
>
> Signed-off-by: Viresh Kumar <[email protected]>

Thanks!
Applied to the hisilicon fix tree.

Best Regards,
Wei

> ---
> arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> index 586b281cd531..247024df714f 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> @@ -88,8 +88,8 @@
> next-level-cache = <&CLUSTER0_L2>;
> clocks = <&stub_clock 0>;
> operating-points-v2 = <&cpu_opp_table>;
> - #cooling-cells = <2>; /* min followed by max */
> cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
> + #cooling-cells = <2>; /* min followed by max */
> dynamic-power-coefficient = <311>;
> };
>
> @@ -101,6 +101,8 @@
> next-level-cache = <&CLUSTER0_L2>;
> operating-points-v2 = <&cpu_opp_table>;
> cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
> + #cooling-cells = <2>; /* min followed by max */
> + dynamic-power-coefficient = <311>;
> };
>
> cpu2: cpu@2 {
> @@ -111,6 +113,8 @@
> next-level-cache = <&CLUSTER0_L2>;
> operating-points-v2 = <&cpu_opp_table>;
> cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
> + #cooling-cells = <2>; /* min followed by max */
> + dynamic-power-coefficient = <311>;
> };
>
> cpu3: cpu@3 {
> @@ -121,6 +125,8 @@
> next-level-cache = <&CLUSTER0_L2>;
> operating-points-v2 = <&cpu_opp_table>;
> cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
> + #cooling-cells = <2>; /* min followed by max */
> + dynamic-power-coefficient = <311>;
> };
>
> cpu4: cpu@100 {
> @@ -131,6 +137,8 @@
> next-level-cache = <&CLUSTER1_L2>;
> operating-points-v2 = <&cpu_opp_table>;
> cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
> + #cooling-cells = <2>; /* min followed by max */
> + dynamic-power-coefficient = <311>;
> };
>
> cpu5: cpu@101 {
> @@ -141,6 +149,8 @@
> next-level-cache = <&CLUSTER1_L2>;
> operating-points-v2 = <&cpu_opp_table>;
> cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
> + #cooling-cells = <2>; /* min followed by max */
> + dynamic-power-coefficient = <311>;
> };
>
> cpu6: cpu@102 {
> @@ -151,6 +161,8 @@
> next-level-cache = <&CLUSTER1_L2>;
> operating-points-v2 = <&cpu_opp_table>;
> cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
> + #cooling-cells = <2>; /* min followed by max */
> + dynamic-power-coefficient = <311>;
> };
>
> cpu7: cpu@103 {
> @@ -161,6 +173,8 @@
> next-level-cache = <&CLUSTER1_L2>;
> operating-points-v2 = <&cpu_opp_table>;
> cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
> + #cooling-cells = <2>; /* min followed by max */
> + dynamic-power-coefficient = <311>;
> };
>
> CLUSTER0_L2: l2-cache0 {
>

2018-05-26 18:23:03

by Wei Xu

[permalink] [raw]

Subject: Re: [PATCH 3/6] arm64: dts: hisilicon: Add missing cooling device properties for CPUs

Hi Viresh,

On 2018/5/26 19:00, Wei Xu wrote:
> Hi Viresh,
>
> On 2018/5/25 6:40, Viresh Kumar wrote:
>> 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.
>>
>> Do minor rearrangement as well to keep ordering consistent.
>>
>> Signed-off-by: Viresh Kumar <[email protected]>
>
> Thanks!
> Applied to the hisilicon fix tree.

Sorry for the noise!
It seems this patch is still under discussion.
I will drop it firstly.

Best Regards,
Wei

>
> Best Regards,
> Wei
>
>> ---
>> arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 16 +++++++++++++++-
>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
>> index 586b281cd531..247024df714f 100644
>> --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
>> +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
>> @@ -88,8 +88,8 @@
>> next-level-cache = <&CLUSTER0_L2>;
>> clocks = <&stub_clock 0>;
>> operating-points-v2 = <&cpu_opp_table>;
>> - #cooling-cells = <2>; /* min followed by max */
>> cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
>> + #cooling-cells = <2>; /* min followed by max */
>> dynamic-power-coefficient = <311>;
>> };
>>
>> @@ -101,6 +101,8 @@
>> next-level-cache = <&CLUSTER0_L2>;
>> operating-points-v2 = <&cpu_opp_table>;
>> cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
>> + #cooling-cells = <2>; /* min followed by max */
>> + dynamic-power-coefficient = <311>;
>> };
>>
>> cpu2: cpu@2 {
>> @@ -111,6 +113,8 @@
>> next-level-cache = <&CLUSTER0_L2>;
>> operating-points-v2 = <&cpu_opp_table>;
>> cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
>> + #cooling-cells = <2>; /* min followed by max */
>> + dynamic-power-coefficient = <311>;
>> };
>>
>> cpu3: cpu@3 {
>> @@ -121,6 +125,8 @@
>> next-level-cache = <&CLUSTER0_L2>;
>> operating-points-v2 = <&cpu_opp_table>;
>> cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
>> + #cooling-cells = <2>; /* min followed by max */
>> + dynamic-power-coefficient = <311>;
>> };
>>
>> cpu4: cpu@100 {
>> @@ -131,6 +137,8 @@
>> next-level-cache = <&CLUSTER1_L2>;
>> operating-points-v2 = <&cpu_opp_table>;
>> cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
>> + #cooling-cells = <2>; /* min followed by max */
>> + dynamic-power-coefficient = <311>;
>> };
>>
>> cpu5: cpu@101 {
>> @@ -141,6 +149,8 @@
>> next-level-cache = <&CLUSTER1_L2>;
>> operating-points-v2 = <&cpu_opp_table>;
>> cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
>> + #cooling-cells = <2>; /* min followed by max */
>> + dynamic-power-coefficient = <311>;
>> };
>>
>> cpu6: cpu@102 {
>> @@ -151,6 +161,8 @@
>> next-level-cache = <&CLUSTER1_L2>;
>> operating-points-v2 = <&cpu_opp_table>;
>> cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
>> + #cooling-cells = <2>; /* min followed by max */
>> + dynamic-power-coefficient = <311>;
>> };
>>
>> cpu7: cpu@103 {
>> @@ -161,6 +173,8 @@
>> next-level-cache = <&CLUSTER1_L2>;
>> operating-points-v2 = <&cpu_opp_table>;
>> cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
>> + #cooling-cells = <2>; /* min followed by max */
>> + dynamic-power-coefficient = <311>;
>> };
>>
>> CLUSTER0_L2: l2-cache0 {
>>

2018-05-28 11:19:15

by Viresh Kumar

[permalink] [raw]

Subject: Re: [PATCH 1/6] arm64: dts: amlogic: Add missing cooling device properties for CPUs

On 26-05-18, 10:37, Neil Armstrong wrote:
> Hi,
>
> On 25/05/2018 23:10, Olof Johansson wrote:
> > On Fri, May 25, 2018 at 11:10:01AM +0530, Viresh Kumar wrote:
> >> 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.
> >
> > This seems awkward compared to just having one cooling-cells in the /cpus node
> > instead.
> >
> > What's it used for? I don't see any properties in the device nodes on meson-gxm
> > that have any cooling-foo cells in them? So why should #cooling-cells be
> > needed?
>
>
> There is no reason to have the cooling-cells on these other CPUs, the DVFS is
> controlled on the first CPU of each cluster, here cpu0 and cpu4 and only
> cpu0 and cpu4 are used as cooling-cells.

First, this is an incomplete definition of the hardware as all the
CPUs are cooling-devices here and DT shouldn't be written assuming how
OS will interpret it.

And then it is broken right now. You can offline your second cluster
(4567 CPUs) and bring CPU5 up first. You will see things breaking.

I have explained more in detail here.

https://marc.info/?l=linux-kernel&m=152750569414761

--
viresh

2018-05-28 11:34:34

by Viresh Kumar

[permalink] [raw]

Subject: Re: [PATCH 1/6] arm64: dts: amlogic: Add missing cooling device properties for CPUs

On 25-05-18, 14:10, Olof Johansson wrote:
> On Fri, May 25, 2018 at 11:10:01AM +0530, Viresh Kumar wrote:
> > 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.
>
> This seems awkward compared to just having one cooling-cells in the /cpus node
> instead.

Well, we don't allow that property to be present in /cpus node right
now and it is per device. And then we may not want all the CPUs to be
cooling devices really.

> What's it used for? I don't see any properties in the device nodes on meson-gxm
> that have any cooling-foo cells in them? So why should #cooling-cells be
> needed?

This property is required to declare a device as a cooling-device and
the device here is CPU. We use it as a cooling device by limiting its
higher range of frequencies, so that it doesn't generate too much
heat.

It is already there for CPU0 and CPU4, but it should really be there
for all the CPUs, like we have clock, supply, caches, etc.

--
viresh

2018-06-02 08:48:03

by Olof Johansson

[permalink] [raw]

Subject: Re: [PATCH 1/6] arm64: dts: amlogic: Add missing cooling device properties for CPUs

On Mon, May 28, 2018 at 04:43:58PM +0530, Viresh Kumar wrote:
> On 25-05-18, 14:10, Olof Johansson wrote:
> > On Fri, May 25, 2018 at 11:10:01AM +0530, Viresh Kumar wrote:
> > > 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.
> >
> > This seems awkward compared to just having one cooling-cells in the /cpus node
> > instead.
>
> Well, we don't allow that property to be present in /cpus node right
> now and it is per device. And then we may not want all the CPUs to be
> cooling devices really.

And what I am saying is that it sounds like a broken binding if you don't allow
that, especially since it'll be a super common case that all CPUs will specify
the same cooling-device specifier.

> > What's it used for? I don't see any properties in the device nodes on meson-gxm
> > that have any cooling-foo cells in them? So why should #cooling-cells be
> > needed?
>
> This property is required to declare a device as a cooling-device and
> the device here is CPU. We use it as a cooling device by limiting its
> higher range of frequencies, so that it doesn't generate too much
> heat.
>
> It is already there for CPU0 and CPU4, but it should really be there
> for all the CPUs, like we have clock, supply, caches, etc.

You have #cooling-cells in the cpu node, but the actual data is in the
thermal-zones nodes. Why isn't #cooling-cells under thermal-zones, next to
cooling-maps?

-Olof

2018-06-04 07:15:20

by Viresh Kumar

[permalink] [raw]

Subject: Re: [PATCH 6/6] arm64: dts: socionext: Add missing cooling device properties for CPUs

On 01-06-18, 12:25, Masahiro Yamada wrote:
> 2018-05-25 14:40 GMT+09:00 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]>
>
>
> Applied to linux-uniphier.
>
> I had already sent a PR for v4.18-rc1 before I received this patch.
> Please wait for v4.19-rc1.

Sure, no hurry. Though I thought this kind of fixes can go in 4.18-rc2 as well,
isn't it ?

--
viresh

2018-06-05 04:37:57

by Viresh Kumar

[permalink] [raw]

Subject: Re: [PATCH 1/6] arm64: dts: amlogic: Add missing cooling device properties for CPUs

On 02-06-18, 01:14, Olof Johansson wrote:
> And what I am saying is that it sounds like a broken binding if you don't allow
> that, especially since it'll be a super common case that all CPUs will specify
> the same cooling-device specifier.

I am fine with allowing the #cooling-cells property in the cpus node if the DT
maintainers are fine with it.

@Rob: comments ?

@Olof: What about other properties which are still going to be duplicated for
the most common cases today, like: clocks, supply information, cache
information, cpu-idle-states and others. When we can duplicate these properties,
why not keep following the same for #cpu-cooling property ?

Note that the OPP table doesn't really need to get duplicated (for new
platforms) as the platforms use the v2 bindings now which just duplicates a
phandle assignment for all CPUs. Its a mess with older platforms which use the
earlier version of OPP table.

> > This property is required to declare a device as a cooling-device and
> > the device here is CPU. We use it as a cooling device by limiting its
> > higher range of frequencies, so that it doesn't generate too much
> > heat.
> >
> > It is already there for CPU0 and CPU4, but it should really be there
> > for all the CPUs, like we have clock, supply, caches, etc.
>
> You have #cooling-cells in the cpu node, but the actual data is in the
> thermal-zones nodes. Why isn't #cooling-cells under thermal-zones, next to
> cooling-maps?

Actually I thought about that when I worked on these patches initially and this
is why I felt convinced that the CPU nodes are the right place for this.

We add #interrupt-cells to an Interrupt controller's DT node, #gpio-cells to a
GPIO controller's DT node, #clock-cells to a clock controller's DT node and
that's exactly why we should (and we do) add #cooling-cells property to a
cooling device's DT node. This information is used in two ways, first it enables
the OS to know that the device is capable of being a cooling device and second
it tells us how many arguments will be required with a phandle of this device.

And so the cooling-maps always contain two arguments with the cooling device's
phandle (which is mostly a CPU or a gpio fan) as the #cooling-cells currently
is fixed to 2.

And so I am not really sure if thermal-zones is the right place to define this
thing. Is my understanding correct ?

--
viresh

2018-06-15 11:24:47

by Heiko Stuebner

[permalink] [raw]

Subject: Re: [PATCH 5/6] arm64: dts: rockchip: Add missing cooling device properties for CPUs

Am Freitag, 25. Mai 2018, 07:40:05 CEST schrieb 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.
>
> Do minor rearrangement as well to keep ordering consistent.
>
> Signed-off-by: Viresh Kumar <[email protected]>

applied for 4.19

Thanks
Heiko

2018-07-02 16:57:48

by Matthias Brugger

[permalink] [raw]

Subject: Re: [PATCH 4/6] arm64: dts: mediatek: Add missing cooling device properties for CPUs

On 25/05/18 07:40, Viresh Kumar wrote:
> 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]>

pushed now to v4.18-next/dts64

> ---
> arch/arm64/boot/dts/mediatek/mt7622.dtsi | 1 +
> arch/arm64/boot/dts/mediatek/mt8173.dtsi | 2 ++
> 2 files changed, 3 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/mediatek/mt7622.dtsi b/arch/arm64/boot/dts/mediatek/mt7622.dtsi
> index 9213c966c224..d49fe125e770 100644
> --- a/arch/arm64/boot/dts/mediatek/mt7622.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt7622.dtsi
> @@ -89,6 +89,7 @@
> <&apmixedsys CLK_APMIXED_MAIN_CORE_EN>;
> clock-names = "cpu", "intermediate";
> operating-points-v2 = <&cpu_opp_table>;
> + #cooling-cells = <2>;
> enable-method = "psci";
> clock-frequency = <1300000000>;
> };
> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> index 94597e33c806..abd2f15a544b 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> @@ -168,6 +168,7 @@
> reg = <0x001>;
> enable-method = "psci";
> cpu-idle-states = <&CPU_SLEEP_0>;
> + #cooling-cells = <2>;
> clocks = <&infracfg CLK_INFRA_CA53SEL>,
> <&apmixedsys CLK_APMIXED_MAINPLL>;
> clock-names = "cpu", "intermediate";
> @@ -193,6 +194,7 @@
> reg = <0x101>;
> enable-method = "psci";
> cpu-idle-states = <&CPU_SLEEP_0>;
> + #cooling-cells = <2>;
> clocks = <&infracfg CLK_INFRA_CA57SEL>,
> <&apmixedsys CLK_APMIXED_MAINPLL>;
> clock-names = "cpu", "intermediate";
>

2018-07-03 07:35:39

by Shawn Guo

[permalink] [raw]

Subject: Re: [PATCH 2/6] arm64: dts: freescale: Add missing cooling device properties for CPUs

On Fri, May 25, 2018 at 1:40 PM Viresh Kumar <[email protected]> wrote:
>
> 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.
>
> Do minor rearrangement as well to keep ordering consistent.
>
> Signed-off-by: Viresh Kumar <[email protected]>

Applied, thanks.

Please use my kernel.org email address for future patches.

Shawn

2018-07-03 08:36:17

by Viresh Kumar

[permalink] [raw]

Subject: Re: [PATCH 2/6] arm64: dts: freescale: Add missing cooling device properties for CPUs

On 03-07-18, 15:34, Shawn Guo wrote:
> Please use my kernel.org email address for future patches.

As there were many patches I relied on get_maintainers to do that
stuff and it didn't pick you up by default. Maybe try fixing
MAINTAINERS to add an entry against your email id ?

--
viresh

2018-07-03 10:10:25

by Shawn Guo

[permalink] [raw]

Subject: Re: [PATCH 2/6] arm64: dts: freescale: Add missing cooling device properties for CPUs

On Tue, Jul 03, 2018 at 02:04:31PM +0530, Viresh Kumar wrote:
> On 03-07-18, 15:34, Shawn Guo wrote:
> > Please use my kernel.org email address for future patches.
>
> As there were many patches I relied on get_maintainers to do that
> stuff and it didn't pick you up by default. Maybe try fixing
> MAINTAINERS to add an entry against your email id ?

Good point. Will patch MAINTAINERS for arm64 freescale DTS.

Shawn

2018-07-18 05:59:16

by Viresh Kumar

[permalink] [raw]

Subject: Re: [PATCH 3/6] arm64: dts: hisilicon: Add missing cooling device properties for CPUs

On 26-05-18, 19:21, Wei Xu wrote:
> Hi Viresh,
>
> On 2018/5/26 19:00, Wei Xu wrote:
> > Hi Viresh,
> >
> > On 2018/5/25 6:40, Viresh Kumar wrote:
> >> 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.
> >>
> >> Do minor rearrangement as well to keep ordering consistent.
> >>
> >> Signed-off-by: Viresh Kumar <[email protected]>
> >
> > Thanks!
> > Applied to the hisilicon fix tree.
>
> Sorry for the noise!
> It seems this patch is still under discussion.
> I will drop it firstly.

Wei, can you please apply it again now that all the discussions are
over ?

--
viresh

2018-07-18 15:34:04

by Wei Xu

[permalink] [raw]