[PATCH v2 0/3] sdhci-pci voltage switch fixes

[PATCH v2 0/3] sdhci-pci voltage switch fixes

[Anisse Astier]

Hi,

This new patch series changes the approach to enabling the no-1.8V
quirk. We determined that not having the DSM method might not be a good
enough signal for the board/eMMC not supporting 1.8V.

It now uses a DMI quirk; I've also added another machine which benefited
from the previous patch, but for which the issue is harder to reproduce.

I've kept the previous cleanup of the voltage_switch callback in an
optional third patch, but it does not enable any quirk.

Anisse Astier (3):
  mmc: sdhci-pci: disable 1.8V with dmi quirk
  mmc: sdhci-pci: add new machine with 1.8V dmi quirk
  mmc: sdhci-pci: only install voltage switch method when useful

 drivers/mmc/host/sdhci-pci-core.c | 34 +++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

-- 
2.17.2

[PATCH v2 1/3] mmc: sdhci-pci: disable 1.8V with dmi quirk

[Anisse Astier]

If the motherboard is known not to support 1.8V properly, add the
necessary quirk on probe.

This fixes an issue on a Gemini Lake (GLK) laptop : eMMC driver will
timeout on boot (from 60seconds to 10minutes ) as the cqhci attempts CQE
recovery after a failed voltage switch. In earlier kernels, the problem
existed, but only delayed boot for about 10 seconds after an I/O error,
allowing booting on the eMMC (almost) unnoticed.

Signed-off-by: Anisse Astier <anisse@astier.eu>
---
 drivers/mmc/host/sdhci-pci-core.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
index 7bfd366d970d..396413f7c854 100644
--- a/drivers/mmc/host/sdhci-pci-core.c
+++ b/drivers/mmc/host/sdhci-pci-core.c
@@ -29,6 +29,7 @@
 #include <linux/mmc/slot-gpio.h>
 #include <linux/mmc/sdhci-pci-data.h>
 #include <linux/acpi.h>
+#include <linux/dmi.h>
 
 #include "cqhci.h"
 
@@ -703,6 +704,16 @@ static int intel_execute_tuning(struct mmc_host *mmc, u32 opcode)
 	return 0;
 }
 
+static const struct dmi_system_id board_no_1_8v[] = {
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Notebook"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "N75_77GU"),
+		},
+	},
+	{ }
+};
+
 static void byt_probe_slot(struct sdhci_pci_slot *slot)
 {
 	struct mmc_host_ops *ops = &slot->host->mmc_host_ops;
@@ -710,6 +721,12 @@ static void byt_probe_slot(struct sdhci_pci_slot *slot)
 	byt_read_dsm(slot);
 
 	ops->execute_tuning = intel_execute_tuning;
+
+	if (dmi_check_system(board_no_1_8v)) {
+		pr_debug("%s: motherboard does not support 1.8V\n",
+				mmc_hostname(slot->host->mmc));
+		slot->host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
+	}
 	ops->start_signal_voltage_switch = intel_start_signal_voltage_switch;
 }
 
-- 
2.17.2

[PATCH v2 2/3] mmc: sdhci-pci: add new machine with 1.8V dmi quirk

[Anisse Astier]

This machine also displays the same issue that is fixed by the quirk.

Signed-off-by: Anisse Astier <anisse@astier.eu>
---
 drivers/mmc/host/sdhci-pci-core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
index 396413f7c854..f2c1fb339d66 100644
--- a/drivers/mmc/host/sdhci-pci-core.c
+++ b/drivers/mmc/host/sdhci-pci-core.c
@@ -711,6 +711,12 @@ static const struct dmi_system_id board_no_1_8v[] = {
 			DMI_MATCH(DMI_PRODUCT_NAME, "N75_77GU"),
 		},
 	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Ordissimo"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Agathe3"),
+		},
+	},
 	{ }
 };
 
-- 
2.17.2

[PATCH v2 3/3] mmc: sdhci-pci: only install voltage switch method when useful

[Anisse Astier]

If there's no ACPI DSM for voltage switch, it will just cause a lot of
debug info down the line, we only need one at startup.

Signed-off-by: Anisse Astier <anisse@astier.eu>
---
 drivers/mmc/host/sdhci-pci-core.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
