[PATCH v4 0/6] Several changes for UFS WriteBooster

[PATCH v4 0/6] Several changes for UFS WriteBooster

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Changelog:
v3--v4:
  1. Rebase patch on 5.11/scsi-staging
  2. Add WB cleanup patches 3/6, 4/6 adn 5/6

v2--v3:
  1. Change multi-line comments style in patch 1/3 (Can Guo)

v1--v2:
  1. Take is_hibern8_wb_flush checkup out from function
     ufshcd_wb_need_flush() in patch 2/3
  2. Add UFSHCD_CAP_CLK_SCALING checkup in patch 1/3. that means
     only for the platform, which doesn't support UFSHCD_CAP_CLK_SCALING,
     can control WB through "wb_on".

Bean Huo (6):
  scsi: ufs: Add "wb_on" sysfs node to control WB on/off
  scsi: ufs: Changes comment in the function ufshcd_wb_probe()
  scsi: ufs: Group UFS WB related flags to struct ufs_dev_info
  scsi: ufs: Remove d_wb_alloc_units from struct ufs_dev_info
  scsi: ufs: Cleanup WB buffer flush toggle implementation
  scsi: ufs: Keep device active mode only
    fWriteBoosterBufferFlushDuringHibernate == 1

 drivers/scsi/ufs/ufs-sysfs.c |  41 +++++++++++++
 drivers/scsi/ufs/ufs.h       |  33 +++++-----
 drivers/scsi/ufs/ufshcd.c    | 114 +++++++++++++++--------------------
 drivers/scsi/ufs/ufshcd.h    |   4 +-
 4 files changed, 112 insertions(+), 80 deletions(-)

-- 
2.17.1

[PATCH v4 1/6] scsi: ufs: Add "wb_on" sysfs node to control WB on/off

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Currently UFS WriteBooster driver uses clock scaling up/down to set
WB on/off, for the platform which doesn't support UFSHCD_CAP_CLK_SCALING,
WB will be always on. Provide a sysfs attribute to enable/disable WB
during runtime. Write 1/0 to "wb_on" sysfs node to enable/disable UFS WB.

Reviewed-by: Avri Altman <avri.altman@wdc.com>
Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufs-sysfs.c | 41 ++++++++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufshcd.c    |  3 +--
 drivers/scsi/ufs/ufshcd.h    |  2 ++
 3 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index 08e72b7eef6a..2b4e9fe935cc 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -189,6 +189,45 @@ static ssize_t auto_hibern8_store(struct device *dev,
 	return count;
 }
 
+static ssize_t wb_on_show(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", hba->wb_enabled);
+}
+
+static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	unsigned int wb_enable;
+	ssize_t res;
+
+	if (ufshcd_is_clkscaling_supported(hba)) {
+		/*
+		 * If the platform supports UFSHCD_CAP_AUTO_BKOPS_SUSPEND,
+		 * turn WB on/off will be done while clock scaling up/down.
+		 */
+		dev_warn(dev, "To control WB through wb_on is not allowed!\n");
+		return -EOPNOTSUPP;
+	}
+	if (!ufshcd_is_wb_allowed(hba))
+		return -EOPNOTSUPP;
+
+	if (kstrtouint(buf, 0, &wb_enable))
+		return -EINVAL;
+
+	if (wb_enable != 0 && wb_enable != 1)
+		return -EINVAL;
+
+	pm_runtime_get_sync(hba->dev);
+	res = ufshcd_wb_ctrl(hba, wb_enable);
+	pm_runtime_put_sync(hba->dev);
+
+	return res < 0 ? res : count;
+}
+
 static DEVICE_ATTR_RW(rpm_lvl);
 static DEVICE_ATTR_RO(rpm_target_dev_state);
 static DEVICE_ATTR_RO(rpm_target_link_state);
@@ -196,6 +235,7 @@ static DEVICE_ATTR_RW(spm_lvl);
 static DEVICE_ATTR_RO(spm_target_dev_state);
 static DEVICE_ATTR_RO(spm_target_link_state);
 static DEVICE_ATTR_RW(auto_hibern8);
+static DEVICE_ATTR_RW(wb_on);
 
 static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
 	&dev_attr_rpm_lvl.attr,
@@ -205,6 +245,7 @@ static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
 	&dev_attr_spm_target_dev_state.attr,
 	&dev_attr_spm_target_link_state.attr,
 	&dev_attr_auto_hibern8.attr,
+	&dev_attr_wb_on.attr,
 	NULL
 };
 
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index e221add25a7e..5e1dcf4de67e 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -246,7 +246,6 @@ static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba,
 static int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag);
 static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba);
 static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba);
-static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable);
 static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set);
 static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable);
 static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba);
@@ -5351,7 +5350,7 @@ static void ufshcd_bkops_exception_event_handler(struct ufs_hba *hba)
 				__func__, err);
 }
 
-static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
+int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
 {
 	int ret;
 	u8 index;
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 9bb5f0ed4124..2a97006a2c93 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -1068,6 +1068,8 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
 			     u8 *desc_buff, int *buff_len,
 			     enum query_opcode desc_op);
 
