Skip to contentSkip to conversation

Add RP1 PWM controller support

PATCH Superseded Andrea della Porta 4 participants 13 messages Jul 3 – Jul 17

Subsystems (4)

v6 of 12 Superseded Latest is v12 →

lore
Share & tools
lore

Andrea della Porta

author maintainer
#0 lore raw

v6 cover Add RP1 PWM controller support

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

- dt-bindings: added Andrea and Stanimir as maintiners, Naushir if you want to be dropped from the maintainer list, please just give me a shout
- dt-bindings: added dmas properties in case the driver will support that in the future
- _tohw(): fetch the actual polarity in the disable case to avoid potential glitch due to garbage data in the passed-in param
- _tohw(): fixed a compilation error on 32 bit target due to missing 64 bit divider helper function
- _fromhw(): division by clk_rate explicitly cast to 32 bit for the divisor
- _fromhw(): fixed an unsigned underflow in ticks calculation
- set the atomic flag to allow calling from atomic context

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                         | 435 ++++++++++++++++++
 6 files changed, 532 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/raspberrypi,rp1-pwm.yaml
 create mode 100644 drivers/pwm/pwm-rp1.c
Signature · 1 line
2.35.3
author maintainer
#1 lore raw

PATCH v6 1/3 dt-bindings: pwm: Add Raspberry Pi RP1 PWM controller

From: Naushir Patuck <[email protected]>

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

---
 .../bindings/pwm/raspberrypi,rp1-pwm.yaml     | 66 +++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/raspberrypi,rp1-pwm.yaml
Documentation/devicetree/bindings/pwm/raspberrypi,rp1-pwm.yaml new file +66 -0
new file mode 100644
index 0000000000000..970d0f3b33bbb
@@ -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 <[email protected]>
+  - Andrea della Porta <[email protected]>
+  - Stanimir Varbanov <[email protected]>
+
+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>;
+    };
Signature · 1 line
2.35.3
Reviewed-by on v6 1/3
#2 lore raw

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

Quoted 10 lines · Andrea della Porta

From: Naushir Patuck <[email protected]>

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

Quoted 77 lines
---
  .../bindings/pwm/raspberrypi,rp1-pwm.yaml     | 66 +++++++++++++++++++
  1 file changed, 66 insertions(+)

create mode 100644 Documentation/devicetree/bindings/pwm/raspberrypi,rp1-pwm.yaml

Documentation/devicetree/bindings/pwm/raspberrypi,rp1-pwm.yaml new file +66 -0
new file mode 100644
index 0000000000000..970d0f3b33bbb
@@ -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 <[email protected]>
+  - Andrea della Porta <[email protected]>
+  - Stanimir Varbanov <[email protected]>
+
+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>;
+    };
author maintainer
#3 lore raw

PATCH v6 2/3 pwm: rp1: Add RP1 PWM controller driver

In reply to Andrea della Porta

From: Naushir Patuck <[email protected]>

The Raspberry Pi RP1 southbridge features an embedded PWM controller with 4 output channels, alongside an RPM interface to read the fan speed on the Raspberry Pi 5.

Add the supporting driver.

---
 drivers/pwm/Kconfig   |   9 +
 drivers/pwm/Makefile  |   1 +
 drivers/pwm/pwm-rp1.c | 435 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 445 insertions(+)
 create mode 100644 drivers/pwm/pwm-rp1.c
drivers/pwm/Kconfig +9 -0
index e8886a9b64d96..0373dcf8d400d 100644
@@ -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
drivers/pwm/Makefile +1 -0
index 5630a521a7cff..c07fd24f69f39 100644
@@ -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
drivers/pwm/pwm-rp1.c new file +435 -0

435 lines not on this page · raw message

Signature · 1 line
2.35.3
#4 lore raw

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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Dangling syscon pointer and Use-After-Free upon device unbind due to devres regmap cleanup.…

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Dangling syscon pointer and Use-After-Free upon device unbind due to devres regmap cleanup.
- [High] Non-atomic Read-Modify-Write of shared syscon registers causing data corruption. --

commit ed6030e515052cf4e3fde5de4c8c7ffcb06a014a
Author: Naushir Patuck <[email protected]>

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.

Quoted 6 lines
drivers/pwm/pwm-rp1.c new file +0 -0
new file mode 100644
index 0000000000000..8ef5ec9ed3a07
@@ -0,0 +1,435 @@

[ ... ]

drivers/pwm/pwm-rp1.c line 111

Quoted 9 lines
drivers/pwm/pwm-rp1.c new file +9 -0
+static void rp1_pwm_apply_config(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
+	u32 value;
+
+	/* update the changed registers on the next strobe to avoid glitches */
+	regmap_read(rp1->regmap, RP1_PWM_GLB_CTRL, &value);
+	value |= RP1_PWM_GLB_CTRL_SET_UPDATE;
+	regmap_write(rp1->regmap, RP1_PWM_GLB_CTRL, value);

[Severity: High]
In rp1_pwm_apply_config(), since this regmap is exposed to other drivers via syscon (like the RP1 RPM fan driver), could this manual read-modify-write sequence race with concurrent register updates?

The regmap lock is released between regmap_read() and regmap_write(), which might allow another syscon client's changes to RP1_PWM_GLB_CTRL to be overwritten with stale data. Does this need to use regmap_update_bits() instead?

Quoted 1 line

+}

[ ... ]

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

drivers/pwm/pwm-rp1.c line 228

Quoted 10 lines
drivers/pwm/pwm-rp1.c new file +10 -0
+	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]
Does rp1_pwm_write_waveform() present the same non-atomic read-modify-write race condition here when updating RP1_PWM_CHAN_CTRL?

Quoted 8 lines
drivers/pwm/pwm-rp1.c new file +8 -0
+
+	/* 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);
+		goto exit_disable;
+	}
+

drivers/pwm/pwm-rp1.c line 246

Quoted 10 lines
drivers/pwm/pwm-rp1.c new file +10 -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);
+exit_disable:
+	regmap_write(rp1->regmap, RP1_PWM_GLB_CTRL, ctrl);

[Severity: High]
And similarly here in rp1_pwm_write_waveform() for RP1_PWM_GLB_CTRL?

Quoted 5 lines
drivers/pwm/pwm-rp1.c new file +5 -0
+
+	rp1_pwm_apply_config(chip, pwm);
+
+	return 0;
+}

[ ... ]

Quoted 2 lines
drivers/pwm/pwm-rp1.c new file +2 -0
+static int rp1_pwm_probe(struct platform_device *pdev)
+{

[ ... ]

Quoted 3 lines
drivers/pwm/pwm-rp1.c new file +3 -0
+	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");

[ ... ]

drivers/pwm/pwm-rp1.c line 359

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

[Severity: High]
In rp1_pwm_probe(), because rp1->regmap is managed by devres (allocated via devm_regmap_init_mmio), it will be automatically freed when the device is unbound. However, of_syscon_register_regmap() registers it globally.

Since there is no mechanism to unregister the regmap from syscon upon device removal, wouldn't unbinding the parent PCI device leave a dangling pointer in the global syscon list?

Could this lead to a use-after-free if another driver attempts to use this syscon client after this device is unbound?

Quoted 8 lines
drivers/pwm/pwm-rp1.c new file +8 -0
+
+err_remove_chip:
+	pwmchip_remove(chip);
+err_disable_clk:
+	clk_disable_unprepare(rp1->clk);
+
+	return ret;
+}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
author maintainer
#5 lore raw

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

Hi,

Quoted 43 lines · On 17:17 Fri 03 Jul , [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Dangling syscon pointer and Use-After-Free upon device unbind due to devres regmap cleanup.
- [High] Non-atomic Read-Modify-Write of shared syscon registers causing data corruption. --

commit ed6030e515052cf4e3fde5de4c8c7ffcb06a014a
Author: Naushir Patuck <[email protected]>

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.

Quoted 6 lines
drivers/pwm/pwm-rp1.c new file +0 -0
new file mode 100644
index 0000000000000..8ef5ec9ed3a07
@@ -0,0 +1,435 @@

[ ... ]

drivers/pwm/pwm-rp1.c line 111

Quoted 9 lines
drivers/pwm/pwm-rp1.c new file +9 -0
+static void rp1_pwm_apply_config(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
+	u32 value;
+
+	/* update the changed registers on the next strobe to avoid glitches */
+	regmap_read(rp1->regmap, RP1_PWM_GLB_CTRL, &value);
+	value |= RP1_PWM_GLB_CTRL_SET_UPDATE;
+	regmap_write(rp1->regmap, RP1_PWM_GLB_CTRL, value);

