Message ID | 20230126150313.764284-1-Leif.Middelschulte@gmail.com |
---|---|
State | Changes Requested |
Headers | show |
Series | [v2] pwm: imx27: fix race condition .apply,.get_state | expand |
On Thu, Jan 26, 2023 at 04:03:13PM +0100, 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); Please don't use pwm_get_period. This one is intended for PWM consumers only. Directly using pwm->state.period is fine however. > + 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); This is a rather long sleep. Maybe check in each iteration that fifoav decreases as expected? > + } > + 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) > { > 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. > + */ > + pwm_imx27_wait_fifo_empty(chip, pwm); Instead of issuing a warning, I'd return an error code if wait_fifo_empty fails. > val = readl(imx->mmio_base + MX3_PWMSAR); > - else > + } else { > val = imx->duty_cycle; > + } > > tmp = NSEC_PER_SEC * (u64)(val) * prescaler; > state->duty_cycle = DIV_ROUND_UP_ULL(tmp, pwm_clk); Best regards Uwe
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) { 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. + */ + pwm_imx27_wait_fifo_empty(chip, pwm); val = readl(imx->mmio_base + MX3_PWMSAR); - else + } else { val = imx->duty_cycle; + } tmp = NSEC_PER_SEC * (u64)(val) * prescaler; state->duty_cycle = DIV_ROUND_UP_ULL(tmp, pwm_clk);