Messages in this thread Patch in this message | | | Date | Sat, 02 May 2020 15:47:36 +0800 | From | Can Guo <> | Subject | Re: [PATCH v3 1/5] scsi: ufs: enable WriteBooster on some pre-3.1 UFS devices |
| |
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@mediatek.com> > --- > 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.
> /* > * 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;
| |