| 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
| ||
|
Message-ID: <0f0a35ba-d85d-49eb-92dd-7bc17358aeaf@kwiboo.se>
Date: Thu, 20 Mar 2025 08:02:16 +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 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@...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(+)
>>>
>>> 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@...28000 {
>>> status = "disabled";
>>> };
>>>
>>> + pwm0: pwm@...90000 {
>>> + 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@...90010 {
>>> + 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@...90020 {
>>> + 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@...90030 {
>>> + 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@...98000 {
>>> + 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@...98010 {
>>> + 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@...98020 {
>>> + 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@...98030 {
>>> + 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@...e0000 {
>>> compatible = "rockchip,rk3528-saradc";
>>> reg = <0x0 0xffae0000 0x0 0x10000>;
>>
>>
>
>
>
>
Powered by blists - more mailing lists