| Message ID | 20210428001946.1059426-1-roman.beranek@prusa3d.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pwm: sun4i: Round delay time up to a nearest jiffy | expand |
Hello Roman, On Wed, Apr 28, 2021 at 02:19:46AM +0200, Roman Beranek wrote: > More often than not, a PWM period may span nowhere near as far > as 1 jiffy, yet it still must be waited upon before the channel > is disabled. I wonder what happens if you don't wait long enough. Is this a theoretical issue, or do you see an (occasional?) breakage that is fixed by this patch? I guess the problem is that if you disable too early the output freezes and that might be in a state where the output is still active? Would polling the PWMx_RDY bit in the control register help here? Best regards Uwe
Hello Uwe, Correct, the output may stay in an active state. I only discovered this bug as I investigated a report of unreliable screen timeout. The period we use the PWM with is 50 us. The PWMx_RDY bit stays 0 well after the last period ends, so if the bit has any function at all, this one is certainly not it. Cheers, Roman Note: my apologies for the previous HTML message On Wed, Apr 28, 2021 at 8:14 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hello Roman, > > On Wed, Apr 28, 2021 at 02:19:46AM +0200, Roman Beranek wrote: > > More often than not, a PWM period may span nowhere near as far > > as 1 jiffy, yet it still must be waited upon before the channel > > is disabled. > > I wonder what happens if you don't wait long enough. Is this a > theoretical issue, or do you see an (occasional?) breakage that is fixed > by this patch? > > I guess the problem is that if you disable too early the output freezes > and that might be in a state where the output is still active? Would > polling the PWMx_RDY bit in the control register help here? > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |
Hello Roman, On Wed, Apr 28, 2021 at 02:14:31PM +0200, Roman Beránek wrote: > Correct, the output may stay in an active state. I only discovered this > bug as I investigated a report of unreliable screen timeout. The period > we use the PWM with is 50 us. What I don't like here is that the delay is quite coarse and might still be too short. (Maybe I miss something, but consider the current period is quite long, then you configure a short one and then disable. In this case the inital long setting might still be active.) > The PWMx_RDY bit stays 0 well after the last period ends, so if the bit > has any function at all, this one is certainly not it. OK. A way out could be to not disable the clock on .enable = 0. This might (or might not) have an impact on power consumption, but it improves correctness and simplifies the driver as the delay just goes away. So you might consider it a good trade-off, too. Maybe there is also a nice solution with runtime-PM?! > Note: my apologies for the previous HTML message I didn't notice (because the message also contained a txt part). Another thing to improve is to reply inline instead of top posting :-) Best regards Uwe
Hello Uwe, On Thu, Apr 29, 2021 at 2:04 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hello Roman, > > On Wed, Apr 28, 2021 at 02:14:31PM +0200, Roman Beránek wrote: > > Correct, the output may stay in an active state. I only discovered this > > bug as I investigated a report of unreliable screen timeout. The period > > we use the PWM with is 50 us. > > What I don't like here is that the delay is quite coarse and might still > be too short. (Maybe I miss something, but consider the current period > is quite long, then you configure a short one and then disable. In this > case the inital long setting might still be active.) The delay is calculated from the original period (cstate.period), not the one that was just written into PWM_CHx_PRD 2 lines above. > > Note: my apologies for the previous HTML message > > I didn't notice (because the message also contained a txt part). Another > thing to improve is to reply inline instead of top posting :-) Yeah, learning the conventions one reply at a time :-) Cheers, Roman
Hello Roman, On Fri, Apr 30, 2021 at 04:19:32AM +0200, Roman Beránek wrote: > On Thu, Apr 29, 2021 at 2:04 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Wed, Apr 28, 2021 at 02:14:31PM +0200, Roman Beránek wrote: > > > Correct, the output may stay in an active state. I only discovered this > > > bug as I investigated a report of unreliable screen timeout. The period > > > we use the PWM with is 50 us. > > > > What I don't like here is that the delay is quite coarse and might still > > be too short. (Maybe I miss something, but consider the current period > > is quite long, then you configure a short one and then disable. In this > > case the inital long setting might still be active.) > > The delay is calculated from the original period (cstate.period), > not the one that was just written into PWM_CHx_PRD 2 lines above. Yes, but that's not good enough. Consider the PWM is running with a period of 4s and the period just started. Then you call pwm_apply_state(mypwm, &(struct pwm_state){ .period = 20, .enabled = 1, ... }) This doesn't result into the waiting code being run, because .enabled = 1. Then immidiately after that call: pwm_apply_state(mypwm, &(struct pwm_state){ .period = 20, .enabled = 0, ... }) which triggers the waiting but only based on a period of 20 ns while the 4s period is still active. Best regards Uwe
Hello Uwe, On Fri Apr 30, 2021 at 8:41 AM CEST, Uwe Kleine-König wrote: > On Fri, Apr 30, 2021 at 04:19:32AM +0200, Roman Beránek wrote: > > On Thu, Apr 29, 2021 at 2:04 PM Uwe Kleine-König wrote: > > > On Wed, Apr 28, 2021 at 02:14:31PM +0200, Roman Beránek wrote: > > > > Correct, the output may stay in an active state. I only discovered this > > > > bug as I investigated a report of unreliable screen timeout. The period > > > > we use the PWM with is 50 us. > > > > > > What I don't like here is that the delay is quite coarse and might still > > > be too short. (Maybe I miss something, but consider the current period > > > is quite long, then you configure a short one and then disable. In this > > > case the inital long setting might still be active.) > > > > The delay is calculated from the original period (cstate.period), > > not the one that was just written into PWM_CHx_PRD 2 lines above. > > Yes, but that's not good enough. Consider the PWM is running with a > period of 4s and the period just started. Then you call > > pwm_apply_state(mypwm, &(struct pwm_state){ > .period = 20, > .enabled = 1, > ... > }) > > This doesn't result into the waiting code being run, because > .enabled = 1. Then immidiately after that call: > > pwm_apply_state(mypwm, &(struct pwm_state){ > .period = 20, > .enabled = 0, > ... > }) > > which triggers the waiting but only based on a period of 20 ns while the > 4s period is still active. OK, now I see what you mean. To remedy this, the delay shall occur regardless of state->enabled. In my view, this lies beneath the scope of the patch though and would be better served as a follow-up. Cheers, Roman
Hello Roman, On Fri, Apr 30, 2021 at 09:17:49AM +0200, Roman Beranek wrote: > On Fri Apr 30, 2021 at 8:41 AM CEST, Uwe Kleine-König wrote: > > Consider the PWM is running with a period of 4s and the period just > > started. Then you call > > > > pwm_apply_state(mypwm, &(struct pwm_state){ > > .period = 20, > > .enabled = 1, > > ... > > }) > > > > This doesn't result into the waiting code being run, because > > .enabled = 1. Then immidiately after that call: > > > > pwm_apply_state(mypwm, &(struct pwm_state){ > > .period = 20, > > .enabled = 0, > > ... > > }) > > > > which triggers the waiting but only based on a period of 20 ns while the > > 4s period is still active. > > OK, now I see what you mean. To remedy this, the delay shall occur > regardless of state->enabled. > > In my view, this lies beneath the scope of the patch though and would be > better served as a follow-up. If you agree that dropping both delay and clk_disable completely is the right thing, you address both problems and going forward with your patch isn't sensible. Best regards Uwe
Hello Uwe, On Fri Apr 30, 2021 at 11:51 AM CEST, Uwe Kleine-König wrote: > If you agree that dropping both delay and clk_disable completely is the > right thing, you address both problems and going forward with your patch > isn't sensible. I had my doubts whether simply clearing the PWMx_EN bit would be enough to turn the PWM off but I stand corrected. It does work. The added bonusu is that once without the invocations of {u,m}sleep and clk_{enable_,disable_un}prepare, the sun4i_pwm_apply function finally becomes safe for running in an atomic context. I will therefore prepare a new patch and come back some time next week. Have a great weekend, Roman
On 2021-04-30 17:10, Roman Beranek wrote: > Hello Uwe, > > On Fri Apr 30, 2021 at 11:51 AM CEST, Uwe Kleine-König wrote: >> If you agree that dropping both delay and clk_disable completely is the >> right thing, you address both problems and going forward with your patch >> isn't sensible. > > I had my doubts whether simply clearing the PWMx_EN bit would be enough > to turn the PWM off but I stand corrected. It does work. > > The added bonusu is that once without the invocations of {u,m}sleep and > clk_{enable_,disable_un}prepare, the sun4i_pwm_apply function finally > becomes safe for running in an atomic context. > > I will therefore prepare a new patch and come back some time next week. > > Have a great weekend, > Roman Hi Roman, A while ago others and I have made an attempt at fixing this as well. Before you continue, I'd like to draw your attention to these previous attempts. Maybe in the mean time things have changed, maybe they haven't, but I can definitely tell you that just clearing the enable bit isn't enough. There is some precise timing involved, hence the delays are there. Unfortunately to my knowledge, this hasn't been documented anywhere so it's just trial and error. Please take a look at this post: https://lkml.org/lkml/2020/3/17/1158 Good luck! Regards, Pascal
diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c index ce5c4fc8d..f4b991048 100644 --- a/drivers/pwm/pwm-sun4i.c +++ b/drivers/pwm/pwm-sun4i.c @@ -285,7 +285,7 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, val = (duty & PWM_DTY_MASK) | PWM_PRD(period); sun4i_pwm_writel(sun4i_pwm, val, PWM_CH_PRD(pwm->hwpwm)); sun4i_pwm->next_period[pwm->hwpwm] = jiffies + - nsecs_to_jiffies(cstate.period + 1000); + nsecs_to_jiffies(cstate.period) + 1; if (state->polarity != PWM_POLARITY_NORMAL) ctrl &= ~BIT_CH(PWM_ACT_STATE, pwm->hwpwm);
More often than not, a PWM period may span nowhere near as far as 1 jiffy, yet it still must be waited upon before the channel is disabled. Signed-off-by: Roman Beranek <roman.beranek@prusa3d.com> --- drivers/pwm/pwm-sun4i.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)