Re: [PATCH v1 1/3] scsi: ufs: add write booster feature support

[Asutosh Das (asd)]

On 4/12/2020 5:43 AM, Avri Altman wrote:
> Hi,
>
> >   enum ufs_desc_def_size {
> > -       QUERY_DESC_DEVICE_DEF_SIZE              = 0x40,
> > +       QUERY_DESC_DEVICE_DEF_SIZE              = 0x59,
> >          QUERY_DESC_CONFIGURATION_DEF_SIZE       = 0x90,
> >          QUERY_DESC_UNIT_DEF_SIZE                = 0x23,
> Might as well update the unit descriptor size - its 0x2D now,
> As you are adding its new fields
>
> >          QUERY_DESC_INTERCONNECT_DEF_SIZE        = 0x06,
> > @@ -219,6 +226,7 @@ enum unit_desc_param {
> >          UNIT_DESC_PARAM_PHY_MEM_RSRC_CNT        = 0x18,
> >          UNIT_DESC_PARAM_CTX_CAPABILITIES        = 0x20,
> >          UNIT_DESC_PARAM_LARGE_UNIT_SIZE_M1      = 0x22,
> > +       UNIT_DESC_PARAM_WB_BUF_ALLOC_UNITS      = 0x29,
> >   };
> >
> >   /* Device descriptor parameters offsets in bytes*/
> > @@ -258,6 +266,9 @@ enum device_desc_param {
> >          DEVICE_DESC_PARAM_PSA_MAX_DATA          = 0x25,
> >          DEVICE_DESC_PARAM_PSA_TMT               = 0x29,
> >          DEVICE_DESC_PARAM_PRDCT_REV             = 0x2A,
> > +       DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP   = 0x4F,
> DEVICE_DESC_PARAM_WB_USER_TYPE               = 0x53,
>
> > +       DEVICE_DESC_PARAM_WB_TYPE               = 0x54,
> > +       DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS = 0x55,
> >   };
>
>
> > +enum ufs_dev_wb_buf_user_cap_config {
> > +       UFS_WB_BUFF_PRESERVE_USER_SPACE = 1,
> > +       UFS_WB_BUFF_USER_SPACE_RED_EN = 2,
> > +};
> Maybe better to follow the spec here:
> Reduced - 0
> Preserve - 1
I don't think these enums would be needed. Let me check and remove these 
otherwise.
 >> +static inline void ufshcd_wb_config(struct ufs_hba *hba)
> > +{
> > +       int ret;
> > +
> > +       if (!ufshcd_wb_sup(hba))
> > +               return;
> > +
> > +       ret = ufshcd_wb_ctrl(hba, true);
> Why are you setting WB on init?
During init the clocks are scaled-up. Hence, WB is set at init.
It gets disabled when the clocks are scaled down, which is almost 
immediate anyway.

> > +       if (ret)
> > +               dev_err(hba->dev, "%s: Enable WB failed: %d\n", __func__, ret);
> > +       else
> > +               dev_info(hba->dev, "%s: Write Booster Configured\n", __func__);
> > +       ret = ufshcd_wb_toggle_flush_during_h8(hba, true);
> > +       if (ret)
> > +               dev_err(hba->dev, "%s: En WB flush during H8: failed: %d\n",
> > +                       __func__, ret);
> > +       ufshcd_wb_toggle_flush(hba, true);
> Why set explicit flush on init?
The design is to enable flush and let the device handle flush when DB's 
are empty on its own. Hence I'm setting flush at init itself.
> > +
> > +
> > +static bool ufshcd_wb_keep_vcc_on(struct ufs_hba *hba)
> > +{
> > +       int ret;
> > +       u32 cur_buf, avail_buf;
> > +
> > +       if (!ufshcd_wb_sup(hba))
> > +               return false;
> > +       /*
> > +        * The ufs device needs the vcc to be ON to flush.
> > +        * With user-space reduction enabled, it's enough to enable flush
> > +        * by checking only the available buffer. The threshold
> > +        * defined here is > 90% full.
> > +        * With user-space preserved enabled, the current-buffer
> > +        * should be checked too because the wb buffer size can reduce
> > +        * when disk tends to be full. This info is provided by current
> > +        * buffer (dCurrentWriteBoosterBufferSize). There's no point in
> > +        * keeping vcc on when current buffer is empty.
> > +        */
> > +       ret = ufshcd_query_attr_retry(hba,
> > UPIU_QUERY_OPCODE_READ_ATTR,
> > +                                     QUERY_ATTR_IDN_AVAIL_WB_BUFF_SIZE,
> > +                                     0, 0, &avail_buf);
> > +       if (ret) {
> > +               dev_warn(hba->dev, "%s dAvailableWriteBoosterBufferSize read
> > failed %d\n",
> > +                        __func__, ret);
> > +               return false;
> > +       }
> > +
> > +       ret = ufshcd_vops_get_user_cap_mode(hba);
> The Preserve User Space mode should be read from -
> bWriteBoosterBufferPreserveUserSpaceEn of the device descriptor - 0ffset 0x53.
> The driver should have no judgement here.
> Based on what is configured, better to attach a helper pointer that will perform the below check,
> e.g. ufshcd_wb_preserve_keep_vcc_on() and ufshcd_wb_reduced_keep_vcc_on().
> Which will simplify the logic here and make it much more readable.
> This makes the preparations you made for ufshcd_vops_get_user_cap_mode,
> and your second patch unneeded.
Thanks, that makes sense.
> >   /**
> >    * ufshcd_exception_event_handler - handle exceptions raised by device
> >    * @work: pointer to work data
> > @@ -6632,6 +6829,28 @@ static int ufs_get_device_desc(struct ufs_hba
> > *hba)
> >                                        desc_buf[DEVICE_DESC_PARAM_SPEC_VER + 1];
> >
> >          model_index = desc_buf[DEVICE_DESC_PARAM_PRDCT_NAME];
> > +       /* Enable WB only for UFS-3.1 */
> > +       if (dev_info->wspecversion >= 0x310) {
> > +               hba->dev_info.d_ext_ufs_feature_sup =
> > +                       get_unaligned_be32(desc_buf +
> > +                               DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP);
> > +               /*
> > +                * WB may be supported but not configured while provisioning.
> > +                * The spec says, in dedicated wb buffer mode,
> > +                * a max of 1 lun would have wb buffer configured.
> > +                * Now only shared buffer mode is supported.
> > +                */
> > +               hba->dev_info.b_wb_buffer_type =
> > +                       desc_buf[DEVICE_DESC_PARAM_WB_TYPE];
> > +
> > +               if (!hba->dev_info.b_wb_buffer_type)
> > +                       goto skip_alloc_unit;
> > +               hba->dev_info.d_wb_alloc_units =
> > +                       get_unaligned_be32(desc_buf +
> > +                               DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS);
> > +       }
> Maybe pack the above code in a wb_probe() designated helper,
> which will establish if WB is supported or not.
> If one of your validation tests fails, maybe you can just
> hba->caps &= ~UFSHCD_CAP_WB_EN;
> which will further simplify your ufshcd_wb_sup()
Good idea.
>   
> >          if ((req_dev_pwr_mode != hba->curr_dev_pwr_mode) &&
> > -            ((ufshcd_is_runtime_pm(pm_op) && !hba->auto_bkops_enabled) ||
> > -              !ufshcd_is_runtime_pm(pm_op))) {
> > +           ((ufshcd_is_runtime_pm(pm_op) && !hba->auto_bkops_enabled) ||
> > +            !ufshcd_is_runtime_pm(pm_op))) {
> Redundant space removal
>
>
>
> Thanks,
> Avri


--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
Linux Foundation Collaborative Project