[PATCH v2 1/2] mmc: sdhci-acpi: Switch signal voltage back to 3.3V on suspend on external microSD on Lenovo Miix 320

[PATCH v2 1/2] mmc: sdhci-acpi: Switch signal voltage back to 3.3V on suspend on external microSD on Lenovo Miix 320

[Hans de Goede]

Based on a sample of 7 DSDTs from Cherry Trail devices using an AXP288
PMIC depending on the design one of 2 possible LDOs on the PMIC is used
for the MMC signalling voltage, either DLDO3 or GPIO1LDO (GPIO1 pin in
low noise LDO mode).

The Lenovo Miix 320-10ICR uses GPIO1LDO in the SHC1 ACPI device's DSM
methods to set 3.3 or 1.8 signalling voltage and this appears to work
as advertised, so presumably the device is actually using GPIO1LDO for
the external microSD signalling voltage.

But this device has a bug in the _PS0 method of the SHC1 ACPI device,
the DSM remembers the last set signalling voltage and the _PS0 restores
this after a (runtime) suspend-resume cycle, but it "restores" the voltage
on DLDO3 instead of setting it on GPIO1LDO as the DSM method does. DLDO3
is used for the LCD and setting it to 1.8V causes the LCD to go black.

This commit works around this issue by calling the Intel DSM to reset the
signal voltage to 3.3V after the host has been runtime suspended.
This will make the _PS0 method reprogram the DLDO3 voltage to 3.3V, which
leaves it at its original setting fixing the LCD going black.

And this commit then resets the signal voltage back to the original 1.8V
from the (runtime) resume handler, which runs after the ACPI _PS0 method
has run.

This commit adds and uses a DMI quirk mechanism to only trigger this
workaround on the Lenovo Miix 320 while leaving the behavior of the
driver unchanged on other devices.

BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=111294
BugLink: https://gitlab.freedesktop.org/drm/intel/issues/355
Reported-by: russianneuromancer <russianneuromancer@ya.ru>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Make the quirk reset the signal voltage to 3.3V at the end of the
  (runtime) suspend handler instead of disabling 1.8V modes
- Drop the module option to allow overridig the quirks
---
 drivers/mmc/host/sdhci-acpi.c | 87 ++++++++++++++++++++++++++++++++++-
 1 file changed, 85 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index 9651dca6863e..d54a3592f40f 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -23,6 +23,7 @@
 #include <linux/pm.h>
 #include <linux/pm_runtime.h>
 #include <linux/delay.h>
+#include <linux/dmi.h>
 
 #include <linux/mmc/host.h>
 #include <linux/mmc/pm.h>
@@ -72,9 +73,14 @@ struct sdhci_acpi_host {
 	const struct sdhci_acpi_slot	*slot;
 	struct platform_device		*pdev;
 	bool				use_runtime_pm;
+	bool				reset_signal_volt_on_suspend;
 	unsigned long			private[0] ____cacheline_aligned;
 };
 
+enum {
+	DMI_QUIRK_RESET_SD_SIGNAL_VOLT_ON_SUSP			= BIT(0),
+};
+
 static inline void *sdhci_acpi_priv(struct sdhci_acpi_host *c)
 {
 	return (void *)c->private;
@@ -647,6 +653,24 @@ static const struct acpi_device_id sdhci_acpi_ids[] = {
 };
 MODULE_DEVICE_TABLE(acpi, sdhci_acpi_ids);
 
+static const struct dmi_system_id sdhci_acpi_quirks[] = {
+	{
+		/*
+		 * The Lenovo Miix 320-10ICR has a bug in the _PS0 method of
+		 * the SHC1 ACPI device, this bug causes it to reprogram the
+		 * wrong LDO (DLDO3) to 1.8V if 1.8V modes are used and the
+		 * card is (runtime) suspended + resumed. DLDO3 is used for
+		 * the LCD and setting it to 1.8V causes the LCD to go black.
+		 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+			DMI_MATCH(DMI_PRODUCT_VERSION, "Lenovo MIIX 320-10ICR"),
+		},
+		.driver_data = (void *)DMI_QUIRK_RESET_SD_SIGNAL_VOLT_ON_SUSP,
+	},
+	{} /* Terminating entry */
+};
+
 static const struct sdhci_acpi_slot *sdhci_acpi_get_slot(struct acpi_device *adev)
 {
 	const struct sdhci_acpi_uid_slot *u;
@@ -663,17 +687,23 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	const struct sdhci_acpi_slot *slot;
 	struct acpi_device *device, *child;
+	const struct dmi_system_id *id;
 	struct sdhci_acpi_host *c;
 	struct sdhci_host *host;
 	struct resource *iomem;
 	resource_size_t len;
 	size_t priv_size;
+	int quirks = 0;
 	int err;
 
 	device = ACPI_COMPANION(dev);
 	if (!device)
 		return -ENODEV;
 
+	id = dmi_first_match(sdhci_acpi_quirks);
+	if (id)
+		quirks = (long)id->driver_data;
+
 	slot = sdhci_acpi_get_slot(device);
 
 	/* Power on the SDHCI controller and its children */
@@ -759,6 +789,9 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
 			dev_warn(dev, "failed to setup card detect gpio\n");
 			c->use_runtime_pm = false;
 		}
+
+		if (quirks & DMI_QUIRK_RESET_SD_SIGNAL_VOLT_ON_SUSP)
+			c->reset_signal_volt_on_suspend = true;
 	}
 
 	err = sdhci_setup_host(host);
@@ -823,17 +856,59 @@ static int sdhci_acpi_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static void __maybe_unused sdhci_acpi_reset_signal_voltage_if_needed(
+	struct device *dev)
+{
+	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
+	struct sdhci_host *host = c->host;
+
+	if (c->reset_signal_volt_on_suspend &&
+	    host->mmc_host_ops.start_signal_voltage_switch ==
+					intel_start_signal_voltage_switch &&
+	    host->mmc->ios.signal_voltage != MMC_SIGNAL_VOLTAGE_330) {
+		struct intel_host *intel_host = sdhci_acpi_priv(c);
+		unsigned int fn = INTEL_DSM_V33_SWITCH;
+		u32 result = 0;
+
+		intel_dsm(intel_host, dev, fn, &result);
+	}
+}
+
+static void __maybe_unused sdhci_acpi_restore_signal_voltage_if_needed(
+	struct device *dev)
+{
+	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
+	struct sdhci_host *host = c->host;
+
+	if (c->reset_signal_volt_on_suspend &&
+	    host->mmc_host_ops.start_signal_voltage_switch ==
+					intel_start_signal_voltage_switch &&
+	    host->mmc->ios.signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
+		struct intel_host *intel_host = sdhci_acpi_priv(c);
+		unsigned int fn = INTEL_DSM_V18_SWITCH;
+		u32 result = 0;
+
+		intel_dsm(intel_host, dev, fn, &result);
+	}
+}
+
 #ifdef CONFIG_PM_SLEEP
 
 static int sdhci_acpi_suspend(struct device *dev)
 {
 	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
 	struct sdhci_host *host = c->host;
+	int ret;
 
 	if (host->tuning_mode != SDHCI_TUNING_MODE_3)
 		mmc_retune_needed(host->mmc);
 
-	return sdhci_suspend_host(host);
+	ret = sdhci_suspend_host(host);
+	if (ret)
+		return ret;
+
+	sdhci_acpi_reset_signal_voltage_if_needed(dev);
+	return 0;
 }
 
 static int sdhci_acpi_resume(struct device *dev)
@@ -841,6 +916,7 @@ static int sdhci_acpi_resume(struct device *dev)
 	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
 
 	sdhci_acpi_byt_setting(&c->pdev->dev);
+	sdhci_acpi_restore_signal_voltage_if_needed(dev);
 
 	return sdhci_resume_host(c->host);
 }
@@ -853,11 +929,17 @@ static int sdhci_acpi_runtime_suspend(struct device *dev)
 {
 	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
 	struct sdhci_host *host = c->host;
+	int ret;
 
 	if (host->tuning_mode != SDHCI_TUNING_MODE_3)
 		mmc_retune_needed(host->mmc);
 
-	return sdhci_runtime_suspend_host(host);
+	ret = sdhci_runtime_suspend_host(host);
+	if (ret)
+		return ret;
+
+	sdhci_acpi_reset_signal_voltage_if_needed(dev);
+	return 0;
 }
 
 static int sdhci_acpi_runtime_resume(struct device *dev)
@@ -865,6 +947,7 @@ static int sdhci_acpi_runtime_resume(struct device *dev)
 	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
 
 	sdhci_acpi_byt_setting(&c->pdev->dev);
+	sdhci_acpi_restore_signal_voltage_if_needed(dev);
 
 	return sdhci_runtime_resume_host(c->host, 0);
 }
-- 
2.25.1

[PATCH v2 2/2] mmc: sdhci-acpi: Disable write protect detection on Acer Aspire Switch 10 (SW5-012)

[Hans de Goede]

On the Acer Aspire Switch 10 (SW5-012) microSD slot always reports the card
being write-protected even though microSD cards do not have a write-protect
switch at all.

Add a new DMI_QUIRK_SD_NO_WRITE_PROTECT quirk which when set sets
the MMC_CAP2_NO_WRITE_PROTECT flag on the controller for the external SD
slot; and add a DMI quirk table entry which selects this quirk for the
Acer SW5-012.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Drop the module option to allow overridig the quirks
---
 drivers/mmc/host/sdhci-acpi.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index d54a3592f40f..f17d5e556b43 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -79,6 +79,7 @@ struct sdhci_acpi_host {
 
 enum {
 	DMI_QUIRK_RESET_SD_SIGNAL_VOLT_ON_SUSP			= BIT(0),
+	DMI_QUIRK_SD_NO_WRITE_PROTECT				= BIT(1),
 };
 
 static inline void *sdhci_acpi_priv(struct sdhci_acpi_host *c)
@@ -668,6 +669,18 @@ static const struct dmi_system_id sdhci_acpi_quirks[] = {
 		},
 		.driver_data = (void *)DMI_QUIRK_RESET_SD_SIGNAL_VOLT_ON_SUSP,
 	},
+	{
+		/*
+		 * The Acer Aspire Switch 10 (SW5-012) microSD slot always
+		 * reports the card being write-protected even though microSD
+		 * cards do not have a write-protect switch at all.
+		 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Aspire SW5-012"),
+		},
+		.driver_data = (void *)DMI_QUIRK_SD_NO_WRITE_PROTECT,
+	},
 	{} /* Terminating entry */
 };
 
@@ -792,6 +805,9 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
 
 		if (quirks & DMI_QUIRK_RESET_SD_SIGNAL_VOLT_ON_SUSP)
 			c->reset_signal_volt_on_suspend = true;
+
+		if (quirks & DMI_QUIRK_SD_NO_WRITE_PROTECT)
+			host->mmc->caps2 |= MMC_CAP2_NO_WRITE_PROTECT;
 	}
 
 	err = sdhci_setup_host(host);
