RE: [RFC PATCH v2 1/3] scsi: ufs: modify write booster

[서호영 <hy50.seo@samsung.com>]

> -----Original Message-----
> From: asutoshd=codeaurora.org@mg.codeaurora.org
> [mailto:asutoshd=codeaurora.org@mg.codeaurora.org] On Behalf Of Asutosh
> Das (asd)
> Sent: Tuesday, July 21, 2020 1:47 AM
> To: SEO HOYOUNG; linux-scsi@vger.kernel.org; alim.akhtar@samsung.com;
> avri.altman@wdc.com; jejb@linux.ibm.com; martin.petersen@oracle.com;
> beanhuo@micron.com; cang@codeaurora.org; bvanassche@acm.org;
> grant.jung@samsung.com
> Subject: Re: [RFC PATCH v2 1/3] scsi: ufs: modify write booster
> 
> On 7/20/2020 3:40 AM, SEO HOYOUNG wrote:
> > 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(-)
> >
> > 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;
> I didn't understand this check. Have you considered this -
> ufshcd_is_wb_allowed(...).
Ok. I will modify to using this function

> > +
> >   	/*
> >   	 * 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);
> >   		}
> > +
> Unnecessary new line, perhaps?
I will remove new line.
> >   		/*
> >   		 * 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 */
> >
> 
> 
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> Linux Foundation Collaborative Project