[RFC PATCH 00/13] mmc: host: Add facility to support re-tuning

[RFC PATCH 00/13] mmc: host: Add facility to support re-tuning

[Adrian Hunter]

Hi

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

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

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

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


Adrian Hunter (13):
      mmc: core: Simplify by adding mmc_execute_tuning()
      mmc: host: Add facility to support re-tuning
      mmc: core: Disable re-tuning when card is no longer initialized
      mmc: core: Move mmc_card_removed() into mmc_start_request()
      mmc: core: Add support for re-tuning before each request
      mmc: core: Check re-tuning before retrying
      mmc: core: Hold re-tuning during switch commands
      mmc: core: Hold re-tuning during erase commands
      mmc: core: Hold re-tuning while bkops ongoing
      mmc: mmc: Comment that callers need to hold re-tuning if the card is put to sleep
      mmc: core: Add support for HS400 re-tuning
      mmc: sdhci: Always init buf_ready_int
      mmc: sdhci: Change to new way of doing re-tuning

 drivers/mmc/core/core.c    |  93 +++++++++++++++++++++++++++++++-----
 drivers/mmc/core/core.h    |   5 ++
 drivers/mmc/core/host.c    |  63 +++++++++++++++++++++++++
 drivers/mmc/core/mmc.c     |  58 ++++++++++++++++++-----
 drivers/mmc/core/mmc_ops.c |  31 ++++++++----
 drivers/mmc/core/sd.c      |  13 ++---
 drivers/mmc/core/sdio.c    |  14 ++----
 drivers/mmc/host/sdhci.c   | 115 ++++++---------------------------------------
 include/linux/mmc/host.h   |  58 +++++++++++++++++++++++
 include/linux/mmc/sdhci.h  |   3 --
 10 files changed, 296 insertions(+), 157 deletions(-)


Regards
Adrian

[PATCH 01/13] mmc: core: Simplify by adding mmc_execute_tuning()

[Adrian Hunter]

For each MMC, SD and SDIO there is code that
holds the clock, calls ops->execute_tuning, and
releases the clock. Simplify the code a bit by
providing a separate function to do that.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/core.c | 24 ++++++++++++++++++++++++
 drivers/mmc/core/core.h |  3 +++
 drivers/mmc/core/mmc.c  | 14 +-------------
 drivers/mmc/core/sd.c   | 13 ++++---------
 drivers/mmc/core/sdio.c | 14 ++++----------
 5 files changed, 36 insertions(+), 32 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 9584bff..b329725 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1077,6 +1077,30 @@ void mmc_set_ungated(struct mmc_host *host)
 }
 #endif
 
+int mmc_execute_tuning(struct mmc_card *card)
+{
+	struct mmc_host *host = card->host;
+	u32 opcode;
+	int err;
+
+	if (!host->ops->execute_tuning)
+		return 0;
+
+	if (mmc_card_mmc(card))
+		opcode = MMC_SEND_TUNING_BLOCK_HS200;
+	else
+		opcode = MMC_SEND_TUNING_BLOCK;
+
+	mmc_host_clk_hold(host);
+	err = host->ops->execute_tuning(host, opcode);
+	mmc_host_clk_release(host);
+
+	if (err)
+		pr_err("%s: tuning execution failed\n", mmc_hostname(host));
+
+	return err;
+}
+
 /*
  * Change the bus mode (open drain/push-pull) of a host.
  */
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index d76597c..1b022c9 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -82,5 +82,8 @@ void mmc_add_card_debugfs(struct mmc_card *card);
 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);
+
 #endif
 
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 02ad792..c712ba7 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1194,7 +1194,6 @@ EXPORT_SYMBOL(tuning_blk_pattern_8bit);
 static int mmc_hs200_tuning(struct mmc_card *card)
 {
 	struct mmc_host *host = card->host;
-	int err = 0;
 
 	/*
 	 * Timing should be adjusted to the HS400 target
@@ -1205,18 +1204,7 @@ static int mmc_hs200_tuning(struct mmc_card *card)
 		if (host->ops->prepare_hs400_tuning)
 			host->ops->prepare_hs400_tuning(host, &host->ios);
 
-	if (host->ops->execute_tuning) {
-		mmc_host_clk_hold(host);
-		err = host->ops->execute_tuning(host,
-				MMC_SEND_TUNING_BLOCK_HS200);
-		mmc_host_clk_release(host);
-
-		if (err)
-			pr_err("%s: tuning execution failed\n",
-				mmc_hostname(host));
-	}
-
-	return err;
+	return mmc_execute_tuning(card);
 }
 
 /*
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index d90a6de..7907544 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -660,15 +660,10 @@ static int mmc_sd_init_uhs_card(struct mmc_card *card)
 	 * SPI mode doesn't define CMD19 and tuning is only valid for SDR50 and
 	 * SDR104 mode SD-cards. Note that tuning is mandatory for SDR104.
 	 */
-	if (!mmc_host_is_spi(card->host) && card->host->ops->execute_tuning &&
-			(card->sd_bus_speed == UHS_SDR50_BUS_SPEED ||
-			 card->sd_bus_speed == UHS_SDR104_BUS_SPEED)) {
-		mmc_host_clk_hold(card->host);
-		err = card->host->ops->execute_tuning(card->host,
-						      MMC_SEND_TUNING_BLOCK);
-		mmc_host_clk_release(card->host);
-	}
-
+	if (!mmc_host_is_spi(card->host) &&
+	    (card->sd_bus_speed == UHS_SDR50_BUS_SPEED ||
+	     card->sd_bus_speed == UHS_SDR104_BUS_SPEED))
+		err = mmc_execute_tuning(card);
 out:
 	kfree(status);
 
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index fd0750b..ce6cc47 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -567,17 +567,11 @@ static int mmc_sdio_init_uhs_card(struct mmc_card *card)
 	 * SPI mode doesn't define CMD19 and tuning is only valid for SDR50 and
 	 * SDR104 mode SD-cards. Note that tuning is mandatory for SDR104.
 	 */
-	if (!mmc_host_is_spi(card->host) && card->host->ops->execute_tuning &&
-			((card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR50) ||
-			 (card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR104))) {
-		mmc_host_clk_hold(card->host);
-		err = card->host->ops->execute_tuning(card->host,
-						      MMC_SEND_TUNING_BLOCK);
-		mmc_host_clk_release(card->host);
-	}
-
+	if (!mmc_host_is_spi(card->host) &&
+	    ((card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR50) ||
+	     (card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR104)))
+		err = mmc_execute_tuning(card);
 out:
-
 	return err;
 }
 
-- 
1.9.1

[PATCH 02/13] mmc: host: Add facility to support re-tuning

[Adrian Hunter]

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

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

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

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

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

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

Subsequent patches take those functions into use.

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

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

[PATCH 03/13] mmc: core: Disable re-tuning when card is no longer initialized

[Adrian Hunter]

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

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

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

[PATCH 04/13] mmc: core: Move mmc_card_removed() into mmc_start_request()

[Adrian Hunter]

Both callers of mmc_start_request() call mmc_card_removed()
so move that call into mmc_start_request().

This patch is preparation for adding re-tuning support.

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

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index fcde4da..884f35a 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -185,13 +185,14 @@ 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)
+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
+	if (mmc_card_removed(host->card))
+		return -ENOMEDIUM;
 
 	if (mrq->sbc) {
 		pr_debug("<%s: starting CMD%u arg %08x flags %08x>\n",
@@ -251,6 +252,8 @@ 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);
+
+	return 0;
 }
 
 /**
@@ -345,29 +348,34 @@ static void mmc_wait_done(struct mmc_request *mrq)
  */
 static int __mmc_start_data_req(struct mmc_host *host, struct mmc_request *mrq)
 {
+	int err;
+
 	mrq->done = mmc_wait_data_done;
 	mrq->host = host;
-	if (mmc_card_removed(host->card)) {
-		mrq->cmd->error = -ENOMEDIUM;
+
+	err = mmc_start_request(host, mrq);
+	if (err) {
+		mrq->cmd->error = err;
 		mmc_wait_data_done(mrq);
-		return -ENOMEDIUM;
 	}
-	mmc_start_request(host, mrq);
 
-	return 0;
+	return err;
 }
 
 static int __mmc_start_req(struct mmc_host *host, struct mmc_request *mrq)
 {
+	int err;
+
 	init_completion(&mrq->completion);
 	mrq->done = mmc_wait_done;
-	if (mmc_card_removed(host->card)) {
-		mrq->cmd->error = -ENOMEDIUM;
+
+	err = mmc_start_request(host, mrq);
+	if (err) {
+		mrq->cmd->error = err;
 		complete(&mrq->completion);
-		return -ENOMEDIUM;
 	}
-	mmc_start_request(host, mrq);
-	return 0;
+
+	return err;
 }
 
 /*
-- 
1.9.1

[PATCH 05/13] 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 | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 884f35a..263b2d6 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -191,9 +191,15 @@ static int mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
 	unsigned int i, sz;
 	struct scatterlist *sg;
 #endif
+	int err;
+
 	if (mmc_card_removed(host->card))
 		return -ENOMEDIUM;
 
+	err = mmc_retune_and_hold(host);
+	if (err)
+		return err;
+
 	if (mrq->sbc) {
 		pr_debug("<%s: starting CMD%u arg %08x flags %08x>\n",
 			 mmc_hostname(host), mrq->sbc->opcode,
@@ -428,10 +434,12 @@ static int mmc_wait_for_data_req_done(struct mmc_host *host,
 			context_info->is_new_req = false;
 			if (!next_req) {
 				err = MMC_BLK_NEW_REQUEST;
+				mmc_retune_hold(host);
 				break; /* return err */
 			}
 		}
 	}
+	mmc_retune_release(host);
 	return err;
 }
 
@@ -472,6 +480,8 @@ static void mmc_wait_for_req_done(struct mmc_host *host,
 		cmd->error = 0;
 		host->ops->request(host, mrq);
 	}
+
+	mmc_retune_release(host);
 }
 
 /**
-- 
1.9.1

[PATCH 06/13] mmc: core: Check re-tuning before retrying

[Adrian Hunter]

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

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

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

[PATCH 07/13] mmc: core: Hold re-tuning during switch commands

[Adrian Hunter]

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

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

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 3b044c5..772c585 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -444,6 +444,10 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 	u32 status = 0;
 	bool use_r1b_resp = use_busy_signal;
 
+	err = mmc_retune_and_hold(host);
+	if (err)
+		return err;
+
 	/*
 	 * If the cmd timeout and the max_busy_timeout of the host are both
 	 * specified, let's validate them. A failure means we need to prevent
@@ -476,11 +480,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
@@ -499,7 +503,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;
@@ -513,29 +517,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 08/13] mmc: core: Hold re-tuning during erase commands

[Adrian Hunter]

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

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

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 2c10fb9..3da5fa8 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1956,6 +1956,10 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from,
 	unsigned long timeout;
 	int err;
 
+	err = mmc_retune_and_hold(card->host);
+	if (err)
+		return err;
+
 	/*
 	 * qty is used to calculate the erase timeout which depends on how many
 	 * erase groups (or allocation units in SD terminology) are affected.
@@ -2059,6 +2063,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 09/13] mmc: core: Hold re-tuning while bkops ongoing

[Adrian Hunter]

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

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

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 3da5fa8..8fc90d3 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -304,6 +304,10 @@ void mmc_start_bkops(struct mmc_card *card, bool from_exception)
 	} else {
 		timeout = 0;
 		use_busy_signal = false;
+		/* Hold re-tuning for ongoing bkops */
+		err = mmc_retune_and_hold(card->host);
+		if (err)
+			goto out;
 	}
 
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
@@ -312,6 +316,9 @@ void mmc_start_bkops(struct mmc_card *card, bool from_exception)
 	if (err) {
 		pr_warn("%s: Error %d starting bkops\n",
 			mmc_hostname(card->host), err);
+		/* bkops not ongoing, so release re-tuning */
+		if (!use_busy_signal)
+			mmc_retune_release(card->host);
 		goto out;
 	}
 
@@ -749,6 +756,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 10/13] mmc: mmc: Comment that callers need to hold re-tuning if the card is put to sleep

[Adrian Hunter]

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

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

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

[PATCH 11/13] 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  | 43 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+)

diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index 1b022c9..2a7d9219 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -84,6 +84,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 a4aa25b..62c4770 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -318,6 +318,7 @@ void mmc_retune_timer_stop(struct mmc_host *host)
 
 int mmc_retune(struct mmc_host *host)
 {
+	bool return_to_hs400 = false;
 	int err;
 
 	if (!host->need_retune || host->doing_retune || host->hold_retune ||
@@ -328,8 +329,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 7a913e6..167340c 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1081,6 +1081,49 @@ 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);
+}
+
+int mmc_hs400_to_hs200(struct mmc_card *card)
+{
+	unsigned int max_dtr;
+	int err;
+
+	/* Reduce frequency to HS */
+	max_dtr = card->ext_csd.hs_max_dtr;
+	mmc_set_clock(card->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, true, true);
+	if (err)
+		return err;
+
+	mmc_set_timing(card->host, MMC_TIMING_MMC_DDR52);
+
+	/* 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);
+	if (err)
+		return err;
+
+	mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
+
+	/* 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, true, true);
+	if (err)
+		return err;
+
+	mmc_set_timing(card->host, MMC_TIMING_MMC_HS200);
+
+	return err;
+}
+
 /*
  * For device supporting HS200 mode, the following sequence
  * should be done before executing the tuning process.
-- 
1.9.1

[PATCH 12/13] mmc: sdhci: Always init buf_ready_int

[Adrian Hunter]

There is no point making the initialization
of buf_ready_int conditional on host version.
Simplify by just doing it always. Note that
the other conditional initializations will be
removed when the new way of doing re-tuning
is taken into use.

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

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index bfde165..dd4cc4c 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -3331,9 +3331,9 @@ int sdhci_add_host(struct sdhci_host *host)
 
 	setup_timer(&host->timer, sdhci_timeout_timer, (unsigned long)host);
 
-	if (host->version >= SDHCI_SPEC_300) {
-		init_waitqueue_head(&host->buf_ready_int);
+	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;
-- 
1.9.1

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

[Adrian Hunter]

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

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

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

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index dd4cc4c..4e50db1 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -51,7 +51,6 @@ static void sdhci_finish_data(struct sdhci_host *);
 
 static void sdhci_finish_command(struct sdhci_host *);
 static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode);
-static void sdhci_tuning_timer(unsigned long data);
 static void sdhci_enable_preset_value(struct sdhci_host *host, bool enable);
 
 #ifdef CONFIG_PM_RUNTIME
@@ -249,17 +248,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);
 }
 
@@ -1345,7 +1333,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);
 
@@ -1393,39 +1380,6 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 		host->mrq->cmd->error = -ENOMEDIUM;
 		tasklet_schedule(&host->finish_tasklet);
 	} else {
-		u32 present_state;
-
-		present_state = sdhci_readl(host, SDHCI_PRESENT_STATE);
-		/*
-		 * Check if the re-tuning timer has already expired and there
-		 * is no on-going data transfer and DAT0 is not busy. If so,
-		 * we need to execute tuning procedure before sending command.
-		 */
-		if ((host->flags & SDHCI_NEEDS_RETUNING) &&
-		    !(present_state & (SDHCI_DOING_WRITE | SDHCI_DOING_READ)) &&
-		    (present_state & SDHCI_DATA_0_LVL_MASK)) {
-			if (mmc->card) {
-				/* eMMC uses cmd21 but sd and sdio use cmd19 */
-				tuning_opcode =
-					mmc->card->type == MMC_TYPE_MMC ?
-					MMC_SEND_TUNING_BLOCK_HS200 :
-					MMC_SEND_TUNING_BLOCK;
-
-				/* Here we need to set the host->mrq to NULL,
-				 * in case the pending finish_tasklet
-				 * finishes it incorrectly.
-				 */
-				host->mrq = NULL;
-
-				spin_unlock_irqrestore(&host->lock, flags);
-				sdhci_execute_tuning(mmc, tuning_opcode);
-				spin_lock_irqsave(&host->lock, flags);
-
-				/* Restore original mmc_request structure */
-				host->mrq = mrq;
-			}
-		}
-
 		if (mrq->sbc && !(host->flags & SDHCI_AUTO_CMD23))
 			sdhci_send_command(host, mrq->sbc);
 		else
@@ -2065,23 +2019,19 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
 	}
 
 out:
-	host->flags &= ~SDHCI_NEEDS_RETUNING;
-
 	if (tuning_count) {
-		host->flags |= SDHCI_USING_RETUNING_TIMER;
-		mod_timer(&host->tuning_timer, jiffies + tuning_count * HZ);
+		/*
+		 * In case tuning fails, host controllers which support
+		 * re-tuning can try tuning again at a later time, when the
+		 * re-tuning timer expires.  So for these controllers, we
+		 * return 0. Since there might be other controllers who do not
+		 * have this capability, we return error for them.
+		 */
+		err = 0;
 	}
 
-	/*
-	 * In case tuning fails, host controllers which support re-tuning can
-	 * try tuning again at a later time, when the re-tuning timer expires.
-	 * So for these controllers, we return 0. Since there might be other
-	 * controllers who do not have this capability, we return error for
-	 * them. SDHCI_USING_RETUNING_TIMER means the host is currently using
-	 * a retuning timer to do the retuning for the card.
-	 */
-	if (err && (host->flags & SDHCI_USING_RETUNING_TIMER))
-		err = 0;
+	if (!err)
+		mmc_retune_enable(host->mmc, tuning_count);
 
 	sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
 	sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
@@ -2261,20 +2211,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                                                        *
@@ -2652,11 +2588,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;
@@ -2706,10 +2639,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;
 }
 
@@ -2749,11 +2678,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;
@@ -2796,10 +2722,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;
@@ -3333,13 +3255,6 @@ int sdhci_add_host(struct sdhci_host *host)
 
 	init_waitqueue_head(&host->buf_ready_int);
 
-	if (host->version >= SDHCI_SPEC_300) {
-		/* Initialize re-tuning timer */
-		init_timer(&host->tuning_timer);
-		host->tuning_timer.data = (unsigned long)host;
-		host->tuning_timer.function = sdhci_tuning_timer;
-	}
-
 	sdhci_init(host, 0);
 
 	ret = request_threaded_irq(host->irq, sdhci_irq, sdhci_thread_irq,
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index f767a0d..c59ef8d 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -129,13 +129,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 */
 
@@ -201,7 +199,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 */
 
 	unsigned long private[0] ____cacheline_aligned;
 };
-- 
1.9.1

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

[Adrian Hunter]

On 05/12/14 19:40, Adrian Hunter wrote:
> Hi
> 
> Here are some patches to move re-tuning support out of
> sdhci and into the core, and add support for HS400
> re-tuning.
> 
> Currently sdhci does re-tuning transparently by
> calling sdhci_execute_tuning() from its ->request()
> function.
> 
> The problem with HS400 re-tuning is that it must be
> done in HS200 mode. That means using switch commands
> and making ios changes. That means it potentially
> conflicts with other command sequences. The new
> re-tuning support accomodates that, however it is
> not strictly necessary because HS400 re-tuning
> should only be needed after runtime suspend when
> there would not be a conflict.
> 
> Nevertheless, this approach has more structure
> and potentially more flexibility.
> 
> 
> Adrian Hunter (13):
>       mmc: core: Simplify by adding mmc_execute_tuning()
>       mmc: host: Add facility to support re-tuning
>       mmc: core: Disable re-tuning when card is no longer initialized
>       mmc: core: Move mmc_card_removed() into mmc_start_request()
>       mmc: core: Add support for re-tuning before each request
>       mmc: core: Check re-tuning before retrying
>       mmc: core: Hold re-tuning during switch commands
>       mmc: core: Hold re-tuning during erase commands
>       mmc: core: Hold re-tuning while bkops ongoing
>       mmc: mmc: Comment that callers need to hold re-tuning if the card is put to sleep
>       mmc: core: Add support for HS400 re-tuning
>       mmc: sdhci: Always init buf_ready_int
>       mmc: sdhci: Change to new way of doing re-tuning
> 
>  drivers/mmc/core/core.c    |  93 +++++++++++++++++++++++++++++++-----
>  drivers/mmc/core/core.h    |   5 ++
>  drivers/mmc/core/host.c    |  63 +++++++++++++++++++++++++
>  drivers/mmc/core/mmc.c     |  58 ++++++++++++++++++-----
>  drivers/mmc/core/mmc_ops.c |  31 ++++++++----
>  drivers/mmc/core/sd.c      |  13 ++---
>  drivers/mmc/core/sdio.c    |  14 ++----
>  drivers/mmc/host/sdhci.c   | 115 ++++++---------------------------------------
>  include/linux/mmc/host.h   |  58 +++++++++++++++++++++++
>  include/linux/mmc/sdhci.h  |   3 --
>  10 files changed, 296 insertions(+), 157 deletions(-)

Ulf, do you have any comments?

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

[Ulf Hansson]

On 19 December 2014 at 15:07, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 05/12/14 19:40, Adrian Hunter wrote:
>> Hi
>>
>> Here are some patches to move re-tuning support out of
>> sdhci and into the core, and add support for HS400
>> re-tuning.
>>
>> Currently sdhci does re-tuning transparently by
>> calling sdhci_execute_tuning() from its ->request()
>> function.
>>
>> The problem with HS400 re-tuning is that it must be
>> done in HS200 mode. That means using switch commands
>> and making ios changes. That means it potentially
>> conflicts with other command sequences. The new
>> re-tuning support accomodates that, however it is
>> not strictly necessary because HS400 re-tuning
>> should only be needed after runtime suspend when
>> there would not be a conflict.
>>
>> Nevertheless, this approach has more structure
>> and potentially more flexibility.
>>
>>
>> Adrian Hunter (13):
>>       mmc: core: Simplify by adding mmc_execute_tuning()
>>       mmc: host: Add facility to support re-tuning
>>       mmc: core: Disable re-tuning when card is no longer initialized
>>       mmc: core: Move mmc_card_removed() into mmc_start_request()
>>       mmc: core: Add support for re-tuning before each request
>>       mmc: core: Check re-tuning before retrying
>>       mmc: core: Hold re-tuning during switch commands
>>       mmc: core: Hold re-tuning during erase commands
>>       mmc: core: Hold re-tuning while bkops ongoing
>>       mmc: mmc: Comment that callers need to hold re-tuning if the card is put to sleep
>>       mmc: core: Add support for HS400 re-tuning
>>       mmc: sdhci: Always init buf_ready_int
>>       mmc: sdhci: Change to new way of doing re-tuning
>>
>>  drivers/mmc/core/core.c    |  93 +++++++++++++++++++++++++++++++-----
>>  drivers/mmc/core/core.h    |   5 ++
>>  drivers/mmc/core/host.c    |  63 +++++++++++++++++++++++++
>>  drivers/mmc/core/mmc.c     |  58 ++++++++++++++++++-----
>>  drivers/mmc/core/mmc_ops.c |  31 ++++++++----
>>  drivers/mmc/core/sd.c      |  13 ++---
>>  drivers/mmc/core/sdio.c    |  14 ++----
>>  drivers/mmc/host/sdhci.c   | 115 ++++++---------------------------------------
>>  include/linux/mmc/host.h   |  58 +++++++++++++++++++++++
>>  include/linux/mmc/sdhci.h  |   3 --
>>  10 files changed, 296 insertions(+), 157 deletions(-)
>
> Ulf, do you have any comments?
>

I am going through them as soon as I can. It's been a busy time lately.

Kind regards
Uffe

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

[Adrian Hunter]

On 19/12/14 16:07, Adrian Hunter wrote:
> On 05/12/14 19:40, Adrian Hunter wrote:
>> Hi
>>
>> Here are some patches to move re-tuning support out of
>> sdhci and into the core, and add support for HS400
>> re-tuning.
>>
>> Currently sdhci does re-tuning transparently by
>> calling sdhci_execute_tuning() from its ->request()
>> function.
>>
>> The problem with HS400 re-tuning is that it must be
>> done in HS200 mode. That means using switch commands
>> and making ios changes. That means it potentially
>> conflicts with other command sequences. The new
>> re-tuning support accomodates that, however it is
>> not strictly necessary because HS400 re-tuning
>> should only be needed after runtime suspend when
>> there would not be a conflict.
>>
>> Nevertheless, this approach has more structure
>> and potentially more flexibility.
>>
>>
>> Adrian Hunter (13):
>>       mmc: core: Simplify by adding mmc_execute_tuning()
>>       mmc: host: Add facility to support re-tuning
>>       mmc: core: Disable re-tuning when card is no longer initialized
>>       mmc: core: Move mmc_card_removed() into mmc_start_request()
>>       mmc: core: Add support for re-tuning before each request
>>       mmc: core: Check re-tuning before retrying
>>       mmc: core: Hold re-tuning during switch commands
>>       mmc: core: Hold re-tuning during erase commands
>>       mmc: core: Hold re-tuning while bkops ongoing
>>       mmc: mmc: Comment that callers need to hold re-tuning if the card is put to sleep
>>       mmc: core: Add support for HS400 re-tuning
>>       mmc: sdhci: Always init buf_ready_int
>>       mmc: sdhci: Change to new way of doing re-tuning
>>
>>  drivers/mmc/core/core.c    |  93 +++++++++++++++++++++++++++++++-----
>>  drivers/mmc/core/core.h    |   5 ++
>>  drivers/mmc/core/host.c    |  63 +++++++++++++++++++++++++
>>  drivers/mmc/core/mmc.c     |  58 ++++++++++++++++++-----
>>  drivers/mmc/core/mmc_ops.c |  31 ++++++++----
>>  drivers/mmc/core/sd.c      |  13 ++---
>>  drivers/mmc/core/sdio.c    |  14 ++----
>>  drivers/mmc/host/sdhci.c   | 115 ++++++---------------------------------------
>>  include/linux/mmc/host.h   |  58 +++++++++++++++++++++++
>>  include/linux/mmc/sdhci.h  |   3 --
>>  10 files changed, 296 insertions(+), 157 deletions(-)
> 
> Ulf, do you have any comments?

Ping?

Re: [PATCH 01/13] mmc: core: Simplify by adding mmc_execute_tuning()

[Ulf Hansson]

On 5 December 2014 at 18:40, Adrian Hunter <adrian.hunter@intel.com> wrote:
> For each MMC, SD and SDIO there is code that
> holds the clock, calls ops->execute_tuning, and
> releases the clock. Simplify the code a bit by
> providing a separate function to do that.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>

Thanks! Applied for next.

Kind regards
Uffe

> ---
>  drivers/mmc/core/core.c | 24 ++++++++++++++++++++++++
>  drivers/mmc/core/core.h |  3 +++
>  drivers/mmc/core/mmc.c  | 14 +-------------
>  drivers/mmc/core/sd.c   | 13 ++++---------
>  drivers/mmc/core/sdio.c | 14 ++++----------
>  5 files changed, 36 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 9584bff..b329725 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1077,6 +1077,30 @@ void mmc_set_ungated(struct mmc_host *host)
>  }
>  #endif
>
> +int mmc_execute_tuning(struct mmc_card *card)
> +{
> +       struct mmc_host *host = card->host;
> +       u32 opcode;
> +       int err;
> +
> +       if (!host->ops->execute_tuning)
> +               return 0;
> +
> +       if (mmc_card_mmc(card))
> +               opcode = MMC_SEND_TUNING_BLOCK_HS200;
> +       else
> +               opcode = MMC_SEND_TUNING_BLOCK;
> +
> +       mmc_host_clk_hold(host);
> +       err = host->ops->execute_tuning(host, opcode);
> +       mmc_host_clk_release(host);
> +
> +       if (err)
> +               pr_err("%s: tuning execution failed\n", mmc_hostname(host));
> +
> +       return err;
> +}
> +
>  /*
>   * Change the bus mode (open drain/push-pull) of a host.
>   */
> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
> index d76597c..1b022c9 100644
> --- a/drivers/mmc/core/core.h
> +++ b/drivers/mmc/core/core.h
> @@ -82,5 +82,8 @@ void mmc_add_card_debugfs(struct mmc_card *card);
>  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);
> +
>  #endif
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 02ad792..c712ba7 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1194,7 +1194,6 @@ EXPORT_SYMBOL(tuning_blk_pattern_8bit);
>  static int mmc_hs200_tuning(struct mmc_card *card)
>  {
>         struct mmc_host *host = card->host;
> -       int err = 0;
>
>         /*
>          * Timing should be adjusted to the HS400 target
> @@ -1205,18 +1204,7 @@ static int mmc_hs200_tuning(struct mmc_card *card)
>                 if (host->ops->prepare_hs400_tuning)
>                         host->ops->prepare_hs400_tuning(host, &host->ios);
>
> -       if (host->ops->execute_tuning) {
> -               mmc_host_clk_hold(host);
> -               err = host->ops->execute_tuning(host,
> -                               MMC_SEND_TUNING_BLOCK_HS200);
> -               mmc_host_clk_release(host);
> -
> -               if (err)
> -                       pr_err("%s: tuning execution failed\n",
> -                               mmc_hostname(host));
> -       }
> -
> -       return err;
> +       return mmc_execute_tuning(card);
>  }
>
>  /*
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index d90a6de..7907544 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -660,15 +660,10 @@ static int mmc_sd_init_uhs_card(struct mmc_card *card)
>          * SPI mode doesn't define CMD19 and tuning is only valid for SDR50 and
>          * SDR104 mode SD-cards. Note that tuning is mandatory for SDR104.
>          */
> -       if (!mmc_host_is_spi(card->host) && card->host->ops->execute_tuning &&
> -                       (card->sd_bus_speed == UHS_SDR50_BUS_SPEED ||
> -                        card->sd_bus_speed == UHS_SDR104_BUS_SPEED)) {
> -               mmc_host_clk_hold(card->host);
> -               err = card->host->ops->execute_tuning(card->host,
> -                                                     MMC_SEND_TUNING_BLOCK);
> -               mmc_host_clk_release(card->host);
> -       }
> -
> +       if (!mmc_host_is_spi(card->host) &&
> +           (card->sd_bus_speed == UHS_SDR50_BUS_SPEED ||
> +            card->sd_bus_speed == UHS_SDR104_BUS_SPEED))
> +               err = mmc_execute_tuning(card);
>  out:
>         kfree(status);
>
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index fd0750b..ce6cc47 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -567,17 +567,11 @@ static int mmc_sdio_init_uhs_card(struct mmc_card *card)
>          * SPI mode doesn't define CMD19 and tuning is only valid for SDR50 and
>          * SDR104 mode SD-cards. Note that tuning is mandatory for SDR104.
>          */
> -       if (!mmc_host_is_spi(card->host) && card->host->ops->execute_tuning &&
> -                       ((card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR50) ||
> -                        (card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR104))) {
> -               mmc_host_clk_hold(card->host);
> -               err = card->host->ops->execute_tuning(card->host,
> -                                                     MMC_SEND_TUNING_BLOCK);
> -               mmc_host_clk_release(card->host);
> -       }
> -
> +       if (!mmc_host_is_spi(card->host) &&
> +           ((card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR50) ||
> +            (card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR104)))
> +               err = mmc_execute_tuning(card);
>  out:
> -
>         return err;
>  }
>
> --
> 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 04/13] mmc: core: Move mmc_card_removed() into mmc_start_request()

