linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 00/11] mmc: core: PM fixes/improvements for SDIO IRQs
@ 2019-09-08 10:12 Ulf Hansson
  2019-09-08 10:12 ` [PATCH v2 01/11] mmc: core: Add helper function to indicate if SDIO IRQs is enabled Ulf Hansson
                   ` (11 more replies)
  0 siblings, 12 replies; 22+ messages in thread
From: Ulf Hansson @ 2019-09-08 10:12 UTC (permalink / raw)
  To: linux-mmc, Ulf Hansson, Adrian Hunter, Douglas Anderson,
	Matthias Kaehlcke
  Cc: Shawn Lin, Jaehoon Chung, Yong Mao, Chaotian Jing, linux-kernel

Changes in v2:
	- Added reviewed/tested-by tags.
	- Updated some changelogs.
	- Renamed sdio_irq_enabled() to sdio_irq_claimed().
	- Don't set sdio_irq_pending when resuming SDIO card, but just queue the
	work.

The power management support for SDIO cards have slowly been improved, but
there are still quite some serious problems, especially when dealing with the
so called SDIO IRQs during system suspend/resume.

This series makes some additional improvements to this code in the mmc core,
but also includes some needed adaptations for the sdhci, the dw_mmc and the
mtk-sd host drivers.

The series is also available at:
git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git sdio_irq_suspend_next_v2

Kind regards
Uffe


Matthias Kaehlcke (1):
  mmc: core: Move code to get pending SDIO IRQs to a function

Ulf Hansson (10):
  mmc: core: Add helper function to indicate if SDIO IRQs is enabled
  mmc: dw_mmc: Re-store SDIO IRQs mask at system resume
  mmc: mtk-sd: Re-store SDIO IRQs mask at system resume
  mmc: core: Clarify sdio_irq_pending flag for
    MMC_CAP2_SDIO_IRQ_NOTHREAD
  mmc: core: Clarify that the ->ack_sdio_irq() callback is mandatory
  mmc: core: WARN if SDIO IRQs are enabled for non-powered card in
    suspend
  mmc: core: Fixup processing of SDIO IRQs during system suspend/resume
  mmc: sdhci: Drop redundant check in sdhci_ack_sdio_irq()
  mmc: sdhci: Drop redundant code for SDIO IRQs
  mmc: sdhci: Convert to use sdio_irq_claimed()

 drivers/mmc/core/sdio.c            |  4 ++-
 drivers/mmc/core/sdio_irq.c        | 57 +++++++++++++++++++-----------
 drivers/mmc/host/dw_mmc.c          |  4 +++
 drivers/mmc/host/mtk-sd.c          |  3 ++
 drivers/mmc/host/sdhci-esdhc-imx.c | 34 ++++++++----------
 drivers/mmc/host/sdhci.c           | 12 ++-----
 drivers/mmc/host/sdhci.h           |  6 ----
 include/linux/mmc/host.h           | 10 ++++++
 8 files changed, 75 insertions(+), 55 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v2 01/11] mmc: core: Add helper function to indicate if SDIO IRQs is enabled
  2019-09-08 10:12 [PATCH v2 00/11] mmc: core: PM fixes/improvements for SDIO IRQs Ulf Hansson
@ 2019-09-08 10:12 ` Ulf Hansson
  2019-09-09 22:32   ` Doug Anderson
  2019-09-08 10:12 ` [PATCH v2 02/11] mmc: dw_mmc: Re-store SDIO IRQs mask at system resume Ulf Hansson
                   ` (10 subsequent siblings)
  11 siblings, 1 reply; 22+ messages in thread
From: Ulf Hansson @ 2019-09-08 10:12 UTC (permalink / raw)
  To: linux-mmc, Ulf Hansson, Adrian Hunter, Douglas Anderson,
	Matthias Kaehlcke
  Cc: Shawn Lin, Jaehoon Chung, Yong Mao, Chaotian Jing, linux-kernel

To avoid each host driver supporting SDIO IRQs, from keeping track
internally about if SDIO IRQs has been claimed, let's introduce a common
helper function, sdio_irq_claimed().

The function returns true if SDIO IRQs are claimed, via using the
information about the number of claimed irqs. This is safe, even without
any locks, as long as the helper function is called only from
runtime/system suspend callbacks of the host driver.

Tested-by: Matthias Kaehlcke <mka@chromium.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v2:
	- Renamed function to sdio_irq_claimed().

---
 include/linux/mmc/host.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 4a351cb7f20f..a9d52a4d5041 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -493,6 +493,15 @@ void mmc_command_done(struct mmc_host *host, struct mmc_request *mrq);
 
 void mmc_cqe_request_done(struct mmc_host *host, struct mmc_request *mrq);
 
