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

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

[Adrian Hunter]

Hi

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


Changes in V6:

    mmc: host: Add facility to support re-tuning
	Don't export functions only used in core

    mmc: core: Enable / disable re-tuning
	Ensure re-tuning timer is disabled in SDIO suspend

    mmc: core: Hold re-tuning while bkops ongoing
	Hold re-tuning always and release only if
	bkops is not ongoing.

    mmc: mmc: Hold re-tuning if the card is put to sleep
	Hold re-tuning for mmc_sleep even if card is
	immediately powered off, disabling re-tuning.

Changes in V5:

    mmc: host: Add facility to support re-tuning
	Make mmc_retune_enable() / mmc_retune_disable()
	only called from core.

    mmc: core: Enable / disable re-tuning
	Replaces mmc: core: Disable re-tuning when card is no longer initialized
	Enables re-tuning when tuning is executed

    mmc: sdhci: Change to new way of doing re-tuning
	Set host->retune_period instead of enabling re-tuning.

Changes in V4:

    These patches now depend on Ulf's patch:
	mmc: core: Enable runtime PM management of host devices

    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: Enable / disable re-tuning
      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: 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    |  51 +++++++++++++++---
 drivers/mmc/core/core.h    |   2 +
 drivers/mmc/core/host.c    |  85 ++++++++++++++++++++++++++++++
 drivers/mmc/core/host.h    |   6 +++
 drivers/mmc/core/mmc.c     | 102 ++++++++++++++++++++++++++++++++++--
 drivers/mmc/core/mmc_ops.c |  44 ++++++++++------
 drivers/mmc/core/mmc_ops.h |   1 +
 drivers/mmc/core/sdio.c    |   2 +
 drivers/mmc/host/sdhci.c   | 126 ++++++++-------------------------------------
 drivers/mmc/host/sdhci.h   |   3 --
 include/linux/mmc/host.h   |  28 ++++++++++
 13 files changed, 330 insertions(+), 135 deletions(-)


Regards
Adrian

[PATCH V6 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 7 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 */
  unsigned int		retune_period;  /* re-tuning period in secs */
  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  | 68 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/mmc/core/host.h  |  6 +++++
 include/linux/mmc/host.h | 28 ++++++++++++++++++++
 3 files changed, 102 insertions(+)

diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 8be0df7..e90e02f 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -301,6 +301,73 @@ static inline void mmc_host_clk_sysfs_init(struct mmc_host *host)
 
 #endif
 
+void mmc_retune_enable(struct mmc_host *host)
+{
+	host->can_retune = 1;
+	if (host->retune_period)
+		mod_timer(&host->retune_timer,
+			  jiffies + host->retune_period * HZ);
+}
+
+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;
+}
+
+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;
+}
+
+void mmc_retune_release(struct mmc_host *host)
+{
+	if (host->hold_retune)
+		host->hold_retune -= 1;
+	else
+		WARN_ON(1);
+}
+
+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;
+}
+
+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 +571,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/drivers/mmc/core/host.h b/drivers/mmc/core/host.h
index f2ab9e5..992bf53 100644
--- a/drivers/mmc/core/host.h
+++ b/drivers/mmc/core/host.h
@@ -15,5 +15,11 @@
 int mmc_register_host_class(void);
 void mmc_unregister_host_class(void);
 
+void mmc_retune_enable(struct mmc_host *host);
+void mmc_retune_disable(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);
+
 #endif
 
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index b5bedae..9c9e51b 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,18 @@ 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 */
+	unsigned int		retune_period;	/* re-tuning period in secs */
+	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 +522,23 @@ static inline bool mmc_card_hs400(struct mmc_card *card)
 	return card->host->ios.timing == MMC_TIMING_MMC_HS400;
 }
 
+void mmc_retune_timer_stop(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 V6 02/15] mmc: core: Enable / disable re-tuning

[Adrian Hunter]

Enable re-tuning when tuning is executed and
disable re-tuning when card is no longer initialized.

In the case of SDIO suspend, the card can keep power.
In that case, re-tuning need not be disabled, but ensure
the re-tuning timer is disabled.

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

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index c296bc0..dd43f9b 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1109,6 +1109,8 @@ int mmc_execute_tuning(struct mmc_card *card)
 
 	if (err)
 		pr_err("%s: tuning execution failed\n", mmc_hostname(host));
+	else
+		mmc_retune_enable(host);
 
 	return err;
 }
@@ -1140,6 +1142,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
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 5bc6c7d..ee7bfd4 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -936,6 +936,8 @@ static int mmc_sdio_suspend(struct mmc_host *host)
 
 	if (!mmc_card_keep_power(host))
 		mmc_power_off(host);
+	else
+		mmc_retune_timer_stop(host);
 
 	return 0;
 }
-- 
1.9.1

[PATCH V6 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 dd43f9b..a9936cb 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 V6 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 a9936cb..08bf7e3 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 V6 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 | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 0ea042d..4cad5f0 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -19,6 +19,7 @@
 #include <linux/mmc/mmc.h>
 
 #include "core.h"
+#include "host.h"
 #include "mmc_ops.h"
 
 #define MMC_OPS_TIMEOUT_MS	(10 * 60 * 1000) /* 10 minute timeout */
@@ -474,6 +475,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 +509,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 +532,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 +546,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 V6 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 08bf7e3..dbd7a77 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1995,6 +1995,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.
@@ -2098,6 +2100,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 V6 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 dbd7a77..7aae0ff 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -318,12 +318,15 @@ void mmc_start_bkops(struct mmc_card *card, bool from_exception)
 		use_busy_signal = false;
 	}
 
+	mmc_retune_hold(card->host);
+
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 			EXT_CSD_BKOPS_START, 1, timeout,
 			use_busy_signal, true, false);
 	if (err) {
 		pr_warn("%s: Error %d starting bkops\n",
 			mmc_hostname(card->host), err);
+		mmc_retune_release(card->host);
 		goto out;
 	}
 
@@ -334,6 +337,8 @@ void mmc_start_bkops(struct mmc_card *card, bool from_exception)
 	 */
 	if (!use_busy_signal)
 		mmc_card_set_doing_bkops(card);
+	else
+		mmc_retune_release(card->host);
 out:
 	mmc_release_host(card->host);
 }
@@ -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 V6 08/15] mmc: mmc: Hold re-tuning if the card is put to sleep

[Adrian Hunter]

Currently "mmc sleep" is used before power off and
is not paired with waking up. Nevertheless hold
re-tuning.

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

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index f36c76f..daf9954 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -21,6 +21,7 @@
 #include <linux/mmc/mmc.h>
 
 #include "core.h"
+#include "host.h"
 #include "bus.h"
 #include "mmc_ops.h"
 #include "sd_ops.h"
@@ -1504,6 +1505,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};
@@ -1631,6 +1633,7 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
 	int err = 0;
 	unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT :
 					EXT_CSD_POWER_OFF_LONG;
+	bool retune_release = false;
 
 	BUG_ON(!host);
 	BUG_ON(!host->card);
@@ -1651,17 +1654,22 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
 		goto out;
 
 	if (mmc_can_poweroff_notify(host->card) &&
-		((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend))
+		((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) {
 		err = mmc_poweroff_notify(host->card, notify_type);
-	else if (mmc_can_sleep(host->card))
+	} else if (mmc_can_sleep(host->card)) {
+		mmc_retune_hold(host);
 		err = mmc_sleep(host);
-	else if (!mmc_host_is_spi(host))
+	} else if (!mmc_host_is_spi(host)) {
 		err = mmc_deselect_cards(host);
+	}
 
 	if (!err) {
 		mmc_power_off(host);
 		mmc_card_set_suspended(host->card);
 	}
+
+	if (retune_release)
+		mmc_retune_release(host);
 out:
 	mmc_release_host(host);
 	return err;
-- 
1.9.1

[PATCH V6 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 4cad5f0..0e9ae1c 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -450,6 +450,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
@@ -558,20 +573,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 V6 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 e90e02f..86c495b 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -340,6 +340,7 @@ void mmc_retune_release(struct mmc_host *host)
 
 int mmc_retune(struct mmc_host *host)
 {
+	bool return_to_hs400 = false;
 	int err;
 
 	if (host->retune_now)
@@ -354,8 +355,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 daf9954..70a091d 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1092,6 +1092,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 V6 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 | 112 ++++++-----------------------------------------
 drivers/mmc/host/sdhci.h |   3 --
 2 files changed, 13 insertions(+), 102 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index c80287a..b345844 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,18 @@ 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;
+	host->mmc->retune_period = err ? 0 : tuning_count;
 
 	sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
 	sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
@@ -2337,20 +2286,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 +2663,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 +2714,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 +2750,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 +2794,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 +3329,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 V6 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 b345844..d11fae7 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2307,8 +2307,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);
@@ -2433,13 +2435,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 V6 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 2c25271..325ae1e 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 V6 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 325ae1e..509fcca 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 V6 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 7aae0ff..0b25e70 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2365,7 +2365,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 V6 02/15] mmc: core: Enable / disable re-tuning

[Ulf Hansson]

On 20 April 2015 at 14:09, Adrian Hunter <adrian.hunter@intel.com> wrote:
> Enable re-tuning when tuning is executed and
> disable re-tuning when card is no longer initialized.
>
> In the case of SDIO suspend, the card can keep power.
> In that case, re-tuning need not be disabled, but ensure
> the re-tuning timer is disabled.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  drivers/mmc/core/core.c | 4 ++++
>  drivers/mmc/core/sdio.c | 2 ++
>  2 files changed, 6 insertions(+)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index c296bc0..dd43f9b 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1109,6 +1109,8 @@ int mmc_execute_tuning(struct mmc_card *card)
>
>         if (err)
>                 pr_err("%s: tuning execution failed\n", mmc_hostname(host));
> +       else
> +               mmc_retune_enable(host);
>
>         return err;
>  }
> @@ -1140,6 +1142,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
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index 5bc6c7d..ee7bfd4 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -936,6 +936,8 @@ static int mmc_sdio_suspend(struct mmc_host *host)
>
>         if (!mmc_card_keep_power(host))
>                 mmc_power_off(host);
> +       else
> +               mmc_retune_timer_stop(host);
>

We also need to re-start the timer at the end of mmc_sdio_resume() in
case of mmc_card_keep_power().

>         return 0;
>  }
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kind regards
Uffe

Re: [PATCH V6 08/15] mmc: mmc: Hold re-tuning if the card is put to sleep

[Ulf Hansson]

On 20 April 2015 at 14:09, Adrian Hunter <adrian.hunter@intel.com> wrote:
> Currently "mmc sleep" is used before power off and
> is not paired with waking up. Nevertheless hold
> re-tuning.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  drivers/mmc/core/mmc.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index f36c76f..daf9954 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -21,6 +21,7 @@
>  #include <linux/mmc/mmc.h>
>
>  #include "core.h"
> +#include "host.h"
>  #include "bus.h"
>  #include "mmc_ops.h"
>  #include "sd_ops.h"
> @@ -1504,6 +1505,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};
> @@ -1631,6 +1633,7 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>         int err = 0;
>         unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT :
>                                         EXT_CSD_POWER_OFF_LONG;
> +       bool retune_release = false;
>
>         BUG_ON(!host);
>         BUG_ON(!host->card);
> @@ -1651,17 +1654,22 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>                 goto out;
>
>         if (mmc_can_poweroff_notify(host->card) &&
> -               ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend))
> +               ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) {
>                 err = mmc_poweroff_notify(host->card, notify_type);
> -       else if (mmc_can_sleep(host->card))
> +       } else if (mmc_can_sleep(host->card)) {
> +               mmc_retune_hold(host);
>                 err = mmc_sleep(host);
> -       else if (!mmc_host_is_spi(host))
> +       } else if (!mmc_host_is_spi(host)) {
>                 err = mmc_deselect_cards(host);
> +       }
>
>         if (!err) {
>                 mmc_power_off(host);
>                 mmc_card_set_suspended(host->card);
>         }
> +
> +       if (retune_release)
> +               mmc_retune_release(host);
>  out:
>         mmc_release_host(host);
>         return err;
> --
> 1.9.1
>

According to our previous discussions I have given this some more thinking.

I don't think we can allow to hold/disable re-tune in this path at
all. That's because we are claiming the host here and the sleep
command might then be the first command we invoke during the system PM
sequence.

That means sdhci might have flagged need_retune, since it's been
runtime PM suspended. And for those scenarios I guess we really need
to do a re-tune prior sending the sleep command, right?

Earlier I only had the re-tune timer in mind, which is why I was less
restrictive and suggesting you to add hold/disable. Sorry about that.

Now, with the above in mind I believe you have similar issues with
patch5 (mmc: core: Hold re-tuning during switch commands) and patch6
(mmc: core: Hold re-tuning during erase commands). And that's because
there are cases when the switch/erase commands are the first commands
sent, after the sdhci host has been runtime PM suspended. I guess we
need a way to make sure we don't hold re-tune for these cases.

An option to deal with that is to use a separate flag set by host
drivers, though the mmc_needs_retune() API and let that one override
another.

Forgive me for pushing you back and forth for how to do this, but it
seems like we still have some outstanding issues to resolve.

Kind regards
Uffe

Re: [PATCH V6 02/15] mmc: core: Enable / disable re-tuning

[Adrian Hunter]

