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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <364bed4f-4ae1-48d5-ae6e-369668456d8e@kwiboo.se>
Date: Sun, 23 Mar 2025 23:03:46 +0100
From: Jonas Karlman <jonas@...boo.se>
To: Heiko Stuebner <heiko@...ech.de>, Chukun Pan <amadeus@....edu.cn>
Cc: Yao Zi <ziyao@...root.org>, Rob Herring <robh@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Krzysztof Kozlowski
 <krzk+dt@...nel.org>, linux-arm-kernel@...ts.infradead.org,
 linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org
Subject: Re: [PATCH v2 1/2] arm64: dts: rockchip: Add pwm nodes for RK3528

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@...90000 {
>>> 		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@...98000 {
>>> 		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@....edu.cn>
>>>> ---
>>>>  arch/arm64/boot/dts/rockchip/rk3528.dtsi | 80 ++++++++++++++++++++++++
>>>>  1 file changed, 80 insertions(+)
>>>>

[snip]