* [PATCH] PWM: atmel: fix incorrect CDTY value after enabling or disabling
@ 2014-03-14 19:05 ` Alexandre Belloni
0 siblings, 0 replies; 8+ messages in thread
From: Alexandre Belloni @ 2014-03-14 19:05 UTC (permalink / raw)
To: Thierry Reding
Cc: linux-kernel, Nicolas Ferre, Bo Shen, linux-pwm,
linux-arm-kernel, 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 by reading ISR that at least one period has past before disabling the
channel.
The other issue is that it may happen that 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.
Tested on at91sam9g45 and sama5d31ek.
Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
drivers/pwm/pwm-atmel.c | 46 +++++++++++++++++++++++++++++-----------------
1 file changed, 29 insertions(+), 17 deletions(-)
diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index 0adc952cc4ef..0e589594b1cb 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -16,11 +16,13 @@
#include <linux/platform_device.h>
#include <linux/pwm.h>
#include <linux/slab.h>
+#include <linux/delay.h>
/* The following is global registers for PWM controller */
#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
@@ -157,24 +159,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,
@@ -245,6 +248,15 @@ static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
{
struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
+ /*
+ * 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)))
+ usleep_range(10, 100);
+
atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm);
clk_disable(atmel_pwm->clk);
--
1.8.3.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] PWM: atmel: fix incorrect CDTY value after enabling or disabling
@ 2014-03-14 19:05 ` Alexandre Belloni
0 siblings, 0 replies; 8+ messages in thread
From: Alexandre Belloni @ 2014-03-14 19:05 UTC (permalink / raw)
To: linux-arm-kernel
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 by reading ISR that at least one period has past before disabling the
channel.
The other issue is that it may happen that 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.
Tested on at91sam9g45 and sama5d31ek.
Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
drivers/pwm/pwm-atmel.c | 46 +++++++++++++++++++++++++++++-----------------
1 file changed, 29 insertions(+), 17 deletions(-)
diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index 0adc952cc4ef..0e589594b1cb 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -16,11 +16,13 @@
#include <linux/platform_device.h>
#include <linux/pwm.h>
#include <linux/slab.h>
+#include <linux/delay.h>
/* The following is global registers for PWM controller */
#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
@@ -157,24 +159,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,
@@ -245,6 +248,15 @@ static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
{
struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
+ /*
+ * 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)))
+ usleep_range(10, 100);
+
atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm);
clk_disable(atmel_pwm->clk);
--
1.8.3.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] PWM: atmel: fix incorrect CDTY value after enabling or disabling
2014-03-14 19:05 ` Alexandre Belloni
@ 2014-08-25 10:15 ` Thierry Reding
-1 siblings, 0 replies; 8+ messages in thread
From: Thierry Reding @ 2014-08-25 10:15 UTC (permalink / raw)
To: Alexandre Belloni
Cc: linux-kernel, Nicolas Ferre, Bo Shen, linux-pwm, linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 1090 bytes --]
On Fri, Mar 14, 2014 at 08:05:31PM +0100, 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 by reading ISR that at least one period has past before disabling the
> channel.
>
> The other issue is that it may happen that 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.
>
> Tested on at91sam9g45 and sama5d31ek.
>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
> drivers/pwm/pwm-atmel.c | 46 +++++++++++++++++++++++++++++-----------------
> 1 file changed, 29 insertions(+), 17 deletions(-)
Hi Alexandre,
going through the list of unapplied patches I came across this old
patch. It was never reviewed nor acked by anyone and you didn't ping me,
so I always assumed it must no longer be required. Is that so?
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] PWM: atmel: fix incorrect CDTY value after enabling or disabling
@ 2014-08-25 10:15 ` Thierry Reding
0 siblings, 0 replies; 8+ messages in thread
From: Thierry Reding @ 2014-08-25 10:15 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Mar 14, 2014 at 08:05:31PM +0100, 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 by reading ISR that at least one period has past before disabling the
> channel.
>
> The other issue is that it may happen that 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.
>
> Tested on at91sam9g45 and sama5d31ek.
>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
> drivers/pwm/pwm-atmel.c | 46 +++++++++++++++++++++++++++++-----------------
> 1 file changed, 29 insertions(+), 17 deletions(-)
Hi Alexandre,
going through the list of unapplied patches I came across this old
patch. It was never reviewed nor acked by anyone and you didn't ping me,
so I always assumed it must no longer be required. Is that so?
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/20140825/98b64092/attachment.sig>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] PWM: atmel: fix incorrect CDTY value after enabling or disabling
2014-08-25 10:15 ` Thierry Reding
@ 2014-08-25 11:54 ` Alexandre Belloni
-1 siblings, 0 replies; 8+ messages in thread
From: Alexandre Belloni @ 2014-08-25 11:54 UTC (permalink / raw)
To: Thierry Reding
Cc: linux-kernel, Nicolas Ferre, Bo Shen, linux-pwm, linux-arm-kernel
Hi,
On 25/08/2014 at 12:15:23 +0200, Thierry Reding wrote :
> On Fri, Mar 14, 2014 at 08:05:31PM +0100, 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 by reading ISR that at least one period has past before disabling the
> > channel.
> >
> > The other issue is that it may happen that 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.
> >
> > Tested on at91sam9g45 and sama5d31ek.
> >
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > ---
> > drivers/pwm/pwm-atmel.c | 46 +++++++++++++++++++++++++++++-----------------
> > 1 file changed, 29 insertions(+), 17 deletions(-)
>
> going through the list of unapplied patches I came across this old
> patch. It was never reviewed nor acked by anyone and you didn't ping me,
> so I always assumed it must no longer be required. Is that so?
>
It is still required but Nicolas is not happy with the polling on
PWM_ISR and he was supposed to discuss that internally with the IP
designer to understand if there is a better way.
I'll either ping on that one or send a new version when I'll know a bit
more.
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] PWM: atmel: fix incorrect CDTY value after enabling or disabling
@ 2014-08-25 11:54 ` Alexandre Belloni
0 siblings, 0 replies; 8+ messages in thread
From: Alexandre Belloni @ 2014-08-25 11:54 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On 25/08/2014 at 12:15:23 +0200, Thierry Reding wrote :
> On Fri, Mar 14, 2014 at 08:05:31PM +0100, 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 by reading ISR that at least one period has past before disabling the
> > channel.
> >
> > The other issue is that it may happen that 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.
> >
> > Tested on at91sam9g45 and sama5d31ek.
> >
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > ---
> > drivers/pwm/pwm-atmel.c | 46 +++++++++++++++++++++++++++++-----------------
> > 1 file changed, 29 insertions(+), 17 deletions(-)
>
> going through the list of unapplied patches I came across this old
> patch. It was never reviewed nor acked by anyone and you didn't ping me,
> so I always assumed it must no longer be required. Is that so?
>
It is still required but Nicolas is not happy with the polling on
PWM_ISR and he was supposed to discuss that internally with the IP
designer to understand if there is a better way.
I'll either ping on that one or send a new version when I'll know a bit
more.
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] PWM: atmel: fix incorrect CDTY value after enabling or disabling
2014-08-25 11:54 ` Alexandre Belloni
@ 2014-08-25 12:35 ` Thierry Reding
-1 siblings, 0 replies; 8+ messages in thread
From: Thierry Reding @ 2014-08-25 12:35 UTC (permalink / raw)
To: Alexandre Belloni
Cc: linux-kernel, Nicolas Ferre, Bo Shen, linux-pwm, linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 1601 bytes --]
On Mon, Aug 25, 2014 at 01:54:54PM +0200, Alexandre Belloni wrote:
> Hi,
>
> On 25/08/2014 at 12:15:23 +0200, Thierry Reding wrote :
> > On Fri, Mar 14, 2014 at 08:05:31PM +0100, 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 by reading ISR that at least one period has past before disabling the
> > > channel.
> > >
> > > The other issue is that it may happen that 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.
> > >
> > > Tested on at91sam9g45 and sama5d31ek.
> > >
> > > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > > ---
> > > drivers/pwm/pwm-atmel.c | 46 +++++++++++++++++++++++++++++-----------------
> > > 1 file changed, 29 insertions(+), 17 deletions(-)
> >
> > going through the list of unapplied patches I came across this old
> > patch. It was never reviewed nor acked by anyone and you didn't ping me,
> > so I always assumed it must no longer be required. Is that so?
> >
>
> It is still required but Nicolas is not happy with the polling on
> PWM_ISR and he was supposed to discuss that internally with the IP
> designer to understand if there is a better way.
>
> I'll either ping on that one or send a new version when I'll know a bit
> more.
Okay, thanks.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] PWM: atmel: fix incorrect CDTY value after enabling or disabling
@ 2014-08-25 12:35 ` Thierry Reding
0 siblings, 0 replies; 8+ messages in thread
From: Thierry Reding @ 2014-08-25 12:35 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Aug 25, 2014 at 01:54:54PM +0200, Alexandre Belloni wrote:
> Hi,
>
> On 25/08/2014 at 12:15:23 +0200, Thierry Reding wrote :
> > On Fri, Mar 14, 2014 at 08:05:31PM +0100, 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 by reading ISR that at least one period has past before disabling the
> > > channel.
> > >
> > > The other issue is that it may happen that 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.
> > >
> > > Tested on at91sam9g45 and sama5d31ek.
> > >
> > > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > > ---
> > > drivers/pwm/pwm-atmel.c | 46 +++++++++++++++++++++++++++++-----------------
> > > 1 file changed, 29 insertions(+), 17 deletions(-)
> >
> > going through the list of unapplied patches I came across this old
> > patch. It was never reviewed nor acked by anyone and you didn't ping me,
> > so I always assumed it must no longer be required. Is that so?
> >
>
> It is still required but Nicolas is not happy with the polling on
> PWM_ISR and he was supposed to discuss that internally with the IP
> designer to understand if there is a better way.
>
> I'll either ping on that one or send a new version when I'll know a bit
> more.
Okay, 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/20140825/11b7fcf1/attachment.sig>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-08-25 12:35 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-03-14 19:05 [PATCH] PWM: atmel: fix incorrect CDTY value after enabling or disabling Alexandre Belloni
2014-03-14 19:05 ` Alexandre Belloni
2014-08-25 10:15 ` Thierry Reding
2014-08-25 10:15 ` Thierry Reding
2014-08-25 11:54 ` Alexandre Belloni
2014-08-25 11:54 ` Alexandre Belloni
2014-08-25 12:35 ` Thierry Reding
2014-08-25 12:35 ` Thierry Reding
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.