[PATCH 1/2] PWM: atmel: fix incorrect CDTY value after enabling

[PATCH 1/2] PWM: atmel: fix incorrect CDTY value after enabling

[Alexandre Belloni]

CUPD is not flushed before enabling the channel so it will update CDTY/CPRD just
after one period. So we always set CUPD, even when the channel is not enabled.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 drivers/pwm/pwm-atmel.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index d3c22de9ee47..89f9ca41d9af 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -155,24 +155,25 @@ static void atmel_pwm_config_v1(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
 	unsigned int val;
 
-	if (test_bit(PWMF_ENABLED, &pwm->flags)) {
-		/*
-		 * If the PWM channel is enabled, using the update register,
-		 * it needs to set bit 10 of CMR to 0
-		 */
-		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CUPD, dty);
 
-		val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
-		val &= ~PWM_CMR_UPD_CDTY;
-		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
-	} else {
-		/*
-		 * If the PWM channel is disabled, write value to duty and
-		 * period registers directly.
-		 */
-		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CDTY, dty);
-		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CPRD, prd);
-	}
+	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CUPD, dty);
+
+	val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
+	val &= ~PWM_CMR_UPD_CDTY;
+	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
+
+	/*
+	 * If the PWM channel is enabled, only update CDTY by using the update
+	 * register, it needs to set bit 10 of CMR to 0
+	 */
+	if (test_bit(PWMF_ENABLED, &pwm->flags))
+		return;
+	/*
+	 * If the PWM channel is disabled, write value to duty and period
+	 * registers directly.
+	 */
+	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CDTY, dty);
+	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CPRD, prd);
 }
 
 static void atmel_pwm_config_v2(struct pwm_chip *chip, struct pwm_device *pwm,
-- 
2.1.4

[PATCH 1/2] PWM: atmel: fix incorrect CDTY value after enabling

[Alexandre Belloni]

CUPD is not flushed before enabling the channel so it will update CDTY/CPRD just
after one period. So we always set CUPD, even when the channel is not enabled.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 drivers/pwm/pwm-atmel.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index d3c22de9ee47..89f9ca41d9af 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -155,24 +155,25 @@ static void atmel_pwm_config_v1(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
 	unsigned int val;
 
-	if (test_bit(PWMF_ENABLED, &pwm->flags)) {
-		/*
-		 * If the PWM channel is enabled, using the update register,
-		 * it needs to set bit 10 of CMR to 0
-		 */
-		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CUPD, dty);
 
-		val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
-		val &= ~PWM_CMR_UPD_CDTY;
-		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
-	} else {
-		/*
-		 * If the PWM channel is disabled, write value to duty and
-		 * period registers directly.
-		 */
-		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CDTY, dty);
-		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CPRD, prd);
-	}
+	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CUPD, dty);
+
+	val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
+	val &= ~PWM_CMR_UPD_CDTY;
+	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
+
+	/*
+	 * If the PWM channel is enabled, only update CDTY by using the update
+	 * register, it needs to set bit 10 of CMR to 0
+	 */
+	if (test_bit(PWMF_ENABLED, &pwm->flags))
+		return;
+	/*
+	 * If the PWM channel is disabled, write value to duty and period
+	 * registers directly.
+	 */
+	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CDTY, dty);
+	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CPRD, prd);
 }
 
 static void atmel_pwm_config_v2(struct pwm_chip *chip, struct pwm_device *pwm,
-- 
2.1.4

[PATCH 2/2] PWM: atmel: fix incorrect CDTY value after disabling

[Alexandre Belloni]

pwm-leds calls .config() and .disable() in a row. This exhibits that it may
happen that the channel gets disabled before CDTY has been updated with CUPD.
The issue gets quite worse with long periods.
So, ensure that at least one period has past before disabling the channel by
polling ISR.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 drivers/pwm/pwm-atmel.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index 89f9ca41d9af..054db5aacd7a 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -8,6 +8,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/module.h>
@@ -21,6 +22,7 @@
 #define PWM_ENA			0x04
 #define PWM_DIS			0x08
 #define PWM_SR			0x0C
+#define PWM_ISR			0x1C
 /* Bit field in SR */
 #define PWM_SR_ALL_CH_ON	0x0F
 
@@ -243,6 +245,17 @@ static int atmel_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
+	unsigned long timeout = jiffies + 2 * HZ;
+
+	/*
+	 * Wait for at least a complete period to have passed before disabling a
+	 * channel to be sure that CDTY has been updated
+	 */
+	atmel_pwm_readl(atmel_pwm, PWM_ISR);
+
+	while (!(atmel_pwm_readl(atmel_pwm, PWM_ISR) & (1 << pwm->hwpwm)) &&
+	       time_before(jiffies, timeout))
+		usleep_range(10, 100);
 
 	atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm);
 
-- 
2.1.4

[PATCH 2/2] PWM: atmel: fix incorrect CDTY value after disabling

[Alexandre Belloni]

