[RFC v4 00/10] RFC on synpsys pwm driver changes

[RFC v4 00/10] RFC on synpsys pwm driver changes

[Ben Dooks]

New version of the pwm timers patch, hopefully all review comments
are sorted out, however I have not had time to fully test this and
I do not have a PCI system to test it on either.

The series has been moved around a bit to try to get some of the
simpler changes in before splitting and to make the OF driver a
single addition.

v4:
 - split pci and of into new modules
 - fixup review comments
 - fix typos in dt-bindings
v3:
- change the compatible name
- squash down pwm count patch
- fixup patch naming

v2:
- fix #pwm-cells count to be 3
- fix indetation 
- merge the two clock patches
- add HAS_IOMEM as a config dependency


Ben Dooks (10):
  dt-bindings: pwm: Document Synopsys DesignWare
    snps,pwm-dw-apb-timers-pwm2
  pwm: dwc: allow driver to be built with COMPILE_TEST
  pwm: dwc: change &pci->dev to dev in probe
  pwm: dwc: move memory alloc to own function
  pwm: dwc: use devm_pwmchip_add
  pwm: dwc: split pci out of core driver
  pwm: dwc: make timer clock configurable
  pwm: dwc: add of/platform support
  pwm: dwc: add snps,pwm-number to limit pwm count
  pwm: dwc: add PWM bit unset in get_state call

 .../bindings/pwm/snps,dw-apb-timers-pwm2.yaml |  69 ++++++
 drivers/pwm/Kconfig                           |  24 ++-
 drivers/pwm/Makefile                          |   2 +
 drivers/pwm/pwm-dwc-of.c                      |  86 ++++++++
 drivers/pwm/pwm-dwc-pci.c                     | 134 ++++++++++++
 drivers/pwm/pwm-dwc.c                         | 197 +++---------------
 drivers/pwm/pwm-dwc.h                         |  60 ++++++
 7 files changed, 402 insertions(+), 170 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml
 create mode 100644 drivers/pwm/pwm-dwc-of.c
 create mode 100644 drivers/pwm/pwm-dwc-pci.c
 create mode 100644 drivers/pwm/pwm-dwc.h

-- 
2.35.1

[RFC v4 01/10] dt-bindings: pwm: Document Synopsys DesignWare snps,pwm-dw-apb-timers-pwm2

[Ben Dooks]

Add documentation for the bindings for Synopsys' DesignWare PWM block
as we will be adding DT/platform support to the Linux driver soon.

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
v4:
 - fixed typos, added reg
v3:
 - add description and example
 - merge the snps,pwm-number into this patch
 - rename snps,pwm to snps,dw-apb-timers-pwm2
v2:
 - fix #pwm-cells to be 3
 - fix indentation and ordering issues
---
 .../bindings/pwm/snps,dw-apb-timers-pwm2.yaml | 69 +++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml

diff --git a/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml b/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml
new file mode 100644
index 000000000000..e7feae6d4404
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2022 SiFive, Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/snps,dw-apb-timers-pwm2.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Synopsys DW-APB timers PWM controller
+
+maintainers:
+  - Ben Dooks <ben.dooks@sifive.com>
+
+description:
+  This describes the DesignWare APB timers module when used in the PWM
+  mode. The IP core can be generated with various options which can
+  control the functionality, the number of PWMs available and other
+  internal controls the designer requires.
+
+  The IP block has a version register so this can be used for detection
+  instead of having to encode the IP version number in the device tree
+  comaptible.
+
+allOf:
+  - $ref: pwm.yaml#
+
+properties:
+  compatible:
+    const: snps,dw-apb-timers-pwm2
+
+  "#pwm-cells":
+    const: 3
+
+  clocks:
+    items:
+      - description: Interface bus clock
+      - description: PWM reference clock
+
+  clock-names:
+    items:
+      - const: bus
+      - const: timer
+
+  snps,pwm-number:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: The number of PWM channels configured for this instance
+    enum: [1, 2, 3, 4, 5, 6, 7, 8]
+
+  reg:
+    maxItems: 1
+
+required:
+  - "#pwm-cells"
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+
+examples:
+  - |
+    pwm: pwm@180000 {
+      #pwm-cells = <3>;
+      compatible = "snps,dw-apb-timers-pwm2";
+      reg = <0x180000 0x200>;
+      clocks = <&bus &timer>;
+      clock-names = "bus", "timer";
+    };
-- 
2.35.1

Re: [RFC v4 01/10] dt-bindings: pwm: Document Synopsys DesignWare snps,pwm-dw-apb-timers-pwm2

[Krzysztof Kozlowski]

On 17/08/2022 00:14, Ben Dooks wrote:
> Add documentation for the bindings for Synopsys' DesignWare PWM block
> as we will be adding DT/platform support to the Linux driver soon.
> 
> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
> ---
> v4:
>  - fixed typos, added reg
> v3:
>  - add description and example
>  - merge the snps,pwm-number into this patch
>  - rename snps,pwm to snps,dw-apb-timers-pwm2
> v2:
>  - fix #pwm-cells to be 3
>  - fix indentation and ordering issues
> ---
>  .../bindings/pwm/snps,dw-apb-timers-pwm2.yaml | 69 +++++++++++++++++++
>  1 file changed, 69 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml b/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml
> new file mode 100644
> index 000000000000..e7feae6d4404
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2022 SiFive, Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pwm/snps,dw-apb-timers-pwm2.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Synopsys DW-APB timers PWM controller
> +
> +maintainers:
> +  - Ben Dooks <ben.dooks@sifive.com>
> +
> +description:
> +  This describes the DesignWare APB timers module when used in the PWM
> +  mode. The IP core can be generated with various options which can
> +  control the functionality, the number of PWMs available and other
> +  internal controls the designer requires.
> +
> +  The IP block has a version register so this can be used for detection
> +  instead of having to encode the IP version number in the device tree
> +  comaptible.
> +
> +allOf:
> +  - $ref: pwm.yaml#
> +
> +properties:
> +  compatible:
> +    const: snps,dw-apb-timers-pwm2
> +
> +  "#pwm-cells":
> +    const: 3
> +
> +  clocks:
> +    items:
> +      - description: Interface bus clock
> +      - description: PWM reference clock
> +
> +  clock-names:
> +    items:
> +      - const: bus
> +      - const: timer
> +
> +  snps,pwm-number:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: The number of PWM channels configured for this instance
> +    enum: [1, 2, 3, 4, 5, 6, 7, 8]
> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - "#pwm-cells"
> +  - compatible
> +  - reg

Keep the same order as list of properties.

> +  - clocks
> +  - clock-names
> +
> +additionalProperties: false
> +
> +

Just one blank line.

> +examples:
> +  - |
> +    pwm: pwm@180000 {
> +      #pwm-cells = <3>;
> +      compatible = "snps,dw-apb-timers-pwm2";
> +      reg = <0x180000 0x200>;

The convention of DTS is: compatible, then reg, then rest of properties.

> +      clocks = <&bus &timer>;

You put here one item, not two. This has to be <&bus>, <&timer>

> +      clock-names = "bus", "timer";
> +    };


Best regards,
Krzysztof

Re: [RFC v4 01/10] dt-bindings: pwm: Document Synopsys DesignWare snps,pwm-dw-apb-timers-pwm2

[Ben Dooks]

On 17/08/2022 06:58, Krzysztof Kozlowski wrote:
> On 17/08/2022 00:14, Ben Dooks wrote:
>> Add documentation for the bindings for Synopsys' DesignWare PWM block
>> as we will be adding DT/platform support to the Linux driver soon.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
>> ---
>> v4:
>>   - fixed typos, added reg
>> v3:
>>   - add description and example
>>   - merge the snps,pwm-number into this patch
>>   - rename snps,pwm to snps,dw-apb-timers-pwm2
>> v2:
>>   - fix #pwm-cells to be 3
>>   - fix indentation and ordering issues
>> ---
>>   .../bindings/pwm/snps,dw-apb-timers-pwm2.yaml | 69 +++++++++++++++++++
>>   1 file changed, 69 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml b/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml
>> new file mode 100644
>> index 000000000000..e7feae6d4404
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml
>> @@ -0,0 +1,69 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +# Copyright (C) 2022 SiFive, Inc.
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pwm/snps,dw-apb-timers-pwm2.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Synopsys DW-APB timers PWM controller
>> +
>> +maintainers:
>> +  - Ben Dooks <ben.dooks@sifive.com>
>> +
>> +description:
>> +  This describes the DesignWare APB timers module when used in the PWM
>> +  mode. The IP core can be generated with various options which can
>> +  control the functionality, the number of PWMs available and other
>> +  internal controls the designer requires.
>> +
>> +  The IP block has a version register so this can be used for detection
>> +  instead of having to encode the IP version number in the device tree
>> +  comaptible.
>> +
>> +allOf:
>> +  - $ref: pwm.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    const: snps,dw-apb-timers-pwm2
>> +
>> +  "#pwm-cells":
>> +    const: 3
>> +
>> +  clocks:
>> +    items:
>> +      - description: Interface bus clock
>> +      - description: PWM reference clock
>> +
>> +  clock-names:
>> +    items:
>> +      - const: bus
>> +      - const: timer
>> +
>> +  snps,pwm-number:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: The number of PWM channels configured for this instance
>> +    enum: [1, 2, 3, 4, 5, 6, 7, 8]
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +required:
>> +  - "#pwm-cells"
>> +  - compatible
>> +  - reg
> 
> Keep the same order as list of properties.

