[PATCH 0/3] arm64: zynqmp: Add thermal zones

[PATCH 0/3] arm64: zynqmp: Add thermal zones

[Sean Anderson]

At the moment, the ZynqMP Analog Monitoring System (AMS) is only used
sporadically. As it is built into the SoC and doesn't depend on external
hardware, it can be exposed to userspace for all boards. Additionally,
we can use it to implement thermal zones.


Sean Anderson (3):
  arm64: zynqmp: Enable AMS for all boards
  arm64: zynqmp: Expose AMS to userspace as HWMON
  arm64: zynqmp: Add thermal zones

 .../boot/dts/xilinx/zynqmp-sm-k26-revA.dts    |  18 ----
 .../boot/dts/xilinx/zynqmp-zcu100-revC.dts    |   4 -
 .../boot/dts/xilinx/zynqmp-zcu102-revA.dts    |   4 -
 .../boot/dts/xilinx/zynqmp-zcu104-revA.dts    |   4 -
 .../boot/dts/xilinx/zynqmp-zcu104-revC.dts    |   4 -
 arch/arm64/boot/dts/xilinx/zynqmp.dtsi        | 101 +++++++++++++++++-
 6 files changed, 100 insertions(+), 35 deletions(-)

-- 
2.35.1.1320.gc452695387.dirty

[PATCH 1/3] arm64: zynqmp: Enable AMS for all boards

[Sean Anderson]

The AMS does not rely on external hardware or features, so it can be
enabled unconditionally.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 arch/arm64/boot/dts/xilinx/zynqmp-sm-k26-revA.dts | 4 ----
 arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts | 4 ----
 arch/arm64/boot/dts/xilinx/zynqmp-zcu102-revA.dts | 4 ----
 arch/arm64/boot/dts/xilinx/zynqmp-zcu104-revA.dts | 4 ----
 arch/arm64/boot/dts/xilinx/zynqmp-zcu104-revC.dts | 4 ----
 arch/arm64/boot/dts/xilinx/zynqmp.dtsi            | 1 -
 6 files changed, 21 deletions(-)

diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-sm-k26-revA.dts b/arch/arm64/boot/dts/xilinx/zynqmp-sm-k26-revA.dts
index 86e6c4990560..4e0e7fdf29ca 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp-sm-k26-revA.dts
+++ b/arch/arm64/boot/dts/xilinx/zynqmp-sm-k26-revA.dts
@@ -366,10 +366,6 @@ &gpio {
 			  "", "", "", ""; /* 170 - 173 */
 };
 
-&xilinx_ams {
-	status = "okay";
-};
-
 &ams_ps {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts b/arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts
index c5945067cd57..62c2503a502a 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts
+++ b/arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts
@@ -590,10 +590,6 @@ &watchdog0 {
 	status = "okay";
 };
 
-&xilinx_ams {
-	status = "okay";
-};
-
 &ams_ps {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-zcu102-revA.dts b/arch/arm64/boot/dts/xilinx/zynqmp-zcu102-revA.dts
index ad8f23a0ec67..ec1d3be703c0 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp-zcu102-revA.dts
+++ b/arch/arm64/boot/dts/xilinx/zynqmp-zcu102-revA.dts
@@ -1027,10 +1027,6 @@ &watchdog0 {
 	status = "okay";
 };
 
-&xilinx_ams {
-	status = "okay";
-};
-
 &ams_ps {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-zcu104-revA.dts b/arch/arm64/boot/dts/xilinx/zynqmp-zcu104-revA.dts
index b1eca1bb6a63..eb2090673ec1 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp-zcu104-revA.dts
+++ b/arch/arm64/boot/dts/xilinx/zynqmp-zcu104-revA.dts
@@ -511,10 +511,6 @@ &watchdog0 {
 	status = "okay";
 };
 
-&xilinx_ams {
-	status = "okay";
-};
-
 &ams_ps {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-zcu104-revC.dts b/arch/arm64/boot/dts/xilinx/zynqmp-zcu104-revC.dts
index ddc74d963a05..4694d0a841f1 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp-zcu104-revC.dts
+++ b/arch/arm64/boot/dts/xilinx/zynqmp-zcu104-revC.dts
@@ -523,10 +523,6 @@ &watchdog0 {
 	status = "okay";
 };
 
-&xilinx_ams {
-	status = "okay";
-};
-
 &ams_ps {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
index b1b31dcf6291..256cff250717 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
+++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
@@ -1157,7 +1157,6 @@ lpd_watchdog: watchdog@ff150000 {
 
 		xilinx_ams: ams@ffa50000 {
 			compatible = "xlnx,zynqmp-ams";
-			status = "disabled";
 			interrupt-parent = <&gic>;
 			interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
 			reg = <0x0 0xffa50000 0x0 0x800>;
-- 
2.35.1.1320.gc452695387.dirty

[PATCH 2/3] arm64: zynqmp: Expose AMS to userspace as HWMON

[Sean Anderson]

Expose the AMS to userspace, allowing monitoring of internal voltages
and temperatures. For compatibility, we keep the node name the same as
on the SM-K26, and we keep the ZCU100 Rev C. around (since it is named
differently).

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 arch/arm64/boot/dts/xilinx/zynqmp-sm-k26-revA.dts | 14 --------------
 arch/arm64/boot/dts/xilinx/zynqmp.dtsi            | 14 ++++++++++++++
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-sm-k26-revA.dts b/arch/arm64/boot/dts/xilinx/zynqmp-sm-k26-revA.dts
index 4e0e7fdf29ca..bfa7ea6b9224 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp-sm-k26-revA.dts
+++ b/arch/arm64/boot/dts/xilinx/zynqmp-sm-k26-revA.dts
@@ -90,20 +90,6 @@ ds36-led {
 		};
 	};
 
-	ams {
-		compatible = "iio-hwmon";
-		io-channels = <&xilinx_ams 0>, <&xilinx_ams 1>, <&xilinx_ams 2>,
-			<&xilinx_ams 3>, <&xilinx_ams 4>, <&xilinx_ams 5>,
-			<&xilinx_ams 6>, <&xilinx_ams 7>, <&xilinx_ams 8>,
-			<&xilinx_ams 9>, <&xilinx_ams 10>, <&xilinx_ams 11>,
-			<&xilinx_ams 12>, <&xilinx_ams 13>, <&xilinx_ams 14>,
-			<&xilinx_ams 15>, <&xilinx_ams 16>, <&xilinx_ams 17>,
-			<&xilinx_ams 18>, <&xilinx_ams 19>, <&xilinx_ams 20>,
-			<&xilinx_ams 21>, <&xilinx_ams 22>, <&xilinx_ams 23>,
-			<&xilinx_ams 24>, <&xilinx_ams 25>, <&xilinx_ams 26>,
-			<&xilinx_ams 27>, <&xilinx_ams 28>, <&xilinx_ams 29>;
-	};
-
 	pwm-fan {
 		compatible = "pwm-fan";
 		status = "okay";
diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
index 256cff250717..21c1adbaf35f 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
+++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
@@ -392,6 +392,20 @@ r5f@1 {
 		};
 	};
 
+	ams {
+		compatible = "iio-hwmon";
+		io-channels = <&xilinx_ams 0>, <&xilinx_ams 1>, <&xilinx_ams 2>,
+			<&xilinx_ams 3>, <&xilinx_ams 4>, <&xilinx_ams 5>,
+			<&xilinx_ams 6>, <&xilinx_ams 7>, <&xilinx_ams 8>,
+			<&xilinx_ams 9>, <&xilinx_ams 10>, <&xilinx_ams 11>,
+			<&xilinx_ams 12>, <&xilinx_ams 13>, <&xilinx_ams 14>,
+			<&xilinx_ams 15>, <&xilinx_ams 16>, <&xilinx_ams 17>,
+			<&xilinx_ams 18>, <&xilinx_ams 19>, <&xilinx_ams 20>,
+			<&xilinx_ams 21>, <&xilinx_ams 22>, <&xilinx_ams 23>,
+			<&xilinx_ams 24>, <&xilinx_ams 25>, <&xilinx_ams 26>,
+			<&xilinx_ams 27>, <&xilinx_ams 28>, <&xilinx_ams 29>;
+	};
+
 	amba: axi {
 		compatible = "simple-bus";
 		bootph-all;
-- 
2.35.1.1320.gc452695387.dirty

[PATCH 3/3] arm64: zynqmp: Add thermal zones

[Sean Anderson]

Add some thermal trip points. We can't undervolt the CPUs to save power
when we underclock them, so there isn't really a point in throttling
them until we are about to overheat. As such, the passive trip point is
right below the critical trip point.

The critical trip point is the extended/industrial-grade maximum
junction temperature of 100C minus the maximum temperature sensor error
of 3.5C (in the range -55C to 110C). Automotive- and military-grade
parts can go up to 125C, but as far as I can tell there is no way to
detect them at runtime. Userspace can adjust the trip points at runtime,
but this may not be viable when booting above 100C. I think it's
reasonable to ask automotive/military users to edit their device trees
to bump the trip points, but if that proves to be an issue we can
always go with no default temperatures. However, that wouldn't be too
nice for the majority of extended/industrial users.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 86 ++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
index 21c1adbaf35f..467f084c6469 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
+++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
@@ -18,6 +18,7 @@
 #include <dt-bindings/interrupt-controller/irq.h>
 #include <dt-bindings/power/xlnx-zynqmp-power.h>
 #include <dt-bindings/reset/xlnx-zynqmp-resets.h>
+#include <dt-bindings/thermal/thermal.h>
 
 / {
 	compatible = "xlnx,zynqmp";
@@ -36,6 +37,7 @@ cpus {
 		#size-cells = <0>;
 
 		cpu0: cpu@0 {
+			#cooling-cells = <2>;
 			compatible = "arm,cortex-a53";
 			device_type = "cpu";
 			enable-method = "psci";
@@ -46,6 +48,7 @@ cpu0: cpu@0 {
 		};
 
 		cpu1: cpu@1 {
+			#cooling-cells = <2>;
 			compatible = "arm,cortex-a53";
 			device_type = "cpu";
 			enable-method = "psci";
@@ -56,6 +59,7 @@ cpu1: cpu@1 {
 		};
 
 		cpu2: cpu@2 {
+			#cooling-cells = <2>;
 			compatible = "arm,cortex-a53";
 			device_type = "cpu";
 			enable-method = "psci";
@@ -66,6 +70,7 @@ cpu2: cpu@2 {
 		};
 
 		cpu3: cpu@3 {
+			#cooling-cells = <2>;
 			compatible = "arm,cortex-a53";
 			device_type = "cpu";
 			enable-method = "psci";
@@ -406,6 +411,87 @@ ams {
 			<&xilinx_ams 27>, <&xilinx_ams 28>, <&xilinx_ams 29>;
 	};
 
+
+	tsens_apu: thermal-sensor-apu {
+		compatible = "generic-adc-thermal";
+		#thermal-sensor-cells = <0>;
+		io-channels = <&xilinx_ams 7>;
+		io-channel-names = "sensor-channel";
+	};
+
+	tsens_rpu: thermal-sensor-rpu {
+		compatible = "generic-adc-thermal";
+		#thermal-sensor-cells = <0>;
+		io-channels = <&xilinx_ams 8>;
+		io-channel-names = "sensor-channel";
+	};
+
+	tsens_pl: thermal-sensor-pl {
+		compatible = "generic-adc-thermal";
+		#thermal-sensor-cells = <0>;
+		io-channels = <&xilinx_ams 20>;
+		io-channel-names = "sensor-channel";
+	};
+
+	thermal-zones {
+		apu-thermal {
+			polling-delay-passive = <1000>;
+			polling-delay = <5000>;
+			thermal-sensors = <&tsens_apu>;
+
+			trips {
+				apu_passive: passive {
+					temperature = <93000>;
+					hysteresis = <3500>;
+					type = "passive";
+				};
+
+				apu_critical: critical {
+					temperature = <96500>;
+					hysteresis = <3500>;
+					type = "critical";
+				};
+			};
+
+			cooling-maps {
+				map {
+					trip = <&apu_passive>;
+					cooling-device =
+						<&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+						<&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+						<&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+						<&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+				};
+			};
+		};
+
+		rpu-thermal {
+			polling-delay = <10000>;
+			thermal-sensors = <&tsens_rpu>;
+
+			trips {
+				critical {
+					temperature = <96500>;
+					hysteresis = <3500>;
+					type = "critical";
+				};
+			};
+		};
+
+		pl-thermal {
+			polling-delay = <10000>;
+			thermal-sensors = <&tsens_pl>;
+
+			trips {
+				critical {
+					temperature = <96500>;
+					hysteresis = <3500>;
+					type = "critical";
+				};
+			};
+		};
+	};
+
 	amba: axi {
 		compatible = "simple-bus";
 		bootph-all;
-- 
2.35.1.1320.gc452695387.dirty

RE: [PATCH 3/3] arm64: zynqmp: Add thermal zones

[Thangaraj, Senthil Nathan]

Hi Sean,

Please find my review comments inline.

Thanks,
Senthil.

> -----Original Message-----
> From: Sean Anderson <sean.anderson@linux.dev>
> Sent: Monday, August 12, 2024 2:51 PM
> To: Simek, Michal <michal.simek@amd.com>; linux-arm-
> kernel@lists.infradead.org
> Cc: Rob Herring <robh@kernel.org>; Conor Dooley <conor+dt@kernel.org>;
> Krzysztof Kozlowski <krzk+dt@kernel.org>; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; Sean Anderson <sean.anderson@linux.dev>
> Subject: [PATCH 3/3] arm64: zynqmp: Add thermal zones
> 
> Add some thermal trip points. We can't undervolt the CPUs to save power
> when we underclock them, so there isn't really a point in throttling them until
> we are about to overheat. As such, the passive trip point is right below the
> critical trip point.
> 
> The critical trip point is the extended/industrial-grade maximum junction
> temperature of 100C minus the maximum temperature sensor error of 3.5C
> (in the range -55C to 110C). Automotive- and military-grade parts can go up
> to 125C, but as far as I can tell there is no way to detect them at runtime.
> Userspace can adjust the trip points at runtime, but this may not be viable
> when booting above 100C. I think it's reasonable to ask automotive/military
> users to edit their device trees to bump the trip points, but if that proves to be
> an issue we can always go with no default temperatures. However, that
> wouldn't be too nice for the majority of extended/industrial users.
> 
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
> 
>  arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 86
> ++++++++++++++++++++++++++
>  1 file changed, 86 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> index 21c1adbaf35f..467f084c6469 100644
> --- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> +++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> @@ -18,6 +18,7 @@
>  #include <dt-bindings/interrupt-controller/irq.h>
>  #include <dt-bindings/power/xlnx-zynqmp-power.h>
>  #include <dt-bindings/reset/xlnx-zynqmp-resets.h>
> +#include <dt-bindings/thermal/thermal.h>
> 
>  / {
>  	compatible = "xlnx,zynqmp";
> @@ -36,6 +37,7 @@ cpus {
>  		#size-cells = <0>;
> 
>  		cpu0: cpu@0 {
> +			#cooling-cells = <2>;
>  			compatible = "arm,cortex-a53";
>  			device_type = "cpu";
>  			enable-method = "psci";
> @@ -46,6 +48,7 @@ cpu0: cpu@0 {
>  		};
> 
>  		cpu1: cpu@1 {
> +			#cooling-cells = <2>;
>  			compatible = "arm,cortex-a53";
>  			device_type = "cpu";
>  			enable-method = "psci";
> @@ -56,6 +59,7 @@ cpu1: cpu@1 {
>  		};
> 
>  		cpu2: cpu@2 {
> +			#cooling-cells = <2>;
>  			compatible = "arm,cortex-a53";
>  			device_type = "cpu";
>  			enable-method = "psci";
> @@ -66,6 +70,7 @@ cpu2: cpu@2 {
>  		};
> 
>  		cpu3: cpu@3 {
> +			#cooling-cells = <2>;
>  			compatible = "arm,cortex-a53";
>  			device_type = "cpu";
>  			enable-method = "psci";
> @@ -406,6 +411,87 @@ ams {
>  			<&xilinx_ams 27>, <&xilinx_ams 28>, <&xilinx_ams
> 29>;
>  	};
> 
> +
> +	tsens_apu: thermal-sensor-apu {
> +		compatible = "generic-adc-thermal";
> +		#thermal-sensor-cells = <0>;
> +		io-channels = <&xilinx_ams 7>;
> +		io-channel-names = "sensor-channel";
> +	};
> +
> +	tsens_rpu: thermal-sensor-rpu {
> +		compatible = "generic-adc-thermal";
> +		#thermal-sensor-cells = <0>;
> +		io-channels = <&xilinx_ams 8>;
> +		io-channel-names = "sensor-channel";
> +	};
> +
> +	tsens_pl: thermal-sensor-pl {
> +		compatible = "generic-adc-thermal";
> +		#thermal-sensor-cells = <0>;
> +		io-channels = <&xilinx_ams 20>;
> +		io-channel-names = "sensor-channel";
> +	};
> +
> +	thermal-zones {
> +		apu-thermal {
> +			polling-delay-passive = <1000>;
> +			polling-delay = <5000>;

How did we arrive at these delays, 1000 and 5000 ? could you please clarify ?

> +			thermal-sensors = <&tsens_apu>;
> +
> +			trips {
> +				apu_passive: passive {
> +					temperature = <93000>;
> +					hysteresis = <3500>;
> +					type = "passive";
> +				};
> +
> +				apu_critical: critical {
> +					temperature = <96500>;
> +					hysteresis = <3500>;
> +					type = "critical";
> +				};
> +			};
> +
> +			cooling-maps {
> +				map {
> +					trip = <&apu_passive>;
> +					cooling-device =
> +						<&cpu0 THERMAL_NO_LIMIT
> THERMAL_NO_LIMIT>,
> +						<&cpu1 THERMAL_NO_LIMIT
> THERMAL_NO_LIMIT>,
> +						<&cpu2 THERMAL_NO_LIMIT
> THERMAL_NO_LIMIT>,
> +						<&cpu3 THERMAL_NO_LIMIT
> THERMAL_NO_LIMIT>;
> +				};
> +			};
> +		};
> +
> +		rpu-thermal {
> +			polling-delay = <10000>;

Same questions, how did we come up with number 10000

> +			thermal-sensors = <&tsens_rpu>;
> +
> +			trips {
> +				critical {
> +					temperature = <96500>;
> +					hysteresis = <3500>;
> +					type = "critical";
> +				};

Any reason why we haven't defined for passive trip point for RPU ?

> +			};
> +		};
> +
> +		pl-thermal {
> +			polling-delay = <10000>;
> +			thermal-sensors = <&tsens_pl>;
> +
> +			trips {
> +				critical {
> +					temperature = <96500>;
> +					hysteresis = <3500>;
> +					type = "critical";
> +				};
> +			};
Same question, any reason why we haven't defined for passive trip point for PL ?
> +		};
> +	};
> +
>  	amba: axi {
>  		compatible = "simple-bus";
>  		bootph-all;
> --
> 2.35.1.1320.gc452695387.dirty

Re: [PATCH 3/3] arm64: zynqmp: Add thermal zones

[Sean Anderson]

On 9/3/24 04:32, Thangaraj, Senthil Nathan wrote:
> Hi Sean,
> 
> Please find my review comments inline.
> 
> Thanks,
> Senthil.
> 
>> -----Original Message-----
>> From: Sean Anderson <sean.anderson@linux.dev>
>> Sent: Monday, August 12, 2024 2:51 PM
>> To: Simek, Michal <michal.simek@amd.com>; linux-arm-
>> kernel@lists.infradead.org
>> Cc: Rob Herring <robh@kernel.org>; Conor Dooley <conor+dt@kernel.org>;
>> Krzysztof Kozlowski <krzk+dt@kernel.org>; linux-kernel@vger.kernel.org;
>> devicetree@vger.kernel.org; Sean Anderson <sean.anderson@linux.dev>
>> Subject: [PATCH 3/3] arm64: zynqmp: Add thermal zones
>> 
>> Add some thermal trip points. We can't undervolt the CPUs to save power
>> when we underclock them, so there isn't really a point in throttling them until
>> we are about to overheat. As such, the passive trip point is right below the
>> critical trip point.
>> 
>> The critical trip point is the extended/industrial-grade maximum junction
>> temperature of 100C minus the maximum temperature sensor error of 3.5C
>> (in the range -55C to 110C). Automotive- and military-grade parts can go up
>> to 125C, but as far as I can tell there is no way to detect them at runtime.
>> Userspace can adjust the trip points at runtime, but this may not be viable
>> when booting above 100C. I think it's reasonable to ask automotive/military
>> users to edit their device trees to bump the trip points, but if that proves to be
>> an issue we can always go with no default temperatures. However, that
>> wouldn't be too nice for the majority of extended/industrial users.
>> 
>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> ---
>> 
>>  arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 86
>> ++++++++++++++++++++++++++
>>  1 file changed, 86 insertions(+)
>> 
>> diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
>> b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
>> index 21c1adbaf35f..467f084c6469 100644
>> --- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
>> +++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
>> @@ -18,6 +18,7 @@
>>  #include <dt-bindings/interrupt-controller/irq.h>
>>  #include <dt-bindings/power/xlnx-zynqmp-power.h>
>>  #include <dt-bindings/reset/xlnx-zynqmp-resets.h>
>> +#include <dt-bindings/thermal/thermal.h>
>> 
>>  / {
>>  	compatible = "xlnx,zynqmp";
>> @@ -36,6 +37,7 @@ cpus {
>>  		#size-cells = <0>;
>> 
>>  		cpu0: cpu@0 {
>> +			#cooling-cells = <2>;
>>  			compatible = "arm,cortex-a53";
>>  			device_type = "cpu";
>>  			enable-method = "psci";
>> @@ -46,6 +48,7 @@ cpu0: cpu@0 {
>>  		};
>> 
>>  		cpu1: cpu@1 {
>> +			#cooling-cells = <2>;
>>  			compatible = "arm,cortex-a53";
>>  			device_type = "cpu";
>>  			enable-method = "psci";
>> @@ -56,6 +59,7 @@ cpu1: cpu@1 {
>>  		};
>> 
>>  		cpu2: cpu@2 {
>> +			#cooling-cells = <2>;
>>  			compatible = "arm,cortex-a53";
>>  			device_type = "cpu";
>>  			enable-method = "psci";
>> @@ -66,6 +70,7 @@ cpu2: cpu@2 {
>>  		};
>> 
>>  		cpu3: cpu@3 {
>> +			#cooling-cells = <2>;
>>  			compatible = "arm,cortex-a53";
>>  			device_type = "cpu";
>>  			enable-method = "psci";
>> @@ -406,6 +411,87 @@ ams {
>>  			<&xilinx_ams 27>, <&xilinx_ams 28>, <&xilinx_ams
>> 29>;
>>  	};
>> 
>> +
>> +	tsens_apu: thermal-sensor-apu {
>> +		compatible = "generic-adc-thermal";
>> +		#thermal-sensor-cells = <0>;
>> +		io-channels = <&xilinx_ams 7>;
>> +		io-channel-names = "sensor-channel";
>> +	};
>> +
>> +	tsens_rpu: thermal-sensor-rpu {
>> +		compatible = "generic-adc-thermal";
>> +		#thermal-sensor-cells = <0>;
>> +		io-channels = <&xilinx_ams 8>;
>> +		io-channel-names = "sensor-channel";
>> +	};
>> +
>> +	tsens_pl: thermal-sensor-pl {
>> +		compatible = "generic-adc-thermal";
>> +		#thermal-sensor-cells = <0>;
>> +		io-channels = <&xilinx_ams 20>;
>> +		io-channel-names = "sensor-channel";
>> +	};
>> +
>> +	thermal-zones {
>> +		apu-thermal {
>> +			polling-delay-passive = <1000>;
>> +			polling-delay = <5000>;
> 
> How did we arrive at these delays, 1000 and 5000 ? could you please clarify ?

They're just arbitrary. Feel free to suggest other numbers. I could find
no guidance on this matter (or anything else thermal-related).

>> +			thermal-sensors = <&tsens_apu>;
>> +
>> +			trips {
>> +				apu_passive: passive {
>> +					temperature = <93000>;
>> +					hysteresis = <3500>;
>> +					type = "passive";
>> +				};
>> +
>> +				apu_critical: critical {
>> +					temperature = <96500>;
>> +					hysteresis = <3500>;
>> +					type = "critical";
>> +				};
>> +			};
>> +
>> +			cooling-maps {
>> +				map {
>> +					trip = <&apu_passive>;
>> +					cooling-device =
>> +						<&cpu0 THERMAL_NO_LIMIT
>> THERMAL_NO_LIMIT>,
>> +						<&cpu1 THERMAL_NO_LIMIT
>> THERMAL_NO_LIMIT>,
>> +						<&cpu2 THERMAL_NO_LIMIT
>> THERMAL_NO_LIMIT>,
>> +						<&cpu3 THERMAL_NO_LIMIT
>> THERMAL_NO_LIMIT>;
>> +				};
>> +			};
>> +		};
>> +
>> +		rpu-thermal {
>> +			polling-delay = <10000>;
> 
> Same questions, how did we come up with number 10000
> 
>> +			thermal-sensors = <&tsens_rpu>;
>> +
>> +			trips {
>> +				critical {
>> +					temperature = <96500>;
>> +					hysteresis = <3500>;
>> +					type = "critical";
>> +				};
> 
> Any reason why we haven't defined for passive trip point for RPU ?

What is there to do? It is up to the RPU software to do something if the
RPU is going to overheat. But the more-likely occurrence is that the APU
is overheating and the heat is spreading to the RPU. In which case, the
APU passive trip point will handle things.

>> +			};
>> +		};
>> +
>> +		pl-thermal {
>> +			polling-delay = <10000>;
>> +			thermal-sensors = <&tsens_pl>;
>> +
>> +			trips {
>> +				critical {
>> +					temperature = <96500>;
>> +					hysteresis = <3500>;
>> +					type = "critical";
>> +				};
>> +			};
> Same question, any reason why we haven't defined for passive trip point for PL ?
>> +		};
>> +	};
>> +
>>  	amba: axi {
>>  		compatible = "simple-bus";
>>  		bootph-all;
>> --
>> 2.35.1.1320.gc452695387.dirty
> 

RE: [PATCH 3/3] arm64: zynqmp: Add thermal zones

[Thangaraj, Senthil Nathan]

Hi Sean,

Thank you for your response.

Unfortunately, I’m not the right person to recommend the thermal polling delay. I’m looping Matthew into this thread for further assistance.

Hi Matthew,

Could you or your team please provide a recommendation on this?

Thanks,
Senthil

> -----Original Message-----
> From: Sean Anderson <sean.anderson@linux.dev>
> Sent: Tuesday, September 3, 2024 7:28 AM
> To: Thangaraj, Senthil Nathan <SenthilNathan.Thangaraj@amd.com>; Simek,
> Michal <michal.simek@amd.com>; linux-arm-kernel@lists.infradead.org
> Cc: Rob Herring <robh@kernel.org>; Conor Dooley <conor+dt@kernel.org>;
> Krzysztof Kozlowski <krzk+dt@kernel.org>; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org
> Subject: Re: [PATCH 3/3] arm64: zynqmp: Add thermal zones
> 
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
> 
> 
> On 9/3/24 04:32, Thangaraj, Senthil Nathan wrote:
> > Hi Sean,
> >
> > Please find my review comments inline.
> >
> > Thanks,
> > Senthil.
> >
> >> -----Original Message-----
> >> From: Sean Anderson <sean.anderson@linux.dev>
> >> Sent: Monday, August 12, 2024 2:51 PM
> >> To: Simek, Michal <michal.simek@amd.com>; linux-arm-
> >> kernel@lists.infradead.org
> >> Cc: Rob Herring <robh@kernel.org>; Conor Dooley
> >> <conor+dt@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>;
> >> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; Sean
> >> Anderson <sean.anderson@linux.dev>
> >> Subject: [PATCH 3/3] arm64: zynqmp: Add thermal zones
> >>
> >> Add some thermal trip points. We can't undervolt the CPUs to save
> >> power when we underclock them, so there isn't really a point in
> >> throttling them until we are about to overheat. As such, the passive
> >> trip point is right below the critical trip point.
> >>
> >> The critical trip point is the extended/industrial-grade maximum
> >> junction temperature of 100C minus the maximum temperature sensor
> >> error of 3.5C (in the range -55C to 110C). Automotive- and
> >> military-grade parts can go up to 125C, but as far as I can tell there is no
> way to detect them at runtime.
> >> Userspace can adjust the trip points at runtime, but this may not be
> >> viable when booting above 100C. I think it's reasonable to ask
> >> automotive/military users to edit their device trees to bump the trip
> >> points, but if that proves to be an issue we can always go with no
> >> default temperatures. However, that wouldn't be too nice for the majority
> of extended/industrial users.
> >>
> >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> >> ---
> >>
> >>  arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 86
> >> ++++++++++++++++++++++++++
> >>  1 file changed, 86 insertions(+)
> >>
> >> diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> >> b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> >> index 21c1adbaf35f..467f084c6469 100644
> >> --- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> >> +++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> >> @@ -18,6 +18,7 @@
> >>  #include <dt-bindings/interrupt-controller/irq.h>
> >>  #include <dt-bindings/power/xlnx-zynqmp-power.h>
> >>  #include <dt-bindings/reset/xlnx-zynqmp-resets.h>
> >> +#include <dt-bindings/thermal/thermal.h>
> >>
> >>  / {
> >>      compatible = "xlnx,zynqmp";
> >> @@ -36,6 +37,7 @@ cpus {
> >>              #size-cells = <0>;
> >>
> >>              cpu0: cpu@0 {
> >> +                    #cooling-cells = <2>;
> >>                      compatible = "arm,cortex-a53";
> >>                      device_type = "cpu";
> >>                      enable-method = "psci"; @@ -46,6 +48,7 @@ cpu0:
> >> cpu@0 {
> >>              };
> >>
> >>              cpu1: cpu@1 {
> >> +                    #cooling-cells = <2>;
> >>                      compatible = "arm,cortex-a53";
> >>                      device_type = "cpu";
> >>                      enable-method = "psci"; @@ -56,6 +59,7 @@ cpu1:
> >> cpu@1 {
> >>              };
> >>
> >>              cpu2: cpu@2 {
> >> +                    #cooling-cells = <2>;
> >>                      compatible = "arm,cortex-a53";
> >>                      device_type = "cpu";
> >>                      enable-method = "psci"; @@ -66,6 +70,7 @@ cpu2:
> >> cpu@2 {
> >>              };
> >>
> >>              cpu3: cpu@3 {
> >> +                    #cooling-cells = <2>;
> >>                      compatible = "arm,cortex-a53";
> >>                      device_type = "cpu";
> >>                      enable-method = "psci"; @@ -406,6 +411,87 @@ ams
> >> {
> >>                      <&xilinx_ams 27>, <&xilinx_ams 28>, <&xilinx_ams
> >> 29>;
> >>      };
> >>
> >> +
> >> +    tsens_apu: thermal-sensor-apu {
> >> +            compatible = "generic-adc-thermal";
> >> +            #thermal-sensor-cells = <0>;
> >> +            io-channels = <&xilinx_ams 7>;
> >> +            io-channel-names = "sensor-channel";
> >> +    };
> >> +
> >> +    tsens_rpu: thermal-sensor-rpu {
> >> +            compatible = "generic-adc-thermal";
> >> +            #thermal-sensor-cells = <0>;
> >> +            io-channels = <&xilinx_ams 8>;
> >> +            io-channel-names = "sensor-channel";
> >> +    };
> >> +
> >> +    tsens_pl: thermal-sensor-pl {
> >> +            compatible = "generic-adc-thermal";
> >> +            #thermal-sensor-cells = <0>;
> >> +            io-channels = <&xilinx_ams 20>;
> >> +            io-channel-names = "sensor-channel";
> >> +    };
> >> +
> >> +    thermal-zones {
> >> +            apu-thermal {
> >> +                    polling-delay-passive = <1000>;
> >> +                    polling-delay = <5000>;
> >
> > How did we arrive at these delays, 1000 and 5000 ? could you please clarify
> ?
> 
> They're just arbitrary. Feel free to suggest other numbers. I could find no
> guidance on this matter (or anything else thermal-related).
> 
> >> +                    thermal-sensors = <&tsens_apu>;
> >> +
> >> +                    trips {
> >> +                            apu_passive: passive {
> >> +                                    temperature = <93000>;
> >> +                                    hysteresis = <3500>;
> >> +                                    type = "passive";
> >> +                            };
> >> +
> >> +                            apu_critical: critical {
> >> +                                    temperature = <96500>;
> >> +                                    hysteresis = <3500>;
> >> +                                    type = "critical";
> >> +                            };
> >> +                    };
> >> +
> >> +                    cooling-maps {
> >> +                            map {
> >> +                                    trip = <&apu_passive>;
> >> +                                    cooling-device =
> >> +                                            <&cpu0 THERMAL_NO_LIMIT
> >> THERMAL_NO_LIMIT>,
> >> +                                            <&cpu1 THERMAL_NO_LIMIT
> >> THERMAL_NO_LIMIT>,
> >> +                                            <&cpu2 THERMAL_NO_LIMIT
> >> THERMAL_NO_LIMIT>,
> >> +                                            <&cpu3 THERMAL_NO_LIMIT
> >> THERMAL_NO_LIMIT>;
> >> +                            };
> >> +                    };
> >> +            };
> >> +
> >> +            rpu-thermal {
> >> +                    polling-delay = <10000>;
> >
> > Same questions, how did we come up with number 10000
> >
> >> +                    thermal-sensors = <&tsens_rpu>;
> >> +
> >> +                    trips {
> >> +                            critical {
> >> +                                    temperature = <96500>;
> >> +                                    hysteresis = <3500>;
> >> +                                    type = "critical";
> >> +                            };
> >
> > Any reason why we haven't defined for passive trip point for RPU ?
> 
> What is there to do? It is up to the RPU software to do something if the RPU is
> going to overheat. But the more-likely occurrence is that the APU is
> overheating and the heat is spreading to the RPU. In which case, the APU
> passive trip point will handle things.
> 
> >> +                    };
> >> +            };
> >> +
> >> +            pl-thermal {
> >> +                    polling-delay = <10000>;
> >> +                    thermal-sensors = <&tsens_pl>;
> >> +
> >> +                    trips {
> >> +                            critical {
> >> +                                    temperature = <96500>;
> >> +                                    hysteresis = <3500>;
> >> +                                    type = "critical";
> >> +                            };
> >> +                    };
> > Same question, any reason why we haven't defined for passive trip point for
> PL ?
> >> +            };
> >> +    };
> >> +
> >>      amba: axi {
> >>              compatible = "simple-bus";
> >>              bootph-all;
> >> --
> >> 2.35.1.1320.gc452695387.dirty
> >

Re: [PATCH 3/3] arm64: zynqmp: Add thermal zones

[Michal Simek]

On 9/9/24 20:28, Thangaraj, Senthil Nathan wrote:
> Hi Sean,
> 
> Thank you for your response.
> 
> Unfortunately, I’m not the right person to recommend the thermal polling delay. I’m looping Matthew into this thread for further assistance.
> 
> Hi Matthew,
> 
> Could you or your team please provide a recommendation on this?

Any update on this? If not I will take merge it.

Thanks,
Michal

Re: [PATCH 0/3] arm64: zynqmp: Add thermal zones

[Michal Simek]

On 8/12/24 23:51, Sean Anderson wrote:
> At the moment, the ZynqMP Analog Monitoring System (AMS) is only used
> sporadically. As it is built into the SoC and doesn't depend on external
> hardware, it can be exposed to userspace for all boards. Additionally,
> we can use it to implement thermal zones.
> 
> 
> Sean Anderson (3):
>    arm64: zynqmp: Enable AMS for all boards
>    arm64: zynqmp: Expose AMS to userspace as HWMON
>    arm64: zynqmp: Add thermal zones
> 
>   .../boot/dts/xilinx/zynqmp-sm-k26-revA.dts    |  18 ----
>   .../boot/dts/xilinx/zynqmp-zcu100-revC.dts    |   4 -
>   .../boot/dts/xilinx/zynqmp-zcu102-revA.dts    |   4 -
>   .../boot/dts/xilinx/zynqmp-zcu104-revA.dts    |   4 -
>   .../boot/dts/xilinx/zynqmp-zcu104-revC.dts    |   4 -
>   arch/arm64/boot/dts/xilinx/zynqmp.dtsi        | 101 +++++++++++++++++-
>   6 files changed, 100 insertions(+), 35 deletions(-)
> 

Applied.
M