Add RP1 PWM controller support

From: Naushir Patuck <[email protected]>

Add the devicetree binding documentation for the PWM controller found in the Raspberry Pi RP1 chipset.

Signed-off-by Naushir Patuck <[email protected]>

Co-developed-by Stanimir Varbanov <[email protected]>

Signed-off-by Stanimir Varbanov <[email protected]>

Signed-off-by Andrea della Porta <[email protected]>

---
 .../bindings/pwm/raspberrypi,rp1-pwm.yaml     | 54 +++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/raspberrypi,rp1-pwm.yaml

From: Naushir Patuck <[email protected]>

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 <[email protected]>

Co-developed-by Stanimir Varbanov <[email protected]>

Signed-off-by Stanimir Varbanov <[email protected]>

Signed-off-by Andrea della Porta <[email protected]>

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

drivers/pwm/Kconfig +9 -0

index 6f3147518376a..32031f2af75af 100644
@@ -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

drivers/pwm/Makefile +1 -0

index 0dc0d2b69025d..59f29f60f9123 100644
@@ -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

drivers/pwm/pwm-rp1.c new file +34 -0

new file mode 100644
index 0000000000000..b88c697d9567e
@@ -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 ([email protected])
+ *
+ * Based on the pwm-bcm2835 driver by:
+ * Bart Tanghe <[email protected]>
+ *
+ * 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>

drivers/pwm/pwm-rp1.c new file +12 -0

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

drivers/pwm/pwm-rp1.c new file +10 -0

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

drivers/pwm/pwm-rp1.c new file +8 -0

+	wfhw->duty_ticks = ticks;
+
+	wfhw->enabled = !!wfhw->duty_ticks;
+
+	return 0;
+}
+
+static int rp1_pwm_round_waveform_fromhw(struct pwm_chip *chip,

drivers/pwm/pwm-rp1.c new file +4 -0

+	if (!wfhw->enabled)
+		return 0;
+
+	wf->period_length_ns = DIV_ROUND_UP_ULL((u64)wfhw->period_ticks * NSEC_PER_SEC, clk_rate);

drivers/pwm/pwm-rp1.c new file +22 -0

+	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

drivers/pwm/pwm-rp1.c new file +69 -0

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

drivers/pwm/pwm-rp1.c new file +17 -0

+		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");

drivers/pwm/pwm-rp1.c new file +11 -0

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

Hello Andrea,

nice work for a v2!

Quoted 4 lines

drivers/pwm/pwm-rp1.c new file +4 -0

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

drivers/pwm/pwm-rp1.c line 44

Quoted 4 lines

drivers/pwm/pwm-rp1.c new file +4 -0

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

drivers/pwm/pwm-rp1.c line 48

Quoted 5 lines

drivers/pwm/pwm-rp1.c new file +4 -0

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

Quoted 20 lines

drivers/pwm/pwm-rp1.c new file +19 -0

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

drivers/pwm/pwm-rp1.c line 70

Quoted 3 lines

drivers/pwm/pwm-rp1.c new file +3 -0

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

drivers/pwm/pwm-rp1.c line 81

Quoted 12 lines

drivers/pwm/pwm-rp1.c new file +11 -0

+};
+
+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?

Quoted 11 lines

drivers/pwm/pwm-rp1.c new file +10 -0

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

drivers/pwm/pwm-rp1.c line 101

Quoted 10 lines

drivers/pwm/pwm-rp1.c new file +10 -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;

drivers/pwm/pwm-rp1.c line 103

Quoted 1 line

drivers/pwm/pwm-rp1.c new file +1 -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.)

drivers/pwm/pwm-rp1.c line 107

Quoted 4 lines

drivers/pwm/pwm-rp1.c new file +3 -0

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

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

drivers/pwm/pwm-rp1.c line 109

Quoted 2 lines

drivers/pwm/pwm-rp1.c new file +1 -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.

drivers/pwm/pwm-rp1.c line 111

Quoted 3 lines

drivers/pwm/pwm-rp1.c new file +2 -0

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

drivers/pwm/pwm-rp1.c line 119

Quoted 9 lines

drivers/pwm/pwm-rp1.c new file +8 -0

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

Quoted 9 lines

drivers/pwm/pwm-rp1.c new file +8 -0

+	wfhw->duty_ticks = ticks;
+
+	wfhw->enabled = !!wfhw->duty_ticks;
+
+	return 0;
+}
+
+static int rp1_pwm_round_waveform_fromhw(struct pwm_chip *chip,

drivers/pwm/pwm-rp1.c line 137

Quoted 10 lines

drivers/pwm/pwm-rp1.c new file +10 -0

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

Quoted 5 lines

drivers/pwm/pwm-rp1.c new file +4 -0

+	if (!wfhw->enabled)
+		return 0;
+
+	wf->period_length_ns = DIV_ROUND_UP_ULL((u64)wfhw->period_ticks * NSEC_PER_SEC, clk_rate);

drivers/pwm/pwm-rp1.c line 152

Quoted 10 lines

drivers/pwm/pwm-rp1.c new file +10 -0

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

Quoted 28 lines

drivers/pwm/pwm-rp1.c new file +27 -0

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

drivers/pwm/pwm-rp1.c line 185

Quoted 5 lines

drivers/pwm/pwm-rp1.c new file +5 -0

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

drivers/pwm/pwm-rp1.c line 191

Quoted 6 lines

drivers/pwm/pwm-rp1.c new file +5 -0

+	rp1_pwm_apply_config(chip, pwm);
+
+	return 0;
+}
+

Quoted 4 lines

drivers/pwm/pwm-rp1.c new file +4 -0

+	}
+
+	return 0;
+

drivers/pwm/pwm-rp1.c line 292

Quoted 5 lines

drivers/pwm/pwm-rp1.c new file +5 -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).

Quoted 16 lines

drivers/pwm/pwm-rp1.c new file +15 -0

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

drivers/pwm/pwm-rp1.c line 313

Quoted 6 lines

drivers/pwm/pwm-rp1.c new file +6 -0

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

drivers/pwm/pwm-rp1.c line 320

Quoted 8 lines

drivers/pwm/pwm-rp1.c new file +7 -0

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

Best regards
Uwe

Quoted 3 lines · Uwe Kleine-König

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?

The configuration registers are per-channel but the update flag is global. I don't have details of the hw insights, my best guess is that anything that you set in the registers before updating the flag will take effect, so there should be no glitches.

Quoted 5 lines

drivers/pwm/pwm-rp1.c line 107

Quoted 3 lines

drivers/pwm/pwm-rp1.c new file +3 -0

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

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

I've added a check, returning 1 to signal teh round-up, and a minimum tick of 1 in this case.

Quoted 6 lines

drivers/pwm/pwm-rp1.c line 109

Quoted 1 line

drivers/pwm/pwm-rp1.c new file +1 -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

Quoted 8 lines

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.

Quoted 4 lines

Quoted 4 lines

drivers/pwm/pwm-rp1.c new file +4 -0

+	}
+
+	return 0;
+

Quoted 8 lines

drivers/pwm/pwm-rp1.c line 292

Quoted 5 lines

drivers/pwm/pwm-rp1.c new file +5 -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).

Since this driver now exports a syscon, it's only builtin (=Y) so it cannot be unloaded.
I've also avoided the .remove callback via .suppress_bind_attrs.

Hello Andrea,

one thing I forgot to ask: Is there a public reference manual covering the hardware. If yes, please add a link at the top of the driver.

Quoted 9 lines · Uwe Kleine-König

Quoted 3 lines

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?

The configuration registers are per-channel but the update flag is global. I don't have details of the hw insights, my best guess is that anything that you set in the registers before updating the flag will take effect, so there should be no glitches.

