+Cc: Jarkko and Ulf (they are discussing patch series regarding to I2C and ACPI LPSS as well) +Cc: Mika, he might have a sight on this as well. 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.git/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
