[PATCH v1 0/2] scsi: ufs: fix device power mode during PM flow

[PATCH v1 0/2] scsi: ufs: fix device power mode during PM flow

[Stanley Chu]

Hi,

This series fixes a device power mode issue in suspend and resume flow.

Stanley Chu (2):
  scsi: ufs: set device as default active power mode during
    initialization only
  scsi: ufs: remove link_startup_again flow in ufshcd_link_startup

 drivers/scsi/ufs/ufshcd.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

-- 
2.18.0

[PATCH v1 1/2] scsi: ufs: set device as default active power mode during initialization only

[Stanley Chu]

Currently ufshcd_probe_hba() always sets device status as "active".
This shall be by an assumption that device is already in active state
during the boot stage before kernel.

However, if link is configured as "off" state and device is requested
to enter "sleep" or "powerdown" power mode during suspend flow, device
will NOT be waken up to "active" power mode during resume flow because
device is already set as "active" power mode in ufhcd_probe_hba().

Fix it by setting device as default active power mode during
initialization only, and skipping changing mode during PM flow
in ufshcd_probe_hba().

Fixes: 7caf489b99a4 (scsi: ufs: issue link starup 2 times if device isn't active)
Cc: Alim Akhtar <alim.akhtar@samsung.com>
Cc: Avri Altman <avri.altman@wdc.com>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Bean Huo <beanhuo@micron.com>
Cc: Can Guo <cang@codeaurora.org>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
Cc: Subhash Jadavani <subhashj@codeaurora.org>
Cc: stable@vger.kernel.org
Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufshcd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index ed02a704c1c2..9abb7085a5d0 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6986,7 +6986,8 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
 	ufshcd_tune_unipro_params(hba);
 
 	/* UFS device is also active now */
-	ufshcd_set_ufs_dev_active(hba);
+	if (!hba->pm_op_in_progress)
+		ufshcd_set_ufs_dev_active(hba);
 	ufshcd_force_reset_auto_bkops(hba);
 	hba->wlun_dev_clr_ua = true;
 
-- 
2.18.0

Re: [PATCH v1 1/2] scsi: ufs: set device as default active power mode during initialization only

[asutoshd]

Hi Stanley,

On 2019-12-30 00:12, Stanley Chu wrote:
> Currently ufshcd_probe_hba() always sets device status as "active".
> This shall be by an assumption that device is already in active state
> during the boot stage before kernel.
> 
> However, if link is configured as "off" state and device is requested
> to enter "sleep" or "powerdown" power mode during suspend flow, device
> will NOT be waken up to "active" power mode during resume flow because
> device is already set as "active" power mode in ufhcd_probe_hba().
> 
> Fix it by setting device as default active power mode during
> initialization only, and skipping changing mode during PM flow
> in ufshcd_probe_hba().
> 
> Fixes: 7caf489b99a4 (scsi: ufs: issue link starup 2 times if device
> isn't active)
> Cc: Alim Akhtar <alim.akhtar@samsung.com>
> Cc: Avri Altman <avri.altman@wdc.com>
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Bean Huo <beanhuo@micron.com>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Matthias Brugger <matthias.bgg@gmail.com>
> Cc: Subhash Jadavani <subhashj@codeaurora.org>
> Cc: stable@vger.kernel.org
> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index ed02a704c1c2..9abb7085a5d0 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6986,7 +6986,8 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
>  	ufshcd_tune_unipro_params(hba);
> 
>  	/* UFS device is also active now */
> -	ufshcd_set_ufs_dev_active(hba);
> +	if (!hba->pm_op_in_progress)
> +		ufshcd_set_ufs_dev_active(hba);
>  	ufshcd_force_reset_auto_bkops(hba);
>  	hba->wlun_dev_clr_ua = true;

I see that there's a get_sync done before.
So, how would the suspend be triggered in that case?

Thanks,
asd

Re: [PATCH v1 1/2] scsi: ufs: set device as default active power mode during initialization only

[Stanley Chu]

Hi Asutosh,


> I see that there's a get_sync done before.
> So, how would the suspend be triggered in that case?
> 

Would you mean pm_runtime_get_sync() in ufshcd_init()?
If yes, it will only happen during initialization.

The runtime resume path may go through ufshcd_probe_hba() without
ufshcd_init() invoked before, for example,

ufshcd_probe_hba+0xe10/0x1874
ufshcd_host_reset_and_restore+0x114/0x1a4
ufshcd_resume+0x1d0/0x480
ufshcd_runtime_resume+0x40/0x188
ufshcd_pltfrm_runtime_resume+0x10/0x18
pm_generic_runtime_resume+0x24/0x44
__rpm_callback+0x100/0x250
rpm_resume+0x548/0x7c8
rpm_resume+0x2b4/0x7c8
rpm_resume+0x2b4/0x7c8
rpm_resume+0x2b4/0x7c8
pm_runtime_work+0x9c/0xa0
process_one_work+0x210/0x4e0
worker_thread+0x390/0x520
kthread+0x154/0x18c
ret_from_fork+0x10/0x18

This case happens if link is in "off" state while resume.

Thanks,
Stanley

Re: [PATCH v1 1/2] scsi: ufs: set device as default active power mode during initialization only

[Can Guo]

On 2019-12-31 09:07, Stanley Chu wrote:
> Hi Asutosh,
> 
> 
>> I see that there's a get_sync done before.
>> So, how would the suspend be triggered in that case?
>> 
> 
> Would you mean pm_runtime_get_sync() in ufshcd_init()?
> If yes, it will only happen during initialization.
> 
> The runtime resume path may go through ufshcd_probe_hba() without
> ufshcd_init() invoked before, for example,
> 
> ufshcd_probe_hba+0xe10/0x1874
> ufshcd_host_reset_and_restore+0x114/0x1a4
> ufshcd_resume+0x1d0/0x480
> ufshcd_runtime_resume+0x40/0x188
> ufshcd_pltfrm_runtime_resume+0x10/0x18
> pm_generic_runtime_resume+0x24/0x44
> __rpm_callback+0x100/0x250
> rpm_resume+0x548/0x7c8
> rpm_resume+0x2b4/0x7c8
> rpm_resume+0x2b4/0x7c8
> rpm_resume+0x2b4/0x7c8
> pm_runtime_work+0x9c/0xa0
> process_one_work+0x210/0x4e0
> worker_thread+0x390/0x520
> kthread+0x154/0x18c
> ret_from_fork+0x10/0x18
> 
> This case happens if link is in "off" state while resume.
> 
> Thanks,
> Stanley

Hi Stanley,

I see skipping ufshcd_set_ufs_dev_active() in ufshcd_probe_hba()
if it is called from ufshcd_resume() path is the purpose here.

If so, then ufshcd_set_dev_pwr_mode() would be called, meaning
SSU command will be sent. Why is this SSU command needed to be
sent after a full host reset and restore? Is ufshcd_probe_hba()
not enough to make UFS device fully functional?

<snip>
         } else if (ufshcd_is_link_off(hba)) {
                 ret = ufshcd_host_reset_and_restore(hba);
                 /*
                  * ufshcd_host_reset_and_restore() should have already
                  * set the link state as active
                  */
                 if (ret || !ufshcd_is_link_active(hba))
                         goto vendor_suspend;
         }

         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;
         }
