| Message ID | 20170315163145.15475-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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 > -- 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
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 >> > -- 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
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 -- 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
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. -- 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
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 -- 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
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 > -- 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
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 -- 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
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 */
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(+)