Re: [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 29-03-17 13:24, Andy Shevchenko wrote:
> On Tue, 2017-03-28 at 21:16 +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 03/28/2017 07:33 PM, Andy Shevchenko wrote:
>>> On Tue, 2017-03-28 at 20:28 +0300, Andy Shevchenko wrote:
>>>> On Tue, 2017-03-28 at 19:20 +0200, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 03/28/2017 04:45 PM, Andy Shevchenko wrote:
>>>>>> On Sat, 2017-03-25 at 15:06 +0100, Hans de Goede wrote:
>>>>>>> Hi Andy, Thierry,
>>>>>>>
>>>>>>> This patch fixes a regression with the pwm-lpss driver in
>>>>>>> 4.11,
>>>>>>> where it once turned off will not turn back on again on some
>>>>>>> machines. Yet it has been silent around this patch for some
>>>>>>> time now. Can you please review this and get it queued as a
>>>>>>> fix
>>>>>>> for 4.11 ?
>>>>>>
>>>>>> I have tested this patch (*) on 3 boards:
>>>>>> 1) internal development board (ApolloLake)
>>>>>> 2) MinnowBoard MAX (BayTrail)
>>>>>> 3) Intel Edison / Arduino break out (Tangier)
>>>>>> 4) ...not yet... (CherryTrail / Braswell)
>>>>>>
>>>>>> So, the patch *broke* functionality on 1), while 2) and 3) are
>>>>>> survived.
>>>>>
>>>>> Bummer, so on Apollo Lake we need to set the update bit
>>>>> *and* wait for it to get acked before setting enable ?
>>>>
>>>> It's designed behaviour as far as I know.
>>>>
>>>> Let me cite a bit of documentation (the wordings is the same
>>>> across
>>>> *all* supported Intel SoCs):
>>>>
>>>> --- 8< --- 8< ---
>>>>
>>>> 9.8.2 Programming Sequence
>>>> To ensure that there are no operational issues with PWM the
>>>> following
>>>> programming sequences must be performed in the order defined.
>>>>
>>>> • Initial Enable or First Activation
>>>> — Program the Base Unit and On Time Divisor values
>>>> — Set the Software Update Bit
>>>> — Enable the PWM Output by setting the PWM Enable bit
>>>> — Repeat the above steps for the next PWM module
>>>>
>>>> • Dynamic update while PWM is Enabled
>>>> — Program the Base Unit and On Time Divisor values
>>>> — Set the Software Update Bit
>>>> — Repeat the above steps for the next PWM module
>>>>
>>>> --- 8< --- 8< ---
>>>>
>>>> (I'm a bit confused about last item in each list, it doesn't
>>>> clarify
>>>> should we use _same_ values or different ones. I hope it just a
>>>> recommendation how to program multiple PWMs, not an *obligation*)
>>>>
>>>
>>> Missed additional part
>>>
>>> --- 8< --- 8< ---
>>>
>>> 9.8.1 Functional Description
>>> Software controls the PWM block by updating the PWMCTRL register and
>>> setting the sw_update bit whenever a change in frequency or duty
>>> cycle
>>> of the PWM output signal is required. When the sw_update bit is set
>>> the
>>> PWM block applies the new settings at the start of the next output
>>> cycle
>>> and resets the sw_update bit.
>>>
>>> --- 8< --- 8< ---
>>
>> Right, notice the "pplies the new settings at the start of the next
>> output cycle"
>> with the enable bit not set the next cycle never starts, at least that
>> seems to
>> be the case on Cherry Trail.
>
> So, you are telling that "Initial Enable or First Activation" is buggy
> in your case?

No what I'm saying is that if you set the update bit to 1 while the enable
bit is 0 and then wait for the update bit to clear it will never clear
because it will clear when "the PWM block applies the new settings at
the start of the next output cycle" and there is no start of the next
output cycle with enable being 0 because then there is no output.

IOW it seems that the cherrytrail hw is following the spec at least
how I read it.

Either way I think the best way to resolve this is by having different
code paths on enable for Cherry Trail vs Apollo Lake. As I already said
I will happily test a patch for this on Cherry Trail.

Regards,

Hans