[PATCH 1/2] pwm: vt8500: Register write busy test performed incorrectly

[PATCH 1/2] pwm: vt8500: Register write busy test performed incorrectly

[Tony Prisk]

Correct operation for register writes is to perform a busy-wait
after writing the register. Currently the busy wait it performed
before, meaning subsequent register writes to bitfields may occur
before the previous field has been updated.

Also, all registers are defined as 32-bit read/write. Change
pwm_busy_wait() to use readl rather than readb.

Improve readability of code with defines for registers and bitfields.

Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
---
Thierry,

This patch is a fix but it can go to 3.9 rather than 3.8 (if you prefer)
as the incorrect behaviour doesn't seem to cause a problem on current
hardware.

 drivers/pwm/pwm-vt8500.c |   62 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 48 insertions(+), 14 deletions(-)

diff --git a/drivers/pwm/pwm-vt8500.c b/drivers/pwm/pwm-vt8500.c
index b0ba2d4..27ed0f4 100644
--- a/drivers/pwm/pwm-vt8500.c
+++ b/drivers/pwm/pwm-vt8500.c
@@ -36,6 +36,25 @@
  */
 #define VT8500_NR_PWMS	2
 
+#define REG_CTRL(pwm)		(pwm << 4) + 0x00
+#define REG_SCALAR(pwm)		(pwm << 4) + 0x04
+#define REG_PERIOD(pwm)		(pwm << 4) + 0x08
+#define REG_DUTY(pwm)		(pwm << 4) + 0x0C
+#define REG_STATUS		0x40
+
+#define CTRL_ENABLE		BIT(0)
+#define CTRL_INVERT		BIT(1)
+#define CTRL_AUTOLOAD		BIT(2)
+#define CTRL_STOP_IMM		BIT(3)
+#define CTRL_LOAD_PRESCALE	BIT(4)
+#define CTRL_LOAD_PERIOD	BIT(5)
+
+#define STATUS_CTRL_UPDATE	BIT(0)
+#define STATUS_SCALAR_UPDATE	BIT(1)
+#define STATUS_PERIOD_UPDATE	BIT(2)
+#define STATUS_DUTY_UPDATE	BIT(3)
+#define STATUS_ALL_UPDATE	0x0F
+
 struct vt8500_chip {
 	struct pwm_chip chip;
 	void __iomem *base;
@@ -45,15 +64,17 @@ struct vt8500_chip {
 #define to_vt8500_chip(chip)	container_of(chip, struct vt8500_chip, chip)
 
 #define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
-static inline void pwm_busy_wait(void __iomem *reg, u8 bitmask)
+static inline void pwm_busy_wait(struct vt8500_chip *vt8500, int nr, u8 bitmask)
 {
 	int loops = msecs_to_loops(10);
-	while ((readb(reg) & bitmask) && --loops)
+	u32 mask = bitmask << (nr << 8);
+
+	while ((readl(vt8500->base + REG_STATUS) & mask) && --loops)
 		cpu_relax();
 
 	if (unlikely(!loops))
 		pr_warn("Waiting for status bits 0x%x to clear timed out\n",
-			   bitmask);
+			   mask);
 }
 
 static int vt8500_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -63,6 +84,7 @@ static int vt8500_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	unsigned long long c;
 	unsigned long period_cycles, prescale, pv, dc;
 	int err;
+	u32 val;
 
 	err = clk_enable(vt8500->clk);
 	if (err < 0) {
@@ -91,14 +113,19 @@ static int vt8500_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	do_div(c, period_ns);
 	dc = c;
 
-	pwm_busy_wait(vt8500->base + 0x40 + pwm->hwpwm, (1 << 1));
-	writel(prescale, vt8500->base + 0x4 + (pwm->hwpwm << 4));
+	writel(prescale, vt8500->base + REG_SCALAR(pwm->hwpwm));
+	pwm_busy_wait(vt8500, pwm->hwpwm, STATUS_SCALAR_UPDATE);
+
+	writel(pv, vt8500->base + REG_PERIOD(pwm->hwpwm));
+	pwm_busy_wait(vt8500, pwm->hwpwm, STATUS_PERIOD_UPDATE);
 
-	pwm_busy_wait(vt8500->base + 0x40 + pwm->hwpwm, (1 << 2));
-	writel(pv, vt8500->base + 0x8 + (pwm->hwpwm << 4));
+	writel(dc, vt8500->base + REG_DUTY(pwm->hwpwm));
+	pwm_busy_wait(vt8500, pwm->hwpwm, STATUS_DUTY_UPDATE);
 
-	pwm_busy_wait(vt8500->base + 0x40 + pwm->hwpwm, (1 << 3));
-	writel(dc, vt8500->base + 0xc + (pwm->hwpwm << 4));
+	val = readl(vt8500->base + REG_CTRL(pwm->hwpwm));
+	val |= CTRL_AUTOLOAD;
+	writel(val, vt8500->base + REG_CTRL(pwm->hwpwm));
+	pwm_busy_wait(vt8500, pwm->hwpwm, STATUS_CTRL_UPDATE);
 
 	clk_disable(vt8500->clk);
 	return 0;
@@ -106,8 +133,9 @@ static int vt8500_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 static int vt8500_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
-	int err;
 	struct vt8500_chip *vt8500 = to_vt8500_chip(chip);
+	int err;
+	u32 val;
 
 	err = clk_enable(vt8500->clk);
 	if (err < 0) {
@@ -115,17 +143,23 @@ static int vt8500_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 		return err;
 	}
 
-	pwm_busy_wait(vt8500->base + 0x40 + pwm->hwpwm, (1 << 0));
-	writel(5, vt8500->base + (pwm->hwpwm << 4));
+	val = readl(vt8500->base + REG_CTRL(pwm->hwpwm));
+	val |= CTRL_ENABLE;
+	writel(val, vt8500->base + REG_CTRL(pwm->hwpwm));
+	pwm_busy_wait(vt8500, pwm->hwpwm, STATUS_CTRL_UPDATE);
+
 	return 0;
 }
 
 static void vt8500_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct vt8500_chip *vt8500 = to_vt8500_chip(chip);
+	u32 val;
 
-	pwm_busy_wait(vt8500->base + 0x40 + pwm->hwpwm, (1 << 0));
-	writel(0, vt8500->base + (pwm->hwpwm << 4));
+	val = readl(vt8500->base + REG_CTRL(pwm->hwpwm));
+	val &= ~CTRL_ENABLE;
+	writel(val, vt8500->base + REG_CTRL(pwm->hwpwm));
+	pwm_busy_wait(vt8500, pwm->hwpwm, STATUS_CTRL_UPDATE);
 
 	clk_disable(vt8500->clk);
 }
