[PATCH V2 00/15] mmc: host: Add facility to support re-tuning

[PATCH V2 00/15] mmc: host: Add facility to support re-tuning

[Adrian Hunter]

Hi

Here is V2 of some patches to move re-tuning support
out of sdhci and into the core, and add support for HS400
re-tuning.

Currently sdhci does re-tuning transparently by
calling sdhci_execute_tuning() from its ->request()
function.

The problem with HS400 re-tuning is that it must be
done in HS200 mode. That means using switch commands
and making ios changes. That means it potentially
conflicts with other command sequences. The new
re-tuning support accomodates that, however it is
not strictly necessary because HS400 re-tuning
should only be needed after runtime suspend when
there would not be a conflict.

Nevertheless, this approach has more structure
and potentially more flexibility.

Changes in V2:

    Added support to the block driver for re-tuning
    and retrying after a CRC error. The host driver
    is left to decide when an error indicates re-tuning
    is needed. The block driver will retry a data request
    once if re-tuning is flagged as needed.

    SDIO drivers need not be aware of re-tuning because
    retrying will anyway cause re-tuning when re-tuning
    is flagged as needed. Nevertheless SDIO drivers could
    use the need_retune flag to instigate a retry when
    otherwise they might not have.

    mmc: core: Simplify by adding mmc_execute_tuning()
        Dropped because it has been applied

    mmc: host: Add facility to support re-tuning
        Renamed mmc_retune_retry() to mmc_retune_recheck()
        to better reflect what it does.

    mmc: core: Move mmc_card_removed() into mmc_start_request()
        Dropped because it has been applied

    mmc: core: Add support for re-tuning before each request
        Fixed un-balanced re-tune hold / release

    mmc: sdhci: Always init buf_ready_int
        Dropped because it has been applied

    mmc: core: Separate out the mmc_switch status check so it can be re-used
        New patch

    mmc: core: Add support for HS400 re-tuning
        It was found that that the original code was not reliable
        after a CRC error. The problem was that the CMD13 after a
        switch was faiing. So the code was changed to check the
        switch status *after* changing the I/O state to match the
        switch i.e. the new I/O state is the correct one to use
        after a switch.

    mmc: sdhci: Flag re-tuning is needed on CRC or End-Bit errors
        New patch

    mmc: block: Check re-tuning in the recovery path
        New patch

    mmc: block: Retry data requests when re-tuning is needed
        New patch

    mmc: core: Don't print reset warning if reset is not supported
        New patch


Adrian Hunter (15):
      mmc: host: Add facility to support re-tuning
      mmc: core: Disable re-tuning when card is no longer initialized
      mmc: core: Add support for re-tuning before each request
      mmc: core: Check re-tuning before retrying
      mmc: core: Hold re-tuning during switch commands
      mmc: core: Hold re-tuning during erase commands
      mmc: core: Hold re-tuning while bkops ongoing
      mmc: mmc: Comment that callers need to hold re-tuning if the card is put to sleep
      mmc: core: Separate out the mmc_switch status check so it can be re-used
      mmc: core: Add support for HS400 re-tuning
      mmc: sdhci: Change to new way of doing re-tuning
      mmc: sdhci: Flag re-tuning is needed on CRC or End-Bit errors
      mmc: block: Check re-tuning in the recovery path
      mmc: block: Retry data requests when re-tuning is needed
      mmc: core: Don't print reset warning if reset is not supported

 drivers/mmc/card/block.c   |  19 ++++++-
 drivers/mmc/card/queue.h   |   1 +
 drivers/mmc/core/core.c    |  51 ++++++++++++++++--
 drivers/mmc/core/core.h    |   2 +
 drivers/mmc/core/host.c    |  63 ++++++++++++++++++++++
 drivers/mmc/core/mmc.c     |  87 +++++++++++++++++++++++++++++++
 drivers/mmc/core/mmc_ops.c |  45 ++++++++++------
 drivers/mmc/core/mmc_ops.h |   1 +
 drivers/mmc/host/sdhci.c   | 127 ++++++++-------------------------------------
 include/linux/mmc/host.h   |  58 +++++++++++++++++++++
 include/linux/mmc/sdhci.h  |   3 --
 11 files changed, 328 insertions(+), 129 deletions(-)


Regards
Adrian

[PATCH V2 01/15] mmc: host: Add facility to support re-tuning

[Adrian Hunter]

Currently, there is core support for tuning during
initialization. There can also be a need to re-tune
periodically (e.g. sdhci) or to re-tune after the
host controller is powered off (e.g. after PM
runtime suspend / resume) or to re-tune in response
to CRC errors.

The main requirements for re-tuning are:
  - ability to enable /disable re-tuning
  - ability to flag that re-tuning is needed
  - ability to re-tune before any request
  - ability to hold off re-tuning if the card is busy
  - ability to hold off re-tuning if re-tuning is in
  progress
  - ability to run a re-tuning timer

To support those requirements 5 members are added to struct
mmc_host:

  unsigned int		can_retune:1;	/* re-tuning can be used */
  unsigned int		doing_retune:1;	/* re-tuning in progress */
  int			need_retune;	/* re-tuning is needed */
  int			hold_retune;	/* hold off re-tuning */
  struct timer_list	retune_timer;	/* for periodic re-tuning */

need_retune is an integer so it can be set without needing
synchronization. hold_retune is a integer to allow nesting.

Various simple functions are provided to set / clear those
variables.

Subsequent patches take those functions into use.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/host.c  | 46 ++++++++++++++++++++++++++++++++++++++
 include/linux/mmc/host.h | 58 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 104 insertions(+)

diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 8be0df7..221d46d 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -301,6 +301,51 @@ static inline void mmc_host_clk_sysfs_init(struct mmc_host *host)
 
 #endif
 
+void mmc_retune_enable(struct mmc_host *host, unsigned int period)
+{
+	host->can_retune = 1;
+	if (period)
+		mod_timer(&host->retune_timer, jiffies + period * HZ);
+}
+
+void mmc_retune_disable(struct mmc_host *host)
+{
+	host->can_retune = 0;
+	del_timer_sync(&host->retune_timer);
+	host->need_retune = 0;
+}
+
+void mmc_retune_timer_stop(struct mmc_host *host)
+{
+	del_timer_sync(&host->retune_timer);
+}
+
+int mmc_retune(struct mmc_host *host)
+{
+	int err;
+
+	if (!host->need_retune || host->doing_retune || host->hold_retune ||
+	    !host->card)
+		return 0;
+
+	host->need_retune = 0;
+
+	host->doing_retune = 1;
+
+	err = mmc_execute_tuning(host->card);
+
+	host->doing_retune = 0;
+
+	return err;
+}
+
+static void mmc_retune_timer(unsigned long data)
+{
+	struct mmc_host *host = (struct mmc_host *)data;
+
+	mmc_retune_needed(host);
+}
+
 /**
  *	mmc_of_parse() - parse host's device-tree node
  *	@host: host whose node should be parsed.
@@ -504,6 +549,7 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
 #ifdef CONFIG_PM
 	host->pm_notify.notifier_call = mmc_pm_notify;
 #endif
+	setup_timer(&host->retune_timer, mmc_retune_timer, (unsigned long)host);
 
 	/*
 	 * By default, hosts do not support SGIO or large requests.
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 0c8cbe5..e9a7470 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -12,6 +12,7 @@
 
 #include <linux/leds.h>
 #include <linux/mutex.h>
+#include <linux/timer.h>
 #include <linux/sched.h>
 #include <linux/device.h>
 #include <linux/fault-inject.h>
@@ -327,10 +328,16 @@ struct mmc_host {
 #ifdef CONFIG_MMC_DEBUG
 	unsigned int		removed:1;	/* host is being removed */
 #endif
+	unsigned int		can_retune:1;	/* re-tuning can be used */
+	unsigned int		doing_retune:1;	/* re-tuning in progress */
 
 	int			rescan_disable;	/* disable card detection */
 	int			rescan_entered;	/* used with nonremovable devices */
 
+	int			need_retune;	/* re-tuning is needed */
+	int			hold_retune;	/* hold off re-tuning */
+	struct timer_list	retune_timer;	/* for periodic re-tuning */
+
 	bool			trigger_card_event; /* card_event necessary */
 
 	struct mmc_card		*card;		/* device attached to this host */
@@ -519,4 +526,55 @@ static inline bool mmc_card_hs400(struct mmc_card *card)
 	return card->host->ios.timing == MMC_TIMING_MMC_HS400;
 }
 
+void mmc_retune_enable(struct mmc_host *host, unsigned int period);
+void mmc_retune_disable(struct mmc_host *host);
+void mmc_retune_timer_stop(struct mmc_host *host);
+int mmc_retune(struct mmc_host *host);
+
+static inline void mmc_retune_needed(struct mmc_host *host)
+{
+	if (host->can_retune)
+		host->need_retune = 1;
+}
+
+static inline void mmc_retune_not_needed(struct mmc_host *host)
+{
+	host->need_retune = 0;
+}
+
+static inline void mmc_retune_hold(struct mmc_host *host)
+{
+	host->hold_retune += 1;
+}
+
+static inline void mmc_retune_release(struct mmc_host *host)
+{
+	if (host->hold_retune)
+		host->hold_retune -= 1;
+	else
+		WARN_ON(1);
+}
+
+static inline int mmc_retune_and_hold(struct mmc_host *host)
+{
+	int err;
+
+	err = mmc_retune(host);
+	if (!err)
+		mmc_retune_hold(host);
+
+	return err;
+}
+
+static inline int mmc_retune_recheck(struct mmc_host *host)
+{
+	int err;
+
+	mmc_retune_release(host);
+	err = mmc_retune(host);
+	mmc_retune_hold(host);
+
+	return err;
+}
+
 #endif /* LINUX_MMC_HOST_H */
-- 
1.9.1

[PATCH V2 02/15] mmc: core: Disable re-tuning when card is no longer initialized

[Adrian Hunter]

Re-tuning is done before a request is sent to the card.
Host controller drivers can choose to enable re-tuning
when tuning is done during card initialization. To
ensure that re-tuning gets disabled, add disabling to
mmc_set_initial_state() which is called whenever the
card is powered on, off, or reset.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 1be7055..e9ab5ff 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1137,6 +1137,8 @@ void mmc_set_bus_width(struct mmc_host *host, unsigned int width)
  */
 void mmc_set_initial_state(struct mmc_host *host)
 {
+	mmc_retune_disable(host);
+
 	if (mmc_host_is_spi(host))
 		host->ios.chip_select = MMC_CS_HIGH;
 	else
-- 
1.9.1

[PATCH V2 03/15] mmc: core: Add support for re-tuning before each request

[Adrian Hunter]

At the start of each request, re-tune if needed and
then hold off re-tuning again until the request is done.

Note that though there is one function that starts
requests (mmc_start_request) there are two that wait for
the request to be done (mmc_wait_for_req_done and
mmc_wait_for_data_req_done).  Also note that
mmc_wait_for_data_req_done can return even when the
request is not done (which allows the block driver
to prepare a newly arrived request while still
waiting for the previous request).

This patch ensures re-tuning is held for the duration
of a request.  Subsequent patches will also hold
re-tuning at other times when it might cause a
conflict.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/core.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index e9ab5ff..6dad56d 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -192,9 +192,21 @@ static int mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
 	unsigned int i, sz;
 	struct scatterlist *sg;
 #endif
+	int err;
+
+	mmc_retune_hold(host);
+
 	if (mmc_card_removed(host->card))
 		return -ENOMEDIUM;
 
+	err = mmc_retune_recheck(host);
+	if (err) {
+		/* Retry path expects the clock to be held */
+		if (mrq->cmd->retries)
+			mmc_host_clk_hold(host);
+		return err;
+	}
+
 	if (mrq->sbc) {
 		pr_debug("<%s: starting CMD%u arg %08x flags %08x>\n",
 			 mmc_hostname(host), mrq->sbc->opcode,
@@ -427,12 +439,11 @@ static int mmc_wait_for_data_req_done(struct mmc_host *host,
 			}
 		} else if (context_info->is_new_req) {
 			context_info->is_new_req = false;
-			if (!next_req) {
-				err = MMC_BLK_NEW_REQUEST;
-				break; /* return err */
-			}
+			if (!next_req)
+				return MMC_BLK_NEW_REQUEST;
 		}
 	}
+	mmc_retune_release(host);
 	return err;
 }
 
@@ -473,6 +484,8 @@ static void mmc_wait_for_req_done(struct mmc_host *host,
 		cmd->error = 0;
 		host->ops->request(host, mrq);
 	}
+
+	mmc_retune_release(host);
 }
 
 /**
-- 
1.9.1

[PATCH V2 04/15] mmc: core: Check re-tuning before retrying

[Adrian Hunter]

Possibly a command is failing because re-tuning is needed.
Use mmc_retune_recheck() to check re-tuning. At that point
re-tuning is held, at least by the request, so
mmc_retune_recheck() does a mmc_retune_release() and
mmc_retune_hold().

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/core.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 6dad56d..f1d663e 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -429,6 +429,11 @@ static int mmc_wait_for_data_req_done(struct mmc_host *host,
 							    host->areq);
 				break; /* return err */
 			} else {
+				err = mmc_retune_recheck(host);
+				if (err) {
+					mmc_host_clk_release(host);
+					break;
+				}
 				pr_info("%s: req failed (CMD%u): %d, retrying...\n",
 					mmc_hostname(host),
 					cmd->opcode, cmd->error);
@@ -451,6 +456,7 @@ static void mmc_wait_for_req_done(struct mmc_host *host,
 				  struct mmc_request *mrq)
 {
 	struct mmc_command *cmd;
+	int err;
 
 	while (1) {
 		wait_for_completion(&mrq->completion);
@@ -478,6 +484,12 @@ static void mmc_wait_for_req_done(struct mmc_host *host,
 		    mmc_card_removed(host->card))
 			break;
 
+		err = mmc_retune_recheck(host);
+		if (err) {
+			mmc_host_clk_release(host);
+			break;
+		}
+
 		pr_debug("%s: req failed (CMD%u): %d, retrying...\n",
 			 mmc_hostname(host), cmd->opcode, cmd->error);
 		cmd->retries--;
-- 
1.9.1

[PATCH V2 05/15] mmc: core: Hold re-tuning during switch commands

[Adrian Hunter]

Hold re-tuning during switch commands to prevent
it from conflicting with the busy state or the CMD13
verification.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/mmc_ops.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 0ea042d..9fbf39c 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -474,6 +474,10 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 	u32 status = 0;
 	bool use_r1b_resp = use_busy_signal;
 
+	err = mmc_retune_and_hold(host);
+	if (err)
+		return err;
+
 	/*
 	 * If the cmd timeout and the max_busy_timeout of the host are both
 	 * specified, let's validate them. A failure means we need to prevent
@@ -506,11 +510,11 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 
 	err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES);
 	if (err)
-		return err;
+		goto out;
 
 	/* No need to check card status in case of unblocking command */
 	if (!use_busy_signal)
-		return 0;
+		goto out;
 
 	/*
 	 * CRC errors shall only be ignored in cases were CMD13 is used to poll
@@ -529,7 +533,7 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		if (send_status) {
 			err = __mmc_send_status(card, &status, ignore_crc);
 			if (err)
-				return err;
+				goto out;
 		}
 		if ((host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp)
 			break;
@@ -543,29 +547,36 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		 */
 		if (!send_status) {
 			mmc_delay(timeout_ms);
-			return 0;
+			goto out;
 		}
 
 		/* Timeout if the device never leaves the program state. */
 		if (time_after(jiffies, timeout)) {
 			pr_err("%s: Card stuck in programming state! %s\n",
 				mmc_hostname(host), __func__);
-			return -ETIMEDOUT;
+			err = -ETIMEDOUT;
+			goto out;
 		}
 	} while (R1_CURRENT_STATE(status) == R1_STATE_PRG);
 
 	if (mmc_host_is_spi(host)) {
-		if (status & R1_SPI_ILLEGAL_COMMAND)
-			return -EBADMSG;
+		if (status & R1_SPI_ILLEGAL_COMMAND) {
+			err = -EBADMSG;
+			goto out;
+		}
 	} else {
 		if (status & 0xFDFFA000)
 			pr_warn("%s: unexpected status %#x after switch\n",
 				mmc_hostname(host), status);
-		if (status & R1_SWITCH_ERROR)
-			return -EBADMSG;
+		if (status & R1_SWITCH_ERROR) {
+			err = -EBADMSG;
+			goto out;
+		}
 	}
+out:
+	mmc_retune_release(host);
 
-	return 0;
+	return err;
 }
 EXPORT_SYMBOL_GPL(__mmc_switch);
 
-- 
1.9.1

[PATCH V2 06/15] mmc: core: Hold re-tuning during erase commands

[Adrian Hunter]

Hold re-tuning during erase commands to prevent
it from conflicting with the sequence of commands.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index f1d663e..91838ea 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1994,6 +1994,10 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from,
 	unsigned long timeout;
 	int err;
 
+	err = mmc_retune_and_hold(card->host);
+	if (err)
+		return err;
+
 	/*
 	 * qty is used to calculate the erase timeout which depends on how many
 	 * erase groups (or allocation units in SD terminology) are affected.
@@ -2097,6 +2101,7 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from,
 	} while (!(cmd.resp[0] & R1_READY_FOR_DATA) ||
 		 (R1_CURRENT_STATE(cmd.resp[0]) == R1_STATE_PRG));
 out:
+	mmc_retune_release(card->host);
 	return err;
 }
 
-- 
1.9.1

[PATCH V2 07/15] mmc: core: Hold re-tuning while bkops ongoing

[Adrian Hunter]

Hold re-tuning during bkops to prevent
it from conflicting with the busy state.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 91838ea..392a150 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -311,6 +311,10 @@ void mmc_start_bkops(struct mmc_card *card, bool from_exception)
 	} else {
 		timeout = 0;
 		use_busy_signal = false;
+		/* Hold re-tuning for ongoing bkops */
+		err = mmc_retune_and_hold(card->host);
+		if (err)
+			goto out;
 	}
 
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
@@ -319,6 +323,9 @@ void mmc_start_bkops(struct mmc_card *card, bool from_exception)
 	if (err) {
 		pr_warn("%s: Error %d starting bkops\n",
 			mmc_hostname(card->host), err);
+		/* bkops not ongoing, so release re-tuning */
+		if (!use_busy_signal)
+			mmc_retune_release(card->host);
 		goto out;
 	}
 
@@ -753,6 +760,7 @@ int mmc_stop_bkops(struct mmc_card *card)
 	 */
 	if (!err || (err == -EINVAL)) {
 		mmc_card_clr_doing_bkops(card);
+		mmc_retune_release(card->host);
 		err = 0;
 	}
 
-- 
1.9.1

[PATCH V2 08/15] mmc: mmc: Comment that callers need to hold re-tuning if the card is put to sleep

[Adrian Hunter]

Currently "mmc sleep" is only used before power off and
is not paired with waking up.  If that ever changed,
re-tuning might need to be held, so add a comment for that.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/mmc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 1fc48a2..d4156e9 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1494,6 +1494,7 @@ static int mmc_can_sleep(struct mmc_card *card)
 	return (card && card->ext_csd.rev >= 3);
 }
 
+/* If necessary, callers must hold re-tuning */
 static int mmc_sleep(struct mmc_host *host)
 {
 	struct mmc_command cmd = {0};
-- 
1.9.1

[PATCH V2 09/15] mmc: core: Separate out the mmc_switch status check so it can be re-used

[Adrian Hunter]

Make a separate function to do the mmc_switch status check
so it can be re-used. This is preparation for adding support
for HS400 re-tuning.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/mmc_ops.c | 30 ++++++++++++++++--------------
 drivers/mmc/core/mmc_ops.h |  1 +
 2 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 9fbf39c..976c2c7 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -449,6 +449,21 @@ int mmc_spi_set_crc(struct mmc_host *host, int use_crc)
 	return err;
 }
 
+int mmc_switch_status_error(struct mmc_host *host, u32 status)
+{
+	if (mmc_host_is_spi(host)) {
+		if (status & R1_SPI_ILLEGAL_COMMAND)
+			return -EBADMSG;
+	} else {
+		if (status & 0xFDFFA000)
+			pr_warn("%s: unexpected status %#x after switch\n",
+				mmc_hostname(host), status);
+		if (status & R1_SWITCH_ERROR)
+			return -EBADMSG;
+	}
+	return 0;
+}
+
 /**
  *	__mmc_switch - modify EXT_CSD register
  *	@card: the MMC card associated with the data transfer
@@ -559,20 +574,7 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		}
 	} while (R1_CURRENT_STATE(status) == R1_STATE_PRG);
 
-	if (mmc_host_is_spi(host)) {
-		if (status & R1_SPI_ILLEGAL_COMMAND) {
-			err = -EBADMSG;
-			goto out;
-		}
-	} else {
-		if (status & 0xFDFFA000)
-			pr_warn("%s: unexpected status %#x after switch\n",
-				mmc_hostname(host), status);
-		if (status & R1_SWITCH_ERROR) {
-			err = -EBADMSG;
-			goto out;
-		}
-	}
+	err = mmc_switch_status_error(host, status);
 out:
 	mmc_retune_release(host);
 
diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
index 6f4b00e..f498f9a 100644
--- a/drivers/mmc/core/mmc_ops.h
+++ b/drivers/mmc/core/mmc_ops.h
@@ -27,6 +27,7 @@ int mmc_spi_set_crc(struct mmc_host *host, int use_crc);
 int mmc_bus_test(struct mmc_card *card, u8 bus_width);
 int mmc_send_hpi_cmd(struct mmc_card *card, u32 *status);
 int mmc_can_ext_csd(struct mmc_card *card);
+int mmc_switch_status_error(struct mmc_host *host, u32 status);
 
 #endif
 
-- 
1.9.1

[PATCH V2 10/15] mmc: core: Add support for HS400 re-tuning

[Adrian Hunter]

HS400 re-tuning must be done in HS200 mode. Add
the ability to switch from HS400 mode to HS200
mode before re-tuning and switch back to HS400
after re-tuning.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/core.h |  2 ++
 drivers/mmc/core/host.c | 17 ++++++++++
 drivers/mmc/core/mmc.c  | 86 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 105 insertions(+)

diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index cfba3c0..e6f2de7 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -88,6 +88,8 @@ void mmc_remove_card_debugfs(struct mmc_card *card);
 void mmc_init_context_info(struct mmc_host *host);
 
 int mmc_execute_tuning(struct mmc_card *card);
+int mmc_hs200_to_hs400(struct mmc_card *card);
+int mmc_hs400_to_hs200(struct mmc_card *card);
 
 #endif
 
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 221d46d..20354a5 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -322,6 +322,7 @@ void mmc_retune_timer_stop(struct mmc_host *host)
 
 int mmc_retune(struct mmc_host *host)
 {
+	bool return_to_hs400 = false;
 	int err;
 
 	if (!host->need_retune || host->doing_retune || host->hold_retune ||
@@ -332,8 +333,24 @@ int mmc_retune(struct mmc_host *host)
 
 	host->doing_retune = 1;
 
+	if (host->ios.timing == MMC_TIMING_MMC_HS400) {
+		err = mmc_hs400_to_hs200(host->card);
+		if (err)
+			goto out;
+
+		return_to_hs400 = true;
+
+		if (host->ops->prepare_hs400_tuning)
+			host->ops->prepare_hs400_tuning(host, &host->ios);
+	}
+
 	err = mmc_execute_tuning(host->card);
+	if (err)
+		goto out;
 
+	if (return_to_hs400)
+		err = mmc_hs200_to_hs400(host->card);
+out:
 	host->doing_retune = 0;
 
 	return err;
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index d4156e9..e35701a 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1081,6 +1081,92 @@ static int mmc_select_hs400(struct mmc_card *card)
 	return 0;
 }
 
+int mmc_hs200_to_hs400(struct mmc_card *card)
+{
+	return mmc_select_hs400(card);
+}
+
+/* Caller must hold re-tuning */
+static int mmc_switch_status(struct mmc_card *card)
+{
+	u32 status;
+	int err;
+
+	err = mmc_send_status(card, &status);
+	if (err)
+		return err;
+
+	return mmc_switch_status_error(card->host, status);
+}
+
+int mmc_hs400_to_hs200(struct mmc_card *card)
+{
+	struct mmc_host *host = card->host;
+	bool send_status = true;
+	unsigned int max_dtr;
+	int err;
+
+	if (host->caps & MMC_CAP_WAIT_WHILE_BUSY)
+		send_status = false;
+
+	/* Reduce frequency to HS */
+	max_dtr = card->ext_csd.hs_max_dtr;
+	mmc_set_clock(host, max_dtr);
+
+	/* Switch HS400 to HS DDR */
+	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING,
+			   EXT_CSD_TIMING_HS, card->ext_csd.generic_cmd6_time,
+			   true, send_status, true);
+	if (err)
+		goto out_err;
+
+	mmc_set_timing(host, MMC_TIMING_MMC_DDR52);
+
+	if (!send_status) {
+		err = mmc_switch_status(card);
+		if (err)
+			goto out_err;
+	}
+
+	/* Switch HS DDR to HS */
+	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BUS_WIDTH,
+			   EXT_CSD_BUS_WIDTH_8, card->ext_csd.generic_cmd6_time,
+			   true, send_status, true);
+	if (err)
+		goto out_err;
+
+	mmc_set_timing(host, MMC_TIMING_MMC_HS);
+
+	if (!send_status) {
+		err = mmc_switch_status(card);
+		if (err)
+			goto out_err;
+	}
+
+	/* Switch HS to HS200 */
+	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING,
+			   EXT_CSD_TIMING_HS200,
+			   card->ext_csd.generic_cmd6_time, true, send_status,
+			   true);
+	if (err)
+		goto out_err;
+
+	mmc_set_timing(host, MMC_TIMING_MMC_HS200);
+
+	if (!send_status) {
+		err = mmc_switch_status(card);
+		if (err)
+			goto out_err;
+	}
+
+	return 0;
+
+out_err:
+	pr_err("%s: %s failed, error %d\n", mmc_hostname(card->host),
+	       __func__, err);
+	return err;
+}
+
 /*
  * For device supporting HS200 mode, the following sequence
  * should be done before executing the tuning process.
-- 
1.9.1

[PATCH V3 10/15] mmc: core: Add support for HS400 re-tuning

[Adrian Hunter]

HS400 re-tuning must be done in HS200 mode. Add
the ability to switch from HS400 mode to HS200
mode before re-tuning and switch back to HS400
after re-tuning.

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


V3:
	Remember to mmc_set_bus_speed(card) in mmc_hs400_to_hs200()


 drivers/mmc/core/core.h |  2 ++
 drivers/mmc/core/host.c | 17 ++++++++++
 drivers/mmc/core/mmc.c  | 88 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 107 insertions(+)

diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index cfba3c0..e6f2de7 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -88,6 +88,8 @@ void mmc_remove_card_debugfs(struct mmc_card *card);
 void mmc_init_context_info(struct mmc_host *host);
 
 int mmc_execute_tuning(struct mmc_card *card);
+int mmc_hs200_to_hs400(struct mmc_card *card);
+int mmc_hs400_to_hs200(struct mmc_card *card);
 
 #endif
 
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 221d46d..20354a5 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -322,6 +322,7 @@ void mmc_retune_timer_stop(struct mmc_host *host)
 
 int mmc_retune(struct mmc_host *host)
 {
+	bool return_to_hs400 = false;
 	int err;
 
 	if (!host->need_retune || host->doing_retune || host->hold_retune ||
@@ -332,8 +333,24 @@ int mmc_retune(struct mmc_host *host)
 
 	host->doing_retune = 1;
 
+	if (host->ios.timing == MMC_TIMING_MMC_HS400) {
+		err = mmc_hs400_to_hs200(host->card);
+		if (err)
+			goto out;
+
+		return_to_hs400 = true;
+
+		if (host->ops->prepare_hs400_tuning)
+			host->ops->prepare_hs400_tuning(host, &host->ios);
+	}
+
 	err = mmc_execute_tuning(host->card);
+	if (err)
+		goto out;
 
+	if (return_to_hs400)
+		err = mmc_hs200_to_hs400(host->card);
+out:
 	host->doing_retune = 0;
 
 	return err;
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 813b02a..03577be 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1083,6 +1083,94 @@ static int mmc_select_hs400(struct mmc_card *card)
 	return 0;
 }
 
+int mmc_hs200_to_hs400(struct mmc_card *card)
+{
+	return mmc_select_hs400(card);
+}
+
+/* Caller must hold re-tuning */
+static int mmc_switch_status(struct mmc_card *card)
+{
+	u32 status;
+	int err;
+
+	err = mmc_send_status(card, &status);
+	if (err)
+		return err;
+
+	return mmc_switch_status_error(card->host, status);
+}
+
+int mmc_hs400_to_hs200(struct mmc_card *card)
+{
+	struct mmc_host *host = card->host;
+	bool send_status = true;
+	unsigned int max_dtr;
+	int err;
+
+	if (host->caps & MMC_CAP_WAIT_WHILE_BUSY)
+		send_status = false;
+
+	/* Reduce frequency to HS */
+	max_dtr = card->ext_csd.hs_max_dtr;
+	mmc_set_clock(host, max_dtr);
+
+	/* Switch HS400 to HS DDR */
+	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING,
+			   EXT_CSD_TIMING_HS, card->ext_csd.generic_cmd6_time,
+			   true, send_status, true);
+	if (err)
+		goto out_err;
+
+	mmc_set_timing(host, MMC_TIMING_MMC_DDR52);
+
+	if (!send_status) {
+		err = mmc_switch_status(card);
+		if (err)
+			goto out_err;
+	}
+
+	/* Switch HS DDR to HS */
+	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BUS_WIDTH,
+			   EXT_CSD_BUS_WIDTH_8, card->ext_csd.generic_cmd6_time,
+			   true, send_status, true);
+	if (err)
+		goto out_err;
+
+	mmc_set_timing(host, MMC_TIMING_MMC_HS);
+
+	if (!send_status) {
+		err = mmc_switch_status(card);
+		if (err)
+			goto out_err;
+	}
+
+	/* Switch HS to HS200 */
+	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING,
+			   EXT_CSD_TIMING_HS200,
+			   card->ext_csd.generic_cmd6_time, true, send_status,
+			   true);
+	if (err)
+		goto out_err;
+
+	mmc_set_timing(host, MMC_TIMING_MMC_HS200);
+
+	if (!send_status) {
+		err = mmc_switch_status(card);
+		if (err)
+			goto out_err;
+	}
+
+	mmc_set_bus_speed(card);
+
+	return 0;
+
+out_err:
+	pr_err("%s: %s failed, error %d\n", mmc_hostname(card->host),
+	       __func__, err);
+	return err;
+}
+
 /*
  * For device supporting HS200 mode, the following sequence
  * should be done before executing the tuning process.
-- 
1.9.1

[PATCH V2 11/15] mmc: sdhci: Change to new way of doing re-tuning

[Adrian Hunter]

Make use of mmc core support for re-tuning instead
of doing it all in the sdhci driver.

This patch also changes to flag the need for re-tuning
always after runtime suspend when tuning has been used
at initialization. Previously it was only done if
the re-tuning timer was in use.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci.c  | 113 ++++++----------------------------------------
 include/linux/mmc/sdhci.h |   3 --
 2 files changed, 14 insertions(+), 102 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index c9881ca..dd0be76 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -51,7 +51,6 @@ static void sdhci_finish_data(struct sdhci_host *);
 
 static void sdhci_finish_command(struct sdhci_host *);
 static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode);
-static void sdhci_tuning_timer(unsigned long data);
 static void sdhci_enable_preset_value(struct sdhci_host *host, bool enable);
 static int sdhci_pre_dma_transfer(struct sdhci_host *host,
 					struct mmc_data *data,
@@ -252,17 +251,6 @@ static void sdhci_init(struct sdhci_host *host, int soft)
 static void sdhci_reinit(struct sdhci_host *host)
 {
 	sdhci_init(host, 0);
-	/*
-	 * Retuning stuffs are affected by different cards inserted and only
-	 * applicable to UHS-I cards. So reset these fields to their initial
-	 * value when card is removed.
-	 */
-	if (host->flags & SDHCI_USING_RETUNING_TIMER) {
-		host->flags &= ~SDHCI_USING_RETUNING_TIMER;
-
-		del_timer_sync(&host->tuning_timer);
-		host->flags &= ~SDHCI_NEEDS_RETUNING;
-	}
 	sdhci_enable_card_detection(host);
 }
 
@@ -1350,7 +1338,6 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 	struct sdhci_host *host;
 	int present;
 	unsigned long flags;
