[PATCH v1 0/2] Add PWM driver for Suplus SP7021 SoC

[PATCH v1 0/2] Add PWM driver for Suplus SP7021 SoC

[Hammer Hsieh]

This is a patch series for PWM driver for Suplus SP7021 SoC.

Sunplus SP7021 is an ARM Cortex A7 (4 cores) based SoC. It integrates
many peripherals (ex: UART. I2C, SPI, SDIO, eMMC, USB, SD card and
etc.) into a single chip. It is designed for industrial control.

Refer to:
https://sunplus-tibbo.atlassian.net/wiki/spaces/doc/overview
https://tibbo.com/store/plus1.html

Hammer Hsieh (2):
  dt-bindings:pwm:Add bindings doc for Sunplus SoC PWM Driver
  pwm:sunplus-pwm:Add Sunplus SoC PWM Driver

 .../devicetree/bindings/pwm/pwm-sunplus.yaml       |  45 +++++
 MAINTAINERS                                        |   6 +
 drivers/pwm/Kconfig                                |  11 ++
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-sunplus.c                          | 192 +++++++++++++++++++++
 5 files changed, 255 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sunplus.yaml
 create mode 100644 drivers/pwm/pwm-sunplus.c

-- 
2.7.4

[PATCH v1 1/2] dt-bindings:pwm:Add bindings doc for Sunplus SoC PWM Driver

[Hammer Hsieh]

Add bindings doc for Sunplus SoC PWM Driver

Signed-off-by: Hammer Hsieh <hammer.hsieh@sunplus.com>
---
 .../devicetree/bindings/pwm/pwm-sunplus.yaml       | 45 ++++++++++++++++++++++
 MAINTAINERS                                        |  5 +++
 2 files changed, 50 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sunplus.yaml

