Hi,
On 1/17/20 10:16 AM, Adrian Hunter wrote:
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?
First of all, sorry that I've been quite slow to respond to this.
I think your proposal is interesting, although I'm not sure it is
immediately useful for the sdhci-acpi issues from my patch-series
as those are sdhci-acpi / Intel DSM specific. I guess we could still
use quirk flags at the shared sdhci level for them despite this, but
I'm not sure if that is the right approach.
For now for the v2 of this series which I'm preparing I've just removed
the module option. If we feel the need for something like this later we
can always add it later.
Regards,
Hans
From: Adrian Hunter <adrian.hunter@xxxxxxxxx>
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@xxxxxxxxx>
---
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@xxxxxxxxx>");
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.");