[Severity: High]
In rp1_pwm_apply_config(), since this regmap is exposed to other drivers via syscon (like the RP1 RPM fan driver), could this manual read-modify-write sequence race with concurrent register updates?

Quoted 4 lines

The regmap lock is released between regmap_read() and regmap_write(), which might allow another syscon client's changes to RP1_PWM_GLB_CTRL to be overwritten with stale data. Does this need to use regmap_update_bits() instead?

The registers are guaranteed by design to never overlap: the producer is the fw which writes in the phase register and the consumer will ionly read that register. No other registers are shared.

Quoted 14 lines
Quoted 2 lines

+}

[ ... ]

Quoted 11 lines
drivers/pwm/pwm-rp1.c new file +11 -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)
Quoted 8 lines

drivers/pwm/pwm-rp1.c line 228

Quoted 4 lines
drivers/pwm/pwm-rp1.c new file +4 -0
+		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]
Does rp1_pwm_write_waveform() present the same non-atomic read-modify-write race condition here when updating RP1_PWM_CHAN_CTRL?

See above.

Quoted 14 lines
Quoted 14 lines
drivers/pwm/pwm-rp1.c new file +13 -0
+
+	/* 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);
+		goto exit_disable;
+	}
+
+	/* 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);
Quoted 8 lines

drivers/pwm/pwm-rp1.c line 246

Quoted 5 lines
drivers/pwm/pwm-rp1.c new file +5 -0
+
+	/* enable the channel */
+	ctrl |= RP1_PWM_GLB_CTRL_CHANNEL_ENABLE(pwm->hwpwm);
+exit_disable:
+	regmap_write(rp1->regmap, RP1_PWM_GLB_CTRL, ctrl);

[Severity: High]
And similarly here in rp1_pwm_write_waveform() for RP1_PWM_GLB_CTRL?

See above.

Quoted 27 lines
Quoted 6 lines
drivers/pwm/pwm-rp1.c new file +5 -0
+
+	rp1_pwm_apply_config(chip, pwm);
+
+	return 0;
+}

[ ... ]

Quoted 2 lines
drivers/pwm/pwm-rp1.c new file +2 -0
+static int rp1_pwm_probe(struct platform_device *pdev)
+{

[ ... ]

Quoted 3 lines
drivers/pwm/pwm-rp1.c new file +3 -0
+	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");

[ ... ]

drivers/pwm/pwm-rp1.c line 359

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

[Severity: High]
In rp1_pwm_probe(), because rp1->regmap is managed by devres (allocated via devm_regmap_init_mmio), it will be automatically freed when the device is unbound. However, of_syscon_register_regmap() registers it globally.

Quoted 7 lines

Since there is no mechanism to unregister the regmap from syscon upon device removal, wouldn't unbinding the parent PCI device leave a dangling pointer in the global syscon list?

Could this lead to a use-after-free if another driver attempts to use this syscon client after this device is unbound?

The device is not hotpluggable and the driver cannot be unloaded/unbounded, so there should be no dangling pointer around.

Regards,
Andrea

Quoted 11 lines
Quoted 8 lines
drivers/pwm/pwm-rp1.c new file +8 -0
+
+err_remove_chip:
+	pwmchip_remove(chip);
+err_disable_clk:
+	clk_disable_unprepare(rp1->clk);
+
+	return ret;
+}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Reviewed-by on v6 2/3
#6 lore raw

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

In reply to Andrea della Porta
Quoted 6 lines · Andrea della Porta

From: Naushir Patuck <[email protected]>

The Raspberry Pi RP1 southbridge features an embedded PWM controller with 4 output channels, alongside an RPM interface to read the fan speed on the Raspberry Pi 5.

Quoted 6 lines

Add the supporting driver.

Quoted 480 lines
---
  drivers/pwm/Kconfig   |   9 +
  drivers/pwm/Makefile  |   1 +
  drivers/pwm/pwm-rp1.c | 435 ++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 445 insertions(+)

