[PATCH 0/1] pwm: meson: fix scheduling while atomic issue

[PATCH 0/1] pwm: meson: fix scheduling while atomic issue

[Martin Blumenstingl]

Back in January a "BUG: scheduling while atomic" error showed up during
boot on my Meson8b Odroid-C1 (which uses a PWM regulator as CPU supply).
The call trace comes down to:
  __mutex_lock
  clk_prepare_lock
  clk_core_get_rate
  meson_pwm_apply
  ..
  dev_pm_opp_set_rate
  ..

Jerome has also seen the same problem but from pwm-leds (instead of a
pwm-regulator). He posted a patch which replaces the spinlock with a
mutex. That works. I believe we can optimize this by reducing the time
where the lock is held - that also allows to keep the spin-lock.

Analyzing this issue helped me understand the pwm-meson driver better.
My plan is to send some cleanups (with the goal of re-using more of the
goodies from the PWM core in the pwm-meson driver) after this single fix
is merged (they can be found here: [1]).

Dependencies: none

Target version: please queue this for -fixes so it makes it's way into
v5.1-rc (so we can get it backported from there, because this issue has
existed since the pwm-meson driver was introduced).


[0] http://lists.infradead.org/pipermail/linux-amlogic/2019-January/009690.html
[1] https://github.com/xdarklight/linux/commits/meson-pwm-for-5.2-v0


Martin Blumenstingl (1):
  pwm: meson: use the spin-lock only to protect register modifications

 drivers/pwm/pwm-meson.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

-- 
2.21.0

[PATCH 1/1] pwm: meson: use the spin-lock only to protect register modifications

[Martin Blumenstingl]

Holding the spin-lock for all of the code in meson_pwm_apply() can
result in a "BUG: scheduling while atomic". This can happen because
clk_get_rate() (which is called from meson_pwm_calc()) may sleep.
Only hold the spin-lock when modifying registers to solve this.

The reason why we need a spin-lock in the driver is because the
REG_MISC_AB register is shared between the two channels provided by one
PWM controller. The only functions where REG_MISC_AB is modified are
meson_pwm_enable() and meson_pwm_disable() so the register reads/writes
in there need to be protected by the spin-lock.

The original code also used the spin-lock to protect the values in
struct meson_pwm_channel. This could be necessary if two consumers can
use the same PWM channel. However, PWM core doesn't allow this so we
don't need to protect the values in struct meson_pwm_channel with a
lock.

Fixes: 211ed630753d2f ("pwm: Add support for Meson PWM Controller")
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/pwm/pwm-meson.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index c1ed641b3e26..2b03938039b6 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -235,6 +235,7 @@ static void meson_pwm_enable(struct meson_pwm *meson,
 {
 	u32 value, clk_shift, clk_enable, enable;
 	unsigned int offset;
+	unsigned long flags;
 
 	switch (id) {
 	case 0:
@@ -255,6 +256,8 @@ static void meson_pwm_enable(struct meson_pwm *meson,
 		return;
 	}
 
+	spin_lock_irqsave(&meson->lock, flags);
+
 	value = readl(meson->base + REG_MISC_AB);
 	value &= ~(MISC_CLK_DIV_MASK << clk_shift);
 	value |= channel->pre_div << clk_shift;
@@ -267,11 +270,14 @@ static void meson_pwm_enable(struct meson_pwm *meson,
 	value = readl(meson->base + REG_MISC_AB);
 	value |= enable;
 	writel(value, meson->base + REG_MISC_AB);
+
+	spin_unlock_irqrestore(&meson->lock, flags);
 }
 
 static void meson_pwm_disable(struct meson_pwm *meson, unsigned int id)
 {
 	u32 value, enable;
+	unsigned long flags;
 
 	switch (id) {
 	case 0:
@@ -286,9 +292,13 @@ static void meson_pwm_disable(struct meson_pwm *meson, unsigned int id)
 		return;
 	}
 
+	spin_lock_irqsave(&meson->lock, flags);
+
 	value = readl(meson->base + REG_MISC_AB);
 	value &= ~enable;
 	writel(value, meson->base + REG_MISC_AB);
+
+	spin_unlock_irqrestore(&meson->lock, flags);
 }
 
 static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -296,19 +306,16 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 {
 	struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
 	struct meson_pwm *meson = to_meson_pwm(chip);
-	unsigned long flags;
 	int err = 0;
 
 	if (!state)
 		return -EINVAL;
 
-	spin_lock_irqsave(&meson->lock, flags);
-
 	if (!state->enabled) {
 		meson_pwm_disable(meson, pwm->hwpwm);
 		channel->state.enabled = false;
 
-		goto unlock;
+		return 0;
 	}
 
 	if (state->period != channel->state.period ||
@@ -329,7 +336,7 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		err = meson_pwm_calc(meson, channel, pwm->hwpwm,
 				     state->duty_cycle, state->period);
 		if (err < 0)
-			goto unlock;
+			return err;
 
 		channel->state.polarity = state->polarity;
 		channel->state.period = state->period;
@@ -341,9 +348,7 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		channel->state.enabled = true;
 	}
 
-unlock:
-	spin_unlock_irqrestore(&meson->lock, flags);
-	return err;
+	return 0;
 }
 
 static void meson_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
-- 
2.21.0

Re: [PATCH 1/1] pwm: meson: use the spin-lock only to protect register modifications

[Uwe Kleine-König]

Hello,

On Sun, Mar 24, 2019 at 11:02:17PM +0100, Martin Blumenstingl wrote:
> Holding the spin-lock for all of the code in meson_pwm_apply() can
> result in a "BUG: scheduling while atomic". This can happen because
> clk_get_rate() (which is called from meson_pwm_calc()) may sleep.
> Only hold the spin-lock when modifying registers to solve this.
> 
> The reason why we need a spin-lock in the driver is because the
> REG_MISC_AB register is shared between the two channels provided by one
> PWM controller. The only functions where REG_MISC_AB is modified are
> meson_pwm_enable() and meson_pwm_disable() so the register reads/writes
> in there need to be protected by the spin-lock.
> 
> The original code also used the spin-lock to protect the values in
> struct meson_pwm_channel. This could be necessary if two consumers can
> use the same PWM channel. However, PWM core doesn't allow this so we
> don't need to protect the values in struct meson_pwm_channel with a
> lock.
> 
> Fixes: 211ed630753d2f ("pwm: Add support for Meson PWM Controller")
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Looks right.

Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Bonus points for adding a comment to struct meson_pwm::lock. Something
like:

	/*
	 * Protects register access to the MISC_AB register that is
	 * shared between the two PWMs.
	 */

Best regards
Uwe

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

Re: [PATCH 0/1] pwm: meson: fix scheduling while atomic issue

[Uwe Kleine-König]

Hello Martin,

On Sun, Mar 24, 2019 at 11:02:16PM +0100, Martin Blumenstingl wrote:
> Back in January a "BUG: scheduling while atomic" error showed up during
> boot on my Meson8b Odroid-C1 (which uses a PWM regulator as CPU supply).
> The call trace comes down to:
>   __mutex_lock
>   clk_prepare_lock
>   clk_core_get_rate
>   meson_pwm_apply
>   ..
>   dev_pm_opp_set_rate
>   ..
> 
> Jerome has also seen the same problem but from pwm-leds (instead of a
> pwm-regulator). He posted a patch which replaces the spinlock with a
> mutex. That works. I believe we can optimize this by reducing the time
> where the lock is held - that also allows to keep the spin-lock.
> 
> Analyzing this issue helped me understand the pwm-meson driver better.
> My plan is to send some cleanups (with the goal of re-using more of the
> goodies from the PWM core in the pwm-meson driver) after this single fix
> is merged (they can be found here: [1]).

I didn't look over these in detail, but I see an issue that according
to the shortlogs isn't addressed: In the .apply callback there is
(simplified):

	if (!state->enabled) {
		meson_pwm_disable(meson, pwm->hwpwm);
		return;
	}

This results in the wrong output after:

	pwm_apply_state(pwm, { .enabled = true, .polarity = PWM_POLARITY_NORMAL, ...});
	pwm_apply_state(pwm, { .enabled = false, .polarity = PWM_POLARITY_INVERTED, ...});

because the polarity isn't checked.

If you want to implement further cleanups, my questions and propositions
are:

 - Is there a publicly available manual for this hardware? If yes, you
   can add a link to it in the header of the driver.

 - Why do you handle reparenting of the PWM's clk in .request? Wouldn't
   this be more suitable in .apply?

 - Does stopping the PWM (i.e. clearing MISC_{A,B}_EN in the MISC_AB
   register) freeze the output, or is the currently running period
   completed first? (The latter is the right behaviour.)

 - Please point out in the header that for changing period/duty
   cycle/polarity the hardware must be stopped. (I suggest to apply the
   style used in https://www.spinics.net/lists/linux-pwm/msg09262.html
   for some consistency.)

Best regards
Uwe

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

Re: [PATCH 0/1] pwm: meson: fix scheduling while atomic issue

[Uwe Kleine-König]

Hello,

On Mon, Mar 25, 2019 at 09:41:53AM +0100, Uwe Kleine-König wrote:
> If you want to implement further cleanups, my questions and propositions
> are:
> 
>  - Is there a publicly available manual for this hardware? If yes, you
>    can add a link to it in the header of the driver.
> 
>  - Why do you handle reparenting of the PWM's clk in .request? Wouldn't
>    this be more suitable in .apply?
> 
>  - Does stopping the PWM (i.e. clearing MISC_{A,B}_EN in the MISC_AB
>    register) freeze the output, or is the currently running period
>    completed first? (The latter is the right behaviour.)
> 
>  - Please point out in the header that for changing period/duty
>    cycle/polarity the hardware must be stopped. (I suggest to apply the
>    style used in https://www.spinics.net/lists/linux-pwm/msg09262.html
>    for some consistency.)

Another thing I just noted: The .get_state callback only sets .enabled
but nothing of the remaining information is provided.

Best regards
Uwe

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

Re: [PATCH 0/1] pwm: meson: fix scheduling while atomic issue

[Martin Blumenstingl]

Hello Uwe,

On Mon, Mar 25, 2019 at 9:41 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello Martin,
>
> On Sun, Mar 24, 2019 at 11:02:16PM +0100, Martin Blumenstingl wrote:
> > Back in January a "BUG: scheduling while atomic" error showed up during
> > boot on my Meson8b Odroid-C1 (which uses a PWM regulator as CPU supply).
> > The call trace comes down to:
> >   __mutex_lock
> >   clk_prepare_lock
> >   clk_core_get_rate
> >   meson_pwm_apply
> >   ..
> >   dev_pm_opp_set_rate
> >   ..
> >
> > Jerome has also seen the same problem but from pwm-leds (instead of a
> > pwm-regulator). He posted a patch which replaces the spinlock with a
> > mutex. That works. I believe we can optimize this by reducing the time
> > where the lock is held - that also allows to keep the spin-lock.
> >
> > Analyzing this issue helped me understand the pwm-meson driver better.
> > My plan is to send some cleanups (with the goal of re-using more of the
> > goodies from the PWM core in the pwm-meson driver) after this single fix
> > is merged (they can be found here: [1]).
>
> I didn't look over these in detail, but I see an issue that according
> to the shortlogs isn't addressed: In the .apply callback there is
> (simplified):
>
>         if (!state->enabled) {
>                 meson_pwm_disable(meson, pwm->hwpwm);
>                 return;
>         }
>
> This results in the wrong output after:
>
>         pwm_apply_state(pwm, { .enabled = true, .polarity = PWM_POLARITY_NORMAL, ...});
>         pwm_apply_state(pwm, { .enabled = false, .polarity = PWM_POLARITY_INVERTED, ...});
>
> because the polarity isn't checked.
let me rephrase this to make sure I understand this correctly:
- applying a PWM state with .enabled = true and PWM_POLARITY_NORMAL
will enable the PWM output
- applying a PWM state with .enabled = false and PWM_POLARITY_NORMAL
will disable the PWM output
- applying a PWM state with .enabled = true and PWM_POLARITY_INVERTED
will disable the PWM output
- applying a PWM state with .enabled = false and PWM_POLARITY_INVERTED
will enable the PWM output

in other words: the polarity doesn't only apply to period and
duty_cycle but also to the enabled state.

> If you want to implement further cleanups, my questions and propositions
> are:
>
>  - Is there a publicly available manual for this hardware? If yes, you
>    can add a link to it in the header of the driver.
yes, it's documented in the public S912 datasheet [0] page 542 and following
I'll add a patch which adds the link to the driver

>  - Why do you handle reparenting of the PWM's clk in .request? Wouldn't
>    this be more suitable in .apply?
Jerome's answer (not long after yours) basically covers this:
- the assigned-clock-parents property could have been used but it wasn't
- the driver could automatically set the correct parent, but this
isn't implemented

(I assume this was done to keep it short and simple to for the first
version of the driver)

>  - Does stopping the PWM (i.e. clearing MISC_{A,B}_EN in the MISC_AB
>    register) freeze the output, or is the currently running period
>    completed first? (The latter is the right behaviour.)
I don't know, I would have to measure this with a logic analyzer.
can you please explain why this is important?

>  - Please point out in the header that for changing period/duty
>    cycle/polarity the hardware must be stopped. (I suggest to apply the
>    style used in https://www.spinics.net/lists/linux-pwm/msg09262.html
>    for some consistency.)
I'm not sure about this. Amlogic's vendor kernel uses a modified
version of this driver [1] which has an explicit comment not to
disable the PWM output when changing the period/duty cycle.
the PWM is configured with two separate registers (PWM_MISC_REG_AB
contains the divider and PWM_PWM_A contains the high/low count).
there's a short timeframe where the PWM output signal is neither the
"old setting" nor the "new setting" (but rather a mix of both). what
do other PWM drivers do in this case (if this is a common thing)?

> Another thing I just noted: The .get_state callback only sets .enabled
> but nothing of the remaining information is provided.
as far as I can see the PWM core uses .get_state only during registration:
this means we should read (and calculate) .duty_cycle and .period from
the register values. polarity always has to be "normal" since there's
no information about it in the registers.


Regards
Martin


[0] https://dl.khadas.com/Hardware/VIM2/Datasheet/S912_Datasheet_V0.220170314publicversion-Wesion.pdf
[1] https://github.com/hardkernel/linux/blob/9a4a1f4b14fe66d7ebd73323b39fbf3bda9e1356/drivers/amlogic/pwm/pwm_meson.c#L381

Re: [PATCH 0/1] pwm: meson: fix scheduling while atomic issue

[Uwe Kleine-König]

Hello Martin,

On Mon, Mar 25, 2019 at 06:41:57PM +0100, Martin Blumenstingl wrote:
> On Mon, Mar 25, 2019 at 9:41 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Sun, Mar 24, 2019 at 11:02:16PM +0100, Martin Blumenstingl wrote:
> > > Analyzing this issue helped me understand the pwm-meson driver better.
> > > My plan is to send some cleanups (with the goal of re-using more of the
> > > goodies from the PWM core in the pwm-meson driver) after this single fix
> > > is merged (they can be found here: [1]).
> >
> > I didn't look over these in detail, but I see an issue that according
> > to the shortlogs isn't addressed: In the .apply callback there is
> > (simplified):
> >
> >         if (!state->enabled) {
> >                 meson_pwm_disable(meson, pwm->hwpwm);
> >                 return;
> >         }
> >
> > This results in the wrong output after:
> >
> >         pwm_apply_state(pwm, { .enabled = true, .polarity = PWM_POLARITY_NORMAL, ...});
> >         pwm_apply_state(pwm, { .enabled = false, .polarity = PWM_POLARITY_INVERTED, ...});
> >
> > because the polarity isn't checked.
> let me rephrase this to make sure I understand this correctly:
> - applying a PWM state with .enabled = true and PWM_POLARITY_NORMAL
> will enable the PWM output
> - applying a PWM state with .enabled = false and PWM_POLARITY_NORMAL
> will disable the PWM output
> - applying a PWM state with .enabled = true and PWM_POLARITY_INVERTED
> will disable the PWM output
> - applying a PWM state with .enabled = false and PWM_POLARITY_INVERTED
> will enable the PWM output
> 
> in other words: the polarity doesn't only apply to period and
> duty_cycle but also to the enabled state.

You're wrong (I think):

 - if .enabled = true, you should configure the output to repeat the
   following pattern: be active for $duty_cycle ns and then inactive for
   the rest of $period ns.

 - if .enabled = false, you should configure the output to be constant
   inactive.

 - if .polarity = PWM_POLARITY_NORMAL we have: inactive = low, active =
   high

 - if .polarity = PWM_POLARITY_INVERTED we have: inactive = high, active =
   low

So after the two pwm_apply_state above the expectation is that the
output is constant high. But as the meson driver's apply function
doesn't check for .polarity when .enabled = false is requested the
result is probably constant low. (Unless the driver is still more broken
and doesn't ensure the output gets inactive on .disable().)

> > If you want to implement further cleanups, my questions and propositions
> > are:
> >
> >  - Is there a publicly available manual for this hardware? If yes, you
> >    can add a link to it in the header of the driver.
> yes, it's documented in the public S912 datasheet [0] page 542 and following
> I'll add a patch which adds the link to the driver
> 
> >  - Why do you handle reparenting of the PWM's clk in .request? Wouldn't
> >    this be more suitable in .apply?
> Jerome's answer (not long after yours) basically covers this:
> - the assigned-clock-parents property could have been used but it wasn't
> - the driver could automatically set the correct parent, but this
> isn't implemented
> 
> (I assume this was done to keep it short and simple to for the first
> version of the driver)

I don't know how assigned-clock-parents works, but maybe it is even
simpler to use than the hardcoding that currently is used in the driver?
 
> >  - Does stopping the PWM (i.e. clearing MISC_{A,B}_EN in the MISC_AB
> >    register) freeze the output, or is the currently running period
> >    completed first? (The latter is the right behaviour.)
> I don't know, I would have to measure this with a logic analyzer.

In practise you can do this with a multimeter, too. Just do something
like:

	pwm_apply_state({ .enabled = true, .period = 5s, .duty_cycle = 5s, .polarity = PWM_POLARITY_NORMAL });
	pwm_apply_state({ .enabled = false, .period = 5s, .duty_cycle = 5s, .polarity = PWM_POLARITY_NORMAL });

(assuming the PWM supports periods that long). The expectation is that
the last command takes nearly 5 s to complete and while it waits the
output is high and on return it's low. If that isn't the case, there is
a bug somewhere.

> can you please explain why this is important?

Well, that's the semantic that the PWM API promises to its users.
Up to now this is poorly documented, there is an RFC patch waiting for
review that improves the situation.

> >  - Please point out in the header that for changing period/duty
> >    cycle/polarity the hardware must be stopped. (I suggest to apply the
> >    style used in https://www.spinics.net/lists/linux-pwm/msg09262.html
> >    for some consistency.)
> I'm not sure about this. Amlogic's vendor kernel uses a modified
> version of this driver [1] which has an explicit comment not to
> disable the PWM output when changing the period/duty cycle.

That would be better as stopping the driver also violates the API's
requirements. If this is not a hardware imposed limit, it would be great
to fix the driver accordingly.

> the PWM is configured with two separate registers (PWM_MISC_REG_AB
> contains the divider and PWM_PWM_A contains the high/low count).
> there's a short timeframe where the PWM output signal is neither the
> "old setting" nor the "new setting" (but rather a mix of both). what
> do other PWM drivers do in this case (if this is a common thing)?

If this cannot be prevented in hardware, I think this should be
documented clearly at the top of the driver. Up to now this kind of
problem isn't (TTBOMK) well tracked and questions like "How many drivers
suffer from $problem" cannot be easily answered.
(A bit unrelated: I think the violation that disabling a PWM doesn't
complete the currently running period is a common one. But because the
corresponding questions where not asked for new driver submissions until
recently, this is untracked and probably driver authors are not even
aware of this requirement.)

> > Another thing I just noted: The .get_state callback only sets .enabled
> > but nothing of the remaining information is provided.
> as far as I can see the PWM core uses .get_state only during registration:
> this means we should read (and calculate) .duty_cycle and .period from
> the register values. polarity always has to be "normal" since there's
> no information about it in the registers.

So you can change the polarity, but you cannot read that information
back? That's another item for the list of limitations.

Best regards
Uwe

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

Re: [PATCH 0/1] pwm: meson: fix scheduling while atomic issue

[Martin Blumenstingl]

Hello Uwe,

On Mon, Mar 25, 2019 at 9:07 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello Martin,
>
> On Mon, Mar 25, 2019 at 06:41:57PM +0100, Martin Blumenstingl wrote:
> > On Mon, Mar 25, 2019 at 9:41 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Sun, Mar 24, 2019 at 11:02:16PM +0100, Martin Blumenstingl wrote:
> > > > Analyzing this issue helped me understand the pwm-meson driver better.
> > > > My plan is to send some cleanups (with the goal of re-using more of the
> > > > goodies from the PWM core in the pwm-meson driver) after this single fix
> > > > is merged (they can be found here: [1]).
> > >
> > > I didn't look over these in detail, but I see an issue that according
> > > to the shortlogs isn't addressed: In the .apply callback there is
> > > (simplified):
> > >
> > >         if (!state->enabled) {
> > >                 meson_pwm_disable(meson, pwm->hwpwm);
> > >                 return;
> > >         }
> > >
> > > This results in the wrong output after:
> > >
> > >         pwm_apply_state(pwm, { .enabled = true, .polarity = PWM_POLARITY_NORMAL, ...});
> > >         pwm_apply_state(pwm, { .enabled = false, .polarity = PWM_POLARITY_INVERTED, ...});
> > >
> > > because the polarity isn't checked.
> > let me rephrase this to make sure I understand this correctly:
> > - applying a PWM state with .enabled = true and PWM_POLARITY_NORMAL
> > will enable the PWM output
> > - applying a PWM state with .enabled = false and PWM_POLARITY_NORMAL
> > will disable the PWM output
> > - applying a PWM state with .enabled = true and PWM_POLARITY_INVERTED
> > will disable the PWM output
> > - applying a PWM state with .enabled = false and PWM_POLARITY_INVERTED
> > will enable the PWM output
> >
> > in other words: the polarity doesn't only apply to period and
> > duty_cycle but also to the enabled state.
>
> You're wrong (I think):
>
>  - if .enabled = true, you should configure the output to repeat the
>    following pattern: be active for $duty_cycle ns and then inactive for
>    the rest of $period ns.
>
>  - if .enabled = false, you should configure the output to be constant
>    inactive.
>
>  - if .polarity = PWM_POLARITY_NORMAL we have: inactive = low, active =
>    high
>
>  - if .polarity = PWM_POLARITY_INVERTED we have: inactive = high, active =
>    low
thank you for explaining it again!

now I see what you mean that we're missing the case with
PWM_POLARITY_INVERTED and enabled = false:
* PWM_POLARITY_INVERTED/PWM_POLARITY_NORMAL and enabled = true are
managed in meson_pwm_calc()
* PWM_POLARITY_NORMAL and enabled = false is managed in meson_pwm_apply()
* logic for PWM_POLARITY_INVERTED and enabled = false is missing
(current result: same as PWM_POLARITY_NORMAL and enabled = false which
means: output is constant LOW, expected result: output is constant
HIGH)

> So after the two pwm_apply_state above the expectation is that the
> output is constant high. But as the meson driver's apply function
> doesn't check for .polarity when .enabled = false is requested the
> result is probably constant low. (Unless the driver is still more broken
> and doesn't ensure the output gets inactive on .disable().)
I will put this on my TODO-list for my cleanups

> > > If you want to implement further cleanups, my questions and propositions
> > > are:
> > >
> > >  - Is there a publicly available manual for this hardware? If yes, you
> > >    can add a link to it in the header of the driver.
> > yes, it's documented in the public S912 datasheet [0] page 542 and following
> > I'll add a patch which adds the link to the driver
> >
> > >  - Why do you handle reparenting of the PWM's clk in .request? Wouldn't
> > >    this be more suitable in .apply?
> > Jerome's answer (not long after yours) basically covers this:
> > - the assigned-clock-parents property could have been used but it wasn't
> > - the driver could automatically set the correct parent, but this
> > isn't implemented
> >
> > (I assume this was done to keep it short and simple to for the first
> > version of the driver)
>
> I don't know how assigned-clock-parents works, but maybe it is even
> simpler to use than the hardcoding that currently is used in the driver?
>
> > >  - Does stopping the PWM (i.e. clearing MISC_{A,B}_EN in the MISC_AB
> > >    register) freeze the output, or is the currently running period
> > >    completed first? (The latter is the right behaviour.)
> > I don't know, I would have to measure this with a logic analyzer.
>
> In practise you can do this with a multimeter, too. Just do something
> like:
>
>         pwm_apply_state({ .enabled = true, .period = 5s, .duty_cycle = 5s, .polarity = PWM_POLARITY_NORMAL });
>         pwm_apply_state({ .enabled = false, .period = 5s, .duty_cycle = 5s, .polarity = PWM_POLARITY_NORMAL });
>
> (assuming the PWM supports periods that long). The expectation is that
> the last command takes nearly 5 s to complete and while it waits the
> output is high and on return it's low. If that isn't the case, there is
> a bug somewhere.
thank you for explaining the test-case!
I'll do the maths on the weekend and determine the longest supported
period - then I'll measure this with a multimeter.

> > can you please explain why this is important?
>
> Well, that's the semantic that the PWM API promises to its users.
> Up to now this is poorly documented, there is an RFC patch waiting for
> review that improves the situation.
that makes sense, thank you for the explanation.

> > >  - Please point out in the header that for changing period/duty
> > >    cycle/polarity the hardware must be stopped. (I suggest to apply the
> > >    style used in https://www.spinics.net/lists/linux-pwm/msg09262.html
> > >    for some consistency.)
> > I'm not sure about this. Amlogic's vendor kernel uses a modified
> > version of this driver [1] which has an explicit comment not to
> > disable the PWM output when changing the period/duty cycle.
>
> That would be better as stopping the driver also violates the API's
> requirements. If this is not a hardware imposed limit, it would be great
> to fix the driver accordingly.
I found out that Bichao Zheng from Amlogic changed the PWM driver in
the vendor kernel. He even provided useful details in the commit
message (which I'll quote here for archive purposes): [0]
  pwm: meson: don't disable pwm when setting duty repeatedly

  There is an abnormally low about 20ms,when setting duty repeatedly.
  Because setting the duty will disable pwm and then enable.Delete
  this operation now.

> > the PWM is configured with two separate registers (PWM_MISC_REG_AB
> > contains the divider and PWM_PWM_A contains the high/low count).
> > there's a short timeframe where the PWM output signal is neither the
> > "old setting" nor the "new setting" (but rather a mix of both). what
> > do other PWM drivers do in this case (if this is a common thing)?
>
> If this cannot be prevented in hardware, I think this should be
> documented clearly at the top of the driver. Up to now this kind of
> problem isn't (TTBOMK) well tracked and questions like "How many drivers
> suffer from $problem" cannot be easily answered.
> (A bit unrelated: I think the violation that disabling a PWM doesn't
> complete the currently running period is a common one. But because the
> corresponding questions where not asked for new driver submissions until
> recently, this is untracked and probably driver authors are not even
> aware of this requirement.)
I will port Bichao Zheng's commit to the mainline driver and try it on
one of my 32-bit Meson boards (where a PWM controlled regulator
supplies the CPU cores) and one my 64-bit Meson boards (where a PWM
clock is used for the 32.768kHz LPO clock for the Wifi chipset).

I'm not sure if we still have a small timeframe where the clock
divider gets reconfigured.
However, I'm not sure if that's an actual problem for the use-cases
that we have on the boards with Amlogic Meson SoCs:
- maybe the controller is smart enough to read the clock frequency
only when starting the PWM
- for the pwm-regulator and pwm-clock use-cases we only configure the
period once. thus we always get the same clock divider internally, so
we don't run into issues
(I still think that adding documentation for the actual behavior is
good, but maybe we don't need a "fix" right now because it probably
works fine anyways for our current use-cases)

> > > Another thing I just noted: The .get_state callback only sets .enabled
> > > but nothing of the remaining information is provided.
> > as far as I can see the PWM core uses .get_state only during registration:
> > this means we should read (and calculate) .duty_cycle and .period from
> > the register values. polarity always has to be "normal" since there's
> > no information about it in the registers.
>
> So you can change the polarity, but you cannot read that information
> back? That's another item for the list of limitations.
OK, noted
(as Neil explained we swap the low/high duration in software, the
hardware doesn't know about this)


Regards
Martin


[0] https://github.com/hardkernel/linux/commit/11573cdb34ec790c3dd6f860442be4f7b4d651d5#diff-e432928020d0ce66e5bef757ba2c6f36

Re: [PATCH 0/1] pwm: meson: fix scheduling while atomic issue

[Martin Blumenstingl]

Hello Uwe,

On Mon, Mar 25, 2019 at 9:07 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
[...]
> > >  - Does stopping the PWM (i.e. clearing MISC_{A,B}_EN in the MISC_AB
> > >    register) freeze the output, or is the currently running period
> > >    completed first? (The latter is the right behaviour.)
> > I don't know, I would have to measure this with a logic analyzer.
>
> In practise you can do this with a multimeter, too. Just do something
> like:
>
>         pwm_apply_state({ .enabled = true, .period = 5s, .duty_cycle = 5s, .polarity = PWM_POLARITY_NORMAL });
>         pwm_apply_state({ .enabled = false, .period = 5s, .duty_cycle = 5s, .polarity = PWM_POLARITY_NORMAL });
>
> (assuming the PWM supports periods that long). The expectation is that
> the last command takes nearly 5 s to complete and while it waits the
> output is high and on return it's low. If that isn't the case, there is
> a bug somewhere.
the longest supported period (using the 24MHz crystal as input, which
is the slowest input clock and thus gives the longest possible
duration) is 349514407ns (that's approx. 0.35 seconds). my multimeter
isn't fast enough to measure this so I'm using my logic analyzer with
puleseview instead: [0]

I added the following code to meson_pwm_request:
  struct pwm_state enable = {
        .enabled = true,
        .period = 349514407U,
        .duty_cycle = 349514407U,
        .polarity = PWM_POLARITY_NORMAL };
  struct pwm_state disable = {
        .enabled = false,
        .period = 349514407U,
        .duty_cycle = 349514407U,
        .polarity = PWM_POLARITY_NORMAL };
  pwm_apply_state(pwm, &enable);
  pwm_apply_state(pwm, &disable);

this returns immediately. my logic analyzer doesn't see signal change
(I'm sampling at 1MHz).

can you please confirm that my test code and measurement procedure is correct?
if it is then my observation is that disabling the PWM does so
immediately, without waiting for the current period to complete


Regards
Martin


[0] https://sigrok.org/wiki/Lcsoft_Mini_Board

Re: [PATCH 0/1] pwm: meson: fix scheduling while atomic issue

[Uwe Kleine-König]

On Sat, Mar 30, 2019 at 08:29:35PM +0100, Martin Blumenstingl wrote:
> Hello Uwe,
> 
> On Mon, Mar 25, 2019 at 9:07 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> [...]
> > > >  - Does stopping the PWM (i.e. clearing MISC_{A,B}_EN in the MISC_AB
> > > >    register) freeze the output, or is the currently running period
> > > >    completed first? (The latter is the right behaviour.)
> > > I don't know, I would have to measure this with a logic analyzer.
> >
> > In practise you can do this with a multimeter, too. Just do something
> > like:
> >
> >         pwm_apply_state({ .enabled = true, .period = 5s, .duty_cycle = 5s, .polarity = PWM_POLARITY_NORMAL });
> >         pwm_apply_state({ .enabled = false, .period = 5s, .duty_cycle = 5s, .polarity = PWM_POLARITY_NORMAL });
> >
> > (assuming the PWM supports periods that long). The expectation is that
> > the last command takes nearly 5 s to complete and while it waits the
> > output is high and on return it's low. If that isn't the case, there is
> > a bug somewhere.
> the longest supported period (using the 24MHz crystal as input, which
> is the slowest input clock and thus gives the longest possible
> duration) is 349514407ns (that's approx. 0.35 seconds). my multimeter
> isn't fast enough to measure this so I'm using my logic analyzer with
> puleseview instead: [0]
> 
> I added the following code to meson_pwm_request:
>   struct pwm_state enable = {
>         .enabled = true,
>         .period = 349514407U,
>         .duty_cycle = 349514407U,
>         .polarity = PWM_POLARITY_NORMAL };
>   struct pwm_state disable = {
>         .enabled = false,
>         .period = 349514407U,
>         .duty_cycle = 349514407U,
>         .polarity = PWM_POLARITY_NORMAL };
>   pwm_apply_state(pwm, &enable);
>   pwm_apply_state(pwm, &disable);
> 
> this returns immediately. my logic analyzer doesn't see signal change
> (I'm sampling at 1MHz).
> 
> can you please confirm that my test code and measurement procedure is correct?
> if it is then my observation is that disabling the PWM does so
> immediately, without waiting for the current period to complete

Ack, with the above two pwm_apply_state the output must be high when the
first pwm_apply_state returns. Then it must stay high for n *
349514407 ns (for an natural n >= 1) and then go low.

Best regards
Uwe

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

Re: [PATCH 0/1] pwm: meson: fix scheduling while atomic issue

[Neil Armstrong]

On 30/03/2019 20:29, Martin Blumenstingl wrote:
> Hello Uwe,
> 
> On Mon, Mar 25, 2019 at 9:07 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> [...]
>>>>  - Does stopping the PWM (i.e. clearing MISC_{A,B}_EN in the MISC_AB
>>>>    register) freeze the output, or is the currently running period
>>>>    completed first? (The latter is the right behaviour.)
>>> I don't know, I would have to measure this with a logic analyzer.
>>
>> In practise you can do this with a multimeter, too. Just do something
>> like:
>>
>>         pwm_apply_state({ .enabled = true, .period = 5s, .duty_cycle = 5s, .polarity = PWM_POLARITY_NORMAL });
>>         pwm_apply_state({ .enabled = false, .period = 5s, .duty_cycle = 5s, .polarity = PWM_POLARITY_NORMAL });
>>
>> (assuming the PWM supports periods that long). The expectation is that
>> the last command takes nearly 5 s to complete and while it waits the
>> output is high and on return it's low. If that isn't the case, there is
>> a bug somewhere.
> the longest supported period (using the 24MHz crystal as input, which
> is the slowest input clock and thus gives the longest possible
> duration) is 349514407ns (that's approx. 0.35 seconds). my multimeter
> isn't fast enough to measure this so I'm using my logic analyzer with
> puleseview instead: [0]
> 
> I added the following code to meson_pwm_request:
>   struct pwm_state enable = {
>         .enabled = true,
>         .period = 349514407U,
>         .duty_cycle = 349514407U,
>         .polarity = PWM_POLARITY_NORMAL };
>   struct pwm_state disable = {
>         .enabled = false,
>         .period = 349514407U,
>         .duty_cycle = 349514407U,
>         .polarity = PWM_POLARITY_NORMAL };
>   pwm_apply_state(pwm, &enable);
>   pwm_apply_state(pwm, &disable);
> 
> this returns immediately. my logic analyzer doesn't see signal change
> (I'm sampling at 1MHz).
> 
> can you please confirm that my test code and measurement procedure is correct?
> if it is then my observation is that disabling the PWM does so
> immediately, without waiting for the current period to complete

I'm pretty 100,00000000000000% sure the HW doesn't permit waiting for a period to finish.

For disable states, we can either play on the period (high period at 0xffff for disable
at PWM_POLARITY_INVERSED and high period at 0 for PWM_POLARITY_NORMAL, if the HW behaves
correctly) or by adding some pinctrl states switching to GPIO modes and adding the
enable output high and output low property.

Neil


> 
> 
> Regards
> Martin
> 
> 
> [0] https://sigrok.org/wiki/Lcsoft_Mini_Board
> 

Re: [PATCH 0/1] pwm: meson: fix scheduling while atomic issue

[Neil Armstrong]

On 25/03/2019 18:41, Martin Blumenstingl wrote:
> Hello Uwe,
> 
> On Mon, Mar 25, 2019 at 9:41 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
>>
>> Hello Martin,
>>
>> On Sun, Mar 24, 2019 at 11:02:16PM +0100, Martin Blumenstingl wrote:
>>> Back in January a "BUG: scheduling while atomic" error showed up during
>>> boot on my Meson8b Odroid-C1 (which uses a PWM regulator as CPU supply).
>>> The call trace comes down to:
>>>   __mutex_lock
>>>   clk_prepare_lock
>>>   clk_core_get_rate
>>>   meson_pwm_apply
>>>   ..
>>>   dev_pm_opp_set_rate
>>>   ..
>>>
>>> Jerome has also seen the same problem but from pwm-leds (instead of a
>>> pwm-regulator). He posted a patch which replaces the spinlock with a
>>> mutex. That works. I believe we can optimize this by reducing the time
>>> where the lock is held - that also allows to keep the spin-lock.
>>>
>>> Analyzing this issue helped me understand the pwm-meson driver better.
>>> My plan is to send some cleanups (with the goal of re-using more of the
>>> goodies from the PWM core in the pwm-meson driver) after this single fix
>>> is merged (they can be found here: [1]).
>>
>> I didn't look over these in detail, but I see an issue that according
>> to the shortlogs isn't addressed: In the .apply callback there is
>> (simplified):
>>
>>         if (!state->enabled) {
>>                 meson_pwm_disable(meson, pwm->hwpwm);
>>                 return;
>>         }
>>
>> This results in the wrong output after:
>>
>>         pwm_apply_state(pwm, { .enabled = true, .polarity = PWM_POLARITY_NORMAL, ...});
>>         pwm_apply_state(pwm, { .enabled = false, .polarity = PWM_POLARITY_INVERTED, ...});
>>
>> because the polarity isn't checked.
> let me rephrase this to make sure I understand this correctly:
> - applying a PWM state with .enabled = true and PWM_POLARITY_NORMAL
> will enable the PWM output
> - applying a PWM state with .enabled = false and PWM_POLARITY_NORMAL
> will disable the PWM output
> - applying a PWM state with .enabled = true and PWM_POLARITY_INVERTED
> will disable the PWM output
> - applying a PWM state with .enabled = false and PWM_POLARITY_INVERTED
> will enable the PWM output
> 
> in other words: the polarity doesn't only apply to period and
> duty_cycle but also to the enabled state.

Sorry I don't understand your point.
If the apply state is disable, well we disable the PWM output, I don't see why
the polarity changes the enable state.

I'd like to point out the architecture of the PWM.
The PWM is only a set of Gates, Dividers and Counters.

We achieve a PWM output by calculating a clock that permits us to calculate
2 periods (low and high) and we set the counter to switch after N cycles
for the first half period.

We do not have an explicit "polarity" setting, we simply reverse the period
cycles (the low length is inversed with the high length).

To apply the dividers and counters, we need to disable the PWM input clock
gate, set the dividers and counter and release the input gate.

So yes, we should maybe stop disabling the PWM for a long period of time
when we calculate the new settings, it can be fixed easily by calculating
the settings and applying in a separate code path.

But while re-reading the code, I can't find the issue you point at.

Neil

> 
>> If you want to implement further cleanups, my questions and propositions
>> are:
>>
>>  - Is there a publicly available manual for this hardware? If yes, you
>>    can add a link to it in the header of the driver.
> yes, it's documented in the public S912 datasheet [0] page 542 and following
> I'll add a patch which adds the link to the driver
> 
>>  - Why do you handle reparenting of the PWM's clk in .request? Wouldn't
>>    this be more suitable in .apply?
> Jerome's answer (not long after yours) basically covers this:
> - the assigned-clock-parents property could have been used but it wasn't
> - the driver could automatically set the correct parent, but this
> isn't implemented
> 
> (I assume this was done to keep it short and simple to for the first
> version of the driver)
> 
>>  - Does stopping the PWM (i.e. clearing MISC_{A,B}_EN in the MISC_AB
>>    register) freeze the output, or is the currently running period
>>    completed first? (The latter is the right behaviour.)
> I don't know, I would have to measure this with a logic analyzer.
> can you please explain why this is important?
> 
>>  - Please point out in the header that for changing period/duty
>>    cycle/polarity the hardware must be stopped. (I suggest to apply the
>>    style used in https://www.spinics.net/lists/linux-pwm/msg09262.html
>>    for some consistency.)
> I'm not sure about this. Amlogic's vendor kernel uses a modified
> version of this driver [1] which has an explicit comment not to
> disable the PWM output when changing the period/duty cycle.
> the PWM is configured with two separate registers (PWM_MISC_REG_AB
> contains the divider and PWM_PWM_A contains the high/low count).
> there's a short timeframe where the PWM output signal is neither the
> "old setting" nor the "new setting" (but rather a mix of both). what
> do other PWM drivers do in this case (if this is a common thing)?
> 
>> Another thing I just noted: The .get_state callback only sets .enabled
>> but nothing of the remaining information is provided.
> as far as I can see the PWM core uses .get_state only during registration:
> this means we should read (and calculate) .duty_cycle and .period from
> the register values. polarity always has to be "normal" since there's
> no information about it in the registers.
> 
> 
> Regards
> Martin
> 
> 
> [0] https://dl.khadas.com/Hardware/VIM2/Datasheet/S912_Datasheet_V0.220170314publicversion-Wesion.pdf
> [1] https://github.com/hardkernel/linux/blob/9a4a1f4b14fe66d7ebd73323b39fbf3bda9e1356/drivers/amlogic/pwm/pwm_meson.c#L381
> 

Re: [PATCH 0/1] pwm: meson: fix scheduling while atomic issue

[Uwe Kleine-König]

Hello,

On Tue, Mar 26, 2019 at 10:06:31AM +0100, Neil Armstrong wrote:
> On 25/03/2019 18:41, Martin Blumenstingl wrote:
> > On Mon, Mar 25, 2019 at 9:41 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> >> On Sun, Mar 24, 2019 at 11:02:16PM +0100, Martin Blumenstingl wrote:
> >>> Back in January a "BUG: scheduling while atomic" error showed up during
> >>> boot on my Meson8b Odroid-C1 (which uses a PWM regulator as CPU supply).
> >>> The call trace comes down to:
> >>>   __mutex_lock
> >>>   clk_prepare_lock
> >>>   clk_core_get_rate
> >>>   meson_pwm_apply
> >>>   ..
> >>>   dev_pm_opp_set_rate
> >>>   ..
> >>>
> >>> Jerome has also seen the same problem but from pwm-leds (instead of a
> >>> pwm-regulator). He posted a patch which replaces the spinlock with a
> >>> mutex. That works. I believe we can optimize this by reducing the time
> >>> where the lock is held - that also allows to keep the spin-lock.
> >>>
> >>> Analyzing this issue helped me understand the pwm-meson driver better.
> >>> My plan is to send some cleanups (with the goal of re-using more of the
> >>> goodies from the PWM core in the pwm-meson driver) after this single fix
> >>> is merged (they can be found here: [1]).
> >>
> >> I didn't look over these in detail, but I see an issue that according
> >> to the shortlogs isn't addressed: In the .apply callback there is
> >> (simplified):
> >>
> >>         if (!state->enabled) {
> >>                 meson_pwm_disable(meson, pwm->hwpwm);
> >>                 return;
> >>         }
> >>
> >> This results in the wrong output after:
> >>
> >>         pwm_apply_state(pwm, { .enabled = true, .polarity = PWM_POLARITY_NORMAL, ...});
> >>         pwm_apply_state(pwm, { .enabled = false, .polarity = PWM_POLARITY_INVERTED, ...});
> >>
> >> because the polarity isn't checked.
> > let me rephrase this to make sure I understand this correctly:
> > - applying a PWM state with .enabled = true and PWM_POLARITY_NORMAL
> > will enable the PWM output
> > - applying a PWM state with .enabled = false and PWM_POLARITY_NORMAL
> > will disable the PWM output
> > - applying a PWM state with .enabled = true and PWM_POLARITY_INVERTED
> > will disable the PWM output
> > - applying a PWM state with .enabled = false and PWM_POLARITY_INVERTED
> > will enable the PWM output
> > 
> > in other words: the polarity doesn't only apply to period and
> > duty_cycle but also to the enabled state.
> 
> Sorry I don't understand your point.
> If the apply state is disable, well we disable the PWM output, I don't see why
> the polarity changes the enable state.

Martin's summary was at least misleading, I didn't understand what he
meant.

The relevant point is: When the PWM is disabled (either by pwm_disable
or equivalent by pwm_apply_state(pwm, { .enabled = false, ... })) the
expectation is that the output becomes "inactive". That means "constant
low" for a normal PWM and "constant high" for an inverted PWM.

Then as meson_pwm_apply doesn't check for state->polarity if
state->enabled == false there is a bug.

> I'd like to point out the architecture of the PWM.
> The PWM is only a set of Gates, Dividers and Counters.
> 
> We achieve a PWM output by calculating a clock that permits us to calculate
> 2 periods (low and high) and we set the counter to switch after N cycles
> for the first half period.
> 
> We do not have an explicit "polarity" setting, we simply reverse the period
> cycles (the low length is inversed with the high length).
> 
> To apply the dividers and counters, we need to disable the PWM input clock
> gate, set the dividers and counter and release the input gate.
> 
> So yes, we should maybe stop disabling the PWM for a long period of time
> when we calculate the new settings, it can be fixed easily by calculating
> the settings and applying in a separate code path.

If the hardware supports it the counter should not be stopped---most
other PWMs in my bubble can at least change the duty cycle as required.
(That is, complete the currently running period and that without delay
start a new period with the new settings.)
 
Best regards
Uwe

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

Re: [PATCH 0/1] pwm: meson: fix scheduling while atomic issue

[Jerome Brunet]

On Sun, 2019-03-24 at 23:02 +0100, Martin Blumenstingl wrote:
> Back in January a "BUG: scheduling while atomic" error showed up during
> boot on my Meson8b Odroid-C1 (which uses a PWM regulator as CPU supply).
> The call trace comes down to:
>   __mutex_lock
>   clk_prepare_lock
>   clk_core_get_rate
>   meson_pwm_apply
>   ..
>   dev_pm_opp_set_rate
>   ..
> 
> Jerome has also seen the same problem but from pwm-leds (instead of a
> pwm-regulator). He posted a patch which replaces the spinlock with a
> mutex. That works. I believe we can optimize this by reducing the time
> where the lock is held - that also allows to keep the spin-lock.
> 
> Analyzing this issue helped me understand the pwm-meson driver better.
> My plan is to send some cleanups (with the goal of re-using more of the
> goodies from the PWM core in the pwm-meson driver) after this single fix
> is merged (they can be found here: [1]).

Thanks for fixing this Martin.

As for the future enhancement, I'd like to know what you have in mind.
As I have told you previously, I think the clock bindings of this driver are
not great.

The global name of the input clocks are hard coded in this driver and it
sucks. CCF is evolving to rely less on these global names.

In addition, the 'clock' binding should be used to refer to the clock
'consumed' by the device, not to define a setting (as done now). 'assigned-
clock' binding can be used for that.

This would be a significant change in the binding meaning of this driver,
which probably calls for a v2.

Last, instead of specifying the parent to be used, I think we should come up
with some code to let the driver pick the most appropriate parent for the period/duty requested.

> 
> Dependencies: none
> 
> Target version: please queue this for -fixes so it makes it's way into
> v5.1-rc (so we can get it backported from there, because this issue has
> existed since the pwm-meson driver was introduced).
> 
> 
> [0] http://lists.infradead.org/pipermail/linux-amlogic/2019-January/009690.html
> [1] https://github.com/xdarklight/linux/commits/meson-pwm-for-5.2-v0
> 
> 
> Martin Blumenstingl (1):
>   pwm: meson: use the spin-lock only to protect register modifications
> 
>  drivers/pwm/pwm-meson.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 

Re: [PATCH 0/1] pwm: meson: fix scheduling while atomic issue

[Martin Blumenstingl]

Hi Jerome,

On Mon, Mar 25, 2019 at 10:35 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
>
> On Sun, 2019-03-24 at 23:02 +0100, Martin Blumenstingl wrote:
> > Back in January a "BUG: scheduling while atomic" error showed up during
> > boot on my Meson8b Odroid-C1 (which uses a PWM regulator as CPU supply).
> > The call trace comes down to:
> >   __mutex_lock
> >   clk_prepare_lock
> >   clk_core_get_rate
> >   meson_pwm_apply
> >   ..
> >   dev_pm_opp_set_rate
> >   ..
> >
> > Jerome has also seen the same problem but from pwm-leds (instead of a
> > pwm-regulator). He posted a patch which replaces the spinlock with a
> > mutex. That works. I believe we can optimize this by reducing the time
> > where the lock is held - that also allows to keep the spin-lock.
> >
> > Analyzing this issue helped me understand the pwm-meson driver better.
> > My plan is to send some cleanups (with the goal of re-using more of the
> > goodies from the PWM core in the pwm-meson driver) after this single fix
> > is merged (they can be found here: [1]).
>
> Thanks for fixing this Martin.
you're welcome!

> As for the future enhancement, I'd like to know what you have in mind.
> As I have told you previously, I think the clock bindings of this driver are
> not great.
>
> The global name of the input clocks are hard coded in this driver and it
> sucks. CCF is evolving to rely less on these global names.
I fully agree with you on the clock setup, but I'm not sure if we have
to break the dt-bindings for it.

the datasheet notes: "Each PWM is driven by a programmable divider
driven by a 4:1 clock selector".
In my own words this means that each PWM controller has up to 8 clock inputs:
- up to 4 inputs for the first channel (PWM_A)
- up to 4 inputs for the second channel (PWM_B)

the current pwm-meson driver assumes that both the inputs for both
channels are identical.
the "clock trees" section of the public S912 datasheet (page 65)
clearly documents the clock inputs per PWM channel, not per PWM
controller.

Thus I believe we should name our clock-names (the inputs to the PWM
controller) "pwm-a-clkin0", "pwm-a-clkin1", "pwm-b-clkin0", ...
That way we don't have a conflict with the existing bindings (which
already reserve "clkin0" and "clkin1").

> In addition, the 'clock' binding should be used to refer to the clock
> 'consumed' by the device, not to define a setting (as done now). 'assigned-
> clock' binding can be used for that.
using the assigned-clock* properties requires self-referencing the PWM
controller (which I'm not used to), for example:
  &pwm_ab {
      #clock-cells = <1>;
      assigned-clocks = <&pwm_ab 0>, <&pwm_ab 1>; /* references itself */
      assigned-clock-parents = <&xtal>, <&clkc CLKID_FCLK_DIV5>;
  };

if we want to auto-detect the parent clock (like you suggested below)
we need to consider if we can detect whether a .dts-author assigned a
specific parent.
I know that it's easy to detect this when the clock is passed in the
"clocks" property, but I'm not sure if it's easy to parse it from the
assigned-clocks/assigned-clock-parents properties.

[...]
> Last, instead of specifying the parent to be used, I think we should come up
> with some code to let the driver pick the most appropriate parent for the period/duty requested.
that will make it easier for .dts authors.
I would like to postpone this until we have solved the other topics though.


Regards
Martin


[0] https://dl.khadas.com/Hardware/VIM2/Datasheet/S912_Datasheet_V0.220170314publicversion-Wesion.pdf

Re: [PATCH 0/1] pwm: meson: fix scheduling while atomic issue

[Jerome Brunet]

On Mon, 2019-03-25 at 19:04 +0100, Martin Blumenstingl wrote:
> > Thanks for fixing this Martin.
> you're welcome!
> 
> > As for the future enhancement, I'd like to know what you have in mind.
> > As I have told you previously, I think the clock bindings of this driver are
> > not great.
> > 
> > The global name of the input clocks are hard coded in this driver and it
> > sucks. CCF is evolving to rely less on these global names.
> I fully agree with you on the clock setup, but I'm not sure if we have
> to break the dt-bindings for it.
> 
> the datasheet notes: "Each PWM is driven by a programmable divider
> driven by a 4:1 clock selector".
> In my own words this means that each PWM controller has up to 8 clock inputs:
> - up to 4 inputs for the first channel (PWM_A)
> - up to 4 inputs for the second channel (PWM_B)

Not from the pwm device POV. there is one device (PWM_AB) with 4 (max) input
clocks. Those are consumed by two internal muxes. There would be 8 if the
input was different between path A and B.

> 
> the current pwm-meson driver assumes that both the inputs for both
> channels are identical.
> the "clock trees" section of the public S912 datasheet (page 65)
> clearly documents the clock inputs per PWM channel, not per PWM
> controller.
> 
> Thus I believe we should name our clock-names (the inputs to the PWM
> controller) "pwm-a-clkin0", "pwm-a-clkin1", "pwm-b-clkin0", ...
> That way we don't have a conflict with the existing bindings (which
> already reserve "clkin0" and "clkin1").

I think this is overkill an inaccurate. The experience of all the soc we have
seen so far (meson8, gxbb, gxl, gxm, axg and g12) confirms the sources the are
the same input clock for both path.

The documentation just shows the clock src of each pwm. That just how the the
table is presented. That does not change the fact the pwms are organized in
modules (pairs) and the that the clock source are the same for each pwm of the
module. IOW, there is only 4 lines of clocks getting to the IP, not 8. Feel
free to ask amlogic if you want to make sure.

The name clash is not really my point. The purpose of the clock binding would
be different (from stating a setting to describing hw connection)

> 
> > In addition, the 'clock' binding should be used to refer to the clock
> > 'consumed' by the device, not to define a setting (as done now). 'assigned-
> > clock' binding can be used for that.
> using the assigned-clock* properties requires self-referencing the PWM
> controller (which I'm not used to), for example:
>   &pwm_ab {
>       #clock-cells = <1>;
>       assigned-clocks = <&pwm_ab 0>, <&pwm_ab 1>; /* references itself */
>       assigned-clock-parents = <&xtal>, <&clkc CLKID_FCLK_DIV5>;
>   };
> 
> if we want to auto-detect the parent clock (like you suggested below)
> we need to consider if we can detect whether a .dts-author assigned a
> specific parent.

I (personally) don't want to keep supporting the manual assignment of the
parent. If the driver can guarantee than it will pick the most appropriate
parent, there is no reason to have that.

> I know that it's easy to detect this when the clock is passed in the
> "clocks" property, but I'm not sure if it's easy to parse it from the
> assigned-clocks/assigned-clock-parents properties.

Assigned parent is the poor man solution and not necessarily easier to
implement (the pwm device would have to export its own clocks) ... I have just
mentioned it to make  the point that current method is not ideal

> 
> [...]
> > Last, instead of specifying the parent to be used, I think we should come up
> > with some code to let the driver pick the most appropriate parent for the period/duty requested.
> that will make it easier for .dts authors.
> I would like to postpone this until we have solved the other topics though.

I much prefer this last solution. Since the algorithm and the bindings would
change, I think it would be easier (in DT) to just make v2 driver with a new
compatible, progressively transition dts to it and finally remove the old
driver.

Re: [PATCH 0/1] pwm: meson: fix scheduling while atomic issue

[Neil Armstrong]

On 26/03/2019 09:37, Jerome Brunet wrote:
> On Mon, 2019-03-25 at 19:04 +0100, Martin Blumenstingl wrote:
>>> Thanks for fixing this Martin.
>> you're welcome!
>>
>>> As for the future enhancement, I'd like to know what you have in mind.
>>> As I have told you previously, I think the clock bindings of this driver are
>>> not great.
>>>
>>> The global name of the input clocks are hard coded in this driver and it
>>> sucks. CCF is evolving to rely less on these global names.
>> I fully agree with you on the clock setup, but I'm not sure if we have
>> to break the dt-bindings for it.
>>
>> the datasheet notes: "Each PWM is driven by a programmable divider
>> driven by a 4:1 clock selector".
>> In my own words this means that each PWM controller has up to 8 clock inputs:
>> - up to 4 inputs for the first channel (PWM_A)
>> - up to 4 inputs for the second channel (PWM_B)
> 
> Not from the pwm device POV. there is one device (PWM_AB) with 4 (max) input
> clocks. Those are consumed by two internal muxes. There would be 8 if the
> input was different between path A and B.

The PWM pair is a imple duplicate of a PWM HW, sharing the same clock parents,
as he driver is already designed, you can only have (for now) only
4 possible parents per pair.

It may change in the future, but for SoC from Meson8 to G12B, it's how it's
designed _now_, no need to discuss an eventual future change.

> 
>>
>> the current pwm-meson driver assumes that both the inputs for both
>> channels are identical.
>> the "clock trees" section of the public S912 datasheet (page 65)
>> clearly documents the clock inputs per PWM channel, not per PWM
>> controller.
>>
>> Thus I believe we should name our clock-names (the inputs to the PWM
>> controller) "pwm-a-clkin0", "pwm-a-clkin1", "pwm-b-clkin0", ...
>> That way we don't have a conflict with the existing bindings (which
>> already reserve "clkin0" and "clkin1").
> 
> I think this is overkill an inaccurate. The experience of all the soc we have
> seen so far (meson8, gxbb, gxl, gxm, axg and g12) confirms the sources the are
> the same input clock for both path.
> 
> The documentation just shows the clock src of each pwm. That just how the the
> table is presented. That does not change the fact the pwms are organized in
> modules (pairs) and the that the clock source are the same for each pwm of the
> module. IOW, there is only 4 lines of clocks getting to the IP, not 8. Feel
> free to ask amlogic if you want to make sure.

Indeed, the possible parents for each pair on PWM is the same, so we would only
need 4 input clocks per PWM pair.

> 
> The name clash is not really my point. The purpose of the clock binding would
> be different (from stating a setting to describing hw connection)
> 
>>
>>> In addition, the 'clock' binding should be used to refer to the clock
>>> 'consumed' by the device, not to define a setting (as done now). 'assigned-
>>> clock' binding can be used for that.
>> using the assigned-clock* properties requires self-referencing the PWM
>> controller (which I'm not used to), for example:
>>   &pwm_ab {
>>       #clock-cells = <1>;
>>       assigned-clocks = <&pwm_ab 0>, <&pwm_ab 1>; /* references itself */
>>       assigned-clock-parents = <&xtal>, <&clkc CLKID_FCLK_DIV5>;
>>   };
>>
>> if we want to auto-detect the parent clock (like you suggested below)
>> we need to consider if we can detect whether a .dts-author assigned a
>> specific parent.
> 
> I (personally) don't want to keep supporting the manual assignment of the
> parent. If the driver can guarantee than it will pick the most appropriate
> parent, there is no reason to have that.
> 
>> I know that it's easy to detect this when the clock is passed in the
>> "clocks" property, but I'm not sure if it's easy to parse it from the
>> assigned-clocks/assigned-clock-parents properties.
> 
> Assigned parent is the poor man solution and not necessarily easier to
> implement (the pwm device would have to export its own clocks) ... I have just
> mentioned it to make  the point that current method is not ideal
> 
>>
>> [...]
>>> Last, instead of specifying the parent to be used, I think we should come up
>>> with some code to let the driver pick the most appropriate parent for the period/duty requested.
>> that will make it easier for .dts authors.
>> I would like to postpone this until we have solved the other topics though.
> 
> I much prefer this last solution. Since the algorithm and the bindings would
> change, I think it would be easier (in DT) to just make v2 driver with a new
> compatible, progressively transition dts to it and finally remove the old
> driver.

I'd prefer this, but to be frank, the parent only determines the precision of
the PWM internal divider to give the most precise period possible.
For this the highest frequency is the best.

But you could also use an input clock with the lowest jitter, and this cannot be
determined automatically.

> 
> 

Neil

Re: [PATCH 0/1] pwm: meson: fix scheduling while atomic issue

[Martin Blumenstingl]

Hi Jerome,

On Tue, Mar 26, 2019 at 9:37 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
>
> On Mon, 2019-03-25 at 19:04 +0100, Martin Blumenstingl wrote:
> > > Thanks for fixing this Martin.
> > you're welcome!
> >
> > > As for the future enhancement, I'd like to know what you have in mind.
> > > As I have told you previously, I think the clock bindings of this driver are
> > > not great.
> > >
> > > The global name of the input clocks are hard coded in this driver and it
> > > sucks. CCF is evolving to rely less on these global names.
> > I fully agree with you on the clock setup, but I'm not sure if we have
> > to break the dt-bindings for it.
> >
> > the datasheet notes: "Each PWM is driven by a programmable divider
> > driven by a 4:1 clock selector".
> > In my own words this means that each PWM controller has up to 8 clock inputs:
> > - up to 4 inputs for the first channel (PWM_A)
> > - up to 4 inputs for the second channel (PWM_B)
>
> Not from the pwm device POV. there is one device (PWM_AB) with 4 (max) input
> clocks. Those are consumed by two internal muxes. There would be 8 if the
> input was different between path A and B.
>
> >
> > the current pwm-meson driver assumes that both the inputs for both
> > channels are identical.
> > the "clock trees" section of the public S912 datasheet (page 65)
> > clearly documents the clock inputs per PWM channel, not per PWM
> > controller.
> >
> > Thus I believe we should name our clock-names (the inputs to the PWM
> > controller) "pwm-a-clkin0", "pwm-a-clkin1", "pwm-b-clkin0", ...
> > That way we don't have a conflict with the existing bindings (which
> > already reserve "clkin0" and "clkin1").
>
> I think this is overkill an inaccurate. The experience of all the soc we have
> seen so far (meson8, gxbb, gxl, gxm, axg and g12) confirms the sources the are
> the same input clock for both path.
>
> The documentation just shows the clock src of each pwm. That just how the the
> table is presented. That does not change the fact the pwms are organized in
> modules (pairs) and the that the clock source are the same for each pwm of the
> module. IOW, there is only 4 lines of clocks getting to the IP, not 8. Feel
> free to ask amlogic if you want to make sure.
interesting, the way you describe it also seems valid.

I don't want to implement either way and find out that we have to
change the bindings again later on.

Bichao, Jianxin, can you answer the question whether each PWM controller has:
- up to four clock inputs. clock parents for channels PWM_A and PWM_B
are always the same
- up to eight clock inputs. four parents for channel PWM_A and four
more parents for PWM_B, where the parents of PWM_A and PWM_B could be
different
(for all known SoCs the parents of the channels PWM_A and PWM_B are
identical, so we don't know how the hardware is modelled)

> The name clash is not really my point. The purpose of the clock binding would
> be different (from stating a setting to describing hw connection)
OK, so your main concern is that we're breaking the DT bindings - so
instead of coming up with a "quick fix" we might as well take the long
but proper route?

> >
> > > In addition, the 'clock' binding should be used to refer to the clock
> > > 'consumed' by the device, not to define a setting (as done now). 'assigned-
> > > clock' binding can be used for that.
> > using the assigned-clock* properties requires self-referencing the PWM
> > controller (which I'm not used to), for example:
> >   &pwm_ab {
> >       #clock-cells = <1>;
> >       assigned-clocks = <&pwm_ab 0>, <&pwm_ab 1>; /* references itself */
> >       assigned-clock-parents = <&xtal>, <&clkc CLKID_FCLK_DIV5>;
> >   };
> >
> > if we want to auto-detect the parent clock (like you suggested below)
> > we need to consider if we can detect whether a .dts-author assigned a
> > specific parent.
>
> I (personally) don't want to keep supporting the manual assignment of the
> parent. If the driver can guarantee than it will pick the most appropriate
> parent, there is no reason to have that.
>
> > I know that it's easy to detect this when the clock is passed in the
> > "clocks" property, but I'm not sure if it's easy to parse it from the
> > assigned-clocks/assigned-clock-parents properties.
>
> Assigned parent is the poor man solution and not necessarily easier to
> implement (the pwm device would have to export its own clocks) ... I have just
> mentioned it to make  the point that current method is not ideal
>
> >
> > [...]
> > > Last, instead of specifying the parent to be used, I think we should come up
> > > with some code to let the driver pick the most appropriate parent for the period/duty requested.
> > that will make it easier for .dts authors.
> > I would like to postpone this until we have solved the other topics though.
>
> I much prefer this last solution. Since the algorithm and the bindings would
> change, I think it would be easier (in DT) to just make v2 driver with a new
> compatible, progressively transition dts to it and finally remove the old
> driver.
Neil has already raised the question whether clock jitter may be relevant.

what also came to my mind is the "system suspend" use-case:
I'm not sure if the fixed_pll (and it's post-dividers) are enabled
during system suspend.
in that case we probably need clock the PWM channel off the main XTAL.
(I don't know if this use-case really exists, I want to bring it up so
we can discuss it)


Regards
Martin