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
V6:
- Change the function name from ufshcd_wb_config() to
ufshcd_configure_wb()
- modify document description for wb_buf_flush_en attribute
- fix some dev_err messages
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 | 9 +++
drivers/ufs/core/ufs-sysfs.c | 47 ++++++++++++++-
drivers/ufs/core/ufshcd-priv.h | 6 ++
drivers/ufs/core/ufshcd.c | 70 ++++++++++++----------
include/ufs/ufshcd.h | 1 +
5 files changed, 99 insertions(+), 34 deletions(-)
--
2.25.1
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 <[email protected]>
Reviewed-by: Avri Altman <[email protected]>
Reviewed-by: Bart Van Assche <[email protected]>
Signed-off-by: Jinyoung Choi <[email protected]>
---
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 69cc9b26c762..2c0b7f45de4b 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 flushing!\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 b7b8efd17659..5099d161f115 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -294,7 +294,8 @@ static void ufshcd_configure_wb(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
The location of the comment is wrong. so fix it.
Reviewed-by: Avri Altman <[email protected]>
Reviewed-by: Bart Van Assche <[email protected]>
Signed-off-by: Jinyoung Choi <[email protected]>
---
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 dcd7f03db2a2..196f964c0877 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
Messages are modified to fit the format of others.
Reviewed-by: Avri Altman <[email protected]>
Reviewed-by: Bart Van Assche <[email protected]>
Signed-off-by: Jinyoung Choi <[email protected]>
---
drivers/ufs/core/ufs-sysfs.c | 2 +-
drivers/ufs/core/ufshcd.c | 23 +++++++++++------------
2 files changed, 12 insertions(+), 13 deletions(-)
diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
index 2c0b7f45de4b..117272cf7d61 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 5099d161f115..dcd7f03db2a2 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",
- __func__, enable ? "enable" : "disable", ret);
+ dev_err(hba->dev, "%s: Write Booster %s failed %d\n",
+ __func__, enable ? "enabling" : "disabling", 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;
@@ -5757,11 +5757,11 @@ static void ufshcd_wb_toggle_buf_flush_during_h8(struct ufs_hba *hba,
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__, enable ? "enable" : "disable", ret);
+ dev_err(hba->dev, "%s: WB-Buf Flush during H8 %s failed %d\n",
+ __func__, enable ? "enabling" : "disabling", 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__,
- enable ? "enable" : "disable", ret);
+ dev_err(hba->dev, "%s: WB-Buf Flush %s failed %d\n",
+ __func__, enable ? "enabling" : "disabling", 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
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 <[email protected]>
Signed-off-by: Jinyoung Choi <[email protected]>
---
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 b7e7c0c4eb75..df00441f89a1 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_configure_wb(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_configure_wb(hba);
+
if (hba->ee_usr_mask)
ufshcd_write_ee_control(hba);
/* Enable Auto-Hibernate if configured */
--
2.25.1
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 <[email protected]>
Signed-off-by: Jinyoung Choi <[email protected]>
---
Documentation/ABI/testing/sysfs-driver-ufs | 9 +++++
drivers/ufs/core/ufs-sysfs.c | 46 ++++++++++++++++++++++
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..1255cbda0ac9 100644
--- a/Documentation/ABI/testing/sysfs-driver-ufs
+++ b/Documentation/ABI/testing/sysfs-driver-ufs
@@ -1417,6 +1417,15 @@ 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 <[email protected]>
+Description: This entry shows the status of WriteBooster buffer flushing
+ and it can be used to allow or disallow the flushing.
+ If the flushing is allowed, the device executes the flush
+ operation when the command queue is empty.
+
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..69cc9b26c762 100644
--- a/drivers/ufs/core/ufs-sysfs.c
+++ b/drivers/ufs/core/ufs-sysfs.c
@@ -254,6 +254,50 @@ 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 flushing!\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 +306,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 +317,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 df00441f89a1..b7b8efd17659 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
On Tue, Aug 2, 2022 at 4:27 PM Jinyoung CHOI <[email protected]> wrote:
>
> The location of the comment is wrong. so fix it.
>
> Reviewed-by: Avri Altman <[email protected]>
> Reviewed-by: Bart Van Assche <[email protected]>
> Signed-off-by: Jinyoung Choi <[email protected]>
Reviewed-by: Stanley Chu <[email protected]>
Hi,
On Tue, Aug 2, 2022 at 4:29 PM Jinyoung CHOI <[email protected]> wrote:
>
> Messages are modified to fit the format of others.
>
> Reviewed-by: Avri Altman <[email protected]>
> Reviewed-by: Bart Van Assche <[email protected]>
> Signed-off-by: Jinyoung Choi <[email protected]>
> ---
> drivers/ufs/core/ufs-sysfs.c | 2 +-
> drivers/ufs/core/ufshcd.c | 23 +++++++++++------------
> 2 files changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
> index 2c0b7f45de4b..117272cf7d61 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 5099d161f115..dcd7f03db2a2 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",
> - __func__, enable ? "enable" : "disable", ret);
> + dev_err(hba->dev, "%s: Write Booster %s failed %d\n",
> + __func__, enable ? "enabling" : "disabling", 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");
You need to rebase this patch to follow the latest change as
https://lore.kernel.org/all/[email protected]/
>
> return ret;
> @@ -5757,11 +5757,11 @@ static void ufshcd_wb_toggle_buf_flush_during_h8(struct ufs_hba *hba,
> 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__, enable ? "enable" : "disable", ret);
> + dev_err(hba->dev, "%s: WB-Buf Flush during H8 %s failed %d\n",
> + __func__, enable ? "enabling" : "disabling", 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__,
> - enable ? "enable" : "disable", ret);
> + dev_err(hba->dev, "%s: WB-Buf Flush %s failed %d\n",
> + __func__, enable ? "enabling" : "disabling", 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
--
Yours truly,
朱原陞 (Stanley Chu)
Reviewed-by: Stanley Chu <[email protected]>
Hi, Stanley,
>Hi,
>
>On Tue, Aug 2, 2022 at 4:29 PM Jinyoung CHOI <[email protected]> wrote:
>>
>> Messages are modified to fit the format of others.
>>
>> Reviewed-by: Avri Altman <[email protected]>
>> Reviewed-by: Bart Van Assche <[email protected]>
>> Signed-off-by: Jinyoung Choi <[email protected]>
>> ---
>> drivers/ufs/core/ufs-sysfs.c | 2 +-
>> drivers/ufs/core/ufshcd.c | 23 +++++++++++------------
>> 2 files changed, 12 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
>> index 2c0b7f45de4b..117272cf7d61 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 5099d161f115..dcd7f03db2a2 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",
>> - __func__, enable ? "enable" : "disable", ret);
>> + dev_err(hba->dev, "%s: Write Booster %s failed %d\n",
>> + __func__, enable ? "enabling" : "disabling", 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");
>
>You need to rebase this patch to follow the latest change as
>https://lore.kernel.org/all/[email protected]/
I am currently working on the latest 5.20/scsi-staging.
In this case, can I refer the commit of 5.19/scsi-fixes
and add commit to 5.20/scsi-staging?
Or can I reflect it in my change?
I have no experience, so please guide. :)
Thank you for checking.
Jinyoung.
hi Jinyoung,
On Wed, Aug 3, 2022 at 12:11 PM Jinyoung CHOI <[email protected]> wrote:
>
> Hi, Stanley,
>
> >Hi,
> >
> >On Tue, Aug 2, 2022 at 4:29 PM Jinyoung CHOI <[email protected]> wrote:
> >>
> >> Messages are modified to fit the format of others.
> >>
> >> Reviewed-by: Avri Altman <[email protected]>
> >> Reviewed-by: Bart Van Assche <[email protected]>
> >> Signed-off-by: Jinyoung Choi <[email protected]>
> >> ---
> >> drivers/ufs/core/ufs-sysfs.c | 2 +-
> >> drivers/ufs/core/ufshcd.c | 23 +++++++++++------------
> >> 2 files changed, 12 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
> >> index 2c0b7f45de4b..117272cf7d61 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 5099d161f115..dcd7f03db2a2 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",
> >> - __func__, enable ? "enable" : "disable", ret);
> >> + dev_err(hba->dev, "%s: Write Booster %s failed %d\n",
> >> + __func__, enable ? "enabling" : "disabling", 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");
> >
> >You need to rebase this patch to follow the latest change as
> >https://lore.kernel.org/all/[email protected]/
>
> I am currently working on the latest 5.20/scsi-staging.
> In this case, can I refer the commit of 5.19/scsi-fixes
> and add commit to 5.20/scsi-staging?
> Or can I reflect it in my change?
> I have no experience, so please guide. :)
>
> Thank you for checking.
> Jinyoung.
>
>
I did not notice that the below patch is merged to 5.19/scsi-fixes, so
I guess perhaps you could keep working on the latest
5.20/scsi-staging, and then Martin would help resolve the conflict.
https://lore.kernel.org/all/[email protected]/
Thanks,
Stanley
>hi Jinyoung,
>
>On Wed, Aug 3, 2022 at 12:11 PM Jinyoung CHOI <[email protected]> wrote:
>>
>> Hi, Stanley,
>>
>> >Hi,
>> >
>> >On Tue, Aug 2, 2022 at 4:29 PM Jinyoung CHOI <[email protected]> wrote:
>> >>
>> >> Messages are modified to fit the format of others.
>> >>
>> >> Reviewed-by: Avri Altman <[email protected]>
>> >> Reviewed-by: Bart Van Assche <[email protected]>
>> >> Signed-off-by: Jinyoung Choi <[email protected]>
>> >> ---
>> >> drivers/ufs/core/ufs-sysfs.c | 2 +-
>> >> drivers/ufs/core/ufshcd.c | 23 +++++++++++------------
>> >> 2 files changed, 12 insertions(+), 13 deletions(-)
>> >>
>> >> diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
>> >> index 2c0b7f45de4b..117272cf7d61 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 5099d161f115..dcd7f03db2a2 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",
>> >> - __func__, enable ? "enable" : "disable", ret);
>> >> + dev_err(hba->dev, "%s: Write Booster %s failed %d\n",
>> >> + __func__, enable ? "enabling" : "disabling", 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");
>> >
>> >You need to rebase this patch to follow the latest change as
>> >https://lore.kernel.org/all/[email protected]/
>>
>> I am currently working on the latest 5.20/scsi-staging.
>> In this case, can I refer the commit of 5.19/scsi-fixes
>> and add commit to 5.20/scsi-staging?
>> Or can I reflect it in my change?
>> I have no experience, so please guide. :)
>>
>> Thank you for checking.
>> Jinyoung.
>>
>>
>
>I did not notice that the below patch is merged to 5.19/scsi-fixes, so
>I guess perhaps you could keep working on the latest
>5.20/scsi-staging, and then Martin would help resolve the conflict.
>https://lore.kernel.org/all/[email protected]/
>
>Thanks,
>
>Stanley
It has been applied to 5.19/scsi-fixes,
so I will modify the debug level at my change for sync.
Best Regards,
Jinyoung.
On 8/2/22 01:07, Jinyoung CHOI wrote:
> +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 <[email protected]>
> +Description: This entry shows the status of WriteBooster buffer flushing
Can we rename this attribute into something that has a word order that
is grammatically correct, e.g. enable_wb_buf_flush?
> + and it can be used to allow or disallow the flushing.
> + If the flushing is allowed, the device executes the flush
> + operation when the command queue is empty.
The attribute has "enabled" in its name while the above text uses the
verb "allowed". Consider changing "allowed" into "enabled". Please also
change "If the flushing" into "If flushing".
Thanks,
Bart.
On 8/2/22 01:05, Jinyoung CHOI wrote:
> The parameter name of ufshcd_wb_toggle_flush_during_h8() has been changed
> in the same as other toggle functions.
Reviewed-by: Bart Van Assche <[email protected]>
On 8/2/22 01:08, Jinyoung CHOI wrote:
> Changed to improve readability.
The above description is a bit too cryptic. Consider changing it into
"Introduce ufshcd_is_wb_buf_flush_allowed() to improve readability."
Thanks,
Bart.
>On 8/2/22 01:07, Jinyoung CHOI wrote:
>> +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 <[email protected]>
>> +Description: This entry shows the status of WriteBooster buffer flushing
>
>Can we rename this attribute into something that has a word order that
>is grammatically correct, e.g. enable_wb_buf_flush?
>
OK, I will replace it.
Instead, When the list is printed through "ls",
it may be difficult to check because the prefix is different.
>> + and it can be used to allow or disallow the flushing.
>> + If the flushing is allowed, the device executes the flush
>> + operation when the command queue is empty.
>
>The attribute has "enabled" in its name while the above text uses the
>verb "allowed". Consider changing "allowed" into "enabled". Please also
>change "If the flushing" into "If flushing".
>
>Thanks,
>
>Bart
OK, I will apply next patch.
Warm Regards,
Jinyoung.