| Message ID | 1593678039-139543-1-git-send-email-kwmad.kim@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC,v1] ufs: introduce async ufs interface initialization | expand |
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
> > 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
> > > 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.
> 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
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.
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; > }
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; > }
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; }
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(-)