| Message ID | 20241209125131.4101-4-naoki@radxa.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: rockchip: refine dts for Radxa ROCK 5C | expand |
Hello Fukaumi, On 2024-12-09 13:51, FUKAUMI Naoki wrote: > fix pwm period to match with vendor kernel[1]. Instead of simply referring to the downstream vendor kernel, in this specific case the reasons for adjusting the fan PWM parameters should be explained by referring to the actual fan setup you're using, the observed fan RPM behavior, etc. > [1] > https://github.com/radxa/kernel/blob/linux-6.1-stan-rkr1/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts > > Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C") > Signed-off-by: FUKAUMI Naoki <naoki@radxa.com> > --- > Changes in v4: > - none > Changes in v3: > - none > Changes in v2: > - reword commit message > --- > arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts > b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts > index 630f026d856c..85589d1a6d3b 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts > +++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts > @@ -73,7 +73,7 @@ pwm-fan { > #cooling-cells = <2>; > cooling-levels = <0 64 128 192 255>; > fan-supply = <&vcc_5v0>; > - pwms = <&pwm3 0 10000 0>; > + pwms = <&pwm3 0 60000 0>; > }; > > pcie2x1l2_3v3: regulator-pcie2x1l2-3v3 {
Hi, Thanks for your review! On 12/10/24 01:32, Dragan Simic wrote: > Hello Fukaumi, > > On 2024-12-09 13:51, FUKAUMI Naoki wrote: >> fix pwm period to match with vendor kernel[1]. > > Instead of simply referring to the downstream vendor kernel, in this > specific case the reasons for adjusting the fan PWM parameters should > be explained by referring to the actual fan setup you're using, the > observed fan RPM behavior, etc. original commit message is: | arm64: dts: rockchip: modify fan pwm period to 60us | Reduce pwm frequency to 16.6 KHz for a larger adjustable range of AO3416 mosfet. I have no knowledge about this kind of things. Is quoting this message enough? Best regards, -- FUKAUMI Naoki Radxa Computer (Shenzhen) Co., Ltd. >> [1] https://github.com/radxa/kernel/blob/linux-6.1-stan-rkr1/arch/ >> arm64/boot/dts/rockchip/rk3588s-rock-5c.dts >> >> Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C") >> Signed-off-by: FUKAUMI Naoki <naoki@radxa.com> >> --- >> Changes in v4: >> - none >> Changes in v3: >> - none >> Changes in v2: >> - reword commit message >> --- >> arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts >> b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts >> index 630f026d856c..85589d1a6d3b 100644 >> --- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts >> +++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts >> @@ -73,7 +73,7 @@ pwm-fan { >> #cooling-cells = <2>; >> cooling-levels = <0 64 128 192 255>; >> fan-supply = <&vcc_5v0>; >> - pwms = <&pwm3 0 10000 0>; >> + pwms = <&pwm3 0 60000 0>; >> }; >> >> pcie2x1l2_3v3: regulator-pcie2x1l2-3v3 { >
Hi Naoki, On Tue, Dec 10, 2024 at 3:30 AM FUKAUMI Naoki <naoki@radxa.com> wrote: > > Hi, > > Thanks for your review! > > On 12/10/24 01:32, Dragan Simic wrote: > > Hello Fukaumi, > > > > On 2024-12-09 13:51, FUKAUMI Naoki wrote: > >> fix pwm period to match with vendor kernel[1]. > > > > Instead of simply referring to the downstream vendor kernel, in this > > specific case the reasons for adjusting the fan PWM parameters should > > be explained by referring to the actual fan setup you're using, the > > observed fan RPM behavior, etc. > > original commit message is: > > | arm64: dts: rockchip: modify fan pwm period to 60us > | Reduce pwm frequency to 16.6 KHz for a larger adjustable range of > AO3416 mosfet. > > I have no knowledge about this kind of things. Is quoting this message > enough? I think it would be better to expand a bit to make sure the commit message explains the whole rationale without too much extra digging. Something like this: arm64: dts: rockchip: Use a longer PWM period for the fan on Radxa ROCK 5C The fan on Radxa ROCK 5C is driven via an AO3416 MOSFET, which has a total switch-on time of 0,6us and a total switch-off time of 6us [1], meaning that the current PWM period of just 10us is too short for fine-grained fan speed control. Increase the PWM period to 60us, so that the switch-on and switch-off time of the MOSFET fall within a more reasonable ~10% of the full period, thus making lower PWM duty cycles meaningful. [1] https://www.aosmd.com/pdfs/datasheet/AO3416.pdf Best regards, Alexey
Hi Alexey, On 12/10/24 09:40, Alexey Charkov wrote: > Hi Naoki, > > On Tue, Dec 10, 2024 at 3:30 AM FUKAUMI Naoki <naoki@radxa.com> wrote: >> >> Hi, >> >> Thanks for your review! >> >> On 12/10/24 01:32, Dragan Simic wrote: >>> Hello Fukaumi, >>> >>> On 2024-12-09 13:51, FUKAUMI Naoki wrote: >>>> fix pwm period to match with vendor kernel[1]. >>> >>> Instead of simply referring to the downstream vendor kernel, in this >>> specific case the reasons for adjusting the fan PWM parameters should >>> be explained by referring to the actual fan setup you're using, the >>> observed fan RPM behavior, etc. >> >> original commit message is: >> >> | arm64: dts: rockchip: modify fan pwm period to 60us >> | Reduce pwm frequency to 16.6 KHz for a larger adjustable range of >> AO3416 mosfet. >> >> I have no knowledge about this kind of things. Is quoting this message >> enough? > > I think it would be better to expand a bit to make sure the commit > message explains the whole rationale without too much extra digging. > Something like this: > > arm64: dts: rockchip: Use a longer PWM period for the fan on Radxa ROCK 5C > > The fan on Radxa ROCK 5C is driven via an AO3416 MOSFET, which has a > total switch-on time of 0,6us and a total switch-off time of 6us [1], > meaning that the current PWM period of just 10us is too short for > fine-grained fan speed control. Increase the PWM period to 60us, so > that the switch-on and switch-off time of the MOSFET fall within a > more reasonable ~10% of the full period, thus making lower PWM duty > cycles meaningful. > > [1] https://www.aosmd.com/pdfs/datasheet/AO3416.pdf Thank you so much! Best regards, -- FUKAUMI Naoki Radxa Computer (Shenzhen) Co., Ltd. > Best regards, > Alexey >
Hello Alexey, On 2024-12-10 01:40, Alexey Charkov wrote: > On Tue, Dec 10, 2024 at 3:30 AM FUKAUMI Naoki <naoki@radxa.com> wrote: >> Thanks for your review! >> >> On 12/10/24 01:32, Dragan Simic wrote: >> > Hello Fukaumi, >> > >> > On 2024-12-09 13:51, FUKAUMI Naoki wrote: >> >> fix pwm period to match with vendor kernel[1]. >> > >> > Instead of simply referring to the downstream vendor kernel, in this >> > specific case the reasons for adjusting the fan PWM parameters should >> > be explained by referring to the actual fan setup you're using, the >> > observed fan RPM behavior, etc. >> >> original commit message is: >> >> | arm64: dts: rockchip: modify fan pwm period to 60us >> | Reduce pwm frequency to 16.6 KHz for a larger adjustable range of >> AO3416 mosfet. >> >> I have no knowledge about this kind of things. Is quoting this message >> enough? > > I think it would be better to expand a bit to make sure the commit > message explains the whole rationale without too much extra digging. > Something like this: > > arm64: dts: rockchip: Use a longer PWM period for the fan on Radxa ROCK > 5C > > The fan on Radxa ROCK 5C is driven via an AO3416 MOSFET, which has a > total switch-on time of 0,6us and a total switch-off time of 6us [1], > meaning that the current PWM period of just 10us is too short for > fine-grained fan speed control. Increase the PWM period to 60us, so > that the switch-on and switch-off time of the MOSFET fall within a > more reasonable ~10% of the full period, thus making lower PWM duty > cycles meaningful. > > [1] https://www.aosmd.com/pdfs/datasheet/AO3416.pdf Well written, thanks. That's pretty much the same how I wanted to explain it, but you were faster. :) Additionally, it's quite strange that the AO3416 FET is used in that place. Its large continuous current-carrying capability (over 5 A at 70 oC!) is an absolute overkill for driving a small fan, and its integrated ESD protection ends up basically useless in this case. Radxa could've easily redirected a few pennies into something else by using a much less beefy FET.
diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts index 630f026d856c..85589d1a6d3b 100644 --- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts +++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts @@ -73,7 +73,7 @@ pwm-fan { #cooling-cells = <2>; cooling-levels = <0 64 128 192 255>; fan-supply = <&vcc_5v0>; - pwms = <&pwm3 0 10000 0>; + pwms = <&pwm3 0 60000 0>; }; pcie2x1l2_3v3: regulator-pcie2x1l2-3v3 {
fix pwm period to match with vendor kernel[1]. [1] https://github.com/radxa/kernel/blob/linux-6.1-stan-rkr1/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C") Signed-off-by: FUKAUMI Naoki <naoki@radxa.com> --- Changes in v4: - none Changes in v3: - none Changes in v2: - reword commit message --- arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)