[PATCH 1/2] pwm: atmel: Fix duty cycle calculation in .get_state()

[PATCH 1/2] pwm: atmel: Fix duty cycle calculation in .get_state()

[Uwe Kleine-König]

The CDTY register contains the number of inactive cycles. .apply() does
this correctly, however .get_state() got this wrong.

Fixes: 651b510a74d4 ("pwm: atmel: Implement .get_state()")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-atmel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index d49da708337f..ebaeb50dcfde 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -319,7 +319,7 @@ static void atmel_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 
 		cdty = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm,
 					  atmel_pwm->data->regs.duty);
-		tmp = (u64)cdty * NSEC_PER_SEC;
+		tmp = (u64)(cprd - cdty) * NSEC_PER_SEC;
 		tmp <<= pres;
 		state->duty_cycle = DIV64_U64_ROUND_UP(tmp, rate);
 
-- 
2.30.2

[PATCH 1/2] pwm: atmel: Fix duty cycle calculation in .get_state()

[Uwe Kleine-König]

The CDTY register contains the number of inactive cycles. .apply() does
this correctly, however .get_state() got this wrong.

Fixes: 651b510a74d4 ("pwm: atmel: Implement .get_state()")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-atmel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index d49da708337f..ebaeb50dcfde 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -319,7 +319,7 @@ static void atmel_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 
 		cdty = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm,
 					  atmel_pwm->data->regs.duty);
-		tmp = (u64)cdty * NSEC_PER_SEC;
+		tmp = (u64)(cprd - cdty) * NSEC_PER_SEC;
 		tmp <<= pres;
 		state->duty_cycle = DIV64_U64_ROUND_UP(tmp, rate);
 
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] pwm: atmel: Improve duty cycle calculation in .apply()

[Uwe Kleine-König]

In the calculation of the register value determining the duty cycle the
requested period is used instead of the actually implemented period which
results in suboptimal settings.

The following example assumes an input clock of 133333333 Hz on one of
the SoCs with 16 bit period.

When the following state is to be applied:

        .period = 414727681
        .duty_cycle = 652806

the following register values used to be  calculated:

        PRES = 10
        CPRD = 54000
        CDTY = 53916

which yields an actual duty cycle of a bit more than 645120 ns.

The setting

        PRES = 10
        CPRD = 54000
        CDTY = 53915

however yields a duty of 652800 ns which is between the current result
and the requested value and so is a better approximation.

The reason for this error is that for the calculation of CDTY the
requested period was used instead of the actually implemented one.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-atmel.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index ebaeb50dcfde..29b5ad03f715 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -124,6 +124,7 @@ static inline void atmel_pwm_ch_writel(struct atmel_pwm_chip *chip,
 }
 
 static int atmel_pwm_calculate_cprd_and_pres(struct pwm_chip *chip,
+					     unsigned long clkrate,
 					     const struct pwm_state *state,
 					     unsigned long *cprd, u32 *pres)
 {
@@ -132,7 +133,7 @@ static int atmel_pwm_calculate_cprd_and_pres(struct pwm_chip *chip,
 	int shift;
 
 	/* Calculate the period cycles and prescale value */
-	cycles *= clk_get_rate(atmel_pwm->clk);
+	cycles *= clkrate;
 	do_div(cycles, NSEC_PER_SEC);
 
 	/*
@@ -158,12 +159,14 @@ static int atmel_pwm_calculate_cprd_and_pres(struct pwm_chip *chip,
 }
 
 static void atmel_pwm_calculate_cdty(const struct pwm_state *state,
-				     unsigned long cprd, unsigned long *cdty)
+				     unsigned long clkrate, unsigned long cprd,
+				     u32 pres, unsigned long *cdty)
 {
 	unsigned long long cycles = state->duty_cycle;
 
-	cycles *= cprd;
-	do_div(cycles, state->period);
+	cycles *= clkrate;
+	do_div(cycles, NSEC_PER_SEC);
+	cycles >>= pres;
 	*cdty = cprd - cycles;
 }
 
@@ -244,17 +247,23 @@ static int atmel_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	pwm_get_state(pwm, &cstate);
 
 	if (state->enabled) {
+		unsigned long clkrate = clk_get_rate(atmel_pwm->clk);
+
 		if (cstate.enabled &&
 		    cstate.polarity == state->polarity &&
 		    cstate.period == state->period) {
+			u32 cmr = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
+
 			cprd = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm,
 						  atmel_pwm->data->regs.period);
-			atmel_pwm_calculate_cdty(state, cprd, &cdty);
+			pres = cmr & PWM_CMR_CPRE_MSK;
+
+			atmel_pwm_calculate_cdty(state, clkrate, cprd, pres, &cdty);
 			atmel_pwm_update_cdty(chip, pwm, cdty);
 			return 0;
 		}
 
-		ret = atmel_pwm_calculate_cprd_and_pres(chip, state, &cprd,
+		ret = atmel_pwm_calculate_cprd_and_pres(chip, clkrate, state, &cprd,
 							&pres);
 		if (ret) {
 			dev_err(chip->dev,
@@ -262,7 +271,7 @@ static int atmel_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			return ret;
 		}
 
-		atmel_pwm_calculate_cdty(state, cprd, &cdty);
+		atmel_pwm_calculate_cdty(state, clkrate, cprd, pres, &cdty);
 
 		if (cstate.enabled) {
 			atmel_pwm_disable(chip, pwm, false);
-- 
2.30.2

[PATCH 2/2] pwm: atmel: Improve duty cycle calculation in .apply()

[Uwe Kleine-König]

In the calculation of the register value determining the duty cycle the
requested period is used instead of the actually implemented period which
results in suboptimal settings.

The following example assumes an input clock of 133333333 Hz on one of
the SoCs with 16 bit period.

When the following state is to be applied:

        .period = 414727681
        .duty_cycle = 652806

the following register values used to be  calculated:

        PRES = 10
        CPRD = 54000
        CDTY = 53916

which yields an actual duty cycle of a bit more than 645120 ns.

The setting

        PRES = 10
        CPRD = 54000
        CDTY = 53915

however yields a duty of 652800 ns which is between the current result
and the requested value and so is a better approximation.

The reason for this error is that for the calculation of CDTY the
requested period was used instead of the actually implemented one.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-atmel.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index ebaeb50dcfde..29b5ad03f715 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -124,6 +124,7 @@ static inline void atmel_pwm_ch_writel(struct atmel_pwm_chip *chip,
 }
 
 static int atmel_pwm_calculate_cprd_and_pres(struct pwm_chip *chip,
+					     unsigned long clkrate,
 					     const struct pwm_state *state,
 					     unsigned long *cprd, u32 *pres)
 {
@@ -132,7 +133,7 @@ static int atmel_pwm_calculate_cprd_and_pres(struct pwm_chip *chip,
 	int shift;
 
 	/* Calculate the period cycles and prescale value */
-	cycles *= clk_get_rate(atmel_pwm->clk);
+	cycles *= clkrate;
 	do_div(cycles, NSEC_PER_SEC);
 
 	/*
@@ -158,12 +159,14 @@ static int atmel_pwm_calculate_cprd_and_pres(struct pwm_chip *chip,
 }
 
 static void atmel_pwm_calculate_cdty(const struct pwm_state *state,
-				     unsigned long cprd, unsigned long *cdty)
+				     unsigned long clkrate, unsigned long cprd,
+				     u32 pres, unsigned long *cdty)
 {
 	unsigned long long cycles = state->duty_cycle;
 
-	cycles *= cprd;
-	do_div(cycles, state->period);
+	cycles *= clkrate;
+	do_div(cycles, NSEC_PER_SEC);
+	cycles >>= pres;
 	*cdty = cprd - cycles;
 }
 
@@ -244,17 +247,23 @@ static int atmel_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	pwm_get_state(pwm, &cstate);
 
 	if (state->enabled) {
+		unsigned long clkrate = clk_get_rate(atmel_pwm->clk);
+
 		if (cstate.enabled &&
 		    cstate.polarity == state->polarity &&
 		    cstate.period == state->period) {
+			u32 cmr = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
+
 			cprd = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm,
 						  atmel_pwm->data->regs.period);
-			atmel_pwm_calculate_cdty(state, cprd, &cdty);
+			pres = cmr & PWM_CMR_CPRE_MSK;
+
+			atmel_pwm_calculate_cdty(state, clkrate, cprd, pres, &cdty);
 			atmel_pwm_update_cdty(chip, pwm, cdty);
 			return 0;
 		}
 
-		ret = atmel_pwm_calculate_cprd_and_pres(chip, state, &cprd,
+		ret = atmel_pwm_calculate_cprd_and_pres(chip, clkrate, state, &cprd,
 							&pres);
 		if (ret) {
 			dev_err(chip->dev,
@@ -262,7 +271,7 @@ static int atmel_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			return ret;
 		}
 
-		atmel_pwm_calculate_cdty(state, cprd, &cdty);
+		atmel_pwm_calculate_cdty(state, clkrate, cprd, pres, &cdty);
 
 		if (cstate.enabled) {
 			atmel_pwm_disable(chip, pwm, false);
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] pwm: atmel: Improve duty cycle calculation in .apply()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1282 bytes --]

On Tue, Apr 20, 2021 at 11:51:18AM +0200, Uwe Kleine-König wrote:
> In the calculation of the register value determining the duty cycle the
> requested period is used instead of the actually implemented period which
> results in suboptimal settings.
> 
> The following example assumes an input clock of 133333333 Hz on one of
> the SoCs with 16 bit period.
> 
> When the following state is to be applied:
> 
>         .period = 414727681
>         .duty_cycle = 652806
> 
> the following register values used to be  calculated:
> 
>         PRES = 10
>         CPRD = 54000
>         CDTY = 53916
> 
> which yields an actual duty cycle of a bit more than 645120 ns.
> 
> The setting
> 
>         PRES = 10
>         CPRD = 54000
>         CDTY = 53915
> 
> however yields a duty of 652800 ns which is between the current result
> and the requested value and so is a better approximation.
> 
> The reason for this error is that for the calculation of CDTY the
> requested period was used instead of the actually implemented one.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/pwm-atmel.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)

Applied, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] pwm: atmel: Improve duty cycle calculation in .apply()

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1282 bytes --]

On Tue, Apr 20, 2021 at 11:51:18AM +0200, Uwe Kleine-König wrote:
> In the calculation of the register value determining the duty cycle the
> requested period is used instead of the actually implemented period which
> results in suboptimal settings.
> 
> The following example assumes an input clock of 133333333 Hz on one of
> the SoCs with 16 bit period.
> 
> When the following state is to be applied:
> 
>         .period = 414727681
>         .duty_cycle = 652806
> 
> the following register values used to be  calculated:
> 
>         PRES = 10
>         CPRD = 54000
>         CDTY = 53916
> 
> which yields an actual duty cycle of a bit more than 645120 ns.
> 
> The setting
> 
>         PRES = 10
>         CPRD = 54000
>         CDTY = 53915
> 
> however yields a duty of 652800 ns which is between the current result
> and the requested value and so is a better approximation.
> 
> The reason for this error is that for the calculation of CDTY the
> requested period was used instead of the actually implemented one.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/pwm-atmel.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)

Applied, thanks.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] pwm: atmel: rework tracking updates pending in hardware

[Uwe Kleine-König]

This improves the driver's behavior in several ways:

 - The lock is held for shorter periods and so a channel that is currently
   waited for doesn't block disabling another channel.

 - It's easier to understand because the procedure is split into more
   semantic units and documentation is improved

 - A channel is only set to pending when such an event is actually
   scheduled in hardware (by writing the CUPD register).

 - Also wait in .get_state() to report the last configured state instead
   of (maybe) the previous one. This fixes the read back duty cycle and so
   prevents a warning being emitted when PWM_DEBUG is on.

Tested on an AriettaG25.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

I tested this patch on top of the series I sent for this driver
yesterday. (I set In-Reply-To accordingly.)

With these three patches PWM_DEBUG is now happy. (At least I couldn't
trigger a warning any more. I think there are still a few problems with
integer overflows.)

Best regards
Uwe

 drivers/pwm/pwm-atmel.c | 101 +++++++++++++++++++++++++++++++---------
 1 file changed, 78 insertions(+), 23 deletions(-)

diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index 29b5ad03f715..38d86340201c 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -84,9 +84,19 @@ struct atmel_pwm_chip {
 	void __iomem *base;
 	const struct atmel_pwm_data *data;
 
-	unsigned int updated_pwms;
-	/* ISR is cleared when read, ensure only one thread does that */
-	struct mutex isr_lock;
+	/*
+	 * The hardware supports a mechanism to update a channel's duty cycle at
+	 * the end of the currently running period. When such an update is
+	 * pending we delay disabling the PWM until the new configuration is
+	 * active because otherwise pmw_config(duty_cycle=0); pwm_disable();
+	 * might not result in an inactive output.
+	 * This bitmask tracks for which channels an update is pending in
+	 * hardware.
+	 */
+	u32 update_pending;
+
+	/* Protects .update_pending */
+	spinlock_t lock;
 };
 
 static inline struct atmel_pwm_chip *to_atmel_pwm_chip(struct pwm_chip *chip)
@@ -123,6 +133,63 @@ static inline void atmel_pwm_ch_writel(struct atmel_pwm_chip *chip,
 	atmel_pwm_writel(chip, base + offset, val);
 }
 
+static void atmel_pwm_update_pending(struct atmel_pwm_chip *chip)
+{
+	/*
+	 * Each channel that has its bit in ISR set started a new period since
+	 * ISR was cleared and so there is no more update pending.  Note that
+	 * reading ISR clears it, so this needs to handle all channels to not
+	 * loose information.
+	 */
+	u32 isr = atmel_pwm_readl(chip, PWM_ISR);
+	chip->update_pending &= ~isr;
+}
+
+static void atmel_pwm_set_pending(struct atmel_pwm_chip *chip, unsigned int ch)
+{
+	spin_lock(&chip->lock);
+
+	/*
+	 * Clear pending flags in hardware because otherwise there might still
+	 * be a stale flag in ISR.
+	 */
+	atmel_pwm_update_pending(chip);
+
+	chip->update_pending |= (1 << ch);
+
+	spin_unlock(&chip->lock);
+}
+
+static int atmel_pwm_test_pending(struct atmel_pwm_chip *chip, unsigned int ch)
+{
+	int ret = 0;
+
+	spin_lock(&chip->lock);
+
+	if (chip->update_pending & (1 << ch)) {
+		atmel_pwm_update_pending(chip);
+
+		if (chip->update_pending & (1 << ch))
+			ret = 1;
+	}
+
+	spin_unlock(&chip->lock);
+
+	return ret;
+}
+
+static int atmel_pwm_wait_nonpending(struct atmel_pwm_chip *chip, unsigned int ch)
+{
+	unsigned long timeout = jiffies + 2 * HZ;
+	int ret;
+
+	while ((ret = atmel_pwm_test_pending(chip, ch)) &&
+	       time_before(jiffies, timeout))
+		usleep_range(10, 100);
+
+	return ret ? -ETIMEDOUT : 0;
+}
+
 static int atmel_pwm_calculate_cprd_and_pres(struct pwm_chip *chip,
 					     unsigned long clkrate,
 					     const struct pwm_state *state,
@@ -185,6 +252,7 @@ static void atmel_pwm_update_cdty(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm,
 			    atmel_pwm->data->regs.duty_upd, cdty);
+	atmel_pwm_set_pending(atmel_pwm, pwm->hwpwm);
 }
 
 static void atmel_pwm_set_cprd_cdty(struct pwm_chip *chip,
@@ -205,20 +273,8 @@ static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
 	unsigned long timeout = jiffies + 2 * HZ;
 
-	/*
-	 * Wait for at least a complete period to have passed before disabling a
-	 * channel to be sure that CDTY has been updated
-	 */
-	mutex_lock(&atmel_pwm->isr_lock);
-	atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR);
-
-	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);
-	}
+	atmel_pwm_wait_nonpending(atmel_pwm, pwm->hwpwm);
 