<snip>

Thanks,

Can Guo.

Re: [PATCH v1 1/2] scsi: ufs: set device as default active power mode during initialization only

[Stanley Chu]

Hi Can,


> Hi Stanley,
> 
> I see skipping ufshcd_set_ufs_dev_active() in ufshcd_probe_hba()
> if it is called from ufshcd_resume() path is the purpose here.
> 
> If so, then ufshcd_set_dev_pwr_mode() would be called, meaning
> SSU command will be sent. Why is this SSU command needed to be
> sent after a full host reset and restore? Is ufshcd_probe_hba()
> not enough to make UFS device fully functional?

After resume (for both runtime resume and system resume), device power
mode shall be back to "Active" to service incoming requests.

I see two cases need device power mode transition during resume flow
1. Device Power Mode = Sleep
2. Device Power Mode = PowerDown

For 1, ufshcd_probe_hba() is not invoked during resume flow,
hba->curr_dev_pwr_mode = SLEEP, thus ufshcd_resume() can invoke
ufshcd_set_dev_pwr_mode() to change device power mode.

For 2, ufshcd_probe_hba() is invoked during resume flow, before this
fix, hba->curr_dev_pwr_mode will be set to ACTIVE (note that only this
flag is set as ACTIVE, but device may be still in PowerDown state if
device power is not fully shutdown or device HW reset is not executed
before resume), in the end, ufshcd_resume() will not invoke
ufshcd_set_dev_pwr_mode() to send SSU command to make device change back
to Active power mode.

But if device is fully reset before resume (not by current mainstream
driver), device can be already in "Active" power mode after power on
again during resume flow. In this case, it is OK to set
hba->curr_dev_pwr_mode as ACTIVE in ufshcd_probe_hba() and SSU command
is not necessary.

Thanks,
Stanley

> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v1 1/2] scsi: ufs: set device as default active power mode during initialization only

[Stanley Chu]

Hi Can,

On Tue, 2019-12-31 at 12:22 +0800, Stanley Chu wrote:
> Hi Can,
> 
> 
> > Hi Stanley,
> > 
> > I see skipping ufshcd_set_ufs_dev_active() in ufshcd_probe_hba()
> > if it is called from ufshcd_resume() path is the purpose here.
> > 
> > If so, then ufshcd_set_dev_pwr_mode() would be called, meaning
> > SSU command will be sent. Why is this SSU command needed to be
> > sent after a full host reset and restore? Is ufshcd_probe_hba()
> > not enough to make UFS device fully functional?
> 
> After resume (for both runtime resume and system resume), device power
> mode shall be back to "Active" to service incoming requests.
> 
> I see two cases need device power mode transition during resume flow
> 1. Device Power Mode = Sleep
> 2. Device Power Mode = PowerDown
> 
> For 1, ufshcd_probe_hba() is not invoked during resume flow,
> hba->curr_dev_pwr_mode = SLEEP, thus ufshcd_resume() can invoke
> ufshcd_set_dev_pwr_mode() to change device power mode.
> 
> For 2, ufshcd_probe_hba() is invoked during resume flow, before this
> fix, hba->curr_dev_pwr_mode will be set to ACTIVE (note that only this
> flag is set as ACTIVE, but device may be still in PowerDown state if
> device power is not fully shutdown or device HW reset is not executed
> before resume), in the end, ufshcd_resume() will not invoke
> ufshcd_set_dev_pwr_mode() to send SSU command to make device change back
> to Active power mode.
> 
> But if device is fully reset before resume (not by current mainstream
> driver), device can be already in "Active" power mode after power on
> again during resume flow. In this case, it is OK to set
> hba->curr_dev_pwr_mode as ACTIVE in ufshcd_probe_hba() and SSU command
> is not necessary.
> 
> Thanks,
> Stanley

I think currently the assumption in ufshcd_probe_hba() that "device
shall be already in Active power mode" makes sense because many device
commands will be sent to device in ufshcd_probe_hba() but device is not
promised to handle those commands in PowerDown power mode according to
UFS spec. 

So, maybe always ensuring device being Active power mode before leaving
ufshcd_probe_hba() is more reasonable. If so, I will drop this patch
first.

Thanks so much for your reviews.

Happy new year!

Thanks,
Stanley

Re: [PATCH v1 1/2] scsi: ufs: set device as default active power mode during initialization only

[Can Guo]