-- 
2.25.1

Re: [PATCH v2 1/2] mmc: sdhci-acpi: Switch signal voltage back to 3.3V on suspend on external microSD on Lenovo Miix 320

[Adrian Hunter]

Thanks for doing this.  A couple of questions below.

On 6/03/20 4:30 pm, Hans de Goede wrote:
> Based on a sample of 7 DSDTs from Cherry Trail devices using an AXP288
> PMIC depending on the design one of 2 possible LDOs on the PMIC is used
> for the MMC signalling voltage, either DLDO3 or GPIO1LDO (GPIO1 pin in
> low noise LDO mode).
> 
> The Lenovo Miix 320-10ICR uses GPIO1LDO in the SHC1 ACPI device's DSM
> methods to set 3.3 or 1.8 signalling voltage and this appears to work
> as advertised, so presumably the device is actually using GPIO1LDO for
> the external microSD signalling voltage.
> 
> But this device has a bug in the _PS0 method of the SHC1 ACPI device,
> the DSM remembers the last set signalling voltage and the _PS0 restores
> this after a (runtime) suspend-resume cycle, but it "restores" the voltage
> on DLDO3 instead of setting it on GPIO1LDO as the DSM method does. DLDO3
> is used for the LCD and setting it to 1.8V causes the LCD to go black.
> 
> This commit works around this issue by calling the Intel DSM to reset the
> signal voltage to 3.3V after the host has been runtime suspended.
> This will make the _PS0 method reprogram the DLDO3 voltage to 3.3V, which
> leaves it at its original setting fixing the LCD going black.
> 
> And this commit then resets the signal voltage back to the original 1.8V
> from the (runtime) resume handler, which runs after the ACPI _PS0 method
> has run.

Don't sdhci_resume_host, sdhci_runtime_resume_host do that anyway?

> 
> This commit adds and uses a DMI quirk mechanism to only trigger this
> workaround on the Lenovo Miix 320 while leaving the behavior of the
> driver unchanged on other devices.
> 
> BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=111294
> BugLink: https://gitlab.freedesktop.org/drm/intel/issues/355
> Reported-by: russianneuromancer <russianneuromancer@ya.ru>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> - Make the quirk reset the signal voltage to 3.3V at the end of the
>   (runtime) suspend handler instead of disabling 1.8V modes
> - Drop the module option to allow overridig the quirks
> ---
>  drivers/mmc/host/sdhci-acpi.c | 87 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 85 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
> index 9651dca6863e..d54a3592f40f 100644
> --- a/drivers/mmc/host/sdhci-acpi.c
> +++ b/drivers/mmc/host/sdhci-acpi.c
> @@ -23,6 +23,7 @@
>  #include <linux/pm.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/delay.h>
> +#include <linux/dmi.h>
>  
>  #include <linux/mmc/host.h>
>  #include <linux/mmc/pm.h>
> @@ -72,9 +73,14 @@ struct sdhci_acpi_host {
>  	const struct sdhci_acpi_slot	*slot;
>  	struct platform_device		*pdev;
>  	bool				use_runtime_pm;
> +	bool				reset_signal_volt_on_suspend;
>  	unsigned long			private[0] ____cacheline_aligned;
>  };
>  
> +enum {
> +	DMI_QUIRK_RESET_SD_SIGNAL_VOLT_ON_SUSP			= BIT(0),
> +};
> +
>  static inline void *sdhci_acpi_priv(struct sdhci_acpi_host *c)
>  {
>  	return (void *)c->private;
> @@ -647,6 +653,24 @@ static const struct acpi_device_id sdhci_acpi_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(acpi, sdhci_acpi_ids);
>  
> +static const struct dmi_system_id sdhci_acpi_quirks[] = {
> +	{
> +		/*
> +		 * The Lenovo Miix 320-10ICR has a bug in the _PS0 method of
> +		 * the SHC1 ACPI device, this bug causes it to reprogram the
> +		 * wrong LDO (DLDO3) to 1.8V if 1.8V modes are used and the
> +		 * card is (runtime) suspended + resumed. DLDO3 is used for
> +		 * the LCD and setting it to 1.8V causes the LCD to go black.
> +		 */
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> +			DMI_MATCH(DMI_PRODUCT_VERSION, "Lenovo MIIX 320-10ICR"),
> +		},
> +		.driver_data = (void *)DMI_QUIRK_RESET_SD_SIGNAL_VOLT_ON_SUSP,
> +	},
> +	{} /* Terminating entry */
> +};
> +
>  static const struct sdhci_acpi_slot *sdhci_acpi_get_slot(struct acpi_device *adev)
>  {
>  	const struct sdhci_acpi_uid_slot *u;
> @@ -663,17 +687,23 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	const struct sdhci_acpi_slot *slot;
>  	struct acpi_device *device, *child;
> +	const struct dmi_system_id *id;
>  	struct sdhci_acpi_host *c;
>  	struct sdhci_host *host;
>  	struct resource *iomem;
>  	resource_size_t len;
>  	size_t priv_size;
> +	int quirks = 0;
>  	int err;
>  
>  	device = ACPI_COMPANION(dev);
>  	if (!device)
>  		return -ENODEV;
>  
> +	id = dmi_first_match(sdhci_acpi_quirks);
> +	if (id)
> +		quirks = (long)id->driver_data;
> +
>  	slot = sdhci_acpi_get_slot(device);
>  
>  	/* Power on the SDHCI controller and its children */
> @@ -759,6 +789,9 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
>  			dev_warn(dev, "failed to setup card detect gpio\n");
>  			c->use_runtime_pm = false;
>  		}
> +
> +		if (quirks & DMI_QUIRK_RESET_SD_SIGNAL_VOLT_ON_SUSP)
> +			c->reset_signal_volt_on_suspend = true;
>  	}
>  
>  	err = sdhci_setup_host(host);
> @@ -823,17 +856,59 @@ static int sdhci_acpi_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static void __maybe_unused sdhci_acpi_reset_signal_voltage_if_needed(
> +	struct device *dev)
> +{
> +	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
> +	struct sdhci_host *host = c->host;
> +
> +	if (c->reset_signal_volt_on_suspend &&
> +	    host->mmc_host_ops.start_signal_voltage_switch ==
> +					intel_start_signal_voltage_switch &&

This creates a unexpected dependency on
host->mmc_host_ops.start_signal_voltage_switch.  Is it really necessary?

> +	    host->mmc->ios.signal_voltage != MMC_SIGNAL_VOLTAGE_330) {
> +		struct intel_host *intel_host = sdhci_acpi_priv(c);
> +		unsigned int fn = INTEL_DSM_V33_SWITCH;
> +		u32 result = 0;
> +
> +		intel_dsm(intel_host, dev, fn, &result);
> +	}
> +}
> +
> +static void __maybe_unused sdhci_acpi_restore_signal_voltage_if_needed(
> +	struct device *dev)
> +{
> +	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
> +	struct sdhci_host *host = c->host;
> +
> +	if (c->reset_signal_volt_on_suspend &&
> +	    host->mmc_host_ops.start_signal_voltage_switch ==
> +					intel_start_signal_voltage_switch &&
> +	    host->mmc->ios.signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
> +		struct intel_host *intel_host = sdhci_acpi_priv(c);
> +		unsigned int fn = INTEL_DSM_V18_SWITCH;
> +		u32 result = 0;
> +
> +		intel_dsm(intel_host, dev, fn, &result);
> +	}
> +}
> +
>  #ifdef CONFIG_PM_SLEEP
>  
>  static int sdhci_acpi_suspend(struct device *dev)
>  {
>  	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
>  	struct sdhci_host *host = c->host;
> +	int ret;
>  
>  	if (host->tuning_mode != SDHCI_TUNING_MODE_3)
>  		mmc_retune_needed(host->mmc);
>  
> -	return sdhci_suspend_host(host);
> +	ret = sdhci_suspend_host(host);
> +	if (ret)
> +		return ret;
> +
> +	sdhci_acpi_reset_signal_voltage_if_needed(dev);
> +	return 0;
>  }
>  
>  static int sdhci_acpi_resume(struct device *dev)
> @@ -841,6 +916,7 @@ static int sdhci_acpi_resume(struct device *dev)
>  	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
>  
>  	sdhci_acpi_byt_setting(&c->pdev->dev);
> +	sdhci_acpi_restore_signal_voltage_if_needed(dev);
>  
>  	return sdhci_resume_host(c->host);
>  }
> @@ -853,11 +929,17 @@ static int sdhci_acpi_runtime_suspend(struct device *dev)
>  {
>  	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
>  	struct sdhci_host *host = c->host;
> +	int ret;
>  
>  	if (host->tuning_mode != SDHCI_TUNING_MODE_3)
>  		mmc_retune_needed(host->mmc);
>  
> -	return sdhci_runtime_suspend_host(host);
> +	ret = sdhci_runtime_suspend_host(host);
> +	if (ret)
> +		return ret;
> +
> +	sdhci_acpi_reset_signal_voltage_if_needed(dev);
> +	return 0;
>  }
>  
>  static int sdhci_acpi_runtime_resume(struct device *dev)
> @@ -865,6 +947,7 @@ static int sdhci_acpi_runtime_resume(struct device *dev)
>  	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
>  
>  	sdhci_acpi_byt_setting(&c->pdev->dev);
> +	sdhci_acpi_restore_signal_voltage_if_needed(dev);
>  
>  	return sdhci_runtime_resume_host(c->host, 0);
>  }
> 

Re: [PATCH v2 1/2] mmc: sdhci-acpi: Switch signal voltage back to 3.3V on suspend on external microSD on Lenovo Miix 320

[Hans de Goede]

Hi,

On 3/9/20 10:26 AM, Adrian Hunter wrote:
> Thanks for doing this.  A couple of questions below.
> 
> On 6/03/20 4:30 pm, Hans de Goede wrote:
>> Based on a sample of 7 DSDTs from Cherry Trail devices using an AXP288
>> PMIC depending on the design one of 2 possible LDOs on the PMIC is used
>> for the MMC signalling voltage, either DLDO3 or GPIO1LDO (GPIO1 pin in
>> low noise LDO mode).
>>
>> The Lenovo Miix 320-10ICR uses GPIO1LDO in the SHC1 ACPI device's DSM
>> methods to set 3.3 or 1.8 signalling voltage and this appears to work
>> as advertised, so presumably the device is actually using GPIO1LDO for
>> the external microSD signalling voltage.
>>
>> But this device has a bug in the _PS0 method of the SHC1 ACPI device,
>> the DSM remembers the last set signalling voltage and the _PS0 restores
>> this after a (runtime) suspend-resume cycle, but it "restores" the voltage
>> on DLDO3 instead of setting it on GPIO1LDO as the DSM method does. DLDO3
>> is used for the LCD and setting it to 1.8V causes the LCD to go black.
>>
>> This commit works around this issue by calling the Intel DSM to reset the
>> signal voltage to 3.3V after the host has been runtime suspended.
>> This will make the _PS0 method reprogram the DLDO3 voltage to 3.3V, which
>> leaves it at its original setting fixing the LCD going black.
>>
>> And this commit then resets the signal voltage back to the original 1.8V
>> from the (runtime) resume handler, which runs after the ACPI _PS0 method
>> has run.
> 
> Don't sdhci_resume_host, sdhci_runtime_resume_host do that anyway?

It does not look like that, I've added the following debugging patch:

--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -147,6 +147,10 @@ static int intel_dsm(struct intel_host *intel_host, struct device *dev,
  	if (fn > 31 || !(intel_host->dsm_fns & (1 << fn)))
  		return -EOPNOTSUPP;

+	if (fn == INTEL_DSM_V18_SWITCH || fn == INTEL_DSM_V33_SWITCH)
+		pr_err("Intel DSM switching to %s volt\n",
+			(fn == INTEL_DSM_V18_SWITCH) ? "1.8" : "3.3");
+
  	return __intel_dsm(intel_host, dev, fn, result);
  }

@@ -903,8 +907,9 @@ static void __maybe_unused sdhci_acpi_restore_signal_voltage_if_needed(
  		struct intel_host *intel_host = sdhci_acpi_priv(c);
  		unsigned int fn = INTEL_DSM_V18_SWITCH;
  		u32 result = 0;
-
+		pr_err("Calling Intel DSM from %s\n", __func__);
  		intel_dsm(intel_host, dev, fn, &result);
+		pr_err("Calling Intel DSM from %s done\n", __func__);
  	}
  }


And this gives the following output:

[  368.079932] Intel DSM switching to 3.3 volt
[  368.414832] Intel DSM switching to 1.8 volt
[  371.989717] Intel DSM switching to 3.3 volt
[  407.176050] Calling Intel DSM from sdhci_acpi_restore_signal_voltage_if_needed
[  407.176052] Intel DSM switching to 1.8 volt
[  407.200276] Calling Intel DSM from sdhci_acpi_restore_signal_voltage_if_needed done
[  407.205846] Intel DSM switching to 3.3 volt
[  407.527003] Intel DSM switching to 1.8 volt
[  407.571991] Intel DSM switching to 3.3 volt
[  407.893990] Intel DSM switching to 1.8 volt

[  412.242658] Intel DSM switching to 1.8 volt
[  412.289387] Intel DSM switching to 3.3 volt
[  412.606210] Intel DSM switching to 1.8 volt
[  423.292135] Intel DSM switching to 3.3 volt
[  424.520057] Calling Intel DSM from sdhci_acpi_restore_signal_voltage_if_needed
[  424.520065] Intel DSM switching to 1.8 volt
[  424.546960] Calling Intel DSM from sdhci_acpi_restore_signal_voltage_if_needed done
[  424.552940] Intel DSM switching to 3.3 volt
[  424.890483] Intel DSM switching to 1.8 volt

Notice how the switch to 1.8 volt is not repeated.

It looks almost as if the voltage is being reset
to 3.3 volt by some higher level code, but only
after the sdhci_acpi_runtime_resume(), rather then
before the sdhci_acpi_runtime_suspend() completes.

If that is indeed the case then indeed we might not
need the sdhci_acpi_restore_signal_voltage_if_needed()
function, since the first call to set the signal
voltage to 3.3V on resume would just be a no-op
because with the quirk we already do that on suspend.

And then the second call to switch to 1.8 volt would
take care of switching to 1.8V for us.

But if this is indeed what is happening, then wouldn't
it make more sense to switch back to 3.3V from somewhere
in higher-level code before sdhci_runtime_suspend_host()
completes, replacing the current switch-back done just
after resume?  If we move that switch-back to 3.3V
done just after resume to suspend time then this entire
model-specific patch will likely become unnecessary ?

Note one more answer to some of your review remarks below.

>> This commit adds and uses a DMI quirk mechanism to only trigger this
>> workaround on the Lenovo Miix 320 while leaving the behavior of the
>> driver unchanged on other devices.
>>
>> BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=111294
>> BugLink: https://gitlab.freedesktop.org/drm/intel/issues/355
>> Reported-by: russianneuromancer <russianneuromancer@ya.ru>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> - Make the quirk reset the signal voltage to 3.3V at the end of the
>>    (runtime) suspend handler instead of disabling 1.8V modes
>> - Drop the module option to allow overridig the quirks
>> ---
>>   drivers/mmc/host/sdhci-acpi.c | 87 ++++++++++++++++++++++++++++++++++-
>>   1 file changed, 85 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
>> index 9651dca6863e..d54a3592f40f 100644
>> --- a/drivers/mmc/host/sdhci-acpi.c
>> +++ b/drivers/mmc/host/sdhci-acpi.c
>> @@ -23,6 +23,7 @@
>>   #include <linux/pm.h>
>>   #include <linux/pm_runtime.h>
>>   #include <linux/delay.h>
>> +#include <linux/dmi.h>
>>   
>>   #include <linux/mmc/host.h>
>>   #include <linux/mmc/pm.h>
>> @@ -72,9 +73,14 @@ struct sdhci_acpi_host {
>>   	const struct sdhci_acpi_slot	*slot;
>>   	struct platform_device		*pdev;
>>   	bool				use_runtime_pm;
>> +	bool				reset_signal_volt_on_suspend;
>>   	unsigned long			private[0] ____cacheline_aligned;
>>   };
>>   
>> +enum {
>> +	DMI_QUIRK_RESET_SD_SIGNAL_VOLT_ON_SUSP			= BIT(0),
>> +};
>> +
>>   static inline void *sdhci_acpi_priv(struct sdhci_acpi_host *c)
>>   {
>>   	return (void *)c->private;
>> @@ -647,6 +653,24 @@ static const struct acpi_device_id sdhci_acpi_ids[] = {
>>   };
>>   MODULE_DEVICE_TABLE(acpi, sdhci_acpi_ids);
>>   
>> +static const struct dmi_system_id sdhci_acpi_quirks[] = {
>> +	{
>> +		/*
>> +		 * The Lenovo Miix 320-10ICR has a bug in the _PS0 method of
>> +		 * the SHC1 ACPI device, this bug causes it to reprogram the
>> +		 * wrong LDO (DLDO3) to 1.8V if 1.8V modes are used and the
>> +		 * card is (runtime) suspended + resumed. DLDO3 is used for
>> +		 * the LCD and setting it to 1.8V causes the LCD to go black.
>> +		 */
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
>> +			DMI_MATCH(DMI_PRODUCT_VERSION, "Lenovo MIIX 320-10ICR"),
>> +		},
>> +		.driver_data = (void *)DMI_QUIRK_RESET_SD_SIGNAL_VOLT_ON_SUSP,
>> +	},
>> +	{} /* Terminating entry */
>> +};
>> +
>>   static const struct sdhci_acpi_slot *sdhci_acpi_get_slot(struct acpi_device *adev)
>>   {
>>   	const struct sdhci_acpi_uid_slot *u;
>> @@ -663,17 +687,23 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
>>   	struct device *dev = &pdev->dev;
>>   	const struct sdhci_acpi_slot *slot;
>>   	struct acpi_device *device, *child;
>> +	const struct dmi_system_id *id;
>>   	struct sdhci_acpi_host *c;
>>   	struct sdhci_host *host;
>>   	struct resource *iomem;
>>   	resource_size_t len;
>>   	size_t priv_size;
>> +	int quirks = 0;
>>   	int err;
>>   
>>   	device = ACPI_COMPANION(dev);
>>   	if (!device)
>>   		return -ENODEV;
>>   
>> +	id = dmi_first_match(sdhci_acpi_quirks);
>> +	if (id)
>> +		quirks = (long)id->driver_data;
>> +
>>   	slot = sdhci_acpi_get_slot(device);
>>   
>>   	/* Power on the SDHCI controller and its children */
>> @@ -759,6 +789,9 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
>>   			dev_warn(dev, "failed to setup card detect gpio\n");
>>   			c->use_runtime_pm = false;
>>   		}
>> +
>> +		if (quirks & DMI_QUIRK_RESET_SD_SIGNAL_VOLT_ON_SUSP)
>> +			c->reset_signal_volt_on_suspend = true;
>>   	}
>>   
>>   	err = sdhci_setup_host(host);
>> @@ -823,17 +856,59 @@ static int sdhci_acpi_remove(struct platform_device *pdev)
>>   	return 0;
>>   }
>>   
>> +static void __maybe_unused sdhci_acpi_reset_signal_voltage_if_needed(
>> +	struct device *dev)
>> +{
>> +	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
>> +	struct sdhci_host *host = c->host;
>> +
>> +	if (c->reset_signal_volt_on_suspend &&
>> +	    host->mmc_host_ops.start_signal_voltage_switch ==
>> +					intel_start_signal_voltage_switch &&
> 
> This creates a unexpected dependency on
> host->mmc_host_ops.start_signal_voltage_switch.  Is it really necessary?

Well we are directly invoking the intel_dsm here, although the
DMI match should only happen on a system which is using an
Intel SDHCI controller, I thought it would be better to check for
that rather then just assuming it.

Also see the:

+		struct intel_host *intel_host = sdhci_acpi_priv(c);

Line, doing this on a non Intel SDHCI ACPI controller would be bad.

Regards,

Hans


> 
>> +	    host->mmc->ios.signal_voltage != MMC_SIGNAL_VOLTAGE_330) {
>> +		struct intel_host *intel_host = sdhci_acpi_priv(c);
>> +		unsigned int fn = INTEL_DSM_V33_SWITCH;
>> +		u32 result = 0;
>> +
>> +		intel_dsm(intel_host, dev, fn, &result);
>> +	}
>> +}
>> +
>> +static void __maybe_unused sdhci_acpi_restore_signal_voltage_if_needed(
>> +	struct device *dev)
>> +{
>> +	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
>> +	struct sdhci_host *host = c->host;
>> +
>> +	if (c->reset_signal_volt_on_suspend &&
>> +	    host->mmc_host_ops.start_signal_voltage_switch ==
>> +					intel_start_signal_voltage_switch &&
>> +	    host->mmc->ios.signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
>> +		struct intel_host *intel_host = sdhci_acpi_priv(c);
>> +		unsigned int fn = INTEL_DSM_V18_SWITCH;
>> +		u32 result = 0;
>> +
>> +		intel_dsm(intel_host, dev, fn, &result);
>> +	}
>> +}
>> +
>>   #ifdef CONFIG_PM_SLEEP
>>   
>>   static int sdhci_acpi_suspend(struct device *dev)
>>   {
>>   	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
>>   	struct sdhci_host *host = c->host;
>> +	int ret;
>>   
>>   	if (host->tuning_mode != SDHCI_TUNING_MODE_3)
>>   		mmc_retune_needed(host->mmc);
>>   
>> -	return sdhci_suspend_host(host);
>> +	ret = sdhci_suspend_host(host);
>> +	if (ret)
>> +		return ret;
>> +
>> +	sdhci_acpi_reset_signal_voltage_if_needed(dev);
>> +	return 0;
>>   }
>>   
>>   static int sdhci_acpi_resume(struct device *dev)
>> @@ -841,6 +916,7 @@ static int sdhci_acpi_resume(struct device *dev)
>>   	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
>>   
>>   	sdhci_acpi_byt_setting(&c->pdev->dev);
>> +	sdhci_acpi_restore_signal_voltage_if_needed(dev);
>>   
>>   	return sdhci_resume_host(c->host);
>>   }
>> @@ -853,11 +929,17 @@ static int sdhci_acpi_runtime_suspend(struct device *dev)
>>   {
>>   	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
>>   	struct sdhci_host *host = c->host;
>> +	int ret;
>>   
>>   	if (host->tuning_mode != SDHCI_TUNING_MODE_3)
>>   		mmc_retune_needed(host->mmc);
>>   
>> -	return sdhci_runtime_suspend_host(host);
>> +	ret = sdhci_runtime_suspend_host(host);
>> +	if (ret)
>> +		return ret;
>> +
>> +	sdhci_acpi_reset_signal_voltage_if_needed(dev);
>> +	return 0;
>>   }
>>   
>>   static int sdhci_acpi_runtime_resume(struct device *dev)
>> @@ -865,6 +947,7 @@ static int sdhci_acpi_runtime_resume(struct device *dev)
>>   	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
>>   
>>   	sdhci_acpi_byt_setting(&c->pdev->dev);
>> +	sdhci_acpi_restore_signal_voltage_if_needed(dev);
>>   
>>   	return sdhci_runtime_resume_host(c->host, 0);
>>   }
>>
> 

