Re: [<RFC PATCH v1> 1/2] scsi: ufs: add write booster feature support

[Asutosh Das (asd)]

On 2/25/2020 4:50 AM, Avri Altman wrote:
> > +/*
> > + * ufshcd_get_wb_alloc_units - returns
> > "dLUNumWriteBoosterBufferAllocUnits"
> > + * @hba: per-adapter instance
> > + * @lun: UFS device lun id
> > + * @d_lun_wbb_au: pointer to buffer to hold the LU's alloc units info
> > + *
> > + * Returns 0 in case of success and d_lun_wbb_au would be returned
> > + * Returns -ENOTSUPP if reading d_lun_wbb_au is not supported.
> > + * Returns -EINVAL in case of invalid parameters passed to this function.
> > + */
> > +static int ufshcd_get_wb_alloc_units(struct ufs_hba *hba,
> > +                           u8 lun,
> > +                           u8 *d_lun_wbb_au)
> > +{
> > +       int ret;
> > +
> > +       if (!d_lun_wbb_au)
> > +               ret = -EINVAL;
> > +
> > +       /* WB can be supported only from LU0..LU7 */
> > +       else if (lun >= UFS_UPIU_MAX_GENERAL_LUN)
> > +               ret = -ENOTSUPP;
> > +       else
> > +               ret = ufshcd_read_unit_desc_param(hba,
> > +                                         lun,
> > +                                         UNIT_DESC_PARAM_WB_BUF_ALLOC_UNITS,
> > +                                         d_lun_wbb_au,
> > +                                         sizeof(*d_lun_wbb_au));
> You are reading here a single byte, instead of 4
>
> > +       return ret;
> > +}
> > +
> >   /**
> >    * ufshcd_get_lu_power_on_wp_status - get LU's power on write protect
> >    * status
> > @@ -5194,6 +5267,165 @@ static void
> > ufshcd_bkops_exception_event_handler(struct ufs_hba *hba)
> >                                  __func__, err);
> >   }
> >
> > +static bool ufshcd_wb_sup(struct ufs_hba *hba)
> > +{
> > +       return ((hba->dev_info.d_ext_ufs_feature_sup &
> > +                  UFS_DEV_WRITE_BOOSTER_SUP) &&
> Don't you want to have a vendor cap as well,
> to allow the platform vendor to control this feature?
I presume each platform vendor would provide a provisioning 
script/method which would configure the WB properties.
It can be controlled through that.
> > +                 (hba->dev_info.b_wb_buffer_type
> > +                  || hba->dev_info.wb_config_lun));
> > +}
> > +
> > +
>
>
>
> > +static bool ufshcd_wb_is_buf_flush_needed(struct ufs_hba *hba)
> > +{
> > +       int ret;
> > +       u32 cur_buf, status, avail_buf;
> > +
> > +       if (!ufshcd_wb_sup(hba))
> > +               return false;
> > +
> > +       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);
> > +       if (ret <= 0) {
> > +               dev_dbg(hba->dev, "Get user-cap reduction mode: failed: %d\n",
> > +                       ret);
> > +               /* Most commonly used */
> > +               ret = UFS_WB_BUFF_PRESERVE_USER_SPACE;
> > +       }
> > +
> > +       hba->dev_info.keep_vcc_on = false;
> > +       if (ret == UFS_WB_BUFF_USER_SPACE_RED_EN) {
> > +               if (avail_buf <= UFS_WB_10_PERCENT_BUF_REMAIN) {
> > +                       hba->dev_info.keep_vcc_on = true;
> > +                       return true;
> > +               }
> > +               return false;
> > +       } else if (ret == UFS_WB_BUFF_PRESERVE_USER_SPACE) {
> > +               ret = ufshcd_query_attr_retry(hba,
> > UPIU_QUERY_OPCODE_READ_ATTR,
> > +                                             QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE,
> > +                                             0, 0, &cur_buf);
> > +               if (ret) {
> > +                       dev_err(hba->dev, "%s dCurWriteBoosterBufferSize read failed
> > %d\n",
> > +                                __func__, ret);
> > +                       return false;
> > +               }
> > +
> > +               if (!cur_buf) {
> > +                       dev_info(hba->dev, "dCurWBBuf: %d WB disabled until free-
> > space is available\n",
> > +                                cur_buf);
> > +                       return false;
> > +               }
> > +
> > +               ret = ufshcd_get_ee_status(hba, &status);
> > +               if (ret) {
> > +                       dev_err(hba->dev, "%s: failed to get exception status %d\n",
> > +                               __func__, ret);
> > +                       if (avail_buf < UFS_WB_40_PERCENT_BUF_REMAIN) {
> > +                               hba->dev_info.keep_vcc_on = true;
> > +                               return true;
> > +                       }
> > +                       return false;
> > +               }
> > +
> > +               status &= hba->ee_ctrl_mask;
> > +
> > +               if ((status & MASK_EE_URGENT_BKOPS) ||
> So you are getting the status, but not the bkops level.
> And what about WRITEBOOSTER_EVENT_EN? After all it was invented specifically for WB...
Yeah - WB exception event is not supported in this series. I will push 
another change to support that.
With WB enabled, any BKOPS exception level warrants the vcc to be on.
This is to minimize performance impact. Please correct this 
understanding from device perspective.

> > +                   (avail_buf < UFS_WB_40_PERCENT_BUF_REMAIN)) {
> > +                       hba->dev_info.keep_vcc_on = true;
> > +                       return true;
> > +               }
> > +       }
> > +       return false;
> > +}
>
> Thanks,
> Avri


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