On 21/04/15 11:59, Ulf Hansson wrote:
> On 20 April 2015 at 14:09, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> Enable re-tuning when tuning is executed and
>> disable re-tuning when card is no longer initialized.
>>
>> In the case of SDIO suspend, the card can keep power.
>> In that case, re-tuning need not be disabled, but ensure
>> the re-tuning timer is disabled.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>  drivers/mmc/core/core.c | 4 ++++
>>  drivers/mmc/core/sdio.c | 2 ++
>>  2 files changed, 6 insertions(+)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index c296bc0..dd43f9b 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -1109,6 +1109,8 @@ int mmc_execute_tuning(struct mmc_card *card)
>>
>>         if (err)
>>                 pr_err("%s: tuning execution failed\n", mmc_hostname(host));
>> +       else
>> +               mmc_retune_enable(host);
>>
>>         return err;
>>  }
>> @@ -1140,6 +1142,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
>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>> index 5bc6c7d..ee7bfd4 100644
>> --- a/drivers/mmc/core/sdio.c
>> +++ b/drivers/mmc/core/sdio.c
>> @@ -936,6 +936,8 @@ static int mmc_sdio_suspend(struct mmc_host *host)
>>
>>         if (!mmc_card_keep_power(host))
>>                 mmc_power_off(host);
>> +       else
>> +               mmc_retune_timer_stop(host);
>>
> 
> We also need to re-start the timer at the end of mmc_sdio_resume() in
> case of mmc_card_keep_power().

It seems to me if a timer is being used, and the suspend can last an
arbitrary length of time, then the safe thing to do is to re-tune after
resume. i.e.

        if (!mmc_card_keep_power(host)) {
                mmc_power_off(host);
        } else if (host->host->retune_period) {
                mmc_retune_timer_stop(host);
		mmc_retune_needed(host);
	}

That is what sdhci has to do anyway.

> 
>>         return 0;
>>  }
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> Kind regards
> Uffe
> 
> 

Re: [PATCH V6 08/15] mmc: mmc: Hold re-tuning if the card is put to sleep

[Adrian Hunter]

On 21/04/15 12:42, Ulf Hansson wrote:
> On 20 April 2015 at 14:09, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> Currently "mmc sleep" is used before power off and
>> is not paired with waking up. Nevertheless hold
>> re-tuning.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>  drivers/mmc/core/mmc.c | 14 +++++++++++---
>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index f36c76f..daf9954 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -21,6 +21,7 @@
>>  #include <linux/mmc/mmc.h>
>>
>>  #include "core.h"
>> +#include "host.h"
>>  #include "bus.h"
>>  #include "mmc_ops.h"
>>  #include "sd_ops.h"
>> @@ -1504,6 +1505,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};
>> @@ -1631,6 +1633,7 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>>         int err = 0;
>>         unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT :
>>                                         EXT_CSD_POWER_OFF_LONG;
>> +       bool retune_release = false;
>>
>>         BUG_ON(!host);
>>         BUG_ON(!host->card);
>> @@ -1651,17 +1654,22 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>>                 goto out;
>>
>>         if (mmc_can_poweroff_notify(host->card) &&
>> -               ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend))
>> +               ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) {
>>                 err = mmc_poweroff_notify(host->card, notify_type);
>> -       else if (mmc_can_sleep(host->card))
>> +       } else if (mmc_can_sleep(host->card)) {
>> +               mmc_retune_hold(host);
>>                 err = mmc_sleep(host);
>> -       else if (!mmc_host_is_spi(host))
>> +       } else if (!mmc_host_is_spi(host)) {
>>                 err = mmc_deselect_cards(host);
>> +       }
>>
>>         if (!err) {
>>                 mmc_power_off(host);
>>                 mmc_card_set_suspended(host->card);
>>         }
>> +
>> +       if (retune_release)
>> +               mmc_retune_release(host);
>>  out:
>>         mmc_release_host(host);
>>         return err;
>> --
>> 1.9.1
>>
> 
> According to our previous discussions I have given this some more thinking.
> 
> I don't think we can allow to hold/disable re-tune in this path at
> all. That's because we are claiming the host here and the sleep
> command might then be the first command we invoke during the system PM
> sequence.
> 
> That means sdhci might have flagged need_retune, since it's been
> runtime PM suspended. And for those scenarios I guess we really need
> to do a re-tune prior sending the sleep command, right?

Yes, although that is how it works.

Previously I had two functions mmc_retune_hold() and mmc_retune_and_hold()
but after one of the revisions I found that only one was needed. I stuck
with the mmc_retune_hold() name because it doesn't necessarily cause a
re-tune, but only if the hold count was zero and a retune is needed.

> 
> Earlier I only had the re-tune timer in mind, which is why I was less
> restrictive and suggesting you to add hold/disable. Sorry about that.
> 
> Now, with the above in mind I believe you have similar issues with
> patch5 (mmc: core: Hold re-tuning during switch commands) and patch6
> (mmc: core: Hold re-tuning during erase commands). And that's because
> there are cases when the switch/erase commands are the first commands
> sent, after the sdhci host has been runtime PM suspended. I guess we
> need a way to make sure we don't hold re-tune for these cases.
> 
> An option to deal with that is to use a separate flag set by host
> drivers, though the mmc_needs_retune() API and let that one override
> another.
> 
> Forgive me for pushing you back and forth for how to do this, but it

Not a problem. Thanks for persevering.

> seems like we still have some outstanding issues to resolve.

Re: [PATCH V6 08/15] mmc: mmc: Hold re-tuning if the card is put to sleep

[Ulf Hansson]

On 21 April 2015 at 13:00, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 21/04/15 12:42, Ulf Hansson wrote:
>> On 20 April 2015 at 14:09, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> Currently "mmc sleep" is used before power off and
>>> is not paired with waking up. Nevertheless hold
>>> re-tuning.
>>>
>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>> ---
>>>  drivers/mmc/core/mmc.c | 14 +++++++++++---
>>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>> index f36c76f..daf9954 100644
>>> --- a/drivers/mmc/core/mmc.c
>>> +++ b/drivers/mmc/core/mmc.c
>>> @@ -21,6 +21,7 @@
>>>  #include <linux/mmc/mmc.h>
>>>
>>>  #include "core.h"
>>> +#include "host.h"
>>>  #include "bus.h"
>>>  #include "mmc_ops.h"
>>>  #include "sd_ops.h"
>>> @@ -1504,6 +1505,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};
>>> @@ -1631,6 +1633,7 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>>>         int err = 0;
>>>         unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT :
>>>                                         EXT_CSD_POWER_OFF_LONG;
>>> +       bool retune_release = false;
>>>
>>>         BUG_ON(!host);
>>>         BUG_ON(!host->card);
>>> @@ -1651,17 +1654,22 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>>>                 goto out;
>>>
>>>         if (mmc_can_poweroff_notify(host->card) &&
>>> -               ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend))
>>> +               ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) {
>>>                 err = mmc_poweroff_notify(host->card, notify_type);
>>> -       else if (mmc_can_sleep(host->card))
>>> +       } else if (mmc_can_sleep(host->card)) {
>>> +               mmc_retune_hold(host);
>>>                 err = mmc_sleep(host);
>>> -       else if (!mmc_host_is_spi(host))
>>> +       } else if (!mmc_host_is_spi(host)) {
>>>                 err = mmc_deselect_cards(host);
>>> +       }
>>>
>>>         if (!err) {
>>>                 mmc_power_off(host);
>>>                 mmc_card_set_suspended(host->card);
>>>         }
>>> +
>>> +       if (retune_release)
>>> +               mmc_retune_release(host);
>>>  out:
>>>         mmc_release_host(host);
>>>         return err;
>>> --
>>> 1.9.1
>>>
>>
>> According to our previous discussions I have given this some more thinking.
>>
>> I don't think we can allow to hold/disable re-tune in this path at
>> all. That's because we are claiming the host here and the sleep
>> command might then be the first command we invoke during the system PM
>> sequence.
>>
>> That means sdhci might have flagged need_retune, since it's been
>> runtime PM suspended. And for those scenarios I guess we really need
>> to do a re-tune prior sending the sleep command, right?
>
> Yes, although that is how it works.

Ohh, you are one step ahead of me. Good! :-)

>
> Previously I had two functions mmc_retune_hold() and mmc_retune_and_hold()
> but after one of the revisions I found that only one was needed. I stuck
> with the mmc_retune_hold() name because it doesn't necessarily cause a
> re-tune, but only if the hold count was zero and a retune is needed.
>
>>
>> Earlier I only had the re-tune timer in mind, which is why I was less
>> restrictive and suggesting you to add hold/disable. Sorry about that.
>>
>> Now, with the above in mind I believe you have similar issues with
>> patch5 (mmc: core: Hold re-tuning during switch commands) and patch6
>> (mmc: core: Hold re-tuning during erase commands). And that's because
>> there are cases when the switch/erase commands are the first commands
>> sent, after the sdhci host has been runtime PM suspended. I guess we
>> need a way to make sure we don't hold re-tune for these cases.
>>
>> An option to deal with that is to use a separate flag set by host
>> drivers, though the mmc_needs_retune() API and let that one override
>> another.
>>
>> Forgive me for pushing you back and forth for how to do this, but it
>
> Not a problem. Thanks for persevering.
>
>> seems like we still have some outstanding issues to resolve.

So that then more or less leaves us with one outstanding issue. The
SDIO irq wakeup scenario.

How will that work for sdhci?

Your suggestion is to hold re-tune for the SDIO wakeup command. If I
understand correct that could be overridden when the host flags
need_retune from its runtime PM suspend callback, right?

That then mean that the re-tuning will be done prior sending the
wakeup command? That wouldn't work, unless the re-tune command also
act as wakeup, which I doubt.

If I _haven't_ understand correctly and you mean that the SDIO wakeup
command shall be invoked prior re-tuning is done; that would mean that
SDHCI will send a command to the card without first satisfying its
need for a re-tune. And that wouldn't work either, right?

So then the only solution for SDHCI would be to prevent it from being
runtime PM suspended when configured for SDIO. Urgh, that's really
bad.

Comments?

Kind regards
Uffe

>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V6 08/15] mmc: mmc: Hold re-tuning if the card is put to sleep

[Adrian Hunter]

On 21/04/15 14:53, Ulf Hansson wrote:
> On 21 April 2015 at 13:00, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 21/04/15 12:42, Ulf Hansson wrote:
>>> On 20 April 2015 at 14:09, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> Currently "mmc sleep" is used before power off and
>>>> is not paired with waking up. Nevertheless hold
>>>> re-tuning.
>>>>
>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>>> ---
>>>>  drivers/mmc/core/mmc.c | 14 +++++++++++---
>>>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>>> index f36c76f..daf9954 100644
>>>> --- a/drivers/mmc/core/mmc.c
>>>> +++ b/drivers/mmc/core/mmc.c
>>>> @@ -21,6 +21,7 @@
>>>>  #include <linux/mmc/mmc.h>
>>>>
>>>>  #include "core.h"
>>>> +#include "host.h"
>>>>  #include "bus.h"
>>>>  #include "mmc_ops.h"
>>>>  #include "sd_ops.h"
>>>> @@ -1504,6 +1505,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};
>>>> @@ -1631,6 +1633,7 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>>>>         int err = 0;
>>>>         unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT :
>>>>                                         EXT_CSD_POWER_OFF_LONG;
>>>> +       bool retune_release = false;
>>>>
>>>>         BUG_ON(!host);
>>>>         BUG_ON(!host->card);
>>>> @@ -1651,17 +1654,22 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>>>>                 goto out;
>>>>
>>>>         if (mmc_can_poweroff_notify(host->card) &&
>>>> -               ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend))
>>>> +               ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) {
>>>>                 err = mmc_poweroff_notify(host->card, notify_type);
>>>> -       else if (mmc_can_sleep(host->card))
>>>> +       } else if (mmc_can_sleep(host->card)) {
>>>> +               mmc_retune_hold(host);
>>>>                 err = mmc_sleep(host);
>>>> -       else if (!mmc_host_is_spi(host))
>>>> +       } else if (!mmc_host_is_spi(host)) {
>>>>                 err = mmc_deselect_cards(host);
>>>> +       }
>>>>
>>>>         if (!err) {
>>>>                 mmc_power_off(host);
>>>>                 mmc_card_set_suspended(host->card);
>>>>         }
>>>> +
>>>> +       if (retune_release)
>>>> +               mmc_retune_release(host);
>>>>  out:
>>>>         mmc_release_host(host);
>>>>         return err;
>>>> --
>>>> 1.9.1
>>>>
>>>
>>> According to our previous discussions I have given this some more thinking.
>>>
>>> I don't think we can allow to hold/disable re-tune in this path at
>>> all. That's because we are claiming the host here and the sleep
>>> command might then be the first command we invoke during the system PM
>>> sequence.
>>>
>>> That means sdhci might have flagged need_retune, since it's been
>>> runtime PM suspended. And for those scenarios I guess we really need
>>> to do a re-tune prior sending the sleep command, right?
>>
>> Yes, although that is how it works.
> 
> Ohh, you are one step ahead of me. Good! :-)
> 
>>
>> Previously I had two functions mmc_retune_hold() and mmc_retune_and_hold()
>> but after one of the revisions I found that only one was needed. I stuck
>> with the mmc_retune_hold() name because it doesn't necessarily cause a
>> re-tune, but only if the hold count was zero and a retune is needed.
>>
>>>
>>> Earlier I only had the re-tune timer in mind, which is why I was less
>>> restrictive and suggesting you to add hold/disable. Sorry about that.
>>>
>>> Now, with the above in mind I believe you have similar issues with
>>> patch5 (mmc: core: Hold re-tuning during switch commands) and patch6
>>> (mmc: core: Hold re-tuning during erase commands). And that's because
>>> there are cases when the switch/erase commands are the first commands
>>> sent, after the sdhci host has been runtime PM suspended. I guess we
>>> need a way to make sure we don't hold re-tune for these cases.
>>>
>>> An option to deal with that is to use a separate flag set by host
>>> drivers, though the mmc_needs_retune() API and let that one override
>>> another.
>>>
>>> Forgive me for pushing you back and forth for how to do this, but it
>>
>> Not a problem. Thanks for persevering.
>>
>>> seems like we still have some outstanding issues to resolve.
> 
> So that then more or less leaves us with one outstanding issue. The
> SDIO irq wakeup scenario.
> 
> How will that work for sdhci?
> 
> Your suggestion is to hold re-tune for the SDIO wakeup command. If I
> understand correct that could be overridden when the host flags
> need_retune from its runtime PM suspend callback, right?
> 
> That then mean that the re-tuning will be done prior sending the
> wakeup command? That wouldn't work, unless the re-tune command also
> act as wakeup, which I doubt.

