| Message ID | 20221003015546.202308-5-doug@schmorgal.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | pwm: pxa: Fixes for enable/disable transitions | expand |
On Sun, Oct 02, 2022 at 06:55:45PM -0700, Doug Brown wrote: > If the clock is turned on too quickly after being turned off, it won't > actually turn back on. Work around this problem by waiting for the final > period to complete when disabling the PWM. The delay logic is borrowed > from the pwm-sun4i driver. > > To avoid unnecessary delays, skip the whole config process if the PWM is > already disabled and staying disabled. I wonder if there is some documentation available about this issue. This feels like a workaround without knowledge about the details and so might break at the next opportunity. > [...] > @@ -122,6 +127,18 @@ static int pxa_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > if (!state->enabled && pwm->state.enabled) > clk_disable_unprepare(pc->clk); > > + if (state->enabled) > + return 0; > + > + /* Wait for the final PWM period to finish. This prevents it from > + * being re-enabled too quickly (which can fail silently). > + */ Please stick to the usual comment style. i.e. put the /* on a line for itself. Best regards Uwe
Hi Uwe, On 10/19/2022 12:39 AM, Uwe Kleine-König wrote: > On Sun, Oct 02, 2022 at 06:55:45PM -0700, Doug Brown wrote: >> If the clock is turned on too quickly after being turned off, it won't >> actually turn back on. Work around this problem by waiting for the final >> period to complete when disabling the PWM. The delay logic is borrowed >> from the pwm-sun4i driver. >> >> To avoid unnecessary delays, skip the whole config process if the PWM is >> already disabled and staying disabled. > > I wonder if there is some documentation available about this issue. This > feels like a workaround without knowledge about the details and so might > break at the next opportunity. Thanks for reviewing! Yes, it does feel like a crazy workaround. I'm not super proud of it. The best documentation I've been able to look at is the PXA168 software manual [1]. Page 502 of the PDF talks about a "graceful shutdown" where turning off the clock enable bit doesn't immediately stop the clock, and instead it waits for the current PWM period to complete first. This driver is currently configuring it for graceful shutdown mode, because the PWMCR_SD bit is not set (page 1257). I've experimentally determined that if you try to turn the clock back on when a graceful shutdown is still scheduled, it doesn't cancel the graceful shutdown, so the clock ends up off afterward, even though the common clock framework thinks it's still on. The hacky delay in this commit works around that problem. This almost seems like a problem that should be solved on the common clock framework side instead, but it doesn't know what the PWM frequency is so it wouldn't know how long to delay. Do all the other similar drivers in the kernel do a graceful shutdown like this when they are turned off? If not, a simpler solution would be to start turning on the PWMCR_SD bit instead, so the clock stops immediately (potentially resulting in the final duty cycle being short). I tested that change in place of this commit and it seems to work pretty well, although I can still cause it to fail if I turn my PWM backlight off and back on quickly without a "sleep 0.000001" in between. It feels to me like there are some weird interdependencies between the clock enable bits and the actual PWM controller block, at least in the PXA168, likely due to "graceful shutdown" mode's existence. What do you think? Turning on the PWMCR_SD bit would be very simple, but it doesn't fully fix the issue in my testing. I'd still be okay with it though, because the only failure case I can reproduce is a minor edge case (plus, I don't love the delay solution). > >> [...] >> @@ -122,6 +127,18 @@ static int pxa_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, >> if (!state->enabled && pwm->state.enabled) >> clk_disable_unprepare(pc->clk); >> >> + if (state->enabled) >> + return 0; >> + >> + /* Wait for the final PWM period to finish. This prevents it from >> + * being re-enabled too quickly (which can fail silently). >> + */ > > Please stick to the usual comment style. i.e. put the /* on a line for > itself. My bad, thanks for pointing this out. If this comment still exists in the next version of the patch, I'll fix it. Thanks, Doug [1] https://web.archive.org/web/20160428154454/http://www.marvell.com/application-processors/armada-100/assets/armada_16x_software_manual.pdf
Hello Doug, On Sat, Oct 22, 2022 at 12:35:23PM -0700, Doug Brown wrote: > On 10/19/2022 12:39 AM, Uwe Kleine-König wrote: > > On Sun, Oct 02, 2022 at 06:55:45PM -0700, Doug Brown wrote: > > > If the clock is turned on too quickly after being turned off, it won't > > > actually turn back on. Work around this problem by waiting for the final > > > period to complete when disabling the PWM. The delay logic is borrowed > > > from the pwm-sun4i driver. > > > > > > To avoid unnecessary delays, skip the whole config process if the PWM is > > > already disabled and staying disabled. > > > > I wonder if there is some documentation available about this issue. This > > feels like a workaround without knowledge about the details and so might > > break at the next opportunity. > > Thanks for reviewing! Yes, it does feel like a crazy workaround. I'm not > super proud of it. The best documentation I've been able to look at is > the PXA168 software manual [1]. Page 502 of the PDF talks about a > "graceful shutdown" where turning off the clock enable bit doesn't > immediately stop the clock, and instead it waits for the current PWM > period to complete first. This driver is currently configuring it for > graceful shutdown mode, because the PWMCR_SD bit is not set (page 1257). > > I've experimentally determined that if you try to turn the clock back on > when a graceful shutdown is still scheduled, it doesn't cancel the > graceful shutdown, so the clock ends up off afterward, even though the > common clock framework thinks it's still on. The hacky delay in this > commit works around that problem. This almost seems like a problem that > should be solved on the common clock framework side instead, but it > doesn't know what the PWM frequency is so it wouldn't know how long to > delay. > > Do all the other similar drivers in the kernel do a graceful shutdown > like this when they are turned off? If not, a simpler solution would be > to start turning on the PWMCR_SD bit instead, so the clock stops > immediately (potentially resulting in the final duty cycle being short). There are supported hardwares that (only) support immediate shutdown, so consumers cannot rely on a graceful stop anyhow. So I'd say using PWMCR_SD=1 is fine. The documentation suggests that in this case "PWM_OUTx stops after at most one PSCLK_PWM" which might explain that the problem can still be triggered. (i.e. if you reenable before the next PSCLK_PWM cycle.) BTW, the driver lacks information about how the hardware behaves on disable. The most common cases are - emits the inactive output - freezes where it just happens to be (assuming PWMCR_SD=1) Would you mind testing that and adding a patch to your series? (Look at e.g. drivers/pwm/pwm-sl28cpld.c for the desired format to ease grepping.) > I tested that change in place of this commit and it seems to work pretty > well, although I can still cause it to fail if I turn my PWM backlight > off and back on quickly without a "sleep 0.000001" in between. It feels > to me like there are some weird interdependencies between the clock > enable bits and the actual PWM controller block, at least in the PXA168, > likely due to "graceful shutdown" mode's existence. > > What do you think? Turning on the PWMCR_SD bit would be very simple, but > it doesn't fully fix the issue in my testing. I'd still be okay with it > though, because the only failure case I can reproduce is a minor edge > case (plus, I don't love the delay solution). +1 for no love for the delay solution. Best regards Uwe
Hi Uwe, On 11/9/2022 1:26 AM, Uwe Kleine-König wrote: > There are supported hardwares that (only) support immediate shutdown, so > consumers cannot rely on a graceful stop anyhow. So I'd say using > PWMCR_SD=1 is fine. The documentation suggests that in this case "PWM_OUTx stops > after at most one PSCLK_PWM" which might explain that the problem can > still be triggered. (i.e. if you reenable before the next PSCLK_PWM > cycle.) Thanks for taking a look! Your point about PSCLK_PWM does make sense and likely explains how I'm still able to trigger the problem. > BTW, the driver lacks information about how the hardware behaves on > disable. The most common cases are > > - emits the inactive output > - freezes where it just happens to be (assuming PWMCR_SD=1) > > Would you mind testing that and adding a patch to your series? (Look at > e.g. drivers/pwm/pwm-sl28cpld.c for the desired format to ease > grepping.) Will do. I assume you're referring to the "Limitations" section at the top of a lot of the drivers. It appears that this hardware always emits the inactive input when the clock disables -- almost immediately if PWMCR_SD=1, or at the end of the current duty cycle if PWMCR_SD=0. Doug
diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c index cf4d22c91929..edcef67f7ffe 100644 --- a/drivers/pwm/pwm-pxa.c +++ b/drivers/pwm/pwm-pxa.c @@ -17,6 +17,7 @@ #include <linux/io.h> #include <linux/pwm.h> #include <linux/of_device.h> +#include <linux/delay.h> #include <asm/div64.h> @@ -96,12 +97,16 @@ static int pxa_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, const struct pwm_state *state) { struct pxa_pwm_chip *pc = to_pxa_pwm_chip(chip); + unsigned int delay_us; u64 duty_cycle; int err; if (state->polarity != PWM_POLARITY_NORMAL) return -EINVAL; + if (!state->enabled && !pwm->state.enabled) + return 0; + err = clk_prepare_enable(pc->clk); if (err) return err; @@ -122,6 +127,18 @@ static int pxa_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, if (!state->enabled && pwm->state.enabled) clk_disable_unprepare(pc->clk); + if (state->enabled) + return 0; + + /* Wait for the final PWM period to finish. This prevents it from + * being re-enabled too quickly (which can fail silently). + */ + delay_us = DIV_ROUND_UP_ULL(state->period, NSEC_PER_USEC); + if ((delay_us / 500) > MAX_UDELAY_MS) + msleep(delay_us / 1000 + 1); + else + usleep_range(delay_us, delay_us * 2); + return 0; }
If the clock is turned on too quickly after being turned off, it won't actually turn back on. Work around this problem by waiting for the final period to complete when disabling the PWM. The delay logic is borrowed from the pwm-sun4i driver. To avoid unnecessary delays, skip the whole config process if the PWM is already disabled and staying disabled. Signed-off-by: Doug Brown <doug@schmorgal.com> --- drivers/pwm/pwm-pxa.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)