Re: [PATCH v2 2/3] pwm: rp1: Add RP1 PWM controller driver

["Uwe Kleine-König" <ukleinek@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 14571 bytes --]

Hello Andrea,

nice work for a v2!

On Fri, Apr 10, 2026 at 04:09:58PM +0200, Andrea della Porta wrote:
> From: Naushir Patuck <naush@raspberrypi.com>
> 
> The Raspberry Pi RP1 southbridge features an embedded PWM
> controller with 4 output channels, alongside an RPM interface
> to read the fan speed on the Raspberry Pi 5.
> 
> Add the supporting driver.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Co-developed-by: Stanimir Varbanov <svarbanov@suse.de>
> Signed-off-by: Stanimir Varbanov <svarbanov@suse.de>
> Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
> ---
>  drivers/pwm/Kconfig   |   9 ++
>  drivers/pwm/Makefile  |   1 +
>  drivers/pwm/pwm-rp1.c | 344 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 354 insertions(+)
>  create mode 100644 drivers/pwm/pwm-rp1.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 6f3147518376a..32031f2af75af 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -625,6 +625,15 @@ config PWM_ROCKCHIP
>  	  Generic PWM framework driver for the PWM controller found on
>  	  Rockchip SoCs.
>  
> +config PWM_RASPBERRYPI_RP1
> +	bool "RP1 PWM support"
> +	depends on MISC_RP1 || COMPILE_TEST
> +	depends on HAS_IOMEM
> +	select REGMAP_MMIO
> +	select MFD_SYSCON
> +	help
> +	  PWM framework driver for Raspberry Pi RP1 controller.
> +
>  config PWM_SAMSUNG
>  	tristate "Samsung PWM support"
>  	depends on PLAT_SAMSUNG || ARCH_S5PV210 || ARCH_EXYNOS || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 0dc0d2b69025d..59f29f60f9123 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -56,6 +56,7 @@ obj-$(CONFIG_PWM_RENESAS_RZG2L_GPT)	+= pwm-rzg2l-gpt.o
>  obj-$(CONFIG_PWM_RENESAS_RZ_MTU3)	+= pwm-rz-mtu3.o
>  obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
>  obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
> +obj-$(CONFIG_PWM_RASPBERRYPI_RP1)	+= pwm-rp1.o
>  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
>  obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o
>  obj-$(CONFIG_PWM_SL28CPLD)	+= pwm-sl28cpld.o
> diff --git a/drivers/pwm/pwm-rp1.c b/drivers/pwm/pwm-rp1.c
> new file mode 100644
> index 0000000000000..b88c697d9567e
> --- /dev/null
> +++ b/drivers/pwm/pwm-rp1.c
> @@ -0,0 +1,344 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * pwm-rp1.c
> + *
> + * Raspberry Pi RP1 PWM.
> + *
> + * Copyright © 2026 Raspberry Pi Ltd.
> + *
> + * Author: Naushir Patuck (naush@raspberrypi.com)
> + *
> + * Based on the pwm-bcm2835 driver by:
> + * Bart Tanghe <bart.tanghe@thomasmore.be>
> + *
> + * Datasheet: https://pip-assets.raspberrypi.com/categories/892-raspberry-pi-5/documents/RP-008370-DS-1-rp1-peripherals.pdf?disposition=inline
> + *
> + * Limitations:
> + * - Channels can be enabled/disabled and their duty cycle and period can
> + *   be updated glitchlessly. Update are synchronized with the next strobe
> + *   at the end of the current period of the respective channel, once the
> + *   update bit is set. The update flag is global, not per-channel.
> + * - Channels are phase-capable, but on RPi5, the firmware can use a channel
> + *   phase register to report the RPM of the fan connected to that PWM
> + *   channel. As a result, phase control will be ignored for now.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +
> +#define RP1_PWM_GLOBAL_CTRL	0x000
> +#define RP1_PWM_CHANNEL_CTRL(x)	(0x014 + ((x) * 0x10))
> +#define RP1_PWM_RANGE(x)	(0x018 + ((x) * 0x10))
> +#define RP1_PWM_PHASE(x)	(0x01C + ((x) * 0x10))
> +#define RP1_PWM_DUTY(x)		(0x020 + ((x) * 0x10))
> +
> +/* 8:FIFO_POP_MASK + 0:Trailing edge M/S modulation */
> +#define RP1_PWM_CHANNEL_DEFAULT		(BIT(8) + BIT(0))

Please add a #define for BIT(8) and then use that and
FIELD_PREP(RP1_PWM_MODE, RP1_PWM_MODE_SOMENICENAME) to define the
constant. Also I would define it below the register defines.

> +#define RP1_PWM_CHANNEL_ENABLE(x)	BIT(x)
> +#define RP1_PWM_POLARITY		BIT(3)
> +#define RP1_PWM_SET_UPDATE		BIT(31)
> +#define RP1_PWM_MODE_MASK		GENMASK(1, 0)

s/_MASK// please

It would be great if the bitfield's names started with the register
name.

> +
> +#define RP1_PWM_NUM_PWMS	4
> +
> +struct rp1_pwm {
> +	struct regmap	*regmap;
> +	struct clk	*clk;
> +	unsigned long	clk_rate;
> +	bool		clk_enabled;
> +};
> +
> +struct rp1_pwm_waveform {
> +	u32	period_ticks;
> +	u32	duty_ticks;
> +	bool	enabled;
> +	bool	inverted_polarity;
> +};
> +
> +static const struct regmap_config rp1_pwm_regmap_config = {
> +	.reg_bits    = 32,
> +	.val_bits    = 32,
> +	.reg_stride  = 4,
> +	.max_register = 0x60,

I'm not a fan of aligning the = in a struct, still more if it fails like
here. Please consistently align all =s, or even better, use a single
space before each =. (Same for the struct definitions above, but I won't
insist.)

> +};
> +
> +static void rp1_pwm_apply_config(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
> +	u32 value;
> +
> +	/* update the changed registers on the next strobe to avoid glitches */
> +	regmap_read(rp1->regmap, RP1_PWM_GLOBAL_CTRL, &value);
> +	value |= RP1_PWM_SET_UPDATE;
> +	regmap_write(rp1->regmap, RP1_PWM_GLOBAL_CTRL, value);

I assume there is a glitch if I update two channels and the old
configuration of the first channel ends while I'm in the middle of
configuring the second?

> +}
> +
> +static int rp1_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
> +
> +	/* init channel to reset defaults */
> +	regmap_write(rp1->regmap, RP1_PWM_CHANNEL_CTRL(pwm->hwpwm), RP1_PWM_CHANNEL_DEFAULT);
> +	return 0;
> +}
> +
> +static int rp1_pwm_round_waveform_tohw(struct pwm_chip *chip,
> +				       struct pwm_device *pwm,
> +				       const struct pwm_waveform *wf,
> +				       void *_wfhw)
> +{
> +	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
> +	struct rp1_pwm_waveform *wfhw = _wfhw;
> +	u64 clk_rate = rp1->clk_rate;
> +	u64 ticks;

	if (!wf->period_length_ns)
		wfhw->enabled = false
		return 0;

> +	ticks = mul_u64_u64_div_u64(wf->period_length_ns, clk_rate, NSEC_PER_SEC);

To ensure this doesn't overflow please fail to probe the driver if
clk_rate > 1 GHz with an explaining comment. (Or alternatively calculate
the length of period_ticks = U32_MAX and skip the calculation if
wf->period_length_ns is bigger.)

> +	if (ticks > U32_MAX)
> +		ticks = U32_MAX;
> +	wfhw->period_ticks = ticks;

What happens if wf->period_length_ns > 0 but ticks == 0?

> +	if (wf->duty_offset_ns + wf->duty_length_ns >= wf->period_length_ns) {

The maybe surprising effect here is that in the two cases

	wf->duty_offset_ns == wf->period_length_ns and wf->duty_length_ns == 0

and
	
	wf->duty_length_ns == wf->period_length_ns and wf->duty_offset_ns == 0

you're configuring inverted polarity. I doesn't matter technically
because the result is the same, but for consumers still using pwm_state
this is irritating. That's why pwm-stm32 uses inverted polarity only if
also wf->duty_length_ns and wf->duty_offset_ns are non-zero.

> +		ticks = mul_u64_u64_div_u64(wf->period_length_ns - wf->duty_length_ns,
> +					    clk_rate, NSEC_PER_SEC);

The rounding is wrong here. You should pick the biggest duty_length not
bigger than wf->duty_length_ns, so you have to use

	ticks = wfhw->period_ticks - mul_u64_u64_div_u64(wf->duty_length_ns, clk_rate, NSEC_PER_SEC):

. I see this is a hole in the pwmtestperf coverage.

> +		wfhw->inverted_polarity = true;
> +	} else {
> +		ticks = mul_u64_u64_div_u64(wf->duty_length_ns, clk_rate, NSEC_PER_SEC);
> +		wfhw->inverted_polarity = false;
> +	}
> +
> +	if (ticks > wfhw->period_ticks)
> +		ticks = wfhw->period_ticks;

You can and should assume that wf->duty_length_ns <=
wf->period_length_ns. Then the if condition can never become true.

> +	wfhw->duty_ticks = ticks;
> +
> +	wfhw->enabled = !!wfhw->duty_ticks;
> +
> +	return 0;
> +}
> +
> +static int rp1_pwm_round_waveform_fromhw(struct pwm_chip *chip,
> +					 struct pwm_device *pwm,
> +					 const void *_wfhw,
> +					 struct pwm_waveform *wf)
> +{
> +	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
> +	const struct rp1_pwm_waveform *wfhw = _wfhw;
> +	u64 clk_rate = rp1->clk_rate;
> +	u32 ticks;
> +
> +	memset(wf, 0, sizeof(*wf));

	wf = (struct pwm_waveform){ };

is usually more efficient.

> +	if (!wfhw->enabled)
> +		return 0;
> +
> +	wf->period_length_ns = DIV_ROUND_UP_ULL((u64)wfhw->period_ticks * NSEC_PER_SEC, clk_rate);
> +
> +	if (wfhw->inverted_polarity) {
> +		wf->duty_length_ns = DIV_ROUND_UP_ULL((u64)wfhw->duty_ticks * NSEC_PER_SEC,
> +						      clk_rate);
> +	} else {
> +		wf->duty_offset_ns = DIV_ROUND_UP_ULL((u64)wfhw->duty_ticks * NSEC_PER_SEC,
> +						      clk_rate);
> +		ticks = wfhw->period_ticks - wfhw->duty_ticks;
> +		wf->duty_length_ns = DIV_ROUND_UP_ULL((u64)ticks * NSEC_PER_SEC, clk_rate);
> +	}

This needs adaption after the rounding issue in tohw is fixed.

> +	return 0;
> +}
> +
> +static int rp1_pwm_write_waveform(struct pwm_chip *chip,
> +				  struct pwm_device *pwm,
> +				  const void *_wfhw)
> +{
> +	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
> +	const struct rp1_pwm_waveform *wfhw = _wfhw;
> +	u32 value;
> +
> +	/* set period and duty cycle */
> +	regmap_write(rp1->regmap,
> +		     RP1_PWM_RANGE(pwm->hwpwm), wfhw->period_ticks);
> +	regmap_write(rp1->regmap,
> +		     RP1_PWM_DUTY(pwm->hwpwm), wfhw->duty_ticks);
> +
> +	/* set polarity */
> +	regmap_read(rp1->regmap, RP1_PWM_CHANNEL_CTRL(pwm->hwpwm), &value);
> +	if (!wfhw->inverted_polarity)
> +		value &= ~RP1_PWM_POLARITY;
> +	else
> +		value |= RP1_PWM_POLARITY;
> +	regmap_write(rp1->regmap, RP1_PWM_CHANNEL_CTRL(pwm->hwpwm), value);
> +
> +	/* enable/disable */
> +	regmap_read(rp1->regmap, RP1_PWM_GLOBAL_CTRL, &value);
> +	if (wfhw->enabled)
> +		value |= RP1_PWM_CHANNEL_ENABLE(pwm->hwpwm);
> +	else
> +		value &= ~RP1_PWM_CHANNEL_ENABLE(pwm->hwpwm);
> +	regmap_write(rp1->regmap, RP1_PWM_GLOBAL_CTRL, value);

You can exit early if wfhw->enabled is false after clearing the channel
enable bit.

> +	rp1_pwm_apply_config(chip, pwm);
> +
> +	return 0;
> +}
> +
> +static int rp1_pwm_read_waveform(struct pwm_chip *chip,
> +				 struct pwm_device *pwm,
> +				 void *_wfhw)
> +{
> +	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
> +	struct rp1_pwm_waveform *wfhw = _wfhw;
> +	u32 value;
> +
> +	regmap_read(rp1->regmap, RP1_PWM_GLOBAL_CTRL, &value);
> +	wfhw->enabled = !!(value & RP1_PWM_CHANNEL_ENABLE(pwm->hwpwm));
> +
> +	regmap_read(rp1->regmap, RP1_PWM_CHANNEL_CTRL(pwm->hwpwm), &value);
> +	wfhw->inverted_polarity = !!(value & RP1_PWM_POLARITY);
> +
> +	if (wfhw->enabled) {
> +		regmap_read(rp1->regmap, RP1_PWM_RANGE(pwm->hwpwm), &wfhw->period_ticks);
> +		regmap_read(rp1->regmap, RP1_PWM_DUTY(pwm->hwpwm), &wfhw->duty_ticks);
> +	} else {
> +		wfhw->period_ticks = 0;
> +		wfhw->duty_ticks = 0;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops rp1_pwm_ops = {
> +	.sizeof_wfhw = sizeof(struct rp1_pwm_waveform),
> +	.request = rp1_pwm_request,
> +	.round_waveform_tohw = rp1_pwm_round_waveform_tohw,
> +	.round_waveform_fromhw = rp1_pwm_round_waveform_fromhw,
> +	.read_waveform = rp1_pwm_read_waveform,
> +	.write_waveform = rp1_pwm_write_waveform,
> +};
> +
> +static int rp1_pwm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	unsigned long clk_rate;
> +	struct pwm_chip *chip;
> +	void __iomem	*base;
> +	struct rp1_pwm *rp1;
> +	int ret;
> +
> +	chip = devm_pwmchip_alloc(dev, RP1_PWM_NUM_PWMS, sizeof(*rp1));
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +
> +	rp1 = pwmchip_get_drvdata(chip);
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	rp1->regmap = devm_regmap_init_mmio(dev, base, &rp1_pwm_regmap_config);
> +	if (IS_ERR(rp1->regmap))
> +		return dev_err_probe(dev, PTR_ERR(rp1->regmap), "Cannot initialize regmap\n");
> +
> +	ret = of_syscon_register_regmap(np, rp1->regmap);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to register syscon\n");
> +
> +	rp1->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(rp1->clk))
> +		return dev_err_probe(dev, PTR_ERR(rp1->clk), "Clock not found\n");
> +
> +	ret = clk_prepare_enable(rp1->clk);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable clock\n");
> +	rp1->clk_enabled = true;
> +
> +	ret = devm_clk_rate_exclusive_get(dev, rp1->clk);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "Fail to get exclusive rate\n");

s/Fail/Failed/

> +		goto err_disable_clk;
> +	}
> +
> +	clk_rate = clk_get_rate(rp1->clk);
> +	if (!clk_rate) {
> +		ret = dev_err_probe(dev, -EINVAL, "Failed to get clock rate\n");
> +		goto err_disable_clk;
> +	}
> +	rp1->clk_rate = clk_rate;
> +
> +	chip->ops = &rp1_pwm_ops;
> +
> +	platform_set_drvdata(pdev, chip);
> +
> +	ret = devm_pwmchip_add(dev, chip);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "Failed to register PWM chip\n");
> +		goto err_disable_clk;
> +	}
> +
> +	return 0;
> +
> +err_disable_clk:
> +	clk_disable_unprepare(rp1->clk);
> +
> +	return ret;
> +}

On remove you miss to balance the call to clk_prepare_enable() (if no
failed call to clk_prepare_enable() in rp1_pwm_resume() happend).

> +
> +static int rp1_pwm_suspend(struct device *dev)
> +{
> +	struct rp1_pwm *rp1 = dev_get_drvdata(dev);
> +
> +	if (rp1->clk_enabled) {
> +		clk_disable_unprepare(rp1->clk);
> +		rp1->clk_enabled = false;
> +	}
> +
> +	return 0;
> +}
> +
> +static int rp1_pwm_resume(struct device *dev)
> +{
> +	struct rp1_pwm *rp1 = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = clk_prepare_enable(rp1->clk);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable clock on resume: %d\n", ret);

Please use %pe for error codes.

> +		return ret;
> +	}
> +
> +	rp1->clk_enabled = true;
> +
> +	return 0;
> +}

Best regards
Uwe

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