* [PATCH v2 1/2] mmc: sdhci-acpi: Sync quirks with sdhci_intel_byt_* from sdhci-pci-core.c
@ 2017-03-15 16:31 Hans de Goede
2017-03-15 16:31 ` [PATCH v2 2/2] mmc: sdhci: intel: Disable runtime pm when the sdio_irq is enabled Hans de Goede
0 siblings, 1 reply; 10+ messages in thread
From: Hans de Goede @ 2017-03-15 16:31 UTC (permalink / raw)
To: Adrian Hunter, Ulf Hansson; +Cc: Hans de Goede, linux-mmc
The sdhci_intel_byt_* and the non _qcom_ sdhci_acpi_slot_ entries in
sdhci-acpi.c are for the same mmc host in either pci or acpi mapped
mode, so the flags should be the same.
The syncing removes the SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk from
sdhci_acpi_slot_int_sdio, this quirk is a nop as this slot also has
MMC_CAP_NONREMOVABLE, and adds SDHCI_QUIRK2_PRESET_VALUE_BROKEN to
sdhci_acpi_slot_int_sdio and sdhci_acpi_slot_int_sd. Note that
sdhci_acpi_slot_int_mmc already has the SDHCI_QUIRK2_PRESET_VALUE_BROKEN
quirk, which is another indication that the others need it too.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/mmc/host/sdhci-acpi.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index 9dcb704..237f318 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -286,9 +286,9 @@ static const struct sdhci_acpi_slot sdhci_acpi_slot_int_emmc = {
};
static const struct sdhci_acpi_slot sdhci_acpi_slot_int_sdio = {
- .quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION |
- SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
- .quirks2 = SDHCI_QUIRK2_HOST_OFF_CARD_ON,
+ .quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
+ .quirks2 = SDHCI_QUIRK2_HOST_OFF_CARD_ON |
+ SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
.caps = MMC_CAP_NONREMOVABLE | MMC_CAP_POWER_OFF_CARD |
MMC_CAP_WAIT_WHILE_BUSY,
.flags = SDHCI_ACPI_RUNTIME_PM,
@@ -301,6 +301,7 @@ static const struct sdhci_acpi_slot sdhci_acpi_slot_int_sd = {
SDHCI_ACPI_RUNTIME_PM,
.quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
.quirks2 = SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON |
+ SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
SDHCI_QUIRK2_STOP_WITH_TC,
.caps = MMC_CAP_WAIT_WHILE_BUSY,
.probe_slot = sdhci_acpi_sd_probe_slot,
--
2.9.3
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] mmc: sdhci: intel: Disable runtime pm when the sdio_irq is enabled.
2017-03-15 16:31 [PATCH v2 1/2] mmc: sdhci-acpi: Sync quirks with sdhci_intel_byt_* from sdhci-pci-core.c Hans de Goede
@ 2017-03-15 16:31 ` Hans de Goede
2017-03-21 10:00 ` Ulf Hansson
0 siblings, 1 reply; 10+ messages in thread
From: Hans de Goede @ 2017-03-15 16:31 UTC (permalink / raw)
To: Adrian Hunter, Ulf Hansson; +Cc: Hans de Goede, linux-mmc, Dong Aisheng
SDIO cards may need clock to send the card interrupt to the host.
On a cherrytrail tablet with a RTL8723BS wifi chip, without this patch
pinging the tablet results in:
PING 192.168.1.14 (192.168.1.14) 56(84) bytes of data.
64 bytes from 192.168.1.14: icmp_seq=1 ttl=64 time=78.6 ms
64 bytes from 192.168.1.14: icmp_seq=2 ttl=64 time=1760 ms
64 bytes from 192.168.1.14: icmp_seq=3 ttl=64 time=753 ms
64 bytes from 192.168.1.14: icmp_seq=4 ttl=64 time=3.88 ms
64 bytes from 192.168.1.14: icmp_seq=5 ttl=64 time=795 ms
64 bytes from 192.168.1.14: icmp_seq=6 ttl=64 time=1841 ms
64 bytes from 192.168.1.14: icmp_seq=7 ttl=64 time=810 ms
64 bytes from 192.168.1.14: icmp_seq=8 ttl=64 time=1860 ms
64 bytes from 192.168.1.14: icmp_seq=9 ttl=64 time=812 ms
64 bytes from 192.168.1.14: icmp_seq=10 ttl=64 time=48.6 ms
Where as with this patch I get:
PING 192.168.1.14 (192.168.1.14) 56(84) bytes of data.
64 bytes from 192.168.1.14: icmp_seq=1 ttl=64 time=3.96 ms
64 bytes from 192.168.1.14: icmp_seq=2 ttl=64 time=1.97 ms
64 bytes from 192.168.1.14: icmp_seq=3 ttl=64 time=17.2 ms
64 bytes from 192.168.1.14: icmp_seq=4 ttl=64 time=2.46 ms
64 bytes from 192.168.1.14: icmp_seq=5 ttl=64 time=2.83 ms
64 bytes from 192.168.1.14: icmp_seq=6 ttl=64 time=1.40 ms
64 bytes from 192.168.1.14: icmp_seq=7 ttl=64 time=2.10 ms
64 bytes from 192.168.1.14: icmp_seq=8 ttl=64 time=1.40 ms
64 bytes from 192.168.1.14: icmp_seq=9 ttl=64 time=2.04 ms
64 bytes from 192.168.1.14: icmp_seq=10 ttl=64 time=1.40 ms
Cc: Dong Aisheng <b29396@freescale.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-New patch replacing "mmc: sdhci: sdio-intel: Set
SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON quirk"
---
drivers/mmc/host/sdhci-acpi.c | 1 +
drivers/mmc/host/sdhci-pci-core.c | 1 +
drivers/mmc/host/sdhci.c | 6 ++++++
drivers/mmc/host/sdhci.h | 2 ++
4 files changed, 10 insertions(+)
diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index 237f318..53b431a 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -288,6 +288,7 @@ static const struct sdhci_acpi_slot sdhci_acpi_slot_int_emmc = {
static const struct sdhci_acpi_slot sdhci_acpi_slot_int_sdio = {
.quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
.quirks2 = SDHCI_QUIRK2_HOST_OFF_CARD_ON |
+ SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM |
SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
.caps = MMC_CAP_NONREMOVABLE | MMC_CAP_POWER_OFF_CARD |
MMC_CAP_WAIT_WHILE_BUSY,
diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
index 982b3e3..a3b242e 100644
--- a/drivers/mmc/host/sdhci-pci-core.c
+++ b/drivers/mmc/host/sdhci-pci-core.c
@@ -498,6 +498,7 @@ static const struct sdhci_pci_fixes sdhci_ni_byt_sdio = {
static const struct sdhci_pci_fixes sdhci_intel_byt_sdio = {
.quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
.quirks2 = SDHCI_QUIRK2_HOST_OFF_CARD_ON |
+ SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM |
SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
.allow_runtime_pm = true,
.probe_slot = byt_sdio_probe_slot,
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 6fdd7a7..59c5cad 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1828,6 +1828,9 @@ static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
struct sdhci_host *host = mmc_priv(mmc);
unsigned long flags;
+ if ((host->quirks2 & SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM) && enable)
+ pm_runtime_get(host->mmc->parent);
+
spin_lock_irqsave(&host->lock, flags);
if (enable)
host->flags |= SDHCI_SDIO_IRQ_ENABLED;
@@ -1836,6 +1839,9 @@ static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
sdhci_enable_sdio_irq_nolock(host, enable);
spin_unlock_irqrestore(&host->lock, flags);
+
+ if ((host->quirks2 & SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM) && !enable)
+ pm_runtime_put(host->mmc->parent);
}
static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index edf3adf..b72c38d 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -427,6 +427,8 @@ struct sdhci_host {
#define SDHCI_QUIRK2_ACMD23_BROKEN (1<<14)
/* Broken Clock divider zero in controller */
#define SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN (1<<15)
+/* Must not runtime suspend when sdio_irq is enabled */
+#define SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM (1<<16)
int irq; /* Device IRQ */
void __iomem *ioaddr; /* Mapped address */
--
2.9.3
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] mmc: sdhci: intel: Disable runtime pm when the sdio_irq is enabled.
2017-03-15 16:31 ` [PATCH v2 2/2] mmc: sdhci: intel: Disable runtime pm when the sdio_irq is enabled Hans de Goede
@ 2017-03-21 10:00 ` Ulf Hansson
2017-03-21 12:34 ` Adrian Hunter
0 siblings, 1 reply; 10+ messages in thread
From: Ulf Hansson @ 2017-03-21 10:00 UTC (permalink / raw)
To: Hans de Goede; +Cc: Adrian Hunter, linux-mmc, Dong Aisheng
On 15 March 2017 at 17:31, Hans de Goede <hdegoede@redhat.com> wrote:
> SDIO cards may need clock to send the card interrupt to the host.
>
> On a cherrytrail tablet with a RTL8723BS wifi chip, without this patch
> pinging the tablet results in:
>
> PING 192.168.1.14 (192.168.1.14) 56(84) bytes of data.
> 64 bytes from 192.168.1.14: icmp_seq=1 ttl=64 time=78.6 ms
> 64 bytes from 192.168.1.14: icmp_seq=2 ttl=64 time=1760 ms
> 64 bytes from 192.168.1.14: icmp_seq=3 ttl=64 time=753 ms
> 64 bytes from 192.168.1.14: icmp_seq=4 ttl=64 time=3.88 ms
> 64 bytes from 192.168.1.14: icmp_seq=5 ttl=64 time=795 ms
> 64 bytes from 192.168.1.14: icmp_seq=6 ttl=64 time=1841 ms
> 64 bytes from 192.168.1.14: icmp_seq=7 ttl=64 time=810 ms
> 64 bytes from 192.168.1.14: icmp_seq=8 ttl=64 time=1860 ms
> 64 bytes from 192.168.1.14: icmp_seq=9 ttl=64 time=812 ms
> 64 bytes from 192.168.1.14: icmp_seq=10 ttl=64 time=48.6 ms
>
> Where as with this patch I get:
>
> PING 192.168.1.14 (192.168.1.14) 56(84) bytes of data.
> 64 bytes from 192.168.1.14: icmp_seq=1 ttl=64 time=3.96 ms
> 64 bytes from 192.168.1.14: icmp_seq=2 ttl=64 time=1.97 ms
> 64 bytes from 192.168.1.14: icmp_seq=3 ttl=64 time=17.2 ms
> 64 bytes from 192.168.1.14: icmp_seq=4 ttl=64 time=2.46 ms
> 64 bytes from 192.168.1.14: icmp_seq=5 ttl=64 time=2.83 ms
> 64 bytes from 192.168.1.14: icmp_seq=6 ttl=64 time=1.40 ms
> 64 bytes from 192.168.1.14: icmp_seq=7 ttl=64 time=2.10 ms
> 64 bytes from 192.168.1.14: icmp_seq=8 ttl=64 time=1.40 ms
> 64 bytes from 192.168.1.14: icmp_seq=9 ttl=64 time=2.04 ms
> 64 bytes from 192.168.1.14: icmp_seq=10 ttl=64 time=1.40 ms
>
> Cc: Dong Aisheng <b29396@freescale.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -New patch replacing "mmc: sdhci: sdio-intel: Set
> SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON quirk"
> ---
> drivers/mmc/host/sdhci-acpi.c | 1 +
> drivers/mmc/host/sdhci-pci-core.c | 1 +
> drivers/mmc/host/sdhci.c | 6 ++++++
> drivers/mmc/host/sdhci.h | 2 ++
> 4 files changed, 10 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
> index 237f318..53b431a 100644
> --- a/drivers/mmc/host/sdhci-acpi.c
> +++ b/drivers/mmc/host/sdhci-acpi.c
> @@ -288,6 +288,7 @@ static const struct sdhci_acpi_slot sdhci_acpi_slot_int_emmc = {
> static const struct sdhci_acpi_slot sdhci_acpi_slot_int_sdio = {
> .quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
> .quirks2 = SDHCI_QUIRK2_HOST_OFF_CARD_ON |
> + SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM |
> SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
> .caps = MMC_CAP_NONREMOVABLE | MMC_CAP_POWER_OFF_CARD |
> MMC_CAP_WAIT_WHILE_BUSY,
> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
> index 982b3e3..a3b242e 100644
> --- a/drivers/mmc/host/sdhci-pci-core.c
> +++ b/drivers/mmc/host/sdhci-pci-core.c
> @@ -498,6 +498,7 @@ static const struct sdhci_pci_fixes sdhci_ni_byt_sdio = {
> static const struct sdhci_pci_fixes sdhci_intel_byt_sdio = {
> .quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
> .quirks2 = SDHCI_QUIRK2_HOST_OFF_CARD_ON |
> + SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM |
> SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
> .allow_runtime_pm = true,
> .probe_slot = byt_sdio_probe_slot,
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 6fdd7a7..59c5cad 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1828,6 +1828,9 @@ static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
> struct sdhci_host *host = mmc_priv(mmc);
> unsigned long flags;
>
> + if ((host->quirks2 & SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM) && enable)
> + pm_runtime_get(host->mmc->parent);
There are currently no sdhci variant for any SoC that implements
wakeup support in runtime suspend for SDIO irqs. In other words, it
seems all sdhci hosts suffers from the same problem.
I imagine there are vendor trees implementing wakeups, however until
we see such code being pushed upstream, I suggest we make this the
default behavior instead of using a quirk.
> +
> spin_lock_irqsave(&host->lock, flags);
> if (enable)
> host->flags |= SDHCI_SDIO_IRQ_ENABLED;
> @@ -1836,6 +1839,9 @@ static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
>
> sdhci_enable_sdio_irq_nolock(host, enable);
> spin_unlock_irqrestore(&host->lock, flags);
> +
> + if ((host->quirks2 & SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM) && !enable)
> + pm_runtime_put(host->mmc->parent);
> }
>
> static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index edf3adf..b72c38d 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -427,6 +427,8 @@ struct sdhci_host {
> #define SDHCI_QUIRK2_ACMD23_BROKEN (1<<14)
> /* Broken Clock divider zero in controller */
> #define SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN (1<<15)
> +/* Must not runtime suspend when sdio_irq is enabled */
> +#define SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM (1<<16)
>
> int irq; /* Device IRQ */
> void __iomem *ioaddr; /* Mapped address */
> --
> 2.9.3
>
Kind regards
Uffe
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] mmc: sdhci: intel: Disable runtime pm when the sdio_irq is enabled.
2017-03-21 10:00 ` Ulf Hansson
@ 2017-03-21 12:34 ` Adrian Hunter
2017-03-21 13:30 ` Hans de Goede
2017-03-21 14:54 ` Ulf Hansson
0 siblings, 2 replies; 10+ messages in thread
From: Adrian Hunter @ 2017-03-21 12:34 UTC (permalink / raw)
To: Ulf Hansson, Hans de Goede; +Cc: linux-mmc, Dong Aisheng
On 21/03/17 12:00, Ulf Hansson wrote:
> On 15 March 2017 at 17:31, Hans de Goede <hdegoede@redhat.com> wrote:
>> SDIO cards may need clock to send the card interrupt to the host.
>>
>> On a cherrytrail tablet with a RTL8723BS wifi chip, without this patch
>> pinging the tablet results in:
>>
>> PING 192.168.1.14 (192.168.1.14) 56(84) bytes of data.
>> 64 bytes from 192.168.1.14: icmp_seq=1 ttl=64 time=78.6 ms
>> 64 bytes from 192.168.1.14: icmp_seq=2 ttl=64 time=1760 ms
>> 64 bytes from 192.168.1.14: icmp_seq=3 ttl=64 time=753 ms
>> 64 bytes from 192.168.1.14: icmp_seq=4 ttl=64 time=3.88 ms
>> 64 bytes from 192.168.1.14: icmp_seq=5 ttl=64 time=795 ms
>> 64 bytes from 192.168.1.14: icmp_seq=6 ttl=64 time=1841 ms
>> 64 bytes from 192.168.1.14: icmp_seq=7 ttl=64 time=810 ms
>> 64 bytes from 192.168.1.14: icmp_seq=8 ttl=64 time=1860 ms
>> 64 bytes from 192.168.1.14: icmp_seq=9 ttl=64 time=812 ms
>> 64 bytes from 192.168.1.14: icmp_seq=10 ttl=64 time=48.6 ms
>>
>> Where as with this patch I get:
>>
>> PING 192.168.1.14 (192.168.1.14) 56(84) bytes of data.
>> 64 bytes from 192.168.1.14: icmp_seq=1 ttl=64 time=3.96 ms
>> 64 bytes from 192.168.1.14: icmp_seq=2 ttl=64 time=1.97 ms
>> 64 bytes from 192.168.1.14: icmp_seq=3 ttl=64 time=17.2 ms
>> 64 bytes from 192.168.1.14: icmp_seq=4 ttl=64 time=2.46 ms
>> 64 bytes from 192.168.1.14: icmp_seq=5 ttl=64 time=2.83 ms
>> 64 bytes from 192.168.1.14: icmp_seq=6 ttl=64 time=1.40 ms
>> 64 bytes from 192.168.1.14: icmp_seq=7 ttl=64 time=2.10 ms
>> 64 bytes from 192.168.1.14: icmp_seq=8 ttl=64 time=1.40 ms
>> 64 bytes from 192.168.1.14: icmp_seq=9 ttl=64 time=2.04 ms
>> 64 bytes from 192.168.1.14: icmp_seq=10 ttl=64 time=1.40 ms
>>
>> Cc: Dong Aisheng <b29396@freescale.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> -New patch replacing "mmc: sdhci: sdio-intel: Set
>> SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON quirk"
>> ---
>> drivers/mmc/host/sdhci-acpi.c | 1 +
>> drivers/mmc/host/sdhci-pci-core.c | 1 +
>> drivers/mmc/host/sdhci.c | 6 ++++++
>> drivers/mmc/host/sdhci.h | 2 ++
>> 4 files changed, 10 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
>> index 237f318..53b431a 100644
>> --- a/drivers/mmc/host/sdhci-acpi.c
>> +++ b/drivers/mmc/host/sdhci-acpi.c
>> @@ -288,6 +288,7 @@ static const struct sdhci_acpi_slot sdhci_acpi_slot_int_emmc = {
>> static const struct sdhci_acpi_slot sdhci_acpi_slot_int_sdio = {
>> .quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
>> .quirks2 = SDHCI_QUIRK2_HOST_OFF_CARD_ON |
>> + SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM |
>> SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
>> .caps = MMC_CAP_NONREMOVABLE | MMC_CAP_POWER_OFF_CARD |
>> MMC_CAP_WAIT_WHILE_BUSY,
>> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
>> index 982b3e3..a3b242e 100644
>> --- a/drivers/mmc/host/sdhci-pci-core.c
>> +++ b/drivers/mmc/host/sdhci-pci-core.c
>> @@ -498,6 +498,7 @@ static const struct sdhci_pci_fixes sdhci_ni_byt_sdio = {
>> static const struct sdhci_pci_fixes sdhci_intel_byt_sdio = {
>> .quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
>> .quirks2 = SDHCI_QUIRK2_HOST_OFF_CARD_ON |
>> + SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM |
>> SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
>> .allow_runtime_pm = true,
>> .probe_slot = byt_sdio_probe_slot,
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 6fdd7a7..59c5cad 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -1828,6 +1828,9 @@ static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
>> struct sdhci_host *host = mmc_priv(mmc);
>> unsigned long flags;
>>
>> + if ((host->quirks2 & SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM) && enable)
>> + pm_runtime_get(host->mmc->parent);
>
> There are currently no sdhci variant for any SoC that implements
> wakeup support in runtime suspend for SDIO irqs. In other words, it
> seems all sdhci hosts suffers from the same problem.
There might still be drivers that expect SDIO interrupts during
runtime suspend, considering this patch:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=be138554a7923658ded799b0e8794d9c1d08a6e5
>
> I imagine there are vendor trees implementing wakeups, however until
> we see such code being pushed upstream, I suggest we make this the
> default behavior instead of using a quirk.
>
>> +
>> spin_lock_irqsave(&host->lock, flags);
>> if (enable)
>> host->flags |= SDHCI_SDIO_IRQ_ENABLED;
>> @@ -1836,6 +1839,9 @@ static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
>>
>> sdhci_enable_sdio_irq_nolock(host, enable);
>> spin_unlock_irqrestore(&host->lock, flags);
>> +
>> + if ((host->quirks2 & SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM) && !enable)
>> + pm_runtime_put(host->mmc->parent);
>> }
>>
>> static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>> index edf3adf..b72c38d 100644
>> --- a/drivers/mmc/host/sdhci.h
>> +++ b/drivers/mmc/host/sdhci.h
>> @@ -427,6 +427,8 @@ struct sdhci_host {
>> #define SDHCI_QUIRK2_ACMD23_BROKEN (1<<14)
>> /* Broken Clock divider zero in controller */
>> #define SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN (1<<15)
>> +/* Must not runtime suspend when sdio_irq is enabled */
>> +#define SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM (1<<16)
>>
>> int irq; /* Device IRQ */
>> void __iomem *ioaddr; /* Mapped address */
>> --
>> 2.9.3
>>
>
> Kind regards
> Uffe
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] mmc: sdhci: intel: Disable runtime pm when the sdio_irq is enabled.
2017-03-21 12:34 ` Adrian Hunter
@ 2017-03-21 13:30 ` Hans de Goede
2017-03-21 14:54 ` Ulf Hansson
1 sibling, 0 replies; 10+ messages in thread
From: Hans de Goede @ 2017-03-21 13:30 UTC (permalink / raw)
To: Adrian Hunter, Ulf Hansson; +Cc: linux-mmc, Dong Aisheng
Hu,
On 21-03-17 13:34, Adrian Hunter wrote:
> On 21/03/17 12:00, Ulf Hansson wrote:
>> On 15 March 2017 at 17:31, Hans de Goede <hdegoede@redhat.com> wrote:
>>> SDIO cards may need clock to send the card interrupt to the host.
>>>
>>> On a cherrytrail tablet with a RTL8723BS wifi chip, without this patch
>>> pinging the tablet results in:
>>>
>>> PING 192.168.1.14 (192.168.1.14) 56(84) bytes of data.
>>> 64 bytes from 192.168.1.14: icmp_seq=1 ttl=64 time=78.6 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=2 ttl=64 time=1760 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=3 ttl=64 time=753 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=4 ttl=64 time=3.88 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=5 ttl=64 time=795 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=6 ttl=64 time=1841 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=7 ttl=64 time=810 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=8 ttl=64 time=1860 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=9 ttl=64 time=812 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=10 ttl=64 time=48.6 ms
>>>
>>> Where as with this patch I get:
>>>
>>> PING 192.168.1.14 (192.168.1.14) 56(84) bytes of data.
>>> 64 bytes from 192.168.1.14: icmp_seq=1 ttl=64 time=3.96 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=2 ttl=64 time=1.97 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=3 ttl=64 time=17.2 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=4 ttl=64 time=2.46 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=5 ttl=64 time=2.83 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=6 ttl=64 time=1.40 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=7 ttl=64 time=2.10 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=8 ttl=64 time=1.40 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=9 ttl=64 time=2.04 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=10 ttl=64 time=1.40 ms
>>>
>>> Cc: Dong Aisheng <b29396@freescale.com>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> Changes in v2:
>>> -New patch replacing "mmc: sdhci: sdio-intel: Set
>>> SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON quirk"
>>> ---
>>> drivers/mmc/host/sdhci-acpi.c | 1 +
>>> drivers/mmc/host/sdhci-pci-core.c | 1 +
>>> drivers/mmc/host/sdhci.c | 6 ++++++
>>> drivers/mmc/host/sdhci.h | 2 ++
>>> 4 files changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
>>> index 237f318..53b431a 100644
>>> --- a/drivers/mmc/host/sdhci-acpi.c
>>> +++ b/drivers/mmc/host/sdhci-acpi.c
>>> @@ -288,6 +288,7 @@ static const struct sdhci_acpi_slot sdhci_acpi_slot_int_emmc = {
>>> static const struct sdhci_acpi_slot sdhci_acpi_slot_int_sdio = {
>>> .quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
>>> .quirks2 = SDHCI_QUIRK2_HOST_OFF_CARD_ON |
>>> + SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM |
>>> SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
>>> .caps = MMC_CAP_NONREMOVABLE | MMC_CAP_POWER_OFF_CARD |
>>> MMC_CAP_WAIT_WHILE_BUSY,
>>> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
>>> index 982b3e3..a3b242e 100644
>>> --- a/drivers/mmc/host/sdhci-pci-core.c
>>> +++ b/drivers/mmc/host/sdhci-pci-core.c
>>> @@ -498,6 +498,7 @@ static const struct sdhci_pci_fixes sdhci_ni_byt_sdio = {
>>> static const struct sdhci_pci_fixes sdhci_intel_byt_sdio = {
>>> .quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
>>> .quirks2 = SDHCI_QUIRK2_HOST_OFF_CARD_ON |
>>> + SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM |
>>> SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
>>> .allow_runtime_pm = true,
>>> .probe_slot = byt_sdio_probe_slot,
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 6fdd7a7..59c5cad 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -1828,6 +1828,9 @@ static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
>>> struct sdhci_host *host = mmc_priv(mmc);
>>> unsigned long flags;
>>>
>>> + if ((host->quirks2 & SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM) && enable)
>>> + pm_runtime_get(host->mmc->parent);
>>
>> There are currently no sdhci variant for any SoC that implements
>> wakeup support in runtime suspend for SDIO irqs. In other words, it
>> seems all sdhci hosts suffers from the same problem.
>
> There might still be drivers that expect SDIO interrupts during
> runtime suspend, considering this patch:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=be138554a7923658ded799b0e8794d9c1d08a6e5
Right, I do agree with Ulf that disabling runtime_pm while an
sdio-irq is requested should probably be the default, so maybe
revert the quirk and make it allow runtime pm if set, e.g. :
if (enable && !(host->quirks2 & SDHCI_QUIRK2_SDIO_IRQ_RUNTIME_PM))
pm_runtime_get(host->mmc->parent)
And then add the quirk to drivers/mmc/host/sdhci-esdhc-imx.c ?
Regards,
Hans
>
>>
>> I imagine there are vendor trees implementing wakeups, however until
>> we see such code being pushed upstream, I suggest we make this the
>> default behavior instead of using a quirk.
>>
>>> +
>>> spin_lock_irqsave(&host->lock, flags);
>>> if (enable)
>>> host->flags |= SDHCI_SDIO_IRQ_ENABLED;
>>> @@ -1836,6 +1839,9 @@ static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
>>>
>>> sdhci_enable_sdio_irq_nolock(host, enable);
>>> spin_unlock_irqrestore(&host->lock, flags);
>>> +
>>> + if ((host->quirks2 & SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM) && !enable)
>>> + pm_runtime_put(host->mmc->parent);
>>> }
>>>
>>> static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
>>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>>> index edf3adf..b72c38d 100644
>>> --- a/drivers/mmc/host/sdhci.h
>>> +++ b/drivers/mmc/host/sdhci.h
>>> @@ -427,6 +427,8 @@ struct sdhci_host {
>>> #define SDHCI_QUIRK2_ACMD23_BROKEN (1<<14)
>>> /* Broken Clock divider zero in controller */
>>> #define SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN (1<<15)
>>> +/* Must not runtime suspend when sdio_irq is enabled */
>>> +#define SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM (1<<16)
>>>
>>> int irq; /* Device IRQ */
>>> void __iomem *ioaddr; /* Mapped address */
>>> --
>>> 2.9.3
>>>
>>
>> Kind regards
>> Uffe
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] mmc: sdhci: intel: Disable runtime pm when the sdio_irq is enabled.
2017-03-21 12:34 ` Adrian Hunter
2017-03-21 13:30 ` Hans de Goede
@ 2017-03-21 14:54 ` Ulf Hansson
2017-03-22 12:06 ` Adrian Hunter
1 sibling, 1 reply; 10+ messages in thread
From: Ulf Hansson @ 2017-03-21 14:54 UTC (permalink / raw)
To: Adrian Hunter; +Cc: Hans de Goede, linux-mmc, Dong Aisheng
On 21 March 2017 at 13:34, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 21/03/17 12:00, Ulf Hansson wrote:
>> On 15 March 2017 at 17:31, Hans de Goede <hdegoede@redhat.com> wrote:
>>> SDIO cards may need clock to send the card interrupt to the host.
>>>
>>> On a cherrytrail tablet with a RTL8723BS wifi chip, without this patch
>>> pinging the tablet results in:
>>>
>>> PING 192.168.1.14 (192.168.1.14) 56(84) bytes of data.
>>> 64 bytes from 192.168.1.14: icmp_seq=1 ttl=64 time=78.6 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=2 ttl=64 time=1760 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=3 ttl=64 time=753 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=4 ttl=64 time=3.88 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=5 ttl=64 time=795 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=6 ttl=64 time=1841 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=7 ttl=64 time=810 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=8 ttl=64 time=1860 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=9 ttl=64 time=812 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=10 ttl=64 time=48.6 ms
>>>
>>> Where as with this patch I get:
>>>
>>> PING 192.168.1.14 (192.168.1.14) 56(84) bytes of data.
>>> 64 bytes from 192.168.1.14: icmp_seq=1 ttl=64 time=3.96 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=2 ttl=64 time=1.97 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=3 ttl=64 time=17.2 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=4 ttl=64 time=2.46 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=5 ttl=64 time=2.83 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=6 ttl=64 time=1.40 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=7 ttl=64 time=2.10 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=8 ttl=64 time=1.40 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=9 ttl=64 time=2.04 ms
>>> 64 bytes from 192.168.1.14: icmp_seq=10 ttl=64 time=1.40 ms
>>>
>>> Cc: Dong Aisheng <b29396@freescale.com>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> Changes in v2:
>>> -New patch replacing "mmc: sdhci: sdio-intel: Set
>>> SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON quirk"
>>> ---
>>> drivers/mmc/host/sdhci-acpi.c | 1 +
>>> drivers/mmc/host/sdhci-pci-core.c | 1 +
>>> drivers/mmc/host/sdhci.c | 6 ++++++
>>> drivers/mmc/host/sdhci.h | 2 ++
>>> 4 files changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
>>> index 237f318..53b431a 100644
>>> --- a/drivers/mmc/host/sdhci-acpi.c
>>> +++ b/drivers/mmc/host/sdhci-acpi.c
>>> @@ -288,6 +288,7 @@ static const struct sdhci_acpi_slot sdhci_acpi_slot_int_emmc = {
>>> static const struct sdhci_acpi_slot sdhci_acpi_slot_int_sdio = {
>>> .quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
>>> .quirks2 = SDHCI_QUIRK2_HOST_OFF_CARD_ON |
>>> + SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM |
>>> SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
>>> .caps = MMC_CAP_NONREMOVABLE | MMC_CAP_POWER_OFF_CARD |
>>> MMC_CAP_WAIT_WHILE_BUSY,
>>> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
>>> index 982b3e3..a3b242e 100644
>>> --- a/drivers/mmc/host/sdhci-pci-core.c
>>> +++ b/drivers/mmc/host/sdhci-pci-core.c
>>> @@ -498,6 +498,7 @@ static const struct sdhci_pci_fixes sdhci_ni_byt_sdio = {
>>> static const struct sdhci_pci_fixes sdhci_intel_byt_sdio = {
>>> .quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
>>> .quirks2 = SDHCI_QUIRK2_HOST_OFF_CARD_ON |
>>> + SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM |
>>> SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
>>> .allow_runtime_pm = true,
>>> .probe_slot = byt_sdio_probe_slot,
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 6fdd7a7..59c5cad 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -1828,6 +1828,9 @@ static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
>>> struct sdhci_host *host = mmc_priv(mmc);
>>> unsigned long flags;
>>>
>>> + if ((host->quirks2 & SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM) && enable)
>>> + pm_runtime_get(host->mmc->parent);
>>
>> There are currently no sdhci variant for any SoC that implements
>> wakeup support in runtime suspend for SDIO irqs. In other words, it
>> seems all sdhci hosts suffers from the same problem.
>
> There might still be drivers that expect SDIO interrupts during
> runtime suspend, considering this patch:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=be138554a7923658ded799b0e8794d9c1d08a6e5
Right. Thanks for pointing it out.
However I don't think that is how runtime PM should be deployed when
it is used for controlling devices like sdhci. Let's change that once
we have the common issue for sdhci and SDIO irqs resolved.
[...]
Kind regards
Uffe
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] mmc: sdhci: intel: Disable runtime pm when the sdio_irq is enabled.
2017-03-21 14:54 ` Ulf Hansson
@ 2017-03-22 12:06 ` Adrian Hunter
2017-03-22 14:00 ` Ulf Hansson
0 siblings, 1 reply; 10+ messages in thread
From: Adrian Hunter @ 2017-03-22 12:06 UTC (permalink / raw)
To: Ulf Hansson; +Cc: Hans de Goede, linux-mmc, Dong Aisheng
On 21/03/17 16:54, Ulf Hansson wrote:
> On 21 March 2017 at 13:34, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 21/03/17 12:00, Ulf Hansson wrote:
>>> On 15 March 2017 at 17:31, Hans de Goede <hdegoede@redhat.com> wrote:
>>>> SDIO cards may need clock to send the card interrupt to the host.
>>>>
>>>> On a cherrytrail tablet with a RTL8723BS wifi chip, without this patch
>>>> pinging the tablet results in:
>>>>
>>>> PING 192.168.1.14 (192.168.1.14) 56(84) bytes of data.
>>>> 64 bytes from 192.168.1.14: icmp_seq=1 ttl=64 time=78.6 ms
>>>> 64 bytes from 192.168.1.14: icmp_seq=2 ttl=64 time=1760 ms
>>>> 64 bytes from 192.168.1.14: icmp_seq=3 ttl=64 time=753 ms
>>>> 64 bytes from 192.168.1.14: icmp_seq=4 ttl=64 time=3.88 ms
>>>> 64 bytes from 192.168.1.14: icmp_seq=5 ttl=64 time=795 ms
>>>> 64 bytes from 192.168.1.14: icmp_seq=6 ttl=64 time=1841 ms
>>>> 64 bytes from 192.168.1.14: icmp_seq=7 ttl=64 time=810 ms
>>>> 64 bytes from 192.168.1.14: icmp_seq=8 ttl=64 time=1860 ms
>>>> 64 bytes from 192.168.1.14: icmp_seq=9 ttl=64 time=812 ms
>>>> 64 bytes from 192.168.1.14: icmp_seq=10 ttl=64 time=48.6 ms
>>>>
>>>> Where as with this patch I get:
>>>>
>>>> PING 192.168.1.14 (192.168.1.14) 56(84) bytes of data.
>>>> 64 bytes from 192.168.1.14: icmp_seq=1 ttl=64 time=3.96 ms
>>>> 64 bytes from 192.168.1.14: icmp_seq=2 ttl=64 time=1.97 ms
>>>> 64 bytes from 192.168.1.14: icmp_seq=3 ttl=64 time=17.2 ms
>>>> 64 bytes from 192.168.1.14: icmp_seq=4 ttl=64 time=2.46 ms
>>>> 64 bytes from 192.168.1.14: icmp_seq=5 ttl=64 time=2.83 ms
>>>> 64 bytes from 192.168.1.14: icmp_seq=6 ttl=64 time=1.40 ms
>>>> 64 bytes from 192.168.1.14: icmp_seq=7 ttl=64 time=2.10 ms
>>>> 64 bytes from 192.168.1.14: icmp_seq=8 ttl=64 time=1.40 ms
>>>> 64 bytes from 192.168.1.14: icmp_seq=9 ttl=64 time=2.04 ms
>>>> 64 bytes from 192.168.1.14: icmp_seq=10 ttl=64 time=1.40 ms
>>>>
>>>> Cc: Dong Aisheng <b29396@freescale.com>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> Changes in v2:
>>>> -New patch replacing "mmc: sdhci: sdio-intel: Set
>>>> SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON quirk"
>>>> ---
>>>> drivers/mmc/host/sdhci-acpi.c | 1 +
>>>> drivers/mmc/host/sdhci-pci-core.c | 1 +
>>>> drivers/mmc/host/sdhci.c | 6 ++++++
>>>> drivers/mmc/host/sdhci.h | 2 ++
>>>> 4 files changed, 10 insertions(+)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
>>>> index 237f318..53b431a 100644
>>>> --- a/drivers/mmc/host/sdhci-acpi.c
>>>> +++ b/drivers/mmc/host/sdhci-acpi.c
>>>> @@ -288,6 +288,7 @@ static const struct sdhci_acpi_slot sdhci_acpi_slot_int_emmc = {
>>>> static const struct sdhci_acpi_slot sdhci_acpi_slot_int_sdio = {
>>>> .quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
>>>> .quirks2 = SDHCI_QUIRK2_HOST_OFF_CARD_ON |
>>>> + SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM |
>>>> SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
>>>> .caps = MMC_CAP_NONREMOVABLE | MMC_CAP_POWER_OFF_CARD |
>>>> MMC_CAP_WAIT_WHILE_BUSY,
>>>> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
>>>> index 982b3e3..a3b242e 100644
>>>> --- a/drivers/mmc/host/sdhci-pci-core.c
>>>> +++ b/drivers/mmc/host/sdhci-pci-core.c
>>>> @@ -498,6 +498,7 @@ static const struct sdhci_pci_fixes sdhci_ni_byt_sdio = {
>>>> static const struct sdhci_pci_fixes sdhci_intel_byt_sdio = {
>>>> .quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
>>>> .quirks2 = SDHCI_QUIRK2_HOST_OFF_CARD_ON |
>>>> + SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM |
>>>> SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
>>>> .allow_runtime_pm = true,
>>>> .probe_slot = byt_sdio_probe_slot,
>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>>> index 6fdd7a7..59c5cad 100644
>>>> --- a/drivers/mmc/host/sdhci.c
>>>> +++ b/drivers/mmc/host/sdhci.c
>>>> @@ -1828,6 +1828,9 @@ static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
>>>> struct sdhci_host *host = mmc_priv(mmc);
>>>> unsigned long flags;
>>>>
>>>> + if ((host->quirks2 & SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM) && enable)
>>>> + pm_runtime_get(host->mmc->parent);
>>>
>>> There are currently no sdhci variant for any SoC that implements
>>> wakeup support in runtime suspend for SDIO irqs. In other words, it
>>> seems all sdhci hosts suffers from the same problem.
>>
>> There might still be drivers that expect SDIO interrupts during
>> runtime suspend, considering this patch:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=be138554a7923658ded799b0e8794d9c1d08a6e5
>
> Right. Thanks for pointing it out.
>
> However I don't think that is how runtime PM should be deployed when
> it is used for controlling devices like sdhci. Let's change that once
> we have the common issue for sdhci and SDIO irqs resolved.
Yes, then perhaps we could accept the inverted quirk on the basis that it
will be removed.
Hans, as I wrote somewhere else, we can use pm_runtime_get_noresume() and
pm_runtime_put_noidle() since sdhci_enable_sdio_irq() must be called with
the host claimed and therefore runtime resumed.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] mmc: sdhci: intel: Disable runtime pm when the sdio_irq is enabled.
2017-03-22 12:06 ` Adrian Hunter
@ 2017-03-22 14:00 ` Ulf Hansson
2017-03-22 14:29 ` Adrian Hunter
0 siblings, 1 reply; 10+ messages in thread
From: Ulf Hansson @ 2017-03-22 14:00 UTC (permalink / raw)
To: Adrian Hunter; +Cc: Hans de Goede, linux-mmc, Dong Aisheng
On 22 March 2017 at 13:06, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 21/03/17 16:54, Ulf Hansson wrote:
>> On 21 March 2017 at 13:34, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> On 21/03/17 12:00, Ulf Hansson wrote:
>>>> On 15 March 2017 at 17:31, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>> SDIO cards may need clock to send the card interrupt to the host.
>>>>>
>>>>> On a cherrytrail tablet with a RTL8723BS wifi chip, without this patch
>>>>> pinging the tablet results in:
>>>>>
>>>>> PING 192.168.1.14 (192.168.1.14) 56(84) bytes of data.
>>>>> 64 bytes from 192.168.1.14: icmp_seq=1 ttl=64 time=78.6 ms
>>>>> 64 bytes from 192.168.1.14: icmp_seq=2 ttl=64 time=1760 ms
>>>>> 64 bytes from 192.168.1.14: icmp_seq=3 ttl=64 time=753 ms
>>>>> 64 bytes from 192.168.1.14: icmp_seq=4 ttl=64 time=3.88 ms
>>>>> 64 bytes from 192.168.1.14: icmp_seq=5 ttl=64 time=795 ms
>>>>> 64 bytes from 192.168.1.14: icmp_seq=6 ttl=64 time=1841 ms
>>>>> 64 bytes from 192.168.1.14: icmp_seq=7 ttl=64 time=810 ms
>>>>> 64 bytes from 192.168.1.14: icmp_seq=8 ttl=64 time=1860 ms
>>>>> 64 bytes from 192.168.1.14: icmp_seq=9 ttl=64 time=812 ms
>>>>> 64 bytes from 192.168.1.14: icmp_seq=10 ttl=64 time=48.6 ms
>>>>>
>>>>> Where as with this patch I get:
>>>>>
>>>>> PING 192.168.1.14 (192.168.1.14) 56(84) bytes of data.
>>>>> 64 bytes from 192.168.1.14: icmp_seq=1 ttl=64 time=3.96 ms
>>>>> 64 bytes from 192.168.1.14: icmp_seq=2 ttl=64 time=1.97 ms
>>>>> 64 bytes from 192.168.1.14: icmp_seq=3 ttl=64 time=17.2 ms
>>>>> 64 bytes from 192.168.1.14: icmp_seq=4 ttl=64 time=2.46 ms
>>>>> 64 bytes from 192.168.1.14: icmp_seq=5 ttl=64 time=2.83 ms
>>>>> 64 bytes from 192.168.1.14: icmp_seq=6 ttl=64 time=1.40 ms
>>>>> 64 bytes from 192.168.1.14: icmp_seq=7 ttl=64 time=2.10 ms
>>>>> 64 bytes from 192.168.1.14: icmp_seq=8 ttl=64 time=1.40 ms
>>>>> 64 bytes from 192.168.1.14: icmp_seq=9 ttl=64 time=2.04 ms
>>>>> 64 bytes from 192.168.1.14: icmp_seq=10 ttl=64 time=1.40 ms
>>>>>
>>>>> Cc: Dong Aisheng <b29396@freescale.com>
>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>> ---
>>>>> Changes in v2:
>>>>> -New patch replacing "mmc: sdhci: sdio-intel: Set
>>>>> SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON quirk"
>>>>> ---
>>>>> drivers/mmc/host/sdhci-acpi.c | 1 +
>>>>> drivers/mmc/host/sdhci-pci-core.c | 1 +
>>>>> drivers/mmc/host/sdhci.c | 6 ++++++
>>>>> drivers/mmc/host/sdhci.h | 2 ++
>>>>> 4 files changed, 10 insertions(+)
>>>>>
>>>>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
>>>>> index 237f318..53b431a 100644
>>>>> --- a/drivers/mmc/host/sdhci-acpi.c
>>>>> +++ b/drivers/mmc/host/sdhci-acpi.c
>>>>> @@ -288,6 +288,7 @@ static const struct sdhci_acpi_slot sdhci_acpi_slot_int_emmc = {
>>>>> static const struct sdhci_acpi_slot sdhci_acpi_slot_int_sdio = {
>>>>> .quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
>>>>> .quirks2 = SDHCI_QUIRK2_HOST_OFF_CARD_ON |
>>>>> + SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM |
>>>>> SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
>>>>> .caps = MMC_CAP_NONREMOVABLE | MMC_CAP_POWER_OFF_CARD |
>>>>> MMC_CAP_WAIT_WHILE_BUSY,
>>>>> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
>>>>> index 982b3e3..a3b242e 100644
>>>>> --- a/drivers/mmc/host/sdhci-pci-core.c
>>>>> +++ b/drivers/mmc/host/sdhci-pci-core.c
>>>>> @@ -498,6 +498,7 @@ static const struct sdhci_pci_fixes sdhci_ni_byt_sdio = {
>>>>> static const struct sdhci_pci_fixes sdhci_intel_byt_sdio = {
>>>>> .quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
>>>>> .quirks2 = SDHCI_QUIRK2_HOST_OFF_CARD_ON |
>>>>> + SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM |
>>>>> SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
>>>>> .allow_runtime_pm = true,
>>>>> .probe_slot = byt_sdio_probe_slot,
>>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>>>> index 6fdd7a7..59c5cad 100644
>>>>> --- a/drivers/mmc/host/sdhci.c
>>>>> +++ b/drivers/mmc/host/sdhci.c
>>>>> @@ -1828,6 +1828,9 @@ static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
>>>>> struct sdhci_host *host = mmc_priv(mmc);
>>>>> unsigned long flags;
>>>>>
>>>>> + if ((host->quirks2 & SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM) && enable)
>>>>> + pm_runtime_get(host->mmc->parent);
>>>>
>>>> There are currently no sdhci variant for any SoC that implements
>>>> wakeup support in runtime suspend for SDIO irqs. In other words, it
>>>> seems all sdhci hosts suffers from the same problem.
>>>
>>> There might still be drivers that expect SDIO interrupts during
>>> runtime suspend, considering this patch:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=be138554a7923658ded799b0e8794d9c1d08a6e5
>>
>> Right. Thanks for pointing it out.
>>
>> However I don't think that is how runtime PM should be deployed when
>> it is used for controlling devices like sdhci. Let's change that once
>> we have the common issue for sdhci and SDIO irqs resolved.
>
> Yes, then perhaps we could accept the inverted quirk on the basis that it
> will be removed.
Well, why have quirk for something that will be always executed. Isn't
it better to invent one, when we see a need for it?
>
> Hans, as I wrote somewhere else, we can use pm_runtime_get_noresume() and
> pm_runtime_put_noidle() since sdhci_enable_sdio_irq() must be called with
> the host claimed and therefore runtime resumed.
>
Good point. This makes it more clear on what goes on.
Kind regards
Uffe
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] mmc: sdhci: intel: Disable runtime pm when the sdio_irq is enabled.
2017-03-22 14:00 ` Ulf Hansson
@ 2017-03-22 14:29 ` Adrian Hunter
2017-03-30 6:13 ` Dong Aisheng
0 siblings, 1 reply; 10+ messages in thread
From: Adrian Hunter @ 2017-03-22 14:29 UTC (permalink / raw)
To: Ulf Hansson; +Cc: Hans de Goede, linux-mmc, Dong Aisheng
On 22/03/17 16:00, Ulf Hansson wrote:
> On 22 March 2017 at 13:06, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 21/03/17 16:54, Ulf Hansson wrote:
>>> On 21 March 2017 at 13:34, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> On 21/03/17 12:00, Ulf Hansson wrote:
>>>>> On 15 March 2017 at 17:31, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>> SDIO cards may need clock to send the card interrupt to the host.
>>>>>>
>>>>>> On a cherrytrail tablet with a RTL8723BS wifi chip, without this patch
>>>>>> pinging the tablet results in:
>>>>>>
>>>>>> PING 192.168.1.14 (192.168.1.14) 56(84) bytes of data.
>>>>>> 64 bytes from 192.168.1.14: icmp_seq=1 ttl=64 time=78.6 ms
>>>>>> 64 bytes from 192.168.1.14: icmp_seq=2 ttl=64 time=1760 ms
>>>>>> 64 bytes from 192.168.1.14: icmp_seq=3 ttl=64 time=753 ms
>>>>>> 64 bytes from 192.168.1.14: icmp_seq=4 ttl=64 time=3.88 ms
>>>>>> 64 bytes from 192.168.1.14: icmp_seq=5 ttl=64 time=795 ms
>>>>>> 64 bytes from 192.168.1.14: icmp_seq=6 ttl=64 time=1841 ms
>>>>>> 64 bytes from 192.168.1.14: icmp_seq=7 ttl=64 time=810 ms
>>>>>> 64 bytes from 192.168.1.14: icmp_seq=8 ttl=64 time=1860 ms
>>>>>> 64 bytes from 192.168.1.14: icmp_seq=9 ttl=64 time=812 ms
>>>>>> 64 bytes from 192.168.1.14: icmp_seq=10 ttl=64 time=48.6 ms
>>>>>>
>>>>>> Where as with this patch I get:
>>>>>>
>>>>>> PING 192.168.1.14 (192.168.1.14) 56(84) bytes of data.
>>>>>> 64 bytes from 192.168.1.14: icmp_seq=1 ttl=64 time=3.96 ms
>>>>>> 64 bytes from 192.168.1.14: icmp_seq=2 ttl=64 time=1.97 ms
>>>>>> 64 bytes from 192.168.1.14: icmp_seq=3 ttl=64 time=17.2 ms
>>>>>> 64 bytes from 192.168.1.14: icmp_seq=4 ttl=64 time=2.46 ms
>>>>>> 64 bytes from 192.168.1.14: icmp_seq=5 ttl=64 time=2.83 ms
>>>>>> 64 bytes from 192.168.1.14: icmp_seq=6 ttl=64 time=1.40 ms
>>>>>> 64 bytes from 192.168.1.14: icmp_seq=7 ttl=64 time=2.10 ms
>>>>>> 64 bytes from 192.168.1.14: icmp_seq=8 ttl=64 time=1.40 ms
>>>>>> 64 bytes from 192.168.1.14: icmp_seq=9 ttl=64 time=2.04 ms
>>>>>> 64 bytes from 192.168.1.14: icmp_seq=10 ttl=64 time=1.40 ms
>>>>>>
>>>>>> Cc: Dong Aisheng <b29396@freescale.com>
>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>> ---
>>>>>> Changes in v2:
>>>>>> -New patch replacing "mmc: sdhci: sdio-intel: Set
>>>>>> SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON quirk"
>>>>>> ---
>>>>>> drivers/mmc/host/sdhci-acpi.c | 1 +
>>>>>> drivers/mmc/host/sdhci-pci-core.c | 1 +
>>>>>> drivers/mmc/host/sdhci.c | 6 ++++++
>>>>>> drivers/mmc/host/sdhci.h | 2 ++
>>>>>> 4 files changed, 10 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
>>>>>> index 237f318..53b431a 100644
>>>>>> --- a/drivers/mmc/host/sdhci-acpi.c
>>>>>> +++ b/drivers/mmc/host/sdhci-acpi.c
>>>>>> @@ -288,6 +288,7 @@ static const struct sdhci_acpi_slot sdhci_acpi_slot_int_emmc = {
>>>>>> static const struct sdhci_acpi_slot sdhci_acpi_slot_int_sdio = {
>>>>>> .quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
>>>>>> .quirks2 = SDHCI_QUIRK2_HOST_OFF_CARD_ON |
>>>>>> + SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM |
>>>>>> SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
>>>>>> .caps = MMC_CAP_NONREMOVABLE | MMC_CAP_POWER_OFF_CARD |
>>>>>> MMC_CAP_WAIT_WHILE_BUSY,
>>>>>> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
>>>>>> index 982b3e3..a3b242e 100644
>>>>>> --- a/drivers/mmc/host/sdhci-pci-core.c
>>>>>> +++ b/drivers/mmc/host/sdhci-pci-core.c
>>>>>> @@ -498,6 +498,7 @@ static const struct sdhci_pci_fixes sdhci_ni_byt_sdio = {
>>>>>> static const struct sdhci_pci_fixes sdhci_intel_byt_sdio = {
>>>>>> .quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
>>>>>> .quirks2 = SDHCI_QUIRK2_HOST_OFF_CARD_ON |
>>>>>> + SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM |
>>>>>> SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
>>>>>> .allow_runtime_pm = true,
>>>>>> .probe_slot = byt_sdio_probe_slot,
>>>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>>>>> index 6fdd7a7..59c5cad 100644
>>>>>> --- a/drivers/mmc/host/sdhci.c
>>>>>> +++ b/drivers/mmc/host/sdhci.c
>>>>>> @@ -1828,6 +1828,9 @@ static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
>>>>>> struct sdhci_host *host = mmc_priv(mmc);
>>>>>> unsigned long flags;
>>>>>>
>>>>>> + if ((host->quirks2 & SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM) && enable)
>>>>>> + pm_runtime_get(host->mmc->parent);
>>>>>
>>>>> There are currently no sdhci variant for any SoC that implements
>>>>> wakeup support in runtime suspend for SDIO irqs. In other words, it
>>>>> seems all sdhci hosts suffers from the same problem.
>>>>
>>>> There might still be drivers that expect SDIO interrupts during
>>>> runtime suspend, considering this patch:
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=be138554a7923658ded799b0e8794d9c1d08a6e5
>>>
>>> Right. Thanks for pointing it out.
>>>
>>> However I don't think that is how runtime PM should be deployed when
>>> it is used for controlling devices like sdhci. Let's change that once
>>> we have the common issue for sdhci and SDIO irqs resolved.
>>
>> Yes, then perhaps we could accept the inverted quirk on the basis that it
>> will be removed.
>
> Well, why have quirk for something that will be always executed. Isn't
> it better to invent one, when we see a need for it?
I think the idea was to add the quirk to sdhci-esdhc-imx but I doubt
sdhci-esdhc-imx is really saving much power with the function and interface
clocks still on, so we can dispense with a quirk for now if that is what you
prefer.
>
>>
>> Hans, as I wrote somewhere else, we can use pm_runtime_get_noresume() and
>> pm_runtime_put_noidle() since sdhci_enable_sdio_irq() must be called with
>> the host claimed and therefore runtime resumed.
>>
>
> Good point. This makes it more clear on what goes on.
>
> Kind regards
> Uffe
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] mmc: sdhci: intel: Disable runtime pm when the sdio_irq is enabled.
2017-03-22 14:29 ` Adrian Hunter
@ 2017-03-30 6:13 ` Dong Aisheng
0 siblings, 0 replies; 10+ messages in thread
From: Dong Aisheng @ 2017-03-30 6:13 UTC (permalink / raw)
To: Adrian Hunter; +Cc: Ulf Hansson, Hans de Goede, linux-mmc, Dong Aisheng
On Wed, Mar 22, 2017 at 04:29:07PM +0200, Adrian Hunter wrote:
> On 22/03/17 16:00, Ulf Hansson wrote:
> > On 22 March 2017 at 13:06, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >> On 21/03/17 16:54, Ulf Hansson wrote:
> >>> On 21 March 2017 at 13:34, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>>> On 21/03/17 12:00, Ulf Hansson wrote:
> >>>>> On 15 March 2017 at 17:31, Hans de Goede <hdegoede@redhat.com> wrote:
> >>>>>> SDIO cards may need clock to send the card interrupt to the host.
> >>>>>>
> >>>>>> On a cherrytrail tablet with a RTL8723BS wifi chip, without this patch
> >>>>>> pinging the tablet results in:
> >>>>>>
> >>>>>> PING 192.168.1.14 (192.168.1.14) 56(84) bytes of data.
> >>>>>> 64 bytes from 192.168.1.14: icmp_seq=1 ttl=64 time=78.6 ms
> >>>>>> 64 bytes from 192.168.1.14: icmp_seq=2 ttl=64 time=1760 ms
> >>>>>> 64 bytes from 192.168.1.14: icmp_seq=3 ttl=64 time=753 ms
> >>>>>> 64 bytes from 192.168.1.14: icmp_seq=4 ttl=64 time=3.88 ms
> >>>>>> 64 bytes from 192.168.1.14: icmp_seq=5 ttl=64 time=795 ms
> >>>>>> 64 bytes from 192.168.1.14: icmp_seq=6 ttl=64 time=1841 ms
> >>>>>> 64 bytes from 192.168.1.14: icmp_seq=7 ttl=64 time=810 ms
> >>>>>> 64 bytes from 192.168.1.14: icmp_seq=8 ttl=64 time=1860 ms
> >>>>>> 64 bytes from 192.168.1.14: icmp_seq=9 ttl=64 time=812 ms
> >>>>>> 64 bytes from 192.168.1.14: icmp_seq=10 ttl=64 time=48.6 ms
> >>>>>>
> >>>>>> Where as with this patch I get:
> >>>>>>
> >>>>>> PING 192.168.1.14 (192.168.1.14) 56(84) bytes of data.
> >>>>>> 64 bytes from 192.168.1.14: icmp_seq=1 ttl=64 time=3.96 ms
> >>>>>> 64 bytes from 192.168.1.14: icmp_seq=2 ttl=64 time=1.97 ms
> >>>>>> 64 bytes from 192.168.1.14: icmp_seq=3 ttl=64 time=17.2 ms
> >>>>>> 64 bytes from 192.168.1.14: icmp_seq=4 ttl=64 time=2.46 ms
> >>>>>> 64 bytes from 192.168.1.14: icmp_seq=5 ttl=64 time=2.83 ms
> >>>>>> 64 bytes from 192.168.1.14: icmp_seq=6 ttl=64 time=1.40 ms
> >>>>>> 64 bytes from 192.168.1.14: icmp_seq=7 ttl=64 time=2.10 ms
> >>>>>> 64 bytes from 192.168.1.14: icmp_seq=8 ttl=64 time=1.40 ms
> >>>>>> 64 bytes from 192.168.1.14: icmp_seq=9 ttl=64 time=2.04 ms
> >>>>>> 64 bytes from 192.168.1.14: icmp_seq=10 ttl=64 time=1.40 ms
> >>>>>>
> >>>>>> Cc: Dong Aisheng <b29396@freescale.com>
> >>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>>>>> ---
> >>>>>> Changes in v2:
> >>>>>> -New patch replacing "mmc: sdhci: sdio-intel: Set
> >>>>>> SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON quirk"
> >>>>>> ---
> >>>>>> drivers/mmc/host/sdhci-acpi.c | 1 +
> >>>>>> drivers/mmc/host/sdhci-pci-core.c | 1 +
> >>>>>> drivers/mmc/host/sdhci.c | 6 ++++++
> >>>>>> drivers/mmc/host/sdhci.h | 2 ++
> >>>>>> 4 files changed, 10 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
> >>>>>> index 237f318..53b431a 100644
> >>>>>> --- a/drivers/mmc/host/sdhci-acpi.c
> >>>>>> +++ b/drivers/mmc/host/sdhci-acpi.c
> >>>>>> @@ -288,6 +288,7 @@ static const struct sdhci_acpi_slot sdhci_acpi_slot_int_emmc = {
> >>>>>> static const struct sdhci_acpi_slot sdhci_acpi_slot_int_sdio = {
> >>>>>> .quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
> >>>>>> .quirks2 = SDHCI_QUIRK2_HOST_OFF_CARD_ON |
> >>>>>> + SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM |
> >>>>>> SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
> >>>>>> .caps = MMC_CAP_NONREMOVABLE | MMC_CAP_POWER_OFF_CARD |
> >>>>>> MMC_CAP_WAIT_WHILE_BUSY,
> >>>>>> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
> >>>>>> index 982b3e3..a3b242e 100644
> >>>>>> --- a/drivers/mmc/host/sdhci-pci-core.c
> >>>>>> +++ b/drivers/mmc/host/sdhci-pci-core.c
> >>>>>> @@ -498,6 +498,7 @@ static const struct sdhci_pci_fixes sdhci_ni_byt_sdio = {
> >>>>>> static const struct sdhci_pci_fixes sdhci_intel_byt_sdio = {
> >>>>>> .quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
> >>>>>> .quirks2 = SDHCI_QUIRK2_HOST_OFF_CARD_ON |
> >>>>>> + SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM |
> >>>>>> SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
> >>>>>> .allow_runtime_pm = true,
> >>>>>> .probe_slot = byt_sdio_probe_slot,
> >>>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> >>>>>> index 6fdd7a7..59c5cad 100644
> >>>>>> --- a/drivers/mmc/host/sdhci.c
> >>>>>> +++ b/drivers/mmc/host/sdhci.c
> >>>>>> @@ -1828,6 +1828,9 @@ static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
> >>>>>> struct sdhci_host *host = mmc_priv(mmc);
> >>>>>> unsigned long flags;
> >>>>>>
> >>>>>> + if ((host->quirks2 & SDHCI_QUIRK2_SDIO_IRQ_NO_RUNTIME_PM) && enable)
> >>>>>> + pm_runtime_get(host->mmc->parent);
> >>>>>
> >>>>> There are currently no sdhci variant for any SoC that implements
> >>>>> wakeup support in runtime suspend for SDIO irqs. In other words, it
> >>>>> seems all sdhci hosts suffers from the same problem.
> >>>>
> >>>> There might still be drivers that expect SDIO interrupts during
> >>>> runtime suspend, considering this patch:
> >>>>
> >>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=be138554a7923658ded799b0e8794d9c1d08a6e5
> >>>
> >>> Right. Thanks for pointing it out.
> >>>
> >>> However I don't think that is how runtime PM should be deployed when
> >>> it is used for controlling devices like sdhci. Let's change that once
> >>> we have the common issue for sdhci and SDIO irqs resolved.
> >>
> >> Yes, then perhaps we could accept the inverted quirk on the basis that it
> >> will be removed.
> >
> > Well, why have quirk for something that will be always executed. Isn't
> > it better to invent one, when we see a need for it?
>
> I think the idea was to add the quirk to sdhci-esdhc-imx but I doubt
> sdhci-esdhc-imx is really saving much power with the function and interface
> clocks still on, so we can dispense with a quirk for now if that is what you
> prefer.
>
The ahb clock IMX wants to disable can hardly be disabled since it's
system clock and widely used, so it really does not too much power.
And actually Freescale internally was using the similar fix.
http://git.freescale.com/git/cgit.cgi/imx/linux-imx.git/commit/drivers/mmc/host/sdhci.c?h=imx_4.1.15_2.0.0_ga&id=250899a9ca2fdb31fc8d9d5405ac7b1c86beef44
So i'm fine with dispense the quirk for IMX.
Regards
Dong Aisheng
> >
> >>
> >> Hans, as I wrote somewhere else, we can use pm_runtime_get_noresume() and
> >> pm_runtime_put_noidle() since sdhci_enable_sdio_irq() must be called with
> >> the host claimed and therefore runtime resumed.
> >>
> >
> > Good point. This makes it more clear on what goes on.
> >
> > Kind regards
> > Uffe
> >
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-03-29 14:16 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-15 16:31 [PATCH v2 1/2] mmc: sdhci-acpi: Sync quirks with sdhci_intel_byt_* from sdhci-pci-core.c Hans de Goede
2017-03-15 16:31 ` [PATCH v2 2/2] mmc: sdhci: intel: Disable runtime pm when the sdio_irq is enabled Hans de Goede
2017-03-21 10:00 ` Ulf Hansson
2017-03-21 12:34 ` Adrian Hunter
2017-03-21 13:30 ` Hans de Goede
2017-03-21 14:54 ` Ulf Hansson
2017-03-22 12:06 ` Adrian Hunter
2017-03-22 14:00 ` Ulf Hansson
2017-03-22 14:29 ` Adrian Hunter
2017-03-30 6:13 ` Dong Aisheng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.