[PATCH v2 0/4] mmc: {tmio,renesas_sdhi}: fix tuning behavior

[PATCH v2 0/4] mmc: {tmio,renesas_sdhi}: fix tuning behavior

[Niklas Söderlund]

Hi,

Tuning failed on my R-Car H3 ES2.0 board using latest mmc/next while the
Renesas BSP kernel worked. After some digging I found patches in the BSP
which remedied this and whit these applied tuning now works for me.

I have done small fixes, updated commit messages and rebased on latest
mmc/next but Hayakawa-san did all the real work.

Masaharu Hayakawa (4):
  mmc: tmio: Fix tuning flow
  mmc: tmio: Fix SCC error detection
  mmc: renesas_sdhi: Fix sampling clock position selecting
  mmc: renesas_sdhi: skip SCC error check when retuning

 drivers/mmc/host/renesas_sdhi_core.c | 20 ++++++++++++++++++++
 drivers/mmc/host/tmio_mmc_core.c     |  9 +++------
 2 files changed, 23 insertions(+), 6 deletions(-)

-- 
2.17.0

[PATCH v2 1/4] mmc: tmio: Fix tuning flow

[Niklas Söderlund]

From: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>

If the return value of mmc_send_tuning() is error other than -EILSEQ,
the tuning fails and process goes out of for_loop. The correct
processing is to judge their TAP as not good (NG) and continue.

Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
[Niklas: update commit message]
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

---

* Changes since v1
- Change '!mmc_send_tuning()' to 'mmc_send_tuning() == 0'.
---
 drivers/mmc/host/tmio_mmc_core.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 416f9e078fda8b24..000fa9dff784ecb0 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -817,10 +817,7 @@ static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
 		if (host->prepare_tuning)
 			host->prepare_tuning(host, i % host->tap_num);
 
-		ret = mmc_send_tuning(mmc, opcode, NULL);
-		if (ret && ret != -EILSEQ)
-			goto out;
-		if (ret == 0)
+		if (mmc_send_tuning(mmc, opcode, NULL) == 0)
 			set_bit(i, host->taps);
 
 		usleep_range(1000, 1200);
-- 
2.17.0

[PATCH v2 2/4] mmc: tmio: Fix SCC error detection

[Niklas Söderlund]

From: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>

SDR104 and HS200 need to check for SCC error. If SCC error is detected,
retuning is necessary.

Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
[Niklas: update commit message]
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/mmc/host/tmio_mmc_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 000fa9dff784ecb0..525f16f4e33b01ae 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -919,8 +919,8 @@ static void tmio_mmc_finish_request(struct tmio_mmc_host *host)
 	if (mrq->cmd->error || (mrq->data && mrq->data->error))
 		tmio_mmc_abort_dma(host);
 
-	if (host->check_scc_error)
-		host->check_scc_error(host);
+	if (host->check_scc_error && host->check_scc_error(host))
+		mrq->cmd->error = -EILSEQ;
 
 	/* If SET_BLOCK_COUNT, continue with main command */
 	if (host->mrq && !mrq->cmd->error) {
-- 
2.17.0

[PATCH v2 3/4] mmc: renesas_sdhi: Fix sampling clock position selecting

[Niklas Söderlund]

From: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>

When tuning each tap is issued CMD19 twice and the result of both runs
recorded in host->taps. If the result is different between the two runs
the wrong sampling clock position was selected. Fix this by merging the
two runs and only keep the result for each tap if it was good in both
sets.

Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
[Niklas: update commit message]
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

---

* Changes since v1
- Updated commit message after discussion with Wolfram.
- Expanded comment in code to better explain why the results are merged.
---
 drivers/mmc/host/renesas_sdhi_core.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index 384ae6cfa289e22c..589da18bd636c2f4 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -384,6 +384,19 @@ static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host)
 	/* Clear SCC_RVSREQ */
 	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_RVSREQ, 0);
 
