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

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

[Fabio Estevam]

From: Fabio Estevam <festevam@denx.de>

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 <festevam@denx.de>
---
 .../devicetree/bindings/thermal/imx8mm-thermal.yaml         | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/thermal/imx8mm-thermal.yaml b/Documentation/devicetree/bindings/thermal/imx8mm-thermal.yaml
index d2c1e4573c32..9ac70360fd35 100644
--- a/Documentation/devicetree/bindings/thermal/imx8mm-thermal.yaml
+++ b/Documentation/devicetree/bindings/thermal/imx8mm-thermal.yaml
@@ -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
-- 
2.34.1

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

[Fabio Estevam]

From: Fabio Estevam <festevam@denx.de>

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 <festevam@denx.de>
---
 drivers/thermal/imx8mm_thermal.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/thermal/imx8mm_thermal.c b/drivers/thermal/imx8mm_thermal.c
index e89b11b3f2b9..cc95a1416976 100644
--- a/drivers/thermal/imx8mm_thermal.c
+++ b/drivers/thermal/imx8mm_thermal.c
@@ -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),
-- 
2.34.1

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

[Krzysztof Kozlowski]

On 23/08/2023 19:33, Fabio Estevam wrote:
> From: Fabio Estevam <festevam@denx.de>
> 
> 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 <festevam@denx.de>
> ---
>  .../devicetree/bindings/thermal/imx8mm-thermal.yaml         | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/imx8mm-thermal.yaml b/Documentation/devicetree/bindings/thermal/imx8mm-thermal.yaml
> index d2c1e4573c32..9ac70360fd35 100644
> --- a/Documentation/devicetree/bindings/thermal/imx8mm-thermal.yaml
> +++ b/Documentation/devicetree/bindings/thermal/imx8mm-thermal.yaml
> @@ -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

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

[Fabio Estevam]

On Thu, Aug 24, 2023 at 4:18 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:

> 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

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

[Daniel Lezcano]

On 24/08/2023 11:57, Fabio Estevam wrote:
> On Thu, Aug 24, 2023 at 4:18 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> 
>> 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 ?

-- 
<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

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

[Fabio Estevam]

Hi Daniel,

On Thu, Aug 24, 2023 at 7:35 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:

> > 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!

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

[Krzysztof Kozlowski]

On 24/08/2023 12:53, Fabio Estevam wrote:
> Hi Daniel,
> 
> On Thu, Aug 24, 2023 at 7:35 AM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
> 
>>> 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.

> 
> 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

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

[Fabio Estevam]

Hi Krzysztof,

On Thu, Aug 24, 2023 at 9:38 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:

> 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.

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

[Daniel Lezcano]

Hi Krzysztof,

On 24/08/2023 14:38, Krzysztof Kozlowski wrote:
> On 24/08/2023 12:53, Fabio Estevam wrote:
>> Hi Daniel,
>>
>> On Thu, Aug 24, 2023 at 7:35 AM Daniel Lezcano
>> <daniel.lezcano@linaro.org> wrote:
>>
>>>> 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.

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

Why?

> 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



-- 
<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

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

[Krzysztof Kozlowski]

On 24/08/2023 16:54, Daniel Lezcano wrote:
> 
> Hi Krzysztof,
> 
> On 24/08/2023 14:38, Krzysztof Kozlowski wrote:
>> On 24/08/2023 12:53, Fabio Estevam wrote:
>>> Hi Daniel,
>>>
>>> On Thu, Aug 24, 2023 at 7:35 AM Daniel Lezcano
>>> <daniel.lezcano@linaro.org> wrote:
>>>
>>>>> 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.

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

> 
>>> 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/

> 
>> 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

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

[Fabio Estevam]

Hi Krzysztof and Daniel,

On Thu, Aug 24, 2023 at 12:43 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:

> 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