[Ulf Hansson]

On 5 December 2014 at 18:41, Adrian Hunter <adrian.hunter@intel.com> wrote:
> Both callers of mmc_start_request() call mmc_card_removed()
> so move that call into mmc_start_request().
>
> This patch is preparation for adding re-tuning support.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>

Thanks! Applied for next.

Kind regards
Uffe

> ---
>  drivers/mmc/core/core.c | 32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index fcde4da..884f35a 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -185,13 +185,14 @@ 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)
> +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
> +       if (mmc_card_removed(host->card))
> +               return -ENOMEDIUM;
>
>         if (mrq->sbc) {
>                 pr_debug("<%s: starting CMD%u arg %08x flags %08x>\n",
> @@ -251,6 +252,8 @@ 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);
> +
> +       return 0;
>  }
>
>  /**
> @@ -345,29 +348,34 @@ static void mmc_wait_done(struct mmc_request *mrq)
>   */
>  static int __mmc_start_data_req(struct mmc_host *host, struct mmc_request *mrq)
>  {
> +       int err;
> +
>         mrq->done = mmc_wait_data_done;
>         mrq->host = host;
> -       if (mmc_card_removed(host->card)) {
> -               mrq->cmd->error = -ENOMEDIUM;
> +
> +       err = mmc_start_request(host, mrq);
> +       if (err) {
> +               mrq->cmd->error = err;
>                 mmc_wait_data_done(mrq);
> -               return -ENOMEDIUM;
>         }
> -       mmc_start_request(host, mrq);
>
> -       return 0;
> +       return err;
>  }
>
>  static int __mmc_start_req(struct mmc_host *host, struct mmc_request *mrq)
>  {
> +       int err;
> +
>         init_completion(&mrq->completion);
>         mrq->done = mmc_wait_done;
> -       if (mmc_card_removed(host->card)) {
> -               mrq->cmd->error = -ENOMEDIUM;
> +
> +       err = mmc_start_request(host, mrq);
> +       if (err) {
> +               mrq->cmd->error = err;
>                 complete(&mrq->completion);
> -               return -ENOMEDIUM;
>         }
> -       mmc_start_request(host, mrq);
> -       return 0;
> +
> +       return err;
>  }
>
>  /*
> --
> 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 12/13] mmc: sdhci: Always init buf_ready_int

[Ulf Hansson]

On 5 December 2014 at 18:41, Adrian Hunter <adrian.hunter@intel.com> wrote:
> There is no point making the initialization
> of buf_ready_int conditional on host version.
> Simplify by just doing it always. Note that
> the other conditional initializations will be
> removed when the new way of doing re-tuning
> is taken into use.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>

Thanks! Applied for next.

Kind regards
Uffe

> ---
>  drivers/mmc/host/sdhci.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index bfde165..dd4cc4c 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -3331,9 +3331,9 @@ int sdhci_add_host(struct sdhci_host *host)
>
>         setup_timer(&host->timer, sdhci_timeout_timer, (unsigned long)host);
>
> -       if (host->version >= SDHCI_SPEC_300) {
> -               init_waitqueue_head(&host->buf_ready_int);
> +       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;
> --
> 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 02/13] mmc: host: Add facility to support re-tuning

[Ulf Hansson]

Hi Adrian,

Thanks for working on this and apologize for my late reply!

On 5 December 2014 at 18:41, 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

I suggest we skip the support for the re-tuning timer in this initial
step and thus remove the related functionality from this patchset. It
adds complexity, but more important it's not obvious that it actually
will help. I am more concerned that it randomly will cause a request
latency and thus decrease performance.

The re-tuning period can't be selected "perfectly", so in this initial
step let's instead just rely on re-tune from the request retry path.

If we do see a need for a doing re-tuning periodically, how about
using the runtime PM suspend path (of the mmc card device). In that
way, we should be able to minimize the impact on performance.

Kind regards
Uffe

>
> To support those requirements 5 members are added to struct
> mmc_host:
>
>   unsigned int          can_retune:1;   /* re-tuning can be used */
>   unsigned int          doing_retune:1; /* re-tuning in progress */
>   int                   need_retune;    /* re-tuning is needed */
>   int                   hold_retune;    /* hold off re-tuning */
>   struct timer_list     retune_timer;   /* for periodic re-tuning */
>
> need_retune is an integer so it can be set without needing
> synchronization. hold_retune is a integer to allow nesting.
>
> Various simple functions are provided to set / clear those
> variables.
>
> Subsequent patches take those functions into use.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>

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

[Ulf Hansson]

On 5 December 2014 at 18:40, Adrian Hunter <adrian.hunter@intel.com> wrote:
> Hi
>
> Here are some patches to move re-tuning support out of
> sdhci and into the core, and add support for HS400
> re-tuning.
>
> Currently sdhci does re-tuning transparently by
> calling sdhci_execute_tuning() from its ->request()
> function.
>
> The problem with HS400 re-tuning is that it must be
> done in HS200 mode. That means using switch commands
> and making ios changes. That means it potentially
> conflicts with other command sequences. The new
> re-tuning support accomodates that, however it is
> not strictly necessary because HS400 re-tuning
> should only be needed after runtime suspend when
> there would not be a conflict.
>
> Nevertheless, this approach has more structure
> and potentially more flexibility.
>
>
> Adrian Hunter (13):
>       mmc: core: Simplify by adding mmc_execute_tuning()
>       mmc: host: Add facility to support re-tuning
>       mmc: core: Disable re-tuning when card is no longer initialized
>       mmc: core: Move mmc_card_removed() into mmc_start_request()
>       mmc: core: Add support for re-tuning before each request
>       mmc: core: Check re-tuning before retrying
>       mmc: core: Hold re-tuning during switch commands
>       mmc: core: Hold re-tuning during erase commands
>       mmc: core: Hold re-tuning while bkops ongoing
>       mmc: mmc: Comment that callers need to hold re-tuning if the card is put to sleep
>       mmc: core: Add support for HS400 re-tuning
>       mmc: sdhci: Always init buf_ready_int
>       mmc: sdhci: Change to new way of doing re-tuning

I have applied patch1, patch4, and patch12. Let's discuss more about
the re-tuning timer in the other thread for how to proceed with the
rest of the patches.

Kind regards
Uffe

>
>  drivers/mmc/core/core.c    |  93 +++++++++++++++++++++++++++++++-----
>  drivers/mmc/core/core.h    |   5 ++
>  drivers/mmc/core/host.c    |  63 +++++++++++++++++++++++++
>  drivers/mmc/core/mmc.c     |  58 ++++++++++++++++++-----
>  drivers/mmc/core/mmc_ops.c |  31 ++++++++----
>  drivers/mmc/core/sd.c      |  13 ++---
>  drivers/mmc/core/sdio.c    |  14 ++----
>  drivers/mmc/host/sdhci.c   | 115 ++++++---------------------------------------
>  include/linux/mmc/host.h   |  58 +++++++++++++++++++++++
>  include/linux/mmc/sdhci.h  |   3 --
>  10 files changed, 296 insertions(+), 157 deletions(-)
>
>
> Regards
> Adrian
>
> --
> 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 02/13] mmc: host: Add facility to support re-tuning

[Adrian Hunter]

On 13/01/15 13:25, Ulf Hansson wrote:
> Hi Adrian,
> 
> Thanks for working on this and apologize for my late reply!
> 
> On 5 December 2014 at 18:41, 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
> 
> I suggest we skip the support for the re-tuning timer in this initial
> step and thus remove the related functionality from this patchset. It
> adds complexity, but more important it's not obvious that it actually
> will help. I am more concerned that it randomly will cause a request
> latency and thus decrease performance.
> 
> The re-tuning period can't be selected "perfectly", so in this initial
> step let's instead just rely on re-tune from the request retry path.
> 
> If we do see a need for a doing re-tuning periodically, how about
> using the runtime PM suspend path (of the mmc card device). In that
> way, we should be able to minimize the impact on performance.

Thank you for looking at the patches.

I am not sure I know what you mean. sdhci already has a re-tuning timer, so
this is just moving it into core, where it won't be used by other drivers
unless they enable it.

I am not sure what you want to leave in sdhci.c and what you want in core,
if anything.

At a minimum I need sdhci to be able to switch from hs400 to hs200, re-tune,
and switch back.

Re: [PATCH 02/13] mmc: host: Add facility to support re-tuning

[Ulf Hansson]

On 13 January 2015 at 14:23, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 13/01/15 13:25, Ulf Hansson wrote:
>> Hi Adrian,
>>
>> Thanks for working on this and apologize for my late reply!
>>
>> On 5 December 2014 at 18:41, 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
>>
>> I suggest we skip the support for the re-tuning timer in this initial
>> step and thus remove the related functionality from this patchset. It
>> adds complexity, but more important it's not obvious that it actually
>> will help. I am more concerned that it randomly will cause a request
>> latency and thus decrease performance.
>>
>> The re-tuning period can't be selected "perfectly", so in this initial
>> step let's instead just rely on re-tune from the request retry path.
>>
>> If we do see a need for a doing re-tuning periodically, how about
>> using the runtime PM suspend path (of the mmc card device). In that
>> way, we should be able to minimize the impact on performance.
>
> Thank you for looking at the patches.
>
> I am not sure I know what you mean. sdhci already has a re-tuning timer, so
> this is just moving it into core, where it won't be used by other drivers
> unless they enable it.

I am kind of questioning the re-tuning timer in sdhci. What is it good for?

Can sdhci rely on that the mmc core performs a re-tune from the
request retry mechanism instead?

>
> I am not sure what you want to leave in sdhci.c and what you want in core,
> if anything.

We need all the infrastructure code in the core. Much like what your
patchset does. Except that I would like you to remove the option of
having a timer and the corresponding complexity it adds.

>
> At a minimum I need sdhci to be able to switch from hs400 to hs200, re-tune,
> and switch back.

As stated, I am only questioning the timer, nothing else.

Kind regards
Uffe

Re: [PATCH 02/13] mmc: host: Add facility to support re-tuning

[Adrian Hunter]

On 13/01/15 16:22, Ulf Hansson wrote:
> On 13 January 2015 at 14:23, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 13/01/15 13:25, Ulf Hansson wrote:
>>> Hi Adrian,
>>>
>>> Thanks for working on this and apologize for my late reply!
>>>
>>> On 5 December 2014 at 18:41, 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
>>>
>>> I suggest we skip the support for the re-tuning timer in this initial
>>> step and thus remove the related functionality from this patchset. It
>>> adds complexity, but more important it's not obvious that it actually
>>> will help. I am more concerned that it randomly will cause a request
>>> latency and thus decrease performance.
>>>
>>> The re-tuning period can't be selected "perfectly", so in this initial
>>> step let's instead just rely on re-tune from the request retry path.
>>>
>>> If we do see a need for a doing re-tuning periodically, how about
>>> using the runtime PM suspend path (of the mmc card device). In that
>>> way, we should be able to minimize the impact on performance.
>>
>> Thank you for looking at the patches.
>>
>> I am not sure I know what you mean. sdhci already has a re-tuning timer, so
>> this is just moving it into core, where it won't be used by other drivers
>> unless they enable it.
> 
> I am kind of questioning the re-tuning timer in sdhci. What is it good for?

It is part of the SD Host Controller Standard Specification. The timer
ensures that re-tuning is done before temperature changes could affect the
"sampling point". It is needed for re-tuning mode 1 for UHS-I modes like SDR104.

> 
> Can sdhci rely on that the mmc core performs a re-tune from the
> request retry mechanism instead?

Not according to the standard.

> 
>>
>> I am not sure what you want to leave in sdhci.c and what you want in core,
>> if anything.
> 
> We need all the infrastructure code in the core. Much like what your
> patchset does. Except that I would like you to remove the option of
> having a timer and the corresponding complexity it adds.

If we are going to follow the standard then that doesn't seem to be an option.

> 
>>
>> At a minimum I need sdhci to be able to switch from hs400 to hs200, re-tune,
>> and switch back.
> 
> As stated, I am only questioning the timer, nothing else.

Ok so how should I proceed?

Re: [PATCH 02/13] mmc: host: Add facility to support re-tuning

[Ulf Hansson]

[...]

>>> Thank you for looking at the patches.
>>>
>>> I am not sure I know what you mean. sdhci already has a re-tuning timer, so
>>> this is just moving it into core, where it won't be used by other drivers
>>> unless they enable it.
>>
>> I am kind of questioning the re-tuning timer in sdhci. What is it good for?
>
> It is part of the SD Host Controller Standard Specification. The timer
> ensures that re-tuning is done before temperature changes could affect the
> "sampling point". It is needed for re-tuning mode 1 for UHS-I modes like SDR104.

Does the spec say what value the timer should have?

>
>>
>> Can sdhci rely on that the mmc core performs a re-tune from the
>> request retry mechanism instead?
>
> Not according to the standard.

We don't have to implement everything that comes with the standard. We
can leave things out.

>
>>
>>>
>>> I am not sure what you want to leave in sdhci.c and what you want in core,
>>> if anything.
>>
>> We need all the infrastructure code in the core. Much like what your
>> patchset does. Except that I would like you to remove the option of
>> having a timer and the corresponding complexity it adds.
>
> If we are going to follow the standard then that doesn't seem to be an option.

I am not suggestion us to violating the spec. I am suggesting to
currently not support all of it.

>
>>
>>>
>>> At a minimum I need sdhci to be able to switch from hs400 to hs200, re-tune,
>>> and switch back.
>>
>> As stated, I am only questioning the timer, nothing else.
>
> Ok so how should I proceed?
>

As I stated, let's try without the timer first.

If we find it's not enough to recover at the request retry path, since
it happens too often - lets deal with that then.

Okay?

Kind regards
Uffe

Re: [PATCH 02/13] mmc: host: Add facility to support re-tuning

[Arend van Spriel]

On 01/13/15 15:22, Ulf Hansson wrote:
> On 13 January 2015 at 14:23, Adrian Hunter<adrian.hunter@intel.com>  wrote:
>> On 13/01/15 13:25, Ulf Hansson wrote:
>>> Hi Adrian,
>>>
>>> Thanks for working on this and apologize for my late reply!
>>>
>>> On 5 December 2014 at 18:41, 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
>>>
>>> I suggest we skip the support for the re-tuning timer in this initial
>>> step and thus remove the related functionality from this patchset. It
>>> adds complexity, but more important it's not obvious that it actually
>>> will help. I am more concerned that it randomly will cause a request
>>> latency and thus decrease performance.
>>>
>>> The re-tuning period can't be selected "perfectly", so in this initial
>>> step let's instead just rely on re-tune from the request retry path.
>>>
>>> If we do see a need for a doing re-tuning periodically, how about
>>> using the runtime PM suspend path (of the mmc card device). In that
>>> way, we should be able to minimize the impact on performance.
>>
>> Thank you for looking at the patches.
>>
>> I am not sure I know what you mean. sdhci already has a re-tuning timer, so
>> this is just moving it into core, where it won't be used by other drivers
>> unless they enable it.
>
> I am kind of questioning the re-tuning timer in sdhci. What is it good for?

It is good for complying with the SD Host controller spec. In section 
"2.2.25 Capabilities Register (pg. 74) the different "Re-Tuning Modes" 
are described. Guess these different should be taken into account in 
this patch series although in sdhci only the retuning timer was supported.

Regards,
Arend

> Can sdhci rely on that the mmc core performs a re-tune from the
> request retry mechanism instead?
>
>>
>> I am not sure what you want to leave in sdhci.c and what you want in core,
>> if anything.
>
> We need all the infrastructure code in the core. Much like what your
> patchset does. Except that I would like you to remove the option of
> having a timer and the corresponding complexity it adds.
>
>>
>> At a minimum I need sdhci to be able to switch from hs400 to hs200, re-tune,
>> and switch back.
>
> As stated, I am only questioning the timer, nothing else.
>
> Kind regards
> Uffe

Re: [PATCH 02/13] mmc: host: Add facility to support re-tuning

[Arend van Spriel]

On 01/13/15 15:56, Ulf Hansson wrote:
> [...]
>
>>>> Thank you for looking at the patches.
>>>>
>>>> I am not sure I know what you mean. sdhci already has a re-tuning timer, so
>>>> this is just moving it into core, where it won't be used by other drivers
>>>> unless they enable it.
>>>
>>> I am kind of questioning the re-tuning timer in sdhci. What is it good for?
>>
>> It is part of the SD Host Controller Standard Specification. The timer
>> ensures that re-tuning is done before temperature changes could affect the
>> "sampling point". It is needed for re-tuning mode 1 for UHS-I modes like SDR104.
>
> Does the spec say what value the timer should have?

It is read from the Capabilities register in the SD host controller, ie. 
in field "Timer Count for Re-Tuning" (see below).

Regards,
Arend

Timer Count for Re-Tuning
This field indicates an initial value of the Re-Tuning Timer for 
Re-Tuning Mode 1 to 3. Setting to 0 disables Re-Tuning Timer.
0h	Re-Tuning Timer disabled
1h	1 seconds
2h	2 seconds
3h	4 seconds
4h	8 seconds
.....	......................
n	2(n-1) seconds
.....	......................
Bh	1024 seconds
Eh - Ch	Reserved
Fh	Get information from other source

>>
>>>
>>> Can sdhci rely on that the mmc core performs a re-tune from the
>>> request retry mechanism instead?
>>
>> Not according to the standard.
>
> We don't have to implement everything that comes with the standard. We
> can leave things out.
>
>>
>>>
>>>>
>>>> I am not sure what you want to leave in sdhci.c and what you want in core,
>>>> if anything.
>>>
>>> We need all the infrastructure code in the core. Much like what your
>>> patchset does. Except that I would like you to remove the option of
>>> having a timer and the corresponding complexity it adds.
>>
>> If we are going to follow the standard then that doesn't seem to be an option.
>
> I am not suggestion us to violating the spec. I am suggesting to
> currently not support all of it.
>
>>
>>>
>>>>
>>>> At a minimum I need sdhci to be able to switch from hs400 to hs200, re-tune,
>>>> and switch back.
>>>
>>> As stated, I am only questioning the timer, nothing else.
>>
>> Ok so how should I proceed?
>>
>
> As I stated, let's try without the timer first.
>
> If we find it's not enough to recover at the request retry path, since
> it happens too often - lets deal with that then.
>
> Okay?
>
> Kind regards
> Uffe

Re: [PATCH 02/13] mmc: host: Add facility to support re-tuning

[Ulf Hansson]

On 13 January 2015 at 16:11, Arend van Spriel <arend@broadcom.com> wrote:
> On 01/13/15 15:56, Ulf Hansson wrote:
>>
>> [...]
>>
>>>>> Thank you for looking at the patches.
>>>>>
>>>>> I am not sure I know what you mean. sdhci already has a re-tuning
>>>>> timer, so
>>>>> this is just moving it into core, where it won't be used by other
>>>>> drivers
>>>>> unless they enable it.
>>>>
>>>>
>>>> I am kind of questioning the re-tuning timer in sdhci. What is it good
>>>> for?
>>>
>>>
>>> It is part of the SD Host Controller Standard Specification. The timer
>>> ensures that re-tuning is done before temperature changes could affect
>>> the
>>> "sampling point". It is needed for re-tuning mode 1 for UHS-I modes like
>>> SDR104.
>>
>>
>> Does the spec say what value the timer should have?
>
>
> It is read from the Capabilities register in the SD host controller, ie. in
> field "Timer Count for Re-Tuning" (see below).
>
> Regards,
> Arend
>
> Timer Count for Re-Tuning
> This field indicates an initial value of the Re-Tuning Timer for Re-Tuning
> Mode 1 to 3. Setting to 0 disables Re-Tuning Timer.
> 0h      Re-Tuning Timer disabled
> 1h      1 seconds
> 2h      2 seconds
> 3h      4 seconds
> 4h      8 seconds
> .....   ......................
> n       2(n-1) seconds
> .....   ......................
> Bh      1024 seconds
> Eh - Ch Reserved
> Fh      Get information from other source

Thanks for sharing this information, but unfortunate I don't
understand much from it.

Is the host driver intended to read/poll this register to find a good value?

Isn't heat one of the most important factor that could effect the need
for a re-tune? Does then the controller internally dynamically update
this register (since it keep track of heat or similar)?


Kind regards
Uffe

Re: [PATCH 02/13] mmc: host: Add facility to support re-tuning

[Arend van Spriel]

On 01/13/15 16:41, Ulf Hansson wrote:
> On 13 January 2015 at 16:11, Arend van Spriel<arend@broadcom.com>  wrote:
>> On 01/13/15 15:56, Ulf Hansson wrote:
>>>
>>> [...]
>>>
>>>>>> Thank you for looking at the patches.
>>>>>>
>>>>>> I am not sure I know what you mean. sdhci already has a re-tuning
>>>>>> timer, so
>>>>>> this is just moving it into core, where it won't be used by other
>>>>>> drivers
>>>>>> unless they enable it.
>>>>>
>>>>>
>>>>> I am kind of questioning the re-tuning timer in sdhci. What is it good
>>>>> for?
>>>>
>>>>
>>>> It is part of the SD Host Controller Standard Specification. The timer
>>>> ensures that re-tuning is done before temperature changes could affect
>>>> the
>>>> "sampling point". It is needed for re-tuning mode 1 for UHS-I modes like
>>>> SDR104.
>>>
>>>
>>> Does the spec say what value the timer should have?
>>
>>
>> It is read from the Capabilities register in the SD host controller, ie. in
>> field "Timer Count for Re-Tuning" (see below).
>>
>> Regards,
>> Arend
>>
>> Timer Count for Re-Tuning
>> This field indicates an initial value of the Re-Tuning Timer for Re-Tuning
>> Mode 1 to 3. Setting to 0 disables Re-Tuning Timer.
>> 0h      Re-Tuning Timer disabled
>> 1h      1 seconds
>> 2h      2 seconds
>> 3h      4 seconds
>> 4h      8 seconds
>> .....   ......................
>> n       2(n-1) seconds
>> .....   ......................
>> Bh      1024 seconds
>> Eh - Ch Reserved
>> Fh      Get information from other source
>
> Thanks for sharing this information, but unfortunate I don't
> understand much from it.
>
> Is the host driver intended to read/poll this register to find a good value?

You can download the spec (and others) here [1]. sdhci currently 
implements retuning mode 1, which is decribed in the spec:

Re-Tuning Timer Control Example for Re-Tuning Mode 1
The initial value of re-tuning timer is provided by Timer Count for 
Re-Tuning field in this register. The timer starts counting by loading 
the initial value. When the timer expires, the Host Driver marks an 
expiration flag. On receiving a command request, the Host driver checks 
the expiration flag. If the expiration flag is set, then the Host Driver 
should perform the re-tuning procedure before issuing a command. If the 
expiration flag is not set, then the Host Driver issues a command 
without performing the re-tuning procedure. Every time the re-tuning 
procedure is performed, the timer loads the new initial value and the 
expiration flag is cleared.

So the host controller could indeed update this register for subsequent 
retuning.

Regards,
Arend

[1] https://www.sdcard.org/downloads/pls/

> Isn't heat one of the most important factor that could effect the need
> for a re-tune? Does then the controller internally dynamically update
> this register (since it keep track of heat or similar)?

>
> Kind regards
> Uffe

Re: [PATCH 02/13] mmc: host: Add facility to support re-tuning

[Ulf Hansson]

On 13 January 2015 at 17:02, Arend van Spriel <arend@broadcom.com> wrote:
> On 01/13/15 16:41, Ulf Hansson wrote:
>>
>> On 13 January 2015 at 16:11, Arend van Spriel<arend@broadcom.com>  wrote:
>>>
>>> On 01/13/15 15:56, Ulf Hansson wrote:
>>>>
>>>>
>>>> [...]
>>>>
>>>>>>> Thank you for looking at the patches.
>>>>>>>
>>>>>>> I am not sure I know what you mean. sdhci already has a re-tuning
>>>>>>> timer, so
>>>>>>> this is just moving it into core, where it won't be used by other
>>>>>>> drivers
>>>>>>> unless they enable it.
>>>>>>
>>>>>>
>>>>>>
>>>>>> I am kind of questioning the re-tuning timer in sdhci. What is it good
>>>>>> for?
>>>>>
>>>>>
>>>>>
>>>>> It is part of the SD Host Controller Standard Specification. The timer
>>>>> ensures that re-tuning is done before temperature changes could affect
>>>>> the
>>>>> "sampling point". It is needed for re-tuning mode 1 for UHS-I modes
>>>>> like
>>>>> SDR104.
>>>>
>>>>
>>>>
>>>> Does the spec say what value the timer should have?
>>>
>>>
>>>
>>> It is read from the Capabilities register in the SD host controller, ie.
>>> in
>>> field "Timer Count for Re-Tuning" (see below).
>>>
>>> Regards,
>>> Arend
>>>
>>> Timer Count for Re-Tuning
>>> This field indicates an initial value of the Re-Tuning Timer for
>>> Re-Tuning
>>> Mode 1 to 3. Setting to 0 disables Re-Tuning Timer.
>>> 0h      Re-Tuning Timer disabled
>>> 1h      1 seconds
>>> 2h      2 seconds
>>> 3h      4 seconds
>>> 4h      8 seconds
>>> .....   ......................
>>> n       2(n-1) seconds
>>> .....   ......................
>>> Bh      1024 seconds
>>> Eh - Ch Reserved
>>> Fh      Get information from other source
>>
>>
>> Thanks for sharing this information, but unfortunate I don't
>> understand much from it.
>>
>> Is the host driver intended to read/poll this register to find a good
>> value?
>
>
> You can download the spec (and others) here [1]. sdhci currently implements
> retuning mode 1, which is decribed in the spec:
>
> Re-Tuning Timer Control Example for Re-Tuning Mode 1
> The initial value of re-tuning timer is provided by Timer Count for
> Re-Tuning field in this register. The timer starts counting by loading the
> initial value. When the timer expires, the Host Driver marks an expiration
> flag. On receiving a command request, the Host driver checks the expiration
> flag. If the expiration flag is set, then the Host Driver should perform the
> re-tuning procedure before issuing a command. If the expiration flag is not
> set, then the Host Driver issues a command without performing the re-tuning
> procedure. Every time the re-tuning procedure is performed, the timer loads
> the new initial value and the expiration flag is cleared.
>
> So the host controller could indeed update this register for subsequent
> retuning.

Arend, thanks for the link and information. So, I decided to go for a
look in there.

>From the same section you quoted above:
------
(1) Re-Tuning Mode 1
The host controller does not have any internal logic to detect when
the re-tuning needs to be performed. In this case, the Host Driver
should maintain all re-tuning timings by using a Re-Tuning Timer. To
enable inserting the re-tuning procedure during data transfers, the
data length per read/write command shall be limited up to 4MB.
------

That means, we can't get _any_ help from the controller HW (in mode 1)
to find a good value for the timer. It simply says that it's
recommended to do a periodic re-tuning at some times, which is also
stated by the SD and eMMC specs.

Thus, to find a decent value for the timer, the mmc core would have to
collect statistics for how data is read/written to the card to
anticipate the heat. I don't think that's an effort that justifies its
need.

That leaves us with these options:
1) Use a timer with a random selected value.
2) Perform a re-tune at runtime PM suspend or resume (of the mmc card).
3) While catching request errors (like CRC), perform a re-tune in the
request retry path.

