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

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

[Adrian Hunter]

Hi

Here is V4 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.

These patches now depend on Ulf's patch:

    mmc: core: Enable runtime PM management of host devices


Changes in V4:

    mmc: host: Add facility to support re-tuning

	Assume mmc_claim_host() runtime resumes the host
	controller so there are no races with runtime pm.
	Consequently remove now un-needed re-tuning host
	operations.

    mmc: core: Add support for re-tuning before each request

	Call mmc_retune() prior to ->request()

    mmc: sdhci: Change to new way of doing re-tuning

	Updated to reflect the changes above.

Changes in V3:

    mmc: host: Add facility to support re-tuning

        Add host->retune_now flag so re-tuning can be
        started in the host drivers ->request()
        function to avoid racing with Runtime PM.

        Add host operations to let the host know when
        re-tuning is held, for eaxmple, to enable
        synchronization with runtime suspend / resume.

        Ensure functions are exported.

    mmc: core: Add support for re-tuning before each request
        Updated to reflect the change above.

    mmc: core: Check re-tuning before retrying
        Updated to reflect the change above.

    mmc: core: Hold re-tuning during switch commands
        Updated to reflect the change above.

    mmc: core: Hold re-tuning during erase commands
        Updated to reflect the change above.

    mmc: core: Hold re-tuning while bkops ongoing
        Updated to reflect the change above.

    mmc: core: Add support for HS400 re-tuning
        Updated and as already sent separately as V3:
        Remember to mmc_set_bus_speed(card) in mmc_hs400_to_hs200()

    mmc: sdhci: Change to new way of doing re-tuning
        Call mmc_retune() from ->request() function to
        avoid racing with Runtime PM. And implement
        hold_tuning / release_tuning operations to prevent
        runtime suspend while re-tuning is held.

    mmc: block: Retry errored data requests when re-tuning is needed
        Updated and as already sent separately as V3:
        Only retry when there is an error

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 errored data requests when re-tuning is needed
      mmc: core: Don't print reset warning if reset is not supported

 drivers/mmc/card/block.c   |  14 ++++-
 drivers/mmc/card/queue.h   |   1 +
 drivers/mmc/core/core.c    |  49 ++++++++++++++---
 drivers/mmc/core/core.h    |   2 +
 drivers/mmc/core/host.c    |  89 +++++++++++++++++++++++++++++++
 drivers/mmc/core/mmc.c     |  89 +++++++++++++++++++++++++++++++
 drivers/mmc/core/mmc_ops.c |  43 +++++++++------
 drivers/mmc/core/mmc_ops.h |   1 +
 drivers/mmc/host/sdhci.c   | 127 ++++++++-------------------------------------
 drivers/mmc/host/sdhci.h   |   3 --
 include/linux/mmc/host.h   |  32 ++++++++++++
 11 files changed, 318 insertions(+), 132 deletions(-)

Regards
Adrian

[PATCH V4 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 6 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 */
  unsigned int		retune_now:1;   /* do re-tuning at next req */
  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  | 72 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mmc/host.h | 32 +++++++++++++++++++++
 2 files changed, 104 insertions(+)

diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 8be0df7..ce0ddf7 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -301,6 +301,77 @@ 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);
+}
+EXPORT_SYMBOL(mmc_retune_enable);
+
+void mmc_retune_disable(struct mmc_host *host)
+{
+	host->can_retune = 0;
+	del_timer_sync(&host->retune_timer);
+	host->retune_now = 0;
+	host->need_retune = 0;
+}
+EXPORT_SYMBOL(mmc_retune_disable);
+
+void mmc_retune_timer_stop(struct mmc_host *host)
+{
+	del_timer_sync(&host->retune_timer);
+}
+EXPORT_SYMBOL(mmc_retune_timer_stop);
+
+void mmc_retune_hold(struct mmc_host *host)
+{
+	if (!host->hold_retune)
+		host->retune_now = 1;
+	host->hold_retune += 1;
+}
+EXPORT_SYMBOL(mmc_retune_hold);
+
+void mmc_retune_release(struct mmc_host *host)
+{
+	if (host->hold_retune)
+		host->hold_retune -= 1;
+	else
+		WARN_ON(1);
+}
+EXPORT_SYMBOL(mmc_retune_release);
+
+int mmc_retune(struct mmc_host *host)
+{
+	int err;
+
+	if (host->retune_now)
+		host->retune_now = 0;
+	else
+		return 0;
+
+	if (!host->need_retune || host->doing_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;
+}
+EXPORT_SYMBOL(mmc_retune);
+
+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 +575,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 b5bedae..f8957eb 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>
@@ -321,10 +322,17 @@ 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 */
+	unsigned int		retune_now:1;	/* do re-tuning at next req */
 
 	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 */
@@ -513,4 +521,28 @@ 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);
+void mmc_retune_hold(struct mmc_host *host);
+void mmc_retune_release(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_recheck(struct mmc_host *host)
+{
+	if (host->hold_retune <= 1)
+		host->retune_now = 1;
+}
+
 #endif /* LINUX_MMC_HOST_H */
-- 
1.9.1

[PATCH V4 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 c296bc0..2679264 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1140,6 +1140,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 V4 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 | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 2679264..bb50d82 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -186,12 +186,29 @@ void mmc_request_done(struct mmc_host *host, struct mmc_request *mrq)
 
 EXPORT_SYMBOL(mmc_request_done);
 
+static void __mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
+{
+	int err;
+
+	/* Assumes host controller has been runtime resumed by mmc_claim_host */
+	err = mmc_retune(host);
+	if (err) {
+		mrq->cmd->error = err;
+		mmc_request_done(host, mrq);
+		return;
+	}
+
+	host->ops->request(host, mrq);
+}
+
 static int mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
 {
 #ifdef CONFIG_MMC_DEBUG
 	unsigned int i, sz;
 	struct scatterlist *sg;
 #endif
+	mmc_retune_hold(host);
+
 	if (mmc_card_removed(host->card))
 		return -ENOMEDIUM;
 
@@ -252,7 +269,7 @@ static int mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
 	}
 	mmc_host_clk_hold(host);
 	led_trigger_event(host->led, LED_FULL);
-	host->ops->request(host, mrq);
+	__mmc_start_request(host, mrq);
 
 	return 0;
 }
@@ -422,17 +439,16 @@ static int mmc_wait_for_data_req_done(struct mmc_host *host,
 					cmd->opcode, cmd->error);
 				cmd->retries--;
 				cmd->error = 0;
-				host->ops->request(host, mrq);
+				__mmc_start_request(host, mrq);
 				continue; /* wait for done/new event again */
 			}
 		} 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;
 }
 
@@ -471,8 +487,10 @@ static void mmc_wait_for_req_done(struct mmc_host *host,
 			 mmc_hostname(host), cmd->opcode, cmd->error);
 		cmd->retries--;
 		cmd->error = 0;
-		host->ops->request(host, mrq);
+		__mmc_start_request(host, mrq);
 	}
+
+	mmc_retune_release(host);
 }
 
 /**
-- 
1.9.1

[PATCH V4 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() flags host->retune_now if the hold
count is 1.

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

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index bb50d82..16881c8 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -434,6 +434,7 @@ static int mmc_wait_for_data_req_done(struct mmc_host *host,
 							    host->areq);
 				break; /* return err */
 			} else {
+				mmc_retune_recheck(host);
 				pr_info("%s: req failed (CMD%u): %d, retrying...\n",
 					mmc_hostname(host),
 					cmd->opcode, cmd->error);
@@ -483,6 +484,8 @@ static void mmc_wait_for_req_done(struct mmc_host *host,
 		    mmc_card_removed(host->card))
 			break;
 
+		mmc_retune_recheck(host);
+
 		pr_debug("%s: req failed (CMD%u): %d, retrying...\n",
 			 mmc_hostname(host), cmd->opcode, cmd->error);
 		cmd->retries--;
-- 
1.9.1

[PATCH V4 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 | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 0ea042d..f67b0fd 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -474,6 +474,8 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 	u32 status = 0;
 	bool use_r1b_resp = use_busy_signal;
 
+	mmc_retune_hold(host);
+
 	/*
 	 * 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 +508,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 +531,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 +545,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 V4 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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 16881c8..d4e3248 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1993,6 +1993,8 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from,
 	unsigned long timeout;
 	int err;
 
+	mmc_retune_hold(card->host);
+
 	/*
 	 * qty is used to calculate the erase timeout which depends on how many
 	 * erase groups (or allocation units in SD terminology) are affected.
@@ -2096,6 +2098,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 V4 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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index d4e3248..f8ae793 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -316,6 +316,8 @@ void mmc_start_bkops(struct mmc_card *card, bool from_exception)
 	} else {
 		timeout = 0;
 		use_busy_signal = false;
+		/* Hold re-tuning for ongoing bkops */
+		mmc_retune_hold(card->host);
 	}
 
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
@@ -324,6 +326,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;
 	}
 
@@ -749,6 +754,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 V4 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 1d41e85..813b02a 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1496,6 +1496,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 V4 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 f67b0fd..02be77e 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
@@ -557,20 +572,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 V4 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  | 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 ce0ddf7..9361312 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -343,6 +343,7 @@ EXPORT_SYMBOL(mmc_retune_release);
 
 int mmc_retune(struct mmc_host *host)
 {
+	bool return_to_hs400 = false;
 	int err;
 
 	if (host->retune_now)
@@ -357,8 +358,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 V4 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 ++++++-----------------------------------------
 drivers/mmc/host/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 c80287a..c9a34b5 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -52,7 +52,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,
@@ -254,17 +253,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);
 }
 
@@ -1353,7 +1341,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);
 
@@ -1387,39 +1374,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
@@ -2065,23 +2019,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);
@@ -2337,20 +2287,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                                                        *
@@ -2728,11 +2664,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;
@@ -2782,10 +2715,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;
 }
 
@@ -2822,11 +2751,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;
@@ -2869,10 +2795,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;
@@ -3408,13 +3330,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/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index e639b7f..923ff22 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -432,13 +432,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 */
 
@@ -504,7 +502,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

[PATCH V4 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 c9a34b5..e6125e9 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2308,8 +2308,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);
@@ -2434,13 +2436,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 V4 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 V4 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>
---
 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 V4 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 f8ae793..595be30 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2363,7 +2363,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 V4 00/15] mmc: host: Add facility to support re-tuning

[Adrian Hunter]

On 27/03/15 22:57, Adrian Hunter wrote:
> Hi
> 
> Here is V4 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.
> 
> These patches now depend on Ulf's patch:
> 
>     mmc: core: Enable runtime PM management of host devices
> 
> 
> Changes in V4:
> 
>     mmc: host: Add facility to support re-tuning
> 
> 	Assume mmc_claim_host() runtime resumes the host
> 	controller so there are no races with runtime pm.
> 	Consequently remove now un-needed re-tuning host
> 	operations.
> 
>     mmc: core: Add support for re-tuning before each request
> 
> 	Call mmc_retune() prior to ->request()
> 
>     mmc: sdhci: Change to new way of doing re-tuning
> 
> 	Updated to reflect the changes above.

Hi

What is the status of this?

Regards
Adrian

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

[Ulf Hansson]

On 27 March 2015 at 21:57, Adrian Hunter <adrian.hunter@intel.com> wrote:
> 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:

It's a bit hard to follow why these requirements.

I am giving some comments below, also for my own reference. Please
tell me if I am way off.

>   - ability to enable / disable re-tuning

Handled internally by the mmc core.

Or maybe SDIO func drivers needs an API to control this?

>   - ability to flag that re-tuning is needed

Both from mmc core and mmc host point of view.

>   - ability to re-tune before any request

Internal mechanism by the core.

>   - ability to hold off re-tuning if the card is busy

What do you mean by "card is busy"?

Is this requirement due to that the re-tune timer may complete at any
point, which then flags that a re-tune is needed?

>   - ability to hold off re-tuning if re-tuning is in
>   progress

This I don't understand. How can a re-tune sequence be triggered when
there is already a request ongoing towards the host. I mean the host
is claimed during the re-tuning process, so why do we need one more
layer of protection?

>   - ability to run a re-tuning timer

As we discussed earlier, it's not obvious when it make sense to run this timer.

For the SDIO case, we should likely run it all the time, since we want
to prevent I/O errors as long as possible.

For MMC/SD, I would rather see that we perform re-tune as a part of an
"idle operation" and not from a timer (yes I know about the SDHCI
spec, but it doesn't make sense). We can do this because we are able
to re-cover from I/O errors (re-trying when getting CRC errors for
example).

I will continue to review the rest of series in more detail.

Kind regards
Uffe

>
> To support those requirements 6 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 */
>   unsigned int          retune_now:1;   /* do re-tuning at next req */
>   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  | 72 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mmc/host.h | 32 +++++++++++++++++++++
>  2 files changed, 104 insertions(+)
>
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 8be0df7..ce0ddf7 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -301,6 +301,77 @@ 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);
> +}
> +EXPORT_SYMBOL(mmc_retune_enable);
> +
> +void mmc_retune_disable(struct mmc_host *host)
> +{
> +       host->can_retune = 0;
> +       del_timer_sync(&host->retune_timer);
> +       host->retune_now = 0;
> +       host->need_retune = 0;
> +}
> +EXPORT_SYMBOL(mmc_retune_disable);
> +
> +void mmc_retune_timer_stop(struct mmc_host *host)
> +{
> +       del_timer_sync(&host->retune_timer);
> +}
> +EXPORT_SYMBOL(mmc_retune_timer_stop);
> +
> +void mmc_retune_hold(struct mmc_host *host)
> +{
> +       if (!host->hold_retune)
> +               host->retune_now = 1;
> +       host->hold_retune += 1;
> +}
> +EXPORT_SYMBOL(mmc_retune_hold);
> +
> +void mmc_retune_release(struct mmc_host *host)
> +{
> +       if (host->hold_retune)
> +               host->hold_retune -= 1;
> +       else
> +               WARN_ON(1);
> +}
> +EXPORT_SYMBOL(mmc_retune_release);
> +
> +int mmc_retune(struct mmc_host *host)
> +{
> +       int err;
> +
> +       if (host->retune_now)
> +               host->retune_now = 0;
> +       else
> +               return 0;
> +
> +       if (!host->need_retune || host->doing_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;
> +}
> +EXPORT_SYMBOL(mmc_retune);
> +
> +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 +575,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 b5bedae..f8957eb 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>
> @@ -321,10 +322,17 @@ 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 */
> +       unsigned int            retune_now:1;   /* do re-tuning at next req */
>
>         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 */
> @@ -513,4 +521,28 @@ 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);
> +void mmc_retune_hold(struct mmc_host *host);
> +void mmc_retune_release(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_recheck(struct mmc_host *host)
> +{
> +       if (host->hold_retune <= 1)
> +               host->retune_now = 1;
> +}
> +
>  #endif /* LINUX_MMC_HOST_H */
> --
> 1.9.1
>

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

[Ulf Hansson]

On 27 March 2015 at 21:57, Adrian Hunter <adrian.hunter@intel.com> wrote:
> 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.

How about holding of re-tune from __mmc_claim_host() (after
pm_runtime_get_sync() ) -> mmc_release_host(). Would that work? If so,
that would simplify a lot around this.

Kind regards
Uffe

>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  drivers/mmc/core/core.c | 32 +++++++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 2679264..bb50d82 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -186,12 +186,29 @@ void mmc_request_done(struct mmc_host *host, struct mmc_request *mrq)
>
>  EXPORT_SYMBOL(mmc_request_done);
>
> +static void __mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
> +{
> +       int err;
> +
> +       /* Assumes host controller has been runtime resumed by mmc_claim_host */
> +       err = mmc_retune(host);
> +       if (err) {
> +               mrq->cmd->error = err;
> +               mmc_request_done(host, mrq);
> +               return;
> +       }
> +
> +       host->ops->request(host, mrq);
> +}
> +
>  static int mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
>  {
>  #ifdef CONFIG_MMC_DEBUG
>         unsigned int i, sz;
>         struct scatterlist *sg;
>  #endif
> +       mmc_retune_hold(host);
> +
>         if (mmc_card_removed(host->card))
>                 return -ENOMEDIUM;
>
> @@ -252,7 +269,7 @@ static int mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
>         }
>         mmc_host_clk_hold(host);
>         led_trigger_event(host->led, LED_FULL);
> -       host->ops->request(host, mrq);
> +       __mmc_start_request(host, mrq);
>
>         return 0;
>  }
> @@ -422,17 +439,16 @@ static int mmc_wait_for_data_req_done(struct mmc_host *host,
>                                         cmd->opcode, cmd->error);
>                                 cmd->retries--;
>                                 cmd->error = 0;
> -                               host->ops->request(host, mrq);
> +                               __mmc_start_request(host, mrq);
>                                 continue; /* wait for done/new event again */
>                         }
>                 } 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;
>  }
>
> @@ -471,8 +487,10 @@ static void mmc_wait_for_req_done(struct mmc_host *host,
>                          mmc_hostname(host), cmd->opcode, cmd->error);
>                 cmd->retries--;
>                 cmd->error = 0;
> -               host->ops->request(host, mrq);
> +               __mmc_start_request(host, mrq);
>         }
> +
> +       mmc_retune_release(host);
>  }
>
>  /**
> --
> 1.9.1
>

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

[Adrian Hunter]

On 01/04/15 12:50, Ulf Hansson wrote:
> On 27 March 2015 at 21:57, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> 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:
> 
> It's a bit hard to follow why these requirements.

Yes they are driven by SDHCI requirements.

> 
> I am giving some comments below, also for my own reference. Please
> tell me if I am way off.
> 
>>   - ability to enable / disable re-tuning
> 
> Handled internally by the mmc core.

The host controller driver enables re-tuning based on whether the host
controller requires it for that transfer mode. For example, only the SDHCI
host controller knows if tuning is required for SDR50 mode according to the
SDHCI capability register bit 45.

> 
> Or maybe SDIO func drivers needs an API to control this?

No - it is for the host controller drivers.

> 
>>   - ability to flag that re-tuning is needed
> 
> Both from mmc core and mmc host point of view.

At the moment only the host controller driver flags re-tuning is needed.

> 
>>   - ability to re-tune before any request
> 
> Internal mechanism by the core.

Yes

> 
>>   - ability to hold off re-tuning if the card is busy
> 
> What do you mean by "card is busy"?

I guess, more accurately, any time the card is in a state that is incapable
of supporting re-tuning. For example:
	- DAT0 busy
	- between dependent commands like erase start, end, etc
	- card is asleep
Also SDIO cards can have a custom sleep state where tuning won't work.

> Is this requirement due to that the re-tune timer may complete at any
> point, which then flags that a re-tune is needed?

Primarily. But control is also needed when handling recovery. Or in the SDIO
custom sleep case.

There is also SDHCI re-tuning modes 2 and 3 (presently not supported) where
the host controller itself indicates that re-tuning is needed via the
present state and interrupt status registers.

> 
>>   - ability to hold off re-tuning if re-tuning is in
>>   progress
> 
> This I don't understand. How can a re-tune sequence be triggered when
> there is already a request ongoing towards the host. I mean the host
> is claimed during the re-tuning process, so why do we need one more
> layer of protection?

This is primarily for safety. We shouldn't have to think about what would
happen if the need_retune flag is set at an inopportune time.

> 
>>   - ability to run a re-tuning timer
> 
> As we discussed earlier, it's not obvious when it make sense to run this timer.
> 
> For the SDIO case, we should likely run it all the time, since we want
> to prevent I/O errors as long as possible.
> 
> For MMC/SD, I would rather see that we perform re-tune as a part of an
> "idle operation" and not from a timer (yes I know about the SDHCI
> spec, but it doesn't make sense). We can do this because we are able
> to re-cover from I/O errors (re-trying when getting CRC errors for
> example).

It makes sense to attempt to re-tune before CRC errors occur. If the
hardware vendor has gone to the trouble to determine when that might be,
then it makes sense to use that timeout. It is surely an approximation
(SDHCI only has values in powers-of-2 with units of seconds) but it seems
unreasonable to use a completely different value.

Doing it in the idle operation would not work because we would then runtime
suspend and just have to do it again after resuming.

> 
> I will continue to review the rest of series in more detail.

Thank you! :-)

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

[Adrian Hunter]

On 01/04/15 13:13, Ulf Hansson wrote:
> On 27 March 2015 at 21:57, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> 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.
> 
> How about holding of re-tune from __mmc_claim_host() (after
> pm_runtime_get_sync() ) -> mmc_release_host(). Would that work? If so,
> that would simplify a lot around this.

I don't think it would work. The host is claimed both when we want to do
re-tuning (timer, CRC error, or host requests it) and when we want to
prevent it (in between dependent commands like erase, polling when card is
busy, SDIO card custom wake-up command).

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

[Arend van Spriel]

On 04/01/15 13:47, Adrian Hunter wrote:
> On 01/04/15 12:50, Ulf Hansson wrote:
>> On 27 March 2015 at 21:57, Adrian Hunter<adrian.hunter@intel.com>  wrote:
>>> 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:
>>
>> It's a bit hard to follow why these requirements.
>
> Yes they are driven by SDHCI requirements.
>
>>
>> I am giving some comments below, also for my own reference. Please
>> tell me if I am way off.
>>
>>>    - ability to enable / disable re-tuning
>>
>> Handled internally by the mmc core.
>
> The host controller driver enables re-tuning based on whether the host
> controller requires it for that transfer mode. For example, only the SDHCI
> host controller knows if tuning is required for SDR50 mode according to the
> SDHCI capability register bit 45.
>
>>
>> Or maybe SDIO func drivers needs an API to control this?
>
> No - it is for the host controller drivers.
>
>>
>>>    - ability to flag that re-tuning is needed
>>
>> Both from mmc core and mmc host point of view.
>
> At the moment only the host controller driver flags re-tuning is needed.
>
>>
>>>    - ability to re-tune before any request
>>
>> Internal mechanism by the core.
>
> Yes
>
>>
>>>    - ability to hold off re-tuning if the card is busy
>>
>> What do you mean by "card is busy"?
>
> I guess, more accurately, any time the card is in a state that is incapable
> of supporting re-tuning. For example:
> 	- DAT0 busy

This state is not specific to re-tuning. An SDIO device can keep DAT0 
busy at which the host controller can not start another request.

> 	- between dependent commands like erase start, end, etc
> 	- card is asleep
> Also SDIO cards can have a custom sleep state where tuning won't work.

Our SDIO wifi device has such a state and it can only come out of it 
upon CMD52 write or CMD14 (ExitSleep).

Regards,
Arend

>> Is this requirement due to that the re-tune timer may complete at any
>> point, which then flags that a re-tune is needed?
>
> Primarily. But control is also needed when handling recovery. Or in the SDIO
> custom sleep case.
>
> There is also SDHCI re-tuning modes 2 and 3 (presently not supported) where
> the host controller itself indicates that re-tuning is needed via the
> present state and interrupt status registers.
>
>>
>>>    - ability to hold off re-tuning if re-tuning is in
>>>    progress
>>
>> This I don't understand. How can a re-tune sequence be triggered when
>> there is already a request ongoing towards the host. I mean the host
>> is claimed during the re-tuning process, so why do we need one more
>> layer of protection?
>
> This is primarily for safety. We shouldn't have to think about what would
> happen if the need_retune flag is set at an inopportune time.
>
>>
>>>    - ability to run a re-tuning timer
>>
>> As we discussed earlier, it's not obvious when it make sense to run this timer.
>>
>> For the SDIO case, we should likely run it all the time, since we want
>> to prevent I/O errors as long as possible.
>>
>> For MMC/SD, I would rather see that we perform re-tune as a part of an
>> "idle operation" and not from a timer (yes I know about the SDHCI
>> spec, but it doesn't make sense). We can do this because we are able
>> to re-cover from I/O errors (re-trying when getting CRC errors for
>> example).
>
> It makes sense to attempt to re-tune before CRC errors occur. If the
> hardware vendor has gone to the trouble to determine when that might be,
> then it makes sense to use that timeout. It is surely an approximation
> (SDHCI only has values in powers-of-2 with units of seconds) but it seems
> unreasonable to use a completely different value.
>
> Doing it in the idle operation would not work because we would then runtime
> suspend and just have to do it again after resuming.
>
>>
>> I will continue to review the rest of series in more detail.
>
> Thank you! :-)
>
>

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

[Ulf Hansson]

[...]
>>>>    - ability to hold off re-tuning if the card is busy
>>>
>>>
>>> What do you mean by "card is busy"?
>>
>>
>> I guess, more accurately, any time the card is in a state that is
>> incapable
>> of supporting re-tuning. For example:
>>         - DAT0 busy
>
>
> This state is not specific to re-tuning. An SDIO device can keep DAT0 busy
> at which the host controller can not start another request.

Not entirely true. Some commands are allowed during this period, for
example CMD13 (which doesn't exist for SDIO)

Anyway, I get the point. Thanks!

>
>>         - between dependent commands like erase start, end, etc
>>         - card is asleep
>> Also SDIO cards can have a custom sleep state where tuning won't work.
>
>
> Our SDIO wifi device has such a state and it can only come out of it upon
> CMD52 write or CMD14 (ExitSleep).

So how will the mmc core know about these states? I guess we will
require SDIO func drivers to deal with enable/disable or hold/release
of re-tuning then?

I don't like this, but if there is no other way... Also, we must be
very careful on which API we exports for SDIO func drivers to use. The
can easily be misinterpreted.

[...]

Kind regards
Uffe

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

[Arend van Spriel]

On 04/02/15 10:43, Ulf Hansson wrote:
> [...]
>>>>>     - ability to hold off re-tuning if the card is busy
>>>>
>>>>
>>>> What do you mean by "card is busy"?
>>>
>>>
>>> I guess, more accurately, any time the card is in a state that is
>>> incapable
>>> of supporting re-tuning. For example:
>>>          - DAT0 busy
>>
>>
>> This state is not specific to re-tuning. An SDIO device can keep DAT0 busy
>> at which the host controller can not start another request.
>
> Not entirely true. Some commands are allowed during this period, for
> example CMD13 (which doesn't exist for SDIO)

Yeah. I was wondering whether to mention that.

> Anyway, I get the point. Thanks!
>
>>
>>>          - between dependent commands like erase start, end, etc
>>>          - card is asleep
>>> Also SDIO cards can have a custom sleep state where tuning won't work.
>>
>>
>> Our SDIO wifi device has such a state and it can only come out of it upon
>> CMD52 write or CMD14 (ExitSleep).
>
> So how will the mmc core know about these states? I guess we will
> require SDIO func drivers to deal with enable/disable or hold/release
> of re-tuning then?

This is actually why in the past we tried to only kick off retuning 
before a request that requires use of data line(s) so mmc core (or 
sdhci) would not do re-tuning when SDIO func used CMD52 to wakeup the 
device. I never tried CMD14 approach and not even sure from which spec 
it comes (maybe eMMC).

> I don't like this, but if there is no other way... Also, we must be
> very careful on which API we exports for SDIO func drivers to use. The
> can easily be misinterpreted.
>
> [...]
>
> Kind regards
> Uffe

Regards,
Arend

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

[Ulf Hansson]

On 2 April 2015 at 12:30, Arend van Spriel <arend@broadcom.com> wrote:
> On 04/02/15 10:43, Ulf Hansson wrote:
>>
>> [...]
>>>>>>
>>>>>>     - ability to hold off re-tuning if the card is busy
>>>>>
>>>>>
>>>>>
>>>>> What do you mean by "card is busy"?
>>>>
>>>>
>>>>
>>>> I guess, more accurately, any time the card is in a state that is
>>>> incapable
>>>> of supporting re-tuning. For example:
>>>>          - DAT0 busy
>>>
>>>
>>>
>>> This state is not specific to re-tuning. An SDIO device can keep DAT0
>>> busy
>>> at which the host controller can not start another request.
>>
>>
>> Not entirely true. Some commands are allowed during this period, for
>> example CMD13 (which doesn't exist for SDIO)
>
>
> Yeah. I was wondering whether to mention that.
>
>> Anyway, I get the point. Thanks!
>>
>>>
>>>>          - between dependent commands like erase start, end, etc
>>>>          - card is asleep
>>>> Also SDIO cards can have a custom sleep state where tuning won't work.
>>>
>>>
>>>
>>> Our SDIO wifi device has such a state and it can only come out of it upon
>>> CMD52 write or CMD14 (ExitSleep).
>>
>>
>> So how will the mmc core know about these states? I guess we will
>> require SDIO func drivers to deal with enable/disable or hold/release
>> of re-tuning then?
>
>
> This is actually why in the past we tried to only kick off retuning before a
> request that requires use of data line(s) so mmc core (or sdhci) would not
> do re-tuning when SDIO func used CMD52 to wakeup the device. I never tried
> CMD14 approach and not even sure from which spec it comes (maybe eMMC).

That's an interesting idea. It would eliminate the need for SDIO func
drivers to care about holding/releasing re-tuning.

Would be nice to hear about Adrian's thoughts around this as well.

>
>> I don't like this, but if there is no other way... Also, we must be
>> very careful on which API we exports for SDIO func drivers to use. The
>> can easily be misinterpreted.
>>
>> [...]

Kind regards
Uffe

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

[Adrian Hunter]

On 02/04/15 15:10, Ulf Hansson wrote:
> On 2 April 2015 at 12:30, Arend van Spriel <arend@broadcom.com> wrote:
>> On 04/02/15 10:43, Ulf Hansson wrote:
>>>
>>> [...]
>>>>>>>
>>>>>>>     - ability to hold off re-tuning if the card is busy
>>>>>>
>>>>>>
>>>>>>
>>>>>> What do you mean by "card is busy"?
>>>>>
>>>>>
>>>>>
>>>>> I guess, more accurately, any time the card is in a state that is
>>>>> incapable
>>>>> of supporting re-tuning. For example:
>>>>>          - DAT0 busy
>>>>
>>>>
>>>>
>>>> This state is not specific to re-tuning. An SDIO device can keep DAT0
>>>> busy
>>>> at which the host controller can not start another request.
>>>
>>>
>>> Not entirely true. Some commands are allowed during this period, for
>>> example CMD13 (which doesn't exist for SDIO)
>>
>>
>> Yeah. I was wondering whether to mention that.
>>
>>> Anyway, I get the point. Thanks!
>>>
>>>>
>>>>>          - between dependent commands like erase start, end, etc
>>>>>          - card is asleep
>>>>> Also SDIO cards can have a custom sleep state where tuning won't work.
>>>>
>>>>
>>>>
>>>> Our SDIO wifi device has such a state and it can only come out of it upon
>>>> CMD52 write or CMD14 (ExitSleep).
>>>
>>>
>>> So how will the mmc core know about these states? I guess we will
>>> require SDIO func drivers to deal with enable/disable or hold/release
>>> of re-tuning then?
>>
>>
>> This is actually why in the past we tried to only kick off retuning before a
>> request that requires use of data line(s) so mmc core (or sdhci) would not
>> do re-tuning when SDIO func used CMD52 to wakeup the device. I never tried
>> CMD14 approach and not even sure from which spec it comes (maybe eMMC).
> 
> That's an interesting idea. It would eliminate the need for SDIO func
> drivers to care about holding/releasing re-tuning.
> 
> Would be nice to hear about Adrian's thoughts around this as well.

I don't see how it would work because re-tuning is needed after the host
controller runtime resumes. i.e. once the SDIO wifi card stops being active
the host controller will runtime suspend.

Also I would expect doing re-tuning before every data request would likely
be unacceptable from a performance point of view.

> 
>>
>>> I don't like this, but if there is no other way... Also, we must be
>>> very careful on which API we exports for SDIO func drivers to use. The
>>> can easily be misinterpreted.
>>>
>>> [...]
> 
> Kind regards
> Uffe
> 
> 

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

[Ulf Hansson]

On 2 April 2015 at 14:18, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 02/04/15 15:10, Ulf Hansson wrote:
>> On 2 April 2015 at 12:30, Arend van Spriel <arend@broadcom.com> wrote:
>>> On 04/02/15 10:43, Ulf Hansson wrote:
>>>>
>>>> [...]
>>>>>>>>
>>>>>>>>     - ability to hold off re-tuning if the card is busy
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> What do you mean by "card is busy"?
>>>>>>
>>>>>>
>>>>>>
>>>>>> I guess, more accurately, any time the card is in a state that is
>>>>>> incapable
>>>>>> of supporting re-tuning. For example:
>>>>>>          - DAT0 busy
>>>>>
>>>>>
>>>>>
>>>>> This state is not specific to re-tuning. An SDIO device can keep DAT0
>>>>> busy
>>>>> at which the host controller can not start another request.
>>>>
>>>>
>>>> Not entirely true. Some commands are allowed during this period, for
>>>> example CMD13 (which doesn't exist for SDIO)
>>>
>>>
>>> Yeah. I was wondering whether to mention that.
>>>
>>>> Anyway, I get the point. Thanks!
>>>>
>>>>>
>>>>>>          - between dependent commands like erase start, end, etc
>>>>>>          - card is asleep
>>>>>> Also SDIO cards can have a custom sleep state where tuning won't work.
>>>>>
>>>>>
>>>>>
>>>>> Our SDIO wifi device has such a state and it can only come out of it upon
>>>>> CMD52 write or CMD14 (ExitSleep).
>>>>
>>>>
>>>> So how will the mmc core know about these states? I guess we will
>>>> require SDIO func drivers to deal with enable/disable or hold/release
>>>> of re-tuning then?
>>>
>>>
>>> This is actually why in the past we tried to only kick off retuning before a
>>> request that requires use of data line(s) so mmc core (or sdhci) would not
>>> do re-tuning when SDIO func used CMD52 to wakeup the device. I never tried
>>> CMD14 approach and not even sure from which spec it comes (maybe eMMC).
>>
>> That's an interesting idea. It would eliminate the need for SDIO func
>> drivers to care about holding/releasing re-tuning.
>>
>> Would be nice to hear about Adrian's thoughts around this as well.
>
> I don't see how it would work because re-tuning is needed after the host
> controller runtime resumes. i.e. once the SDIO wifi card stops being active
> the host controller will runtime suspend.

Why do we always need to re-tune from this phase?

What Arend points out is that we could "delay" the re-tune until we
know there is a DATA request. Wouldn't that work for SDHCI as well?

>
> Also I would expect doing re-tuning before every data request would likely
> be unacceptable from a performance point of view.
>
>>
>>>
>>>> I don't like this, but if there is no other way... Also, we must be
>>>> very careful on which API we exports for SDIO func drivers to use. The
>>>> can easily be misinterpreted.
>>>>
>>>> [...]
>>
>> Kind regards
>> Uffe
>>
>>
>

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

[Arend van Spriel]

On 04/02/15 14:25, Ulf Hansson wrote:
> On 2 April 2015 at 14:18, Adrian Hunter<adrian.hunter@intel.com>  wrote:
>> On 02/04/15 15:10, Ulf Hansson wrote:
>>> On 2 April 2015 at 12:30, Arend van Spriel<arend@broadcom.com>  wrote:
>>>> On 04/02/15 10:43, Ulf Hansson wrote:
>>>>>
>>>>> [...]
>>>>>>>>>
>>>>>>>>>      - ability to hold off re-tuning if the card is busy
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> What do you mean by "card is busy"?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> I guess, more accurately, any time the card is in a state that is
>>>>>>> incapable
>>>>>>> of supporting re-tuning. For example:
>>>>>>>           - DAT0 busy
>>>>>>
>>>>>>
>>>>>>
>>>>>> This state is not specific to re-tuning. An SDIO device can keep DAT0
>>>>>> busy
>>>>>> at which the host controller can not start another request.
>>>>>
>>>>>
>>>>> Not entirely true. Some commands are allowed during this period, for
>>>>> example CMD13 (which doesn't exist for SDIO)
>>>>
>>>>
>>>> Yeah. I was wondering whether to mention that.
>>>>
>>>>> Anyway, I get the point. Thanks!
>>>>>
>>>>>>
>>>>>>>           - between dependent commands like erase start, end, etc
>>>>>>>           - card is asleep
>>>>>>> Also SDIO cards can have a custom sleep state where tuning won't work.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Our SDIO wifi device has such a state and it can only come out of it upon
>>>>>> CMD52 write or CMD14 (ExitSleep).
>>>>>
>>>>>
>>>>> So how will the mmc core know about these states? I guess we will
>>>>> require SDIO func drivers to deal with enable/disable or hold/release
>>>>> of re-tuning then?
>>>>
>>>>
>>>> This is actually why in the past we tried to only kick off retuning before a
>>>> request that requires use of data line(s) so mmc core (or sdhci) would not
>>>> do re-tuning when SDIO func used CMD52 to wakeup the device. I never tried
>>>> CMD14 approach and not even sure from which spec it comes (maybe eMMC).
>>>
>>> That's an interesting idea. It would eliminate the need for SDIO func
>>> drivers to care about holding/releasing re-tuning.
>>>
>>> Would be nice to hear about Adrian's thoughts around this as well.
>>
>> I don't see how it would work because re-tuning is needed after the host
>> controller runtime resumes. i.e. once the SDIO wifi card stops being active
>> the host controller will runtime suspend.
>
> Why do we always need to re-tune from this phase?
>
> What Arend points out is that we could "delay" the re-tune until we
> know there is a DATA request. Wouldn't that work for SDHCI as well?

You beat me to it, but that is indeed what I meant.

Regards,
Arend

>>
>> Also I would expect doing re-tuning before every data request would likely
>> be unacceptable from a performance point of view.
>>
>>>
>>>>
>>>>> I don't like this, but if there is no other way... Also, we must be
>>>>> very careful on which API we exports for SDIO func drivers to use. The
>>>>> can easily be misinterpreted.
>>>>>
>>>>> [...]
>>>
>>> Kind regards
>>> Uffe
>>>
>>>
>>

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

[Adrian Hunter]

On 02/04/15 15:27, Arend van Spriel wrote:
> On 04/02/15 14:25, Ulf Hansson wrote:
>> On 2 April 2015 at 14:18, Adrian Hunter<adrian.hunter@intel.com>  wrote:
>>> On 02/04/15 15:10, Ulf Hansson wrote:
>>>> On 2 April 2015 at 12:30, Arend van Spriel<arend@broadcom.com>  wrote:
>>>>> On 04/02/15 10:43, Ulf Hansson wrote:
>>>>>>
>>>>>> [...]
>>>>>>>>>>
>>>>>>>>>>      - ability to hold off re-tuning if the card is busy
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> What do you mean by "card is busy"?
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> I guess, more accurately, any time the card is in a state that is
>>>>>>>> incapable
>>>>>>>> of supporting re-tuning. For example:
>>>>>>>>           - DAT0 busy
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> This state is not specific to re-tuning. An SDIO device can keep DAT0
>>>>>>> busy
>>>>>>> at which the host controller can not start another request.
>>>>>>
>>>>>>
>>>>>> Not entirely true. Some commands are allowed during this period, for
>>>>>> example CMD13 (which doesn't exist for SDIO)
>>>>>
>>>>>
>>>>> Yeah. I was wondering whether to mention that.
>>>>>
>>>>>> Anyway, I get the point. Thanks!
>>>>>>
>>>>>>>
>>>>>>>>           - between dependent commands like erase start, end, etc
>>>>>>>>           - card is asleep
>>>>>>>> Also SDIO cards can have a custom sleep state where tuning won't work.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Our SDIO wifi device has such a state and it can only come out of it
>>>>>>> upon
>>>>>>> CMD52 write or CMD14 (ExitSleep).
>>>>>>
>>>>>>
>>>>>> So how will the mmc core know about these states? I guess we will
>>>>>> require SDIO func drivers to deal with enable/disable or hold/release
>>>>>> of re-tuning then?
>>>>>
>>>>>
>>>>> This is actually why in the past we tried to only kick off retuning
>>>>> before a
>>>>> request that requires use of data line(s) so mmc core (or sdhci) would not
>>>>> do re-tuning when SDIO func used CMD52 to wakeup the device. I never tried
>>>>> CMD14 approach and not even sure from which spec it comes (maybe eMMC).
>>>>
>>>> That's an interesting idea. It would eliminate the need for SDIO func
>>>> drivers to care about holding/releasing re-tuning.
>>>>
>>>> Would be nice to hear about Adrian's thoughts around this as well.
>>>
>>> I don't see how it would work because re-tuning is needed after the host
>>> controller runtime resumes. i.e. once the SDIO wifi card stops being active
>>> the host controller will runtime suspend.
>>
>> Why do we always need to re-tune from this phase?
>>
>> What Arend points out is that we could "delay" the re-tune until we
>> know there is a DATA request. Wouldn't that work for SDHCI as well?
> 
> You beat me to it, but that is indeed what I meant.

The CMD line needs tuning too, so delaying tuning on every command will
cause errors. It is better to hold tuning for the specific command used to
wake-up.

> 
> Regards,
> Arend
> 
>>>
>>> Also I would expect doing re-tuning before every data request would likely
>>> be unacceptable from a performance point of view.
>>>
>>>>
>>>>>
>>>>>> I don't like this, but if there is no other way... Also, we must be
>>>>>> very careful on which API we exports for SDIO func drivers to use. The
>>>>>> can easily be misinterpreted.
>>>>>>
>>>>>> [...]
>>>>
>>>> Kind regards
>>>> Uffe
>>>>
>>>>
>>>
> 
> 
> 

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

[Ulf Hansson]

[...]

>>
>>>   - ability to enable / disable re-tuning
>>
>> Handled internally by the mmc core.
>
> The host controller driver enables re-tuning based on whether the host
> controller requires it for that transfer mode. For example, only the SDHCI
> host controller knows if tuning is required for SDR50 mode according to the
> SDHCI capability register bit 45.

That seems a bit silly.

All hosts wants the re-tuning to be "enabled" if the current used
speed mode requires it. It's not a host driver thing to deal with,
just the core.

What's needed for the SDHCI case (in runtime PM suspend), is to be
able to disable the re-tune timer and to flag that a re-tune is
needed. Both of these things can be dealt with from the
mmc_retune_needed() API, since I believe there should never be a case
when we want the re-tune timer to continue to run, when someone have
set the need_retune flag.

[...]

>>>   - ability to hold off re-tuning if the card is busy
>>
>> What do you mean by "card is busy"?
>
> I guess, more accurately, any time the card is in a state that is incapable
> of supporting re-tuning. For example:
>         - DAT0 busy
>         - between dependent commands like erase start, end, etc
>         - card is asleep
> Also SDIO cards can have a custom sleep state where tuning won't work.
>
>> Is this requirement due to that the re-tune timer may complete at any
>> point, which then flags that a re-tune is needed?
>
> Primarily. But control is also needed when handling recovery. Or in the SDIO
> custom sleep case.
>
> There is also SDHCI re-tuning modes 2 and 3 (presently not supported) where
> the host controller itself indicates that re-tuning is needed via the
> present state and interrupt status registers.

To me, I assume mmc_retune_needed() API should be enough to cover
SDHCI mode 2 and 3, right?

Potentially mmc_retune_needed() may then be called from IRQ context so
we just have to be sure the API also copes with that (thinking that
cancelling the timer must not "sleep").

[...]

>>
>>>   - ability to run a re-tuning timer
>>
>> As we discussed earlier, it's not obvious when it make sense to run this timer.
>>
>> For the SDIO case, we should likely run it all the time, since we want
>> to prevent I/O errors as long as possible.
>>
>> For MMC/SD, I would rather see that we perform re-tune as a part of an
>> "idle operation" and not from a timer (yes I know about the SDHCI
>> spec, but it doesn't make sense). We can do this because we are able
>> to re-cover from I/O errors (re-trying when getting CRC errors for
>> example).
>
> It makes sense to attempt to re-tune before CRC errors occur. If the
> hardware vendor has gone to the trouble to determine when that might be,
> then it makes sense to use that timeout. It is surely an approximation
> (SDHCI only has values in powers-of-2 with units of seconds) but it seems
> unreasonable to use a completely different value.

I agree in cases where the hardware itself can measure heat and thus
anticipate+signal when a re-tuning is needed. SDHCI mode 1 doesn't
work like that, it's just a guess - as good as any. Therefore I would
only use a timer in the SDIO case and rely on the recover sequence for
the SD/MMC case.

Now, I am not going to argue about this any more. Let's follow your
suggestion and to make it possible to use a timer for _all_ cases.

>
> Doing it in the idle operation would not work because we would then runtime
> suspend and just have to do it again after resuming.

That's correct, it will be completely useless for SDHCI. That's isn't
the case for other hosts.

Anyway, let's leave the "idle operation" scenarios out of this
discussion. If considered, later it needs to be a configurable option.

Kind regards
Uffe

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

[Ulf Hansson]

[...]

>>>>>>>>> Also SDIO cards can have a custom sleep state where tuning won't work.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Our SDIO wifi device has such a state and it can only come out of it
>>>>>>>> upon
>>>>>>>> CMD52 write or CMD14 (ExitSleep).
>>>>>>>
>>>>>>>
>>>>>>> So how will the mmc core know about these states? I guess we will
>>>>>>> require SDIO func drivers to deal with enable/disable or hold/release
>>>>>>> of re-tuning then?
>>>>>>
>>>>>>
>>>>>> This is actually why in the past we tried to only kick off retuning
>>>>>> before a
>>>>>> request that requires use of data line(s) so mmc core (or sdhci) would not
>>>>>> do re-tuning when SDIO func used CMD52 to wakeup the device. I never tried
>>>>>> CMD14 approach and not even sure from which spec it comes (maybe eMMC).
>>>>>
>>>>> That's an interesting idea. It would eliminate the need for SDIO func
>>>>> drivers to care about holding/releasing re-tuning.
>>>>>
>>>>> Would be nice to hear about Adrian's thoughts around this as well.
>>>>
>>>> I don't see how it would work because re-tuning is needed after the host
>>>> controller runtime resumes. i.e. once the SDIO wifi card stops being active
>>>> the host controller will runtime suspend.
>>>
>>> Why do we always need to re-tune from this phase?
>>>
>>> What Arend points out is that we could "delay" the re-tune until we
>>> know there is a DATA request. Wouldn't that work for SDHCI as well?
>>
>> You beat me to it, but that is indeed what I meant.
>
> The CMD line needs tuning too, so delaying tuning on every command will
> cause errors. It is better to hold tuning for the specific command used to
> wake-up.

Hmm.

This touches the discussion where Neil Brown also was involved, around
how to handle "idle operations" for SDIO.

How does SDIO func drivers detects "request inactivity", which
triggers them to send its device to "sleep mode"? That answer should
be runtime PM.

So, from mmc core perspective we should be able to get notifications
through runtime PM callbacks (from mmc or sdio bus) to understand
whether we need to hold of re-tune.

I know, it's a bit of a vague idea so I will give it some more
thinking during the Easter holidays.

Kind regards
Uffe

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

[Adrian Hunter]

On 2/04/2015 4:05 p.m., Ulf Hansson wrote:
> [...]
>
>>>
>>>>    - ability to enable / disable re-tuning
>>>
>>> Handled internally by the mmc core.
>>
>> The host controller driver enables re-tuning based on whether the host
>> controller requires it for that transfer mode. For example, only the SDHCI
>> host controller knows if tuning is required for SDR50 mode according to the
>> SDHCI capability register bit 45.
>
> That seems a bit silly.
>
> All hosts wants the re-tuning to be "enabled" if the current used
> speed mode requires it. It's not a host driver thing to deal with,
> just the core.

No it is up to the host controller. That is how it is in the
SD Host Controller Specification. Both whether to tune SDR50 and
whether to run a re-tuning timer.

Tuning is inherently a host controller problem. The card can always
receive correctly from the host, but the host has to adjust its
"sampling point" to receive from the card. Only the host knows its
capabilities in this regard.

>
> What's needed for the SDHCI case (in runtime PM suspend), is to be
> able to disable the re-tune timer and to flag that a re-tune is
> needed.

Yes that is how it works.

>         Both of these things can be dealt with from the
> mmc_retune_needed() API, since I believe there should never be a case
> when we want the re-tune timer to continue to run, when someone have
> set the need_retune flag.

The approach implemented is to update the timer when tuning takes place.
That allows mmc_retune_needed() simply to set a flag, so it can be called
from any context.

>
> [...]
>
>>>>    - ability to hold off re-tuning if the card is busy
>>>
>>> What do you mean by "card is busy"?
>>
>> I guess, more accurately, any time the card is in a state that is incapable
>> of supporting re-tuning. For example:
>>          - DAT0 busy
>>          - between dependent commands like erase start, end, etc
>>          - card is asleep
>> Also SDIO cards can have a custom sleep state where tuning won't work.
>>
>>> Is this requirement due to that the re-tune timer may complete at any
>>> point, which then flags that a re-tune is needed?
>>
>> Primarily. But control is also needed when handling recovery. Or in the SDIO
>> custom sleep case.
>>
>> There is also SDHCI re-tuning modes 2 and 3 (presently not supported) where
>> the host controller itself indicates that re-tuning is needed via the
>> present state and interrupt status registers.
>
> To me, I assume mmc_retune_needed() API should be enough to cover
> SDHCI mode 2 and 3, right?

Yes.

>
> Potentially mmc_retune_needed() may then be called from IRQ context so
> we just have to be sure the API also copes with that (thinking that
> cancelling the timer must not "sleep").

According to the SDHC spec, if the host controller will itself request
re-tuning, then no timer is to be used. So the having the mmc_retune_needed()
API simply set a flag, as it does now, makes sense.

>
> [...]
>
>>>
>>>>    - ability to run a re-tuning timer
>>>
>>> As we discussed earlier, it's not obvious when it make sense to run this timer.
>>>
>>> For the SDIO case, we should likely run it all the time, since we want
>>> to prevent I/O errors as long as possible.
>>>
>>> For MMC/SD, I would rather see that we perform re-tune as a part of an
>>> "idle operation" and not from a timer (yes I know about the SDHCI
>>> spec, but it doesn't make sense). We can do this because we are able
>>> to re-cover from I/O errors (re-trying when getting CRC errors for
>>> example).
>>
>> It makes sense to attempt to re-tune before CRC errors occur. If the
>> hardware vendor has gone to the trouble to determine when that might be,
>> then it makes sense to use that timeout. It is surely an approximation
>> (SDHCI only has values in powers-of-2 with units of seconds) but it seems
>> unreasonable to use a completely different value.
>
> I agree in cases where the hardware itself can measure heat and thus
> anticipate+signal when a re-tuning is needed. SDHCI mode 1 doesn't
> work like that, it's just a guess - as good as any. Therefore I would
> only use a timer in the SDIO case and rely on the recover sequence for
> the SD/MMC case.
>
> Now, I am not going to argue about this any more. Let's follow your
> suggestion and to make it possible to use a timer for _all_ cases.

Thank you! :-)

>
>>
>> Doing it in the idle operation would not work because we would then runtime
>> suspend and just have to do it again after resuming.
>
> That's correct, it will be completely useless for SDHCI. That's isn't
> the case for other hosts.
>
> Anyway, let's leave the "idle operation" scenarios out of this
> discussion. If considered, later it needs to be a configurable option.
>
> Kind regards
> Uffe
>

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

[NeilBrown]

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

On Thu, 2 Apr 2015 16:00:52 +0200 Ulf Hansson <ulf.hansson@linaro.org> wrote:

> [...]
> 
> >>>>>>>>> Also SDIO cards can have a custom sleep state where tuning won't work.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Our SDIO wifi device has such a state and it can only come out of it
> >>>>>>>> upon
> >>>>>>>> CMD52 write or CMD14 (ExitSleep).
> >>>>>>>
> >>>>>>>
> >>>>>>> So how will the mmc core know about these states? I guess we will
> >>>>>>> require SDIO func drivers to deal with enable/disable or hold/release
> >>>>>>> of re-tuning then?
> >>>>>>
> >>>>>>
> >>>>>> This is actually why in the past we tried to only kick off retuning
> >>>>>> before a
> >>>>>> request that requires use of data line(s) so mmc core (or sdhci) would not
> >>>>>> do re-tuning when SDIO func used CMD52 to wakeup the device. I never tried
> >>>>>> CMD14 approach and not even sure from which spec it comes (maybe eMMC).
> >>>>>
> >>>>> That's an interesting idea. It would eliminate the need for SDIO func
> >>>>> drivers to care about holding/releasing re-tuning.
> >>>>>
> >>>>> Would be nice to hear about Adrian's thoughts around this as well.
> >>>>
> >>>> I don't see how it would work because re-tuning is needed after the host
> >>>> controller runtime resumes. i.e. once the SDIO wifi card stops being active
> >>>> the host controller will runtime suspend.
> >>>
> >>> Why do we always need to re-tune from this phase?
> >>>
> >>> What Arend points out is that we could "delay" the re-tune until we
> >>> know there is a DATA request. Wouldn't that work for SDHCI as well?
> >>
> >> You beat me to it, but that is indeed what I meant.
> >
> > The CMD line needs tuning too, so delaying tuning on every command will
> > cause errors. It is better to hold tuning for the specific command used to
> > wake-up.
> 
> Hmm.
> 
> This touches the discussion where Neil Brown also was involved, around
> how to handle "idle operations" for SDIO.

Does it?  I'm not at all sure that I follow all the issues with re-tuning.

It *seems* that there are two sorts of events:
 1/ those that indicate that a retune will be needed.  This includes
    power cycling of some device, and time passing.
 2/ those that need to have retuning done (if needed) before they are
    performed.

Did I over-simplify there?

If I didn't, then do we just need a "retune needed" flag which type-1 events
can set asynchronously, and which type-2 events check before they are
performed.
The code I looked at seems to contain those ideas, but it seems more
complicated so I must have missed something.

For my purposes, "idle" it exactly "not mmc_claimed for a while".

I cannot quite see where "idle" fits in to retuning.... unless maybe you want
to retune proactively *before* a type-2 event comes along.  Is that the issue?

> 
> How does SDIO func drivers detects "request inactivity", which
> triggers them to send its device to "sleep mode"? That answer should
> be runtime PM.

"should be" :-)
I just had a look at libertas/if_sdio because that is  the driver that I use.
It has an "auto_deepsleep_timer" which works a lot like runtime_pm
autosuspend.   However it doesn't appear that auto_deepsleep mode is ever
enabled :-( so there isn't much point converting it to use runtime_pm.


> 
> So, from mmc core perspective we should be able to get notifications
> through runtime PM callbacks (from mmc or sdio bus) to understand
> whether we need to hold of re-tune.

I'm having trouble getting to grips with this idea of "holding off re-tune".
Is it not sufficient to only allow a re-tune to happen when the host is
claimed by the retuner?
In generally if you want to cause something to "hold off", you use a lock of
some sort.  Can we not just create a lock (if no present lock is sufficient)
- which may just be a flag - and take it whenever re-tune is not allowed?


> 
> I know, it's a bit of a vague idea so I will give it some more
> thinking during the Easter holidays.

To give you something more concrete to work with, below is how I want to do
idle detection.  I'm defining "idle" as "mmc_host hasn't been claimed for
100ms".  I do it by enabling runtime-pm on the mmc_host.
I have a follow-on patch which adds an idle handler for the 4-bit->1-bit
transition.

One issue that I haven't quite resolved to my own satisfaction is whether the
mmc host devices (parents of the mmc_host class device) should have
ignore_children set.  Several already do, but this is currently completely
pointless as the child (the mmc_host) doesn't do power management, so there
is nothing to ignore.

I think I'd like to remove all the pm_suspend_ignore_children() calls from
drivers/mmc/host/*.c and from mmc_alloc_host()  in this patch.  Then whenever
the mmc_host was pm_runtime active - i.e. when the mmc_host was claimed - the
host device would be held active.  Then we could revert

mmc: core: Enable runtime PM management of host devices

as it would no longer be needed.

Happy Easter!

NeilBrown




From: NeilBrown <neil@brown.name>
Date: Tue, 24 Mar 2015 17:18:15 +1100
Subject: [PATCH] mmc/core: add pm_runtime tracking to mmc_host devices.

Enable runtime pm for mmc_host devices, and take a
reference while the host is claimed.

Use an autosuspend timeout so that the device isn't
put to sleep until we have been idle for a while.

Set the parent to ignore children, so the PM status of
the host does not affect the controller at all.

This functionality will be used in a future patch to allow
commands to be sent to the device when the host is idle.

Signed-off-by: NeilBrown <neil@brown.name>

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index f6faa18edf7b..39e2c781e382 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -901,6 +901,7 @@ int __mmc_claim_host(struct mmc_host *host, atomic_t *abort)
 
 	might_sleep();
 
+	pm_runtime_get_sync(mmc_classdev(host));
 	add_wait_queue(&host->wq, &wait);
 	spin_lock_irqsave(&host->lock, flags);
 	while (1) {
@@ -924,6 +925,8 @@ int __mmc_claim_host(struct mmc_host *host, atomic_t *abort)
 	spin_unlock_irqrestore(&host->lock, flags);
 	remove_wait_queue(&host->wq, &wait);
 
+	if (stop)
+		pm_runtime_put(mmc_classdev(host));
 	if (pm)
 		pm_runtime_get_sync(mmc_dev(host));
 
@@ -956,6 +959,8 @@ void mmc_release_host(struct mmc_host *host)
 		pm_runtime_mark_last_busy(mmc_dev(host));
 		pm_runtime_put_autosuspend(mmc_dev(host));
 	}
+	pm_runtime_mark_last_busy(mmc_classdev(host));
+	pm_runtime_put_autosuspend(mmc_classdev(host));
 }
 EXPORT_SYMBOL(mmc_release_host);
 
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 8be0df758e68..86692b427301 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -22,6 +22,7 @@
 #include <linux/leds.h>
 #include <linux/slab.h>
 #include <linux/suspend.h>
+#include <linux/pm_runtime.h>
 
 #include <linux/mmc/host.h>
 #include <linux/mmc/card.h>
@@ -46,9 +47,39 @@ static void mmc_host_classdev_release(struct device *dev)
 	kfree(host);
 }
 
+static int mmc_host_runtime_suspend(struct device *dev)
+{
+	struct mmc_host *host = cls_dev_to_mmc_host(dev);
+
+	BUG_ON(host->claimed);
+	host->claimed = 1;
+	/* idle handling happens here */
+
+	host->claimed = 0;
+	return 0;
+}
+
+static int mmc_host_runtime_resume(struct device *dev)
+{
+	struct mmc_host *host = cls_dev_to_mmc_host(dev);
+
+	BUG_ON(host->claimed);
+	host->claimed = 1;
+	/* undo any idle handling here */
+
+	host->claimed = 0;
+	return 0;
+}
+
+static struct dev_pm_ops mmc_host_dev_pm_ops = {
+	.runtime_suspend = mmc_host_runtime_suspend,
+	.runtime_resume = mmc_host_runtime_resume,
+};
+
 static struct class mmc_host_class = {
 	.name		= "mmc_host",
 	.dev_release	= mmc_host_classdev_release,
+	.pm		= &mmc_host_dev_pm_ops,
 };
 
 int mmc_register_host_class(void)
