* [PATCH 0/3] mmc: sdhci: Fix card detect race for Intel BXT/APL
@ 2016-01-22 9:44 Adrian Hunter
2016-01-22 9:45 ` [PATCH 1/3] mmc: sdhci: Add get_cd sdhci host operation Adrian Hunter
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Adrian Hunter @ 2016-01-22 9:44 UTC (permalink / raw)
To: Ulf Hansson
Cc: linux-mmc, Lakshmi Sai Krishna Potthuri, Ivan T. Ivanov,
Russell King
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] mmc: sdhci: Add get_cd sdhci host operation
2016-01-22 9:44 [PATCH 0/3] mmc: sdhci: Fix card detect race for Intel BXT/APL Adrian Hunter
@ 2016-01-22 9:45 ` Adrian Hunter
2016-01-22 12:07 ` Ulf Hansson
2016-01-22 9:45 ` [PATCH 2/3] mmc: sdhci-pci: Fix card detect race for Intel BXT/APL Adrian Hunter
2016-01-22 9:45 ` [PATCH 3/3] mmc: sdhci-acpi: " Adrian Hunter
2 siblings, 1 reply; 6+ messages in thread
From: Adrian Hunter @ 2016-01-22 9:45 UTC (permalink / raw)
To: Ulf Hansson
Cc: linux-mmc, Lakshmi Sai Krishna Potthuri, Ivan T. Ivanov,
Russell King
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] mmc: sdhci-pci: Fix card detect race for Intel BXT/APL
2016-01-22 9:44 [PATCH 0/3] mmc: sdhci: Fix card detect race for Intel BXT/APL Adrian Hunter
2016-01-22 9:45 ` [PATCH 1/3] mmc: sdhci: Add get_cd sdhci host operation Adrian Hunter
@ 2016-01-22 9:45 ` Adrian Hunter
2016-01-22 9:45 ` [PATCH 3/3] mmc: sdhci-acpi: " Adrian Hunter
2 siblings, 0 replies; 6+ messages in thread
From: Adrian Hunter @ 2016-01-22 9:45 UTC (permalink / raw)
To: Ulf Hansson
Cc: linux-mmc, Lakshmi Sai Krishna Potthuri, Ivan T. Ivanov,
Russell King
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] mmc: sdhci-acpi: Fix card detect race for Intel BXT/APL
2016-01-22 9:44 [PATCH 0/3] mmc: sdhci: Fix card detect race for Intel BXT/APL Adrian Hunter
2016-01-22 9:45 ` [PATCH 1/3] mmc: sdhci: Add get_cd sdhci host operation Adrian Hunter
2016-01-22 9:45 ` [PATCH 2/3] mmc: sdhci-pci: Fix card detect race for Intel BXT/APL Adrian Hunter
@ 2016-01-22 9:45 ` Adrian Hunter
2 siblings, 0 replies; 6+ messages in thread
From: Adrian Hunter @ 2016-01-22 9:45 UTC (permalink / raw)
To: Ulf Hansson
Cc: linux-mmc, Lakshmi Sai Krishna Potthuri, Ivan T. Ivanov,
Russell King
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] mmc: sdhci: Add get_cd sdhci host operation
2016-01-22 9:45 ` [PATCH 1/3] mmc: sdhci: Add get_cd sdhci host operation Adrian Hunter
@ 2016-01-22 12:07 ` Ulf Hansson
2016-01-22 14:58 ` Adrian Hunter
0 siblings, 1 reply; 6+ messages in thread
From: Ulf Hansson @ 2016-01-22 12:07 UTC (permalink / raw)
To: Adrian Hunter
Cc: linux-mmc, Lakshmi Sai Krishna Potthuri, Ivan T. Ivanov,
Russell King
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] mmc: sdhci: Add get_cd sdhci host operation
2016-01-22 12:07 ` Ulf Hansson
@ 2016-01-22 14:58 ` Adrian Hunter
0 siblings, 0 replies; 6+ messages in thread
From: Adrian Hunter @ 2016-01-22 14:58 UTC (permalink / raw)
To: Ulf Hansson
Cc: linux-mmc, Lakshmi Sai Krishna Potthuri, Ivan T. Ivanov,
Russell King
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);
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-01-22 15:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-01-22 9:44 [PATCH 0/3] mmc: sdhci: Fix card detect race for Intel BXT/APL Adrian Hunter
2016-01-22 9:45 ` [PATCH 1/3] mmc: sdhci: Add get_cd sdhci host operation Adrian Hunter
2016-01-22 12:07 ` Ulf Hansson
2016-01-22 14:58 ` Adrian Hunter
2016-01-22 9:45 ` [PATCH 2/3] mmc: sdhci-pci: Fix card detect race for Intel BXT/APL Adrian Hunter
2016-01-22 9:45 ` [PATCH 3/3] mmc: sdhci-acpi: " Adrian Hunter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).