Hi Ulf, On Fri, Jun 15, 2012 at 9:22 AM, 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) The remove parameter takes care of the remove case. >>> >>> @@ -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. I have addressed this in my prototype patch above. The remove param in __suspend tells if we are removing or just suspending. This approach actually removes lines of code in core.c mmc_stop_host(). > > >> >>> >>> 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. :-) > I'm in favour of simplifying this patch. CMD0 instead of CMD5 reduces the number of lines in this patch. It also make this patch work :) Using CMD 5 to wake up could be done in a separate patch. Bottom line. If the patch is clean and works I'm happy. I can help out and test the patch. Regards, 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