Message-ID: <gcirox4uq33lbfthusrphuabvxc2jnnjfazuhuyhohwlnv2gnu@5trpjknqaivm>
Date: Fri, 23 May 2025 17:02:34 +0200
From: Uwe Kleine-König <ukleinek@...nel.org>
To: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
Cc: Linus Walleij <linus.walleij@...aro.org>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>, 
	William Breathitt Gray <wbg@...nel.org>, Sebastian Reichel <sebastian.reichel@...labora.com>, 
	Kever Yang <kever.yang@...k-chips.com>, linux-gpio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	linux-pwm@...r.kernel.org, linux-iio@...r.kernel.org, kernel@...labora.com, 
	Jonas Karlman <jonas@...boo.se>, Detlev Casanova <detlev.casanova@...labora.com>
Subject: Re: [PATCH 5/7] pwm: Add rockchip PWMv4 driver

[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@...labora.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@...ts.infradead.org
> > >  L:	linux-pwm@...r.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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists