[PATCH] pwm: sun4i: Round delay time up to a nearest jiffy

[Roman Beranek]

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....@prusa3d.com>
---
drivers/pwm/pwm-sun4i.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

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);
--
2.31.1

[Uwe Kleine-König]

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/ |

[Roman Beránek]

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

[Roman Beránek]

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.klein...@pengutronix.de> wrote:
>

[Uwe Kleine-König]

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 :-)

[Roman Beránek]

Hello Uwe,

On Thu, Apr 29, 2021 at 2:04 PM Uwe Kleine-König
<u.klein...@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

[Uwe Kleine-König]

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.klein...@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.

[Roman Beranek]

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:

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

[Uwe Kleine-König]

Hello Roman,

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.

[Roman Beranek]

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

[d...@pascalroeleven.nl]

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