On 6 May 2015 at 10:39, Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:
> On 04/05/15 16:44, Ulf Hansson wrote:
>> On 20 April 2015 at 14:09, Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:
>>> Currently "mmc sleep" is used before power off and
>>> is not paired with waking up. Nevertheless hold
>>> re-tuning.
>>>
>>> Signed-off-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
>>> ---
>>> drivers/mmc/core/mmc.c | 14 +++++++++++---
>>> 1 file changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>> index f36c76f..daf9954 100644
>>> --- a/drivers/mmc/core/mmc.c
>>> +++ b/drivers/mmc/core/mmc.c
>>> @@ -21,6 +21,7 @@
>>> #include <linux/mmc/mmc.h>
>>>
>>> #include "core.h"
>>> +#include "host.h"
>>> #include "bus.h"
>>> #include "mmc_ops.h"
>>> #include "sd_ops.h"
>>> @@ -1504,6 +1505,7 @@ static int mmc_can_sleep(struct mmc_card *card)
>>> return (card && card->ext_csd.rev >= 3);
>>> }
>>>
>>> +/* If necessary, callers must hold re-tuning */
>>
>> Remove this comment.
>>
>>> static int mmc_sleep(struct mmc_host *host)
>>> {
>>> struct mmc_command cmd = {0};
>>> @@ -1631,6 +1633,7 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>>> int err = 0;
>>> unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT :
>>> EXT_CSD_POWER_OFF_LONG;
>>> + bool retune_release = false;
>>>
>>> BUG_ON(!host);
>>> BUG_ON(!host->card);
>>> @@ -1651,17 +1654,22 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>>> goto out;
>>>
>>> if (mmc_can_poweroff_notify(host->card) &&
>>> - ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend))
>>> + ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) {
>>> err = mmc_poweroff_notify(host->card, notify_type);
>>> - else if (mmc_can_sleep(host->card))
>>> + } else if (mmc_can_sleep(host->card)) {
>>> + mmc_retune_hold(host);
>>> err = mmc_sleep(host);
>>> - else if (!mmc_host_is_spi(host))
>>> + } else if (!mmc_host_is_spi(host)) {
>>> err = mmc_deselect_cards(host);
>>> + }
>>>
>>> if (!err) {
>>> mmc_power_off(host);
>>> mmc_card_set_suspended(host->card);
>>> }
>>> +
>>> + if (retune_release)
>>> + mmc_retune_release(host);
>>> out:
>>> mmc_release_host(host);
>>> return err;
>>
>> 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.
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.
>
> 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?
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