The wakeup command has to come first.

> 
> If I _haven't_ understand correctly and you mean that the SDIO wakeup
> command shall be invoked prior re-tuning is done; that would mean that
> SDHCI will send a command to the card without first satisfying its
> need for a re-tune. And that wouldn't work either, right?

My understanding is that the wakeup command will still work but there might
be a CRC error.

Need Arend to comment on this since it is his driver we are talking about.

So the plan would be:
	- re-tuning hold_count is incremented
	- wakeup command is issued (and no re-tuning is done)
	- errors are ignored
	- re-tuning hold_count is decremented
	- continue as normal, re-tuning before the next request as needed

> 
> So then the only solution for SDHCI would be to prevent it from being
> runtime PM suspended when configured for SDIO. Urgh, that's really
> bad.

Yes that would defeat the point of sleeping.

Re: [PATCH V6 08/15] mmc: mmc: Hold re-tuning if the card is put to sleep

[Arend van Spriel]

On 04/21/15 14:26, Adrian Hunter wrote:
> On 21/04/15 14:53, Ulf Hansson wrote:
>> On 21 April 2015 at 13:00, Adrian Hunter<adrian.hunter@intel.com>  wrote:
>>> On 21/04/15 12:42, Ulf Hansson wrote:
>>>> On 20 April 2015 at 14:09, Adrian Hunter<adrian.hunter@intel.com>  wrote:
>>>>> Currently "mmc sleep" is used before power off and
>>>>> is not paired with waking up. Nevertheless hold
>>>>> re-tuning.
>>>>>
>>>>> Signed-off-by: Adrian Hunter<adrian.hunter@intel.com>
>>>>> ---
>>>>>   drivers/mmc/core/mmc.c | 14 +++++++++++---
>>>>>   1 file changed, 11 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>>>> index f36c76f..daf9954 100644
>>>>> --- a/drivers/mmc/core/mmc.c
>>>>> +++ b/drivers/mmc/core/mmc.c
>>>>> @@ -21,6 +21,7 @@
>>>>>   #include<linux/mmc/mmc.h>
>>>>>
>>>>>   #include "core.h"
>>>>> +#include "host.h"
>>>>>   #include "bus.h"
>>>>>   #include "mmc_ops.h"
>>>>>   #include "sd_ops.h"
>>>>> @@ -1504,6 +1505,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};
>>>>> @@ -1631,6 +1633,7 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>>>>>          int err = 0;
>>>>>          unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT :
>>>>>                                          EXT_CSD_POWER_OFF_LONG;
>>>>> +       bool retune_release = false;
>>>>>
>>>>>          BUG_ON(!host);
>>>>>          BUG_ON(!host->card);
>>>>> @@ -1651,17 +1654,22 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>>>>>                  goto out;
>>>>>
>>>>>          if (mmc_can_poweroff_notify(host->card)&&
>>>>> -               ((host->caps2&  MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend))
>>>>> +               ((host->caps2&  MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) {
>>>>>                  err = mmc_poweroff_notify(host->card, notify_type);
>>>>> -       else if (mmc_can_sleep(host->card))
>>>>> +       } else if (mmc_can_sleep(host->card)) {
>>>>> +               mmc_retune_hold(host);
>>>>>                  err = mmc_sleep(host);
>>>>> -       else if (!mmc_host_is_spi(host))
>>>>> +       } else if (!mmc_host_is_spi(host)) {
>>>>>                  err = mmc_deselect_cards(host);
>>>>> +       }
>>>>>
>>>>>          if (!err) {
>>>>>                  mmc_power_off(host);
>>>>>                  mmc_card_set_suspended(host->card);
>>>>>          }
>>>>> +
>>>>> +       if (retune_release)
>>>>> +               mmc_retune_release(host);
>>>>>   out:
>>>>>          mmc_release_host(host);
>>>>>          return err;
>>>>> --
>>>>> 1.9.1
>>>>>
>>>>
>>>> According to our previous discussions I have given this some more thinking.
>>>>
>>>> I don't think we can allow to hold/disable re-tune in this path at
>>>> all. That's because we are claiming the host here and the sleep
>>>> command might then be the first command we invoke during the system PM
>>>> sequence.
>>>>
>>>> That means sdhci might have flagged need_retune, since it's been
>>>> runtime PM suspended. And for those scenarios I guess we really need
>>>> to do a re-tune prior sending the sleep command, right?
>>>
>>> Yes, although that is how it works.
>>
>> Ohh, you are one step ahead of me. Good! :-)
>>
>>>
>>> Previously I had two functions mmc_retune_hold() and mmc_retune_and_hold()
>>> but after one of the revisions I found that only one was needed. I stuck
>>> with the mmc_retune_hold() name because it doesn't necessarily cause a
>>> re-tune, but only if the hold count was zero and a retune is needed.
>>>
>>>>
>>>> Earlier I only had the re-tune timer in mind, which is why I was less
>>>> restrictive and suggesting you to add hold/disable. Sorry about that.
>>>>
>>>> Now, with the above in mind I believe you have similar issues with
>>>> patch5 (mmc: core: Hold re-tuning during switch commands) and patch6
>>>> (mmc: core: Hold re-tuning during erase commands). And that's because
>>>> there are cases when the switch/erase commands are the first commands
>>>> sent, after the sdhci host has been runtime PM suspended. I guess we
>>>> need a way to make sure we don't hold re-tune for these cases.
>>>>
>>>> An option to deal with that is to use a separate flag set by host
>>>> drivers, though the mmc_needs_retune() API and let that one override
>>>> another.
>>>>
>>>> Forgive me for pushing you back and forth for how to do this, but it
>>>
>>> Not a problem. Thanks for persevering.
>>>
>>>> seems like we still have some outstanding issues to resolve.
>>
>> So that then more or less leaves us with one outstanding issue. The
>> SDIO irq wakeup scenario.
>>
>> How will that work for sdhci?
>>
>> Your suggestion is to hold re-tune for the SDIO wakeup command. If I
>> understand correct that could be overridden when the host flags
>> need_retune from its runtime PM suspend callback, right?
>>
>> That then mean that the re-tuning will be done prior sending the
>> wakeup command? That wouldn't work, unless the re-tune command also
>> act as wakeup, which I doubt.
>
> The wakeup command has to come first.
>
>>
>> If I _haven't_ understand correctly and you mean that the SDIO wakeup
>> command shall be invoked prior re-tuning is done; that would mean that
>> SDHCI will send a command to the card without first satisfying its
>> need for a re-tune. And that wouldn't work either, right?
>
> My understanding is that the wakeup command will still work but there might
> be a CRC error.
>
> Need Arend to comment on this since it is his driver we are talking about.

Are we?

> So the plan would be:
> 	- re-tuning hold_count is incremented
> 	- wakeup command is issued (and no re-tuning is done)
> 	- errors are ignored
> 	- re-tuning hold_count is decremented
> 	- continue as normal, re-tuning before the next request as needed
>
>>
>> So then the only solution for SDHCI would be to prevent it from being
>> runtime PM suspended when configured for SDIO. Urgh, that's really
>> bad.
>
> Yes that would defeat the point of sleeping.

I recently submitted a patch in brcmfmac to disable runtime pm for the 
SDIO host controller as it interfered with communication between driver 
and device, ie. driver send request to device but response is never 
received because runtime-pm kicked in. Our sdio func driver does not 
provide runtime-pm (yet) and figured using pm_runtime_forbid() was the 
only way to let the host controller know this fact.

Regards,
Arend

Re: [PATCH V6 08/15] mmc: mmc: Hold re-tuning if the card is put to sleep

[Adrian Hunter]

On 21/04/15 21:25, Arend van Spriel wrote:
> On 04/21/15 14:26, Adrian Hunter wrote:
>> On 21/04/15 14:53, Ulf Hansson wrote:
>>> On 21 April 2015 at 13:00, Adrian Hunter<adrian.hunter@intel.com>  wrote:
>>>> On 21/04/15 12:42, Ulf Hansson wrote:
>>>>> On 20 April 2015 at 14:09, Adrian Hunter<adrian.hunter@intel.com>  wrote:
>>>>>> Currently "mmc sleep" is used before power off and
>>>>>> is not paired with waking up. Nevertheless hold
>>>>>> re-tuning.
>>>>>>
>>>>>> Signed-off-by: Adrian Hunter<adrian.hunter@intel.com>
>>>>>> ---
>>>>>>   drivers/mmc/core/mmc.c | 14 +++++++++++---
>>>>>>   1 file changed, 11 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>>>>> index f36c76f..daf9954 100644
>>>>>> --- a/drivers/mmc/core/mmc.c
>>>>>> +++ b/drivers/mmc/core/mmc.c
>>>>>> @@ -21,6 +21,7 @@
>>>>>>   #include<linux/mmc/mmc.h>
>>>>>>
>>>>>>   #include "core.h"
>>>>>> +#include "host.h"
>>>>>>   #include "bus.h"
>>>>>>   #include "mmc_ops.h"
>>>>>>   #include "sd_ops.h"
>>>>>> @@ -1504,6 +1505,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};
>>>>>> @@ -1631,6 +1633,7 @@ static int _mmc_suspend(struct mmc_host *host,
>>>>>> bool is_suspend)
>>>>>>          int err = 0;
>>>>>>          unsigned int notify_type = is_suspend ?
>>>>>> EXT_CSD_POWER_OFF_SHORT :
>>>>>>                                          EXT_CSD_POWER_OFF_LONG;
>>>>>> +       bool retune_release = false;
>>>>>>
>>>>>>          BUG_ON(!host);
>>>>>>          BUG_ON(!host->card);
>>>>>> @@ -1651,17 +1654,22 @@ static int _mmc_suspend(struct mmc_host *host,
>>>>>> bool is_suspend)
>>>>>>                  goto out;
>>>>>>
>>>>>>          if (mmc_can_poweroff_notify(host->card)&&
>>>>>> -               ((host->caps2&  MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend))
>>>>>> +               ((host->caps2&  MMC_CAP2_FULL_PWR_CYCLE) ||
>>>>>> !is_suspend)) {
>>>>>>                  err = mmc_poweroff_notify(host->card, notify_type);
>>>>>> -       else if (mmc_can_sleep(host->card))
>>>>>> +       } else if (mmc_can_sleep(host->card)) {
>>>>>> +               mmc_retune_hold(host);
>>>>>>                  err = mmc_sleep(host);
>>>>>> -       else if (!mmc_host_is_spi(host))
>>>>>> +       } else if (!mmc_host_is_spi(host)) {
>>>>>>                  err = mmc_deselect_cards(host);
>>>>>> +       }
>>>>>>
>>>>>>          if (!err) {
>>>>>>                  mmc_power_off(host);
>>>>>>                  mmc_card_set_suspended(host->card);
>>>>>>          }
>>>>>> +
>>>>>> +       if (retune_release)
>>>>>> +               mmc_retune_release(host);
>>>>>>   out:
>>>>>>          mmc_release_host(host);
>>>>>>          return err;
>>>>>> -- 
>>>>>> 1.9.1
>>>>>>
>>>>>
>>>>> According to our previous discussions I have given this some more
>>>>> thinking.
>>>>>
>>>>> I don't think we can allow to hold/disable re-tune in this path at
>>>>> all. That's because we are claiming the host here and the sleep
>>>>> command might then be the first command we invoke during the system PM
>>>>> sequence.
>>>>>
>>>>> That means sdhci might have flagged need_retune, since it's been
>>>>> runtime PM suspended. And for those scenarios I guess we really need
>>>>> to do a re-tune prior sending the sleep command, right?
>>>>
>>>> Yes, although that is how it works.
>>>
>>> Ohh, you are one step ahead of me. Good! :-)
>>>
>>>>
>>>> Previously I had two functions mmc_retune_hold() and mmc_retune_and_hold()
>>>> but after one of the revisions I found that only one was needed. I stuck
>>>> with the mmc_retune_hold() name because it doesn't necessarily cause a
>>>> re-tune, but only if the hold count was zero and a retune is needed.
>>>>
>>>>>
>>>>> Earlier I only had the re-tune timer in mind, which is why I was less
>>>>> restrictive and suggesting you to add hold/disable. Sorry about that.
>>>>>
>>>>> Now, with the above in mind I believe you have similar issues with
>>>>> patch5 (mmc: core: Hold re-tuning during switch commands) and patch6
>>>>> (mmc: core: Hold re-tuning during erase commands). And that's because
>>>>> there are cases when the switch/erase commands are the first commands
>>>>> sent, after the sdhci host has been runtime PM suspended. I guess we
>>>>> need a way to make sure we don't hold re-tune for these cases.
>>>>>
>>>>> An option to deal with that is to use a separate flag set by host
>>>>> drivers, though the mmc_needs_retune() API and let that one override
>>>>> another.
>>>>>
>>>>> Forgive me for pushing you back and forth for how to do this, but it
>>>>
>>>> Not a problem. Thanks for persevering.
>>>>
>>>>> seems like we still have some outstanding issues to resolve.
>>>
>>> So that then more or less leaves us with one outstanding issue. The
>>> SDIO irq wakeup scenario.
>>>
>>> How will that work for sdhci?
>>>
>>> Your suggestion is to hold re-tune for the SDIO wakeup command. If I
>>> understand correct that could be overridden when the host flags
>>> need_retune from its runtime PM suspend callback, right?
>>>
>>> That then mean that the re-tuning will be done prior sending the
>>> wakeup command? That wouldn't work, unless the re-tune command also
>>> act as wakeup, which I doubt.
>>
>> The wakeup command has to come first.
>>
>>>
>>> If I _haven't_ understand correctly and you mean that the SDIO wakeup
>>> command shall be invoked prior re-tuning is done; that would mean that
>>> SDHCI will send a command to the card without first satisfying its
>>> need for a re-tune. And that wouldn't work either, right?
>>
>> My understanding is that the wakeup command will still work but there might
>> be a CRC error.
>>
>> Need Arend to comment on this since it is his driver we are talking about.
> 
> Are we?

At a guess, something that would have the effect of:

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
index ab0c898..4e5e97f 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
@@ -774,8 +774,12 @@ brcmf_sdio_kso_control(struct brcmf_sdio *bus, bool on)
 
        wr_val = (on << SBSDIO_FUNC1_SLEEPCSR_KSO_SHIFT);
        /* 1st KSO write goes to AOS wake up core if device is asleep  */
+       if (on)
+               bus->sdiodev->func[SDIO_FUNC_1]->card->host->hold_retune += 1;
        brcmf_sdiod_regwb(bus->sdiodev, SBSDIO_FUNC1_SLEEPCSR,
                          wr_val, &err);
+       if (on)
+               bus->sdiodev->func[SDIO_FUNC_1]->card->host->hold_retune -= 1;
 
        if (on) {
                /* device WAKEUP through KSO:


> 
>> So the plan would be:
>>     - re-tuning hold_count is incremented
>>     - wakeup command is issued (and no re-tuning is done)
>>     - errors are ignored
>>     - re-tuning hold_count is decremented
>>     - continue as normal, re-tuning before the next request as needed
>>
>>>
>>> So then the only solution for SDHCI would be to prevent it from being
>>> runtime PM suspended when configured for SDIO. Urgh, that's really
>>> bad.
>>
>> Yes that would defeat the point of sleeping.
> 
> I recently submitted a patch in brcmfmac to disable runtime pm for the SDIO
> host controller as it interfered with communication between driver and
> device, ie. driver send request to device but response is never received
> because runtime-pm kicked in. Our sdio func driver does not provide
> runtime-pm (yet) and figured using pm_runtime_forbid() was the only way to
> let the host controller know this fact.

We need to sort this out properly at some point.

Re: [PATCH V6 08/15] mmc: mmc: Hold re-tuning if the card is put to sleep

[Arend van Spriel]

On 04/22/15 09:24, Adrian Hunter wrote:
> On 21/04/15 21:25, Arend van Spriel wrote:
>> On 04/21/15 14:26, Adrian Hunter wrote:
>>> On 21/04/15 14:53, Ulf Hansson wrote:
>>>> On 21 April 2015 at 13:00, Adrian Hunter<adrian.hunter@intel.com>   wrote:
>>>>> On 21/04/15 12:42, Ulf Hansson wrote:
>>>>>> On 20 April 2015 at 14:09, Adrian Hunter<adrian.hunter@intel.com>   wrote:
>>>>>>> Currently "mmc sleep" is used before power off and
>>>>>>> is not paired with waking up. Nevertheless hold
>>>>>>> re-tuning.
>>>>>>>
>>>>>>> Signed-off-by: Adrian Hunter<adrian.hunter@intel.com>
>>>>>>> ---
>>>>>>>    drivers/mmc/core/mmc.c | 14 +++++++++++---
>>>>>>>    1 file changed, 11 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>>>>>> index f36c76f..daf9954 100644
>>>>>>> --- a/drivers/mmc/core/mmc.c
>>>>>>> +++ b/drivers/mmc/core/mmc.c
>>>>>>> @@ -21,6 +21,7 @@
>>>>>>>    #include<linux/mmc/mmc.h>
>>>>>>>
>>>>>>>    #include "core.h"
>>>>>>> +#include "host.h"
>>>>>>>    #include "bus.h"
>>>>>>>    #include "mmc_ops.h"
>>>>>>>    #include "sd_ops.h"
>>>>>>> @@ -1504,6 +1505,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};
>>>>>>> @@ -1631,6 +1633,7 @@ static int _mmc_suspend(struct mmc_host *host,
>>>>>>> bool is_suspend)
>>>>>>>           int err = 0;
>>>>>>>           unsigned int notify_type = is_suspend ?
>>>>>>> EXT_CSD_POWER_OFF_SHORT :
>>>>>>>                                           EXT_CSD_POWER_OFF_LONG;
>>>>>>> +       bool retune_release = false;
>>>>>>>
>>>>>>>           BUG_ON(!host);
>>>>>>>           BUG_ON(!host->card);
>>>>>>> @@ -1651,17 +1654,22 @@ static int _mmc_suspend(struct mmc_host *host,
>>>>>>> bool is_suspend)
>>>>>>>                   goto out;
>>>>>>>
>>>>>>>           if (mmc_can_poweroff_notify(host->card)&&
>>>>>>> -               ((host->caps2&   MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend))
>>>>>>> +               ((host->caps2&   MMC_CAP2_FULL_PWR_CYCLE) ||
>>>>>>> !is_suspend)) {
>>>>>>>                   err = mmc_poweroff_notify(host->card, notify_type);
>>>>>>> -       else if (mmc_can_sleep(host->card))
>>>>>>> +       } else if (mmc_can_sleep(host->card)) {
>>>>>>> +               mmc_retune_hold(host);
>>>>>>>                   err = mmc_sleep(host);
>>>>>>> -       else if (!mmc_host_is_spi(host))
>>>>>>> +       } else if (!mmc_host_is_spi(host)) {
>>>>>>>                   err = mmc_deselect_cards(host);
>>>>>>> +       }
>>>>>>>
>>>>>>>           if (!err) {
>>>>>>>                   mmc_power_off(host);
>>>>>>>                   mmc_card_set_suspended(host->card);
>>>>>>>           }
>>>>>>> +
>>>>>>> +       if (retune_release)
>>>>>>> +               mmc_retune_release(host);
>>>>>>>    out:
>>>>>>>           mmc_release_host(host);
>>>>>>>           return err;
>>>>>>> --
>>>>>>> 1.9.1
>>>>>>>
>>>>>>
>>>>>> According to our previous discussions I have given this some more
>>>>>> thinking.
>>>>>>
>>>>>> I don't think we can allow to hold/disable re-tune in this path at
>>>>>> all. That's because we are claiming the host here and the sleep
>>>>>> command might then be the first command we invoke during the system PM
>>>>>> sequence.
>>>>>>
>>>>>> That means sdhci might have flagged need_retune, since it's been
>>>>>> runtime PM suspended. And for those scenarios I guess we really need
>>>>>> to do a re-tune prior sending the sleep command, right?
>>>>>
>>>>> Yes, although that is how it works.
>>>>
>>>> Ohh, you are one step ahead of me. Good! :-)
>>>>
>>>>>
>>>>> Previously I had two functions mmc_retune_hold() and mmc_retune_and_hold()
>>>>> but after one of the revisions I found that only one was needed. I stuck
>>>>> with the mmc_retune_hold() name because it doesn't necessarily cause a
>>>>> re-tune, but only if the hold count was zero and a retune is needed.
>>>>>
>>>>>>
>>>>>> Earlier I only had the re-tune timer in mind, which is why I was less
>>>>>> restrictive and suggesting you to add hold/disable. Sorry about that.
>>>>>>
>>>>>> Now, with the above in mind I believe you have similar issues with
>>>>>> patch5 (mmc: core: Hold re-tuning during switch commands) and patch6
>>>>>> (mmc: core: Hold re-tuning during erase commands). And that's because
>>>>>> there are cases when the switch/erase commands are the first commands
>>>>>> sent, after the sdhci host has been runtime PM suspended. I guess we
>>>>>> need a way to make sure we don't hold re-tune for these cases.
>>>>>>
>>>>>> An option to deal with that is to use a separate flag set by host
>>>>>> drivers, though the mmc_needs_retune() API and let that one override
>>>>>> another.
>>>>>>
>>>>>> Forgive me for pushing you back and forth for how to do this, but it
>>>>>
>>>>> Not a problem. Thanks for persevering.
>>>>>
>>>>>> seems like we still have some outstanding issues to resolve.
>>>>
>>>> So that then more or less leaves us with one outstanding issue. The
>>>> SDIO irq wakeup scenario.
>>>>
>>>> How will that work for sdhci?
>>>>
>>>> Your suggestion is to hold re-tune for the SDIO wakeup command. If I
>>>> understand correct that could be overridden when the host flags
>>>> need_retune from its runtime PM suspend callback, right?
>>>>
>>>> That then mean that the re-tuning will be done prior sending the
>>>> wakeup command? That wouldn't work, unless the re-tune command also
>>>> act as wakeup, which I doubt.
>>>
>>> The wakeup command has to come first.
>>>
>>>>
>>>> If I _haven't_ understand correctly and you mean that the SDIO wakeup
>>>> command shall be invoked prior re-tuning is done; that would mean that
>>>> SDHCI will send a command to the card without first satisfying its
>>>> need for a re-tune. And that wouldn't work either, right?
>>>
>>> My understanding is that the wakeup command will still work but there might
>>> be a CRC error.
>>>
>>> Need Arend to comment on this since it is his driver we are talking about.
>>
>> Are we?
>
> At a guess, something that would have the effect of:
>
> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
> index ab0c898..4e5e97f 100644
> --- a/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
> +++ b/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
> @@ -774,8 +774,12 @@ brcmf_sdio_kso_control(struct brcmf_sdio *bus, bool on)
>
>          wr_val = (on<<  SBSDIO_FUNC1_SLEEPCSR_KSO_SHIFT);
>          /* 1st KSO write goes to AOS wake up core if device is asleep  */
> +       if (on)
> +               bus->sdiodev->func[SDIO_FUNC_1]->card->host->hold_retune += 1;
>          brcmf_sdiod_regwb(bus->sdiodev, SBSDIO_FUNC1_SLEEPCSR,
>                            wr_val,&err);
> +       if (on)
> +               bus->sdiodev->func[SDIO_FUNC_1]->card->host->hold_retune -= 1;

I see. I was indeed wondering about the wakeup as the retune would fail 
because the device will not respond to retune command. This function 
currently is triggered by idle detection in the driver instead of using 
runtime-pm. If we would add runtime-pm support and do pm_runtime_get() 
on the function dev, would that also keep the host controller from 
entering runtime-pm?

>          if (on) {
>                  /* device WAKEUP through KSO:
>
>
>>
>>> So the plan would be:
>>>      - re-tuning hold_count is incremented
>>>      - wakeup command is issued (and no re-tuning is done)
>>>      - errors are ignored
>>>      - re-tuning hold_count is decremented
>>>      - continue as normal, re-tuning before the next request as needed
>>>
>>>>
>>>> So then the only solution for SDHCI would be to prevent it from being
>>>> runtime PM suspended when configured for SDIO. Urgh, that's really
>>>> bad.
>>>
>>> Yes that would defeat the point of sleeping.
>>
>> I recently submitted a patch in brcmfmac to disable runtime pm for the SDIO
>> host controller as it interfered with communication between driver and
>> device, ie. driver send request to device but response is never received
>> because runtime-pm kicked in. Our sdio func driver does not provide
>> runtime-pm (yet) and figured using pm_runtime_forbid() was the only way to
>> let the host controller know this fact.
>
> We need to sort this out properly at some point.

Agree. One thing that makes me raise my eyebrows is piece of code in 
mmc_attach_sdio():

	/*
        	 * Enable runtime PM only if supported by host+card+board
        	 */
         if (host->caps & MMC_CAP_POWER_OFF_CARD) {
        		/*
        		 * Let runtime PM core know our card is active
        		 */
        		err = pm_runtime_set_active(&card->dev);
        		if (err)
        			goto remove;

		/*
        		 * Enable runtime PM for this card
        		 */
        		pm_runtime_enable(&card->dev);
         }

The comment above the if statement is what I would expect to be correct 
behavior, but it only checks the host->caps.

Regards,
Arend

Re: [PATCH V6 08/15] mmc: mmc: Hold re-tuning if the card is put to sleep

[Ulf Hansson]

On 22 April 2015 at 10:30, Arend van Spriel <arend@broadcom.com> wrote:
> On 04/22/15 09:24, Adrian Hunter wrote:
>>
>> On 21/04/15 21:25, Arend van Spriel wrote:
>>>
>>> On 04/21/15 14:26, Adrian Hunter wrote:
>>>>
>>>> On 21/04/15 14:53, Ulf Hansson wrote:
>>>>>
>>>>> On 21 April 2015 at 13:00, Adrian Hunter<adrian.hunter@intel.com>
>>>>> wrote:
>>>>>>
>>>>>> On 21/04/15 12:42, Ulf Hansson wrote:
>>>>>>>
>>>>>>> On 20 April 2015 at 14:09, Adrian Hunter<adrian.hunter@intel.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>> Currently "mmc sleep" is used before power off and
>>>>>>>> is not paired with waking up. Nevertheless hold
>>>>>>>> re-tuning.
>>>>>>>>
>>>>>>>> Signed-off-by: Adrian Hunter<adrian.hunter@intel.com>
>>>>>>>> ---
>>>>>>>>    drivers/mmc/core/mmc.c | 14 +++++++++++---
>>>>>>>>    1 file changed, 11 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>>>>>>> index f36c76f..daf9954 100644
>>>>>>>> --- a/drivers/mmc/core/mmc.c
>>>>>>>> +++ b/drivers/mmc/core/mmc.c
>>>>>>>> @@ -21,6 +21,7 @@
>>>>>>>>    #include<linux/mmc/mmc.h>
>>>>>>>>
>>>>>>>>    #include "core.h"
>>>>>>>> +#include "host.h"
>>>>>>>>    #include "bus.h"
>>>>>>>>    #include "mmc_ops.h"
>>>>>>>>    #include "sd_ops.h"
>>>>>>>> @@ -1504,6 +1505,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};
>>>>>>>> @@ -1631,6 +1633,7 @@ static int _mmc_suspend(struct mmc_host *host,
>>>>>>>> bool is_suspend)
>>>>>>>>           int err = 0;
>>>>>>>>           unsigned int notify_type = is_suspend ?
>>>>>>>> EXT_CSD_POWER_OFF_SHORT :
>>>>>>>>                                           EXT_CSD_POWER_OFF_LONG;
>>>>>>>> +       bool retune_release = false;
>>>>>>>>
>>>>>>>>           BUG_ON(!host);
>>>>>>>>           BUG_ON(!host->card);
>>>>>>>> @@ -1651,17 +1654,22 @@ static int _mmc_suspend(struct mmc_host
>>>>>>>> *host,
>>>>>>>> bool is_suspend)
>>>>>>>>                   goto out;
>>>>>>>>
>>>>>>>>           if (mmc_can_poweroff_notify(host->card)&&
>>>>>>>> -               ((host->caps2&   MMC_CAP2_FULL_PWR_CYCLE) ||
>>>>>>>> !is_suspend))
>>>>>>>> +               ((host->caps2&   MMC_CAP2_FULL_PWR_CYCLE) ||
>>>>>>>> !is_suspend)) {
>>>>>>>>                   err = mmc_poweroff_notify(host->card,
>>>>>>>> notify_type);
>>>>>>>> -       else if (mmc_can_sleep(host->card))
>>>>>>>> +       } else if (mmc_can_sleep(host->card)) {
>>>>>>>> +               mmc_retune_hold(host);
>>>>>>>>                   err = mmc_sleep(host);
>>>>>>>> -       else if (!mmc_host_is_spi(host))
>>>>>>>> +       } else if (!mmc_host_is_spi(host)) {
>>>>>>>>                   err = mmc_deselect_cards(host);
>>>>>>>> +       }
>>>>>>>>
>>>>>>>>           if (!err) {
>>>>>>>>                   mmc_power_off(host);
>>>>>>>>                   mmc_card_set_suspended(host->card);
>>>>>>>>           }
>>>>>>>> +
>>>>>>>> +       if (retune_release)
>>>>>>>> +               mmc_retune_release(host);
>>>>>>>>    out:
>>>>>>>>           mmc_release_host(host);
>>>>>>>>           return err;
>>>>>>>> --
>>>>>>>> 1.9.1
>>>>>>>>
>>>>>>>
>>>>>>> According to our previous discussions I have given this some more
>>>>>>> thinking.
>>>>>>>
>>>>>>> I don't think we can allow to hold/disable re-tune in this path at
>>>>>>> all. That's because we are claiming the host here and the sleep
>>>>>>> command might then be the first command we invoke during the system
>>>>>>> PM
>>>>>>> sequence.
>>>>>>>
>>>>>>> That means sdhci might have flagged need_retune, since it's been
>>>>>>> runtime PM suspended. And for those scenarios I guess we really need
>>>>>>> to do a re-tune prior sending the sleep command, right?
>>>>>>
>>>>>>
>>>>>> Yes, although that is how it works.
>>>>>
>>>>>
>>>>> Ohh, you are one step ahead of me. Good! :-)
>>>>>
>>>>>>
>>>>>> Previously I had two functions mmc_retune_hold() and
>>>>>> mmc_retune_and_hold()
>>>>>> but after one of the revisions I found that only one was needed. I
>>>>>> stuck
>>>>>> with the mmc_retune_hold() name because it doesn't necessarily cause a
>>>>>> re-tune, but only if the hold count was zero and a retune is needed.
>>>>>>
>>>>>>>
>>>>>>> Earlier I only had the re-tune timer in mind, which is why I was less
>>>>>>> restrictive and suggesting you to add hold/disable. Sorry about that.
>>>>>>>
>>>>>>> Now, with the above in mind I believe you have similar issues with
>>>>>>> patch5 (mmc: core: Hold re-tuning during switch commands) and patch6
>>>>>>> (mmc: core: Hold re-tuning during erase commands). And that's because
>>>>>>> there are cases when the switch/erase commands are the first commands
>>>>>>> sent, after the sdhci host has been runtime PM suspended. I guess we
>>>>>>> need a way to make sure we don't hold re-tune for these cases.
>>>>>>>
>>>>>>> An option to deal with that is to use a separate flag set by host
>>>>>>> drivers, though the mmc_needs_retune() API and let that one override
>>>>>>> another.
>>>>>>>
>>>>>>> Forgive me for pushing you back and forth for how to do this, but it
>>>>>>
>>>>>>
>>>>>> Not a problem. Thanks for persevering.
>>>>>>
>>>>>>> seems like we still have some outstanding issues to resolve.
>>>>>
>>>>>
>>>>> So that then more or less leaves us with one outstanding issue. The
>>>>> SDIO irq wakeup scenario.
>>>>>
>>>>> How will that work for sdhci?
>>>>>
>>>>> Your suggestion is to hold re-tune for the SDIO wakeup command. If I
>>>>> understand correct that could be overridden when the host flags
>>>>> need_retune from its runtime PM suspend callback, right?
>>>>>
>>>>> That then mean that the re-tuning will be done prior sending the
>>>>> wakeup command? That wouldn't work, unless the re-tune command also
>>>>> act as wakeup, which I doubt.
>>>>
>>>>
>>>> The wakeup command has to come first.
>>>>
>>>>>
>>>>> If I _haven't_ understand correctly and you mean that the SDIO wakeup
>>>>> command shall be invoked prior re-tuning is done; that would mean that
>>>>> SDHCI will send a command to the card without first satisfying its
>>>>> need for a re-tune. And that wouldn't work either, right?
>>>>
>>>>
>>>> My understanding is that the wakeup command will still work but there
>>>> might
>>>> be a CRC error.
>>>>
>>>> Need Arend to comment on this since it is his driver we are talking
>>>> about.
>>>
>>>
>>> Are we?
>>
>>
>> At a guess, something that would have the effect of:
>>
>> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
>> b/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
>> index ab0c898..4e5e97f 100644
>> --- a/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
>> +++ b/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
>> @@ -774,8 +774,12 @@ brcmf_sdio_kso_control(struct brcmf_sdio *bus, bool
>> on)
>>
>>          wr_val = (on<<  SBSDIO_FUNC1_SLEEPCSR_KSO_SHIFT);
>>          /* 1st KSO write goes to AOS wake up core if device is asleep  */
>> +       if (on)
>> +               bus->sdiodev->func[SDIO_FUNC_1]->card->host->hold_retune
>> += 1;
>>          brcmf_sdiod_regwb(bus->sdiodev, SBSDIO_FUNC1_SLEEPCSR,
>>                            wr_val,&err);
>> +       if (on)
>> +               bus->sdiodev->func[SDIO_FUNC_1]->card->host->hold_retune
>> -= 1;
>
>
> I see. I was indeed wondering about the wakeup as the retune would fail
> because the device will not respond to retune command. This function
> currently is triggered by idle detection in the driver instead of using
> runtime-pm. If we would add runtime-pm support and do pm_runtime_get() on
> the function dev, would that also keep the host controller from entering
> runtime-pm?