diff --git a/Documentation/devicetree/bindings/pwm/pwm-sunplus.yaml b/Documentation/devicetree/bindings/pwm/pwm-sunplus.yaml
new file mode 100644
index 0000000..9af19df
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-sunplus.yaml
@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) Sunplus Co., Ltd. 2021
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/pwm-sunplus.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sunplus SoC PWM Controller
+
+maintainers:
+  - Hammer Hsieh <hammer.hsieh@sunplus.com>
+
+properties:
+  '#pwm-cells':
+    const: 2
+
+  compatible:
+    items:
+      - const: sunplus,sp7021-pwm
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+required:
+  - '#pwm-cells'
+  - compatible
+  - reg
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |
+    pwm: pwm@9c007a00 {
+      #pwm-cells = <2>;
+      compatible = "sunplus,sp7021-pwm";
+      reg = <0x9c007a00 0x80>;
+      clocks = <&clkc 0xa2>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 13f9a84..721ed79 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18242,6 +18242,11 @@ L:	netdev@vger.kernel.org
 S:	Maintained
 F:	drivers/net/ethernet/dlink/sundance.c
 
+SUNPLUS PWM DRIVER
+M:	Hammer Hsieh <hammer.hsieh@sunplus.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/pwm/pwm-sunplus.yaml
+
 SUPERH
 M:	Yoshinori Sato <ysato@users.sourceforge.jp>
 M:	Rich Felker <dalias@libc.org>
-- 
2.7.4

[PATCH v1 2/2] pwm:sunplus-pwm:Add Sunplus SoC PWM Driver

[Hammer Hsieh]

Add Sunplus SoC PWM Driver

Signed-off-by: Hammer Hsieh <hammer.hsieh@sunplus.com>
---
 MAINTAINERS               |   1 +
 drivers/pwm/Kconfig       |  11 +++
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-sunplus.c | 192 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 205 insertions(+)
 create mode 100644 drivers/pwm/pwm-sunplus.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 721ed79..1c9e3c5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18246,6 +18246,7 @@ SUNPLUS PWM DRIVER
 M:	Hammer Hsieh <hammer.hsieh@sunplus.com>
 S:	Maintained
 F:	Documentation/devicetree/bindings/pwm/pwm-sunplus.yaml
+F:	drivers/pwm/pwm-sunplus.c
 
 SUPERH
 M:	Yoshinori Sato <ysato@users.sourceforge.jp>
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 21e3b05..9df5d5f 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -526,6 +526,17 @@ config PWM_SPRD
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-sprd.
 
+config PWM_SUNPLUS
+	tristate "Sunplus PWM support"
+	depends on ARCH_SUNPLUS || COMPILE_TEST
+	depends on HAS_IOMEM && OF
+	help
+	  Generic PWM framework driver for the PWM controller on
+	  Sunplus SoCs.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-sunplus.
+
 config PWM_STI
 	tristate "STiH4xx PWM support"
 	depends on ARCH_STI || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 708840b..be58616 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -53,6 +53,7 @@ obj-$(CONFIG_PWM_STM32)		+= pwm-stm32.o
 obj-$(CONFIG_PWM_STM32_LP)	+= pwm-stm32-lp.o
 obj-$(CONFIG_PWM_STMPE)		+= pwm-stmpe.o
 obj-$(CONFIG_PWM_SUN4I)		+= pwm-sun4i.o
+obj-$(CONFIG_PWM_SUNPLUS)	+= pwm-sunplus.o
 obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
 obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
 obj-$(CONFIG_PWM_TIEHRPWM)	+= pwm-tiehrpwm.o
diff --git a/drivers/pwm/pwm-sunplus.c b/drivers/pwm/pwm-sunplus.c
new file mode 100644
index 0000000..0ae59fc
--- /dev/null
+++ b/drivers/pwm/pwm-sunplus.c
@@ -0,0 +1,192 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PWM device driver for SUNPLUS SoCs
+ *
+ * Author: Hammer Hsieh <hammer.hsieh@sunplus.com>
+ */
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+
+#define PWM_SUP_CONTROL0	0x000
+#define PWM_SUP_CONTROL1	0x004
+#define PWM_SUP_FREQ_BASE	0x008
+#define PWM_SUP_DUTY_BASE	0x018
+#define PWM_SUP_FREQ(ch)	(PWM_SUP_FREQ_BASE + 4 * (ch))
+#define PWM_SUP_DUTY(ch)	(PWM_SUP_DUTY_BASE + 4 * (ch))
+#define PWM_SUP_FREQ_MAX	GENMASK(15, 0)
+#define PWM_SUP_DUTY_MAX	GENMASK(7, 0)
+
+#define PWM_SUP_NUM		4
+#define PWM_BYPASS_BIT_SHIFT	8
+#define PWM_DD_SEL_BIT_SHIFT	8
+#define PWM_SUP_FREQ_SCALER	256
+
+struct sunplus_pwm {
+	struct pwm_chip chip;
+	void __iomem *base;
+	struct clk *clk;
+};
+
+static inline struct sunplus_pwm *to_sunplus_pwm(struct pwm_chip *chip)
+{
+	return container_of(chip, struct sunplus_pwm, chip);
+}
+
+static void sunplus_reg_init(void __iomem *base)
+{
+	u32 i, value;
+
+	/* turn off all pwm channel output */
+	value = readl(base + PWM_SUP_CONTROL0);
+	value &= ~GENMASK((PWM_SUP_NUM - 1), 0);
+	writel(value, base + PWM_SUP_CONTROL0);
+
+	/* init all pwm channel clock source */
+	value = readl(base + PWM_SUP_CONTROL1);
+	value |= GENMASK((PWM_SUP_NUM - 1), 0);
+	writel(value, base + PWM_SUP_CONTROL1);
+
+	/* init all freq and duty setting */
+	for (i = 0; i < PWM_SUP_NUM; i++) {
+		writel(0, base + PWM_SUP_FREQ(i));
+		writel(0, base + PWM_SUP_DUTY(i));
+	}
+}
+
+static int sunplus_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			     const struct pwm_state *state)
+{
+	struct sunplus_pwm *priv = to_sunplus_pwm(chip);
+	u32 period_ns, duty_ns, value;
+	u32 dd_freq, duty;
+	u64 tmp;
+
+	if (!state->enabled) {
+		value = readl(priv->base + PWM_SUP_CONTROL0);
+		value &= ~BIT(pwm->hwpwm);
+		writel(value, priv->base + PWM_SUP_CONTROL0);
+		return 0;
+	}
+
+	period_ns = state->period;
+	duty_ns = state->duty_cycle;
+
+	/* cal pwm freq and check value under range */
+	tmp = clk_get_rate(priv->clk) * (u64)period_ns;
+	tmp = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
+	tmp = DIV_ROUND_CLOSEST_ULL(tmp, PWM_SUP_FREQ_SCALER);
+	dd_freq = (u32)tmp;
+
+	if (dd_freq == 0)
+		return -EINVAL;
+
+	if (dd_freq > PWM_SUP_FREQ_MAX)
+		dd_freq = PWM_SUP_FREQ_MAX;
+
+	writel(dd_freq, priv->base + PWM_SUP_FREQ(pwm->hwpwm));
+
+	/* cal and set pwm duty */
+	value = readl(priv->base + PWM_SUP_CONTROL0);
+	value |= BIT(pwm->hwpwm);
+	if (duty_ns == period_ns) {
+		value |= BIT(pwm->hwpwm + PWM_BYPASS_BIT_SHIFT);
+		duty = PWM_SUP_DUTY_MAX;
+	} else {
+		value &= ~BIT(pwm->hwpwm + PWM_BYPASS_BIT_SHIFT);
+		tmp = (u64)duty_ns * PWM_SUP_FREQ_SCALER + (period_ns >> 1);
+		tmp = DIV_ROUND_CLOSEST_ULL(tmp, (u64)period_ns);
+		duty = (u32)tmp;
+		duty |= (pwm->hwpwm << PWM_DD_SEL_BIT_SHIFT);
+	}
+	writel(value, priv->base + PWM_SUP_CONTROL0);
+	writel(duty, priv->base + PWM_SUP_DUTY(pwm->hwpwm));
+
+	return 0;
+}
+
+static void sunplus_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+				  struct pwm_state *state)
+{
+	struct sunplus_pwm *priv = to_sunplus_pwm(chip);
+	u32 value;
+
+	value = readl(priv->base + PWM_SUP_CONTROL0);
+
+	if (value & BIT(pwm->hwpwm))
+		state->enabled = true;
+	else
+		state->enabled = false;
+}
+
+static const struct pwm_ops sunplus_pwm_ops = {
+	.apply = sunplus_pwm_apply,
+	.get_state = sunplus_pwm_get_state,
+	.owner = THIS_MODULE,
+};
+
+static int sunplus_pwm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct sunplus_pwm *priv;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	priv->clk = devm_clk_get_optional(dev, NULL);
+	if (IS_ERR(priv->clk))
+		return dev_err_probe(dev, PTR_ERR(priv->clk),
+				     "get pwm clock failed\n");
+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret)
+		return ret;
+
+	ret = devm_add_action_or_reset(dev,
+				       (void(*)(void *))clk_disable_unprepare,
+				       priv->clk);
+	if (ret)
+		return ret;
+
+	priv->chip.dev = dev;
+	priv->chip.ops = &sunplus_pwm_ops;
+	priv->chip.npwm = PWM_SUP_NUM;
+
+	sunplus_reg_init(priv->base);
+
+	platform_set_drvdata(pdev, priv);
+
+	ret = devm_pwmchip_add(dev, &priv->chip);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Cannot register sunplus PWM\n");
+
+	return 0;
+}
+
+static const struct of_device_id sunplus_pwm_of_match[] = {
+	{ .compatible = "sunplus,sp7021-pwm", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, sunplus_pwm_of_match);
+
+static struct platform_driver sunplus_pwm_driver = {
+	.probe		= sunplus_pwm_probe,
+	.driver		= {
+		.name	= "sunplus-pwm",
+		.of_match_table = sunplus_pwm_of_match,
+	},
+};
+module_platform_driver(sunplus_pwm_driver);
+
+MODULE_DESCRIPTION("Sunplus SoC PWM Driver");
+MODULE_AUTHOR("Hammer Hsieh <hammer.hsieh@sunplus.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH v1 2/2] pwm:sunplus-pwm:Add Sunplus SoC PWM Driver

[Uwe Kleine-König]

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

Hello,

On Fri, Dec 17, 2021 at 07:46:08PM +0800, Hammer Hsieh wrote:
> Add Sunplus SoC PWM Driver
> 
> Signed-off-by: Hammer Hsieh <hammer.hsieh@sunplus.com>
> ---
>  MAINTAINERS               |   1 +
>  drivers/pwm/Kconfig       |  11 +++
>  drivers/pwm/Makefile      |   1 +
>  drivers/pwm/pwm-sunplus.c | 192 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 205 insertions(+)
>  create mode 100644 drivers/pwm/pwm-sunplus.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 721ed79..1c9e3c5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18246,6 +18246,7 @@ SUNPLUS PWM DRIVER
>  M:	Hammer Hsieh <hammer.hsieh@sunplus.com>
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/pwm/pwm-sunplus.yaml
> +F:	drivers/pwm/pwm-sunplus.c
>  
>  SUPERH
>  M:	Yoshinori Sato <ysato@users.sourceforge.jp>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 21e3b05..9df5d5f 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -526,6 +526,17 @@ config PWM_SPRD
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-sprd.
>  
> +config PWM_SUNPLUS
> +	tristate "Sunplus PWM support"
> +	depends on ARCH_SUNPLUS || COMPILE_TEST
> +	depends on HAS_IOMEM && OF
> +	help
> +	  Generic PWM framework driver for the PWM controller on
> +	  Sunplus SoCs.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-sunplus.
> +
>  config PWM_STI
>  	tristate "STiH4xx PWM support"
>  	depends on ARCH_STI || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 708840b..be58616 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -53,6 +53,7 @@ obj-$(CONFIG_PWM_STM32)		+= pwm-stm32.o
>  obj-$(CONFIG_PWM_STM32_LP)	+= pwm-stm32-lp.o
>  obj-$(CONFIG_PWM_STMPE)		+= pwm-stmpe.o
>  obj-$(CONFIG_PWM_SUN4I)		+= pwm-sun4i.o
> +obj-$(CONFIG_PWM_SUNPLUS)	+= pwm-sunplus.o
>  obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
>  obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
>  obj-$(CONFIG_PWM_TIEHRPWM)	+= pwm-tiehrpwm.o
> diff --git a/drivers/pwm/pwm-sunplus.c b/drivers/pwm/pwm-sunplus.c
> new file mode 100644
> index 0000000..0ae59fc
> --- /dev/null
> +++ b/drivers/pwm/pwm-sunplus.c
> @@ -0,0 +1,192 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PWM device driver for SUNPLUS SoCs
> + *
> + * Author: Hammer Hsieh <hammer.hsieh@sunplus.com>
> + */

Please add a section here about your hardware limitations. Please stick
to the format used in e.g. pwm-sifive.c. That is a block starting with

 * Limitations:

and then a list of issues. One such item is: Only supports normal
polarity.

> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +
> +#define PWM_SUP_CONTROL0	0x000
> +#define PWM_SUP_CONTROL1	0x004
> +#define PWM_SUP_FREQ_BASE	0x008
> +#define PWM_SUP_DUTY_BASE	0x018
> +#define PWM_SUP_FREQ(ch)	(PWM_SUP_FREQ_BASE + 4 * (ch))
> +#define PWM_SUP_DUTY(ch)	(PWM_SUP_DUTY_BASE + 4 * (ch))
> +#define PWM_SUP_FREQ_MAX	GENMASK(15, 0)
> +#define PWM_SUP_DUTY_MAX	GENMASK(7, 0)
> +
> +#define PWM_SUP_NUM		4
> +#define PWM_BYPASS_BIT_SHIFT	8
> +#define PWM_DD_SEL_BIT_SHIFT	8
> +#define PWM_SUP_FREQ_SCALER	256
> +
> +struct sunplus_pwm {
> +	struct pwm_chip chip;
> +	void __iomem *base;
> +	struct clk *clk;
> +};
> +
> +static inline struct sunplus_pwm *to_sunplus_pwm(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct sunplus_pwm, chip);
> +}
> +
> +static void sunplus_reg_init(void __iomem *base)
> +{
> +	u32 i, value;
> +
> +	/* turn off all pwm channel output */
> +	value = readl(base + PWM_SUP_CONTROL0);
> +	value &= ~GENMASK((PWM_SUP_NUM - 1), 0);
> +	writel(value, base + PWM_SUP_CONTROL0);
> +
> +	/* init all pwm channel clock source */
> +	value = readl(base + PWM_SUP_CONTROL1);
> +	value |= GENMASK((PWM_SUP_NUM - 1), 0);
> +	writel(value, base + PWM_SUP_CONTROL1);
> +
> +	/* init all freq and duty setting */
> +	for (i = 0; i < PWM_SUP_NUM; i++) {
> +		writel(0, base + PWM_SUP_FREQ(i));
> +		writel(0, base + PWM_SUP_DUTY(i));
> +	}

Please keep the PWM in their boot-up state. That is, if the bootloader
enabled a display with a bootsplash, don't disable the backlight when
the PWM driver loads.

> +}
> +
> +static int sunplus_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			     const struct pwm_state *state)
> +{
> +	struct sunplus_pwm *priv = to_sunplus_pwm(chip);
> +	u32 period_ns, duty_ns, value;
> +	u32 dd_freq, duty;
> +	u64 tmp;
> +

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

> +	if (!state->enabled) {
> +		value = readl(priv->base + PWM_SUP_CONTROL0);
> +		value &= ~BIT(pwm->hwpwm);
> +		writel(value, priv->base + PWM_SUP_CONTROL0);
> +		return 0;
> +	}
> +
> +	period_ns = state->period;

state->period is an u64, so you might loose precision here.

> +	duty_ns = state->duty_cycle;

ditto

> +
> +	/* cal pwm freq and check value under range */
> +	tmp = clk_get_rate(priv->clk) * (u64)period_ns;

This might overflow?

> +	tmp = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
> +	tmp = DIV_ROUND_CLOSEST_ULL(tmp, PWM_SUP_FREQ_SCALER);

In general you should pick the highest period that isn't bigger than the
requested period. I didn't check in detail, but using round-closest is a
strong hint that you get that wrong.

> +	dd_freq = (u32)tmp;
> +
> +	if (dd_freq == 0)
> +		return -EINVAL;
> +
> +	if (dd_freq > PWM_SUP_FREQ_MAX)
> +		dd_freq = PWM_SUP_FREQ_MAX;
> +
> +	writel(dd_freq, priv->base + PWM_SUP_FREQ(pwm->hwpwm));
> +
> +	/* cal and set pwm duty */
> +	value = readl(priv->base + PWM_SUP_CONTROL0);
> +	value |= BIT(pwm->hwpwm);
> +	if (duty_ns == period_ns) {
> +		value |= BIT(pwm->hwpwm + PWM_BYPASS_BIT_SHIFT);
> +		duty = PWM_SUP_DUTY_MAX;
> +	} else {
> +		value &= ~BIT(pwm->hwpwm + PWM_BYPASS_BIT_SHIFT);
> +		tmp = (u64)duty_ns * PWM_SUP_FREQ_SCALER + (period_ns >> 1);
> +		tmp = DIV_ROUND_CLOSEST_ULL(tmp, (u64)period_ns);
> +		duty = (u32)tmp;
> +		duty |= (pwm->hwpwm << PWM_DD_SEL_BIT_SHIFT);

This is also more inexact than necessary. In general don't use period_ns
in the calculation of duty register settings. As with period you're
supposed to pick the biggest possible dutycycle not bigger than the
requested value.

Consider a PWM that with register P = P and register D = D implements a
PWM output with period = 1000 * P ns and duty_cycle = 1000 * D ns

For a request of period = 39900 and duty_cycle = 12100, you have to pick
P = 39 and D = 12. However P * duty_ns / period_ns = 11.82 ...

> +	}
> +	writel(value, priv->base + PWM_SUP_CONTROL0);
> +	writel(duty, priv->base + PWM_SUP_DUTY(pwm->hwpwm));
> +
> +	return 0;
> +}
> +
> +static void sunplus_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +				  struct pwm_state *state)
> +{
> +	struct sunplus_pwm *priv = to_sunplus_pwm(chip);
> +	u32 value;
> +
> +	value = readl(priv->base + PWM_SUP_CONTROL0);
> +
> +	if (value & BIT(pwm->hwpwm))
> +		state->enabled = true;
> +	else
> +		state->enabled = false;

This looks incomplete. Please enable PWM_DEBUG during your tests and
address all output generated by that.

As the general idea is that passing the result from .get_state() to
.apply shouldn't modify the output, you have (in general) round up
divisions in .get_state().

> +}
> +
> +static const struct pwm_ops sunplus_pwm_ops = {
> +	.apply = sunplus_pwm_apply,
> +	.get_state = sunplus_pwm_get_state,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int sunplus_pwm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct sunplus_pwm *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	priv->clk = devm_clk_get_optional(dev, NULL);
> +	if (IS_ERR(priv->clk))
> +		return dev_err_probe(dev, PTR_ERR(priv->clk),
> +				     "get pwm clock failed\n");
> +
> +	ret = clk_prepare_enable(priv->clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(dev,
> +				       (void(*)(void *))clk_disable_unprepare,
> +				       priv->clk);
> +	if (ret)
> +		return ret;
> +
> +	priv->chip.dev = dev;
> +	priv->chip.ops = &sunplus_pwm_ops;
> +	priv->chip.npwm = PWM_SUP_NUM;
> +
> +	sunplus_reg_init(priv->base);
> +
> +	platform_set_drvdata(pdev, priv);

This is unused, so please drop this.

> +
> +	ret = devm_pwmchip_add(dev, &priv->chip);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Cannot register sunplus PWM\n");
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id sunplus_pwm_of_match[] = {
> +	{ .compatible = "sunplus,sp7021-pwm", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, sunplus_pwm_of_match);
> +
> +static struct platform_driver sunplus_pwm_driver = {
> +	.probe		= sunplus_pwm_probe,
> +	.driver		= {
> +		.name	= "sunplus-pwm",
> +		.of_match_table = sunplus_pwm_of_match,
> +	},
> +};
> +module_platform_driver(sunplus_pwm_driver);
> +
> +MODULE_DESCRIPTION("Sunplus SoC PWM Driver");
> +MODULE_AUTHOR("Hammer Hsieh <hammer.hsieh@sunplus.com>");
> +MODULE_LICENSE("GPL v2");

"GPL" has the same semantic and is the more usual, so I suggest to use
that one.

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 v1 2/2] pwm:sunplus-pwm:Add Sunplus SoC PWM Driver

[hammer hsieh]

Hi: Uwe:

Thanks for your review.
Please see my response below.

Regards,
Hammer Hsieh

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 於 2021年12月17日 週五 下午11:28寫道：
>
> Hello,
>
> On Fri, Dec 17, 2021 at 07:46:08PM +0800, Hammer Hsieh wrote:
> > Add Sunplus SoC PWM Driver
> >
> > Signed-off-by: Hammer Hsieh <hammer.hsieh@sunplus.com>
> > ---
> >  MAINTAINERS               |   1 +
> >  drivers/pwm/Kconfig       |  11 +++
> >  drivers/pwm/Makefile      |   1 +
> >  drivers/pwm/pwm-sunplus.c | 192 ++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 205 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-sunplus.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 721ed79..1c9e3c5 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -18246,6 +18246,7 @@ SUNPLUS PWM DRIVER
> >  M:   Hammer Hsieh <hammer.hsieh@sunplus.com>
> >  S:   Maintained
> >  F:   Documentation/devicetree/bindings/pwm/pwm-sunplus.yaml
> > +F:   drivers/pwm/pwm-sunplus.c
> >
> >  SUPERH
> >  M:   Yoshinori Sato <ysato@users.sourceforge.jp>
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index 21e3b05..9df5d5f 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -526,6 +526,17 @@ config PWM_SPRD
> >         To compile this driver as a module, choose M here: the module
> >         will be called pwm-sprd.
> >
> > +config PWM_SUNPLUS
> > +     tristate "Sunplus PWM support"
> > +     depends on ARCH_SUNPLUS || COMPILE_TEST
> > +     depends on HAS_IOMEM && OF
> > +     help
> > +       Generic PWM framework driver for the PWM controller on
> > +       Sunplus SoCs.
> > +
> > +       To compile this driver as a module, choose M here: the module
> > +       will be called pwm-sunplus.
> > +
> >  config PWM_STI
> >       tristate "STiH4xx PWM support"
> >       depends on ARCH_STI || COMPILE_TEST
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index 708840b..be58616 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -53,6 +53,7 @@ obj-$(CONFIG_PWM_STM32)             += pwm-stm32.o
> >  obj-$(CONFIG_PWM_STM32_LP)   += pwm-stm32-lp.o
> >  obj-$(CONFIG_PWM_STMPE)              += pwm-stmpe.o
> >  obj-$(CONFIG_PWM_SUN4I)              += pwm-sun4i.o
> > +obj-$(CONFIG_PWM_SUNPLUS)    += pwm-sunplus.o
> >  obj-$(CONFIG_PWM_TEGRA)              += pwm-tegra.o
> >  obj-$(CONFIG_PWM_TIECAP)     += pwm-tiecap.o
> >  obj-$(CONFIG_PWM_TIEHRPWM)   += pwm-tiehrpwm.o
> > diff --git a/drivers/pwm/pwm-sunplus.c b/drivers/pwm/pwm-sunplus.c
> > new file mode 100644
> > index 0000000..0ae59fc
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-sunplus.c
> > @@ -0,0 +1,192 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * PWM device driver for SUNPLUS SoCs
> > + *
> > + * Author: Hammer Hsieh <hammer.hsieh@sunplus.com>
> > + */
>
> Please add a section here about your hardware limitations. Please stick
> to the format used in e.g. pwm-sifive.c. That is a block starting with
>
>  * Limitations:
>
> and then a list of issues. One such item is: Only supports normal
> polarity.
>
ok, will modify it.

> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +
> > +#define PWM_SUP_CONTROL0     0x000
> > +#define PWM_SUP_CONTROL1     0x004
> > +#define PWM_SUP_FREQ_BASE    0x008
> > +#define PWM_SUP_DUTY_BASE    0x018
> > +#define PWM_SUP_FREQ(ch)     (PWM_SUP_FREQ_BASE + 4 * (ch))
> > +#define PWM_SUP_DUTY(ch)     (PWM_SUP_DUTY_BASE + 4 * (ch))
> > +#define PWM_SUP_FREQ_MAX     GENMASK(15, 0)
> > +#define PWM_SUP_DUTY_MAX     GENMASK(7, 0)
> > +
> > +#define PWM_SUP_NUM          4
> > +#define PWM_BYPASS_BIT_SHIFT 8
> > +#define PWM_DD_SEL_BIT_SHIFT 8
> > +#define PWM_SUP_FREQ_SCALER  256
> > +
> > +struct sunplus_pwm {
> > +     struct pwm_chip chip;
> > +     void __iomem *base;
> > +     struct clk *clk;
> > +};
> > +
> > +static inline struct sunplus_pwm *to_sunplus_pwm(struct pwm_chip *chip)
> > +{
> > +     return container_of(chip, struct sunplus_pwm, chip);
> > +}
> > +
> > +static void sunplus_reg_init(void __iomem *base)
> > +{
> > +     u32 i, value;
> > +
> > +     /* turn off all pwm channel output */
> > +     value = readl(base + PWM_SUP_CONTROL0);
> > +     value &= ~GENMASK((PWM_SUP_NUM - 1), 0);
> > +     writel(value, base + PWM_SUP_CONTROL0);
> > +
> > +     /* init all pwm channel clock source */
> > +     value = readl(base + PWM_SUP_CONTROL1);
> > +     value |= GENMASK((PWM_SUP_NUM - 1), 0);
> > +     writel(value, base + PWM_SUP_CONTROL1);
> > +
> > +     /* init all freq and duty setting */
> > +     for (i = 0; i < PWM_SUP_NUM; i++) {
> > +             writel(0, base + PWM_SUP_FREQ(i));
> > +             writel(0, base + PWM_SUP_DUTY(i));
> > +     }
>
> Please keep the PWM in their boot-up state. That is, if the bootloader
> enabled a display with a bootsplash, don't disable the backlight when
> the PWM driver loads.
>

ok, will remove init reg code.

> > +}
> > +
> > +static int sunplus_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > +                          const struct pwm_state *state)
> > +{
> > +     struct sunplus_pwm *priv = to_sunplus_pwm(chip);
> > +     u32 period_ns, duty_ns, value;
> > +     u32 dd_freq, duty;
> > +     u64 tmp;
> > +
>
>         if (state->polarity != PWM_POLARITY_NORMAL)
>                 return -EINVAL;
>
> > +     if (!state->enabled) {
> > +             value = readl(priv->base + PWM_SUP_CONTROL0);
> > +             value &= ~BIT(pwm->hwpwm);
> > +             writel(value, priv->base + PWM_SUP_CONTROL0);
> > +             return 0;
> > +     }
> > +
> > +     period_ns = state->period;
>
> state->period is an u64, so you might loose precision here.
>
> > +     duty_ns = state->duty_cycle;
>
> ditto
>
> > +
> > +     /* cal pwm freq and check value under range */
> > +     tmp = clk_get_rate(priv->clk) * (u64)period_ns;
>
> This might overflow?
>
> > +     tmp = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
> > +     tmp = DIV_ROUND_CLOSEST_ULL(tmp, PWM_SUP_FREQ_SCALER);
>
> In general you should pick the highest period that isn't bigger than the
> requested period. I didn't check in detail, but using round-closest is a
> strong hint that you get that wrong.
>
> > +     dd_freq = (u32)tmp;
> > +
> > +     if (dd_freq == 0)
> > +             return -EINVAL;
> > +
> > +     if (dd_freq > PWM_SUP_FREQ_MAX)
> > +             dd_freq = PWM_SUP_FREQ_MAX;
> > +
> > +     writel(dd_freq, priv->base + PWM_SUP_FREQ(pwm->hwpwm));
> > +
> > +     /* cal and set pwm duty */
> > +     value = readl(priv->base + PWM_SUP_CONTROL0);
> > +     value |= BIT(pwm->hwpwm);
> > +     if (duty_ns == period_ns) {
> > +             value |= BIT(pwm->hwpwm + PWM_BYPASS_BIT_SHIFT);
> > +             duty = PWM_SUP_DUTY_MAX;
> > +     } else {
> > +             value &= ~BIT(pwm->hwpwm + PWM_BYPASS_BIT_SHIFT);
> > +             tmp = (u64)duty_ns * PWM_SUP_FREQ_SCALER + (period_ns >> 1);
> > +             tmp = DIV_ROUND_CLOSEST_ULL(tmp, (u64)period_ns);
> > +             duty = (u32)tmp;
> > +             duty |= (pwm->hwpwm << PWM_DD_SEL_BIT_SHIFT);
>
> This is also more inexact than necessary. In general don't use period_ns
> in the calculation of duty register settings. As with period you're
> supposed to pick the biggest possible dutycycle not bigger than the
> requested value.
>
> Consider a PWM that with register P = P and register D = D implements a
> PWM output with period = 1000 * P ns and duty_cycle = 1000 * D ns
>
> For a request of period = 39900 and duty_cycle = 12100, you have to pick
> P = 39 and D = 12. However P * duty_ns / period_ns = 11.82 ...
>

static int sunplus_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
     const struct pwm_state *state)
{
  struct sunplus_pwm *priv = to_sunplus_pwm(chip);
  u32 dd_freq, duty, value, value1;
  u64 period_ns, duty_ns, tmp;
  u64 period_ns_max;

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

  if (!state->enabled) {
  /* disable pwm channel output */
  value = readl(priv->base + PWM_SUP_CONTROL0);
  value &= ~BIT(pwm->hwpwm);
  writel(value, priv->base + PWM_SUP_CONTROL0);
  /* disable pwm channel clk source */
  value = readl(priv->base + PWM_SUP_CONTROL1);
  value &= ~BIT(pwm->hwpwm);
  writel(value, priv->base + PWM_SUP_CONTROL1);
  return 0;
  }

  tmp = PWM_SUP_FREQ_SCALER * NSEC_PER_SEC;
  tmp = DIV64_U64_ROUND_CLOSEST(tmp, clk_get_rate(priv->clk));
  period_ns_max = PWM_SUP_FREQ_MAX * tmp;

  if (state->period > period_ns_max)
    return -EINVAL;

  period_ns = state->period;
  duty_ns = state->duty_cycle;

  /* cal pwm freq and check value under range */
  tmp = DIV64_U64_ROUND_CLOSEST(clk_get_rate(priv->clk), PWM_SUP_FREQ_SCALER);
  tmp = tmp * period_ns >> 10;
  tmp = DIV64_U64_ROUND_CLOSEST(tmp, NSEC_PER_SEC >> 10);
  dd_freq = (u32)tmp;

  if (dd_freq == 0)
    return -EINVAL;

  if (dd_freq > PWM_SUP_FREQ_MAX)
    dd_freq = PWM_SUP_FREQ_MAX;

  writel(dd_freq, priv->base + PWM_SUP_FREQ(pwm->hwpwm));

  /* cal and set pwm duty */
  value = readl(priv->base + PWM_SUP_CONTROL0);
  value |= BIT(pwm->hwpwm);
  value1 = readl(priv->base + PWM_SUP_CONTROL1);
  value1 |= BIT(pwm->hwpwm);
  if (duty_ns == period_ns) {
  value |= BIT(pwm->hwpwm + PWM_BYPASS_BIT_SHIFT);
  duty = PWM_SUP_DUTY_MAX;
  } else {
  value &= ~BIT(pwm->hwpwm + PWM_BYPASS_BIT_SHIFT);
  tmp = (duty_ns >> 10) * PWM_SUP_FREQ_SCALER;
  tmp = DIV64_U64_ROUND_CLOSEST(tmp, (period_ns >> 10));
  duty = (u32)tmp;
  duty |= (pwm->hwpwm << PWM_DD_SEL_BIT_SHIFT);
  }
  writel(value, priv->base + PWM_SUP_CONTROL0);
  writel(value1, priv->base + PWM_SUP_CONTROL1);
  writel(duty, priv->base + PWM_SUP_DUTY(pwm->hwpwm));

  return 0;
}

While I turn on PWM_DEBUG.
I still can see the warning message.
"sunplus-pwm 9c007a00.pwm: .apply is not idempotent (ena=1 pol=0
9998240/19996480)->(ena=1 pol=0 9996976/19993952)
I'm not sure if it is an issue or not.
echo 20000000 > period
echo 10000000 > duty_cycle
echo 1 > enable
get_state: Calculate reg value to state->period and state->duty_cycle.
apply: Calculate state->period and state->duty_cycle to reg value.
Can't match always.

> > +     }
> > +     writel(value, priv->base + PWM_SUP_CONTROL0);
> > +     writel(duty, priv->base + PWM_SUP_DUTY(pwm->hwpwm));
> > +
> > +     return 0;
> > +}
> > +
> > +static void sunplus_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > +                               struct pwm_state *state)
> > +{
> > +     struct sunplus_pwm *priv = to_sunplus_pwm(chip);
> > +     u32 value;
> > +
> > +     value = readl(priv->base + PWM_SUP_CONTROL0);
> > +
> > +     if (value & BIT(pwm->hwpwm))
> > +             state->enabled = true;
> > +     else
> > +             state->enabled = false;
>
> This looks incomplete. Please enable PWM_DEBUG during your tests and
> address all output generated by that.
>
> As the general idea is that passing the result from .get_state() to
> .apply shouldn't modify the output, you have (in general) round up
> divisions in .get_state().

static void sunplus_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
  struct pwm_state *state)
{
  struct sunplus_pwm *priv = to_sunplus_pwm(chip);
  u32 value, freq, duty;
  u64 tmp, rate;

