[PATCH v2 0/2] Auto BKOPS PM suspend

[PATCH v2 0/2] Auto BKOPS PM suspend

[Uri Yanai]

Resubmitting three patches for approval, originally submitted by Alex Lemberg <alex.lemberg@sandisk.com>
[1/3] mmc: replacing hardcoded value for runtime PM suspend(removed)
[2/3] mmc: Adding AUTO_BKOPS_EN bit set for Auto BKOPS support(changed)
[3/3] mmc: Checking BKOPS status prior to Suspend(changed)

Uri Yanai (2):
  mmc: Adding AUTO_BKOPS_EN bit set for Auto BKOPS support
  mmc: Checking BKOPS status prior to Suspend

 drivers/mmc/core/mmc.c   | 37 +++++++++++++++++++++++++++++++------
 include/linux/mmc/card.h |  1 +
 include/linux/mmc/mmc.h  |  1 +
 3 files changed, 33 insertions(+), 6 deletions(-)

-- 
1.9.1

[PATCH v2 1/2] 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

Changes in v2:
- inverse the logic for printing the debug message.
  only print when bits are set.

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

Re: [PATCH v2 1/2] mmc: Adding AUTO_BKOPS_EN bit set for Auto BKOPS support

[Ulf Hansson]

On 22 January 2017 at 10:15, Uri Yanai <uri.yanai@sandisk.com> wrote:
> Read from the device EXT_CSD and set to the card->ext_csd structure
>

Please improve the change-log. Ask yourself the questions, what, how
and why when you update it.

> Changes in v2:
> - inverse the logic for printing the debug message.
>   only print when bits are set.

It's good that you provide the a revision history, although this shall
not be apart of the change log.

Instead use "patch notes". That is, add a newline after "---" below,
type your text, add a new line and add "---".

>
> 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));

This change seem reasonable to me.

However the change-log refers only to AUTO_BKOPS. So, I think it's
better if you move this change into a separate patch, preceding
@subject patch.

> +                       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
>

Kind regards
Uffe

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

[Uri Yanai]

In case of Runtime Suspend, check the device BKOPS level.
Return –EBUSY if device need more time to complete its internal BKOPS.

Changes in v2:
- return –EBUSY instead of delaying suspend
- remove define EXT_CSD_BKOPS_LEVEL_1

Signed-off-by: Uri Yanai <uri.yanai@sandisk.com>
Signed-off-by: Alex Lemberg <alex.lemberg@sandisk.com>
---
 drivers/mmc/core/mmc.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index f70f6a1..211b726 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 :
@@ -1954,6 +1955,24 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
 		goto out;
 
 	if (mmc_card_doing_bkops(host->card)) {
+		if (is_runtime_pm && 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 (host->card->ext_csd.raw_bkops_status >=
+					EXT_CSD_BKOPS_LEVEL_2) {
+				/*
+				 * We don’t allow Runtime Suspend while device
+				 * still needs time to complete internal BKOPS
+				 */
+				err = -EBUSY;
+				goto out;
+			}
+		}
 		err = mmc_stop_bkops(host->card);
 		if (err)
 			goto out;
@@ -1987,7 +2006,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 +2053,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 +2077,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);
-- 
1.9.1

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

[Ulf Hansson]

+ Adrian, Shawn-Lin, Jaehoon, Ritesh

On 22 January 2017 at 10:15, Uri Yanai <uri.yanai@sandisk.com> wrote:
> In case of Runtime Suspend, check the device BKOPS level.
> Return –EBUSY if device need more time to complete its internal BKOPS.
>
> Changes in v2:
> - return –EBUSY instead of delaying suspend
> - remove define EXT_CSD_BKOPS_LEVEL_1
>
> Signed-off-by: Uri Yanai <uri.yanai@sandisk.com>
> Signed-off-by: Alex Lemberg <alex.lemberg@sandisk.com>
> ---
>  drivers/mmc/core/mmc.c | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index f70f6a1..211b726 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 :
> @@ -1954,6 +1955,24 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>                 goto out;
>
>         if (mmc_card_doing_bkops(host->card)) {
> +               if (is_runtime_pm && host->card->ext_csd.auto_bkops_en) {

This don't work. Primarily because mmc_card_doing_bkops(host->card)
don't ever return true, unless manual BKOPS is enabled.

I think you need to start with checking if *auto* BKOPS is enabled,
then you can continue to read the BKOPS status to find out whether you
should return -EBUSY.

> +                       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 (host->card->ext_csd.raw_bkops_status >=
> +                                       EXT_CSD_BKOPS_LEVEL_2) {
> +                               /*
> +                                * We don’t allow Runtime Suspend while device
> +                                * still needs time to complete internal BKOPS
> +                                */

Hmm.

Shouldn't we abort (return -EBUSY) also in the system PM suspend case
and not only for runtime PM suspend?

I think we need to discuss this with some other mmc folkz as well,
looping in Adrian, Shawn, Jaehoon and Ritesh.

Of course, unless we are being clever, in could mean that the system
will never enter system PM suspend state, due to a bad behaving eMMC
always reporting EXT_CSD_BKOPS_LEVEL_2. So we need to be careful here.

Now, assume we may want to abort in the system PM suspend case as
well, then we consider to avoid the "Opportunistic sleep" from
hammering us with new system PM suspend attempts. That can be avoided
by activating a wakeup source (aka wakelock), via calling
pm_wakeup_event(), using the struct device for the mmc_card as the
parameter.

> +                               err = -EBUSY;
> +                               goto out;
> +                       }
> +               }
>                 err = mmc_stop_bkops(host->card);
>                 if (err)
>                         goto out;
> @@ -1987,7 +2006,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 +2053,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 +2077,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);
> --
> 1.9.1
>

