Linux-mmc Archive on lore.kernel.org
 help / color / Atom feed
* [RFC PATCH 0/6] mmc: tmio: move TAP handling to SDHI driver
@ 2020-01-29 20:37 Wolfram Sang
  2020-01-29 20:37 ` [RFC PATCH 1/6] mmc: tmio: refactor tuning execution into " Wolfram Sang
                   ` (7 more replies)
  0 siblings, 8 replies; 17+ messages in thread
From: Wolfram Sang @ 2020-01-29 20:37 UTC (permalink / raw)
  To: linux-mmc; +Cc: linux-renesas-soc, Yoshihiro Shimoda, Wolfram Sang

TAP and SCC handling is Renesas specific, so it should be moved to the
SDHI driver. After previous refactoring, this is possible now. And
feasible, too, to simplify further HS400 corrections. IMHO it also makes
the driver less complicated.

See the patches why this series is still RFC.

This is based on top of the series:
[RFC PATCH v2 0/5] mmc: renesas_sdhi: add manual correction

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

It has been tested with a Renesas Salvator-XS boards, one with a R-Car
M3-N and one with H3-ES2.0. Tuning to HS400 still works.


Wolfram Sang (6):
  mmc: tmio: refactor tuning execution into SDHI driver
  mmc: renesas_sdhi: complain loudly if driver needs update
  mmc: tmio: give callback a generic name
  mmc: tmio: enforce retune after runtime suspend
  mmc: tmio: factor out TAP usage
  mmc: tmio: remove superfluous callback wrappers

 drivers/mmc/host/renesas_sdhi.h      |  5 ++
 drivers/mmc/host/renesas_sdhi_core.c | 90 +++++++++++++++++-----------
 drivers/mmc/host/tmio_mmc.h          | 11 +---
 drivers/mmc/host/tmio_mmc_core.c     | 77 +++---------------------
 4 files changed, 71 insertions(+), 112 deletions(-)

-- 
2.20.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [RFC PATCH 1/6] mmc: tmio: refactor tuning execution into SDHI driver
  2020-01-29 20:37 [RFC PATCH 0/6] mmc: tmio: move TAP handling to SDHI driver Wolfram Sang
@ 2020-01-29 20:37 ` " Wolfram Sang
  2020-02-10 22:44   ` Niklas Söderlund
  2020-01-29 20:37 ` [RFC PATCH 2/6] mmc: renesas_sdhi: complain loudly if driver needs update Wolfram Sang
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 17+ messages in thread
From: Wolfram Sang @ 2020-01-29 20:37 UTC (permalink / raw)
  To: linux-mmc; +Cc: linux-renesas-soc, Yoshihiro Shimoda, Wolfram Sang

Move Renesas specific code for executing the tuning with a SCC into the
SDHI driver and leave only a generic call in the TMIO driver. Simplify
the code a little by removing init_tuning() and prepare_tuning()
callbacks. The latter is directly folded into the new execute_tuning()
callbacks.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/renesas_sdhi_core.c | 45 ++++++++++++++++++++--------
 drivers/mmc/host/tmio_mmc.h          |  3 +-
 drivers/mmc/host/tmio_mmc_core.c     | 33 +++-----------------
 3 files changed, 37 insertions(+), 44 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index 6a112454ca26..b3ab66f963f8 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -321,17 +321,6 @@ static unsigned int renesas_sdhi_init_tuning(struct tmio_mmc_host *host)
 		SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_MASK;
 }
 
-static void renesas_sdhi_prepare_tuning(struct tmio_mmc_host *host,
-					unsigned long tap)
-{
-	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);
-}
-
 static void renesas_sdhi_hs400_complete(struct tmio_mmc_host *host)
 {
 	struct renesas_sdhi *priv = host_to_priv(host);
@@ -500,6 +489,37 @@ static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host)
 	return 0;
 }
 
+static int renesas_sdhi_execute_tuning(struct tmio_mmc_host *host, u32 opcode)
+{
+	struct renesas_sdhi *priv = host_to_priv(host);
+	int i, ret;
+
+	host->tap_num = renesas_sdhi_init_tuning(host);
+	if (!host->tap_num)
+		return 0; /* Tuning is not supported */
+
+	if (host->tap_num * 2 >= sizeof(host->taps) * BITS_PER_BYTE) {
+		dev_warn_once(&host->pdev->dev,
+			"Too many taps, skipping tuning. Please consider updating size of taps field of tmio_mmc_host\n");
+		return 0;
+	}
+
+	priv->doing_tune = true;
+	bitmap_zero(host->taps, host->tap_num * 2);
+
+	/* Issue CMD19 twice for each tap */
+	for (i = 0; i < 2 * host->tap_num; i++) {
+		/* Set sampling clock position */
+		sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, i % host->tap_num);
+
+		ret = mmc_send_tuning(host->mmc, opcode, NULL);
+		if (ret == 0)
+			set_bit(i, host->taps);
+	}
+
+	return renesas_sdhi_select_tuning(host);
+}
+
 static bool renesas_sdhi_manual_correction(struct tmio_mmc_host *host, bool use_4tap)
 {
 	struct renesas_sdhi *priv = host_to_priv(host);
@@ -877,8 +897,7 @@ int renesas_sdhi_probe(struct platform_device *pdev,
 		if (!hit)
 			dev_warn(&host->pdev->dev, "Unknown clock rate for tuning\n");
 
-		host->init_tuning = renesas_sdhi_init_tuning;
-		host->prepare_tuning = renesas_sdhi_prepare_tuning;
+		host->execute_tuning = renesas_sdhi_execute_tuning;
 		host->select_tuning = renesas_sdhi_select_tuning;
 		host->check_scc_error = renesas_sdhi_check_scc_error;
 		host->prepare_hs400_tuning =
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index c5ba13fae399..bfebbe368f02 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -176,14 +176,13 @@ struct tmio_mmc_host {
 	int (*write16_hook)(struct tmio_mmc_host *host, int addr);
 	void (*reset)(struct tmio_mmc_host *host);
 	void (*hw_reset)(struct tmio_mmc_host *host);
-	void (*prepare_tuning)(struct tmio_mmc_host *host, unsigned long tap);
 	bool (*check_scc_error)(struct tmio_mmc_host *host);
 
 	/*
 	 * Mandatory callback for tuning to occur which is optional for SDR50
 	 * and mandatory for SDR104.
 	 */
-	unsigned int (*init_tuning)(struct tmio_mmc_host *host);
+	int (*execute_tuning)(struct tmio_mmc_host *host, u32 opcode);
 	int (*select_tuning)(struct tmio_mmc_host *host);
 
 	/* Tuning values: 1 for success, 0 for failure */
diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index c4a1d49fbea4..593f88cafb6e 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -718,38 +718,13 @@ static int tmio_mmc_start_data(struct tmio_mmc_host *host,
 static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
 {
 	struct tmio_mmc_host *host = mmc_priv(mmc);
-	int i, ret = 0;
-
-	if (!host->init_tuning || !host->select_tuning)
-		/* Tuning is not supported */
-		goto out;
-
-	host->tap_num = host->init_tuning(host);
-	if (!host->tap_num)
-		/* Tuning is not supported */
-		goto out;
-
-	if (host->tap_num * 2 >= sizeof(host->taps) * BITS_PER_BYTE) {
-		dev_warn_once(&host->pdev->dev,
-			"Too many taps, skipping tuning. Please consider updating size of taps field of tmio_mmc_host\n");
-		goto out;
-	}
-
-	bitmap_zero(host->taps, host->tap_num * 2);
-
-	/* Issue CMD19 twice for each tap */
-	for (i = 0; i < 2 * host->tap_num; i++) {
-		if (host->prepare_tuning)
-			host->prepare_tuning(host, i % host->tap_num);
+	int ret;
 
-		ret = mmc_send_tuning(mmc, opcode, NULL);
-		if (ret == 0)
-			set_bit(i, host->taps);
-	}
+	if (!host->execute_tuning)
+		return 0;
 
-	ret = host->select_tuning(host);
+	ret = host->execute_tuning(host, opcode);
 
-out:
 	if (ret < 0) {
 		dev_warn(&host->pdev->dev, "Tuning procedure failed\n");
 		tmio_mmc_hw_reset(mmc);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [RFC PATCH 2/6] mmc: renesas_sdhi: complain loudly if driver needs update
  2020-01-29 20:37 [RFC PATCH 0/6] mmc: tmio: move TAP handling to SDHI driver Wolfram Sang
  2020-01-29 20:37 ` [RFC PATCH 1/6] mmc: tmio: refactor tuning execution into " Wolfram Sang
