[RFC PATCH 0/2] mmc: renesas_sdhi: add manual correction

[RFC PATCH 0/2] mmc: renesas_sdhi: add manual correction

[Wolfram Sang]

Add manual tap correction because the automatic one fails for HS400 on
Gen3.

(Limited) testing on Renesas Salvator-XS with M3-N and H3 ES2.0. I want
to apply more testing before sending out the real versions. Yet, release
early...

Depends on this series:
[PATCH 0/5] mmc: renesas_sdhi: prepare for recent HS400 updates

A branch is here:
git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/sdhi/new_manual_calib


Takeshi Saito (1):
  mmc: renesas_sdhi: Add manual correction

Wolfram Sang (1):
  mmc: renesas_sdhi: enfore manual correction for Gen3

 drivers/mmc/host/renesas_sdhi.h      |  1 +
 drivers/mmc/host/renesas_sdhi_core.c | 68 +++++++++++++++++++++++-----
 2 files changed, 58 insertions(+), 11 deletions(-)

-- 
2.20.1

[RFC PATCH 1/2] mmc: renesas_sdhi: Add manual correction

[Wolfram Sang]

From: Takeshi Saito <takeshi.saito.xv@renesas.com>

This patch adds a manual correction mechanism for SDHI. Currently, SDHI
uses automatic TAP position correction. However, TAP position can also
be corrected manually via correction error status flags.

Signed-off-by: Takeshi Saito <takeshi.saito.xv@renesas.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Pretty much like the BSP patch. Just some rebasing, refactorization and
updates of the commit message. I especially changed the original
calculation from:

+               host->tap_set = (host->tap_set +
+                                host->tap_num + 1) % host->tap_num;

 to:

+		host->tap_set = (host->tap_set + 1) % host->tap_num;

(same for -1). Because "+ tap_num % tap_num" is a NOOP. Or did I miss
something?


 drivers/mmc/host/renesas_sdhi.h      |  1 +
 drivers/mmc/host/renesas_sdhi_core.c | 62 +++++++++++++++++++++++-----
 2 files changed, 52 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi.h b/drivers/mmc/host/renesas_sdhi.h
index f524251d5113..11a0b2bca3aa 100644
--- a/drivers/mmc/host/renesas_sdhi.h
+++ b/drivers/mmc/host/renesas_sdhi.h
@@ -57,6 +57,7 @@ struct renesas_sdhi {
 	void __iomem *scc_ctl;
 	u32 scc_tappos;
 	u32 scc_tappos_hs400;
+	bool doing_tune;
 };
 
 #define host_to_priv(host) \
diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index 282c133defd6..903da3ba399b 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -263,6 +263,8 @@ static int renesas_sdhi_start_signal_voltage_switch(struct mmc_host *mmc,
 #define SH_MOBILE_SDHI_SCC_RVSCNTL_RVSEN	BIT(0)
 /* Definitions for values the SH_MOBILE_SDHI_SCC_RVSREQ register */
 #define SH_MOBILE_SDHI_SCC_RVSREQ_RVSERR	BIT(2)
+#define SH_MOBILE_SDHI_SCC_RVSREQ_REQTAPUP	BIT(1)
+#define SH_MOBILE_SDHI_SCC_RVSREQ_REQTAPDOWN	BIT(0)
 /* Definitions for values the SH_MOBILE_SDHI_SCC_TMPPORT2 register */
 #define SH_MOBILE_SDHI_SCC_TMPPORT2_HS400OSEL	BIT(4)
 #define SH_MOBILE_SDHI_SCC_TMPPORT2_HS400EN	BIT(31)
@@ -321,6 +323,8 @@ static void renesas_sdhi_prepare_tuning(struct tmio_mmc_host *host,
 {
 	struct renesas_sdhi *priv = host_to_priv(host);
 
+	priv->doing_tune = true;
+
 	/* Set sampling clock position */
 	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, tap);
 }
@@ -426,6 +430,8 @@ static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host)
 	unsigned long ntap;     /* temporary counter of tuning success */
 	unsigned long i;
 
+	priv->doing_tune = false;
+
 	/* Clear SCC_RVSREQ */
 	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_RVSREQ, 0);
 
@@ -485,12 +491,52 @@ static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host)
 	return 0;
 }
 
+static bool renesas_sdhi_manual_correction(struct tmio_mmc_host *host, bool use_4tap)
+{
+	struct renesas_sdhi *priv = host_to_priv(host);
+	u32 val;
+
+	val = sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_RVSREQ);
+	if (!val)
+		return false;
+
+	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_RVSREQ, 0);
+
+	/* Change TAP position according to correction status */
+	if (val & SH_MOBILE_SDHI_SCC_RVSREQ_RVSERR)
+		return true;    /* Need re-tune */
+	else if (val & SH_MOBILE_SDHI_SCC_RVSREQ_REQTAPUP)
+		host->tap_set = (host->tap_set + 1) % host->tap_num;
+	else if (val & SH_MOBILE_SDHI_SCC_RVSREQ_REQTAPDOWN)
+		host->tap_set = (host->tap_set - 1) % host->tap_num;
+	else
+		return false;
+
+	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET,
+		       host->tap_set / (use_4tap ? 2 : 1));
+
+	return false;
+}
+
+static bool renesas_sdhi_auto_correction(struct tmio_mmc_host *host)
+{
+	struct renesas_sdhi *priv = host_to_priv(host);
+
+	/* Check SCC error */
+	if (sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_RVSREQ) &
+	    SH_MOBILE_SDHI_SCC_RVSREQ_RVSERR) {
+		sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_RVSREQ, 0);
+		return true;
+	}
+
+	return false;
+}
+
 static bool renesas_sdhi_check_scc_error(struct tmio_mmc_host *host)
 {
 	struct renesas_sdhi *priv = host_to_priv(host);
 	bool use_4tap = priv->quirks && priv->quirks->hs400_4taps;
 
-
 	/*
 	 * Skip checking SCC errors when running on 4 taps in HS400 mode as
 	 * any retuning would still result in the same 4 taps being used.
@@ -500,20 +546,14 @@ static bool renesas_sdhi_check_scc_error(struct tmio_mmc_host *host)
 	    !(host->mmc->ios.timing == MMC_TIMING_MMC_HS400 && !use_4tap))
 		return false;
 
-	if (mmc_doing_retune(host->mmc))
+	if (mmc_doing_retune(host->mmc) || priv->doing_tune)
 		return false;
 
-	/* Check SCC error */
 	if (sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL) &
-	    SH_MOBILE_SDHI_SCC_RVSCNTL_RVSEN &&
-	    sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_RVSREQ) &
-	    SH_MOBILE_SDHI_SCC_RVSREQ_RVSERR) {
-		/* Clear SCC error */
-		sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_RVSREQ, 0);
-		return true;
-	}
+	    SH_MOBILE_SDHI_SCC_RVSCNTL_RVSEN)
+		return renesas_sdhi_auto_correction(host);
 
-	return false;
+	return renesas_sdhi_manual_correction(host, use_4tap);
 }
 
 static void renesas_sdhi_hw_reset(struct tmio_mmc_host *host)
-- 
2.20.1

[RFC PATCH 2/2] mmc: renesas_sdhi: enfore manual correction for Gen3

[Wolfram Sang]

HW engineers say that automatic tap correction cannot be used for HS400
in all R-Car Gen3 SoCs. So, check for that SDHI variant and disable it
when HS400 is about to be enabled.

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

The BSP handles this differently and adds a quirk entry to every
soc_device_match and checks the quirk entry. I chose this less intrusive
method because all Gen3 SoCs are affected. But not all SDHI instances,
only those which can access eMMC, of course. So, I likes this approach a
tad better. If you think all quirks should be centralized in the quirks
structure, I am open for discussion.

 drivers/mmc/host/renesas_sdhi_core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index 903da3ba399b..a29290ee2051 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -343,6 +343,12 @@ static void renesas_sdhi_hs400_complete(struct tmio_mmc_host *host)
 	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_DT2FF,
 		       priv->scc_tappos_hs400);
 
+	/* Gen3 can't do automatic tap correction with HS400, so disable it */
+	if (sd_ctrl_read16(host, CTL_VERSION) == SDHI_VER_GEN3_SDMMC)
+		sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL,
+			       ~SH_MOBILE_SDHI_SCC_RVSCNTL_RVSEN &
+			       sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL));
+
 	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TMPPORT2,
 		       (SH_MOBILE_SDHI_SCC_TMPPORT2_HS400EN |
 			SH_MOBILE_SDHI_SCC_TMPPORT2_HS400OSEL) |
-- 
2.20.1

[RFC PATCH 3/2] mmc: renesas_sdhi: remove double clear of automatic correction

[Wolfram Sang]

hw_reset() clears the automatic correction bit twice. I couldn't find
anything in the docs recommending that. Removing one of them didn't
cause any regressions here, so keep it simple.

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

Last minute patch, sorry for the confusing numbering...

 drivers/mmc/host/renesas_sdhi_core.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index a29290ee2051..10ccd90e5c64 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -578,10 +578,6 @@ static void renesas_sdhi_hw_reset(struct tmio_mmc_host *host)
 		       ~SH_MOBILE_SDHI_SCC_RVSCNTL_RVSEN &
 		       sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL));
 
-	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL,
-		       ~SH_MOBILE_SDHI_SCC_RVSCNTL_RVSEN &
-		       sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL));
-
 	if (host->pdata->flags & TMIO_MMC_MIN_RCAR2)
 		sd_ctrl_write32_as_16_and_16(host, CTL_IRQ_MASK,
 					     TMIO_MASK_INIT_RCAR2);
-- 
2.20.1

Re: [RFC PATCH 2/2] mmc: renesas_sdhi: enfore manual correction for Gen3

[Geert Uytterhoeven]

enforce?

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: [RFC PATCH 1/2] mmc: renesas_sdhi: Add manual correction

[Geert Uytterhoeven]

Hi Wolfram,

On Tue, Dec 3, 2019 at 9:33 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> Pretty much like the BSP patch. Just some rebasing, refactorization and
> updates of the commit message. I especially changed the original
> calculation from:
>
> +               host->tap_set = (host->tap_set +
> +                                host->tap_num + 1) % host->tap_num;
>
>  to:
>
> +               host->tap_set = (host->tap_set + 1) % host->tap_num;
>
> (same for -1). Because "+ tap_num % tap_num" is a NOOP. Or did I miss
> something?

Since all variables are unsigned, this should be equivalent.
BTW, why is tap_set unsigned long instead of unsigned int?
And perhaps it should be moved from host to priv?

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: [RFC PATCH 1/2] mmc: renesas_sdhi: Add manual correction

[Yoshihiro Shimoda]

Hi Wolfram-san,

> From: Wolfram Sang, Sent: Wednesday, December 4, 2019 5:33 AM
> 
> This patch adds a manual correction mechanism for SDHI. Currently, SDHI
> uses automatic TAP position correction. However, TAP position can also
> be corrected manually via correction error status flags.
> 
> Signed-off-by: Takeshi Saito <takeshi.saito.xv@renesas.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> Pretty much like the BSP patch. Just some rebasing, refactorization and
> updates of the commit message. I especially changed the original
> calculation from:
> 
> +               host->tap_set = (host->tap_set +
> +                                host->tap_num + 1) % host->tap_num;
> 
>  to:
> 
> +		host->tap_set = (host->tap_set + 1) % host->tap_num;
> 
> (same for -1). Because "+ tap_num % tap_num" is a NOOP. Or did I miss
> something?

Thank you for the patch! I checked your calculation and it's no problem
because the type of host->tap_set is unsigned long.

example 1 (set = 0, num = 8, down):
 BSP code:  (0 + 8 - 1) % 8 = 7 % 8 = 7
 your code: (0 - 1) % 8 = 0xffffffffffffffff % 8 = 7

example 2 (set = 7, num = 8, up):
 BSP code:  (7 + 8 + 1) % 8 = 16 % 8 = 0
 your code: (7 + 1) % 8 = 8 % 8 = 0

So,

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

I have a nit comment below:

<snip>
>  static bool renesas_sdhi_check_scc_error(struct tmio_mmc_host *host)
>  {
>  	struct renesas_sdhi *priv = host_to_priv(host);
>  	bool use_4tap = priv->quirks && priv->quirks->hs400_4taps;
> 
> -

nit: This deleting a blank line is not needed after the following patch was fixed :)
https://patchwork.kernel.org/patch/11271859/

Best regards,
Yoshihiro Shimoda

