[PATCH 0/2] scsi: ufs: Add DeepSleep feature

[PATCH 0/2] scsi: ufs: Add DeepSleep feature

[Adrian Hunter]

Hi

Here is a patch to add DeepSleep, and a patch to workaround an issue hit
when testing.


Adrian Hunter (2):
      scsi: ufs: Add DeepSleep feature
      scsi: ufs: Workaround UFS devices that object to DeepSleep IMMED

 drivers/scsi/ufs/ufs-sysfs.c |  7 ++++
 drivers/scsi/ufs/ufs.h       |  1 +
 drivers/scsi/ufs/ufshcd.c    | 86 +++++++++++++++++++++++++++++++++++++++++---
 drivers/scsi/ufs/ufshcd.h    | 28 ++++++++++++++-
 include/trace/events/ufs.h   |  3 +-
 5 files changed, 119 insertions(+), 6 deletions(-)


Regards
Adrian

[PATCH 1/2] scsi: ufs: Add DeepSleep feature

[Adrian Hunter]

DeepSleep is a UFS v3.1 feature that achieves the lowest power consumption
of the device, apart from power off.

In DeepSleep mode, no commands are accepted, and the only way to exit is
using a hardware reset or power cycle.

This patch assumes that if a power cycle was an option, then power off
would be preferable, so only exit via a hardware reset is supported.

Drivers that wish to support DeepSleep need to set a new capability flag
UFSHCD_CAP_DEEPSLEEP and provide a hardware reset via the existing
 ->device_reset() callback.

It is assumed that UFS devices with wspecversion >= 0x310 support
DeepSleep.

The UFS specification says to set the IMMED (immediate) flag for the
Start/Stop Unit command when entering DeepSleep. However some UFS
devices object to that, which is addressed in a subsequent patch.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/scsi/ufs/ufs-sysfs.c |  7 ++++++
 drivers/scsi/ufs/ufs.h       |  1 +
 drivers/scsi/ufs/ufshcd.c    | 45 ++++++++++++++++++++++++++++++++++--
 drivers/scsi/ufs/ufshcd.h    | 17 +++++++++++++-
 include/trace/events/ufs.h   |  3 ++-
 5 files changed, 69 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index bdcd27faa054..08e72b7eef6a 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -28,6 +28,7 @@ static const char *ufschd_ufs_dev_pwr_mode_to_string(
 	case UFS_ACTIVE_PWR_MODE:	return "ACTIVE";
 	case UFS_SLEEP_PWR_MODE:	return "SLEEP";
 	case UFS_POWERDOWN_PWR_MODE:	return "POWERDOWN";
+	case UFS_DEEPSLEEP_PWR_MODE:	return "DEEPSLEEP";
 	default:			return "UNKNOWN";
 	}
 }
@@ -38,6 +39,7 @@ static inline ssize_t ufs_sysfs_pm_lvl_store(struct device *dev,
 					     bool rpm)
 {
 	struct ufs_hba *hba = dev_get_drvdata(dev);
+	struct ufs_dev_info *dev_info = &hba->dev_info;
 	unsigned long flags, value;
 
 	if (kstrtoul(buf, 0, &value))
@@ -46,6 +48,11 @@ static inline ssize_t ufs_sysfs_pm_lvl_store(struct device *dev,
 	if (value >= UFS_PM_LVL_MAX)
 		return -EINVAL;
 
+	if (ufs_pm_lvl_states[value].dev_state == UFS_DEEPSLEEP_PWR_MODE &&
+	    (!(hba->caps & UFSHCD_CAP_DEEPSLEEP) ||
+	     !(dev_info->wspecversion >= 0x310)))
+		return -EINVAL;
+
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	if (rpm)
 		hba->rpm_lvl = value;
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index f8ab16f30fdc..d593edb48767 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -442,6 +442,7 @@ enum ufs_dev_pwr_mode {
 	UFS_ACTIVE_PWR_MODE	= 1,
 	UFS_SLEEP_PWR_MODE	= 2,
 	UFS_POWERDOWN_PWR_MODE	= 3,
+	UFS_DEEPSLEEP_PWR_MODE	= 4,
 };
 
 #define UFS_WB_BUF_REMAIN_PERCENT(val) ((val) / 10)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index b8f573a02713..d072b0c80bd8 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -163,6 +163,11 @@ struct ufs_pm_lvl_states ufs_pm_lvl_states[] = {
 	{UFS_SLEEP_PWR_MODE, UIC_LINK_HIBERN8_STATE},
 	{UFS_POWERDOWN_PWR_MODE, UIC_LINK_HIBERN8_STATE},
 	{UFS_POWERDOWN_PWR_MODE, UIC_LINK_OFF_STATE},
+	/*
+	 * For DeepSleep, the link is first put in hibern8 and then off.
+	 * Leaving the link in hibern8 is not supported.
+	 */
+	{UFS_DEEPSLEEP_PWR_MODE, UIC_LINK_OFF_STATE},
 };
 
 static inline enum ufs_dev_pwr_mode
@@ -8246,6 +8251,12 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
 		hba->wlun_dev_clr_ua = false;
 	}
 
