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

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

[Hans de Goede]

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 ?

Regards,

Hans

[PATCH v2 resend] pwm: lpss: Set enable-bit before waiting for update-bit to go low

[Hans de Goede]

At least on cherrytrail, the update bit will never go low when the
enabled bit is not set.

This causes the backlight on my cube iwork8 air tablet to never turn on
again after being turned off because in the pwm_lpss_apply enable path
pwm_lpss_update will fail causing an error exit and the enable-bit to
never get set. Any following pwm_lpss_apply calls will fail the
pwm_lpss_is_updating check.

Since the docs say that the update bit should be set before the
enable-bit, split pwm_lpss_update into setting the update-bit and
pwm_lpss_wait_for_update, and move the pwm_lpss_wait_for_update call
in the enable path to after setting the enable-bit.

Fixes: 10d56a4 ("pwm: lpss: Avoid reconfiguring while UPDATE bit...")
Cc: Ilkka Koskinen <ilkka.koskinen@intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-This is a new approach to fixing the problem, replacing
 "pwm: lpss: Do not set / wait_for update_bit when not enabled"
---
 drivers/pwm/pwm-lpss.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index 689d2c1..eac3d52 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -57,7 +57,7 @@ static inline void pwm_lpss_write(const struct pwm_device *pwm, u32 value)
 	writel(value, lpwm->regs + pwm->hwpwm * PWM_SIZE + PWM);
 }
 