index f2c1fb339d66..95fdbb883c7e 100644
--- a/drivers/mmc/host/sdhci-pci-core.c
+++ b/drivers/mmc/host/sdhci-pci-core.c
@@ -723,6 +723,7 @@ static const struct dmi_system_id board_no_1_8v[] = {
 static void byt_probe_slot(struct sdhci_pci_slot *slot)
 {
 	struct mmc_host_ops *ops = &slot->host->mmc_host_ops;
+	struct intel_host *intel_host = sdhci_pci_priv(slot);
 
 	byt_read_dsm(slot);
 
@@ -733,6 +734,16 @@ static void byt_probe_slot(struct sdhci_pci_slot *slot)
 				mmc_hostname(slot->host->mmc));
 		slot->host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
 	}
+	/* Check if we have the appropriate voltage switch DSM methods */
+	if (!(intel_host->dsm_fns & (1 << INTEL_DSM_V18_SWITCH)) &&
+	    !(intel_host->dsm_fns & (1 << INTEL_DSM_V33_SWITCH))) {
+		/* No voltage switching supported at all, there's no
+		 * point in installing the callback: return.
+		 */
+		pr_debug("%s: No voltage switching ACPI DSM helper\n",
+				mmc_hostname(slot->host->mmc));
+		return;
+	}
 	ops->start_signal_voltage_switch = intel_start_signal_voltage_switch;
 }
 
-- 
2.17.2

Re: [PATCH v2 3/3] mmc: sdhci-pci: only install voltage switch method when useful

[Anisse Astier]

Hi Adrian,

On Tue, Oct 23, 2018 at 12:07:29PM +0200, Anisse Astier wrote:
> If there's no ACPI DSM for voltage switch, it will just cause a lot of
> debug info down the line, we only need one at startup.
> 
> Signed-off-by: Anisse Astier <anisse@astier.eu>
> ---
>  drivers/mmc/host/sdhci-pci-core.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
> index f2c1fb339d66..95fdbb883c7e 100644
> --- a/drivers/mmc/host/sdhci-pci-core.c
> +++ b/drivers/mmc/host/sdhci-pci-core.c
> @@ -723,6 +723,7 @@ static const struct dmi_system_id board_no_1_8v[] = {
>  static void byt_probe_slot(struct sdhci_pci_slot *slot)
>  {
>  	struct mmc_host_ops *ops = &slot->host->mmc_host_ops;
> +	struct intel_host *intel_host = sdhci_pci_priv(slot);
>  
>  	byt_read_dsm(slot);
>  
> @@ -733,6 +734,16 @@ static void byt_probe_slot(struct sdhci_pci_slot *slot)
>  				mmc_hostname(slot->host->mmc));
>  		slot->host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
>  	}
> +	/* Check if we have the appropriate voltage switch DSM methods */
> +	if (!(intel_host->dsm_fns & (1 << INTEL_DSM_V18_SWITCH)) &&
> +	    !(intel_host->dsm_fns & (1 << INTEL_DSM_V33_SWITCH))) {
> +		/* No voltage switching supported at all, there's no
> +		 * point in installing the callback: return.
> +		 */
> +		pr_debug("%s: No voltage switching ACPI DSM helper\n",
> +				mmc_hostname(slot->host->mmc));
> +		return;
> +	}
>  	ops->start_signal_voltage_switch = intel_start_signal_voltage_switch;
>  }
>  
> -- 
> 2.17.2
> 

What do you think of picking this last patch ? Or maybe you had
different cleanups in mind when you said you wanted to rework this ?

Regards,

Anisse

Re: [PATCH v2 3/3] mmc: sdhci-pci: only install voltage switch method when useful

[Adrian Hunter]

On 16/11/18 6:58 PM, Anisse Astier wrote:
> Hi Adrian,
> 
> On Tue, Oct 23, 2018 at 12:07:29PM +0200, Anisse Astier wrote:
>> If there's no ACPI DSM for voltage switch, it will just cause a lot of
>> debug info down the line, we only need one at startup.
>>
>> Signed-off-by: Anisse Astier <anisse@astier.eu>
>> ---
>>  drivers/mmc/host/sdhci-pci-core.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
>> index f2c1fb339d66..95fdbb883c7e 100644
>> --- a/drivers/mmc/host/sdhci-pci-core.c
>> +++ b/drivers/mmc/host/sdhci-pci-core.c
>> @@ -723,6 +723,7 @@ static const struct dmi_system_id board_no_1_8v[] = {
>>  static void byt_probe_slot(struct sdhci_pci_slot *slot)
>>  {
>>  	struct mmc_host_ops *ops = &slot->host->mmc_host_ops;
>> +	struct intel_host *intel_host = sdhci_pci_priv(slot);
>>  
>>  	byt_read_dsm(slot);
>>  
>> @@ -733,6 +734,16 @@ static void byt_probe_slot(struct sdhci_pci_slot *slot)
>>  				mmc_hostname(slot->host->mmc));
>>  		slot->host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
>>  	}
>> +	/* Check if we have the appropriate voltage switch DSM methods */
>> +	if (!(intel_host->dsm_fns & (1 << INTEL_DSM_V18_SWITCH)) &&
>> +	    !(intel_host->dsm_fns & (1 << INTEL_DSM_V33_SWITCH))) {
>> +		/* No voltage switching supported at all, there's no
>> +		 * point in installing the callback: return.
>> +		 */
>> +		pr_debug("%s: No voltage switching ACPI DSM helper\n",
>> +				mmc_hostname(slot->host->mmc));
>> +		return;
>> +	}
>>  	ops->start_signal_voltage_switch = intel_start_signal_voltage_switch;
>>  }
>>  
>> -- 
>> 2.17.2
>>
> 
> What do you think of picking this last patch ? Or maybe you had
> different cleanups in mind when you said you wanted to rework this ?

Voltage switches are relatively rare, and dynamic debug allows control over
exactly which debug messages display, so I am not sure this patch is needed.

Re: [PATCH v2 3/3] mmc: sdhci-pci: only install voltage switch method when useful

[Anisse Astier]

On Mon, Nov 19, 2018 at 09:45:03AM +0200, Adrian Hunter wrote:
> On 16/11/18 6:58 PM, Anisse Astier wrote:
> > Hi Adrian,
> > 
> > On Tue, Oct 23, 2018 at 12:07:29PM +0200, Anisse Astier wrote:
> >> If there's no ACPI DSM for voltage switch, it will just cause a lot of
> >> debug info down the line, we only need one at startup.
> >>
> >> Signed-off-by: Anisse Astier <anisse@astier.eu>
> >> ---
> >>  drivers/mmc/host/sdhci-pci-core.c | 11 +++++++++++
> >>  1 file changed, 11 insertions(+)
> >>
> >> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
> >> index f2c1fb339d66..95fdbb883c7e 100644
> >> --- a/drivers/mmc/host/sdhci-pci-core.c
> >> +++ b/drivers/mmc/host/sdhci-pci-core.c
> >> @@ -723,6 +723,7 @@ static const struct dmi_system_id board_no_1_8v[] = {
> >>  static void byt_probe_slot(struct sdhci_pci_slot *slot)
> >>  {
> >>  	struct mmc_host_ops *ops = &slot->host->mmc_host_ops;
> >> +	struct intel_host *intel_host = sdhci_pci_priv(slot);
> >>  
> >>  	byt_read_dsm(slot);
> >>  
> >> @@ -733,6 +734,16 @@ static void byt_probe_slot(struct sdhci_pci_slot *slot)
> >>  				mmc_hostname(slot->host->mmc));
> >>  		slot->host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
> >>  	}
> >> +	/* Check if we have the appropriate voltage switch DSM methods */
> >> +	if (!(intel_host->dsm_fns & (1 << INTEL_DSM_V18_SWITCH)) &&
> >> +	    !(intel_host->dsm_fns & (1 << INTEL_DSM_V33_SWITCH))) {
> >> +		/* No voltage switching supported at all, there's no
> >> +		 * point in installing the callback: return.
> >> +		 */
> >> +		pr_debug("%s: No voltage switching ACPI DSM helper\n",
> >> +				mmc_hostname(slot->host->mmc));
> >> +		return;
> >> +	}
> >>  	ops->start_signal_voltage_switch = intel_start_signal_voltage_switch;
> >>  }
> >>  
> >> -- 
> >> 2.17.2
> >>
> > 
> > What do you think of picking this last patch ? Or maybe you had
> > different cleanups in mind when you said you wanted to rework this ?
> 
> Voltage switches are relatively rare, and dynamic debug allows control over
> exactly which debug messages display, so I am not sure this patch is needed.

I just thought this message was a bit clearer than:
mmc0: intel_start_signal_voltage_switch DSM fn 4 error -95 result 0

But you're correct, with dynamic debug it shouldn't be an issue, and the
original message is easily searchable in the code. It only happens every
two seconds.

I'm ok to drop this patch.

Regards,

Anisse