[PATCH] sdhci: always use max timeout for xfers

[PATCH] sdhci: always use max timeout for xfers

[Philip Rakity]

Rather then special case busy etc .. lets just use the max value.  

Did not remove BROKEN_TIMEOUT QUIRK so existing code will compile
we can remove this once existing platform drivers delete usage and get
quirk back.

Patch starts after ====
=====

The card/host controller may sometimes return a value that is
too low and cause the h/w to timeout a transfer that would have
worked.  Using the maximum value avoids this.

Signed-off-by: Philip Rakity <prakity@marvell.com>
---
 drivers/mmc/host/sdhci.c |   48 ++++-----------------------------------------
 1 files changed, 5 insertions(+), 43 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 655617c..dd99da8 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -592,53 +592,15 @@ static void sdhci_adma_table_post(struct sdhci_host *host,
 		data->sg_len, direction);
 }
 
-static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_data *data)
+static inline u8 sdhci_calc_timeout(void)
 {
-	u8 count;
-	unsigned target_timeout, current_timeout;
-
 	/*
-	 * If the host controller provides us with an incorrect timeout
-	 * value, just skip the check and use 0xE.  The hardware may take
+	 * The host controller/card can provide us with an incorrect timeout
+	 * value, just use the maximum value 0xE.  The hardware may take
 	 * longer to time out, but that's much better than having a too-short
 	 * timeout value.
 	 */
-	if (host->quirks & SDHCI_QUIRK_BROKEN_TIMEOUT_VAL)
-		return 0xE;
-
-	/* timeout in us */
-	target_timeout = data->timeout_ns / 1000 +
-		data->timeout_clks / host->clock;
-
-	if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK)
-		host->timeout_clk = host->clock / 1000;
-
-	/*
-	 * Figure out needed cycles.
-	 * We do this in steps in order to fit inside a 32 bit int.
-	 * The first step is the minimum timeout, which will have a
-	 * minimum resolution of 6 bits:
-	 * (1) 2^13*1000 > 2^22,
-	 * (2) host->timeout_clk < 2^16
-	 *     =>
-	 *     (1) / (2) > 2^6
-	 */
-	count = 0;
-	current_timeout = (1 << 13) * 1000 / host->timeout_clk;
-	while (current_timeout < target_timeout) {
-		count++;
-		current_timeout <<= 1;
-		if (count >= 0xF)
-			break;
-	}
-
-	if (count >= 0xF) {
-		printk(KERN_WARNING "%s: Too large timeout requested!\n",
-			mmc_hostname(host->mmc));
-		count = 0xE;
-	}
-
-	return count;
+	return 0xE;
 }
 
 static void sdhci_set_transfer_irqs(struct sdhci_host *host)
@@ -671,7 +633,7 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data)
 	host->data = data;
 	host->data_early = 0;
 
-	count = sdhci_calc_timeout(host, data);
+	count = sdhci_calc_timeout();
 	sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
 
 	if (host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA))
-- 
1.7.0.4

Re: [PATCH] sdhci: always use max timeout for xfers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 3676 bytes --]

Hi Philip,

On Fri, Feb 25, 2011 at 09:54:35AM -0800, Philip Rakity wrote:
> Rather then special case busy etc .. lets just use the max value.  

OK.

> 
> Did not remove BROKEN_TIMEOUT QUIRK so existing code will compile
> we can remove this once existing platform drivers delete usage and get
> quirk back.

If we wait for that, we'll probably wait till eternity ;) I'd vote that
removing the quirk should be part of the patch.

> 
> Patch starts after ====
> =====

The usual nomenclature is that such comments simply go between '---' and the
diffstat. Most tools are prepared for this...

> The card/host controller may sometimes return a value that is
> too low and cause the h/w to timeout a transfer that would have
> worked.  Using the maximum value avoids this.
> 
> Signed-off-by: Philip Rakity <prakity@marvell.com>
> ---

... to handle them here.

