On Sun, Sep 22, 2024 at 10:28:02PM +0200, Marek Vasut wrote:
> Hi,
>
> On 9/17/24 9:25 PM, Frank Li wrote:
>
> [...]
>
> > @@ -223,6 +224,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;
> > @@ -263,7 +266,69 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>
> [...]
>
> > + 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));
>
> I think the multi-write I mentioned in v5 for > 500 kHz case could further
> improve the patch, let's see what others think:
>
> 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 */
> writel_relaxed(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> writel_relaxed(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> /* Last write is outside, after this conditional */
> } else if (duty_cycles ...
>
> > + if (duty_cycles < imx->duty_cycle && val < MX3_PWMSR_FIFOAV_2WORDS) {
> > + val = readl_relaxed(imx->mmio_base + MX3_PWMCNR);
> > + /*
> > + * If counter is close to period, controller may roll over when
> > + * next IO write.
> > + */
>
> c is only used in this if (duty_cycles ...) { } conditional, the do_div()
> above can be moved here:
It is in local_irq_save(flags) scope, it'd better as less as possible. So
I prefer do_div() is outside local_irq_save()
Frank
>
> c = clkrate * 1500;
> do_div(c, NSEC_PER_SEC);
>
> > + if ((val + c >= duty_cycles && val < imx->duty_cycle) ||
> > + 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);
