diff mbox series

pwm: atmel: rework tracking updates pending in hardware

Message ID 20210421092606.1634092-1-u.kleine-koenig@pengutronix.de (mailing list archive)
State New
Headers show
Series pwm: atmel: rework tracking updates pending in hardware | expand

Commit Message

Uwe Kleine-König April 21, 2021, 9:26 a.m. UTC
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(-)

Comments

Uwe Kleine-König April 21, 2021, 1:48 p.m. UTC | #1
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
Alexandre Belloni April 21, 2021, 2:18 p.m. UTC | #2
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.
Uwe Kleine-König April 21, 2021, 3:26 p.m. UTC | #3
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
diff mbox series

Patch

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