-- 
1.7.9.5

[PATCH 2/2] pwm: vt8500: Add support for .set_polarity

[Tony Prisk]

Add support to set polarity on pwm devices, allowing for inverted
duty cycles.

Also update the binding document to #pwm-cells = <3> to allow
passing the flags from devicetree.

Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
---
 .../devicetree/bindings/pwm/vt8500-pwm.txt         |    7 ++++---
 drivers/pwm/pwm-vt8500.c                           |   21 ++++++++++++++++++++
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt b/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt
index bcc6367..f71cc8d 100644
--- a/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt
@@ -3,14 +3,15 @@ VIA/Wondermedia VT8500/WM8xxx series SoC PWM controller
 Required properties:
 - compatible: should be "via,vt8500-pwm"
 - reg: physical base address and length of the controller's registers
-- #pwm-cells: should be 2.  The first cell specifies the per-chip index
-  of the PWM to use and the second cell is the period in nanoseconds.
+- #pwm-cells: should be 3.  The first cell specifies the per-chip index
+  of the PWM to use, the second cell is the period in nanoseconds, and the
+  third cell is for flags.
 - clocks: phandle to the PWM source clock
 
 Example:
 
 pwm1: pwm at d8220000 {
-	#pwm-cells = <2>;
+	#pwm-cells = <3>;
 	compatible = "via,vt8500-pwm";
 	reg = <0xd8220000 0x1000>;
 	clocks = <&clkpwm>;
diff --git a/drivers/pwm/pwm-vt8500.c b/drivers/pwm/pwm-vt8500.c
index 27ed0f4..9abf561 100644
--- a/drivers/pwm/pwm-vt8500.c
+++ b/drivers/pwm/pwm-vt8500.c
@@ -164,10 +164,31 @@ static void vt8500_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	clk_disable(vt8500->clk);
 }
 
+static int vt8500_pwm_set_polarity(struct pwm_chip *chip,
+				   struct pwm_device *pwm,
+				   enum pwm_polarity polarity)
+{
+	struct vt8500_chip *vt8500 = to_vt8500_chip(chip);
+	u32 val;
+
+	val = readl(vt8500->base + REG_CTRL(pwm->hwpwm));
+
+	if (polarity == PWM_POLARITY_INVERSED)
+		val |= CTRL_INVERT;
+	else
+		val &= ~CTRL_INVERT;
+
+	writel(val, vt8500->base + REG_CTRL(pwm->hwpwm));
+	pwm_busy_wait(vt8500, pwm->hwpwm, STATUS_CTRL_UPDATE);
+
+	return 0;
+}
+
 static struct pwm_ops vt8500_pwm_ops = {
 	.enable = vt8500_pwm_enable,
 	.disable = vt8500_pwm_disable,
 	.config = vt8500_pwm_config,
+	.set_polarity = vt8500_pwm_set_polarity,
 	.owner = THIS_MODULE,
 };
 
-- 
1.7.9.5

[PATCH 2/2] pwm: vt8500: Add support for .set_polarity

[Thierry Reding]

On Mon, Dec 31, 2012 at 09:23:25AM +1300, Tony Prisk wrote:
> Add support to set polarity on pwm devices, allowing for inverted
> duty cycles.

"PWM devices", please. Maybe the subject could be something less API
specific, like "pwm: vt8500: Add polarity support".

