* [PATCH 1/2] ACPI / LPSS: Call lpss_iosf_exit_d3_state on device activate @ 2017-06-26 21:56 Hans de Goede 2017-06-26 21:57 ` [PATCH 2/2] ACPI / LPSS: Remove lpss_iosf_enter_d3_state() Hans de Goede 2017-06-28 17:29 ` [PATCH 1/2] ACPI / LPSS: Call lpss_iosf_exit_d3_state on device activate kbuild test robot 0 siblings, 2 replies; 6+ messages in thread From: Hans de Goede @ 2017-06-26 21:56 UTC (permalink / raw) To: Rafael J . Wysocki, Len Brown, Andy Shevchenko; +Cc: Hans de Goede, linux-acpi The purpose of lpss_iosf_exit_d3_state is to ensure that the LPSS DMA controllers are in D0 and will not automatically power-down. Since we do not know what the state is when activating a device, we should call lpss_iosf_exit_d3_state on activate too. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/acpi/acpi_lpss.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c index 43f376aa7adc..51feee309b13 100644 --- a/drivers/acpi/acpi_lpss.c +++ b/drivers/acpi/acpi_lpss.c @@ -97,6 +97,8 @@ struct lpss_private_data { u32 prv_reg_ctx[LPSS_PRV_REG_COUNT]; }; +static void lpss_iosf_exit_d3_state(void); + /* LPSS run time quirks */ static unsigned int lpss_quirks; @@ -682,6 +684,9 @@ static int acpi_lpss_activate(struct device *dev) struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev)); int ret; + if (lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available()) + lpss_iosf_exit_d3_state(); + ret = acpi_dev_runtime_resume(dev); if (ret) return ret; -- 2.13.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] ACPI / LPSS: Remove lpss_iosf_enter_d3_state() 2017-06-26 21:56 [PATCH 1/2] ACPI / LPSS: Call lpss_iosf_exit_d3_state on device activate Hans de Goede @ 2017-06-26 21:57 ` Hans de Goede 2017-06-27 13:31 ` Andy Shevchenko 2017-06-28 17:29 ` [PATCH 1/2] ACPI / LPSS: Call lpss_iosf_exit_d3_state on device activate kbuild test robot 1 sibling, 1 reply; 6+ messages in thread From: Hans de Goede @ 2017-06-26 21:57 UTC (permalink / raw) To: Rafael J . Wysocki, Len Brown, Andy Shevchenko; +Cc: Hans de Goede, linux-acpi If lpss_iosf_enter_d3_state() hits the code path where it actually puts the DMA controllers into D3, then, at least on Bay Trail, the LPSS PWM controller will stop working / gets stuck. After this happening the PWM controller's control reg will always reads 0x00010000 and writes seem to be ignored. Note that the chances of this code-path actually being hit are actually very low. On Bay Trail devices with an AXP288 PMIC and on any Cherry Trail device, the I2C controller connected to the PMIC has (runtime) suspend disabled, so the condition of all LPSS and SCC devices being in D3 will never happen. Even on Bay Trail devices with another PMIC testing has shown that lpss_iosf_enter_d3_state() will only put the DMA controllers in D3 during early boot when not all devices have been initialized yet, which is enough to get the PWM controller stuck, while not resulting in any significant power saving as this only happens during boot. So in practice lpss_iosf_enter_d3_state() is almost always a no-op and when it is not, it is causing problems. Therefor this commit simply removes it. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/acpi/acpi_lpss.c | 61 +----------------------------------------------- 1 file changed, 1 insertion(+), 60 deletions(-) diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c index 51feee309b13..8ea8c0988381 100644 --- a/drivers/acpi/acpi_lpss.c +++ b/drivers/acpi/acpi_lpss.c @@ -762,51 +762,6 @@ static int acpi_lpss_resume_early(struct device *dev) static DEFINE_MUTEX(lpss_iosf_mutex); -static void lpss_iosf_enter_d3_state(void) -{ - u32 value1 = 0; - u32 mask1 = LPSS_GPIODEF0_DMA_D3_MASK | LPSS_GPIODEF0_DMA_LLP; - u32 value2 = LPSS_PMCSR_D3hot; - u32 mask2 = LPSS_PMCSR_Dx_MASK; - /* - * PMC provides an information about actual status of the LPSS devices. - * Here we read the values related to LPSS power island, i.e. LPSS - * devices, excluding both LPSS DMA controllers, along with SCC domain. - */ - u32 func_dis, d3_sts_0, pmc_status, pmc_mask = 0xfe000ffe; - int ret; - - ret = pmc_atom_read(PMC_FUNC_DIS, &func_dis); - if (ret) - return; - - mutex_lock(&lpss_iosf_mutex); - - ret = pmc_atom_read(PMC_D3_STS_0, &d3_sts_0); - if (ret) - goto exit; - - /* - * Get the status of entire LPSS power island per device basis. - * Shutdown both LPSS DMA controllers if and only if all other devices - * are already in D3hot. - */ - pmc_status = (~(d3_sts_0 | func_dis)) & pmc_mask; - if (pmc_status) - goto exit; - - 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); -exit: - mutex_unlock(&lpss_iosf_mutex); -} - static void lpss_iosf_exit_d3_state(void) { u32 value1 = LPSS_GPIODEF0_DMA1_D3 | LPSS_GPIODEF0_DMA2_D3 | @@ -841,17 +796,7 @@ static int acpi_lpss_runtime_suspend(struct device *dev) if (pdata->dev_desc->flags & LPSS_SAVE_CTX) acpi_lpss_save_ctx(dev, pdata); - ret = acpi_dev_runtime_suspend(dev); - - /* - * This call must be last in the sequence, otherwise PMC will return - * wrong status for devices being about to be powered off. See - * lpss_iosf_enter_d3_state() for further information. - */ - if (lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available()) - lpss_iosf_enter_d3_state(); - - return ret; + return acpi_dev_runtime_suspend(dev); } static int acpi_lpss_runtime_resume(struct device *dev) @@ -859,10 +804,6 @@ static int acpi_lpss_runtime_resume(struct device *dev) struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev)); int ret; - /* - * This call is kept first to be in symmetry with - * acpi_lpss_runtime_suspend() one. - */ if (lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available()) lpss_iosf_exit_d3_state(); -- 2.13.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ACPI / LPSS: Remove lpss_iosf_enter_d3_state() 2017-06-26 21:57 ` [PATCH 2/2] ACPI / LPSS: Remove lpss_iosf_enter_d3_state() Hans de Goede @ 2017-06-27 13:31 ` Andy Shevchenko 2017-06-27 13:56 ` Hans de Goede 0 siblings, 1 reply; 6+ messages in thread From: Andy Shevchenko @ 2017-06-27 13:31 UTC (permalink / raw) To: Hans de Goede, Rafael J . Wysocki, Len Brown; +Cc: linux-acpi On Mon, 2017-06-26 at 23:57 +0200, Hans de Goede wrote: I didn't get why the first patch exists if you effectively remove what it brought here. > If lpss_iosf_enter_d3_state() hits the code path where it actually > puts the DMA controllers into D3, then, at least on Bay Trail, the > LPSS > PWM controller will stop working / gets stuck. After this happening > the > PWM controller's control reg will always reads 0x00010000 and writes > seem to be ignored. > > Note that the chances of this code-path actually being hit are > actually > very low. On Bay Trail devices with an AXP288 PMIC and on any Cherry > Trail > device, the I2C controller connected to the PMIC has (runtime) suspend > disabled, so the condition of all LPSS and SCC devices being in D3 > will > never happen. > > Even on Bay Trail devices with another PMIC testing has shown that > lpss_iosf_enter_d3_state() will only put the DMA controllers in D3 > during > early boot when not all devices have been initialized yet, which is > enough > to get the PWM controller stuck, while not resulting in any > significant > power saving as this only happens during boot. > > So in practice lpss_iosf_enter_d3_state() is almost always a no-op and > when it is not, it is causing problems. Therefor this commit simply > removes it. Let's continue discuss in your letter regarding IOSF LPSS stuff. -- Andy Shevchenko <andriy.shevchenko@linux.intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ACPI / LPSS: Remove lpss_iosf_enter_d3_state() 2017-06-27 13:31 ` Andy Shevchenko @ 2017-06-27 13:56 ` Hans de Goede 2017-06-27 14:38 ` Andy Shevchenko 0 siblings, 1 reply; 6+ messages in thread From: Hans de Goede @ 2017-06-27 13:56 UTC (permalink / raw) To: Andy Shevchenko, Rafael J . Wysocki, Len Brown; +Cc: linux-acpi Hi, On 06/27/2017 03:31 PM, Andy Shevchenko wrote: > On Mon, 2017-06-26 at 23:57 +0200, Hans de Goede wrote: > > I didn't get why the first patch exists if you effectively remove what > it brought here. The first patch calls lpss_iosf_exit_d3_state() on activate, but if you're right that the probe functions will cause a runtime_resume when enabling runtime-pm then that may not be necessary. If that is right it does make one wonder why we have the activate function at all though? I guess to de-assert the reset ? But shouldn't that then not be all it does ? > >> If lpss_iosf_enter_d3_state() hits the code path where it actually >> puts the DMA controllers into D3, then, at least on Bay Trail, the >> LPSS >> PWM controller will stop working / gets stuck. After this happening >> the >> PWM controller's control reg will always reads 0x00010000 and writes >> seem to be ignored. >> >> Note that the chances of this code-path actually being hit are >> actually >> very low. On Bay Trail devices with an AXP288 PMIC and on any Cherry >> Trail >> device, the I2C controller connected to the PMIC has (runtime) suspend >> disabled, so the condition of all LPSS and SCC devices being in D3 >> will >> never happen. >> >> Even on Bay Trail devices with another PMIC testing has shown that >> lpss_iosf_enter_d3_state() will only put the DMA controllers in D3 >> during >> early boot when not all devices have been initialized yet, which is >> enough >> to get the PWM controller stuck, while not resulting in any >> significant >> power saving as this only happens during boot. >> >> So in practice lpss_iosf_enter_d3_state() is almost always a no-op and >> when it is not, it is causing problems. Therefor this commit simply >> removes it. > > Let's continue discuss in your letter regarding IOSF LPSS stuff. Ok. Regards, Hans ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ACPI / LPSS: Remove lpss_iosf_enter_d3_state() 2017-06-27 13:56 ` Hans de Goede @ 2017-06-27 14:38 ` Andy Shevchenko 0 siblings, 0 replies; 6+ messages in thread From: Andy Shevchenko @ 2017-06-27 14:38 UTC (permalink / raw) To: Hans de Goede, Rafael J . Wysocki, Len Brown; +Cc: linux-acpi On Tue, 2017-06-27 at 15:56 +0200, Hans de Goede wrote: > Hi, > > On 06/27/2017 03:31 PM, Andy Shevchenko wrote: > > On Mon, 2017-06-26 at 23:57 +0200, Hans de Goede wrote: > > > > I didn't get why the first patch exists if you effectively remove > > what > > it brought here. > > The first patch calls lpss_iosf_exit_d3_state() on activate, but > if you're right that the probe functions will cause a runtime_resume > when enabling runtime-pm then that may not be necessary. If that is > right it does make one wonder why we have the activate function at all > though? I guess to de-assert the reset ? But shouldn't that then not > be > all it does ? Sorry for misleading comment. ACPI (and so platform drivers) should take care of power (runtime PM) by themselves. So, I dunno how to make the code less uglifying except put pm_runtime_get_sync() on each ->probe() of LPSS drivers. Perhaps your patch does that in better way and we may get rid of DW DMA uglified piece of code. > > > > > > If lpss_iosf_enter_d3_state() hits the code path where it actually > > > puts the DMA controllers into D3, then, at least on Bay Trail, the > > > LPSS > > > PWM controller will stop working / gets stuck. After this > > > happening > > > the > > > PWM controller's control reg will always reads 0x00010000 and > > > writes > > > seem to be ignored. > > > > > > Note that the chances of this code-path actually being hit are > > > actually > > > very low. On Bay Trail devices with an AXP288 PMIC and on any > > > Cherry > > > Trail > > > device, the I2C controller connected to the PMIC has (runtime) > > > suspend > > > disabled, so the condition of all LPSS and SCC devices being in D3 > > > will > > > never happen. > > > > > > Even on Bay Trail devices with another PMIC testing has shown that > > > lpss_iosf_enter_d3_state() will only put the DMA controllers in D3 > > > during > > > early boot when not all devices have been initialized yet, which > > > is > > > enough > > > to get the PWM controller stuck, while not resulting in any > > > significant > > > power saving as this only happens during boot. > > > > > > So in practice lpss_iosf_enter_d3_state() is almost always a no-op > > > and > > > when it is not, it is causing problems. Therefor this commit > > > simply > > > removes it. > > > > Let's continue discuss in your letter regarding IOSF LPSS stuff. > > Ok. > > Regards, > > Hans -- Andy Shevchenko <andriy.shevchenko@linux.intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] ACPI / LPSS: Call lpss_iosf_exit_d3_state on device activate 2017-06-26 21:56 [PATCH 1/2] ACPI / LPSS: Call lpss_iosf_exit_d3_state on device activate Hans de Goede 2017-06-26 21:57 ` [PATCH 2/2] ACPI / LPSS: Remove lpss_iosf_enter_d3_state() Hans de Goede @ 2017-06-28 17:29 ` kbuild test robot 1 sibling, 0 replies; 6+ messages in thread From: kbuild test robot @ 2017-06-28 17:29 UTC (permalink / raw) Cc: kbuild-all, Rafael J . Wysocki, Len Brown, Andy Shevchenko, Hans de Goede, linux-acpi [-- Attachment #1: Type: text/plain, Size: 1739 bytes --] Hi Hans, [auto build test WARNING on pm/linux-next] [also build test WARNING on v4.12-rc7 next-20170628] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Hans-de-Goede/ACPI-LPSS-Call-lpss_iosf_exit_d3_state-on-device-activate/20170628-072414 base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next config: x86_64-randconfig-b0-06282134 (attached as .config) compiler: gcc-4.4 (Debian 4.4.7-8) 4.4.7 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All warnings (new ones prefixed by >>): >> drivers/acpi/acpi_lpss.c:96: warning: 'lpss_iosf_exit_d3_state' declared 'static' but never defined vim +96 drivers/acpi/acpi_lpss.c 80 void (*setup)(struct lpss_private_data *pdata); 81 }; 82 83 static const struct lpss_device_desc lpss_dma_desc = { 84 .flags = LPSS_CLK, 85 }; 86 87 struct lpss_private_data { 88 void __iomem *mmio_base; 89 resource_size_t mmio_size; 90 unsigned int fixed_clk_rate; 91 struct clk *clk; 92 const struct lpss_device_desc *dev_desc; 93 u32 prv_reg_ctx[LPSS_PRV_REG_COUNT]; 94 }; 95 > 96 static void lpss_iosf_exit_d3_state(void); 97 98 /* LPSS run time quirks */ 99 static unsigned int lpss_quirks; 100 101 /* 102 * LPSS_QUIRK_ALWAYS_POWER_ON: override power state for LPSS DMA device. 103 * 104 * The LPSS DMA controller has neither _PS0 nor _PS3 method. Moreover --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 25597 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-06-28 17:29 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-26 21:56 [PATCH 1/2] ACPI / LPSS: Call lpss_iosf_exit_d3_state on device activate Hans de Goede 2017-06-26 21:57 ` [PATCH 2/2] ACPI / LPSS: Remove lpss_iosf_enter_d3_state() Hans de Goede 2017-06-27 13:31 ` Andy Shevchenko 2017-06-27 13:56 ` Hans de Goede 2017-06-27 14:38 ` Andy Shevchenko 2017-06-28 17:29 ` [PATCH 1/2] ACPI / LPSS: Call lpss_iosf_exit_d3_state on device activate kbuild test robot
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.