-	mutex_unlock(&atmel_pwm->isr_lock);
 	atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm);
 
 	/*
@@ -292,10 +348,6 @@ static int atmel_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			val |= PWM_CMR_CPOL;
 		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
 		atmel_pwm_set_cprd_cdty(chip, pwm, cprd, cdty);
-		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);
 		atmel_pwm_writel(atmel_pwm, PWM_ENA, 1 << pwm->hwpwm);
 	} else if (cstate.enabled) {
 		atmel_pwm_disable(chip, pwm, true);
@@ -326,6 +378,9 @@ static void atmel_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 		tmp <<= pres;
 		state->period = DIV64_U64_ROUND_UP(tmp, rate);
 
+		/* Wait for an updated duty_cycle queued in hardware */
+		atmel_pwm_wait_nonpending(atmel_pwm, pwm->hwpwm);
+
 		cdty = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm,
 					  atmel_pwm->data->regs.duty);
 		tmp = (u64)(cprd - cdty) * NSEC_PER_SEC;
@@ -416,9 +471,10 @@ static int atmel_pwm_probe(struct platform_device *pdev)
 	if (!atmel_pwm)
 		return -ENOMEM;
 
-	mutex_init(&atmel_pwm->isr_lock);
 	atmel_pwm->data = of_device_get_match_data(&pdev->dev);
