[PATCH 1/2] mwifiex: pcie: use posted write to wake up firmware

[PATCH 1/2] mwifiex: pcie: use posted write to wake up firmware

[Brian Norris]

Depending on system factors (e.g., the PCIe link PM state), the first
read to wake up the Wifi firmware can take a long time. There is no
reason to use a (blocking, non-posted) read at this point, so let's just
use a write instead. Write vs. read doesn't matter functionality-wise --
it's just a dummy operation.

This has been shown to decrease the time spent blocking in this function
on a Rockchip RK3399 SoC.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 66226c615be0..435ba879ef29 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -458,7 +458,6 @@ static void mwifiex_delay_for_sleep_cookie(struct mwifiex_adapter *adapter,
 /* This function wakes up the card by reading fw_status register. */
 static int mwifiex_pm_wakeup_card(struct mwifiex_adapter *adapter)
 {
-	u32 fw_status;
 	struct pcie_service_card *card = adapter->card;
 	const struct mwifiex_pcie_card_reg *reg = card->pcie.reg;
 
@@ -468,10 +467,10 @@ static int mwifiex_pm_wakeup_card(struct mwifiex_adapter *adapter)
 	if (reg->sleep_cookie)
 		mwifiex_pcie_dev_wakeup_delay(adapter);
 
-	/* Reading fw_status register will wakeup device */
-	if (mwifiex_read_reg(adapter, reg->fw_status, &fw_status)) {
+	/* Accessing fw_status register will wakeup device */
+	if (mwifiex_write_reg(adapter, reg->fw_status, 0)) {
 		mwifiex_dbg(adapter, ERROR,
-			    "Reading fw_status register failed\n");
+			    "Writing fw_status register failed\n");
 		return -1;
 	}
 
-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH 2/2] mwifiex: pcie: don't delay for sleep cookie when not required

[Brian Norris]

Wifi modules like 8997 don't support the "sleep cookie", and so most of
the time, we just time out in the mwifiex_delay_for_sleep_cookie()
function ("max count reached while accessing sleep cookie"). This is a
waste of time, and we should skip it for modules without the sleep
cookie flag.

Additionally, this delay is sometimes counterproductive. For instance,
when PCIe ASPM is enabled, this extra delay can leave the link idle for
long enough to re-enter a low-power state even while we are trying to
wake the module, compounding an additional delay when it comes time to
read the next register (e.g., the interrupt status). On some systems,
this is detrimental to overall system latency.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
Tested on Marvell 8997, but would be good to get confirmation from Marvell.

 drivers/net/wireless/marvell/mwifiex/pcie.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 435ba879ef29..11e0673617c7 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -1712,11 +1712,13 @@ static int mwifiex_pcie_process_cmd_complete(struct mwifiex_adapter *adapter)
 					    "Write register failed\n");
 				return -1;
 			}
-			mwifiex_delay_for_sleep_cookie(adapter,
-						       MWIFIEX_MAX_DELAY_COUNT);
-			while (reg->sleep_cookie && (count++ < 10) &&
-			       mwifiex_pcie_ok_to_access_hw(adapter))
-				usleep_range(50, 60);
+			if (reg->sleep_cookie) {
+				mwifiex_delay_for_sleep_cookie(adapter,
+							       MWIFIEX_MAX_DELAY_COUNT);
+				while ((count++ < 10) &&
+				       mwifiex_pcie_ok_to_access_hw(adapter))
+					usleep_range(50, 60);
+			}
 			mwifiex_pcie_enable_host_int(adapter);
 			mwifiex_process_sleep_confirm_resp(adapter, skb->data,
 							   skb->len);
-- 
2.11.0.390.gc69c2f50cf-goog

Re: [PATCH 2/2] mwifiex: pcie: don't delay for sleep cookie when not required

[Brian Norris]

On Thu, Jan 12, 2017 at 01:02:32PM -0800, Brian Norris wrote:
> Wifi modules like 8997 don't support the "sleep cookie", and so most of
> the time, we just time out in the mwifiex_delay_for_sleep_cookie()
> function ("max count reached while accessing sleep cookie"). This is a
> waste of time, and we should skip it for modules without the sleep
> cookie flag.
> 
> Additionally, this delay is sometimes counterproductive. For instance,
> when PCIe ASPM is enabled, this extra delay can leave the link idle for
> long enough to re-enter a low-power state even while we are trying to
> wake the module, compounding an additional delay when it comes time to
> read the next register (e.g., the interrupt status). On some systems,
> this is detrimental to overall system latency.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> Tested on Marvell 8997, but would be good to get confirmation from Marvell.

