Hi Bart
On 7/21/22 5:40 AM, Bart Van Assche wrote:
On 7/19/22 06:02, peter.wang@xxxxxxxxxxxx wrote:
From: Peter Wang <peter.wang@xxxxxxxxxxxx>
After ufshcd_wl_shutdown set device poweroff and link off,
ufshcd_shutdown not turn off regulators/clocks.
Correct the flow to wait ufshcd_wl_shutdown done and turn off
regulators/clocks by polling ufs device/link state 500ms.
Also remove pm_runtime_get_sync because it is unnecessary.
Please explain in the patch description why the pm_runtime_get_sync()
call is not necessary.
Because shutdown is focus on turn off clock/power, we don't need turn
on(resume) and turn off, right?
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index c7b337480e3e..1c11af48b584 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -9461,10 +9461,14 @@ EXPORT_SYMBOL(ufshcd_runtime_resume);
*/
int ufshcd_shutdown(struct ufs_hba *hba)
{
- if (ufshcd_is_ufs_dev_poweroff(hba) && ufshcd_is_link_off(hba))
- goto out;
+ ktime_t timeout = ktime_add_ms(ktime_get(), 500);
Where does the 500 ms timeout come from?
It is a time to wait device into power off, if the 500 ms is not
suitable, could you suggess a value?
Additionally, given the large timeout, please use jiffies instead of
ktime_get().
Okay, will change next version.
- pm_runtime_get_sync(hba->dev);
+ /* Wait ufshcd_wl_shutdown clear ufs state, timeout 500 ms */
+ while (!ufshcd_is_ufs_dev_poweroff(hba) ||
!ufshcd_is_link_off(hba)) {
+ if (ktime_after(ktime_get(), timeout))
+ goto out;
+ msleep(1);
+ }
Please explain why this wait loop has been introduced.
Both ufshcd_shtdown and ufshcd_wl_shutdown could run concurrently.
if ufshcd_wl_shutdown -> ufshcd_shtdown, clock/power off should ok.
If ufshcd_shtdown -> ufshcd_wl_shutdown, wait ufshcd_wl_shutdown set
device to power off and turn off clock/power.
If timeout happen, means device still in active mode, cannot turn off
clock/power directly. Skip and keep clock/power on in this case.
Thanks,
Bart.