Re: [PATCH v2 1/2] mmc: sdhci-acpi: Switch signal voltage back to 3.3V on suspend on external microSD on Lenovo Miix 320

[Adrian Hunter]

On 14/03/20 3:59 pm, Hans de Goede wrote:
> Hi,
> 
> On 3/9/20 10:26 AM, Adrian Hunter wrote:
>> Thanks for doing this.  A couple of questions below.
>>
>> On 6/03/20 4:30 pm, Hans de Goede wrote:
>>> Based on a sample of 7 DSDTs from Cherry Trail devices using an AXP288
>>> PMIC depending on the design one of 2 possible LDOs on the PMIC is used
>>> for the MMC signalling voltage, either DLDO3 or GPIO1LDO (GPIO1 pin in
>>> low noise LDO mode).
>>>
>>> The Lenovo Miix 320-10ICR uses GPIO1LDO in the SHC1 ACPI device's DSM
>>> methods to set 3.3 or 1.8 signalling voltage and this appears to work
>>> as advertised, so presumably the device is actually using GPIO1LDO for
>>> the external microSD signalling voltage.
>>>
>>> But this device has a bug in the _PS0 method of the SHC1 ACPI device,
>>> the DSM remembers the last set signalling voltage and the _PS0 restores
>>> this after a (runtime) suspend-resume cycle, but it "restores" the voltage
>>> on DLDO3 instead of setting it on GPIO1LDO as the DSM method does. DLDO3
>>> is used for the LCD and setting it to 1.8V causes the LCD to go black.
>>>
>>> This commit works around this issue by calling the Intel DSM to reset the
>>> signal voltage to 3.3V after the host has been runtime suspended.
>>> This will make the _PS0 method reprogram the DLDO3 voltage to 3.3V, which
>>> leaves it at its original setting fixing the LCD going black.
>>>
>>> And this commit then resets the signal voltage back to the original 1.8V
>>> from the (runtime) resume handler, which runs after the ACPI _PS0 method
>>> has run.
>>
>> Don't sdhci_resume_host, sdhci_runtime_resume_host do that anyway?
> 
> It does not look like that, I've added the following debugging patch:
> 
> --- a/drivers/mmc/host/sdhci-acpi.c
> +++ b/drivers/mmc/host/sdhci-acpi.c
> @@ -147,6 +147,10 @@ static int intel_dsm(struct intel_host *intel_host,
> struct device *dev,
>      if (fn > 31 || !(intel_host->dsm_fns & (1 << fn)))
>          return -EOPNOTSUPP;
> 
> +    if (fn == INTEL_DSM_V18_SWITCH || fn == INTEL_DSM_V33_SWITCH)
> +        pr_err("Intel DSM switching to %s volt\n",
> +            (fn == INTEL_DSM_V18_SWITCH) ? "1.8" : "3.3");
> +
>      return __intel_dsm(intel_host, dev, fn, result);
>  }
> 
> @@ -903,8 +907,9 @@ static void __maybe_unused
> sdhci_acpi_restore_signal_voltage_if_needed(
>          struct intel_host *intel_host = sdhci_acpi_priv(c);
>          unsigned int fn = INTEL_DSM_V18_SWITCH;
>          u32 result = 0;
> -
> +        pr_err("Calling Intel DSM from %s\n", __func__);
>          intel_dsm(intel_host, dev, fn, &result);
> +        pr_err("Calling Intel DSM from %s done\n", __func__);
>      }
>  }
> 
> 
> And this gives the following output:
> 
> [  368.079932] Intel DSM switching to 3.3 volt
> [  368.414832] Intel DSM switching to 1.8 volt
> [  371.989717] Intel DSM switching to 3.3 volt
> [  407.176050] Calling Intel DSM from
> sdhci_acpi_restore_signal_voltage_if_needed
> [  407.176052] Intel DSM switching to 1.8 volt
> [  407.200276] Calling Intel DSM from
> sdhci_acpi_restore_signal_voltage_if_needed done
> [  407.205846] Intel DSM switching to 3.3 volt
> [  407.527003] Intel DSM switching to 1.8 volt
> [  407.571991] Intel DSM switching to 3.3 volt
> [  407.893990] Intel DSM switching to 1.8 volt
> 
> [  412.242658] Intel DSM switching to 1.8 volt
> [  412.289387] Intel DSM switching to 3.3 volt
> [  412.606210] Intel DSM switching to 1.8 volt
> [  423.292135] Intel DSM switching to 3.3 volt
> [  424.520057] Calling Intel DSM from
> sdhci_acpi_restore_signal_voltage_if_needed
> [  424.520065] Intel DSM switching to 1.8 volt
> [  424.546960] Calling Intel DSM from
> sdhci_acpi_restore_signal_voltage_if_needed done
> [  424.552940] Intel DSM switching to 3.3 volt
> [  424.890483] Intel DSM switching to 1.8 volt
> 
> Notice how the switch to 1.8 volt is not repeated.

Because the card has been suspended i.e. it will be re-initialized completely.