pwm-leds calls .config() and .disable() in a row. This exhibits that it may
happen that the channel gets disabled before CDTY has been updated with CUPD.
The issue gets quite worse with long periods.
So, ensure that at least one period has past before disabling the channel by
polling ISR.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 drivers/pwm/pwm-atmel.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index 89f9ca41d9af..054db5aacd7a 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -8,6 +8,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/module.h>
@@ -21,6 +22,7 @@
 #define PWM_ENA			0x04
 #define PWM_DIS			0x08
 #define PWM_SR			0x0C
+#define PWM_ISR			0x1C
 /* Bit field in SR */
 #define PWM_SR_ALL_CH_ON	0x0F
 
@@ -243,6 +245,17 @@ static int atmel_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
+	unsigned long timeout = jiffies + 2 * HZ;
+
+	/*
+	 * Wait for at least a complete period to have passed before disabling a
+	 * channel to be sure that CDTY has been updated
+	 */
+	atmel_pwm_readl(atmel_pwm, PWM_ISR);
+
+	while (!(atmel_pwm_readl(atmel_pwm, PWM_ISR) & (1 << pwm->hwpwm)) &&
+	       time_before(jiffies, timeout))
+		usleep_range(10, 100);
 
 	atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm);
 
-- 
2.1.4

[PATCHv2 2/2] PWM: atmel: fix incorrect CDTY value after disabling

[Alexandre Belloni]

pwm-leds calls .config() and .disable() in a row. This exhibits that it may
happen that the channel gets disabled before CDTY has been updated with CUPD.
The issue gets quite worse with long periods.
So, ensure that at least one period has past before disabling the channel by
polling ISR.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
I had the previous patch tested on a multicolor LED and unfortunately, the delay
is not nice in that case. I wanted to avoid a mutex.
This version uses a variable to store the state of the update of the pwm. This
is reset everytime is gets a new configuration and updated when it is disabled
or another pwm is configured.
When disabling a multicolor LED, we only wait for the first pwm to update then
we remember that we already seen the other one having their period expire and it
looks much better.

 drivers/pwm/pwm-atmel.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)


diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index 89f9ca41d9af..a947c9095d9d 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -8,9 +8,11 @@
  */
 
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
@@ -21,6 +23,7 @@
 #define PWM_ENA			0x04
 #define PWM_DIS			0x08
 #define PWM_SR			0x0C
+#define PWM_ISR			0x1C
 /* Bit field in SR */
 #define PWM_SR_ALL_CH_ON	0x0F
 
@@ -60,6 +63,9 @@ struct atmel_pwm_chip {
 	struct clk *clk;
 	void __iomem *base;
 
+	unsigned int updated_pwms;
+	struct mutex isr_lock; /* ISR is cleared when read, ensure only one thread does that */
+
 	void (*config)(struct pwm_chip *chip, struct pwm_device *pwm,
 		       unsigned long dty, unsigned long prd);
 };
@@ -144,6 +150,10 @@ static int atmel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	val = (val & ~PWM_CMR_CPRE_MSK) | (pres & PWM_CMR_CPRE_MSK);
 	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
 	atmel_pwm->config(chip, pwm, dty, prd);
+	mutex_lock(&atmel_pwm->isr_lock);
+	atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR);
+	atmel_pwm->updated_pwms &= ~(1 << pwm->hwpwm);
+	mutex_unlock(&atmel_pwm->isr_lock);
 
 	clk_disable(atmel_pwm->clk);
 	return ret;
@@ -243,7 +253,22 @@ static int atmel_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
+	unsigned long timeout = jiffies + 2 * HZ;
+
+	/*
+	 * Wait for at least a complete period to have passed before disabling a
+	 * channel to be sure that CDTY has been updated
+	 */
+	mutex_lock(&atmel_pwm->isr_lock);
+	atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR);
+
+	while (!(atmel_pwm->updated_pwms & (1 << pwm->hwpwm)) &&
+	       time_before(jiffies, timeout)) {
+		usleep_range(10, 100);
+		atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR);
+	}
 
+	mutex_unlock(&atmel_pwm->isr_lock);
 	atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm);
 
 	clk_disable(atmel_pwm->clk);
@@ -358,6 +383,8 @@ static int atmel_pwm_probe(struct platform_device *pdev)
 	atmel_pwm->chip.npwm = 4;
 	atmel_pwm->chip.can_sleep = true;
 	atmel_pwm->config = data->config;
+	atmel_pwm->updated_pwms = 0;
+	mutex_init(&atmel_pwm->isr_lock);
 
 	ret = pwmchip_add(&atmel_pwm->chip);
 	if (ret < 0) {
@@ -379,6 +406,7 @@ static int atmel_pwm_remove(struct platform_device *pdev)
 	struct atmel_pwm_chip *atmel_pwm = platform_get_drvdata(pdev);
 
 	clk_unprepare(atmel_pwm->clk);
+	mutex_destroy(&atmel_pwm->isr_lock);
 
 	return pwmchip_remove(&atmel_pwm->chip);
 }
-- 
2.1.4

[PATCHv2 2/2] PWM: atmel: fix incorrect CDTY value after disabling

[Alexandre Belloni]

