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