Nope!

>
>>          if (on) {
>>                  /* device WAKEUP through KSO:
>>
>>
>>>
>>>> So the plan would be:
>>>>      - re-tuning hold_count is incremented
>>>>      - wakeup command is issued (and no re-tuning is done)
>>>>      - errors are ignored
>>>>      - re-tuning hold_count is decremented
>>>>      - continue as normal, re-tuning before the next request as needed
>>>>
>>>>>
>>>>> So then the only solution for SDHCI would be to prevent it from being
>>>>> runtime PM suspended when configured for SDIO. Urgh, that's really
>>>>> bad.
>>>>
>>>>
>>>> Yes that would defeat the point of sleeping.
>>>
>>>
>>> I recently submitted a patch in brcmfmac to disable runtime pm for the
>>> SDIO
>>> host controller as it interfered with communication between driver and
>>> device, ie. driver send request to device but response is never received
>>> because runtime-pm kicked in. Our sdio func driver does not provide
>>> runtime-pm (yet) and figured using pm_runtime_forbid() was the only way
>>> to
>>> let the host controller know this fact.
>>
>>
>> We need to sort this out properly at some point.
>
>
> Agree. One thing that makes me raise my eyebrows is piece of code in
> mmc_attach_sdio():
>
>         /*
>          * Enable runtime PM only if supported by host+card+board
>          */
>         if (host->caps & MMC_CAP_POWER_OFF_CARD) {
>                 /*
>                  * Let runtime PM core know our card is active
>                  */
>                 err = pm_runtime_set_active(&card->dev);
>                 if (err)
>                         goto remove;
>
>                 /*
>                  * Enable runtime PM for this card
>                  */
>                 pm_runtime_enable(&card->dev);
>         }
>
> The comment above the if statement is what I would expect to be correct
> behavior, but it only checks the host->caps.

_I_ think the entire runtime PM implementation related to the
MMC_CAP_POWER_OFF_CARD is a bit fragile to use. So I won't be
surprised if SDIO func clients will have issues to use it.

Currently, I would instead advise them to use
mmc_power_save|restore_host() API, to accomplish the similar
operations. I do realize that using that API does also have some
limitations, for example when using SDIO combo cards and there are
some corner cases around system PM.

We need to go over this piece of code in the mmc core to make it more
robust. We also need to figure out which APIs SDIO func drivers shall
use to deal with PM.

Kind regards
Uffe

[PATCH V7 02/15] mmc: core: Enable / disable re-tuning

[Adrian Hunter]

Enable re-tuning when tuning is executed and
disable re-tuning when card is no longer initialized.

In the case of SDIO suspend, the card can keep power.
In that case, re-tuning need not be disabled, but, if
a re-tuning timer is being used, ensure it is disabled
and assume that re-tuning will be needed upon resume
since it is not known how long the suspend will last.

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


Changes in V7:


	Also flag re-tune needed in SDIO 'keep_power'
	case, when a re-tuning timer is being used.


 drivers/mmc/core/core.c | 4 ++++
 drivers/mmc/core/sdio.c | 6 +++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 92e7671..007c444 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1109,6 +1109,8 @@ int mmc_execute_tuning(struct mmc_card *card)
 
 	if (err)
 		pr_err("%s: tuning execution failed\n", mmc_hostname(host));
+	else
+		mmc_retune_enable(host);
 
 	return err;
 }
@@ -1140,6 +1142,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
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 5bc6c7d..16e1f39 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -934,8 +934,12 @@ static int mmc_sdio_suspend(struct mmc_host *host)
 		mmc_release_host(host);
 	}
 
-	if (!mmc_card_keep_power(host))
+	if (!mmc_card_keep_power(host)) {
 		mmc_power_off(host);
+	} else if (host->retune_period) {
+		mmc_retune_timer_stop(host);
+		mmc_retune_needed(host);
+	}
 
 	return 0;
 }
-- 
1.9.1

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

[Adrian Hunter]

On 20/04/15 15:09, Adrian Hunter wrote:
> Hi
> 
> Here is V6 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.
> 

Hi

I sent a V7 of patch 2, and a couple of patches that fix the
brcm custom sleep re-tuning issue. So is there anything else
that needs to be done?

Regards
Adrian

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

[Ulf Hansson]

On 4 May 2015 at 12:39, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 20/04/15 15:09, Adrian Hunter wrote:
>> Hi
>>
>> Here is V6 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.
>>
>
> Hi
>
> I sent a V7 of patch 2, and a couple of patches that fix the
> brcm custom sleep re-tuning issue. So is there anything else
> that needs to be done?

I am looking through v6/7, I found that there are a few things that I
think you need to address. I will comment on them separately per
patch.

Kind regards
Uffe

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

[Ulf Hansson]

On 20 April 2015 at 14:09, 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:
>   - 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 7 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 */
>   unsigned int          retune_period;  /* re-tuning period in secs */
>   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  | 68 ++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/mmc/core/host.h  |  6 +++++
>  include/linux/mmc/host.h | 28 ++++++++++++++++++++
>  3 files changed, 102 insertions(+)
>
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 8be0df7..e90e02f 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -301,6 +301,73 @@ static inline void mmc_host_clk_sysfs_init(struct mmc_host *host)
>
>  #endif
>
> +void mmc_retune_enable(struct mmc_host *host)
> +{
> +       host->can_retune = 1;
> +       if (host->retune_period)
> +               mod_timer(&host->retune_timer,
> +                         jiffies + host->retune_period * HZ);
> +}
> +
> +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;
> +}
> +
> +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;
> +}
> +
> +void mmc_retune_release(struct mmc_host *host)
> +{
> +       if (host->hold_retune)
> +               host->hold_retune -= 1;
> +       else
> +               WARN_ON(1);
> +}
> +
> +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;
> +}
> +
> +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 +571,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/drivers/mmc/core/host.h b/drivers/mmc/core/host.h
> index f2ab9e5..992bf53 100644
> --- a/drivers/mmc/core/host.h
> +++ b/drivers/mmc/core/host.h
> @@ -15,5 +15,11 @@
>  int mmc_register_host_class(void);
>  void mmc_unregister_host_class(void);
>
> +void mmc_retune_enable(struct mmc_host *host);
> +void mmc_retune_disable(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);
> +
>  #endif
>
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index b5bedae..9c9e51b 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,18 @@ 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 */
> +       unsigned int            retune_period;  /* re-tuning period in secs */
> +       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 +522,23 @@ static inline bool mmc_card_hs400(struct mmc_card *card)
>         return card->host->ios.timing == MMC_TIMING_MMC_HS400;
>  }
>
> +void mmc_retune_timer_stop(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;
> +}

This function isn't required and has no users. Let's remove it.

> +
> +static inline void mmc_retune_recheck(struct mmc_host *host)
> +{
> +       if (host->hold_retune <= 1)
> +               host->retune_now = 1;
> +}
> +
>  #endif /* LINUX_MMC_HOST_H */

Kind regards
Uffe

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

[Ulf Hansson]

On 20 April 2015 at 14:09, 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.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>

Patch2 is calling mmc_retune_needed() and thus actually triggers a
re-tune to potentially happen.

To avoid bisectability issues about not holding re-tune when needed, I
suggest we move $subject patch to after patch8.

Kind regards
Uffe

> ---
>  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 dd43f9b..a9936cb 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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Ulf Hansson]

On 20 April 2015 at 14:09, Adrian Hunter <adrian.hunter@intel.com> wrote:
> 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>

This patch should probably be folded into patch3.

Kind regards
Uffe


> ---
>  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 a9936cb..08bf7e3 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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V6 08/15] mmc: mmc: Hold re-tuning if the card is put to sleep

[Ulf Hansson]

On 20 April 2015 at 14:09, Adrian Hunter <adrian.hunter@intel.com> wrote:
> Currently "mmc sleep" is used before power off and
> is not paired with waking up. Nevertheless hold
> re-tuning.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  drivers/mmc/core/mmc.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index f36c76f..daf9954 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -21,6 +21,7 @@
>  #include <linux/mmc/mmc.h>
>
>  #include "core.h"
> +#include "host.h"
>  #include "bus.h"
>  #include "mmc_ops.h"
>  #include "sd_ops.h"
> @@ -1504,6 +1505,7 @@ static int mmc_can_sleep(struct mmc_card *card)
>         return (card && card->ext_csd.rev >= 3);
>  }
>
> +/* If necessary, callers must hold re-tuning */

Remove this comment.

>  static int mmc_sleep(struct mmc_host *host)
>  {
>         struct mmc_command cmd = {0};
> @@ -1631,6 +1633,7 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>         int err = 0;
>         unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT :
>                                         EXT_CSD_POWER_OFF_LONG;
> +       bool retune_release = false;
>
>         BUG_ON(!host);
>         BUG_ON(!host->card);
> @@ -1651,17 +1654,22 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>                 goto out;
>
>         if (mmc_can_poweroff_notify(host->card) &&
> -               ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend))
> +               ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) {
>                 err = mmc_poweroff_notify(host->card, notify_type);
> -       else if (mmc_can_sleep(host->card))
> +       } else if (mmc_can_sleep(host->card)) {
> +               mmc_retune_hold(host);
>                 err = mmc_sleep(host);
> -       else if (!mmc_host_is_spi(host))
> +       } else if (!mmc_host_is_spi(host)) {
>                 err = mmc_deselect_cards(host);
> +       }
>
>         if (!err) {
>                 mmc_power_off(host);
>                 mmc_card_set_suspended(host->card);
>         }
> +
> +       if (retune_release)
> +               mmc_retune_release(host);
>  out:
>         mmc_release_host(host);
>         return err;

Instead of add mmc_retune_hold|release() to _mmc_suspend(), I would
like you to move that handling into mmc_sleep(). The code should be
easier and it becomes more clear that it's because of a command
sequence.

I think mmc_retune_hold() should be invoked before mmc_wait_for_cmd()
and then mmc_retune_release() just after, in mmc_sleep(). That should
work, right!?

Kind regards
Uffe

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

[Ulf Hansson]

On 20 April 2015 at 14:09, Adrian Hunter <adrian.hunter@intel.com> wrote:
> 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 b345844..d11fae7 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2307,8 +2307,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);

I would rather see this to be handled by the mmc core, to have all
hosts benefit from it.

> +       }
>
>         if (host->cmd->error) {
>                 tasklet_schedule(&host->finish_tasklet);
> @@ -2433,13 +2435,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
>

Kind regards
Uffe

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

[Adrian Hunter]

On 04/05/15 16:28, Ulf Hansson wrote:
> On 20 April 2015 at 14:09, 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.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> 
> Patch2 is calling mmc_retune_needed() and thus actually triggers a
> re-tune to potentially happen.

That won't happen because host->retune_period is not set, so
mmc_retune_needed() won't be called.

> 
> To avoid bisectability issues about not holding re-tune when needed, I
> suggest we move $subject patch to after patch8.
> 
> Kind regards
> Uffe
> 
>> ---
>>  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 dd43f9b..a9936cb 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
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH V6 08/15] mmc: mmc: Hold re-tuning if the card is put to sleep

[Adrian Hunter]

On 04/05/15 16:44, Ulf Hansson wrote:
> On 20 April 2015 at 14:09, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> Currently "mmc sleep" is used before power off and
>> is not paired with waking up. Nevertheless hold
>> re-tuning.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>  drivers/mmc/core/mmc.c | 14 +++++++++++---
>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index f36c76f..daf9954 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -21,6 +21,7 @@
>>  #include <linux/mmc/mmc.h>
>>
>>  #include "core.h"
>> +#include "host.h"
>>  #include "bus.h"
>>  #include "mmc_ops.h"
>>  #include "sd_ops.h"
>> @@ -1504,6 +1505,7 @@ static int mmc_can_sleep(struct mmc_card *card)
>>         return (card && card->ext_csd.rev >= 3);
>>  }
>>
>> +/* If necessary, callers must hold re-tuning */
> 
> Remove this comment.
> 
>>  static int mmc_sleep(struct mmc_host *host)
>>  {
>>         struct mmc_command cmd = {0};
>> @@ -1631,6 +1633,7 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>>         int err = 0;
>>         unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT :
>>                                         EXT_CSD_POWER_OFF_LONG;
>> +       bool retune_release = false;
>>
>>         BUG_ON(!host);
>>         BUG_ON(!host->card);
>> @@ -1651,17 +1654,22 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>>                 goto out;
>>
>>         if (mmc_can_poweroff_notify(host->card) &&
>> -               ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend))
>> +               ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) {
>>                 err = mmc_poweroff_notify(host->card, notify_type);
>> -       else if (mmc_can_sleep(host->card))
>> +       } else if (mmc_can_sleep(host->card)) {
>> +               mmc_retune_hold(host);
>>                 err = mmc_sleep(host);
>> -       else if (!mmc_host_is_spi(host))
>> +       } else if (!mmc_host_is_spi(host)) {
>>                 err = mmc_deselect_cards(host);
>> +       }
>>
>>         if (!err) {
>>                 mmc_power_off(host);
>>                 mmc_card_set_suspended(host->card);
>>         }
>> +
>> +       if (retune_release)
>> +               mmc_retune_release(host);
>>  out:
>>         mmc_release_host(host);
>>         return err;
> 
> Instead of add mmc_retune_hold|release() to _mmc_suspend(), I would
> like you to move that handling into mmc_sleep(). The code should be
> easier and it becomes more clear that it's because of a command
> sequence.
> 
> I think mmc_retune_hold() should be invoked before mmc_wait_for_cmd()
> and then mmc_retune_release() just after, in mmc_sleep(). That should
> work, right!?

That would be the same as holding re-tuning for that request, which is
what already happens i.e. adding hold()/release() around mmc_wait_for_cmd()
is redundant.

The options for the caller are:

1)
	hold re-tuning
	put emmc to sleep
	later wake up emmc
	release re-tuning

2)
	put emmc to sleep
	later increment hold_count
	wake up emmc ignoring CRC errors
	release re-tuning

But there is no wake-up function and the suspend path is using an unbalanced
mmc_sleep i.e. no corresponding wake up.

So that leaves what is happening now i.e. a comment plus explicit
hold()/release() in _mmc_suspend() so that future changes to _mmc_suspend()
know to take mmc_sleep re-tuning requirements into account.

Re: [PATCH V6 08/15] mmc: mmc: Hold re-tuning if the card is put to sleep

[Ulf Hansson]

On 6 May 2015 at 10:39, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 04/05/15 16:44, Ulf Hansson wrote:
>> On 20 April 2015 at 14:09, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> Currently "mmc sleep" is used before power off and
>>> is not paired with waking up. Nevertheless hold
>>> re-tuning.
>>>
>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>> ---
>>>  drivers/mmc/core/mmc.c | 14 +++++++++++---
>>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>> index f36c76f..daf9954 100644
>>> --- a/drivers/mmc/core/mmc.c
>>> +++ b/drivers/mmc/core/mmc.c
>>> @@ -21,6 +21,7 @@
>>>  #include <linux/mmc/mmc.h>
>>>
>>>  #include "core.h"
>>> +#include "host.h"
>>>  #include "bus.h"
>>>  #include "mmc_ops.h"
>>>  #include "sd_ops.h"
>>> @@ -1504,6 +1505,7 @@ static int mmc_can_sleep(struct mmc_card *card)
>>>         return (card && card->ext_csd.rev >= 3);
>>>  }
>>>
>>> +/* If necessary, callers must hold re-tuning */
>>
>> Remove this comment.
>>
>>>  static int mmc_sleep(struct mmc_host *host)
>>>  {
>>>         struct mmc_command cmd = {0};
>>> @@ -1631,6 +1633,7 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>>>         int err = 0;
>>>         unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT :
>>>                                         EXT_CSD_POWER_OFF_LONG;
>>> +       bool retune_release = false;
>>>
>>>         BUG_ON(!host);
>>>         BUG_ON(!host->card);
>>> @@ -1651,17 +1654,22 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>>>                 goto out;
>>>
>>>         if (mmc_can_poweroff_notify(host->card) &&
>>> -               ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend))
>>> +               ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) {
>>>                 err = mmc_poweroff_notify(host->card, notify_type);
>>> -       else if (mmc_can_sleep(host->card))
>>> +       } else if (mmc_can_sleep(host->card)) {
>>> +               mmc_retune_hold(host);
>>>                 err = mmc_sleep(host);
>>> -       else if (!mmc_host_is_spi(host))
>>> +       } else if (!mmc_host_is_spi(host)) {
>>>                 err = mmc_deselect_cards(host);
>>> +       }
>>>
>>>         if (!err) {
>>>                 mmc_power_off(host);
>>>                 mmc_card_set_suspended(host->card);
>>>         }
>>> +
>>> +       if (retune_release)
>>> +               mmc_retune_release(host);
>>>  out:
>>>         mmc_release_host(host);
>>>         return err;
>>
>> Instead of add mmc_retune_hold|release() to _mmc_suspend(), I would
>> like you to move that handling into mmc_sleep(). The code should be
>> easier and it becomes more clear that it's because of a command
>> sequence.
>>
>> I think mmc_retune_hold() should be invoked before mmc_wait_for_cmd()
>> and then mmc_retune_release() just after, in mmc_sleep(). That should
>> work, right!?
>
> That would be the same as holding re-tuning for that request, which is
> what already happens i.e. adding hold()/release() around mmc_wait_for_cmd()
> is redundant.

I don't understand your point, sorry.

Anyway, my proposal didn't quite work, which is due to that
mmc_deselect_cards() (invoked from mmc_sleep()) deals with retries. If
there had been only one try, I thought it could be okay to have that
command to be preceded by a re-tune.

Anyway, I would like you to move the mmc_retune_hold|release() calls
into the mmc_sleep() function.

>
> The options for the caller are:
>
> 1)
>         hold re-tuning
>         put emmc to sleep
>         later wake up emmc
>         release re-tuning
>
> 2)
>         put emmc to sleep
>         later increment hold_count
>         wake up emmc ignoring CRC errors
>         release re-tuning
>
> But there is no wake-up function and the suspend path is using an unbalanced
> mmc_sleep i.e. no corresponding wake up.
>
> So that leaves what is happening now i.e. a comment plus explicit
> hold()/release() in _mmc_suspend() so that future changes to _mmc_suspend()
> know to take mmc_sleep re-tuning requirements into account.

Why all this complexity?

mmc_power_off() is called in _mmc_suspend(), that will eventually
disable re-tune. Thus re-tuning will be prevented for
commands/requests during the system PM resume sequence, until the card
has been fully re-initialized (and a tuning sequence done). Isn't that
sufficient?

Kind regards
Uffe

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

[Ulf Hansson]

On 6 May 2015 at 10:02, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 04/05/15 16:28, Ulf Hansson wrote:
>> On 20 April 2015 at 14:09, 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.
>>>
>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>
>> Patch2 is calling mmc_retune_needed() and thus actually triggers a
>> re-tune to potentially happen.
>
> That won't happen because host->retune_period is not set, so
> mmc_retune_needed() won't be called.

mmc_retune_needed() is indeed called in patch2 (v7). From
mmc_sdio_suspend() and when mmc_card_keep_power().

I guess that wont be an issue!?

But I just felt that it seemed more appropriate to manage hold/release
of re-tune before actually enabling the feature.

Kind regards
Uffe

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

[Adrian Hunter]

On 06/05/15 12:45, Ulf Hansson wrote:
> On 6 May 2015 at 10:02, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 04/05/15 16:28, Ulf Hansson wrote:
>>> On 20 April 2015 at 14:09, 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.
>>>>
>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>>
>>> Patch2 is calling mmc_retune_needed() and thus actually triggers a
>>> re-tune to potentially happen.
>>
>> That won't happen because host->retune_period is not set, so
>> mmc_retune_needed() won't be called.
> 
> mmc_retune_needed() is indeed called in patch2 (v7). From
> mmc_sdio_suspend() and when mmc_card_keep_power().

Yes, here is the chunk but it checks host->retune_period which  is zero
because it never gets set.

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 5bc6c7d..16e1f39 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -934,8 +934,12 @@ static int mmc_sdio_suspend(struct mmc_host *host)
 		mmc_release_host(host);
 	}

-	if (!mmc_card_keep_power(host))
+	if (!mmc_card_keep_power(host)) {
 		mmc_power_off(host);
+	} else if (host->retune_period) {
+		mmc_retune_timer_stop(host);
+		mmc_retune_needed(host);
+	}

 	return 0;
 }

> 
> I guess that wont be an issue!?

So I don't see an issue.

> 
> But I just felt that it seemed more appropriate to manage hold/release
> of re-tune before actually enabling the feature.

Yes the feature is gated until setting host->retune_period or calling
mmc_retune_needed().

Re: [PATCH V6 08/15] mmc: mmc: Hold re-tuning if the card is put to sleep

[Adrian Hunter]

