| Message ID | 20240402032729.2736685-16-nico@fluxnic.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Mediatek thermal sensor driver support for MT8186 and MT8188 | expand |
Hi Nico, a few comments about this description. On 02/04/2024 05:25, Nicolas Pitre wrote: > From: Nicolas Pitre <npitre@baylibre.com> > > Inspired by the vendor kernel but adapted to the upstream thermal > driver version. [ ... ] > + thermal_zones: thermal-zones { > + cpu-little0-thermal { > + polling-delay = <1000>; Except if I'm wrong, the driver supports the interrupt mode, so it not necessary to poll constantly when there is no mitigation. You can remove the line and everywhere else. > + polling-delay-passive = <250>; As little CPU, 200ms or 150ms may be more adequate. > + thermal-sensors = <&lvts_mcu MT8188_MCU_LITTLE_CPU0>; > + > + trips { > + cpu_little0_alert: trip-alert { > + temperature = <85000>; > + hysteresis = <2000>; > + type = "passive"; > + }; You may want to add a 'hot' trip point in between, so the userspace can be notified and take an action before reaching 'critical' (like unplugging a CPU) > + cpu_little0_crit: trip-crit { > + temperature = <100000>; > + hysteresis = <2000>; critical is a point of no return. Hysteresis does not make sense. These comments apply to all thermal zones. [ .. ] > + cpu_big0-thermal { > + polling-delay = <1000>; > + polling-delay-passive = <250>; Same comments as the little but may be an even lower value. eg. 100ms. > + thermal-sensors = <&lvts_mcu MT8188_MCU_BIG_CPU0>; > + > + trips { > + cpu_big0_alert: trip-alert { > + temperature = <85000>; > + hysteresis = <2000>; > + type = "passive"; > + }; > + > + cpu_big0_crit: trip-crit { > + temperature = <100000>; > + hysteresis = <2000>; > + type = "critical"; > + }; > + }; > + > + cooling-maps { > + map0 { > + trip = <&cpu_big0_alert>; > + cooling-device = <&cpu6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > + <&cpu7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; > + }; > + }; > + }; [ ... ] > + gpu1-thermal { > + polling-delay = <1000>; > + polling-delay-passive = <250>; > + thermal-sensors = <&lvts_ap MT8188_AP_GPU1>; > + > + trips { > + gpu1_alert: trip-alert { > + temperature = <85000>; > + hysteresis = <2000>; > + type = "passive"; > + }; > + > + gpu1_crit: trip-crit { > + temperature = <100000>; > + hysteresis = <2000>; > + type = "critical"; > + }; > + }; > + }; > + > + gpu2-thermal { > + polling-delay = <1000>; > + polling-delay-passive = <250>; > + thermal-sensors = <&lvts_ap MT8188_AP_GPU2>; > + > + trips { > + gpu2_alert: trip-alert { > + temperature = <85000>; > + hysteresis = <2000>; > + type = "passive"; > + }; > + > + gpu2_crit: trip-crit { > + temperature = <100000>; > + hysteresis = <2000>; > + type = "critical"; > + }; > + }; You can add a devfreq cooling device for the GPU here. [ ... ]
On 4/4/24 17:16, Daniel Lezcano wrote: > > Hi Nico, > > a few comments about this description. > > On 02/04/2024 05:25, Nicolas Pitre wrote: >> From: Nicolas Pitre <npitre@baylibre.com> >> >> Inspired by the vendor kernel but adapted to the upstream thermal >> driver version. > > [ ... ] > >> + thermal_zones: thermal-zones { >> + cpu-little0-thermal { >> + polling-delay = <1000>; > > Except if I'm wrong, the driver supports the interrupt mode, so it not necessary to poll > constantly when there is no mitigation. You can remove the line and everywhere else. Hello Daniel, By reading at the comment in the driver, above lvts_irq_handler(), I'm not sure actually. Here is what I read: "The interrupt is configured for thermal events when crossing the hot temperature limit". IOW, lvts_irq_handler() is not meant to notify some "data ready" event, as far as I understand. Don't you think we should keep this polling-delay ? Julien
Hello Daniel, On 4/4/24 17:16, Daniel Lezcano wrote: > > Hi Nico, > > a few comments about this description. > > On 02/04/2024 05:25, Nicolas Pitre wrote: >> From: Nicolas Pitre <npitre@baylibre.com> >> >> Inspired by the vendor kernel but adapted to the upstream thermal >> driver version. > > [ ... ] > >> + thermal_zones: thermal-zones { >> + cpu-little0-thermal { >> + polling-delay = <1000>; > > Except if I'm wrong, the driver supports the interrupt mode, so it not necessary to poll > constantly when there is no mitigation. You can remove the line and everywhere else. > >> + polling-delay-passive = <250>; > > As little CPU, 200ms or 150ms may be more adequate. > >> + thermal-sensors = <&lvts_mcu MT8188_MCU_LITTLE_CPU0>; >> + >> + trips { >> + cpu_little0_alert: trip-alert { >> + temperature = <85000>; >> + hysteresis = <2000>; >> + type = "passive"; >> + }; > > You may want to add a 'hot' trip point in between, so the userspace can be notified and take an > action before reaching 'critical' (like unplugging a CPU) There's no "notify" function in the driver. Do you think it's worth adding such 'hot' trip point, though ? > >> + cpu_little0_crit: trip-crit { >> + temperature = <100000>; >> + hysteresis = <2000>; > > critical is a point of no return. Hysteresis does not make sense. > > These comments apply to all thermal zones. > > [ .. ] > >> + cpu_big0-thermal { >> + polling-delay = <1000>; >> + polling-delay-passive = <250>; > > Same comments as the little but may be an even lower value. eg. 100ms. > >> + thermal-sensors = <&lvts_mcu MT8188_MCU_BIG_CPU0>; >> + >> + trips { >> + cpu_big0_alert: trip-alert { >> + temperature = <85000>; >> + hysteresis = <2000>; >> + type = "passive"; >> + }; >> + >> + cpu_big0_crit: trip-crit { >> + temperature = <100000>; >> + hysteresis = <2000>; >> + type = "critical"; >> + }; >> + }; >> + >> + cooling-maps { >> + map0 { >> + trip = <&cpu_big0_alert>; >> + cooling-device = <&cpu6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, >> + <&cpu7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; >> + }; >> + }; >> + }; > > [ ... ] > >> + gpu1-thermal { >> + polling-delay = <1000>; >> + polling-delay-passive = <250>; >> + thermal-sensors = <&lvts_ap MT8188_AP_GPU1>; >> + >> + trips { >> + gpu1_alert: trip-alert { >> + temperature = <85000>; >> + hysteresis = <2000>; >> + type = "passive"; >> + }; >> + >> + gpu1_crit: trip-crit { >> + temperature = <100000>; >> + hysteresis = <2000>; >> + type = "critical"; >> + }; >> + }; >> + }; >> + >> + gpu2-thermal { >> + polling-delay = <1000>; >> + polling-delay-passive = <250>; >> + thermal-sensors = <&lvts_ap MT8188_AP_GPU2>; >> + >> + trips { >> + gpu2_alert: trip-alert { >> + temperature = <85000>; >> + hysteresis = <2000>; >> + type = "passive"; >> + }; >> + >> + gpu2_crit: trip-crit { >> + temperature = <100000>; >> + hysteresis = <2000>; >> + type = "critical"; >> + }; >> + }; > > You can add a devfreq cooling device for the GPU here. I tried but realized then that GPU support has not been added in the DT yet. So, I added the gpu node (from vendor's kernel), but there are many missing dependencies. I guess it should be part of another series. As a result, I can't add such cooling device here. Julien
On 21/05/2024 13:10, Julien Panis wrote: > Hello Daniel, > > On 4/4/24 17:16, Daniel Lezcano wrote: >> >> Hi Nico, >> >> a few comments about this description. >> >> On 02/04/2024 05:25, Nicolas Pitre wrote: >>> From: Nicolas Pitre <npitre@baylibre.com> >>> >>> Inspired by the vendor kernel but adapted to the upstream thermal >>> driver version. >> >> [ ... ] >> >>> + thermal_zones: thermal-zones { >>> + cpu-little0-thermal { >>> + polling-delay = <1000>; >> >> Except if I'm wrong, the driver supports the interrupt mode, so it not >> necessary to poll constantly when there is no mitigation. You can >> remove the line and everywhere else. >> >>> + polling-delay-passive = <250>; >> >> As little CPU, 200ms or 150ms may be more adequate. >> >>> + thermal-sensors = <&lvts_mcu MT8188_MCU_LITTLE_CPU0>; >>> + >>> + trips { >>> + cpu_little0_alert: trip-alert { >>> + temperature = <85000>; >>> + hysteresis = <2000>; >>> + type = "passive"; >>> + }; >> >> You may want to add a 'hot' trip point in between, so the userspace >> can be notified and take an action before reaching 'critical' (like >> unplugging a CPU) > > There's no "notify" function in the driver. The trip point crossing is always notified to userspace in the core code. It is not driver specific. > Do you think it's worth adding such 'hot' trip point, though ? Having this trip point would allow generic trip point handling in a userspace daemon to do an action. If the userspace does not take care of this event, it won't hurt.
Il 21/05/24 13:31, Daniel Lezcano ha scritto: > On 21/05/2024 13:10, Julien Panis wrote: >> Hello Daniel, >> >> On 4/4/24 17:16, Daniel Lezcano wrote: >>> >>> Hi Nico, >>> >>> a few comments about this description. >>> >>> On 02/04/2024 05:25, Nicolas Pitre wrote: >>>> From: Nicolas Pitre <npitre@baylibre.com> >>>> >>>> Inspired by the vendor kernel but adapted to the upstream thermal >>>> driver version. >>> >>> [ ... ] >>> >>>> + thermal_zones: thermal-zones { >>>> + cpu-little0-thermal { >>>> + polling-delay = <1000>; >>> >>> Except if I'm wrong, the driver supports the interrupt mode, so it not necessary >>> to poll constantly when there is no mitigation. You can remove the line and >>> everywhere else. >>> >>>> + polling-delay-passive = <250>; >>> >>> As little CPU, 200ms or 150ms may be more adequate. >>> >>>> + thermal-sensors = <&lvts_mcu MT8188_MCU_LITTLE_CPU0>; >>>> + >>>> + trips { >>>> + cpu_little0_alert: trip-alert { >>>> + temperature = <85000>; >>>> + hysteresis = <2000>; >>>> + type = "passive"; >>>> + }; >>> >>> You may want to add a 'hot' trip point in between, so the userspace can be >>> notified and take an action before reaching 'critical' (like unplugging a CPU) >> >> There's no "notify" function in the driver. > > The trip point crossing is always notified to userspace in the core code. It is not > driver specific. > >> Do you think it's worth adding such 'hot' trip point, though ? > > Having this trip point would allow generic trip point handling in a userspace > daemon to do an action. > > If the userspace does not take care of this event, it won't hurt. > > > Hello Julien, I'll push two series tomorrow to add most of the power domains (all but the img_vcore ones, as those require a bit more work), the two vdosys and, more importantly, support for the GPU in both panfrost and the MT8188 devicetree. Actually, if it's not tomorrow, it's going to be on Monday - though I'm not sure why you feel like you're blocked by not having the GPU node in there. Cheers, Angelo
On 5/21/24 16:07, AngeloGioacchino Del Regno wrote: > Il 21/05/24 13:31, Daniel Lezcano ha scritto: >> On 21/05/2024 13:10, Julien Panis wrote: >>> Hello Daniel, >>> >>> On 4/4/24 17:16, Daniel Lezcano wrote: >>>> >>>> Hi Nico, >>>> >>>> a few comments about this description. >>>> >>>> On 02/04/2024 05:25, Nicolas Pitre wrote: >>>>> From: Nicolas Pitre <npitre@baylibre.com> >>>>> >>>>> Inspired by the vendor kernel but adapted to the upstream thermal >>>>> driver version. >>>> >>>> [ ... ] >>>> >>>>> + thermal_zones: thermal-zones { >>>>> + cpu-little0-thermal { >>>>> + polling-delay = <1000>; >>>> >>>> Except if I'm wrong, the driver supports the interrupt mode, so it not necessary to poll >>>> constantly when there is no mitigation. You can remove the line and everywhere else. >>>> >>>>> + polling-delay-passive = <250>; >>>> >>>> As little CPU, 200ms or 150ms may be more adequate. >>>> >>>>> + thermal-sensors = <&lvts_mcu MT8188_MCU_LITTLE_CPU0>; >>>>> + >>>>> + trips { >>>>> + cpu_little0_alert: trip-alert { >>>>> + temperature = <85000>; >>>>> + hysteresis = <2000>; >>>>> + type = "passive"; >>>>> + }; >>>> >>>> You may want to add a 'hot' trip point in between, so the userspace can be notified and take an >>>> action before reaching 'critical' (like unplugging a CPU) >>> >>> There's no "notify" function in the driver. >> >> The trip point crossing is always notified to userspace in the core code. It is not driver specific. >> >>> Do you think it's worth adding such 'hot' trip point, though ? >> >> Having this trip point would allow generic trip point handling in a userspace daemon to do an >> action. >> >> If the userspace does not take care of this event, it won't hurt. >> >> >> > > Hello Julien, > > I'll push two series tomorrow to add most of the power domains (all but the > img_vcore ones, as those require a bit more work), the two vdosys and, more > importantly, support for the GPU in both panfrost and the MT8188 devicetree. > > Actually, if it's not tomorrow, it's going to be on Monday - though I'm not > sure why you feel like you're blocked by not having the GPU node in there. > > Cheers, > Angelo Hello Angelo, Sorry, I sent the v4 a few seconds before receiving your email. :) OK for the GPU support, thanks. After that, I'll submit a v5 that will include the related cooling-device. IIUC, this should look something like that: cooling-maps { map0 { trip = <&gpu1_alert0>; cooling-device = <&gpu THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; } } That's why I need some 'gpu' node somewhere in the device tree. The build fails without it. Julien
diff --git a/arch/arm64/boot/dts/mediatek/mt8188.dtsi b/arch/arm64/boot/dts/mediatek/mt8188.dtsi index 101c9c9f4b..1101dd1b75 100644 --- a/arch/arm64/boot/dts/mediatek/mt8188.dtsi +++ b/arch/arm64/boot/dts/mediatek/mt8188.dtsi @@ -12,6 +12,8 @@ #include <dt-bindings/pinctrl/mediatek,mt8188-pinfunc.h> #include <dt-bindings/power/mediatek,mt8188-power.h> #include <dt-bindings/reset/mt8188-resets.h> +#include <dt-bindings/thermal/thermal.h> +#include <dt-bindings/thermal/mediatek,lvts-thermal.h> / { compatible = "mediatek,mt8188"; @@ -311,6 +313,352 @@ psci { method = "smc"; }; + thermal_zones: thermal-zones { + cpu-little0-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts_mcu MT8188_MCU_LITTLE_CPU0>; + + trips { + cpu_little0_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + cpu_little0_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + + cooling-maps { + map0 { + trip = <&cpu_little0_alert>; + cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; + }; + }; + }; + + cpu-little1-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts_mcu MT8188_MCU_LITTLE_CPU1>; + + trips { + cpu_little1_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + cpu_little1_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + + cooling-maps { + map0 { + trip = <&cpu_little1_alert>; + cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; + }; + }; + }; + + cpu-little2-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts_mcu MT8188_MCU_LITTLE_CPU2>; + + trips { + cpu_little2_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + cpu_little2_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + + cooling-maps { + map0 { + trip = <&cpu_little2_alert>; + cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; + }; + }; + }; + + cpu-little3-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts_mcu MT8188_MCU_LITTLE_CPU3>; + + trips { + cpu_little3_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + cpu_little3_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + + cooling-maps { + map0 { + trip = <&cpu_little3_alert>; + cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; + }; + }; + }; + + cpu_big0-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts_mcu MT8188_MCU_BIG_CPU0>; + + trips { + cpu_big0_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + cpu_big0_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + + cooling-maps { + map0 { + trip = <&cpu_big0_alert>; + cooling-device = <&cpu6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; + }; + }; + }; + + cpu_big1-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts_mcu MT8188_MCU_BIG_CPU1>; + + trips { + cpu_big1_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + cpu_big1_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + + cooling-maps { + map0 { + trip = <&cpu_big1_alert>; + cooling-device = <&cpu6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; + }; + }; + }; + + apu-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts_ap MT8188_AP_APU>; + + trips { + apu_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + apu_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + }; + + gpu1-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts_ap MT8188_AP_GPU1>; + + trips { + gpu1_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + gpu1_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + }; + + gpu2-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts_ap MT8188_AP_GPU2>; + + trips { + gpu2_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + gpu2_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + }; + + soc1-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts_ap MT8188_AP_SOC1>; + + trips { + soc1_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + soc1_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + }; + + soc2-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts_ap MT8188_AP_SOC2>; + + trips { + soc2_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + soc2_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + }; + + soc3-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts_ap MT8188_AP_SOC3>; + + trips { + soc3_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + soc3_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + }; + + cam1-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts_ap MT8188_AP_CAM1>; + + trips { + cam1_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + cam1_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + }; + + cam2-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts_ap MT8188_AP_CAM2>; + + trips { + cam2_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + cam2_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + }; + }; + timer: timer { compatible = "arm,armv8-timer"; interrupt-parent = <&gic>;