Re: [PATCH V1] mmc: sdhci: Update the software timeout value for sdhc

From: Adrian Hunter
Date: Mon Jun 21 2021 - 05:16:45 EST

On 19/06/21 1:49 am, Doug Anderson wrote:
> Hi,
>
> On Fri, Jun 18, 2021 at 8:31 AM Veerabhadrarao Badiganti
> <vbadigan@xxxxxxxxxxxxxx> wrote:
>>
>>
>> On 6/16/2021 2:55 PM, Shaik Sajida Bhanu wrote:
>>> Whenever SDHC run at clock rate 50MHZ or below, the hardware data
>>> timeout value will be 21.47secs, which is approx. 22secs and we have
>>> a current software timeout value as 10secs. We have to set software
>>> timeout value more than the hardware data timeout value to avioid seeing
>>> the below register dumps.
>>>
>>> [ 332.953670] mmc2: Timeout waiting for hardware interrupt.
>>> [ 332.959608] mmc2: sdhci: ============ SDHCI REGISTER DUMP ===========
>>> [ 332.966450] mmc2: sdhci: Sys addr: 0x00000000 | Version: 0x00007202
>>> [ 332.973256] mmc2: sdhci: Blk size: 0x00000200 | Blk cnt: 0x00000001
>>> [ 332.980054] mmc2: sdhci: Argument: 0x00000000 | Trn mode: 0x00000027
>>> [ 332.986864] mmc2: sdhci: Present: 0x01f801f6 | Host ctl: 0x0000001f
>>> [ 332.993671] mmc2: sdhci: Power: 0x00000001 | Blk gap: 0x00000000
>>> [ 333.000583] mmc2: sdhci: Wake-up: 0x00000000 | Clock: 0x00000007
>>> [ 333.007386] mmc2: sdhci: Timeout: 0x0000000e | Int stat: 0x00000000
>>> [ 333.014182] mmc2: sdhci: Int enab: 0x03ff100b | Sig enab: 0x03ff100b
>>> [ 333.020976] mmc2: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00000000
>>> [ 333.027771] mmc2: sdhci: Caps: 0x322dc8b2 | Caps_1: 0x0000808f
>>> [ 333.034561] mmc2: sdhci: Cmd: 0x0000183a | Max curr: 0x00000000
>>> [ 333.041359] mmc2: sdhci: Resp[0]: 0x00000900 | Resp[1]: 0x00000000
>>> [ 333.048157] mmc2: sdhci: Resp[2]: 0x00000000 | Resp[3]: 0x00000000
>>> [ 333.054945] mmc2: sdhci: Host ctl2: 0x00000000
>>> [ 333.059657] mmc2: sdhci: ADMA Err: 0x00000000 | ADMA Ptr:
>>> 0x0000000ffffff218
>>> [ 333.067178] mmc2: sdhci_msm: ----------- VENDOR REGISTER DUMP
>>> -----------
>>> [ 333.074343] mmc2: sdhci_msm: DLL sts: 0x00000000 | DLL cfg:
>>> 0x6000642c | DLL cfg2: 0x0020a000
>>> [ 333.083417] mmc2: sdhci_msm: DLL cfg3: 0x00000000 | DLL usr ctl:
>>> 0x00000000 | DDR cfg: 0x80040873
>>> [ 333.092850] mmc2: sdhci_msm: Vndr func: 0x00008a9c | Vndr func2 :
>>> 0xf88218a8 Vndr func3: 0x02626040
>>> [ 333.102371] mmc2: sdhci: ============================================
>>>
>>> So, set software timeout value more than hardware timeout value.
>>>
>>> Signed-off-by: Shaik Sajida Bhanu <sbhanu@xxxxxxxxxxxxxx>
>>> ---
>>> drivers/mmc/host/sdhci.c | 9 ++++++++-
>>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index bf238ad..1386f7d 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -1670,7 +1670,14 @@ static bool sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
>>> else if (!cmd->data && cmd->busy_timeout > 9000)
>>> timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;
>>> else
>>> - timeout += 10 * HZ;
>>> + /*
>>> + * In some of the conditions hardware data timeout value could be
>>> + * approx 21.5 seconds and driver is setting software data timeout
>>> + * value less than the hardware data timeout value and software data
>>> + * timeout value should be more than the hardware data timeout value.
>>> + * So, set software data timeout value more than 21.5 sec i.e. 22sec.
>>> + */
>>> + timeout += 22 * HZ;
>>
>> This timeout is qcom SDHC specific.
>> I think right way is to, define your own set_timeout op and update
>> host->data_timeout
>> in that as per qcom SDHC requirements.
>
> It is? Off-thread Shaik was indicating that the problem had to do with
> the inaccuracy of the "SDHCI_TIMEOUT_CONTROL" register. That seems to
> be in the common SDHCI code. Specifically looking at
> sdhci_calc_timeout() it can be seen that the possible hardware values
> for the timeout double each time, so if you need a hardware timeout
> that's slightly higher than one of the possible values you might end
> up with a hardware timeout that's almost double what you want.
>
> Assuming that the problem actually is with the inaccuracy of
> SDHCI_TIMEOUT_CONTROL (I didn't walk through and validate), it
> actually seems like we should generally be doubling the value we were
> programming for the software timeout (in other words, not just ones
> that are <= 9 seconds). I haven't done all the math, but I presume the
> reason that we need 22 instead of 20 is some type of extra fudge
> factor somewhere?
>
> Maybe the only reason that Qualcomm hits this is due to the PLL that's
> sourcing the SDHCI controller at a non-standard rate?
>
> I suppose another reason maybe why people aren't hitting it is just
> the random chance of what rate the integer overflow in
> mmc_set_data_timeout() leaves you at? I pointed this out to Shiak and
> was hoping a patch would be included for that, but I can always try to
> write one later if not. To be concrete, I have
> card->csd.taac_ns=5000000 and card->csd.r2w_factor=5. Multiplying
> things out (and accounting for mult=100 for SD cards), I end up with a
> timeout_ns of 0x3b9aca000 (16 seconds) which doesn't fit in the 32-bit
> data->timeout_ns earlier. The truncation was making it look like the
> card requested a max timeout of 3,115,098,112 ns = ~3.1 seconds.

The software timeout is meant to catch cases where the hardware
timeout has failed, but SDHCI also supports a mechanism to use
software timeouts when the timeout is too big for the hardware
- refer SDHCI_QUIRK2_DISABLE_HW_TIMEOUT and sdhci_calc_sw_timeout()

Historically, R/W timeouts were never longer than 2.5 seconds.
(5ms x 5 x 100 == 2.5s), so 10s has been enough ever since the
driver was written around 2006

Longer timeouts are for erase etc, but they are now always set
in cmd->busy_timeout.

So 22s is a strangely long timeout for R/W operations, and I would say
even 10s is longer than we really want.

One option would be to make it a variable that host controllers
can re-assign if needed e.g.

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 6aaf5c3ce34c..e5b4317937ed 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1670,7 +1670,7 @@ static bool sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
else if (!cmd->data && cmd->busy_timeout > 9000)
timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;
else
- timeout += 10 * HZ;
+ timeout += host->fallback_timeout_secs * HZ;
sdhci_mod_timer(host, cmd->mrq, timeout);

if (host->use_external_dma)
@@ -3936,6 +3936,8 @@ struct sdhci_host *sdhci_alloc_host(struct device *dev,
*/
host->adma_table_cnt = SDHCI_MAX_SEGS * 2 + 1;

+ host->fallback_timeout_secs = 10;
+
return host;
}