[U-Boot] [PATCH 2/4] mmc: dw_mmc: Zap endless timeout

[Alexey Brodkin]

Hi Marek,

On Mon, 2015-07-27 at 22:39 +0200, Marek Vasut wrote:
> Endless timeouts are bad, since if we get stuck in one, we have no
> way out. Zap this one by implementing proper timeout.
> 
> Signed-off-by: Marek Vasut <marex at denx.de>
> Cc: Dinh Nguyen <dinguyen at opensource.altera.com>
> Cc: Pantelis Antoniou <panto at antoniou-consulting.com>
> Cc: Tom Rini <trini at konsulko.com>
> ---
>  drivers/mmc/dw_mmc.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c
> index 3fffa71..0f61f16 100644
> --- a/drivers/mmc/dw_mmc.c
> +++ b/drivers/mmc/dw_mmc.c
> @@ -211,14 +211,29 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
>  	}
>  
>  	if (data) {
> -		do {
> +		start = get_timer(0);
> +		timeout = 1000;
> +		for (;;) {
>  			mask = dwmci_readl(host, DWMCI_RINTSTS);
> +			/* Error during data transfer. */
>  			if (mask & (DWMCI_DATA_ERR | DWMCI_DATA_TOUT)) {
>  				printf("%s: DATA ERROR!\n", __func__);
>  				bounce_buffer_stop(&bbstate);
>  				return -1;
>  			}
> -		} while (!(mask & DWMCI_INTMSK_DTO));
> +
> +			/* Data arrived correctly. */
> +			if (mask & DWMCI_INTMSK_DTO)
> +				break;
> +
> +			/* Check for timeout. */
> +			if (get_timer(start) > timeout) {
> +				printf("%s: Timeout waiting for data!\n",
> +				       __func__);
> +				bounce_buffer_stop(&bbstate);
> +				return TIMEOUT;
> +			}
> +		}
>  
>  		dwmci_writel(host, DWMCI_RINTSTS, mask);
>  

It turned out that patch breaks functionality in some cases.
For me on every attempt to download something significant (at least I see it on
5/7 Mb files) from SD I'm seeing timeout firing too early.

I added a bit of extra instrumentation to see where time is spent and why.

So my diff is:
----------------------------------->8--------------------------------
diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c
index 77b87e0..2da77a7 100644
--- a/drivers/mmc/dw_mmc.c
+++ b/drivers/mmc/dw_mmc.c
@@ -213,7 +213,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
 
        if (data) {
                start = get_timer(0);
-               timeout = 1000;
+               timeout = 10000; // That's required to get to the end of the transfer
                for (;;) {
                        mask = dwmci_readl(host, DWMCI_RINTSTS);
                        /* Error during data transfer. */
@@ -226,6 +226,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
                        /* Data arrived correctly. */
                        if (mask & DWMCI_INTMSK_DTO) {
                                ret = 0;
+                               printf(" * time spent: %d, data size: %d, blocks: %d\n", (int)get_timer(start), data
->blocksize * data->blocks, data->blocks);
                                break;
                        }
----------------------------------->8--------------------------------

And that's what I see then:
----------------------------------->8--------------------------------
AXS# fatload mmc 0
 * time spent: 0, data size: 8, blocks: 1
 * time spent: 0, data size: 512, blocks: 1
 * time spent: 0, data size: 512, blocks: 1
 * time spent: 0, data size: 512, blocks: 1
reading uImage
 * time spent: 1, data size: 512, blocks: 1
 * time spent: 0, data size: 1024, blocks: 2
 * time spent: 1, data size: 3072, blocks: 6
 * time spent: 1, data size: 3072, blocks: 6
 * time spent: 1, data size: 3072, blocks: 6
 * time spent: 0, data size: 3072, blocks: 6
 * time spent: 0, data size: 3072, blocks: 6
 * time spent: 1599, data size: 13338112, blocks: 26051
 * time spent: 0, data size: 512, blocks: 1
13338188 bytes read in 1651 ms (7.7 MiB/s)
----------------------------------->8--------------------------------

So you see real data transfer takes  ~1.7 seconds when getting 26k blocks.

In other words timeout check has to be a bit smarter, for example
taking into account number of blocks to be transferred.

Any thoughts?

-Alexey