+	/*
+	 * When tuning CMD19 is issued twice for each tap, merge the
+	 * result requiring the tap to be good in both runs before
+	 * consider it for tuning selection.
+	 */
+	for (i = 0; i < host->tap_num * 2; i++) {
+		if (!test_bit(i, host->taps)) {
+			clear_bit(i % host->tap_num, host->taps);
+			clear_bit((i % host->tap_num) + host->tap_num,
+				  host->taps);
+		}
+	}
+
 	/*
 	 * Find the longest consecutive run of successful probes.  If that
 	 * is more than SH_MOBILE_SDHI_MAX_TAP probes long then use the
-- 
2.17.0

[PATCH v2 4/4] mmc: renesas_sdhi: skip SCC error check when retuning

[Niklas Söderlund]

From: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>

Checking for SCC error during retuning is unnecessary.

Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
[Niklas: fix small style issue]
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/mmc/host/renesas_sdhi_core.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index 589da18bd636c2f4..ef711c532a26babe 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -445,6 +445,13 @@ static bool renesas_sdhi_check_scc_error(struct tmio_mmc_host *host)
 {
 	struct renesas_sdhi *priv = host_to_priv(host);
 
+	if (!(host->mmc->ios.timing == MMC_TIMING_UHS_SDR104) &&
+	    !(host->mmc->ios.timing == MMC_TIMING_MMC_HS200))
+		return false;
+
+	if (host->mmc->doing_retune)
+		return false;
+
 	/* Check SCC error */
 	if (sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL) &
 	    SH_MOBILE_SDHI_SCC_RVSCNTL_RVSEN &&
-- 
2.17.0

Re: [PATCH v2 1/4] mmc: tmio: Fix tuning flow

[Geert Uytterhoeven]

Hi Niklas,

On Thu, Jul 5, 2018 at 4:20 PM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> From: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
>
> If the return value of mmc_send_tuning() is error other than -EILSEQ,
> the tuning fails and process goes out of for_loop. The correct
> processing is to judge their TAP as not good (NG) and continue.
>
> Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> [Niklas: update commit message]
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>
> ---
>
> * Changes since v1
> - Change '!mmc_send_tuning()' to 'mmc_send_tuning() == 0'.
> ---
>  drivers/mmc/host/tmio_mmc_core.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
> index 416f9e078fda8b24..000fa9dff784ecb0 100644
> --- a/drivers/mmc/host/tmio_mmc_core.c
> +++ b/drivers/mmc/host/tmio_mmc_core.c
> @@ -817,10 +817,7 @@ static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
>                 if (host->prepare_tuning)
>                         host->prepare_tuning(host, i % host->tap_num);
>
> -               ret = mmc_send_tuning(mmc, opcode, NULL);
> -               if (ret && ret != -EILSEQ)
> -                       goto out;

I'd just drop the two lines above, and keep the assignment to and test of ret.
Kernel coding style is to not do checks on function return values in
if statements,
but assign to and check an intermediate variable.

> -               if (ret == 0)
> +               if (mmc_send_tuning(mmc, opcode, NULL) == 0)
>                         set_bit(i, host->taps);
>
>                 usleep_range(1000, 1200);

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 3/4] mmc: renesas_sdhi: Fix sampling clock position selecting

[Geert Uytterhoeven]

On Thu, Jul 5, 2018 at 4:20 PM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> From: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
>
> When tuning each tap is issued CMD19 twice and the result of both runs
> recorded in host->taps. If the result is different between the two runs
> the wrong sampling clock position was selected. Fix this by merging the
> two runs and only keep the result for each tap if it was good in both
> sets.
>
> Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> [Niklas: update commit message]
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> --- a/drivers/mmc/host/renesas_sdhi_core.c
> +++ b/drivers/mmc/host/renesas_sdhi_core.c
> @@ -384,6 +384,19 @@ static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host)
>         /* Clear SCC_RVSREQ */
>         sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_RVSREQ, 0);
>
> +       /*
> +        * When tuning CMD19 is issued twice for each tap, merge the
> +        * result requiring the tap to be good in both runs before
> +        * consider it for tuning selection.

considering


Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 0/4] mmc: {tmio,renesas_sdhi}: fix tuning behavior

[Wolfram Sang]

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

On Thu, Jul 05, 2018 at 04:18:37PM +0200, Niklas Söderlund wrote:
> Hi,
> 
> Tuning failed on my R-Car H3 ES2.0 board using latest mmc/next while the
> Renesas BSP kernel worked. After some digging I found patches in the BSP
> which remedied this and whit these applied tuning now works for me.
> 
> I have done small fixes, updated commit messages and rebased on latest
> mmc/next but Hayakawa-san did all the real work.

Thanks, Niklas! I will try to get them reviewed this week.


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

Re: [PATCH v2 2/4] mmc: tmio: Fix SCC error detection

[Simon Horman]

On Thu, Jul 05, 2018 at 04:18:39PM +0200, Niklas Söderlund wrote:
> From: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> 
> SDR104 and HS200 need to check for SCC error. If SCC error is detected,
> retuning is necessary.
> 
> Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> [Niklas: update commit message]
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

> ---
>  drivers/mmc/host/tmio_mmc_core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
> index 000fa9dff784ecb0..525f16f4e33b01ae 100644
> --- a/drivers/mmc/host/tmio_mmc_core.c
> +++ b/drivers/mmc/host/tmio_mmc_core.c
> @@ -919,8 +919,8 @@ static void tmio_mmc_finish_request(struct tmio_mmc_host *host)
>  	if (mrq->cmd->error || (mrq->data && mrq->data->error))
>  		tmio_mmc_abort_dma(host);
>  
> -	if (host->check_scc_error)
> -		host->check_scc_error(host);
> +	if (host->check_scc_error && host->check_scc_error(host))
> +		mrq->cmd->error = -EILSEQ;
>  
>  	/* If SET_BLOCK_COUNT, continue with main command */
>  	if (host->mrq && !mrq->cmd->error) {
> -- 
> 2.17.0
> 

Re: [PATCH v2 4/4] mmc: renesas_sdhi: skip SCC error check when retuning

[Simon Horman]

On Thu, Jul 05, 2018 at 04:18:41PM +0200, Niklas Söderlund wrote:
> From: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> 
> Checking for SCC error during retuning is unnecessary.
> 
> Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> [Niklas: fix small style issue]
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/mmc/host/renesas_sdhi_core.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
> index 589da18bd636c2f4..ef711c532a26babe 100644
> --- a/drivers/mmc/host/renesas_sdhi_core.c
> +++ b/drivers/mmc/host/renesas_sdhi_core.c
> @@ -445,6 +445,13 @@ static bool renesas_sdhi_check_scc_error(struct tmio_mmc_host *host)
>  {
>  	struct renesas_sdhi *priv = host_to_priv(host);
>  
> +	if (!(host->mmc->ios.timing == MMC_TIMING_UHS_SDR104) &&
> +	    !(host->mmc->ios.timing == MMC_TIMING_MMC_HS200))
> +		return false;
> +
> +	if (host->mmc->doing_retune)
> +		return false;
> +

I believe this patch needs to be updaed now that HS400 support has
been merged.

>  	/* Check SCC error */
>  	if (sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL) &
>  	    SH_MOBILE_SDHI_SCC_RVSCNTL_RVSEN &&
> -- 
> 2.17.0
> 

Re: [PATCH v2 3/4] mmc: renesas_sdhi: Fix sampling clock position selecting

[Wolfram Sang]

On Thu, Jul 05, 2018 at 04:18:40PM +0200, Niklas Söderlund wrote:
> From: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> 
> When tuning each tap is issued CMD19 twice and the result of both runs
> recorded in host->taps. If the result is different between the two runs
> the wrong sampling clock position was selected. Fix this by merging the
> two runs and only keep the result for each tap if it was good in both
> sets.
> 
> Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> [Niklas: update commit message]
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Much better commit message.


> +	for (i = 0; i < host->tap_num * 2; i++) {
> +		if (!test_bit(i, host->taps)) {
> +			clear_bit(i % host->tap_num, host->taps);
> +			clear_bit((i % host->tap_num) + host->tap_num,
> +				  host->taps);
> +		}
> +	}

I just think the code is a bit clumsy maybe?

a) it clears the bit which is already cleared
b) if a bit in the first half clears a bit in the second half,
   they will both be cleared again when the loop processes the
   second half

