[PATCH 3/3] mmc: Checking BKOPS status prior to Suspend

[PATCH 3/3] mmc: Checking BKOPS status prior to Suspend

[alex lemberg]

Rescheduling Suspend in case of BKOPS Level >= 1
in order to let eMMC device to complete its internal GC.
Applicable for Runtime Suspend Only.

Signed-off-by: alex lemberg <alex.lemberg@sandisk.com>
---
 drivers/mmc/core/mmc.c  | 30 +++++++++++++++++++++++-------
 include/linux/mmc/mmc.h |  1 +
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index e2e987f..c4c6326 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1904,7 +1904,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 :
@@ -1918,10 +1919,25 @@ 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_1) {
+			pm_schedule_suspend(&host->card->dev,
+				MMC_RUNTIME_SUSPEND_DELAY_MS);
 			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);
@@ -1952,7 +1968,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);
@@ -2002,7 +2018,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;
 }
@@ -2026,7 +2042,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 0fe3908..0f08d5c 100644
--- a/include/linux/mmc/mmc.h
+++ b/include/linux/mmc/mmc.h
@@ -430,6 +430,7 @@ struct _mmc_csd {
 /*
  * BKOPS status level
  */
+#define EXT_CSD_BKOPS_LEVEL_1		0x1
 #define EXT_CSD_BKOPS_LEVEL_2		0x2
 
 /*
-- 
1.9.1

Re: [PATCH 3/3] mmc: Checking BKOPS status prior to Suspend

[Ulf Hansson]

On 1 September 2016 at 16:24, alex lemberg <alex.lemberg@sandisk.com> wrote:
> Rescheduling Suspend in case of BKOPS Level >= 1
> in order to let eMMC device to complete its internal GC.
> Applicable for Runtime Suspend Only.
>
> Signed-off-by: alex lemberg <alex.lemberg@sandisk.com>
> ---
>  drivers/mmc/core/mmc.c  | 30 +++++++++++++++++++++++-------
>  include/linux/mmc/mmc.h |  1 +
>  2 files changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index e2e987f..c4c6326 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1904,7 +1904,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 :
> @@ -1918,10 +1919,25 @@ 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;
> +               }

I would appreciate some simple comment of what you want to do here below.

> +               if (is_runtime_pm && host->card->ext_csd.raw_bkops_status >=
> +                               EXT_CSD_BKOPS_LEVEL_1) {
> +                       pm_schedule_suspend(&host->card->dev,
> +                               MMC_RUNTIME_SUSPEND_DELAY_MS);
>                         goto out;

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.
Moreover, perhaps we should discuss at what BKOPS_LEVEL* we should
allow to abort.

> +               }
> +               if (mmc_card_doing_bkops(host->card)) {
> +                       err = mmc_stop_bkops(host->card);
> +                       if (err)
> +                               goto out;
> +               }
>         }
>
>         err = mmc_flush_cache(host->card);
> @@ -1952,7 +1968,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);
> @@ -2002,7 +2018,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;
>  }
> @@ -2026,7 +2042,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 0fe3908..0f08d5c 100644
> --- a/include/linux/mmc/mmc.h
> +++ b/include/linux/mmc/mmc.h
> @@ -430,6 +430,7 @@ struct _mmc_csd {
>  /*
>   * BKOPS status level
>   */
> +#define EXT_CSD_BKOPS_LEVEL_1          0x1
>  #define EXT_CSD_BKOPS_LEVEL_2          0x2
>
>  /*
> --
> 1.9.1
>

Another idea I had, was to actually make use of that we know the card
will be inactive. So instead of checking if there is an ongoing BKOPS
(mmc_card_doing_bkops()), in case of the manual BKOPS, we could
potentially check the needed BKOPS LEVEL and schedule a BKOPS to
start. In other words, do manual BKOPS when the card is idle.

Of course, one need to be able to interrupt/cancel the BKOPS if there
is a new request that needs to be managed. I am not sure how we can
achieve that, but I hope you get the idea.

Kind regards
Uffe

Re: [PATCH 3/3] mmc: Checking BKOPS status prior to Suspend

[Alex Lemberg]

Hi Ulf,

Thank you for reviewing the patch!

On 10/13/16, 10:36 AM, "Ulf Hansson" <ulf.hansson@linaro.org> wrote:

>On 1 September 2016 at 16:24, alex lemberg <alex.lemberg@sandisk.com> wrote:
>> Rescheduling Suspend in case of BKOPS Level >= 1
>> in order to let eMMC device to complete its internal GC.
>> Applicable for Runtime Suspend Only.
>>
>> Signed-off-by: alex lemberg <alex.lemberg@sandisk.com>
>> ---
>>  drivers/mmc/core/mmc.c  | 30 +++++++++++++++++++++++-------
>>  include/linux/mmc/mmc.h |  1 +
>>  2 files changed, 24 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index e2e987f..c4c6326 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1904,7 +1904,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 :
>> @@ -1918,10 +1919,25 @@ 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;
>> +               }
>
>I would appreciate some simple comment of what you want to do here below.
>
>> +               if (is_runtime_pm && host->card->ext_csd.raw_bkops_status >=
>> +                               EXT_CSD_BKOPS_LEVEL_1) {
>> +                       pm_schedule_suspend(&host->card->dev,
>> +                               MMC_RUNTIME_SUSPEND_DELAY_MS);
>>                         goto out;
>
>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.

In my mind I wanted to reschedule runtime suspend, but your recommendation
is makes sense. I will change it and re-submit.

>Moreover, perhaps we should discuss at what BKOPS_LEVEL* we should
>allow to abort.

Agree, the BKOPS_LEVEL value is something that can be interpreted differently
by different vendors.
Should I define a default value like “BKOPS_LEVEL_TO_USE”, and 
also maybe add a sysfs entry to allow user configuration? 

>
>> +               }
>> +               if (mmc_card_doing_bkops(host->card)) {
>> +                       err = mmc_stop_bkops(host->card);
>> +                       if (err)
>> +                               goto out;
>> +               }
>>         }
>>
>>         err = mmc_flush_cache(host->card);
>> @@ -1952,7 +1968,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);
>> @@ -2002,7 +2018,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;
>>  }
>> @@ -2026,7 +2042,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 0fe3908..0f08d5c 100644
>> --- a/include/linux/mmc/mmc.h
>> +++ b/include/linux/mmc/mmc.h
>> @@ -430,6 +430,7 @@ struct _mmc_csd {
>>  /*
>>   * BKOPS status level
>>   */
>> +#define EXT_CSD_BKOPS_LEVEL_1          0x1
>>  #define EXT_CSD_BKOPS_LEVEL_2          0x2
>>
>>  /*
>> --
>> 1.9.1
>>
>
>Another idea I had, was to actually make use of that we know the card
>will be inactive. So instead of checking if there is an ongoing BKOPS
>(mmc_card_doing_bkops()), in case of the manual BKOPS, we could
>potentially check the needed BKOPS LEVEL and schedule a BKOPS to
>start. In other words, do manual BKOPS when the card is idle.

If I understand correctly, the idea is to allow MANUAL_ BKOPS in 
a similar way as it done for AUTO_BKOPS during runtime suspend, right?

>
>Of course, one need to be able to interrupt/cancel the BKOPS if there
>is a new request that needs to be managed. I am not sure how we can
>achieve that, but I hope you get the idea.

Right, in case of MANUAL_BKOPS, the need the HPI to be issued
in order to stop MANUAL_BKOPS immediately on a new request.
BTW, the same challenge we had on SLEEP_NOTIFICATION…

Since the AUTO_BKOPS is more simple case and doesn’t require 
the interruption ability from the driver, may I suggest to make it 
in a separate patchset?

>
>Kind regards
>Uffe

[PATCH 0/3] Auto BKOPS PM suspend

[Uri Yanai]

Hi Ulf
Resubmitting three patches for approval, originally submitted by Alex Lemberg <alex.lemberg@sandisk.com>
[1/3] mmc: replacing hardcoded value for runtime PM suspend  		Message ID:<1472739890-3384-2-git-send-email-alex.lemberg@sandisk.com>
[2/3] mmc: Adding AUTO_BKOPS_EN bit set for Auto BKOPS support		Message ID:<1472739890-3384-3-git-send-email-alex.lemberg@sandisk.com>
[3/3] mmc: Checking BKOPS status prior to Suspend 			Message ID:<1472739890-3384-4-git-send-email-alex.lemberg@sandisk.com>

Thanks
Uri Yanai

Uri Yanai (3):
  mmc: replacing hardcoded value for runtime PM suspend
  mmc: Adding AUTO_BKOPS_EN bit set for Auto BKOPS support
  mmc: Checking BKOPS status prior to Suspend

 drivers/mmc/core/block.c |  3 ++-
 drivers/mmc/core/mmc.c   | 45 ++++++++++++++++++++++++++++++++++++---------
 include/linux/mmc/card.h |  1 +
 include/linux/mmc/host.h |  2 ++
 include/linux/mmc/mmc.h  |  3 +++
 5 files changed, 44 insertions(+), 10 deletions(-)

-- 
1.9.1

[PATCH 1/3] mmc: replacing hardcoded value for runtime PM suspend

[Uri Yanai]

Adds define MMC_RUNTIME_SUSPEND_DELAY_MS for setting delay
time(in milliseconds) before entering device runtime suspend.
Setting the default to 3000 milliseconds.

Change-Id: Ifab084cc01d8dff8c79606f8de0858f0821e98bc
Signed-off-by: Uri Yanai <uri.yanai@sandisk.com>
Signed-off-by: Alex Lemberg <alex.lemberg@sandisk.com>
---
 drivers/mmc/core/block.c | 3 ++-
 include/linux/mmc/host.h | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 6648a17..7954f15 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -2202,7 +2202,8 @@ static int mmc_blk_probe(struct mmc_card *card)
 			goto out;
 	}
 
-	pm_runtime_set_autosuspend_delay(&card->dev, 3000);
+	pm_runtime_set_autosuspend_delay(&card->dev,
+			MMC_RUNTIME_SUSPEND_DELAY_MS);
 	pm_runtime_use_autosuspend(&card->dev);
 
 	/*
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 8bc8841..4a0b30f 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -22,6 +22,8 @@
 #include <linux/mmc/mmc.h>
 #include <linux/mmc/pm.h>
 
+#define MMC_RUNTIME_SUSPEND_DELAY_MS	3000
+
 struct mmc_ios {
 	unsigned int	clock;			/* clock rate */
 	unsigned short	vdd;
-- 
1.9.1

[PATCH 2/3] mmc: Adding AUTO_BKOPS_EN bit set for Auto BKOPS support

[Uri Yanai]

Read from the device EXT_CSD and set to the card->ext_csd structure

Change-Id: I940b1612f5bac078c330ad3829d7aa5f0ba0989e
Signed-off-by: Uri Yanai <uri.yanai@sandisk.com>
Signed-off-by: Alex Lemberg <alex.lemberg@sandisk.com>
---
 drivers/mmc/core/mmc.c   | 10 ++++++++--
 include/linux/mmc/card.h |  1 +
 include/linux/mmc/mmc.h  |  1 +
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index c0e2507..f70f6a1 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -530,8 +530,14 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
 						EXT_CSD_MANUAL_BKOPS_MASK);
 			card->ext_csd.raw_bkops_status =
 				ext_csd[EXT_CSD_BKOPS_STATUS];
