* [PATCH] scsi: ufs: wb: Add Manual Flush sysfs and cleanup toggle functions
[not found] <CGME20220422120240epcms2p24bdcb416becf76b417f7c39006aa40f2@epcms2p1>
@ 2022-04-22 12:14 ` Jinyoung CHOI
2022-04-22 13:46 ` Avri Altman
2022-04-22 17:25 ` Bart Van Assche
0 siblings, 2 replies; 7+ messages in thread
From: Jinyoung CHOI @ 2022-04-22 12:14 UTC (permalink / raw)
To: ALIM AKHTAR, avri.altman, jejb, martin.petersen, bvanassche,
beanhuo, Daejun Park, adrian.hunter, cang, asutoshd, linux-scsi,
linux-kernel
There is the following quirk to bypass "WB Manual Flush" in Write
Booster.
- UFSHCD_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL
If this quirk is not set, there is no knob that can controll "WB Manual Flush".
There are three flags that control Write Booster Feature.
1. WB ON/OFF
2. WB Hibern Flush ON/OFF
3. WB Flush ON/OFF
The sysfs 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, "Manual flush" may be determined that it can affect
performance or power consumption.
So the sysfs that controls this may be necessary. (3)
In addition, toggle functions for controlling the above flags are cleaned.
Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com>
---
drivers/scsi/ufs/ufs-sysfs.c | 46 ++++++++++++++++++-
drivers/scsi/ufs/ufs.h | 2 +-
drivers/scsi/ufs/ufshcd.c | 86 +++++++++++++++++-------------------
drivers/scsi/ufs/ufshcd.h | 7 +++
4 files changed, 94 insertions(+), 47 deletions(-)
diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index 5c405ff7b6ea..6bbb56152708 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -229,7 +229,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, "To control Write Booster is not allowed!\n");
return -EOPNOTSUPP;
}
@@ -253,6 +253,48 @@ static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
return res < 0 ? res : count;
}
+static ssize_t wb_flush_on_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_flush_enabled);
+}
+
+static ssize_t wb_flush_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_flush_enable;
+ ssize_t res;
+
+ if (!ufshcd_is_wb_flush_allowed(hba)) {
+ dev_warn(dev, "To control WB Flush is not allowed!\n");
+ return -EOPNOTSUPP;
+ }
+
+ if (kstrtouint(buf, 0, &wb_flush_enable))
+ return -EINVAL;
+
+ if (wb_flush_enable != 0 && wb_flush_enable != 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_flush(hba, wb_flush_enable);
+ 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);
@@ -261,6 +303,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_flush_on);
static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
&dev_attr_rpm_lvl.attr,
@@ -271,6 +314,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_flush_on.attr,
NULL
};
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 4a00c24a3209..6c85f512f82f 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -611,7 +611,7 @@ struct ufs_dev_info {
/* UFS WB related flags */
bool wb_enabled;
- bool wb_buf_flush_enabled;
+ bool wb_flush_enabled;
u8 wb_dedicated_lu;
u8 wb_buffer_type;
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 3f9caafa91bf..7bd510582471 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -249,7 +249,6 @@ 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_hba_vreg_set_lpm(struct ufs_hba *hba);
static void ufshcd_hba_vreg_set_hpm(struct ufs_hba *hba);
@@ -269,7 +268,7 @@ 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;
@@ -277,7 +276,7 @@ static inline void ufshcd_wb_config(struct ufs_hba *hba)
ufshcd_wb_toggle(hba, true);
ufshcd_wb_toggle_flush_during_h8(hba, true);
- if (!(hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL))
+ if (ufshcd_is_wb_flush_allowed(hba))
ufshcd_wb_toggle_flush(hba, true);
}
@@ -608,7 +607,7 @@ static void ufshcd_device_reset(struct ufs_hba *hba)
ufshcd_set_ufs_dev_active(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_flush_enabled = false;
}
}
if (err != -EOPNOTSUPP)
@@ -5689,74 +5688,70 @@ static void ufshcd_temp_exception_event_handler(struct ufs_hba *hba, u16 status)
*/
}
-static int __ufshcd_wb_toggle(struct ufs_hba *hba, bool set, enum flag_idn idn)
+static int __ufshcd_wb_toggle(struct ufs_hba *hba, const char *knob,
+ bool set, enum flag_idn idn)
{
+ int ret;
u8 index;
enum query_opcode opcode = set ? UPIU_QUERY_OPCODE_SET_FLAG :
- UPIU_QUERY_OPCODE_CLEAR_FLAG;
+ UPIU_QUERY_OPCODE_CLEAR_FLAG;
+
+ if (!ufshcd_is_wb_allowed(hba))
+ return -EPERM;
index = ufshcd_wb_get_query_index(hba);
- return ufshcd_query_flag_retry(hba, opcode, idn, index, NULL);
+
+ ret = ufshcd_query_flag_retry(hba, opcode, idn, index, NULL);
+ if (ret) {
+ dev_err(hba->dev, "%s: %s %s failed %d\n",
+ __func__, knob, set ? "enable" : "disable", ret);
+ return ret;
+ }
+
+ dev_dbg(hba->dev, "%s: %s %s\n",
+ __func__, knob, set ? "enabled" : "disabled");
+
+ return ret;
}
int ufshcd_wb_toggle(struct ufs_hba *hba, bool enable)
{
int ret;
- if (!ufshcd_is_wb_allowed(hba))
+ if (hba->dev_info.wb_enabled == enable)
return 0;
- if (!(enable ^ hba->dev_info.wb_enabled))
- return 0;
-
- 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);
+ ret = __ufshcd_wb_toggle(hba, "Write Booster", enable,
+ QUERY_FLAG_IDN_WB_EN);
+ if (ret)
return ret;
- }
hba->dev_info.wb_enabled = enable;
- dev_info(hba->dev, "%s Write Booster %s\n",
- __func__, enable ? "enabled" : "disabled");
return ret;
}
-static void ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set)
+static void ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool enable)
{
- int ret;
-
- ret = __ufshcd_wb_toggle(hba, set,
- 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);
- return;
- }
- dev_dbg(hba->dev, "%s WB-Buf Flush during H8 %s\n",
- __func__, set ? "enabled" : "disabled");
+ __ufshcd_wb_toggle(hba, "WB-Buf Flush during H8", enable,
+ QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8);
}
-static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable)
+int ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable)
{
int ret;
- if (!ufshcd_is_wb_allowed(hba) ||
- hba->dev_info.wb_buf_flush_enabled == enable)
- return;
+ if (hba->dev_info.wb_flush_enabled == enable)
+ 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;
- }
+ ret = __ufshcd_wb_toggle(hba, "WB-Buf Flush", enable,
+ QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN);
+ if (ret)
+ return ret;
- hba->dev_info.wb_buf_flush_enabled = enable;
+ hba->dev_info.wb_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,
@@ -5790,7 +5785,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_flush_allowed(hba))
ufshcd_wb_toggle_flush(hba, false);
ufshcd_wb_toggle_flush_during_h8(hba, false);
@@ -8178,7 +8173,8 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
*/
ufshcd_set_active_icc_lvl(hba);
- ufshcd_wb_config(hba);
+ ufshcd_wb_set_default_flags(hba);
+
if (hba->ee_usr_mask)
ufshcd_write_ee_control(hba);
/* Enable Auto-Hibernate if configured */
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 94f545be183a..abd2be248dc5 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -991,6 +991,12 @@ static inline bool ufshcd_is_wb_allowed(struct ufs_hba *hba)
return hba->caps & UFSHCD_CAP_WB_EN;
}
+static inline bool ufshcd_is_wb_flush_allowed(struct ufs_hba *hba)
+{
+ return ufshcd_is_wb_allowed(hba) &&
+ !(hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL);
+}
+
static inline bool ufshcd_is_user_access_allowed(struct ufs_hba *hba)
{
return !hba->shutting_down;
@@ -1209,6 +1215,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_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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* RE: [PATCH] scsi: ufs: wb: Add Manual Flush sysfs and cleanup toggle functions
2022-04-22 12:14 ` [PATCH] scsi: ufs: wb: Add Manual Flush sysfs and cleanup toggle functions Jinyoung CHOI
@ 2022-04-22 13:46 ` Avri Altman
2022-04-23 14:24 ` jin young choi
2022-04-22 17:25 ` Bart Van Assche
1 sibling, 1 reply; 7+ messages in thread
From: Avri Altman @ 2022-04-22 13:46 UTC (permalink / raw)
To: j-young.choi, ALIM AKHTAR, jejb, martin.petersen, bvanassche,
beanhuo, Daejun Park, adrian.hunter, cang, asutoshd, linux-scsi,
linux-kernel
> There is the following quirk to bypass "WB Manual Flush" in Write
> Booster.
>
> - UFSHCD_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL
UFSHCD_ --> UFSHCI_
>
> If this quirk is not set, there is no knob that can controll "WB Manual Flush".
>
> There are three flags that control Write Booster Feature.
> 1. WB ON/OFF
> 2. WB Hibern Flush ON/OFF
> 3. WB Flush ON/OFF
>
> The sysfs 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, "Manual flush" may be determined that it can affect
> performance or power consumption.
> So the sysfs that controls this may be necessary. (3)
>
> In addition, toggle functions for controlling the above flags are cleaned.
>
> Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com>
> ---
> drivers/scsi/ufs/ufs-sysfs.c | 46 ++++++++++++++++++-
> drivers/scsi/ufs/ufs.h | 2 +-
> drivers/scsi/ufs/ufshcd.c | 86 +++++++++++++++++-------------------
> drivers/scsi/ufs/ufshcd.h | 7 +++
> 4 files changed, 94 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
> index 5c405ff7b6ea..6bbb56152708 100644
> --- a/drivers/scsi/ufs/ufs-sysfs.c
> +++ b/drivers/scsi/ufs/ufs-sysfs.c
> @@ -229,7 +229,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, "To control Write Booster is not allowed!\n");
> return -EOPNOTSUPP;
> }
>
> @@ -253,6 +253,48 @@ static ssize_t wb_on_store(struct device *dev,
> struct device_attribute *attr,
> return res < 0 ? res : count;
> }
>
> +static ssize_t wb_flush_on_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_flush_enabled);
> +}
> +
> +static ssize_t wb_flush_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_flush_enable;
> + ssize_t res;
> +
> + if (!ufshcd_is_wb_flush_allowed(hba)) {
> + dev_warn(dev, "To control WB Flush is not allowed!\n");
> + return -EOPNOTSUPP;
> + }
> +
> + if (kstrtouint(buf, 0, &wb_flush_enable))
> + return -EINVAL;
> +
> + if (wb_flush_enable != 0 && wb_flush_enable != 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_flush(hba, wb_flush_enable);
> + 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);
> @@ -261,6 +303,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_flush_on);
Maybe wb_flush_enable ?
>
> static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
> &dev_attr_rpm_lvl.attr,
> @@ -271,6 +314,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_flush_on.attr,
> NULL
> };
>
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index 4a00c24a3209..6c85f512f82f 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -611,7 +611,7 @@ struct ufs_dev_info {
>
> /* UFS WB related flags */
> bool wb_enabled;
> - bool wb_buf_flush_enabled;
> + bool wb_flush_enabled;
> u8 wb_dedicated_lu;
> u8 wb_buffer_type;
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 3f9caafa91bf..7bd510582471 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -249,7 +249,6 @@ 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_hba_vreg_set_lpm(struct ufs_hba *hba);
> static void ufshcd_hba_vreg_set_hpm(struct ufs_hba *hba);
>
> @@ -269,7 +268,7 @@ 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;
> @@ -277,7 +276,7 @@ static inline void ufshcd_wb_config(struct ufs_hba
> *hba)
> ufshcd_wb_toggle(hba, true);
>
> ufshcd_wb_toggle_flush_during_h8(hba, true);
> - if (!(hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL))
> + if (ufshcd_is_wb_flush_allowed(hba))
> ufshcd_wb_toggle_flush(hba, true);
> }
>
> @@ -608,7 +607,7 @@ static void ufshcd_device_reset(struct ufs_hba *hba)
> ufshcd_set_ufs_dev_active(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_flush_enabled = false;
> }
> }
> if (err != -EOPNOTSUPP)
> @@ -5689,74 +5688,70 @@ static void
> ufshcd_temp_exception_event_handler(struct ufs_hba *hba, u16 status)
> */
> }
>
> -static int __ufshcd_wb_toggle(struct ufs_hba *hba, bool set, enum flag_idn
> idn)
> +static int __ufshcd_wb_toggle(struct ufs_hba *hba, const char *knob,
> + bool set, enum flag_idn idn)
> {
> + int ret;
> u8 index;
> enum query_opcode opcode = set ? UPIU_QUERY_OPCODE_SET_FLAG :
> - UPIU_QUERY_OPCODE_CLEAR_FLAG;
> + UPIU_QUERY_OPCODE_CLEAR_FLAG;
> +
> + if (!ufshcd_is_wb_allowed(hba))
> + return -EPERM;
>
> index = ufshcd_wb_get_query_index(hba);
> - return ufshcd_query_flag_retry(hba, opcode, idn, index, NULL);
> +
> + ret = ufshcd_query_flag_retry(hba, opcode, idn, index, NULL);
> + if (ret) {
> + dev_err(hba->dev, "%s: %s %s failed %d\n",
> + __func__, knob, set ? "enable" : "disable", ret);
> + return ret;
> + }
> +
> + dev_dbg(hba->dev, "%s: %s %s\n",
> + __func__, knob, set ? "enabled" : "disabled");
> +
> + return ret;
> }
Is this debug info (knob extra argument) worth the complication?
Thanks,
Avri
>
> int ufshcd_wb_toggle(struct ufs_hba *hba, bool enable)
> {
> int ret;
>
> - if (!ufshcd_is_wb_allowed(hba))
> + if (hba->dev_info.wb_enabled == enable)
> return 0;
>
> - if (!(enable ^ hba->dev_info.wb_enabled))
> - return 0;
> -
> - 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);
> + ret = __ufshcd_wb_toggle(hba, "Write Booster", enable,
> + QUERY_FLAG_IDN_WB_EN);
> + if (ret)
> return ret;
> - }
>
> hba->dev_info.wb_enabled = enable;
> - dev_info(hba->dev, "%s Write Booster %s\n",
> - __func__, enable ? "enabled" : "disabled");
>
> return ret;
> }
>
> -static void ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool
> set)
> +static void ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool
> enable)
> {
> - int ret;
> -
> - ret = __ufshcd_wb_toggle(hba, set,
> - 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);
> - return;
> - }
> - dev_dbg(hba->dev, "%s WB-Buf Flush during H8 %s\n",
> - __func__, set ? "enabled" : "disabled");
> + __ufshcd_wb_toggle(hba, "WB-Buf Flush during H8", enable,
> + QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8);
> }
>
> -static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable)
> +int ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable)
> {
> int ret;
>
> - if (!ufshcd_is_wb_allowed(hba) ||
> - hba->dev_info.wb_buf_flush_enabled == enable)
> - return;
> + if (hba->dev_info.wb_flush_enabled == enable)
> + 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;
> - }
> + ret = __ufshcd_wb_toggle(hba, "WB-Buf Flush", enable,
> + QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN);
> + if (ret)
> + return ret;
>
> - hba->dev_info.wb_buf_flush_enabled = enable;
> + hba->dev_info.wb_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,
> @@ -5790,7 +5785,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_flush_allowed(hba))
> ufshcd_wb_toggle_flush(hba, false);
>
> ufshcd_wb_toggle_flush_during_h8(hba, false);
> @@ -8178,7 +8173,8 @@ static int ufshcd_probe_hba(struct ufs_hba *hba,
> bool init_dev_params)
> */
> ufshcd_set_active_icc_lvl(hba);
>
> - ufshcd_wb_config(hba);
> + ufshcd_wb_set_default_flags(hba);
> +
> if (hba->ee_usr_mask)
> ufshcd_write_ee_control(hba);
> /* Enable Auto-Hibernate if configured */
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 94f545be183a..abd2be248dc5 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -991,6 +991,12 @@ static inline bool ufshcd_is_wb_allowed(struct
> ufs_hba *hba)
> return hba->caps & UFSHCD_CAP_WB_EN;
> }
>
> +static inline bool ufshcd_is_wb_flush_allowed(struct ufs_hba *hba)
> +{
> + return ufshcd_is_wb_allowed(hba) &&
> + !(hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL);
> +}
> +
> static inline bool ufshcd_is_user_access_allowed(struct ufs_hba *hba)
> {
> return !hba->shutting_down;
> @@ -1209,6 +1215,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_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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] scsi: ufs: wb: Add Manual Flush sysfs and cleanup toggle functions
2022-04-22 12:14 ` [PATCH] scsi: ufs: wb: Add Manual Flush sysfs and cleanup toggle functions Jinyoung CHOI
2022-04-22 13:46 ` Avri Altman
@ 2022-04-22 17:25 ` Bart Van Assche
2022-04-23 18:40 ` jin young choi
1 sibling, 1 reply; 7+ messages in thread
From: Bart Van Assche @ 2022-04-22 17:25 UTC (permalink / raw)
To: j-young.choi, ALIM AKHTAR, avri.altman, jejb, martin.petersen,
beanhuo, Daejun Park, adrian.hunter, cang, asutoshd, linux-scsi,
linux-kernel
On 4/22/22 05:14, Jinyoung CHOI wrote:
> There is the following quirk to bypass "WB Manual Flush" in Write
> Booster.
>
> - UFSHCD_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL
>
> If this quirk is not set, there is no knob that can controll "WB Manual Flush".
>
> There are three flags that control Write Booster Feature.
> 1. WB ON/OFF
> 2. WB Hibern Flush ON/OFF
> 3. WB Flush ON/OFF
>
> The sysfs 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, "Manual flush" may be determined that it can affect
> performance or power consumption.
> So the sysfs that controls this may be necessary. (3)
>
> In addition, toggle functions for controlling the above flags are cleaned.
Please make all sentences in the patch description start at the left margin.
> diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
> index 5c405ff7b6ea..6bbb56152708 100644
> --- a/drivers/scsi/ufs/ufs-sysfs.c
> +++ b/drivers/scsi/ufs/ufs-sysfs.c
> @@ -229,7 +229,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, "To control Write Booster is not allowed!\n");
> return -EOPNOTSUPP;
> }
The new error message is grammatically incorrect. Please fix.
> + if (!ufshcd_is_wb_flush_allowed(hba)) {
> + dev_warn(dev, "To control WB Flush is not allowed!\n");
Same issue for the above error message.
> +static DEVICE_ATTR_RW(wb_flush_on);
"wb_flush_enabled" is probably a better name than "wb_flush_on".
Additionally, the "wb_flush_en" is closer to the terminology used in the
UFS specification (fWriteBoosterBufferFlushEn).
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index 4a00c24a3209..6c85f512f82f 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -611,7 +611,7 @@ struct ufs_dev_info {
>
> /* UFS WB related flags */
> bool wb_enabled;
> - bool wb_buf_flush_enabled;
> + bool wb_flush_enabled;
> u8 wb_dedicated_lu;
> u8 wb_buffer_type;
Adding a variable with the name "wb_flush_enabled" next to a variable with
the name "wb_buf_flush_enabled" is confusing. Please chose better names and
add comments.
> -static int __ufshcd_wb_toggle(struct ufs_hba *hba, bool set, enum flag_idn idn)
> +static int __ufshcd_wb_toggle(struct ufs_hba *hba, const char *knob,
> + bool set, enum flag_idn idn)
> {
> + int ret;
> u8 index;
> enum query_opcode opcode = set ? UPIU_QUERY_OPCODE_SET_FLAG :
> - UPIU_QUERY_OPCODE_CLEAR_FLAG;
> + UPIU_QUERY_OPCODE_CLEAR_FLAG;
> +
> + if (!ufshcd_is_wb_allowed(hba))
> + return -EPERM;
>
> index = ufshcd_wb_get_query_index(hba);
> - return ufshcd_query_flag_retry(hba, opcode, idn, index, NULL);
> +
> + ret = ufshcd_query_flag_retry(hba, opcode, idn, index, NULL);
> + if (ret) {
> + dev_err(hba->dev, "%s: %s %s failed %d\n",
> + __func__, knob, set ? "enable" : "disable", ret);
> + return ret;
> + }
> +
> + dev_dbg(hba->dev, "%s: %s %s\n",
> + __func__, knob, set ? "enabled" : "disabled");
> +
> + return ret;
> }
Please leave out the dev_dbg() message and move the dev_err() message to
the callers of __ufshcd_wb_toggle() such that the 'knob' argument does not
have to be added to __ufshcd_wb_toggle().
Thanks,
Bart.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] scsi: ufs: wb: Add Manual Flush sysfs and cleanup toggle functions
2022-04-22 13:46 ` Avri Altman
@ 2022-04-23 14:24 ` jin young choi
2022-04-23 21:38 ` Bart Van Assche
0 siblings, 1 reply; 7+ messages in thread
From: jin young choi @ 2022-04-23 14:24 UTC (permalink / raw)
To: Avri Altman
Cc: j-young.choi, ALIM AKHTAR, jejb, martin.petersen, bvanassche,
beanhuo, Daejun Park, adrian.hunter, cang, asutoshd, linux-scsi,
linux-kernel
>
> > There is the following quirk to bypass "WB Manual Flush" in Write
> > Booster.
> >
> > - UFSHCD_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL
> UFSHCD_ --> UFSHCI_
>
I'll fix it. :)
> >
> > If this quirk is not set, there is no knob that can controll "WB Manual Flush".
> >
> > There are three flags that control Write Booster Feature.
> > 1. WB ON/OFF
> > 2. WB Hibern Flush ON/OFF
> > 3. WB Flush ON/OFF
> >
> > The sysfs 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, "Manual flush" may be determined that it can affect
> > performance or power consumption.
> > So the sysfs that controls this may be necessary. (3)
> >
> > In addition, toggle functions for controlling the above flags are cleaned.
> >
> > Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com>
> > ---
> > drivers/scsi/ufs/ufs-sysfs.c | 46 ++++++++++++++++++-
> > drivers/scsi/ufs/ufs.h | 2 +-
> > drivers/scsi/ufs/ufshcd.c | 86 +++++++++++++++++-------------------
> > drivers/scsi/ufs/ufshcd.h | 7 +++
> > 4 files changed, 94 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
> > index 5c405ff7b6ea..6bbb56152708 100644
> > --- a/drivers/scsi/ufs/ufs-sysfs.c
> > +++ b/drivers/scsi/ufs/ufs-sysfs.c
> > @@ -229,7 +229,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, "To control Write Booster is not allowed!\n");
> > return -EOPNOTSUPP;
> > }
> >
> > @@ -253,6 +253,48 @@ static ssize_t wb_on_store(struct device *dev,
> > struct device_attribute *attr,
> > return res < 0 ? res : count;
> > }
> >
> > +static ssize_t wb_flush_on_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_flush_enabled);
> > +}
> > +
> > +static ssize_t wb_flush_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_flush_enable;
> > + ssize_t res;
> > +
> > + if (!ufshcd_is_wb_flush_allowed(hba)) {
> > + dev_warn(dev, "To control WB Flush is not allowed!\n");
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + if (kstrtouint(buf, 0, &wb_flush_enable))
> > + return -EINVAL;
> > +
> > + if (wb_flush_enable != 0 && wb_flush_enable != 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_flush(hba, wb_flush_enable);
> > + 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);
> > @@ -261,6 +303,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_flush_on);
> Maybe wb_flush_enable ?
>
'wb_on' sysfs already existed. So I named it in the same format (_on)
I'll change both. (_on -> _enable)
> >
> > static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
> > &dev_attr_rpm_lvl.attr,
> > @@ -271,6 +314,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_flush_on.attr,
> > NULL
> > };
> >
> > diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> > index 4a00c24a3209..6c85f512f82f 100644
> > --- a/drivers/scsi/ufs/ufs.h
> > +++ b/drivers/scsi/ufs/ufs.h
> > @@ -611,7 +611,7 @@ struct ufs_dev_info {
> >
> > /* UFS WB related flags */
> > bool wb_enabled;
> > - bool wb_buf_flush_enabled;
> > + bool wb_flush_enabled;
> > u8 wb_dedicated_lu;
> > u8 wb_buffer_type;
> >
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 3f9caafa91bf..7bd510582471 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -249,7 +249,6 @@ 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_hba_vreg_set_lpm(struct ufs_hba *hba);
> > static void ufshcd_hba_vreg_set_hpm(struct ufs_hba *hba);
> >
> > @@ -269,7 +268,7 @@ 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;
> > @@ -277,7 +276,7 @@ static inline void ufshcd_wb_config(struct ufs_hba
> > *hba)
> > ufshcd_wb_toggle(hba, true);
> >
> > ufshcd_wb_toggle_flush_during_h8(hba, true);
> > - if (!(hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL))
> > + if (ufshcd_is_wb_flush_allowed(hba))
> > ufshcd_wb_toggle_flush(hba, true);
> > }
> >
> > @@ -608,7 +607,7 @@ static void ufshcd_device_reset(struct ufs_hba *hba)
> > ufshcd_set_ufs_dev_active(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_flush_enabled = false;
> > }
> > }
> > if (err != -EOPNOTSUPP)
> > @@ -5689,74 +5688,70 @@ static void
> > ufshcd_temp_exception_event_handler(struct ufs_hba *hba, u16 status)
> > */
> > }
> >
> > -static int __ufshcd_wb_toggle(struct ufs_hba *hba, bool set, enum flag_idn
> > idn)
> > +static int __ufshcd_wb_toggle(struct ufs_hba *hba, const char *knob,
> > + bool set, enum flag_idn idn)
> > {
> > + int ret;
> > u8 index;
> > enum query_opcode opcode = set ? UPIU_QUERY_OPCODE_SET_FLAG :
> > - UPIU_QUERY_OPCODE_CLEAR_FLAG;
> > + UPIU_QUERY_OPCODE_CLEAR_FLAG;
> > +
> > + if (!ufshcd_is_wb_allowed(hba))
> > + return -EPERM;
> >
> > index = ufshcd_wb_get_query_index(hba);
> > - return ufshcd_query_flag_retry(hba, opcode, idn, index, NULL);
> > +
> > + ret = ufshcd_query_flag_retry(hba, opcode, idn, index, NULL);
> > + if (ret) {
> > + dev_err(hba->dev, "%s: %s %s failed %d\n",
> > + __func__, knob, set ? "enable" : "disable", ret);
> > + return ret;
> > + }
> > +
> > + dev_dbg(hba->dev, "%s: %s %s\n",
> > + __func__, knob, set ? "enabled" : "disabled");
> > +
> > + return ret;
> > }
> Is this debug info (knob extra argument) worth the complication?
>
> Thanks,
> Avri
>
Only the location of the existing code has been moved.
I will edit this part again.
This is because the msg for query execution already exists in the
following function.
- ufshcd_query_flag_retry()
Thanks,
Jinyoung
> >
> > int ufshcd_wb_toggle(struct ufs_hba *hba, bool enable)
> > {
> > int ret;
> >
> > - if (!ufshcd_is_wb_allowed(hba))
> > + if (hba->dev_info.wb_enabled == enable)
> > return 0;
> >
> > - if (!(enable ^ hba->dev_info.wb_enabled))
> > - return 0;
> > -
> > - 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);
> > + ret = __ufshcd_wb_toggle(hba, "Write Booster", enable,
> > + QUERY_FLAG_IDN_WB_EN);
> > + if (ret)
> > return ret;
> > - }
> >
> > hba->dev_info.wb_enabled = enable;
> > - dev_info(hba->dev, "%s Write Booster %s\n",
> > - __func__, enable ? "enabled" : "disabled");
> >
> > return ret;
> > }
> >
> > -static void ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool
> > set)
> > +static void ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool
> > enable)
> > {
> > - int ret;
> > -
> > - ret = __ufshcd_wb_toggle(hba, set,
> > - 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);
> > - return;
> > - }
> > - dev_dbg(hba->dev, "%s WB-Buf Flush during H8 %s\n",
> > - __func__, set ? "enabled" : "disabled");
> > + __ufshcd_wb_toggle(hba, "WB-Buf Flush during H8", enable,
> > + QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8);
> > }
> >
> > -static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable)
> > +int ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable)
> > {
> > int ret;
> >
> > - if (!ufshcd_is_wb_allowed(hba) ||
> > - hba->dev_info.wb_buf_flush_enabled == enable)
> > - return;
> > + if (hba->dev_info.wb_flush_enabled == enable)
> > + 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;
> > - }
> > + ret = __ufshcd_wb_toggle(hba, "WB-Buf Flush", enable,
> > + QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN);
> > + if (ret)
> > + return ret;
> >
> > - hba->dev_info.wb_buf_flush_enabled = enable;
> > + hba->dev_info.wb_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,
> > @@ -5790,7 +5785,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_flush_allowed(hba))
> > ufshcd_wb_toggle_flush(hba, false);
> >
> > ufshcd_wb_toggle_flush_during_h8(hba, false);
> > @@ -8178,7 +8173,8 @@ static int ufshcd_probe_hba(struct ufs_hba *hba,
> > bool init_dev_params)
> > */
> > ufshcd_set_active_icc_lvl(hba);
> >
> > - ufshcd_wb_config(hba);
> > + ufshcd_wb_set_default_flags(hba);
> > +
> > if (hba->ee_usr_mask)
> > ufshcd_write_ee_control(hba);
> > /* Enable Auto-Hibernate if configured */
> > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> > index 94f545be183a..abd2be248dc5 100644
> > --- a/drivers/scsi/ufs/ufshcd.h
> > +++ b/drivers/scsi/ufs/ufshcd.h
> > @@ -991,6 +991,12 @@ static inline bool ufshcd_is_wb_allowed(struct
> > ufs_hba *hba)
> > return hba->caps & UFSHCD_CAP_WB_EN;
> > }
> >
> > +static inline bool ufshcd_is_wb_flush_allowed(struct ufs_hba *hba)
> > +{
> > + return ufshcd_is_wb_allowed(hba) &&
> > + !(hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL);
> > +}
> > +
> > static inline bool ufshcd_is_user_access_allowed(struct ufs_hba *hba)
> > {
> > return !hba->shutting_down;
> > @@ -1209,6 +1215,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_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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] scsi: ufs: wb: Add Manual Flush sysfs and cleanup toggle functions
2022-04-22 17:25 ` Bart Van Assche
@ 2022-04-23 18:40 ` jin young choi
0 siblings, 0 replies; 7+ messages in thread
From: jin young choi @ 2022-04-23 18:40 UTC (permalink / raw)
To: Bart Van Assche
Cc: j-young.choi, ALIM AKHTAR, avri.altman, jejb, martin.petersen,
beanhuo, Daejun Park, adrian.hunter, cang, asutoshd, linux-scsi,
linux-kernel
>
> On 4/22/22 05:14, Jinyoung CHOI wrote:
> > There is the following quirk to bypass "WB Manual Flush" in Write
> > Booster.
> >
> > - UFSHCD_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL
> >
> > If this quirk is not set, there is no knob that can controll "WB Manual Flush".
> >
> > There are three flags that control Write Booster Feature.
> > 1. WB ON/OFF
> > 2. WB Hibern Flush ON/OFF
> > 3. WB Flush ON/OFF
> >
> > The sysfs 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, "Manual flush" may be determined that it can affect
> > performance or power consumption.
> > So the sysfs that controls this may be necessary. (3)
> >
> > In addition, toggle functions for controlling the above flags are cleaned.
>
> Please make all sentences in the patch description start at the left margin.
>
OK. I'll fix it. :)
> > diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
> > index 5c405ff7b6ea..6bbb56152708 100644
> > --- a/drivers/scsi/ufs/ufs-sysfs.c
> > +++ b/drivers/scsi/ufs/ufs-sysfs.c
> > @@ -229,7 +229,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, "To control Write Booster is not allowed!\n");
> > return -EOPNOTSUPP;
> > }
>
> The new error message is grammatically incorrect. Please fix.
>
OK. I'll fix it. :)
> > + if (!ufshcd_is_wb_flush_allowed(hba)) {
> > + dev_warn(dev, "To control WB Flush is not allowed!\n");
>
> Same issue for the above error message.
>
> > +static DEVICE_ATTR_RW(wb_flush_on);
>
> "wb_flush_enabled" is probably a better name than "wb_flush_on".
> Additionally, the "wb_flush_en" is closer to the terminology used in the
> UFS specification (fWriteBoosterBufferFlushEn).
>
'wb_on' sysfs already existed. So I named it in the same format. (_on)
I'll change both. (_on -> _enable)
> > diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> > index 4a00c24a3209..6c85f512f82f 100644
> > --- a/drivers/scsi/ufs/ufs.h
> > +++ b/drivers/scsi/ufs/ufs.h
> > @@ -611,7 +611,7 @@ struct ufs_dev_info {
> >
> > /* UFS WB related flags */
> > bool wb_enabled;
> > - bool wb_buf_flush_enabled;
> > + bool wb_flush_enabled;
> > u8 wb_dedicated_lu;
> > u8 wb_buffer_type;
>
> Adding a variable with the name "wb_flush_enabled" next to a variable with
> the name "wb_buf_flush_enabled" is confusing. Please chose better names and
> add comments.
>
Hmm... it would be better not to modify the variable name.
I'll put it back
> > -static int __ufshcd_wb_toggle(struct ufs_hba *hba, bool set, enum flag_idn idn)
> > +static int __ufshcd_wb_toggle(struct ufs_hba *hba, const char *knob,
> > + bool set, enum flag_idn idn)
> > {
> > + int ret;
> > u8 index;
> > enum query_opcode opcode = set ? UPIU_QUERY_OPCODE_SET_FLAG :
> > - UPIU_QUERY_OPCODE_CLEAR_FLAG;
> > + UPIU_QUERY_OPCODE_CLEAR_FLAG;
> > +
> > + if (!ufshcd_is_wb_allowed(hba))
> > + return -EPERM;
> >
> > index = ufshcd_wb_get_query_index(hba);
> > - return ufshcd_query_flag_retry(hba, opcode, idn, index, NULL);
> > +
> > + ret = ufshcd_query_flag_retry(hba, opcode, idn, index, NULL);
> > + if (ret) {
> > + dev_err(hba->dev, "%s: %s %s failed %d\n",
> > + __func__, knob, set ? "enable" : "disable", ret);
> > + return ret;
> > + }
> > +
> > + dev_dbg(hba->dev, "%s: %s %s\n",
> > + __func__, knob, set ? "enabled" : "disabled");
> > +
> > + return ret;
> > }
>
> Please leave out the dev_dbg() message and move the dev_err() message to
> the callers of __ufshcd_wb_toggle() such that the 'knob' argument does not
> have to be added to __ufshcd_wb_toggle().
>
> Thanks,
>
> Bart.
OK. I got it.
Regarding this review, I wrote a comment on avri's comment.
Thanks,
Jinyoung
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] scsi: ufs: wb: Add Manual Flush sysfs and cleanup toggle functions
2022-04-23 14:24 ` jin young choi
@ 2022-04-23 21:38 ` Bart Van Assche
2022-04-25 10:59 ` Jinyoung Choi
0 siblings, 1 reply; 7+ messages in thread
From: Bart Van Assche @ 2022-04-23 21:38 UTC (permalink / raw)
To: jin young choi, Avri Altman
Cc: j-young.choi, ALIM AKHTAR, jejb, martin.petersen, beanhuo,
Daejun Park, adrian.hunter, cang, asutoshd, linux-scsi,
linux-kernel
On 4/23/22 07:24, jin young choi wrote:
>>> +static DEVICE_ATTR_RW(wb_flush_on);
>> Maybe wb_flush_enable ?
>>
> 'wb_on' sysfs already existed. So I named it in the same format (_on)
> I'll change both. (_on -> _enable)
sysfs attributes constitute an ABI. Breaking the user space ABI is not
allowed. Hence, renaming existing sysfs attributes is not an option.
Thanks,
Bart.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] scsi: ufs: wb: Add Manual Flush sysfs and cleanup toggle functions
2022-04-23 21:38 ` Bart Van Assche
@ 2022-04-25 10:59 ` Jinyoung Choi
0 siblings, 0 replies; 7+ messages in thread
From: Jinyoung Choi @ 2022-04-25 10:59 UTC (permalink / raw)
To: Bart Van Assche
Cc: Avri Altman, j-young.choi, ALIM AKHTAR, jejb, martin.petersen,
beanhuo, Daejun Park, adrian.hunter, cang, asutoshd, linux-scsi,
linux-kernel
>
> On 4/23/22 07:24, jin young choi wrote:
>
> >>> +static DEVICE_ATTR_RW(wb_flush_on);
> >> Maybe wb_flush_enable ?
> >>
> > 'wb_on' sysfs already existed. So I named it in the same format (_on)
> > I'll change both. (_on -> _enable)
>
> sysfs attributes constitute an ABI. Breaking the user space ABI is not
> allowed. Hence, renaming existing sysfs attributes is not an option.
>
> Thanks,
>
> Bart.
I got it. Applied to v2 patch.
I sent a mail from Google Web,
but the mailing list was messed up with a tab problem.
(Changed to git send-email)
I'll be careful.
Thanks,
Jinyoung.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-04-25 11:00 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20220422120240epcms2p24bdcb416becf76b417f7c39006aa40f2@epcms2p1>
2022-04-22 12:14 ` [PATCH] scsi: ufs: wb: Add Manual Flush sysfs and cleanup toggle functions Jinyoung CHOI
2022-04-22 13:46 ` Avri Altman
2022-04-23 14:24 ` jin young choi
2022-04-23 21:38 ` Bart Van Assche
2022-04-25 10:59 ` Jinyoung Choi
2022-04-22 17:25 ` Bart Van Assche
2022-04-23 18:40 ` jin young choi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.