pwm-leds calls .config() and .disable() in a row. This exhibits that it may
happen that the channel gets disabled before CDTY has been updated with CUPD.
The issue gets quite worse with long periods.
So, ensure that at least one period has past before disabling the channel by
polling ISR.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
I had the previous patch tested on a multicolor LED and unfortunately, the delay
is not nice in that case. I wanted to avoid a mutex.
This version uses a variable to store the state of the update of the pwm. This
is reset everytime is gets a new configuration and updated when it is disabled
or another pwm is configured.
When disabling a multicolor LED, we only wait for the first pwm to update then
we remember that we already seen the other one having their period expire and it
looks much better.

 drivers/pwm/pwm-atmel.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)


diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index 89f9ca41d9af..a947c9095d9d 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -8,9 +8,11 @@
  */
 
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
@@ -21,6 +23,7 @@
 #define PWM_ENA			0x04
 #define PWM_DIS			0x08
 #define PWM_SR			0x0C
+#define PWM_ISR			0x1C
 /* Bit field in SR */
 #define PWM_SR_ALL_CH_ON	0x0F
 
@@ -60,6 +63,9 @@ struct atmel_pwm_chip {
 	struct clk *clk;
 	void __iomem *base;
 
+	unsigned int updated_pwms;
+	struct mutex isr_lock; /* ISR is cleared when read, ensure only one thread does that */
+
 	void (*config)(struct pwm_chip *chip, struct pwm_device *pwm,
 		       unsigned long dty, unsigned long prd);
 };
@@ -144,6 +150,10 @@ static int atmel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	val = (val & ~PWM_CMR_CPRE_MSK) | (pres & PWM_CMR_CPRE_MSK);
 	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
 	atmel_pwm->config(chip, pwm, dty, prd);
+	mutex_lock(&atmel_pwm->isr_lock);
+	atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR);
+	atmel_pwm->updated_pwms &= ~(1 << pwm->hwpwm);
+	mutex_unlock(&atmel_pwm->isr_lock);
 
 	clk_disable(atmel_pwm->clk);
 	return ret;
@@ -243,7 +253,22 @@ static int atmel_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
+	unsigned long timeout = jiffies + 2 * HZ;
+
+	/*
+	 * Wait for at least a complete period to have passed before disabling a
+	 * channel to be sure that CDTY has been updated
+	 */
+	mutex_lock(&atmel_pwm->isr_lock);
+	atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR);
+
+	while (!(atmel_pwm->updated_pwms & (1 << pwm->hwpwm)) &&
+	       time_before(jiffies, timeout)) {
+		usleep_range(10, 100);
+		atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR);
+	}
 
+	mutex_unlock(&atmel_pwm->isr_lock);
 	atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm);
 
 	clk_disable(atmel_pwm->clk);
@@ -358,6 +383,8 @@ static int atmel_pwm_probe(struct platform_device *pdev)
 	atmel_pwm->chip.npwm = 4;
 	atmel_pwm->chip.can_sleep = true;
 	atmel_pwm->config = data->config;
+	atmel_pwm->updated_pwms = 0;
+	mutex_init(&atmel_pwm->isr_lock);
 
 	ret = pwmchip_add(&atmel_pwm->chip);
 	if (ret < 0) {
@@ -379,6 +406,7 @@ static int atmel_pwm_remove(struct platform_device *pdev)
 	struct atmel_pwm_chip *atmel_pwm = platform_get_drvdata(pdev);
 
 	clk_unprepare(atmel_pwm->clk);
+	mutex_destroy(&atmel_pwm->isr_lock);
 
 	return pwmchip_remove(&atmel_pwm->chip);
 }
-- 
2.1.4

Re: [PATCH 1/2] PWM: atmel: fix incorrect CDTY value after enabling

[Nicolas Ferre]

Le 25/05/2015 15:19, Alexandre Belloni a écrit :
> CUPD is not flushed before enabling the channel so it will update CDTY/CPRD just
> after one period. So we always set CUPD, even when the channel is not enabled.
> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>

Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

BTW, is a "stable tag" needed for this one?

