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;