-static int pwm_lpss_update(struct pwm_device *pwm)
+static int pwm_lpss_wait_for_update(struct pwm_device *pwm)
 {
 	struct pwm_lpss_chip *lpwm = to_lpwm(pwm->chip);
 	const void __iomem *addr = lpwm->regs + pwm->hwpwm * PWM_SIZE + PWM;
@@ -65,8 +65,6 @@ static int pwm_lpss_update(struct pwm_device *pwm)
 	u32 val;
 	int err;
 
-	pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_SW_UPDATE);
-
 	/*
 	 * PWM Configuration register has SW_UPDATE bit that is set when a new
 	 * configuration is written to the register. The bit is automatically
@@ -137,18 +135,20 @@ static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 				return ret;
 			}
 			pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, state->period);
-			ret = pwm_lpss_update(pwm);
+			pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_SW_UPDATE);
+			pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE);
+			ret = pwm_lpss_wait_for_update(pwm);
 			if (ret) {
 				pm_runtime_put(chip->dev);
 				return ret;
 			}
-			pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_ENABLE);
 		} else {
 			ret = pwm_lpss_is_updating(pwm);
 			if (ret)
 				return ret;
 			pwm_lpss_prepare(lpwm, pwm, state->duty_cycle, state->period);
-			return pwm_lpss_update(pwm);
+			pwm_lpss_write(pwm, pwm_lpss_read(pwm) | PWM_SW_UPDATE);
+			return pwm_lpss_wait_for_update(pwm);
 		}
 	} else if (pwm_is_enabled(pwm)) {
 		pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE);
-- 
2.9.3

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

[Andy Shevchenko]

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 ?
> 

Hans, please, give me and Ilkka time to test this. It might appear that
this fix breaks other platforms and we might need to introduce a quirk
based on PCI ID. (Hope not)

P.S. I will try to allocate time next week soonish for that.

Ilkka, can you do some tests on your side?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Hans de Goede]

Hi,

On 26-03-17 14:25, 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 ?
>>
>
> Hans, please, give me and Ilkka time to test this. It might appear that
> this fix breaks other platforms and we might need to introduce a quirk
> based on PCI ID. (Hope not)

Ok, thank you for looking into this.

Regards,

Hans

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

[Ilkka Koskinen]

Hi,

On Sun, Mar 26, 2017 at 03:25:24PM +0300, 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 ?
> > 

Sorry, my bad. I thought I replied to this but obviously didn't.

> Hans, please, give me and Ilkka time to test this. It might appear that
> this fix breaks other platforms and we might need to introduce a quirk
> based on PCI ID. (Hope not)

While the patch seems ok, I have the same concern. 
 
> P.S. I will try to allocate time next week soonish for that.
> 
> Ilkka, can you do some tests on your side?

Unfortunately, I don't have a suitable device anymore as I changed the
team. However, I have asked a colleague to test the patch on BXT. He's
planning to do that this week.

Br, Ilkka

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

[Andy Shevchenko]

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.

(*) The base is my eds branch (https://github.com/andy-shev/linux/tree/e
ds, v4.11-rc4 based) + few pin control related patches to enable PWM
output.

P.S. I'll continue looking for CherryTrail / Braswell based board to
have some coverage there in the future.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Hans de Goede]

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 ?

NOte my patch does not change the order of writes, only when we
do the wait.

The only good options I see to fix this is to introduce SoC family
specific code paths :|

If you can whip up a new version which is tested on Apollo Lake
and Bay Trail I can run some tests on Cherry Trail.

Regards,

Hans



>
> (*) The base is my eds branch (https://github.com/andy-shev/linux/tree/e
> ds, v4.11-rc4 based) + few pin control related patches to enable PWM
> output.
>
> P.S. I'll continue looking for CherryTrail / Braswell based board to
> have some coverage there in the future.
>

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

[Andy Shevchenko]

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*)

> 
> NOte my patch does not change the order of writes, only when we
> do the wait.
> 
> The only good options I see to fix this is to introduce SoC family
> specific code paths :|
> 
> If you can whip up a new version which is tested on Apollo Lake
> and Bay Trail I can run some tests on Cherry Trail.
> 
> Regards,
> 
> Hans
> 
> 
> 
> > 
> > (*) The base is my eds branch (https://github.com/andy-shev/linux/tr
> > ee/e
> > ds, v4.11-rc4 based) + few pin control related patches to enable PWM
> > output.
> > 
> > P.S. I'll continue looking for CherryTrail / Braswell based board to
> > have some coverage there in the future.
> > 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Andy Shevchenko]

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< ---

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Hans de Goede]

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.

Regards,

Hans

>

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

[Ilkka Koskinen]

On Tue, Mar 28, 2017 at 05:45:27PM +0300, 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.

I was told that it broke the functionality on Joule as well, which isn't
suprising given that it did the same thing on ApolloLake

> 
> (*) The base is my eds branch (https://github.com/andy-shev/linux/tree/e
> ds, v4.11-rc4 based) + few pin control related patches to enable PWM
> output.
> 
> P.S. I'll continue looking for CherryTrail / Braswell based board to
> have some coverage there in the future.
> 
> -- 
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Andy Shevchenko]

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?

I'm also wondering how different BayTrail vs. CherryTrail in that sense.

It seems I really need to find CherryTrail with PWM available to test
(we have few, but on all of them PWM device is disabled by BIOS, besides
that fact that we have no schematics of most of them).

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Hans de Goede]

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

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

[Andy Shevchenko]

On Wed, 2017-03-29 at 14:42 +0200, Hans de Goede wrote:
> On 29-03-17 13:24, Andy Shevchenko wrote:
> > On Tue, 2017-03-28 at 21:16 +0200, Hans de Goede wrote:
> > > 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:

> > > > > 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
> > > > > 

> > > > 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.

Okay, in that case, why it works on the rest of SoCs either way?

To me it sounds that spec doesn't clarify what exactly enable bit does
vs. SW update one in actual hardware.

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

Spec doesn't clarify whether we should wait or not for SW update bit to
be cleared when we enable PWM.

> 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.

I have one more theory, but would like to check first.

Can you add something like the following whenever you about to set
values to PWM (in either case if it's enabled or not)

{
void __iomem *x = iomap(phys_base + 0x800, 8);
pr_info("Values are: %x %x\n", readl(x+0), readl(x+4));
iounmap(x);
pr_info("PWMCTRL: %x\n", readl(base));
}
...

update register
...


I would like to see this when it works and when it doesn't.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Hans de Goede]

Hi,

On 29-03-17 19:41, Andy Shevchenko wrote:
> On Wed, 2017-03-29 at 14:42 +0200, Hans de Goede wrote:
>> On 29-03-17 13:24, Andy Shevchenko wrote:
>>> On Tue, 2017-03-28 at 21:16 +0200, Hans de Goede wrote:
>>>> 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:
>
>>>>>> 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
>>>>>>
>
>>>>> 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.
>
> Okay, in that case, why it works on the rest of SoCs either way?
>
> To me it sounds that spec doesn't clarify what exactly enable bit does
> vs. SW update one in actual hardware.
>
>>
>> IOW it seems that the cherrytrail hw is following the spec at least
>> how I read it.
>
> Spec doesn't clarify whether we should wait or not for SW update bit to
> be cleared when we enable PWM.
>
>> 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.
>
> I have one more theory, but would like to check first.
>
> Can you add something like the following whenever you about to set
> values to PWM (in either case if it's enabled or not)
>
> {
> void __iomem *x = iomap(phys_base + 0x800, 8);
> pr_info("Values are: %x %x\n", readl(x+0), readl(x+4));
> iounmap(x);
> pr_info("PWMCTRL: %x\n", readl(base));
> }
> ...
>
> update register
> ...
>
>
> I would like to see this when it works and when it doesn't.

OK, so with this diff:

--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -54,6 +54,9 @@ static inline void pwm_lpss_write(const struct pwm_device *pwm, u32 value)
  {
         struct pwm_lpss_chip *lpwm = to_lpwm(pwm->chip);

+ pr_err("pwm-lpss writing 0x%08x, values are 0x%08x 0x%08x\n",
+         value, readl(lpwm->regs + 0x800), readl(lpwm->regs + 0x804));
+
         writel(value, lpwm->regs + pwm->hwpwm * PWM_SIZE + PWM);
  }


I get the following without my latest patch to fix the issues I'm seeing
on cherrytrail:

[root@localhost ~]# dmesg | grep pwm-lpss
[    3.349048] pwm-lpss writing 0x8000a000, values are 0x00000000 0x00000003
[    3.349063] pwm-lpss writing 0xc000a000, values are 0x00000000 0x00000003
[    3.691037] pwm-lpss writing 0x8000a0ff, values are 0x00000000 0x00000003
[    3.691044] pwm-lpss writing 0xc000a0ff, values are 0x00000000 0x00000003
[    3.694369] pwm-lpss writing 0x0000a0ff, values are 0x00000000 0x00000003
[    4.752778] pwm-lpss writing 0x0000a0ff, values are 0x00000000 0x00000003
[    4.752788] pwm-lpss writing 0x4000a0ff, values are 0x00000000 0x00000003
[    5.252803] pwm-lpss 80862288:00: PWM_SW_UPDATE was not cleared

And a black lcd panel after that.

And the following with my patch added:

[root@ipv6 ~]# dmesg | grep pwm-lpss
[    3.312239] pwm-lpss writing 0x8000a000, values are 0x00000000 0x00000003
[    3.312253] pwm-lpss writing 0xc000a000, values are 0x00000000 0x00000003
[    3.652711] pwm-lpss writing 0x8000a0ff, values are 0x00000000 0x00000003
[    3.652720] pwm-lpss writing 0xc000a0ff, values are 0x00000000 0x00000003
[    3.655850] pwm-lpss writing 0x0000a0ff, values are 0x00000000 0x00000003
[    4.703856] pwm-lpss writing 0x0000a0ff, values are 0x00000000 0x00000003
[    4.703866] pwm-lpss writing 0x4000a0ff, values are 0x00000000 0x00000003
[    4.703875] pwm-lpss writing 0xc000a0ff, values are 0x00000000 0x00000003
[    4.703974] pwm-lpss writing 0x8000a000, values are 0x00000000 0x00000003
[    4.703982] pwm-lpss writing 0xc000a000, values are 0x00000000 0x00000003
[    9.617330] pwm-lpss writing 0x8000a0b3, values are 0x00000000 0x00000003
[    9.617343] pwm-lpss writing 0xc000a0b3, values are 0x00000000 0x00000003

And everything works as expected, note the brightness gets dimmed
after boot by userspace restoring the last brightness set before shutdown /
reboot.

Regards,

Hans

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

[Andy Shevchenko]

On Wed, 2017-03-29 at 20:25 +0200, Hans de Goede wrote:
> Hi,
> 
> On 29-03-17 19:41, Andy Shevchenko wrote:
> > On Wed, 2017-03-29 at 14:42 +0200, Hans de Goede wrote:

Thanks for your patience and valuable input.

So, I found CharryTrail with enabled PWM (UP board v0.4) and confirm the
bug.

Moreover, I have re-tested again all 4 platforms with and without your
fix, and I dunno how I did not notice this before, but looks like either
mine (though commit message shows that I have tested on 3 platforms at
least, so, I can re-test for sure) or Ilkka's patch broke it on all
platforms except Broxton / Apollo Lake.

So, summurize what we need is a quirk for Broxton / Apollo Lake. I need
to check Gemini Lake also to be sure.

And we definitely need this as a fix for stable. I would appreciate if
you can figure out which patch from previous series (b14e8ceff034
or 10d56a4cb1c6) broke it.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Hans de Goede]

Hi,

On 31-03-17 22:07, Andy Shevchenko wrote:
> On Wed, 2017-03-29 at 20:25 +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 29-03-17 19:41, Andy Shevchenko wrote:
>>> On Wed, 2017-03-29 at 14:42 +0200, Hans de Goede wrote:
>
> Thanks for your patience and valuable input.
>
> So, I found CharryTrail with enabled PWM (UP board v0.4) and confirm the
> bug.
>
> Moreover, I have re-tested again all 4 platforms with and without your
> fix, and I dunno how I did not notice this before, but looks like either
> mine (though commit message shows that I have tested on 3 platforms at
> least, so, I can re-test for sure) or Ilkka's patch broke it on all
> platforms except Broxton / Apollo Lake.
>
> So, summurize what we need is a quirk for Broxton / Apollo Lake. I need
> to check Gemini Lake also to be sure.
>
> And we definitely need this as a fix for stable. I would appreciate if
> you can figure out which patch from previous series (b14e8ceff034
> or 10d56a4cb1c6) broke it.

My patch commit msg says: "fixes 10d56a4cb1c6c894c60acbaec0f8aa44aba833b0"
and unless my memory deceives me I tested that that was the bad commit
by reverting it.

The problem for Cherry Trail is not perse the order in which the
update / enable bits are written (AFAICT), but the waiting for
the update bit to clear while the enable bit is not set, which
is why my latest version only moves the wait.

That wait was already present before 10d56a4cb1c6, but with a
reasonable short timeout, and just continuing on after the
timeout. 10d56a4cb1c6 makes pwm_lpss_apply() exit with an
error (without ever setting the enable bit) when the timeout
expires.

The troublesome commit caused 2 issues:
1) An IMHO unacceptable long timeout (0.5 seconds of near busy waiting on each enable)
2) Erroring out when the timeout expires without the update flag clearing

Regards,

Hans

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

[Andy Shevchenko]

On Fri, 2017-03-31 at 22:52 +0200, Hans de Goede wrote:
> Hi,
> 
> On 31-03-17 22:07, Andy Shevchenko wrote:
> > On Wed, 2017-03-29 at 20:25 +0200, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 29-03-17 19:41, Andy Shevchenko wrote:
> > > > On Wed, 2017-03-29 at 14:42 +0200, Hans de Goede wrote:
> > 
> > Thanks for your patience and valuable input.
> > 
> > So, I found CharryTrail with enabled PWM (UP board v0.4) and confirm
> > the
> > bug.
> > 
> > Moreover, I have re-tested again all 4 platforms with and without
> > your
> > fix, and I dunno how I did not notice this before, but looks like
> > either
> > mine (though commit message shows that I have tested on 3 platforms
> > at
> > least, so, I can re-test for sure) or Ilkka's patch broke it on all
> > platforms except Broxton / Apollo Lake.
> > 
> > So, summurize what we need is a quirk for Broxton / Apollo Lake. I
> > need
> > to check Gemini Lake also to be sure.
> > 
> > And we definitely need this as a fix for stable. I would appreciate
> > if
> > you can figure out which patch from previous series (b14e8ceff034
> > or 10d56a4cb1c6) broke it.
> 
> My patch commit msg says: "fixes
> 10d56a4cb1c6c894c60acbaec0f8aa44aba833b0"
> and unless my memory deceives me I tested that that was the bad commit
> by reverting it.

Oh, yes, thanks for pointing this out.

> 
> The problem for Cherry Trail is not perse the order in which the
> update / enable bits are written (AFAICT), but the waiting for
> the update bit to clear while the enable bit is not set, which
> is why my latest version only moves the wait.

And it looks sane.

> That wait was already present before 10d56a4cb1c6, but with a
> reasonable short timeout, and just continuing on after the
> timeout. 10d56a4cb1c6 makes pwm_lpss_apply() exit with an
> error (without ever setting the enable bit) when the timeout
> expires.
> 

> The troublesome commit caused 2 issues:
> 1) An IMHO unacceptable long timeout (0.5 seconds of near busy waiting
> on each enable
> )

The current clocks and divisor values allow us to set pulses up to
218 ms. I have no idea how we can decrease this significantly. 300 ms?
Or other way which I proposed to Mika and Ilkka during internal review
is to calculate it from last cycle.

> 2) Erroring out when the timeout expires without the update flag
> clearing

What would we do in such case? There is no recovery for us except
waiting more.

I'm about to submit a patch which splits Tangier out of Broxton
configuration (it should go before your fix) and then we need to
introduce a quirk for Broxton and derivatives (Apollo Lake, Gemini
Lake).

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Hans de Goede]

HI,

On 03-04-17 16:22, Andy Shevchenko wrote:
> On Fri, 2017-03-31 at 22:52 +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 31-03-17 22:07, Andy Shevchenko wrote:
>>> On Wed, 2017-03-29 at 20:25 +0200, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 29-03-17 19:41, Andy Shevchenko wrote:
>>>>> On Wed, 2017-03-29 at 14:42 +0200, Hans de Goede wrote:
>>>
>>> Thanks for your patience and valuable input.
>>>
>>> So, I found CharryTrail with enabled PWM (UP board v0.4) and confirm
>>> the
>>> bug.
>>>
>>> Moreover, I have re-tested again all 4 platforms with and without
>>> your
>>> fix, and I dunno how I did not notice this before, but looks like
>>> either
>>> mine (though commit message shows that I have tested on 3 platforms
>>> at
>>> least, so, I can re-test for sure) or Ilkka's patch broke it on all
>>> platforms except Broxton / Apollo Lake.
>>>
>>> So, summurize what we need is a quirk for Broxton / Apollo Lake. I
>>> need
>>> to check Gemini Lake also to be sure.
>>>
>>> And we definitely need this as a fix for stable. I would appreciate
>>> if
>>> you can figure out which patch from previous series (b14e8ceff034
>>> or 10d56a4cb1c6) broke it.
>>
>> My patch commit msg says: "fixes
>> 10d56a4cb1c6c894c60acbaec0f8aa44aba833b0"
>> and unless my memory deceives me I tested that that was the bad commit
>> by reverting it.
>
> Oh, yes, thanks for pointing this out.
>
>>
>> The problem for Cherry Trail is not perse the order in which the
>> update / enable bits are written (AFAICT), but the waiting for
>> the update bit to clear while the enable bit is not set, which
>> is why my latest version only moves the wait.
>
> And it looks sane.
>
>> That wait was already present before 10d56a4cb1c6, but with a
>> reasonable short timeout, and just continuing on after the
>> timeout. 10d56a4cb1c6 makes pwm_lpss_apply() exit with an
>> error (without ever setting the enable bit) when the timeout
>> expires.
>>
>
>> The troublesome commit caused 2 issues:
>> 1) An IMHO unacceptable long timeout (0.5 seconds of near busy waiting
>> on each enable
>> )
>
> The current clocks and divisor values allow us to set pulses up to
> 218 ms. I have no idea how we can decrease this significantly. 300 ms?
> Or other way which I proposed to Mika and Ilkka during internal review
> is to calculate it from last cycle.

Right, I was not clear, sorry I mean this is a problem because
normally we should not wait so long, but now on Cherry Trail
(and others) we do wait so long as the enabled bit is not set.

What I was trying to say is that the code was already wrongly
waiting for the timeout to expire (since update would never
clear before commit 10d56a4cb1c6 already) but this was not a big
deal as it was not waiting for a long time.

>> 2) Erroring out when the timeout expires without the update flag
>> clearing
>
> What would we do in such case? There is no recovery for us except
> waiting more.

Same here, again what I meant is that the commit makes the already
wrong behavior from before a problem because it has turned it
into an error condition. The proper fix is of course to make sure
we do not hit the timeout by setting enabled before waiting for
the update bit to clear on all hardware except broxton.

Regards,

Hans

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

[Andy Shevchenko]

On Mon, 2017-04-03 at 16:32 +0200, Hans de Goede wrote:
> HI,
> 
> On 03-04-17 16:22, Andy Shevchenko wrote:
> > On Fri, 2017-03-31 at 22:52 +0200, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 31-03-17 22:07, Andy Shevchenko wrote:
> > > > On Wed, 2017-03-29 at 20:25 +0200, Hans de Goede wrote:
> > > > > Hi,
> > > > > 
> > > > > On 29-03-17 19:41, Andy Shevchenko wrote:
> > > > > > On Wed, 2017-03-29 at 14:42 +0200, Hans de Goede wrote:
> > > > 
> > > > Thanks for your patience and valuable input.
> > > > 
> > > > So, I found CharryTrail with enabled PWM (UP board v0.4) and
> > > > confirm
> > > > the
> > > > bug.
> > > > 
> > > > Moreover, I have re-tested again all 4 platforms with and
> > > > without
> > > > your
> > > > fix, and I dunno how I did not notice this before, but looks
> > > > like
> > > > either
> > > > mine (though commit message shows that I have tested on 3
> > > > platforms
> > > > at
> > > > least, so, I can re-test for sure) or Ilkka's patch broke it on
> > > > all
> > > > platforms except Broxton / Apollo Lake.
> > > > 
> > > > So, summurize what we need is a quirk for Broxton / Apollo Lake.
> > > > I
> > > > need
> > > > to check Gemini Lake also to be sure.
> > > > 
> > > > And we definitely need this as a fix for stable. I would
> > > > appreciate
> > > > if
> > > > you can figure out which patch from previous series
> > > > (b14e8ceff034
> > > > or 10d56a4cb1c6) broke it.
> > > 
> > > My patch commit msg says: "fixes
> > > 10d56a4cb1c6c894c60acbaec0f8aa44aba833b0"
> > > and unless my memory deceives me I tested that that was the bad
> > > commit
> > > by reverting it.
> > 
> > Oh, yes, thanks for pointing this out.
> > 
> > > 
> > > The problem for Cherry Trail is not perse the order in which the
> > > update / enable bits are written (AFAICT), but the waiting for
> > > the update bit to clear while the enable bit is not set, which
> > > is why my latest version only moves the wait.
> > 
> > And it looks sane.
> > 
> > > That wait was already present before 10d56a4cb1c6, but with a
> > > reasonable short timeout, and just continuing on after the
> > > timeout. 10d56a4cb1c6 makes pwm_lpss_apply() exit with an
> > > error (without ever setting the enable bit) when the timeout
> > > expires.
> > > 
> > > The troublesome commit caused 2 issues:
> > > 1) An IMHO unacceptable long timeout (0.5 seconds of near busy
> > > waiting
> > > on each enable
> > > )
> > 
> > The current clocks and divisor values allow us to set pulses up to
> > 218 ms. I have no idea how we can decrease this significantly. 300
> > ms?
> > Or other way which I proposed to Mika and Ilkka during internal
> > review
> > is to calculate it from last cycle.
> 
> Right, I was not clear, sorry I mean this is a problem because
> normally we should not wait so long, but now on Cherry Trail
> (and others) we do wait so long as the enabled bit is not set.
> 
> What I was trying to say is that the code was already wrongly
> waiting for the timeout to expire (since update would never
> clear before commit 10d56a4cb1c6 already) but this was not a big
> deal as it was not waiting for a long time.
> 
> > > 2) Erroring out when the timeout expires without the update flag
> > > clearing
> > 
> > What would we do in such case? There is no recovery for us except
> > waiting more.
> 
> Same here, again what I meant is that the commit makes the already
> wrong behavior from before a problem because it has turned it
> into an error condition. The proper fix is of course to make sure
> we do not hit the timeout by setting enabled before waiting for
> the update bit to clear on all hardware except broxton.

Thanks for elaborative message.
Feel free to use my patch as a base for yours to provide a quirk-based
solution. I would test it on my side for 4 platforms I have PWM enabled
on.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Hans de Goede]

Hi,

On 04/04/2017 06:20 PM, Andy Shevchenko wrote:
> On Mon, 2017-04-03 at 16:32 +0200, Hans de Goede wrote:
>> HI,
>>
>> On 03-04-17 16:22, Andy Shevchenko wrote:
>>> On Fri, 2017-03-31 at 22:52 +0200, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 31-03-17 22:07, Andy Shevchenko wrote:
>>>>> On Wed, 2017-03-29 at 20:25 +0200, Hans de Goede wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 29-03-17 19:41, Andy Shevchenko wrote:
>>>>>>> On Wed, 2017-03-29 at 14:42 +0200, Hans de Goede wrote:
>>>>>
>>>>> Thanks for your patience and valuable input.
>>>>>
>>>>> So, I found CharryTrail with enabled PWM (UP board v0.4) and
>>>>> confirm
>>>>> the
>>>>> bug.
>>>>>
>>>>> Moreover, I have re-tested again all 4 platforms with and
>>>>> without
>>>>> your
>>>>> fix, and I dunno how I did not notice this before, but looks
>>>>> like
>>>>> either
>>>>> mine (though commit message shows that I have tested on 3
>>>>> platforms
>>>>> at
>>>>> least, so, I can re-test for sure) or Ilkka's patch broke it on
>>>>> all
>>>>> platforms except Broxton / Apollo Lake.
>>>>>
>>>>> So, summurize what we need is a quirk for Broxton / Apollo Lake.
>>>>> I
>>>>> need
>>>>> to check Gemini Lake also to be sure.
>>>>>
>>>>> And we definitely need this as a fix for stable. I would
>>>>> appreciate
>>>>> if
>>>>> you can figure out which patch from previous series
>>>>> (b14e8ceff034
>>>>> or 10d56a4cb1c6) broke it.
>>>>
>>>> My patch commit msg says: "fixes
>>>> 10d56a4cb1c6c894c60acbaec0f8aa44aba833b0"
>>>> and unless my memory deceives me I tested that that was the bad
>>>> commit
>>>> by reverting it.
>>>
>>> Oh, yes, thanks for pointing this out.
>>>
>>>>
>>>> The problem for Cherry Trail is not perse the order in which the
>>>> update / enable bits are written (AFAICT), but the waiting for
>>>> the update bit to clear while the enable bit is not set, which
>>>> is why my latest version only moves the wait.
>>>
>>> And it looks sane.
>>>
>>>> That wait was already present before 10d56a4cb1c6, but with a
>>>> reasonable short timeout, and just continuing on after the
>>>> timeout. 10d56a4cb1c6 makes pwm_lpss_apply() exit with an
>>>> error (without ever setting the enable bit) when the timeout
>>>> expires.
>>>>
>>>> The troublesome commit caused 2 issues:
>>>> 1) An IMHO unacceptable long timeout (0.5 seconds of near busy
>>>> waiting
>>>> on each enable
>>>> )
>>>
>>> The current clocks and divisor values allow us to set pulses up to
>>> 218 ms. I have no idea how we can decrease this significantly. 300
>>> ms?
>>> Or other way which I proposed to Mika and Ilkka during internal
>>> review
>>> is to calculate it from last cycle.
>>
>> Right, I was not clear, sorry I mean this is a problem because
>> normally we should not wait so long, but now on Cherry Trail
>> (and others) we do wait so long as the enabled bit is not set.
>>
>> What I was trying to say is that the code was already wrongly
>> waiting for the timeout to expire (since update would never
>> clear before commit 10d56a4cb1c6 already) but this was not a big
>> deal as it was not waiting for a long time.
>>
>>>> 2) Erroring out when the timeout expires without the update flag
>>>> clearing
>>>
>>> What would we do in such case? There is no recovery for us except
>>> waiting more.
>>
>> Same here, again what I meant is that the commit makes the already
>> wrong behavior from before a problem because it has turned it
>> into an error condition. The proper fix is of course to make sure
>> we do not hit the timeout by setting enabled before waiting for
>> the update bit to clear on all hardware except broxton.
>
> Thanks for elaborative message.
> Feel free to use my patch as a base for yours to provide a quirk-based
> solution. I would test it on my side for 4 platforms I have PWM enabled
> on.

I was kinda expecting you or Ilkka to do a new version, as I don't
have access to Apollo Lake hardware and atm also not really much time
for this.

Regards,

Hans

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

[Andy Shevchenko]

On Tue, 2017-04-04 at 19:02 +0200, Hans de Goede wrote:
> Hi,
> 
> On 04/04/2017 06:20 PM, Andy Shevchenko wrote:
> > On Mon, 2017-04-03 at 16:32 +0200, Hans de Goede wrote:
> > > HI,
> > > 
> > > On 03-04-17 16:22, Andy Shevchenko wrote:
> > > > On Fri, 2017-03-31 at 22:52 +0200, Hans de Goede wrote:
> > > > > Hi,
> > > > > 
> > > > > On 31-03-17 22:07, Andy Shevchenko wrote:
> > > > > > On Wed, 2017-03-29 at 20:25 +0200, Hans de Goede wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > On 29-03-17 19:41, Andy Shevchenko wrote:
> > > > > > > > On Wed, 2017-03-29 at 14:42 +0200, Hans de Goede wrote:
> > > > > > 
> > > > > > Thanks for your patience and valuable input.
> > > > > > 
> > > > > > So, I found CharryTrail with enabled PWM (UP board v0.4) and
> > > > > > confirm
> > > > > > the
> > > > > > bug.
> > > > > > 
> > > > > > Moreover, I have re-tested again all 4 platforms with and
> > > > > > without
> > > > > > your
> > > > > > fix, and I dunno how I did not notice this before, but looks
> > > > > > like
> > > > > > either
> > > > > > mine (though commit message shows that I have tested on 3
> > > > > > platforms
> > > > > > at
> > > > > > least, so, I can re-test for sure) or Ilkka's patch broke it
> > > > > > on
> > > > > > all
> > > > > > platforms except Broxton / Apollo Lake.
> > > > > > 
> > > > > > So, summurize what we need is a quirk for Broxton / Apollo
> > > > > > Lake.
> > > > > > I
> > > > > > need
> > > > > > to check Gemini Lake also to be sure.
> > > > > > 
> > > > > > And we definitely need this as a fix for stable. I would
> > > > > > appreciate
> > > > > > if
> > > > > > you can figure out which patch from previous series
> > > > > > (b14e8ceff034
> > > > > > or 10d56a4cb1c6) broke it.
> > > > > 
> > > > > My patch commit msg says: "fixes
> > > > > 10d56a4cb1c6c894c60acbaec0f8aa44aba833b0"
> > > > > and unless my memory deceives me I tested that that was the
> > > > > bad
> > > > > commit
> > > > > by reverting it.
> > > > 
> > > > Oh, yes, thanks for pointing this out.
> > > > 
> > > > > 
> > > > > The problem for Cherry Trail is not perse the order in which
> > > > > the
> > > > > update / enable bits are written (AFAICT), but the waiting for
> > > > > the update bit to clear while the enable bit is not set, which
> > > > > is why my latest version only moves the wait.
> > > > 
> > > > And it looks sane.
> > > > 
> > > > > That wait was already present before 10d56a4cb1c6, but with a
> > > > > reasonable short timeout, and just continuing on after the
> > > > > timeout. 10d56a4cb1c6 makes pwm_lpss_apply() exit with an
> > > > > error (without ever setting the enable bit) when the timeout
> > > > > expires.
> > > > > 
> > > > > The troublesome commit caused 2 issues:
> > > > > 1) An IMHO unacceptable long timeout (0.5 seconds of near busy
> > > > > waiting
> > > > > on each enable
> > > > > )
> > > > 
> > > > The current clocks and divisor values allow us to set pulses up
> > > > to
> > > > 218 ms. I have no idea how we can decrease this significantly.
> > > > 300
> > > > ms?
> > > > Or other way which I proposed to Mika and Ilkka during internal
> > > > review
> > > > is to calculate it from last cycle.
> > > 
> > > Right, I was not clear, sorry I mean this is a problem because
> > > normally we should not wait so long, but now on Cherry Trail
> > > (and others) we do wait so long as the enabled bit is not set.
> > > 
> > > What I was trying to say is that the code was already wrongly
> > > waiting for the timeout to expire (since update would never
> > > clear before commit 10d56a4cb1c6 already) but this was not a big
> > > deal as it was not waiting for a long time.
> > > 
> > > > > 2) Erroring out when the timeout expires without the update
> > > > > flag
> > > > > clearing
> > > > 
> > > > What would we do in such case? There is no recovery for us
> > > > except
> > > > waiting more.
> > > 
> > > Same here, again what I meant is that the commit makes the already
> > > wrong behavior from before a problem because it has turned it
> > > into an error condition. The proper fix is of course to make sure
> > > we do not hit the timeout by setting enabled before waiting for
> > > the update bit to clear on all hardware except broxton.
> > 
> > Thanks for elaborative message.
> > Feel free to use my patch as a base for yours to provide a quirk-
> > based
> > solution. I would test it on my side for 4 platforms I have PWM
> > enabled
> > on.
> 
> I was kinda expecting you or Ilkka to do a new version, as I don't
> have access to Apollo Lake hardware and atm also not really much time
> for this.

Okay, I can do it, but it will be delayed a bit.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy