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

[PATCH v9 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
a function. 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 V9:

All changes are in response of Sashiko's review (where applicable),
see:
https://lore.kernel.org/all/20260908132302.358D91F00ADB@smtp.kernel.org/#t

- linux/bitfield.h is now explicitly included to avoid compile
  time issue related to header refactory
- synchronized the exported function to read the tachometer with
  respect to both the system PM ops and the driver unbind/unload
- reworked the error path of all functions using regmap_read. As
  a consequence, rp1_pwm_apply_config() now returns and int for
  the error.
- made u32 cast explicit for the divisor of DIV_ROUND_UP_ULL
- moved teh struct initializer in rp1_pwm_read_waveform() to the
  top of the function to avoid overwriting part of the resulting
  data.
- dropped suppress_bind_attrs flag and replaced builtin_platform_driver()
  with module_platform_driver((), since the driver can now be
  unbound/unloaded.

Link to V8:
https://lore.kernel.org/all/cover.1788872294.git.andrea.porta@suse.com/


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     |  66 +++
 .../boot/dts/broadcom/bcm2712-rpi-5-b.dts     |  12 +
 arch/arm64/boot/dts/broadcom/rp1-common.dtsi  |   9 +
 drivers/pwm/Kconfig                           |   8 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-rp1.c                         | 499 ++++++++++++++++++
 include/soc/bcm2835/raspberrypi-pwm-rp1.h     |  10 +
 7 files changed, 605 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/raspberrypi,rp1-pwm.yaml
 create mode 100644 drivers/pwm/pwm-rp1.c
 create mode 100644 include/soc/bcm2835/raspberrypi-pwm-rp1.h

-- 
2.35.3

[PATCH v9 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>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
---
 .../bindings/pwm/raspberrypi,rp1-pwm.yaml     | 66 +++++++++++++++++++
 1 file changed, 66 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..be020583fd0ba
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/raspberrypi,rp1-pwm.yaml
@@ -0,0 +1,66 @@
+# 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>
+  - Andrea della Porta <andrea.porta@suse.com>
+  - Stanimir Varbanov <svarbanov@suse.de>
+
+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.
+
+properties:
+  compatible:
+    const: raspberrypi,rp1-pwm
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 3
+
+  dmas:
+    maxItems: 1
+    description: |
+      Phandle to the DMA controller and the channel specifier for
+      the duty-cycle FIFO.
+
+  dma-names:
+    items:
+      - const: tx
+
+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 v9 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                       |   8 +
 drivers/pwm/Makefile                      |   1 +
 drivers/pwm/pwm-rp1.c                     | 499 ++++++++++++++++++++++
 include/soc/bcm2835/raspberrypi-pwm-rp1.h |  10 +
 4 files changed, 518 insertions(+)
 create mode 100644 drivers/pwm/pwm-rp1.c
 create mode 100644 include/soc/bcm2835/raspberrypi-pwm-rp1.h

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 7297760868790..e1baec8d294d1 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -637,6 +637,14 @@ config PWM_ROCKCHIP
 	  Generic PWM framework driver for the PWM controller found on
 	  Rockchip SoCs.
 
+config PWM_RASPBERRYPI_RP1
+	tristate "RP1 PWM support"
+	depends on MISC_RP1 || COMPILE_TEST
+	depends on HAS_IOMEM
+	select REGMAP_MMIO
+	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 5630a521a7cff..c07fd24f69f39 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -57,6 +57,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..cfd38e46cc589
--- /dev/null
+++ b/drivers/pwm/pwm-rp1.c
@@ -0,0 +1,499 @@
+// 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 through a global update flag, while the
+ *   period and duty per-channel registers are independently updatable, and
+ *   they are latched on the end of (specific channel) period strobe.
+ *   This means that period and duty changes might result in glitches if the
+ *   period/duty is changed exactly during an end of period strobe.
+ * - Since the duty/period registers are freely updatable (do not depend on
+ *   the global update flag), setting one of them close to the period end and
+ *   the other right afterwards results in a mixed output for that cycle because
+ *   the write ops are not atomic.
+ * - The global update flag prevents mis-sampling of multi-bit bus signals in
+ *   the PWM clock domain. This ensures that all PWM channel settings update
+ *   on the same PWM clock cycle. Channels start in sync only if they share the
+ *   same period.
+ * - If both duty and period are set to 0, the output is a constant low signal
+ *   if polarity is normal or a constant high signal if polarity is inversed.
+ * - When disabled the output is driven to 0 if polarity is normal, or to 1
+ *   if polarity is inversed.
+ * - Disabling the PWM stops the output immediately, without waiting for current
+ *   period to complete first.
+ * - 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/bitfield.h>
+#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/units.h>
+#include <soc/bcm2835/raspberrypi-pwm-rp1.h>
+
+#define RP1_PWM_GLB_CTRL			0x000
+#define RP1_PWM_GLB_CTRL_CHANNEL_ENABLE(chan)	BIT(chan)
+#define RP1_PWM_GLB_CTRL_SET_UPDATE		BIT(31)
+
+#define RP1_PWM_CHAN_CTRL(chan)			(0x014 + ((chan) * 0x10))
+#define RP1_PWM_CHAN_CTRL_POLARITY		BIT(3)
+#define RP1_PWM_CHAN_CTRL_FIFO_POP_MASK		BIT(8)
+#define RP1_PWM_CHAN_CTRL_MODE			GENMASK(2, 0)
+enum rp1_pwm_ctrl_mode {
+	RP1_PWM_CHAN_CTRL_MODE_ZERO,
+	RP1_PWM_CHAN_CTRL_MODE_TE_MS,
+	RP1_PWM_CHAN_CTRL_MODE_PC_MS,
+	RP1_PWM_CHAN_CTRL_MODE_PD_ENC,
+	RP1_PWM_CHAN_CTRL_MODE_MSB_SER,
+	RP1_PWM_CHAN_CTRL_MODE_PPM,
+	RP1_PWM_CHAN_CTRL_MODE_LE_MS,
+	RP1_PWM_CHAN_CTRL_MODE_LSB_SER,
+};
+
+#define RP1_PWM_CHAN_CTRL_DEFAULT		(RP1_PWM_CHAN_CTRL_FIFO_POP_MASK +  \
+						FIELD_PREP(RP1_PWM_CHAN_CTRL_MODE, \
+						RP1_PWM_CHAN_CTRL_MODE_TE_MS))
+
+#define RP1_PWM_RANGE(chan)			(0x018 + ((chan) * 0x10))
+#define RP1_PWM_PHASE(chan)			(0x01C + ((chan) * 0x10))
+#define RP1_PWM_DUTY(chan)			(0x020 + ((chan) * 0x10))
+
+#define RP1_PWM_NUM_PWMS			4
+
+struct rp1_pwm {
+	struct regmap *regmap;
+	struct clk *clk;
+	unsigned long clk_rate;
+	bool clk_enabled;
+	struct mutex lock; /* sync pm ops with read_tachometer */
+};
+
+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 int rp1_pwm_apply_config(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
+	u32 value;
+	int ret;
+
+	/* update the changed registers on the next strobe to avoid glitches */
+	ret = regmap_read(rp1->regmap, RP1_PWM_GLB_CTRL, &value);
+	if (ret)
+		return ret;
+	value |= RP1_PWM_GLB_CTRL_SET_UPDATE;
+	regmap_write(rp1->regmap, RP1_PWM_GLB_CTRL, value);
+
+	return 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, preserving the polarity bit */
+	regmap_update_bits(rp1->regmap, RP1_PWM_CHAN_CTRL(pwm->hwpwm),
+			   ~(u32)RP1_PWM_CHAN_CTRL_POLARITY, RP1_PWM_CHAN_CTRL_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);
+	u64 period_ticks, duty_ticks, offset_ticks;
+	struct rp1_pwm_waveform *wfhw = _wfhw;
+	u64 clk_rate = rp1->clk_rate;
+	int ret = 0;
+
+	if (!wf->period_length_ns) {
+		*wfhw = (typeof(*wfhw)){
+			.enabled = false,
+			.inverted_polarity = false,
+		};
+		return 0;
+	}
+
+	period_ticks = mul_u64_u64_div_u64(wf->period_length_ns, clk_rate, NSEC_PER_SEC);
+
+	/*
+	 * The period is limited to U32_MAX, and it will be decremented by one later
+	 * to allow 100% duty cycle.
+	 */
+	if (period_ticks > U32_MAX) {
+		period_ticks = U32_MAX;
+	} else if (period_ticks < 2) {
+		period_ticks = 2;
+		ret = 1;
+	}
+
+	duty_ticks = mul_u64_u64_div_u64(wf->duty_length_ns, clk_rate, NSEC_PER_SEC);
+	duty_ticks = min(duty_ticks, period_ticks);
+	offset_ticks = mul_u64_u64_div_u64(wf->duty_offset_ns, clk_rate, NSEC_PER_SEC);
+	if (offset_ticks >= period_ticks)
+		offset_ticks = period_ticks - 1;
+	if (duty_ticks && offset_ticks &&
+	    duty_ticks + offset_ticks >= period_ticks) {
+		wfhw->duty_ticks = period_ticks - duty_ticks;
+		wfhw->inverted_polarity = true;
+	} else {
+		wfhw->duty_ticks = duty_ticks;
+		wfhw->inverted_polarity = false;
+	}
+	/* Account for the extra tick at the end of the period */
+	wfhw->period_ticks = period_ticks - 1;
+
+	wfhw->enabled = true;
+
+	return ret;
+}
+
+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;
+	u64 ticks;
+
+	*wf = (struct pwm_waveform){ };
+
+	if (!wfhw->enabled)
+		return 0;
+
+	wf->period_length_ns = DIV_ROUND_UP_ULL(((u64)wfhw->period_ticks + 1) * NSEC_PER_SEC,
+						(u32)clk_rate);
+
+	if (!wfhw->inverted_polarity) {
+		wf->duty_length_ns = DIV_ROUND_UP_ULL((u64)wfhw->duty_ticks * NSEC_PER_SEC,
+						      (u32)clk_rate);
+	} else {
+		if (wfhw->duty_ticks > (u64)wfhw->period_ticks + 1) {
+			/* 100% duty cycle case */
+			ticks = 0;
+		} else {
+			ticks = (u64)wfhw->period_ticks + 1 - wfhw->duty_ticks;
+		}
+		wf->duty_length_ns = DIV_ROUND_UP_ULL(ticks * NSEC_PER_SEC, (u32)clk_rate);
+		wf->duty_offset_ns = DIV_ROUND_UP_ULL((u64)(wfhw->period_ticks + 1 - ticks) *
+						      NSEC_PER_SEC, (u32)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, ctrl;
+	int ret;
+
+	/* set polarity */
+	ret = regmap_read(rp1->regmap, RP1_PWM_CHAN_CTRL(pwm->hwpwm), &value);
+	if (ret)
+		return ret;
+	if (!wfhw->inverted_polarity)
+		value &= ~RP1_PWM_CHAN_CTRL_POLARITY;
+	else
+		value |= RP1_PWM_CHAN_CTRL_POLARITY;
+	regmap_write(rp1->regmap, RP1_PWM_CHAN_CTRL(pwm->hwpwm), value);
+
+	/* early exit if disabled */
+	ret = regmap_read(rp1->regmap, RP1_PWM_GLB_CTRL, &ctrl);
+	if (ret)
+		return ret;
+	if (!wfhw->enabled) {
+		ctrl &= ~RP1_PWM_GLB_CTRL_CHANNEL_ENABLE(pwm->hwpwm);
+		/* just disable the channel and bail out early */
+		regmap_write(rp1->regmap, RP1_PWM_GLB_CTRL, ctrl);
+		return rp1_pwm_apply_config(chip, pwm);
+	}
+
+	/* 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);
+
+	/* enable the channel */
+	ctrl |= RP1_PWM_GLB_CTRL_CHANNEL_ENABLE(pwm->hwpwm);
+	regmap_write(rp1->regmap, RP1_PWM_GLB_CTRL, ctrl);
+
+	return rp1_pwm_apply_config(chip, pwm);
+}
+
+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;
+	int ret;
+
+	*wfhw = (typeof(*wfhw)) {
+		.period_ticks = 0,
+		.duty_ticks = 0,
+	};
+
+	ret = regmap_read(rp1->regmap, RP1_PWM_GLB_CTRL, &value);
+	if (ret)
+		return ret;
+	wfhw->enabled = !!(value & RP1_PWM_GLB_CTRL_CHANNEL_ENABLE(pwm->hwpwm));
+
+	ret = regmap_read(rp1->regmap, RP1_PWM_CHAN_CTRL(pwm->hwpwm), &value);
+	if (ret)
+		return ret;
+	wfhw->inverted_polarity = !!(value & RP1_PWM_CHAN_CTRL_POLARITY);
+
+	if (wfhw->enabled) {
+		ret = regmap_read(rp1->regmap, RP1_PWM_RANGE(pwm->hwpwm), &wfhw->period_ticks);
+		if (ret)
+			return ret;
+		ret = regmap_read(rp1->regmap, RP1_PWM_DUTY(pwm->hwpwm), &wfhw->duty_ticks);
+		if (ret)
+			return ret;
+	}
+
+	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,
+};
+
+int rp1_pwm_read_tachometer(struct device *dev)
+{
+	struct pwm_chip *chip;
+	struct rp1_pwm *rp1;
+	u32 tach_val;
+	int ret;
+
+	if (!dev)
+		return -EINVAL;
+
+	device_lock(dev);
+
+	chip = dev_get_drvdata(dev);
+	if (!chip) {
+		ret = -ENODEV;
+		goto err_dev_unlock;
+	}
+
+	rp1 = pwmchip_get_drvdata(chip);
+	if (!rp1) {
+		ret = -ENODEV;
+		goto err_dev_unlock;
+	}
+
+	mutex_lock(&rp1->lock);
+	if (!rp1->clk_enabled) {
+		ret = -EBUSY;
+		goto err_clk_unlock;
+	}
+
+	ret = regmap_read(rp1->regmap, RP1_PWM_PHASE(2), &tach_val);
+	if (ret)
+		goto err_clk_unlock;
+
+	ret = (int)tach_val;
+
+err_clk_unlock:
+	mutex_unlock(&rp1->lock);
+err_dev_unlock:
+	device_unlock(dev);
+
+	return ret;
+}
+EXPORT_SYMBOL_NS_GPL(rp1_pwm_read_tachometer, "RP1_PWM_FAN");
+
+static int rp1_pwm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	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);
+	ret = devm_mutex_init(dev, &rp1->lock);
+	if (ret)
+		return ret;
+
+	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");
+
+	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, "Failed 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;
+	}
+	/*
+	 * To prevent u64 overflow in period calculations:
+	 * mul_u64_u64_div_u64(period_ns, clk_rate, NSEC_PER_SEC)
+	 * If clk_rate > 1 GHz, the result can overflow.
+	 */
+	if (clk_rate > HZ_PER_GHZ) {
+		ret = dev_err_probe(dev, -EINVAL, "Clock rate > 1 GHz is not supported\n");
+		goto err_disable_clk;
+	}
+	rp1->clk_rate = clk_rate;
+
+	chip->ops = &rp1_pwm_ops;
+	chip->atomic = true;
+
+	platform_set_drvdata(pdev, chip);
+
+	ret = pwmchip_add(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 void rp1_pwm_remove(struct platform_device *pdev)
+{
+	struct pwm_chip *chip = platform_get_drvdata(pdev);
+	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
+
+	pwmchip_remove(chip);
+
+	if (rp1->clk_enabled) {
+		clk_disable_unprepare(rp1->clk);
+		rp1->clk_enabled = false;
+	}
+}
+
+static int rp1_pwm_suspend(struct device *dev)
+{
+	struct pwm_chip *chip = dev_get_drvdata(dev);
+	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
+
+	mutex_lock(&rp1->lock);
+	if (rp1->clk_enabled) {
+		clk_disable_unprepare(rp1->clk);
+		rp1->clk_enabled = false;
+	}
+	mutex_unlock(&rp1->lock);
+
+	return 0;
+}
+
+static int rp1_pwm_resume(struct device *dev)
+{
+	struct pwm_chip *chip = dev_get_drvdata(dev);
+	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
+	int ret;
+
+	mutex_lock(&rp1->lock);
+	ret = clk_prepare_enable(rp1->clk);
+	if (ret) {
+		dev_err(dev, "Failed to enable clock on resume: %pe\n", ERR_PTR(ret));
+		goto err_unlock;
+	}
+	rp1->clk_enabled = true;
+
+err_unlock:
+	mutex_unlock(&rp1->lock);
+
+	return ret;
+}
+
+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,
+	.remove = rp1_pwm_remove,
+	.driver = {
+		.name = "rp1-pwm",
+		.of_match_table = rp1_pwm_of_match,
+		.pm = pm_ptr(&rp1_pwm_pm_ops),
+	},
+};
+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");
diff --git a/include/soc/bcm2835/raspberrypi-pwm-rp1.h b/include/soc/bcm2835/raspberrypi-pwm-rp1.h
new file mode 100644
index 0000000000000..3be0892da667e
--- /dev/null
+++ b/include/soc/bcm2835/raspberrypi-pwm-rp1.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __SOC_RASPBERRY_PWM_RP1_H__
+#define __SOC_RASPBERRY_PWM_RP1_H__
+
+#include <linux/device.h>
+
+int rp1_pwm_read_tachometer(struct device *dev);
+
+#endif /* __SOC_RASPBERRY_PWM_RP1_H__ */
-- 
2.35.3

