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

[PATCH v7 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 V7:

- _tohw(): use *wfhw = (typeof(*wfhw)){...} to initialize the
  structure in the disable case 
- _tohw(): fixed the offset_ticks calculation
- _fromhw(): fixed a rounding error in the duty_offset_ns calculation
- _write_waveform(): dropped the goto in favor of return in diasble
  channel exit path
- _read_waveform(): use *wfhw = (typeof(*wfhw)){...} to initialize
  the structure in the disable case
- dropped rp1_pwm_remove() since this driver depends on syscon
  and cannot be removed/unbound (added a comment to explain that)

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                           |   9 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-rp1.c                         | 428 ++++++++++++++++++
 6 files changed, 525 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/raspberrypi,rp1-pwm.yaml
 create mode 100644 drivers/pwm/pwm-rp1.c

-- 
2.35.3

[PATCH v7 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..970d0f3b33bbb
--- /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 using shared registers (syscon).
+
+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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The binding description states the device acts as a syscon provider for shared registers, but the schema prohibits the use of the "syscon" compatible string.
--

--- Patch [1]: [PATCH v7 1/3] dt-bindings: pwm: Add Raspberry Pi RP1 PWM controller ---
commit 06ebdbe78362ab02ea5e88d503ad7c179855b48a
Author: Naushir Patuck <naush@raspberrypi.com>

dt-bindings: pwm: Add Raspberry Pi RP1 PWM controller

This commit adds the devicetree binding documentation for the PWM controller
found in the Raspberry Pi RP1 chipset.

> diff --git a/Documentation/devicetree/bindings/pwm/raspberrypi,rp1-pwm.yaml b/Documentation/devicetree/bindings/pwm/raspberrypi,rp1-pwm.yaml
> new file mode 100644
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/raspberrypi,rp1-pwm.yaml
> @@ -0,0 +1,66 @@

[ ... ]

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

[Severity: Medium]
The description states this device acts as a syscon provider for shared
registers, but the compatible property is restricted to a single string.

Will device tree validation fail if a user includes the "syscon" string 
normally required for standard kernel syscon lookup APIs alongside the
"raspberrypi,rp1-pwm" compatible string?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784539788.git.andrea.porta@suse.com?part=1

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

[Andrea della Porta]

Hi,

On 09:50 Mon 20 Jul     , sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The binding description states the device acts as a syscon provider for shared registers, but the schema prohibits the use of the "syscon" compatible string.
> --
> 
> --- Patch [1]: [PATCH v7 1/3] dt-bindings: pwm: Add Raspberry Pi RP1 PWM controller ---
> commit 06ebdbe78362ab02ea5e88d503ad7c179855b48a
> Author: Naushir Patuck <naush@raspberrypi.com>
> 
> dt-bindings: pwm: Add Raspberry Pi RP1 PWM controller
> 
> This commit adds the devicetree binding documentation for the PWM controller
> found in the Raspberry Pi RP1 chipset.
> 
> > diff --git a/Documentation/devicetree/bindings/pwm/raspberrypi,rp1-pwm.yaml b/Documentation/devicetree/bindings/pwm/raspberrypi,rp1-pwm.yaml
> > new file mode 100644
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/raspberrypi,rp1-pwm.yaml
> > @@ -0,0 +1,66 @@
> 
> [ ... ]
> 
> > +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
> 
> [Severity: Medium]
> The description states this device acts as a syscon provider for shared
> registers, but the compatible property is restricted to a single string.
> 
> Will device tree validation fail if a user includes the "syscon" string 
> normally required for standard kernel syscon lookup APIs alongside the
> "raspberrypi,rp1-pwm" compatible string?

I deliberately chose not to include 'syscon' in the 'compatible' property
to retain the flexibility to constrain the regmap exported through syscon
in the future.
Including 'syscon' in the producer's compatible string causes the syscon
framework to register the entire 'reg' property range, whereas the driver
uses of_syscon_register_regmap(), which could restrict the regmap later on.

Additionally, the fact that a specific register is used as a tachometer
depends solely on the firmware; if this changes in the future, the syscon
requirement might go away. Therefore, we might not want 'syscon' in the
compatible string at all.

Thanks,
Andrea

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784539788.git.andrea.porta@suse.com?part=1

[PATCH v7 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 | 428 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 438 insertions(+)
 create mode 100644 drivers/pwm/pwm-rp1.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index e8886a9b64d96..0373dcf8d400d 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -637,6 +637,15 @@ 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
+	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 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..e2d7af80dc071
--- /dev/null
+++ b/drivers/pwm/pwm-rp1.c
@@ -0,0 +1,428 @@
+// 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/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>
+#include <linux/units.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 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_GLB_CTRL, &value);
+	value |= RP1_PWM_GLB_CTRL_SET_UPDATE;
+	regmap_write(rp1->regmap, RP1_PWM_GLB_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, 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,
+						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, clk_rate);
+		wf->duty_offset_ns = DIV_ROUND_UP_ULL((u64)(wfhw->period_ticks + 1 - 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, ctrl;
+
+	/* set polarity */
+	regmap_read(rp1->regmap, RP1_PWM_CHAN_CTRL(pwm->hwpwm), &value);
+	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 */
+	regmap_read(rp1->regmap, RP1_PWM_GLB_CTRL, &ctrl);
+	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);
+		rp1_pwm_apply_config(chip, pwm);
+		return 0;
+	}
+
+	/* 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);
+
+	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_GLB_CTRL, &value);
+	wfhw->enabled = !!(value & RP1_PWM_GLB_CTRL_CHANNEL_ENABLE(pwm->hwpwm));
+
+	regmap_read(rp1->regmap, RP1_PWM_CHAN_CTRL(pwm->hwpwm), &value);
+	wfhw->inverted_polarity = !!(value & RP1_PWM_CHAN_CTRL_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 = (typeof(*wfhw)) {
+			.period_ticks = 0,
+			.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");
+
+	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;
+	}
+
+	ret = of_syscon_register_regmap(np, rp1->regmap);
+	if (ret) {
+		dev_err_probe(dev, ret, "Failed to register syscon\n");
+		goto err_remove_chip;
+	}
+
+	return 0;
+
+err_remove_chip:
+	pwmchip_remove(chip);
+err_disable_clk:
+	clk_disable_unprepare(rp1->clk);
+
+	return ret;
+}
+
+static int rp1_pwm_suspend(struct device *dev)
+{
+	struct pwm_chip *chip = dev_get_drvdata(dev);
+	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
+
+	if (rp1->clk_enabled) {
+		clk_disable_unprepare(rp1->clk);
+		rp1->clk_enabled = false;
+	}
+
+	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;
+
+	ret = clk_prepare_enable(rp1->clk);
+	if (ret) {
+		dev_err(dev, "Failed to enable clock on resume: %pe\n", ERR_PTR(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,
+	/*
+	 * .remove is not needed because this driver cannot be removed/unbound
+	 * cause it relies on syscon which cannot be removed
+	 */
+	.driver = {
+		.name = "rp1-pwm",
+		.of_match_table = rp1_pwm_of_match,
+		.pm = pm_ptr(&rp1_pwm_pm_ops),
+		.suppress_bind_attrs = true,
+	},
+};
+builtin_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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing `.remove` callback leads to Use-After-Free in the PWM and Syscon cores and leaks clock resources on driver unbind.
- [High] Register context loss on suspend/resume silently breaks PWM output.
- [High] Unclamped hardware duty ticks lead to integer underflow in the PWM core for normal polarity.
--

--- Patch [2]: [PATCH v7 2/3] pwm: rp1: Add RP1 PWM controller driver ---
commit 0c98703d857577902e57d25b42a3d46dfbde5795
Author: Naushir Patuck <naush@raspberrypi.com>

pwm: rp1: Add RP1 PWM controller driver

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.

> diff --git a/drivers/pwm/pwm-rp1.c b/drivers/pwm/pwm-rp1.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/pwm/pwm-rp1.c

[ ... ]

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

[Severity: High]
Does the normal polarity case need to clamp the duty ticks similar to the
inverted polarity case?

If the bootloader configures a continuous-high state where the raw hardware
duty ticks exceed the period ticks + 1, this path calculates a duty_length_ns
that is strictly greater than the period_length_ns.

When the PWM core subsequently reads this initial state, pwm_wf2state()
handles duty > period by treating it as an inverted polarity and calculating
duty_cycle = period - duty_length_ns. This subtraction can silently
underflow, causing the core to interpret the hardware state as having a badly
corrupted duty cycle.

> +		if (wfhw->duty_ticks > (u64)wfhw->period_ticks + 1) {
> +			/* 100% duty cycle case */
> +			ticks = 0;
> +		} else {

[ ... ]

> +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;
> +
> +	/* set polarity */
> +	regmap_read(rp1->regmap, RP1_PWM_CHAN_CTRL(pwm->hwpwm), &value);
> +	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);

[Severity: High]
Could the PWM controller lose its hardware configuration after a system
suspend and resume?

During rp1_pwm_request(), the channel control register is initialized with
essential bits like RP1_PWM_CHAN_CTRL_DEFAULT, which includes the
MODE_TE_MS and FIFO_POP_MASK configurations.

If the hardware loses power during suspend, the registers reset. On resume,
when a consumer re-applies its state, this regmap_read() will read the wiped
hardware register. The driver will then update only the polarity bit and
write it back, failing to restore the required operational mode bits that
were initialized during the request phase.

> +
> +	/* early exit if disabled */

[ ... ]

> +static struct platform_driver rp1_pwm_driver = {
> +	.probe = rp1_pwm_probe,
> +	/*
> +	 * .remove is not needed because this driver cannot be removed/unbound
> +	 * cause it relies on syscon which cannot be removed
> +	 */
> +	.driver = {
> +		.name = "rp1-pwm",

[Severity: High]
Is it safe to omit the remove callback here?

Even with suppress_bind_attrs set on the driver, if the parent RP1 PCIe
device is removed, reset, or unbound via sysfs, it will forcibly unbind all
child devices.

Without a remove callback to run cleanup, pwmchip_remove() is never called
and the clock is left enabled. Because the backing memory is devm-managed,
the devres subsystem will eventually free the pwm_chip and regmap memory.
This can leave dangling pointers in the core pwm_chips IDR and the global
syscon_list, which could result in a use-after-free.

> +		.of_match_table = rp1_pwm_of_match,
> +		.pm = pm_ptr(&rp1_pwm_pm_ops),
> +		.suppress_bind_attrs = true,
> +	},
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784539788.git.andrea.porta@suse.com?part=2

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

[Andrea della Porta]

Hi,

On 10:02 Mon 20 Jul     , sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] Missing `.remove` callback leads to Use-After-Free in the PWM and Syscon cores and leaks clock resources on driver unbind.
> - [High] Register context loss on suspend/resume silently breaks PWM output.
> - [High] Unclamped hardware duty ticks lead to integer underflow in the PWM core for normal polarity.
> --
> 
> --- Patch [2]: [PATCH v7 2/3] pwm: rp1: Add RP1 PWM controller driver ---
> commit 0c98703d857577902e57d25b42a3d46dfbde5795
> Author: Naushir Patuck <naush@raspberrypi.com>
> 
> pwm: rp1: Add RP1 PWM controller driver
> 
> 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.
> 
> > diff --git a/drivers/pwm/pwm-rp1.c b/drivers/pwm/pwm-rp1.c
> > new file mode 100644
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-rp1.c
> 
> [ ... ]
> 
> > +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,
> > +						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 {
> 
> [Severity: High]
> Does the normal polarity case need to clamp the duty ticks similar to the
> inverted polarity case?
> 
> If the bootloader configures a continuous-high state where the raw hardware
> duty ticks exceed the period ticks + 1, this path calculates a duty_length_ns
> that is strictly greater than the period_length_ns.
> 
> When the PWM core subsequently reads this initial state, pwm_wf2state()
> handles duty > period by treating it as an inverted polarity and calculating
> duty_cycle = period - duty_length_ns. This subtraction can silently
> underflow, causing the core to interpret the hardware state as having a badly
> corrupted duty cycle.

The conditional right after the opening bracket of this else branch (see below
for the code) is there just for that: clamping the duty to sane values. So it
already takes care of that.

> 
> > +		if (wfhw->duty_ticks > (u64)wfhw->period_ticks + 1) {
> > +			/* 100% duty cycle case */
> > +			ticks = 0;
> > +		} else {
> 
> [ ... ]
> 
> > +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;
> > +
> > +	/* set polarity */
> > +	regmap_read(rp1->regmap, RP1_PWM_CHAN_CTRL(pwm->hwpwm), &value);
> > +	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);
> 
> [Severity: High]
> Could the PWM controller lose its hardware configuration after a system
> suspend and resume?
> 
> During rp1_pwm_request(), the channel control register is initialized with
> essential bits like RP1_PWM_CHAN_CTRL_DEFAULT, which includes the
> MODE_TE_MS and FIFO_POP_MASK configurations.
> 
> If the hardware loses power during suspend, the registers reset. On resume,
> when a consumer re-applies its state, this regmap_read() will read the wiped
> hardware register. The driver will then update only the polarity bit and
> write it back, failing to restore the required operational mode bits that
> were initialized during the request phase.

I think I've already answered this here: 
https://lore.kernel.org/all/aiGAINsTG8VZLn28@apocalypse/
so I guess Sashiko is not sotring the entire history as
context, so I'll reply it here:

AFAIK the RP1 is never powered-off, so just shutting down the clock should be ok
and no register are reset to default.

> 
> > +
> > +	/* early exit if disabled */
> 
> [ ... ]
> 
> > +static struct platform_driver rp1_pwm_driver = {
> > +	.probe = rp1_pwm_probe,
> > +	/*
> > +	 * .remove is not needed because this driver cannot be removed/unbound
> > +	 * cause it relies on syscon which cannot be removed
> > +	 */
> > +	.driver = {
> > +		.name = "rp1-pwm",
> 
> [Severity: High]
> Is it safe to omit the remove callback here?
> 
> Even with suppress_bind_attrs set on the driver, if the parent RP1 PCIe
> device is removed, reset, or unbound via sysfs, it will forcibly unbind all
> child devices.
> 
> Without a remove callback to run cleanup, pwmchip_remove() is never called
> and the clock is left enabled. Because the backing memory is devm-managed,
> the devres subsystem will eventually free the pwm_chip and regmap memory.
> This can leave dangling pointers in the core pwm_chips IDR and the global
> syscon_list, which could result in a use-after-free.

This is true. Uwe, I guess I should reintroduce the .remove callback,
what do you think?

Thanks,
Andrea

> 
> > +		.of_match_table = rp1_pwm_of_match,
> > +		.pm = pm_ptr(&rp1_pwm_pm_ops),
> > +		.suppress_bind_attrs = true,
> > +	},
> > +};
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784539788.git.andrea.porta@suse.com?part=2

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

[Gary Guo]

On Fri Jul 24, 2026 at 4:04 PM BST, Andrea della Porta wrote:
> Hi,
>
> On 10:02 Mon 20 Jul     , sashiko-bot@kernel.org wrote:
>> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>> - [High] Missing `.remove` callback leads to Use-After-Free in the PWM and Syscon cores and leaks clock resources on driver unbind.
>> - [High] Register context loss on suspend/resume silently breaks PWM output.
>> - [High] Unclamped hardware duty ticks lead to integer underflow in the PWM core for normal polarity.
>> --
>> 
>> --- Patch [2]: [PATCH v7 2/3] pwm: rp1: Add RP1 PWM controller driver ---
>> commit 0c98703d857577902e57d25b42a3d46dfbde5795
>> Author: Naushir Patuck <naush@raspberrypi.com>
>> 
>> pwm: rp1: Add RP1 PWM controller driver
>> 
>> 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.
> 
>> > +static struct platform_driver rp1_pwm_driver = {
>> > +	.probe = rp1_pwm_probe,
>> > +	/*
>> > +	 * .remove is not needed because this driver cannot be removed/unbound
>> > +	 * cause it relies on syscon which cannot be removed
>> > +	 */
>> > +	.driver = {
>> > +		.name = "rp1-pwm",
>> 
>> [Severity: High]
>> Is it safe to omit the remove callback here?
>> 
>> Even with suppress_bind_attrs set on the driver, if the parent RP1 PCIe
>> device is removed, reset, or unbound via sysfs, it will forcibly unbind all
>> child devices.
>> 
>> Without a remove callback to run cleanup, pwmchip_remove() is never called
>> and the clock is left enabled. Because the backing memory is devm-managed,
>> the devres subsystem will eventually free the pwm_chip and regmap memory.
>> This can leave dangling pointers in the core pwm_chips IDR and the global
>> syscon_list, which could result in a use-after-free.
>
> This is true. Uwe, I guess I should reintroduce the .remove callback,
> what do you think?
>

How is remove going to work with syscon? The previous version seems to just
ignore it completely.

Also, do you actually need syscon here? Given that the pwm fan device is going
to depend on the PWM fan device anyway, you could just EXPORT_SYMBOL_NS an API
from the rp1 driver that returns the tachometer reading when supplied with a rp1
pwm device pointer?

Best,
Gary

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

[Andrea della Porta]

Hi Gary,

On 19:42 Tue 25 Aug     , Gary Guo wrote:
> On Fri Jul 24, 2026 at 4:04 PM BST, Andrea della Porta wrote:
> > Hi,
> >
> > On 10:02 Mon 20 Jul     , sashiko-bot@kernel.org wrote:
> >> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> >> - [High] Missing `.remove` callback leads to Use-After-Free in the PWM and Syscon cores and leaks clock resources on driver unbind.
> >> - [High] Register context loss on suspend/resume silently breaks PWM output.
> >> - [High] Unclamped hardware duty ticks lead to integer underflow in the PWM core for normal polarity.
> >> --
> >> 
> >> --- Patch [2]: [PATCH v7 2/3] pwm: rp1: Add RP1 PWM controller driver ---
> >> commit 0c98703d857577902e57d25b42a3d46dfbde5795
> >> Author: Naushir Patuck <naush@raspberrypi.com>
> >> 
> >> pwm: rp1: Add RP1 PWM controller driver
> >> 
> >> 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.
> > 
> >> > +static struct platform_driver rp1_pwm_driver = {
> >> > +	.probe = rp1_pwm_probe,
> >> > +	/*
> >> > +	 * .remove is not needed because this driver cannot be removed/unbound
> >> > +	 * cause it relies on syscon which cannot be removed
> >> > +	 */
> >> > +	.driver = {
> >> > +		.name = "rp1-pwm",
> >> 
> >> [Severity: High]
> >> Is it safe to omit the remove callback here?
> >> 
> >> Even with suppress_bind_attrs set on the driver, if the parent RP1 PCIe
> >> device is removed, reset, or unbound via sysfs, it will forcibly unbind all
> >> child devices.
> >> 
> >> Without a remove callback to run cleanup, pwmchip_remove() is never called
> >> and the clock is left enabled. Because the backing memory is devm-managed,
> >> the devres subsystem will eventually free the pwm_chip and regmap memory.
> >> This can leave dangling pointers in the core pwm_chips IDR and the global
> >> syscon_list, which could result in a use-after-free.
> >
> > This is true. Uwe, I guess I should reintroduce the .remove callback,
> > what do you think?
> >
> 
> How is remove going to work with syscon? The previous version seems to just
> ignore it completely.

True, unfortunately we have not much choice with syscon because it does not
contemplate a corresponding remove function. The best well-behaved code could
just deal with clocks and resources that can be released, while syscon is
unfortunately not among them.

On the other hand, I'd like to emphasize the following:

- the driver could not be unbinded nor unloaded (directly) by the user, and...
- ...PWM driver removal as a consequence of the pci bus device being forcibly
  removed will almost surely leave the system unusable.

As a matter of fact, even though it's not impossible for the remove callback
to be called, it seems highly unlikely. So maybe it's not that big deal.

> 
> Also, do you actually need syscon here? Given that the pwm fan device is going
> to depend on the PWM fan device anyway, you could just EXPORT_SYMBOL_NS an API
> from the rp1 driver that returns the tachometer reading when supplied with a rp1
> pwm device pointer?

I'm not devoted to syscon so if everyone agrees on the EXPORT_SYMBOL_NS solution
I'm perfectly fine. It seems to be used throughout several drivers and as long as
it's not considered to couple the consumer and the PWM producer too much, I think
it's an easy alternative. Uwe, are you fine with that?

Many thanks,
Andrea

> 
> Best,
> Gary
> 

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

[Gary Guo]

On Fri Sep 4, 2026 at 2:53 PM BST, Andrea della Porta wrote:
> Hi Gary,
>
> On 19:42 Tue 25 Aug     , Gary Guo wrote:
>> On Fri Jul 24, 2026 at 4:04 PM BST, Andrea della Porta wrote:
>> > Hi,
>> >
>> > On 10:02 Mon 20 Jul     , sashiko-bot@kernel.org wrote:
>> >> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>> >> - [High] Missing `.remove` callback leads to Use-After-Free in the PWM and Syscon cores and leaks clock resources on driver unbind.
>> >> - [High] Register context loss on suspend/resume silently breaks PWM output.
>> >> - [High] Unclamped hardware duty ticks lead to integer underflow in the PWM core for normal polarity.
>> >> --
>> >> 
>> >> --- Patch [2]: [PATCH v7 2/3] pwm: rp1: Add RP1 PWM controller driver ---
>> >> commit 0c98703d857577902e57d25b42a3d46dfbde5795
>> >> Author: Naushir Patuck <naush@raspberrypi.com>
>> >> 
>> >> pwm: rp1: Add RP1 PWM controller driver
>> >> 
>> >> 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.
>> > 
>> >> > +static struct platform_driver rp1_pwm_driver = {
>> >> > +	.probe = rp1_pwm_probe,
>> >> > +	/*
>> >> > +	 * .remove is not needed because this driver cannot be removed/unbound
>> >> > +	 * cause it relies on syscon which cannot be removed
>> >> > +	 */
>> >> > +	.driver = {
>> >> > +		.name = "rp1-pwm",
>> >> 
>> >> [Severity: High]
>> >> Is it safe to omit the remove callback here?
>> >> 
>> >> Even with suppress_bind_attrs set on the driver, if the parent RP1 PCIe
>> >> device is removed, reset, or unbound via sysfs, it will forcibly unbind all
>> >> child devices.
>> >> 
>> >> Without a remove callback to run cleanup, pwmchip_remove() is never called
>> >> and the clock is left enabled. Because the backing memory is devm-managed,
>> >> the devres subsystem will eventually free the pwm_chip and regmap memory.
>> >> This can leave dangling pointers in the core pwm_chips IDR and the global
>> >> syscon_list, which could result in a use-after-free.
>> >
>> > This is true. Uwe, I guess I should reintroduce the .remove callback,
>> > what do you think?
>> >
>> 
>> How is remove going to work with syscon? The previous version seems to just
>> ignore it completely.
>
> True, unfortunately we have not much choice with syscon because it does not
> contemplate a corresponding remove function. The best well-behaved code could
> just deal with clocks and resources that can be released, while syscon is
> unfortunately not among them.
>
> On the other hand, I'd like to emphasize the following:
>
> - the driver could not be unbinded nor unloaded (directly) by the user, and...
> - ...PWM driver removal as a consequence of the pci bus device being forcibly
>   removed will almost surely leave the system unusable.
>
> As a matter of fact, even though it's not impossible for the remove callback
> to be called, it seems highly unlikely. So maybe it's not that big deal.
>
>> 
>> Also, do you actually need syscon here? Given that the pwm fan device is going
>> to depend on the PWM fan device anyway,

Oops, it should have been "PWM fan device depend on PWM device". But you got it.

>> you could just EXPORT_SYMBOL_NS an API
>> from the rp1 driver that returns the tachometer reading when supplied with a rp1
>> pwm device pointer?
>
> I'm not devoted to syscon so if everyone agrees on the EXPORT_SYMBOL_NS solution
> I'm perfectly fine. It seems to be used throughout several drivers and as long as
> it's not considered to couple the consumer and the PWM producer too much, I think
> it's an easy alternative. Uwe, are you fine with that?

I think that's pretty typical for platform devices, provided that the consumer
and the producer are closely related.

I checked the RPi vendor kernel code and it looks like they're modifying the PWM
fan driver code directly -- in which case the coupling would definitely be
problematic. But I think that modification is problematic itself and not
upstreamable. I suppose you'll add a separate, dedicated RP1 fan driver?

In which case I don't see the coupling being an issue.

Best,
Gary

>
> Many thanks,
> Andrea
>
>> 
>> Best,
>> Gary
>> 

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

[Andrea della Porta]

Hi Gary,

On 15:05 Fri 04 Sep     , Gary Guo wrote:
> On Fri Sep 4, 2026 at 2:53 PM BST, Andrea della Porta wrote:
> > Hi Gary,
> >
> > On 19:42 Tue 25 Aug     , Gary Guo wrote:
> >> On Fri Jul 24, 2026 at 4:04 PM BST, Andrea della Porta wrote:
> >> > Hi,
> >> >
> >> > On 10:02 Mon 20 Jul     , sashiko-bot@kernel.org wrote:
> >> >> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> >> >> - [High] Missing `.remove` callback leads to Use-After-Free in the PWM and Syscon cores and leaks clock resources on driver unbind.
> >> >> - [High] Register context loss on suspend/resume silently breaks PWM output.
> >> >> - [High] Unclamped hardware duty ticks lead to integer underflow in the PWM core for normal polarity.
> >> >> --
> >> >> 
> >> >> --- Patch [2]: [PATCH v7 2/3] pwm: rp1: Add RP1 PWM controller driver ---
> >> >> commit 0c98703d857577902e57d25b42a3d46dfbde5795
> >> >> Author: Naushir Patuck <naush@raspberrypi.com>
> >> >> 
> >> >> pwm: rp1: Add RP1 PWM controller driver
> >> >> 
> >> >> 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.
> >> > 
> >> >> > +static struct platform_driver rp1_pwm_driver = {
> >> >> > +	.probe = rp1_pwm_probe,
> >> >> > +	/*
> >> >> > +	 * .remove is not needed because this driver cannot be removed/unbound
> >> >> > +	 * cause it relies on syscon which cannot be removed
> >> >> > +	 */
> >> >> > +	.driver = {
> >> >> > +		.name = "rp1-pwm",
> >> >> 
> >> >> [Severity: High]
> >> >> Is it safe to omit the remove callback here?
> >> >> 
> >> >> Even with suppress_bind_attrs set on the driver, if the parent RP1 PCIe
> >> >> device is removed, reset, or unbound via sysfs, it will forcibly unbind all
> >> >> child devices.
> >> >> 
> >> >> Without a remove callback to run cleanup, pwmchip_remove() is never called
> >> >> and the clock is left enabled. Because the backing memory is devm-managed,
> >> >> the devres subsystem will eventually free the pwm_chip and regmap memory.
> >> >> This can leave dangling pointers in the core pwm_chips IDR and the global
> >> >> syscon_list, which could result in a use-after-free.
> >> >
> >> > This is true. Uwe, I guess I should reintroduce the .remove callback,
> >> > what do you think?
> >> >
> >> 
> >> How is remove going to work with syscon? The previous version seems to just
> >> ignore it completely.
> >
> > True, unfortunately we have not much choice with syscon because it does not
> > contemplate a corresponding remove function. The best well-behaved code could
> > just deal with clocks and resources that can be released, while syscon is
> > unfortunately not among them.
> >
> > On the other hand, I'd like to emphasize the following:
> >
> > - the driver could not be unbinded nor unloaded (directly) by the user, and...
> > - ...PWM driver removal as a consequence of the pci bus device being forcibly
> >   removed will almost surely leave the system unusable.
> >
> > As a matter of fact, even though it's not impossible for the remove callback
> > to be called, it seems highly unlikely. So maybe it's not that big deal.
> >
> >> 
> >> Also, do you actually need syscon here? Given that the pwm fan device is going
> >> to depend on the PWM fan device anyway,
> 
> Oops, it should have been "PWM fan device depend on PWM device". But you got it.
> 
> >> you could just EXPORT_SYMBOL_NS an API
> >> from the rp1 driver that returns the tachometer reading when supplied with a rp1
> >> pwm device pointer?
> >
> > I'm not devoted to syscon so if everyone agrees on the EXPORT_SYMBOL_NS solution
> > I'm perfectly fine. It seems to be used throughout several drivers and as long as
> > it's not considered to couple the consumer and the PWM producer too much, I think
> > it's an easy alternative. Uwe, are you fine with that?
> 
> I think that's pretty typical for platform devices, provided that the consumer
> and the producer are closely related.
> 
> I checked the RPi vendor kernel code and it looks like they're modifying the PWM
> fan driver code directly -- in which case the coupling would definitely be
> problematic. But I think that modification is problematic itself and not
> upstreamable. I suppose you'll add a separate, dedicated RP1 fan driver?

You nailed it. I've changed downstream driver approach because it was too invasive
to the pwm-fan driver and, as you mentioned, not upstreamable.
A separate RP1-custom fan driver was in plan from teh beginning.

> 
> In which case I don't see the coupling being an issue.

Perfect!

Many thanks,
Andrea

> 
> Best,
> Gary
> 
> >
> > Many thanks,
> > Andrea
> >
> >> 
> >> Best,
> >> Gary
> >> 
> 
> 

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

[Christophe JAILLET]

Le 20/07/2026 à 11:44, Andrea della Porta a écrit :
> 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>

Hi,

...

> +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");
> +
> +	rp1->clk = devm_clk_get(dev, NULL);

Could it be devm_clk_get_enabled() to simplify the error handling path 
as done above with other devm function?
...

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

... this also saves these 3 lines.

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

Could it be devm_pwmchip_add() to simplify the error handling path as 
done above with other devm function?

> +	if (ret) {
> +		dev_err_probe(dev, ret, "Failed to register PWM chip\n");
> +		goto err_disable_clk;
> +	}
> +
> +	ret = of_syscon_register_regmap(np, rp1->regmap);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "Failed to register syscon\n");
> +		goto err_remove_chip;
> +	}
> +
> +	return 0;
> +
> +err_remove_chip:
> +	pwmchip_remove(chip);
> +err_disable_clk:
> +	clk_disable_unprepare(rp1->clk);
> +
> +	return ret;
> +}
...

CJ

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

[Andrea della Porta]

Hi Christophe,

On 22:36 Thu 03 Sep     , Christophe JAILLET wrote:
> Le 20/07/2026 à 11:44, Andrea della Porta a écrit :
> > 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>
> 
> Hi,
> 
> ...
> 
> > +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");
> > +
> > +	rp1->clk = devm_clk_get(dev, NULL);
> 
> Could it be devm_clk_get_enabled() to simplify the error handling path as
> done above with other devm function?

The very first version of this patches had devres everywhere, but Uwe has correctly
spotted that this could lead to clock ops imbalance, please see:
https://lore.kernel.org/all/adLTwOTbkJ0VQXy6@monoceros/
As a result, I turned devm_clk_get_enabled() into the corresponding non devres/single
component functions since now disengaging the clock depends on a conditional.
Of course this does not make much sense in case we don't need a .remove callback,
but it seems that I can reintroduce it again if we agree to use EXPORT_SYMBOL_NS.

> ...
> 
> > +	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");
> 
> ... this also saves these 3 lines.

See above.

> 
> > +	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);
> 
> Could it be devm_pwmchip_add() to simplify the error handling path as done
> above with other devm function?

Due to the above-mentioned scenario and since .remove is called before devres release
funtions, that would make the clock to be released before the pwm chip, causing
inconsistencies if the pwm is used in the meanwhile. 

Many thanks,
Andrea.

> 
> > +	if (ret) {
> > +		dev_err_probe(dev, ret, "Failed to register PWM chip\n");
> > +		goto err_disable_clk;
> > +	}
> > +
> > +	ret = of_syscon_register_regmap(np, rp1->regmap);
> > +	if (ret) {
> > +		dev_err_probe(dev, ret, "Failed to register syscon\n");
> > +		goto err_remove_chip;
> > +	}
> > +
> > +	return 0;
> > +
> > +err_remove_chip:
> > +	pwmchip_remove(chip);
> > +err_disable_clk:
> > +	clk_disable_unprepare(rp1->clk);
> > +
> > +	return ret;
> > +}
> ...
> 
> CJ

[PATCH v7 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 v7 0/3] Add RP1 PWM controller support

[Florian Fainelli]

On 7/20/26 02:44, Andrea della Porta wrote:
> 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

Uwe, how do we go about applying these patches? Do you want to take 
patch #2 and I take patches #1 and #3 through the Broadcom ARM SoC tree(s)?
-- 
Florian