Hi,
>
> Add vendor specific functions for WB
> Use callback additional setting when use write booster.
>
> Signed-off-by: SEO HOYOUNG <hy50.seo@xxxxxxxxxxx>
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
