Date:   Wed, 1 Mar 2017 15:56:26 +0200
From:   m18063 <Claudiu.Beznea@...rochip.com>
To:     Boris Brezillon <boris.brezillon@...e-electrons.com>
CC:     <thierry.reding@...il.com>, <robh+dt@...nel.org>,
        <pawel.moll@....com>, <mark.rutland@....com>,
        <ijc+devicetree@...lion.org.uk>, <galak@...eaurora.org>,
        <alexandre.belloni@...e-electrons.com>,
        <linux-pwm@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 1/2] drivers: pwm: pwm-atmel: switch to atomic PWM

Hi Boris,

Thank you for the closer review. Please see my answers
inline.

Thank you,
Claudiu


On 28.02.2017 23:07, Boris Brezillon wrote:
> Hi Claudiu,
>
> On Tue, 28 Feb 2017 12:40:14 +0200
> Claudiu Beznea <claudiu.beznea@...rochip.com> wrote:
>
>> The currently Atmel PWM controllers supported by this driver
>> could change period and duty factor without channel disable
>> (sama5d3 supports this by using period and duty factor update
>> registers, sam9rl support this by writing channel update
>> register and select the corresponding update: period or duty
>> factor).
> Hm, I had a closer a look at the datasheet, and I'm not sure this is
> possible in a safe way. AFAICT (I might be wrong), there's no way to
> atomically update both the period and duty cycles. So, imagine you're
> just at the end of the current period and you update one of the 2 params
> (either the period or the duty cycle) before the end of the period, but
> the other one is updated just after the beginning of a new period.
> During one cycle you'll have a bad config.
I was also think at this scenario. I thought that this should be
good for most of the cases.
>
> While this should be acceptable in most cases, there are a few cases
> where glitches are not permitted (when the PWM drives a critical
> device). Also, I don't know what happens if we have duty > period.
duty > period is checked by the PWM core before calling apply method.
>
>> The chip doesn't support run time changings of signal
>> polarity. This has been adapted by first disabling the channel,
>> update registers and the enabling the channel.
> Yep. I agree with that one.
>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@...rochip.com>
>> ---
>>  drivers/pwm/pwm-atmel.c | 217 ++++++++++++++++++++++++++----------------------
>>  1 file changed, 118 insertions(+), 99 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
>> index 67a7023..014b86c 100644
>> --- a/drivers/pwm/pwm-atmel.c
>> +++ b/drivers/pwm/pwm-atmel.c
>> @@ -68,7 +68,7 @@ struct atmel_pwm_chip {
>>  	struct mutex isr_lock;
>>  
>>  	void (*config)(struct pwm_chip *chip, struct pwm_device *pwm,
>> -		       unsigned long dty, unsigned long prd);
>> +		       unsigned long dty, unsigned long prd, bool enabled);
>>  };
>>  
>>  static inline struct atmel_pwm_chip *to_atmel_pwm_chip(struct pwm_chip *chip)
>> @@ -105,99 +105,120 @@ static inline void atmel_pwm_ch_writel(struct atmel_pwm_chip *chip,
>>  	writel_relaxed(val, chip->base + base + offset);
>>  }
>>  
>> -static int atmel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>> -			    int duty_ns, int period_ns)
>> +static int atmel_pwm_config_prepare(struct pwm_chip *chip,
>> +				    struct pwm_state *state, unsigned long *prd,
>> +				    unsigned long *dty, unsigned int *pres)
> I'd rename the function atmel_pwm_calculate_params(). Also, when the
> period does not change we don't have to calculate prd and pres, so
> maybe we should have one function to calculate prd+pres and another one
> to calculate dty:
I agree. I didn't want to change the current implementation from
this point of view.
> static int atmel_pwm_calculate_cprd_and_pres(struct pwm_chip *chip,
> 					const struct pwm_state *state,
> 					u32 *cprd, u32 *pres)
> {
> 	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
> 	unsigned long long cycles = state->period;
>
>         /* Calculate the period cycles and prescale value */
>         cycles *= clk_get_rate(atmel_pwm->clk);
>         do_div(cycles, NSEC_PER_SEC);
>
>         for (*pres = 0; cycles > PWM_MAX_PRD; cycles >>= 1)
>                 (*pres)++;
>
>         if (*pres > PRD_MAX_PRES) {
>                 dev_err(chip->dev, "pres exceeds the maximum value\n");
>                 return -EINVAL;
>         }
>
>         *cprd = cycles;
>
>         return 0;
> }
>
> static void atmel_pwm_calculate_cdty(const struct pwm_state *state,
>                                      u32 cprd, u32 *cdty)
> {
>         unsigned long long cycles = state->duty_cycle;
>
>         cycles *= cprd;
>         do_div(cycles, state->period);
>
>         *cdty = cycles;
> }
>
>
>>  {
>>  	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
>> -	unsigned long prd, dty;
>>  	unsigned long long div;
>> -	unsigned int pres = 0;
>> -	u32 val;
>> -	int ret;
>> -
>> -	if (pwm_is_enabled(pwm) && (period_ns != pwm_get_period(pwm))) {
>> -		dev_err(chip->dev, "cannot change PWM period while enabled\n");
>> -		return -EBUSY;
>> -	}
>>  
>>  	/* Calculate the period cycles and prescale value */
>> -	div = (unsigned long long)clk_get_rate(atmel_pwm->clk) * period_ns;
>> +	div = (unsigned long long)clk_get_rate(atmel_pwm->clk) * state->period;
>>  	do_div(div, NSEC_PER_SEC);
>>  
>> +	*pres = 0;
>>  	while (div > PWM_MAX_PRD) {
>>  		div >>= 1;
>> -		pres++;
>> +		(*pres)++;
>>  	}
>>  
>> -	if (pres > PRD_MAX_PRES) {
>> +	if (*pres > PRD_MAX_PRES) {
>>  		dev_err(chip->dev, "pres exceeds the maximum value\n");
>>  		return -EINVAL;
>>  	}
>>  
>>  	/* Calculate the duty cycles */
>> -	prd = div;
>> -	div *= duty_ns;
>> -	do_div(div, period_ns);
>> -	dty = prd - div;
>> +	*prd = div;
>> +	div *= state->duty_cycle;
>> +	do_div(div, state->period);
>> +	*dty = *prd - div;
> Not sure this subtraction is needed, you just need to revert the
> polarity selection (if polarity == NORMAL => set CPOL, otherwise, clear
> it).
I didn't want to change the existing implementation.
>
>>  
>> -	ret = clk_enable(atmel_pwm->clk);
>> -	if (ret) {
>> -		dev_err(chip->dev, "failed to enable PWM clock\n");
>> -		return ret;
>> -	}
>> +	return 0;
>> +}
>> +
>> +static void atmel_pwm_config_set(struct pwm_chip *chip, struct pwm_device *pwm,
>> +				 unsigned long dty, unsigned long prd,
>> +				 unsigned long pres, enum pwm_polarity polarity,
>> +				 bool enabled)
>> +{
>> +	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
>> +	u32 val;
>>  
>>  	/* It is necessary to preserve CPOL, inside CMR */
>>  	val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
>>  	val = (val & ~PWM_CMR_CPRE_MSK) | (pres & PWM_CMR_CPRE_MSK);
>> +	if (polarity == PWM_POLARITY_NORMAL)
>> +		val &= ~PWM_CMR_CPOL;
>> +	else
>> +		val |= PWM_CMR_CPOL;
>>  	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
>> -	atmel_pwm->config(chip, pwm, dty, prd);
>> +	atmel_pwm->config(chip, pwm, dty, prd, enabled);
>>  	mutex_lock(&atmel_pwm->isr_lock);
>>  	atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR);
>>  	atmel_pwm->updated_pwms &= ~(1 << pwm->hwpwm);
>>  	mutex_unlock(&atmel_pwm->isr_lock);
>> -
>> -	clk_disable(atmel_pwm->clk);
>> -	return ret;
>>  }
>>  
> You can move the code in atmel_pwm_config_set() directly in
> atmel_pwm_apply().
Ok. I will.
>
>>  static void atmel_pwm_config_v1(struct pwm_chip *chip, struct pwm_device *pwm,
>> -				unsigned long dty, unsigned long prd)
>> +				unsigned long dty, unsigned long prd,
>> +				bool enabled)
>>  {
>>  	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
>>  	unsigned int val;
>> +	unsigned long timeout;
>>  
>> +	if (pwm_is_enabled(pwm) && enabled) {
>> +		/* Update duty factor. */
>> +		val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
>> +		val &= ~PWM_CMR_UPD_CDTY;
>> +		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
>> +		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CUPD, dty);
>>  
>> -	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CUPD, dty);
>> -
>> -	val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
>> -	val &= ~PWM_CMR_UPD_CDTY;
>> -	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
>> -
>> -	/*
>> -	 * If the PWM channel is enabled, only update CDTY by using the update
>> -	 * register, it needs to set bit 10 of CMR to 0
>> -	 */
>> -	if (pwm_is_enabled(pwm))
>> -		return;
>> -	/*
>> -	 * If the PWM channel is disabled, write value to duty and period
>> -	 * registers directly.
>> -	 */
>> -	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CDTY, dty);
>> -	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CPRD, prd);
>> +		/*
>> +		 * Wait for the duty factor update.
>> +		 */
>> +		mutex_lock(&atmel_pwm->isr_lock);
>> +		atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR) &
>> +				~(1 << pwm->hwpwm);
>> +
>> +		timeout = jiffies + 2 * HZ;
>> +		while (!(atmel_pwm->updated_pwms & (1 << pwm->hwpwm)) &&
>> +		       time_before(jiffies, timeout)) {
>> +			usleep_range(10, 100);
>> +			atmel_pwm->updated_pwms |=
>> +					atmel_pwm_readl(atmel_pwm, PWM_ISR);
>> +		}
>> +
>> +		mutex_unlock(&atmel_pwm->isr_lock);
>> +
>> +		/* Update period. */
>> +		val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
>> +		val |= PWM_CMR_UPD_CDTY;
>> +		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
>> +		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CUPD, prd);
> As I said above, I'm almost sure it's not 100% safe to update both
> parameters. We'd better stick to the existing implementation and see if
> new IPs provide an atomic period+duty update feature.
There is such approach only for synchronous channels, which I didn't cover
in this implementation.
>
>> +	} else {
>> +		/*
>> +		 * If the PWM channel is disabled, write value to duty and
>> +		 * period registers directly.
>> +		 */
>> +		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CDTY, dty);
>> +		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CPRD, prd);
>> +	}
>>  }
> There are 2 different cases in this _config() function, and
> atmel_pwm_apply() is already doing different things when the PWM is
> enabled than when it's disabled, so maybe it's worth creating 2
> different hooks, one for the update-while-running case, and another for
> for the initialization case.
>
> How about having the following hooks in atmel_pwm_data?
>
> 	void (*update_cdty)(struct pwm_chip *chip,
>                             struct pwm_device *pwm,
>                             u32 cdty);
>         void (*set_cprd_cdty)(struct pwm_chip *chip,
>                               struct pwm_device *pwm,
>                               u32 cprd, u32 cdty);
I agree.
>
>>  
>>  static void atmel_pwm_config_v2(struct pwm_chip *chip, struct pwm_device *pwm,
>> -				unsigned long dty, unsigned long prd)
>> +				unsigned long dty, unsigned long prd,
>> +				bool enabled)
>>  {
>>  	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
>>  
>> -	if (pwm_is_enabled(pwm)) {
>> +	if (pwm_is_enabled(pwm) && enabled) {
>>  		/*
>> -		 * If the PWM channel is enabled, using the duty update register
>> -		 * to update the value.
>> +		 * If the PWM channel is enabled, use update registers
>> +		 * to update the duty and period.
>>  		 */
>>  		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CDTYUPD, dty);
>> +		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CPRDUPD, prd);
> Same here, I'm not convinced we are guaranteed that both parameters will
> be applied atomically. There's a sync mechanism described in the
> datasheet, but I'm not sure I understand how it works, and anyway,
> you're not using it here, so let's stick to the "update duty only"
> approach for now.
Only for synchronous channels the atomicity is granted. I didn't cover that
in this commit. With this approach the dty, period will be changed at the
next period but there is no guarantee that they will be synchronously
updated.

>
>>  	} else {
>>  		/*
>>  		 * If the PWM channel is disabled, write value to duty and
>> @@ -208,49 +229,6 @@ static void atmel_pwm_config_v2(struct pwm_chip *chip, struct pwm_device *pwm,
>>  	}
>>  }
>>  
>> -static int atmel_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
>> -				  enum pwm_polarity polarity)
>> -{
>> -	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
>> -	u32 val;
>> -	int ret;
>> -
>> -	val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
>> -
>> -	if (polarity == PWM_POLARITY_NORMAL)
>> -		val &= ~PWM_CMR_CPOL;
>> -	else
>> -		val |= PWM_CMR_CPOL;
>> -
>> -	ret = clk_enable(atmel_pwm->clk);
>> -	if (ret) {
>> -		dev_err(chip->dev, "failed to enable PWM clock\n");
>> -		return ret;
>> -	}
>> -
>> -	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
>> -
>> -	clk_disable(atmel_pwm->clk);
>> -
>> -	return 0;
>> -}
>> -
>> -static int atmel_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>> -{
>> -	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
>> -	int ret;
>> -
>> -	ret = clk_enable(atmel_pwm->clk);
>> -	if (ret) {
>> -		dev_err(chip->dev, "failed to enable PWM clock\n");
>> -		return ret;
>> -	}
>> -
>> -	atmel_pwm_writel(atmel_pwm, PWM_ENA, 1 << pwm->hwpwm);
>> -
>> -	return 0;
>> -}
>> -
>>  static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>>  {
>>  	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
>> @@ -285,17 +263,58 @@ static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>>  	clk_disable(atmel_pwm->clk);
>>  }
>>  
>> +static int atmel_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>> +			   struct pwm_state *state)
>> +{
>> +	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
>> +	struct pwm_state cstate;
>> +	unsigned long prd, dty;
>> +	unsigned int pres;
>> +	bool enabled = true;
>> +	int ret;
>> +
>> +	pwm_get_state(pwm, &cstate);
>> +
>> +	if (state->enabled) {
>> +		ret = atmel_pwm_config_prepare(chip, state, &prd, &dty,
>> +					       &pres);
>> +		if (ret) {
>> +			dev_err(chip->dev, "failed to prepare config\n");
>> +			return ret;
>> +		}
>> +
>> +		if (cstate.polarity != state->polarity) {
> Hm, you seem to unconditionally disable the PWM. What if it was already
> disabled? The atmel_pwm->clk refcounting is likely to be wrong after
> that.
I agree. I should take care of the current PWM state before disabling it.
>  
> Moreover, if you follow my previous suggestions, you should have
>
> 		if (cstate.enabled &&
> 		    (cstate.polarity != state->polarity ||
> 		     cstate.period != state->period))
>
>> +			atmel_pwm_disable(chip, pwm);
>> +			enabled = false;
> Just set cstate.enabled to false here, no need for an extra variable.
I agree with you.
>
>> +		}
>> +
>> +		if (!cstate.enabled || !enabled) {
> 		if (!cstate.enabled) {
>
>> +			ret = clk_enable(atmel_pwm->clk);
>> +			if (ret) {
>> +				dev_err(chip->dev, "failed to enable clock\n");
>> +				return ret;
>> +			}
>> +		}
>> +
>> +		atmel_pwm_config_set(chip, pwm, dty, prd, pres,
>> +				     state->polarity, enabled);
>> +		if (!cstate.enabled || !enabled)
>> +			atmel_pwm_writel(atmel_pwm, PWM_ENA, 1 << pwm->hwpwm);
> I would differentiate the update CDTY only and set CDTY+CPRD+CMR cases
> here. Something like:
>
> 		if (cstate.enabled) {
> 			/*
> 			 * 1/ read CPRD
> 			 * 2/ call atmel_pwm_calculate_cdty()
> 			 * 3/ call ->update_cdty() hook
> 			 */
> 		} else {
> 			/*
> 			 * 1/ enable the clk
> 			 * 2/ read CPRD
> 			 * 3/ call atmel_pwm_calculate_cprd_and_pres()
> 			 * 4/ call atmel_pwm_calculate_cdty()
> 			 * 3/ call ->set_cprd_cdty() hook
> 			 * 4/ write CMR (PRES + polarity)
> 			 * 5/ start the PWM (PWM_ENA)
> 			 */
> 		}
>
>> +	} else if (cstate.enabled) {
>> +		atmel_pwm_disable(chip, pwm);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static const struct pwm_ops atmel_pwm_ops = {
>> -	.config = atmel_pwm_config,
>> -	.set_polarity = atmel_pwm_set_polarity,
>> -	.enable = atmel_pwm_enable,
>> -	.disable = atmel_pwm_disable,
>> +	.apply = atmel_pwm_apply,
>>  	.owner = THIS_MODULE,
>>  };
>>  
>>  struct atmel_pwm_data {
>>  	void (*config)(struct pwm_chip *chip, struct pwm_device *pwm,
>> -		       unsigned long dty, unsigned long prd);
>> +		       unsigned long dty, unsigned long prd, bool enabled);
>>  };
>>  
>>  static const struct atmel_pwm_data atmel_pwm_data_v1 = {

Powered by blists - more mailing lists