create mode 100644 drivers/pwm/pwm-rp1.c

drivers/pwm/Kconfig +9 -0
index e8886a9b64d96..0373dcf8d400d 100644
@@ -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
drivers/pwm/Makefile +1 -0
index 5630a521a7cff..c07fd24f69f39 100644
@@ -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
drivers/pwm/pwm-rp1.c new file +435 -0
new file mode 100644
index 0000000000000..8ef5ec9ed3a07
@@ -0,0 +1,435 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * pwm-rp1.c
+ *
+ * Raspberry Pi RP1 PWM.
+ *
+ * Copyright © 2026 Raspberry Pi Ltd.
+ *
+ * Author: Naushir Patuck ([email protected])
+ *
+ * Based on the pwm-bcm2835 driver by:
+ * Bart Tanghe <[email protected]>
+ *
+ * Datasheet: https://pip-assets.raspberrypi.com/categories/892-raspberry-pi-5/documents/RP-008370-DS-1-rp1-peripherals.pdf?disposition=inline
+ *
+ * Limitations:
+ * - Channels can be enabled/disabled 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->enabled = false;
+		wfhw->inverted_polarity = (pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED);
+		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) {
+		u64 remainder;
+
+		div64_u64_rem(offset_ticks, period_ticks, &remainder);
+		offset_ticks = remainder;
+	}
+	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 = wf->period_length_ns - wf->duty_length_ns;
+	}
+
+	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);
+		goto exit_disable;
+	}
+
+	/* 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);
+exit_disable:
+	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->period_ticks = 0;
+		wfhw->duty_ticks = 0;
+	}
+
+	return 0;
+}
+
+static const struct pwm_ops rp1_pwm_ops = {
+	.sizeof_wfhw = sizeof(struct rp1_pwm_waveform),
+	.request = rp1_pwm_request,
+	.round_waveform_tohw = rp1_pwm_round_waveform_tohw,
+	.round_waveform_fromhw = rp1_pwm_round_waveform_fromhw,
+	.read_waveform = rp1_pwm_read_waveform,
+	.write_waveform = rp1_pwm_write_waveform,
+};
+
+static int rp1_pwm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	unsigned long clk_rate;
+	struct pwm_chip *chip;
+	void __iomem	*base;
+	struct rp1_pwm *rp1;
+	int ret;
+
+	chip = devm_pwmchip_alloc(dev, RP1_PWM_NUM_PWMS, sizeof(*rp1));
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+
+	rp1 = pwmchip_get_drvdata(chip);
+
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	rp1->regmap = devm_regmap_init_mmio(dev, base, &rp1_pwm_regmap_config);
+	if (IS_ERR(rp1->regmap))
+		return dev_err_probe(dev, PTR_ERR(rp1->regmap), "Cannot initialize regmap\n");
+
+	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 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);
+
+	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 = rp1_pwm_remove,
+	.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 <[email protected]>");
+MODULE_AUTHOR("Andrea della Porta <[email protected]>");
+MODULE_LICENSE("GPL");
author maintainer
#7 lore raw

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

In reply to Andrea della Porta

Hi Uwe,

Quoted 19 lines · Andrea della Porta

From: Naushir Patuck <[email protected]>

The Raspberry Pi RP1 southbridge features an embedded PWM controller with 4 output channels, alongside an RPM interface to read the fan speed on the Raspberry Pi 5.

Add the supporting driver.

---
 drivers/pwm/Kconfig   |   9 +
 drivers/pwm/Makefile  |   1 +
 drivers/pwm/pwm-rp1.c | 435 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 445 insertions(+)
 create mode 100644 drivers/pwm/pwm-rp1.c

<...snip...>

Quoted 16 lines
drivers/pwm/pwm-rp1.c new file +13 -0
+	.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 <[email protected]>");
+MODULE_AUTHOR("Andrea della Porta <[email protected]>");
+MODULE_LICENSE("GPL");
Signature · 1 line
2.35.3

I didn't get any feedback after I replied to Sashiko's latest concerns, which I don't think are relevant. Can we proceed with this patch or are there other concerns about it?

Many thanks,
Andrea

#8 lore raw

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

In reply to Andrea della Porta

Hello,

Quoted 4 lines · Andrea della Porta
drivers/pwm/pwm-rp1.c new file +4 -0
+static int rp1_pwm_round_waveform_tohw(struct pwm_chip *chip,
+				       struct pwm_device *pwm,
+				       const struct pwm_waveform *wf,
+				       void *_wfhw)

drivers/pwm/pwm-rp1.c line 137

Quoted 10 lines
drivers/pwm/pwm-rp1.c new file +10 -0
+{
+	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->enabled = false;
+		wfhw->inverted_polarity = (pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED);

pwm_get_polarity(pwm) looks wrong here for several reasons. 1st the polarity is defined in *wf and should not depend on the current state, 2nd for a disabled hardware the polarity doesn't matter anyhow, and 3rd you should not call pwm API functions from the lowlevel driver (which might interfere with subsystem locking).

Also please initialize the whole structure, best done using:

	*wfhw = (typeof(*wfhw)){
		.enabled = false,
	}
Quoted 14 lines
drivers/pwm/pwm-rp1.c new file +14 -0
+		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;

drivers/pwm/pwm-rp1.c line 161

Quoted 10 lines
drivers/pwm/pwm-rp1.c new file +10 -0
+	}
+
+	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) {
+		u64 remainder;
+
+		div64_u64_rem(offset_ticks, period_ticks, &remainder);
+		offset_ticks = remainder;
		offset_ticks = period_ticks - 1;
Quoted 37 lines
drivers/pwm/pwm-rp1.c new file +37 -0
+	}
+	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,

drivers/pwm/pwm-rp1.c line 208

Quoted 10 lines
drivers/pwm/pwm-rp1.c new file +10 -0
+						      (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 = wf->period_length_ns - wf->duty_length_ns;

The duty_offset_ns calculation is wrong.

Consider clk_rate = 3000000, period_ticks = 8, inverted_polarity = true and duty_ticks = 4.

Then you have:

	.period_length_ns = 2666.6666666666666 ns ~> 2667
	.duty_length_ns = 1333.3333333333333 ns -> 1334
	.duty_offset_ns = 1333.3333333333333 ns -> 1334

but .period_length_ns - .duty_length_ns is 1333.

To get this right, you have to calculate

	wf->duty_offset_ns = DIV_ROUND_UP_ULL((u64)(wfhw->period_ticks + 1 - ticks) * NSEC_PER_SEC, clk_rate);
Quoted 16 lines
drivers/pwm/pwm-rp1.c new file +16 -0
+	}
+
+	return 0;
+}
+
+static int rp1_pwm_write_waveform(struct pwm_chip *chip,
+				  struct pwm_device *pwm,
+				  const void *_wfhw)
+{
+	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
+	const struct rp1_pwm_waveform *wfhw = _wfhw;
+	u32 value, ctrl;
+
+	/* set polarity */
+	regmap_read(rp1->regmap, RP1_PWM_CHAN_CTRL(pwm->hwpwm), &value);
+	if (!wfhw->inverted_polarity)

drivers/pwm/pwm-rp1.c line 234

Quoted 10 lines
drivers/pwm/pwm-rp1.c new file +10 -0
+		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);
+		goto exit_disable;

Please don't use goto unless for error handling.

Quoted 29 lines
drivers/pwm/pwm-rp1.c new file +29 -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);
+exit_disable:
+	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));
+

drivers/pwm/pwm-rp1.c line 273

Quoted 10 lines
drivers/pwm/pwm-rp1.c new file +10 -0
+	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->period_ticks = 0;
+		wfhw->duty_ticks = 0;
+	}

Ideally use *wfhw = (typeof(*wfhw)){ ... } here, too, to make it obvious that the complete struct is initialized.

drivers/pwm/pwm-rp1.c line 427

Quoted 12 lines
drivers/pwm/pwm-rp1.c new file +11 -0
+
+	return 0;
+}
[...]
+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),
+		.suppress_bind_attrs = true,

If the driver cannot be removed, .remove() is just dead code. Drop it and note in a comment that this is needed because a syscon cannot be removed.

Best regards
Uwe

author maintainer
#9 lore raw

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

Hi Uwe,

Quoted 15 lines · Uwe Kleine-König

Hello,

Quoted 12 lines · Andrea della Porta
drivers/pwm/pwm-rp1.c new file +12 -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) {
Quoted 8 lines

drivers/pwm/pwm-rp1.c line 137

Quoted 2 lines
drivers/pwm/pwm-rp1.c new file +2 -0
+		wfhw->enabled = false;
+		wfhw->inverted_polarity = (pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED);

pwm_get_polarity(pwm) looks wrong here for several reasons. 1st the polarity is defined in *wf and should not depend on the current state, 2nd for a disabled hardware the polarity doesn't matter anyhow, and 3rd you should not call pwm API functions from the lowlevel driver (which might interfere with subsystem locking).

Ok, but in this case what the output of the disabled channel should be? If it was inverted before the disable, would it make sense to let the output be high? Or are we allowed to hard code a polarity value on disable?

Quoted 6 lines

Also please initialize the whole structure, best done using:

	*wfhw = (typeof(*wfhw)){
		.enabled = false,
	}

Ack. In this case inverted_polarity would be falsei though, see above.

Quoted 19 lines
Quoted 19 lines
drivers/pwm/pwm-rp1.c new file +18 -0
+		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);
Quoted 8 lines