ok, will fix.

>> +  - clocks
>> +  - clock-names
>> +
>> +additionalProperties: false
>> +
>> +
> 
> Just one blank line.
> 
>> +examples:
>> +  - |
>> +    pwm: pwm@180000 {
>> +      #pwm-cells = <3>;
>> +      compatible = "snps,dw-apb-timers-pwm2";
>> +      reg = <0x180000 0x200>;
> 
> The convention of DTS is: compatible, then reg, then rest of properties.
> 
>> +      clocks = <&bus &timer>;
> 
> You put here one item, not two. This has to be <&bus>, <&timer>
> 
>> +      clock-names = "bus", "timer";
>> +    };

Argh, thanks, I completely missed this as our platform only has
one clock provider for this (the bus and timer clocks are the
same)


> Best regards,
> Krzysztof

Thanks for the review.

I guess this is now too late for 6.0-rc ?

Re: [RFC v4 01/10] dt-bindings: pwm: Document Synopsys DesignWare snps,pwm-dw-apb-timers-pwm2

[Krzysztof Kozlowski]

On 18/08/2022 16:43, Ben Dooks wrote:
> 
>> Best regards,
>> Krzysztof
> 
> Thanks for the review.
> 
> I guess this is now too late for 6.0-rc ?

Code for 6.0-rc would have to be accepted by subsystem maintainer
between 3 to 5 weeks ago (depending on maintainer), so it's late by 3-5
weeks.

Best regards,
Krzysztof

[RFC v4 02/10] pwm: dwc: allow driver to be built with COMPILE_TEST

[Ben Dooks]

Allow dwc driver to be built with COMPILE_TEST should allow
better coverage when build testing.

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
v4:
 - moved to earlier in the series
v3:
 - add HAS_IOMEM depdency for compile testing
---
 drivers/pwm/Kconfig | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 60d13a949bc5..3f3c53af4a56 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -176,7 +176,8 @@ config PWM_CROS_EC
 
 config PWM_DWC
 	tristate "DesignWare PWM Controller"
-	depends on PCI
+	depends on PCI || COMPILE_TEST
+	depends on HAS_IOMEM
 	help
 	  PWM driver for Synopsys DWC PWM Controller attached to a PCI bus.
 
-- 
2.35.1

Re: [RFC v4 02/10] pwm: dwc: allow driver to be built with COMPILE_TEST

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 485 bytes --]

Hello,

On Tue, Aug 16, 2022 at 10:14:46PM +0100, Ben Dooks wrote:
> Allow dwc driver to be built with COMPILE_TEST should allow
> better coverage when build testing.
> 
> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>

Very welcome, so:

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC v4 02/10] pwm: dwc: allow driver to be built with COMPILE_TEST

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 598 bytes --]

Hello Thierry,

On Tue, Aug 16, 2022 at 10:14:46PM +0100, Ben Dooks wrote:
> Allow dwc driver to be built with COMPILE_TEST should allow
> better coverage when build testing.
> 
> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>

You marked this patch as "changes requested" without feedback. This
patch is part of a bigger series, but makes sense to be applied stand
alone, too.

Ditto for patch #3 IMHO.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC v4 03/10] pwm: dwc: change &pci->dev to dev in probe

[Ben Dooks]

The dwc_pwm_probe() assignes dev to be &pci->dev but then uses
&pci->dev throughout the function. Change these all to the be
'dev' variable to make lines shorter.

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-dwc.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index 7568300bb11e..c706ef9a7ba1 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -202,14 +202,13 @@ static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
 	struct dwc_pwm *dwc;
 	int ret;
 
-	dwc = devm_kzalloc(&pci->dev, sizeof(*dwc), GFP_KERNEL);
+	dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
 	if (!dwc)
 		return -ENOMEM;
 
 	ret = pcim_enable_device(pci);
 	if (ret) {
-		dev_err(&pci->dev,
-			"Failed to enable device (%pe)\n", ERR_PTR(ret));
+		dev_err(dev, "Failed to enable device (%pe)\n", ERR_PTR(ret));
 		return ret;
 	}
 
@@ -217,14 +216,13 @@ static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
 
 	ret = pcim_iomap_regions(pci, BIT(0), pci_name(pci));
 	if (ret) {
-		dev_err(&pci->dev,
-			"Failed to iomap PCI BAR (%pe)\n", ERR_PTR(ret));
+		dev_err(dev, "Failed to iomap PCI BAR (%pe)\n", ERR_PTR(ret));
 		return ret;
 	}
 
 	dwc->base = pcim_iomap_table(pci)[0];
 	if (!dwc->base) {
-		dev_err(&pci->dev, "Base address missing\n");
+		dev_err(dev, "Base address missing\n");
 		return -ENOMEM;
 	}
 
-- 
2.35.1

[RFC v4 04/10] pwm: dwc: move memory alloc to own function

[Ben Dooks]

In preparation for adding other bus support, move the allocation
of the pwm struct out of the main driver code.

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
 drivers/pwm/pwm-dwc.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index c706ef9a7ba1..61f11e0a9319 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -196,13 +196,29 @@ static const struct pwm_ops dwc_pwm_ops = {
 	.owner = THIS_MODULE,
 };
 
+static struct dwc_pwm *dwc_pwm_alloc(struct device *dev)
+{
+	struct dwc_pwm *dwc;
+
+	dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
+	if (!dwc)
+		return NULL;
+
+	dwc->chip.dev = dev;
+	dwc->chip.ops = &dwc_pwm_ops;
+	dwc->chip.npwm = DWC_TIMERS_TOTAL;
+
+	dev_set_drvdata(dev, dwc);
+	return dwc;
+}
+
 static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
 {
 	struct device *dev = &pci->dev;
 	struct dwc_pwm *dwc;
 	int ret;
 
-	dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
+	dwc = dwc_pwm_alloc(dev);
 	if (!dwc)
 		return -ENOMEM;
 
@@ -226,12 +242,6 @@ static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
 		return -ENOMEM;
 	}
 
-	pci_set_drvdata(pci, dwc);
-
-	dwc->chip.dev = dev;
-	dwc->chip.ops = &dwc_pwm_ops;
-	dwc->chip.npwm = DWC_TIMERS_TOTAL;
-
 	ret = pwmchip_add(&dwc->chip);
 	if (ret)
 		return ret;
-- 
2.35.1

[RFC v4 05/10] pwm: dwc: use devm_pwmchip_add

[Ben Dooks]

Use devm_pwmchip_add() to add the pwm chip to avoid having to manually
remove it (useful for the next patch which adds the platform-device
support).

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
 drivers/pwm/pwm-dwc.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index 61f11e0a9319..56cde9da2c0e 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -242,7 +242,7 @@ static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
 		return -ENOMEM;
 	}
 
-	ret = pwmchip_add(&dwc->chip);
+	ret = devm_pwmchip_add(dev, &dwc->chip);
 	if (ret)
 		return ret;
 
@@ -254,12 +254,8 @@ static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
 
 static void dwc_pwm_remove(struct pci_dev *pci)
 {
-	struct dwc_pwm *dwc = pci_get_drvdata(pci);
-
 	pm_runtime_forbid(&pci->dev);
 	pm_runtime_get_noresume(&pci->dev);
-
-	pwmchip_remove(&dwc->chip);
 }
 
 #ifdef CONFIG_PM_SLEEP
-- 
2.35.1

Re: [RFC v4 05/10] pwm: dwc: use devm_pwmchip_add

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 500 bytes --]

On Tue, Aug 16, 2022 at 10:14:49PM +0100, Ben Dooks wrote:
> Use devm_pwmchip_add() to add the pwm chip to avoid having to manually
> remove it (useful for the next patch which adds the platform-device
> support).
> 
> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>

Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC v4 06/10] pwm: dwc: split pci out of core driver

[Ben Dooks]

Moving towards adding non-pci support for the driver, move the pci
parts out of the core into their own module. This is partly due to
the module_driver() code only being allowed once in a module and also
to avoid a number of #ifdef if we build a single file in a system
without pci support.

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
 drivers/pwm/Kconfig       |  14 +++-
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-dwc-pci.c | 133 ++++++++++++++++++++++++++++++++
 drivers/pwm/pwm-dwc.c     | 158 +-------------------------------------
 drivers/pwm/pwm-dwc.h     |  58 ++++++++++++++
 5 files changed, 207 insertions(+), 157 deletions(-)
 create mode 100644 drivers/pwm/pwm-dwc-pci.c
 create mode 100644 drivers/pwm/pwm-dwc.h

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 3f3c53af4a56..a9f1c554db2b 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -175,15 +175,23 @@ config PWM_CROS_EC
 	  Controller.
 
 config PWM_DWC
-	tristate "DesignWare PWM Controller"
-	depends on PCI || COMPILE_TEST
+	tristate "DesignWare PWM Controller core"
 	depends on HAS_IOMEM
 	help
