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

[Adrian Hunter <adrian.hunter@intel.com>]

On 19/06/21 1:49 am, Doug Anderson wrote:
> Hi,
> 
> On Fri, Jun 18, 2021 at 8:31 AM Veerabhadrarao Badiganti
> <vbadigan@codeaurora.org> 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@codeaurora.org>
>>> ---
>>>   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;
 }