| Message ID | 20240224112902.55539-1-Leif.Middelschulte@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v4,1/2] pwm: imx27: fix race condition .apply,.get_state | expand |
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@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 | 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
Hello, On Sat, Feb 24, 2024 at 12:29:00PM +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> Another few things I noticed only after replying to this patch and trying to apply #2: - Please make sure you have a S-o-b line for the sender (i.e. you with your gmail identity). - My reply to your klsmartin.com address couldn't be delivered, The MS mailserver told: "leif.middelschulte wasn't found at klsmartin.com." - Please start a new thread if you send a v5, as applying patches from a thread containing many patches is a bit less trivial. - Consider using git format-patch's --base parameter to document your patch base. Best regards Uwe
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 /* 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); + int tries = MX3_PWM_FIFOAV_EMPTY_LOOP; + int fifoav, previous_fifoav = INT_MAX; + 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 0; + /* if the FIFO value does not decrease, there is another problem */ + if (previous_fifoav == fifoav) + break; + previous_fifoav = fifoav; + msleep(period_ms); + } + + return -EAGAIN; +} + static int pwm_imx27_get_state(struct pwm_chip *chip, struct pwm_device *pwm, struct pwm_state *state) { @@ -161,10 +187,35 @@ 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. + */ + ret = pwm_imx27_wait_fifo_empty(chip, pwm); + if (ret) + return ret; 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);