drivers/pwm/pwm-rp1.c line 161

Quoted 6 lines
drivers/pwm/pwm-rp1.c new file +6 -0
+	offset_ticks = mul_u64_u64_div_u64(wf->duty_offset_ns, clk_rate, NSEC_PER_SEC);
+	if (offset_ticks >= period_ticks) {
+		u64 remainder;
+
+		div64_u64_rem(offset_ticks, period_ticks, &remainder);
+		offset_ticks = remainder;
		offset_ticks = period_ticks - 1;

Ack.

Quoted 60 lines
Quoted 38 lines
drivers/pwm/pwm-rp1.c new file +37 -0
+	}
+	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,

drivers/pwm/pwm-rp1.c line 208

Quoted 10 lines
drivers/pwm/pwm-rp1.c new file +10 -0
+						      (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 = wf->period_length_ns - wf->duty_length_ns;

The duty_offset_ns calculation is wrong.

Consider clk_rate = 3000000, period_ticks = 8, inverted_polarity = true and duty_ticks = 4.

Then you have:

	.period_length_ns = 2666.6666666666666 ns ~> 2667
	.duty_length_ns = 1333.3333333333333 ns -> 1334
	.duty_offset_ns = 1333.3333333333333 ns -> 1334
Quoted 5 lines

but .period_length_ns - .duty_length_ns is 1333.

To get this right, you have to calculate

	wf->duty_offset_ns = DIV_ROUND_UP_ULL((u64)(wfhw->period_ticks + 1 - ticks) * NSEC_PER_SEC, clk_rate);

So it will end up as 'duty_length_ns + duty_offset_ns > period_length_ns', which is timing violation. Is it allowed (or even required) in the PWM subsystem?

Quoted 21 lines
Quoted 21 lines
drivers/pwm/pwm-rp1.c new file +20 -0
+	}
+
+	return 0;
+}
+
+static int rp1_pwm_write_waveform(struct pwm_chip *chip,
+				  struct pwm_device *pwm,
+				  const void *_wfhw)
+{
+	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
+	const struct rp1_pwm_waveform *wfhw = _wfhw;
+	u32 value, 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);
Quoted 8 lines

drivers/pwm/pwm-rp1.c line 234

