[PATCH 0/6] mmc: sdhci: a few fixes on timeout and max_discard_to

[PATCH 0/6] mmc: sdhci: a few fixes on timeout and max_discard_to

[Dong Aisheng]

Patch 1~4 mainly fixes the issue that the max timeout counter for uSDHC is
1 << 28 rather than 1 << 27. 1~2 fix getting the max timeout counter
while 3~4 fix setting the max timeout.
Thus it introduces two new platform hook: get_max_timeout and set_timeout
for those platform which have different timeout setting.

This issue is firstly reported here by Ed Sutter:
http://www.spinics.net/lists/linux-mmc/msg23375.html
The root cause is the max_discard_to got from uSDHC is too small, only 677ms,
which cause the max_discard_bytes for eMMC is only 512, then the discard operation
of mkfs.ext3 for an eMMC card is too slow, just like dead.
With above patches, the issue can be fixed.

Patch 5~6 adds the capability to calcalute the max_discard_to dynamically
for SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK.

Originally the max_discard_to for a high speed sdhc card may be:
mmc1: new high speed SDHC card at address aaaa
mmc1: calculated max. discard sectors 49152 for timeout 1355 ms
After fix:
mmc1: new high speed SDHC card@address aaaa
mmc1: calculated max. discard sectors 712704 for timeout 5422 ms

It also improves the card discard performance a lot due to max_discard_sectors
increase a lot.

There's also discussion about remove max_discard_to limit here:
http://www.spinics.net/lists/linux-mmc/msg23395.html
But it does not help for uSDHC since we can observe data timeout
on a Toshiba SD3.0 cards if we do not disable data timeout interrupt.

Dong Aisheng (6):
  mmc: sdhci: add platfrom get_max_timeout hook
  mmc: sdhci-esdhc-imx: fix incorrect max_discard_to for uSDHC
  mmc: sdhci: add platform set_timeout hook
  mmc: sdhci-esdhc-imx: set the correct max timeout value for uSDHC
  mmc: sdhci: calculate max_discard_to dynamically for
    SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK
  mmc: sdhci-esdhc-imx: use actual_clock to calculate timeout

 drivers/mmc/host/sdhci-esdhc-imx.c |   22 ++++++++++++++++++
 drivers/mmc/host/sdhci.c           |   43 +++++++++++++++++++++++++++++------
 drivers/mmc/host/sdhci.h           |    3 ++
 3 files changed, 60 insertions(+), 8 deletions(-)

-- 
1.7.2.rc3

[PATCH 1/6] mmc: sdhci: add platfrom get_max_timeout hook

[Dong Aisheng]

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);
+	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

[PATCH 2/6] mmc: sdhci-esdhc-imx: fix incorrect max_discard_to for uSDHC

[Dong Aisheng]

The default sdhci code use the 1 << 27 as the max timeout counter to
to calculate the max_discard_to, however it's not correct for uSDHC
since its the max counter is 1 << 28.
Implement esdhc_get_max_timeout to handle it correctly.

Signed-off-by: Dong Aisheng <b29396@freescale.com>
Reported-by: Ed Sutter <ed.sutter@alcatel-lucent.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 461a4c3..97b35e1 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -859,6 +859,15 @@ static int esdhc_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
 	return esdhc_change_pinstate(host, uhs);
 }
 
+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);
+}
+
 static struct sdhci_ops sdhci_esdhc_ops = {
 	.read_l = esdhc_readl_le,
 	.read_w = esdhc_readw_le,
@@ -871,6 +880,7 @@ static struct sdhci_ops sdhci_esdhc_ops = {
 	.get_ro = esdhc_pltfm_get_ro,
 	.platform_bus_width = esdhc_pltfm_bus_width,
 	.set_uhs_signaling = esdhc_set_uhs_signaling,
+	.get_max_timeout = esdhc_get_max_timeout,
 };
 
 static const struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
-- 
1.7.2.rc3

[PATCH 3/6] mmc: sdhci: add platform set_timeout hook

[Dong Aisheng]

Currently the common code assume 0xE is the maximum timeout counter
value and use it to write into the timeout counter register.
However, it's fairly possible that the different SoCs may have
different register layout on the timeout counter register.
That means 0xE may not be the correct maximum timeout value, then
the 0xE becomes meaningless.

To be flexible, this patch provides another approach for platforms
to set the correct timeout on their way.

Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
 drivers/mmc/host/sdhci.c |   19 ++++++++++++++-----
 drivers/mmc/host/sdhci.h |    2 ++
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 464d42c..4cc3bd6 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -726,19 +726,28 @@ static void sdhci_set_transfer_irqs(struct sdhci_host *host)
 		sdhci_clear_set_irqs(host, dma_irqs, pio_irqs);
 }
 
-static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
+static void sdhci_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 {
 	u8 count;
+
+	if (host->ops->set_timeout) {
+		host->ops->set_timeout(host, cmd);
+	} else {
+		count = sdhci_calc_timeout(host, cmd);
+		sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
+	}
+}
+
+static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
+{
 	u8 ctrl;
 	struct mmc_data *data = cmd->data;
 	int ret;
 
 	WARN_ON(host->data);
 
-	if (data || (cmd->flags & MMC_RSP_BUSY)) {
-		count = sdhci_calc_timeout(host, cmd);
-		sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
-	}
+	if (data || (cmd->flags & MMC_RSP_BUSY))
+		sdhci_set_timeout(host, cmd);
 
 	if (!data)
 		return;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 1591cbb..a4851a0 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -282,6 +282,8 @@ struct sdhci_ops {
 	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);
+	void		(*set_timeout)(struct sdhci_host *host,
+						struct mmc_command *cmd);
 	int		(*platform_bus_width)(struct sdhci_host *host,
 					       int width);
 	void (*platform_send_init_74_clocks)(struct sdhci_host *host,
-- 
1.7.2.rc3

[PATCH 4/6] mmc: sdhci-esdhc-imx: set the correct max timeout value for uSDHC

[Dong Aisheng]

The default sdhci driver write 0xE into timeout counter register to
set the maximum timeout. The value is not correct for uSDHC since the
max counter value for uSDHC is 0xF.
Instead of using common timeout code in sdhci, we implement esdhc_set_timeout
to handle the difference between eSDHC and uSDHC.

Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 97b35e1..c24cb23 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -868,6 +868,16 @@ unsigned int esdhc_get_max_timeout(struct sdhci_host *host)
 	return max_to / (esdhc_pltfm_get_max_clock(host) / 1000);
 }
 
+void esdhc_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct pltfm_imx_data *imx_data = pltfm_host->priv;
+
+	/* use maximum timeout counter */
+	sdhci_writeb(host, esdhc_is_usdhc(imx_data) ? 0xF : 0xE,
+			SDHCI_TIMEOUT_CONTROL);
+}
+
 static struct sdhci_ops sdhci_esdhc_ops = {
 	.read_l = esdhc_readl_le,
 	.read_w = esdhc_readw_le,
@@ -881,6 +891,7 @@ static struct sdhci_ops sdhci_esdhc_ops = {
 	.platform_bus_width = esdhc_pltfm_bus_width,
 	.set_uhs_signaling = esdhc_set_uhs_signaling,
 	.get_max_timeout = esdhc_get_max_timeout,
+	.set_timeout = esdhc_set_timeout,
 };
 
 static const struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
-- 
1.7.2.rc3

[PATCH 5/6] mmc: sdhci: calculate max_discard_to dynamically for SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK

[Dong Aisheng]

For host controllers using SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK, since the card
clock is changed dynamically for different cards, it does not make sense
to use the maximum host clock to calculate max_discard_to which may lead
the max_discard_to to be much smaller than its capbility and affect the card
discard performance a lot.
e.g. the host clock is 200Mhz, but the card is working on 50Mhz. Then the
max_discard_to is only 1/4 of its real capbility.

In this patch, it uses the actual_clock to calculate the max_discard_to
dynamically as long as a new clock speed is set.

Tested with a high speed SDHC card shows:
Originally:
mmc1: new high speed SDHC card at address aaaa
mmc1: calculated max. discard sectors 49152 for timeout 1355 ms
Now:
mmc1: new high speed SDHC card at address aaaa
mmc1: calculated max. discard sectors 712704 for timeout 5422 ms
The max_discard_sectors will increase a lot which will also improve discard
performance a lot.

The one known limitation of this approach is that it does not cover the special
case for user changes the clock via sysfs, since the max_discard_to is only
initialised for one time during the mmc queue init.

Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
 drivers/mmc/host/sdhci.c |   27 +++++++++++++++++++++------
 1 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 4cc3bd6..9be8a79 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1143,14 +1143,14 @@ static void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 	unsigned long timeout;
 
 	if (clock && clock == host->clock)
-		return;
+		goto out;
 
 	host->mmc->actual_clock = 0;
 
 	if (host->ops->set_clock) {
 		host->ops->set_clock(host, clock);
 		if (host->quirks & SDHCI_QUIRK_NONSTANDARD_CLOCK)
-			return;
+			goto out;
 	}
 
 	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
@@ -1249,6 +1249,19 @@ clock_set:
 
 out:
 	host->clock = clock;
+
+	/* update timeout_clk and max_discard_to once the SDCLK is changed */
+	if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK && clock) {
+		host->timeout_clk = host->mmc->actual_clock ?
+					host->mmc->actual_clock / 1000 :
+					host->clock / 1000;
+		if (host->ops->get_max_timeout)
+			host->mmc->max_discard_to =
+					host->ops->get_max_timeout(host);
+		else
+			host->mmc->max_discard_to = (1 << 27) /
+					host->timeout_clk;
+	}
 }
 
 static inline void sdhci_update_clock(struct sdhci_host *host)
@@ -2939,10 +2952,12 @@ int sdhci_add_host(struct sdhci_host *host)
 	if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK)
 		host->timeout_clk = mmc->f_max / 1000;
 
-	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;
+	if (!(host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK)) {
+		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;
 
-- 
1.7.2.rc3

[PATCH 6/6] mmc: sdhci-esdhc-imx: use actual_clock to calculate timeout

[Dong Aisheng]

The timeout clock for uSDHC is SDCLK, so it's reasonable to use
the actual_clock to calculate the max timeout.

Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index c24cb23..83236d3 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -865,7 +865,8 @@ unsigned int esdhc_get_max_timeout(struct sdhci_host *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);
+	return host->mmc->actual_clock ?
+			max_to / (host->mmc->actual_clock / 1000) : 0;
 }
 
 void esdhc_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