+int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable);
+
 /* Wrapper functions for safely calling variant operations */
 static inline const char *ufshcd_get_var_name(struct ufs_hba *hba)
 {
-- 
2.17.1

[PATCH v4 2/6] scsi: ufs: Changes comment in the function ufshcd_wb_probe()

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

USFHCD supports WriteBooster "LU dedicated buffer” mode and
“shared buffer” mode both, so changes the comment in the
function ufshcd_wb_probe().

Reviewed-by: Can Guo <cang@codeaurora.org>
Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufshcd.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 5e1dcf4de67e..6a5532b752aa 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7232,10 +7232,9 @@ static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf)
 		goto wb_disabled;
 
 	/*
-	 * WB may be supported but not configured while provisioning.
-	 * The spec says, in dedicated wb buffer mode,
-	 * a max of 1 lun would have wb buffer configured.
-	 * Now only shared buffer mode is supported.
+	 * WB may be supported but not configured while provisioning. The spec
+	 * says, in dedicated wb buffer mode, a max of 1 lun would have wb
+	 * buffer configured.
 	 */
 	dev_info->b_wb_buffer_type =
 		desc_buf[DEVICE_DESC_PARAM_WB_TYPE];
-- 
2.17.1

[PATCH v4 3/6] scsi: ufs: Group UFS WB related flags to struct ufs_dev_info

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

UFS device-related flags should be grouped in ufs_dev_info. Take
wb_enabled and wb_buf_flush_enabled out from the struct ufs_hba,
group them to struct ufs_dev_info, and align the names of the structure
members vertically

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufs-sysfs.c |  2 +-
 drivers/scsi/ufs/ufs.h       | 33 +++++++++++++++++++--------------
 drivers/scsi/ufs/ufshcd.c    | 16 ++++++++--------
 drivers/scsi/ufs/ufshcd.h    |  2 --
 4 files changed, 28 insertions(+), 25 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index 2b4e9fe935cc..4bd7e18bb486 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -194,7 +194,7 @@ static ssize_t wb_on_show(struct device *dev, struct device_attribute *attr,
 {
 	struct ufs_hba *hba = dev_get_drvdata(dev);
 
-	return scnprintf(buf, PAGE_SIZE, "%d\n", hba->wb_enabled);
+	return scnprintf(buf, PAGE_SIZE, "%d\n", hba->dev_info.wb_enabled);
 }
 
 static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 14dfda735adf..45bebca29fdd 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -527,22 +527,27 @@ struct ufs_vreg_info {
 };
 
 struct ufs_dev_info {
-	bool f_power_on_wp_en;
+	bool	f_power_on_wp_en;
 	/* Keeps information if any of the LU is power on write protected */
-	bool is_lu_power_on_wp;
+	bool	is_lu_power_on_wp;
 	/* Maximum number of general LU supported by the UFS device */
-	u8 max_lu_supported;
-	u8 wb_dedicated_lu;
-	u16 wmanufacturerid;
-	/*UFS device Product Name */
-	u8 *model;
-	u16 wspecversion;
-	u32 clk_gating_wait_us;
-	u32 d_ext_ufs_feature_sup;
-	u8 b_wb_buffer_type;
-	u32 d_wb_alloc_units;
-	bool b_rpm_dev_flush_capable;
-	u8 b_presrv_uspc_en;
+	u8	max_lu_supported;
+	u16	wmanufacturerid;
+	/* UFS device Product Name */
+	u8	*model;
+	u16	wspecversion;
+	u32	clk_gating_wait_us;
+	u32	d_ext_ufs_feature_sup;
+
+	/* UFS WB related flags */
+	bool	wb_enabled;
+	bool	wb_buf_flush_enabled;
+	u8	wb_dedicated_lu;
+	u8	b_wb_buffer_type;
+	u32	d_wb_alloc_units;
+
+	bool	b_rpm_dev_flush_capable;
+	u8	b_presrv_uspc_en;
 };
 
 /**
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 6a5532b752aa..528c257df48c 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -589,8 +589,8 @@ static void ufshcd_device_reset(struct ufs_hba *hba)
 	if (!err) {
 		ufshcd_set_ufs_dev_active(hba);
 		if (ufshcd_is_wb_allowed(hba)) {
-			hba->wb_enabled = false;
-			hba->wb_buf_flush_enabled = false;
+			hba->dev_info.wb_enabled = false;
+			hba->dev_info.wb_buf_flush_enabled = false;
 		}
 	}
 	if (err != -EOPNOTSUPP)
@@ -5359,7 +5359,7 @@ int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
 	if (!ufshcd_is_wb_allowed(hba))
 		return 0;
 
-	if (!(enable ^ hba->wb_enabled))
+	if (!(enable ^ hba->dev_info.wb_enabled))
 		return 0;
 	if (enable)
 		opcode = UPIU_QUERY_OPCODE_SET_FLAG;
@@ -5375,7 +5375,7 @@ int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
 		return ret;
 	}
 
-	hba->wb_enabled = enable;
+	hba->dev_info.wb_enabled = enable;
 	dev_dbg(hba->dev, "%s write booster %s %d\n",
 			__func__, enable ? "enable" : "disable", ret);
 
@@ -5415,7 +5415,7 @@ static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba)
 	int ret;
 	u8 index;
 
-	if (!ufshcd_is_wb_allowed(hba) || hba->wb_buf_flush_enabled)
+	if (!ufshcd_is_wb_allowed(hba) || hba->dev_info.wb_buf_flush_enabled)
 		return 0;
 
 	index = ufshcd_wb_get_query_index(hba);
@@ -5426,7 +5426,7 @@ static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba)
 		dev_err(hba->dev, "%s WB - buf flush enable failed %d\n",
 			__func__, ret);
 	else
-		hba->wb_buf_flush_enabled = true;
+		hba->dev_info.wb_buf_flush_enabled = true;
 
 	dev_dbg(hba->dev, "WB - Flush enabled: %d\n", ret);
 	return ret;
@@ -5437,7 +5437,7 @@ static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba)
 	int ret;
 	u8 index;
 
-	if (!ufshcd_is_wb_allowed(hba) || !hba->wb_buf_flush_enabled)
+	if (!ufshcd_is_wb_allowed(hba) || !hba->dev_info.wb_buf_flush_enabled)
 		return 0;
 
 	index = ufshcd_wb_get_query_index(hba);
@@ -5448,7 +5448,7 @@ static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba)
 		dev_warn(hba->dev, "%s: WB - buf flush disable failed %d\n",
 			 __func__, ret);
 	} else {
-		hba->wb_buf_flush_enabled = false;
+		hba->dev_info.wb_buf_flush_enabled = false;
 		dev_dbg(hba->dev, "WB - Flush disabled: %d\n", ret);
 	}
 
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 2a97006a2c93..45c3eca88f0e 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -805,8 +805,6 @@ struct ufs_hba {
 
 	struct device		bsg_dev;
 	struct request_queue	*bsg_queue;
-	bool wb_buf_flush_enabled;
-	bool wb_enabled;
 	struct delayed_work rpm_dev_flush_recheck_work;
 
 #ifdef CONFIG_SCSI_UFS_CRYPTO
-- 
2.17.1

[PATCH v4 4/6] scsi: ufs: Remove d_wb_alloc_units from struct ufs_dev_info

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

d_wb_alloc_units only be used while WB probe, just used to confirm the
condition that "if bWriteBoosterBufferType is set to 01h but
dNumSharedWriteBoosterBufferAllocUnits is set to zero, the WriteBooster
feature is disabled". So, don't need to keep it in runtime.

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufs.h    | 1 -
 drivers/scsi/ufs/ufshcd.c | 6 ++----
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 45bebca29fdd..8ed342e43883 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -544,7 +544,6 @@ struct ufs_dev_info {
 	bool	wb_buf_flush_enabled;
 	u8	wb_dedicated_lu;
 	u8	b_wb_buffer_type;
-	u32	d_wb_alloc_units;
 
 	bool	b_rpm_dev_flush_capable;
 	u8	b_presrv_uspc_en;
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 528c257df48c..0998e6103cd7 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7243,10 +7243,8 @@ static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf)
 		desc_buf[DEVICE_DESC_PARAM_WB_PRESRV_USRSPC_EN];
 
 	if (dev_info->b_wb_buffer_type == WB_BUF_MODE_SHARED) {
-		dev_info->d_wb_alloc_units =
-		get_unaligned_be32(desc_buf +
-				   DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS);
-		if (!dev_info->d_wb_alloc_units)
+		if (!get_unaligned_be32(desc_buf +
+				   DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS))
 			goto wb_disabled;
 	} else {
 		for (lun = 0; lun < UFS_UPIU_MAX_WB_LUN_ID; lun++) {
-- 
2.17.1

[PATCH v4 5/6] scsi: ufs: Cleanup WB buffer flush toggle implementation

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Delete ufshcd_wb_buf_flush_enable() and ufshcd_wb_buf_flush_disable(),
move the implementation into ufshcd_wb_toggle_flush().

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufshcd.c | 69 ++++++++++++++-------------------------
 1 file changed, 24 insertions(+), 45 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 0998e6103cd7..fb3c98724005 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -244,10 +244,8 @@ static int ufshcd_setup_vreg(struct ufs_hba *hba, bool on);
 static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba,
 					 struct ufs_vreg *vreg);
 static int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag);
-static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba);
-static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba);
 static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set);
-static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable);
+static inline int ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable);
 static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba);
 static void ufshcd_hba_vreg_set_hpm(struct ufs_hba *hba);
 
@@ -5398,60 +5396,41 @@ static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set)
 				index, NULL);
 }
 
-static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable)
-{
-	if (hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)
-		return;
-
-	if (enable)
-		ufshcd_wb_buf_flush_enable(hba);
-	else
-		ufshcd_wb_buf_flush_disable(hba);
-
-}
-
-static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba)
+static inline int ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable)
 {
 	int ret;
 	u8 index;
+	enum query_opcode opcode;
 
-	if (!ufshcd_is_wb_allowed(hba) || hba->dev_info.wb_buf_flush_enabled)
+	if (hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)
 		return 0;
 
-	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);
-	if (ret)
-		dev_err(hba->dev, "%s WB - buf flush enable failed %d\n",
-			__func__, ret);
-	else
-		hba->dev_info.wb_buf_flush_enabled = true;
-
-	dev_dbg(hba->dev, "WB - Flush enabled: %d\n", ret);
-	return ret;
-}
-
-static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba)
-{
-	int ret;
-	u8 index;
-
-	if (!ufshcd_is_wb_allowed(hba) || !hba->dev_info.wb_buf_flush_enabled)
+	if (!ufshcd_is_wb_allowed(hba) ||
+	    hba->dev_info.wb_buf_flush_enabled == enable)
 		return 0;
 
+	if (enable)
+		opcode = UPIU_QUERY_OPCODE_SET_FLAG;
+	else
+		opcode = UPIU_QUERY_OPCODE_CLEAR_FLAG;
+
 	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);
+	ret = ufshcd_query_flag_retry(hba, opcode,
+				      QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN, index,
+				      NULL);
 	if (ret) {
-		dev_warn(hba->dev, "%s: WB - buf flush disable failed %d\n",
-			 __func__, ret);
-	} else {
-		hba->dev_info.wb_buf_flush_enabled = false;
-		dev_dbg(hba->dev, "WB - Flush disabled: %d\n", ret);
+		dev_err(hba->dev, "%s WB-Buf Flush %s failed %d\n", __func__,
+			enable ? "enable" : "disable", ret);
+		goto out;
 	}
 
+	if (enable)
+		hba->dev_info.wb_buf_flush_enabled = true;
+	else
+		hba->dev_info.wb_buf_flush_enabled = false;
+
+	dev_dbg(hba->dev, "WB-Buf Flush %s\n", enable ? "enabled" : "disabled");
+out:
 	return ret;
 }
 
-- 
2.17.1

[PATCH v4 6/6] scsi: ufs: Keep device active mode only fWriteBoosterBufferFlushDuringHibernate == 1

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

According to the JEDEC UFS 3.1 Spec, If fWriteBoosterBufferFlushDuringHibernate
is set to one, the device flushes the WriteBooster Buffer data automatically
whenever the link enters the hibernate (HIBERN8) state. While the flushing
operation is in progress, the device should be kept in Active power mode.
Currently, we set this flag during the UFSHCD probe stage, but we didn't deal
with its programming failure. Even this failure is less likely to occur, but
still it is possible.
This patch is to add checkup of fWriteBoosterBufferFlushDuringHibernate setting,
keep the device as "active power mode" only when this flag be successfully set
to 1.

Fixes: 51dd905bd2f6 ("scsi: ufs: Fix WriteBooster flush during runtime suspend")
Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufs.h    |  1 +
 drivers/scsi/ufs/ufshcd.c | 21 ++++++++++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 8ed342e43883..1b3866f608d9 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -542,6 +542,7 @@ struct ufs_dev_info {
 	/* UFS WB related flags */
 	bool	wb_enabled;
 	bool	wb_buf_flush_enabled;
+	bool	wb_buf_flush_in_hibern8;
 	u8	wb_dedicated_lu;
 	u8	b_wb_buffer_type;
 
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index fb3c98724005..7ff486f047d8 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -282,10 +282,16 @@ static inline void ufshcd_wb_config(struct ufs_hba *hba)
 		dev_err(hba->dev, "%s: Enable WB failed: %d\n", __func__, ret);
 	else
 		dev_info(hba->dev, "%s: Write Booster Configured\n", __func__);
+
 	ret = ufshcd_wb_toggle_flush_during_h8(hba, true);
-	if (ret)
+	if (ret) {
 		dev_err(hba->dev, "%s: En WB flush during H8: failed: %d\n",
 			__func__, ret);
+		hba->dev_info.wb_buf_flush_in_hibern8 = false;
+	} else {
+		hba->dev_info.wb_buf_flush_in_hibern8 = true;
+	}
+
 	ufshcd_wb_toggle_flush(hba, true);
 }
 
