From: Bean Huo <[email protected]>
Changelog:
v3--v4:
1. Rebase patch on 5.11/scsi-staging
2. Add WB cleanup patches 3/6, 4/6 adn 5/6
v2--v3:
1. Change multi-line comments style in patch 1/3 (Can Guo)
v1--v2:
1. Take is_hibern8_wb_flush checkup out from function
ufshcd_wb_need_flush() in patch 2/3
2. Add UFSHCD_CAP_CLK_SCALING checkup in patch 1/3. that means
only for the platform, which doesn't support UFSHCD_CAP_CLK_SCALING,
can control WB through "wb_on".
Bean Huo (6):
scsi: ufs: Add "wb_on" sysfs node to control WB on/off
scsi: ufs: Changes comment in the function ufshcd_wb_probe()
scsi: ufs: Group UFS WB related flags to struct ufs_dev_info
scsi: ufs: Remove d_wb_alloc_units from struct ufs_dev_info
scsi: ufs: Cleanup WB buffer flush toggle implementation
scsi: ufs: Keep device active mode only
fWriteBoosterBufferFlushDuringHibernate == 1
drivers/scsi/ufs/ufs-sysfs.c | 41 +++++++++++++
drivers/scsi/ufs/ufs.h | 33 +++++-----
drivers/scsi/ufs/ufshcd.c | 114 +++++++++++++++--------------------
drivers/scsi/ufs/ufshcd.h | 4 +-
4 files changed, 112 insertions(+), 80 deletions(-)
--
2.17.1
From: Bean Huo <[email protected]>
Delete ufshcd_wb_buf_flush_enable() and ufshcd_wb_buf_flush_disable(),
move the implementation into ufshcd_wb_toggle_flush().
Signed-off-by: Bean Huo <[email protected]>
---
drivers/scsi/ufs/ufshcd.c | 69 ++++++++++++++-------------------------
1 file changed, 24 insertions(+), 45 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 0998e6103cd7..fb3c98724005 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -244,10 +244,8 @@ static int ufshcd_setup_vreg(struct ufs_hba *hba, bool on);
static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba,
struct ufs_vreg *vreg);
static int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag);
-static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba);
-static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba);
static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set);
-static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable);
+static inline int ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable);
static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba);
static void ufshcd_hba_vreg_set_hpm(struct ufs_hba *hba);
@@ -5398,60 +5396,41 @@ static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set)
index, NULL);
}
-static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable)
-{
- if (hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)
- return;
-
- if (enable)
- ufshcd_wb_buf_flush_enable(hba);
- else
- ufshcd_wb_buf_flush_disable(hba);
-
-}
-
-static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba)
+static inline int ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable)
{
int ret;
u8 index;
+ enum query_opcode opcode;
- if (!ufshcd_is_wb_allowed(hba) || hba->dev_info.wb_buf_flush_enabled)
+ if (hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)
return 0;
- index = ufshcd_wb_get_query_index(hba);
- ret = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_SET_FLAG,
- QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN,
- index, NULL);
- if (ret)
- dev_err(hba->dev, "%s WB - buf flush enable failed %d\n",
- __func__, ret);
- else
- hba->dev_info.wb_buf_flush_enabled = true;
-
- dev_dbg(hba->dev, "WB - Flush enabled: %d\n", ret);
- return ret;
-}
-
-static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba)
-{
- int ret;
- u8 index;
-
- if (!ufshcd_is_wb_allowed(hba) || !hba->dev_info.wb_buf_flush_enabled)
+ if (!ufshcd_is_wb_allowed(hba) ||
+ hba->dev_info.wb_buf_flush_enabled == enable)
return 0;
+ if (enable)
+ opcode = UPIU_QUERY_OPCODE_SET_FLAG;
+ else
+ opcode = UPIU_QUERY_OPCODE_CLEAR_FLAG;
+
index = ufshcd_wb_get_query_index(hba);
- ret = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_CLEAR_FLAG,
- QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN,
- index, NULL);
+ ret = ufshcd_query_flag_retry(hba, opcode,
+ QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN, index,
+ NULL);
if (ret) {
- dev_warn(hba->dev, "%s: WB - buf flush disable failed %d\n",
- __func__, ret);
- } else {
- hba->dev_info.wb_buf_flush_enabled = false;
- dev_dbg(hba->dev, "WB - Flush disabled: %d\n", ret);
+ dev_err(hba->dev, "%s WB-Buf Flush %s failed %d\n", __func__,
+ enable ? "enable" : "disable", ret);
+ goto out;
}
+ if (enable)
+ hba->dev_info.wb_buf_flush_enabled = true;
+ else
+ hba->dev_info.wb_buf_flush_enabled = false;
+
+ dev_dbg(hba->dev, "WB-Buf Flush %s\n", enable ? "enabled" : "disabled");
+out:
return ret;
}
--
2.17.1
From: Bean Huo <[email protected]>
According to the JEDEC UFS 3.1 Spec, If fWriteBoosterBufferFlushDuringHibernate
is set to one, the device flushes the WriteBooster Buffer data automatically
whenever the link enters the hibernate (HIBERN8) state. While the flushing
operation is in progress, the device should be kept in Active power mode.
Currently, we set this flag during the UFSHCD probe stage, but we didn't deal
with its programming failure. Even this failure is less likely to occur, but
still it is possible.
This patch is to add checkup of fWriteBoosterBufferFlushDuringHibernate setting,
keep the device as "active power mode" only when this flag be successfully set
to 1.
Fixes: 51dd905bd2f6 ("scsi: ufs: Fix WriteBooster flush during runtime suspend")
Signed-off-by: Bean Huo <[email protected]>
---
drivers/scsi/ufs/ufs.h | 1 +
drivers/scsi/ufs/ufshcd.c | 21 ++++++++++++++++-----
2 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 8ed342e43883..1b3866f608d9 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -542,6 +542,7 @@ struct ufs_dev_info {
/* UFS WB related flags */
bool wb_enabled;
bool wb_buf_flush_enabled;
+ bool wb_buf_flush_in_hibern8;
u8 wb_dedicated_lu;
u8 b_wb_buffer_type;
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index fb3c98724005..7ff486f047d8 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -282,10 +282,16 @@ static inline void ufshcd_wb_config(struct ufs_hba *hba)
dev_err(hba->dev, "%s: Enable WB failed: %d\n", __func__, ret);
else
dev_info(hba->dev, "%s: Write Booster Configured\n", __func__);
+
ret = ufshcd_wb_toggle_flush_during_h8(hba, true);
- if (ret)
+ if (ret) {
dev_err(hba->dev, "%s: En WB flush during H8: failed: %d\n",
__func__, ret);
+ hba->dev_info.wb_buf_flush_in_hibern8 = false;
+ } else {
+ hba->dev_info.wb_buf_flush_in_hibern8 = true;
+ }
+
ufshcd_wb_toggle_flush(hba, true);
}
@@ -589,6 +595,7 @@ static void ufshcd_device_reset(struct ufs_hba *hba)
if (ufshcd_is_wb_allowed(hba)) {
hba->dev_info.wb_enabled = false;
hba->dev_info.wb_buf_flush_enabled = false;
+ hba->dev_info.wb_buf_flush_in_hibern8 = false;
}
}
if (err != -EOPNOTSUPP)
@@ -5471,6 +5478,7 @@ static bool ufshcd_wb_need_flush(struct ufs_hba *hba)
if (!ufshcd_is_wb_allowed(hba))
return false;
+
/*
* The ufs device needs the vcc to be ON to flush.
* With user-space reduction enabled, it's enough to enable flush
@@ -8571,6 +8579,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
enum ufs_pm_level pm_lvl;
enum ufs_dev_pwr_mode req_dev_pwr_mode;
enum uic_link_state req_link_state;
+ bool hibern8;
hba->pm_op_in_progress = 1;
if (!ufshcd_is_shutdown_pm(pm_op)) {
@@ -8630,11 +8639,13 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
* Hibern8, keep device power mode as "active power mode"
* and VCC supply.
*/
+ hibern8 = req_link_state == UIC_LINK_HIBERN8_STATE ||
+ (req_link_state == UIC_LINK_ACTIVE_STATE &&
+ ufshcd_is_auto_hibern8_enabled(hba));
+
hba->dev_info.b_rpm_dev_flush_capable =
- hba->auto_bkops_enabled ||
- (((req_link_state == UIC_LINK_HIBERN8_STATE) ||
- ((req_link_state == UIC_LINK_ACTIVE_STATE) &&
- ufshcd_is_auto_hibern8_enabled(hba))) &&
+ hba->auto_bkops_enabled || (hibern8 &&
+ hba->dev_info.wb_buf_flush_in_hibern8 &&
ufshcd_wb_need_flush(hba));
}
--
2.17.1
From: Bean Huo <[email protected]>
Currently UFS WriteBooster driver uses clock scaling up/down to set
WB on/off, for the platform which doesn't support UFSHCD_CAP_CLK_SCALING,
WB will be always on. Provide a sysfs attribute to enable/disable WB
during runtime. Write 1/0 to "wb_on" sysfs node to enable/disable UFS WB.
Reviewed-by: Avri Altman <[email protected]>
Reviewed-by: Stanley Chu <[email protected]>
Signed-off-by: Bean Huo <[email protected]>
---
drivers/scsi/ufs/ufs-sysfs.c | 41 ++++++++++++++++++++++++++++++++++++
drivers/scsi/ufs/ufshcd.c | 3 +--
drivers/scsi/ufs/ufshcd.h | 2 ++
3 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index 08e72b7eef6a..2b4e9fe935cc 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -189,6 +189,45 @@ static ssize_t auto_hibern8_store(struct device *dev,
return count;
}
+static ssize_t wb_on_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct ufs_hba *hba = dev_get_drvdata(dev);
+
+ return scnprintf(buf, PAGE_SIZE, "%d\n", hba->wb_enabled);
+}
+
+static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct ufs_hba *hba = dev_get_drvdata(dev);
+ unsigned int wb_enable;
+ ssize_t res;
+
+ if (ufshcd_is_clkscaling_supported(hba)) {
+ /*
+ * If the platform supports UFSHCD_CAP_AUTO_BKOPS_SUSPEND,
+ * turn WB on/off will be done while clock scaling up/down.
+ */
+ dev_warn(dev, "To control WB through wb_on is not allowed!\n");
+ return -EOPNOTSUPP;
+ }
+ if (!ufshcd_is_wb_allowed(hba))
+ return -EOPNOTSUPP;
+
+ if (kstrtouint(buf, 0, &wb_enable))
+ return -EINVAL;
+
+ if (wb_enable != 0 && wb_enable != 1)
+ return -EINVAL;
+
+ pm_runtime_get_sync(hba->dev);
+ res = ufshcd_wb_ctrl(hba, wb_enable);
+ pm_runtime_put_sync(hba->dev);
+
+ return res < 0 ? res : count;
+}
+
static DEVICE_ATTR_RW(rpm_lvl);
static DEVICE_ATTR_RO(rpm_target_dev_state);
static DEVICE_ATTR_RO(rpm_target_link_state);
@@ -196,6 +235,7 @@ static DEVICE_ATTR_RW(spm_lvl);
static DEVICE_ATTR_RO(spm_target_dev_state);
static DEVICE_ATTR_RO(spm_target_link_state);
static DEVICE_ATTR_RW(auto_hibern8);
+static DEVICE_ATTR_RW(wb_on);
static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
&dev_attr_rpm_lvl.attr,
@@ -205,6 +245,7 @@ static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
&dev_attr_spm_target_dev_state.attr,
&dev_attr_spm_target_link_state.attr,
&dev_attr_auto_hibern8.attr,
+ &dev_attr_wb_on.attr,
NULL
};
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index e221add25a7e..5e1dcf4de67e 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -246,7 +246,6 @@ static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba,
static int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag);
static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba);
static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba);
-static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable);
static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set);
static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable);
static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba);
@@ -5351,7 +5350,7 @@ static void ufshcd_bkops_exception_event_handler(struct ufs_hba *hba)
__func__, err);
}
-static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
+int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
{
int ret;
u8 index;
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 9bb5f0ed4124..2a97006a2c93 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -1068,6 +1068,8 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
u8 *desc_buff, int *buff_len,
enum query_opcode desc_op);
+int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable);
+
/* Wrapper functions for safely calling variant operations */
static inline const char *ufshcd_get_var_name(struct ufs_hba *hba)
{
--
2.17.1
From: Bean Huo <[email protected]>
UFS device-related flags should be grouped in ufs_dev_info. Take
wb_enabled and wb_buf_flush_enabled out from the struct ufs_hba,
group them to struct ufs_dev_info, and align the names of the structure
members vertically
Signed-off-by: Bean Huo <[email protected]>
---
drivers/scsi/ufs/ufs-sysfs.c | 2 +-
drivers/scsi/ufs/ufs.h | 33 +++++++++++++++++++--------------
drivers/scsi/ufs/ufshcd.c | 16 ++++++++--------
drivers/scsi/ufs/ufshcd.h | 2 --
4 files changed, 28 insertions(+), 25 deletions(-)
diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index 2b4e9fe935cc..4bd7e18bb486 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -194,7 +194,7 @@ static ssize_t wb_on_show(struct device *dev, struct device_attribute *attr,
{
struct ufs_hba *hba = dev_get_drvdata(dev);
- return scnprintf(buf, PAGE_SIZE, "%d\n", hba->wb_enabled);
+ return scnprintf(buf, PAGE_SIZE, "%d\n", hba->dev_info.wb_enabled);
}
static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 14dfda735adf..45bebca29fdd 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -527,22 +527,27 @@ struct ufs_vreg_info {
};
struct ufs_dev_info {
- bool f_power_on_wp_en;
+ bool f_power_on_wp_en;
/* Keeps information if any of the LU is power on write protected */
- bool is_lu_power_on_wp;
+ bool is_lu_power_on_wp;
/* Maximum number of general LU supported by the UFS device */
- u8 max_lu_supported;
- u8 wb_dedicated_lu;
- u16 wmanufacturerid;
- /*UFS device Product Name */
- u8 *model;
- u16 wspecversion;
- u32 clk_gating_wait_us;
- u32 d_ext_ufs_feature_sup;
- u8 b_wb_buffer_type;
- u32 d_wb_alloc_units;
- bool b_rpm_dev_flush_capable;
- u8 b_presrv_uspc_en;
+ u8 max_lu_supported;
+ u16 wmanufacturerid;
+ /* UFS device Product Name */
+ u8 *model;
+ u16 wspecversion;
+ u32 clk_gating_wait_us;
+ u32 d_ext_ufs_feature_sup;
+
+ /* UFS WB related flags */
+ bool wb_enabled;
+ bool wb_buf_flush_enabled;
+ u8 wb_dedicated_lu;
+ u8 b_wb_buffer_type;
+ u32 d_wb_alloc_units;
+
+ bool b_rpm_dev_flush_capable;
+ u8 b_presrv_uspc_en;
};
/**
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 6a5532b752aa..528c257df48c 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -589,8 +589,8 @@ static void ufshcd_device_reset(struct ufs_hba *hba)
if (!err) {
ufshcd_set_ufs_dev_active(hba);
if (ufshcd_is_wb_allowed(hba)) {
- hba->wb_enabled = false;
- hba->wb_buf_flush_enabled = false;
+ hba->dev_info.wb_enabled = false;
+ hba->dev_info.wb_buf_flush_enabled = false;
}
}
if (err != -EOPNOTSUPP)
@@ -5359,7 +5359,7 @@ int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
if (!ufshcd_is_wb_allowed(hba))
return 0;
- if (!(enable ^ hba->wb_enabled))
+ if (!(enable ^ hba->dev_info.wb_enabled))
return 0;
if (enable)
opcode = UPIU_QUERY_OPCODE_SET_FLAG;
@@ -5375,7 +5375,7 @@ int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
return ret;
}
- hba->wb_enabled = enable;
+ hba->dev_info.wb_enabled = enable;
dev_dbg(hba->dev, "%s write booster %s %d\n",
__func__, enable ? "enable" : "disable", ret);
@@ -5415,7 +5415,7 @@ static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba)
int ret;
u8 index;
- if (!ufshcd_is_wb_allowed(hba) || hba->wb_buf_flush_enabled)
+ if (!ufshcd_is_wb_allowed(hba) || hba->dev_info.wb_buf_flush_enabled)
return 0;
index = ufshcd_wb_get_query_index(hba);
@@ -5426,7 +5426,7 @@ static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba)
dev_err(hba->dev, "%s WB - buf flush enable failed %d\n",
__func__, ret);
else
- hba->wb_buf_flush_enabled = true;
+ hba->dev_info.wb_buf_flush_enabled = true;
dev_dbg(hba->dev, "WB - Flush enabled: %d\n", ret);
return ret;
@@ -5437,7 +5437,7 @@ static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba)
int ret;
u8 index;
- if (!ufshcd_is_wb_allowed(hba) || !hba->wb_buf_flush_enabled)
+ if (!ufshcd_is_wb_allowed(hba) || !hba->dev_info.wb_buf_flush_enabled)
return 0;
index = ufshcd_wb_get_query_index(hba);
@@ -5448,7 +5448,7 @@ static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba)
dev_warn(hba->dev, "%s: WB - buf flush disable failed %d\n",
__func__, ret);
} else {
- hba->wb_buf_flush_enabled = false;
+ hba->dev_info.wb_buf_flush_enabled = false;
dev_dbg(hba->dev, "WB - Flush disabled: %d\n", ret);
}
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 2a97006a2c93..45c3eca88f0e 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -805,8 +805,6 @@ struct ufs_hba {
struct device bsg_dev;
struct request_queue *bsg_queue;
- bool wb_buf_flush_enabled;
- bool wb_enabled;
struct delayed_work rpm_dev_flush_recheck_work;
#ifdef CONFIG_SCSI_UFS_CRYPTO
--
2.17.1
Hi Bean,
On Fri, 2020-12-11 at 15:00 +0100, Bean Huo wrote:
> From: Bean Huo <[email protected]>
>
> UFS device-related flags should be grouped in ufs_dev_info. Take
> wb_enabled and wb_buf_flush_enabled out from the struct ufs_hba,
> group them to struct ufs_dev_info, and align the names of the structure
> members vertically
>
> Signed-off-by: Bean Huo <[email protected]>
> ---
> drivers/scsi/ufs/ufs-sysfs.c | 2 +-
> drivers/scsi/ufs/ufs.h | 33 +++++++++++++++++++--------------
> drivers/scsi/ufs/ufshcd.c | 16 ++++++++--------
> drivers/scsi/ufs/ufshcd.h | 2 --
> 4 files changed, 28 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
> index 2b4e9fe935cc..4bd7e18bb486 100644
> --- a/drivers/scsi/ufs/ufs-sysfs.c
> +++ b/drivers/scsi/ufs/ufs-sysfs.c
> @@ -194,7 +194,7 @@ static ssize_t wb_on_show(struct device *dev, struct device_attribute *attr,
> {
> struct ufs_hba *hba = dev_get_drvdata(dev);
>
> - return scnprintf(buf, PAGE_SIZE, "%d\n", hba->wb_enabled);
> + return scnprintf(buf, PAGE_SIZE, "%d\n", hba->dev_info.wb_enabled);
> }
>
> static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index 14dfda735adf..45bebca29fdd 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -527,22 +527,27 @@ struct ufs_vreg_info {
> };
>
> struct ufs_dev_info {
> - bool f_power_on_wp_en;
> + bool f_power_on_wp_en;
> /* Keeps information if any of the LU is power on write protected */
> - bool is_lu_power_on_wp;
> + bool is_lu_power_on_wp;
> /* Maximum number of general LU supported by the UFS device */
> - u8 max_lu_supported;
> - u8 wb_dedicated_lu;
> - u16 wmanufacturerid;
> - /*UFS device Product Name */
> - u8 *model;
> - u16 wspecversion;
> - u32 clk_gating_wait_us;
> - u32 d_ext_ufs_feature_sup;
> - u8 b_wb_buffer_type;
> - u32 d_wb_alloc_units;
> - bool b_rpm_dev_flush_capable;
> - u8 b_presrv_uspc_en;
> + u8 max_lu_supported;
> + u16 wmanufacturerid;
> + /* UFS device Product Name */
> + u8 *model;
> + u16 wspecversion;
> + u32 clk_gating_wait_us;
> + u32 d_ext_ufs_feature_sup;
> +
> + /* UFS WB related flags */
> + bool wb_enabled;
> + bool wb_buf_flush_enabled;
> + u8 wb_dedicated_lu;
> + u8 b_wb_buffer_type;
> + u32 d_wb_alloc_units;
> +
> + bool b_rpm_dev_flush_capable;
> + u8 b_presrv_uspc_en;
Perhaps we could unify the style of these WB related stuff to wb_* ?
Besides, I am not sure if using tab instead space between the type and
name in this struct is a good idea.
Thanks,
Stanley Chu
> };
>
> /**
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 6a5532b752aa..528c257df48c 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -589,8 +589,8 @@ static void ufshcd_device_reset(struct ufs_hba *hba)
> if (!err) {
> ufshcd_set_ufs_dev_active(hba);
> if (ufshcd_is_wb_allowed(hba)) {
> - hba->wb_enabled = false;
> - hba->wb_buf_flush_enabled = false;
> + hba->dev_info.wb_enabled = false;
> + hba->dev_info.wb_buf_flush_enabled = false;
> }
> }
> if (err != -EOPNOTSUPP)
> @@ -5359,7 +5359,7 @@ int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
> if (!ufshcd_is_wb_allowed(hba))
> return 0;
>
> - if (!(enable ^ hba->wb_enabled))
> + if (!(enable ^ hba->dev_info.wb_enabled))
> return 0;
> if (enable)
> opcode = UPIU_QUERY_OPCODE_SET_FLAG;
> @@ -5375,7 +5375,7 @@ int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
> return ret;
> }
>
> - hba->wb_enabled = enable;
> + hba->dev_info.wb_enabled = enable;
> dev_dbg(hba->dev, "%s write booster %s %d\n",
> __func__, enable ? "enable" : "disable", ret);
>
> @@ -5415,7 +5415,7 @@ static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba)
> int ret;
> u8 index;
>
> - if (!ufshcd_is_wb_allowed(hba) || hba->wb_buf_flush_enabled)
> + if (!ufshcd_is_wb_allowed(hba) || hba->dev_info.wb_buf_flush_enabled)
> return 0;
>
> index = ufshcd_wb_get_query_index(hba);
> @@ -5426,7 +5426,7 @@ static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba)
> dev_err(hba->dev, "%s WB - buf flush enable failed %d\n",
> __func__, ret);
> else
> - hba->wb_buf_flush_enabled = true;
> + hba->dev_info.wb_buf_flush_enabled = true;
>
> dev_dbg(hba->dev, "WB - Flush enabled: %d\n", ret);
> return ret;
> @@ -5437,7 +5437,7 @@ static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba)
> int ret;
> u8 index;
>
> - if (!ufshcd_is_wb_allowed(hba) || !hba->wb_buf_flush_enabled)
> + if (!ufshcd_is_wb_allowed(hba) || !hba->dev_info.wb_buf_flush_enabled)
> return 0;
>
> index = ufshcd_wb_get_query_index(hba);
> @@ -5448,7 +5448,7 @@ static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba)
> dev_warn(hba->dev, "%s: WB - buf flush disable failed %d\n",
> __func__, ret);
> } else {
> - hba->wb_buf_flush_enabled = false;
> + hba->dev_info.wb_buf_flush_enabled = false;
> dev_dbg(hba->dev, "WB - Flush disabled: %d\n", ret);
> }
>
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 2a97006a2c93..45c3eca88f0e 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -805,8 +805,6 @@ struct ufs_hba {
>
> struct device bsg_dev;
> struct request_queue *bsg_queue;
> - bool wb_buf_flush_enabled;
> - bool wb_enabled;
> struct delayed_work rpm_dev_flush_recheck_work;
>
> #ifdef CONFIG_SCSI_UFS_CRYPTO
Hi Bean,
On Fri, 2020-12-11 at 15:00 +0100, Bean Huo wrote:
> From: Bean Huo <[email protected]>
>
> Delete ufshcd_wb_buf_flush_enable() and ufshcd_wb_buf_flush_disable(),
> move the implementation into ufshcd_wb_toggle_flush().
>
> Signed-off-by: Bean Huo <[email protected]>
> ---
> drivers/scsi/ufs/ufshcd.c | 69 ++++++++++++++-------------------------
> 1 file changed, 24 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 0998e6103cd7..fb3c98724005 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -244,10 +244,8 @@ static int ufshcd_setup_vreg(struct ufs_hba *hba, bool on);
> static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba,
> struct ufs_vreg *vreg);
> static int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag);
> -static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba);
> -static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba);
> static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set);
> -static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable);
> +static inline int ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable);
> static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba);
> static void ufshcd_hba_vreg_set_hpm(struct ufs_hba *hba);
>
> @@ -5398,60 +5396,41 @@ static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set)
> index, NULL);
> }
>
> -static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable)
> -{
> - if (hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)
> - return;
> -
> - if (enable)
> - ufshcd_wb_buf_flush_enable(hba);
> - else
> - ufshcd_wb_buf_flush_disable(hba);
> -
> -}
> -
> -static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba)
> +static inline int ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable)
> {
> int ret;
> u8 index;
> + enum query_opcode opcode;
>
> - if (!ufshcd_is_wb_allowed(hba) || hba->dev_info.wb_buf_flush_enabled)
> + if (hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)
> return 0;
>
> - index = ufshcd_wb_get_query_index(hba);
> - ret = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_SET_FLAG,
> - QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN,
> - index, NULL);
> - if (ret)
> - dev_err(hba->dev, "%s WB - buf flush enable failed %d\n",
> - __func__, ret);
> - else
> - hba->dev_info.wb_buf_flush_enabled = true;
> -
> - dev_dbg(hba->dev, "WB - Flush enabled: %d\n", ret);
> - return ret;
> -}
> -
> -static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba)
> -{
> - int ret;
> - u8 index;
> -
> - if (!ufshcd_is_wb_allowed(hba) || !hba->dev_info.wb_buf_flush_enabled)
> + if (!ufshcd_is_wb_allowed(hba) ||
> + hba->dev_info.wb_buf_flush_enabled == enable)
> return 0;
>
> + if (enable)
> + opcode = UPIU_QUERY_OPCODE_SET_FLAG;
> + else
> + opcode = UPIU_QUERY_OPCODE_CLEAR_FLAG;
> +
> index = ufshcd_wb_get_query_index(hba);
> - ret = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_CLEAR_FLAG,
> - QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN,
> - index, NULL);
> + ret = ufshcd_query_flag_retry(hba, opcode,
> + QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN, index,
> + NULL);
> if (ret) {
> - dev_warn(hba->dev, "%s: WB - buf flush disable failed %d\n",
> - __func__, ret);
> - } else {
> - hba->dev_info.wb_buf_flush_enabled = false;
> - dev_dbg(hba->dev, "WB - Flush disabled: %d\n", ret);
> + dev_err(hba->dev, "%s WB-Buf Flush %s failed %d\n", __func__,
> + enable ? "enable" : "disable", ret);
> + goto out;
> }
>
> + if (enable)
> + hba->dev_info.wb_buf_flush_enabled = true;
> + else
> + hba->dev_info.wb_buf_flush_enabled = false;
Perhaps this could be simpler as below?
hba->dev_info.wb_buf_flush_enabled = enable;
Thanks,
Stanley Chu
> +
> + dev_dbg(hba->dev, "WB-Buf Flush %s\n", enable ? "enabled" : "disabled");
> +out:
> return ret;
> }
>
On Tue, 2020-12-15 at 17:07 +0800, Stanley Chu wrote:
> > if (ret) {
> > - dev_warn(hba->dev, "%s: WB - buf flush disable failed
> > %d\n",
> > - __func__, ret);
> > - } else {
> > - hba->dev_info.wb_buf_flush_enabled = false;
> > - dev_dbg(hba->dev, "WB - Flush disabled: %d\n", ret);
> > + dev_err(hba->dev, "%s WB-Buf Flush %s failed %d\n",
> > __func__,
> > + enable ? "enable" : "disable", ret);
> > + goto out;
> > }
> >
> > + if (enable)
> > + hba->dev_info.wb_buf_flush_enabled = true;
> > + else
> > + hba->dev_info.wb_buf_flush_enabled = false;
>
> Perhaps this could be simpler as below?
>
> hba->dev_info.wb_buf_flush_enabled = enable;
Thanks, will be changed in next version.
Bean
On Tue, 2020-12-15 at 17:01 +0800, Stanley Chu wrote:
> > + bool wb_buf_flush_enabled;
> > + u8 wb_dedicated_lu;
> > + u8 b_wb_buffer_type;
> > + u32 d_wb_alloc_units;
> > +
> > + bool b_rpm_dev_flush_capable;
> > + u8 b_presrv_uspc_en;
>
> Perhaps we could unify the style of these WB related stuff to wb_* ?
yes, agree. I will change them.
>
> Besides, I am not sure if using tab instead space between the type
> and
> name in this struct is a good idea.
>
using space, in addition single space, type and parameter names are
mixed.
use space:
/* UFS WB related flags */
bool wb_enabled;
bool wb_buf_flush_enabled;
u8
wb_dedicated_lu;
u8 b_wb_buffer_type;
u32 d_wb_alloc_units;
use table:
/* UFS WB related flags */
bool wb_enabled;
bool wb_buf_flush_enabled;
u8 wb_dedicated_lu;
u8 b_wb_buffer_type;
u32 d_wb_alloc_units;
I think, the result is very clear comparing above two examples. yes,
there is no explicit stipulation that we must use space or tab. Both
styles exist in Linux. Maybe this is just matter of personal interest.
Bean
> Thanks,
> Stanley Chu
On Tue, 2020-12-15 at 10:42 +0100, Bean Huo wrote:
> On Tue, 2020-12-15 at 17:01 +0800, Stanley Chu wrote:
> > > + bool wb_buf_flush_enabled;
> > > + u8 wb_dedicated_lu;
> > > + u8 b_wb_buffer_type;
> > > + u32 d_wb_alloc_units;
> > > +
> > > + bool b_rpm_dev_flush_capable;
> > > + u8 b_presrv_uspc_en;
> >
> > Perhaps we could unify the style of these WB related stuff to wb_* ?
>
> yes, agree. I will change them.
>
> >
> > Besides, I am not sure if using tab instead space between the type
> > and
> > name in this struct is a good idea.
> >
> using space, in addition single space, type and parameter names are
> mixed.
>
>
> use space:
>
> /* UFS WB related flags */
> bool wb_enabled;
> bool wb_buf_flush_enabled;
> u8
> wb_dedicated_lu;
> u8 b_wb_buffer_type;
> u32 d_wb_alloc_units;
>
> use table:
>
> /* UFS WB related flags */
> bool wb_enabled;
> bool wb_buf_flush_enabled;
> u8 wb_dedicated_lu;
> u8 b_wb_buffer_type;
> u32 d_wb_alloc_units;
>
> I think, the result is very clear comparing above two examples. yes,
> there is no explicit stipulation that we must use space or tab. Both
> styles exist in Linux. Maybe this is just matter of personal interest.
Hi Bean,
Yes, I got your point. I am fine with this style change, but just wonder
if it would be better to change all structures in all ufs headers (or at
least all structures in ufs.h) in the same time to make the style
unified in the same file?
Besides, we may need other reviewer's comments for the new style.
Thanks,
Stanley Chu
>
>
> Bean
>
> > Thanks,
> > Stanley Chu
>
On Fri, Dec 11, 2020 at 03:00:30PM +0100, Bean Huo wrote:
> From: Bean Huo <[email protected]>
>
> Currently UFS WriteBooster driver uses clock scaling up/down to set
> WB on/off, for the platform which doesn't support UFSHCD_CAP_CLK_SCALING,
> WB will be always on. Provide a sysfs attribute to enable/disable WB
> during runtime. Write 1/0 to "wb_on" sysfs node to enable/disable UFS WB.
>
> Reviewed-by: Avri Altman <[email protected]>
> Reviewed-by: Stanley Chu <[email protected]>
> Signed-off-by: Bean Huo <[email protected]>
> ---
> drivers/scsi/ufs/ufs-sysfs.c | 41 ++++++++++++++++++++++++++++++++++++
> drivers/scsi/ufs/ufshcd.c | 3 +--
> drivers/scsi/ufs/ufshcd.h | 2 ++
> 3 files changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
> index 08e72b7eef6a..2b4e9fe935cc 100644
> --- a/drivers/scsi/ufs/ufs-sysfs.c
> +++ b/drivers/scsi/ufs/ufs-sysfs.c
> @@ -189,6 +189,45 @@ static ssize_t auto_hibern8_store(struct device *dev,
> return count;
> }
>
> +static ssize_t wb_on_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct ufs_hba *hba = dev_get_drvdata(dev);
> +
> + return scnprintf(buf, PAGE_SIZE, "%d\n", hba->wb_enabled);
Please just use sysfs_emit().
> +}
> +
> +static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct ufs_hba *hba = dev_get_drvdata(dev);
> + unsigned int wb_enable;
> + ssize_t res;
> +
> + if (ufshcd_is_clkscaling_supported(hba)) {
> + /*
> + * If the platform supports UFSHCD_CAP_AUTO_BKOPS_SUSPEND,
> + * turn WB on/off will be done while clock scaling up/down.
> + */
> + dev_warn(dev, "To control WB through wb_on is not allowed!\n");
> + return -EOPNOTSUPP;
> + }
> + if (!ufshcd_is_wb_allowed(hba))
> + return -EOPNOTSUPP;
> +
> + if (kstrtouint(buf, 0, &wb_enable))
> + return -EINVAL;
> +
> + if (wb_enable != 0 && wb_enable != 1)
> + return -EINVAL;
> +
> + pm_runtime_get_sync(hba->dev);
> + res = ufshcd_wb_ctrl(hba, wb_enable);
> + pm_runtime_put_sync(hba->dev);
> +
> + return res < 0 ? res : count;
> +}
Where is the new Documentation/ABI/ update for this new sysfs file you
are adding?
thanks,
greg k-h