Re: [PATCH v1 1/2] scsi: ufs: core: Remove UFS_DEVICE_QUIRK_DELAY_AFTER_LPM quirk

[Bao D. Nguyen]

On 10/2/2025 12:57 AM, Peter Wang (王信友) wrote:
> On Wed, 2025-10-01 at 13:57 -0700, Bao D. Nguyen wrote:
> > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> > index 2e1fa8c..45e509b 100644
> > --- a/drivers/ufs/core/ufshcd.c
> > +++ b/drivers/ufs/core/ufshcd.c
> > @@ -9738,10 +9738,9 @@ static void ufshcd_vreg_set_lpm(struct ufs_hba
> > *hba)
> >          }
> >
> >          /*
> > -        * Some UFS devices require delay after VCC power rail is
> > turned-off.
> > +        * All UFS devices require delay after VCC power rail is
> > turned-off.
> >           */
> > -       if (vcc_off && hba->vreg_info.vcc &&
> > -               hba->dev_quirks & UFS_DEVICE_QUIRK_DELAY_AFTER_LPM)
> > +       if (vcc_off && hba->vreg_info.vcc)
> >                  usleep_range(5000, 5100);
> >   }
> >
> > diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-
> > mediatek.c
> > index f902ce0..5c204d1 100644
> > --- a/drivers/ufs/host/ufs-mediatek.c
> > +++ b/drivers/ufs/host/ufs-mediatek.c
> > @@ -40,8 +40,7 @@ static int  ufs_mtk_config_mcq(struct ufs_hba *hba,
> > bool irq);
> >   static const struct ufs_dev_quirk ufs_mtk_dev_fixups[] = {
> >          { .wmanufacturerid = UFS_ANY_VENDOR,
> >            .model = UFS_ANY_MODEL,
> > -         .quirk = UFS_DEVICE_QUIRK_DELAY_AFTER_LPM |
> > -               UFS_DEVICE_QUIRK_DELAY_BEFORE_LPM },
> > +         .quirk = UFS_DEVICE_QUIRK_DELAY_BEFORE_LPM },
> >          { .wmanufacturerid = UFS_VENDOR_SKHYNIX,
> >            .model = "H9HQ21AFAMZDAR",
> >            .quirk = UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES },
> > @@ -1713,15 +1712,13 @@ static void ufs_mtk_fixup_dev_quirks(struct
> > ufs_hba *hba)
> >   {
> >          ufshcd_fixup_dev_quirks(hba, ufs_mtk_dev_fixups);
> >
> > -       if (ufs_mtk_is_broken_vcc(hba) && hba->vreg_info.vcc &&
> > -           (hba->dev_quirks & UFS_DEVICE_QUIRK_DELAY_AFTER_LPM)) {
> > +       if (ufs_mtk_is_broken_vcc(hba) && hba->vreg_info.vcc) {
>
> Hi Bao,
>
> Adding a delay is not reasonable if we have decided to
> keep VCC always on.

Hi Peter,

The current Mediatek platform driver applies this quirk to all ufs 
vendors which is consistent with what we would like to do in the 
Qualcomm platform driver per the vendor's requests.

I do see that, about 5 years ago, Mediatek merged a patch to keep the 
device vcc always on, probably to workaround some HW issues. Since this 
is a very old patch and the impact of this change on a broken hardware 
is minimal, I would like weight the benefit of cleaning up the ufs core 
driver by removing the unnecessary quirk 
UFS_DEVICE_QUIRK_DELAY_AFTER_LPM vs the inconvenience of a 5ms 
(potentially reduce to 2ms) delay impact it may cause on an old broken 
HW in the suspend/shutdown path.

I believe removing the UFS_DEVICE_QUIRK_DELAY_AFTER_LPM quirk in the ufs 
core driver as well as all the platform drivers yields positive net 
benefits in this case.

Thanks, Bao

> Thanks
> Peter
>
>
> >                  hba->vreg_info.vcc->always_on = true;
> >                  /*
> >                   * VCC will be kept always-on thus we don't
> > -                * need any delay during regulator operations
> > +                * need any delay before putting device's VCC in LPM
> > mode.
> >                   */
> > -               hba->dev_quirks &=
> > ~(UFS_DEVICE_QUIRK_DELAY_BEFORE_LPM |
> > -                       UFS_DEVICE_QUIRK_DELAY_AFTER_LPM);
> > +               hba->dev_quirks &=
> > ~UFS_DEVICE_QUIRK_DELAY_BEFORE_LPM;