>  .../devicetree/bindings/pwm/vt8500-pwm.txt         |    7 ++++---
>  drivers/pwm/pwm-vt8500.c                           |   21 ++++++++++++++++++++
>  2 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt b/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt
> index bcc6367..f71cc8d 100644
> --- a/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt
> +++ b/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt
> @@ -3,14 +3,15 @@ VIA/Wondermedia VT8500/WM8xxx series SoC PWM controller
>  Required properties:
>  - compatible: should be "via,vt8500-pwm"
>  - reg: physical base address and length of the controller's registers
> -- #pwm-cells: should be 2.  The first cell specifies the per-chip index
> -  of the PWM to use and the second cell is the period in nanoseconds.
> +- #pwm-cells: should be 3.  The first cell specifies the per-chip index
> +  of the PWM to use, the second cell is the period in nanoseconds, and the
> +  third cell is for flags.

This needs to document what flags the third cell encodes and how. Either
that or refer to the generic PWM binding documentation.

> diff --git a/drivers/pwm/pwm-vt8500.c b/drivers/pwm/pwm-vt8500.c
> index 27ed0f4..9abf561 100644
> --- a/drivers/pwm/pwm-vt8500.c
> +++ b/drivers/pwm/pwm-vt8500.c
> @@ -164,10 +164,31 @@ static void vt8500_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	clk_disable(vt8500->clk);
>  }
>  
> +static int vt8500_pwm_set_polarity(struct pwm_chip *chip,
> +				   struct pwm_device *pwm,
> +				   enum pwm_polarity polarity)
> +{
> +	struct vt8500_chip *vt8500 = to_vt8500_chip(chip);
> +	u32 val;
> +
> +	val = readl(vt8500->base + REG_CTRL(pwm->hwpwm));
> +
> +	if (polarity == PWM_POLARITY_INVERSED)
> +		val |= CTRL_INVERT;
> +	else
> +		val &= ~CTRL_INVERT;
> +
> +	writel(val, vt8500->base + REG_CTRL(pwm->hwpwm));
> +	pwm_busy_wait(vt8500, pwm->hwpwm, STATUS_CTRL_UPDATE);
> +
> +	return 0;
> +}
> +
>  static struct pwm_ops vt8500_pwm_ops = {
>  	.enable = vt8500_pwm_enable,
>  	.disable = vt8500_pwm_disable,
>  	.config = vt8500_pwm_config,
> +	.set_polarity = vt8500_pwm_set_polarity,
>  	.owner = THIS_MODULE,
>  };

This patch is missing the corresponding changes to the DT code. You need
to override the pwm_chip's .of_xlate() and .of_pwm_n_cells fields. You
can look at the TI EHRPWM driver if you need inspiration.

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

[PATCH 1/2] pwm: vt8500: Register write busy test performed incorrectly

[Thierry Reding]

On Mon, Dec 31, 2012 at 09:23:24AM +1300, Tony Prisk wrote:
> Correct operation for register writes is to perform a busy-wait
> after writing the register. Currently the busy wait it performed
> before, meaning subsequent register writes to bitfields may occur
> before the previous field has been updated.
> 
> Also, all registers are defined as 32-bit read/write. Change
> pwm_busy_wait() to use readl rather than readb.
> 
> Improve readability of code with defines for registers and bitfields.
> 
> Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
> ---
> Thierry,
> 
> This patch is a fix but it can go to 3.9 rather than 3.8 (if you prefer)
> as the incorrect behaviour doesn't seem to cause a problem on current
> hardware.
> 
>  drivers/pwm/pwm-vt8500.c |   62 +++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 48 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-vt8500.c b/drivers/pwm/pwm-vt8500.c
> index b0ba2d4..27ed0f4 100644
> --- a/drivers/pwm/pwm-vt8500.c
> +++ b/drivers/pwm/pwm-vt8500.c
> @@ -36,6 +36,25 @@
>   */
>  #define VT8500_NR_PWMS	2
>  
> +#define REG_CTRL(pwm)		(pwm << 4) + 0x00
> +#define REG_SCALAR(pwm)		(pwm << 4) + 0x04
> +#define REG_PERIOD(pwm)		(pwm << 4) + 0x08
> +#define REG_DUTY(pwm)		(pwm << 4) + 0x0C

To be on the safe side, I think these should be:

	(((pwm) << 4) + offset)

> -static inline void pwm_busy_wait(void __iomem *reg, u8 bitmask)
> +static inline void pwm_busy_wait(struct vt8500_chip *vt8500, int nr, u8 bitmask)
>  {
>  	int loops = msecs_to_loops(10);
> -	while ((readb(reg) & bitmask) && --loops)
> +	u32 mask = bitmask << (nr << 8);
> +
> +	while ((readl(vt8500->base + REG_STATUS) & mask) && --loops)
>  		cpu_relax();
>  
>  	if (unlikely(!loops))
>  		pr_warn("Waiting for status bits 0x%x to clear timed out\n",
> -			   bitmask);
> +			   mask);
>  }

Now that you're passing a struct vt8500_chip, couldn't you use
dev_warn() instead?

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