> ---
>  drivers/pwm/pwm-atmel.c | 35 ++++++++++++++++++-----------------
>  1 file changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index d3c22de9ee47..89f9ca41d9af 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -155,24 +155,25 @@ static void atmel_pwm_config_v1(struct pwm_chip *chip, struct pwm_device *pwm,
>  	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
>  	unsigned int val;
>  
> -	if (test_bit(PWMF_ENABLED, &pwm->flags)) {
> -		/*
> -		 * If the PWM channel is enabled, using the update register,
> -		 * it needs to set bit 10 of CMR to 0
> -		 */
> -		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CUPD, dty);
>  
> -		val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
> -		val &= ~PWM_CMR_UPD_CDTY;
> -		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
> -	} else {
> -		/*
> -		 * If the PWM channel is disabled, write value to duty and
> -		 * period registers directly.
> -		 */
> -		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CDTY, dty);
> -		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CPRD, prd);
> -	}
> +	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CUPD, dty);
> +
> +	val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
> +	val &= ~PWM_CMR_UPD_CDTY;
> +	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
> +
> +	/*
> +	 * If the PWM channel is enabled, only update CDTY by using the update
> +	 * register, it needs to set bit 10 of CMR to 0
> +	 */
> +	if (test_bit(PWMF_ENABLED, &pwm->flags))
> +		return;
> +	/*
> +	 * If the PWM channel is disabled, write value to duty and period
> +	 * registers directly.
> +	 */
> +	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CDTY, dty);
> +	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CPRD, prd);
>  }
>  
>  static void atmel_pwm_config_v2(struct pwm_chip *chip, struct pwm_device *pwm,
> 


-- 
Nicolas Ferre

Re: [PATCH 1/2] PWM: atmel: fix incorrect CDTY value after enabling

[Nicolas Ferre]

Le 25/05/2015 15:19, Alexandre Belloni a écrit :
> CUPD is not flushed before enabling the channel so it will update CDTY/CPRD just
> after one period. So we always set CUPD, even when the channel is not enabled.
> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>

Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

BTW, is a "stable tag" needed for this one?

> ---
>  drivers/pwm/pwm-atmel.c | 35 ++++++++++++++++++-----------------
>  1 file changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index d3c22de9ee47..89f9ca41d9af 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -155,24 +155,25 @@ static void atmel_pwm_config_v1(struct pwm_chip *chip, struct pwm_device *pwm,
>  	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
>  	unsigned int val;
>  
> -	if (test_bit(PWMF_ENABLED, &pwm->flags)) {
> -		/*
> -		 * If the PWM channel is enabled, using the update register,
> -		 * it needs to set bit 10 of CMR to 0
> -		 */
> -		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CUPD, dty);
>  
> -		val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
> -		val &= ~PWM_CMR_UPD_CDTY;
> -		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
> -	} else {
> -		/*
> -		 * If the PWM channel is disabled, write value to duty and
> -		 * period registers directly.
> -		 */
> -		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CDTY, dty);
> -		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CPRD, prd);
> -	}
> +	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CUPD, dty);
> +
> +	val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
> +	val &= ~PWM_CMR_UPD_CDTY;
> +	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
> +
> +	/*
> +	 * If the PWM channel is enabled, only update CDTY by using the update
> +	 * register, it needs to set bit 10 of CMR to 0
> +	 */
> +	if (test_bit(PWMF_ENABLED, &pwm->flags))
> +		return;
> +	/*
> +	 * If the PWM channel is disabled, write value to duty and period
> +	 * registers directly.
> +	 */
> +	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CDTY, dty);
> +	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CPRD, prd);
>  }
>  
>  static void atmel_pwm_config_v2(struct pwm_chip *chip, struct pwm_device *pwm,
> 


-- 
Nicolas Ferre

[PATCH 1/2] PWM: atmel: fix incorrect CDTY value after enabling

[Nicolas Ferre]

Le 25/05/2015 15:19, Alexandre Belloni a ?crit :
> CUPD is not flushed before enabling the channel so it will update CDTY/CPRD just
> after one period. So we always set CUPD, even when the channel is not enabled.
> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>

Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

BTW, is a "stable tag" needed for this one?

> ---
>  drivers/pwm/pwm-atmel.c | 35 ++++++++++++++++++-----------------
>  1 file changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index d3c22de9ee47..89f9ca41d9af 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -155,24 +155,25 @@ static void atmel_pwm_config_v1(struct pwm_chip *chip, struct pwm_device *pwm,
>  	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
>  	unsigned int val;
>  
> -	if (test_bit(PWMF_ENABLED, &pwm->flags)) {
> -		/*
> -		 * If the PWM channel is enabled, using the update register,
> -		 * it needs to set bit 10 of CMR to 0
> -		 */
> -		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CUPD, dty);
>  
> -		val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
> -		val &= ~PWM_CMR_UPD_CDTY;
> -		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
> -	} else {
> -		/*
> -		 * If the PWM channel is disabled, write value to duty and
> -		 * period registers directly.
> -		 */
> -		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CDTY, dty);
> -		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CPRD, prd);
> -	}
> +	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CUPD, dty);
> +
> +	val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
> +	val &= ~PWM_CMR_UPD_CDTY;
> +	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
> +
> +	/*
> +	 * If the PWM channel is enabled, only update CDTY by using the update
> +	 * register, it needs to set bit 10 of CMR to 0
> +	 */
> +	if (test_bit(PWMF_ENABLED, &pwm->flags))
> +		return;
> +	/*
> +	 * If the PWM channel is disabled, write value to duty and period
> +	 * registers directly.
> +	 */
> +	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CDTY, dty);
> +	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CPRD, prd);
>  }
>  
>  static void atmel_pwm_config_v2(struct pwm_chip *chip, struct pwm_device *pwm,
> 


-- 
Nicolas Ferre

Re: [PATCHv2 2/2] PWM: atmel: fix incorrect CDTY value after disabling

[Gaël PORTAY]

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

On 25 May 2015 at 18:11, Alexandre Belloni <
alexandre.belloni@free-electrons.com> wrote:

> I had the previous patch tested on a multicolor LED and unfortunately, the
> delay
> is not nice in that case. I wanted to avoid a mutex.
> This version uses a variable to store the state of the update of the pwm.
> This
> is reset everytime is gets a new configuration and updated when it is
> disabled
> or another pwm is configured.
>

This version (including a mutex) works fine on at91-kizbox2.

All tree PWM LEDs now shut down.


> When disabling a multicolor LED, we only wait for the first pwm to update
> then
> we remember that we already seen the other one having their period expire
> and it
> looks much better.
>

Indeed, without mutex (and with a long period), I noticed a kind of
"glitch" when I switch off all 3 LEDs: White ->
Red|Blue|Green|Yellow|Cyan|Magenta -> Off.

With the mutex, all 3 LEDs shut down at the same time.

Tested-by: Gaël PORTAY <gael.portay@gmail.com>

[-- Attachment #2: Type: text/html, Size: 1745 bytes --]

Re: [PATCHv2 2/2] PWM: atmel: fix incorrect CDTY value after disabling

[Nicolas Ferre]

Le 25/05/2015 18:11, Alexandre Belloni a écrit :
> pwm-leds calls .config() and .disable() in a row. This exhibits that it may
> happen that the channel gets disabled before CDTY has been updated with CUPD.
> The issue gets quite worse with long periods.
> So, ensure that at least one period has past before disabling the channel by
> polling ISR.
> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
> I had the previous patch tested on a multicolor LED and unfortunately, the delay
> is not nice in that case. I wanted to avoid a mutex.
> This version uses a variable to store the state of the update of the pwm. This
> is reset everytime is gets a new configuration and updated when it is disabled
> or another pwm is configured.
> When disabling a multicolor LED, we only wait for the first pwm to update then
> we remember that we already seen the other one having their period expire and it
> looks much better.

It seems valid:
Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

Thanks!

> 
>  drivers/pwm/pwm-atmel.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> 
> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index 89f9ca41d9af..a947c9095d9d 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -8,9 +8,11 @@
>   */
>  
>  #include <linux/clk.h>
> +#include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
> @@ -21,6 +23,7 @@
>  #define PWM_ENA			0x04
>  #define PWM_DIS			0x08
>  #define PWM_SR			0x0C
> +#define PWM_ISR			0x1C
>  /* Bit field in SR */
>  #define PWM_SR_ALL_CH_ON	0x0F
>  
> @@ -60,6 +63,9 @@ struct atmel_pwm_chip {
>  	struct clk *clk;
>  	void __iomem *base;
>  
> +	unsigned int updated_pwms;
> +	struct mutex isr_lock; /* ISR is cleared when read, ensure only one thread does that */
> +
>  	void (*config)(struct pwm_chip *chip, struct pwm_device *pwm,
>  		       unsigned long dty, unsigned long prd);
>  };
> @@ -144,6 +150,10 @@ static int atmel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	val = (val & ~PWM_CMR_CPRE_MSK) | (pres & PWM_CMR_CPRE_MSK);
>  	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
>  	atmel_pwm->config(chip, pwm, dty, prd);
> +	mutex_lock(&atmel_pwm->isr_lock);
> +	atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR);
> +	atmel_pwm->updated_pwms &= ~(1 << pwm->hwpwm);
> +	mutex_unlock(&atmel_pwm->isr_lock);
>  
>  	clk_disable(atmel_pwm->clk);
>  	return ret;
> @@ -243,7 +253,22 @@ static int atmel_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>  static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>  	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
> +	unsigned long timeout = jiffies + 2 * HZ;
> +
> +	/*
> +	 * Wait for at least a complete period to have passed before disabling a
> +	 * channel to be sure that CDTY has been updated
> +	 */
> +	mutex_lock(&atmel_pwm->isr_lock);
> +	atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR);
> +
> +	while (!(atmel_pwm->updated_pwms & (1 << pwm->hwpwm)) &&
> +	       time_before(jiffies, timeout)) {
> +		usleep_range(10, 100);
> +		atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR);
> +	}
>  
> +	mutex_unlock(&atmel_pwm->isr_lock);
>  	atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm);
>  
>  	clk_disable(atmel_pwm->clk);
> @@ -358,6 +383,8 @@ static int atmel_pwm_probe(struct platform_device *pdev)
>  	atmel_pwm->chip.npwm = 4;
>  	atmel_pwm->chip.can_sleep = true;
>  	atmel_pwm->config = data->config;
> +	atmel_pwm->updated_pwms = 0;
> +	mutex_init(&atmel_pwm->isr_lock);
>  
>  	ret = pwmchip_add(&atmel_pwm->chip);
>  	if (ret < 0) {
> @@ -379,6 +406,7 @@ static int atmel_pwm_remove(struct platform_device *pdev)
>  	struct atmel_pwm_chip *atmel_pwm = platform_get_drvdata(pdev);
>  
>  	clk_unprepare(atmel_pwm->clk);
> +	mutex_destroy(&atmel_pwm->isr_lock);
>  
>  	return pwmchip_remove(&atmel_pwm->chip);
>  }
> 