@@ -589,6 +595,7 @@ static void ufshcd_device_reset(struct ufs_hba *hba)
 		if (ufshcd_is_wb_allowed(hba)) {
 			hba->dev_info.wb_enabled = false;
 			hba->dev_info.wb_buf_flush_enabled = false;
+			hba->dev_info.wb_buf_flush_in_hibern8 = false;
 		}
 	}
 	if (err != -EOPNOTSUPP)
@@ -5471,6 +5478,7 @@ static bool ufshcd_wb_need_flush(struct ufs_hba *hba)
 
 	if (!ufshcd_is_wb_allowed(hba))
 		return false;
+
 	/*
 	 * The ufs device needs the vcc to be ON to flush.
 	 * With user-space reduction enabled, it's enough to enable flush
@@ -8571,6 +8579,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 hibern8;
 
 	hba->pm_op_in_progress = 1;
 	if (!ufshcd_is_shutdown_pm(pm_op)) {
@@ -8630,11 +8639,13 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 		 * Hibern8, keep device power mode as "active power mode"
 		 * and VCC supply.
 		 */
+		hibern8 = req_link_state == UIC_LINK_HIBERN8_STATE ||
+			(req_link_state == UIC_LINK_ACTIVE_STATE &&
+			 ufshcd_is_auto_hibern8_enabled(hba));
+
 		hba->dev_info.b_rpm_dev_flush_capable =
-			hba->auto_bkops_enabled ||
-			(((req_link_state == UIC_LINK_HIBERN8_STATE) ||
-			((req_link_state == UIC_LINK_ACTIVE_STATE) &&
-			ufshcd_is_auto_hibern8_enabled(hba))) &&
+			hba->auto_bkops_enabled || (hibern8 &&
+			hba->dev_info.wb_buf_flush_in_hibern8 &&
 			ufshcd_wb_need_flush(hba));
 	}
 
-- 
2.17.1

Re: [PATCH v4 4/6] scsi: ufs: Remove d_wb_alloc_units from struct ufs_dev_info

[Stanley Chu]

Hi Bean,

