Add RP1 PWM controller support
Subsystems (4)
Andrea della Porta
maintainerv6 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
Andrea della Porta
maintainerPATCH 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
@@ -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
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.
Signed-off-by Naushir Patuck <[email protected]>Co-developed-by Stanimir Varbanov <[email protected]>Signed-off-by Stanimir Varbanov <[email protected]>Signed-off-by Andrea della Porta <[email protected]>Reviewed-by Krzysztof Kozlowski <[email protected]>
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.yamlDocumentation/devicetree/bindings/pwm/raspberrypi,rp1-pwm.yaml new file +66
-0@@ -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>; + };
Andrea della Porta
maintainerPATCH v6 2/3 pwm: rp1: Add RP1 PWM controller driver
In reply to Andrea della PortaFrom: 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
@@ -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
@@ -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
[email protected]
AI reviewRe: [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@@ -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
Andrea della Porta
maintainerRe: [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.
Signed-off-by Naushir Patuck <[email protected]>Co-developed-by Stanimir Varbanov <[email protected]>Signed-off-by Stanimir Varbanov <[email protected]>Signed-off-by Andrea della Porta <[email protected]>Quoted 6 lines
drivers/pwm/pwm-rp1.c new file +0
-0@@ -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]
Inrp1_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()andregmap_write(), which might allow another syscon client's changes toRP1_PWM_GLB_CTRLto be overwritten with stale data. Does this need to useregmap_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]
Doesrp1_pwm_write_waveform()present the same non-atomic read-modify-write race condition here when updatingRP1_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 inrp1_pwm_write_waveform()forRP1_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]
Inrp1_pwm_probe(), becauserp1->regmapis managed by devres (allocated viadevm_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
Re: [PATCH v6 2/3] pwm: rp1: Add RP1 PWM controller driver
In reply to Andrea della PortaQuoted 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.
Signed-off-by Naushir Patuck <[email protected]>Co-developed-by Stanimir Varbanov <[email protected]>Signed-off-by Stanimir Varbanov <[email protected]>Signed-off-by Andrea della Porta <[email protected]>
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.cdrivers/pwm/Kconfig +9
-0@@ -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_TESTdrivers/pwm/Makefile +1
-0@@ -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.odrivers/pwm/pwm-rp1.c new file +435
-0@@ -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" );
Andrea della Porta
maintainerRe: [PATCH v6 2/3] pwm: rp1: Add RP1 PWM controller driver
In reply to Andrea della PortaHi 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.
Signed-off-by Naushir Patuck <[email protected]>Co-developed-by Stanimir Varbanov <[email protected]>Signed-off-by Stanimir Varbanov <[email protected]>Signed-off-by Andrea della Porta <[email protected]>--- 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
Uwe Kleine-König
maintainerRe: [PATCH v6 2/3] pwm: rp1: Add RP1 PWM controller driver
In reply to Andrea della PortaHello,
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
Andrea della Porta
maintainerRe: [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_nscalculation is wrong.Consider
clk_rate= 3000000,period_ticks= 8,inverted_polarity= true andduty_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
Uwe Kleine-König
maintainerRe: [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_nscalculation is wrong.Consider
clk_rate= 3000000,period_ticks= 8,inverted_polarity= true andduty_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 -> 1334but .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
Andrea della Porta
maintainerPATCH v6 3/3 arm64: dts: broadcom: rpi-5: Add RP1 PWM node
In reply to Andrea della PortaFrom: 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
@@ -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
@@ -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
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.
Signed-off-by Stanimir Varbanov <[email protected]>Co-developed-by Andrea della Porta <[email protected]>Signed-off-by Andrea della Porta <[email protected]>
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@@ -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@@ -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"; };