-- 
Nicolas Ferre

Re: [PATCHv2 2/2] PWM: atmel: fix incorrect CDTY value after disabling

[Nicolas Ferre]

Le 25/05/2015 18:11, Alexandre Belloni a écrit :
> pwm-leds calls .config() and .disable() in a row. This exhibits that it may
> happen that the channel gets disabled before CDTY has been updated with CUPD.
> The issue gets quite worse with long periods.
> So, ensure that at least one period has past before disabling the channel by
> polling ISR.
> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
> I had the previous patch tested on a multicolor LED and unfortunately, the delay
> is not nice in that case. I wanted to avoid a mutex.
> This version uses a variable to store the state of the update of the pwm. This
> is reset everytime is gets a new configuration and updated when it is disabled
> or another pwm is configured.
> When disabling a multicolor LED, we only wait for the first pwm to update then
> we remember that we already seen the other one having their period expire and it
> looks much better.

It seems valid:
Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

Thanks!

> 
>  drivers/pwm/pwm-atmel.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> 
> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index 89f9ca41d9af..a947c9095d9d 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -8,9 +8,11 @@
>   */
>  
>  #include <linux/clk.h>
> +#include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
> @@ -21,6 +23,7 @@
>  #define PWM_ENA			0x04
>  #define PWM_DIS			0x08
>  #define PWM_SR			0x0C
> +#define PWM_ISR			0x1C
>  /* Bit field in SR */
>  #define PWM_SR_ALL_CH_ON	0x0F
>  
> @@ -60,6 +63,9 @@ struct atmel_pwm_chip {
>  	struct clk *clk;
>  	void __iomem *base;
>  
> +	unsigned int updated_pwms;
> +	struct mutex isr_lock; /* ISR is cleared when read, ensure only one thread does that */
> +
>  	void (*config)(struct pwm_chip *chip, struct pwm_device *pwm,
>  		       unsigned long dty, unsigned long prd);
>  };
> @@ -144,6 +150,10 @@ static int atmel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	val = (val & ~PWM_CMR_CPRE_MSK) | (pres & PWM_CMR_CPRE_MSK);
>  	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
>  	atmel_pwm->config(chip, pwm, dty, prd);
> +	mutex_lock(&atmel_pwm->isr_lock);
> +	atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR);
> +	atmel_pwm->updated_pwms &= ~(1 << pwm->hwpwm);
> +	mutex_unlock(&atmel_pwm->isr_lock);
>  
>  	clk_disable(atmel_pwm->clk);
>  	return ret;
> @@ -243,7 +253,22 @@ static int atmel_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>  static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>  	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
> +	unsigned long timeout = jiffies + 2 * HZ;
> +
> +	/*
> +	 * Wait for at least a complete period to have passed before disabling a
> +	 * channel to be sure that CDTY has been updated
> +	 */
> +	mutex_lock(&atmel_pwm->isr_lock);
> +	atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR);
> +
> +	while (!(atmel_pwm->updated_pwms & (1 << pwm->hwpwm)) &&
> +	       time_before(jiffies, timeout)) {
> +		usleep_range(10, 100);
> +		atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR);
> +	}
>  
> +	mutex_unlock(&atmel_pwm->isr_lock);
>  	atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm);
>  
>  	clk_disable(atmel_pwm->clk);
> @@ -358,6 +383,8 @@ static int atmel_pwm_probe(struct platform_device *pdev)
>  	atmel_pwm->chip.npwm = 4;
>  	atmel_pwm->chip.can_sleep = true;
>  	atmel_pwm->config = data->config;
> +	atmel_pwm->updated_pwms = 0;
> +	mutex_init(&atmel_pwm->isr_lock);
>  
>  	ret = pwmchip_add(&atmel_pwm->chip);
>  	if (ret < 0) {
> @@ -379,6 +406,7 @@ static int atmel_pwm_remove(struct platform_device *pdev)
>  	struct atmel_pwm_chip *atmel_pwm = platform_get_drvdata(pdev);
>  
>  	clk_unprepare(atmel_pwm->clk);
> +	mutex_destroy(&atmel_pwm->isr_lock);
>  
>  	return pwmchip_remove(&atmel_pwm->chip);
>  }
> 


