pwm: lpc32xx: fixes in the LPC32xx PWM driver

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 not accepted.

Correction of PWM channels requires a correspondent change in arch/arm/boot/dts/lpc32xx.dtsi, it has been already sent for review.

Changes from v1 to v2:
- corrected style of examples in documentation,
- improved commit messages

Version v1 can be found here:
http://comments.gmane.org/gmane.linux.pwm/2828
http://www.spinics.net/lists/devicetree/msg105398.html

Vladimir Zapolskiy (6):
dt: lpc32xx: pwm: correct LPC32xx PWM device node example
dt: lpc32xx: pwm: update documentation of LPC32xx PWM device
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

 .../devicetree/bindings/pwm/lpc32xx-pwm.txt        |  9 +++-
 drivers/pwm/pwm-lpc32xx.c                          | 59 ++++++++--------------
 2 files changed, 29 insertions(+), 39 deletions(-)

2.1.4

The change removes '0x' from a device node address and uses lower case hex chars.

---
Changes from v1 to v2:
- new change

 Documentation/devicetree/bindings/pwm/lpc32xx-pwm.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index cfe1db3..1ab1abc 100644
@@ -6,7 +6,7 @@ Required properties:
 
 Examples:
 
-pwm at 0x4005C000 {
+pwm at 4005c000 {
 	compatible = "nxp,lpc3220-pwm";
-	reg = <0x4005C000 0x8>;
+	reg = <0x4005c000 0x8>;
 };

2.1.4

NXP LPC32xx SoC has two separate PWM controller devices, update device tree binding documentation to reflect this fact.

The change makes previous PWM device node description incompatible with the new version.

---
Changes from v1 to v2:
- removed '0x' from a device node address and use lower case hex chars.

 Documentation/devicetree/bindings/pwm/lpc32xx-pwm.txt | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

index 1ab1abc..74b5bc5 100644
@@ -8,5 +8,10 @@ Examples:
 
 pwm at 4005c000 {
 	compatible = "nxp,lpc3220-pwm";
-	reg = <0x4005c000 0x8>;
+	reg = <0x4005c000 0x4>;
+};
+
+pwm at 4005c004 {
+	compatible = "nxp,lpc3220-pwm";
+	reg = <0x4005c004 0x4>;
 };

2.1.4

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.

The change makes previous PWM device node description incompatible with this update.

---
Changes from v1 to v2:
- improved commit message

 drivers/pwm/pwm-lpc32xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 9fde60c..ce8ab20 100644
@@ -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

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.

The driver can not be used on a platform with common clock framework until clk_prepare/clk_unprepare calls are added, otherwise clk_enable calls will fail and a WARN is generated:

    # 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)

---
Changes from v1 to v2:
- improved commit message

 drivers/pwm/pwm-lpc32xx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index ce8ab20..63468a8 100644
@@ -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

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.

---
Changes from v1 to v2:
- none

 drivers/pwm/pwm-lpc32xx.c | 53 +++++++++++++++++------------------------------
 1 file changed, 19 insertions(+), 34 deletions(-)

index 63468a8..294a68f 100644
@@ -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

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.

---
Changes from v1 to v2:
- none

 drivers/pwm/pwm-lpc32xx.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index 294a68f..4d470c1 100644
@@ -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

All patches applied, thanks.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151216/241bf709/attachment.sig>