mmc: sdhci: Fix card detect race for Intel BXT/APL

Hi

Here is V2 of patches to fix a problem on Intel BXT/APL. The PCI and ACPI Ids for BXT/APL were added in v4.4 so the patches are cc stable v4.4+.

Changes in V2:

Override the mmc host ->get_cd() operation instead of adding a new sdhci host operation.

Adrian Hunter (4):
mmc: sdhci: Allow override of mmc host operations
mmc: sdhci: Allow override of get_cd() called from sdhci_request()
mmc: sdhci-pci: Fix card detect race for Intel BXT/APL
mmc: sdhci-acpi: Fix card detect race for Intel BXT/APL

 drivers/mmc/host/sdhci-acpi.c     | 30 ++++++++++++++++++++++++++++++
 drivers/mmc/host/sdhci-pci-core.c | 31 +++++++++++++++++++++++++++++++
 drivers/mmc/host/sdhci.c          |  5 +++--
 drivers/mmc/host/sdhci.h          |  1 +
 4 files changed, 65 insertions(+), 2 deletions(-)

Regards
Adrian

In the past, fixes for specific hardware devices were implemented in sdhci using quirks. That approach is no longer accepted because the growing number of quirks was starting to make the code difficult to understand and maintain.

One alternative to quirks, is to allow drivers to override the default mmc host operations. This patch makes it easy to do that, and it is needed for a subsequent bug fix, for which separate patches are provided.

---
 drivers/mmc/host/sdhci.c | 3 ++-
 drivers/mmc/host/sdhci.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

index d622435d1bcc..9bfa66df963a 100644
@@ -2849,6 +2849,8 @@ struct sdhci_host *sdhci_alloc_host(struct device *dev,
 
 	host = mmc_priv(mmc);
 	host->mmc = mmc;
+	host->mmc_host_ops = sdhci_ops;
+	mmc->ops = &host->mmc_host_ops;
 
 	return host;
 }

@@ -3037,7 +3039,6 @@ int sdhci_add_host(struct sdhci_host *host)
 	/*
 	 * Set host parameters.
 	 */
-	mmc->ops = &sdhci_ops;
 	max_clk = host->max_clk;
 
 	if (host->ops->get_min_clock)

index 7654ae5d2b4e..0115e9907bf8 100644
@@ -430,6 +430,7 @@ struct sdhci_host {
 
 	/* Internal data */
 	struct mmc_host *mmc;	/* MMC structure */
+	struct mmc_host_ops mmc_host_ops;	/* MMC host ops */
 	u64 dma_mask;		/* custom DMA mask */
 
 #if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)

1.9.1

Drivers may need to provide their own get_cd() mmc host op, but currently the internals of the current op (sdhci_get_cd()) are provided by sdhci_do_get_cd() which is also called from sdhci_request().

To allow override of the get_cd functionality, change sdhci_request() to call ->get_cd() instead of sdhci_do_get_cd().

Note, in the future the call to ->get_cd() will likely be removed from sdhci_request() since most drivers don't need actually it. However this change is being done now to facilitate a subsequent bug fix.

---
 drivers/mmc/host/sdhci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 9bfa66df963a..add9fdfd1d8f 100644
@@ -1360,7 +1360,7 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 	sdhci_runtime_pm_get(host);
 
 	/* Firstly check card presence */
-	present = sdhci_do_get_cd(host);
+	present = mmc->ops->get_cd(mmc);
 
 	spin_lock_irqsave(&host->lock, flags);
 

1.9.1

Intel BXT/APL use a card detect GPIO however the host controller will not enable bus power unless it's card detect also reflects the presence of a card. Unfortunately those 2 things race which can result in commands not starting, after which the controller does nothing and there is a 10 second wait for the driver's 10-second timer to timeout.

That is fixed by having the driver look also at the present state register to determine if the card is present. Consequently, provide a 'get_cd' mmc host operation for BXT/APL that does that.

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

index cc851b065d0a..df3b8eced8c4 100644
@@ -330,6 +330,33 @@ static void spt_read_drive_strength(struct sdhci_host *host)
 	sdhci_pci_spt_drive_strength = 0x10 | ((val >> 12) & 0xf);
 }
 
+static int bxt_get_cd(struct mmc_host *mmc)
+{
+	int gpio_cd = mmc_gpio_get_cd(mmc);
+	struct sdhci_host *host = mmc_priv(mmc);
+	unsigned long flags;
+	int ret = 0;
+
+	if (!gpio_cd)
+		return 0;
+
+	pm_runtime_get_sync(mmc->parent);
+
+	spin_lock_irqsave(&host->lock, flags);
+
+	if (host->flags & SDHCI_DEVICE_DEAD)
+		goto out;
+
+	ret = !!(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT);
+out:
+	spin_unlock_irqrestore(&host->lock, flags);
+
+	pm_runtime_mark_last_busy(mmc->parent);
+	pm_runtime_put_autosuspend(mmc->parent);
+
+	return ret;
+}
+
 static int byt_emmc_probe_slot(struct sdhci_pci_slot *slot)
 {
 	slot->host->mmc->caps |= MMC_CAP_8_BIT_DATA | MMC_CAP_NONREMOVABLE |

@@ -362,6 +389,10 @@ static int byt_sd_probe_slot(struct sdhci_pci_slot *slot)
 	slot->cd_con_id = NULL;
 	slot->cd_idx = 0;
 	slot->cd_override_level = true;
+	if (slot->chip->pdev->device == PCI_DEVICE_ID_INTEL_BXT_SD ||
+	    slot->chip->pdev->device == PCI_DEVICE_ID_INTEL_APL_SD)
+		slot->host->mmc_host_ops.get_cd = bxt_get_cd;
+
 	return 0;
 }
 

1.9.1

Intel BXT/APL use a card detect GPIO however the host controller will not enable bus power unless it's card detect also reflects the presence of a card. Unfortunately those 2 things race which can result in commands not starting, after which the controller does nothing and there is a 10 second wait for the driver's 10-second timer to timeout.

That is fixed by having the driver look also at the present state register to determine if the card is present. Consequently, provide a 'get_cd' mmc host operation for BXT/APL that does that.

---
 drivers/mmc/host/sdhci-acpi.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

index 3d27f2de0054..195ff0853dc8 100644
@@ -146,6 +146,33 @@ static const struct sdhci_acpi_chip sdhci_acpi_chip_int = {
 	.ops = &sdhci_acpi_ops_int,
 };
 
+static int bxt_get_cd(struct mmc_host *mmc)
+{
+	int gpio_cd = mmc_gpio_get_cd(mmc);
+	struct sdhci_host *host = mmc_priv(mmc);
+	unsigned long flags;
+	int ret = 0;
+
+	if (!gpio_cd)
+		return 0;
+
+	pm_runtime_get_sync(mmc->parent);
+
+	spin_lock_irqsave(&host->lock, flags);
+
+	if (host->flags & SDHCI_DEVICE_DEAD)
+		goto out;
+
+	ret = !!(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT);
+out:
+	spin_unlock_irqrestore(&host->lock, flags);
+
+	pm_runtime_mark_last_busy(mmc->parent);
+	pm_runtime_put_autosuspend(mmc->parent);
+
+	return ret;
+}
+
 static int sdhci_acpi_emmc_probe_slot(struct platform_device *pdev,
 				      const char *hid, const char *uid)
 {

@@ -196,6 +223,9 @@ static int sdhci_acpi_sd_probe_slot(struct platform_device *pdev,
 
 	/* Platform specific code during sd probe slot goes here */
 
+	if (hid && !strcmp(hid, "80865ACA"))
+		host->mmc_host_ops.get_cd = bxt_get_cd;
+
 	return 0;
 }
 

1.9.1

This seems like step in good direction for sdhci, as well as it solves a regression.

Do you want me to pick it up for fixes?

Kind regards
Uffe

That would be good, thanks!

Thanks, applied for fixes!

Kind regards
Uffe