  rate = clk_get_rate(priv->clk);
  value = readl(priv->base + PWM_SUP_CONTROL0);
  freq = readl(priv->base + PWM_SUP_FREQ(pwm->hwpwm));
  duty = readl(priv->base + PWM_SUP_DUTY(pwm->hwpwm));
  duty &= ~GENMASK(9,8);

  tmp = PWM_SUP_FREQ_SCALER * NSEC_PER_SEC;
  tmp = DIV64_U64_ROUND_CLOSEST(tmp, rate);
  state->period = (u64)freq * tmp;
  tmp = (u64)duty * state->period;
  state->duty_cycle = DIV64_U64_ROUND_CLOSEST(tmp, PWM_SUP_FREQ_SCALER);

  if (value & BIT(pwm->hwpwm))
    state->enabled = true;
  else
    state->enabled = false;

  state->polarity = PWM_POLARITY_NORMAL;
}

>
> > +}
> > +
> > +static const struct pwm_ops sunplus_pwm_ops = {
> > +     .apply = sunplus_pwm_apply,
> > +     .get_state = sunplus_pwm_get_state,
> > +     .owner = THIS_MODULE,
> > +};
> > +
> > +static int sunplus_pwm_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct sunplus_pwm *priv;
> > +     int ret;
> > +
> > +     priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +     if (!priv)
> > +             return -ENOMEM;
> > +
> > +     priv->base = devm_platform_ioremap_resource(pdev, 0);
> > +     if (IS_ERR(priv->base))
> > +             return PTR_ERR(priv->base);
> > +
> > +     priv->clk = devm_clk_get_optional(dev, NULL);
> > +     if (IS_ERR(priv->clk))
> > +             return dev_err_probe(dev, PTR_ERR(priv->clk),
> > +                                  "get pwm clock failed\n");
> > +
> > +     ret = clk_prepare_enable(priv->clk);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = devm_add_action_or_reset(dev,
> > +                                    (void(*)(void *))clk_disable_unprepare,
> > +                                    priv->clk);
> > +     if (ret)
> > +             return ret;
> > +
> > +     priv->chip.dev = dev;
> > +     priv->chip.ops = &sunplus_pwm_ops;
> > +     priv->chip.npwm = PWM_SUP_NUM;
> > +
> > +     sunplus_reg_init(priv->base);
> > +
> > +     platform_set_drvdata(pdev, priv);
>
> This is unused, so please drop this.
>

