| Message ID | 20200909130739.26717-3-m.felsch@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PWM i.MX27 fix disabled state for inverted signals | expand |
Hello, the usage of "static" in the subject is unclear. I guess you mean: pwm: imx27: setup some PWMCR bits in .probe() On Wed, Sep 09, 2020 at 03:07:38PM +0200, Marco Felsch wrote: > The STOPEN, DOZEN, WAITEN, DBGEN and the CLKSRC bit values never change. > So it should be save to move this bit settings into probe() and change s/save/safe/ > only the necessary bits during apply(). Therefore I added the > pwm_imx27_update_bits() helper. > > Furthermore the patch adds the support to reset the pwm device during > probe() if the pwm device is disabled. IMHO this needs a better motivation ... > Both steps are required in preparation of the further patch which fixes > the "pwm-disabled" state for inverted pwms. ... and should maybe go into this other patch? > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > drivers/pwm/pwm-imx27.c | 41 +++++++++++++++++++++++++++++------------ > 1 file changed, 29 insertions(+), 12 deletions(-) > > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > index 3cf9f1774244..30388a9ece04 100644 > --- a/drivers/pwm/pwm-imx27.c > +++ b/drivers/pwm/pwm-imx27.c > @@ -96,6 +96,16 @@ struct pwm_imx27_chip { > > #define to_pwm_imx27_chip(chip) container_of(chip, struct pwm_imx27_chip, chip) > > +static void pwm_imx27_update_bits(void __iomem *reg, u32 mask, u32 val) > +{ > + u32 tmp; > + > + tmp = readl(reg); > + tmp &= ~mask; > + tmp |= val & mask; > + return writel(tmp, reg); > +} > + > static int pwm_imx27_clk_prepare_enable(struct pwm_imx27_chip *imx) > { > int ret; > @@ -183,10 +193,8 @@ static void pwm_imx27_get_state(struct pwm_chip *chip, > pwm_imx27_clk_disable_unprepare(imx); > } > > -static void pwm_imx27_sw_reset(struct pwm_chip *chip) > +static void pwm_imx27_sw_reset(struct pwm_imx27_chip *imx, struct device *dev) > { > - struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip); > - struct device *dev = chip->dev; This change is fine, but it does belong into a separate patch. > int wait_count = 0; > u32 cr; > > @@ -232,7 +240,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > unsigned long long c; > unsigned long long clkrate; > int ret; > - u32 cr; > + u32 cr, mask; > > ret = pwm_imx27_clk_prepare_enable(imx); > if (ret) > @@ -269,7 +277,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > if (cstate.enabled) { > pwm_imx27_wait_fifo_slot(chip, pwm); > } else { > - pwm_imx27_sw_reset(chip); > + pwm_imx27_sw_reset(imx, chip->dev); > } > > writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); > @@ -281,10 +289,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > */ > 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) | > - MX3_PWMCR_DBGEN; > + cr = MX3_PWMCR_PRESCALER_SET(prescale); > > if (state->polarity == PWM_POLARITY_INVERSED) > cr |= FIELD_PREP(MX3_PWMCR_POUTC, > @@ -293,7 +298,9 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > if (state->enabled) > cr |= MX3_PWMCR_EN; > > - writel(cr, imx->mmio_base + MX3_PWMCR); > + mask = MX3_PWMCR_PRESCALER | MX3_PWMCR_POUTC | MX3_PWMCR_EN; > + > + pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, cr); > > if (!state->enabled) > pwm_imx27_clk_disable_unprepare(imx); > @@ -364,10 +371,20 @@ static int pwm_imx27_probe(struct platform_device *pdev) > if (ret) > return ret; > > - /* keep clks on if pwm is running */ > + /* Keep clks on and pwm settings unchanged if the PWM is already running */ > pwmcr = readl(imx->mmio_base + MX3_PWMCR); > - if (!(pwmcr & MX3_PWMCR_EN)) > + if (!(pwmcr & MX3_PWMCR_EN)) { > + u32 mask; > + > + pwm_imx27_sw_reset(imx, &pdev->dev); > + mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | > + MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC; > + pwmcr = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | > + MX3_PWMCR_DBGEN | > + FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH); > + pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, pwmcr); > pwm_imx27_clk_disable_unprepare(imx); > + } So you don't enforce MX3_PWMCR_STOPEN (and the others) if the PWM is already running. Is this by design? Best regards Uwe
diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c index 3cf9f1774244..30388a9ece04 100644 --- a/drivers/pwm/pwm-imx27.c +++ b/drivers/pwm/pwm-imx27.c @@ -96,6 +96,16 @@ struct pwm_imx27_chip { #define to_pwm_imx27_chip(chip) container_of(chip, struct pwm_imx27_chip, chip) +static void pwm_imx27_update_bits(void __iomem *reg, u32 mask, u32 val) +{ + u32 tmp; + + tmp = readl(reg); + tmp &= ~mask; + tmp |= val & mask; + return writel(tmp, reg); +} + static int pwm_imx27_clk_prepare_enable(struct pwm_imx27_chip *imx) { int ret; @@ -183,10 +193,8 @@ static void pwm_imx27_get_state(struct pwm_chip *chip, pwm_imx27_clk_disable_unprepare(imx); } -static void pwm_imx27_sw_reset(struct pwm_chip *chip) +static void pwm_imx27_sw_reset(struct pwm_imx27_chip *imx, struct device *dev) { - struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip); - struct device *dev = chip->dev; int wait_count = 0; u32 cr; @@ -232,7 +240,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, unsigned long long c; unsigned long long clkrate; int ret; - u32 cr; + u32 cr, mask; ret = pwm_imx27_clk_prepare_enable(imx); if (ret) @@ -269,7 +277,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, if (cstate.enabled) { pwm_imx27_wait_fifo_slot(chip, pwm); } else { - pwm_imx27_sw_reset(chip); + pwm_imx27_sw_reset(imx, chip->dev); } writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); @@ -281,10 +289,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, */ 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) | - MX3_PWMCR_DBGEN; + cr = MX3_PWMCR_PRESCALER_SET(prescale); if (state->polarity == PWM_POLARITY_INVERSED) cr |= FIELD_PREP(MX3_PWMCR_POUTC, @@ -293,7 +298,9 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, if (state->enabled) cr |= MX3_PWMCR_EN; - writel(cr, imx->mmio_base + MX3_PWMCR); + mask = MX3_PWMCR_PRESCALER | MX3_PWMCR_POUTC | MX3_PWMCR_EN; + + pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, cr); if (!state->enabled) pwm_imx27_clk_disable_unprepare(imx); @@ -364,10 +371,20 @@ static int pwm_imx27_probe(struct platform_device *pdev) if (ret) return ret; - /* keep clks on if pwm is running */ + /* Keep clks on and pwm settings unchanged if the PWM is already running */ pwmcr = readl(imx->mmio_base + MX3_PWMCR); - if (!(pwmcr & MX3_PWMCR_EN)) + if (!(pwmcr & MX3_PWMCR_EN)) { + u32 mask; + + pwm_imx27_sw_reset(imx, &pdev->dev); + mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | + MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC; + pwmcr = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | + MX3_PWMCR_DBGEN | + FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH); + pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, pwmcr); pwm_imx27_clk_disable_unprepare(imx); + } return pwmchip_add(&imx->chip); }
The STOPEN, DOZEN, WAITEN, DBGEN and the CLKSRC bit values never change. So it should be save to move this bit settings into probe() and change only the necessary bits during apply(). Therefore I added the pwm_imx27_update_bits() helper. Furthermore the patch adds the support to reset the pwm device during probe() if the pwm device is disabled. Both steps are required in preparation of the further patch which fixes the "pwm-disabled" state for inverted pwms. Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- drivers/pwm/pwm-imx27.c | 41 +++++++++++++++++++++++++++++------------ 1 file changed, 29 insertions(+), 12 deletions(-)