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 <[email protected]>
---
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
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 <[email protected]>
> ---
... 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/ |
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 <[email protected]>
>> ---
>
> ... 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/ |
> >> - 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/ |
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 <[email protected]>
---
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
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 <[email protected]>
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/ |
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 <[email protected]>
>
> 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
>