[PATCH v3 1/5] arm64: dts: rockchip: enable built-in thermal monitoring on RK3588
Alexey Charkov
alchark at gmail.com
Thu Feb 29 23:51:56 PST 2024
On Fri, Mar 1, 2024 at 10:14 AM Dragan Simic <dsimic at manjaro.org> wrote:
>
> On 2024-03-01 06:20, Alexey Charkov wrote:
> > On Fri, Mar 1, 2024 at 1:11 AM Dragan Simic <dsimic at manjaro.org> wrote:
> >> Please see also some nitpicks below, which I forgot to mention in
> >> my earlier response. I'm sorry for that.
> >>
> >> On 2024-02-29 20:26, Alexey Charkov wrote:
> >> > Include thermal zones information in device tree for RK3588 variants.
> >> >
> >> > This also enables the TSADC controller unconditionally on all boards
> >> > to ensure that thermal protections are in place via throttling and
> >> > emergency reset, once OPPs are added to enable CPU DVFS.
> >> >
> >> > The default settings (using CRU as the emergency reset mechanism)
> >> > should work on all boards regardless of their wiring, as CRU resets
> >> > do not depend on any external components. Boards that have the TSHUT
> >> > signal wired to the reset line of the PMIC may opt to switch to GPIO
> >> > tshut mode instead (rockchip,hw-tshut-mode = <1>;)
> >> >
> >> > It seems though that downstream kernels don't use that, even for
> >> > those boards where the wiring allows for GPIO based tshut, such as
> >> > Radxa Rock 5B [1], [2], [3]
> >> >
> >> > [1]
> >> > https://github.com/radxa/kernel/blob/stable-5.10-rock5/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts#L540
> >> > [2]
> >> > https://github.com/radxa/kernel/blob/stable-5.10-rock5/arch/arm64/boot/dts/rockchip/rk3588s.dtsi#L5433
> >> > [3] https://dl.radxa.com/rock5/5b/docs/hw/radxa_rock_5b_v1423_sch.pdf
> >> > page 11 (TSADC_SHUT_H)
> >> >
> >> > Signed-off-by: Alexey Charkov <alchark at gmail.com>
> >> > ---
> >> > arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 176
> >> > +++++++++++++++++++++++++++++-
> >> > 1 file changed, 175 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> >> > b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> >> > index 36b1b7acfe6a..9bf197358642 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";
> >> > @@ -2225,7 +2226,180 @@ tsadc: tsadc at fec00000 {
> >> > pinctrl-1 = <&tsadc_shut>;
> >> > pinctrl-names = "gpio", "otpout";
> >> > #thermal-sensor-cells = <1>;
> >> > - status = "disabled";
> >> > + status = "okay";
> >> > + };
> >> > +
> >> > + 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 {
> >> > + /* threshold to start collecting temperature
> >> > + * statistics e.g. with the IPA governor
> >> > + */
> >>
> >> See, I'm not a native English speaker, but I've spent a lot of time
> >> and effort improving my English skills. Thus, perhaps these comments
> >> may or may not seem like unnecessary nitpicking, depending on how much
> >> someone pays attention to writing style in general, but I'll risk to
> >> be annoying and state these comments anyway. :)
> >>
> >> The comment above could be written in a much more condensed form like
> >> this, which would also be a bit more accurate:
> >>
> >>
> >> /* IPA threshold, when IPA governor is
> >> used */
> >>
> >> IOW, we're writing all this for someone to read later, but we should
> >> (and can) perfectly reasonably expect some already existing background
> >> knowledge from the readers. In other words, we should be as concise
> >> as possible.
> >
> > In fact, the power allocation governor code itself doesn't call those
> > trips threshold or target as your suggested wording would imply.
> > Instead, it calls them "switch on temperature" and "maximum desired
> > temperature" [1]. Maybe we can call them that in the comments (and
> > also avoid calling the governor IPA, because upstream code only calls
> > it a "power allocator").
>
> Hmm, but "IPA" is still mentioned in exactly three places in the files
> under drivers/thermal. I think that warrants the use of "IPA", which
> is also widely used pretty much everywhere.
>
> Perhaps a win-win would be to have only the very first of the comments
> like this, to introduce "IPA" as an acronym:
>
> /* Power allocator (IPA) thermal
> governor */
> /* switch-on point, when IPA governor
> is used */
Yes, good point, thanks!
> Next, "the target temperature" is mentioned more than a few times in
> drivers/thermal/gov_power_allocator.c, which I believe makes the use
> of "IPA target" perfectly valid. Actually, let's use "IPA target
> temperature", if you agree, to make it self descriptive.
Or perhaps simply "target temperature"? Stepwise governor will also
use this trip as its target, so it's not IPA specific, unlike the
switch-on point.
> Finally, the threshold... Based on
> drivers/thermal/gov_power_allocator.c,
> I think "IPA switch-on point" would be a good choice, which I already
> used above in the proposed opening comment.
Agreed, that sounds good to me, will reflect in the next iteration.
Thanks for bringing it up!
Best,
Alexey
More information about the linux-arm-kernel
mailing list