-	u32 tuning_opcode;
 
 	host = mmc_priv(mmc);
 
@@ -1399,39 +1386,6 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 		host->mrq->cmd->error = -ENOMEDIUM;
 		tasklet_schedule(&host->finish_tasklet);
 	} else {
-		u32 present_state;
-
-		present_state = sdhci_readl(host, SDHCI_PRESENT_STATE);
-		/*
-		 * Check if the re-tuning timer has already expired and there
-		 * is no on-going data transfer and DAT0 is not busy. If so,
-		 * we need to execute tuning procedure before sending command.
-		 */
-		if ((host->flags & SDHCI_NEEDS_RETUNING) &&
-		    !(present_state & (SDHCI_DOING_WRITE | SDHCI_DOING_READ)) &&
-		    (present_state & SDHCI_DATA_0_LVL_MASK)) {
-			if (mmc->card) {
-				/* eMMC uses cmd21 but sd and sdio use cmd19 */
-				tuning_opcode =
-					mmc->card->type == MMC_TYPE_MMC ?
-					MMC_SEND_TUNING_BLOCK_HS200 :
-					MMC_SEND_TUNING_BLOCK;
-
-				/* Here we need to set the host->mrq to NULL,
-				 * in case the pending finish_tasklet
-				 * finishes it incorrectly.
-				 */
-				host->mrq = NULL;
-
-				spin_unlock_irqrestore(&host->lock, flags);
-				sdhci_execute_tuning(mmc, tuning_opcode);
-				spin_lock_irqsave(&host->lock, flags);
-
-				/* Restore original mmc_request structure */
-				host->mrq = mrq;
-			}
-		}
-
 		if (mrq->sbc && !(host->flags & SDHCI_AUTO_CMD23))
 			sdhci_send_command(host, mrq->sbc);
 		else
@@ -2077,23 +2031,19 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
 	}
 
 out:
-	host->flags &= ~SDHCI_NEEDS_RETUNING;
-
 	if (tuning_count) {
-		host->flags |= SDHCI_USING_RETUNING_TIMER;
-		mod_timer(&host->tuning_timer, jiffies + tuning_count * HZ);
+		/*
+		 * In case tuning fails, host controllers which support
+		 * re-tuning can try tuning again at a later time, when the
+		 * re-tuning timer expires.  So for these controllers, we
+		 * return 0. Since there might be other controllers who do not
+		 * have this capability, we return error for them.
+		 */
+		err = 0;
 	}
 
-	/*
-	 * In case tuning fails, host controllers which support re-tuning can
-	 * try tuning again at a later time, when the re-tuning timer expires.
-	 * So for these controllers, we return 0. Since there might be other
-	 * controllers who do not have this capability, we return error for
-	 * them. SDHCI_USING_RETUNING_TIMER means the host is currently using
-	 * a retuning timer to do the retuning for the card.
-	 */
-	if (err && (host->flags & SDHCI_USING_RETUNING_TIMER))
-		err = 0;
+	if (!err)
+		mmc_retune_enable(host->mmc, tuning_count);
 
 	sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
 	sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
@@ -2349,20 +2299,6 @@ static void sdhci_timeout_timer(unsigned long data)
 	spin_unlock_irqrestore(&host->lock, flags);
 }
 
-static void sdhci_tuning_timer(unsigned long data)
-{
-	struct sdhci_host *host;
-	unsigned long flags;
-
-	host = (struct sdhci_host *)data;
-
-	spin_lock_irqsave(&host->lock, flags);
-
-	host->flags |= SDHCI_NEEDS_RETUNING;
-
-	spin_unlock_irqrestore(&host->lock, flags);
-}
-
 /*****************************************************************************\
  *                                                                           *
  * Interrupt handling                                                        *
@@ -2740,11 +2676,8 @@ int sdhci_suspend_host(struct sdhci_host *host)
 {
 	sdhci_disable_card_detection(host);
 
-	/* Disable tuning since we are suspending */
-	if (host->flags & SDHCI_USING_RETUNING_TIMER) {
-		del_timer_sync(&host->tuning_timer);
-		host->flags &= ~SDHCI_NEEDS_RETUNING;
-	}
+	mmc_retune_timer_stop(host->mmc);
+	mmc_retune_needed(host->mmc);
 
 	if (!device_may_wakeup(mmc_dev(host->mmc))) {
 		host->ier = 0;
@@ -2794,10 +2727,6 @@ int sdhci_resume_host(struct sdhci_host *host)
 
 	sdhci_enable_card_detection(host);
 
-	/* Set the re-tuning expiration flag */
-	if (host->flags & SDHCI_USING_RETUNING_TIMER)
-		host->flags |= SDHCI_NEEDS_RETUNING;
-
 	return ret;
 }
 
@@ -2834,11 +2763,8 @@ int sdhci_runtime_suspend_host(struct sdhci_host *host)
 {
 	unsigned long flags;
 
-	/* Disable tuning since we are suspending */
-	if (host->flags & SDHCI_USING_RETUNING_TIMER) {
-		del_timer_sync(&host->tuning_timer);
-		host->flags &= ~SDHCI_NEEDS_RETUNING;
-	}
+	mmc_retune_timer_stop(host->mmc);
+	mmc_retune_needed(host->mmc);
 
 	spin_lock_irqsave(&host->lock, flags);
 	host->ier &= SDHCI_INT_CARD_INT;
@@ -2881,10 +2807,6 @@ int sdhci_runtime_resume_host(struct sdhci_host *host)
 		spin_unlock_irqrestore(&host->lock, flags);
 	}
 
-	/* Set the re-tuning expiration flag */
-	if (host->flags & SDHCI_USING_RETUNING_TIMER)
-		host->flags |= SDHCI_NEEDS_RETUNING;
-
 	spin_lock_irqsave(&host->lock, flags);
 
 	host->runtime_suspended = false;
@@ -3419,13 +3341,6 @@ int sdhci_add_host(struct sdhci_host *host)
 
 	init_waitqueue_head(&host->buf_ready_int);
 
-	if (host->version >= SDHCI_SPEC_300) {
-		/* Initialize re-tuning timer */
-		init_timer(&host->tuning_timer);
-		host->tuning_timer.data = (unsigned long)host;
-		host->tuning_timer.function = sdhci_tuning_timer;
-	}
-
 	sdhci_init(host, 0);
 
 	ret = request_threaded_irq(host->irq, sdhci_irq, sdhci_thread_irq,
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index c3e3db1..c5b00be 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -138,13 +138,11 @@ struct sdhci_host {
 #define SDHCI_REQ_USE_DMA	(1<<2)	/* Use DMA for this req. */
 #define SDHCI_DEVICE_DEAD	(1<<3)	/* Device unresponsive */
 #define SDHCI_SDR50_NEEDS_TUNING (1<<4)	/* SDR50 needs tuning */
-#define SDHCI_NEEDS_RETUNING	(1<<5)	/* Host needs retuning */
 #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_SDR104_NEEDS_TUNING (1<<10)	/* SDR104/HS200 needs tuning */
-#define SDHCI_USING_RETUNING_TIMER (1<<11)	/* Host is using a retuning timer for the card */
 #define SDHCI_USE_64_BIT_DMA	(1<<12)	/* Use 64-bit DMA */
 #define SDHCI_HS400_TUNING	(1<<13)	/* Tuning for HS400 */
 
@@ -210,7 +208,6 @@ struct sdhci_host {
 	unsigned int		tuning_count;	/* Timer count for re-tuning */
 	unsigned int		tuning_mode;	/* Re-tuning mode supported by host */
 #define SDHCI_TUNING_MODE_1	0
-	struct timer_list	tuning_timer;	/* Timer for tuning */
 
 	struct sdhci_host_next	next_data;
 	unsigned long private[0] ____cacheline_aligned;
-- 
1.9.1

Re: [PATCH V2 11/15] mmc: sdhci: Change to new way of doing re-tuning

[Ulf Hansson]

On 29 January 2015 at 10:00, Adrian Hunter <adrian.hunter@intel.com> wrote:
> Make use of mmc core support for re-tuning instead
> of doing it all in the sdhci driver.
>
> This patch also changes to flag the need for re-tuning
> always after runtime suspend when tuning has been used
> at initialization. Previously it was only done if
> the re-tuning timer was in use.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  drivers/mmc/host/sdhci.c  | 113 ++++++----------------------------------------
>  include/linux/mmc/sdhci.h |   3 --
>  2 files changed, 14 insertions(+), 102 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index c9881ca..dd0be76 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -51,7 +51,6 @@ static void sdhci_finish_data(struct sdhci_host *);
>
>  static void sdhci_finish_command(struct sdhci_host *);
>  static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode);
> -static void sdhci_tuning_timer(unsigned long data);
>  static void sdhci_enable_preset_value(struct sdhci_host *host, bool enable);
>  static int sdhci_pre_dma_transfer(struct sdhci_host *host,
>                                         struct mmc_data *data,
> @@ -252,17 +251,6 @@ static void sdhci_init(struct sdhci_host *host, int soft)
>  static void sdhci_reinit(struct sdhci_host *host)
>  {
>         sdhci_init(host, 0);
> -       /*
> -        * Retuning stuffs are affected by different cards inserted and only
> -        * applicable to UHS-I cards. So reset these fields to their initial
> -        * value when card is removed.
> -        */
> -       if (host->flags & SDHCI_USING_RETUNING_TIMER) {
> -               host->flags &= ~SDHCI_USING_RETUNING_TIMER;
> -
> -               del_timer_sync(&host->tuning_timer);
> -               host->flags &= ~SDHCI_NEEDS_RETUNING;
> -       }
>         sdhci_enable_card_detection(host);
>  }
>
> @@ -1350,7 +1338,6 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>         struct sdhci_host *host;
>         int present;
>         unsigned long flags;
> -       u32 tuning_opcode;
>
>         host = mmc_priv(mmc);
>
> @@ -1399,39 +1386,6 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>                 host->mrq->cmd->error = -ENOMEDIUM;
>                 tasklet_schedule(&host->finish_tasklet);
>         } else {
> -               u32 present_state;
> -
> -               present_state = sdhci_readl(host, SDHCI_PRESENT_STATE);
> -               /*
> -                * Check if the re-tuning timer has already expired and there
> -                * is no on-going data transfer and DAT0 is not busy. If so,
> -                * we need to execute tuning procedure before sending command.
> -                */
> -               if ((host->flags & SDHCI_NEEDS_RETUNING) &&
> -                   !(present_state & (SDHCI_DOING_WRITE | SDHCI_DOING_READ)) &&
> -                   (present_state & SDHCI_DATA_0_LVL_MASK)) {
> -                       if (mmc->card) {
> -                               /* eMMC uses cmd21 but sd and sdio use cmd19 */
> -                               tuning_opcode =
> -                                       mmc->card->type == MMC_TYPE_MMC ?
> -                                       MMC_SEND_TUNING_BLOCK_HS200 :
> -                                       MMC_SEND_TUNING_BLOCK;
> -
> -                               /* Here we need to set the host->mrq to NULL,
> -                                * in case the pending finish_tasklet
> -                                * finishes it incorrectly.
> -                                */
> -                               host->mrq = NULL;
> -
> -                               spin_unlock_irqrestore(&host->lock, flags);
> -                               sdhci_execute_tuning(mmc, tuning_opcode);
> -                               spin_lock_irqsave(&host->lock, flags);
> -
> -                               /* Restore original mmc_request structure */
> -                               host->mrq = mrq;
> -                       }
> -               }
> -
>                 if (mrq->sbc && !(host->flags & SDHCI_AUTO_CMD23))
>                         sdhci_send_command(host, mrq->sbc);
>                 else
> @@ -2077,23 +2031,19 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>         }
>
>  out:
> -       host->flags &= ~SDHCI_NEEDS_RETUNING;
> -
>         if (tuning_count) {
> -               host->flags |= SDHCI_USING_RETUNING_TIMER;
> -               mod_timer(&host->tuning_timer, jiffies + tuning_count * HZ);
> +               /*
> +                * In case tuning fails, host controllers which support
> +                * re-tuning can try tuning again at a later time, when the
> +                * re-tuning timer expires.  So for these controllers, we
> +                * return 0. Since there might be other controllers who do not
> +                * have this capability, we return error for them.
> +                */
> +               err = 0;
>         }
>
> -       /*
> -        * In case tuning fails, host controllers which support re-tuning can
> -        * try tuning again at a later time, when the re-tuning timer expires.
> -        * So for these controllers, we return 0. Since there might be other
> -        * controllers who do not have this capability, we return error for
> -        * them. SDHCI_USING_RETUNING_TIMER means the host is currently using
> -        * a retuning timer to do the retuning for the card.
> -        */
> -       if (err && (host->flags & SDHCI_USING_RETUNING_TIMER))
> -               err = 0;
> +       if (!err)
> +               mmc_retune_enable(host->mmc, tuning_count);
>
>         sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
>         sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
> @@ -2349,20 +2299,6 @@ static void sdhci_timeout_timer(unsigned long data)
>         spin_unlock_irqrestore(&host->lock, flags);
>  }
>
> -static void sdhci_tuning_timer(unsigned long data)
> -{
> -       struct sdhci_host *host;
> -       unsigned long flags;
> -
> -       host = (struct sdhci_host *)data;
> -
> -       spin_lock_irqsave(&host->lock, flags);
> -
> -       host->flags |= SDHCI_NEEDS_RETUNING;
> -
> -       spin_unlock_irqrestore(&host->lock, flags);
> -}
> -
>  /*****************************************************************************\
>   *                                                                           *
>   * Interrupt handling                                                        *
> @@ -2740,11 +2676,8 @@ int sdhci_suspend_host(struct sdhci_host *host)
>  {
>         sdhci_disable_card_detection(host);
>
> -       /* Disable tuning since we are suspending */
> -       if (host->flags & SDHCI_USING_RETUNING_TIMER) {
> -               del_timer_sync(&host->tuning_timer);
> -               host->flags &= ~SDHCI_NEEDS_RETUNING;
> -       }
> +       mmc_retune_timer_stop(host->mmc);
> +       mmc_retune_needed(host->mmc);
>
>         if (!device_may_wakeup(mmc_dev(host->mmc))) {
>                 host->ier = 0;
> @@ -2794,10 +2727,6 @@ int sdhci_resume_host(struct sdhci_host *host)
>
>         sdhci_enable_card_detection(host);
>
> -       /* Set the re-tuning expiration flag */
> -       if (host->flags & SDHCI_USING_RETUNING_TIMER)
> -               host->flags |= SDHCI_NEEDS_RETUNING;
> -
>         return ret;
>  }
>
> @@ -2834,11 +2763,8 @@ int sdhci_runtime_suspend_host(struct sdhci_host *host)
>  {
>         unsigned long flags;
>
> -       /* Disable tuning since we are suspending */
> -       if (host->flags & SDHCI_USING_RETUNING_TIMER) {
> -               del_timer_sync(&host->tuning_timer);
> -               host->flags &= ~SDHCI_NEEDS_RETUNING;
> -       }
> +       mmc_retune_timer_stop(host->mmc);

I think this could give a deadlock.

What if the retuning is just about to start and thus sdhci's
->execute_tuning() callback has been invoked, which is waiting for the
pm_runtime_get_sync() to return.

> +       mmc_retune_needed(host->mmc);

This seems racy.

What if a new request has already been started from the mmc core
(waiting for sdhci's ->request() callback to return). That would mean
the mmc core won't detect that a retune was needed.

>
>         spin_lock_irqsave(&host->lock, flags);
>         host->ier &= SDHCI_INT_CARD_INT;
> @@ -2881,10 +2807,6 @@ int sdhci_runtime_resume_host(struct sdhci_host *host)
>                 spin_unlock_irqrestore(&host->lock, flags);
>         }
>
> -       /* Set the re-tuning expiration flag */
> -       if (host->flags & SDHCI_USING_RETUNING_TIMER)
> -               host->flags |= SDHCI_NEEDS_RETUNING;
> -
>         spin_lock_irqsave(&host->lock, flags);
>
>         host->runtime_suspended = false;
> @@ -3419,13 +3341,6 @@ int sdhci_add_host(struct sdhci_host *host)
>
>         init_waitqueue_head(&host->buf_ready_int);
>
> -       if (host->version >= SDHCI_SPEC_300) {
> -               /* Initialize re-tuning timer */
> -               init_timer(&host->tuning_timer);
> -               host->tuning_timer.data = (unsigned long)host;
> -               host->tuning_timer.function = sdhci_tuning_timer;
> -       }
> -
>         sdhci_init(host, 0);
>
>         ret = request_threaded_irq(host->irq, sdhci_irq, sdhci_thread_irq,
> diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
> index c3e3db1..c5b00be 100644
> --- a/include/linux/mmc/sdhci.h
> +++ b/include/linux/mmc/sdhci.h
> @@ -138,13 +138,11 @@ struct sdhci_host {
>  #define SDHCI_REQ_USE_DMA      (1<<2)  /* Use DMA for this req. */
>  #define SDHCI_DEVICE_DEAD      (1<<3)  /* Device unresponsive */
>  #define SDHCI_SDR50_NEEDS_TUNING (1<<4)        /* SDR50 needs tuning */
> -#define SDHCI_NEEDS_RETUNING   (1<<5)  /* Host needs retuning */
>  #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_SDR104_NEEDS_TUNING (1<<10)      /* SDR104/HS200 needs tuning */
> -#define SDHCI_USING_RETUNING_TIMER (1<<11)     /* Host is using a retuning timer for the card */
>  #define SDHCI_USE_64_BIT_DMA   (1<<12) /* Use 64-bit DMA */
>  #define SDHCI_HS400_TUNING     (1<<13) /* Tuning for HS400 */
>
> @@ -210,7 +208,6 @@ struct sdhci_host {
>         unsigned int            tuning_count;   /* Timer count for re-tuning */
>         unsigned int            tuning_mode;    /* Re-tuning mode supported by host */
>  #define SDHCI_TUNING_MODE_1    0
> -       struct timer_list       tuning_timer;   /* Timer for tuning */
>
>         struct sdhci_host_next  next_data;
>         unsigned long private[0] ____cacheline_aligned;
> --
> 1.9.1
>

Kind regards
Uffe

Re: [PATCH V2 11/15] mmc: sdhci: Change to new way of doing re-tuning

[Adrian Hunter]

On 06/03/15 14:51, Ulf Hansson wrote:
> On 29 January 2015 at 10:00, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> Make use of mmc core support for re-tuning instead
>> of doing it all in the sdhci driver.
>>
>> This patch also changes to flag the need for re-tuning
>> always after runtime suspend when tuning has been used
>> at initialization. Previously it was only done if
>> the re-tuning timer was in use.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>  drivers/mmc/host/sdhci.c  | 113 ++++++----------------------------------------
>>  include/linux/mmc/sdhci.h |   3 --
>>  2 files changed, 14 insertions(+), 102 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index c9881ca..dd0be76 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -51,7 +51,6 @@ static void sdhci_finish_data(struct sdhci_host *);
>>
>>  static void sdhci_finish_command(struct sdhci_host *);
>>  static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode);
>> -static void sdhci_tuning_timer(unsigned long data);
>>  static void sdhci_enable_preset_value(struct sdhci_host *host, bool enable);
>>  static int sdhci_pre_dma_transfer(struct sdhci_host *host,
>>                                         struct mmc_data *data,
>> @@ -252,17 +251,6 @@ static void sdhci_init(struct sdhci_host *host, int soft)
>>  static void sdhci_reinit(struct sdhci_host *host)
>>  {
>>         sdhci_init(host, 0);
>> -       /*
>> -        * Retuning stuffs are affected by different cards inserted and only
>> -        * applicable to UHS-I cards. So reset these fields to their initial
>> -        * value when card is removed.
>> -        */
>> -       if (host->flags & SDHCI_USING_RETUNING_TIMER) {
>> -               host->flags &= ~SDHCI_USING_RETUNING_TIMER;
>> -
>> -               del_timer_sync(&host->tuning_timer);
>> -               host->flags &= ~SDHCI_NEEDS_RETUNING;
>> -       }
>>         sdhci_enable_card_detection(host);
>>  }
>>
>> @@ -1350,7 +1338,6 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>>         struct sdhci_host *host;
>>         int present;
>>         unsigned long flags;
>> -       u32 tuning_opcode;
>>
>>         host = mmc_priv(mmc);
>>
>> @@ -1399,39 +1386,6 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>>                 host->mrq->cmd->error = -ENOMEDIUM;
>>                 tasklet_schedule(&host->finish_tasklet);
>>         } else {
>> -               u32 present_state;
>> -
>> -               present_state = sdhci_readl(host, SDHCI_PRESENT_STATE);
>> -               /*
>> -                * Check if the re-tuning timer has already expired and there
>> -                * is no on-going data transfer and DAT0 is not busy. If so,
>> -                * we need to execute tuning procedure before sending command.
>> -                */
>> -               if ((host->flags & SDHCI_NEEDS_RETUNING) &&
>> -                   !(present_state & (SDHCI_DOING_WRITE | SDHCI_DOING_READ)) &&
>> -                   (present_state & SDHCI_DATA_0_LVL_MASK)) {
>> -                       if (mmc->card) {
>> -                               /* eMMC uses cmd21 but sd and sdio use cmd19 */
>> -                               tuning_opcode =
>> -                                       mmc->card->type == MMC_TYPE_MMC ?
>> -                                       MMC_SEND_TUNING_BLOCK_HS200 :
>> -                                       MMC_SEND_TUNING_BLOCK;
>> -
>> -                               /* Here we need to set the host->mrq to NULL,
>> -                                * in case the pending finish_tasklet
>> -                                * finishes it incorrectly.
>> -                                */
>> -                               host->mrq = NULL;
>> -
>> -                               spin_unlock_irqrestore(&host->lock, flags);
>> -                               sdhci_execute_tuning(mmc, tuning_opcode);
>> -                               spin_lock_irqsave(&host->lock, flags);
>> -
>> -                               /* Restore original mmc_request structure */
>> -                               host->mrq = mrq;
>> -                       }
>> -               }
>> -
>>                 if (mrq->sbc && !(host->flags & SDHCI_AUTO_CMD23))
>>                         sdhci_send_command(host, mrq->sbc);
>>                 else
>> @@ -2077,23 +2031,19 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>>         }
>>
>>  out:
>> -       host->flags &= ~SDHCI_NEEDS_RETUNING;
>> -
>>         if (tuning_count) {
>> -               host->flags |= SDHCI_USING_RETUNING_TIMER;
>> -               mod_timer(&host->tuning_timer, jiffies + tuning_count * HZ);
>> +               /*
>> +                * In case tuning fails, host controllers which support
>> +                * re-tuning can try tuning again at a later time, when the
>> +                * re-tuning timer expires.  So for these controllers, we
>> +                * return 0. Since there might be other controllers who do not
>> +                * have this capability, we return error for them.
>> +                */
>> +               err = 0;
>>         }
>>
>> -       /*
>> -        * In case tuning fails, host controllers which support re-tuning can
>> -        * try tuning again at a later time, when the re-tuning timer expires.
>> -        * So for these controllers, we return 0. Since there might be other
>> -        * controllers who do not have this capability, we return error for
>> -        * them. SDHCI_USING_RETUNING_TIMER means the host is currently using
>> -        * a retuning timer to do the retuning for the card.
>> -        */
>> -       if (err && (host->flags & SDHCI_USING_RETUNING_TIMER))
>> -               err = 0;
>> +       if (!err)
>> +               mmc_retune_enable(host->mmc, tuning_count);
>>
>>         sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
>>         sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
>> @@ -2349,20 +2299,6 @@ static void sdhci_timeout_timer(unsigned long data)
>>         spin_unlock_irqrestore(&host->lock, flags);
>>  }
>>
>> -static void sdhci_tuning_timer(unsigned long data)
>> -{
>> -       struct sdhci_host *host;
>> -       unsigned long flags;
>> -
>> -       host = (struct sdhci_host *)data;
>> -
>> -       spin_lock_irqsave(&host->lock, flags);
>> -
>> -       host->flags |= SDHCI_NEEDS_RETUNING;
>> -
>> -       spin_unlock_irqrestore(&host->lock, flags);
>> -}
>> -
>>  /*****************************************************************************\
>>   *                                                                           *
>>   * Interrupt handling                                                        *
>> @@ -2740,11 +2676,8 @@ int sdhci_suspend_host(struct sdhci_host *host)
>>  {
>>         sdhci_disable_card_detection(host);
>>
>> -       /* Disable tuning since we are suspending */
>> -       if (host->flags & SDHCI_USING_RETUNING_TIMER) {
>> -               del_timer_sync(&host->tuning_timer);
>> -               host->flags &= ~SDHCI_NEEDS_RETUNING;
>> -       }
>> +       mmc_retune_timer_stop(host->mmc);
>> +       mmc_retune_needed(host->mmc);
>>
>>         if (!device_may_wakeup(mmc_dev(host->mmc))) {
>>                 host->ier = 0;
>> @@ -2794,10 +2727,6 @@ int sdhci_resume_host(struct sdhci_host *host)
>>
>>         sdhci_enable_card_detection(host);
>>
>> -       /* Set the re-tuning expiration flag */
>> -       if (host->flags & SDHCI_USING_RETUNING_TIMER)
>> -               host->flags |= SDHCI_NEEDS_RETUNING;
>> -
>>         return ret;
>>  }
>>
>> @@ -2834,11 +2763,8 @@ int sdhci_runtime_suspend_host(struct sdhci_host *host)
>>  {
>>         unsigned long flags;
>>
>> -       /* Disable tuning since we are suspending */
>> -       if (host->flags & SDHCI_USING_RETUNING_TIMER) {
>> -               del_timer_sync(&host->tuning_timer);
>> -               host->flags &= ~SDHCI_NEEDS_RETUNING;
>> -       }
>> +       mmc_retune_timer_stop(host->mmc);
> 
> I think this could give a deadlock.
> 
> What if the retuning is just about to start and thus sdhci's
> ->execute_tuning() callback has been invoked, which is waiting for the
> pm_runtime_get_sync() to return.

The re-tune timer is mmc_retune_timer() and it does not take any locks
so it can't deadlock.

> 
>> +       mmc_retune_needed(host->mmc);
> 
> This seems racy.
> 
> What if a new request has already been started from the mmc core
> (waiting for sdhci's ->request() callback to return). That would mean
> the mmc core won't detect that a retune was needed.

That is a good point. The host controller must not runtime suspend after
re-tuning until retuning is released. I can think of a couple of options:
	- move the retuning call into the ->request function
	- add extra host ops for the host to runtime resume/suspend

Re: [PATCH V2 11/15] mmc: sdhci: Change to new way of doing re-tuning

[Ulf Hansson]

[...]

>>> @@ -2834,11 +2763,8 @@ int sdhci_runtime_suspend_host(struct sdhci_host *host)
>>>  {
>>>         unsigned long flags;
>>>
>>> -       /* Disable tuning since we are suspending */
>>> -       if (host->flags & SDHCI_USING_RETUNING_TIMER) {
>>> -               del_timer_sync(&host->tuning_timer);
>>> -               host->flags &= ~SDHCI_NEEDS_RETUNING;
>>> -       }
>>> +       mmc_retune_timer_stop(host->mmc);
>>
>> I think this could give a deadlock.
>>
>> What if the retuning is just about to start and thus sdhci's
>> ->execute_tuning() callback has been invoked, which is waiting for the
>> pm_runtime_get_sync() to return.
>
> The re-tune timer is mmc_retune_timer() and it does not take any locks
> so it can't deadlock.
>

You missed my point. The problem is related to runtime PM.

Here the sequence I think will cause the deadlock.
mmc_retune_timer_stop()
  ->del_timer_sync()
...
Wait for timer-handler to finish.

If the timer-handler is running, it has invoked the ->execute_tuning()
callback and is thus waiting for a pm_runtime_get_sync() to return.

Now, waiting for a pm_runtime_get_sync() to return from a runtime PM
suspend callback will deadlock!

>>
>>> +       mmc_retune_needed(host->mmc);
>>
>> This seems racy.
>>
>> What if a new request has already been started from the mmc core
>> (waiting for sdhci's ->request() callback to return). That would mean
>> the mmc core won't detect that a retune was needed.
>
> That is a good point. The host controller must not runtime suspend after
> re-tuning until retuning is released. I can think of a couple of options:
>         - move the retuning call into the ->request function
>         - add extra host ops for the host to runtime resume/suspend
>

I am not sure which approach I prefer yet. Need some more time to think.

For your information, Neil Brown is having a similar issue which he is
trying to address [1].

I think we need an generic approach to deal with the runtime PM
synchronization issue described above. More precisely in those
scenarios when mmc hosts needs to notify the mmc core to take some
specific actions, from a mmc host's runtime PM callback.

Kind regards
Uffe

[1]
https://lkml.org/lkml/2015/2/23/721

Re: [PATCH V2 11/15] mmc: sdhci: Change to new way of doing re-tuning

[Adrian Hunter]

On 10/03/15 15:55, Ulf Hansson wrote:
> [...]
> 
>>>> @@ -2834,11 +2763,8 @@ int sdhci_runtime_suspend_host(struct sdhci_host *host)
>>>>  {
>>>>         unsigned long flags;
>>>>
>>>> -       /* Disable tuning since we are suspending */
>>>> -       if (host->flags & SDHCI_USING_RETUNING_TIMER) {
>>>> -               del_timer_sync(&host->tuning_timer);
>>>> -               host->flags &= ~SDHCI_NEEDS_RETUNING;
>>>> -       }
>>>> +       mmc_retune_timer_stop(host->mmc);
>>>
>>> I think this could give a deadlock.
>>>
>>> What if the retuning is just about to start and thus sdhci's
>>> ->execute_tuning() callback has been invoked, which is waiting for the
>>> pm_runtime_get_sync() to return.
>>
>> The re-tune timer is mmc_retune_timer() and it does not take any locks
>> so it can't deadlock.
>>
> 
> You missed my point. The problem is related to runtime PM.
> 
> Here the sequence I think will cause the deadlock.
> mmc_retune_timer_stop()
>   ->del_timer_sync()
> ...
> Wait for timer-handler to finish.
> 
> If the timer-handler is running, it has invoked the ->execute_tuning()

No, the timer handler does not invoke anything. It just sets a flag.

> callback and is thus waiting for a pm_runtime_get_sync() to return.
> 
> Now, waiting for a pm_runtime_get_sync() to return from a runtime PM
> suspend callback will deadlock!
> 
>>>
>>>> +       mmc_retune_needed(host->mmc);
>>>
>>> This seems racy.
>>>
>>> What if a new request has already been started from the mmc core
>>> (waiting for sdhci's ->request() callback to return). That would mean
>>> the mmc core won't detect that a retune was needed.
>>
>> That is a good point. The host controller must not runtime suspend after
>> re-tuning until retuning is released. I can think of a couple of options:
>>         - move the retuning call into the ->request function
>>         - add extra host ops for the host to runtime resume/suspend
>>
> 
> I am not sure which approach I prefer yet. Need some more time to think.
> 
> For your information, Neil Brown is having a similar issue which he is
> trying to address [1].

It is a bit different. Re-tuning is about doing something before a
request, rather than before a suspend.

> I think we need an generic approach to deal with the runtime PM
> synchronization issue described above. More precisely in those
> scenarios when mmc hosts needs to notify the mmc core to take some
> specific actions, from a mmc host's runtime PM callback.

For the re-tune case I did not want to assume what the host driver
needed to do, so I added ->hold_tuning() and ->release_tuning()
host operations.

Please see V3 of the patches I sent earlier today.

Thanks very much for looking at the patches by the way! :-)

Re: [PATCH V2 11/15] mmc: sdhci: Change to new way of doing re-tuning

[Ulf Hansson]

On 10 March 2015 at 15:20, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 10/03/15 15:55, Ulf Hansson wrote:
>> [...]
>>
>>>>> @@ -2834,11 +2763,8 @@ int sdhci_runtime_suspend_host(struct sdhci_host *host)
>>>>>  {
>>>>>         unsigned long flags;
>>>>>
>>>>> -       /* Disable tuning since we are suspending */
>>>>> -       if (host->flags & SDHCI_USING_RETUNING_TIMER) {
>>>>> -               del_timer_sync(&host->tuning_timer);
>>>>> -               host->flags &= ~SDHCI_NEEDS_RETUNING;
>>>>> -       }
>>>>> +       mmc_retune_timer_stop(host->mmc);
>>>>
>>>> I think this could give a deadlock.
>>>>
>>>> What if the retuning is just about to start and thus sdhci's
>>>> ->execute_tuning() callback has been invoked, which is waiting for the
>>>> pm_runtime_get_sync() to return.
>>>
>>> The re-tune timer is mmc_retune_timer() and it does not take any locks
>>> so it can't deadlock.
>>>
>>
>> You missed my point. The problem is related to runtime PM.
>>
>> Here the sequence I think will cause the deadlock.
>> mmc_retune_timer_stop()
>>   ->del_timer_sync()
>> ...
>> Wait for timer-handler to finish.
>>
>> If the timer-handler is running, it has invoked the ->execute_tuning()
>
> No, the timer handler does not invoke anything. It just sets a flag.

Ah, yes. I say that now.

>
>> callback and is thus waiting for a pm_runtime_get_sync() to return.
>>
>> Now, waiting for a pm_runtime_get_sync() to return from a runtime PM
>> suspend callback will deadlock!
>>
>>>>
>>>>> +       mmc_retune_needed(host->mmc);
>>>>
>>>> This seems racy.
>>>>
>>>> What if a new request has already been started from the mmc core
>>>> (waiting for sdhci's ->request() callback to return). That would mean
>>>> the mmc core won't detect that a retune was needed.
>>>
>>> That is a good point. The host controller must not runtime suspend after
>>> re-tuning until retuning is released. I can think of a couple of options:
>>>         - move the retuning call into the ->request function
>>>         - add extra host ops for the host to runtime resume/suspend
>>>
>>
>> I am not sure which approach I prefer yet. Need some more time to think.
>>
>> For your information, Neil Brown is having a similar issue which he is
>> trying to address [1].
>
> It is a bit different. Re-tuning is about doing something before a
> request, rather than before a suspend.

That's true, but we are still have the same locking issues to consider
for runtime PM.

>
>> I think we need an generic approach to deal with the runtime PM
>> synchronization issue described above. More precisely in those
>> scenarios when mmc hosts needs to notify the mmc core to take some
>> specific actions, from a mmc host's runtime PM callback.
>
> For the re-tune case I did not want to assume what the host driver
> needed to do, so I added ->hold_tuning() and ->release_tuning()
> host operations.

I have thought a bit more on how I would like this to be implemented.
It's a bit closer to what Neil's suggests in his approach [1].

First, I would like only one API provided from the mmc core. This API
shall be invoked from the host driver's runtime PM suspend callback.

Something along the lines, "mmc_host_runtime_suspend()". This function
will return -EBUSY if the host isn't allowed to be runtime PM
suspended.

To make sure this function don't deadlock, it must not do any requests
towards the host (since that would trigger a pm_runtime_get_sync() of
the host's device).

Also, it need to claim the mmc host in a non-blocking manner
(mmc_claim_host() needs to be extended to support that) since that
prevents deadlocking and also tells us whether there are new requests
prepared by the mmc core. If that's the case, there are no need to
complete the runtime PM suspend operation, but instead immediately
return -EBUSY.

In your case mmc_host_runtime_suspend(), also needs to assign a flag
indicating that a re-tune is needed at next request. That flag shall
be set when the host is claimed to prevent the host_ops->request()
callback to be invoked, which removes the race condition.

Kind regards
Uffe

Re: [PATCH V2 11/15] mmc: sdhci: Change to new way of doing re-tuning

[Adrian Hunter]

On 23/03/15 14:54, Ulf Hansson wrote:
> On 10 March 2015 at 15:20, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 10/03/15 15:55, Ulf Hansson wrote:
>>> [...]
>>>
>>>>>> @@ -2834,11 +2763,8 @@ int sdhci_runtime_suspend_host(struct sdhci_host *host)
>>>>>>  {
>>>>>>         unsigned long flags;
>>>>>>
>>>>>> -       /* Disable tuning since we are suspending */
>>>>>> -       if (host->flags & SDHCI_USING_RETUNING_TIMER) {
>>>>>> -               del_timer_sync(&host->tuning_timer);
>>>>>> -               host->flags &= ~SDHCI_NEEDS_RETUNING;
>>>>>> -       }
>>>>>> +       mmc_retune_timer_stop(host->mmc);
>>>>>
>>>>> I think this could give a deadlock.
>>>>>
>>>>> What if the retuning is just about to start and thus sdhci's
>>>>> ->execute_tuning() callback has been invoked, which is waiting for the
>>>>> pm_runtime_get_sync() to return.
>>>>
>>>> The re-tune timer is mmc_retune_timer() and it does not take any locks
>>>> so it can't deadlock.
>>>>
>>>
>>> You missed my point. The problem is related to runtime PM.
>>>
>>> Here the sequence I think will cause the deadlock.
>>> mmc_retune_timer_stop()
>>>   ->del_timer_sync()
>>> ...
>>> Wait for timer-handler to finish.
>>>
>>> If the timer-handler is running, it has invoked the ->execute_tuning()
>>
>> No, the timer handler does not invoke anything. It just sets a flag.
> 
> Ah, yes. I say that now.
> 
>>
>>> callback and is thus waiting for a pm_runtime_get_sync() to return.
>>>
>>> Now, waiting for a pm_runtime_get_sync() to return from a runtime PM
>>> suspend callback will deadlock!
>>>
>>>>>
>>>>>> +       mmc_retune_needed(host->mmc);
>>>>>
>>>>> This seems racy.
>>>>>
>>>>> What if a new request has already been started from the mmc core
>>>>> (waiting for sdhci's ->request() callback to return). That would mean
>>>>> the mmc core won't detect that a retune was needed.
>>>>
>>>> That is a good point. The host controller must not runtime suspend after
>>>> re-tuning until retuning is released. I can think of a couple of options:
>>>>         - move the retuning call into the ->request function
>>>>         - add extra host ops for the host to runtime resume/suspend
>>>>
>>>
>>> I am not sure which approach I prefer yet. Need some more time to think.
>>>
>>> For your information, Neil Brown is having a similar issue which he is
>>> trying to address [1].
>>
>> It is a bit different. Re-tuning is about doing something before a
>> request, rather than before a suspend.
> 
> That's true, but we are still have the same locking issues to consider
> for runtime PM.

I have no locking issues, so I am not sure what you mean here.

> 
>>
>>> I think we need an generic approach to deal with the runtime PM
>>> synchronization issue described above. More precisely in those
>>> scenarios when mmc hosts needs to notify the mmc core to take some
>>> specific actions, from a mmc host's runtime PM callback.
>>
>> For the re-tune case I did not want to assume what the host driver
>> needed to do, so I added ->hold_tuning() and ->release_tuning()
>> host operations.
> 
> I have thought a bit more on how I would like this to be implemented.
> It's a bit closer to what Neil's suggests in his approach [1].

I am not sure it is valuable to mix up the two issues.

For Neil's problem I would do something quiet different:

1. The host driver already knows the bus width so can easily "get/put"
runtime pm to prevent suspend when the bus width does not permit it.

2. The need to do things when the card is idle comes up a lot (e.g. bkops,
sleep notification, cache flush etc etc). In Neil's case he wants to switch
to 1-bit mode, but that just seems another of these "idle" operations. So I
would investigate the requirements of supporting idle operations in general.


> 
> First, I would like only one API provided from the mmc core. This API
> shall be invoked from the host driver's runtime PM suspend callback.
> 
> Something along the lines, "mmc_host_runtime_suspend()". This function
> will return -EBUSY if the host isn't allowed to be runtime PM
> suspended.
> 
> To make sure this function don't deadlock, it must not do any requests
> towards the host (since that would trigger a pm_runtime_get_sync() of
> the host's device).
> 
> Also, it need to claim the mmc host in a non-blocking manner
> (mmc_claim_host() needs to be extended to support that) since that
> prevents deadlocking and also tells us whether there are new requests
> prepared by the mmc core. If that's the case, there are no need to
> complete the runtime PM suspend operation, but instead immediately
> return -EBUSY.
> 
> In your case mmc_host_runtime_suspend(), also needs to assign a flag
> indicating that a re-tune is needed at next request. That flag shall
> be set when the host is claimed to prevent the host_ops->request()
> callback to be invoked, which removes the race condition.
> 
> Kind regards
> Uffe
> 
> 

Re: [PATCH V2 11/15] mmc: sdhci: Change to new way of doing re-tuning

[Ulf Hansson]

[...]

>
> I have no locking issues, so I am not sure what you mean here.

Okay, I should have stated race conditions.

>
>>
>>>
>>>> I think we need an generic approach to deal with the runtime PM
>>>> synchronization issue described above. More precisely in those
>>>> scenarios when mmc hosts needs to notify the mmc core to take some
>>>> specific actions, from a mmc host's runtime PM callback.
>>>
>>> For the re-tune case I did not want to assume what the host driver
>>> needed to do, so I added ->hold_tuning() and ->release_tuning()
>>> host operations.
>>
>> I have thought a bit more on how I would like this to be implemented.
>> It's a bit closer to what Neil's suggests in his approach [1].
>
> I am not sure it is valuable to mix up the two issues.
>
> For Neil's problem I would do something quiet different:
>
> 1. The host driver already knows the bus width so can easily "get/put"
> runtime pm to prevent suspend when the bus width does not permit it.

To me the problem is the other way around.

The host driver don't want prevent runtime PM suspend. Instead it want
to notify the core that it's ready to be runtime PM suspended.

Due to restrictions by the SDIO spec, the mmc core first need to
switch to 1-bit data, before the host can do clock gating in runtime
PM suspend.

>
> 2. The need to do things when the card is idle comes up a lot (e.g. bkops,
> sleep notification, cache flush etc etc). In Neil's case he wants to switch
> to 1-bit mode, but that just seems another of these "idle" operations. So I
> would investigate the requirements of supporting idle operations in general.

That won't work for the SDIO case, since runtime PM is being deployed
differently for SDIO than for MMC/SD.

In the SDIO case it's the SDIO func drivers that handles the get/put.
For the MMC/SD case it's handled by the block device layer.

Still, yes I agree that it would be of great interest to look into
"idle" operations of eMMC/SD, especially for those other things you
mention, but I don't see it as the solution for Neil's SDIO issue.

Kind regards
Uffe

Re: [PATCH V2 11/15] mmc: sdhci: Change to new way of doing re-tuning

[Adrian Hunter]

On 23/03/2015 5:02 p.m., Ulf Hansson wrote:
> [...]
>
>>
>> I have no locking issues, so I am not sure what you mean here.
>
> Okay, I should have stated race conditions.

Which I resolved using runtime get / put calls.

Returning -EBUSY from runtime suspend, on the other hand, seems
less than ideal.

First, reading the hold count from runtime suspend is a new race.

Secondly, returning -EBUSY leaves the host controller active until the
host controller driver is accessed again, which breaks runtime pm.

>
>>
>>>
>>>>
>>>>> I think we need an generic approach to deal with the runtime PM
>>>>> synchronization issue described above. More precisely in those
>>>>> scenarios when mmc hosts needs to notify the mmc core to take some
>>>>> specific actions, from a mmc host's runtime PM callback.
>>>>
>>>> For the re-tune case I did not want to assume what the host driver
>>>> needed to do, so I added ->hold_tuning() and ->release_tuning()
>>>> host operations.
>>>
>>> I have thought a bit more on how I would like this to be implemented.
>>> It's a bit closer to what Neil's suggests in his approach [1].
>>
>> I am not sure it is valuable to mix up the two issues.
>>
>> For Neil's problem I would do something quiet different:
>>
>> 1. The host driver already knows the bus width so can easily "get/put"
>> runtime pm to prevent suspend when the bus width does not permit it.
>
> To me the problem is the other way around.
>
> The host driver don't want prevent runtime PM suspend. Instead it want
> to notify the core that it's ready to be runtime PM suspended.
>
> Due to restrictions by the SDIO spec, the mmc core first need to
> switch to 1-bit data, before the host can do clock gating in runtime
> PM suspend.

That makes two things dependent instead of decoupling them.

>
>>
>> 2. The need to do things when the card is idle comes up a lot (e.g. bkops,
>> sleep notification, cache flush etc etc). In Neil's case he wants to switch
>> to 1-bit mode, but that just seems another of these "idle" operations. So I
>> would investigate the requirements of supporting idle operations in general.
>
> That won't work for the SDIO case, since runtime PM is being deployed
> differently for SDIO than for MMC/SD.
>
> In the SDIO case it's the SDIO func drivers that handles the get/put.
> For the MMC/SD case it's handled by the block device layer.

It doesn't need to have anything to do with runtime pm. It just needs
to be a way the block or SDIO function drivers can inform the core
that other stuff can be done.

Re: [PATCH V2 11/15] mmc: sdhci: Change to new way of doing re-tuning

[Ulf Hansson]

On 23 March 2015 at 22:11, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 23/03/2015 5:02 p.m., Ulf Hansson wrote:
>>
>> [...]
>>
>>>
>>> I have no locking issues, so I am not sure what you mean here.
>>
>>
>> Okay, I should have stated race conditions.
>
>
> Which I resolved using runtime get / put calls.

Yes, I noticed that and it works! Though, I would rather avoid to add
yet another pair of host ops callbacks for this.

What do you think if these options instead?
1) Do runtime PM get/put from the host ops ->enable|disable()
callbacks. As omap_hsmmc does.
2) Or even better, do runtime PM get/put directly of the host device
from mmc_claim|release_host().

>
> Returning -EBUSY from runtime suspend, on the other hand, seems
> less than ideal.

Agree, if we can avoid it, we certainly should!

>
> First, reading the hold count from runtime suspend is a new race.

I am not sure I understand, could you please elaborate why?

>
> Secondly, returning -EBUSY leaves the host controller active until the
> host controller driver is accessed again, which breaks runtime pm.

Returning -EBUSY is "allowed" for any runtime PM suspend callback and
it's supported by the runtime PM core. We won't be breaking anything.

Anyway, I suggest we put this idea on hold and try other options.

>
>>
>>>
>>>>
>>>>>
>>>>>> I think we need an generic approach to deal with the runtime PM
>>>>>> synchronization issue described above. More precisely in those
>>>>>> scenarios when mmc hosts needs to notify the mmc core to take some
>>>>>> specific actions, from a mmc host's runtime PM callback.
>>>>>
>>>>>
>>>>> For the re-tune case I did not want to assume what the host driver
>>>>> needed to do, so I added ->hold_tuning() and ->release_tuning()
>>>>> host operations.
>>>>
>>>>
>>>> I have thought a bit more on how I would like this to be implemented.
>>>> It's a bit closer to what Neil's suggests in his approach [1].
>>>
>>>
>>> I am not sure it is valuable to mix up the two issues.
>>>
>>> For Neil's problem I would do something quiet different:
>>>
>>> 1. The host driver already knows the bus width so can easily "get/put"
>>> runtime pm to prevent suspend when the bus width does not permit it.
>>
>>
>> To me the problem is the other way around.
>>
>> The host driver don't want prevent runtime PM suspend. Instead it want
>> to notify the core that it's ready to be runtime PM suspended.
>>
>> Due to restrictions by the SDIO spec, the mmc core first need to
>> switch to 1-bit data, before the host can do clock gating in runtime
>> PM suspend.
>
>
> That makes two things dependent instead of decoupling them.

Good point!

I don't like it either, let's try do better!

>
>>
>>>
>>> 2. The need to do things when the card is idle comes up a lot (e.g.
>>> bkops,
>>> sleep notification, cache flush etc etc). In Neil's case he wants to
>>> switch
>>> to 1-bit mode, but that just seems another of these "idle" operations. So
>>> I
>>> would investigate the requirements of supporting idle operations in
>>> general.
>>
>>
>> That won't work for the SDIO case, since runtime PM is being deployed
>> differently for SDIO than for MMC/SD.
>>
>> In the SDIO case it's the SDIO func drivers that handles the get/put.
>> For the MMC/SD case it's handled by the block device layer.
>
>
> It doesn't need to have anything to do with runtime pm. It just needs
> to be a way the block or SDIO function drivers can inform the core
> that other stuff can be done.

I have thought more about this since yesterday and I somewhat agree
with your suggestion. Especially in that sense that I think we should
consider Neil's issue as an "idle operation" for SDIO.

For "idle operations" for MMC/SD cards, runtime PM is already
deployed. So, I think it's just a matter of extending that support to
cover more "idle operations" besides the MMC_CAP_AGGRESSIVE_PM thing.

What we need to figure out is how to make this work for SDIO - and
that's when it becomes more complex. I really would like us to avoid
exporting new SDIO APIs, unless really needed.

Today runtime PM is deployed by the following SDIO func drivers:
drivers/net/wireless/libertas/if_sdio.c
drivers/net/wireless/ti/wl1251/sdio.c
drivers/net/wireless/ti/wlcore/sdio.c

We _could_ consider to convert above drivers to use something else
than runtime PM to control the power to the card. I think that would
be the ideal solution, since then we can deploy runtime PM for SDIO
fairly similar to how MMC/SDIO is done. That means doing runtime PM
get/put of the device for the struct mmc_card, inside the mmc core
while handling SDIO requests.

The above requires some work, both in the mmc core and in the SDIO
func drivers. The nice thing is that we don't need to export new APIs
to the SDIO func drivers and we can keep the complexity of dealing
with "idle operations" inside the mmc core.

Thoughts?

Kind regards
Uffe

Re: [PATCH V2 11/15] mmc: sdhci: Change to new way of doing re-tuning

[Adrian Hunter]

On 24/03/15 23:12, Ulf Hansson wrote:
> On 23 March 2015 at 22:11, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 23/03/2015 5:02 p.m., Ulf Hansson wrote:
>>>
>>> [...]
>>>
>>>>
>>>> I have no locking issues, so I am not sure what you mean here.
>>>
>>>
>>> Okay, I should have stated race conditions.
>>
>>
>> Which I resolved using runtime get / put calls.
> 
> Yes, I noticed that and it works! Though, I would rather avoid to add
> yet another pair of host ops callbacks for this.
> 
> What do you think if these options instead?
> 1) Do runtime PM get/put from the host ops ->enable|disable()
> callbacks. As omap_hsmmc does.
> 2) Or even better, do runtime PM get/put directly of the host device
> from mmc_claim|release_host().

Claiming the host is not directly related to host controller runtime pm. It
is possible to imagine cases for claiming the host for synchronization
reasons that do not need the host controller powered up. And card drivers
could reasonably assume that they can claim the host for long periods
without affecting the runtime pm of the host controller.

Currently we have that the host controller driver is the exclusive owner of
runtime pm for the host controller. So the first thing is to decide if we
want to keep that or let the core be involved as well. I would argue for
sticking with the status quo. Let the host controller driver know what is
going on and leave it to do the right thing with its own power management.

That means the callbacks, which are

> 
>>
>> Returning -EBUSY from runtime suspend, on the other hand, seems
>> less than ideal.
> 
> Agree, if we can avoid it, we certainly should!
> 
>>
>> First, reading the hold count from runtime suspend is a new race.
> 
> I am not sure I understand, could you please elaborate why?

Just in the sense that there is no synchronization between the suspend
callback and updating the hold count. So the upper layers could finish a
request and become idle but the suspend callback might have seen the hold
count before it was reduced to zero, leaving the host controller active when
it should be runtime suspended.

> 
>>
>> Secondly, returning -EBUSY leaves the host controller active until the
>> host controller driver is accessed again, which breaks runtime pm.
> 
> Returning -EBUSY is "allowed" for any runtime PM suspend callback and
> it's supported by the runtime PM core. We won't be breaking anything.

Yes, I agree it is not a violation of the API.

I just meant that if the host controller remains active when it should be
runtime suspended then power management is, in a sense, "broken".

> 
> Anyway, I suggest we put this idea on hold and try other options.
> 
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>>> I think we need an generic approach to deal with the runtime PM
>>>>>>> synchronization issue described above. More precisely in those
>>>>>>> scenarios when mmc hosts needs to notify the mmc core to take some
>>>>>>> specific actions, from a mmc host's runtime PM callback.
>>>>>>
>>>>>>
>>>>>> For the re-tune case I did not want to assume what the host driver
>>>>>> needed to do, so I added ->hold_tuning() and ->release_tuning()
>>>>>> host operations.
>>>>>
>>>>>
>>>>> I have thought a bit more on how I would like this to be implemented.
>>>>> It's a bit closer to what Neil's suggests in his approach [1].
>>>>
>>>>
>>>> I am not sure it is valuable to mix up the two issues.
>>>>
>>>> For Neil's problem I would do something quiet different:
>>>>
>>>> 1. The host driver already knows the bus width so can easily "get/put"
>>>> runtime pm to prevent suspend when the bus width does not permit it.
>>>
>>>
>>> To me the problem is the other way around.
>>>
>>> The host driver don't want prevent runtime PM suspend. Instead it want
>>> to notify the core that it's ready to be runtime PM suspended.
>>>
>>> Due to restrictions by the SDIO spec, the mmc core first need to
>>> switch to 1-bit data, before the host can do clock gating in runtime
>>> PM suspend.
>>
>>
>> That makes two things dependent instead of decoupling them.
> 
> Good point!
> 
> I don't like it either, let's try do better!
> 
>>
>>>
>>>>
>>>> 2. The need to do things when the card is idle comes up a lot (e.g.
>>>> bkops,
>>>> sleep notification, cache flush etc etc). In Neil's case he wants to
>>>> switch
>>>> to 1-bit mode, but that just seems another of these "idle" operations. So
>>>> I
>>>> would investigate the requirements of supporting idle operations in
>>>> general.
>>>
>>>
>>> That won't work for the SDIO case, since runtime PM is being deployed
>>> differently for SDIO than for MMC/SD.
>>>
>>> In the SDIO case it's the SDIO func drivers that handles the get/put.
>>> For the MMC/SD case it's handled by the block device layer.
>>
>>
>> It doesn't need to have anything to do with runtime pm. It just needs
>> to be a way the block or SDIO function drivers can inform the core
>> that other stuff can be done.
> 
> I have thought more about this since yesterday and I somewhat agree
> with your suggestion. Especially in that sense that I think we should
> consider Neil's issue as an "idle operation" for SDIO.
> 
> For "idle operations" for MMC/SD cards, runtime PM is already
> deployed. So, I think it's just a matter of extending that support to
> cover more "idle operations" besides the MMC_CAP_AGGRESSIVE_PM thing.
> 
> What we need to figure out is how to make this work for SDIO - and
> that's when it becomes more complex. I really would like us to avoid
> exporting new SDIO APIs, unless really needed.
> 
> Today runtime PM is deployed by the following SDIO func drivers:
> drivers/net/wireless/libertas/if_sdio.c
> drivers/net/wireless/ti/wl1251/sdio.c
> drivers/net/wireless/ti/wlcore/sdio.c
> 
> We _could_ consider to convert above drivers to use something else
> than runtime PM to control the power to the card. I think that would
> be the ideal solution, since then we can deploy runtime PM for SDIO
> fairly similar to how MMC/SDIO is done. That means doing runtime PM
> get/put of the device for the struct mmc_card, inside the mmc core
> while handling SDIO requests.
> 
> The above requires some work, both in the mmc core and in the SDIO
> func drivers. The nice thing is that we don't need to export new APIs
> to the SDIO func drivers and we can keep the complexity of dealing
> with "idle operations" inside the mmc core.
> 
> Thoughts?

There isn't necessarily any link between "idle operations" and runtime pm.

For example for eMMC background operations I would expect to see:

- queue is empty so block driver enables "idle operations".
- core waits (delayed work?) a few milliseconds and then starts bkops.
- a new I/O request arrives, block driver wakes up and tells the core to
stop "idle operations" ASAP, and waits until it does.
- or, the core notifies (callback perhaps) the block driver that "idle
operations" are finished, so the block driver can runtime-put the card

Also need to stop "idle operations" for system suspend, maybe other places.

Now in Neil's case there is a relation to runtime pm in that it would be
nice if the switch to 1-bit mode was delayed by the host controllers
autosuspend_delay. But potentially the host controller driver could
routinely set the delay so that it matches the autosuspend_delay anyway.

Currently the SDIO function drivers all use sdio_claim_host(). So for Neil's
case I would see:

- host controller driver sees the switch to 4-bit mode and does a runtime
"get" to prevent runtime suspend
- sdio_release_host enables "idle operations"
- the core sees that someone has requested a switch to 1-bit mode after a
certain delay, so it waits that delay (delayed work?) and does the switch
- host controller driver sees the switch to 1-bit mode and runtime suspends
via a pm_runtime_put
- sdio_claim_host tells the core to stop "idle operations" ASAP and waits
until it has
- host controller driver sees the switch to 4-bit mode and does a runtime
"get" to prevent runtime suspend

Re: [PATCH V2 11/15] mmc: sdhci: Change to new way of doing re-tuning

[Ulf Hansson]

On 25 March 2015 at 14:48, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 24/03/15 23:12, Ulf Hansson wrote:
> > On 23 March 2015 at 22:11, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >> On 23/03/2015 5:02 p.m., Ulf Hansson wrote:
> >>>
> >>> [...]
> >>>
> >>>>
> >>>> I have no locking issues, so I am not sure what you mean here.
> >>>
> >>>
> >>> Okay, I should have stated race conditions.
> >>
> >>
> >> Which I resolved using runtime get / put calls.
> >
> > Yes, I noticed that and it works! Though, I would rather avoid to add
> > yet another pair of host ops callbacks for this.
> >
> > What do you think if these options instead?
> > 1) Do runtime PM get/put from the host ops ->enable|disable()
> > callbacks. As omap_hsmmc does.
> > 2) Or even better, do runtime PM get/put directly of the host device
> > from mmc_claim|release_host().
>
> Claiming the host is not directly related to host controller runtime pm. It
> is possible to imagine cases for claiming the host for synchronization
> reasons that do not need the host controller powered up. And card drivers
> could reasonably assume that they can claim the host for long periods
> without affecting the runtime pm of the host controller.


Yes, it _may_ power up the host controller sometimes when not needed.
I don't think this will be an issue, mostly because it should be rare
and not happening frequently - if ever.

The only users of mmc_claim_host() for SD/MMC is the core itself, so I
don't see any issue with misbehaving "card drivers" here.

SDIO is again different, since it's up to the SDIO func drivers to
behave - as you stated. But, if they don't they anyway need to be
fixed, right!?

>
> Currently we have that the host controller driver is the exclusive owner of
> runtime pm for the host controller. So the first thing is to decide if we
> want to keep that or let the core be involved as well. I would argue for
> sticking with the status quo. Let the host controller driver know what is
> going on and leave it to do the right thing with its own power management.

The problem I see with the current solution, is that we will be
scheduling a runtime PM suspend for each an every request towards the
host.

It's ineffective, due to that we will unnecessary involve the runtime
PM core, thus increasing CPU utilization - when we actually don't need
to.

I will send a patch for this tomorrow, let's discuss it separately.

[...]

> >
> > I have thought more about this since yesterday and I somewhat agree
> > with your suggestion. Especially in that sense that I think we should
> > consider Neil's issue as an "idle operation" for SDIO.
> >
> > For "idle operations" for MMC/SD cards, runtime PM is already
> > deployed. So, I think it's just a matter of extending that support to
> > cover more "idle operations" besides the MMC_CAP_AGGRESSIVE_PM thing.
> >
> > What we need to figure out is how to make this work for SDIO - and
> > that's when it becomes more complex. I really would like us to avoid
> > exporting new SDIO APIs, unless really needed.
> >
> > Today runtime PM is deployed by the following SDIO func drivers:
> > drivers/net/wireless/libertas/if_sdio.c
> > drivers/net/wireless/ti/wl1251/sdio.c
> > drivers/net/wireless/ti/wlcore/sdio.c
> >
> > We _could_ consider to convert above drivers to use something else
> > than runtime PM to control the power to the card. I think that would
> > be the ideal solution, since then we can deploy runtime PM for SDIO
> > fairly similar to how MMC/SDIO is done. That means doing runtime PM
> > get/put of the device for the struct mmc_card, inside the mmc core
> > while handling SDIO requests.
> >
> > The above requires some work, both in the mmc core and in the SDIO
> > func drivers. The nice thing is that we don't need to export new APIs
> > to the SDIO func drivers and we can keep the complexity of dealing
> > with "idle operations" inside the mmc core.
> >
> > Thoughts?
>
> There isn't necessarily any link between "idle operations" and runtime pm.

I think this is exactly what runtime PM is designed for so I don't
want us to re-invent something that is specific for mmc.

>
> For example for eMMC background operations I would expect to see:
>
> - queue is empty so block driver enables "idle operations".
> - core waits (delayed work?) a few milliseconds and then starts bkops.
> - a new I/O request arrives, block driver wakes up and tells the core to
> stop "idle operations" ASAP, and waits until it does.
> - or, the core notifies (callback perhaps) the block driver that "idle
> operations" are finished, so the block driver can runtime-put the card
>
> Also need to stop "idle operations" for system suspend, maybe other places.

Conceptual wise, I fully agree. Though, I want to make use of runtime PM.

For the eMMC/SD case, the runtime PM suspend callbacks (specified per
bus_ops) should be able to act as the "trigger" point to kick off
"idle operations".

Now, the thing to figure out, is how to execute "idle operations" and
at the same time being able to interrupt/abort them from another
context. An option for how to deal with this, could be to schedule a
"delayed work" from the runtime PM suspend callback. But if we can
avoid it, I think we should.

>
> Now in Neil's case there is a relation to runtime pm in that it would be
> nice if the switch to 1-bit mode was delayed by the host controllers
> autosuspend_delay. But potentially the host controller driver could
> routinely set the delay so that it matches the autosuspend_delay anyway.
>
> Currently the SDIO function drivers all use sdio_claim_host(). So for Neil's
> case I would see:
>
> - host controller driver sees the switch to 4-bit mode and does a runtime
> "get" to prevent runtime suspend
> - sdio_release_host enables "idle operations"
> - the core sees that someone has requested a switch to 1-bit mode after a
> certain delay, so it waits that delay (delayed work?) and does the switch
> - host controller driver sees the switch to 1-bit mode and runtime suspends
> via a pm_runtime_put
> - sdio_claim_host tells the core to stop "idle operations" ASAP and waits
> until it has
> - host controller driver sees the switch to 4-bit mode and does a runtime
> "get" to prevent runtime suspend

That seems like a way forward!

Still I rather would like the above mentioned SDIO func drivers to use
something else than runtime PM to control the power to the cards, as I
suggested. But that might be too much to fix!?

Kind regards
Uffe

Re: [PATCH V2 11/15] mmc: sdhci: Change to new way of doing re-tuning

[Adrian Hunter]

On 26/03/15 18:06, Ulf Hansson wrote:
> On 25 March 2015 at 14:48, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 24/03/15 23:12, Ulf Hansson wrote:
>>> On 23 March 2015 at 22:11, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> On 23/03/2015 5:02 p.m., Ulf Hansson wrote:
>>>>>
>>>>> [...]
>>>>>
>>>>>>
>>>>>> I have no locking issues, so I am not sure what you mean here.
>>>>>
>>>>>
>>>>> Okay, I should have stated race conditions.
>>>>
>>>>
>>>> Which I resolved using runtime get / put calls.
>>>
>>> Yes, I noticed that and it works! Though, I would rather avoid to add
>>> yet another pair of host ops callbacks for this.
>>>
>>> What do you think if these options instead?
>>> 1) Do runtime PM get/put from the host ops ->enable|disable()
>>> callbacks. As omap_hsmmc does.
>>> 2) Or even better, do runtime PM get/put directly of the host device
>>> from mmc_claim|release_host().
>>
>> Claiming the host is not directly related to host controller runtime pm. It
>> is possible to imagine cases for claiming the host for synchronization
>> reasons that do not need the host controller powered up. And card drivers
>> could reasonably assume that they can claim the host for long periods
>> without affecting the runtime pm of the host controller.
> 
> 
> Yes, it _may_ power up the host controller sometimes when not needed.
> I don't think this will be an issue, mostly because it should be rare
> and not happening frequently - if ever.
> 
> The only users of mmc_claim_host() for SD/MMC is the core itself, so I
> don't see any issue with misbehaving "card drivers" here.
> 
> SDIO is again different, since it's up to the SDIO func drivers to
> behave - as you stated. But, if they don't they anyway need to be
> fixed, right!?

Be aware that that links the holding of re-tuning with claiming the host.
It will then not be allowed for re-tuning to be held when the host is
released. Will we have to add to mmc_release_host():

	WARN_ON(host->hold_count);

That could be a problem. Say you wanted to start bkops and then release the
host so that a different context could issue an HPI. That wouldn't be
allowed anymore.

Generally it is impossible to see all ends, which begs the question: why
link things if you don't have to?

> 
>>
>> Currently we have that the host controller driver is the exclusive owner of
>> runtime pm for the host controller. So the first thing is to decide if we
>> want to keep that or let the core be involved as well. I would argue for
>> sticking with the status quo. Let the host controller driver know what is
>> going on and leave it to do the right thing with its own power management.
> 
> The problem I see with the current solution, is that we will be
> scheduling a runtime PM suspend for each an every request towards the
> host.
> 
> It's ineffective, due to that we will unnecessary involve the runtime
> PM core, thus increasing CPU utilization - when we actually don't need
> to.
> 
> I will send a patch for this tomorrow, let's discuss it separately.

Yes please let's keep that separate.

> 
> [...]
> 
>>>
>>> I have thought more about this since yesterday and I somewhat agree
>>> with your suggestion. Especially in that sense that I think we should
>>> consider Neil's issue as an "idle operation" for SDIO.
>>>
>>> For "idle operations" for MMC/SD cards, runtime PM is already
>>> deployed. So, I think it's just a matter of extending that support to
>>> cover more "idle operations" besides the MMC_CAP_AGGRESSIVE_PM thing.
>>>
>>> What we need to figure out is how to make this work for SDIO - and
>>> that's when it becomes more complex. I really would like us to avoid
>>> exporting new SDIO APIs, unless really needed.
>>>
>>> Today runtime PM is deployed by the following SDIO func drivers:
>>> drivers/net/wireless/libertas/if_sdio.c
>>> drivers/net/wireless/ti/wl1251/sdio.c
>>> drivers/net/wireless/ti/wlcore/sdio.c
>>>
>>> We _could_ consider to convert above drivers to use something else
>>> than runtime PM to control the power to the card. I think that would
>>> be the ideal solution, since then we can deploy runtime PM for SDIO
>>> fairly similar to how MMC/SDIO is done. That means doing runtime PM
>>> get/put of the device for the struct mmc_card, inside the mmc core
>>> while handling SDIO requests.
>>>
>>> The above requires some work, both in the mmc core and in the SDIO
>>> func drivers. The nice thing is that we don't need to export new APIs
>>> to the SDIO func drivers and we can keep the complexity of dealing
>>> with "idle operations" inside the mmc core.
>>>
>>> Thoughts?
>>
>> There isn't necessarily any link between "idle operations" and runtime pm.
> 
> I think this is exactly what runtime PM is designed for so I don't
> want us to re-invent something that is specific for mmc.

Need to remember that PM can theoretically be configured out, which makes
using it for bkops seem inappropriate.

> 
>>
>> For example for eMMC background operations I would expect to see:
>>
>> - queue is empty so block driver enables "idle operations".
>> - core waits (delayed work?) a few milliseconds and then starts bkops.
>> - a new I/O request arrives, block driver wakes up and tells the core to
>> stop "idle operations" ASAP, and waits until it does.
>> - or, the core notifies (callback perhaps) the block driver that "idle
>> operations" are finished, so the block driver can runtime-put the card
>>
>> Also need to stop "idle operations" for system suspend, maybe other places.
> 
> Conceptual wise, I fully agree. Though, I want to make use of runtime PM.

So long as it is power management. I am not sure bkops falls into that category.

> 
> For the eMMC/SD case, the runtime PM suspend callbacks (specified per
> bus_ops) should be able to act as the "trigger" point to kick off
> "idle operations".
> 
> Now, the thing to figure out, is how to execute "idle operations" and
> at the same time being able to interrupt/abort them from another
> context. An option for how to deal with this, could be to schedule a
> "delayed work" from the runtime PM suspend callback. But if we can
> avoid it, I think we should.

Whatever the context that runs the idle operations, for the case where a
driver wants to stop the idle operations ASAP, it would be nice if it could
simply take over control - particularly if the idle operation is anyway
waiting for something. So the HPI or whatever is done in the driver context
directly and the idle operation context (if there even is one at that stage)
just exits.

> 
>>
>> Now in Neil's case there is a relation to runtime pm in that it would be
>> nice if the switch to 1-bit mode was delayed by the host controllers
>> autosuspend_delay. But potentially the host controller driver could
>> routinely set the delay so that it matches the autosuspend_delay anyway.
>>
>> Currently the SDIO function drivers all use sdio_claim_host(). So for Neil's
>> case I would see:
>>
>> - host controller driver sees the switch to 4-bit mode and does a runtime
>> "get" to prevent runtime suspend
>> - sdio_release_host enables "idle operations"
>> - the core sees that someone has requested a switch to 1-bit mode after a
>> certain delay, so it waits that delay (delayed work?) and does the switch
>> - host controller driver sees the switch to 1-bit mode and runtime suspends
>> via a pm_runtime_put
>> - sdio_claim_host tells the core to stop "idle operations" ASAP and waits
>> until it has
>> - host controller driver sees the switch to 4-bit mode and does a runtime
>> "get" to prevent runtime suspend
> 
> That seems like a way forward!
> 
> Still I rather would like the above mentioned SDIO func drivers to use
> something else than runtime PM to control the power to the cards, as I
> suggested. But that might be too much to fix!?

Let's keep that a separate issue.

Re: [PATCH V2 11/15] mmc: sdhci: Change to new way of doing re-tuning

[Adrian Hunter]

On 27/03/15 11:54, Adrian Hunter wrote:
> On 26/03/15 18:06, Ulf Hansson wrote:
>> On 25 March 2015 at 14:48, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>
>>> On 24/03/15 23:12, Ulf Hansson wrote:
>>>> On 23 March 2015 at 22:11, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>> On 23/03/2015 5:02 p.m., Ulf Hansson wrote:
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>>
>>>>>>> I have no locking issues, so I am not sure what you mean here.
>>>>>>
>>>>>>
>>>>>> Okay, I should have stated race conditions.
>>>>>
>>>>>
>>>>> Which I resolved using runtime get / put calls.
>>>>
>>>> Yes, I noticed that and it works! Though, I would rather avoid to add
>>>> yet another pair of host ops callbacks for this.
>>>>
>>>> What do you think if these options instead?
>>>> 1) Do runtime PM get/put from the host ops ->enable|disable()
>>>> callbacks. As omap_hsmmc does.
>>>> 2) Or even better, do runtime PM get/put directly of the host device
>>>> from mmc_claim|release_host().
>>>
>>> Claiming the host is not directly related to host controller runtime pm. It
>>> is possible to imagine cases for claiming the host for synchronization
>>> reasons that do not need the host controller powered up. And card drivers
>>> could reasonably assume that they can claim the host for long periods
>>> without affecting the runtime pm of the host controller.
>>
>>
>> Yes, it _may_ power up the host controller sometimes when not needed.
>> I don't think this will be an issue, mostly because it should be rare
>> and not happening frequently - if ever.
>>
>> The only users of mmc_claim_host() for SD/MMC is the core itself, so I
>> don't see any issue with misbehaving "card drivers" here.
>>
>> SDIO is again different, since it's up to the SDIO func drivers to
>> behave - as you stated. But, if they don't they anyway need to be
>> fixed, right!?
> 
> Be aware that that links the holding of re-tuning with claiming the host.
> It will then not be allowed for re-tuning to be held when the host is
> released. Will we have to add to mmc_release_host():
> 
> 	WARN_ON(host->hold_count);
> 
> That could be a problem. Say you wanted to start bkops and then release the
> host so that a different context could issue an HPI. That wouldn't be
> allowed anymore.

On the other hand, we would need to prevent the host controller runtime
suspending in that case anyway.

> 
> Generally it is impossible to see all ends, which begs the question: why
> link things if you don't have to?

So I correct myself. Any time we would need to hold re-tuning but release
the host would anyway require preventing runtime suspend of the host
controller. So the requirement:	"don't allow the host controller to runtime
suspend while re-tuning is held" is covered by the requirement "don't allow
the host controller to runtime suspend when it is doing something".

> 
>>
>>>
>>> Currently we have that the host controller driver is the exclusive owner of
>>> runtime pm for the host controller. So the first thing is to decide if we
>>> want to keep that or let the core be involved as well. I would argue for
>>> sticking with the status quo. Let the host controller driver know what is
>>> going on and leave it to do the right thing with its own power management.
>>
>> The problem I see with the current solution, is that we will be
>> scheduling a runtime PM suspend for each an every request towards the
>> host.
>>
>> It's ineffective, due to that we will unnecessary involve the runtime
>> PM core, thus increasing CPU utilization - when we actually don't need
>> to.
>>
>> I will send a patch for this tomorrow, let's discuss it separately.
> 
> Yes please let's keep that separate.
> 
>>
>> [...]
>>
>>>>
>>>> I have thought more about this since yesterday and I somewhat agree
>>>> with your suggestion. Especially in that sense that I think we should
>>>> consider Neil's issue as an "idle operation" for SDIO.
>>>>
>>>> For "idle operations" for MMC/SD cards, runtime PM is already
>>>> deployed. So, I think it's just a matter of extending that support to
>>>> cover more "idle operations" besides the MMC_CAP_AGGRESSIVE_PM thing.
>>>>
>>>> What we need to figure out is how to make this work for SDIO - and
>>>> that's when it becomes more complex. I really would like us to avoid
>>>> exporting new SDIO APIs, unless really needed.
>>>>
>>>> Today runtime PM is deployed by the following SDIO func drivers:
>>>> drivers/net/wireless/libertas/if_sdio.c
>>>> drivers/net/wireless/ti/wl1251/sdio.c
>>>> drivers/net/wireless/ti/wlcore/sdio.c
>>>>
>>>> We _could_ consider to convert above drivers to use something else
>>>> than runtime PM to control the power to the card. I think that would
>>>> be the ideal solution, since then we can deploy runtime PM for SDIO
>>>> fairly similar to how MMC/SDIO is done. That means doing runtime PM
>>>> get/put of the device for the struct mmc_card, inside the mmc core
>>>> while handling SDIO requests.
>>>>
>>>> The above requires some work, both in the mmc core and in the SDIO
>>>> func drivers. The nice thing is that we don't need to export new APIs
>>>> to the SDIO func drivers and we can keep the complexity of dealing
>>>> with "idle operations" inside the mmc core.
>>>>
>>>> Thoughts?
>>>
>>> There isn't necessarily any link between "idle operations" and runtime pm.
>>
>> I think this is exactly what runtime PM is designed for so I don't
>> want us to re-invent something that is specific for mmc.
> 
> Need to remember that PM can theoretically be configured out, which makes
> using it for bkops seem inappropriate.
> 
>>
>>>
>>> For example for eMMC background operations I would expect to see:
>>>
>>> - queue is empty so block driver enables "idle operations".
>>> - core waits (delayed work?) a few milliseconds and then starts bkops.
>>> - a new I/O request arrives, block driver wakes up and tells the core to
>>> stop "idle operations" ASAP, and waits until it does.
>>> - or, the core notifies (callback perhaps) the block driver that "idle
>>> operations" are finished, so the block driver can runtime-put the card
>>>
>>> Also need to stop "idle operations" for system suspend, maybe other places.
>>
>> Conceptual wise, I fully agree. Though, I want to make use of runtime PM.
> 
> So long as it is power management. I am not sure bkops falls into that category.
> 
>>
>> For the eMMC/SD case, the runtime PM suspend callbacks (specified per
>> bus_ops) should be able to act as the "trigger" point to kick off
>> "idle operations".
>>
>> Now, the thing to figure out, is how to execute "idle operations" and
>> at the same time being able to interrupt/abort them from another
>> context. An option for how to deal with this, could be to schedule a
>> "delayed work" from the runtime PM suspend callback. But if we can
>> avoid it, I think we should.
> 
> Whatever the context that runs the idle operations, for the case where a
> driver wants to stop the idle operations ASAP, it would be nice if it could
> simply take over control - particularly if the idle operation is anyway
> waiting for something. So the HPI or whatever is done in the driver context
> directly and the idle operation context (if there even is one at that stage)
> just exits.
> 
>>
>>>
>>> Now in Neil's case there is a relation to runtime pm in that it would be
>>> nice if the switch to 1-bit mode was delayed by the host controllers
>>> autosuspend_delay. But potentially the host controller driver could
>>> routinely set the delay so that it matches the autosuspend_delay anyway.
>>>
>>> Currently the SDIO function drivers all use sdio_claim_host(). So for Neil's
>>> case I would see:
>>>
>>> - host controller driver sees the switch to 4-bit mode and does a runtime
>>> "get" to prevent runtime suspend
>>> - sdio_release_host enables "idle operations"
>>> - the core sees that someone has requested a switch to 1-bit mode after a
>>> certain delay, so it waits that delay (delayed work?) and does the switch
>>> - host controller driver sees the switch to 1-bit mode and runtime suspends
>>> via a pm_runtime_put
>>> - sdio_claim_host tells the core to stop "idle operations" ASAP and waits
>>> until it has
>>> - host controller driver sees the switch to 4-bit mode and does a runtime
>>> "get" to prevent runtime suspend
>>
>> That seems like a way forward!
>>
>> Still I rather would like the above mentioned SDIO func drivers to use
>> something else than runtime PM to control the power to the cards, as I
>> suggested. But that might be too much to fix!?
> 
> Let's keep that a separate issue.
> 
> 
> 

Re: [PATCH V2 11/15] mmc: sdhci: Change to new way of doing re-tuning

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3137 bytes --]

On Mon, 23 Mar 2015 16:26:07 +0200 Adrian Hunter <adrian.hunter@intel.com>
wrote:

> For Neil's problem I would do something quiet different:
> 
> 1. The host driver already knows the bus width so can easily "get/put"
> runtime pm to prevent suspend when the bus width does not permit it.
> 
> 2. The need to do things when the card is idle comes up a lot (e.g. bkops,
> sleep notification, cache flush etc etc). In Neil's case he wants to switch
> to 1-bit mode, but that just seems another of these "idle" operations. So I
> would investigate the requirements of supporting idle operations in general.
> 

Hi,
 I agree that what I want to achieve could be characterised as an "idle"
 operation.
 I also happen to think that runtime PM is designed to support "idle"
 operations - it can track when a device goes 'idle' and "do the right thing".
 It handles all the needed ref counting and races with resume etc.  So I
 think it should be used to manage these "idle" operations.

 The difficulty is that in the naive approach, we want to do something in the
 "runtime_suspend" operation which actually uses the device.  So it has to be
 non-idle in order to go idle.  I agree that this can appear clumsy.

 What we effectively have here is two levels of "idle".  First the "host"
 goes idle in that no new commands are arriving and no interrupts have
 happened.  In response to this the host sends a few final commands to the
 controller and then the controller goes idle.

 This two-stage process should be able to be modelled with the two levels of
 device: the mmc_host class device and the platform device which controls the
 hardware.  e.g. mmc1 and omap_hsmmc.1 on my board.

 So this is (roughly) what I would do if I wanted to implement fully general
 "idle" operations for mmc cards.

 1/ enable runtime_pm on the host device - in mmc_add_host.
   I think pm_suspend_ignore_children() would be needed so that the host
   can go to sleep while the card is still active.
 2/ Use pm_runtime_get / pm_runtime_put_autosuspend in mmc_claim_host
    and mmc_release_host.
 3/ Remove the ->enable and  ->disable calls from mmc_{claim,release}_host.
    I think they only affect omap_hsmmc and it only uses them for runtime
    PM, which now will happen automatically.
 4/ In the 'runtime_suspend' method for mmc_host, take a runtime_pm reference
    on the platform device and schedule a work item to run the "idle"
    operations


 When the "idle" operations complete, the runtime_pm reference will be
 dropped and then - having no active children or references - the
 platform device will go to sleep (stop its clocks or whatever).
 When something then calls mmc_claim_host(), the "idle" operations can be
 undone, either directly or via the runtime_resume operation.

This would be a fairly intrusive change.  I'm happy to try to find time to
write bits of it if there is general agreement, but I cannot test anything
other than omap_hsmmc, and I don't know any details about any other possible
"idle" operations so I would need input from someone who does.

NeilBrown

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH V2 11/15] mmc: sdhci: Change to new way of doing re-tuning

[Ulf Hansson]

On 24 March 2015 at 03:49, NeilBrown <neilb@suse.de> wrote:
> On Mon, 23 Mar 2015 16:26:07 +0200 Adrian Hunter <adrian.hunter@intel.com>
> wrote:
>
>> For Neil's problem I would do something quiet different:
>>
>> 1. The host driver already knows the bus width so can easily "get/put"
>> runtime pm to prevent suspend when the bus width does not permit it.
>>
>> 2. The need to do things when the card is idle comes up a lot (e.g. bkops,
>> sleep notification, cache flush etc etc). In Neil's case he wants to switch
>> to 1-bit mode, but that just seems another of these "idle" operations. So I
>> would investigate the requirements of supporting idle operations in general.
>>
>
> Hi,
>  I agree that what I want to achieve could be characterised as an "idle"
>  operation.
>  I also happen to think that runtime PM is designed to support "idle"
>  operations - it can track when a device goes 'idle' and "do the right thing".
>  It handles all the needed ref counting and races with resume etc.  So I
>  think it should be used to manage these "idle" operations.
>
>  The difficulty is that in the naive approach, we want to do something in the
>  "runtime_suspend" operation which actually uses the device.  So it has to be
>  non-idle in order to go idle.  I agree that this can appear clumsy.
>
>  What we effectively have here is two levels of "idle".  First the "host"
>  goes idle in that no new commands are arriving and no interrupts have
>  happened.  In response to this the host sends a few final commands to the
>  controller and then the controller goes idle.
>
>  This two-stage process should be able to be modelled with the two levels of
>  device: the mmc_host class device and the platform device which controls the
>  hardware.  e.g. mmc1 and omap_hsmmc.1 on my board.
>
>  So this is (roughly) what I would do if I wanted to implement fully general
>  "idle" operations for mmc cards.
>
>  1/ enable runtime_pm on the host device - in mmc_add_host.
>    I think pm_suspend_ignore_children() would be needed so that the host
>    can go to sleep while the card is still active.
>  2/ Use pm_runtime_get / pm_runtime_put_autosuspend in mmc_claim_host
>     and mmc_release_host.
>  3/ Remove the ->enable and  ->disable calls from mmc_{claim,release}_host.
>     I think they only affect omap_hsmmc and it only uses them for runtime
>     PM, which now will happen automatically.
>  4/ In the 'runtime_suspend' method for mmc_host, take a runtime_pm reference
>     on the platform device and schedule a work item to run the "idle"
>     operations
>
>
>  When the "idle" operations complete, the runtime_pm reference will be
>  dropped and then - having no active children or references - the
>  platform device will go to sleep (stop its clocks or whatever).
>  When something then calls mmc_claim_host(), the "idle" operations can be
>  undone, either directly or via the runtime_resume operation.

I don't think this will work.

The problem is that the scheduled work which performs the idle
operations will invoke mmc_claim|release_host() when it's about to
execute the  "idle commands/requests".

>
> This would be a fairly intrusive change.  I'm happy to try to find time to
> write bits of it if there is general agreement, but I cannot test anything
> other than omap_hsmmc, and I don't know any details about any other possible
> "idle" operations so I would need input from someone who does.
>
> NeilBrown

Kind regards
Uffe

[PATCH V2 12/15] mmc: sdhci: Flag re-tuning is needed on CRC or End-Bit errors

[Adrian Hunter]

CRC or End-Bit errors could possibly be alleviated by
re-tuning so flag re-tuning needed in those cases.
Note this has no effect if re-tuning has not been
enabled.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index dd0be76..06c5b96 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2320,8 +2320,10 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask, u32 *mask)
 	if (intmask & SDHCI_INT_TIMEOUT)
 		host->cmd->error = -ETIMEDOUT;
 	else if (intmask & (SDHCI_INT_CRC | SDHCI_INT_END_BIT |
-			SDHCI_INT_INDEX))
+			SDHCI_INT_INDEX)) {
 		host->cmd->error = -EILSEQ;
+		mmc_retune_needed(host->mmc);
+	}
 
 	if (host->cmd->error) {
 		tasklet_schedule(&host->finish_tasklet);
@@ -2446,13 +2448,15 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
 
 	if (intmask & SDHCI_INT_DATA_TIMEOUT)
 		host->data->error = -ETIMEDOUT;
-	else if (intmask & SDHCI_INT_DATA_END_BIT)
+	else if (intmask & SDHCI_INT_DATA_END_BIT) {
 		host->data->error = -EILSEQ;
-	else if ((intmask & SDHCI_INT_DATA_CRC) &&
+		mmc_retune_needed(host->mmc);
+	} else if ((intmask & SDHCI_INT_DATA_CRC) &&
 		SDHCI_GET_CMD(sdhci_readw(host, SDHCI_COMMAND))
-			!= MMC_BUS_TEST_R)
+			!= MMC_BUS_TEST_R) {
 		host->data->error = -EILSEQ;
-	else if (intmask & SDHCI_INT_ADMA_ERROR) {
+		mmc_retune_needed(host->mmc);
+	} else if (intmask & SDHCI_INT_ADMA_ERROR) {
 		pr_err("%s: ADMA error\n", mmc_hostname(host->mmc));
 		sdhci_adma_show_error(host);
 		host->data->error = -EIO;
-- 
1.9.1

[PATCH V2 13/15] mmc: block: Check re-tuning in the recovery path

[Adrian Hunter]

If re-tuning is needed, do it in the recovery path to
give recovery commands a better chance of success.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/card/block.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index c69afb5..293e938 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -913,6 +913,9 @@ static int mmc_blk_cmd_recovery(struct mmc_card *card, struct request *req,
 		if (!err)
 			break;
 
+		/* Re-tune if needed */
+		mmc_retune_recheck(card->host);
+
 		prev_cmd_status_valid = false;
 		pr_err("%s: error %d sending status command, %sing\n",
 		       req->rq_disk->disk_name, err, retry ? "retry" : "abort");
-- 
1.9.1

[PATCH V2 14/15] mmc: block: Retry data requests when re-tuning is needed

[Adrian Hunter]

Retry data requests when re-tuning is needed and
add a flag to struct mmc_blk_request so that the retry
is only done once.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/card/block.c | 12 +++++++++++-
 drivers/mmc/card/queue.h |  1 +
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 293e938..6d873d6 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1195,6 +1195,7 @@ static int mmc_blk_err_check(struct mmc_card *card,
 						    mmc_active);
 	struct mmc_blk_request *brq = &mq_mrq->brq;
 	struct request *req = mq_mrq->req;
+	int need_retune = card->host->need_retune;
 	int ecc_err = 0, gen_err = 0;
 
 	/*
@@ -1261,6 +1262,13 @@ static int mmc_blk_err_check(struct mmc_card *card,
 		return MMC_BLK_RETRY;
 	}
 
+	if (need_retune && !brq->retune_retry_done) {
+		pr_info("%s: retrying because a re-tune was needed\n",
+			req->rq_disk->disk_name);
+		brq->retune_retry_done = 1;
+		return MMC_BLK_RETRY;
+	}
+
 	if (brq->data.error) {
 		pr_err("%s: error %d transferring data, sector %u, nr %u, cmd response %#x, card status %#x\n",
 		       req->rq_disk->disk_name, brq->data.error,
@@ -1821,7 +1829,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 	struct mmc_blk_data *md = mq->data;
 	struct mmc_card *card = md->queue.card;
 	struct mmc_blk_request *brq = &mq->mqrq_cur->brq;
-	int ret = 1, disable_multi = 0, retry = 0, type;
+	int ret = 1, disable_multi = 0, retry = 0, type, retune_retry_done = 0;
 	enum mmc_blk_status status;
 	struct mmc_queue_req *mq_rq;
 	struct request *req = rqc;
@@ -1905,6 +1913,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 				break;
 			goto cmd_abort;
 		case MMC_BLK_RETRY:
+			retune_retry_done = brq->retune_retry_done;
 			if (retry++ < 5)
 				break;
 			/* Fall through */
@@ -1967,6 +1976,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 				mmc_start_req(card->host,
 						&mq_rq->mmc_active, NULL);
 			}
+			mq_rq->brq.retune_retry_done = retune_retry_done;
 		}
 	} while (ret);
 
diff --git a/drivers/mmc/card/queue.h b/drivers/mmc/card/queue.h
index 5752d50..7e27915 100644
--- a/drivers/mmc/card/queue.h
+++ b/drivers/mmc/card/queue.h
@@ -12,6 +12,7 @@ struct mmc_blk_request {
 	struct mmc_command	cmd;
 	struct mmc_command	stop;
 	struct mmc_data		data;
+	int			retune_retry_done;
 };
 
 enum mmc_packed_type {
-- 
1.9.1

[PATCH V3 14/15] mmc: block: Retry errored data requests when re-tuning is needed

[Adrian Hunter]

Retry errored data requests when re-tuning is needed and
add a flag to struct mmc_blk_request so that the retry
is only done once.

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

Changes in V3:

	Only retry when there is an error

 drivers/mmc/card/block.c | 11 ++++++++++-
 drivers/mmc/card/queue.h |  1 +
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 293e938..3baf7ea 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1195,6 +1195,7 @@ static int mmc_blk_err_check(struct mmc_card *card,
 						    mmc_active);
 	struct mmc_blk_request *brq = &mq_mrq->brq;
 	struct request *req = mq_mrq->req;
+	int need_retune = card->host->need_retune;
 	int ecc_err = 0, gen_err = 0;
 
 	/*
@@ -1262,6 +1263,12 @@ static int mmc_blk_err_check(struct mmc_card *card,
 	}
 
 	if (brq->data.error) {
+		if (need_retune && !brq->retune_retry_done) {
+			pr_info("%s: retrying because a re-tune was needed\n",
+				req->rq_disk->disk_name);
+			brq->retune_retry_done = 1;
+			return MMC_BLK_RETRY;
+		}
 		pr_err("%s: error %d transferring data, sector %u, nr %u, cmd response %#x, card status %#x\n",
 		       req->rq_disk->disk_name, brq->data.error,
 		       (unsigned)blk_rq_pos(req),
@@ -1821,7 +1828,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 	struct mmc_blk_data *md = mq->data;
 	struct mmc_card *card = md->queue.card;
 	struct mmc_blk_request *brq = &mq->mqrq_cur->brq;
-	int ret = 1, disable_multi = 0, retry = 0, type;
+	int ret = 1, disable_multi = 0, retry = 0, type, retune_retry_done = 0;
 	enum mmc_blk_status status;
 	struct mmc_queue_req *mq_rq;
 	struct request *req = rqc;
@@ -1905,6 +1912,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 				break;
 			goto cmd_abort;
 		case MMC_BLK_RETRY:
+			retune_retry_done = brq->retune_retry_done;
 			if (retry++ < 5)
 				break;
 			/* Fall through */
@@ -1967,6 +1975,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 				mmc_start_req(card->host,
 						&mq_rq->mmc_active, NULL);
 			}
+			mq_rq->brq.retune_retry_done = retune_retry_done;
 		}
 	} while (ret);
 
diff --git a/drivers/mmc/card/queue.h b/drivers/mmc/card/queue.h
index 5752d50..7e27915 100644
--- a/drivers/mmc/card/queue.h
+++ b/drivers/mmc/card/queue.h
@@ -12,6 +12,7 @@ struct mmc_blk_request {
 	struct mmc_command	cmd;
 	struct mmc_command	stop;
 	struct mmc_data		data;
+	int			retune_retry_done;
 };
 
 enum mmc_packed_type {
-- 
1.9.1

[PATCH V2 15/15] mmc: core: Don't print reset warning if reset is not supported

[Adrian Hunter]

Check the error code for EOPNOTSUPP and do not print
reset warning in that case.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 392a150..d439bf0 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2368,7 +2368,8 @@ int mmc_hw_reset(struct mmc_host *host)
 	ret = host->bus_ops->reset(host);
 	mmc_bus_put(host);
 
-	pr_warn("%s: tried to reset card\n", mmc_hostname(host));
+	if (ret != -EOPNOTSUPP)
+		pr_warn("%s: tried to reset card\n", mmc_hostname(host));
 
 	return ret;
 }
-- 
1.9.1

Re: [PATCH V2 15/15] mmc: core: Don't print reset warning if reset is not supported

[Arend van Spriel]

