Received-SPF: pass (google.com: domain of linux-kernel+bounces-32362-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
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>
 <e45f72c2-0b5b-44dd-ad39-e7e5bd17ae26@linaro.org>
In-Reply-To: <e45f72c2-0b5b-44dd-ad39-e7e5bd17ae26@linaro.org>
From: Alexey Charkov <alchark@gmail.com>
Date: Mon, 22 Jan 2024 09:57:08 +0400
Message-ID: <CABjd4YyL1ZwNOJrWPwZtc7=e4h18a9tQOnuKP6soy=iTHv-WEA@mail.gmail.com>
Subject: Re: [PATCH v2] arm64: dts: rockchip: enable built-in thermal
 monitoring on rk3588
To: Daniel Lezcano <daniel.lezcano@linaro.org>
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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Jan 22, 2024 at 4:04=E2=80=AFAM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
>
> Hi Alexey,
>
>
> On 21/01/2024 20:57, Alexey Charkov wrote:
> > On Fri, Jan 19, 2024 at 8:21=E2=80=AFPM 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/a=
rm64/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 =3D "okay";
> >>>    };
> >>> +
> >>> +&tsadc {
> >>> +     status =3D "okay";
> >>> +};
> >>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/b=
oot/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 =3D "rockchip,rk3588";
> >>> @@ -2112,6 +2113,156 @@ tsadc: tsadc@fec00000 {
> >>>                status =3D "disabled";
> >>>        };
> >>>
> >>> +     thermal_zones: thermal-zones {
> >>> +             /* sensor near the center of the whole chip */
> >>> +             soc_thermal: soc-thermal {
> >>> +                     polling-delay-passive =3D <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 =3D <0>;
> polling-delay =3D <0>;
>
> cf:
>
> git grep "polling-delay =3D <0>" arch/arm64/boot/dts

Indeed, thanks a lot for the pointer! Somehow it slipped my attention.
Will test and amend accordingly.

> >>> +                     polling-delay =3D <1000>;
> >>
> >> The driver is interrupt driven. No need to poll.
> >
> > Same here as above
> >
> >>> +                     sustainable-power =3D <2100>;
> >>
> >> There is no mitigation with this thermal zone. Specifying a sustainabl=
e
> >> power does not make sense.
> >
> > Thanks, will drop this in v3!
> >
> >>> +                     thermal-sensors =3D <&tsadc 0>;
> >>> +
> >>> +                     trips {
> >>> +                             soc_crit: soc-crit {
> >>> +                                     temperature =3D <115000>;
> >>> +                                     hysteresis =3D <2000>;
> >>
> >> This trip point leads to a system shutdown / reboot. It is not necessa=
ry
> >> 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 =3D <0>;

Makes sense, thank you! Will amend accordingly.

Best regards,
Alexey