diff mbox

[v3] mmc: sdhci: clarify the get_timeout_clock callback

Message ID 1488512252-8452-1-git-send-email-shawn.lin@rock-chips.com (mailing list archive)
State New, archived
Headers show

Commit Message

Shawn Lin March 3, 2017, 3:37 a.m. UTC
Currently the get_timeout_clock callabck doesn't clearly
have a statment that it needs the variant drivers to return
the timeout clock rate in KHz if the SDHCI_TIMEOUT_CLK_UNIT
isn't present, otherwise the variant drivers should return it
in KHz. So actually sdhci-of-arasan return the wrong value found
by Anssi[1]. It's also very likely that further variant drivers which
are going to use this callback will be confused by this situation.
Given the fact that modem sdhci variant hosts are very prone to get
the timeout clock from common clock framework(actuall the only three
users did that), it's more nature to return the value in Hz and we
make a explicit comment there. Then we put the unit conversion inside
the sdhci core. Thus will improve the code and prevent further misues.

[1]: https://patchwork.kernel.org/patch/9569431/
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Anssi Hannula <anssi.hannula@bitwise.fi>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

---

Changes in v3:
- squash all the patches into one to keep bisectability
- fix wrong return value of sdhci-cadence
- slight improve the condition check to avoid the complaint of
  checkpatch

Changes in v2:
- fix the comment and make it return in Hz

 drivers/mmc/host/sdhci-cadence.c   |  4 ++--
 drivers/mmc/host/sdhci-of-arasan.c | 23 +----------------------
 drivers/mmc/host/sdhci.c           | 16 +++++++++-------
 drivers/mmc/host/sdhci.h           |  1 +
 4 files changed, 13 insertions(+), 31 deletions(-)

Comments

Masahiro Yamada March 3, 2017, 2:31 p.m. UTC | #1
2017-03-03 12:37 GMT+09:00 Shawn Lin <shawn.lin@rock-chips.com>:
> Currently the get_timeout_clock callabck doesn't clearly
> have a statment that it needs the variant drivers to return
> the timeout clock rate in KHz if the SDHCI_TIMEOUT_CLK_UNIT
> isn't present, otherwise the variant drivers should return it
> in KHz. So actually sdhci-of-arasan return the wrong value found
> by Anssi[1]. It's also very likely that further variant drivers which
> are going to use this callback will be confused by this situation.
> Given the fact that modem sdhci variant hosts are very prone to get
> the timeout clock from common clock framework(actuall the only three
> users did that), it's more nature to return the value in Hz and we
> make a explicit comment there. Then we put the unit conversion inside
> the sdhci core. Thus will improve the code and prevent further misues.
>
> [1]: https://patchwork.kernel.org/patch/9569431/
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Anssi Hannula <anssi.hannula@bitwise.fi>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>


For sdhci-cadence.c bit,

Acked-by: Masahiro Yamada <yamada.masahiro@socionext.com>


Thanks!
diff mbox

Patch

diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence.c
index 316cfec..7baeeaf 100644
--- a/drivers/mmc/host/sdhci-cadence.c
+++ b/drivers/mmc/host/sdhci-cadence.c
@@ -103,9 +103,9 @@  static unsigned int sdhci_cdns_get_timeout_clock(struct sdhci_host *host)
 {
 	/*
 	 * Cadence's spec says the Timeout Clock Frequency is the same as the
-	 * Base Clock Frequency.  Divide it by 1000 to return a value in kHz.
+	 * Base Clock Frequency.
 	 */
-	return host->max_clk / 1000;
+	return host->max_clk;
 }
 
 static void sdhci_cdns_set_uhs_signaling(struct sdhci_host *host,
diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index 410a55b..663467d 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -28,14 +28,8 @@ 
 #include "sdhci-pltfm.h"
 #include <linux/of.h>
 
-#define SDHCI_ARASAN_CLK_CTRL_OFFSET	0x2c
 #define SDHCI_ARASAN_VENDOR_REGISTER	0x78
-
 #define VENDOR_ENHANCED_STROBE		BIT(0)
-#define CLK_CTRL_TIMEOUT_SHIFT		16
-#define CLK_CTRL_TIMEOUT_MASK		(0xf << CLK_CTRL_TIMEOUT_SHIFT)
-#define CLK_CTRL_TIMEOUT_MIN_EXP	13
-
 #define PHY_CLK_TOO_SLOW_HZ		400000
 
 /*
@@ -161,21 +155,6 @@  static int sdhci_arasan_syscon_write(struct sdhci_host *host,
 	return ret;
 }
 
-static unsigned int sdhci_arasan_get_timeout_clock(struct sdhci_host *host)
-{
-	u32 div;
-	unsigned long freq;
-	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
-
-	div = readl(host->ioaddr + SDHCI_ARASAN_CLK_CTRL_OFFSET);
-	div = (div & CLK_CTRL_TIMEOUT_MASK) >> CLK_CTRL_TIMEOUT_SHIFT;
-
-	freq = clk_get_rate(pltfm_host->clk);
-	freq /= 1 << (CLK_CTRL_TIMEOUT_MIN_EXP + div);
-
-	return freq;
-}
-
 static void sdhci_arasan_set_clock(struct sdhci_host *host, unsigned int clock)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -290,7 +269,7 @@  static int sdhci_arasan_voltage_switch(struct mmc_host *mmc,
 static struct sdhci_ops sdhci_arasan_ops = {
 	.set_clock = sdhci_arasan_set_clock,
 	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
-	.get_timeout_clock = sdhci_arasan_get_timeout_clock,
+	.get_timeout_clock = sdhci_pltfm_clk_get_max_clock,
 	.set_bus_width = sdhci_set_bus_width,
 	.reset = sdhci_arasan_reset,
 	.set_uhs_signaling = sdhci_set_uhs_signaling,
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 6fdd7a7..f73494e 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -3289,20 +3289,22 @@  int sdhci_setup_host(struct sdhci_host *host)
 	if (!(host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK)) {
 		host->timeout_clk = (host->caps & SDHCI_TIMEOUT_CLK_MASK) >>
 					SDHCI_TIMEOUT_CLK_SHIFT;
+
+		if (host->caps & SDHCI_TIMEOUT_CLK_UNIT)
+			host->timeout_clk *= 1000;
+
 		if (host->timeout_clk == 0) {
-			if (host->ops->get_timeout_clock) {
-				host->timeout_clk =
-					host->ops->get_timeout_clock(host);
-			} else {
+			if (unlikely(!host->ops->get_timeout_clock)) {
 				pr_err("%s: Hardware doesn't specify timeout clock frequency.\n",
 					mmc_hostname(mmc));
 				ret = -ENODEV;
 				goto undma;
 			}
-		}
 
-		if (host->caps & SDHCI_TIMEOUT_CLK_UNIT)
-			host->timeout_clk *= 1000;
+			host->timeout_clk =
+				DIV_ROUND_UP(host->ops->get_timeout_clock(host),
+					     1000);
+		}
 
 		if (override_timeout_clk)
 			host->timeout_clk = override_timeout_clk;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index edf3adf..27c252c 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -547,6 +547,7 @@  struct sdhci_ops {
 	int		(*enable_dma)(struct sdhci_host *host);
 	unsigned int	(*get_max_clock)(struct sdhci_host *host);
 	unsigned int	(*get_min_clock)(struct sdhci_host *host);
+	/* get_timeout_clock should return clk rate in unit of Hz */
 	unsigned int	(*get_timeout_clock)(struct sdhci_host *host);
 	unsigned int	(*get_max_timeout_count)(struct sdhci_host *host);
 	void		(*set_timeout)(struct sdhci_host *host,