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

[PATCH RFT 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 with two more patches added which
will be applied separately:

"mmc: renesas_sdhi: simplify execute_tuning"
"mmc: renesas_sdhi: Use BITS_PER_LONG helper"

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 a Renesas Salvator-XS board (R-Car M3-N). There
were no regressions detected. HS400 could be tuned the same way, and
checksumming large files still works. Note that I couldn't test the 'all
TAPs are good' case without hacking the code. So, maybe the BSP team has
a setup where this happens without tweaking? This is why the series is
marked as 'RFT' still.

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.

Looking forward to comments,

   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 RFT 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>
---
 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..f3ef26e29e8f 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 - 1 - rs > tap_cnt) {
+			tap_end = re - 1;
+			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 RFT 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>
---
 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 f3ef26e29e8f..beac70a0defb 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 - 1 - 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 RFT 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>
---
 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 beac70a0defb..4a5abf28dee5 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 - 1 - rs > tap_cnt) {
 			tap_end = re - 1;
 			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 RFT 1/3] mmc: renesas_sdhi: refactor calculation of best TAP

[Yoshihiro Shimoda]

Hi Wolfram-san,

Thank you for the patch!

> From: Wolfram Sang, Sent: Friday, March 6, 2020 6:32 PM
<snip>
> @@ -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 - 1 - rs > tap_cnt) {
> +			tap_end = re - 1;
> +			tap_start = rs;
> +			tap_cnt = tap_end - tap_start;

The left value on the condition and tap_cnt should be added "+ 1" like below.

+		if (re - 1 - rs + 1 > tap_cnt) {	// "if (re - rs > tap_cnt)" is more better?
+			tap_end = re - 1;
+			tap_start = rs;
+			tap_cnt = tap_end - tap_start + 1;

For example, the priv->taps value is 0x0007, the tap_cnt should be 3.
But, this patch's result will be 2.

Best regards,
Yoshihiro Shimoda

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

[Yoshihiro Shimoda]

Hi Wolfram-san,

> From: Wolfram Sang, Sent: Friday, March 6, 2020 6:32 PM
> 
> 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>

Thank you for the patch!

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

Best regards,
Yoshihiro Shimoda

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

[Yoshihiro Shimoda]

Hi Wolfram-san,

> From: Wolfram Sang, Sent: Friday, March 6, 2020 6:32 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>

Thank you for the patch!

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

Best regards,
Yoshihiro Shimoda

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

[Yoshihiro Shimoda]

Hi Wolfram-san,

Thank you for the patch!

> From: Wolfram Sang, Sent: Friday, March 6, 2020 6:32 PM
<snip>
> It has been tested on a Renesas Salvator-XS board (R-Car M3-N). There
> were no regressions detected. HS400 could be tuned the same way, and
> checksumming large files still works. Note that I couldn't test the 'all
> TAPs are good' case without hacking the code. So, maybe the BSP team has
> a setup where this happens without tweaking? This is why the series is
> marked as 'RFT' still.

Fortunately, I found such an 'all TAPs are good' case board (R-Car H3).
So, I tested this patch series with update tap_cnt on patch 1/3.
And then, I didn't find any regression. So,

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

Best regards,
Yoshihiro Shimoda

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

[Wolfram Sang]

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

Hi Shimoda-san,

> +		if (re - 1 - rs + 1 > tap_cnt) {	// "if (re - rs > tap_cnt)" is more better?

Oops, looks really like an off-by-one error on my side :( Yes, I think
're - rs' is good here...

> +			tap_end = re - 1;

... plain 'tap_end = re;' here ...

> +			tap_start = rs;
> +			tap_cnt = tap_end - tap_start + 1;

... and drop the '+ 1' here.

I will double check, and resend soon.

Thank you very much!

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Wolfram Sang]

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


> Fortunately, I found such an 'all TAPs are good' case board (R-Car H3).
> So, I tested this patch series with update tap_cnt on patch 1/3.

Great find, Shimoda-san! I can confirm that my R-Car H3 ES2.0 board also
has all good TAPs. So, I will run some more tests before resending.

All the best!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]