Kind regards
Uffe

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

[Ritesh Harjani]

Hi,

On 2/2/2017 6:19 PM, Ulf Hansson wrote:
> + Adrian, Shawn-Lin, Jaehoon, Ritesh
>
> On 22 January 2017 at 10:15, Uri Yanai <uri.yanai@sandisk.com> wrote:
>> In case of Runtime Suspend, check the device BKOPS level.
>> Return –EBUSY if device need more time to complete its internal BKOPS.
Do we need to abort the runtime suspend at all even though we are using 
CMD5?

 From what I understood from the spec is as follows - (please correct me 
if I am wrong) -
CMD7 -> CMD5 -> Busy line de-asserted by device(means autobkops 
execution is complete) -> Enter sleep mode.

Is the above understanding correct? In that case we may not need to 
abort the runtime suspend right?
Since CMD5 completion should ensure that background operation execution 
is complete (until then the busy line will be kept asserted).

Opinion?


>>
>> Changes in v2:
>> - return –EBUSY instead of delaying suspend
>> - remove define EXT_CSD_BKOPS_LEVEL_1
>>
>> Signed-off-by: Uri Yanai <uri.yanai@sandisk.com>
>> Signed-off-by: Alex Lemberg <alex.lemberg@sandisk.com>
>> ---
>>  drivers/mmc/core/mmc.c | 27 +++++++++++++++++++++++----
>>  1 file changed, 23 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index f70f6a1..211b726 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 :
>> @@ -1954,6 +1955,24 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>>                 goto out;
>>
>>         if (mmc_card_doing_bkops(host->card)) {
>> +               if (is_runtime_pm && host->card->ext_csd.auto_bkops_en) {
>
> This don't work. Primarily because mmc_card_doing_bkops(host->card)
> don't ever return true, unless manual BKOPS is enabled.
>
> I think you need to start with checking if *auto* BKOPS is enabled,
> then you can continue to read the BKOPS status to find out whether you
> should return -EBUSY.
>
>> +                       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 (host->card->ext_csd.raw_bkops_status >=
>> +                                       EXT_CSD_BKOPS_LEVEL_2) {
>> +                               /*
>> +                                * We don’t allow Runtime Suspend while device
>> +                                * still needs time to complete internal BKOPS
>> +                                */
>
> Hmm.
>
> Shouldn't we abort (return -EBUSY) also in the system PM suspend case
> and not only for runtime PM suspend?

My opinion - Auto bkops generally is meant for device to autonomously 
initiate the background transfer whenever the device is idle -> in such 
cases, we expect the device to keep it's autobkops exception level under 
control.
In that case will it be good idea to abort the suspend as well?

Off-course unless we would like to cover a case where device is not idle 
at all and during this heavy device usage suspend is getting triggered 
manually - which gives no time for device to do auto-bkops
Please correct me if my understanding is wrong?


>
> I think we need to discuss this with some other mmc folkz as well,
> looping in Adrian, Shawn, Jaehoon and Ritesh.
Thanks :)

>
> Of course, unless we are being clever, in could mean that the system
> will never enter system PM suspend state, due to a bad behaving eMMC
> always reporting EXT_CSD_BKOPS_LEVEL_2. So we need to be careful here.
>
> Now, assume we may want to abort in the system PM suspend case as
> well, then we consider to avoid the "Opportunistic sleep" from
> hammering us with new system PM suspend attempts. That can be avoided
> by activating a wakeup source (aka wakelock), via calling
> pm_wakeup_event(), using the struct device for the mmc_card as the
> parameter.
>
>> +                               err = -EBUSY;
>> +                               goto out;
>> +                       }
>> +               }
>>                 err = mmc_stop_bkops(host->card);
>>                 if (err)
>>                         goto out;
>> @@ -1987,7 +2006,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 +2053,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 +2077,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);
>> --
>> 1.9.1
>>
>
> Kind regards
> Uffe
> --
> 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
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Ulf Hansson]

On 3 February 2017 at 10:33, Ritesh Harjani <riteshh@codeaurora.org> wrote:
> Hi,
>
> On 2/2/2017 6:19 PM, Ulf Hansson wrote:
>>
>> + Adrian, Shawn-Lin, Jaehoon, Ritesh
>>
>> On 22 January 2017 at 10:15, Uri Yanai <uri.yanai@sandisk.com> wrote:
>>>
>>> In case of Runtime Suspend, check the device BKOPS level.
>>> Return –EBUSY if device need more time to complete its internal BKOPS.
>
> Do we need to abort the runtime suspend at all even though we are using
> CMD5?
>
> From what I understood from the spec is as follows - (please correct me if I
> am wrong) -
> CMD7 -> CMD5 -> Busy line de-asserted by device(means autobkops execution is
> complete) -> Enter sleep mode.
>
> Is the above understanding correct? In that case we may not need to abort
> the runtime suspend right?
> Since CMD5 completion should ensure that background operation execution is
> complete (until then the busy line will be kept asserted).
>
> Opinion?

Unfortunate the eMMC spec isn't crystal clear on this point. Anyway,
my interpretation of it is not as yours. Let me elaborate on my view.

In case we have enabled POWERED_ON bit (0x1) in POWER_OFF_NOTIFICATION
register byte, during the card initialization, and we later want to
put the device into sleep state by using CMD5. Then we need to follow
the below sequence:
1) Using CMD6, write SLEEP_NOTIFICATION bits (0x4) to
POWER_OFF_NOTIFICATION register byte.
2) Wait for busy! Maximum time specified in
SLEEP_NOTIFICATION_TIME[216] (worst case 83.88s).
3) Put the card to sleep by send CMD5 and wait for busy (current
implemented method of issuing sleep).

As we have enabled POWERED_ON for those cards that supports
POWER_OFF_NOTIFICATION (added in eMMC 4.5 spec), then we also need to
follow the above sequence for when sending sleep (CMD5).

This may become a severe problem, because the SLEEP_NOTIFICATION_TIME
may be ridiculously long and since it immediately affects the total
system suspend time for the platform.

We need to think of something clever here to avoid a long sleep
notification timeout from happen.

[...]

>>
>> Hmm.
>>
>> Shouldn't we abort (return -EBUSY) also in the system PM suspend case
>> and not only for runtime PM suspend?
>
>
> My opinion - Auto bkops generally is meant for device to autonomously
> initiate the background transfer whenever the device is idle -> in such
> cases, we expect the device to keep it's autobkops exception level under
> control.
> In that case will it be good idea to abort the suspend as well?

I follow your reasoning and it seems reasonable. However, see my comment below.

>
> Off-course unless we would like to cover a case where device is not idle at
> all and during this heavy device usage suspend is getting triggered manually
> - which gives no time for device to do auto-bkops
> Please correct me if my understanding is wrong?

This is the case that I thought off. Don't you think this could be a
valid scenario for an Android device?

[...]

It seems like we are discussing several related things at the same
time. Perhaps this is the only way, as they are so closely related.

Kind regards
Uffe

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

[Ritesh Harjani]

On 2/3/2017 5:16 PM, Ulf Hansson wrote:
> On 3 February 2017 at 10:33, Ritesh Harjani <riteshh@codeaurora.org> wrote:
>> Hi,
>>
>> On 2/2/2017 6:19 PM, Ulf Hansson wrote:
>>>
>>> + Adrian, Shawn-Lin, Jaehoon, Ritesh
>>>
>>> On 22 January 2017 at 10:15, Uri Yanai <uri.yanai@sandisk.com> wrote:
>>>>
>>>> In case of Runtime Suspend, check the device BKOPS level.
>>>> Return –EBUSY if device need more time to complete its internal BKOPS.
>>
>> Do we need to abort the runtime suspend at all even though we are using
>> CMD5?
>>
>> From what I understood from the spec is as follows - (please correct me if I
>> am wrong) -
>> CMD7 -> CMD5 -> Busy line de-asserted by device(means autobkops execution is
>> complete) -> Enter sleep mode.
>>
>> Is the above understanding correct? In that case we may not need to abort
>> the runtime suspend right?
>> Since CMD5 completion should ensure that background operation execution is
>> complete (until then the busy line will be kept asserted).
>>
>> Opinion?
>
> Unfortunate the eMMC spec isn't crystal clear on this point. Anyway,
> my interpretation of it is not as yours. Let me elaborate on my view.
>
> In case we have enabled POWERED_ON bit (0x1) in POWER_OFF_NOTIFICATION
> register byte, during the card initialization, and we later want to
> put the device into sleep state by using CMD5. Then we need to follow
> the below sequence:
> 1) Using CMD6, write SLEEP_NOTIFICATION bits (0x4) to
> POWER_OFF_NOTIFICATION register byte.
> 2) Wait for busy! Maximum time specified in
> SLEEP_NOTIFICATION_TIME[216] (worst case 83.88s).
> 3) Put the card to sleep by send CMD5 and wait for busy (current
> implemented method of issuing sleep).
>
> As we have enabled POWERED_ON for those cards that supports
> POWER_OFF_NOTIFICATION (added in eMMC 4.5 spec), then we also need to
> follow the above sequence for when sending sleep (CMD5).
Thanks for clarifying. I initially misunderstood and missed 
SLEEP_NOTIFICATION part.
Yes, you are right, we do need to follow the above sequence you mentioned.

>
> This may become a severe problem, because the SLEEP_NOTIFICATION_TIME
> may be ridiculously long and since it immediately affects the total
> system suspend time for the platform.
Do we know more card internal details on why this time could be long?
Although POWER_OFF_SHORT time is within generic CMD6 response time.


So enabling auto bkops *may* help reduce the sleep notification time to 
some extent I guess.
Since card may already would have completed it's background operations 
while the card was idle, then sleep notification time may reduce, right?
Although, someone who have more knowledge of card internals would know 
better here.

And in case where the bkops exception level is high, patch is anyway 
aborting suspend by returning -EBUSY, giving enough time for card to 
first complete it's background operations.

>
> We need to think of something clever here to avoid a long sleep
> notification timeout from happen.
>
> [...]
>
>>>
>>> Hmm.
>>>
>>> Shouldn't we abort (return -EBUSY) also in the system PM suspend case
>>> and not only for runtime PM suspend?
>>
>>
>> My opinion - Auto bkops generally is meant for device to autonomously
>> initiate the background transfer whenever the device is idle -> in such
>> cases, we expect the device to keep it's autobkops exception level under
>> control.
>> In that case will it be good idea to abort the suspend as well?
>
> I follow your reasoning and it seems reasonable. However, see my comment below.
>
>>
>> Off-course unless we would like to cover a case where device is not idle at
>> all and during this heavy device usage suspend is getting triggered manually
>> - which gives no time for device to do auto-bkops
>> Please correct me if my understanding is wrong?
>
> This is the case that I thought off. Don't you think this could be a
> valid scenario for an Android device?
Yes, totally agree. This can be a valid use case on Android.
There can be watchdog timer monitoring per device suspend time. So, if 
we don't take care of mmc suspend case, it may result into watchdog 
bark, since mmc_suspend may take longer.

So as you mentioned, even suspend case needs to be taken care here.


>
> [...]
>
> It seems like we are discussing several related things at the same
> time. Perhaps this is the only way, as they are so closely related.
>
> Kind regards
> Uffe
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Ulf Hansson]

[...]

>>
>> This may become a severe problem, because the SLEEP_NOTIFICATION_TIME
>> may be ridiculously long and since it immediately affects the total
>> system suspend time for the platform.
>
> Do we know more card internal details on why this time could be long?

I think this will be hard to get an answer to, because it do varies
depending on the eMMC vendor.

> Although POWER_OFF_SHORT time is within generic CMD6 response time.

Assuming you mean the case when sending a power off notification using
POWER_OFF_SHORT, instead of sending sleep (CMD5) when suspending? This
is the case for those mmc hosts using MMC_CAP2_FULL_PWR_CYCLE (DT
binding "full-pwr-cycle", MMC_CAP2_FULL_PWR_CYCLE == host can power
off both VCC and VCCQ). So these have no issues.

However, the majority of platforms/hosts don't use
MMC_CAP2_FULL_PWR_CYCLE, which means using sleep (CMD5) at suspend.
For these cases and when the eMMC cards is a v5.0+ (eMMC v5.0 added
sleep notification), the mmc core are currently violating the eMMC
spec - because we don't write the SLEEP_NOTIFICATION bits.

>
>
> So enabling auto bkops *may* help reduce the sleep notification time to some
> extent I guess.
> Since card may already would have completed it's background operations while
> the card was idle, then sleep notification time may reduce, right?

I agree, this seems like a reasonable conclusion we should make.
Unless eMMC vendors tells us different.

> Although, someone who have more knowledge of card internals would know
> better here.
>
> And in case where the bkops exception level is high, patch is anyway
> aborting suspend by returning -EBUSY, giving enough time for card to first
> complete it's background operations.
>
>>
>> We need to think of something clever here to avoid a long sleep
>> notification timeout from happen.
>>
>> [...]
>>
>>>>
>>>> Hmm.
>>>>
>>>> Shouldn't we abort (return -EBUSY) also in the system PM suspend case
>>>> and not only for runtime PM suspend?
>>>
>>>
>>>
>>> My opinion - Auto bkops generally is meant for device to autonomously
>>> initiate the background transfer whenever the device is idle -> in such
>>> cases, we expect the device to keep it's autobkops exception level under
>>> control.
>>> In that case will it be good idea to abort the suspend as well?
>>
>>
>> I follow your reasoning and it seems reasonable. However, see my comment
>> below.
>>
>>>
>>> Off-course unless we would like to cover a case where device is not idle
>>> at
>>> all and during this heavy device usage suspend is getting triggered
>>> manually
>>> - which gives no time for device to do auto-bkops
>>> Please correct me if my understanding is wrong?
>>
>>
>> This is the case that I thought off. Don't you think this could be a
>> valid scenario for an Android device?
>
> Yes, totally agree. This can be a valid use case on Android.
> There can be watchdog timer monitoring per device suspend time. So, if we
> don't take care of mmc suspend case, it may result into watchdog bark, since
> mmc_suspend may take longer.

