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

[Marco Felsch <m.felsch@pengutronix.de>]

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/ |



-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |