Re: [PATCH V6 08/15] mmc: mmc: Hold re-tuning if the card is put to sleep

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

On 06/05/15 16:21, Ulf Hansson wrote:
> On 6 May 2015 at 14:42, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 06/05/15 14:36, Ulf Hansson wrote:
>>> [...]
>>>
>>>>>>> Instead of add mmc_retune_hold|release() to _mmc_suspend(), I would
>>>>>>> like you to move that handling into mmc_sleep(). The code should be
>>>>>>> easier and it becomes more clear that it's because of a command
>>>>>>> sequence.
>>>>>>>
>>>>>>> I think mmc_retune_hold() should be invoked before mmc_wait_for_cmd()
>>>>>>> and then mmc_retune_release() just after, in mmc_sleep(). That should
>>>>>>> work, right!?
>>>>>>
>>>>>> That would be the same as holding re-tuning for that request, which is
>>>>>> what already happens i.e. adding hold()/release() around mmc_wait_for_cmd()
>>>>>> is redundant.
>>>>>
>>>>> I don't understand your point, sorry.
>>>>
>>>> mmc_wait_for_cmd() calls mmc_wait_for_req() which calls __mmc_start_req()
>>>> which calls mmc_start_request() which calls mmc_retune_hold()
>>>>
>>>> Then mmc_wait_for_req() calls mmc_wait_for_req_done() which calls
>>>> mmc_retune_release().
>>>>
>>>> So
>>>>         mmc_wait_for_cmd() (with no retries)
>>>> has the same effect as
>>>>         mmc_retune_hold()
>>>>         mmc_wait_for_cmd()
>>>>         mmc_retune_release()
>>>>
>>>
>>> Huh, you are right - again.
>>>
>>> There have been a couple of iterations of this patchset, I don't
>>> recall why we need to hold retune for all requests? It seems awkward.
>>> Shouldn't we just hold retune for those requests that needs it?
>>
>> For data requests (which also call __mmc_start_req()) there is the
>> possibility that a 'write' is not finished and is polled with CMD13.
>> So re-tuning is held to avoid conflicting with the busy state.
>> It also aids controlling when re-tuning happens in the recovery path
>> i.e. we have a go at getting the status first and if that doesn't
>> work first time, then re-tune if needed.
>>
>> Also mmc_retune_hold() does not only hold retuning, it also causes
>> re-tuning to happen if the hold_count was zero, so it does
>> "make-retuning-happen-if-needed-and-not-already-held-and-then-hold-retuning"
> 
> Hmm, is there anyway we can make this easier to understand in the code
> path and maybe clarify via the name of functions/APIs you add? Could
> we have a state variable instead of bunch of int variables?

The ints are needed either to allow nesting or atomic update.

> 
> Since I apparently have a bit hard time to understand how this
> actually works, I am a bit concerned about the maintenance of it. :-)

There are only a few things to remember:

	1. Re-tuning can happen before every request i.e. inside mmc_wait_for_req()
or mmc_start_req()

	2. If you have several requests where re-tuning can't be done in between
them, then you can put mmc_retune_hold() / mmc_retune_release()
around them

	3. A sleep state, like the brcm custom sleep state, might need to prevent
re-tuning for the wakeup command

> 
> Anyway, if you can't find any better option - I will accept it as is.
> 
>>
>>>
>>>>>
>>>>> Anyway, my proposal didn't quite work, which is due to that
>>>>> mmc_deselect_cards() (invoked from mmc_sleep()) deals with retries. If
>>>>> there had been only one try, I thought it could be okay to have that
>>>>> command to be preceded by a re-tune.
>>>>>
>>>>> Anyway, I would like you to move the mmc_retune_hold|release() calls
>>>>> into the mmc_sleep() function.
>>>>
>>>> That would have no effect as explained above.
>>>
>>> Then why did you add it to the _mmc_suspend() function? What am I missing here?
>>
>> It was added in response to our discussions. It was not in my original
>> patches. I can take it out.
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> The options for the caller are:
>>>>>>
>>>>>> 1)
>>>>>>         hold re-tuning
>>>>>>         put emmc to sleep
>>>>>>         later wake up emmc
>>>>>>         release re-tuning
>>>>>>
>>>>>> 2)
>>>>>>         put emmc to sleep
>>>>>>         later increment hold_count
>>>>>>         wake up emmc ignoring CRC errors
>>>>>>         release re-tuning
>>>>>>
>>>>>> But there is no wake-up function and the suspend path is using an unbalanced
>>>>>> mmc_sleep i.e. no corresponding wake up.
>>>>>>
>>>>>> So that leaves what is happening now i.e. a comment plus explicit
>>>>>> hold()/release() in _mmc_suspend() so that future changes to _mmc_suspend()
>>>>>> know to take mmc_sleep re-tuning requirements into account.
>>>>>
>>>>> Why all this complexity?
>>>>>
>>>>> mmc_power_off() is called in _mmc_suspend(), that will eventually
>>>>> disable re-tune. Thus re-tuning will be prevented for
>>>>> commands/requests during the system PM resume sequence, until the card
>>>>> has been fully re-initialized (and a tuning sequence done). Isn't that
>>>>> sufficient?
>>>>
>>>> Yes my original patch did not have any of that complexity. I added it in
>>>> response to our discussions.
>>>>
>>>> As you wrote, _mmc_suspend() does not need to do anything with retuning
>>>> because mmc_sleep() is followed by mmc_power_off().
>>>>
>>>> The original patch added a comment to mmc_sleep() and that was all. That
>>>> would still be the best approach.
>>>>
>>>
>>> What I had in mind was that the re-tune timer could time out in the
>>> middle of the _mmc_suspend() sequence.
>>>
>>> If that happens in-between mmc_deselect_cards() and when the CMD5 is
>>> to be sent, in mmc_sleep() - we must not allow a re-tune sequence.
>>> Unless holding re-tune here, how is that prevented?
>>
>> Oh yes, I have overlooked that re-tuning can't be done on a de-selected
>> card. So I will add mmc_retune_hold()/mmc_retune_release(). I will have to
>> think about the error handling. It looks broken now anyway since it doesn't
>> reselect the card in the error path.
>>
> 
> I suggest you don't bother about the error handling for now, we can
> take that separately.

Ok, thanks!