-	atmel_pwm->updated_pwms = 0;
+
+	atmel_pwm->update_pending = 0;
+	spin_lock_init(&atmel_pwm->lock);
 
 	atmel_pwm->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(atmel_pwm->base))
@@ -462,7 +518,6 @@ static int atmel_pwm_remove(struct platform_device *pdev)
 	pwmchip_remove(&atmel_pwm->chip);
 
 	clk_unprepare(atmel_pwm->clk);
-	mutex_destroy(&atmel_pwm->isr_lock);
 
 	return 0;
 }
-- 
2.30.2

[PATCH] pwm: atmel: rework tracking updates pending in hardware

[Uwe Kleine-König]

This improves the driver's behavior in several ways:

 - The lock is held for shorter periods and so a channel that is currently
   waited for doesn't block disabling another channel.

 - It's easier to understand because the procedure is split into more
   semantic units and documentation is improved

 - A channel is only set to pending when such an event is actually
   scheduled in hardware (by writing the CUPD register).

 - Also wait in .get_state() to report the last configured state instead
   of (maybe) the previous one. This fixes the read back duty cycle and so
   prevents a warning being emitted when PWM_DEBUG is on.

Tested on an AriettaG25.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

I tested this patch on top of the series I sent for this driver
yesterday. (I set In-Reply-To accordingly.)

With these three patches PWM_DEBUG is now happy. (At least I couldn't
trigger a warning any more. I think there are still a few problems with
integer overflows.)

Best regards
Uwe

 drivers/pwm/pwm-atmel.c | 101 +++++++++++++++++++++++++++++++---------
 1 file changed, 78 insertions(+), 23 deletions(-)

diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index 29b5ad03f715..38d86340201c 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -84,9 +84,19 @@ struct atmel_pwm_chip {
 	void __iomem *base;
 	const struct atmel_pwm_data *data;
 
-	unsigned int updated_pwms;
-	/* ISR is cleared when read, ensure only one thread does that */
-	struct mutex isr_lock;
+	/*
+	 * The hardware supports a mechanism to update a channel's duty cycle at
+	 * the end of the currently running period. When such an update is
+	 * pending we delay disabling the PWM until the new configuration is
+	 * active because otherwise pmw_config(duty_cycle=0); pwm_disable();
+	 * might not result in an inactive output.
+	 * This bitmask tracks for which channels an update is pending in
+	 * hardware.
+	 */
+	u32 update_pending;
+
+	/* Protects .update_pending */
+	spinlock_t lock;
 };
 
 static inline struct atmel_pwm_chip *to_atmel_pwm_chip(struct pwm_chip *chip)
@@ -123,6 +133,63 @@ static inline void atmel_pwm_ch_writel(struct atmel_pwm_chip *chip,
 	atmel_pwm_writel(chip, base + offset, val);
 }
 
+static void atmel_pwm_update_pending(struct atmel_pwm_chip *chip)
+{
+	/*
+	 * Each channel that has its bit in ISR set started a new period since
+	 * ISR was cleared and so there is no more update pending.  Note that
+	 * reading ISR clears it, so this needs to handle all channels to not
+	 * loose information.
+	 */
+	u32 isr = atmel_pwm_readl(chip, PWM_ISR);
+	chip->update_pending &= ~isr;
+}
+
+static void atmel_pwm_set_pending(struct atmel_pwm_chip *chip, unsigned int ch)
+{
+	spin_lock(&chip->lock);
+
+	/*
+	 * Clear pending flags in hardware because otherwise there might still
+	 * be a stale flag in ISR.
+	 */
+	atmel_pwm_update_pending(chip);
+
+	chip->update_pending |= (1 << ch);
+
+	spin_unlock(&chip->lock);
+}
+
+static int atmel_pwm_test_pending(struct atmel_pwm_chip *chip, unsigned int ch)
+{
+	int ret = 0;
+
+	spin_lock(&chip->lock);
+
+	if (chip->update_pending & (1 << ch)) {
+		atmel_pwm_update_pending(chip);
+
+		if (chip->update_pending & (1 << ch))
+			ret = 1;
+	}
+
+	spin_unlock(&chip->lock);
+
+	return ret;
+}
+
+static int atmel_pwm_wait_nonpending(struct atmel_pwm_chip *chip, unsigned int ch)
+{
+	unsigned long timeout = jiffies + 2 * HZ;
+	int ret;
+
+	while ((ret = atmel_pwm_test_pending(chip, ch)) &&
+	       time_before(jiffies, timeout))
+		usleep_range(10, 100);
+
+	return ret ? -ETIMEDOUT : 0;
+}
+
 static int atmel_pwm_calculate_cprd_and_pres(struct pwm_chip *chip,
 					     unsigned long clkrate,
 					     const struct pwm_state *state,
@@ -185,6 +252,7 @@ static void atmel_pwm_update_cdty(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm,
 			    atmel_pwm->data->regs.duty_upd, cdty);
+	atmel_pwm_set_pending(atmel_pwm, pwm->hwpwm);
 }
 
 static void atmel_pwm_set_cprd_cdty(struct pwm_chip *chip,
@@ -205,20 +273,8 @@ static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
 	unsigned long timeout = jiffies + 2 * HZ;
 
-	/*
-	 * Wait for at least a complete period to have passed before disabling a
-	 * channel to be sure that CDTY has been updated
-	 */
-	mutex_lock(&atmel_pwm->isr_lock);
-	atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR);
-
-	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);
-	}
+	atmel_pwm_wait_nonpending(atmel_pwm, pwm->hwpwm);
 
-	mutex_unlock(&atmel_pwm->isr_lock);
 	atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm);
 
 	/*
@@ -292,10 +348,6 @@ static int atmel_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			val |= PWM_CMR_CPOL;
 		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
 		atmel_pwm_set_cprd_cdty(chip, pwm, cprd, cdty);
-		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);
 		atmel_pwm_writel(atmel_pwm, PWM_ENA, 1 << pwm->hwpwm);
 	} else if (cstate.enabled) {
 		atmel_pwm_disable(chip, pwm, true);
@@ -326,6 +378,9 @@ static void atmel_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 		tmp <<= pres;
 		state->period = DIV64_U64_ROUND_UP(tmp, rate);
 
+		/* Wait for an updated duty_cycle queued in hardware */
+		atmel_pwm_wait_nonpending(atmel_pwm, pwm->hwpwm);
+
 		cdty = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm,
 					  atmel_pwm->data->regs.duty);
 		tmp = (u64)(cprd - cdty) * NSEC_PER_SEC;
@@ -416,9 +471,10 @@ static int atmel_pwm_probe(struct platform_device *pdev)
 	if (!atmel_pwm)
 		return -ENOMEM;
 
-	mutex_init(&atmel_pwm->isr_lock);
 	atmel_pwm->data = of_device_get_match_data(&pdev->dev);
-	atmel_pwm->updated_pwms = 0;
+
+	atmel_pwm->update_pending = 0;
+	spin_lock_init(&atmel_pwm->lock);
 
 	atmel_pwm->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(atmel_pwm->base))
@@ -462,7 +518,6 @@ static int atmel_pwm_remove(struct platform_device *pdev)
 	pwmchip_remove(&atmel_pwm->chip);
 
 	clk_unprepare(atmel_pwm->clk);
-	mutex_destroy(&atmel_pwm->isr_lock);
 
 	return 0;
 }
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

overflow and wrong timeout errors in pwm-atmel

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 924 bytes --]

On Wed, Apr 21, 2021 at 11:26:08AM +0200, Uwe Kleine-König wrote:
> With these three patches PWM_DEBUG is now happy. (At least I couldn't
> trigger a warning any more. I think there are still a few problems with
> integer overflows.)

BTW, setting the period to 138350580899 (with a clock rate of 133333333
Hz) results in setting period=0 because

	state->period * clkrate =
	138350580899 * 133333333 =
	40254751 (discarded from 18446744073749806367).

Another problem I just noticed is: The driver configures a period of 4s
just fine (i.e. without an overflow). But waiting for a new period to
start (e.g. in atmel_pwm_disable()) aborts after 2s.

So in the area of big period states there are still a few things to
improve.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: overflow and wrong timeout errors in pwm-atmel

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1628 bytes --]

On Wed, Apr 21, 2021 at 01:03:36PM +0200, Uwe Kleine-König wrote:
> On Wed, Apr 21, 2021 at 11:26:08AM +0200, Uwe Kleine-König wrote:
> > With these three patches PWM_DEBUG is now happy. (At least I couldn't
> > trigger a warning any more. I think there are still a few problems with
> > integer overflows.)
> 
> BTW, setting the period to 138350580899 (with a clock rate of 133333333
> Hz) results in setting period=0 because
> 
> 	state->period * clkrate =
> 	138350580899 * 133333333 =
> 	40254751 (discarded from 18446744073749806367).

As a first remedy the following could be done:

diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index 38d86340201c..02d69fa5f7d2 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -199,6 +199,11 @@ static int atmel_pwm_calculate_cprd_and_pres(struct pwm_chip *chip,
 	unsigned long long cycles = state->period;
 	int shift;
 
+	if (fls(cycles) + fls(clkrate) > 64) {
+		dev_err(chip->dev, "period to big to calculate HW parameters\n");
+		return -EINVAL;
+	}
+
 	/* Calculate the period cycles and prescale value */
 	cycles *= clkrate;
 	do_div(cycles, NSEC_PER_SEC);

Is this sensible? (Actually I'd prefer to just continue with

	period = (ULL(1) << (64 - fls(clkrate))) - 1

according to the motto to yield the highest possible period, but this
function has another error path that returns -EINVAL so this would be
inconsistent.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: overflow and wrong timeout errors in pwm-atmel

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1628 bytes --]

On Wed, Apr 21, 2021 at 01:03:36PM +0200, Uwe Kleine-König wrote:
> On Wed, Apr 21, 2021 at 11:26:08AM +0200, Uwe Kleine-König wrote:
> > With these three patches PWM_DEBUG is now happy. (At least I couldn't
> > trigger a warning any more. I think there are still a few problems with
> > integer overflows.)
> 
> BTW, setting the period to 138350580899 (with a clock rate of 133333333
> Hz) results in setting period=0 because
> 
> 	state->period * clkrate =
> 	138350580899 * 133333333 =
> 	40254751 (discarded from 18446744073749806367).

As a first remedy the following could be done:

diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index 38d86340201c..02d69fa5f7d2 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -199,6 +199,11 @@ static int atmel_pwm_calculate_cprd_and_pres(struct pwm_chip *chip,
 	unsigned long long cycles = state->period;
 	int shift;
 
+	if (fls(cycles) + fls(clkrate) > 64) {
+		dev_err(chip->dev, "period to big to calculate HW parameters\n");
+		return -EINVAL;
+	}
+
 	/* Calculate the period cycles and prescale value */
 	cycles *= clkrate;
 	do_div(cycles, NSEC_PER_SEC);

Is this sensible? (Actually I'd prefer to just continue with

	period = (ULL(1) << (64 - fls(clkrate))) - 1

according to the motto to yield the highest possible period, but this
function has another error path that returns -EINVAL so this would be
inconsistent.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: overflow and wrong timeout errors in pwm-atmel

[Alexandre Belloni]

On 21/04/2021 15:48:25+0200, Uwe Kleine-König wrote:
> On Wed, Apr 21, 2021 at 01:03:36PM +0200, Uwe Kleine-König wrote:
> > On Wed, Apr 21, 2021 at 11:26:08AM +0200, Uwe Kleine-König wrote:
> > > With these three patches PWM_DEBUG is now happy. (At least I couldn't
> > > trigger a warning any more. I think there are still a few problems with
> > > integer overflows.)
> > 
> > BTW, setting the period to 138350580899 (with a clock rate of 133333333
> > Hz) results in setting period=0 because
> > 
> > 	state->period * clkrate =
> > 	138350580899 * 133333333 =
> > 	40254751 (discarded from 18446744073749806367).
> 
> As a first remedy the following could be done:
> 
> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index 38d86340201c..02d69fa5f7d2 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -199,6 +199,11 @@ static int atmel_pwm_calculate_cprd_and_pres(struct pwm_chip *chip,
>  	unsigned long long cycles = state->period;
>  	int shift;
>  
> +	if (fls(cycles) + fls(clkrate) > 64) {
> +		dev_err(chip->dev, "period to big to calculate HW parameters\n");
> +		return -EINVAL;
> +	}
> +
>  	/* Calculate the period cycles and prescale value */
>  	cycles *= clkrate;
>  	do_div(cycles, NSEC_PER_SEC);
> 
> Is this sensible? (Actually I'd prefer to just continue with
> 
> 	period = (ULL(1) << (64 - fls(clkrate))) - 1
> 
> according to the motto to yield the highest possible period, but this
> function has another error path that returns -EINVAL so this would be
> inconsistent.)

Shouldn't that be -ERANGE? I do think it is better to return an error
and let userspace decide what is the policy instead of having the policy
in the driver.


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: overflow and wrong timeout errors in pwm-atmel

[Alexandre Belloni]

On 21/04/2021 15:48:25+0200, Uwe Kleine-König wrote:
> On Wed, Apr 21, 2021 at 01:03:36PM +0200, Uwe Kleine-König wrote:
> > On Wed, Apr 21, 2021 at 11:26:08AM +0200, Uwe Kleine-König wrote:
> > > With these three patches PWM_DEBUG is now happy. (At least I couldn't
> > > trigger a warning any more. I think there are still a few problems with
> > > integer overflows.)
> > 
> > BTW, setting the period to 138350580899 (with a clock rate of 133333333
> > Hz) results in setting period=0 because
> > 
> > 	state->period * clkrate =
> > 	138350580899 * 133333333 =
> > 	40254751 (discarded from 18446744073749806367).
> 
> As a first remedy the following could be done:
> 
> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index 38d86340201c..02d69fa5f7d2 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -199,6 +199,11 @@ static int atmel_pwm_calculate_cprd_and_pres(struct pwm_chip *chip,
>  	unsigned long long cycles = state->period;
>  	int shift;
>  
> +	if (fls(cycles) + fls(clkrate) > 64) {
> +		dev_err(chip->dev, "period to big to calculate HW parameters\n");
> +		return -EINVAL;
> +	}
> +
>  	/* Calculate the period cycles and prescale value */
>  	cycles *= clkrate;
>  	do_div(cycles, NSEC_PER_SEC);
> 
> Is this sensible? (Actually I'd prefer to just continue with
> 
> 	period = (ULL(1) << (64 - fls(clkrate))) - 1
> 
> according to the motto to yield the highest possible period, but this
> function has another error path that returns -EINVAL so this would be
> inconsistent.)

Shouldn't that be -ERANGE? I do think it is better to return an error
and let userspace decide what is the policy instead of having the policy
in the driver.


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: overflow and wrong timeout errors in pwm-atmel

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3322 bytes --]

Hello Alexandre,

On Wed, Apr 21, 2021 at 04:18:37PM +0200, Alexandre Belloni wrote:
> On 21/04/2021 15:48:25+0200, Uwe Kleine-König wrote:
> > On Wed, Apr 21, 2021 at 01:03:36PM +0200, Uwe Kleine-König wrote:
> > > On Wed, Apr 21, 2021 at 11:26:08AM +0200, Uwe Kleine-König wrote:
> > > > With these three patches PWM_DEBUG is now happy. (At least I couldn't
> > > > trigger a warning any more. I think there are still a few problems with
> > > > integer overflows.)
> > > 
> > > BTW, setting the period to 138350580899 (with a clock rate of 133333333
> > > Hz) results in setting period=0 because
> > > 
> > > 	state->period * clkrate =
> > > 	138350580899 * 133333333 =
> > > 	40254751 (discarded from 18446744073749806367).
> > 
> > As a first remedy the following could be done:
> > 
> > diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> > index 38d86340201c..02d69fa5f7d2 100644
> > --- a/drivers/pwm/pwm-atmel.c
> > +++ b/drivers/pwm/pwm-atmel.c
> > @@ -199,6 +199,11 @@ static int atmel_pwm_calculate_cprd_and_pres(struct pwm_chip *chip,
> >  	unsigned long long cycles = state->period;
> >  	int shift;
> >  
> > +	if (fls(cycles) + fls(clkrate) > 64) {
> > +		dev_err(chip->dev, "period to big to calculate HW parameters\n");
> > +		return -EINVAL;
> > +	}
> > +
> >  	/* Calculate the period cycles and prescale value */
> >  	cycles *= clkrate;
> >  	do_div(cycles, NSEC_PER_SEC);
> > 
> > Is this sensible? (Actually I'd prefer to just continue with
> > 
> > 	period = (ULL(1) << (64 - fls(clkrate))) - 1
> > 
> > according to the motto to yield the highest possible period, but this
> > function has another error path that returns -EINVAL so this would be
> > inconsistent.)
> 
> Shouldn't that be -ERANGE?

The other exit point a few lines down also uses -EINVAL so I sticked to
that.

> I do think it is better to return an error and let userspace decide
> what is the policy instead of having the policy in the driver.

I agree that the consumer (userspace is just one of them) should have
the choice what happens. IMHO the only sane way to implement this is
using a round_state function that tells the consumer what they can
expect when a certain state is passed to apply. (Otherwise the consumer
might already be unhappy if they request a period of say 652799 ns and
the driver implements 645120 ns (which is BTW what happens with the
pwm-atmel driver when feed by a 133333333 Hz clk).)

And another critical detail of this round_state function is that it
should expose the same behaviour for all lowlevel drivers. I think
first rounding down period and then duty_cycle is a good strategy that
can be worked with. With that in place the next (IMHO) obvious
conclusion is that .apply() should behave in the same way for
consistency and also to keep the drivers simple.

If then a consumer prefers a different rounding strategy (e.g. for the
pwm-ir-tx driver rounding to the nearest values is better), this can be
(more or less) easily and (more important) generically implemented in a
single place.

Does this make sense in your eyes?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: overflow and wrong timeout errors in pwm-atmel

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 3322 bytes --]

Hello Alexandre,

On Wed, Apr 21, 2021 at 04:18:37PM +0200, Alexandre Belloni wrote:
> On 21/04/2021 15:48:25+0200, Uwe Kleine-König wrote:
> > On Wed, Apr 21, 2021 at 01:03:36PM +0200, Uwe Kleine-König wrote:
> > > On Wed, Apr 21, 2021 at 11:26:08AM +0200, Uwe Kleine-König wrote:
> > > > With these three patches PWM_DEBUG is now happy. (At least I couldn't
> > > > trigger a warning any more. I think there are still a few problems with
> > > > integer overflows.)
> > > 
> > > BTW, setting the period to 138350580899 (with a clock rate of 133333333
> > > Hz) results in setting period=0 because
> > > 
> > > 	state->period * clkrate =
> > > 	138350580899 * 133333333 =
> > > 	40254751 (discarded from 18446744073749806367).
> > 
> > As a first remedy the following could be done:
> > 
> > diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> > index 38d86340201c..02d69fa5f7d2 100644
> > --- a/drivers/pwm/pwm-atmel.c
> > +++ b/drivers/pwm/pwm-atmel.c
> > @@ -199,6 +199,11 @@ static int atmel_pwm_calculate_cprd_and_pres(struct pwm_chip *chip,
> >  	unsigned long long cycles = state->period;
> >  	int shift;
> >  
> > +	if (fls(cycles) + fls(clkrate) > 64) {
> > +		dev_err(chip->dev, "period to big to calculate HW parameters\n");
> > +		return -EINVAL;
> > +	}
> > +
> >  	/* Calculate the period cycles and prescale value */
> >  	cycles *= clkrate;
> >  	do_div(cycles, NSEC_PER_SEC);
> > 
> > Is this sensible? (Actually I'd prefer to just continue with
> > 
> > 	period = (ULL(1) << (64 - fls(clkrate))) - 1
> > 
> > according to the motto to yield the highest possible period, but this
> > function has another error path that returns -EINVAL so this would be
> > inconsistent.)
> 
> Shouldn't that be -ERANGE?

The other exit point a few lines down also uses -EINVAL so I sticked to
that.

> I do think it is better to return an error and let userspace decide
> what is the policy instead of having the policy in the driver.

I agree that the consumer (userspace is just one of them) should have
the choice what happens. IMHO the only sane way to implement this is
using a round_state function that tells the consumer what they can
expect when a certain state is passed to apply. (Otherwise the consumer
might already be unhappy if they request a period of say 652799 ns and
the driver implements 645120 ns (which is BTW what happens with the
pwm-atmel driver when feed by a 133333333 Hz clk).)

And another critical detail of this round_state function is that it
should expose the same behaviour for all lowlevel drivers. I think
first rounding down period and then duty_cycle is a good strategy that
can be worked with. With that in place the next (IMHO) obvious
conclusion is that .apply() should behave in the same way for
consistency and also to keep the drivers simple.

If then a consumer prefers a different rounding strategy (e.g. for the
pwm-ir-tx driver rounding to the nearest values is better), this can be
(more or less) easily and (more important) generically implemented in a
single place.

Does this make sense in your eyes?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] pwm: atmel: rework tracking updates pending in hardware

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1353 bytes --]

Hello Thierry,

On Wed, Apr 21, 2021 at 11:26:08AM +0200, Uwe Kleine-König wrote:
> This improves the driver's behavior in several ways:
> 
>  - The lock is held for shorter periods and so a channel that is currently
>    waited for doesn't block disabling another channel.
> 
>  - It's easier to understand because the procedure is split into more
>    semantic units and documentation is improved
> 
>  - A channel is only set to pending when such an event is actually
>    scheduled in hardware (by writing the CUPD register).
> 
>  - Also wait in .get_state() to report the last configured state instead
>    of (maybe) the previous one. This fixes the read back duty cycle and so
>    prevents a warning being emitted when PWM_DEBUG is on.
> 
> Tested on an AriettaG25.

On patchwork this patch is in the state "Under Review". Did you change
this? What does this mean? Does indeed someone look into this patch?

There was some discussion in reply to this patch, but that doesn't
affect correctness of it. It's just that there are still some problems
in the driver that this patch doesn't address, but the net effect of it
is still positive.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] pwm: atmel: rework tracking updates pending in hardware

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1353 bytes --]

Hello Thierry,

On Wed, Apr 21, 2021 at 11:26:08AM +0200, Uwe Kleine-König wrote:
> This improves the driver's behavior in several ways:
> 
>  - The lock is held for shorter periods and so a channel that is currently
>    waited for doesn't block disabling another channel.
> 
>  - It's easier to understand because the procedure is split into more
>    semantic units and documentation is improved
> 
>  - A channel is only set to pending when such an event is actually
>    scheduled in hardware (by writing the CUPD register).
> 
>  - Also wait in .get_state() to report the last configured state instead
>    of (maybe) the previous one. This fixes the read back duty cycle and so
>    prevents a warning being emitted when PWM_DEBUG is on.
> 
> Tested on an AriettaG25.

On patchwork this patch is in the state "Under Review". Did you change
this? What does this mean? Does indeed someone look into this patch?

There was some discussion in reply to this patch, but that doesn't
affect correctness of it. It's just that there are still some problems
in the driver that this patch doesn't address, but the net effect of it
is still positive.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] pwm: atmel: Fix duty cycle calculation in .get_state()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 457 bytes --]

On Tue, Apr 20, 2021 at 11:51:17AM +0200, Uwe Kleine-König wrote:
> The CDTY register contains the number of inactive cycles. .apply() does
> this correctly, however .get_state() got this wrong.
> 
> Fixes: 651b510a74d4 ("pwm: atmel: Implement .get_state()")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/pwm-atmel.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] pwm: atmel: Fix duty cycle calculation in .get_state()

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 457 bytes --]

On Tue, Apr 20, 2021 at 11:51:17AM +0200, Uwe Kleine-König wrote:
> The CDTY register contains the number of inactive cycles. .apply() does
> this correctly, however .get_state() got this wrong.
> 
> Fixes: 651b510a74d4 ("pwm: atmel: Implement .get_state()")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/pwm-atmel.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied, thanks.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel