[PATCH v2 0/2] mmc: renesas_sdhi: avoid bad TAPs for Gen3

[PATCH v2 0/2] mmc: renesas_sdhi: avoid bad TAPs for Gen3

[Wolfram Sang]

From: Wolfram Sang <wsa@the-dreams.de>

Here is a series which avoids using TAPs on R-Car Gen3 SoCs which are
bad to use according to HW engineers. The patch was originally for the
BSP (Thanks, Saito-san) and has been upported for the upstream driver.
Before we can apply this, we need to add some mising information for
R-Car M3-W ES1.[23].

Patches have been tested on Renesas Salvator-XS boards with R-Car H3
ES2.0 (changing from TAP3 to TAP4 here) and R-Car M3-N (changing from
TAP7 to TAP0 here).

A branch can be found here:

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

It is based on current mmc/next + "[PATCH] mmc: renesas_sdhi: shorten
types after refactorization"

One implementation detail: For now, I kept using a generic naming for
the quirks, e.g. "sdhi_quirks_bad_taps2367". It is possible that we need
to rename/add ones later which are SoC bound, e.g. "sdhi_quirks_m3w_30".
Especially when we add manual calibration somewhen. But for now, the
generic approach seems better to me, so I sticked to it.

Looking forward to further comments and/or testing.

Happy hacking,

   Wolfram

Takeshi Saito (1):
  mmc: renesas_sdhi: Avoid bad TAP in HS400

Wolfram Sang (1):
  mmc: renesas_sdhi: handle M3-N ES1.2 and 1.3 revisions

 drivers/mmc/host/renesas_sdhi.h      |  1 +
 drivers/mmc/host/renesas_sdhi_core.c | 61 ++++++++++++++++++++++++----
 2 files changed, 54 insertions(+), 8 deletions(-)

-- 
2.20.1

[PATCH v2 1/2] mmc: renesas_sdhi: handle M3-N ES1.2 and 1.3 revisions

[Wolfram Sang]

For ES1.2, add a comment explaining the situation. For ES1.3 (and
later, although unlikely), add a new entry defining it as 4tap.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---

Change since V1:
* added tag from Shimoda-san

 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 28b0830c4251..33b51105c788 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -711,11 +711,17 @@ static const struct renesas_sdhi_quirks sdhi_quirks_nohs400 = {
 	.hs400_disabled = true,
 };
 
+/*
+ * Note for r8a7796 / r8a774a1: we can't distinguish ES1.1 and 1.2 as of now.
+ * So, we want to treat them equally and only have a match for ES1.2 to enforce
+ * this if there ever will be a way to distinguish ES1.2.
+ */
 static const struct soc_device_attribute sdhi_quirks_match[]  = {
 	{ .soc_id = "r8a774a1", .revision = "ES1.[012]", .data = &sdhi_quirks_4tap_nohs400 },
 	{ .soc_id = "r8a7795", .revision = "ES1.*", .data = &sdhi_quirks_4tap_nohs400 },
 	{ .soc_id = "r8a7795", .revision = "ES2.0", .data = &sdhi_quirks_4tap },
 	{ .soc_id = "r8a7796", .revision = "ES1.[012]", .data = &sdhi_quirks_4tap_nohs400 },
+	{ .soc_id = "r8a7796", .revision = "ES1.*", .data = &sdhi_quirks_4tap },
 	{ .soc_id = "r8a77980", .data = &sdhi_quirks_nohs400 },
 	{ /* Sentinel. */ },
 };
-- 
2.20.1

Re: [PATCH v2 1/2] mmc: renesas_sdhi: handle M3-N ES1.2 and 1.3 revisions

[Geert Uytterhoeven]

Hi Wolfram,

Thanks for your patch!

On Thu, Apr 23, 2020 at 2:26 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> For ES1.2, add a comment explaining the situation. For ES1.3 (and
> later, although unlikely), add a new entry defining it as 4tap.

Usually we don't add soc_device_match quirks for unknown future revisions.

> --- a/drivers/mmc/host/renesas_sdhi_core.c
> +++ b/drivers/mmc/host/renesas_sdhi_core.c
> @@ -711,11 +711,17 @@ static const struct renesas_sdhi_quirks sdhi_quirks_nohs400 = {
>         .hs400_disabled = true,
>  };
>
> +/*
> + * Note for r8a7796 / r8a774a1: we can't distinguish ES1.1 and 1.2 as of now.
> + * So, we want to treat them equally and only have a match for ES1.2 to enforce
> + * this if there ever will be a way to distinguish ES1.2.
> + */
>  static const struct soc_device_attribute sdhi_quirks_match[]  = {
>         { .soc_id = "r8a774a1", .revision = "ES1.[012]", .data = &sdhi_quirks_4tap_nohs400 },
>         { .soc_id = "r8a7795", .revision = "ES1.*", .data = &sdhi_quirks_4tap_nohs400 },
>         { .soc_id = "r8a7795", .revision = "ES2.0", .data = &sdhi_quirks_4tap },
>         { .soc_id = "r8a7796", .revision = "ES1.[012]", .data = &sdhi_quirks_4tap_nohs400 },
> +       { .soc_id = "r8a7796", .revision = "ES1.*", .data = &sdhi_quirks_4tap },

R-Car M3-N is r8a77965, not r8a7796?

>         { .soc_id = "r8a77980", .data = &sdhi_quirks_nohs400 },
>         { /* Sentinel. */ },
>  };

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 1/2] mmc: renesas_sdhi: handle M3-N ES1.2 and 1.3 revisions

[Wolfram Sang]

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


> Usually we don't add soc_device_match quirks for unknown future revisions.

I was just following...

> 
> > --- a/drivers/mmc/host/renesas_sdhi_core.c
> > +++ b/drivers/mmc/host/renesas_sdhi_core.c
> > @@ -711,11 +711,17 @@ static const struct renesas_sdhi_quirks sdhi_quirks_nohs400 = {
> >         .hs400_disabled = true,
> >  };
> >
> > +/*
> > + * Note for r8a7796 / r8a774a1: we can't distinguish ES1.1 and 1.2 as of now.
> > + * So, we want to treat them equally and only have a match for ES1.2 to enforce
> > + * this if there ever will be a way to distinguish ES1.2.
> > + */
> >  static const struct soc_device_attribute sdhi_quirks_match[]  = {
> >         { .soc_id = "r8a774a1", .revision = "ES1.[012]", .data = &sdhi_quirks_4tap_nohs400 },
> >         { .soc_id = "r8a7795", .revision = "ES1.*", .data = &sdhi_quirks_4tap_nohs400 },

... this example here. This also applies to all future versions (not
that there will be any), no?

> >         { .soc_id = "r8a7795", .revision = "ES2.0", .data = &sdhi_quirks_4tap },
> >         { .soc_id = "r8a7796", .revision = "ES1.[012]", .data = &sdhi_quirks_4tap_nohs400 },
> > +       { .soc_id = "r8a7796", .revision = "ES1.*", .data = &sdhi_quirks_4tap },
> 
> R-Car M3-N is r8a77965, not r8a7796?

Right. $subject should be M3-W :(


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

Re: [PATCH v2 1/2] mmc: renesas_sdhi: handle M3-N ES1.2 and 1.3 revisions

[Geert Uytterhoeven]

Hi Wolfram,

On Thu, Apr 23, 2020 at 2:39 PM Wolfram Sang <wsa@the-dreams.de> wrote:
> > Usually we don't add soc_device_match quirks for unknown future revisions.
>
> I was just following...
>
> > > --- a/drivers/mmc/host/renesas_sdhi_core.c
> > > +++ b/drivers/mmc/host/renesas_sdhi_core.c
> > > @@ -711,11 +711,17 @@ static const struct renesas_sdhi_quirks sdhi_quirks_nohs400 = {
> > >         .hs400_disabled = true,
> > >  };
> > >
> > > +/*
> > > + * Note for r8a7796 / r8a774a1: we can't distinguish ES1.1 and 1.2 as of now.
> > > + * So, we want to treat them equally and only have a match for ES1.2 to enforce
> > > + * this if there ever will be a way to distinguish ES1.2.
> > > + */
> > >  static const struct soc_device_attribute sdhi_quirks_match[]  = {
> > >         { .soc_id = "r8a774a1", .revision = "ES1.[012]", .data = &sdhi_quirks_4tap_nohs400 },
> > >         { .soc_id = "r8a7795", .revision = "ES1.*", .data = &sdhi_quirks_4tap_nohs400 },
>
> ... this example here. This also applies to all future versions (not
> that there will be any), no?

Ah, but r8a7795 already moved to ES3.0, so it's very unlikely a newer
ES1.x will be made. Hence "ES1.*" is assumed to cover all known existing
ES1.x revisions.

For M3-N, we're still at ES1.x, AFAIK.

> > >         { .soc_id = "r8a7795", .revision = "ES2.0", .data = &sdhi_quirks_4tap },
> > >         { .soc_id = "r8a7796", .revision = "ES1.[012]", .data = &sdhi_quirks_4tap_nohs400 },
> > > +       { .soc_id = "r8a7796", .revision = "ES1.*", .data = &sdhi_quirks_4tap },
> >
> > R-Car M3-N is r8a77965, not r8a7796?
>
> Right. $subject should be M3-W :(

OK, that changes my point of view, as M3-W is also at ES3.0 (aka M3-W+ ;-),
so using "ES1.*" is fine.

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 1/2] mmc: renesas_sdhi: handle M3-N ES1.2 and 1.3 revisions

[Wolfram Sang]

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


> OK, that changes my point of view, as M3-W is also at ES3.0 (aka M3-W+ ;-),
> so using "ES1.*" is fine.

I agree. So, only $subject is wrong. Will fix!


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

[PATCH v2 2/2] mmc: renesas_sdhi: Avoid bad TAP in HS400

[Wolfram Sang]

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

With R-Car Gen3, CRC error occue at the following TAPs.

H3, M3W 1.3, M3N... TAP=2,3,6,7
M3W 3.0		... TAP=1,3,5,7

(Note: for 4tap SoCs, the numbers get divided by 2)

Do not use these TAPs in HS400, and also don't use auto correction but
manual correction.

We check for bad taps in two places:

1) After tuning HS400: Then, we select a neighbouring TAP. One of them
   must be good, because there are never three bad taps in a row.
   Retuning won't help because we just finished tuning.

2) After a manual correction request: Here, we can't switch to the
   requested TAP. But we can retune (if the HS200 tuning was good)
   because the environment might have changed since the last tuning.
   If not, we stay on the same TAP.

Signed-off-by: Takeshi Saito <takeshi.saito.xv@renesas.com>
[wsa: refactored to match upstream driver, reworded commit msg]
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---

Change since V1:
* added tag from Shimoda-san
* removed unneeded curly braces

 drivers/mmc/host/renesas_sdhi.h      |  1 +
 drivers/mmc/host/renesas_sdhi_core.c | 55 ++++++++++++++++++++++++----
 2 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi.h b/drivers/mmc/host/renesas_sdhi.h
index 86efa9d5cd6d..14c64caefc64 100644
--- a/drivers/mmc/host/renesas_sdhi.h
+++ b/drivers/mmc/host/renesas_sdhi.h
@@ -36,6 +36,7 @@ struct renesas_sdhi_of_data {
 struct renesas_sdhi_quirks {
 	bool hs400_disabled;
 	bool hs400_4taps;
+	u32 hs400_bad_taps;
 };
 
 struct tmio_mmc_dma {
diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index 33b51105c788..ff72b381a6b3 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -325,6 +325,8 @@ static void renesas_sdhi_hs400_complete(struct mmc_host *mmc)
 {
 	struct tmio_mmc_host *host = mmc_priv(mmc);
 	struct renesas_sdhi *priv = host_to_priv(host);
+	u32 bad_taps = priv->quirks ? priv->quirks->hs400_bad_taps : 0;
+	bool use_4tap = priv->quirks && priv->quirks->hs400_4taps;
 
 	sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, ~CLK_CTL_SCLKEN &
 		sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
@@ -352,10 +354,23 @@ static void renesas_sdhi_hs400_complete(struct mmc_host *mmc)
 		       SH_MOBILE_SDHI_SCC_DTCNTL_TAPEN |
 		       0x4 << SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_SHIFT);
 
+	/* Avoid bad TAP */
+	if (bad_taps & BIT(priv->tap_set)) {
+		u32 new_tap = (priv->tap_set + 1) % priv->tap_num;
+
+		if (bad_taps & BIT(new_tap))
+			new_tap = (priv->tap_set - 1) % priv->tap_num;
 
-	if (priv->quirks && priv->quirks->hs400_4taps)
-		sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET,
-			       priv->tap_set / 2);
+		if (bad_taps & BIT(new_tap)) {
+			new_tap = priv->tap_set;
+			dev_dbg(&host->pdev->dev, "Can't handle three bad tap in a row\n");
+		}
+
+		priv->tap_set = new_tap;
+	}
+
+	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET,
+		       priv->tap_set / (use_4tap ? 2 : 1));
 
 	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_CKSEL,
 		       SH_MOBILE_SDHI_SCC_CKSEL_DTSEL |
@@ -527,7 +542,7 @@ static int renesas_sdhi_execute_tuning(struct tmio_mmc_host *host, u32 opcode)
 static bool renesas_sdhi_manual_correction(struct tmio_mmc_host *host, bool use_4tap)
 {
 	struct renesas_sdhi *priv = host_to_priv(host);
-	unsigned int new_tap = priv->tap_set;
+	unsigned int new_tap = priv->tap_set, error_tap = priv->tap_set;
 	u32 val;
 
 	val = sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_RVSREQ);
@@ -539,20 +554,32 @@ static bool renesas_sdhi_manual_correction(struct tmio_mmc_host *host, bool use_
 	/* Change TAP position according to correction status */
 	if (sd_ctrl_read16(host, CTL_VERSION) == SDHI_VER_GEN3_SDMMC &&
 	    host->mmc->ios.timing == MMC_TIMING_MMC_HS400) {
+		u32 bad_taps = priv->quirks ? priv->quirks->hs400_bad_taps : 0;
 		/*
 		 * With HS400, the DAT signal is based on DS, not CLK.
 		 * Therefore, use only CMD status.
 		 */
 		u32 smpcmp = sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_SMPCMP) &
 					   SH_MOBILE_SDHI_SCC_SMPCMP_CMD_ERR;
-		if (!smpcmp)
+		if (!smpcmp) {
 			return false;	/* no error in CMD signal */
-		else if (smpcmp == SH_MOBILE_SDHI_SCC_SMPCMP_CMD_REQUP)
+		} else if (smpcmp == SH_MOBILE_SDHI_SCC_SMPCMP_CMD_REQUP) {
 			new_tap++;
-		else if (smpcmp == SH_MOBILE_SDHI_SCC_SMPCMP_CMD_REQDOWN)
+			error_tap--;
+		} else if (smpcmp == SH_MOBILE_SDHI_SCC_SMPCMP_CMD_REQDOWN) {
 			new_tap--;
-		else
+			error_tap++;
+		} else {
 			return true;	/* need retune */
+		}
+
+		/*
+		 * When new_tap is a bad tap, we cannot change. Then, we compare
+		 * with the HS200 tuning result. When smpcmp[error_tap] is OK,
+		 * we can at least retune.
+		 */
+		if (bad_taps & BIT(new_tap % priv->tap_num))
+			return test_bit(error_tap % priv->tap_num, priv->smpcmp);
 	} else {
 		if (val & SH_MOBILE_SDHI_SCC_RVSREQ_RVSERR)
 			return true;    /* need retune */
@@ -705,12 +732,21 @@ static const struct renesas_sdhi_quirks sdhi_quirks_4tap_nohs400 = {
 
 static const struct renesas_sdhi_quirks sdhi_quirks_4tap = {
 	.hs400_4taps = true,
+	.hs400_bad_taps = BIT(2) | BIT(3) | BIT(6) | BIT(7),
 };
 
 static const struct renesas_sdhi_quirks sdhi_quirks_nohs400 = {
 	.hs400_disabled = true,
 };
 
+static const struct renesas_sdhi_quirks sdhi_quirks_bad_taps1357 = {
+	.hs400_bad_taps = BIT(1) | BIT(3) | BIT(5) | BIT(7),
+};
+
+static const struct renesas_sdhi_quirks sdhi_quirks_bad_taps2367 = {
+	.hs400_bad_taps = BIT(2) | BIT(3) | BIT(6) | BIT(7),
+};
+
 /*
  * Note for r8a7796 / r8a774a1: we can't distinguish ES1.1 and 1.2 as of now.
  * So, we want to treat them equally and only have a match for ES1.2 to enforce
@@ -720,8 +756,11 @@ static const struct soc_device_attribute sdhi_quirks_match[]  = {
 	{ .soc_id = "r8a774a1", .revision = "ES1.[012]", .data = &sdhi_quirks_4tap_nohs400 },
 	{ .soc_id = "r8a7795", .revision = "ES1.*", .data = &sdhi_quirks_4tap_nohs400 },
 	{ .soc_id = "r8a7795", .revision = "ES2.0", .data = &sdhi_quirks_4tap },
+	{ .soc_id = "r8a7795", .revision = "ES3.*", .data = &sdhi_quirks_bad_taps2367 },
 	{ .soc_id = "r8a7796", .revision = "ES1.[012]", .data = &sdhi_quirks_4tap_nohs400 },
 	{ .soc_id = "r8a7796", .revision = "ES1.*", .data = &sdhi_quirks_4tap },
+	{ .soc_id = "r8a7796", .revision = "ES3.*", .data = &sdhi_quirks_bad_taps1357 },
+	{ .soc_id = "r8a77965", .data = &sdhi_quirks_bad_taps2367 },
 	{ .soc_id = "r8a77980", .data = &sdhi_quirks_nohs400 },
 	{ /* Sentinel. */ },
 };
-- 
2.20.1