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

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

On Sunday, 3 May 2026 12:46:23 Central European Summer Time William Breathitt Gray wrote:
> On Mon, Apr 20, 2026 at 03:52:41PM +0200, 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>
> 
> Hi Nicolas,
> 
> Forgive me if I asked this before, but I'm having trouble finding it
> online: do you have a link to a publicly available RK357 technical
> reference manual? I think that will help me better understand how this
> PWMv4 IP works.

Hi!

The RK3576 TRM isn't public, but the same hardware is used in the
RK3506, which does have its TRM online from Rockchip themselves:
https://opensource.rock-chips.com/images/3/36/Rockchip_RK3506_TRM_Part_1_V1.2-20250811.pdf

See chapter 31 of this. Right now, this counter driver implements
what's briefly described in Chapter 31.3.1 "Capture Mode".

> 
> Regardless, some comments inline below from my current understanding.
> 
> > +static struct counter_signal rkpwmc_signals[] = {
> > +	{
> > +		.id = 0,
> > +		.name = "PWM Clock"
> > +	},
> > +};
> 
> If the capture mode is used to measure the duty cycle of the PWM input,
> then we actually have two Signals to define here: "PWM Clock" and "PWM
> Input".
> 
> I imagine the capture mode waveforms look something like this:
> 
>                __    __    __    __    __    __    __
>     clk     __|  |__|  |__|  |__|  |__|  |__|  |__|  |__
>     
>                       __________________ LPC: 2 edges ____
>     pwm_in  _________|   HPC: 3 edges   |____________|
> 
> So the level of the pwm_in signal is counted each rising edge of clk;
> the number of clk edges while pwm_in is high is the HPC count, whereas
> the number of clk edges while pwm_in is low is the LPC count.

Correct. In fact, your diagram is very close to what's in the
aforementioned chapter.

> 
> In the Generic Counter paradigm, this would look like the following:
> 
>     Signals             Synapses              Counts
>     =======             ========              ======
>     +-----------+                               _______________________
>     |           | <---- "Rising Edge" ---+---> /                       \
>     |"PWM Clock"|                        |     |"High Polarity Capture"|
>     |           |                    +---|---> \                       /
>     +-----------+                    |   |      -----------------------
>                                      |   |
>     +-----------+                    |   |      _______________________
>     |           |                    |   +---> /                       \
>     |"PWM Input"|                    |         | "Low Polarity Capture |
>     |           | <---- "None" ------+-------> \                       /
>     +-----------+                               -----------------------
> 
> The key idea is that the clock and PWM input Signals are associated to
> both Counts (HPC and LPC) through their respective Synapses. However,
> while the clock Synapse ("Rising Edge") indicates the clock Signal
> state triggers updates in the Counts, the PWM input Synapse ("None")
> indicates the PWM input Signal state does not trigger but is simply
> evaluated to determine the new Count value.
> 
> > +static const enum counter_synapse_action rkpwmc_hpc_lpc_actions[] = {
> > +	COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
> > +	COUNTER_SYNAPSE_ACTION_NONE,
> > +};
> 
> To simplify my above example, I assumed that the HPC and LPC counts are
> only updated on rising edges of the clock Signal. If the Count values
> actually update on both edges, then you don't need to make a change
> here. Otherwise, change the "both edges" enum constant to "rising edge"
> or "falling edge" as required.

I checked the TRM and couldn't see any word on whether the count
is updated on the clock's rising edge only, but judging by Damon
Ding's review, I assume that is the case. I'll modify it to
reflect that.

> > +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]
> > +	},
> > +};
> 
> Add a Synapse here for the "PWM Input" Signal.
> 
> You should also implement an action_read() callback. Check the value of
> synapse->signal->id to determine which Signal is associated to the
> Synapse and set the respective action; i.e. for the PWM clock set
> COUNTER_SYNAPSE_ACTION_BOTH_EDGES, while for the PWM input set
> COUNTER_SYNAPSE_ACTION_NONE.

Will do!

> 
> > +static const enum counter_function rkpwmc_functions[] = {
> > +	COUNTER_FUNCTION_INCREASE,
> > +};
> 
> I wonder if we need a new enum counter_function constant to express
> what's happening in this driver. In theory, the
> COUNTER_FUNCTION_INCREASE represent a Count whose value only increases,
> but what we're describing here is more of a duty cycle sample, right?

