Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

[Sascha Hauer]

On Thu, Oct 27, 2016 at 09:40:05AM +0200, Boris Brezillon wrote:
> On Thu, 27 Oct 2016 08:29:39 +0200
> Lukasz Majewski <l.majewski@xxxxxxxxx> wrote:
> 
> > The code has been rewritten to remove "generic" calls to
> > imx_pwm_{enable|disable|config}.
> > 
> > Such approach would facilitate switch to atomic PWM (a.k.a ->apply())
> > implementation.
> > 
> > Suggested-by: Stefan Agner <stefan@xxxxxxxx>
> > Suggested-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Lukasz Majewski <l.majewski@xxxxxxxxx>
> > ---
> > Changes for v2:
> > - Add missing clock unprepare for clk_ipg
> > - Enable peripheral PWM clock (clk_per)
> > ---
> >  drivers/pwm/pwm-imx.c | 50 ++++++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 38 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > index ea3ce79..822eb5a 100644
> > --- a/drivers/pwm/pwm-imx.c
> > +++ b/drivers/pwm/pwm-imx.c
> > @@ -65,8 +65,6 @@ struct imx_chip {
> >  static int imx_pwm_config_v1(struct pwm_chip *chip,
> >  		struct pwm_device *pwm, int duty_ns, int period_ns)
> >  {
> > -	struct imx_chip *imx = to_imx_chip(chip);
> > -
> >  	/*
> >  	 * The PWM subsystem allows for exact frequencies. However,
> >  	 * I cannot connect a scope on my device to the PWM line and
> > @@ -84,26 +82,56 @@ static int imx_pwm_config_v1(struct pwm_chip *chip,
> >  	 * both the prescaler (/1 .. /128) and then by CLKSEL
> >  	 * (/2 .. /16).
> >  	 */
> > +	struct imx_chip *imx = to_imx_chip(chip);
> >  	u32 max = readl(imx->mmio_base + MX1_PWMP);
> >  	u32 p = max * duty_ns / period_ns;
> > +	int ret;
> > +
> > +	ret = clk_prepare_enable(imx->clk_ipg);
> > +	if (ret)
> > +		return ret;
> > +
> >  	writel(max - p, imx->mmio_base + MX1_PWMS);
> >  
> > +	clk_disable_unprepare(imx->clk_ipg);
> > +
> >  	return 0;
> >  }
> >  
> > -static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
> > +static int imx_pwm_enable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
> >  {
> >  	struct imx_chip *imx = to_imx_chip(chip);
> > +	int ret;
> >  	u32 val;
> >  
> > +	ret = clk_prepare_enable(imx->clk_ipg);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = clk_prepare_enable(imx->clk_per);
> > +	if (ret)
> > +		return ret;
> > +
> >  	val = readl(imx->mmio_base + MX1_PWMC);
> > +	val |= MX1_PWMC_EN;
> > +	writel(val, imx->mmio_base + MX1_PWMC);
> >  
> > -	if (enable)
> > -		val |= MX1_PWMC_EN;
> > -	else
> > -		val &= ~MX1_PWMC_EN;
> > +	clk_disable_unprepare(imx->clk_ipg);
> > +
> > +	return 0;
> > +}
> > +
> > +static void imx_pwm_disable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct imx_chip *imx = to_imx_chip(chip);
> > +	u32 val;
> > +
> > +	val = readl(imx->mmio_base + MX1_PWMC);
> > +	val &= ~MX1_PWMC_EN;
> >  
> >  	writel(val, imx->mmio_base + MX1_PWMC);
> 
> Are you sure you don't need to enable the ipg clk when manipulating the
> PWMC register?
> If it's not needed here, then it's probably not needed in
> imx_pwm_enable_v1() either.

As said, even the commit 7b27c160c68 introducing the register clk did not
enable the clock consistently for all register accesses. Maybe it's best
to include the following patch so that we can find a clear culprit and
do not bury the ipg clock changes in larger patches.

Sascha

-----------------------------8<-----------------------------------