[PATCH v2 0/3] mmc: renesas_sdhi: improve TAP selection if all TAPs are good

[PATCH v2 0/3] mmc: renesas_sdhi: improve TAP selection if all TAPs are good

[Wolfram Sang]

SDHI (with SCC) has a mechanism to select an optimal TAP even if all
were considered good during the tuning process. This series implements
support for it. Before that, some refactoring of 'best TAP selection' is
done to avoid code duplication and get more understandable code.

This work is based on BSP patches by Masaharu Hayakawa and Takeshi
Saito. It is built on top of mmc/next. For convenience, a branch is
here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/sdhi/new_manual_calib

It has been tested on Renesas Salvator-XS boards (R-Car M3-N and R-Car
H3 ES2.0). There were no regressions detected. HS400 could be tuned the
same way, and checksumming large files still works.

And while this series is useful by itself, it is also the last
prerequisite to implement some 'bad tap avoidance' on top which is what
we are originally aiming for.

Note about backporting: The super useful iterator
bitmap_for_each_set_region() is only available since v5.6. If you want
that before, you need to backport the needed bits of e837dfde15a4
("bitmap: genericize percpu bitmap region iterators"), too.

Thank you to Shimoda-san for his valuable input since the RFT version
of this patchset (see patch 1 for details)!

Kind regards,

   Wolfram

Wolfram Sang (3):
  mmc: renesas_sdhi: refactor calculation of best TAP
  mmc: renesas_sdhi: clarify handling of selecting TAPs
  mmc: renesas_sdhi: improve TAP selection if all TAPs are good

 drivers/mmc/host/renesas_sdhi.h      |  2 +
 drivers/mmc/host/renesas_sdhi_core.c | 64 ++++++++++++++--------------
 2 files changed, 34 insertions(+), 32 deletions(-)

-- 
2.20.1

[PATCH v2 1/3] mmc: renesas_sdhi: refactor calculation of best TAP

[Wolfram Sang]

To select the best TAP, we need to find the longest stream of set bits
in a bit field. There is now a helper function for bitmaps which
iterates over all region of set bits. Using it makes the code much more
concise and easier to understand. Double so, because we need to handle
two bitmaps in the near future. Remove a superfluous comment while here.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Change since RFT/v1:
* fix off by one error spotted by Shimoda-san

 drivers/mmc/host/renesas_sdhi_core.c | 36 +++++++---------------------
 1 file changed, 8 insertions(+), 28 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index df826661366f..f5d174d86117 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -427,15 +427,10 @@ static int renesas_sdhi_prepare_hs400_tuning(struct mmc_host *mmc, struct mmc_io
 static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host)
 {
 	struct renesas_sdhi *priv = host_to_priv(host);
-	unsigned long tap_cnt;  /* counter of tuning success */
-	unsigned long tap_start;/* start position of tuning success */
-	unsigned long tap_end;  /* end position of tuning success */
-	unsigned long ntap;     /* temporary counter of tuning success */
-	unsigned long i;
+	unsigned int tap_start = 0, tap_end = 0, tap_cnt = 0, rs, re, i;
+	unsigned int taps_size = priv->tap_num * 2;
 
 	priv->doing_tune = false;
-
-	/* Clear SCC_RVSREQ */
 	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_RVSREQ, 0);
 
 	/*
@@ -443,7 +438,7 @@ static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host)
 	 * result requiring the tap to be good in both runs before
 	 * considering it for tuning selection.
 	 */
-	for (i = 0; i < priv->tap_num * 2; i++) {
+	for (i = 0; i < taps_size; i++) {
 		int offset = priv->tap_num * (i < priv->tap_num ? 1 : -1);
 
 		if (!test_bit(i, priv->taps))
@@ -455,29 +450,14 @@ static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host)
 	 * is more than SH_MOBILE_SDHI_MAX_TAP probes long then use the
 	 * center index as the tap.
 	 */
-	tap_cnt = 0;
-	ntap = 0;
-	tap_start = 0;
-	tap_end = 0;
-	for (i = 0; i < priv->tap_num * 2; i++) {
-		if (test_bit(i, priv->taps)) {
-			ntap++;
-		} else {
-			if (ntap > tap_cnt) {
-				tap_start = i - ntap;
-				tap_end = i - 1;
-				tap_cnt = ntap;
-			}
-			ntap = 0;
+	bitmap_for_each_set_region(priv->taps, rs, re, 0, taps_size) {
+		if (re - rs > tap_cnt) {
+			tap_end = re;
+			tap_start = rs;
+			tap_cnt = tap_end - tap_start;
 		}
 	}
 
-	if (ntap > tap_cnt) {
-		tap_start = i - ntap;
-		tap_end = i - 1;
-		tap_cnt = ntap;
-	}
-
 	if (tap_cnt >= SH_MOBILE_SDHI_MAX_TAP)
 		priv->tap_set = (tap_start + tap_end) / 2 % priv->tap_num;
 	else
-- 
2.20.1

[PATCH v2 2/3] mmc: renesas_sdhi: clarify handling of selecting TAPs

[Wolfram Sang]

The comment and the define about how TAPs are selected were confusing to
me because the good TAP was only valid if it was bigger than a *_MAX_*
value. Rename the define and adapt the comment to what really happens.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/mmc/host/renesas_sdhi_core.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index f5d174d86117..0dbee47eafa1 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -422,7 +422,7 @@ static int renesas_sdhi_prepare_hs400_tuning(struct mmc_host *mmc, struct mmc_io
 	return 0;
 }
 
-#define SH_MOBILE_SDHI_MAX_TAP 3
+#define SH_MOBILE_SDHI_MIN_TAP_ROW 3
 
 static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host)
 {
@@ -446,9 +446,9 @@ static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host)
 	}
 
 	/*
-	 * Find the longest consecutive run of successful probes.  If that
-	 * is more than SH_MOBILE_SDHI_MAX_TAP probes long then use the
-	 * center index as the tap.
+	 * Find the longest consecutive run of successful probes. If that
+	 * is at least SH_MOBILE_SDHI_MIN_TAP_ROW probes long then use the
+	 * center index as the tap, otherwise bail out.
 	 */
 	bitmap_for_each_set_region(priv->taps, rs, re, 0, taps_size) {
 		if (re - rs > tap_cnt) {
@@ -458,7 +458,7 @@ static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host)
 		}
 	}
 
-	if (tap_cnt >= SH_MOBILE_SDHI_MAX_TAP)
+	if (tap_cnt >= SH_MOBILE_SDHI_MIN_TAP_ROW)
 		priv->tap_set = (tap_start + tap_end) / 2 % priv->tap_num;
 	else
 		return -EIO;
-- 
2.20.1

[PATCH v2 3/3] mmc: renesas_sdhi: improve TAP selection if all TAPs are good

[Wolfram Sang]

When tuning HS400, if all TAPS are good, we can utilize the SMPCMP
register to select the optimal TAP. For that, we populate a second
bitmap with SMPCMP results and query it in case the regular bitmap is
full (= all good).

Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
Signed-off-by: Takeshi Saito <takeshi.saito.xv@renesas.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/mmc/host/renesas_sdhi.h      |  2 ++
 drivers/mmc/host/renesas_sdhi_core.c | 26 +++++++++++++++++++++++---
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi.h b/drivers/mmc/host/renesas_sdhi.h
index 2a4c83a5f32e..12d8016672b0 100644
--- a/drivers/mmc/host/renesas_sdhi.h
+++ b/drivers/mmc/host/renesas_sdhi.h
@@ -61,6 +61,8 @@ struct renesas_sdhi {
 
 	/* Tuning values: 1 for success, 0 for failure */
 	DECLARE_BITMAP(taps, BITS_PER_LONG);
+	/* Sampling data comparison: 1 for match, 0 for mismatch */
+	DECLARE_BITMAP(smpcmp, BITS_PER_LONG);
 	unsigned int tap_num;
 	unsigned long tap_set;
 };
diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index 0dbee47eafa1..796b5eb50415 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -428,7 +428,8 @@ static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host)
 {
 	struct renesas_sdhi *priv = host_to_priv(host);
 	unsigned int tap_start = 0, tap_end = 0, tap_cnt = 0, rs, re, i;
-	unsigned int taps_size = priv->tap_num * 2;
+	unsigned int taps_size = priv->tap_num * 2, min_tap_row;
+	unsigned long *bitmap;
 
 	priv->doing_tune = false;
 	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_RVSREQ, 0);
@@ -443,6 +444,21 @@ static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host)
 
 		if (!test_bit(i, priv->taps))
 			clear_bit(i + offset, priv->taps);
+
+		if (!test_bit(i, priv->smpcmp))
+			clear_bit(i + offset, priv->smpcmp);
+	}
+
+	/*
+	 * If all TAP are OK, the sampling clock position is selected by
+	 * identifying the change point of data.
+	 */
+	if (bitmap_full(priv->taps, taps_size)) {
+		bitmap = priv->smpcmp;
+		min_tap_row = 1;
+	} else {
+		bitmap = priv->taps;
+		min_tap_row = SH_MOBILE_SDHI_MIN_TAP_ROW;
 	}
 
 	/*
@@ -450,7 +466,7 @@ static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host)
 	 * is at least SH_MOBILE_SDHI_MIN_TAP_ROW probes long then use the
 	 * center index as the tap, otherwise bail out.
 	 */