Now, since we don't have any statistics available for how often a
re-tuning actually would be needed, let's first try out option 3) to
see if that's enough.

Kind regards
Uffe

Re: [PATCH 02/13] mmc: host: Add facility to support re-tuning

[Adrian Hunter]

On 14/01/15 11:47, Ulf Hansson wrote:
> On 13 January 2015 at 17:02, Arend van Spriel <arend@broadcom.com> wrote:
>> On 01/13/15 16:41, Ulf Hansson wrote:
>>>
>>> On 13 January 2015 at 16:11, Arend van Spriel<arend@broadcom.com>  wrote:
>>>>
>>>> On 01/13/15 15:56, Ulf Hansson wrote:
>>>>>
>>>>>
>>>>> [...]
>>>>>
>>>>>>>> Thank you for looking at the patches.
>>>>>>>>
>>>>>>>> I am not sure I know what you mean. sdhci already has a re-tuning
>>>>>>>> timer, so
>>>>>>>> this is just moving it into core, where it won't be used by other
>>>>>>>> drivers
>>>>>>>> unless they enable it.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> I am kind of questioning the re-tuning timer in sdhci. What is it good
>>>>>>> for?
>>>>>>
>>>>>>
>>>>>>
>>>>>> It is part of the SD Host Controller Standard Specification. The timer
>>>>>> ensures that re-tuning is done before temperature changes could affect
>>>>>> the
>>>>>> "sampling point". It is needed for re-tuning mode 1 for UHS-I modes
>>>>>> like
>>>>>> SDR104.
>>>>>
>>>>>
>>>>>
>>>>> Does the spec say what value the timer should have?
>>>>
>>>>
>>>>
>>>> It is read from the Capabilities register in the SD host controller, ie.
>>>> in
>>>> field "Timer Count for Re-Tuning" (see below).
>>>>
>>>> Regards,
>>>> Arend
>>>>
>>>> Timer Count for Re-Tuning
>>>> This field indicates an initial value of the Re-Tuning Timer for
>>>> Re-Tuning
>>>> Mode 1 to 3. Setting to 0 disables Re-Tuning Timer.
>>>> 0h      Re-Tuning Timer disabled
>>>> 1h      1 seconds
>>>> 2h      2 seconds
>>>> 3h      4 seconds
>>>> 4h      8 seconds
>>>> .....   ......................
>>>> n       2(n-1) seconds
>>>> .....   ......................
>>>> Bh      1024 seconds
>>>> Eh - Ch Reserved
>>>> Fh      Get information from other source
>>>
>>>
>>> Thanks for sharing this information, but unfortunate I don't
>>> understand much from it.
>>>
>>> Is the host driver intended to read/poll this register to find a good
>>> value?
>>
>>
>> You can download the spec (and others) here [1]. sdhci currently implements
>> retuning mode 1, which is decribed in the spec:
>>
>> Re-Tuning Timer Control Example for Re-Tuning Mode 1
>> The initial value of re-tuning timer is provided by Timer Count for
>> Re-Tuning field in this register. The timer starts counting by loading the
>> initial value. When the timer expires, the Host Driver marks an expiration
>> flag. On receiving a command request, the Host driver checks the expiration
>> flag. If the expiration flag is set, then the Host Driver should perform the
>> re-tuning procedure before issuing a command. If the expiration flag is not
>> set, then the Host Driver issues a command without performing the re-tuning
>> procedure. Every time the re-tuning procedure is performed, the timer loads
>> the new initial value and the expiration flag is cleared.
>>
>> So the host controller could indeed update this register for subsequent
>> retuning.
> 
> Arend, thanks for the link and information. So, I decided to go for a
> look in there.
> 
>>From the same section you quoted above:
> ------
> (1) Re-Tuning Mode 1
> The host controller does not have any internal logic to detect when
> the re-tuning needs to be performed. In this case, the Host Driver
> should maintain all re-tuning timings by using a Re-Tuning Timer. To
> enable inserting the re-tuning procedure during data transfers, the
> data length per read/write command shall be limited up to 4MB.
> ------
> 
> That means, we can't get _any_ help from the controller HW (in mode 1)
> to find a good value for the timer.

