Re: [PATCH v5 3/6] pwm: Add rockchip PWMv4 driver

Re: [PATCH v5 3/6] pwm: Add rockchip PWMv4 driver

[Damon Ding]

Hi Uwe,

On 4/26/2026 9:06 PM, Uwe Kleine-König wrote:
> Hello,
> 
> first of all thanks for your extensive testing, very appreciated.
> 
> On Sun, Apr 26, 2026 at 05:44:46PM +0800, Damon Ding wrote:
>> On 4/20/2026 9:52 PM, Nicolas Frattaroli wrote:
>>> The Rockchip RK3576 brings with it a new PWM IP, in downstream code
>>> referred to as "v4". This new IP is different enough from the previous
>>> Rockchip IP that I felt it necessary to add a new driver for it, instead
>>> of shoehorning it in the old one.
>>>
>>> Add this new driver, based on the PWM core's waveform APIs. Its platform
>>> device is registered by the parent mfpwm driver, from which it also
>>> receives a little platform data struct, so that mfpwm can guarantee that
>>> all the platform device drivers spread across different subsystems for
>>> this specific hardware IP do not interfere with each other.
>>>
>>> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
>> Tested-by: Damon Ding <damon.ding@rock-chips.com>
>>
>> The continuous mode of all PWM channels has been preliminarily tested
>> and verified working on the RK3576 IoT board.
>>
>> I have tested with several typical period and duty cycle configurations.
>>
>> Following Uwe's suggestion [0], I also tested with libpwm using commands
>> similar to the following:
>>
>> ./pwmset -c 0 -p 0 -P 1000000 -D 500000 -s 5000
> 
> This one is good if you have an oscilloscope (or something similar) to
> verify the output. Without that (or additionally) pwmtestperf creates a
> series of requests that in combination with PWM_DEBUG should uncover
> rounding errors in the .tohw and .fromhw callbacks.

Aha, this is my oversight. I did not elaborate on my verification 
methods. ;-)

I have adopted three ways to complete the test:

1. Measure the output waveform of each channel with a simple logic 
analyzer, Kinst LA2016.
2. Read relevant registers to verify that the configured period and duty 
values match expectations.
3. Connect the PWM continuous output pins to capture input pins 
directly, to confirm the capture measurement results.

> 
> A good set of calls then is:
> 
> 	pwmtestperf -p ... -c ... -P 50000 -S1
> 	pwmtestperf -p ... -c ... -P 50000 -S1 -I
> 
> 	pwmtestperf -p ... -c ... -P 50000 -S-1
> 	pwmtestperf -p ... -c ... -P 50000 -S-1 -I
> 
> (Assuming that 50000 is a sensible period for the device under test.)
> 

Okay, I will look into this tool and run these commands to conduct more 
in-depth verification.

> And yes, I know, I need to document that using something more permanent
> than a mailing list post.
> 

Best regards,
Damon

Re: [PATCH v5 6/6] arm64: dts: rockchip: Add cooling fan to ROCK 4D

[Nicolas Frattaroli]

Hello,

On Sunday, 26 April 2026 09:23:01 Central European Summer Time Damon Ding wrote:
> Hi Nicolas,
> 
> On 4/20/2026 9:52 PM, Nicolas Frattaroli wrote:
> > The ROCK 4D has a header to connect a small cooling fan. This fan is
> > driven by one of the SoC's PWM outputs driving a transistor, that in
> > turn controls the fan's power.
> > 
> > With the introduction of PWM support, add a description of this cooling
> > fan, as well as the additional trips and cooling-maps for it.
> > 
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> >   arch/arm64/boot/dts/rockchip/rk3576-rock-4d.dts | 50 +++++++++++++++++++++++++
> >   1 file changed, 50 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3576-rock-4d.dts b/arch/arm64/boot/dts/rockchip/rk3576-rock-4d.dts
> > index 899a84b1fbf9..2d5ede010ad0 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3576-rock-4d.dts
> > +++ b/arch/arm64/boot/dts/rockchip/rk3576-rock-4d.dts
> > @@ -45,6 +45,14 @@ rfkill {
> >   		shutdown-gpios = <&gpio2 RK_PD1 GPIO_ACTIVE_HIGH>;
> >   	};
> >   
> > +	fan: pwm-fan {
> > +		compatible = "pwm-fan";
> > +		cooling-levels = <0 180 205 230 255>;
> > +		fan-supply = <&vcc_5v0_sys>;
> > +		pwms = <&pwm2_8ch_5 0 60000 0>;
> > +		#cooling-cells = <2>;
> > +	};
> > +
> >   	leds: leds {
> >   		compatible = "gpio-leds";
> >   		pinctrl-names = "default";
> > @@ -711,6 +719,36 @@ rgmii_phy0: ethernet-phy@1 {
> >   	};
> >   };
> >   
> > +&package_thermal {
> > +	polling-delay = <100>;
> > +
> > +	trips {
> > +		package_fan0: package-fan0 {
> > +			temperature = <50000>;
> > +			hysteresis = <2000>;
> > +			type = "active";
> > +		};
> > +
> > +		package_fan1: package-fan1 {
> > +			temperature = <60000>;
> > +			hysteresis = <2000>;
> > +			type = "active";
> > +		};
> > +	};
> > +
> > +	cooling-maps {
> > +		map1 {
> > +			trip = <&package_fan0>;
> > +			cooling-device = <&fan THERMAL_NO_LIMIT 1>;
> > +		};
> > +
> > +		map2 {
> > +			trip = <&package_fan1>;
> > +			cooling-device = <&fan 2 THERMAL_NO_LIMIT>;
> > +		};
> > +	};
> > +};
> > +
> >   &pcie0 {
> >   	pinctrl-names = "default";
> >   	pinctrl-0 = <&pcie_reset>;
> > @@ -720,6 +758,13 @@ &pcie0 {
> >   };
> >   
> >   &pinctrl {
> > +	fan {
> > +		fan_pwm: fan-pwm {
> > +			rockchip,pins =
> > +				<4 RK_PC5 14 &pcfg_pull_down_drv_level_5>;
> > +		};
> > +	};
> > +
> >   	hym8563 {
> >   		hym8563_int: hym8563-int {
> >   			rockchip,pins = <0 RK_PA0 RK_FUNC_GPIO &pcfg_pull_up>;
> > @@ -770,6 +815,11 @@ wifi_en_h: wifi-en-h {
> >   	};
> >   };
> >   
> > +&pwm2_8ch_5 {
> > +	pinctrl-0 = <&fan_pwm>;
> 
> May I ask why the pinctrl does not directly use &pwm2m1_ch5?
> 
> Is it because the default pin configuration cannot meet the requirements 
> of the fan?

Yes, I'm adding a strong pull down. The PWM pin controls a transistor,
and if it floats when not in use the fan can start spinning when no
PWM signal is present. If I recall correctly (it's been a while since
I wrote this DT patch) it could be left spinning when the board was
shut down with a `poweroff` command.

> 
> > +	status = "okay";
> > +};
> > +
> >   &sai6 {
> >   	status = "okay";
> >   };
> > 
> 
> Best regards,
> Damon
> 
> 

Kind regards,
Nicolas Frattaroli

Re: [PATCH v5 4/6] counter: Add rockchip-pwm-capture driver

[Nicolas Frattaroli]

Hi Damon,

On Sunday, 26 April 2026 12:55:20 Central European Summer Time Damon Ding wrote:
> Hi Nicolas,
> 
> On 4/20/2026 9:52 PM, Nicolas Frattaroli wrote:
> > Among many other things, Rockchip's new PWMv4 IP in the RK3576 supports
> > PWM capture functionality.
> > 
> > Add a basic driver for this that works to expose HPC/LPC counts and
> > state change events to userspace through the counter framework. It's
> > quite basic, but works well enough to demonstrate the device function
> > exclusion stuff that mfpwm does, in order to eventually support all the
> > functions of this device in drivers within their appropriate subsystems,
> > without them interfering with each other.
> > 
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> >   MAINTAINERS                            |   1 +
> >   drivers/counter/Kconfig                |  11 ++
> >   drivers/counter/Makefile               |   1 +
> >   drivers/counter/rockchip-pwm-capture.c | 307 +++++++++++++++++++++++++++++++++
> >   4 files changed, 320 insertions(+)
> > 
> 
> For functional validation, I connected PWM0/PWM1 (continuous output)
> to PWM2 (capture input) pairwise.
> 
> I enabled the counter via:
> /sys/bus/counter/devices/counter0/count0/enable
> 
> Then I verified the functionality by reading the count values from:
> /sys/bus/counter/devices/counter0/count0/count
> /sys/bus/counter/devices/counter0/count1/count
> 
> Tested-by: Damon Ding <damon.ding@rock-chips.com>

Thanks for testing! To make sure a bug on the pwm output driver won't
be cancelled out by an equivalent bug in the counter driver, I did
my counter driver testing by using an RK3588 as the source of the
PWM signal, with the two boards sharing a common ground. Maybe I
should get a proper function generator. :)

> BTW: Is there any user-space test tool similar to libpwm for the
> counter subsystem?

I've tried looking for this as well, and couldn't find anything. If
it exists then adding a mention of it to generic-counter.rst would
be in order I think.

> 
> ......
> > diff --git a/drivers/counter/rockchip-pwm-capture.c b/drivers/counter/rockchip-pwm-capture.c
> > new file mode 100644
> > index 000000000000..09a92f2bc409
> > --- /dev/null
> > +++ b/drivers/counter/rockchip-pwm-capture.c
> > @@ -0,0 +1,307 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2025 Collabora Ltd.
> > + *
> > + * A counter driver for the Pulse-Width-Modulation (PWM) hardware found on
> > + * Rockchip SoCs such as the RK3576, internally referred to as "PWM v4". It
> > + * allows for measuring the high cycles and low cycles of a PWM signal through
> > + * the generic counter framework, while guaranteeing exclusive use over the
> > + * MFPWM device while the counter is enabled.
> > + *
> > + * Authors:
> > + *     Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > + */
> > +
> > +#include <linux/cleanup.h>
> > +#include <linux/counter.h>
> > +#include <linux/devm-helpers.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/mfd/rockchip-mfpwm.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/spinlock.h>
> > +
> > +#define RKPWMC_INT_MASK			(PWMV4_INT_LPC | PWMV4_INT_HPC)
> > +
> > +struct rockchip_pwm_capture {
> > +	struct rockchip_mfpwm_func *pwmf;
> > +	struct counter_device *counter;
> > +};
> > +
> > +static struct counter_signal rkpwmc_signals[] = {
> > +	{
> > +		.id = 0,
> > +		.name = "PWM Clock"
> > +	},
> > +};
> > +
> > +static const enum counter_synapse_action rkpwmc_hpc_lpc_actions[] = {
> > +	COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
> > +	COUNTER_SYNAPSE_ACTION_NONE,
> > +};
> 
> For the capture function, it uses the PWM's reference clock (dclk) as 
> the time base to measure how many reference cycles the high and low 
> levels of the input waveform last respectively.
> 
> I find it a bit strange to set COUNTER_SYNAPSE_ACTION_BOTH_EDGES for 
> counting. If we treat the input waveform as a sequence of square waves 
> sampled by dclk cycles, it feels like we should count on a single edge 
> (rising edge only) rather than both edges.
> 

Yeah, that's a good point. I think I've been struggling to wrap my head
around what the signal is and what the synapse should trigger on because
the signal isn't something exposed in this case.

Your explanation makes sense, and perhaps I should rename
rkpwmc_hpc_lpc_actions as well. Currently it makes it seem like HPC
and LPC are the signal, when actually the waveform is the signal, HPC
and LPC are the counts, and the synapse is a sample every dclk spotting
a transition if I understand this correctly.

> > +
> > +static struct counter_synapse rkpwmc_pwm_synapses[] = {
> > +	{
> > +		.actions_list = rkpwmc_hpc_lpc_actions,
> > +		.num_actions = ARRAY_SIZE(rkpwmc_hpc_lpc_actions),
> > +		.signal = &rkpwmc_signals[0]
> > +	},
> > +};
> > +
> 
> Best regards,
> Damon
> 

Kind regards,
Nicolas Frattaroli