[PATCH 0/3] mmc: sdhci: Fix card detect race for Intel BXT/APL

[PATCH 0/3] mmc: sdhci: Fix card detect race for Intel BXT/APL

[Adrian Hunter]

Hi

Here are 3 little 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+.

AFAICT the patches do not conflict with other SDHCI patches
on the mailing list at the moment.


Adrian Hunter (3):
      mmc: sdhci: Add get_cd sdhci host operation
      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     | 35 ++++++++++++++++++++++++++++++++++-
 drivers/mmc/host/sdhci-pci-core.c | 27 +++++++++++++++++++++++++++
 drivers/mmc/host/sdhci.c          |  6 +++++-
 drivers/mmc/host/sdhci.h          |  1 +
 4 files changed, 67 insertions(+), 2 deletions(-)


Regards
Adrian

[PATCH 1/3] mmc: sdhci: Add get_cd sdhci host operation

[Adrian Hunter]

In the future sdhci will be a library.  When that happens
a callback will be needed to allow drivers easily to override
the standard way of determining whether a card is present.
That is needed because functions like sdhci_request() also
check the card presence.

The get_cd callback is being added now to facilitate
a bug fix, for which subsequent patches are provided.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Cc: stable@vger.kernel.org # v4.4+
---
 drivers/mmc/host/sdhci.c | 6 +++++-
 drivers/mmc/host/sdhci.h | 1 +
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index d622435d1bcc..535236084b27 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1612,7 +1612,7 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 
 static int sdhci_do_get_cd(struct sdhci_host *host)
 {
-	int gpio_cd = mmc_gpio_get_cd(host->mmc);
+	int gpio_cd;
 
 	if (host->flags & SDHCI_DEVICE_DEAD)
 		return 0;
@@ -1621,10 +1621,14 @@ static int sdhci_do_get_cd(struct sdhci_host *host)
 	if (host->mmc->caps & MMC_CAP_NONREMOVABLE)
 		return 1;
 
+	if (host->ops->get_cd)
+		return host->ops->get_cd(host);
+
 	/*
 	 * Try slot gpio detect, if defined it take precedence
 	 * over build in controller functionality
 	 */
+	gpio_cd = mmc_gpio_get_cd(host->mmc);
 	if (!IS_ERR_VALUE(gpio_cd))
 		return !!gpio_cd;
 
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 7654ae5d2b4e..a6c2cd8ef0b2 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -552,6 +552,7 @@ struct sdhci_ops {
 					 struct mmc_card *card,
 					 unsigned int max_dtr, int host_drv,
 					 int card_drv, int *drv_type);
+	int	(*get_cd)(struct sdhci_host *host);
 };
 
 #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
-- 
1.9.1

Re: [PATCH 1/3] mmc: sdhci: Add get_cd sdhci host operation

[Ulf Hansson]

On 22 January 2016 at 10:45, Adrian Hunter <adrian.hunter@intel.com> wrote:
> In the future sdhci will be a library.  When that happens
> a callback will be needed to allow drivers easily to override
> the standard way of determining whether a card is present.

There's already a ->get_cd() callback in the struct mmc_host_ops.

By turning sdhci into a library, each sdhci variant should decide
whether they want to use a default "sdhci_get_cd()" version, rely on
slot-gpio or implement their own.

I don't understand why we need a specific sdhci callback to deal with
this, it doesn't make sense to me.

> That is needed because functions like sdhci_request() also
> check the card presence.

Why does sdhci do that? Is that because some variants needs it or
because of all?

Reading a state of a GPIO pin before every request doesn't come
without a cost in performance. My point is, it should only be done
when *really* needed.
Again, by having sdhci to become a library each variant should decide
what's needed for them.

>
> The get_cd callback is being added now to facilitate
> a bug fix, for which subsequent patches are provided.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> Cc: stable@vger.kernel.org # v4.4+
> ---
>  drivers/mmc/host/sdhci.c | 6 +++++-
>  drivers/mmc/host/sdhci.h | 1 +
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index d622435d1bcc..535236084b27 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1612,7 +1612,7 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>
>  static int sdhci_do_get_cd(struct sdhci_host *host)
>  {
> -       int gpio_cd = mmc_gpio_get_cd(host->mmc);
> +       int gpio_cd;
>
>         if (host->flags & SDHCI_DEVICE_DEAD)
>                 return 0;
> @@ -1621,10 +1621,14 @@ static int sdhci_do_get_cd(struct sdhci_host *host)
>         if (host->mmc->caps & MMC_CAP_NONREMOVABLE)
>                 return 1;
>
> +       if (host->ops->get_cd)
> +               return host->ops->get_cd(host);
> +
>         /*
>          * Try slot gpio detect, if defined it take precedence
>          * over build in controller functionality
>          */
> +       gpio_cd = mmc_gpio_get_cd(host->mmc);
>         if (!IS_ERR_VALUE(gpio_cd))
>                 return !!gpio_cd;
>
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 7654ae5d2b4e..a6c2cd8ef0b2 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -552,6 +552,7 @@ struct sdhci_ops {
>                                          struct mmc_card *card,
>                                          unsigned int max_dtr, int host_drv,
>                                          int card_drv, int *drv_type);
> +       int     (*get_cd)(struct sdhci_host *host);
>  };
>
>  #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
> --
> 1.9.1
>

