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

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

On 01/15/15 15:07, Arend van Spriel wrote:
> 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.

Could leave it to the function driver to call mmc_retune_needed().

Regards,
Arend

> 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
>