| Message ID | 1489627251-93539-1-git-send-email-shawn.lin@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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, -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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, > > > >
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, > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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, >> > > > >
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. > >
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,