[PATCH v2 0/3] Add RP1 PWM controller support

[PATCH v2 0/3] Add RP1 PWM controller support

[Andrea della Porta]

This patchset adds support for the PWM controller found on the
Raspberry Pi RP1 southbridge. This is necessary to operate the
cooling fan connected to one of the PWM channels.

The tachometer pin for the fan speed is managed by the firmware 
running on the RP1's M-core. It uses the PHASE2 register
to report the RPM, which is then exported by this driver via
syscon registers. A subsequent patch will add a new device
and driver to read the RPM and export this value via hwmon.
 
Subsequent patches will also add the CPU thermal zone, which
acts as a consumer of the PWM device.

Best regards,
Andrea

CHANGES in V2:

- bindings: added syscon to the description
- bindings: changed additionalProperties to unevaluatedProperties
- dts: reordered pwm node to follow the unit address ordering
- Kconfig/Makefile: renamed config option to PWM_RASPBERRYPI_RP1
- Kconfig: added dependency for syscon/regmap
- driver: added 'Limitations' and 'Datasheet' paragraphs in top comment
- driver: all macros are now prefixed by RP1_PWM_
- driver: implemented waveform callbacks instead of legacy ones
- driver: dropped hwmon device registration for fan speed (this will be
  handled in a separate patch with its own driver reading the value via
  syscon)
- driver: added new regmap/syscon to export the registers.
- driver: added a comment in rp1_pwm_apply_config() to describe what it does
- driver: added a comment to rp1_pwm_request() to define the purpose of the
  last write
- driver: new clk_enabled flag to deal with the clock on suspend/resume path
- driver: clk_rate is now obtained during probe right after exclusive_get()
- driver/Kconfig: module is now static only and has suppress_bind_attr to
  avoid racing with syscon consumer drivers and with syscon unload issue

Naushir Patuck (2):
  dt-bindings: pwm: Add Raspberry Pi RP1 PWM controller
  pwm: rp1: Add RP1 PWM controller driver

Stanimir Varbanov (1):
  arm64: dts: broadcom: rpi-5: Add RP1 PWM node

 .../bindings/pwm/raspberrypi,rp1-pwm.yaml     |  54 +++
 .../boot/dts/broadcom/bcm2712-rpi-5-b.dts     |  12 +
 arch/arm64/boot/dts/broadcom/rp1-common.dtsi  |  10 +
 drivers/pwm/Kconfig                           |   9 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-rp1.c                         | 344 ++++++++++++++++++
 6 files changed, 430 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/raspberrypi,rp1-pwm.yaml
 create mode 100644 drivers/pwm/pwm-rp1.c

-- 
2.35.3

[PATCH v2 1/3] dt-bindings: pwm: Add Raspberry Pi RP1 PWM controller

[Andrea della Porta]

From: Naushir Patuck <naush@raspberrypi.com>

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

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>
---
 .../bindings/pwm/raspberrypi,rp1-pwm.yaml     | 54 +++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/raspberrypi,rp1-pwm.yaml

diff --git a/Documentation/devicetree/bindings/pwm/raspberrypi,rp1-pwm.yaml b/Documentation/devicetree/bindings/pwm/raspberrypi,rp1-pwm.yaml
new file mode 100644
index 0000000000000..6f8461d0454f7
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/raspberrypi,rp1-pwm.yaml
@@ -0,0 +1,54 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/raspberrypi,rp1-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Raspberry Pi RP1 PWM controller
+
+maintainers:
+  - Naushir Patuck <naush@raspberrypi.com>
+
+allOf:
+  - $ref: pwm.yaml#
+
+description: |
+  The PWM peripheral is a flexible waveform generator with a
+  variety of operational modes. It has the following features:
+   - four independent output channels
+   - 32-bit counter widths
+   - Seven output generation modes
+   - Optional per-channel output inversion
+   - Optional duty-cycle data FIFO with DMA support
+   - Optional sigma-delta noise shaping engine
+  Serves as a fan speed provider to other nodes for a PWM-connected
+  fan using shared registers (syscon).
+
+properties:
+  compatible:
+    const: raspberrypi,rp1-pwm
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 3
+
+required:
+  - compatible
+  - reg
+  - clocks
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    pwm@98000 {
+      compatible = "raspberrypi,rp1-pwm";
+      reg = <0x98000 0x100>;
+      clocks = <&rp1_clocks 17>;
+      #pwm-cells = <3>;
+    };
-- 
2.35.3

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

[Andrea della Porta]

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))
+#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)
+
+#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,
+};
+
+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);
+}
+
+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;
+
+	ticks = mul_u64_u64_div_u64(wf->period_length_ns, clk_rate, NSEC_PER_SEC);
+
+	if (ticks > U32_MAX)
+		ticks = U32_MAX;
+	wfhw->period_ticks = ticks;
+
+	if (wf->duty_offset_ns + wf->duty_length_ns >= wf->period_length_ns) {
+		ticks = mul_u64_u64_div_u64(wf->period_length_ns - wf->duty_length_ns,
+					    clk_rate, NSEC_PER_SEC);
+		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;
+	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));
+
+	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);
+	}
+
+	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);
+
+	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");
+		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;
+}
+
+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);
+		return ret;
+	}
+
+	rp1->clk_enabled = true;
+
+	return 0;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(rp1_pwm_pm_ops, rp1_pwm_suspend, rp1_pwm_resume);
+
+static const struct of_device_id rp1_pwm_of_match[] = {
+	{ .compatible = "raspberrypi,rp1-pwm" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rp1_pwm_of_match);
+
+static struct platform_driver rp1_pwm_driver = {
+	.probe = rp1_pwm_probe,
+	.driver = {
+		.name = "rp1-pwm",
+		.of_match_table = rp1_pwm_of_match,
+		.pm = pm_ptr(&rp1_pwm_pm_ops),
+		.suppress_bind_attrs = true,
+	},
+};
+module_platform_driver(rp1_pwm_driver);
+
+MODULE_DESCRIPTION("RP1 PWM driver");
+MODULE_AUTHOR("Naushir Patuck <naush@raspberrypi.com>");
+MODULE_AUTHOR("Andrea della Porta <andrea.porta@suse.com>");
+MODULE_LICENSE("GPL");
-- 
2.35.3

[PATCH v2 3/3] arm64: dts: broadcom: rpi-5: Add RP1 PWM node

[Andrea della Porta]

From: Stanimir Varbanov <svarbanov@suse.de>

The RP1 chipset used on the Raspberry Pi 5 features an integrated
PWM controller to drive the cooling fan.

Add the corresponding DT node for this PWM controller.

Signed-off-by: Stanimir Varbanov <svarbanov@suse.de>
Co-developed-by: Andrea della Porta <andrea.porta@suse.com>
Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
---
 arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts | 12 ++++++++++++
 arch/arm64/boot/dts/broadcom/rp1-common.dtsi     | 10 ++++++++++
 2 files changed, 22 insertions(+)

diff --git a/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts b/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts
index 2856082814462..a4e5ba23bf536 100644
--- a/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts
+++ b/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts
@@ -64,12 +64,24 @@ phy1: ethernet-phy@1 {
 };
 
 &rp1_gpio {
+	fan_pwm_default_state: fan-pwm-default-state {
+		function = "pwm1";
+		pins = "gpio45";
+		bias-pull-down;
+	};
+
 	usb_vbus_default_state: usb-vbus-default-state {
 		function = "vbus1";
 		groups = "vbus1";
 	};
 };
 
+&rp1_pwm {
+	pinctrl-0 = <&fan_pwm_default_state>;
+	pinctrl-names = "default";
+	status = "okay";
+};
+
 &rp1_usb0 {
 	pinctrl-0 = <&usb_vbus_default_state>;
 	pinctrl-names = "default";
diff --git a/arch/arm64/boot/dts/broadcom/rp1-common.dtsi b/arch/arm64/boot/dts/broadcom/rp1-common.dtsi
index 5a815c3797945..d0f4d6be75500 100644
--- a/arch/arm64/boot/dts/broadcom/rp1-common.dtsi
+++ b/arch/arm64/boot/dts/broadcom/rp1-common.dtsi
@@ -26,6 +26,16 @@ rp1_clocks: clocks@40018000 {
 				       <200000000>;  // RP1_CLK_SYS
 	};
 
+	rp1_pwm: pwm@4009c000 {
+		compatible = "raspberrypi,rp1-pwm";
+		reg = <0x00 0x4009c000  0x0 0x100>;
+		clocks = <&rp1_clocks RP1_CLK_PWM1>;
+		assigned-clocks = <&rp1_clocks RP1_CLK_PWM1>;
+		assigned-clock-rates = <50000000>;
+		#pwm-cells = <3>;
+		status = "disabled";
+	};
+
 	rp1_gpio: pinctrl@400d0000 {
 		compatible = "raspberrypi,rp1-gpio";
 		reg = <0x00 0x400d0000  0x0 0xc000>,
-- 
2.35.3

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

[Uwe Kleine-König]

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

Re: [PATCH v2 1/3] dt-bindings: pwm: Add Raspberry Pi RP1 PWM controller

[Krzysztof Kozlowski]

On Fri, Apr 10, 2026 at 04:09:57PM +0200, Andrea della Porta wrote:
> From: Naushir Patuck <naush@raspberrypi.com>
> 
> Add the devicetree binding documentation for the PWM
> controller found in the Raspberry Pi RP1 chipset.
> 
> 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>
> ---
>  .../bindings/pwm/raspberrypi,rp1-pwm.yaml     | 54 +++++++++++++++++++
>  1 file changed, 54 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/raspberrypi,rp1-pwm.yaml
> 

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

Best regards,
Krzysztof

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

[Andrea della Porta]

Hi Uwe,

On 19:31 Fri 10 Apr     , Uwe Kleine-König wrote:
> Hello Andrea,
> 
> nice work for a v2!

Thanks!

> 
> On Fri, Apr 10, 2026 at 04:09:58PM +0200, Andrea della Porta wrote:

<...snip...>

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

Ack.

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

Ack.

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

Let's use the single space.

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

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.

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

Ack.

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

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

Ack.

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

Ack.

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

Ack.

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

Ack.

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

Ack.

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

Ack.

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

<,...snip...>

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

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.

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

Ack.

Best regards,
Andrea

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

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

[Uwe Kleine-König]

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

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.

On Thu, Apr 16, 2026 at 12:30:43PM +0200, Andrea della Porta wrote:
> On 19:31 Fri 10 Apr     , Uwe Kleine-König wrote:
> > 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.

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

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?

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

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.

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

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

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

[Andrea della Porta]

Hi Uwe,

On 15:48 Thu 16 Apr     , Uwe Kleine-König wrote:
> 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.

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.

> 
> On Thu, Apr 16, 2026 at 12:30:43PM +0200, Andrea della Porta wrote:
> > On 19:31 Fri 10 Apr     , Uwe Kleine-König wrote:
> > > 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.

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.

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

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

Yep, I did already done when you sent that patch.

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

Many thanks,
Andrea

> 
> Best regards
> Uwe

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

[Uwe Kleine-König]

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

Hello Andrea,

On Fri, Apr 17, 2026 at 11:05:51AM +0200, Andrea della Porta wrote:
> On 15:48 Thu 16 Apr     , Uwe Kleine-König wrote:
> > 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.

Ah, then I missed something different than I thought :-)

> > On Thu, Apr 16, 2026 at 12:30:43PM +0200, Andrea della Porta wrote:
> > > On 19:31 Fri 10 Apr     , Uwe Kleine-König wrote:
> > > > 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.
> 
> 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.

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

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

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

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

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

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

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