Re: [PATCH v3 1/5] scsi: ufs: enable WriteBooster on some pre-3.1 UFS devices

[Stanley Chu]

Hi Can,

On Sat, 2020-05-02 at 15:47 +0800, Can Guo wrote:
> Hi Stanley,
> On 2020-05-01 22:38, Stanley Chu wrote:
> > WriteBooster feature can be supported by some pre-3.1 UFS devices
> > by upgrading firmware.
> > 
> > To enable WriteBooster feature in such devices, introduce a device
> > quirk to relax the entrance condition of ufshcd_wb_probe() to allow
> > host driver to check those devices' WriteBooster capability.
> > 
> > WriteBooster feature can be available if below all conditions are
> > satisfied,
> > 
> > 1. Host enables WriteBooster capability
> > 2. UFS 3.1 device or UFS pre-3.1 device with quirk
> >    UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES enabled
> > 3. Device descriptor has dExtendedUFSFeaturesSupport field
> > 4. WriteBooster support is specified in above field
> > 
> > Signed-off-by: Stanley Chu <stanley.chu@xxxxxxxxxxxx>
> > ---
> >  drivers/scsi/ufs/ufs_quirks.h |  7 ++++
> >  drivers/scsi/ufs/ufshcd.c     | 66 ++++++++++++++++++++++-------------
> >  2 files changed, 48 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/scsi/ufs/ufs_quirks.h 
> > b/drivers/scsi/ufs/ufs_quirks.h
> > index df7a1e6805a3..e3175a63c676 100644
> > --- a/drivers/scsi/ufs/ufs_quirks.h
> > +++ b/drivers/scsi/ufs/ufs_quirks.h
> > @@ -101,4 +101,11 @@ struct ufs_dev_fix {
> >   */
> >  #define UFS_DEVICE_QUIRK_HOST_VS_DEBUGSAVECONFIGTIME	(1 << 9)
> > 
> > +/*
> > + * Some pre-3.1 UFS devices can support extended features by upgrading
> > + * the firmware. Enable this quirk to make UFS core driver probe and 
> > enable
> > + * supported features on such devices.
> > + */
> > +#define UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES (1 << 10)
> > +
> >  #endif /* UFS_QUIRKS_H_ */
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 915e963398c4..c6668799d956 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -229,6 +229,8 @@ static struct ufs_dev_fix ufs_fixups[] = {
> >  		UFS_DEVICE_QUIRK_HOST_PA_SAVECONFIGTIME),
> >  	UFS_FIX(UFS_VENDOR_SKHYNIX, "hB8aL1" /*H28U62301AMR*/,
> >  		UFS_DEVICE_QUIRK_HOST_VS_DEBUGSAVECONFIGTIME),
> > +	UFS_FIX(UFS_VENDOR_SKHYNIX, "H9HQ21AFAMZDAR",
> > +		UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES),
> > 
> >  	END_FIX
> >  };
> > @@ -6800,9 +6802,19 @@ static int ufshcd_scsi_add_wlus(struct ufs_hba 
> > *hba)
> > 
> >  static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf)
> >  {
> > +	if (!ufshcd_is_wb_allowed(hba))
> > +		return;
> > +
> > +	if (hba->desc_size.dev_desc <= DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP)
> > +		goto wb_disabled;
> > +
> >  	hba->dev_info.d_ext_ufs_feature_sup =
> >  		get_unaligned_be32(desc_buf +
> >  				   DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP);
> > +
> > +	if (!(hba->dev_info.d_ext_ufs_feature_sup & 
> > UFS_DEV_WRITE_BOOSTER_SUP))
> > +		goto wb_disabled;
> > +
> >  	/*
> >  	 * WB may be supported but not configured while provisioning.
> >  	 * The spec says, in dedicated wb buffer mode,
> > @@ -6818,11 +6830,29 @@ static void ufshcd_wb_probe(struct ufs_hba
> > *hba, u8 *desc_buf)
> >  	hba->dev_info.b_presrv_uspc_en =
> >  		desc_buf[DEVICE_DESC_PARAM_WB_PRESRV_USRSPC_EN];
> > 
> > -	if (!((hba->dev_info.d_ext_ufs_feature_sup &
> > -		 UFS_DEV_WRITE_BOOSTER_SUP) &&
> > -		hba->dev_info.b_wb_buffer_type &&
> > +	if (!(hba->dev_info.b_wb_buffer_type &&
> >  	      hba->dev_info.d_wb_alloc_units))
> > -		hba->caps &= ~UFSHCD_CAP_WB_EN;
> > +		goto wb_disabled;
> > +
> > +	return;
> > +
> > +wb_disabled:
> > +	hba->caps &= ~UFSHCD_CAP_WB_EN;
> > +}
> > +
> > +static void ufs_fixup_device_setup(struct ufs_hba *hba)
> > +{
> > +	struct ufs_dev_fix *f;
> > +	struct ufs_dev_info *dev_info = &hba->dev_info;
> > +
> > +	for (f = ufs_fixups; f->quirk; f++) {
> > +		if ((f->wmanufacturerid == dev_info->wmanufacturerid ||
> > +		     f->wmanufacturerid == UFS_ANY_VENDOR) &&
> > +		     ((dev_info->model &&
> > +		       STR_PRFX_EQUAL(f->model, dev_info->model)) ||
> > +		      !strcmp(f->model, UFS_ANY_MODEL)))
> > +			hba->dev_quirks |= f->quirk;
> > +	}
> >  }
> > 
> >  static int ufs_get_device_desc(struct ufs_hba *hba)
> > @@ -6862,10 +6892,6 @@ static int ufs_get_device_desc(struct ufs_hba 
> > *hba)
> > 
> >  	model_index = desc_buf[DEVICE_DESC_PARAM_PRDCT_NAME];
> > 
> > -	/* Enable WB only for UFS-3.1 */
> > -	if (dev_info->wspecversion >= 0x310)
> > -		ufshcd_wb_probe(hba, desc_buf);
> > -
> >  	err = ufshcd_read_string_desc(hba, model_index,
> >  				      &dev_info->model, SD_ASCII_STD);
> >  	if (err < 0) {
> > @@ -6874,6 +6900,13 @@ static int ufs_get_device_desc(struct ufs_hba 
> > *hba)
> >  		goto out;
> >  	}
> > 
> > +	ufs_fixup_device_setup(hba);
> > +
> > +	/* Enable WB only for UFS-3.1 */
> 
> Also update this comment to reflect your change?
> 
> > +	if (dev_info->wspecversion >= 0x310 ||
> > +	    (hba->dev_quirks & UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES))
> > +		ufshcd_wb_probe(hba, desc_buf);
> > +
> 
> Can we somehow move this after ufshcd_tune_unipro_params() or come up 
> with
> a better way to leverage ufshcd_vops_apply_dev_quirks()? I am asking 
> this
> because if we only rely on adding quirks to ufs_fixups in ufshcd.c, the
> table will keep growing and I am sure it will - as flash vendors are 
> trying
> to make their UFS2.1 products to be capable of WB (different densities 
> and
> different NAND processes from different vendors, the combos can be quite 
> a
> few). Meanwhile, some models are specifically made for some customers to
> support WB, meaning having them in the table may not help in a 
> generalized
> way, and it is not like some hot fixes that we have to take, it is just 
> for
> a non-standard feature. If we can leverage 
> ufshcd_vops_apply_dev_quirks(),
> SoC vendors can freely add the quirk without touching ufs_fixups table,
> which means you don't need to update ufs_fixups every time just for 
> adding
> a new model (GKI rules), you can have your own WB white list in vendor
> driver. What do you think?
> 
> Thanks,
> 
> Can Guo.

Very appreciate your useful and constructive comments : )

Please take a look at v4. In v4, I introduce a "fixup_dev_quirks" vop to
allow vendors to "fix" device quirks which can be a general solution not
only for newly introduced UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES but
also for other quirks.

Thanks,
Stanley Chu

> 
> >  	/*
> >  	 * ufshcd_read_string_desc returns size of the string
> >  	 * reset the error value
> > @@ -6893,21 +6926,6 @@ static void ufs_put_device_desc(struct ufs_hba 
> > *hba)
> >  	dev_info->model = NULL;
> >  }
> > 
> > -static void ufs_fixup_device_setup(struct ufs_hba *hba)
> > -{
> > -	struct ufs_dev_fix *f;
> > -	struct ufs_dev_info *dev_info = &hba->dev_info;
> > -
> > -	for (f = ufs_fixups; f->quirk; f++) {
> > -		if ((f->wmanufacturerid == dev_info->wmanufacturerid ||
> > -		     f->wmanufacturerid == UFS_ANY_VENDOR) &&
> > -		     ((dev_info->model &&
> > -		       STR_PRFX_EQUAL(f->model, dev_info->model)) ||
> > -		      !strcmp(f->model, UFS_ANY_MODEL)))
> > -			hba->dev_quirks |= f->quirk;
> > -	}
> > -}
> > -
> >  /**
> >   * ufshcd_tune_pa_tactivate - Tunes PA_TActivate of local UniPro
> >   * @hba: per-adapter instance
> > @@ -7244,8 +7262,6 @@ static int ufshcd_device_params_init(struct 
> > ufs_hba *hba)
> > 
> >  	ufshcd_get_ref_clk_gating_wait(hba);
> > 
> > -	ufs_fixup_device_setup(hba);
> > -
> >  	if (!ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_READ_FLAG,
> >  			QUERY_FLAG_IDN_PWR_ON_WPE, &flag))
> >  		hba->dev_info.f_power_on_wp_en = flag;