Date:   Tue, 3 Jan 2017 12:43:14 +0100
From:   Lukasz Majewski <lukma@...x.de>
To:     Boris Brezillon <boris.brezillon@...e-electrons.com>
Cc:     Thierry Reding <thierry.reding@...il.com>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Stefan Agner <stefan@...er.ch>, linux-pwm@...r.kernel.org,
        Bhuvanchandra DV <bhuvanchandra.dv@...adex.com>,
        linux-kernel@...r.kernel.org,
        Fabio Estevam <fabio.estevam@....com>,
        Fabio Estevam <festevam@...il.com>,
        Lothar Wassmann <LW@...o-electronics.de>, kernel@...gutronix.de
Subject: Re: [PATCH v3 RESEND 07/11] pwm: imx: Provide atomic PWM support
 for i.MX PWMv2

Hi Boris,

> Hi Lukasz,
> 
> On Thu, 29 Dec 2016 17:45:35 +0100
> Lukasz Majewski <l.majewski@...ess.pl> wrote:
> 
> > Hi Boris,
> > 
> > > Hi Lukasz,
> > > 
> > > On Mon, 26 Dec 2016 23:55:57 +0100
> > > Lukasz Majewski <l.majewski@...ess.pl> wrote:
> > >   
> > > > This commit provides apply() callback implementation for i.MX's
> > > > PWMv2.
> > > > 
> > > > Suggested-by: Stefan Agner <stefan@...er.ch>
> > > > Suggested-by: Boris Brezillon
> > > > <boris.brezillon@...e-electrons.com> Signed-off-by: Lukasz
> > > > Majewski <l.majewski@...ess.pl> Reviewed-by: Boris Brezillon
> > > > <boris.brezillon@...e-electrons.com> ---
> > > > 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);
	}



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

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

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

In such a way we would preserve bisectibility.

What is your opinion?



[1] http://patchwork.ozlabs.org/patch/709510/

> 
> > 
> > > 
> > > That's just my opinion, but when you get reviews on a patch
> > > series, it's better to address them directly (especially when
> > > issues can be easily fixed) than provide follow-up patches.  
> > 
> > I do not have iMX7 for testing/development, so I could not reproduce
> > the error and address the issue directly.
> 
> Well, the description made by Stefan seemed pretty clear to me: you
> need to enable the periph clock before accessing PWM registers.
> 
> > 
> > I can at best integrate Stefan's patch and hope to not introduce
> > regression.
> 
> You can ask others to test your own patches. In this case, just
> clearly state that the patch is untested and that you'd like people
> owning a specific platform to test it.
> 
> Regards,
> 
> Boris




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@...x.de

Powered by blists - more mailing lists