[Dropped Jonas Karlman from Cc as his email address doesn't work.]

Hello Nicolas,

On Thu, May 22, 2025 at 03:02:29PM +0200, Nicolas Frattaroli wrote:

On Tuesday, 13 May 2025 19:26:31 Central European Summer Time Uwe Kleine-König wrote:

quoted

On Tue, Apr 08, 2025 at 02:32:17PM +0200, Nicolas Frattaroli wrote:

quoted

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           |  13 ++
 drivers/pwm/Makefile          |   1 +
 drivers/pwm/pwm-rockchip-v4.c | 336 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 351 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index e6a9347be1e7889089e1d9e655cb23c2d8399b40..3ddd245fd4ad8d9ed2e762910a7a1f6436f93e34 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS

@@ -20891,6 +20891,7 @@ L:	linux-rockchip@lists.infradead.org
 L:	linux-pwm@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/pwm/rockchip,rk3576-pwm.yaml
+F:	drivers/pwm/pwm-rockchip-v4.c
 F:	drivers/soc/rockchip/mfpwm.c
 F:	include/soc/rockchip/mfpwm.h
 

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 4731d5b90d7edcc61138e4a5bf7e98906953ece4..242039f62ab091cea337bf27ef310bcf696b6ed0 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig

@@ -540,6 +540,19 @@ config PWM_ROCKCHIP
 	  Generic PWM framework driver for the PWM controller found on
 	  Rockchip SoCs.
 
+config PWM_ROCKCHIP_V4
+	tristate "Rockchip PWM v4 support"
+	depends on ARCH_ROCKCHIP || COMPILE_TEST
+	depends on ROCKCHIP_MFPWM
+	depends on HAS_IOMEM
+	help
+	  Generic PWM framework driver for the PWM controller found on
+	  later Rockchip SoCs such as the RK3576.
+
+	  Uses the Rockchip Multi-function PWM controller driver infrastructure
+	  to guarantee fearlessly concurrent operation with other functions of
+	  the same device implemented by drivers in other subsystems.
+
 config PWM_RZ_MTU3
 	tristate "Renesas RZ/G2L MTU3a PWM Timer support"
 	depends on RZ_MTU3

diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 539e0def3f82fcb866ab83a0346a15f7efdd7127..b5aca7ff58ac83f844581df526624617025291de 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile

@@ -49,6 +49,7 @@ obj-$(CONFIG_PWM_RASPBERRYPI_POE)	+= pwm-raspberrypi-poe.o
 obj-$(CONFIG_PWM_RCAR)		+= pwm-rcar.o
 obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
 obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
+obj-$(CONFIG_PWM_ROCKCHIP_V4)	+= pwm-rockchip-v4.o
 obj-$(CONFIG_PWM_RZ_MTU3)	+= pwm-rz-mtu3.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
 obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o

diff --git a/drivers/pwm/pwm-rockchip-v4.c b/drivers/pwm/pwm-rockchip-v4.c
new file mode 100644
index 0000000000000000000000000000000000000000..980b27454ef9b930bef0496ca528533cf419fa0e
--- /dev/null
+++ b/drivers/pwm/pwm-rockchip-v4.c

@@ -0,0 +1,336 @@
+// 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.

Can you please add a "Limitations" paragraph here similar to the other
newer drivers that explains how the hardware behave on disable
(inactive? High-Z? freeze?), if there are glitches possible when
settings are changed or if the currently running period is completed on
reconfiguration.

Will do. Might need a few long hours with the logic analyzer and poking at
the common clock framework to cover all bases.

Usually it's simpler. e.g. if you set period=1s,duty=0 and then
period=2s,duty=2 an LED is enough to determine if the current period is
completed before a change.

You don't find High-Z with an LED but can distinguish between "inactive
when off" and "freeze when off". The datasheet might know about High-Z.

Apropos datasheet: If that is publically available, a reference to it in
the driver's header would be awesome.

quoted

quoted

+static int rockchip_pwm_v4_round_wf_tohw(struct pwm_chip *chip,
+					 struct pwm_device *pwm,
+					 const struct pwm_waveform *wf,
+					 void *_wfhw)
+{
+	struct rockchip_pwm_v4 *pc = to_rockchip_pwm_v4(chip);
+	struct rockchip_pwm_v4_wf *wfhw = _wfhw;
+	unsigned long rate;
+	int ret = 0;
+
+	/* We do not want chosen_clk to change out from under us here */
+	ret = mfpwm_acquire(pc->pwmf);
+	if (ret)
+		return ret;
+
+	rate = mfpwm_clk_get_rate(pc->pwmf->parent);
+
+	ret = rockchip_pwm_v4_round_params(rate, wf->duty_length_ns,
+					   wf->period_length_ns,
+					   wf->duty_offset_ns, &wfhw->duty,
+					   &wfhw->period, &wfhw->offset);
+
+	if (wf->period_length_ns > 0)
+		wfhw->enable = PWMV4_EN_BOTH_MASK;
+	else
+		wfhw->enable = 0;
+
+	dev_dbg(&chip->dev, "tohw: duty = %u, period = %u, offset = %u, rate %lu\n",
+		wfhw->duty, wfhw->period, wfhw->offset, rate);
+
+	mfpwm_release(pc->pwmf);
+	return ret;

This is wrong. If a too high value for (say) period_length_ns is
requested, you're supposed to configure the maximal possible
period_length and not return a failure.

Ack. What if offset > period - duty is requested? Should I just saturate it
to period - duty in that case?

If you configure period = 10, duty = offset = 6 the period restart is
supposed to happen during the active phase, that is it looks as follows:

    __     _____     _____     _____     ____
      \___/     \___/     \___/     \___/    
    ^         ^         ^         ^         ^
    01234567890

('^' marks the period start.)

quoted

quoted

+	ret = mfpwm_acquire(pc->pwmf);
+	if (ret)
+		return ret;
+
+	rate = mfpwm_clk_get_rate(pc->pwmf->parent);

Why isn't that a proper clock that you can call clk_get_rate() (and
clk_rate_exclusive_get()) for?

Because I didn't know clk-mux.c existed :( But even with it, I'm not sure
if letting mfpwm function drivers touch the clk directly is a good idea,
as this either means storing it in their pwmf struct or making the members
of the mfpwm struct part of the shared header.

The different drivers shouldn't need to touch the clk directly, the clk
API functions should be enough?!
 

quoted

quoted

+	wfhw->period = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_PERIOD);
+	wfhw->duty = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_DUTY);
+	wfhw->offset = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_OFFSET);
+	wfhw->enable = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_ENABLE) & PWMV4_EN_BOTH_MASK;
+
+	mfpwm_release(pc->pwmf);
+
+	return 0;
+}
+
+static int rockchip_pwm_v4_write_wf(struct pwm_chip *chip, struct pwm_device *pwm,
+				    const void *_wfhw)
+{
+	struct rockchip_pwm_v4 *pc = to_rockchip_pwm_v4(chip);
+	const struct rockchip_pwm_v4_wf *wfhw = _wfhw;
+	bool was_enabled = false;
+	int ret = 0;
+
+	ret = mfpwm_acquire(pc->pwmf);
+	if (ret)
+		return ret;
+
+	was_enabled = pwmv4_is_enabled(mfpwm_reg_read(pc->pwmf->base,
+						      PWMV4_REG_ENABLE));

Just noticed: pwmv4_is_enabled has the wrong prefix. Please use
"rockchip_pwm_v4" consistently.

quoted

quoted

+	/*
+	 * "But Nicolas", you ask with valid concerns, "why would you enable the
+	 * PWM before setting all the parameter registers?"

Funny, I had this thought alread for mfpwm_acquire() above. Do you also
need that if wfhw->enable == 0?

The only thing mfpwm_acquire does is check that this driver is the only
one using the hardware, and enabling the bus clk so registers can be
accessed. The clock that the PWM signal is derived from, as well as
enabling and disabling PWM, is a separate step. In this case, we need
to have acquired mfpwm beforehand to do the reg read for was_enabled.

ok.

quoted

quoted

+	if (pwmv4_is_enabled(wfhw->enable)) {
+		if (!was_enabled) {
+			dev_dbg(&chip->dev, "enabling PWM output\n");
+			ret = mfpwm_pwmclk_enable(pc->pwmf);
+			if (ret)
+				goto err_mfpwm_release;
+
+			/*
+			 * Output should be on now, acquire device to guarantee
+			 * exclusion with other device functions while it's on.
+			 */
+			ret = mfpwm_acquire(pc->pwmf);
+			if (ret)
+				goto err_mfpwm_release;

Alternatively to calling mfpwm_acquire once more, you could also skip
the mfpwm_release() below. That makes the code a bit more complicated,
but also reduces the number of possible failing points.

I think I did this at some stage but it'd just necessitate duplicating the
if condition at the release point. The else-if branch just down below still
needs to exist since we need to not just balance this function's acquire,
but the fact that we kept it acquired when we turned it on so we need to
release it an extra time when we turn it off. I don't think changing this
to eliminate the additional acquire call has clear benefits here.

I'll keep that in mind and will try it maybe myself on top of v2.

quoted

quoted

+static int rockchip_pwm_v4_probe(struct platform_device *pdev)
+{
+	struct rockchip_mfpwm_func *pwmf = dev_get_platdata(&pdev->dev);
+	struct rockchip_pwm_v4 *pc;
+	struct pwm_chip *chip;
+	int ret;
+
+	chip = devm_pwmchip_alloc(&pdev->dev, 1, sizeof(*pc));
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+
+	pc = to_rockchip_pwm_v4(chip);
+	pc->pwmf = pwmf;
+
+	platform_set_drvdata(pdev, pc);
+
+	chip->ops = &rockchip_pwm_v4_ops;
+	chip->atomic = true;
+

If the PWM is already enabled you better call mfpwm_acquire() here?

As in, if the hardware set the PWM on before the driver probed? I hadn't
considered this case, and will need to think about it. Could very well be
a possibility as u-boot does things before us.

The typical application is that the bootloader already shows a splash
screen and then you don't want Linux booting result in a flashing
display.

Best regards
Uwe