* [PATCH] pwm: imx: don't reprogram PWMSAR if PWM is disabled
@ 2014-07-23 8:09 Dmitry Eremin-Solenikov
2014-08-01 18:02 ` Dmitry Eremin-Solenikov
0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Eremin-Solenikov @ 2014-07-23 8:09 UTC (permalink / raw)
To: Sascha Hauer, Thierry Reding; +Cc: linux-pwm, Dmitry Eremin-Solenikov
From: Dmitry Eremin-Solenikov <dmitry_eremin@mentor.com>
Writing several values to PWMSAR register with PWM being disabled can
lead to FIFO (connected to PWMSAR) being overflown. Then after enabling
PWM, hardware will use stale values. Instead cache the duty cycles and
write them to the hardware only before enabling PWM.
Signed-off-by: Dmitry Eremin-Solenikov <dmitry_eremin@mentor.com>
---
drivers/pwm/pwm-imx.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index c735127..79c2b24 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -46,6 +46,7 @@ struct imx_chip {
struct clk *clk_ipg;
void __iomem *mmio_base;
+ unsigned long duty_cycles;
struct pwm_chip chip;
@@ -105,7 +106,7 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
{
struct imx_chip *imx = to_imx_chip(chip);
unsigned long long c;
- unsigned long period_cycles, duty_cycles, prescale;
+ unsigned long period_cycles, prescale;
u32 cr;
c = clk_get_rate(imx->clk_per);
@@ -118,7 +119,7 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
period_cycles /= prescale;
c = (unsigned long long)period_cycles * duty_ns;
do_div(c, period_ns);
- duty_cycles = c;
+ imx->duty_cycles = c;
/*
* according to imx pwm RM, the real period value should be
@@ -134,7 +135,8 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
period_cycles -= 2;
- writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
+ if (test_bit(PWMF_ENABLED, &pwm->flags))
+ writel(imx->duty_cycles, imx->mmio_base + MX3_PWMSAR);
writel(period_cycles, imx->mmio_base + MX3_PWMPR);
cr = readl(imx->mmio_base + MX3_PWMCR);
@@ -157,6 +159,9 @@ static void imx_pwm_set_enable_v2(struct pwm_chip *chip, bool enable)
struct imx_chip *imx = to_imx_chip(chip);
u32 val;
+ if (enable)
+ writel(imx->duty_cycles, imx->mmio_base + MX3_PWMSAR);
+
val = readl(imx->mmio_base + MX3_PWMCR);
if (enable)
--
1.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] pwm: imx: don't reprogram PWMSAR if PWM is disabled
2014-07-23 8:09 [PATCH] pwm: imx: don't reprogram PWMSAR if PWM is disabled Dmitry Eremin-Solenikov
@ 2014-08-01 18:02 ` Dmitry Eremin-Solenikov
2014-08-05 0:47 ` Dmitry Eremin-Solenikov
2014-08-05 1:36 ` Shawn Guo
0 siblings, 2 replies; 6+ messages in thread
From: Dmitry Eremin-Solenikov @ 2014-08-01 18:02 UTC (permalink / raw)
To: Sascha Hauer, Thierry Reding, Shawn Guo; +Cc: linux-pwm
On Wed, Jul 23, 2014 at 12:09 PM, Dmitry Eremin-Solenikov
<dbaryshkov@gmail.com> wrote:
> From: Dmitry Eremin-Solenikov <dmitry_eremin@mentor.com>
>
> Writing several values to PWMSAR register with PWM being disabled can
> lead to FIFO (connected to PWMSAR) being overflown. Then after enabling
> PWM, hardware will use stale values. Instead cache the duty cycles and
> write them to the hardware only before enabling PWM.
What about this patch?
>
> Signed-off-by: Dmitry Eremin-Solenikov <dmitry_eremin@mentor.com>
> ---
> drivers/pwm/pwm-imx.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index c735127..79c2b24 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -46,6 +46,7 @@ struct imx_chip {
> struct clk *clk_ipg;
>
> void __iomem *mmio_base;
> + unsigned long duty_cycles;
>
> struct pwm_chip chip;
>
> @@ -105,7 +106,7 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
> {
> struct imx_chip *imx = to_imx_chip(chip);
> unsigned long long c;
> - unsigned long period_cycles, duty_cycles, prescale;
> + unsigned long period_cycles, prescale;
> u32 cr;
>
> c = clk_get_rate(imx->clk_per);
> @@ -118,7 +119,7 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
> period_cycles /= prescale;
> c = (unsigned long long)period_cycles * duty_ns;
> do_div(c, period_ns);
> - duty_cycles = c;
> + imx->duty_cycles = c;
>
> /*
> * according to imx pwm RM, the real period value should be
> @@ -134,7 +135,8 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
>
> period_cycles -= 2;
>
> - writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> + if (test_bit(PWMF_ENABLED, &pwm->flags))
> + writel(imx->duty_cycles, imx->mmio_base + MX3_PWMSAR);
> writel(period_cycles, imx->mmio_base + MX3_PWMPR);
>
> cr = readl(imx->mmio_base + MX3_PWMCR);
> @@ -157,6 +159,9 @@ static void imx_pwm_set_enable_v2(struct pwm_chip *chip, bool enable)
> struct imx_chip *imx = to_imx_chip(chip);
> u32 val;
>
> + if (enable)
> + writel(imx->duty_cycles, imx->mmio_base + MX3_PWMSAR);
> +
> val = readl(imx->mmio_base + MX3_PWMCR);
>
> if (enable)
> --
> 1.9.3
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] pwm: imx: don't reprogram PWMSAR if PWM is disabled
2014-08-01 18:02 ` Dmitry Eremin-Solenikov
@ 2014-08-05 0:47 ` Dmitry Eremin-Solenikov
2014-08-05 1:36 ` Shawn Guo
1 sibling, 0 replies; 6+ messages in thread
From: Dmitry Eremin-Solenikov @ 2014-08-05 0:47 UTC (permalink / raw)
To: Sascha Hauer, Thierry Reding, Shawn Guo; +Cc: linux-pwm
On Fri, Aug 1, 2014 at 10:02 PM, Dmitry Eremin-Solenikov
<dmitry_eremin@mentor.com> wrote:
> On Wed, Jul 23, 2014 at 12:09 PM, Dmitry Eremin-Solenikov
> <dbaryshkov@gmail.com> wrote:
>> From: Dmitry Eremin-Solenikov <dmitry_eremin@mentor.com>
>>
>> Writing several values to PWMSAR register with PWM being disabled can
>> lead to FIFO (connected to PWMSAR) being overflown. Then after enabling
>> PWM, hardware will use stale values. Instead cache the duty cycles and
>> write them to the hardware only before enabling PWM.
>
> What about this patch?
For the reference: the issue with the FIFO can be easily demonstrated on i.MX6
with the help of pwm-backlight. Steps to reproduce:
1) Disable backlight: echo 1 > /sys/class/..../bl_power
2) Change config several times:
echo 1 > /sys/class/.../brightness
echo 2 > /sys/class/.../brightness
echo 3 > /sys/class/.../brightness
echo 4 > /sys/class/.../brightness
echo 7 > /sys/class/.../brightness
3) Reenable backlight and pwm:
echo 0 > /sys/class/.../bl_power
At this point you get PWMSAR = 0, because of FIFO overflow (can be
verified by reading PWM status register).
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] pwm: imx: don't reprogram PWMSAR if PWM is disabled
2014-08-01 18:02 ` Dmitry Eremin-Solenikov
2014-08-05 0:47 ` Dmitry Eremin-Solenikov
@ 2014-08-05 1:36 ` Shawn Guo
2014-08-05 8:48 ` Liu Ying
1 sibling, 1 reply; 6+ messages in thread
From: Shawn Guo @ 2014-08-05 1:36 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov; +Cc: Sascha Hauer, Thierry Reding, linux-pwm, Liu Ying
On Fri, Aug 01, 2014 at 10:02:17PM +0400, Dmitry Eremin-Solenikov wrote:
> On Wed, Jul 23, 2014 at 12:09 PM, Dmitry Eremin-Solenikov
> <dbaryshkov@gmail.com> wrote:
> > From: Dmitry Eremin-Solenikov <dmitry_eremin@mentor.com>
> >
> > Writing several values to PWMSAR register with PWM being disabled can
> > lead to FIFO (connected to PWMSAR) being overflown. Then after enabling
> > PWM, hardware will use stale values. Instead cache the duty cycles and
> > write them to the hardware only before enabling PWM.
>
> What about this patch?
Copy Liu Ying who seems to have a patch [1] addressing the same problem?
Shawn
[1] http://thread.gmane.org/gmane.linux.pwm/837/focus=836
>
> >
> > Signed-off-by: Dmitry Eremin-Solenikov <dmitry_eremin@mentor.com>
> > ---
> > drivers/pwm/pwm-imx.c | 11 ++++++++---
> > 1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > index c735127..79c2b24 100644
> > --- a/drivers/pwm/pwm-imx.c
> > +++ b/drivers/pwm/pwm-imx.c
> > @@ -46,6 +46,7 @@ struct imx_chip {
> > struct clk *clk_ipg;
> >
> > void __iomem *mmio_base;
> > + unsigned long duty_cycles;
> >
> > struct pwm_chip chip;
> >
> > @@ -105,7 +106,7 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
> > {
> > struct imx_chip *imx = to_imx_chip(chip);
> > unsigned long long c;
> > - unsigned long period_cycles, duty_cycles, prescale;
> > + unsigned long period_cycles, prescale;
> > u32 cr;
> >
> > c = clk_get_rate(imx->clk_per);
> > @@ -118,7 +119,7 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
> > period_cycles /= prescale;
> > c = (unsigned long long)period_cycles * duty_ns;
> > do_div(c, period_ns);
> > - duty_cycles = c;
> > + imx->duty_cycles = c;
> >
> > /*
> > * according to imx pwm RM, the real period value should be
> > @@ -134,7 +135,8 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
> >
> > period_cycles -= 2;
> >
> > - writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> > + if (test_bit(PWMF_ENABLED, &pwm->flags))
> > + writel(imx->duty_cycles, imx->mmio_base + MX3_PWMSAR);
> > writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> >
> > cr = readl(imx->mmio_base + MX3_PWMCR);
> > @@ -157,6 +159,9 @@ static void imx_pwm_set_enable_v2(struct pwm_chip *chip, bool enable)
> > struct imx_chip *imx = to_imx_chip(chip);
> > u32 val;
> >
> > + if (enable)
> > + writel(imx->duty_cycles, imx->mmio_base + MX3_PWMSAR);
> > +
> > val = readl(imx->mmio_base + MX3_PWMCR);
> >
> > if (enable)
> > --
> > 1.9.3
> >
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] pwm: imx: don't reprogram PWMSAR if PWM is disabled
2014-08-05 1:36 ` Shawn Guo
@ 2014-08-05 8:48 ` Liu Ying
2014-08-25 13:56 ` Thierry Reding
0 siblings, 1 reply; 6+ messages in thread
From: Liu Ying @ 2014-08-05 8:48 UTC (permalink / raw)
To: Shawn Guo, Dmitry Eremin-Solenikov
Cc: Sascha Hauer, Thierry Reding, linux-pwm
On 08/05/2014 09:36 AM, Shawn Guo wrote:
> On Fri, Aug 01, 2014 at 10:02:17PM +0400, Dmitry Eremin-Solenikov wrote:
>> On Wed, Jul 23, 2014 at 12:09 PM, Dmitry Eremin-Solenikov
>> <dbaryshkov@gmail.com> wrote:
>>> From: Dmitry Eremin-Solenikov <dmitry_eremin@mentor.com>
>>>
>>> Writing several values to PWMSAR register with PWM being disabled can
>>> lead to FIFO (connected to PWMSAR) being overflown. Then after enabling
>>> PWM, hardware will use stale values. Instead cache the duty cycles and
>>> write them to the hardware only before enabling PWM.
>>
>> What about this patch?
>
> Copy Liu Ying who seems to have a patch [1] addressing the same problem?
>
Yes, my patch may address the same problem. And, my patch may cache the
last duty cycle as well when the PWM is disabled. The difference is that
my patch caches it in the register PWMSAR instead of a variable.
Regards,
Liu Ying
> Shawn
>
> [1] http://thread.gmane.org/gmane.linux.pwm/837/focus=836
>
>>
>>>
>>> Signed-off-by: Dmitry Eremin-Solenikov <dmitry_eremin@mentor.com>
>>> ---
>>> drivers/pwm/pwm-imx.c | 11 ++++++++---
>>> 1 file changed, 8 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
>>> index c735127..79c2b24 100644
>>> --- a/drivers/pwm/pwm-imx.c
>>> +++ b/drivers/pwm/pwm-imx.c
>>> @@ -46,6 +46,7 @@ struct imx_chip {
>>> struct clk *clk_ipg;
>>>
>>> void __iomem *mmio_base;
>>> + unsigned long duty_cycles;
>>>
>>> struct pwm_chip chip;
>>>
>>> @@ -105,7 +106,7 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
>>> {
>>> struct imx_chip *imx = to_imx_chip(chip);
>>> unsigned long long c;
>>> - unsigned long period_cycles, duty_cycles, prescale;
>>> + unsigned long period_cycles, prescale;
>>> u32 cr;
>>>
>>> c = clk_get_rate(imx->clk_per);
>>> @@ -118,7 +119,7 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
>>> period_cycles /= prescale;
>>> c = (unsigned long long)period_cycles * duty_ns;
>>> do_div(c, period_ns);
>>> - duty_cycles = c;
>>> + imx->duty_cycles = c;
>>>
>>> /*
>>> * according to imx pwm RM, the real period value should be
>>> @@ -134,7 +135,8 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
>>>
>>> period_cycles -= 2;
>>>
>>> - writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
>>> + if (test_bit(PWMF_ENABLED, &pwm->flags))
>>> + writel(imx->duty_cycles, imx->mmio_base + MX3_PWMSAR);
>>> writel(period_cycles, imx->mmio_base + MX3_PWMPR);
>>>
>>> cr = readl(imx->mmio_base + MX3_PWMCR);
>>> @@ -157,6 +159,9 @@ static void imx_pwm_set_enable_v2(struct pwm_chip *chip, bool enable)
>>> struct imx_chip *imx = to_imx_chip(chip);
>>> u32 val;
>>>
>>> + if (enable)
>>> + writel(imx->duty_cycles, imx->mmio_base + MX3_PWMSAR);
>>> +
>>> val = readl(imx->mmio_base + MX3_PWMCR);
>>>
>>> if (enable)
>>> --
>>> 1.9.3
>>>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] pwm: imx: don't reprogram PWMSAR if PWM is disabled
2014-08-05 8:48 ` Liu Ying
@ 2014-08-25 13:56 ` Thierry Reding
0 siblings, 0 replies; 6+ messages in thread
From: Thierry Reding @ 2014-08-25 13:56 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov; +Cc: Shawn Guo, Liu Ying, Sascha Hauer, linux-pwm
[-- Attachment #1: Type: text/plain, Size: 1285 bytes --]
On Tue, Aug 05, 2014 at 04:48:41PM +0800, Liu Ying wrote:
> On 08/05/2014 09:36 AM, Shawn Guo wrote:
> >On Fri, Aug 01, 2014 at 10:02:17PM +0400, Dmitry Eremin-Solenikov wrote:
> >>On Wed, Jul 23, 2014 at 12:09 PM, Dmitry Eremin-Solenikov
> >><dbaryshkov@gmail.com> wrote:
> >>>From: Dmitry Eremin-Solenikov <dmitry_eremin@mentor.com>
> >>>
> >>>Writing several values to PWMSAR register with PWM being disabled can
> >>>lead to FIFO (connected to PWMSAR) being overflown. Then after enabling
> >>>PWM, hardware will use stale values. Instead cache the duty cycles and
> >>>write them to the hardware only before enabling PWM.
> >>
> >>What about this patch?
> >
> >Copy Liu Ying who seems to have a patch [1] addressing the same problem?
> >
>
> Yes, my patch may address the same problem. And, my patch may cache the last
> duty cycle as well when the PWM is disabled. The difference is that my patch
> caches it in the register PWMSAR instead of a variable.
Dmitry,
I've just pushed Liu's patch that might fix this problem. Can you give
it a quick spin to see if it fixes the issue that you're seeing? It's in
the for-next branch of the PWM tree[0].
Thanks,
Thierry
[0]: git://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-08-25 13:56 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-07-23 8:09 [PATCH] pwm: imx: don't reprogram PWMSAR if PWM is disabled Dmitry Eremin-Solenikov
2014-08-01 18:02 ` Dmitry Eremin-Solenikov
2014-08-05 0:47 ` Dmitry Eremin-Solenikov
2014-08-05 1:36 ` Shawn Guo
2014-08-05 8:48 ` Liu Ying
2014-08-25 13:56 ` Thierry Reding
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).