>  drivers/mmc/host/sdhci.c |   48 ++++-----------------------------------------
>  1 files changed, 5 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 655617c..dd99da8 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -592,53 +592,15 @@ static void sdhci_adma_table_post(struct sdhci_host *host,
>  		data->sg_len, direction);
>  }
>  
> -static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_data *data)
> +static inline u8 sdhci_calc_timeout(void)
>  {
> -	u8 count;
> -	unsigned target_timeout, current_timeout;
> -
>  	/*
> -	 * If the host controller provides us with an incorrect timeout
> -	 * value, just skip the check and use 0xE.  The hardware may take
> +	 * The host controller/card can provide us with an incorrect timeout
> +	 * value, just use the maximum value 0xE.  The hardware may take
>  	 * longer to time out, but that's much better than having a too-short
>  	 * timeout value.
>  	 */
> -	if (host->quirks & SDHCI_QUIRK_BROKEN_TIMEOUT_VAL)
> -		return 0xE;
> -
> -	/* timeout in us */
> -	target_timeout = data->timeout_ns / 1000 +
> -		data->timeout_clks / host->clock;
> -
> -	if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK)
> -		host->timeout_clk = host->clock / 1000;

This quirk could go then as well?

> -
> -	/*
> -	 * Figure out needed cycles.
> -	 * We do this in steps in order to fit inside a 32 bit int.
> -	 * The first step is the minimum timeout, which will have a
> -	 * minimum resolution of 6 bits:
> -	 * (1) 2^13*1000 > 2^22,
> -	 * (2) host->timeout_clk < 2^16
> -	 *     =>
> -	 *     (1) / (2) > 2^6
> -	 */
> -	count = 0;
> -	current_timeout = (1 << 13) * 1000 / host->timeout_clk;
> -	while (current_timeout < target_timeout) {
> -		count++;
> -		current_timeout <<= 1;
> -		if (count >= 0xF)
> -			break;
> -	}
> -
> -	if (count >= 0xF) {
> -		printk(KERN_WARNING "%s: Too large timeout requested!\n",
> -			mmc_hostname(host->mmc));
> -		count = 0xE;
> -	}
> -
> -	return count;
> +	return 0xE;

Why don't you remove the function entirely?

>  }
>  
>  static void sdhci_set_transfer_irqs(struct sdhci_host *host)
> @@ -671,7 +633,7 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data)
>  	host->data = data;
>  	host->data_early = 0;
>  
> -	count = sdhci_calc_timeout(host, data);
> +	count = sdhci_calc_timeout();
>  	sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
>  
>  	if (host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA))

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] sdhci: always use max timeout for xfers

[Philip Rakity]

On Feb 25, 2011, at 10:02 AM, Wolfram Sang wrote:

> Hi Philip,
> 
> On Fri, Feb 25, 2011 at 09:54:35AM -0800, Philip Rakity wrote:
>> Rather then special case busy etc .. lets just use the max value.  
> 
> OK.
> 
>> 
>> Did not remove BROKEN_TIMEOUT QUIRK so existing code will compile
>> we can remove this once existing platform drivers delete usage and get
>> quirk back.
> 
> If we wait for that, we'll probably wait till eternity ;) I'd vote that
> removing the quirk should be part of the patch.

I concur (see below)

> 
>> 
>> Patch starts after ====
>> =====
> 
> The usual nomenclature is that such comments simply go between '---' and the
> diffstat. Most tools are prepared for this...
> 
>> The card/host controller may sometimes return a value that is
>> too low and cause the h/w to timeout a transfer that would have
>> worked.  Using the maximum value avoids this.
>> 
>> Signed-off-by: Philip Rakity <prakity@marvell.com>
>> ---
> 
> ... to handle them here.
> 
>> drivers/mmc/host/sdhci.c |   48 ++++-----------------------------------------
>> 1 files changed, 5 insertions(+), 43 deletions(-)
>> 
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 655617c..dd99da8 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -592,53 +592,15 @@ static void sdhci_adma_table_post(struct sdhci_host *host,
>> 		data->sg_len, direction);
>> }
>> 
>> -static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_data *data)
>> +static inline u8 sdhci_calc_timeout(void)
>> {
>> -	u8 count;
>> -	unsigned target_timeout, current_timeout;
>> -
>> 	/*
>> -	 * If the host controller provides us with an incorrect timeout
>> -	 * value, just skip the check and use 0xE.  The hardware may take
>> +	 * The host controller/card can provide us with an incorrect timeout
>> +	 * value, just use the maximum value 0xE.  The hardware may take
>> 	 * longer to time out, but that's much better than having a too-short
>> 	 * timeout value.
>> 	 */
>> -	if (host->quirks & SDHCI_QUIRK_BROKEN_TIMEOUT_VAL)
>> -		return 0xE;
>> -
>> -	/* timeout in us */
>> -	target_timeout = data->timeout_ns / 1000 +
>> -		data->timeout_clks / host->clock;
>> -
>> -	if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK)
>> -		host->timeout_clk = host->clock / 1000;
> 
> This quirk could go then as well?

I am all for that -- did not want to touch other drivers but will remove for sdhci-pxa if
patch is okay.  

> 
>> -
>> -	/*
>> -	 * Figure out needed cycles.
>> -	 * We do this in steps in order to fit inside a 32 bit int.
>> -	 * The first step is the minimum timeout, which will have a
>> -	 * minimum resolution of 6 bits:
>> -	 * (1) 2^13*1000 > 2^22,
>> -	 * (2) host->timeout_clk < 2^16
>> -	 *     =>
>> -	 *     (1) / (2) > 2^6
>> -	 */
>> -	count = 0;
>> -	current_timeout = (1 << 13) * 1000 / host->timeout_clk;
>> -	while (current_timeout < target_timeout) {
>> -		count++;
>> -		current_timeout <<= 1;
>> -		if (count >= 0xF)
>> -			break;
>> -	}
>> -
>> -	if (count >= 0xF) {
>> -		printk(KERN_WARNING "%s: Too large timeout requested!\n",
>> -			mmc_hostname(host->mmc));
>> -		count = 0xE;
>> -	}
>> -
>> -	return count;
>> +	return 0xE;
> 
> Why don't you remove the function entirely?

better to rename it --- to set_maximum_timeout  since a little clearer.
left the old name for historical reasons -- if no need I will change it

> 
>> }
>> 
>> static void sdhci_set_transfer_irqs(struct sdhci_host *host)
>> @@ -671,7 +633,7 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data)
>> 	host->data = data;
>> 	host->data_early = 0;
>> 
>> -	count = sdhci_calc_timeout(host, data);
>> +	count = sdhci_calc_timeout();
>> 	sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
>> 
>> 	if (host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA))
> 
> Thanks,
> 
>   Wolfram
> 
> -- 
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] sdhci: always use max timeout for xfers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 915 bytes --]

> >> -	return count;
> >> +	return 0xE;
> > 
> > Why don't you remove the function entirely?
> 
> better to rename it --- to set_maximum_timeout  since a little clearer.
> left the old name for historical reasons -- if no need I will change it

I'd suggest...

> 
> > 
> >> }
> >> 
> >> static void sdhci_set_transfer_irqs(struct sdhci_host *host)
> >> @@ -671,7 +633,7 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data)
> >> 	host->data = data;
> >> 	host->data_early = 0;
> >> 
> >> -	count = sdhci_calc_timeout(host, data);
> >> +	count = sdhci_calc_timeout();
> >> 	sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);

using SDHCI_TIMEOUT_MAX here instead of count with a proper define in
sdhci.h.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH V2] sdhci: always use max timeout for xfers

[Philip Rakity]

v2

use define for max timeout.  remove subroutine call and just
set the register directly

v1

The card/host controller may sometimes return a value that is
too low and cause the h/w to timeout a transfer that would have
worked.  Using the maximum value avoids this.

Signed-off-by: Philip Rakity <prakity@marvell.com>
---
 drivers/mmc/host/sdhci.c |   59 +++++----------------------------------------
 drivers/mmc/host/sdhci.h |    1 +
 2 files changed, 8 insertions(+), 52 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 655617c..d615173 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -592,55 +592,6 @@ static void sdhci_adma_table_post(struct sdhci_host *host,
 		data->sg_len, direction);
 }
 
