Re: [PATCH v3 RESEND 07/11] pwm: imx: Provide atomic PWM support for i.MX PWMv2

[Boris Brezillon]

Hi Lukasz,

On Tue, 3 Jan 2017 12:43:14 +0100
Lukasz Majewski <lukma@xxxxxxx> wrote:

> Hi Boris,
> 
> > Hi Lukasz,
> > 
> > On Thu, 29 Dec 2016 17:45:35 +0100
> > Lukasz Majewski <l.majewski@xxxxxxxxx> wrote:
> >   
> > > Hi Boris,
> > >   
> > > > Hi Lukasz,
> > > > 
> > > > On Mon, 26 Dec 2016 23:55:57 +0100
> > > > Lukasz Majewski <l.majewski@xxxxxxxxx> wrote:
> > > >     
> > > > > This commit provides apply() callback implementation for i.MX's
> > > > > PWMv2.
> > > > > 
> > > > > Suggested-by: Stefan Agner <stefan@xxxxxxxx>
> > > > > Suggested-by: Boris Brezillon
> > > > > <boris.brezillon@xxxxxxxxxxxxxxxxxx> Signed-off-by: Lukasz
> > > > > Majewski <l.majewski@xxxxxxxxx> Reviewed-by: Boris Brezillon
> > > > > <boris.brezillon@xxxxxxxxxxxxxxxxxx> ---
> > > > > Changes for v3:
> > > > > - Remove ipg clock enable/disable functions
> > > > > 
> > > > > Changes for v2:
> > > > > - None
> > > > > ---
> > > > >  drivers/pwm/pwm-imx.c | 70
> > > > > +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file
> > > > > changed, 70 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > > > index ebe9b0c..cd53c05 100644
> > > > > --- a/drivers/pwm/pwm-imx.c
> > > > > +++ b/drivers/pwm/pwm-imx.c
> > > > > @@ -159,6 +159,75 @@ static void imx_pwm_wait_fifo_slot(struct
> > > > > pwm_chip *chip, }
> > > > >  }
> > > > >  
> > > > > +static int imx_pwm_apply_v2(struct pwm_chip *chip, struct
> > > > > pwm_device *pwm,
> > > > > +			    struct pwm_state *state)
> > > > > +{
> > > > > +	unsigned long period_cycles, duty_cycles, prescale;
> > > > > +	struct imx_chip *imx = to_imx_chip(chip);
> > > > > +	struct pwm_state cstate;
> > > > > +	unsigned long long c;
> > > > > +	u32 cr = 0;
> > > > > +	int ret;
> > > > > +
> > > > > +	pwm_get_state(pwm, &cstate);
> > > > > +
> > > > > +	c = clk_get_rate(imx->clk_per);
> > > > > +	c *= state->period;
> > > > > +
> > > > > +	do_div(c, 1000000000);
> > > > > +	period_cycles = c;
> > > > > +
> > > > > +	prescale = period_cycles / 0x10000 + 1;
> > > > > +
> > > > > +	period_cycles /= prescale;
> > > > > +	c = (unsigned long long)period_cycles *
> > > > > state->duty_cycle;
> > > > > +	do_div(c, state->period);
> > > > > +	duty_cycles = c;
> > > > > +
> > > > > +	/*
> > > > > +	 * according to imx pwm RM, the real period value
> > > > > should be
> > > > > +	 * PERIOD value in PWMPR plus 2.
> > > > > +	 */
> > > > > +	if (period_cycles > 2)
> > > > > +		period_cycles -= 2;
> > > > > +	else
> > > > > +		period_cycles = 0;
> > > > > +
> > > > > +	/* Enable the clock if the PWM is being enabled. */
> > > > > +	if (state->enabled && !cstate.enabled) {
> > > > > +		ret = clk_prepare_enable(imx->clk_per);
> > > > > +		if (ret)
> > > > > +			return ret;
> > > > > +	}
> > > > > +
> > > > > +	/*
> > > > > +	 * Wait for a free FIFO slot if the PWM is already
> > > > > enabled, and flush
> > > > > +	 * the FIFO if the PWM was disabled and is about to be
> > > > > enabled.
> > > > > +	 */
> > > > > +	if (cstate.enabled)
> > > > > +		imx_pwm_wait_fifo_slot(chip, pwm);
> > > > > +	else if (state->enabled)
> > > > > +		imx_pwm_sw_reset(chip);
> > > > > +
> > > > > +	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> > > > > +	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> > > > > +
> > > > > +	cr |= MX3_PWMCR_PRESCALER(prescale) |
> > > > > +	      MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
> > > > > +	      MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH;
> > > > > +
> > > > > +	if (state->enabled)
> > > > > +		cr |= MX3_PWMCR_EN;
> > > > > +
> > > > > +	writel(cr, imx->mmio_base + MX3_PWMCR);
> > > > > +
> > > > > +	/* Disable the clock if the PWM is being disabled. */
> > > > > +	if (!state->enabled && cstate.enabled)
> > > > > +		clk_disable_unprepare(imx->clk_per);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +    
> > > > 
> > > > Stefan suggested to rework this function to avoid unneeded
> > > > duty/period calculation and reg write when disabling the PWM. Why
> > > > didn't you send a v4 addressing that instead of resending the
> > > > exact same v3?    
> > > 
> > > The discussion between you and Stefan was in this thread:
> > > http://patchwork.ozlabs.org/patch/689790/
> > > 
> > > Stefan proposed change, you replied with your concerns and that is
> > > all.  
> >   
> 
> Please correct me if I've misunderstood something :-).
> 
> > Well, regarding the imx_pwm_apply_v2() suggested by Stefan, I think we
> > both agreed that most of the code was unneeded when all we want to do
> > is disable the PWM.  
> 
> So for the PATCH 7/11 we fix the issue with recalculating clocks
> when we want to disable PWM.
> 	
> if (state->enabled) {
> 		c = clk_get_rate(imx->clk_per);
> 		c *= state->period;
> 
> 		do_div(c, 1000000000);
> 		period_cycles = c;
> 
> 		prescale = period_cycles / 0x10000 + 1;
> 
> 		period_cycles /= prescale;
> 		c = (unsigned long long)period_cycles *
> 		    state->duty_cycle;
> 		do_div(c, state->period);
> 		duty_cycles = c;
> 
> 		/*
> 		 * According to imx pwm RM, the real period value
> 		 * should be PERIOD value in PWMPR plus 2.
> 		 */
> 		if (period_cycles > 2)
> 			period_cycles -= 2;
> 		else
> 			period_cycles = 0;
> 
> 		/*
> 		 * Enable the clock if the PWM is not already
> 		 * enabled.
> 		 */
> 		if (!cstate.enabled) {
> 			ret = clk_prepare_enable(imx->clk_per);
> 			if (ret)
> 			return ret;
> 		}
> 
> 		/*
> 		 * Wait for a free FIFO slot if the PWM is already
> 		 * enabled, and flush the FIFO if the PWM was disabled
> 		 * and is about to be enabled.
> 		 */
> 		if (cstate.enabled)
> 			imx_pwm_wait_fifo_slot(chip, pwm);
> 		else
> 			imx_pwm_sw_reset(chip);
> 
> 		writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> 		writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> 
> 		writel(MX3_PWMCR_PRESCALER(prescale) |
> 		       MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
> 		       MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH |
> 		       MX3_PWMCR_EN,
> 		       imx->mmio_base + MX3_PWMCR);
> 	} else {
> 
> 		writel(0, imx->mmio_base + MX3_PWMCR);
> 
> 		/* Disable the clock if the PWM is currently enabled. */
> 		if (cstate.enabled)
> 			clk_disable_unprepare(imx->clk_per);
> 	}
> 
> 

Yep.

> 
> > 
> > My concern was more about the way PWM changes are applied (->apply()
> > returns before the change is actually applied), but I agreed that it
> > could be fixed later on (if other people think it's really needed),
> > since the existing code already handles it this way.  
> 
> This is the issue with FIFO setting - but for now we do not deal with
> it.

Exactly.

> 
> >   
> > > No clear decision what to change until today when Stefan prepared
> > > separate (concise) patch (now I see what is the problem).
> > >   
> > 
> > The patch proposed by Stefan is addressing a different problem: the
> > periph clock has to be enabled before accessing registers.  
> 
> So for this reason Stefan's patch [1] always enable the clock no matter
> if PWM clock is generated or not.

Yes.

> 
> >   
> > >   
> > > > 
> > > > Same goes for the regression introduced in patch 2: I think it's
> > > > better to keep things bisectable on all platforms (even if it
> > > > appeared to work by chance on imx7, it did work before this
> > > > change).    
> > > 
> > > Could you be more specific about your idea to solve this problem?  
> > 
> > Stefan already provided a patch, I just think it should be fixed
> > before patch 2 to avoid breaking bisectibility.  
> 
> My idea is as follows:
> 
> I will drop patch v2 (prepared by Sasha) and then squash Stefan's patch
> [1] to patch 7/11. The "old" ipg enable code will be removed with other
> not needed code during conversion.

How about keeping patch 2 but enabling/disabling the periph clk
in imx_pwm_config() instead of completely dropping the enable/disable
clk sequence.

In patch 7 you just add the logic we talked about earlier:
unconditionally enable the periph clk when entering the
imx_pwm_apply_v2() function and disable it before leaving the function.

This way you can preserve bisectibility and still get rid of the ipg
clk.

Stefan, what's your opinion?

Regards,

Boris