On 2019-12-31 15:44, Stanley Chu wrote:
> Hi Can,
> 
> On Tue, 2019-12-31 at 12:22 +0800, Stanley Chu wrote:
>> Hi Can,
>> 
>> 
>> > Hi Stanley,
>> >
>> > I see skipping ufshcd_set_ufs_dev_active() in ufshcd_probe_hba()
>> > if it is called from ufshcd_resume() path is the purpose here.
>> >
>> > If so, then ufshcd_set_dev_pwr_mode() would be called, meaning
>> > SSU command will be sent. Why is this SSU command needed to be
>> > sent after a full host reset and restore? Is ufshcd_probe_hba()
>> > not enough to make UFS device fully functional?
>> 
>> After resume (for both runtime resume and system resume), device power
>> mode shall be back to "Active" to service incoming requests.
>> 
>> I see two cases need device power mode transition during resume flow
>> 1. Device Power Mode = Sleep
>> 2. Device Power Mode = PowerDown
>> 
>> For 1, ufshcd_probe_hba() is not invoked during resume flow,
>> hba->curr_dev_pwr_mode = SLEEP, thus ufshcd_resume() can invoke
>> ufshcd_set_dev_pwr_mode() to change device power mode.
>> 
>> For 2, ufshcd_probe_hba() is invoked during resume flow, before this
>> fix, hba->curr_dev_pwr_mode will be set to ACTIVE (note that only this
>> flag is set as ACTIVE, but device may be still in PowerDown state if
>> device power is not fully shutdown or device HW reset is not executed
>> before resume), in the end, ufshcd_resume() will not invoke
>> ufshcd_set_dev_pwr_mode() to send SSU command to make device change 
>> back
>> to Active power mode.
>> 
>> But if device is fully reset before resume (not by current mainstream
>> driver), device can be already in "Active" power mode after power on
>> again during resume flow. In this case, it is OK to set
>> hba->curr_dev_pwr_mode as ACTIVE in ufshcd_probe_hba() and SSU command
>> is not necessary.
>> 
>> Thanks,
>> Stanley
> 
> I think currently the assumption in ufshcd_probe_hba() that "device
> shall be already in Active power mode" makes sense because many device
> commands will be sent to device in ufshcd_probe_hba() but device is not
> promised to handle those commands in PowerDown power mode according to
> UFS spec.
> 
> So, maybe always ensuring device being Active power mode before leaving
> ufshcd_probe_hba() is more reasonable. If so, I will drop this patch
> first.
> 
> Thanks so much for your reviews.
> 
> Happy new year!
> 
> Thanks,
> Stanley

Hi Stanley,

I missed this mail before I hit send. In current code, as per my 
understanding,
UFS device's power state should be Active after ufshcd_link_startup() 
returns.
If I am wrong, please feel free to correct me.

Due to you are almost trying to revert commit 7caf489b99a42a, I am just 
wondering
if you encounter failure/error caused by it.

Happy new year to you too!

Thanks,

Can Guo

Re: [PATCH v1 1/2] scsi: ufs: set device as default active power mode during initialization only

[Stanley Chu]

Hi Can,

On Tue, 2019-12-31 at 16:35 +0800, Can Guo wrote:

> Hi Stanley,
> 
> I missed this mail before I hit send. In current code, as per my 
> understanding,
> UFS device's power state should be Active after ufshcd_link_startup() 
> returns.
> If I am wrong, please feel free to correct me.
> 

Yes, this assumption of ufshcd_probe_hba() is true so I will drop this
patch.
Thanks for remind.

> Due to you are almost trying to revert commit 7caf489b99a42a, I am just 
> wondering
> if you encounter failure/error caused by it.

Yes, we actually have some doubts from the commit message of "scsi: ufs:
issue link startup 2 times if device isn't active"

If we configured system suspend as device=PowerDown/Link=LinkDown mode,
during resume, the 1st link startup will be successful, and after that
device could be accessed normally so it shall be already in Active power
mode. We did not find devices which need twice linkup for normal work.

And because the 1st linkup is OK, the forced 2nd linkup by commit "scsi:
ufs: issue link startup 2 times if device isn't active" leads to link
lost and finally the 3rd linkup is made again by retry mechanism in
ufshcd_link_startup() and be successful. So a linkup performance issue
is introduced here: We actually need one-time linkup only but finally
got 3 linkup operations.

