Skip to contentSkip to conversation

Add RP1 PWM controller support

PATCH Superseded Andrea della Porta 4 participants 8 messages Sep 28 – Oct 16

Subsystems (4)

v11 of 12 Superseded Latest is v12 →

lore
Share & tools
lore

Andrea della Porta

author maintainer
#0 lore raw

v11 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 will be exported by this driver via a function. A subsequent patch will add the function to read the speed to this driver and 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 V11:

- capped duty_cycle in rp1_pwm_round_waveform_fromhw()
- fixed a conditional in probe function dealing with an unitialized local variable (base instead of rp1->base). As a result, drop the unused local.

Link to V10:
https://lore.kernel.org/all/[email protected]/

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                           |   7 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-rp1.c                         | 417 ++++++++++++++++++
 6 files changed, 512 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 v10 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..be020583fd0ba
@@ -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.
+
+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
author maintainer
#2 lore raw

PATCH v10 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   |   7 +
 drivers/pwm/Makefile  |   1 +
 drivers/pwm/pwm-rp1.c | 417 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 425 insertions(+)
 create mode 100644 drivers/pwm/pwm-rp1.c
drivers/pwm/Kconfig +7 -0
index 7297760868790..523789e82355e 100644
@@ -637,6 +637,13 @@ 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
+	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 +417 -0

417 lines not on this page · raw message

Signature · 1 line
2.35.3
#3 lore raw

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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] pwm: rp1: channel control register state loss across suspend --

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] pwm: rp1: channel control register state loss across suspend
--

commit b77b6f077f5aa3ca8ba16d313a2e9952c97e47f7
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. This patch adds the supporting driver.

Quoted 5 lines
drivers/pwm/pwm-rp1.c new file +0 -0
new file mode 100644
index 0000000000000..a993b86a5bf85

[ ... ]

drivers/pwm/pwm-rp1.c line 115

Quoted 10 lines
drivers/pwm/pwm-rp1.c new file +10 -0
+static int rp1_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
+	u32 value, polarity;
+
+	/* init channel to reset defaults, preserving the polarity bit */
+	value = readl(rp1->base + RP1_PWM_CHAN_CTRL(pwm->hwpwm));
+	polarity = FIELD_GET(RP1_PWM_CHAN_CTRL_POLARITY, value);
+	value = u32_replace_bits(RP1_PWM_CHAN_CTRL_DEFAULT, polarity, RP1_PWM_CHAN_CTRL_POLARITY);
+	writel(value, rp1->base + RP1_PWM_CHAN_CTRL(pwm->hwpwm));

[Severity: Medium]
Will this channel control mode configuration be lost if the hardware loses power or is reset during system suspend?

The required MODE_TE_MS and FIFO_POP_MASK settings are initialized here, but they do not appear to be restored anywhere on resume.

Quoted 3 lines
drivers/pwm/pwm-rp1.c new file +3 -0
+
+	return 0;
+}

[ ... ]

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 224

Quoted 10 lines
drivers/pwm/pwm-rp1.c new file +10 -0
+	const struct rp1_pwm_waveform *wfhw = _wfhw;
+	u32 value, ctrl;
+
+	/* set polarity */
+	value = readl(rp1->base + RP1_PWM_CHAN_CTRL(pwm->hwpwm));
+	if (!wfhw->inverted_polarity)
+		value &= ~RP1_PWM_CHAN_CTRL_POLARITY;
+	else
+		value |= RP1_PWM_CHAN_CTRL_POLARITY;
+	writel(value, rp1->base + RP1_PWM_CHAN_CTRL(pwm->hwpwm));

[Severity: Medium]
If the channel control register loses its state across suspend, could this read-modify-write operation preserve the corrupted state?

If a PWM consumer calls pwm_apply_state() upon resume, it would invoke rp1_pwm_write_waveform() and read the potentially zeroed state, modify only the polarity bit, and write the corrupted state back. This would cause the PWM channel to fail to output correctly since the proper mode settings are missing.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/ee0aa89faa14e0073f1271693bae28678652998b.1790606188.git.andrea.porta@suse.com?part=2
author maintainer
#4 lore raw

PATCH v10 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
#5 lore raw

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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] pwm: rp1: missing spinlock for shared RP1_PWM_GLB_CTRL register --

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pwm: rp1: missing spinlock for shared RP1_PWM_GLB_CTRL register
--

commit 0c157fdfbde9f74961d4b31ce35f1c71872efa00
Author: Stanimir Varbanov <[email protected]>

arm64: dts: broadcom: rpi-5: Add RP1 PWM node

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 4 lines
arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts +0 -0
index 0fc57e72632ed..748be8f1ee9e2 100644

[ ... ]

arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts &rp1_i2c6 { · line 107

Quoted 9 lines
arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts +5 -0
@@ -94,6 +100,12 @@ &rp1_i2c6 {
 	pinctrl-names = "default";
 };
 
+&rp1_pwm1 {
+	pinctrl-0 = <&fan_pwm_default_state>;
+	pinctrl-names = "default";
+	status = "okay";
+};

[Severity: High]
Can concurrent PWM channel configuration cause lost updates when this node is enabled?

Looking at drivers/pwm/pwm-rp1.c, there is a read-modify-write race on the shared hardware register RP1_PWM_GLB_CTRL:

drivers/pwm/pwm-rp1.c:rp1_pwm_write_waveform() {
    ...
	/* enable the channel */
	ctrl |= RP1_PWM_GLB_CTRL_CHANNEL_ENABLE(pwm->hwpwm);
	writel(ctrl,  rp1->base + RP1_PWM_GLB_CTRL);
    ...
}

and also in:

drivers/pwm/pwm-rp1.c:rp1_pwm_apply_config() {
    ...
	value = readl(rp1->base + RP1_PWM_GLB_CTRL);
	value |= RP1_PWM_GLB_CTRL_SET_UPDATE;
	writel(value, rp1->base + RP1_PWM_GLB_CTRL);
    ...
}

If two threads call pwm_apply_state() on different PWM channels concurrently, the PWM core locks at the channel level, not the chip level.

One thread's enable/disable action could overwrite the other's, leading to channels unexpectedly disabling or failing to enable.

Since this PWM drives the cooling fan, could a lost update disabling the fan lead to a thermal shutdown?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/ee0aa89faa14e0073f1271693bae28678652998b.1790606188.git.andrea.porta@suse.com?part=3
maintainer Applied
#6 lore raw

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

In reply to Andrea della Porta
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.

Applied to devicetre-arm64/next, thanks!

Signature · 1 line
Florian
maintainer
#7 lore raw

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

Quoted 7 lines · Florian Fainelli
Quoted 5 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.

Quoted 7 lines
Quoted 5 lines

Add the corresponding DT node for this PWM controller.

Applied to devicetre-arm64/next, thanks!

And that introduces warnings because the PWM binding is not applied. Either wait for the binding to be applied or apply the binding with the .dts change. IOW, Broadcom is warning free (you're welcome), don't introduce warnings.

Rob