+/*
+ * May be called from host driver's system/runtime suspend/resume callbacks,
+ * to know if SDIO IRQs has been claimed.
+*/
+static inline bool sdio_irq_claimed(struct mmc_host *host)
+{
+	return host->sdio_irqs > 0;
+}
+
 static inline void mmc_signal_sdio_irq(struct mmc_host *host)
 {
 	host->ops->enable_sdio_irq(host, 0);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 02/11] mmc: dw_mmc: Re-store SDIO IRQs mask at system resume
  2019-09-08 10:12 [PATCH v2 00/11] mmc: core: PM fixes/improvements for SDIO IRQs Ulf Hansson
  2019-09-08 10:12 ` [PATCH v2 01/11] mmc: core: Add helper function to indicate if SDIO IRQs is enabled Ulf Hansson
@ 2019-09-08 10:12 ` Ulf Hansson
  2019-09-09 22:32   ` Doug Anderson
  2019-09-08 10:12 ` [PATCH v2 03/11] mmc: mtk-sd: " Ulf Hansson
                   ` (9 subsequent siblings)
  11 siblings, 1 reply; 22+ messages in thread
From: Ulf Hansson @ 2019-09-08 10:12 UTC (permalink / raw)
  To: linux-mmc, Ulf Hansson, Adrian Hunter, Douglas Anderson,
	Matthias Kaehlcke
  Cc: Shawn Lin, Jaehoon Chung, Yong Mao, Chaotian Jing, linux-kernel

In cases when SDIO IRQs have been enabled, runtime suspend is prevented by
the driver. However, this still means dw_mci_runtime_suspend|resume() gets
called during system suspend/resume, via pm_runtime_force_suspend|resume().
This means during system suspend/resume, the register context of the dw_mmc
device most likely loses its register context, even in cases when SDIO IRQs
have been enabled.

To re-enable the SDIO IRQs during system resume, the dw_mmc driver
currently relies on the mmc core to re-enable the SDIO IRQs when it resumes
the SDIO card, but this isn't the recommended solution. Instead, it's
better to deal with this locally in the dw_mmc driver, so let's do that.

Tested-by: Matthias Kaehlcke <mka@chromium.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/dw_mmc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index eea52e2c5a0c..79c55c7b4afd 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -3460,6 +3460,10 @@ int dw_mci_runtime_resume(struct device *dev)
 	/* Force setup bus to guarantee available clock output */
 	dw_mci_setup_bus(host->slot, true);
 
+	/* Re-enable SDIO interrupts. */
+	if (sdio_irq_claimed(host->slot->mmc))
+		__dw_mci_enable_sdio_irq(host->slot, 1);
+
 	/* Now that slots are all setup, we can enable card detect */
 	dw_mci_enable_cd(host);
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 03/11] mmc: mtk-sd: Re-store SDIO IRQs mask at system resume
  2019-09-08 10:12 [PATCH v2 00/11] mmc: core: PM fixes/improvements for SDIO IRQs Ulf Hansson
  2019-09-08 10:12 ` [PATCH v2 01/11] mmc: core: Add helper function to indicate if SDIO IRQs is enabled Ulf Hansson
  2019-09-08 10:12 ` [PATCH v2 02/11] mmc: dw_mmc: Re-store SDIO IRQs mask at system resume Ulf Hansson
@ 2019-09-08 10:12 ` Ulf Hansson
  2019-09-09 22:29   ` Doug Anderson
  2019-09-08 10:12 ` [PATCH v2 04/11] mmc: core: Move code to get pending SDIO IRQs to a function Ulf Hansson
                   ` (8 subsequent siblings)
  11 siblings, 1 reply; 22+ messages in thread
From: Ulf Hansson @ 2019-09-08 10:12 UTC (permalink / raw)
  To: linux-mmc, Ulf Hansson, Adrian Hunter, Douglas Anderson,
	Matthias Kaehlcke
  Cc: Shawn Lin, Jaehoon Chung, Yong Mao, Chaotian Jing, linux-kernel

In cases when SDIO IRQs have been enabled, runtime suspend is prevented by
the driver. However, this still means msdc_runtime_suspend|resume() gets
called during system suspend/resume, via pm_runtime_force_suspend|resume().

This means during system suspend/resume, the register context of the mtk-sd
device most likely loses its register context, even in cases when SDIO IRQs
have been enabled.

To re-enable the SDIO IRQs during system resume, the mtk-sd driver
currently relies on the mmc core to re-enable the SDIO IRQs when it resumes
the SDIO card, but this isn't the recommended solution. Instead, it's
better to deal with this locally in the mtk-sd driver, so let's do that.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/mtk-sd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
index 6946bb040a28..ae7688098b7b 100644
--- a/drivers/mmc/host/mtk-sd.c
+++ b/drivers/mmc/host/mtk-sd.c
@@ -2408,6 +2408,9 @@ static void msdc_save_reg(struct msdc_host *host)
 	} else {
 		host->save_para.pad_tune = readl(host->base + tune_reg);
 	}
+
+	if (sdio_irq_claimed(host->mmc))
+		__msdc_enable_sdio_irq(host, 1);
 }
 
 static void msdc_restore_reg(struct msdc_host *host)
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 04/11] mmc: core: Move code to get pending SDIO IRQs to a function
  2019-09-08 10:12 [PATCH v2 00/11] mmc: core: PM fixes/improvements for SDIO IRQs Ulf Hansson
                   ` (2 preceding siblings ...)
  2019-09-08 10:12 ` [PATCH v2 03/11] mmc: mtk-sd: " Ulf Hansson
@ 2019-09-08 10:12 ` Ulf Hansson
  2019-09-09 22:32   ` Doug Anderson
  2019-09-08 10:12 ` [PATCH v2 05/11] mmc: core: Clarify sdio_irq_pending flag for MMC_CAP2_SDIO_IRQ_NOTHREAD Ulf Hansson
                   ` (7 subsequent siblings)
  11 siblings, 1 reply; 22+ messages in thread
From: Ulf Hansson @ 2019-09-08 10:12 UTC (permalink / raw)
  To: linux-mmc, Ulf Hansson, Adrian Hunter, Douglas Anderson,
	Matthias Kaehlcke
  Cc: Shawn Lin, Jaehoon Chung, Yong Mao, Chaotian Jing, linux-kernel

From: Matthias Kaehlcke <mka@chromium.org>

To improve code quality, let's move the code that gets pending SDIO IRQs
from process_sdio_pending_irqs() into a dedicated function.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
[Ulf: Converted function into static]
Tested-by: Matthias Kaehlcke <mka@chromium.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/sdio_irq.c | 46 ++++++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 16 deletions(-)

diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
index 0bcc5e83bd1a..f75043266984 100644
--- a/drivers/mmc/core/sdio_irq.c
+++ b/drivers/mmc/core/sdio_irq.c
@@ -27,6 +27,34 @@
 #include "core.h"
 #include "card.h"
 
+static int sdio_get_pending_irqs(struct mmc_host *host, u8 *pending)
+{
+	struct mmc_card *card = host->card;
+	int ret;
+
+	WARN_ON(!host->claimed);
+
+	ret = mmc_io_rw_direct(card, 0, 0, SDIO_CCCR_INTx, 0, pending);
+	if (ret) {
+		pr_debug("%s: error %d reading SDIO_CCCR_INTx\n",
+		       mmc_card_id(card), ret);
+		return ret;
+	}
+
+	if (*pending && mmc_card_broken_irq_polling(card) &&
+	    !(host->caps & MMC_CAP_SDIO_IRQ)) {
+		unsigned char dummy;
+
+		/* A fake interrupt could be created when we poll SDIO_CCCR_INTx
+		 * register with a Marvell SD8797 card. A dummy CMD52 read to
+		 * function 0 register 0xff can avoid this.
+		 */
+		mmc_io_rw_direct(card, 0, 0, 0xff, 0, &dummy);
+	}
+
+	return 0;
+}
+
 static int process_sdio_pending_irqs(struct mmc_host *host)
 {
 	struct mmc_card *card = host->card;
@@ -49,23 +77,9 @@ static int process_sdio_pending_irqs(struct mmc_host *host)
 		return 1;
 	}
 
-	ret = mmc_io_rw_direct(card, 0, 0, SDIO_CCCR_INTx, 0, &pending);
-	if (ret) {
-		pr_debug("%s: error %d reading SDIO_CCCR_INTx\n",
-		       mmc_card_id(card), ret);
+	ret = sdio_get_pending_irqs(host, &pending);
+	if (ret)
 		return ret;
-	}
-
-	if (pending && mmc_card_broken_irq_polling(card) &&
-	    !(host->caps & MMC_CAP_SDIO_IRQ)) {
-		unsigned char dummy;
-
-		/* A fake interrupt could be created when we poll SDIO_CCCR_INTx
-		 * register with a Marvell SD8797 card. A dummy CMD52 read to
-		 * function 0 register 0xff can avoid this.
-		 */
-		mmc_io_rw_direct(card, 0, 0, 0xff, 0, &dummy);
-	}
 
 	count = 0;
 	for (i = 1; i <= 7; i++) {
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 05/11] mmc: core: Clarify sdio_irq_pending flag for MMC_CAP2_SDIO_IRQ_NOTHREAD
  2019-09-08 10:12 [PATCH v2 00/11] mmc: core: PM fixes/improvements for SDIO IRQs Ulf Hansson
                   ` (3 preceding siblings ...)
  2019-09-08 10:12 ` [PATCH v2 04/11] mmc: core: Move code to get pending SDIO IRQs to a function Ulf Hansson
@ 2019-09-08 10:12 ` Ulf Hansson
  2019-09-09 22:33   ` Doug Anderson
  2019-09-08 10:12 ` [PATCH v2 06/11] mmc: core: Clarify that the ->ack_sdio_irq() callback is mandatory Ulf Hansson
                   ` (6 subsequent siblings)
  11 siblings, 1 reply; 22+ messages in thread
From: Ulf Hansson @ 2019-09-08 10:12 UTC (permalink / raw)
  To: linux-mmc, Ulf Hansson, Adrian Hunter, Douglas Anderson,
	Matthias Kaehlcke
  Cc: Shawn Lin, Jaehoon Chung, Yong Mao, Chaotian Jing, linux-kernel

The sdio_irq_pending flag is used to let host drivers indicate that it has
signaled an IRQ. If that is the case and we only have a single SDIO func
that have claimed an SDIO IRQ, our assumption is that we can avoid reading
the SDIO_CCCR_INTx register and just call the SDIO func irq handler
immediately. This makes sense, but the flag is set/cleared in a somewhat
messy order, let's fix that up according to below.

First, the flag is currently set in sdio_run_irqs(), which is executed as a
work that was scheduled from sdio_signal_irq(). To make it more implicit
that the host have signaled an IRQ, let's instead immediately set the flag
in sdio_signal_irq(). This also makes the behavior consistent with host
drivers that uses the legacy, mmc_signal_sdio_irq() API. This have no
functional impact, because we don't expect host drivers to call
sdio_signal_irq() until after the work (sdio_run_irqs()) have been executed
anyways.

Second, currently we never clears the flag when using the sdio_run_irqs()
work, but only when using the sdio_irq_thread(). Let make the behavior
consistent, by moving the flag to be cleared inside the common
process_sdio_pending_irqs() function. Additionally, tweak the behavior of
the flag slightly, by avoiding to clear it unless we processed the SDIO
IRQ. The purpose with this at this point, is to keep the information about
whether there have been an SDIO IRQ signaled by the host, so at system
resume we can decide to process it without reading the SDIO_CCCR_INTx
register.

Tested-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v2:
	- Re-wrote changelog.

---
 drivers/mmc/core/sdio_irq.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
index f75043266984..0962a4357d54 100644
--- a/drivers/mmc/core/sdio_irq.c
+++ b/drivers/mmc/core/sdio_irq.c
@@ -59,6 +59,7 @@ static int process_sdio_pending_irqs(struct mmc_host *host)
 {
 	struct mmc_card *card = host->card;
 	int i, ret, count;
+	bool sdio_irq_pending = host->sdio_irq_pending;
 	unsigned char pending;
 	struct sdio_func *func;
 
@@ -66,13 +67,16 @@ static int process_sdio_pending_irqs(struct mmc_host *host)
 	if (mmc_card_suspended(card))
 		return 0;
 
+	/* Clear the flag to indicate that we have processed the IRQ. */
+	host->sdio_irq_pending = false;
+
 	/*
 	 * Optimization, if there is only 1 function interrupt registered
 	 * and we know an IRQ was signaled then call irq handler directly.
 	 * Otherwise do the full probe.
 	 */
 	func = card->sdio_single_irq;
-	if (func && host->sdio_irq_pending) {
+	if (func && sdio_irq_pending) {
 		func->irq_handler(func);
 		return 1;
 	}
@@ -110,7 +114,6 @@ static void sdio_run_irqs(struct mmc_host *host)
 {
 	mmc_claim_host(host);
 	if (host->sdio_irqs) {
-		host->sdio_irq_pending = true;
 		process_sdio_pending_irqs(host);
 		if (host->ops->ack_sdio_irq)
 			host->ops->ack_sdio_irq(host);
@@ -128,6 +131,7 @@ void sdio_irq_work(struct work_struct *work)
 
 void sdio_signal_irq(struct mmc_host *host)
 {
+	host->sdio_irq_pending = true;
 	queue_delayed_work(system_wq, &host->sdio_irq_work, 0);
 }
 EXPORT_SYMBOL_GPL(sdio_signal_irq);
@@ -173,7 +177,6 @@ static int sdio_irq_thread(void *_host)
 		if (ret)
 			break;
 		ret = process_sdio_pending_irqs(host);
-		host->sdio_irq_pending = false;
 		mmc_release_host(host);
 
 		/*
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 06/11] mmc: core: Clarify that the ->ack_sdio_irq() callback is mandatory
  2019-09-08 10:12 [PATCH v2 00/11] mmc: core: PM fixes/improvements for SDIO IRQs Ulf Hansson
                   ` (4 preceding siblings ...)
  2019-09-08 10:12 ` [PATCH v2 05/11] mmc: core: Clarify sdio_irq_pending flag for MMC_CAP2_SDIO_IRQ_NOTHREAD Ulf Hansson
@ 2019-09-08 10:12 ` Ulf Hansson
  2019-09-09 22:34   ` Doug Anderson
  2019-09-08 10:12 ` [PATCH v2 07/11] mmc: core: WARN if SDIO IRQs are enabled for non-powered card in suspend Ulf Hansson
                   ` (5 subsequent siblings)
  11 siblings, 1 reply; 22+ messages in thread
From: Ulf Hansson @ 2019-09-08 10:12 UTC (permalink / raw)
  To: linux-mmc, Ulf Hansson, Adrian Hunter, Douglas Anderson,
	Matthias Kaehlcke
  Cc: Shawn Lin, Jaehoon Chung, Yong Mao, Chaotian Jing, linux-kernel

For the MMC_CAP2_SDIO_IRQ_NOTHREAD case and when using sdio_signal_irq(),
the ->ack_sdio_irq() is already mandatory, which was not the case for those
host drivers that called sdio_run_irqs() directly.

As there are no longer any drivers calling sdio_run_irqs(), let's clarify
the code by dropping the unnecessary check and explicitly state that the
callback is mandatory in the header file.

Tested-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/sdio_irq.c | 3 +--
 include/linux/mmc/host.h    | 1 +
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
index 0962a4357d54..d7965b53a6d2 100644
--- a/drivers/mmc/core/sdio_irq.c
+++ b/drivers/mmc/core/sdio_irq.c
@@ -115,8 +115,7 @@ static void sdio_run_irqs(struct mmc_host *host)
 	mmc_claim_host(host);
 	if (host->sdio_irqs) {
 		process_sdio_pending_irqs(host);
-		if (host->ops->ack_sdio_irq)
-			host->ops->ack_sdio_irq(host);
+		host->ops->ack_sdio_irq(host);
 	}
 	mmc_release_host(host);
 }
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index a9d52a4d5041..0e6afe66cf68 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -128,6 +128,7 @@ struct mmc_host_ops {
 	int	(*get_cd)(struct mmc_host *host);
 
 	void	(*enable_sdio_irq)(struct mmc_host *host, int enable);
+	/* Mandatory callback when using MMC_CAP2_SDIO_IRQ_NOTHREAD. */
 	void	(*ack_sdio_irq)(struct mmc_host *host);
 
 	/* optional callback for HC quirks */
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 07/11] mmc: core: WARN if SDIO IRQs are enabled for non-powered card in suspend
  2019-09-08 10:12 [PATCH v2 00/11] mmc: core: PM fixes/improvements for SDIO IRQs Ulf Hansson
                   ` (5 preceding siblings ...)
  2019-09-08 10:12 ` [PATCH v2 06/11] mmc: core: Clarify that the ->ack_sdio_irq() callback is mandatory Ulf Hansson
@ 2019-09-08 10:12 ` Ulf Hansson
  2019-09-08 10:12 ` [PATCH v2 08/11] mmc: core: Fixup processing of SDIO IRQs during system suspend/resume Ulf Hansson
                   ` (4 subsequent siblings)
  11 siblings, 0 replies; 22+ messages in thread
