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

Ulf Hansson
2015-01-14T10:13:35Z

On 14 January 2015 at 10:57, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 14/01/15 11:47, Ulf Hansson wrote:
>> On 13 January 2015 at 17:02, Arend van Spriel <arend@broadcom.com> wrote:
>>> On 01/13/15 16:41, Ulf Hansson wrote:
>>>>
>>>> On 13 January 2015 at 16:11, Arend van Spriel<arend@broadcom.com> wrote:
>>>>>
>>>>> On 01/13/15 15:56, Ulf Hansson wrote:
>>>>>>
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>>>> Thank you for looking at the patches.
>>>>>>>>>
>>>>>>>>> I am not sure I know what you mean. sdhci already has a re-tuning
>>>>>>>>> timer, so
>>>>>>>>> this is just moving it into core, where it won't be used by other
>>>>>>>>> drivers
>>>>>>>>> unless they enable it.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> I am kind of questioning the re-tuning timer in sdhci. What is it good
>>>>>>>> for?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> It is part of the SD Host Controller Standard Specification. The timer
>>>>>>> ensures that re-tuning is done before temperature changes could affect
>>>>>>> the
>>>>>>> "sampling point". It is needed for re-tuning mode 1 for UHS-I modes
>>>>>>> like
>>>>>>> SDR104.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Does the spec say what value the timer should have?
>>>>>
>>>>>
>>>>>
>>>>> It is read from the Capabilities register in the SD host controller, ie.
>>>>> in
>>>>> field "Timer Count for Re-Tuning" (see below).
>>>>>
>>>>> Regards,
>>>>> Arend
>>>>>
>>>>> Timer Count for Re-Tuning
>>>>> This field indicates an initial value of the Re-Tuning Timer for
>>>>> Re-Tuning
>>>>> Mode 1 to 3. Setting to 0 disables Re-Tuning Timer.
>>>>> 0h Re-Tuning Timer disabled
>>>>> 1h 1 seconds
>>>>> 2h 2 seconds
>>>>> 3h 4 seconds
>>>>> 4h 8 seconds
>>>>> ..... ......................
>>>>> n 2(n-1) seconds
>>>>> ..... ......................
>>>>> Bh 1024 seconds
>>>>> Eh - Ch Reserved
>>>>> Fh Get information from other source
>>>>
>>>>
>>>> Thanks for sharing this information, but unfortunate I don't
>>>> understand much from it.
>>>>
>>>> Is the host driver intended to read/poll this register to find a good
>>>> value?
>>>
>>>
>>> You can download the spec (and others) here [1]. sdhci currently implements
>>> retuning mode 1, which is decribed in the spec:
>>>
>>> Re-Tuning Timer Control Example for Re-Tuning Mode 1
>>> The initial value of re-tuning timer is provided by Timer Count for
>>> Re-Tuning field in this register. The timer starts counting by loading the
>>> initial value. When the timer expires, the Host Driver marks an expiration
>>> flag. On receiving a command request, the Host driver checks the expiration
>>> flag. If the expiration flag is set, then the Host Driver should perform the
>>> re-tuning procedure before issuing a command. If the expiration flag is not
>>> set, then the Host Driver issues a command without performing the re-tuning
>>> procedure. Every time the re-tuning procedure is performed, the timer loads
>>> the new initial value and the expiration flag is cleared.
>>>
>>> So the host controller could indeed update this register for subsequent
>>> retuning.
>>
>> Arend, thanks for the link and information. So, I decided to go for a
>> look in there.
>>
>>>From the same section you quoted above:
>> ------
>> (1) Re-Tuning Mode 1
>> The host controller does not have any internal logic to detect when
>> the re-tuning needs to be performed. In this case, the Host Driver
>> should maintain all re-tuning timings by using a Re-Tuning Timer. To
>> enable inserting the re-tuning procedure during data transfers, the
>> data length per read/write command shall be limited up to 4MB.
>> ------
>>
>> That means, we can't get _any_ help from the controller HW (in mode 1)
>> to find a good value for the timer.
>
> In fact the timer value *is* defined in the Capabilities Register (Offset
> 040h) bits 43-40 Timer Count for Re-Tuning
>
> It has been supported since 2011, see:
>
> commit cf2b5eea1ea0ff9b3184bc6771bcb93a9fdcd1d9
> "mmc: sdhci: add support for retuning mode 1"
>

The value from the register is also just randomly selected, only
difference is that it's the HW that has randomly set it.

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.

Kind regards
Uffe