* [PATCH v9 0/3] Update designware pwm driver
@ 2026-07-01 0:41 dongxuyang
2026-07-01 0:42 ` [PATCH v9 1/3] dt-bindings: pwm: dwc: Document optional resets property dongxuyang
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: dongxuyang @ 2026-07-01 0:41 UTC (permalink / raw)
To: ukleinek, robh, krzk+dt, conor+dt, ben-linux, ben.dooks, p.zabel,
linux-pwm, devicetree, linux-kernel
Cc: ningyu, linmin, xuxiang, wangguosheng, pinkesh.vaghela,
Xuyang Dong
From: Xuyang Dong <dongxuyang@eswincomputing.com>
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/20230907161242.67190-1-ben.dooks@codethink.co.uk/
Updates:
Changes in v9:
- YAML:
- Add 'Acked-by: Conor Dooley <conor.dooley@microchip.com>' 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.
Update this in the commit message (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/20260623071329.2034-1-dongxuyang@eswincomputing.com/
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/20260605082242.1541-1-dongxuyang@eswincomputing.com/
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/20260424094529.1691-1-dongxuyang@eswincomputing.com/
Changes in v6:
- YAML:
- Drop properties resets and its items description for eswin,eic7700-pwm.
- Link to v5: https://lore.kernel.org/all/20260423083644.1168-1-dongxuyang@eswincomputing.com/
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/20260415094908.1539-1-dongxuyang@eswincomputing.com/
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/20260402091718.1608-1-dongxuyang@eswincomputing.com/
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/20260306093000.2065-1-dongxuyang@eswincomputing.com/
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.
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202512061720.j31AsgM7-lkp@intel.com/
- Link to v1: https://lore.kernel.org/all/20251205090411.1388-1-dongxuyang@eswincomputing.com/
- Link to v9: https://lore.kernel.org/lkml/20230907161242.67190-1-ben.dooks@codethink.co.uk/
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 | 356 ++++++++++++++++++
drivers/pwm/pwm-dwc.h | 25 +-
6 files changed, 548 insertions(+), 42 deletions(-)
create mode 100644 drivers/pwm/pwm-dwc-of.c
--
2.34.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v9 1/3] dt-bindings: pwm: dwc: Document optional resets property
2026-07-01 0:41 [PATCH v9 0/3] Update designware pwm driver dongxuyang
@ 2026-07-01 0:42 ` dongxuyang
2026-07-01 0:43 ` [PATCH v9 2/3] dt-bindings: pwm: dwc: Add eswin compatible dongxuyang
2026-07-01 0:43 ` [PATCH v9 3/3] pwm: dwc: add of/platform support dongxuyang
2 siblings, 0 replies; 6+ messages in thread
From: dongxuyang @ 2026-07-01 0:42 UTC (permalink / raw)
To: ukleinek, robh, krzk+dt, conor+dt, ben-linux, ben.dooks, p.zabel,
linux-pwm, devicetree, linux-kernel
Cc: ningyu, linmin, xuxiang, wangguosheng, pinkesh.vaghela,
Xuyang Dong, Conor Dooley
From: Xuyang Dong <dongxuyang@eswincomputing.com>
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.
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Xuyang Dong <dongxuyang@eswincomputing.com>
---
.../devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml b/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml
index 7523a89a1773..213fdaef25d9 100644
--- a/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml
+++ b/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml
@@ -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
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v9 2/3] dt-bindings: pwm: dwc: Add eswin compatible
2026-07-01 0:41 [PATCH v9 0/3] Update designware pwm driver dongxuyang
2026-07-01 0:42 ` [PATCH v9 1/3] dt-bindings: pwm: dwc: Document optional resets property dongxuyang
@ 2026-07-01 0:43 ` dongxuyang
2026-07-01 0:43 ` [PATCH v9 3/3] pwm: dwc: add of/platform support dongxuyang
2 siblings, 0 replies; 6+ messages in thread
From: dongxuyang @ 2026-07-01 0:43 UTC (permalink / raw)
To: ukleinek, robh, krzk+dt, conor+dt, ben-linux, ben.dooks, p.zabel,
linux-pwm, devicetree, linux-kernel
Cc: ningyu, linmin, xuxiang, wangguosheng, pinkesh.vaghela,
Xuyang Dong, Conor Dooley
From: Xuyang Dong <dongxuyang@eswincomputing.com>
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.
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Xuyang Dong <dongxuyang@eswincomputing.com>
---
.../bindings/pwm/snps,dw-apb-timers-pwm2.yaml | 32 ++++++++++++++++---
1 file changed, 28 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml b/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml
index 213fdaef25d9..9a9a24ddc61b 100644
--- a/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml
+++ b/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml
@@ -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>;
+ };
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v9 3/3] pwm: dwc: add of/platform support
2026-07-01 0:41 [PATCH v9 0/3] Update designware pwm driver dongxuyang
2026-07-01 0:42 ` [PATCH v9 1/3] dt-bindings: pwm: dwc: Document optional resets property dongxuyang
2026-07-01 0:43 ` [PATCH v9 2/3] dt-bindings: pwm: dwc: Add eswin compatible dongxuyang
@ 2026-07-01 0:43 ` dongxuyang
2026-07-01 0:56 ` sashiko-bot
2 siblings, 1 reply; 6+ messages in thread
From: dongxuyang @ 2026-07-01 0:43 UTC (permalink / raw)
To: ukleinek, robh, krzk+dt, conor+dt, ben-linux, ben.dooks, p.zabel,
linux-pwm, devicetree, linux-kernel
Cc: ningyu, linmin, xuxiang, wangguosheng, pinkesh.vaghela,
Xuyang Dong
From: Xuyang Dong <dongxuyang@eswincomputing.com>
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.
Co-developed-by: Ben Dooks <ben.dooks@codethink.co.uk>
Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
Signed-off-by: Xiang Xu <xuxiang@eswincomputing.com>
Signed-off-by: Guosheng Wang <wangguosheng@eswincomputing.com>
Signed-off-by: Xuyang Dong <dongxuyang@eswincomputing.com>
---
drivers/pwm/Kconfig | 10 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-dwc-core.c | 161 +++++++++++++----
drivers/pwm/pwm-dwc-of.c | 356 +++++++++++++++++++++++++++++++++++++
drivers/pwm/pwm-dwc.h | 25 ++-
5 files changed, 515 insertions(+), 38 deletions(-)
create mode 100644 drivers/pwm/pwm-dwc-of.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index e8886a9b64d9..fd1d68beab67 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -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
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 5630a521a7cf..acd7dfe98dff 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -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
diff --git a/drivers/pwm/pwm-dwc-core.c b/drivers/pwm/pwm-dwc-core.c
index 6dabec93a3c6..1387d006946a 100644
--- a/drivers/pwm/pwm-dwc-core.c
+++ b/drivers/pwm/pwm-dwc-core.c
@@ -12,8 +12,10 @@
#define DEFAULT_SYMBOL_NAMESPACE "dwc_pwm"
#include <linux/bitops.h>
+#include <linux/clk.h>
#include <linux/export.h>
#include <linux/kernel.h>
+#include <linux/math64.h>
#include <linux/module.h>
#include <linux/pci.h>
#include <linux/pm_runtime.h>
@@ -44,21 +46,73 @@ static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc,
u32 high;
u32 low;
- /*
- * Calculate width of low and high period in terms of input clock
- * periods and check are the result within HW limits between 1 and
- * 2^32 periods.
- */
- tmp = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, dwc->clk_ns);
- if (tmp < 1 || tmp > (1ULL << 32))
- return -ERANGE;
- low = tmp - 1;
-
- tmp = DIV_ROUND_CLOSEST_ULL(state->period - state->duty_cycle,
- dwc->clk_ns);
- if (tmp < 1 || tmp > (1ULL << 32))
- return -ERANGE;
- high = tmp - 1;
+ if (dwc->clk)
+ dwc->clk_rate = clk_get_rate(dwc->clk);
+
+ if (dwc->features & DWC_TIM_CTRL_0N100PWM_EN) {
+ /*
+ * Calculate width of low and high period in terms of input
+ * clock periods and check are the result within HW limits
+ * between 0 and 2^32 periods.
+ * Use mul_u64_u64_div_u64() to avoid overflowing the 64-bit
+ * intermediate result and to round down to the nearest
+ * achievable hardware value, as required by the PWM core.
+ */
+ tmp = mul_u64_u64_div_u64(state->duty_cycle, dwc->clk_rate,
+ NSEC_PER_SEC);
+ if (tmp >= (1ULL << 32))
+ return -ERANGE;
+
+ /*
+ * The hardware has no polarity register. Polarity inversion is
+ * achieved by swapping the low and high load-count registers:
+ * NORMAL (active-high): duty_cycle ->
+ * HIGH period (DWC_TIM_LD_CNT2)
+ * INVERSED (active-low): duty_cycle ->
+ * LOW period (DWC_TIM_LD_CNT)
+ */
+ if (state->polarity == PWM_POLARITY_NORMAL)
+ high = tmp;
+ else
+ low = tmp;
+
+ tmp = mul_u64_u64_div_u64(state->period - state->duty_cycle,
+ dwc->clk_rate, NSEC_PER_SEC);
+ if (tmp >= (1ULL << 32))
+ return -ERANGE;
+
+ if (state->polarity == PWM_POLARITY_NORMAL)
+ low = tmp;
+ else
+ high = tmp;
+ } else {
+ /*
+ * Calculate width of low and high period in terms of input
+ * clock periods and check are the result within HW limits
+ * between 1 and 2^32 periods.
+ * Polarity inversion uses the same register-swap technique as
+ * the 0N100 path above.
+ */
+ tmp = mul_u64_u64_div_u64(state->duty_cycle, dwc->clk_rate,
+ NSEC_PER_SEC);
+ if (tmp < 1 || tmp > (1ULL << 32))
+ return -ERANGE;
+
+ if (state->polarity == PWM_POLARITY_NORMAL)
+ high = tmp - 1;
+ else
+ low = tmp - 1;
+
+ tmp = mul_u64_u64_div_u64(state->period - state->duty_cycle,
+ dwc->clk_rate, NSEC_PER_SEC);
+ if (tmp < 1 || tmp > (1ULL << 32))
+ return -ERANGE;
+
+ if (state->polarity == PWM_POLARITY_NORMAL)
+ low = tmp - 1;
+ else
+ high = tmp - 1;
+ }
/*
* Specification says timer usage flow is to disable timer, then
@@ -74,6 +128,7 @@ static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc,
* width of low period and latter the width of high period in terms
* multiple of input clock periods:
* Width = ((Count + 1) * input clock period).
+ * Width = (Count * input clock period) : supported 0% and 100%.
*/
dwc_pwm_writel(dwc, low, DWC_TIM_LD_CNT(pwm->hwpwm));
dwc_pwm_writel(dwc, high, DWC_TIM_LD_CNT2(pwm->hwpwm));
@@ -85,6 +140,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));
/*
@@ -99,14 +157,18 @@ static int dwc_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
const struct pwm_state *state)
{
struct dwc_pwm *dwc = to_dwc_pwm(chip);
-
- if (state->polarity != PWM_POLARITY_INVERSED)
- return -EINVAL;
+ int ret;
if (state->enabled) {
- if (!pwm->state.enabled)
- pm_runtime_get_sync(pwmchip_parent(chip));
- return __dwc_pwm_configure_timer(dwc, pwm, state);
+ if (!pwm->state.enabled) {
+ ret = pm_runtime_resume_and_get(pwmchip_parent(chip));
+ if (ret < 0)
+ return ret;
+ }
+ ret = __dwc_pwm_configure_timer(dwc, pwm, state);
+ if (ret && !pwm->state.enabled)
+ pm_runtime_put_sync(pwmchip_parent(chip));
+ return ret;
} else {
if (pwm->state.enabled) {
__dwc_pwm_set_enable(dwc, pwm->hwpwm, false);
@@ -121,10 +183,23 @@ static int dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
struct pwm_state *state)
{
struct dwc_pwm *dwc = to_dwc_pwm(chip);
- u64 duty, period;
+ unsigned long clk_rate;
u32 ctrl, ld, ld2;
+ u64 duty, period;
+ int ret;
+
+ ret = pm_runtime_resume_and_get(pwmchip_parent(chip));
+ if (ret)
+ return ret;
- pm_runtime_get_sync(pwmchip_parent(chip));
+ if (dwc->clk)
+ dwc->clk_rate = clk_get_rate(dwc->clk);
+
+ clk_rate = dwc->clk_rate;
+ if (!clk_rate) {
+ pm_runtime_put_sync(pwmchip_parent(chip));
+ return -EINVAL;
+ }
ctrl = dwc_pwm_readl(dwc, DWC_TIM_CTRL(pwm->hwpwm));
ld = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
@@ -132,22 +207,46 @@ static int dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
state->enabled = !!(ctrl & DWC_TIM_CTRL_EN);
+ /*
+ * The hardware has no polarity status register; polarity is encoded
+ * implicitly by which of DWC_TIM_LD_CNT / DWC_TIM_LD_CNT2 holds the
+ * duty-cycle period (see __dwc_pwm_configure_timer). Report the
+ * polarity that was last programmed by apply(). On the initial read
+ * (before any apply call), pwm->state.polarity defaults to
+ * PWM_POLARITY_NORMAL, which is the natural zero-initialised value.
+ */
+ state->polarity = pwm->state.polarity;
+
/*
* If we're not in PWM, technically the output is a 50-50
* based on the timer load-count only.
*/
if (ctrl & DWC_TIM_CTRL_PWM) {
- duty = (ld + 1) * dwc->clk_ns;
- period = (ld2 + 1) * dwc->clk_ns;
- period += duty;
+ if (dwc->features & DWC_TIM_CTRL_0N100PWM_EN) {
+ /*
+ * NORMAL: duty_cycle was written to DWC_TIM_LD_CNT2.
+ * INVERSED: duty_cycle was written to DWC_TIM_LD_CNT.
+ */
+ if (state->polarity == PWM_POLARITY_NORMAL)
+ duty = ld2;
+ else
+ duty = ld;
+ period = (u64)ld + ld2;
+ } else {
+ if (state->polarity == PWM_POLARITY_NORMAL)
+ duty = ld2 + 1;
+ else
+ duty = ld + 1;
+ period = (u64)ld + ld2 + 2;
+ }
} else {
- duty = (ld + 1) * dwc->clk_ns;
+ duty = ld + 1;
period = duty * 2;
+ state->polarity = PWM_POLARITY_INVERSED;
}
- state->polarity = PWM_POLARITY_INVERSED;
- state->period = period;
- state->duty_cycle = duty;
+ state->period = mul_u64_u64_div_u64(period, NSEC_PER_SEC, clk_rate);
+ state->duty_cycle = mul_u64_u64_div_u64(duty, NSEC_PER_SEC, clk_rate);
pm_runtime_put_sync(pwmchip_parent(chip));
@@ -169,7 +268,7 @@ struct pwm_chip *dwc_pwm_alloc(struct device *dev)
return chip;
dwc = to_dwc_pwm(chip);
- dwc->clk_ns = 10;
+ dwc->clk_rate = NSEC_PER_SEC / 10;
chip->ops = &dwc_pwm_ops;
return chip;
diff --git a/drivers/pwm/pwm-dwc-of.c b/drivers/pwm/pwm-dwc-of.c
new file mode 100644
index 000000000000..80700e9bd6e9
--- /dev/null
+++ b/drivers/pwm/pwm-dwc-of.c
@@ -0,0 +1,356 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * DesignWare PWM Controller driver OF
+ *
+ * Copyright (C) 2026 SiFive, Inc.
+ */
+
+#define DEFAULT_SYMBOL_NAMESPACE "dwc_pwm_of"
+
+#include <linux/clk.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/pwm.h>
+#include <linux/reset.h>
+
+#include "pwm-dwc.h"
+
+struct dwc_pwm_plat_data {
+ bool reset_required;
+};
+
+static int dwc_pwm_plat_probe(struct platform_device *pdev)
+{
+ const struct dwc_pwm_plat_data *pdata;
+ struct device *dev = &pdev->dev;
+ struct dwc_pwm_drvdata *data;
+ u32 ctrl[DWC_TIMERS_TOTAL];
+ struct pwm_chip *chip;
+ struct dwc_pwm *dwc;
+ bool pwm_en = false;
+ u32 nr_pwm, tim_id;
+ unsigned int i;
+ int ret;
+
+ data = devm_kzalloc(dev, struct_size(data, chips, 1), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ chip = dwc_pwm_alloc(dev);
+ if (IS_ERR(chip))
+ return dev_err_probe(dev, PTR_ERR(chip),
+ "failed to alloc pwm\n");
+
+ dwc = to_dwc_pwm(chip);
+
+ dwc->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(dwc->base))
+ return PTR_ERR(dwc->base);
+
+ if (!device_property_read_u32(dev, "snps,pwm-number", &nr_pwm)) {
+ if (nr_pwm > DWC_TIMERS_TOTAL)
+ dev_warn(dev, "too many PWMs (%d), capping at %d\n",
+ nr_pwm, chip->npwm);
+ else
+ chip->npwm = nr_pwm;
+ }
+
+ dwc->bus_clk = devm_clk_get(dev, "bus");
+ if (IS_ERR(dwc->bus_clk))
+ return dev_err_probe(dev, PTR_ERR(dwc->bus_clk),
+ "failed to get bus clock\n");
+
+ dwc->clk = devm_clk_get(dev, "timer");
+ if (IS_ERR(dwc->clk))
+ return dev_err_probe(dev, PTR_ERR(dwc->clk),
+ "failed to get timer clock\n");
+
+ ret = devm_clk_rate_exclusive_get(dev, dwc->clk);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "failed to get exclusive rate\n");
+
+ dwc->clk_rate = clk_get_rate(dwc->clk);
+ if (!dwc->clk_rate)
+ return dev_err_probe(dev, -EINVAL,
+ "failed to get a valid clock rate\n");
+
+ pdata = device_get_match_data(dev);
+ if (pdata && pdata->reset_required)
+ dwc->rst = devm_reset_control_get_exclusive(dev, NULL);
+ else
+ dwc->rst = devm_reset_control_array_get_optional_exclusive(dev);
+
+ if (IS_ERR(dwc->rst))
+ return dev_err_probe(dev, PTR_ERR(dwc->rst),
+ "failed to get reset control\n");
+
+ ret = clk_prepare_enable(dwc->bus_clk);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "failed to enable bus clock\n");
+
+ ret = clk_prepare_enable(dwc->clk);
+ if (ret) {
+ dev_err(dev, "failed to enable timer clock\n");
+ goto disable_busclk;
+ }
+
+ /*
+ * Check all channels to see if any channel is enabled.
+ * Read the control register of each channel and extract the enable bit
+ */
+ for (i = 0; i < chip->npwm; i++) {
+ ctrl[i] = dwc_pwm_readl(dwc, DWC_TIM_CTRL(i)) & DWC_TIM_CTRL_EN;
+ if (ctrl[i])
+ pwm_en = true;
+ }
+
+ /*
+ * Only issue a reset pulse when all channels are disabled, so a PWM
+ * channel already running (e.g. configured by firmware before Linux
+ * took over) is left undisturbed.
+ */
+ if (!pwm_en) {
+ ret = reset_control_reset(dwc->rst);
+ if (ret) {
+ dev_err(dev, "failed to reset\n");
+ goto disable_clk;
+ }
+ }
+
+ /* init PWM feature */
+ dwc->features = 0;
+ /*
+ * Support for 0% and 100% duty cycle mode was added in version 2.11a
+ * and later.
+ */
+ tim_id = dwc_pwm_readl(dwc, DWC_TIMERS_COMP_VERSION);
+ if (tim_id >= DWC_TIM_VERSION_ID_2_11A)
+ dwc->features |= DWC_TIM_CTRL_0N100PWM_EN;
+
+ data->chips[0] = chip;
+ dev_set_drvdata(dev, data);
+
+ /*
+ * If any PWM channel is enabled, mark device active and hold runtime PM
+ * references for each enabled channel. Otherwise, gate the clocks.
+ */
+ if (pwm_en) {
+ pm_runtime_set_active(dev);
+ for (i = 0; i < chip->npwm; i++) {
+ if (ctrl[i])
+ pm_runtime_get_noresume(dev);
+ }
+ } else {
+ clk_disable_unprepare(dwc->clk);
+ clk_disable_unprepare(dwc->bus_clk);
+ }
+
+ pm_runtime_enable(dev);
+
+ ret = pwmchip_add(chip);
+ if (ret) {
+ dev_err(dev, "failed to add pwm chip\n");
+ goto pm_disable;
+ }
+
+ return 0;
+
+pm_disable:
+ pm_runtime_disable(dev);
+ if (pwm_en) {
+ for (i = 0; i < chip->npwm; i++) {
+ if (ctrl[i])
+ pm_runtime_put_noidle(dev);
+ }
+ goto disable_clk;
+ }
+
+ return ret;
+
+disable_clk:
+ clk_disable_unprepare(dwc->clk);
+disable_busclk:
+ clk_disable_unprepare(dwc->bus_clk);
+
+ return ret;
+}
+
+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)
+ 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);
+
+ if (dwc->rst) {
+ ret = reset_control_assert(dwc->rst);
+ if (ret)
+ dev_warn(&pdev->dev, "failed to assert reset: %d\n",
+ ret);
+ }
+}
+
+static int dwc_pwm_runtime_suspend(struct device *dev)
+{
+ struct dwc_pwm_drvdata *data = dev_get_drvdata(dev);
+ struct pwm_chip *chip = data->chips[0];
+ struct dwc_pwm *dwc = to_dwc_pwm(chip);
+
+ clk_disable_unprepare(dwc->clk);
+ clk_disable_unprepare(dwc->bus_clk);
+
+ return 0;
+}
+
+static int dwc_pwm_runtime_resume(struct device *dev)
+{
+ struct dwc_pwm_drvdata *data = dev_get_drvdata(dev);
+ struct pwm_chip *chip = data->chips[0];
+ struct dwc_pwm *dwc = to_dwc_pwm(chip);
+ int ret;
+
+ ret = clk_prepare_enable(dwc->bus_clk);
+ if (ret) {
+ dev_err(dev, "failed to enable bus clock: %d\n", ret);
+ return ret;
+ }
+
+ ret = clk_prepare_enable(dwc->clk);
+ if (ret) {
+ dev_err(dev, "failed to enable timer clock: %d\n", ret);
+ clk_disable_unprepare(dwc->bus_clk);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int dwc_pwm_suspend(struct device *dev)
+{
+ struct dwc_pwm_drvdata *data = dev_get_drvdata(dev);
+ struct pwm_chip *chip = data->chips[0];
+ struct dwc_pwm *dwc = to_dwc_pwm(chip);
+ unsigned int idx;
+ int ret;
+
+ if (pm_runtime_status_suspended(dev)) {
+ ret = dwc_pwm_runtime_resume(dev);
+ if (ret)
+ return ret;
+ }
+
+ for (idx = 0; idx < chip->npwm; idx++) {
+ dwc->ctx[idx].cnt = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(idx));
+ dwc->ctx[idx].cnt2 = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(idx));
+ dwc->ctx[idx].ctrl = dwc_pwm_readl(dwc, DWC_TIM_CTRL(idx));
+ }
+
+ ret = dwc_pwm_runtime_suspend(dev);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static int dwc_pwm_resume(struct device *dev)
+{
+ struct dwc_pwm_drvdata *data = dev_get_drvdata(dev);
+ struct pwm_chip *chip = data->chips[0];
+ struct dwc_pwm *dwc = to_dwc_pwm(chip);
+ unsigned int idx;
+ bool pm_flags;
+ int ret;
+
+ /* Check if device was runtime suspended before system resume */
+ pm_flags = pm_runtime_status_suspended(dev);
+ if (pm_flags) {
+ /*
+ * Use PM framework to resume device
+ * (calls dwc_pwm_runtime_resume)
+ */
+ ret = pm_runtime_resume_and_get(dev);
+ if (ret < 0)
+ return ret;
+ } else {
+ /*
+ * Device was active, but clocks might be off after system
+ * sleep.
+ * Call runtime_resume directly to restore hardware state.
+ */
+ ret = dwc_pwm_runtime_resume(dev);
+ if (ret)
+ return ret;
+ }
+
+ for (idx = 0; idx < chip->npwm; idx++) {
+ dwc_pwm_writel(dwc, dwc->ctx[idx].cnt, DWC_TIM_LD_CNT(idx));
+ dwc_pwm_writel(dwc, dwc->ctx[idx].cnt2, DWC_TIM_LD_CNT2(idx));
+ dwc_pwm_writel(dwc, dwc->ctx[idx].ctrl, DWC_TIM_CTRL(idx));
+ }
+
+ if (pm_flags) {
+ /*
+ * Balance the refcount taken by pm_runtime_resume_and_get
+ * if it was used.
+ */
+ pm_runtime_put_sync(dev);
+ }
+
+ return 0;
+}
+
+static const struct dev_pm_ops dwc_pwm_pm_ops = {
+ RUNTIME_PM_OPS(dwc_pwm_runtime_suspend, dwc_pwm_runtime_resume, NULL)
+ SYSTEM_SLEEP_PM_OPS(dwc_pwm_suspend, dwc_pwm_resume)
+};
+
+static const struct dwc_pwm_plat_data pwm_eic7700_pdata = {
+ .reset_required = true,
+};
+
+static const struct of_device_id dwc_pwm_dt_ids[] = {
+ { .compatible = "snps,dw-apb-timers-pwm2" },
+ { .compatible = "eswin,eic7700-pwm", .data = &pwm_eic7700_pdata },
+ { }
+};
+MODULE_DEVICE_TABLE(of, dwc_pwm_dt_ids);
+
+static struct platform_driver dwc_pwm_plat_driver = {
+ .driver = {
+ .name = "dwc-pwm",
+ .pm = pm_ptr(&dwc_pwm_pm_ops),
+ .of_match_table = dwc_pwm_dt_ids,
+ },
+ .probe = dwc_pwm_plat_probe,
+ .remove = dwc_pwm_plat_remove,
+};
+
+module_platform_driver(dwc_pwm_plat_driver);
+
+MODULE_ALIAS("platform:dwc-pwm-of");
+MODULE_AUTHOR("Ben Dooks <ben.dooks@codethink.co.uk>");
+MODULE_DESCRIPTION("DesignWare PWM Controller");
+MODULE_LICENSE("GPL");
diff --git a/drivers/pwm/pwm-dwc.h b/drivers/pwm/pwm-dwc.h
index 1562594e7f85..75f7c2d031c4 100644
--- a/drivers/pwm/pwm-dwc.h
+++ b/drivers/pwm/pwm-dwc.h
@@ -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 0x3231312a
struct 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)
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v9 3/3] pwm: dwc: add of/platform support
2026-07-01 0:43 ` [PATCH v9 3/3] pwm: dwc: add of/platform support dongxuyang
@ 2026-07-01 0:56 ` sashiko-bot
2026-07-02 9:25 ` Xuyang Dong
0 siblings, 1 reply; 6+ messages in thread
From: sashiko-bot @ 2026-07-01 0:56 UTC (permalink / raw)
To: dongxuyang; +Cc: devicetree, conor+dt, robh
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Missing CONFIG_PM dependency leads to unclocked MMIO accesses and kernel panics.
- [High] Bypassing the PM framework during system suspend causes bus faults on devices attached to a PM domain.
- [High] Reading MMIO registers while the device is in reset causes a bus stall and kernel panic.
- [Medium] Data race on the shared `dwc->clk_rate` variable.
--
commit f417f1c0148d8abb6dc7081bbc13d8a85dcc173e
Author: Xuyang Dong <dongxuyang@eswincomputing.com>
pwm: dwc: add of/platform support
This patch adds platform and OF device tree support for the DesignWare PWM
controller. It introduces runtime PM support, manages clock enabling and
disabling based on PWM channel usage, and implements polarity control by
swapping load count registers.
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -249,6 +249,16 @@ config PWM_DWC
[ ... ]
> +config PWM_DWC_OF
> + tristate "DesignWare PWM Controller (OF bus)"
> + depends on HAS_IOMEM && (OF || COMPILE_TEST)
[Severity: High]
Should this include a dependency on PM?
If the kernel is compiled without CONFIG_PM, Kconfig allows building
PWM_DWC_OF. During probe(), if no PWMs are enabled, the driver explicitly
disables dwc->clk and dwc->bus_clk and relies on PM runtime to re-enable
them. Because CONFIG_PM is disabled, pm_runtime_resume_and_get() evaluates to
a stub that returns 1 (success) without calling the runtime resume callback.
This leads to unclocked register accesses and kernel panics.
> diff --git a/drivers/pwm/pwm-dwc-core.c b/drivers/pwm/pwm-dwc-core.c
> --- a/drivers/pwm/pwm-dwc-core.c
> +++ b/drivers/pwm/pwm-dwc-core.c
> @@ -44,21 +46,73 @@ static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc,
[ ... ]
> - high = tmp - 1;
> + if (dwc->clk)
> + dwc->clk_rate = clk_get_rate(dwc->clk);
[Severity: Medium]
Can this write to dwc->clk_rate cause a data race?
The PWM framework serializes operations on a per-channel basis, but not
across the entire controller. If multiple threads concurrently request state
changes (apply) or state reads (get_state) on different PWM channels of the
same controller, they might simultaneously write to this shared variable.
> @@ -121,10 +183,23 @@ static int dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
[ ... ]
> + if (dwc->clk)
> + dwc->clk_rate = clk_get_rate(dwc->clk);
[Severity: Medium]
Does this introduce a similar data race on dwc->clk_rate as the one mentioned
above in __dwc_pwm_configure_timer()?
> diff --git a/drivers/pwm/pwm-dwc-of.c b/drivers/pwm/pwm-dwc-of.c
> --- /dev/null
> +++ b/drivers/pwm/pwm-dwc-of.c
> @@ -0,0 +1,356 @@
[ ... ]
> +static int dwc_pwm_plat_probe(struct platform_device *pdev)
> +{
[ ... ]
> + ret = clk_prepare_enable(dwc->clk);
> + if (ret) {
> + dev_err(dev, "failed to enable timer clock\n");
> + goto disable_busclk;
> + }
> +
> + /*
> + * Check all channels to see if any channel is enabled.
> + * Read the control register of each channel and extract the enable bit
> + */
> + for (i = 0; i < chip->npwm; i++) {
> + ctrl[i] = dwc_pwm_readl(dwc, DWC_TIM_CTRL(i)) & DWC_TIM_CTRL_EN;
> + if (ctrl[i])
> + pwm_en = true;
> + }
> +
> + /*
> + * Only issue a reset pulse when all channels are disabled, so a PWM
> + * channel already running (e.g. configured by firmware before Linux
> + * took over) is left undisturbed.
> + */
> + if (!pwm_en) {
> + ret = reset_control_reset(dwc->rst);
[Severity: High]
Could this read from DWC_TIM_CTRL(i) cause a bus stall and kernel panic if the
device is currently in reset?
Module removal explicitly asserts the reset line in dwc_pwm_plat_remove(),
leaving the device in reset. On a subsequent probe (e.g., reloading the
module), this loop attempts to read from the MMIO space before the reset is
deasserted.
[ ... ]
> + if (pwm_en) {
> + pm_runtime_set_active(dev);
> + for (i = 0; i < chip->npwm; i++) {
> + if (ctrl[i])
> + pm_runtime_get_noresume(dev);
> + }
> + } else {
> + clk_disable_unprepare(dwc->clk);
> + clk_disable_unprepare(dwc->bus_clk);
> + }
[Severity: High]
If CONFIG_PM is disabled in the kernel, does gating these clocks here cause
subsequent unclocked register accesses?
Since pm_runtime_resume_and_get() becomes a stub that returns 1 without
re-enabling clocks, subsequent operations like apply() or get_state() would
perform unclocked MMIO accesses leading to panics.
[ ... ]
> +static void dwc_pwm_plat_remove(struct platform_device *pdev)
> +{
[ ... ]
> + if (!pm_runtime_status_suspended(&pdev->dev)) {
> + clk_disable_unprepare(dwc->clk);
> + clk_disable_unprepare(dwc->bus_clk);
> + }
[Severity: High]
If CONFIG_PM is disabled, pm_runtime_status_suspended() will always return
false. Does this cause a double-disable of the clocks if they were already
gated in probe?
[ ... ]
> +static int dwc_pwm_suspend(struct device *dev)
> +{
> + struct dwc_pwm_drvdata *data = dev_get_drvdata(dev);
> + struct pwm_chip *chip = data->chips[0];
> + struct dwc_pwm *dwc = to_dwc_pwm(chip);
> + unsigned int idx;
> + int ret;
> +
> + if (pm_runtime_status_suspended(dev)) {
> + ret = dwc_pwm_runtime_resume(dev);
> + if (ret)
> + return ret;
> + }
> +
> + for (idx = 0; idx < chip->npwm; idx++) {
> + dwc->ctx[idx].cnt = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(idx));
[Severity: High]
Does calling the local dwc_pwm_runtime_resume() directly bypass the PM
framework?
If the device is attached to a generic power domain (genpd) and is
runtime-suspended, the PM domain itself might be powered off. Bypassing
pm_runtime_resume_and_get() means the PM domain is never powered back on,
and the subsequent dwc_pwm_readl() calls will access unpowered hardware
causing a bus fault.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260701004139.347-1-dongxuyang@eswincomputing.com?part=3
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Re: [PATCH v9 3/3] pwm: dwc: add of/platform support
2026-07-01 0:56 ` sashiko-bot
@ 2026-07-02 9:25 ` Xuyang Dong
0 siblings, 0 replies; 6+ messages in thread
From: Xuyang Dong @ 2026-07-02 9:25 UTC (permalink / raw)
To: sashiko-reviews
Cc: ukleinek, robh, krzk+dt, conor+dt, ben-linux, ben.dooks, p.zabel,
linux-pwm, devicetree, linux-kernel, linmin, pinkesh.vaghela
>
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] Missing CONFIG_PM dependency leads to unclocked MMIO accesses and kernel panics.
> - [High] Bypassing the PM framework during system suspend causes bus faults on devices attached to a PM domain.
> - [High] Reading MMIO registers while the device is in reset causes a bus stall and kernel panic.
> - [Medium] Data race on the shared `dwc->clk_rate` variable.
> --
>
> commit f417f1c0148d8abb6dc7081bbc13d8a85dcc173e
> Author: Xuyang Dong <dongxuyang@eswincomputing.com>
>
> pwm: dwc: add of/platform support
>
> This patch adds platform and OF device tree support for the DesignWare PWM
> controller. It introduces runtime PM support, manages clock enabling and
> disabling based on PWM channel usage, and implements polarity control by
> swapping load count registers.
>
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -249,6 +249,16 @@ config PWM_DWC
> [ ... ]
> > +config PWM_DWC_OF
> > + tristate "DesignWare PWM Controller (OF bus)"
> > + depends on HAS_IOMEM && (OF || COMPILE_TEST)
>
> [Severity: High]
> Should this include a dependency on PM?
>
> If the kernel is compiled without CONFIG_PM, Kconfig allows building
> PWM_DWC_OF. During probe(), if no PWMs are enabled, the driver explicitly
> disables dwc->clk and dwc->bus_clk and relies on PM runtime to re-enable
> them. Because CONFIG_PM is disabled, pm_runtime_resume_and_get() evaluates to
> a stub that returns 1 (success) without calling the runtime resume callback.
> This leads to unclocked register accesses and kernel panics.
>
Hi Sashiko,
Do not add a dependency on PM. Additionally, the issues below will be
fixed to ensure compatibility with and without PM support.
> > diff --git a/drivers/pwm/pwm-dwc-core.c b/drivers/pwm/pwm-dwc-core.c
> > --- a/drivers/pwm/pwm-dwc-core.c
> > +++ b/drivers/pwm/pwm-dwc-core.c
> > @@ -44,21 +46,73 @@ static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc,
> [ ... ]
> > - high = tmp - 1;
> > + if (dwc->clk)
> > + dwc->clk_rate = clk_get_rate(dwc->clk);
>
> [Severity: Medium]
> Can this write to dwc->clk_rate cause a data race?
>
> The PWM framework serializes operations on a per-channel basis, but not
> across the entire controller. If multiple threads concurrently request state
> changes (apply) or state reads (get_state) on different PWM channels of the
> same controller, they might simultaneously write to this shared variable.
>
The PWM core's chip->nonatomic_lock (per-chip mutex) serializes
all apply() and get_state() calls across all channels on the same
chip. The apparent write to dwc->clk_rate in both
__dwc_pwm_configure_timer() and dwc_pwm_get_state() cannot race
because both code paths are reached only under this lock.
No locking concerns.
> > @@ -121,10 +183,23 @@ static int dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> [ ... ]
> > + if (dwc->clk)
> > + dwc->clk_rate = clk_get_rate(dwc->clk);
>
> [Severity: Medium]
> Does this introduce a similar data race on dwc->clk_rate as the one mentioned
> above in __dwc_pwm_configure_timer()?
>
It is the same as above.
> > diff --git a/drivers/pwm/pwm-dwc-of.c b/drivers/pwm/pwm-dwc-of.c
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-dwc-of.c
> > @@ -0,0 +1,356 @@
> [ ... ]
> > +static int dwc_pwm_plat_probe(struct platform_device *pdev)
> > +{
> [ ... ]
> > + ret = clk_prepare_enable(dwc->clk);
> > + if (ret) {
> > + dev_err(dev, "failed to enable timer clock\n");
> > + goto disable_busclk;
> > + }
> > +
> > + /*
> > + * Check all channels to see if any channel is enabled.
> > + * Read the control register of each channel and extract the enable bit
> > + */
> > + for (i = 0; i < chip->npwm; i++) {
> > + ctrl[i] = dwc_pwm_readl(dwc, DWC_TIM_CTRL(i)) & DWC_TIM_CTRL_EN;
> > + if (ctrl[i])
> > + pwm_en = true;
> > + }
> > +
> > + /*
> > + * Only issue a reset pulse when all channels are disabled, so a PWM
> > + * channel already running (e.g. configured by firmware before Linux
> > + * took over) is left undisturbed.
> > + */
> > + if (!pwm_en) {
> > + ret = reset_control_reset(dwc->rst);
>
> [Severity: High]
> Could this read from DWC_TIM_CTRL(i) cause a bus stall and kernel panic if the
> device is currently in reset?
>
> Module removal explicitly asserts the reset line in dwc_pwm_plat_remove(),
> leaving the device in reset. On a subsequent probe (e.g., reloading the
> module), this loop attempts to read from the MMIO space before the reset is
> deasserted.
>
Deassert reset before reading MMIO registers. The channel-enable
scan reads DWC_TIM_CTRL while the device may still be held in reset
after a previous remove(). Reading MMIO while in reset causes a bus
stall on many SoCs; call reset_control_deassert() before the scan.
> [ ... ]
> > + if (pwm_en) {
> > + pm_runtime_set_active(dev);
> > + for (i = 0; i < chip->npwm; i++) {
> > + if (ctrl[i])
> > + pm_runtime_get_noresume(dev);
> > + }
> > + } else {
> > + clk_disable_unprepare(dwc->clk);
> > + clk_disable_unprepare(dwc->bus_clk);
> > + }
>
> [Severity: High]
> If CONFIG_PM is disabled in the kernel, does gating these clocks here cause
> subsequent unclocked register accesses?
>
> Since pm_runtime_resume_and_get() becomes a stub that returns 1 without
> re-enabling clocks, subsequent operations like apply() or get_state() would
> perform unclocked MMIO accesses leading to panics.
>
Keep clocks enabled when CONFIG_PM is disabled. When CONFIG_PM=n,
pm_runtime callbacks are stubs that never invoke the runtime_resume
hook, so clocks gated in probe are never re-enabled before MMIO
access in apply() and get_state(). Condition the clock gating on
IS_ENABLED(CONFIG_PM), and update the pm_disable error path to
disable clocks when CONFIG_PM=n (they were never gated in that case).
> [ ... ]
> > +static void dwc_pwm_plat_remove(struct platform_device *pdev)
> > +{
> [ ... ]
> > + if (!pm_runtime_status_suspended(&pdev->dev)) {
> > + clk_disable_unprepare(dwc->clk);
> > + clk_disable_unprepare(dwc->bus_clk);
> > + }
>
> [Severity: High]
> If CONFIG_PM is disabled, pm_runtime_status_suspended() will always return
> false. Does this cause a double-disable of the clocks if they were already
> gated in probe?
>
With the above fix, the clocks in probe will not be disabled when
CONFIG_PM=n. This prevents the double-disable issue.
> [ ... ]
> > +static int dwc_pwm_suspend(struct device *dev)
> > +{
> > + struct dwc_pwm_drvdata *data = dev_get_drvdata(dev);
> > + struct pwm_chip *chip = data->chips[0];
> > + struct dwc_pwm *dwc = to_dwc_pwm(chip);
> > + unsigned int idx;
> > + int ret;
> > +
> > + if (pm_runtime_status_suspended(dev)) {
> > + ret = dwc_pwm_runtime_resume(dev);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + for (idx = 0; idx < chip->npwm; idx++) {
> > + dwc->ctx[idx].cnt = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(idx));
>
> [Severity: High]
> Does calling the local dwc_pwm_runtime_resume() directly bypass the PM
> framework?
>
> If the device is attached to a generic power domain (genpd) and is
> runtime-suspended, the PM domain itself might be powered off. Bypassing
> pm_runtime_resume_and_get() means the PM domain is never powered back on,
> and the subsequent dwc_pwm_readl() calls will access unpowered hardware
> causing a bus fault.
>
Use pm_runtime_resume_and_get() instead of calling
dwc_pwm_runtime_resume() directly.
Move the register read/write operations from dwc_pwm_suspend/resume() into
dwc_pwm_runtime_suspend/resume().
Switch system sleep callbacks to pm_runtime_force_suspend() and
pm_runtime_force_resume(), instead of dwc_pwm_suspend() and
dwc_pwm_resume().
Would this approach be more suitable?
Best regards,
Xuyang Dong
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260701004139.347-1-dongxuyang@eswincomputing.com?part=3
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-07-02 9:25 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-01 0:41 [PATCH v9 0/3] Update designware pwm driver dongxuyang
2026-07-01 0:42 ` [PATCH v9 1/3] dt-bindings: pwm: dwc: Document optional resets property dongxuyang
2026-07-01 0:43 ` [PATCH v9 2/3] dt-bindings: pwm: dwc: Add eswin compatible dongxuyang
2026-07-01 0:43 ` [PATCH v9 3/3] pwm: dwc: add of/platform support dongxuyang
2026-07-01 0:56 ` sashiko-bot
2026-07-02 9:25 ` Xuyang Dong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox