Re: [PATCH v7 1/1] pwm: imx27: workaround of the pwm output bug when decrease the duty cycle

[Marek Vasut <marex@denx.de>]

On 10/4/24 10:58 PM, Uwe Kleine-König wrote:

[...]

>> @@ -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:

Why not simply duplicate the ERRATA description for iMX8M Nano 
MX8MN_0N14Y errata sheet ?

"
ERR051198:
PWM: 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%.
"

That is very clear to me.

> 	/*
> 	 * 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.

> 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")

> Or busy loop until
> MX3_PWMSR_FIFOAV becomes 0?

Do we really want a busy wait here if we can avoid it ?

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

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).

[...]