Would be great if you could test that. (Something along the lines of:
configure a very short period and wait a bit to be sure the short configuration is active. Configure something with a long period and wait shortly to be sure that the long period started, then change the duty, toggle the update bit and modify a 2nd channel without toggling update again. Then check the output of the 2nd channel after the first channel's period ended.

Quoted 7 lines

Quoted 6 lines

drivers/pwm/pwm-rp1.c line 107

Quoted 4 lines

drivers/pwm/pwm-rp1.c new file +3 -0

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

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

Quoted 2 lines

I've added a check, returning 1 to signal teh round-up, and a minimum tick of 1 in this case.

Sounds good. Are you able to verify that there is no +1 missing in the calculation, e.g. using 1 as register value really gives you a period of 1 tick and not 2?

Quoted 11 lines

Quoted 11 lines

drivers/pwm/pwm-rp1.c line 109

Quoted 2 lines

drivers/pwm/pwm-rp1.c new file +1 -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

Quoted 4 lines

Quoted 4 lines

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.

Please align to the pwm-stm32 algorithm (as of
https://patch.msgid.link/c5e7767cee821b5f6e00f95bd14a5e13015646fb.1776264104.git.u.kleine-koenig@baylibre.com)
here to decide when to select inverted polarity.

Quoted 14 lines

Quoted 13 lines

drivers/pwm/pwm-rp1.c line 292

Quoted 10 lines

drivers/pwm/pwm-rp1.c new file +9 -0

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

Quoted 3 lines

Since this driver now exports a syscon, it's only builtin (=Y) so it cannot be unloaded.
I've also avoided the .remove callback via .suppress_bind_attrs.

Oh no, please work cleanly here and make the driver unbindable. This yields better code quality and also helps during development and debugging.

Best regards
Uwe

Quoted 2 lines · Uwe Kleine-König

one thing I forgot to ask: Is there a public reference manual covering the hardware. If yes, please add a link at the top of the driver.

Sort of, it's already reported in this driver top comment (Datasheet: tag). The PWM controller is part of the RP1 chipset and you can find its description under the PWM section. This is not a full-fledged datasheet but the registers for the controller are somewhow documented.

Quoted 11 lines · Andrea della Porta

Quoted 9 lines · Uwe Kleine-König

Quoted 3 lines

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?

The configuration registers are per-channel but the update flag is global. I don't have details of the hw insights, my best guess is that anything that you set in the registers before updating the flag will take effect, so there should be no glitches.

Quoted 7 lines

Would be great if you could test that. (Something along the lines of:
configure a very short period and wait a bit to be sure the short configuration is active. Configure something with a long period and wait shortly to be sure that the long period started, then change the duty, toggle the update bit and modify a 2nd channel without toggling update again. Then check the output of the 2nd channel after the first channel's period ended.

I stand corrected here: after some more investigation it seems that only the enable/disable (plus osme other not currently used registers) depends on the global update flag, while the period and duty per-channel registers are independtly updatable while they are latched on the end of (specific channel) period strobe.
I'd say that this should avoid any cross-channel glitches since they are managed independently. Unfortunately I'm not able to test this with my current (and rather old) equipment, this would require at least an external trigger channel. Regarding the setup of a new value exactly during the strobe: I think this is quite hard to achieve.

Quoted 9 lines

Quoted 8 lines

Quoted 5 lines

drivers/pwm/pwm-rp1.c line 107

Quoted 3 lines

drivers/pwm/pwm-rp1.c new file +3 -0

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

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

I've added a check, returning 1 to signal teh round-up, and a minimum tick of 1 in this case.

Quoted 3 lines

Sounds good. Are you able to verify that there is no +1 missing in the calculation, e.g. using 1 as register value really gives you a period of 1 tick and not 2?

You are right. The scope reveals there's always one extra (low signal) tick at the end of each period.

Let's say that teh user want 10 tick period, we have to use
9 instead to account for the extra tick at the end, so that the complete period contains that extra tick?

This also means that if we ask for 100% duty cycle, the output waveform will have the high part of the signal lasting one tick less than expected.a I guess this is the accepted compromise.

OTOH, the minimum tick period would be 2 tick, less than that will otherwise degenerate in a disabled channel.

Quoted 10 lines

Quoted 6 lines

Quoted 2 lines

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

Since this driver now exports a syscon, it's only builtin (=Y) so it cannot be unloaded.
I've also avoided the .remove callback via .suppress_bind_attrs.

Oh no, please work cleanly here and make the driver unbindable. This yields better code quality and also helps during development and debugging.

I wish to, but the issue here is that this driver exports a syscon via of_syscon_register_regmap() which I think doesn't have the unregister counterpart. So the consumer will break in case we can unbind/unload the module and the syscon will leak.
If you have any alternative I'll be glad to discuss.

Hello Andrea,

Quoted 10 lines

I stand corrected here: after some more investigation it seems that only the enable/disable (plus osme other not currently used registers) depends on the global update flag, while the period and duty per-channel registers are independtly updatable while they are latched on the end of (specific channel) period strobe.
I'd say that this should avoid any cross-channel glitches since they are managed independently. Unfortunately I'm not able to test this with my current (and rather old) equipment, this would require at least an external trigger channel. Regarding the setup of a new value exactly during the strobe: I think this is quite hard to achieve.

To sum up: period and duty_cycle changes might result in glitches unless the channel is disabled. This is ok, please just document it.

The purpose of the update flag then is only to start several channels in sync?

What happens if sync is asserted while a disabled channel didn't complete the last period yet?

Maybe it's worth to test the following procedure for updating duty and period:

disable channel
configure duty
configure period
enable
set update flag

Assumint disable is delayed until the end of the currently running period, the effect of this procedure might be that no glitch happens if the update flag is asserted before the currently running period ends and the anormality is reduced to a longer inactive state if the updates are not that lucky (in contrast to more severe glitches).

If you can configure a short and a long period that is distinguishable "manually" with an LED I think this should be testable even without further equipment.

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

Quoted 5 lines

drivers/pwm/pwm-rp1.c line 107

Quoted 3 lines

drivers/pwm/pwm-rp1.c new file +3 -0

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

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

I've added a check, returning 1 to signal teh round-up, and a minimum tick of 1 in this case.

Quoted 6 lines

Quoted 3 lines

Sounds good. Are you able to verify that there is no +1 missing in the calculation, e.g. using 1 as register value really gives you a period of 1 tick and not 2?

You are right. The scope reveals there's always one extra (low signal) tick at the end of each period.

So the hardware cannot do 100% relative duty, right? Please document that.

Quoted 3 lines

Let's say that teh user want 10 tick period, we have to use
9 instead to account for the extra tick at the end, so that the complete period contains that extra tick?

I would describe that a bit differently, but in general: yes.

The more straight forward description is that setting

	RP1_PWM_RANGE(pwm->hwpwm) := x

results in a period of x + 1 ticks.

Quoted 4 lines

This also means that if we ask for 100% duty cycle, the output waveform will have the high part of the signal lasting one tick less than expected.a I guess this is the accepted compromise.

I assume you considered something like:

	RP1_PWM_RANGE(pwm->hwpwm) := 17
	RP1_PWM_DUTY(pwm->hwpwm) := 18

to get a 100% relative duty?

If this doesn't work that means that this has to be formalized in the callbacks. That is the fromhw function has to always report duty_length_ns less than period_length_ns.

Quoted 3 lines

OTOH, the minimum tick period would be 2 tick, less than that will otherwise degenerate in a disabled channel.

It's expected that in general for a period_length of 1 tick you can only have 0% and 100% relative duty. IIUC for this hardware you cannot do the 100% case so there is only a single valid duty_length for period_length = 1 tick.

I think it would be more complicated to consistently filter out period_length = 1 tick in the driver than to just accept the conceptual limitations. (Otherwise: What would you report in the fromhw callback if period_length = 1 tick is configured in wfhw? Would you refuse to commit that wfhw to hardware in .write_waveform()? The pwm core handles that just fine and consumers have all the means to detect and prevent that if they care enough.)

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

Quoted 2 lines

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

Since this driver now exports a syscon, it's only builtin (=Y) so it cannot be unloaded.
I've also avoided the .remove callback via .suppress_bind_attrs.

Oh no, please work cleanly here and make the driver unbindable. This yields better code quality and also helps during development and debugging.

Quoted 5 lines

I wish to, but the issue here is that this driver exports a syscon via of_syscon_register_regmap() which I think doesn't have the unregister counterpart. So the consumer will break in case we can unbind/unload the module and the syscon will leak.
If you have any alternative I'll be glad to discuss.

My (not so well articulated) point is: Please be stringent about clock handling to not bank up technical dept more than necessary and such that the driver can be made unbindable if and when syscons grow that feature. Optionally wail at the syscon guys :-)

Best regards
Uwe

Quoted 2 lines · Uwe Kleine-König

What happens if sync is asserted while a disabled channel didn't complete the last period yet?

The output stops immediately without waiting for the current period to finish.

Quoted 9 lines

Maybe it's worth to test the following procedure for updating duty and period:

disable channel
configure duty
configure period
enable
set update flag

Quoted 5 lines

Assumint disable is delayed until the end of the currently running period, the effect of this procedure might be that no glitch happens if the update flag is asserted before the currently running period ends and the anormality is reduced to a longer inactive state if the updates are not that lucky (in contrast to more severe glitches).

The disable isn't delayed as explained above. Setting just the new period/duty (which do not depend on the sync bit) correctly waits for the end of the current period without noticeable glitches (tested with a scope).

Quoted 6 lines

Quoted 3 lines

Let's say that teh user want 10 tick period, we have to use
9 instead to account for the extra tick at the end, so that the complete period contains that extra tick?

I would describe that a bit differently, but in general: yes.

Quoted 5 lines

The more straight forward description is that setting

	RP1_PWM_RANGE(pwm->hwpwm) := x

results in a period of x + 1 ticks.

Exactly. So whatever the user request I have to subtract one from the value to be written to the RANGE register.

Quoted 6 lines

Quoted 3 lines

This also means that if we ask for 100% duty cycle, the output waveform will have the high part of the signal lasting one tick less than expected.a I guess this is the accepted compromise.

I assume you considered something like:

Quoted 4 lines

	RP1_PWM_RANGE(pwm->hwpwm) := 17
	RP1_PWM_DUTY(pwm->hwpwm) := 18

to get a 100% relative duty?

Ah right! It's working fine and I've got 100% duty. So at hw register level the duty can be greater that the period.

Quoted 4 lines

My (not so well articulated) point is: Please be stringent about clock handling to not bank up technical dept more than necessary and such that the driver can be made unbindable if and when syscons grow that feature. Optionally wail at the syscon guys :-)

Hmmm not sure I've understood your point: is it a requirement that the driver must be unbindable? In this case I should avoid registering the syscon. Or should I just provide a .remove callback in case there will be a way to unregister the syscon (even if this callback will not be called as of now)?

Hello Andrea,

Quoted 5 lines · Andrea della Porta

Quoted 2 lines · Uwe Kleine-König

What happens if sync is asserted while a disabled channel didn't complete the last period yet?

The output stops immediately without waiting for the current period to finish.

This is a good info for the Limitations block.

Quoted 16 lines

Quoted 10 lines

Maybe it's worth to test the following procedure for updating duty and period:

disable channel
configure duty
configure period
enable
set update flag

Quoted 5 lines

Assumint disable is delayed until the end of the currently running period, the effect of this procedure might be that no glitch happens if the update flag is asserted before the currently running period ends and the anormality is reduced to a longer inactive state if the updates are not that lucky (in contrast to more severe glitches).

Quoted 3 lines

The disable isn't delayed as explained above. Setting just the new period/duty (which do not depend on the sync bit) correctly waits for the end of the current period without noticeable glitches (tested with a scope).

So if you happen to change both and one is done before the end of the current period and the other shortly afterwards (which might happen as those are configured in two different registers and the update trigger isn't used), you get a mixed output for one cycle, right? If yes, please also mention that in the Limitations paragraph.

Quoted 11 lines

Quoted 11 lines

Quoted 4 lines

Let's say that teh user want 10 tick period, we have to use
9 instead to account for the extra tick at the end, so that the complete period contains that extra tick?

I would describe that a bit differently, but in general: yes.

The more straight forward description is that setting

	RP1_PWM_RANGE(pwm->hwpwm) := x

Quoted 4 lines

Quoted 1 line

results in a period of x + 1 ticks.

Exactly. So whatever the user request I have to subtract one from the value to be written to the RANGE register.

Unless the calculation is already rounded to 0, in that case don't subtract 1 and let the tohw callback return 1.

Quoted 10 lines

Quoted 10 lines

Quoted 4 lines

This also means that if we ask for 100% duty cycle, the output waveform will have the high part of the signal lasting one tick less than expected.a I guess this is the accepted compromise.

I assume you considered something like:

	RP1_PWM_RANGE(pwm->hwpwm) := 17
	RP1_PWM_DUTY(pwm->hwpwm) := 18

Quoted 4 lines

Quoted 1 line

to get a 100% relative duty?

Ah right! It's working fine and I've got 100% duty. So at hw register level the duty can be greater that the period.

In that case please make sure to not use the maximal value for RP1_PWM_RANGE(pwm->hwpwm) to ensure that for each (possible) period length a 100% relative duty cycle can be configured.

Quoted 10 lines

Quoted 5 lines

My (not so well articulated) point is: Please be stringent about clock handling to not bank up technical dept more than necessary and such that the driver can be made unbindable if and when syscons grow that feature. Optionally wail at the syscon guys :-)

Hmmm not sure I've understood your point: is it a requirement that the driver must be unbindable? In this case I should avoid registering the syscon. Or should I just provide a .remove callback in case there will be a way to unregister the syscon (even if this callback will not be called as of now)?

It's a requirement to properly manage the resources you allocate. If a driver isn't unbindable due to restrictions of other subsystems that's unfortunate and I don't like it, but I wouldn't block a patch because of that.

Best regards
Uwe