[PATCH v9 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     |  9 +++++++++
 2 files changed, 21 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 0fc57e72632ed..748be8f1ee9e2 100644
--- a/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts
+++ b/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts
@@ -64,6 +64,12 @@ 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";
@@ -94,6 +100,12 @@ &rp1_i2c6 {
 	pinctrl-names = "default";
 };
 
+&rp1_pwm1 {
+	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 16f5359395835..df4c2d09c8d34 100644
--- a/arch/arm64/boot/dts/broadcom/rp1-common.dtsi
+++ b/arch/arm64/boot/dts/broadcom/rp1-common.dtsi
@@ -99,7 +99,16 @@ rp1_i2c6: i2c@40088000 {
 		clocks = <&rp1_clocks RP1_CLK_SYS>;
 		i2c-scl-rising-time-ns = <65>;
 		i2c-scl-falling-time-ns = <100>;
+		status = "disabled";
+	};
 
+	rp1_pwm1: 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";
 	};
 
-- 
2.35.3

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

[Gary Guo]

On Fri Sep 18, 2026 at 10:59 AM BST, 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>

Put yourself a Co-developed-by perhaps? You made significant changes.

I've tested the device on my Pi 5, and the fan is indeed spinning now :)

> ---
>  drivers/pwm/Kconfig                       |   8 +
>  drivers/pwm/Makefile                      |   1 +
>  drivers/pwm/pwm-rp1.c                     | 499 ++++++++++++++++++++++
>  include/soc/bcm2835/raspberrypi-pwm-rp1.h |  10 +
>  4 files changed, 518 insertions(+)
>  create mode 100644 drivers/pwm/pwm-rp1.c
>  create mode 100644 include/soc/bcm2835/raspberrypi-pwm-rp1.h
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 7297760868790..e1baec8d294d1 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -637,6 +637,14 @@ config PWM_ROCKCHIP
>  	  Generic PWM framework driver for the PWM controller found on
>  	  Rockchip SoCs.
>  
> +config PWM_RASPBERRYPI_RP1
> +	tristate "RP1 PWM support"
> +	depends on MISC_RP1 || COMPILE_TEST
> +	depends on HAS_IOMEM
> +	select REGMAP_MMIO
> +	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 5630a521a7cff..c07fd24f69f39 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -57,6 +57,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..cfd38e46cc589
> --- /dev/null
> +++ b/drivers/pwm/pwm-rp1.c
> +
> +int rp1_pwm_read_tachometer(struct device *dev)
> +{
> +	struct pwm_chip *chip;
> +	struct rp1_pwm *rp1;
> +	u32 tach_val;
> +	int ret;
> +
> +	if (!dev)
> +		return -EINVAL;
> +
> +	device_lock(dev);

You should use the device link mechanism for synchronizing unbind / runtime PM.
That can be done in your RP1 fan driver, and it doesn't need locking on this
driver.

So Sashiko is kinda reporting a false positive here.

> +
> +	chip = dev_get_drvdata(dev);
> +	if (!chip) {
> +		ret = -ENODEV;
> +		goto err_dev_unlock;
> +	}
> +
> +	rp1 = pwmchip_get_drvdata(chip);
> +	if (!rp1) {
> +		ret = -ENODEV;
> +		goto err_dev_unlock;
> +	}
> +
> +	mutex_lock(&rp1->lock);
> +	if (!rp1->clk_enabled) {
> +		ret = -EBUSY;
> +		goto err_clk_unlock;
> +	}
> +
> +	ret = regmap_read(rp1->regmap, RP1_PWM_PHASE(2), &tach_val);
> +	if (ret)
> +		goto err_clk_unlock;
> +
> +	ret = (int)tach_val;
> +
> +err_clk_unlock:
> +	mutex_unlock(&rp1->lock);
> +err_dev_unlock:
> +	device_unlock(dev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_NS_GPL(rp1_pwm_read_tachometer, "RP1_PWM_FAN");
> +
> +static int rp1_pwm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	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);
> +	ret = devm_mutex_init(dev, &rp1->lock);
> +	if (ret)
> +		return ret;
> +
> +	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");

You mentioned "rework regmap error paths" in cover letter.

But the issue is that regmap doesn't need be used here at all. RP1 is on PCIe
so there is no need for bus abstraction, direct use of MMIO is sufficient.
MMIO accessors have no error paths, so you're saying yourself from having to
handle that, and also reduce the overhead by not having to go through an
abstraction w/ indirect funicton calls.

I suppose regmap was used when syscon was there; but it's not needed anymore.

Best,
Gary

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

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

[Andrea della Porta]

Hi Gary,
thanks for your feedback!

On 20:51 Tue 22 Sep     , Gary Guo wrote:
> On Fri Sep 18, 2026 at 10:59 AM BST, 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>
> 
> Put yourself a Co-developed-by perhaps? You made significant changes.

Ack.

> 
> I've tested the device on my Pi 5, and the fan is indeed spinning now :)

Nice! Thanks for testing it :)

> 
> > ---
> >  drivers/pwm/Kconfig                       |   8 +
> >  drivers/pwm/Makefile                      |   1 +
> >  drivers/pwm/pwm-rp1.c                     | 499 ++++++++++++++++++++++
> >  include/soc/bcm2835/raspberrypi-pwm-rp1.h |  10 +
> >  4 files changed, 518 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-rp1.c
> >  create mode 100644 include/soc/bcm2835/raspberrypi-pwm-rp1.h
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index 7297760868790..e1baec8d294d1 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -637,6 +637,14 @@ config PWM_ROCKCHIP
> >  	  Generic PWM framework driver for the PWM controller found on
> >  	  Rockchip SoCs.
> >  
> > +config PWM_RASPBERRYPI_RP1
> > +	tristate "RP1 PWM support"
> > +	depends on MISC_RP1 || COMPILE_TEST
> > +	depends on HAS_IOMEM
> > +	select REGMAP_MMIO
> > +	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 5630a521a7cff..c07fd24f69f39 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -57,6 +57,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..cfd38e46cc589
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-rp1.c
> > +
> > +int rp1_pwm_read_tachometer(struct device *dev)
> > +{
> > +	struct pwm_chip *chip;
> > +	struct rp1_pwm *rp1;
> > +	u32 tach_val;
> > +	int ret;
> > +
> > +	if (!dev)
> > +		return -EINVAL;
> > +
> > +	device_lock(dev);
> 
> You should use the device link mechanism for synchronizing unbind / runtime PM.
> That can be done in your RP1 fan driver, and it doesn't need locking on this
> driver.

This does not enforce the locking though. Is it enough to just rely on
caller to create the device link in advance? IOW, just add a documentation
comment to the exported function prologue stating that the consumer is
responsible to sync via a device link is acceptable?
Otherwise the only way I see to protect it in any scenario is via the mutex
I've already implemented and a second flag for the removing path (device_lock
will be dropped, of course).

> 
> So Sashiko is kinda reporting a false positive here.
> 
> > +
> > +	chip = dev_get_drvdata(dev);
> > +	if (!chip) {
> > +		ret = -ENODEV;
> > +		goto err_dev_unlock;
> > +	}
> > +
> > +	rp1 = pwmchip_get_drvdata(chip);
> > +	if (!rp1) {
> > +		ret = -ENODEV;
> > +		goto err_dev_unlock;
> > +	}
> > +
> > +	mutex_lock(&rp1->lock);
> > +	if (!rp1->clk_enabled) {
> > +		ret = -EBUSY;
> > +		goto err_clk_unlock;
> > +	}
> > +
> > +	ret = regmap_read(rp1->regmap, RP1_PWM_PHASE(2), &tach_val);
> > +	if (ret)
> > +		goto err_clk_unlock;
> > +
> > +	ret = (int)tach_val;
> > +
> > +err_clk_unlock:
> > +	mutex_unlock(&rp1->lock);
> > +err_dev_unlock:
> > +	device_unlock(dev);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(rp1_pwm_read_tachometer, "RP1_PWM_FAN");
> > +
> > +static int rp1_pwm_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	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);
> > +	ret = devm_mutex_init(dev, &rp1->lock);
> > +	if (ret)
> > +		return ret;
> > +
> > +	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");
> 
> You mentioned "rework regmap error paths" in cover letter.
> 
> But the issue is that regmap doesn't need be used here at all. RP1 is on PCIe
> so there is no need for bus abstraction, direct use of MMIO is sufficient.
> MMIO accessors have no error paths, so you're saying yourself from having to
> handle that, and also reduce the overhead by not having to go through an
> abstraction w/ indirect funicton calls.
>
> I suppose regmap was used when syscon was there; but it's not needed anymore.

True, and I don't have any issue in converting back to MMIO call and drop the conditional for
error checking, but please consider the following, since the driver may be extended in the
future to support more features:

- regmap gives you free debugfs view on the registers, which may be useful to test
  the new features.
- regmap_write/read may still return an error in case the passed register is not in range.
  This will be trapped at runtime only, but could still be useful during development
- I expect the PWM driver to be a access with very low frequency, so I guess the overhead 
  imposed by regmap is negligible.

Since we already have it, I'd prefer to leave regmap if possible for the aforementioned reasons,
but I'm obviously open to drop it in favor of direct MMIO calls in case you or anyone else are
not seeing those as real benefits.

Many thanks,
Andrea

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

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

[Gary Guo]

On Wed Sep 23, 2026 at 1:51 PM BST, Andrea della Porta wrote:
> Hi Gary,
> thanks for your feedback!
>
> On 20:51 Tue 22 Sep     , Gary Guo wrote:
>> On Fri Sep 18, 2026 at 10:59 AM BST, 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>
>> 
>> Put yourself a Co-developed-by perhaps? You made significant changes.
>
> Ack.
>
>> 
>> I've tested the device on my Pi 5, and the fan is indeed spinning now :)
>
> Nice! Thanks for testing it :)
>
>> 
>> > ---
>> >  drivers/pwm/Kconfig                       |   8 +
>> >  drivers/pwm/Makefile                      |   1 +
>> >  drivers/pwm/pwm-rp1.c                     | 499 ++++++++++++++++++++++
>> >  include/soc/bcm2835/raspberrypi-pwm-rp1.h |  10 +
>> >  4 files changed, 518 insertions(+)
>> >  create mode 100644 drivers/pwm/pwm-rp1.c
>> >  create mode 100644 include/soc/bcm2835/raspberrypi-pwm-rp1.h
>> >
>> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> > index 7297760868790..e1baec8d294d1 100644
>> > --- a/drivers/pwm/Kconfig
>> > +++ b/drivers/pwm/Kconfig
>> > @@ -637,6 +637,14 @@ config PWM_ROCKCHIP
>> >  	  Generic PWM framework driver for the PWM controller found on
>> >  	  Rockchip SoCs.
>> >  
>> > +config PWM_RASPBERRYPI_RP1
>> > +	tristate "RP1 PWM support"
>> > +	depends on MISC_RP1 || COMPILE_TEST
>> > +	depends on HAS_IOMEM
>> > +	select REGMAP_MMIO
>> > +	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 5630a521a7cff..c07fd24f69f39 100644
>> > --- a/drivers/pwm/Makefile
>> > +++ b/drivers/pwm/Makefile
>> > @@ -57,6 +57,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..cfd38e46cc589
>> > --- /dev/null
>> > +++ b/drivers/pwm/pwm-rp1.c
>> > +
>> > +int rp1_pwm_read_tachometer(struct device *dev)
>> > +{
>> > +	struct pwm_chip *chip;
>> > +	struct rp1_pwm *rp1;
>> > +	u32 tach_val;
>> > +	int ret;
>> > +
>> > +	if (!dev)
>> > +		return -EINVAL;
>> > +
>> > +	device_lock(dev);
>> 
>> You should use the device link mechanism for synchronizing unbind / runtime PM.
>> That can be done in your RP1 fan driver, and it doesn't need locking on this
>> driver.
>
> This does not enforce the locking though.

What locking do you think is needed?

Driver core takes care of ordering so you will never see a depended device
going away while a dependant device is still bound. Your RP1 fan driver just
need to make sure that it never calls this API after it's itself unbound --
which it needs to guarantee anyway.

> Is it enough to just rely on
> caller to create the device link in advance? IOW, just add a documentation
> comment to the exported function prologue stating that the consumer is
> responsible to sync via a device link is acceptable?

If you use pwm_get API then a device link is automatically created for you
already (however, this automatic link does not pass runtime PM flags, so if you
need that you still need to add link explicitly).

How this is supposed to be synchronized or documented is very hard to get right
without seeing the user side driver -- if you already have a working version of
the RP1 fan driver it might benefit to have it attached as a RFC patch in the
series; or an option to to drop the tachometer API and add it as part of the fan
driver series.

> Otherwise the only way I see to protect it in any scenario is via the mutex
> I've already implemented and a second flag for the removing path (device_lock
> will be dropped, of course).
>
>> 
>> So Sashiko is kinda reporting a false positive here.
>> 
>> > +
>> > +	chip = dev_get_drvdata(dev);
>> > +	if (!chip) {
>> > +		ret = -ENODEV;
>> > +		goto err_dev_unlock;
>> > +	}
>> > +
>> > +	rp1 = pwmchip_get_drvdata(chip);
>> > +	if (!rp1) {
>> > +		ret = -ENODEV;
>> > +		goto err_dev_unlock;
>> > +	}
>> > +
>> > +	mutex_lock(&rp1->lock);
>> > +	if (!rp1->clk_enabled) {
>> > +		ret = -EBUSY;
>> > +		goto err_clk_unlock;
>> > +	}
>> > +
>> > +	ret = regmap_read(rp1->regmap, RP1_PWM_PHASE(2), &tach_val);
>> > +	if (ret)
>> > +		goto err_clk_unlock;
>> > +
>> > +	ret = (int)tach_val;
>> > +
>> > +err_clk_unlock:
>> > +	mutex_unlock(&rp1->lock);
>> > +err_dev_unlock:
>> > +	device_unlock(dev);
>> > +
>> > +	return ret;
>> > +}
>> > +EXPORT_SYMBOL_NS_GPL(rp1_pwm_read_tachometer, "RP1_PWM_FAN");
>> > +
>> > +static int rp1_pwm_probe(struct platform_device *pdev)
>> > +{
>> > +	struct device *dev = &pdev->dev;
>> > +	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);
>> > +	ret = devm_mutex_init(dev, &rp1->lock);
>> > +	if (ret)
>> > +		return ret;
>> > +
>> > +	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");
>> 
>> You mentioned "rework regmap error paths" in cover letter.
>> 
>> But the issue is that regmap doesn't need be used here at all. RP1 is on PCIe
>> so there is no need for bus abstraction, direct use of MMIO is sufficient.
>> MMIO accessors have no error paths, so you're saying yourself from having to
>> handle that, and also reduce the overhead by not having to go through an
>> abstraction w/ indirect funicton calls.
>>
>> I suppose regmap was used when syscon was there; but it's not needed anymore.
>
> True, and I don't have any issue in converting back to MMIO call and drop the conditional for
> error checking, but please consider the following, since the driver may be extended in the
> future to support more features:
>
> - regmap gives you free debugfs view on the registers, which may be useful to test
>   the new features.

Do you have any register that we want to access that is not part of the PWM
facility, other than tachometer?

> - regmap_write/read may still return an error in case the passed register is not in range.
>   This will be trapped at runtime only, but could still be useful during development

I think this is rather a anti-feature. Having additional error paths for some
thing that never happens is not a good idea, especially that you basically get 0
coverage for these paths.

You already know the shape of the register region, so the bounds checking
provided by regmap would be better served by an ahead-of-time check:

    #define RP1_PWM_REG_MAX (RP1_PWM_DUTY(RP1_PWM_NUM_PWMS) + 4)

    struct resource *res;
    base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
    if (IS_ERR(base))
        return PTR_ERR(base);

    if (resource_size(res) < RP1_PWM_REG_MAX) ...

Shameless plug: Rust abstractions for I/O access is actually pretty good in this
regard in the sense that we try to prove statically that access canot fail. It
also has PWM and platform abstractions. If you're interested in learning Rust
this driver might be a good candidate :) If you're going to LPC we can chat
about this there.

Best,
Gary

> - I expect the PWM driver to be a access with very low frequency, so I guess the overhead 
>   imposed by regmap is negligible.
>
> Since we already have it, I'd prefer to leave regmap if possible for the aforementioned reasons,
> but I'm obviously open to drop it in favor of direct MMIO calls in case you or anyone else are
> not seeing those as real benefits.

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

