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@xxxxxxxxxxxxxxx> Intel Finland Oy -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
