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

[Marek Vasut <marex@denx.de>]

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:

	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);