[PATCH v4 00/12] arm64: dts: rockchip: refine dts for Radxa ROCK 5C

[PATCH v4 00/12] arm64: dts: rockchip: refine dts for Radxa ROCK 5C

[FUKAUMI Naoki]

this patch series includes fixes for difference from schematic and
vendor kernel, minor improvements, cosmetic changes, etc.

FUKAUMI Naoki (12):
  arm64: dts: rockchip: change function name for blue LED for Radxa ROCK
    5C
  arm64: dts: rockchip: change node name for pwm-fan for Radxa ROCK 5C
  arm64: dts: rockchip: fix property for pwm-fan for Radxa ROCK 5C
  arm64: dts: rockchip: rename regulator for pcie2x1l2 for Radxa ROCK 5C
  arm64: dts: rockchip: add properties for pcie2x1l2 for Radxa ROCK 5C
  arm64: dts: rockchip: remove RTC and related node for Radxa ROCK 5C
  arm64: dts: rockchip: separate pinctrl for vcc_5v0 for Radxa ROCK 5C
  arm64: dts: rockchip: add cd-gpios for sdmmc for Radxa ROCK 5C
  arm64: dts: rockchip: fix pmic dcdc-reg7 regulator-name for Radxa ROCK
    5C
  arm64: dts: rockchip: fix pmic dcdc-reg10 label for Radxa ROCK 5C
  arm64: dts: rockchip: fix proparties for pmic regulators for Radxa
    ROCK 5C
  arm64: dts: rockchip: sort nodes for Radxa ROCK 5C

 .../boot/dts/rockchip/rk3588s-rock-5c.dts     | 62 +++++++------------
 1 file changed, 22 insertions(+), 40 deletions(-)

-- 
2.43.0

[PATCH v4 01/12] arm64: dts: rockchip: change function name for blue LED for Radxa ROCK 5C

[FUKAUMI Naoki]

since Radxa ROCK 5C is updated version of Radxa ROCK 5A, everything
should be compatible as much as possible.

change function for blue LED from HEARTBEAT to STATUS.

Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")
Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
---
Changes in v4
- new
---
 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 9b14d5383cdc..23e9b447b6f0 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
@@ -62,7 +62,7 @@ led-0 {
 		led-1 {
 			color = <LED_COLOR_ID_BLUE>;
 			default-state = "on";
-			function = LED_FUNCTION_HEARTBEAT;
+			function = LED_FUNCTION_STATUS;
 			gpios = <&gpio3 RK_PD5 GPIO_ACTIVE_HIGH>;
 			linux,default-trigger = "heartbeat";
 		};
-- 
2.43.0

[PATCH v4 02/12] arm64: dts: rockchip: change node name for pwm-fan for Radxa ROCK 5C

[FUKAUMI Naoki]

"pwm-fan" is more common node name.

Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
---
Changes in v4
- new
---
 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 23e9b447b6f0..630f026d856c 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
@@ -68,7 +68,7 @@ led-1 {
 		};
 	};
 
-	fan {
+	pwm-fan {
 		compatible = "pwm-fan";
 		#cooling-cells = <2>;
 		cooling-levels = <0 64 128 192 255>;
-- 
2.43.0

[PATCH v4 03/12] arm64: dts: rockchip: fix property for pwm-fan for Radxa ROCK 5C

[FUKAUMI Naoki]

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(-)

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 {
-- 
2.43.0

[PATCH v4 04/12] arm64: dts: rockchip: rename regulator for pcie2x1l2 for Radxa ROCK 5C

[FUKAUMI Naoki]

use consistent name with other regulators. no functional change is
intended.

Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")
Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
---
Changes in v4:
- reword commit message
Changes in v3:
- none
Changes in v2:
- new
---
 arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
index 85589d1a6d3b..61d75ab503b2 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
@@ -76,13 +76,13 @@ pwm-fan {
 		pwms = <&pwm3 0 60000 0>;
 	};
 
-	pcie2x1l2_3v3: regulator-pcie2x1l2-3v3 {
+	vcc3v3_pcie2x1l2: regulator-vcc3v3_pcie2x1l2 {
 		compatible = "regulator-fixed";
 		enable-active-high;
 		gpio = <&gpio0 RK_PC5 GPIO_ACTIVE_HIGH>;
 		pinctrl-names = "default";
 		pinctrl-0 = <&pow_en>;
-		regulator-name = "pcie2x1l2_3v3";
+		regulator-name = "vcc3v3_pcie2x1l2";
 		regulator-min-microvolt = <3300000>;
 		regulator-max-microvolt = <3300000>;
 		vin-supply = <&vcc_sysin>;
@@ -421,7 +421,7 @@ &pcie2x1l2 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pcie20x1_2_perstn_m0>;
 	reset-gpios = <&gpio3 RK_PD1 GPIO_ACTIVE_HIGH>;
-	vpcie3v3-supply = <&pcie2x1l2_3v3>;
+	vpcie3v3-supply = <&vcc3v3_pcie2x1l2>;
 	status = "okay";
 };
 
-- 
2.43.0

[PATCH v4 05/12] arm64: dts: rockchip: add properties for pcie2x1l2 for Radxa ROCK 5C

[FUKAUMI Naoki]

by schematic[1], pcie2x1l2 CLKREQ and WAKE pins are connected.
describe them in dts.

[1] https://dl.radxa.com/rock5/5c/docs/hw/v1100/radxa_rock_5c_schematic_v1100.pdf

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 | 4 +++-
 1 file changed, 3 insertions(+), 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 61d75ab503b2..01537b47f880 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
@@ -419,7 +419,9 @@ rgmii_phy1: ethernet-phy@1 {
 
 &pcie2x1l2 {
 	pinctrl-names = "default";
-	pinctrl-0 = <&pcie20x1_2_perstn_m0>;
+	pinctrl-0 = <&pcie20x1_2_perstn_m0>,
+		    <&pcie20x1m0_clkreqn>,
+		    <&pcie20x1m0_waken>;
 	reset-gpios = <&gpio3 RK_PD1 GPIO_ACTIVE_HIGH>;
 	vpcie3v3-supply = <&vcc3v3_pcie2x1l2>;
 	status = "okay";
-- 
2.43.0

[PATCH v4 06/12] arm64: dts: rockchip: remove RTC and related node for Radxa ROCK 5C

[FUKAUMI Naoki]

RTC doesn't exist on actual board. remove it.

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:
- new
---
 .../boot/dts/rockchip/rk3588s-rock-5c.dts     | 23 -------------------
 1 file changed, 23 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
index 01537b47f880..1ba94396aab8 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
@@ -350,23 +350,6 @@ regulator-state-mem {
 	};
 };
 
-&i2c5 {
-	pinctrl-names = "default";
-	pinctrl-0 = <&i2c5m2_xfer>;
-	status = "okay";
-
-	rtc@51 {
-		compatible = "haoyu,hym8563";
-		reg = <0x51>;
-		#clock-cells = <0>;
-		clock-output-names = "rtcic_32kout";
-		interrupt-parent = <&gpio0>;
-		interrupts = <RK_PB0 IRQ_TYPE_LEVEL_LOW>;
-		pinctrl-names = "default";
-		pinctrl-0 = <&rtc_int_l>;
-	};
-};
-
 &i2c7 {
 	status = "okay";
 
@@ -451,12 +434,6 @@ pow_en: pow-en {
 		};
 	};
 
-	rtc {
-		rtc_int_l: rtc-int-l {
-			rockchip,pins = <0 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
-		};
-	};
-
 	usb {
 		usb_host_pwren_h: usb-host-pwren-h {
 			rockchip,pins = <4 RK_PB5 RK_FUNC_GPIO &pcfg_pull_none>;
-- 
2.43.0

[PATCH v4 07/12] arm64: dts: rockchip: separate pinctrl for vcc_5v0 for Radxa ROCK 5C

[FUKAUMI Naoki]

vcc_5v0 is used in many places, separate vcc_5v0_pwren_h from usb in
pinctrl.

Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")
Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
---
Changes in v4:
- none
Changes in v3:
- new
---
 arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
index 1ba94396aab8..b632093183b2 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
@@ -446,7 +446,9 @@ usb_otg_pwren_h: usb-otg-pwren-h {
 		usb_wifi_pwr: usb-wifi-pwr {
 			rockchip,pins = <0 RK_PA0 RK_FUNC_GPIO &pcfg_pull_none>;
 		};
+	};
 
+	vcc-5v0 {
 		vcc_5v0_pwren_h: vcc-5v0-pwren-h {
 			rockchip,pins = <4 RK_PA3 RK_FUNC_GPIO &pcfg_pull_none>;
 		};
-- 
2.43.0

[PATCH v4 08/12] arm64: dts: rockchip: add cd-gpios for sdmmc for Radxa ROCK 5C

[FUKAUMI Naoki]

by discussion about cd-gpios and sdmmmc_det pin function[1], it's
better to add cd-gpios for now.

[1] https://lore.kernel.org/linux-rockchip/4920950.GXAFRqVoOG@diego/T/#u

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:
- none
---
 arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
index b632093183b2..1da082b18c0e 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
@@ -480,6 +480,7 @@ &sdmmc {
 	bus-width = <4>;
 	cap-mmc-highspeed;
 	cap-sd-highspeed;
+	cd-gpios = <&gpio0 RK_PA4 GPIO_ACTIVE_LOW>;
 	disable-wp;
 	no-sdio;
 	no-mmc;
-- 
2.43.0

[PATCH v4 09/12] arm64: dts: rockchip: fix pmic dcdc-reg7 regulator-name for Radxa ROCK 5C

[FUKAUMI Naoki]

fix regulator-name to match with node label.

Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")
Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
---
Changes in v4:
- none
Changes in v3:
- new
---
 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 1da082b18c0e..0f8048f6511b 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
@@ -634,7 +634,7 @@ regulator-state-mem {
 			};
 
 			vcc_2v0_pldo_s3: dcdc-reg7 {
-				regulator-name = "vdd_2v0_pldo_s3";
+				regulator-name = "vcc_2v0_pldo_s3";
 				regulator-always-on;
 				regulator-boot-on;
 				regulator-min-microvolt = <2000000>;
-- 
2.43.0

[PATCH v4 10/12] arm64: dts: rockchip: fix pmic dcdc-reg10 label for Radxa ROCK 5C

[FUKAUMI Naoki]

fix pmic dcdc-reg10 label to match with schematic[1].

[1] https://dl.radxa.com/rock5/5c/docs/hw/v1100/radxa_rock_5c_schematic_v1100.pdf

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:
- split from "arm64: dts: rockchip: fix pmic properties for Radxa ROCK 5C"
---
 arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
index 0f8048f6511b..fbe491e63852 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
@@ -670,8 +670,8 @@ regulator-state-mem {
 				};
 			};
 
-			vcc1v8_pmu_ddr_s3: dcdc-reg10 {
-				regulator-name = "vcc1v8_pmu_ddr_s3";
+			vcc_1v8_s3: dcdc-reg10 {
+				regulator-name = "vcc_1v8_s3";
 				regulator-always-on;
 				regulator-boot-on;
 				regulator-min-microvolt = <1800000>;
-- 
2.43.0

[PATCH v4 11/12] arm64: dts: rockchip: fix proparties for pmic regulators for Radxa ROCK 5C

[FUKAUMI Naoki]

fix proparties for dcdc-reg7 and nldo-reg3 regulators 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:
- split from "arm64: dts: rockchip: fix pmic properties for Radxa ROCK 5C"
---
 arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
index fbe491e63852..60eabe22ef02 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
@@ -639,7 +639,6 @@ vcc_2v0_pldo_s3: dcdc-reg7 {
 				regulator-boot-on;
 				regulator-min-microvolt = <2000000>;
 				regulator-max-microvolt = <2000000>;
-				regulator-ramp-delay = <12500>;
 
 				regulator-state-mem {
 					regulator-on-in-suspend;
@@ -793,7 +792,8 @@ vdda_0v75_s0: nldo-reg3 {
 				regulator-max-microvolt = <750000>;
 
 				regulator-state-mem {
-					regulator-off-in-suspend;
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <750000>;
 				};
 			};
 
-- 
2.43.0

[PATCH v4 12/12] arm64: dts: rockchip: sort nodes for Radxa ROCK 5C

[FUKAUMI Naoki]

sort nodes alphanumerically.

Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")
Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
---
Changes in v4:
- reflect other commits, no functional change
Changes in v3:
- none
Changes in v2:
- new
---
 arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
index 60eabe22ef02..a987d256c947 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
@@ -861,10 +861,6 @@ &uart2 {
 	status = "okay";
 };
 
-&usbdp_phy0 {
-	status = "okay";
-};
-
 &usb_host0_ehci {
 	status = "okay";
 };
@@ -886,7 +882,7 @@ &usb_host2_xhci {
 	status = "okay";
 };
 
-&vop_mmu {
+&usbdp_phy0 {
 	status = "okay";
 };
 
@@ -894,6 +890,10 @@ &vop {
 	status = "okay";
 };
 
+&vop_mmu {
+	status = "okay";
+};
+
 &vp0 {
 	vp0_out_hdmi0: endpoint@ROCKCHIP_VOP2_EP_HDMI0 {
 		reg = <ROCKCHIP_VOP2_EP_HDMI0>;
-- 
2.43.0

Re: [PATCH v4 03/12] arm64: dts: rockchip: fix property for pwm-fan for Radxa ROCK 5C

[Dragan Simic]

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 {

Re: [PATCH v4 03/12] arm64: dts: rockchip: fix property for pwm-fan for Radxa ROCK 5C

[FUKAUMI Naoki]

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 {
> 

Re: [PATCH v4 03/12] arm64: dts: rockchip: fix property for pwm-fan for Radxa ROCK 5C

[Alexey Charkov]

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

Re: [PATCH v4 03/12] arm64: dts: rockchip: fix property for pwm-fan for Radxa ROCK 5C

[FUKAUMI Naoki]

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
> 

Re: [PATCH v4 03/12] arm64: dts: rockchip: fix property for pwm-fan for Radxa ROCK 5C

[Dragan Simic]

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.