Update designware pwm driver
Subsystems (2)
v10 cover Update designware pwm driver
From: Xuyang Dong <[email protected]>
There is already a patch [1] for the DesignWare PWM driver, which is posted by Ben and still under review. Based on this patch, this series is a continuation of [1] to add support for IP versions 2.11a and later, which includes support for "Pulse Width Modulation with 0% and 100% Duty Cycle".
Supported chips:
ESWIN EIC7700 series SoC.
Test:
Tested this patch on the Sifive HiFive Premier P550 (which uses the EIC7700 SoC).
[1] https://lore.kernel.org/lkml/[email protected]/
Updates:
Changes in v10:
- Driver:
- Do not add a dependency on PM. Additionally, the issues below will be fixed to ensure compatibility with and without PM support (Sashiko review of v9).
- Note that writes to dwc->clk_rate in apply()/get_state() cannot race due to PWM core's chip->nonatomic_lock (Sashiko review of v9).
- Fix __dwc_pwm_configure_timer() precision loss from dual rounding (compute period_cyc once); reject sub-clock-cycle periods in 0N100 mode (add !period_cyc check to avoid writing 0 to LD_CNT/LD_CNT2).
- dwc_pwm_get_state():
Fix pm_runtime_resume_and_get() error check to use 'if (ret < 0)' to allow stub return value 1 when CONFIG_PM=n.
- Deassert reset before reading MMIO in probe() to avoid bus stalls on SoCs that leave the block in reset (Sashiko review of v9).
- Keep clocks enabled when CONFIG_PM=n.
Condition the clock gating on IS_ENABLED(CONFIG_PM) (Sashiko review of v9).
- Move register save/restore into runtime PM callbacks. Replace legacy suspend/resume with pm_runtime_force_suspend()/resume() to properly manage the generic power domain.
- Link to v9: https://lore.kernel.org/all/[email protected]/
Changes in v9:
- YAML:
- Add 'Acked-by: Conor Dooley <[email protected]>' for patch 1 and 2.
- Driver:
- __dwc_pwm_configure_timer():
reads state->polarity and swaps registers:
NORMAL (active-high): duty_cycle to DWC_TIM_LD_CNT2 (HIGH), remainder to DWC_TIM_LD_CNT (LOW)
INVERSED (active-low): duty_cycle to DWC_TIM_LD_CNT (LOW), remainder to DWC_TIM_LD_CNT2 (HIGH)
Applies to both 0N100 and classic paths.
dwc_pwm_apply():
removed -EINVAL polarity guard; both polarities accepted. dwc_pwm_get_state():
reconstructs polarity from pwm->state.polarity (last apply() value); initial read zero-initialized to PWM_POLARITY_NORMAL; decode mirrors apply() swap logic for consistent round-trip (Sashiko review of v8).
- Replace pm_runtime_get_sync() with pm_runtime_resume_and_get() in dwc_pwm_apply() and add error checking (Sashiko review of v8).
- Add a non-zero check for clk_rate in dwc_pwm_get_state(). Add a non-zero check for dwc->clk_rate in probe (Sashiko review of v8).
- When pwm_en is true, adding 'goto disable_clk;'. When pwm_en is false, return early from pm_disable without falling through to disable_clk. (Sashiko review of v8).
- Replace pm_runtime_get_sync() with pm_runtime_resume_and_get() in dwc_pwm_plat_remove() and add error checking.
On failure, skip register reads and pm_runtime_put_sync() (Sashiko review of v8).
- Remove the -EBUSY check in dwc_pwm_suspend() and save all contexts (Sashiko review of v8).
- Replace pm_runtime_get_sync() with pm_runtime_resume_and_get() in dwc_pwm_resume() (Sashiko review of v8).
- Link to v8: https://lore.kernel.org/all/[email protected]/
Changes in v8:
- YAML:
- Split the v7 binding into two patches.
Patch 1 explains why to add the resets property.
Patch 2 adds the eswin compatible string and specified reset.
- Driver:
- Use mul_u64_u64_div_u64() to safely scale the values and avoid 64-bit multiplication overflow in __dwc_pwm_configure_timer() and dwc_pwm_get_state().
Add the include for linux/math64.h (Sashiko review of v7).
- Keep the current usage of pwm->args.polarity until a better solution is available.
- Use pm_runtime_resume_and_get() in dwc_pwm_get_state() instead of pm_runtime_get_sync(), so that register access is skipped if the device fails to resume (Sashiko review of v7).
- Replace devm_pwmchip_add() with pwmchip_add() and move it after pm_runtime_enable(), so that the PWM chip is registered only after runtime PM has been fully initialized (Sashiko review of v7).
- Remove the reset_assert label and reset_control_assert() (Sashiko review of v7).
- Remove the pm_runtime_status_suspended() check and unconditionally use pm_runtime_get_sync() instead (Sashiko review of v7).
- Remove the pwm_en flag, but keep the pm_runtime_put_noidle() call (Sashiko review of v7; see email for explanation)
- Use pm_runtime_status_suspended() to check the runtime PM status. If the device is not suspended (i.e., active), call clk_disable_unprepare(). If it is suspended, skip this block (Sashiko review of v7).
- Use an explicit pwmchip_remove() as the first step of .remove(), instead of relying on devm_pwmchip_add() to unregister the chip after .remove() returns. This prevents the hardware teardown that follows from racing against a still-registered chip (Sashiko review of v7).
- Add a check for dwc->rst before asserting reset in the remove path (Sashiko review of v7).
- Drop the return value check from pm_runtime_put_sync() (Sashiko review of v7).
- Link to v7: https://lore.kernel.org/all/[email protected]/
Changes in v7:
- YAML:
- Dropped Conor's Acked-by due to significant schema changes.
- Rename patch 1 from "dt-bindings: pwm: dwc: add optional reset" to "dt-bindings: pwm: dwc: Add eswin compatible and resets property".
- Update the commit message to explain why the EIC7700 supports only one reset.
- Add constraints 'minItems: 1' and 'maxItems: 1' for the 'resets' property of eswin,eic7700-pwm.
- Add an example for eswin,eic7700-pwm.
- Link to v6: https://lore.kernel.org/all/[email protected]/
Changes in v6:
- YAML:
- Drop properties resets and its items description for eswin,eic7700-pwm.
- Link to v5: https://lore.kernel.org/all/[email protected]/
Changes in v5:
- YAML:
- Add 'eswin,eic7700-pwm' compatible string.
- Add the items description for the resets property and set minItems to 1.
- Require resets property with exactly 1 reset for eswin,eic7700-pwm compatible.
- Driver:
- Add support for 'eswin,eic7700-pwm' compatible.
- Add structure dwc_pwm_plat_data to manage the API for obtaining resets.
- Link to v4: https://lore.kernel.org/all/[email protected]/
Changes in v4:
- YAML:
- Change maxItems from 1 to 2. As there is a corresponding reset signal for each clock domain, the effective maxItems of the resets property is set to 2.
- Update the YAML commit message to describe the hardware.
- Driver:
- Replace devm_reset_control_get_optional_exclusive() with devm_reset_control_array_get_optional_exclusive(). Since the number of reset signals has increased from one to two, we need to use the array API to acquire them.
- Link to v3: https://lore.kernel.org/all/[email protected]/
Changes in v3:
- YAML:
- Added a clear justification for the optional resets property. It is required to support proper controller initialization when no PWM channel is active at boot time, while allowing the driver to skip reset deassertion if any channel is already enabled.
- Driver:
- Update the boundary value check of tmp in __dwc_pwm_configure_timer() for DWC_TIM_CTRL_0N100PWM_EN.
- Replace 'sizeof(struct dwc_pwm_drvdata)' with 'struct_size(data, chips, 1)'.
- Drop devm_clk_get_enabled() in favor of devm_clk_get() with explicit clk_prepare_enable() and clk_disable_unprepare() allowing runtime PM to manage clock state.
- Replace devm_reset_control_get_optional_exclusive_deasserted() with devm_reset_control_get_optional_exclusive() and issue a full reset via reset_control_reset() only when no PWM channel is active at probe time.
- Detect bootloader-enabled PWM channels by reading the enable bit, and initialize runtime PM as active for those channels by calling pm_runtime_set_active() and pm_runtime_get_noresume().
- Remove autosuspend as it is not required for this driver.
- Use explicit pm_runtime_enable() and pm_runtime_disable() instead of the managed devm_pm_runtime_enable() variant to ensure correct cleanup.
- On device removal, recheck the channel enable status. If any channel remains active, call pm_runtime_put_noidle() before disabling clocks via clk_disable_unprepare().
Resume device before register access during removal if it is runtime suspended, and re-suspend it afterward.
- If device is suspended, resume it before register access during system resume/suspend.
- Use pm_ptr() instead of pm_sleep_ptr() for correct PM operation.
- Link to v2: https://lore.kernel.org/all/[email protected]/
Changes in v2:
- YAML:
- Remove eswin,eic7700-pwm.yaml. Use snps,dw-apb-timers-pwm2.yaml. The description in snps,dw-apb-timers-pwm2.yaml is better.
- Add the resets property as optional, as defined in the databook.
- Remove snps,pwm-full-range-enable as no additional property is needed.
- Driver:
- Change the file from pwm-dwc-eic7700.c to pwm-dwc-of.c from [1].
- Define DWC_TIM_VERSION_ID_2_11A 2.11a as the baseline version.
- Enable the 0% and 100% duty cycle mode by setting dwc->feature if the version read from the TIMERS_COMP_VERSION register is later than or equal to DWC_TIM_VERSION_ID_2_11A.
- Use the DIV_ROUND_UP_ULL() to calculate width in the .apply and .get_state.
- Additionally, Power Management (PM) support has been added to the pwm-dwc-of.c driver.
- Drop the headers that are not used.
- Use devm_clk_get_enabled() instead of devm_clk_get().
- Drop of_match_ptr.
- Fix build error with 1ULL << 32.
- Link to v1: https://lore.kernel.org/all/[email protected]/
- Link to v9: https://lore.kernel.org/lkml/[email protected]/
Xuyang Dong (3):
dt-bindings: pwm: dwc: Document optional resets property
dt-bindings: pwm: dwc: Add eswin compatible
pwm: dwc: add of/platform support
.../bindings/pwm/snps,dw-apb-timers-pwm2.yaml | 37 +- drivers/pwm/Kconfig | 10 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-dwc-core.c | 161 +++++++-- drivers/pwm/pwm-dwc-of.c | 316 ++++++++++++++++++ drivers/pwm/pwm-dwc.h | 25 +- 6 files changed, 507 insertions(+), 43 deletions(-) create mode 100644 drivers/pwm/pwm-dwc-of.c
--
2.34.1
PATCH v10 1/3 dt-bindings: pwm: dwc: Document optional resets property
The DesignWare PWM IP has two active-low reset inputs: presetn resets the register interface logic in the pclk (bus) domain, and timer_N_resetn resets the counter/timer logic in the timer_N_clk domain. The existing snps,dw-apb-timers-pwm2 binding does not describe either of these lines.
Add the resets property and describe the function of each reset to support future use of resets.
--- .../devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml | 5 +++++ 1 file changed, 5 insertions(+)
Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml +5 -0
@@ -43,6 +43,11 @@ properties: - const: bus - const: timer + resets: + items: + - description: Interface bus (presetn) reset + - description: PWM timer logic (timer_N_resetn) reset + snps,pwm-number: $ref: /schemas/types.yaml#/definitions/uint32 description: The number of PWM channels configured for this instance
Signature · 1 line
2.34.1
[email protected]
AI reviewRe: [PATCH v10 1/3] dt-bindings: pwm: dwc: Document optional resets property
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] Missing matching updates for related bindings in the same IP family. --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing matching updates for related bindings in the same IP family. --
commit 05dd50c982c598b86944765d91f297a8c37b303e
Author: Xuyang Dong <[email protected]>
dt-bindings: pwm: dwc: Document optional resets property
This commit updates the DesignWare PWM binding to document two active-low reset inputs: presetn and timer_N_resetn. It adds the resets property with two items to describe the function of each reset line.
Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml properties: · line 50
Quoted 12 lines
Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml +5
-0@@ -43,6 +43,11 @@ properties: - const: bus - const: timer + resets: + items: + - description: Interface bus (presetn) reset + - description: PWM timer logic (timer_N_resetn) reset +
[Severity: Medium]
The subsystem guidelines note that when a binding has multiple YAML files for different device types in the same family, related files may need matching updates. Since the DesignWare APB timer IP is described by both this PWM binding and the timer binding, does the timer binding also need an update?
Currently, the timer binding restricts the resets property to a single item:
Documentation/devicetree/bindings/timer/snps,dw-apb-timer.yaml:properties:resets
resets:
maxItems: 1
Defining both hardware resets for the snps,dw-apb-timer compatible will cause dt_binding_check to fail due to this inconsistency.
Quoted 3 lines
Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml +0
-0snps,pwm-number: $ref: /schemas/types.yaml#/definitions/uint32 description: The number of PWM channels configured for this instance
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Re: Re: [PATCH v10 1/3] dt-bindings: pwm: dwc: Document optional resets property
Quoted 33 lines
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing matching updates for related bindings in the same IP family. --commit
05dd50c982c598b86944765d91f297a8c37b303e
Author: Xuyang Dong <[email protected]>dt-bindings: pwm: dwc: Document optional resets property
This commit updates the DesignWare PWM binding to document two active-low reset inputs: presetn and
timer_N_resetn. It adds the resets property with two items to describe the function of each reset line.Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml properties: · line 50
Quoted 12 lines
Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml +5
-0@@ -43,6 +43,11 @@ properties: - const: bus - const: timer + resets: + items: + - description: Interface bus (presetn) reset + - description: PWM timer logic (timer_N_resetn) reset +[Severity: Medium]
The subsystem guidelines note that when a binding has multiple YAML files for different device types in the same family, related files may need matching updates. Since the DesignWare APB timer IP is described by both this PWM binding and the timer binding, does the timer binding also need an update?
Quoted 9 lines
Currently, the timer binding restricts the resets property to a single item:
Documentation/devicetree/bindings/timer/snps,dw-apb-timer.yaml:properties:resets
resets:
maxItems: 1Defining both hardware resets for the snps,dw-apb-timer compatible will cause
dt_binding_checkto fail due to this inconsistency.
Hi Sashiko,
Thanks for the careful review.
This is not a binding inconsistency issue:
- We're describing different hardware usage scenarios (PWM vs clocksource)
- The PWM binding describes what the PWM driver actually needs
- The timer binding describes what the timer driver actually needs
- Both reflect their respective hardware requirements accurately
I don't think the number of items in the resets property of the timer binding should be updated. Do you think this is acceptable?
Best regards,
Xuyang Dong
Quoted 6 lines
Quoted 3 lines
Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml +0
-0snps,pwm-number: $ref: /schemas/types.yaml#/definitions/uint32 description: The number of PWM channels configured for this instanceSignature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
PATCH v10 2/3 dt-bindings: pwm: dwc: Add eswin compatible
In reply to [email protected]EIC7700 integrates the DesignWare PWM IP described by the generic snps,dw-apb-timers-pwm2 binding. On this SoC, the presetn and timer_N_resetn inputs are physically tied together to a single reset line, so exactly one reset is both required and sufficient, unlike the generic binding where up to two independent lines are optional.
Add the eswin,eic7700-pwm compatible string and constrain its resets property to exactly one entry.
--- .../bindings/pwm/snps,dw-apb-timers-pwm2.yaml | 32 ++++++++++++++++--- 1 file changed, 28 insertions(+), 4 deletions(-)
Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml +28 -4
@@ -20,12 +20,11 @@ description: instead of having to encode the IP version number in the device tree compatible. -allOf: - - $ref: pwm.yaml# - properties: compatible: - const: snps,dw-apb-timers-pwm2 + enum: + - snps,dw-apb-timers-pwm2 + - eswin,eic7700-pwm reg: maxItems: 1
@@ -44,6 +43,7 @@ properties: - const: timer resets: + minItems: 1 items: - description: Interface bus (presetn) reset - description: PWM timer logic (timer_N_resetn) reset
@@ -59,6 +59,21 @@ required: - clocks - clock-names +allOf: + - $ref: pwm.yaml# + + - if: + properties: + compatible: + contains: + const: eswin,eic7700-pwm + then: + properties: + resets: + maxItems: 1 + required: + - resets + additionalProperties: false examples:
@@ -70,3 +85,12 @@ examples: clocks = <&bus>, <&timer>; clock-names = "bus", "timer"; }; + - | + pwm@50818000 { + compatible = "eswin,eic7700-pwm"; + reg = <0x50818000 0x4000>; + #pwm-cells = <3>; + clocks = <&bus>, <&timer>; + clock-names = "bus", "timer"; + resets = <&reset>; + };
Signature · 1 line
2.34.1
PATCH v10 3/3 pwm: dwc: add of/platform support
In reply to [email protected]From: Xuyang Dong <[email protected]>
The dwc pwm controller can be used in non-PCI systems, so allow either platform or OF based probing.
The controller is reset only when no PWM channel is enabled. Otherwise, clocks are enabled and the runtime PM state is updated to reflect the active hardware configuration.
The DWC PWM controller does not provide a hardware polarity bit. Currently, the driver only supports active-low output, which is incompatible with devices requiring active-high waveforms (e.g., backlight controllers, fan speed regulators).
Implement polarity control by exploiting the timer's dual load registers. The hardware uses:
- LD_CNT: LOW period count
- LD_CNT2: HIGH period count
The total period is defined as (LD_CNT + LD_CNT2). By swapping the duty cycle between these registers, we invert the polarity while keeping the period unchanged:
- PWM_POLARITY_NORMAL: write duty_cycle to LD_CNT2 (HIGH period)
- PWM_POLARITY_INVERSED: write duty_cycle to LD_CNT (LOW period)
Implementation:
Update both apply() and get_state() to handle state->polarity consistently. Since the hardware does not store polarity, get_state() returns the last successfully applied software state, ensuring that read-back matches what was originally set.
--- drivers/pwm/Kconfig | 10 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-dwc-core.c | 161 +++++++++++++++---- drivers/pwm/pwm-dwc-of.c | 316 +++++++++++++++++++++++++++++++++++++ drivers/pwm/pwm-dwc.h | 25 ++- 5 files changed, 474 insertions(+), 39 deletions(-) create mode 100644 drivers/pwm/pwm-dwc-of.c
drivers/pwm/Kconfig +10 -0
@@ -249,6 +249,16 @@ config PWM_DWC To compile this driver as a module, choose M here: the module will be called pwm-dwc. +config PWM_DWC_OF + tristate "DesignWare PWM Controller (OF bus)" + depends on HAS_IOMEM && (OF || COMPILE_TEST) + select PWM_DWC_CORE + help + PWM driver for Synopsys DWC PWM Controller on an OF bus or + a platform bus. + To compile this driver as a module, choose M here: the module + will be called pwm-dwc-of. + config PWM_EP93XX tristate "Cirrus Logic EP93xx PWM support" depends on ARCH_EP93XX || COMPILE_TEST
drivers/pwm/Makefile +1 -0
@@ -20,6 +20,7 @@ obj-$(CONFIG_PWM_CRC) += pwm-crc.o obj-$(CONFIG_PWM_CROS_EC) += pwm-cros-ec.o obj-$(CONFIG_PWM_DWC_CORE) += pwm-dwc-core.o obj-$(CONFIG_PWM_DWC) += pwm-dwc.o +obj-$(CONFIG_PWM_DWC_OF) += pwm-dwc-of.o obj-$(CONFIG_PWM_EP93XX) += pwm-ep93xx.o obj-$(CONFIG_PWM_FSL_FTM) += pwm-fsl-ftm.o obj-$(CONFIG_PWM_GPIO) += pwm-gpio.o
drivers/pwm/pwm-dwc-core.c +129 -32
228 lines not on this page · raw message
drivers/pwm/pwm-dwc-of.c new file +316 -0
316 lines not on this page · raw message
drivers/pwm/pwm-dwc.h +18 -7
@@ -26,12 +26,19 @@ MODULE_IMPORT_NS("dwc_pwm");#define DWC_TIMERS_TOTAL 8/* Timer Control Register */ -#define DWC_TIM_CTRL_EN BIT(0) -#define DWC_TIM_CTRL_MODE BIT(1) -#define DWC_TIM_CTRL_MODE_FREE (0 << 1) -#define DWC_TIM_CTRL_MODE_USER (1 << 1) -#define DWC_TIM_CTRL_INT_MASK BIT(2) -#define DWC_TIM_CTRL_PWM BIT(3) +#define DWC_TIM_CTRL_EN BIT(0) +#define DWC_TIM_CTRL_MODE BIT(1) +#define DWC_TIM_CTRL_MODE_FREE (0 << 1) +#define DWC_TIM_CTRL_MODE_USER BIT(1) +#define DWC_TIM_CTRL_INT_MASK BIT(2) +#define DWC_TIM_CTRL_PWM BIT(3) +#define DWC_TIM_CTRL_0N100PWM_EN BIT(4) + +/* +* The version 2.11a and later add "Pulse Width Modulation with +* 0% and 100% Duty Cycle". +*/ +#define DWC_TIM_VERSION_ID_2_11A 0x3231312astruct dwc_pwm_info {unsigned int nr;
@@ -52,8 +59,12 @@ struct dwc_pwm_ctx {struct dwc_pwm {void __iomem *base; -unsigned int clk_ns; +struct clk *bus_clk; +struct clk *clk; +unsigned long clk_rate; +struct reset_control *rst;struct dwc_pwm_ctx ctx[DWC_TIMERS_TOTAL ]; +u32 features; };static inline struct dwc_pwm *to_dwc_pwm (struct pwm_chip *chip)
Signature · 1 line
2.34.1
[email protected]
AI reviewRe: [PATCH v10 3/3] pwm: dwc: add of/platform support
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] Hardware reset is asserted after gating clocks, which can cause bus hangs.
- [High] Race condition between runtime PM check and disable causes a permanent clock leak.
Pre-existing issues:
- [High] Unmasking the timer interrupt without an IRQ handler can cause a severe interrupt storm. --
commit a50609ba96938c02fbc94f54318c95561f661758
Author: Xuyang Dong <[email protected]>
Subject: pwm: dwc: add of/platform support
The dwc pwm controller can be used in non-PCI systems, so allow either platform or OF based probing. Implements polarity control by exploiting the timer's dual load registers.
Quoted 4 lines
drivers/pwm/pwm-dwc-core.c +0
-0
[ ... ]
drivers/pwm/pwm-dwc-core.c static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc, · line 144
Quoted 8 lines
drivers/pwm/pwm-dwc-core.c +3
-0@@ -85,6 +138,9 @@ static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc,* periods are set by Load Count registers. */ ctrl =DWC_TIM_CTRL_MODE_USER |DWC_TIM_CTRL_PWM ; +if (dwc->features &DWC_TIM_CTRL_0N100PWM_EN ) + ctrl |=DWC_TIM_CTRL_0N100PWM_EN ; +dwc_pwm_writel (dwc, ctrl,DWC_TIM_CTRL (pwm->hwpwm));
[Severity: High]
This isn't a bug introduced by this patch, but does writing this control register unmask the timer interrupt?
The ctrl value is constructed without DWC_TIM_CTRL_INT_MASK set. If this hardware is on an OF platform where interrupts are commonly shared, could this unmasked timer flood the system with interrupts since there is no IRQ handler?
Quoted 5 lines
drivers/pwm/pwm-dwc-of.c new file +0
-0
[ ... ]
Quoted 17 lines
drivers/pwm/pwm-dwc-of.c new file +17
-0+static void dwc_pwm_plat_remove (struct platform_device *pdev) +{ +struct dwc_pwm_drvdata *data =platform_get_drvdata (pdev); +struct pwm_chip *chip = data->chips[0 ]; +struct dwc_pwm *dwc =to_dwc_pwm (chip); +unsigned int idx; +int ret; + +pwmchip_remove (chip); + + ret =pm_runtime_resume_and_get (&pdev->dev); +if (ret <0 ) { +dev_warn (&pdev->dev,"failed to resume device: %d\n" , ret); + }else { +for (idx =0 ; idx < chip->npwm; idx++) { +if (dwc_pwm_readl (dwc,DWC_TIM_CTRL (idx)) & +DWC_TIM_CTRL_EN )
drivers/pwm/pwm-dwc-of.c line 226
Quoted 10 lines
drivers/pwm/pwm-dwc-of.c new file +10
-0+pm_runtime_put_noidle (&pdev->dev); + } +pm_runtime_put_sync (&pdev->dev); + } + +if (!pm_runtime_status_suspended (&pdev->dev)) { +clk_disable_unprepare (dwc->clk); +clk_disable_unprepare (dwc->bus_clk); + } +pm_runtime_disable (&pdev->dev);
[Severity: High]
Could this ordering cause a permanent clock leak if a concurrent runtime PM resume occurs?
If userspace triggers a resume (e.g. via sysfs) between the pm_runtime_status_suspended() check and pm_runtime_disable(), the dwc_pwm_runtime_resume() callback will re-enable the clocks.
Once pm_runtime_disable() runs, the PM state is frozen, but the driver will have lost track of the re-enabled clocks. Should pm_runtime_disable() be called before manually rolling back the clock state?
drivers/pwm/pwm-dwc-of.c line 229
Quoted 3 lines
drivers/pwm/pwm-dwc-of.c new file +3
-0+ +if (dwc->rst) { + ret =reset_control_assert (dwc->rst);
[Severity: High]
Does asserting the reset while the IP block's clocks are disabled cause a bus lockup?
In dwc_pwm_plat_remove(), the clocks are guaranteed to be off here (either implicitly by allowing the device to suspend, or explicitly via the clk_disable_unprepare() calls just above).
On AMBA/APB buses, asserting a reset on an unclocked IP block often prevents the reset signal from propagating, leading to a system hang when the reset controller attempts to complete the transaction.
Quoted 5 lines
drivers/pwm/pwm-dwc-of.c new file +5
-0+if (ret) +dev_warn (&pdev->dev,"failed to assert reset: %d\n" , + ret); + } +}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
Re: Re: [PATCH v10 3/3] pwm: dwc: add of/platform support
Quoted 39 lines
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] Hardware reset is asserted after gating clocks, which can cause bus hangs.
- [High] Race condition between runtime PM check and disable causes a permanent clock leak.Pre-existing issues:
- [High] Unmasking the timer interrupt without an IRQ handler can cause a severe interrupt storm. --commit
a50609ba96938c02fbc94f54318c95561f661758
Author: Xuyang Dong <[email protected]>
Subject: pwm: dwc: add of/platform supportThe dwc pwm controller can be used in non-PCI systems, so allow either platform or OF based probing. Implements polarity control by exploiting the timer's dual load registers.
Quoted 4 lines
drivers/pwm/pwm-dwc-core.c +0
-0[ ... ]
drivers/pwm/pwm-dwc-core.c static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc, · line 144
Quoted 8 lines
drivers/pwm/pwm-dwc-core.c +3
-0@@ -85,6 +138,9 @@ static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc,* periods are set by Load Count registers. */ ctrl =DWC_TIM_CTRL_MODE_USER |DWC_TIM_CTRL_PWM ; +if (dwc->features &DWC_TIM_CTRL_0N100PWM_EN ) + ctrl |=DWC_TIM_CTRL_0N100PWM_EN ; +dwc_pwm_writel (dwc, ctrl,DWC_TIM_CTRL (pwm->hwpwm));[Severity: High]
This isn't a bug introduced by this patch, but does writing this control register unmask the timer interrupt?
Quoted 5 lines
The ctrl value is constructed without
DWC_TIM_CTRL_INT_MASKset. If this hardware is on an OF platform where interrupts are commonly shared, could this unmasked timer flood the system with interrupts since there is no IRQ handler?
Hi Sashiko,
Will add DWC_TIM_CTRL_INT_MASK in the next version.
Quoted 40 lines
Quoted 5 lines
drivers/pwm/pwm-dwc-of.c new file +0
-0[ ... ]
Quoted 17 lines
drivers/pwm/pwm-dwc-of.c new file +17
-0+static void dwc_pwm_plat_remove (struct platform_device *pdev) +{ +struct dwc_pwm_drvdata *data =platform_get_drvdata (pdev); +struct pwm_chip *chip = data->chips[0 ]; +struct dwc_pwm *dwc =to_dwc_pwm (chip); +unsigned int idx; +int ret; + +pwmchip_remove (chip); + + ret =pm_runtime_resume_and_get (&pdev->dev); +if (ret <0 ) { +dev_warn (&pdev->dev,"failed to resume device: %d\n" , ret); + }else { +for (idx =0 ; idx < chip->npwm; idx++) { +if (dwc_pwm_readl (dwc,DWC_TIM_CTRL (idx)) & +DWC_TIM_CTRL_EN )drivers/pwm/pwm-dwc-of.c line 226
Quoted 10 lines
drivers/pwm/pwm-dwc-of.c new file +10
-0+pm_runtime_put_noidle (&pdev->dev); + } +pm_runtime_put_sync (&pdev->dev); + } + +if (!pm_runtime_status_suspended (&pdev->dev)) { +clk_disable_unprepare (dwc->clk); +clk_disable_unprepare (dwc->bus_clk); + } +pm_runtime_disable (&pdev->dev);[Severity: High]
Could this ordering cause a permanent clock leak if a concurrent runtime PM resume occurs?
Quoted 8 lines
If userspace triggers a resume (e.g. via sysfs) between the
pm_runtime_status_suspended()check andpm_runtime_disable(), thedwc_pwm_runtime_resume()callback will re-enable the clocks.Once
pm_runtime_disable()runs, the PM state is frozen, but the driver will have lost track of the re-enabled clocks. Shouldpm_runtime_disable()be called before manually rolling back the clock state?
Call pm_runtime_disable() before manually rolling back the clock state.
Quoted 8 lines
drivers/pwm/pwm-dwc-of.c line 229
Quoted 3 lines
drivers/pwm/pwm-dwc-of.c new file +3
-0+ +if (dwc->rst) { + ret =reset_control_assert (dwc->rst);[Severity: High]
Does asserting the reset while the IP block's clocks are disabled cause a bus lockup?
Quoted 8 lines
In
dwc_pwm_plat_remove(), the clocks are guaranteed to be off here (either implicitly by allowing the device to suspend, or explicitly via theclk_disable_unprepare()calls just above).On AMBA/APB buses, asserting a reset on an unclocked IP block often prevents the reset signal from propagating, leading to a system hang when the reset controller attempts to complete the transaction.
Remove reset assert in remove function, as disabling PM and clocks prevents further operation
Best regards,
Xuyang Dong
Quoted 8 lines
Quoted 5 lines
drivers/pwm/pwm-dwc-of.c new file +5
-0+if (ret) +dev_warn (&pdev->dev,"failed to assert reset: %d\n" , + ret); + } +}Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3