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
Attachment:
signature.asc
Description: PGP signature