From: Ulf Hansson @ 2019-09-08 10:12 UTC (permalink / raw)
  To: linux-mmc, Ulf Hansson, Adrian Hunter, Douglas Anderson,
	Matthias Kaehlcke
  Cc: Shawn Lin, Jaehoon Chung, Yong Mao, Chaotian Jing, linux-kernel

To make sure SDIO func drivers behaves correctly during system
suspend/resume, let add a WARN_ON in case the condition is a non-powered
SDIO card and there are some SDIO IRQs still being claimed.

Tested-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/sdio.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 8dd8fc32ecca..c557f1519b77 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -951,6 +951,8 @@ static int mmc_sdio_pre_suspend(struct mmc_host *host)
  */
 static int mmc_sdio_suspend(struct mmc_host *host)
 {
+	WARN_ON(host->sdio_irqs && !mmc_card_keep_power(host));
+
 	/* Prevent processing of SDIO IRQs in suspended state. */
 	mmc_card_set_suspended(host->card);
 	cancel_delayed_work_sync(&host->sdio_irq_work);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 08/11] mmc: core: Fixup processing of SDIO IRQs during system suspend/resume
  2019-09-08 10:12 [PATCH v2 00/11] mmc: core: PM fixes/improvements for SDIO IRQs Ulf Hansson
                   ` (6 preceding siblings ...)
  2019-09-08 10:12 ` [PATCH v2 07/11] mmc: core: WARN if SDIO IRQs are enabled for non-powered card in suspend Ulf Hansson
@ 2019-09-08 10:12 ` Ulf Hansson
  2019-09-09 22:34   ` Doug Anderson
  2019-09-08 10:12 ` [PATCH v2 09/11] mmc: sdhci: Drop redundant check in sdhci_ack_sdio_irq() Ulf Hansson
                   ` (3 subsequent siblings)
  11 siblings, 1 reply; 22+ messages in thread
From: Ulf Hansson @ 2019-09-08 10:12 UTC (permalink / raw)
  To: linux-mmc, Ulf Hansson, Adrian Hunter, Douglas Anderson,
	Matthias Kaehlcke
  Cc: Shawn Lin, Jaehoon Chung, Yong Mao, Chaotian Jing, linux-kernel

System suspend/resume of SDIO cards, with SDIO IRQs enabled and when using
MMC_CAP2_SDIO_IRQ_NOTHREAD is unfortunate still suffering from a fragile
behaviour. Some problems have been taken care of so far, but more issues
remains.

For example, calling the ->ack_sdio_irq() callback to let host drivers
re-enable the SDIO IRQs is a bad idea, unless the IRQ have been consumed,
which may not be the case during system suspend/resume. This may lead to
that a host driver re-signals the same SDIO IRQ over and over again,
causing a storm of IRQs and gives a ping-pong effect towards the
sdio_irq_work().

Moreover, calling the ->enable_sdio_irq() callback at system resume to
re-enable already enabled SDIO IRQs for the host, causes the runtime PM
count for some host drivers to become in-balanced. This then leads to the
host to remain runtime resumed, no matter if it's needed or not.

To fix these problems, let's check if process_sdio_pending_irqs() actually
consumed the SDIO IRQ, before we continue to ack the IRQ by invoking the
->ack_sdio_irq() callback.

Additionally, there should be no need to re-enable SDIO IRQs as the host
driver already knows if they were enabled at system suspend, thus also
whether it needs to re-enable them at system resume. For this reason, drop
the call to ->enable_sdio_irq() during system resume.

In regards to these changes there is yet another issue, which is when there
is an SDIO IRQ being signaled by the host driver, but after the SDIO card
has been system suspended. Currently these IRQs are just thrown away, while
we should at least make sure to try to consume them when the SDIO card has
been system resumed. Fix this by queueing a sdio_irq_work() after we system
resumed the SDIO card.

Tested-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v2:
	- Queue a sdio_irq_work() rather using sdio_signal_irq().

---
 drivers/mmc/core/sdio.c     | 2 +-
 drivers/mmc/core/sdio_irq.c | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index c557f1519b77..26cabd53ddc5 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -1015,7 +1015,7 @@ static int mmc_sdio_resume(struct mmc_host *host)
 		if (!(host->caps2 & MMC_CAP2_SDIO_IRQ_NOTHREAD))
 			wake_up_process(host->sdio_irq_thread);
 		else if (host->caps & MMC_CAP_SDIO_IRQ)
-			host->ops->enable_sdio_irq(host, 1);
+			queue_delayed_work(system_wq, &host->sdio_irq_work, 0);
 	}
 
 out:
diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
index d7965b53a6d2..900871073bd7 100644
--- a/drivers/mmc/core/sdio_irq.c
+++ b/drivers/mmc/core/sdio_irq.c
@@ -115,7 +115,8 @@ static void sdio_run_irqs(struct mmc_host *host)
 	mmc_claim_host(host);
 	if (host->sdio_irqs) {
 		process_sdio_pending_irqs(host);
-		host->ops->ack_sdio_irq(host);
+		if (!host->sdio_irq_pending)
+			host->ops->ack_sdio_irq(host);
 	}
 	mmc_release_host(host);
 }
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 09/11] mmc: sdhci: Drop redundant check in sdhci_ack_sdio_irq()
  2019-09-08 10:12 [PATCH v2 00/11] mmc: core: PM fixes/improvements for SDIO IRQs Ulf Hansson
                   ` (7 preceding siblings ...)
  2019-09-08 10:12 ` [PATCH v2 08/11] mmc: core: Fixup processing of SDIO IRQs during system suspend/resume Ulf Hansson
@ 2019-09-08 10:12 ` Ulf Hansson
  2019-09-09 11:19   ` Adrian Hunter
  2019-09-08 10:12 ` [PATCH v2 10/11] mmc: sdhci: Drop redundant code for SDIO IRQs Ulf Hansson
                   ` (2 subsequent siblings)
  11 siblings, 1 reply; 22+ messages in thread
From: Ulf Hansson @ 2019-09-08 10:12 UTC (permalink / raw)
  To: linux-mmc, Ulf Hansson, Adrian Hunter, Douglas Anderson,
	Matthias Kaehlcke
  Cc: Shawn Lin, Jaehoon Chung, Yong Mao, Chaotian Jing, linux-kernel

The sdhci_ack_sdio_irq() is called only when SDIO IRQs are enabled.
Therefore, let's drop the redundant check of the internal
SDHCI_SDIO_IRQ_ENABLED flag and just re-enable the IRQs immediately.

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/sdhci.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index d814dc004bad..efa6cda8c991 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2163,8 +2163,7 @@ static void sdhci_ack_sdio_irq(struct mmc_host *mmc)
 	unsigned long flags;
 
 	spin_lock_irqsave(&host->lock, flags);
-	if (host->flags & SDHCI_SDIO_IRQ_ENABLED)
-		sdhci_enable_sdio_irq_nolock(host, true);
+	sdhci_enable_sdio_irq_nolock(host, true);
 	spin_unlock_irqrestore(&host->lock, flags);
 }
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 10/11] mmc: sdhci: Drop redundant code for SDIO IRQs
  2019-09-08 10:12 [PATCH v2 00/11] mmc: core: PM fixes/improvements for SDIO IRQs Ulf Hansson
                   ` (8 preceding siblings ...)
  2019-09-08 10:12 ` [PATCH v2 09/11] mmc: sdhci: Drop redundant check in sdhci_ack_sdio_irq() Ulf Hansson