-- 
1.7.2.rc3

[PATCH 1/6] mmc: sdhci: add platfrom get_max_timeout hook

[Shawn Guo]

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
> 
> 

[PATCH 2/6] mmc: sdhci-esdhc-imx: fix incorrect max_discard_to for uSDHC

[Shawn Guo]

On Tue, Dec 10, 2013 at 08:56:04PM +0800, Dong Aisheng wrote:
> The default sdhci code use the 1 << 27 as the max timeout counter to
> to calculate the max_discard_to, however it's not correct for uSDHC
> since its the max counter is 1 << 28.
> Implement esdhc_get_max_timeout to handle it correctly.
> 
> Signed-off-by: Dong Aisheng <b29396@freescale.com>
> Reported-by: Ed Sutter <ed.sutter@alcatel-lucent.com>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 461a4c3..97b35e1 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -859,6 +859,15 @@ static int esdhc_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
>  	return esdhc_change_pinstate(host, uhs);
>  }
>  
> +unsigned int esdhc_get_max_timeout(struct sdhci_host *host)

static?

Shawn

> +{
> +	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);
> +}
> +
>  static struct sdhci_ops sdhci_esdhc_ops = {
>  	.read_l = esdhc_readl_le,
>  	.read_w = esdhc_readw_le,
> @@ -871,6 +880,7 @@ static struct sdhci_ops sdhci_esdhc_ops = {
>  	.get_ro = esdhc_pltfm_get_ro,
>  	.platform_bus_width = esdhc_pltfm_bus_width,
>  	.set_uhs_signaling = esdhc_set_uhs_signaling,
> +	.get_max_timeout = esdhc_get_max_timeout,
>  };
>  
>  static const struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
> -- 
> 1.7.2.rc3
> 
> 

[PATCH 3/6] mmc: sdhci: add platform set_timeout hook

[Shawn Guo]

On Tue, Dec 10, 2013 at 08:56:05PM +0800, Dong Aisheng wrote:
> Currently the common code assume 0xE is the maximum timeout counter
> value and use it to write into the timeout counter register.
> However, it's fairly possible that the different SoCs may have
> different register layout on the timeout counter register.
> That means 0xE may not be the correct maximum timeout value, then
> the 0xE becomes meaningless.
> 
> To be flexible, this patch provides another approach for platforms
> to set the correct timeout on their way.
> 
> Signed-off-by: Dong Aisheng <b29396@freescale.com>
> ---
>  drivers/mmc/host/sdhci.c |   19 ++++++++++++++-----
>  drivers/mmc/host/sdhci.h |    2 ++
>  2 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 464d42c..4cc3bd6 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -726,19 +726,28 @@ static void sdhci_set_transfer_irqs(struct sdhci_host *host)
>  		sdhci_clear_set_irqs(host, dma_irqs, pio_irqs);
>  }
>  
> -static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
> +static void sdhci_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
>  {
>  	u8 count;
> +
> +	if (host->ops->set_timeout) {
> +		host->ops->set_timeout(host, cmd);
> +	} else {
> +		count = sdhci_calc_timeout(host, cmd);
> +		sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
> +	}
> +}
> +
> +static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
> +{
>  	u8 ctrl;
>  	struct mmc_data *data = cmd->data;
>  	int ret;
>  
>  	WARN_ON(host->data);
>  
> -	if (data || (cmd->flags & MMC_RSP_BUSY)) {
> -		count = sdhci_calc_timeout(host, cmd);

>From what I read the commit log, I think it might be more appropriate to
patch sdhci_calc_timeout() like the following?

	if (host->quirks & SDHCI_QUIRK_BROKEN_TIMEOUT_VAL)
		if (host->ops->get_max_timeout)
			return host->ops->get_max_timeout(host);
		else
			return 0xE;

Shawn

> -		sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
> -	}
> +	if (data || (cmd->flags & MMC_RSP_BUSY))
> +		sdhci_set_timeout(host, cmd);
>  
>  	if (!data)
>  		return;
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 1591cbb..a4851a0 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -282,6 +282,8 @@ struct sdhci_ops {
>  	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);
> +	void		(*set_timeout)(struct sdhci_host *host,
> +						struct mmc_command *cmd);
>  	int		(*platform_bus_width)(struct sdhci_host *host,
>  					       int width);
>  	void (*platform_send_init_74_clocks)(struct sdhci_host *host,
> -- 
> 1.7.2.rc3
> 
> 

[PATCH 4/6] mmc: sdhci-esdhc-imx: set the correct max timeout value for uSDHC

[Shawn Guo]

On Tue, Dec 10, 2013 at 08:56:06PM +0800, Dong Aisheng wrote:
> The default sdhci driver write 0xE into timeout counter register to
> set the maximum timeout. The value is not correct for uSDHC since the
> max counter value for uSDHC is 0xF.
> Instead of using common timeout code in sdhci, we implement esdhc_set_timeout
> to handle the difference between eSDHC and uSDHC.
> 
> Signed-off-by: Dong Aisheng <b29396@freescale.com>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 97b35e1..c24cb23 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -868,6 +868,16 @@ unsigned int esdhc_get_max_timeout(struct sdhci_host *host)
>  	return max_to / (esdhc_pltfm_get_max_clock(host) / 1000);
>  }
>  
> +void esdhc_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)

static?

Shawn

> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct pltfm_imx_data *imx_data = pltfm_host->priv;
> +
> +	/* use maximum timeout counter */
> +	sdhci_writeb(host, esdhc_is_usdhc(imx_data) ? 0xF : 0xE,
> +			SDHCI_TIMEOUT_CONTROL);
> +}
> +
>  static struct sdhci_ops sdhci_esdhc_ops = {
>  	.read_l = esdhc_readl_le,
>  	.read_w = esdhc_readw_le,
> @@ -881,6 +891,7 @@ static struct sdhci_ops sdhci_esdhc_ops = {
>  	.platform_bus_width = esdhc_pltfm_bus_width,
>  	.set_uhs_signaling = esdhc_set_uhs_signaling,
>  	.get_max_timeout = esdhc_get_max_timeout,
> +	.set_timeout = esdhc_set_timeout,
>  };
>  
>  static const struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
> -- 
> 1.7.2.rc3
> 
> 

[PATCH 1/6] mmc: sdhci: add platfrom get_max_timeout hook

[Dong Aisheng]

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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/6] mmc: sdhci: calculate max_discard_to dynamically for SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK

[Shawn Guo]

On Tue, Dec 10, 2013 at 08:56:07PM +0800, Dong Aisheng wrote:
> For host controllers using SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK, since the card
> clock is changed dynamically for different cards, it does not make sense
> to use the maximum host clock to calculate max_discard_to which may lead
> the max_discard_to to be much smaller than its capbility and affect the card
> discard performance a lot.
> e.g. the host clock is 200Mhz, but the card is working on 50Mhz. Then the
> max_discard_to is only 1/4 of its real capbility.
> 
> In this patch, it uses the actual_clock to calculate the max_discard_to
> dynamically as long as a new clock speed is set.
> 
> Tested with a high speed SDHC card shows:
> Originally:
> mmc1: new high speed SDHC card at address aaaa
> mmc1: calculated max. discard sectors 49152 for timeout 1355 ms
> Now:
> mmc1: new high speed SDHC card at address aaaa
> mmc1: calculated max. discard sectors 712704 for timeout 5422 ms
> The max_discard_sectors will increase a lot which will also improve discard
> performance a lot.
> 
> The one known limitation of this approach is that it does not cover the special
> case for user changes the clock via sysfs, since the max_discard_to is only
> initialised for one time during the mmc queue init.
> 
> Signed-off-by: Dong Aisheng <b29396@freescale.com>
> ---
>  drivers/mmc/host/sdhci.c |   27 +++++++++++++++++++++------
>  1 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 4cc3bd6..9be8a79 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1143,14 +1143,14 @@ static void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
>  	unsigned long timeout;
>  
>  	if (clock && clock == host->clock)
> -		return;
> +		goto out;

I do not quite understand this change.  Why do we need to recalculate
max_discard_to if the clock does not change since the last time that
the function is called?

Shawn

>  
>  	host->mmc->actual_clock = 0;
>  
>  	if (host->ops->set_clock) {
>  		host->ops->set_clock(host, clock);
>  		if (host->quirks & SDHCI_QUIRK_NONSTANDARD_CLOCK)
> -			return;
> +			goto out;
>  	}
>  
>  	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
> @@ -1249,6 +1249,19 @@ clock_set:
>  
>  out:
>  	host->clock = clock;
> +
> +	/* update timeout_clk and max_discard_to once the SDCLK is changed */
> +	if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK && clock) {
> +		host->timeout_clk = host->mmc->actual_clock ?
> +					host->mmc->actual_clock / 1000 :
> +					host->clock / 1000;
> +		if (host->ops->get_max_timeout)
> +			host->mmc->max_discard_to =
> +					host->ops->get_max_timeout(host);
> +		else
> +			host->mmc->max_discard_to = (1 << 27) /
> +					host->timeout_clk;
> +	}
>  }
>  
>  static inline void sdhci_update_clock(struct sdhci_host *host)
> @@ -2939,10 +2952,12 @@ int sdhci_add_host(struct sdhci_host *host)
>  	if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK)
>  		host->timeout_clk = mmc->f_max / 1000;
>  
> -	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;
> +	if (!(host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK)) {
> +		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;
>  
> -- 
> 1.7.2.rc3
> 
> 

[PATCH 3/6] mmc: sdhci: add platform set_timeout hook

[Dong Aisheng]

On Wed, Dec 11, 2013 at 10:16 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Tue, Dec 10, 2013 at 08:56:05PM +0800, Dong Aisheng wrote:
>> Currently the common code assume 0xE is the maximum timeout counter
>> value and use it to write into the timeout counter register.
>> However, it's fairly possible that the different SoCs may have
>> different register layout on the timeout counter register.
>> That means 0xE may not be the correct maximum timeout value, then
>> the 0xE becomes meaningless.
>>
>> To be flexible, this patch provides another approach for platforms
>> to set the correct timeout on their way.
>>
>> Signed-off-by: Dong Aisheng <b29396@freescale.com>
>> ---
>>  drivers/mmc/host/sdhci.c |   19 ++++++++++++++-----
>>  drivers/mmc/host/sdhci.h |    2 ++
>>  2 files changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 464d42c..4cc3bd6 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -726,19 +726,28 @@ static void sdhci_set_transfer_irqs(struct sdhci_host *host)
>>               sdhci_clear_set_irqs(host, dma_irqs, pio_irqs);
>>  }
>>
>> -static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
>> +static void sdhci_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
>>  {
>>       u8 count;
>> +
>> +     if (host->ops->set_timeout) {
>> +             host->ops->set_timeout(host, cmd);
>> +     } else {
>> +             count = sdhci_calc_timeout(host, cmd);
>> +             sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
>> +     }
>> +}
>> +
>> +static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
>> +{
>>       u8 ctrl;
>>       struct mmc_data *data = cmd->data;
>>       int ret;
>>
>>       WARN_ON(host->data);
>>
>> -     if (data || (cmd->flags & MMC_RSP_BUSY)) {
>> -             count = sdhci_calc_timeout(host, cmd);
>
> From what I read the commit log, I think it might be more appropriate to
> patch sdhci_calc_timeout() like the following?
>
>         if (host->quirks & SDHCI_QUIRK_BROKEN_TIMEOUT_VAL)
>                 if (host->ops->get_max_timeout)
>                         return host->ops->get_max_timeout(host);
>                 else
>                         return 0xE;
>

The return val of sdhci_calc_timeout is the register value to be
written into timeout counter register.
host->ops->get_max_timeout returns the max timeout in miliseconds directly.
So we can not do like that here.
They're two concepts.

Regards
Dong Aisheng

> Shawn
>
>> -             sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
>> -     }
>> +     if (data || (cmd->flags & MMC_RSP_BUSY))
>> +             sdhci_set_timeout(host, cmd);
>>
>>       if (!data)
>>               return;
>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>> index 1591cbb..a4851a0 100644
>> --- a/drivers/mmc/host/sdhci.h
>> +++ b/drivers/mmc/host/sdhci.h
>> @@ -282,6 +282,8 @@ struct sdhci_ops {
>>       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);
>> +     void            (*set_timeout)(struct sdhci_host *host,
>> +                                             struct mmc_command *cmd);
>>       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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/6] mmc: sdhci-esdhc-imx: set the correct max timeout value for uSDHC

[Dong Aisheng]

On Wed, Dec 11, 2013 at 10:17 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Tue, Dec 10, 2013 at 08:56:06PM +0800, Dong Aisheng wrote:
>> The default sdhci driver write 0xE into timeout counter register to
>> set the maximum timeout. The value is not correct for uSDHC since the
>> max counter value for uSDHC is 0xF.
>> Instead of using common timeout code in sdhci, we implement esdhc_set_timeout
>> to handle the difference between eSDHC and uSDHC.
>>
>> Signed-off-by: Dong Aisheng <b29396@freescale.com>
>> ---
>>  drivers/mmc/host/sdhci-esdhc-imx.c |   11 +++++++++++
>>  1 files changed, 11 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
>> index 97b35e1..c24cb23 100644
>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
>> @@ -868,6 +868,16 @@ unsigned int esdhc_get_max_timeout(struct sdhci_host *host)
>>       return max_to / (esdhc_pltfm_get_max_clock(host) / 1000);
>>  }
>>
>> +void esdhc_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
>
> static?
>

Sorry, i missed it when do copy&paste.
Will update it in v2 with another same issue you pointed.

Regards
Dong Aisheng

> Shawn
>
>> +{
>> +     struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +     struct pltfm_imx_data *imx_data = pltfm_host->priv;
>> +
>> +     /* use maximum timeout counter */
>> +     sdhci_writeb(host, esdhc_is_usdhc(imx_data) ? 0xF : 0xE,
>> +                     SDHCI_TIMEOUT_CONTROL);
>> +}
>> +
>>  static struct sdhci_ops sdhci_esdhc_ops = {
>>       .read_l = esdhc_readl_le,
>>       .read_w = esdhc_readw_le,
>> @@ -881,6 +891,7 @@ static struct sdhci_ops sdhci_esdhc_ops = {
>>       .platform_bus_width = esdhc_pltfm_bus_width,
>>       .set_uhs_signaling = esdhc_set_uhs_signaling,
>>       .get_max_timeout = esdhc_get_max_timeout,
>> +     .set_timeout = esdhc_set_timeout,
>>  };
>>
>>  static const struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
>> --
>> 1.7.2.rc3
>>
>>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/6] mmc: sdhci: add platfrom get_max_timeout hook

[Shawn Guo]

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

[PATCH 5/6] mmc: sdhci: calculate max_discard_to dynamically for SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK

[Dong Aisheng]

