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

[Jonas Karlman <jonas@kwiboo.se>]

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