i.MX53 (also i.MX6) PWM Glitches on Duty Cycle Change

[TomE]

I'm using PWM on the i.MX53 as an LCD Backlight Driver control.

When the brightness is ramped up it all works fine. When it is ramped down there are glitches.

That happens a lot on old and cheap PWM timers. What it means is that the Comparison Count register was reloaded just as the counter was approaching the previous value in that register and about to get a match. But the reload jumped the comparison value "back over" the counter, and so it ran to the end of the cycle without getting a match and without turning the output off. Hence you get one full cycle at 100%, causing a one-cycle "glitch".

Better PWM controllers keep a copy of the Sample/Comparison value, and only replace it at the end of a cycle so there are no glitches. With cheaper controllers you have to write code to take an interrupt at the end of a cycle and load the new value then. This goes badly wrong at low duty-cycles when the PWM counter beats the interrupt routine, or the interrupt gets delayed.

Here's an example of the glitch happening on a controller that doesn't buffer the sample value. The Yellow line is a falling ramp value which is the brightness of the screen. The Pink is the PWM output. You can see one full-length cycle when it missed the comparison.

Except that's the PWM controller in the i.MX53. It has a four-deep FIFO that is MEANT to stop this from happening. Except the Data Sheet is contradictory on how it works.

This bit implies "there's a FIFO and it works like you'd expect so as to prevent glitches":

51.7.5 PWM Sample Register (PWMx_PWMSAR)

The PWM will run at the last set duty-cycle setting if all the values of
the FIFO has been utilized, until the FIFO is reloaded or the PWM is disabled. When a
new value is written, the duty cycle changes after the current period is over.

But the HIGHLIGHTED bit in the following implies it uses the LIVE value in the FIFO and not a copy that it took at the start of the cycle (which it would have to do if it was to work properly):

51.4.1 Operation

At the beginning of a count period cycle, the PWMO pin is set to one (default) and the
counter begins counting up from 0x0000. The sample value in the sample FIFO is
compared on each count of prescaler clock. When the sample and count values match, the
PWMO signal is cleared to zero (default). The counter continues counting until the period
match occurs and subsequently another period cycle begins.

From the flashing on my screen and the oscilloscope trace above it looks like it doesn't work properly. If the FIFO is empty it takes the new value immediately. It only seems to buffer the sample when the FIFO is "loaded". That may be the case when sending a continuous waveform (music) through the PWM, but not when changing it intermittently when controlling a backlight.

I have found a work-around that proves my assertion.

If I read the FIFO then write the same value back (to load a value) and THEN load a new value in, it doesn't flash any more. Having the second value in the FIFO makes it wait until the end of the cycle. Of course the driver could keep a copy of the previous value and always write previous/next on an update.

That looks like a silicon bug to me. I've checked the i.MX53 and i.MX6 Errata and the PWM isn't listed. At the very least the chip isn't doing what the first quote from the manual above states.

Edit: I've just found the following Forum post which shows the same problem on the i.MX6"

PWM glitches on iMX6

There are two fixes listed there. First invert the PWM signal so it glitches DOWN instead of glitching UP (but it still glitches). Secondly, setting the REPEAT bits seems to fix this as well.

Tom

[geoff3]

Hi all, just to say here's a kernel module that uses the SDMA to drive multiple PWM channels for LED applications:

GitHub - unumotors/kernel-module-imx-pwm-led 

Thanks to TomE‌ for the hint of loading the current duty as the first FIFO sample to avoid this issue!

[gabrielvalcazar]

Hi all,

I'm aware that this thread is quite old, but I'm seeing the same issue on an i.MX6UL using the v4.14.98 2.3.0 BSP. When reducing the duty cycle, sometimes the signal remains high. I applied Tom's workaround of writing the old duty cycle before writing the new one, and although the issue doesn't happen as often, it still happens quite frequently.

This issue is blocking an important project where the PWM is supposed to make a backlight's brightness changes as smooth as possible, without small flashes. Is there any known workaround for this issue?

[TomE]