-- 
Nicolas Ferre

[PATCHv2 2/2] PWM: atmel: fix incorrect CDTY value after disabling

[Nicolas Ferre]

Le 25/05/2015 18:11, Alexandre Belloni a ?crit :
> pwm-leds calls .config() and .disable() in a row. This exhibits that it may
> happen that the channel gets disabled before CDTY has been updated with CUPD.
> The issue gets quite worse with long periods.
> So, ensure that at least one period has past before disabling the channel by
> polling ISR.
> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
> I had the previous patch tested on a multicolor LED and unfortunately, the delay
> is not nice in that case. I wanted to avoid a mutex.
> This version uses a variable to store the state of the update of the pwm. This
> is reset everytime is gets a new configuration and updated when it is disabled
> or another pwm is configured.
> When disabling a multicolor LED, we only wait for the first pwm to update then
> we remember that we already seen the other one having their period expire and it
> looks much better.

It seems valid:
Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

Thanks!

> 
>  drivers/pwm/pwm-atmel.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> 
> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index 89f9ca41d9af..a947c9095d9d 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -8,9 +8,11 @@
>   */
>  
>  #include <linux/clk.h>
> +#include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
> @@ -21,6 +23,7 @@
>  #define PWM_ENA			0x04
>  #define PWM_DIS			0x08
>  #define PWM_SR			0x0C
> +#define PWM_ISR			0x1C
>  /* Bit field in SR */
>  #define PWM_SR_ALL_CH_ON	0x0F
>  
> @@ -60,6 +63,9 @@ struct atmel_pwm_chip {
>  	struct clk *clk;
>  	void __iomem *base;
>  
> +	unsigned int updated_pwms;
> +	struct mutex isr_lock; /* ISR is cleared when read, ensure only one thread does that */
> +
>  	void (*config)(struct pwm_chip *chip, struct pwm_device *pwm,
>  		       unsigned long dty, unsigned long prd);
>  };
> @@ -144,6 +150,10 @@ static int atmel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	val = (val & ~PWM_CMR_CPRE_MSK) | (pres & PWM_CMR_CPRE_MSK);
>  	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
>  	atmel_pwm->config(chip, pwm, dty, prd);
> +	mutex_lock(&atmel_pwm->isr_lock);
> +	atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR);
> +	atmel_pwm->updated_pwms &= ~(1 << pwm->hwpwm);
> +	mutex_unlock(&atmel_pwm->isr_lock);
>  
>  	clk_disable(atmel_pwm->clk);
>  	return ret;
> @@ -243,7 +253,22 @@ static int atmel_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>  static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>  	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
> +	unsigned long timeout = jiffies + 2 * HZ;
> +
> +	/*
> +	 * Wait for at least a complete period to have passed before disabling a
> +	 * channel to be sure that CDTY has been updated
> +	 */
> +	mutex_lock(&atmel_pwm->isr_lock);
> +	atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR);
> +
> +	while (!(atmel_pwm->updated_pwms & (1 << pwm->hwpwm)) &&
> +	       time_before(jiffies, timeout)) {
> +		usleep_range(10, 100);
> +		atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR);
> +	}
>  
> +	mutex_unlock(&atmel_pwm->isr_lock);
>  	atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm);
>  
>  	clk_disable(atmel_pwm->clk);
> @@ -358,6 +383,8 @@ static int atmel_pwm_probe(struct platform_device *pdev)
>  	atmel_pwm->chip.npwm = 4;
>  	atmel_pwm->chip.can_sleep = true;
>  	atmel_pwm->config = data->config;
> +	atmel_pwm->updated_pwms = 0;
> +	mutex_init(&atmel_pwm->isr_lock);
>  
>  	ret = pwmchip_add(&atmel_pwm->chip);
>  	if (ret < 0) {
> @@ -379,6 +406,7 @@ static int atmel_pwm_remove(struct platform_device *pdev)
>  	struct atmel_pwm_chip *atmel_pwm = platform_get_drvdata(pdev);
>  
>  	clk_unprepare(atmel_pwm->clk);
> +	mutex_destroy(&atmel_pwm->isr_lock);
>  
>  	return pwmchip_remove(&atmel_pwm->chip);
>  }
> 


