Re: thermal/drivers/tegra: Getting rid of the get_thermal_instance() usage
From: Daniel Lezcano
Date: Fri Feb 10 2023 - 09:37:09 EST
On 10/02/2023 15:09, Thierry Reding wrote:
On Fri, Feb 10, 2023 at 02:17:03PM +0100, Daniel Lezcano wrote:
Hi Thierry,
On Thu, Jan 26, 2023 at 01:55:52PM +0100, Thierry Reding wrote:
On Tue, Jan 24, 2023 at 08:57:23PM +0100, Daniel Lezcano wrote:
Hi,
does anyone know what is the purpose of the get_thermal_instance() usage in
this code:
https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/drivers/thermal/tegra/soctherm.c?h=thermal/linux-next#n623
The driver is using a function which is reserved for the thermal core. It
should not.
Is the following change ok ?
diff --git a/drivers/thermal/tegra/soctherm.c
b/drivers/thermal/tegra/soctherm.c
index 220873298d77..5f552402d987 100644
--- a/drivers/thermal/tegra/soctherm.c
+++ b/drivers/thermal/tegra/soctherm.c
@@ -620,9 +620,8 @@ static int tegra_thermctl_set_trip_temp(struct
thermal_zone_device *tz, int trip
continue;
cdev = ts->throt_cfgs[i].cdev;
- if (get_thermal_instance(tz, cdev, trip_id))
- stc = find_throttle_cfg_by_name(ts, cdev->type);
- else
+ stc = find_throttle_cfg_by_name(ts, cdev->type);
+ if (!stc)
continue;
return throttrip_program(dev, sg, stc, temp);
@@ -768,9 +767,9 @@ static int tegra_soctherm_set_hwtrips(struct device
*dev,
continue;
cdev = ts->throt_cfgs[i].cdev;
- if (get_thermal_instance(tz, cdev, trip))
- stc = find_throttle_cfg_by_name(ts, cdev->type);
- else
+
+ stc = find_throttle_cfg_by_name(ts, cdev->type);
+ if (!stc)
continue;
ret = throttrip_program(dev, sg, stc, temperature);
There's a small difference in behavior after applying this patch. Prior
to this I get (on Tegra210):
[ 12.354091] tegra_soctherm 700e2000.thermal-sensor: missing thermtrips, will use critical trips as shut down temp
[ 12.379009] tegra_soctherm 700e2000.thermal-sensor: thermtrip: will shut down when cpu reaches 102500 mC
[ 12.388882] tegra_soctherm 700e2000.thermal-sensor: programming throttle for cpu to 102500
[ 12.401007] tegra_soctherm 700e2000.thermal-sensor: throttrip: will throttle when cpu reaches 102500 mC
[ 12.471041] tegra_soctherm 700e2000.thermal-sensor: thermtrip: will shut down when gpu reaches 103000 mC
[ 12.482852] tegra_soctherm 700e2000.thermal-sensor: programming throttle for gpu to 103000
[ 12.482860] tegra_soctherm 700e2000.thermal-sensor: throttrip: will throttle when gpu reaches 103000 mC
[ 12.485357] tegra_soctherm 700e2000.thermal-sensor: thermtrip: will shut down when pll reaches 103000 mC
[ 12.501774] tegra_soctherm 700e2000.thermal-sensor: thermtrip: will shut down when mem reaches 103000 mC
and after these changes, it turns into:
[ 12.447113] tegra_soctherm 700e2000.thermal-sensor: missing thermtrips, will use critical trips as shut down temp
[ 12.472300] tegra_soctherm 700e2000.thermal-sensor: thermtrip: will shut down when cpu reaches 102500 mC
[ 12.481789] tegra_soctherm 700e2000.thermal-sensor: programming throttle for cpu to 102500
[ 12.495447] tegra_soctherm 700e2000.thermal-sensor: throttrip: will throttle when cpu reaches 102500 mC
[ 12.496514] tegra_soctherm 700e2000.thermal-sensor: thermtrip: will shut down when gpu reaches 103000 mC
[ 12.510353] tegra_soctherm 700e2000.thermal-sensor: programming throttle for gpu to 103000
[ 12.526856] tegra_soctherm 700e2000.thermal-sensor: throttrip: will throttle when gpu reaches 103000 mC
[ 12.528774] tegra_soctherm 700e2000.thermal-sensor: thermtrip: will shut down when pll reaches 103000 mC
[ 12.569352] tegra_soctherm 700e2000.thermal-sensor: programming throttle for pll to 103000
[ 12.577635] tegra_soctherm 700e2000.thermal-sensor: throttrip: will throttle when pll reaches 103000 mC
[ 12.590952] tegra_soctherm 700e2000.thermal-sensor: thermtrip: will shut down when mem reaches 103000 mC
[ 12.600783] tegra_soctherm 700e2000.thermal-sensor: programming throttle for mem to 103000
[ 12.609204] tegra_soctherm 700e2000.thermal-sensor: throttrip: will throttle when mem reaches 103000 mC
The "programming throttle ..." messages are something I've added locally
to trace what gets called. So it looks like for "pll" and "mem" thermal
zones, we now program trip points whereas we previously didn't.
The DT descriptioni (tegra210.dtsi) says one thing and the implementation says
something else.
If we refer to the PLL description, there is one 'hot' trip point and
one 'critical' trip point. No polling delay at all, so we need the
interrupts.
Logically, we should set the 'hot' trip point first, when the trip
point is crossed, we setup the next trip point, which is the critical.
With these two trip points, the first one will send a notification to
the userspace and the second one will force a shutdown of the
system. For both, no cooling device is expected.
I think the intention here is to use the soctherm's built-in throttling
mechanism as a last resort measure to try and cool the system down. I
suppose that could count as "passive" cooling, so specifying it as the
cooling device for the "passive" trip point may be more appropriate.
The throttling that happens here is quite severe, so we don't want it to
happen too early. I would expect that our "passive" trip point shouldn't
be a lot less than the "hot" temperature. I suspect that's the reason
why the "hot" trip point was reused for this.
I'm also beginning to think that we should just not expose the soctherm
throttling as a cooling device and instead keep it internal to the
soctherm driver entirely.
Yes, and perhaps separate it from the sensor driver.
There is a similar hardware limiter for the qcom platform [1]. The
description in the device tree is separated from the sensor and the
binding has temperatures to begin the mitigation [2].
There is no trip point associated as those are related to the in-kernel
mitigation.
If this mitigation is a heavy mitigation, above what the kernel is able
to do with a passive cooling device. It would make sense to just have
configured outside of the thermal zone.
So the configuration would be something like:
myperformance_limite {
@ = <0x...>
temperature_limit = 95000;
};
thermal_zone {
cpu : {
trips {
alert {
temperature = 90000;
hysteresis = 2000;
type = passive;
};
hot {
temperature = 97000;
type = hot;
};
critical {
temperature = 100000;
hysteresis = 2000;
type = critical;
};
cooling-maps = <&cpu NO_LIMIT NO_LIMIT>;
};
}
};
The behavior will be a passive mitigation, if it fails the hardware
limiter will take over, if that fails then hot sends a notification to
the userspace (giving the opportunity to hotplug a cpu or kill a task or
suspend), if that fails then shutdown.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/drivers/thermal/qcom/lmh.c?h=thermal/bleeding-edge
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/arch/arm64/boot/dts/qcom/sdm845.dtsi?h=thermal/bleeding-edge#n3922
[ ... ]
On the DT side, I think most of the cooling maps can be cleaned up. We
can remove the entries for "critical" and "hot" trip points if the
driver unconditionally programs the automated throttling.
You may want to keep the critical trip points at least. Even if the
hardware limiter is certainly very effective, having the critical point
is another fail safe allowing to gracefully shutdown the system before a
wild hardware reset.
For EMC we
want to reverse the "passive" and "active" trip points and possibly drop
the dram-passive cooling map as well, since you mentioned the core would
take care of disabling the cooling device automatically.
--
<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