Re: [PATCH] ACPI / LPSS: Remove overriding of powerstate for LPSS DMA device

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 06-09-18 18:55, Hans de Goede wrote:
Commit eebb3e8d8aaf ("ACPI / LPSS: override power state for LPSS DMA
device") was added to work around a shutdown / reboot problem reported
on some Bay Trail devices. Specifically this was reported on a HP
Pavilion x360 11" and on an Asus T100TA.

Before Commit 12864ff8545f ("ACPI / LPSS: Avoid PM quirks on suspend and
resume from hibernation") this code would force on the DMA controllers
and never turn them off again on systems where the conditions for
lpss_iosf_enter_d3_state() did not become true which is actually the case
on a lot of systems.

It turns out this was blocking entering of S0ix state on these systems,
now lpss_iosf_exit_d3_state() only forces the DMA controllers to D0
after the conditions in enter_d3_state() have become true at least once.

The Pavilion x360 11" uses S3 suspend, not suspend to idle, so
lpss_iosf_enter_d3_state() never gets called there and the fix has
effectively been disabled for this device.

On the Asus T100TA the conditions do become true, but the forcing to D0
state of the DMA controllers may still not happen when the system is booted
and then directly shutdown without a suspend/resume in between.

This made me wonder if this workaround is still necessary at all, or if
other fixes which have been done then means that this is no longer
necessary.

I've tested running a kernel with the workaround removed on an Asus T100TA
and on a HP stream x360 - 11, which is the same hardware (same mainboard)
as the Pavilion x360 11" with a lower-power version of the SoC.

I've further tested this on an Asus T200TA and a Toshiba Click Mini L9W-B
which are both devices of the same generation as the other 2.

Shutdown and reboot work fine on all these devices without the workaround.

So it seems that the workaround is no longer necessary and therefor this
commit removes it, leading to a substantial cleanup of the acpi_lpss code.

Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>

Self NACK, it looks like we still need this one some devices:

https://bugzilla.kernel.org/show_bug.cgi?id=200989

Still investigating further, but for now lets not apply this.

Regards,

Hans



---
  drivers/acpi/acpi_lpss.c | 163 ++-------------------------------------
  1 file changed, 5 insertions(+), 158 deletions(-)

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index 99ca9534384c..7a52ea82adce 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -15,7 +15,6 @@
  #include <linux/clk-provider.h>
  #include <linux/err.h>
  #include <linux/io.h>
-#include <linux/mutex.h>
  #include <linux/platform_device.h>
  #include <linux/platform_data/clk-lpss.h>
  #include <linux/platform_data/x86/pmc_atom.h>
@@ -31,10 +30,6 @@ ACPI_MODULE_NAME("acpi_lpss");
#ifdef CONFIG_X86_INTEL_LPSS -#include <asm/cpu_device_id.h>
-#include <asm/intel-family.h>
-#include <asm/iosf_mbi.h>
-
  #define LPSS_ADDR(desc) ((unsigned long)&desc)
#define LPSS_CLK_SIZE 0x04
@@ -99,23 +94,6 @@ struct lpss_private_data {
  	u32 prv_reg_ctx[LPSS_PRV_REG_COUNT];
  };
-/* LPSS run time quirks */
-static unsigned int lpss_quirks;
-
-/*
- * LPSS_QUIRK_ALWAYS_POWER_ON: override power state for LPSS DMA device.
- *
- * The LPSS DMA controller has neither _PS0 nor _PS3 method. Moreover
- * it can be powered off automatically whenever the last LPSS device goes down.
- * In case of no power any access to the DMA controller will hang the system.
- * The behaviour is reproduced on some HP laptops based on Intel BayTrail as
- * well as on ASuS T100TA transformer.
- *
- * This quirk overrides power state of entire LPSS island to keep DMA powered
- * on whenever we have at least one other device in use.
- */
-#define LPSS_QUIRK_ALWAYS_POWER_ON	BIT(0)
-
  /* UART Component Parameter Register */
  #define LPSS_UART_CPR			0xF4
  #define LPSS_UART_CPR_AFCE		BIT(4)
@@ -289,14 +267,6 @@ static const struct lpss_device_desc bsw_spi_dev_desc = {
  	.setup = lpss_deassert_reset,
  };
-#define ICPU(model) { X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY, }
-
-static const struct x86_cpu_id lpss_cpu_ids[] = {
-	ICPU(INTEL_FAM6_ATOM_SILVERMONT1),	/* Valleyview, Bay Trail */
-	ICPU(INTEL_FAM6_ATOM_AIRMONT),	/* Braswell, Cherry Trail */
-	{}
-};
-
  #else
#define LPSS_ADDR(desc) (0UL)
@@ -864,121 +834,14 @@ static void acpi_lpss_dismiss(struct device *dev)
  	acpi_dev_suspend(dev, false);
  }
-/* IOSF SB for LPSS island */
-#define LPSS_IOSF_UNIT_LPIOEP		0xA0
-#define LPSS_IOSF_UNIT_LPIO1		0xAB
-#define LPSS_IOSF_UNIT_LPIO2		0xAC
-
-#define LPSS_IOSF_PMCSR			0x84
-#define LPSS_PMCSR_D0			0
-#define LPSS_PMCSR_D3hot		3
-#define LPSS_PMCSR_Dx_MASK		GENMASK(1, 0)
-
-#define LPSS_IOSF_GPIODEF0		0x154
-#define LPSS_GPIODEF0_DMA1_D3		BIT(2)
-#define LPSS_GPIODEF0_DMA2_D3		BIT(3)
-#define LPSS_GPIODEF0_DMA_D3_MASK	GENMASK(3, 2)
-#define LPSS_GPIODEF0_DMA_LLP		BIT(13)
-
-static DEFINE_MUTEX(lpss_iosf_mutex);
-static bool lpss_iosf_d3_entered;
-
-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);
-
-	lpss_iosf_d3_entered = true;
-
-exit:
-	mutex_unlock(&lpss_iosf_mutex);
-}
-
-static void lpss_iosf_exit_d3_state(void)
-{
-	u32 value1 = LPSS_GPIODEF0_DMA1_D3 | LPSS_GPIODEF0_DMA2_D3 |
-		     LPSS_GPIODEF0_DMA_LLP;
-	u32 mask1 = LPSS_GPIODEF0_DMA_D3_MASK | LPSS_GPIODEF0_DMA_LLP;
-	u32 value2 = LPSS_PMCSR_D0;
-	u32 mask2 = LPSS_PMCSR_Dx_MASK;
-
-	mutex_lock(&lpss_iosf_mutex);
-
-	if (!lpss_iosf_d3_entered)
-		goto exit;
-
-	lpss_iosf_d3_entered = false;
-
-	iosf_mbi_modify(LPSS_IOSF_UNIT_LPIOEP, MBI_CR_WRITE,
-			LPSS_IOSF_GPIODEF0, value1, mask1);
-
-	iosf_mbi_modify(LPSS_IOSF_UNIT_LPIO2, MBI_CFG_WRITE,
-			LPSS_IOSF_PMCSR, value2, mask2);
-
-	iosf_mbi_modify(LPSS_IOSF_UNIT_LPIO1, MBI_CFG_WRITE,
-			LPSS_IOSF_PMCSR, value2, mask2);
-
-exit:
-	mutex_unlock(&lpss_iosf_mutex);
-}
-
  static int acpi_lpss_suspend(struct device *dev, bool wakeup)
  {
  	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
-	int ret;
if (pdata->dev_desc->flags & LPSS_SAVE_CTX)
  		acpi_lpss_save_ctx(dev, pdata);
- ret = acpi_dev_suspend(dev, wakeup);
-
-	/*
-	 * 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 (acpi_target_system_state() == ACPI_STATE_S0 &&
-	    lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available())
-		lpss_iosf_enter_d3_state();
-
-	return ret;
+	return acpi_dev_suspend(dev, wakeup);
  }
static int acpi_lpss_resume(struct device *dev)
@@ -986,13 +849,6 @@ static int acpi_lpss_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();
-
  	ret = acpi_dev_resume(dev);
  	if (ret)
  		return ret;
@@ -1153,19 +1009,10 @@ static struct acpi_scan_handler lpss_handler = {
void __init acpi_lpss_init(void)
  {
-	const struct x86_cpu_id *id;
-	int ret;
-
-	ret = lpt_clk_init();
-	if (ret)
-		return;
-
-	id = x86_match_cpu(lpss_cpu_ids);
-	if (id)
-		lpss_quirks |= LPSS_QUIRK_ALWAYS_POWER_ON;
-
-	bus_register_notifier(&platform_bus_type, &acpi_lpss_nb);
-	acpi_scan_add_handler(&lpss_handler);
+	if (!lpt_clk_init()) {
+		bus_register_notifier(&platform_bus_type, &acpi_lpss_nb);
+		acpi_scan_add_handler(&lpss_handler);
+	}
  }
#else




[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux