| Message ID | 20241204045447.1036-1-naoki@radxa.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] arm64: dts: rockchip: add package_thermal for Radxa ROCK 5A/5C | expand |
Hello Fukaumi, Please, see a few comments below. On 2024-12-04 05:54, FUKAUMI Naoki wrote: > add and enable package temperature based active cooling. I'd suggest that you rewrite the patch summary to read like this: arm64: dts: rockchip: Enable automatic fan control on Rock 5A/5C and to rewrite the patch description like this, which is a recycled and a bit improved description of the commit 4a152231b050 ("arm64: dts: rockchip: enable automatic fan control on Rock 5B"): Link the PWM fan on Radxa ROCK 5A/5C as an active cooling device managed automatically by the thermal subsystem, with a target SoC temperature of 65 oC and a minimum-spin interval from 55 oC to 65 oC, to ensure airflow when the system gets warm. > Signed-off-by: FUKAUMI Naoki <naoki@radxa.com> > --- > this patch depends on [1] which depends on [2]. > > [1] > https://patchwork.kernel.org/project/linux-rockchip/cover/20241128121929.62646-1-naoki@radxa.com/ > [2] > https://patchwork.kernel.org/project/linux-rockchip/patch/20241119095113.78151-1-naoki@radxa.com/ > --- > .../boot/dts/rockchip/rk3588s-rock-5.dtsi | 32 ++++++++++++++++++- > 1 file changed, 31 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5.dtsi > b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5.dtsi > index a8f40f43c838..a1cac40d439e 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5.dtsi > @@ -33,7 +33,7 @@ analog-sound { > "Headphone", "Headphones"; > }; > > - fan { > + fan: fan { It would be better to use "fan: pwm-fan { ... }", to ensure some kind of consistency with the other similar portions of board dts files. > compatible = "pwm-fan"; > #cooling-cells = <2>; > cooling-levels = <0 64 128 192 255>; > @@ -379,6 +379,36 @@ rgmii_phy1: ethernet-phy@1 { > }; > }; > > +&package_thermal { > + polling-delay = <1000>; > + > + trips { > + package_fan0: package-fan0 { > + hysteresis = <2000>; > + temperature = <55000>; > + type = "active"; > + }; > + > + package_fan1: package-fan1 { > + hysteresis = <2000>; > + temperature = <65000>; > + type = "active"; > + }; > + }; > + > + cooling-maps { > + map0 { > + cooling-device = <&fan THERMAL_NO_LIMIT 1>; > + trip = <&package_fan0>; > + }; > + > + map1 { > + cooling-device = <&fan 2 THERMAL_NO_LIMIT>; > + trip = <&package_fan1>; > + }; > + }; > +}; > + > &pcie2x1l2 { > pinctrl-names = "default"; > pinctrl-0 = <&pcie20x1_2_perstn_m0>, The rest of the patch is looking good to me. Thanks for the patch!
Hi, On 12/4/24 14:32, Dragan Simic wrote: > Hello Fukaumi, > > Please, see a few comments below. > > On 2024-12-04 05:54, FUKAUMI Naoki wrote: >> add and enable package temperature based active cooling. > > I'd suggest that you rewrite the patch summary to read like this: > > arm64: dts: rockchip: Enable automatic fan control on Rock 5A/5C > > and to rewrite the patch description like this, which is a recycled > and a bit improved description of the commit 4a152231b050 ("arm64: > dts: rockchip: enable automatic fan control on Rock 5B"): > > Link the PWM fan on Radxa ROCK 5A/5C as an active cooling device > managed automatically by the thermal subsystem, with a target SoC > temperature of 65 oC and a minimum-spin interval from 55 oC to > 65 oC, to ensure airflow when the system gets warm. thanks! I'll use this message :) >> Signed-off-by: FUKAUMI Naoki <naoki@radxa.com> >> --- >> this patch depends on [1] which depends on [2]. >> >> [1] https://patchwork.kernel.org/project/linux-rockchip/ >> cover/20241128121929.62646-1-naoki@radxa.com/ >> [2] https://patchwork.kernel.org/project/linux-rockchip/ >> patch/20241119095113.78151-1-naoki@radxa.com/ >> --- >> .../boot/dts/rockchip/rk3588s-rock-5.dtsi | 32 ++++++++++++++++++- >> 1 file changed, 31 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5.dtsi >> b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5.dtsi >> index a8f40f43c838..a1cac40d439e 100644 >> --- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5.dtsi >> +++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5.dtsi >> @@ -33,7 +33,7 @@ analog-sound { >> "Headphone", "Headphones"; >> }; >> >> - fan { >> + fan: fan { > > It would be better to use "fan: pwm-fan { ... }", to ensure some kind > of consistency with the other similar portions of board dts files. sure. I'll change it in previous patch series for Radxa ROCK 4C. Best regards, -- FUKAUMI Naoki Radxa Computer (Shenzhen) Co., Ltd. >> compatible = "pwm-fan"; >> #cooling-cells = <2>; >> cooling-levels = <0 64 128 192 255>; >> @@ -379,6 +379,36 @@ rgmii_phy1: ethernet-phy@1 { >> }; >> }; >> >> +&package_thermal { >> + polling-delay = <1000>; >> + >> + trips { >> + package_fan0: package-fan0 { >> + hysteresis = <2000>; >> + temperature = <55000>; >> + type = "active"; >> + }; >> + >> + package_fan1: package-fan1 { >> + hysteresis = <2000>; >> + temperature = <65000>; >> + type = "active"; >> + }; >> + }; >> + >> + cooling-maps { >> + map0 { >> + cooling-device = <&fan THERMAL_NO_LIMIT 1>; >> + trip = <&package_fan0>; >> + }; >> + >> + map1 { >> + cooling-device = <&fan 2 THERMAL_NO_LIMIT>; >> + trip = <&package_fan1>; >> + }; >> + }; >> +}; >> + >> &pcie2x1l2 { >> pinctrl-names = "default"; >> pinctrl-0 = <&pcie20x1_2_perstn_m0>, > > The rest of the patch is looking good to me. Thanks for the patch! >
diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5.dtsi index a8f40f43c838..a1cac40d439e 100644 --- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5.dtsi @@ -33,7 +33,7 @@ analog-sound { "Headphone", "Headphones"; }; - fan { + fan: fan { compatible = "pwm-fan"; #cooling-cells = <2>; cooling-levels = <0 64 128 192 255>; @@ -379,6 +379,36 @@ rgmii_phy1: ethernet-phy@1 { }; }; +&package_thermal { + polling-delay = <1000>; + + trips { + package_fan0: package-fan0 { + hysteresis = <2000>; + temperature = <55000>; + type = "active"; + }; + + package_fan1: package-fan1 { + hysteresis = <2000>; + temperature = <65000>; + type = "active"; + }; + }; + + cooling-maps { + map0 { + cooling-device = <&fan THERMAL_NO_LIMIT 1>; + trip = <&package_fan0>; + }; + + map1 { + cooling-device = <&fan 2 THERMAL_NO_LIMIT>; + trip = <&package_fan1>; + }; + }; +}; + &pcie2x1l2 { pinctrl-names = "default"; pinctrl-0 = <&pcie20x1_2_perstn_m0>,
add and enable package temperature based active cooling. Signed-off-by: FUKAUMI Naoki <naoki@radxa.com> --- this patch depends on [1] which depends on [2]. [1] https://patchwork.kernel.org/project/linux-rockchip/cover/20241128121929.62646-1-naoki@radxa.com/ [2] https://patchwork.kernel.org/project/linux-rockchip/patch/20241119095113.78151-1-naoki@radxa.com/ --- .../boot/dts/rockchip/rk3588s-rock-5.dtsi | 32 ++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-)