Hello Marek,
On 2025-06-30 00:32:54 +0200, Marek Vasut wrote:
> On 6/26/25 11:41 PM, Niklas Söderlund wrote:
>
> Hello Niklas,
>
> > > +&a76_3 {
> > > + a76_3_thermal_idle: thermal-idle {
> > > + #cooling-cells = <2>;
> > > + duration-us = <10000>;
> > > + exit-latency-us = <500>;
> > > + };
> > > +};
> >
> > I did not know you could do this and use it as a cooling device, thanks
> > for teaching me something new!
>
> You could, although the cooling effect may vary. Some cores enter e.g. clock
> stop during idle and then they really cool down, some do not.
>
> > > +/* THS sensors in SoC, critical temperature trip point is 100C */
> > > +&sensor1_crit {
> > > + temperature = <100000>;
> > > +};
> > > +
> > > +&sensor2_crit {
> > > + temperature = <100000>;
> > > +};
> > > +
> > > +&sensor3_crit {
> > > + temperature = <100000>;
> > > +};
> > > +
> > > +&sensor4_crit {
> > > + temperature = <100000>;
> > > +};
> > > +
> > > +&sensor_thermal_cr52 {
> > > + critical-action = "shutdown";
> > > +};
> > > +
> > > +&sensor_thermal_cnn {
> > > + critical-action = "shutdown";
> > > +};
> >
> > Is this not the default action for critical trip points? In my testing
> > in the past R-Car systems have always shutdown when the critical trip is
> > reached.
>
> It isn't quite that clear cut.
>
> drivers/thermal/thermal_of.c thermal_of_zone_register() contains this piece
> of code:
>
> "
> 407 ret = of_property_read_string(np, "critical-action", &action);
> 408 if (!ret && !of_ops.critical) {
> 409 if (!strcasecmp(action, "reboot"))
> 410 of_ops.critical =
> thermal_zone_device_critical_reboot;
> 411 else if (!strcasecmp(action, "shutdown"))
> 412 of_ops.critical =
> thermal_zone_device_critical_shutdown;
> 413 }
> "
>
> If "critical-action" DT property is not set, then of_ops.critical are not
> modified.
>
> drivers/thermal/thermal_core.c thermal_zone_device_register_with_trips()
> contains this piece of code:
>
> 1571 if (!tz->ops.critical)
> 1572 tz->ops.critical = thermal_zone_device_critical;
>
> If (in case of OF) of_ops.critical is not set, use
> thermal_zone_device_critical() handler.
>
> There is a slight difference:
> - If critical-action = "shutdown" is set in DT, then handler
> thermal_zone_device_critical_shutdown() is called, which is a wrapper
> around thermal_zone_device_halt(tz, HWPROT_ACT_SHUTDOWN);
> - If critical-action = "shutdown" is NOT set in DT, then handler
> thermal_zone_device_critical() is called, which is a wrapper
> around thermal_zone_device_halt(tz, HWPROT_ACT_DEFAULT);
>
> thermal_zone_device_halt() itself is a wrapper around
> __hw_protection_trigger(msg, poweroff_delay_ms, action); , where action is
> either HWPROT_ACT_SHUTDOWN or HWPROT_ACT_DEFAULT , which is handled in
> kernel/reboot.c __hw_protection_trigger() implementation :
>
> 1028 void __hw_protection_trigger(const char *reason, int ms_until_forced,
> 1029 enum hw_protection_action action)
> 1030 {
> 1031 static atomic_t allow_proceed = ATOMIC_INIT(1);
> 1032
> 1033 if (action == HWPROT_ACT_DEFAULT)
> 1034 action = hw_protection_action;
>
> In case of HWPROT_ACT_DEFAULT , the 'hw_protection_action' which is assigned
> into 'action' can be overridden, either via sysfs write, or hw_protection_
> kernel command line parameter . In case of HWPROT_ACT_SHUTDOWN , the action
> cannot be overridden .
>
> In case this hardware starts to melt, we surely want HWPROT_ACT_SHUTDOWN
> with no override options ...
>
> > If it's not I think we should move these to r8a779g0.dtsi. And
> > likely add them for all other SoCs too?
>
> ... the other hardware has non-optional heatsink, where override-able
> HWPROT_ACT_DEFAULT is the right option I think .
Wow, thanks for the detailed rundown. With that I agree with you, we
should only force the shutdown on this particular platform. Nice work.
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
--
Kind Regards,
Niklas Söderlund
