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;
>> };
>>
>>
>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
