Re: [PATCH 02/13] mmc: host: Add facility to support re-tuning

[Arend van Spriel <arend@broadcom.com>]

On 01/15/15 14:39, Ulf Hansson wrote:
> On 15 January 2015 at 11:17, Adrian Hunter<adrian.hunter@intel.com>  wrote:
>> On 14/01/15 14:59, Ulf Hansson wrote:
>>> [...]
>>>
>>>>>
>>>>> The value from the register is also just randomly selected, only
>>>>> difference is that it's the HW that has randomly set it.
>>>>
>>>> Presumably the value is chosen based on the maximum rate of temperature
>>>> change and the corresponding effect that has on the signal.
>>>>
>>>>>
>>>>> Even if the above commit was merged, I don't think it was the correct
>>>>> way of dealing with re-tuning.
>>>>>
>>>>> First of all, re-tuning this is a mmc protocol specific thing should
>>>>> be managed from the mmc core, like the approach you have taken in your
>>>>> $subject patchset. Second I question whether the timer is useful at
>>>>> all.
>>>>
>>>> The SD Host Controller Specification does not document another way to do
>>>> mode 1 re-tuning. The timer is it. Otherwise re-tuning is never done.
>>>>
>>>> In the patches I sent, the driver must call mmc_retune_needed() to set
>>>> host->need_retune = 1 otherwise mmc_retune() does nothing.
>>>>
>>>> I would like to extend the model to include transparently re-tuning and
>>>> re-trying when there is a CRC error, but that is a separate issue, not
>>>> documented in the spec but recommended by others.
>>>>
>>>
>>> That perfect and in line from what I heard as recommendations from
>>> memory vendors as well.
>>
>> How would that work for SDIO? How do you know it is OK to retry SDIO operations?
>
> Retries or error handling, needs to be handled from SDIO func drivers
> or upper level code. They certainly also need it for other errors,
> which are not caused by the lack of a re-tune. I believe they exist
> already.
>
> For mmc core point of view, we need to act on SDIO data transfers
> errors and perform re-tuning for cases when it makes sense.
>
> More importantly, using a timer won't make SDIO data transfers error
> free, since we can still end up needing a re-tune at any point.
>
> Still, you do have point for SDIO. Minimizing the number of errors for
> SDIO could be important, due to that an SDIO func driver may not be
> able to recover from data errors as smoothly as the mmc block layer
> can. Thus, a timer could help to improve the situation, but I think it
> only makes sense in the SDIO case.
>
> BTW, what's your experience around SDIO cards supporting SDR104. I
> have never used such, have you?

My primary focus in all this discussing is about SDIO cards. The main 
reason being that our 11ac wifi SDIO cards do support SDR104. So the 
brcmfmac driver support SDIO and has retry mechanisms in place. However, 
it may also end-up doing an abort under certain conditions.

You also mentioned using runtime-pm, but how do you deal with func 
drivers not supporting runtime-pm. That is already an issue aka. bug 
right now. Our driver does not support runtime-pm (yet) and we have 
reported issues that host controller does runtime-pm basically killing 
communication between device and func driver.

Gr. AvS

>>
>>>
>>> Now, can we stop arguing about the timer and try without it?
>>>
>>> If we do see a need for a more frequent re-tuning to happen, due to
>>> that we get lots of CRC errors to recover from, then I think we should
>>> look into using runtime PM instead of the timer. And that's because I
>>> want to minimize the impact on performance.
>>
>> The minimum timer value is 1 second. The maximum is 1024 seconds. The ASUS
>> T100TA had a timer value of 128 seconds. The timer is not a performance issue.
>>
>> There is a performance question with runtime PM because that happens far
>> more frequently (typical auto-suspend delay is 50ms) and we re-tune after
>> that. In fact I generalized that a bit in patch 13.
>>
>>          [PATCH 13/13] mmc: sdhci: Change to new way of doing re-tuning
>>
>>          Make use of mmc core support for re-tuning instead
>>          of doing it all in the sdhci driver.
>>
>>          This patch also changes to flag the need for re-tuning
>>          always after runtime suspend when tuning has been used
>>          at initialization. Previously it was only done if
>>          the re-tuning timer was in use.
>>
>> One option to reduce the impact of the latency would be to increase the
>> auto-suspend delay.
>
> The latency will affect the first request after a runtime PM
> suspend/resume cycle. So for continues data transfers the impact
> should be zero. Also, increasing the delay would impact power
> consumption, but it's a balance I guess. :-)
>
> This is a specific issue for SDHCI (it's not clear to me if all SDHCI
> variants have the same behaviour). Obviously the mmc core needs to
> support the demand from SDHCI, such enable it to tell the core to
> perform a re-tune. Exactly what your patchset does.
>
> For your reference, I know about other controllers which can restore a
> bunch of register values, saved from earlier re-tunings, from its
> runtime PM resume callbacks. Thus preventing a re-tuning from happen.
> I wonder if some of the SDHCI variant are capable of this as well.
>
>
> Kind regards
> Uffe