On Wed, Dec 11, 2013 at 11:01 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Tue, Dec 10, 2013 at 08:56:07PM +0800, Dong Aisheng wrote:
>> For host controllers using SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK, since the card
>> clock is changed dynamically for different cards, it does not make sense
>> to use the maximum host clock to calculate max_discard_to which may lead
>> the max_discard_to to be much smaller than its capbility and affect the card
>> discard performance a lot.
>> e.g. the host clock is 200Mhz, but the card is working on 50Mhz. Then the
>> max_discard_to is only 1/4 of its real capbility.
>>
>> In this patch, it uses the actual_clock to calculate the max_discard_to
>> dynamically as long as a new clock speed is set.
>>
>> Tested with a high speed SDHC card shows:
>> Originally:
>> mmc1: new high speed SDHC card at address aaaa
>> mmc1: calculated max. discard sectors 49152 for timeout 1355 ms
>> Now:
>> mmc1: new high speed SDHC card at address aaaa
>> mmc1: calculated max. discard sectors 712704 for timeout 5422 ms
>> The max_discard_sectors will increase a lot which will also improve discard
>> performance a lot.
>>
>> The one known limitation of this approach is that it does not cover the special
>> case for user changes the clock via sysfs, since the max_discard_to is only
>> initialised for one time during the mmc queue init.
>>
>> Signed-off-by: Dong Aisheng <b29396@freescale.com>
>> ---
>>  drivers/mmc/host/sdhci.c |   27 +++++++++++++++++++++------
>>  1 files changed, 21 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 4cc3bd6..9be8a79 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -1143,14 +1143,14 @@ static void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
>>       unsigned long timeout;
>>
>>       if (clock && clock == host->clock)
>> -             return;
>> +             goto out;
>
> I do not quite understand this change.  Why do we need to recalculate
> max_discard_to if the clock does not change since the last time that
> the function is called?
>

Good catch.
It's safe to return directly here.
Will update in v2.

Regards
Dong Aisheng

> Shawn
>
>>
>>       host->mmc->actual_clock = 0;
>>
>>       if (host->ops->set_clock) {
>>               host->ops->set_clock(host, clock);
>>               if (host->quirks & SDHCI_QUIRK_NONSTANDARD_CLOCK)
>> -                     return;
>> +                     goto out;
>>       }
>>
>>       sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
>> @@ -1249,6 +1249,19 @@ clock_set:
>>
>>  out:
>>       host->clock = clock;
>> +
>> +     /* update timeout_clk and max_discard_to once the SDCLK is changed */
>> +     if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK && clock) {
>> +             host->timeout_clk = host->mmc->actual_clock ?
>> +                                     host->mmc->actual_clock / 1000 :
>> +                                     host->clock / 1000;
>> +             if (host->ops->get_max_timeout)
>> +                     host->mmc->max_discard_to =
>> +                                     host->ops->get_max_timeout(host);
>> +             else
>> +                     host->mmc->max_discard_to = (1 << 27) /
>> +                                     host->timeout_clk;
>> +     }
>>  }
>>
>>  static inline void sdhci_update_clock(struct sdhci_host *host)
>> @@ -2939,10 +2952,12 @@ int sdhci_add_host(struct sdhci_host *host)
>>       if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK)
>>               host->timeout_clk = mmc->f_max / 1000;
>>
>> -     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;
>> +     if (!(host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK)) {
>> +             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;
>>
>> --
>> 1.7.2.rc3
>>
>>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/6] mmc: sdhci: add platform set_timeout hook

[Shawn Guo]

On Wed, Dec 11, 2013 at 11:03:31AM +0800, Dong Aisheng wrote:
> >> +static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
> >> +{
> >>       u8 ctrl;
> >>       struct mmc_data *data = cmd->data;
> >>       int ret;
> >>
> >>       WARN_ON(host->data);
> >>
> >> -     if (data || (cmd->flags & MMC_RSP_BUSY)) {
> >> -             count = sdhci_calc_timeout(host, cmd);
> >
> > From what I read the commit log, I think it might be more appropriate to
> > patch sdhci_calc_timeout() like the following?
> >
> >         if (host->quirks & SDHCI_QUIRK_BROKEN_TIMEOUT_VAL)
> >                 if (host->ops->get_max_timeout)
> >                         return host->ops->get_max_timeout(host);
> >                 else
> >                         return 0xE;
> >
> 
> The return val of sdhci_calc_timeout is the register value to be
> written into timeout counter register.
> host->ops->get_max_timeout returns the max timeout in miliseconds directly.
> So we can not do like that here.
> They're two concepts.

I did not make my comment clear.  The .get_max_timeout() is not the one
you defined in patch #1 any more, but something like the following for
esdhc driver, which returns correct timeout value not milliseconds.

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;

	return esdhc_is_usdhc(imx_data) ? 0xF : 0xE;

}

Does that match the problem you described in the commit log better?

Shawn

[PATCH 1/6] mmc: sdhci: add platfrom get_max_timeout hook

[Dong Aisheng]

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
>

[PATCH 3/6] mmc: sdhci: add platform set_timeout hook

[Dong Aisheng]

On Wed, Dec 11, 2013 at 11:18 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Wed, Dec 11, 2013 at 11:03:31AM +0800, Dong Aisheng wrote:
>> >> +static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
>> >> +{
>> >>       u8 ctrl;
>> >>       struct mmc_data *data = cmd->data;
>> >>       int ret;
>> >>
>> >>       WARN_ON(host->data);
>> >>
>> >> -     if (data || (cmd->flags & MMC_RSP_BUSY)) {
>> >> -             count = sdhci_calc_timeout(host, cmd);
>> >
>> > From what I read the commit log, I think it might be more appropriate to
>> > patch sdhci_calc_timeout() like the following?
>> >
>> >         if (host->quirks & SDHCI_QUIRK_BROKEN_TIMEOUT_VAL)
>> >                 if (host->ops->get_max_timeout)
>> >                         return host->ops->get_max_timeout(host);
>> >                 else
>> >                         return 0xE;
>> >
>>
>> The return val of sdhci_calc_timeout is the register value to be
>> written into timeout counter register.
>> host->ops->get_max_timeout returns the max timeout in miliseconds directly.
>> So we can not do like that here.
>> They're two concepts.
>
> I did not make my comment clear.  The .get_max_timeout() is not the one
> you defined in patch #1 any more, but something like the following for
> esdhc driver, which returns correct timeout value not milliseconds.
>
> 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;
>
>         return esdhc_is_usdhc(imx_data) ? 0xF : 0xE;
>
> }
>
> Does that match the problem you described in the commit log better?
>

This actually is the register value, not the max timeout counter value.
Then you may want define the API as:
unsigned int    (*get_max_timeout_val)(struct sdhci_host *host);
But i don't think it's necessary to do the max timeout setting in two steps.
First, deinfe a API to get the max timeout counter val,
Second, write this val into register.
Why not simply implement .set_timeout and handle the details in
platform specific
host driver respectively?

Furthermore, this API does not  help for the patch#1 issue.


Regards
Dong Aisheng


> Shawn
>

[PATCH 3/6] mmc: sdhci: add platform set_timeout hook

[Shawn Guo]

On Wed, Dec 11, 2013 at 11:35:17AM +0800, Dong Aisheng wrote:
> This actually is the register value, not the max timeout counter value.
> Then you may want define the API as:
> unsigned int    (*get_max_timeout_val)(struct sdhci_host *host);

Yes, something like that.

> But i don't think it's necessary to do the max timeout setting in two steps.
> First, deinfe a API to get the max timeout counter val,
> Second, write this val into register.
> Why not simply implement .set_timeout and handle the details in
> platform specific
> host driver respectively?

Well, that's how sdhci host driver is structured.  Doing so leaves the
least details to platform driver, and calling sdhci_writeb() to access
SDHCI_TIMEOUT_CONTROL in sdhci-esdhc-imx seems a layer violation to me.

> 
> Furthermore, this API does not  help for the patch#1 issue.

Oh, they two different issues, and should be addressed by different
hooks.

Shawn

[PATCH 1/6] mmc: sdhci: add platfrom get_max_timeout hook

[Shawn Guo]

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

[PATCH 5/6] mmc: sdhci: calculate max_discard_to dynamically for SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK

[Shawn Guo]

On Tue, Dec 10, 2013 at 08:56:07PM +0800, Dong Aisheng wrote:
>  static inline void sdhci_update_clock(struct sdhci_host *host)
> @@ -2939,10 +2952,12 @@ int sdhci_add_host(struct sdhci_host *host)
>  	if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK)
>  		host->timeout_clk = mmc->f_max / 1000;

Since max_discard_to calculation below will not happen for
SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK case, does it still make sense to
keep the above code?

Or put it in the other way, if we keep the above code and do not make
the change below, will there be any problem besides the max_discard_to
initialization plays for nothing?

All in all, I'm just confused why we keep the above code and make the
change below at the same time.

Shawn

>  
> -	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;
> +	if (!(host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK)) {
> +		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;
>  
> -- 
> 1.7.2.rc3
> 
> 

[PATCH 1/6] mmc: sdhci: add platfrom get_max_timeout hook

[Dong Aisheng]

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
>

[PATCH 3/6] mmc: sdhci: add platform set_timeout hook

[Dong Aisheng]

On Wed, Dec 11, 2013 at 11:48 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Wed, Dec 11, 2013 at 11:35:17AM +0800, Dong Aisheng wrote:
>> This actually is the register value, not the max timeout counter value.
>> Then you may want define the API as:
>> unsigned int    (*get_max_timeout_val)(struct sdhci_host *host);
>
> Yes, something like that.
>
>> But i don't think it's necessary to do the max timeout setting in two steps.
>> First, deinfe a API to get the max timeout counter val,
>> Second, write this val into register.
>> Why not simply implement .set_timeout and handle the details in
>> platform specific
>> host driver respectively?
>
> Well, that's how sdhci host driver is structured.  Doing so leaves the
> least details to platform driver, and calling sdhci_writeb() to access
> SDHCI_TIMEOUT_CONTROL in sdhci-esdhc-imx seems a layer violation to me.
>

The current sdhci-esdhci-imx already does something like that.
You can search SDHCI_* in the code.
It just reuses the register offset definition in sdhci,
It's not the layer violation.

Regards
Dong Aisheng

>>
>> Furthermore, this API does not  help for the patch#1 issue.
>
> Oh, they two different issues, and should be addressed by different
> hooks.
>
> Shawn
>

[PATCH 5/6] mmc: sdhci: calculate max_discard_to dynamically for SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK

[Dong Aisheng]

On Wed, Dec 11, 2013 at 12:05 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Tue, Dec 10, 2013 at 08:56:07PM +0800, Dong Aisheng wrote:
>>  static inline void sdhci_update_clock(struct sdhci_host *host)
>> @@ -2939,10 +2952,12 @@ int sdhci_add_host(struct sdhci_host *host)
>>       if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK)
>>               host->timeout_clk = mmc->f_max / 1000;
>
> Since max_discard_to calculation below will not happen for
> SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK case, does it still make sense to
> keep the above code?
>

Right, i missed to remove it.
Will update in v2.

> Or put it in the other way, if we keep the above code and do not make
> the change below, will there be any problem besides the max_discard_to
> initialization plays for nothing?
>
> All in all, I'm just confused why we keep the above code and make the
> change below at the same time.
>

THe max_discard_to should be dynamically updated for
SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK
when changing the clock, so we can remove above lines.

Regards
Dong Aisheng

> Shawn
>
>>
>> -     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;
>> +     if (!(host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK)) {
>> +             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;
>>
>> --
>> 1.7.2.rc3
>>
>>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/6] mmc: sdhci: add platfrom get_max_timeout hook

[Shawn Guo]

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

[PATCH 3/6] mmc: sdhci: add platform set_timeout hook

[Shawn Guo]

On Wed, Dec 11, 2013 at 01:03:43PM +0800, Dong Aisheng wrote:
> On Wed, Dec 11, 2013 at 11:48 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> > On Wed, Dec 11, 2013 at 11:35:17AM +0800, Dong Aisheng wrote:
> >> This actually is the register value, not the max timeout counter value.
> >> Then you may want define the API as:
> >> unsigned int    (*get_max_timeout_val)(struct sdhci_host *host);
> >
> > Yes, something like that.
> >
> >> But i don't think it's necessary to do the max timeout setting in two steps.
> >> First, deinfe a API to get the max timeout counter val,
> >> Second, write this val into register.
> >> Why not simply implement .set_timeout and handle the details in
> >> platform specific
> >> host driver respectively?
> >
> > Well, that's how sdhci host driver is structured.  Doing so leaves the
> > least details to platform driver, and calling sdhci_writeb() to access
> > SDHCI_TIMEOUT_CONTROL in sdhci-esdhc-imx seems a layer violation to me.
> >
> 
> The current sdhci-esdhci-imx already does something like that.
> You can search SDHCI_* in the code.
> It just reuses the register offset definition in sdhci,
> It's not the layer violation.

I do not understand why we have to do this SDHCI_TIMEOUT_CONTROL setup
in our sdhci-esdhc-imx driver, when sdhci driver is already structured
to do it in its way.  All we need is a hook to give the correct
max_timeout_val.

Shawn

[PATCH 1/6] mmc: sdhci: add platfrom get_max_timeout hook

[Dong Aisheng]

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
>

[PATCH 3/6] mmc: sdhci: add platform set_timeout hook

[Dong Aisheng]

On Wed, Dec 11, 2013 at 2:04 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Wed, Dec 11, 2013 at 01:03:43PM +0800, Dong Aisheng wrote:
>> On Wed, Dec 11, 2013 at 11:48 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
>> > On Wed, Dec 11, 2013 at 11:35:17AM +0800, Dong Aisheng wrote:
>> >> This actually is the register value, not the max timeout counter value.
>> >> Then you may want define the API as:
>> >> unsigned int    (*get_max_timeout_val)(struct sdhci_host *host);
>> >
>> > Yes, something like that.
>> >
>> >> But i don't think it's necessary to do the max timeout setting in two steps.
>> >> First, deinfe a API to get the max timeout counter val,
>> >> Second, write this val into register.
>> >> Why not simply implement .set_timeout and handle the details in
>> >> platform specific
>> >> host driver respectively?
>> >
>> > Well, that's how sdhci host driver is structured.  Doing so leaves the
>> > least details to platform driver, and calling sdhci_writeb() to access
>> > SDHCI_TIMEOUT_CONTROL in sdhci-esdhc-imx seems a layer violation to me.
>> >
>>
>> The current sdhci-esdhci-imx already does something like that.
>> You can search SDHCI_* in the code.
>> It just reuses the register offset definition in sdhci,
>> It's not the layer violation.
>
> I do not understand why we have to do this SDHCI_TIMEOUT_CONTROL setup
> in our sdhci-esdhc-imx driver, when sdhci driver is already structured
> to do it in its way.  All we need is a hook to give the correct
> max_timeout_val.
>

Because i don't think sdhci_calc_timeout is still meaningful anymore for a
SDHCI_QUIRK_BROKEN_TIMEOUT_VAL anymore.
The only thing you reply sdhci to do is set a max_timeout.

And i don't think it's necessary to use a hook to get max_timeout_val first,
then use this val to write into register later to do the max_timeout
setting work.
It's perfect fine to let the platform .set_timeout to do it directly.
And we already have .get_max_timeout_count hook, i don't want
.get_max_timeout_val again.

There's another important reason that .set_timeout provides other platforms
host controller ability to set timeout based on their way if they want.
The sdhci should have this capability since it is common case just like
.set_clock and etc.
e.g. imx5. the timeout count setting is different when DDR is enabled.
(Though we still not add DDR support for mx5)
Then we must need such hook to do specific IMX timeout setting instead
of using common sdhci timeout setting.

Regards
Dong Aisheng

> Shawn
>