Yeah, that's a great point. I think we can say it "increases",
in that either HPC or LPC are increased by the pwm_clk signal
based on the value of the pwm_in signal by the hardware.

But of course, the two counts are limited by the full period
of the PWM waveform. So reading a counter will give the
reader a snapshot view of the high cycles vs. low cycles of
the last observed PWM waveform period, which is both a duty
cycle sample and a period length sample, as the counts are
in clock cycles, so if we add the counts together we get the
PWM signal period in clock cycles.

This is also why you saw me convert counts to nanoseconds in a
previous revision; it felt like I should decouple it from the clock,
but I think the can of worms that this opens isn't worth it.

> 
> I haven't made up my mind on this, so for now you can stick with
> COUNTER_FUNCTION_INCREASE. I'll reconsider it in the next revision.
> 
> > +static int rkpwmc_enable_write(struct counter_device *counter,
> > +			       struct counter_count *count,
> > +			       u8 enable)
> > +{
> > +	struct rockchip_pwm_capture *pc = counter_priv(counter);
> > +	int ret;
> > +
> > +	ret = mfpwm_acquire(pc->pwmf);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!!enable != rkpwmc_is_enabled(pc->pwmf)) {
> 
> The Counter subsystem gurantees enable is a boolean value so there's no
> need for the double negation here in the conditional.
> 
> Also, instead of checking if the enable value is different from
> rkpwm_is_enabled(), check if it is the same and exit early. That will
> avoid the large conditional block as what you have inside can now move
> outside after the conditional check.

Good call, will do.

> 
> > +		if (enable) {
> > +			mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_ENABLE,
> > +					 PWMV4_EN(false));
> > +			mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_CTRL,
> > +					 PWMV4_CTRL_CAP_FLAGS);
> > +			mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_INT_EN,
> > +					 PWMV4_INT_LPC_W(true) |
> > +					 PWMV4_INT_HPC_W(true));
> > +			mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_ENABLE,
> > +					 PWMV4_EN(true) | PWMV4_CLK_EN(true));
> > +
> > +			ret = clk_enable(pc->pwmf->core);
> > +			if (ret)
> > +				goto err_release;
> > +
> > +			ret = clk_rate_exclusive_get(pc->pwmf->core);
> > +			if (ret)
> > +				goto err_disable_pwm_clk;
> > +
> > +			ret = mfpwm_acquire(pc->pwmf);
> > +			if (ret)
> > +				goto err_unprotect_pwm_clk;
> > +		} else {
> > +			mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_INT_EN,
> > +					 PWMV4_INT_LPC_W(false) |
> > +					 PWMV4_INT_HPC_W(false));
> > +			mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_ENABLE,
> > +					 PWMV4_EN(false) | PWMV4_CLK_EN(false));
> > +			clk_rate_exclusive_put(pc->pwmf->core);
> > +			clk_disable(pc->pwmf->core);
> > +			mfpwm_release(pc->pwmf);
> > +		}
> > +	}
> > +
> > +	mfpwm_release(pc->pwmf);
> 
> The call to mfpwm_release() in the else block is redundant because it is
> called immediately again here.

It's actually intentional, and not redundant. The if (enable) branch
does an mfpwm_acquire() so that as long as the counter functionality
is enabled, the use count remains > 0. This way, the PWM output
driver can't tell the mfpwm driver that it'd like to have exclusive
use of the PWM device now, as the counter driver is claiming it.

To balance this, the else block here needs to release it.

The release outside the else block is to balance the acquire
earlier in the function, the pair of which is there to ensure
mfpwm has powered on the hardware and associated clocks for
the register reads and writes.

Admittedly, I could get rid of the additional mfpwm_acquire
in the if (enable) branch by letting the function entry acquire
"leak" on purpose, by getting rid of the function end mfpwm_release.
But this feels less obvious to me than the current way. I'll
definitely add comments though to indicate what's going on, and
maybe even define a cleanup.h guard() class for the function
level acquire-release.

Thanks for the review, and especially the explanations.

Kind regards,
Nicolas Frattaroli

> 
> William Breathitt Gray
> 





_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip