[PATCH v7 00/10] Designware PWM driver updates for OF

[PATCH v7 00/10] Designware PWM driver updates for OF

[Ben Dooks]

An updated set of patches for the Designware PWM driver
split into PCI and OF versions. I think I got all the
review issues in this set.

Sorry for the delay in getting this out, between conferences
and other absences there has been little time to deal with
this set. I will be now out of office until 3rd Jan 2023.

v7:
 - fixup kconfig from previous pcie changes
 - re-order kconfig to make dwc core be selected by PCI driver
 - move clk variable to patch it is used in
v6:
 - fix removal ordering of DWC_PERIOD_NS
v5:
 - fixed kconfig string error
 - merged pwm-nr into main of code
 - split of code from pci code
 - updated pwm-nr capping
 - fix duplicate error reporting in of-code
 - fix return in of-probe
 - remove unecessary remove function as devm_ functions sort this
 - fixed ordering of properties
 - added missing reg item
 - fixed missing split of the two clock sources.
 - get bus clock in of code
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 PWM bit unset in get_state call
  pwm: dwc: use clock rate in hz to avoid rounding issues

 .../bindings/pwm/snps,dw-apb-timers-pwm2.yaml |  68 ++++++
 drivers/pwm/Kconfig                           |  26 ++-
 drivers/pwm/Makefile                          |   2 +
 drivers/pwm/pwm-dwc-of.c                      |  76 +++++++
 drivers/pwm/pwm-dwc-pci.c                     | 134 +++++++++++
 drivers/pwm/pwm-dwc.c                         | 210 ++++--------------
 drivers/pwm/pwm-dwc.h                         |  59 +++++
 7 files changed, 404 insertions(+), 171 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

