| Message ID | 20210421092606.1634092-1-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | pwm: atmel: rework tracking updates pending in hardware | expand |
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
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
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.
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
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
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; }
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(-)