[PATCH v2 0/2] arm64: dts: rockchip: Add pwm nodes for RK3528

[PATCH v2 0/2] arm64: dts: rockchip: Add pwm nodes for RK3528

[Chukun Pan]

Add pwm nodes for RK3528. Most rk3528 boards use pwm-regulator to
supply to CPU, add node to enable them. The PWM core on RK3528 is
the same as RK3328, but the driver doesn't support interrupts yet.

Note that pwm regulator needs to be initialized in U-Boot:
```
&vdd_arm {
	regulator-init-microvolt = <953000>;
};

&vdd_logic {
	regulator-init-microvolt = <900000>;
};
```

Changes in v2:
  Remove merged bindings patch
  Remove pwm pinctrl in rk3528.dtsi
  Enable pwm regulator for Radxa E20C

Chukun Pan (2):
  arm64: dts: rockchip: Add pwm nodes for RK3528
  arm64: dts: rockchip: Enable regulators for Radxa E20C

 .../boot/dts/rockchip/rk3528-radxa-e20c.dts   | 72 +++++++++++++++++
 arch/arm64/boot/dts/rockchip/rk3528.dtsi      | 80 +++++++++++++++++++
 2 files changed, 152 insertions(+)

-- 
2.25.1

[PATCH v2 1/2] arm64: dts: rockchip: Add pwm nodes for RK3528

[Chukun Pan]

Add pwm nodes for RK3528. The PWM core on RK3528 is the same as
RK3328, but the driver does not support interrupts yet.

Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
---
 arch/arm64/boot/dts/rockchip/rk3528.dtsi | 80 ++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3528.dtsi b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
index 1af0d036cf32..621fc19ac0b3 100644
--- a/arch/arm64/boot/dts/rockchip/rk3528.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
@@ -465,6 +465,86 @@ uart7: serial@ffa28000 {
 			status = "disabled";
 		};
 
+		pwm0: pwm@ffa90000 {
+			compatible = "rockchip,rk3528-pwm",
+				     "rockchip,rk3328-pwm";
+			reg = <0x0 0xffa90000 0x0 0x10>;
+			clocks = <&cru CLK_PWM0>, <&cru PCLK_PWM0>;
+			clock-names = "pwm", "pclk";
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
+		pwm1: pwm@ffa90010 {
+			compatible = "rockchip,rk3528-pwm",
+				     "rockchip,rk3328-pwm";
+			reg = <0x0 0xffa90010 0x0 0x10>;
+			clocks = <&cru CLK_PWM0>, <&cru PCLK_PWM0>;
+			clock-names = "pwm", "pclk";
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
+		pwm2: pwm@ffa90020 {
+			compatible = "rockchip,rk3528-pwm",
+				     "rockchip,rk3328-pwm";
+			reg = <0x0 0xffa90020 0x0 0x10>;
+			clocks = <&cru CLK_PWM0>, <&cru PCLK_PWM0>;
+			clock-names = "pwm", "pclk";
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
+		pwm3: pwm@ffa90030 {
+			compatible = "rockchip,rk3528-pwm",
+				     "rockchip,rk3328-pwm";
+			reg = <0x0 0xffa90030 0x0 0x10>;
+			clocks = <&cru CLK_PWM0>, <&cru PCLK_PWM0>;
+			clock-names = "pwm", "pclk";
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
+		pwm4: pwm@ffa98000 {
+			compatible = "rockchip,rk3528-pwm",
+				     "rockchip,rk3328-pwm";
+			reg = <0x0 0xffa98000 0x0 0x10>;
+			clocks = <&cru CLK_PWM1>, <&cru PCLK_PWM1>;
+			clock-names = "pwm", "pclk";
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
+		pwm5: pwm@ffa98010 {
+			compatible = "rockchip,rk3528-pwm",
+				     "rockchip,rk3328-pwm";
+			reg = <0x0 0xffa98010 0x0 0x10>;
+			clocks = <&cru CLK_PWM1>, <&cru PCLK_PWM1>;
+			clock-names = "pwm", "pclk";
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
+		pwm6: pwm@ffa98020 {
+			compatible = "rockchip,rk3528-pwm",
+				     "rockchip,rk3328-pwm";
+			reg = <0x0 0xffa98020 0x0 0x10>;
+			clocks = <&cru CLK_PWM1>, <&cru PCLK_PWM1>;
+			clock-names = "pwm", "pclk";
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
+		pwm7: pwm@ffa98030 {
+			compatible = "rockchip,rk3528-pwm",
+				     "rockchip,rk3328-pwm";
+			reg = <0x0 0xffa98030 0x0 0x10>;
+			clocks = <&cru CLK_PWM1>, <&cru PCLK_PWM1>;
+			clock-names = "pwm", "pclk";
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
 		saradc: adc@ffae0000 {
 			compatible = "rockchip,rk3528-saradc";
 			reg = <0x0 0xffae0000 0x0 0x10000>;
-- 
2.25.1

Re: [PATCH v2 1/2] arm64: dts: rockchip: Add pwm nodes for RK3528

[Jonas Karlman]

Hi Chukun,

On 2025-03-18 13:00, Chukun Pan wrote:
> Add pwm nodes for RK3528. The PWM core on RK3528 is the same as
> RK3328, but the driver does not support interrupts yet.

The device tree should describe the hardware, not what the driver
support, so interrupts should probably be included.

However, looking closer at TRM for i.e. RK3328, RK3568 and RK3588 it
look like the following description is not a true description of the
hardware.

Each PWM controller seem to support 4 channels, here (and for older RK
SoCs) we instead describe each channel and not the controller.

Maybe something like following would better represent the hardware:

	pwm0: pwm@ffa90000 {
		compatible = "rockchip,rk3528-pwm";
		reg = <0x0 0xffa90000 0x0 0x10000>;
		clocks = <&cru CLK_PWM0>, <&cru PCLK_PWM0>;
		clock-names = "pwm", "pclk";
		interrupts = <GIC_SPI 53 IRQ_TYPE_LEVEL_HIGH>,
			     <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
	};

	pwm1: pwm@ffa98000 {
		compatible = "rockchip,rk3528-pwm";
		reg = <0x0 0xffa98000 0x0 0x10000>;
		clocks = <&cru CLK_PWM1>, <&cru PCLK_PWM1>;
		clock-names = "pwm", "pclk";
		interrupts = <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>,
			     <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
	};

Regards,
Jonas

> 
> Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
> ---
>  arch/arm64/boot/dts/rockchip/rk3528.dtsi | 80 ++++++++++++++++++++++++
>  1 file changed, 80 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3528.dtsi b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
> index 1af0d036cf32..621fc19ac0b3 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3528.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
> @@ -465,6 +465,86 @@ uart7: serial@ffa28000 {
>  			status = "disabled";
>  		};
>  
> +		pwm0: pwm@ffa90000 {
> +			compatible = "rockchip,rk3528-pwm",
> +				     "rockchip,rk3328-pwm";
> +			reg = <0x0 0xffa90000 0x0 0x10>;
> +			clocks = <&cru CLK_PWM0>, <&cru PCLK_PWM0>;
> +			clock-names = "pwm", "pclk";
> +			#pwm-cells = <3>;
> +			status = "disabled";
> +		};
> +
> +		pwm1: pwm@ffa90010 {
> +			compatible = "rockchip,rk3528-pwm",
> +				     "rockchip,rk3328-pwm";
> +			reg = <0x0 0xffa90010 0x0 0x10>;
> +			clocks = <&cru CLK_PWM0>, <&cru PCLK_PWM0>;
> +			clock-names = "pwm", "pclk";
> +			#pwm-cells = <3>;
> +			status = "disabled";
> +		};
> +
> +		pwm2: pwm@ffa90020 {
> +			compatible = "rockchip,rk3528-pwm",
> +				     "rockchip,rk3328-pwm";
> +			reg = <0x0 0xffa90020 0x0 0x10>;
> +			clocks = <&cru CLK_PWM0>, <&cru PCLK_PWM0>;
> +			clock-names = "pwm", "pclk";
> +			#pwm-cells = <3>;
> +			status = "disabled";
> +		};
> +
> +		pwm3: pwm@ffa90030 {
> +			compatible = "rockchip,rk3528-pwm",
> +				     "rockchip,rk3328-pwm";
> +			reg = <0x0 0xffa90030 0x0 0x10>;
> +			clocks = <&cru CLK_PWM0>, <&cru PCLK_PWM0>;
> +			clock-names = "pwm", "pclk";
> +			#pwm-cells = <3>;
> +			status = "disabled";
> +		};
> +
> +		pwm4: pwm@ffa98000 {
> +			compatible = "rockchip,rk3528-pwm",
> +				     "rockchip,rk3328-pwm";
> +			reg = <0x0 0xffa98000 0x0 0x10>;
> +			clocks = <&cru CLK_PWM1>, <&cru PCLK_PWM1>;
> +			clock-names = "pwm", "pclk";
> +			#pwm-cells = <3>;
> +			status = "disabled";
> +		};
> +
> +		pwm5: pwm@ffa98010 {
> +			compatible = "rockchip,rk3528-pwm",
> +				     "rockchip,rk3328-pwm";
> +			reg = <0x0 0xffa98010 0x0 0x10>;
> +			clocks = <&cru CLK_PWM1>, <&cru PCLK_PWM1>;
> +			clock-names = "pwm", "pclk";
> +			#pwm-cells = <3>;
> +			status = "disabled";
> +		};
> +
> +		pwm6: pwm@ffa98020 {
> +			compatible = "rockchip,rk3528-pwm",
> +				     "rockchip,rk3328-pwm";
> +			reg = <0x0 0xffa98020 0x0 0x10>;
> +			clocks = <&cru CLK_PWM1>, <&cru PCLK_PWM1>;
> +			clock-names = "pwm", "pclk";
> +			#pwm-cells = <3>;
> +			status = "disabled";
> +		};
> +
> +		pwm7: pwm@ffa98030 {
> +			compatible = "rockchip,rk3528-pwm",
> +				     "rockchip,rk3328-pwm";
> +			reg = <0x0 0xffa98030 0x0 0x10>;
> +			clocks = <&cru CLK_PWM1>, <&cru PCLK_PWM1>;
> +			clock-names = "pwm", "pclk";
> +			#pwm-cells = <3>;
> +			status = "disabled";
> +		};
> +
>  		saradc: adc@ffae0000 {
>  			compatible = "rockchip,rk3528-saradc";
>  			reg = <0x0 0xffae0000 0x0 0x10000>;

Re: [PATCH v2 1/2] arm64: dts: rockchip: Add pwm nodes for RK3528

[Heiko Stuebner]

Am Donnerstag, 20. März 2025, 00:26:14 MEZ schrieb Jonas Karlman:
> Hi Chukun,
> 
> On 2025-03-18 13:00, Chukun Pan wrote:
> > Add pwm nodes for RK3528. The PWM core on RK3528 is the same as
> > RK3328, but the driver does not support interrupts yet.
> 
> The device tree should describe the hardware, not what the driver
> support, so interrupts should probably be included.
> 
> However, looking closer at TRM for i.e. RK3328, RK3568 and RK3588 it
> look like the following description is not a true description of the
> hardware.
> 
> Each PWM controller seem to support 4 channels, here (and for older RK
> SoCs) we instead describe each channel and not the controller.

Yep, that is something that did go wrong in the very early days.
And all other Rockchip socs also have the same issue - even back
to the rk3066.

So yes, at some point we should overhaul the thing.

But I think this is more involved, as right now everything is aimed
at the current single-channel status quo.


Heiko


> Maybe something like following would better represent the hardware:
> 
> 	pwm0: pwm@ffa90000 {
> 		compatible = "rockchip,rk3528-pwm";
> 		reg = <0x0 0xffa90000 0x0 0x10000>;
> 		clocks = <&cru CLK_PWM0>, <&cru PCLK_PWM0>;
> 		clock-names = "pwm", "pclk";
> 		interrupts = <GIC_SPI 53 IRQ_TYPE_LEVEL_HIGH>,
> 			     <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
> 	};
> 
> 	pwm1: pwm@ffa98000 {
> 		compatible = "rockchip,rk3528-pwm";
> 		reg = <0x0 0xffa98000 0x0 0x10000>;
> 		clocks = <&cru CLK_PWM1>, <&cru PCLK_PWM1>;
> 		clock-names = "pwm", "pclk";
> 		interrupts = <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>,
> 			     <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
> 	};
> 
> Regards,
> Jonas
> 
> > 
> > Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
> > ---
> >  arch/arm64/boot/dts/rockchip/rk3528.dtsi | 80 ++++++++++++++++++++++++
> >  1 file changed, 80 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3528.dtsi b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
> > index 1af0d036cf32..621fc19ac0b3 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3528.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
> > @@ -465,6 +465,86 @@ uart7: serial@ffa28000 {
> >  			status = "disabled";
> >  		};
> >  
> > +		pwm0: pwm@ffa90000 {
> > +			compatible = "rockchip,rk3528-pwm",
> > +				     "rockchip,rk3328-pwm";
> > +			reg = <0x0 0xffa90000 0x0 0x10>;
> > +			clocks = <&cru CLK_PWM0>, <&cru PCLK_PWM0>;
> > +			clock-names = "pwm", "pclk";
> > +			#pwm-cells = <3>;
> > +			status = "disabled";
> > +		};
> > +
> > +		pwm1: pwm@ffa90010 {
> > +			compatible = "rockchip,rk3528-pwm",
> > +				     "rockchip,rk3328-pwm";
> > +			reg = <0x0 0xffa90010 0x0 0x10>;
> > +			clocks = <&cru CLK_PWM0>, <&cru PCLK_PWM0>;
> > +			clock-names = "pwm", "pclk";
> > +			#pwm-cells = <3>;
> > +			status = "disabled";
> > +		};
> > +
> > +		pwm2: pwm@ffa90020 {
> > +			compatible = "rockchip,rk3528-pwm",
> > +				     "rockchip,rk3328-pwm";
> > +			reg = <0x0 0xffa90020 0x0 0x10>;
> > +			clocks = <&cru CLK_PWM0>, <&cru PCLK_PWM0>;
> > +			clock-names = "pwm", "pclk";
> > +			#pwm-cells = <3>;
> > +			status = "disabled";
> > +		};
> > +
> > +		pwm3: pwm@ffa90030 {
> > +			compatible = "rockchip,rk3528-pwm",
> > +				     "rockchip,rk3328-pwm";
> > +			reg = <0x0 0xffa90030 0x0 0x10>;
> > +			clocks = <&cru CLK_PWM0>, <&cru PCLK_PWM0>;
> > +			clock-names = "pwm", "pclk";
> > +			#pwm-cells = <3>;
> > +			status = "disabled";
> > +		};
> > +
> > +		pwm4: pwm@ffa98000 {
> > +			compatible = "rockchip,rk3528-pwm",
> > +				     "rockchip,rk3328-pwm";
> > +			reg = <0x0 0xffa98000 0x0 0x10>;
> > +			clocks = <&cru CLK_PWM1>, <&cru PCLK_PWM1>;
> > +			clock-names = "pwm", "pclk";
> > +			#pwm-cells = <3>;
> > +			status = "disabled";
> > +		};
> > +
> > +		pwm5: pwm@ffa98010 {
> > +			compatible = "rockchip,rk3528-pwm",
> > +				     "rockchip,rk3328-pwm";
> > +			reg = <0x0 0xffa98010 0x0 0x10>;
> > +			clocks = <&cru CLK_PWM1>, <&cru PCLK_PWM1>;
> > +			clock-names = "pwm", "pclk";
> > +			#pwm-cells = <3>;
> > +			status = "disabled";
> > +		};
> > +
> > +		pwm6: pwm@ffa98020 {
> > +			compatible = "rockchip,rk3528-pwm",
> > +				     "rockchip,rk3328-pwm";
> > +			reg = <0x0 0xffa98020 0x0 0x10>;
> > +			clocks = <&cru CLK_PWM1>, <&cru PCLK_PWM1>;
> > +			clock-names = "pwm", "pclk";
> > +			#pwm-cells = <3>;
> > +			status = "disabled";
> > +		};
> > +
> > +		pwm7: pwm@ffa98030 {
> > +			compatible = "rockchip,rk3528-pwm",
> > +				     "rockchip,rk3328-pwm";
> > +			reg = <0x0 0xffa98030 0x0 0x10>;
> > +			clocks = <&cru CLK_PWM1>, <&cru PCLK_PWM1>;
> > +			clock-names = "pwm", "pclk";
> > +			#pwm-cells = <3>;
> > +			status = "disabled";
> > +		};
> > +
> >  		saradc: adc@ffae0000 {
> >  			compatible = "rockchip,rk3528-saradc";
> >  			reg = <0x0 0xffae0000 0x0 0x10000>;
> 
> 

Re: [PATCH v2 1/2] arm64: dts: rockchip: Add pwm nodes for RK3528

[Jonas Karlman]

On 2025-03-20 00:47, Heiko Stuebner wrote:
> Am Donnerstag, 20. März 2025, 00:26:14 MEZ schrieb Jonas Karlman:
>> Hi Chukun,
>>
>> On 2025-03-18 13:00, Chukun Pan wrote:
>>> Add pwm nodes for RK3528. The PWM core on RK3528 is the same as
>>> RK3328, but the driver does not support interrupts yet.
>>
>> The device tree should describe the hardware, not what the driver
>> support, so interrupts should probably be included.
>>
>> However, looking closer at TRM for i.e. RK3328, RK3568 and RK3588 it
>> look like the following description is not a true description of the
>> hardware.
>>
>> Each PWM controller seem to support 4 channels, here (and for older RK
>> SoCs) we instead describe each channel and not the controller.
> 
> Yep, that is something that did go wrong in the very early days.
> And all other Rockchip socs also have the same issue - even back
> to the rk3066.

I see, look like the PWM has evolved something like following:

- The controller has always been 1 controller for 4 channels
- Initial versions only had 2 regs for interrupt outside of the 4x
  0x10 reg space, one for each channel
- FIFO was introduced for channel 3
- Interrupts for FIFO was introduced (in RK3399 or earlier)
- Additional features/regs was introduced (in PX30 or earlier)
- PWM_VERSION_ID = 0x02120b34 is used (in RK3308 and later)

> 
> So yes, at some point we should overhaul the thing.
> 
> But I think this is more involved, as right now everything is aimed
> at the current single-channel status quo.

I did a quick and dirty change in driver to use npwms = 4, and that was
a rather trivial change, see top commit at [1]. Minimum required change
in U-Boot also look to be very trivial.

  cat /sys/kernel/debug/pwm
  0: platform/ffa90000.pwm, 4 PWM devices
   pwm-0   ((null)              ): period: 0 ns duty: 0 ns polarity: normal
   pwm-1   (regulator-vdd-arm   ): requested enabled period: 5000 ns duty: 2250 ns polarity: inverse
   pwm-2   (regulator-vdd-logic ): requested enabled period: 5000 ns duty: 2800 ns polarity: inverse
   pwm-3   ((null)              ): period: 0 ns duty: 0 ns polarity: normal

Not really seeing any reason not to describe these PWM controllers more
correctly, we need to start somewhere.

However, I have no idea on how to deal with historic and wrong bindings.
Driver could possible check if resource space is above 0x10 size and state
of PWM_VERSION_ID.

  PWM_VERSION_ID  0x005c  W  0x02120b34  PWM Version ID Register  

[1] https://github.com/Kwiboo/linux-rockchip/commits/next-20250319-rk3528/

Regards,
Jonas

> 
> 
> Heiko
> 
> 
>> Maybe something like following would better represent the hardware:
>>
>> 	pwm0: pwm@ffa90000 {
>> 		compatible = "rockchip,rk3528-pwm";
>> 		reg = <0x0 0xffa90000 0x0 0x10000>;
>> 		clocks = <&cru CLK_PWM0>, <&cru PCLK_PWM0>;
>> 		clock-names = "pwm", "pclk";
>> 		interrupts = <GIC_SPI 53 IRQ_TYPE_LEVEL_HIGH>,
>> 			     <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
>> 	};
>>
>> 	pwm1: pwm@ffa98000 {
>> 		compatible = "rockchip,rk3528-pwm";
>> 		reg = <0x0 0xffa98000 0x0 0x10000>;
>> 		clocks = <&cru CLK_PWM1>, <&cru PCLK_PWM1>;
>> 		clock-names = "pwm", "pclk";
>> 		interrupts = <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>,
>> 			     <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
>> 	};
>>
>> Regards,
>> Jonas
>>
>>>
>>> Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
>>> ---
>>>  arch/arm64/boot/dts/rockchip/rk3528.dtsi | 80 ++++++++++++++++++++++++
>>>  1 file changed, 80 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3528.dtsi b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
>>> index 1af0d036cf32..621fc19ac0b3 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3528.dtsi
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
>>> @@ -465,6 +465,86 @@ uart7: serial@ffa28000 {
>>>  			status = "disabled";
>>>  		};
>>>  
>>> +		pwm0: pwm@ffa90000 {
>>> +			compatible = "rockchip,rk3528-pwm",
>>> +				     "rockchip,rk3328-pwm";
>>> +			reg = <0x0 0xffa90000 0x0 0x10>;
>>> +			clocks = <&cru CLK_PWM0>, <&cru PCLK_PWM0>;
>>> +			clock-names = "pwm", "pclk";
>>> +			#pwm-cells = <3>;
>>> +			status = "disabled";
>>> +		};
>>> +
>>> +		pwm1: pwm@ffa90010 {
>>> +			compatible = "rockchip,rk3528-pwm",
>>> +				     "rockchip,rk3328-pwm";
>>> +			reg = <0x0 0xffa90010 0x0 0x10>;
>>> +			clocks = <&cru CLK_PWM0>, <&cru PCLK_PWM0>;
>>> +			clock-names = "pwm", "pclk";
>>> +			#pwm-cells = <3>;
>>> +			status = "disabled";
>>> +		};
>>> +
>>> +		pwm2: pwm@ffa90020 {
>>> +			compatible = "rockchip,rk3528-pwm",
>>> +				     "rockchip,rk3328-pwm";
>>> +			reg = <0x0 0xffa90020 0x0 0x10>;
>>> +			clocks = <&cru CLK_PWM0>, <&cru PCLK_PWM0>;
>>> +			clock-names = "pwm", "pclk";
>>> +			#pwm-cells = <3>;
>>> +			status = "disabled";
>>> +		};
>>> +
>>> +		pwm3: pwm@ffa90030 {
>>> +			compatible = "rockchip,rk3528-pwm",
>>> +				     "rockchip,rk3328-pwm";
>>> +			reg = <0x0 0xffa90030 0x0 0x10>;
>>> +			clocks = <&cru CLK_PWM0>, <&cru PCLK_PWM0>;
>>> +			clock-names = "pwm", "pclk";
>>> +			#pwm-cells = <3>;
>>> +			status = "disabled";
>>> +		};
>>> +
>>> +		pwm4: pwm@ffa98000 {
>>> +			compatible = "rockchip,rk3528-pwm",
>>> +				     "rockchip,rk3328-pwm";
>>> +			reg = <0x0 0xffa98000 0x0 0x10>;
>>> +			clocks = <&cru CLK_PWM1>, <&cru PCLK_PWM1>;
>>> +			clock-names = "pwm", "pclk";
>>> +			#pwm-cells = <3>;
>>> +			status = "disabled";
>>> +		};
>>> +
>>> +		pwm5: pwm@ffa98010 {
>>> +			compatible = "rockchip,rk3528-pwm",
>>> +				     "rockchip,rk3328-pwm";
>>> +			reg = <0x0 0xffa98010 0x0 0x10>;
>>> +			clocks = <&cru CLK_PWM1>, <&cru PCLK_PWM1>;
>>> +			clock-names = "pwm", "pclk";
>>> +			#pwm-cells = <3>;
>>> +			status = "disabled";
>>> +		};
>>> +
>>> +		pwm6: pwm@ffa98020 {
>>> +			compatible = "rockchip,rk3528-pwm",
>>> +				     "rockchip,rk3328-pwm";
>>> +			reg = <0x0 0xffa98020 0x0 0x10>;
>>> +			clocks = <&cru CLK_PWM1>, <&cru PCLK_PWM1>;
>>> +			clock-names = "pwm", "pclk";
>>> +			#pwm-cells = <3>;
>>> +			status = "disabled";
>>> +		};
>>> +
>>> +		pwm7: pwm@ffa98030 {
>>> +			compatible = "rockchip,rk3528-pwm",
>>> +				     "rockchip,rk3328-pwm";
>>> +			reg = <0x0 0xffa98030 0x0 0x10>;
>>> +			clocks = <&cru CLK_PWM1>, <&cru PCLK_PWM1>;
>>> +			clock-names = "pwm", "pclk";
>>> +			#pwm-cells = <3>;
>>> +			status = "disabled";
>>> +		};
>>> +
>>>  		saradc: adc@ffae0000 {
>>>  			compatible = "rockchip,rk3528-saradc";
>>>  			reg = <0x0 0xffae0000 0x0 0x10000>;
>>
>>
> 
> 
> 
> 

Re: [PATCH v2 1/2] arm64: dts: rockchip: Add pwm nodes for RK3528

[Jonas Karlman]

On 2025-03-20 08:02, Jonas Karlman wrote:
> On 2025-03-20 00:47, Heiko Stuebner wrote:
>> Am Donnerstag, 20. März 2025, 00:26:14 MEZ schrieb Jonas Karlman:
>>> Hi Chukun,
>>>
>>> On 2025-03-18 13:00, Chukun Pan wrote:
>>>> Add pwm nodes for RK3528. The PWM core on RK3528 is the same as
>>>> RK3328, but the driver does not support interrupts yet.
>>>
>>> The device tree should describe the hardware, not what the driver
>>> support, so interrupts should probably be included.
>>>
>>> However, looking closer at TRM for i.e. RK3328, RK3568 and RK3588 it
>>> look like the following description is not a true description of the
>>> hardware.
>>>
>>> Each PWM controller seem to support 4 channels, here (and for older RK
>>> SoCs) we instead describe each channel and not the controller.
>>
>> Yep, that is something that did go wrong in the very early days.
>> And all other Rockchip socs also have the same issue - even back
>> to the rk3066.
> 
> I see, look like the PWM has evolved something like following:
> 
> - The controller has always been 1 controller for 4 channels
> - Initial versions only had 2 regs for interrupt outside of the 4x
>   0x10 reg space, one for each channel
> - FIFO was introduced for channel 3
> - Interrupts for FIFO was introduced (in RK3399 or earlier)
> - Additional features/regs was introduced (in PX30 or earlier)
> - PWM_VERSION_ID = 0x02120b34 is used (in RK3308 and later)
> 
>>
>> So yes, at some point we should overhaul the thing.
>>
>> But I think this is more involved, as right now everything is aimed
>> at the current single-channel status quo.
> 
> I did a quick and dirty change in driver to use npwms = 4, and that was
> a rather trivial change, see top commit at [1]. Minimum required change
> in U-Boot also look to be very trivial.

I did a quick test on U-Boot and are preparing a series to add support
for multiple channels in U-Boot, see [2] for an early version that
should hit U-Boot ML in a day or two.

Found some other other issues when digging in the U-Boot pwm-regulator
code so ended up with a bigger series than anticipated, only the last
patch is related to preparation for multiple channels in rk-pwm driver.

[2] https://source.denx.de/u-boot/contributors/kwiboo/u-boot/-/commits/pwm-regulator

Regards,
Jonas

> 
>   cat /sys/kernel/debug/pwm
>   0: platform/ffa90000.pwm, 4 PWM devices
>    pwm-0   ((null)              ): period: 0 ns duty: 0 ns polarity: normal
>    pwm-1   (regulator-vdd-arm   ): requested enabled period: 5000 ns duty: 2250 ns polarity: inverse
>    pwm-2   (regulator-vdd-logic ): requested enabled period: 5000 ns duty: 2800 ns polarity: inverse
>    pwm-3   ((null)              ): period: 0 ns duty: 0 ns polarity: normal
> 
> Not really seeing any reason not to describe these PWM controllers more
> correctly, we need to start somewhere.
> 
> However, I have no idea on how to deal with historic and wrong bindings.
> Driver could possible check if resource space is above 0x10 size and state
> of PWM_VERSION_ID.
> 
>   PWM_VERSION_ID  0x005c  W  0x02120b34  PWM Version ID Register  
> 
> [1] https://github.com/Kwiboo/linux-rockchip/commits/next-20250319-rk3528/
> 
> Regards,
> Jonas
> 
>>
>>
>> Heiko
>>
>>
>>> Maybe something like following would better represent the hardware:
>>>
>>> 	pwm0: pwm@ffa90000 {
>>> 		compatible = "rockchip,rk3528-pwm";
>>> 		reg = <0x0 0xffa90000 0x0 0x10000>;
>>> 		clocks = <&cru CLK_PWM0>, <&cru PCLK_PWM0>;
>>> 		clock-names = "pwm", "pclk";
>>> 		interrupts = <GIC_SPI 53 IRQ_TYPE_LEVEL_HIGH>,
>>> 			     <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
>>> 	};
>>>
>>> 	pwm1: pwm@ffa98000 {
>>> 		compatible = "rockchip,rk3528-pwm";
>>> 		reg = <0x0 0xffa98000 0x0 0x10000>;
>>> 		clocks = <&cru CLK_PWM1>, <&cru PCLK_PWM1>;
>>> 		clock-names = "pwm", "pclk";
>>> 		interrupts = <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>,
>>> 			     <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
>>> 	};
>>>
>>> Regards,
>>> Jonas
>>>
>>>>
>>>> Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
>>>> ---
>>>>  arch/arm64/boot/dts/rockchip/rk3528.dtsi | 80 ++++++++++++++++++++++++
>>>>  1 file changed, 80 insertions(+)
>>>>

[snip]

Re: [PATCH v2 1/2] arm64: dts: rockchip: Add pwm nodes for RK3528

[Chukun Pan]

Hi,

> The device tree should describe the hardware, not what the driver
> support, so interrupts should probably be included.

Before I submitted, I noticed these two commits:

1. ARM: dts: rockchip: Drop interrupts property from pwm-rockchip nodes
https://github.com/torvalds/linux/commit/f98643d8daf3443e3b414a82d0cb3d745f8c8bbc

2. arm64: dts: rockchip: Drop interrupts property from rk3328 pwm-rockchip node
https://github.com/torvalds/linux/commit/1bbd894e2ae67faf52632bc9290ff926d9b741ea

So I removed the interrupts to avoid dtbs_check warnings.

Thanks,
Chukun

--
2.25.1

[PATCH v2 2/2] arm64: dts: rockchip: Enable regulators for Radxa E20C

[Chukun Pan]

Enable pwm and fixed regulators for Radxa E20C. The pwm regulator is
used to power the CPU and GPU. Note that the LPDDR4 voltage is 1.1V.

Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
---
 .../boot/dts/rockchip/rk3528-radxa-e20c.dts   | 72 +++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts b/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts
index 57a446b5cbd6..159b55aa970d 100644
--- a/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts
@@ -100,6 +100,16 @@ vcc_3v3: regulator-3v3-vcc {
 		vin-supply = <&vcc5v0_sys>;
 	};
 
+	vcc_ddr: regulator-vcc-ddr {
+		compatible = "regulator-fixed";
+		regulator-name = "vcc_ddr";
+		regulator-always-on;
+		regulator-boot-on;
+		regulator-min-microvolt = <1100000>;
+		regulator-max-microvolt = <1100000>;
+		vin-supply = <&vcc5v0_sys>;
+	};
+
 	vcc5v0_sys: regulator-5v0-vcc-sys {
 		compatible = "regulator-fixed";
 		regulator-name = "vcc5v0_sys";
@@ -108,6 +118,56 @@ vcc5v0_sys: regulator-5v0-vcc-sys {
 		regulator-min-microvolt = <5000000>;
 		regulator-max-microvolt = <5000000>;
 	};
+
+	vdd_0v9: regulator-0v9-vdd {
+		compatible = "regulator-fixed";
+		regulator-name = "vdd_0v9";
+		regulator-always-on;
+		regulator-boot-on;
+		regulator-min-microvolt = <900000>;
+		regulator-max-microvolt = <900000>;
+		vin-supply = <&vcc5v0_sys>;
+	};
+
+	vdd_arm: regulator-vdd-arm {
+		compatible = "pwm-regulator";
+		pwms = <&pwm1 0 5000 1>;
+		pwm-supply = <&vcc5v0_sys>;
+		regulator-name = "vdd_arm";
+		regulator-always-on;
+		regulator-boot-on;
+		regulator-min-microvolt = <746000>;
+		regulator-max-microvolt = <1201000>;
+		regulator-settling-time-up-us = <250>;
+	};
+
+	vdd_logic: regulator-vdd-logic {
+		compatible = "pwm-regulator";
+		pwms = <&pwm2 0 5000 1>;
+		pwm-supply = <&vcc5v0_sys>;
+		regulator-name = "vdd_logic";
+		regulator-always-on;
+		regulator-boot-on;
+		regulator-min-microvolt = <705000>;
+		regulator-max-microvolt = <1006000>;
+		regulator-settling-time-up-us = <250>;
+	};
+};
+
+&cpu0 {
+	cpu-supply = <&vdd_arm>;
+};
+
+&cpu1 {
+	cpu-supply = <&vdd_arm>;
+};
+
+&cpu2 {
+	cpu-supply = <&vdd_arm>;
+};
+
+&cpu3 {
+	cpu-supply = <&vdd_arm>;
 };
 
 &pinctrl {
@@ -132,6 +192,18 @@ wan_led_g: wan-led-g {
 	};
 };
 
+&pwm1 {
+	pinctrl-0 = <&pwm1m0_pins>;
+	pinctrl-names = "default";
+	status = "okay";
+};
+
+&pwm2 {
+	pinctrl-0 = <&pwm2m0_pins>;
+	pinctrl-names = "default";
+	status = "okay";
+};
+
 &saradc {
 	vref-supply = <&vcc_1v8>;
 	status = "okay";
-- 
2.25.1

Re: [PATCH v2 2/2] arm64: dts: rockchip: Enable regulators for Radxa E20C

[Jonas Karlman]

Hi Chukun,

On 2025-03-18 13:00, Chukun Pan wrote:
> Enable pwm and fixed regulators for Radxa E20C. The pwm regulator is
> used to power the CPU and GPU. Note that the LPDDR4 voltage is 1.1V.
> 
> Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
> ---
>  .../boot/dts/rockchip/rk3528-radxa-e20c.dts   | 72 +++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts b/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts
> index 57a446b5cbd6..159b55aa970d 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts
> @@ -100,6 +100,16 @@ vcc_3v3: regulator-3v3-vcc {
>  		vin-supply = <&vcc5v0_sys>;
>  	};
>  
> +	vcc_ddr: regulator-vcc-ddr {

Since we know the voltage for this regulator please name the node
regulator-1v1-vcc-ddr and place it above the regulator-1v8 to keep node
order by node name (not label name).

> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc_ddr";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <1100000>;
> +		regulator-max-microvolt = <1100000>;
> +		vin-supply = <&vcc5v0_sys>;
> +	};
> +
>  	vcc5v0_sys: regulator-5v0-vcc-sys {
>  		compatible = "regulator-fixed";
>  		regulator-name = "vcc5v0_sys";
> @@ -108,6 +118,56 @@ vcc5v0_sys: regulator-5v0-vcc-sys {
>  		regulator-min-microvolt = <5000000>;
>  		regulator-max-microvolt = <5000000>;
>  	};
> +
> +	vdd_0v9: regulator-0v9-vdd {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vdd_0v9";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <900000>;
> +		regulator-max-microvolt = <900000>;
> +		vin-supply = <&vcc5v0_sys>;
> +	};

This node is out of node name order, please place it before the
regulator-1v1-vcc-ddr node.

> +
> +	vdd_arm: regulator-vdd-arm {
> +		compatible = "pwm-regulator";

Suggest we add the pinctrl props here instead:

 		pinctrl-names = "default";
 		pinctrl-0 = <&pwm1m0_pins>;

> +		pwms = <&pwm1 0 5000 1>;

This should use the PWM_POLARITY_INVERTED flag not 1.

Will require include of dt-bindings/pwm/pwm.h.

> +		pwm-supply = <&vcc5v0_sys>;
> +		regulator-name = "vdd_arm";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <746000>;
> +		regulator-max-microvolt = <1201000>;
> +		regulator-settling-time-up-us = <250>;
> +	};
> +
> +	vdd_logic: regulator-vdd-logic {
> +		compatible = "pwm-regulator";

Same here, suggest we add the pinctrl props here instead:

 		pinctrl-names = "default";
 		pinctrl-0 = <&pwm2m0_pins>;

> +		pwms = <&pwm2 0 5000 1>;

Same here, this should use the PWM_POLARITY_INVERTED flag not 1.

> +		pwm-supply = <&vcc5v0_sys>;
> +		regulator-name = "vdd_logic";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <705000>;
> +		regulator-max-microvolt = <1006000>;
> +		regulator-settling-time-up-us = <250>;
> +	};
> +};
> +
> +&cpu0 {
> +	cpu-supply = <&vdd_arm>;
> +};
> +
> +&cpu1 {
> +	cpu-supply = <&vdd_arm>;
> +};
> +
> +&cpu2 {
> +	cpu-supply = <&vdd_arm>;
> +};
> +
> +&cpu3 {
> +	cpu-supply = <&vdd_arm>;
>  };
>  
>  &pinctrl {
> @@ -132,6 +192,18 @@ wan_led_g: wan-led-g {
>  	};
>  };
>  
> +&pwm1 {
> +	pinctrl-0 = <&pwm1m0_pins>;
> +	pinctrl-names = "default";

For consistency please put the pinctrl-names before the pinctrl-X props.
And as stated above, I suggest we move this to the regulator node.

> +	status = "okay";
> +};
> +
> +&pwm2 {
> +	pinctrl-0 = <&pwm2m0_pins>;
> +	pinctrl-names = "default";

Same here.

Regards,
Jonas

> +	status = "okay";
> +};
> +
>  &saradc {
>  	vref-supply = <&vcc_1v8>;
>  	status = "okay";