On 06/05/15 12:32, Ulf Hansson wrote:
> On 6 May 2015 at 10:39, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 04/05/15 16:44, Ulf Hansson wrote:
>>> On 20 April 2015 at 14:09, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> Currently "mmc sleep" is used before power off and
>>>> is not paired with waking up. Nevertheless hold
>>>> re-tuning.
>>>>
>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>>> ---
>>>>  drivers/mmc/core/mmc.c | 14 +++++++++++---
>>>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>>> index f36c76f..daf9954 100644
>>>> --- a/drivers/mmc/core/mmc.c
>>>> +++ b/drivers/mmc/core/mmc.c
>>>> @@ -21,6 +21,7 @@
>>>>  #include <linux/mmc/mmc.h>
>>>>
>>>>  #include "core.h"
>>>> +#include "host.h"
>>>>  #include "bus.h"
>>>>  #include "mmc_ops.h"
>>>>  #include "sd_ops.h"
>>>> @@ -1504,6 +1505,7 @@ static int mmc_can_sleep(struct mmc_card *card)
>>>>         return (card && card->ext_csd.rev >= 3);
>>>>  }
>>>>
>>>> +/* If necessary, callers must hold re-tuning */
>>>
>>> Remove this comment.
>>>
>>>>  static int mmc_sleep(struct mmc_host *host)
>>>>  {
>>>>         struct mmc_command cmd = {0};
>>>> @@ -1631,6 +1633,7 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>>>>         int err = 0;
>>>>         unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT :
>>>>                                         EXT_CSD_POWER_OFF_LONG;
>>>> +       bool retune_release = false;
>>>>
>>>>         BUG_ON(!host);
>>>>         BUG_ON(!host->card);
>>>> @@ -1651,17 +1654,22 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>>>>                 goto out;
>>>>
>>>>         if (mmc_can_poweroff_notify(host->card) &&
>>>> -               ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend))
>>>> +               ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) {
>>>>                 err = mmc_poweroff_notify(host->card, notify_type);
>>>> -       else if (mmc_can_sleep(host->card))
>>>> +       } else if (mmc_can_sleep(host->card)) {
>>>> +               mmc_retune_hold(host);
>>>>                 err = mmc_sleep(host);
>>>> -       else if (!mmc_host_is_spi(host))
>>>> +       } else if (!mmc_host_is_spi(host)) {
>>>>                 err = mmc_deselect_cards(host);
>>>> +       }
>>>>
>>>>         if (!err) {
>>>>                 mmc_power_off(host);
>>>>                 mmc_card_set_suspended(host->card);
>>>>         }
>>>> +
>>>> +       if (retune_release)
>>>> +               mmc_retune_release(host);
>>>>  out:
>>>>         mmc_release_host(host);
>>>>         return err;
>>>
>>> Instead of add mmc_retune_hold|release() to _mmc_suspend(), I would
>>> like you to move that handling into mmc_sleep(). The code should be
>>> easier and it becomes more clear that it's because of a command
>>> sequence.
>>>
>>> I think mmc_retune_hold() should be invoked before mmc_wait_for_cmd()
>>> and then mmc_retune_release() just after, in mmc_sleep(). That should
>>> work, right!?
>>
>> That would be the same as holding re-tuning for that request, which is
>> what already happens i.e. adding hold()/release() around mmc_wait_for_cmd()
>> is redundant.
> 
> I don't understand your point, sorry.

mmc_wait_for_cmd() calls mmc_wait_for_req() which calls __mmc_start_req()
which calls mmc_start_request() which calls mmc_retune_hold()

Then mmc_wait_for_req() calls mmc_wait_for_req_done() which calls
mmc_retune_release().

So
	mmc_wait_for_cmd() (with no retries)
has the same effect as
	mmc_retune_hold()
	mmc_wait_for_cmd()
	mmc_retune_release()

> 
> Anyway, my proposal didn't quite work, which is due to that
> mmc_deselect_cards() (invoked from mmc_sleep()) deals with retries. If
> there had been only one try, I thought it could be okay to have that
> command to be preceded by a re-tune.
> 
> Anyway, I would like you to move the mmc_retune_hold|release() calls
> into the mmc_sleep() function.

That would have no effect as explained above.

> 
>>
>> The options for the caller are:
>>
>> 1)
>>         hold re-tuning
>>         put emmc to sleep
>>         later wake up emmc
>>         release re-tuning
>>
>> 2)
>>         put emmc to sleep
>>         later increment hold_count
>>         wake up emmc ignoring CRC errors
>>         release re-tuning
>>
>> But there is no wake-up function and the suspend path is using an unbalanced
>> mmc_sleep i.e. no corresponding wake up.
>>
>> So that leaves what is happening now i.e. a comment plus explicit
>> hold()/release() in _mmc_suspend() so that future changes to _mmc_suspend()
>> know to take mmc_sleep re-tuning requirements into account.
> 
> Why all this complexity?
> 
> mmc_power_off() is called in _mmc_suspend(), that will eventually
> disable re-tune. Thus re-tuning will be prevented for
> commands/requests during the system PM resume sequence, until the card
> has been fully re-initialized (and a tuning sequence done). Isn't that
> sufficient?

Yes my original patch did not have any of that complexity. I added it in
response to our discussions.

As you wrote, _mmc_suspend() does not need to do anything with retuning
because mmc_sleep() is followed by mmc_power_off().

The original patch added a comment to mmc_sleep() and that was all. That
would still be the best approach.

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

[Ulf Hansson]

On 6 May 2015 at 12:17, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 06/05/15 12:45, Ulf Hansson wrote:
>> On 6 May 2015 at 10:02, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> On 04/05/15 16:28, Ulf Hansson wrote:
>>>> On 20 April 2015 at 14:09, 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.
>>>>>
>>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>>>
>>>> Patch2 is calling mmc_retune_needed() and thus actually triggers a
>>>> re-tune to potentially happen.
>>>
>>> That won't happen because host->retune_period is not set, so
>>> mmc_retune_needed() won't be called.
>>
>> mmc_retune_needed() is indeed called in patch2 (v7). From
>> mmc_sdio_suspend() and when mmc_card_keep_power().
>
> Yes, here is the chunk but it checks host->retune_period which  is zero
> because it never gets set.
>
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index 5bc6c7d..16e1f39 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -934,8 +934,12 @@ static int mmc_sdio_suspend(struct mmc_host *host)
>                 mmc_release_host(host);
>         }
>
> -       if (!mmc_card_keep_power(host))
> +       if (!mmc_card_keep_power(host)) {
>                 mmc_power_off(host);
> +       } else if (host->retune_period) {
> +               mmc_retune_timer_stop(host);
> +               mmc_retune_needed(host);
> +       }
>
>         return 0;
>  }
>
>>
>> I guess that wont be an issue!?
>
> So I don't see an issue.
>
>>
>> But I just felt that it seemed more appropriate to manage hold/release
>> of re-tune before actually enabling the feature.
>
> Yes the feature is gated until setting host->retune_period or calling
> mmc_retune_needed().
>

Okay, fair enough. Let's keep the order of patches as is.

Kind regards
Uffe

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

[Adrian Hunter]

On 04/05/15 16:55, Ulf Hansson wrote:
> On 20 April 2015 at 14:09, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> 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 b345844..d11fae7 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -2307,8 +2307,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);
> 
> I would rather see this to be handled by the mmc core, to have all
> hosts benefit from it.

Now that the core is enabling re-tuning, that will cause re-tuning to happen
on platforms where it has never been used before (or more specifically where
tuning has been used only during initialization)

I have no idea if that is the right thing for all host controllers and
platforms.

Are you sure you want to do that?

> 
>> +       }
>>
>>         if (host->cmd->error) {
>>                 tasklet_schedule(&host->finish_tasklet);
>> @@ -2433,13 +2435,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
>>
> 
> Kind regards
> Uffe
> 
> 

Re: [PATCH V6 08/15] mmc: mmc: Hold re-tuning if the card is put to sleep

[Ulf Hansson]

[...]

>>>> Instead of add mmc_retune_hold|release() to _mmc_suspend(), I would
>>>> like you to move that handling into mmc_sleep(). The code should be
>>>> easier and it becomes more clear that it's because of a command
>>>> sequence.
>>>>
>>>> I think mmc_retune_hold() should be invoked before mmc_wait_for_cmd()
>>>> and then mmc_retune_release() just after, in mmc_sleep(). That should
>>>> work, right!?
>>>
>>> That would be the same as holding re-tuning for that request, which is
>>> what already happens i.e. adding hold()/release() around mmc_wait_for_cmd()
>>> is redundant.
>>
>> I don't understand your point, sorry.
>
> mmc_wait_for_cmd() calls mmc_wait_for_req() which calls __mmc_start_req()
> which calls mmc_start_request() which calls mmc_retune_hold()
>
> Then mmc_wait_for_req() calls mmc_wait_for_req_done() which calls
> mmc_retune_release().
>
> So
>         mmc_wait_for_cmd() (with no retries)
> has the same effect as
>         mmc_retune_hold()
>         mmc_wait_for_cmd()
>         mmc_retune_release()
>

Huh, you are right - again.

There have been a couple of iterations of this patchset, I don't
recall why we need to hold retune for all requests? It seems awkward.
Shouldn't we just hold retune for those requests that needs it?

>>
>> Anyway, my proposal didn't quite work, which is due to that
>> mmc_deselect_cards() (invoked from mmc_sleep()) deals with retries. If
>> there had been only one try, I thought it could be okay to have that
>> command to be preceded by a re-tune.
>>
>> Anyway, I would like you to move the mmc_retune_hold|release() calls
>> into the mmc_sleep() function.
>
> That would have no effect as explained above.

Then why did you add it to the _mmc_suspend() function? What am I missing here?

>
>>
>>>
>>> The options for the caller are:
>>>
>>> 1)
>>>         hold re-tuning
>>>         put emmc to sleep
>>>         later wake up emmc
>>>         release re-tuning
>>>
>>> 2)
>>>         put emmc to sleep
>>>         later increment hold_count
>>>         wake up emmc ignoring CRC errors
>>>         release re-tuning
>>>
>>> But there is no wake-up function and the suspend path is using an unbalanced
>>> mmc_sleep i.e. no corresponding wake up.
>>>
>>> So that leaves what is happening now i.e. a comment plus explicit
>>> hold()/release() in _mmc_suspend() so that future changes to _mmc_suspend()
>>> know to take mmc_sleep re-tuning requirements into account.
>>
>> Why all this complexity?
>>
>> mmc_power_off() is called in _mmc_suspend(), that will eventually
>> disable re-tune. Thus re-tuning will be prevented for
>> commands/requests during the system PM resume sequence, until the card
>> has been fully re-initialized (and a tuning sequence done). Isn't that
>> sufficient?
>
> Yes my original patch did not have any of that complexity. I added it in
> response to our discussions.
>
> As you wrote, _mmc_suspend() does not need to do anything with retuning
> because mmc_sleep() is followed by mmc_power_off().
>
> The original patch added a comment to mmc_sleep() and that was all. That
> would still be the best approach.
>

What I had in mind was that the re-tune timer could time out in the
middle of the _mmc_suspend() sequence.

If that happens in-between mmc_deselect_cards() and when the CMD5 is
to be sent, in mmc_sleep() - we must not allow a re-tune sequence.
Unless holding re-tune here, how is that prevented?

Kind regards
Uffe

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

[Ulf Hansson]

On 6 May 2015 at 13:09, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 04/05/15 16:55, Ulf Hansson wrote:
>> On 20 April 2015 at 14:09, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> 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 b345844..d11fae7 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -2307,8 +2307,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);
>>
>> I would rather see this to be handled by the mmc core, to have all
>> hosts benefit from it.
>
> Now that the core is enabling re-tuning, that will cause re-tuning to happen
> on platforms where it has never been used before (or more specifically where
> tuning has been used only during initialization)
>
> I have no idea if that is the right thing for all host controllers and
> platforms.
>
> Are you sure you want to do that?

This is required by the specs, so at least I think we should give it a try.

If we see issues with it, we could have it as an opt-in/out MMC_CAP.

Kind regards
Uffe

Re: [PATCH V6 08/15] mmc: mmc: Hold re-tuning if the card is put to sleep

[Adrian Hunter]

On 06/05/15 14:36, Ulf Hansson wrote:
> [...]
> 
>>>>> Instead of add mmc_retune_hold|release() to _mmc_suspend(), I would
>>>>> like you to move that handling into mmc_sleep(). The code should be
>>>>> easier and it becomes more clear that it's because of a command
>>>>> sequence.
>>>>>
>>>>> I think mmc_retune_hold() should be invoked before mmc_wait_for_cmd()
>>>>> and then mmc_retune_release() just after, in mmc_sleep(). That should
>>>>> work, right!?
>>>>
>>>> That would be the same as holding re-tuning for that request, which is
>>>> what already happens i.e. adding hold()/release() around mmc_wait_for_cmd()
>>>> is redundant.
>>>
>>> I don't understand your point, sorry.
>>
>> mmc_wait_for_cmd() calls mmc_wait_for_req() which calls __mmc_start_req()
>> which calls mmc_start_request() which calls mmc_retune_hold()
>>
>> Then mmc_wait_for_req() calls mmc_wait_for_req_done() which calls
>> mmc_retune_release().
>>
>> So
>>         mmc_wait_for_cmd() (with no retries)
>> has the same effect as
>>         mmc_retune_hold()
>>         mmc_wait_for_cmd()
>>         mmc_retune_release()
>>
> 
> Huh, you are right - again.
> 
> There have been a couple of iterations of this patchset, I don't
> recall why we need to hold retune for all requests? It seems awkward.
> Shouldn't we just hold retune for those requests that needs it?

For data requests (which also call __mmc_start_req()) there is the
possibility that a 'write' is not finished and is polled with CMD13.
So re-tuning is held to avoid conflicting with the busy state.
It also aids controlling when re-tuning happens in the recovery path
i.e. we have a go at getting the status first and if that doesn't
work first time, then re-tune if needed.

Also mmc_retune_hold() does not only hold retuning, it also causes
re-tuning to happen if the hold_count was zero, so it does
"make-retuning-happen-if-needed-and-not-already-held-and-then-hold-retuning"