ok, will modify it.

> > +
> > +     ret = devm_pwmchip_add(dev, &priv->chip);
> > +     if (ret < 0)
> > +             return dev_err_probe(dev, ret, "Cannot register sunplus PWM\n");
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct of_device_id sunplus_pwm_of_match[] = {
> > +     { .compatible = "sunplus,sp7021-pwm", },
> > +     {}
> > +};
> > +MODULE_DEVICE_TABLE(of, sunplus_pwm_of_match);
> > +
> > +static struct platform_driver sunplus_pwm_driver = {
> > +     .probe          = sunplus_pwm_probe,
> > +     .driver         = {
> > +             .name   = "sunplus-pwm",
> > +             .of_match_table = sunplus_pwm_of_match,
> > +     },
> > +};
> > +module_platform_driver(sunplus_pwm_driver);
> > +
> > +MODULE_DESCRIPTION("Sunplus SoC PWM Driver");
> > +MODULE_AUTHOR("Hammer Hsieh <hammer.hsieh@sunplus.com>");
> > +MODULE_LICENSE("GPL v2");
>
> "GPL" has the same semantic and is the more usual, so I suggest to use
> that one.
>
ok, will modify it.


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

Re: [PATCH v1 2/2] pwm:sunplus-pwm:Add Sunplus SoC PWM Driver

[Uwe Kleine-König]

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

Hello,

On Tue, Dec 21, 2021 at 02:28:24PM +0800, hammer hsieh wrote:
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 於 2021年12月17日 週五
> 下午11:28寫道：
> 
> > On Fri, Dec 17, 2021 at 07:46:08PM +0800, Hammer Hsieh wrote:
> > > Add Sunplus SoC PWM Driver
> > >
> > > Signed-off-by: Hammer Hsieh <hammer.hsieh@sunplus.com>
> > > ---
> > >  MAINTAINERS               |   1 +
> > >  drivers/pwm/Kconfig       |  11 +++
> > >  drivers/pwm/Makefile      |   1 +
> > >  drivers/pwm/pwm-sunplus.c | 192
> > ++++++++++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 205 insertions(+)
> > >  create mode 100644 drivers/pwm/pwm-sunplus.c
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 721ed79..1c9e3c5 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -18246,6 +18246,7 @@ SUNPLUS PWM DRIVER
> > >  M:   Hammer Hsieh <hammer.hsieh@sunplus.com>
> > >  S:   Maintained
> > >  F:   Documentation/devicetree/bindings/pwm/pwm-sunplus.yaml
> > > +F:   drivers/pwm/pwm-sunplus.c
> > >
> > >  SUPERH
> > >  M:   Yoshinori Sato <ysato@users.sourceforge.jp>
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > index 21e3b05..9df5d5f 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -526,6 +526,17 @@ config PWM_SPRD
> > >         To compile this driver as a module, choose M here: the module
> > >         will be called pwm-sprd.
> > >
> > > +config PWM_SUNPLUS
> > > +     tristate "Sunplus PWM support"
> > > +     depends on ARCH_SUNPLUS || COMPILE_TEST
> > > +     depends on HAS_IOMEM && OF
> > > +     help
> > > +       Generic PWM framework driver for the PWM controller on
> > > +       Sunplus SoCs.
> > > +
> > > +       To compile this driver as a module, choose M here: the module
> > > +       will be called pwm-sunplus.
> > > +
> > >  config PWM_STI
> > >       tristate "STiH4xx PWM support"
> > >       depends on ARCH_STI || COMPILE_TEST
> > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > > index 708840b..be58616 100644
> > > --- a/drivers/pwm/Makefile
> > > +++ b/drivers/pwm/Makefile
> > > @@ -53,6 +53,7 @@ obj-$(CONFIG_PWM_STM32)             += pwm-stm32.o
> > >  obj-$(CONFIG_PWM_STM32_LP)   += pwm-stm32-lp.o
> > >  obj-$(CONFIG_PWM_STMPE)              += pwm-stmpe.o
> > >  obj-$(CONFIG_PWM_SUN4I)              += pwm-sun4i.o
> > > +obj-$(CONFIG_PWM_SUNPLUS)    += pwm-sunplus.o
> > >  obj-$(CONFIG_PWM_TEGRA)              += pwm-tegra.o
> > >  obj-$(CONFIG_PWM_TIECAP)     += pwm-tiecap.o
> > >  obj-$(CONFIG_PWM_TIEHRPWM)   += pwm-tiehrpwm.o
> > > diff --git a/drivers/pwm/pwm-sunplus.c b/drivers/pwm/pwm-sunplus.c
> > > new file mode 100644
> > > index 0000000..0ae59fc
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-sunplus.c
> > > @@ -0,0 +1,192 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * PWM device driver for SUNPLUS SoCs
> > > + *
> > > + * Author: Hammer Hsieh <hammer.hsieh@sunplus.com>
> > > + */
> >
> > Please add a section here about your hardware limitations. Please stick
> > to the format used in e.g. pwm-sifive.c. That is a block starting with
> >
> >  * Limitations:
> >
> > and then a list of issues. One such item is: Only supports normal
> > polarity.
>
> ok, will add it.

