Re: lpss_iosf_enter_d3_state breaks PWM on Baytrail

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

On Tue, 2017-06-27 at 16:15 +0200, Hans de Goede wrote:
> Hi,
> 
> On 06/27/2017 03:41 PM, Andy Shevchenko wrote:
> > +Cc: Jarkko and Ulf (they are discussing patch series regarding to
> > I2C
> > and ACPI LPSS as well)
> 
> Hmm, do you have a link to this patch series?

https://www.spinics.net/lists/linux-i2c/msg29078.html

In the thread Ulf mentioned his WIP branch.

> 
> > +Cc: Mika, he might have a sight on this as well.
> 
> I've added everyone in me reply to your first reply, but Jarkko's
> email address seems to not work.

Oops, wrong Jarkko. Now is proper one. (That's why entire message is
kept here)

> 
> Regards,
> 
> Hans
> 
> 
> > 
> > On Tue, 2017-06-27 at 16:29 +0300, Andy Shevchenko wrote:
> > > On Mon, 2017-06-26 at 11:31 +0200, Hans de Goede wrote:
> > > > Hi Andy, et al.,
> > > > 
> > > > I've spend quite some time this weekend debugging an issue
> > > > an a Bay Trail laptop with a Crystal Cove PMIC which uses
> > > > the LPSS PWM for backlight control rather then the PMIC's
> > > > PWM.
> > > > 
> > > > The problem is lpss_iosf_enter_d3_state. Specifically when
> > > > called when the PWM runtime-suspends during i915 driver load,
> > > > during which the i915 driver disables then re-enables the PWM.
> > > > 
> > > > This is the only time (including normal suspend(to-idle)
> > > > and resume) when this check:
> > > > 
> > > >           pmc_status = (~(d3_sts_0 | func_dis)) & pmc_mask;
> > > >           if (pmc_status)
> > > >                   goto exit;
> > > > 
> > > > Does not hit the goto exit path
> > > 
> > > ...meaning all LPSS devices (except maybe DMA) are in D3hot.
> > > 
> > > >   and lpss_iosf_enter_d3_state
> > > > actually executes these 3 statements:
> > > > 
> > > >           iosf_mbi_modify(LPSS_IOSF_UNIT_LPIO1, MBI_CFG_WRITE,
> > > >                           LPSS_IOSF_PMCSR, value2, mask2);
> > > > 
> > > >           iosf_mbi_modify(LPSS_IOSF_UNIT_LPIO2, MBI_CFG_WRITE,
> > > >                           LPSS_IOSF_PMCSR, value2, mask2);
> > > > 
> > > >           iosf_mbi_modify(LPSS_IOSF_UNIT_LPIOEP, MBI_CR_WRITE,
> > > >                           LPSS_IOSF_GPIODEF0, value1, mask1);
> > > > 
> > > > If I comment out these 3 statements then everything works fine,
> > > > with these 3 statements in place though, then the PWM controller
> > > > gets stuck when the i915 driver tries to re-enable it (and it
> > > > runtime-resumes). After these 3 statements have been run once
> > > > then any subsequent reads from the PWM's control register
> > > > always return 0x00010000 and writes seem to be ignored.
> > > 
> > > Sounds like powered off LPSS power island.
> > > 
> > > > Given the past troubles with the LPSS DMA controller suspend,
> > > > see all the links in this commit message (which introduces
> > > > the current fix) :
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.g
> > > > it/c
> > > > om
> > > > mit/?id=eebb3e8d8aaf2
> > > > 
> > > > I'm tempted to just rip out lpss_iosf_enter_d3_state,
> > > > especially since it does not seem to do anything other
> > > > then during early boot when not all drivers are loaded yet.
> > > 
> > > Interesting...
> > > 
> > > You are telling that there is guaranteed that at least one LPSS
> > > device
> > > (except DMA) will be on at any time?
> > > 
> > > > Note that on Bay Trail devices with an AXP288 PMIC (most new
> > > > Bay Trail devices) and on any Cherry Trail device the LPSS I2C
> > > > controller connected to the PMIC will never suspend because
> > > > some DSTDs make ACPI OpRegion calls during late suspend /
> > > > early resume and as such we need to keep the I2C bus alive
> > > > the whole time. This means that the check in
> > > > lpss_iosf_enter_d3_state will always hit the goto exit path
> > > > on these devices,
> > > 
> > > Looks like an answer to the previous question.
> > > 
> > > >   leaving just (older) Bay Trail + non AXP288
> > > > PMIC devices as potentially hitting the code path where
> > > > lpss_iosf_enter_d3_state actually does something and these are
> > > > exactly the devices with which people are having a lot of
> > > > problems with freezes / hangs.
> > > > 
> > > > So I believe that the best approach would be to simply remove
> > > > lpss_iosf_enter_d3_state. Which leaves me wondering what it
> > > > actually does. It would be nice for the future if undocumented
> > > > (not publicly documented) registers are used that some comments
> > > > are added describing what each register write actually does.
> > > 
> > > Do the commit messages describe this enough along with several
> > > comments
> > > inside acpi_lpss.c?
> > > 
> > > > These seem to simply put the DMA controller into D3:
> > > > 
> > > >           u32 value2 = LPSS_PMCSR_D3hot;
> > > >           u32 mask2 = LPSS_PMCSR_Dx_MASK;
> > > > 
> > > >           iosf_mbi_modify(LPSS_IOSF_UNIT_LPIO1, MBI_CFG_WRITE,
> > > >                           LPSS_IOSF_PMCSR, value2, mask2);
> > > > 
> > > >           iosf_mbi_modify(LPSS_IOSF_UNIT_LPIO2, MBI_CFG_WRITE,
> > > >                           LPSS_IOSF_PMCSR, value2, mask2);
> > > 
> > > It overrides the lines which propagate clock and reset signals to
> > > the
> > > entire LPSS islands.
> > > 
> > > DMA just happened to be a function 0 (in terms of PCI), so, if it
> > > is
> > > off
> > > the rest of the functions can not be on.
> > > 
> > > > But what does this one do?   :
> > > > 
> > > >           u32 value1 = 0;
> > > >           u32 mask1 = LPSS_GPIODEF0_DMA_D3_MASK |
> > > > LPSS_GPIODEF0_DMA_LLP;
> > > 
> > > (this one is a chicken bit to enable hardware linked list
> > > transfers on
> > > DMA)
> > > 
> > > > 
> > > >           iosf_mbi_modify(LPSS_IOSF_UNIT_LPIOEP, MBI_CR_WRITE,
> > > >                           LPSS_IOSF_GPIODEF0, value1, mask1);
> > > > 
> > > > I guess this turns on(?) the automatically down powering of the
> > > > DMA controllers ? If so then why not just do only that ?
> > > 
> > > No, automatic power gating is a *hardware* (PMC firmware? I do not
> > > remember details now) feature which is overridden by the code
> > > above.
> > > 
> > > It's something like logical gates where one of the input is what
> > > we
> > > supply through these chicken bits.
> > > 
> > > > And if this turns it on
> > > >   then lpss_iosf_exit_d3_state turns the
> > > > automagic off, if that is true,
> > > 
> > > The proper wording here I suppose *it allows or forbids the
> > > automatic
> > > power gating*. It's one layer up I would say. It means it might be
> > > a
> > > latency between writing these bits and actual hardware response on
> > > them.
> > > 
> > > >   then there is an ordering issue.
> > > > lpss_iosf_exit_d3_state gets only run on (runtime)resume, not
> > > > on the initial probe of LPSS devices, so that would mean that
> > > > between
> > > > the initial probe and the first resume we have the undesirable
> > > > auto-off feature active ?
> > > 
> > > Looks like ACPI PM for LPSS should take care of this at probe.
> > > When I
> > > wrote the code I was assuming that ACPI enumerated devices are
> > > runtime
> > > powered on during ->probe(). For such I did explicit calls in DMA
> > > driver: bb32baf76e56 ("dmaengine: dw: enable runtime PM").
> > > 

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