diff mbox series

[v2,2/5] pwm: imx27: move constant PWMCR register values into probe

Message ID 20200925155330.32301-3-m.felsch@pengutronix.de (mailing list archive)
State New, archived
Headers show
Series PWM i.MX27 fix disabled state for inverted signals | expand

Commit Message

Marco Felsch Sept. 25, 2020, 3:53 p.m. UTC 
The STOPEN, DOZEN, WAITEN, DBGEN and the CLKSRC bit values never change.
So it should be safe to move this bit settings into probe() and change
only the necessary bits during apply(). Therefore I added the
pwm_imx27_update_bits() helper.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
v2:
- drop software reset from the logic
- fix setting STOPEN, DOZEN, WAITEN and DBGEN bits in case the pwm is
  already enabled

 drivers/pwm/pwm-imx27.c | 37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)

Comments

Uwe Kleine-König Sept. 26, 2020, 1:46 p.m. UTC | #1 
On Fri, Sep 25, 2020 at 05:53:27PM +0200, Marco Felsch wrote:
> The STOPEN, DOZEN, WAITEN, DBGEN and the CLKSRC bit values never change.
> So it should be safe to move this bit settings into probe() and change
> only the necessary bits during apply(). Therefore I added the
> pwm_imx27_update_bits() helper.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> v2:
> - drop software reset from the logic
> - fix setting STOPEN, DOZEN, WAITEN and DBGEN bits in case the pwm is
>   already enabled
> 
>  drivers/pwm/pwm-imx27.c | 37 ++++++++++++++++++++++++++++---------
>  1 file changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index 7edac4ac6395..b761764b8375 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;
> @@ -221,7 +231,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)
> @@ -267,10 +277,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,
> @@ -279,7 +286,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 (imx->enabled != state->enabled) {
>  		if (state->enabled) {
> @@ -314,7 +323,7 @@ static int pwm_imx27_probe(struct platform_device *pdev)
>  {
>  	struct pwm_imx27_chip *imx;
>  	int ret;
> -	u32 pwmcr;
> +	u32 pwmcr, mask;
>  
>  	imx = devm_kzalloc(&pdev->dev, sizeof(*imx), GFP_KERNEL);
>  	if (imx == NULL)
> @@ -361,10 +370,20 @@ static int pwm_imx27_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	/* keep clks on if pwm is running */
> +	mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> +	       MX3_PWMCR_DBGEN;
> +	pwmcr = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> +		MX3_PWMCR_DBGEN;
> +	pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, pwmcr);
> +
> +	/* keep clks on and clk settings unchanged if pwm is running */
>  	pwmcr = readl(imx->mmio_base + MX3_PWMCR);
> -	if (!(pwmcr & MX3_PWMCR_EN))
> +	if (!(pwmcr & MX3_PWMCR_EN)) {
> +		mask = MX3_PWMCR_CLKSRC;
> +		pwmcr = 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);
> +	}

You're doing more register accesses here than necessary, that is 3 reads
and two writes while one read and (maybe) one write would be enough.
(Though this doesn't work if you want to use the pwm_imx27_update_bits
helper.)

You'd need to do something like:

	val = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | MX3_PWMCR_DBGEN;
	mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | MX3_PWMCR_DBGEN;

	pwmcr = readl(imx->mmio_base + MX3_PWMCR);

	if (pwmcr & MX3_PWMCR_EN) {
		imx->enabled = true;
	} else {
		val |= FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH);
		mask |= MX3_PWMCR_CLKSRC;
	}

	pwmcr_new = (pwmcr & ~mask) | val;

	if (pwmcr_new != val)
		writel(imx->mmio_base + MX3_PWMCR, pwmcr_new);

	if (!imx->enabled)
		pwm_imx27_clk_disable_unprepare(imx);

Hmm, not sure this is pretty enough to actually recommend this. I let
you decide.

>  	return pwmchip_add(&imx->chip);

Best regards
Uwe
Marco Felsch Sept. 28, 2020, 5:50 a.m. UTC | #2 
On 20-09-26 15:46, Uwe Kleine-König wrote:
> On Fri, Sep 25, 2020 at 05:53:27PM +0200, Marco Felsch wrote:
> > The STOPEN, DOZEN, WAITEN, DBGEN and the CLKSRC bit values never change.
> > So it should be safe to move this bit settings into probe() and change
> > only the necessary bits during apply(). Therefore I added the
> > pwm_imx27_update_bits() helper.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > v2:
> > - drop software reset from the logic
> > - fix setting STOPEN, DOZEN, WAITEN and DBGEN bits in case the pwm is
> >   already enabled
> > 
> >  drivers/pwm/pwm-imx27.c | 37 ++++++++++++++++++++++++++++---------
> >  1 file changed, 28 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> > index 7edac4ac6395..b761764b8375 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;
> > @@ -221,7 +231,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)
> > @@ -267,10 +277,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,
> > @@ -279,7 +286,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 (imx->enabled != state->enabled) {
> >  		if (state->enabled) {
> > @@ -314,7 +323,7 @@ static int pwm_imx27_probe(struct platform_device *pdev)
> >  {
> >  	struct pwm_imx27_chip *imx;
> >  	int ret;
> > -	u32 pwmcr;
> > +	u32 pwmcr, mask;
> >  
> >  	imx = devm_kzalloc(&pdev->dev, sizeof(*imx), GFP_KERNEL);
> >  	if (imx == NULL)
> > @@ -361,10 +370,20 @@ static int pwm_imx27_probe(struct platform_device *pdev)
> >  	if (ret)
> >  		return ret;
> >  
> > -	/* keep clks on if pwm is running */
> > +	mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> > +	       MX3_PWMCR_DBGEN;
> > +	pwmcr = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> > +		MX3_PWMCR_DBGEN;
> > +	pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, pwmcr);
> > +
> > +	/* keep clks on and clk settings unchanged if pwm is running */
> >  	pwmcr = readl(imx->mmio_base + MX3_PWMCR);
> > -	if (!(pwmcr & MX3_PWMCR_EN))
> > +	if (!(pwmcr & MX3_PWMCR_EN)) {
> > +		mask = MX3_PWMCR_CLKSRC;
> > +		pwmcr = 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);
> > +	}
> 
> You're doing more register accesses here than necessary, that is 3 reads
> and two writes while one read and (maybe) one write would be enough.
> (Though this doesn't work if you want to use the pwm_imx27_update_bits
> helper.)

This shouldn't be a real timing issue.

> You'd need to do something like:
> 
> 	val = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | MX3_PWMCR_DBGEN;
> 	mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | MX3_PWMCR_DBGEN;
> 
> 	pwmcr = readl(imx->mmio_base + MX3_PWMCR);
> 
> 	if (pwmcr & MX3_PWMCR_EN) {
> 		imx->enabled = true;
> 	} else {
> 		val |= FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH);
> 		mask |= MX3_PWMCR_CLKSRC;
> 	}
> 
> 	pwmcr_new = (pwmcr & ~mask) | val;
> 
> 	if (pwmcr_new != val)
> 		writel(imx->mmio_base + MX3_PWMCR, pwmcr_new);
> 
> 	if (!imx->enabled)
> 		pwm_imx27_clk_disable_unprepare(imx);
> 
> Hmm, not sure this is pretty enough to actually recommend this. I let
> you decide.

I don't like this version since it is more confusing.

Regards,
  Marco

> >  	return pwmchip_add(&imx->chip);
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |
diff mbox series

Patch

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index 7edac4ac6395..b761764b8375 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;
@@ -221,7 +231,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)
@@ -267,10 +277,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,
@@ -279,7 +286,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 (imx->enabled != state->enabled) {
 		if (state->enabled) {
@@ -314,7 +323,7 @@  static int pwm_imx27_probe(struct platform_device *pdev)
 {
 	struct pwm_imx27_chip *imx;
 	int ret;
-	u32 pwmcr;
+	u32 pwmcr, mask;
 
 	imx = devm_kzalloc(&pdev->dev, sizeof(*imx), GFP_KERNEL);
 	if (imx == NULL)
@@ -361,10 +370,20 @@  static int pwm_imx27_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	/* keep clks on if pwm is running */
+	mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
+	       MX3_PWMCR_DBGEN;
+	pwmcr = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
+		MX3_PWMCR_DBGEN;
+	pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, pwmcr);
+
+	/* keep clks on and clk settings unchanged if pwm is running */
 	pwmcr = readl(imx->mmio_base + MX3_PWMCR);
-	if (!(pwmcr & MX3_PWMCR_EN))
+	if (!(pwmcr & MX3_PWMCR_EN)) {
+		mask = MX3_PWMCR_CLKSRC;
+		pwmcr = 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);
 }