On Fri, 2020-12-11 at 15:00 +0100, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> d_wb_alloc_units only be used while WB probe, just used to confirm the
> condition that "if bWriteBoosterBufferType is set to 01h but
> dNumSharedWriteBoosterBufferAllocUnits is set to zero, the WriteBooster
> feature is disabled". So, don't need to keep it in runtime.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/scsi/ufs/ufs.h    | 1 -
>  drivers/scsi/ufs/ufshcd.c | 6 ++----
>  2 files changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index 45bebca29fdd..8ed342e43883 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -544,7 +544,6 @@ struct ufs_dev_info {
>  	bool	wb_buf_flush_enabled;
>  	u8	wb_dedicated_lu;
>  	u8	b_wb_buffer_type;
> -	u32	d_wb_alloc_units;

Perhaps below two fields could be also removed from struct ufs_dev_info
for the same reason?

u32 d_ext_ufs_feature_sup;
u32 d_wb_alloc_units;

Thanks,
Stanley Chu

>  
>  	bool	b_rpm_dev_flush_capable;
>  	u8	b_presrv_uspc_en;
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 528c257df48c..0998e6103cd7 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -7243,10 +7243,8 @@ static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf)
>  		desc_buf[DEVICE_DESC_PARAM_WB_PRESRV_USRSPC_EN];
>  
>  	if (dev_info->b_wb_buffer_type == WB_BUF_MODE_SHARED) {
> -		dev_info->d_wb_alloc_units =
> -		get_unaligned_be32(desc_buf +
> -				   DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS);
> -		if (!dev_info->d_wb_alloc_units)
> +		if (!get_unaligned_be32(desc_buf +
> +				   DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS))
>  			goto wb_disabled;
>  	} else {
>  		for (lun = 0; lun < UFS_UPIU_MAX_WB_LUN_ID; lun++) {

Re: [PATCH v4 3/6] scsi: ufs: Group UFS WB related flags to struct ufs_dev_info

[Stanley Chu]

Hi Bean,

On Fri, 2020-12-11 at 15:00 +0100, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> UFS device-related flags should be grouped in ufs_dev_info. Take
> wb_enabled and wb_buf_flush_enabled out from the struct ufs_hba,
> group them to struct ufs_dev_info, and align the names of the structure
> members vertically
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/scsi/ufs/ufs-sysfs.c |  2 +-
>  drivers/scsi/ufs/ufs.h       | 33 +++++++++++++++++++--------------
>  drivers/scsi/ufs/ufshcd.c    | 16 ++++++++--------
>  drivers/scsi/ufs/ufshcd.h    |  2 --
>  4 files changed, 28 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
> index 2b4e9fe935cc..4bd7e18bb486 100644
> --- a/drivers/scsi/ufs/ufs-sysfs.c
> +++ b/drivers/scsi/ufs/ufs-sysfs.c
> @@ -194,7 +194,7 @@ static ssize_t wb_on_show(struct device *dev, struct device_attribute *attr,
>  {
>  	struct ufs_hba *hba = dev_get_drvdata(dev);
>  
> -	return scnprintf(buf, PAGE_SIZE, "%d\n", hba->wb_enabled);
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", hba->dev_info.wb_enabled);
>  }
>  
>  static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index 14dfda735adf..45bebca29fdd 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -527,22 +527,27 @@ struct ufs_vreg_info {
>  };
>  
>  struct ufs_dev_info {
> -	bool f_power_on_wp_en;
> +	bool	f_power_on_wp_en;
>  	/* Keeps information if any of the LU is power on write protected */
> -	bool is_lu_power_on_wp;
> +	bool	is_lu_power_on_wp;
>  	/* Maximum number of general LU supported by the UFS device */
> -	u8 max_lu_supported;
> -	u8 wb_dedicated_lu;
> -	u16 wmanufacturerid;
> -	/*UFS device Product Name */
> -	u8 *model;
> -	u16 wspecversion;
> -	u32 clk_gating_wait_us;
> -	u32 d_ext_ufs_feature_sup;
> -	u8 b_wb_buffer_type;
> -	u32 d_wb_alloc_units;
> -	bool b_rpm_dev_flush_capable;
> -	u8 b_presrv_uspc_en;
> +	u8	max_lu_supported;
> +	u16	wmanufacturerid;
> +	/* UFS device Product Name */
> +	u8	*model;
> +	u16	wspecversion;
> +	u32	clk_gating_wait_us;
> +	u32	d_ext_ufs_feature_sup;
> +
> +	/* UFS WB related flags */
> +	bool	wb_enabled;
> +	bool	wb_buf_flush_enabled;
> +	u8	wb_dedicated_lu;
> +	u8	b_wb_buffer_type;
> +	u32	d_wb_alloc_units;
> +
> +	bool	b_rpm_dev_flush_capable;
> +	u8	b_presrv_uspc_en;

Perhaps we could unify the style of these WB related stuff to wb_* ?

Besides, I am not sure if using tab instead space between the type and
name in this struct is a good idea.

Thanks,
Stanley Chu

>  };
>  
>  /**
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 6a5532b752aa..528c257df48c 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -589,8 +589,8 @@ static void ufshcd_device_reset(struct ufs_hba *hba)
>  	if (!err) {
>  		ufshcd_set_ufs_dev_active(hba);
>  		if (ufshcd_is_wb_allowed(hba)) {
> -			hba->wb_enabled = false;
> -			hba->wb_buf_flush_enabled = false;
> +			hba->dev_info.wb_enabled = false;
> +			hba->dev_info.wb_buf_flush_enabled = false;
>  		}
>  	}
>  	if (err != -EOPNOTSUPP)
> @@ -5359,7 +5359,7 @@ int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
>  	if (!ufshcd_is_wb_allowed(hba))
>  		return 0;
>  
> -	if (!(enable ^ hba->wb_enabled))
> +	if (!(enable ^ hba->dev_info.wb_enabled))
>  		return 0;
>  	if (enable)
>  		opcode = UPIU_QUERY_OPCODE_SET_FLAG;
> @@ -5375,7 +5375,7 @@ int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
>  		return ret;
>  	}
>  
> -	hba->wb_enabled = enable;
> +	hba->dev_info.wb_enabled = enable;
>  	dev_dbg(hba->dev, "%s write booster %s %d\n",
>  			__func__, enable ? "enable" : "disable", ret);
>  
> @@ -5415,7 +5415,7 @@ static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba)
>  	int ret;
>  	u8 index;
>  
> -	if (!ufshcd_is_wb_allowed(hba) || hba->wb_buf_flush_enabled)
> +	if (!ufshcd_is_wb_allowed(hba) || hba->dev_info.wb_buf_flush_enabled)
>  		return 0;
>  
>  	index = ufshcd_wb_get_query_index(hba);
> @@ -5426,7 +5426,7 @@ static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba)
>  		dev_err(hba->dev, "%s WB - buf flush enable failed %d\n",
>  			__func__, ret);
>  	else
> -		hba->wb_buf_flush_enabled = true;
> +		hba->dev_info.wb_buf_flush_enabled = true;
>  
>  	dev_dbg(hba->dev, "WB - Flush enabled: %d\n", ret);
>  	return ret;
> @@ -5437,7 +5437,7 @@ static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba)
>  	int ret;
>  	u8 index;
>  
> -	if (!ufshcd_is_wb_allowed(hba) || !hba->wb_buf_flush_enabled)
> +	if (!ufshcd_is_wb_allowed(hba) || !hba->dev_info.wb_buf_flush_enabled)
>  		return 0;
>  
>  	index = ufshcd_wb_get_query_index(hba);
> @@ -5448,7 +5448,7 @@ static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba)
>  		dev_warn(hba->dev, "%s: WB - buf flush disable failed %d\n",
>  			 __func__, ret);
>  	} else {
> -		hba->wb_buf_flush_enabled = false;
> +		hba->dev_info.wb_buf_flush_enabled = false;
>  		dev_dbg(hba->dev, "WB - Flush disabled: %d\n", ret);
>  	}
>  
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 2a97006a2c93..45c3eca88f0e 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -805,8 +805,6 @@ struct ufs_hba {
>  
>  	struct device		bsg_dev;
>  	struct request_queue	*bsg_queue;
> -	bool wb_buf_flush_enabled;
> -	bool wb_enabled;
>  	struct delayed_work rpm_dev_flush_recheck_work;
>  
>  #ifdef CONFIG_SCSI_UFS_CRYPTO

Re: [PATCH v4 5/6] scsi: ufs: Cleanup WB buffer flush toggle implementation

[Stanley Chu]

Hi Bean,

On Fri, 2020-12-11 at 15:00 +0100, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> Delete ufshcd_wb_buf_flush_enable() and ufshcd_wb_buf_flush_disable(),
> move the implementation into ufshcd_wb_toggle_flush().
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 69 ++++++++++++++-------------------------
>  1 file changed, 24 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 0998e6103cd7..fb3c98724005 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -244,10 +244,8 @@ static int ufshcd_setup_vreg(struct ufs_hba *hba, bool on);
>  static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba,
>  					 struct ufs_vreg *vreg);
>  static int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag);
> -static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba);
> -static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba);
>  static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set);
> -static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable);
> +static inline int ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable);
>  static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba);
>  static void ufshcd_hba_vreg_set_hpm(struct ufs_hba *hba);
>  
> @@ -5398,60 +5396,41 @@ static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set)
>  				index, NULL);
>  }
>  
> -static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable)
> -{
> -	if (hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)
> -		return;
> -
> -	if (enable)
> -		ufshcd_wb_buf_flush_enable(hba);
> -	else
> -		ufshcd_wb_buf_flush_disable(hba);
> -
> -}
> -
> -static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba)
> +static inline int ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable)
>  {
>  	int ret;
>  	u8 index;
> +	enum query_opcode opcode;
>  
> -	if (!ufshcd_is_wb_allowed(hba) || hba->dev_info.wb_buf_flush_enabled)
> +	if (hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)
>  		return 0;
>  
> -	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);
> -	if (ret)
> -		dev_err(hba->dev, "%s WB - buf flush enable failed %d\n",
> -			__func__, ret);
> -	else
> -		hba->dev_info.wb_buf_flush_enabled = true;
> -
> -	dev_dbg(hba->dev, "WB - Flush enabled: %d\n", ret);
> -	return ret;
> -}
> -
> -static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba)
> -{
> -	int ret;
> -	u8 index;
> -
> -	if (!ufshcd_is_wb_allowed(hba) || !hba->dev_info.wb_buf_flush_enabled)
> +	if (!ufshcd_is_wb_allowed(hba) ||
> +	    hba->dev_info.wb_buf_flush_enabled == enable)
>  		return 0;
>  
> +	if (enable)
> +		opcode = UPIU_QUERY_OPCODE_SET_FLAG;
> +	else
> +		opcode = UPIU_QUERY_OPCODE_CLEAR_FLAG;
> +
>  	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);
> +	ret = ufshcd_query_flag_retry(hba, opcode,
> +				      QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN, index,
> +				      NULL);
>  	if (ret) {
> -		dev_warn(hba->dev, "%s: WB - buf flush disable failed %d\n",
> -			 __func__, ret);
> -	} else {
> -		hba->dev_info.wb_buf_flush_enabled = false;
> -		dev_dbg(hba->dev, "WB - Flush disabled: %d\n", ret);
> +		dev_err(hba->dev, "%s WB-Buf Flush %s failed %d\n", __func__,
> +			enable ? "enable" : "disable", ret);
> +		goto out;
>  	}
>  
> +	if (enable)
> +		hba->dev_info.wb_buf_flush_enabled = true;
> +	else
> +		hba->dev_info.wb_buf_flush_enabled = false;

Perhaps this could be simpler as below?

hba->dev_info.wb_buf_flush_enabled = enable;

Thanks,
Stanley Chu

> +
> +	dev_dbg(hba->dev, "WB-Buf Flush %s\n", enable ? "enabled" : "disabled");
> +out:
>  	return ret;
>  }
>  

Re: [PATCH v4 5/6] scsi: ufs: Cleanup WB buffer flush toggle implementation

[Bean Huo]

On Tue, 2020-12-15 at 17:07 +0800, Stanley Chu wrote:
> >        if (ret) {
> > -             dev_warn(hba->dev, "%s: WB - buf flush disable failed
> > %d\n",
> > -                      __func__, ret);
> > -     } else {
> > -             hba->dev_info.wb_buf_flush_enabled = false;
> > -             dev_dbg(hba->dev, "WB - Flush disabled: %d\n", ret);
> > +             dev_err(hba->dev, "%s WB-Buf Flush %s failed %d\n",
> > __func__,
> > +                     enable ? "enable" : "disable", ret);
> > +             goto out;
> >        }
> >   
> > +     if (enable)
> > +             hba->dev_info.wb_buf_flush_enabled = true;
> > +     else
> > +             hba->dev_info.wb_buf_flush_enabled = false;
> 
> Perhaps this could be simpler as below?
> 
> hba->dev_info.wb_buf_flush_enabled = enable;

Thanks, will be changed in next version.

Bean

Re: [PATCH v4 3/6] scsi: ufs: Group UFS WB related flags to struct ufs_dev_info

[Bean Huo]

On Tue, 2020-12-15 at 17:01 +0800, Stanley Chu wrote:
> > +     bool    wb_buf_flush_enabled;
> > +     u8      wb_dedicated_lu;
> > +     u8      b_wb_buffer_type;
> > +     u32     d_wb_alloc_units;
> > +
> > +     bool    b_rpm_dev_flush_capable;
> > +     u8      b_presrv_uspc_en;
> 
> Perhaps we could unify the style of these WB related stuff to wb_* ?

yes, agree. I will change them.

> 
> Besides, I am not sure if using tab instead space between the type
> and
> name in this struct is a good idea.
> 
using space, in addition single space, type and parameter names are
mixed. 


use space:

 /* UFS WB related flags */
bool wb_enabled;
bool wb_buf_flush_enabled;
u8
wb_dedicated_lu;
u8 b_wb_buffer_type;
u32 d_wb_alloc_units;

use table:

 /* UFS WB related flags */
bool    wb_enabled;
bool    wb_buf_flush_enabled;
u8      wb_dedicated_lu;
u8      b_wb_buffer_type;
u32     d_wb_alloc_units;

I think, the result is very clear comparing above two examples. yes,
there is no explicit stipulation that we must use space or tab. Both
styles exist in Linux. Maybe this is just matter of personal interest.


Bean

> Thanks,
> Stanley Chu

Re: [PATCH v4 4/6] scsi: ufs: Remove d_wb_alloc_units from struct ufs_dev_info

[Bean Huo]

On Tue, 2020-12-15 at 16:57 +0800, Stanley Chu wrote:
> >        u8      b_wb_buffer_type;
> > -     u32     d_wb_alloc_units;
> 
> Perhaps below two fields could be also removed from struct
> ufs_dev_info
> for the same reason?
> 
> u32 d_ext_ufs_feature_sup;
I thought twice before this patch. maybe will be used in near future,
so I keep d_ext_ufs_feature_sup. Now that you suggest, we can remove it
as well. 

> u32 d_wb_alloc_units;
This patch is to remove it.

Thanks,
Bean

Re: [PATCH v4 3/6] scsi: ufs: Group UFS WB related flags to struct ufs_dev_info

[Stanley Chu]

On Tue, 2020-12-15 at 10:42 +0100, Bean Huo wrote:
> On Tue, 2020-12-15 at 17:01 +0800, Stanley Chu wrote:
> > > +     bool    wb_buf_flush_enabled;
> > > +     u8      wb_dedicated_lu;
> > > +     u8      b_wb_buffer_type;
> > > +     u32     d_wb_alloc_units;
> > > +
> > > +     bool    b_rpm_dev_flush_capable;
> > > +     u8      b_presrv_uspc_en;
> > 
> > Perhaps we could unify the style of these WB related stuff to wb_* ?
> 
> yes, agree. I will change them.
> 
> > 
> > Besides, I am not sure if using tab instead space between the type
> > and
> > name in this struct is a good idea.
> > 
> using space, in addition single space, type and parameter names are
> mixed. 
> 
> 
> use space:
> 
>  /* UFS WB related flags */
> bool wb_enabled;
> bool wb_buf_flush_enabled;
> u8
> wb_dedicated_lu;
> u8 b_wb_buffer_type;
> u32 d_wb_alloc_units;
> 
> use table:
> 
>  /* UFS WB related flags */
> bool    wb_enabled;
> bool    wb_buf_flush_enabled;
> u8      wb_dedicated_lu;
> u8      b_wb_buffer_type;
> u32     d_wb_alloc_units;
> 
> I think, the result is very clear comparing above two examples. yes,
> there is no explicit stipulation that we must use space or tab. Both
> styles exist in Linux. Maybe this is just matter of personal interest.

Hi Bean,

Yes, I got your point. I am fine with this style change, but just wonder
if it would be better to change all structures in all ufs headers (or at
least all structures in ufs.h) in the same time to make the style
unified in the same file?

Besides, we may need other reviewer's comments for the new style.

Thanks,
Stanley Chu

> 
> 
> Bean
> 
> > Thanks,
> > Stanley Chu
> 

Re: [PATCH v4 1/6] scsi: ufs: Add "wb_on" sysfs node to control WB on/off

[Greg KH]

On Fri, Dec 11, 2020 at 03:00:30PM +0100, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> Currently UFS WriteBooster driver uses clock scaling up/down to set
> WB on/off, for the platform which doesn't support UFSHCD_CAP_CLK_SCALING,
> WB will be always on. Provide a sysfs attribute to enable/disable WB
> during runtime. Write 1/0 to "wb_on" sysfs node to enable/disable UFS WB.
> 
> Reviewed-by: Avri Altman <avri.altman@wdc.com>
> Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/scsi/ufs/ufs-sysfs.c | 41 ++++++++++++++++++++++++++++++++++++
>  drivers/scsi/ufs/ufshcd.c    |  3 +--
>  drivers/scsi/ufs/ufshcd.h    |  2 ++
>  3 files changed, 44 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
> index 08e72b7eef6a..2b4e9fe935cc 100644
> --- a/drivers/scsi/ufs/ufs-sysfs.c
> +++ b/drivers/scsi/ufs/ufs-sysfs.c
> @@ -189,6 +189,45 @@ static ssize_t auto_hibern8_store(struct device *dev,
>  	return count;
>  }
>  
> +static ssize_t wb_on_show(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", hba->wb_enabled);

Please just use sysfs_emit().

> +}
> +
> +static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
> +			   const char *buf, size_t count)
> +{
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
> +	unsigned int wb_enable;
> +	ssize_t res;
> +
> +	if (ufshcd_is_clkscaling_supported(hba)) {
> +		/*
> +		 * If the platform supports UFSHCD_CAP_AUTO_BKOPS_SUSPEND,
> +		 * turn WB on/off will be done while clock scaling up/down.
> +		 */
> +		dev_warn(dev, "To control WB through wb_on is not allowed!\n");
> +		return -EOPNOTSUPP;
> +	}
> +	if (!ufshcd_is_wb_allowed(hba))
> +		return -EOPNOTSUPP;
> +
> +	if (kstrtouint(buf, 0, &wb_enable))
> +		return -EINVAL;
> +
> +	if (wb_enable != 0 && wb_enable != 1)
> +		return -EINVAL;
> +
> +	pm_runtime_get_sync(hba->dev);
> +	res = ufshcd_wb_ctrl(hba, wb_enable);
> +	pm_runtime_put_sync(hba->dev);
> +
> +	return res < 0 ? res : count;
> +}

Where is the new Documentation/ABI/ update for this new sysfs file you
are adding?

thanks,

greg k-h

Re: [PATCH v4 1/6] scsi: ufs: Add "wb_on" sysfs node to control WB on/off

[Bean Huo]

Greg k-h,

> >  
> > +static ssize_t wb_on_show(struct device *dev, struct
> > device_attribute *attr,
> > +			  char *buf)
> > +{
> > +	struct ufs_hba *hba = dev_get_drvdata(dev);
> > +
> > +	return scnprintf(buf, PAGE_SIZE, "%d\n", hba->wb_enabled);
> 
> Please just use sysfs_emit().

thanks, will be changed in next version.
> 
> > +}
> > +
> > +static ssize_t wb_on_store(struct device *dev, struct
> > device_attribute *attr,
> > +			   const char *buf, size_t count)
> > +{
> > +	struct ufs_hba *hba = dev_get_drvdata(dev);
> > +	unsigned int wb_enable;
> > +	ssize_t res;
> > +
> > +	if (ufshcd_is_clkscaling_supported(hba)) {
> > +		/*
> > +		 * If the platform supports
> > UFSHCD_CAP_AUTO_BKOPS_SUSPEND,
> > +		 * turn WB on/off will be done while clock scaling
> > up/down.
> > +		 */
> > +		dev_warn(dev, "To control WB through wb_on is not
> > allowed!\n");
> > +		return -EOPNOTSUPP;
> > +	}
> > +	if (!ufshcd_is_wb_allowed(hba))
> > +		return -EOPNOTSUPP;
> > +
> > +	if (kstrtouint(buf, 0, &wb_enable))
> > +		return -EINVAL;
> > +
> > +	if (wb_enable != 0 && wb_enable != 1)
> > +		return -EINVAL;
> > +
> > +	pm_runtime_get_sync(hba->dev);
> > +	res = ufshcd_wb_ctrl(hba, wb_enable);
> > +	pm_runtime_put_sync(hba->dev);
> > +
> > +	return res < 0 ? res : count;
> > +}
> 
> Where is the new Documentation/ABI/ update for this new sysfs file
> you
> are adding?
> 

It is missing, and will add it.

thanks.
Bean