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

Ulf Hansson
2015-01-15T14:46:04Z

On 15 January 2015 at 15:17, Arend van Spriel <arend@broadcom.com> wrote:
> 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.
>

Runtime PM is implemented a bit differently between SDIO vs MMC/SD.
Your are right.

For MMC/SD the mmc block device handles pm_runtime_get|put() in
principle per request basis and makes use of the
pm_runtime_autosuspend feature. While in the SDIO case, it's entirely
up the SDIO func driver to deal with pm_runtime_get|put().

So it seems like we can use runtime PM for MMC/SD but not for SDIO. At
least not using the SDIO func device.

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

Hmm, the positive side from such approach would be that the SDIO func
driver can decide when it's convenient to do a re-tune.

The negative side is that all SDIO func driver would need to care
about this. I am not sure we want that.

Kind regards
Uffe