I've just read through all the posts, and the conclusion that I come to is that the PWM module in these chips isn't usable for this. It isn't possibly to program it so that it "just works" like you'd expect a PWM controller to. The only thing it is meant to support is "continuously streaming audio", where the FIFO is kept full. Obviously that's a job for the (extremely complicated) SDMA controller, but in the i.MX53, the PWM controller wasn't connected to the SDMA controller. Looking at "3.3 SDMA event mapping" in the i.MX6UL Manual, it still isn't.

So you shouldn't even try to use the PWM controller to control a backlight that you need to adjust. It isn't suitable. Instead you should look at other timer resources in the chip. I don't think the EPITs are suitable. Maybe you could use the SAI and the MQS to generate usable PWM signals.

Given that you've probably spent the money and time to make a design and a circuit board with existing connection that assumed the PWM would work, it's too late and you're trying to get it working somehow. Just like I did.

> When reducing the duty cycle, sometimes the signal remains high

That's ambiguous. "Remains high forever" or "remains high for one cycle"? I'm assuming the latter. Otherwise there's some other nasty bug outside of this specific problem.

Is it flashing at any time during your lowering, or only at high or low brightness? These "glitches" are obviously more visible at the low end, but it would be interesting to see if they're more frequent at one end of the scale.

Since this controller can't be used "generally and in all cases" (see my notes above on the impossibility of making patches that would work for everyone), any fixes has to be highly customised to the specific use you're putting it to. Specifically, what is your PWM Frequency, and what's the frequency of how often you're updating the duty cycle? Without any patches, you have to "update" less frequently that the PWM frequency. With my "jam the value in twice" modification you have to update at less than half the PWM rate.

Reading through my changes, there's an obvious bug, or more accurately a race condition. It is possible for the PWM controller to read the FIFO between the first and second values being jammed in. Except that's probably safe as it won't read the next one until one whole period later. What isn't safe is that the PWM driver can probably take an interrupt between those two writes, and it may take a long time until it returns to put the second one in. If that's longer than the PWM period, then you've just returned to the original problem. Which would make it still Flash, but less often then otherwise. With our product, the brightness is only stepped manually and not ramped. So the change I made reduced the frequency of the flashing enough so it doesn't matter to us. You're "ramping" dynamically, so the likelihood of this problem happening is higher in your case.

I'd suggest two changes. First, the brutal-but-simple one. Jam TWO copies of the previous value into the FIFO instead of just one. Make sure your update rate is less than 1/3 of the PWM frequency. Try that as a quick fix. What you're doing is doubling the interrupt duration that would cause the Flash, but not reducing it entirely.

The proper fix is to disable interrupts around the two writes to "MX3_PWMSAR". Good luck finding which interrupt locking calls to use. "spin_lock_irqsave()" and "spin_unlock_irqrestore()" are probably the one you need. Look for examples in other drivers to copy from. You'll need to add a "spin_lock_init()" lock variable to an appropriate structure somewhere. Since you're not running SMP these probably turn into something really simple (like disabling the CPU interrupts), but that's really hard to prove, or to find good documentation about.

Good luck.

Tom

[gabrielvalcazar]

Hi Tom,

Many thanks for the quick and informative reply.

When I said that sometimes the signal remains high, I meant to say for one whole cycle only (which is the glitch mentioned in the original post of this thread).

I don't have the exact period or duty cycle change frequency for this specific use case, since it's for a client using an i.MX-based solution. I have requested the info as it will definitely come in handy when deciding which workaround to choose. The client has mentioned that the issue (PWM signal high for one whole cycle/flashing) is highly reproducible when reducing the duty cycle, regardless of its original value.

Writing two copies of the previous value resulted in the issue happening even more frequently, but disabling interrupts while writing the values to "MX3_PWMSAR" seems to do the trick (the calls you mentioned were spot on!). I've been running a continuous test for over an hour now and the glitch hasn't happened a single time yet. The test consists of a shell script that modifies the parameters of a PWM via sysfs so that it runs with a period of 100 KHz, and it reduces the duty cycle continuously in 10% decrements (from 90% of the total period until 10%, then back to 90%). There are no delays between loops, so the duty cycle is being modified as quickly as the shell can allow (which is still less than half of the PWM's period).

Here's the patch I'm using:

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index e67a6172deea..f9d19a66b0a4 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -21,6 +21,7 @@
 #include <linux/pwm.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/spinlock.h>
 
 /* i.MX1 and i.MX21 share the same PWM function block: */
 
@@ -57,6 +58,9 @@ struct imx_chip {
 
        void __iomem    *mmio_base;
 
+       unsigned long   duty_previous;
+       spinlock_t lock;
+
        struct pwm_chip chip;
 };
 
@@ -202,7 +206,7 @@ static void imx_pwm_wait_fifo_slot(struct pwm_chip *chip,
 static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
                            struct pwm_state *state)
 {
-       unsigned long period_cycles, duty_cycles, prescale;
+       unsigned long period_cycles, duty_cycles, prescale, flags;
        struct imx_chip *imx = to_imx_chip(chip);
        struct pwm_state cstate;
        unsigned long long c;
@@ -249,8 +253,23 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
                        imx_pwm_sw_reset(chip);
                }
 
+               spin_lock_irqsave(&imx->lock, flags);
+
+               /*
+                * PWM generates a bad glitch on a duty cycle update for
+                * a lower duty cycle as there is no synchronisation on
+                * either period or frequency updates. The only way around
+                * this that doesn't involve spinning in a loop trying
+                * software synchronisation is to load up the FIFO. So
+                * on dropping duty-cycle, load FIFO with previous value.
+                */
+               if (duty_cycles < imx->duty_previous)
+                       writel(imx->duty_previous, imx->mmio_base + MX3_PWMSAR);
                writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
                writel(period_cycles, imx->mmio_base + MX3_PWMPR);
+               spin_unlock_irqrestore(&imx->lock, flags);
+
+               imx->duty_previous = duty_cycles;
 
                cr = MX3_PWMCR_PRESCALER(prescale) |
                     MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |

Again, many thanks for the reply. Best regards,

Gabriel

[TomE]

Congratulations on implementing and testing it so quickly. The interrupt disable and enable should be "the complete fix". There's no "tuning" necessary for that problem.

Which I've just realised isn't the interrupt on its own and how long it takes to execute. It's that when the interrupt happens, that task is a candidate for being context-switched to something else.

I've also checked the current kernel for the other problem that I ran into, which was the brightness arithmetic overflowing in 32 bits. That part of the code (in pwm_backlight_update_status() in drivers/video/backlight/pwm_bl.c) seems to have been rewritten, but I can't prove that it has been fixed. The backlight setup we used defaulted to having only 100 steps. We have a product that required 10,000:1 dimming, and so that maximum (max_brightness) had to increase from 100 to 10,000. That showed up the problems with the 32-bit arithmetic. That problem might still be there as far as I know. If your period in nanoseconds multiplied by the max_brightness overflows 32 bits it does what you'd expect.

In case you're wondering, getting LCD backlight hardware that can work properly with a 10,000:1 dimming ratio was interesting in itself.

Tom

[Pavel]

We met similar problem using i.MX27.

We have the following answer from the factory about this problem:

This code is result of all conditions happens:

- FIFO is empty after ROV_INT occur

- Current PWMCNR register smaller than current PWMSAR value

- New value is smaller than current PWMSAR value

- New value is smaller than current PWMSNR value

Have a great day,
Pavel

-----------------------------------------------------------------------------------------------------------------------
Note: If this post answers your question, please click the Correct Answer button. Thank you!
-----------------------------------------------------------------------------------------------------------------------

[TomE]

Pavel Chubakov wrote:

> We have the following answer from the factory about this problem:

Thank you for that data point.

Since that part came out in 2007 I'd guess Freescale has known about this problem for 8 years now.

Edit: This PWM controller is in the i.MX31 and that came out in 2005. 10 years then.

There's nothing in the documentation, no errata, and no handling of this problem in the Freescale-supplied and modified Linux Driver code.

Does "the factory" have any suggestions on workarounds?

Tom

[Pavel]

Your code should exclude that all conditions happens.

See attachment.

Have a great day,
Pavel

-----------------------------------------------------------------------------------------------------------------------
Note: If this post answers your question, please click the Correct Answer button. Thank you!
-----------------------------------------------------------------------------------------------------------------------

[TomE]

> Your code should exclude that all conditions happens.

Could you please try that sentence again. It is meaningless without context and details.

> See attachment.

I see you wrote that document in October 2011. Who was it written for? Was it to document a test you did, or to suggest a solution? It isn't very clear.

Why did you post that document? Was it meant to show workarounds or working driver code? It doesn't. Was it meant to demonstrate the problem? It does that in a rather forced way. I can't see anything in the document that helps to get around the problem.

It shows a "pwm_set()" function that has to be called ALL THE TIME in order to spot when a value has been removed from the FIFO in order to jam the next on in within a fraction of a microsecond.

That's one way to try and overcome the problems with this chip, but while it is doing that, the CPU can't possibly be doing anything else - like running user programs or Linux. It isn't at all practical. That isn't a solution for a Linux-based system.

Freescale already provides Linux Drivers for the PWM that don't handle this problem. The only practical advice would be a patched driver that avoids these problems properly. But I don't think that is possible with this chip.

After "Figure 2" the document says "If the application requires the comparison to be done, an alternative is to maintain the duty cycles of the signal but increase its period, "

What does that mean? Does it actually mean that the Application doesn't want the glitch, but can't spend 100% of its time in the PWM function, but needs to do something else? And how? There is no sample code given for this suggestion. The paragraph suggests changing the frequency, but "Figure 3" doesn't show any frequency change. The next section mentions changing a "+100" value, which isn't explained, and from my reading of the code isn't necessary anyway.

If this document was written to try and help a customer, how did they end up fixing their problem? If it was written to document the problem, what happened next?

Tom

[TomE]

Resolution and Frequency Interaction

As part of the code changes I've been making to get better control and resolution of the backlight, I wanted to use a low frequency with a high (meaning fine-grained) resolution.

The target was to use 220Hz and a "max_brightness" of 1000 steps, rather than the default of 100.

It didn't work. I could have a "max_brightness" of about 944, but no higher without serious and weird problems.

Which are due to this bit of code in drivers/video/backlight/pwm_bl.c:

static int pwm_backlight_update_status(struct backlight_device *bl)
{
    ...
    pwm_config(pb->pwm, brightness * pb->period / max, pb->period);

"brightness" and "max" are "int" (32-bit), but "pb->period" is an "unsigned int", so the "arithmetic conversion" might allow "brightness * period" to get to 2^32 or it might only get to 2^31 before it overflows and starts producing the wrong results.

Since the period is in nanosecond, this limits the maximum brightness to 2147483648/period_ns, or 214 at 100Hz. Or 2140 at 1kHz. And so on.

If you want to use 256 steps to match some external hardware, then the minimum frequency is about 120Hz.

Or change the above line to use 64-bit arithmetic like I did.

Those watching closely will notice I had a maximum count of 944 working at 220Hz, when from the above maths it should have only got to half of that. It looks like the limit is 2^32, but I'd rather avoid it altogether rather than being within a factor of 2 of it going wrong.

Tom

[TomE]

Curiouser and curiouser...

I'm now looking at the "pwm.c" code in the following branch of the following source:

http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/log/?h=imx_2.6.35_maintain

It behaves differently. It doesn't have the bug where it glitches HIGH for a full cycle when the period is changed.

Instead it changes the FREQUENCY! Or more accurately, it changes the PHASE of the waveform on every brightness change.

The main drivers/video/backlight/backlight.c file calls pwm_backlight_update_status() in whatever PWM module is compiled in.

That function has always looked something like this (barring power and regulator complications added later):

static int pwm_backlight_update_status(struct backlight_device *bl)
{
    ...
     if (brightness == 0) {
           pwm_config(pb->pwm, 0, pb->period);
           pwm_disable(pb->pwm);
      } else {
           pwm_config(pb->pwm, brightness * pb->period / max, pb->period);
           pwm_enable(pb->pwm);
      }

So changing the brightness calls pwm_config() then pwm_enable().

The version of the platform PWM code (which used to be in arch/arm/plat-mxc/pwm.c, but has recently moved to drivers/pwm/pwm-imx.c) used to look like this:

int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
{
    ...
    cr = MX3_PWMCR_PRESCALER(prescale) |
         MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
         MX3_PWMCR_DBGEN | MX3_PWMCR_EN;
int pwm_enable(struct pwm_device *pwm)
{
    ...
    if (!pwm->clk_enabled) {
        rc = clk_prepare_enable(pwm->clk);

It was then changed to this:

int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
{
    ...
    cr = MX3_PWMCR_PRESCALER(prescale) |
         MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEEN |
         MX3_PWMCR_WAITEN | MX3_PWMCR_DBGEN;
int pwm_enable(struct pwm_device *pwm)
{
    ...
        pwm->clk_enabled = 1;
    reg = readl(pwm->mmio_base + MX3_PWMCR);
    reg |= MX3_PWMCR_EN;
    writel(reg, pwm->mmio_base + MX3_PWMCR);

The older pwm_config() enabled the PWM module, or left it enabled and the pwm_enable() function did nothing.

The newer pwm_config() disables the PWM module and the following call to pwm_enable() starts it up again.

That results in a change to the brightness resetting the phase and doing this:

There are two places in the above where the brightness was changed and the waveform "restarted". The first one generated an over-long positive pulse with a short glitch in it, and the second one generated a short negative pulse.

Here's another example of what is an unexpected and unwanted output waveform from the PWM on a duty cycle change.

So you have a choice between the old code that glitches high, the newer code that glitches the phase/frequency, or a variation on my code which works without these problems.

The code has changed so much (and keeps changing) that it isn't possible to have anything like a "single patch" that will handle all the different variations. The addition of the "pwm: imx: Avoid sample FIFO overflow for i.MX PWM version2" in the mainline version just prior to v3.17rc1 complicates this even more.

And apparently by inspection (of the patch) changed the code from "phase glitch" back to "cycle glitch" as it leaves the PWM enabled after a call to pwm_config().

There are other bugs in previous versions the people may be using too.

The v3.1 code in pwm.c didn't handle the fact that the period is two cycles longer than the value loaded into the PWMPR register. That was fixed in v3.2

Meanwhile the Freescale code fixed this in a the following patch, which added a different bug:

Author: Yuxi Sun <b36102@freescale.com>  2011-12-15 13:12:53
Branches: imx_2.6.35_maintain
Follows: rel_imx_2.6.35_11.03.00
    ENGR00170342 PWM: fix pwm output can't be set to 100% full duty
           period_cycles = c;
 
           prescale = period_cycles / 0x10000 + 1;
 
           period_cycles /= prescale;
-          c = (unsigned long long)period_cycles * duty_ns;
+          /* the chip document says the counter counts up to
+           * period_cycles + 1 and then is reset to 0, so the
+           *  actual period of the PWM wave is period_cycles + 2
+           */
+          c = (unsigned long long)(period_cycles + 2) * duty_ns;
           do_div(c, period_ns);
           duty_cycles = c;
 
           writel(duty_cycles, pwm->mmio_base + MX3_PWMSAR);
           writel(period_cycles, pwm->mmio_base + MX3_PWMPR);

The problem with the above is that it calculates the duty-cycle on the "+2" value of the period instead of the actual period.

For instance, with a period of 4 clocks and a duty of 50%, the correct values are PCMCR=6 and PWMSAR=2. The above calculates PWMCR=6 and PWMSAR=3, giving a 75% resulting duty cycle.

In the "usual case" of a backlight driver where the output frequency is low (less than 1kHz), the period is usually between 30,000 and 65536, so the error caused by this is minimal. But if you're generating a high frequency for power control it might cause a problem.

The Freescale "imx_2.6.35_maintain" branch has the above patch. The last "released" code for the i.MX53 (on the web page for the chip is "L2.6.35_11.09.01_ER" one, which also has the patch and h is probably this:

http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/log/?h=imx_2.6.35_11.09.01

Tom

[TomE]

> Secondly, setting the REPEAT bits seems to fix this as well.

Setting the REPEAT bits was asserted to fix the problem. I've tested this. It doesn't.

The "test code" is as simple as:

root@triton1:/sys/class/backlight/pwm-backlight.15# while true; do echo 50 > brightness; sleep 0.1; echo 90 > brightness; sleep 0.1; done

root@triton1:/sys/class/backlight/pwm-backlight.15# while true; do echo 50 > brightness; sleep 0.01; echo 90 > brightness; sleep 0.01; done

I have inverted dimming, so "90" is "10%" and the above switches between 10% and 50%. Here's what that looks like with the glitches going to 100% "occasionally", about every second try!

Here's the change to "arch/arm/plat-mxc/pwm.c" to add the Repeat Count:

#define MX3_PWMCR_CLKSRC_IPG      (1 << 16)
#define MX3_PWMCR_REPEAT_1        (0 << 1)
#define MX3_PWMCR_REPEAT_2        (1 << 1)
#define MX3_PWMCR_REPEAT_4        (2 << 1)
#define MX3_PWMCR_REPEAT_8        (3 << 1)
#define MX3_PWMCR_EN          (1 << 0)
...

        cr = MX3_PWMCR_PRESCALER(prescale) |
            MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
            MX3_PWMCR_DBGEN | MX3_PWMCR_EN |
            MX3_PWMCR_REPEAT_4;

Here it is with "REPEAT_2", still showing glitches:

With "REPEAT_*" and with the test code cycling slower at 0.1 second to allow the repeats to "drain". The trace is zoomed in on a glitch.

Here's a trace where I'm writing the old-then-new values into PWMSAR on each update. There are no glitches.

This is the only fix I know of for this problem.

The same PWM controller is also in at least the i.MX51. i.MX31 and i.MX27, but not the i.MX28.

Tom

[TomE]

I've found another post on this subject:

https://community.freescale.com/message/404268

In a follow up on that post, "igorpadykov" clarified his previous answer with:

my reply was that FIFO should be always

filled (not empty) and check issue with SDK.

So the PWM driver has to be written to enable and have a handler for PWM interrupts, and then try to keep the FIFO full (or at least not empty)? That's not clear from the Reference Manual, and isn't what the Linux Driver authors have done.

Here's a patch to fix this the way I've found works. It could be better written if the driver kept a copy of the previous duty cycle instead of reading it, as that wastes about 200 CPU cycles.

diff --git a/arm-linux/arch/arm/plat-mxc/pwm.c b/arm-linux/arch/arm/plat-mxc/pwm.c
index 6f1c258..cc48f43 100644
--- a/arm-linux/arch/arm/plat-mxc/pwm.c
+++ b/arm-linux/arch/arm/plat-mxc/pwm.c
@@ -72,7 +72,7 @@ int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
    if (pwm->pwm_type != IMX_PWM_TYPE_MX1_MX21) {
        unsigned long long c;
        unsigned long period_cycles, duty_cycles, prescale;
-       u32 cr;
+       u32 cr, tmp;
 
        c = clk_get_rate(pwm->clk);
        c = c * period_ns;
@@ -98,6 +98,9 @@ int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
        else
            period_cycles = 0;
 
+        /* PWM has bad glitch on duty cycle update unless FIFO loaded */
+       tmp = readl(pwm->mmio_base + MX3_PWMSAR);
+       writel(tmp, pwm->mmio_base + MX3_PWMSAR);
        writel(duty_cycles, pwm->mmio_base + MX3_PWMSAR);
        writel(period_cycles, pwm->mmio_base + MX3_PWMPR);

Tom

[TomE]

I added two lines of code in the above patch and also added two bugs. That's about average for Linux Driver programming.

1. Reading PWMSAR shouldn't be done when the PWM is disabled, and when controlled from the backlight code, the PWM is disabled whenever the value goes to zero.
2. The Brightness driver handles a set of zero with a call to change the level to zero and then another call to DISABLE the PWM. That leaves the PWM disabled with the previous non-zero value on the FIFO. That magically reappears when a non-zero brightness is requested.

I have a different patch that fixes the above, but the code is at work.

Tom

[TomE]

With those two bugs fixed, I'm now using this patch, which is on the 3.4 mainline kernel:

diff --git a/arm-linux/arch/arm/plat-mxc/pwm.c b/arm-linux/arch/arm/plat-mxc/pwm.c
index 6f1c258..a41e212 100644
--- a/arm-linux/arch/arm/plat-mxc/pwm.c
+++ b/arm-linux/arch/arm/plat-mxc/pwm.c
@@ -41,6 +41,7 @@
 #define MX3_PWMCR_POUTC(n)   (((n) & 3) << 18)
 #define MX3_PWMCR_CLKSRC_IPG_HIGH (2 << 16)
 #define MX3_PWMCR_CLKSRC_IPG     (1 << 16)
+#define MX3_PWMCR_SWR        (1 << 3)
 #define MX3_PWMCR_EN         (1 << 0)
 
 enum imx_pwm_type {
@@ -62,6 +63,7 @@ struct pwm_device {
 
    unsigned int    use_count;
    unsigned int    pwm_id;
+   unsigned long   duty_previous;
 };
 
 int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
@@ -98,6 +100,16 @@ int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
        else
            period_cycles = 0;
 
+       /* PWM generates a bad glitch on a duty cycle update for
+        * a lower duty cycle as there is no synchronisation on
+        * either period or frequency updates. The only way around
+        * this that doesn't involve spinning in a loop trying
+        * software synchronisation is to load up the FIFO. So
+        * on dropping duty-cycle, load FIFO with previous value.
+        */
+       if (duty_cycles < pwm->duty_previous)
+           writel(pwm->duty_previous, pwm->mmio_base + MX3_PWMSAR);
+       pwm->duty_previous = duty_cycles;
        writel(duty_cycles, pwm->mmio_base + MX3_PWMSAR);
        writel(period_cycles, pwm->mmio_base + MX3_PWMPR);
 
@@ -154,8 +166,9 @@ EXPORT_SYMBOL(pwm_enable);
 
 void pwm_disable(struct pwm_device *pwm)
 {
-   writel(0, pwm->mmio_base + MX3_PWMCR);
-
+   /* Reset the PWM to empty the FIFO */
+   writel(MX3_PWMCR_SWR, pwm->mmio_base + MX3_PWMCR);
+   pwm->duty_previous = 0;
    if (pwm->clk_enabled) {
        clk_disable_unprepare(pwm->clk);
        pwm->clk_enabled = 0;

The above patch is on a older tree that doesn't contain the "pwm: imx: Avoid sample FIFO overflow for i.MX PWM version2" patch:

http://permalink.gmane.org/gmane.linux.pwm/832

It would have to be rewritten to be added to that version.

This patch would also halve the maximum duty cycle update rate when the duty cycle is falling, so it may not be appropriate for some usages.

Tom

[fabio_estevam]

Hi Tom,

Converting your fix to the mainline kernel we would have:

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c index 66d6f0c..aa8fd - Pastebin.com 

Care to submit it as a formal patch to the linux-pwm mailing list?

Thanks

[TomE]

Fabio Estevam wrote:

> Converting your fix to the mainline kernel we would have:

...

> Care to submit it as a formal patch to the linux-pwm mailing list?

No, because that patch makes the change that I demonstrated in my post DOESN'T work. I have just posted my actual patch that does work. I hope that makes it clearer.

Qiang Li​ has just posted a similar patch in the other thread - I've applied it and shown that it didn't work, and replied in that thread.

I'd prefer it if Freescale posted any resulting patch for this problem.

But note that enabling a *2 or *4 Repeat Count in the driver could fix the issue for some customers while wrecking the driver for others who need to update the duty cycle more rapidly. My suggested change would also halve the maximum update rate. it works for me, but not for everybody.

A better thought out patch would read PWMSR[FIFOAV] and only jam a previous copy in if the FIFO is empty ... that would of course cause a race condition, so it would have to jam in a copy if there's none or one word in the FIFO.

Tom

[fabio_estevam]

Ok, Tom. It is clear now.

I can locate the patch you made which seems to work for you.

If you feel confident with this change and would like to submit it upstream for review/comments, just let me know and I can help with this if you want.

[TomE]

Thank you.

The problem with a patch like that is that it may fix MY problem, but changes the driver in a way that it may badly affect other users.

Since my patch double-loads the FIFO on every write, that means the PWM can only be updated at half its original rate. Anyone who has code (say) running the PWM at 100Hz would now only be able to change the duty (in /sys/class/backlight/whatever/brightness) at 50Hz. Any faster and they'd trigger a FIFO write error. That patch could break existing user code.

A more complicated change could be something like:

if ((duty_cycle < duty_cycle_prev) &&
    (duty_cycle_prev > readl(pwm->mmio_base + MX3_PWMCNR) &&
    (readl((pwm->mmio_base + MX3_PWMSR) & MX3_PWM_SAR_FIFOAV) <= 2))
{
    writel(duty_cycle_prev, pwm->mmio_base + MX3_PWMSAR
}
writel(duty_cycles, pwm->mmio_base + MX3_PWMSAR);

The "glitch" only happens when the duty cycle is decreasing, so the above checks for that. It also checks to see if the current counter is above the previous sample point (already switched), and also that the FIFO isn't already loaded.

So can you see all the bugs in that code? The problem is that the counter value is continuously changing, the frequency can be high and there are corner-cases all over, I'm checking for the FIFO depth of "<= 2" instead of "<= 1" because the FIFO might get emptied during or AFTER the test.

The test against the current counter value isn't valid either. The CPU is executing code at 800MHz, but you may not know it takes nearly 200ns [1] to read and write a peripheral register. So if the PWM is being clocked at 66MHz it may have counted up by about 20 in the time it took to execute that "if" statement above. That means you can't trust the comparison with the counter. With a small period it may have wrapped more than once in that time too. So any comparisons with the values or even the FIFO depth don't work at high clock rates. At "medium" frequencies it could be reliable on a single-core CPU, but it could fail on the Dual or Quad cores. I've read that Cache flushes from one core can lock up a second core for the duration of the flush, which can be in the millisecond range.

I can make a patch that works for me, with the current driver, using the PWM as a LED Backlight control at less than 1kHz and with infrequent updates (to the brightness). The patch would work for anyone else using the PWM for similar things. I can imagine cases where it won't work and may make things worse for some users.

To do this properly and professionally you'd need the code patch as well as a Device Tree entry to enable it. Not a CONFIG setting - that's "old school". That would require patches to all relevant "dts"/"dtsi" files for all chips using this part (mx27, mx3x, mx5x, mx6x) and a patch to "Documentation/devicetree/bindings/something" describing it the patch, limitations and corner-cases to be avoided.

I don't think the problems with this PWM controller can be reliably worked around. Not glitching is something the hardware has to support. The i.MX28 PWM controller handles this. The PWM controllers built into the ColdFire chips I've checked has this built in (MCF51 FTM and the MCF53xx ones). Ditto all the way back to the HCS08s.

They can all handle variable duty-cycle AND variable frequency without glitching. This one can't do either. It is documented to be "optimized to generate sound from stored sample audio

images and it can also generate tones", so I guess trying to use it to control an LED is my fault for not reading the manual.

I had an idea to fix this though. It should be "easy" to program the DMA Controller to keep the FIFO part full. Then the driver would only have to update the memory location the DMAC is reading from. Overkill, but it should be possible. Except the SDMA Controller is more complicated than most CPUs. There's 249 pages in the Reference Manual describing it. It is a CPU with 51 instructions. It has about 171 32-bit registers controlling it. It can perform DMA to 48 different peripherals ... but the PWM controllers aren't on that list!

Tom

Reference 1: i.MX53 i.RAM (OCRAM) seems very slow, test code included.