| Message ID | 20200713112022.169887-1-hy50.seo@samsung.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [RFC,v1] scsi: ufs: modify write booster | expand |
Hi, > > Add vendor specific functions for WB > Use callback additional setting when use write booster. > > Signed-off-by: SEO HOYOUNG <hy50.seo@samsung.com> If you are introducing a new vops - your series should include your implementation, Otherwise why introduce a vop that nobody uses? > --- > drivers/scsi/ufs/ufshcd.c | 23 ++++++++++++++++----- > drivers/scsi/ufs/ufshcd.h | 43 +++++++++++++++++++++++++++++++++++++++ > 2 files changed, 61 insertions(+), 5 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index efc0a6cbfe22..efa16bf4fd76 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -3306,11 +3306,11 @@ int ufshcd_read_string_desc(struct ufs_hba *hba, > u8 desc_index, > * > * Return 0 in case of success, non-zero otherwise > */ > -static inline int ufshcd_read_unit_desc_param(struct ufs_hba *hba, > - int lun, > - enum unit_desc_param param_offset, > - u8 *param_read_buf, > - u32 param_size) > +int ufshcd_read_unit_desc_param(struct ufs_hba *hba, > + int lun, > + enum unit_desc_param param_offset, > + u8 *param_read_buf, > + u32 param_size) > { > /* > * Unit descriptors are only available for general purpose LUs (LUN id > @@ -3322,6 +3322,7 @@ static inline int > ufshcd_read_unit_desc_param(struct ufs_hba *hba, > return ufshcd_read_desc_param(hba, QUERY_DESC_IDN_UNIT, lun, > param_offset, param_read_buf, param_size); > } > +EXPORT_SYMBOL_GPL(ufshcd_read_unit_desc_param); Are you exporting this because you need ufsfeatures to use it? If so, you need to wait until it is merged, if not, add an explanation in your commit log. > > static int ufshcd_get_ref_clk_gating_wait(struct ufs_hba *hba) > { > @@ -5257,6 +5258,10 @@ static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool > enable) > > if (!(enable ^ hba->wb_enabled)) > return 0; > + > + if (!ufshcd_wb_ctrl_vendor(hba, enable)) > + return 0; If the vop fail just keep going with the standard implementation? > + > if (enable) > opcode = UPIU_QUERY_OPCODE_SET_FLAG; > else > @@ -6610,6 +6615,8 @@ static int ufshcd_reset_and_restore(struct ufs_hba > *hba) > int err = 0; > int retries = MAX_HOST_RESET_RETRIES; > > + ufshcd_reset_vendor(hba); What reset has to do with WB? If you are changing the flow, need to do that in a different patch, with a proper commit log. > + > do { > /* Reset the attached device */ > ufshcd_vops_device_reset(hba); > @@ -6903,6 +6910,9 @@ static void ufshcd_wb_probe(struct ufs_hba *hba, > u8 *desc_buf) > if (!(dev_info->d_ext_ufs_feature_sup & > UFS_DEV_WRITE_BOOSTER_SUP)) > goto wb_disabled; > > + if (!ufshcd_wb_alloc_units_vendor(hba)) > + return; > + > /* > * WB may be supported but not configured while provisioning. > * The spec says, in dedicated wb buffer mode, > @@ -8260,6 +8270,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, > enum ufs_pm_op pm_op) > /* make sure that auto bkops is disabled */ > ufshcd_disable_auto_bkops(hba); > } > + > /* > * If device needs to do BKOP or WB buffer flush during > * Hibern8, keep device power mode as "active power mode" > @@ -8273,6 +8284,8 @@ static int ufshcd_suspend(struct ufs_hba *hba, > enum ufs_pm_op pm_op) > ufshcd_wb_need_flush(hba)); > } > > + ufshcd_wb_toggle_flush_vendor(hba, pm_op); > + > if (req_dev_pwr_mode != hba->curr_dev_pwr_mode) { > if ((ufshcd_is_runtime_pm(pm_op) && !hba->auto_bkops_enabled) > || > !ufshcd_is_runtime_pm(pm_op)) { > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h > index 656c0691c858..deb9577e0eaa 100644 > --- a/drivers/scsi/ufs/ufshcd.h > +++ b/drivers/scsi/ufs/ufshcd.h > @@ -254,6 +254,13 @@ struct ufs_pwr_mode_info { > struct ufs_pa_layer_attr info; > }; > > +struct ufs_wb_ops { > + int (*wb_toggle_flush_vendor)(struct ufs_hba *hba, enum ufs_pm_op > pm_op); > + int (*wb_alloc_units_vendor)(struct ufs_hba *hba); > + int (*wb_ctrl_vendor)(struct ufs_hba *hba, bool enable); > + int (*wb_reset_vendor)(struct ufs_hba *hba, bool force); > +}; > + > /** > * struct ufs_hba_variant_ops - variant specific callbacks > * @name: variant name > @@ -752,6 +759,7 @@ struct ufs_hba { > struct request_queue *bsg_queue; > bool wb_buf_flush_enabled; > bool wb_enabled; > + struct ufs_wb_ops *wb_ops; This actually should not be directly under ufs_hba, but a member of hba->vops, and also please follow the vop naming convention, e.g. ufshcd_vops_wd_xxx > struct delayed_work rpm_dev_flush_recheck_work; > > #ifdef CONFIG_SCSI_UFS_CRYPTO > @@ -1004,6 +1012,10 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba > *hba, > u8 *desc_buff, int *buff_len, > enum query_opcode desc_op); > > +int ufshcd_read_unit_desc_param(struct ufs_hba *hba, > + int lun, enum unit_desc_param param_offset, > + u8 *param_read_buf, u32 param_size); > + > /* Wrapper functions for safely calling variant operations */ > static inline const char *ufshcd_get_var_name(struct ufs_hba *hba) > { > @@ -1181,4 +1193,35 @@ static inline u8 ufshcd_scsi_to_upiu_lun(unsigned > int scsi_lun) > int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len, > const char *prefix); > > +static inline int ufshcd_wb_toggle_flush_vendor(struct ufs_hba *hba, enum > ufs_pm_op pm_op) > +{ > + if (!hba->wb_ops || !hba->wb_ops->wb_toggle_flush_vendor) > + return -1; > + > + return hba->wb_ops->wb_toggle_flush_vendor(hba, pm_op); > +} > + > +static int ufshcd_wb_alloc_units_vendor(struct ufs_hba *hba) > +{ > + if (!hba->wb_ops || !hba->wb_ops->wb_alloc_units_vendor) > + return -1; > + > + return hba->wb_ops->wb_alloc_units_vendor(hba); > +} > + > +static int ufshcd_wb_ctrl_vendor(struct ufs_hba *hba, bool enable) > +{ > + if (!hba->wb_ops || !hba->wb_ops->wb_ctrl_vendor) > + return -1; > + > + return hba->wb_ops->wb_ctrl_vendor(hba, enable); > +} > + > +static int ufshcd_reset_vendor(struct ufs_hba *hba) > +{ > + if (!hba->wb_ops || !hba->wb_ops->wb_reset_vendor) > + return -1; > + > + return hba->wb_ops->wb_reset_vendor(hba, false); > +} > #endif /* End of Header */ > -- > 2.26.0
> Hi, > > > > > Add vendor specific functions for WB > > Use callback additional setting when use write booster. > > > > Signed-off-by: SEO HOYOUNG <hy50.seo@samsung.com> > If you are introducing a new vops - your series should include your > implementation, Otherwise why introduce a vop that nobody uses? Ok, I already upload with vops functions > > > > --- > > drivers/scsi/ufs/ufshcd.c | 23 ++++++++++++++++----- > > drivers/scsi/ufs/ufshcd.h | 43 +++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 61 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > > index efc0a6cbfe22..efa16bf4fd76 100644 > > --- a/drivers/scsi/ufs/ufshcd.c > > +++ b/drivers/scsi/ufs/ufshcd.c > > @@ -3306,11 +3306,11 @@ int ufshcd_read_string_desc(struct ufs_hba > > *hba, > > u8 desc_index, > > * > > * Return 0 in case of success, non-zero otherwise > > */ > > -static inline int ufshcd_read_unit_desc_param(struct ufs_hba *hba, > > - int lun, > > - enum unit_desc_param param_offset, > > - u8 *param_read_buf, > > - u32 param_size) > > +int ufshcd_read_unit_desc_param(struct ufs_hba *hba, > > + int lun, > > + enum unit_desc_param param_offset, > > + u8 *param_read_buf, > > + u32 param_size) > > { > > /* > > * Unit descriptors are only available for general purpose LUs > > (LUN id @@ -3322,6 +3322,7 @@ static inline int > > ufshcd_read_unit_desc_param(struct ufs_hba *hba, > > return ufshcd_read_desc_param(hba, QUERY_DESC_IDN_UNIT, lun, > > param_offset, param_read_buf, > > param_size); } > > +EXPORT_SYMBOL_GPL(ufshcd_read_unit_desc_param); > Are you exporting this because you need ufsfeatures to use it? > If so, you need to wait until it is merged, if not, add an explanation in > your commit log. Yes, I need ufsfeatures for use. What patch will merged? Is there patch to export ufsfeatures? > > > > > static int ufshcd_get_ref_clk_gating_wait(struct ufs_hba *hba) { @@ > > -5257,6 +5258,10 @@ static int ufshcd_wb_ctrl(struct ufs_hba *hba, > > bool > > enable) > > > > if (!(enable ^ hba->wb_enabled)) > > return 0; > > + > > + if (!ufshcd_wb_ctrl_vendor(hba, enable)) > > + return 0; > If the vop fail just keep going with the standard implementation? > > > + > > if (enable) > > opcode = UPIU_QUERY_OPCODE_SET_FLAG; > > else > > @@ -6610,6 +6615,8 @@ static int ufshcd_reset_and_restore(struct > > ufs_hba > > *hba) > > int err = 0; > > int retries = MAX_HOST_RESET_RETRIES; > > > > + ufshcd_reset_vendor(hba); > What reset has to do with WB? > If you are changing the flow, need to do that in a different patch, with a > proper commit log. For disable WB feature. > > > + > > do { > > /* Reset the attached device */ > > ufshcd_vops_device_reset(hba); @@ -6903,6 +6910,9 @@ > > static void ufshcd_wb_probe(struct ufs_hba *hba, > > u8 *desc_buf) > > if (!(dev_info->d_ext_ufs_feature_sup & > > UFS_DEV_WRITE_BOOSTER_SUP)) > > goto wb_disabled; > > > > + if (!ufshcd_wb_alloc_units_vendor(hba)) > > + return; > > + > > /* > > * WB may be supported but not configured while provisioning. > > * The spec says, in dedicated wb buffer mode, @@ -8260,6 > > +8270,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum > > ufs_pm_op pm_op) > > /* make sure that auto bkops is disabled */ > > ufshcd_disable_auto_bkops(hba); > > } > > + > > /* > > * If device needs to do BKOP or WB buffer flush during > > * Hibern8, keep device power mode as "active power mode" > > @@ -8273,6 +8284,8 @@ static int ufshcd_suspend(struct ufs_hba *hba, > > enum ufs_pm_op pm_op) > > ufshcd_wb_need_flush(hba)); > > } > > > > + ufshcd_wb_toggle_flush_vendor(hba, pm_op); > > + > > if (req_dev_pwr_mode != hba->curr_dev_pwr_mode) { > > if ((ufshcd_is_runtime_pm(pm_op) && > > !hba->auto_bkops_enabled) > > || > > !ufshcd_is_runtime_pm(pm_op)) { diff --git > > a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index > > 656c0691c858..deb9577e0eaa 100644 > > --- a/drivers/scsi/ufs/ufshcd.h > > +++ b/drivers/scsi/ufs/ufshcd.h > > @@ -254,6 +254,13 @@ struct ufs_pwr_mode_info { > > struct ufs_pa_layer_attr info; }; > > > > +struct ufs_wb_ops { > > + int (*wb_toggle_flush_vendor)(struct ufs_hba *hba, enum > > +ufs_pm_op > > pm_op); > > + int (*wb_alloc_units_vendor)(struct ufs_hba *hba); > > + int (*wb_ctrl_vendor)(struct ufs_hba *hba, bool enable); > > + int (*wb_reset_vendor)(struct ufs_hba *hba, bool force); }; > > + > > /** > > * struct ufs_hba_variant_ops - variant specific callbacks > > * @name: variant name > > @@ -752,6 +759,7 @@ struct ufs_hba { > > struct request_queue *bsg_queue; > > bool wb_buf_flush_enabled; > > bool wb_enabled; > > + struct ufs_wb_ops *wb_ops; > This actually should not be directly under ufs_hba, but a member of hba- > >vops, and also please follow the vop naming convention, e.g. > ufshcd_vops_wd_xxx It is a variable with a different valid. So I define in hba structure. > > > > struct delayed_work rpm_dev_flush_recheck_work; > > > > #ifdef CONFIG_SCSI_UFS_CRYPTO > > @@ -1004,6 +1012,10 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba > > *hba, > > u8 *desc_buff, int *buff_len, > > enum query_opcode desc_op); > > > > +int ufshcd_read_unit_desc_param(struct ufs_hba *hba, > > + int lun, enum unit_desc_param param_offset, > > + u8 *param_read_buf, u32 param_size); > > + > > /* Wrapper functions for safely calling variant operations */ static > > inline const char *ufshcd_get_var_name(struct ufs_hba *hba) { @@ > > -1181,4 +1193,35 @@ static inline u8 ufshcd_scsi_to_upiu_lun(unsigned > > int scsi_lun) int ufshcd_dump_regs(struct ufs_hba *hba, size_t > > offset, size_t len, > > const char *prefix); > > > > +static inline int ufshcd_wb_toggle_flush_vendor(struct ufs_hba *hba, > > +enum > > ufs_pm_op pm_op) > > +{ > > + if (!hba->wb_ops || !hba->wb_ops->wb_toggle_flush_vendor) > > + return -1; > > + > > + return hba->wb_ops->wb_toggle_flush_vendor(hba, pm_op); } > > + > > +static int ufshcd_wb_alloc_units_vendor(struct ufs_hba *hba) { > > + if (!hba->wb_ops || !hba->wb_ops->wb_alloc_units_vendor) > > + return -1; > > + > > + return hba->wb_ops->wb_alloc_units_vendor(hba); > > +} > > + > > +static int ufshcd_wb_ctrl_vendor(struct ufs_hba *hba, bool enable) { > > + if (!hba->wb_ops || !hba->wb_ops->wb_ctrl_vendor) > > + return -1; > > + > > + return hba->wb_ops->wb_ctrl_vendor(hba, enable); } > > + > > +static int ufshcd_reset_vendor(struct ufs_hba *hba) { > > + if (!hba->wb_ops || !hba->wb_ops->wb_reset_vendor) > > + return -1; > > + > > + return hba->wb_ops->wb_reset_vendor(hba, false); } > > #endif /* End of Header */ > > -- > > 2.26.0
> > > -static inline int ufshcd_read_unit_desc_param(struct ufs_hba *hba, > > > - int lun, > > > - enum unit_desc_param param_offset, > > > - u8 *param_read_buf, > > > - u32 param_size) > > > +int ufshcd_read_unit_desc_param(struct ufs_hba *hba, > > > + int lun, > > > + enum unit_desc_param param_offset, > > > + u8 *param_read_buf, > > > + u32 param_size) > > > { > > > /* > > > * Unit descriptors are only available for general purpose LUs > > > (LUN id @@ -3322,6 +3322,7 @@ static inline int > > > ufshcd_read_unit_desc_param(struct ufs_hba *hba, > > > return ufshcd_read_desc_param(hba, QUERY_DESC_IDN_UNIT, lun, > > > param_offset, param_read_buf, > > > param_size); } > > > +EXPORT_SYMBOL_GPL(ufshcd_read_unit_desc_param); > > Are you exporting this because you need ufsfeatures to use it? > > If so, you need to wait until it is merged, if not, add an explanation in > > your commit log. > Yes, I need ufsfeatures for use. > What patch will merged? Is there patch to export ufsfeatures? https://www.spinics.net/lists/linux-scsi/msg144408.html and they exported ufshcd_query_descriptor_retry in the patch afterwards. Thanks, Avri
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index efc0a6cbfe22..efa16bf4fd76 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -3306,11 +3306,11 @@ int ufshcd_read_string_desc(struct ufs_hba *hba, u8 desc_index, * * Return 0 in case of success, non-zero otherwise */ -static inline int ufshcd_read_unit_desc_param(struct ufs_hba *hba, - int lun, - enum unit_desc_param param_offset, - u8 *param_read_buf, - u32 param_size) +int ufshcd_read_unit_desc_param(struct ufs_hba *hba, + int lun, + enum unit_desc_param param_offset, + u8 *param_read_buf, + u32 param_size) { /* * Unit descriptors are only available for general purpose LUs (LUN id @@ -3322,6 +3322,7 @@ static inline int ufshcd_read_unit_desc_param(struct ufs_hba *hba, return ufshcd_read_desc_param(hba, QUERY_DESC_IDN_UNIT, lun, param_offset, param_read_buf, param_size); } +EXPORT_SYMBOL_GPL(ufshcd_read_unit_desc_param); static int ufshcd_get_ref_clk_gating_wait(struct ufs_hba *hba) { @@ -5257,6 +5258,10 @@ static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable) if (!(enable ^ hba->wb_enabled)) return 0; + + if (!ufshcd_wb_ctrl_vendor(hba, enable)) + return 0; + if (enable) opcode = UPIU_QUERY_OPCODE_SET_FLAG; else @@ -6610,6 +6615,8 @@ static int ufshcd_reset_and_restore(struct ufs_hba *hba) int err = 0; int retries = MAX_HOST_RESET_RETRIES; + ufshcd_reset_vendor(hba); + do { /* Reset the attached device */ ufshcd_vops_device_reset(hba); @@ -6903,6 +6910,9 @@ static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf) if (!(dev_info->d_ext_ufs_feature_sup & UFS_DEV_WRITE_BOOSTER_SUP)) goto wb_disabled; + if (!ufshcd_wb_alloc_units_vendor(hba)) + return; + /* * WB may be supported but not configured while provisioning. * The spec says, in dedicated wb buffer mode, @@ -8260,6 +8270,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) /* make sure that auto bkops is disabled */ ufshcd_disable_auto_bkops(hba); } + /* * If device needs to do BKOP or WB buffer flush during * Hibern8, keep device power mode as "active power mode" @@ -8273,6 +8284,8 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) ufshcd_wb_need_flush(hba)); } + ufshcd_wb_toggle_flush_vendor(hba, pm_op); + if (req_dev_pwr_mode != hba->curr_dev_pwr_mode) { if ((ufshcd_is_runtime_pm(pm_op) && !hba->auto_bkops_enabled) || !ufshcd_is_runtime_pm(pm_op)) { diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index 656c0691c858..deb9577e0eaa 100644 --- a/drivers/scsi/ufs/ufshcd.h +++ b/drivers/scsi/ufs/ufshcd.h @@ -254,6 +254,13 @@ struct ufs_pwr_mode_info { struct ufs_pa_layer_attr info; }; +struct ufs_wb_ops { + int (*wb_toggle_flush_vendor)(struct ufs_hba *hba, enum ufs_pm_op pm_op); + int (*wb_alloc_units_vendor)(struct ufs_hba *hba); + int (*wb_ctrl_vendor)(struct ufs_hba *hba, bool enable); + int (*wb_reset_vendor)(struct ufs_hba *hba, bool force); +}; + /** * struct ufs_hba_variant_ops - variant specific callbacks * @name: variant name @@ -752,6 +759,7 @@ struct ufs_hba { struct request_queue *bsg_queue; bool wb_buf_flush_enabled; bool wb_enabled; + struct ufs_wb_ops *wb_ops; struct delayed_work rpm_dev_flush_recheck_work; #ifdef CONFIG_SCSI_UFS_CRYPTO @@ -1004,6 +1012,10 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba, u8 *desc_buff, int *buff_len, enum query_opcode desc_op); +int ufshcd_read_unit_desc_param(struct ufs_hba *hba, + int lun, enum unit_desc_param param_offset, + u8 *param_read_buf, u32 param_size); + /* Wrapper functions for safely calling variant operations */ static inline const char *ufshcd_get_var_name(struct ufs_hba *hba) { @@ -1181,4 +1193,35 @@ static inline u8 ufshcd_scsi_to_upiu_lun(unsigned int scsi_lun) int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len, const char *prefix); +static inline int ufshcd_wb_toggle_flush_vendor(struct ufs_hba *hba, enum ufs_pm_op pm_op) +{ + if (!hba->wb_ops || !hba->wb_ops->wb_toggle_flush_vendor) + return -1; + + return hba->wb_ops->wb_toggle_flush_vendor(hba, pm_op); +} + +static int ufshcd_wb_alloc_units_vendor(struct ufs_hba *hba) +{ + if (!hba->wb_ops || !hba->wb_ops->wb_alloc_units_vendor) + return -1; + + return hba->wb_ops->wb_alloc_units_vendor(hba); +} + +static int ufshcd_wb_ctrl_vendor(struct ufs_hba *hba, bool enable) +{ + if (!hba->wb_ops || !hba->wb_ops->wb_ctrl_vendor) + return -1; + + return hba->wb_ops->wb_ctrl_vendor(hba, enable); +} + +static int ufshcd_reset_vendor(struct ufs_hba *hba) +{ + if (!hba->wb_ops || !hba->wb_ops->wb_reset_vendor) + return -1; + + return hba->wb_ops->wb_reset_vendor(hba, false); +} #endif /* End of Header */
Add vendor specific functions for WB Use callback additional setting when use write booster. Signed-off-by: SEO HOYOUNG <hy50.seo@samsung.com> --- drivers/scsi/ufs/ufshcd.c | 23 ++++++++++++++++----- drivers/scsi/ufs/ufshcd.h | 43 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 5 deletions(-)