In fact the timer value *is* defined in the Capabilities Register (Offset
040h) bits 43-40 Timer Count for Re-Tuning

It has been supported since 2011, see:

	commit cf2b5eea1ea0ff9b3184bc6771bcb93a9fdcd1d9
	"mmc: sdhci: add support for retuning mode 1"

Re: [PATCH 02/13] mmc: host: Add facility to support re-tuning

[Ulf Hansson]

On 14 January 2015 at 10:57, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 14/01/15 11:47, Ulf Hansson wrote:
>> On 13 January 2015 at 17:02, Arend van Spriel <arend@broadcom.com> wrote:
>>> On 01/13/15 16:41, Ulf Hansson wrote:
>>>>
>>>> On 13 January 2015 at 16:11, Arend van Spriel<arend@broadcom.com>  wrote:
>>>>>
>>>>> On 01/13/15 15:56, Ulf Hansson wrote:
>>>>>>
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>>>> Thank you for looking at the patches.
>>>>>>>>>
>>>>>>>>> I am not sure I know what you mean. sdhci already has a re-tuning
>>>>>>>>> timer, so
>>>>>>>>> this is just moving it into core, where it won't be used by other
>>>>>>>>> drivers
>>>>>>>>> unless they enable it.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> I am kind of questioning the re-tuning timer in sdhci. What is it good
>>>>>>>> for?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> It is part of the SD Host Controller Standard Specification. The timer
>>>>>>> ensures that re-tuning is done before temperature changes could affect
>>>>>>> the
>>>>>>> "sampling point". It is needed for re-tuning mode 1 for UHS-I modes
>>>>>>> like
>>>>>>> SDR104.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Does the spec say what value the timer should have?
>>>>>
>>>>>
>>>>>
>>>>> It is read from the Capabilities register in the SD host controller, ie.
>>>>> in
>>>>> field "Timer Count for Re-Tuning" (see below).
>>>>>
>>>>> Regards,
>>>>> Arend
>>>>>
>>>>> Timer Count for Re-Tuning
>>>>> This field indicates an initial value of the Re-Tuning Timer for
>>>>> Re-Tuning
>>>>> Mode 1 to 3. Setting to 0 disables Re-Tuning Timer.
>>>>> 0h      Re-Tuning Timer disabled
>>>>> 1h      1 seconds
>>>>> 2h      2 seconds
>>>>> 3h      4 seconds
>>>>> 4h      8 seconds
>>>>> .....   ......................
>>>>> n       2(n-1) seconds
>>>>> .....   ......................
>>>>> Bh      1024 seconds
>>>>> Eh - Ch Reserved
>>>>> Fh      Get information from other source
>>>>
>>>>
>>>> Thanks for sharing this information, but unfortunate I don't
>>>> understand much from it.
>>>>
>>>> Is the host driver intended to read/poll this register to find a good
>>>> value?
>>>
>>>
>>> You can download the spec (and others) here [1]. sdhci currently implements
>>> retuning mode 1, which is decribed in the spec:
>>>
>>> Re-Tuning Timer Control Example for Re-Tuning Mode 1
>>> The initial value of re-tuning timer is provided by Timer Count for
>>> Re-Tuning field in this register. The timer starts counting by loading the
>>> initial value. When the timer expires, the Host Driver marks an expiration
>>> flag. On receiving a command request, the Host driver checks the expiration
>>> flag. If the expiration flag is set, then the Host Driver should perform the
>>> re-tuning procedure before issuing a command. If the expiration flag is not
>>> set, then the Host Driver issues a command without performing the re-tuning
>>> procedure. Every time the re-tuning procedure is performed, the timer loads
>>> the new initial value and the expiration flag is cleared.
>>>
>>> So the host controller could indeed update this register for subsequent
>>> retuning.
>>
>> Arend, thanks for the link and information. So, I decided to go for a
>> look in there.
>>
>>>From the same section you quoted above:
>> ------
>> (1) Re-Tuning Mode 1
>> The host controller does not have any internal logic to detect when
>> the re-tuning needs to be performed. In this case, the Host Driver
>> should maintain all re-tuning timings by using a Re-Tuning Timer. To
>> enable inserting the re-tuning procedure during data transfers, the
>> data length per read/write command shall be limited up to 4MB.
>> ------
>>
>> That means, we can't get _any_ help from the controller HW (in mode 1)
>> to find a good value for the timer.
>
> In fact the timer value *is* defined in the Capabilities Register (Offset
> 040h) bits 43-40 Timer Count for Re-Tuning
>
> It has been supported since 2011, see:
>
>         commit cf2b5eea1ea0ff9b3184bc6771bcb93a9fdcd1d9
>         "mmc: sdhci: add support for retuning mode 1"
>

The value from the register is also just randomly selected, only
difference is that it's the HW that has randomly set it.

Even if the above commit was merged, I don't think it was the correct
way of dealing with re-tuning.

First of all, re-tuning this is a mmc protocol specific thing should
be managed from the mmc core, like the approach you have taken in your
$subject patchset. Second I question whether the timer is useful at
all.

Kind regards
Uffe

Re: [PATCH 02/13] mmc: host: Add facility to support re-tuning

[Adrian Hunter]

On 14/01/15 12:13, Ulf Hansson wrote:
> On 14 January 2015 at 10:57, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 14/01/15 11:47, Ulf Hansson wrote:
>>> On 13 January 2015 at 17:02, Arend van Spriel <arend@broadcom.com> wrote:
>>>> On 01/13/15 16:41, Ulf Hansson wrote:
>>>>>
>>>>> On 13 January 2015 at 16:11, Arend van Spriel<arend@broadcom.com>  wrote:
>>>>>>
>>>>>> On 01/13/15 15:56, Ulf Hansson wrote:
>>>>>>>
>>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>>>>> Thank you for looking at the patches.
>>>>>>>>>>
>>>>>>>>>> I am not sure I know what you mean. sdhci already has a re-tuning
>>>>>>>>>> timer, so
>>>>>>>>>> this is just moving it into core, where it won't be used by other
>>>>>>>>>> drivers
>>>>>>>>>> unless they enable it.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I am kind of questioning the re-tuning timer in sdhci. What is it good
>>>>>>>>> for?
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> It is part of the SD Host Controller Standard Specification. The timer
>>>>>>>> ensures that re-tuning is done before temperature changes could affect
>>>>>>>> the
>>>>>>>> "sampling point". It is needed for re-tuning mode 1 for UHS-I modes
>>>>>>>> like
>>>>>>>> SDR104.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Does the spec say what value the timer should have?
>>>>>>
>>>>>>
>>>>>>
>>>>>> It is read from the Capabilities register in the SD host controller, ie.
>>>>>> in
>>>>>> field "Timer Count for Re-Tuning" (see below).
>>>>>>
>>>>>> Regards,
>>>>>> Arend
>>>>>>
>>>>>> Timer Count for Re-Tuning
>>>>>> This field indicates an initial value of the Re-Tuning Timer for
>>>>>> Re-Tuning
>>>>>> Mode 1 to 3. Setting to 0 disables Re-Tuning Timer.
>>>>>> 0h      Re-Tuning Timer disabled
>>>>>> 1h      1 seconds
>>>>>> 2h      2 seconds
>>>>>> 3h      4 seconds
>>>>>> 4h      8 seconds
>>>>>> .....   ......................
>>>>>> n       2(n-1) seconds
>>>>>> .....   ......................
>>>>>> Bh      1024 seconds
>>>>>> Eh - Ch Reserved
>>>>>> Fh      Get information from other source
>>>>>
>>>>>
>>>>> Thanks for sharing this information, but unfortunate I don't
>>>>> understand much from it.
>>>>>
>>>>> Is the host driver intended to read/poll this register to find a good
>>>>> value?
>>>>
>>>>
>>>> You can download the spec (and others) here [1]. sdhci currently implements
>>>> retuning mode 1, which is decribed in the spec:
>>>>
>>>> Re-Tuning Timer Control Example for Re-Tuning Mode 1
>>>> The initial value of re-tuning timer is provided by Timer Count for
>>>> Re-Tuning field in this register. The timer starts counting by loading the
>>>> initial value. When the timer expires, the Host Driver marks an expiration
>>>> flag. On receiving a command request, the Host driver checks the expiration
>>>> flag. If the expiration flag is set, then the Host Driver should perform the
>>>> re-tuning procedure before issuing a command. If the expiration flag is not
>>>> set, then the Host Driver issues a command without performing the re-tuning
>>>> procedure. Every time the re-tuning procedure is performed, the timer loads
>>>> the new initial value and the expiration flag is cleared.
>>>>
>>>> So the host controller could indeed update this register for subsequent
>>>> retuning.
>>>
>>> Arend, thanks for the link and information. So, I decided to go for a
>>> look in there.
>>>
>>> >From the same section you quoted above:
>>> ------
>>> (1) Re-Tuning Mode 1
>>> The host controller does not have any internal logic to detect when
>>> the re-tuning needs to be performed. In this case, the Host Driver
>>> should maintain all re-tuning timings by using a Re-Tuning Timer. To
>>> enable inserting the re-tuning procedure during data transfers, the
>>> data length per read/write command shall be limited up to 4MB.
>>> ------
>>>
>>> That means, we can't get _any_ help from the controller HW (in mode 1)
>>> to find a good value for the timer.
>>
>> In fact the timer value *is* defined in the Capabilities Register (Offset
>> 040h) bits 43-40 Timer Count for Re-Tuning
>>
>> It has been supported since 2011, see:
>>
>>         commit cf2b5eea1ea0ff9b3184bc6771bcb93a9fdcd1d9
>>         "mmc: sdhci: add support for retuning mode 1"
>>
> 
> The value from the register is also just randomly selected, only
> difference is that it's the HW that has randomly set it.

Presumably the value is chosen based on the maximum rate of temperature
change and the corresponding effect that has on the signal.

> 
> Even if the above commit was merged, I don't think it was the correct
> way of dealing with re-tuning.
> 
> First of all, re-tuning this is a mmc protocol specific thing should
> be managed from the mmc core, like the approach you have taken in your
> $subject patchset. Second I question whether the timer is useful at
> all.

The SD Host Controller Specification does not document another way to do
mode 1 re-tuning. The timer is it. Otherwise re-tuning is never done.

In the patches I sent, the driver must call mmc_retune_needed() to set
host->need_retune = 1 otherwise mmc_retune() does nothing.

I would like to extend the model to include transparently re-tuning and
re-trying when there is a CRC error, but that is a separate issue, not
documented in the spec but recommended by others.

Re: [PATCH 02/13] mmc: host: Add facility to support re-tuning

[Arend van Spriel]

On 01/14/15 11:13, Ulf Hansson wrote:
> On 14 January 2015 at 10:57, Adrian Hunter<adrian.hunter@intel.com>  wrote:
>> On 14/01/15 11:47, Ulf Hansson wrote:
>>> On 13 January 2015 at 17:02, Arend van Spriel<arend@broadcom.com>  wrote:
>>>> On 01/13/15 16:41, Ulf Hansson wrote:
>>>>>
>>>>> On 13 January 2015 at 16:11, Arend van Spriel<arend@broadcom.com>   wrote:
>>>>>>
>>>>>> On 01/13/15 15:56, Ulf Hansson wrote:
>>>>>>>
>>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>>>>> Thank you for looking at the patches.
>>>>>>>>>>
>>>>>>>>>> I am not sure I know what you mean. sdhci already has a re-tuning
>>>>>>>>>> timer, so
>>>>>>>>>> this is just moving it into core, where it won't be used by other
>>>>>>>>>> drivers
>>>>>>>>>> unless they enable it.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I am kind of questioning the re-tuning timer in sdhci. What is it good
>>>>>>>>> for?
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> It is part of the SD Host Controller Standard Specification. The timer
>>>>>>>> ensures that re-tuning is done before temperature changes could affect
>>>>>>>> the
>>>>>>>> "sampling point". It is needed for re-tuning mode 1 for UHS-I modes
>>>>>>>> like
>>>>>>>> SDR104.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Does the spec say what value the timer should have?
>>>>>>
>>>>>>
>>>>>>
>>>>>> It is read from the Capabilities register in the SD host controller, ie.
>>>>>> in
>>>>>> field "Timer Count for Re-Tuning" (see below).
>>>>>>
>>>>>> Regards,
>>>>>> Arend
>>>>>>
>>>>>> Timer Count for Re-Tuning
>>>>>> This field indicates an initial value of the Re-Tuning Timer for
>>>>>> Re-Tuning
>>>>>> Mode 1 to 3. Setting to 0 disables Re-Tuning Timer.
>>>>>> 0h      Re-Tuning Timer disabled
>>>>>> 1h      1 seconds
>>>>>> 2h      2 seconds
>>>>>> 3h      4 seconds
>>>>>> 4h      8 seconds
>>>>>> .....   ......................
>>>>>> n       2(n-1) seconds
>>>>>> .....   ......................
>>>>>> Bh      1024 seconds
>>>>>> Eh - Ch Reserved
>>>>>> Fh      Get information from other source
>>>>>
>>>>>
>>>>> Thanks for sharing this information, but unfortunate I don't
>>>>> understand much from it.
>>>>>
>>>>> Is the host driver intended to read/poll this register to find a good
>>>>> value?
>>>>
>>>>
>>>> You can download the spec (and others) here [1]. sdhci currently implements
>>>> retuning mode 1, which is decribed in the spec:
>>>>
>>>> Re-Tuning Timer Control Example for Re-Tuning Mode 1
>>>> The initial value of re-tuning timer is provided by Timer Count for
>>>> Re-Tuning field in this register. The timer starts counting by loading the
>>>> initial value. When the timer expires, the Host Driver marks an expiration
>>>> flag. On receiving a command request, the Host driver checks the expiration
>>>> flag. If the expiration flag is set, then the Host Driver should perform the
>>>> re-tuning procedure before issuing a command. If the expiration flag is not
>>>> set, then the Host Driver issues a command without performing the re-tuning
>>>> procedure. Every time the re-tuning procedure is performed, the timer loads
>>>> the new initial value and the expiration flag is cleared.
>>>>
>>>> So the host controller could indeed update this register for subsequent
>>>> retuning.
>>>
>>> Arend, thanks for the link and information. So, I decided to go for a
>>> look in there.
>>>
>>> > From the same section you quoted above:
>>> ------
>>> (1) Re-Tuning Mode 1
>>> The host controller does not have any internal logic to detect when
>>> the re-tuning needs to be performed. In this case, the Host Driver
>>> should maintain all re-tuning timings by using a Re-Tuning Timer. To
>>> enable inserting the re-tuning procedure during data transfers, the
>>> data length per read/write command shall be limited up to 4MB.
>>> ------

Hi Ulf,

After sending my email I read that part as well and figured my response 
was incorrect.

>>> That means, we can't get _any_ help from the controller HW (in mode 1)
>>> to find a good value for the timer.
>>
>> In fact the timer value *is* defined in the Capabilities Register (Offset
>> 040h) bits 43-40 Timer Count for Re-Tuning
>>
>> It has been supported since 2011, see:
>>
>>          commit cf2b5eea1ea0ff9b3184bc6771bcb93a9fdcd1d9
>>          "mmc: sdhci: add support for retuning mode 1"
>>
>
> The value from the register is also just randomly selected, only
> difference is that it's the HW that has randomly set it.

I think you can not say it like that. The value from the register is set 
by the manufacturer of the host controller. I would not say they would 
set that randomly. It is just hard-coded in their IP design. Now whether 
the value comes from actual hardware validation is hard to say.

> Even if the above commit was merged, I don't think it was the correct
> way of dealing with re-tuning.

It seems a reasonable choice to follow the specification.

> First of all, re-tuning this is a mmc protocol specific thing should
> be managed from the mmc core, like the approach you have taken in your
> $subject patchset. Second I question whether the timer is useful at
> all.

Not sure I understand what the alternative approach is here. You 
mentioned earlier something about "the request retry path". Does that 
mean you proposal is to only do a re-tuning procedure when a request 
fails. That does not seem like "the correct way of dealing with 
re-tuning" either as it introduces additional delay of the failed 
request. I would rather see some algorithm to adapt the timer value and 
thus keep a re-tuning timer. If you are concerned about doing 
unnecessary re-tuning cycles retuning could be limited to ADTC request 
as from what I understand about retuning is that it is only needed for 
requests that involve using the DAT lines.

Regards,
Arend

> Kind regards
> Uffe

Re: [PATCH 02/13] mmc: host: Add facility to support re-tuning

[Ulf Hansson]

[...]

>
> Hi Ulf,
>
> After sending my email I read that part as well and figured my response was
> incorrect.
>
>>>> That means, we can't get _any_ help from the controller HW (in mode 1)
>>>> to find a good value for the timer.
>>>
>>>
>>> In fact the timer value *is* defined in the Capabilities Register (Offset
>>> 040h) bits 43-40 Timer Count for Re-Tuning
>>>
>>> It has been supported since 2011, see:
>>>
>>>          commit cf2b5eea1ea0ff9b3184bc6771bcb93a9fdcd1d9
>>>          "mmc: sdhci: add support for retuning mode 1"
>>>
>>
>> The value from the register is also just randomly selected, only
>> difference is that it's the HW that has randomly set it.
>
>
> I think you can not say it like that. The value from the register is set by
> the manufacturer of the host controller. I would not say they would set that
> randomly. It is just hard-coded in their IP design. Now whether the value
> comes from actual hardware validation is hard to say.

How can they pre-validate use-cases? They don't have any clue of how
the card is going to be exercised on a real SOC. It can't be more than
just a guess.

>
>> Even if the above commit was merged, I don't think it was the correct
>> way of dealing with re-tuning.
>
>
> It seems a reasonable choice to follow the specification.
>
>> First of all, re-tuning this is a mmc protocol specific thing should
>> be managed from the mmc core, like the approach you have taken in your
>> $subject patchset. Second I question whether the timer is useful at
>> all.
>
>
> Not sure I understand what the alternative approach is here. You mentioned
> earlier something about "the request retry path". Does that mean you
> proposal is to only do a re-tuning procedure when a request fails.

Correct. It actually already implemented as part of $subject patchset.

> That does
> not seem like "the correct way of dealing with re-tuning" either as it
> introduces additional delay of the failed request. I would rather see some
> algorithm to adapt the timer value and thus keep a re-tuning timer. If you
> are concerned about doing unnecessary re-tuning cycles retuning could be
> limited to ADTC request as from what I understand about retuning is that it
> is only needed for requests that involve using the DAT lines.

It will introduce a delay/latency, but only when it's actually needed
to do a re-tune.

With the timer, it will add a latency at a random point in time,
depending on the selected value for it.
More importantly, user the timer means we will potentially insert
latencies, when in fact the re-tune wasn't needed.

Kind regards
Uffe

Re: [PATCH 02/13] mmc: host: Add facility to support re-tuning

[Ulf Hansson]

[...]

>>
>> The value from the register is also just randomly selected, only
>> difference is that it's the HW that has randomly set it.
>
> Presumably the value is chosen based on the maximum rate of temperature
> change and the corresponding effect that has on the signal.
>
>>
>> Even if the above commit was merged, I don't think it was the correct
>> way of dealing with re-tuning.
>>
>> First of all, re-tuning this is a mmc protocol specific thing should
>> be managed from the mmc core, like the approach you have taken in your
>> $subject patchset. Second I question whether the timer is useful at
>> all.
>
> The SD Host Controller Specification does not document another way to do
> mode 1 re-tuning. The timer is it. Otherwise re-tuning is never done.
>
> In the patches I sent, the driver must call mmc_retune_needed() to set
> host->need_retune = 1 otherwise mmc_retune() does nothing.
>
> I would like to extend the model to include transparently re-tuning and
> re-trying when there is a CRC error, but that is a separate issue, not
> documented in the spec but recommended by others.
>

That perfect and in line from what I heard as recommendations from
memory vendors as well.

Now, can we stop arguing about the timer and try without it?

If we do see a need for a more frequent re-tuning to happen, due to
that we get lots of CRC errors to recover from, then I think we should
look into using runtime PM instead of the timer. And that's because I
want to minimize the impact on performance.

Kind regards
Uffe

Re: [PATCH 02/13] mmc: host: Add facility to support re-tuning

[Adrian Hunter]

On 14/01/15 14:59, Ulf Hansson wrote:
> [...]
> 
>>>
>>> The value from the register is also just randomly selected, only
>>> difference is that it's the HW that has randomly set it.
>>
>> Presumably the value is chosen based on the maximum rate of temperature
>> change and the corresponding effect that has on the signal.
>>
>>>
>>> Even if the above commit was merged, I don't think it was the correct
>>> way of dealing with re-tuning.
>>>
>>> First of all, re-tuning this is a mmc protocol specific thing should
>>> be managed from the mmc core, like the approach you have taken in your
>>> $subject patchset. Second I question whether the timer is useful at
>>> all.
>>
>> The SD Host Controller Specification does not document another way to do
>> mode 1 re-tuning. The timer is it. Otherwise re-tuning is never done.
>>
>> In the patches I sent, the driver must call mmc_retune_needed() to set
>> host->need_retune = 1 otherwise mmc_retune() does nothing.
>>
>> I would like to extend the model to include transparently re-tuning and
>> re-trying when there is a CRC error, but that is a separate issue, not
>> documented in the spec but recommended by others.
>>
> 
> That perfect and in line from what I heard as recommendations from
> memory vendors as well.

How would that work for SDIO? How do you know it is OK to retry SDIO operations?

> 
> Now, can we stop arguing about the timer and try without it?
> 
> If we do see a need for a more frequent re-tuning to happen, due to
> that we get lots of CRC errors to recover from, then I think we should
> look into using runtime PM instead of the timer. And that's because I
> want to minimize the impact on performance.

The minimum timer value is 1 second. The maximum is 1024 seconds. The ASUS
T100TA had a timer value of 128 seconds. The timer is not a performance issue.

There is a performance question with runtime PM because that happens far
more frequently (typical auto-suspend delay is 50ms) and we re-tune after
that. In fact I generalized that a bit in patch 13.

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

	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.

One option to reduce the impact of the latency would be to increase the
auto-suspend delay.

I have cc'ed the author of "mmc: sdhci: add support for retuning mode 1"

Re: [PATCH 02/13] mmc: host: Add facility to support re-tuning

[Ulf Hansson]

On 15 January 2015 at 11:17, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 14/01/15 14:59, Ulf Hansson wrote:
>> [...]
>>
>>>>
>>>> The value from the register is also just randomly selected, only
>>>> difference is that it's the HW that has randomly set it.
>>>
>>> Presumably the value is chosen based on the maximum rate of temperature
>>> change and the corresponding effect that has on the signal.
>>>
>>>>
>>>> Even if the above commit was merged, I don't think it was the correct
>>>> way of dealing with re-tuning.
>>>>
>>>> First of all, re-tuning this is a mmc protocol specific thing should
>>>> be managed from the mmc core, like the approach you have taken in your
>>>> $subject patchset. Second I question whether the timer is useful at
>>>> all.
>>>
>>> The SD Host Controller Specification does not document another way to do
>>> mode 1 re-tuning. The timer is it. Otherwise re-tuning is never done.
>>>
>>> In the patches I sent, the driver must call mmc_retune_needed() to set
>>> host->need_retune = 1 otherwise mmc_retune() does nothing.
>>>
>>> I would like to extend the model to include transparently re-tuning and
>>> re-trying when there is a CRC error, but that is a separate issue, not
>>> documented in the spec but recommended by others.
>>>
>>
>> That perfect and in line from what I heard as recommendations from
>> memory vendors as well.
>
> How would that work for SDIO? How do you know it is OK to retry SDIO operations?

Retries or error handling, needs to be handled from SDIO func drivers
or upper level code. They certainly also need it for other errors,
which are not caused by the lack of a re-tune. I believe they exist
already.

For mmc core point of view, we need to act on SDIO data transfers
errors and perform re-tuning for cases when it makes sense.

More importantly, using a timer won't make SDIO data transfers error
free, since we can still end up needing a re-tune at any point.

Still, you do have point for SDIO. Minimizing the number of errors for
SDIO could be important, due to that an SDIO func driver may not be
able to recover from data errors as smoothly as the mmc block layer
can. Thus, a timer could help to improve the situation, but I think it
only makes sense in the SDIO case.

BTW, what's your experience around SDIO cards supporting SDR104. I
have never used such, have you?

>
>>
>> Now, can we stop arguing about the timer and try without it?
>>
>> If we do see a need for a more frequent re-tuning to happen, due to
>> that we get lots of CRC errors to recover from, then I think we should
>> look into using runtime PM instead of the timer. And that's because I
>> want to minimize the impact on performance.
>
> The minimum timer value is 1 second. The maximum is 1024 seconds. The ASUS
> T100TA had a timer value of 128 seconds. The timer is not a performance issue.
>
> There is a performance question with runtime PM because that happens far
> more frequently (typical auto-suspend delay is 50ms) and we re-tune after
> that. In fact I generalized that a bit in patch 13.
>
>         [PATCH 13/13] mmc: sdhci: Change to new way of doing re-tuning
>
>         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.
>
> One option to reduce the impact of the latency would be to increase the
> auto-suspend delay.

The latency will affect the first request after a runtime PM
suspend/resume cycle. So for continues data transfers the impact
should be zero. Also, increasing the delay would impact power
consumption, but it's a balance I guess. :-)

This is a specific issue for SDHCI (it's not clear to me if all SDHCI
variants have the same behaviour). Obviously the mmc core needs to
support the demand from SDHCI, such enable it to tell the core to
perform a re-tune. Exactly what your patchset does.

For your reference, I know about other controllers which can restore a
bunch of register values, saved from earlier re-tunings, from its
runtime PM resume callbacks. Thus preventing a re-tuning from happen.
I wonder if some of the SDHCI variant are capable of this as well.


Kind regards
Uffe

Re: [PATCH 02/13] mmc: host: Add facility to support re-tuning

[Arend van Spriel]

