[v3,0/3] WriteBooster Feature Support

v2 -> v3:
- Addressed Comments on refactoring
- Corrected the commit message

v1 -> v2:
- Refactor WriteBooster initialization, introduce ufshcd_wb_probe()
- Refactor ufshcd_wb_keep_vcc_on() and introduce a new function
  ufshcd_wb_presrv_usrspc_keep_vcc_on()
- Get the WriteBooster configuration by reading
  bWriteBoosterBufferPreserveUserSpaceEn

RFC -> v1:
- Added platform capability for WriteBooster 

RFC-:
v1 -> v2:
- Addressed comments on v1

- Supports shared buffer mode only

- Didn't use exception event as suggested.
  The reason being while testing I saw that the WriteBooster
  available buffer remains at 0x1 for a longer time if flush is
  enabled all the time as compared to an event-based enablement.
  This essentially means that writes go to the WriteBooster buffer
  more. Spec says that the if flush is enabled, the device would
  flush when it sees the command queue empty. So I guess that'd trigger
  flush more than an event based approach.
  Anyway the Vcc would be turned-off during system suspend, so flush
  would stop anyway.
  In this patchset, I never turn-off flush.
  Hence the RFC.

Asutosh Das (3):
  scsi: ufs: add write booster feature support
  ufs-qcom: scsi: configure write booster type
  ufs: sysfs: add sysfs entries for write booster

 drivers/scsi/ufs/ufs-qcom.c  |   8 ++
 drivers/scsi/ufs/ufs-sysfs.c |  39 ++++++-
 drivers/scsi/ufs/ufs.h       |  37 ++++++-
 drivers/scsi/ufs/ufshcd.c    | 241 ++++++++++++++++++++++++++++++++++++++++++-
 drivers/scsi/ufs/ufshcd.h    |  43 ++++++++
 5 files changed, 362 insertions(+), 6 deletions(-)

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
>