diff mbox series

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

Message ID 20191203203301.2202-2-wsa+renesas@sang-engineering.com (mailing list archive)
State New, archived
Headers show
Series mmc: renesas_sdhi: add manual correction | expand

Commit Message

Wolfram Sang Dec. 3, 2019, 8:32 p.m. UTC 
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(-)

Comments

Geert Uytterhoeven Dec. 4, 2019, 9:44 a.m. UTC | #1 
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
Yoshihiro Shimoda Dec. 9, 2019, 6:42 a.m. UTC | #2 
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
Wolfram Sang Dec. 10, 2019, 10:28 p.m. UTC | #3 
> 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.
Geert Uytterhoeven Dec. 11, 2019, 7:43 a.m. UTC | #4 
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
Wolfram Sang Dec. 13, 2019, 4:18 p.m. UTC | #5 
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
diff mbox series

Patch

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)