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

[Ulf Hansson <ulf.hansson@linaro.org>]

[...]

>>>> 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?

>>
>> 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?

>
>>
>>>
>>> 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?

Kind regards
Uffe