* [PATCH v4] mmc: sdhci: clarify the get_timeout_clock callback
@ 2017-03-16 1:20 Shawn Lin
2017-03-16 5:25 ` Jisheng Zhang
2017-03-16 8:31 ` Adrian Hunter
0 siblings, 2 replies; 7+ messages in thread
From: Shawn Lin @ 2017-03-16 1:20 UTC (permalink / raw)
To: Adrian Hunter, Ulf Hansson
Cc: linux-mmc, Shawn Lin, Anssi Hannula, Masahiro Yamada
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>
Acked-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v4:
- rebased on -next
- add Masahiro's ack for sdhci-cadence
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 | 17 +----------------
drivers/mmc/host/sdhci.c | 16 +++++++++-------
drivers/mmc/host/sdhci.h | 1 +
4 files changed, 13 insertions(+), 25 deletions(-)
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 1cfd7f9..c52f153 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -157,21 +157,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)
-{
- unsigned long freq;
- struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
-
- /* SDHCI timeout clock is in kHz */
- freq = DIV_ROUND_UP(clk_get_rate(pltfm_host->clk), 1000);
-
- /* or in MHz */
- if (host->caps & SDHCI_TIMEOUT_CLK_UNIT)
- freq = DIV_ROUND_UP(freq, 1000);
-
- return freq;
-}
-
static void sdhci_arasan_set_clock(struct sdhci_host *host, unsigned int clock)
{
struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -286,7 +271,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,
--
1.9.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v4] mmc: sdhci: clarify the get_timeout_clock callback
2017-03-16 1:20 [PATCH v4] mmc: sdhci: clarify the get_timeout_clock callback Shawn Lin
@ 2017-03-16 5:25 ` Jisheng Zhang
2017-03-16 7:16 ` Shawn Lin
2017-03-16 8:31 ` Adrian Hunter
1 sibling, 1 reply; 7+ messages in thread
From: Jisheng Zhang @ 2017-03-16 5:25 UTC (permalink / raw)
To: Shawn Lin
Cc: Adrian Hunter, Ulf Hansson, linux-mmc, Anssi Hannula,
Masahiro Yamada
On Thu, 16 Mar 2017 09:20:51 +0800 Shawn Lin wrote:
> 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/
I think we need a blank line here
> 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>
>
This blank line need to be removed.
> Acked-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
> Changes in v4:
> - rebased on -next
> - add Masahiro's ack for sdhci-cadence
>
> 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 | 17 +----------------
> drivers/mmc/host/sdhci.c | 16 +++++++++-------
> drivers/mmc/host/sdhci.h | 1 +
> 4 files changed, 13 insertions(+), 25 deletions(-)
>
> 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 1cfd7f9..c52f153 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -157,21 +157,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)
> -{
> - unsigned long freq;
> - struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> -
> - /* SDHCI timeout clock is in kHz */
> - freq = DIV_ROUND_UP(clk_get_rate(pltfm_host->clk), 1000);
> -
> - /* or in MHz */
> - if (host->caps & SDHCI_TIMEOUT_CLK_UNIT)
> - freq = DIV_ROUND_UP(freq, 1000);
> -
> - return freq;
> -}
> -
> static void sdhci_arasan_set_clock(struct sdhci_host *host, unsigned int clock)
> {
> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -286,7 +271,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,
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4] mmc: sdhci: clarify the get_timeout_clock callback
2017-03-16 5:25 ` Jisheng Zhang
@ 2017-03-16 7:16 ` Shawn Lin
0 siblings, 0 replies; 7+ messages in thread
From: Shawn Lin @ 2017-03-16 7:16 UTC (permalink / raw)
To: Jisheng Zhang
Cc: shawn.lin, Adrian Hunter, Ulf Hansson, linux-mmc, Anssi Hannula,
Masahiro Yamada
On 2017/3/16 13:25, Jisheng Zhang wrote:
> On Thu, 16 Mar 2017 09:20:51 +0800 Shawn Lin wrote:
>
>> 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/
>
> I think we need a blank line here
>
>> 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>
>>
>
> This blank line need to be removed.
okay, thanks.
Should I respin v5 or maybe Ulf could help amend these two
when applied if there is no other issues?
>
>> Acked-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>
>> Changes in v4:
>> - rebased on -next
>> - add Masahiro's ack for sdhci-cadence
>>
>> 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 | 17 +----------------
>> drivers/mmc/host/sdhci.c | 16 +++++++++-------
>> drivers/mmc/host/sdhci.h | 1 +
>> 4 files changed, 13 insertions(+), 25 deletions(-)
>>
>> 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 1cfd7f9..c52f153 100644
>> --- a/drivers/mmc/host/sdhci-of-arasan.c
>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
>> @@ -157,21 +157,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)
>> -{
>> - unsigned long freq;
>> - struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> -
>> - /* SDHCI timeout clock is in kHz */
>> - freq = DIV_ROUND_UP(clk_get_rate(pltfm_host->clk), 1000);
>> -
>> - /* or in MHz */
>> - if (host->caps & SDHCI_TIMEOUT_CLK_UNIT)
>> - freq = DIV_ROUND_UP(freq, 1000);
>> -
>> - return freq;
>> -}
>> -
>> static void sdhci_arasan_set_clock(struct sdhci_host *host, unsigned int clock)
>> {
>> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> @@ -286,7 +271,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,
>
>
>
>
--
Best Regards
Shawn Lin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4] mmc: sdhci: clarify the get_timeout_clock callback
2017-03-16 1:20 [PATCH v4] mmc: sdhci: clarify the get_timeout_clock callback Shawn Lin
2017-03-16 5:25 ` Jisheng Zhang
@ 2017-03-16 8:31 ` Adrian Hunter
2017-03-16 8:46 ` Masahiro Yamada
2017-03-16 8:53 ` Shawn Lin
1 sibling, 2 replies; 7+ messages in thread
From: Adrian Hunter @ 2017-03-16 8:31 UTC (permalink / raw)
To: Shawn Lin, Ulf Hansson; +Cc: linux-mmc, Anssi Hannula, Masahiro Yamada
On 16/03/17 03:20, Shawn Lin wrote:
> Currently the get_timeout_clock callabck doesn't clearly
callabck -> callback
> have a statment that it needs the variant drivers to return
statment -> statement
> the timeout clock rate in KHz if the SDHCI_TIMEOUT_CLK_UNIT
KHz -> kHz
> isn't present, otherwise the variant drivers should return it
> in KHz. So actually sdhci-of-arasan return the wrong value found
KHz -> MHz
> 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
modem -> modern
> the timeout clock from common clock framework(actuall the only three
framework( -> framework (
actuall -> actually
> users did that), it's more nature to return the value in Hz and we
nature -> natural
> make a explicit comment there. Then we put the unit conversion inside
> the sdhci core. Thus will improve the code and prevent further misues.
misues -> misuses
>
> [1]: https://patchwork.kernel.org/patch/9569431/
+ blank line
> 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>
>
- blank line
> Acked-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
> Changes in v4:
> - rebased on -next
> - add Masahiro's ack for sdhci-cadence
>
> 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 | 17 +----------------
> drivers/mmc/host/sdhci.c | 16 +++++++++-------
> drivers/mmc/host/sdhci.h | 1 +
> 4 files changed, 13 insertions(+), 25 deletions(-)
>
> 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 1cfd7f9..c52f153 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -157,21 +157,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)
> -{
> - unsigned long freq;
> - struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> -
> - /* SDHCI timeout clock is in kHz */
> - freq = DIV_ROUND_UP(clk_get_rate(pltfm_host->clk), 1000);
> -
> - /* or in MHz */
> - if (host->caps & SDHCI_TIMEOUT_CLK_UNIT)
> - freq = DIV_ROUND_UP(freq, 1000);
> -
> - return freq;
> -}
> -
> static void sdhci_arasan_set_clock(struct sdhci_host *host, unsigned int clock)
> {
> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -286,7 +271,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)) {
This is not a CPU hot path, so I don't want to use 'likely' or 'unlikely'.
> 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,
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4] mmc: sdhci: clarify the get_timeout_clock callback
2017-03-16 8:31 ` Adrian Hunter
@ 2017-03-16 8:46 ` Masahiro Yamada
2017-03-16 8:54 ` Shawn Lin
2017-03-16 8:53 ` Shawn Lin
1 sibling, 1 reply; 7+ messages in thread
From: Masahiro Yamada @ 2017-03-16 8:46 UTC (permalink / raw)
To: Adrian Hunter, Shawn Lin; +Cc: Ulf Hansson, linux-mmc, Anssi Hannula
Hi Shawn,
>
>> users did that), it's more nature to return the value in Hz and we
>
> nature -> natural
>
>> make a explicit comment there. Then we put the unit conversion inside
While you are at it,
a explicit -> an explicit
>> the sdhci core. Thus will improve the code and prevent further misues.
>
> misues -> misuses
>
>>
>> [1]: https://patchwork.kernel.org/patch/9569431/
>
> + blank line
>
>> Cc: Adrian Hunter <adrian.hunter@intel.com>
>> Cc: Anssi Hannula <anssi.hannula@bitwise.fi>
>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
When applied, my Cc can be dropped
because I had already given Acked-by.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4] mmc: sdhci: clarify the get_timeout_clock callback
2017-03-16 8:31 ` Adrian Hunter
2017-03-16 8:46 ` Masahiro Yamada
@ 2017-03-16 8:53 ` Shawn Lin
1 sibling, 0 replies; 7+ messages in thread
From: Shawn Lin @ 2017-03-16 8:53 UTC (permalink / raw)
To: Adrian Hunter
Cc: Ulf Hansson, shawn.lin, linux-mmc, Anssi Hannula, Masahiro Yamada
On 2017/3/16 16:31, Adrian Hunter wrote:
> On 16/03/17 03:20, Shawn Lin wrote:
>> Currently the get_timeout_clock callabck doesn't clearly
>
> callabck -> callback
>
>> have a statment that it needs the variant drivers to return
>
> statment -> statement
>
>> the timeout clock rate in KHz if the SDHCI_TIMEOUT_CLK_UNIT
>
> KHz -> kHz
>
>> isn't present, otherwise the variant drivers should return it
>> in KHz. So actually sdhci-of-arasan return the wrong value found
>
> KHz -> MHz
>
>
>> 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
>
> modem -> modern
>
>> the timeout clock from common clock framework(actuall the only three
>
> framework( -> framework (
>
> actuall -> actually
>
>
>> users did that), it's more nature to return the value in Hz and we
>
> nature -> natural
>
>> make a explicit comment there. Then we put the unit conversion inside
>> the sdhci core. Thus will improve the code and prevent further misues.
>
> misues -> misuses
>
>>
>> [1]: https://patchwork.kernel.org/patch/9569431/
>
> + blank line
>
>> 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>
>>
>
> - blank line
>
>> Acked-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>
>> Changes in v4:
>> - rebased on -next
>> - add Masahiro's ack for sdhci-cadence
>>
>> 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 | 17 +----------------
>> drivers/mmc/host/sdhci.c | 16 +++++++++-------
>> drivers/mmc/host/sdhci.h | 1 +
>> 4 files changed, 13 insertions(+), 25 deletions(-)
>>
>> 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 1cfd7f9..c52f153 100644
>> --- a/drivers/mmc/host/sdhci-of-arasan.c
>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
>> @@ -157,21 +157,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)
>> -{
>> - unsigned long freq;
>> - struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> -
>> - /* SDHCI timeout clock is in kHz */
>> - freq = DIV_ROUND_UP(clk_get_rate(pltfm_host->clk), 1000);
>> -
>> - /* or in MHz */
>> - if (host->caps & SDHCI_TIMEOUT_CLK_UNIT)
>> - freq = DIV_ROUND_UP(freq, 1000);
>> -
>> - return freq;
>> -}
>> -
>> static void sdhci_arasan_set_clock(struct sdhci_host *host, unsigned int clock)
>> {
>> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> @@ -286,7 +271,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)) {
>
> This is not a CPU hot path, so I don't want to use 'likely' or 'unlikely'.
will remove this and fix all the typo. :)
>
>> 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,
>>
>
>
>
>
--
Best Regards
Shawn Lin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4] mmc: sdhci: clarify the get_timeout_clock callback
2017-03-16 8:46 ` Masahiro Yamada
@ 2017-03-16 8:54 ` Shawn Lin
0 siblings, 0 replies; 7+ messages in thread
From: Shawn Lin @ 2017-03-16 8:54 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Adrian Hunter, shawn.lin, Ulf Hansson, linux-mmc, Anssi Hannula
On 2017/3/16 16:46, Masahiro Yamada wrote:
> Hi Shawn,
>
>
>>
>>> users did that), it's more nature to return the value in Hz and we
>>
>> nature -> natural
>>
>>> make a explicit comment there. Then we put the unit conversion inside
>
> While you are at it,
>
> a explicit -> an explicit
>
>
Okay, thanks.
>
>>> the sdhci core. Thus will improve the code and prevent further misues.
>>
>> misues -> misuses
>>
>>>
>>> [1]: https://patchwork.kernel.org/patch/9569431/
>>
>> + blank line
>>
>>> Cc: Adrian Hunter <adrian.hunter@intel.com>
>>> Cc: Anssi Hannula <anssi.hannula@bitwise.fi>
>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>
>
> When applied, my Cc can be dropped
> because I had already given Acked-by.
will remove it. Thanks.
>
>
--
Best Regards
Shawn Lin
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-03-16 8:55 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-16 1:20 [PATCH v4] mmc: sdhci: clarify the get_timeout_clock callback Shawn Lin
2017-03-16 5:25 ` Jisheng Zhang
2017-03-16 7:16 ` Shawn Lin
2017-03-16 8:31 ` Adrian Hunter
2017-03-16 8:46 ` Masahiro Yamada
2017-03-16 8:54 ` Shawn Lin
2017-03-16 8:53 ` Shawn Lin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox