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

Damon Ding
2026-04-26T10:10:13Z

Hi Nicolas，

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>
> ---
> MAINTAINERS | 1 +
> drivers/pwm/Kconfig | 11 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-rockchip-v4.c | 383 ++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 396 insertions(+)
>
......
> diff --git a/drivers/pwm/pwm-rockchip-v4.c b/drivers/pwm/pwm-rockchip-v4.c
> new file mode 100644
> index 000000000000..b7de72c433c5
> --- /dev/null
> +++ b/drivers/pwm/pwm-rockchip-v4.c
> @@ -0,0 +1,383 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2025 Collabora Ltd.
> + *
> + * A Pulse-Width-Modulation (PWM) generator driver for the generators found in
> + * Rockchip SoCs such as the RK3576, internally referred to as "PWM v4". Uses
> + * the MFPWM infrastructure to guarantee exclusive use over the device without
> + * other functions of the device from different drivers interfering with its
> + * operation while it's active.
> + *
> + * Technical Reference Manual: Chapter 31 of the RK3506 TRM Part 1, a SoC which
> + * uses the same PWM hardware and has a publicly available TRM.
> + * https://opensource.rock-chips.com/images/3/36/Rockchip_RK3506_TRM_Part_1_V1.2-20250811.pdf
> + *
> + * Authors:
> + * Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> + *
> + * Limitations:
> + * - The hardware supports both completing the currently running period
> + * on disable (by switching to oneshot mode with a single repetition and
> + * only disable when the complete irq fires), and abrupt disable (freeze).
> + * Only the latter is implemented in the driver.
> + * - When the output is disabled, the pin will remain driven to whatever state
> + * it last had.

This limitation exists because after disabling the PWM output via
registers, the actual shutdown only happens after the current period
completes.

Therefore, the better approach is to add a delay of one full period
before disabling &rockchip_mfpwm_func.core.

> + * - Adjustments to the duty cycle will only take effect during the next period.
> + * - Adjustments to the period length will only take effect during the next
> + * period.
> + * - The hardware only supports offsets in [0, period - duty_cycle]
> + */
> +
......
> +
> + if (wfhw->rate) {
> + if (!was_enabled) {
> + dev_dbg(&chip->dev, "Enabling PWM output\n");
> + ret = clk_enable(pc->pwmf->core);
> + if (ret)
> + goto err_mfpwm_release;
> + ret = clk_set_rate_exclusive(pc->pwmf->core, wfhw->rate);
> + if (ret) {
> + clk_disable(pc->pwmf->core);
> + goto err_mfpwm_release;
> + }
> +
> + /*
> + * Output should be on now, acquire device to guarantee
> + * exclusion with other device functions while it's on.
> + *
> + * It's highly unlikely that this fails, as mfpwm has
> + * already been acquired before, and this is just a
> + * usage counter increase. Not worth the added
> + * complexity of clearing the PWMV4_REG_ENABLE again,
> + * especially considering the CTRL_UPDATE_EN behaviour.
> + */
> + ret = mfpwm_acquire(pc->pwmf);
> + if (ret) {
> + clk_rate_exclusive_put(pc->pwmf->core);
> + clk_disable(pc->pwmf->core);
> + goto err_mfpwm_release;
> + }
> + }
> + } else if (was_enabled) {
> + dev_dbg(&chip->dev, "Disabling PWM output\n");

Delay for one full PWM period before disabling the dclk.

Although this may introduce some latency for disable -> re-enable
operations, it ensures that the state after shutdown aligns with the
actual polarity configuration.

> + clk_rate_exclusive_put(pc->pwmf->core);
> + clk_disable(pc->pwmf->core);
> + /* Output is off now, extra release to balance extra acquire */
> + mfpwm_release(pc->pwmf);
> + }
> +
> +err_mfpwm_release:
> + mfpwm_release(pc->pwmf);
> +
> + return ret;
> +}
> +
>

Best regards,
Damon