> 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@xxxxxxxxxxx>
> > ---
> > 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
