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@xxxxxxx/T/#u
It should be little bit better.
I think the v4 is indeed better, thanks.