> 
> It looks almost as if the voltage is being reset
> to 3.3 volt by some higher level code, but only
> after the sdhci_acpi_runtime_resume(), rather then
> before the sdhci_acpi_runtime_suspend() completes.
> 
> If that is indeed the case then indeed we might not
> need the sdhci_acpi_restore_signal_voltage_if_needed()
> function, since the first call to set the signal
> voltage to 3.3V on resume would just be a no-op
> because with the quirk we already do that on suspend.
> 
> And then the second call to switch to 1.8 volt would
> take care of switching to 1.8V for us.
> 
> But if this is indeed what is happening, then wouldn't
> it make more sense to switch back to 3.3V from somewhere
> in higher-level code before sdhci_runtime_suspend_host()
> completes, replacing the current switch-back done just
> after resume?  If we move that switch-back to 3.3V
> done just after resume to suspend time then this entire
> model-specific patch will likely become unnecessary ?
> 
> Note one more answer to some of your review remarks below.
> 
>>> This commit adds and uses a DMI quirk mechanism to only trigger this
>>> workaround on the Lenovo Miix 320 while leaving the behavior of the
>>> driver unchanged on other devices.
>>>
>>> BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=111294
>>> BugLink: https://gitlab.freedesktop.org/drm/intel/issues/355
>>> Reported-by: russianneuromancer <russianneuromancer@ya.ru>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> Changes in v2:
>>> - Make the quirk reset the signal voltage to 3.3V at the end of the
>>>    (runtime) suspend handler instead of disabling 1.8V modes
>>> - Drop the module option to allow overridig the quirks
>>> ---
>>>   drivers/mmc/host/sdhci-acpi.c | 87 ++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 85 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
>>> index 9651dca6863e..d54a3592f40f 100644
>>> --- a/drivers/mmc/host/sdhci-acpi.c
>>> +++ b/drivers/mmc/host/sdhci-acpi.c
>>> @@ -23,6 +23,7 @@
>>>   #include <linux/pm.h>
>>>   #include <linux/pm_runtime.h>
>>>   #include <linux/delay.h>
>>> +#include <linux/dmi.h>
>>>     #include <linux/mmc/host.h>
>>>   #include <linux/mmc/pm.h>
>>> @@ -72,9 +73,14 @@ struct sdhci_acpi_host {
>>>       const struct sdhci_acpi_slot    *slot;
>>>       struct platform_device        *pdev;
>>>       bool                use_runtime_pm;
>>> +    bool                reset_signal_volt_on_suspend;
>>>       unsigned long            private[0] ____cacheline_aligned;
>>>   };
>>>   +enum {
>>> +    DMI_QUIRK_RESET_SD_SIGNAL_VOLT_ON_SUSP            = BIT(0),
>>> +};
>>> +
>>>   static inline void *sdhci_acpi_priv(struct sdhci_acpi_host *c)
>>>   {
>>>       return (void *)c->private;
>>> @@ -647,6 +653,24 @@ static const struct acpi_device_id sdhci_acpi_ids[] = {
>>>   };
>>>   MODULE_DEVICE_TABLE(acpi, sdhci_acpi_ids);
>>>   +static const struct dmi_system_id sdhci_acpi_quirks[] = {
>>> +    {
>>> +        /*
>>> +         * The Lenovo Miix 320-10ICR has a bug in the _PS0 method of
>>> +         * the SHC1 ACPI device, this bug causes it to reprogram the
>>> +         * wrong LDO (DLDO3) to 1.8V if 1.8V modes are used and the
>>> +         * card is (runtime) suspended + resumed. DLDO3 is used for
>>> +         * the LCD and setting it to 1.8V causes the LCD to go black.
>>> +         */
>>> +        .matches = {
>>> +            DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
>>> +            DMI_MATCH(DMI_PRODUCT_VERSION, "Lenovo MIIX 320-10ICR"),
>>> +        },
>>> +        .driver_data = (void *)DMI_QUIRK_RESET_SD_SIGNAL_VOLT_ON_SUSP,
>>> +    },
>>> +    {} /* Terminating entry */
>>> +};
>>> +
>>>   static const struct sdhci_acpi_slot *sdhci_acpi_get_slot(struct
>>> acpi_device *adev)
>>>   {
>>>       const struct sdhci_acpi_uid_slot *u;
>>> @@ -663,17 +687,23 @@ static int sdhci_acpi_probe(struct platform_device
>>> *pdev)
>>>       struct device *dev = &pdev->dev;
>>>       const struct sdhci_acpi_slot *slot;
>>>       struct acpi_device *device, *child;
>>> +    const struct dmi_system_id *id;
>>>       struct sdhci_acpi_host *c;
>>>       struct sdhci_host *host;
>>>       struct resource *iomem;
>>>       resource_size_t len;
>>>       size_t priv_size;
>>> +    int quirks = 0;
>>>       int err;
>>>         device = ACPI_COMPANION(dev);
>>>       if (!device)
>>>           return -ENODEV;
>>>   +    id = dmi_first_match(sdhci_acpi_quirks);
>>> +    if (id)
>>> +        quirks = (long)id->driver_data;
>>> +
>>>       slot = sdhci_acpi_get_slot(device);
>>>         /* Power on the SDHCI controller and its children */
>>> @@ -759,6 +789,9 @@ static int sdhci_acpi_probe(struct platform_device
>>> *pdev)
>>>               dev_warn(dev, "failed to setup card detect gpio\n");
>>>               c->use_runtime_pm = false;
>>>           }
>>> +
>>> +        if (quirks & DMI_QUIRK_RESET_SD_SIGNAL_VOLT_ON_SUSP)
>>> +            c->reset_signal_volt_on_suspend = true;
>>>       }
>>>         err = sdhci_setup_host(host);
>>> @@ -823,17 +856,59 @@ static int sdhci_acpi_remove(struct platform_device
>>> *pdev)
>>>       return 0;
>>>   }
>>>   +static void __maybe_unused sdhci_acpi_reset_signal_voltage_if_needed(
>>> +    struct device *dev)
>>> +{
>>> +    struct sdhci_acpi_host *c = dev_get_drvdata(dev);
>>> +    struct sdhci_host *host = c->host;
>>> +
>>> +    if (c->reset_signal_volt_on_suspend &&
>>> +        host->mmc_host_ops.start_signal_voltage_switch ==
>>> +                    intel_start_signal_voltage_switch &&
>>
>> This creates a unexpected dependency on
>> host->mmc_host_ops.start_signal_voltage_switch.  Is it really necessary?
> 
> Well we are directly invoking the intel_dsm here, although the
> DMI match should only happen on a system which is using an
> Intel SDHCI controller, I thought it would be better to check for
> that rather then just assuming it.
> 
> Also see the:
> 
> +        struct intel_host *intel_host = sdhci_acpi_priv(c);
> 
> Line, doing this on a non Intel SDHCI ACPI controller would be bad.

Then you need to add a comment to intel_probe_slot() to explain that
sdhci_acpi_reset_signal_voltage_if_needed() is dependent on:
	host->mmc_host_ops.start_signal_voltage_switch =
					intel_start_signal_voltage_switch;


> 
> Regards,
> 
> Hans
> 
> 
>>
>>> +        host->mmc->ios.signal_voltage != MMC_SIGNAL_VOLTAGE_330) {
>>> +        struct intel_host *intel_host = sdhci_acpi_priv(c);
>>> +        unsigned int fn = INTEL_DSM_V33_SWITCH;
>>> +        u32 result = 0;
>>> +
>>> +        intel_dsm(intel_host, dev, fn, &result);
>>> +    }
>>> +}
>>> +
>>> +static void __maybe_unused sdhci_acpi_restore_signal_voltage_if_needed(
>>> +    struct device *dev)
>>> +{
>>> +    struct sdhci_acpi_host *c = dev_get_drvdata(dev);
>>> +    struct sdhci_host *host = c->host;
>>> +
>>> +    if (c->reset_signal_volt_on_suspend &&
>>> +        host->mmc_host_ops.start_signal_voltage_switch ==
>>> +                    intel_start_signal_voltage_switch &&
>>> +        host->mmc->ios.signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
>>> +        struct intel_host *intel_host = sdhci_acpi_priv(c);
>>> +        unsigned int fn = INTEL_DSM_V18_SWITCH;
>>> +        u32 result = 0;
>>> +
>>> +        intel_dsm(intel_host, dev, fn, &result);
>>> +    }
>>> +}
>>> +
>>>   #ifdef CONFIG_PM_SLEEP
>>>     static int sdhci_acpi_suspend(struct device *dev)
>>>   {
>>>       struct sdhci_acpi_host *c = dev_get_drvdata(dev);
>>>       struct sdhci_host *host = c->host;
>>> +    int ret;
>>>         if (host->tuning_mode != SDHCI_TUNING_MODE_3)
>>>           mmc_retune_needed(host->mmc);
>>>   -    return sdhci_suspend_host(host);
>>> +    ret = sdhci_suspend_host(host);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    sdhci_acpi_reset_signal_voltage_if_needed(dev);
>>> +    return 0;
>>>   }
>>>     static int sdhci_acpi_resume(struct device *dev)
>>> @@ -841,6 +916,7 @@ static int sdhci_acpi_resume(struct device *dev)
>>>       struct sdhci_acpi_host *c = dev_get_drvdata(dev);
>>>         sdhci_acpi_byt_setting(&c->pdev->dev);
>>> +    sdhci_acpi_restore_signal_voltage_if_needed(dev);
>>>         return sdhci_resume_host(c->host);
>>>   }
>>> @@ -853,11 +929,17 @@ static int sdhci_acpi_runtime_suspend(struct device
>>> *dev)
>>>   {
>>>       struct sdhci_acpi_host *c = dev_get_drvdata(dev);
>>>       struct sdhci_host *host = c->host;
>>> +    int ret;
>>>         if (host->tuning_mode != SDHCI_TUNING_MODE_3)
>>>           mmc_retune_needed(host->mmc);
>>>   -    return sdhci_runtime_suspend_host(host);
>>> +    ret = sdhci_runtime_suspend_host(host);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    sdhci_acpi_reset_signal_voltage_if_needed(dev);
>>> +    return 0;
>>>   }
>>>     static int sdhci_acpi_runtime_resume(struct device *dev)
>>> @@ -865,6 +947,7 @@ static int sdhci_acpi_runtime_resume(struct device *dev)
>>>       struct sdhci_acpi_host *c = dev_get_drvdata(dev);
>>>         sdhci_acpi_byt_setting(&c->pdev->dev);
>>> +    sdhci_acpi_restore_signal_voltage_if_needed(dev);
>>>         return sdhci_runtime_resume_host(c->host, 0);
>>>   }
>>>
>>
> 

Re: [PATCH v2 1/2] mmc: sdhci-acpi: Switch signal voltage back to 3.3V on suspend on external microSD on Lenovo Miix 320

[Hans de Goede]

Hi,

On 3/16/20 10:07 AM, Adrian Hunter wrote:
> On 14/03/20 3:59 pm, Hans de Goede wrote:
>> Hi,
>>
>> On 3/9/20 10:26 AM, Adrian Hunter wrote:
>>> Thanks for doing this.  A couple of questions below.
>>>
>>> On 6/03/20 4:30 pm, Hans de Goede wrote:
>>>> Based on a sample of 7 DSDTs from Cherry Trail devices using an AXP288
>>>> PMIC depending on the design one of 2 possible LDOs on the PMIC is used
>>>> for the MMC signalling voltage, either DLDO3 or GPIO1LDO (GPIO1 pin in
>>>> low noise LDO mode).
>>>>
>>>> The Lenovo Miix 320-10ICR uses GPIO1LDO in the SHC1 ACPI device's DSM
>>>> methods to set 3.3 or 1.8 signalling voltage and this appears to work
>>>> as advertised, so presumably the device is actually using GPIO1LDO for
>>>> the external microSD signalling voltage.
>>>>
>>>> But this device has a bug in the _PS0 method of the SHC1 ACPI device,
>>>> the DSM remembers the last set signalling voltage and the _PS0 restores
>>>> this after a (runtime) suspend-resume cycle, but it "restores" the voltage
>>>> on DLDO3 instead of setting it on GPIO1LDO as the DSM method does. DLDO3
>>>> is used for the LCD and setting it to 1.8V causes the LCD to go black.
>>>>
>>>> This commit works around this issue by calling the Intel DSM to reset the
>>>> signal voltage to 3.3V after the host has been runtime suspended.
>>>> This will make the _PS0 method reprogram the DLDO3 voltage to 3.3V, which
>>>> leaves it at its original setting fixing the LCD going black.
>>>>
>>>> And this commit then resets the signal voltage back to the original 1.8V
>>>> from the (runtime) resume handler, which runs after the ACPI _PS0 method
>>>> has run.
>>>
>>> Don't sdhci_resume_host, sdhci_runtime_resume_host do that anyway?
>>
>> It does not look like that, I've added the following debugging patch:
>>
>> --- a/drivers/mmc/host/sdhci-acpi.c
>> +++ b/drivers/mmc/host/sdhci-acpi.c
>> @@ -147,6 +147,10 @@ static int intel_dsm(struct intel_host *intel_host,
>> struct device *dev,
>>       if (fn > 31 || !(intel_host->dsm_fns & (1 << fn)))
>>           return -EOPNOTSUPP;
>>
>> +    if (fn == INTEL_DSM_V18_SWITCH || fn == INTEL_DSM_V33_SWITCH)
>> +        pr_err("Intel DSM switching to %s volt\n",
>> +            (fn == INTEL_DSM_V18_SWITCH) ? "1.8" : "3.3");
>> +
>>       return __intel_dsm(intel_host, dev, fn, result);
>>   }
>>
>> @@ -903,8 +907,9 @@ static void __maybe_unused
>> sdhci_acpi_restore_signal_voltage_if_needed(
>>           struct intel_host *intel_host = sdhci_acpi_priv(c);
>>           unsigned int fn = INTEL_DSM_V18_SWITCH;
>>           u32 result = 0;
>> -
>> +        pr_err("Calling Intel DSM from %s\n", __func__);
>>           intel_dsm(intel_host, dev, fn, &result);
>> +        pr_err("Calling Intel DSM from %s done\n", __func__);
>>       }
>>   }
>>
>>
>> And this gives the following output:
>>
>> [  368.079932] Intel DSM switching to 3.3 volt
>> [  368.414832] Intel DSM switching to 1.8 volt
>> [  371.989717] Intel DSM switching to 3.3 volt
>> [  407.176050] Calling Intel DSM from
>> sdhci_acpi_restore_signal_voltage_if_needed
>> [  407.176052] Intel DSM switching to 1.8 volt
>> [  407.200276] Calling Intel DSM from
>> sdhci_acpi_restore_signal_voltage_if_needed done
>> [  407.205846] Intel DSM switching to 3.3 volt
>> [  407.527003] Intel DSM switching to 1.8 volt
>> [  407.571991] Intel DSM switching to 3.3 volt
>> [  407.893990] Intel DSM switching to 1.8 volt
>>
>> [  412.242658] Intel DSM switching to 1.8 volt
>> [  412.289387] Intel DSM switching to 3.3 volt
>> [  412.606210] Intel DSM switching to 1.8 volt
>> [  423.292135] Intel DSM switching to 3.3 volt
>> [  424.520057] Calling Intel DSM from
>> sdhci_acpi_restore_signal_voltage_if_needed
>> [  424.520065] Intel DSM switching to 1.8 volt
>> [  424.546960] Calling Intel DSM from
>> sdhci_acpi_restore_signal_voltage_if_needed done
>> [  424.552940] Intel DSM switching to 3.3 volt
>> [  424.890483] Intel DSM switching to 1.8 volt
>>
>> Notice how the switch to 1.8 volt is not repeated.
> 
> Because the card has been suspended i.e. it will be re-initialized completely.