-	  PWM driver for Synopsys DWC PWM Controller attached to a PCI bus.
+	  PWM driver for Synopsys DWC PWM Controller.
 
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-dwc.
 
+config PWM_DWC_PCI
+	tristate "DesignWare PWM Controller core"
+	depends on PWM_DWC && HAS_IOMEM && PCI
+	help
+	  PWM driver for Synopsys DWC PWM Controller attached to a PCI bus.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-dwc-pci.
+
 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 7bf1a29f02b8..a70d36623129 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_PWM_CLPS711X)	+= pwm-clps711x.o
 obj-$(CONFIG_PWM_CRC)		+= pwm-crc.o
 obj-$(CONFIG_PWM_CROS_EC)	+= pwm-cros-ec.o
 obj-$(CONFIG_PWM_DWC)		+= pwm-dwc.o
+obj-$(CONFIG_PWM_DWC_PCI)	+= pwm-dwc-pci.o
 obj-$(CONFIG_PWM_EP93XX)	+= pwm-ep93xx.o
 obj-$(CONFIG_PWM_FSL_FTM)	+= pwm-fsl-ftm.o
 obj-$(CONFIG_PWM_HIBVT)		+= pwm-hibvt.o
diff --git a/drivers/pwm/pwm-dwc-pci.c b/drivers/pwm/pwm-dwc-pci.c
new file mode 100644
index 000000000000..2213d0e7f3c8
--- /dev/null
+++ b/drivers/pwm/pwm-dwc-pci.c
@@ -0,0 +1,133 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * DesignWare PWM Controller driver (PCI part)
+ *
+ * Copyright (C) 2018-2020 Intel Corporation
+ *
+ * Author: Felipe Balbi (Intel)
+ * Author: Jarkko Nikula <jarkko.nikula@linux.intel.com>
+ * Author: Raymond Tan <raymond.tan@intel.com>
+ *
+ * Limitations:
+ * - The hardware cannot generate a 0 % or 100 % duty cycle. Both high and low
+ *   periods are one or more input clock periods long.
+ */
+
+#include <linux/bitops.h>
+#include <linux/export.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/pm_runtime.h>
+#include <linux/pwm.h>
+
+#include "pwm-dwc.h"
+
+static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
+{
+	struct device *dev = &pci->dev;
+	struct dwc_pwm *dwc;
+	int ret;
+
+	dwc = dwc_pwm_alloc(dev);
+	if (!dwc)
+		return -ENOMEM;
+
+	ret = pcim_enable_device(pci);
+	if (ret) {
+		dev_err(dev, "Failed to enable device (%pe)\n", ERR_PTR(ret));
+		return ret;
+	}
+
+	pci_set_master(pci);
+
+	ret = pcim_iomap_regions(pci, BIT(0), pci_name(pci));
+	if (ret) {
+		dev_err(dev, "Failed to iomap PCI BAR (%pe)\n", ERR_PTR(ret));
+		return ret;
+	}
+
+	dwc->base = pcim_iomap_table(pci)[0];
+	if (!dwc->base) {
+		dev_err(dev, "Base address missing\n");
+		return -ENOMEM;
+	}
+
+	ret = devm_pwmchip_add(dev, &dwc->chip);
+	if (ret)
+		return ret;
+
+	pm_runtime_put(dev);
+	pm_runtime_allow(dev);
+
+	return 0;
+}
+
+static void dwc_pwm_remove(struct pci_dev *pci)
+{
+	pm_runtime_forbid(&pci->dev);
+	pm_runtime_get_noresume(&pci->dev);
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int dwc_pwm_suspend(struct device *dev)
+{
+	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
+	struct dwc_pwm *dwc = pci_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < DWC_TIMERS_TOTAL; i++) {
+		if (dwc->chip.pwms[i].state.enabled) {
+			dev_err(dev, "PWM %u in use by consumer (%s)\n",
+				i, dwc->chip.pwms[i].label);
+			return -EBUSY;
+		}
+		dwc->ctx[i].cnt = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(i));
+		dwc->ctx[i].cnt2 = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(i));
+		dwc->ctx[i].ctrl = dwc_pwm_readl(dwc, DWC_TIM_CTRL(i));
+	}
+
+	return 0;
+}
+
+static int dwc_pwm_resume(struct device *dev)
+{
+	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
+	struct dwc_pwm *dwc = pci_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < DWC_TIMERS_TOTAL; i++) {
+		dwc_pwm_writel(dwc, dwc->ctx[i].cnt, DWC_TIM_LD_CNT(i));
+		dwc_pwm_writel(dwc, dwc->ctx[i].cnt2, DWC_TIM_LD_CNT2(i));
+		dwc_pwm_writel(dwc, dwc->ctx[i].ctrl, DWC_TIM_CTRL(i));
+	}
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(dwc_pwm_pm_ops, dwc_pwm_suspend, dwc_pwm_resume);
+
+static const struct pci_device_id dwc_pwm_id_table[] = {
+	{ PCI_VDEVICE(INTEL, 0x4bb7) }, /* Elkhart Lake */
+	{  }	/* Terminating Entry */
+};
+MODULE_DEVICE_TABLE(pci, dwc_pwm_id_table);
+
+static struct pci_driver dwc_pwm_driver = {
+	.name = "pwm-dwc",
+	.probe = dwc_pwm_probe,
+	.remove = dwc_pwm_remove,
+	.id_table = dwc_pwm_id_table,
+	.driver = {
+		.pm = &dwc_pwm_pm_ops,
+	},
+};
+
+module_pci_driver(dwc_pwm_driver);
+
+MODULE_AUTHOR("Felipe Balbi (Intel)");
+MODULE_AUTHOR("Jarkko Nikula <jarkko.nikula@linux.intel.com>");
+MODULE_AUTHOR("Raymond Tan <raymond.tan@intel.com>");
+MODULE_DESCRIPTION("DesignWare PWM Controller");
+MODULE_LICENSE("GPL");
diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index 56cde9da2c0e..90a8ae1252a1 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -1,16 +1,12 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * DesignWare PWM Controller driver
+ * DesignWare PWM Controller driver core
  *
  * Copyright (C) 2018-2020 Intel Corporation
  *
  * Author: Felipe Balbi (Intel)
  * Author: Jarkko Nikula <jarkko.nikula@linux.intel.com>
  * Author: Raymond Tan <raymond.tan@intel.com>
- *
- * Limitations:
- * - The hardware cannot generate a 0 % or 100 % duty cycle. Both high and low
- *   periods are one or more input clock periods long.
  */
 
 #include <linux/bitops.h>
@@ -21,51 +17,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/pwm.h>
 
-#define DWC_TIM_LD_CNT(n)	((n) * 0x14)
-#define DWC_TIM_LD_CNT2(n)	(((n) * 4) + 0xb0)
-#define DWC_TIM_CUR_VAL(n)	(((n) * 0x14) + 0x04)
-#define DWC_TIM_CTRL(n)		(((n) * 0x14) + 0x08)
-#define DWC_TIM_EOI(n)		(((n) * 0x14) + 0x0c)
-#define DWC_TIM_INT_STS(n)	(((n) * 0x14) + 0x10)
-
-#define DWC_TIMERS_INT_STS	0xa0
-#define DWC_TIMERS_EOI		0xa4
-#define DWC_TIMERS_RAW_INT_STS	0xa8
-#define DWC_TIMERS_COMP_VERSION	0xac
-
-#define DWC_TIMERS_TOTAL	8
-#define DWC_CLK_PERIOD_NS	10
-
-/* 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)
-
-struct dwc_pwm_ctx {
-	u32 cnt;
-	u32 cnt2;
-	u32 ctrl;
-};
-
-struct dwc_pwm {
-	struct pwm_chip chip;
-	void __iomem *base;
-	struct dwc_pwm_ctx ctx[DWC_TIMERS_TOTAL];
-};
-#define to_dwc_pwm(p)	(container_of((p), struct dwc_pwm, chip))
-
-static inline u32 dwc_pwm_readl(struct dwc_pwm *dwc, u32 offset)
-{
-	return readl(dwc->base + offset);
-}
-
-static inline void dwc_pwm_writel(struct dwc_pwm *dwc, u32 value, u32 offset)
-{
-	writel(value, dwc->base + offset);
-}
+#include "pwm-dwc.h"
 
 static void __dwc_pwm_set_enable(struct dwc_pwm *dwc, int pwm, int enabled)
 {
@@ -196,7 +148,7 @@ static const struct pwm_ops dwc_pwm_ops = {
 	.owner = THIS_MODULE,
 };
 
-static struct dwc_pwm *dwc_pwm_alloc(struct device *dev)
+struct dwc_pwm *dwc_pwm_alloc(struct device *dev)
 {
 	struct dwc_pwm *dwc;
 
@@ -211,109 +163,7 @@ static struct dwc_pwm *dwc_pwm_alloc(struct device *dev)
 	dev_set_drvdata(dev, dwc);
 	return dwc;
 }
-
-static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
-{
-	struct device *dev = &pci->dev;
-	struct dwc_pwm *dwc;
-	int ret;
-
-	dwc = dwc_pwm_alloc(dev);
-	if (!dwc)
-		return -ENOMEM;
-
-	ret = pcim_enable_device(pci);
-	if (ret) {
-		dev_err(dev, "Failed to enable device (%pe)\n", ERR_PTR(ret));
-		return ret;
-	}
-
-	pci_set_master(pci);
-
-	ret = pcim_iomap_regions(pci, BIT(0), pci_name(pci));
-	if (ret) {
-		dev_err(dev, "Failed to iomap PCI BAR (%pe)\n", ERR_PTR(ret));
-		return ret;
-	}
-
-	dwc->base = pcim_iomap_table(pci)[0];
-	if (!dwc->base) {
-		dev_err(dev, "Base address missing\n");
-		return -ENOMEM;
-	}
-
-	ret = devm_pwmchip_add(dev, &dwc->chip);
-	if (ret)
-		return ret;
-
-	pm_runtime_put(dev);
-	pm_runtime_allow(dev);
-
-	return 0;
-}
-
-static void dwc_pwm_remove(struct pci_dev *pci)
-{
-	pm_runtime_forbid(&pci->dev);
-	pm_runtime_get_noresume(&pci->dev);
-}
-
-#ifdef CONFIG_PM_SLEEP
-static int dwc_pwm_suspend(struct device *dev)
-{
-	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
-	struct dwc_pwm *dwc = pci_get_drvdata(pdev);
-	int i;
-
-	for (i = 0; i < DWC_TIMERS_TOTAL; i++) {
-		if (dwc->chip.pwms[i].state.enabled) {
-			dev_err(dev, "PWM %u in use by consumer (%s)\n",
-				i, dwc->chip.pwms[i].label);
-			return -EBUSY;
-		}
-		dwc->ctx[i].cnt = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(i));
-		dwc->ctx[i].cnt2 = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(i));
-		dwc->ctx[i].ctrl = dwc_pwm_readl(dwc, DWC_TIM_CTRL(i));
-	}
-
-	return 0;
-}
-
-static int dwc_pwm_resume(struct device *dev)
-{
-	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
-	struct dwc_pwm *dwc = pci_get_drvdata(pdev);
-	int i;
-
-	for (i = 0; i < DWC_TIMERS_TOTAL; i++) {
-		dwc_pwm_writel(dwc, dwc->ctx[i].cnt, DWC_TIM_LD_CNT(i));
-		dwc_pwm_writel(dwc, dwc->ctx[i].cnt2, DWC_TIM_LD_CNT2(i));
-		dwc_pwm_writel(dwc, dwc->ctx[i].ctrl, DWC_TIM_CTRL(i));
-	}
-
-	return 0;
-}
-#endif
-
-static SIMPLE_DEV_PM_OPS(dwc_pwm_pm_ops, dwc_pwm_suspend, dwc_pwm_resume);
-
-static const struct pci_device_id dwc_pwm_id_table[] = {
-	{ PCI_VDEVICE(INTEL, 0x4bb7) }, /* Elkhart Lake */
-	{  }	/* Terminating Entry */
-};
-MODULE_DEVICE_TABLE(pci, dwc_pwm_id_table);
-
-static struct pci_driver dwc_pwm_driver = {
-	.name = "pwm-dwc",
-	.probe = dwc_pwm_probe,
-	.remove = dwc_pwm_remove,
-	.id_table = dwc_pwm_id_table,
-	.driver = {
-		.pm = &dwc_pwm_pm_ops,
-	},
-};
-
-module_pci_driver(dwc_pwm_driver);
+EXPORT_SYMBOL_GPL(dwc_pwm_alloc);
 
 MODULE_AUTHOR("Felipe Balbi (Intel)");
 MODULE_AUTHOR("Jarkko Nikula <jarkko.nikula@linux.intel.com>");
diff --git a/drivers/pwm/pwm-dwc.h b/drivers/pwm/pwm-dwc.h
new file mode 100644
index 000000000000..68f98eb76152
--- /dev/null
+++ b/drivers/pwm/pwm-dwc.h
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * DesignWare PWM Controller driver
+ *
+ * Copyright (C) 2018-2020 Intel Corporation
+ *
+ * Author: Felipe Balbi (Intel)
+ * Author: Jarkko Nikula <jarkko.nikula@linux.intel.com>
+ * Author: Raymond Tan <raymond.tan@intel.com>
+ */
+
+#define DWC_TIM_LD_CNT(n)	((n) * 0x14)
+#define DWC_TIM_LD_CNT2(n)	(((n) * 4) + 0xb0)
+#define DWC_TIM_CUR_VAL(n)	(((n) * 0x14) + 0x04)
+#define DWC_TIM_CTRL(n)		(((n) * 0x14) + 0x08)
+#define DWC_TIM_EOI(n)		(((n) * 0x14) + 0x0c)
+#define DWC_TIM_INT_STS(n)	(((n) * 0x14) + 0x10)
+
+#define DWC_TIMERS_INT_STS	0xa0
+#define DWC_TIMERS_EOI		0xa4
+#define DWC_TIMERS_RAW_INT_STS	0xa8
+#define DWC_TIMERS_COMP_VERSION	0xac
+
+#define DWC_TIMERS_TOTAL	8
+#define DWC_CLK_PERIOD_NS	10
+
+/* 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)
+
+struct dwc_pwm_ctx {
+	u32 cnt;
+	u32 cnt2;
+	u32 ctrl;
+};
+
+struct dwc_pwm {
+	struct pwm_chip chip;
+	void __iomem *base;
+	struct dwc_pwm_ctx ctx[DWC_TIMERS_TOTAL];
+};
+#define to_dwc_pwm(p)	(container_of((p), struct dwc_pwm, chip))
+
+static inline u32 dwc_pwm_readl(struct dwc_pwm *dwc, u32 offset)
+{
+	return readl(dwc->base + offset);
+}
+
+static inline void dwc_pwm_writel(struct dwc_pwm *dwc, u32 value, u32 offset)
+{
+	writel(value, dwc->base + offset);
+}
+
+extern struct dwc_pwm *dwc_pwm_alloc(struct device *dev);
-- 
2.35.1

Re: [RFC v4 06/10] pwm: dwc: split pci out of core driver

[Jarkko Nikula]

Hi

On 8/17/22 00:14, Ben Dooks wrote:
> Moving towards adding non-pci support for the driver, move the pci
> parts out of the core into their own module. This is partly due to
> the module_driver() code only being allowed once in a module and also
> to avoid a number of #ifdef if we build a single file in a system
> without pci support.
> 
> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
> ---

I quickly tested this on Intel Elkhart and didn't notice any regression. 
A few comments below.

>   drivers/pwm/Kconfig       |  14 +++-
>   drivers/pwm/Makefile      |   1 +
>   drivers/pwm/pwm-dwc-pci.c | 133 ++++++++++++++++++++++++++++++++
>   drivers/pwm/pwm-dwc.c     | 158 +-------------------------------------
>   drivers/pwm/pwm-dwc.h     |  58 ++++++++++++++
>   5 files changed, 207 insertions(+), 157 deletions(-)
>   create mode 100644 drivers/pwm/pwm-dwc-pci.c
>   create mode 100644 drivers/pwm/pwm-dwc.h
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 3f3c53af4a56..a9f1c554db2b 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -175,15 +175,23 @@ config PWM_CROS_EC
>   	  Controller.
>   
>   config PWM_DWC
> -	tristate "DesignWare PWM Controller"
> -	depends on PCI || COMPILE_TEST
> +	tristate "DesignWare PWM Controller core"
>   	depends on HAS_IOMEM
>   	help
> -	  PWM driver for Synopsys DWC PWM Controller attached to a PCI bus.
> +	  PWM driver for Synopsys DWC PWM Controller.
>   
>   	  To compile this driver as a module, choose M here: the module
>   	  will be called pwm-dwc.
>   
> +config PWM_DWC_PCI
> +	tristate "DesignWare PWM Controller core"

Same text as core part has. How about "DesignWare PWM Controller PCI 
driver"?

> +	depends on PWM_DWC && HAS_IOMEM && PCI
> +	help
> +	  PWM driver for Synopsys DWC PWM Controller attached to a PCI bus.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-dwc-pci.
> +
>   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 7bf1a29f02b8..a70d36623129 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_PWM_CLPS711X)	+= pwm-clps711x.o
>   obj-$(CONFIG_PWM_CRC)		+= pwm-crc.o
>   obj-$(CONFIG_PWM_CROS_EC)	+= pwm-cros-ec.o
>   obj-$(CONFIG_PWM_DWC)		+= pwm-dwc.o
> +obj-$(CONFIG_PWM_DWC_PCI)	+= pwm-dwc-pci.o
>   obj-$(CONFIG_PWM_EP93XX)	+= pwm-ep93xx.o
>   obj-$(CONFIG_PWM_FSL_FTM)	+= pwm-fsl-ftm.o
>   obj-$(CONFIG_PWM_HIBVT)		+= pwm-hibvt.o
> diff --git a/drivers/pwm/pwm-dwc-pci.c b/drivers/pwm/pwm-dwc-pci.c
> new file mode 100644
> index 000000000000..2213d0e7f3c8
> --- /dev/null
> +++ b/drivers/pwm/pwm-dwc-pci.c
> @@ -0,0 +1,133 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DesignWare PWM Controller driver (PCI part)
> + *
> + * Copyright (C) 2018-2020 Intel Corporation
> + *
> + * Author: Felipe Balbi (Intel)
> + * Author: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> + * Author: Raymond Tan <raymond.tan@intel.com>
> + *
> + * Limitations:
> + * - The hardware cannot generate a 0 % or 100 % duty cycle. Both high and low
> + *   periods are one or more input clock periods long.
> + */
> +

I think this is more common limitation rather than PCI part.

> --- a/drivers/pwm/pwm-dwc.c
> +++ b/drivers/pwm/pwm-dwc.c
> @@ -1,16 +1,12 @@
>   // SPDX-License-Identifier: GPL-2.0
>   /*
> - * DesignWare PWM Controller driver
> + * DesignWare PWM Controller driver core
>    *
>    * Copyright (C) 2018-2020 Intel Corporation
>    *
>    * Author: Felipe Balbi (Intel)
>    * Author: Jarkko Nikula <jarkko.nikula@linux.intel.com>
>    * Author: Raymond Tan <raymond.tan@intel.com>
> - *
> - * Limitations:
> - * - The hardware cannot generate a 0 % or 100 % duty cycle. Both high and low
> - *   periods are one or more input clock periods long.
>    */

Relates to previous comment, is there reason why this limitation is 
removed from the core part?

> --- /dev/null
> +++ b/drivers/pwm/pwm-dwc.h
> +#define DWC_CLK_PERIOD_NS	10

Perhaps this addition can be removed if patch 7/10 goes before this 
patch? It's anyway specific to PCI part only.

Re: [RFC v4 06/10] pwm: dwc: split pci out of core driver

[Ben Dooks]

On 19/08/2022 14:38, Jarkko Nikula wrote:
> Hi
> 
> On 8/17/22 00:14, Ben Dooks wrote:
>> Moving towards adding non-pci support for the driver, move the pci
>> parts out of the core into their own module. This is partly due to
>> the module_driver() code only being allowed once in a module and also
>> to avoid a number of #ifdef if we build a single file in a system
>> without pci support.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
>> ---
> 
> I quickly tested this on Intel Elkhart and didn't notice any regression. 
> A few comments below.
> 
>>   drivers/pwm/Kconfig       |  14 +++-
>>   drivers/pwm/Makefile      |   1 +
>>   drivers/pwm/pwm-dwc-pci.c | 133 ++++++++++++++++++++++++++++++++
>>   drivers/pwm/pwm-dwc.c     | 158 +-------------------------------------
>>   drivers/pwm/pwm-dwc.h     |  58 ++++++++++++++
>>   5 files changed, 207 insertions(+), 157 deletions(-)
>>   create mode 100644 drivers/pwm/pwm-dwc-pci.c
>>   create mode 100644 drivers/pwm/pwm-dwc.h
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index 3f3c53af4a56..a9f1c554db2b 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -175,15 +175,23 @@ config PWM_CROS_EC
>>         Controller.
>>   config PWM_DWC
>> -    tristate "DesignWare PWM Controller"
>> -    depends on PCI || COMPILE_TEST
>> +    tristate "DesignWare PWM Controller core"
>>       depends on HAS_IOMEM
>>       help
>> -      PWM driver for Synopsys DWC PWM Controller attached to a PCI bus.
>> +      PWM driver for Synopsys DWC PWM Controller.
>>         To compile this driver as a module, choose M here: the module
>>         will be called pwm-dwc.
>> +config PWM_DWC_PCI
>> +    tristate "DesignWare PWM Controller core"
> 
> Same text as core part has. How about "DesignWare PWM Controller PCI 
> driver"?

Thanks, did notice a couple of kconfig issues so will look at that.

> 
>> +    depends on PWM_DWC && HAS_IOMEM && PCI
>> +    help
>> +      PWM driver for Synopsys DWC PWM Controller attached to a PCI bus.
>> +
>> +      To compile this driver as a module, choose M here: the module
>> +      will be called pwm-dwc-pci.
>> +
>>   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 7bf1a29f02b8..a70d36623129 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -15,6 +15,7 @@ obj-$(CONFIG_PWM_CLPS711X)    += pwm-clps711x.o
>>   obj-$(CONFIG_PWM_CRC)        += pwm-crc.o
>>   obj-$(CONFIG_PWM_CROS_EC)    += pwm-cros-ec.o
>>   obj-$(CONFIG_PWM_DWC)        += pwm-dwc.o
>> +obj-$(CONFIG_PWM_DWC_PCI)    += pwm-dwc-pci.o
>>   obj-$(CONFIG_PWM_EP93XX)    += pwm-ep93xx.o
>>   obj-$(CONFIG_PWM_FSL_FTM)    += pwm-fsl-ftm.o
>>   obj-$(CONFIG_PWM_HIBVT)        += pwm-hibvt.o
>> diff --git a/drivers/pwm/pwm-dwc-pci.c b/drivers/pwm/pwm-dwc-pci.c
>> new file mode 100644
>> index 000000000000..2213d0e7f3c8
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-dwc-pci.c
>> @@ -0,0 +1,133 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * DesignWare PWM Controller driver (PCI part)
>> + *
>> + * Copyright (C) 2018-2020 Intel Corporation
>> + *
>> + * Author: Felipe Balbi (Intel)
>> + * Author: Jarkko Nikula <jarkko.nikula@linux.intel.com>
>> + * Author: Raymond Tan <raymond.tan@intel.com>
>> + *
>> + * Limitations:
>> + * - The hardware cannot generate a 0 % or 100 % duty cycle. Both 
>> high and low
>> + *   periods are one or more input clock periods long.
>> + */
>> +
> 
> I think this is more common limitation rather than PCI part.

The PCI is based off an core without the support, it is added
as a build option as of (IIRC) the 2.13 core.

> 
>> --- a/drivers/pwm/pwm-dwc.c
>> +++ b/drivers/pwm/pwm-dwc.c
>> @@ -1,16 +1,12 @@
>>   // SPDX-License-Identifier: GPL-2.0
>>   /*
>> - * DesignWare PWM Controller driver
>> + * DesignWare PWM Controller driver core
>>    *
>>    * Copyright (C) 2018-2020 Intel Corporation
>>    *
>>    * Author: Felipe Balbi (Intel)
>>    * Author: Jarkko Nikula <jarkko.nikula@linux.intel.com>
>>    * Author: Raymond Tan <raymond.tan@intel.com>
>> - *
>> - * Limitations:
>> - * - The hardware cannot generate a 0 % or 100 % duty cycle. Both 
>> high and low
>> - *   periods are one or more input clock periods long.
>>    */
> 
> Relates to previous comment, is there reason why this limitation is 
> removed from the core part?

See above.

> 
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-dwc.h
>> +#define DWC_CLK_PERIOD_NS    10
> 
> Perhaps this addition can be removed if patch 7/10 goes before this 
> patch? It's anyway specific to PCI part only.

Will look into that

[RFC v4 07/10] pwm: dwc: make timer clock configurable

[Ben Dooks]

Add a configurable clock base rate for the pwm as when being built
for non-PCI the block may be sourced from an internal clock.

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
v4:
 - moved earlier before the of changes to make the of changes one patch
v2:
  - removed the ifdef and merged the other clock patch in here
---
 drivers/pwm/pwm-dwc-pci.c |  1 +
 drivers/pwm/pwm-dwc.c     | 10 ++++++----
 drivers/pwm/pwm-dwc.h     |  2 ++
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-dwc-pci.c b/drivers/pwm/pwm-dwc-pci.c
index 2213d0e7f3c8..949423e368f9 100644
--- a/drivers/pwm/pwm-dwc-pci.c
+++ b/drivers/pwm/pwm-dwc-pci.c
@@ -20,6 +20,7 @@
 #include <linux/pci.h>
 #include <linux/pm_runtime.h>
 #include <linux/pwm.h>
+#include <linux/clk.h>
 
 #include "pwm-dwc.h"
 
diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index 90a8ae1252a1..1251620ab771 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -14,6 +14,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/clk.h>
 #include <linux/pm_runtime.h>
 #include <linux/pwm.h>
 
@@ -47,13 +48,13 @@ static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc,
 	 * 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_PERIOD_NS);
+	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_PERIOD_NS);
+				    dwc->clk_ns);
 	if (tmp < 1 || tmp > (1ULL << 32))
 		return -ERANGE;
 	high = tmp - 1;
@@ -128,12 +129,12 @@ static void dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	duty = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
 	duty += 1;
-	duty *= DWC_CLK_PERIOD_NS;
+	duty *= dwc->clk_ns;
 	state->duty_cycle = duty;
 
 	period = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
 	period += 1;
-	period *= DWC_CLK_PERIOD_NS;
+	period *= dwc->clk_ns;
 	period += duty;
 	state->period = period;
 
@@ -156,6 +157,7 @@ struct dwc_pwm *dwc_pwm_alloc(struct device *dev)
 	if (!dwc)
 		return NULL;
 
+	dwc->clk_ns = 10;
 	dwc->chip.dev = dev;
 	dwc->chip.ops = &dwc_pwm_ops;
 	dwc->chip.npwm = DWC_TIMERS_TOTAL;
