[U-Boot] [PATCH] mmc: sdhci: Avoid commands errors by simple timeout adaptation.

[Pantelis Antoniou]

Hi there,


On Oct 1, 2013, at 3:16 PM, Przemyslaw Marczak wrote:

> Old command timeout value was too small and it caused I/O errors which
> led to uncompleted read/write/erase operations and filesystem errors.
> Timeout adaptation fixes this issue.
> 
> Changes in sdhci_send_command() function:
> - change timeout variable to static
> - increase default command timeout to 100 ms
> - add definition of max command timeout value,
>  which can be redefined in each board config file
> - wait for card ready state for max defined time
>  if it doesn't exceed defined maximum or return COMM_ERR
> 
> Once successfully increased timeout value will be used in next function
> call. This fix was tested on Goni, Trats, Trats2 boards by testing UMS
> on MMC storage.
> 
> Signed-off-by: Przemyslaw Marczak <p.marczak at samsung.com>
> ---
> drivers/mmc/sdhci.c |   34 +++++++++++++++++++++++++++-------
> 1 file changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
> index 4261991..af11fc5 100644
> --- a/drivers/mmc/sdhci.c
> +++ b/drivers/mmc/sdhci.c
> @@ -110,6 +110,22 @@ static int sdhci_transfer_data(struct sdhci_host *host, struct mmc_data *data,
> 	return 0;
> }
> 
> +/*
> + * No command will be sent by driver if card is busy, so driver must wait
> + * for card ready state.
> + * Every time when card is busy after timeout then (last) timeout value will be
> + * increased twice but only if it doesn't exceed global defined maximum.
> + * Each function call will use last timeout value. Max timeout can be redefined
> + * in board config file.
> + */
> +#ifndef CONFIG_SDHCI_CMD_MAX_TIMEOUT
> +#define CONFIG_SDHCI_CMD_MAX_TIMEOUT		3200
> +#endif
> +#define CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT	100
> +
> +/* Timeout unit - ms */
> +static unsigned int cmd_timeout = CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT;
> +
> int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
> 		       struct mmc_data *data)
> {
> @@ -118,12 +134,9 @@ int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
> 	int ret = 0;
> 	int trans_bytes = 0, is_aligned = 1;
> 	u32 mask, flags, mode;
> -	unsigned int timeout, start_addr = 0;
> +	unsigned int time = 0, start_addr = 0;
> 	unsigned int retry = 10000;
> 
> -	/* Wait max 10 ms */
> -	timeout = 10;
> -
> 	sdhci_writel(host, SDHCI_INT_ALL_MASK, SDHCI_INT_STATUS);
> 	mask = SDHCI_CMD_INHIBIT | SDHCI_DATA_INHIBIT;
> 
> @@ -133,11 +146,18 @@ int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
> 		mask &= ~SDHCI_DATA_INHIBIT;
> 
> 	while (sdhci_readl(host, SDHCI_PRESENT_STATE) & mask) {
> -		if (timeout == 0) {
> +		if (time == cmd_timeout) {

time >= cmd_timeout here.

You rely on the timeout hitting exactly the same value which is not guaranteed.

> 			printf("Controller never released inhibit bit(s).\n");
> -			return COMM_ERR;
> +			if (2 * cmd_timeout <= CONFIG_SDHCI_CMD_MAX_TIMEOUT) {
> +				cmd_timeout += cmd_timeout;
> +				debug("Increasing command timeout to: %u ms.\n",
> +				      cmd_timeout);
> +			} else {
> +				error("Command timeout is set to max.\n");
> +				return COMM_ERR;
> +			}
> 		}
> -		timeout--;
> +		time++;
> 		udelay(1000);
> 	}
> 
> -- 
> 1.7.9.5
> 

Other than that the concept seems sound.

Regards

-- Pantelis