+	/*
+	 * DeepSleep requires the Immediate flag. DeepSleep state is actually
+	 * entered when the link state goes to Hibern8.
+	 */
+	if (pwr_mode == UFS_DEEPSLEEP_PWR_MODE)
+		cmd[1] = 1;
 	cmd[4] = pwr_mode << 4;
 
 	/*
@@ -8292,7 +8303,8 @@ static int ufshcd_link_state_transition(struct ufs_hba *hba,
 	}
 	/*
 	 * If autobkops is enabled, link can't be turned off because
-	 * turning off the link would also turn off the device.
+	 * turning off the link would also turn off the device, except in the
+	 * case of DeepSleep where the device is expected to remain powered.
 	 */
 	else if ((req_link_state == UIC_LINK_OFF_STATE) &&
 		 (!check_for_bkops || !hba->auto_bkops_enabled)) {
@@ -8302,6 +8314,9 @@ static int ufshcd_link_state_transition(struct ufs_hba *hba,
 		 * put the link in low power mode is to send the DME end point
 		 * to device and then send the DME reset command to local
 		 * unipro. But putting the link in hibern8 is much faster.
+		 *
+		 * Note also that putting the link in Hibern8 is a requirement
+		 * for entering DeepSleep.
 		 */
 		ret = ufshcd_uic_hibern8_enter(hba);
 		if (ret) {
@@ -8434,6 +8449,7 @@ static void ufshcd_hba_vreg_set_hpm(struct ufs_hba *hba)
 static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 {
 	int ret = 0;
+	int check_for_bkops;
 	enum ufs_pm_level pm_lvl;
 	enum ufs_dev_pwr_mode req_dev_pwr_mode;
 	enum uic_link_state req_link_state;
@@ -8519,7 +8535,13 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 	}
 
 	flush_work(&hba->eeh_work);
-	ret = ufshcd_link_state_transition(hba, req_link_state, 1);
+
+	/*
+	 * In the case of DeepSleep, the device is expected to remain powered
+	 * with the link off, so do not check for bkops.
+	 */
+	check_for_bkops = !ufshcd_is_ufs_dev_deepsleep(hba);
+	ret = ufshcd_link_state_transition(hba, req_link_state, check_for_bkops);
 	if (ret)
 		goto set_dev_active;
 
@@ -8560,11 +8582,25 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 	if (hba->clk_scaling.is_allowed)
 		ufshcd_resume_clkscaling(hba);
 	ufshcd_vreg_set_hpm(hba);
+	/*
+	 * Device hardware reset is required to exit DeepSleep. Also, for
+	 * DeepSleep, the link is off so host reset and restore will be done
+	 * further below.
+	 */
+	if (ufshcd_is_ufs_dev_deepsleep(hba)) {
+		ufshcd_vops_device_reset(hba);
+		WARN_ON(!ufshcd_is_link_off(hba));
+	}
 	if (ufshcd_is_link_hibern8(hba) && !ufshcd_uic_hibern8_exit(hba))
 		ufshcd_set_link_active(hba);
 	else if (ufshcd_is_link_off(hba))
 		ufshcd_host_reset_and_restore(hba);
 set_dev_active:
+	/* Can also get here needing to exit DeepSleep */
+	if (ufshcd_is_ufs_dev_deepsleep(hba)) {
+		ufshcd_vops_device_reset(hba);
+		ufshcd_host_reset_and_restore(hba);
+	}
 	if (!ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE))
 		ufshcd_disable_auto_bkops(hba);
 enable_gating:
@@ -8626,6 +8662,9 @@ static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 	if (ret)
 		goto disable_vreg;
 
+	/* For DeepSleep, the only supported option is to have the link off */
+	WARN_ON(ufshcd_is_ufs_dev_deepsleep(hba) && !ufshcd_is_link_off(hba));
+
 	if (ufshcd_is_link_hibern8(hba)) {
 		ret = ufshcd_uic_hibern8_exit(hba);
 		if (!ret) {
@@ -8639,6 +8678,8 @@ static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 		/*
 		 * A full initialization of the host and the device is
 		 * required since the link was put to off during suspend.
+		 * Note, in the case of DeepSleep, the device will exit
+		 * DeepSleep due to device reset.
 		 */
 		ret = ufshcd_reset_and_restore(hba);
 		/*
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 6663325ed8a0..8c6094fb35f4 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -114,16 +114,22 @@ enum uic_link_state {
 	((h)->curr_dev_pwr_mode = UFS_SLEEP_PWR_MODE)
 #define ufshcd_set_ufs_dev_poweroff(h) \
 	((h)->curr_dev_pwr_mode = UFS_POWERDOWN_PWR_MODE)
+#define ufshcd_set_ufs_dev_deepsleep(h) \
+	((h)->curr_dev_pwr_mode = UFS_DEEPSLEEP_PWR_MODE)
 #define ufshcd_is_ufs_dev_active(h) \
 	((h)->curr_dev_pwr_mode == UFS_ACTIVE_PWR_MODE)
 #define ufshcd_is_ufs_dev_sleep(h) \
 	((h)->curr_dev_pwr_mode == UFS_SLEEP_PWR_MODE)
 #define ufshcd_is_ufs_dev_poweroff(h) \
 	((h)->curr_dev_pwr_mode == UFS_POWERDOWN_PWR_MODE)
+#define ufshcd_is_ufs_dev_deepsleep(h) \
+	((h)->curr_dev_pwr_mode == UFS_DEEPSLEEP_PWR_MODE)
 
 /*
  * UFS Power management levels.
- * Each level is in increasing order of power savings.
+ * Each level is in increasing order of power savings, except DeepSleep
+ * which is lower than PowerDown with power on but not PowerDown with
+ * power off.
  */
 enum ufs_pm_level {
 	UFS_PM_LVL_0, /* UFS_ACTIVE_PWR_MODE, UIC_LINK_ACTIVE_STATE */
@@ -132,6 +138,7 @@ enum ufs_pm_level {
 	UFS_PM_LVL_3, /* UFS_SLEEP_PWR_MODE, UIC_LINK_HIBERN8_STATE */
 	UFS_PM_LVL_4, /* UFS_POWERDOWN_PWR_MODE, UIC_LINK_HIBERN8_STATE */
 	UFS_PM_LVL_5, /* UFS_POWERDOWN_PWR_MODE, UIC_LINK_OFF_STATE */
+	UFS_PM_LVL_6, /* UFS_DEEPSLEEP_PWR_MODE, UIC_LINK_OFF_STATE */
 	UFS_PM_LVL_MAX
 };
 
@@ -591,6 +598,14 @@ enum ufshcd_caps {
 	 * inline crypto engine, if it is present
 	 */
 	UFSHCD_CAP_CRYPTO				= 1 << 8,
+
+	/*
+	 * This capability allows the host controller driver to use DeepSleep,
+	 * if it is supported by the UFS device. The host controller driver must
+	 * support device hardware reset via the hba->device_reset() callback,
+	 * in order to exit DeepSleep state.
+	 */
+	UFSHCD_CAP_DEEPSLEEP				= 1 << 9,
 };
 
 struct ufs_hba_variant_params {
diff --git a/include/trace/events/ufs.h b/include/trace/events/ufs.h
index 84841b3a7ffd..2362244c2a9e 100644
--- a/include/trace/events/ufs.h
+++ b/include/trace/events/ufs.h
@@ -19,7 +19,8 @@
 #define UFS_PWR_MODES			\
 	EM(UFS_ACTIVE_PWR_MODE)		\
 	EM(UFS_SLEEP_PWR_MODE)		\
-	EMe(UFS_POWERDOWN_PWR_MODE)
+	EM(UFS_POWERDOWN_PWR_MODE)	\
+	EMe(UFS_DEEPSLEEP_PWR_MODE)
 
 #define UFSCHD_CLK_GATING_STATES	\
 	EM(CLKS_OFF)			\
-- 
2.17.1

[PATCH 2/2] scsi: ufs: Workaround UFS devices that object to DeepSleep IMMED

[Adrian Hunter]

The UFS specification says to set the IMMED (immediate) flag for the
Start/Stop Unit command when entering DeepSleep. However some UFS
devices object to that. Workaround that by retrying without IMMED.
Whichever possibility works, the result is recorded for the next
time.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/scsi/ufs/ufshcd.c | 53 +++++++++++++++++++++++++++++++++------
 drivers/scsi/ufs/ufshcd.h | 11 ++++++++
 2 files changed, 56 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index d072b0c80bd8..3a67a711c0ae 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -8202,6 +8202,44 @@ ufshcd_send_request_sense(struct ufs_hba *hba, struct scsi_device *sdp)
 	return ret;
 }
 
+static bool ufshcd_set_immed(struct ufs_hba *hba,
+			     enum ufs_dev_pwr_mode pwr_mode)
+{
+	/*
+	 * DeepSleep requires the Immediate flag. DeepSleep state is actually
+	 * entered when the link state goes to Hibern8.
+	 */
+	return pwr_mode == UFS_DEEPSLEEP_PWR_MODE &&
+	       hba->deepsleep_immed != UFS_DEEPSLEEP_IMMED_BROKEN;
+}
+
+static bool ufshcd_retry_dev_pwr_mode(struct ufs_hba *hba,
+				      enum ufs_dev_pwr_mode pwr_mode,
+				      unsigned char *cmd, int ret,
+				      struct scsi_sense_hdr *sshdr)
+{
+	if (pwr_mode == UFS_DEEPSLEEP_PWR_MODE &&
+	    hba->deepsleep_immed == UFS_DEEPSLEEP_IMMED_UNKNOWN &&
+	    (cmd[1] & 1) && driver_byte(ret) == DRIVER_SENSE &&
+	    scsi_sense_valid(sshdr) && sshdr->sense_key == ILLEGAL_REQUEST) {
+		cmd[1] &= ~1;
+		return true;
+	}
+	return false;
+}
+
+static void ufshcd_set_dev_pwr_mode_success(struct ufs_hba *hba,
+					    enum ufs_dev_pwr_mode pwr_mode,
+					    unsigned char *cmd)
+{
+	if (pwr_mode == UFS_DEEPSLEEP_PWR_MODE &&
+	    hba->deepsleep_immed == UFS_DEEPSLEEP_IMMED_UNKNOWN) {
+		hba->deepsleep_immed = (cmd[1] & 1) ?
+					UFS_DEEPSLEEP_IMMED_OK :
+					UFS_DEEPSLEEP_IMMED_BROKEN;
+	}
+}
+
 /**
  * ufshcd_set_dev_pwr_mode - sends START STOP UNIT command to set device
  *			     power mode
@@ -8251,14 +8289,9 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
 		hba->wlun_dev_clr_ua = false;
 	}
 
-	/*
-	 * DeepSleep requires the Immediate flag. DeepSleep state is actually
-	 * entered when the link state goes to Hibern8.
-	 */
-	if (pwr_mode == UFS_DEEPSLEEP_PWR_MODE)
-		cmd[1] = 1;
+	cmd[1] = ufshcd_set_immed(hba, pwr_mode) ? 1 : 0;
 	cmd[4] = pwr_mode << 4;
-
+retry:
 	/*
 	 * Current function would be generally called from the power management
 	 * callbacks hence set the RQF_PM flag so that it doesn't resume the
@@ -8267,6 +8300,8 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
 	ret = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
 			START_STOP_TIMEOUT, 0, 0, RQF_PM, NULL);
 	if (ret) {
+		if (ufshcd_retry_dev_pwr_mode(hba, pwr_mode, cmd, ret, &sshdr))
+			goto retry;
 		sdev_printk(KERN_WARNING, sdp,
 			    "START_STOP failed for power mode: %d, result %x\n",
 			    pwr_mode, ret);
@@ -8274,8 +8309,10 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
 			scsi_print_sense_hdr(sdp, NULL, &sshdr);
 	}
 
-	if (!ret)
+	if (!ret) {
 		hba->curr_dev_pwr_mode = pwr_mode;
+		ufshcd_set_dev_pwr_mode_success(hba, pwr_mode, cmd);
+	}
 out:
 	scsi_device_put(sdp);
 	hba->host->eh_noresume = 0;
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 8c6094fb35f4..b4bf00891c9f 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -147,6 +147,16 @@ struct ufs_pm_lvl_states {
 	enum uic_link_state link_state;
 };
 
+/*
+ * Whether or not to set the immediate flag for the DeepSleep START_STOP unit
+ * command.
+ */
+enum ufs_deepsleep_immed {
+	UFS_DEEPSLEEP_IMMED_UNKNOWN,	/* Set IMMED, but retry without it */
+	UFS_DEEPSLEEP_IMMED_OK,		/* Set IMMED */
+	UFS_DEEPSLEEP_IMMED_BROKEN,	/* Do not set IMMED */
+};
+
 /**
  * struct ufshcd_lrb - local reference block
  * @utr_descriptor_ptr: UTRD address of the command
@@ -705,6 +715,7 @@ struct ufs_hba {
 	struct device_attribute rpm_lvl_attr;
 	struct device_attribute spm_lvl_attr;
 	int pm_op_in_progress;
+	enum ufs_deepsleep_immed deepsleep_immed;
 
 	/* Auto-Hibernate Idle Timer register value */
 	u32 ahit;
-- 
2.17.1

RE: [PATCH 1/2] scsi: ufs: Add DeepSleep feature

[Avri Altman]

> +       /*
> +        * DeepSleep requires the Immediate flag. DeepSleep state is actually
> +        * entered when the link state goes to Hibern8.
> +        */
> +       if (pwr_mode == UFS_DEEPSLEEP_PWR_MODE)
> +               cmd[1] = 1;
Shouldn't it be bit1, i.e. cmd[1] = 2 ?

>         cmd[4] = pwr_mode << 4;
> 

RE: [PATCH 1/2] scsi: ufs: Add DeepSleep feature

[Avri Altman]

Please ignore - I was confused with pre-fetch.
Sorry,
Avri

> -----Original Message-----
> From: Avri Altman
> Sent: Sunday, October 4, 2020 10:21 AM
> To: 'Adrian Hunter' <adrian.hunter@intel.com>; Martin K . Petersen
> <martin.petersen@oracle.com>; James E . J . Bottomley <jejb@linux.ibm.com>
> Cc: linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org; Alim Akhtar
> <alim.akhtar@samsung.com>
> Subject: RE: [PATCH 1/2] scsi: ufs: Add DeepSleep feature
> 
> > +       /*
> > +        * DeepSleep requires the Immediate flag. DeepSleep state is actually
> > +        * entered when the link state goes to Hibern8.
> > +        */
> > +       if (pwr_mode == UFS_DEEPSLEEP_PWR_MODE)
> > +               cmd[1] = 1;
> Shouldn't it be bit1, i.e. cmd[1] = 2 ?
> 
> >         cmd[4] = pwr_mode << 4;
> >

RE: [PATCH 1/2] scsi: ufs: Add DeepSleep feature

[Avri Altman]

HI,

> Drivers that wish to support DeepSleep need to set a new capability flag
> UFSHCD_CAP_DEEPSLEEP and provide a hardware reset via the existing
>  ->device_reset() callback.
I would expect that this capability controls sending SSU 4, but it only controls the sysfs entry?

> 
> It is assumed that UFS devices with wspecversion >= 0x310 support
> DeepSleep.
> 
> The UFS specification says to set the IMMED (immediate) flag for the
> Start/Stop Unit command when entering DeepSleep. However some UFS
> devices object to that, which is addressed in a subsequent patch.
After failing to understand what the proper behavior should be with respect of the IMMED bit,
Although I read the applicable section few time, I gave up and consult our system guy,
Which is our jedec representative.  This is his answer:
"...
In order to avoid uncertainty - the host need to set IMMED bit to '0' (this is explicitly specified by the standard).
The device responds only after it switches to Pre-DeepSleep state. The host then switch to H8 and this would trigger the device to transition to DeepSleep state.
..."

So maybe the 2nd patch isn't really needed. 
Thanks,
Avri

RE: [PATCH 2/2] scsi: ufs: Workaround UFS devices that object to DeepSleep IMMED

[Avri Altman]

 
> 
> The UFS specification says to set the IMMED (immediate) flag for the
> Start/Stop Unit command when entering DeepSleep. However some UFS
> devices object to that. Workaround that by retrying without IMMED.
> Whichever possibility works, the result is recorded for the next
> time.

As aforesaid, this patch might not be needed once IMMED is set to 0.
However, Any spec violation should come in a form of a quirk.
The driver is not supposed to figure out the peculiarities of each vendor.

Thanks
Avri

Re: [PATCH 1/2] scsi: ufs: Add DeepSleep feature

[Adrian Hunter]

On 5/10/20 11:02 am, Avri Altman wrote:
> HI,
> 
>> Drivers that wish to support DeepSleep need to set a new capability flag
>> UFSHCD_CAP_DEEPSLEEP and provide a hardware reset via the existing
>>  ->device_reset() callback.
> I would expect that this capability controls sending SSU 4, but it only controls the sysfs entry?

The sysfs entry is the only way to request DeepSleep.

> 
>>
>> It is assumed that UFS devices with wspecversion >= 0x310 support
>> DeepSleep.
>>
>> The UFS specification says to set the IMMED (immediate) flag for the
>> Start/Stop Unit command when entering DeepSleep. However some UFS
>> devices object to that, which is addressed in a subsequent patch.
> After failing to understand what the proper behavior should be with respect of the IMMED bit,
> Although I read the applicable section few time, I gave up and consult our system guy,
> Which is our jedec representative.  This is his answer:
> "...
> In order to avoid uncertainty - the host need to set IMMED bit to '0' (this is explicitly specified by the standard).
> The device responds only after it switches to Pre-DeepSleep state. The host then switch to H8 and this would trigger the device to transition to DeepSleep state.
> ..."
> 
> So maybe the 2nd patch isn't really needed. 

Yes I managed to get it the wrong way around!  I will drop patch 2 and send
V2 of patch 1 in due course.

RE: [PATCH 1/2] scsi: ufs: Add DeepSleep feature

[Avri Altman]

> 
> 
> On 5/10/20 11:02 am, Avri Altman wrote:
> > HI,
> >
> >> Drivers that wish to support DeepSleep need to set a new capability flag
> >> UFSHCD_CAP_DEEPSLEEP and provide a hardware reset via the existing
> >>  ->device_reset() callback.
> > I would expect that this capability controls sending SSU 4, but it only controls
> the sysfs entry?
> 
> The sysfs entry is the only way to request DeepSleep.
Some chipset vendors use their own modules, or even the device tree
to set their default rpm_lvl / spm_lvl.

Thanks,
Avri

Re: [PATCH 1/2] scsi: ufs: Add DeepSleep feature

[Adrian Hunter]

On 5/10/20 12:51 pm, Avri Altman wrote:
>>
>>
>> On 5/10/20 11:02 am, Avri Altman wrote:
>>> HI,
>>>
>>>> Drivers that wish to support DeepSleep need to set a new capability flag
>>>> UFSHCD_CAP_DEEPSLEEP and provide a hardware reset via the existing
>>>>  ->device_reset() callback.
>>> I would expect that this capability controls sending SSU 4, but it only controls
>> the sysfs entry?
>>
>> The sysfs entry is the only way to request DeepSleep.
> Some chipset vendors use their own modules, or even the device tree
> to set their default rpm_lvl / spm_lvl.

I would not expect them to set it wrongly but what are you suggesting?

RE: [PATCH 1/2] scsi: ufs: Add DeepSleep feature

[Avri Altman]

> 
> On 5/10/20 12:51 pm, Avri Altman wrote:
> >>
> >>
> >> On 5/10/20 11:02 am, Avri Altman wrote:
> >>> HI,
> >>>
> >>>> Drivers that wish to support DeepSleep need to set a new capability flag
> >>>> UFSHCD_CAP_DEEPSLEEP and provide a hardware reset via the existing
> >>>>  ->device_reset() callback.
> >>> I would expect that this capability controls sending SSU 4, but it only
> controls
> >> the sysfs entry?
> >>
> >> The sysfs entry is the only way to request DeepSleep.
> > Some chipset vendors use their own modules, or even the device tree
> > to set their default rpm_lvl / spm_lvl.
> 
> I would not expect them to set it wrongly but what are you suggesting?
Right. Let's leave it as it is.

Thanks,
Avri