Re: [PATCH v4 2/4] mmc: tmio: Add tuning support

[Simon Horman <horms@verge.net.au>]

On Fri, Aug 26, 2016 at 10:01:35AM +0200, Ulf Hansson wrote:
> On 25 August 2016 at 14:04, Simon Horman <horms@verge.net.au> wrote:
> > On Tue, Aug 23, 2016 at 05:02:56PM +0200, Ulf Hansson wrote:

...

> >> >> I am wondering whether it would it be possible to keep a cache of the
> >> >> currently used tuning values and then restore these values at
> >> >> runtime_resume?
> >> >>
> >> >> In that way you wouldn't need to force new re-tuning cycle here.
> >> >
> >> > I will check with the hardware people to see if it is practical from
> >> > that POV.
> >>
> >> Okay!
> >
> > The feedback I received is something like this:
> >
> > * The tap values calculated during retuning depend on the temperature of
> >   the SoC and card.
> > * So after resume the values may be invalid.
> 
> They values may also become invalid during long continues transfers,
> which prevents the ->runtime_suspend() callback to be invoked -
> because the temperature may increase.
> 
> > * It may be possible to re-use the TAP values and re-tune if a transfer
> >   fails but the people I spoke with were unsure of the merit of that
> >   approach
> 
> There's is a huge benefit!
> 
> You will get a significant decreased latency at ->runtime_resume() as
> you don't need to run a complete re-tuning cycle.
> 
> I can't really give you fresh numbers as it's a long time I tried this
> myself, although if you did some measurements on this it would be
> nice! :-)
> 
> >
> > Personally my feeling is that we should initiate a retune on resume for
> > now as that seems to be the safest option.
> 
> I don't agree. :-) I think it's better to postpone re-tune until it's
> actually needed.
> 
> Re-tune happens in the request error handling path, but also if you
> configure a re-tuning period. That ought to be sufficient, don't you
> think?

Hi Ulf,

