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

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(+)

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

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

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 ?

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!

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

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.

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

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

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