On 23/01/17 19:07, Alex Lemberg wrote: > Hi Adrian, > > I would like to add several notes on top of Uri’s answer... > I agree with you, by the spec, PON Sleep_Notification should be > set before calling Sleep command. > As you probably remember, we have tried to implement it > in the past and even submitted a patch – “[PATCH] mmc: sleep notification” > https://www.mail-archive.com/linux-mmc@xxxxxxxxxxxxxxx/msg30906.html > As you can see from the thread, one of the main issues in Sleep_Notification > is a SLEEP_NOTIFICATION_TIME (defined in EXT_CSD [216]), which need to be > considered in implementation… Does it? I would tend to assume SLEEP_NOTIFICATION_TIME will be acceptable during _suspend until proven otherwise. > Thus, I think postponing Sleep during suspend requires a different from current > AUTO_BKOPS implementation, and I suggest to handle it in separate > patchset, if possible. Not at all sure what you mean by "postponing Sleep"? > > > Thanks, > Alex > > On 1/22/17, 5:55 PM, "Uri Yanai" <Uri.Yanai@xxxxxxxxxxx> wrote: > > Hi Adrian > > Thanks for your review. > Ulf rejected our approach in the first patch submission calling pm_schedule_suspend() > > " If I understand correctly, you would like to abort the runtime suspend > and allow the background operations to complete. That seems > reasonable, although in such case you need to return -EBUSY from this > function." > > If I understood Ulf correctly, instead of suspending he suggest aborting the suspend altogether returning -EBUSY. > > If I understood you correctly, your approach is similar to the one taken in the first patch submission. > Instead of calling pm_schedule_suspend(), you suggest calling mmc_poweroff_notify(host->card, SLEEP_NOTIFICATION). > > Regards > Uri > > -----Original Message----- > From: Adrian Hunter [mailto:adrian.hunter@xxxxxxxxx] > Sent: Friday, January 20, 2017 1:56 PM > To: Uri Yanai; ulf.hansson@xxxxxxxxxx > Cc: linux-mmc@xxxxxxxxxxxxxxx; chris@xxxxxxxxxx; Alex Lemberg > Subject: Re: [PATCH 3/3] mmc: Checking BKOPS status prior to Suspend > > On 17/01/17 14:15, Uri Yanai wrote: > > If asked to do device Runtime Suspend while in BKOPS and auto BKOPS is > > enabled return –EBUSY > > The spec also says > > "Before moving to Sleep state hosts may set the POWER_OFF_NOTIFICATION byte to SLEEP_NOTIFICATION (0x04) if aware that the device is capable of autonomously initiating background operations for possible performance improvements." > > Have you considered that? > > But I wonder if we shouldn't always use SLEEP_NOTIFICATION before sleep? > > > > > Change-Id: Ie4b38a32fe98179f0bca5b57edaaa47a02d0a389 > > Signed-off-by: Uri Yanai <uri.yanai@xxxxxxxxxxx> > > Signed-off-by: Alex Lemberg <alex.lemberg@xxxxxxxxxxx> > > --- > > drivers/mmc/core/mmc.c | 35 ++++++++++++++++++++++++++++------- > > include/linux/mmc/mmc.h | 2 ++ > > 2 files changed, 30 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index > > f70f6a1..21f5851 100644 > > --- a/drivers/mmc/core/mmc.c > > +++ b/drivers/mmc/core/mmc.c > > @@ -1942,7 +1942,8 @@ static void mmc_detect(struct mmc_host *host) > > } > > } > > > > -static int _mmc_suspend(struct mmc_host *host, bool is_suspend) > > +static int _mmc_suspend(struct mmc_host *host, bool is_suspend, > > + bool is_runtime_pm) > > { > > int err = 0; > > unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT : > > @@ -1953,10 +1954,30 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend) > > if (mmc_card_suspended(host->card)) > > goto out; > > > > - if (mmc_card_doing_bkops(host->card)) { > > - err = mmc_stop_bkops(host->card); > > - if (err) > > + if (mmc_card_doing_bkops(host->card) || > > + host->card->ext_csd.auto_bkops_en) { > > + err = mmc_read_bkops_status(host->card); > > + if (err) { > > + pr_err("%s: error %d reading BKOPS Status\n", > > + mmc_hostname(host), err); > > goto out; > > + } > > + if (is_runtime_pm && host->card->ext_csd.raw_bkops_status >= > > + EXT_CSD_BKOPS_LEVEL_PM_SUSPEND) { > > + /* > > + * We don’t allow Runtime Suspend while device still > > + * needs time to complete internal BKOPS, returning > > + * -EBUSY while BKOPS level is above > > + * EXT_CSD_BKOPS_LEVEL_PM_SUSPEND > > + */ > > + err = -EBUSY; > > + goto out; > > + } > > + if (mmc_card_doing_bkops(host->card)) { > > + err = mmc_stop_bkops(host->card); > > + if (err) > > + goto out; > > + } > > } > > > > err = mmc_flush_cache(host->card); > > @@ -1987,7 +2008,7 @@ static int mmc_suspend(struct mmc_host *host) { > > int err; > > > > - err = _mmc_suspend(host, true); > > + err = _mmc_suspend(host, true, false); > > if (!err) { > > pm_runtime_disable(&host->card->dev); > > pm_runtime_set_suspended(&host->card->dev); > > @@ -2034,7 +2055,7 @@ static int mmc_shutdown(struct mmc_host *host) > > err = _mmc_resume(host); > > > > if (!err) > > - err = _mmc_suspend(host, false); > > + err = _mmc_suspend(host, false, false); > > > > return err; > > } > > @@ -2058,7 +2079,7 @@ static int mmc_runtime_suspend(struct mmc_host *host) > > if (!(host->caps & MMC_CAP_AGGRESSIVE_PM)) > > return 0; > > > > - err = _mmc_suspend(host, true); > > + err = _mmc_suspend(host, true, true); > > if (err) > > pr_err("%s: error %d doing aggressive suspend\n", > > mmc_hostname(host), err); > > diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h index > > 126b8d5..2633274 100644 > > --- a/include/linux/mmc/mmc.h > > +++ b/include/linux/mmc/mmc.h > > @@ -445,6 +445,8 @@ struct _mmc_csd { > > */ > > #define EXT_CSD_BKOPS_LEVEL_2 0x2 > > > > +#define EXT_CSD_BKOPS_LEVEL_PM_SUSPEND EXT_CSD_BKOPS_LEVEL_2 > > + > > /* > > * BKOPS modes > > */ > > > > > -- 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
