[PATCH] pwm: sun4i: Avoid waiting until the next period

[Roman Beranek]

As disabling PWM by clearing the PWM_EN bit doesn't take an effect until
the last pulse cycle ends, gating the clock too soon may result in the
output signal getting stuck in an active state. Although the code gives
an appearance that it takes care of this particular problem by waiting
for the next period before finally clearing the CLK_GATING and EN bits,
unless the EN bit has already been cleared by the time the delay begins,
this measure doesn't achieve anything.

However, even if this detail were to be fixed, there would still remain
another issue to deal with: if the PWM were to be disabled shortly after
having its period shortened, the length of the delay might turn out
insufficient. So instead of waiting for the moment when it becomes safe
to gate the clock, let's not bother gating it in the first place.

Signed-off-by: Roman Beranek <roman....@prusa3d.com>
Suggested-by: Uwe Kleine-König <u.klein...@pengutronix.de>
---
drivers/pwm/pwm-sun4i.c | 52 +++++++++++------------------------------
1 file changed, 13 insertions(+), 39 deletions(-)

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index e01becd10..809163186 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -89,7 +89,6 @@ struct sun4i_pwm_chip {
void __iomem *base;
spinlock_t ctrl_lock;
const struct sun4i_pwm_data *data;
- unsigned long next_period[2];
};

static inline struct sun4i_pwm_chip *to_sun4i_pwm_chip(struct pwm_chip *chip)
@@ -235,26 +234,15 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
struct pwm_state cstate;
u32 ctrl, duty = 0, period = 0, val;
int ret;
- unsigned int delay_us, prescaler = 0;
- unsigned long now;
+ unsigned int prescaler = 0;
bool bypass;

pwm_get_state(pwm, &cstate);

- if (!cstate.enabled) {
- ret = clk_prepare_enable(sun4i_pwm->clk);
- if (ret) {
- dev_err(chip->dev, "failed to enable PWM clock\n");
- return ret;
- }
- }
-
ret = sun4i_pwm_calculate(sun4i_pwm, state, &duty, &period, &prescaler,
&bypass);
if (ret) {
dev_err(chip->dev, "period exceeds the maximum value\n");
- if (!cstate.enabled)
- clk_disable_unprepare(sun4i_pwm->clk);
return ret;
}

@@ -284,8 +272,6 @@ 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);

if (state->polarity != PWM_POLARITY_NORMAL)
ctrl &= ~BIT_CH(PWM_ACT_STATE, pwm->hwpwm);
@@ -296,34 +282,12 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,

if (state->enabled)
ctrl |= BIT_CH(PWM_EN, pwm->hwpwm);
+ else
+ ctrl &= ~BIT_CH(PWM_EN, pwm->hwpwm);

sun4i_pwm_writel(sun4i_pwm, ctrl, PWM_CTRL_REG);
-
- spin_unlock(&sun4i_pwm->ctrl_lock);
-
- if (state->enabled)
- return 0;
-
- /* We need a full period to elapse before disabling the channel. */
- now = jiffies;
- if (time_before(now, sun4i_pwm->next_period[pwm->hwpwm])) {
- delay_us = jiffies_to_usecs(sun4i_pwm->next_period[pwm->hwpwm] -
- now);
- if ((delay_us / 500) > MAX_UDELAY_MS)
- msleep(delay_us / 1000 + 1);
- else
- usleep_range(delay_us, delay_us * 2);
- }
-
- spin_lock(&sun4i_pwm->ctrl_lock);
- ctrl = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
- ctrl &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
- ctrl &= ~BIT_CH(PWM_EN, pwm->hwpwm);
- sun4i_pwm_writel(sun4i_pwm, ctrl, PWM_CTRL_REG);
spin_unlock(&sun4i_pwm->ctrl_lock);

- clk_disable_unprepare(sun4i_pwm->clk);
-
return 0;
}

@@ -457,6 +421,13 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
goto err_bus;
}

+ ret = clk_prepare_enable(pwm->clk);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to prepare and enable PWM clock %pe\n",
+ ERR_PTR(ret));
+ goto err_clk;
+ }
+
pwm->chip.dev = &pdev->dev;
pwm->chip.ops = &sun4i_pwm_ops;
pwm->chip.npwm = pwm->data->npwm;
@@ -476,6 +447,8 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
return 0;

err_pwm_add:
+ clk_disable_unprepare(pwm->clk);
+err_clk:
clk_disable_unprepare(pwm->bus_clk);
err_bus:
reset_control_assert(pwm->rst);
@@ -492,6 +465,7 @@ static int sun4i_pwm_remove(struct platform_device *pdev)
if (ret)
return ret;

+ clk_disable_unprepare(pwm->clk);
clk_disable_unprepare(pwm->bus_clk);
reset_control_assert(pwm->rst);

--
2.31.1

[Uwe Kleine-König]

Hello Emil,

On Wed, May 12, 2021 at 02:55:26AM +0200, Emil Lenngren wrote:
> Well that's one way of "solving it" ;)
>
> But on what hardware do you really need to wait until one full pulse
> cycle ends, before a disable command takes effect?
>
> On the hardware I've tested on (GR8 and V3s), it's enough to wait at
> most two clock cycles in order for it to take effect before we can
> close the gate. And with clock cycle I mean 24 MHz divided by the
> prescaler. With prescaler 1, that's 84 nanoseconds. By closing the
> gate when the pwm should be disabled, I guess we could save some
> nanoampere or microampere (is this important?)

If I understood correctly you really have to wait longer to achieve that
the output is inactive in the disabled state. Do you talk about the same
thing?

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |

[Roman Beranek]

Hello Emil,

On Wed May 12, 2021 at 2:55 AM CEST, Emil Lenngren wrote:
> But on what hardware do you really need to wait until one full pulse
> cycle ends, before a disable command takes effect?

I have no idea. The value has been there already for nearly 4 years
(since c32c5c50d4fe).

> By closing the gate when the pwm should be disabled, I guess we could
> save some nanoampere or microampere (is this important?)

My guess is that once the last cycle ends, the counter won't get
incremented any longer. But my guess is of course as good as yours,
I don't have an easy access to equipment capable of measurement this
precise.

> On the hardware I've tested on (GR8 and V3s), it's enough to wait at
> most two clock cycles in order for it to take effect before we can
> close the gate. And with clock cycle I mean 24 MHz divided by the
> prescaler. With prescaler 1, that's 84 nanoseconds.

In such case I could easily imagine keeping the clock gate around. Like
this:
---
drivers/pwm/pwm-sun4i.c | 33 +++++++++++++++++----------------
1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index d6d6d43f6e81..3350f6517dbd 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -73,7 +73,7 @@ static const u32 prescaler_table[] = {
72000,
0,
0,
- 0, /* Actually 1 but tested separately */
+ 1, /* Tested separately */
};

struct sun4i_pwm_data {
@@ -91,7 +91,7 @@ struct sun4i_pwm_chip {

void __iomem *base;
spinlock_t ctrl_lock;
const struct sun4i_pwm_data *data;
- unsigned long next_period[2];

+ u64 ready_to_be_gated[2];

};

static inline struct sun4i_pwm_chip *to_sun4i_pwm_chip(struct pwm_chip *chip)

@@ -237,10 +237,12 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,

struct pwm_state cstate;
u32 ctrl, duty = 0, period = 0, val;
int ret;
- unsigned int delay_us, prescaler = 0;
- unsigned long now;

+ unsigned int cycle_ns, delay, prescaler = 0;
+ u64 now;
bool bypass;

+ cycle_ns = NSEC_PER_SEC / clk_get_rate(sun4i_pwm->clk);
+
pwm_get_state(pwm, &cstate);

if (!cstate.enabled) {
@@ -286,8 +288,11 @@ 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);

+
+ now = get_jiffies_64();
+ delay = nsecs_to_jiffies(2 * prescaler_table[prescaler] * cycle_ns) + 1;
+ if (time_before64(sun4i_pwm->ready_to_be_gated[pwm->hwpwm], now + delay))
+ sun4i_pwm->ready_to_be_gated[pwm->hwpwm] = now + delay;

if (state->polarity != PWM_POLARITY_NORMAL)
ctrl &= ~BIT_CH(PWM_ACT_STATE, pwm->hwpwm);

@@ -298,6 +303,8 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,

if (state->enabled)
ctrl |= BIT_CH(PWM_EN, pwm->hwpwm);
+ else
+ ctrl &= ~BIT_CH(PWM_EN, pwm->hwpwm);

sun4i_pwm_writel(sun4i_pwm, ctrl, PWM_CTRL_REG);

@@ -306,21 +313,15 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
if (state->enabled)
return 0;

- /* We need a full period to elapse before disabling the channel. */
- now = jiffies;
- if (time_before(now, sun4i_pwm->next_period[pwm->hwpwm])) {
- delay_us = jiffies_to_usecs(sun4i_pwm->next_period[pwm->hwpwm] -
- now);
- if ((delay_us / 500) > MAX_UDELAY_MS)
- msleep(delay_us / 1000 + 1);
- else
- usleep_range(delay_us, delay_us * 2);

+ /* We need 1-2 clock cycles to elapse before disabling the channel. */
+ if (time_before64(now, sun4i_pwm->ready_to_be_gated[pwm->hwpwm])) {
+ delay = (unsigned int)(sun4i_pwm->ready_to_be_gated[pwm->hwpwm] - now);
+ msleep(jiffies_to_msecs(delay));
}

spin_lock(&sun4i_pwm->ctrl_lock);
ctrl = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);

ctrl &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
- ctrl &= ~BIT_CH(PWM_EN, pwm->hwpwm);

[Roman Beranek]

On Wed May 12, 2021 at 2:55 AM CEST, Emil Lenngren wrote:
> By closing the gate when the pwm should be disabled, I guess we could
> save some nanoampere or microampere (is this important?)

Or perhaps I could add suspend/resume ops and have the gate closed for
disabled channels after a fixed-duration delay of 2 cycles * 72k prsclr
(6 ms) until the system is resumed?

Cheers,
Roman

[Thierry Reding]

On Wed, May 12, 2021 at 11:18:24AM +0200, Emil Lenngren wrote:
> Hi Uwe,
>
> Den ons 12 maj 2021 kl 06:41 skrev Uwe Kleine-König
> <u.klein...@pengutronix.de>:

> >
> > Hello Emil,
> >
> > On Wed, May 12, 2021 at 02:55:26AM +0200, Emil Lenngren wrote:
> > > Well that's one way of "solving it" ;)
> > >
> > > But on what hardware do you really need to wait until one full pulse
> > > cycle ends, before a disable command takes effect?
> > >
> > > On the hardware I've tested on (GR8 and V3s), it's enough to wait at
> > > most two clock cycles in order for it to take effect before we can
> > > close the gate. And with clock cycle I mean 24 MHz divided by the
> > > prescaler. With prescaler 1, that's 84 nanoseconds. By closing the
> > > gate when the pwm should be disabled, I guess we could save some
> > > nanoampere or microampere (is this important?)
> >
> > If I understood correctly you really have to wait longer to achieve that
> > the output is inactive in the disabled state. Do you talk about the same
> > thing?
>

> Exactly, i.e. after writing 0 to the EN bit, we don't have to wait
> until the current period ends before we can observe that the output
> signal goes to the inactive state.
>
> Simple test:
>
> 1. Set pwm interval to a long time like 2 seconds, and duty to 50%.
> 2. Enable clock gating.
> 3. Enable the pwm by writing 1 to the EN bit.
> 4. Observe the LED blink once per second.
> 5. Now at a random time write 0 to the EN bit in order to disable the
> pwm. Don't turn off the clock gating.
> 6. If you just look with the eye it appears the LED turns off
> immediately, regardless of when in the pulse cycle we disabled it.
>
> Just tested the above using "devmem" on a V3s.
>
> By using a large prescaler and testing some different prescalers, I've
> concluded that it takes at least 1 and at most 2 clock cycles before
> we can safely turn off the gate and be certain that the output pin has
> changed to disabled.
>
> It would be good if people having other hardware could confirm this is
> correct there as well.
>
> Please take a look at some previous material I wrote:
> https://lkml.org/lkml/2020/3/17/1158
> https://linux-sunxi.org/PWM_Controller_Register_Guide (Observed
> behaviour on GR8 from NextThing)
> https://pastebin.com/GWrhWzPJ