According to the UFS spec, all reset types (including POR and Host
UniPro Warm Reset which both may happen in above configurations) other
than LU reset, UFS device power mode shall return to Sleep mode or
Active mode depending on bInitPowerMode, by default, it's Active mode.

So we are curious that why enforcing twice linkup is necessary here?
Could you kindly help us clarify this?

If anything wrong in above description, please feel free to correct me.

> 
> Happy new year to you too!
> 
> Thanks,
> 
> Can Guo

Thanks,

Stanley

> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v1 1/2] scsi: ufs: set device as default active power mode during initialization only

[Can Guo]

On 2020-01-02 14:38, Stanley Chu wrote:
> Hi Can,
> 
> On Tue, 2019-12-31 at 16:35 +0800, Can Guo wrote:
> 
>> Hi Stanley,
>> 
>> I missed this mail before I hit send. In current code, as per my
>> understanding,
>> UFS device's power state should be Active after ufshcd_link_startup()
>> returns.
>> If I am wrong, please feel free to correct me.
>> 
> 
> Yes, this assumption of ufshcd_probe_hba() is true so I will drop this
> patch.
> Thanks for remind.
> 
>> Due to you are almost trying to revert commit 7caf489b99a42a, I am 
>> just
>> wondering
>> if you encounter failure/error caused by it.
> 
> Yes, we actually have some doubts from the commit message of "scsi: 
> ufs:
> issue link startup 2 times if device isn't active"
> 
> If we configured system suspend as device=PowerDown/Link=LinkDown mode,
> during resume, the 1st link startup will be successful, and after that
> device could be accessed normally so it shall be already in Active 
> power
> mode. We did not find devices which need twice linkup for normal work.
> 
> And because the 1st linkup is OK, the forced 2nd linkup by commit 
> "scsi:
> ufs: issue link startup 2 times if device isn't active" leads to link
> lost and finally the 3rd linkup is made again by retry mechanism in
> ufshcd_link_startup() and be successful. So a linkup performance issue
> is introduced here: We actually need one-time linkup only but finally
> got 3 linkup operations.
> 
> According to the UFS spec, all reset types (including POR and Host
> UniPro Warm Reset which both may happen in above configurations) other
> than LU reset, UFS device power mode shall return to Sleep mode or
> Active mode depending on bInitPowerMode, by default, it's Active mode.
> 
> So we are curious that why enforcing twice linkup is necessary here?
> Could you kindly help us clarify this?
> 
> If anything wrong in above description, please feel free to correct me.
> 

Hi Stanley,

Above description is correct. The reason why the UFS device becomes
Active after the 1st link startup in your experiment is due to you
set spm_lvl to 5, during system suspend, UFS device is powered down.
When resume kicks start, the UFS device is power cycled once.

Moreover, if you set rpm_lvl to 5, during runtime suspend, if bkops is
enabled, the UFS device will not be powered off, meaning when runtime
resume kicks start, the UFS device is not power cycled, in this case,
we need 3 times of link startup.

Does above explain?

Thanks,

Can Guo.

>> 
>> Happy new year to you too!
>> 
>> Thanks,
>> 
>> Can Guo
> 
> Thanks,
> 
> Stanley
> 
>> 
>> _______________________________________________
>> Linux-mediatek mailing list
>> Linux-mediatek@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v1 1/2] scsi: ufs: set device as default active power mode during initialization only

[Can Guo]