-	bitmap_for_each_set_region(priv->taps, rs, re, 0, taps_size) {
+	bitmap_for_each_set_region(bitmap, rs, re, 0, taps_size) {
 		if (re - rs > tap_cnt) {
 			tap_end = re;
 			tap_start = rs;
@@ -458,7 +474,7 @@ static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host)
 		}
 	}
 
-	if (tap_cnt >= SH_MOBILE_SDHI_MIN_TAP_ROW)
+	if (tap_cnt >= min_tap_row)
 		priv->tap_set = (tap_start + tap_end) / 2 % priv->tap_num;
 	else
 		return -EIO;
@@ -491,6 +507,7 @@ static int renesas_sdhi_execute_tuning(struct tmio_mmc_host *host, u32 opcode)
 
 	priv->doing_tune = true;
 	bitmap_zero(priv->taps, priv->tap_num * 2);
+	bitmap_zero(priv->smpcmp, priv->tap_num * 2);
 
 	/* Issue CMD19 twice for each tap */
 	for (i = 0; i < 2 * priv->tap_num; i++) {
@@ -499,6 +516,9 @@ static int renesas_sdhi_execute_tuning(struct tmio_mmc_host *host, u32 opcode)
 
 		if (mmc_send_tuning(host->mmc, opcode, NULL) == 0)
 			set_bit(i, priv->taps);
+
+		if (sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_SMPCMP) == 0)
+			set_bit(i, priv->smpcmp);
 	}
 
 	return renesas_sdhi_select_tuning(host);
-- 
2.20.1

RE: [PATCH v2 1/3] mmc: renesas_sdhi: refactor calculation of best TAP

[Yoshihiro Shimoda]

Hi Wolfram-san,

> From: Wolfram Sang, Sent: Wednesday, April 8, 2020 6:47 PM
> 
> To select the best TAP, we need to find the longest stream of set bits
> in a bit field. There is now a helper function for bitmaps which
> iterates over all region of set bits. Using it makes the code much more
> concise and easier to understand. Double so, because we need to handle
> two bitmaps in the near future. Remove a superfluous comment while here.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thank you for the patch!

Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Best regards,
Yoshihiro Shimoda

RE: [PATCH v2 3/3] mmc: renesas_sdhi: improve TAP selection if all TAPs are good

[Yoshihiro Shimoda]

Hi Wolfram-san,

> From: Wolfram Sang, Sent: Wednesday, April 8, 2020 6:47 PM
> 
> When tuning HS400, if all TAPS are good, we can utilize the SMPCMP
> register to select the optimal TAP. For that, we populate a second
> bitmap with SMPCMP results and query it in case the regular bitmap is
> full (= all good).
> 
> Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> Signed-off-by: Takeshi Saito <takeshi.saito.xv@renesas.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Thank you for the patch! I tested on the my environment (R-Car H3)
and then using this SMPCMP result works correctly. So,

Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Best regards,
Yoshihiro Shimoda

Re: [PATCH v2 0/3] mmc: renesas_sdhi: improve TAP selection if all TAPs are good

[Ulf Hansson]

On Wed, 8 Apr 2020 at 11:46, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> SDHI (with SCC) has a mechanism to select an optimal TAP even if all
> were considered good during the tuning process. This series implements
> support for it. Before that, some refactoring of 'best TAP selection' is
> done to avoid code duplication and get more understandable code.
>
> This work is based on BSP patches by Masaharu Hayakawa and Takeshi
> Saito. It is built on top of mmc/next. For convenience, a branch is
> here:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/sdhi/new_manual_calib
>
> It has been tested on Renesas Salvator-XS boards (R-Car M3-N and R-Car
> H3 ES2.0). There were no regressions detected. HS400 could be tuned the
> same way, and checksumming large files still works.
>
> And while this series is useful by itself, it is also the last
> prerequisite to implement some 'bad tap avoidance' on top which is what
> we are originally aiming for.
>
> Note about backporting: The super useful iterator
> bitmap_for_each_set_region() is only available since v5.6. If you want
> that before, you need to backport the needed bits of e837dfde15a4
> ("bitmap: genericize percpu bitmap region iterators"), too.
>
> Thank you to Shimoda-san for his valuable input since the RFT version
> of this patchset (see patch 1 for details)!
>
> Kind regards,
>
>    Wolfram
>
> Wolfram Sang (3):
>   mmc: renesas_sdhi: refactor calculation of best TAP
>   mmc: renesas_sdhi: clarify handling of selecting TAPs
>   mmc: renesas_sdhi: improve TAP selection if all TAPs are good
>
>  drivers/mmc/host/renesas_sdhi.h      |  2 +
>  drivers/mmc/host/renesas_sdhi_core.c | 64 ++++++++++++++--------------
>  2 files changed, 34 insertions(+), 32 deletions(-)
>
> --
> 2.20.1
>

Applied for next, thanks!

Kind regards
Uffe