@@ -490,6 +521,11 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
 	host->class_dev.parent = dev;
 	host->class_dev.class = &mmc_host_class;
 	device_initialize(&host->class_dev);
+	if (dev)
+		/*
+		 * The device can sleep even when host is claimed.
+		 */
+		pm_suspend_ignore_children(dev, true);
 
 	if (mmc_gpio_alloc(host)) {
 		put_device(&host->class_dev);
@@ -532,15 +568,25 @@ EXPORT_SYMBOL(mmc_alloc_host);
 int mmc_add_host(struct mmc_host *host)
 {
 	int err;
+	struct device *dev = mmc_classdev(host);
 
 	WARN_ON((host->caps & MMC_CAP_SDIO_IRQ) &&
 		!host->ops->enable_sdio_irq);
 
-	err = device_add(&host->class_dev);
+	err = device_add(dev);
 	if (err)
 		return err;
+	pm_runtime_enable(dev);
+	/*
+	 * The host should be able to suspend while the attached card
+	 * stays awake.
+	 */
+	pm_suspend_ignore_children(dev, true);
+	pm_runtime_get_sync(dev);
+	pm_runtime_set_autosuspend_delay(dev, 100);
+	pm_runtime_use_autosuspend(dev);
 
-	led_trigger_register_simple(dev_name(&host->class_dev), &host->led);
+	led_trigger_register_simple(dev_name(dev), &host->led);
 
 #ifdef CONFIG_DEBUG_FS
 	mmc_add_host_debugfs(host);
@@ -550,6 +596,9 @@ int mmc_add_host(struct mmc_host *host)
 	mmc_start_host(host);
 	register_pm_notifier(&host->pm_notify);
 
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
 	return 0;
 }
 

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

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

[Adrian Hunter]

On 02/04/15 17:00, Ulf Hansson wrote:
> [...]
> 
>>>>>>>>>> Also SDIO cards can have a custom sleep state where tuning won't work.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Our SDIO wifi device has such a state and it can only come out of it
>>>>>>>>> upon
>>>>>>>>> CMD52 write or CMD14 (ExitSleep).
>>>>>>>>
>>>>>>>>
>>>>>>>> So how will the mmc core know about these states? I guess we will
>>>>>>>> require SDIO func drivers to deal with enable/disable or hold/release
>>>>>>>> of re-tuning then?
>>>>>>>
>>>>>>>
>>>>>>> This is actually why in the past we tried to only kick off retuning
>>>>>>> before a
>>>>>>> request that requires use of data line(s) so mmc core (or sdhci) would not
>>>>>>> do re-tuning when SDIO func used CMD52 to wakeup the device. I never tried
>>>>>>> CMD14 approach and not even sure from which spec it comes (maybe eMMC).
>>>>>>
>>>>>> That's an interesting idea. It would eliminate the need for SDIO func
>>>>>> drivers to care about holding/releasing re-tuning.
>>>>>>
>>>>>> Would be nice to hear about Adrian's thoughts around this as well.
>>>>>
>>>>> I don't see how it would work because re-tuning is needed after the host
>>>>> controller runtime resumes. i.e. once the SDIO wifi card stops being active
>>>>> the host controller will runtime suspend.
>>>>
>>>> Why do we always need to re-tune from this phase?
>>>>
>>>> What Arend points out is that we could "delay" the re-tune until we
>>>> know there is a DATA request. Wouldn't that work for SDHCI as well?
>>>
>>> You beat me to it, but that is indeed what I meant.
>>
>> The CMD line needs tuning too, so delaying tuning on every command will
>> cause errors. It is better to hold tuning for the specific command used to
>> wake-up.
> 
> Hmm.
> 
> This touches the discussion where Neil Brown also was involved, around
> how to handle "idle operations" for SDIO.
> 
> How does SDIO func drivers detects "request inactivity", which
> triggers them to send its device to "sleep mode"? That answer should
> be runtime PM.

Using runtime pm for custom sleep states would seem to conflict with its use
by the power domain. For example ACPI will enumerate embedded SDIO function
devices and link them to the ACPI power domain. Then ACPI will choose the
lowest power state for runtime suspend.

It isn't obvious how a driver that doesn't know its power domain should
handle runtime pm, other than assuming it means power off.

> 
> So, from mmc core perspective we should be able to get notifications
> through runtime PM callbacks (from mmc or sdio bus) to understand
> whether we need to hold of re-tune.

That doesn't match the requirement. Re-tuning needs to be held for the
wake-up command, not while asleep.

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

[Adrian Hunter]

On 01/04/15 09:21, Adrian Hunter wrote:
> On 27/03/15 22:57, Adrian Hunter wrote:
>> Hi
>>
>> Here is V4 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.
>>
>> These patches now depend on Ulf's patch:
>>
>>     mmc: core: Enable runtime PM management of host devices
>>
>>
>> Changes in V4:
>>
>>     mmc: host: Add facility to support re-tuning
>>
>> 	Assume mmc_claim_host() runtime resumes the host
>> 	controller so there are no races with runtime pm.
>> 	Consequently remove now un-needed re-tuning host
>> 	operations.
>>
>>     mmc: core: Add support for re-tuning before each request
>>
>> 	Call mmc_retune() prior to ->request()
>>
>>     mmc: sdhci: Change to new way of doing re-tuning
>>
>> 	Updated to reflect the changes above.
> 
> Hi
> 
> What is the status of this?

Hi

What needs to be done to move this forward?

Regards
Adrian

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

[Ulf Hansson]

On 10 April 2015 at 12:39, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 01/04/15 09:21, Adrian Hunter wrote:
>> On 27/03/15 22:57, Adrian Hunter wrote:
>>> Hi
>>>
>>> Here is V4 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.
>>>
>>> These patches now depend on Ulf's patch:
>>>
>>>     mmc: core: Enable runtime PM management of host devices
>>>
>>>
>>> Changes in V4:
>>>
>>>     mmc: host: Add facility to support re-tuning
>>>
>>>      Assume mmc_claim_host() runtime resumes the host
>>>      controller so there are no races with runtime pm.
>>>      Consequently remove now un-needed re-tuning host
>>>      operations.
>>>
>>>     mmc: core: Add support for re-tuning before each request
>>>
>>>      Call mmc_retune() prior to ->request()
>>>
>>>     mmc: sdhci: Change to new way of doing re-tuning
>>>
>>>      Updated to reflect the changes above.
>>
>> Hi
>>
>> What is the status of this?
>
> Hi
>
> What needs to be done to move this forward?

I need some more time to think about it, from a design perspective.
Give me a few days, please.

Kind regards
Uffe

>
> Regards
> Adrian
>

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

[Ulf Hansson]

On 8 April 2015 at 09:42, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 02/04/15 17:00, Ulf Hansson wrote:
>> [...]
>>
>>>>>>>>>>> Also SDIO cards can have a custom sleep state where tuning won't work.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Our SDIO wifi device has such a state and it can only come out of it
>>>>>>>>>> upon
>>>>>>>>>> CMD52 write or CMD14 (ExitSleep).
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> So how will the mmc core know about these states? I guess we will
>>>>>>>>> require SDIO func drivers to deal with enable/disable or hold/release
>>>>>>>>> of re-tuning then?
>>>>>>>>
>>>>>>>>
>>>>>>>> This is actually why in the past we tried to only kick off retuning
>>>>>>>> before a
>>>>>>>> request that requires use of data line(s) so mmc core (or sdhci) would not
>>>>>>>> do re-tuning when SDIO func used CMD52 to wakeup the device. I never tried
>>>>>>>> CMD14 approach and not even sure from which spec it comes (maybe eMMC).
>>>>>>>
>>>>>>> That's an interesting idea. It would eliminate the need for SDIO func
>>>>>>> drivers to care about holding/releasing re-tuning.
>>>>>>>
>>>>>>> Would be nice to hear about Adrian's thoughts around this as well.
>>>>>>
>>>>>> I don't see how it would work because re-tuning is needed after the host
>>>>>> controller runtime resumes. i.e. once the SDIO wifi card stops being active
>>>>>> the host controller will runtime suspend.
>>>>>
>>>>> Why do we always need to re-tune from this phase?
>>>>>
>>>>> What Arend points out is that we could "delay" the re-tune until we
>>>>> know there is a DATA request. Wouldn't that work for SDHCI as well?
>>>>
>>>> You beat me to it, but that is indeed what I meant.
>>>
>>> The CMD line needs tuning too, so delaying tuning on every command will
>>> cause errors. It is better to hold tuning for the specific command used to
>>> wake-up.
>>
>> Hmm.
>>
>> This touches the discussion where Neil Brown also was involved, around
>> how to handle "idle operations" for SDIO.
>>
>> How does SDIO func drivers detects "request inactivity", which
>> triggers them to send its device to "sleep mode"? That answer should
>> be runtime PM.
>
> Using runtime pm for custom sleep states would seem to conflict with its use
> by the power domain. For example ACPI will enumerate embedded SDIO function
> devices and link them to the ACPI power domain. Then ACPI will choose the
> lowest power state for runtime suspend.
>
> It isn't obvious how a driver that doesn't know its power domain should
> handle runtime pm, other than assuming it means power off.

I am not sure what you mean by "power off" in this context. Is that
the power of the actual SDIO card? I don't think so, but I may be
wrong.

To enumerate a SDIO card the mmc core first needs to apply power to
it. At this point the PM domain isn't yet attached to the SDIO func
device (the device doesn't even exist yet) and thus it can't be used
to provide power the card. Right?

>
>>
>> So, from mmc core perspective we should be able to get notifications
>> through runtime PM callbacks (from mmc or sdio bus) to understand
>> whether we need to hold of re-tune.
>
> That doesn't match the requirement. Re-tuning needs to be held for the
> wake-up command, not while asleep.
>

Why should we ever want to re-tune if the card is in a sleep state?
Isn't it better to postpone that until it wakes up?

Kind regards
Uffe

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

[Ulf Hansson]

On 2 April 2015 at 18:18, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 2/04/2015 4:05 p.m., Ulf Hansson wrote:
>>
>> [...]
>>
>>>>
>>>>>    - ability to enable / disable re-tuning
>>>>
>>>>
>>>> Handled internally by the mmc core.
>>>
>>>
>>> The host controller driver enables re-tuning based on whether the host
>>> controller requires it for that transfer mode. For example, only the
>>> SDHCI
>>> host controller knows if tuning is required for SDR50 mode according to
>>> the
>>> SDHCI capability register bit 45.
>>
>>
>> That seems a bit silly.
>>
>> All hosts wants the re-tuning to be "enabled" if the current used
>> speed mode requires it. It's not a host driver thing to deal with,
>> just the core.
>
>
> No it is up to the host controller. That is how it is in the
> SD Host Controller Specification. Both whether to tune SDR50 and
> whether to run a re-tuning timer.
>
> Tuning is inherently a host controller problem. The card can always
> receive correctly from the host, but the host has to adjust its
> "sampling point" to receive from the card. Only the host knows its
> capabilities in this regard.

The requirement of doing re-tuning is stated in the eMMC/SD specs. The
SDHCI spec is just adopting to what these specs already states. Then,
I think the mmc core is the only one, who shall be able of
enable/disable re-tuning and thus we shouldn't need any APIs for that.

Hosts needs to be able to flag that re-tuning is needed and to
configure the re-tuning timeout (optional).

[...]

Kind regards
Uffe

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

[Adrian Hunter]

On 13/04/15 15:30, Ulf Hansson wrote:
> On 2 April 2015 at 18:18, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 2/04/2015 4:05 p.m., Ulf Hansson wrote:
>>>
>>> [...]
>>>
>>>>>
>>>>>>    - ability to enable / disable re-tuning
>>>>>
>>>>>
>>>>> Handled internally by the mmc core.
>>>>
>>>>
>>>> The host controller driver enables re-tuning based on whether the host
>>>> controller requires it for that transfer mode. For example, only the
>>>> SDHCI
>>>> host controller knows if tuning is required for SDR50 mode according to
>>>> the
>>>> SDHCI capability register bit 45.
>>>
>>>
>>> That seems a bit silly.
>>>
>>> All hosts wants the re-tuning to be "enabled" if the current used
>>> speed mode requires it. It's not a host driver thing to deal with,
>>> just the core.
>>
>>
>> No it is up to the host controller. That is how it is in the
>> SD Host Controller Specification. Both whether to tune SDR50 and
>> whether to run a re-tuning timer.
>>
>> Tuning is inherently a host controller problem. The card can always
>> receive correctly from the host, but the host has to adjust its
>> "sampling point" to receive from the card. Only the host knows its
>> capabilities in this regard.
> 
> The requirement of doing re-tuning is stated in the eMMC/SD specs. The
> SDHCI spec is just adopting to what these specs already states. Then,
> I think the mmc core is the only one, who shall be able of
> enable/disable re-tuning and thus we shouldn't need any APIs for that.
> 
> Hosts needs to be able to flag that re-tuning is needed and to
> configure the re-tuning timeout (optional).

OK sent patch set V5

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

[Adrian Hunter]

On 13/04/15 15:07, Ulf Hansson wrote:
> On 8 April 2015 at 09:42, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 02/04/15 17:00, Ulf Hansson wrote:
>>> [...]
>>>
>>>>>>>>>>>> Also SDIO cards can have a custom sleep state where tuning won't work.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Our SDIO wifi device has such a state and it can only come out of it
>>>>>>>>>>> upon
>>>>>>>>>>> CMD52 write or CMD14 (ExitSleep).
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> So how will the mmc core know about these states? I guess we will
>>>>>>>>>> require SDIO func drivers to deal with enable/disable or hold/release
>>>>>>>>>> of re-tuning then?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This is actually why in the past we tried to only kick off retuning
>>>>>>>>> before a
>>>>>>>>> request that requires use of data line(s) so mmc core (or sdhci) would not
>>>>>>>>> do re-tuning when SDIO func used CMD52 to wakeup the device. I never tried
>>>>>>>>> CMD14 approach and not even sure from which spec it comes (maybe eMMC).
>>>>>>>>
>>>>>>>> That's an interesting idea. It would eliminate the need for SDIO func
>>>>>>>> drivers to care about holding/releasing re-tuning.
>>>>>>>>
>>>>>>>> Would be nice to hear about Adrian's thoughts around this as well.
>>>>>>>
>>>>>>> I don't see how it would work because re-tuning is needed after the host
>>>>>>> controller runtime resumes. i.e. once the SDIO wifi card stops being active
>>>>>>> the host controller will runtime suspend.
>>>>>>
>>>>>> Why do we always need to re-tune from this phase?
>>>>>>
>>>>>> What Arend points out is that we could "delay" the re-tune until we
>>>>>> know there is a DATA request. Wouldn't that work for SDHCI as well?
>>>>>
>>>>> You beat me to it, but that is indeed what I meant.
>>>>
>>>> The CMD line needs tuning too, so delaying tuning on every command will
>>>> cause errors. It is better to hold tuning for the specific command used to
>>>> wake-up.
>>>
>>> Hmm.
>>>
>>> This touches the discussion where Neil Brown also was involved, around
>>> how to handle "idle operations" for SDIO.
>>>
>>> How does SDIO func drivers detects "request inactivity", which
>>> triggers them to send its device to "sleep mode"? That answer should
>>> be runtime PM.
>>
>> Using runtime pm for custom sleep states would seem to conflict with its use
>> by the power domain. For example ACPI will enumerate embedded SDIO function
>> devices and link them to the ACPI power domain. Then ACPI will choose the
>> lowest power state for runtime suspend.
>>
>> It isn't obvious how a driver that doesn't know its power domain should
>> handle runtime pm, other than assuming it means power off.
> 
> I am not sure what you mean by "power off" in this context. Is that

I guess "power off" is the wrong term. They might have to assume they have
lost device context.

You should ask the power management mailing list what assumptions a device
driver can make about the use of runtime pm. If you do, please cc me.

> the power of the actual SDIO card? I don't think so, but I may be
> wrong.

I was talking about SDIO function devices.

> 
> To enumerate a SDIO card the mmc core first needs to apply power to
> it. At this point the PM domain isn't yet attached to the SDIO func
> device (the device doesn't even exist yet) and thus it can't be used
> to provide power the card. Right?

In the ACPI case the SDIO function device ACPI nodes are children of the
host controller ACPI node. One possibility is to have the host controller
driver power on its children.

> 
>>
>>>
>>> So, from mmc core perspective we should be able to get notifications
>>> through runtime PM callbacks (from mmc or sdio bus) to understand
>>> whether we need to hold of re-tune.
>>
>> That doesn't match the requirement. Re-tuning needs to be held for the
>> wake-up command, not while asleep.
>>
> 
> Why should we ever want to re-tune if the card is in a sleep state?
> Isn't it better to postpone that until it wakes up?

That is what I was saying i.e. hold re-tuning for the wake-up command.
So if re-tuning is needed it is done after the wake-up command.

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

[Arend van Spriel]

On 04/14/15 15:38, Adrian Hunter wrote:
>> To enumerate a SDIO card the mmc core first needs to apply power to
>> >  it. At this point the PM domain isn't yet attached to the SDIO func
>> >  device (the device doesn't even exist yet) and thus it can't be used
>> >  to provide power the card. Right?
> In the ACPI case the SDIO function device ACPI nodes are children of the
> host controller ACPI node. One possibility is to have the host controller
> driver power on its children.

This has recently been added to the DT as well as some SDIO devices 
first need to be powered to be discoverable.

Regards,
Arend

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

[Ulf Hansson]

[...]

>>>
>>> Using runtime pm for custom sleep states would seem to conflict with its use
>>> by the power domain. For example ACPI will enumerate embedded SDIO function
>>> devices and link them to the ACPI power domain. Then ACPI will choose the
>>> lowest power state for runtime suspend.
>>>
>>> It isn't obvious how a driver that doesn't know its power domain should
>>> handle runtime pm, other than assuming it means power off.
>>
>> I am not sure what you mean by "power off" in this context. Is that
>
> I guess "power off" is the wrong term. They might have to assume they have
> lost device context.
>
> You should ask the power management mailing list what assumptions a device
> driver can make about the use of runtime pm. If you do, please cc me.
>
>> the power of the actual SDIO card? I don't think so, but I may be
>> wrong.
>
> I was talking about SDIO function devices.

OK!

>
>>
>> To enumerate a SDIO card the mmc core first needs to apply power to
>> it. At this point the PM domain isn't yet attached to the SDIO func
>> device (the device doesn't even exist yet) and thus it can't be used
>> to provide power the card. Right?
>
> In the ACPI case the SDIO function device ACPI nodes are children of the
> host controller ACPI node. One possibility is to have the host controller
> driver power on its children.
>
>>
>>>
>>>>
>>>> So, from mmc core perspective we should be able to get notifications
>>>> through runtime PM callbacks (from mmc or sdio bus) to understand
>>>> whether we need to hold of re-tune.
>>>
>>> That doesn't match the requirement. Re-tuning needs to be held for the
>>> wake-up command, not while asleep.
>>>
>>
>> Why should we ever want to re-tune if the card is in a sleep state?
>> Isn't it better to postpone that until it wakes up?
>
> That is what I was saying i.e. hold re-tuning for the wake-up command.
> So if re-tuning is needed it is done after the wake-up command.
>

Okay. So that means we might be able to use runtime PM from the SDIO
func driver to notify the mmc core through the mmc or sdio bus's
runtime PM callback to understand whether we should hold/allow
re-tune.

That do requires some additional re-work, both from mmc core point of
view and from SDIO func drivers point of view. I will have a look in
more detail to see if this really is viable.

The goal from my side is to prevent us from exporting unnecessary APIs
and thus mmc_retune_hold() and mmc_retune_release() could also be
internal functions of the mmc core.

Kind regards
Uffe

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

[Adrian Hunter]

On 16/04/15 10:24, Ulf Hansson wrote:
> [...]
> 
>>>>
>>>> Using runtime pm for custom sleep states would seem to conflict with its use
>>>> by the power domain. For example ACPI will enumerate embedded SDIO function
>>>> devices and link them to the ACPI power domain. Then ACPI will choose the
>>>> lowest power state for runtime suspend.
>>>>
>>>> It isn't obvious how a driver that doesn't know its power domain should
>>>> handle runtime pm, other than assuming it means power off.
>>>
>>> I am not sure what you mean by "power off" in this context. Is that
>>
>> I guess "power off" is the wrong term. They might have to assume they have
>> lost device context.
>>
>> You should ask the power management mailing list what assumptions a device
>> driver can make about the use of runtime pm. If you do, please cc me.
>>
>>> the power of the actual SDIO card? I don't think so, but I may be
>>> wrong.
>>
>> I was talking about SDIO function devices.
> 
> OK!
> 
>>
>>>
>>> To enumerate a SDIO card the mmc core first needs to apply power to
>>> it. At this point the PM domain isn't yet attached to the SDIO func
>>> device (the device doesn't even exist yet) and thus it can't be used
>>> to provide power the card. Right?
>>
>> In the ACPI case the SDIO function device ACPI nodes are children of the
>> host controller ACPI node. One possibility is to have the host controller
>> driver power on its children.
>>
>>>
>>>>
>>>>>
>>>>> So, from mmc core perspective we should be able to get notifications
>>>>> through runtime PM callbacks (from mmc or sdio bus) to understand
>>>>> whether we need to hold of re-tune.
>>>>
>>>> That doesn't match the requirement. Re-tuning needs to be held for the
>>>> wake-up command, not while asleep.
>>>>
>>>
>>> Why should we ever want to re-tune if the card is in a sleep state?
>>> Isn't it better to postpone that until it wakes up?
>>
>> That is what I was saying i.e. hold re-tuning for the wake-up command.
>> So if re-tuning is needed it is done after the wake-up command.
>>
> 
> Okay. So that means we might be able to use runtime PM from the SDIO
> func driver to notify the mmc core through the mmc or sdio bus's
> runtime PM callback to understand whether we should hold/allow
> re-tune.

I don't see how that would work. Only the SDIO function driver knows that
tuning can't be done before it's wake-up command. So you still need an API.

> 
> That do requires some additional re-work, both from mmc core point of
> view and from SDIO func drivers point of view. I will have a look in
> more detail to see if this really is viable.
> 
> The goal from my side is to prevent us from exporting unnecessary APIs
> and thus mmc_retune_hold() and mmc_retune_release() could also be
> internal functions of the mmc core.

That is fine if you have an alternative, but we can't wait forever.

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

[Ulf Hansson]

On 16 April 2015 at 10:59, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 16/04/15 10:24, Ulf Hansson wrote:
>> [...]
>>
>>>>>
>>>>> Using runtime pm for custom sleep states would seem to conflict with its use
>>>>> by the power domain. For example ACPI will enumerate embedded SDIO function
>>>>> devices and link them to the ACPI power domain. Then ACPI will choose the
>>>>> lowest power state for runtime suspend.
>>>>>
>>>>> It isn't obvious how a driver that doesn't know its power domain should
>>>>> handle runtime pm, other than assuming it means power off.
>>>>
>>>> I am not sure what you mean by "power off" in this context. Is that
>>>
>>> I guess "power off" is the wrong term. They might have to assume they have
>>> lost device context.
>>>
>>> You should ask the power management mailing list what assumptions a device
>>> driver can make about the use of runtime pm. If you do, please cc me.
>>>
>>>> the power of the actual SDIO card? I don't think so, but I may be
>>>> wrong.
>>>
>>> I was talking about SDIO function devices.
>>
>> OK!
>>
>>>
>>>>
>>>> To enumerate a SDIO card the mmc core first needs to apply power to
>>>> it. At this point the PM domain isn't yet attached to the SDIO func
>>>> device (the device doesn't even exist yet) and thus it can't be used
>>>> to provide power the card. Right?
>>>
>>> In the ACPI case the SDIO function device ACPI nodes are children of the
>>> host controller ACPI node. One possibility is to have the host controller
>>> driver power on its children.
>>>
>>>>
>>>>>
>>>>>>
>>>>>> So, from mmc core perspective we should be able to get notifications
>>>>>> through runtime PM callbacks (from mmc or sdio bus) to understand
>>>>>> whether we need to hold of re-tune.
>>>>>
>>>>> That doesn't match the requirement. Re-tuning needs to be held for the
>>>>> wake-up command, not while asleep.
>>>>>
>>>>
>>>> Why should we ever want to re-tune if the card is in a sleep state?
>>>> Isn't it better to postpone that until it wakes up?
>>>
>>> That is what I was saying i.e. hold re-tuning for the wake-up command.
>>> So if re-tuning is needed it is done after the wake-up command.
>>>
>>
>> Okay. So that means we might be able to use runtime PM from the SDIO
>> func driver to notify the mmc core through the mmc or sdio bus's
>> runtime PM callback to understand whether we should hold/allow
>> re-tune.
>
> I don't see how that would work. Only the SDIO function driver knows that
> tuning can't be done before it's wake-up command. So you still need an API.

Let me elaborate on what I have in mind.

The SDIO function driver do pm_runtime_put*() when it's ready to allow
its device to go into sleep state.

At some point later, the sleep state is about to be entered. If there
are any specific operations the SDIO func driver needs to do to put
its device into sleep state, that should be managed through the SDIO
func driver's runtime PM ->suspend() callback (potentially a PM domain
may be involved as well).

When the mmc/sdio bus' runtime PM ->suspend() callback will be
triggered, it indicates to the mmc core to hold retune. So, when the
sleep state of the SDIO func device has been reached, re-tune is being
held by the mmc core.

At some point later, an SDIO irq is triggered.

The SDIO func driver's irq handler will be triggered and should make
sure the needed wakeup SDIO command to be sent.

When the SDIO wakeup command has been sent, the SDIO func driver will
invoke pm_runtime_get() to fully wakeup its func device from the sleep
state.

The mmc/sdio bus' runtime PM ->resume() callback will thus be invoked
as a part of the wakeup sequence of the SDIO func device, but _after_
the actual SDIO wakeup command has been sent. The mmc/sdio bus'
runtime PM ->resume() callback shall then release retune.

Finally the SDIO func driver starts handling the request.

>
>>
>> That do requires some additional re-work, both from mmc core point of
>> view and from SDIO func drivers point of view. I will have a look in
>> more detail to see if this really is viable.
>>
>> The goal from my side is to prevent us from exporting unnecessary APIs
>> and thus mmc_retune_hold() and mmc_retune_release() could also be
>> internal functions of the mmc core.
>
> That is fine if you have an alternative, but we can't wait forever.
>

True.

So, I will continue to investigate the runtime PM proposal and I
appreciate to get your and other opinions.

While I do that, let's continue to move forward with the approach you
have taken in @subject patchset.

Kind regards
Uffe