RE: [RFC PATCH 2/2] mmc: renesas_sdhi: enfore manual correction for Gen3

[Yoshihiro Shimoda]

Hi Wolfram-san,

> From: Wolfram Sang, Sent: Wednesday, December 4, 2019 5:33 AM
> 
> HW engineers say that automatic tap correction cannot be used for HS400
> in all R-Car Gen3 SoCs. So, check for that SDHI variant and disable it
> when HS400 is about to be enabled.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> The BSP handles this differently and adds a quirk entry to every
> soc_device_match and checks the quirk entry. I chose this less intrusive
> method because all Gen3 SoCs are affected. But not all SDHI instances,
> only those which can access eMMC, of course. So, I likes this approach a
> tad better. If you think all quirks should be centralized in the quirks
> structure, I am open for discussion.

Thank you for the patch! I agreed your choice. If we had a SoC which can use
auto correction, we could add such a quirk at that timing :) So,

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

Best regards,
Yoshihiro Shimoda

RE: [RFC PATCH 3/2] mmc: renesas_sdhi: remove double clear of automatic correction

[Yoshihiro Shimoda]

Hi Wolfram-san,

> From: Wolfram Sang, Sent: Wednesday, December 4, 2019 5:33 AM
> 
> hw_reset() clears the automatic correction bit twice. I couldn't find
> anything in the docs recommending that. Removing one of them didn't
> cause any regressions here, so keep it simple.
> 
> 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: [RFC PATCH 1/2] mmc: renesas_sdhi: Add manual correction

[Wolfram Sang]

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


> BTW, why is tap_set unsigned long instead of unsigned int?

Because we use bitmap functions on it, and those have all unsigned long
as arguments.

> And perhaps it should be moved from host to priv?

That would be great, but other tap_* variables are still used in
tmio_mmc_core.c. We maybe can refactor all tap handling into
renesas_sdhi_core.c meanwhile, but this is a seperate issue.


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

Re: [RFC PATCH 0/2] mmc: renesas_sdhi: add manual correction

[Wolfram Sang]

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

On Tue, Dec 03, 2019 at 09:32:58PM +0100, Wolfram Sang wrote:
> Add manual tap correction because the automatic one fails for HS400 on
> Gen3.
> 
> (Limited) testing on Renesas Salvator-XS with M3-N and H3 ES2.0. I want
> to apply more testing before sending out the real versions. Yet, release
> early...

And to keep everyone updated: I think the BSP patch about ignoring DAT
correction error status should be folded into this series. I hope I can
finish RFC v2 this week.


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

Re: [RFC PATCH 1/2] mmc: renesas_sdhi: Add manual correction

[Geert Uytterhoeven]

Hi Wolfram,

On Tue, Dec 10, 2019 at 11:28 PM Wolfram Sang <wsa@the-dreams.de> wrote:
> > BTW, why is tap_set unsigned long instead of unsigned int?
>
> Because we use bitmap functions on it, and those have all unsigned long
> as arguments.

Do we? I only see bitops on host->taps?

> > And perhaps it should be moved from host to priv?
>
> That would be great, but other tap_* variables are still used in
> tmio_mmc_core.c. We maybe can refactor all tap handling into
> renesas_sdhi_core.c meanwhile, but this is a seperate issue.

tap_num is shared by the Renesas and TMIO code.
tap_set is Renesas-specific.

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: [RFC PATCH 1/2] mmc: renesas_sdhi: Add manual correction

[Wolfram Sang]

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

Hi Geert,

> > > BTW, why is tap_set unsigned long instead of unsigned int?
> >
> > Because we use bitmap functions on it, and those have all unsigned long
> > as arguments.
> 
> Do we? I only see bitops on host->taps?

Point taken on this one...

> 
> > > And perhaps it should be moved from host to priv?
> >
> > That would be great, but other tap_* variables are still used in
> > tmio_mmc_core.c. We maybe can refactor all tap handling into
> > renesas_sdhi_core.c meanwhile, but this is a seperate issue.
> 
> tap_num is shared by the Renesas and TMIO code.
> tap_set is Renesas-specific.

... here however, I think we should keep all tap_* related variables
close together. The driver is messy enough as is. Investigation if all
of that code could be moved to renesas_sdhi_core, though, seems very
useful.

Thanks for the comments!

   Wolfram


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