I'm pretty sure Alexandre at the time reported that the instantiation of
the controller that he was using required waiting for the period to
complete before the output went to the disabled state. It's possible
that this was changed in subsequent versions of the IP, so perhaps we
need to distinguish based on compatible string?

Thierry

[Roman Beranek]

Hello Thierry,

On Tue May 25, 2021 at 6:41 PM CEST, Thierry Reding wrote:
> I'm pretty sure Alexandre at the time reported that the instantiation of
> the controller that he was using required waiting for the period to
> complete before the output went to the disabled state. It's possible
> that this was changed in subsequent versions of the IP, so perhaps we
> need to distinguish based on compatible string?

I've got myself an A10 (sun4i) board to test my new patchset with and
indeed the 2 cycles seem to be enough.

I have yet to write a cover letter for it though, expect it by Monday
at the latest.

Best regards,
Roman

[Alexandre Belloni]

Hi,

On 27/05/2021 14:10:35+0200, Roman Beranek wrote:
> Hello Thierry,
>
> On Tue May 25, 2021 at 6:41 PM CEST, Thierry Reding wrote:
> > I'm pretty sure Alexandre at the time reported that the instantiation of
> > the controller that he was using required waiting for the period to
> > complete before the output went to the disabled state. It's possible
> > that this was changed in subsequent versions of the IP, so perhaps we
> > need to distinguish based on compatible string?
>

I can't recall what I tested exactly. I probably assumed it would take
the whole period to update because this is how it is working on v1 of
the atmel PWM and this is what I was working on at the time. I did test
on a CHIP. I guess linux-sunxi.org is more correct than I was at the
time.

> I've got myself an A10 (sun4i) board to test my new patchset with and
> indeed the 2 cycles seem to be enough.
>
> I have yet to write a cover letter for it though, expect it by Monday
> at the latest.
>
> Best regards,
> Roman

--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Uwe Kleine-König]

Hello Roman,

You didn't send an update here (or did I miss it?). Still I think it's
correct that the patch we're discussion here is not suiteable to be
merged as is and so I marked the patch as "Changes Requested" in
patchwork. Please speak up if you think that's wrong.