@ 2020-01-29 20:37 ` Wolfram Sang
  2020-02-10 22:47   ` Niklas Söderlund
  2020-01-29 20:37 ` [RFC PATCH 3/6] mmc: tmio: give callback a generic name Wolfram Sang
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 17+ messages in thread
From: Wolfram Sang @ 2020-01-29 20:37 UTC (permalink / raw)
  To: linux-mmc; +Cc: linux-renesas-soc, Yoshihiro Shimoda, Wolfram Sang

When the tap array in the driver is too low, this is not a warning but
an error. Also _once is not helpful, we should make sure it is
prominently in the logs. It is safe to do this because this will only
show up during SoC enablement when we a new SoCs needs more taps (if
that ever will happen).

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

Note: 'unsigned long' seems big enough for a while. But, famous last
words(tm). We could handle this at runtime by reallocating a bigger
buffer. Very unsure if it is worth it, though.

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

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index b3ab66f963f8..d63aeb35bd0b 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -499,9 +499,9 @@ static int renesas_sdhi_execute_tuning(struct tmio_mmc_host *host, u32 opcode)
 		return 0; /* Tuning is not supported */
 
 	if (host->tap_num * 2 >= sizeof(host->taps) * BITS_PER_BYTE) {
-		dev_warn_once(&host->pdev->dev,
-			"Too many taps, skipping tuning. Please consider updating size of taps field of tmio_mmc_host\n");
-		return 0;
+		dev_err(&host->pdev->dev,
+			"Too many taps, please update 'taps' in tmio_mmc_host!\n");
+		return -EINVAL;
 	}
 
 	priv->doing_tune = true;
-- 
2.20.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [RFC PATCH 3/6] mmc: tmio: give callback a generic name
  2020-01-29 20:37 [RFC PATCH 0/6] mmc: tmio: move TAP handling to SDHI driver Wolfram Sang
  2020-01-29 20:37 ` [RFC PATCH 1/6] mmc: tmio: refactor tuning execution into " Wolfram Sang
  2020-01-29 20:37 ` [RFC PATCH 2/6] mmc: renesas_sdhi: complain loudly if driver needs update Wolfram Sang
@ 2020-01-29 20:37 ` Wolfram Sang
  2020-02-10 22:51   ` Niklas Söderlund
  2020-01-29 20:37 ` [RFC PATCH 4/6] mmc: tmio: enforce retune after runtime suspend Wolfram Sang
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 17+ messages in thread
From: Wolfram Sang @ 2020-01-29 20:37 UTC (permalink / raw)
  To: linux-mmc; +Cc: linux-renesas-soc, Yoshihiro Shimoda, Wolfram Sang

check_scc_error() is too Renesas specific. Let's just call it
check_retune() to make it also easier understandable what it does.
Only a rename, no functional change.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/renesas_sdhi_core.c | 2 +-
 drivers/mmc/host/tmio_mmc.h          | 2 +-
 drivers/mmc/host/tmio_mmc_core.c     | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index d63aeb35bd0b..24ee8ac1fe21 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -899,7 +899,7 @@ int renesas_sdhi_probe(struct platform_device *pdev,
 
 		host->execute_tuning = renesas_sdhi_execute_tuning;
 		host->select_tuning = renesas_sdhi_select_tuning;
-		host->check_scc_error = renesas_sdhi_check_scc_error;
+		host->check_retune = renesas_sdhi_check_scc_error;
 		host->prepare_hs400_tuning =
 			renesas_sdhi_prepare_hs400_tuning;
 		host->hs400_downgrade = renesas_sdhi_disable_scc;
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index bfebbe368f02..bdb9973981ff 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -176,7 +176,7 @@ struct tmio_mmc_host {
 	int (*write16_hook)(struct tmio_mmc_host *host, int addr);
 	void (*reset)(struct tmio_mmc_host *host);
 	void (*hw_reset)(struct tmio_mmc_host *host);
-	bool (*check_scc_error)(struct tmio_mmc_host *host);
+	bool (*check_retune)(struct tmio_mmc_host *host);
 
 	/*
 	 * Mandatory callback for tuning to occur which is optional for SDR50
diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 593f88cafb6e..3cacb516a66e 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -818,8 +818,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);
 
-	/* SCC error means retune, but executed command was still successful */
-	if (host->check_scc_error && host->check_scc_error(host))
+	/* Error means retune, but executed command was still successful */
+	if (host->check_retune && host->check_retune(host))
 		mmc_retune_needed(host->mmc);
 
 	/* If SET_BLOCK_COUNT, continue with main command */
-- 
2.20.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [RFC PATCH 4/6] mmc: tmio: enforce retune after runtime suspend
  2020-01-29 20:37 [RFC PATCH 0/6] mmc: tmio: move TAP handling to SDHI driver Wolfram Sang
                   ` (2 preceding siblings ...)
  2020-01-29 20:37 ` [RFC PATCH 3/6] mmc: tmio: give callback a generic name Wolfram Sang
@ 2020-01-29 20:37 ` Wolfram Sang
  2020-02-10 23:53   ` Niklas Söderlund
  2020-01-29 20:37 ` [RFC PATCH 5/6] mmc: tmio: factor out TAP usage Wolfram Sang
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 17+ messages in thread
From: Wolfram Sang @ 2020-01-29 20:37 UTC (permalink / raw)
  To: linux-mmc; +Cc: linux-renesas-soc, Yoshihiro Shimoda, Wolfram Sang

Currently, select_tuning() is called after RPM resume. But
select_tuning() needs some additional function calls to work correctly.
Instead of reimplementing the whole postprocessing, just enforce
retuning.

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

I couldn't trigger RPM suspend even with debug code. Shimoda-san said
it should only occur with removed cards which is not so easy with
soldered eMMC. For those cases, I think the aproach taken here is fine.
Needs more discussion, though, to make sure...

 drivers/mmc/host/renesas_sdhi_core.c | 1 -
 drivers/mmc/host/tmio_mmc.h          | 1 -
 drivers/mmc/host/tmio_mmc_core.c     | 8 +-------
 3 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index 24ee8ac1fe21..0c9e5e010bda 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -898,7 +898,6 @@ int renesas_sdhi_probe(struct platform_device *pdev,
 			dev_warn(&host->pdev->dev, "Unknown clock rate for tuning\n");
 
 		host->execute_tuning = renesas_sdhi_execute_tuning;
-		host->select_tuning = renesas_sdhi_select_tuning;
 		host->check_retune = renesas_sdhi_check_scc_error;
 		host->prepare_hs400_tuning =
 			renesas_sdhi_prepare_hs400_tuning;
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index bdb9973981ff..b6fffd3d2650 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -183,7 +183,6 @@ struct tmio_mmc_host {
 	 * and mandatory for SDR104.
 	 */
 	int (*execute_tuning)(struct tmio_mmc_host *host, u32 opcode);
-	int (*select_tuning)(struct tmio_mmc_host *host);
 
 	/* Tuning values: 1 for success, 0 for failure */
 	DECLARE_BITMAP(taps, BITS_PER_BYTE * sizeof(long));
diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 3cacb516a66e..aeafa1c68ce2 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -1302,11 +1302,6 @@ int tmio_mmc_host_runtime_suspend(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(tmio_mmc_host_runtime_suspend);
 
-static bool tmio_mmc_can_retune(struct tmio_mmc_host *host)
-{
-	return host->tap_num && mmc_can_retune(host->mmc);
-}
-
 int tmio_mmc_host_runtime_resume(struct device *dev)
 {
 	struct tmio_mmc_host *host = dev_get_drvdata(dev);
@@ -1323,8 +1318,7 @@ int tmio_mmc_host_runtime_resume(struct device *dev)
 
 	tmio_mmc_enable_dma(host, true);
 
-	if (tmio_mmc_can_retune(host) && host->select_tuning(host))
-		dev_warn(&host->pdev->dev, "Tuning selection failed\n");
+	mmc_retune_needed(host->mmc);
 
 	return 0;
 }
-- 
2.20.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [RFC PATCH 5/6] mmc: tmio: factor out TAP usage
  2020-01-29 20:37 [RFC PATCH 0/6] mmc: tmio: move TAP handling to SDHI driver Wolfram Sang
                   ` (3 preceding siblings ...)
  2020-01-29 20:37 ` [RFC PATCH 4/6] mmc: tmio: enforce retune after runtime suspend Wolfram Sang
@ 2020-01-29 20:37 ` Wolfram Sang
  2020-02-10 23:57   ` Niklas Söderlund
  2020-01-29 20:37 ` [RFC PATCH 6/6] mmc: tmio: remove superfluous callback wrappers Wolfram Sang
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 17+ messages in thread
From: Wolfram Sang @ 2020-01-29 20:37 UTC (permalink / raw)
  To: linux-mmc; +Cc: linux-renesas-soc, Yoshihiro Shimoda, Wolfram Sang

TAPs are Renesas SDHI specific. Now that we moved all handling to the
SDHI core, we can also move the definitions from the TMIO struct to the
SDHI one.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/renesas_sdhi.h      |  5 ++++
 drivers/mmc/host/renesas_sdhi_core.c | 38 ++++++++++++++--------------
 drivers/mmc/host/tmio_mmc.h          |  5 ----
 3 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi.h b/drivers/mmc/host/renesas_sdhi.h
index 11a0b2bca3aa..7a1a741547f2 100644
--- a/drivers/mmc/host/renesas_sdhi.h
+++ b/drivers/mmc/host/renesas_sdhi.h
@@ -58,6 +58,11 @@ struct renesas_sdhi {
 	u32 scc_tappos;
 	u32 scc_tappos_hs400;
 	bool doing_tune;
+
+	/* Tuning values: 1 for success, 0 for failure */
+	DECLARE_BITMAP(taps, BITS_PER_BYTE * sizeof(long));
+	unsigned int tap_num;
+	unsigned long tap_set;
 };
 
 #define host_to_priv(host) \
diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index 0c9e5e010bda..22eaabe513d0 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -354,7 +354,7 @@ static void renesas_sdhi_hs400_complete(struct tmio_mmc_host *host)
 
 	if (priv->quirks && priv->quirks->hs400_4taps)
 		sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET,
-			       host->tap_set / 2);
+			       priv->tap_set / 2);
 
 	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_CKSEL,
 		       SH_MOBILE_SDHI_SCC_CKSEL_DTSEL |
@@ -438,11 +438,11 @@ 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 < host->tap_num * 2; i++) {
-		int offset = host->tap_num * (i < host->tap_num ? 1 : -1);
+	for (i = 0; i < priv->tap_num * 2; i++) {
+		int offset = priv->tap_num * (i < priv->tap_num ? 1 : -1);
 
-		if (!test_bit(i, host->taps))
-			clear_bit(i + offset, host->taps);
+		if (!test_bit(i, priv->taps))
+			clear_bit(i + offset, priv->taps);
 	}
 
 	/*
@@ -454,8 +454,8 @@ static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host)
 	ntap = 0;
 	tap_start = 0;
 	tap_end = 0;
-	for (i = 0; i < host->tap_num * 2; i++) {
-		if (test_bit(i, host->taps)) {
+	for (i = 0; i < priv->tap_num * 2; i++) {
+		if (test_bit(i, priv->taps)) {
 			ntap++;
 		} else {
 			if (ntap > tap_cnt) {
@@ -474,12 +474,12 @@ static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host)
 	}
 
 	if (tap_cnt >= SH_MOBILE_SDHI_MAX_TAP)
-		host->tap_set = (tap_start + tap_end) / 2 % host->tap_num;
+		priv->tap_set = (tap_start + tap_end) / 2 % priv->tap_num;
 	else
 		return -EIO;
 
 	/* Set SCC */
-	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, host->tap_set);
+	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, priv->tap_set);
 
 	/* Enable auto re-tuning */
 	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL,
@@ -494,27 +494,27 @@ static int renesas_sdhi_execute_tuning(struct tmio_mmc_host *host, u32 opcode)
 	struct renesas_sdhi *priv = host_to_priv(host);
 	int i, ret;
 
-	host->tap_num = renesas_sdhi_init_tuning(host);
-	if (!host->tap_num)
+	priv->tap_num = renesas_sdhi_init_tuning(host);
+	if (!priv->tap_num)
 		return 0; /* Tuning is not supported */
 
-	if (host->tap_num * 2 >= sizeof(host->taps) * BITS_PER_BYTE) {
+	if (priv->tap_num * 2 >= sizeof(priv->taps) * BITS_PER_BYTE) {
 		dev_err(&host->pdev->dev,
 			"Too many taps, please update 'taps' in tmio_mmc_host!\n");
 		return -EINVAL;
 	}
 
 	priv->doing_tune = true;
-	bitmap_zero(host->taps, host->tap_num * 2);
+	bitmap_zero(priv->taps, priv->tap_num * 2);
 
 	/* Issue CMD19 twice for each tap */
-	for (i = 0; i < 2 * host->tap_num; i++) {
+	for (i = 0; i < 2 * priv->tap_num; i++) {
 		/* Set sampling clock position */
-		sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, i % host->tap_num);
+		sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, i % priv->tap_num);
 
 		ret = mmc_send_tuning(host->mmc, opcode, NULL);
 		if (ret == 0)
-			set_bit(i, host->taps);
+			set_bit(i, priv->taps);
 	}
 
 	return renesas_sdhi_select_tuning(host);
@@ -523,7 +523,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 long new_tap = host->tap_set;
+	unsigned long new_tap = priv->tap_set;
 	u32 val;
 
 	val = sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_RVSREQ);
@@ -560,9 +560,9 @@ static bool renesas_sdhi_manual_correction(struct tmio_mmc_host *host, bool use_
 			return false;
 	}
 
-	host->tap_set = (new_tap % host->tap_num);
+	priv->tap_set = (new_tap % priv->tap_num);
 	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET,
-		       host->tap_set / (use_4tap ? 2 : 1));
+		       priv->tap_set / (use_4tap ? 2 : 1));
 
 	return false;
 }
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index b6fffd3d2650..b4cf10109162 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -184,11 +184,6 @@ struct tmio_mmc_host {
 	 */
 	int (*execute_tuning)(struct tmio_mmc_host *host, u32 opcode);
 
-	/* Tuning values: 1 for success, 0 for failure */
-	DECLARE_BITMAP(taps, BITS_PER_BYTE * sizeof(long));
-	unsigned int tap_num;
-	unsigned long tap_set;
-
 	void (*prepare_hs400_tuning)(struct tmio_mmc_host *host);
 	void (*hs400_downgrade)(struct tmio_mmc_host *host);
 	void (*hs400_complete)(struct tmio_mmc_host *host);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [RFC PATCH 6/6] mmc: tmio: remove superfluous callback wrappers
  2020-01-29 20:37 [RFC PATCH 0/6] mmc: tmio: move TAP handling to SDHI driver Wolfram Sang
                   ` (4 preceding siblings ...)
  2020-01-29 20:37 ` [RFC PATCH 5/6] mmc: tmio: factor out TAP usage Wolfram Sang
@ 2020-01-29 20:37 ` Wolfram Sang
  2020-02-11  0:02   ` Niklas Söderlund
  2020-02-05  0:22 ` [RFC PATCH 0/6] mmc: tmio: move TAP handling to SDHI driver Yoshihiro Shimoda
  2020-02-13 13:56 ` Ulf Hansson
  7 siblings, 1 reply; 17+ messages in thread
From: Wolfram Sang @ 2020-01-29 20:37 UTC (permalink / raw)
  To: linux-mmc; +Cc: linux-renesas-soc, Yoshihiro Shimoda, Wolfram Sang

After various refactoring, we can populate the mmc_ops callbacks
directly and don't need to have wrappers for them anymore.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/renesas_sdhi_core.c | 18 ++++++++++------
 drivers/mmc/host/tmio_mmc_core.c     | 32 +---------------------------
 2 files changed, 12 insertions(+), 38 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index 22eaabe513d0..0f07cc1aee34 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -321,8 +321,9 @@ static unsigned int renesas_sdhi_init_tuning(struct tmio_mmc_host *host)
 		SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_MASK;
 }
 
-static void renesas_sdhi_hs400_complete(struct tmio_mmc_host *host)
+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);
 
 	sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, ~CLK_CTL_SCLKEN &
@@ -376,8 +377,9 @@ static void renesas_sdhi_reset_scc(struct tmio_mmc_host *host,
 				     SH_MOBILE_SDHI_SCC_CKSEL));
 }
 
-static void renesas_sdhi_disable_scc(struct tmio_mmc_host *host)
+static void renesas_sdhi_disable_scc(struct mmc_host *mmc)
 {
+	struct tmio_mmc_host *host = mmc_priv(mmc);
 	struct renesas_sdhi *priv = host_to_priv(host);
 
 	renesas_sdhi_reset_scc(host, priv);
@@ -412,9 +414,12 @@ static void renesas_sdhi_reset_hs400_mode(struct tmio_mmc_host *host,
 			sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
 }
 
-static void renesas_sdhi_prepare_hs400_tuning(struct tmio_mmc_host *host)
+static int renesas_sdhi_prepare_hs400_tuning(struct mmc_host *mmc, struct mmc_ios *ios)
 {
+	struct tmio_mmc_host *host = mmc_priv(mmc);
+
 	renesas_sdhi_reset_hs400_mode(host, host_to_priv(host));
+	return 0;
 }
 
 #define SH_MOBILE_SDHI_MAX_TAP 3
@@ -899,10 +904,9 @@ int renesas_sdhi_probe(struct platform_device *pdev,
 
 		host->execute_tuning = renesas_sdhi_execute_tuning;
 		host->check_retune = renesas_sdhi_check_scc_error;
-		host->prepare_hs400_tuning =
-			renesas_sdhi_prepare_hs400_tuning;
-		host->hs400_downgrade = renesas_sdhi_disable_scc;
-		host->hs400_complete = renesas_sdhi_hs400_complete;
+		host->ops.prepare_hs400_tuning = renesas_sdhi_prepare_hs400_tuning;
+		host->ops.hs400_downgrade = renesas_sdhi_disable_scc;
+		host->ops.hs400_complete = renesas_sdhi_hs400_complete;
 	}
 
 	num_irqs = platform_irq_count(pdev);
diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index aeafa1c68ce2..a2415fb5dde0 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -997,34 +997,7 @@ static int tmio_multi_io_quirk(struct mmc_card *card,
 	return blk_size;
 }
 
-static int tmio_mmc_prepare_hs400_tuning(struct mmc_host *mmc,
-					 struct mmc_ios *ios)
-{
-	struct tmio_mmc_host *host = mmc_priv(mmc);
-
-	if (host->prepare_hs400_tuning)
-		host->prepare_hs400_tuning(host);
-
-	return 0;
-}
-
-static void tmio_mmc_hs400_downgrade(struct mmc_host *mmc)
-{
-	struct tmio_mmc_host *host = mmc_priv(mmc);
-
-	if (host->hs400_downgrade)
-		host->hs400_downgrade(host);
-}
-
-static void tmio_mmc_hs400_complete(struct mmc_host *mmc)
-{
-	struct tmio_mmc_host *host = mmc_priv(mmc);
-
-	if (host->hs400_complete)
-		host->hs400_complete(host);
-}
-
-static const struct mmc_host_ops tmio_mmc_ops = {
+static struct mmc_host_ops tmio_mmc_ops = {
 	.request	= tmio_mmc_request,
 	.set_ios	= tmio_mmc_set_ios,
 	.get_ro         = tmio_mmc_get_ro,
@@ -1033,9 +1006,6 @@ static const struct mmc_host_ops tmio_mmc_ops = {
 	.multi_io_quirk	= tmio_multi_io_quirk,
 	.hw_reset	= tmio_mmc_hw_reset,
 	.execute_tuning = tmio_mmc_execute_tuning,
-	.prepare_hs400_tuning = tmio_mmc_prepare_hs400_tuning,
-	.hs400_downgrade = tmio_mmc_hs400_downgrade,
-	.hs400_complete	= tmio_mmc_hs400_complete,
 };
 
 static int tmio_mmc_init_ocr(struct tmio_mmc_host *host)
-- 
2.20.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [RFC PATCH 0/6] mmc: tmio: move TAP handling to SDHI driver
  2020-01-29 20:37 [RFC PATCH 0/6] mmc: tmio: move TAP handling to SDHI driver Wolfram Sang
                   ` (5 preceding siblings ...)
  2020-01-29 20:37 ` [RFC PATCH 6/6] mmc: tmio: remove superfluous callback wrappers Wolfram Sang
@ 2020-02-05  0:22 ` Yoshihiro Shimoda
  2020-02-13 13:56 ` Ulf Hansson
  7 siblings, 0 replies; 17+ messages in thread
From: Yoshihiro Shimoda @ 2020-02-05  0:22 UTC (permalink / raw)
  To: Wolfram Sang, linux-mmc; +Cc: linux-renesas-soc

Hi Wolfram-san,

> From: Wolfram Sang, Sent: Thursday, January 30, 2020 5:37 AM
> 
> TAP and SCC handling is Renesas specific, so it should be moved to the
> SDHI driver. After previous refactoring, this is possible now. And
> feasible, too, to simplify further HS400 corrections. IMHO it also makes
> the driver less complicated.
> 
> See the patches why this series is still RFC.
> 
> This is based on top of the series:
> [RFC PATCH v2 0/5] mmc: renesas_sdhi: add manual correction
> 
> A branch can be found here:
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/sdhi/new_manual_calib
> 
> It has been tested with a Renesas Salvator-XS boards, one with a R-Car
> M3-N and one with H3-ES2.0. Tuning to HS400 still works.

Thank you for the refactoring!
This patch series looks good to me. So,

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

Best regards,
Yoshihiro Shimoda


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC PATCH 1/6] mmc: tmio: refactor tuning execution into SDHI driver
  2020-01-29 20:37 ` [RFC PATCH 1/6] mmc: tmio: refactor tuning execution into " Wolfram Sang
@ 2020-02-10 22:44   ` Niklas Söderlund
  2020-02-11  9:07     ` Wolfram Sang
  0 siblings, 1 reply; 17+ messages in thread
From: Niklas Söderlund @ 2020-02-10 22:44 UTC (permalink / raw)
  To: Wolfram Sang; +Cc: linux-mmc, linux-renesas-soc, Yoshihiro Shimoda

Hi Wolfram,

Thanks for your work.

On 2020-01-29 21:37:04 +0100, Wolfram Sang wrote:
> Move Renesas specific code for executing the tuning with a SCC into the
> SDHI driver and leave only a generic call in the TMIO driver. Simplify
> the code a little by removing init_tuning() and prepare_tuning()
> callbacks. The latter is directly folded into the new execute_tuning()
> callbacks.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/mmc/host/renesas_sdhi_core.c | 45 ++++++++++++++++++++--------
>  drivers/mmc/host/tmio_mmc.h          |  3 +-
>  drivers/mmc/host/tmio_mmc_core.c     | 33 +++-----------------
>  3 files changed, 37 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
> index 6a112454ca26..b3ab66f963f8 100644
> --- a/drivers/mmc/host/renesas_sdhi_core.c
> +++ b/drivers/mmc/host/renesas_sdhi_core.c
> @@ -321,17 +321,6 @@ static unsigned int renesas_sdhi_init_tuning(struct tmio_mmc_host *host)
>  		SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_MASK;
>  }
>  
> -static void renesas_sdhi_prepare_tuning(struct tmio_mmc_host *host,
> -					unsigned long tap)
> -{
> -	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);
> -}
> -
>  static void renesas_sdhi_hs400_complete(struct tmio_mmc_host *host)
>  {
>  	struct renesas_sdhi *priv = host_to_priv(host);
> @@ -500,6 +489,37 @@ static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host)
>  	return 0;
>  }
>  
> +static int renesas_sdhi_execute_tuning(struct tmio_mmc_host *host, u32 opcode)
> +{
> +	struct renesas_sdhi *priv = host_to_priv(host);
> +	int i, ret;
> +
> +	host->tap_num = renesas_sdhi_init_tuning(host);
> +	if (!host->tap_num)
> +		return 0; /* Tuning is not supported */
> +
> +	if (host->tap_num * 2 >= sizeof(host->taps) * BITS_PER_BYTE) {
> +		dev_warn_once(&host->pdev->dev,
> +			"Too many taps, skipping tuning. Please consider updating size of taps field of tmio_mmc_host\n");
> +		return 0;
> +	}
> +
> +	priv->doing_tune = true;
> +	bitmap_zero(host->taps, host->tap_num * 2);
> +
> +	/* Issue CMD19 twice for each tap */
> +	for (i = 0; i < 2 * host->tap_num; i++) {
> +		/* Set sampling clock position */
> +		sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, i % host->tap_num);
> +
> +		ret = mmc_send_tuning(host->mmc, opcode, NULL);
> +		if (ret == 0)

The variable ret is only used here after the refactor so you could 
possibly drop it and just check mmc_send_tuning() == 0. With or without 
this small nit addressed,

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> +			set_bit(i, host->taps);
> +	}
> +
> +	return renesas_sdhi_select_tuning(host);
> +}
> +
>  static bool renesas_sdhi_manual_correction(struct tmio_mmc_host *host, bool use_4tap)
>  {
>  	struct renesas_sdhi *priv = host_to_priv(host);
> @@ -877,8 +897,7 @@ int renesas_sdhi_probe(struct platform_device *pdev,
>  		if (!hit)
>  			dev_warn(&host->pdev->dev, "Unknown clock rate for tuning\n");
>  
> -		host->init_tuning = renesas_sdhi_init_tuning;
> -		host->prepare_tuning = renesas_sdhi_prepare_tuning;
> +		host->execute_tuning = renesas_sdhi_execute_tuning;
>  		host->select_tuning = renesas_sdhi_select_tuning;
>  		host->check_scc_error = renesas_sdhi_check_scc_error;
>  		host->prepare_hs400_tuning =
> diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> index c5ba13fae399..bfebbe368f02 100644
> --- a/drivers/mmc/host/tmio_mmc.h
> +++ b/drivers/mmc/host/tmio_mmc.h
> @@ -176,14 +176,13 @@ struct tmio_mmc_host {
>  	int (*write16_hook)(struct tmio_mmc_host *host, int addr);
>  	void (*reset)(struct tmio_mmc_host *host);
>  	void (*hw_reset)(struct tmio_mmc_host *host);
> -	void (*prepare_tuning)(struct tmio_mmc_host *host, unsigned long tap);
>  	bool (*check_scc_error)(struct tmio_mmc_host *host);
>  
>  	/*
>  	 * Mandatory callback for tuning to occur which is optional for SDR50
>  	 * and mandatory for SDR104.
>  	 */
> -	unsigned int (*init_tuning)(struct tmio_mmc_host *host);
> +	int (*execute_tuning)(struct tmio_mmc_host *host, u32 opcode);
>  	int (*select_tuning)(struct tmio_mmc_host *host);
>  
>  	/* Tuning values: 1 for success, 0 for failure */
> diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
> index c4a1d49fbea4..593f88cafb6e 100644
> --- a/drivers/mmc/host/tmio_mmc_core.c
> +++ b/drivers/mmc/host/tmio_mmc_core.c
> @@ -718,38 +718,13 @@ static int tmio_mmc_start_data(struct tmio_mmc_host *host,
>  static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
>  {
>  	struct tmio_mmc_host *host = mmc_priv(mmc);
> -	int i, ret = 0;
> -
> -	if (!host->init_tuning || !host->select_tuning)
> -		/* Tuning is not supported */
> -		goto out;
> -
> -	host->tap_num = host->init_tuning(host);
> -	if (!host->tap_num)
> -		/* Tuning is not supported */
> -		goto out;
> -
> -	if (host->tap_num * 2 >= sizeof(host->taps) * BITS_PER_BYTE) {
> -		dev_warn_once(&host->pdev->dev,
> -			"Too many taps, skipping tuning. Please consider updating size of taps field of tmio_mmc_host\n");
> -		goto out;
> -	}
> -
> -	bitmap_zero(host->taps, host->tap_num * 2);
> -
> -	/* Issue CMD19 twice for each tap */
> -	for (i = 0; i < 2 * host->tap_num; i++) {
> -		if (host->prepare_tuning)
> -			host->prepare_tuning(host, i % host->tap_num);
> +	int ret;
>  
> -		ret = mmc_send_tuning(mmc, opcode, NULL);
> -		if (ret == 0)
> -			set_bit(i, host->taps);
> -	}
> +	if (!host->execute_tuning)
> +		return 0;
>  
> -	ret = host->select_tuning(host);
> +	ret = host->execute_tuning(host, opcode);
>  
> -out:
>  	if (ret < 0) {
>  		dev_warn(&host->pdev->dev, "Tuning procedure failed\n");
>  		tmio_mmc_hw_reset(mmc);
> -- 
> 2.20.1
> 

-- 
Regards,
Niklas Söderlund

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC PATCH 2/6] mmc: renesas_sdhi: complain loudly if driver needs update
  2020-01-29 20:37 ` [RFC PATCH 2/6] mmc: renesas_sdhi: complain loudly if driver needs update Wolfram Sang
@ 2020-02-10 22:47   ` Niklas Söderlund
  0 siblings, 0 replies; 17+ messages in thread
From: Niklas Söderlund @ 2020-02-10 22:47 UTC (permalink / raw)
  To: Wolfram Sang; +Cc: linux-mmc, linux-renesas-soc, Yoshihiro Shimoda

Hi Wolfram,

Thanks for your patch.

On 2020-01-29 21:37:05 +0100, Wolfram Sang wrote:
> When the tap array in the driver is too low, this is not a warning but
> an error. Also _once is not helpful, we should make sure it is
> prominently in the logs. It is safe to do this because this will only
> show up during SoC enablement when we a new SoCs needs more taps (if
> that ever will happen).
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> Note: 'unsigned long' seems big enough for a while. But, famous last
> words(tm). We could handle this at runtime by reallocating a bigger
> buffer. Very unsure if it is worth it, though.

I can not tell if it's worth doing this now or not. But if it's a error 
instead of a warning it will be easier to spot if we should have done so 
;-)

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> 
>  drivers/mmc/host/renesas_sdhi_core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
> index b3ab66f963f8..d63aeb35bd0b 100644
> --- a/drivers/mmc/host/renesas_sdhi_core.c
> +++ b/drivers/mmc/host/renesas_sdhi_core.c
> @@ -499,9 +499,9 @@ static int renesas_sdhi_execute_tuning(struct tmio_mmc_host *host, u32 opcode)
>  		return 0; /* Tuning is not supported */
>  
>  	if (host->tap_num * 2 >= sizeof(host->taps) * BITS_PER_BYTE) {
> -		dev_warn_once(&host->pdev->dev,
> -			"Too many taps, skipping tuning. Please consider updating size of taps field of tmio_mmc_host\n");
> -		return 0;
> +		dev_err(&host->pdev->dev,
> +			"Too many taps, please update 'taps' in tmio_mmc_host!\n");
> +		return -EINVAL;
>  	}
>  
>  	priv->doing_tune = true;
> -- 
> 2.20.1
> 

-- 
Regards,
Niklas Söderlund

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC PATCH 3/6] mmc: tmio: give callback a generic name
  2020-01-29 20:37 ` [RFC PATCH 3/6] mmc: tmio: give callback a generic name Wolfram Sang
@ 2020-02-10 22:51   ` Niklas Söderlund
  0 siblings, 0 replies; 17+ messages in thread
From: Niklas Söderlund @ 2020-02-10 22:51 UTC (permalink / raw)
  To: Wolfram Sang; +Cc: linux-mmc, linux-renesas-soc, Yoshihiro Shimoda

Hi Wolfram,

Thanks for your work.

On 2020-01-29 21:37:06 +0100, Wolfram Sang wrote:
> check_scc_error() is too Renesas specific. Let's just call it
> check_retune() to make it also easier understandable what it does.
> Only a rename, no functional change.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
>  drivers/mmc/host/renesas_sdhi_core.c | 2 +-
>  drivers/mmc/host/tmio_mmc.h          | 2 +-
>  drivers/mmc/host/tmio_mmc_core.c     | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
> index d63aeb35bd0b..24ee8ac1fe21 100644
> --- a/drivers/mmc/host/renesas_sdhi_core.c
> +++ b/drivers/mmc/host/renesas_sdhi_core.c
> @@ -899,7 +899,7 @@ int renesas_sdhi_probe(struct platform_device *pdev,
>  
>  		host->execute_tuning = renesas_sdhi_execute_tuning;
>  		host->select_tuning = renesas_sdhi_select_tuning;
> -		host->check_scc_error = renesas_sdhi_check_scc_error;
> +		host->check_retune = renesas_sdhi_check_scc_error;
>  		host->prepare_hs400_tuning =
>  			renesas_sdhi_prepare_hs400_tuning;
>  		host->hs400_downgrade = renesas_sdhi_disable_scc;
> diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> index bfebbe368f02..bdb9973981ff 100644
> --- a/drivers/mmc/host/tmio_mmc.h
> +++ b/drivers/mmc/host/tmio_mmc.h
> @@ -176,7 +176,7 @@ struct tmio_mmc_host {
>  	int (*write16_hook)(struct tmio_mmc_host *host, int addr);
>  	void (*reset)(struct tmio_mmc_host *host);
>  	void (*hw_reset)(struct tmio_mmc_host *host);
> -	bool (*check_scc_error)(struct tmio_mmc_host *host);
> +	bool (*check_retune)(struct tmio_mmc_host *host);
>  
>  	/*
>  	 * Mandatory callback for tuning to occur which is optional for SDR50
> diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
> index 593f88cafb6e..3cacb516a66e 100644
> --- a/drivers/mmc/host/tmio_mmc_core.c
> +++ b/drivers/mmc/host/tmio_mmc_core.c
> @@ -818,8 +818,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);
>  
> -	/* SCC error means retune, but executed command was still successful */
> -	if (host->check_scc_error && host->check_scc_error(host))
> +	/* Error means retune, but executed command was still successful */
> +	if (host->check_retune && host->check_retune(host))
>  		mmc_retune_needed(host->mmc);
>  
>  	/* If SET_BLOCK_COUNT, continue with main command */
> -- 
> 2.20.1
> 

-- 
Regards,
Niklas Söderlund

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC PATCH 4/6] mmc: tmio: enforce retune after runtime suspend
  2020-01-29 20:37 ` [RFC PATCH 4/6] mmc: tmio: enforce retune after runtime suspend Wolfram Sang
@ 2020-02-10 23:53   ` Niklas Söderlund
  0 siblings, 0 replies; 17+ messages in thread
From: Niklas Söderlund @ 2020-02-10 23:53 UTC (permalink / raw)
  To: Wolfram Sang; +Cc: linux-mmc, linux-renesas-soc, Yoshihiro Shimoda

Hi Wolfram,

Thanks for your patch.

On 2020-01-29 21:37:07 +0100, Wolfram Sang wrote:
> Currently, select_tuning() is called after RPM resume. But
> select_tuning() needs some additional function calls to work correctly.
> Instead of reimplementing the whole postprocessing, just enforce
> retuning.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> I couldn't trigger RPM suspend even with debug code. Shimoda-san said
> it should only occur with removed cards which is not so easy with
> soldered eMMC. For those cases, I think the aproach taken here is fine.
> Needs more discussion, though, to make sure...

I really like the change so I really wanted to provoke a suspend to be 
able to provide a tested by tag, but I have so far been unable to do so 
:-(

> 
>  drivers/mmc/host/renesas_sdhi_core.c | 1 -
>  drivers/mmc/host/tmio_mmc.h          | 1 -
>  drivers/mmc/host/tmio_mmc_core.c     | 8 +-------
>  3 files changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
> index 24ee8ac1fe21..0c9e5e010bda 100644
> --- a/drivers/mmc/host/renesas_sdhi_core.c
> +++ b/drivers/mmc/host/renesas_sdhi_core.c
> @@ -898,7 +898,6 @@ int renesas_sdhi_probe(struct platform_device *pdev,
>  			dev_warn(&host->pdev->dev, "Unknown clock rate for tuning\n");
>  
>  		host->execute_tuning = renesas_sdhi_execute_tuning;
> -		host->select_tuning = renesas_sdhi_select_tuning;
>  		host->check_retune = renesas_sdhi_check_scc_error;
>  		host->prepare_hs400_tuning =
>  			renesas_sdhi_prepare_hs400_tuning;
> diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> index bdb9973981ff..b6fffd3d2650 100644
> --- a/drivers/mmc/host/tmio_mmc.h
> +++ b/drivers/mmc/host/tmio_mmc.h
> @@ -183,7 +183,6 @@ struct tmio_mmc_host {
>  	 * and mandatory for SDR104.
>  	 */
>  	int (*execute_tuning)(struct tmio_mmc_host *host, u32 opcode);
> -	int (*select_tuning)(struct tmio_mmc_host *host);
>  
>  	/* Tuning values: 1 for success, 0 for failure */
>  	DECLARE_BITMAP(taps, BITS_PER_BYTE * sizeof(long));
> diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
> index 3cacb516a66e..aeafa1c68ce2 100644
> --- a/drivers/mmc/host/tmio_mmc_core.c
> +++ b/drivers/mmc/host/tmio_mmc_core.c
> @@ -1302,11 +1302,6 @@ int tmio_mmc_host_runtime_suspend(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(tmio_mmc_host_runtime_suspend);
>  
> -static bool tmio_mmc_can_retune(struct tmio_mmc_host *host)
> -{
> -	return host->tap_num && mmc_can_retune(host->mmc);
> -}
> -
>  int tmio_mmc_host_runtime_resume(struct device *dev)
>  {
>  	struct tmio_mmc_host *host = dev_get_drvdata(dev);
> @@ -1323,8 +1318,7 @@ int tmio_mmc_host_runtime_resume(struct device *dev)
>  
>  	tmio_mmc_enable_dma(host, true);
>  
> -	if (tmio_mmc_can_retune(host) && host->select_tuning(host))
> -		dev_warn(&host->pdev->dev, "Tuning selection failed\n");
> +	mmc_retune_needed(host->mmc);
>  
>  	return 0;
>  }
> -- 
> 2.20.1
> 

-- 
Regards,
Niklas Söderlund

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC PATCH 5/6] mmc: tmio: factor out TAP usage
  2020-01-29 20:37 ` [RFC PATCH 5/6] mmc: tmio: factor out TAP usage Wolfram Sang
@ 2020-02-10 23:57   ` Niklas Söderlund
  0 siblings, 0 replies; 17+ messages in thread
From: Niklas Söderlund @ 2020-02-10 23:57 UTC (permalink / raw)
  To: Wolfram Sang; +Cc: linux-mmc, linux-renesas-soc, Yoshihiro Shimoda

Hi Wolfram,

Thanks for your work.

On 2020-01-29 21:37:08 +0100, Wolfram Sang wrote:
> TAPs are Renesas SDHI specific. Now that we moved all handling to the
> SDHI core, we can also move the definitions from the TMIO struct to the
> SDHI one.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
>  drivers/mmc/host/renesas_sdhi.h      |  5 ++++
>  drivers/mmc/host/renesas_sdhi_core.c | 38 ++++++++++++++--------------
>  drivers/mmc/host/tmio_mmc.h          |  5 ----
>  3 files changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/mmc/host/renesas_sdhi.h b/drivers/mmc/host/renesas_sdhi.h
> index 11a0b2bca3aa..7a1a741547f2 100644
> --- a/drivers/mmc/host/renesas_sdhi.h
> +++ b/drivers/mmc/host/renesas_sdhi.h
> @@ -58,6 +58,11 @@ struct renesas_sdhi {
>  	u32 scc_tappos;
>  	u32 scc_tappos_hs400;
>  	bool doing_tune;
> +
> +	/* Tuning values: 1 for success, 0 for failure */
> +	DECLARE_BITMAP(taps, BITS_PER_BYTE * sizeof(long));
> +	unsigned int tap_num;
> +	unsigned long tap_set;
>  };
>  
>  #define host_to_priv(host) \
> diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
> index 0c9e5e010bda..22eaabe513d0 100644
> --- a/drivers/mmc/host/renesas_sdhi_core.c
> +++ b/drivers/mmc/host/renesas_sdhi_core.c
> @@ -354,7 +354,7 @@ static void renesas_sdhi_hs400_complete(struct tmio_mmc_host *host)
>  
>  	if (priv->quirks && priv->quirks->hs400_4taps)
>  		sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET,
> -			       host->tap_set / 2);
> +			       priv->tap_set / 2);
>  
>  	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_CKSEL,
>  		       SH_MOBILE_SDHI_SCC_CKSEL_DTSEL |
> @@ -438,11 +438,11 @@ 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 < host->tap_num * 2; i++) {
> -		int offset = host->tap_num * (i < host->tap_num ? 1 : -1);
> +	for (i = 0; i < priv->tap_num * 2; i++) {
> +		int offset = priv->tap_num * (i < priv->tap_num ? 1 : -1);
>  
> -		if (!test_bit(i, host->taps))
> -			clear_bit(i + offset, host->taps);
> +		if (!test_bit(i, priv->taps))
> +			clear_bit(i + offset, priv->taps);
>  	}
>  
>  	/*
> @@ -454,8 +454,8 @@ static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host)
>  	ntap = 0;
>  	tap_start = 0;
>  	tap_end = 0;
> -	for (i = 0; i < host->tap_num * 2; i++) {
> -		if (test_bit(i, host->taps)) {
> +	for (i = 0; i < priv->tap_num * 2; i++) {
> +		if (test_bit(i, priv->taps)) {
>  			ntap++;
>  		} else {
>  			if (ntap > tap_cnt) {
> @@ -474,12 +474,12 @@ static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host)
>  	}
>  
>  	if (tap_cnt >= SH_MOBILE_SDHI_MAX_TAP)
> -		host->tap_set = (tap_start + tap_end) / 2 % host->tap_num;
> +		priv->tap_set = (tap_start + tap_end) / 2 % priv->tap_num;
>  	else
>  		return -EIO;
>  
>  	/* Set SCC */
> -	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, host->tap_set);
> +	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, priv->tap_set);
>  
>  	/* Enable auto re-tuning */
>  	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_RVSCNTL,
> @@ -494,27 +494,27 @@ static int renesas_sdhi_execute_tuning(struct tmio_mmc_host *host, u32 opcode)
>  	struct renesas_sdhi *priv = host_to_priv(host);
>  	int i, ret;
>  
> -	host->tap_num = renesas_sdhi_init_tuning(host);
> -	if (!host->tap_num)
> +	priv->tap_num = renesas_sdhi_init_tuning(host);
> +	if (!priv->tap_num)
>  		return 0; /* Tuning is not supported */
>  
> -	if (host->tap_num * 2 >= sizeof(host->taps) * BITS_PER_BYTE) {
> +	if (priv->tap_num * 2 >= sizeof(priv->taps) * BITS_PER_BYTE) {
>  		dev_err(&host->pdev->dev,
>  			"Too many taps, please update 'taps' in tmio_mmc_host!\n");
>  		return -EINVAL;
>  	}
>  
>  	priv->doing_tune = true;
> -	bitmap_zero(host->taps, host->tap_num * 2);
> +	bitmap_zero(priv->taps, priv->tap_num * 2);
>  
>  	/* Issue CMD19 twice for each tap */
> -	for (i = 0; i < 2 * host->tap_num; i++) {
> +	for (i = 0; i < 2 * priv->tap_num; i++) {
>  		/* Set sampling clock position */
> -		sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, i % host->tap_num);
> +		sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET, i % priv->tap_num);
>  
>  		ret = mmc_send_tuning(host->mmc, opcode, NULL);
>  		if (ret == 0)
> -			set_bit(i, host->taps);
> +			set_bit(i, priv->taps);
>  	}
>  
>  	return renesas_sdhi_select_tuning(host);
> @@ -523,7 +523,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 long new_tap = host->tap_set;
> +	unsigned long new_tap = priv->tap_set;
>  	u32 val;
>  
>  	val = sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_RVSREQ);
> @@ -560,9 +560,9 @@ static bool renesas_sdhi_manual_correction(struct tmio_mmc_host *host, bool use_
>  			return false;
>  	}
>  
> -	host->tap_set = (new_tap % host->tap_num);
> +	priv->tap_set = (new_tap % priv->tap_num);
>  	sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TAPSET,
> -		       host->tap_set / (use_4tap ? 2 : 1));
> +		       priv->tap_set / (use_4tap ? 2 : 1));
>  
>  	return false;
>  }
> diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> index b6fffd3d2650..b4cf10109162 100644
> --- a/drivers/mmc/host/tmio_mmc.h
> +++ b/drivers/mmc/host/tmio_mmc.h
> @@ -184,11 +184,6 @@ struct tmio_mmc_host {
>  	 */
>  	int (*execute_tuning)(struct tmio_mmc_host *host, u32 opcode);
>  
> -	/* Tuning values: 1 for success, 0 for failure */
> -	DECLARE_BITMAP(taps, BITS_PER_BYTE * sizeof(long));
> -	unsigned int tap_num;
> -	unsigned long tap_set;
> -
>  	void (*prepare_hs400_tuning)(struct tmio_mmc_host *host);
>  	void (*hs400_downgrade)(struct tmio_mmc_host *host);
>  	void (*hs400_complete)(struct tmio_mmc_host *host);
> -- 
> 2.20.1
> 

-- 
Regards,
Niklas Söderlund

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC PATCH 6/6] mmc: tmio: remove superfluous callback wrappers
  2020-01-29 20:37 ` [RFC PATCH 6/6] mmc: tmio: remove superfluous callback wrappers Wolfram Sang
@ 2020-02-11  0:02   ` Niklas Söderlund
  2020-02-11  9:09     ` Wolfram Sang
  0 siblings, 1 reply; 17+ messages in thread
From: Niklas Söderlund @ 2020-02-11  0:02 UTC (permalink / raw)
  To: Wolfram Sang; +Cc: linux-mmc, linux-renesas-soc, Yoshihiro Shimoda

Hi Wolfram,

Thanks for your patch.

On 2020-01-29 21:37:09 +0100, Wolfram Sang wrote:
> After various refactoring, we can populate the mmc_ops callbacks
> directly and don't need to have wrappers for them anymore.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/mmc/host/renesas_sdhi_core.c | 18 ++++++++++------
>  drivers/mmc/host/tmio_mmc_core.c     | 32 +---------------------------
>  2 files changed, 12 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
> index 22eaabe513d0..0f07cc1aee34 100644
> --- a/drivers/mmc/host/renesas_sdhi_core.c
> +++ b/drivers/mmc/host/renesas_sdhi_core.c
> @@ -321,8 +321,9 @@ static unsigned int renesas_sdhi_init_tuning(struct tmio_mmc_host *host)
>  		SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_MASK;
>  }
>  
> -static void renesas_sdhi_hs400_complete(struct tmio_mmc_host *host)
> +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);
>  
>  	sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, ~CLK_CTL_SCLKEN &
> @@ -376,8 +377,9 @@ static void renesas_sdhi_reset_scc(struct tmio_mmc_host *host,
>  				     SH_MOBILE_SDHI_SCC_CKSEL));
>  }
>  
> -static void renesas_sdhi_disable_scc(struct tmio_mmc_host *host)
> +static void renesas_sdhi_disable_scc(struct mmc_host *mmc)
>  {
> +	struct tmio_mmc_host *host = mmc_priv(mmc);
>  	struct renesas_sdhi *priv = host_to_priv(host);
>  
>  	renesas_sdhi_reset_scc(host, priv);
> @@ -412,9 +414,12 @@ static void renesas_sdhi_reset_hs400_mode(struct tmio_mmc_host *host,
>  			sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
>  }
>  
> -static void renesas_sdhi_prepare_hs400_tuning(struct tmio_mmc_host *host)
> +static int renesas_sdhi_prepare_hs400_tuning(struct mmc_host *mmc, struct mmc_ios *ios)
>  {
> +	struct tmio_mmc_host *host = mmc_priv(mmc);
> +
>  	renesas_sdhi_reset_hs400_mode(host, host_to_priv(host));
> +	return 0;
>  }
>  
>  #define SH_MOBILE_SDHI_MAX_TAP 3
> @@ -899,10 +904,9 @@ int renesas_sdhi_probe(struct platform_device *pdev,
>  
>  		host->execute_tuning = renesas_sdhi_execute_tuning;
>  		host->check_retune = renesas_sdhi_check_scc_error;
> -		host->prepare_hs400_tuning =
> -			renesas_sdhi_prepare_hs400_tuning;
> -		host->hs400_downgrade = renesas_sdhi_disable_scc;
> -		host->hs400_complete = renesas_sdhi_hs400_complete;
> +		host->ops.prepare_hs400_tuning = renesas_sdhi_prepare_hs400_tuning;
> +		host->ops.hs400_downgrade = renesas_sdhi_disable_scc;

Would it make sens to rename renesas_sdhi_disable_scc() to 
renesas_sdhi_hs400_downgrade() to fit the pattern that it's called from 
the mmc_ops?

With or without this nit,

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> +		host->ops.hs400_complete = renesas_sdhi_hs400_complete;
>  	}
>  
>  	num_irqs = platform_irq_count(pdev);
> diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
> index aeafa1c68ce2..a2415fb5dde0 100644
> --- a/drivers/mmc/host/tmio_mmc_core.c
> +++ b/drivers/mmc/host/tmio_mmc_core.c
> @@ -997,34 +997,7 @@ static int tmio_multi_io_quirk(struct mmc_card *card,
>  	return blk_size;
>  }
>  
> -static int tmio_mmc_prepare_hs400_tuning(struct mmc_host *mmc,
> -					 struct mmc_ios *ios)
> -{
> -	struct tmio_mmc_host *host = mmc_priv(mmc);
> -
> -	if (host->prepare_hs400_tuning)
> -		host->prepare_hs400_tuning(host);
> -
> -	return 0;
> -}
> -
> -static void tmio_mmc_hs400_downgrade(struct mmc_host *mmc)
> -{
> -	struct tmio_mmc_host *host = mmc_priv(mmc);
> -
> -	if (host->hs400_downgrade)
> -		host->hs400_downgrade(host);
> -}
> -
> -static void tmio_mmc_hs400_complete(struct mmc_host *mmc)
> -{
> -	struct tmio_mmc_host *host = mmc_priv(mmc);
> -
> -	if (host->hs400_complete)
> -		host->hs400_complete(host);
> -}
> -
> -static const struct mmc_host_ops tmio_mmc_ops = {
> +static struct mmc_host_ops tmio_mmc_ops = {
>  	.request	= tmio_mmc_request,
>  	.set_ios	= tmio_mmc_set_ios,
>  	.get_ro         = tmio_mmc_get_ro,
> @@ -1033,9 +1006,6 @@ static const struct mmc_host_ops tmio_mmc_ops = {
>  	.multi_io_quirk	= tmio_multi_io_quirk,
>  	.hw_reset	= tmio_mmc_hw_reset,
>  	.execute_tuning = tmio_mmc_execute_tuning,
> -	.prepare_hs400_tuning = tmio_mmc_prepare_hs400_tuning,
> -	.hs400_downgrade = tmio_mmc_hs400_downgrade,
> -	.hs400_complete	= tmio_mmc_hs400_complete,
>  };
>  
>  static int tmio_mmc_init_ocr(struct tmio_mmc_host *host)
> -- 
> 2.20.1
> 

-- 
Regards,
Niklas Söderlund

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC PATCH 1/6] mmc: tmio: refactor tuning execution into SDHI driver
  2020-02-10 22:44   ` Niklas Söderlund
@ 2020-02-11  9:07     ` Wolfram Sang
  0 siblings, 0 replies; 17+ messages in thread
From: Wolfram Sang @ 2020-02-11  9:07 UTC (permalink / raw)
  To: Niklas Söderlund
  Cc: Wolfram Sang, linux-mmc, linux-renesas-soc, Yoshihiro Shimoda

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

Hi Niklas,

thanks for the review!

> > +		ret = mmc_send_tuning(host->mmc, opcode, NULL);
> > +		if (ret == 0)
> 
> The variable ret is only used here after the refactor so you could 
> possibly drop it and just check mmc_send_tuning() == 0. With or without 

Yeah, makes sense. I will fix it.

Regards,

   Wolfram


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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC PATCH 6/6] mmc: tmio: remove superfluous callback wrappers
  2020-02-11  0:02   ` Niklas Söderlund
@ 2020-02-11  9:09     ` Wolfram Sang
  0 siblings, 0 replies; 17+ messages in thread
From: Wolfram Sang @ 2020-02-11  9:09 UTC (permalink / raw)
  To: Niklas Söderlund
  Cc: Wolfram Sang, linux-mmc, linux-renesas-soc, Yoshihiro Shimoda

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


> > +		host->ops.prepare_hs400_tuning = renesas_sdhi_prepare_hs400_tuning;
> > +		host->ops.hs400_downgrade = renesas_sdhi_disable_scc;
> 
> Would it make sens to rename renesas_sdhi_disable_scc() to 
> renesas_sdhi_hs400_downgrade() to fit the pattern that it's called from 
> the mmc_ops?

I also thought about it and came to the conclusion that this is more
readable. To downgrade from HS400, we need to disable SCC. But no big
deal to change it if you/others think the other pattern...


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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC PATCH 0/6] mmc: tmio: move TAP handling to SDHI driver
  2020-01-29 20:37 [RFC PATCH 0/6] mmc: tmio: move TAP handling to SDHI driver Wolfram Sang
                   ` (6 preceding siblings ...)
  2020-02-05  0:22 ` [RFC PATCH 0/6] mmc: tmio: move TAP handling to SDHI driver Yoshihiro Shimoda
@ 2020-02-13 13:56 ` Ulf Hansson
  7 siblings, 0 replies; 17+ messages in thread
From: Ulf Hansson @ 2020-02-13 13:56 UTC (permalink / raw)
  To: Wolfram Sang; +Cc: linux-mmc, Linux-Renesas, Yoshihiro Shimoda

On Wed, 29 Jan 2020 at 21:37, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> TAP and SCC handling is Renesas specific, so it should be moved to the
> SDHI driver. After previous refactoring, this is possible now. And
> feasible, too, to simplify further HS400 corrections. IMHO it also makes
> the driver less complicated.
>
> See the patches why this series is still RFC.
>
> This is based on top of the series:
> [RFC PATCH v2 0/5] mmc: renesas_sdhi: add manual correction
>
> A branch can be found here:
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/sdhi/new_manual_calib
>
> It has been tested with a Renesas Salvator-XS boards, one with a R-Car
> M3-N and one with H3-ES2.0. Tuning to HS400 still works.
>
>
> Wolfram Sang (6):
>   mmc: tmio: refactor tuning execution into SDHI driver
>   mmc: renesas_sdhi: complain loudly if driver needs update
>   mmc: tmio: give callback a generic name
>   mmc: tmio: enforce retune after runtime suspend
>   mmc: tmio: factor out TAP usage
>   mmc: tmio: remove superfluous callback wrappers
>
>  drivers/mmc/host/renesas_sdhi.h      |  5 ++
>  drivers/mmc/host/renesas_sdhi_core.c | 90 +++++++++++++++++-----------
>  drivers/mmc/host/tmio_mmc.h          | 11 +---
>  drivers/mmc/host/tmio_mmc_core.c     | 77 +++---------------------
>  4 files changed, 71 insertions(+), 112 deletions(-)
>
> --
> 2.20.1
>

Wolfram, I realize that there were potentially a few minor nitpicks to
address, but let's do that on top.

So, applied for next, thanks!

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, back to index

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-29 20:37 [RFC PATCH 0/6] mmc: tmio: move TAP handling to SDHI driver Wolfram Sang
2020-01-29 20:37 ` [RFC PATCH 1/6] mmc: tmio: refactor tuning execution into " Wolfram Sang
2020-02-10 22:44   ` Niklas Söderlund
2020-02-11  9:07     ` Wolfram Sang
2020-01-29 20:37 ` [RFC PATCH 2/6] mmc: renesas_sdhi: complain loudly if driver needs update Wolfram Sang
2020-02-10 22:47   ` Niklas Söderlund
2020-01-29 20:37 ` [RFC PATCH 3/6] mmc: tmio: give callback a generic name Wolfram Sang
2020-02-10 22:51   ` Niklas Söderlund
2020-01-29 20:37 ` [RFC PATCH 4/6] mmc: tmio: enforce retune after runtime suspend Wolfram Sang
2020-02-10 23:53   ` Niklas Söderlund
2020-01-29 20:37 ` [RFC PATCH 5/6] mmc: tmio: factor out TAP usage Wolfram Sang
2020-02-10 23:57   ` Niklas Söderlund
2020-01-29 20:37 ` [RFC PATCH 6/6] mmc: tmio: remove superfluous callback wrappers Wolfram Sang
2020-02-11  0:02   ` Niklas Söderlund
2020-02-11  9:09     ` Wolfram Sang
2020-02-05  0:22 ` [RFC PATCH 0/6] mmc: tmio: move TAP handling to SDHI driver Yoshihiro Shimoda
2020-02-13 13:56 ` Ulf Hansson

Linux-mmc Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mmc/0 linux-mmc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mmc linux-mmc/ https://lore.kernel.org/linux-mmc \
		linux-mmc@vger.kernel.org
	public-inbox-index linux-mmc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-mmc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git