Can you please fix your mailer to properly quote and mark your text
accordingly? (I fixed it up, so the above looks right, in your mail
however it doesn't.)

> > > +#include <linux/clk.h>
> > > +#include <linux/io.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/pwm.h>
> > > +
> > > +#define PWM_SUP_CONTROL0     0x000
> > > +#define PWM_SUP_CONTROL1     0x004
> > > +#define PWM_SUP_FREQ_BASE    0x008
> > > +#define PWM_SUP_DUTY_BASE    0x018
> > > +#define PWM_SUP_FREQ(ch)     (PWM_SUP_FREQ_BASE + 4 * (ch))
> > > +#define PWM_SUP_DUTY(ch)     (PWM_SUP_DUTY_BASE + 4 * (ch))
> > > +#define PWM_SUP_FREQ_MAX     GENMASK(15, 0)
> > > +#define PWM_SUP_DUTY_MAX     GENMASK(7, 0)
> > > +
> > > +#define PWM_SUP_NUM          4
> > > +#define PWM_BYPASS_BIT_SHIFT 8
> > > +#define PWM_DD_SEL_BIT_SHIFT 8
> > > +#define PWM_SUP_FREQ_SCALER  256
> > > +
> > > +struct sunplus_pwm {
> > > +     struct pwm_chip chip;
> > > +     void __iomem *base;
> > > +     struct clk *clk;
> > > +};
> > > +
> > > +static inline struct sunplus_pwm *to_sunplus_pwm(struct pwm_chip *chip)
> > > +{
> > > +     return container_of(chip, struct sunplus_pwm, chip);
> > > +}
> > > +
> > > +static void sunplus_reg_init(void __iomem *base)
> > > +{
> > > +     u32 i, value;
> > > +
> > > +     /* turn off all pwm channel output */
> > > +     value = readl(base + PWM_SUP_CONTROL0);
> > > +     value &= ~GENMASK((PWM_SUP_NUM - 1), 0);
> > > +     writel(value, base + PWM_SUP_CONTROL0);
> > > +
> > > +     /* init all pwm channel clock source */
> > > +     value = readl(base + PWM_SUP_CONTROL1);
> > > +     value |= GENMASK((PWM_SUP_NUM - 1), 0);
> > > +     writel(value, base + PWM_SUP_CONTROL1);
> > > +
> > > +     /* init all freq and duty setting */
> > > +     for (i = 0; i < PWM_SUP_NUM; i++) {
> > > +             writel(0, base + PWM_SUP_FREQ(i));
> > > +             writel(0, base + PWM_SUP_DUTY(i));
> > > +     }
> >
> > Please keep the PWM in their boot-up state. That is, if the bootloader
> > enabled a display with a bootsplash, don't disable the backlight when
> > the PWM driver loads.
>
> ok, will remove the init register code.
> 
> 
> > > +}
> > > +
> > > +static int sunplus_pwm_apply(struct pwm_chip *chip, struct pwm_device
> > *pwm,
> > > +                          const struct pwm_state *state)
> > > +{
> > > +     struct sunplus_pwm *priv = to_sunplus_pwm(chip);
> > > +     u32 period_ns, duty_ns, value;
> > > +     u32 dd_freq, duty;
> > > +     u64 tmp;
> > > +
> >
> >         if (state->polarity != PWM_POLARITY_NORMAL)
> >                 return -EINVAL;
> >
> > > +     if (!state->enabled) {
> > > +             value = readl(priv->base + PWM_SUP_CONTROL0);
> > > +             value &= ~BIT(pwm->hwpwm);
> > > +             writel(value, priv->base + PWM_SUP_CONTROL0);
> > > +             return 0;
> > > +     }
> > > +
> > > +     period_ns = state->period;
> >
> > state->period is an u64, so you might loose precision here.
> >
> > > +     duty_ns = state->duty_cycle;
> >
> > ditto
> >
> > > +
> > > +     /* cal pwm freq and check value under range */
> > > +     tmp = clk_get_rate(priv->clk) * (u64)period_ns;
> >
> > This might overflow?
> >
> > > +     tmp = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
> > > +     tmp = DIV_ROUND_CLOSEST_ULL(tmp, PWM_SUP_FREQ_SCALER);
> >
> > In general you should pick the highest period that isn't bigger than the
> > requested period. I didn't check in detail, but using round-closest is a
> > strong hint that you get that wrong.
> >
> > > +     dd_freq = (u32)tmp;
> > > +
> > > +     if (dd_freq == 0)
> > > +             return -EINVAL;
> > > +
> > > +     if (dd_freq > PWM_SUP_FREQ_MAX)
> > > +             dd_freq = PWM_SUP_FREQ_MAX;
> > > +
> > > +     writel(dd_freq, priv->base + PWM_SUP_FREQ(pwm->hwpwm));
> > > +
> > > +     /* cal and set pwm duty */
> > > +     value = readl(priv->base + PWM_SUP_CONTROL0);
> > > +     value |= BIT(pwm->hwpwm);
> > > +     if (duty_ns == period_ns) {
> > > +             value |= BIT(pwm->hwpwm + PWM_BYPASS_BIT_SHIFT);
> > > +             duty = PWM_SUP_DUTY_MAX;
> > > +     } else {
> > > +             value &= ~BIT(pwm->hwpwm + PWM_BYPASS_BIT_SHIFT);
> > > +             tmp = (u64)duty_ns * PWM_SUP_FREQ_SCALER + (period_ns >>
> > 1);
> > > +             tmp = DIV_ROUND_CLOSEST_ULL(tmp, (u64)period_ns);
> > > +             duty = (u32)tmp;
> > > +             duty |= (pwm->hwpwm << PWM_DD_SEL_BIT_SHIFT);
> >
> > This is also more inexact than necessary. In general don't use period_ns
> > in the calculation of duty register settings. As with period you're
> > supposed to pick the biggest possible dutycycle not bigger than the
> > requested value.
> >
> > Consider a PWM that with register P = P and register D = D implements a
> > PWM output with period = 1000 * P ns and duty_cycle = 1000 * D ns
> >
> > For a request of period = 39900 and duty_cycle = 12100, you have to pick
> > P = 39 and D = 12. However P * duty_ns / period_ns = 11.82 ...
> >
> >
> static int sunplus_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>     const struct pwm_state *state)
> {
>   struct sunplus_pwm *priv = to_sunplus_pwm(chip);
>   u32 dd_freq, duty, value, value1;
>   u64 period_ns, duty_ns, tmp;
>   u64 period_ns_max;
> 
>   if (state->polarity != pwm->state.polarity)
>     return -EINVAL;
> 
>   if (!state->enabled) {
>   /* disable pwm channel output */
>   value = readl(priv->base + PWM_SUP_CONTROL0);
>   value &= ~BIT(pwm->hwpwm);
>   writel(value, priv->base + PWM_SUP_CONTROL0);
>   /* disable pwm channel clk source */
>   value = readl(priv->base + PWM_SUP_CONTROL1);
>   value &= ~BIT(pwm->hwpwm);
>   writel(value, priv->base + PWM_SUP_CONTROL1);
>   return 0;
>   }
> 
>   tmp = PWM_SUP_FREQ_SCALER * NSEC_PER_SEC;
>   tmp = DIV64_U64_ROUND_CLOSEST(tmp, clk_get_rate(priv->clk));
>   period_ns_max = PWM_SUP_FREQ_MAX * tmp;
> 
>   if (state->period > period_ns_max)
>   return -EINVAL;
> 
>   period_ns = state->period;
>   duty_ns = state->duty_cycle;
> 
>  /* cal pwm freq and check value under range */
>   tmp = DIV64_U64_ROUND_CLOSEST(clk_get_rate(priv->clk),
> PWM_SUP_FREQ_SCALER);
>   tmp = tmp * period_ns >> 10;
>   tmp = DIV64_U64_ROUND_CLOSEST(tmp, NSEC_PER_SEC >> 10);
>   dd_freq = (u32)tmp;
> 
>   if (dd_freq == 0)
>   return -EINVAL;
> 
>   if (dd_freq > PWM_SUP_FREQ_MAX)
>     dd_freq = PWM_SUP_FREQ_MAX;
> 
>   writel(dd_freq, priv->base + PWM_SUP_FREQ(pwm->hwpwm));
> 
>   /* cal and set pwm duty */
>   value = readl(priv->base + PWM_SUP_CONTROL0);
>   value |= BIT(pwm->hwpwm);
>   value1 = readl(priv->base + PWM_SUP_CONTROL1);
>   value1 |= BIT(pwm->hwpwm);
>   if (duty_ns == period_ns) {
>     value |= BIT(pwm->hwpwm + PWM_BYPASS_BIT_SHIFT);
>     duty = PWM_SUP_DUTY_MAX;
>   } else {
>   value &= ~BIT(pwm->hwpwm + PWM_BYPASS_BIT_SHIFT);
>   tmp = (duty_ns >> 10) * PWM_SUP_FREQ_SCALER;
>   tmp = DIV64_U64_ROUND_CLOSEST(tmp, (period_ns >> 10));
>   duty = (u32)tmp;
>   duty |= (pwm->hwpwm << PWM_DD_SEL_BIT_SHIFT);
>   }
>   writel(value, priv->base + PWM_SUP_CONTROL0);
>   writel(value1, priv->base + PWM_SUP_CONTROL1);
>   writel(duty, priv->base + PWM_SUP_DUTY(pwm->hwpwm));
> 
>   return 0;
> }

This is badly indented, I'm delaying review until the next patch.

> while turn on PWM_DEBUG.
> I still get warning message like
> sunplus-pwm 9c007a00.pwm: .apply is not idempotent (ena=1 pol=0
> 9998240/19996480)->(ena=1 pol=0 9996976/19993952)

After you applied a setting the debug code calls .get_state and applies
its result. The problem is that applying
duty_cycle = 9998240 + period = 19996480 yields duty_cycle = 9996976 +
period = 19993952 though there should be an exact match.

There is still a ROUND_CLOSEST in your .apply callback ... that probably
cannot implement "pick the biggest possible period not bigger than the
rquested period".

> echo 20000000 > period
> echo 10000000 > duty_cycle
> echo 1 > enable
> I'm not sure it's a issue or not?
> get_state calculate reg value to state->period and state->duty_cycle.
> apply calculate state->period and state->duty_cycle to reg value.
> Can't match always.

Yes, it's not a 1:1 relation. If however applying

	period = $pa
	duty_cycle = $da

yields a state with

	period = $pr
	duty_cycle = $dr

then applying $dr / $pr should give an exact match.

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 v1 1/2] dt-bindings:pwm:Add bindings doc for Sunplus SoC PWM Driver

[Rob Herring]

On Fri, Dec 17, 2021 at 07:46:07PM +0800, Hammer Hsieh wrote:
> Add bindings doc for Sunplus SoC PWM Driver
> 
> Signed-off-by: Hammer Hsieh <hammer.hsieh@sunplus.com>

The author email and S-o-b must match.

> ---
>  .../devicetree/bindings/pwm/pwm-sunplus.yaml       | 45 ++++++++++++++++++++++
>  MAINTAINERS                                        |  5 +++
>  2 files changed, 50 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sunplus.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-sunplus.yaml b/Documentation/devicetree/bindings/pwm/pwm-sunplus.yaml
> new file mode 100644
> index 0000000..9af19df
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-sunplus.yaml
> @@ -0,0 +1,45 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) Sunplus Co., Ltd. 2021
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pwm/pwm-sunplus.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sunplus SoC PWM Controller
> +
> +maintainers:
> +  - Hammer Hsieh <hammer.hsieh@sunplus.com>
> +
> +properties:
> +  '#pwm-cells':
> +    const: 2
> +
> +  compatible:
> +    items:
> +      - const: sunplus,sp7021-pwm
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +required:
> +  - '#pwm-cells'
> +  - compatible
> +  - reg
> +  - clocks
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    pwm: pwm@9c007a00 {
> +      #pwm-cells = <2>;
> +      compatible = "sunplus,sp7021-pwm";
> +      reg = <0x9c007a00 0x80>;
> +      clocks = <&clkc 0xa2>;
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 13f9a84..721ed79 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18242,6 +18242,11 @@ L:	netdev@vger.kernel.org
>  S:	Maintained
>  F:	drivers/net/ethernet/dlink/sundance.c
>  
> +SUNPLUS PWM DRIVER
> +M:	Hammer Hsieh <hammer.hsieh@sunplus.com>
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/pwm/pwm-sunplus.yaml
> +
>  SUPERH
>  M:	Yoshinori Sato <ysato@users.sourceforge.jp>
>  M:	Rich Felker <dalias@libc.org>
> -- 
> 2.7.4
> 
> 

Re: [PATCH v1 1/2] dt-bindings:pwm:Add bindings doc for Sunplus SoC PWM Driver

[hammer hsieh]

Hi, Rob Herring:

I will fix it in next submit patch.

Regards,
Hammer Hsieh


Rob Herring <robh@kernel.org> 於 2021年12月22日 週三 上午2:14寫道：
>
> On Fri, Dec 17, 2021 at 07:46:07PM +0800, Hammer Hsieh wrote:
> > Add bindings doc for Sunplus SoC PWM Driver
> >
> > Signed-off-by: Hammer Hsieh <hammer.hsieh@sunplus.com>
>
> The author email and S-o-b must match.
>
> > ---
> >  .../devicetree/bindings/pwm/pwm-sunplus.yaml       | 45 ++++++++++++++++++++++
> >  MAINTAINERS                                        |  5 +++
> >  2 files changed, 50 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sunplus.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/pwm/pwm-sunplus.yaml b/Documentation/devicetree/bindings/pwm/pwm-sunplus.yaml
> > new file mode 100644
> > index 0000000..9af19df
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-sunplus.yaml
> > @@ -0,0 +1,45 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +# Copyright (C) Sunplus Co., Ltd. 2021
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pwm/pwm-sunplus.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Sunplus SoC PWM Controller
> > +
> > +maintainers:
> > +  - Hammer Hsieh <hammer.hsieh@sunplus.com>
> > +
> > +properties:
> > +  '#pwm-cells':
> > +    const: 2
> > +
> > +  compatible:
> > +    items:
> > +      - const: sunplus,sp7021-pwm
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  resets:
> > +    maxItems: 1
> > +
> > +required:
> > +  - '#pwm-cells'
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    pwm: pwm@9c007a00 {
> > +      #pwm-cells = <2>;
> > +      compatible = "sunplus,sp7021-pwm";
> > +      reg = <0x9c007a00 0x80>;
> > +      clocks = <&clkc 0xa2>;
> > +    };
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 13f9a84..721ed79 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -18242,6 +18242,11 @@ L:   netdev@vger.kernel.org
> >  S:   Maintained
> >  F:   drivers/net/ethernet/dlink/sundance.c
> >
> > +SUNPLUS PWM DRIVER
> > +M:   Hammer Hsieh <hammer.hsieh@sunplus.com>
> > +S:   Maintained
> > +F:   Documentation/devicetree/bindings/pwm/pwm-sunplus.yaml
> > +
> >  SUPERH
> >  M:   Yoshinori Sato <ysato@users.sourceforge.jp>
> >  M:   Rich Felker <dalias@libc.org>
> > --
> > 2.7.4
> >
> >