Re: [PATCH v4 1/2] pwm: imx27: fix race condition .apply,.get_state

[Uwe Kleine-König]

Hello Leif,

thanks for this new round addressing the identified issues.

On Sat, Feb 24, 2024 at 12:29:00PM +0100, Leif Middelschulte wrote:
> From: Leif Middelschulte <Leif.Middelschulte@xxxxxxxxxxxxx>
> 
> With CONFIG_PWM_DEBUG=y after writing a value to the PWMSAR
> register in .apply(), the register is read in .get_state().
> Unless a period completed in the meantime, this read yields the
> previously used duty cycle configuration. As the PWM_DEBUG code
> applies the read out configuration for testing purposes this
> effectively undoes the intended effect by rewriting the previous
> hardware state.
> 
> Note that this change merely implements a sensible heuristic.
> The i.MX has a 4 slot FIFO to configure the duty cycle. This FIFO
> cannot be read back in its entirety. The "write x then read back
> x from hw" semantics are therefore not easily applicable.
> With this change, the .get_state() function tries to wait for some
> stabilization in the FIFO (empty state). In this state it keeps
> applying the last value written to the sample register.
> 
> Signed-off-by: Leif Middelschulte <Leif.Middelschulte@xxxxxxxxxxxxx>
> ---
>  drivers/pwm/pwm-imx27.c | 55 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index 7d9bc43f12b0..cb564460b79c 100644
> --- a/drivers/pwm/pwm-imx27.c
> +++ b/drivers/pwm/pwm-imx27.c
> @@ -75,6 +75,7 @@
>  						   (x)) + 1)
>  
>  #define MX3_PWM_SWR_LOOP		5
> +#define MX3_PWM_FIFOAV_EMPTY_LOOP	4

This looks like a register definition, but it's only a number that
defines the iterations waiting for the FIFO to empty. (The same critic
applies to MX3_PWM_SWR_LOOP, though.)

>  /* PWMPR register value of 0xffff has the same effect as 0xfffe */
>  #define MX3_PWMPR_MAX			0xfffe
> @@ -118,6 +119,31 @@ static void pwm_imx27_clk_disable_unprepare(struct pwm_imx27_chip *imx)
>  	clk_disable_unprepare(imx->clk_ipg);
>  }
>  
> +static int pwm_imx27_wait_fifo_empty(struct pwm_chip *chip,
> +				     struct pwm_device *pwm)
> +{
> +	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
> +	struct device *dev = chip->dev;
> +	unsigned int period_ms = DIV_ROUND_UP_ULL(pwm->state.period, NSEC_PER_MSEC);

Given that waiting here is unfortunate it would be nice so reduce the
waiting as much as possible. So it might make sense to read the actual
period from the hardware and use that as it might be smaller that
pwm->state.period.

> +	int tries = MX3_PWM_FIFOAV_EMPTY_LOOP;
> +	int fifoav, previous_fifoav = INT_MAX;
> +	u32 sr;

Most variables can go into the while loop.

> +	while (tries--) {
> +		sr = readl(imx->mmio_base + MX3_PWMSR);
> +		fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr);
> +		if (fifoav == MX3_PWMSR_FIFOAV_EMPTY)
> +			return 0;
> +		/* if the FIFO value does not decrease, there is another problem */
> +		if (previous_fifoav == fifoav)
> +			break;
> +		previous_fifoav = fifoav;
> +		msleep(period_ms);
> +	}

I wonder if a loop is necessary at all. Why not use
msleep(FIELD_GET(MX3_PWMSR_FIFOAV, sr) * period_ms)?

Maybe take PWMCNR into account to shorten the sleep a bit.

Best regards
Uwe

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

Attachment: signature.asc
Description: PGP signature