On 2020-01-03 09:51, Can Guo wrote:
> On 2020-01-02 14:38, Stanley Chu wrote:
>> Hi Can,
>> 
>> On Tue, 2019-12-31 at 16:35 +0800, Can Guo wrote:
>> 
>>> Hi Stanley,
>>> 
>>> I missed this mail before I hit send. In current code, as per my
>>> understanding,
>>> UFS device's power state should be Active after ufshcd_link_startup()
>>> returns.
>>> If I am wrong, please feel free to correct me.
>>> 
>> 
>> Yes, this assumption of ufshcd_probe_hba() is true so I will drop this
>> patch.
>> Thanks for remind.
>> 
>>> Due to you are almost trying to revert commit 7caf489b99a42a, I am 
>>> just
>>> wondering
>>> if you encounter failure/error caused by it.
>> 
>> Yes, we actually have some doubts from the commit message of "scsi: 
>> ufs:
>> issue link startup 2 times if device isn't active"
>> 
>> If we configured system suspend as device=PowerDown/Link=LinkDown 
>> mode,
>> during resume, the 1st link startup will be successful, and after that
>> device could be accessed normally so it shall be already in Active 
>> power
>> mode. We did not find devices which need twice linkup for normal work.
>> 
>> And because the 1st linkup is OK, the forced 2nd linkup by commit 
>> "scsi:
>> ufs: issue link startup 2 times if device isn't active" leads to link
>> lost and finally the 3rd linkup is made again by retry mechanism in
>> ufshcd_link_startup() and be successful. So a linkup performance issue
>> is introduced here: We actually need one-time linkup only but finally
>> got 3 linkup operations.
>> 
>> According to the UFS spec, all reset types (including POR and Host
>> UniPro Warm Reset which both may happen in above configurations) other
>> than LU reset, UFS device power mode shall return to Sleep mode or
>> Active mode depending on bInitPowerMode, by default, it's Active mode.
>> 
>> So we are curious that why enforcing twice linkup is necessary here?
>> Could you kindly help us clarify this?
>> 
>> If anything wrong in above description, please feel free to correct 
>> me.
>> 
> 
> Hi Stanley,
> 
> Above description is correct. The reason why the UFS device becomes
> Active after the 1st link startup in your experiment is due to you
> set spm_lvl to 5, during system suspend, UFS device is powered down.
> When resume kicks start, the UFS device is power cycled once.
> 
> Moreover, if you set rpm_lvl to 5, during runtime suspend, if bkops is
> enabled, the UFS device will not be powered off, meaning when runtime
> resume kicks start, the UFS device is not power cycled, in this case,
> we need 3 times of link startup.
> 
> Does above explain?
> 
> Thanks,
> 
> Can Guo.
> 

Hi Stanley,

Sorry, typo before. I meant if set rpm_lvl/spm_lvl to 5, during suspend,
if is_lu_power_on_wp is set, the UFS device will not be fully powered 
off
(only VCC is down), meaning when resume kicks start, the UFS device is 
not
power cycled, in this case, we need 3 times of link startup.

Regards,

Can Guo.

>>> 
>>> Happy new year to you too!
>>> 
>>> Thanks,
>>> 
>>> Can Guo
>> 
>> Thanks,
>> 
>> Stanley
>> 
>>> 
>>> _______________________________________________
>>> Linux-mediatek mailing list
>>> Linux-mediatek@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v1 1/2] scsi: ufs: set device as default active power mode during initialization only

[Stanley Chu]

Hi Can,

On Fri, 2020-01-03 at 13:28 +0800, Can Guo wrote:
> > 
> > Hi Stanley,
> > 
> > Above description is correct. The reason why the UFS device becomes
> > Active after the 1st link startup in your experiment is due to you
> > set spm_lvl to 5, during system suspend, UFS device is powered down.
> > When resume kicks start, the UFS device is power cycled once.
> > 
> > Moreover, if you set rpm_lvl to 5, during runtime suspend, if bkops is
> > enabled, the UFS device will not be powered off, meaning when runtime
> > resume kicks start, the UFS device is not power cycled, in this case,
> > we need 3 times of link startup.
> > 
> > Does above explain?
> > 
> > Thanks,
> > 
> > Can Guo.
> > 
> 
> Hi Stanley,
> 
> Sorry, typo before. I meant if set rpm_lvl/spm_lvl to 5, during suspend,
> if is_lu_power_on_wp is set, the UFS device will not be fully powered 
> off
> (only VCC is down), meaning when resume kicks start, the UFS device is 
> not
> power cycled, in this case, we need 3 times of link startup.
> 
> Regards,
> 
> Can Guo.

