Re: [PATCH 3/3] mmc: sdhci: Disable re-tuning for HS400

[Adrian Hunter <adrian.hunter@intel.com>]

On 02/12/14 13:20, Ulf Hansson wrote:
> On 2 December 2014 at 11:08, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 02/12/14 11:35, Ulf Hansson wrote:
>>> On 1 December 2014 at 14:16, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> Re-tuning for HS400 mode must be done in HS200
>>>> mode. Currently there is no support for that.
>>>> That needs to be reflected in the code.
>>>> Specifically, if tuning is executed in HS400 mode
>>>> then return an error, and if the re-tuning timer
>>>> is running when switching to HS400 mode, then
>>>> disable the timer.
>>>>
>>>> Note that periodic re-tuning is not expected
>>>> to be needed for HS400 but re-tuning is still
>>>> needed after the host controller has lost power.
>>>
>>> Why can't the old values be restored instead of trigger a re-tuning?
>>
>> The "values" (not sure what you mean by that) are not available to the
>> driver. Even if they were the operating conditions may have changed, (i.e.
>> temperature change) so the old "values" could still be wrong.
> 
> The "values" I refer to is those which we "calculated" during the
> tuning process.
> 
> What I had in mind, was that we should save these values at runtime PM
> suspend. And restore them at runtime PM resume. For some mmc
> controllers the "values" are typically just a some bits in a
> controller register, but that might not be true for all cases.
> 
> Regarding the temperature change, etc. I think that is what the
> periodic retuning should be taken care off.
> 
> Could you elaborate on why the "values" is not available to the driver?

The "optimal sampling point" has no corresponding register or value
in SDHCI.

> 
>>
>> Jedec spec. says:
>>
>>         It is recommended to perform tuning procedure while Device wakes
>>         up, after sleep.
>>
>> SDHCI spec. says:
>>
>>         If the Host System goes into power down mode, the Host Driver
>>         should stop the re-tuning timer and set the expiration flag
>>         to 1 when the Host System resumes from power down mode.
> 
> I am not sure how to interpret this. Is the context about system PM or
> runtime PM?

I interpret it to mean any situation where the host controller loses power
and therefore loses its tuning.

> 
>>
>>>
>>>> In the case of suspend/resume that is not necessary
>>>> because the card is fully re-initialised. That
>>>> just leaves runtime suspend/resume with no support
>>>> for HS400 re-tuning.
>>>>
>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>>> ---
>>>>  drivers/mmc/host/sdhci.c | 16 +++++++++++++++-
>>>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>>> index 2efa7fe..a7c9e67 100644
>>>> --- a/drivers/mmc/host/sdhci.c
>>>> +++ b/drivers/mmc/host/sdhci.c
>>>> @@ -1476,8 +1476,18 @@ void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing)
>>>>         else if ((timing == MMC_TIMING_UHS_DDR50) ||
>>>>                  (timing == MMC_TIMING_MMC_DDR52))
>>>>                 ctrl_2 |= SDHCI_CTRL_UHS_DDR50;
>>>> -       else if (timing == MMC_TIMING_MMC_HS400)
>>>> +       else if (timing == MMC_TIMING_MMC_HS400) {
>>>>                 ctrl_2 |= SDHCI_CTRL_HS400; /* Non-standard */
>>>> +               /*
>>>> +                * Periodic re-tuning for HS400 is not expected to be needed, so
>>>> +                * disable it here.
>>>
>>> Urgh, I don't like that the periodic tuning is handled by the host. We
>>> should never had merged that.
>>>
>>> How about trying to move the periodic tuning to be handled by the mmc
>>> core instead?
>>
>> I have patches for that. I hope to send them today.
> 
> Great! Looking forward to review them!
> 
> Kind regards
> Uffe
> 
>