Message-ID: <ZwBdOSobshR6mOi3@lizhi-Precision-Tower-5810>
Date: Fri, 4 Oct 2024 17:25:13 -0400
From: Frank Li <Frank.li@....com>
To: Uwe Kleine-König <u.kleine-koenig@...libre.com>
Cc: marex@...x.de, conor+dt@...nel.org, devicetree@...r.kernel.org,
	festevam@...il.com, francesco@...cini.it, imx@...ts.linux.dev,
	jun.li@....com, kernel@...gutronix.de, krzk+dt@...nel.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linux-pwm@...r.kernel.org, p.zabel@...gutronix.de,
	pratikmanvar09@...il.com, robh@...nel.org, s.hauer@...gutronix.de,
	shawnguo@...nel.org, xiaoning.wang@....com
Subject: Re: [PATCH v7 1/1] pwm: imx27: workaround of the pwm output bug when
 decrease the duty cycle

On Fri, Oct 04, 2024 at 10:58:49PM +0200, Uwe Kleine-König wrote:
> On Fri, Oct 04, 2024 at 03:35:31PM -0400, Frank Li wrote:
> > From: Clark Wang <xiaoning.wang@....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
>
> That should be "pulse" I guess ------------------^^^^

Yes,

>
> I would have expected a much lower value for the second write to
> duty_cycle. I guess it depends on the machine you run this on if this
> hits the race window.

Yes, lower value can increase reproduce rate. I can change to 8000 at
next version.

>
> > Fixes: 166091b1894d ("[ARM] MXC: add pwm driver for i.MX SoCs")
> > Reviewed-by: Jun Li <jun.li@....com>
> > Signed-off-by: Clark Wang <xiaoning.wang@....com>
> > Signed-off-by: Frank Li <Frank.Li@....com>
> > ---
> > Change from v6 to v7
> > - Add continue write for < 500hz case to try best to workaround this
> > problem.
> >
> > 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 | 75 ++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 74 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> > index 9e2bbf5b4a8ce..00a7189ba46ca 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,77 @@ 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
>
> Hmm, ok, so SAR is the register value that implements the duty cycle
> setting. And if a new SAR is written, it is directly applied to the
> hardware and this way it can happen (if SAR_new < counter < SAR_old)
> that no falling edge happens in the current period. Right?

Yes

>
> If so, I think the depicted PWM output is misleading. I'd describe and
> picture it as follows:
>
> 	/*
> 	 * At each clock tick the hardware compares the SAR value with
> 	 * the current counter. If they are equal the output is changed
> 	 * to the inactive level. As a new SAR value is applied
> 	 * immediately to the currently running period, it can happen
> 	 * that no falling edge happens in a period and so the output is
> 	 * active for a whole period. Consider a change from
>        *     ________
> 	 *    /        \______/
>        *    ^      *        ^
> 	 * to
>        *     ____
> 	 *    /    \__________/
>        *    ^               ^
> 	 *
> 	 * where SAR is written at the time marked by *. The counter
> 	 * didn't reach the old (bigger) value because it was changed
> 	 * before the counter reached that value and when the new value
> 	 * becomes active it is already lower than the current counter
> 	 * and so doesn't trigger either while the counter continues to
> 	 * grow. So the resulting waveform looks as follows:
> 	 *
>        *     ________        ____________________
> 	 *    /        \______/                    \__________/
>        *    ^               ^      *        ^               ^
> 	 *    |<-- old SAR -->|               |<-- new SAR -->|
> 	 *
> 	 * that is the output is active for a whole period.
> 	 */

Good.

>
> > +	 *
> > +	 * 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) {
> > +		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 */
>
> I don't understand the motivation to wait here. Wouldn't it be better to
> write the old value 3 - val times and not sleep? Or busy loop until
> MX3_PWMSR_FIFOAV becomes 0?

It is required by Marek Vasut. Read register is also quite slow. It is
hard to hit this branch and can not 100% workaround this problem when
period is short. Just choose simplest mathod here.

>
> > +			writel_relaxed(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> > +			writel_relaxed(duty_cycles, imx->mmio_base + MX3_PWMSAR);
>
> With the comment above I would have expected __raw_writel here?!

I forget update comment. writel_relaxed() is wrap of __raw_writel().

>
> > +		} else if (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);
>
> I didn't find the time yet to look into your other pwm-imx27 series.
> Does it conflict with this patch? Which should be applied first?

No conflict, but let's work out this patch first. I think 32k patch may not
necessary because driver have not use 32k clock source. It should work
without 32k clk.

Frank

>
> Best regards
> Uwe

Powered by blists - more mailing lists