Re: [RFT 6/6] wlcore: Use generic runtime pm calls for wowlan elp configuration

Re: [RFT 6/6] wlcore: Use generic runtime pm calls for wowlan elp configuration

[Reizer, Eyal]

> > >
> > > With runtime PM enabled, we can now use generic calls to
> > > pm_generic_runtime_suspend and pm_generic_runtime_resume for
> enabling elp
> > > during suspend when wowlan is enabled and waking the chip from elp
> > > on resume.
> >
> > Sry, but not sure you can :(
> >
> > These functions are not used by drivers directly because system suspend
> > are not synchronized with PM runtime, so if you call
> pm_generic_runtime_suspend()
> > and, at the same time, there is pm_runtime_get_() in progress --> race ...
> >
> > The pm_runtime_force_() APIs have to be used, or
> > PM runtime drivers functions can be called directly, but only if it possible to
> be
> > sure no other PM runtime calls active which is usually true at
> suspend_noirq stage.
> 
> Oh right, those are subsystem calls. Seems like
> pm_runtime_force_suspend/resume() should work here,
> Eyal?
> 

Oh, nice, wasn't aware of the pm_runtime_force_() calls.
For some reason they are not documented in:
https://www.kernel.org/doc/Documentation/power/runtime_pm.txt
Perhaps they should be?

Anyway I have tried them instead of pm_generic_runtime_() and they seem
To work fine on my platform.
Will test some more and submit a v2.

Best Regards,
Eyal

Re: [RFT 6/6] wlcore: Use generic runtime pm calls for wowlan elp configuration

[Tony Lindgren]

* Reizer, Eyal <eyalr-l0cyMroinI0@public.gmane.org> [180530 06:37]:
> > > >
> > > > With runtime PM enabled, we can now use generic calls to
> > > > pm_generic_runtime_suspend and pm_generic_runtime_resume for
> > enabling elp
> > > > during suspend when wowlan is enabled and waking the chip from elp
> > > > on resume.
> > >
> > > Sry, but not sure you can :(
> > >
> > > These functions are not used by drivers directly because system suspend
> > > are not synchronized with PM runtime, so if you call
> > pm_generic_runtime_suspend()
> > > and, at the same time, there is pm_runtime_get_() in progress --> race ...
> > >
> > > The pm_runtime_force_() APIs have to be used, or
> > > PM runtime drivers functions can be called directly, but only if it possible to
> > be
> > > sure no other PM runtime calls active which is usually true at
> > suspend_noirq stage.
> > 
> > Oh right, those are subsystem calls. Seems like
> > pm_runtime_force_suspend/resume() should work here,
> > Eyal?
> > 
> 
> Oh, nice, wasn't aware of the pm_runtime_force_() calls.
> For some reason they are not documented in:
> https://www.kernel.org/doc/Documentation/power/runtime_pm.txt
> Perhaps they should be?

Sure why not submit a patch for that.

> Anyway I have tried them instead of pm_generic_runtime_() and they seem
> To work fine on my platform.
> Will test some more and submit a v2.

OK I'll your pick v2 into my series.

Regards,

Tony

[RFTv3 0/6] Runtime PM support for wlcore

[Tony Lindgren]

Hi all,

Here's the third version of wlcore runtime PM changes. Things
seem to be working for me just fine now after few fixes listed
below. Please do test again though, and then I'll post this
series and one more patch after v4.18-rc1 to enable runtime PM
autosuspend support. So hopefully we can have these merged early
on for v4.19.

For testing, please make sure you have also applied patch
"[PATCHv2] wlcore: sdio: Fix flakey SDIO runtime PM handling"
to avoid bogus errors.

Regards,

Tony

Changes since v2:

- Add fix "wclore: Fix timout errors after recovery" that is not needed
  before runtime PM conversion

- Add fix from Eyal for "wlcore: Use generic runtime pm calls for wowlan
  elp configuration" that is also not needed before runtime PM conversion

- Return early from wlcore_runtime_resume() on ELP timeout to avoid
  clearing WL1271_FLAG_IN_ELP bit

- Tag as RFT as we still need to do more testing and add runtime PM
  autosuspend support before merging

- Drop "wlcore: sdio: Warn about runtime PM suspend errors" that should
  no longer be needed

Changes since v1:

- Fix issues reported by Eyal for recovery

- Add few patches for enable/disable issues found when using runtime PM

- Remove unused ps.h includes


Eyal Reizer (1):
  wlcore: Use generic runtime pm calls for wowlan elp configuration

Tony Lindgren (5):
  wlcore: Add missing PM call for wlcore_cmd_wait_for_event_or_timeout()
  wlcore: Make sure PM calls are paired
  wlcore: Add support for runtime PM
  wlcore: Fix misplaced PM call for scan_complete_work()
  wclore: Fix timout errors after recovery

 drivers/net/wireless/ti/wl18xx/debugfs.c    |  26 +-
 drivers/net/wireless/ti/wlcore/acx.c        |   1 -
 drivers/net/wireless/ti/wlcore/cmd.c        |   9 +
 drivers/net/wireless/ti/wlcore/debugfs.c    |  79 ++--
 drivers/net/wireless/ti/wlcore/main.c       | 464 +++++++++++++-------
 drivers/net/wireless/ti/wlcore/ps.c         | 146 ------
 drivers/net/wireless/ti/wlcore/ps.h         |   3 -
 drivers/net/wireless/ti/wlcore/scan.c       |  12 +-
 drivers/net/wireless/ti/wlcore/sysfs.c      |  12 +-
 drivers/net/wireless/ti/wlcore/testmode.c   |  18 +-
 drivers/net/wireless/ti/wlcore/tx.c         |   9 +-
 drivers/net/wireless/ti/wlcore/vendor_cmd.c |  27 +-
 drivers/net/wireless/ti/wlcore/wlcore.h     |   1 -
 drivers/net/wireless/ti/wlcore/wlcore_i.h   |   1 -
 14 files changed, 436 insertions(+), 372 deletions(-)

-- 
2.17.0

[RFT 6/6] wlcore: Use generic runtime pm calls for wowlan elp configuration

[Tony Lindgren]

From: Eyal Reizer <eyalreizer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

With runtime PM enabled, we can now use generic calls to
pm_generic_runtime_suspend and pm_generic_runtime_resume for enabling elp
during suspend when wowlan is enabled and waking the chip from elp
on resume.

Remove the custom API that was used to ensure that the command
that is used to allow ELP during suspend is completed before the system
suspend.

Signed-off-by: Eyal Reizer <eyalr-l0cyMroinI0@public.gmane.org>
Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
---
 drivers/net/wireless/ti/wlcore/main.c | 51 +++++++--------------------
 1 file changed, 13 insertions(+), 38 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/main.c b/drivers/net/wireless/ti/wlcore/main.c
--- a/drivers/net/wireless/ti/wlcore/main.c
+++ b/drivers/net/wireless/ti/wlcore/main.c
@@ -998,24 +998,6 @@ static int wlcore_fw_wakeup(struct wl1271 *wl)
 	return wlcore_raw_write32(wl, HW_ACCESS_ELP_CTRL_REG, ELPCTRL_WAKE_UP);
 }
 
-static int wlcore_fw_sleep(struct wl1271 *wl)
-{
-	int ret;
-
-	mutex_lock(&wl->mutex);
-	ret = wlcore_raw_write32(wl, HW_ACCESS_ELP_CTRL_REG, ELPCTRL_SLEEP);
-	if (ret < 0) {
-		wl12xx_queue_recovery_work(wl);
-		goto out;
-	}
-	set_bit(WL1271_FLAG_IN_ELP, &wl->flags);
-out:
-	mutex_unlock(&wl->mutex);
-	mdelay(WL1271_SUSPEND_SLEEP);
-
-	return 0;
-}
-
 static int wl1271_setup(struct wl1271 *wl)
 {
 	wl->raw_fw_status = kzalloc(wl->fw_status_len, GFP_KERNEL);
@@ -1738,6 +1720,7 @@ static int __maybe_unused wl1271_op_suspend(struct ieee80211_hw *hw,
 {
 	struct wl1271 *wl = hw->priv;
 	struct wl12xx_vif *wlvif;
+	unsigned long flags;
 	int ret;
 
 	wl1271_debug(DEBUG_MAC80211, "mac80211 suspend wow=%d", !!wow);
@@ -1796,19 +1779,6 @@ static int __maybe_unused wl1271_op_suspend(struct ieee80211_hw *hw,
 	/* flush any remaining work */
 	wl1271_debug(DEBUG_MAC80211, "flushing remaining works");
 
-	/*
-	 * disable and re-enable interrupts in order to flush
-	 * the threaded_irq
-	 */
-	wlcore_disable_interrupts(wl);
-
-	/*
-	 * set suspended flag to avoid triggering a new threaded_irq
-	 * work. no need for spinlock as interrupts are disabled.
-	 */
-	set_bit(WL1271_FLAG_SUSPENDED, &wl->flags);
-
-	wlcore_enable_interrupts(wl);
 	flush_work(&wl->tx_work);
 
 	/*
@@ -1818,15 +1788,14 @@ static int __maybe_unused wl1271_op_suspend(struct ieee80211_hw *hw,
 	cancel_delayed_work(&wl->tx_watchdog_work);
 
 	/*
-	 * Use an immediate call for allowing the firmware to go into power
-	 * save during suspend.
-	 * Using a workque for this last write was only hapenning on resume
-	 * leaving the firmware with power save disabled during suspend,
-	 * while consuming full power during wowlan suspend.
+	 * set suspended flag to avoid triggering a new threaded_irq
+	 * work.
 	 */
-	wlcore_fw_sleep(wl);
+	spin_lock_irqsave(&wl->wl_lock, flags);
+	set_bit(WL1271_FLAG_SUSPENDED, &wl->flags);
+	spin_unlock_irqrestore(&wl->wl_lock, flags);
 
-	return 0;
+	return pm_generic_runtime_suspend(wl->dev);
 }
 
 static int __maybe_unused wl1271_op_resume(struct ieee80211_hw *hw)
@@ -1841,6 +1810,12 @@ static int __maybe_unused wl1271_op_resume(struct ieee80211_hw *hw)
 		     wl->wow_enabled);
 	WARN_ON(!wl->wow_enabled);
 
+	ret = pm_generic_runtime_resume(wl->dev);
+	if (ret < 0) {
+		wl1271_error("ELP wakeup failure!");
+		goto out_sleep;
+	}
+
 	/*
 	 * re-enable irq_work enqueuing, and call irq_work directly if
 	 * there is a pending work.
-- 
2.17.0

Re: [RFT 6/6] wlcore: Use generic runtime pm calls for wowlan elp configuration

[Grygorii Strashko]

On 05/29/2018 01:06 PM, Tony Lindgren wrote:
> From: Eyal Reizer <eyalreizer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> With runtime PM enabled, we can now use generic calls to
> pm_generic_runtime_suspend and pm_generic_runtime_resume for enabling elp
> during suspend when wowlan is enabled and waking the chip from elp
> on resume.

Sry, but not sure you can :(

These functions are not used by drivers directly because system suspend
are not synchronized with PM runtime, so if you call pm_generic_runtime_suspend()
and, at the same time, there is pm_runtime_get_() in progress --> race ...

The pm_runtime_force_() APIs have to be used, or 
PM runtime drivers functions can be called directly, but only if it possible to be
sure no other PM runtime calls active which is usually true at suspend_noirq stage. 

> 
> Remove the custom API that was used to ensure that the command
> that is used to allow ELP during suspend is completed before the system
> suspend.
> 
> Signed-off-by: Eyal Reizer <eyalr-l0cyMroinI0@public.gmane.org>
> Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> ---
>   drivers/net/wireless/ti/wlcore/main.c | 51 +++++++--------------------
>   1 file changed, 13 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/net/wireless/ti/wlcore/main.c b/drivers/net/wireless/ti/wlcore/main.c
> --- a/drivers/net/wireless/ti/wlcore/main.c
> +++ b/drivers/net/wireless/ti/wlcore/main.c
> @@ -998,24 +998,6 @@ static int wlcore_fw_wakeup(struct wl1271 *wl)
>   	return wlcore_raw_write32(wl, HW_ACCESS_ELP_CTRL_REG, ELPCTRL_WAKE_UP);
>   }
>   
> -static int wlcore_fw_sleep(struct wl1271 *wl)

[...]

-- 
regards,
-grygorii

Re: [RFT 6/6] wlcore: Use generic runtime pm calls for wowlan elp configuration

[Tony Lindgren]

* Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> [180529 19:25]:
> On 05/29/2018 01:06 PM, Tony Lindgren wrote:
> > From: Eyal Reizer <eyalreizer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > 
> > With runtime PM enabled, we can now use generic calls to
> > pm_generic_runtime_suspend and pm_generic_runtime_resume for enabling elp
> > during suspend when wowlan is enabled and waking the chip from elp
> > on resume.
> 
> Sry, but not sure you can :(
> 
> These functions are not used by drivers directly because system suspend
> are not synchronized with PM runtime, so if you call pm_generic_runtime_suspend()
> and, at the same time, there is pm_runtime_get_() in progress --> race ...
> 
> The pm_runtime_force_() APIs have to be used, or 
> PM runtime drivers functions can be called directly, but only if it possible to be
> sure no other PM runtime calls active which is usually true at suspend_noirq stage. 

Oh right, those are subsystem calls. Seems like
pm_runtime_force_suspend/resume() should work here,
Eyal?

Regards,

Tony