[PATCH] wcn36xx: Set BTLE coexistence related configuration values to defaults

[PATCH] wcn36xx: Set BTLE coexistence related configuration values to defaults

[Ramon Fried]

From: Eyal Ilsar <eilsar@codeaurora.org>

If the value for the firmware configuration parameters BTC_STATIC_LEN_LE_BT
and BTC_STATIC_LEN_LE_WLAN are not set the duty cycle between BT and WLAN
is such that if BT (including BLE) is active WLAN gets 0 bandwidth.
When tuning these parameters having a too high value for WLAN means that BLE performance degrades.
The "sweet" point of roughly half of the maximal values was empirically found to achieve
a balance between BLE and Wi-Fi coexistence performance.

Signed-off-by: Eyal Ilsar <eilsar@codeaurora.org>
Signed-off-by: Ramon Fried <rfried@codeaurora.org>
---
 drivers/net/wireless/ath/wcn36xx/smd.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index 9c6590d..1c75987 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -72,8 +72,10 @@ struct wcn36xx_cfg_val {
 	WCN36XX_CFG_VAL(DYNAMIC_PS_POLL_VALUE, 0),
 	WCN36XX_CFG_VAL(TX_PWR_CTRL_ENABLE, 1),
 	WCN36XX_CFG_VAL(ENABLE_CLOSE_LOOP, 1),
-	WCN36XX_CFG_VAL(ENABLE_LPWR_IMG_TRANSITION, 0),
+	WCN36XX_CFG_VAL(BTC_STATIC_LEN_LE_BT, 120000),
+	WCN36XX_CFG_VAL(BTC_STATIC_LEN_LE_WLAN, 30000),
 	WCN36XX_CFG_VAL(MAX_ASSOC_LIMIT, 10),
+	WCN36XX_CFG_VAL(ENABLE_LPWR_IMG_TRANSITION, 0),
 	WCN36XX_CFG_VAL(ENABLE_MCC_ADAPTIVE_SCHEDULER, 0),
 };
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] wcn36xx: Set BTLE coexistence related configuration values to defaults

[Bjorn Andersson]

On Sun 12 Nov 06:21 PST 2017, Ramon Fried wrote:

> From: Eyal Ilsar <eilsar@codeaurora.org>
> 
> If the value for the firmware configuration parameters BTC_STATIC_LEN_LE_BT
> and BTC_STATIC_LEN_LE_WLAN are not set the duty cycle between BT and WLAN
> is such that if BT (including BLE) is active WLAN gets 0 bandwidth.
> When tuning these parameters having a too high value for WLAN means that BLE performance degrades.
> The "sweet" point of roughly half of the maximal values was empirically found to achieve
> a balance between BLE and Wi-Fi coexistence performance.
> 

Thanks for the patch! Just some minor comments.

Please limit subject to 50 chars and wrap body at 72 chars.

> Signed-off-by: Eyal Ilsar <eilsar@codeaurora.org>
> Signed-off-by: Ramon Fried <rfried@codeaurora.org>
> ---
>  drivers/net/wireless/ath/wcn36xx/smd.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
> index 9c6590d..1c75987 100644
> --- a/drivers/net/wireless/ath/wcn36xx/smd.c
> +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
> @@ -72,8 +72,10 @@ struct wcn36xx_cfg_val {
>  	WCN36XX_CFG_VAL(DYNAMIC_PS_POLL_VALUE, 0),
>  	WCN36XX_CFG_VAL(TX_PWR_CTRL_ENABLE, 1),
>  	WCN36XX_CFG_VAL(ENABLE_CLOSE_LOOP, 1),
> -	WCN36XX_CFG_VAL(ENABLE_LPWR_IMG_TRANSITION, 0),

I don't see a need for moving this line.

> +	WCN36XX_CFG_VAL(BTC_STATIC_LEN_LE_BT, 120000),
> +	WCN36XX_CFG_VAL(BTC_STATIC_LEN_LE_WLAN, 30000),

These looks reasonable, are we okay leaving the other coexistence
properties at their preconfigured values?

>  	WCN36XX_CFG_VAL(MAX_ASSOC_LIMIT, 10),
> +	WCN36XX_CFG_VAL(ENABLE_LPWR_IMG_TRANSITION, 0),
>  	WCN36XX_CFG_VAL(ENABLE_MCC_ADAPTIVE_SCHEDULER, 0),

Regards,
Bjorn

Re: [PATCH] wcn36xx: Set BTLE coexistence related configuration values to defaults

[Kalle Valo]

Ramon Fried <rfried@codeaurora.org> writes:

> From: Eyal Ilsar <eilsar@codeaurora.org>
>
> If the value for the firmware configuration parameters
> BTC_STATIC_LEN_LE_BT and BTC_STATIC_LEN_LE_WLAN are not set the duty
> cycle between BT and WLAN is such that if BT (including BLE) is active
> WLAN gets 0 bandwidth. When tuning these parameters having a too high
> value for WLAN means that BLE performance degrades. The "sweet" point
> of roughly half of the maximal values was empirically found to achieve
> a balance between BLE and Wi-Fi coexistence performance.
>
> Signed-off-by: Eyal Ilsar <eilsar@codeaurora.org>
> Signed-off-by: Ramon Fried <rfried@codeaurora.org>

Then submit a new version of the patch then please include the version
number:

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#patch_version_missing

So after fixing Bjorn's comments the next version should be v3.

-- 
Kalle Valo

Re: [PATCH] wcn36xx: Set BTLE coexistence related configuration values to defaults

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1390 bytes --]

