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