[PATCH] pwm: Add DesignWare PWM Controller Driver

[PATCH] pwm: Add DesignWare PWM Controller Driver

[Jarkko Nikula]

Introduce driver for Synopsys DesignWare PWM Controller used on Intel
Elkhart Lake.

Initial implementation is done by Felipe Balbi while he was working at
Intel with later changes from Raymond Tan and me.

Co-developed-by: Raymond Tan <raymond.tan@intel.com>
Signed-off-by: Raymond Tan <raymond.tan@intel.com>
Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
 drivers/pwm/Kconfig   |   9 ++
 drivers/pwm/Makefile  |   1 +
 drivers/pwm/pwm-dwc.c | 328 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 338 insertions(+)
 create mode 100644 drivers/pwm/pwm-dwc.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 30190beeb6e9..cfded9efdb17 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -150,6 +150,15 @@ 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
+	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.
+
 config PWM_EP93XX
 	tristate "Cirrus Logic EP93xx PWM support"
 	depends on ARCH_EP93XX
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 9a475073dafc..eb52012f0e79 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_PWM_BRCMSTB)	+= pwm-brcmstb.o
 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_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.c b/drivers/pwm/pwm-dwc.c
new file mode 100644
index 000000000000..962d504c1281
--- /dev/null
+++ b/drivers/pwm/pwm-dwc.c
@@ -0,0 +1,328 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * DesignWare PWM Controller driver
+ *
+ * Copyright (C) 2018-2020 Intel Corporation
+ *
+ * Author: Felipe Balbi
+ */
+
+#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 <linux/mutex.h>
+#include <linux/pm_runtime.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
+
+/* 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_driver_data {
+	unsigned long clk_period_ns;
+	int npwm;
+};
+
+struct dwc_pwm {
+	struct pwm_chip chip;
+	struct device *dev;
+	struct mutex lock;
+
+	unsigned long clk_period_ns;
+	unsigned int version;
+
+	void __iomem *base;
+
+	u32 saved_registers[24];
+};
+#define to_dwc_pwm(p)	(container_of((p), struct dwc_pwm, chip))
+
+static inline u32 dwc_pwm_readl(void __iomem *base, u32 offset)
+{
+	return readl(base + offset);
+}
+
+static inline void dwc_pwm_writel(void __iomem *base, u32 offset, u32 value)
+{
+	writel(value, base + offset);
+}
+
+static void __dwc_pwm_configure(struct dwc_pwm *dwc, int pwm,
+				unsigned int duty_ns,
+				unsigned int period_ns)
+{
+	u32 ctrl;
+	u32 high;
+	u32 low;
+
+	high = DIV_ROUND_CLOSEST(duty_ns, dwc->clk_period_ns) - 1;
+	low = DIV_ROUND_CLOSEST(period_ns - duty_ns, dwc->clk_period_ns) - 1;
+
+	dwc_pwm_writel(dwc->base, DWC_TIM_LD_CNT(pwm), low);
+	dwc_pwm_writel(dwc->base, DWC_TIM_LD_CNT2(pwm), high);
+
+	ctrl = DWC_TIM_CTRL_MODE_USER | DWC_TIM_CTRL_PWM;
+	dwc_pwm_writel(dwc->base, DWC_TIM_CTRL(pwm), ctrl);
+}
+
+static u32 __dwc_pwm_duty_ns(struct dwc_pwm *dwc, int pwm)
+{
+	u32 duty;
+
+	duty = dwc_pwm_readl(dwc->base, DWC_TIM_LD_CNT2(pwm));
+	duty += 1;
+	duty *= dwc->clk_period_ns;
+
+	return duty;
+}
+
+static u32 __dwc_pwm_period_ns(struct dwc_pwm *dwc, int pwm, u32 duty)
+{
+	u32 period;
+
+	period = dwc_pwm_readl(dwc->base, DWC_TIM_LD_CNT(pwm));
+	period += 1;
+	period *= dwc->clk_period_ns;
+	period += duty;
+
+	return period;
+}
+
+static bool __dwc_pwm_is_enabled(struct dwc_pwm *dwc, int pwm)
+{
+	return !!(dwc_pwm_readl(dwc->base,
+				DWC_TIM_CTRL(pwm)) & DWC_TIM_CTRL_EN);
+}
+
+static void __dwc_pwm_set_enable(struct dwc_pwm *dwc, int pwm, int enabled)
+{
+	u32 reg;
+
+	reg = dwc_pwm_readl(dwc->base, DWC_TIM_CTRL(pwm));
+
+	if (enabled)
+		reg |= DWC_TIM_CTRL_EN;
+	else
+		reg &= ~DWC_TIM_CTRL_EN;
+
+	dwc_pwm_writel(dwc->base, DWC_TIM_CTRL(pwm), reg);
+}
+
+static void __dwc_pwm_configure_timer(struct dwc_pwm *dwc,
+				      struct pwm_device *pwm,
+				      const struct pwm_state *state)
+{
+	__dwc_pwm_set_enable(dwc, pwm->hwpwm, false);
+	__dwc_pwm_configure(dwc, pwm->hwpwm, state->duty_cycle,
+			    state->period);
+	__dwc_pwm_set_enable(dwc, pwm->hwpwm, state->enabled);
+}
+
+static int dwc_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			 const struct pwm_state *state)
+{
+	struct dwc_pwm *dwc = to_dwc_pwm(chip);
+
+	mutex_lock(&dwc->lock);
+
+	if (state->enabled) {
+		if (!pwm_is_enabled(pwm))
+			pm_runtime_get_sync(dwc->dev);
+		__dwc_pwm_configure_timer(dwc, pwm, state);
+	} else {
+		if (pwm_is_enabled(pwm)) {
+			__dwc_pwm_set_enable(dwc, pwm->hwpwm, false);
+			pm_runtime_put_sync(dwc->dev);
+		}
+	}
+
+	mutex_unlock(&dwc->lock);
+
+	return 0;
+}
+
+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);
+
+	pm_runtime_get_sync(dwc->dev);
+
+	mutex_lock(&dwc->lock);
+	state->enabled = __dwc_pwm_is_enabled(dwc, pwm->hwpwm);
+	state->duty_cycle = __dwc_pwm_duty_ns(dwc, pwm->hwpwm);
+	state->period = __dwc_pwm_period_ns(dwc, pwm->hwpwm,
+					    state->duty_cycle);
+	mutex_unlock(&dwc->lock);
+
+	pm_runtime_put_sync(dwc->dev);
+}
+
+static const struct pwm_ops dwc_pwm_ops = {
+	.apply		= dwc_pwm_apply,
+	.get_state	= dwc_pwm_get_state,
+	.owner		= THIS_MODULE,
+};
+
+static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
+{
+	struct dwc_pwm_driver_data *data;
+	struct dwc_pwm *dwc;
+	struct device *dev;
+	int ret;
+	int i;
+
+	data = (struct dwc_pwm_driver_data *) id->driver_data;
+	dev = &pci->dev;
+
+	dwc = devm_kzalloc(&pci->dev, sizeof(*dwc), GFP_KERNEL);
+	if (!dwc)
+		return -ENOMEM;
+
+	dwc->dev = dev;
+	dwc->clk_period_ns = data->clk_period_ns;
+
+	ret = pcim_enable_device(pci);
+	if (ret)
+		return ret;
+
+	pci_set_master(pci);
+
+	ret = pcim_iomap_regions(pci, BIT(0), pci_name(pci));
+	if (ret)
+		return ret;
+
+	dwc->base = pcim_iomap_table(pci)[0];
+	if (!dwc->base)
+		return -ENOMEM;
+
+	dwc->version = dwc_pwm_readl(dwc->base, DWC_TIMERS_COMP_VERSION);
+
+	/* mask all interrupts and disable all timers */
+	for (i = 0; i < data->npwm; i++) {
+		dwc_pwm_writel(dwc->base, DWC_TIM_CTRL(i), 0);
+		dwc_pwm_writel(dwc->base, DWC_TIM_LD_CNT(i), 0);
+		dwc_pwm_writel(dwc->base, DWC_TIM_CUR_VAL(i), 0);
+	}
+
+	mutex_init(&dwc->lock);
+	pci_set_drvdata(pci, dwc);
+
+	dwc->chip.dev = dev;
+	dwc->chip.ops = &dwc_pwm_ops;
+	dwc->chip.npwm = data->npwm;
+	dwc->chip.base = -1;
+
+	ret = pwmchip_add(&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)
+{
+	struct dwc_pwm *dwc = pci_get_drvdata(pci);
+	int i;
+
+	pm_runtime_forbid(&pci->dev);
+	pm_runtime_get_noresume(&pci->dev);
+
+	for (i = 0; i < dwc->chip.npwm; i++)
+		pwm_disable(&dwc->chip.pwms[i]);
+
+	pwmchip_remove(&dwc->chip);
+}
+
+#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, index_base;
+
+	for (i = 0; i < DWC_TIMERS_TOTAL; i++) {
+		index_base = i * 3;
+		dwc->saved_registers[index_base] =
+			dwc_pwm_readl(dwc->base, DWC_TIM_LD_CNT(i));
+		dwc->saved_registers[index_base+1] =
+			dwc_pwm_readl(dwc->base, DWC_TIM_LD_CNT2(i));
+		dwc->saved_registers[index_base+2] =
+			dwc_pwm_readl(dwc->base, 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, index_base;
+
+	for (i = 0; i < DWC_TIMERS_TOTAL; i++) {
+		index_base = i * 3;
+		dwc_pwm_writel(dwc->base, DWC_TIM_LD_CNT(i),
+			       dwc->saved_registers[index_base]);
+		dwc_pwm_writel(dwc->base, DWC_TIM_LD_CNT2(i),
+			       dwc->saved_registers[index_base+1]);
+		dwc_pwm_writel(dwc->base, DWC_TIM_CTRL(i),
+			       dwc->saved_registers[index_base+2]);
+	}
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(dwc_pwm_pm_ops, dwc_pwm_suspend, dwc_pwm_resume);
+
+static const struct dwc_pwm_driver_data ehl_driver_data = {
+	.npwm = 8,
+	.clk_period_ns = 10,
+};
+
+static const struct pci_device_id dwc_pwm_id_table[] = {
+	{ PCI_VDEVICE(INTEL, 0x4bb7), (kernel_ulong_t) &ehl_driver_data },
+	{  }	/* 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>");
+MODULE_DESCRIPTION("DesignWare PWM Controller");
+MODULE_LICENSE("GPL v2");
-- 
2.25.1

Re: [PATCH] pwm: Add DesignWare PWM Controller Driver

[Felipe Balbi]

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


Hi Jarkko,

Jarkko Nikula <jarkko.nikula@linux.intel.com> writes:

> Introduce driver for Synopsys DesignWare PWM Controller used on Intel
> Elkhart Lake.
>
> Initial implementation is done by Felipe Balbi while he was working at
> Intel with later changes from Raymond Tan and me.
>
> Co-developed-by: Raymond Tan <raymond.tan@intel.com>
> Signed-off-by: Raymond Tan <raymond.tan@intel.com>
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Thanks for sending this. FWIW:

Signed-off-by: Felipe Balbi <balbi@kernel.org>

-- 
balbi

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

Re: [PATCH] pwm: Add DesignWare PWM Controller Driver

[Uwe Kleine-König]

Hello Jarkko,

On Wed, Apr 08, 2020 at 03:54:04PM +0300, Jarkko Nikula wrote:
> Introduce driver for Synopsys DesignWare PWM Controller used on Intel
> Elkhart Lake.
> 
> Initial implementation is done by Felipe Balbi while he was working at
> Intel with later changes from Raymond Tan and me.
> 
> Co-developed-by: Raymond Tan <raymond.tan@intel.com>
> Signed-off-by: Raymond Tan <raymond.tan@intel.com>
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> ---
>  drivers/pwm/Kconfig   |   9 ++
>  drivers/pwm/Makefile  |   1 +
>  drivers/pwm/pwm-dwc.c | 328 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 338 insertions(+)
>  create mode 100644 drivers/pwm/pwm-dwc.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 30190beeb6e9..cfded9efdb17 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -150,6 +150,15 @@ 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
> +	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.
> +
>  config PWM_EP93XX
>  	tristate "Cirrus Logic EP93xx PWM support"
>  	depends on ARCH_EP93XX
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 9a475073dafc..eb52012f0e79 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_PWM_BRCMSTB)	+= pwm-brcmstb.o
>  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_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.c b/drivers/pwm/pwm-dwc.c
> new file mode 100644
> index 000000000000..962d504c1281
> --- /dev/null
> +++ b/drivers/pwm/pwm-dwc.c
> @@ -0,0 +1,328 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
> + * DesignWare PWM Controller driver
> + *
> + * Copyright (C) 2018-2020 Intel Corporation
> + *
> + * Author: Felipe Balbi

Is there a publicly available reference manual available? If so, adding
a link here would be great.

> + */
> +
> +#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 <linux/mutex.h>
> +#include <linux/pm_runtime.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
> +
> +/* 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_driver_data {
> +	unsigned long clk_period_ns;
> +	int npwm;
> +};
> +
> +struct dwc_pwm {
> +	struct pwm_chip chip;
> +	struct device *dev;
> +	struct mutex lock;
> +
> +	unsigned long clk_period_ns;
> +	unsigned int version;

version is a write-only variable, this can be dropped.

> +
> +	void __iomem *base;
> +
> +	u32 saved_registers[24];
> +};
> +#define to_dwc_pwm(p)	(container_of((p), struct dwc_pwm, chip))
> +
> +static inline u32 dwc_pwm_readl(void __iomem *base, u32 offset)
> +{
> +	return readl(base + offset);
> +}
> +
> +static inline void dwc_pwm_writel(void __iomem *base, u32 offset, u32 value)
> +{
> +	writel(value, base + offset);

I think it's unfortunate that the order of parameters is different for
dwc_pwm_writel and writel.

> +}
> +
> +static void __dwc_pwm_configure(struct dwc_pwm *dwc, int pwm,
> +				unsigned int duty_ns,
> +				unsigned int period_ns)
> +{
> +	u32 ctrl;
> +	u32 high;
> +	u32 low;
> +
> +	high = DIV_ROUND_CLOSEST(duty_ns, dwc->clk_period_ns) - 1;
> +	low = DIV_ROUND_CLOSEST(period_ns - duty_ns, dwc->clk_period_ns) - 1;

Have you tested your driver with CONFIG_PWM_DEBUG? I would expect that
using ROUND_CLOSEST triggers

	.apply is supposed to round down period

in some situations.

Maybe soon duty_ns and period_ns will become 64 bit integers. I suggest
to already cope for this today and introduce proper range checking.

> +	dwc_pwm_writel(dwc->base, DWC_TIM_LD_CNT(pwm), low);
> +	dwc_pwm_writel(dwc->base, DWC_TIM_LD_CNT2(pwm), high);
> +
> +	ctrl = DWC_TIM_CTRL_MODE_USER | DWC_TIM_CTRL_PWM;
> +	dwc_pwm_writel(dwc->base, DWC_TIM_CTRL(pwm), ctrl);
> +}
> +
> +static u32 __dwc_pwm_duty_ns(struct dwc_pwm *dwc, int pwm)
> +{
> +	u32 duty;
> +
> +	duty = dwc_pwm_readl(dwc->base, DWC_TIM_LD_CNT2(pwm));
> +	duty += 1;
> +	duty *= dwc->clk_period_ns;
> +
> +	return duty;
> +}
> +
> +static u32 __dwc_pwm_period_ns(struct dwc_pwm *dwc, int pwm, u32 duty)
> +{
> +	u32 period;
> +
> +	period = dwc_pwm_readl(dwc->base, DWC_TIM_LD_CNT(pwm));
> +	period += 1;
> +	period *= dwc->clk_period_ns;

Can this overflow? (Hmm, I wonder what should be done about this as
.get_state() returns void.)

> +	period += duty;
> +
> +	return period;
> +}
> +
> +static bool __dwc_pwm_is_enabled(struct dwc_pwm *dwc, int pwm)
> +{
> +	return !!(dwc_pwm_readl(dwc->base,
> +				DWC_TIM_CTRL(pwm)) & DWC_TIM_CTRL_EN);
> +}
> +
> +static void __dwc_pwm_set_enable(struct dwc_pwm *dwc, int pwm, int enabled)
> +{
> +	u32 reg;
> +
> +	reg = dwc_pwm_readl(dwc->base, DWC_TIM_CTRL(pwm));
> +
> +	if (enabled)
> +		reg |= DWC_TIM_CTRL_EN;
> +	else
> +		reg &= ~DWC_TIM_CTRL_EN;
> +
> +	dwc_pwm_writel(dwc->base, DWC_TIM_CTRL(pwm), reg);

What happens to the output if the EN bit is cleared? Does the hardware
complete the currently running period? If not, does the output "freeze"
or get inactive?

> +}
> +
> +static void __dwc_pwm_configure_timer(struct dwc_pwm *dwc,
> +				      struct pwm_device *pwm,
> +				      const struct pwm_state *state)
> +{
> +	__dwc_pwm_set_enable(dwc, pwm->hwpwm, false);
> +	__dwc_pwm_configure(dwc, pwm->hwpwm, state->duty_cycle,
> +			    state->period);

Is it necessary to stop the hardware before writing the counter
registers? Or is this only done to prevent race conditions that yield
unexpected cycles?

> +	__dwc_pwm_set_enable(dwc, pwm->hwpwm, state->enabled);
> +}
> +
> +static int dwc_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			 const struct pwm_state *state)
> +{
> +	struct dwc_pwm *dwc = to_dwc_pwm(chip);
> +
> +	mutex_lock(&dwc->lock);
> +
> +	if (state->enabled) {
> +		if (!pwm_is_enabled(pwm))

If pwm_is_enabled() would be changed to call .get_state() you'd run in a
dead lock. Please don't call PWM API functions in driver callbacks.

> +			pm_runtime_get_sync(dwc->dev);
> +		__dwc_pwm_configure_timer(dwc, pwm, state);
> +	} else {
> +		if (pwm_is_enabled(pwm)) {
> +			__dwc_pwm_set_enable(dwc, pwm->hwpwm, false);
> +			pm_runtime_put_sync(dwc->dev);
> +		}
> +	}

You don't check state->polarity here.

> +
> +	mutex_unlock(&dwc->lock);
> +
> +	return 0;
> +}
> +
> +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);
> +
> +	pm_runtime_get_sync(dwc->dev);
> +
> +	mutex_lock(&dwc->lock);
> +	state->enabled = __dwc_pwm_is_enabled(dwc, pwm->hwpwm);
> +	state->duty_cycle = __dwc_pwm_duty_ns(dwc, pwm->hwpwm);
> +	state->period = __dwc_pwm_period_ns(dwc, pwm->hwpwm,
> +					    state->duty_cycle);

I wonder if doing the math in .get_state() directly
instead of introducing a function with IMHO strange semantics that is
only used once would be easier to understand.

> +	mutex_unlock(&dwc->lock);
> +
> +	pm_runtime_put_sync(dwc->dev);
> +}
> +
> +static const struct pwm_ops dwc_pwm_ops = {
> +	.apply		= dwc_pwm_apply,
> +	.get_state	= dwc_pwm_get_state,
> +	.owner		= THIS_MODULE,

I personally don't like aligning the = in structure initializers. There
seem to be different tastes involved. If you don't care, please use just
one space before and after the = please.

> +};
> +
> +static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
> +{
> +	struct dwc_pwm_driver_data *data;
> +	struct dwc_pwm *dwc;
> +	struct device *dev;
> +	int ret;
> +	int i;
> +
> +	data = (struct dwc_pwm_driver_data *) id->driver_data;
> +	dev = &pci->dev;
> +
> +	dwc = devm_kzalloc(&pci->dev, sizeof(*dwc), GFP_KERNEL);
> +	if (!dwc)
> +		return -ENOMEM;
> +
> +	dwc->dev = dev;
> +	dwc->clk_period_ns = data->clk_period_ns;
> +
> +	ret = pcim_enable_device(pci);
> +	if (ret)

Error message here ..

> +		return ret;
> +
> +	pci_set_master(pci);
> +
> +	ret = pcim_iomap_regions(pci, BIT(0), pci_name(pci));
> +	if (ret)

.. and here ..

> +		return ret;
> +
> +	dwc->base = pcim_iomap_table(pci)[0];
> +	if (!dwc->base)

.. and here?

> +		return -ENOMEM;
> +
> +	dwc->version = dwc_pwm_readl(dwc->base, DWC_TIMERS_COMP_VERSION);
> +
> +	/* mask all interrupts and disable all timers */
> +	for (i = 0; i < data->npwm; i++) {
> +		dwc_pwm_writel(dwc->base, DWC_TIM_CTRL(i), 0);
> +		dwc_pwm_writel(dwc->base, DWC_TIM_LD_CNT(i), 0);
> +		dwc_pwm_writel(dwc->base, DWC_TIM_CUR_VAL(i), 0);
> +	}

Does "disable all timers" result in stopping all outputs? If so, please
don't to allow taking over running hardware from the boot loader.

> +	mutex_init(&dwc->lock);

What does this mutex protect? I think it's save to assume that calls to
.get_state() and .apply_state() for a single channel are serialized by the
core, so locking might not be necessary at the driver level.

> +	pci_set_drvdata(pci, dwc);
> +
> +	dwc->chip.dev = dev;
> +	dwc->chip.ops = &dwc_pwm_ops;
> +	dwc->chip.npwm = data->npwm;
> +	dwc->chip.base = -1;
> +
> +	ret = pwmchip_add(&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)
> +{
> +	struct dwc_pwm *dwc = pci_get_drvdata(pci);
> +	int i;
> +
> +	pm_runtime_forbid(&pci->dev);
> +	pm_runtime_get_noresume(&pci->dev);
> +
> +	for (i = 0; i < dwc->chip.npwm; i++)
> +		pwm_disable(&dwc->chip.pwms[i]);

You're not supposed to call pwm_disable() here.

> +
> +	pwmchip_remove(&dwc->chip);
> +}
> +
> +#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, index_base;
> +
> +	for (i = 0; i < DWC_TIMERS_TOTAL; i++) {
> +		index_base = i * 3;
> +		dwc->saved_registers[index_base] =
> +			dwc_pwm_readl(dwc->base, DWC_TIM_LD_CNT(i));
> +		dwc->saved_registers[index_base+1] =
> +			dwc_pwm_readl(dwc->base, DWC_TIM_LD_CNT2(i));
> +		dwc->saved_registers[index_base+2] =
> +			dwc_pwm_readl(dwc->base, DWC_TIM_CTRL(i));
> +	}

Does sleeping stop the output? If so (I think) you should prevent going
to sleep if the consumers didn't stop the output.

> +	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, index_base;
> +
> +	for (i = 0; i < DWC_TIMERS_TOTAL; i++) {
> +		index_base = i * 3;
> +		dwc_pwm_writel(dwc->base, DWC_TIM_LD_CNT(i),
> +			       dwc->saved_registers[index_base]);
> +		dwc_pwm_writel(dwc->base, DWC_TIM_LD_CNT2(i),
> +			       dwc->saved_registers[index_base+1]);
> +		dwc_pwm_writel(dwc->base, DWC_TIM_CTRL(i),
> +			       dwc->saved_registers[index_base+2]);

I think this code would benefit from saved_registers having a better
type. Something like

	struct {
		u32 cnt;
		u32 cnt2;
		u32 ctrl;
	} saved_registers[DWC_TIMERS_TOTAL];

	
> +	}
> +
> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(dwc_pwm_pm_ops, dwc_pwm_suspend, dwc_pwm_resume);
> +
> +static const struct dwc_pwm_driver_data ehl_driver_data = {
> +	.npwm = 8,
> +	.clk_period_ns = 10,
> +};
> +
> +static const struct pci_device_id dwc_pwm_id_table[] = {
> +	{ PCI_VDEVICE(INTEL, 0x4bb7), (kernel_ulong_t) &ehl_driver_data },

If there is only a single device type, you could better hard code (with
cpp symbols) the values contained in ehl_driver_data.

> +	{  }	/* 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>");
> +MODULE_DESCRIPTION("DesignWare PWM Controller");
> +MODULE_LICENSE("GPL v2");

Is "GPL v2" still the one that should be used? I thought "GPL" was the
one to use.

Best regards
Uwe

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

Re: [PATCH] pwm: Add DesignWare PWM Controller Driver

[Jarkko Nikula]

Hi

On 4/9/20 11:48 AM, Uwe Kleine-König wrote:
>> diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
>> new file mode 100644
>> index 000000000000..962d504c1281
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-dwc.c
>> @@ -0,0 +1,328 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/**
>> + * DesignWare PWM Controller driver
>> + *
>> + * Copyright (C) 2018-2020 Intel Corporation
>> + *
>> + * Author: Felipe Balbi
> 
> Is there a publicly available reference manual available? If so, adding
> a link here would be great.
> 
Point taken as well as for other trivial ones. Replies to more complex 
ones below.

>> +static void __dwc_pwm_configure(struct dwc_pwm *dwc, int pwm,
>> +				unsigned int duty_ns,
>> +				unsigned int period_ns)
>> +{
>> +	u32 ctrl;
>> +	u32 high;
>> +	u32 low;
>> +
>> +	high = DIV_ROUND_CLOSEST(duty_ns, dwc->clk_period_ns) - 1;
>> +	low = DIV_ROUND_CLOSEST(period_ns - duty_ns, dwc->clk_period_ns) - 1;
> 
> Have you tested your driver with CONFIG_PWM_DEBUG? I would expect that
> using ROUND_CLOSEST triggers
> 
> 	.apply is supposed to round down period
> 
> in some situations.
> 
Hmm.. in fact not tested, nor near the and over the boundaries. Will check.

> Maybe soon duty_ns and period_ns will become 64 bit integers. I suggest
> to already cope for this today and introduce proper range checking.
> 
Ah, good to know.

>> +static void __dwc_pwm_set_enable(struct dwc_pwm *dwc, int pwm, int enabled)
>> +{
>> +	u32 reg;
>> +
>> +	reg = dwc_pwm_readl(dwc->base, DWC_TIM_CTRL(pwm));
>> +
>> +	if (enabled)
>> +		reg |= DWC_TIM_CTRL_EN;
>> +	else
>> +		reg &= ~DWC_TIM_CTRL_EN;
>> +
>> +	dwc_pwm_writel(dwc->base, DWC_TIM_CTRL(pwm), reg);
> 
> What happens to the output if the EN bit is cleared? Does the hardware
> complete the currently running period? If not, does the output "freeze"
> or get inactive?
...
> 
> Is it necessary to stop the hardware before writing the counter
> registers? Or is this only done to prevent race conditions that yield
> unexpected cycles?
> 
Good questions. I need to spend time in the lab next week and debug :-)

>> +static int dwc_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>> +			 const struct pwm_state *state)
>> +{
>> +	struct dwc_pwm *dwc = to_dwc_pwm(chip);
>> +
>> +	mutex_lock(&dwc->lock);
>> +
>> +	if (state->enabled) {
>> +		if (!pwm_is_enabled(pwm))
> 
> If pwm_is_enabled() would be changed to call .get_state() you'd run in a
> dead lock. Please don't call PWM API functions in driver callbacks.
> 
Oh. I guess we copied this from pwm-lpss.c and pwm-mxs.c.

>> +			pm_runtime_get_sync(dwc->dev);
>> +		__dwc_pwm_configure_timer(dwc, pwm, state);
>> +	} else {
>> +		if (pwm_is_enabled(pwm)) {
>> +			__dwc_pwm_set_enable(dwc, pwm->hwpwm, false);
>> +			pm_runtime_put_sync(dwc->dev);
>> +		}
>> +	}
> 
> You don't check state->polarity here.
> 
If I recall correctly I didn't see the polarity bit in HW but will 
check. Should the driver emulate it by recalculating duty etc since I 
didn't see quickly from PWM core can a driver with apply() callback 
report support for polarity?

>> +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);
>> +
>> +	pm_runtime_get_sync(dwc->dev);
>> +
>> +	mutex_lock(&dwc->lock);
>> +	state->enabled = __dwc_pwm_is_enabled(dwc, pwm->hwpwm);
>> +	state->duty_cycle = __dwc_pwm_duty_ns(dwc, pwm->hwpwm);
>> +	state->period = __dwc_pwm_period_ns(dwc, pwm->hwpwm,
>> +					    state->duty_cycle);
> 
> I wonder if doing the math in .get_state() directly
> instead of introducing a function with IMHO strange semantics that is
> only used once would be easier to understand.
> 
Maybe. Will check how does it look that way.

>> +	dwc->version = dwc_pwm_readl(dwc->base, DWC_TIMERS_COMP_VERSION);
>> +
>> +	/* mask all interrupts and disable all timers */
>> +	for (i = 0; i < data->npwm; i++) {
>> +		dwc_pwm_writel(dwc->base, DWC_TIM_CTRL(i), 0);
>> +		dwc_pwm_writel(dwc->base, DWC_TIM_LD_CNT(i), 0);
>> +		dwc_pwm_writel(dwc->base, DWC_TIM_CUR_VAL(i), 0);
>> +	}
> 
> Does "disable all timers" result in stopping all outputs? If so, please
> don't to allow taking over running hardware from the boot loader.
> 
Very good point. We probably didn't think about that.

>> +	mutex_init(&dwc->lock);
> 
> What does this mutex protect? I think it's save to assume that calls to
> .get_state() and .apply_state() for a single channel are serialized by the
> core, so locking might not be necessary at the driver level.
> 
That would be good. Will check.

>> +#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, index_base;
>> +
>> +	for (i = 0; i < DWC_TIMERS_TOTAL; i++) {
>> +		index_base = i * 3;
>> +		dwc->saved_registers[index_base] =
>> +			dwc_pwm_readl(dwc->base, DWC_TIM_LD_CNT(i));
>> +		dwc->saved_registers[index_base+1] =
>> +			dwc_pwm_readl(dwc->base, DWC_TIM_LD_CNT2(i));
>> +		dwc->saved_registers[index_base+2] =
>> +			dwc_pwm_readl(dwc->base, DWC_TIM_CTRL(i));
>> +	}
> 
> Does sleeping stop the output? If so (I think) you should prevent going
> to sleep if the consumers didn't stop the output.
> 
I believe stopping the activity is an expectation during suspend but 
don't have the knowledge here. So in general PWM consumers should stop 
the PWM in their suspend handlers, e.g. in display panel drivers etc, 
rather than blindly stop it here? How about when PWM is enabled from sysfs?

-- 
Jarkko

Re: [PATCH] pwm: Add DesignWare PWM Controller Driver

[Uwe Kleine-König]

Hi Jarkko,

On Thu, Apr 09, 2020 at 03:10:46PM +0300, Jarkko Nikula wrote:
> > Maybe soon duty_ns and period_ns will become 64 bit integers. I suggest
> > to already cope for this today and introduce proper range checking.
> > 
> Ah, good to know.
> 
> > > +static void __dwc_pwm_set_enable(struct dwc_pwm *dwc, int pwm, int enabled)
> > > +{
> > > +	u32 reg;
> > > +
> > > +	reg = dwc_pwm_readl(dwc->base, DWC_TIM_CTRL(pwm));
> > > +
> > > +	if (enabled)
> > > +		reg |= DWC_TIM_CTRL_EN;
> > > +	else
> > > +		reg &= ~DWC_TIM_CTRL_EN;
> > > +
> > > +	dwc_pwm_writel(dwc->base, DWC_TIM_CTRL(pwm), reg);
> > 
> > What happens to the output if the EN bit is cleared? Does the hardware
> > complete the currently running period? If not, does the output "freeze"
> > or get inactive?
> ...
> > 
> > Is it necessary to stop the hardware before writing the counter
> > registers? Or is this only done to prevent race conditions that yield
> > unexpected cycles?
>
> Good questions. I need to spend time in the lab next week and debug :-)

So I assume this means modifying registers while the hardware is running
at least isn't prohibited in the reference manual. Note this question
was more out of interest and I think with the register semantics an
atomic update probably doesn't work in a sane way anyhow. (The
background is that with some controllers you can update while the PWM is
running but this might result in bogus output and you can discuss
felicitously about which is the right approach to handle such hardware.
I just wanted to know if this hardware has potential for this discussion
;-) As far as I understood your hardware you would get bogus output for
quite sure and so disable + reconfigure + enable seems sane.)

> > > +static int dwc_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > +			 const struct pwm_state *state)
> > > +{
> > > +	struct dwc_pwm *dwc = to_dwc_pwm(chip);
> > > +
> > > +	mutex_lock(&dwc->lock);
> > > +
> > > +	if (state->enabled) {
> > > +		if (!pwm_is_enabled(pwm))
> > 
> > If pwm_is_enabled() would be changed to call .get_state() you'd run in a
> > dead lock. Please don't call PWM API functions in driver callbacks.
>
> Oh. I guess we copied this from pwm-lpss.c and pwm-mxs.c.

But at least those don't use locking ...

> > > +			pm_runtime_get_sync(dwc->dev);
> > > +		__dwc_pwm_configure_timer(dwc, pwm, state);
> > > +	} else {
> > > +		if (pwm_is_enabled(pwm)) {
> > > +			__dwc_pwm_set_enable(dwc, pwm->hwpwm, false);
> > > +			pm_runtime_put_sync(dwc->dev);
> > > +		}
> > > +	}
> > 
> > You don't check state->polarity here.
>
> If I recall correctly I didn't see the polarity bit in HW but will check.
> Should the driver emulate it by recalculating duty etc since I didn't see
> quickly from PWM core can a driver with apply() callback report support for
> polarity?

The usual thing is

	if (state->polarity != PWM_POLARITY_NORMAL)
		return -EINVAL;

in .apply. (Maybe the return code isn't the same everywhere.)

> > > +	for (i = 0; i < DWC_TIMERS_TOTAL; i++) {
> > > +		index_base = i * 3;
> > > +		dwc->saved_registers[index_base] =
> > > +			dwc_pwm_readl(dwc->base, DWC_TIM_LD_CNT(i));
> > > +		dwc->saved_registers[index_base+1] =
> > > +			dwc_pwm_readl(dwc->base, DWC_TIM_LD_CNT2(i));
> > > +		dwc->saved_registers[index_base+2] =
> > > +			dwc_pwm_readl(dwc->base, DWC_TIM_CTRL(i));
> > > +	}
> > 
> > Does sleeping stop the output? If so (I think) you should prevent going
> > to sleep if the consumers didn't stop the output.
>
> I believe stopping the activity is an expectation during suspend but don't
> have the knowledge here. So in general PWM consumers should stop the PWM in
> their suspend handlers, e.g. in display panel drivers etc, rather than
> blindly stop it here?

Yes.

> How about when PWM is enabled from sysfs?

sysfs isn't different here in a relevant way. The key idea is: Don't
stop the output unless the consumer requested it.

Best regards
Uwe

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

[PATCH] pwm: Add DesignWare PWM Controller Driver

[Jarkko Nikula]

Introduce driver for Synopsys DesignWare PWM Controller used on Intel
Elkhart Lake.

Initial implementation is done by Felipe Balbi while he was working at
Intel with later changes from Raymond Tan and me.

Co-developed-by: Felipe Balbi (Intel) <balbi@kernel.org>
Signed-off-by: Felipe Balbi (Intel) <balbi@kernel.org>
Co-developed-by: Raymond Tan <raymond.tan@intel.com>
Signed-off-by: Raymond Tan <raymond.tan@intel.com>
Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
v3. Previous version: https://www.spinics.net/lists/linux-pwm/msg12363.html
Changes:
- I got confirmation specification is not publicly available
- HW is actually fixed inversed polarity by the PWM framework
  conventions: HW cycle begins with a low period. Fixed polarity and
  low/high period calculation accordingly
- Added range check to period and duty_cycle
- Bogus '*' removed from head of the file comment
- struct dwc_pwm * passed to readl/writel wrappers instead of __iomem *
- %pe for error code prints
- clk_period_ns removed from struct dwc_pwm and use DWC_CLK_PERIOD_NS instead
- Device pointer removed from struct dwc_pwm, it's already carried in
  struct pwm_chip
- Added Limitations paragraph and commenting timer usage flow in code
- duty_cycle and period capping to 32-bit removed from
  dwc_pwm_get_state() since PWM core has been converted to 64-bit
- s/DIV_ROUND_CLOSEST/DIV_ROUND_CLOSEST_ULL/ to fix a link error on
  32-bit ARM and older GCC-9 build. Reported by kernel test robot
  <lkp@intel.com> on an internal tree
- Random cleanups, empty line removals etc
v2. First version here https://www.spinics.net/lists/linux-pwm/msg12122.html
Thanks to Uwe Kleine-König for good review comments, hopefully I captured
them all.
Changes:
- Added Felipe's Signed-of-by. I added (Intel) to his kernel.org address
  to highlight contribution was done while working at Intel
- Version register read removed as result was unused
- Order of dwc_pwm_writel() arguments changed to match with writel()
- Structure initializers use one space instead of tab alignment
- Error messages added to dwc_pwm_probe()
- MODULE_LICENSE() Updated based on a review comment and commit bf7fbeeae6db
  ("module: Cure the MODULE_LICENSE "GPL" vs. "GPL v2" bogosity")
- Polarity handled. HW supports only normal polarity and driver errors
  out in case of wrong polarity in dwc_pwm_apply() and returns fixed
  normal polarity in dwc_pwm_get_state()
- Running timers are not stopped on probe and remove. Those may be set
  running by a bootloader and driver should leave them runnning
- pwm_is_enabled() call changed to pwm->state.enabled in wc_pwm_apply()
- Co-authors added to MODULE_AUTHOR() and comment
- mutex removed
- Add struct dwc_pwm_ctx for register save/restore instead of word array
- suspend prevented in case of active PWM consumers. Please note this
  checks only PWMs enabled by Linux consumers and not the ones enabled
  by bootloader
- Duplicate linux/pm_runtime.h include removed
- Only once used trivial functions moved to dwc_pwm_get_state()
- struct dwc_pwm_driver_data removed and used hard coded properties
  instead since currently driver supports single device type
- Driver uses internally 64-bit duty and period calculation and caps
  them to 32-bit ns max value for PWM core. HW supports 32-bit high and
  low period counters with 10 ns resolution so HW can do ~42,9 s duty and
  ~85.9 s period at maximum
---
 drivers/pwm/Kconfig   |   9 ++
 drivers/pwm/Makefile  |   1 +
 drivers/pwm/pwm-dwc.c | 321 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 331 insertions(+)
 create mode 100644 drivers/pwm/pwm-dwc.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 9448e4ca8c73..ab79cb4734f1 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -160,6 +160,15 @@ 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
+	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.
+
 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 2c2ba0a03557..a1b7098fc4b8 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_PWM_BRCMSTB)	+= pwm-brcmstb.o
 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_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.c b/drivers/pwm/pwm-dwc.c
new file mode 100644
index 000000000000..e9cc57fc58d6
--- /dev/null
+++ b/drivers/pwm/pwm-dwc.c
@@ -0,0 +1,321 @@
+// 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>
+ *
+ * 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>
+
+#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);
+}
+
+static void __dwc_pwm_set_enable(struct dwc_pwm *dwc, int pwm, int enabled)
+{
+	u32 reg;
+
+	reg = dwc_pwm_readl(dwc, DWC_TIM_CTRL(pwm));
+
+	if (enabled)
+		reg |= DWC_TIM_CTRL_EN;
+	else
+		reg &= ~DWC_TIM_CTRL_EN;
+
+	dwc_pwm_writel(dwc, reg, DWC_TIM_CTRL(pwm));
+}
+
+static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc,
+				     struct pwm_device *pwm,
+				     const struct pwm_state *state)
+{
+	u64 tmp;
+	u32 ctrl;
+	u32 high;
+	u32 low;
+
+	/*
+	 * Calculate width of low and high period in terms of input clock
+	 * periods and check are the result within HW limits between 1 and
+	 * 2^32 periods.
+	 */
+	tmp = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, DWC_CLK_PERIOD_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);
+	if (tmp < 1 || tmp > (1ULL << 32))
+		return -ERANGE;
+	high = tmp - 1;
+
+	/*
+	 * Specification says timer usage flow is to disable timer, then
+	 * program it followed by enable. It also says Load Count is loaded
+	 * into timer after it is enabled - either after a disable or
+	 * a reset. Based on measurements it happens also without disable
+	 * whenever Load Count is updated. But follow the specification.
+	 */
+	__dwc_pwm_set_enable(dwc, pwm->hwpwm, false);
+
+	/*
+	 * Write Load Count and Load Count 2 registers. Former defines the
+	 * width of low period and latter the width of high period in terms
+	 * multiple of input clock periods:
+	 * Width = ((Count + 1) * input clock period).
+	 */
+	dwc_pwm_writel(dwc, low, DWC_TIM_LD_CNT(pwm->hwpwm));
+	dwc_pwm_writel(dwc, high, DWC_TIM_LD_CNT2(pwm->hwpwm));
+
+	/*
+	 * Set user-defined mode, timer reloads from Load Count registers
+	 * when it counts down to 0.
+	 * Set PWM mode, it makes output to toggle and width of low and high
+	 * periods are set by Load Count registers.
+	 */
+	ctrl = DWC_TIM_CTRL_MODE_USER | DWC_TIM_CTRL_PWM;
+	dwc_pwm_writel(dwc, ctrl, DWC_TIM_CTRL(pwm->hwpwm));
+
+	/*
+	 * Enable timer. Output starts from low period.
+	 */
+	__dwc_pwm_set_enable(dwc, pwm->hwpwm, state->enabled);
+
+	return 0;
+}
+
+static int dwc_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			 const struct pwm_state *state)
+{
+	struct dwc_pwm *dwc = to_dwc_pwm(chip);
+
+	if (state->polarity != PWM_POLARITY_INVERSED)
+		return -EINVAL;
+
+	if (state->enabled) {
+		if (!pwm->state.enabled)
+			pm_runtime_get_sync(chip->dev);
+		return __dwc_pwm_configure_timer(dwc, pwm, state);
+	} else {
+		if (pwm->state.enabled) {
+			__dwc_pwm_set_enable(dwc, pwm->hwpwm, false);
+			pm_runtime_put_sync(chip->dev);
+		}
+	}
+
+	return 0;
+}
+
+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);
+	u64 duty, period;
+
+	pm_runtime_get_sync(chip->dev);
+
+	state->enabled = !!(dwc_pwm_readl(dwc,
+				DWC_TIM_CTRL(pwm->hwpwm)) & DWC_TIM_CTRL_EN);
+
+	duty = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
+	duty += 1;
+	duty *= DWC_CLK_PERIOD_NS;
+	state->duty_cycle = duty;
+
+	period = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
+	period += 1;
+	period *= DWC_CLK_PERIOD_NS;
+	period += duty;
+	state->period = period;
+
+	state->polarity = PWM_POLARITY_INVERSED;
+
+	pm_runtime_put_sync(chip->dev);
+}
+
+static const struct pwm_ops dwc_pwm_ops = {
+	.apply = dwc_pwm_apply,
+	.get_state = dwc_pwm_get_state,
+	.owner = THIS_MODULE,
+};
+
+static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
+{
+	struct dwc_pwm *dwc;
+	struct device *dev;
+	int ret;
+
+	dev = &pci->dev;
+
+	dwc = devm_kzalloc(&pci->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));
+		return ret;
+	}
+
+	pci_set_master(pci);
+
+	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));
+		return ret;
+	}
+
+	dwc->base = pcim_iomap_table(pci)[0];
+	if (!dwc->base) {
+		dev_err(&pci->dev, "Base address missing\n");
+		return -ENOMEM;
+	}
+
+	pci_set_drvdata(pci, dwc);
+
+	dwc->chip.dev = dev;
+	dwc->chip.ops = &dwc_pwm_ops;
+	dwc->chip.npwm = DWC_TIMERS_TOTAL;
+	dwc->chip.base = -1;
+
+	ret = pwmchip_add(&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)
+{
+	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
+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");
-- 
2.28.0