Can you please try to fix this without adding a new callback!? I know,
it requires some more efforts but for sure it's doable.

Kind regards
Uffe

Re: [PATCH 1/3] mmc: sdhci: Add get_cd sdhci host operation

[Adrian Hunter]

On 22/01/16 14:07, Ulf Hansson wrote:
> On 22 January 2016 at 10:45, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> In the future sdhci will be a library.  When that happens
>> a callback will be needed to allow drivers easily to override
>> the standard way of determining whether a card is present.
> 
> There's already a ->get_cd() callback in the struct mmc_host_ops.
> 
> By turning sdhci into a library, each sdhci variant should decide
> whether they want to use a default "sdhci_get_cd()" version, rely on
> slot-gpio or implement their own.
> 
> I don't understand why we need a specific sdhci callback to deal with
> this, it doesn't make sense to me.
> 
>> That is needed because functions like sdhci_request() also
>> check the card presence.
> 
> Why does sdhci do that? Is that because some variants needs it or
> because of all?

There has been a check for card present in the request function since the
driver was added in 2006.  On our host controllers, and I presume others, if
a command is started when the host controller has decided there is no card,
then nothing happens.  No interrupts, no errors, it just sits there.
The driver has a 10-second timer that eventually catches it.

So that is the reason for checking the Present State register.  When GPIO
support was added, people just decided to substitute the check.

> 
> Reading a state of a GPIO pin before every request doesn't come
> without a cost in performance. My point is, it should only be done
> when *really* needed.
> Again, by having sdhci to become a library each variant should decide
> what's needed for them.

Sure but this is a bug fix, ideally for stable.  It can't deal with
unrelated improvements.

> 
>>
>> The get_cd callback is being added now to facilitate
>> a bug fix, for which subsequent patches are provided.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> Cc: stable@vger.kernel.org # v4.4+
>> ---
>>  drivers/mmc/host/sdhci.c | 6 +++++-
>>  drivers/mmc/host/sdhci.h | 1 +
>>  2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index d622435d1bcc..535236084b27 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -1612,7 +1612,7 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>
>>  static int sdhci_do_get_cd(struct sdhci_host *host)
>>  {
>> -       int gpio_cd = mmc_gpio_get_cd(host->mmc);
>> +       int gpio_cd;
>>
>>         if (host->flags & SDHCI_DEVICE_DEAD)
>>                 return 0;
>> @@ -1621,10 +1621,14 @@ static int sdhci_do_get_cd(struct sdhci_host *host)
>>         if (host->mmc->caps & MMC_CAP_NONREMOVABLE)
>>                 return 1;
>>
>> +       if (host->ops->get_cd)
>> +               return host->ops->get_cd(host);
>> +
>>         /*
>>          * Try slot gpio detect, if defined it take precedence
>>          * over build in controller functionality
>>          */
>> +       gpio_cd = mmc_gpio_get_cd(host->mmc);
>>         if (!IS_ERR_VALUE(gpio_cd))
>>                 return !!gpio_cd;
>>
>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>> index 7654ae5d2b4e..a6c2cd8ef0b2 100644
>> --- a/drivers/mmc/host/sdhci.h
>> +++ b/drivers/mmc/host/sdhci.h
>> @@ -552,6 +552,7 @@ struct sdhci_ops {
>>                                          struct mmc_card *card,
>>                                          unsigned int max_dtr, int host_drv,
>>                                          int card_drv, int *drv_type);
>> +       int     (*get_cd)(struct sdhci_host *host);
>>  };
>>
>>  #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
>> --
>> 1.9.1
>>
> 
> Can you please try to fix this without adding a new callback!? I know,
> it requires some more efforts but for sure it's doable.

This is a bug fix for v4.4+ so there is limited scope for major changes.
Could do the change below and replace the other get_cd.  Thoughts?

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index ba4f5a0a7b06..e7dd4c82c5f1 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1301,7 +1301,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);

[PATCH 2/3] mmc: sdhci-pci: Fix card detect race for Intel BXT/APL

[Adrian Hunter]

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' sdhci host operation for BXT/APL that does that.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Cc: stable@vger.kernel.org # v4.4+
---
 drivers/mmc/host/sdhci-pci-core.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
index cc851b065d0a..c92652142860 100644
--- a/drivers/mmc/host/sdhci-pci-core.c
+++ b/drivers/mmc/host/sdhci-pci-core.c
@@ -330,6 +330,18 @@ 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 sdhci_host *host)
+{
+	int gpio_cd = mmc_gpio_get_cd(host->mmc);
+
+	if (!gpio_cd)
+		return 0;
+
+	return !!(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT);
+}
+
+static const struct sdhci_ops bxt_ops;
+
 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 +374,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->ops = &bxt_ops;
+
 	return 0;
 }
 
@@ -1362,6 +1378,17 @@ static int sdhci_pci_select_drive_strength(struct sdhci_host *host,
 					   card_drv, drv_type);
 }
 
+static const struct sdhci_ops bxt_ops = {
+	.set_clock	= sdhci_set_clock,
+	.enable_dma	= sdhci_pci_enable_dma,
+	.set_bus_width	= sdhci_pci_set_bus_width,
+	.reset		= sdhci_reset,
+	.set_uhs_signaling = sdhci_set_uhs_signaling,
+	.hw_reset		= sdhci_pci_hw_reset,
+	.select_drive_strength	= sdhci_pci_select_drive_strength,
+	.get_cd		= bxt_get_cd,
+};
+
 static const struct sdhci_ops sdhci_pci_ops = {
 	.set_clock	= sdhci_set_clock,
 	.enable_dma	= sdhci_pci_enable_dma,
-- 
1.9.1

[PATCH 3/3] mmc: sdhci-acpi: Fix card detect race for Intel BXT/APL

[Adrian Hunter]

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' sdhci host operation for BXT/APL that does that.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Cc: stable@vger.kernel.org # v4.4+
---
 drivers/mmc/host/sdhci-acpi.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index f6047fc94062..d330e99bdd87 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -146,6 +146,29 @@ static const struct sdhci_acpi_chip sdhci_acpi_chip_int = {
 	.ops = &sdhci_acpi_ops_int,
 };
 
+static int bxt_get_cd(struct sdhci_host *host)
+{
+	int gpio_cd = mmc_gpio_get_cd(host->mmc);
+
+	if (!gpio_cd)
+		return 0;
+
+	return !!(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT);
+}
+
+static const struct sdhci_ops sdhci_acpi_ops_bxt = {
+	.set_clock = sdhci_set_clock,
+	.enable_dma = sdhci_acpi_enable_dma,
+	.set_bus_width = sdhci_set_bus_width,
+	.reset = sdhci_reset,
+	.set_uhs_signaling = sdhci_set_uhs_signaling,
+	.get_cd = bxt_get_cd,
+};
+
+static const struct sdhci_acpi_chip sdhci_acpi_chip_bxt = {
+	.ops = &sdhci_acpi_ops_bxt,
+};
+
 static int sdhci_acpi_emmc_probe_slot(struct platform_device *pdev,
 				      const char *hid, const char *uid)
 {
@@ -234,6 +257,16 @@ static const struct sdhci_acpi_slot sdhci_acpi_slot_int_sd = {
 	.probe_slot	= sdhci_acpi_sd_probe_slot,
 };
 
+static const struct sdhci_acpi_slot sdhci_acpi_slot_bxt_sd = {
+	.chip    = &sdhci_acpi_chip_bxt,
+	.flags   = SDHCI_ACPI_SD_CD | SDHCI_ACPI_SD_CD_OVERRIDE_LEVEL |
+		   SDHCI_ACPI_RUNTIME_PM,
+	.quirks  = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
+	.quirks2 = SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON |
+		   SDHCI_QUIRK2_STOP_WITH_TC,
+	.caps    = MMC_CAP_BUS_WIDTH_TEST | MMC_CAP_WAIT_WHILE_BUSY,
+};
+
 struct sdhci_acpi_uid_slot {
 	const char *hid;
 	const char *uid;
@@ -241,7 +274,7 @@ struct sdhci_acpi_uid_slot {
 };
 
 static const struct sdhci_acpi_uid_slot sdhci_acpi_uids[] = {
-	{ "80865ACA", NULL, &sdhci_acpi_slot_int_sd },
+	{ "80865ACA", NULL, &sdhci_acpi_slot_bxt_sd },
 	{ "80865ACC", NULL, &sdhci_acpi_slot_int_emmc },
 	{ "80865AD0", NULL, &sdhci_acpi_slot_int_sdio },
 	{ "80860F14" , "1" , &sdhci_acpi_slot_int_emmc },
-- 
1.9.1