diff mbox series

[v6,1/1] pwm: imx27: workaround of the pwm output bug when decrease the duty cycle

Message ID 20240917192510.3031493-1-Frank.Li@nxp.com (mailing list archive)
State New, archived
Headers show
Series [v6,1/1] pwm: imx27: workaround of the pwm output bug when decrease the duty cycle | expand

Commit Message

Frank Li Sept. 17, 2024, 7:25 p.m. UTC 
From: Clark Wang <xiaoning.wang@nxp.com>

Implement workaround for ERR051198
(https://www.nxp.com/docs/en/errata/IMX8MN_0N14Y.pdf)

PWM output may not function correctly if the FIFO is empty when a new SAR
value is programmed

Description:
  When the PWM FIFO is empty, a new value programmed to the PWM Sample
  register (PWM_PWMSAR) will be directly applied even if the current timer
  period has not expired. If the new SAMPLE value programmed in the
  PWM_PWMSAR register is less than the previous value, and the PWM counter
  register (PWM_PWMCNR) that contains the current COUNT value is greater
  than the new programmed SAMPLE value, the current period will not flip
  the level. This may result in an output pulse with a duty cycle of 100%.

Workaround:
  Program the current SAMPLE value in the PWM_PWMSAR register before
  updating the new duty cycle to the SAMPLE value in the PWM_PWMSAR
  register. This will ensure that the new SAMPLE value is modified during
  a non-empty FIFO, and can be successfully updated after the period
  expires.

Write the old SAR value before updating the new duty cycle to SAR. This
avoids writing the new value into an empty FIFO.

This only resolves the issue when the PWM period is longer than 2us
(or <500kHz) because write register is not quick enough when PWM period is
very short.

Reproduce steps:
  cd /sys/class/pwm/pwmchip1/pwm0
  echo 2000000000 > period     # It is easy to observe by using long period
  echo 1000000000 > duty_cycle
  echo 1 > enable
  echo  800000000 > duty_cycle # One full high plus will be seen by scope

Fixes: 166091b1894d ("[ARM] MXC: add pwm driver for i.MX SoCs")
Reviewed-by: Jun Li <jun.li@nxp.com>
Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v5 to v6
- KHz to KHz
- sar to SAR
- move comments above if

Change from v4 to v5
- fix typo PMW & If
- using imx->mmio_base + MX3_PWMSAR

Change from v3 to v4
- none, wrong bump version number
Change from v2 to v3
- simple workaround implement.
- add reproduce steps.

Change from v1 to v2
- address comments in https://lore.kernel.org/linux-pwm/20211221095053.uz4qbnhdqziftymw@pengutronix.de/
  About disable/enable pwm instead of disable/enable irq:
  Some pmw periphal may sensitive to period. Disable/enable pwm will
increase period, althouhg it is okay for most case, such as LED backlight
or FAN speed. But some device such servo may require strict period.

- address comments in https://lore.kernel.org/linux-pwm/d72d1ae5-0378-4bac-8b77-0bb69f55accd@gmx.net/
  Using official errata number
  fix typo 'filp'
  add {} for else

I supposed fixed all previous issues, let me know if I missed one.
---
 drivers/pwm/pwm-imx27.c | 67 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 66 insertions(+), 1 deletion(-)

Comments

Marek Vasut Sept. 22, 2024, 8:28 p.m. UTC | #1 
Hi,

On 9/17/24 9:25 PM, Frank Li wrote:

[...]

> @@ -223,6 +224,8 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>   	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
>   	unsigned long long c;
>   	unsigned long long clkrate;
> +	unsigned long flags;
> +	int val;
>   	int ret;
>   	u32 cr;
>   
> @@ -263,7 +266,69 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,

[...]

> +	c = clkrate * 1500;
> +	do_div(c, NSEC_PER_SEC);
> +
> +	local_irq_save(flags);
> +	val = FIELD_GET(MX3_PWMSR_FIFOAV, readl_relaxed(imx->mmio_base + MX3_PWMSR));

I think the multi-write I mentioned in v5 for > 500 kHz case could 
further improve the patch, let's see what others think:

if (state->period < 2000) { /* 2000ns = 500 kHz */
    /* Best effort attempt to fix up >500 kHz case */
    udelay(6); /* 2us per FIFO entry, 3 FIFO entries written => 6 us */
    writel_relaxed(duty_cycles, imx->mmio_base + MX3_PWMSAR);
    writel_relaxed(duty_cycles, imx->mmio_base + MX3_PWMSAR);
    /* Last write is outside, after this conditional */
} else if (duty_cycles ...

> +	if (duty_cycles < imx->duty_cycle && val < MX3_PWMSR_FIFOAV_2WORDS) {
> +		val = readl_relaxed(imx->mmio_base + MX3_PWMCNR);
> +		/*
> +		 * If counter is close to period, controller may roll over when
> +		 * next IO write.
> +		 */

c is only used in this if (duty_cycles ...) { } conditional, the 
do_div() above can be moved here:

	c = clkrate * 1500;
	do_div(c, NSEC_PER_SEC);

> +		if ((val + c >= duty_cycles && val < imx->duty_cycle) ||
> +		    val + c >= period_cycles)
> +			writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR);
> +	}
> +	writel_relaxed(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> +	local_irq_restore(flags);
Frank Li Oct. 2, 2024, 7:45 p.m. UTC | #2 
On Sun, Sep 22, 2024 at 10:28:02PM +0200, Marek Vasut wrote:
> Hi,
>
> On 9/17/24 9:25 PM, Frank Li wrote:
>
> [...]
>
> > @@ -223,6 +224,8 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >   	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
> >   	unsigned long long c;
> >   	unsigned long long clkrate;
> > +	unsigned long flags;
> > +	int val;
> >   	int ret;
> >   	u32 cr;
> > @@ -263,7 +266,69 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>
> [...]
>
> > +	c = clkrate * 1500;
> > +	do_div(c, NSEC_PER_SEC);
> > +
> > +	local_irq_save(flags);
> > +	val = FIELD_GET(MX3_PWMSR_FIFOAV, readl_relaxed(imx->mmio_base + MX3_PWMSR));
>
> I think the multi-write I mentioned in v5 for > 500 kHz case could further
> improve the patch, let's see what others think:
>
> if (state->period < 2000) { /* 2000ns = 500 kHz */
>    /* Best effort attempt to fix up >500 kHz case */
>    udelay(6); /* 2us per FIFO entry, 3 FIFO entries written => 6 us */
>    writel_relaxed(duty_cycles, imx->mmio_base + MX3_PWMSAR);
>    writel_relaxed(duty_cycles, imx->mmio_base + MX3_PWMSAR);
>    /* Last write is outside, after this conditional */
> } else if (duty_cycles ...
>
> > +	if (duty_cycles < imx->duty_cycle && val < MX3_PWMSR_FIFOAV_2WORDS) {
> > +		val = readl_relaxed(imx->mmio_base + MX3_PWMCNR);
> > +		/*
> > +		 * If counter is close to period, controller may roll over when
> > +		 * next IO write.
> > +		 */
>
> c is only used in this if (duty_cycles ...) { } conditional, the do_div()
> above can be moved here:

It is in local_irq_save(flags) scope, it'd better as less as possible. So
I prefer do_div() is outside local_irq_save()

Frank

>
> 	c = clkrate * 1500;
> 	do_div(c, NSEC_PER_SEC);
>
> > +		if ((val + c >= duty_cycles && val < imx->duty_cycle) ||
> > +		    val + c >= period_cycles)
> > +			writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR);
> > +	}
> > +	writel_relaxed(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> > +	local_irq_restore(flags);
Marek Vasut Oct. 2, 2024, 7:48 p.m. UTC | #3 
On 10/2/24 9:45 PM, Frank Li wrote:
> On Sun, Sep 22, 2024 at 10:28:02PM +0200, Marek Vasut wrote:
>> Hi,
>>
>> On 9/17/24 9:25 PM, Frank Li wrote:
>>
>> [...]
>>
>>> @@ -223,6 +224,8 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>>>    	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
>>>    	unsigned long long c;
>>>    	unsigned long long clkrate;
>>> +	unsigned long flags;
>>> +	int val;
>>>    	int ret;
>>>    	u32 cr;
>>> @@ -263,7 +266,69 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>>
>> [...]
>>
>>> +	c = clkrate * 1500;
>>> +	do_div(c, NSEC_PER_SEC);
>>> +
>>> +	local_irq_save(flags);
>>> +	val = FIELD_GET(MX3_PWMSR_FIFOAV, readl_relaxed(imx->mmio_base + MX3_PWMSR));
>>
>> I think the multi-write I mentioned in v5 for > 500 kHz case could further
>> improve the patch, let's see what others think:
>>
>> if (state->period < 2000) { /* 2000ns = 500 kHz */
>>     /* Best effort attempt to fix up >500 kHz case */
>>     udelay(6); /* 2us per FIFO entry, 3 FIFO entries written => 6 us */
>>     writel_relaxed(duty_cycles, imx->mmio_base + MX3_PWMSAR);
>>     writel_relaxed(duty_cycles, imx->mmio_base + MX3_PWMSAR);
>>     /* Last write is outside, after this conditional */
>> } else if (duty_cycles ...

Can you have a look at this part ?

>>> +	if (duty_cycles < imx->duty_cycle && val < MX3_PWMSR_FIFOAV_2WORDS) {
>>> +		val = readl_relaxed(imx->mmio_base + MX3_PWMCNR);
>>> +		/*
>>> +		 * If counter is close to period, controller may roll over when
>>> +		 * next IO write.
>>> +		 */
>>
>> c is only used in this if (duty_cycles ...) { } conditional, the do_div()
>> above can be moved here:
> 
> It is in local_irq_save(flags) scope, it'd better as less as possible. So
> I prefer do_div() is outside local_irq_save()
Good point, either way is fine by me.
Frank Li Oct. 2, 2024, 7:57 p.m. UTC | #4 
On Wed, Oct 02, 2024 at 09:48:12PM +0200, Marek Vasut wrote:
> On 10/2/24 9:45 PM, Frank Li wrote:
> > On Sun, Sep 22, 2024 at 10:28:02PM +0200, Marek Vasut wrote:
> > > Hi,
> > >
> > > On 9/17/24 9:25 PM, Frank Li wrote:
> > >
> > > [...]
> > >
> > > > @@ -223,6 +224,8 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > >    	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
> > > >    	unsigned long long c;
> > > >    	unsigned long long clkrate;
> > > > +	unsigned long flags;
> > > > +	int val;
> > > >    	int ret;
> > > >    	u32 cr;
> > > > @@ -263,7 +266,69 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > >
> > > [...]
> > >
> > > > +	c = clkrate * 1500;
> > > > +	do_div(c, NSEC_PER_SEC);
> > > > +
> > > > +	local_irq_save(flags);
> > > > +	val = FIELD_GET(MX3_PWMSR_FIFOAV, readl_relaxed(imx->mmio_base + MX3_PWMSR));
> > >
> > > I think the multi-write I mentioned in v5 for > 500 kHz case could further
> > > improve the patch, let's see what others think:
> > >
> > > if (state->period < 2000) { /* 2000ns = 500 kHz */
> > >     /* Best effort attempt to fix up >500 kHz case */
> > >     udelay(6); /* 2us per FIFO entry, 3 FIFO entries written => 6 us */
> > >     writel_relaxed(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> > >     writel_relaxed(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> > >     /* Last write is outside, after this conditional */
> > > } else if (duty_cycles ...
>
> Can you have a look at this part ?

I can put here and it should be no harmful.

Frank

>
> > > > +	if (duty_cycles < imx->duty_cycle && val < MX3_PWMSR_FIFOAV_2WORDS) {
> > > > +		val = readl_relaxed(imx->mmio_base + MX3_PWMCNR);
> > > > +		/*
> > > > +		 * If counter is close to period, controller may roll over when
> > > > +		 * next IO write.
> > > > +		 */
> > >
> > > c is only used in this if (duty_cycles ...) { } conditional, the do_div()
> > > above can be moved here:
> >
> > It is in local_irq_save(flags) scope, it'd better as less as possible. So
> > I prefer do_div() is outside local_irq_save()
> Good point, either way is fine by me.
diff mbox series

Patch

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index 9e2bbf5b4a8ce..bac09d40767b6 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -26,6 +26,7 @@ 
 #define MX3_PWMSR			0x04    /* PWM Status Register */
 #define MX3_PWMSAR			0x0C    /* PWM Sample Register */
 #define MX3_PWMPR			0x10    /* PWM Period Register */
+#define MX3_PWMCNR			0x14    /* PWM Counter Register */
 
 #define MX3_PWMCR_FWM			GENMASK(27, 26)
 #define MX3_PWMCR_STOPEN		BIT(25)
@@ -223,6 +224,8 @@  static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
 	unsigned long long c;
 	unsigned long long clkrate;
+	unsigned long flags;
+	int val;
 	int ret;
 	u32 cr;
 
@@ -263,7 +266,69 @@  static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		pwm_imx27_sw_reset(chip);
 	}
 
-	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
+	/*
+	 * This is a limited workaround. When the SAR FIFO is empty, the new
+	 * write value will be directly applied to SAR even the current period
+	 * is not over.
+	 *
+	 *           ─────────────────────┐
+	 * PWM OUTPUT                     │
+	 *                                └─────────────────────────
+	 *
+	 *           ┌──────────────────────────────────────────────┐
+	 * Counter   │       XXXXXXXXXXXXXX                         │
+	 *           └──────────────────────────────────────────────┘
+	 *                   ▲            ▲
+	 *                   │            │
+	 *                 New SAR      Old SAR
+	 *
+	 *           XXXX  Errata happen window
+	 *
+	 * If the new SAR value is less than the old one, and the counter is
+	 * greater than the new SAR value (see above diagram XXXX), the current
+	 * period will not flip the level. This will result in a pulse with a
+	 * duty cycle of 100%.
+	 *
+	 * Check new SAR less than old SAR and current counter is in errata
+	 * windows, write extra old SAR into FIFO and new SAR will effect at
+	 * next period.
+	 *
+	 * Sometime period is quite long, such as over 1 second. If add old SAR
+	 * into FIFO unconditional, new SAR have to wait for next period. It
+	 * may be too long.
+	 *
+	 * Turn off the interrupt to ensure that not IRQ and schedule happen
+	 * during above operations. If any irq and schedule happen, counter
+	 * in PWM will be out of data and take wrong action.
+	 *
+	 * Add a safety margin 1.5us because it needs some time to complete
+	 * IO write.
+	 *
+	 * Use __raw_writel() to minimize the interval between two writes to
+	 * the SAR register to increase the fastest PWM frequency supported.
+	 *
+	 * When the PWM period is longer than 2us(or <500kHz), this workaround
+	 * can solve this problem. No software workaround is available if PWM
+	 * period is shorter than IO write.
+	 */
+	c = clkrate * 1500;
+	do_div(c, NSEC_PER_SEC);
+
+	local_irq_save(flags);
+	val = FIELD_GET(MX3_PWMSR_FIFOAV, readl_relaxed(imx->mmio_base + MX3_PWMSR));
+	if (duty_cycles < imx->duty_cycle && val < MX3_PWMSR_FIFOAV_2WORDS) {
+		val = readl_relaxed(imx->mmio_base + MX3_PWMCNR);
+		/*
+		 * If counter is close to period, controller may roll over when
+		 * next IO write.
+		 */
+		if ((val + c >= duty_cycles && val < imx->duty_cycle) ||
+		    val + c >= period_cycles)
+			writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR);
+	}
+	writel_relaxed(duty_cycles, imx->mmio_base + MX3_PWMSAR);
+	local_irq_restore(flags);
+
 	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
 
 	/*