Hi Can,

Very sorry for late responding this thread.

Now I would like to focus on 3-times link startup behavior found in our
platform because this dramatically downgrade resume performance.

According to your description, then could the driver set
"link_startup_again" as true only if "is_lu_power_on_wp" is set to avoid
unnecessary three-times link startup in other cases?

In addition, for the scenario you mentioned: "the UFS device will not be
fully powered off (only VCC is down), meaning when resume kicks start,
the UFS device is not power cycled": 

1. Actually I tried to set xpm_lvl=5, and enforced "is_lu_power_on_wp"
as true for evaluation, but found device can be still back to Active
PowerMode (can be accessed normally) after one-time link startup after
resumed. 

Only VCC is down and VCCQ2 is kept during suspend in my evaluation.

2. In this scenario, during resume the peer device shall have "UniPro
Warm Reset" triggered by the first link start-up and then device power
mode shall become Active according to UFS specification.

So what device power mode did you see after the first link startup in
this scenario? Or any other conditions to make device need
"link_startup_again"?

Thanks,
Stanley

RE: [PATCH v1 1/2] scsi: ufs: set device as default active power mode during initialization only

[Avri Altman]

Hi Stanley,
I am aware that this discussion is already concluded,
Just pointing out a small issue that might ease your mind further.

Thanks,
Avri

> 
> Hi Can,
> 
> On Tue, 2019-12-31 at 16:35 +0800, Can Guo wrote:
> 
> > Hi Stanley,
> >
> > I missed this mail before I hit send. In current code, as per my
> > understanding, UFS device's power state should be Active after
> > ufshcd_link_startup() returns.
> > If I am wrong, please feel free to correct me.
> >
> 
> Yes, this assumption of ufshcd_probe_hba() is true so I will drop this patch.
> Thanks for remind.
> 
> > Due to you are almost trying to revert commit 7caf489b99a42a, I am
> > just wondering if you encounter failure/error caused by it.
> 
> Yes, we actually have some doubts from the commit message of "scsi: ufs:
> issue link startup 2 times if device isn't active"
> 
> If we configured system suspend as device=PowerDown/Link=LinkDown mode,
> during resume, the 1st link startup will be successful, and after that device could
> be accessed normally so it shall be already in Active power mode. We did not
> find devices which need twice linkup for normal work.
> 
> And because the 1st linkup is OK, the forced 2nd linkup by commit "scsi:
> ufs: issue link startup 2 times if device isn't active" leads to link lost and finally
> the 3rd linkup is made again by retry mechanism in
> ufshcd_link_startup() and be successful. So a linkup performance issue is
> introduced here: We actually need one-time linkup only but finally got 3 linkup
> operations.
> 
> According to the UFS spec, all reset types (including POR and Host UniPro Warm
> Reset which both may happen in above configurations) other than LU reset, UFS
> device power mode shall return to Sleep mode or Active mode depending on
> bInitPowerMode, by default, it's Active mode.
As for bInitPowerMode - please see the discussion in https://www.mail-archive.com/linux-scsi@vger.kernel.org/msg78262.html



> 
> So we are curious that why enforcing twice linkup is necessary here?
> Could you kindly help us clarify this?

Re: [PATCH v1 1/2] scsi: ufs: set device as default active power mode during initialization only

[Can Guo]

