* [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit @ 2017-03-25 14:06 Hans de Goede 2017-03-25 14:06 ` [PATCH v2 resend] pwm: lpss: Set enable-bit before waiting for update-bit to go low Hans de Goede ` (2 more replies) 0 siblings, 3 replies; 22+ messages in thread From: Hans de Goede @ 2017-03-25 14:06 UTC (permalink / raw) To: Thierry Reding, Andy Shevchenko; +Cc: Hans de Goede, linux-pwm 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 resend] pwm: lpss: Set enable-bit before waiting for update-bit to go low 2017-03-25 14:06 [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit Hans de Goede @ 2017-03-25 14:06 ` Hans de Goede 2017-03-26 12:25 ` [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit Andy Shevchenko 2017-03-28 14:45 ` Andy Shevchenko 2 siblings, 0 replies; 22+ messages in thread From: Hans de Goede @ 2017-03-25 14:06 UTC (permalink / raw) To: Thierry Reding, Andy Shevchenko; +Cc: Hans de Goede, linux-pwm, Ilkka Koskinen 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit 2017-03-25 14:06 [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit Hans de Goede 2017-03-25 14:06 ` [PATCH v2 resend] pwm: lpss: Set enable-bit before waiting for update-bit to go low Hans de Goede @ 2017-03-26 12:25 ` Andy Shevchenko 2017-03-26 14:44 ` Hans de Goede 2017-03-27 22:14 ` Ilkka Koskinen 2017-03-28 14:45 ` Andy Shevchenko 2 siblings, 2 replies; 22+ messages in thread From: Andy Shevchenko @ 2017-03-26 12:25 UTC (permalink / raw) To: Hans de Goede, Thierry Reding; +Cc: linux-pwm, Koskinen, Ilkka 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit 2017-03-26 12:25 ` [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit Andy Shevchenko @ 2017-03-26 14:44 ` Hans de Goede 2017-03-27 22:14 ` Ilkka Koskinen 1 sibling, 0 replies; 22+ messages in thread From: Hans de Goede @ 2017-03-26 14:44 UTC (permalink / raw) To: Andy Shevchenko, Thierry Reding; +Cc: linux-pwm, Koskinen, Ilkka 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit 2017-03-26 12:25 ` [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit Andy Shevchenko 2017-03-26 14:44 ` Hans de Goede @ 2017-03-27 22:14 ` Ilkka Koskinen 1 sibling, 0 replies; 22+ messages in thread From: Ilkka Koskinen @ 2017-03-27 22:14 UTC (permalink / raw) To: Andy Shevchenko; +Cc: Hans de Goede, Thierry Reding, linux-pwm 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit 2017-03-25 14:06 [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit Hans de Goede 2017-03-25 14:06 ` [PATCH v2 resend] pwm: lpss: Set enable-bit before waiting for update-bit to go low Hans de Goede 2017-03-26 12:25 ` [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit Andy Shevchenko @ 2017-03-28 14:45 ` Andy Shevchenko 2017-03-28 17:20 ` Hans de Goede 2017-03-29 4:50 ` Ilkka Koskinen 2 siblings, 2 replies; 22+ messages in thread From: Andy Shevchenko @ 2017-03-28 14:45 UTC (permalink / raw) To: Hans de Goede, Thierry Reding; +Cc: linux-pwm, Koskinen, Ilkka 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit 2017-03-28 14:45 ` Andy Shevchenko @ 2017-03-28 17:20 ` Hans de Goede 2017-03-28 17:28 ` Andy Shevchenko 2017-03-29 4:50 ` Ilkka Koskinen 1 sibling, 1 reply; 22+ messages in thread From: Hans de Goede @ 2017-03-28 17:20 UTC (permalink / raw) To: Andy Shevchenko, Thierry Reding; +Cc: linux-pwm, Koskinen, Ilkka 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. > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit 2017-03-28 17:20 ` Hans de Goede @ 2017-03-28 17:28 ` Andy Shevchenko 2017-03-28 17:33 ` Andy Shevchenko 0 siblings, 1 reply; 22+ messages in thread From: Andy Shevchenko @ 2017-03-28 17:28 UTC (permalink / raw) To: Hans de Goede, Thierry Reding; +Cc: linux-pwm, Koskinen, Ilkka 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit 2017-03-28 17:28 ` Andy Shevchenko @ 2017-03-28 17:33 ` Andy Shevchenko 2017-03-28 19:16 ` Hans de Goede 0 siblings, 1 reply; 22+ messages in thread From: Andy Shevchenko @ 2017-03-28 17:33 UTC (permalink / raw) To: Hans de Goede, Thierry Reding; +Cc: linux-pwm, Koskinen, Ilkka 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit 2017-03-28 17:33 ` Andy Shevchenko @ 2017-03-28 19:16 ` Hans de Goede 2017-03-29 11:24 ` Andy Shevchenko 0 siblings, 1 reply; 22+ messages in thread From: Hans de Goede @ 2017-03-28 19:16 UTC (permalink / raw) To: Andy Shevchenko, Thierry Reding; +Cc: linux-pwm, Koskinen, Ilkka 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 > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit 2017-03-28 19:16 ` Hans de Goede @ 2017-03-29 11:24 ` Andy Shevchenko 2017-03-29 12:42 ` Hans de Goede 0 siblings, 1 reply; 22+ messages in thread From: Andy Shevchenko @ 2017-03-29 11:24 UTC (permalink / raw) To: Hans de Goede, Thierry Reding; +Cc: linux-pwm, Koskinen, Ilkka 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit 2017-03-29 11:24 ` Andy Shevchenko @ 2017-03-29 12:42 ` Hans de Goede 2017-03-29 17:41 ` Andy Shevchenko 0 siblings, 1 reply; 22+ messages in thread From: Hans de Goede @ 2017-03-29 12:42 UTC (permalink / raw) To: Andy Shevchenko, Thierry Reding; +Cc: linux-pwm, Koskinen, Ilkka 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit 2017-03-29 12:42 ` Hans de Goede @ 2017-03-29 17:41 ` Andy Shevchenko 2017-03-29 18:25 ` Hans de Goede 0 siblings, 1 reply; 22+ messages in thread From: Andy Shevchenko @ 2017-03-29 17:41 UTC (permalink / raw) To: Hans de Goede, Thierry Reding; +Cc: linux-pwm, Koskinen, Ilkka 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit 2017-03-29 17:41 ` Andy Shevchenko @ 2017-03-29 18:25 ` Hans de Goede 2017-03-31 20:07 ` Andy Shevchenko 0 siblings, 1 reply; 22+ messages in thread From: Hans de Goede @ 2017-03-29 18:25 UTC (permalink / raw) To: Andy Shevchenko, Thierry Reding; +Cc: linux-pwm, Koskinen, Ilkka 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit 2017-03-29 18:25 ` Hans de Goede @ 2017-03-31 20:07 ` Andy Shevchenko 2017-03-31 20:52 ` Hans de Goede 0 siblings, 1 reply; 22+ messages in thread From: Andy Shevchenko @ 2017-03-31 20:07 UTC (permalink / raw) To: Hans de Goede, Thierry Reding; +Cc: linux-pwm, Koskinen, Ilkka 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit 2017-03-31 20:07 ` Andy Shevchenko @ 2017-03-31 20:52 ` Hans de Goede 2017-04-03 14:22 ` Andy Shevchenko 0 siblings, 1 reply; 22+ messages in thread From: Hans de Goede @ 2017-03-31 20:52 UTC (permalink / raw) To: Andy Shevchenko, Thierry Reding; +Cc: linux-pwm, Koskinen, Ilkka 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit 2017-03-31 20:52 ` Hans de Goede @ 2017-04-03 14:22 ` Andy Shevchenko 2017-04-03 14:32 ` Hans de Goede 0 siblings, 1 reply; 22+ messages in thread From: Andy Shevchenko @ 2017-04-03 14:22 UTC (permalink / raw) To: Hans de Goede, Thierry Reding; +Cc: linux-pwm, Koskinen, Ilkka 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit 2017-04-03 14:22 ` Andy Shevchenko @ 2017-04-03 14:32 ` Hans de Goede 2017-04-04 16:20 ` Andy Shevchenko 0 siblings, 1 reply; 22+ messages in thread From: Hans de Goede @ 2017-04-03 14:32 UTC (permalink / raw) To: Andy Shevchenko, Thierry Reding; +Cc: linux-pwm, Koskinen, Ilkka 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit 2017-04-03 14:32 ` Hans de Goede @ 2017-04-04 16:20 ` Andy Shevchenko 2017-04-04 17:02 ` Hans de Goede 0 siblings, 1 reply; 22+ messages in thread From: Andy Shevchenko @ 2017-04-04 16:20 UTC (permalink / raw) To: Hans de Goede, Thierry Reding; +Cc: linux-pwm, Koskinen, Ilkka 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit 2017-04-04 16:20 ` Andy Shevchenko @ 2017-04-04 17:02 ` Hans de Goede 2017-04-04 17:23 ` Andy Shevchenko 0 siblings, 1 reply; 22+ messages in thread From: Hans de Goede @ 2017-04-04 17:02 UTC (permalink / raw) To: Andy Shevchenko, Thierry Reding; +Cc: linux-pwm, Koskinen, Ilkka 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit 2017-04-04 17:02 ` Hans de Goede @ 2017-04-04 17:23 ` Andy Shevchenko 0 siblings, 0 replies; 22+ messages in thread From: Andy Shevchenko @ 2017-04-04 17:23 UTC (permalink / raw) To: Hans de Goede, Thierry Reding; +Cc: linux-pwm, Koskinen, Ilkka 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit 2017-03-28 14:45 ` Andy Shevchenko 2017-03-28 17:20 ` Hans de Goede @ 2017-03-29 4:50 ` Ilkka Koskinen 1 sibling, 0 replies; 22+ messages in thread From: Ilkka Koskinen @ 2017-03-29 4:50 UTC (permalink / raw) To: Andy Shevchenko; +Cc: Hans de Goede, Thierry Reding, linux-pwm 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2017-04-04 17:23 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-25 14:06 [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit Hans de Goede 2017-03-25 14:06 ` [PATCH v2 resend] pwm: lpss: Set enable-bit before waiting for update-bit to go low Hans de Goede 2017-03-26 12:25 ` [PATCH REGRESSION-FIX resend] pwm: lpss: Set enable-bit before waiting for update-bit Andy Shevchenko 2017-03-26 14:44 ` Hans de Goede 2017-03-27 22:14 ` Ilkka Koskinen 2017-03-28 14:45 ` Andy Shevchenko 2017-03-28 17:20 ` Hans de Goede 2017-03-28 17:28 ` Andy Shevchenko 2017-03-28 17:33 ` Andy Shevchenko 2017-03-28 19:16 ` Hans de Goede 2017-03-29 11:24 ` Andy Shevchenko 2017-03-29 12:42 ` Hans de Goede 2017-03-29 17:41 ` Andy Shevchenko 2017-03-29 18:25 ` Hans de Goede 2017-03-31 20:07 ` Andy Shevchenko 2017-03-31 20:52 ` Hans de Goede 2017-04-03 14:22 ` Andy Shevchenko 2017-04-03 14:32 ` Hans de Goede 2017-04-04 16:20 ` Andy Shevchenko 2017-04-04 17:02 ` Hans de Goede 2017-04-04 17:23 ` Andy Shevchenko 2017-03-29 4:50 ` Ilkka Koskinen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.