> 
>>>
>>> Anyway, my proposal didn't quite work, which is due to that
>>> mmc_deselect_cards() (invoked from mmc_sleep()) deals with retries. If
>>> there had been only one try, I thought it could be okay to have that
>>> command to be preceded by a re-tune.
>>>
>>> Anyway, I would like you to move the mmc_retune_hold|release() calls
>>> into the mmc_sleep() function.
>>
>> That would have no effect as explained above.
> 
> Then why did you add it to the _mmc_suspend() function? What am I missing here?

It was added in response to our discussions. It was not in my original
patches. I can take it out.

> 
>>
>>>
>>>>
>>>> The options for the caller are:
>>>>
>>>> 1)
>>>>         hold re-tuning
>>>>         put emmc to sleep
>>>>         later wake up emmc
>>>>         release re-tuning
>>>>
>>>> 2)
>>>>         put emmc to sleep
>>>>         later increment hold_count
>>>>         wake up emmc ignoring CRC errors
>>>>         release re-tuning
>>>>
>>>> But there is no wake-up function and the suspend path is using an unbalanced
>>>> mmc_sleep i.e. no corresponding wake up.
>>>>
>>>> So that leaves what is happening now i.e. a comment plus explicit
>>>> hold()/release() in _mmc_suspend() so that future changes to _mmc_suspend()
>>>> know to take mmc_sleep re-tuning requirements into account.
>>>
>>> Why all this complexity?
>>>
>>> mmc_power_off() is called in _mmc_suspend(), that will eventually
>>> disable re-tune. Thus re-tuning will be prevented for
>>> commands/requests during the system PM resume sequence, until the card
>>> has been fully re-initialized (and a tuning sequence done). Isn't that
>>> sufficient?
>>
>> Yes my original patch did not have any of that complexity. I added it in
>> response to our discussions.
>>
>> As you wrote, _mmc_suspend() does not need to do anything with retuning
>> because mmc_sleep() is followed by mmc_power_off().
>>
>> The original patch added a comment to mmc_sleep() and that was all. That
>> would still be the best approach.
>>
> 
> What I had in mind was that the re-tune timer could time out in the
> middle of the _mmc_suspend() sequence.
> 
> If that happens in-between mmc_deselect_cards() and when the CMD5 is
> to be sent, in mmc_sleep() - we must not allow a re-tune sequence.
> Unless holding re-tune here, how is that prevented?

Oh yes, I have overlooked that re-tuning can't be done on a de-selected
card. So I will add mmc_retune_hold()/mmc_retune_release(). I will have to
think about the error handling. It looks broken now anyway since it doesn't
reselect the card in the error path.

Re: [PATCH V6 08/15] mmc: mmc: Hold re-tuning if the card is put to sleep

[Ulf Hansson]

On 6 May 2015 at 14:42, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 06/05/15 14:36, Ulf Hansson wrote:
>> [...]
>>
>>>>>> Instead of add mmc_retune_hold|release() to _mmc_suspend(), I would
>>>>>> like you to move that handling into mmc_sleep(). The code should be
>>>>>> easier and it becomes more clear that it's because of a command
>>>>>> sequence.
>>>>>>
>>>>>> I think mmc_retune_hold() should be invoked before mmc_wait_for_cmd()
>>>>>> and then mmc_retune_release() just after, in mmc_sleep(). That should
>>>>>> work, right!?
>>>>>
>>>>> That would be the same as holding re-tuning for that request, which is
>>>>> what already happens i.e. adding hold()/release() around mmc_wait_for_cmd()
>>>>> is redundant.
>>>>
>>>> I don't understand your point, sorry.
>>>
>>> mmc_wait_for_cmd() calls mmc_wait_for_req() which calls __mmc_start_req()
>>> which calls mmc_start_request() which calls mmc_retune_hold()
>>>
>>> Then mmc_wait_for_req() calls mmc_wait_for_req_done() which calls
>>> mmc_retune_release().
>>>
>>> So
>>>         mmc_wait_for_cmd() (with no retries)
>>> has the same effect as
>>>         mmc_retune_hold()
>>>         mmc_wait_for_cmd()
>>>         mmc_retune_release()
>>>
>>
>> Huh, you are right - again.
>>
>> There have been a couple of iterations of this patchset, I don't
>> recall why we need to hold retune for all requests? It seems awkward.
>> Shouldn't we just hold retune for those requests that needs it?
>
> For data requests (which also call __mmc_start_req()) there is the
> possibility that a 'write' is not finished and is polled with CMD13.
> So re-tuning is held to avoid conflicting with the busy state.
> It also aids controlling when re-tuning happens in the recovery path
> i.e. we have a go at getting the status first and if that doesn't
> work first time, then re-tune if needed.
>
> Also mmc_retune_hold() does not only hold retuning, it also causes
> re-tuning to happen if the hold_count was zero, so it does
> "make-retuning-happen-if-needed-and-not-already-held-and-then-hold-retuning"

Hmm, is there anyway we can make this easier to understand in the code
path and maybe clarify via the name of functions/APIs you add? Could
we have a state variable instead of bunch of int variables?

Since I apparently have a bit hard time to understand how this
actually works, I am a bit concerned about the maintenance of it. :-)

Anyway, if you can't find any better option - I will accept it as is.

>
>>
>>>>
>>>> Anyway, my proposal didn't quite work, which is due to that
>>>> mmc_deselect_cards() (invoked from mmc_sleep()) deals with retries. If
>>>> there had been only one try, I thought it could be okay to have that
>>>> command to be preceded by a re-tune.
>>>>
>>>> Anyway, I would like you to move the mmc_retune_hold|release() calls
>>>> into the mmc_sleep() function.
>>>
>>> That would have no effect as explained above.
>>
>> Then why did you add it to the _mmc_suspend() function? What am I missing here?
>
> It was added in response to our discussions. It was not in my original
> patches. I can take it out.
>
>>
>>>
>>>>
>>>>>
>>>>> The options for the caller are:
>>>>>
>>>>> 1)
>>>>>         hold re-tuning
>>>>>         put emmc to sleep
>>>>>         later wake up emmc
>>>>>         release re-tuning
>>>>>
>>>>> 2)
>>>>>         put emmc to sleep
>>>>>         later increment hold_count
>>>>>         wake up emmc ignoring CRC errors
>>>>>         release re-tuning
>>>>>
>>>>> But there is no wake-up function and the suspend path is using an unbalanced
>>>>> mmc_sleep i.e. no corresponding wake up.
>>>>>
>>>>> So that leaves what is happening now i.e. a comment plus explicit
>>>>> hold()/release() in _mmc_suspend() so that future changes to _mmc_suspend()
>>>>> know to take mmc_sleep re-tuning requirements into account.
>>>>
>>>> Why all this complexity?
>>>>
>>>> mmc_power_off() is called in _mmc_suspend(), that will eventually
>>>> disable re-tune. Thus re-tuning will be prevented for
>>>> commands/requests during the system PM resume sequence, until the card
>>>> has been fully re-initialized (and a tuning sequence done). Isn't that
>>>> sufficient?
>>>
>>> Yes my original patch did not have any of that complexity. I added it in
>>> response to our discussions.
>>>
>>> As you wrote, _mmc_suspend() does not need to do anything with retuning
>>> because mmc_sleep() is followed by mmc_power_off().
>>>
>>> The original patch added a comment to mmc_sleep() and that was all. That
>>> would still be the best approach.
>>>
>>
>> What I had in mind was that the re-tune timer could time out in the
>> middle of the _mmc_suspend() sequence.
>>
>> If that happens in-between mmc_deselect_cards() and when the CMD5 is
>> to be sent, in mmc_sleep() - we must not allow a re-tune sequence.
>> Unless holding re-tune here, how is that prevented?
>
> Oh yes, I have overlooked that re-tuning can't be done on a de-selected
> card. So I will add mmc_retune_hold()/mmc_retune_release(). I will have to
> think about the error handling. It looks broken now anyway since it doesn't
> reselect the card in the error path.
>

I suggest you don't bother about the error handling for now, we can
take that separately.

Kind regards
Uffe

Re: [PATCH V6 08/15] mmc: mmc: Hold re-tuning if the card is put to sleep

[Adrian Hunter]

On 06/05/15 16:21, Ulf Hansson wrote:
> On 6 May 2015 at 14:42, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 06/05/15 14:36, Ulf Hansson wrote:
>>> [...]
>>>
>>>>>>> Instead of add mmc_retune_hold|release() to _mmc_suspend(), I would
>>>>>>> like you to move that handling into mmc_sleep(). The code should be
>>>>>>> easier and it becomes more clear that it's because of a command
>>>>>>> sequence.
>>>>>>>
>>>>>>> I think mmc_retune_hold() should be invoked before mmc_wait_for_cmd()
>>>>>>> and then mmc_retune_release() just after, in mmc_sleep(). That should
>>>>>>> work, right!?
>>>>>>
>>>>>> That would be the same as holding re-tuning for that request, which is
>>>>>> what already happens i.e. adding hold()/release() around mmc_wait_for_cmd()
>>>>>> is redundant.
>>>>>
>>>>> I don't understand your point, sorry.
>>>>
>>>> mmc_wait_for_cmd() calls mmc_wait_for_req() which calls __mmc_start_req()
>>>> which calls mmc_start_request() which calls mmc_retune_hold()
>>>>
>>>> Then mmc_wait_for_req() calls mmc_wait_for_req_done() which calls
>>>> mmc_retune_release().
>>>>
>>>> So
>>>>         mmc_wait_for_cmd() (with no retries)
>>>> has the same effect as
>>>>         mmc_retune_hold()
>>>>         mmc_wait_for_cmd()
>>>>         mmc_retune_release()
>>>>
>>>
>>> Huh, you are right - again.
>>>
>>> There have been a couple of iterations of this patchset, I don't
>>> recall why we need to hold retune for all requests? It seems awkward.
>>> Shouldn't we just hold retune for those requests that needs it?
>>
>> For data requests (which also call __mmc_start_req()) there is the
>> possibility that a 'write' is not finished and is polled with CMD13.
>> So re-tuning is held to avoid conflicting with the busy state.
>> It also aids controlling when re-tuning happens in the recovery path
>> i.e. we have a go at getting the status first and if that doesn't
>> work first time, then re-tune if needed.
>>
>> Also mmc_retune_hold() does not only hold retuning, it also causes
>> re-tuning to happen if the hold_count was zero, so it does
>> "make-retuning-happen-if-needed-and-not-already-held-and-then-hold-retuning"
> 
> Hmm, is there anyway we can make this easier to understand in the code
> path and maybe clarify via the name of functions/APIs you add? Could
> we have a state variable instead of bunch of int variables?

The ints are needed either to allow nesting or atomic update.

> 
> Since I apparently have a bit hard time to understand how this
> actually works, I am a bit concerned about the maintenance of it. :-)

There are only a few things to remember:

	1. Re-tuning can happen before every request i.e. inside mmc_wait_for_req()
or mmc_start_req()

	2. If you have several requests where re-tuning can't be done in between
them, then you can put mmc_retune_hold() / mmc_retune_release()
around them

	3. A sleep state, like the brcm custom sleep state, might need to prevent
re-tuning for the wakeup command

> 
> Anyway, if you can't find any better option - I will accept it as is.
> 
>>
>>>
>>>>>
>>>>> Anyway, my proposal didn't quite work, which is due to that
>>>>> mmc_deselect_cards() (invoked from mmc_sleep()) deals with retries. If
>>>>> there had been only one try, I thought it could be okay to have that
>>>>> command to be preceded by a re-tune.
>>>>>
>>>>> Anyway, I would like you to move the mmc_retune_hold|release() calls
>>>>> into the mmc_sleep() function.
>>>>
>>>> That would have no effect as explained above.
>>>
>>> Then why did you add it to the _mmc_suspend() function? What am I missing here?
>>
>> It was added in response to our discussions. It was not in my original
>> patches. I can take it out.
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> The options for the caller are:
>>>>>>
>>>>>> 1)
>>>>>>         hold re-tuning
>>>>>>         put emmc to sleep
>>>>>>         later wake up emmc
>>>>>>         release re-tuning
>>>>>>
>>>>>> 2)
>>>>>>         put emmc to sleep
>>>>>>         later increment hold_count
>>>>>>         wake up emmc ignoring CRC errors
>>>>>>         release re-tuning
>>>>>>
>>>>>> But there is no wake-up function and the suspend path is using an unbalanced
>>>>>> mmc_sleep i.e. no corresponding wake up.
>>>>>>
>>>>>> So that leaves what is happening now i.e. a comment plus explicit
>>>>>> hold()/release() in _mmc_suspend() so that future changes to _mmc_suspend()
>>>>>> know to take mmc_sleep re-tuning requirements into account.
>>>>>
>>>>> Why all this complexity?
>>>>>
>>>>> mmc_power_off() is called in _mmc_suspend(), that will eventually
>>>>> disable re-tune. Thus re-tuning will be prevented for
>>>>> commands/requests during the system PM resume sequence, until the card
>>>>> has been fully re-initialized (and a tuning sequence done). Isn't that
>>>>> sufficient?
>>>>
>>>> Yes my original patch did not have any of that complexity. I added it in
>>>> response to our discussions.
>>>>
>>>> As you wrote, _mmc_suspend() does not need to do anything with retuning
>>>> because mmc_sleep() is followed by mmc_power_off().
>>>>
>>>> The original patch added a comment to mmc_sleep() and that was all. That
>>>> would still be the best approach.
>>>>
>>>
>>> What I had in mind was that the re-tune timer could time out in the
>>> middle of the _mmc_suspend() sequence.
>>>
>>> If that happens in-between mmc_deselect_cards() and when the CMD5 is
>>> to be sent, in mmc_sleep() - we must not allow a re-tune sequence.
>>> Unless holding re-tune here, how is that prevented?
>>
>> Oh yes, I have overlooked that re-tuning can't be done on a de-selected
>> card. So I will add mmc_retune_hold()/mmc_retune_release(). I will have to
>> think about the error handling. It looks broken now anyway since it doesn't
>> reselect the card in the error path.
>>
> 
> I suggest you don't bother about the error handling for now, we can
> take that separately.

Ok, thanks!