Messages in this thread | | | Subject | Re: [PATCH v3 1/4] pwm: imx27: fix race condition .apply,.get_state | From | Leif Middelschulte <> | Date | Sat, 24 Feb 2024 12:00:02 +0100 |
| |
> Am 06.09.2023 um 17:42 schrieb Uwe Kleine-König <u.kleine-koenig@pengutronix.de>: > > On Tue, Aug 15, 2023 at 12:43:29PM +0200, Leif Middelschulte wrote: >> From: Leif Middelschulte <Leif.Middelschulte@klsmartin.com> >> >> 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@klsmartin.com> >> --- >> drivers/pwm/pwm-imx27.c | 50 ++++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 47 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c >> index 29a3089c534c..32389ca2da3e 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 >> >> /* PWMPR register value of 0xffff has the same effect as 0xfffe */ >> #define MX3_PWMPR_MAX 0xfffe >> @@ -118,8 +119,28 @@ 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_get_period(pwm), NSEC_PER_MSEC); >> + int tries = MX3_PWM_FIFOAV_EMPTY_LOOP; >> + int fifoav; >> + u32 sr; >> + >> + while (tries--) { >> + sr = readl(imx->mmio_base + MX3_PWMSR); >> + fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr); >> + if (fifoav == MX3_PWMSR_FIFOAV_EMPTY) >> + return; >> + msleep(period_ms); >> + } >> + dev_warn(dev, "FIFO has been refilled concurrently\n"); >> +} >> + >> static int pwm_imx27_get_state(struct pwm_chip *chip, >> - struct pwm_device *pwm, struct pwm_state *state) >> + struct pwm_device *pwm, struct pwm_state *state) > > This looks wrong. Aligning at the opening ( was right.
Good catch. That change as unintentional.
> >> { >> struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip); >> u32 period, prescaler, pwm_clk, val; >> @@ -161,10 +182,33 @@ static int pwm_imx27_get_state(struct pwm_chip *chip, >> * PWMSAR can be read only if PWM is enabled. If the PWM is disabled, >> * use the cached value. >> */ >> - if (state->enabled) >> + if (state->enabled) { >> + /* >> + * From the i.MX PWM reference manual: >> + * "A read on the sample register yields the current FIFO value that >> + * is being used, or will be used, by the PWM for generation on the >> + * output signal. Therefore, a write and a subsequent read on the >> + * sample register may result in different values being obtained." >> + * Furthermore: >> + * "When a new value is written, the duty cycle changes after the >> + * current period is over." >> + * Note "changes" vs. "changes to the given value"! >> + * Finally: >> + * "The PWM will run at the last set duty-cycle setting if all the >> + * values of the FIFO has been utilized, until the FIFO is reloaded >> + * or the PWM is disabled." >> + * Try to be at least a bit more deterministic about which value is >> + * read by waiting until the FIFO is empty. In this state the last/most >> + * recently pushed sample (duty cycle) value is continuously applied. >> + * Beware that this approach is still racy, as a new value could have >> + * been supplied and a period expired between the call of the wait >> + * function and the subsequent readl. > > this would only happen if there are concurrent calls into the driver, > wouldn't it? I think it's safe to assume this doesn't happen.
This assessment seems correct at them moment. This comment is supposed to be a source of explanation to future developers if - for some reason - the driver/subsystem is changed and they trip over this behaviour (again), just as I did.
> > Patch 3 of this series improves the function that is only introduced > here. I suggest to squash these together.
I will provide v4 with the changes squashed into the initial commit, that introduced the function.
> > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |
Thank you,
Leif
| |