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

Wolfram Sang
2011-02-25T18:02:48Z

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