dt-bindings: imx8mm-thermal: Document 'nxp,reboot-on-critical'

PATCH Superseded Fabio Estevam 3 participants 11 messages Aug 23, 2023 – Aug 24, 2023

Subsystems (3)

Device tree bindings Arm i.MX and Layerscape Thermal

lore

Share & tools

lore

Fabio Estevam

author reviewer

2023-08-23 17:33 UTC #0 lore raw

PATCH 1/2 dt-bindings: imx8mm-thermal: Document 'nxp,reboot-on-critical'

From: Fabio Estevam <[email protected]>

Currently, after the board reaches the critical temperature, the system goes through a poweroff mechanism.

In some cases, such behavior does not suit well, as the board may be unattended in the field and rebooting may be a better approach.

The bootloader may also check the temperature and only allow the boot to proceed when the temperature is below a certain threshold.

Introduce the 'nxp,reboot-on-critical' property to indicate that the board will go through a reboot after the critical temperature is reached.

When this property is absent, the default behavior of forcing a shutdown is kept.

Signed-off-by Fabio Estevam <[email protected]>

---
 .../devicetree/bindings/thermal/imx8mm-thermal.yaml         | 6 ++++++
 1 file changed, 6 insertions(+)

Documentation/devicetree/bindings/thermal/imx8mm-thermal.yaml +6 -0

index d2c1e4573c32..9ac70360fd35 100644
@@ -32,6 +32,12 @@ properties:
   clocks:
     maxItems: 1
 
+  nxp,reboot-on-critical:
+    description: Property to indicate that the system will go through a reboot
+      after the critical temperature is reached. If absent, the system will
+      go through shutdown after the critical temperature is reached.
+    type: boolean
+
   nvmem-cells:
     maxItems: 1
     description: Phandle to the calibration data provided by ocotp

Signature · 1 line

2.34.1

Fabio Estevam

author reviewer

2023-08-23 17:33 UTC #1 lore raw

PATCH 2/2 thermal: imx8mm: Allow reboot after critical temperature

From: Fabio Estevam <[email protected]>

Currently, after the board reaches the critical temperature, the system goes through a poweroff mechanism.

In some cases, such behavior does not suit well, as the board may be unattended in the field and rebooting may be a better approach.

The bootloader may also check the temperature and only allow the boot to proceed when the temperature is below a certain threshold.

Introduce the 'nxp,reboot-on-critical' property to indicate that the board will go through a reboot after the critical temperature is reached.

When this property is absent, the default behavior of forcing a shutdown is kept.

Tested on a imx8mm-evk board.

Signed-off-by Fabio Estevam <[email protected]>

---
 drivers/thermal/imx8mm_thermal.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

drivers/thermal/imx8mm_thermal.c +19 -0

index e89b11b3f2b9..cc95a1416976 100644
@@ -15,6 +15,7 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/thermal.h>
+#include <linux/reboot.h>
 
 #include "thermal_hwmon.h"

@@ -91,6 +92,7 @@ struct imx8mm_tmu {
 	void __iomem *base;
 	struct clk *clk;
 	const struct thermal_soc_data *socdata;
+	bool reboot;
 	struct tmu_sensor sensors[];
 };

@@ -146,8 +148,23 @@ static int tmu_get_temp(struct thermal_zone_device *tz, int *temp)
 	return tmu->socdata->get_temp(sensor, temp);
 }
 
+static void tmu_critical(struct thermal_zone_device *tz)
+{
+	struct tmu_sensor *sensor = thermal_zone_device_priv(tz);
+	struct imx8mm_tmu *tmu = sensor->priv;
+
+	if (tmu->reboot) {
+		dev_emerg(thermal_zone_device(tz), "%s: critical temperature reached\n",
+			  thermal_zone_device_type(tz));
+		kernel_restart(NULL);
+	} else {
+		thermal_zone_device_critical(tz);
+	}
+}
+
 static const struct thermal_zone_device_ops tmu_tz_ops = {
 	.get_temp = tmu_get_temp,
+	.critical = tmu_critical,
 };
 
 static void imx8mm_tmu_enable(struct imx8mm_tmu *tmu, bool enable)

