Re: [PATCH v2 11/16] mmc: sdhci: Program a relatively accurate SW timeout value

[Adrian Hunter]

On 19/02/18 11:24, Adrian Hunter wrote:
> On 05/02/18 14:50, Kishon Vijay Abraham I wrote:
>> sdhci has a 10 second timeout to catch devices that stop responding.
>> Instead of programming 10 second arbitrary value, calculate the total time
>> it would take for the entire transfer to happen and program the timeout
>> value accordingly.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@xxxxxx>
>> ---
>>  drivers/mmc/host/sdhci.c | 46 +++++++++++++++++++++++++++++++++++++++-------
>>  drivers/mmc/host/sdhci.h | 10 ++++++++++
>>  2 files changed, 49 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 0489572d1892..d52f9e7eabe2 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -673,6 +673,37 @@ static void sdhci_adma_table_post(struct sdhci_host *host,
>>  	}
>>  }
>>  
>> +static void sdhci_calc_sw_timeout(struct sdhci_host *host,
>> +				  struct mmc_command *cmd,
>> +				  unsigned int target_timeout)
>> +{
>> +	struct mmc_data *data = cmd->data;
>> +	struct mmc_host *mmc = host->mmc;
>> +	unsigned long long transfer_time;
>> +	struct mmc_ios *ios = &mmc->ios;
>> +	unsigned char bus_width = ios->bus_width;
>> +	unsigned int blksz;
>> +	unsigned int freq;
>> +
>> +	if (data) {
>> +		blksz = data->blksz;
>> +		freq = host->mmc->actual_clock ? host->mmc->actual_clock :
>> +						host->clock;
> 
> I think this can be:
> 
> 		freq = host->mmc->actual_clock ? : host->clock;
> 
>> +		transfer_time = (unsigned long long)(blksz * NSEC_PER_SEC *
>> +						     (8 / bus_width)) / freq;
> 
> You have got a 32-bit overflow here and a 64-bit division that can't always
> be done with '/'
> 
>> +		/* multiply by '2' to account for any unknowns */
>> +		transfer_time = transfer_time * 2;
> 
> 		transfer_time *= 2;
> 
>> +		/* calculate timeout for the entire data */
>> +		host->data_timeout = (data->blocks * ((target_timeout *
>> +						       NSEC_PER_USEC) +
>> +						       transfer_time));
>> +	} else {
>> +		host->data_timeout = target_timeout * NSEC_PER_USEC;
> 
> And another 32-bit overflow here
> 
>> +	}
>> +
>> +	host->data_timeout += MMC_CMD_TRANSFER_TIME;
>> +}
>> +
>>  static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
>>  {
>>  	u8 count;
>> @@ -742,6 +773,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
>>  			host->hw_timeout_disabled = true;
>>  		}
>>  	}
>> +	sdhci_calc_sw_timeout(host, cmd, target_timeout);
>>  
>>  	return count;
>>  }
>> @@ -1130,13 +1162,6 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
>>  		mdelay(1);
>>  	}
>>  
>> -	timeout = jiffies;
>> -	if (!cmd->data && cmd->busy_timeout > 9000)
>> -		timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;
>> -	else
>> -		timeout += 10 * HZ;
>> -	sdhci_mod_timer(host, cmd, timeout);
>> -
>>  	host->cmd = cmd;
>>  	if (sdhci_data_line_cmd(cmd)) {
>>  		WARN_ON(host->data_cmd);
>> @@ -1176,6 +1201,13 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
>>  	    cmd->opcode == MMC_SEND_TUNING_BLOCK_HS200)
>>  		flags |= SDHCI_CMD_DATA;
>>  
>> +	timeout = jiffies;
>> +	if (sdhci_data_line_cmd(cmd))
>> +		timeout += nsecs_to_jiffies(host->data_timeout);
>> +	else
>> +		timeout += 10 * HZ;
>> +	sdhci_mod_timer(host, cmd, timeout);
> 
> Here you probably want to avoid updating the timer if the mrq has already
> started using host->data_timeout.
> 
>> +
>>  	sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags), SDHCI_COMMAND);
>>  }
>>  EXPORT_SYMBOL_GPL(sdhci_send_command);
>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>> index 3a967a56fcc3..b73577d77856 100644
>> --- a/drivers/mmc/host/sdhci.h
>> +++ b/drivers/mmc/host/sdhci.h
>> @@ -332,6 +332,14 @@ struct sdhci_adma2_64_desc {
>>  /* Allow for a a command request and a data request at the same time */
>>  #define SDHCI_MAX_MRQS		2
>>  
>> +/*
>> + * 48bit command and 136 bit response in 400KHz clock should take 0.46ms.
> 
> I am not sure the math is correct here.  I would add the 64 clocks before
> the response also, and allow for 100 kHz clock.
> 
> 	(48 + 64 + 136) * 100 us = 24.8 ms

No I am off by x10, sorry!  I would still go for 10 ms.

> 
> Given that you are looking at data block timeouts in excess of 700 ms, and
> anything up to 10% of that is relatively negligible, anything less that 70
> ms seems fine, so I would set 30 ms here as a round number.
> 
>> + * However since the start time of the command, the time between
>> + * command and response, and the time between response and start of data is
>> + * not known, set the command transfer time to 2ms.
>> + */
>> +#define MMC_CMD_TRANSFER_TIME	(2 * NSEC_PER_MSEC) /* max 2 ms */
>> +
>>  enum sdhci_cookie {
>>  	COOKIE_UNMAPPED,
>>  	COOKIE_PRE_MAPPED,	/* mapped by sdhci_pre_req() */
>> @@ -554,6 +562,8 @@ struct sdhci_host {
>>  	/* Host SDMA buffer boundary. */
>>  	u32			sdma_boundary;
>>  
>> +	unsigned long long	data_timeout;
> 
> nsecs_to_jiffies() uses u64 which is nicer I think
> 
>> +
>>  	unsigned long private[0] ____cacheline_aligned;
>>  };
>>  
>>
> 
>