Right.

What I was trying to say is:

Since the re-init starts with resetting the signal voltage
to 3.3 volt, couldn't we do the reset of the signal voltage
at the end of sdhci_[runtime_]suspend_host() or even deeper
in the stack (after the card has been suspended/turned off)?
That would remove the need for this patch.

I guess that would be a pretty major change, so perhaps it
is best to remove the sdhci_acpi_restore_signal_voltage_if_needed
function as you suggest and move forward with a v3 of this
patch with that changed?

Regards,

Hans



> 
>>
>> It looks almost as if the voltage is being reset
>> to 3.3 volt by some higher level code, but only
>> after the sdhci_acpi_runtime_resume(), rather then
>> before the sdhci_acpi_runtime_suspend() completes.
>>
>> If that is indeed the case then indeed we might not
>> need the sdhci_acpi_restore_signal_voltage_if_needed()
>> function, since the first call to set the signal
>> voltage to 3.3V on resume would just be a no-op
>> because with the quirk we already do that on suspend.
>>
>> And then the second call to switch to 1.8 volt would
>> take care of switching to 1.8V for us.
>>
>> But if this is indeed what is happening, then wouldn't
>> it make more sense to switch back to 3.3V from somewhere
>> in higher-level code before sdhci_runtime_suspend_host()
>> completes, replacing the current switch-back done just
>> after resume?  If we move that switch-back to 3.3V
>> done just after resume to suspend time then this entire
>> model-specific patch will likely become unnecessary ?
>>
>> Note one more answer to some of your review remarks below.
>>
>>>> This commit adds and uses a DMI quirk mechanism to only trigger this
>>>> workaround on the Lenovo Miix 320 while leaving the behavior of the
>>>> driver unchanged on other devices.
>>>>
>>>> BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=111294
>>>> BugLink: https://gitlab.freedesktop.org/drm/intel/issues/355
>>>> Reported-by: russianneuromancer <russianneuromancer@ya.ru>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> Changes in v2:
>>>> - Make the quirk reset the signal voltage to 3.3V at the end of the
>>>>     (runtime) suspend handler instead of disabling 1.8V modes
>>>> - Drop the module option to allow overridig the quirks
>>>> ---
>>>>    drivers/mmc/host/sdhci-acpi.c | 87 ++++++++++++++++++++++++++++++++++-
>>>>    1 file changed, 85 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
>>>> index 9651dca6863e..d54a3592f40f 100644
>>>> --- a/drivers/mmc/host/sdhci-acpi.c
>>>> +++ b/drivers/mmc/host/sdhci-acpi.c
>>>> @@ -23,6 +23,7 @@
>>>>    #include <linux/pm.h>
>>>>    #include <linux/pm_runtime.h>
>>>>    #include <linux/delay.h>
>>>> +#include <linux/dmi.h>
>>>>      #include <linux/mmc/host.h>
>>>>    #include <linux/mmc/pm.h>
>>>> @@ -72,9 +73,14 @@ struct sdhci_acpi_host {
>>>>        const struct sdhci_acpi_slot    *slot;
>>>>        struct platform_device        *pdev;
>>>>        bool                use_runtime_pm;
>>>> +    bool                reset_signal_volt_on_suspend;
>>>>        unsigned long            private[0] ____cacheline_aligned;
>>>>    };
>>>>    +enum {
>>>> +    DMI_QUIRK_RESET_SD_SIGNAL_VOLT_ON_SUSP            = BIT(0),
>>>> +};
>>>> +
>>>>    static inline void *sdhci_acpi_priv(struct sdhci_acpi_host *c)
>>>>    {
>>>>        return (void *)c->private;
>>>> @@ -647,6 +653,24 @@ static const struct acpi_device_id sdhci_acpi_ids[] = {
>>>>    };
>>>>    MODULE_DEVICE_TABLE(acpi, sdhci_acpi_ids);
>>>>    +static const struct dmi_system_id sdhci_acpi_quirks[] = {
>>>> +    {
>>>> +        /*
>>>> +         * The Lenovo Miix 320-10ICR has a bug in the _PS0 method of
>>>> +         * the SHC1 ACPI device, this bug causes it to reprogram the
>>>> +         * wrong LDO (DLDO3) to 1.8V if 1.8V modes are used and the
>>>> +         * card is (runtime) suspended + resumed. DLDO3 is used for
>>>> +         * the LCD and setting it to 1.8V causes the LCD to go black.
>>>> +         */
>>>> +        .matches = {
>>>> +            DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
>>>> +            DMI_MATCH(DMI_PRODUCT_VERSION, "Lenovo MIIX 320-10ICR"),
>>>> +        },
>>>> +        .driver_data = (void *)DMI_QUIRK_RESET_SD_SIGNAL_VOLT_ON_SUSP,
>>>> +    },
>>>> +    {} /* Terminating entry */
>>>> +};
>>>> +
>>>>    static const struct sdhci_acpi_slot *sdhci_acpi_get_slot(struct
>>>> acpi_device *adev)
>>>>    {
>>>>        const struct sdhci_acpi_uid_slot *u;
>>>> @@ -663,17 +687,23 @@ static int sdhci_acpi_probe(struct platform_device
>>>> *pdev)
>>>>        struct device *dev = &pdev->dev;
>>>>        const struct sdhci_acpi_slot *slot;
>>>>        struct acpi_device *device, *child;
>>>> +    const struct dmi_system_id *id;
>>>>        struct sdhci_acpi_host *c;
>>>>        struct sdhci_host *host;
>>>>        struct resource *iomem;
>>>>        resource_size_t len;
>>>>        size_t priv_size;
>>>> +    int quirks = 0;
>>>>        int err;
>>>>          device = ACPI_COMPANION(dev);
>>>>        if (!device)
>>>>            return -ENODEV;
>>>>    +    id = dmi_first_match(sdhci_acpi_quirks);
>>>> +    if (id)
>>>> +        quirks = (long)id->driver_data;
>>>> +
>>>>        slot = sdhci_acpi_get_slot(device);
>>>>          /* Power on the SDHCI controller and its children */
>>>> @@ -759,6 +789,9 @@ static int sdhci_acpi_probe(struct platform_device
>>>> *pdev)
>>>>                dev_warn(dev, "failed to setup card detect gpio\n");
>>>>                c->use_runtime_pm = false;
>>>>            }
>>>> +
>>>> +        if (quirks & DMI_QUIRK_RESET_SD_SIGNAL_VOLT_ON_SUSP)
>>>> +            c->reset_signal_volt_on_suspend = true;
>>>>        }
>>>>          err = sdhci_setup_host(host);
>>>> @@ -823,17 +856,59 @@ static int sdhci_acpi_remove(struct platform_device
>>>> *pdev)
>>>>        return 0;
>>>>    }
>>>>    +static void __maybe_unused sdhci_acpi_reset_signal_voltage_if_needed(
>>>> +    struct device *dev)
>>>> +{
>>>> +    struct sdhci_acpi_host *c = dev_get_drvdata(dev);
>>>> +    struct sdhci_host *host = c->host;
>>>> +
>>>> +    if (c->reset_signal_volt_on_suspend &&
>>>> +        host->mmc_host_ops.start_signal_voltage_switch ==
>>>> +                    intel_start_signal_voltage_switch &&
>>>
>>> This creates a unexpected dependency on
>>> host->mmc_host_ops.start_signal_voltage_switch.  Is it really necessary?
>>
>> Well we are directly invoking the intel_dsm here, although the
>> DMI match should only happen on a system which is using an
>> Intel SDHCI controller, I thought it would be better to check for
>> that rather then just assuming it.
>>
>> Also see the:
>>
>> +        struct intel_host *intel_host = sdhci_acpi_priv(c);
>>
>> Line, doing this on a non Intel SDHCI ACPI controller would be bad.
> 
> Then you need to add a comment to intel_probe_slot() to explain that
> sdhci_acpi_reset_signal_voltage_if_needed() is dependent on:
> 	host->mmc_host_ops.start_signal_voltage_switch =
> 					intel_start_signal_voltage_switch;
> 
> 
>>
>> Regards,
>>
>> Hans
>>
>>
>>>
>>>> +        host->mmc->ios.signal_voltage != MMC_SIGNAL_VOLTAGE_330) {
>>>> +        struct intel_host *intel_host = sdhci_acpi_priv(c);
>>>> +        unsigned int fn = INTEL_DSM_V33_SWITCH;
>>>> +        u32 result = 0;
>>>> +
>>>> +        intel_dsm(intel_host, dev, fn, &result);
>>>> +    }
>>>> +}
>>>> +
>>>> +static void __maybe_unused sdhci_acpi_restore_signal_voltage_if_needed(
>>>> +    struct device *dev)
>>>> +{
>>>> +    struct sdhci_acpi_host *c = dev_get_drvdata(dev);
>>>> +    struct sdhci_host *host = c->host;
>>>> +
>>>> +    if (c->reset_signal_volt_on_suspend &&
>>>> +        host->mmc_host_ops.start_signal_voltage_switch ==
>>>> +                    intel_start_signal_voltage_switch &&
>>>> +        host->mmc->ios.signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
>>>> +        struct intel_host *intel_host = sdhci_acpi_priv(c);
>>>> +        unsigned int fn = INTEL_DSM_V18_SWITCH;
>>>> +        u32 result = 0;
>>>> +
>>>> +        intel_dsm(intel_host, dev, fn, &result);
>>>> +    }
>>>> +}
>>>> +
>>>>    #ifdef CONFIG_PM_SLEEP
>>>>      static int sdhci_acpi_suspend(struct device *dev)
>>>>    {
>>>>        struct sdhci_acpi_host *c = dev_get_drvdata(dev);
>>>>        struct sdhci_host *host = c->host;
>>>> +    int ret;
>>>>          if (host->tuning_mode != SDHCI_TUNING_MODE_3)
>>>>            mmc_retune_needed(host->mmc);
>>>>    -    return sdhci_suspend_host(host);
>>>> +    ret = sdhci_suspend_host(host);
>>>> +    if (ret)
>>>> +        return ret;
>>>> +
>>>> +    sdhci_acpi_reset_signal_voltage_if_needed(dev);
>>>> +    return 0;
>>>>    }
>>>>      static int sdhci_acpi_resume(struct device *dev)
>>>> @@ -841,6 +916,7 @@ static int sdhci_acpi_resume(struct device *dev)
>>>>        struct sdhci_acpi_host *c = dev_get_drvdata(dev);
>>>>          sdhci_acpi_byt_setting(&c->pdev->dev);
>>>> +    sdhci_acpi_restore_signal_voltage_if_needed(dev);
>>>>          return sdhci_resume_host(c->host);
>>>>    }
>>>> @@ -853,11 +929,17 @@ static int sdhci_acpi_runtime_suspend(struct device
>>>> *dev)
>>>>    {
>>>>        struct sdhci_acpi_host *c = dev_get_drvdata(dev);
>>>>        struct sdhci_host *host = c->host;
>>>> +    int ret;
>>>>          if (host->tuning_mode != SDHCI_TUNING_MODE_3)
>>>>            mmc_retune_needed(host->mmc);
>>>>    -    return sdhci_runtime_suspend_host(host);
>>>> +    ret = sdhci_runtime_suspend_host(host);
>>>> +    if (ret)
>>>> +        return ret;
>>>> +
>>>> +    sdhci_acpi_reset_signal_voltage_if_needed(dev);
>>>> +    return 0;
>>>>    }
>>>>      static int sdhci_acpi_runtime_resume(struct device *dev)
>>>> @@ -865,6 +947,7 @@ static int sdhci_acpi_runtime_resume(struct device *dev)
>>>>        struct sdhci_acpi_host *c = dev_get_drvdata(dev);
>>>>          sdhci_acpi_byt_setting(&c->pdev->dev);
>>>> +    sdhci_acpi_restore_signal_voltage_if_needed(dev);
>>>>          return sdhci_runtime_resume_host(c->host, 0);
>>>>    }
>>>>
>>>
>>
> 

Re: [PATCH v2 1/2] mmc: sdhci-acpi: Switch signal voltage back to 3.3V on suspend on external microSD on Lenovo Miix 320

[Adrian Hunter]

On 16/03/20 11:34 am, Hans de Goede wrote:
> Hi,
> 
> On 3/16/20 10:07 AM, Adrian Hunter wrote:
>> On 14/03/20 3:59 pm, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 3/9/20 10:26 AM, Adrian Hunter wrote:
>>>> Thanks for doing this.  A couple of questions below.
>>>>
>>>> On 6/03/20 4:30 pm, Hans de Goede wrote:
>>>>> Based on a sample of 7 DSDTs from Cherry Trail devices using an AXP288
>>>>> PMIC depending on the design one of 2 possible LDOs on the PMIC is used
>>>>> for the MMC signalling voltage, either DLDO3 or GPIO1LDO (GPIO1 pin in
>>>>> low noise LDO mode).
>>>>>
>>>>> The Lenovo Miix 320-10ICR uses GPIO1LDO in the SHC1 ACPI device's DSM
>>>>> methods to set 3.3 or 1.8 signalling voltage and this appears to work
>>>>> as advertised, so presumably the device is actually using GPIO1LDO for
>>>>> the external microSD signalling voltage.
>>>>>
>>>>> But this device has a bug in the _PS0 method of the SHC1 ACPI device,
>>>>> the DSM remembers the last set signalling voltage and the _PS0 restores
>>>>> this after a (runtime) suspend-resume cycle, but it "restores" the voltage
>>>>> on DLDO3 instead of setting it on GPIO1LDO as the DSM method does. DLDO3
>>>>> is used for the LCD and setting it to 1.8V causes the LCD to go black.
>>>>>
>>>>> This commit works around this issue by calling the Intel DSM to reset the
>>>>> signal voltage to 3.3V after the host has been runtime suspended.
>>>>> This will make the _PS0 method reprogram the DLDO3 voltage to 3.3V, which
>>>>> leaves it at its original setting fixing the LCD going black.
>>>>>
>>>>> And this commit then resets the signal voltage back to the original 1.8V
>>>>> from the (runtime) resume handler, which runs after the ACPI _PS0 method
>>>>> has run.
>>>>
>>>> Don't sdhci_resume_host, sdhci_runtime_resume_host do that anyway?
>>>
>>> It does not look like that, I've added the following debugging patch:
>>>
>>> --- a/drivers/mmc/host/sdhci-acpi.c
>>> +++ b/drivers/mmc/host/sdhci-acpi.c
>>> @@ -147,6 +147,10 @@ static int intel_dsm(struct intel_host *intel_host,
>>> struct device *dev,
>>>       if (fn > 31 || !(intel_host->dsm_fns & (1 << fn)))
>>>           return -EOPNOTSUPP;
>>>
>>> +    if (fn == INTEL_DSM_V18_SWITCH || fn == INTEL_DSM_V33_SWITCH)
>>> +        pr_err("Intel DSM switching to %s volt\n",
>>> +            (fn == INTEL_DSM_V18_SWITCH) ? "1.8" : "3.3");
>>> +
>>>       return __intel_dsm(intel_host, dev, fn, result);
>>>   }
>>>
>>> @@ -903,8 +907,9 @@ static void __maybe_unused
>>> sdhci_acpi_restore_signal_voltage_if_needed(
>>>           struct intel_host *intel_host = sdhci_acpi_priv(c);
>>>           unsigned int fn = INTEL_DSM_V18_SWITCH;
>>>           u32 result = 0;
>>> -
>>> +        pr_err("Calling Intel DSM from %s\n", __func__);
>>>           intel_dsm(intel_host, dev, fn, &result);
>>> +        pr_err("Calling Intel DSM from %s done\n", __func__);
>>>       }
>>>   }
>>>
>>>
>>> And this gives the following output:
>>>
>>> [  368.079932] Intel DSM switching to 3.3 volt
>>> [  368.414832] Intel DSM switching to 1.8 volt
>>> [  371.989717] Intel DSM switching to 3.3 volt
>>> [  407.176050] Calling Intel DSM from
>>> sdhci_acpi_restore_signal_voltage_if_needed
>>> [  407.176052] Intel DSM switching to 1.8 volt
>>> [  407.200276] Calling Intel DSM from
>>> sdhci_acpi_restore_signal_voltage_if_needed done
>>> [  407.205846] Intel DSM switching to 3.3 volt
>>> [  407.527003] Intel DSM switching to 1.8 volt
>>> [  407.571991] Intel DSM switching to 3.3 volt
>>> [  407.893990] Intel DSM switching to 1.8 volt
>>>
>>> [  412.242658] Intel DSM switching to 1.8 volt
>>> [  412.289387] Intel DSM switching to 3.3 volt
>>> [  412.606210] Intel DSM switching to 1.8 volt
>>> [  423.292135] Intel DSM switching to 3.3 volt
>>> [  424.520057] Calling Intel DSM from
>>> sdhci_acpi_restore_signal_voltage_if_needed
>>> [  424.520065] Intel DSM switching to 1.8 volt
>>> [  424.546960] Calling Intel DSM from
>>> sdhci_acpi_restore_signal_voltage_if_needed done
>>> [  424.552940] Intel DSM switching to 3.3 volt
>>> [  424.890483] Intel DSM switching to 1.8 volt
>>>
>>> Notice how the switch to 1.8 volt is not repeated.
>>
>> Because the card has been suspended i.e. it will be re-initialized
>> completely.
> 
> Right.
> 
> What I was trying to say is:
> 
> Since the re-init starts with resetting the signal voltage
> to 3.3 volt, couldn't we do the reset of the signal voltage
> at the end of sdhci_[runtime_]suspend_host() or even deeper
> in the stack (after the card has been suspended/turned off)?
> That would remove the need for this patch.
> 
> I guess that would be a pretty major change, so perhaps it
> is best to remove the sdhci_acpi_restore_signal_voltage_if_needed
> function as you suggest and move forward with a v3 of this
> patch with that changed?

Yes please

> 
> Regards,
> 
> Hans
> 
> 
> 
>>
>>>
>>> It looks almost as if the voltage is being reset
>>> to 3.3 volt by some higher level code, but only
>>> after the sdhci_acpi_runtime_resume(), rather then
>>> before the sdhci_acpi_runtime_suspend() completes.
>>>
>>> If that is indeed the case then indeed we might not
>>> need the sdhci_acpi_restore_signal_voltage_if_needed()
>>> function, since the first call to set the signal
>>> voltage to 3.3V on resume would just be a no-op
>>> because with the quirk we already do that on suspend.
>>>
>>> And then the second call to switch to 1.8 volt would
>>> take care of switching to 1.8V for us.
>>>
>>> But if this is indeed what is happening, then wouldn't
>>> it make more sense to switch back to 3.3V from somewhere
>>> in higher-level code before sdhci_runtime_suspend_host()
>>> completes, replacing the current switch-back done just
>>> after resume?  If we move that switch-back to 3.3V
>>> done just after resume to suspend time then this entire
>>> model-specific patch will likely become unnecessary ?
>>>
>>> Note one more answer to some of your review remarks below.
>>>
>>>>> This commit adds and uses a DMI quirk mechanism to only trigger this
>>>>> workaround on the Lenovo Miix 320 while leaving the behavior of the
>>>>> driver unchanged on other devices.
>>>>>
>>>>> BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=111294
>>>>> BugLink: https://gitlab.freedesktop.org/drm/intel/issues/355
>>>>> Reported-by: russianneuromancer <russianneuromancer@ya.ru>
>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>> ---
>>>>> Changes in v2:
>>>>> - Make the quirk reset the signal voltage to 3.3V at the end of the
>>>>>     (runtime) suspend handler instead of disabling 1.8V modes
>>>>> - Drop the module option to allow overridig the quirks
>>>>> ---
>>>>>    drivers/mmc/host/sdhci-acpi.c | 87 ++++++++++++++++++++++++++++++++++-
>>>>>    1 file changed, 85 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
>>>>> index 9651dca6863e..d54a3592f40f 100644
>>>>> --- a/drivers/mmc/host/sdhci-acpi.c
>>>>> +++ b/drivers/mmc/host/sdhci-acpi.c
>>>>> @@ -23,6 +23,7 @@
>>>>>    #include <linux/pm.h>
>>>>>    #include <linux/pm_runtime.h>
>>>>>    #include <linux/delay.h>
>>>>> +#include <linux/dmi.h>
>>>>>      #include <linux/mmc/host.h>
>>>>>    #include <linux/mmc/pm.h>
>>>>> @@ -72,9 +73,14 @@ struct sdhci_acpi_host {
>>>>>        const struct sdhci_acpi_slot    *slot;
>>>>>        struct platform_device        *pdev;
>>>>>        bool                use_runtime_pm;
>>>>> +    bool                reset_signal_volt_on_suspend;
>>>>>        unsigned long            private[0] ____cacheline_aligned;
>>>>>    };
>>>>>    +enum {
>>>>> +    DMI_QUIRK_RESET_SD_SIGNAL_VOLT_ON_SUSP            = BIT(0),
>>>>> +};
>>>>> +
>>>>>    static inline void *sdhci_acpi_priv(struct sdhci_acpi_host *c)
>>>>>    {
>>>>>        return (void *)c->private;
>>>>> @@ -647,6 +653,24 @@ static const struct acpi_device_id
>>>>> sdhci_acpi_ids[] = {
>>>>>    };
>>>>>    MODULE_DEVICE_TABLE(acpi, sdhci_acpi_ids);
>>>>>    +static const struct dmi_system_id sdhci_acpi_quirks[] = {
>>>>> +    {
>>>>> +        /*
>>>>> +         * The Lenovo Miix 320-10ICR has a bug in the _PS0 method of
>>>>> +         * the SHC1 ACPI device, this bug causes it to reprogram the
>>>>> +         * wrong LDO (DLDO3) to 1.8V if 1.8V modes are used and the
>>>>> +         * card is (runtime) suspended + resumed. DLDO3 is used for
>>>>> +         * the LCD and setting it to 1.8V causes the LCD to go black.
>>>>> +         */
>>>>> +        .matches = {
>>>>> +            DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
>>>>> +            DMI_MATCH(DMI_PRODUCT_VERSION, "Lenovo MIIX 320-10ICR"),
>>>>> +        },
>>>>> +        .driver_data = (void *)DMI_QUIRK_RESET_SD_SIGNAL_VOLT_ON_SUSP,
>>>>> +    },
>>>>> +    {} /* Terminating entry */
>>>>> +};
>>>>> +
>>>>>    static const struct sdhci_acpi_slot *sdhci_acpi_get_slot(struct
>>>>> acpi_device *adev)
>>>>>    {
>>>>>        const struct sdhci_acpi_uid_slot *u;
>>>>> @@ -663,17 +687,23 @@ static int sdhci_acpi_probe(struct platform_device
>>>>> *pdev)
>>>>>        struct device *dev = &pdev->dev;
>>>>>        const struct sdhci_acpi_slot *slot;
>>>>>        struct acpi_device *device, *child;
>>>>> +    const struct dmi_system_id *id;
>>>>>        struct sdhci_acpi_host *c;
>>>>>        struct sdhci_host *host;
>>>>>        struct resource *iomem;
>>>>>        resource_size_t len;
>>>>>        size_t priv_size;
>>>>> +    int quirks = 0;
>>>>>        int err;
>>>>>          device = ACPI_COMPANION(dev);
>>>>>        if (!device)
>>>>>            return -ENODEV;
>>>>>    +    id = dmi_first_match(sdhci_acpi_quirks);
>>>>> +    if (id)
>>>>> +        quirks = (long)id->driver_data;
>>>>> +
>>>>>        slot = sdhci_acpi_get_slot(device);
>>>>>          /* Power on the SDHCI controller and its children */
>>>>> @@ -759,6 +789,9 @@ static int sdhci_acpi_probe(struct platform_device
>>>>> *pdev)
>>>>>                dev_warn(dev, "failed to setup card detect gpio\n");
>>>>>                c->use_runtime_pm = false;
>>>>>            }
>>>>> +
>>>>> +        if (quirks & DMI_QUIRK_RESET_SD_SIGNAL_VOLT_ON_SUSP)
>>>>> +            c->reset_signal_volt_on_suspend = true;
>>>>>        }
>>>>>          err = sdhci_setup_host(host);
>>>>> @@ -823,17 +856,59 @@ static int sdhci_acpi_remove(struct platform_device
>>>>> *pdev)
>>>>>        return 0;
>>>>>    }
>>>>>    +static void __maybe_unused sdhci_acpi_reset_signal_voltage_if_needed(
>>>>> +    struct device *dev)
>>>>> +{
>>>>> +    struct sdhci_acpi_host *c = dev_get_drvdata(dev);
>>>>> +    struct sdhci_host *host = c->host;
>>>>> +
>>>>> +    if (c->reset_signal_volt_on_suspend &&
>>>>> +        host->mmc_host_ops.start_signal_voltage_switch ==
>>>>> +                    intel_start_signal_voltage_switch &&
>>>>
>>>> This creates a unexpected dependency on
>>>> host->mmc_host_ops.start_signal_voltage_switch.  Is it really necessary?
>>>
>>> Well we are directly invoking the intel_dsm here, although the
>>> DMI match should only happen on a system which is using an
>>> Intel SDHCI controller, I thought it would be better to check for
>>> that rather then just assuming it.
>>>
>>> Also see the:
>>>
>>> +        struct intel_host *intel_host = sdhci_acpi_priv(c);
>>>
>>> Line, doing this on a non Intel SDHCI ACPI controller would be bad.
>>
>> Then you need to add a comment to intel_probe_slot() to explain that
>> sdhci_acpi_reset_signal_voltage_if_needed() is dependent on:
>>     host->mmc_host_ops.start_signal_voltage_switch =
>>                     intel_start_signal_voltage_switch;
>>
>>
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>>
>>>>
>>>>> +        host->mmc->ios.signal_voltage != MMC_SIGNAL_VOLTAGE_330) {
>>>>> +        struct intel_host *intel_host = sdhci_acpi_priv(c);
>>>>> +        unsigned int fn = INTEL_DSM_V33_SWITCH;
>>>>> +        u32 result = 0;
>>>>> +
>>>>> +        intel_dsm(intel_host, dev, fn, &result);
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> +static void __maybe_unused sdhci_acpi_restore_signal_voltage_if_needed(
>>>>> +    struct device *dev)
>>>>> +{
>>>>> +    struct sdhci_acpi_host *c = dev_get_drvdata(dev);
>>>>> +    struct sdhci_host *host = c->host;
>>>>> +
>>>>> +    if (c->reset_signal_volt_on_suspend &&
>>>>> +        host->mmc_host_ops.start_signal_voltage_switch ==
>>>>> +                    intel_start_signal_voltage_switch &&
>>>>> +        host->mmc->ios.signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
>>>>> +        struct intel_host *intel_host = sdhci_acpi_priv(c);
>>>>> +        unsigned int fn = INTEL_DSM_V18_SWITCH;
>>>>> +        u32 result = 0;
>>>>> +
>>>>> +        intel_dsm(intel_host, dev, fn, &result);
>>>>> +    }
>>>>> +}
>>>>> +
>>>>>    #ifdef CONFIG_PM_SLEEP
>>>>>      static int sdhci_acpi_suspend(struct device *dev)
>>>>>    {
>>>>>        struct sdhci_acpi_host *c = dev_get_drvdata(dev);
>>>>>        struct sdhci_host *host = c->host;
>>>>> +    int ret;
>>>>>          if (host->tuning_mode != SDHCI_TUNING_MODE_3)
>>>>>            mmc_retune_needed(host->mmc);
>>>>>    -    return sdhci_suspend_host(host);
>>>>> +    ret = sdhci_suspend_host(host);
>>>>> +    if (ret)
>>>>> +        return ret;
>>>>> +
>>>>> +    sdhci_acpi_reset_signal_voltage_if_needed(dev);
>>>>> +    return 0;
>>>>>    }
>>>>>      static int sdhci_acpi_resume(struct device *dev)
>>>>> @@ -841,6 +916,7 @@ static int sdhci_acpi_resume(struct device *dev)
>>>>>        struct sdhci_acpi_host *c = dev_get_drvdata(dev);
>>>>>          sdhci_acpi_byt_setting(&c->pdev->dev);
>>>>> +    sdhci_acpi_restore_signal_voltage_if_needed(dev);
>>>>>          return sdhci_resume_host(c->host);
>>>>>    }
>>>>> @@ -853,11 +929,17 @@ static int sdhci_acpi_runtime_suspend(struct device
>>>>> *dev)
>>>>>    {
>>>>>        struct sdhci_acpi_host *c = dev_get_drvdata(dev);
>>>>>        struct sdhci_host *host = c->host;
>>>>> +    int ret;
>>>>>          if (host->tuning_mode != SDHCI_TUNING_MODE_3)
>>>>>            mmc_retune_needed(host->mmc);
>>>>>    -    return sdhci_runtime_suspend_host(host);
>>>>> +    ret = sdhci_runtime_suspend_host(host);
>>>>> +    if (ret)
>>>>> +        return ret;
>>>>> +
>>>>> +    sdhci_acpi_reset_signal_voltage_if_needed(dev);
>>>>> +    return 0;
>>>>>    }
>>>>>      static int sdhci_acpi_runtime_resume(struct device *dev)
>>>>> @@ -865,6 +947,7 @@ static int sdhci_acpi_runtime_resume(struct device
>>>>> *dev)
>>>>>        struct sdhci_acpi_host *c = dev_get_drvdata(dev);
>>>>>          sdhci_acpi_byt_setting(&c->pdev->dev);
>>>>> +    sdhci_acpi_restore_signal_voltage_if_needed(dev);
>>>>>          return sdhci_runtime_resume_host(c->host, 0);
>>>>>    }
>>>>>
>>>>
>>>
>>
> 

Re: [PATCH v2 1/2] mmc: sdhci-acpi: Switch signal voltage back to 3.3V on suspend on external microSD on Lenovo Miix 320

[Hans de Goede]

HI,

On 3/16/20 10:07 AM, Adrian Hunter wrote:
> On 14/03/20 3:59 pm, Hans de Goede wrote:
>> Hi,
>>
>> On 3/9/20 10:26 AM, Adrian Hunter wrote:
>>> Thanks for doing this.  A couple of questions below.

<snip>

>>>> @@ -823,17 +856,59 @@ static int sdhci_acpi_remove(struct platform_device
>>>> *pdev)
>>>>        return 0;
>>>>    }
>>>>    +static void __maybe_unused sdhci_acpi_reset_signal_voltage_if_needed(
>>>> +    struct device *dev)
>>>> +{
>>>> +    struct sdhci_acpi_host *c = dev_get_drvdata(dev);
>>>> +    struct sdhci_host *host = c->host;
>>>> +
>>>> +    if (c->reset_signal_volt_on_suspend &&
>>>> +        host->mmc_host_ops.start_signal_voltage_switch ==
>>>> +                    intel_start_signal_voltage_switch &&
>>>
>>> This creates a unexpected dependency on
>>> host->mmc_host_ops.start_signal_voltage_switch.  Is it really necessary?
>>
>> Well we are directly invoking the intel_dsm here, although the
>> DMI match should only happen on a system which is using an
>> Intel SDHCI controller, I thought it would be better to check for
>> that rather then just assuming it.
>>
>> Also see the:
>>
>> +        struct intel_host *intel_host = sdhci_acpi_priv(c);
>>
>> Line, doing this on a non Intel SDHCI ACPI controller would be bad.
> 
> Then you need to add a comment to intel_probe_slot() to explain that
> sdhci_acpi_reset_signal_voltage_if_needed() is dependent on:
> 	host->mmc_host_ops.start_signal_voltage_switch =
> 					intel_start_signal_voltage_switch;

For v3 which I'm preparing now, I've decided that adding an "is_intel"
bool to sdhci_acpi_host is a cleaner way to deal with this.

Regards,

Hans