-static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_data *data)
-{
-	u8 count;
-	unsigned target_timeout, current_timeout;
-
-	/*
-	 * If the host controller provides us with an incorrect timeout
-	 * value, just skip the check and use 0xE.  The hardware may take
-	 * longer to time out, but that's much better than having a too-short
-	 * timeout value.
-	 */
-	if (host->quirks & SDHCI_QUIRK_BROKEN_TIMEOUT_VAL)
-		return 0xE;
-
-	/* timeout in us */
-	target_timeout = data->timeout_ns / 1000 +
-		data->timeout_clks / host->clock;
-
-	if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK)
-		host->timeout_clk = host->clock / 1000;
-
-	/*
-	 * Figure out needed cycles.
-	 * We do this in steps in order to fit inside a 32 bit int.
-	 * The first step is the minimum timeout, which will have a
-	 * minimum resolution of 6 bits:
-	 * (1) 2^13*1000 > 2^22,
-	 * (2) host->timeout_clk < 2^16
-	 *     =>
-	 *     (1) / (2) > 2^6
-	 */
-	count = 0;
-	current_timeout = (1 << 13) * 1000 / host->timeout_clk;
-	while (current_timeout < target_timeout) {
-		count++;
-		current_timeout <<= 1;
-		if (count >= 0xF)
-			break;
-	}
-
-	if (count >= 0xF) {
-		printk(KERN_WARNING "%s: Too large timeout requested!\n",
-			mmc_hostname(host->mmc));
-		count = 0xE;
-	}
-
-	return count;
-}
-
 static void sdhci_set_transfer_irqs(struct sdhci_host *host)
 {
 	u32 pio_irqs = SDHCI_INT_DATA_AVAIL | SDHCI_INT_SPACE_AVAIL;
@@ -654,7 +605,6 @@ static void sdhci_set_transfer_irqs(struct sdhci_host *host)
 
 static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data)
 {
-	u8 count;
 	u8 ctrl;
 	int ret;
 
@@ -671,8 +621,13 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data)
 	host->data = data;
 	host->data_early = 0;
 
-	count = sdhci_calc_timeout(host, data);
-	sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
+	/*
+	 * The host controller/card can provide us with an incorrect timeout
+	 * value, just use the maximum value 0xE.  The hardware may take
+	 * longer to time out, but that's much better than having a too-short
+	 * timeout value.
+	 */
+	sdhci_writeb(host, SDHCI_TIMEOUT_MAX, SDHCI_TIMEOUT_CONTROL);
 
 	if (host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA))
 		host->flags |= SDHCI_REQ_USE_DMA;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 1f032c0..19b4d41 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -103,6 +103,7 @@
 #define  SDHCI_CLOCK_INT_EN	0x0001
 
 #define SDHCI_TIMEOUT_CONTROL	0x2E
+#define  SDHCI_TIMEOUT_MAX	0xE
 
 #define SDHCI_SOFTWARE_RESET	0x2F
 #define  SDHCI_RESET_ALL	0x01
-- 
1.7.0.4

Re: [PATCH V2] sdhci: always use max timeout for xfers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1038 bytes --]

On Fri, Feb 25, 2011 at 10:49:25AM -0800, Philip Rakity wrote:
> v2
> 
> use define for max timeout.  remove subroutine call and just
> set the register directly

The generic description goes above the "---" line, the incremental
history of the patch usually below.

> 
> v1
> 
> The card/host controller may sometimes return a value that is
> too low and cause the h/w to timeout a transfer that would have
> worked.  Using the maximum value avoids this.
> 
> Signed-off-by: Philip Rakity <prakity@marvell.com>

What is there seems ok, but it is not enough yet. The quirks can also go
from the users.

After that, it gets even more complicated; after this patch
'host->timeout_clk' becomes obsolete which should probably cleaned up in
a later patch together with host->ops->get_timeout_clk. Hmmmm, that
needs careful auditing.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH V2] sdhci: always use max timeout for xfers

[Jaehoon Chung]

I think that always use max timeout for xfers is not bed..
But when i have sent the RFC patch, during suspend/resume is appeared some problem.
(when busy-waiting, occurred interrupt..so illegal sequence error is occurred..)
Anyone found same problem when suspend/resume?

So, i think that setting maximum timeout value is not good solution about every case.

Regards,
Jaehoon Chung

Wolfram Sang wrote:
> On Fri, Feb 25, 2011 at 10:49:25AM -0800, Philip Rakity wrote:
>> v2
>>
>> use define for max timeout.  remove subroutine call and just
>> set the register directly
> 
> The generic description goes above the "---" line, the incremental
> history of the patch usually below.
> 
>> v1
>>
>> The card/host controller may sometimes return a value that is
>> too low and cause the h/w to timeout a transfer that would have
>> worked.  Using the maximum value avoids this.
>>
>> Signed-off-by: Philip Rakity <prakity@marvell.com>
> 
> What is there seems ok, but it is not enough yet. The quirks can also go
> from the users.
> 
> After that, it gets even more complicated; after this patch
> 'host->timeout_clk' becomes obsolete which should probably cleaned up in
> a later patch together with host->ops->get_timeout_clk. Hmmmm, that
> needs careful auditing.
> 
> Regards,
> 
>    Wolfram
>