Message-ID: <gnlm4u7dc2aktycxzn4nqw3anzeit6tbtgcq7kv3pzbrorwg6o@h35yxit5y2a3>
Date: Sat, 5 Oct 2024 17:57:18 +0200
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
To: Marek Vasut <marex@...x.de>
Cc: Frank Li <Frank.Li@....com>, 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 Sat, Oct 05, 2024 at 02:41:29AM +0200, Marek Vasut wrote:
> On 10/4/24 10:58 PM, Uwe Kleine-König wrote:
> 
> [...]
> 
> Why not simply duplicate the ERRATA description for iMX8M Nano MX8MN_0N14Y
> errata sheet ?
> 
> "
> [...]
> "
> 
> That is very clear to me.

Fine for me. Frank, do you want to try creating the right mix of the NXP
text, your and my description?

> > 	/*
> > 	 * 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.
> 
> I would skip this ^ part unless you can surely say the IP works exactly that
> way because you checked the RTL.

That it works that way is clear from the errata text IMHO.

> > 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.
> 
> The ascii/infographics is nice and would be good to keep, but regarding the
> description, frankly, the NXP errata description says the same thing in
> fewer words :)
> 
> > 	 */
> > 
> > > +	 *
> > > +	 * 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?
> 
> No, because you would overflow the FIFO, see:
> 
> 137fd45ffec1 ("pwm: imx: Avoid sample FIFO overflow for i.MX PWM version2")

val holds the number of uses FIFO entries, so writing (3 - val) new
items should be fine?!

> > Or busy loop until
> > MX3_PWMSR_FIFOAV becomes 0?
> 
> Do we really want a busy wait here if we can avoid it ?

udelay(6) is a busy loop, so we're already there.

> We can do udelay(3 * state->period / 1000); so faster PWMs would wait
> shorter.

state->period is the new value (and you want the old, right?), but
otherwise I agree

> The delay is here to basically wait until the FIFO is surely empty and has
> space for 3 consecutive writes (see the commit above wrt. FIFO overflow).

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists