| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4166d68c-5eca-406a-936f-412dd2ae72fc@denx.de>
Date: Sat, 5 Oct 2024 02:41:29 +0200
From: Marek Vasut <marex@...x.de>
To: Uwe Kleine-König <u.kleine-koenig@...libre.com>,
Frank Li <Frank.Li@....com>
Cc: 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 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).
[...]
Powered by blists - more mailing lists