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

Ulf Hansson
2015-01-19T09:27:58Z

On 15 January 2015 at 15:59, Arend van Spriel <arend@broadcom.com> wrote:
> On 01/15/15 15:46, Ulf Hansson wrote:
>>
>> 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.
>
>
> I would say "appropriate" instead of "convenient".
>
>> The negative side is that all SDIO func driver would need to care
>> about this. I am not sure we want that.
>
>
> The whole retry handling also seems deferred to the SDIO func driver and the
> same for runtime-pm. As the "retune needed" question would pops up during
> the retry handling it seems not a bad option.

Okay, your argument seems reasonable, let's got for this approach.

Kind regards
Uffe