[PATCH v7 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>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
v5:
 - fixed order of properties
 - corrected clock to two items
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 | 68 +++++++++++++++++++
 1 file changed, 68 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..9aabdb373afa
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml
@@ -0,0 +1,68 @@
+# 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
+
+  reg:
+    maxItems: 1
+
+  "#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]
+
+required:
+  - compatible
+  - reg
+  - "#pwm-cells"
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    pwm: pwm@180000 {
+      compatible = "snps,dw-apb-timers-pwm2";
+      reg = <0x180000 0x200>;
+      #pwm-cells = <3>;
+      clocks = <&bus>, <&timer>;
+      clock-names = "bus", "timer";
+    };
-- 
2.35.1

[PATCH v7 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>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
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

[PATCH v7 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

[PATCH v7 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

Re: [PATCH v7 04/10] pwm: dwc: move memory alloc to own function

[Uwe Kleine-König]

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

On Fri, Dec 23, 2022 at 03:38:14PM +0000, Ben Dooks wrote:
> 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);

I don't particularily like that this dev_set_drvdata is matched by
pci_get_drvdata in .remove(), but this isn't going to break I guess. So:

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

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 --]

[PATCH v7 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>
Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 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

[PATCH v7 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>
---
v7:
 - re-order kconfig to make dwc core be selected by PCI driver
v6:
 - put DWC_PERIOD_NS back to avoid bisect issues
v4:
 - removed DWC_PERIOD_NS as not needed
---
 drivers/pwm/Kconfig       |  17 +++-
 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, 209 insertions(+), 158 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..8c5ef388a981 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -174,16 +174,25 @@ config PWM_CROS_EC
 	  PWM driver for exposing a PWM attached to the ChromeOS Embedded
 	  Controller.
 
-config PWM_DWC
-	tristate "DesignWare PWM Controller"
-	depends on PCI || COMPILE_TEST
+config PWM_DWC_CORE
+	tristate
 	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
+	tristate "DesignWare PWM Controller (PCI bus)"
+	depends on HAS_IOMEM && PCI
+	select PWM_DWC_CORE
+	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: [PATCH v7 06/10] pwm: dwc: split pci out of core driver

[Uwe Kleine-König]

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

Hello,

On Fri, Dec 23, 2022 at 03:38:16PM +0000, 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>
> ---
> v7:
>  - re-order kconfig to make dwc core be selected by PCI driver
> v6:
>  - put DWC_PERIOD_NS back to avoid bisect issues
> v4:
>  - removed DWC_PERIOD_NS as not needed
> ---
>  drivers/pwm/Kconfig       |  17 +++-
>  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, 209 insertions(+), 158 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..8c5ef388a981 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -174,16 +174,25 @@ config PWM_CROS_EC
>  	  PWM driver for exposing a PWM attached to the ChromeOS Embedded
>  	  Controller.
>  
> -config PWM_DWC
> -	tristate "DesignWare PWM Controller"
> -	depends on PCI || COMPILE_TEST

You're loosing COMPILE_TEST here, as it's not present for the new
PWM_DWC.

> +config PWM_DWC_CORE
> +	tristate
>  	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
> +	tristate "DesignWare PWM Controller (PCI bus)"
> +	depends on HAS_IOMEM && PCI
> +	select PWM_DWC_CORE
> +	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);

If you respin this patch for the COMPILE_TEST issue I pointed out above,
would you mind using a module namespace?

That would work by adding e.g.

	#define DEFAULT_SYMBOL_NAMESPACE dwc-pwm

to drivers/pwm/pwm-dwc.c (before the includes) and

	MODULE_IMPORT_NS(dwc-pwm)

to drivers/pwm/pwm-dwc.h.

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: [PATCH v7 06/10] pwm: dwc: split pci out of core driver

[Ben Dooks]

On 16/02/2023 21:30, Uwe Kleine-König wrote:
> Hello,
> 
> On Fri, Dec 23, 2022 at 03:38:16PM +0000, 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>
>> ---
>> v7:
>>   - re-order kconfig to make dwc core be selected by PCI driver
>> v6:
>>   - put DWC_PERIOD_NS back to avoid bisect issues
>> v4:
>>   - removed DWC_PERIOD_NS as not needed
>> ---
>>   drivers/pwm/Kconfig       |  17 +++-
>>   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, 209 insertions(+), 158 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..8c5ef388a981 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -174,16 +174,25 @@ config PWM_CROS_EC
>>   	  PWM driver for exposing a PWM attached to the ChromeOS Embedded
>>   	  Controller.
>>   
>> -config PWM_DWC
>> -	tristate "DesignWare PWM Controller"
>> -	depends on PCI || COMPILE_TEST
> 
> You're loosing COMPILE_TEST here, as it's not present for the new
> PWM_DWC.
> 
>> +config PWM_DWC_CORE
>> +	tristate
>>   	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
>> +	tristate "DesignWare PWM Controller (PCI bus)"
>> +	depends on HAS_IOMEM && PCI
>> +	select PWM_DWC_CORE
>> +	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);
> 
> If you respin this patch for the COMPILE_TEST issue I pointed out above,
> would you mind using a module namespace?
> 
> That would work by adding e.g.
> 
> 	#define DEFAULT_SYMBOL_NAMESPACE dwc-pwm
> 
> to drivers/pwm/pwm-dwc.c (before the includes) and
> 
> 	MODULE_IMPORT_NS(dwc-pwm)
> 
> to drivers/pwm/pwm-dwc.h.
> 
> Best regards
> Uwe
> 

I tried adding these, but all I get is a bunch of compile errors.

Re: [PATCH v7 06/10] pwm: dwc: split pci out of core driver

[Uwe Kleine-König]

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

On Tue, Jun 13, 2023 at 08:22:34PM +0100, Ben Dooks wrote:
> On 16/02/2023 21:30, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Fri, Dec 23, 2022 at 03:38:16PM +0000, 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>
> > > ---
> > > v7:
> > >   - re-order kconfig to make dwc core be selected by PCI driver
> > > v6:
> > >   - put DWC_PERIOD_NS back to avoid bisect issues
> > > v4:
> > >   - removed DWC_PERIOD_NS as not needed
> > > ---
> > >   drivers/pwm/Kconfig       |  17 +++-
> > >   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, 209 insertions(+), 158 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..8c5ef388a981 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -174,16 +174,25 @@ config PWM_CROS_EC
> > >   	  PWM driver for exposing a PWM attached to the ChromeOS Embedded
> > >   	  Controller.
> > > -config PWM_DWC
> > > -	tristate "DesignWare PWM Controller"
> > > -	depends on PCI || COMPILE_TEST
> > 
> > You're loosing COMPILE_TEST here, as it's not present for the new
> > PWM_DWC.
> > 
> > > +config PWM_DWC_CORE
> > > +	tristate
> > >   	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
> > > +	tristate "DesignWare PWM Controller (PCI bus)"
> > > +	depends on HAS_IOMEM && PCI
> > > +	select PWM_DWC_CORE
> > > +	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);
> > 
> > If you respin this patch for the COMPILE_TEST issue I pointed out above,
> > would you mind using a module namespace?
> > 
> > That would work by adding e.g.
> > 
> > 	#define DEFAULT_SYMBOL_NAMESPACE dwc-pwm
> > 
> > to drivers/pwm/pwm-dwc.c (before the includes) and
> > 
> > 	MODULE_IMPORT_NS(dwc-pwm)
> > 
> > to drivers/pwm/pwm-dwc.h.
> > 
> > Best regards
> > Uwe
> > 
> 
> I tried adding these, but all I get is a bunch of compile errors.

I guess: Add a ; after MODULE_IMPORT_NS(dwc-pwm) fixes that?

To talk code:
https://git.pengutronix.de/cgit/ukl/linux/commit/?h=pwm-dwc-for-bjdooks&id=f3ebe7f1b46f57afee44d42cb8ab3835e8f662c9
works for me.

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 --]