On 01/29/15 10:00, Adrian Hunter wrote:
> Check the error code for EOPNOTSUPP and do not print
> reset warning in that case.
>
> Signed-off-by: Adrian Hunter<adrian.hunter@intel.com>
> ---
>   drivers/mmc/core/core.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 392a150..d439bf0 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2368,7 +2368,8 @@ int mmc_hw_reset(struct mmc_host *host)
>   	ret = host->bus_ops->reset(host);
>   	mmc_bus_put(host);
>
> -	pr_warn("%s: tried to reset card\n", mmc_hostname(host));
> +	if (ret != -EOPNOTSUPP)
> +		pr_warn("%s: tried to reset card\n", mmc_hostname(host));

Guess you don't want this message either if ret is zero.

Gr. Avs

>   	return ret;
>   }

Re: [PATCH V2 15/15] mmc: core: Don't print reset warning if reset is not supported

[Adrian Hunter]

On 09/02/15 11:33, Arend van Spriel wrote:
> On 01/29/15 10:00, Adrian Hunter wrote:
>> Check the error code for EOPNOTSUPP and do not print
>> reset warning in that case.
>>
>> Signed-off-by: Adrian Hunter<adrian.hunter@intel.com>
>> ---
>>   drivers/mmc/core/core.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index 392a150..d439bf0 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -2368,7 +2368,8 @@ int mmc_hw_reset(struct mmc_host *host)
>>       ret = host->bus_ops->reset(host);
>>       mmc_bus_put(host);
>>
>> -    pr_warn("%s: tried to reset card\n", mmc_hostname(host));
>> +    if (ret != -EOPNOTSUPP)
>> +        pr_warn("%s: tried to reset card\n", mmc_hostname(host));
> 
> Guess you don't want this message either if ret is zero.

Thanks for your comment.

I assumed the original author wants to see the message whenever reset is
attempted, which is OK by me because it is on the recovery path i.e.
hopefully rare. The EOPNOTSUPP case is consistent with the code further up
which returns EOPNOTSUPP when there is no ->reset() callback.

Re: [PATCH V2 15/15] mmc: core: Don't print reset warning if reset is not supported

[Johan Rudholm]

Hi Adrian and Arend,

2015-02-09 10:47 GMT+01:00 Adrian Hunter <adrian.hunter@intel.com>:
> On 09/02/15 11:33, Arend van Spriel wrote:
>> On 01/29/15 10:00, Adrian Hunter wrote:
>>> Check the error code for EOPNOTSUPP and do not print
>>> reset warning in that case.
>>>
>>> Signed-off-by: Adrian Hunter<adrian.hunter@intel.com>
>>> ---
>>>   drivers/mmc/core/core.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> index 392a150..d439bf0 100644
>>> --- a/drivers/mmc/core/core.c
>>> +++ b/drivers/mmc/core/core.c
>>> @@ -2368,7 +2368,8 @@ int mmc_hw_reset(struct mmc_host *host)
>>>       ret = host->bus_ops->reset(host);
>>>       mmc_bus_put(host);
>>>
>>> -    pr_warn("%s: tried to reset card\n", mmc_hostname(host));
>>> +    if (ret != -EOPNOTSUPP)
>>> +        pr_warn("%s: tried to reset card\n", mmc_hostname(host));
>>
>> Guess you don't want this message either if ret is zero.
>
> Thanks for your comment.
>
> I assumed the original author wants to see the message whenever reset is
> attempted, which is OK by me because it is on the recovery path i.e.
> hopefully rare. The EOPNOTSUPP case is consistent with the code further up
> which returns EOPNOTSUPP when there is no ->reset() callback.

Speaking as the original author, yes, this was the idea. Thank you for
this patch.

BR, Johan

Re: [PATCH V2 00/15] mmc: host: Add facility to support re-tuning

[Adrian Hunter]

On 29/01/15 11:00, Adrian Hunter wrote:
> Hi
> 
> Here is V2 of some patches to move re-tuning support
> out of sdhci and into the core, and add support for HS400
> re-tuning.
> 
> Currently sdhci does re-tuning transparently by
> calling sdhci_execute_tuning() from its ->request()
> function.
> 
> The problem with HS400 re-tuning is that it must be
> done in HS200 mode. That means using switch commands
> and making ios changes. That means it potentially
> conflicts with other command sequences. The new
> re-tuning support accomodates that, however it is
> not strictly necessary because HS400 re-tuning
> should only be needed after runtime suspend when
> there would not be a conflict.
> 
> Nevertheless, this approach has more structure
> and potentially more flexibility.
> 
> Changes in V2:
> 
>     Added support to the block driver for re-tuning
>     and retrying after a CRC error. The host driver
>     is left to decide when an error indicates re-tuning
>     is needed. The block driver will retry a data request
>     once if re-tuning is flagged as needed.
> 
>     SDIO drivers need not be aware of re-tuning because
>     retrying will anyway cause re-tuning when re-tuning
>     is flagged as needed. Nevertheless SDIO drivers could
>     use the need_retune flag to instigate a retry when
>     otherwise they might not have.

Ulf, Arend, is this OK?