On 2024-05-06 12:29, Alexey Charkov wrote:
On Mon, May 6, 2024 at 1:52 PM Dragan Simic <dsimic@xxxxxxxxxxx> wrote:Thanks for submitting the v4 of this series! Please, see a couple of my comments below. On 2024-05-06 11:36, Alexey Charkov wrote: > This includes the necessary device tree data to allow thermal > monitoring on RK3588(s) using the on-chip TSADC device, along with > trip points for automatic thermal management. > > Each of the CPU clusters (one for the little cores and two for > the big cores) get a passive cooling trip point at 85C, which > will trigger DVFS throttling of the respective cluster upon > reaching a high temperature condition. > > All zones also have a critical trip point at 115C, which will > trigger a reset. > > Signed-off-by: Alexey Charkov <alchark@xxxxxxxxx> > --- > arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 147 > ++++++++++++++++++++++++++++++ > 1 file changed, 147 insertions(+) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi > b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi > index 6ac5ac8b48ab..ef06c1f742e8 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"; > @@ -2368,6 +2369,152 @@ pwm15: pwm@febf0030 { > status = "disabled"; > }; > > + 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 { > + bigcore0_alert: bigcore0-alert { > + temperature = <85000>; > + hysteresis = <2000>; > + type = "passive"; > + }; Doesn't removing the second passive trip, which was present in the v3, result in confusing the IPA governor?Not really - it will just treat the missing trip as 0C for its initial PID calculations [1], and will continually run the governor as opposed to putting it to rest when the temperature is below the "switch on" value [2]. Getting the power allocation governor to work optimally (i.e. to provide tangible benefits over, say, stepwise) is much more involved than defining an arbitrary switch-on trip point, as it requires an accurate estimate of sustainable power per thermal zone (which we don't have for RK3588 in general, and furthermore it must depend a lot on a particular cooling setup), and ideally some userspace power/thermal model capable of tuning the PID coefficients and updating them via sysfs based on how a particular system accumulates and dissipates heat under different load. So after thinking over it for a while I decided that those extra passive trips were rather self-deceiving, as they are only useful in the context of a power allocation governor but we do not have any of the other pieces in place for the power allocation governor to work. Better not to clutter the device tree IMO.
I see, thanks for the clarification. Please, give me some time to thoroughly test your patches, which I'll hopefully be able to do in the next few days.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/thermal/gov_power_allocator.c#n156 [2] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/thermal/gov_power_allocator.c#n487
