RE: [RFC PATCH v1] ufs: introduce async ufs interface initialization

["Kiwoong Kim" <kwmad.kim@samsung.com>]

> Hi,
> 
> >
> > When you set uic_link_state during sleep statae to UIC_LINK_OFF_STATE,
> > UFS driver does interface initialization that is a series of some
> > steps including fDeviceInit and thus, You might feel that its latency
> > is a little bit longer.
> >
> > This patch is run it asynchronously to reduce system wake-up time.
> Can you share your initial testing findings?
> How much time does it save?
For this, you can refer to the Grant's comment and
As you might know, the time reduction relies on devices,
situations - after spo or not or whatever.
The thing is that system wake-up time is very important for product makers
and the period that I'm seeing on this is not an amount that you can ignore.

> 
> >
> > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
> > ---
> >  drivers/scsi/ufs/Kconfig  |  10 ++++
> >  drivers/scsi/ufs/ufshcd.c | 120
> > ++++++++++++++++++++++++++++++++++---------
> > ---
> >  2 files changed, 100 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig index
> > 8cd9026..723e7cb 100644
> > --- a/drivers/scsi/ufs/Kconfig
> > +++ b/drivers/scsi/ufs/Kconfig
> > @@ -172,3 +172,13 @@ config SCSI_UFS_EXYNOS
> >
> >           Select this if you have UFS host controller on EXYNOS chipset.
> >           If unsure, say N.
> > +
> > +config SCSI_UFSHCD_ASYNC_INIT
> > +       bool "Asynchronous UFS interface initialization support"
> > +       depends on SCSI_UFSHCD
> > +       default n
> > +       ---help---
> > +       This selects the support of doing UFS interface initialization
> > +       asynchronously when you set link state to link off,
> > +       i.e. UIC_LINK_OFF_STATE, to reduce system wake-up time.
> > +       Select this if you have UFS Host Controller.
> Maybe replace this config switch with platform capability?
> So each platform vendor can choose if he is using a sync vs async init?
Got it.

> 
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 52abe82..b65d38c 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -8319,6 +8319,80 @@ static int ufshcd_suspend(struct ufs_hba *hba,
> > enum ufs_pm_op pm_op)
> >         return ret;
> >  }
> >
> > +static int ufshcd_post_resume(struct ufs_hba *hba)
> Why do you need to move this code around?
> If its async - then there is no shared code - you go through the full init
> flow, and just goto out?
With SCSI_UFSHCD_ASYNC_INIT, ufshcd_reset_and_restore would be run by
worker asynchronously and in this case, some stuffs that are supposed to run
after completion of ufshcd_reset_and_restore without SCSI_UFSHCD_ASYNC_INIT.
So the stuffs should be run somewhere in kworker context.
That's why I teared it.

