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

[Avri Altman <Avri.Altman@wdc.com>]

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

> +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?

> +       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?


> +
> +
> +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.


>  /**
>   * 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()

 
>         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