It would still be good to get comment from Marvell here, but elsewhere,
they've told me that this breaks the expected handshake procedure. I'm
still not quite sure how that is true, considering that we time out in
the mwifiex_delay_for_sleep_cookie() all the time anyway (so what's the
point of waiting then?)...

But anyway I think I have discovered a proper root cause [1] that is
causing my latency problems above. I'll post a v2 which replaces the
current patch with something else.

Brian

[1] The short version: re-reading the interrupt status register from the
card after we've sent it to sleep takes a long time. We shouldn't do
that.

>  drivers/net/wireless/marvell/mwifiex/pcie.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index 435ba879ef29..11e0673617c7 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -1712,11 +1712,13 @@ static int mwifiex_pcie_process_cmd_complete(struct mwifiex_adapter *adapter)
>  					    "Write register failed\n");
>  				return -1;
>  			}
> -			mwifiex_delay_for_sleep_cookie(adapter,
> -						       MWIFIEX_MAX_DELAY_COUNT);
> -			while (reg->sleep_cookie && (count++ < 10) &&
> -			       mwifiex_pcie_ok_to_access_hw(adapter))
> -				usleep_range(50, 60);
> +			if (reg->sleep_cookie) {
> +				mwifiex_delay_for_sleep_cookie(adapter,
> +							       MWIFIEX_MAX_DELAY_COUNT);
> +				while ((count++ < 10) &&
> +				       mwifiex_pcie_ok_to_access_hw(adapter))
> +					usleep_range(50, 60);
> +			}
>  			mwifiex_pcie_enable_host_int(adapter);
>  			mwifiex_process_sleep_confirm_resp(adapter, skb->data,
>  							   skb->len);
> -- 
> 2.11.0.390.gc69c2f50cf-goog
> 

Re: [PATCH 1/2] mwifiex: pcie: use posted write to wake up firmware

[Brian Norris]

On Thu, Jan 12, 2017 at 01:02:31PM -0800, Brian Norris wrote:
> Depending on system factors (e.g., the PCIe link PM state), the first
> read to wake up the Wifi firmware can take a long time. There is no
> reason to use a (blocking, non-posted) read at this point, so let's just
> use a write instead. Write vs. read doesn't matter functionality-wise --
> it's just a dummy operation.
> 
> This has been shown to decrease the time spent blocking in this function
> on a Rockchip RK3399 SoC.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
>  drivers/net/wireless/marvell/mwifiex/pcie.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index 66226c615be0..435ba879ef29 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -458,7 +458,6 @@ static void mwifiex_delay_for_sleep_cookie(struct mwifiex_adapter *adapter,
>  /* This function wakes up the card by reading fw_status register. */
>  static int mwifiex_pm_wakeup_card(struct mwifiex_adapter *adapter)
>  {
> -	u32 fw_status;
>  	struct pcie_service_card *card = adapter->card;
>  	const struct mwifiex_pcie_card_reg *reg = card->pcie.reg;
>  
> @@ -468,10 +467,10 @@ static int mwifiex_pm_wakeup_card(struct mwifiex_adapter *adapter)
>  	if (reg->sleep_cookie)
>  		mwifiex_pcie_dev_wakeup_delay(adapter);
>  
> -	/* Reading fw_status register will wakeup device */
> -	if (mwifiex_read_reg(adapter, reg->fw_status, &fw_status)) {
> +	/* Accessing fw_status register will wakeup device */
> +	if (mwifiex_write_reg(adapter, reg->fw_status, 0)) {

As Amit noted to me elsewhere, the firmware only writes this status once
at FW init time to FIRMWARE_READY_PCIE, and we later check it in a few
places. So I noticed that this actually breaks re-probing the adapter
(e.g., 'rmmod mwifiex_pcie; modprobe mwifiex_pcie'); the second time,
we'll fail to find the FIRMWARE_READY_PCIE signature, and so we'll
abort.

I'll resend this patch with s/0/FIRMWARE_READY_PCIE/ instead.

Brian

>  		mwifiex_dbg(adapter, ERROR,
> -			    "Reading fw_status register failed\n");
> +			    "Writing fw_status register failed\n");
>  		return -1;
>  	}
>  
> -- 
> 2.11.0.390.gc69c2f50cf-goog
>