@@ -313,6 +330,8 @@ static int imx8mm_tmu_probe(struct platform_device *pdev)
 	if (IS_ERR(tmu->base))
 		return PTR_ERR(tmu->base);
 
+	tmu->reboot = of_property_present(pdev->dev.of_node, "nxp,reboot-on-critical");
+
 	tmu->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(tmu->clk))
 		return dev_err_probe(&pdev->dev, PTR_ERR(tmu->clk),

Signature · 1 line

2.34.1

Krzysztof Kozlowski

2023-08-24 07:19 UTC #2 lore raw

In reply to Fabio Estevam

Quoted 27 lines · Fabio Estevam

From: Fabio Estevam <[email protected]>
Currently, after the board reaches the critical temperature, the system goes through a poweroff mechanism.
In some cases, such behavior does not suit well, as the board may be unattended in the field and rebooting may be a better approach.
The bootloader may also check the temperature and only allow the boot to proceed when the temperature is below a certain threshold.
Introduce the 'nxp,reboot-on-critical' property to indicate that the board will go through a reboot after the critical temperature is reached.
When this property is absent, the default behavior of forcing a shutdown is kept.
Signed-off-by Fabio Estevam <[email protected]>
---
 .../devicetree/bindings/thermal/imx8mm-thermal.yaml         | 6 ++++++
 1 file changed, 6 insertions(+)
Documentation/devicetree/bindings/thermal/imx8mm-thermal.yaml +0 -0
index d2c1e4573c32..9ac70360fd35 100644

Documentation/devicetree/bindings/thermal/imx8mm-thermal.yaml properties: · line 38

Quoted 7 lines

Documentation/devicetree/bindings/thermal/imx8mm-thermal.yaml +4 -0

@@ -32,6 +32,12 @@ properties:
   clocks:
     maxItems: 1
 
+  nxp,reboot-on-critical:
+    description: Property to indicate that the system will go through a reboot
+      after the critical temperature is reached. If absent, the system will
+      go through shutdown after the critical temperature is reached.

And if Linux changes the behavior of critical temperature to be "reboot" instead of "poweroff", what happens with this property? You add now property for a SW policy depending on one given implementation. Not suitable for bindings, sorry.

Best regards,
Krzysztof

Fabio Estevam

author reviewer

2023-08-24 09:58 UTC #3 lore raw

Quoted 4 lines · Krzysztof Kozlowski

And if Linux changes the behavior of critical temperature to be "reboot" instead of "poweroff", what happens with this property? You add now property for a SW policy depending on one given implementation. Not suitable for bindings, sorry.

Ok, understood.

I will try a different approach by introducing a Kconfig option.

Thanks

Daniel Lezcano

2023-08-24 10:36 UTC #4 lore raw

Quoted 11 lines · Fabio Estevam

Quoted 4 lines · Krzysztof Kozlowski
And if Linux changes the behavior of critical temperature to be "reboot" instead of "poweroff", what happens with this property? You add now property for a SW policy depending on one given implementation. Not suitable for bindings, sorry.
Ok, understood.
I will try a different approach by introducing a Kconfig option.

Alternatively, the 'chosen' DT node could be used, no ?

Signature · 5 lines

<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Fabio Estevam

author reviewer

2023-08-24 10:54 UTC #5 lore raw

Hi Daniel,

Quoted 3 lines · Daniel Lezcano

Quoted 1 line
I will try a different approach by introducing a Kconfig option.
Alternatively, the 'chosen' DT node could be used, no ?

Good idea. I will introduce a module_param() then.

Thanks!

Krzysztof Kozlowski

2023-08-24 12:39 UTC #6 lore raw

Quoted 8 lines · Fabio Estevam

Hi Daniel,
Quoted 3 lines · Daniel Lezcano
Quoted 1 line
I will try a different approach by introducing a Kconfig option.
Alternatively, the 'chosen' DT node could be used, no ?

Any DT property would be a problem, because I don't think it is static configuration. For example board with the same DTS once should reboot (during development) and once shutdown (some customer wants to be sure it will stay shutdown after critical condition). It's runtime feature.

Quoted 2 lines

Good idea. I will introduce a module_param() then.

Module params are usually discouraged and it also does not allow any runtime configuration. I think you need sysfs ABI.

Best regards,
Krzysztof

Fabio Estevam

author reviewer

2023-08-24 14:36 UTC #7 lore raw

Hi Krzysztof,

Quoted 2 lines · Krzysztof Kozlowski

Module params are usually discouraged and it also does not allow any runtime configuration. I think you need sysfs ABI.

I will go with the sysfs approach then, thanks.

Daniel Lezcano

2023-08-24 14:55 UTC #8 lore raw

In reply to Krzysztof Kozlowski

Hi Krzysztof,

Quoted 6 lines · Krzysztof Kozlowski

Quoted 5 lines · Fabio Estevam
Hi Daniel,
On Thu, Aug 24, 2023 at 7:35 AM Daniel Lezcano
<[email protected]> wrote:

Quoted 8 lines

Quoted 3 lines
Quoted 3 lines
Quoted 1 line
I will try a different approach by introducing a Kconfig option.
Alternatively, the 'chosen' DT node could be used, no ?
Any DT property would be a problem, because I don't think it is static configuration. For example board with the same DTS once should reboot (during development) and once shutdown (some customer wants to be sure it will stay shutdown after critical condition). It's runtime feature.

Fabio described the feature as a firmware feature where the board does not boot until the temperature goes below a certain temperature.

That does not look a runtime feature but a platform specific one.

From my POV, if the firmware wants to take over the thermal boot of the board, it is probably for a good reason (eg. the board will overheat between the boot and the kernel puts in place the mitigation framework). Letting the user to change that behavior can be dangerous.

Quoted 3 lines

Quoted 1 line
Good idea. I will introduce a module_param() then.
Module params are usually discouraged

Why?

Quoted 2 lines

and it also does not allow any
runtime configuration. I think you need sysfs ABI.

There is already the sysfs ABI with module params

/sys/module/<name>/parameters/reboot_on_critical

Signature · 5 lines

<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Krzysztof Kozlowski

2023-08-24 15:44 UTC #9 lore raw

Quoted 22 lines · Daniel Lezcano

Hi Krzysztof,
Quoted 6 lines · Krzysztof Kozlowski
Quoted 5 lines · Fabio Estevam
Hi Daniel,
On Thu, Aug 24, 2023 at 7:35 AM Daniel Lezcano
<[email protected]> wrote:
Quoted 8 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
I will try a different approach by introducing a Kconfig option.
Alternatively, the 'chosen' DT node could be used, no ?
Any DT property would be a problem, because I don't think it is static configuration. For example board with the same DTS once should reboot (during development) and once shutdown (some customer wants to be sure it will stay shutdown after critical condition). It's runtime feature.
Fabio described the feature as a firmware feature where the board does not boot until the temperature goes below a certain temperature.

Quoted 6 lines

That does not look a runtime feature but a platform specific one.
From my POV, if the firmware wants to take over the thermal boot of the
board, it is probably for a good reason (eg. the board will overheat between the boot and the kernel puts in place the mitigation framework). Letting the user to change that behavior can be dangerous.

OK, if this is supposed to be also accessed before user-space or even before kernel, then it makes sense in DT.

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
Good idea. I will introduce a module_param() then.
Module params are usually discouraged
Why?

Because they don't scale with number of devices, are poorly documented and have general limitations comparing to sysfs. You can ask Greg for more details:

https://lore.kernel.org/all/2023071135-opt-choosing-51dd@gregkh/

Quoted 7 lines

Quoted 3 lines
and it also does not allow any
runtime configuration. I think you need sysfs ABI.
There is already the sysfs ABI with module params
/sys/module/<name>/parameters/reboot_on_critical

So patch solved?

Best regards,
Krzysztof

Fabio Estevam

author reviewer

2023-08-24 16:47 UTC #10 lore raw

Hi Krzysztof and Daniel,

Quoted 2 lines · Krzysztof Kozlowski

OK, if this is supposed to be also accessed before user-space or even before kernel, then it makes sense in DT.

I think we reached a consensus now.

I will send a new version that uses the DT approach and populate tmu_tz_ops.critical inside probe() as suggested by Daniel.

Thanks a lot for the review