Re: [PATCH] mmc: core: Send SLEEP_NOTIFICATION for eMMC 5.x device

From: Gwendal Grignou <gwendal@chromium.org>
To: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Gwendal Grignou <gwendal@chromium.org>,
	"linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
	Avi Shchislowski <Avi.Shchislowski@sandisk.com>,
	Chris Ball <cjb@laptop.org>,
	Alex Lemberg <Alex.Lemberg@sandisk.com>,
	Thierry Escande <thierry.escande@collabora.com>
Subject: Re: [PATCH] mmc: core: Send SLEEP_NOTIFICATION for eMMC 5.x device
Date: Tue, 19 Dec 2017 17:57:12 -0800	[thread overview]
Message-ID: <CAMHSBOU9YPsMZ0aGDy6v+p6GY5ndQrqpz4rt6G9M2aKj0MXcGw@mail.gmail.com> (raw)
In-Reply-To: <CAPDyKFqH3w4j-K0QJj5o9ag42BTDxkpjT5DbqErcZxNK8-Da+w@mail.gmail.com>

Ufe,

I have been side-tracked but will get back on it. I will be the worst
case scenario (lot of random write before suspend for different eMMC).
I also need to rework the wait for DAT0 to deassert instead of the
busy line as usual.

Gwendal.

On Fri, Dec 15, 2017 at 1:24 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 29 November 2017 at 15:14, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> [...]
>>
>>>>  static int mmc_poweroff_notify(struct mmc_card *card, unsigned int notify_type)
>>>>  {
>>>> -       unsigned int timeout = card->ext_csd.generic_cmd6_time;
>>>> +       unsigned int timeout;
>>>> +       bool use_busy_signal = true;
>>>>         int err;
>>>>
>>>> -       /* Use EXT_CSD_POWER_OFF_SHORT as default notification type. */
>>>> -       if (notify_type == EXT_CSD_POWER_OFF_LONG)
>>>> +       switch (notify_type) {
>>>> +       case EXT_CSD_POWER_OFF_LONG:
>>>>                 timeout = card->ext_csd.power_off_longtime;
>>>> +               break;
>>>> +       case EXT_CSD_SLEEP_NOTIFICATION:
>>>> +               timeout = card->ext_csd.sleep_notification_time;
>>>> +               use_busy_signal = false;
>>>
>>> This is wrong.
>>>
>>> If you set use_busy_signal to false, it means that we don't care about
>>> waiting for the card to stop signaling busy on DAT0 after setting
>>> EXT_CSD_SLEEP_NOTIFICATION. This then becomes a violation of the eMMC
>>> spec, because we must not issue a CMD5 before the card have stopped
>>> signaling busy.
>>>
>>> I realize that for those devices that don't support
>>> MMC_CAP_WAIT_WHILE_BUSY or have the ->card_busy() callback
>>> implemented, the consequence may be that mmc_poll_for_busy() may call
>>> mmc_delay() with a very big timeout. This could be a big problem, I
>>> guess.
>>
>> Well, it may also be a big problem even if the host supports
>> ->card_busy() and/or MMC_CAP_WAIT_WHILE_BUSY (but maybe not as big),
>> simply because we can't be waiting here for several seconds to allow
>> the card to deal with background operations.
>>
>> I guess we need to try and see what happens. Perhaps you can share
>> some data about the timeouts you get?
>>
>>>
>>>> +               break;
>>>> +       default:
>>>> +               /* Use EXT_CSD_POWER_OFF_SHORT as default notification type. */
>>>> +               timeout = card->ext_csd.generic_cmd6_time;
>>>> +       }
>>>>
>>>>         err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>>>>                         EXT_CSD_POWER_OFF_NOTIFICATION,
>>>> -                       notify_type, timeout, 0, true, false, false);
>>>> +                       notify_type, timeout, 0, use_busy_signal, false, false);
>>>>         if (err)
>>>>                 pr_err("%s: Power Off Notification timed out, %u\n",
>>>>                        mmc_hostname(card->host), timeout);
>>
>> [...]
>>
>> Kind regards
>> Uffe
>
> Gwendal, any news on this?
>
> As changes for the SLEEP_NOTIFICATION keeps being posted, clearly
> there is a need for us to deal with it in some way. I have been
> thinking of a fall-back solution, if we can't get it it work, but
> anyway I am eager to find a solution so we finally can sort this out.
>
> Kind regards
> Uffe