Hi Eyal,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on wireless-drivers-next/master]
[also build test ERROR on v4.14 next-20171114]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ramon-Fried/wcn36xx-Set-BTLE-coexistence-related-configuration-values-to-defaults/20171114-194715
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git master
config: i386-randconfig-x019-201746 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

>> drivers/net/wireless/ath/wcn36xx/main.c:30:33: error: 'WCN36XX_DBG_NONE' undeclared here (not in a function)
    unsigned int wcn36xx_dbg_mask = WCN36XX_DBG_NONE;
                                    ^~~~~~~~~~~~~~~~

vim +/WCN36XX_DBG_NONE +30 drivers/net/wireless/ath/wcn36xx/main.c

    29	
  > 30	unsigned int wcn36xx_dbg_mask = WCN36XX_DBG_NONE;
    31	module_param_named(debug_mask, wcn36xx_dbg_mask, uint, 0644);
    32	MODULE_PARM_DESC(debug_mask, "Debugging mask");
    33	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32975 bytes --]

Re: [PATCH] wcn36xx: Set BTLE coexistence related configuration values to defaults

[Kalle Valo]

Ramon Fried <rfried@codeaurora.org> writes:

> From: Eyal Ilsar <eilsar@codeaurora.org>
>
> Signed-off-by: Eyal Ilsar <eilsar@codeaurora.org>
> Signed-off-by: Ramon Fried <rfried@codeaurora.org>
> ---
>  drivers/net/wireless/ath/wcn36xx/main.c | 2 +-
>  drivers/net/wireless/ath/wcn36xx/smd.c  | 4 +++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c
> index b83f01d..0d4ed41 100644
> --- a/drivers/net/wireless/ath/wcn36xx/main.c
> +++ b/drivers/net/wireless/ath/wcn36xx/main.c
> @@ -27,7 +27,7 @@
>  #include <linux/soc/qcom/wcnss_ctrl.h>
>  #include "wcn36xx.h"
>  
> -unsigned int wcn36xx_dbg_mask;
> +unsigned int wcn36xx_dbg_mask = WCN36XX_DBG_NONE;

And this looks unrelated to the title?

-- 
Kalle Valo

Re: [PATCH] wcn36xx: Set BTLE coexistence related configuration values to defaults

[Kalle Valo]

Ramon Fried <rfried@codeaurora.org> writes:

> From: Eyal Ilsar <eilsar@codeaurora.org>
>
> Signed-off-by: Eyal Ilsar <eilsar@codeaurora.org>
> Signed-off-by: Ramon Fried <rfried@codeaurora.org>

Why? No empty commit logs, please.

-- 
Kalle Valo

[PATCH] wcn36xx: Set BTLE coexistence related configuration values to defaults

[Ramon Fried]

From: Eyal Ilsar <eilsar@codeaurora.org>

Signed-off-by: Eyal Ilsar <eilsar@codeaurora.org>
Signed-off-by: Ramon Fried <rfried@codeaurora.org>
---
 drivers/net/wireless/ath/wcn36xx/main.c | 2 +-
 drivers/net/wireless/ath/wcn36xx/smd.c  | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c
index b83f01d..0d4ed41 100644
--- a/drivers/net/wireless/ath/wcn36xx/main.c
+++ b/drivers/net/wireless/ath/wcn36xx/main.c
@@ -27,7 +27,7 @@
 #include <linux/soc/qcom/wcnss_ctrl.h>
 #include "wcn36xx.h"
 
-unsigned int wcn36xx_dbg_mask;
+unsigned int wcn36xx_dbg_mask = WCN36XX_DBG_NONE;
 module_param_named(debug_mask, wcn36xx_dbg_mask, uint, 0644);
 MODULE_PARM_DESC(debug_mask, "Debugging mask");
 
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index 9c6590d..1c75987 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -72,8 +72,10 @@ struct wcn36xx_cfg_val {
 	WCN36XX_CFG_VAL(DYNAMIC_PS_POLL_VALUE, 0),
 	WCN36XX_CFG_VAL(TX_PWR_CTRL_ENABLE, 1),
 	WCN36XX_CFG_VAL(ENABLE_CLOSE_LOOP, 1),
-	WCN36XX_CFG_VAL(ENABLE_LPWR_IMG_TRANSITION, 0),
+	WCN36XX_CFG_VAL(BTC_STATIC_LEN_LE_BT, 120000),
+	WCN36XX_CFG_VAL(BTC_STATIC_LEN_LE_WLAN, 30000),
 	WCN36XX_CFG_VAL(MAX_ASSOC_LIMIT, 10),
+	WCN36XX_CFG_VAL(ENABLE_LPWR_IMG_TRANSITION, 0),
 	WCN36XX_CFG_VAL(ENABLE_MCC_ADAPTIVE_SCHEDULER, 0),
 };
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project