diff mbox series

[v7,1/3] dt-bindings: thermal-zones: Document critical-action

Message ID 20230831130242.2290502-1-festevam@gmail.com
State Superseded
Headers show
Series [v7,1/3] dt-bindings: thermal-zones: Document critical-action | expand

Commit Message

Fabio Estevam Aug. 31, 2023, 1:02 p.m. UTC
From: Fabio Estevam <festevam@denx.de>

Document the critical-action property to describe the thermal action
the OS should perform after the critical temperature is reached.

The possible values are "shutdown" and "reboot".

The motivation for introducing the critical-action property is that
different systems may need different thermal actions when the critical
temperature is reached.

For example, a desktop PC may want the OS to trigger a shutdown
when the critical temperature is reached.

However, in some embedded 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 benefit from this new property as it can check
the SoC temperature and in case the temperature is above the critical
point, it can trigger a shutdown or reboot accordingly.

Signed-off-by: Fabio Estevam <festevam@denx.de>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
Changes since v6:
- None.

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

Comments

Rafael J. Wysocki Sept. 7, 2023, 4:23 p.m. UTC | #1
On Thu, Aug 31, 2023 at 3:02 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> From: Fabio Estevam <festevam@denx.de>
>
> Document the critical-action property to describe the thermal action
> the OS should perform after the critical temperature is reached.
>
> The possible values are "shutdown" and "reboot".
>
> The motivation for introducing the critical-action property is that
> different systems may need different thermal actions when the critical
> temperature is reached.
>
> For example, a desktop PC may want the OS to trigger a shutdown
> when the critical temperature is reached.
>
> However, in some embedded cases, such behavior does not suit well,
> as the board may be unattended in the field and rebooting may be a
> better approach.

So one more question here: Why is this a property of a thermal zone
and not the property of the whole system?

Presumably, on a system where the platform integrator prefers to
reboot on critical temperature, it would be necessary to add this
property to every thermal zone.

Also, what if this property has different values for different thermal zones?

> The bootloader may also benefit from this new property as it can check
> the SoC temperature and in case the temperature is above the critical
> point, it can trigger a shutdown or reboot accordingly.
>
> Signed-off-by: Fabio Estevam <festevam@denx.de>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
> Changes since v6:
> - None.
>
>  .../devicetree/bindings/thermal/thermal-zones.yaml       | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> index 4f3acdc4dec0..c2e4d28f885b 100644
> --- a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> +++ b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> @@ -75,6 +75,15 @@ patternProperties:
>            framework and assumes that the thermal sensors in this zone
>            support interrupts.
>
> +      critical-action:
> +        $ref: /schemas/types.yaml#/definitions/string
> +        description:
> +          The action the OS should perform after the critical temperature is reached.
> +
> +        enum:
> +          - shutdown
> +          - reboot
> +
>        thermal-sensors:
>          $ref: /schemas/types.yaml#/definitions/phandle-array
>          maxItems: 1
> --
> 2.34.1
>
Fabio Estevam Sept. 8, 2023, 5:37 p.m. UTC | #2
Hi Rafael,

On 07/09/2023 13:23, Rafael J. Wysocki wrote:

> So one more question here: Why is this a property of a thermal zone
> and not the property of the whole system?
> 
> Presumably, on a system where the platform integrator prefers to
> reboot on critical temperature, it would be necessary to add this
> property to every thermal zone.
> 
> Also, what if this property has different values for different thermal 
> zones?

I got your point and I can make the 'critical-action' property to be 
valid
for the whole thermal system.

Originally, I have been doing like this:

	thermal-zones {
		cpu-thermal {
			critical-action = "reboot";
			polling-delay-passive = <250>;
			polling-delay = <2000>;
			thermal-sensors = <&tmu>;

			trips {
				cpu_alert0: trip0 {
					temperature = <85000>;
					hysteresis = <2000>;
					type = "passive";
				};

I can change it to be:


	thermal-zones {
		critical-action = "reboot";

		cpu-thermal {
			polling-delay-passive = <250>;
			polling-delay = <2000>;
			thermal-sensors = <&tmu>;

			trips {
				cpu_alert0: trip0 {
					temperature = <85000>;
					hysteresis = <2000>;
					type = "passive";
				};

Thanks,

Fabio Estevam
Rafael J. Wysocki Sept. 8, 2023, 6:01 p.m. UTC | #3
On Fri, Sep 8, 2023 at 7:37 PM Fabio Estevam <festevam@denx.de> wrote:
>
> Hi Rafael,
>
> On 07/09/2023 13:23, Rafael J. Wysocki wrote:
>
> > So one more question here: Why is this a property of a thermal zone
> > and not the property of the whole system?
> >
> > Presumably, on a system where the platform integrator prefers to
> > reboot on critical temperature, it would be necessary to add this
> > property to every thermal zone.
> >
> > Also, what if this property has different values for different thermal
> > zones?
>
> I got your point and I can make the 'critical-action' property to be
> valid
> for the whole thermal system.
>
> Originally, I have been doing like this:
>
>         thermal-zones {
>                 cpu-thermal {
>                         critical-action = "reboot";
>                         polling-delay-passive = <250>;
>                         polling-delay = <2000>;
>                         thermal-sensors = <&tmu>;
>
>                         trips {
>                                 cpu_alert0: trip0 {
>                                         temperature = <85000>;
>                                         hysteresis = <2000>;
>                                         type = "passive";
>                                 };
>
> I can change it to be:
>
>
>         thermal-zones {
>                 critical-action = "reboot";
>
>                 cpu-thermal {
>                         polling-delay-passive = <250>;
>                         polling-delay = <2000>;
>                         thermal-sensors = <&tmu>;
>
>                         trips {
>                                 cpu_alert0: trip0 {
>                                         temperature = <85000>;
>                                         hysteresis = <2000>;
>                                         type = "passive";
>                                 };
>

I think that this would match the use case better.

I still would love to hear about it from the people who take care of
the DT-based thermal control (mostly Daniel and Amit, who BTW is
listed as the maintainer of the file being updated by this patch),
though.
Fabio Estevam Sept. 12, 2023, 3:14 p.m. UTC | #4
Hi Amit and Daniel,

On Fri, Sep 8, 2023 at 3:02 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Fri, Sep 8, 2023 at 7:37 PM Fabio Estevam <festevam@denx.de> wrote:
> >
> > Hi Rafael,
> >
> > On 07/09/2023 13:23, Rafael J. Wysocki wrote:
> >
> > > So one more question here: Why is this a property of a thermal zone
> > > and not the property of the whole system?
> > >
> > > Presumably, on a system where the platform integrator prefers to
> > > reboot on critical temperature, it would be necessary to add this
> > > property to every thermal zone.
> > >
> > > Also, what if this property has different values for different thermal
> > > zones?
> >
> > I got your point and I can make the 'critical-action' property to be
> > valid
> > for the whole thermal system.
> >
> > Originally, I have been doing like this:
> >
> >         thermal-zones {
> >                 cpu-thermal {
> >                         critical-action = "reboot";
> >                         polling-delay-passive = <250>;
> >                         polling-delay = <2000>;
> >                         thermal-sensors = <&tmu>;
> >
> >                         trips {
> >                                 cpu_alert0: trip0 {
> >                                         temperature = <85000>;
> >                                         hysteresis = <2000>;
> >                                         type = "passive";
> >                                 };
> >
> > I can change it to be:
> >
> >
> >         thermal-zones {
> >                 critical-action = "reboot";
> >
> >                 cpu-thermal {
> >                         polling-delay-passive = <250>;
> >                         polling-delay = <2000>;
> >                         thermal-sensors = <&tmu>;
> >
> >                         trips {
> >                                 cpu_alert0: trip0 {
> >                                         temperature = <85000>;
> >                                         hysteresis = <2000>;
> >                                         type = "passive";
> >                                 };
> >
>
> I think that this would match the use case better.
>
> I still would love to hear about it from the people who take care of
> the DT-based thermal control (mostly Daniel and Amit, who BTW is
> listed as the maintainer of the file being updated by this patch),
> though.

Please let us know what you think.

I can send v8 which places the 'critical-action' property to be valid
for the whole thermal system, as suggested by Rafael.

Thanks
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
index 4f3acdc4dec0..c2e4d28f885b 100644
--- a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
+++ b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
@@ -75,6 +75,15 @@  patternProperties:
           framework and assumes that the thermal sensors in this zone
           support interrupts.
 
+      critical-action:
+        $ref: /schemas/types.yaml#/definitions/string
+        description:
+          The action the OS should perform after the critical temperature is reached.
+
+        enum:
+          - shutdown
+          - reboot
+
       thermal-sensors:
         $ref: /schemas/types.yaml#/definitions/phandle-array
         maxItems: 1