Messages in this thread | | | Date | Tue, 11 Apr 2023 12:48:25 +0200 | Subject | Re: thermal/drivers/tegra: Getting rid of the get_thermal_instance() usage | From | Daniel Lezcano <> |
| |
Hi Thierry,
did you have time to look at this ?
This driver is the only one using get_thermal_instance() and I would like to remove this function along with the thermal_core.h inclusion in this driver
Thanks -- Daniel
On 10/02/2023 16:12, Thierry Reding wrote: > On Fri, Feb 10, 2023 at 03:36:59PM +0100, Daniel Lezcano wrote: >> 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]. > > The hardware throttling is controlled using registers that are part of > the SOCTHERM block, so we can't separate it from the sensor driver. I > don't think that's much of a problem, though. The code for this already > exists in the current soctherm driver, so it's just a matter of removing > the cooling device registration code. > >> >> 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. > > Yeah, that's exactly what I had in mind. > >> [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. > > Yeah. What I meant was to remove only the cooling map entries for > critical and hot since they would be unused. We absolutely want to > keep the trip points themselves around to make sure the system will > forcefully shutdown as a last resort. > > Thierry
-- <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
| |