One idea I have is to let the loop iterate only over tap_num and then
use a mask 'BIT(i) | BIT(i+tap_num)' and work with binary operators
then. But maybe there are also macros to test and clear bit patterns?

And Geert's comment.

Re: [PATCH v2 0/4] mmc: {tmio,renesas_sdhi}: fix tuning behavior

[Wolfram Sang]

> Tuning failed on my R-Car H3 ES2.0 board using latest mmc/next while the
> Renesas BSP kernel worked. After some digging I found patches in the BSP
> which remedied this and whit these applied tuning now works for me.
> 
> I have done small fixes, updated commit messages and rebased on latest
> mmc/next but Hayakawa-san did all the real work.

Thanks, Niklas. Did you also test some SD card accesses? It helped with
your "stubborn" card, too, or?

Re: [PATCH v2 1/4] mmc: tmio: Fix tuning flow

[Wolfram Sang]

On Thu, Jul 05, 2018 at 04:18:38PM +0200, Niklas Söderlund wrote:
> From: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> 
> If the return value of mmc_send_tuning() is error other than -EILSEQ,
> the tuning fails and process goes out of for_loop. The correct
> processing is to judge their TAP as not good (NG) and continue.
> 
> Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> [Niklas: update commit message]
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

I tend to agree with Geert, but it is a minor issue for me, so

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Re: [PATCH v2 2/4] mmc: tmio: Fix SCC error detection

[Wolfram Sang]

On Thu, Jul 05, 2018 at 04:18:39PM +0200, Niklas Söderlund wrote:
> From: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> 
> SDR104 and HS200 need to check for SCC error. If SCC error is detected,
> retuning is necessary.
> 
> Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> [Niklas: update commit message]
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Re: [PATCH v2 4/4] mmc: renesas_sdhi: skip SCC error check when retuning

[Wolfram Sang]

On Thu, Jul 05, 2018 at 04:18:41PM +0200, Niklas Söderlund wrote:
> From: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> 
> Checking for SCC error during retuning is unnecessary.
> 
> Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> [Niklas: fix small style issue]
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Re: [PATCH v2 4/4] mmc: renesas_sdhi: skip SCC error check when retuning

[Niklas Söderlund]

Hi Simon,

Thanks for your feedback.

On 2018-07-12 14:57:22 +0200, Simon Horman wrote:
> On Thu, Jul 05, 2018 at 04:18:41PM +0200, Niklas S�derlund wrote:
> > From: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> > 
> > Checking for SCC error during retuning is unnecessary.
> > 
> > Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> > [Niklas: fix small style issue]
> > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/mmc/host/renesas_sdhi_core.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
> > index 589da18bd636c2f4..ef711c532a26babe 100644
> > --- a/drivers/mmc/host/renesas_sdhi_core.c
> > +++ b/drivers/mmc/host/renesas_sdhi_core.c
> > @@ -445,6 +445,13 @@ static bool renesas_sdhi_check_scc_error(struct tmio_mmc_host *host)
> >  {
> >  	struct renesas_sdhi *priv = host_to_priv(host);
> >  
> > +	if (!(host->mmc->ios.timing == MMC_TIMING_UHS_SDR104) &&
> > +	    !(host->mmc->ios.timing == MMC_TIMING_MMC_HS200))
> > +		return false;
> > +
> > +	if (host->mmc->doing_retune)
> > +		return false;
> > +
> 
> I believe this patch needs to be updaed now that HS400 support has
> been merged.

Thanks I missed that HS400 had been merged, will update for next 
version.

> 
> >  	/* Check SCC error */
> >  	if (sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL) &
> >  	    SH_MOBILE_SDHI_SCC_RVSCNTL_RVSEN &&
> > -- 
> > 2.17.0
> > 

-- 
Regards,
Niklas S�derlund

Re: [PATCH v2 0/4] mmc: {tmio,renesas_sdhi}: fix tuning behavior

[Niklas Söderlund]

On 2018-07-15 20:45:26 +0200, Wolfram Sang wrote:
> > Tuning failed on my R-Car H3 ES2.0 board using latest mmc/next while the
> > Renesas BSP kernel worked. After some digging I found patches in the BSP
> > which remedied this and whit these applied tuning now works for me.
> > 
> > I have done small fixes, updated commit messages and rebased on latest
> > mmc/next but Hayakawa-san did all the real work.
> 
> Thanks, Niklas. Did you also test some SD card accesses? It helped with
> your "stubborn" card, too, or?
> 

Yes I did a lot of testing and as you mention patch 1/4 fixes the 
problem I have experienced with my "stubborn" card.

-- 
Regards,
Niklas S�derlund

Re: [PATCH v2 3/4] mmc: renesas_sdhi: Fix sampling clock position selecting

[Niklas Söderlund]

Hi Wolfram,

Thanks for your feedback.

On 2018-07-15 20:45:06 +0200, Wolfram Sang wrote:
> On Thu, Jul 05, 2018 at 04:18:40PM +0200, Niklas S�derlund wrote:
> > From: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> > 
> > When tuning each tap is issued CMD19 twice and the result of both runs
> > recorded in host->taps. If the result is different between the two runs
> > the wrong sampling clock position was selected. Fix this by merging the
> > two runs and only keep the result for each tap if it was good in both
> > sets.
> > 
> > Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> > [Niklas: update commit message]
> > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> 
> Much better commit message.
> 
> 
> > +	for (i = 0; i < host->tap_num * 2; i++) {
> > +		if (!test_bit(i, host->taps)) {
> > +			clear_bit(i % host->tap_num, host->taps);
> > +			clear_bit((i % host->tap_num) + host->tap_num,
> > +				  host->taps);
> > +		}
> > +	}
> 
> I just think the code is a bit clumsy maybe?
> 
> a) it clears the bit which is already cleared
> b) if a bit in the first half clears a bit in the second half,
>    they will both be cleared again when the loop processes the
>    second half
> 
> One idea I have is to let the loop iterate only over tap_num and then
> use a mask 'BIT(i) | BIT(i+tap_num)' and work with binary operators
> then. But maybe there are also macros to test and clear bit patterns?

I agree that the loop is clumsy. Unfortunately I can't find any bitmap 
operations that work with bit patterns. If this where just integers the 
following would have been a better solution:

    mask = (1 << host->tap_num) - 1;
    taps = (host->taps & (host->taps >> host->tap_num)) & mask;
    host->taps = (taps << host->tap_num) | taps;

Doing the same using the bitmap operations turns out to be more complex 
then the original loop. There is the option of using the 
bitmap_{from,to}_arr32() and use the bit operations above but that would 
limit the code to 16 taps. And that would deprive the value of using the 
bitmap in the first place.

However, I have reworked the loop to be easier to read and only use one 
clear_bit(). Hopefully this will make it less clumsy, if anyone know of 
a better way to handle this please let me know.

    for (i = 0; i < host->tap_num * 2; i++) {
	    int offset = host->tap_num * (i < host->tap_num ? 1 : -1);

	    if (!test_bit(i, host->taps))
		    clear_bit(i + offset, host->taps);
    }

> 
> And Geert's comment.
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH v2 3/4] mmc: renesas_sdhi: Fix sampling clock position selecting

