Re: [PATCH v4 1/6] arm64: dts: rockchip: add thermal zones information on RK3588

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux