[PATCH 0/2] switch to atomic PWM

[PATCH 0/2] switch to atomic PWM

[Claudiu Beznea]

Switch atmel-pwm driver to atomic PWM and
enable it for sama5d2

Claudiu Beznea (2):
  drivers: pwm: pwm-atmel: switch to atomic PWM
  drivers: pwm: pwm-atmel: enable pwm on sama5d2

 .../devicetree/bindings/pwm/atmel-pwm.txt          |   1 +
 drivers/pwm/pwm-atmel.c                            | 220 +++++++++++----------
 2 files changed, 122 insertions(+), 99 deletions(-)

-- 
2.7.4

[PATCH 1/2] drivers: pwm: pwm-atmel: switch to atomic PWM

[Claudiu Beznea]

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

Signed-off-by: Claudiu Beznea <claudiu.beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
---
 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)
 {
 	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;
 
-	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;
 }
 
 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);
+	} 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);
+	}
 }
 
 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);
 	} 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) {
+			atmel_pwm_disable(chip, pwm);
+			enabled = false;
+		}
+
+		if (!cstate.enabled || !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);
+	} 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 = {
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] drivers: pwm: pwm-atmel: switch to atomic PWM

[Boris Brezillon]

Hi Claudiu,

On Tue, 28 Feb 2017 12:40:14 +0200
Claudiu Beznea <claudiu.beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org> 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.

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.

> 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-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
> ---
>  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:

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

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

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

> +	} 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);

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

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

> +		}
> +
> +		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 = {

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] drivers: pwm: pwm-atmel: switch to atomic PWM

[m18063]

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@microchip.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@microchip.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 = {

Re: [PATCH 1/2] drivers: pwm: pwm-atmel: switch to atomic PWM

[Boris Brezillon]

Hi Claudiu,

On Wed, 1 Mar 2017 15:56:26 +0200
m18063 <Claudiu.Beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org> wrote:

> 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-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org> 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.

Unlikely, but not impossible.

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

No, I meant that, if you update the period then the duty (or the other
way around), one update might make it in one period-cycle, and the
other one might be delayed until the next period. In this case you
might end up with inconsistent values and possibly duty > period.

> >  
> >> 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-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
> >> ---
> >>  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.

Well, I think this is preferable to have a one-time rework than keeping
things for historical reasons.


[...]

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

Again, if it simplifies the whole logic, I think it's preferable to do
it now, since we're anyway refactoring the driver for the atomic
transition.

[...]

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

Yep, that's what I understood. So let's stick to "update duty only" for
now.

[...]

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

Ditto, let's stick to the current approach.

Regards,

Boris
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] drivers: pwm: pwm-atmel: switch to atomic PWM

[m18063]

Hi Boris,


On 01.03.2017 16:13, Boris Brezillon wrote:
> Hi Claudiu,
>
> On Wed, 1 Mar 2017 15:56:26 +0200
> m18063 <Claudiu.Beznea@microchip.com> wrote:
>
>> 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@microchip.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.
> Unlikely, but not impossible.
>
>>> 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.
> No, I meant that, if you update the period then the duty (or the other
> way around), one update might make it in one period-cycle, and the
> other one might be delayed until the next period. In this case you
> might end up with inconsistent values and possibly duty > period.
I see what you're saying. Yes, you're right.
>
>>>  
>>>> 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@microchip.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.
> Well, I think this is preferable to have a one-time rework than keeping
> things for historical reasons.
I will change it in v2.
>
>
> [...]
>
>>>  
>>>>  {
>>>>  	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.
> Again, if it simplifies the whole logic, I think it's preferable to do
> it now, since we're anyway refactoring the driver for the atomic
> transition.
I'll do it in v2.
>
> [...]
>
>>>>  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.
> Yep, that's what I understood. So let's stick to "update duty only" for
> now.
Ok.
>
> [...]
>
>>>>  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.
> Ditto, let's stick to the current approach.
>
> Regards,
>
> Boris

[PATCH 2/2] drivers: pwm: pwm-atmel: enable pwm on sama5d2

[Claudiu Beznea]

sama5d2 can use the same atmel_pwm_data as sama5d3.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 Documentation/devicetree/bindings/pwm/atmel-pwm.txt | 1 +
 drivers/pwm/pwm-atmel.c                             | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/pwm/atmel-pwm.txt b/Documentation/devicetree/bindings/pwm/atmel-pwm.txt
index 02331b9..c8c831d 100644
--- a/Documentation/devicetree/bindings/pwm/atmel-pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/atmel-pwm.txt
@@ -4,6 +4,7 @@ Required properties:
   - compatible: should be one of:
     - "atmel,at91sam9rl-pwm"
     - "atmel,sama5d3-pwm"
+    - "atmel,sama5d2-pwm"
   - reg: physical base address and length of the controller's registers
   - #pwm-cells: Should be 3. See pwm.txt in this directory for a
     description of the cells format.
diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index 014b86c..209e877 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -346,6 +346,9 @@ static const struct of_device_id atmel_pwm_dt_ids[] = {
 		.compatible = "atmel,sama5d3-pwm",
 		.data = &atmel_pwm_data_v2,
 	}, {
+		.compatible = "atmel,sama5d2-pwm",
+		.data = &atmel_pwm_data_v2,
+	}, {
 		/* sentinel */
 	},
 };
-- 
2.7.4

Re: [PATCH 2/2] drivers: pwm: pwm-atmel: enable pwm on sama5d2

[Rob Herring]

On Tue, Feb 28, 2017 at 12:40:15PM +0200, Claudiu Beznea wrote:
> sama5d2 can use the same atmel_pwm_data as sama5d3.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  Documentation/devicetree/bindings/pwm/atmel-pwm.txt | 1 +
>  drivers/pwm/pwm-atmel.c                             | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/atmel-pwm.txt b/Documentation/devicetree/bindings/pwm/atmel-pwm.txt
> index 02331b9..c8c831d 100644
> --- a/Documentation/devicetree/bindings/pwm/atmel-pwm.txt
> +++ b/Documentation/devicetree/bindings/pwm/atmel-pwm.txt
> @@ -4,6 +4,7 @@ Required properties:
>    - compatible: should be one of:
>      - "atmel,at91sam9rl-pwm"
>      - "atmel,sama5d3-pwm"
> +    - "atmel,sama5d2-pwm"

Perhaps sama5d3 should be a fallback, then you don't need a driver 
change.

Rob

Re: [PATCH 2/2] drivers: pwm: pwm-atmel: enable pwm on sama5d2

[m18063]

Hi,


On 03.03.2017 08:21, Rob Herring wrote:
> On Tue, Feb 28, 2017 at 12:40:15PM +0200, Claudiu Beznea wrote:
>> sama5d2 can use the same atmel_pwm_data as sama5d3.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>>  Documentation/devicetree/bindings/pwm/atmel-pwm.txt | 1 +
>>  drivers/pwm/pwm-atmel.c                             | 3 +++
>>  2 files changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/atmel-pwm.txt b/Documentation/devicetree/bindings/pwm/atmel-pwm.txt
>> index 02331b9..c8c831d 100644
>> --- a/Documentation/devicetree/bindings/pwm/atmel-pwm.txt
>> +++ b/Documentation/devicetree/bindings/pwm/atmel-pwm.txt
>> @@ -4,6 +4,7 @@ Required properties:
>>    - compatible: should be one of:
>>      - "atmel,at91sam9rl-pwm"
>>      - "atmel,sama5d3-pwm"
>> +    - "atmel,sama5d2-pwm"
> Perhaps sama5d3 should be a fallback, then you don't need a driver 
> change.
>
> Rob

sama5d2 PWM controller have additional features. We intend to support
some of these. This is why I choose to add specific compatible string
for it.

Thank you,
Claudiu Beznea