Is something like this close to what you have in mind?

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index 94e79449c533..fc43cf5ce958 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -357,11 +357,9 @@ static void sh_mobile_sdhi_prepare_tuning(struct tmio_mmc_host *host,
 
 #define SH_MOBILE_SDHI_MAX_TAP 3
 
-static int sh_mobile_sdhi_select_tuning(struct tmio_mmc_host *host,
-					bool *tap, int tap_size)
+static int sh_mobile_sdhi_select_tuning(struct tmio_mmc_host *host)
 {
 	struct sh_mobile_sdhi *priv = host_to_priv(host);
-	unsigned long tap_num;  /* total number of taps */
 	unsigned long tap_cnt;  /* counter of tuning success */
 	unsigned long tap_set;  /* tap position */
 	unsigned long tap_start;/* start position of tuning success */
@@ -372,14 +370,6 @@ static int sh_mobile_sdhi_select_tuning(struct tmio_mmc_host *host,
 	/* Clear SCC_RVSREQ */
 	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_RVSREQ, 0);
 
-	/* Select SCC */
-	tap_num = (sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_DTCNTL) >>
-		   SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_SHIFT) &
-		SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_MASK;
-
-	if (tap_num * 2 != tap_size)
-		return -EINVAL;
-
 	/*
 	 * Find the longest consecutive run of successful probes.  If that
 	 * is more than SH_MOBILE_SDHI_MAX_TAP probes long then use the
@@ -389,8 +379,8 @@ static int sh_mobile_sdhi_select_tuning(struct tmio_mmc_host *host,
 	ntap = 0;
 	tap_start = 0;
 	tap_end = 0;
-	for (i = 0; i < tap_num * 2; i++) {
-		if (tap[i] == 0)
+	for (i = 0; i < host->tap_num * 2; i++) {
+		if (test_bit(i, host->taps))
 			ntap++;
 		else {
 			if (ntap > tap_cnt) {
@@ -409,7 +399,7 @@ static int sh_mobile_sdhi_select_tuning(struct tmio_mmc_host *host,
 	}
 
 	if (tap_cnt >= SH_MOBILE_SDHI_MAX_TAP)
-		tap_set = (tap_start + tap_end) / 2 % tap_num;
+		tap_set = (tap_start + tap_end) / 2 % host->tap_num;
 	else
 		return -EIO;
 
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index c932fe876690..4186045cce0d 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -171,8 +171,12 @@ struct tmio_mmc_host {
 	/* Mandatory callback for tuning to occur which is
 	 * optional for SDR50 and mandatory for SDR104 */
 	unsigned int (*init_tuning)(struct tmio_mmc_host *host);
-	int (*select_tuning)(struct tmio_mmc_host *host, bool *tap,
-			     int tap_size);
+	int (*select_tuning)(struct tmio_mmc_host *host);
+
+	/* Tuning values: 1 for success, 0 for failure */
+	DECLARE_BITMAP(taps, BITS_PER_BYTE * sizeof(long));
+	unsigned int tap_num;
+	bool use_saved_taps;
 };
 
 struct tmio_mmc_host *tmio_mmc_host_alloc(struct platform_device *pdev);
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index ded8fa53e0a0..2b5c8b090ed3 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -773,43 +773,50 @@ static void tmio_mmc_hw_reset(struct mmc_host *mmc)
 static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
 {
 	struct tmio_mmc_host *host = mmc_priv(mmc);
-	unsigned int num;
-	int i, ret = 0;
-	bool *tap;
+	int ret = 0;
 
-	if (!host->init_tuning || !host->select_tuning)
-		/* Tuning is not supported */
-		goto out;
+	if (!host->tap_num) {
+		if (!host->init_tuning || !host->select_tuning)
+			/* Tuning is not supported */
+			goto out;
 
-	num = host->init_tuning(host);
-	if (!num)
-		/* Tuning is not supported */
-		goto out;
+		host->tap_num = host->init_tuning(host);
+		if (!host->tap_num)
+			/* Tuning is not supported */
+			goto out;
 
-	tap = kmalloc(num * 2 * sizeof(*tap), GFP_KERNEL);
-	if (!tap) {
-		ret = -ENOMEM;
-		goto out;
+		if (host->tap_num * 2 >= sizeof(host->taps) * BITS_PER_BYTE)
+			dev_warn_once(&host->pdev->dev,
+			      "Too many taps, skipping tuning. Please consider "
+			      "updating size of taps field of tmio_mmc_host\n");
+
+		host->use_saved_taps = false;
 	}
 
-	/* Issue CMD19 twice for each tap */
-	for (i = 0; i < 2 * num; i++) {
-		if (host->prepare_tuning)
-			host->prepare_tuning(host, i % num);
+	if (!host->use_saved_taps) {
+		int i;
+
+		bitmap_zero(host->taps, host->tap_num * 2);
 
-		ret = mmc_send_tuning(mmc, opcode, NULL);
-		if (ret && ret != -EILSEQ)
-			goto err_free;
-		tap[i] = (ret != 0);
+		/* Issue CMD19 twice for each tap */
+		for (i = 0; i < 2 * host->tap_num; i++) {
+			if (host->prepare_tuning)
+				host->prepare_tuning(host, i % host->tap_num);
 
-		mdelay(1);
+			ret = mmc_send_tuning(mmc, opcode, NULL);
+			if (ret && ret != -EILSEQ)
+				goto out;
+			if (ret == 0)
+				set_bit(i, host->taps);
+
+			mdelay(1);
+		}
 	}
 
-	ret = host->select_tuning(host, tap, num * 2);
+	ret = host->select_tuning(host);
 
-err_free:
-	kfree(tap);
 out:
+	host->use_saved_taps = false;
 	if (ret < 0) {
 		dev_warn(&host->pdev->dev, "Tuning procedure failed\n");
 		mmc_hw_reset(mmc);
@@ -1271,6 +1278,7 @@ int tmio_mmc_host_runtime_suspend(struct device *dev)
 
 	mmc_retune_timer_stop(host->mmc);
 	mmc_retune_needed(host->mmc);
+	host->use_saved_taps = true;
 
 	tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL);