Re: [PATCH 5/7] pwm: Add rockchip PWMv4 driver

Uwe Kleine-König
2025-05-23T15:14:28Z

[-- Attachment #1.1: Type: text/plain, Size: 11763 bytes --]

[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:
> > On Tue, Apr 08, 2025 at 02:32:17PM +0200, 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 | 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.

> > > +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.)

> > > + 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?!

> > > + 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.

> > > + /*
> > > + * "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.

> > > + 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.

> > > +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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]