Received-SPF: pass (google.com: domain of linux-kernel+bounces-32227-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <e45f72c2-0b5b-44dd-ad39-e7e5bd17ae26@linaro.org>
Date: Mon, 22 Jan 2024 01:04:38 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] arm64: dts: rockchip: enable built-in thermal
 monitoring on rk3588
Content-Language: en-US
To: Alexey Charkov <alchark@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
 Conor Dooley <conor+dt@kernel.org>, Heiko Stuebner <heiko@sntech.de>,
 Sebastian Reichel <sebastian.reichel@collabora.com>,
 Cristian Ciocaltea <cristian.ciocaltea@collabora.com>,
 Christopher Obbard <chris.obbard@collabora.com>,
 =?UTF-8?B?VGFtw6FzIFN6xbFjcw==?= <szucst@iit.uni-miskolc.hu>,
 Shreeya Patel <shreeya.patel@collabora.com>,
 Kever Yang <kever.yang@rock-chips.com>, Jagan Teki <jagan@edgeble.ai>,
 Chris Morgan <macromorgan@hotmail.com>, devicetree@vger.kernel.org,
 linux-arm-kernel@lists.infradead.org, linux-rockchip@lists.infradead.org,
 linux-kernel@vger.kernel.org
References: <20240106222357.23835-1-alchark@gmail.com>
 <20240109192608.5981-1-alchark@gmail.com>
 <c517f26c-34bc-4b99-b744-8e2549cd28b5@linaro.org>
 <CABjd4YxYpsvf+ghHTn1z8TAZxQb-2dFOQaVSm8zHKSSWOokqww@mail.gmail.com>
From: Daniel Lezcano <daniel.lezcano@linaro.org>
In-Reply-To: <CABjd4YxYpsvf+ghHTn1z8TAZxQb-2dFOQaVSm8zHKSSWOokqww@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


Hi Alexey,


On 21/01/2024 20:57, Alexey Charkov wrote:
> On Fri, Jan 19, 2024 at 8:21 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
> Hello Daniel,
> 
> Thanks a lot for your review and comments! Please see some reflections below.
> 
>> On 09/01/2024 20:19, Alexey Charkov wrote:
>>> Include thermal zones information in device tree for rk3588 variants
>>> and enable the built-in thermal sensing ADC on RADXA Rock 5B
>>>
>>> Signed-off-by: Alexey Charkov <alchark@gmail.com>
>>> ---
>>> Changes in v2:
>>>    - Dropped redundant comments
>>>    - Included all CPU cores in cooling maps
>>>    - Split cooling maps into more granular ones utilizing TSADC
>>>      channels 1-3 which measure temperature by separate CPU clusters
>>>      instead of channel 0 which measures the center of the SoC die
>>> ---
>>>    .../boot/dts/rockchip/rk3588-rock-5b.dts      |   4 +
>>>    arch/arm64/boot/dts/rockchip/rk3588s.dtsi     | 151 ++++++++++++++++++
>>>    2 files changed, 155 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>>> index a5a104131403..f9d540000de3 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>>> @@ -772,3 +772,7 @@ &usb_host1_ehci {
>>>    &usb_host1_ohci {
>>>        status = "okay";
>>>    };
>>> +
>>> +&tsadc {
>>> +     status = "okay";
>>> +};
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>>> index 8aa0499f9b03..8d54998d0ecc 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";
>>> @@ -2112,6 +2113,156 @@ tsadc: tsadc@fec00000 {
>>>                status = "disabled";
>>>        };
>>>
>>> +     thermal_zones: thermal-zones {
>>> +             /* sensor near the center of the whole chip */
>>> +             soc_thermal: soc-thermal {
>>> +                     polling-delay-passive = <20>;
>>
>> There is no mitigation set for this thermal zone. It is pointless to
>> specify a passive polling.
> 
> Indeed, it makes sense to me. There seems to be a catch though in that
> the driver calls the generic thermal_of_zone_register during the
> initial probe, which expects both of those polling delays to be
> present in the device tree, otherwise it simply refuses to add the
> respective thermal zone, see drivers/thermal/thermal_of.c:502

Usually:

polling-delay-passive = <0>;
polling-delay = <0>;

cf:

git grep "polling-delay = <0>" arch/arm64/boot/dts

>>> +                     polling-delay = <1000>;
>>
>> The driver is interrupt driven. No need to poll.
> 
> Same here as above
> 
>>> +                     sustainable-power = <2100>;
>>
>> There is no mitigation with this thermal zone. Specifying a sustainable
>> power does not make sense.
> 
> Thanks, will drop this in v3!
> 
>>> +                     thermal-sensors = <&tsadc 0>;
>>> +
>>> +                     trips {
>>> +                             soc_crit: soc-crit {
>>> +                                     temperature = <115000>;
>>> +                                     hysteresis = <2000>;
>>
>> This trip point leads to a system shutdown / reboot. It is not necessary
>> to specify a hysteresis.
> 
> Similar to the above, the generic thermal_of code refuses to add the
> trip point if it has no hysteresis property defined (regardless of the
> trip type), see drivers/thermal/thermal_of.c:109

hysteresis = <0>;


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog