[PATCH] mmc: sleep notification

[PATCH] mmc: sleep notification

[Avi Shchislowski]

This patch is implements the new additional state of
Power_Off_Notification – SLEEP_NOTIFICATION.
Until now, the implementation of Power_Off_Notification
supported only three modes – POWERED_ON (0x01),
POWER_OFF_SHORT (0x02) and POWER_OFF_LONG (0x03).

As part of eMMC5.0 before moving to Sleep state hosts may set the
POWER_OFF_NOTIFICATION byte to SLEEP_NOTIFICATION (0x04).
After setting SLEEP_NOTIFICATION, host should wait for
the busy line to be de-asserted.
The max timeout allowed for busy line de-assertion defined
in SLEEP_NOTIFICATION_TIME byte in EXT_CSD [216].
HPI may interrupt the SLEEP_NOTIFICATION operation.
In that case POWER_OFF_NOTIFICATION byte will restore to POWERED_ON.

Signed-off-by: Alex Lemberg <alex.lemberg@sandisk.com>
Signed-off-by: Avi Shchislowski <avi.shchislowski@sandisk.com>
---
 drivers/mmc/card/block.c |   19 +++++-
 drivers/mmc/core/core.c  |   16 +++++-
 drivers/mmc/core/core.h  |    2 +
 drivers/mmc/core/mmc.c   |  143 +++++++++++++++++++++++++++++++++++++++++++---
 include/linux/mmc/card.h |    6 ++
 include/linux/mmc/host.h |    1 +
 include/linux/mmc/mmc.h  |    2 +
 7 files changed, 180 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 4409d79..f511ecc3 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -2010,9 +2010,26 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
 	unsigned long flags;
 	unsigned int cmd_flags = req ? req->cmd_flags : 0;
 
-	if (req && !mq->mqrq_prev->req)
+	if (unlikely(req && mmc_card_mmc(card) &&
+		(card->ext_csd.power_off_notification ==
+		EXT_CSD_SLEEP_NOTIFICATION))) {
+		/* restoring the power_off_notification
+		 * field's state to as it was before so
+		 * that the sleep notification will be
+		 * able to resume later
+		 */
+		card->ext_csd.power_off_notification = EXT_CSD_POWER_ON;
+	}
+
+	if (req && !mq->mqrq_prev->req) {
 		/* claim host only for the first request */
 		mmc_get_card(card);
+		if (unlikely(req &&
+			mmc_card_doing_sleep_notify(card))) {
+			mmc_interrupt_hpi(card);
+			mmc_card_clr_sleep_notify(card);
+		}
+	}
 
 	ret = mmc_blk_part_switch(card, md);
 	if (ret) {
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 5bda29b..a090593 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -271,7 +271,8 @@ void mmc_start_bkops(struct mmc_card *card, bool from_exception)
 
 	BUG_ON(!card);
 
-	if (!card->ext_csd.bkops_en || mmc_card_doing_bkops(card))
+	if (!card->ext_csd.bkops_en || mmc_card_doing_bkops(card) ||
+		mmc_card_doing_sleep_notify(card))
 		return;
 
 	err = mmc_read_bkops_status(card);
@@ -2630,6 +2631,19 @@ int mmc_pm_notify(struct notifier_block *notify_block,
 			err = host->bus_ops->pre_suspend(host);
 		if (!err)
 			break;
+		if (host->card && host->bus_ops->suspend) {
+			err = mmc_sleep_notify(host->card);
+			/* We assume that HPI was sent
+			 * in case of -ETIMEDOUT error,
+			 * so suspend flow can be continued
+			 */
+			if (err && err != -ETIMEDOUT) {
+				pr_err("%s:sleep notify err=%d\n",
+					__func__, err);
+				return -EBUSY;
+			}
+			break;
+		}
 
 		/* Calling bus_ops->remove() with a claimed host can deadlock */
 		host->bus_ops->remove(host);
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index d76597c..b6b4431 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -14,6 +14,7 @@
 #include <linux/delay.h>
 
 #define MMC_CMD_RETRIES        3
+#define MMC_SLEEP_NOTIFY_MAX_TIME	0x17
 
 struct mmc_bus_ops {
 	void (*remove)(struct mmc_host *);
@@ -33,6 +34,7 @@ void mmc_attach_bus(struct mmc_host *host, const struct mmc_bus_ops *ops);
 void mmc_detach_bus(struct mmc_host *host);
 
 void mmc_init_erase(struct mmc_card *card);
+int mmc_sleep_notify(struct mmc_card *card);
 
 void mmc_set_chip_select(struct mmc_host *host, int mode);
 void mmc_set_clock(struct mmc_host *host, unsigned int hz);
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 02ad792..1d97d24 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -57,6 +57,11 @@ static const unsigned int tacc_mant[] = {
 		__res & __mask;						\
 	})
 
+#define GET_SLEEP_NOTIFY_TIME(value) \
+	(10 * (1 << (unsigned int)(value)))
+#define GET_SLEEP_NOTIFY_TIME_MSEC(value) \
+	(DIV_ROUND_UP(GET_SLEEP_NOTIFY_TIME(value), 1000))
+
 /*
  * Given the decoded CSD structure, decode the raw CID to our CID structure.
  */
@@ -571,6 +576,8 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
 		card->ext_csd.ffu_capable =
 			(ext_csd[EXT_CSD_SUPPORTED_MODE] & 0x1) &&
 			!(ext_csd[EXT_CSD_FW_CONFIG] & 0x1);
+		card->ext_csd.sleep_notify_time =
+			ext_csd[EXT_CSD_SLEEP_NOTIFICATION_TIME];
 	}
 out:
 	return err;
@@ -1468,6 +1475,15 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
 			card->ext_csd.hpi_en = 1;
 	}
 
+	/* sleep notify enable/disable for eMMC 5.0 and above */
+	if ((card->ext_csd.rev >= 7) &&  card->ext_csd.hpi_en &&
+			(card->host->caps2 & MMC_CAP2_SLEEP_NOTIFY) &&
+			card->ext_csd.sleep_notify_time > 0 &&
+			card->ext_csd.sleep_notify_time <=
+				MMC_SLEEP_NOTIFY_MAX_TIME) {
+		card->can_sleep_notify = 1;
+	}
+
 	/*
 	 * If cache size is higher than 0, this indicates
 	 * the existence of cache and it can be turned on.
@@ -1576,6 +1592,33 @@ static int mmc_sleep(struct mmc_host *host)
 	return err;
 }
 
+/*
+ * check if device is in program state (busy)
+ */
+static bool mmc_device_prg_state(struct mmc_card *card)
+{
+	int rc;
+	u32 status;
+	bool state;
+
+	mmc_get_card(card);
+	rc = mmc_send_status(card, &status);
+	if (rc) {
+		pr_err("%s: Get card status fail. rc=%d\n",
+			mmc_hostname(card->host), rc);
+		state = false;
+		goto out;
+	}
+
+	if (R1_CURRENT_STATE(status) == R1_STATE_PRG)
+		state =  true;
+	else
+		state =  false;
+out:
+	mmc_put_card(card);
+	return state;
+}
+
 static int mmc_can_poweroff_notify(const struct mmc_card *card)
 {
 	return card &&
@@ -1585,22 +1628,108 @@ static int mmc_can_poweroff_notify(const struct mmc_card *card)
 
 static int mmc_poweroff_notify(struct mmc_card *card, unsigned int notify_type)
 {
-	unsigned int timeout = card->ext_csd.generic_cmd6_time;
+	unsigned int timeout_ms = card->ext_csd.generic_cmd6_time;
 	int err;
+	bool use_busy_signal;
 
 	/* Use EXT_CSD_POWER_OFF_SHORT as default notification type. */
 	if (notify_type == EXT_CSD_POWER_OFF_LONG)
-		timeout = card->ext_csd.power_off_longtime;
+		timeout_ms = card->ext_csd.power_off_longtime;
+	else if (notify_type == EXT_CSD_SLEEP_NOTIFICATION) {
+		/* calculate the maximum timeout for the
+		 * switch command when notifying the device
+		 * that it is about to move to sleep */
+		timeout_ms = GET_SLEEP_NOTIFY_TIME_MSEC(
+			card->ext_csd.sleep_notify_time);
+	}
 
+	/* do not wait on busy signal in case of
+	 * Sleep Notification - to let host get
+	 * another requests
+	 */
+	use_busy_signal = (notify_type == EXT_CSD_SLEEP_NOTIFICATION) ?
+			false : true;
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 			EXT_CSD_POWER_OFF_NOTIFICATION,
-			notify_type, timeout, true, false, false);
-	if (err)
+			notify_type, timeout_ms,
+			use_busy_signal, false, false);
+	if (!err) {
+		card->ext_csd.power_off_notification = notify_type;
+	} else {
 		pr_err("%s: Power Off Notification timed out, %u\n",
-		       mmc_hostname(card->host), timeout);
+			mmc_hostname(card->host), timeout_ms);
+	}
+
+	return err;
+}
 
-	/* Disable the power off notification after the switch operation. */
-	card->ext_csd.power_off_notification = EXT_CSD_NO_POWER_NOTIFICATION;
+int mmc_sleep_notify(struct mmc_card *card)
+{
+	int err = 0;
+	bool is_busy = 0;
+	unsigned long prg_wait = 0;
+
+	if (!card->can_sleep_notify || !mmc_can_poweroff_notify(card))
+		return 0;
+
+	if (!mmc_card_doing_sleep_notify(card)) {
+		mmc_get_card(card);
+		mmc_card_set_sleep_notify(card);
+		err = mmc_poweroff_notify(card,
+			EXT_CSD_SLEEP_NOTIFICATION);
+		mmc_put_card(card);
+		if (err) {
+			pr_err("%s: mmc_poweroff_notify failed with %d\n",
+			       __func__, err);
+			goto out;
+		}
+
+		prg_wait = jiffies +
+			msecs_to_jiffies(GET_SLEEP_NOTIFY_TIME_MSEC(
+			card->ext_csd.sleep_notify_time));
+		}
+
+	/*
+	 * Loop will run until:
+	 * 1. Device is no more in Busy state
+	 * 2. Sleep notification is not interrupted by HPI & IO request
+	 */
+	do {
+		/* added some delay to avoid sending card status too often */
+		msleep(20);
+		err = 0;
+		/* Stop polling in case sleep notification was HPIed already */
+		if (!mmc_card_doing_sleep_notify(card)) {
+			is_busy = mmc_device_prg_state(card);
+			if (is_busy)
+				err = -EBUSY;
+			break;
+		}
+		is_busy = mmc_device_prg_state(card);
+		if (is_busy && time_after(jiffies, prg_wait)) {
+			/*
+			 * making sure we are still in busy before
+			 * sending HPI due to timeout error
+			 */
+			is_busy = mmc_device_prg_state(card);
+			if (is_busy) {
+				if (mmc_card_doing_sleep_notify(card)) {
+					card->ext_csd.power_off_notification =
+						EXT_CSD_POWER_ON;
+					mmc_interrupt_hpi(card);
+				}
+				err = -ETIMEDOUT;
+				break;
+			}
+		}
+	} while (is_busy);
+
+out:
+	mmc_card_clr_sleep_notify(card);
+	if (err) {
+		pr_err("%s: mmc_poweroff_notify for sleep failed with %d\n",
+		       mmc_hostname(card->host), err);
+	}
 
 	return err;
 }
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 4d69c00..6998344 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -62,6 +62,7 @@ struct mmc_ext_csd {
 	unsigned int		sa_timeout;		/* Units: 100ns */
 	unsigned int		generic_cmd6_time;	/* Units: 10ms */
 	unsigned int            power_off_longtime;     /* Units: ms */
+	unsigned int		sleep_notify_time;	/* Units: us */
 	u8			power_off_notification;	/* state */
 	unsigned int		hs_max_dtr;
 	unsigned int		hs200_max_dtr;
@@ -262,6 +263,7 @@ struct mmc_card {
 #define MMC_CARD_REMOVED	(1<<4)		/* card has been removed */
 #define MMC_STATE_DOING_BKOPS	(1<<5)		/* card is doing BKOPS */
 #define MMC_STATE_SUSPENDED	(1<<6)		/* card is suspended */
+#define MMC_STATE_SLEEP_NOTIFY (1<<7)		/* card in sleep notify */
 	unsigned int		quirks; 	/* card quirks */
 #define MMC_QUIRK_LENIENT_FN0	(1<<0)		/* allow SDIO FN0 writes outside of the VS CCCR range */
 #define MMC_QUIRK_BLKSZ_FOR_BYTE_MODE (1<<1)	/* use func->cur_blksize */
@@ -309,6 +311,7 @@ struct mmc_card {
 	struct dentry		*debugfs_root;
 	struct mmc_part	part[MMC_NUM_PHY_PARTITION]; /* physical partitions */
 	unsigned int    nr_parts;
+	u8 can_sleep_notify;	/* sleep_notify on/off */
 };
 
 /*
@@ -427,6 +430,7 @@ static inline void __maybe_unused remove_quirk(struct mmc_card *card, int data)
 #define mmc_card_removed(c)	((c) && ((c)->state & MMC_CARD_REMOVED))
 #define mmc_card_doing_bkops(c)	((c)->state & MMC_STATE_DOING_BKOPS)
 #define mmc_card_suspended(c)	((c)->state & MMC_STATE_SUSPENDED)
+#define mmc_card_doing_sleep_notify(c) ((c)->state & MMC_STATE_SLEEP_NOTIFY)
 
 #define mmc_card_set_present(c)	((c)->state |= MMC_STATE_PRESENT)
 #define mmc_card_set_readonly(c) ((c)->state |= MMC_STATE_READONLY)
@@ -437,6 +441,8 @@ static inline void __maybe_unused remove_quirk(struct mmc_card *card, int data)
 #define mmc_card_clr_doing_bkops(c)	((c)->state &= ~MMC_STATE_DOING_BKOPS)
 #define mmc_card_set_suspended(c) ((c)->state |= MMC_STATE_SUSPENDED)
 #define mmc_card_clr_suspended(c) ((c)->state &= ~MMC_STATE_SUSPENDED)
+#define mmc_card_set_sleep_notify(c)	((c)->state |= MMC_STATE_SLEEP_NOTIFY)
+#define mmc_card_clr_sleep_notify(c)	((c)->state &= ~MMC_STATE_SLEEP_NOTIFY)
 
 /*
  * Quirk add/remove for MMC products.
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 9f32270..4c0542a 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -291,6 +291,7 @@ struct mmc_host {
 				 MMC_CAP2_HS400_1_2V)
 #define MMC_CAP2_HSX00_1_2V	(MMC_CAP2_HS200_1_2V_SDR | MMC_CAP2_HS400_1_2V)
 #define MMC_CAP2_SDIO_IRQ_NOTHREAD (1 << 17)
+#define MMC_CAP2_SLEEP_NOTIFY	(1 << 18)	/* sleep notify supported */
 
 	mmc_pm_flag_t		pm_caps;	/* supported pm features */
 
diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
index 49ad7a9..69bda9a 100644
--- a/include/linux/mmc/mmc.h
+++ b/include/linux/mmc/mmc.h
@@ -314,6 +314,7 @@ struct _mmc_csd {
 #define EXT_CSD_PWR_CL_52_360		202	/* RO */
 #define EXT_CSD_PWR_CL_26_360		203	/* RO */
 #define EXT_CSD_SEC_CNT			212	/* RO, 4 bytes */
+#define EXT_CSD_SLEEP_NOTIFICATION_TIME	216	/* RO */
 #define EXT_CSD_S_A_TIMEOUT		217	/* RO */
 #define EXT_CSD_REL_WR_SEC_C		222	/* RO */
 #define EXT_CSD_HC_WP_GRP_SIZE		221	/* RO */
@@ -408,6 +409,7 @@ struct _mmc_csd {
 #define EXT_CSD_POWER_ON		1
 #define EXT_CSD_POWER_OFF_SHORT		2
 #define EXT_CSD_POWER_OFF_LONG		3
+#define EXT_CSD_SLEEP_NOTIFICATION	4
 
 #define EXT_CSD_PWR_CL_8BIT_MASK	0xF0	/* 8 bit PWR CLS */
 #define EXT_CSD_PWR_CL_4BIT_MASK	0x0F	/* 8 bit PWR CLS */
-- 
1.7.9.5

Re: [PATCH] mmc: sleep notification

[Ulf Hansson]

On 10 March 2015 at 10:36, Avi Shchislowski
<avi.shchislowski@sandisk.com> wrote:
> This patch is implements the new additional state of
> Power_Off_Notification – SLEEP_NOTIFICATION.
> Until now, the implementation of Power_Off_Notification
> supported only three modes – POWERED_ON (0x01),
> POWER_OFF_SHORT (0x02) and POWER_OFF_LONG (0x03).
>
> As part of eMMC5.0 before moving to Sleep state hosts may set the
> POWER_OFF_NOTIFICATION byte to SLEEP_NOTIFICATION (0x04).
> After setting SLEEP_NOTIFICATION, host should wait for
> the busy line to be de-asserted.
> The max timeout allowed for busy line de-assertion defined
> in SLEEP_NOTIFICATION_TIME byte in EXT_CSD [216].
> HPI may interrupt the SLEEP_NOTIFICATION operation.
> In that case POWER_OFF_NOTIFICATION byte will restore to POWERED_ON.

Oh, just great! JEDEC has invented yet another way of how to send a
device to sleep state. I wonder what was wrong with the CMD5 option.

Anyway, thanks for looking into this.

>
> Signed-off-by: Alex Lemberg <alex.lemberg@sandisk.com>
> Signed-off-by: Avi Shchislowski <avi.shchislowski@sandisk.com>
> ---
>  drivers/mmc/card/block.c |   19 +++++-
>  drivers/mmc/core/core.c  |   16 +++++-
>  drivers/mmc/core/core.h  |    2 +
>  drivers/mmc/core/mmc.c   |  143 +++++++++++++++++++++++++++++++++++++++++++---
>  include/linux/mmc/card.h |    6 ++
>  include/linux/mmc/host.h |    1 +
>  include/linux/mmc/mmc.h  |    2 +
>  7 files changed, 180 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index 4409d79..f511ecc3 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -2010,9 +2010,26 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
>         unsigned long flags;
>         unsigned int cmd_flags = req ? req->cmd_flags : 0;
>
> -       if (req && !mq->mqrq_prev->req)
> +       if (unlikely(req && mmc_card_mmc(card) &&
> +               (card->ext_csd.power_off_notification ==
> +               EXT_CSD_SLEEP_NOTIFICATION))) {
> +               /* restoring the power_off_notification
> +                * field's state to as it was before so
> +                * that the sleep notification will be
> +                * able to resume later
> +                */
> +               card->ext_csd.power_off_notification = EXT_CSD_POWER_ON;
> +       }
> +
> +       if (req && !mq->mqrq_prev->req) {
>                 /* claim host only for the first request */
>                 mmc_get_card(card);
> +               if (unlikely(req &&
> +                       mmc_card_doing_sleep_notify(card))) {
> +                       mmc_interrupt_hpi(card);
> +                       mmc_card_clr_sleep_notify(card);
> +               }
> +       }

Both above new added code blocks makes no sense to me. Why does the
mmc block layer need to care about this?

>
>         ret = mmc_blk_part_switch(card, md);
>         if (ret) {
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 5bda29b..a090593 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -271,7 +271,8 @@ void mmc_start_bkops(struct mmc_card *card, bool from_exception)
>
>         BUG_ON(!card);
>
> -       if (!card->ext_csd.bkops_en || mmc_card_doing_bkops(card))
> +       if (!card->ext_csd.bkops_en || mmc_card_doing_bkops(card) ||
> +               mmc_card_doing_sleep_notify(card))

We can't do BKOPS when the card is suspended (which is when the card
may be put in sleep state), so no need to protect for this.

>                 return;
>
>         err = mmc_read_bkops_status(card);
> @@ -2630,6 +2631,19 @@ int mmc_pm_notify(struct notifier_block *notify_block,
>                         err = host->bus_ops->pre_suspend(host);
>                 if (!err)
>                         break;
> +               if (host->card && host->bus_ops->suspend) {

Nope, this is the wrong place for this code. You should look into
mmc.c for adding this.

> +                       err = mmc_sleep_notify(host->card);
> +                       /* We assume that HPI was sent
> +                        * in case of -ETIMEDOUT error,
> +                        * so suspend flow can be continued
> +                        */
> +                       if (err && err != -ETIMEDOUT) {
> +                               pr_err("%s:sleep notify err=%d\n",
> +                                       __func__, err);
> +                               return -EBUSY;
> +                       }
> +                       break;
> +               }
>
>                 /* Calling bus_ops->remove() with a claimed host can deadlock */
>                 host->bus_ops->remove(host);
> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
> index d76597c..b6b4431 100644
> --- a/drivers/mmc/core/core.h
> +++ b/drivers/mmc/core/core.h
> @@ -14,6 +14,7 @@
>  #include <linux/delay.h>
>
>  #define MMC_CMD_RETRIES        3
> +#define MMC_SLEEP_NOTIFY_MAX_TIME      0x17
>
>  struct mmc_bus_ops {
>         void (*remove)(struct mmc_host *);
> @@ -33,6 +34,7 @@ void mmc_attach_bus(struct mmc_host *host, const struct mmc_bus_ops *ops);
>  void mmc_detach_bus(struct mmc_host *host);
>
>  void mmc_init_erase(struct mmc_card *card);
> +int mmc_sleep_notify(struct mmc_card *card);

If we need a separate function to deal with this, it should be static
function within mmc.c.

>
>  void mmc_set_chip_select(struct mmc_host *host, int mode);
>  void mmc_set_clock(struct mmc_host *host, unsigned int hz);
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 02ad792..1d97d24 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -57,6 +57,11 @@ static const unsigned int tacc_mant[] = {
>                 __res & __mask;                                         \
>         })
>
> +#define GET_SLEEP_NOTIFY_TIME(value) \
> +       (10 * (1 << (unsigned int)(value)))
> +#define GET_SLEEP_NOTIFY_TIME_MSEC(value) \
> +       (DIV_ROUND_UP(GET_SLEEP_NOTIFY_TIME(value), 1000))
> +
>  /*
>   * Given the decoded CSD structure, decode the raw CID to our CID structure.
>   */
> @@ -571,6 +576,8 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
>                 card->ext_csd.ffu_capable =
>                         (ext_csd[EXT_CSD_SUPPORTED_MODE] & 0x1) &&
>                         !(ext_csd[EXT_CSD_FW_CONFIG] & 0x1);
> +               card->ext_csd.sleep_notify_time =
> +                       ext_csd[EXT_CSD_SLEEP_NOTIFICATION_TIME];
>         }
>  out:
>         return err;
> @@ -1468,6 +1475,15 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
>                         card->ext_csd.hpi_en = 1;
>         }
>
> +       /* sleep notify enable/disable for eMMC 5.0 and above */
> +       if ((card->ext_csd.rev >= 7) &&  card->ext_csd.hpi_en &&

Why depend on ext_csd.hpi_en?

> +                       (card->host->caps2 & MMC_CAP2_SLEEP_NOTIFY) &&

Please don't add a new CAP for this. MMC_CAP2_FULL_PWR_CYCLE tells us
if the card can be fully power gated, I think we can use that
instead!?

> +                       card->ext_csd.sleep_notify_time > 0 &&
> +                       card->ext_csd.sleep_notify_time <=
> +                               MMC_SLEEP_NOTIFY_MAX_TIME) {
> +               card->can_sleep_notify = 1;

Instead of adding "can_sleep_notify", let's just encapsulate similar
code as above in a helper function which tells us if card supports
"sleep notification".

Compare how mmc_can_sleep() is implemented and used.

> +       }
> +
>         /*
>          * If cache size is higher than 0, this indicates
>          * the existence of cache and it can be turned on.
> @@ -1576,6 +1592,33 @@ static int mmc_sleep(struct mmc_host *host)
>         return err;
>  }
>
> +/*
> + * check if device is in program state (busy)
> + */
> +static bool mmc_device_prg_state(struct mmc_card *card)
> +{
> +       int rc;
> +       u32 status;
> +       bool state;
> +
> +       mmc_get_card(card);
> +       rc = mmc_send_status(card, &status);
> +       if (rc) {
> +               pr_err("%s: Get card status fail. rc=%d\n",
> +                       mmc_hostname(card->host), rc);
> +               state = false;
> +               goto out;
> +       }
> +
> +       if (R1_CURRENT_STATE(status) == R1_STATE_PRG)
> +               state =  true;
> +       else
> +               state =  false;
> +out:
> +       mmc_put_card(card);
> +       return state;
> +}
> +
>  static int mmc_can_poweroff_notify(const struct mmc_card *card)
>  {
>         return card &&
> @@ -1585,22 +1628,108 @@ static int mmc_can_poweroff_notify(const struct mmc_card *card)
>
>  static int mmc_poweroff_notify(struct mmc_card *card, unsigned int notify_type)
>  {
> -       unsigned int timeout = card->ext_csd.generic_cmd6_time;
> +       unsigned int timeout_ms = card->ext_csd.generic_cmd6_time;

What's reason to do a rename in this patch? If you want that as a
clarification of the code, please do that in a separate patch.

>         int err;
> +       bool use_busy_signal;
>
>         /* Use EXT_CSD_POWER_OFF_SHORT as default notification type. */
>         if (notify_type == EXT_CSD_POWER_OFF_LONG)
> -               timeout = card->ext_csd.power_off_longtime;
> +               timeout_ms = card->ext_csd.power_off_longtime;
> +       else if (notify_type == EXT_CSD_SLEEP_NOTIFICATION) {
> +               /* calculate the maximum timeout for the
> +                * switch command when notifying the device
> +                * that it is about to move to sleep */
> +               timeout_ms = GET_SLEEP_NOTIFY_TIME_MSEC(
> +                       card->ext_csd.sleep_notify_time);
> +       }
>
> +       /* do not wait on busy signal in case of
> +        * Sleep Notification - to let host get
> +        * another requests

At this point, there should be no other request. Or what am I missing?

> +        */
> +       use_busy_signal = (notify_type == EXT_CSD_SLEEP_NOTIFICATION) ?
> +                       false : true;
>         err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>                         EXT_CSD_POWER_OFF_NOTIFICATION,
> -                       notify_type, timeout, true, false, false);
> -       if (err)
> +                       notify_type, timeout_ms,
> +                       use_busy_signal, false, false);
> +       if (!err) {
> +               card->ext_csd.power_off_notification = notify_type;
> +       } else {
>                 pr_err("%s: Power Off Notification timed out, %u\n",
> -                      mmc_hostname(card->host), timeout);
> +                       mmc_hostname(card->host), timeout_ms);
> +       }
> +
> +       return err;
> +}
>
> -       /* Disable the power off notification after the switch operation. */
> -       card->ext_csd.power_off_notification = EXT_CSD_NO_POWER_NOTIFICATION;

Why remove this?

> +int mmc_sleep_notify(struct mmc_card *card)

I don't understand the need for this function. From what scenarios do
we want to invoke it?

> +{
> +       int err = 0;
> +       bool is_busy = 0;
> +       unsigned long prg_wait = 0;
> +
> +       if (!card->can_sleep_notify || !mmc_can_poweroff_notify(card))
> +               return 0;
> +
> +       if (!mmc_card_doing_sleep_notify(card)) {
> +               mmc_get_card(card);
> +               mmc_card_set_sleep_notify(card);
> +               err = mmc_poweroff_notify(card,
> +                       EXT_CSD_SLEEP_NOTIFICATION);
> +               mmc_put_card(card);
> +               if (err) {
> +                       pr_err("%s: mmc_poweroff_notify failed with %d\n",
> +                              __func__, err);
> +                       goto out;
> +               }
> +
> +               prg_wait = jiffies +
> +                       msecs_to_jiffies(GET_SLEEP_NOTIFY_TIME_MSEC(
> +                       card->ext_csd.sleep_notify_time));
> +               }
> +
> +       /*
> +        * Loop will run until:
> +        * 1. Device is no more in Busy state
> +        * 2. Sleep notification is not interrupted by HPI & IO request
> +        */
> +       do {
> +               /* added some delay to avoid sending card status too often */
> +               msleep(20);
> +               err = 0;
> +               /* Stop polling in case sleep notification was HPIed already */
> +               if (!mmc_card_doing_sleep_notify(card)) {
> +                       is_busy = mmc_device_prg_state(card);
> +                       if (is_busy)
> +                               err = -EBUSY;
> +                       break;
> +               }
> +               is_busy = mmc_device_prg_state(card);
> +               if (is_busy && time_after(jiffies, prg_wait)) {
> +                       /*
> +                        * making sure we are still in busy before
> +                        * sending HPI due to timeout error
> +                        */
> +                       is_busy = mmc_device_prg_state(card);
> +                       if (is_busy) {
> +                               if (mmc_card_doing_sleep_notify(card)) {
> +                                       card->ext_csd.power_off_notification =
> +                                               EXT_CSD_POWER_ON;
> +                                       mmc_interrupt_hpi(card);
> +                               }
> +                               err = -ETIMEDOUT;
> +                               break;
> +                       }
> +               }
> +       } while (is_busy);
> +
> +out:
> +       mmc_card_clr_sleep_notify(card);
> +       if (err) {
> +               pr_err("%s: mmc_poweroff_notify for sleep failed with %d\n",
> +                      mmc_hostname(card->host), err);
> +       }
>
>         return err;
>  }
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index 4d69c00..6998344 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -62,6 +62,7 @@ struct mmc_ext_csd {
>         unsigned int            sa_timeout;             /* Units: 100ns */
>         unsigned int            generic_cmd6_time;      /* Units: 10ms */
>         unsigned int            power_off_longtime;     /* Units: ms */
> +       unsigned int            sleep_notify_time;      /* Units: us */
>         u8                      power_off_notification; /* state */
>         unsigned int            hs_max_dtr;
>         unsigned int            hs200_max_dtr;
> @@ -262,6 +263,7 @@ struct mmc_card {
>  #define MMC_CARD_REMOVED       (1<<4)          /* card has been removed */
>  #define MMC_STATE_DOING_BKOPS  (1<<5)          /* card is doing BKOPS */
>  #define MMC_STATE_SUSPENDED    (1<<6)          /* card is suspended */
> +#define MMC_STATE_SLEEP_NOTIFY (1<<7)          /* card in sleep notify */

I think MMC_STATE_SUSPENDED should be enough, we don't need another
one specific for SLEEP_NOTIFY.

>         unsigned int            quirks;         /* card quirks */
>  #define MMC_QUIRK_LENIENT_FN0  (1<<0)          /* allow SDIO FN0 writes outside of the VS CCCR range */
>  #define MMC_QUIRK_BLKSZ_FOR_BYTE_MODE (1<<1)   /* use func->cur_blksize */
> @@ -309,6 +311,7 @@ struct mmc_card {
>         struct dentry           *debugfs_root;
>         struct mmc_part part[MMC_NUM_PHY_PARTITION]; /* physical partitions */
>         unsigned int    nr_parts;
> +       u8 can_sleep_notify;    /* sleep_notify on/off */

As stated, please remove.

>  };
>
>  /*
> @@ -427,6 +430,7 @@ static inline void __maybe_unused remove_quirk(struct mmc_card *card, int data)
>  #define mmc_card_removed(c)    ((c) && ((c)->state & MMC_CARD_REMOVED))
>  #define mmc_card_doing_bkops(c)        ((c)->state & MMC_STATE_DOING_BKOPS)
>  #define mmc_card_suspended(c)  ((c)->state & MMC_STATE_SUSPENDED)
> +#define mmc_card_doing_sleep_notify(c) ((c)->state & MMC_STATE_SLEEP_NOTIFY)
>
>  #define mmc_card_set_present(c)        ((c)->state |= MMC_STATE_PRESENT)
>  #define mmc_card_set_readonly(c) ((c)->state |= MMC_STATE_READONLY)
> @@ -437,6 +441,8 @@ static inline void __maybe_unused remove_quirk(struct mmc_card *card, int data)
>  #define mmc_card_clr_doing_bkops(c)    ((c)->state &= ~MMC_STATE_DOING_BKOPS)
>  #define mmc_card_set_suspended(c) ((c)->state |= MMC_STATE_SUSPENDED)
>  #define mmc_card_clr_suspended(c) ((c)->state &= ~MMC_STATE_SUSPENDED)
> +#define mmc_card_set_sleep_notify(c)   ((c)->state |= MMC_STATE_SLEEP_NOTIFY)
> +#define mmc_card_clr_sleep_notify(c)   ((c)->state &= ~MMC_STATE_SLEEP_NOTIFY)
>
>  /*
>   * Quirk add/remove for MMC products.
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 9f32270..4c0542a 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -291,6 +291,7 @@ struct mmc_host {
>                                  MMC_CAP2_HS400_1_2V)
>  #define MMC_CAP2_HSX00_1_2V    (MMC_CAP2_HS200_1_2V_SDR | MMC_CAP2_HS400_1_2V)
>  #define MMC_CAP2_SDIO_IRQ_NOTHREAD (1 << 17)
> +#define MMC_CAP2_SLEEP_NOTIFY  (1 << 18)       /* sleep notify supported */
>
>         mmc_pm_flag_t           pm_caps;        /* supported pm features */
>
> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
> index 49ad7a9..69bda9a 100644
> --- a/include/linux/mmc/mmc.h
> +++ b/include/linux/mmc/mmc.h
> @@ -314,6 +314,7 @@ struct _mmc_csd {
>  #define EXT_CSD_PWR_CL_52_360          202     /* RO */
>  #define EXT_CSD_PWR_CL_26_360          203     /* RO */
>  #define EXT_CSD_SEC_CNT                        212     /* RO, 4 bytes */
> +#define EXT_CSD_SLEEP_NOTIFICATION_TIME        216     /* RO */
>  #define EXT_CSD_S_A_TIMEOUT            217     /* RO */
>  #define EXT_CSD_REL_WR_SEC_C           222     /* RO */
>  #define EXT_CSD_HC_WP_GRP_SIZE         221     /* RO */
> @@ -408,6 +409,7 @@ struct _mmc_csd {
>  #define EXT_CSD_POWER_ON               1
>  #define EXT_CSD_POWER_OFF_SHORT                2
>  #define EXT_CSD_POWER_OFF_LONG         3
> +#define EXT_CSD_SLEEP_NOTIFICATION     4
>
>  #define EXT_CSD_PWR_CL_8BIT_MASK       0xF0    /* 8 bit PWR CLS */
>  #define EXT_CSD_PWR_CL_4BIT_MASK       0x0F    /* 8 bit PWR CLS */
> --
> 1.7.9.5
>

Kind regards
Uffe

Re: [PATCH] mmc: sleep notification

[Adrian Hunter]

On 10/03/15 11:36, Avi Shchislowski wrote:
> This patch is implements the new additional state of
> Power_Off_Notification – SLEEP_NOTIFICATION.
> Until now, the implementation of Power_Off_Notification
> supported only three modes – POWERED_ON (0x01),
> POWER_OFF_SHORT (0x02) and POWER_OFF_LONG (0x03).
> 
> As part of eMMC5.0 before moving to Sleep state hosts may set the
> POWER_OFF_NOTIFICATION byte to SLEEP_NOTIFICATION (0x04).
> After setting SLEEP_NOTIFICATION, host should wait for
> the busy line to be de-asserted.
> The max timeout allowed for busy line de-assertion defined
> in SLEEP_NOTIFICATION_TIME byte in EXT_CSD [216].
> HPI may interrupt the SLEEP_NOTIFICATION operation.
> In that case POWER_OFF_NOTIFICATION byte will restore to POWERED_ON.

Is it so that the point of SLEEP_NOTIFICATION is to notify that
VCC will be powered off after the device is put to sleep by CMD5?

At the moment, the card does not get put to sleep if there is
support for power-off-notification?

RE: [PATCH] mmc: sleep notification

[Alex Lemberg]

Hi Ulf,

Thanks for your review and comments!

> -----Original Message-----
> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> Sent: Tuesday, March 10, 2015 5:08 AM
> To: Avi Shchislowski
> Cc: linux-mmc; Chris Ball; Alex Lemberg
> Subject: Re: [PATCH] mmc: sleep notification
> 
> On 10 March 2015 at 10:36, Avi Shchislowski <avi.shchislowski@sandisk.com>
> wrote:
> > This patch is implements the new additional state of
> > Power_Off_Notification – SLEEP_NOTIFICATION.
> > Until now, the implementation of Power_Off_Notification supported only
> > three modes – POWERED_ON (0x01), POWER_OFF_SHORT (0x02) and
> > POWER_OFF_LONG (0x03).
> >
> > As part of eMMC5.0 before moving to Sleep state hosts may set the
> > POWER_OFF_NOTIFICATION byte to SLEEP_NOTIFICATION (0x04).
> > After setting SLEEP_NOTIFICATION, host should wait for the busy line
> > to be de-asserted.
> > The max timeout allowed for busy line de-assertion defined in
> > SLEEP_NOTIFICATION_TIME byte in EXT_CSD [216].
> > HPI may interrupt the SLEEP_NOTIFICATION operation.
> > In that case POWER_OFF_NOTIFICATION byte will restore to POWERED_ON.
> 
> Oh, just great! JEDEC has invented yet another way of how to send a device to
> sleep state. I wonder what was wrong with the CMD5 option.

I think before adding sleep_notification, host has no way to notify device before sending
sleep command. Now device can better prepare itself for moving into the sleep state.

Also, I think we need to clarify one more point for this patch:
As was mentioned in commit message - Sleep_Notification can be interrupted by HPI.
This allows not blocking the host during the Sleep_Notification busy time and allows accepting 
requests coming during this stage. Thus, without having HPI supported, suspend/resume process
might be influenced by Sleep_Notification busy time, and this should not happen - suspend/resume
should be done in very fast and not blocking manner.


Thanks,
Alex

RE: [PATCH] mmc: sleep notification

[Alex Lemberg]

Hi Adrian,

> -----Original Message-----
> From: Adrian Hunter [mailto:adrian.hunter@intel.com]
> Sent: Tuesday, March 10, 2015 6:36 AM
> To: Avi Shchislowski; ulf.hansson@linaro.org
> Cc: linux-mmc@vger.kernel.org; chris@printf.net; Alex Lemberg
> Subject: Re: [PATCH] mmc: sleep notification
> 
> On 10/03/15 11:36, Avi Shchislowski wrote:
> > This patch is implements the new additional state of
> > Power_Off_Notification - SLEEP_NOTIFICATION.
> > Until now, the implementation of Power_Off_Notification supported only
> > three modes - POWERED_ON (0x01), POWER_OFF_SHORT (0x02) and
> > POWER_OFF_LONG (0x03).
> >
> > As part of eMMC5.0 before moving to Sleep state hosts may set the
> > POWER_OFF_NOTIFICATION byte to SLEEP_NOTIFICATION (0x04).
> > After setting SLEEP_NOTIFICATION, host should wait for the busy line
> > to be de-asserted.
> > The max timeout allowed for busy line de-assertion defined in
> > SLEEP_NOTIFICATION_TIME byte in EXT_CSD [216].
> > HPI may interrupt the SLEEP_NOTIFICATION operation.
> > In that case POWER_OFF_NOTIFICATION byte will restore to POWERED_ON.
> 
> Is it so that the point of SLEEP_NOTIFICATION is to notify that VCC will be
> powered off after the device is put to sleep by CMD5?

>From the spec: 

the host should issue a power off notification (POWER_OFF_LONG, POWER_OFF_SHORT )
if it intends to turn off both VCC and VCCQ power supplies or it may use 
to a power off notification (SLEEP_NOTIFICATION ) if it intends to turn-off VCC
after moving the device to Sleep state.

> 
> At the moment, the card does not get put to sleep if there is support for power-
> off-notification?

Driver will send Sleep only if PON is not supported...

Thanks,
Alex

Re: [PATCH] mmc: sleep notification

[Ulf Hansson]

On 10 March 2015 at 17:32, Alex Lemberg <Alex.Lemberg@sandisk.com> wrote:
> Hi Adrian,
>
>> -----Original Message-----
>> From: Adrian Hunter [mailto:adrian.hunter@intel.com]
>> Sent: Tuesday, March 10, 2015 6:36 AM
>> To: Avi Shchislowski; ulf.hansson@linaro.org
>> Cc: linux-mmc@vger.kernel.org; chris@printf.net; Alex Lemberg
>> Subject: Re: [PATCH] mmc: sleep notification
>>
>> On 10/03/15 11:36, Avi Shchislowski wrote:
>> > This patch is implements the new additional state of
>> > Power_Off_Notification - SLEEP_NOTIFICATION.
>> > Until now, the implementation of Power_Off_Notification supported only
>> > three modes - POWERED_ON (0x01), POWER_OFF_SHORT (0x02) and
>> > POWER_OFF_LONG (0x03).
>> >
>> > As part of eMMC5.0 before moving to Sleep state hosts may set the
>> > POWER_OFF_NOTIFICATION byte to SLEEP_NOTIFICATION (0x04).
>> > After setting SLEEP_NOTIFICATION, host should wait for the busy line
>> > to be de-asserted.
>> > The max timeout allowed for busy line de-assertion defined in
>> > SLEEP_NOTIFICATION_TIME byte in EXT_CSD [216].
>> > HPI may interrupt the SLEEP_NOTIFICATION operation.
>> > In that case POWER_OFF_NOTIFICATION byte will restore to POWERED_ON.
>>
>> Is it so that the point of SLEEP_NOTIFICATION is to notify that VCC will be
>> powered off after the device is put to sleep by CMD5?
>
> From the spec:
>
> the host should issue a power off notification (POWER_OFF_LONG, POWER_OFF_SHORT )
> if it intends to turn off both VCC and VCCQ power supplies or it may use
> to a power off notification (SLEEP_NOTIFICATION ) if it intends to turn-off VCC
> after moving the device to Sleep state.

Ah, so CMD5 will still need to be issued. Thanks for clarifying.

>
>>
>> At the moment, the card does not get put to sleep if there is support for power-
>> off-notification?
>
> Driver will send Sleep only if PON is not supported...

Nope, that's not entirely correct.

The MMC core requires PON to be supported by the card and the host to
have MMC_CAP2_FULL_PWR_CYCLE set. Else it will send sleep (CMD5)
instead.

Kind regards
Uffe

RE: [PATCH] mmc: sleep notification

[Alex Lemberg]

> -----Original Message-----
> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> Sent: Tuesday, March 10, 2015 10:22 AM
> To: Alex Lemberg
> Cc: Adrian Hunter; Avi Shchislowski; linux-mmc@vger.kernel.org;
> chris@printf.net
> Subject: Re: [PATCH] mmc: sleep notification
> 
> On 10 March 2015 at 17:32, Alex Lemberg <Alex.Lemberg@sandisk.com>
> wrote:
> > Hi Adrian,
> >
> >> -----Original Message-----
> >> From: Adrian Hunter [mailto:adrian.hunter@intel.com]
> >> Sent: Tuesday, March 10, 2015 6:36 AM
> >> To: Avi Shchislowski; ulf.hansson@linaro.org
> >> Cc: linux-mmc@vger.kernel.org; chris@printf.net; Alex Lemberg
> >> Subject: Re: [PATCH] mmc: sleep notification
> >>
> >> On 10/03/15 11:36, Avi Shchislowski wrote:
> >> > This patch is implements the new additional state of
> >> > Power_Off_Notification - SLEEP_NOTIFICATION.
> >> > Until now, the implementation of Power_Off_Notification supported
> >> > only three modes - POWERED_ON (0x01), POWER_OFF_SHORT (0x02) and
> >> > POWER_OFF_LONG (0x03).
> >> >
> >> > As part of eMMC5.0 before moving to Sleep state hosts may set the
> >> > POWER_OFF_NOTIFICATION byte to SLEEP_NOTIFICATION (0x04).
> >> > After setting SLEEP_NOTIFICATION, host should wait for the busy
> >> > line to be de-asserted.
> >> > The max timeout allowed for busy line de-assertion defined in
> >> > SLEEP_NOTIFICATION_TIME byte in EXT_CSD [216].
> >> > HPI may interrupt the SLEEP_NOTIFICATION operation.
> >> > In that case POWER_OFF_NOTIFICATION byte will restore to
> POWERED_ON.
> >>
> >> Is it so that the point of SLEEP_NOTIFICATION is to notify that VCC
> >> will be powered off after the device is put to sleep by CMD5?
> >
> > From the spec:
> >
> > the host should issue a power off notification (POWER_OFF_LONG,
> > POWER_OFF_SHORT ) if it intends to turn off both VCC and VCCQ power
> > supplies or it may use to a power off notification (SLEEP_NOTIFICATION
> > ) if it intends to turn-off VCC after moving the device to Sleep state.
> 
> Ah, so CMD5 will still need to be issued. Thanks for clarifying.
> 
> >
> >>
> >> At the moment, the card does not get put to sleep if there is support
> >> for power- off-notification?
> >
> > Driver will send Sleep only if PON is not supported...
> 
> Nope, that's not entirely correct.
> 
> The MMC core requires PON to be supported by the card and the host to have
> MMC_CAP2_FULL_PWR_CYCLE set. Else it will send sleep (CMD5) instead.
>

You are right - it requires both PON and MMC_CAP2_FULL_PWR_CYCLE to be set...
Thanks for mentioning this.

 
> Kind regards
> Uffe

Re: [PATCH] mmc: sleep notification

[Ulf Hansson]

On 10 March 2015 at 15:32, Alex Lemberg <Alex.Lemberg@sandisk.com> wrote:
> Hi Ulf,
>
> Thanks for your review and comments!
>
>> -----Original Message-----
>> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
>> Sent: Tuesday, March 10, 2015 5:08 AM
>> To: Avi Shchislowski
>> Cc: linux-mmc; Chris Ball; Alex Lemberg
>> Subject: Re: [PATCH] mmc: sleep notification
>>
>> On 10 March 2015 at 10:36, Avi Shchislowski <avi.shchislowski@sandisk.com>
>> wrote:
>> > This patch is implements the new additional state of
>> > Power_Off_Notification – SLEEP_NOTIFICATION.
>> > Until now, the implementation of Power_Off_Notification supported only
>> > three modes – POWERED_ON (0x01), POWER_OFF_SHORT (0x02) and
>> > POWER_OFF_LONG (0x03).
>> >
>> > As part of eMMC5.0 before moving to Sleep state hosts may set the
>> > POWER_OFF_NOTIFICATION byte to SLEEP_NOTIFICATION (0x04).
>> > After setting SLEEP_NOTIFICATION, host should wait for the busy line
>> > to be de-asserted.
>> > The max timeout allowed for busy line de-assertion defined in
>> > SLEEP_NOTIFICATION_TIME byte in EXT_CSD [216].
>> > HPI may interrupt the SLEEP_NOTIFICATION operation.
>> > In that case POWER_OFF_NOTIFICATION byte will restore to POWERED_ON.
>>
>> Oh, just great! JEDEC has invented yet another way of how to send a device to
>> sleep state. I wonder what was wrong with the CMD5 option.
>
> I think before adding sleep_notification, host has no way to notify device before sending
> sleep command. Now device can better prepare itself for moving into the sleep state.
>
> Also, I think we need to clarify one more point for this patch:
> As was mentioned in commit message - Sleep_Notification can be interrupted by HPI.
> This allows not blocking the host during the Sleep_Notification busy time and allows accepting
> requests coming during this stage. Thus, without having HPI supported, suspend/resume process
> might be influenced by Sleep_Notification busy time, and this should not happen - suspend/resume
> should be done in very fast and not blocking manner.

I fail to understand your comment here.

Please tell me at what point(s) your think it make sense to issue the
SLEEP_NOTIFICATION? If that is during the suspend phase, then a HPI
request can't be triggered.

Kind regards
Uffe

RE: [PATCH] mmc: sleep notification

[Alex Lemberg]

Hi Ulf,

> -----Original Message-----
> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> Sent: Wednesday, March 11, 2015 4:50 AM
> To: Alex Lemberg
> Cc: Avi Shchislowski; linux-mmc; Chris Ball
> Subject: Re: [PATCH] mmc: sleep notification
> 
> On 10 March 2015 at 15:32, Alex Lemberg <Alex.Lemberg@sandisk.com>
> wrote:
> > Hi Ulf,
> >
> > Thanks for your review and comments!
> >
> >> -----Original Message-----
> >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> >> Sent: Tuesday, March 10, 2015 5:08 AM
> >> To: Avi Shchislowski
> >> Cc: linux-mmc; Chris Ball; Alex Lemberg
> >> Subject: Re: [PATCH] mmc: sleep notification
> >>
> >> On 10 March 2015 at 10:36, Avi Shchislowski
> >> <avi.shchislowski@sandisk.com>
> >> wrote:
> >> > This patch is implements the new additional state of
> >> > Power_Off_Notification – SLEEP_NOTIFICATION.
> >> > Until now, the implementation of Power_Off_Notification supported
> >> > only three modes – POWERED_ON (0x01), POWER_OFF_SHORT (0x02)
> and
> >> > POWER_OFF_LONG (0x03).
> >> >
> >> > As part of eMMC5.0 before moving to Sleep state hosts may set the
> >> > POWER_OFF_NOTIFICATION byte to SLEEP_NOTIFICATION (0x04).
> >> > After setting SLEEP_NOTIFICATION, host should wait for the busy
> >> > line to be de-asserted.
> >> > The max timeout allowed for busy line de-assertion defined in
> >> > SLEEP_NOTIFICATION_TIME byte in EXT_CSD [216].
> >> > HPI may interrupt the SLEEP_NOTIFICATION operation.
> >> > In that case POWER_OFF_NOTIFICATION byte will restore to
> POWERED_ON.
> >>
> >> Oh, just great! JEDEC has invented yet another way of how to send a
> >> device to sleep state. I wonder what was wrong with the CMD5 option.
> >
> > I think before adding sleep_notification, host has no way to notify
> > device before sending sleep command. Now device can better prepare itself
> for moving into the sleep state.
> >
> > Also, I think we need to clarify one more point for this patch:
> > As was mentioned in commit message - Sleep_Notification can be interrupted
> by HPI.
> > This allows not blocking the host during the Sleep_Notification busy
> > time and allows accepting requests coming during this stage. Thus,
> > without having HPI supported, suspend/resume process might be
> > influenced by Sleep_Notification busy time, and this should not happen -
> suspend/resume should be done in very fast and not blocking manner.
> 
> I fail to understand your comment here.
> 
> Please tell me at what point(s) your think it make sense to issue the
> SLEEP_NOTIFICATION? If that is during the suspend phase, then a HPI request
> can't be triggered.

I think SLEEP_NOTIFICATION should be issued on mmc_pm_notify() call,
on PM_SUSPEND_PREPARE case.
 

> Kind regards
> Uffe

Re: [PATCH] mmc: sleep notification

[Ulf Hansson]

[...]

>> > Also, I think we need to clarify one more point for this patch:
>> > As was mentioned in commit message - Sleep_Notification can be interrupted
>> by HPI.
>> > This allows not blocking the host during the Sleep_Notification busy
>> > time and allows accepting requests coming during this stage. Thus,
>> > without having HPI supported, suspend/resume process might be
>> > influenced by Sleep_Notification busy time, and this should not happen -
>> suspend/resume should be done in very fast and not blocking manner.
>>
>> I fail to understand your comment here.
>>
>> Please tell me at what point(s) your think it make sense to issue the
>> SLEEP_NOTIFICATION? If that is during the suspend phase, then a HPI request
>> can't be triggered.
>
> I think SLEEP_NOTIFICATION should be issued on mmc_pm_notify() call,
> on PM_SUSPEND_PREPARE case.

So, exactly why is that to prefer, comparing doing it in system PM
->suspend() callback?

Kind regards
Uffe

Re: [PATCH] mmc: sleep notification

[Jaehoon Chung]

On 03/12/2015 06:09 PM, Ulf Hansson wrote:
> [...]
> 
>>>> Also, I think we need to clarify one more point for this patch:
>>>> As was mentioned in commit message - Sleep_Notification can be interrupted
>>> by HPI.
>>>> This allows not blocking the host during the Sleep_Notification busy
>>>> time and allows accepting requests coming during this stage. Thus,
>>>> without having HPI supported, suspend/resume process might be
>>>> influenced by Sleep_Notification busy time, and this should not happen -
>>> suspend/resume should be done in very fast and not blocking manner.
>>>
>>> I fail to understand your comment here.
>>>
>>> Please tell me at what point(s) your think it make sense to issue the
>>> SLEEP_NOTIFICATION? If that is during the suspend phase, then a HPI request
>>> can't be triggered.
>>
>> I think SLEEP_NOTIFICATION should be issued on mmc_pm_notify() call,
>> on PM_SUSPEND_PREPARE case.
> 
> So, exactly why is that to prefer, comparing doing it in system PM
> ->suspend() callback?

Actually, i didn't know the benefit of this feature.

Best Regards,
Jaehoon Chung

> 
> 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] mmc: sleep notification

[Avi Shchislowski]

>-----Original Message-----
>From: Adrian Hunter [mailto:adrian.hunter@intel.com]
>Sent: Tuesday, March 10, 2015 3:36 PM
>To: Avi Shchislowski; ulf.hansson@linaro.org
>Cc: linux-mmc@vger.kernel.org; chris@printf.net; Alex Lemberg
>Subject: Re: [PATCH] mmc: sleep notification
>
>On 10/03/15 11:36, Avi Shchislowski wrote:
>> This patch is implements the new additional state of
>> Power_Off_Notification - SLEEP_NOTIFICATION.
>> Until now, the implementation of Power_Off_Notification supported only
>> three modes - POWERED_ON (0x01), POWER_OFF_SHORT (0x02) and
>> POWER_OFF_LONG (0x03).
>>
>> As part of eMMC5.0 before moving to Sleep state hosts may set the
>> POWER_OFF_NOTIFICATION byte to SLEEP_NOTIFICATION (0x04).
>> After setting SLEEP_NOTIFICATION, host should wait for the busy line
>> to be de-asserted.
>> The max timeout allowed for busy line de-assertion defined in
>> SLEEP_NOTIFICATION_TIME byte in EXT_CSD [216].
>> HPI may interrupt the SLEEP_NOTIFICATION operation.
>> In that case POWER_OFF_NOTIFICATION byte will restore to POWERED_ON.
>
>Is it so that the point of SLEEP_NOTIFICATION is to notify that VCC will be
>powered off after the device is put to sleep by CMD5?
The purpose of SLEEP_NOTIFICATION is to give the eMMC enough time to prepare itself before powered off.
Before the SLEEP_NOTIFICATION fetcher the eMMC did not know he will get into sleep but only when the sleep was issued
With this solution the device start to prepare itself during the sleep preparation

>
>At the moment, the card does not get put to sleep if there is support for
>power-off-notification?
 No, it will enter to sleep after the device will finish its "internal work"

RE: [PATCH] mmc: sleep notification

[Alex Lemberg]

Hi Ulf,

> -----Original Message-----
> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> Sent: Thursday, March 12, 2015 11:10 AM
> To: Alex Lemberg
> Cc: Avi Shchislowski; linux-mmc; Chris Ball
> Subject: Re: [PATCH] mmc: sleep notification
> 
> [...]
> 
> >> > Also, I think we need to clarify one more point for this patch:
> >> > As was mentioned in commit message - Sleep_Notification can be
> >> > interrupted
> >> by HPI.
> >> > This allows not blocking the host during the Sleep_Notification
> >> > busy time and allows accepting requests coming during this stage.
> >> > Thus, without having HPI supported, suspend/resume process might be
> >> > influenced by Sleep_Notification busy time, and this should not
> >> > happen -
> >> suspend/resume should be done in very fast and not blocking manner.
> >>
> >> I fail to understand your comment here.
> >>
> >> Please tell me at what point(s) your think it make sense to issue the
> >> SLEEP_NOTIFICATION? If that is during the suspend phase, then a HPI
> >> request can't be triggered.
> >
> > I think SLEEP_NOTIFICATION should be issued on mmc_pm_notify() call,
> > on PM_SUSPEND_PREPARE case.
> 
> So, exactly why is that to prefer, comparing doing it in system PM
> ->suspend() callback?

Assuming that SLEEP_NOTIFICATION may take time
(defined in SLEEP_NOTIFICATION_TIME byte in EXT_CSD [216]),
I think it is better to send it from pm notifier - mmc_pm_notify().

> 
> Kind regards
> Uffe

Re: [PATCH] mmc: sleep notification

[Ulf Hansson]

On 16 March 2015 at 12:37, Alex Lemberg <Alex.Lemberg@sandisk.com> wrote:
> Hi Ulf,
>
>> -----Original Message-----
>> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
>> Sent: Thursday, March 12, 2015 11:10 AM
>> To: Alex Lemberg
>> Cc: Avi Shchislowski; linux-mmc; Chris Ball
>> Subject: Re: [PATCH] mmc: sleep notification
>>
>> [...]
>>
>> >> > Also, I think we need to clarify one more point for this patch:
>> >> > As was mentioned in commit message - Sleep_Notification can be
>> >> > interrupted
>> >> by HPI.
>> >> > This allows not blocking the host during the Sleep_Notification
>> >> > busy time and allows accepting requests coming during this stage.
>> >> > Thus, without having HPI supported, suspend/resume process might be
>> >> > influenced by Sleep_Notification busy time, and this should not
>> >> > happen -
>> >> suspend/resume should be done in very fast and not blocking manner.
>> >>
>> >> I fail to understand your comment here.
>> >>
>> >> Please tell me at what point(s) your think it make sense to issue the
>> >> SLEEP_NOTIFICATION? If that is during the suspend phase, then a HPI
>> >> request can't be triggered.
>> >
>> > I think SLEEP_NOTIFICATION should be issued on mmc_pm_notify() call,
>> > on PM_SUSPEND_PREPARE case.
>>
>> So, exactly why is that to prefer, comparing doing it in system PM
>> ->suspend() callback?
>
> Assuming that SLEEP_NOTIFICATION may take time
> (defined in SLEEP_NOTIFICATION_TIME byte in EXT_CSD [216]),
> I think it is better to send it from pm notifier - mmc_pm_notify().

I assume you think that you will "earn" some milliseconds doing it in
this phase, but I am not so sure.

After the PM_SUSPEND_PREPARE notifier, we still have the mmc block
queue open and are ready to serve requests. Therefore I would expect
the SLEEP_NOTIFICATION to potentially be interrupted by using HPI.

Then we end up with the following sequence during the system PM sleep phase.
1. Issue SLEEP_NOTIFICATION.
2. Interrupt SLEEP_NOTIFICATION, using HPI.
3. Serve blk request
4. Issue SLEEP_NOTIFICATION.
5. Issue SLEEP (CMD5).

That seems like a bad idea to me.

Kind regards
Uffe

RE: [PATCH] mmc: sleep notification

[Alex Lemberg]

> -----Original Message-----
> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> Sent: Monday, March 16, 2015 3:35 PM
> To: Alex Lemberg
> Cc: Avi Shchislowski; linux-mmc; Chris Ball
> Subject: Re: [PATCH] mmc: sleep notification
> 
> On 16 March 2015 at 12:37, Alex Lemberg <Alex.Lemberg@sandisk.com>
> wrote:
> > Hi Ulf,
> >
> >> -----Original Message-----
> >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> >> Sent: Thursday, March 12, 2015 11:10 AM
> >> To: Alex Lemberg
> >> Cc: Avi Shchislowski; linux-mmc; Chris Ball
> >> Subject: Re: [PATCH] mmc: sleep notification
> >>
> >> [...]
> >>
> >> >> > Also, I think we need to clarify one more point for this patch:
> >> >> > As was mentioned in commit message - Sleep_Notification can be
> >> >> > interrupted
> >> >> by HPI.
> >> >> > This allows not blocking the host during the Sleep_Notification
> >> >> > busy time and allows accepting requests coming during this stage.
> >> >> > Thus, without having HPI supported, suspend/resume process might
> >> >> > be influenced by Sleep_Notification busy time, and this should
> >> >> > not happen -
> >> >> suspend/resume should be done in very fast and not blocking manner.
> >> >>
> >> >> I fail to understand your comment here.
> >> >>
> >> >> Please tell me at what point(s) your think it make sense to issue
> >> >> the SLEEP_NOTIFICATION? If that is during the suspend phase, then
> >> >> a HPI request can't be triggered.
> >> >
> >> > I think SLEEP_NOTIFICATION should be issued on mmc_pm_notify()
> >> > call, on PM_SUSPEND_PREPARE case.
> >>
> >> So, exactly why is that to prefer, comparing doing it in system PM
> >> ->suspend() callback?
> >
> > Assuming that SLEEP_NOTIFICATION may take time (defined in
> > SLEEP_NOTIFICATION_TIME byte in EXT_CSD [216]), I think it is better
> > to send it from pm notifier - mmc_pm_notify().
> 
> I assume you think that you will "earn" some milliseconds doing it in this phase,
> but I am not so sure.

I was thinking mainly about how to prevent "blocking" during Sleep_Notification process.

> 
> After the PM_SUSPEND_PREPARE notifier, we still have the mmc block queue
> open and are ready to serve requests. Therefore I would expect the
> SLEEP_NOTIFICATION to potentially be interrupted by using HPI.

Right, and this was a reason for adding HPI support during the Sleep_Notification.

> 
> Then we end up with the following sequence during the system PM sleep phase.
> 1. Issue SLEEP_NOTIFICATION.
> 2. Interrupt SLEEP_NOTIFICATION, using HPI.
> 3. Serve blk request
> 4. Issue SLEEP_NOTIFICATION.
> 5. Issue SLEEP (CMD5).

Correct, this is a sequence that we see after adding the patch.

> That seems like a bad idea to me.

Could you please explain why?
Would you suggest to call Sleep_Notification from Suspend only?
What if Sleep_Notification will take several seconds?

> 
> Kind regards
> Uffe

Re: [PATCH] mmc: sleep notification

[Ulf Hansson]

On 16 March 2015 at 17:58, Alex Lemberg <Alex.Lemberg@sandisk.com> wrote:
>
>
>> -----Original Message-----
>> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
>> Sent: Monday, March 16, 2015 3:35 PM
>> To: Alex Lemberg
>> Cc: Avi Shchislowski; linux-mmc; Chris Ball
>> Subject: Re: [PATCH] mmc: sleep notification
>>
>> On 16 March 2015 at 12:37, Alex Lemberg <Alex.Lemberg@sandisk.com>
>> wrote:
>> > Hi Ulf,
>> >
>> >> -----Original Message-----
>> >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
>> >> Sent: Thursday, March 12, 2015 11:10 AM
>> >> To: Alex Lemberg
>> >> Cc: Avi Shchislowski; linux-mmc; Chris Ball
>> >> Subject: Re: [PATCH] mmc: sleep notification
>> >>
>> >> [...]
>> >>
>> >> >> > Also, I think we need to clarify one more point for this patch:
>> >> >> > As was mentioned in commit message - Sleep_Notification can be
>> >> >> > interrupted
>> >> >> by HPI.
>> >> >> > This allows not blocking the host during the Sleep_Notification
>> >> >> > busy time and allows accepting requests coming during this stage.
>> >> >> > Thus, without having HPI supported, suspend/resume process might
>> >> >> > be influenced by Sleep_Notification busy time, and this should
>> >> >> > not happen -
>> >> >> suspend/resume should be done in very fast and not blocking manner.
>> >> >>
>> >> >> I fail to understand your comment here.
>> >> >>
>> >> >> Please tell me at what point(s) your think it make sense to issue
>> >> >> the SLEEP_NOTIFICATION? If that is during the suspend phase, then
>> >> >> a HPI request can't be triggered.
>> >> >
>> >> > I think SLEEP_NOTIFICATION should be issued on mmc_pm_notify()
>> >> > call, on PM_SUSPEND_PREPARE case.
>> >>
>> >> So, exactly why is that to prefer, comparing doing it in system PM
>> >> ->suspend() callback?
>> >
>> > Assuming that SLEEP_NOTIFICATION may take time (defined in
>> > SLEEP_NOTIFICATION_TIME byte in EXT_CSD [216]), I think it is better
>> > to send it from pm notifier - mmc_pm_notify().
>>
>> I assume you think that you will "earn" some milliseconds doing it in this phase,
>> but I am not so sure.
>
> I was thinking mainly about how to prevent "blocking" during Sleep_Notification process.
>
>>
>> After the PM_SUSPEND_PREPARE notifier, we still have the mmc block queue
>> open and are ready to serve requests. Therefore I would expect the
>> SLEEP_NOTIFICATION to potentially be interrupted by using HPI.
>
> Right, and this was a reason for adding HPI support during the Sleep_Notification.
>
>>
>> Then we end up with the following sequence during the system PM sleep phase.
>> 1. Issue SLEEP_NOTIFICATION.
>> 2. Interrupt SLEEP_NOTIFICATION, using HPI.
>> 3. Serve blk request
>> 4. Issue SLEEP_NOTIFICATION.
>> 5. Issue SLEEP (CMD5).
>
> Correct, this is a sequence that we see after adding the patch.
>
>> That seems like a bad idea to me.
>
> Could you please explain why?
> Would you suggest to call Sleep_Notification from Suspend only?

Since otherwise we may end up issuing SLEEP_NOTIFICATION not only
once, but twice during a system PM sleep sequence. So then we actually
get an increased system PM suspend time.

> What if Sleep_Notification will take several seconds?

Yes, that horrible! You should tell your colleagues designing FTLs to
make sure that _never_ happens.

Also, this makes me wonder how this kind of feature ever could make it
into the JEDEC specification.

What we had earlier with "CMD5 only" should never have been changed.

Kind regards
Uffe

RE: [PATCH] mmc: sleep notification

[Alex Lemberg]

Hi Ulf,

Sorry for the delay in response...

> -----Original Message-----
> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> Sent: Tuesday, March 17, 2015 12:44 PM
> To: Alex Lemberg
> Cc: Avi Shchislowski; linux-mmc; Chris Ball
> Subject: Re: [PATCH] mmc: sleep notification
> 
> On 16 March 2015 at 17:58, Alex Lemberg <Alex.Lemberg@sandisk.com>
> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> >> Sent: Monday, March 16, 2015 3:35 PM
> >> To: Alex Lemberg
> >> Cc: Avi Shchislowski; linux-mmc; Chris Ball
> >> Subject: Re: [PATCH] mmc: sleep notification
> >>
> >> On 16 March 2015 at 12:37, Alex Lemberg <Alex.Lemberg@sandisk.com>
> >> wrote:
> >> > Hi Ulf,
> >> >
> >> >> -----Original Message-----
> >> >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> >> >> Sent: Thursday, March 12, 2015 11:10 AM
> >> >> To: Alex Lemberg
> >> >> Cc: Avi Shchislowski; linux-mmc; Chris Ball
> >> >> Subject: Re: [PATCH] mmc: sleep notification
> >> >>
> >> >> [...]
> >> >>
> >> >> >> > Also, I think we need to clarify one more point for this patch:
> >> >> >> > As was mentioned in commit message - Sleep_Notification can
> >> >> >> > be interrupted
> >> >> >> by HPI.
> >> >> >> > This allows not blocking the host during the
> >> >> >> > Sleep_Notification busy time and allows accepting requests coming
> during this stage.
> >> >> >> > Thus, without having HPI supported, suspend/resume process
> >> >> >> > might be influenced by Sleep_Notification busy time, and this
> >> >> >> > should not happen -
> >> >> >> suspend/resume should be done in very fast and not blocking manner.
> >> >> >>
> >> >> >> I fail to understand your comment here.
> >> >> >>
> >> >> >> Please tell me at what point(s) your think it make sense to
> >> >> >> issue the SLEEP_NOTIFICATION? If that is during the suspend
> >> >> >> phase, then a HPI request can't be triggered.
> >> >> >
> >> >> > I think SLEEP_NOTIFICATION should be issued on mmc_pm_notify()
> >> >> > call, on PM_SUSPEND_PREPARE case.
> >> >>
> >> >> So, exactly why is that to prefer, comparing doing it in system PM
> >> >> ->suspend() callback?
> >> >
> >> > Assuming that SLEEP_NOTIFICATION may take time (defined in
> >> > SLEEP_NOTIFICATION_TIME byte in EXT_CSD [216]), I think it is
> >> > better to send it from pm notifier - mmc_pm_notify().
> >>
> >> I assume you think that you will "earn" some milliseconds doing it in
> >> this phase, but I am not so sure.
> >
> > I was thinking mainly about how to prevent "blocking" during
> Sleep_Notification process.
> >
> >>
> >> After the PM_SUSPEND_PREPARE notifier, we still have the mmc block
> >> queue open and are ready to serve requests. Therefore I would expect
> >> the SLEEP_NOTIFICATION to potentially be interrupted by using HPI.
> >
> > Right, and this was a reason for adding HPI support during the
> Sleep_Notification.
> >
> >>
> >> Then we end up with the following sequence during the system PM sleep
> phase.
> >> 1. Issue SLEEP_NOTIFICATION.
> >> 2. Interrupt SLEEP_NOTIFICATION, using HPI.
> >> 3. Serve blk request
> >> 4. Issue SLEEP_NOTIFICATION.
> >> 5. Issue SLEEP (CMD5).
> >
> > Correct, this is a sequence that we see after adding the patch.
> >
> >> That seems like a bad idea to me.
> >
> > Could you please explain why?
> > Would you suggest to call Sleep_Notification from Suspend only?
> 
> Since otherwise we may end up issuing SLEEP_NOTIFICATION not only once, but
> twice during a system PM sleep sequence. So then we actually get an increased
> system PM suspend time.
> 
> > What if Sleep_Notification will take several seconds?
> 
> Yes, that horrible! You should tell your colleagues designing FTLs to make sure
> that _never_ happens.

Agree, but we need to consider and take care of all such cases in the driver side. 
Device might be busy with its internal garbage collection, and as spec allows, it
will complete it in a great manner after host sends PON command.
 
> 
> Also, this makes me wonder how this kind of feature ever could make it into the
> JEDEC specification.
> 
> What we had earlier with "CMD5 only" should never have been changed.
> 
> Kind regards
> Uffe

Re: [PATCH] mmc: sleep notification

[Ulf Hansson]

[...]

>> > What if Sleep_Notification will take several seconds?
>>
>> Yes, that horrible! You should tell your colleagues designing FTLs to make sure
>> that _never_ happens.
>
> Agree, but we need to consider and take care of all such cases in the driver side.
> Device might be busy with its internal garbage collection, and as spec allows, it
> will complete it in a great manner after host sends PON command.
>

Okay, so to me, I think the SLEEP notification needs to be sent as a
part of the system PM suspend phase. And more exactly from the
bus_ops->suspend() callback.

In addition to that, we should be able improve the situation by also
sending the SLEEP notification as part of an "idle operation". With
"idle operation" I mean the mmc block layer hasn’t received any new
request for a while (the framework for this is already implemented by
using runtime PM).

So if the SLEEP notification was sent during the "idle operation" and
no new request was needed during the system PM suspend phase we
wouldn't have to wake up the card again using HPI, but instead just
leave it "sleeping" and send CMD5.

Still, there would be no guarantees that "idle operations" is executed
before a system PM suspend phase starts. So there are no way we can
improve the worst case scenario.

Kind regards
Uffe

RE: [PATCH] mmc: sleep notification

[Alex Lemberg]

Hi Ulf,

> -----Original Message-----
> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> Sent: Friday, March 27, 2015 3:57 PM
> To: Alex Lemberg
> Cc: Avi Shchislowski; linux-mmc; Chris Ball
> Subject: Re: [PATCH] mmc: sleep notification
> 
> [...]
> 
> >> > What if Sleep_Notification will take several seconds?
> >>
> >> Yes, that horrible! You should tell your colleagues designing FTLs to
> >> make sure that _never_ happens.
> >
> > Agree, but we need to consider and take care of all such cases in the driver
> side.
> > Device might be busy with its internal garbage collection, and as spec
> > allows, it will complete it in a great manner after host sends PON command.
> >
> 
> Okay, so to me, I think the SLEEP notification needs to be sent as a part of the
> system PM suspend phase. And more exactly from the
> bus_ops->suspend() callback.
> 

PM suspend phase is a right place for PON Sleep_Notification,
but it also the critical one and need to be completed as fast as possible.

If I'm not mistaken, during the PM Suspend, the user space is freezing.
Is it OK to let PON Sleep_Notification to be called at that stage?

Calling from the notifier_call (mmc_pm_notify()) will allow to complete 
Sleep_Notification process before entering to the critical part - the Suspend.
Isn't it?

> In addition to that, we should be able improve the situation by also sending the
> SLEEP notification as part of an "idle operation". With "idle operation" I mean
> the mmc block layer hasn’t received any new request for a while (the
> framework for this is already implemented by using runtime PM).

OK, sounds like a good approach. 
In case runtime PM is supported, we can call PON Sleep_Notification from 
mmc_runtime_suspend() during the "idle operation",
and then call it again during PM Suspend.

> 
> So if the SLEEP notification was sent during the "idle operation" and no new
> request was needed during the system PM suspend phase we wouldn't have to
> wake up the card again using HPI, but instead just leave it "sleeping" and send
> CMD5.
> 
> Still, there would be no guarantees that "idle operations" is executed before a
> system PM suspend phase starts. So there are no way we can improve the
> worst case scenario.
> 
> Kind regards
> Uffe

Re: [PATCH] mmc: sleep notification

[Ulf Hansson]

[...]

>> Okay, so to me, I think the SLEEP notification needs to be sent as a part of the
>> system PM suspend phase. And more exactly from the
>> bus_ops->suspend() callback.
>>
>
> PM suspend phase is a right place for PON Sleep_Notification,
> but it also the critical one and need to be completed as fast as possible.
>
> If I'm not mistaken, during the PM Suspend, the user space is freezing.
> Is it OK to let PON Sleep_Notification to be called at that stage?

Of course we shall strive in minimizing the system PM suspend time.
Though to me, I think the system PM _resume_ time is of more
importance.

In this case we don't have any option. We need to issue the sleep
notification as part of the the system PM suspend phase and that will
unfortunate and likely increase the total system PM suspend time.

>
> Calling from the notifier_call (mmc_pm_notify()) will allow to complete
> Sleep_Notification process before entering to the critical part - the Suspend.
> Isn't it?

Why is mmc_pm_notify() a less critical part?

More importantly as stated several times, we can still get I/O
requests after the mmc_pm_notify() phase and then the sleep
notification needs to be interrupted and re-issued anyway.

>
>> In addition to that, we should be able improve the situation by also sending the
>> SLEEP notification as part of an "idle operation". With "idle operation" I mean
>> the mmc block layer hasn’t received any new request for a while (the
>> framework for this is already implemented by using runtime PM).
>
> OK, sounds like a good approach.
> In case runtime PM is supported, we can call PON Sleep_Notification from
> mmc_runtime_suspend() during the "idle operation",
> and then call it again during PM Suspend.

Yes.

If you plan to send a new version of $subject patch, please split into
at least two patches. One which adds the system PM part and one which
adds the "idle operation" part.

Kind regards
Uffe

RE: [PATCH] mmc: sleep notification

[Alex Lemberg]

Hi Ulf,

> -----Original Message-----
> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> Sent: Wednesday, April 01, 2015 2:46 PM
> To: Alex Lemberg
> Cc: Avi Shchislowski; linux-mmc; Chris Ball
> Subject: Re: [PATCH] mmc: sleep notification
> 
> [...]
> 
> >> Okay, so to me, I think the SLEEP notification needs to be sent as a
> >> part of the system PM suspend phase. And more exactly from the
> >> bus_ops->suspend() callback.
> >>
> >
> > PM suspend phase is a right place for PON Sleep_Notification, but it
> > also the critical one and need to be completed as fast as possible.
> >
> > If I'm not mistaken, during the PM Suspend, the user space is freezing.
> > Is it OK to let PON Sleep_Notification to be called at that stage?
> 
> Of course we shall strive in minimizing the system PM suspend time.
> Though to me, I think the system PM _resume_ time is of more importance.
> 
> In this case we don't have any option. We need to issue the sleep notification as
> part of the the system PM suspend phase and that will unfortunate and likely
> increase the total system PM suspend time.
> 
> >
> > Calling from the notifier_call (mmc_pm_notify()) will allow to
> > complete Sleep_Notification process before entering to the critical part - the
> Suspend.
> > Isn't it?
> 
> Why is mmc_pm_notify() a less critical part?
> 
> More importantly as stated several times, we can still get I/O requests after
> the mmc_pm_notify() phase and then the sleep notification needs to be
> interrupted and re-issued anyway.

You are right - the sleep_notification might be re-issued several times,
but in case of mmc_pm_notify() the user space is not frozen yet, and 
system will not stuck until sleep_notification completion, like in case of 
PM suspend.

> 
> >
> >> In addition to that, we should be able improve the situation by also
> >> sending the SLEEP notification as part of an "idle operation". With
> >> "idle operation" I mean the mmc block layer hasn’t received any new
> >> request for a while (the framework for this is already implemented by using
> runtime PM).
> >
> > OK, sounds like a good approach.
> > In case runtime PM is supported, we can call PON Sleep_Notification
> > from
> > mmc_runtime_suspend() during the "idle operation", and then call it
> > again during PM Suspend.
> 
> Yes.
> 
> If you plan to send a new version of $subject patch, please split into at least two
> patches. One which adds the system PM part and one which adds the "idle
> operation" part.

As far as I see, the same _mmc_suspend() function is used in both cases - 
PM Suspend and "idle operation".
The _mmc_suspend() is called from both  - mmc_suspend() and
mmc_runtime_suspend().
In case we decide to go with PM Suspend and "idle operation",
should we just add the sleep_notification support to _mmc_suspend() for
covering both cases?

> 
> Kind regards
> Uffe

Re: [PATCH] mmc: sleep notification

[Ulf Hansson]

On 6 April 2015 at 15:44, Alex Lemberg <Alex.Lemberg@sandisk.com> wrote:
> Hi Ulf,
>
>> -----Original Message-----
>> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
>> Sent: Wednesday, April 01, 2015 2:46 PM
>> To: Alex Lemberg
>> Cc: Avi Shchislowski; linux-mmc; Chris Ball
>> Subject: Re: [PATCH] mmc: sleep notification
>>
>> [...]
>>
>> >> Okay, so to me, I think the SLEEP notification needs to be sent as a
>> >> part of the system PM suspend phase. And more exactly from the
>> >> bus_ops->suspend() callback.
>> >>
>> >
>> > PM suspend phase is a right place for PON Sleep_Notification, but it
>> > also the critical one and need to be completed as fast as possible.
>> >
>> > If I'm not mistaken, during the PM Suspend, the user space is freezing.
>> > Is it OK to let PON Sleep_Notification to be called at that stage?
>>
>> Of course we shall strive in minimizing the system PM suspend time.
>> Though to me, I think the system PM _resume_ time is of more importance.
>>
>> In this case we don't have any option. We need to issue the sleep notification as
>> part of the the system PM suspend phase and that will unfortunate and likely
>> increase the total system PM suspend time.
>>
>> >
>> > Calling from the notifier_call (mmc_pm_notify()) will allow to
>> > complete Sleep_Notification process before entering to the critical part - the
>> Suspend.
>> > Isn't it?
>>
>> Why is mmc_pm_notify() a less critical part?
>>
>> More importantly as stated several times, we can still get I/O requests after
>> the mmc_pm_notify() phase and then the sleep notification needs to be
>> interrupted and re-issued anyway.
>
> You are right - the sleep_notification might be re-issued several times,
> but in case of mmc_pm_notify() the user space is not frozen yet, and
> system will not stuck until sleep_notification completion, like in case of
> PM suspend.
>
>>
>> >
>> >> In addition to that, we should be able improve the situation by also
>> >> sending the SLEEP notification as part of an "idle operation". With
>> >> "idle operation" I mean the mmc block layer hasn’t received any new
>> >> request for a while (the framework for this is already implemented by using
>> runtime PM).
>> >
>> > OK, sounds like a good approach.
>> > In case runtime PM is supported, we can call PON Sleep_Notification
>> > from
>> > mmc_runtime_suspend() during the "idle operation", and then call it
>> > again during PM Suspend.
>>
>> Yes.
>>
>> If you plan to send a new version of $subject patch, please split into at least two
>> patches. One which adds the system PM part and one which adds the "idle
>> operation" part.
>
> As far as I see, the same _mmc_suspend() function is used in both cases -
> PM Suspend and "idle operation".
> The _mmc_suspend() is called from both  - mmc_suspend() and
> mmc_runtime_suspend().
> In case we decide to go with PM Suspend and "idle operation",
> should we just add the sleep_notification support to _mmc_suspend() for
> covering both cases?

That works as a start.

Though, it would mean MMC_CAP_AGGRESSIVE_PM is needed to enable "idle
operations" for this case. It might be good enough. If not, we need to
send the sleep notification from mmc_runtime_suspend() as well.

Kind regards
Uffe