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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1e6600a8-0b1a-472b-9f84-9b3c646a931a@denx.de>
Date: Sun, 6 Oct 2024 21:12:25 +0200
From: Marek Vasut <marex@...x.de>
To: Uwe Kleine-König <u.kleine-koenig@...libre.com>
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 10/5/24 5:57 PM, Uwe Kleine-König wrote:
> 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.

The errata description does not say anything about comparing SAR value 
on each clock tick. Better stick to exactly what the errata does say.

[...]

>>>> +	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?!

Not necessarily, consider the case where:
- The PWM is very fast
- There are currently 3 entries in the FIFO according to driver state
- The driver determines 3-val is 1 and performs 1 single write to FIFO
=> If the PWM consumed the FIFO (FIFO is empty) before the 1 single
    write arrives, then the aforementioned errata still occurs

I believe the better option is to wait until the FIFO is surely depleted 
and then write three entries in short sequence -- OLD-OLD-NEW -- this 
way the FIFO would get updated with old value first and then switched to 
new value, hopefully mitigating the issue as best as possible even for 
fast PWM settings.

btw. the two writes here should be writing the old value twice, now 
there are three new value writes in this patch version.

>>> 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
Right