* [PATCH v4 1/6] scsi: ufs: Add "wb_on" sysfs node to control WB on/off
2020-12-11 14:00 [PATCH v4 0/6] Several changes for UFS WriteBooster Bean Huo
@ 2020-12-11 14:00 ` Bean Huo
2020-12-15 10:32 ` Greg KH
2020-12-11 14:00 ` [PATCH v4 2/6] scsi: ufs: Changes comment in the function ufshcd_wb_probe() Bean Huo
` (4 subsequent siblings)
5 siblings, 1 reply; 16+ messages in thread
From: Bean Huo @ 2020-12-11 14:00 UTC (permalink / raw)
To: alim.akhtar, avri.altman, asutoshd, jejb, martin.petersen,
stanley.chu, beanhuo, bvanassche, tomas.winkler, cang
Cc: linux-scsi, linux-kernel
From: Bean Huo <beanhuo@micron.com>
Currently UFS WriteBooster driver uses clock scaling up/down to set
WB on/off, for the platform which doesn't support UFSHCD_CAP_CLK_SCALING,
WB will be always on. Provide a sysfs attribute to enable/disable WB
during runtime. Write 1/0 to "wb_on" sysfs node to enable/disable UFS WB.
Reviewed-by: Avri Altman <avri.altman@wdc.com>
Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
Signed-off-by: Bean Huo <beanhuo@micron.com>
---
drivers/scsi/ufs/ufs-sysfs.c | 41 ++++++++++++++++++++++++++++++++++++
drivers/scsi/ufs/ufshcd.c | 3 +--
drivers/scsi/ufs/ufshcd.h | 2 ++
3 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index 08e72b7eef6a..2b4e9fe935cc 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -189,6 +189,45 @@ static ssize_t auto_hibern8_store(struct device *dev,
return count;
}
+static ssize_t wb_on_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct ufs_hba *hba = dev_get_drvdata(dev);
+
+ return scnprintf(buf, PAGE_SIZE, "%d\n", hba->wb_enabled);
+}
+
+static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct ufs_hba *hba = dev_get_drvdata(dev);
+ unsigned int wb_enable;
+ ssize_t res;
+
+ if (ufshcd_is_clkscaling_supported(hba)) {
+ /*
+ * If the platform supports UFSHCD_CAP_AUTO_BKOPS_SUSPEND,
+ * turn WB on/off will be done while clock scaling up/down.
+ */
+ dev_warn(dev, "To control WB through wb_on is not allowed!\n");
+ return -EOPNOTSUPP;
+ }
+ if (!ufshcd_is_wb_allowed(hba))
+ return -EOPNOTSUPP;
+
+ if (kstrtouint(buf, 0, &wb_enable))
+ return -EINVAL;
+
+ if (wb_enable != 0 && wb_enable != 1)
+ return -EINVAL;
+
+ pm_runtime_get_sync(hba->dev);
+ res = ufshcd_wb_ctrl(hba, wb_enable);
+ pm_runtime_put_sync(hba->dev);
+
+ return res < 0 ? res : count;
+}
+
static DEVICE_ATTR_RW(rpm_lvl);
static DEVICE_ATTR_RO(rpm_target_dev_state);
static DEVICE_ATTR_RO(rpm_target_link_state);
@@ -196,6 +235,7 @@ static DEVICE_ATTR_RW(spm_lvl);
static DEVICE_ATTR_RO(spm_target_dev_state);
static DEVICE_ATTR_RO(spm_target_link_state);
static DEVICE_ATTR_RW(auto_hibern8);
+static DEVICE_ATTR_RW(wb_on);
static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
&dev_attr_rpm_lvl.attr,
@@ -205,6 +245,7 @@ static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
&dev_attr_spm_target_dev_state.attr,
&dev_attr_spm_target_link_state.attr,
&dev_attr_auto_hibern8.attr,
+ &dev_attr_wb_on.attr,
NULL
};
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index e221add25a7e..5e1dcf4de67e 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -246,7 +246,6 @@ static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba,
static int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag);
static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba);
static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba);
-static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable);
static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set);
static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable);
static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba);
@@ -5351,7 +5350,7 @@ static void ufshcd_bkops_exception_event_handler(struct ufs_hba *hba)
__func__, err);
}
-static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
+int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
{
int ret;
u8 index;
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 9bb5f0ed4124..2a97006a2c93 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -1068,6 +1068,8 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
u8 *desc_buff, int *buff_len,
enum query_opcode desc_op);
+int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable);
+
/* Wrapper functions for safely calling variant operations */
static inline const char *ufshcd_get_var_name(struct ufs_hba *hba)
{
--
2.17.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v4 1/6] scsi: ufs: Add "wb_on" sysfs node to control WB on/off
2020-12-11 14:00 ` [PATCH v4 1/6] scsi: ufs: Add "wb_on" sysfs node to control WB on/off Bean Huo
@ 2020-12-15 10:32 ` Greg KH
2020-12-15 19:56 ` Bean Huo
0 siblings, 1 reply; 16+ messages in thread
From: Greg KH @ 2020-12-15 10:32 UTC (permalink / raw)
To: Bean Huo
Cc: alim.akhtar, avri.altman, asutoshd, jejb, martin.petersen,
stanley.chu, beanhuo, bvanassche, tomas.winkler, cang,
linux-scsi, linux-kernel
On Fri, Dec 11, 2020 at 03:00:30PM +0100, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
>
> Currently UFS WriteBooster driver uses clock scaling up/down to set
> WB on/off, for the platform which doesn't support UFSHCD_CAP_CLK_SCALING,
> WB will be always on. Provide a sysfs attribute to enable/disable WB
> during runtime. Write 1/0 to "wb_on" sysfs node to enable/disable UFS WB.
>
> Reviewed-by: Avri Altman <avri.altman@wdc.com>
> Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
> drivers/scsi/ufs/ufs-sysfs.c | 41 ++++++++++++++++++++++++++++++++++++
> drivers/scsi/ufs/ufshcd.c | 3 +--
> drivers/scsi/ufs/ufshcd.h | 2 ++
> 3 files changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
> index 08e72b7eef6a..2b4e9fe935cc 100644
> --- a/drivers/scsi/ufs/ufs-sysfs.c
> +++ b/drivers/scsi/ufs/ufs-sysfs.c
> @@ -189,6 +189,45 @@ static ssize_t auto_hibern8_store(struct device *dev,
> return count;
> }
>
> +static ssize_t wb_on_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct ufs_hba *hba = dev_get_drvdata(dev);
> +
> + return scnprintf(buf, PAGE_SIZE, "%d\n", hba->wb_enabled);
Please just use sysfs_emit().
> +}
> +
> +static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct ufs_hba *hba = dev_get_drvdata(dev);
> + unsigned int wb_enable;
> + ssize_t res;
> +
> + if (ufshcd_is_clkscaling_supported(hba)) {
> + /*
> + * If the platform supports UFSHCD_CAP_AUTO_BKOPS_SUSPEND,
> + * turn WB on/off will be done while clock scaling up/down.
> + */
> + dev_warn(dev, "To control WB through wb_on is not allowed!\n");
> + return -EOPNOTSUPP;
> + }
> + if (!ufshcd_is_wb_allowed(hba))
> + return -EOPNOTSUPP;
> +
> + if (kstrtouint(buf, 0, &wb_enable))
> + return -EINVAL;
> +
> + if (wb_enable != 0 && wb_enable != 1)
> + return -EINVAL;
> +
> + pm_runtime_get_sync(hba->dev);
> + res = ufshcd_wb_ctrl(hba, wb_enable);
> + pm_runtime_put_sync(hba->dev);
> +
> + return res < 0 ? res : count;
> +}
Where is the new Documentation/ABI/ update for this new sysfs file you
are adding?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v4 1/6] scsi: ufs: Add "wb_on" sysfs node to control WB on/off
2020-12-15 10:32 ` Greg KH
@ 2020-12-15 19:56 ` Bean Huo
0 siblings, 0 replies; 16+ messages in thread
From: Bean Huo @ 2020-12-15 19:56 UTC (permalink / raw)
To: Greg KH
Cc: alim.akhtar, avri.altman, asutoshd, jejb, martin.petersen,
stanley.chu, beanhuo, bvanassche, tomas.winkler, cang,
linux-scsi, linux-kernel
Greg k-h,
> >
> > +static ssize_t wb_on_show(struct device *dev, struct
> > device_attribute *attr,
> > + char *buf)
> > +{
> > + struct ufs_hba *hba = dev_get_drvdata(dev);
> > +
> > + return scnprintf(buf, PAGE_SIZE, "%d\n", hba->wb_enabled);
>
> Please just use sysfs_emit().
thanks, will be changed in next version.
>
> > +}
> > +
> > +static ssize_t wb_on_store(struct device *dev, struct
> > device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct ufs_hba *hba = dev_get_drvdata(dev);
> > + unsigned int wb_enable;
> > + ssize_t res;
> > +
> > + if (ufshcd_is_clkscaling_supported(hba)) {
> > + /*
> > + * If the platform supports
> > UFSHCD_CAP_AUTO_BKOPS_SUSPEND,
> > + * turn WB on/off will be done while clock scaling
> > up/down.
> > + */
> > + dev_warn(dev, "To control WB through wb_on is not
> > allowed!\n");
> > + return -EOPNOTSUPP;
> > + }
> > + if (!ufshcd_is_wb_allowed(hba))
> > + return -EOPNOTSUPP;
> > +
> > + if (kstrtouint(buf, 0, &wb_enable))
> > + return -EINVAL;
> > +
> > + if (wb_enable != 0 && wb_enable != 1)
> > + return -EINVAL;
> > +
> > + pm_runtime_get_sync(hba->dev);
> > + res = ufshcd_wb_ctrl(hba, wb_enable);
> > + pm_runtime_put_sync(hba->dev);
> > +
> > + return res < 0 ? res : count;
> > +}
>
> Where is the new Documentation/ABI/ update for this new sysfs file
> you
> are adding?
>
It is missing, and will add it.
thanks.
Bean
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 2/6] scsi: ufs: Changes comment in the function ufshcd_wb_probe()
2020-12-11 14:00 [PATCH v4 0/6] Several changes for UFS WriteBooster Bean Huo
2020-12-11 14:00 ` [PATCH v4 1/6] scsi: ufs: Add "wb_on" sysfs node to control WB on/off Bean Huo
@ 2020-12-11 14:00 ` Bean Huo
2020-12-11 14:00 ` [PATCH v4 3/6] scsi: ufs: Group UFS WB related flags to struct ufs_dev_info Bean Huo
` (3 subsequent siblings)
5 siblings, 0 replies; 16+ messages in thread
From: Bean Huo @ 2020-12-11 14:00 UTC (permalink / raw)
To: alim.akhtar, avri.altman, asutoshd, jejb, martin.petersen,
stanley.chu, beanhuo, bvanassche, tomas.winkler, cang
Cc: linux-scsi, linux-kernel
From: Bean Huo <beanhuo@micron.com>
USFHCD supports WriteBooster "LU dedicated buffer” mode and
“shared buffer” mode both, so changes the comment in the
function ufshcd_wb_probe().
Reviewed-by: Can Guo <cang@codeaurora.org>
Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
Signed-off-by: Bean Huo <beanhuo@micron.com>
---
drivers/scsi/ufs/ufshcd.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 5e1dcf4de67e..6a5532b752aa 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7232,10 +7232,9 @@ static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf)
goto wb_disabled;
/*
- * WB may be supported but not configured while provisioning.
- * The spec says, in dedicated wb buffer mode,
- * a max of 1 lun would have wb buffer configured.
- * Now only shared buffer mode is supported.
+ * WB may be supported but not configured while provisioning. The spec
+ * says, in dedicated wb buffer mode, a max of 1 lun would have wb
+ * buffer configured.
*/
dev_info->b_wb_buffer_type =
desc_buf[DEVICE_DESC_PARAM_WB_TYPE];
--
2.17.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v4 3/6] scsi: ufs: Group UFS WB related flags to struct ufs_dev_info
2020-12-11 14:00 [PATCH v4 0/6] Several changes for UFS WriteBooster Bean Huo
2020-12-11 14:00 ` [PATCH v4 1/6] scsi: ufs: Add "wb_on" sysfs node to control WB on/off Bean Huo
2020-12-11 14:00 ` [PATCH v4 2/6] scsi: ufs: Changes comment in the function ufshcd_wb_probe() Bean Huo
@ 2020-12-11 14:00 ` Bean Huo
2020-12-15 9:01 ` Stanley Chu
2020-12-11 14:00 ` [PATCH v4 4/6] scsi: ufs: Remove d_wb_alloc_units from " Bean Huo
` (2 subsequent siblings)
5 siblings, 1 reply; 16+ messages in thread
From: Bean Huo @ 2020-12-11 14:00 UTC (permalink / raw)
To: alim.akhtar, avri.altman, asutoshd, jejb, martin.petersen,
stanley.chu, beanhuo, bvanassche, tomas.winkler, cang
Cc: linux-scsi, linux-kernel
From: Bean Huo <beanhuo@micron.com>
UFS device-related flags should be grouped in ufs_dev_info. Take
wb_enabled and wb_buf_flush_enabled out from the struct ufs_hba,
group them to struct ufs_dev_info, and align the names of the structure
members vertically
Signed-off-by: Bean Huo <beanhuo@micron.com>
---
drivers/scsi/ufs/ufs-sysfs.c | 2 +-
drivers/scsi/ufs/ufs.h | 33 +++++++++++++++++++--------------
drivers/scsi/ufs/ufshcd.c | 16 ++++++++--------
drivers/scsi/ufs/ufshcd.h | 2 --
4 files changed, 28 insertions(+), 25 deletions(-)
diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index 2b4e9fe935cc..4bd7e18bb486 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -194,7 +194,7 @@ static ssize_t wb_on_show(struct device *dev, struct device_attribute *attr,
{
struct ufs_hba *hba = dev_get_drvdata(dev);
- return scnprintf(buf, PAGE_SIZE, "%d\n", hba->wb_enabled);
+ return scnprintf(buf, PAGE_SIZE, "%d\n", hba->dev_info.wb_enabled);
}
static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 14dfda735adf..45bebca29fdd 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -527,22 +527,27 @@ struct ufs_vreg_info {
};
struct ufs_dev_info {
- bool f_power_on_wp_en;
+ bool f_power_on_wp_en;
/* Keeps information if any of the LU is power on write protected */
- bool is_lu_power_on_wp;
+ bool is_lu_power_on_wp;
/* Maximum number of general LU supported by the UFS device */
- u8 max_lu_supported;
- u8 wb_dedicated_lu;
- u16 wmanufacturerid;
- /*UFS device Product Name */
- u8 *model;
- u16 wspecversion;
- u32 clk_gating_wait_us;
- u32 d_ext_ufs_feature_sup;
- u8 b_wb_buffer_type;
- u32 d_wb_alloc_units;
- bool b_rpm_dev_flush_capable;
- u8 b_presrv_uspc_en;
+ u8 max_lu_supported;
+ u16 wmanufacturerid;
+ /* UFS device Product Name */
+ u8 *model;
+ u16 wspecversion;
+ u32 clk_gating_wait_us;
+ u32 d_ext_ufs_feature_sup;
+
+ /* UFS WB related flags */
+ bool wb_enabled;
+ bool wb_buf_flush_enabled;
+ u8 wb_dedicated_lu;
+ u8 b_wb_buffer_type;
+ u32 d_wb_alloc_units;
+
+ bool b_rpm_dev_flush_capable;
+ u8 b_presrv_uspc_en;
};
/**
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 6a5532b752aa..528c257df48c 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -589,8 +589,8 @@ static void ufshcd_device_reset(struct ufs_hba *hba)
if (!err) {
ufshcd_set_ufs_dev_active(hba);
if (ufshcd_is_wb_allowed(hba)) {
- hba->wb_enabled = false;
- hba->wb_buf_flush_enabled = false;
+ hba->dev_info.wb_enabled = false;
+ hba->dev_info.wb_buf_flush_enabled = false;
}
}
if (err != -EOPNOTSUPP)
@@ -5359,7 +5359,7 @@ int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
if (!ufshcd_is_wb_allowed(hba))
return 0;
- if (!(enable ^ hba->wb_enabled))
+ if (!(enable ^ hba->dev_info.wb_enabled))
return 0;
if (enable)
opcode = UPIU_QUERY_OPCODE_SET_FLAG;
@@ -5375,7 +5375,7 @@ int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
return ret;
}
- hba->wb_enabled = enable;
+ hba->dev_info.wb_enabled = enable;
dev_dbg(hba->dev, "%s write booster %s %d\n",
__func__, enable ? "enable" : "disable", ret);
@@ -5415,7 +5415,7 @@ static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba)
int ret;
u8 index;
- if (!ufshcd_is_wb_allowed(hba) || hba->wb_buf_flush_enabled)
+ if (!ufshcd_is_wb_allowed(hba) || hba->dev_info.wb_buf_flush_enabled)
return 0;
index = ufshcd_wb_get_query_index(hba);
@@ -5426,7 +5426,7 @@ static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba)
dev_err(hba->dev, "%s WB - buf flush enable failed %d\n",
__func__, ret);
else
- hba->wb_buf_flush_enabled = true;
+ hba->dev_info.wb_buf_flush_enabled = true;
dev_dbg(hba->dev, "WB - Flush enabled: %d\n", ret);
return ret;
@@ -5437,7 +5437,7 @@ static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba)
int ret;
u8 index;
- if (!ufshcd_is_wb_allowed(hba) || !hba->wb_buf_flush_enabled)
+ if (!ufshcd_is_wb_allowed(hba) || !hba->dev_info.wb_buf_flush_enabled)
return 0;
index = ufshcd_wb_get_query_index(hba);
@@ -5448,7 +5448,7 @@ static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba)
dev_warn(hba->dev, "%s: WB - buf flush disable failed %d\n",
__func__, ret);
} else {
- hba->wb_buf_flush_enabled = false;
+ hba->dev_info.wb_buf_flush_enabled = false;
dev_dbg(hba->dev, "WB - Flush disabled: %d\n", ret);
}
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 2a97006a2c93..45c3eca88f0e 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -805,8 +805,6 @@ struct ufs_hba {
struct device bsg_dev;
struct request_queue *bsg_queue;
- bool wb_buf_flush_enabled;
- bool wb_enabled;
struct delayed_work rpm_dev_flush_recheck_work;
#ifdef CONFIG_SCSI_UFS_CRYPTO
--
2.17.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v4 3/6] scsi: ufs: Group UFS WB related flags to struct ufs_dev_info
2020-12-11 14:00 ` [PATCH v4 3/6] scsi: ufs: Group UFS WB related flags to struct ufs_dev_info Bean Huo
@ 2020-12-15 9:01 ` Stanley Chu
2020-12-15 9:42 ` Bean Huo
0 siblings, 1 reply; 16+ messages in thread
From: Stanley Chu @ 2020-12-15 9:01 UTC (permalink / raw)
To: Bean Huo
Cc: alim.akhtar, avri.altman, asutoshd, jejb, linux-scsi, linux-kernel
Hi Bean,
On Fri, 2020-12-11 at 15:00 +0100, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
>
> UFS device-related flags should be grouped in ufs_dev_info. Take
> wb_enabled and wb_buf_flush_enabled out from the struct ufs_hba,
> group them to struct ufs_dev_info, and align the names of the structure
> members vertically
>
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
> drivers/scsi/ufs/ufs-sysfs.c | 2 +-
> drivers/scsi/ufs/ufs.h | 33 +++++++++++++++++++--------------
> drivers/scsi/ufs/ufshcd.c | 16 ++++++++--------
> drivers/scsi/ufs/ufshcd.h | 2 --
> 4 files changed, 28 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
> index 2b4e9fe935cc..4bd7e18bb486 100644
> --- a/drivers/scsi/ufs/ufs-sysfs.c
> +++ b/drivers/scsi/ufs/ufs-sysfs.c
> @@ -194,7 +194,7 @@ static ssize_t wb_on_show(struct device *dev, struct device_attribute *attr,
> {
> struct ufs_hba *hba = dev_get_drvdata(dev);
>
> - return scnprintf(buf, PAGE_SIZE, "%d\n", hba->wb_enabled);
> + return scnprintf(buf, PAGE_SIZE, "%d\n", hba->dev_info.wb_enabled);
> }
>
> static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index 14dfda735adf..45bebca29fdd 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -527,22 +527,27 @@ struct ufs_vreg_info {
> };
>
> struct ufs_dev_info {
> - bool f_power_on_wp_en;
> + bool f_power_on_wp_en;
> /* Keeps information if any of the LU is power on write protected */
> - bool is_lu_power_on_wp;
> + bool is_lu_power_on_wp;
> /* Maximum number of general LU supported by the UFS device */
> - u8 max_lu_supported;
> - u8 wb_dedicated_lu;
> - u16 wmanufacturerid;
> - /*UFS device Product Name */
> - u8 *model;
> - u16 wspecversion;
> - u32 clk_gating_wait_us;
> - u32 d_ext_ufs_feature_sup;
> - u8 b_wb_buffer_type;
> - u32 d_wb_alloc_units;
> - bool b_rpm_dev_flush_capable;
> - u8 b_presrv_uspc_en;
> + u8 max_lu_supported;
> + u16 wmanufacturerid;
> + /* UFS device Product Name */
> + u8 *model;
> + u16 wspecversion;
> + u32 clk_gating_wait_us;
> + u32 d_ext_ufs_feature_sup;
> +
> + /* UFS WB related flags */
> + bool wb_enabled;
> + bool wb_buf_flush_enabled;
> + u8 wb_dedicated_lu;
> + u8 b_wb_buffer_type;
> + u32 d_wb_alloc_units;
> +
> + bool b_rpm_dev_flush_capable;
> + u8 b_presrv_uspc_en;
Perhaps we could unify the style of these WB related stuff to wb_* ?
Besides, I am not sure if using tab instead space between the type and
name in this struct is a good idea.
Thanks,
Stanley Chu
> };
>
> /**
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 6a5532b752aa..528c257df48c 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -589,8 +589,8 @@ static void ufshcd_device_reset(struct ufs_hba *hba)
> if (!err) {
> ufshcd_set_ufs_dev_active(hba);
> if (ufshcd_is_wb_allowed(hba)) {
> - hba->wb_enabled = false;
> - hba->wb_buf_flush_enabled = false;
> + hba->dev_info.wb_enabled = false;
> + hba->dev_info.wb_buf_flush_enabled = false;
> }
> }
> if (err != -EOPNOTSUPP)
> @@ -5359,7 +5359,7 @@ int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
> if (!ufshcd_is_wb_allowed(hba))
> return 0;
>
> - if (!(enable ^ hba->wb_enabled))
> + if (!(enable ^ hba->dev_info.wb_enabled))
> return 0;
> if (enable)
> opcode = UPIU_QUERY_OPCODE_SET_FLAG;
> @@ -5375,7 +5375,7 @@ int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
> return ret;
> }
>
> - hba->wb_enabled = enable;
> + hba->dev_info.wb_enabled = enable;
> dev_dbg(hba->dev, "%s write booster %s %d\n",
> __func__, enable ? "enable" : "disable", ret);
>
> @@ -5415,7 +5415,7 @@ static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba)
> int ret;
> u8 index;
>
> - if (!ufshcd_is_wb_allowed(hba) || hba->wb_buf_flush_enabled)
> + if (!ufshcd_is_wb_allowed(hba) || hba->dev_info.wb_buf_flush_enabled)
> return 0;
>
> index = ufshcd_wb_get_query_index(hba);
> @@ -5426,7 +5426,7 @@ static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba)
> dev_err(hba->dev, "%s WB - buf flush enable failed %d\n",
> __func__, ret);
> else
> - hba->wb_buf_flush_enabled = true;
> + hba->dev_info.wb_buf_flush_enabled = true;
>
> dev_dbg(hba->dev, "WB - Flush enabled: %d\n", ret);
> return ret;
> @@ -5437,7 +5437,7 @@ static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba)
> int ret;
> u8 index;
>
> - if (!ufshcd_is_wb_allowed(hba) || !hba->wb_buf_flush_enabled)
> + if (!ufshcd_is_wb_allowed(hba) || !hba->dev_info.wb_buf_flush_enabled)
> return 0;
>
> index = ufshcd_wb_get_query_index(hba);
> @@ -5448,7 +5448,7 @@ static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba)
> dev_warn(hba->dev, "%s: WB - buf flush disable failed %d\n",
> __func__, ret);
> } else {
> - hba->wb_buf_flush_enabled = false;
> + hba->dev_info.wb_buf_flush_enabled = false;
> dev_dbg(hba->dev, "WB - Flush disabled: %d\n", ret);
> }
>
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 2a97006a2c93..45c3eca88f0e 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -805,8 +805,6 @@ struct ufs_hba {
>
> struct device bsg_dev;
> struct request_queue *bsg_queue;
> - bool wb_buf_flush_enabled;
> - bool wb_enabled;
> struct delayed_work rpm_dev_flush_recheck_work;
>
> #ifdef CONFIG_SCSI_UFS_CRYPTO
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v4 3/6] scsi: ufs: Group UFS WB related flags to struct ufs_dev_info
2020-12-15 9:01 ` Stanley Chu
@ 2020-12-15 9:42 ` Bean Huo
2020-12-15 10:11 ` Stanley Chu
0 siblings, 1 reply; 16+ messages in thread
From: Bean Huo @ 2020-12-15 9:42 UTC (permalink / raw)
To: Stanley Chu
Cc: alim.akhtar, avri.altman, asutoshd, jejb, linux-scsi, linux-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 3/6] scsi: ufs: Group UFS WB related flags to struct ufs_dev_info
2020-12-15 9:42 ` Bean Huo
@ 2020-12-15 10:11 ` Stanley Chu
0 siblings, 0 replies; 16+ messages in thread
From: Stanley Chu @ 2020-12-15 10:11 UTC (permalink / raw)
To: Bean Huo
Cc: alim.akhtar, avri.altman, asutoshd, jejb, linux-scsi, linux-kernel
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
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 4/6] scsi: ufs: Remove d_wb_alloc_units from struct ufs_dev_info
2020-12-11 14:00 [PATCH v4 0/6] Several changes for UFS WriteBooster Bean Huo
` (2 preceding siblings ...)
2020-12-11 14:00 ` [PATCH v4 3/6] scsi: ufs: Group UFS WB related flags to struct ufs_dev_info Bean Huo
@ 2020-12-11 14:00 ` Bean Huo
2020-12-15 8:57 ` Stanley Chu
2020-12-11 14:00 ` [PATCH v4 5/6] scsi: ufs: Cleanup WB buffer flush toggle implementation Bean Huo
2020-12-11 14:00 ` [PATCH v4 6/6] scsi: ufs: Keep device active mode only fWriteBoosterBufferFlushDuringHibernate == 1 Bean Huo
5 siblings, 1 reply; 16+ messages in thread
From: Bean Huo @ 2020-12-11 14:00 UTC (permalink / raw)
To: alim.akhtar, avri.altman, asutoshd, jejb, martin.petersen,
stanley.chu, beanhuo, bvanassche, tomas.winkler, cang
Cc: linux-scsi, linux-kernel
From: Bean Huo <beanhuo@micron.com>
d_wb_alloc_units only be used while WB probe, just used to confirm the
condition that "if bWriteBoosterBufferType is set to 01h but
dNumSharedWriteBoosterBufferAllocUnits is set to zero, the WriteBooster
feature is disabled". So, don't need to keep it in runtime.
Signed-off-by: Bean Huo <beanhuo@micron.com>
---
drivers/scsi/ufs/ufs.h | 1 -
drivers/scsi/ufs/ufshcd.c | 6 ++----
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 45bebca29fdd..8ed342e43883 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -544,7 +544,6 @@ struct ufs_dev_info {
bool wb_buf_flush_enabled;
u8 wb_dedicated_lu;
u8 b_wb_buffer_type;
- u32 d_wb_alloc_units;
bool b_rpm_dev_flush_capable;
u8 b_presrv_uspc_en;
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 528c257df48c..0998e6103cd7 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7243,10 +7243,8 @@ static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf)
desc_buf[DEVICE_DESC_PARAM_WB_PRESRV_USRSPC_EN];
if (dev_info->b_wb_buffer_type == WB_BUF_MODE_SHARED) {
- dev_info->d_wb_alloc_units =
- get_unaligned_be32(desc_buf +
- DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS);
- if (!dev_info->d_wb_alloc_units)
+ if (!get_unaligned_be32(desc_buf +
+ DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS))
goto wb_disabled;
} else {
for (lun = 0; lun < UFS_UPIU_MAX_WB_LUN_ID; lun++) {
--
2.17.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v4 4/6] scsi: ufs: Remove d_wb_alloc_units from struct ufs_dev_info
2020-12-11 14:00 ` [PATCH v4 4/6] scsi: ufs: Remove d_wb_alloc_units from " Bean Huo
@ 2020-12-15 8:57 ` Stanley Chu
2020-12-15 9:59 ` Bean Huo
0 siblings, 1 reply; 16+ messages in thread
From: Stanley Chu @ 2020-12-15 8:57 UTC (permalink / raw)
To: Bean Huo
Cc: alim.akhtar, avri.altman, asutoshd, jejb, martin.petersen,
beanhuo, bvanassche, tomas.winkler, cang, linux-scsi,
linux-kernel
Hi Bean,
On Fri, 2020-12-11 at 15:00 +0100, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
>
> d_wb_alloc_units only be used while WB probe, just used to confirm the
> condition that "if bWriteBoosterBufferType is set to 01h but
> dNumSharedWriteBoosterBufferAllocUnits is set to zero, the WriteBooster
> feature is disabled". So, don't need to keep it in runtime.
>
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
> drivers/scsi/ufs/ufs.h | 1 -
> drivers/scsi/ufs/ufshcd.c | 6 ++----
> 2 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index 45bebca29fdd..8ed342e43883 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -544,7 +544,6 @@ struct ufs_dev_info {
> bool wb_buf_flush_enabled;
> u8 wb_dedicated_lu;
> u8 b_wb_buffer_type;
> - u32 d_wb_alloc_units;
Perhaps below two fields could be also removed from struct ufs_dev_info
for the same reason?
u32 d_ext_ufs_feature_sup;
u32 d_wb_alloc_units;
Thanks,
Stanley Chu
>
> bool b_rpm_dev_flush_capable;
> u8 b_presrv_uspc_en;
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 528c257df48c..0998e6103cd7 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -7243,10 +7243,8 @@ static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf)
> desc_buf[DEVICE_DESC_PARAM_WB_PRESRV_USRSPC_EN];
>
> if (dev_info->b_wb_buffer_type == WB_BUF_MODE_SHARED) {
> - dev_info->d_wb_alloc_units =
> - get_unaligned_be32(desc_buf +
> - DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS);
> - if (!dev_info->d_wb_alloc_units)
> + if (!get_unaligned_be32(desc_buf +
> + DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS))
> goto wb_disabled;
> } else {
> for (lun = 0; lun < UFS_UPIU_MAX_WB_LUN_ID; lun++) {
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v4 4/6] scsi: ufs: Remove d_wb_alloc_units from struct ufs_dev_info
2020-12-15 8:57 ` Stanley Chu
@ 2020-12-15 9:59 ` Bean Huo
0 siblings, 0 replies; 16+ messages in thread
From: Bean Huo @ 2020-12-15 9:59 UTC (permalink / raw)
To: Stanley Chu
Cc: alim.akhtar, avri.altman, asutoshd, jejb, martin.petersen,
beanhuo, bvanassche, tomas.winkler, cang, linux-scsi,
linux-kernel
On Tue, 2020-12-15 at 16:57 +0800, Stanley Chu wrote:
> > u8 b_wb_buffer_type;
> > - u32 d_wb_alloc_units;
>
> Perhaps below two fields could be also removed from struct
> ufs_dev_info
> for the same reason?
>
> u32 d_ext_ufs_feature_sup;
I thought twice before this patch. maybe will be used in near future,
so I keep d_ext_ufs_feature_sup. Now that you suggest, we can remove it
as well.
> u32 d_wb_alloc_units;
This patch is to remove it.
Thanks,
Bean
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 5/6] scsi: ufs: Cleanup WB buffer flush toggle implementation
2020-12-11 14:00 [PATCH v4 0/6] Several changes for UFS WriteBooster Bean Huo
` (3 preceding siblings ...)
2020-12-11 14:00 ` [PATCH v4 4/6] scsi: ufs: Remove d_wb_alloc_units from " Bean Huo
@ 2020-12-11 14:00 ` Bean Huo
2020-12-15 9:07 ` Stanley Chu
2020-12-11 14:00 ` [PATCH v4 6/6] scsi: ufs: Keep device active mode only fWriteBoosterBufferFlushDuringHibernate == 1 Bean Huo
5 siblings, 1 reply; 16+ messages in thread
From: Bean Huo @ 2020-12-11 14:00 UTC (permalink / raw)
To: alim.akhtar, avri.altman, asutoshd, jejb, martin.petersen,
stanley.chu, beanhuo, bvanassche, tomas.winkler, cang
Cc: linux-scsi, linux-kernel
From: Bean Huo <beanhuo@micron.com>
Delete ufshcd_wb_buf_flush_enable() and ufshcd_wb_buf_flush_disable(),
move the implementation into ufshcd_wb_toggle_flush().
Signed-off-by: Bean Huo <beanhuo@micron.com>
---
drivers/scsi/ufs/ufshcd.c | 69 ++++++++++++++-------------------------
1 file changed, 24 insertions(+), 45 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 0998e6103cd7..fb3c98724005 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -244,10 +244,8 @@ static int ufshcd_setup_vreg(struct ufs_hba *hba, bool on);
static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba,
struct ufs_vreg *vreg);
static int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag);
-static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba);
-static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba);
static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set);
-static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable);
+static inline int ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable);
static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba);
static void ufshcd_hba_vreg_set_hpm(struct ufs_hba *hba);
@@ -5398,60 +5396,41 @@ static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set)
index, NULL);
}
-static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable)
-{
- if (hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)
- return;
-
- if (enable)
- ufshcd_wb_buf_flush_enable(hba);
- else
- ufshcd_wb_buf_flush_disable(hba);
-
-}
-
-static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba)
+static inline int ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable)
{
int ret;
u8 index;
+ enum query_opcode opcode;
- if (!ufshcd_is_wb_allowed(hba) || hba->dev_info.wb_buf_flush_enabled)
+ if (hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)
return 0;
- index = ufshcd_wb_get_query_index(hba);
- ret = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_SET_FLAG,
- QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN,
- index, NULL);
- if (ret)
- dev_err(hba->dev, "%s WB - buf flush enable failed %d\n",
- __func__, ret);
- else
- hba->dev_info.wb_buf_flush_enabled = true;
-
- dev_dbg(hba->dev, "WB - Flush enabled: %d\n", ret);
- return ret;
-}
-
-static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba)
-{
- int ret;
- u8 index;
-
- if (!ufshcd_is_wb_allowed(hba) || !hba->dev_info.wb_buf_flush_enabled)
+ if (!ufshcd_is_wb_allowed(hba) ||
+ hba->dev_info.wb_buf_flush_enabled == enable)
return 0;
+ if (enable)
+ opcode = UPIU_QUERY_OPCODE_SET_FLAG;
+ else
+ opcode = UPIU_QUERY_OPCODE_CLEAR_FLAG;
+
index = ufshcd_wb_get_query_index(hba);
- ret = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_CLEAR_FLAG,
- QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN,
- index, NULL);
+ ret = ufshcd_query_flag_retry(hba, opcode,
+ QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN, index,
+ NULL);
if (ret) {
- dev_warn(hba->dev, "%s: WB - buf flush disable failed %d\n",
- __func__, ret);
- } else {
- hba->dev_info.wb_buf_flush_enabled = false;
- dev_dbg(hba->dev, "WB - Flush disabled: %d\n", ret);
+ dev_err(hba->dev, "%s WB-Buf Flush %s failed %d\n", __func__,
+ enable ? "enable" : "disable", ret);
+ goto out;
}
+ if (enable)
+ hba->dev_info.wb_buf_flush_enabled = true;
+ else
+ hba->dev_info.wb_buf_flush_enabled = false;
+
+ dev_dbg(hba->dev, "WB-Buf Flush %s\n", enable ? "enabled" : "disabled");
+out:
return ret;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v4 5/6] scsi: ufs: Cleanup WB buffer flush toggle implementation
2020-12-11 14:00 ` [PATCH v4 5/6] scsi: ufs: Cleanup WB buffer flush toggle implementation Bean Huo
@ 2020-12-15 9:07 ` Stanley Chu
2020-12-15 9:28 ` Bean Huo
0 siblings, 1 reply; 16+ messages in thread
From: Stanley Chu @ 2020-12-15 9:07 UTC (permalink / raw)
To: Bean Huo
Cc: alim.akhtar, avri.altman, asutoshd, jejb, martin.petersen,
beanhuo, bvanassche, tomas.winkler, cang, linux-scsi,
linux-kernel
Hi Bean,
On Fri, 2020-12-11 at 15:00 +0100, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
>
> Delete ufshcd_wb_buf_flush_enable() and ufshcd_wb_buf_flush_disable(),
> move the implementation into ufshcd_wb_toggle_flush().
>
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
> drivers/scsi/ufs/ufshcd.c | 69 ++++++++++++++-------------------------
> 1 file changed, 24 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 0998e6103cd7..fb3c98724005 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -244,10 +244,8 @@ static int ufshcd_setup_vreg(struct ufs_hba *hba, bool on);
> static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba,
> struct ufs_vreg *vreg);
> static int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag);
> -static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba);
> -static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba);
> static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set);
> -static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable);
> +static inline int ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable);
> static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba);
> static void ufshcd_hba_vreg_set_hpm(struct ufs_hba *hba);
>
> @@ -5398,60 +5396,41 @@ static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set)
> index, NULL);
> }
>
> -static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable)
> -{
> - if (hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)
> - return;
> -
> - if (enable)
> - ufshcd_wb_buf_flush_enable(hba);
> - else
> - ufshcd_wb_buf_flush_disable(hba);
> -
> -}
> -
> -static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba)
> +static inline int ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable)
> {
> int ret;
> u8 index;
> + enum query_opcode opcode;
>
> - if (!ufshcd_is_wb_allowed(hba) || hba->dev_info.wb_buf_flush_enabled)
> + if (hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)
> return 0;
>
> - index = ufshcd_wb_get_query_index(hba);
> - ret = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_SET_FLAG,
> - QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN,
> - index, NULL);
> - if (ret)
> - dev_err(hba->dev, "%s WB - buf flush enable failed %d\n",
> - __func__, ret);
> - else
> - hba->dev_info.wb_buf_flush_enabled = true;
> -
> - dev_dbg(hba->dev, "WB - Flush enabled: %d\n", ret);
> - return ret;
> -}
> -
> -static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba)
> -{
> - int ret;
> - u8 index;
> -
> - if (!ufshcd_is_wb_allowed(hba) || !hba->dev_info.wb_buf_flush_enabled)
> + if (!ufshcd_is_wb_allowed(hba) ||
> + hba->dev_info.wb_buf_flush_enabled == enable)
> return 0;
>
> + if (enable)
> + opcode = UPIU_QUERY_OPCODE_SET_FLAG;
> + else
> + opcode = UPIU_QUERY_OPCODE_CLEAR_FLAG;
> +
> index = ufshcd_wb_get_query_index(hba);
> - ret = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_CLEAR_FLAG,
> - QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN,
> - index, NULL);
> + ret = ufshcd_query_flag_retry(hba, opcode,
> + QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN, index,
> + NULL);
> if (ret) {
> - dev_warn(hba->dev, "%s: WB - buf flush disable failed %d\n",
> - __func__, ret);
> - } else {
> - hba->dev_info.wb_buf_flush_enabled = false;
> - dev_dbg(hba->dev, "WB - Flush disabled: %d\n", ret);
> + dev_err(hba->dev, "%s WB-Buf Flush %s failed %d\n", __func__,
> + enable ? "enable" : "disable", ret);
> + goto out;
> }
>
> + if (enable)
> + hba->dev_info.wb_buf_flush_enabled = true;
> + else
> + hba->dev_info.wb_buf_flush_enabled = false;
Perhaps this could be simpler as below?
hba->dev_info.wb_buf_flush_enabled = enable;
Thanks,
Stanley Chu
> +
> + dev_dbg(hba->dev, "WB-Buf Flush %s\n", enable ? "enabled" : "disabled");
> +out:
> return ret;
> }
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v4 5/6] scsi: ufs: Cleanup WB buffer flush toggle implementation
2020-12-15 9:07 ` Stanley Chu
@ 2020-12-15 9:28 ` Bean Huo
0 siblings, 0 replies; 16+ messages in thread
From: Bean Huo @ 2020-12-15 9:28 UTC (permalink / raw)
To: Stanley Chu
Cc: alim.akhtar, avri.altman, asutoshd, jejb, martin.petersen,
beanhuo, bvanassche, tomas.winkler, cang, linux-scsi,
linux-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 6/6] scsi: ufs: Keep device active mode only fWriteBoosterBufferFlushDuringHibernate == 1
2020-12-11 14:00 [PATCH v4 0/6] Several changes for UFS WriteBooster Bean Huo
` (4 preceding siblings ...)
2020-12-11 14:00 ` [PATCH v4 5/6] scsi: ufs: Cleanup WB buffer flush toggle implementation Bean Huo
@ 2020-12-11 14:00 ` Bean Huo
5 siblings, 0 replies; 16+ messages in thread
From: Bean Huo @ 2020-12-11 14:00 UTC (permalink / raw)
To: alim.akhtar, avri.altman, asutoshd, jejb, martin.petersen,
stanley.chu, beanhuo, bvanassche, tomas.winkler, cang
Cc: linux-scsi, linux-kernel
From: Bean Huo <beanhuo@micron.com>
According to the JEDEC UFS 3.1 Spec, If fWriteBoosterBufferFlushDuringHibernate
is set to one, the device flushes the WriteBooster Buffer data automatically
whenever the link enters the hibernate (HIBERN8) state. While the flushing
operation is in progress, the device should be kept in Active power mode.
Currently, we set this flag during the UFSHCD probe stage, but we didn't deal
with its programming failure. Even this failure is less likely to occur, but
still it is possible.
This patch is to add checkup of fWriteBoosterBufferFlushDuringHibernate setting,
keep the device as "active power mode" only when this flag be successfully set
to 1.
Fixes: 51dd905bd2f6 ("scsi: ufs: Fix WriteBooster flush during runtime suspend")
Signed-off-by: Bean Huo <beanhuo@micron.com>
---
drivers/scsi/ufs/ufs.h | 1 +
drivers/scsi/ufs/ufshcd.c | 21 ++++++++++++++++-----
2 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 8ed342e43883..1b3866f608d9 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -542,6 +542,7 @@ struct ufs_dev_info {
/* UFS WB related flags */
bool wb_enabled;
bool wb_buf_flush_enabled;
+ bool wb_buf_flush_in_hibern8;
u8 wb_dedicated_lu;
u8 b_wb_buffer_type;
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index fb3c98724005..7ff486f047d8 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -282,10 +282,16 @@ static inline void ufshcd_wb_config(struct ufs_hba *hba)
dev_err(hba->dev, "%s: Enable WB failed: %d\n", __func__, ret);
else
dev_info(hba->dev, "%s: Write Booster Configured\n", __func__);
+
ret = ufshcd_wb_toggle_flush_during_h8(hba, true);
- if (ret)
+ if (ret) {
dev_err(hba->dev, "%s: En WB flush during H8: failed: %d\n",
__func__, ret);
+ hba->dev_info.wb_buf_flush_in_hibern8 = false;
+ } else {
+ hba->dev_info.wb_buf_flush_in_hibern8 = true;
+ }
+
ufshcd_wb_toggle_flush(hba, true);
}
@@ -589,6 +595,7 @@ static void ufshcd_device_reset(struct ufs_hba *hba)
if (ufshcd_is_wb_allowed(hba)) {
hba->dev_info.wb_enabled = false;
hba->dev_info.wb_buf_flush_enabled = false;
+ hba->dev_info.wb_buf_flush_in_hibern8 = false;
}
}
if (err != -EOPNOTSUPP)
@@ -5471,6 +5478,7 @@ static bool ufshcd_wb_need_flush(struct ufs_hba *hba)
if (!ufshcd_is_wb_allowed(hba))
return false;
+
/*
* The ufs device needs the vcc to be ON to flush.
* With user-space reduction enabled, it's enough to enable flush
@@ -8571,6 +8579,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
enum ufs_pm_level pm_lvl;
enum ufs_dev_pwr_mode req_dev_pwr_mode;
enum uic_link_state req_link_state;
+ bool hibern8;
hba->pm_op_in_progress = 1;
if (!ufshcd_is_shutdown_pm(pm_op)) {
@@ -8630,11 +8639,13 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
* Hibern8, keep device power mode as "active power mode"
* and VCC supply.
*/
+ hibern8 = req_link_state == UIC_LINK_HIBERN8_STATE ||
+ (req_link_state == UIC_LINK_ACTIVE_STATE &&
+ ufshcd_is_auto_hibern8_enabled(hba));
+
hba->dev_info.b_rpm_dev_flush_capable =
- hba->auto_bkops_enabled ||
- (((req_link_state == UIC_LINK_HIBERN8_STATE) ||
- ((req_link_state == UIC_LINK_ACTIVE_STATE) &&
- ufshcd_is_auto_hibern8_enabled(hba))) &&
+ hba->auto_bkops_enabled || (hibern8 &&
+ hba->dev_info.wb_buf_flush_in_hibern8 &&
ufshcd_wb_need_flush(hba));
}
--
2.17.1
^ permalink raw reply related [flat|nested] 16+ messages in thread