Yes, exactly.

So, hopefully we can improve the situation for when we need to write
the SLEEP_NOFITICATION bits, by first checking BKOPS status and
returning -EBUSY.

>
> So as you mentioned, even suspend case needs to be taken care here.

Great, thanks for your input!

[...]

Kind regards
Uffe

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

[Shawn Lin]

On 2017/2/2 20:49, Ulf Hansson wrote:
> + Adrian, Shawn-Lin, Jaehoon, Ritesh
>
> On 22 January 2017 at 10:15, Uri Yanai <uri.yanai@sandisk.com> wrote:
>> In case of Runtime Suspend, check the device BKOPS level.
>> Return –EBUSY if device need more time to complete its internal BKOPS.
>>
>> Changes in v2:
>> - return –EBUSY instead of delaying suspend
>> - remove define EXT_CSD_BKOPS_LEVEL_1
>>
>> Signed-off-by: Uri Yanai <uri.yanai@sandisk.com>
>> Signed-off-by: Alex Lemberg <alex.lemberg@sandisk.com>
>> ---
>>  drivers/mmc/core/mmc.c | 27 +++++++++++++++++++++++----
>>  1 file changed, 23 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index f70f6a1..211b726 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 :
>> @@ -1954,6 +1955,24 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>>                 goto out;
>>
>>         if (mmc_card_doing_bkops(host->card)) {
>> +               if (is_runtime_pm && host->card->ext_csd.auto_bkops_en) {
>
> This don't work. Primarily because mmc_card_doing_bkops(host->card)
> don't ever return true, unless manual BKOPS is enabled.
>
> I think you need to start with checking if *auto* BKOPS is enabled,
> then you can continue to read the BKOPS status to find out whether you
> should return -EBUSY.
>
>> +                       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 (host->card->ext_csd.raw_bkops_status >=
>> +                                       EXT_CSD_BKOPS_LEVEL_2) {
>> +                               /*
>> +                                * We don’t allow Runtime Suspend while device
>> +                                * still needs time to complete internal BKOPS
>> +                                */
>
> Hmm.
>
> Shouldn't we abort (return -EBUSY) also in the system PM suspend case
> and not only for runtime PM suspend?

I would rather we don't abort runtime PM, but do it for system PM
instead. What we do for runtime PM is just for saving power for
our hosts including manipulating clock and genpd etc.. Thus we don't
touch any power-supply for eMMC. If so, it could still be possible
for eMMC firmware to work on doing bkops in rpm context.

>
> I think we need to discuss this with some other mmc folkz as well,
> looping in Adrian, Shawn, Jaehoon and Ritesh.
>
> Of course, unless we are being clever, in could mean that the system
> will never enter system PM suspend state, due to a bad behaving eMMC
> always reporting EXT_CSD_BKOPS_LEVEL_2. So we need to be careful here.
>

We don't have enough data to support this hypothesis but if it does
happen, it seems we should allow more time for the bad behaving eMMC
to do bkops.


> Now, assume we may want to abort in the system PM suspend case as
> well, then we consider to avoid the "Opportunistic sleep" from
> hammering us with new system PM suspend attempts. That can be avoided
> by activating a wakeup source (aka wakelock), via calling
> pm_wakeup_event(), using the struct device for the mmc_card as the
> parameter.

I was trying to figure out could we reuse "keep-power-in-suspend" policy
for eMMC as well.

If the power-supply(aka vmmc/vqmmc) couldn't be controlled by mmc
core for reasons:
a) it's fixed and couldn't be turn off actually, then we shouldn't
abort the system PM as we treat it as rpm case.
b) dt folkz forget to add them for mmc node, so the power-supply won't
be off during system PM that we don't need to abort the system PM as
well.

If the power-supply could be controlled by mmc core, things would be a
little complex from my mind. Two possible ideas then:
c) we avoid to enter the system PM by whatever methods, for instace,
by calling pm_wakeup_event or simply returning -EBUSY. OR
d) keep the power-supply and register a wakeup timer. So the system PM
could still go on and later the system wakeup to check should we remove
the power-supply then as we know there is no more access for eMMC
during the period of suspend and we expect it's also enough for eMMC
firmware to finish critical bkops.

Always we need some tradeoff to balance the perf & power :)

>
>> +                               err = -EBUSY;
>> +                               goto out;
>> +                       }
>> +               }
>>                 err = mmc_stop_bkops(host->card);
>>                 if (err)
>>                         goto out;
>> @@ -1987,7 +2006,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 +2053,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 +2077,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);
>> --
>> 1.9.1
>>
>
> Kind regards
> Uffe
>
>
>


-- 
Best Regards
Shawn Lin

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

[Alex Lemberg]

Hi Shawn,

On 2/5/17, 3:43 AM, "Shawn Lin" <shawn.lin@rock-chips.com> wrote:


    >
    > Hmm.
    >
    > Shouldn't we abort (return -EBUSY) also in the system PM suspend case
    > and not only for runtime PM suspend?
    
    I would rather we don't abort runtime PM, but do it for system PM
    instead. What we do for runtime PM is just for saving power for
    our hosts including manipulating clock and genpd etc.. Thus we don't
    touch any power-supply for eMMC. If so, it could still be possible
    for eMMC firmware to work on doing bkops in rpm context.

If I understand correctly, you are suggesting do not send SLEEP command
on Runtime Suspend?
If so, and if mmc host does not touch any power supply for eMMC, 
the device can continue doing Auto BKOPS during Runtime Suspend.
The question is why the Sleep command is sent now,
in the original implementation?
I assume it is sent for saving a power from the device side as well? 

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

[Shawn Lin]

Hi Alex,

On 2017/2/5 23:13, Alex Lemberg wrote:
> Hi Shawn,
>
> On 2/5/17, 3:43 AM, "Shawn Lin" <shawn.lin@rock-chips.com> wrote:
>
>
>     >
>     > Hmm.
>     >
>     > Shouldn't we abort (return -EBUSY) also in the system PM suspend case
>     > and not only for runtime PM suspend?
>
>     I would rather we don't abort runtime PM, but do it for system PM
>     instead. What we do for runtime PM is just for saving power for
>     our hosts including manipulating clock and genpd etc.. Thus we don't
>     touch any power-supply for eMMC. If so, it could still be possible
>     for eMMC firmware to work on doing bkops in rpm context.
>
> If I understand correctly, you are suggesting do not send SLEEP command
> on Runtime Suspend?

yes.

> If so, and if mmc host does not touch any power supply for eMMC,
> the device can continue doing Auto BKOPS during Runtime Suspend.
> The question is why the Sleep command is sent now,
> in the original implementation?
> I assume it is sent for saving a power from the device side as well?

right, it was invented with the cap of MMC_CAP_AGGRESSIVE_PM, so some 
platforms ask for the aggressive power policy will send sleep command
to the cards in rpm context. So I assume they care power-saving more
than others.


