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

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

[Kiwoong Kim]

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.

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.
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)
+{
+	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;
 }
 
-- 
2.7.4

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3440 bytes --]

Hi Kiwoong,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on scsi/for-next]
[also build test WARNING on mkp-scsi/for-next]
[cannot apply to v5.8-rc3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Kiwoong-Kim/ufs-introduce-async-ufs-interface-initialization/20200702-163018
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 003a086ffc0d1affbb8300b36225fb8150a2d40a)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/scsi/ufs/ufshcd.c:8329:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
           if (!ufshcd_is_ufs_dev_active(hba)) {
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/scsi/ufs/ufshcd.c:8360:9: note: uninitialized use occurs here
           return ret;
                  ^~~
   drivers/scsi/ufs/ufshcd.c:8329:2: note: remove the 'if' if its condition is always true
           if (!ufshcd_is_ufs_dev_active(hba)) {
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/scsi/ufs/ufshcd.c:8327:9: note: initialize the variable 'ret' to silence this warning
           int ret;
                  ^
                   = 0
   1 warning generated.

vim +8329 drivers/scsi/ufs/ufshcd.c

  8324	
  8325	static int ufshcd_post_resume(struct ufs_hba *hba)
  8326	{
  8327		int ret;
  8328	
> 8329		if (!ufshcd_is_ufs_dev_active(hba)) {
  8330			ret = ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE);
  8331			if (ret)
  8332				goto out;
  8333		}
  8334	
  8335		if (ufshcd_keep_autobkops_enabled_except_suspend(hba))
  8336			ufshcd_enable_auto_bkops(hba);
  8337		else
  8338			/*
  8339			 * If BKOPs operations are urgently needed at this moment then
  8340			 * keep auto-bkops enabled or else disable it.
  8341			 */
  8342			ufshcd_urgent_bkops(hba);
  8343	
  8344		hba->clk_gating.is_suspended = false;
  8345	
  8346		if (hba->clk_scaling.is_allowed)
  8347			ufshcd_resume_clkscaling(hba);
  8348	
  8349		/* Enable Auto-Hibernate if configured */
  8350		ufshcd_auto_hibern8_enable(hba);
  8351	
  8352		if (hba->dev_info.b_rpm_dev_flush_capable) {
  8353			hba->dev_info.b_rpm_dev_flush_capable = false;
  8354			cancel_delayed_work(&hba->rpm_dev_flush_recheck_work);
  8355		}
  8356	
  8357		/* Schedule clock gating in case of no access to UFS device yet */
  8358		ufshcd_release(hba);
  8359	out:
  8360		return ret;
  8361	}
  8362	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 75316 bytes --]

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

[Avri Altman]

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?

> 
> 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?

> 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?

> +{
> +       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

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

[Kiwoong Kim]

> > 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?

Will share when I'm done. And I think you might already know and
the time is variant per device and its situation, particularly for fDeviceInit.

Thanks.
Kiwoong Kim

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

[Grant Jung]

> > > 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?
> 
> Will share when I'm done. And I think you might already know and the time
> is variant per device and its situation, particularly for fDeviceInit.
> 
> Thanks.
> Kiwoong Kim

I think it depends on each platform. 
I has used this since years ago to reduce system wake-up time and could save about 60ms at that time.

BR
Grant.

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

[Kiwoong Kim]

> 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

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

[Bart Van Assche]

On 2020-07-02 01:20, Kiwoong Kim wrote:
> 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.

Device drivers like UFS should only perform tasks that are specific to
the supported device(s). Asynchronous resume from a sleep state is a
mechanism that may also benefit other device drivers. Please work with
the maintainers of the power management subsystem (Rafael J. Wysocki and
Pavel Machek) to integrate support for this feature in the kernel power
management subsystem. The kernel power management subsystem exists in
the directory kernel/power.

Thanks,

Bart.

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

[Can Guo]

Hi Kiwoong,

On 2020-07-02 16:20, Kiwoong Kim wrote:
> 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.

Have you considered the existing async suspend/resume support from
kernel power management framework? Can device_enable_async_suspend()
serve your purpose?

I don't see how this change works in two ways

#1 During system resume, how do you make sure resume of children
devices come after the resume of their parent? To be more specific,
in UFS's case, how do you make sure scsi devices (sda, sdb...) start
to resume only AFTER hba resume is finished (I mean all steps in hba
probe finished, not just schedule the async resume work). Your test
passed only because that in current scsi device's resume ops, no actual
commands need to be sent during scsi devices resume. If some commands
need to be sent during scsi device resume (say SSU commands), you would
run into error since scsi commands are sent before host is fully 
resumed.
If you use kernel power management framework, during resume, dpm_wait()
is called for each device to make its parent has finished resuming.

#2 ufshcd_resume() is called in both hba system and runtime resume path.
Your change even makes hba runtime resume ops "async". How can you make
sure that hba is resumed after pm_runtime_get_sync() returns? Besides,
I don't think it can work well along with block layer PM.

Thanks,

Can Guo

> 
> 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.
> 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)
> +{
> +	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;
>  }

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

[Can Guo]

Resent, typo fixed

Hi Kiwoong,

On 2020-07-02 16:20, Kiwoong Kim wrote:
> 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.
> 

Have you considered the existing async suspend/resume support from
kernel power management framework? Can device_enable_async_suspend()
serve your purpose?

I don't see how this change works in two ways. With this change,
hba resume ops just schedules an async resume work and returns.

#1 During system resume, how do you make sure resume of children
devices come after the resume of their parent? To be more specific,
in UFS's case, how do you make sure scsi devices (sda, sdb...) start
to resume only AFTER hba resume is finished (I mean all steps in hba
probe finished, not just schedule the async resume work)? Your test
passed only because that in current UFS scsi device/target/host's resume
ops, no actual commands need to be sent. If some commands need to be
sent from scsi dev resume ops (say SSU commands sent by sd_resume if
sdp->manage_start_stop == 1), you would run into error since scsi 
commands
may be sent before host is fully resumed. If you use kernel power
management framework, during system resume, dpm_wait() is called for
each device to make sure its parent has finished resuming.

#2 ufshcd_resume() is called in both hba system and runtime resume path.
How can you make sure that hba is fully resumed after 
pm_runtime_get_sync()
returns? Besides, similar doubt as above in #1, how do you make sure 
that,
during runtime resume, scsi device runtime resume ops happen after hba 
has
been fully resumed? I don't think it can even work along with block 
layer PM.

Thanks,

Can Guo

> 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.
> 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)
> +{
> +	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;
>  }