| Message ID | 20191021105739.1357629-3-thierry.reding@gmail.com |
|---|---|
| State | Accepted |
| Commit | a3597d6c89d70ff0bcb1dd74dc0a88442fe79da6 |
| Headers | show |
| Series | [1/4] pwm: Read initial hardware state at request time | expand |
+Adam On 21. 10. 19 12:57, Thierry Reding wrote: > The hardware register containing the duty cycle value cannot be accessed > when the PWM is disabled. This causes the ->get_state() callback to read > back a duty cycle value of 0, which can confuse consumer drivers. Me and Adam Ford already tested the patches [3/4] and [4/4] and gave ours Tested-by tags in the previous thread but do not see those here. I re-tested these again and have no issues. Tested-by: Michal Vokáč <michal.vokac@ysoft.com> Thank you Thierry, Michal > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > --- > drivers/pwm/pwm-imx27.c | 31 ++++++++++++++++++++++++------- > 1 file changed, 24 insertions(+), 7 deletions(-) > > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > index ae11d8577f18..4113d5cd4c62 100644 > --- a/drivers/pwm/pwm-imx27.c > +++ b/drivers/pwm/pwm-imx27.c > @@ -85,6 +85,13 @@ struct pwm_imx27_chip { > struct clk *clk_per; > void __iomem *mmio_base; > struct pwm_chip chip; > + > + /* > + * The driver cannot read the current duty cycle from the hardware if > + * the hardware is disabled. Cache the last programmed duty cycle > + * value to return in that case. > + */ > + unsigned int duty_cycle; > }; > > #define to_pwm_imx27_chip(chip) container_of(chip, struct pwm_imx27_chip, chip) > @@ -155,14 +162,17 @@ static void pwm_imx27_get_state(struct pwm_chip *chip, > tmp = NSEC_PER_SEC * (u64)(period + 2); > state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > > - /* PWMSAR can be read only if PWM is enabled */ > - if (state->enabled) { > + /* > + * PWMSAR can be read only if PWM is enabled. If the PWM is disabled, > + * use the cached value. > + */ > + if (state->enabled) > val = readl(imx->mmio_base + MX3_PWMSAR); > - tmp = NSEC_PER_SEC * (u64)(val); > - state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > - } else { > - state->duty_cycle = 0; > - } > + else > + val = imx->duty_cycle; > + > + tmp = NSEC_PER_SEC * (u64)(val); > + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > > if (!state->enabled) > pwm_imx27_clk_disable_unprepare(chip); > @@ -261,6 +271,13 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); > writel(period_cycles, imx->mmio_base + MX3_PWMPR); > > + /* > + * Store the duty cycle for future reference in cases where > + * the MX3_PWMSAR register can't be read (i.e. when the PWM > + * is disabled). > + */ > + imx->duty_cycle = duty_cycles; > + > cr = MX3_PWMCR_PRESCALER_SET(prescale) | > MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | > FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) | >
On Mon, Oct 21, 2019 at 8:46 AM Michal Vokáč <michal.vokac@ysoft.com> wrote: > > +Adam > > On 21. 10. 19 12:57, Thierry Reding wrote: > > The hardware register containing the duty cycle value cannot be accessed > > when the PWM is disabled. This causes the ->get_state() callback to read > > back a duty cycle value of 0, which can confuse consumer drivers. > > Me and Adam Ford already tested the patches [3/4] and [4/4] and gave ours > Tested-by tags in the previous thread but do not see those here. > I re-tested these again and have no issues. > > Tested-by: Michal Vokáč <michal.vokac@ysoft.com> Tested-by: Adam Ford <aford173@gmail.com> > > Thank you Thierry, > Michal > > > > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > > --- > > drivers/pwm/pwm-imx27.c | 31 ++++++++++++++++++++++++------- > > 1 file changed, 24 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > > index ae11d8577f18..4113d5cd4c62 100644 > > --- a/drivers/pwm/pwm-imx27.c > > +++ b/drivers/pwm/pwm-imx27.c > > @@ -85,6 +85,13 @@ struct pwm_imx27_chip { > > struct clk *clk_per; > > void __iomem *mmio_base; > > struct pwm_chip chip; > > + > > + /* > > + * The driver cannot read the current duty cycle from the hardware if > > + * the hardware is disabled. Cache the last programmed duty cycle > > + * value to return in that case. > > + */ > > + unsigned int duty_cycle; > > }; > > > > #define to_pwm_imx27_chip(chip) container_of(chip, struct pwm_imx27_chip, chip) > > @@ -155,14 +162,17 @@ static void pwm_imx27_get_state(struct pwm_chip *chip, > > tmp = NSEC_PER_SEC * (u64)(period + 2); > > state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > > > > - /* PWMSAR can be read only if PWM is enabled */ > > - if (state->enabled) { > > + /* > > + * PWMSAR can be read only if PWM is enabled. If the PWM is disabled, > > + * use the cached value. > > + */ > > + if (state->enabled) > > val = readl(imx->mmio_base + MX3_PWMSAR); > > - tmp = NSEC_PER_SEC * (u64)(val); > > - state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > > - } else { > > - state->duty_cycle = 0; > > - } > > + else > > + val = imx->duty_cycle; > > + > > + tmp = NSEC_PER_SEC * (u64)(val); > > + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); > > > > if (!state->enabled) > > pwm_imx27_clk_disable_unprepare(chip); > > @@ -261,6 +271,13 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); > > writel(period_cycles, imx->mmio_base + MX3_PWMPR); > > > > + /* > > + * Store the duty cycle for future reference in cases where > > + * the MX3_PWMSAR register can't be read (i.e. when the PWM > > + * is disabled). > > + */ > > + imx->duty_cycle = duty_cycles; > > + > > cr = MX3_PWMCR_PRESCALER_SET(prescale) | > > MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | > > FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) | > > >
diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c index ae11d8577f18..4113d5cd4c62 100644 --- a/drivers/pwm/pwm-imx27.c +++ b/drivers/pwm/pwm-imx27.c @@ -85,6 +85,13 @@ struct pwm_imx27_chip { struct clk *clk_per; void __iomem *mmio_base; struct pwm_chip chip; + + /* + * The driver cannot read the current duty cycle from the hardware if + * the hardware is disabled. Cache the last programmed duty cycle + * value to return in that case. + */ + unsigned int duty_cycle; }; #define to_pwm_imx27_chip(chip) container_of(chip, struct pwm_imx27_chip, chip) @@ -155,14 +162,17 @@ static void pwm_imx27_get_state(struct pwm_chip *chip, tmp = NSEC_PER_SEC * (u64)(period + 2); state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); - /* PWMSAR can be read only if PWM is enabled */ - if (state->enabled) { + /* + * PWMSAR can be read only if PWM is enabled. If the PWM is disabled, + * use the cached value. + */ + if (state->enabled) val = readl(imx->mmio_base + MX3_PWMSAR); - tmp = NSEC_PER_SEC * (u64)(val); - state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); - } else { - state->duty_cycle = 0; - } + else + val = imx->duty_cycle; + + tmp = NSEC_PER_SEC * (u64)(val); + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk); if (!state->enabled) pwm_imx27_clk_disable_unprepare(chip); @@ -261,6 +271,13 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); writel(period_cycles, imx->mmio_base + MX3_PWMPR); + /* + * Store the duty cycle for future reference in cases where + * the MX3_PWMSAR register can't be read (i.e. when the PWM + * is disabled). + */ + imx->duty_cycle = duty_cycles; + cr = MX3_PWMCR_PRESCALER_SET(prescale) | MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) |
The hardware register containing the duty cycle value cannot be accessed when the PWM is disabled. This causes the ->get_state() callback to read back a duty cycle value of 0, which can confuse consumer drivers. Signed-off-by: Thierry Reding <thierry.reding@gmail.com> --- drivers/pwm/pwm-imx27.c | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-)