On 01/15/15 14:39, Ulf Hansson wrote:
> On 15 January 2015 at 11:17, Adrian Hunter<adrian.hunter@intel.com>  wrote:
>> On 14/01/15 14:59, Ulf Hansson wrote:
>>> [...]
>>>
>>>>>
>>>>> The value from the register is also just randomly selected, only
>>>>> difference is that it's the HW that has randomly set it.
>>>>
>>>> Presumably the value is chosen based on the maximum rate of temperature
>>>> change and the corresponding effect that has on the signal.
>>>>
>>>>>
>>>>> Even if the above commit was merged, I don't think it was the correct
>>>>> way of dealing with re-tuning.
>>>>>
>>>>> First of all, re-tuning this is a mmc protocol specific thing should
>>>>> be managed from the mmc core, like the approach you have taken in your
>>>>> $subject patchset. Second I question whether the timer is useful at
>>>>> all.
>>>>
>>>> The SD Host Controller Specification does not document another way to do
>>>> mode 1 re-tuning. The timer is it. Otherwise re-tuning is never done.
>>>>
>>>> In the patches I sent, the driver must call mmc_retune_needed() to set
>>>> host->need_retune = 1 otherwise mmc_retune() does nothing.
>>>>
>>>> I would like to extend the model to include transparently re-tuning and
>>>> re-trying when there is a CRC error, but that is a separate issue, not
>>>> documented in the spec but recommended by others.
>>>>
>>>
>>> That perfect and in line from what I heard as recommendations from
>>> memory vendors as well.
>>
>> How would that work for SDIO? How do you know it is OK to retry SDIO operations?
>
> Retries or error handling, needs to be handled from SDIO func drivers
> or upper level code. They certainly also need it for other errors,
> which are not caused by the lack of a re-tune. I believe they exist
> already.
>
> For mmc core point of view, we need to act on SDIO data transfers
> errors and perform re-tuning for cases when it makes sense.
>
> More importantly, using a timer won't make SDIO data transfers error
> free, since we can still end up needing a re-tune at any point.
>
> Still, you do have point for SDIO. Minimizing the number of errors for
> SDIO could be important, due to that an SDIO func driver may not be
> able to recover from data errors as smoothly as the mmc block layer
> can. Thus, a timer could help to improve the situation, but I think it
> only makes sense in the SDIO case.
>
> BTW, what's your experience around SDIO cards supporting SDR104. I
> have never used such, have you?

My primary focus in all this discussing is about SDIO cards. The main 
reason being that our 11ac wifi SDIO cards do support SDR104. So the 
brcmfmac driver support SDIO and has retry mechanisms in place. However, 
it may also end-up doing an abort under certain conditions.

You also mentioned using runtime-pm, but how do you deal with func 
drivers not supporting runtime-pm. That is already an issue aka. bug 
right now. Our driver does not support runtime-pm (yet) and we have 
reported issues that host controller does runtime-pm basically killing 
communication between device and func driver.

Gr. AvS

>>
>>>
>>> Now, can we stop arguing about the timer and try without it?
>>>
>>> If we do see a need for a more frequent re-tuning to happen, due to
>>> that we get lots of CRC errors to recover from, then I think we should
>>> look into using runtime PM instead of the timer. And that's because I
>>> want to minimize the impact on performance.
>>
>> The minimum timer value is 1 second. The maximum is 1024 seconds. The ASUS
>> T100TA had a timer value of 128 seconds. The timer is not a performance issue.
>>
>> There is a performance question with runtime PM because that happens far
>> more frequently (typical auto-suspend delay is 50ms) and we re-tune after
>> that. In fact I generalized that a bit in patch 13.
>>
>>          [PATCH 13/13] mmc: sdhci: Change to new way of doing re-tuning
>>
>>          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.
>>
>> One option to reduce the impact of the latency would be to increase the
>> auto-suspend delay.
>
> The latency will affect the first request after a runtime PM
> suspend/resume cycle. So for continues data transfers the impact
> should be zero. Also, increasing the delay would impact power
> consumption, but it's a balance I guess. :-)
>
> This is a specific issue for SDHCI (it's not clear to me if all SDHCI
> variants have the same behaviour). Obviously the mmc core needs to
> support the demand from SDHCI, such enable it to tell the core to
> perform a re-tune. Exactly what your patchset does.
>
> For your reference, I know about other controllers which can restore a
> bunch of register values, saved from earlier re-tunings, from its
> runtime PM resume callbacks. Thus preventing a re-tuning from happen.
> I wonder if some of the SDHCI variant are capable of this as well.
>
>
> Kind regards
> Uffe

Re: [PATCH 02/13] mmc: host: Add facility to support re-tuning

[Arend van Spriel]

On 01/15/15 15:07, Arend van Spriel wrote:
> On 01/15/15 14:39, Ulf Hansson wrote:
>> On 15 January 2015 at 11:17, Adrian Hunter<adrian.hunter@intel.com>
>> wrote:
>>> On 14/01/15 14:59, Ulf Hansson wrote:
>>>> [...]
>>>>
>>>>>>
>>>>>> The value from the register is also just randomly selected, only
>>>>>> difference is that it's the HW that has randomly set it.
>>>>>
>>>>> Presumably the value is chosen based on the maximum rate of
>>>>> temperature
>>>>> change and the corresponding effect that has on the signal.
>>>>>
>>>>>>
>>>>>> Even if the above commit was merged, I don't think it was the correct
>>>>>> way of dealing with re-tuning.
>>>>>>
>>>>>> First of all, re-tuning this is a mmc protocol specific thing should
>>>>>> be managed from the mmc core, like the approach you have taken in
>>>>>> your
>>>>>> $subject patchset. Second I question whether the timer is useful at
>>>>>> all.
>>>>>
>>>>> The SD Host Controller Specification does not document another way
>>>>> to do
>>>>> mode 1 re-tuning. The timer is it. Otherwise re-tuning is never done.
>>>>>
>>>>> In the patches I sent, the driver must call mmc_retune_needed() to set
>>>>> host->need_retune = 1 otherwise mmc_retune() does nothing.
>>>>>
>>>>> I would like to extend the model to include transparently re-tuning
>>>>> and
>>>>> re-trying when there is a CRC error, but that is a separate issue, not
>>>>> documented in the spec but recommended by others.
>>>>>
>>>>
>>>> That perfect and in line from what I heard as recommendations from
>>>> memory vendors as well.
>>>
>>> How would that work for SDIO? How do you know it is OK to retry SDIO
>>> operations?
>>
>> Retries or error handling, needs to be handled from SDIO func drivers
>> or upper level code. They certainly also need it for other errors,
>> which are not caused by the lack of a re-tune. I believe they exist
>> already.
>>
>> For mmc core point of view, we need to act on SDIO data transfers
>> errors and perform re-tuning for cases when it makes sense.
>>
>> More importantly, using a timer won't make SDIO data transfers error
>> free, since we can still end up needing a re-tune at any point.
>>
>> Still, you do have point for SDIO. Minimizing the number of errors for
>> SDIO could be important, due to that an SDIO func driver may not be
>> able to recover from data errors as smoothly as the mmc block layer
>> can. Thus, a timer could help to improve the situation, but I think it
>> only makes sense in the SDIO case.
>>
>> BTW, what's your experience around SDIO cards supporting SDR104. I
>> have never used such, have you?
>
> My primary focus in all this discussing is about SDIO cards. The main
> reason being that our 11ac wifi SDIO cards do support SDR104. So the
> brcmfmac driver support SDIO and has retry mechanisms in place. However,
> it may also end-up doing an abort under certain conditions.
>
> You also mentioned using runtime-pm, but how do you deal with func
> drivers not supporting runtime-pm. That is already an issue aka. bug
> right now. Our driver does not support runtime-pm (yet) and we have
> reported issues that host controller does runtime-pm basically killing
> communication between device and func driver.

Could leave it to the function driver to call mmc_retune_needed().

Regards,
Arend

> Gr. AvS
>
>>>
>>>>
>>>> Now, can we stop arguing about the timer and try without it?
>>>>
>>>> If we do see a need for a more frequent re-tuning to happen, due to
>>>> that we get lots of CRC errors to recover from, then I think we should
>>>> look into using runtime PM instead of the timer. And that's because I
>>>> want to minimize the impact on performance.
>>>
>>> The minimum timer value is 1 second. The maximum is 1024 seconds. The
>>> ASUS
>>> T100TA had a timer value of 128 seconds. The timer is not a
>>> performance issue.
>>>
>>> There is a performance question with runtime PM because that happens far
>>> more frequently (typical auto-suspend delay is 50ms) and we re-tune
>>> after
>>> that. In fact I generalized that a bit in patch 13.
>>>
>>> [PATCH 13/13] mmc: sdhci: Change to new way of doing re-tuning
>>>
>>> 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.
>>>
>>> One option to reduce the impact of the latency would be to increase the
>>> auto-suspend delay.
>>
>> The latency will affect the first request after a runtime PM
>> suspend/resume cycle. So for continues data transfers the impact
>> should be zero. Also, increasing the delay would impact power
>> consumption, but it's a balance I guess. :-)
>>
>> This is a specific issue for SDHCI (it's not clear to me if all SDHCI
>> variants have the same behaviour). Obviously the mmc core needs to
>> support the demand from SDHCI, such enable it to tell the core to
>> perform a re-tune. Exactly what your patchset does.
>>
>> For your reference, I know about other controllers which can restore a
>> bunch of register values, saved from earlier re-tunings, from its
>> runtime PM resume callbacks. Thus preventing a re-tuning from happen.
>> I wonder if some of the SDHCI variant are capable of this as well.
>>
>>
>> Kind regards
>> Uffe
>

Re: [PATCH 02/13] mmc: host: Add facility to support re-tuning

[Ulf Hansson]

On 15 January 2015 at 15:17, Arend van Spriel <arend@broadcom.com> wrote:
> On 01/15/15 15:07, Arend van Spriel wrote:
>>
>> On 01/15/15 14:39, Ulf Hansson wrote:
>>>
>>> On 15 January 2015 at 11:17, Adrian Hunter<adrian.hunter@intel.com>
>>> wrote:
>>>>
>>>> On 14/01/15 14:59, Ulf Hansson wrote:
>>>>>
>>>>> [...]
>>>>>
>>>>>>>
>>>>>>> The value from the register is also just randomly selected, only
>>>>>>> difference is that it's the HW that has randomly set it.
>>>>>>
>>>>>>
>>>>>> Presumably the value is chosen based on the maximum rate of
>>>>>> temperature
>>>>>> change and the corresponding effect that has on the signal.
>>>>>>
>>>>>>>
>>>>>>> Even if the above commit was merged, I don't think it was the correct
>>>>>>> way of dealing with re-tuning.
>>>>>>>
>>>>>>> First of all, re-tuning this is a mmc protocol specific thing should
>>>>>>> be managed from the mmc core, like the approach you have taken in
>>>>>>> your
>>>>>>> $subject patchset. Second I question whether the timer is useful at
>>>>>>> all.
>>>>>>
>>>>>>
>>>>>> The SD Host Controller Specification does not document another way
>>>>>> to do
>>>>>> mode 1 re-tuning. The timer is it. Otherwise re-tuning is never done.
>>>>>>
>>>>>> In the patches I sent, the driver must call mmc_retune_needed() to set
>>>>>> host->need_retune = 1 otherwise mmc_retune() does nothing.
>>>>>>
>>>>>> I would like to extend the model to include transparently re-tuning
>>>>>> and
>>>>>> re-trying when there is a CRC error, but that is a separate issue, not
>>>>>> documented in the spec but recommended by others.
>>>>>>
>>>>>
>>>>> That perfect and in line from what I heard as recommendations from
>>>>> memory vendors as well.
>>>>
>>>>
>>>> How would that work for SDIO? How do you know it is OK to retry SDIO
>>>> operations?
>>>
>>>
>>> Retries or error handling, needs to be handled from SDIO func drivers
>>> or upper level code. They certainly also need it for other errors,
>>> which are not caused by the lack of a re-tune. I believe they exist
>>> already.
>>>
>>> For mmc core point of view, we need to act on SDIO data transfers
>>> errors and perform re-tuning for cases when it makes sense.
>>>
>>> More importantly, using a timer won't make SDIO data transfers error
>>> free, since we can still end up needing a re-tune at any point.
>>>
>>> Still, you do have point for SDIO. Minimizing the number of errors for
>>> SDIO could be important, due to that an SDIO func driver may not be
>>> able to recover from data errors as smoothly as the mmc block layer
>>> can. Thus, a timer could help to improve the situation, but I think it
>>> only makes sense in the SDIO case.
>>>
>>> BTW, what's your experience around SDIO cards supporting SDR104. I
>>> have never used such, have you?
>>
>>
>> My primary focus in all this discussing is about SDIO cards. The main
>> reason being that our 11ac wifi SDIO cards do support SDR104. So the
>> brcmfmac driver support SDIO and has retry mechanisms in place. However,
>> it may also end-up doing an abort under certain conditions.
>>
>> You also mentioned using runtime-pm, but how do you deal with func
>> drivers not supporting runtime-pm. That is already an issue aka. bug
>> right now. Our driver does not support runtime-pm (yet) and we have
>> reported issues that host controller does runtime-pm basically killing
>> communication between device and func driver.
>

Runtime PM is implemented a bit differently between SDIO vs MMC/SD.
Your are right.

For MMC/SD the mmc block device handles pm_runtime_get|put() in
principle per request basis and makes use of the
pm_runtime_autosuspend feature. While in the SDIO case, it's entirely
up the SDIO func driver to deal with pm_runtime_get|put().

So it seems like we can use runtime PM for MMC/SD but not for SDIO. At
least not using the SDIO func device.

>
> Could leave it to the function driver to call mmc_retune_needed().

Hmm, the positive side from such approach would be that the SDIO func
driver can decide when it's convenient to do a re-tune.

The negative side is that all SDIO func driver would need to care
about this. I am not sure we want that.

Kind regards
Uffe

Re: [PATCH 02/13] mmc: host: Add facility to support re-tuning

[Arend van Spriel]

On 01/15/15 15:46, Ulf Hansson wrote:
> On 15 January 2015 at 15:17, Arend van Spriel<arend@broadcom.com>  wrote:
>> On 01/15/15 15:07, Arend van Spriel wrote:
>>>
>>> On 01/15/15 14:39, Ulf Hansson wrote:
>>>>
>>>> On 15 January 2015 at 11:17, Adrian Hunter<adrian.hunter@intel.com>
>>>> wrote:
>>>>>
>>>>> On 14/01/15 14:59, Ulf Hansson wrote:
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>>>
>>>>>>>> The value from the register is also just randomly selected, only
>>>>>>>> difference is that it's the HW that has randomly set it.
>>>>>>>
>>>>>>>
>>>>>>> Presumably the value is chosen based on the maximum rate of
>>>>>>> temperature
>>>>>>> change and the corresponding effect that has on the signal.
>>>>>>>
>>>>>>>>
>>>>>>>> Even if the above commit was merged, I don't think it was the correct
>>>>>>>> way of dealing with re-tuning.
>>>>>>>>
>>>>>>>> First of all, re-tuning this is a mmc protocol specific thing should
>>>>>>>> be managed from the mmc core, like the approach you have taken in
>>>>>>>> your
>>>>>>>> $subject patchset. Second I question whether the timer is useful at
>>>>>>>> all.
>>>>>>>
>>>>>>>
>>>>>>> The SD Host Controller Specification does not document another way
>>>>>>> to do
>>>>>>> mode 1 re-tuning. The timer is it. Otherwise re-tuning is never done.
>>>>>>>
>>>>>>> In the patches I sent, the driver must call mmc_retune_needed() to set
>>>>>>> host->need_retune = 1 otherwise mmc_retune() does nothing.
>>>>>>>
>>>>>>> I would like to extend the model to include transparently re-tuning
>>>>>>> and
>>>>>>> re-trying when there is a CRC error, but that is a separate issue, not
>>>>>>> documented in the spec but recommended by others.
>>>>>>>
>>>>>>
>>>>>> That perfect and in line from what I heard as recommendations from
>>>>>> memory vendors as well.
>>>>>
>>>>>
>>>>> How would that work for SDIO? How do you know it is OK to retry SDIO
>>>>> operations?
>>>>
>>>>
>>>> Retries or error handling, needs to be handled from SDIO func drivers
>>>> or upper level code. They certainly also need it for other errors,
>>>> which are not caused by the lack of a re-tune. I believe they exist
>>>> already.
>>>>
>>>> For mmc core point of view, we need to act on SDIO data transfers
>>>> errors and perform re-tuning for cases when it makes sense.
>>>>
>>>> More importantly, using a timer won't make SDIO data transfers error
>>>> free, since we can still end up needing a re-tune at any point.
>>>>
>>>> Still, you do have point for SDIO. Minimizing the number of errors for
>>>> SDIO could be important, due to that an SDIO func driver may not be
>>>> able to recover from data errors as smoothly as the mmc block layer
>>>> can. Thus, a timer could help to improve the situation, but I think it
>>>> only makes sense in the SDIO case.
>>>>
>>>> BTW, what's your experience around SDIO cards supporting SDR104. I
>>>> have never used such, have you?
>>>
>>>
>>> My primary focus in all this discussing is about SDIO cards. The main
>>> reason being that our 11ac wifi SDIO cards do support SDR104. So the
>>> brcmfmac driver support SDIO and has retry mechanisms in place. However,
>>> it may also end-up doing an abort under certain conditions.
>>>
>>> You also mentioned using runtime-pm, but how do you deal with func
>>> drivers not supporting runtime-pm. That is already an issue aka. bug
>>> right now. Our driver does not support runtime-pm (yet) and we have
>>> reported issues that host controller does runtime-pm basically killing
>>> communication between device and func driver.
>>
>
> Runtime PM is implemented a bit differently between SDIO vs MMC/SD.
> Your are right.
>
> For MMC/SD the mmc block device handles pm_runtime_get|put() in
> principle per request basis and makes use of the
> pm_runtime_autosuspend feature. While in the SDIO case, it's entirely
> up the SDIO func driver to deal with pm_runtime_get|put().
>
> So it seems like we can use runtime PM for MMC/SD but not for SDIO. At
> least not using the SDIO func device.
>
>>
>> Could leave it to the function driver to call mmc_retune_needed().
>
> Hmm, the positive side from such approach would be that the SDIO func
> driver can decide when it's convenient to do a re-tune.

I would say "appropriate" instead of "convenient".

> The negative side is that all SDIO func driver would need to care
> about this. I am not sure we want that.

The whole retry handling also seems deferred to the SDIO func driver and 
the same for runtime-pm. As the "retune needed" question would pops up 
during the retry handling it seems not a bad option.

Regards,
Arend

> Kind regards
> Uffe

Re: [PATCH 02/13] mmc: host: Add facility to support re-tuning

[Ulf Hansson]

On 15 January 2015 at 15:59, Arend van Spriel <arend@broadcom.com> wrote:
> On 01/15/15 15:46, Ulf Hansson wrote:
>>
>> On 15 January 2015 at 15:17, Arend van Spriel<arend@broadcom.com>  wrote:
>>>
>>> On 01/15/15 15:07, Arend van Spriel wrote:
>>>>
>>>>
>>>> On 01/15/15 14:39, Ulf Hansson wrote:
>>>>>
>>>>>
>>>>> On 15 January 2015 at 11:17, Adrian Hunter<adrian.hunter@intel.com>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> On 14/01/15 14:59, Ulf Hansson wrote:
>>>>>>>
>>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>>>>
>>>>>>>>> The value from the register is also just randomly selected, only
>>>>>>>>> difference is that it's the HW that has randomly set it.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Presumably the value is chosen based on the maximum rate of
>>>>>>>> temperature
>>>>>>>> change and the corresponding effect that has on the signal.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Even if the above commit was merged, I don't think it was the
>>>>>>>>> correct
>>>>>>>>> way of dealing with re-tuning.
>>>>>>>>>
>>>>>>>>> First of all, re-tuning this is a mmc protocol specific thing
>>>>>>>>> should
>>>>>>>>> be managed from the mmc core, like the approach you have taken in
>>>>>>>>> your
>>>>>>>>> $subject patchset. Second I question whether the timer is useful at
>>>>>>>>> all.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> The SD Host Controller Specification does not document another way
>>>>>>>> to do
>>>>>>>> mode 1 re-tuning. The timer is it. Otherwise re-tuning is never
>>>>>>>> done.
>>>>>>>>
>>>>>>>> In the patches I sent, the driver must call mmc_retune_needed() to
>>>>>>>> set
>>>>>>>> host->need_retune = 1 otherwise mmc_retune() does nothing.
>>>>>>>>
>>>>>>>> I would like to extend the model to include transparently re-tuning
>>>>>>>> and
>>>>>>>> re-trying when there is a CRC error, but that is a separate issue,
>>>>>>>> not
>>>>>>>> documented in the spec but recommended by others.
>>>>>>>>
>>>>>>>
>>>>>>> That perfect and in line from what I heard as recommendations from
>>>>>>> memory vendors as well.
>>>>>>
>>>>>>
>>>>>>
>>>>>> How would that work for SDIO? How do you know it is OK to retry SDIO
>>>>>> operations?
>>>>>
>>>>>
>>>>>
>>>>> Retries or error handling, needs to be handled from SDIO func drivers
>>>>> or upper level code. They certainly also need it for other errors,
>>>>> which are not caused by the lack of a re-tune. I believe they exist
>>>>> already.
>>>>>
>>>>> For mmc core point of view, we need to act on SDIO data transfers
>>>>> errors and perform re-tuning for cases when it makes sense.
>>>>>
>>>>> More importantly, using a timer won't make SDIO data transfers error
>>>>> free, since we can still end up needing a re-tune at any point.
>>>>>
>>>>> Still, you do have point for SDIO. Minimizing the number of errors for
>>>>> SDIO could be important, due to that an SDIO func driver may not be
>>>>> able to recover from data errors as smoothly as the mmc block layer
>>>>> can. Thus, a timer could help to improve the situation, but I think it
>>>>> only makes sense in the SDIO case.
>>>>>
>>>>> BTW, what's your experience around SDIO cards supporting SDR104. I
>>>>> have never used such, have you?
>>>>
>>>>
>>>>
>>>> My primary focus in all this discussing is about SDIO cards. The main
>>>> reason being that our 11ac wifi SDIO cards do support SDR104. So the
>>>> brcmfmac driver support SDIO and has retry mechanisms in place. However,
>>>> it may also end-up doing an abort under certain conditions.
>>>>
>>>> You also mentioned using runtime-pm, but how do you deal with func
>>>> drivers not supporting runtime-pm. That is already an issue aka. bug
>>>> right now. Our driver does not support runtime-pm (yet) and we have
>>>> reported issues that host controller does runtime-pm basically killing
>>>> communication between device and func driver.
>>>
>>>
>>
>> Runtime PM is implemented a bit differently between SDIO vs MMC/SD.
>> Your are right.
>>
>> For MMC/SD the mmc block device handles pm_runtime_get|put() in
>> principle per request basis and makes use of the
>> pm_runtime_autosuspend feature. While in the SDIO case, it's entirely
>> up the SDIO func driver to deal with pm_runtime_get|put().
>>
>> So it seems like we can use runtime PM for MMC/SD but not for SDIO. At
>> least not using the SDIO func device.
>>
>>>
>>> Could leave it to the function driver to call mmc_retune_needed().
>>
>>
>> Hmm, the positive side from such approach would be that the SDIO func
>> driver can decide when it's convenient to do a re-tune.
>
>
> I would say "appropriate" instead of "convenient".
>
>> The negative side is that all SDIO func driver would need to care
>> about this. I am not sure we want that.
>
>
> The whole retry handling also seems deferred to the SDIO func driver and the
> same for runtime-pm. As the "retune needed" question would pops up during
> the retry handling it seems not a bad option.

Okay, your argument seems reasonable, let's got for this approach.

Kind regards
Uffe

Re: [PATCH 02/13] mmc: host: Add facility to support re-tuning

[Adrian Hunter]

On 19/01/15 11:27, Ulf Hansson wrote:
> On 15 January 2015 at 15:59, Arend van Spriel <arend@broadcom.com> wrote:
>> On 01/15/15 15:46, Ulf Hansson wrote:
>>>
>>> On 15 January 2015 at 15:17, Arend van Spriel<arend@broadcom.com>  wrote:
>>>>
>>>> On 01/15/15 15:07, Arend van Spriel wrote:
>>>>>
>>>>>
>>>>> On 01/15/15 14:39, Ulf Hansson wrote:
>>>>>>
>>>>>>
>>>>>> On 15 January 2015 at 11:17, Adrian Hunter<adrian.hunter@intel.com>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 14/01/15 14:59, Ulf Hansson wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> [...]
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The value from the register is also just randomly selected, only
>>>>>>>>>> difference is that it's the HW that has randomly set it.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Presumably the value is chosen based on the maximum rate of
>>>>>>>>> temperature
>>>>>>>>> change and the corresponding effect that has on the signal.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Even if the above commit was merged, I don't think it was the
>>>>>>>>>> correct
>>>>>>>>>> way of dealing with re-tuning.
>>>>>>>>>>
>>>>>>>>>> First of all, re-tuning this is a mmc protocol specific thing
>>>>>>>>>> should
>>>>>>>>>> be managed from the mmc core, like the approach you have taken in
>>>>>>>>>> your
>>>>>>>>>> $subject patchset. Second I question whether the timer is useful at
>>>>>>>>>> all.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> The SD Host Controller Specification does not document another way
>>>>>>>>> to do
>>>>>>>>> mode 1 re-tuning. The timer is it. Otherwise re-tuning is never
>>>>>>>>> done.
>>>>>>>>>
>>>>>>>>> In the patches I sent, the driver must call mmc_retune_needed() to
>>>>>>>>> set
>>>>>>>>> host->need_retune = 1 otherwise mmc_retune() does nothing.
>>>>>>>>>
>>>>>>>>> I would like to extend the model to include transparently re-tuning
>>>>>>>>> and
>>>>>>>>> re-trying when there is a CRC error, but that is a separate issue,
>>>>>>>>> not
>>>>>>>>> documented in the spec but recommended by others.
>>>>>>>>>
>>>>>>>>
>>>>>>>> That perfect and in line from what I heard as recommendations from
>>>>>>>> memory vendors as well.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> How would that work for SDIO? How do you know it is OK to retry SDIO
>>>>>>> operations?
>>>>>>
>>>>>>
>>>>>>
>>>>>> Retries or error handling, needs to be handled from SDIO func drivers
>>>>>> or upper level code. They certainly also need it for other errors,
>>>>>> which are not caused by the lack of a re-tune. I believe they exist
>>>>>> already.
>>>>>>
>>>>>> For mmc core point of view, we need to act on SDIO data transfers
>>>>>> errors and perform re-tuning for cases when it makes sense.
>>>>>>
>>>>>> More importantly, using a timer won't make SDIO data transfers error
>>>>>> free, since we can still end up needing a re-tune at any point.
>>>>>>
>>>>>> Still, you do have point for SDIO. Minimizing the number of errors for
>>>>>> SDIO could be important, due to that an SDIO func driver may not be
>>>>>> able to recover from data errors as smoothly as the mmc block layer
>>>>>> can. Thus, a timer could help to improve the situation, but I think it
>>>>>> only makes sense in the SDIO case.
>>>>>>
>>>>>> BTW, what's your experience around SDIO cards supporting SDR104. I
>>>>>> have never used such, have you?
>>>>>
>>>>>
>>>>>
>>>>> My primary focus in all this discussing is about SDIO cards. The main
>>>>> reason being that our 11ac wifi SDIO cards do support SDR104. So the
>>>>> brcmfmac driver support SDIO and has retry mechanisms in place. However,
>>>>> it may also end-up doing an abort under certain conditions.
>>>>>
>>>>> You also mentioned using runtime-pm, but how do you deal with func
>>>>> drivers not supporting runtime-pm. That is already an issue aka. bug
>>>>> right now. Our driver does not support runtime-pm (yet) and we have
>>>>> reported issues that host controller does runtime-pm basically killing
>>>>> communication between device and func driver.
>>>>
>>>>
>>>
>>> Runtime PM is implemented a bit differently between SDIO vs MMC/SD.
>>> Your are right.
>>>
>>> For MMC/SD the mmc block device handles pm_runtime_get|put() in
>>> principle per request basis and makes use of the
>>> pm_runtime_autosuspend feature. While in the SDIO case, it's entirely
>>> up the SDIO func driver to deal with pm_runtime_get|put().
>>>
>>> So it seems like we can use runtime PM for MMC/SD but not for SDIO. At
>>> least not using the SDIO func device.
>>>
>>>>
>>>> Could leave it to the function driver to call mmc_retune_needed().
>>>
>>>
>>> Hmm, the positive side from such approach would be that the SDIO func
>>> driver can decide when it's convenient to do a re-tune.
>>
>>
>> I would say "appropriate" instead of "convenient".
>>
>>> The negative side is that all SDIO func driver would need to care
>>> about this. I am not sure we want that.
>>
>>
>> The whole retry handling also seems deferred to the SDIO func driver and the
>> same for runtime-pm. As the "retune needed" question would pops up during
>> the retry handling it seems not a bad option.
> 
> Okay, your argument seems reasonable, let's got for this approach.

A re-tune is needed when there is a CRC error. That should be a low level
decision because it is needed no matter if it is a SDIO function driver or
eMMC block driver. There is a mechanism to hold-off re-tuning if it would
cause a problem. Otherwise re-tuning should be done immediately. Remember we
are already in an error condition, which must be rare to non-existent for
the device to perform reasonably.

The decision of the upper layers is when to retry.

My thought for the block driver was that it would indicate that it was ok to
transparently re-try if re-tuning was needed. Then the core would do the
re-try. A complication is the need to retry just once not get stuck
error->retry->error->retry->...