Re: [PATCH] arm64: dts: renesas: r8a779g3: Update thermal trip points on V4H Sparrow Hawk

["Niklas Söderlund" <niklas.soderlund@ragnatech.se>]

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@ragnatech.se>

-- 
Kind Regards,
Niklas Söderlund