[PATCH 0/4] pwm: lpc32xx: fixes in the LPC32xx PWM driver

[PATCH 0/4] pwm: lpc32xx: fixes in the LPC32xx PWM driver

[Vladimir Zapolskiy]

The changeset fixes a number of issues within current implementation
of LPC32xx PWM controller driver, namely
- the SoC has two independent PWM controllers with one channel each,
- runtime warnings from common clock framework,
- overflow in duty cycle calculation may result in disabled PWM,
- unsupported period values are silently accepted.

Vladimir Zapolskiy (4):
  pwm: lpc32xx: correct number of PWM channels from 2 to 1
  pwm: lpc32xx: fix warnings from common clock framework
  pwm: lpc32xx: fix and simplify duty cycle and period calculations
  pwm: lpc32xx: return ERANGE, if requested period is not supported

 drivers/pwm/pwm-lpc32xx.c | 59 ++++++++++++++++++-----------------------------
 1 file changed, 22 insertions(+), 37 deletions(-)

-- 
2.1.4

[PATCH 1/4] pwm: lpc32xx: correct number of PWM channels from 2 to 1

[Vladimir Zapolskiy]

LPC32xx SoC has two independent PWM controllers, they have different
clock parents, clock gates and even slightly different controls, and
each of these two PWM controllers has one output channel. Due to
almost similar controls arranged in a row it is incorrectly set that
there is one PWM controller with two channels, fix this problem, which
at the moment prevents separate configuration of different clock
parents and gates for both PWM controllers.

Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
 drivers/pwm/pwm-lpc32xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-lpc32xx.c b/drivers/pwm/pwm-lpc32xx.c
index 9fde60c..ce8ab20 100644
--- a/drivers/pwm/pwm-lpc32xx.c
+++ b/drivers/pwm/pwm-lpc32xx.c
@@ -134,7 +134,7 @@ static int lpc32xx_pwm_probe(struct platform_device *pdev)
 
 	lpc32xx->chip.dev = &pdev->dev;
 	lpc32xx->chip.ops = &lpc32xx_pwm_ops;
-	lpc32xx->chip.npwm = 2;
+	lpc32xx->chip.npwm = 1;
 	lpc32xx->chip.base = -1;
 
 	ret = pwmchip_add(&lpc32xx->chip);
-- 
2.1.4

[PATCH 2/4] pwm: lpc32xx: fix warnings from common clock framework

[Vladimir Zapolskiy]

As a preparatory change for switching LPC32xx mach support to common
clock framework fix clk_enable/clk_disable calls without matching
clk_prepare/clk_unprepare.

If the driver is used on a machine with common clock framework,
the following warning is generated and it is fixed by this change:

    # echo 1 > /sys/bus/platform/drivers/lpc32xx-pwm/4005c000.pwm/pwm/pwmchip0/pwm0/enable
    ------------[ cut here ]------------
    WARNING: CPU: 0 PID: 701 at drivers/clk/clk.c:727 clk_core_enable+0x2c/0xa4()
    Modules linked in: sc16is7xx
    CPU: 0 PID: 701 Comm: sh Tainted: G        W       4.3.0-rc2+ #171
    Hardware name: LPC32XX SoC (Flattened Device Tree)
    Backtrace:
    [<>] (dump_backtrace) from [<>] (show_stack+0x18/0x1c)
    [<>] (show_stack) from [<>] (dump_stack+0x20/0x28)
    [<>] (dump_stack) from [<>] (warn_slowpath_common+0x90/0xb8)
    [<>] (warn_slowpath_common) from [<>] (warn_slowpath_null+0x24/0x2c)
    [<>] (warn_slowpath_null) from [<>] (clk_core_enable+0x2c/0xa4)
    [<>] (clk_core_enable) from [<>] (clk_enable+0x24/0x38)
    [<>] (clk_enable) from [<>] (lpc32xx_pwm_enable+0x1c/0x40)
    [<>] (lpc32xx_pwm_enable) from [<>] (pwm_enable+0x48/0x5c)
    [<>] (pwm_enable) from [<>] (pwm_enable_store+0x5c/0x78)
    [<>] (pwm_enable_store) from [<>] (dev_attr_store+0x20/0x2c)
    [<>] (dev_attr_store) from [<>] (sysfs_kf_write+0x44/0x50)
    [<>] (sysfs_kf_write) from [<>] (kernfs_fop_write+0x134/0x194)
    [<>] (kernfs_fop_write) from [<>] (__vfs_write+0x34/0xdc)
    [<>] (__vfs_write) from [<>] (vfs_write+0xb8/0x140)
    [<>] (vfs_write) from [<>] (SyS_write+0x50/0x90)
    [<>] (SyS_write) from [<>] (ret_fast_syscall+0x0/0x38)

Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
 drivers/pwm/pwm-lpc32xx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-lpc32xx.c b/drivers/pwm/pwm-lpc32xx.c
index ce8ab20..63468a8 100644
--- a/drivers/pwm/pwm-lpc32xx.c
+++ b/drivers/pwm/pwm-lpc32xx.c
@@ -83,7 +83,7 @@ static int lpc32xx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 	u32 val;
 	int ret;
 
-	ret = clk_enable(lpc32xx->clk);
+	ret = clk_prepare_enable(lpc32xx->clk);
 	if (ret)
 		return ret;
 
@@ -103,7 +103,7 @@ static void lpc32xx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	val &= ~PWM_ENABLE;
 	writel(val, lpc32xx->base + (pwm->hwpwm << 2));
 
-	clk_disable(lpc32xx->clk);
+	clk_disable_unprepare(lpc32xx->clk);
 }
 
 static const struct pwm_ops lpc32xx_pwm_ops = {
-- 
2.1.4

[PATCH 3/4] pwm: lpc32xx: fix and simplify duty cycle and period calculations

[Vladimir Zapolskiy]

The change fixes a problem, if duty_ns is too small in comparison
to period_ns (as a valid corner case duty_ns is 0 ns), then due to
PWM_DUTY() macro applied on a value the result is overflowed over 8
bits, and instead of the highest bitfield duty cycle value 0xff the
invalid duty cycle bitfield value 0x00 is written.

For reference the LPC32xx spec defines PWMx_DUTY bitfield description
is this way and it seems to be correct:

 [Low]/[High] = [PWM_DUTY]/[256-PWM_DUTY], where 0 < PWM_DUTY <= 255.

In addition according to my oscilloscope measurements LPC32xx PWM is
"tristate" in sense that it produces a wave with floating min/max
voltage levels for different duty cycle values, for corner cases:

  PWM_DUTY == 0x01 => signal is in range from -1.05v to 0v
  ....
  PWM_DUTY == 0x80 => signal is in range from -0.75v to +0.75v
  ....
  PWM_DUTY == 0xff => signal is in range from 0v to +1.05v

  PWM_DUTY == 0x00 => signal is around 0v, PWM is off

Due to this peculiarity on very long period ranges (less than 1KHz)
and odd pre-divider values PWM generated wave does not remind a
clock shape signal, but rather a heartbit shape signal with positive
and negative peaks, so I would recommend to use high-speed HCLK clock
as a PWM parent clock and avoid using RTC clock as a parent.

The change corrects PWM output in corner cases and prevents any
possible overflows in calculation of values for PWM_DUTY and
PWM_RELOADV bitfields, thus helper macro definitions may be removed.

Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
 drivers/pwm/pwm-lpc32xx.c | 53 +++++++++++++++++------------------------------
 1 file changed, 19 insertions(+), 34 deletions(-)

diff --git a/drivers/pwm/pwm-lpc32xx.c b/drivers/pwm/pwm-lpc32xx.c
index 63468a8..294a68f 100644
--- a/drivers/pwm/pwm-lpc32xx.c
+++ b/drivers/pwm/pwm-lpc32xx.c
@@ -24,9 +24,7 @@ struct lpc32xx_pwm_chip {
 	void __iomem *base;
 };
 
-#define PWM_ENABLE	(1 << 31)
-#define PWM_RELOADV(x)	(((x) & 0xFF) << 8)
-#define PWM_DUTY(x)	((x) & 0xFF)
+#define PWM_ENABLE	BIT(31)
 
 #define to_lpc32xx_pwm_chip(_chip) \
 	container_of(_chip, struct lpc32xx_pwm_chip, chip)
@@ -38,40 +36,27 @@ static int lpc32xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	unsigned long long c;
 	int period_cycles, duty_cycles;
 	u32 val;
-
-	c = clk_get_rate(lpc32xx->clk) / 256;
-	c = c * period_ns;
-	do_div(c, NSEC_PER_SEC);
-
-	/* Handle high and low extremes */
-	if (c == 0)
-		c = 1;
-	if (c > 255)
-		c = 0; /* 0 set division by 256 */
-	period_cycles = c;
-
-	/* The duty-cycle value is as follows:
-	 *
-	 *  DUTY-CYCLE     HIGH LEVEL
-	 *      1            99.9%
-	 *      25           90.0%
-	 *      128          50.0%
-	 *      220          10.0%
-	 *      255           0.1%
-	 *      0             0.0%
-	 *
-	 * In other words, the register value is duty-cycle % 256 with
-	 * duty-cycle in the range 1-256.
-	 */
-	c = 256 * duty_ns;
-	do_div(c, period_ns);
-	if (c > 255)
-		c = 255;
-	duty_cycles = 256 - c;
+	c = clk_get_rate(lpc32xx->clk);
+
+	/* The highest acceptable divisor is 256, which is represented by 0 */
+	period_cycles = div64_u64(c * period_ns,
+			       (unsigned long long)NSEC_PER_SEC * 256);
+	if (!period_cycles)
+		period_cycles = 1;
+	if (period_cycles > 255)
+		period_cycles = 0;
+
+	/* Compute 256 x #duty/period value and care for corner cases */
+	duty_cycles = div64_u64((unsigned long long)(period_ns - duty_ns) * 256,
+				period_ns);
+	if (!duty_cycles)
+		duty_cycles = 1;
+	if (duty_cycles > 255)
+		duty_cycles = 255;
 
 	val = readl(lpc32xx->base + (pwm->hwpwm << 2));
 	val &= ~0xFFFF;
-	val |= PWM_RELOADV(period_cycles) | PWM_DUTY(duty_cycles);
+	val |= (period_cycles << 8) | duty_cycles;
 	writel(val, lpc32xx->base + (pwm->hwpwm << 2));
 
 	return 0;
-- 
2.1.4

[PATCH 4/4] pwm: lpc32xx: return ERANGE, if requested period is not supported

[Vladimir Zapolskiy]

Instead of silent acceptance of unsupported requested configuration
for PWM period and setting the boundary supported value, return
-ERANGE to a caller.

Duty period value equal to 0 or period is still accepted to allow
configuration by PWM sysfs interface, when it is set to 0 by default.

For reference this is a list of restrictions on period_ns == 1/freq:

  | PWM parent clock | parent clock divisor | max freq | min freq |
  +------------------+----------------------+----------+----------+
  |   HCLK == 13 MHz |      1 (min)         | 50.7 KHz | 198.3 Hz |
  |   HCLK == 13 MHz |     15 (max)         | 3.38 KHz | 13.22 Hz |
  |  RTC == 32.7 KHz |      1 (min)         |   128 Hz |   0.5 Hz |
  |  RTC == 32.7 KHz |     15 (max)         | 8.533 Hz | 0.033 Hz |

Note that PWM sysfs interface does not support setting of period more
than NSEC_PER_SEC / MAX_INT32 ~ 2 seconds, however this PWM controller
supports a period up to 30 seconds.

Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
 drivers/pwm/pwm-lpc32xx.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-lpc32xx.c b/drivers/pwm/pwm-lpc32xx.c
index 294a68f..4d470c1 100644
--- a/drivers/pwm/pwm-lpc32xx.c
+++ b/drivers/pwm/pwm-lpc32xx.c
@@ -41,9 +41,9 @@ static int lpc32xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	/* The highest acceptable divisor is 256, which is represented by 0 */
 	period_cycles = div64_u64(c * period_ns,
 			       (unsigned long long)NSEC_PER_SEC * 256);
-	if (!period_cycles)
-		period_cycles = 1;
-	if (period_cycles > 255)
+	if (!period_cycles || period_cycles > 256)
+		return -ERANGE;
+	if (period_cycles == 256)
 		period_cycles = 0;
 
 	/* Compute 256 x #duty/period value and care for corner cases */
-- 
2.1.4

Re: [PATCH 0/4] pwm: lpc32xx: fixes in the LPC32xx PWM driver

[Vladimir Zapolskiy]

On 13.10.2015 03:20, Vladimir Zapolskiy wrote:
> The changeset fixes a number of issues within current implementation
> of LPC32xx PWM controller driver, namely
> - the SoC has two independent PWM controllers with one channel each,
> - runtime warnings from common clock framework,
> - overflow in duty cycle calculation may result in disabled PWM,
> - unsupported period values are silently accepted.
> 
> Vladimir Zapolskiy (4):
>   pwm: lpc32xx: correct number of PWM channels from 2 to 1
>   pwm: lpc32xx: fix warnings from common clock framework
>   pwm: lpc32xx: fix and simplify duty cycle and period calculations
>   pwm: lpc32xx: return ERANGE, if requested period is not supported
> 
>  drivers/pwm/pwm-lpc32xx.c | 59 ++++++++++++++++++-----------------------------
>  1 file changed, 22 insertions(+), 37 deletions(-)
> 

Hi Thierry,

are you going to pick these changes up to PWM branch?

Thanks in advance.

--
Vladimir