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

[Marek Vasut <marex@denx.de>]

On 9/9/24 9:41 PM, Frank Li wrote:

Hi,

[...]

>>> I will add it at next version. But I found a problem of your method,
>>> especially when period is quite long, for example 2s. Assume  FIFO is empty.
>>> [old, old, new] will be written to FIFO, new value will takes 2s-6s to make
>>> new value effect.
>>
>> You're right, for long PWM periods, the application of changes will take
>> longer.
>>
>> As far as I can tell, the method implemented in this patch may still suffer
>> from the problem in case of short PWM periods, is that correct ? I think the
>> writesl() might help cover some of that.
> 
> No way can fix very short periods problem because period was shorter then
> register write speed.

What kind of period are we talking about here ? Since the FIFO has 4 
slots, I would expect way 4-8 MHz to be fixable still ?

> The register write go through ips bus, which is
> quit slow. writesl is equal to writel_relaxed and avoid a dmb(). This will
> be over1M hz and user generally use pwm around hz to khz.

I just had a look at the implementation of writel_relaxed() and yes, 
this is basically a 'str', good point.

>> Maybe for longer PWM periods (like 500ms and longer?) where we can be sure
>> the FIFO won't quickly consume the written data and underflow, we can do
>> writesl() with only two longwords {old_sar, new_sar}, first longword to make
>> sure the FIFO is not empty, second word with the new PWMSAR value ? That
>> could speed the update up ?
> 
> We should use this patch's method to read MX3_PWMCNR, if close enough,
> write two data into fifo instead of wait for new peroid start.

Can we guarantee that there would never be any scheduling or other delay 
between the readout of PWMCNR and write of the FIFO, one which would 
trigger an underflow ?

>>> The currect method, most time, the new value will effect at next period.
>> Yes, right, I think we may have to handle the longer PWM periods somehow
>> differently.
>>
>> I would like to avoid local_irq_save()/readl_poll_timeout_atomic() if
>> possible and let the hardware help avoid this, which I think is possible
>> with creative loading of the FIFO with data, hence the writesl() idea.
> 
> I think it hard to avoid local_irq_save() even use writesl(), writesl is
> not atomic,  if irq happen after first raw_write,  FIFO may be empty at
> next write.
> 
> actually, here have problem
> 
> 	val = FIELD_GET(MX3_PWMSR_FIFOAV, readl_relaxed(imx->mmio_base + MX3_PWMSR));
> 	^^^ if irq happen here, schedule happen, when schedule back,
> 
> 	FIFOAV value in hardware may less than MX3_PWMSR_FIFOAV_2WORDS, but
> 	previous read it bigger than MX3_PWMSR_FIFOAV_2WORDS, the below check
> 	will be false and skip workaround.
> 
> 	if (duty_cycles < imx->duty_cycle && val < MX3_PWMSR_FIFOAV_2WORDS) {
> 
> 
> See patch v4
> https://lore.kernel.org/imx/20240909193855.2394830-1-Frank.Li@nxp.com/T/#u
> It should be little bit better.

I think the v4 is indeed better, thanks.