-- 
Nicolas Ferre

Re: [PATCH 1/2] PWM: atmel: fix incorrect CDTY value after enabling

[Alexandre Belloni]

On 26/05/2015 at 11:01:33 +0200, Nicolas Ferre wrote :
> Le 25/05/2015 15:19, Alexandre Belloni a écrit :
> > CUPD is not flushed before enabling the channel so it will update CDTY/CPRD just
> > after one period. So we always set CUPD, even when the channel is not enabled.
> > 
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> 
> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> 
> BTW, is a "stable tag" needed for this one?
> 

I had a look and both can probably be backported up to 3.14.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 1/2] PWM: atmel: fix incorrect CDTY value after enabling

[Alexandre Belloni]

On 26/05/2015 at 11:01:33 +0200, Nicolas Ferre wrote :
> Le 25/05/2015 15:19, Alexandre Belloni a écrit :
> > CUPD is not flushed before enabling the channel so it will update CDTY/CPRD just
> > after one period. So we always set CUPD, even when the channel is not enabled.
> > 
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> 
> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> 
> BTW, is a "stable tag" needed for this one?
> 

I had a look and both can probably be backported up to 3.14.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 1/2] PWM: atmel: fix incorrect CDTY value after enabling

[Alexandre Belloni]

On 26/05/2015 at 11:01:33 +0200, Nicolas Ferre wrote :
> Le 25/05/2015 15:19, Alexandre Belloni a ?crit :
> > CUPD is not flushed before enabling the channel so it will update CDTY/CPRD just
> > after one period. So we always set CUPD, even when the channel is not enabled.
> > 
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> 
> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> 
> BTW, is a "stable tag" needed for this one?
> 

I had a look and both can probably be backported up to 3.14.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 1/2] PWM: atmel: fix incorrect CDTY value after enabling

[Thierry Reding]

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

