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

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

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.

Regards,
Arend

> Kind regards
> Uffe