| Message ID | 20200306093159.6155-2-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: renesas_sdhi: improve TAP selection if all TAPs are good | expand |
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
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
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
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(-)