On 2019-12-31 12:22, Stanley Chu wrote:
> Hi Can,
> 
> 
>> Hi Stanley,
>> 
>> I see skipping ufshcd_set_ufs_dev_active() in ufshcd_probe_hba()
>> if it is called from ufshcd_resume() path is the purpose here.
>> 
>> If so, then ufshcd_set_dev_pwr_mode() would be called, meaning
>> SSU command will be sent. Why is this SSU command needed to be
>> sent after a full host reset and restore? Is ufshcd_probe_hba()
>> not enough to make UFS device fully functional?
> 
> After resume (for both runtime resume and system resume), device power
> mode shall be back to "Active" to service incoming requests.
> 
> I see two cases need device power mode transition during resume flow
> 1. Device Power Mode = Sleep
> 2. Device Power Mode = PowerDown
> 
> For 1, ufshcd_probe_hba() is not invoked during resume flow,
> hba->curr_dev_pwr_mode = SLEEP, thus ufshcd_resume() can invoke
> ufshcd_set_dev_pwr_mode() to change device power mode.
> 
> For 2, ufshcd_probe_hba() is invoked during resume flow, before this
> fix, hba->curr_dev_pwr_mode will be set to ACTIVE (note that only this
> flag is set as ACTIVE, but device may be still in PowerDown state if
> device power is not fully shutdown or device HW reset is not executed
> before resume), in the end, ufshcd_resume() will not invoke
> ufshcd_set_dev_pwr_mode() to send SSU command to make device change 
> back
> to Active power mode.

Hi Stanley,

Isn't below change fixing the problem you are saying above?
With it, after ufshcd_link_startup(), UFS device's power mode will
become Active anyways. Do you mean below change is not working properly
and you are removing it?

commit 7caf489b99a42a9017ef3d733912aea8794677e7
Author: subhashj@codeaurora.org <subhashj@codeaurora.org>
Date:   Wed Nov 23 16:32:20 2016 -0800

     scsi: ufs: issue link starup 2 times if device isn't active

     If we issue the link startup to the device while its UniPro state is
     LinkDown (and device state is sleep/power-down) then link startup
     will not move the device state to Active. Device will only move to
     active state if the link starup is issued when its UniPro state is
     LinkUp. So in this case, we would have to issue the link startup 2
     times to make sure that device moves to active state.

Thanks,

Can Guo

> 
> But if device is fully reset before resume (not by current mainstream
> driver), device can be already in "Active" power mode after power on
> again during resume flow. In this case, it is OK to set
> hba->curr_dev_pwr_mode as ACTIVE in ufshcd_probe_hba() and SSU command
> is not necessary.
> 
> Thanks,
> Stanley
> 
>> _______________________________________________
>> Linux-mediatek mailing list
>> Linux-mediatek@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH v1 2/2] scsi: ufs: remove link_startup_again flow in ufshcd_link_startup

[Stanley Chu]

link_startup_again flow in ufshcd_link_startup() is not necessary
since currently device can be moved to "active" power mode during
resume flow by commit as below,

"scsi: ufs: set device as default active power mode during
initialization only"

Fixes: 7caf489b99a4 (scsi: ufs: issue link starup 2 times if device isn't active)
Cc: Alim Akhtar <alim.akhtar@samsung.com>
Cc: Avri Altman <avri.altman@wdc.com>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Bean Huo <beanhuo@micron.com>
Cc: Can Guo <cang@codeaurora.org>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
Cc: Subhash Jadavani <subhashj@codeaurora.org>
Cc: stable@vger.kernel.org
Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufshcd.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 9abb7085a5d0..1900f811394a 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4365,16 +4365,7 @@ static int ufshcd_link_startup(struct ufs_hba *hba)
 {
 	int ret;
 	int retries = DME_LINKSTARTUP_RETRIES;
-	bool link_startup_again = false;
 
-	/*
-	 * If UFS device isn't active then we will have to issue link startup
-	 * 2 times to make sure the device state move to active.
-	 */
-	if (!ufshcd_is_ufs_dev_active(hba))
-		link_startup_again = true;
-
-link_startup:
 	do {
 		ufshcd_vops_link_startup_notify(hba, PRE_CHANGE);
 
@@ -4408,12 +4399,6 @@ static int ufshcd_link_startup(struct ufs_hba *hba)
 		goto out;
 	}
 
-	if (link_startup_again) {
-		link_startup_again = false;
-		retries = DME_LINKSTARTUP_RETRIES;
-		goto link_startup;
-	}
-
 	/* Mark that link is up in PWM-G1, 1-lane, SLOW-AUTO mode */
 	ufshcd_init_pwr_info(hba);
 	ufshcd_print_pwr_info(hba);
-- 
2.18.0