>
> N�����r��y���b�X��ǧv�^�)޺{.n�+����{��g"��^n�r���z�\x1a��h����&��\x1e�G���h�\x03(�階�ݢj"��\x1a�^[m�����z�ޖ���f���h���~�mml==
>

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

[Alex Lemberg]

> On Feb 6, 2017, at 8:46 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> 
> Hi Alex,
> 
> On 2017/2/5 23:13, Alex Lemberg wrote:
>> Hi Shawn,
>> 
>> On 2/5/17, 3:43 AM, "Shawn Lin" <shawn.lin@rock-chips.com> wrote:
>> 
>> 
>>    >
>>    > Hmm.
>>    >
>>    > Shouldn't we abort (return -EBUSY) also in the system PM suspend case
>>    > and not only for runtime PM suspend?
>> 
>>    I would rather we don't abort runtime PM, but do it for system PM
>>    instead. What we do for runtime PM is just for saving power for
>>    our hosts including manipulating clock and genpd etc.. Thus we don't
>>    touch any power-supply for eMMC. If so, it could still be possible
>>    for eMMC firmware to work on doing bkops in rpm context.
>> 
>> If I understand correctly, you are suggesting do not send SLEEP command
>> on Runtime Suspend?
> 
> yes.
> 
>> If so, and if mmc host does not touch any power supply for eMMC,
>> the device can continue doing Auto BKOPS during Runtime Suspend.
>> The question is why the Sleep command is sent now,
>> in the original implementation?
>> I assume it is sent for saving a power from the device side as well?
> 
> right, it was invented with the cap of MMC_CAP_AGGRESSIVE_PM, so some platforms ask for the aggressive power policy will send sleep command
> to the cards in rpm context. So I assume they care power-saving more
> than others.
> 

OK, I see the MMC_CAP_AGGRESSIVE_PM is already checked in the
mmc_runtime_suspend() function, so your approach, do not send
Sleep command on Runtime Suspend, is already supported.
In case the MMC_CAP_AGGRESSIVE_PM is not set, Sleep command
will not be sent, and the device will be able to perform its
AUTO_BKOPS (if needed).

However, as far as I see in modern mobile hosts,
the Sleep is sent on every Suspend, so I assume the
MMC_CAP_AGGRESSIVE_PM is enabled in most of the cases?

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

[Shawn Lin]

Hi Alex,

On 2017/2/6 20:20, Alex Lemberg wrote:
>
>> On Feb 6, 2017, at 8:46 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>
>> Hi Alex,
>>
>> On 2017/2/5 23:13, Alex Lemberg wrote:
>>> Hi Shawn,
>>>
>>> On 2/5/17, 3:43 AM, "Shawn Lin" <shawn.lin@rock-chips.com> wrote:
>>>
>>>
>>>    >
>>>    > Hmm.
>>>    >
>>>    > Shouldn't we abort (return -EBUSY) also in the system PM suspend case
>>>    > and not only for runtime PM suspend?
>>>
>>>    I would rather we don't abort runtime PM, but do it for system PM
>>>    instead. What we do for runtime PM is just for saving power for
>>>    our hosts including manipulating clock and genpd etc.. Thus we don't
>>>    touch any power-supply for eMMC. If so, it could still be possible
>>>    for eMMC firmware to work on doing bkops in rpm context.
>>>
>>> If I understand correctly, you are suggesting do not send SLEEP command
>>> on Runtime Suspend?
>>
>> yes.
>>
>>> If so, and if mmc host does not touch any power supply for eMMC,
>>> the device can continue doing Auto BKOPS during Runtime Suspend.
>>> The question is why the Sleep command is sent now,
>>> in the original implementation?
>>> I assume it is sent for saving a power from the device side as well?
>>
>> right, it was invented with the cap of MMC_CAP_AGGRESSIVE_PM, so some platforms ask for the aggressive power policy will send sleep command
>> to the cards in rpm context. So I assume they care power-saving more
>> than others.
>>
>
> OK, I see the MMC_CAP_AGGRESSIVE_PM is already checked in the
> mmc_runtime_suspend() function, so your approach, do not send
> Sleep command on Runtime Suspend, is already supported.
> In case the MMC_CAP_AGGRESSIVE_PM is not set, Sleep command
> will not be sent, and the device will be able to perform its
> AUTO_BKOPS (if needed).
>
> However, as far as I see in modern mobile hosts,
> the Sleep is sent on every Suspend, so I assume the
> MMC_CAP_AGGRESSIVE_PM is enabled in most of the cases?

Only Intel platforms add this caps on upstreamed kernel tree and
no, AOSP kernel tree didn't change that much either. But I guess
some vendors want aggressive pm policy for their production?

IMHO, the basic concept of mmc stack is that we could provide
the solutions to support different perf & pm policy for folks but
it's their call to decide it by adding these caps there.

>
> --
> 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
>

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

[Ulf Hansson]

[...]

>>
>> OK, I see the MMC_CAP_AGGRESSIVE_PM is already checked in the
>> mmc_runtime_suspend() function, so your approach, do not send
>> Sleep command on Runtime Suspend, is already supported.
>> In case the MMC_CAP_AGGRESSIVE_PM is not set, Sleep command
>> will not be sent, and the device will be able to perform its
>> AUTO_BKOPS (if needed).
>>
>> However, as far as I see in modern mobile hosts,
>> the Sleep is sent on every Suspend, so I assume the
>> MMC_CAP_AGGRESSIVE_PM is enabled in most of the cases?
>
>
> Only Intel platforms add this caps on upstreamed kernel tree and
> no, AOSP kernel tree didn't change that much either. But I guess
> some vendors want aggressive pm policy for their production?
>
> IMHO, the basic concept of mmc stack is that we could provide
> the solutions to support different perf & pm policy for folks but
> it's their call to decide it by adding these caps there.
>

I assume you know the consequences of using MMC_CAP_AGGRESSIVE_PM,
still allow me to elaborate a bit on my view.

So, MMC_CAP_AGGRESSIVE_PM tells runtime PM suspend of the card to put
it into sleep/power off, meaning saving power! However, it also means
preventing the eMMC from performing any background operations.

The similar applies to the system PM suspend case, no matter if you
are using MMC_CAP_AGGRESSIVE_PM or not.

To me it seems like we somehow need to start to consider the eMMC'ss
BKOPS status in these paths. Depending on the BKOPS state, we may
abort the sleep/power off things via returning -EBUSY etc, as
otherwise we could end up with poor performing eMMCs, couldn't we!?

Kind regards
Uffe

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

[Shawn Lin]

On 2017/2/7 15:54, Ulf Hansson wrote:
> [...]
>
>>>
>>> OK, I see the MMC_CAP_AGGRESSIVE_PM is already checked in the
>>> mmc_runtime_suspend() function, so your approach, do not send
>>> Sleep command on Runtime Suspend, is already supported.
>>> In case the MMC_CAP_AGGRESSIVE_PM is not set, Sleep command
>>> will not be sent, and the device will be able to perform its
>>> AUTO_BKOPS (if needed).
>>>
>>> However, as far as I see in modern mobile hosts,
>>> the Sleep is sent on every Suspend, so I assume the
>>> MMC_CAP_AGGRESSIVE_PM is enabled in most of the cases?
>>
>>
>> Only Intel platforms add this caps on upstreamed kernel tree and
>> no, AOSP kernel tree didn't change that much either. But I guess
>> some vendors want aggressive pm policy for their production?
>>
>> IMHO, the basic concept of mmc stack is that we could provide
>> the solutions to support different perf & pm policy for folks but
>> it's their call to decide it by adding these caps there.
>>
>
> I assume you know the consequences of using MMC_CAP_AGGRESSIVE_PM,
> still allow me to elaborate a bit on my view.
>
> So, MMC_CAP_AGGRESSIVE_PM tells runtime PM suspend of the card to put
> it into sleep/power off, meaning saving power! However, it also means
> preventing the eMMC from performing any background operations.
>
> The similar applies to the system PM suspend case, no matter if you
> are using MMC_CAP_AGGRESSIVE_PM or not.
>
> To me it seems like we somehow need to start to consider the eMMC'ss
> BKOPS status in these paths. Depending on the BKOPS state, we may
> abort the sleep/power off things via returning -EBUSY etc, as
> otherwise we could end up with poor performing eMMCs, couldn't we!?

Nope, we could. But I just wonder how *aggressive* folks wanted.. Do
they would rather sacrificing the performace to save more power when
adding this caps before?

To me, things like this didn't always exist the "best choice", but we
could find a "better choice". :)


>
> Kind regards
> Uffe
> --
> 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
>

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

[Alex Lemberg]

> On Feb 7, 2017, at 10:24 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> 
> 
> 
> On 2017/2/7 15:54, Ulf Hansson wrote:
>> [...]
>> 
>>>> 
>>>> OK, I see the MMC_CAP_AGGRESSIVE_PM is already checked in the
>>>> mmc_runtime_suspend() function, so your approach, do not send
>>>> Sleep command on Runtime Suspend, is already supported.
>>>> In case the MMC_CAP_AGGRESSIVE_PM is not set, Sleep command
>>>> will not be sent, and the device will be able to perform its
>>>> AUTO_BKOPS (if needed).
>>>> 
>>>> However, as far as I see in modern mobile hosts,
>>>> the Sleep is sent on every Suspend, so I assume the
>>>> MMC_CAP_AGGRESSIVE_PM is enabled in most of the cases?
>>> 
>>> 
>>> Only Intel platforms add this caps on upstreamed kernel tree and
>>> no, AOSP kernel tree didn't change that much either. But I guess
>>> some vendors want aggressive pm policy for their production?
>>> 
>>> IMHO, the basic concept of mmc stack is that we could provide
>>> the solutions to support different perf & pm policy for folks but
>>> it's their call to decide it by adding these caps there.
>>> 
>> 
>> I assume you know the consequences of using MMC_CAP_AGGRESSIVE_PM,
>> still allow me to elaborate a bit on my view.
>> 
>> So, MMC_CAP_AGGRESSIVE_PM tells runtime PM suspend of the card to put
>> it into sleep/power off, meaning saving power! However, it also means
>> preventing the eMMC from performing any background operations.
>> 
>> The similar applies to the system PM suspend case, no matter if you
>> are using MMC_CAP_AGGRESSIVE_PM or not.
>> 
>> To me it seems like we somehow need to start to consider the eMMC'ss
>> BKOPS status in these paths. Depending on the BKOPS state, we may
>> abort the sleep/power off things via returning -EBUSY etc, as
>> otherwise we could end up with poor performing eMMCs, couldn't we!?
> 
> Nope, we could. But I just wonder how *aggressive* folks wanted.. Do
> they would rather sacrificing the performace to save more power when
> adding this caps before?
> 
> To me, things like this didn't always exist the "best choice", but we
> could find a "better choice". :)
> 
I think, by letting the BKOPS to be completed before sending Sleep Command,
we are not cancelling/changing the “aggressive” PM policy.
The Suspend still will be recalled and completed, after getting the right
BKOPS level.
Does the “aggressive” means immediate?
If yes, I think it has to be changed, because some modern eMMC devices need
BKOPS, and the performance impact also need to be considered.

Re: [PATCH v2 0/2] Auto BKOPS PM suspend

[Ulf Hansson]

Hi Uri,

Sorry for the delay.

On 22 January 2017 at 10:15, Uri Yanai <uri.yanai@sandisk.com> wrote:
> Resubmitting three patches for approval, originally submitted by Alex Lemberg <alex.lemberg@sandisk.com>

I can only see two patches in this series.

> [1/3] mmc: replacing hardcoded value for runtime PM suspend(removed)
> [2/3] mmc: Adding AUTO_BKOPS_EN bit set for Auto BKOPS support(changed)
> [3/3] mmc: Checking BKOPS status prior to Suspend(changed)

I think also Shawn Lin pointed out for the earlier submission, the
importance of the information in this cover letter.

First, I think this should give a nice summary of what the series
does, how and why. Neither of these questions are answered.

Second, for each new version, please provide a summary of what has changed.

>
> Uri Yanai (2):
>   mmc: Adding AUTO_BKOPS_EN bit set for Auto BKOPS support
>   mmc: Checking BKOPS status prior to Suspend
>
>  drivers/mmc/core/mmc.c   | 37 +++++++++++++++++++++++++++++++------
>  include/linux/mmc/card.h |  1 +
>  include/linux/mmc/mmc.h  |  1 +
>  3 files changed, 33 insertions(+), 6 deletions(-)
>
> --
> 1.9.1
>

Kind regards
Uffe