| Message ID | 20240229-rk-dts-additions-v3-1-6afe8473a631@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | RK3588 and Rock 5B dts additions: thermal, OPP and fan | expand |
Hello Alexey, On 2024-02-29 20:26, Alexey Charkov wrote: > Include thermal zones information in device tree for RK3588 variants. > > This also enables the TSADC controller unconditionally on all boards > to ensure that thermal protections are in place via throttling and > emergency reset, once OPPs are added to enable CPU DVFS. > > The default settings (using CRU as the emergency reset mechanism) > should work on all boards regardless of their wiring, as CRU resets > do not depend on any external components. Boards that have the TSHUT > signal wired to the reset line of the PMIC may opt to switch to GPIO > tshut mode instead (rockchip,hw-tshut-mode = <1>;) Quite frankly, I'm still not sure that enabling this on the SoC level is the way to go. As I already described in detail, [4] according to the RK3588 Hardware Design Guide v1.0 and the Rock 5B schematic, we should actually use GPIO-based handling for the thermal runaways on the Rock 5B. Other boards should also be investigated individually, and the TSADC should be enabled on a board-to-board basis. [4] https://lore.kernel.org/linux-rockchip/4e7c2b5a938bd7c919b852699c951701@manjaro.org/ > It seems though that downstream kernels don't use that, even for > those boards where the wiring allows for GPIO based tshut, such as > Radxa Rock 5B [1], [2], [3] > > [1] > https://github.com/radxa/kernel/blob/stable-5.10-rock5/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts#L540 > [2] > https://github.com/radxa/kernel/blob/stable-5.10-rock5/arch/arm64/boot/dts/rockchip/rk3588s.dtsi#L5433 > [3] https://dl.radxa.com/rock5/5b/docs/hw/radxa_rock_5b_v1423_sch.pdf > page 11 (TSADC_SHUT_H) > > Signed-off-by: Alexey Charkov <alchark@gmail.com> > --- > arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 176 > +++++++++++++++++++++++++++++- > 1 file changed, 175 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi > b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi > index 36b1b7acfe6a..9bf197358642 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi > @@ -10,6 +10,7 @@ > #include <dt-bindings/reset/rockchip,rk3588-cru.h> > #include <dt-bindings/phy/phy.h> > #include <dt-bindings/ata/ahci.h> > +#include <dt-bindings/thermal/thermal.h> > > / { > compatible = "rockchip,rk3588"; > @@ -2225,7 +2226,180 @@ tsadc: tsadc@fec00000 { > pinctrl-1 = <&tsadc_shut>; > pinctrl-names = "gpio", "otpout"; > #thermal-sensor-cells = <1>; > - status = "disabled"; > + status = "okay"; > + }; > + > + thermal_zones: thermal-zones { > + /* sensor near the center of the SoC */ > + package_thermal: package-thermal { > + polling-delay-passive = <0>; > + polling-delay = <0>; > + thermal-sensors = <&tsadc 0>; > + > + trips { > + package_crit: package-crit { > + temperature = <115000>; > + hysteresis = <0>; > + type = "critical"; > + }; > + }; > + }; > + > + /* sensor between A76 cores 0 and 1 */ > + bigcore0_thermal: bigcore0-thermal { > + polling-delay-passive = <100>; > + polling-delay = <0>; > + thermal-sensors = <&tsadc 1>; > + > + trips { > + /* threshold to start collecting temperature > + * statistics e.g. with the IPA governor > + */ > + bigcore0_alert0: bigcore0-alert0 { > + temperature = <75000>; > + hysteresis = <2000>; > + type = "passive"; > + }; > + /* actual control temperature */ > + bigcore0_alert1: bigcore0-alert1 { > + temperature = <85000>; > + hysteresis = <2000>; > + type = "passive"; > + }; > + bigcore0_crit: bigcore0-crit { > + temperature = <115000>; > + hysteresis = <0>; > + type = "critical"; > + }; > + }; > + cooling-maps { > + map0 { > + trip = <&bigcore0_alert1>; > + cooling-device = > + <&cpu_b0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > + <&cpu_b1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; > + }; > + }; > + }; > + > + /* sensor between A76 cores 2 and 3 */ > + bigcore2_thermal: bigcore2-thermal { > + polling-delay-passive = <100>; > + polling-delay = <0>; > + thermal-sensors = <&tsadc 2>; > + > + trips { > + /* threshold to start collecting temperature > + * statistics e.g. with the IPA governor > + */ > + bigcore2_alert0: bigcore2-alert0 { > + temperature = <75000>; > + hysteresis = <2000>; > + type = "passive"; > + }; > + /* actual control temperature */ > + bigcore2_alert1: bigcore2-alert1 { > + temperature = <85000>; > + hysteresis = <2000>; > + type = "passive"; > + }; > + bigcore2_crit: bigcore2-crit { > + temperature = <115000>; > + hysteresis = <0>; > + type = "critical"; > + }; > + }; > + cooling-maps { > + map0 { > + trip = <&bigcore2_alert1>; > + cooling-device = > + <&cpu_b2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > + <&cpu_b3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; > + }; > + }; > + }; > + > + /* sensor between the four A55 cores */ > + little_core_thermal: littlecore-thermal { > + polling-delay-passive = <100>; > + polling-delay = <0>; > + thermal-sensors = <&tsadc 3>; > + > + trips { > + /* threshold to start collecting temperature > + * statistics e.g. with the IPA governor > + */ > + littlecore_alert0: littlecore-alert0 { > + temperature = <75000>; > + hysteresis = <2000>; > + type = "passive"; > + }; > + /* actual control temperature */ > + littlecore_alert1: littlecore-alert1 { > + temperature = <85000>; > + hysteresis = <2000>; > + type = "passive"; > + }; > + littlecore_crit: littlecore-crit { > + temperature = <115000>; > + hysteresis = <0>; > + type = "critical"; > + }; > + }; > + cooling-maps { > + map0 { > + trip = <&littlecore_alert1>; > + cooling-device = > + <&cpu_l0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > + <&cpu_l1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > + <&cpu_l2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > + <&cpu_l3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; > + }; > + }; > + }; > + > + /* sensor near the PD_CENTER power domain */ > + center_thermal: center-thermal { > + polling-delay-passive = <0>; > + polling-delay = <0>; > + thermal-sensors = <&tsadc 4>; > + > + trips { > + center_crit: center-crit { > + temperature = <115000>; > + hysteresis = <0>; > + type = "critical"; > + }; > + }; > + }; > + > + gpu_thermal: gpu-thermal { > + polling-delay-passive = <0>; > + polling-delay = <0>; > + thermal-sensors = <&tsadc 5>; > + > + trips { > + gpu_crit: gpu-crit { > + temperature = <115000>; > + hysteresis = <0>; > + type = "critical"; > + }; > + }; > + }; > + > + npu_thermal: npu-thermal { > + polling-delay-passive = <0>; > + polling-delay = <0>; > + thermal-sensors = <&tsadc 6>; > + > + trips { > + npu_crit: npu-crit { > + temperature = <115000>; > + hysteresis = <0>; > + type = "critical"; > + }; > + }; > + }; > }; > > saradc: adc@fec10000 {
Hello Alexey, Please see also some nitpicks below, which I forgot to mention in my earlier response. I'm sorry for that. On 2024-02-29 20:26, Alexey Charkov wrote: > Include thermal zones information in device tree for RK3588 variants. > > This also enables the TSADC controller unconditionally on all boards > to ensure that thermal protections are in place via throttling and > emergency reset, once OPPs are added to enable CPU DVFS. > > The default settings (using CRU as the emergency reset mechanism) > should work on all boards regardless of their wiring, as CRU resets > do not depend on any external components. Boards that have the TSHUT > signal wired to the reset line of the PMIC may opt to switch to GPIO > tshut mode instead (rockchip,hw-tshut-mode = <1>;) > > It seems though that downstream kernels don't use that, even for > those boards where the wiring allows for GPIO based tshut, such as > Radxa Rock 5B [1], [2], [3] > > [1] > https://github.com/radxa/kernel/blob/stable-5.10-rock5/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts#L540 > [2] > https://github.com/radxa/kernel/blob/stable-5.10-rock5/arch/arm64/boot/dts/rockchip/rk3588s.dtsi#L5433 > [3] https://dl.radxa.com/rock5/5b/docs/hw/radxa_rock_5b_v1423_sch.pdf > page 11 (TSADC_SHUT_H) > > Signed-off-by: Alexey Charkov <alchark@gmail.com> > --- > arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 176 > +++++++++++++++++++++++++++++- > 1 file changed, 175 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi > b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi > index 36b1b7acfe6a..9bf197358642 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi > @@ -10,6 +10,7 @@ > #include <dt-bindings/reset/rockchip,rk3588-cru.h> > #include <dt-bindings/phy/phy.h> > #include <dt-bindings/ata/ahci.h> > +#include <dt-bindings/thermal/thermal.h> > > / { > compatible = "rockchip,rk3588"; > @@ -2225,7 +2226,180 @@ tsadc: tsadc@fec00000 { > pinctrl-1 = <&tsadc_shut>; > pinctrl-names = "gpio", "otpout"; > #thermal-sensor-cells = <1>; > - status = "disabled"; > + status = "okay"; > + }; > + > + thermal_zones: thermal-zones { > + /* sensor near the center of the SoC */ > + package_thermal: package-thermal { > + polling-delay-passive = <0>; > + polling-delay = <0>; > + thermal-sensors = <&tsadc 0>; > + > + trips { > + package_crit: package-crit { > + temperature = <115000>; > + hysteresis = <0>; > + type = "critical"; > + }; > + }; > + }; > + > + /* sensor between A76 cores 0 and 1 */ > + bigcore0_thermal: bigcore0-thermal { > + polling-delay-passive = <100>; > + polling-delay = <0>; > + thermal-sensors = <&tsadc 1>; > + > + trips { > + /* threshold to start collecting temperature > + * statistics e.g. with the IPA governor > + */ See, I'm not a native English speaker, but I've spent a lot of time and effort improving my English skills. Thus, perhaps these comments may or may not seem like unnecessary nitpicking, depending on how much someone pays attention to writing style in general, but I'll risk to be annoying and state these comments anyway. :) The comment above could be written in a much more condensed form like this, which would also be a bit more accurate: /* IPA threshold, when IPA governor is used */ IOW, we're writing all this for someone to read later, but we should (and can) perfectly reasonably expect some already existing background knowledge from the readers. In other words, we should be as concise as possible. > + bigcore0_alert0: bigcore0-alert0 { > + temperature = <75000>; > + hysteresis = <2000>; > + type = "passive"; > + }; > + /* actual control temperature */ Similarly to the above, I'd suggest this: /* IPA target, when IPA governor is used */ Having such brief comments should make it all perfectly understandable to anyone who's already familiar with the way IPA governor works. Everyone else should be welcome to read up a bit on IPA first. > + bigcore0_alert1: bigcore0-alert1 { > + temperature = <85000>; > + hysteresis = <2000>; > + type = "passive"; > + }; > + bigcore0_crit: bigcore0-crit { > + temperature = <115000>; > + hysteresis = <0>; > + type = "critical"; > + }; > + }; > + cooling-maps { > + map0 { > + trip = <&bigcore0_alert1>; > + cooling-device = > + <&cpu_b0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > + <&cpu_b1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; > + }; > + }; > + }; > + > + /* sensor between A76 cores 2 and 3 */ > + bigcore2_thermal: bigcore2-thermal { > + polling-delay-passive = <100>; > + polling-delay = <0>; > + thermal-sensors = <&tsadc 2>; > + > + trips { > + /* threshold to start collecting temperature > + * statistics e.g. with the IPA governor > + */ The same as above. > + bigcore2_alert0: bigcore2-alert0 { > + temperature = <75000>; > + hysteresis = <2000>; > + type = "passive"; > + }; > + /* actual control temperature */ The same as above. > + bigcore2_alert1: bigcore2-alert1 { > + temperature = <85000>; > + hysteresis = <2000>; > + type = "passive"; > + }; > + bigcore2_crit: bigcore2-crit { > + temperature = <115000>; > + hysteresis = <0>; > + type = "critical"; > + }; > + }; > + cooling-maps { > + map0 { > + trip = <&bigcore2_alert1>; > + cooling-device = > + <&cpu_b2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > + <&cpu_b3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; > + }; > + }; > + }; > + > + /* sensor between the four A55 cores */ > + little_core_thermal: littlecore-thermal { > + polling-delay-passive = <100>; > + polling-delay = <0>; > + thermal-sensors = <&tsadc 3>; > + > + trips { > + /* threshold to start collecting temperature > + * statistics e.g. with the IPA governor > + */ The same as above. > + littlecore_alert0: littlecore-alert0 { > + temperature = <75000>; > + hysteresis = <2000>; > + type = "passive"; > + }; > + /* actual control temperature */ The same as above. > + littlecore_alert1: littlecore-alert1 { > + temperature = <85000>; > + hysteresis = <2000>; > + type = "passive"; > + }; > + littlecore_crit: littlecore-crit { > + temperature = <115000>; > + hysteresis = <0>; > + type = "critical"; > + }; > + }; > + cooling-maps { > + map0 { > + trip = <&littlecore_alert1>; > + cooling-device = > + <&cpu_l0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > + <&cpu_l1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > + <&cpu_l2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > + <&cpu_l3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; > + }; > + }; > + }; > + > + /* sensor near the PD_CENTER power domain */ > + center_thermal: center-thermal { > + polling-delay-passive = <0>; > + polling-delay = <0>; > + thermal-sensors = <&tsadc 4>; > + > + trips { > + center_crit: center-crit { > + temperature = <115000>; > + hysteresis = <0>; > + type = "critical"; > + }; > + }; > + }; > + > + gpu_thermal: gpu-thermal { > + polling-delay-passive = <0>; > + polling-delay = <0>; > + thermal-sensors = <&tsadc 5>; > + > + trips { > + gpu_crit: gpu-crit { > + temperature = <115000>; > + hysteresis = <0>; > + type = "critical"; > + }; > + }; > + }; > + > + npu_thermal: npu-thermal { > + polling-delay-passive = <0>; > + polling-delay = <0>; > + thermal-sensors = <&tsadc 6>; > + > + trips { > + npu_crit: npu-crit { > + temperature = <115000>; > + hysteresis = <0>; > + type = "critical"; > + }; > + }; > + }; > }; > > saradc: adc@fec10000 {
Hi Dragan, On Fri, Mar 1, 2024 at 12:21 AM Dragan Simic <dsimic@manjaro.org> wrote: > > Hello Alexey, > > On 2024-02-29 20:26, Alexey Charkov wrote: > > Include thermal zones information in device tree for RK3588 variants. > > > > This also enables the TSADC controller unconditionally on all boards > > to ensure that thermal protections are in place via throttling and > > emergency reset, once OPPs are added to enable CPU DVFS. > > > > The default settings (using CRU as the emergency reset mechanism) > > should work on all boards regardless of their wiring, as CRU resets > > do not depend on any external components. Boards that have the TSHUT > > signal wired to the reset line of the PMIC may opt to switch to GPIO > > tshut mode instead (rockchip,hw-tshut-mode = <1>;) > > Quite frankly, I'm still not sure that enabling this on the SoC level > is the way to go. As I already described in detail, [4] according to > the RK3588 Hardware Design Guide v1.0 and the Rock 5B schematic, we > should actually use GPIO-based handling for the thermal runaways on > the Rock 5B. Other boards should also be investigated individually, > and the TSADC should be enabled on a board-to-board basis. With all due respect, I disagree, here is why: - Neither the schematic nor the hardware design guide, on which the schematic seems to be based, prescribes a particular way to handle thermal runaways. They only provide the possibility of GPIO based resets, along with the CRU based one - My strong belief is that defaults (regardless of context) should be safe and reasonable, and should also minimize the need to override them - In context of dts/dtsi, as far as I understand the general logic behind the split, the SoC .dtsi should contain all the things that are fully contained within the SoC and do not depend on the wiring of a particular board or its target use case. Boards then add/remove/override settings to match their wiring and use case more closely In the light of the last two points, I believe that enabling TSADC by default is the more safe and reasonable choice, because it provides crucial thermal protection logic for the SoC, and it can do so in a board-agnostic way (if the CRU based reset is selected, which is the current default). Furthermore, TSADC and CRU are fully contained within the SoC, and I cannot think of a use case where a board might be somehow disadvantaged by TSADC being enabled, and thus need to disable it altogether (maybe I'm missing something). The only thing that the board might be adjusting is the thermal reset handling, and even then it's rather a matter of choice/preference to switch away from CRU to GPIO resets where the wiring permits it, rather than an existential need. I presume that a PMIC-assisted reset causes deeper power cycling of the SoC and might therefore help in some rare cases where the CRU reset alone is not enough, but that would be niche. All summed up, I believe that the default of "fry my board if I have no heatsink and forget to include &tsadc {status = <okay>;}; in my .dts" is substantially inferior to the default of "my board could do a deep power-cycle in this weird corner-case thermal-runaway situation that somehow didn't get handled by active cooling, then by passive cooling, then by a CRU reset, but I didn't include rockchip,hw-tshut-mode = <1>; so poor luck for me". Would be great to hear other perspectives from people on the list. Best regards, Alexey > [4] > https://lore.kernel.org/linux-rockchip/4e7c2b5a938bd7c919b852699c951701@manjaro.org/ > > > It seems though that downstream kernels don't use that, even for > > those boards where the wiring allows for GPIO based tshut, such as > > Radxa Rock 5B [1], [2], [3] > > > > [1] > > https://github.com/radxa/kernel/blob/stable-5.10-rock5/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts#L540 > > [2] > > https://github.com/radxa/kernel/blob/stable-5.10-rock5/arch/arm64/boot/dts/rockchip/rk3588s.dtsi#L5433 > > [3] https://dl.radxa.com/rock5/5b/docs/hw/radxa_rock_5b_v1423_sch.pdf > > page 11 (TSADC_SHUT_H) > > > > Signed-off-by: Alexey Charkov <alchark@gmail.com> > > --- > > arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 176 > > +++++++++++++++++++++++++++++- > > 1 file changed, 175 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi > > b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi > > index 36b1b7acfe6a..9bf197358642 100644 > > --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi > > +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi > > @@ -10,6 +10,7 @@ > > #include <dt-bindings/reset/rockchip,rk3588-cru.h> > > #include <dt-bindings/phy/phy.h> > > #include <dt-bindings/ata/ahci.h> > > +#include <dt-bindings/thermal/thermal.h> > > > > / { > > compatible = "rockchip,rk3588"; > > @@ -2225,7 +2226,180 @@ tsadc: tsadc@fec00000 { > > pinctrl-1 = <&tsadc_shut>; > > pinctrl-names = "gpio", "otpout"; > > #thermal-sensor-cells = <1>; > > - status = "disabled"; > > + status = "okay"; > > + }; > > + > > + thermal_zones: thermal-zones { > > + /* sensor near the center of the SoC */ > > + package_thermal: package-thermal { > > + polling-delay-passive = <0>; > > + polling-delay = <0>; > > + thermal-sensors = <&tsadc 0>; > > + > > + trips { > > + package_crit: package-crit { > > + temperature = <115000>; > > + hysteresis = <0>; > > + type = "critical"; > > + }; > > + }; > > + }; > > + > > + /* sensor between A76 cores 0 and 1 */ > > + bigcore0_thermal: bigcore0-thermal { > > + polling-delay-passive = <100>; > > + polling-delay = <0>; > > + thermal-sensors = <&tsadc 1>; > > + > > + trips { > > + /* threshold to start collecting temperature > > + * statistics e.g. with the IPA governor > > + */ > > + bigcore0_alert0: bigcore0-alert0 { > > + temperature = <75000>; > > + hysteresis = <2000>; > > + type = "passive"; > > + }; > > + /* actual control temperature */ > > + bigcore0_alert1: bigcore0-alert1 { > > + temperature = <85000>; > > + hysteresis = <2000>; > > + type = "passive"; > > + }; > > + bigcore0_crit: bigcore0-crit { > > + temperature = <115000>; > > + hysteresis = <0>; > > + type = "critical"; > > + }; > > + }; > > + cooling-maps { > > + map0 { > > + trip = <&bigcore0_alert1>; > > + cooling-device = > > + <&cpu_b0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > > + <&cpu_b1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; > > + }; > > + }; > > + }; > > + > > + /* sensor between A76 cores 2 and 3 */ > > + bigcore2_thermal: bigcore2-thermal { > > + polling-delay-passive = <100>; > > + polling-delay = <0>; > > + thermal-sensors = <&tsadc 2>; > > + > > + trips { > > + /* threshold to start collecting temperature > > + * statistics e.g. with the IPA governor > > + */ > > + bigcore2_alert0: bigcore2-alert0 { > > + temperature = <75000>; > > + hysteresis = <2000>; > > + type = "passive"; > > + }; > > + /* actual control temperature */ > > + bigcore2_alert1: bigcore2-alert1 { > > + temperature = <85000>; > > + hysteresis = <2000>; > > + type = "passive"; > > + }; > > + bigcore2_crit: bigcore2-crit { > > + temperature = <115000>; > > + hysteresis = <0>; > > + type = "critical"; > > + }; > > + }; > > + cooling-maps { > > + map0 { > > + trip = <&bigcore2_alert1>; > > + cooling-device = > > + <&cpu_b2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > > + <&cpu_b3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; > > + }; > > + }; > > + }; > > + > > + /* sensor between the four A55 cores */ > > + little_core_thermal: littlecore-thermal { > > + polling-delay-passive = <100>; > > + polling-delay = <0>; > > + thermal-sensors = <&tsadc 3>; > > + > > + trips { > > + /* threshold to start collecting temperature > > + * statistics e.g. with the IPA governor > > + */ > > + littlecore_alert0: littlecore-alert0 { > > + temperature = <75000>; > > + hysteresis = <2000>; > > + type = "passive"; > > + }; > > + /* actual control temperature */ > > + littlecore_alert1: littlecore-alert1 { > > + temperature = <85000>; > > + hysteresis = <2000>; > > + type = "passive"; > > + }; > > + littlecore_crit: littlecore-crit { > > + temperature = <115000>; > > + hysteresis = <0>; > > + type = "critical"; > > + }; > > + }; > > + cooling-maps { > > + map0 { > > + trip = <&littlecore_alert1>; > > + cooling-device = > > + <&cpu_l0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > > + <&cpu_l1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > > + <&cpu_l2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > > + <&cpu_l3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; > > + }; > > + }; > > + }; > > + > > + /* sensor near the PD_CENTER power domain */ > > + center_thermal: center-thermal { > > + polling-delay-passive = <0>; > > + polling-delay = <0>; > > + thermal-sensors = <&tsadc 4>; > > + > > + trips { > > + center_crit: center-crit { > > + temperature = <115000>; > > + hysteresis = <0>; > > + type = "critical"; > > + }; > > + }; > > + }; > > + > > + gpu_thermal: gpu-thermal { > > + polling-delay-passive = <0>; > > + polling-delay = <0>; > > + thermal-sensors = <&tsadc 5>; > > + > > + trips { > > + gpu_crit: gpu-crit { > > + temperature = <115000>; > > + hysteresis = <0>; > > + type = "critical"; > > + }; > > + }; > > + }; > > + > > + npu_thermal: npu-thermal { > > + polling-delay-passive = <0>; > > + polling-delay = <0>; > > + thermal-sensors = <&tsadc 6>; > > + > > + trips { > > + npu_crit: npu-crit { > > + temperature = <115000>; > > + hysteresis = <0>; > > + type = "critical"; > > + }; > > + }; > > + }; > > }; > > > > saradc: adc@fec10000 {
On Fri, Mar 1, 2024 at 1:11 AM Dragan Simic <dsimic@manjaro.org> wrote: > > Hello Alexey, > > Please see also some nitpicks below, which I forgot to mention in > my earlier response. I'm sorry for that. > > On 2024-02-29 20:26, Alexey Charkov wrote: > > Include thermal zones information in device tree for RK3588 variants. > > > > This also enables the TSADC controller unconditionally on all boards > > to ensure that thermal protections are in place via throttling and > > emergency reset, once OPPs are added to enable CPU DVFS. > > > > The default settings (using CRU as the emergency reset mechanism) > > should work on all boards regardless of their wiring, as CRU resets > > do not depend on any external components. Boards that have the TSHUT > > signal wired to the reset line of the PMIC may opt to switch to GPIO > > tshut mode instead (rockchip,hw-tshut-mode = <1>;) > > > > It seems though that downstream kernels don't use that, even for > > those boards where the wiring allows for GPIO based tshut, such as > > Radxa Rock 5B [1], [2], [3] > > > > [1] > > https://github.com/radxa/kernel/blob/stable-5.10-rock5/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts#L540 > > [2] > > https://github.com/radxa/kernel/blob/stable-5.10-rock5/arch/arm64/boot/dts/rockchip/rk3588s.dtsi#L5433 > > [3] https://dl.radxa.com/rock5/5b/docs/hw/radxa_rock_5b_v1423_sch.pdf > > page 11 (TSADC_SHUT_H) > > > > Signed-off-by: Alexey Charkov <alchark@gmail.com> > > --- > > arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 176 > > +++++++++++++++++++++++++++++- > > 1 file changed, 175 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi > > b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi > > index 36b1b7acfe6a..9bf197358642 100644 > > --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi > > +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi > > @@ -10,6 +10,7 @@ > > #include <dt-bindings/reset/rockchip,rk3588-cru.h> > > #include <dt-bindings/phy/phy.h> > > #include <dt-bindings/ata/ahci.h> > > +#include <dt-bindings/thermal/thermal.h> > > > > / { > > compatible = "rockchip,rk3588"; > > @@ -2225,7 +2226,180 @@ tsadc: tsadc@fec00000 { > > pinctrl-1 = <&tsadc_shut>; > > pinctrl-names = "gpio", "otpout"; > > #thermal-sensor-cells = <1>; > > - status = "disabled"; > > + status = "okay"; > > + }; > > + > > + thermal_zones: thermal-zones { > > + /* sensor near the center of the SoC */ > > + package_thermal: package-thermal { > > + polling-delay-passive = <0>; > > + polling-delay = <0>; > > + thermal-sensors = <&tsadc 0>; > > + > > + trips { > > + package_crit: package-crit { > > + temperature = <115000>; > > + hysteresis = <0>; > > + type = "critical"; > > + }; > > + }; > > + }; > > + > > + /* sensor between A76 cores 0 and 1 */ > > + bigcore0_thermal: bigcore0-thermal { > > + polling-delay-passive = <100>; > > + polling-delay = <0>; > > + thermal-sensors = <&tsadc 1>; > > + > > + trips { > > + /* threshold to start collecting temperature > > + * statistics e.g. with the IPA governor > > + */ > > See, I'm not a native English speaker, but I've spent a lot of time > and effort improving my English skills. Thus, perhaps these comments > may or may not seem like unnecessary nitpicking, depending on how much > someone pays attention to writing style in general, but I'll risk to > be annoying and state these comments anyway. :) > > The comment above could be written in a much more condensed form like > this, which would also be a bit more accurate: > > > /* IPA threshold, when IPA governor is used */ > > IOW, we're writing all this for someone to read later, but we should > (and can) perfectly reasonably expect some already existing background > knowledge from the readers. In other words, we should be as concise > as possible. In fact, the power allocation governor code itself doesn't call those trips threshold or target as your suggested wording would imply. Instead, it calls them "switch on temperature" and "maximum desired temperature" [1]. Maybe we can call them that in the comments (and also avoid calling the governor IPA, because upstream code only calls it a "power allocator"). Best regards, Alexey [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thermal/gov_power_allocator.c#n483 > > + bigcore0_alert0: bigcore0-alert0 { > > + temperature = <75000>; > > + hysteresis = <2000>; > > + type = "passive"; > > + }; > > + /* actual control temperature */ > > Similarly to the above, I'd suggest this: > > /* IPA target, when IPA governor is used */ > > Having such brief comments should make it all perfectly understandable > to anyone who's already familiar with the way IPA governor works. > Everyone > else should be welcome to read up a bit on IPA first. > > > + bigcore0_alert1: bigcore0-alert1 { > > + temperature = <85000>; > > + hysteresis = <2000>; > > + type = "passive"; > > + }; > > + bigcore0_crit: bigcore0-crit { > > + temperature = <115000>; > > + hysteresis = <0>; > > + type = "critical"; > > + }; > > + }; > > + cooling-maps { > > + map0 { > > + trip = <&bigcore0_alert1>; > > + cooling-device = > > + <&cpu_b0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > > + <&cpu_b1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; > > + }; > > + }; > > + }; > > + > > + /* sensor between A76 cores 2 and 3 */ > > + bigcore2_thermal: bigcore2-thermal { > > + polling-delay-passive = <100>; > > + polling-delay = <0>; > > + thermal-sensors = <&tsadc 2>; > > + > > + trips { > > + /* threshold to start collecting temperature > > + * statistics e.g. with the IPA governor > > + */ > > The same as above. > > > + bigcore2_alert0: bigcore2-alert0 { > > + temperature = <75000>; > > + hysteresis = <2000>; > > + type = "passive"; > > + }; > > + /* actual control temperature */ > > The same as above. > > > + bigcore2_alert1: bigcore2-alert1 { > > + temperature = <85000>; > > + hysteresis = <2000>; > > + type = "passive"; > > + }; > > + bigcore2_crit: bigcore2-crit { > > + temperature = <115000>; > > + hysteresis = <0>; > > + type = "critical"; > > + }; > > + }; > > + cooling-maps { > > + map0 { > > + trip = <&bigcore2_alert1>; > > + cooling-device = > > + <&cpu_b2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > > + <&cpu_b3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; > > + }; > > + }; > > + }; > > + > > + /* sensor between the four A55 cores */ > > + little_core_thermal: littlecore-thermal { > > + polling-delay-passive = <100>; > > + polling-delay = <0>; > > + thermal-sensors = <&tsadc 3>; > > + > > + trips { > > + /* threshold to start collecting temperature > > + * statistics e.g. with the IPA governor > > + */ > > The same as above. > > > + littlecore_alert0: littlecore-alert0 { > > + temperature = <75000>; > > + hysteresis = <2000>; > > + type = "passive"; > > + }; > > + /* actual control temperature */ > > The same as above. > > > + littlecore_alert1: littlecore-alert1 { > > + temperature = <85000>; > > + hysteresis = <2000>; > > + type = "passive"; > > + }; > > + littlecore_crit: littlecore-crit { > > + temperature = <115000>; > > + hysteresis = <0>; > > + type = "critical"; > > + }; > > + }; > > + cooling-maps { > > + map0 { > > + trip = <&littlecore_alert1>; > > + cooling-device = > > + <&cpu_l0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > > + <&cpu_l1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > > + <&cpu_l2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > > + <&cpu_l3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; > > + }; > > + }; > > + }; > > + > > + /* sensor near the PD_CENTER power domain */ > > + center_thermal: center-thermal { > > + polling-delay-passive = <0>; > > + polling-delay = <0>; > > + thermal-sensors = <&tsadc 4>; > > + > > + trips { > > + center_crit: center-crit { > > + temperature = <115000>; > > + hysteresis = <0>; > > + type = "critical"; > > + }; > > + }; > > + }; > > + > > + gpu_thermal: gpu-thermal { > > + polling-delay-passive = <0>; > > + polling-delay = <0>; > > + thermal-sensors = <&tsadc 5>; > > + > > + trips { > > + gpu_crit: gpu-crit { > > + temperature = <115000>; > > + hysteresis = <0>; > > + type = "critical"; > > + }; > > + }; > > + }; > > + > > + npu_thermal: npu-thermal { > > + polling-delay-passive = <0>; > > + polling-delay = <0>; > > + thermal-sensors = <&tsadc 6>; > > + > > + trips { > > + npu_crit: npu-crit { > > + temperature = <115000>; > > + hysteresis = <0>; > > + type = "critical"; > > + }; > > + }; > > + }; > > }; > > > > saradc: adc@fec10000 {
On 2024-03-01 06:12, Alexey Charkov wrote: > On Fri, Mar 1, 2024 at 12:21 AM Dragan Simic <dsimic@manjaro.org> > wrote: >> On 2024-02-29 20:26, Alexey Charkov wrote: >> > Include thermal zones information in device tree for RK3588 variants. >> > >> > This also enables the TSADC controller unconditionally on all boards >> > to ensure that thermal protections are in place via throttling and >> > emergency reset, once OPPs are added to enable CPU DVFS. >> > >> > The default settings (using CRU as the emergency reset mechanism) >> > should work on all boards regardless of their wiring, as CRU resets >> > do not depend on any external components. Boards that have the TSHUT >> > signal wired to the reset line of the PMIC may opt to switch to GPIO >> > tshut mode instead (rockchip,hw-tshut-mode = <1>;) >> >> Quite frankly, I'm still not sure that enabling this on the SoC level >> is the way to go. As I already described in detail, [4] according to >> the RK3588 Hardware Design Guide v1.0 and the Rock 5B schematic, we >> should actually use GPIO-based handling for the thermal runaways on >> the Rock 5B. Other boards should also be investigated individually, >> and the TSADC should be enabled on a board-to-board basis. > > With all due respect, I disagree, here is why: > - Neither the schematic nor the hardware design guide, on which the > schematic seems to be based, prescribes a particular way to handle > thermal runaways. They only provide the possibility of GPIO based > resets, along with the CRU based one Please note that other documents from Rockchip also exist. Below is a link to a screenshot from the Thermal developer guide, version 1.0, which describes the whole thing further. I believe it's obvious that the thermal runaway is to be treated as a board-level feature. - https://i.imgur.com/IJ6dSAc.png To be fair, that version of the Thermal developer guide dates back to 2019, meaning that it technically applies to the RK3399, for example, but the TSADC and reset circuitry design has basically remained the same for the RK3588. > - My strong belief is that defaults (regardless of context) should be > safe and reasonable, and should also minimize the need to override > them Please note that the TSADC is disabled in the RK3399 SoC dtsi, so having it disabled in the RK3588(s) SoC dtsi would provide some consistency. Though, the RK3399 still does it in a safe way, by moving the OPPs into a separate dtsi file, named rk3399-opp.dtsi, which the board dts files then include together with enabling the TSADC. If you agree, let's employ the same approach for the RK3588(s), by having the its OPPs defined in a separate file, named rk3588s-opp.dtsi, etc. > - In context of dts/dtsi, as far as I understand the general logic > behind the split, the SoC .dtsi should contain all the things that are > fully contained within the SoC and do not depend on the wiring of a > particular board or its target use case. Boards then > add/remove/override settings to match their wiring and use case more > closely Of course, but the thermal shutdown is obviously a board-level feature, which I described further above. > In the light of the last two points, I believe that enabling TSADC by > default is the more safe and reasonable choice, because it provides > crucial thermal protection logic for the SoC, and it can do so in a > board-agnostic way (if the CRU based reset is selected, which is the > current default). > > Furthermore, TSADC and CRU are fully contained within the SoC, and I > cannot think of a use case where a board might be somehow > disadvantaged by TSADC being enabled, and thus need to disable it > altogether (maybe I'm missing something). The only thing that the > board might be adjusting is the thermal reset handling, and even then > it's rather a matter of choice/preference to switch away from CRU to > GPIO resets where the wiring permits it, rather than an existential > need. I presume that a PMIC-assisted reset causes deeper power cycling > of the SoC and might therefore help in some rare cases where the CRU > reset alone is not enough, but that would be niche. > > All summed up, I believe that the default of "fry my board if I have > no heatsink and forget to include &tsadc {status = <okay>;}; in my > .dts" is substantially inferior to the default of "my board could do a > deep power-cycle in this weird corner-case thermal-runaway situation > that somehow didn't get handled by active cooling, then by passive > cooling, then by a CRU reset, but I didn't include > rockchip,hw-tshut-mode = <1>; so poor luck for me". Please see my comments above, regarding the separate dtsi for the OPPs. I think it's a win-win, and I hope you'll agree. >> [4] >> https://lore.kernel.org/linux-rockchip/4e7c2b5a938bd7c919b852699c951701@manjaro.org/ >> >> > It seems though that downstream kernels don't use that, even for >> > those boards where the wiring allows for GPIO based tshut, such as >> > Radxa Rock 5B [1], [2], [3] >> > >> > [1] >> > https://github.com/radxa/kernel/blob/stable-5.10-rock5/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts#L540 >> > [2] >> > https://github.com/radxa/kernel/blob/stable-5.10-rock5/arch/arm64/boot/dts/rockchip/rk3588s.dtsi#L5433 >> > [3] https://dl.radxa.com/rock5/5b/docs/hw/radxa_rock_5b_v1423_sch.pdf >> > page 11 (TSADC_SHUT_H) >> > >> > Signed-off-by: Alexey Charkov <alchark@gmail.com> >> > --- >> > arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 176 >> > +++++++++++++++++++++++++++++- >> > 1 file changed, 175 insertions(+), 1 deletion(-) >> > >> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi >> > b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi >> > index 36b1b7acfe6a..9bf197358642 100644 >> > --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi >> > +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi >> > @@ -10,6 +10,7 @@ >> > #include <dt-bindings/reset/rockchip,rk3588-cru.h> >> > #include <dt-bindings/phy/phy.h> >> > #include <dt-bindings/ata/ahci.h> >> > +#include <dt-bindings/thermal/thermal.h> >> > >> > / { >> > compatible = "rockchip,rk3588"; >> > @@ -2225,7 +2226,180 @@ tsadc: tsadc@fec00000 { >> > pinctrl-1 = <&tsadc_shut>; >> > pinctrl-names = "gpio", "otpout"; >> > #thermal-sensor-cells = <1>; >> > - status = "disabled"; >> > + status = "okay"; >> > + }; >> > + >> > + thermal_zones: thermal-zones { >> > + /* sensor near the center of the SoC */ >> > + package_thermal: package-thermal { >> > + polling-delay-passive = <0>; >> > + polling-delay = <0>; >> > + thermal-sensors = <&tsadc 0>; >> > + >> > + trips { >> > + package_crit: package-crit { >> > + temperature = <115000>; >> > + hysteresis = <0>; >> > + type = "critical"; >> > + }; >> > + }; >> > + }; >> > + >> > + /* sensor between A76 cores 0 and 1 */ >> > + bigcore0_thermal: bigcore0-thermal { >> > + polling-delay-passive = <100>; >> > + polling-delay = <0>; >> > + thermal-sensors = <&tsadc 1>; >> > + >> > + trips { >> > + /* threshold to start collecting temperature >> > + * statistics e.g. with the IPA governor >> > + */ >> > + bigcore0_alert0: bigcore0-alert0 { >> > + temperature = <75000>; >> > + hysteresis = <2000>; >> > + type = "passive"; >> > + }; >> > + /* actual control temperature */ >> > + bigcore0_alert1: bigcore0-alert1 { >> > + temperature = <85000>; >> > + hysteresis = <2000>; >> > + type = "passive"; >> > + }; >> > + bigcore0_crit: bigcore0-crit { >> > + temperature = <115000>; >> > + hysteresis = <0>; >> > + type = "critical"; >> > + }; >> > + }; >> > + cooling-maps { >> > + map0 { >> > + trip = <&bigcore0_alert1>; >> > + cooling-device = >> > + <&cpu_b0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, >> > + <&cpu_b1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; >> > + }; >> > + }; >> > + }; >> > + >> > + /* sensor between A76 cores 2 and 3 */ >> > + bigcore2_thermal: bigcore2-thermal { >> > + polling-delay-passive = <100>; >> > + polling-delay = <0>; >> > + thermal-sensors = <&tsadc 2>; >> > + >> > + trips { >> > + /* threshold to start collecting temperature >> > + * statistics e.g. with the IPA governor >> > + */ >> > + bigcore2_alert0: bigcore2-alert0 { >> > + temperature = <75000>; >> > + hysteresis = <2000>; >> > + type = "passive"; >> > + }; >> > + /* actual control temperature */ >> > + bigcore2_alert1: bigcore2-alert1 { >> > + temperature = <85000>; >> > + hysteresis = <2000>; >> > + type = "passive"; >> > + }; >> > + bigcore2_crit: bigcore2-crit { >> > + temperature = <115000>; >> > + hysteresis = <0>; >> > + type = "critical"; >> > + }; >> > + }; >> > + cooling-maps { >> > + map0 { >> > + trip = <&bigcore2_alert1>; >> > + cooling-device = >> > + <&cpu_b2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, >> > + <&cpu_b3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; >> > + }; >> > + }; >> > + }; >> > + >> > + /* sensor between the four A55 cores */ >> > + little_core_thermal: littlecore-thermal { >> > + polling-delay-passive = <100>; >> > + polling-delay = <0>; >> > + thermal-sensors = <&tsadc 3>; >> > + >> > + trips { >> > + /* threshold to start collecting temperature >> > + * statistics e.g. with the IPA governor >> > + */ >> > + littlecore_alert0: littlecore-alert0 { >> > + temperature = <75000>; >> > + hysteresis = <2000>; >> > + type = "passive"; >> > + }; >> > + /* actual control temperature */ >> > + littlecore_alert1: littlecore-alert1 { >> > + temperature = <85000>; >> > + hysteresis = <2000>; >> > + type = "passive"; >> > + }; >> > + littlecore_crit: littlecore-crit { >> > + temperature = <115000>; >> > + hysteresis = <0>; >> > + type = "critical"; >> > + }; >> > + }; >> > + cooling-maps { >> > + map0 { >> > + trip = <&littlecore_alert1>; >> > + cooling-device = >> > + <&cpu_l0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, >> > + <&cpu_l1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, >> > + <&cpu_l2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, >> > + <&cpu_l3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; >> > + }; >> > + }; >> > + }; >> > + >> > + /* sensor near the PD_CENTER power domain */ >> > + center_thermal: center-thermal { >> > + polling-delay-passive = <0>; >> > + polling-delay = <0>; >> > + thermal-sensors = <&tsadc 4>; >> > + >> > + trips { >> > + center_crit: center-crit { >> > + temperature = <115000>; >> > + hysteresis = <0>; >> > + type = "critical"; >> > + }; >> > + }; >> > + }; >> > + >> > + gpu_thermal: gpu-thermal { >> > + polling-delay-passive = <0>; >> > + polling-delay = <0>; >> > + thermal-sensors = <&tsadc 5>; >> > + >> > + trips { >> > + gpu_crit: gpu-crit { >> > + temperature = <115000>; >> > + hysteresis = <0>; >> > + type = "critical"; >> > + }; >> > + }; >> > + }; >> > + >> > + npu_thermal: npu-thermal { >> > + polling-delay-passive = <0>; >> > + polling-delay = <0>; >> > + thermal-sensors = <&tsadc 6>; >> > + >> > + trips { >> > + npu_crit: npu-crit { >> > + temperature = <115000>; >> > + hysteresis = <0>; >> > + type = "critical"; >> > + }; >> > + }; >> > + }; >> > }; >> > >> > saradc: adc@fec10000 { > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
On 2024-03-01 06:20, Alexey Charkov wrote: > On Fri, Mar 1, 2024 at 1:11 AM Dragan Simic <dsimic@manjaro.org> wrote: >> Please see also some nitpicks below, which I forgot to mention in >> my earlier response. I'm sorry for that. >> >> On 2024-02-29 20:26, Alexey Charkov wrote: >> > Include thermal zones information in device tree for RK3588 variants. >> > >> > This also enables the TSADC controller unconditionally on all boards >> > to ensure that thermal protections are in place via throttling and >> > emergency reset, once OPPs are added to enable CPU DVFS. >> > >> > The default settings (using CRU as the emergency reset mechanism) >> > should work on all boards regardless of their wiring, as CRU resets >> > do not depend on any external components. Boards that have the TSHUT >> > signal wired to the reset line of the PMIC may opt to switch to GPIO >> > tshut mode instead (rockchip,hw-tshut-mode = <1>;) >> > >> > It seems though that downstream kernels don't use that, even for >> > those boards where the wiring allows for GPIO based tshut, such as >> > Radxa Rock 5B [1], [2], [3] >> > >> > [1] >> > https://github.com/radxa/kernel/blob/stable-5.10-rock5/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts#L540 >> > [2] >> > https://github.com/radxa/kernel/blob/stable-5.10-rock5/arch/arm64/boot/dts/rockchip/rk3588s.dtsi#L5433 >> > [3] https://dl.radxa.com/rock5/5b/docs/hw/radxa_rock_5b_v1423_sch.pdf >> > page 11 (TSADC_SHUT_H) >> > >> > Signed-off-by: Alexey Charkov <alchark@gmail.com> >> > --- >> > arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 176 >> > +++++++++++++++++++++++++++++- >> > 1 file changed, 175 insertions(+), 1 deletion(-) >> > >> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi >> > b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi >> > index 36b1b7acfe6a..9bf197358642 100644 >> > --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi >> > +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi >> > @@ -10,6 +10,7 @@ >> > #include <dt-bindings/reset/rockchip,rk3588-cru.h> >> > #include <dt-bindings/phy/phy.h> >> > #include <dt-bindings/ata/ahci.h> >> > +#include <dt-bindings/thermal/thermal.h> >> > >> > / { >> > compatible = "rockchip,rk3588"; >> > @@ -2225,7 +2226,180 @@ tsadc: tsadc@fec00000 { >> > pinctrl-1 = <&tsadc_shut>; >> > pinctrl-names = "gpio", "otpout"; >> > #thermal-sensor-cells = <1>; >> > - status = "disabled"; >> > + status = "okay"; >> > + }; >> > + >> > + thermal_zones: thermal-zones { >> > + /* sensor near the center of the SoC */ >> > + package_thermal: package-thermal { >> > + polling-delay-passive = <0>; >> > + polling-delay = <0>; >> > + thermal-sensors = <&tsadc 0>; >> > + >> > + trips { >> > + package_crit: package-crit { >> > + temperature = <115000>; >> > + hysteresis = <0>; >> > + type = "critical"; >> > + }; >> > + }; >> > + }; >> > + >> > + /* sensor between A76 cores 0 and 1 */ >> > + bigcore0_thermal: bigcore0-thermal { >> > + polling-delay-passive = <100>; >> > + polling-delay = <0>; >> > + thermal-sensors = <&tsadc 1>; >> > + >> > + trips { >> > + /* threshold to start collecting temperature >> > + * statistics e.g. with the IPA governor >> > + */ >> >> See, I'm not a native English speaker, but I've spent a lot of time >> and effort improving my English skills. Thus, perhaps these comments >> may or may not seem like unnecessary nitpicking, depending on how much >> someone pays attention to writing style in general, but I'll risk to >> be annoying and state these comments anyway. :) >> >> The comment above could be written in a much more condensed form like >> this, which would also be a bit more accurate: >> >> >> /* IPA threshold, when IPA governor is >> used */ >> >> IOW, we're writing all this for someone to read later, but we should >> (and can) perfectly reasonably expect some already existing background >> knowledge from the readers. In other words, we should be as concise >> as possible. > > In fact, the power allocation governor code itself doesn't call those > trips threshold or target as your suggested wording would imply. > Instead, it calls them "switch on temperature" and "maximum desired > temperature" [1]. Maybe we can call them that in the comments (and > also avoid calling the governor IPA, because upstream code only calls > it a "power allocator"). Hmm, but "IPA" is still mentioned in exactly three places in the files under drivers/thermal. I think that warrants the use of "IPA", which is also widely used pretty much everywhere. Perhaps a win-win would be to have only the very first of the comments like this, to introduce "IPA" as an acronym: /* Power allocator (IPA) thermal governor */ /* switch-on point, when IPA governor is used */ Next, "the target temperature" is mentioned more than a few times in drivers/thermal/gov_power_allocator.c, which I believe makes the use of "IPA target" perfectly valid. Actually, let's use "IPA target temperature", if you agree, to make it self descriptive. Finally, the threshold... Based on drivers/thermal/gov_power_allocator.c, I think "IPA switch-on point" would be a good choice, which I already used above in the proposed opening comment. > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thermal/gov_power_allocator.c#n483 > >> > + bigcore0_alert0: bigcore0-alert0 { >> > + temperature = <75000>; >> > + hysteresis = <2000>; >> > + type = "passive"; >> > + }; >> > + /* actual control temperature */ >> >> Similarly to the above, I'd suggest this: >> >> /* IPA target, when IPA governor is >> used */ >> >> Having such brief comments should make it all perfectly understandable >> to anyone who's already familiar with the way IPA governor works. >> Everyone else should be welcome to read up a bit on IPA first. >> >> > + bigcore0_alert1: bigcore0-alert1 { >> > + temperature = <85000>; >> > + hysteresis = <2000>; >> > + type = "passive"; >> > + }; >> > + bigcore0_crit: bigcore0-crit { >> > + temperature = <115000>; >> > + hysteresis = <0>; >> > + type = "critical"; >> > + }; >> > + }; >> > + cooling-maps { >> > + map0 { >> > + trip = <&bigcore0_alert1>; >> > + cooling-device = >> > + <&cpu_b0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, >> > + <&cpu_b1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; >> > + }; >> > + }; >> > + }; >> > + >> > + /* sensor between A76 cores 2 and 3 */ >> > + bigcore2_thermal: bigcore2-thermal { >> > + polling-delay-passive = <100>; >> > + polling-delay = <0>; >> > + thermal-sensors = <&tsadc 2>; >> > + >> > + trips { >> > + /* threshold to start collecting temperature >> > + * statistics e.g. with the IPA governor >> > + */ >> >> The same as above. >> >> > + bigcore2_alert0: bigcore2-alert0 { >> > + temperature = <75000>; >> > + hysteresis = <2000>; >> > + type = "passive"; >> > + }; >> > + /* actual control temperature */ >> >> The same as above. >> >> > + bigcore2_alert1: bigcore2-alert1 { >> > + temperature = <85000>; >> > + hysteresis = <2000>; >> > + type = "passive"; >> > + }; >> > + bigcore2_crit: bigcore2-crit { >> > + temperature = <115000>; >> > + hysteresis = <0>; >> > + type = "critical"; >> > + }; >> > + }; >> > + cooling-maps { >> > + map0 { >> > + trip = <&bigcore2_alert1>; >> > + cooling-device = >> > + <&cpu_b2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, >> > + <&cpu_b3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; >> > + }; >> > + }; >> > + }; >> > + >> > + /* sensor between the four A55 cores */ >> > + little_core_thermal: littlecore-thermal { >> > + polling-delay-passive = <100>; >> > + polling-delay = <0>; >> > + thermal-sensors = <&tsadc 3>; >> > + >> > + trips { >> > + /* threshold to start collecting temperature >> > + * statistics e.g. with the IPA governor >> > + */ >> >> The same as above. >> >> > + littlecore_alert0: littlecore-alert0 { >> > + temperature = <75000>; >> > + hysteresis = <2000>; >> > + type = "passive"; >> > + }; >> > + /* actual control temperature */ >> >> The same as above. >> >> > + littlecore_alert1: littlecore-alert1 { >> > + temperature = <85000>; >> > + hysteresis = <2000>; >> > + type = "passive"; >> > + }; >> > + littlecore_crit: littlecore-crit { >> > + temperature = <115000>; >> > + hysteresis = <0>; >> > + type = "critical"; >> > + }; >> > + }; >> > + cooling-maps { >> > + map0 { >> > + trip = <&littlecore_alert1>; >> > + cooling-device = >> > + <&cpu_l0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, >> > + <&cpu_l1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, >> > + <&cpu_l2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, >> > + <&cpu_l3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; >> > + }; >> > + }; >> > + }; >> > + >> > + /* sensor near the PD_CENTER power domain */ >> > + center_thermal: center-thermal { >> > + polling-delay-passive = <0>; >> > + polling-delay = <0>; >> > + thermal-sensors = <&tsadc 4>; >> > + >> > + trips { >> > + center_crit: center-crit { >> > + temperature = <115000>; >> > + hysteresis = <0>; >> > + type = "critical"; >> > + }; >> > + }; >> > + }; >> > + >> > + gpu_thermal: gpu-thermal { >> > + polling-delay-passive = <0>; >> > + polling-delay = <0>; >> > + thermal-sensors = <&tsadc 5>; >> > + >> > + trips { >> > + gpu_crit: gpu-crit { >> > + temperature = <115000>; >> > + hysteresis = <0>; >> > + type = "critical"; >> > + }; >> > + }; >> > + }; >> > + >> > + npu_thermal: npu-thermal { >> > + polling-delay-passive = <0>; >> > + polling-delay = <0>; >> > + thermal-sensors = <&tsadc 6>; >> > + >> > + trips { >> > + npu_crit: npu-crit { >> > + temperature = <115000>; >> > + hysteresis = <0>; >> > + type = "critical"; >> > + }; >> > + }; >> > + }; >> > }; >> > >> > saradc: adc@fec10000 {
On Fri, Mar 1, 2024 at 10:14 AM Dragan Simic <dsimic@manjaro.org> wrote: > > On 2024-03-01 06:20, Alexey Charkov wrote: > > On Fri, Mar 1, 2024 at 1:11 AM Dragan Simic <dsimic@manjaro.org> wrote: > >> Please see also some nitpicks below, which I forgot to mention in > >> my earlier response. I'm sorry for that. > >> > >> On 2024-02-29 20:26, Alexey Charkov wrote: > >> > Include thermal zones information in device tree for RK3588 variants. > >> > > >> > This also enables the TSADC controller unconditionally on all boards > >> > to ensure that thermal protections are in place via throttling and > >> > emergency reset, once OPPs are added to enable CPU DVFS. > >> > > >> > The default settings (using CRU as the emergency reset mechanism) > >> > should work on all boards regardless of their wiring, as CRU resets > >> > do not depend on any external components. Boards that have the TSHUT > >> > signal wired to the reset line of the PMIC may opt to switch to GPIO > >> > tshut mode instead (rockchip,hw-tshut-mode = <1>;) > >> > > >> > It seems though that downstream kernels don't use that, even for > >> > those boards where the wiring allows for GPIO based tshut, such as > >> > Radxa Rock 5B [1], [2], [3] > >> > > >> > [1] > >> > https://github.com/radxa/kernel/blob/stable-5.10-rock5/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts#L540 > >> > [2] > >> > https://github.com/radxa/kernel/blob/stable-5.10-rock5/arch/arm64/boot/dts/rockchip/rk3588s.dtsi#L5433 > >> > [3] https://dl.radxa.com/rock5/5b/docs/hw/radxa_rock_5b_v1423_sch.pdf > >> > page 11 (TSADC_SHUT_H) > >> > > >> > Signed-off-by: Alexey Charkov <alchark@gmail.com> > >> > --- > >> > arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 176 > >> > +++++++++++++++++++++++++++++- > >> > 1 file changed, 175 insertions(+), 1 deletion(-) > >> > > >> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi > >> > b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi > >> > index 36b1b7acfe6a..9bf197358642 100644 > >> > --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi > >> > +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi > >> > @@ -10,6 +10,7 @@ > >> > #include <dt-bindings/reset/rockchip,rk3588-cru.h> > >> > #include <dt-bindings/phy/phy.h> > >> > #include <dt-bindings/ata/ahci.h> > >> > +#include <dt-bindings/thermal/thermal.h> > >> > > >> > / { > >> > compatible = "rockchip,rk3588"; > >> > @@ -2225,7 +2226,180 @@ tsadc: tsadc@fec00000 { > >> > pinctrl-1 = <&tsadc_shut>; > >> > pinctrl-names = "gpio", "otpout"; > >> > #thermal-sensor-cells = <1>; > >> > - status = "disabled"; > >> > + status = "okay"; > >> > + }; > >> > + > >> > + thermal_zones: thermal-zones { > >> > + /* sensor near the center of the SoC */ > >> > + package_thermal: package-thermal { > >> > + polling-delay-passive = <0>; > >> > + polling-delay = <0>; > >> > + thermal-sensors = <&tsadc 0>; > >> > + > >> > + trips { > >> > + package_crit: package-crit { > >> > + temperature = <115000>; > >> > + hysteresis = <0>; > >> > + type = "critical"; > >> > + }; > >> > + }; > >> > + }; > >> > + > >> > + /* sensor between A76 cores 0 and 1 */ > >> > + bigcore0_thermal: bigcore0-thermal { > >> > + polling-delay-passive = <100>; > >> > + polling-delay = <0>; > >> > + thermal-sensors = <&tsadc 1>; > >> > + > >> > + trips { > >> > + /* threshold to start collecting temperature > >> > + * statistics e.g. with the IPA governor > >> > + */ > >> > >> See, I'm not a native English speaker, but I've spent a lot of time > >> and effort improving my English skills. Thus, perhaps these comments > >> may or may not seem like unnecessary nitpicking, depending on how much > >> someone pays attention to writing style in general, but I'll risk to > >> be annoying and state these comments anyway. :) > >> > >> The comment above could be written in a much more condensed form like > >> this, which would also be a bit more accurate: > >> > >> > >> /* IPA threshold, when IPA governor is > >> used */ > >> > >> IOW, we're writing all this for someone to read later, but we should > >> (and can) perfectly reasonably expect some already existing background > >> knowledge from the readers. In other words, we should be as concise > >> as possible. > > > > In fact, the power allocation governor code itself doesn't call those > > trips threshold or target as your suggested wording would imply. > > Instead, it calls them "switch on temperature" and "maximum desired > > temperature" [1]. Maybe we can call them that in the comments (and > > also avoid calling the governor IPA, because upstream code only calls > > it a "power allocator"). > > Hmm, but "IPA" is still mentioned in exactly three places in the files > under drivers/thermal. I think that warrants the use of "IPA", which > is also widely used pretty much everywhere. > > Perhaps a win-win would be to have only the very first of the comments > like this, to introduce "IPA" as an acronym: > > /* Power allocator (IPA) thermal > governor */ > /* switch-on point, when IPA governor > is used */ Yes, good point, thanks! > Next, "the target temperature" is mentioned more than a few times in > drivers/thermal/gov_power_allocator.c, which I believe makes the use > of "IPA target" perfectly valid. Actually, let's use "IPA target > temperature", if you agree, to make it self descriptive. Or perhaps simply "target temperature"? Stepwise governor will also use this trip as its target, so it's not IPA specific, unlike the switch-on point. > Finally, the threshold... Based on > drivers/thermal/gov_power_allocator.c, > I think "IPA switch-on point" would be a good choice, which I already > used above in the proposed opening comment. Agreed, that sounds good to me, will reflect in the next iteration. Thanks for bringing it up! Best, Alexey
On 2024-03-01 08:51, Alexey Charkov wrote: > On Fri, Mar 1, 2024 at 10:14 AM Dragan Simic <dsimic@manjaro.org> > wrote: >> On 2024-03-01 06:20, Alexey Charkov wrote: >> > On Fri, Mar 1, 2024 at 1:11 AM Dragan Simic <dsimic@manjaro.org> wrote: >> >> See, I'm not a native English speaker, but I've spent a lot of time >> >> and effort improving my English skills. Thus, perhaps these comments >> >> may or may not seem like unnecessary nitpicking, depending on how much >> >> someone pays attention to writing style in general, but I'll risk to >> >> be annoying and state these comments anyway. :) >> >> >> >> The comment above could be written in a much more condensed form like >> >> this, which would also be a bit more accurate: >> >> >> >> >> >> /* IPA threshold, when IPA governor is >> >> used */ >> >> >> >> IOW, we're writing all this for someone to read later, but we should >> >> (and can) perfectly reasonably expect some already existing background >> >> knowledge from the readers. In other words, we should be as concise >> >> as possible. >> > >> > In fact, the power allocation governor code itself doesn't call those >> > trips threshold or target as your suggested wording would imply. >> > Instead, it calls them "switch on temperature" and "maximum desired >> > temperature" [1]. Maybe we can call them that in the comments (and >> > also avoid calling the governor IPA, because upstream code only calls >> > it a "power allocator"). >> >> Hmm, but "IPA" is still mentioned in exactly three places in the files >> under drivers/thermal. I think that warrants the use of "IPA", which >> is also widely used pretty much everywhere. >> >> Perhaps a win-win would be to have only the very first of the comments >> like this, to introduce "IPA" as an acronym: >> >> /* Power allocator (IPA) thermal >> governor */ >> /* switch-on point, when IPA >> governor >> is used */ > > Yes, good point, thanks! I'm glad that you agree. :) >> Next, "the target temperature" is mentioned more than a few times in >> drivers/thermal/gov_power_allocator.c, which I believe makes the use >> of "IPA target" perfectly valid. Actually, let's use "IPA target >> temperature", if you agree, to make it self descriptive. > > Or perhaps simply "target temperature"? Stepwise governor will also > use this trip as its target, so it's not IPA specific, unlike the > switch-on point. I also had similar thoughts about the shared nature. I agree, just "/* target temperature */" would be fine. >> Finally, the threshold... Based on >> drivers/thermal/gov_power_allocator.c, >> I think "IPA switch-on point" would be a good choice, which I already >> used above in the proposed opening comment. > > Agreed, that sounds good to me, will reflect in the next iteration. > Thanks for bringing it up! Great, thanks!
On Fri, Mar 1, 2024 at 9:51 AM Dragan Simic <dsimic@manjaro.org> wrote: > > On 2024-03-01 06:12, Alexey Charkov wrote: > > On Fri, Mar 1, 2024 at 12:21 AM Dragan Simic <dsimic@manjaro.org> > > wrote: > >> On 2024-02-29 20:26, Alexey Charkov wrote: > >> > Include thermal zones information in device tree for RK3588 variants. > >> > > >> > This also enables the TSADC controller unconditionally on all boards > >> > to ensure that thermal protections are in place via throttling and > >> > emergency reset, once OPPs are added to enable CPU DVFS. > >> > > >> > The default settings (using CRU as the emergency reset mechanism) > >> > should work on all boards regardless of their wiring, as CRU resets > >> > do not depend on any external components. Boards that have the TSHUT > >> > signal wired to the reset line of the PMIC may opt to switch to GPIO > >> > tshut mode instead (rockchip,hw-tshut-mode = <1>;) > >> > >> Quite frankly, I'm still not sure that enabling this on the SoC level > >> is the way to go. As I already described in detail, [4] according to > >> the RK3588 Hardware Design Guide v1.0 and the Rock 5B schematic, we > >> should actually use GPIO-based handling for the thermal runaways on > >> the Rock 5B. Other boards should also be investigated individually, > >> and the TSADC should be enabled on a board-to-board basis. > > > > With all due respect, I disagree, here is why: > > - Neither the schematic nor the hardware design guide, on which the > > schematic seems to be based, prescribes a particular way to handle > > thermal runaways. They only provide the possibility of GPIO based > > resets, along with the CRU based one > > Please note that other documents from Rockchip also exist. Below is > a link to a screenshot from the Thermal developer guide, version 1.0, > which describes the whole thing further. I believe it's obvious that > the thermal runaway is to be treated as a board-level feature. > > - https://i.imgur.com/IJ6dSAc.png Frankly, that still doesn't make TSADC per se a board-level thing IMO. The only thing that is board-level is the wiring of GPIO based resets, which I fully agree should go to board .dts for boards that support it, but that's not part of the current defaults and can be safely added later. TSADC is inside the SoC. CRU is inside the SoC. They work just fine for a thermal reset, even if no dedicated reset logic is wired on the board. I really don't see any downsides in having TSADC enabled by default with CRU based resets: - it's a safe default (i.e. I cannot think of any configuration or use case where enabled-by-default TSADC does any harm) - it's safer than accidentally forgetting to enable TSADC (as it adds thermal protection which is otherwise missing) - it will work on all boards (even if it doesn't utilize the full hardware functionality by ignoring GPIO resets that some boards also have in addition to the CRU) - and it requires fewer overrides in board .dts files Sounds like a no-regret move to me. > To be fair, that version of the Thermal developer guide dates back to > 2019, meaning that it technically applies to the RK3399, for example, > but the TSADC and reset circuitry design has basically remained the > same for the RK3588. > > > - My strong belief is that defaults (regardless of context) should be > > safe and reasonable, and should also minimize the need to override > > them > > Please note that the TSADC is disabled in the RK3399 SoC dtsi, so having > it disabled in the RK3588(s) SoC dtsi would provide some consistency. I'm happy to produce a patch to reverse the logic in RK3399 (and any others for that matter) to also have TSADC enabled by default there, thus saving several lines of code, if it's just about consistency. > Though, the RK3399 still does it in a safe way, by moving the OPPs into > a separate dtsi file, named rk3399-opp.dtsi, which the board dts files > then include together with enabling the TSADC. > > If you agree, let's employ the same approach for the RK3588(s), by > having > the its OPPs defined in a separate file, named rk3588s-opp.dtsi, etc. Separate file for OPPs is a good no-regret move to declutter the SoC level .dtsi (as the OPP table is long and boring) - happy to move it regardless of the outcome of the above TSADC discussion. Thanks for the pointer! > > - In context of dts/dtsi, as far as I understand the general logic > > behind the split, the SoC .dtsi should contain all the things that are > > fully contained within the SoC and do not depend on the wiring of a > > particular board or its target use case. Boards then > > add/remove/override settings to match their wiring and use case more > > closely > > Of course, but the thermal shutdown is obviously a board-level feature, > which I described further above. Not so obvious to me :-) I don't mean to be stubborn or uncooperative here, but I really can't find any technical merit in having it enabled at board level instead of SoC level. Switching to PMIC-assisted resets is one thing - it definitely should go to board files, as it depends on the specific wiring of the TSADC_SHUT signal. Enabling TSADC in a default configuration that can and will work on all boards regardless of their wiring is another thing. I'm just arguing for the latter. To me it seems similar to the watchdog timer situation: we enable it at the SoC level [1], as it is expected to work in its default configuration regardless of the board wiring, and it provides protection against system malfunctions. Doesn't matter if the board or its userspace code ends up using the full functionality - it just sits there waiting for its spotlight without hurting anybody. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3588s.dtsi#n1872 Best regards, Alexey
On 2024-03-01 09:25, Alexey Charkov wrote: > On Fri, Mar 1, 2024 at 9:51 AM Dragan Simic <dsimic@manjaro.org> wrote: >> On 2024-03-01 06:12, Alexey Charkov wrote: >> > On Fri, Mar 1, 2024 at 12:21 AM Dragan Simic <dsimic@manjaro.org> >> > wrote: >> >> On 2024-02-29 20:26, Alexey Charkov wrote: >> >> > Include thermal zones information in device tree for RK3588 variants. >> >> > >> >> > This also enables the TSADC controller unconditionally on all boards >> >> > to ensure that thermal protections are in place via throttling and >> >> > emergency reset, once OPPs are added to enable CPU DVFS. >> >> > >> >> > The default settings (using CRU as the emergency reset mechanism) >> >> > should work on all boards regardless of their wiring, as CRU resets >> >> > do not depend on any external components. Boards that have the TSHUT >> >> > signal wired to the reset line of the PMIC may opt to switch to GPIO >> >> > tshut mode instead (rockchip,hw-tshut-mode = <1>;) >> >> >> >> Quite frankly, I'm still not sure that enabling this on the SoC level >> >> is the way to go. As I already described in detail, [4] according to >> >> the RK3588 Hardware Design Guide v1.0 and the Rock 5B schematic, we >> >> should actually use GPIO-based handling for the thermal runaways on >> >> the Rock 5B. Other boards should also be investigated individually, >> >> and the TSADC should be enabled on a board-to-board basis. >> > >> > With all due respect, I disagree, here is why: >> > - Neither the schematic nor the hardware design guide, on which the >> > schematic seems to be based, prescribes a particular way to handle >> > thermal runaways. They only provide the possibility of GPIO based >> > resets, along with the CRU based one >> >> Please note that other documents from Rockchip also exist. Below is >> a link to a screenshot from the Thermal developer guide, version 1.0, >> which describes the whole thing further. I believe it's obvious that >> the thermal runaway is to be treated as a board-level feature. >> >> - https://i.imgur.com/IJ6dSAc.png > > Frankly, that still doesn't make TSADC per se a board-level thing IMO. > The only thing that is board-level is the wiring of GPIO based resets, > which I fully agree should go to board .dts for boards that support > it, but that's not part of the current defaults and can be safely > added later. > > TSADC is inside the SoC. CRU is inside the SoC. They work just fine > for a thermal reset, even if no dedicated reset logic is wired on the > board. I really don't see any downsides in having TSADC enabled by > default with CRU based resets: > - it's a safe default (i.e. I cannot think of any configuration or use > case where enabled-by-default TSADC does any harm) > - it's safer than accidentally forgetting to enable TSADC (as it adds > thermal protection which is otherwise missing) > - it will work on all boards (even if it doesn't utilize the full > hardware functionality by ignoring GPIO resets that some boards also > have in addition to the CRU) > - and it requires fewer overrides in board .dts files > > Sounds like a no-regret move to me. Please see my comments below. >> To be fair, that version of the Thermal developer guide dates back to >> 2019, meaning that it technically applies to the RK3399, for example, >> but the TSADC and reset circuitry design has basically remained the >> same for the RK3588. >> >> > - My strong belief is that defaults (regardless of context) should be >> > safe and reasonable, and should also minimize the need to override >> > them >> >> Please note that the TSADC is disabled in the RK3399 SoC dtsi, so >> having >> it disabled in the RK3588(s) SoC dtsi would provide some consistency. > > I'm happy to produce a patch to reverse the logic in RK3399 (and any > others for that matter) to also have TSADC enabled by default there, > thus saving several lines of code, if it's just about consistency. But why should we change something that has served us for years, on multiple SoCs, with zero troubles and with (AFAIK) zero boards producing puffs of bluish smoke? >> Though, the RK3399 still does it in a safe way, by moving the OPPs >> into >> a separate dtsi file, named rk3399-opp.dtsi, which the board dts files >> then include together with enabling the TSADC. >> >> If you agree, let's employ the same approach for the RK3588(s), by >> having >> the its OPPs defined in a separate file, named rk3588s-opp.dtsi, etc. > > Separate file for OPPs is a good no-regret move to declutter the SoC > level .dtsi (as the OPP table is long and boring) - happy to move it > regardless of the outcome of the above TSADC discussion. Thanks for > the pointer! Yeah, but I'm not sure that everyone would like that kind of separation. In fact, such separation may be frowned upon unless it's necessary. As I already described in another thread, the separation for the RK3399 is there only because a couple of different variants of the RK3399 SoC require different OPPs. >> > - In context of dts/dtsi, as far as I understand the general logic >> > behind the split, the SoC .dtsi should contain all the things that are >> > fully contained within the SoC and do not depend on the wiring of a >> > particular board or its target use case. Boards then >> > add/remove/override settings to match their wiring and use case more >> > closely >> >> Of course, but the thermal shutdown is obviously a board-level >> feature, >> which I described further above. > > Not so obvious to me :-) I don't mean to be stubborn or uncooperative > here, but I really can't find any technical merit in having it enabled > at board level instead of SoC level. Well, please also consider that the PMICs from Rockchip are kind of weird little chips, specifically customized to serve particular SoCs. For example, they ensure the right sequencing and ramping-up of different power rails, which is in many cases essential. Thus, who knows what might (or might not) go wrong if we don't reset the PMIC at the same time when the CRU resets the SoC? Unfortunately, the things aren't that straightforward. On top of that, some boards, such as the Rock 5B, use a few additional discrete voltage regulators instead of a master-slave PMIC configuration, which may actually introduce some weird power-related issues, which also may be intermittent. Actually, I've already overheard that the Rock 5B experiences some issues of that nature, but I don't know the details. > Switching to PMIC-assisted resets is one thing - it definitely should > go to board files, as it depends on the specific wiring of the > TSADC_SHUT signal. Enabling TSADC in a default configuration that can > and will work on all boards regardless of their wiring is another > thing. I'm just arguing for the latter. CRU-based thermal runaway handling may in theory work on all boards, but we simply can't be 100% sure without detailed insights into the board designs and testing. Maybe even the downstream U-Boot does some magic during such thermal runaway resets, which we don't know. It may be similar to the SoC reset issues that the RK3399 suffers from. See also my comment above. > To me it seems similar to the watchdog timer situation: we enable it > at the SoC level [1], as it is expected to work in its default > configuration regardless of the board wiring, and it provides > protection against system malfunctions. Doesn't matter if the board or > its userspace code ends up using the full functionality - it just sits > there waiting for its spotlight without hurting anybody. Frankly, I don't know much about the watchdog functionality, so I'd need to research it before I could say something about it. > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3588s.dtsi#n1872
On 2024-03-01 09:52, Dragan Simic wrote: > On 2024-03-01 09:25, Alexey Charkov wrote: >> On Fri, Mar 1, 2024 at 9:51 AM Dragan Simic <dsimic@manjaro.org> >> wrote: >>> On 2024-03-01 06:12, Alexey Charkov wrote: >>> > With all due respect, I disagree, here is why: >>> > - Neither the schematic nor the hardware design guide, on which the >>> > schematic seems to be based, prescribes a particular way to handle >>> > thermal runaways. They only provide the possibility of GPIO based >>> > resets, along with the CRU based one >>> >>> Please note that other documents from Rockchip also exist. Below is >>> a link to a screenshot from the Thermal developer guide, version 1.0, >>> which describes the whole thing further. I believe it's obvious that >>> the thermal runaway is to be treated as a board-level feature. >>> >>> - https://i.imgur.com/IJ6dSAc.png >> >> Frankly, that still doesn't make TSADC per se a board-level thing IMO. >> The only thing that is board-level is the wiring of GPIO based resets, >> which I fully agree should go to board .dts for boards that support >> it, but that's not part of the current defaults and can be safely >> added later. >> >> TSADC is inside the SoC. CRU is inside the SoC. They work just fine >> for a thermal reset, even if no dedicated reset logic is wired on the >> board. I really don't see any downsides in having TSADC enabled by >> default with CRU based resets: >> - it's a safe default (i.e. I cannot think of any configuration or use >> case where enabled-by-default TSADC does any harm) >> - it's safer than accidentally forgetting to enable TSADC (as it adds >> thermal protection which is otherwise missing) >> - it will work on all boards (even if it doesn't utilize the full >> hardware functionality by ignoring GPIO resets that some boards also >> have in addition to the CRU) >> - and it requires fewer overrides in board .dts files >> >> Sounds like a no-regret move to me. > > Please see my comments below. > >>> To be fair, that version of the Thermal developer guide dates back to >>> 2019, meaning that it technically applies to the RK3399, for example, >>> but the TSADC and reset circuitry design has basically remained the >>> same for the RK3588. >>> >>> > - My strong belief is that defaults (regardless of context) should be >>> > safe and reasonable, and should also minimize the need to override >>> > them >>> >>> Please note that the TSADC is disabled in the RK3399 SoC dtsi, so >>> having >>> it disabled in the RK3588(s) SoC dtsi would provide some consistency. >> >> I'm happy to produce a patch to reverse the logic in RK3399 (and any >> others for that matter) to also have TSADC enabled by default there, >> thus saving several lines of code, if it's just about consistency. > > But why should we change something that has served us for years, on > multiple SoCs, with zero troubles and with (AFAIK) zero boards > producing > puffs of bluish smoke? > >>> Though, the RK3399 still does it in a safe way, by moving the OPPs >>> into >>> a separate dtsi file, named rk3399-opp.dtsi, which the board dts >>> files >>> then include together with enabling the TSADC. >>> >>> If you agree, let's employ the same approach for the RK3588(s), by >>> having >>> the its OPPs defined in a separate file, named rk3588s-opp.dtsi, etc. >> >> Separate file for OPPs is a good no-regret move to declutter the SoC >> level .dtsi (as the OPP table is long and boring) - happy to move it >> regardless of the outcome of the above TSADC discussion. Thanks for >> the pointer! > > Yeah, but I'm not sure that everyone would like that kind of > separation. > In fact, such separation may be frowned upon unless it's necessary. > > As I already described in another thread, the separation for the RK3399 > is there only because a couple of different variants of the RK3399 SoC > require different OPPs. > >>> > - In context of dts/dtsi, as far as I understand the general logic >>> > behind the split, the SoC .dtsi should contain all the things that are >>> > fully contained within the SoC and do not depend on the wiring of a >>> > particular board or its target use case. Boards then >>> > add/remove/override settings to match their wiring and use case more >>> > closely >>> >>> Of course, but the thermal shutdown is obviously a board-level >>> feature, >>> which I described further above. >> >> Not so obvious to me :-) I don't mean to be stubborn or uncooperative >> here, but I really can't find any technical merit in having it enabled >> at board level instead of SoC level. > > Well, please also consider that the PMICs from Rockchip are kind of > weird little chips, specifically customized to serve particular SoCs. > For example, they ensure the right sequencing and ramping-up of > different > power rails, which is in many cases essential. > > Thus, who knows what might (or might not) go wrong if we don't reset > the > PMIC at the same time when the CRU resets the SoC? Unfortunately, the > things aren't that straightforward. > > On top of that, some boards, such as the Rock 5B, use a few additional > discrete voltage regulators instead of a master-slave PMIC > configuration, > which may actually introduce some weird power-related issues, which > also > may be intermittent. Actually, I've already overheard that the Rock 5B > experiences some issues of that nature, but I don't know the details. As an example, did you know that LPDDR4 chips, according to the official JEDEC documentation, require proper sequencing of the ramping-down of their power rails when they're to be turned off as part of shutting the system down? The documentation also specifies that the expected lifetime becomes reduced when the powering-off isn't properly performed, and there's even an official number of such unsafe power-offs that the LPDDR4 chips are actually expected to survive. Thus, just yanking a power cord from a device that uses LPDDR4 may actually make it die prematurely. Such behavior is kind of exected when it comes to flash-based storage, but DRAM? Things are weird these days. :)
On Fri, Mar 1, 2024 at 12:52 PM Dragan Simic <dsimic@manjaro.org> wrote: > > On 2024-03-01 09:25, Alexey Charkov wrote: > > On Fri, Mar 1, 2024 at 9:51 AM Dragan Simic <dsimic@manjaro.org> wrote: > >> On 2024-03-01 06:12, Alexey Charkov wrote: > >> > On Fri, Mar 1, 2024 at 12:21 AM Dragan Simic <dsimic@manjaro.org> > >> > wrote: > >> >> On 2024-02-29 20:26, Alexey Charkov wrote: > >> >> > Include thermal zones information in device tree for RK3588 variants. > >> >> > > >> >> > This also enables the TSADC controller unconditionally on all boards > >> >> > to ensure that thermal protections are in place via throttling and > >> >> > emergency reset, once OPPs are added to enable CPU DVFS. > >> >> > > >> >> > The default settings (using CRU as the emergency reset mechanism) > >> >> > should work on all boards regardless of their wiring, as CRU resets > >> >> > do not depend on any external components. Boards that have the TSHUT > >> >> > signal wired to the reset line of the PMIC may opt to switch to GPIO > >> >> > tshut mode instead (rockchip,hw-tshut-mode = <1>;) > >> >> > >> >> Quite frankly, I'm still not sure that enabling this on the SoC level > >> >> is the way to go. As I already described in detail, [4] according to > >> >> the RK3588 Hardware Design Guide v1.0 and the Rock 5B schematic, we > >> >> should actually use GPIO-based handling for the thermal runaways on > >> >> the Rock 5B. Other boards should also be investigated individually, > >> >> and the TSADC should be enabled on a board-to-board basis. > >> > > >> > With all due respect, I disagree, here is why: > >> > - Neither the schematic nor the hardware design guide, on which the > >> > schematic seems to be based, prescribes a particular way to handle > >> > thermal runaways. They only provide the possibility of GPIO based > >> > resets, along with the CRU based one > >> > >> Please note that other documents from Rockchip also exist. Below is > >> a link to a screenshot from the Thermal developer guide, version 1.0, > >> which describes the whole thing further. I believe it's obvious that > >> the thermal runaway is to be treated as a board-level feature. > >> > >> - https://i.imgur.com/IJ6dSAc.png > > > > Frankly, that still doesn't make TSADC per se a board-level thing IMO. > > The only thing that is board-level is the wiring of GPIO based resets, > > which I fully agree should go to board .dts for boards that support > > it, but that's not part of the current defaults and can be safely > > added later. > > > > TSADC is inside the SoC. CRU is inside the SoC. They work just fine > > for a thermal reset, even if no dedicated reset logic is wired on the > > board. I really don't see any downsides in having TSADC enabled by > > default with CRU based resets: > > - it's a safe default (i.e. I cannot think of any configuration or use > > case where enabled-by-default TSADC does any harm) > > - it's safer than accidentally forgetting to enable TSADC (as it adds > > thermal protection which is otherwise missing) > > - it will work on all boards (even if it doesn't utilize the full > > hardware functionality by ignoring GPIO resets that some boards also > > have in addition to the CRU) > > - and it requires fewer overrides in board .dts files > > > > Sounds like a no-regret move to me. > > Please see my comments below. > > >> To be fair, that version of the Thermal developer guide dates back to > >> 2019, meaning that it technically applies to the RK3399, for example, > >> but the TSADC and reset circuitry design has basically remained the > >> same for the RK3588. > >> > >> > - My strong belief is that defaults (regardless of context) should be > >> > safe and reasonable, and should also minimize the need to override > >> > them > >> > >> Please note that the TSADC is disabled in the RK3399 SoC dtsi, so > >> having > >> it disabled in the RK3588(s) SoC dtsi would provide some consistency. > > > > I'm happy to produce a patch to reverse the logic in RK3399 (and any > > others for that matter) to also have TSADC enabled by default there, > > thus saving several lines of code, if it's just about consistency. > > But why should we change something that has served us for years, on > multiple SoCs, with zero troubles and with (AFAIK) zero boards producing > puffs of bluish smoke? That's just if we are concerned about consistency across different SoC series. The point is that I'm happy to make whatever change we agree upon in a consistent way across all related .dtsi/.dts files - thus no need to worry about past decisions that have already been implemented for other chips. Let's just agree on the technical merits of one or the other approach, leaving "we've been doing it differently elsewhere" aside for now. > >> Though, the RK3399 still does it in a safe way, by moving the OPPs > >> into > >> a separate dtsi file, named rk3399-opp.dtsi, which the board dts files > >> then include together with enabling the TSADC. > >> > >> If you agree, let's employ the same approach for the RK3588(s), by > >> having > >> the its OPPs defined in a separate file, named rk3588s-opp.dtsi, etc. > > > > Separate file for OPPs is a good no-regret move to declutter the SoC > > level .dtsi (as the OPP table is long and boring) - happy to move it > > regardless of the outcome of the above TSADC discussion. Thanks for > > the pointer! > > Yeah, but I'm not sure that everyone would like that kind of separation. > In fact, such separation may be frowned upon unless it's necessary. > > As I already described in another thread, the separation for the RK3399 > is there only because a couple of different variants of the RK3399 SoC > require different OPPs. > > >> > - In context of dts/dtsi, as far as I understand the general logic > >> > behind the split, the SoC .dtsi should contain all the things that are > >> > fully contained within the SoC and do not depend on the wiring of a > >> > particular board or its target use case. Boards then > >> > add/remove/override settings to match their wiring and use case more > >> > closely > >> > >> Of course, but the thermal shutdown is obviously a board-level > >> feature, > >> which I described further above. > > > > Not so obvious to me :-) I don't mean to be stubborn or uncooperative > > here, but I really can't find any technical merit in having it enabled > > at board level instead of SoC level. > > Well, please also consider that the PMICs from Rockchip are kind of > weird little chips, specifically customized to serve particular SoCs. > For example, they ensure the right sequencing and ramping-up of > different > power rails, which is in many cases essential. Sure. I'm not saying that switching to a PMIC-assisted reset shouldn't be done where the board supports it - quite the opposite. All I'm saying is that having at least passive cooling and CRU based resets guaranteed for any board, regardless of how thought through its .dts is, seems to be a better default than no thermal protection. > Thus, who knows what might (or might not) go wrong if we don't reset the > PMIC at the same time when the CRU resets the SoC? Unfortunately, the > things aren't that straightforward. > > On top of that, some boards, such as the Rock 5B, use a few additional > discrete voltage regulators instead of a master-slave PMIC > configuration, > which may actually introduce some weird power-related issues, which also > may be intermittent. Actually, I've already overheard that the Rock 5B > experiences some issues of that nature, but I don't know the details. Those discrete regulators seem to be out of scope of this discussion. I agree that a deeper power-cycle with proper power-up sequence to follow it is better when it's available in the respective hardware. I'm also happy to provide a follow-up patch to switch from CRU to PMIC resets for the boards I found to support the latter. The question we have at hand is solely about the default behavior for a hypothetical new board with minimal .dts, or an existing board where we can't determine the wiring of the TSHUT signal: Option 1. Let them stay nice and warm at 120C+ under load, because they should have known better and should have enabled the TSADC in their device tree before putting the system under load Option 2. Get them passively cooled at 85C under load even with no heatsink, then force a CRU reset out of abundance of caution at 120C unless they defined PMIC reset in their device tree I'm advocating for the latter. > > Switching to PMIC-assisted resets is one thing - it definitely should > > go to board files, as it depends on the specific wiring of the > > TSADC_SHUT signal. Enabling TSADC in a default configuration that can > > and will work on all boards regardless of their wiring is another > > thing. I'm just arguing for the latter. > > CRU-based thermal runaway handling may in theory work on all boards, but > we simply can't be 100% sure without detailed insights into the board > designs and testing. Maybe even the downstream U-Boot does some magic > during such thermal runaway resets, which we don't know. It may be > similar to the SoC reset issues that the RK3399 suffers from. That might be true, but we're talking about operation at 120C+ here. I'd rather have my board reboot in any way it pleases under those conditions, and have that behavior triggered by default even if it's imperfect, then worry about the correct state of all regulators and peripherals upon next boot. The latter is important of course, but I'd rather let it cool down and reboot it manually anyway, because that heat could have made more things go sideways than just the regulators. > See also my comment above. > > > To me it seems similar to the watchdog timer situation: we enable it > > at the SoC level [1], as it is expected to work in its default > > configuration regardless of the board wiring, and it provides > > protection against system malfunctions. Doesn't matter if the board or > > its userspace code ends up using the full functionality - it just sits > > there waiting for its spotlight without hurting anybody. > > Frankly, I don't know much about the watchdog functionality, so I'd need > to research it before I could say something about it. FWIW, watchdog resets are exclusively routed through the CRU (see RK3588 TRM V1.0 part 1 page 31). So if we expect that one to work somehow, probably we should expect thermal resets to work too. Best regards, Alexey
On Fri, Mar 1, 2024 at 7:10 PM Alexey Charkov <alchark@gmail.com> wrote: > > On Fri, Mar 1, 2024 at 12:52 PM Dragan Simic <dsimic@manjaro.org> wrote: > > > > On 2024-03-01 09:25, Alexey Charkov wrote: > > > On Fri, Mar 1, 2024 at 9:51 AM Dragan Simic <dsimic@manjaro.org> wrote: > > >> On 2024-03-01 06:12, Alexey Charkov wrote: > > >> > On Fri, Mar 1, 2024 at 12:21 AM Dragan Simic <dsimic@manjaro.org> > > >> > wrote: > > >> >> On 2024-02-29 20:26, Alexey Charkov wrote: > > >> >> > Include thermal zones information in device tree for RK3588 variants. > > >> >> > > > >> >> > This also enables the TSADC controller unconditionally on all boards > > >> >> > to ensure that thermal protections are in place via throttling and > > >> >> > emergency reset, once OPPs are added to enable CPU DVFS. > > >> >> > > > >> >> > The default settings (using CRU as the emergency reset mechanism) > > >> >> > should work on all boards regardless of their wiring, as CRU resets > > >> >> > do not depend on any external components. Boards that have the TSHUT > > >> >> > signal wired to the reset line of the PMIC may opt to switch to GPIO > > >> >> > tshut mode instead (rockchip,hw-tshut-mode = <1>;) > > >> >> > > >> >> Quite frankly, I'm still not sure that enabling this on the SoC level > > >> >> is the way to go. As I already described in detail, [4] according to > > >> >> the RK3588 Hardware Design Guide v1.0 and the Rock 5B schematic, we > > >> >> should actually use GPIO-based handling for the thermal runaways on > > >> >> the Rock 5B. Other boards should also be investigated individually, > > >> >> and the TSADC should be enabled on a board-to-board basis. > > >> > > > >> > With all due respect, I disagree, here is why: > > >> > - Neither the schematic nor the hardware design guide, on which the > > >> > schematic seems to be based, prescribes a particular way to handle > > >> > thermal runaways. They only provide the possibility of GPIO based > > >> > resets, along with the CRU based one > > >> > > >> Please note that other documents from Rockchip also exist. Below is > > >> a link to a screenshot from the Thermal developer guide, version 1.0, > > >> which describes the whole thing further. I believe it's obvious that > > >> the thermal runaway is to be treated as a board-level feature. > > >> > > >> - https://i.imgur.com/IJ6dSAc.png > > > > > > Frankly, that still doesn't make TSADC per se a board-level thing IMO. > > > The only thing that is board-level is the wiring of GPIO based resets, > > > which I fully agree should go to board .dts for boards that support > > > it, but that's not part of the current defaults and can be safely > > > added later. > > > > > > TSADC is inside the SoC. CRU is inside the SoC. They work just fine > > > for a thermal reset, even if no dedicated reset logic is wired on the > > > board. I really don't see any downsides in having TSADC enabled by > > > default with CRU based resets: > > > - it's a safe default (i.e. I cannot think of any configuration or use > > > case where enabled-by-default TSADC does any harm) > > > - it's safer than accidentally forgetting to enable TSADC (as it adds > > > thermal protection which is otherwise missing) > > > - it will work on all boards (even if it doesn't utilize the full > > > hardware functionality by ignoring GPIO resets that some boards also > > > have in addition to the CRU) > > > - and it requires fewer overrides in board .dts files > > > > > > Sounds like a no-regret move to me. > > > > Please see my comments below. > > > > >> To be fair, that version of the Thermal developer guide dates back to > > >> 2019, meaning that it technically applies to the RK3399, for example, > > >> but the TSADC and reset circuitry design has basically remained the > > >> same for the RK3588. > > >> > > >> > - My strong belief is that defaults (regardless of context) should be > > >> > safe and reasonable, and should also minimize the need to override > > >> > them > > >> > > >> Please note that the TSADC is disabled in the RK3399 SoC dtsi, so > > >> having > > >> it disabled in the RK3588(s) SoC dtsi would provide some consistency. > > > > > > I'm happy to produce a patch to reverse the logic in RK3399 (and any > > > others for that matter) to also have TSADC enabled by default there, > > > thus saving several lines of code, if it's just about consistency. > > > > But why should we change something that has served us for years, on > > multiple SoCs, with zero troubles and with (AFAIK) zero boards producing > > puffs of bluish smoke? > > That's just if we are concerned about consistency across different SoC > series. The point is that I'm happy to make whatever change we agree > upon in a consistent way across all related .dtsi/.dts files - thus no > need to worry about past decisions that have already been implemented > for other chips. Let's just agree on the technical merits of one or > the other approach, leaving "we've been doing it differently > elsewhere" aside for now. > > > >> Though, the RK3399 still does it in a safe way, by moving the OPPs > > >> into > > >> a separate dtsi file, named rk3399-opp.dtsi, which the board dts files > > >> then include together with enabling the TSADC. > > >> > > >> If you agree, let's employ the same approach for the RK3588(s), by > > >> having > > >> the its OPPs defined in a separate file, named rk3588s-opp.dtsi, etc. > > > > > > Separate file for OPPs is a good no-regret move to declutter the SoC > > > level .dtsi (as the OPP table is long and boring) - happy to move it > > > regardless of the outcome of the above TSADC discussion. Thanks for > > > the pointer! > > > > Yeah, but I'm not sure that everyone would like that kind of separation. > > In fact, such separation may be frowned upon unless it's necessary. > > > > As I already described in another thread, the separation for the RK3399 > > is there only because a couple of different variants of the RK3399 SoC > > require different OPPs. > > > > >> > - In context of dts/dtsi, as far as I understand the general logic > > >> > behind the split, the SoC .dtsi should contain all the things that are > > >> > fully contained within the SoC and do not depend on the wiring of a > > >> > particular board or its target use case. Boards then > > >> > add/remove/override settings to match their wiring and use case more > > >> > closely > > >> > > >> Of course, but the thermal shutdown is obviously a board-level > > >> feature, > > >> which I described further above. > > > > > > Not so obvious to me :-) I don't mean to be stubborn or uncooperative > > > here, but I really can't find any technical merit in having it enabled > > > at board level instead of SoC level. > > > > Well, please also consider that the PMICs from Rockchip are kind of > > weird little chips, specifically customized to serve particular SoCs. > > For example, they ensure the right sequencing and ramping-up of > > different > > power rails, which is in many cases essential. > > Sure. I'm not saying that switching to a PMIC-assisted reset shouldn't > be done where the board supports it - quite the opposite. All I'm > saying is that having at least passive cooling and CRU based resets > guaranteed for any board, regardless of how thought through its .dts > is, seems to be a better default than no thermal protection. > > > Thus, who knows what might (or might not) go wrong if we don't reset the > > PMIC at the same time when the CRU resets the SoC? Unfortunately, the > > things aren't that straightforward. > > > > On top of that, some boards, such as the Rock 5B, use a few additional > > discrete voltage regulators instead of a master-slave PMIC > > configuration, > > which may actually introduce some weird power-related issues, which also > > may be intermittent. Actually, I've already overheard that the Rock 5B > > experiences some issues of that nature, but I don't know the details. > > Those discrete regulators seem to be out of scope of this discussion. > > I agree that a deeper power-cycle with proper power-up sequence to > follow it is better when it's available in the respective hardware. > I'm also happy to provide a follow-up patch to switch from CRU to PMIC > resets for the boards I found to support the latter. > > The question we have at hand is solely about the default behavior for > a hypothetical new board with minimal .dts, or an existing board where > we can't determine the wiring of the TSHUT signal: > Option 1. Let them stay nice and warm at 120C+ under load, because > they should have known better and should have enabled the TSADC in > their device tree before putting the system under load > Option 2. Get them passively cooled at 85C under load even with no > heatsink, then force a CRU reset out of abundance of caution at 120C > unless they defined PMIC reset in their device tree > > I'm advocating for the latter. FWIW, the CRU reset is what the kernel uses for rebooting the system, either during a reboot or a kernel panic. So it is already used for both normal and abnormal scenarios. And yes, it sometimes leaves regulators or other parts of the system in some weird state that the BROM isn't expecting. Why should a hardware triggered reset be any different? ChenYu > > > Switching to PMIC-assisted resets is one thing - it definitely should > > > go to board files, as it depends on the specific wiring of the > > > TSADC_SHUT signal. Enabling TSADC in a default configuration that can > > > and will work on all boards regardless of their wiring is another > > > thing. I'm just arguing for the latter. > > > > CRU-based thermal runaway handling may in theory work on all boards, but > > we simply can't be 100% sure without detailed insights into the board > > designs and testing. Maybe even the downstream U-Boot does some magic > > during such thermal runaway resets, which we don't know. It may be > > similar to the SoC reset issues that the RK3399 suffers from. > > That might be true, but we're talking about operation at 120C+ here. > I'd rather have my board reboot in any way it pleases under those > conditions, and have that behavior triggered by default even if it's > imperfect, then worry about the correct state of all regulators and > peripherals upon next boot. The latter is important of course, but I'd > rather let it cool down and reboot it manually anyway, because that > heat could have made more things go sideways than just the regulators. > > > See also my comment above. > > > > > To me it seems similar to the watchdog timer situation: we enable it > > > at the SoC level [1], as it is expected to work in its default > > > configuration regardless of the board wiring, and it provides > > > protection against system malfunctions. Doesn't matter if the board or > > > its userspace code ends up using the full functionality - it just sits > > > there waiting for its spotlight without hurting anybody. > > > > Frankly, I don't know much about the watchdog functionality, so I'd need > > to research it before I could say something about it. > > FWIW, watchdog resets are exclusively routed through the CRU (see > RK3588 TRM V1.0 part 1 page 31). So if we expect that one to work > somehow, probably we should expect thermal resets to work too. > > Best regards, > Alexey
On 2024-03-01 12:10, Alexey Charkov wrote: > On Fri, Mar 1, 2024 at 12:52 PM Dragan Simic <dsimic@manjaro.org> > wrote: >> On 2024-03-01 09:25, Alexey Charkov wrote: >> > On Fri, Mar 1, 2024 at 9:51 AM Dragan Simic <dsimic@manjaro.org> wrote: >> >> On 2024-03-01 06:12, Alexey Charkov wrote: >> >> > On Fri, Mar 1, 2024 at 12:21 AM Dragan Simic <dsimic@manjaro.org> >> >> > wrote: >> >> >> On 2024-02-29 20:26, Alexey Charkov wrote: >> >> >> > Include thermal zones information in device tree for RK3588 variants. >> >> >> > >> >> >> > This also enables the TSADC controller unconditionally on all boards >> >> >> > to ensure that thermal protections are in place via throttling and >> >> >> > emergency reset, once OPPs are added to enable CPU DVFS. >> >> >> > >> >> >> > The default settings (using CRU as the emergency reset mechanism) >> >> >> > should work on all boards regardless of their wiring, as CRU resets >> >> >> > do not depend on any external components. Boards that have the TSHUT >> >> >> > signal wired to the reset line of the PMIC may opt to switch to GPIO >> >> >> > tshut mode instead (rockchip,hw-tshut-mode = <1>;) >> >> >> >> >> >> Quite frankly, I'm still not sure that enabling this on the SoC level >> >> >> is the way to go. As I already described in detail, [4] according to >> >> >> the RK3588 Hardware Design Guide v1.0 and the Rock 5B schematic, we >> >> >> should actually use GPIO-based handling for the thermal runaways on >> >> >> the Rock 5B. Other boards should also be investigated individually, >> >> >> and the TSADC should be enabled on a board-to-board basis. >> >> > >> >> > With all due respect, I disagree, here is why: >> >> > - Neither the schematic nor the hardware design guide, on which the >> >> > schematic seems to be based, prescribes a particular way to handle >> >> > thermal runaways. They only provide the possibility of GPIO based >> >> > resets, along with the CRU based one >> >> >> >> Please note that other documents from Rockchip also exist. Below is >> >> a link to a screenshot from the Thermal developer guide, version 1.0, >> >> which describes the whole thing further. I believe it's obvious that >> >> the thermal runaway is to be treated as a board-level feature. >> >> >> >> - https://i.imgur.com/IJ6dSAc.png >> > >> > Frankly, that still doesn't make TSADC per se a board-level thing IMO. >> > The only thing that is board-level is the wiring of GPIO based resets, >> > which I fully agree should go to board .dts for boards that support >> > it, but that's not part of the current defaults and can be safely >> > added later. >> > >> > TSADC is inside the SoC. CRU is inside the SoC. They work just fine >> > for a thermal reset, even if no dedicated reset logic is wired on the >> > board. I really don't see any downsides in having TSADC enabled by >> > default with CRU based resets: >> > - it's a safe default (i.e. I cannot think of any configuration or use >> > case where enabled-by-default TSADC does any harm) >> > - it's safer than accidentally forgetting to enable TSADC (as it adds >> > thermal protection which is otherwise missing) >> > - it will work on all boards (even if it doesn't utilize the full >> > hardware functionality by ignoring GPIO resets that some boards also >> > have in addition to the CRU) >> > - and it requires fewer overrides in board .dts files >> > >> > Sounds like a no-regret move to me. >> >> Please see my comments below. >> >> >> To be fair, that version of the Thermal developer guide dates back to >> >> 2019, meaning that it technically applies to the RK3399, for example, >> >> but the TSADC and reset circuitry design has basically remained the >> >> same for the RK3588. >> >> >> >> > - My strong belief is that defaults (regardless of context) should be >> >> > safe and reasonable, and should also minimize the need to override >> >> > them >> >> >> >> Please note that the TSADC is disabled in the RK3399 SoC dtsi, so >> >> having >> >> it disabled in the RK3588(s) SoC dtsi would provide some consistency. >> > >> > I'm happy to produce a patch to reverse the logic in RK3399 (and any >> > others for that matter) to also have TSADC enabled by default there, >> > thus saving several lines of code, if it's just about consistency. >> >> But why should we change something that has served us for years, on >> multiple SoCs, with zero troubles and with (AFAIK) zero boards >> producing >> puffs of bluish smoke? > > That's just if we are concerned about consistency across different SoC > series. The point is that I'm happy to make whatever change we agree > upon in a consistent way across all related .dtsi/.dts files - thus no > need to worry about past decisions that have already been implemented > for other chips. Let's just agree on the technical merits of one or > the other approach, leaving "we've been doing it differently elsewhere" > aside for now. I see, I'd also be willing to implement such cleanup patches. Though, let's also keep in mind that some past decisions might have some strong reasons behind them, which might not be obvious. That's another reason why I'm against enabling TSADC by default. >> >> Though, the RK3399 still does it in a safe way, by moving the OPPs >> >> into >> >> a separate dtsi file, named rk3399-opp.dtsi, which the board dts files >> >> then include together with enabling the TSADC. >> >> >> >> If you agree, let's employ the same approach for the RK3588(s), by >> >> having >> >> the its OPPs defined in a separate file, named rk3588s-opp.dtsi, etc. >> > >> > Separate file for OPPs is a good no-regret move to declutter the SoC >> > level .dtsi (as the OPP table is long and boring) - happy to move it >> > regardless of the outcome of the above TSADC discussion. Thanks for >> > the pointer! >> >> Yeah, but I'm not sure that everyone would like that kind of >> separation. >> In fact, such separation may be frowned upon unless it's necessary. >> >> As I already described in another thread, the separation for the >> RK3399 >> is there only because a couple of different variants of the RK3399 SoC >> require different OPPs. >> >> >> > - In context of dts/dtsi, as far as I understand the general logic >> >> > behind the split, the SoC .dtsi should contain all the things that are >> >> > fully contained within the SoC and do not depend on the wiring of a >> >> > particular board or its target use case. Boards then >> >> > add/remove/override settings to match their wiring and use case more >> >> > closely >> >> >> >> Of course, but the thermal shutdown is obviously a board-level >> >> feature, >> >> which I described further above. >> > >> > Not so obvious to me :-) I don't mean to be stubborn or uncooperative >> > here, but I really can't find any technical merit in having it enabled >> > at board level instead of SoC level. >> >> Well, please also consider that the PMICs from Rockchip are kind of >> weird little chips, specifically customized to serve particular SoCs. >> For example, they ensure the right sequencing and ramping-up of >> different power rails, which is in many cases essential. > > Sure. I'm not saying that switching to a PMIC-assisted reset shouldn't > be done where the board supports it - quite the opposite. All I'm > saying is that having at least passive cooling and CRU based resets > guaranteed for any board, regardless of how thought through its .dts > is, seems to be a better default than no thermal protection. The way I see it, which I tried to describe in my previous response, is that we can't be 100% sure that CRU-based resets would work as expected and 100% reliably on all boards. >> Thus, who knows what might (or might not) go wrong if we don't reset >> the >> PMIC at the same time when the CRU resets the SoC? Unfortunately, the >> things aren't that straightforward. >> >> On top of that, some boards, such as the Rock 5B, use a few additional >> discrete voltage regulators instead of a master-slave PMIC >> configuration, >> which may actually introduce some weird power-related issues, which >> also >> may be intermittent. Actually, I've already overheard that the Rock >> 5B >> experiences some issues of that nature, but I don't know the details. > > Those discrete regulators seem to be out of scope of this discussion. > > I agree that a deeper power-cycle with proper power-up sequence to > follow it is better when it's available in the respective hardware. > I'm also happy to provide a follow-up patch to switch from CRU to PMIC > resets for the boards I found to support the latter. > > The question we have at hand is solely about the default behavior for > a hypothetical new board with minimal .dts, or an existing board where > we can't determine the wiring of the TSHUT signal: > Option 1. Let them stay nice and warm at 120C+ under load, because > they should have known better and should have enabled the TSADC in > their device tree before putting the system under load > Option 2. Get them passively cooled at 85C under load even with no > heatsink, then force a CRU reset out of abundance of caution at 120C > unless they defined PMIC reset in their device tree > > I'm advocating for the latter. Just to clarify, the way I see it, we'd end up with having TSADC and CRU-based resets enabled for the boards we can't be sure to have support for PMIC resets. It's just that, the way I see it, TSADC wouldn't be enabled on the SoC level, but no boards would be left with no thermal runaway handling in place. >> > Switching to PMIC-assisted resets is one thing - it definitely should >> > go to board files, as it depends on the specific wiring of the >> > TSADC_SHUT signal. Enabling TSADC in a default configuration that can >> > and will work on all boards regardless of their wiring is another >> > thing. I'm just arguing for the latter. >> >> CRU-based thermal runaway handling may in theory work on all boards, >> but >> we simply can't be 100% sure without detailed insights into the board >> designs and testing. Maybe even the downstream U-Boot does some magic >> during such thermal runaway resets, which we don't know. It may be >> similar to the SoC reset issues that the RK3399 suffers from. > > That might be true, but we're talking about operation at 120C+ here. > I'd rather have my board reboot in any way it pleases under those > conditions, and have that behavior triggered by default even if it's > imperfect, then worry about the correct state of all regulators and > peripherals upon next boot. The latter is important of course, but I'd > rather let it cool down and reboot it manually anyway, because that > heat could have made more things go sideways than just the regulators. Makes sense, but as you can see above, the way I propose it no boards would be left to sizzle under load. That would be irresponsible and bad on multiple levels. >> See also my comment above. >> >> > To me it seems similar to the watchdog timer situation: we enable it >> > at the SoC level [1], as it is expected to work in its default >> > configuration regardless of the board wiring, and it provides >> > protection against system malfunctions. Doesn't matter if the board or >> > its userspace code ends up using the full functionality - it just sits >> > there waiting for its spotlight without hurting anybody. >> >> Frankly, I don't know much about the watchdog functionality, so I'd >> need >> to research it before I could say something about it. > > FWIW, watchdog resets are exclusively routed through the CRU (see > RK3588 TRM V1.0 part 1 page 31). So if we expect that one to work > somehow, probably we should expect thermal resets to work too. I'll see to reasearch that a bit, maybe I'll find something out that could be interesting or useful.
Hello Chen-Yu, On 2024-03-01 13:02, Chen-Yu Tsai wrote: > On Fri, Mar 1, 2024 at 7:10 PM Alexey Charkov <alchark@gmail.com> > wrote: >> On Fri, Mar 1, 2024 at 12:52 PM Dragan Simic <dsimic@manjaro.org> >> wrote: >> > On 2024-03-01 09:25, Alexey Charkov wrote: >> > > On Fri, Mar 1, 2024 at 9:51 AM Dragan Simic <dsimic@manjaro.org> wrote: >> > Thus, who knows what might (or might not) go wrong if we don't reset the >> > PMIC at the same time when the CRU resets the SoC? Unfortunately, the >> > things aren't that straightforward. >> > >> > On top of that, some boards, such as the Rock 5B, use a few additional >> > discrete voltage regulators instead of a master-slave PMIC >> > configuration, >> > which may actually introduce some weird power-related issues, which also >> > may be intermittent. Actually, I've already overheard that the Rock 5B >> > experiences some issues of that nature, but I don't know the details. >> >> Those discrete regulators seem to be out of scope of this discussion. >> >> I agree that a deeper power-cycle with proper power-up sequence to >> follow it is better when it's available in the respective hardware. >> I'm also happy to provide a follow-up patch to switch from CRU to PMIC >> resets for the boards I found to support the latter. >> >> The question we have at hand is solely about the default behavior for >> a hypothetical new board with minimal .dts, or an existing board where >> we can't determine the wiring of the TSHUT signal: >> Option 1. Let them stay nice and warm at 120C+ under load, because >> they should have known better and should have enabled the TSADC in >> their device tree before putting the system under load >> Option 2. Get them passively cooled at 85C under load even with no >> heatsink, then force a CRU reset out of abundance of caution at 120C >> unless they defined PMIC reset in their device tree >> >> I'm advocating for the latter. > > FWIW, the CRU reset is what the kernel uses for rebooting the system, > either during a reboot or a kernel panic. So it is already used for > both > normal and abnormal scenarios. And yes, it sometimes leaves regulators > or other parts of the system in some weird state that the BROM isn't > expecting. According to drivers/mfd/rk8xx-core.c, some PMICs (RK809 and RK817, to be precise) already support taking over the board resets when configured with "rockchip,system-power-controller". Perhaps we should do the same with the RK806, to avoid any possible issues with CRU-based board resets; I'll see to investigate that further. Not all Rockchip PMICs (RK808, for example) support software-initiated resets, unfortunately. According to the RK806 datasheet, it seems capable of that; see pages 27 and 28 in the version 1.0 of the datasheet. > Why should a hardware triggered reset be any different? According to the RK806 datasheet, resetting through PMIC(s) causes the PMIC(s) to cut the power rails in a controlled way, i.e. with the expected ramp-downs and sequencing, and the SoC then wakes up in a state of the regulators that's exactly the same as when it gets powered up on cold boot. Doing it that way should be better. The reset procedure _should_ be virtually the same for all Rockchip PMICs, but please don't take my word on that. Resets are described quite poorly in some PMIC datasheets.
Am Donnerstag, 29. Februar 2024, 20:26:32 CET schrieb Alexey Charkov: > Include thermal zones information in device tree for RK3588 variants. > > This also enables the TSADC controller unconditionally on all boards > to ensure that thermal protections are in place via throttling and > emergency reset, once OPPs are added to enable CPU DVFS. > > The default settings (using CRU as the emergency reset mechanism) > should work on all boards regardless of their wiring, as CRU resets > do not depend on any external components. Boards that have the TSHUT > signal wired to the reset line of the PMIC may opt to switch to GPIO > tshut mode instead (rockchip,hw-tshut-mode = <1>;) > > It seems though that downstream kernels don't use that, even for > those boards where the wiring allows for GPIO based tshut, such as > Radxa Rock 5B [1], [2], [3] > > [1] https://github.com/radxa/kernel/blob/stable-5.10-rock5/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts#L540 > [2] https://github.com/radxa/kernel/blob/stable-5.10-rock5/arch/arm64/boot/dts/rockchip/rk3588s.dtsi#L5433 > [3] https://dl.radxa.com/rock5/5b/docs/hw/radxa_rock_5b_v1423_sch.pdf page 11 (TSADC_SHUT_H) > > Signed-off-by: Alexey Charkov <alchark@gmail.com> > --- > arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 176 +++++++++++++++++++++++++++++- > 1 file changed, 175 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi > index 36b1b7acfe6a..9bf197358642 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi > @@ -10,6 +10,7 @@ > #include <dt-bindings/reset/rockchip,rk3588-cru.h> > #include <dt-bindings/phy/phy.h> > #include <dt-bindings/ata/ahci.h> > +#include <dt-bindings/thermal/thermal.h> > > / { > compatible = "rockchip,rk3588"; > @@ -2225,7 +2226,180 @@ tsadc: tsadc@fec00000 { > pinctrl-1 = <&tsadc_shut>; > pinctrl-names = "gpio", "otpout"; > #thermal-sensor-cells = <1>; > - status = "disabled"; > + status = "okay"; > + }; so I've skimmed over the general discussion, though don't have a hard opinion in either direction yet. Still there are some low-hanging fruit: - having the thermal-zones addition in a separate patch would allow to merge the obvious stuff, while this discussion is still ongoing - status=okay in a soc dtsi is wrong, because okay is the default status so if anything the status property should be removed In general I'm not that much of a fan of things just working implicitly. So somehow, when someone submits a board devicetree, I expect them to having ensured stuff is enabled somewhat ok. So even seeing a simple &tsadc { status = "okay" }; suggests that they have at least noticed the existence of thermal stuff. At least that is where my thought-process is at the moment ;-) Heiko > + thermal_zones: thermal-zones { > + /* sensor near the center of the SoC */ > + package_thermal: package-thermal { > + polling-delay-passive = <0>; > + polling-delay = <0>; > + thermal-sensors = <&tsadc 0>; > + > + trips { > + package_crit: package-crit { > + temperature = <115000>; > + hysteresis = <0>; > + type = "critical"; > + }; > + }; > + }; > + > + /* sensor between A76 cores 0 and 1 */ > + bigcore0_thermal: bigcore0-thermal { > + polling-delay-passive = <100>; > + polling-delay = <0>; > + thermal-sensors = <&tsadc 1>; > + > + trips { > + /* threshold to start collecting temperature > + * statistics e.g. with the IPA governor > + */ > + bigcore0_alert0: bigcore0-alert0 { > + temperature = <75000>; > + hysteresis = <2000>; > + type = "passive"; > + }; > + /* actual control temperature */ > + bigcore0_alert1: bigcore0-alert1 { > + temperature = <85000>; > + hysteresis = <2000>; > + type = "passive"; > + }; > + bigcore0_crit: bigcore0-crit { > + temperature = <115000>; > + hysteresis = <0>; > + type = "critical"; > + }; > + }; > + cooling-maps { > + map0 { > + trip = <&bigcore0_alert1>; > + cooling-device = > + <&cpu_b0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > + <&cpu_b1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; > + }; > + }; > + }; > + > + /* sensor between A76 cores 2 and 3 */ > + bigcore2_thermal: bigcore2-thermal { > + polling-delay-passive = <100>; > + polling-delay = <0>; > + thermal-sensors = <&tsadc 2>; > + > + trips { > + /* threshold to start collecting temperature > + * statistics e.g. with the IPA governor > + */ > + bigcore2_alert0: bigcore2-alert0 { > + temperature = <75000>; > + hysteresis = <2000>; > + type = "passive"; > + }; > + /* actual control temperature */ > + bigcore2_alert1: bigcore2-alert1 { > + temperature = <85000>; > + hysteresis = <2000>; > + type = "passive"; > + }; > + bigcore2_crit: bigcore2-crit { > + temperature = <115000>; > + hysteresis = <0>; > + type = "critical"; > + }; > + }; > + cooling-maps { > + map0 { > + trip = <&bigcore2_alert1>; > + cooling-device = > + <&cpu_b2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > + <&cpu_b3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; > + }; > + }; > + }; > + > + /* sensor between the four A55 cores */ > + little_core_thermal: littlecore-thermal { > + polling-delay-passive = <100>; > + polling-delay = <0>; > + thermal-sensors = <&tsadc 3>; > + > + trips { > + /* threshold to start collecting temperature > + * statistics e.g. with the IPA governor > + */ > + littlecore_alert0: littlecore-alert0 { > + temperature = <75000>; > + hysteresis = <2000>; > + type = "passive"; > + }; > + /* actual control temperature */ > + littlecore_alert1: littlecore-alert1 { > + temperature = <85000>; > + hysteresis = <2000>; > + type = "passive"; > + }; > + littlecore_crit: littlecore-crit { > + temperature = <115000>; > + hysteresis = <0>; > + type = "critical"; > + }; > + }; > + cooling-maps { > + map0 { > + trip = <&littlecore_alert1>; > + cooling-device = > + <&cpu_l0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > + <&cpu_l1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > + <&cpu_l2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > + <&cpu_l3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; > + }; > + }; > + }; > + > + /* sensor near the PD_CENTER power domain */ > + center_thermal: center-thermal { > + polling-delay-passive = <0>; > + polling-delay = <0>; > + thermal-sensors = <&tsadc 4>; > + > + trips { > + center_crit: center-crit { > + temperature = <115000>; > + hysteresis = <0>; > + type = "critical"; > + }; > + }; > + }; > + > + gpu_thermal: gpu-thermal { > + polling-delay-passive = <0>; > + polling-delay = <0>; > + thermal-sensors = <&tsadc 5>; > + > + trips { > + gpu_crit: gpu-crit { > + temperature = <115000>; > + hysteresis = <0>; > + type = "critical"; > + }; > + }; > + }; > + > + npu_thermal: npu-thermal { > + polling-delay-passive = <0>; > + polling-delay = <0>; > + thermal-sensors = <&tsadc 6>; > + > + trips { > + npu_crit: npu-crit { > + temperature = <115000>; > + hysteresis = <0>; > + type = "critical"; > + }; > + }; > + }; > }; > > saradc: adc@fec10000 { > >
Hello Heiko, On 2024-03-02 12:25, Heiko Stuebner wrote: > Am Donnerstag, 29. Februar 2024, 20:26:32 CET schrieb Alexey Charkov: >> Include thermal zones information in device tree for RK3588 variants. >> >> This also enables the TSADC controller unconditionally on all boards >> to ensure that thermal protections are in place via throttling and >> emergency reset, once OPPs are added to enable CPU DVFS. >> >> The default settings (using CRU as the emergency reset mechanism) >> should work on all boards regardless of their wiring, as CRU resets >> do not depend on any external components. Boards that have the TSHUT >> signal wired to the reset line of the PMIC may opt to switch to GPIO >> tshut mode instead (rockchip,hw-tshut-mode = <1>;) >> >> It seems though that downstream kernels don't use that, even for >> those boards where the wiring allows for GPIO based tshut, such as >> Radxa Rock 5B [1], [2], [3] >> >> [1] >> https://github.com/radxa/kernel/blob/stable-5.10-rock5/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts#L540 >> [2] >> https://github.com/radxa/kernel/blob/stable-5.10-rock5/arch/arm64/boot/dts/rockchip/rk3588s.dtsi#L5433 >> [3] https://dl.radxa.com/rock5/5b/docs/hw/radxa_rock_5b_v1423_sch.pdf >> page 11 (TSADC_SHUT_H) >> >> Signed-off-by: Alexey Charkov <alchark@gmail.com> >> --- >> arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 176 >> +++++++++++++++++++++++++++++- >> 1 file changed, 175 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi >> b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi >> index 36b1b7acfe6a..9bf197358642 100644 >> --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi >> +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi >> @@ -10,6 +10,7 @@ >> #include <dt-bindings/reset/rockchip,rk3588-cru.h> >> #include <dt-bindings/phy/phy.h> >> #include <dt-bindings/ata/ahci.h> >> +#include <dt-bindings/thermal/thermal.h> >> >> / { >> compatible = "rockchip,rk3588"; >> @@ -2225,7 +2226,180 @@ tsadc: tsadc@fec00000 { >> pinctrl-1 = <&tsadc_shut>; >> pinctrl-names = "gpio", "otpout"; >> #thermal-sensor-cells = <1>; >> - status = "disabled"; >> + status = "okay"; >> + }; > > so I've skimmed over the general discussion, though don't have a hard > opinion in either direction yet. Still there are some low-hanging > fruit: > > - having the thermal-zones addition in a separate patch would allow to > merge the obvious stuff, while this discussion is still ongoing Very good suggestion. > - status=okay in a soc dtsi is wrong, because okay is the default > status > so if anything the status property should be removed > > In general I'm not that much of a fan of things just working > implicitly. > So somehow, when someone submits a board devicetree, I expect them to > having ensured stuff is enabled somewhat ok. So even seeing a simple > > &tsadc { > status = "okay" > }; > > suggests that they have at least noticed the existence of thermal > stuff. I agree that having such additional "signed-off markers", so to speak, in a board dts is quite assuring. I mean, someone implementing a new dts file for a new board should simply know what needs to be done there, and there should be no excuses for not checking the thermal throttling stuff.
diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi index 36b1b7acfe6a..9bf197358642 100644 --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi @@ -10,6 +10,7 @@ #include <dt-bindings/reset/rockchip,rk3588-cru.h> #include <dt-bindings/phy/phy.h> #include <dt-bindings/ata/ahci.h> +#include <dt-bindings/thermal/thermal.h> / { compatible = "rockchip,rk3588"; @@ -2225,7 +2226,180 @@ tsadc: tsadc@fec00000 { pinctrl-1 = <&tsadc_shut>; pinctrl-names = "gpio", "otpout"; #thermal-sensor-cells = <1>; - status = "disabled"; + status = "okay"; + }; + + thermal_zones: thermal-zones { + /* sensor near the center of the SoC */ + package_thermal: package-thermal { + polling-delay-passive = <0>; + polling-delay = <0>; + thermal-sensors = <&tsadc 0>; + + trips { + package_crit: package-crit { + temperature = <115000>; + hysteresis = <0>; + type = "critical"; + }; + }; + }; + + /* sensor between A76 cores 0 and 1 */ + bigcore0_thermal: bigcore0-thermal { + polling-delay-passive = <100>; + polling-delay = <0>; + thermal-sensors = <&tsadc 1>; + + trips { + /* threshold to start collecting temperature + * statistics e.g. with the IPA governor + */ + bigcore0_alert0: bigcore0-alert0 { + temperature = <75000>; + hysteresis = <2000>; + type = "passive"; + }; + /* actual control temperature */ + bigcore0_alert1: bigcore0-alert1 { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + bigcore0_crit: bigcore0-crit { + temperature = <115000>; + hysteresis = <0>; + type = "critical"; + }; + }; + cooling-maps { + map0 { + trip = <&bigcore0_alert1>; + cooling-device = + <&cpu_b0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu_b1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; + }; + }; + }; + + /* sensor between A76 cores 2 and 3 */ + bigcore2_thermal: bigcore2-thermal { + polling-delay-passive = <100>; + polling-delay = <0>; + thermal-sensors = <&tsadc 2>; + + trips { + /* threshold to start collecting temperature + * statistics e.g. with the IPA governor + */ + bigcore2_alert0: bigcore2-alert0 { + temperature = <75000>; + hysteresis = <2000>; + type = "passive"; + }; + /* actual control temperature */ + bigcore2_alert1: bigcore2-alert1 { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + bigcore2_crit: bigcore2-crit { + temperature = <115000>; + hysteresis = <0>; + type = "critical"; + }; + }; + cooling-maps { + map0 { + trip = <&bigcore2_alert1>; + cooling-device = + <&cpu_b2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu_b3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; + }; + }; + }; + + /* sensor between the four A55 cores */ + little_core_thermal: littlecore-thermal { + polling-delay-passive = <100>; + polling-delay = <0>; + thermal-sensors = <&tsadc 3>; + + trips { + /* threshold to start collecting temperature + * statistics e.g. with the IPA governor + */ + littlecore_alert0: littlecore-alert0 { + temperature = <75000>; + hysteresis = <2000>; + type = "passive"; + }; + /* actual control temperature */ + littlecore_alert1: littlecore-alert1 { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + littlecore_crit: littlecore-crit { + temperature = <115000>; + hysteresis = <0>; + type = "critical"; + }; + }; + cooling-maps { + map0 { + trip = <&littlecore_alert1>; + cooling-device = + <&cpu_l0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu_l1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu_l2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu_l3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; + }; + }; + }; + + /* sensor near the PD_CENTER power domain */ + center_thermal: center-thermal { + polling-delay-passive = <0>; + polling-delay = <0>; + thermal-sensors = <&tsadc 4>; + + trips { + center_crit: center-crit { + temperature = <115000>; + hysteresis = <0>; + type = "critical"; + }; + }; + }; + + gpu_thermal: gpu-thermal { + polling-delay-passive = <0>; + polling-delay = <0>; + thermal-sensors = <&tsadc 5>; + + trips { + gpu_crit: gpu-crit { + temperature = <115000>; + hysteresis = <0>; + type = "critical"; + }; + }; + }; + + npu_thermal: npu-thermal { + polling-delay-passive = <0>; + polling-delay = <0>; + thermal-sensors = <&tsadc 6>; + + trips { + npu_crit: npu-crit { + temperature = <115000>; + hysteresis = <0>; + type = "critical"; + }; + }; + }; }; saradc: adc@fec10000 {
Include thermal zones information in device tree for RK3588 variants. This also enables the TSADC controller unconditionally on all boards to ensure that thermal protections are in place via throttling and emergency reset, once OPPs are added to enable CPU DVFS. The default settings (using CRU as the emergency reset mechanism) should work on all boards regardless of their wiring, as CRU resets do not depend on any external components. Boards that have the TSHUT signal wired to the reset line of the PMIC may opt to switch to GPIO tshut mode instead (rockchip,hw-tshut-mode = <1>;) It seems though that downstream kernels don't use that, even for those boards where the wiring allows for GPIO based tshut, such as Radxa Rock 5B [1], [2], [3] [1] https://github.com/radxa/kernel/blob/stable-5.10-rock5/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts#L540 [2] https://github.com/radxa/kernel/blob/stable-5.10-rock5/arch/arm64/boot/dts/rockchip/rk3588s.dtsi#L5433 [3] https://dl.radxa.com/rock5/5b/docs/hw/radxa_rock_5b_v1423_sch.pdf page 11 (TSADC_SHUT_H) Signed-off-by: Alexey Charkov <alchark@gmail.com> --- arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 176 +++++++++++++++++++++++++++++- 1 file changed, 175 insertions(+), 1 deletion(-)