[PATCH v5 0/6] scsi: ufs: wb: Add sysfs attribute and cleanup

[PATCH v5 0/6] scsi: ufs: wb: Add sysfs attribute and cleanup

[Jinyoung CHOI]

This patch series is to clean up UFS's Write Booster code and
adds sysfs attribute which can control the specific feature of it.

V2:
	- modify commit message
	- move & modify err messages
	- remove unnesscessary debug messages
V3:
	- split patch (functional, non-functional)
V4:
	- split patch (The number of patches from 2 to 7)
	- modify dev messages
	- modify commit message
V5:
	- drop (scsi: ufs: wb: Move ufshcd_is_wb_allowed() to callee)
	- fix condition check
	- add Document for sysfs attribute
	- move ufshcd_is_wb_buf_flush_allowed() to ufs-priv.h

Jinyoung Choi (6):
  scsi: ufs: wb: Change wb_enabled condition test
  scsi: ufs: wb: Change functions name and modify parameter name
  scsi: ufs: wb: Add explicit flush sysfs attribute
  scsi: ufs: wb: Add ufshcd_is_wb_buf_flush_allowed()
  scsi: ufs: wb: Modify messages
  scsi: ufs: wb: Move the comment to the right position

 Documentation/ABI/testing/sysfs-driver-ufs | 10 ++++
 drivers/ufs/core/ufs-sysfs.c               | 46 +++++++++++++++-
 drivers/ufs/core/ufshcd-priv.h             |  6 ++
 drivers/ufs/core/ufshcd.c                  | 64 ++++++++++++----------
 include/ufs/ufshcd.h                       |  1 +
 5 files changed, 96 insertions(+), 31 deletions(-)

-- 
2.25.1

[PATCH v5 1/6] scsi: ufs: wb: Change wb_enabled condition test

[Jinyoung CHOI]

Changed to improve readability.
As implemented in ufshcd_wb_togle_flush(), the conditional test is
modified in the same way.

Reviewed-by: Avri Altman <avri.altman@wdc.com>
Reviewed-by: Bean Huo <beanhuo@micron.com>
Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com>
---
 drivers/ufs/core/ufshcd.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 8f11f118c30e..bbf12aa6a5ae 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -5730,10 +5730,8 @@ int ufshcd_wb_toggle(struct ufs_hba *hba, bool enable)
 {
 	int ret;
 
-	if (!ufshcd_is_wb_allowed(hba))
-		return 0;
-
-	if (!(enable ^ hba->dev_info.wb_enabled))
+	if (!ufshcd_is_wb_allowed(hba) ||
+	    hba->dev_info.wb_enabled == enable)
 		return 0;
 
 	ret = __ufshcd_wb_toggle(hba, enable, QUERY_FLAG_IDN_WB_EN);
-- 
2.25.1

Re: [PATCH v5 1/6] scsi: ufs: wb: Change wb_enabled condition test

[Bart Van Assche]

On 7/28/22 21:52, Jinyoung CHOI wrote:
> Changed to improve readability.
> As implemented in ufshcd_wb_togle_flush(), the conditional test is
> modified in the same way.
> 
> Reviewed-by: Avri Altman <avri.altman@wdc.com>
> Reviewed-by: Bean Huo <beanhuo@micron.com>
> Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com>
> ---
>   drivers/ufs/core/ufshcd.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 8f11f118c30e..bbf12aa6a5ae 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -5730,10 +5730,8 @@ int ufshcd_wb_toggle(struct ufs_hba *hba, bool enable)
>   {
>   	int ret;
>   
> -	if (!ufshcd_is_wb_allowed(hba))
> -		return 0;
> -
> -	if (!(enable ^ hba->dev_info.wb_enabled))
> +	if (!ufshcd_is_wb_allowed(hba) ||
> +	    hba->dev_info.wb_enabled == enable)
>   		return 0;
>   
>   	ret = __ufshcd_wb_toggle(hba, enable, QUERY_FLAG_IDN_WB_EN);

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH v5 1/6] scsi: ufs: wb: Change wb_enabled condition test

[Stanley Chu]

On Fri, Jul 29, 2022 at 12:54 PM Jinyoung CHOI <j-young.choi@samsung.com> wrote:
>
> Changed to improve readability.
> As implemented in ufshcd_wb_togle_flush(), the conditional test is
> modified in the same way.
>
> Reviewed-by: Avri Altman <avri.altman@wdc.com>
> Reviewed-by: Bean Huo <beanhuo@micron.com>
> Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com>

Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>

[PATCH v5 2/6] scsi: ufs: wb: Change functions name and modify parameter name

[Jinyoung CHOI]

The parameter name of ufshcd_wb_toggle_flush_during_h8() has been changed
in the same as other toggle functions.

Function names were ambiguous. So changed to suit the meaning.

Reviewed-by: Avri Altman <avri.altman@wdc.com>
Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com>
---
 drivers/ufs/core/ufshcd.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index bbf12aa6a5ae..a14d3fe39570 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -265,8 +265,9 @@ 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 void 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_wb_toggle_buf_flush_during_h8(struct ufs_hba *hba,
+						 bool enable);
+static void ufshcd_wb_toggle_buf_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);
 
