On 15 June 2012 15:22, Per Forlin <per.lkml@xxxxxxxxx> wrote:
> On Fri, Jun 15, 2012 at 10:34 AM, Saugata Das <saugata.das@xxxxxxxxxx> wrote:
>> On 15 June 2012 12:52, Ulf Hansson <ulf.hansson@xxxxxxxxxxxxxx> wrote:
>>> On 06/15/2012 05:49 AM, Saugata Das wrote:
>>>>
>>>> On 15 June 2012 00:36, Per Forlin<per.lkml@xxxxxxxxx> wrote:
>>>>>
>>>>> Hi Saugata,
>>>>>
>>>>> I can have a go and test it. But first I would like to bring up 3
>>>>> concerns that I have with this patch.
>>>>>
>>>>> 1. This patch should be sent to cc-stable in order to repair the bug
>>>>> introduced in 3.4
>>>>
>>>>
>>>> I shall sent it out today.
>>>>
>>>>> 2. Is the bus_ops for poweroff_notify really necessary since only mmc
>>>>> use it?
>>>>
>>>>
>>>> This was recommended in the review from Ulf. If it is not adding to a
>>>> bug, I propose to keep it this way. Otherwise, we will be going in
>>>> circles :-)
>>>
>>>
>>> Moreover it seems close related to sleep, which is implemented with bus_ops.
>>> So I would say, keep as is. We might change later, then both sleep and
>>> poweroff_notify together.
>>>
>>>>
>>>>> There are already bus_ops for suspend/resume,
>>>>> power_save/power_restore and remove. It feels like it would be
>>>>> possible to address poweroff_notify internally from mmc.c from theses
>>>>> bus_ops. I would be nice to add this feature without having to touch
>>>>> core.c.
>>>>>
>>>>> For instance. Call mmc_suspend() from mmc_remove()
>>>>> +++ b/drivers/mmc/core/mmc.c
>>>>> @@ -1302,7 +1302,7 @@ static void mmc_remove(struct mmc_host *host)
>>>>> + __mmc_suspend(host, true);
>>>>> mmc_remove_card(host->card);
>>>>>
>>>>> @@ -1347,7 +1347,7 @@ static void mmc_detect(struct mmc_host *host)
>>>>> -static int mmc_suspend(struct mmc_host *host)
>>>>> +static int __mmc_suspend(struct mmc_host *host, bool remove)
>>>>>
>>>>> @@ -1356,7 +1356,8 @@ static int mmc_suspend(struct mmc_host *host)
>>>>> mmc_claim_host(host);
>>>>> if (mmc_can_poweroff_notify(host->card)&&
>>>>> - (host->caps2& MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND)) {
>>>>> + (host->caps2& MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND ||
>>>>>
>>>>> + remove)) {
>>>>> err = mmc_poweroff_notify(host, MMC_PW_OFF_NOTIFY_SHORT);
>>>>> } else {
>>>>> if (mmc_card_can_sleep(host))
>>>>>
>>>>> @@ -1372,6 +1373,11 @@ static int mmc_suspend(struct mmc_host *host)
>>>>> +static int mmc_suspend(struct mmc_host *host)
>>>>> +{
>>>>> + return __mmc_suspend(host, false);
>>>>> +}
>>>>> +
>>>>>
>>>>> Calling mmc_suspend from mmc_remove() has another advantage since it
>>>>> may issue SLEEP (CMD5) before the card is removed and power off. This
>>>>> is recommended by eMMC Vendors in order to shutdown the eMMC safely to
>>>>> prevent data corruption. When the platform shuts down the power to the
>>>>> eMMC will be turned off no matter what.
>>>>
>>>>
>>>> May be for MMC-4.41 cards this approach can be taken. For MMC-4.5, it
>>>> has to be power OFF notify when the power is removed. Lets do it for
>>>> another patch, since the intention of this patch is to fix the issues
>>>> around power OFF notify.
>>>
>>>
>>> I agree with you Saugata, the exact same sequence as in suspend can not be
>>> used. The reason is simply that we do not want to consider
>>> MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND for doing poweroff_notify here, as we
>>> want in suspend.
>>>
>>> Leave this to be fixed in a separate patch instead.
>>>
>>>
>>>>
>>>>>
>>>>> 3. About the sleep and awake issue. This is not really related to
>>>>> poweroff_notify() as I see it. I would recommend to use CMD 0 to
>>>>> re-init the card safely after sleep in this patch. Then you could send
>>>>> out a sleep/awake patch that address this separately. This would also
>>>>> make #1 easier, send patch to cc-stable. Adding save/restore IOS is a
>>>>> new feature and should not be sent to the cc-stable list. What do you
>>>>> think?
>>>>
>>>>
>>>> The problem in sending CMD0 without power OFF notify is possibility of
>>>> some data loss in MMC-4.5 devices.
>>>
>>>
>>> I don't see the problem here. You will be sending power OFF notify when you
>>> can. The only difference is when you "wake" the device from sleep mode.
>>> Instead of using CMD5, which may work is some cases and in some cases not
>>> (without restoring ios). So using CMD0 as common way of waking up from
>>> suspend should be fine. Unless I missed something of course. :-)
>>>
>>
>> CMD0 is a reset. I expect with power OFF notify enable, the eMMC
>> device will postpone some control information update to its internal
>> non-volatile memory (e.g. some data structures which are kept in the
>> controller buffers and not stored in NAND). If we do a CMD0, then the
>> eMMC device will be reset and we may lose some data. In addition to
>> that, doing complete card initialization will increase the wakeup time
>> (for 4.4 devices).
>>
>> Till now, we have done complete card initialization during resume
> Yes, me and Ulf think we should still do a complete initialization, at
> least for now and in this patch.
>
In my opinion, that's incorrect on MMC-4.5 device and unoptimized for
MMC-4.41 device.
Let me propose a new cap, MMC_CAP2_NO_INIT_ON_RESUME and do something
like following in mmc_resume,
mmc_claim_host(host);
- if (mmc_card_is_sleep(host->card)) {
+ if (mmc_card_is_sleep(host->card) &&
+ (host->caps2 & MMC_CAP2_NO_INIT_ON_RESUME)) {
mmc_restore_ios(host, &host->saved_ios);
err = mmc_card_awake(host);
} else
err = mmc_init_card(host, host->ocr, host->card);
I hope it's OK for Ulf, Per, Subhash, Girish, Asutosh.
> A separate patch may deal with resume awake CMD5 and IOS save/restore.
>
> We may also discuss a clean up patch later on to reduce the number of
> bus_ops. Sleep, awake, and poweroff_notify are MMC specific.
> power_save/power_restore maps to suspend/resume. But let's not discuss
> this now :)
>
> BR
> Per
--
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