> 
> > +{
> > +       int ret;
> > +
> > +       if (!ufshcd_is_ufs_dev_active(hba)) {
> > +               ret = ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE);
> > +               if (ret)
> > +                       goto out;
> > +       }
> > +
> > +       if (ufshcd_keep_autobkops_enabled_except_suspend(hba))
> > +               ufshcd_enable_auto_bkops(hba);
> > +       else
> > +               /*
> > +                * If BKOPs operations are urgently needed at this moment
> then
> > +                * keep auto-bkops enabled or else disable it.
> > +                */
> > +               ufshcd_urgent_bkops(hba);
> > +
> > +       hba->clk_gating.is_suspended = false;
> > +
> > +       if (hba->clk_scaling.is_allowed)
> > +               ufshcd_resume_clkscaling(hba);
> > +
> > +       /* Enable Auto-Hibernate if configured */
> > +       ufshcd_auto_hibern8_enable(hba);
> > +
> > +       if (hba->dev_info.b_rpm_dev_flush_capable) {
> > +               hba->dev_info.b_rpm_dev_flush_capable = false;
> > +               cancel_delayed_work(&hba->rpm_dev_flush_recheck_work);
> > +       }
> > +
> > +       /* Schedule clock gating in case of no access to UFS device yet
> */
> > +       ufshcd_release(hba);
> > +out:
> > +       return ret;
> > +}
> > +
> > +#if defined(SCSI_UFSHCD_ASYNC_INIT)
> > +static void ufshcd_async_resume(void *data, async_cookie_t cookie) {
> > +       struct ufs_hba *hba = (struct ufs_hba *)data;
> > +       unsigned long flags;
> > +       int ret = 0;
> > +       int retries = 2;
> > +
> > +       /* transition to block requests */
> > +       spin_lock_irqsave(hba->host->host_lock, flags);
> > +       hba->ufshcd_state = UFSHCD_STATE_RESET;
> > +       spin_unlock_irqrestore(hba->host->host_lock, flags);
> > +
> > +       /* initialize, instead of set_old_link_state ?? */
> > +       do {
> > +               ret = ufshcd_reset_and_restore(hba);
> > +               if (ret) {
> > +                       dev_err(hba->dev, "%s: reset and restore failed\n",
> > +                                       __func__);
> > +                       spin_lock_irqsave(hba->host->host_lock, flags);
> > +                       hba->ufshcd_state = UFSHCD_STATE_ERROR;
> > +                       hba->pm_op_in_progress = 0;
> > +                       spin_unlock_irqrestore(hba->host->host_lock, flags);
> > +                       return;
> > +               }
> > +               ret = ufshcd_post_resume(hba);
> > +       } while (ret && --retries);
> > +       if (ret)
> > +               goto reset;
> > +
> > +       hba->pm_op_in_progress = 0;
> > +       if (ret)
> > +               ufshcd_update_reg_hist(&hba->ufs_stats.resume_err,
> > +(u32)ret); } #endif
> > +
> >  /**
> >   * ufshcd_resume - helper function for resume operations
> >   * @hba: per adapter instance
> > @@ -8370,6 +8444,14 @@ static int ufshcd_resume(struct ufs_hba *hba,
> > enum ufs_pm_op pm_op)
> >                  * A full initialization of the host and the device is
> >                  * required since the link was put to off during suspend.
> >                  */
> > +#if defined(SCSI_UFSHCD_ASYNC_INIT)
> > +               /*
> > +                * Assuems error free since ufshcd_probe_hba failure is
> > +                * uncorrectable.
> > +                */
> > +               ufshcd_async_schedule(ufshcd_async_resume, hba);
> > +               goto out_new;
> > +#else
> >                 ret = ufshcd_reset_and_restore(hba);
> >                 /*
> >                  * ufshcd_reset_and_restore() should have already @@
> > -8377,38 +8459,12 @@ static int ufshcd_resume(struct ufs_hba *hba,
> > enum ufs_pm_op pm_op)
> >                  */
> >                 if (ret || !ufshcd_is_link_active(hba))
> >                         goto vendor_suspend;
> > +#endif
> >         }
> >
> > -       if (!ufshcd_is_ufs_dev_active(hba)) {
> > -               ret = ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE);
> > -               if (ret)
> > -                       goto set_old_link_state;
> > -       }
> > -
> > -       if (ufshcd_keep_autobkops_enabled_except_suspend(hba))
> > -               ufshcd_enable_auto_bkops(hba);
> > -       else
> > -               /*
> > -                * If BKOPs operations are urgently needed at this moment
> then
> > -                * keep auto-bkops enabled or else disable it.
> > -                */
> > -               ufshcd_urgent_bkops(hba);
> > -
> > -       hba->clk_gating.is_suspended = false;
> > -
> > -       if (hba->clk_scaling.is_allowed)
> > -               ufshcd_resume_clkscaling(hba);
> > -
> > -       /* Enable Auto-Hibernate if configured */
> > -       ufshcd_auto_hibern8_enable(hba);
> > -
> > -       if (hba->dev_info.b_rpm_dev_flush_capable) {
> > -               hba->dev_info.b_rpm_dev_flush_capable = false;
> > -               cancel_delayed_work(&hba->rpm_dev_flush_recheck_work);
> > -       }
> > -
> > -       /* Schedule clock gating in case of no access to UFS device yet
> */
> > -       ufshcd_release(hba);
> > +       ret = ufshcd_post_resume(hba);
> > +       if (ret)
> > +               goto set_old_link_state;
> >
> >         goto out;
> >
> > @@ -8427,6 +8483,10 @@ static int ufshcd_resume(struct ufs_hba *hba,
> > enum ufs_pm_op pm_op)
> >         hba->pm_op_in_progress = 0;
> >         if (ret)
> >                 ufshcd_update_reg_hist(&hba->ufs_stats.resume_err,
> > (u32)ret);
> > +       /* For async init, pm_op_in_progress still needs to be one */
> > +#if defined(SCSI_UFSHCD_ASYNC_INIT)
> > +out_new:
> > +#endif
> >         return ret;
> >  }
> 
> Thanks,
> Avri
> 
> >
> > --
> > 2.7.4



Thanks.
Kiwoong Kim