Re: [PATCH 1/2] mmc: sdhci-acpi: Disable 1.8V modes on external microSD on Lenovo Miix 320

[Adrian Hunter <adrian.hunter@intel.com>]

On 16/01/20 3:26 pm, Hans de Goede wrote:
> HI,
> 
> On 16-01-2020 08:59, Adrian Hunter wrote:
>> On 15/01/20 5:31 pm, Hans de Goede wrote:
> 
> <snip>
> 
>>>>> Note that the suspend/resume handling is broken also in the sense that
>>>>> it does not disable the signal voltage during suspend.
>>>>
>>>> The bus power gets switched off if the card is runtime suspended.  The host
>>>> controller should go to D3cold which means everything off.
>>>
>>> Right, what I mean is that the _PS3 method is broken in that it does
>>> not turn off the voltage-regulator providing the signal voltage, as
>>> it does do on other machines with a non buggy implementation.
>>
>> Is that different to what you would get with Windows?
> 
> No Windows has the same problem.
> 
>> Also, you could possibly build a custom DSDT and fix the _PS0 and _PS3
>> yourself.  That requires building it into a custom kernel also though.
> 
> I have not tried, but yes that should work, but until we get some generic
> mechanism (*) in Linux / distro-s to provide DSDT overrides, that is not
> helpful for regular Linux users.
> 
> *) which also has copyright issues, so the chances of this ever happening
> are slim
> 
> <snip>
> 
>>>>>>> +static int quirks = -1;
>>>>>>> +module_param(quirks, int, 0444);
>>>>>>> +MODULE_PARM_DESC(quirks, "Override sdhci-acpi specific quirks");
>>>>>>
>>>>>> Why is a module parameter needed?
>>>>>
>>>>> The module parameter is purely to make testing if the same quirk(s)
>>>>> help on other devices easier. Like the debug_quirks[2] params in sdhci.c
>>>>
>>>> Mmm, but we already have SDHCI_QUIRK2_NO_1_8_V
>>>
>>> True, but this only applies to the sdcard slot and not to the eMMC,
>>> also you are asking for this to be changed to:
>>>
>>> SDHCI_ACPI_QUIRK_SD_SET_SIGNAL_3_3V_ON_SUSPEND
>>>
>>> Which is not duplicate. Anyways if you dislike the module parameter
>>> bits I can drop them and make this only available through the DMI quirks.
>>>
>>
>> It isn't dislike, it is whether it will ever be needed.
> 
> For this specific issue, chances are not that big we will need it
> on another device. The quirk added by the second patch, to disable
> (broken) read-only detection OTOH might very well be useful on some
> other devices.
> 
> And adding the option to override the quirks from the kernel commandline
> requires very little extra code.
> 
> Anyways, it is your call. Please let me know if you want to drop the
> module parameter for v2, or if you are ok with keeping it.

I would be more interested in something like below (completely untested).
Thoughts?


From: Adrian Hunter <adrian.hunter@intel.com>
Date: Fri, 17 Jan 2020 11:01:31 +0200
Subject: [PATCH] mmc: sdhci: Add module param debug_spec

End users can have linux compatibility issues. SDHCI provides 2 module
params to help debug such issues: debug_quirks and debug_quirks2. Those
have 3 limitations:
 - they apply to all devices instead of a specific device
 - they overwrite all quirks instead of specific bits
 - they do not cover capabilities
Add a new module parameter debug_spec which addresses all 3 limitations.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci.c | 41 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 6e22a800bded..096cec1afe8e 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -46,6 +46,7 @@
 
 static unsigned int debug_quirks = 0;
 static unsigned int debug_quirks2;
+static char *debug_spec;
 
 static void sdhci_finish_data(struct sdhci_host *);
 
@@ -3860,6 +3861,34 @@ void __sdhci_read_caps(struct sdhci_host *host, const u16 *ver,
 }
 EXPORT_SYMBOL_GPL(__sdhci_read_caps);
 
+static void sdhci_update_debug_quirks_and_caps(struct sdhci_host *host)
+{
+	struct mmc_host *mmc = host->mmc;
+	unsigned int *vars[] = {&debug_quirks, &debug_quirks2, &host->caps,
+				&host->caps1, &mmc->caps, &mmc->caps2};
+	unsigned int mask, val, old, new;
+	const char *p = debug_spec;
+	int ret, var, n;
+	char id[32];
+
+	while (1) {
+		ret = sscanf(p, "%31s %d %x %x %n", id, &var, &mask, &val, &n);
+		if (ret != 4)
+			break;
+		p += n;
+		if (var >= ARRAY_SIZE(vars) ||
+		    strcmp(id, dev_name(mmc_dev(mmc))))
+			continue;
+		old = *vars[var];
+		new = (old & ~mask) | val;
+		if (old != new) {
+			*vars[var] = new;
+			DBG("debug_spec: updating var %d %#x -> %#x\n",
+			    var, old, new);
+		}
+	}
+}
+
 static void sdhci_allocate_bounce_buffer(struct sdhci_host *host)
 {
 	struct mmc_host *mmc = host->mmc;
@@ -3967,6 +3996,8 @@ int sdhci_setup_host(struct sdhci_host *host)
 
 	sdhci_read_caps(host);
 
+	sdhci_update_debug_quirks_and_caps(host);
+
 	override_timeout_clk = host->timeout_clk;
 
 	if (host->version > SDHCI_SPEC_420) {
@@ -4513,6 +4544,8 @@ int __sdhci_add_host(struct sdhci_host *host)
 	struct mmc_host *mmc = host->mmc;
 	int ret;
 
+	sdhci_update_debug_quirks_and_caps(host);
+
 	host->complete_wq = alloc_workqueue("sdhci", flags, 0);
 	if (!host->complete_wq)
 		return -ENOMEM;
@@ -4676,6 +4709,7 @@ module_exit(sdhci_drv_exit);
 
 module_param(debug_quirks, uint, 0444);
 module_param(debug_quirks2, uint, 0444);
+module_param(debug_spec, charp, 0444);
 
 MODULE_AUTHOR("Pierre Ossman <pierre@ossman.eu>");
 MODULE_DESCRIPTION("Secure Digital Host Controller Interface core driver");
@@ -4683,3 +4717,10 @@ MODULE_LICENSE("GPL");
 
 MODULE_PARM_DESC(debug_quirks, "Force certain quirks.");
 MODULE_PARM_DESC(debug_quirks2, "Force certain other quirks.");
+MODULE_PARM_DESC(debug_spec,
+		 "Alter quirks, sdhci capabilities and MMC capabilities. The "
+		 "string contains one or more: <device name> <variable> <mask> "
+		 "<value> where <variable> is 0 (debug_quirks), "
+		 "1 (debug_quirks2), 2 (sdhci->caps), 3 (sdhci->caps1), "
+		 "4 (mmc->caps) or 5 (mmc->caps2), <mask> is a hex bit mask of "
+		 "bits to change, and <value> is the hex value.");
-- 
2.17.1