-			if (!card->ext_csd.man_bkops_en)
-				pr_debug("%s: MAN_BKOPS_EN bit is not set\n",
+			if (card->ext_csd.man_bkops_en)
+				pr_debug("%s: MAN_BKOPS_EN bit is set\n",
+					mmc_hostname(card->host));
+			card->ext_csd.auto_bkops_en =
+					(ext_csd[EXT_CSD_BKOPS_EN] &
+						EXT_CSD_AUTO_BKOPS_MASK);
+			if (card->ext_csd.auto_bkops_en)
+				pr_debug("%s: AUTO_BKOPS_EN bit is set\n",
 					mmc_hostname(card->host));
 		}
 
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 00449e5..e3f1031 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -84,6 +84,7 @@ struct mmc_ext_csd {
 	unsigned int		hpi_cmd;		/* cmd used as HPI */
 	bool			bkops;		/* background support bit */
 	bool			man_bkops_en;	/* manual bkops enable bit */
+	bool			auto_bkops_en;	/* auto bkops enable bit */
 	unsigned int            data_sector_size;       /* 512 bytes or 4KB */
 	unsigned int            data_tag_unit_size;     /* DATA TAG UNIT size */
 	unsigned int		boot_ro_lock;		/* ro lock support */
diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
index a074082..126b8d5 100644
--- a/include/linux/mmc/mmc.h
+++ b/include/linux/mmc/mmc.h
@@ -449,6 +449,7 @@ struct _mmc_csd {
  * BKOPS modes
  */
 #define EXT_CSD_MANUAL_BKOPS_MASK	0x01
+#define EXT_CSD_AUTO_BKOPS_MASK		0x02
 
 /*
  * Command Queue
-- 
1.9.1

[PATCH 3/3] mmc: Checking BKOPS status prior to Suspend

[Uri Yanai]

If asked to do device Runtime Suspend while
in BKOPS and auto BKOPS is enabled return –EBUSY

Change-Id: Ie4b38a32fe98179f0bca5b57edaaa47a02d0a389
Signed-off-by: Uri Yanai <uri.yanai@sandisk.com>
Signed-off-by: Alex Lemberg <alex.lemberg@sandisk.com>
---
 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
  */
-- 
1.9.1

Re: [PATCH 0/3] Auto BKOPS PM suspend

[Shawn Lin]

Hi Uri,

On 2017/1/17 20:15, Uri Yanai wrote:
> Hi Ulf
> Resubmitting three patches for approval, originally submitted by Alex Lemberg <alex.lemberg@sandisk.com>
> [1/3] mmc: replacing hardcoded value for runtime PM suspend  		Message ID:<1472739890-3384-2-git-send-email-alex.lemberg@sandisk.com>
> [2/3] mmc: Adding AUTO_BKOPS_EN bit set for Auto BKOPS support		Message ID:<1472739890-3384-3-git-send-email-alex.lemberg@sandisk.com>
> [3/3] mmc: Checking BKOPS status prior to Suspend 			Message ID:<1472739890-3384-4-git-send-email-alex.lemberg@sandisk.com>
>
> Thanks
> Uri Yanai

Firstly, the authorship isn't correct and of course the SOB path.
Then, it'd better to add a changelog since you do improve Alex's
patches, and it would help us to better understand what was changed. :)

Also you should remove Change-Id when fetching patches from your
internal code review system.

Anyway, let's talk about the patches themself, I have
a question for your patch 3/3 there.


>
> Uri Yanai (3):
>   mmc: replacing hardcoded value for runtime PM suspend
>   mmc: Adding AUTO_BKOPS_EN bit set for Auto BKOPS support
>   mmc: Checking BKOPS status prior to Suspend
>
>  drivers/mmc/core/block.c |  3 ++-
>  drivers/mmc/core/mmc.c   | 45 ++++++++++++++++++++++++++++++++++++---------
>  include/linux/mmc/card.h |  1 +
>  include/linux/mmc/host.h |  2 ++
>  include/linux/mmc/mmc.h  |  3 +++
>  5 files changed, 44 insertions(+), 10 deletions(-)
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 3/3] mmc: Checking BKOPS status prior to Suspend

[Shawn Lin]

On 2017/1/17 20:15, Uri Yanai wrote:
> If asked to do device Runtime Suspend while
> in BKOPS and auto BKOPS is enabled return –EBUSY
>

The commit msg isn't correct as the code just return -EBUSY
when the level of BKOPS is EXT_CSD_BKOPS_LEVEL_PM_SUSPEND.

And how do you decide that EXT_CSD_BKOPS_LEVEL_2 should be used.
 From the spec, it means "performance being impacted". It would be
good to know a bit details inside the eMMC firmware.

BTW, should we use EXT_CSD_BKOPS_LEVEL_2 directly since it looks
more strightforward?


> Change-Id: Ie4b38a32fe98179f0bca5b57edaaa47a02d0a389
> Signed-off-by: Uri Yanai <uri.yanai@sandisk.com>
> Signed-off-by: Alex Lemberg <alex.lemberg@sandisk.com>
> ---
>  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
>   */
>


-- 
Best Regards
Shawn Lin

RE: [PATCH 0/3] Auto BKOPS PM suspend

[Uri Yanai]

Hi Shawn

Thanks for the review, As you request I will add changelog and remove the Change-Id.
I am afraid I didn't quite understand your remark about authorship and  the SOB path, appreciate if you can elaborate on this.

Thanks
Uri

-----Original Message-----
From: Shawn Lin [mailto:shawn.lin@rock-chips.com] 
Sent: Wednesday, January 18, 2017 3:54 AM
To: Uri Yanai
Cc: ulf.hansson@linaro.org; shawn.lin@rock-chips.com; linux-mmc@vger.kernel.org; chris@printf.net; Alex Lemberg
Subject: Re: [PATCH 0/3] Auto BKOPS PM suspend

Hi Uri,

On 2017/1/17 20:15, Uri Yanai wrote:
> Hi Ulf
> Resubmitting three patches for approval, originally submitted by Alex Lemberg <alex.lemberg@sandisk.com>
> [1/3] mmc: replacing hardcoded value for runtime PM suspend  		Message ID:<1472739890-3384-2-git-send-email-alex.lemberg@sandisk.com>
> [2/3] mmc: Adding AUTO_BKOPS_EN bit set for Auto BKOPS support		Message ID:<1472739890-3384-3-git-send-email-alex.lemberg@sandisk.com>
> [3/3] mmc: Checking BKOPS status prior to Suspend 			Message ID:<1472739890-3384-4-git-send-email-alex.lemberg@sandisk.com>
>
> Thanks
> Uri Yanai

Firstly, the authorship isn't correct and of course the SOB path.
Then, it'd better to add a changelog since you do improve Alex's patches, and it would help us to better understand what was changed. :)

Also you should remove Change-Id when fetching patches from your internal code review system.

Anyway, let's talk about the patches themself, I have a question for your patch 3/3 there.


>
> Uri Yanai (3):
>   mmc: replacing hardcoded value for runtime PM suspend
>   mmc: Adding AUTO_BKOPS_EN bit set for Auto BKOPS support
>   mmc: Checking BKOPS status prior to Suspend
>
>  drivers/mmc/core/block.c |  3 ++-
>  drivers/mmc/core/mmc.c   | 45 ++++++++++++++++++++++++++++++++++++---------
>  include/linux/mmc/card.h |  1 +
>  include/linux/mmc/host.h |  2 ++
>  include/linux/mmc/mmc.h  |  3 +++
>  5 files changed, 44 insertions(+), 10 deletions(-)
>


--
Best Regards
Shawn Lin

RE: [PATCH 3/3] mmc: Checking BKOPS status prior to Suspend

[Uri Yanai]

Hi Shawn

Thanks for the review.
1. As you suggested replaced EXT_CSD_BKOPS_LEVEL_PM_SUSPEND  calling EXT_CSD_BKOPS_LEVEL_2 directly.
2. Returning  -EBUSY from the point I do in the code, stops the path to enter suspend( i.e. calling  mmc_power_off() and mmc_card_set_suspended())
Did I get this wrong ?

Thanks
Uri
-----Original Message-----
From: Shawn Lin [mailto:shawn.lin@rock-chips.com] 
Sent: Wednesday, January 18, 2017 4:00 AM
To: Uri Yanai; ulf.hansson@linaro.org
Cc: shawn.lin@rock-chips.com; linux-mmc@vger.kernel.org; chris@printf.net; Alex Lemberg
Subject: Re: [PATCH 3/3] mmc: Checking BKOPS status prior to Suspend

On 2017/1/17 20:15, Uri Yanai wrote:
> If asked to do device Runtime Suspend while in BKOPS and auto BKOPS is 
> enabled return –EBUSY
>

The commit msg isn't correct as the code just return -EBUSY when the level of BKOPS is EXT_CSD_BKOPS_LEVEL_PM_SUSPEND.

And how do you decide that EXT_CSD_BKOPS_LEVEL_2 should be used.
 From the spec, it means "performance being impacted". It would be good to know a bit details inside the eMMC firmware.

BTW, should we use EXT_CSD_BKOPS_LEVEL_2 directly since it looks more strightforward?


> Change-Id: Ie4b38a32fe98179f0bca5b57edaaa47a02d0a389
> Signed-off-by: Uri Yanai <uri.yanai@sandisk.com>
> Signed-off-by: Alex Lemberg <alex.lemberg@sandisk.com>
> ---
>  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
>   */
>


--
Best Regards
Shawn Lin

Re: [PATCH 3/3] mmc: Checking BKOPS status prior to Suspend

[Adrian Hunter]

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@sandisk.com>
> Signed-off-by: Alex Lemberg <alex.lemberg@sandisk.com>
> ---
>  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
>   */
> 

RE: [PATCH 3/3] mmc: Checking BKOPS status prior to Suspend

[Uri Yanai]

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@intel.com] 
Sent: Friday, January 20, 2017 1:56 PM
To: Uri Yanai; ulf.hansson@linaro.org
Cc: linux-mmc@vger.kernel.org; chris@printf.net; 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@sandisk.com>
> Signed-off-by: Alex Lemberg <alex.lemberg@sandisk.com>
> ---
>  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
>   */
> 

Re: [PATCH 3/3] mmc: Checking BKOPS status prior to Suspend

[Alex Lemberg]

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@vger.kernel.org/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…
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.


Thanks,
Alex

On 1/22/17, 5:55 PM, "Uri Yanai" <Uri.Yanai@sandisk.com> 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@intel.com] 
    Sent: Friday, January 20, 2017 1:56 PM
    To: Uri Yanai; ulf.hansson@linaro.org
    Cc: linux-mmc@vger.kernel.org; chris@printf.net; 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@sandisk.com>
    > Signed-off-by: Alex Lemberg <alex.lemberg@sandisk.com>
    > ---
    >  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
    >   */
    > 
    
    

Re: [PATCH 3/3] mmc: Checking BKOPS status prior to Suspend

[Adrian Hunter]

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@vger.kernel.org/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@sandisk.com> 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@intel.com] 
>     Sent: Friday, January 20, 2017 1:56 PM
>     To: Uri Yanai; ulf.hansson@linaro.org
>     Cc: linux-mmc@vger.kernel.org; chris@printf.net; 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@sandisk.com>
>     > Signed-off-by: Alex Lemberg <alex.lemberg@sandisk.com>
>     > ---
>     >  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
>     >   */
>     > 
>     
>     
> 

Re: [PATCH 3/3] mmc: Checking BKOPS status prior to Suspend

[Alex Lemberg]

On 1/24/17, 4:36 PM, "Adrian Hunter" <adrian.hunter@intel.com> wrote:

    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@vger.kernel.org/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.

Potentially, by the spec, the Max value of SLEEP_NOTIFICATION_TIME can be 83.88 seconds.
Can we assume that this time is acceptable during _suspend?
    
    > 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"?

I mean letting Sleep_Notification to be completed before Sleep.
 

Re: [PATCH 3/3] mmc: Checking BKOPS status prior to Suspend

[Adrian Hunter]

On 25/01/17 11:48, Alex Lemberg wrote:
> 
> 
> On 1/24/17, 4:36 PM, "Adrian Hunter" <adrian.hunter@intel.com> wrote:
> 
>     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@vger.kernel.org/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.
> 
> Potentially, by the spec, the Max value of SLEEP_NOTIFICATION_TIME can be 83.88 seconds.
> Can we assume that this time is acceptable during _suspend?

Do you know of any cards that take that long?

As I wrote, I would assume it is acceptable until we know 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"?
> 
> I mean letting Sleep_Notification to be completed before Sleep.

ook.

Re: [PATCH 3/3] mmc: Checking BKOPS status prior to Suspend

[Alex Lemberg]

On 1/25/17, 12:30 PM, "Adrian Hunter" <adrian.hunter@intel.com> wrote:

    On 25/01/17 11:48, Alex Lemberg wrote:
    > 
    > 
    > On 1/24/17, 4:36 PM, "Adrian Hunter" <adrian.hunter@intel.com> wrote:
    > 
    >     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@vger.kernel.org/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.
    > 
    > Potentially, by the spec, the Max value of SLEEP_NOTIFICATION_TIME can be 83.88 seconds.
    > Can we assume that this time is acceptable during _suspend?
    
    Do you know of any cards that take that long?

