[PATCH] arm64: dts: rockchip: Fix PWM pinctrl names

[PATCH] arm64: dts: rockchip: Fix PWM pinctrl names

[Yao Zi]

These Rockchip boards assign "active" as the pinctrl name for PWM
controllers, which has never been supported in mainline Rockchip PWM
driver. It seems the name used by downstream kernel is accidentally
brought into maineline. Let's fix them.

Fixes: 4403e1237be3 ("arm64: dts: rockchip: Add devicetree for board roc-rk3308-cc")
Fixes: 964ed0807b5f ("arm64: dts: rockchip: add rk3318 A95X Z2 board")
Fixes: e7a095908227 ("arm64: dts: rockchip: Add devicetree for NanoPC-T4")
Fixes: 3f5d336d64d6 ("arm64: dts: rockchip: Add support for rk3588s based board Cool Pi 4B")
Signed-off-by: Yao Zi <ziyao@disroot.org>
---

It's uncommon to have a commit carrying so many fix tags, but otherwise
four seperate small commits will be created with the same topic. I'm
willing to change this if it's inappropriate.

I don't have these boards for a real test, but the change should be the
right way.

Thanks for your time and review.

 arch/arm64/boot/dts/rockchip/rk3308-roc-cc.dts     | 2 +-
 arch/arm64/boot/dts/rockchip/rk3318-a95x-z2.dts    | 4 ++--
 arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi   | 2 +-
 arch/arm64/boot/dts/rockchip/rk3588s-coolpi-4b.dts | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3308-roc-cc.dts b/arch/arm64/boot/dts/rockchip/rk3308-roc-cc.dts
index 629121de5a13..5e7181948992 100644
--- a/arch/arm64/boot/dts/rockchip/rk3308-roc-cc.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3308-roc-cc.dts
@@ -147,7 +147,7 @@ rtc: rtc@51 {
 
 &pwm5 {
 	status = "okay";
-	pinctrl-names = "active";
+	pinctrl-names = "default";
 	pinctrl-0 = <&pwm5_pin_pull_down>;
 };
 
diff --git a/arch/arm64/boot/dts/rockchip/rk3318-a95x-z2.dts b/arch/arm64/boot/dts/rockchip/rk3318-a95x-z2.dts
index a94114fb7cc1..96c27fc5005d 100644
--- a/arch/arm64/boot/dts/rockchip/rk3318-a95x-z2.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3318-a95x-z2.dts
@@ -274,13 +274,13 @@ otg_vbus_drv: otg-vbus-drv {
 
 &pwm0 {
 	pinctrl-0 = <&pwm0_pin_pull_up>;
-	pinctrl-names = "active";
+	pinctrl-names = "default";
 	status = "okay";
 };
 
 &pwm1 {
 	pinctrl-0 = <&pwm1_pin_pull_up>;
-	pinctrl-names = "active";
+	pinctrl-names = "default";
 	status = "okay";
 };
 
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
index b169be06d4d1..c8eb5481f43d 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
@@ -603,7 +603,7 @@ &pwm1 {
 };
 
 &pwm2 {
-	pinctrl-names = "active";
+	pinctrl-names = "default";
 	pinctrl-0 = <&pwm2_pin_pull_down>;
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-coolpi-4b.dts b/arch/arm64/boot/dts/rockchip/rk3588s-coolpi-4b.dts
index 8f00e0444c00..8b717c4017a4 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-coolpi-4b.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-coolpi-4b.dts
@@ -433,7 +433,7 @@ &pwm2 {
 };
 
 &pwm13 {
-	pinctrl-names = "active";
+	pinctrl-names = "default";
 	pinctrl-0 = <&pwm13m2_pins>;
 	status = "okay";
 };
-- 
2.48.1

Re: [PATCH] arm64: dts: rockchip: Fix PWM pinctrl names

[Heiko Stuebner]

On Mon, 10 Mar 2025 14:09:17 +0000, Yao Zi wrote:
> These Rockchip boards assign "active" as the pinctrl name for PWM
> controllers, which has never been supported in mainline Rockchip PWM
> driver. It seems the name used by downstream kernel is accidentally
> brought into maineline. Let's fix them.
> 
> 

Applied, thanks!

[1/1] arm64: dts: rockchip: Fix PWM pinctrl names
      commit: 96d8d325324639f03fcca12e4ea92265ebb24837

Best regards,
-- 
Heiko Stuebner <heiko@sntech.de>

Re: [PATCH v7 4/5] arm64: dts: rockchip: add core dtsi for RK3562 SoC

[Heiko Stübner]

Hi,

Am Sonntag, 11. Mai 2025, 12:00:22 Mitteleuropäische Sommerzeit schrieb Chukun Pan:
> > <snip>
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/rockchip/rk3562.dtsi
> > @@ -0,0 +1,1187 @@
> > <snip>
> > +#include <dt-bindings/phy/phy.h>
> > +#include <dt-bindings/power/rockchip,rk3562-power.h>
> > +#include <dt-bindings/pinctrl/rockchip.h>
> 
> Does this need to be sorted alphabetically?

the rk3562-power header is gone temporarily anyway (in another tree),
so I moved things to numbers temporarily and created
	https://lore.kernel.org/r/20250510161531.2086706-1-heiko@sntech.de
to be applied after the merge-window.

> 
> > <snip>
> > +		idle-states {
> > +			entry-method = "psci";
> 
> It would be better to leave a blank line here.

added

> > +			CPU_SLEEP: cpu-sleep {
> 
> > <snip>
> > +		pwm0: pwm@ff230000 {
> > +			compatible = "rockchip,rk3562-pwm", "rockchip,rk3328-pwm";
> > +			reg = <0x0 0xff230000 0x0 0x10>;
> > +			#pwm-cells = <3>;
> > +			pinctrl-names = "active";
> 
> It should be `pinctrl-names = "default";` for pwm, see also [1]

fixed the pinctrl


> > +			pinctrl-0 = <&pwm0m0_pins>;
> > +			clocks = <&cru CLK_PMU1_PWM0>, <&cru PCLK_PMU1_PWM0>;
> > +			clock-names = "pwm", "pclk";
> > +			status = "disabled";
> > +		};
> 
> The pinctrl should be placed below the clock.

fixed ordering

> 
> > <snip>
> > +			power: power-controller {
> > +				compatible = "rockchip,rk3562-power-controller";
> > +				#power-domain-cells = <1>;
> > +				#address-cells = <1>;
> > +				#size-cells = <0>;
> > +				status = "okay";
> > +
> 
> `status = "okay"` is not needed here.
> Also remove extra blank lines.

dropped okay ... extra blank was already removing when applying

> 
> > +
> > +				power-domain@RK3562_PD_GPU {
> > <snip>
> > +				power-domain@RK3562_PD_VI {
> > +					reg = <RK3562_PD_VI>;
> > +					#power-domain-cells = <1>;
> > +					#address-cells = <1>;
> > +					#size-cells = <0>;
> > +					pm_qos = <&qos_isp>,
> > +						 <&qos_vicap>;
> > +
> > +					power-domain@RK3562_PD_VEPU {
> > +						reg = <RK3562_PD_VEPU>;
> > +						pm_qos = <&qos_vepu>;
> > +					#power-domain-cells = <0>;
> 
> This line is missing a tab, resulting in an indentation error.

fixed the indentation


> > +					};
> > +				};
> > <snip>
> 
> > +		pcie2x1: pcie@ff500000 {
> > +			compatible = "rockchip,rk3562-pcie", "rockchip,rk3568-pcie";
> > +			#address-cells = <3>;
> > +			#size-cells = <2>;
> 
> #address-cells/#size-cells should be placed above `status = "disabled";`
> I think other nodes also need to change this. (Some for #pwm-cells)

moved them (and the #pwm-cells above)


> > +			interrupt-names = "sys", "pmc", "msg", "legacy", "err", "msi";
> 
> I noticed that the bsp 5.10 kernel said that pcie only has 8 MSI vectors,
> [2][3] but in the bsp 6.1 kernel it changed to 32 MSI vectors [4].
> 
> The rockchip documentation also says there are only 8 MSI vectors:
> 
> [5] Page37 8.8 "RK3528/RK3562/RK3576可分配的MSI或者MSI-X总数是8个"
> Translate into English: "The total number of MSI or MSI-X that
> can be allocated by RK3528/RK3562/RK3576 is 8"
> 
> We noticed this when supporting rk3528, so which one is correct?

I'll leave that for a fixup patch or further discussion ;-)


> > +			phys = <&combphy_pu PHY_TYPE_PCIE>;
> 
> s/combphy_pu/combphy
> 
> > <snip>
> +		combphy_pu: phy@ff750000 {
> 
> Please change it to `combphy` like rk3568.

changed the naming.


> Heiko I know you just merged this, it's a bit offensive but I think
> it would be better to drop these patches and fix them further.
> Or kever would you like to send a fix patch? Although most of
> them are typo issues, it will take a few patches to fix it.

First of all, thanks for noticing all the bits and pieces to improve.
I di think I have now fixed up all the "regular" pieces you mentioned
and amended the commit accordingly:

https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git/commit/?id=1d2f65fa98ddcafdfd1ebcdb87105141861b584a


Heiko

Re: [PATCH v7 4/5] arm64: dts: rockchip: add core dtsi for RK3562 SoC

[Heiko Stübner]

Am Sonntag, 11. Mai 2025, 14:00:09 Mitteleuropäische Sommerzeit schrieb Chukun Pan:
> > First of all, thanks for noticing all the bits and pieces to improve.
> > I di think I have now fixed up all the "regular" pieces you mentioned
> > and amended the commit accordingly:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git/commit/?id=1d2f65fa98ddcafdfd1ebcdb87105141861b584a
> 
> Thanks a lot for the quick fix! It seems there is still a little problem:
> 
> > <snip>
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/rockchip/rk3562.dtsi
> > <snip>
> +	gpu_opp_table: opp-table-gpu {
> +		compatible = "operating-points-v2";
> +
> +		opp-300000000 {
> +			opp-hz = /bits/ 64 <300000000>;
> +			opp-microvolt = <825000 825000 1000000>;
> +		};
> +		opp-400000000 {
> +		opp-hz = /bits/ 64 <400000000>;
> 
> This line is missing a tab.

fixed :-) .

> +			opp-microvolt = <825000 825000 1000000>;
> +		};
> > <snip>
> +		spi0: spi@ff220000 {
> +			compatible = "rockchip,rk3562-spi", "rockchip,rk3066-spi";
> +			reg = <0x0 0xff220000 0x0 0x1000>;
> +			interrupts = <GIC_SPI 52 IRQ_TYPE_LEVEL_HIGH>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> 
> Also needed here.

I might be blind, but I don't see a tab missing here? #adress-cells and
#size-cells are in the same level of indentation as the other properties
of spi0? I did move the -cells down though now.


> > <snip>
> > +		pwm3: pwm@ff230030 {
> > +			compatible = "rockchip,rk3562-pwm", "rockchip,rk3328-pwm";
> > +			reg = <0x0 0xff230030 0x0 0x10>;
> > +			#pwm-cells = <3>;
> 
> Missed here.

adapted like the other pwm-nodes

> > <snip>
> > +				power-domain@12 {
> > +					reg = <12>;
> > +					#power-domain-cells = <1>;
> > +					#address-cells = <1>;
> > +					#size-cells = <0>;
> > ...
> > +				power-domain@13 {
> > +					reg = <13>;
> > +					#power-domain-cells = <1>;
> 
> Does #power/#address/#size need to be put under pm_qos?

moved

> > <snip>
> > +		spi1: spi@ff640000 {
> > +			compatible = "rockchip,rk3066-spi";
> > +			reg = <0x0 0xff640000 0x0 0x1000>;
> > +			interrupts = <GIC_SPI 53 IRQ_TYPE_LEVEL_HIGH>;
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > ...
> > +		spi2: spi@ff650000 {
> > +			compatible = "rockchip,rk3066-spi";
> > +			reg = <0x0 0xff650000 0x0 0x1000>;
> > +			interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> 
> Same here.

moved

> 
> > <snip>
> > +		pwm4: pwm@ff700000 {
> > +			compatible = "rockchip,rk3562-pwm", "rockchip,rk3328-pwm";
> > +			reg = <0x0 0xff700000 0x0 0x10>;
> > +			#pwm-cells = <3>;
> > +			pinctrl-names = "active";
> > ...
> > +		pwm5: pwm@ff700010 {
> > +			compatible = "rockchip,rk3562-pwm", "rockchip,rk3328-pwm";
> > +			reg = <0x0 0xff700010 0x0 0x10>;
> > +			#pwm-cells = <3>;
> > +			pinctrl-names = "active";
> > ...
> > +		pwm6: pwm@ff700020 {
> > +			compatible = "rockchip,rk3562-pwm", "rockchip,rk3328-pwm";
> > +			reg = <0x0 0xff700020 0x0 0x10>;
> > +			#pwm-cells = <3>;
> > +			pinctrl-names = "active";
> > ...
> > +		pwm7: pwm@ff700030 {
> > +			compatible = "rockchip,rk3562-pwm", "rockchip,rk3328-pwm";
> > +			reg = <0x0 0xff700030 0x0 0x10>;
> > +			#pwm-cells = <3>;
> > +			pinctrl-names = "active";
> > ...
> > +		pwm8: pwm@ff710000 {
> > +			compatible = "rockchip,rk3562-pwm", "rockchip,rk3328-pwm";
> > +			reg = <0x0 0xff710000 0x0 0x10>;
> > +			#pwm-cells = <3>;
> > +			pinctrl-names = "active";
> > ...
> > +		pwm9: pwm@ff710010 {
> > +			compatible = "rockchip,rk3562-pwm", "rockchip,rk3328-pwm";
> > +			reg = <0x0 0xff710010 0x0 0x10>;
> > +			#pwm-cells = <3>;
> > +			pinctrl-names = "active";
> > ...
> > +		pwm10: pwm@ff710020 {
> > +			compatible = "rockchip,rk3562-pwm", "rockchip,rk3328-pwm";
> > +			reg = <0x0 0xff710020 0x0 0x10>;
> > +			#pwm-cells = <3>;
> > +			pinctrl-names = "active";
> > ...
> > +		pwm11: pwm@ff710030 {
> > +			compatible = "rockchip,rk3562-pwm", "rockchip,rk3328-pwm";
> > +			reg = <0x0 0xff710030 0x0 0x10>;
> > +			#pwm-cells = <3>;
> > +			pinctrl-names = "active";
> > ...
> > +		pwm12: pwm@ff720000 {
> > +			compatible = "rockchip,rk3562-pwm", "rockchip,rk3328-pwm";
> > +			reg = <0x0 0xff720000 0x0 0x10>;
> > +			#pwm-cells = <3>;
> > +			pinctrl-names = "active";
> > ...
> > +		pwm13: pwm@ff720010 {
> > +			compatible = "rockchip,rk3562-pwm", "rockchip,rk3328-pwm";
> > +			reg = <0x0 0xff720010 0x0 0x10>;
> > +			#pwm-cells = <3>;
> > +			pinctrl-names = "active";
> > ...
> > +		pwm14: pwm@ff720020 {
> > +			compatible = "rockchip,rk3562-pwm", "rockchip,rk3328-pwm";
> > +			reg = <0x0 0xff720020 0x0 0x10>;
> > +			#pwm-cells = <3>;
> > +			pinctrl-names = "active";
> > ...
> > +		pwm15: pwm@ff720030 {
> > +			compatible = "rockchip,rk3562-pwm", "rockchip,rk3328-pwm";
> > +			reg = <0x0 0xff720030 0x0 0x10>;
> > +			#pwm-cells = <3>;
> > +			pinctrl-names = "active";
> 
> pinctrl and #pwm-cells forgot to change.

hopefully caught all pwms now


> > <snip>
> > +		sdmmc0: mmc@ff880000 {
> > +			compatible = "rockchip,rk3562-dw-mshc",
> > +				     "rockchip,rk3288-dw-mshc";
> > +			reg = <0x0 0xff880000 0x0 0x10000>;
> > +			interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
> > +			max-frequency = <200000000>;
> > ...
> > +		sdmmc1: mmc@ff890000 {
> > +			compatible = "rockchip,rk3562-dw-mshc",
> > +				     "rockchip,rk3288-dw-mshc";
> > +			reg = <0x0 0xff890000 0x0 0x10000>;
> > +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> > +			max-frequency = <200000000>;
> 
> max-frequency should be placed below clock

moved and also moved fifo-depth upwards between clock-names
and max-frequency


> > <snip>
> > +		saradc0: adc@ff730000 {
> > +			compatible = "rockchip,rk3562-saradc";
> > +			reg = <0x0 0xff730000 0x0 0x100>;
> > +			interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
> > +			#io-channel-cells = <1>;
> > ...
> > +		saradc1: adc@ffaa0000 {
> > +			compatible = "rockchip,rk3562-saradc";
> > +			reg = <0x0 0xffaa0000 0x0 0x100>;
> > +			interrupts = <GIC_SPI 124 IRQ_TYPE_LEVEL_HIGH>;
> > +			#io-channel-cells = <1>;
> 
> `#io-channel-cells` should be put above `status = "disabled";`

moved now :-)

Hopefully this now caught all the smallish issues.


Thanks a lot
Heiko

Re: [PATCH v7 4/5] arm64: dts: rockchip: add core dtsi for RK3562 SoC

[Chukun Pan]

Hi,

> I might be blind, but I don't see a tab missing here? #adress-cells and
> #size-cells are in the same level of indentation as the other properties
> of spi0? I did move the -cells down though now.

Sorry I didn't make it clear. This refers to -cells.

> hopefully caught all pwms now

The pinctrl-names of pwm4 to pwm15 are still "active".

> +		saradc0: adc@ff730000 {
> +			compatible = "rockchip,rk3562-saradc";
> +			reg = <0x0 0xff730000 0x0 0x100>;
> +			interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
> +			#io-channel-cells = <1>;

> > `#io-channel-cells` should be put above `status = "disabled";`
>
> moved now :-)

It looks like saradc0 forgot to change.

Thanks,
Chukun

--
2.25.1

Re: [PATCH v7 4/5] arm64: dts: rockchip: add core dtsi for RK3562 SoC

[Heiko Stübner]

Am Sonntag, 11. Mai 2025, 17:01:01 Mitteleuropäische Sommerzeit schrieb Chukun Pan:
> Hi,
> 
> > I might be blind, but I don't see a tab missing here? #adress-cells and
> > #size-cells are in the same level of indentation as the other properties
> > of spi0? I did move the -cells down though now.
> 
> Sorry I didn't make it clear. This refers to -cells.
> 
> > hopefully caught all pwms now
> 
> The pinctrl-names of pwm4 to pwm15 are still "active".
> 
> > +		saradc0: adc@ff730000 {
> > +			compatible = "rockchip,rk3562-saradc";
> > +			reg = <0x0 0xff730000 0x0 0x100>;
> > +			interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
> > +			#io-channel-cells = <1>;
> 
> > > `#io-channel-cells` should be put above `status = "disabled";`
> >
> > moved now :-)
> 
> It looks like saradc0 forgot to change.

adapted the rk3562 dtsi again, with those last remants :-)