[Wolfram Sang]

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


> > One idea I have is to let the loop iterate only over tap_num and then
> > use a mask 'BIT(i) | BIT(i+tap_num)' and work with binary operators
> > then. But maybe there are also macros to test and clear bit patterns?
> 
> I agree that the loop is clumsy. Unfortunately I can't find any bitmap 
> operations that work with bit patterns. If this where just integers the 
> following would have been a better solution:
> 
>     mask = (1 << host->tap_num) - 1;
>     taps = (host->taps & (host->taps >> host->tap_num)) & mask;
>     host->taps = (taps << host->tap_num) | taps;

My above sketch doesn't work?

	for (i = 0; i < host->tap_num; i++) {
		mask = BIT(i) | BIT(i + host->tap_num);
		if (host->taps & mask != mask)
			host->taps &= ~mask;
	}

Written from top of my head, maybe I overlooked something.


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

Re: [PATCH v2 3/4] mmc: renesas_sdhi: Fix sampling clock position selecting

[Niklas Söderlund]

Hi Wolfram,

Thanks for your feedback.

On 2018-07-18 11:44:13 +0200, Wolfram Sang wrote:
> 
> > > One idea I have is to let the loop iterate only over tap_num and then
> > > use a mask 'BIT(i) | BIT(i+tap_num)' and work with binary operators
> > > then. But maybe there are also macros to test and clear bit patterns?
> > 
> > I agree that the loop is clumsy. Unfortunately I can't find any bitmap 
> > operations that work with bit patterns. If this where just integers the 
> > following would have been a better solution:
> > 
> >     mask = (1 << host->tap_num) - 1;
> >     taps = (host->taps & (host->taps >> host->tap_num)) & mask;
> >     host->taps = (taps << host->tap_num) | taps;
> 
> My above sketch doesn't work?
> 
> 	for (i = 0; i < host->tap_num; i++) {
> 		mask = BIT(i) | BIT(i + host->tap_num);
> 		if (host->taps & mask != mask)
> 			host->taps &= ~mask;
> 	}
> 
> Written from top of my head, maybe I overlooked something.
> 

Unfortunately not as host->taps is declared as a bitmap

    DECLARE_BITMAP(taps, BITS_PER_BYTE * sizeof(long));

So trying your sketch results in errors.

drivers/mmc/host/renesas_sdhi_core.c:394:32: error: invalid operands to binary & (have 'long unsigned int *' and 'int')
                 if (host->taps & mask != mask)
                     ~~~~       ^ ~~~~~~~~~~~~
drivers/mmc/host/renesas_sdhi_core.c:395:36: error: assignment to expression with array type
                         host->taps &= ~mask;

I'm sure one could dereference the host->taps variable and do the bit 
operations but that feels like a bad idea. The other option is to use 
bitmap_{from,to}_arr32() to access the bitmap as 32 bit integers but 
that too feels odd and more complex then using the bitmap helper 
functions.

I will await your feedback before on this before I post the next version 
of this series.

-- 
Regards,
Niklas S�derlund

Re: [PATCH v2 3/4] mmc: renesas_sdhi: Fix sampling clock position selecting

[Wolfram Sang]

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


> I'm sure one could dereference the host->taps variable and do the bit 
> operations but that feels like a bad idea. The other option is to use 
> bitmap_{from,to}_arr32() to access the bitmap as 32 bit integers but 
> that too feels odd and more complex then using the bitmap helper 
> functions.

I see. I think we need to refactor the tap related code anyhow when we
support the 8 tap SoCs. Because then all the 'tap * 2' is wrong. Then we
can also see if we really want a bitmap or just use a ulong? So, your
solution will do for now.

> I will await your feedback before on this before I post the next version 
> of this series.

One minor idea I had, but this may be bike-shedding:

	int offset = (i + host->num_taps) % (2 * host->num_taps)?

Dunno, your call.


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