@@ -286,16 +287,16 @@ static inline void ufshcd_disable_irq(struct ufs_hba *hba)
 	}
 }
 
-static inline void ufshcd_wb_config(struct ufs_hba *hba)
+static void ufshcd_wb_set_default_flags(struct ufs_hba *hba)
 {
 	if (!ufshcd_is_wb_allowed(hba))
 		return;
 
 	ufshcd_wb_toggle(hba, true);
 
-	ufshcd_wb_toggle_flush_during_h8(hba, true);
+	ufshcd_wb_toggle_buf_flush_during_h8(hba, true);
 	if (!(hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL))
-		ufshcd_wb_toggle_flush(hba, true);
+		ufshcd_wb_toggle_buf_flush(hba, true);
 }
 
 static void ufshcd_scsi_unblock_requests(struct ufs_hba *hba)
@@ -5748,22 +5749,23 @@ int ufshcd_wb_toggle(struct ufs_hba *hba, bool enable)
 	return ret;
 }
 
-static void ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set)
+static void ufshcd_wb_toggle_buf_flush_during_h8(struct ufs_hba *hba,
+						 bool enable)
 {
 	int ret;
 
-	ret = __ufshcd_wb_toggle(hba, set,
+	ret = __ufshcd_wb_toggle(hba, enable,
 			QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8);
 	if (ret) {
 		dev_err(hba->dev, "%s: WB-Buf Flush during H8 %s failed: %d\n",
-			__func__, set ? "enable" : "disable", ret);
+			__func__, enable ? "enable" : "disable", ret);
 		return;
 	}
 	dev_dbg(hba->dev, "%s WB-Buf Flush during H8 %s\n",
-			__func__, set ? "enabled" : "disabled");
+			__func__, enable ? "enabled" : "disabled");
 }
 
-static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable)
+static void ufshcd_wb_toggle_buf_flush(struct ufs_hba *hba, bool enable)
 {
 	int ret;
 
@@ -5813,9 +5815,9 @@ static bool ufshcd_wb_presrv_usrspc_keep_vcc_on(struct ufs_hba *hba,
 static void ufshcd_wb_force_disable(struct ufs_hba *hba)
 {
 	if (!(hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL))
-		ufshcd_wb_toggle_flush(hba, false);
+		ufshcd_wb_toggle_buf_flush(hba, false);
 
-	ufshcd_wb_toggle_flush_during_h8(hba, false);
+	ufshcd_wb_toggle_buf_flush_during_h8(hba, false);
 	ufshcd_wb_toggle(hba, false);
 	hba->caps &= ~UFSHCD_CAP_WB_EN;
 
@@ -8212,7 +8214,9 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
 	 */
 	ufshcd_set_active_icc_lvl(hba);
 
-	ufshcd_wb_config(hba);
+	/* Enable UFS Write Booster if supported */
+	ufshcd_wb_set_default_flags(hba);
+
 	if (hba->ee_usr_mask)
 		ufshcd_write_ee_control(hba);
 	/* Enable Auto-Hibernate if configured */
-- 
2.25.1

Re: [PATCH v5 2/6] scsi: ufs: wb: Change functions name and modify parameter name

[Bart Van Assche]

On 7/28/22 21:54, Jinyoung CHOI wrote:
> -static inline void ufshcd_wb_config(struct ufs_hba *hba)
> +static void ufshcd_wb_set_default_flags(struct ufs_hba *hba)

It is not clear to me why the new name is considered an improvement? The 
old name looks better to me. If you want to rename this function anyway, 
how about ufshcd_configure_wb()?

Thanks,

Bart.

RE:(2) [PATCH v5 2/6] scsi: ufs: wb: Change functions name and modify parameter name

[Jinyoung CHOI]

>On 7/28/22 21:54, Jinyoung CHOI wrote:
>> -static inline void ufshcd_wb_config(struct ufs_hba *hba)
>> +static void ufshcd_wb_set_default_flags(struct ufs_hba *hba)
>
>It is not clear to me why the new name is considered an improvement? The 
>old name looks better to me. If you want to rename this function anyway, 
>how about ufshcd_configure_wb()?
>
>Thanks,
>
>Bart.

It is desirable to express it as configure because it sets several values.
I will change it.
Thank you for your opinion.

Kind Regards,
Jinyoung.

[PATCH v5 3/6] scsi: ufs: wb: Add explicit flush sysfs attribute

[Jinyoung CHOI]

There is the following quirk to bypass "WB Flush" in Write Booster.

	- UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL

If this quirk is not set, there is no knob that can control "WB Flush".

There are three flags that control Write Booster Feature.
	1. WB ON/OFF
	2. WB Hibern Flush ON/OFF (implicitly)
	3. WB Flush ON/OFF (explicit)

The sysfs attribute that controls the WB was implemented. (1)

In the case of "Hibern Flush", it is always good to turn on.
Control may not be required. (2)

Finally, "Flush" may be necessary because the Auto-Hibern8 is not
supported in a specific environment.
So the sysfs attribute that controls this is necessary. (3)

Reviewed-by: Avri Altman <avri.altman@wdc.com>
Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com>
---
 Documentation/ABI/testing/sysfs-driver-ufs | 10 +++++
 drivers/ufs/core/ufs-sysfs.c               | 45 ++++++++++++++++++++++
 drivers/ufs/core/ufshcd.c                  |  9 +++--
 include/ufs/ufshcd.h                       |  1 +
 4 files changed, 61 insertions(+), 4 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
index 6b248abb1bd7..7e9e1db55d60 100644
--- a/Documentation/ABI/testing/sysfs-driver-ufs
+++ b/Documentation/ABI/testing/sysfs-driver-ufs
@@ -1417,6 +1417,16 @@ Description:	This node is used to set or display whether UFS WriteBooster is
 		platform that doesn't support UFSHCD_CAP_CLK_SCALING, we can
 		disable/enable WriteBooster through this sysfs node.
 
+What:		/sys/bus/platform/drivers/ufshcd/*/wb_buf_flush_en
+What:		/sys/bus/platform/devices/*.ufs/wb_buf_flush_en
+Date:		July 2022
+Contact:	Jinyoung Choi <j-young.choi@samsung.com>
+Description:	This node is used to set or display whether WriteBooster
+		buffer flusing is enabled. The data written in the WriteBooster
+		Buffer can be flushed by an explicit host command or
+		implicitly while in hibernate (HIBERN8) state. We can control
+		the explicit flushing through this sysfs node.
+
 What:		/sys/bus/platform/drivers/ufshcd/*/device_descriptor/hpb_version
 What:		/sys/bus/platform/devices/*.ufs/device_descriptor/hpb_version
 Date:		June 2021
diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
index 0a088b47d557..920094665f4d 100644
--- a/drivers/ufs/core/ufs-sysfs.c
+++ b/drivers/ufs/core/ufs-sysfs.c
@@ -254,6 +254,49 @@ static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
 	return res < 0 ? res : count;
 }
 
+static ssize_t wb_buf_flush_en_show(struct device *dev,
+				    struct device_attribute *attr,
+				    char *buf)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%d\n", hba->dev_info.wb_buf_flush_enabled);
+}
+
+static ssize_t wb_buf_flush_en_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_buf_flush_en;
+	ssize_t res;
+
+	if (!ufshcd_is_wb_allowed(hba) ||
+	    (hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)) {
+		dev_warn(dev, "It is not allowed to configure WB buf flush!\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (kstrtouint(buf, 0, &wb_buf_flush_en))
+		return -EINVAL;
+
+	if (wb_buf_flush_en != 0 && wb_buf_flush_en != 1)
+		return -EINVAL;
+
+	down(&hba->host_sem);
+	if (!ufshcd_is_user_access_allowed(hba)) {
+		res = -EBUSY;
+		goto out;
+	}
+
+	ufshcd_rpm_get_sync(hba);
+	res = ufshcd_wb_toggle_buf_flush(hba, wb_buf_flush_en);
+	ufshcd_rpm_put_sync(hba);
+out:
+	up(&hba->host_sem);
+	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);
@@ -262,6 +305,7 @@ 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 DEVICE_ATTR_RW(wb_buf_flush_en);
 
 static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
 	&dev_attr_rpm_lvl.attr,
@@ -272,6 +316,7 @@ static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
 	&dev_attr_spm_target_link_state.attr,
 	&dev_attr_auto_hibern8.attr,
 	&dev_attr_wb_on.attr,
+	&dev_attr_wb_buf_flush_en.attr,
 	NULL
 };
 
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index a14d3fe39570..36d4708eaee0 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -267,7 +267,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 void ufshcd_wb_toggle_buf_flush_during_h8(struct ufs_hba *hba,
 						 bool enable);
-static void ufshcd_wb_toggle_buf_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);
 
@@ -5765,25 +5764,27 @@ static void ufshcd_wb_toggle_buf_flush_during_h8(struct ufs_hba *hba,
 			__func__, enable ? "enabled" : "disabled");
 }
 
-static void ufshcd_wb_toggle_buf_flush(struct ufs_hba *hba, bool enable)
+int ufshcd_wb_toggle_buf_flush(struct ufs_hba *hba, bool enable)
 {
 	int ret;
 
 	if (!ufshcd_is_wb_allowed(hba) ||
 	    hba->dev_info.wb_buf_flush_enabled == enable)
-		return;
+		return 0;
 
 	ret = __ufshcd_wb_toggle(hba, enable, QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN);
 	if (ret) {
 		dev_err(hba->dev, "%s WB-Buf Flush %s failed %d\n", __func__,
 			enable ? "enable" : "disable", ret);
-		return;
+		return ret;
 	}
 
 	hba->dev_info.wb_buf_flush_enabled = enable;
 
 	dev_dbg(hba->dev, "%s WB-Buf Flush %s\n",
 			__func__, enable ? "enabled" : "disabled");
+
+	return ret;
 }
 
 static bool ufshcd_wb_presrv_usrspc_keep_vcc_on(struct ufs_hba *hba,
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 7fe1a926cd99..94bcfec98fb8 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -1211,6 +1211,7 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
 			     enum query_opcode desc_op);
 
 int ufshcd_wb_toggle(struct ufs_hba *hba, bool enable);
+int ufshcd_wb_toggle_buf_flush(struct ufs_hba *hba, bool enable);
 int ufshcd_suspend_prepare(struct device *dev);
 int __ufshcd_suspend_prepare(struct device *dev, bool rpm_ok_for_spm);
 void ufshcd_resume_complete(struct device *dev);
-- 
2.25.1

Re: [PATCH v5 3/6] scsi: ufs: wb: Add explicit flush sysfs attribute

[Bart Van Assche]

On 7/28/22 21:56, Jinyoung CHOI wrote:
> diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
> index 6b248abb1bd7..7e9e1db55d60 100644
> --- a/Documentation/ABI/testing/sysfs-driver-ufs
> +++ b/Documentation/ABI/testing/sysfs-driver-ufs
> @@ -1417,6 +1417,16 @@ Description:	This node is used to set or display whether UFS WriteBooster is
>   		platform that doesn't support UFSHCD_CAP_CLK_SCALING, we can
>   		disable/enable WriteBooster through this sysfs node.
>   
> +What:		/sys/bus/platform/drivers/ufshcd/*/wb_buf_flush_en
> +What:		/sys/bus/platform/devices/*.ufs/wb_buf_flush_en
> +Date:		July 2022
> +Contact:	Jinyoung Choi <j-young.choi@samsung.com>
> +Description:	This node is used to set or display whether WriteBooster
                      ^^^^

Please change "node" into "attribute" (here and below). Sysfs files are 
called attributes.

> +		buffer flusing is enabled. The data written in the WriteBooster
                        ^^^^^^^
                        flushing?
> +		Buffer can be flushed by an explicit host command or
> +		implicitly while in hibernate (HIBERN8) state.

The above sentence is misleading because it suggests that setting this 
attribute causes the WB buffer to be flushed in its entirety. That is 
not correct - what this attribute controls is whether or not the UFS 
device is allowed to start with flushing the WB buffer.

> +	if (!ufshcd_is_wb_allowed(hba) ||
> +	    (hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)) {
> +		dev_warn(dev, "It is not allowed to configure WB buf flush!\n");

flush -> flushing

> +	ufshcd_rpm_get_sync(hba);
> +	res = ufshcd_wb_toggle_buf_flush(hba, wb_buf_flush_en);
> +	ufshcd_rpm_put_sync(hba);
> +out:
> +	up(&hba->host_sem);
> +	return res < 0 ? res : count;
> +}

Please leave a blank line above goto labels as requested by the kernel 
coding style guide.

Thanks,

Bart.

[PATCH v5 5/6] scsi: ufs: wb: Modify messages

[Jinyoung CHOI]

Messages are modified to fit the format of others.

Reviewed-by: Avri Altman <avri.altman@wdc.com>
Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com>
---
 drivers/ufs/core/ufs-sysfs.c |  2 +-
 drivers/ufs/core/ufshcd.c    | 15 +++++++--------
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
index 63b02b2541c8..35e4f0056de6 100644
--- a/drivers/ufs/core/ufs-sysfs.c
+++ b/drivers/ufs/core/ufs-sysfs.c
@@ -230,7 +230,7 @@ static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
 		 * If the platform supports UFSHCD_CAP_CLK_SCALING, 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");
+		dev_warn(dev, "It is not allowed to configure WB!\n");
 		return -EOPNOTSUPP;
 	}
 
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 584e246c769a..309f93122078 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -5737,13 +5737,13 @@ int ufshcd_wb_toggle(struct ufs_hba *hba, bool enable)
 
 	ret = __ufshcd_wb_toggle(hba, enable, QUERY_FLAG_IDN_WB_EN);
 	if (ret) {
-		dev_err(hba->dev, "%s Write Booster %s failed %d\n",
+		dev_err(hba->dev, "%s: Write Booster %s failed %d\n",
 			__func__, enable ? "enable" : "disable", ret);
 		return ret;
 	}
 
 	hba->dev_info.wb_enabled = enable;
-	dev_info(hba->dev, "%s Write Booster %s\n",
+	dev_info(hba->dev, "%s: Write Booster %s\n",
 			__func__, enable ? "enabled" : "disabled");
 
 	return ret;
@@ -5761,7 +5761,7 @@ static void ufshcd_wb_toggle_buf_flush_during_h8(struct ufs_hba *hba,
 			__func__, enable ? "enable" : "disable", ret);
 		return;
 	}
-	dev_dbg(hba->dev, "%s WB-Buf Flush during H8 %s\n",
+	dev_info(hba->dev, "%s: WB-Buf Flush during H8 %s\n",
 			__func__, enable ? "enabled" : "disabled");
 }
 
@@ -5775,14 +5775,13 @@ int ufshcd_wb_toggle_buf_flush(struct ufs_hba *hba, bool enable)
 
 	ret = __ufshcd_wb_toggle(hba, enable, QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN);
 	if (ret) {
-		dev_err(hba->dev, "%s WB-Buf Flush %s failed %d\n", __func__,
+		dev_err(hba->dev, "%s: WB-Buf Flush %s failed %d\n", __func__,
 			enable ? "enable" : "disable", ret);
 		return ret;
 	}
 
 	hba->dev_info.wb_buf_flush_enabled = enable;
-
-	dev_dbg(hba->dev, "%s WB-Buf Flush %s\n",
+	dev_info(hba->dev, "%s: WB-Buf Flush %s\n",
 			__func__, enable ? "enabled" : "disabled");
 
 	return ret;
@@ -5800,7 +5799,7 @@ static bool ufshcd_wb_presrv_usrspc_keep_vcc_on(struct ufs_hba *hba,
 					      QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE,
 					      index, 0, &cur_buf);
 	if (ret) {
-		dev_err(hba->dev, "%s dCurWriteBoosterBufferSize read failed %d\n",
+		dev_err(hba->dev, "%s: dCurWriteBoosterBufferSize read failed %d\n",
 			__func__, ret);
 		return false;
 	}
@@ -5885,7 +5884,7 @@ static bool ufshcd_wb_need_flush(struct ufs_hba *hba)
 				      QUERY_ATTR_IDN_AVAIL_WB_BUFF_SIZE,
 				      index, 0, &avail_buf);
 	if (ret) {
-		dev_warn(hba->dev, "%s dAvailableWriteBoosterBufferSize read failed %d\n",
+		dev_warn(hba->dev, "%s: dAvailableWriteBoosterBufferSize read failed %d\n",
 			 __func__, ret);
 		return false;
 	}
-- 
2.25.1

Re: [PATCH v5 5/6] scsi: ufs: wb: Modify messages

[Bart Van Assche]

On 7/28/22 21:57, Jinyoung CHOI wrote:
>   	ret = __ufshcd_wb_toggle(hba, enable, QUERY_FLAG_IDN_WB_EN);
>   	if (ret) {
> -		dev_err(hba->dev, "%s Write Booster %s failed %d\n",
> +		dev_err(hba->dev, "%s: Write Booster %s failed %d\n",
>   			__func__, enable ? "enable" : "disable", ret);
>   		return ret;
>   	}

Please also fix the grammar (enable -> enabling; disable -> disabling).

Otherwise this patch looks good to me.

Thanks,

Bart.

RE:(2) [PATCH v5 5/6] scsi: ufs: wb: Modify messages

[Jinyoung CHOI]

>On 7/28/22 21:57, Jinyoung CHOI wrote:
>>   	ret = __ufshcd_wb_toggle(hba, enable, QUERY_FLAG_IDN_WB_EN);
>>   	if (ret) {
>> -		dev_err(hba->dev, "%s Write Booster %s failed %d\n",
>> +		dev_err(hba->dev, "%s: Write Booster %s failed %d\n",
>>   			__func__, enable ? "enable" : "disable", ret);
>>   		return ret;
>>   	}
>
>Please also fix the grammar (enable -> enabling; disable -> disabling).
>
>Otherwise this patch looks good to me.
>
>Thanks,
>
>Bart.

OK, I will fix it. 

Thanks,
Jinyoung.

[PATCH v5 6/6] scsi: ufs: wb: Move the comment to the right position

[Jinyoung CHOI]

The location of the comment is wrong. so fix it.

Reviewed-by: Avri Altman <avri.altman@wdc.com>
Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com>
---
 drivers/ufs/core/ufshcd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 309f93122078..4120617ff7a6 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -1298,9 +1298,10 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up)
 		}
 	}
 
-	/* Enable Write Booster if we have scaled up else disable it */
 	downgrade_write(&hba->clk_scaling_lock);
 	is_writelock = false;
+
+	/* Enable Write Booster if we have scaled up else disable it */
 	ufshcd_wb_toggle(hba, scale_up);
 
 out_unprepare:
-- 
2.25.1

Re: [PATCH v5 6/6] scsi: ufs: wb: Move the comment to the right position

[Bart Van Assche]

On 7/28/22 21:59, Jinyoung CHOI wrote:
> The location of the comment is wrong. so fix it.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

[PATCH v5 4/6] scsi: ufs: wb: Add ufshcd_is_wb_buf_flush_allowed()

[Jinyoung CHOI]

The explicit flushing should check the following.
	- UFSHCD_CAP_WB_EN
	- UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL

Changed to improve readability.

Acked-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Avri Altman <avri.altman@wdc.com>
Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com>
---
 drivers/ufs/core/ufs-sysfs.c   | 3 +--
 drivers/ufs/core/ufshcd-priv.h | 6 ++++++
 drivers/ufs/core/ufshcd.c      | 5 +++--
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
index 920094665f4d..63b02b2541c8 100644
--- a/drivers/ufs/core/ufs-sysfs.c
+++ b/drivers/ufs/core/ufs-sysfs.c
@@ -271,8 +271,7 @@ static ssize_t wb_buf_flush_en_store(struct device *dev,
 	unsigned int wb_buf_flush_en;
 	ssize_t res;
 
-	if (!ufshcd_is_wb_allowed(hba) ||
-	    (hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)) {
+	if (!ufshcd_is_wb_buf_flush_allowed(hba)) {
 		dev_warn(dev, "It is not allowed to configure WB buf flush!\n");
 		return -EOPNOTSUPP;
 	}
diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index 8f67db202d7b..d00dba17297d 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -26,6 +26,12 @@ static inline u8 ufshcd_wb_get_query_index(struct ufs_hba *hba)
 	return 0;
 }
 
+static inline bool ufshcd_is_wb_buf_flush_allowed(struct ufs_hba *hba)
+{
+	return ufshcd_is_wb_allowed(hba) &&
+		!(hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL);
+}
+
 #ifdef CONFIG_SCSI_UFS_HWMON
 void ufs_hwmon_probe(struct ufs_hba *hba, u8 mask);
 void ufs_hwmon_remove(struct ufs_hba *hba);
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 36d4708eaee0..584e246c769a 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -294,7 +294,8 @@ static void ufshcd_wb_set_default_flags(struct ufs_hba *hba)
 	ufshcd_wb_toggle(hba, true);
 
 	ufshcd_wb_toggle_buf_flush_during_h8(hba, true);
-	if (!(hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL))
+
+	if (ufshcd_is_wb_buf_flush_allowed(hba))
 		ufshcd_wb_toggle_buf_flush(hba, true);
 }
 
@@ -5815,7 +5816,7 @@ static bool ufshcd_wb_presrv_usrspc_keep_vcc_on(struct ufs_hba *hba,
 
 static void ufshcd_wb_force_disable(struct ufs_hba *hba)
 {
-	if (!(hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL))
+	if (ufshcd_is_wb_buf_flush_allowed(hba))
 		ufshcd_wb_toggle_buf_flush(hba, false);
 
 	ufshcd_wb_toggle_buf_flush_during_h8(hba, false);
-- 
2.25.1

Re: [PATCH v5 4/6] scsi: ufs: wb: Add ufshcd_is_wb_buf_flush_allowed()

[Bart Van Assche]

On 7/28/22 21:56, Jinyoung CHOI wrote:
> The explicit flushing should check the following.
> 	- UFSHCD_CAP_WB_EN
> 	- UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL
> 
> Changed to improve readability.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

RE:(2) [PATCH v5 3/6] scsi: ufs: wb: Add explicit flush sysfs attribute

[Jinyoung CHOI]

>On 7/28/22 21:56, Jinyoung CHOI wrote:
>> diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
>> index 6b248abb1bd7..7e9e1db55d60 100644
>> --- a/Documentation/ABI/testing/sysfs-driver-ufs
>> +++ b/Documentation/ABI/testing/sysfs-driver-ufs
>> @@ -1417,6 +1417,16 @@ Description:	This node is used to set or display whether UFS WriteBooster is
>>   		platform that doesn't support UFSHCD_CAP_CLK_SCALING, we can
>>   		disable/enable WriteBooster through this sysfs node.
>>   
>> +What:		/sys/bus/platform/drivers/ufshcd/*/wb_buf_flush_en
>> +What:		/sys/bus/platform/devices/*.ufs/wb_buf_flush_en
>> +Date:		July 2022
>> +Contact:	Jinyoung Choi <j-young.choi@samsung.com>
>> +Description:	This node is used to set or display whether WriteBooster
>                      ^^^^
>
>Please change "node" into "attribute" (here and below). Sysfs files are 
>called attributes.
>

The wb_on description is also written as node.
This will also be changed to other commit.


>> +		buffer flusing is enabled. The data written in the WriteBooster
>                        ^^^^^^^
>                        flushing?
>> +		Buffer can be flushed by an explicit host command or
>> +		implicitly while in hibernate (HIBERN8) state.
>
>The above sentence is misleading because it suggests that setting this 
>attribute causes the WB buffer to be flushed in its entirety. That is 
>not correct - what this attribute controls is whether or not the UFS 
>device is allowed to start with flushing the WB buffer.
>

It seems to have been written so briefly.
Because each device manufacturer may have different flush policies,
So, I did not describe the amount. This is the same as the ufs spec.
I will add more explanation.
And, I don't fully understand your comment (That is not correct ~)
If you explain it more, I will consider it.


>> +	if (!ufshcd_is_wb_allowed(hba) ||
>> +	    (hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)) {
>> +		dev_warn(dev, "It is not allowed to configure WB buf flush!\n");
>
>flush -> flushing
>

OK, I will fix it.


>> +	ufshcd_rpm_get_sync(hba);
>> +	res = ufshcd_wb_toggle_buf_flush(hba, wb_buf_flush_en);
>> +	ufshcd_rpm_put_sync(hba);
>> +out:
>> +	up(&hba->host_sem);
>> +	return res < 0 ? res : count;
>> +}
>
>Please leave a blank line above goto labels as requested by the kernel 
>coding style guide.
>
>Thanks,
>
>Bart.

OK, I will fix it.
wb_on_store() will also be modified with this. (other commit)

Kind Regards,
Jinyoung