Update designware pwm driver
Subsystems (2)
v6 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 v6:
- YAML:
- Drop the resets property and its items description for eswin,eic7700-pwm. Keep the required.
- 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 (2):
dt-bindings: pwm: dwc: add optional reset
pwm: dwc: add of/platform support
.../bindings/pwm/snps,dw-apb-timers-pwm2.yaml | 25 +- drivers/pwm/Kconfig | 10 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-dwc-core.c | 101 +++-- drivers/pwm/pwm-dwc-of.c | 346 ++++++++++++++++++ drivers/pwm/pwm-dwc.h | 25 +- 6 files changed, 475 insertions(+), 33 deletions(-) create mode 100644 drivers/pwm/pwm-dwc-of.c
--
2.34.1
PATCH v6 1/2 dt-bindings: pwm: dwc: add optional reset
From: Xuyang Dong <[email protected]>
The DesignWare PWM includes separate reset signals dedicated to each clock domain:
The presetn signal resets logic in pclk domain.
The timer_N_resetn signal resets logic in the timer_N_clk domain. The resets are active-low.
EIC7700 use DesignWare IP for PWM controllers. Add ESWIN EIC7700 support
in snps,dw-apb-timers-pwm2.yaml
--- .../bindings/pwm/snps,dw-apb-timers-pwm2.yaml | 25 ++++++++++++++++--- 1 file changed, 21 insertions(+), 4 deletions(-)
Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml +21 -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
@@ -43,6 +42,12 @@ properties: - const: bus - const: timer + resets: + minItems: 1 + items: + - description: Interface bus reset + - description: PWM timer logic reset + snps,pwm-number: $ref: /schemas/types.yaml#/definitions/uint32 description: The number of PWM channels configured for this instance
@@ -54,6 +59,18 @@ required: - clocks - clock-names +allOf: + - $ref: pwm.yaml# + + - if: + properties: + compatible: + contains: + const: eswin,eic7700-pwm + then: + required: + - resets + additionalProperties: false examples:
Signature · 1 line
2.34.1
Re: [PATCH v6 1/2] dt-bindings: pwm: dwc: add optional reset
Quoted 8 lines · On Fri, Apr 24, 2026 at 05:54:35PM +0800, [email protected] wrote:
From: Xuyang Dong <[email protected]>
The DesignWare PWM includes separate reset signals dedicated to each clock domain:
The presetn signal resets logic in pclk domain.
Thetimer_N_resetnsignal resets logic in thetimer_N_clkdomain. The resets are active-low.
Quoted 4 lines
EIC7700 use DesignWare IP for PWM controllers. Add ESWIN EIC7700 support
in snps,dw-apb-timers-pwm2.yamlSigned-off-by Xuyang Dong <[email protected]>
pw-bot: not-applicable
Re: [PATCH v6 1/2] dt-bindings: pwm: dwc: add optional reset
In reply to [email protected]Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml required: · line 69
Quoted 9 lines · On 24/04/2026 11:54, [email protected] wrote:
Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml +8
-0+allOf: + - $ref: pwm.yaml# + + - if: + properties: + compatible: + contains: + const: eswin,eic7700-pwm
Same problem as v3 which I commented. I do not understand why your new device has also 1 reset.
Your commit msg MUST explain why 1 reset is valid.
Best regards,
Krzysztof
Re: Re: [PATCH v6 1/2] dt-bindings: pwm: dwc: add optional reset
Quoted 13 lines
Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml required: · line 69
Quoted 9 lines
Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml +8
-0+allOf: + - $ref: pwm.yaml# + + - if: + properties: + compatible: + contains: + const: eswin,eic7700-pwmSame problem as v3 which I commented. I do not understand why your new device has also 1 reset.
Quoted 2 lines
Your commit msg MUST explain why 1 reset is valid.
Hi Krzysztof,
Although the PWM IP supports two clock domains, each requiring a reset, the EIC7700 implementation uses the same clock domain for both clock signals. Therefore, the eic7700-pwm only supports one reset. This description will be updated in the binding commit in the next version.
Best regards,
Xuyang Dong
Re: [PATCH v6 1/2] dt-bindings: pwm: dwc: add optional reset
Quoted 13 lines · Xuyang Dong
Quoted 13 lines
Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml required: · line 69
Quoted 9 lines
Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml +8
-0+allOf: + - $ref: pwm.yaml# + + - if: + properties: + compatible: + contains: + const: eswin,eic7700-pwmSame problem as v3 which I commented. I do not understand why your new device has also 1 reset.
Quoted 9 lines
Quoted 2 lines
Your commit msg MUST explain why 1 reset is valid.
Hi Krzysztof,
Although the PWM IP supports two clock domains, each requiring a reset, the EIC7700 implementation uses the same clock domain for both clock signals. Therefore, the eic7700-pwm only supports one reset.
If we speak about eic7700, explain why it has two resets now, according to schema, even though you say it has not.
But I was speaking about dw-apb-timers-pwm, which has one reset as well! Why you are not having proper constraints? Please read writing bindings document.
Best regards,
Krzysztof
Re: Re: [PATCH v6 1/2] dt-bindings: pwm: dwc: add optional reset
Quoted 23 lines
Quoted 13 lines
Quoted 13 lines
Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml required: · line 69
Quoted 9 lines
Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml +8
-0+allOf: + - $ref: pwm.yaml# + + - if: + properties: + compatible: + contains: + const: eswin,eic7700-pwmSame problem as v3 which I commented. I do not understand why your new device has also 1 reset.
Quoted 9 lines
Quoted 2 lines
Your commit msg MUST explain why 1 reset is valid.
Hi Krzysztof,
Although the PWM IP supports two clock domains, each requiring a reset, the EIC7700 implementation uses the same clock domain for both clock signals. Therefore, the eic7700-pwm only supports one reset.
Quoted 7 lines
If we speak about eic7700, explain why it has two resets now, according to schema, even though you say it has not.
But I was speaking about dw-apb-timers-pwm, which has one reset as well! Why you are not having proper constraints? Please read writing bindings document.
Hi Krzysztof,
Let me clarify the reset signals.
- snps,dw-apb-timers-pwm2: IP spec has 2 optional reset signals (one per clock domain), SoC vendor decides whether to wire them — so maxItems: 2, optional in required.
- eswin,eic7700-pwm: SoC physically ties both signals to one reset — so exactly 1, required.
So I think the correct patch should be like this:
Modify the commit message as follows:
The DesignWare PWM IP has up to two optional reset signals, one per clock domain (presetn for pclk, timer_N_resetn for timer_N_clk). SoC vendors decides to wire them, so maxItems: 2 is the upper bound for the generic snps,dw-apb-timers-pwm2 compatible.
The ESWIN EIC7700 ties both reset domains to a single physical reset signal, so exactly one reset is required. Add maxItems: 1 to the if-then block for eswin,eic7700-pwm to enforce this.
Modify the YAML as follows:
-allOf:
- - $ref: pwm.yaml#
-
properties:
compatible:
- const: snps,dw-apb-timers-pwm2
+ enum:
+ - snps,dw-apb-timers-pwm2
+ - eswin,eic7700-pwm
reg:
maxItems: 1
@@ -43,6 +42,13 @@ properties:
- const: bus
- const: timer
+ resets:
+ minItems: 1
+ maxItems: 2
+
snps,pwm-number:
$ref: /schemas/types.yaml#/definitions/uint32
description: The number of PWM channels configured for this instance @@ -54,6 +60,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
Do you think this modification is more appropriate?
Best regards,
Xuyang Dong
Re: [PATCH v6 1/2] dt-bindings: pwm: dwc: add optional reset
Quoted 31 lines · Xuyang Dong
Quoted 23 lines
Quoted 13 lines
Quoted 13 lines
Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml required: · line 69
Quoted 9 lines
Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml +8
-0+allOf: + - $ref: pwm.yaml# + + - if: + properties: + compatible: + contains: + const: eswin,eic7700-pwmSame problem as v3 which I commented. I do not understand why your new device has also 1 reset.
Quoted 9 lines
Quoted 2 lines
Your commit msg MUST explain why 1 reset is valid.
Hi Krzysztof,
Although the PWM IP supports two clock domains, each requiring a reset, the EIC7700 implementation uses the same clock domain for both clock signals. Therefore, the eic7700-pwm only supports one reset.
Quoted 7 lines
If we speak about eic7700, explain why it has two resets now, according to schema, even though you say it has not.
But I was speaking about dw-apb-timers-pwm, which has one reset as well! Why you are not having proper constraints? Please read writing bindings document.
Quoted 6 lines
Hi Krzysztof,
Let me clarify the reset signals.
- snps,dw-apb-timers-pwm2: IP spec has 2 optional reset signals (one per clock domain), SoC vendor decides whether to wire them — so maxItems: 2, optional in required.
Two reset signals but what is exactly optional? Each of them? Only the first? Binding does not allow the first to be optional.
Quoted 2 lines
- eswin,eic7700-pwm: SoC physically ties both signals to one reset — so exactly 1, required.
Then two would not be right and you need to restrict that.
Krzysztof
Re: Re: [PATCH v6 1/2] dt-bindings: pwm: dwc: add optional reset
Quoted 35 lines · Xuyang Dong
Quoted 33 lines
Quoted 23 lines
Quoted 13 lines
Quoted 13 lines
Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml required: · line 69
Quoted 9 lines
Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml +8
-0+allOf: + - $ref: pwm.yaml# + + - if: + properties: + compatible: + contains: + const: eswin,eic7700-pwmSame problem as v3 which I commented. I do not understand why your new device has also 1 reset.
Quoted 9 lines
Quoted 2 lines
Your commit msg MUST explain why 1 reset is valid.
Hi Krzysztof,
Although the PWM IP supports two clock domains, each requiring a reset, the EIC7700 implementation uses the same clock domain for both clock signals. Therefore, the eic7700-pwm only supports one reset.
Quoted 7 lines
If we speak about eic7700, explain why it has two resets now, according to schema, even though you say it has not.
But I was speaking about dw-apb-timers-pwm, which has one reset as well! Why you are not having proper constraints? Please read writing bindings document.
Hi Krzysztof,
Quoted 8 lines
Quoted 4 lines
Let me clarify the reset signals.
- snps,dw-apb-timers-pwm2: IP spec has 2 optional reset signals (one per clock domain), SoC vendor decides whether to wire them — so maxItems: 2, optional in required.Two reset signals but what is exactly optional? Each of them? Only the first? Binding does not allow the first to be optional.
Hi Krzysztof,
Thank you for the review.
For the generic snps,dw-apb-timers-pwm2 binding, both reset signals are now fully optional by not including resets in the required list.
When a single optional reset signal is used, the interface bus reset (index 0) is used by default.
Keep the YAML as follows:
+ resets:
+ minItems: 1
+ items:
+ - description: Interface bus reset
+ - description: PWM timer logic reset
Add the following description to the commit message:
Whether each signal is wired on a given SoC is a board integration decision, so the resets property is optional for snps,dw-apb-timers-pwm2. When present, up to two handles may be supplied: the bus reset is always at index 0 and the timer reset at index 1.
Quoted 5 lines
Quoted 2 lines
- eswin,eic7700-pwm: SoC physically ties both signals to one reset — so exactly 1, required.
Then two would not be right and you need to restrict that.
For the specific eswin,eic7700-pwm binding, the reset signal is required and fixed to one via conditional schema (if:then:), with maxItems: 1 and resets added to required. And add an example for eswin,eic7700-pwm. The changes are as follows:
+allOf:
+ - $ref: pwm.yaml#
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: eswin,eic7700-pwm
+ then:
+ properties:
+ resets:
+ maxItems: 1
+ required:
+ - resets
+
+ - |
+ pwm@50818000 {
+ compatible = "eswin,eic7700-pwm";
+ reg = <0x50818000 0x4000>;
+ #pwm-cells = <3>;
+ clocks = <&bus>, <&timer>;
+ clock-names = "bus", "timer";
+ resets = <&reset>;
+ };
Then change the binding's subject from "dt-bindings: pwm: dwc: add optional reset" to "dt-bindings: pwm: dwc: add eswin,eic7700-pwm compatible and resets".
Do these changes look acceptable to you?
Best regards,
Xuyang Dong
Re: [PATCH v6 1/2] dt-bindings: pwm: dwc: add optional reset
Quoted 54 lines · Xuyang Dong
Quoted 35 lines · Xuyang Dong
Quoted 33 lines
Quoted 23 lines
Quoted 13 lines
Quoted 13 lines
Review
Quoted 9 lines
(quoted diff) +8
-0+allOf: + - $ref: pwm.yaml# + + - if: + properties: + compatible: + contains: + const: eswin,eic7700-pwmSame problem as v3 which I commented. I do not understand why your new device has also 1 reset.
Quoted 9 lines
Quoted 2 lines
Your commit msg MUST explain why 1 reset is valid.
Hi Krzysztof,
Although the PWM IP supports two clock domains, each requiring a reset, the EIC7700 implementation uses the same clock domain for both clock signals. Therefore, the eic7700-pwm only supports one reset.
Quoted 7 lines
If we speak about eic7700, explain why it has two resets now, according to schema, even though you say it has not.
But I was speaking about dw-apb-timers-pwm, which has one reset as well! Why you are not having proper constraints? Please read writing bindings document.
Hi Krzysztof,
Quoted 8 lines
Quoted 4 lines
Let me clarify the reset signals.
- snps,dw-apb-timers-pwm2: IP spec has 2 optional reset signals (one per clock domain), SoC vendor decides whether to wire them — so maxItems: 2, optional in required.Two reset signals but what is exactly optional? Each of them? Only the first? Binding does not allow the first to be optional.
Hi Krzysztof,
Thank you for the review.
For the generic snps,dw-apb-timers-pwm2 binding, both reset signals are now fully optional by not including resets in the required list.
When a single optional reset signal is used, the interface bus reset (index 0) is used by default.
Quoted 8 lines
Keep the YAML as follows:
+ resets:
+ minItems: 1
+ items:
+ - description: Interface bus reset
+ - description: PWM timer logic resetAdd the following description to the commit message:
We speak about hardware, not binding. I asked, why your new device has only one reset.
Quoted 43 lines
Whether each signal is wired on a given SoC is a board integration decision, so the resets property is optional for snps,dw-apb-timers-pwm2. When present, up to two handles may be supplied: the bus reset is always at index 0 and the timer reset at index 1.
Quoted 5 lines
Quoted 2 lines
- eswin,eic7700-pwm: SoC physically ties both signals to one reset — so exactly 1, required.
Then two would not be right and you need to restrict that.
For the specific eswin,eic7700-pwm binding, the reset signal is required and fixed to one via conditional schema (if:then:), with maxItems: 1 and resets added to required. And add an example for eswin,eic7700-pwm. The changes are as follows:
(quoted diff) +24
-0+allOf: + - $ref: pwm.yaml# + + - if: + properties: + compatible: + contains: + const: eswin,eic7700-pwm + then: + properties: + resets: + maxItems: 1 + required: + - resets + + - | + pwm@50818000 { + compatible = "eswin,eic7700-pwm"; + reg = <0x50818000 0x4000>; + #pwm-cells = <3>; + clocks = <&bus>, <&timer>; + clock-names = "bus", "timer"; + resets = <&reset>; + };
Quoted 4 lines
Then change the binding's subject from "dt-bindings: pwm: dwc: add optional reset" to "dt-bindings: pwm: dwc: add eswin,eic7700-pwm compatible and resets".
Do these changes look acceptable to you?
So two resets or one reset? I am completely confused what you are replying to.
Please read writing bindings document.
Best regards,
Krzysztof
Re: Re: [PATCH v6 1/2] dt-bindings: pwm: dwc: add optional reset
Quoted 59 lines
Quoted 59 lines
Quoted 33 lines
Quoted 33 lines
Quoted 23 lines
Quoted 13 lines
Quoted 13 lines
Review
Quoted 9 lines
(quoted diff) +8
-0+allOf: + - $ref: pwm.yaml# + + - if: + properties: + compatible: + contains: + const: eswin,eic7700-pwmSame problem as v3 which I commented. I do not understand why your new device has also 1 reset.
Quoted 9 lines
Quoted 2 lines
Your commit msg MUST explain why 1 reset is valid.
Hi Krzysztof,
Although the PWM IP supports two clock domains, each requiring a reset, the EIC7700 implementation uses the same clock domain for both clock signals. Therefore, the eic7700-pwm only supports one reset.
Quoted 7 lines
If we speak about eic7700, explain why it has two resets now, according to schema, even though you say it has not.
But I was speaking about dw-apb-timers-pwm, which has one reset as well! Why you are not having proper constraints? Please read writing bindings document.
Hi Krzysztof,
Quoted 8 lines
Quoted 4 lines
Let me clarify the reset signals.
- snps,dw-apb-timers-pwm2: IP spec has 2 optional reset signals (one per clock domain), SoC vendor decides whether to wire them — so maxItems: 2, optional in required.Two reset signals but what is exactly optional? Each of them? Only the first? Binding does not allow the first to be optional.
Hi Krzysztof,
Thank you for the review.
For the generic snps,dw-apb-timers-pwm2 binding, both reset signals are now fully optional by not including resets in the required list.
When a single optional reset signal is used, the interface bus reset (index 0) is used by default.
Keep the YAML as follows:
+ resets:
+ minItems: 1
+ items:
+ - description: Interface bus reset
+ - description: PWM timer logic reset
Quoted 5 lines
Quoted 1 line
Add the following description to the commit message:
We speak about hardware, not binding. I asked, why your new device has only one reset.
Hi Krzysztof,
Thank you for the detailed review.
I don't quite understand the meaning of your sentence: "We speak about hardware, not binding. I asked, why your new device has only one reset." If you mean that the commit message in dt-bindings does not accurately describe why the EIC7700 has only one reset, I have pasted below the complete commit message that will be included in the next v7 version.
Does this commit message address your question?
The DesignWare PWM includes separate reset signals dedicated to each clock domain:
The presetn signal resets logic in pclk domain.
The timer_N_resetn signal resets logic in the timer_N_clk domain. The resets are active-low.
EIC7700 uses DesignWare IP for PWM controllers. Add ESWIN EIC7700 support in snps,dw-apb-timers-pwm2.yaml.
EIC7700 physically ties presetn signal and timer_N_resetn signal to one reset — so exactly 1, required.
Quoted 46 lines
Quoted 46 lines
Whether each signal is wired on a given SoC is a board integration decision, so the resets property is optional for snps,dw-apb-timers-pwm2. When present, up to two handles may be supplied: the bus reset is always at index 0 and the timer reset at index 1.
Quoted 5 lines
Quoted 2 lines
- eswin,eic7700-pwm: SoC physically ties both signals to one reset — so exactly 1, required.
Then two would not be right and you need to restrict that.
For the specific eswin,eic7700-pwm binding, the reset signal is required and fixed to one via conditional schema (if:then:), with maxItems: 1 and resets added to required. And add an example for eswin,eic7700-pwm. The changes are as follows:
(quoted diff) +24
-0+allOf: + - $ref: pwm.yaml# + + - if: + properties: + compatible: + contains: + const: eswin,eic7700-pwm + then: + properties: + resets: + maxItems: 1 + required: + - resets + + - | + pwm@50818000 { + compatible = "eswin,eic7700-pwm"; + reg = <0x50818000 0x4000>; + #pwm-cells = <3>; + clocks = <&bus>, <&timer>; + clock-names = "bus", "timer"; + resets = <&reset>; + };Then change the binding's subject from "dt-bindings: pwm: dwc: add optional reset" to "dt-bindings: pwm: dwc: add eswin,eic7700-pwm compatible and resets".
Quoted 7 lines
Quoted 1 line
Do these changes look acceptable to you?
So two resets or one reset? I am completely confused what you are replying to.
Please read writing bindings document.
I think the "resets" in the subject is ambiguous. It could mislead people into thinking that the EIC7700 has multiple reset signals.
I think the subject should be changed to
"dt-bindings: pwm: dwc: Add eswin compatible and resets property".
Best regards,
Xuyang Dong
Re: [PATCH v6 1/2] dt-bindings: pwm: dwc: add optional reset
Quoted 83 lines · Xuyang Dong
Quoted 59 lines
Quoted 59 lines
Quoted 33 lines
Quoted 33 lines
Quoted 23 lines
Quoted 13 lines
Quoted 13 lines
Review
Quoted 9 lines
(quoted diff) +8
-0+allOf: + - $ref: pwm.yaml# + + - if: + properties: + compatible: + contains: + const: eswin,eic7700-pwmSame problem as v3 which I commented. I do not understand why your new device has also 1 reset.
Quoted 9 lines
Quoted 2 lines
Your commit msg MUST explain why 1 reset is valid.
Hi Krzysztof,
Although the PWM IP supports two clock domains, each requiring a reset, the EIC7700 implementation uses the same clock domain for both clock signals. Therefore, the eic7700-pwm only supports one reset.
Quoted 7 lines
If we speak about eic7700, explain why it has two resets now, according to schema, even though you say it has not.
But I was speaking about dw-apb-timers-pwm, which has one reset as well! Why you are not having proper constraints? Please read writing bindings document.
Hi Krzysztof,
Quoted 8 lines
Quoted 4 lines
Let me clarify the reset signals.
- snps,dw-apb-timers-pwm2: IP spec has 2 optional reset signals (one per clock domain), SoC vendor decides whether to wire them — so maxItems: 2, optional in required.Two reset signals but what is exactly optional? Each of them? Only the first? Binding does not allow the first to be optional.
Hi Krzysztof,
Thank you for the review.
For the generic snps,dw-apb-timers-pwm2 binding, both reset signals are now fully optional by not including resets in the required list.
When a single optional reset signal is used, the interface bus reset (index 0) is used by default.
Keep the YAML as follows:
+ resets:
+ minItems: 1
+ items:
+ - description: Interface bus reset
+ - description: PWM timer logic resetQuoted 5 lines
Quoted 1 line
Add the following description to the commit message:
We speak about hardware, not binding. I asked, why your new device has only one reset.
Hi Krzysztof,
Thank you for the detailed review.
I don't quite understand the meaning of your sentence: "We speak about hardware, not binding. I asked, why your new device has only one reset." If you mean that the commit message in dt-bindings does not accurately describe why the EIC7700 has only one reset, I have pasted below the complete commit message that will be included in the next v7 version.
Does this commit message address your question?
The DesignWare PWM includes separate reset signals dedicated to each clock domain:
The presetn signal resets logic in pclk domain.
Thetimer_N_resetnsignal resets logic in thetimer_N_clkdomain. The resets are active-low.
Quoted 4 lines
EIC7700 uses DesignWare IP for PWM controllers. Add ESWIN EIC7700 support in snps,dw-apb-timers-pwm2.yaml.
EIC7700 physically ties presetn signal andtimer_N_resetnsignal to one reset — so exactly 1, required.
Your binding says that EIC7700 has one or two resets. Here you say EXACTLY one, so why do you say in the schema one or two? That was the question.
You need to make your binding strict - see writing-bindings and writing-schema.
Or any other example!
Quoted 47 lines
Quoted 47 lines
Quoted 47 lines
Whether each signal is wired on a given SoC is a board integration decision, so the resets property is optional for snps,dw-apb-timers-pwm2. When present, up to two handles may be supplied: the bus reset is always at index 0 and the timer reset at index 1.
Quoted 5 lines
Quoted 2 lines
- eswin,eic7700-pwm: SoC physically ties both signals to one reset — so exactly 1, required.
Then two would not be right and you need to restrict that.
For the specific eswin,eic7700-pwm binding, the reset signal is required and fixed to one via conditional schema (if:then:), with maxItems: 1 and resets added to required. And add an example for eswin,eic7700-pwm. The changes are as follows:
(quoted diff) +24
-0+allOf: + - $ref: pwm.yaml# + + - if: + properties: + compatible: + contains: + const: eswin,eic7700-pwm + then: + properties: + resets: + maxItems: 1 + required: + - resets + + - | + pwm@50818000 { + compatible = "eswin,eic7700-pwm"; + reg = <0x50818000 0x4000>; + #pwm-cells = <3>; + clocks = <&bus>, <&timer>; + clock-names = "bus", "timer"; + resets = <&reset>; + };Then change the binding's subject from "dt-bindings: pwm: dwc: add optional reset" to "dt-bindings: pwm: dwc: add eswin,eic7700-pwm compatible and resets".
Quoted 6 lines
Quoted 6 lines
Quoted 1 line
Do these changes look acceptable to you?
So two resets or one reset? I am completely confused what you are replying to.
Please read writing bindings document.
DID YOU FINALLY READ IT?
Quoted 6 lines
Quoted 1 line
I think the "resets" in the subject is ambiguous. It could mislead people into thinking that the EIC7700 has multiple reset signals.
I think the subject should be changed to
"dt-bindings: pwm: dwc: Add eswin compatible and resets property".
I finished with responses here.
Best regards,
Krzysztof
PATCH v6 2/2 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.
--- drivers/pwm/Kconfig | 10 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-dwc-core.c | 101 ++++++++--- drivers/pwm/pwm-dwc-of.c | 346 +++++++++++++++++++++++++++++++++++++ drivers/pwm/pwm-dwc.h | 25 ++- 5 files changed, 454 insertions(+), 29 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 +79 -22
153 lines not on this page · raw message
drivers/pwm/pwm-dwc-of.c new file +346 -0
346 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