On Mon, May 25, 2015 at 03:19:55PM +0200, Alexandre Belloni wrote:
> CUPD is not flushed before enabling the channel so it will update CDTY/CPRD just
> after one period. So we always set CUPD, even when the channel is not enabled.
> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
>  drivers/pwm/pwm-atmel.c | 35 ++++++++++++++++++-----------------
>  1 file changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index d3c22de9ee47..89f9ca41d9af 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -155,24 +155,25 @@ static void atmel_pwm_config_v1(struct pwm_chip *chip, struct pwm_device *pwm,
>  	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
>  	unsigned int val;
>  
> -	if (test_bit(PWMF_ENABLED, &pwm->flags)) {
> -		/*
> -		 * If the PWM channel is enabled, using the update register,
> -		 * it needs to set bit 10 of CMR to 0
> -		 */
> -		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CUPD, dty);
>  
> -		val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
> -		val &= ~PWM_CMR_UPD_CDTY;
> -		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
> -	} else {
> -		/*
> -		 * If the PWM channel is disabled, write value to duty and
> -		 * period registers directly.
> -		 */
> -		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CDTY, dty);
> -		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CPRD, prd);
> -	}
> +	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CUPD, dty);
> +
> +	val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
> +	val &= ~PWM_CMR_UPD_CDTY;
> +	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
> +
> +	/*
> +	 * If the PWM channel is enabled, only update CDTY by using the update
> +	 * register, it needs to set bit 10 of CMR to 0
> +	 */
> +	if (test_bit(PWMF_ENABLED, &pwm->flags))
> +		return;
> +	/*
> +	 * If the PWM channel is disabled, write value to duty and period
> +	 * registers directly.
> +	 */
> +	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CDTY, dty);
> +	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CPRD, prd);
>  }
>  
>  static void atmel_pwm_config_v2(struct pwm_chip *chip, struct pwm_device *pwm,

Applied, thanks.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 1/2] PWM: atmel: fix incorrect CDTY value after enabling

[Thierry Reding]

On Mon, May 25, 2015 at 03:19:55PM +0200, Alexandre Belloni wrote:
> CUPD is not flushed before enabling the channel so it will update CDTY/CPRD just
> after one period. So we always set CUPD, even when the channel is not enabled.
> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
>  drivers/pwm/pwm-atmel.c | 35 ++++++++++++++++++-----------------
>  1 file changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index d3c22de9ee47..89f9ca41d9af 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -155,24 +155,25 @@ static void atmel_pwm_config_v1(struct pwm_chip *chip, struct pwm_device *pwm,
>  	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
>  	unsigned int val;
>  
> -	if (test_bit(PWMF_ENABLED, &pwm->flags)) {
> -		/*
> -		 * If the PWM channel is enabled, using the update register,
> -		 * it needs to set bit 10 of CMR to 0
> -		 */
> -		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CUPD, dty);
>  
> -		val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
> -		val &= ~PWM_CMR_UPD_CDTY;
> -		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
> -	} else {
> -		/*
> -		 * If the PWM channel is disabled, write value to duty and
> -		 * period registers directly.
> -		 */
> -		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CDTY, dty);
> -		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CPRD, prd);
> -	}
> +	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CUPD, dty);
> +
> +	val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
> +	val &= ~PWM_CMR_UPD_CDTY;
> +	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
> +
> +	/*
> +	 * If the PWM channel is enabled, only update CDTY by using the update
> +	 * register, it needs to set bit 10 of CMR to 0
> +	 */
> +	if (test_bit(PWMF_ENABLED, &pwm->flags))
> +		return;
> +	/*
> +	 * If the PWM channel is disabled, write value to duty and period
> +	 * registers directly.
> +	 */
> +	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CDTY, dty);
> +	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CPRD, prd);
>  }
>  
>  static void atmel_pwm_config_v2(struct pwm_chip *chip, struct pwm_device *pwm,

Applied, thanks.

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

Re: [PATCHv2 2/2] PWM: atmel: fix incorrect CDTY value after disabling

[Thierry Reding]

On Mon, May 25, 2015 at 06:11:49PM +0200, Alexandre Belloni wrote:
> pwm-leds calls .config() and .disable() in a row. This exhibits that it may
> happen that the channel gets disabled before CDTY has been updated with CUPD.
> The issue gets quite worse with long periods.
> So, ensure that at least one period has past before disabling the channel by
> polling ISR.
> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
> I had the previous patch tested on a multicolor LED and unfortunately, the delay
> is not nice in that case. I wanted to avoid a mutex.
> This version uses a variable to store the state of the update of the pwm. This
> is reset everytime is gets a new configuration and updated when it is disabled
> or another pwm is configured.
> When disabling a multicolor LED, we only wait for the first pwm to update then
> we remember that we already seen the other one having their period expire and it
> looks much better.
> 
>  drivers/pwm/pwm-atmel.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)

Applied, thanks.

Thierry

[PATCHv2 2/2] PWM: atmel: fix incorrect CDTY value after disabling

[Thierry Reding]

On Mon, May 25, 2015 at 06:11:49PM +0200, Alexandre Belloni wrote:
> pwm-leds calls .config() and .disable() in a row. This exhibits that it may
> happen that the channel gets disabled before CDTY has been updated with CUPD.
> The issue gets quite worse with long periods.
> So, ensure that at least one period has past before disabling the channel by
> polling ISR.
> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
> I had the previous patch tested on a multicolor LED and unfortunately, the delay
> is not nice in that case. I wanted to avoid a mutex.
> This version uses a variable to store the state of the update of the pwm. This
> is reset everytime is gets a new configuration and updated when it is disabled
> or another pwm is configured.
> When disabling a multicolor LED, we only wait for the first pwm to update then
> we remember that we already seen the other one having their period expire and it
> looks much better.
> 
>  drivers/pwm/pwm-atmel.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)

Applied, thanks.

Thierry