| Message ID | 1386680168-5227-2-git-send-email-b29396@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Dec 10, 2013 at 08:56:03PM +0800, Dong Aisheng wrote: > Currently the max_discard_to is simply got by (1 << 27) / host->timeout_clk > which is assumed to be the maximum timeout value, however, some platforms > maximum timeout counter may not be 1 << 27, e.g. i.MX uSDHC is 1 << 28. > Thus 1 << 27 may not be correct for such platforms. > > It is also possible that other platforms may have different problems. > To be flexible, we add a get_max_timeout hook to get the correct > maximum timeout value for these platforms. > > Signed-off-by: Dong Aisheng <b29396@freescale.com> > Reported-by: Ed Sutter <ed.sutter@alcatel-lucent.com> > --- > drivers/mmc/host/sdhci.c | 5 ++++- > drivers/mmc/host/sdhci.h | 1 + > 2 files changed, 5 insertions(+), 1 deletions(-) > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index bd8a098..464d42c 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -2930,7 +2930,10 @@ int sdhci_add_host(struct sdhci_host *host) > if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK) > host->timeout_clk = mmc->f_max / 1000; > > - mmc->max_discard_to = (1 << 27) / host->timeout_clk; > + if (host->ops->get_max_timeout) > + mmc->max_discard_to = host->ops->get_max_timeout(host); Does "timeout" conceptually equals to "discard_to"? If not, we might want to write it in the either form below to avoid messing these two concepts. mmc->max_discard_to = host->ops->get_max_timeout(host) / host->timeout_clk; or mmc->max_discard_to = host->ops->get_max_discard_to(host); I guess you may want to go for the second one. Shawn > + else > + mmc->max_discard_to = (1 << 27) / host->timeout_clk; > > mmc->caps |= MMC_CAP_SDIO_IRQ | MMC_CAP_ERASE | MMC_CAP_CMD23; > > diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h > index 0a3ed01..1591cbb 100644 > --- a/drivers/mmc/host/sdhci.h > +++ b/drivers/mmc/host/sdhci.h > @@ -281,6 +281,7 @@ struct sdhci_ops { > unsigned int (*get_max_clock)(struct sdhci_host *host); > unsigned int (*get_min_clock)(struct sdhci_host *host); > unsigned int (*get_timeout_clock)(struct sdhci_host *host); > + unsigned int (*get_max_timeout)(struct sdhci_host *host); > int (*platform_bus_width)(struct sdhci_host *host, > int width); > void (*platform_send_init_74_clocks)(struct sdhci_host *host, > -- > 1.7.2.rc3 > >
On Wed, Dec 11, 2013 at 9:56 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > On Tue, Dec 10, 2013 at 08:56:03PM +0800, Dong Aisheng wrote: >> Currently the max_discard_to is simply got by (1 << 27) / host->timeout_clk >> which is assumed to be the maximum timeout value, however, some platforms >> maximum timeout counter may not be 1 << 27, e.g. i.MX uSDHC is 1 << 28. >> Thus 1 << 27 may not be correct for such platforms. >> >> It is also possible that other platforms may have different problems. >> To be flexible, we add a get_max_timeout hook to get the correct >> maximum timeout value for these platforms. >> >> Signed-off-by: Dong Aisheng <b29396@freescale.com> >> Reported-by: Ed Sutter <ed.sutter@alcatel-lucent.com> >> --- >> drivers/mmc/host/sdhci.c | 5 ++++- >> drivers/mmc/host/sdhci.h | 1 + >> 2 files changed, 5 insertions(+), 1 deletions(-) >> >> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >> index bd8a098..464d42c 100644 >> --- a/drivers/mmc/host/sdhci.c >> +++ b/drivers/mmc/host/sdhci.c >> @@ -2930,7 +2930,10 @@ int sdhci_add_host(struct sdhci_host *host) >> if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK) >> host->timeout_clk = mmc->f_max / 1000; >> >> - mmc->max_discard_to = (1 << 27) / host->timeout_clk; >> + if (host->ops->get_max_timeout) >> + mmc->max_discard_to = host->ops->get_max_timeout(host); > > Does "timeout" conceptually equals to "discard_to"? If not, we might > want to write it in the either form below to avoid messing these two > concepts. > No, they're two concepts but the max_discard_to equals to the max timeout value the host supports. > mmc->max_discard_to = host->ops->get_max_timeout(host) / host->timeout_clk; > > or > > mmc->max_discard_to = host->ops->get_max_discard_to(host); > > I guess you may want to go for the second one. > The original way looks ok to me. Platform host driver does not need to know detail about discard, just tell the max timeout value it supports is ok. Common sdhci driver will handle it well. Regards Dong Aisheng > Shawn > >> + else >> + mmc->max_discard_to = (1 << 27) / host->timeout_clk; >> >> mmc->caps |= MMC_CAP_SDIO_IRQ | MMC_CAP_ERASE | MMC_CAP_CMD23; >> >> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h >> index 0a3ed01..1591cbb 100644 >> --- a/drivers/mmc/host/sdhci.h >> +++ b/drivers/mmc/host/sdhci.h >> @@ -281,6 +281,7 @@ struct sdhci_ops { >> unsigned int (*get_max_clock)(struct sdhci_host *host); >> unsigned int (*get_min_clock)(struct sdhci_host *host); >> unsigned int (*get_timeout_clock)(struct sdhci_host *host); >> + unsigned int (*get_max_timeout)(struct sdhci_host *host); >> int (*platform_bus_width)(struct sdhci_host *host, >> int width); >> void (*platform_send_init_74_clocks)(struct sdhci_host *host, >> -- >> 1.7.2.rc3 >> >> > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Dec 11, 2013 at 11:00:03AM +0800, Dong Aisheng wrote: > >> @@ -2930,7 +2930,10 @@ int sdhci_add_host(struct sdhci_host *host) > >> if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK) > >> host->timeout_clk = mmc->f_max / 1000; > >> > >> - mmc->max_discard_to = (1 << 27) / host->timeout_clk; > >> + if (host->ops->get_max_timeout) > >> + mmc->max_discard_to = host->ops->get_max_timeout(host); > > > > Does "timeout" conceptually equals to "discard_to"? If not, we might > > want to write it in the either form below to avoid messing these two > > concepts. > > > > No, they're two concepts but the max_discard_to equals to the max timeout value > the host supports. Does it? Shouldn't max_discard_to equals to max_timeout_value / timeout_clk? > > > mmc->max_discard_to = host->ops->get_max_timeout(host) / host->timeout_clk; > > > > or > > > > mmc->max_discard_to = host->ops->get_max_discard_to(host); > > > > I guess you may want to go for the second one. > > > > The original way looks ok to me. > Platform host driver does not need to know detail about discard, > just tell the max timeout value it supports is ok. > Common sdhci driver will handle it well. Well, looking at the patch #2, esdhc_get_max_timeout() returns max_to / (esdhc_pltfm_get_max_clock(host) / 1000) not just the max timeout value - max_to, so your platform host driver is knowing and handling the details about discard_ro, i.e. the discard_ro has to be max_timeout_value / timeout_clk. Shawn
On Wed, Dec 11, 2013 at 11:12 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > On Wed, Dec 11, 2013 at 11:00:03AM +0800, Dong Aisheng wrote: >> >> @@ -2930,7 +2930,10 @@ int sdhci_add_host(struct sdhci_host *host) >> >> if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK) >> >> host->timeout_clk = mmc->f_max / 1000; >> >> >> >> - mmc->max_discard_to = (1 << 27) / host->timeout_clk; >> >> + if (host->ops->get_max_timeout) >> >> + mmc->max_discard_to = host->ops->get_max_timeout(host); >> > >> > Does "timeout" conceptually equals to "discard_to"? If not, we might >> > want to write it in the either form below to avoid messing these two >> > concepts. >> > >> >> No, they're two concepts but the max_discard_to equals to the max timeout value >> the host supports. > > Does it? Shouldn't max_discard_to equals to max_timeout_value / timeout_clk? > Yes, you probably are confused that get_max_timeout return timeout in miliseconds directly. Do not need to do get_max_timeout(host) /timeout_clk. >> >> > mmc->max_discard_to = host->ops->get_max_timeout(host) / host->timeout_clk; >> > >> > or >> > >> > mmc->max_discard_to = host->ops->get_max_discard_to(host); >> > >> > I guess you may want to go for the second one. >> > >> >> The original way looks ok to me. >> Platform host driver does not need to know detail about discard, >> just tell the max timeout value it supports is ok. >> Common sdhci driver will handle it well. > > Well, looking at the patch #2, esdhc_get_max_timeout() returns > > max_to / (esdhc_pltfm_get_max_clock(host) / 1000) > > not just the max timeout value - max_to, so your platform host driver > is knowing and handling the details about discard_ro, i.e. the > discard_ro has to be max_timeout_value / timeout_clk. > No, the max_to is the max timeout counter value, you need to divide it by the timeout clock to get the timeout time. The defined of this API is return the max timeout value in miliseconds, so you need to divide the clock by 1000. None of these handles the detail bout discard_to. Regards Dong Aisheng > Shawn >
On Wed, Dec 11, 2013 at 11:20:59AM +0800, Dong Aisheng wrote: > No, the max_to is the max timeout counter value, you need to divide it > by the timeout clock > to get the timeout time. > The defined of this API is return the max timeout value in > miliseconds, so you need to > divide the clock by 1000. > None of these handles the detail bout discard_to. Let me start it over again. Here is basically what your patch does. - mmc->max_discard_to = (1 << 27) / host->timeout_clk; + if (host->ops->get_max_timeout) + mmc->max_discard_to = host->ops->get_max_timeout(host); The only thing that does not work for you in the existing code is the (1 << 27) part, right? If so, why not just create a platform hook to return the correct thing for you platform, i.e. (1 << 28)? if (host->ops->get_max_timeout) mmc->max_discard_to = host->ops->hook_foo(host); unsigned int esdhc_hook_foo(struct sdhci_host *host) { struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); struct pltfm_imx_data *imx_data = pltfm_host->priv; return esdhc_is_usdhc(imx_data) ? 1 << 28 : 1 << 27; } Such patch will be ease to be understood and less offensive to the existing code, no? Shawn
On Wed, Dec 11, 2013 at 11:56 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > On Wed, Dec 11, 2013 at 11:20:59AM +0800, Dong Aisheng wrote: >> No, the max_to is the max timeout counter value, you need to divide it >> by the timeout clock >> to get the timeout time. >> The defined of this API is return the max timeout value in >> miliseconds, so you need to >> divide the clock by 1000. >> None of these handles the detail bout discard_to. > > Let me start it over again. Here is basically what your patch does. > > - mmc->max_discard_to = (1 << 27) / host->timeout_clk; > + if (host->ops->get_max_timeout) > + mmc->max_discard_to = host->ops->get_max_timeout(host); > > The only thing that does not work for you in the existing code is the > (1 << 27) part, right? > > If so, why not just create a platform hook to return the correct thing > for you platform, i.e. (1 << 28)? > > if (host->ops->get_max_timeout) > mmc->max_discard_to = host->ops->hook_foo(host); > > unsigned int esdhc_hook_foo(struct sdhci_host *host) > { > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > struct pltfm_imx_data *imx_data = pltfm_host->priv; > > return esdhc_is_usdhc(imx_data) ? 1 << 28 : 1 << 27; > } > > Such patch will be ease to be understood and less offensive to the > existing code, no? > The example you give is not correct here. The max timeout counter returned by esdhc_hook_foo can not be simpled assigned to max_discard_to which is timeout in miliseconds. Actually what i did is like the way you said: - mmc->max_discard_to = (1 << 27) / host->timeout_clk; + if (host->ops->get_max_timeout) + mmc->max_discard_to = host->ops->get_max_timeout(host); + else + mmc->max_discard_to = (1 << 27) / host->timeout_clk; +unsigned int esdhc_get_max_timeout(struct sdhci_host *host) +{ + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); + struct pltfm_imx_data *imx_data = pltfm_host->priv; + u32 max_to = esdhc_is_usdhc(imx_data) ? 1 << 28 : 1 << 27; + + return max_to / (esdhc_pltfm_get_max_clock(host) / 1000); +} Regards Dong Aisheng > Shawn >
On Wed, Dec 11, 2013 at 12:59:55PM +0800, Dong Aisheng wrote: > On Wed, Dec 11, 2013 at 11:56 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > > On Wed, Dec 11, 2013 at 11:20:59AM +0800, Dong Aisheng wrote: > >> No, the max_to is the max timeout counter value, you need to divide it > >> by the timeout clock > >> to get the timeout time. > >> The defined of this API is return the max timeout value in > >> miliseconds, so you need to > >> divide the clock by 1000. > >> None of these handles the detail bout discard_to. > > > > Let me start it over again. Here is basically what your patch does. > > > > - mmc->max_discard_to = (1 << 27) / host->timeout_clk; > > + if (host->ops->get_max_timeout) > > + mmc->max_discard_to = host->ops->get_max_timeout(host); > > > > The only thing that does not work for you in the existing code is the > > (1 << 27) part, right? > > > > If so, why not just create a platform hook to return the correct thing > > for you platform, i.e. (1 << 28)? > > > > if (host->ops->get_max_timeout) > > mmc->max_discard_to = host->ops->hook_foo(host); > > > > unsigned int esdhc_hook_foo(struct sdhci_host *host) > > { > > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > > struct pltfm_imx_data *imx_data = pltfm_host->priv; > > > > return esdhc_is_usdhc(imx_data) ? 1 << 28 : 1 << 27; > > } > > > > Such patch will be ease to be understood and less offensive to the > > existing code, no? > > > > The example you give is not correct here. Sorry. Yes, there is a error in my example code. What about this? mmc->max_discard_to = host->ops->hook_foo ? host->ops->hook_foo(host) : 1 << 27; mmc->max_discard_to /= host->timeout_clk; Shawn
On Wed, Dec 11, 2013 at 1:55 PM, Shawn Guo <shawn.guo@linaro.org> wrote: > On Wed, Dec 11, 2013 at 12:59:55PM +0800, Dong Aisheng wrote: >> On Wed, Dec 11, 2013 at 11:56 AM, Shawn Guo <shawn.guo@linaro.org> wrote: >> > On Wed, Dec 11, 2013 at 11:20:59AM +0800, Dong Aisheng wrote: >> >> No, the max_to is the max timeout counter value, you need to divide it >> >> by the timeout clock >> >> to get the timeout time. >> >> The defined of this API is return the max timeout value in >> >> miliseconds, so you need to >> >> divide the clock by 1000. >> >> None of these handles the detail bout discard_to. >> > >> > Let me start it over again. Here is basically what your patch does. >> > >> > - mmc->max_discard_to = (1 << 27) / host->timeout_clk; >> > + if (host->ops->get_max_timeout) >> > + mmc->max_discard_to = host->ops->get_max_timeout(host); >> > >> > The only thing that does not work for you in the existing code is the >> > (1 << 27) part, right? >> > >> > If so, why not just create a platform hook to return the correct thing >> > for you platform, i.e. (1 << 28)? >> > >> > if (host->ops->get_max_timeout) >> > mmc->max_discard_to = host->ops->hook_foo(host); >> > >> > unsigned int esdhc_hook_foo(struct sdhci_host *host) >> > { >> > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >> > struct pltfm_imx_data *imx_data = pltfm_host->priv; >> > >> > return esdhc_is_usdhc(imx_data) ? 1 << 28 : 1 << 27; >> > } >> > >> > Such patch will be ease to be understood and less offensive to the >> > existing code, no? >> > >> >> The example you give is not correct here. > > Sorry. Yes, there is a error in my example code. What about this? > > mmc->max_discard_to = host->ops->hook_foo ? host->ops->hook_foo(host) : 1 << 27; > mmc->max_discard_to /= host->timeout_clk; > Actually i did like this for FSL internal tree. Just because one concern that other SoCs may have different case e.g. for imx5, the max count becomes 1 << 26 when DDR is enabled. Then this way becomes unwork. And i don't know how many other special SoC exists working on different way. So i prefer-ed a more flexible way in this patch. However, after one more thought, since the issue of the example i gave can be addressed with my patch 5 (able to update max_discard_to when DDR enabled). And we already have timeout_clk hook, so it seems we could also go this way. Regards Dong Aisheng > Shawn >
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index bd8a098..464d42c 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -2930,7 +2930,10 @@ int sdhci_add_host(struct sdhci_host *host) if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK) host->timeout_clk = mmc->f_max / 1000; - mmc->max_discard_to = (1 << 27) / host->timeout_clk; + if (host->ops->get_max_timeout) + mmc->max_discard_to = host->ops->get_max_timeout(host); + else + mmc->max_discard_to = (1 << 27) / host->timeout_clk; mmc->caps |= MMC_CAP_SDIO_IRQ | MMC_CAP_ERASE | MMC_CAP_CMD23; diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h index 0a3ed01..1591cbb 100644 --- a/drivers/mmc/host/sdhci.h +++ b/drivers/mmc/host/sdhci.h @@ -281,6 +281,7 @@ struct sdhci_ops { unsigned int (*get_max_clock)(struct sdhci_host *host); unsigned int (*get_min_clock)(struct sdhci_host *host); unsigned int (*get_timeout_clock)(struct sdhci_host *host); + unsigned int (*get_max_timeout)(struct sdhci_host *host); int (*platform_bus_width)(struct sdhci_host *host, int width); void (*platform_send_init_74_clocks)(struct sdhci_host *host,
Currently the max_discard_to is simply got by (1 << 27) / host->timeout_clk which is assumed to be the maximum timeout value, however, some platforms maximum timeout counter may not be 1 << 27, e.g. i.MX uSDHC is 1 << 28. Thus 1 << 27 may not be correct for such platforms. It is also possible that other platforms may have different problems. To be flexible, we add a get_max_timeout hook to get the correct maximum timeout value for these platforms. Signed-off-by: Dong Aisheng <b29396@freescale.com> Reported-by: Ed Sutter <ed.sutter@alcatel-lucent.com> --- drivers/mmc/host/sdhci.c | 5 ++++- drivers/mmc/host/sdhci.h | 1 + 2 files changed, 5 insertions(+), 1 deletions(-)