@ 2019-09-08 10:12 ` Ulf Hansson
  2019-09-08 10:12 ` [PATCH v2 11/11] mmc: sdhci: Convert to use sdio_irq_claimed() Ulf Hansson
  2019-09-11 14:21 ` [PATCH v2 00/11] mmc: core: PM fixes/improvements for SDIO IRQs Ulf Hansson
  11 siblings, 0 replies; 22+ messages in thread
From: Ulf Hansson @ 2019-09-08 10:12 UTC (permalink / raw)
  To: linux-mmc, Ulf Hansson, Adrian Hunter, Douglas Anderson,
	Matthias Kaehlcke
  Cc: Shawn Lin, Jaehoon Chung, Yong Mao, Chaotian Jing, linux-kernel

Nowadays sdhci prevents runtime suspend when SDIO IRQs are enabled.

However, some variants such as sdhci-esdhc-imx's, tries to allow runtime
suspend while having the SDIO IRQs enabled, but without supporting remote
wakeups. This support is a bit questionable, especially if the host device
have a PM domain attached that can be power gated, but more importantly,
the code have also become redundant (which was not the case when it was
introduced).

Rather than keeping the redundant code around, let's drop it and leave this
to be revisited later on.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 34 +++++++++++++-----------------
 drivers/mmc/host/sdhci.c           |  2 +-
 drivers/mmc/host/sdhci.h           |  5 -----
 3 files changed, 16 insertions(+), 25 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 776a94216248..1c988d6a2433 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -1666,12 +1666,10 @@ static int sdhci_esdhc_runtime_suspend(struct device *dev)
 	if (host->tuning_mode != SDHCI_TUNING_MODE_3)
 		mmc_retune_needed(host->mmc);
 
-	if (!sdhci_sdio_irq_enabled(host)) {
-		imx_data->actual_clock = host->mmc->actual_clock;
-		esdhc_pltfm_set_clock(host, 0);
-		clk_disable_unprepare(imx_data->clk_per);
-		clk_disable_unprepare(imx_data->clk_ipg);
-	}
+	imx_data->actual_clock = host->mmc->actual_clock;
+	esdhc_pltfm_set_clock(host, 0);
+	clk_disable_unprepare(imx_data->clk_per);
+	clk_disable_unprepare(imx_data->clk_ipg);
 	clk_disable_unprepare(imx_data->clk_ahb);
 
 	if (imx_data->socdata->flags & ESDHC_FLAG_PMQOS)
@@ -1695,15 +1693,15 @@ static int sdhci_esdhc_runtime_resume(struct device *dev)
 	if (err)
 		goto remove_pm_qos_request;
 
-	if (!sdhci_sdio_irq_enabled(host)) {
-		err = clk_prepare_enable(imx_data->clk_per);
-		if (err)
-			goto disable_ahb_clk;
-		err = clk_prepare_enable(imx_data->clk_ipg);
-		if (err)
-			goto disable_per_clk;
-		esdhc_pltfm_set_clock(host, imx_data->actual_clock);
-	}
+	err = clk_prepare_enable(imx_data->clk_per);
+	if (err)
+		goto disable_ahb_clk;
+
+	err = clk_prepare_enable(imx_data->clk_ipg);
+	if (err)
+		goto disable_per_clk;
+
+	esdhc_pltfm_set_clock(host, imx_data->actual_clock);
 
 	err = sdhci_runtime_resume_host(host, 0);
 	if (err)
@@ -1715,11 +1713,9 @@ static int sdhci_esdhc_runtime_resume(struct device *dev)
 	return err;
 
 disable_ipg_clk:
-	if (!sdhci_sdio_irq_enabled(host))
-		clk_disable_unprepare(imx_data->clk_ipg);
+	clk_disable_unprepare(imx_data->clk_ipg);
 disable_per_clk:
-	if (!sdhci_sdio_irq_enabled(host))
-		clk_disable_unprepare(imx_data->clk_per);
+	clk_disable_unprepare(imx_data->clk_per);
 disable_ahb_clk:
 	clk_disable_unprepare(imx_data->clk_ahb);
 remove_pm_qos_request:
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index efa6cda8c991..4c4285387b47 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -3048,7 +3048,7 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
 
 	spin_lock(&host->lock);
 
-	if (host->runtime_suspended && !sdhci_sdio_irq_enabled(host)) {
+	if (host->runtime_suspended) {
 		spin_unlock(&host->lock);
 		return IRQ_NONE;
 	}
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index cf3d1ed91909..8effaac61c3a 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -753,11 +753,6 @@ static inline void sdhci_read_caps(struct sdhci_host *host)
 	__sdhci_read_caps(host, NULL, NULL, NULL);
 }
 
-static inline bool sdhci_sdio_irq_enabled(struct sdhci_host *host)
-{
-	return !!(host->flags & SDHCI_SDIO_IRQ_ENABLED);
-}
-
 u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
 		   unsigned int *actual_clock);
 void sdhci_set_clock(struct sdhci_host *host, unsigned int clock);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 11/11] mmc: sdhci: Convert to use sdio_irq_claimed()
  2019-09-08 10:12 [PATCH v2 00/11] mmc: core: PM fixes/improvements for SDIO IRQs Ulf Hansson
                   ` (9 preceding siblings ...)
  2019-09-08 10:12 ` [PATCH v2 10/11] mmc: sdhci: Drop redundant code for SDIO IRQs Ulf Hansson
@ 2019-09-08 10:12 ` Ulf Hansson
  2019-09-11 14:21 ` [PATCH v2 00/11] mmc: core: PM fixes/improvements for SDIO IRQs Ulf Hansson
  11 siblings, 0 replies; 22+ messages in thread
From: Ulf Hansson @ 2019-09-08 10:12 UTC (permalink / raw)
  To: linux-mmc, Ulf Hansson, Adrian Hunter, Douglas Anderson,
	Matthias Kaehlcke
  Cc: Shawn Lin, Jaehoon Chung, Yong Mao, Chaotian Jing, linux-kernel

Instead of keeping track of whether SDIO IRQs have been enabled via an
internal sdhci status flag, avoid the open-coding and convert into using
sdio_irq_claimed().

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/sdhci.c | 7 +------
 drivers/mmc/host/sdhci.h | 1 -
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 4c4285387b47..4b297f397326 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2144,11 +2144,6 @@ void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
 		pm_runtime_get_noresume(host->mmc->parent);
 
 	spin_lock_irqsave(&host->lock, flags);
-	if (enable)
-		host->flags |= SDHCI_SDIO_IRQ_ENABLED;
-	else
-		host->flags &= ~SDHCI_SDIO_IRQ_ENABLED;
-
 	sdhci_enable_sdio_irq_nolock(host, enable);
 	spin_unlock_irqrestore(&host->lock, flags);
 
@@ -3382,7 +3377,7 @@ int sdhci_runtime_resume_host(struct sdhci_host *host, int soft_reset)
 	host->runtime_suspended = false;
 
 	/* Enable SDIO IRQ */
-	if (host->flags & SDHCI_SDIO_IRQ_ENABLED)
+	if (sdio_irq_claimed(mmc))
 		sdhci_enable_sdio_irq_nolock(host, true);
 
 	/* Enable Card Detection */
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 8effaac61c3a..a29c4cd2d92e 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -512,7 +512,6 @@ struct sdhci_host {
 #define SDHCI_AUTO_CMD12	(1<<6)	/* Auto CMD12 support */
 #define SDHCI_AUTO_CMD23	(1<<7)	/* Auto CMD23 support */
 #define SDHCI_PV_ENABLED	(1<<8)	/* Preset value enabled */
-#define SDHCI_SDIO_IRQ_ENABLED	(1<<9)	/* SDIO irq enabled */
 #define SDHCI_USE_64_BIT_DMA	(1<<12)	/* Use 64-bit DMA */
 #define SDHCI_HS400_TUNING	(1<<13)	/* Tuning for HS400 */
 #define SDHCI_SIGNALING_330	(1<<14)	/* Host is capable of 3.3V signaling */
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 09/11] mmc: sdhci: Drop redundant check in sdhci_ack_sdio_irq()
  2019-09-08 10:12 ` [PATCH v2 09/11] mmc: sdhci: Drop redundant check in sdhci_ack_sdio_irq() Ulf Hansson
@ 2019-09-09 11:19   ` Adrian Hunter
  0 siblings, 0 replies; 22+ messages in thread
From: Adrian Hunter @ 2019-09-09 11:19 UTC (permalink / raw)
  To: Ulf Hansson, linux-mmc, Douglas Anderson, Matthias Kaehlcke
  Cc: Shawn Lin, Jaehoon Chung, Yong Mao, Chaotian Jing, linux-kernel

On 8/09/19 1:12 PM, Ulf Hansson wrote:
> The sdhci_ack_sdio_irq() is called only when SDIO IRQs are enabled.
> Therefore, let's drop the redundant check of the internal
> SDHCI_SDIO_IRQ_ENABLED flag and just re-enable the IRQs immediately.
> 
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/mmc/host/sdhci.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index d814dc004bad..efa6cda8c991 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2163,8 +2163,7 @@ static void sdhci_ack_sdio_irq(struct mmc_host *mmc)
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&host->lock, flags);
> -	if (host->flags & SDHCI_SDIO_IRQ_ENABLED)
> -		sdhci_enable_sdio_irq_nolock(host, true);
> +	sdhci_enable_sdio_irq_nolock(host, true);
>  	spin_unlock_irqrestore(&host->lock, flags);
>  }
>  
> 


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 03/11] mmc: mtk-sd: Re-store SDIO IRQs mask at system resume
  2019-09-08 10:12 ` [PATCH v2 03/11] mmc: mtk-sd: " Ulf Hansson
@ 2019-09-09 22:29   ` Doug Anderson
  2019-09-11 12:06     ` Ulf Hansson
  0 siblings, 1 reply; 22+ messages in thread
From: Doug Anderson @ 2019-09-09 22:29 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Linux MMC List, Adrian Hunter, Matthias Kaehlcke, Shawn Lin,
	Jaehoon Chung, Yong Mao, Chaotian Jing, LKML, Nicolas Boichat,
	Daniel Kurtz

Hi,

On Sun, Sep 8, 2019 at 3:12 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> In cases when SDIO IRQs have been enabled, runtime suspend is prevented by
> the driver. However, this still means msdc_runtime_suspend|resume() gets
> called during system suspend/resume, via pm_runtime_force_suspend|resume().
>
> This means during system suspend/resume, the register context of the mtk-sd
> device most likely loses its register context, even in cases when SDIO IRQs
> have been enabled.
>
> To re-enable the SDIO IRQs during system resume, the mtk-sd driver
> currently relies on the mmc core to re-enable the SDIO IRQs when it resumes
> the SDIO card, but this isn't the recommended solution. Instead, it's
> better to deal with this locally in the mtk-sd driver, so let's do that.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/host/mtk-sd.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> index 6946bb040a28..ae7688098b7b 100644
> --- a/drivers/mmc/host/mtk-sd.c
> +++ b/drivers/mmc/host/mtk-sd.c
> @@ -2408,6 +2408,9 @@ static void msdc_save_reg(struct msdc_host *host)
>         } else {
>                 host->save_para.pad_tune = readl(host->base + tune_reg);
>         }
> +
> +       if (sdio_irq_claimed(host->mmc))
> +               __msdc_enable_sdio_irq(host, 1);
>  }
>
>  static void msdc_restore_reg(struct msdc_host *host)

I don't personally have a Mediatek device setup to test this patch on.
If it's super urgent I could try to track down one and try to set it
up, but hopefully it's easier for someone else...

That being said, from code inspection it seems like you should be
adding your code to msdc_restore_reg(), not to msdc_save_reg().  Am I
confused?

-Doug

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 01/11] mmc: core: Add helper function to indicate if SDIO IRQs is enabled
  2019-09-08 10:12 ` [PATCH v2 01/11] mmc: core: Add helper function to indicate if SDIO IRQs is enabled Ulf Hansson
@ 2019-09-09 22:32   ` Doug Anderson
  0 siblings, 0 replies; 22+ messages in thread
From: Doug Anderson @ 2019-09-09 22:32 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Linux MMC List, Adrian Hunter, Matthias Kaehlcke, Shawn Lin,
	Jaehoon Chung, Yong Mao, Chaotian Jing, LKML

Hi,

On Sun, Sep 8, 2019 at 3:12 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> To avoid each host driver supporting SDIO IRQs, from keeping track
> internally about if SDIO IRQs has been claimed, let's introduce a common
> helper function, sdio_irq_claimed().
>
> The function returns true if SDIO IRQs are claimed, via using the
> information about the number of claimed irqs. This is safe, even without
> any locks, as long as the helper function is called only from
> runtime/system suspend callbacks of the host driver.
>
> Tested-by: Matthias Kaehlcke <mka@chromium.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>
> Changes in v2:
>         - Renamed function to sdio_irq_claimed().
>
> ---
>  include/linux/mmc/host.h | 9 +++++++++
>  1 file changed, 9 insertions(+)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 02/11] mmc: dw_mmc: Re-store SDIO IRQs mask at system resume
  2019-09-08 10:12 ` [PATCH v2 02/11] mmc: dw_mmc: Re-store SDIO IRQs mask at system resume Ulf Hansson
@ 2019-09-09 22:32   ` Doug Anderson
  0 siblings, 0 replies; 22+ messages in thread
From: Doug Anderson @ 2019-09-09 22:32 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Linux MMC List, Adrian Hunter, Matthias Kaehlcke, Shawn Lin,
	Jaehoon Chung, Yong Mao, Chaotian Jing, LKML

Hi,

On Sun, Sep 8, 2019 at 3:12 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> In cases when SDIO IRQs have been enabled, runtime suspend is prevented by
> the driver. However, this still means dw_mci_runtime_suspend|resume() gets
> called during system suspend/resume, via pm_runtime_force_suspend|resume().
> This means during system suspend/resume, the register context of the dw_mmc
> device most likely loses its register context, even in cases when SDIO IRQs
> have been enabled.
>
> To re-enable the SDIO IRQs during system resume, the dw_mmc driver
> currently relies on the mmc core to re-enable the SDIO IRQs when it resumes
> the SDIO card, but this isn't the recommended solution. Instead, it's
> better to deal with this locally in the dw_mmc driver, so let's do that.
>
> Tested-by: Matthias Kaehlcke <mka@chromium.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/host/dw_mmc.c | 4 ++++
>  1 file changed, 4 insertions(+)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 04/11] mmc: core: Move code to get pending SDIO IRQs to a function
  2019-09-08 10:12 ` [PATCH v2 04/11] mmc: core: Move code to get pending SDIO IRQs to a function Ulf Hansson
@ 2019-09-09 22:32   ` Doug Anderson
  0 siblings, 0 replies; 22+ messages in thread
From: Doug Anderson @ 2019-09-09 22:32 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Linux MMC List, Adrian Hunter, Matthias Kaehlcke, Shawn Lin,
	Jaehoon Chung, Yong Mao, Chaotian Jing, LKML

Hi,

On Sun, Sep 8, 2019 at 3:12 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> From: Matthias Kaehlcke <mka@chromium.org>
>
> To improve code quality, let's move the code that gets pending SDIO IRQs
> from process_sdio_pending_irqs() into a dedicated function.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> [Ulf: Converted function into static]
> Tested-by: Matthias Kaehlcke <mka@chromium.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/core/sdio_irq.c | 46 ++++++++++++++++++++++++-------------
>  1 file changed, 30 insertions(+), 16 deletions(-)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 05/11] mmc: core: Clarify sdio_irq_pending flag for MMC_CAP2_SDIO_IRQ_NOTHREAD
  2019-09-08 10:12 ` [PATCH v2 05/11] mmc: core: Clarify sdio_irq_pending flag for MMC_CAP2_SDIO_IRQ_NOTHREAD Ulf Hansson
@ 2019-09-09 22:33   ` Doug Anderson
  0 siblings, 0 replies; 22+ messages in thread
From: Doug Anderson @ 2019-09-09 22:33 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Linux MMC List, Adrian Hunter, Matthias Kaehlcke, Shawn Lin,
	Jaehoon Chung, Yong Mao, Chaotian Jing, LKML

Hi,

On Sun, Sep 8, 2019 at 3:12 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> The sdio_irq_pending flag is used to let host drivers indicate that it has
> signaled an IRQ. If that is the case and we only have a single SDIO func
> that have claimed an SDIO IRQ, our assumption is that we can avoid reading
> the SDIO_CCCR_INTx register and just call the SDIO func irq handler
> immediately. This makes sense, but the flag is set/cleared in a somewhat
> messy order, let's fix that up according to below.
>
> First, the flag is currently set in sdio_run_irqs(), which is executed as a
> work that was scheduled from sdio_signal_irq(). To make it more implicit
> that the host have signaled an IRQ, let's instead immediately set the flag
> in sdio_signal_irq(). This also makes the behavior consistent with host
> drivers that uses the legacy, mmc_signal_sdio_irq() API. This have no
> functional impact, because we don't expect host drivers to call
> sdio_signal_irq() until after the work (sdio_run_irqs()) have been executed
> anyways.
>
> Second, currently we never clears the flag when using the sdio_run_irqs()
> work, but only when using the sdio_irq_thread(). Let make the behavior

s/Let/Let's


> consistent, by moving the flag to be cleared inside the common
> process_sdio_pending_irqs() function. Additionally, tweak the behavior of
> the flag slightly, by avoiding to clear it unless we processed the SDIO
> IRQ. The purpose with this at this point, is to keep the information about
> whether there have been an SDIO IRQ signaled by the host, so at system
> resume we can decide to process it without reading the SDIO_CCCR_INTx
> register.
>
> Tested-by: Matthias Kaehlcke <mka@chromium.org>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>
> Changes in v2:
>         - Re-wrote changelog.
>
> ---
>  drivers/mmc/core/sdio_irq.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 06/11] mmc: core: Clarify that the ->ack_sdio_irq() callback is mandatory
  2019-09-08 10:12 ` [PATCH v2 06/11] mmc: core: Clarify that the ->ack_sdio_irq() callback is mandatory Ulf Hansson
@ 2019-09-09 22:34   ` Doug Anderson
  0 siblings, 0 replies; 22+ messages in thread
From: Doug Anderson @ 2019-09-09 22:34 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Linux MMC List, Adrian Hunter, Matthias Kaehlcke, Shawn Lin,
	Jaehoon Chung, Yong Mao, Chaotian Jing, LKML

Hi,

On Sun, Sep 8, 2019 at 3:12 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> For the MMC_CAP2_SDIO_IRQ_NOTHREAD case and when using sdio_signal_irq(),
> the ->ack_sdio_irq() is already mandatory, which was not the case for those
> host drivers that called sdio_run_irqs() directly.
>
> As there are no longer any drivers calling sdio_run_irqs(), let's clarify
> the code by dropping the unnecessary check and explicitly state that the
> callback is mandatory in the header file.
>
> Tested-by: Matthias Kaehlcke <mka@chromium.org>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/core/sdio_irq.c | 3 +--
>  include/linux/mmc/host.h    | 1 +
>  2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 08/11] mmc: core: Fixup processing of SDIO IRQs during system suspend/resume
  2019-09-08 10:12 ` [PATCH v2 08/11] mmc: core: Fixup processing of SDIO IRQs during system suspend/resume Ulf Hansson
@ 2019-09-09 22:34   ` Doug Anderson
  0 siblings, 0 replies; 22+ messages in thread
From: Doug Anderson @ 2019-09-09 22:34 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Linux MMC List, Adrian Hunter, Matthias Kaehlcke, Shawn Lin,
	Jaehoon Chung, Yong Mao, Chaotian Jing, LKML

Hi,

On Sun, Sep 8, 2019 at 3:12 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> System suspend/resume of SDIO cards, with SDIO IRQs enabled and when using
> MMC_CAP2_SDIO_IRQ_NOTHREAD is unfortunate still suffering from a fragile
> behaviour. Some problems have been taken care of so far, but more issues
> remains.
>
> For example, calling the ->ack_sdio_irq() callback to let host drivers
> re-enable the SDIO IRQs is a bad idea, unless the IRQ have been consumed,
> which may not be the case during system suspend/resume. This may lead to
> that a host driver re-signals the same SDIO IRQ over and over again,
> causing a storm of IRQs and gives a ping-pong effect towards the
> sdio_irq_work().
>
> Moreover, calling the ->enable_sdio_irq() callback at system resume to
> re-enable already enabled SDIO IRQs for the host, causes the runtime PM
> count for some host drivers to become in-balanced. This then leads to the
> host to remain runtime resumed, no matter if it's needed or not.
>
> To fix these problems, let's check if process_sdio_pending_irqs() actually
> consumed the SDIO IRQ, before we continue to ack the IRQ by invoking the
> ->ack_sdio_irq() callback.
>
> Additionally, there should be no need to re-enable SDIO IRQs as the host
> driver already knows if they were enabled at system suspend, thus also
> whether it needs to re-enable them at system resume. For this reason, drop
> the call to ->enable_sdio_irq() during system resume.
>
> In regards to these changes there is yet another issue, which is when there
> is an SDIO IRQ being signaled by the host driver, but after the SDIO card
> has been system suspended. Currently these IRQs are just thrown away, while
> we should at least make sure to try to consume them when the SDIO card has
> been system resumed. Fix this by queueing a sdio_irq_work() after we system
> resumed the SDIO card.
>
> Tested-by: Matthias Kaehlcke <mka@chromium.org>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>
> Changes in v2:
>         - Queue a sdio_irq_work() rather using sdio_signal_irq().
>
> ---
>  drivers/mmc/core/sdio.c     | 2 +-
>  drivers/mmc/core/sdio_irq.c | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 03/11] mmc: mtk-sd: Re-store SDIO IRQs mask at system resume
  2019-09-09 22:29   ` Doug Anderson
@ 2019-09-11 12:06     ` Ulf Hansson
  0 siblings, 0 replies; 22+ messages in thread
From: Ulf Hansson @ 2019-09-11 12:06 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Linux MMC List, Adrian Hunter, Matthias Kaehlcke, Shawn Lin,
	Jaehoon Chung, Yong Mao, Chaotian Jing, LKML, Nicolas Boichat,
	Daniel Kurtz

On Tue, 10 Sep 2019 at 00:29, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Sun, Sep 8, 2019 at 3:12 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > In cases when SDIO IRQs have been enabled, runtime suspend is prevented by
> > the driver. However, this still means msdc_runtime_suspend|resume() gets
> > called during system suspend/resume, via pm_runtime_force_suspend|resume().
> >
> > This means during system suspend/resume, the register context of the mtk-sd
> > device most likely loses its register context, even in cases when SDIO IRQs
> > have been enabled.
> >
> > To re-enable the SDIO IRQs during system resume, the mtk-sd driver
> > currently relies on the mmc core to re-enable the SDIO IRQs when it resumes
> > the SDIO card, but this isn't the recommended solution. Instead, it's
> > better to deal with this locally in the mtk-sd driver, so let's do that.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  drivers/mmc/host/mtk-sd.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> > index 6946bb040a28..ae7688098b7b 100644
> > --- a/drivers/mmc/host/mtk-sd.c
> > +++ b/drivers/mmc/host/mtk-sd.c
> > @@ -2408,6 +2408,9 @@ static void msdc_save_reg(struct msdc_host *host)
> >         } else {
> >                 host->save_para.pad_tune = readl(host->base + tune_reg);
> >         }
> > +
> > +       if (sdio_irq_claimed(host->mmc))
> > +               __msdc_enable_sdio_irq(host, 1);
> >  }
> >
> >  static void msdc_restore_reg(struct msdc_host *host)
>
> I don't personally have a Mediatek device setup to test this patch on.
> If it's super urgent I could try to track down one and try to set it
> up, but hopefully it's easier for someone else...
>
> That being said, from code inspection it seems like you should be
> adding your code to msdc_restore_reg(), not to msdc_save_reg().  Am I
> confused?

No, you are absolutely correct, my bad. Thanks a lot for spotting this!

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 00/11] mmc: core: PM fixes/improvements for SDIO IRQs
  2019-09-08 10:12 [PATCH v2 00/11] mmc: core: PM fixes/improvements for SDIO IRQs Ulf Hansson
                   ` (10 preceding siblings ...)
  2019-09-08 10:12 ` [PATCH v2 11/11] mmc: sdhci: Convert to use sdio_irq_claimed() Ulf Hansson
@ 2019-09-11 14:21 ` Ulf Hansson
  11 siblings, 0 replies; 22+ messages in thread
From: Ulf Hansson @ 2019-09-11 14:21 UTC (permalink / raw)
  To: linux-mmc, Ulf Hansson, Adrian Hunter, Douglas Anderson,
	Matthias Kaehlcke
  Cc: Shawn Lin, Jaehoon Chung, Yong Mao, Chaotian Jing,
	Linux Kernel Mailing List

On Sun, 8 Sep 2019 at 12:12, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> Changes in v2:
>         - Added reviewed/tested-by tags.
>         - Updated some changelogs.
>         - Renamed sdio_irq_enabled() to sdio_irq_claimed().
>         - Don't set sdio_irq_pending when resuming SDIO card, but just queue the
>         work.
>
> The power management support for SDIO cards have slowly been improved, but
> there are still quite some serious problems, especially when dealing with the
> so called SDIO IRQs during system suspend/resume.
>
> This series makes some additional improvements to this code in the mmc core,
> but also includes some needed adaptations for the sdhci, the dw_mmc and the
> mtk-sd host drivers.
>
> The series is also available at:
> git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git sdio_irq_suspend_next_v2
>
> Kind regards
> Uffe
>
>
> Matthias Kaehlcke (1):
>   mmc: core: Move code to get pending SDIO IRQs to a function
>
> Ulf Hansson (10):
>   mmc: core: Add helper function to indicate if SDIO IRQs is enabled
>   mmc: dw_mmc: Re-store SDIO IRQs mask at system resume
>   mmc: mtk-sd: Re-store SDIO IRQs mask at system resume
>   mmc: core: Clarify sdio_irq_pending flag for
>     MMC_CAP2_SDIO_IRQ_NOTHREAD
>   mmc: core: Clarify that the ->ack_sdio_irq() callback is mandatory
>   mmc: core: WARN if SDIO IRQs are enabled for non-powered card in
>     suspend
>   mmc: core: Fixup processing of SDIO IRQs during system suspend/resume
>   mmc: sdhci: Drop redundant check in sdhci_ack_sdio_irq()
>   mmc: sdhci: Drop redundant code for SDIO IRQs
>   mmc: sdhci: Convert to use sdio_irq_claimed()
>
>  drivers/mmc/core/sdio.c            |  4 ++-
>  drivers/mmc/core/sdio_irq.c        | 57 +++++++++++++++++++-----------
>  drivers/mmc/host/dw_mmc.c          |  4 +++
>  drivers/mmc/host/mtk-sd.c          |  3 ++
>  drivers/mmc/host/sdhci-esdhc-imx.c | 34 ++++++++----------
>  drivers/mmc/host/sdhci.c           | 12 ++-----
>  drivers/mmc/host/sdhci.h           |  6 ----
>  include/linux/mmc/host.h           | 10 ++++++
>  8 files changed, 75 insertions(+), 55 deletions(-)
>
> --
> 2.17.1
>

So, I have queued this up for next (using v3 of patch 3), thanks all
for reviewing and testing!

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2019-09-11 14:21 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-08 10:12 [PATCH v2 00/11] mmc: core: PM fixes/improvements for SDIO IRQs Ulf Hansson
2019-09-08 10:12 ` [PATCH v2 01/11] mmc: core: Add helper function to indicate if SDIO IRQs is enabled Ulf Hansson
2019-09-09 22:32   ` Doug Anderson
2019-09-08 10:12 ` [PATCH v2 02/11] mmc: dw_mmc: Re-store SDIO IRQs mask at system resume Ulf Hansson
2019-09-09 22:32   ` Doug Anderson
2019-09-08 10:12 ` [PATCH v2 03/11] mmc: mtk-sd: " Ulf Hansson
2019-09-09 22:29   ` Doug Anderson
2019-09-11 12:06     ` Ulf Hansson
2019-09-08 10:12 ` [PATCH v2 04/11] mmc: core: Move code to get pending SDIO IRQs to a function Ulf Hansson
2019-09-09 22:32   ` Doug Anderson
2019-09-08 10:12 ` [PATCH v2 05/11] mmc: core: Clarify sdio_irq_pending flag for MMC_CAP2_SDIO_IRQ_NOTHREAD Ulf Hansson
2019-09-09 22:33   ` Doug Anderson
2019-09-08 10:12 ` [PATCH v2 06/11] mmc: core: Clarify that the ->ack_sdio_irq() callback is mandatory Ulf Hansson
2019-09-09 22:34   ` Doug Anderson
2019-09-08 10:12 ` [PATCH v2 07/11] mmc: core: WARN if SDIO IRQs are enabled for non-powered card in suspend Ulf Hansson
2019-09-08 10:12 ` [PATCH v2 08/11] mmc: core: Fixup processing of SDIO IRQs during system suspend/resume Ulf Hansson
2019-09-09 22:34   ` Doug Anderson
2019-09-08 10:12 ` [PATCH v2 09/11] mmc: sdhci: Drop redundant check in sdhci_ack_sdio_irq() Ulf Hansson
2019-09-09 11:19   ` Adrian Hunter
2019-09-08 10:12 ` [PATCH v2 10/11] mmc: sdhci: Drop redundant code for SDIO IRQs Ulf Hansson
2019-09-08 10:12 ` [PATCH v2 11/11] mmc: sdhci: Convert to use sdio_irq_claimed() Ulf Hansson
2019-09-11 14:21 ` [PATCH v2 00/11] mmc: core: PM fixes/improvements for SDIO IRQs Ulf Hansson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).