Received-SPF: pass (google.com: domain of linux-kernel+bounces-49963-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240130-rk-dts-additions-v2-0-c6222c4c78df@gmail.com>
 <20240130-rk-dts-additions-v2-2-c6222c4c78df@gmail.com> <CAGb2v65--rgb2FqmG_0-w1-jUL0odqKXxiZJ-XPYA4uomfYmaQ@mail.gmail.com>
 <5e3e12d65e4919372e7fb02d56202393@manjaro.org> <CABjd4Yx0kQ67fScrFVavjObMLaNt_PJ3TVOhLpCmj00Dx9dOqA@mail.gmail.com>
 <a8aa04ca0061cd09c7b3eb5336e534a4@manjaro.org> <bd6e93cf907572e86697dc10a50bfe66@manjaro.org>
In-Reply-To: <bd6e93cf907572e86697dc10a50bfe66@manjaro.org>
From: Alexey Charkov <alchark@gmail.com>
Date: Fri, 2 Feb 2024 18:42:42 +0400
Message-ID: <CABjd4Yy0HYFEdW-XA4vmX4aMhCS-fKd7X_f=ar3qcm_4ROxk1A@mail.gmail.com>
Subject: Re: [PATCH v2 2/4] arm64: dts: rockchip: enable temperature driven
 fan control on Rock 5B
To: Dragan Simic <dsimic@manjaro.org>
Cc: wens@kernel.org, Rob Herring <robh+dt@kernel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, 
	Heiko Stuebner <heiko@sntech.de>, Daniel Lezcano <daniel.lezcano@linaro.org>, 
	Viresh Kumar <viresh.kumar@linaro.org>, 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 Thu, Feb 1, 2024 at 11:43=E2=80=AFPM Dragan Simic <dsimic@manjaro.org> w=
rote:
>
> On 2024-02-01 20:31, Dragan Simic wrote:
> > On 2024-02-01 20:15, Alexey Charkov wrote:
> >> On Thu, Feb 1, 2024 at 9:34=E2=80=AFPM Dragan Simic <dsimic@manjaro.or=
g>
> >> wrote:
> >>> On 2024-02-01 15:26, Chen-Yu Tsai wrote:
> >>> > On Wed, Jan 31, 2024 at 2:22=E2=80=AFAM Alexey Charkov <alchark@gma=
il.com>
> >>> > wrote:
> >>> >>
> >>> >> This enables thermal monitoring on Radxa Rock 5B and links the PWM
> >>> >> fan as an active cooling device managed automatically by the therm=
al
> >>> >> subsystem, with a target SoC temperature of 65C and a minimum-spin
> >>> >> interval from 55C to 65C to ensure airflow when the system gets wa=
rm
> >>> >>
> >>> >> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >>> >> Signed-off-by: Alexey Charkov <alchark@gmail.com>
> >>> >> ---
> >>> >>  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 34
> >>> >> ++++++++++++++++++++++++-
> >>> >>  1 file changed, 33 insertions(+), 1 deletion(-)
> >>> >>
> >>> >> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> >>> >> b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> >>> >> index a0e303c3a1dc..b485edeef876 100644
> >>> >> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> >>> >> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> >>> >> @@ -52,7 +52,7 @@ led_rgb_b {
> >>> >>
> >>> >>         fan: pwm-fan {
> >>> >>                 compatible =3D "pwm-fan";
> >>> >> -               cooling-levels =3D <0 95 145 195 255>;
> >>> >> +               cooling-levels =3D <0 120 150 180 210 240 255>;
> >>> >>                 fan-supply =3D <&vcc5v0_sys>;
> >>> >>                 pwms =3D <&pwm1 0 50000 0>;
> >>> >>                 #cooling-cells =3D <2>;
> >>> >> @@ -173,6 +173,34 @@ &cpu_l3 {
> >>> >>         cpu-supply =3D <&vdd_cpu_lit_s0>;
> >>> >>  };
> >>> >>
> >>> >> +&package_thermal {
> >>> >> +       polling-delay =3D <1000>;
> >>> >> +
> >>> >> +       trips {
> >>> >> +               package_fan0: package-fan0 {
> >>> >> +                       temperature =3D <55000>;
> >>> >> +                       hysteresis =3D <2000>;
> >>> >> +                       type =3D "active";
> >>> >> +               };
> >>> >> +               package_fan1: package-fan1 {
> >>> >> +                       temperature =3D <65000>;
> >>> >> +                       hysteresis =3D <2000>;
> >>> >> +                       type =3D "active";
> >>> >> +               };
> >>> >> +       };
> >>> >> +
> >>> >> +       cooling-maps {
> >>> >> +               map0 {
> >>> >> +                       trip =3D <&package_fan0>;
> >>> >> +                       cooling-device =3D <&fan THERMAL_NO_LIMIT =
1>;
> >>> >> +               };
> >>> >> +               map1 {
> >>> >> +                       trip =3D <&package_fan1>;
> >>> >> +                       cooling-device =3D <&fan 1 THERMAL_NO_LIMI=
T>;
> >>> >> +               };
> >>> >> +       };
> >>> >> +};
> >>> >> +
> >>> >>  &i2c0 {
> >>> >>         pinctrl-names =3D "default";
> >>> >>         pinctrl-0 =3D <&i2c0m2_xfer>;
> >>> >> @@ -731,6 +759,10 @@ regulator-state-mem {
> >>> >>         };
> >>> >>  };
> >>> >>
> >>> >> +&tsadc {
> >>> >> +       status =3D "okay";
> >>> >> +};
> >>> >> +
> >>> >
> >>> > Is there any reason this can't be enabled by default in the .dtsi f=
ile?
> >>> > The thermal sensor doesn't depend on anything external, so there sh=
ould
> >>> > be no reason to push this down to the board level.
> >>>
> >>> Actually, there is a reason.  Different boards can handle the
> >>> critical
> >>> overheating differently, by letting the CRU or the PMIC handle it.
> >>> This
> >>> was also the case for the RK3399.
> >>>
> >>> Please, have a look at the following DT properties, which are
> >>> consumed
> >>> by drivers/thermal/rockchip_thermal.c:
> >>>    - "rockchip,hw-tshut-mode"
> >>>    - "rockchip,hw-tshut-polarity"
> >>>
> >>> See also page 1,372 of the RK3588 TRM v1.0.
> >>>
> >>> This has also reminded me to check how is the Rock 5B actually wired,
> >>> just to make sure.  We actually need to provide the two DT properties
> >>> listed above, at least to avoid emitting the warnings.
> >>
> >> Well the defaults are already provided in rk3588s.dtsi, so there won't
> >> be any warnings (see lines 2222-2223 in Linus' master version), and
> >> according to the vendor kernel those are also what Rock 5B uses.
> >
> > Yes, I noticed the same a couple of minutes after sending my last
> > message, but didn't want to make more noise about it. :)  I would've
> > mentioned it in my next message, of course.
>
> Just checked the Rock 5B schematic and it expects the CRU to be used
> to perform the hardware reset in case of a thermal runaway, so the
> defaults in the RK3588s dtsi are fine.  I had to double-check it. :)

I've just looked at Rock 5B, Rock 5A and NanoPC T6 schematics, and
they all seem to have the TSADC_SHUT line connected to RESET_L. At the
same time, Radxa's device tree uses the default CRU-based option. To
me this seems to imply that the CRU option should always work, by the
virtue of CRU being on-chip. At the same time, if the right GPIOs are
wired to the PMIC reset line for a particular board, the board may
also choose to use the GPIO option - or stick with CRU.

If that interpretation is correct, then I suggest we enable TSADC by
default in the .dtsi, and let it handle both throttling and CRU-based
critical resets on all boards. Those who know what they are doing may
then elect to switch their board to GPIO-PMIC based reset.

What do you think?

> However, now I have some open questions related to interrupt-driven
> operation.  I'll research it further and come back with an update.
>
> >> This made me think however: what if a board doesn't enable TSADC, but
> >> has OPPs in place for higher voltage and frequency states? There won't
> >> be any throttling (as there won't be any thermal monitoring) and there
> >> might not be a critical shutdown at all if it heats up - possibly even
> >> causing hardware damage. In this case it seems that having TSADC
> >> enabled by default would at least trigger passive cooling, hopefully
> >> avoiding the critical shutdown altogether and making those properties
> >> irrelevant in 99% cases.
> >
> > Those are very good questions.  Thumbs up!
> >
> > The trouble is that the boards can use different wiring to handle the
> > thermal runaways, by expecting the PMIC to handle it or not.  Thus,
> > it's IMHO better to simply leave that to be tested and enabled on a
> > board-by-board basis, whenever a new RK3588(s)-based board is added.
> >
> > Thus, the only right way at this point would be to merge the addition
> > of the OPPs and the enabling of the TSADC for all currently supported
> > RK3588(s)-based boards at once, instead of just for the Rock 5B.

If we can agree on a workable 'default-on' configuration for TSADC to
be included in the .dtsi I think that would be preferable, because it
would enable all boards to benefit from higher OPPs and throttling.

This would also save us from a scenario when OPPs get included in the
default .dtsi, but TSADC is off by default, and then some poor soul
tries to add a new board with a minimal .dts, forgetting to enable
TSADC and having their SoC fried under high load...

> > I can handle the required changes for the QuartzPro64 dts file.  For
> > other supported RK3588(s)-based boards, if there are no people having
> > access to them and willing to perform the dts changes and the testing,
> > I'd be willing to go through the board schematics, to enable the
> > TSADC for them as well.
>
> Please, let me know are you fine with the above-described approach.

I believe it's great if we can go through the schematics no matter
what! Although if we agree that CRU is an always-working default
option for all, then why don't we just enable TSADC for all, and leave
the conversion to GPIO-PMIC resets for later and for where it's
needed?

Best regards,
Alexey