Re: [PATCH] rtl8xxxu: add bluetooth co-existence support for single antenna

[Jes Sorensen]

On 9/3/19 1:37 AM, Chris Chiu wrote:
> The RTL8723BU suffers the wifi disconnection problem while bluetooth
> device connected. While wifi is doing tx/rx, the bluetooth will scan
> without results. This is due to the wifi and bluetooth share the same
> single antenna for RF communication and they need to have a mechanism
> to collaborate.
> 
> BT information is provided via the packet sent from co-processor to
> host (C2H). It contains the status of BT but the rtl8723bu_handle_c2h
> dose not really handle it. And there's no bluetooth coexistence
> mechanism to deal with it.
> 
> This commit adds a workqueue to set the tdma configurations and
> coefficient table per the parsed bluetooth link status and given
> wifi connection state. The tdma/coef table comes from the vendor
> driver code of the RTL8192EU and RTL8723BU. However, this commit is
> only for single antenna scenario which RTL8192EU is default dual
> antenna. The rtl8xxxu_parse_rxdesc24 which invokes the handle_c2h
> is only for 8723b and 8192e so the mechanism is expected to work
> on both chips with single antenna. Note RTL8192EU dual antenna is
> not supported.

I am pretty excited to see this! It always bugged me the bluetooth
driver was allowed to be applied breaking the existing wifi driver.

Except for some cosmetic stuff, I am all happy with this.

> Signed-off-by: Chris Chiu <chiu@xxxxxxxxxxxx>
> ---
>  .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h  |  37 +++
>  .../realtek/rtl8xxxu/rtl8xxxu_8723b.c         |   2 -
>  .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 243 +++++++++++++++++-
>  3 files changed, 275 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> index 582c2a346cec..22e95b11bfbb 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h

> +
> +struct rtl8xxxu_btcoex {
> +	u8      bt_status;
> +	bool	bt_busy;
> +	bool	has_sco;
> +	bool	has_a2dp;
> +	bool    has_hid;
> +	bool    has_pan;
> +	bool	hid_only;
> +	bool	a2dp_only;
> +	bool    c2h_bt_inquiry;
> +};

bool is large, maybe just use flags or u8's for this? Not a big deal though.

> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> index a6f358b9e447..4f72c2d14d44 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c


> +	if (!btcoex->has_a2dp &&
> +	    !btcoex->has_sco &&
> +	    !btcoex->has_pan &&
> +	    btcoex->has_hid)

This should all fit in one line - 80 characters

> +		btcoex->hid_only = true;
> +	else
> +		btcoex->hid_only = false;
> +
> +	if (!btcoex->has_sco &&
> +	    !btcoex->has_pan &&
> +	    !btcoex->has_hid &&
> +	    btcoex->has_a2dp)

Ditto

> +static void rtl8xxxu_c2hcmd_callback(struct work_struct *work)
> +{
> +	struct rtl8xxxu_priv *priv;
> +	struct rtl8723bu_c2h *c2h;
> +	struct ieee80211_vif *vif;
> +	struct device *dev;
> +	struct sk_buff *skb = NULL;
> +	unsigned long flags;
> +	int len;
> +	u8 bt_info = 0;
> +	struct rtl8xxxu_btcoex *btcoex;
> +
> +	priv = container_of(work, struct rtl8xxxu_priv, c2hcmd_work);
> +	vif = priv->vif;
> +	btcoex = &priv->bt_coex;
> +	dev = &priv->udev->dev;
> +
> +	if (priv->rf_paths > 1)
> +		goto out;
> +
> +	while (!skb_queue_empty(&priv->c2hcmd_queue)) {
> +		spin_lock_irqsave(&priv->c2hcmd_lock, flags);
> +		skb = __skb_dequeue(&priv->c2hcmd_queue);
> +		spin_unlock_irqrestore(&priv->c2hcmd_lock, flags);
> +
> +		c2h = (struct rtl8723bu_c2h *)skb->data;
> +		len = skb->len - 2;
> +
> +		switch (c2h->id) {
> +		case C2H_8723B_BT_INFO:
> +			bt_info = c2h->bt_info.bt_info;
> +
> +			rtl8723bu_update_bt_link_info(priv, bt_info);
> +
> +			if (btcoex->c2h_bt_inquiry) {
> +				if (vif && !vif->bss_conf.assoc) {
> +					rtl8723bu_set_ps_tdma(priv, 0x8, 0x0, 0x0, 0x0, 0x0);
> +					rtl8723bu_set_coex_with_type(priv, 0);
> +				} else if (btcoex->has_sco ||
> +					   btcoex->has_hid ||
> +					   btcoex->has_a2dp) {
> +					rtl8723bu_set_ps_tdma(priv, 0x61, 0x35, 0x3, 0x11, 0x11);
> +					rtl8723bu_set_coex_with_type(priv, 4);
> +				} else if (btcoex->has_pan) {
> +					rtl8723bu_set_ps_tdma(priv, 0x61, 0x3f, 0x3, 0x11, 0x11);
> +					rtl8723bu_set_coex_with_type(priv, 4);
> +				} else {
> +					rtl8723bu_set_ps_tdma(priv, 0x8, 0x0, 0x0, 0x0, 0x0);
> +					rtl8723bu_set_coex_with_type(priv, 7);
> +				}
> +
> +				return;
> +			}

Kernel code is 80 characters wide - maybe create a btcoex helper
function for this?

> +
> +			if (vif && vif->bss_conf.assoc) {
> +				u32 val32 = 0;
> +				u32 high_prio_tx = 0, high_prio_rx = 0;
> +
> +				val32 = rtl8xxxu_read32(priv, 0x770);
> +				high_prio_tx = val32 & 0x0000ffff;
> +				high_prio_rx = (val32  & 0xffff0000) >> 16;
> +
> +				if (btcoex->bt_busy) {
> +					if (btcoex->hid_only) {
> +						rtl8723bu_set_ps_tdma(priv, 0x61, 0x20, 0x3, 0x11, 0x11);
> +						rtl8723bu_set_coex_with_type(priv, 5);
> +					} else if (btcoex->a2dp_only) {
> +						rtl8723bu_set_ps_tdma(priv, 0x61, 0x35, 0x3, 0x11, 0x11);
> +						rtl8723bu_set_coex_with_type(priv, 4);
> +					} else if ((btcoex->has_a2dp &&
> +						    btcoex->has_pan) ||
> +						   (btcoex->has_hid &&
> +						    btcoex->has_a2dp &&
> +						    btcoex->has_pan)) {
> +						rtl8723bu_set_ps_tdma(priv, 0x51, 0x21, 0x3, 0x10, 0x10);
> +						rtl8723bu_set_coex_with_type(priv, 4);
> +					} else if (btcoex->has_hid &&
> +						 btcoex->has_a2dp) {
> +						rtl8723bu_set_ps_tdma(priv, 0x51, 0x21, 0x3, 0x10, 0x10);
> +						rtl8723bu_set_coex_with_type(priv, 3);
> +					} else {
> +						rtl8723bu_set_ps_tdma(priv, 0x61, 0x35, 0x3, 0x11, 0x11);
> +						rtl8723bu_set_coex_with_type(priv, 4);
> +					}

Same here

Otherwise, thanks for digging into this, it's really great to see!

Cheers,
Jes