Message-ID: <ec428b82-f7d9-0f14-363c-a40d14ce4414@baylibre.com>
Date:   Tue, 26 Mar 2019 10:06:31 +0100
From:   Neil Armstrong <narmstrong@...libre.com>
To:     Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
        Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     thierry.reding@...il.com, jbrunet@...libre.com,
        linux-pwm@...r.kernel.org, linux-amlogic@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: 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
> <u.kleine-koenig@...gutronix.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
> 

Powered by blists - more mailing lists