On Wed, Sep 11, 2024 at 10:26:38PM +0200, Marek Vasut wrote:
> On 9/10/24 5:24 PM, Frank Li wrote:
>
> Hi,
>
> purely nitpicking further below, one quick question right below.
>
> > This only resolves the issue when the PWM period is longer than 2us
> > (or <500KHz) because write register is not quick enough when PWM period is
> > very short.
>
> You did mention the IPS bus is slow. Do I understand it correctly that the
> IPS bus write takes about 1us ? Because of the PWM consumes a sample every
> 2us and we need to write 2 samples to avoid FIFO underrun, then to safely
> write those 2 samples, we need to be able to write 1 sample per 1 us into
> the FIFO ?
The above time is just estimated, which variance at difference platform and
impact by other IPs. If there are pending write/read from GPIO, PWM write
have to wait for GPIO's write finish. It actually depend on IPS bus's
loading.
<500Khz is very less possiblity that write slower than PWM's consumes.
>
> Also, would writing more samples help with such "fast" use cases ?
> Something like this:
>
> if (clkrate > 500000) {
> // This usleep() could use some further improvement, e.g. calculate
> // precise delay for the FIFO to get empty based on PWM clkrate
> usleep(2 * 5); // wait 2us for each of the 4 samples in FIFO and a bit
> // Now the FIFO is surely empty, write all four FIFO slots
> writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR);
> writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR);
> writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR);
> writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR);
It can help at some possiblity, but still have problem if > 1Mhz, write
will always less than consume.
If errata happen, only 1 cycle is full high. I think it is quite less
impact at such high frequency.
We found this problem by observe a screen backlight flick when change
ducty_cycle. I think we try fix it after a real user visible impact happen.
Put code here can reduce some possiblity at certain freq range, but may
miss-leading user the problem fixed when > 500k.
Frank
> } else { // clock rate less than 500 kHz
> // Do the workaround below
> if (duty_cycles < imx->duty_cycle && val < MX3_PWMSR_FIFOAV_2WORDS
> ...
> }
>
> > Reproduce steps:
> > cd /sys/class/pwm/pwmchip1/pwm0
> > echo 2000000000 > period # It is easy to observe by using long period
> > echo 1000000000 > duty_cycle
> > echo 1 > enable
> > echo 800000000 > duty_cycle # One full high plus will be seen by scope
> >
> > Fixes: 166091b1894d ("[ARM] MXC: add pwm driver for i.MX SoCs")
> > Reviewed-by: Jun Li <jun.li@xxxxxxx>
> > Signed-off-by: Clark Wang <xiaoning.wang@xxxxxxx>
> > Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> > ---
> > Change from v4 to v5
> > - fix typo PMW & If
> > - using imx->mmio_base + MX3_PWMSAR
> >
> > Change from v3 to v4
> > - none, wrong bump version number
> > Change from v2 to v3
> > - simple workaround implement.
> > - add reproduce steps.
> >
> > Change from v1 to v2
> > - address comments in https://lore.kernel.org/linux-pwm/20211221095053.uz4qbnhdqziftymw@xxxxxxxxxxxxxx/
> > About disable/enable pwm instead of disable/enable irq:
> > Some pmw periphal may sensitive to period. Disable/enable pwm will
> > increase period, althouhg it is okay for most case, such as LED backlight
> > or FAN speed. But some device such servo may require strict period.
> >
> > - address comments in https://lore.kernel.org/linux-pwm/d72d1ae5-0378-4bac-8b77-0bb69f55accd@xxxxxxx/
> > Using official errata number
> > fix typo 'filp'
> > add {} for else
> >
> > I supposed fixed all previous issues, let me know if I missed one.
> > ---
> > drivers/pwm/pwm-imx27.c | 67 ++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 66 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> > index 253afe94c4776..713d368f03931 100644
> > --- a/drivers/pwm/pwm-imx27.c
> > +++ b/drivers/pwm/pwm-imx27.c
> > @@ -27,6 +27,7 @@
> > #define MX3_PWMSR 0x04 /* PWM Status Register */
> > #define MX3_PWMSAR 0x0C /* PWM Sample Register */
> > #define MX3_PWMPR 0x10 /* PWM Period Register */
> > +#define MX3_PWMCNR 0x14 /* PWM Counter Register */
> > #define MX3_PWMCR_FWM GENMASK(27, 26)
> > #define MX3_PWMCR_STOPEN BIT(25)
> > @@ -234,6 +235,8 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
> > unsigned long long c;
> > unsigned long long clkrate;
> > + unsigned long flags;
> > + int val;
> > int ret;
> > u32 cr;
> > @@ -274,7 +277,69 @@ 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
> > + *
> > + * 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
>
> Better do 's@\<sar\>@SAR@g' so all the instances of 'sar' in the text are in
> uppercase. Currently, some are in lowercase and some are in uppercase.
>
> > + * 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
>
> IRQ, in uppercase.
>
> > + * 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.
>
> PWM, in uppercase.
>
> > + * When the PWM period is longer than 2us(or <500KHz), this workaround
>
> kHz, kilo lowercase, Hz Hertz uppercase H lowercase z .
>
> Also fix in the commit message.
>
> > + * 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 && val < MX3_PWMSR_FIFOAV_2WORDS) {
> > + val = readl_relaxed(imx->mmio_base + MX3_PWMCNR);
>
> I would put the comment below above this conditional statement.
>
> > + if ((val + c >= duty_cycles && val < imx->duty_cycle) ||
> > + /*
> > + * If counter is close to period, controller may roll over
> > + * when next IO write.
> > + */
> > + val + c >= period_cycles)
> > + writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR);
> > + }
> > + writel_relaxed(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> > + local_irq_restore(flags);
> > +
> > writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> The patch looks good, the above are just trivial nitpicks.
>
> Thanks !