Quoted 6 lines
drivers/pwm/pwm-rp1.c new file +6 -0
+
+	/* 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);
+		goto exit_disable;

Please don't use goto unless for error handling.

Ack.

Quoted 35 lines
Quoted 35 lines
drivers/pwm/pwm-rp1.c new file +34 -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);
+exit_disable:
+	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);
Quoted 8 lines

drivers/pwm/pwm-rp1.c line 273

Quoted 5 lines
drivers/pwm/pwm-rp1.c new file +5 -0
+		regmap_read(rp1->regmap, RP1_PWM_DUTY(pwm->hwpwm), &wfhw->duty_ticks);
+	} else {
+		wfhw->period_ticks = 0;
+		wfhw->duty_ticks = 0;
+	}

Ideally use *wfhw = (typeof(*wfhw)){ ... } here, too, to make it obvious that the complete struct is initialized.

Ack.

Quoted 17 lines

drivers/pwm/pwm-rp1.c line 427

Quoted 13 lines
drivers/pwm/pwm-rp1.c new file +11 -0
+
+	return 0;
+}
[...]
+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),
+		.suppress_bind_attrs = true,

If the driver cannot be removed, .remove() is just dead code. Drop it and note in a comment that this is needed because a syscon cannot be removed.

Ack.

Many thanks,
Andrea

Quoted 3 lines

Best regards
Uwe

#10 lore raw

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

Hello Andrea,

Quoted 23 lines · Andrea della Porta
Quoted 13 lines · Uwe Kleine-König
Quoted 12 lines · Andrea della Porta
drivers/pwm/pwm-rp1.c new file +12 -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) {
Quoted 8 lines

drivers/pwm/pwm-rp1.c line 137

Quoted 2 lines
drivers/pwm/pwm-rp1.c new file +2 -0
+		wfhw->enabled = false;
+		wfhw->inverted_polarity = (pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED);

pwm_get_polarity(pwm) looks wrong here for several reasons. 1st the polarity is defined in *wf and should not depend on the current state, 2nd for a disabled hardware the polarity doesn't matter anyhow, and 3rd you should not call pwm API functions from the lowlevel driver (which might interfere with subsystem locking).

Quoted 3 lines

Ok, but in this case what the output of the disabled channel should be? If it was inverted before the disable, would it make sense to let the output be high? Or are we allowed to hard code a polarity value on disable?

Given that not all hardwares can drive the output of a disabled PWM to a fixed level, a consumer that relies on a fixed inactive level output must not disable the PWM. So it doesn't matter what you do on wf->period_length_ns == 0, the only objective is to save power.

Quoted 26 lines
Quoted 26 lines

drivers/pwm/pwm-rp1.c line 208

Quoted 12 lines
drivers/pwm/pwm-rp1.c new file +12 -0
+	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 = wf->period_length_ns - wf->duty_length_ns;

The duty_offset_ns calculation is wrong.

Consider clk_rate = 3000000, period_ticks = 8, inverted_polarity = true and duty_ticks = 4.

Then you have:

	.period_length_ns = 2666.6666666666666 ns ~> 2667
	.duty_length_ns = 1333.3333333333333 ns -> 1334
	.duty_offset_ns = 1333.3333333333333 ns -> 1334

but .period_length_ns - .duty_length_ns is 1333.

Quoted 7 lines
Quoted 3 lines

To get this right, you have to calculate

	wf->duty_offset_ns = DIV_ROUND_UP_ULL((u64)(wfhw->period_ticks + 1 - ticks) * NSEC_PER_SEC, clk_rate);

So it will end up as 'duty_length_ns + duty_offset_ns > period_length_ns', which is timing violation. Is it allowed (or even required) in the PWM subsystem?

Not all hardwares support that, but logically it makes sense. The active phase of the output just crosses the period border. With duty_length_ns
+ duty_offset_ns < period_length_ns (and duty_offset > 0) it's the inactive phase that crosses the period border, and there is no reason why the active phase should be more special than the inactive one.

Best regards
Uwe

author maintainer
#11 lore raw

PATCH v6 3/3 arm64: dts: broadcom: rpi-5: Add RP1 PWM node

In reply to Andrea della Porta

From: Stanimir Varbanov <[email protected]>

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.

---
 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(+)
arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts +12 -0
index 0fc57e72632ed..748be8f1ee9e2 100644
@@ -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";
arch/arm64/boot/dts/broadcom/rp1-common.dtsi +9 -0
index 16f5359395835..df4c2d09c8d34 100644
@@ -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";
 	};
 
Signature · 1 line
2.35.3
Reviewed-by on v6 3/3
#12 lore raw

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

Quoted 10 lines · Andrea della Porta

From: Stanimir Varbanov <[email protected]>

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.

Quoted 56 lines
---
  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(+)
arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts +12 -0
index 0fc57e72632ed..748be8f1ee9e2 100644
@@ -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";
arch/arm64/boot/dts/broadcom/rp1-common.dtsi +9 -0
index 16f5359395835..df4c2d09c8d34 100644
@@ -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";
  	};