diff --git a/drivers/pwm/pwm-dwc.h b/drivers/pwm/pwm-dwc.h
index 68f98eb76152..e5a1f7be7bc8 100644
--- a/drivers/pwm/pwm-dwc.h
+++ b/drivers/pwm/pwm-dwc.h
@@ -41,6 +41,8 @@ struct dwc_pwm_ctx {
 struct dwc_pwm {
 	struct pwm_chip chip;
 	void __iomem *base;
+	struct clk *clk;
+	unsigned int clk_ns;
 	struct dwc_pwm_ctx ctx[DWC_TIMERS_TOTAL];
 };
 #define to_dwc_pwm(p)	(container_of((p), struct dwc_pwm, chip))
-- 
2.35.1

Re: [RFC v4 07/10] pwm: dwc: make timer clock configurable

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3526 bytes --]

Hello Ben,

On Tue, Aug 16, 2022 at 10:14:51PM +0100, Ben Dooks wrote:
> Add a configurable clock base rate for the pwm as when being built
> for non-PCI the block may be sourced from an internal clock.
> 
> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
> ---
> v4:
>  - moved earlier before the of changes to make the of changes one patch
> v2:
>   - removed the ifdef and merged the other clock patch in here
> ---
>  drivers/pwm/pwm-dwc-pci.c |  1 +
>  drivers/pwm/pwm-dwc.c     | 10 ++++++----
>  drivers/pwm/pwm-dwc.h     |  2 ++
>  3 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-dwc-pci.c b/drivers/pwm/pwm-dwc-pci.c
> index 2213d0e7f3c8..949423e368f9 100644
> --- a/drivers/pwm/pwm-dwc-pci.c
> +++ b/drivers/pwm/pwm-dwc-pci.c
> @@ -20,6 +20,7 @@
>  #include <linux/pci.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pwm.h>
> +#include <linux/clk.h>
>  
>  #include "pwm-dwc.h"
>  

This hunk is strange. Why do you add a header but no user of a symbol
declared in that header? Maybe it should go to pwm-dwc.h?

> diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
> index 90a8ae1252a1..1251620ab771 100644
> --- a/drivers/pwm/pwm-dwc.c
> +++ b/drivers/pwm/pwm-dwc.c
> @@ -14,6 +14,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
> +#include <linux/clk.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pwm.h>
>  
> @@ -47,13 +48,13 @@ static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc,
>  	 * 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_PERIOD_NS);
> +	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_PERIOD_NS);
> +				    dwc->clk_ns);
>  	if (tmp < 1 || tmp > (1ULL << 32))
>  		return -ERANGE;
>  	high = tmp - 1;
> @@ -128,12 +129,12 @@ static void dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  	duty = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
>  	duty += 1;
> -	duty *= DWC_CLK_PERIOD_NS;
> +	duty *= dwc->clk_ns;
>  	state->duty_cycle = duty;
>  
>  	period = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
>  	period += 1;
> -	period *= DWC_CLK_PERIOD_NS;
> +	period *= dwc->clk_ns;
>  	period += duty;
>  	state->period = period;
>  
> @@ -156,6 +157,7 @@ struct dwc_pwm *dwc_pwm_alloc(struct device *dev)
>  	if (!dwc)
>  		return NULL;
>  
> +	dwc->clk_ns = 10;
>  	dwc->chip.dev = dev;
>  	dwc->chip.ops = &dwc_pwm_ops;
>  	dwc->chip.npwm = DWC_TIMERS_TOTAL;

After this conversion all usage of DWC_CLK_PERIOD_NS is done so the
#define can go away?

> diff --git a/drivers/pwm/pwm-dwc.h b/drivers/pwm/pwm-dwc.h
> index 68f98eb76152..e5a1f7be7bc8 100644
> --- a/drivers/pwm/pwm-dwc.h
> +++ b/drivers/pwm/pwm-dwc.h
> @@ -41,6 +41,8 @@ struct dwc_pwm_ctx {
>  struct dwc_pwm {
>  	struct pwm_chip chip;
>  	void __iomem *base;
> +	struct clk *clk;
> +	unsigned int clk_ns;
>  	struct dwc_pwm_ctx ctx[DWC_TIMERS_TOTAL];
>  };
>  #define to_dwc_pwm(p)	(container_of((p), struct dwc_pwm, chip))
> -- 
> 2.35.1
> 
> 

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC v4 08/10] pwm: dwc: add of/platform support

[Ben Dooks]

The dwc pwm controller can be used in non-PCI systems, so allow
either platform or OF based probing.

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
v4:
 - split the of code out of the core
 - moved the compile test code earlier
 - fixed review comments
  - used NS_PER_SEC
  - use devm_clk_get_enabled
v3:
 - changed compatible name
---
 drivers/pwm/Kconfig      |  9 +++++
 drivers/pwm/Makefile     |  1 +
 drivers/pwm/pwm-dwc-of.c | 78 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 88 insertions(+)
 create mode 100644 drivers/pwm/pwm-dwc-of.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index a9f1c554db2b..f1735653365f 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -192,6 +192,15 @@ config PWM_DWC_PCI
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-dwc-pci.
 
+config PWM_DWC_OF
+	tristate "DesignWare PWM Controller (OF bus)
+	depends on PWM_DWC && OF
+	help
+	  PWM driver for Synopsys DWC PWM Controller on an OF 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 a70d36623129..d1fd1641f077 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_PWM_CLPS711X)	+= pwm-clps711x.o
 obj-$(CONFIG_PWM_CRC)		+= pwm-crc.o
 obj-$(CONFIG_PWM_CROS_EC)	+= pwm-cros-ec.o
 obj-$(CONFIG_PWM_DWC)		+= pwm-dwc.o
+obj-$(CONFIG_PWM_DWC_OF)	+= pwm-dwc-of.o
 obj-$(CONFIG_PWM_DWC_PCI)	+= pwm-dwc-pci.o
 obj-$(CONFIG_PWM_EP93XX)	+= pwm-ep93xx.o
 obj-$(CONFIG_PWM_FSL_FTM)	+= pwm-fsl-ftm.o
diff --git a/drivers/pwm/pwm-dwc-of.c b/drivers/pwm/pwm-dwc-of.c
new file mode 100644
index 000000000000..d18fac287325
--- /dev/null
+++ b/drivers/pwm/pwm-dwc-of.c
@@ -0,0 +1,78 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * DesignWare PWM Controller driver OF
+ *
+ * Copyright (C) 2022 SiFive, Inc.
+ */
+
+#include <linux/bitops.h>
+#include <linux/export.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/clk.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/pwm.h>
+#include <linux/io.h>
+
+#include "pwm-dwc.h"
+
+static int dwc_pwm_plat_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct dwc_pwm *dwc;
+	int ret;
+
+	dwc = dwc_pwm_alloc(dev);
+	if (!dwc)
+		return -ENOMEM;
+
+	dwc->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(dwc->base))
+		return dev_err_probe(dev, PTR_ERR(dwc->base),
+				     "failed to map IO\n");
+
+	dwc->clk = devm_clk_get_enabled(dev, "timer");
+	if (IS_ERR(dwc->clk))
+		return dev_err_probe(dev, PTR_ERR(dwc->clk),
+				     "failed to get timer clock\n");
+
+	clk_prepare_enable(dwc->clk);
+	dwc->clk_ns = NSEC_PER_SEC / clk_get_rate(dwc->clk);
+
+	ret = devm_pwmchip_add(dev, &dwc->chip);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int dwc_pwm_plat_remove(struct platform_device *pdev)
+{
+	struct dwc_pwm *dwc = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(dwc->clk);
+	return 0;
+}
+
+static const struct of_device_id dwc_pwm_dt_ids[] = {
+	{ .compatible = "snps,dw-apb-timers-pwm2" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, dwc_pwm_dt_ids);
+
+static struct platform_driver dwc_pwm_plat_driver = {
+	.driver = {
+		.name		= "dwc-pwm",
+		.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@sifive.com>");
+MODULE_DESCRIPTION("DesignWare PWM Controller");
+MODULE_LICENSE("GPL");
-- 
2.35.1

Re: [RFC v4 08/10] pwm: dwc: add of/platform support

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 5398 bytes --]

On Tue, Aug 16, 2022 at 10:14:52PM +0100, Ben Dooks wrote:
> The dwc pwm controller can be used in non-PCI systems, so allow
> either platform or OF based probing.
> 
> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
> ---
> v4:
>  - split the of code out of the core
>  - moved the compile test code earlier
>  - fixed review comments
>   - used NS_PER_SEC
>   - use devm_clk_get_enabled
> v3:
>  - changed compatible name
> ---
>  drivers/pwm/Kconfig      |  9 +++++
>  drivers/pwm/Makefile     |  1 +
>  drivers/pwm/pwm-dwc-of.c | 78 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 88 insertions(+)
>  create mode 100644 drivers/pwm/pwm-dwc-of.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index a9f1c554db2b..f1735653365f 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -192,6 +192,15 @@ config PWM_DWC_PCI
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-dwc-pci.
>  
> +config PWM_DWC_OF
> +	tristate "DesignWare PWM Controller (OF bus)
> +	depends on PWM_DWC && OF
> +	help
> +	  PWM driver for Synopsys DWC PWM Controller on an OF 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 a70d36623129..d1fd1641f077 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_PWM_CLPS711X)	+= pwm-clps711x.o
>  obj-$(CONFIG_PWM_CRC)		+= pwm-crc.o
>  obj-$(CONFIG_PWM_CROS_EC)	+= pwm-cros-ec.o
>  obj-$(CONFIG_PWM_DWC)		+= pwm-dwc.o
> +obj-$(CONFIG_PWM_DWC_OF)	+= pwm-dwc-of.o
>  obj-$(CONFIG_PWM_DWC_PCI)	+= pwm-dwc-pci.o
>  obj-$(CONFIG_PWM_EP93XX)	+= pwm-ep93xx.o
>  obj-$(CONFIG_PWM_FSL_FTM)	+= pwm-fsl-ftm.o
> diff --git a/drivers/pwm/pwm-dwc-of.c b/drivers/pwm/pwm-dwc-of.c
> new file mode 100644
> index 000000000000..d18fac287325
> --- /dev/null
> +++ b/drivers/pwm/pwm-dwc-of.c
> @@ -0,0 +1,78 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DesignWare PWM Controller driver OF
> + *
> + * Copyright (C) 2022 SiFive, Inc.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/export.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/clk.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/pwm.h>
> +#include <linux/io.h>
> +
> +#include "pwm-dwc.h"
> +
> +static int dwc_pwm_plat_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct dwc_pwm *dwc;
> +	int ret;
> +
> +	dwc = dwc_pwm_alloc(dev);
> +	if (!dwc)
> +		return -ENOMEM;
> +
> +	dwc->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(dwc->base))
> +		return dev_err_probe(dev, PTR_ERR(dwc->base),
> +				     "failed to map IO\n");

devm_platform_ioremap_resource() already emits an error message.

> +
> +	dwc->clk = devm_clk_get_enabled(dev, "timer");
> +	if (IS_ERR(dwc->clk))
> +		return dev_err_probe(dev, PTR_ERR(dwc->clk),
> +				     "failed to get timer clock\n");
> +
> +	clk_prepare_enable(dwc->clk);

You don't need clk_prepare_enable() as you used devm_clk_get_enabled().

(Otherwise, when keeping clk_prepare_enable() you need to check the
return value.)

> +	dwc->clk_ns = NSEC_PER_SEC / clk_get_rate(dwc->clk);

I think I already pointed out this is non-optimal.

Later you use clk_ns in __dwc_pwm_configure_timer():

	tmp = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, dwc->clk_ns);

So what you really want here is:

	tmp = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * clk_get_rate(dwc->clk), NSEC_PER_SEC);

With for example clk_get_rate(dwc->clk) = 201171875 and duty_cycle =
4096 you now get clk_ns = 4 (while the exact value is 4.97087..), tmp =
1024, while the exact value is 824.

So the idea is to add a clkrate member to the private driver struct, let
it default to 100000000 for the pci case and use the line I suggested
above.

> +
> +	ret = devm_pwmchip_add(dev, &dwc->chip);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

This is equivalent to

	return ret;

> +}
> +
> +static int dwc_pwm_plat_remove(struct platform_device *pdev)
> +{
> +	struct dwc_pwm *dwc = platform_get_drvdata(pdev);
> +
> +	clk_disable_unprepare(dwc->clk);
> +	return 0;
> +}

When dropping clk_prepare_enable() the .remove callback can go away,
too.

> +
> +static const struct of_device_id dwc_pwm_dt_ids[] = {
> +	{ .compatible = "snps,dw-apb-timers-pwm2" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, dwc_pwm_dt_ids);
> +
> +static struct platform_driver dwc_pwm_plat_driver = {
> +	.driver = {
> +		.name		= "dwc-pwm",
> +		.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@sifive.com>");
> +MODULE_DESCRIPTION("DesignWare PWM Controller");
> +MODULE_LICENSE("GPL");

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC v4 08/10] pwm: dwc: add of/platform support

[Ben Dooks]

On 14/09/2022 17:47, Uwe Kleine-König wrote:
> On Tue, Aug 16, 2022 at 10:14:52PM +0100, Ben Dooks wrote:
>> The dwc pwm controller can be used in non-PCI systems, so allow
>> either platform or OF based probing.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
>> ---
>> v4:
>>   - split the of code out of the core
>>   - moved the compile test code earlier
>>   - fixed review comments
>>    - used NS_PER_SEC
>>    - use devm_clk_get_enabled
>> v3:
>>   - changed compatible name
>> ---
>>   drivers/pwm/Kconfig      |  9 +++++
>>   drivers/pwm/Makefile     |  1 +
>>   drivers/pwm/pwm-dwc-of.c | 78 ++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 88 insertions(+)
>>   create mode 100644 drivers/pwm/pwm-dwc-of.c
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index a9f1c554db2b..f1735653365f 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -192,6 +192,15 @@ config PWM_DWC_PCI
>>   	  To compile this driver as a module, choose M here: the module
>>   	  will be called pwm-dwc-pci.
>>   
>> +config PWM_DWC_OF
>> +	tristate "DesignWare PWM Controller (OF bus)
>> +	depends on PWM_DWC && OF
>> +	help
>> +	  PWM driver for Synopsys DWC PWM Controller on an OF 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 a70d36623129..d1fd1641f077 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -15,6 +15,7 @@ obj-$(CONFIG_PWM_CLPS711X)	+= pwm-clps711x.o
>>   obj-$(CONFIG_PWM_CRC)		+= pwm-crc.o
>>   obj-$(CONFIG_PWM_CROS_EC)	+= pwm-cros-ec.o
>>   obj-$(CONFIG_PWM_DWC)		+= pwm-dwc.o
>> +obj-$(CONFIG_PWM_DWC_OF)	+= pwm-dwc-of.o
>>   obj-$(CONFIG_PWM_DWC_PCI)	+= pwm-dwc-pci.o
>>   obj-$(CONFIG_PWM_EP93XX)	+= pwm-ep93xx.o
>>   obj-$(CONFIG_PWM_FSL_FTM)	+= pwm-fsl-ftm.o
>> diff --git a/drivers/pwm/pwm-dwc-of.c b/drivers/pwm/pwm-dwc-of.c
>> new file mode 100644
>> index 000000000000..d18fac287325
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-dwc-of.c
>> @@ -0,0 +1,78 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * DesignWare PWM Controller driver OF
>> + *
>> + * Copyright (C) 2022 SiFive, Inc.
>> + */
>> +
>> +#include <linux/bitops.h>
>> +#include <linux/export.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/clk.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/pwm.h>
>> +#include <linux/io.h>
>> +
>> +#include "pwm-dwc.h"
>> +
>> +static int dwc_pwm_plat_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct dwc_pwm *dwc;
>> +	int ret;
>> +
>> +	dwc = dwc_pwm_alloc(dev);
>> +	if (!dwc)
>> +		return -ENOMEM;
>> +
>> +	dwc->base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(dwc->base))
>> +		return dev_err_probe(dev, PTR_ERR(dwc->base),
>> +				     "failed to map IO\n");
> 
> devm_platform_ioremap_resource() already emits an error message.

ok, fixed


>> +
>> +	dwc->clk = devm_clk_get_enabled(dev, "timer");
>> +	if (IS_ERR(dwc->clk))
>> +		return dev_err_probe(dev, PTR_ERR(dwc->clk),
>> +				     "failed to get timer clock\n");
>> +
>> +	clk_prepare_enable(dwc->clk);
> 
> You don't need clk_prepare_enable() as you used devm_clk_get_enabled().
> 
> (Otherwise, when keeping clk_prepare_enable() you need to check the
> return value.)

ok, fixed. I didn't notice that when changing to devm_

> 
>> +	dwc->clk_ns = NSEC_PER_SEC / clk_get_rate(dwc->clk);
> 
> I think I already pointed out this is non-optimal.
> 
> Later you use clk_ns in __dwc_pwm_configure_timer():
> 
> 	tmp = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, dwc->clk_ns);
> 
> So what you really want here is:
> 
> 	tmp = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * clk_get_rate(dwc->clk), NSEC_PER_SEC);
> 
> With for example clk_get_rate(dwc->clk) = 201171875 and duty_cycle =
> 4096 you now get clk_ns = 4 (while the exact value is 4.97087..), tmp =
> 1024, while the exact value is 824.
> 
> So the idea is to add a clkrate member to the private driver struct, let
> it default to 100000000 for the pci case and use the line I suggested
> above.

ok, will consider keeping the rate in hz and modifiying the pci
version to use 10 * NSEC_PER_SEC for the rate.

I've been trying to avoid changing the pci case, but I think for
anything with a clk pointer we should be doing clk_get_rate in the
calc code

> 
>> +
>> +	ret = devm_pwmchip_add(dev, &dwc->chip);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
> 
> This is equivalent to
> 
> 	return ret;

Will sort that out.


>> +}
>> +
>> +static int dwc_pwm_plat_remove(struct platform_device *pdev)
>> +{
>> +	struct dwc_pwm *dwc = platform_get_drvdata(pdev);
>> +
>> +	clk_disable_unprepare(dwc->clk);
>> +	return 0;
>> +}
> 
> When dropping clk_prepare_enable() the .remove callback can go away,
> too.

thanks, spotted that one a while ago.

>> +
>> +static const struct of_device_id dwc_pwm_dt_ids[] = {
>> +	{ .compatible = "snps,dw-apb-timers-pwm2" },
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(of, dwc_pwm_dt_ids);
>> +
>> +static struct platform_driver dwc_pwm_plat_driver = {
>> +	.driver = {
>> +		.name		= "dwc-pwm",
>> +		.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@sifive.com>");
>> +MODULE_DESCRIPTION("DesignWare PWM Controller");
>> +MODULE_LICENSE("GPL");
> 
> Best regards
> Uwe
> 

Re: [RFC v4 08/10] pwm: dwc: add of/platform support

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1371 bytes --]

Hello,

On Tue, Aug 16, 2022 at 10:14:52PM +0100, Ben Dooks wrote:
> The dwc pwm controller can be used in non-PCI systems, so allow
> either platform or OF based probing.
> 
> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
> ---
> v4:
>  - split the of code out of the core
>  - moved the compile test code earlier
>  - fixed review comments
>   - used NS_PER_SEC
>   - use devm_clk_get_enabled
> v3:
>  - changed compatible name
> ---
>  drivers/pwm/Kconfig      |  9 +++++
>  drivers/pwm/Makefile     |  1 +
>  drivers/pwm/pwm-dwc-of.c | 78 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 88 insertions(+)
>  create mode 100644 drivers/pwm/pwm-dwc-of.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index a9f1c554db2b..f1735653365f 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -192,6 +192,15 @@ config PWM_DWC_PCI
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-dwc-pci.
>  
> +config PWM_DWC_OF
> +	tristate "DesignWare PWM Controller (OF bus)

There is a missing " which results in:

	drivers/pwm/Kconfig:196:warning: multi-line strings not supported

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC v4 08/10] pwm: dwc: add of/platform support

[Ben Dooks]

On 15/09/2022 08:24, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, Aug 16, 2022 at 10:14:52PM +0100, Ben Dooks wrote:
>> The dwc pwm controller can be used in non-PCI systems, so allow
>> either platform or OF based probing.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
>> ---
>> v4:
>>   - split the of code out of the core
>>   - moved the compile test code earlier
>>   - fixed review comments
>>    - used NS_PER_SEC
>>    - use devm_clk_get_enabled
>> v3:
>>   - changed compatible name
>> ---
>>   drivers/pwm/Kconfig      |  9 +++++
>>   drivers/pwm/Makefile     |  1 +
>>   drivers/pwm/pwm-dwc-of.c | 78 ++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 88 insertions(+)
>>   create mode 100644 drivers/pwm/pwm-dwc-of.c
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index a9f1c554db2b..f1735653365f 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -192,6 +192,15 @@ config PWM_DWC_PCI
>>   	  To compile this driver as a module, choose M here: the module
>>   	  will be called pwm-dwc-pci.
>>   
>> +config PWM_DWC_OF
>> +	tristate "DesignWare PWM Controller (OF bus)
> 
> There is a missing " which results in:
> 
> 	drivers/pwm/Kconfig:196:warning: multi-line strings not supported
> 
> Best regards
> Uwe

Thanks, fixed.

[RFC v4 09/10] pwm: dwc: add snps,pwm-number to limit pwm count

[Ben Dooks]

Add snps,pwm-number property to indicate if the block does not have
all 8 of the PWM blocks.

Not sure if this should be a general PWM property consider optional
for all PWM types, so have added a specific one here (there is only
one other controller with a property for PWM count at the moment)

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
 drivers/pwm/pwm-dwc-of.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/pwm/pwm-dwc-of.c b/drivers/pwm/pwm-dwc-of.c
index d18fac287325..65c7e6621bba 100644
--- a/drivers/pwm/pwm-dwc-of.c
+++ b/drivers/pwm/pwm-dwc-of.c
@@ -21,12 +21,20 @@ static int dwc_pwm_plat_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct dwc_pwm *dwc;
+	u32 nr_pwm;
 	int ret;
 
 	dwc = dwc_pwm_alloc(dev);
 	if (!dwc)
 		return -ENOMEM;
 
+	if (!device_property_read_u32(dev, "snps,pwm-number", &nr_pwm)) {
+		if (nr_pwm > DWC_TIMERS_TOTAL)
+			dev_err(dev, "too many PWMs specified (%d)\n", nr_pwm);
+		else
+			dwc->chip.npwm = nr_pwm;
+	}
+
 	dwc->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(dwc->base))
 		return dev_err_probe(dev, PTR_ERR(dwc->base),
-- 
2.35.1

Re: [RFC v4 09/10] pwm: dwc: add snps,pwm-number to limit pwm count

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1738 bytes --]

On Tue, Aug 16, 2022 at 10:14:53PM +0100, Ben Dooks wrote:
> Add snps,pwm-number property to indicate if the block does not have
> all 8 of the PWM blocks.
> 
> Not sure if this should be a general PWM property consider optional
> for all PWM types, so have added a specific one here (there is only
> one other controller with a property for PWM count at the moment)
> 
> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
> ---
>  drivers/pwm/pwm-dwc-of.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-dwc-of.c b/drivers/pwm/pwm-dwc-of.c
> index d18fac287325..65c7e6621bba 100644
> --- a/drivers/pwm/pwm-dwc-of.c
> +++ b/drivers/pwm/pwm-dwc-of.c
> @@ -21,12 +21,20 @@ static int dwc_pwm_plat_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct dwc_pwm *dwc;
> +	u32 nr_pwm;
>  	int ret;
>  
>  	dwc = dwc_pwm_alloc(dev);
>  	if (!dwc)
>  		return -ENOMEM;
>  
> +	if (!device_property_read_u32(dev, "snps,pwm-number", &nr_pwm)) {
> +		if (nr_pwm > DWC_TIMERS_TOTAL)
> +			dev_err(dev, "too many PWMs specified (%d)\n", nr_pwm);

Maybe

	dev_err(dev, "too many PWMs specified (%d), falling back to " #DWC_TIMERS_TOTAL "\n", nr_pwm);

to make it obvious the error doesn't prevent probing the device.

Or you believe the dtb and use whatever it specifies.

> +		else
> +			dwc->chip.npwm = nr_pwm;
> +	}
> +
>  	dwc->base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(dwc->base))
>  		return dev_err_probe(dev, PTR_ERR(dwc->base),

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC v4 10/10] pwm: dwc: add PWM bit unset in get_state call

[Ben Dooks]

If we are not in PWM mode, then the output is technically a 50%
output based on a single timer instead of the high-low based on
the two counters. Add a check for the PWM mode in dwc_pwm_get_state()
and if DWC_TIM_CTRL_PWM is not set, then return a 50% cycle.

This may only be an issue on initialisation, as the rest of the
code currently assumes we're always going to have the extended
PWM mode using two counters.

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
v4:
 - fixed review comment on mulit-line calculations
---
 drivers/pwm/pwm-dwc.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index 1251620ab771..5ef0fe7ea3e9 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -121,23 +121,30 @@ static void dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 {
 	struct dwc_pwm *dwc = to_dwc_pwm(chip);
 	u64 duty, period;
+	u32 ctrl, ld, ld2;
 
 	pm_runtime_get_sync(chip->dev);
 
-	state->enabled = !!(dwc_pwm_readl(dwc,
-				DWC_TIM_CTRL(pwm->hwpwm)) & DWC_TIM_CTRL_EN);
+	ctrl = dwc_pwm_readl(dwc, DWC_TIM_CTRL(pwm->hwpwm));
+	ld = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
+	ld2 = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
 
-	duty = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
-	duty += 1;
-	duty *= dwc->clk_ns;
-	state->duty_cycle = duty;
+	state->enabled = !!(ctrl & DWC_TIM_CTRL_EN);
 
-	period = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
-	period += 1;
-	period *= dwc->clk_ns;
-	period += duty;
-	state->period = period;
+	/* 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;
+	} else {
+		duty = (ld + 1) * dwc->clk_ns;
+		period = duty * 2;
+	}
 
+	state->period = period;
+	state->duty_cycle = duty;
 	state->polarity = PWM_POLARITY_INVERSED;
 
 	pm_runtime_put_sync(chip->dev);
-- 
2.35.1

Re: [RFC v4 00/10] RFC on synpsys pwm driver changes

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 400 bytes --]

Hello Ben,

there was some feedback to individual patches that require a rework. I'm
marking the whole series as "changes requested" in the expectation that
you will resend them all once you addressed the feedback.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC v4 00/10] RFC on synpsys pwm driver changes

[Ben Dooks]

On 27/09/2022 17:47, Uwe Kleine-König wrote:
> Hello Ben,
> 
> there was some feedback to individual patches that require a rework. I'm
> marking the whole series as "changes requested" in the expectation that
> you will resend them all once you addressed the feedback.

I have most of the feedback done but we've not had a chance to test them
fully. I can send a new series later today but the changes for the clock
division calculations have yet to be tested.

-- 
Ben