Yes - for some cards it can take several seconds in some corner cases.

    As I wrote, I would assume it is acceptable until we know otherwise.

I think Sleep_Notification requires more detailed review. 
Since PON Sleep_Notification is a blocking command (BUSY asserted),
in case of getting new request, driver need to send HPI command in order
to interrupt the Sleep_Notification process.
We tried to handle this case in our Sleep_Notification patchset…
Anyway, I think we need to discuss it separately, not related to
the AUTO_BKOPS support…

    >     
    >     > 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"?
    > 
    > I mean letting Sleep_Notification to be completed before Sleep.
    
    ook.
    
    

Re: [PATCH 3/3] mmc: Checking BKOPS status prior to Suspend

[Adrian Hunter]

On 25/01/17 14:40, Alex Lemberg wrote:
> 
> 
> On 1/25/17, 12:30 PM, "Adrian Hunter" <adrian.hunter@intel.com> wrote:
> 
>     On 25/01/17 11:48, Alex Lemberg wrote:
>     > 
>     > 
>     > On 1/24/17, 4:36 PM, "Adrian Hunter" <adrian.hunter@intel.com> wrote:
>     > 
>     >     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@vger.kernel.org/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.
>     > 
>     > Potentially, by the spec, the Max value of SLEEP_NOTIFICATION_TIME can be 83.88 seconds.
>     > Can we assume that this time is acceptable during _suspend?
>     
>     Do you know of any cards that take that long?
> 
> Yes - for some cards it can take several seconds in some corner cases.

Wouldn't that mean POWER_OFF_SHORT has the same problem since that is
preparing for *both* Vcc and Vccq to go off?

> 
>     As I wrote, I would assume it is acceptable until we know otherwise.
> 
> I think Sleep_Notification requires more detailed review. 
> Since PON Sleep_Notification is a blocking command (BUSY asserted),
> in case of getting new request, driver need to send HPI command in order
> to interrupt the Sleep_Notification process.
> We tried to handle this case in our Sleep_Notification patchset…
> Anyway, I think we need to discuss it separately, not related to
> the AUTO_BKOPS support…
> 
>     >     
>     >     > 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"?
>     > 
>     > I mean letting Sleep_Notification to be completed before Sleep.
>     
>     ook.
>     
>     
> 

Re: [PATCH 3/3] mmc: Checking BKOPS status prior to Suspend

[Alex Lemberg]

On 1/25/17, 2:45 PM, "linux-mmc-owner@vger.kernel.org on behalf of Adrian Hunter" <linux-mmc-owner@vger.kernel.org on behalf of adrian.hunter@intel.com> wrote:

 
    >     > Potentially, by the spec, the Max value of SLEEP_NOTIFICATION_TIME can be 83.88 seconds.
    >     > Can we assume that this time is acceptable during _suspend?
    >     
    >     Do you know of any cards that take that long?
    > 
    > Yes - for some cards it can take several seconds in some corner cases.
    
    Wouldn't that mean POWER_OFF_SHORT has the same problem since that is
    preparing for *both* Vcc and Vccq to go off?

POWER_OFF_SHORT should be completed within GENERIC_CMD6_TIME,
which is a standard Switch command time.
 
    > 
    >     As I wrote, I would assume it is acceptable until we know otherwise.
    > 
    > I think Sleep_Notification requires more detailed review. 
    > Since PON Sleep_Notification is a blocking command (BUSY asserted),
    > in case of getting new request, driver need to send HPI command in order
    > to interrupt the Sleep_Notification process.
    > We tried to handle this case in our Sleep_Notification patchset…
    > Anyway, I think we need to discuss it separately, not related to
    > the AUTO_BKOPS support…
    > 
    >     >     
    >     >     > 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"?
    >     > 
    >     > I mean letting Sleep_Notification to be completed before Sleep.
    >     
    >     ook.
    >     
    >     
    > 
    
    --
    To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at  http://vger.kernel.org/majordomo-info.html