Re: [PATCH] ufs: core: Add HID support

["Peter Wang (王信友)" <peter.wang@mediatek.com>]

On Thu, 2025-04-17 at 20:50 +0800, Huan Tang wrote:
> +
> +#define HID_SCHED_COUNT_LIMIT  300
> +static int hid_sched_cnt;
> +static void ufs_hid_enable_work_fn(struct work_struct *work)
> +{
> +       struct ufs_hba *hba;
> +       int ret = 0;
> +       enum ufs_hid_defrag_operation defrag_op;
> +       u32 hid_ahit = 0;
> +       bool hid_flag = false;
> +
> +       hba = container_of(work, struct ufs_hba,
> ufs_hid_enable_work.work);
> +
> +       if (!hba->dev_info.hid_sup)
> +               return;
> +
> +       down(&hba->host_sem);
> +
> +       if (!ufshcd_is_user_access_allowed(hba)) {
> +               up(&hba->host_sem);
> +               return;
> +       }
> +
> +       ufshcd_rpm_get_sync(hba);
> +       hid_ahit = hba->ahit;
> +       ufshcd_auto_hibern8_update(hba, 0);
> +
> +       ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> +                       QUERY_ATTR_IDN_HID_STATE, 0, 0, &hba-
> >dev_info.hid_state);
> +       if (ret)
> +               hba->dev_info.hid_state = HID_IDLE;
> +
> +       switch (hba->dev_info.hid_state) {
> +       case HID_IDLE:
> +               defrag_op = HID_ANALYSIS_ENABLE;
> 

Hi Huan,

Can change to HID_ANALYSIS_AND_DEFRAG_ENABLE to save defragment time?


> +               hid_flag = true;
> +               break;
> +       case DEFRAG_REQUIRED:
> +               defrag_op = HID_ANALYSIS_AND_DEFRAG_ENABLE;
> +               hid_flag = true;
> +               break;
> +       case DEFRAG_COMPLETED:
> +       case DEFRAG_IS_NOT_REQUIRED:
> +               defrag_op = HID_ANALYSIS_AND_DEFRAG_DISABLE;
> +               hid_flag = true;
> +               break;
> 

Can just break? Because according the spec. 
After the host reads the bHIDState value when it is 04h (Defrag
Completion) or 05h (Defrag Not Required), the following parameters
shall be initialized: 
bHIDState value to 00h (Idle)


> 
> @@ -9614,6 +9619,8 @@ static int __ufshcd_wl_suspend(struct ufs_hba
> *hba, enum ufs_pm_op pm_op)
>                 req_link_state = UIC_LINK_OFF_STATE;
>         }
> 
> +       if (hba->dev_info.hid_sup)
> +               cancel_delayed_work_sync(&hba->ufs_hid_enable_work);
> 

Will have dead-lock when ufs_hid_enable_work_fn invoke
ufshcd_rpm_get_sync?


>         /*
>          * If we can't transition into any of the low power modes
>          * just gate the clocks.
> diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h
> index 8a24ed59ec46..6e8546024e09 100644
> --- a/include/ufs/ufs.h
> +++ b/include/ufs/ufs.h
> 
>  /* Descriptor idn for Query requests */
> @@ -390,6 +395,7 @@ enum {
>         UFS_DEV_EXT_TEMP_NOTIF          = BIT(6),
>         UFS_DEV_HPB_SUPPORT             = BIT(7),
>         UFS_DEV_WRITE_BOOSTER_SUP       = BIT(8),
> +       UFS_DEV_HID_SUPPORT     = BIT(13),
> 

Please align the arrangement of these enums.


>  };
>  #define UFS_DEV_HPB_SUPPORT_VERSION            0x310
> 
> @@ -454,6 +460,23 @@ enum ufs_ref_clk_freq {
>         REF_CLK_FREQ_INVAL      = -1,
>  };
> 
> +/* bDefragOperation attribute values */
> +enum ufs_hid_defrag_operation {
> +       HID_ANALYSIS_AND_DEFRAG_DISABLE = 0,
> +       HID_ANALYSIS_ENABLE     = 1,
> +       HID_ANALYSIS_AND_DEFRAG_ENABLE  = 2,
> 

Please align the arrangement of these enums.


> +};
> +
> +/* bHIDState attribute values */
> +enum ufs_hid_state {
> +       HID_IDLE        = 0,
> +       ANALYSIS_IN_PROGRESS    = 1,
> +       DEFRAG_REQUIRED = 2,
> +       DEFRAG_IN_PROGRESS      = 3,
> +       DEFRAG_COMPLETED        = 4,
> +       DEFRAG_IS_NOT_REQUIRED  = 5,
> 

Please align the arrangement of these enums.

Thanks.
Peter