[PATCH v7 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>
---
v7:
 - remove the "struct clk *" clk field from dwc_pwm_ctx, not used here,
v6:
 - removed DWC_CLK_PERIOD_NS as it is now not needed
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.c | 9 +++++----
 drivers/pwm/pwm-dwc.h | 2 +-
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index 90a8ae1252a1..0c6beafa8c41 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -47,13 +47,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 +128,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 +156,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..b29d8cd21208 100644
--- a/drivers/pwm/pwm-dwc.h
+++ b/drivers/pwm/pwm-dwc.h
@@ -22,7 +22,6 @@
 #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)
@@ -41,6 +40,7 @@ struct dwc_pwm_ctx {
 struct dwc_pwm {
 	struct pwm_chip chip;
 	void __iomem *base;
+	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: [PATCH v7 07/10] pwm: dwc: make timer clock configurable

[Uwe Kleine-König]

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

On Fri, Dec 23, 2022 at 03:38:17PM +0000, 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>

Reviewed-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 --]

[PATCH v7 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>
---
v7:
 - fixup kconfig from previous pcie changes
v5:
 - fix missing " in kconfig
 - remove .remove method, devm already sorts this.
 - merge pwm-number code
 - split the of code out of the core
 - get bus clock
v4:
 - moved the compile test code earlier
 - fixed review comments
  - used NS_PER_SEC
  - use devm_clk_get_enabled
  - ensure we get the bus clock
v3:
 - changed compatible name

fixup add pwm/Kconfig

fixup: kconfig change for of addition
---
 drivers/pwm/Kconfig       | 10 ++++++
 drivers/pwm/Makefile      |  1 +
 drivers/pwm/pwm-dwc-of.c  | 76 +++++++++++++++++++++++++++++++++++++++
 drivers/pwm/pwm-dwc-pci.c |  1 +
 drivers/pwm/pwm-dwc.c     |  1 +
 drivers/pwm/pwm-dwc.h     |  1 +
 6 files changed, 90 insertions(+)
 create mode 100644 drivers/pwm/pwm-dwc-of.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 8c5ef388a981..74ab526e8b8c 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -193,6 +193,16 @@ config PWM_DWC
 	  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 HAS_IOMEM && OF
+	select PWM_DWC_CORE
+	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..c5b4351cc7b0
--- /dev/null
+++ b/drivers/pwm/pwm-dwc-of.c
@@ -0,0 +1,76 @@
+// 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;
+	struct clk *bus;
+	u32 nr_pwm;
+
+	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 (%d) specified, capping at %d\n",
+				nr_pwm, dwc->chip.npwm);
+		else
+			dwc->chip.npwm = nr_pwm;
+	}
+
+	dwc->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(dwc->base))
+		return PTR_ERR(dwc->base);
+
+	bus = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(bus))
+		return dev_err_probe(dev, PTR_ERR(bus),
+				     "failed to get clock\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");
+
+	dwc->clk_ns = NSEC_PER_SEC / clk_get_rate(dwc->clk);
+	return devm_pwmchip_add(dev, &dwc->chip);
+}
+
+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,
+};
+
+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");
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 0c6beafa8c41..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>
 
diff --git a/drivers/pwm/pwm-dwc.h b/drivers/pwm/pwm-dwc.h
index b29d8cd21208..dc451cb2eff5 100644
--- a/drivers/pwm/pwm-dwc.h
+++ b/drivers/pwm/pwm-dwc.h
@@ -40,6 +40,7 @@ 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];
 };
-- 
2.35.1

[PATCH v7 09/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

[PATCH v7 10/10] pwm: dwc: use clock rate in hz to avoid rounding issues

[Ben Dooks]

As noted, the clock-rate when not a nice multiple of ns is probably
going to end up with inacurate caculations, as well as on a non pci
system the rate may change (although we've not put a clock rate
change notifier in this code yet) so we also add some quick checks
of the rate when we do any calculations with it.

Signed-off-by; Ben Dooks <ben.dooks@sifive.com>
Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-dwc-of.c |  2 +-
 drivers/pwm/pwm-dwc.c    | 29 ++++++++++++++++++++---------
 drivers/pwm/pwm-dwc.h    |  2 +-
 3 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/pwm/pwm-dwc-of.c b/drivers/pwm/pwm-dwc-of.c
index c5b4351cc7b0..5f7f066859d4 100644
--- a/drivers/pwm/pwm-dwc-of.c
+++ b/drivers/pwm/pwm-dwc-of.c
@@ -50,7 +50,7 @@ static int dwc_pwm_plat_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, PTR_ERR(dwc->clk),
 				     "failed to get timer clock\n");
 
-	dwc->clk_ns = NSEC_PER_SEC / clk_get_rate(dwc->clk);
+	dwc->clk_rate = clk_get_rate(dwc->clk);
 	return devm_pwmchip_add(dev, &dwc->chip);
 }
 
diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index 5ef0fe7ea3e9..f48a6245a3b5 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -43,18 +43,22 @@ static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc,
 	u32 high;
 	u32 low;
 
+	if (dwc->clk)
+		dwc->clk_rate = clk_get_rate(dwc->clk);
+
 	/*
 	 * Calculate width of low and high period in terms of input clock
 	 * periods and check are the result within HW limits between 1 and
 	 * 2^32 periods.
 	 */
-	tmp = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, dwc->clk_ns);
+	tmp = state->duty_cycle * dwc->clk_rate;
+	tmp = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
 	if (tmp < 1 || tmp > (1ULL << 32))
 		return -ERANGE;
 	low = tmp - 1;
 
-	tmp = DIV_ROUND_CLOSEST_ULL(state->period - state->duty_cycle,
-				    dwc->clk_ns);
+	tmp = (state->period - state->duty_cycle) * dwc->clk_rate;
+	tmp = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
 	if (tmp < 1 || tmp > (1ULL << 32))
 		return -ERANGE;
 	high = tmp - 1;
@@ -120,6 +124,7 @@ static void dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 			      struct pwm_state *state)
 {
 	struct dwc_pwm *dwc = to_dwc_pwm(chip);
+	unsigned long clk_rate;
 	u64 duty, period;
 	u32 ctrl, ld, ld2;
 
@@ -129,22 +134,28 @@ static void dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 	ld = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
 	ld2 = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
 
+	if (dwc->clk)
+		dwc->clk_rate = clk_get_rate(dwc->clk);
+
+	clk_rate = dwc->clk_rate;
 	state->enabled = !!(ctrl & DWC_TIM_CTRL_EN);
 
 	/* 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;
+		duty = ld + 1;
+		period = ld2 + 1;
 		period += duty;
 	} else {
-		duty = (ld + 1) * dwc->clk_ns;
+		duty = ld + 1;
 		period = duty * 2;
 	}
 
-	state->period = period;
-	state->duty_cycle = duty;
+	duty *= NSEC_PER_SEC;
+	period *= NSEC_PER_SEC;
+	state->period = DIV_ROUND_CLOSEST_ULL(period, clk_rate);
+	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(duty, clk_rate);
 	state->polarity = PWM_POLARITY_INVERSED;
 
 	pm_runtime_put_sync(chip->dev);
@@ -164,7 +175,7 @@ struct dwc_pwm *dwc_pwm_alloc(struct device *dev)
 	if (!dwc)
 		return NULL;
 
-	dwc->clk_ns = 10;
+	dwc->clk_rate = NSEC_PER_SEC / 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 dc451cb2eff5..19bdc2224690 100644
--- a/drivers/pwm/pwm-dwc.h
+++ b/drivers/pwm/pwm-dwc.h
@@ -41,7 +41,7 @@ struct dwc_pwm {
 	struct pwm_chip chip;
 	void __iomem *base;
 	struct clk *clk;
-	unsigned int clk_ns;
+	unsigned long clk_rate;
 	struct dwc_pwm_ctx ctx[DWC_TIMERS_TOTAL];
 };
 #define to_dwc_pwm(p)	(container_of((p), struct dwc_pwm, chip))
-- 
2.35.1

Re: [PATCH v7 10/10] pwm: dwc: use clock rate in hz to avoid rounding issues

[Uwe Kleine-König]

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

On Fri, Dec 23, 2022 at 03:38:20PM +0000, Ben Dooks wrote:
> As noted, the clock-rate when not a nice multiple of ns is probably
> going to end up with inacurate caculations, as well as on a non pci

Given that such a non-nice ca*l*culation only happens in the of case
that is introduced here, it would be nice to move this patch before the
introduction of the of-support.

> system the rate may change (although we've not put a clock rate
> change notifier in this code yet) so we also add some quick checks
> of the rate when we do any calculations with it.

If the clk rate changes while the PWM is on, this modifies the output.
This is unfortunate and so it justifies adding a call to
clk_rate_exclusive_get() when the PWM is on.
 
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: [PATCH v7 10/10] pwm: dwc: use clock rate in hz to avoid rounding issues

[Ben Dooks]

On 16/02/2023 21:39, Uwe Kleine-König wrote:
> On Fri, Dec 23, 2022 at 03:38:20PM +0000, Ben Dooks wrote:
>> As noted, the clock-rate when not a nice multiple of ns is probably
>> going to end up with inacurate caculations, as well as on a non pci
> 
> Given that such a non-nice ca*l*culation only happens in the of case
> that is introduced here, it would be nice to move this patch before the
> introduction of the of-support.

I've moved of support to the end of the series
you
>> system the rate may change (although we've not put a clock rate
>> change notifier in this code yet) so we also add some quick checks
>> of the rate when we do any calculations with it.
> 
> If the clk rate changes while the PWM is on, this modifies the output.
> This is unfortunate and so it justifies adding a call to
> clk_rate_exclusive_get() when the PWM is on.

I can't really test things any more as the hardware has been returned
to the client and I'm technically off the project (and awaiting this
email address to be closed down).

Could this either be solved by the clk_rate_exclusive_get() or adding
a clock change notifier? Either way I would prefer this to be work for
another patch.

I'll send v8 out later as it has had some re-works due to moving
things around.

Thank you for the review.

--
Ben

Re: [PATCH v7 00/10] Designware PWM driver updates for OF

[Ben Dooks]

On 23/12/2022 15:38, Ben Dooks wrote:
> An updated set of patches for the Designware PWM driver
> split into PCI and OF versions. I think I got all the
> review issues in this set.
> 
> Sorry for the delay in getting this out, between conferences
> and other absences there has been little time to deal with
> this set. I will be now out of office until 3rd Jan 2023.

Hi, how's the progress on review and getting this set finalised?

Re: [PATCH v7 00/10] Designware PWM driver updates for OF

[Uwe Kleine-König]

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

Hello Ben,

On Tue, Jan 17, 2023 at 04:39:34PM +0000, Ben Dooks wrote:
> On 23/12/2022 15:38, Ben Dooks wrote:
> > An updated set of patches for the Designware PWM driver
> > split into PCI and OF versions. I think I got all the
> > review issues in this set.
> > 
> > Sorry for the delay in getting this out, between conferences
> > and other absences there has been little time to deal with
> > this set. I will be now out of office until 3rd Jan 2023.
> 
> Hi, how's the progress on review and getting this set finalised?

Speaking for me:

Your patch set isn't forgotton. It's just that my time is limited and
reviewing a new driver is time intensive.

I'm sorry I cannot give feedback in a more timely manner, but I will
come to it eventually.

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: (subset) [PATCH v7 00/10] Designware PWM driver updates for OF

[Thierry Reding]

On Fri, 23 Dec 2022 15:38:10 +0000, Ben Dooks wrote:
> An updated set of patches for the Designware PWM driver
> split into PCI and OF versions. I think I got all the
> review issues in this set.
> 
> Sorry for the delay in getting this out, between conferences
> and other absences there has been little time to deal with
> this set. I will be now out of office until 3rd Jan 2023.
> 
> [...]

Applied, thanks!

[01/10] dt-bindings: pwm: Document Synopsys DesignWare snps,pwm-dw-apb-timers-pwm2
        commit: 0f03bf300833c05d914ab7f5ab3d8bc8564e9912
[02/10] pwm: dwc: allow driver to be built with COMPILE_TEST
        commit: c901a57e39db555ad7950fd61e1470cdecc8e654
[03/10] pwm: dwc: change &pci->dev to dev in probe
        commit: 8f3c7ab881ed7329003e10a2dd58f735abda2259
[04/10] pwm: dwc: move memory alloc to own function
        commit: a4218d7cf8978f397e731d1f15ef33d28f77e42b
[05/10] pwm: dwc: use devm_pwmchip_add
        commit: 7a77daf8223e772a225d6aa6202a5b1ae2392caf

Best regards,
-- 
Thierry Reding <thierry.reding@gmail.com>

Re: (subset) [PATCH v7 00/10] Designware PWM driver updates for OF

[Uwe Kleine-König]

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

On Fri, Feb 17, 2023 at 04:08:09PM +0100, Thierry Reding wrote:
> On Fri, 23 Dec 2022 15:38:10 +0000, Ben Dooks wrote:
> > An updated set of patches for the Designware PWM driver
> > split into PCI and OF versions. I think I got all the
> > review issues in this set.
> > 
> > Sorry for the delay in getting this out, between conferences
> > and other absences there has been little time to deal with
> > this set. I will be now out of office until 3rd Jan 2023.
> > 
> > [...]
> 
> Applied, thanks!
> 
> [01/10] dt-bindings: pwm: Document Synopsys DesignWare snps,pwm-dw-apb-timers-pwm2
>         commit: 0f03bf300833c05d914ab7f5ab3d8bc8564e9912
> [02/10] pwm: dwc: allow driver to be built with COMPILE_TEST
>         commit: c901a57e39db555ad7950fd61e1470cdecc8e654
> [03/10] pwm: dwc: change &pci->dev to dev in probe
>         commit: 8f3c7ab881ed7329003e10a2dd58f735abda2259
> [04/10] pwm: dwc: move memory alloc to own function
>         commit: a4218d7cf8978f397e731d1f15ef33d28f77e42b
> [05/10] pwm: dwc: use devm_pwmchip_add
>         commit: 7a77daf8223e772a225d6aa6202a5b1ae2392caf

I had some comments for patches #6 and #10. Patches #7 - #9 are still
marked as new in the PWM patchwork, but I will mark them as
changes-requested in the assumption that you'll have to rework them to
address my feedback for the other two patches anyhow.

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 --]