Re: [PATCH v2 3/5] pwm: imx27: reset the PWM if it is not running

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

On 20-09-28 09:30, Uwe Kleine-König wrote:
> On Fri, Sep 25, 2020 at 05:53:28PM +0200, Marco Felsch wrote:
> > Trigger a software reset during probe to clear the FIFO and reset the
> > register values to their default. According the datasheet the DBGEN,
> > STOPEN, DOZEN and WAITEN bits should be untouched by the software reset
> > but this is not the case.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > v2:
> > - new patch
> > 
> >  drivers/pwm/pwm-imx27.c | 26 ++++++++++++++------------
> >  1 file changed, 14 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> > index b761764b8375..3b6bcd8d58b7 100644
> > --- a/drivers/pwm/pwm-imx27.c
> > +++ b/drivers/pwm/pwm-imx27.c
> > @@ -183,10 +183,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;
> 
> This is an unrelated hunk that I don't expect to result in any changes
> in the code. If you consider it better this way, you should at least
> mention it in the commit log.

IMO this is required due to the usage from the probe. I'm not a fan of
passing the 'struct pwm_chip' before initializing it. So it is not
unrelated. I will mention it in v3.

> 
> > @@ -266,7 +264,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	if (imx->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);
> >  	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> > @@ -370,19 +368,23 @@ static int pwm_imx27_probe(struct platform_device *pdev)
> >  	if (ret)
> >  		return ret;
> >  
> > -	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)) {
> > -		mask = MX3_PWMCR_CLKSRC;
> > -		pwmcr = FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH);
> > +		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);
> > +	} else {
> > +		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);
> >  	}
> 
> IMHO this is worse than the stuff I suggested for one of the earlier
> patches because there is much repetition. I'd put
> 
> 	mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | MX3_PWMCR_DBGEN;
> 	value = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | MX3_PWMCR_DBGEN;
> 
> before the if and just modify as necessary in the first branch of the
> if.

I've changed the behaviour in v3.

Regards,
  Marco

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