[Andrea della Porta]

On 14:46 Wed 23 Sep     , Gary Guo wrote:
> On Wed Sep 23, 2026 at 1:51 PM BST, Andrea della Porta wrote:
> > Hi Gary,
> > thanks for your feedback!
> >
> > On 20:51 Tue 22 Sep     , Gary Guo wrote:
> >> On Fri Sep 18, 2026 at 10:59 AM BST, Andrea della Porta wrote:

<...snip...>

> >> > +++ b/drivers/pwm/pwm-rp1.c
> >> > +
> >> > +int rp1_pwm_read_tachometer(struct device *dev)
> >> > +{
> >> > +	struct pwm_chip *chip;
> >> > +	struct rp1_pwm *rp1;
> >> > +	u32 tach_val;
> >> > +	int ret;
> >> > +
> >> > +	if (!dev)
> >> > +		return -EINVAL;
> >> > +
> >> > +	device_lock(dev);
> >> 
> >> You should use the device link mechanism for synchronizing unbind / runtime PM.
> >> That can be done in your RP1 fan driver, and it doesn't need locking on this
> >> driver.
> >
> > This does not enforce the locking though.
> 
> What locking do you think is needed?
> 
> Driver core takes care of ordering so you will never see a depended device
> going away while a dependant device is still bound. Your RP1 fan driver just
> need to make sure that it never calls this API after it's itself unbound --
> which it needs to guarantee anyway.

I think you're referring to the struct device (data) and module binary unloading
from memory, which is guaranteed not to happen when there is a dependent device
using it, but what happens when you try to unbind or suspend the device?
More on that below...

> 
> > Is it enough to just rely on
> > caller to create the device link in advance? IOW, just add a documentation
> > comment to the exported function prologue stating that the consumer is
> > responsible to sync via a device link is acceptable?
> 
> If you use pwm_get API then a device link is automatically created for you
> already (however, this automatic link does not pass runtime PM flags, so if you
> need that you still need to add link explicitly).

This driver implements only static PM ops so I think both pwm_get API or
device_link_add should deal automatically with races.
OTOH, what if the consumer obtains a reference to the PWM device via of_* API (or
other means)? Thhose calls does not create device link and we would still have unsync
critical paths.
I'm just trying to figure out whether I should design the exported function as
foolproof and caller agnostic wrt sync issues.
If relying on the caller to use pwm_get/device_link_add is enough, I'd be happy to
find out I'm just being overly paranoid, dropping all the locking to make teh code
simpler.

> 
> How this is supposed to be synchronized or documented is very hard to get right
> without seeing the user side driver -- if you already have a working version of
> the RP1 fan driver it might benefit to have it attached as a RFC patch in the

Agreed, that's why I was trying to be as consumer agnostic as possible. After all,
coupling two modules wrt their locking requirements is usually better to be avoided.
As for the sample fan driver, I have only a simple module that grab a reference
to the pwm device and call the exported tachometer function, so nothing really useful
for this discussion, yet.

> series; or an option to to drop the tachometer API and add it as part of the fan
> driver series.

That is a great advice. I think it's beneficial to discuss everything in one place,
and as a plus we won't block the pwm driver.

> 
> > Otherwise the only way I see to protect it in any scenario is via the mutex
> > I've already implemented and a second flag for the removing path (device_lock
> > will be dropped, of course).
> >
> >> 
> >> So Sashiko is kinda reporting a false positive here.
> >> 
> >> > +
> >> > +	chip = dev_get_drvdata(dev);
> >> > +	if (!chip) {
> >> > +		ret = -ENODEV;
> >> > +		goto err_dev_unlock;
> >> > +	}
> >> > +
> >> > +	rp1 = pwmchip_get_drvdata(chip);
> >> > +	if (!rp1) {
> >> > +		ret = -ENODEV;
> >> > +		goto err_dev_unlock;
> >> > +	}
> >> > +
> >> > +	mutex_lock(&rp1->lock);
> >> > +	if (!rp1->clk_enabled) {
> >> > +		ret = -EBUSY;
> >> > +		goto err_clk_unlock;
> >> > +	}
> >> > +
> >> > +	ret = regmap_read(rp1->regmap, RP1_PWM_PHASE(2), &tach_val);
> >> > +	if (ret)
> >> > +		goto err_clk_unlock;
> >> > +
> >> > +	ret = (int)tach_val;
> >> > +
> >> > +err_clk_unlock:
> >> > +	mutex_unlock(&rp1->lock);
> >> > +err_dev_unlock:
> >> > +	device_unlock(dev);
> >> > +
> >> > +	return ret;
> >> > +}
> >> > +EXPORT_SYMBOL_NS_GPL(rp1_pwm_read_tachometer, "RP1_PWM_FAN");
> >> > +
> >> > +static int rp1_pwm_probe(struct platform_device *pdev)
> >> > +{
> >> > +	struct device *dev = &pdev->dev;
> >> > +	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);
> >> > +	ret = devm_mutex_init(dev, &rp1->lock);
> >> > +	if (ret)
> >> > +		return ret;
> >> > +
> >> > +	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");
> >> 
> >> You mentioned "rework regmap error paths" in cover letter.
> >> 
> >> But the issue is that regmap doesn't need be used here at all. RP1 is on PCIe
> >> so there is no need for bus abstraction, direct use of MMIO is sufficient.
> >> MMIO accessors have no error paths, so you're saying yourself from having to
> >> handle that, and also reduce the overhead by not having to go through an
> >> abstraction w/ indirect funicton calls.
> >>
> >> I suppose regmap was used when syscon was there; but it's not needed anymore.
> >
> > True, and I don't have any issue in converting back to MMIO call and drop the conditional for
> > error checking, but please consider the following, since the driver may be extended in the
> > future to support more features:
> >
> > - regmap gives you free debugfs view on the registers, which may be useful to test
> >   the new features.
> 
> Do you have any register that we want to access that is not part of the PWM
> facility, other than tachometer?

Not at the moment, no. But I don't see how this impact the debugfs usefulness.

> 
> > - regmap_write/read may still return an error in case the passed register is not in range.
> >   This will be trapped at runtime only, but could still be useful during development
> 
> I think this is rather a anti-feature. Having additional error paths for some
> thing that never happens is not a good idea, especially that you basically get 0
> coverage for these paths.

Sure. Well this is true once the code is crystallized and tested, so it's somewhat
still useful (only) during future development. But I got the point, and I agree.

> 
> You already know the shape of the register region, so the bounds checking
> provided by regmap would be better served by an ahead-of-time check:
> 
>     #define RP1_PWM_REG_MAX (RP1_PWM_DUTY(RP1_PWM_NUM_PWMS) + 4)
> 
>     struct resource *res;
>     base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
>     if (IS_ERR(base))
>         return PTR_ERR(base);
> 
>     if (resource_size(res) < RP1_PWM_REG_MAX) ...

Fine for the probe method, but regmap_read/write also check for the range, for free.

> 
> Shameless plug: Rust abstractions for I/O access is actually pretty good in this
> regard in the sense that we try to prove statically that access canot fail. It
> also has PWM and platform abstractions. If you're interested in learning Rust
> this driver might be a good candidate :) If you're going to LPC we can chat
> about this there.

Now I'm tempted! :)
Although it will took far more time to upstream the driver so I think I have
to posticipate Rust for another driver.
Thanks for being so available for the LPC, really appreciated! Not sure if I
can make it this year but we'll see...

Regards,
Andrea

> 
> Best,
> Gary
> 
> > - I expect the PWM driver to be a access with very low frequency, so I guess the overhead 
> >   imposed by regmap is negligible.
> >
> > Since we already have it, I'd prefer to leave regmap if possible for the aforementioned reasons,
> > but I'm obviously open to drop it in favor of direct MMIO calls in case you or anyone else are
> > not seeing those as real benefits.
> 
>