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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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