Message-ID: <20211220105555.zwq22vip7onafrck@pengutronix.de>
Date:   Mon, 20 Dec 2021 11:55:55 +0100
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Clark Wang <xiaoning.wang@....com>
Cc:     thierry.reding@...il.com, lee.jones@...aro.org,
        linux-pwm@...r.kernel.org, shawnguo@...nel.org,
        s.hauer@...gutronix.de, linux-kernel@...r.kernel.org,
        linux-imx@....com, kernel@...gutronix.de, festevam@...il.com,
        linux-arm-kernel@...ts.infradead.org, Jun Li <jun.li@....com>
Subject: Re: [PATCH V2] pwm: imx27: workaround of the pwm output bug when
 decrease the duty cycle

Hello,

[added Jun Li who reviewed the patch to Cc:]

On Mon, Dec 20, 2021 at 03:31:30PM +0800, Clark Wang wrote:
> This is a limited workaround for the PWM IP issue.
> 
> Root cause:
> When the SAR FIFO is empty, the new write value will be directly applied
> to SAR even the current period is not over.


> If the new SAR value is less than the old one, and the counter is
> greater than the new SAR value, the current period will not filp the
> level. This will result in a pulse with a duty cycle of 100%.

I read the i.MX25 PWM chapter a few times now, and I played around a
bit. I could confirm the issue.

> Workaround:
> Add an old value SAR write before updating the new duty cycle to SAR.
> This will keep the new value is always in a not empty fifo, and can be wait
> to update after a period finished.
> 
> Limitation:
> This workaround can only solve this issue when the PWM period is longer than
> 2us(or <500KHz).
> 
> Reviewed-by: Jun Li <jun.li@....com>
> Signed-off-by: Clark Wang <xiaoning.wang@....com>
> ---
> V2:
>  Fix the warnings when built in riscv-gcc, which is reported by kernel test robot <lkp@...el.com>
> ---
>  drivers/pwm/pwm-imx27.c | 67 ++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 62 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index ea91a2f81a9f..3d9ca60e2baa 100644
> --- a/drivers/pwm/pwm-imx27.c
> +++ b/drivers/pwm/pwm-imx27.c
> @@ -21,11 +21,13 @@
>  #include <linux/platform_device.h>
>  #include <linux/pwm.h>
>  #include <linux/slab.h>
> +#include <linux/spinlock.h>
>  
>  #define MX3_PWMCR			0x00    /* PWM Control Register */
>  #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)
> @@ -91,6 +93,7 @@ struct pwm_imx27_chip {
>  	 * value to return in that case.
>  	 */
>  	unsigned int duty_cycle;
> +	spinlock_t lock;
>  };
>  
>  #define to_pwm_imx27_chip(chip)	container_of(chip, struct pwm_imx27_chip, chip)
> @@ -201,10 +204,10 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip,
>  
>  	sr = readl(imx->mmio_base + MX3_PWMSR);
>  	fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr);
> -	if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {
> +	if (fifoav >= MX3_PWMSR_FIFOAV_3WORDS) {
>  		period_ms = DIV_ROUND_UP_ULL(pwm_get_period(pwm),
>  					 NSEC_PER_MSEC);
> -		msleep(period_ms);
> +		msleep(period_ms * (fifoav - 2));

This changes semantic, from "wait for at least one free FIFO slot" to
"wait for at least two FIFO slots". Maybe a comment would be good? At
least the comment above the caller of pwm_imx27_wait_fifo_slot() needs
adaption.

Also I wonder: If there is only a single free slot, there is no problem,
is there?

>  		sr = readl(imx->mmio_base + MX3_PWMSR);
>  		if (fifoav == FIELD_GET(MX3_PWMSR_FIFOAV, sr))

The error test is wrong then. If fifoav was initially 4 and waiting only
reduced it to 3, you don't trigger a warning.

On a side note: pwm_get_period() might return a value that is too big.
This could be improved using readl_poll_timeout.

> @@ -215,13 +218,15 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip,
>  static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  			   const struct pwm_state *state)
>  {
> -	unsigned long period_cycles, duty_cycles, prescale;
> +	unsigned long period_cycles, duty_cycles, prescale, counter_check, flags;
>  	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
> +	void __iomem *reg_sar = imx->mmio_base + MX3_PWMSAR;
> +	__force u32 sar_last, sar_current;
>  	struct pwm_state cstate;
>  	unsigned long long c;
>  	unsigned long long clkrate;
>  	int ret;
> -	u32 cr;
> +	u32 cr, timeout = 1000;
>  
>  	pwm_get_state(pwm, &cstate);
>  
> @@ -262,7 +267,57 @@ 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.

	When the SAR FIFO is empty, a new SAR value will be applied directly
	instead of waiting for the current period to complete.

is grammatically better (at least with my German sense for English
grammar) and might be better to understand.

> +	 * If the new SAR value is less than the old one, and the counter is
> +	 * greater than the new SAR value, the current period will not filp

s/filp/flip/

> +	 * the level. This will result in a pulse with a duty cycle of 100%.
> +	 * So, writing the current value of the SAR to SAR here before updating
> +	 * the new SAR value can avoid this issue.

This can be expressed a bit easier (and more correct) I think: 

	If the SAR value is decreased over the counter value, there is
	no compare event in the current period resulting in a 100%
	period.

(This is more correct because decreasing the SAR from 200 to 100 if the
counter is already at 300 doesn't result in the problem.)

> +	 * Add a spin lock and turn off the interrupt to ensure that the

The usual term is "turn off interrupts" because not only the PWM irq is
disabled, but all are. I wonder if just disabling irqs serves the same
purpose.

> +	 * real-time performance can be guaranteed as much as possible when

Disabling interrupts is never good for real-time performance.

Having said that I think I'd go for a solution without irq disabling.

> +	 * operating the following operations.
> +	 *
> +	 * 1. Add a threshold of 1.5us. If the time T between the read current
> +	 * count value CNR and the end of the cycle is less than 1.5us, wait
> +	 * for T to be longer than 1.5us before updating the SAR register.
> +	 * This is to avoid the situation that when the first SAR is written,
> +	 * the current cycle just ends and the SAR FIFO that just be written
> +	 * is emptied again.
> +	 *
> +	 * 2. Use __raw_writel() to minimize the interval between two writes to
> +	 * the SAR register to increase the fastest pwm frequency supported.
> +	 *
> +	 * When the PWM period is longer than 2us(or <500KHz), this workaround
> +	 * can solve this problem.
> +	 */
> +	if (duty_cycles < imx->duty_cycle) {
> +		c = clkrate * 1500;
> +		do_div(c, NSEC_PER_SEC);
> +		counter_check = c;

This needs to account for PWMCR.PRESCALER.

> +		sar_last = (__force u32) cpu_to_le32(imx->duty_cycle);
> +		sar_current = (__force u32) cpu_to_le32(duty_cycles);

If endianess conversion is necessary, please apply it to the complete
driver in a separate patch. Doing this only in one place and skipping
several others is just irritating.

> +		spin_lock_irqsave(&imx->lock, flags);
> +		if (state->period >= 2000) {

This checks the new period value that doesn't have to do anything with
the hardware state yet. So this check isn't sensible, is it?

> +			while ((period_cycles -
> +				readl_relaxed(imx->mmio_base + MX3_PWMCNR))
> +				< counter_check) {

period_cycles is the new overflow value.  If the current overflow value
is lower, the loop might terminate immediately. If however the current
overflow value is higher than period_cycles, the subtraction might
overflow resulting in the check to be true. In both cases there isn't
any relation to the next overflow event.

> +				if (!--timeout)
> +					break;

On a fast machine with a long period this might end in a timeout before
the rollover.

> +			};
> +		}
> +		if (!(MX3_PWMSR_FIFOAV &
> +		      readl_relaxed(imx->mmio_base + MX3_PWMSR)))
> +			__raw_writel(sar_last, reg_sar);

Why not use FIELD_GET for consistency here?

> +		__raw_writel(sar_current, reg_sar);
> +		spin_unlock_irqrestore(&imx->lock, flags);
> +	} else
> +		writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);

This violates the coding style. If one branch of an if needs braces, all
branches should use them.

> +
>  	writel(period_cycles, imx->mmio_base + MX3_PWMPR);

Changing the period value resets the counter. So to minimize the effect
of the mitigation (i.e. a busy loop for up to 1.5µs with irqs off) I
suggest to do the wait and write SAR twice only if period doesn't change
and SAR is actually decreased over the current counter value.
 
>  	/*
> @@ -323,6 +378,8 @@ static int pwm_imx27_probe(struct platform_device *pdev)
>  		return dev_err_probe(&pdev->dev, PTR_ERR(imx->clk_per),
>  				     "failed to get peripheral clock\n");
>  
> +	spin_lock_init(&imx->lock);
> +	imx->duty_cycle = 0;

duty_cycle is already 0 here, so there is no need to initialize it.

>  	imx->chip.ops = &pwm_imx27_ops;
>  	imx->chip.dev = &pdev->dev;
>  	imx->chip.npwm = 1;

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists