Re: [PATCH 0/1] pwm: meson: fix scheduling while atomic issue
On 30/03/2019 20:29, Martin Blumenstingl wrote:
> Hello Uwe,
>
> On Mon, Mar 25, 2019 at 9:07 PM Uwe Kleine-König
> 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, );
> pwm_apply_state(pwm, );
>
> 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,00% 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 0x 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
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
> 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, );
> pwm_apply_state(pwm, );
>
> 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
Hello Uwe,
On Mon, Mar 25, 2019 at 9:07 PM Uwe Kleine-König
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, );
pwm_apply_state(pwm, );
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
Hi Jerome,
On Tue, Mar 26, 2019 at 9:37 AM 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 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:
> > _ab {
> > #clock-cells = <1>;
> > assigned-clocks = <_ab 0>, <_ab 1>; /* references itself */
> > assigned-clock-parents = <>, < 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
Re: [PATCH 0/1] pwm: meson: fix scheduling while atomic issue
Hello Uwe,
On Mon, Mar 25, 2019 at 9:07 PM Uwe Kleine-König
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
> > 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
Re: [PATCH 0/1] pwm: meson: fix scheduling while atomic issue
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
> > 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
On 25/03/2019 18:41, Martin Blumenstingl wrote:
> Hello Uwe,
>
> On Mon, Mar 25, 2019 at 9:41 AM Uwe Kleine-König
> 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
Re: [PATCH 0/1] pwm: meson: fix scheduling while atomic issue
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:
>> _ab {
>> #clock-cells = <1>;
>> assigned-clocks = <_ab 0>, <_ab 1>; /* references itself */
>> assigned-clock-parents = <>, < 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
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:
> _ab {
> #clock-cells = <1>;
> assigned-clocks = <_ab 0>, <_ab 1>; /* references itself */
> assigned-clock-parents = <>, < 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
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
> 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,
Re: [PATCH 0/1] pwm: meson: fix scheduling while atomic issue
Hi Jerome,
On Mon, Mar 25, 2019 at 10:35 AM Jerome Brunet 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:
_ab {
#clock-cells = <1>;
assigned-clocks = <_ab 0>, <_ab 1>; /* references itself */
assigned-clock-parents = <>, < 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
Hello Uwe,
On Mon, Mar 25, 2019 at 9:41 AM Uwe Kleine-König
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
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
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
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/ |
