Hello,

On Mon, Nov 26, 2018 at 10:31:58PM +0100, Uwe Kleine-König wrote:

On Mon, Nov 26, 2018 at 12:23:19AM +0800, Hao Zhang wrote:

quoted

The sun8i R40/T3/V40 PWM has 8 PWM channals and divides to 4 PWM pairs,
each PWM pair built-in 1 clock module, 2 timer logic module and 1
programmable dead-time generator, it also support waveform capture.
It has 2 clock sources OSC24M and APB1, it is different with the
sun4i-pwm driver, Therefore add a new driver for it.

Signed-off-by: Hao Zhang <redacted>
---
 drivers/pwm/Kconfig     |  12 +-
 drivers/pwm/Makefile    |   1 +
 drivers/pwm/pwm-sun8i.c | 418 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 430 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pwm/pwm-sun8i.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 504d252..6105ac8 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig

@@ -426,7 +426,7 @@ config PWM_STMPE
 	  expanders.
 
 config PWM_SUN4I
-	tristate "Allwinner PWM support"
+	tristate "Allwinner SUN4I PWM support"
 	depends on ARCH_SUNXI || COMPILE_TEST
 	depends on HAS_IOMEM && COMMON_CLK
 	help

@@ -435,6 +435,16 @@ config PWM_SUN4I
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-sun4i.
 
+config PWM_SUN8I
+	tristate "Allwinner SUN8I (R40/V40/T3) PWM support"
+	depends on ARCH_SUNXI || COMPILE_TEST
+	depends on HAS_IOMEM && COMMON_CLK
+	help
+	  Generic PWM framework driver for Allwinner R40/V40/T3 SoCs.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-sun8i.
+
 config PWM_TEGRA
 	tristate "NVIDIA Tegra PWM support"
 	depends on ARCH_TEGRA

diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 9c676a0..32c8d2d 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile

@@ -43,6 +43,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_SUN8I)		+= pwm-sun8i.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-sun8i.c b/drivers/pwm/pwm-sun8i.c
new file mode 100644
index 0000000..d8597e4
--- /dev/null
+++ b/drivers/pwm/pwm-sun8i.c

@@ -0,0 +1,418 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 Hao Zhang <hao5781286@gmail.com>

Given that the documentation is publically available, I suggest to add a
link to it in a comment here.
(http://linux-sunxi.org/File:Allwinner_R40_User_Manual_V1.0.pdf)

quoted

+	/* calculate and set prescaler, div table, PWM entire cycle */
+	clk_div = val;
+	while (clk_div > 65535) {
+		prescaler++;
+		clk_div = val;
+		do_div(clk_div, 1U << div_id);
+		do_div(clk_div, prescaler + 1);
+
+		if (prescaler == 255) {
+			prescaler = 0;
+			div_id++;
+			if (div_id == 9) {
+				dev_err(sun8i_pwm->chip.dev,
+					"unsupport period value\n");
+				return -EINVAL;
+			}
+		}
+	}

Noting the underlying formula for the calculation and the bitwidth for
the related register fields above would be good.

quoted

+
+	sun8i_pwm_set_value(sun8i_pwm, PWM_PERIOD_REG(ch),
+			    PWM_ENTIRE_CYCLE, clk_div << 16);
+	sun8i_pwm_set_value(sun8i_pwm, PWM_CTR_REG(ch),
+			    PWM_PRESCAL_K, prescaler << 0);
+	sun8i_pwm_set_value(sun8i_pwm, CLK_CFG_REG(ch),
+			    CLK_DIV_M, div_id << 0);
+
+	/* set duty cycle */
+	val = state->period;
+	do_div(val, clk_div);
+	clk_div = state->duty_cycle;
+	do_div(clk_div, val);
+	if (clk_div > 65535)
+		clk_div = 65535;
+
+	sun8i_pwm_set_value(sun8i_pwm, PWM_PERIOD_REG(ch),
+			    PWM_ACT_CYCLE, clk_div << 0);

Why "<< 0"?

quoted

+	return 0;
+}
+
+static int sun8i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			   struct pwm_state *state)
+{
+	int ret;
+	struct sun8i_pwm_chip *sun8i_pwm = to_sun8i_pwm_chip(chip);
+	struct pwm_state cstate;
+
+	pwm_get_state(pwm, &cstate);
+	if (!cstate.enabled) {
+		ret = clk_prepare_enable(sun8i_pwm->clk);
+		if (ret) {
+			dev_err(chip->dev, "Failed to enable PWM clock\n");
+			return ret;
+		}
+	}
+
+	if ((cstate.period != state->period) ||
+	    (cstate.duty_cycle != state->duty_cycle)) {
+		ret = sun8i_pwm_config(sun8i_pwm, pwm->hwpwm, state);
+		if (ret) {
+			dev_err(chip->dev, "Failed to config PWM\n");
+			return ret;
+		}
+	}

sun8i_pwm_config writes the registers that are relevant for period and
duty_cycle. When do these values take effect? If it's already here,
switching the polarity below might introduce a glitch.

I think this is the case after taking a look into the reference manual.

There are two 16 bit fields in the PWM_PERIOD_REG. One specifies the
number of clock ticks defining the period ("PWM_ENTIRE_CYCLE") and the
other the duty cycle ("PWM_ACT_CYCLE").

So if you go from duty_cycle=5 + period=10 + POLARITY_NORMAL to
duty_cycle=3 + period=10 + POLARITY_INVERTED this might generate:

 ____      __           ______
/    \____/  \_________/      \__/
^         ^         ^         ^

Also there is a PWM_PERIOD_RDY bit field that probably has to be
consulted before writing to the PWM_PERIOD_REG register.

It's not entirely clear to me if the PWM_ACT_STA bit that is used for
inversion is shadowed until the next period, too. That's what I assumed
above. If it's not the wave might look as follows:

 ____      __  _____    ______
/    \____/  \/     \__/      \__/
^         ^   *     ^         ^

Where * marks the point where the inversion starts to take effect.

Best regards
Uwe

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