[PATCH v1 0/4] scsi: ufs: Fix WriteBooster and cleanup UFS driver

[PATCH v1 0/4] scsi: ufs: Fix WriteBooster and cleanup UFS driver

[Stanley Chu]

Hi,

This patch set fixes some WriteBooster issues and do small cleanup in UFS driver

Stanley Chu (4):
  scsi: ufs: Remove unnecessary memset for dev_info
  scsi: ufs: Allow WriteBooster on UFS 2.2 devices
  scsi: ufs: Fix index of attributes query for WriteBooster feature
  scsi: ufs: Fix WriteBooster flush during runtime suspend

 drivers/scsi/ufs/ufs-sysfs.c | 13 ++++++--
 drivers/scsi/ufs/ufs.h       |  1 -
 drivers/scsi/ufs/ufshcd.c    | 59 ++++++++++++++++++------------------
 drivers/scsi/ufs/ufshcd.h    |  2 +-
 4 files changed, 42 insertions(+), 33 deletions(-)

-- 
2.18.0

[PATCH v1 1/4] scsi: ufs: Remove unnecessary memset for dev_info

[Stanley Chu]

The whole UFS host instance has been zero-initialized by
scsi_host_alloc(), thus UFS driver does not need to clear
"dev_info" member specifically in ufshcd_device_params_init().

Simply remove the unnecessary code.

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufshcd.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 426073a518ef..41ad4501b0d0 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7279,9 +7279,6 @@ static int ufshcd_device_params_init(struct ufs_hba *hba)
 	bool flag;
 	int ret;
 
-	/* Clear any previous UFS device information */
-	memset(&hba->dev_info, 0, sizeof(hba->dev_info));
-
 	/* Init check for device descriptor sizes */
 	ufshcd_init_desc_sizes(hba);
 
-- 
2.18.0

[PATCH v1 2/4] scsi: ufs: Allow WriteBooster on UFS 2.2 devices

[Stanley Chu]

According to the UFS specification, WriteBooster is officially
supported by UFS 2.2.

Since UFS 2.2 specification has been finalized in JEDEC and
such devices have also showed up in the market, modify the
checking rule for ufshcd_wb_probe() to allow these devices to enable
WriteBooster.

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufshcd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 41ad4501b0d0..b298bdd3e697 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6942,6 +6942,7 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
 	 * UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES enabled
 	 */
 	if (dev_info->wspecversion >= 0x310 ||
+	    dev_info->wspecversion == 0x220 ||
 	    (hba->dev_quirks & UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES))
 		ufshcd_wb_probe(hba, desc_buf);
 
-- 
2.18.0

[PATCH v1 3/4] scsi: ufs: Fix index of attributes query for WriteBooster feature

[Stanley Chu]

For WriteBooster feature related attributes, the index used by
query shall be LUN ID if LU Dedicated buffer mode is enabled.

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufs-sysfs.c | 13 +++++++++++--
 drivers/scsi/ufs/ufshcd.c    | 16 ++++++++++------
 drivers/scsi/ufs/ufshcd.h    |  2 +-
 3 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index a0b3763e1dc2..2d71d232a69d 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -637,7 +637,7 @@ static ssize_t _name##_show(struct device *dev,				\
 	int ret;							\
 	struct ufs_hba *hba = dev_get_drvdata(dev);			\
 	if (ufshcd_is_wb_flags(QUERY_FLAG_IDN##_uname))			\
-		index = ufshcd_wb_get_flag_index(hba);			\
+		index = ufshcd_wb_get_query_index(hba);			\
 	pm_runtime_get_sync(hba->dev);					\
 	ret = ufshcd_query_flag(hba, UPIU_QUERY_OPCODE_READ_FLAG,	\
 		QUERY_FLAG_IDN##_uname, index, &flag);			\
@@ -680,6 +680,12 @@ static const struct attribute_group ufs_sysfs_flags_group = {
 	.attrs = ufs_sysfs_device_flags,
 };
 
+static inline bool ufshcd_is_wb_attrs(enum attr_idn idn)
+{
+	return ((idn >= QUERY_ATTR_IDN_WB_FLUSH_STATUS) &&
+		(idn <= QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE));
+}
+
 #define UFS_ATTRIBUTE(_name, _uname)					\
 static ssize_t _name##_show(struct device *dev,				\
 	struct device_attribute *attr, char *buf)			\
@@ -687,9 +693,12 @@ static ssize_t _name##_show(struct device *dev,				\
 	struct ufs_hba *hba = dev_get_drvdata(dev);			\
 	u32 value;							\
 	int ret;							\
+	u8 index = 0;							\
+	if (ufshcd_is_wb_attrs(QUERY_ATTR_IDN##_uname))			\
+		index = ufshcd_wb_get_query_index(hba);			\
 	pm_runtime_get_sync(hba->dev);					\
 	ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,	\
-		QUERY_ATTR_IDN##_uname, 0, 0, &value);			\
+		QUERY_ATTR_IDN##_uname, index, 0, &value);		\
 	pm_runtime_put_sync(hba->dev);					\
 	if (ret)							\
 		return -EINVAL;						\
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index b298bdd3e697..169a3379e468 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5212,7 +5212,7 @@ static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
 	else
 		opcode = UPIU_QUERY_OPCODE_CLEAR_FLAG;
 
-	index = ufshcd_wb_get_flag_index(hba);
+	index = ufshcd_wb_get_query_index(hba);
 	ret = ufshcd_query_flag_retry(hba, opcode,
 				      QUERY_FLAG_IDN_WB_EN, index, NULL);
 	if (ret) {
@@ -5238,7 +5238,7 @@ static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set)
 	else
 		val = UPIU_QUERY_OPCODE_CLEAR_FLAG;
 
-	index = ufshcd_wb_get_flag_index(hba);
+	index = ufshcd_wb_get_query_index(hba);
 	return ufshcd_query_flag_retry(hba, val,
 				QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8,
 				index, NULL);
@@ -5261,7 +5261,7 @@ static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba)
 	if (!ufshcd_is_wb_allowed(hba) || hba->wb_buf_flush_enabled)
 		return 0;
 
-	index = ufshcd_wb_get_flag_index(hba);
+	index = ufshcd_wb_get_query_index(hba);
 	ret = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_SET_FLAG,
 				      QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN,
 				      index, NULL);
@@ -5283,7 +5283,7 @@ static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba)
 	if (!ufshcd_is_wb_allowed(hba) || !hba->wb_buf_flush_enabled)
 		return 0;
 
-	index = ufshcd_wb_get_flag_index(hba);
+	index = ufshcd_wb_get_query_index(hba);
 	ret = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_CLEAR_FLAG,
 				      QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN,
 				      index, NULL);
@@ -5303,10 +5303,12 @@ static bool ufshcd_wb_presrv_usrspc_keep_vcc_on(struct ufs_hba *hba,
 {
 	u32 cur_buf;
 	int ret;
+	u8 index;
 
+	index = ufshcd_wb_get_query_index(hba);
 	ret = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
 					      QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE,
-					      0, 0, &cur_buf);
+					      index, 0, &cur_buf);
 	if (ret) {
 		dev_err(hba->dev, "%s dCurWriteBoosterBufferSize read failed %d\n",
 			__func__, ret);
@@ -5329,6 +5331,7 @@ static bool ufshcd_wb_keep_vcc_on(struct ufs_hba *hba)
 {
 	int ret;
 	u32 avail_buf;
+	u8 index;
 
 	if (!ufshcd_is_wb_allowed(hba))
 		return false;
@@ -5343,9 +5346,10 @@ static bool ufshcd_wb_keep_vcc_on(struct ufs_hba *hba)
 	 * buffer (dCurrentWriteBoosterBufferSize). There's no point in
 	 * keeping vcc on when current buffer is empty.
 	 */
+	index = ufshcd_wb_get_query_index(hba);
 	ret = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
 				      QUERY_ATTR_IDN_AVAIL_WB_BUFF_SIZE,
-				      0, 0, &avail_buf);
+				      index, 0, &avail_buf);
 	if (ret) {
 		dev_warn(hba->dev, "%s dAvailableWriteBoosterBufferSize read failed %d\n",
 			 __func__, ret);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 23a434c03c2a..ab0d180dad6c 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -861,7 +861,7 @@ static inline bool ufshcd_keep_autobkops_enabled_except_suspend(
 	return hba->caps & UFSHCD_CAP_KEEP_AUTO_BKOPS_ENABLED_EXCEPT_SUSPEND;
 }
 
-static inline u8 ufshcd_wb_get_flag_index(struct ufs_hba *hba)
+static inline u8 ufshcd_wb_get_query_index(struct ufs_hba *hba)
 {
 	if (hba->dev_info.b_wb_buffer_type == WB_BUF_MODE_LU_DEDICATED)
 		return hba->dev_info.wb_dedicated_lu;
-- 
2.18.0

[PATCH v1 4/4] scsi: ufs: Fix WriteBooster flush during runtime suspend

[Stanley Chu]

Currently UFS host driver promises VCC supply if UFS device
needs to do WriteBooster flush during runtime suspend.

However the UFS specification mentions,

"While the flushing operation is in progress, the device is
in Active power mode."

Therefore UFS host driver needs to promise more: Keep UFS
device as "Active power mode", otherwise UFS device shall not
do any flush if device enters Sleep or PowerDown power mode.

Fix this by not changing device power mode if WriteBooster
flush is required in ufshcd_suspend().

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufs.h    |  1 -
 drivers/scsi/ufs/ufshcd.c | 39 +++++++++++++++++++--------------------
 2 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index b3135344ab3f..9e4bc2e97ada 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -577,7 +577,6 @@ struct ufs_dev_info {
 	u32 d_ext_ufs_feature_sup;
 	u8 b_wb_buffer_type;
 	u32 d_wb_alloc_units;
-	bool keep_vcc_on;
 	u8 b_presrv_uspc_en;
 };
 
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 169a3379e468..2d0aff8ac260 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -8101,8 +8101,7 @@ static void ufshcd_vreg_set_lpm(struct ufs_hba *hba)
 	    !hba->dev_info.is_lu_power_on_wp) {
 		ufshcd_setup_vreg(hba, false);
 	} else if (!ufshcd_is_ufs_dev_active(hba)) {
-		if (!hba->dev_info.keep_vcc_on)
-			ufshcd_toggle_vreg(hba->dev, hba->vreg_info.vcc, false);
+		ufshcd_toggle_vreg(hba->dev, hba->vreg_info.vcc, false);
 		if (!ufshcd_is_link_active(hba)) {
 			ufshcd_config_vreg_lpm(hba, hba->vreg_info.vccq);
 			ufshcd_config_vreg_lpm(hba, hba->vreg_info.vccq2);
@@ -8172,6 +8171,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 	enum ufs_pm_level pm_lvl;
 	enum ufs_dev_pwr_mode req_dev_pwr_mode;
 	enum uic_link_state req_link_state;
+	bool keep_curr_dev_pwr_mode = false;
 
 	hba->pm_op_in_progress = 1;
 	if (!ufshcd_is_shutdown_pm(pm_op)) {
@@ -8226,28 +8226,27 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 			/* make sure that auto bkops is disabled */
 			ufshcd_disable_auto_bkops(hba);
 		}
+
 		/*
-		 * With wb enabled, if the bkops is enabled or if the
-		 * configured WB type is 70% full, keep vcc ON
-		 * for the device to flush the wb buffer
+		 * If device needs to do BKOP or WB buffer flush, keep device
+		 * power mode as "active power mode" and its VCC supply.
 		 */
-		if ((hba->auto_bkops_enabled && ufshcd_is_wb_allowed(hba)) ||
-		    ufshcd_wb_keep_vcc_on(hba))
-			hba->dev_info.keep_vcc_on = true;
-		else
-			hba->dev_info.keep_vcc_on = false;
-	} else {
-		hba->dev_info.keep_vcc_on = false;
+		keep_curr_dev_pwr_mode = hba->auto_bkops_enabled ||
+			ufshcd_wb_keep_vcc_on(hba);
 	}
 
-	if ((req_dev_pwr_mode != hba->curr_dev_pwr_mode) &&
-	    ((ufshcd_is_runtime_pm(pm_op) && !hba->auto_bkops_enabled) ||
-	    !ufshcd_is_runtime_pm(pm_op))) {
-		/* ensure that bkops is disabled */
-		ufshcd_disable_auto_bkops(hba);
-		ret = ufshcd_set_dev_pwr_mode(hba, req_dev_pwr_mode);
-		if (ret)
-			goto enable_gating;
+	if (req_dev_pwr_mode != hba->curr_dev_pwr_mode) {
+		if ((ufshcd_is_runtime_pm(pm_op) && !hba->auto_bkops_enabled) ||
+		    !ufshcd_is_runtime_pm(pm_op)) {
+			/* ensure that bkops is disabled */
+			ufshcd_disable_auto_bkops(hba);
+		}
+
+		if (!keep_curr_dev_pwr_mode) {
+			ret = ufshcd_set_dev_pwr_mode(hba, req_dev_pwr_mode);
+			if (ret)
+				goto enable_gating;
+		}
 	}
 
 	flush_work(&hba->eeh_work);
-- 
2.18.0

Re: [PATCH v1 4/4] scsi: ufs: Fix WriteBooster flush during runtime suspend

[Asutosh Das (asd)]

Hi Stanley,

On 5/12/2020 3:47 AM, Stanley Chu wrote:
> Currently UFS host driver promises VCC supply if UFS device
> needs to do WriteBooster flush during runtime suspend.
> 
> However the UFS specification mentions,
> 
> "While the flushing operation is in progress, the device is
> in Active power mode."
> 
> Therefore UFS host driver needs to promise more: Keep UFS
> device as "Active power mode", otherwise UFS device shall not
> do any flush if device enters Sleep or PowerDown power mode.
> 
> Fix this by not changing device power mode if WriteBooster
> flush is required in ufshcd_suspend().
> 
> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
> ---
>   drivers/scsi/ufs/ufs.h    |  1 -
>   drivers/scsi/ufs/ufshcd.c | 39 +++++++++++++++++++--------------------
>   2 files changed, 19 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index b3135344ab3f..9e4bc2e97ada 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -577,7 +577,6 @@ struct ufs_dev_info {
>   	u32 d_ext_ufs_feature_sup;
>   	u8 b_wb_buffer_type;
>   	u32 d_wb_alloc_units;
> -	bool keep_vcc_on;
>   	u8 b_presrv_uspc_en;
>   };
>   
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 169a3379e468..2d0aff8ac260 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -8101,8 +8101,7 @@ static void ufshcd_vreg_set_lpm(struct ufs_hba *hba)
>   	    !hba->dev_info.is_lu_power_on_wp) {
>   		ufshcd_setup_vreg(hba, false);
>   	} else if (!ufshcd_is_ufs_dev_active(hba)) {
> -		if (!hba->dev_info.keep_vcc_on)
> -			ufshcd_toggle_vreg(hba->dev, hba->vreg_info.vcc, false);
> +		ufshcd_toggle_vreg(hba->dev, hba->vreg_info.vcc, false);
>   		if (!ufshcd_is_link_active(hba)) {
>   			ufshcd_config_vreg_lpm(hba, hba->vreg_info.vccq);
>   			ufshcd_config_vreg_lpm(hba, hba->vreg_info.vccq2);
> @@ -8172,6 +8171,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>   	enum ufs_pm_level pm_lvl;
>   	enum ufs_dev_pwr_mode req_dev_pwr_mode;
>   	enum uic_link_state req_link_state;
> +	bool keep_curr_dev_pwr_mode = false;
>   
>   	hba->pm_op_in_progress = 1;
>   	if (!ufshcd_is_shutdown_pm(pm_op)) {
> @@ -8226,28 +8226,27 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>   			/* make sure that auto bkops is disabled */
>   			ufshcd_disable_auto_bkops(hba);
>   		}
> +
>   		/*
> -		 * With wb enabled, if the bkops is enabled or if the
> -		 * configured WB type is 70% full, keep vcc ON
> -		 * for the device to flush the wb buffer
> +		 * If device needs to do BKOP or WB buffer flush, keep device
> +		 * power mode as "active power mode" and its VCC supply.
>   		 */
> -		if ((hba->auto_bkops_enabled && ufshcd_is_wb_allowed(hba)) ||
> -		    ufshcd_wb_keep_vcc_on(hba))
> -			hba->dev_info.keep_vcc_on = true;
> -		else
> -			hba->dev_info.keep_vcc_on = false;
> -	} else {
> -		hba->dev_info.keep_vcc_on = false;
> +		keep_curr_dev_pwr_mode = hba->auto_bkops_enabled ||
> +			ufshcd_wb_keep_vcc_on(hba);
>   	}
>   
> -	if ((req_dev_pwr_mode != hba->curr_dev_pwr_mode) &&
> -	    ((ufshcd_is_runtime_pm(pm_op) && !hba->auto_bkops_enabled) ||
> -	    !ufshcd_is_runtime_pm(pm_op))) {
> -		/* ensure that bkops is disabled */
> -		ufshcd_disable_auto_bkops(hba);
> -		ret = ufshcd_set_dev_pwr_mode(hba, req_dev_pwr_mode);
> -		if (ret)
> -			goto enable_gating;
> +	if (req_dev_pwr_mode != hba->curr_dev_pwr_mode) {
> +		if ((ufshcd_is_runtime_pm(pm_op) && !hba->auto_bkops_enabled) ||
> +		    !ufshcd_is_runtime_pm(pm_op)) {
> +			/* ensure that bkops is disabled */
> +			ufshcd_disable_auto_bkops(hba);
> +		}
> +
> +		if (!keep_curr_dev_pwr_mode) {
> +			ret = ufshcd_set_dev_pwr_mode(hba, req_dev_pwr_mode);
> +			if (ret)
> +				goto enable_gating;
> +		}
>   	}
>   
>   	flush_work(&hba->eeh_work);
> 

Can you please confirm that you've tested and found that with the 
previous code, the flush operation in the device was not happening.

If so, please can you let me know the test-case that you ran to figure 
this out.

I'd like to verify this at my end.

--
Thanks,
-asd

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

Re: [PATCH v1 4/4] scsi: ufs: Fix WriteBooster flush during runtime suspend

[Stanley Chu]

Hi Asutosh,

On Tue, 2020-05-12 at 10:04 -0700, Asutosh Das (asd) wrote:
> Hi Stanley,
> 
> On 5/12/2020 3:47 AM, Stanley Chu wrote:
> > Currently UFS host driver promises VCC supply if UFS device
> > needs to do WriteBooster flush during runtime suspend.
> > 
> > However the UFS specification mentions,
> > 
> > "While the flushing operation is in progress, the device is
> > in Active power mode."
> > 
> > Therefore UFS host driver needs to promise more: Keep UFS
> > device as "Active power mode", otherwise UFS device shall not
> > do any flush if device enters Sleep or PowerDown power mode.
> > 
> > Fix this by not changing device power mode if WriteBooster
> > flush is required in ufshcd_suspend().
> > 
> > Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
> > ---
> >   drivers/scsi/ufs/ufs.h    |  1 -
> >   drivers/scsi/ufs/ufshcd.c | 39 +++++++++++++++++++--------------------
> >   2 files changed, 19 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> > index b3135344ab3f..9e4bc2e97ada 100644
> > --- a/drivers/scsi/ufs/ufs.h
> > +++ b/drivers/scsi/ufs/ufs.h
> > @@ -577,7 +577,6 @@ struct ufs_dev_info {
> >   	u32 d_ext_ufs_feature_sup;
> >   	u8 b_wb_buffer_type;
> >   	u32 d_wb_alloc_units;
> > -	bool keep_vcc_on;
> >   	u8 b_presrv_uspc_en;
> >   };
> >   
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 169a3379e468..2d0aff8ac260 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -8101,8 +8101,7 @@ static void ufshcd_vreg_set_lpm(struct ufs_hba *hba)
> >   	    !hba->dev_info.is_lu_power_on_wp) {
> >   		ufshcd_setup_vreg(hba, false);
> >   	} else if (!ufshcd_is_ufs_dev_active(hba)) {
> > -		if (!hba->dev_info.keep_vcc_on)
> > -			ufshcd_toggle_vreg(hba->dev, hba->vreg_info.vcc, false);
> > +		ufshcd_toggle_vreg(hba->dev, hba->vreg_info.vcc, false);
> >   		if (!ufshcd_is_link_active(hba)) {
> >   			ufshcd_config_vreg_lpm(hba, hba->vreg_info.vccq);
> >   			ufshcd_config_vreg_lpm(hba, hba->vreg_info.vccq2);
> > @@ -8172,6 +8171,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
> >   	enum ufs_pm_level pm_lvl;
> >   	enum ufs_dev_pwr_mode req_dev_pwr_mode;
> >   	enum uic_link_state req_link_state;
> > +	bool keep_curr_dev_pwr_mode = false;
> >   
> >   	hba->pm_op_in_progress = 1;
> >   	if (!ufshcd_is_shutdown_pm(pm_op)) {
> > @@ -8226,28 +8226,27 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
> >   			/* make sure that auto bkops is disabled */
> >   			ufshcd_disable_auto_bkops(hba);
> >   		}
> > +
> >   		/*
> > -		 * With wb enabled, if the bkops is enabled or if the
> > -		 * configured WB type is 70% full, keep vcc ON
> > -		 * for the device to flush the wb buffer
> > +		 * If device needs to do BKOP or WB buffer flush, keep device
> > +		 * power mode as "active power mode" and its VCC supply.
> >   		 */
> > -		if ((hba->auto_bkops_enabled && ufshcd_is_wb_allowed(hba)) ||
> > -		    ufshcd_wb_keep_vcc_on(hba))
> > -			hba->dev_info.keep_vcc_on = true;
> > -		else
> > -			hba->dev_info.keep_vcc_on = false;
> > -	} else {
> > -		hba->dev_info.keep_vcc_on = false;
> > +		keep_curr_dev_pwr_mode = hba->auto_bkops_enabled ||
> > +			ufshcd_wb_keep_vcc_on(hba);
> >   	}
> >   
> > -	if ((req_dev_pwr_mode != hba->curr_dev_pwr_mode) &&
> > -	    ((ufshcd_is_runtime_pm(pm_op) && !hba->auto_bkops_enabled) ||
> > -	    !ufshcd_is_runtime_pm(pm_op))) {
> > -		/* ensure that bkops is disabled */
> > -		ufshcd_disable_auto_bkops(hba);
> > -		ret = ufshcd_set_dev_pwr_mode(hba, req_dev_pwr_mode);
> > -		if (ret)
> > -			goto enable_gating;
> > +	if (req_dev_pwr_mode != hba->curr_dev_pwr_mode) {
> > +		if ((ufshcd_is_runtime_pm(pm_op) && !hba->auto_bkops_enabled) ||
> > +		    !ufshcd_is_runtime_pm(pm_op)) {
> > +			/* ensure that bkops is disabled */
> > +			ufshcd_disable_auto_bkops(hba);
> > +		}
> > +
> > +		if (!keep_curr_dev_pwr_mode) {
> > +			ret = ufshcd_set_dev_pwr_mode(hba, req_dev_pwr_mode);
> > +			if (ret)
> > +				goto enable_gating;
> > +		}
> >   	}
> >   
> >   	flush_work(&hba->eeh_work);
> > 
> 
> Can you please confirm that you've tested and found that with the 
> previous code, the flush operation in the device was not happening.
> 
> If so, please can you let me know the test-case that you ran to figure 
> this out.
> 
> I'd like to verify this at my end.

Sorry currently I have no easy test cases or scripts available.

To precisely confirm the flush behavior by logs, I added some codes to
query "available WriteBooster buffer" before entering runtime suspend
and after leaving runtime resume, and observe the trend of available
WriteBooster buffer.

My test steps are as below,

1. Create a writer to write large data in a short time to fill-in
WriteBooster buffer.

2. Do something to prevent system suspend

3. Do something to prevent link entering Hibern8, for example, disable
Auto-Hibern8 and disable Hibern8 during clock gating. Because the
Hibern8 period before runtime-suspend is known that VCC is provided and
device can flush WriteBooster buffer if "Flush During H8" is enabled as
upstream kernel currently.

4. Shrink the runtime suspend delay (maybe 100ms ~ 200ms) to make
runtime suspend happen earlier.

5. After "available WriteBosoter buffer" reaches lower-level, for
example, 10%, stop the writer.

6. Observe the trend of WriteBooster available buffer.


In the previous code, the available WriteBooster buffer is increased
very very slowly. Especially no increasing is observed during
runtime-suspend.

After applying this fix, the available WriteBooster buffer is increased
much faster and the increasing can be easily observed during
runtime-suspend.

Thanks,
Stanley Chu

> 
> --
> Thanks,
> -asd
> 

Re: [PATCH v1 4/4] scsi: ufs: Fix WriteBooster flush during runtime suspend

[Asutosh Das (asd)]

On 5/12/2020 3:47 AM, Stanley Chu wrote:
> Currently UFS host driver promises VCC supply if UFS device
> needs to do WriteBooster flush during runtime suspend.
> 
> However the UFS specification mentions,
> 
> "While the flushing operation is in progress, the device is
> in Active power mode."
> 
> Therefore UFS host driver needs to promise more: Keep UFS
> device as "Active power mode", otherwise UFS device shall not
> do any flush if device enters Sleep or PowerDown power mode.
> 
> Fix this by not changing device power mode if WriteBooster
> flush is required in ufshcd_suspend().
> 
> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
> ---
>   drivers/scsi/ufs/ufs.h    |  1 -
>   drivers/scsi/ufs/ufshcd.c | 39 +++++++++++++++++++--------------------
>   2 files changed, 19 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index b3135344ab3f..9e4bc2e97ada 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -577,7 +577,6 @@ struct ufs_dev_info {
>   	u32 d_ext_ufs_feature_sup;
>   	u8 b_wb_buffer_type;
>   	u32 d_wb_alloc_units;
> -	bool keep_vcc_on;
>   	u8 b_presrv_uspc_en;
>   };
>   
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 169a3379e468..2d0aff8ac260 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -8101,8 +8101,7 @@ static void ufshcd_vreg_set_lpm(struct ufs_hba *hba)
>   	    !hba->dev_info.is_lu_power_on_wp) {
>   		ufshcd_setup_vreg(hba, false);
>   	} else if (!ufshcd_is_ufs_dev_active(hba)) {
> -		if (!hba->dev_info.keep_vcc_on)
> -			ufshcd_toggle_vreg(hba->dev, hba->vreg_info.vcc, false);
> +		ufshcd_toggle_vreg(hba->dev, hba->vreg_info.vcc, false);
>   		if (!ufshcd_is_link_active(hba)) {
>   			ufshcd_config_vreg_lpm(hba, hba->vreg_info.vccq);
>   			ufshcd_config_vreg_lpm(hba, hba->vreg_info.vccq2);
> @@ -8172,6 +8171,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>   	enum ufs_pm_level pm_lvl;
>   	enum ufs_dev_pwr_mode req_dev_pwr_mode;
>   	enum uic_link_state req_link_state;
> +	bool keep_curr_dev_pwr_mode = false;
>   
>   	hba->pm_op_in_progress = 1;
>   	if (!ufshcd_is_shutdown_pm(pm_op)) {
> @@ -8226,28 +8226,27 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>   			/* make sure that auto bkops is disabled */
>   			ufshcd_disable_auto_bkops(hba);
>   		}
> +
Unnecessary newline, perhaps?
>   		/*
> -		 * With wb enabled, if the bkops is enabled or if the
> -		 * configured WB type is 70% full, keep vcc ON
> -		 * for the device to flush the wb buffer
> +		 * If device needs to do BKOP or WB buffer flush, keep device
> +		 * power mode as "active power mode" and its VCC supply.
>   		 */
> -		if ((hba->auto_bkops_enabled && ufshcd_is_wb_allowed(hba)) ||
> -		    ufshcd_wb_keep_vcc_on(hba))
> -			hba->dev_info.keep_vcc_on = true;
> -		else
> -			hba->dev_info.keep_vcc_on = false;
> -	} else {
> -		hba->dev_info.keep_vcc_on = false;
> +		keep_curr_dev_pwr_mode = hba->auto_bkops_enabled ||
> +			ufshcd_wb_keep_vcc_on(hba);
Should the device be in UFS_ACTIVE_PWR_MODE to perform auto-bkops?

Also, is it needed to keep the device in UFS_ACTIVE_PWR_MODE , if flush 
on hibern8 is enabled and the link is being put to hibern8 mode during 
runtime-suspend? Perhaps that should also be factored in here?
>   	}
>   
> -	if ((req_dev_pwr_mode != hba->curr_dev_pwr_mode) &&
> -	    ((ufshcd_is_runtime_pm(pm_op) && !hba->auto_bkops_enabled) ||
> -	    !ufshcd_is_runtime_pm(pm_op))) {
> -		/* ensure that bkops is disabled */
> -		ufshcd_disable_auto_bkops(hba);
> -		ret = ufshcd_set_dev_pwr_mode(hba, req_dev_pwr_mode);
> -		if (ret)
> -			goto enable_gating;
> +	if (req_dev_pwr_mode != hba->curr_dev_pwr_mode) {
> +		if ((ufshcd_is_runtime_pm(pm_op) && !hba->auto_bkops_enabled) ||
> +		    !ufshcd_is_runtime_pm(pm_op)) {
> +			/* ensure that bkops is disabled */
> +			ufshcd_disable_auto_bkops(hba);
> +		}
> +
> +		if (!keep_curr_dev_pwr_mode) {
> +			ret = ufshcd_set_dev_pwr_mode(hba, req_dev_pwr_mode);

Now, when the WB buffer is completely flushed out, the device should be 
put back into UFS_SLEEP_PWR_MODE or UFS_POWERDOWN_PWR_MODE. Say, the 
device buffer has to be flushed and during runtime-suspend, the device 
is put to UFS_ACTIVE_PWR_MODE and Vcc is kept ON; the device doesn't 
resume nor does the system enters suspend for a very long time, and with 
AH8 and hibern8 disabled, there will be an unnecessary power drain for 
that much time.

How about a periodic interval checking of flush status if 
keep_curr_dev_pwr_mode evaluates to be true?

> +			if (ret)
> +				goto enable_gating;
> +		}
>   	}
>   
>   	flush_work(&hba->eeh_work);
> 



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

Re: [PATCH v1 4/4] scsi: ufs: Fix WriteBooster flush during runtime suspend

[Stanley Chu]

Hi Asutosh,

On Wed, 2020-05-13 at 12:31 -0700, Asutosh Das (asd) wrote:
> On 5/12/2020 3:47 AM, Stanley Chu wrote:
> > Currently UFS host driver promises VCC supply if UFS device
> > needs to do WriteBooster flush during runtime suspend.
> > 
> > However the UFS specification mentions,
> > 
> > "While the flushing operation is in progress, the device is
> > in Active power mode."
> > 
> > Therefore UFS host driver needs to promise more: Keep UFS
> > device as "Active power mode", otherwise UFS device shall not
> > do any flush if device enters Sleep or PowerDown power mode.
> > 
> > Fix this by not changing device power mode if WriteBooster
> > flush is required in ufshcd_suspend().
> > 
> > Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
> > ---
> >   drivers/scsi/ufs/ufs.h    |  1 -
> >   drivers/scsi/ufs/ufshcd.c | 39 +++++++++++++++++++--------------------
> >   2 files changed, 19 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> > index b3135344ab3f..9e4bc2e97ada 100644
> > --- a/drivers/scsi/ufs/ufs.h
> > +++ b/drivers/scsi/ufs/ufs.h
> > @@ -577,7 +577,6 @@ struct ufs_dev_info {
> >   	u32 d_ext_ufs_feature_sup;
> >   	u8 b_wb_buffer_type;
> >   	u32 d_wb_alloc_units;
> > -	bool keep_vcc_on;
> >   	u8 b_presrv_uspc_en;
> >   };
> >   
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 169a3379e468..2d0aff8ac260 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -8101,8 +8101,7 @@ static void ufshcd_vreg_set_lpm(struct ufs_hba *hba)
> >   	    !hba->dev_info.is_lu_power_on_wp) {
> >   		ufshcd_setup_vreg(hba, false);
> >   	} else if (!ufshcd_is_ufs_dev_active(hba)) {
> > -		if (!hba->dev_info.keep_vcc_on)
> > -			ufshcd_toggle_vreg(hba->dev, hba->vreg_info.vcc, false);
> > +		ufshcd_toggle_vreg(hba->dev, hba->vreg_info.vcc, false);
> >   		if (!ufshcd_is_link_active(hba)) {
> >   			ufshcd_config_vreg_lpm(hba, hba->vreg_info.vccq);
> >   			ufshcd_config_vreg_lpm(hba, hba->vreg_info.vccq2);
> > @@ -8172,6 +8171,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
> >   	enum ufs_pm_level pm_lvl;
> >   	enum ufs_dev_pwr_mode req_dev_pwr_mode;
> >   	enum uic_link_state req_link_state;
> > +	bool keep_curr_dev_pwr_mode = false;
> >   
> >   	hba->pm_op_in_progress = 1;
> >   	if (!ufshcd_is_shutdown_pm(pm_op)) {
> > @@ -8226,28 +8226,27 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
> >   			/* make sure that auto bkops is disabled */
> >   			ufshcd_disable_auto_bkops(hba);
> >   		}
> > +
> Unnecessary newline, perhaps?

Yap, I will remove it in next version.

> >   		/*
> > -		 * With wb enabled, if the bkops is enabled or if the
> > -		 * configured WB type is 70% full, keep vcc ON
> > -		 * for the device to flush the wb buffer
> > +		 * If device needs to do BKOP or WB buffer flush, keep device
> > +		 * power mode as "active power mode" and its VCC supply.
> >   		 */
> > -		if ((hba->auto_bkops_enabled && ufshcd_is_wb_allowed(hba)) ||
> > -		    ufshcd_wb_keep_vcc_on(hba))
> > -			hba->dev_info.keep_vcc_on = true;
> > -		else
> > -			hba->dev_info.keep_vcc_on = false;
> > -	} else {
> > -		hba->dev_info.keep_vcc_on = false;
> > +		keep_curr_dev_pwr_mode = hba->auto_bkops_enabled ||
> > +			ufshcd_wb_keep_vcc_on(hba);
> Should the device be in UFS_ACTIVE_PWR_MODE to perform auto-bkops?
> 
> Also, is it needed to keep the device in UFS_ACTIVE_PWR_MODE , if flush 
> on hibern8 is enabled and the link is being put to hibern8 mode during 
> runtime-suspend? Perhaps that should also be factored in here?

Both auto-bkops and WriteBooster flush during Hibern8 need device power
mode to be "Active Power Mode".

For auto-bkops, the spec mentions,

"If the background operations enable bit is set and the device is in
Active power mode or Idle power mode, then the device is allowed to
execute any internal operations."

For WriteBooster flush during Hibern8, the spec mentions,

"While the flushing operation is in progress, the device is in Active
power mode."

Therefore here we can use an unified "keep_curr_dev_pwr_mode" to
indicate the same requirements of above both features.

Besides, both operations may access flash array inside UFS device thus
VCC supply shall be also kept.

Before this patch, the original code will keep device power mode (stay
in Active Power Mode) if hba->auto_bkops_enabled is set as true during
runtime-suspend with UFSHCD_CAP_AUTO_BKOPS_SUSPEND capability is
enabled. This patch will not change this decision, just add
"WriteBooster flush during Hibern8" feature as another condition to do
so.

Thank you so much to remind me that "Link shall be put in Hibern8" is a
necessary condition for "WriteBooster flush during Hibern8". I will add
more checking for keep_curr_dev_pwr_mode to prevent unnecessary power
drain.  

> >   	}
> >   
> > -	if ((req_dev_pwr_mode != hba->curr_dev_pwr_mode) &&
> > -	    ((ufshcd_is_runtime_pm(pm_op) && !hba->auto_bkops_enabled) ||
> > -	    !ufshcd_is_runtime_pm(pm_op))) {
> > -		/* ensure that bkops is disabled */
> > -		ufshcd_disable_auto_bkops(hba);
> > -		ret = ufshcd_set_dev_pwr_mode(hba, req_dev_pwr_mode);
> > -		if (ret)
> > -			goto enable_gating;
> > +	if (req_dev_pwr_mode != hba->curr_dev_pwr_mode) {
> > +		if ((ufshcd_is_runtime_pm(pm_op) && !hba->auto_bkops_enabled) ||
> > +		    !ufshcd_is_runtime_pm(pm_op)) {
> > +			/* ensure that bkops is disabled */
> > +			ufshcd_disable_auto_bkops(hba);
> > +		}
> > +
> > +		if (!keep_curr_dev_pwr_mode) {
> > +			ret = ufshcd_set_dev_pwr_mode(hba, req_dev_pwr_mode);
> 
> Now, when the WB buffer is completely flushed out, the device should be 
> put back into UFS_SLEEP_PWR_MODE or UFS_POWERDOWN_PWR_MODE. Say, the 
> device buffer has to be flushed and during runtime-suspend, the device 
> is put to UFS_ACTIVE_PWR_MODE and Vcc is kept ON; the device doesn't 
> resume nor does the system enters suspend for a very long time, and with 
> AH8 and hibern8 disabled, there will be an unnecessary power drain for 
> that much time.
> 
> How about a periodic interval checking of flush status if 
> keep_curr_dev_pwr_mode evaluates to be true?

This is a good point!

The same thing also happens for auto-bkops. How about add a timer to
leave runtime suspend if keep_curr_dev_pwr_mode is set as true? This is
simple and also favors power. The timeout value could be adjustable
according to the available WriteBooster buffer size.

A periodic interval checking of flush status needs to re-activate link
to communicate with the device. This would be tricky and the
re-activation flow is just like runtime-resume.

What would you think?

Thanks.
Stanley Chu

Re: [PATCH v1 4/4] scsi: ufs: Fix WriteBooster flush during runtime suspend

[Stanley Chu]

Hi Asutosh,

On Thu, 2020-05-14 at 10:23 +0800, Stanley Chu wrote:
> Hi Asutosh,
> 
> On Wed, 2020-05-13 at 12:31 -0700, Asutosh Das (asd) wrote:
> > On 5/12/2020 3:47 AM, Stanley Chu wrote:
> > > Currently UFS host driver promises VCC supply if UFS device
> > > needs to do WriteBooster flush during runtime suspend.
> > > 
> > > However the UFS specification mentions,
> > > 
> > > "While the flushing operation is in progress, the device is
> > > in Active power mode."
> > > 
> > > Therefore UFS host driver needs to promise more: Keep UFS
> > > device as "Active power mode", otherwise UFS device shall not
> > > do any flush if device enters Sleep or PowerDown power mode.
> > > 
> > > Fix this by not changing device power mode if WriteBooster
> > > flush is required in ufshcd_suspend().
> > > 
> > > Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
> > > ---
> > >   drivers/scsi/ufs/ufs.h    |  1 -
> > >   drivers/scsi/ufs/ufshcd.c | 39 +++++++++++++++++++--------------------
> > >   2 files changed, 19 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> > > index b3135344ab3f..9e4bc2e97ada 100644
> > > --- a/drivers/scsi/ufs/ufs.h
> > > +++ b/drivers/scsi/ufs/ufs.h
> > > @@ -577,7 +577,6 @@ struct ufs_dev_info {
> > >   	u32 d_ext_ufs_feature_sup;
> > >   	u8 b_wb_buffer_type;
> > >   	u32 d_wb_alloc_units;
> > > -	bool keep_vcc_on;
> > >   	u8 b_presrv_uspc_en;
> > >   };
> > >   
> > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > > index 169a3379e468..2d0aff8ac260 100644
> > > --- a/drivers/scsi/ufs/ufshcd.c
> > > +++ b/drivers/scsi/ufs/ufshcd.c
> > > @@ -8101,8 +8101,7 @@ static void ufshcd_vreg_set_lpm(struct ufs_hba *hba)
> > >   	    !hba->dev_info.is_lu_power_on_wp) {
> > >   		ufshcd_setup_vreg(hba, false);
> > >   	} else if (!ufshcd_is_ufs_dev_active(hba)) {
> > > -		if (!hba->dev_info.keep_vcc_on)
> > > -			ufshcd_toggle_vreg(hba->dev, hba->vreg_info.vcc, false);
> > > +		ufshcd_toggle_vreg(hba->dev, hba->vreg_info.vcc, false);
> > >   		if (!ufshcd_is_link_active(hba)) {
> > >   			ufshcd_config_vreg_lpm(hba, hba->vreg_info.vccq);
> > >   			ufshcd_config_vreg_lpm(hba, hba->vreg_info.vccq2);
> > > @@ -8172,6 +8171,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
> > >   	enum ufs_pm_level pm_lvl;
> > >   	enum ufs_dev_pwr_mode req_dev_pwr_mode;
> > >   	enum uic_link_state req_link_state;
> > > +	bool keep_curr_dev_pwr_mode = false;
> > >   
> > >   	hba->pm_op_in_progress = 1;
> > >   	if (!ufshcd_is_shutdown_pm(pm_op)) {
> > > @@ -8226,28 +8226,27 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
> > >   			/* make sure that auto bkops is disabled */
> > >   			ufshcd_disable_auto_bkops(hba);
> > >   		}
> > > +
> > Unnecessary newline, perhaps?
> 
> Yap, I will remove it in next version.
> 
> > >   		/*
> > > -		 * With wb enabled, if the bkops is enabled or if the
> > > -		 * configured WB type is 70% full, keep vcc ON
> > > -		 * for the device to flush the wb buffer
> > > +		 * If device needs to do BKOP or WB buffer flush, keep device
> > > +		 * power mode as "active power mode" and its VCC supply.
> > >   		 */
> > > -		if ((hba->auto_bkops_enabled && ufshcd_is_wb_allowed(hba)) ||
> > > -		    ufshcd_wb_keep_vcc_on(hba))
> > > -			hba->dev_info.keep_vcc_on = true;
> > > -		else
> > > -			hba->dev_info.keep_vcc_on = false;
> > > -	} else {
> > > -		hba->dev_info.keep_vcc_on = false;
> > > +		keep_curr_dev_pwr_mode = hba->auto_bkops_enabled ||
> > > +			ufshcd_wb_keep_vcc_on(hba);
> > Should the device be in UFS_ACTIVE_PWR_MODE to perform auto-bkops?
> > 
> > Also, is it needed to keep the device in UFS_ACTIVE_PWR_MODE , if flush 
> > on hibern8 is enabled and the link is being put to hibern8 mode during 
> > runtime-suspend? Perhaps that should also be factored in here?
> 
> Both auto-bkops and WriteBooster flush during Hibern8 need device power
> mode to be "Active Power Mode".
> 
> For auto-bkops, the spec mentions,
> 
> "If the background operations enable bit is set and the device is in
> Active power mode or Idle power mode, then the device is allowed to
> execute any internal operations."
> 
> For WriteBooster flush during Hibern8, the spec mentions,
> 
> "While the flushing operation is in progress, the device is in Active
> power mode."
> 
> Therefore here we can use an unified "keep_curr_dev_pwr_mode" to
> indicate the same requirements of above both features.
> 
> Besides, both operations may access flash array inside UFS device thus
> VCC supply shall be also kept.
> 
> Before this patch, the original code will keep device power mode (stay
> in Active Power Mode) if hba->auto_bkops_enabled is set as true during
> runtime-suspend with UFSHCD_CAP_AUTO_BKOPS_SUSPEND capability is
> enabled. This patch will not change this decision, just add
> "WriteBooster flush during Hibern8" feature as another condition to do
> so.
> 
> Thank you so much to remind me that "Link shall be put in Hibern8" is a
> necessary condition for "WriteBooster flush during Hibern8". I will add
> more checking for keep_curr_dev_pwr_mode to prevent unnecessary power
> drain.  
> 
> > >   	}
> > >   
> > > -	if ((req_dev_pwr_mode != hba->curr_dev_pwr_mode) &&
> > > -	    ((ufshcd_is_runtime_pm(pm_op) && !hba->auto_bkops_enabled) ||
> > > -	    !ufshcd_is_runtime_pm(pm_op))) {
> > > -		/* ensure that bkops is disabled */
> > > -		ufshcd_disable_auto_bkops(hba);
> > > -		ret = ufshcd_set_dev_pwr_mode(hba, req_dev_pwr_mode);
> > > -		if (ret)
> > > -			goto enable_gating;
> > > +	if (req_dev_pwr_mode != hba->curr_dev_pwr_mode) {
> > > +		if ((ufshcd_is_runtime_pm(pm_op) && !hba->auto_bkops_enabled) ||
> > > +		    !ufshcd_is_runtime_pm(pm_op)) {
> > > +			/* ensure that bkops is disabled */
> > > +			ufshcd_disable_auto_bkops(hba);
> > > +		}
> > > +
> > > +		if (!keep_curr_dev_pwr_mode) {
> > > +			ret = ufshcd_set_dev_pwr_mode(hba, req_dev_pwr_mode);
> > 
> > Now, when the WB buffer is completely flushed out, the device should be 
> > put back into UFS_SLEEP_PWR_MODE or UFS_POWERDOWN_PWR_MODE. Say, the 
> > device buffer has to be flushed and during runtime-suspend, the device 
> > is put to UFS_ACTIVE_PWR_MODE and Vcc is kept ON; the device doesn't 
> > resume nor does the system enters suspend for a very long time, and with 
> > AH8 and hibern8 disabled, there will be an unnecessary power drain for 
> > that much time.

Another thought is that if keep_curr_dev_pwr_mode will be set as true
only if link is put in Hibern8 or Auto-Hibern8 is enabled. By this way,
the power consumption shall be very small after flush or auto-bkop is
finished.

Then the checking of flush status during runtime-suspend may be not
necessary.

> > 
> > How about a periodic interval checking of flush status if 
> > keep_curr_dev_pwr_mode evaluates to be true?
> 
> This is a good point!
> 
> The same thing also happens for auto-bkops. How about add a timer to
> leave runtime suspend if keep_curr_dev_pwr_mode is set as true? This is
> simple and also favors power. The timeout value could be adjustable
> according to the available WriteBooster buffer size.
> 
> A periodic interval checking of flush status needs to re-activate link
> to communicate with the device. This would be tricky and the
> re-activation flow is just like runtime-resume.
> 
> What would you think?
> 
> Thanks.
> Stanley Chu
> 
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v1 4/4] scsi: ufs: Fix WriteBooster flush during runtime suspend

[Asutosh Das (asd)]

On 5/14/2020 7:49 AM, Stanley Chu wrote:
> Hi Asutosh,
> 
> On Thu, 2020-05-14 at 10:23 +0800, Stanley Chu wrote:
>> Hi Asutosh,
>>
>> On Wed, 2020-05-13 at 12:31 -0700, Asutosh Das (asd) wrote:
>>> On 5/12/2020 3:47 AM, Stanley Chu wrote:
>>>> Currently UFS host driver promises VCC supply if UFS device
>>>> needs to do WriteBooster flush during runtime suspend.
>>>>
>>>> However the UFS specification mentions,
>>>>
>>>> "While the flushing operation is in progress, the device is
>>>> in Active power mode."
>>>>
>>>> Therefore UFS host driver needs to promise more: Keep UFS
>>>> device as "Active power mode", otherwise UFS device shall not
>>>> do any flush if device enters Sleep or PowerDown power mode.
>>>>
>>>> Fix this by not changing device power mode if WriteBooster
>>>> flush is required in ufshcd_suspend().
>>>>
>>>> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
>>>> ---
>>>>    drivers/scsi/ufs/ufs.h    |  1 -
>>>>    drivers/scsi/ufs/ufshcd.c | 39 +++++++++++++++++++--------------------
>>>>    2 files changed, 19 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
>>>> index b3135344ab3f..9e4bc2e97ada 100644
>>>> --- a/drivers/scsi/ufs/ufs.h
>>>> +++ b/drivers/scsi/ufs/ufs.h
>>>> @@ -577,7 +577,6 @@ struct ufs_dev_info {
>>>>    	u32 d_ext_ufs_feature_sup;
>>>>    	u8 b_wb_buffer_type;
>>>>    	u32 d_wb_alloc_units;
>>>> -	bool keep_vcc_on;
>>>>    	u8 b_presrv_uspc_en;
>>>>    };
>>>>    
>>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>>> index 169a3379e468..2d0aff8ac260 100644
>>>> --- a/drivers/scsi/ufs/ufshcd.c
>>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>>> @@ -8101,8 +8101,7 @@ static void ufshcd_vreg_set_lpm(struct ufs_hba *hba)
>>>>    	    !hba->dev_info.is_lu_power_on_wp) {
>>>>    		ufshcd_setup_vreg(hba, false);
>>>>    	} else if (!ufshcd_is_ufs_dev_active(hba)) {
>>>> -		if (!hba->dev_info.keep_vcc_on)
>>>> -			ufshcd_toggle_vreg(hba->dev, hba->vreg_info.vcc, false);
>>>> +		ufshcd_toggle_vreg(hba->dev, hba->vreg_info.vcc, false);
>>>>    		if (!ufshcd_is_link_active(hba)) {
>>>>    			ufshcd_config_vreg_lpm(hba, hba->vreg_info.vccq);
>>>>    			ufshcd_config_vreg_lpm(hba, hba->vreg_info.vccq2);
>>>> @@ -8172,6 +8171,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>>>>    	enum ufs_pm_level pm_lvl;
>>>>    	enum ufs_dev_pwr_mode req_dev_pwr_mode;
>>>>    	enum uic_link_state req_link_state;
>>>> +	bool keep_curr_dev_pwr_mode = false;
>>>>    
>>>>    	hba->pm_op_in_progress = 1;
>>>>    	if (!ufshcd_is_shutdown_pm(pm_op)) {
>>>> @@ -8226,28 +8226,27 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>>>>    			/* make sure that auto bkops is disabled */
>>>>    			ufshcd_disable_auto_bkops(hba);
>>>>    		}
>>>> +
>>> Unnecessary newline, perhaps?
>>
>> Yap, I will remove it in next version.
>>
>>>>    		/*
>>>> -		 * With wb enabled, if the bkops is enabled or if the
>>>> -		 * configured WB type is 70% full, keep vcc ON
>>>> -		 * for the device to flush the wb buffer
>>>> +		 * If device needs to do BKOP or WB buffer flush, keep device
>>>> +		 * power mode as "active power mode" and its VCC supply.
>>>>    		 */
>>>> -		if ((hba->auto_bkops_enabled && ufshcd_is_wb_allowed(hba)) ||
>>>> -		    ufshcd_wb_keep_vcc_on(hba))
>>>> -			hba->dev_info.keep_vcc_on = true;
>>>> -		else
>>>> -			hba->dev_info.keep_vcc_on = false;
>>>> -	} else {
>>>> -		hba->dev_info.keep_vcc_on = false;
>>>> +		keep_curr_dev_pwr_mode = hba->auto_bkops_enabled ||
>>>> +			ufshcd_wb_keep_vcc_on(hba);
>>> Should the device be in UFS_ACTIVE_PWR_MODE to perform auto-bkops?
>>>
>>> Also, is it needed to keep the device in UFS_ACTIVE_PWR_MODE , if flush
>>> on hibern8 is enabled and the link is being put to hibern8 mode during
>>> runtime-suspend? Perhaps that should also be factored in here?
>>
>> Both auto-bkops and WriteBooster flush during Hibern8 need device power
>> mode to be "Active Power Mode".
>>
>> For auto-bkops, the spec mentions,
>>
>> "If the background operations enable bit is set and the device is in
>> Active power mode or Idle power mode, then the device is allowed to
>> execute any internal operations."
>>
>> For WriteBooster flush during Hibern8, the spec mentions,
>>
>> "While the flushing operation is in progress, the device is in Active
>> power mode."
>>
>> Therefore here we can use an unified "keep_curr_dev_pwr_mode" to
>> indicate the same requirements of above both features.
>>
>> Besides, both operations may access flash array inside UFS device thus
>> VCC supply shall be also kept.
>>
>> Before this patch, the original code will keep device power mode (stay
>> in Active Power Mode) if hba->auto_bkops_enabled is set as true during
>> runtime-suspend with UFSHCD_CAP_AUTO_BKOPS_SUSPEND capability is
>> enabled. This patch will not change this decision, just add
>> "WriteBooster flush during Hibern8" feature as another condition to do
>> so.
>>
>> Thank you so much to remind me that "Link shall be put in Hibern8" is a
>> necessary condition for "WriteBooster flush during Hibern8". I will add
>> more checking for keep_curr_dev_pwr_mode to prevent unnecessary power
>> drain.
>>
>>>>    	}
>>>>    
>>>> -	if ((req_dev_pwr_mode != hba->curr_dev_pwr_mode) &&
>>>> -	    ((ufshcd_is_runtime_pm(pm_op) && !hba->auto_bkops_enabled) ||
>>>> -	    !ufshcd_is_runtime_pm(pm_op))) {
>>>> -		/* ensure that bkops is disabled */
>>>> -		ufshcd_disable_auto_bkops(hba);
>>>> -		ret = ufshcd_set_dev_pwr_mode(hba, req_dev_pwr_mode);
>>>> -		if (ret)
>>>> -			goto enable_gating;
>>>> +	if (req_dev_pwr_mode != hba->curr_dev_pwr_mode) {
>>>> +		if ((ufshcd_is_runtime_pm(pm_op) && !hba->auto_bkops_enabled) ||
>>>> +		    !ufshcd_is_runtime_pm(pm_op)) {
>>>> +			/* ensure that bkops is disabled */
>>>> +			ufshcd_disable_auto_bkops(hba);
>>>> +		}
>>>> +
>>>> +		if (!keep_curr_dev_pwr_mode) {
>>>> +			ret = ufshcd_set_dev_pwr_mode(hba, req_dev_pwr_mode);
>>>
>>> Now, when the WB buffer is completely flushed out, the device should be
>>> put back into UFS_SLEEP_PWR_MODE or UFS_POWERDOWN_PWR_MODE. Say, the
>>> device buffer has to be flushed and during runtime-suspend, the device
>>> is put to UFS_ACTIVE_PWR_MODE and Vcc is kept ON; the device doesn't
>>> resume nor does the system enters suspend for a very long time, and with
>>> AH8 and hibern8 disabled, there will be an unnecessary power drain for
>>> that much time.
> 
> Another thought is that if keep_curr_dev_pwr_mode will be set as true
> only if link is put in Hibern8 or Auto-Hibern8 is enabled. By this way,
> the power consumption shall be very small after flush or auto-bkop is
> finished.
> 
> Then the checking of flush status during runtime-suspend may be not
> necessary.
> 
>>>
>>> How about a periodic interval checking of flush status if
>>> keep_curr_dev_pwr_mode evaluates to be true?
>>
>> This is a good point!
>>
>> The same thing also happens for auto-bkops. How about add a timer to
>> leave runtime suspend if keep_curr_dev_pwr_mode is set as true? This is
>> simple and also favors power. The timeout value could be adjustable
>> according to the available WriteBooster buffer size.
>>
>> A periodic interval checking of flush status needs to re-activate link
>> to communicate with the device. This would be tricky and the
>> re-activation flow is just like runtime-resume.
>>
>> What would you think?
>>
>> Thanks.
>> Stanley Chu
>>
>>
>> _______________________________________________
>> Linux-mediatek mailing list
>> Linux-mediatek@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-mediatek
> 

Hi Stanley,
I think that'd work, but there's definitely a penalty of keeping Vcc ON.
And if we do want to keep it ON, then we'd have to measure how much 
excess power is being used - after the flush is done.

I think setting keep_curr_dev_pwr_mode to true iff h8 and ah8 are 
enabled is a good idea. In addition to that, adding a timer to check 
flush status if keep_curr_dev_pwr_mode is set to true would keep the 
power consumption to a minimum. So I suggest to have the delayed check 
of flush status as well.

Thanks,
-asd

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