RE: [PATCH v2 05/11] wifi: rtw88: coex: add the RTL8723BS scan antenna workaround

[Ping-Ke Shih <pkshih@realtek.com>]

luka.gejak@linux.dev <luka.gejak@linux.dev> wrote:
> From: Luka Gejak <luka.gejak@linux.dev>
> 
> On the RTL8723BS the PTA antenna path has to be programmed directly
> during scan. The vendor driver keeps its own scan-time antenna state and
> does not run the generic coexistence algorithm on boards where BT is
> disabled, and following it is necessary here: without the antenna and
> CCK priority setup applied at scan start, the site survey does not hear
> the AP reliably.
> 
> Detect the BT-disabled case, replay the vendor BT_MP and BT_INFO queries
> once the firmware has been up long enough for RX DMA to be stable, then
> establish the scan path antenna configuration and skip the generic run.
> 
> Signed-off-by: Luka Gejak <luka.gejak@linux.dev>
> ---
>  drivers/net/wireless/realtek/rtw88/coex.c | 179 +++++++++++++++++++++-
>  drivers/net/wireless/realtek/rtw88/coex.h |   2 +
>  drivers/net/wireless/realtek/rtw88/reg.h  |   1 +
>  3 files changed, 181 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/coex.c b/drivers/net/wireless/realtek/rtw88/coex.c
> index 37c336def419..daba6082f850 100644
> --- a/drivers/net/wireless/realtek/rtw88/coex.c
> +++ b/drivers/net/wireless/realtek/rtw88/coex.c
> @@ -1443,6 +1443,156 @@ static void rtw_coex_set_ant_path(struct rtw_dev *rtwdev, bool force, u8 phase)
>  #define case_ALGO(src) \
>         case COEX_ALGO_##src: return #src
> 
> +/* 8723BS SDIO WiFi/BT coexistence antenna handling. On BT-disabled boards the
> + * scan/auth window still routes through the PTA mux; these helpers force the
> + * vendor-shaped WiFi-owned antenna path so directed management TX reaches air.
> + */

comment style.

> +#define REG_8723BS_BT_COEX_CTRL                0x0039
> +#define REG_8723BS_BB_ANT_CFG          0x0930
> +#define REG_8723BS_BB_ANT_CFG1         0x0944
> +#define REG_8723BS_BB_ANT_BUF          0x0974
> +#define RTW8723BS_COEX_H_WLAN_ACTIVE   0x1800101b

define these in reg.h

> +
> +static bool rtw_coex_8723bs_ant_is_aux(struct rtw_dev *rtwdev)
> +{
> +       return !!(rtwdev->efuse.bt_setting & BIT(6));
> +}
> +
> +static bool rtw_coex_8723bs_bt_disabled(struct rtw_dev *rtwdev)
> +{
> +       return rtw_is_8723bs(rtwdev) && rtwdev->coex.stat.bt_disabled;
> +}
> +
> +static u32 rtw_coex_8723bs_pta_ant_path(struct rtw_dev *rtwdev)
> +{
> +       return rtw_coex_8723bs_ant_is_aux(rtwdev) ? 0x80 : 0x200;
> +}
> +
> +/* Write BB_SEL_BTG, retrying once with SYS_FUNC BB reset if the first
> + * write does not stick (RF/BB clock may have been gated).
> + */

With the given name, this comment seems not necessary. 

> +static u32 rtw_coex_8723bs_write_bb_sel_btg(struct rtw_dev *rtwdev, u32 value)
> +{
> +       u8 sys_func_before;
> +       u32 readback;
> +
> +       sys_func_before = rtw_read8(rtwdev, REG_SYS_FUNC_EN);
> +       if ((sys_func_before & (BIT(0) | BIT(1))) != (BIT(0) | BIT(1))) {
> +               rtw_write8_set(rtwdev, REG_SYS_FUNC_EN, BIT(0) | BIT(1));

Use given names for BIT 0/1.

> +               usleep_range(10, 11);
> +       }
> +
> +       rtw_write32(rtwdev, REG_BB_SEL_BTG_8723B, value);
> +       readback = rtw_read32(rtwdev, REG_BB_SEL_BTG_8723B);
> +       if (readback == value)
> +               return readback;
> +
> +       usleep_range(10, 11);
> +       rtw_write8_set(rtwdev, REG_SYS_FUNC_EN, BIT(0) | BIT(1));
> +       rtw_write32(rtwdev, REG_BB_SEL_BTG_8723B, value);
> +
> +       return rtw_read32(rtwdev, REG_BB_SEL_BTG_8723B);
> +}
> +
> +static u32 rtw_coex_8723bs_reassert_pta_ant(struct rtw_dev *rtwdev)
> +{
> +       return rtw_coex_8723bs_write_bb_sel_btg(rtwdev,
> +                                               rtw_coex_8723bs_pta_ant_path(rtwdev));

Just call rtw_coex_8723bs_write_bb_sel_btg() ?

> +}
> +
> +static void rtw_coex_8723bs_set_cck_pri(struct rtw_dev *rtwdev, bool high)
> +{
> +       if (!rtw_coex_8723bs_bt_disabled(rtwdev))
> +               return;
> +
> +       if (high) {
> +               rtw_write32(rtwdev, REG_BT_COEX_TABLE_H,
> +                           RTW8723BS_COEX_H_WLAN_ACTIVE);
> +       } else {
> +               rtw_coex_set_wl_pri_mask(rtwdev, COEX_WLPRI_TX_CCK, false);
> +               rtw_coex_set_wl_pri_mask(rtwdev, COEX_WLPRI_RX_CCK, false);
> +       }
> +}
> +
> +static void rtw_coex_8723bs_restore_pad_ctrl(struct rtw_dev *rtwdev,
> +                                            bool keep_pta_owner)
> +{
> +       u32 before, after;
> +
> +       before = rtw_read32(rtwdev, REG_PAD_CTRL1);
> +       after = before & ~(BIT_LNAON_WLBT_SEL | BIT_SW_DPDT_SEL_DATA);
> +       if (keep_pta_owner)
> +               after |= BIT_PAPE_WLBT_SEL;
> +       else
> +               after &= ~BIT_PAPE_WLBT_SEL;
> +       if (after != before)
> +               rtw_write32(rtwdev, REG_PAD_CTRL1, after);
> +}
> +
> +static void rtw_coex_8723bs_fw_gnt_bt_low(struct rtw_dev *rtwdev)
> +{
> +       if (!rtw_coex_8723bs_bt_disabled(rtwdev))

Please review your calling path. I think callers should ensure RTL8723BS
before calling. (But here needs additional condition bt_disabled though)

> +               return;
> +
> +       if (rtw_read8(rtwdev, REG_GNT_BT) == 0x00 &&
> +           rtw_read8(rtwdev, REG_BT_COEX_ENH_INTR_CTRL) == 0x0c)
> +               return;
> +
> +       rtw_fw_set_gnt_bt(rtwdev, 0);
> +}
> +
> +static void rtw_coex_8723bs_reassert_ant_buffer(struct rtw_dev *rtwdev)
> +{
> +       u8 sys_func_before;
> +
> +       sys_func_before = rtw_read8(rtwdev, REG_SYS_FUNC_EN);
> +       if ((sys_func_before & (BIT(0) | BIT(1))) != (BIT(0) | BIT(1))) {

Please use given names for BIT 0/1.

> +               rtw_write8_set(rtwdev, REG_SYS_FUNC_EN, BIT(0) | BIT(1));
> +               usleep_range(10, 11);
> +       }
> +
> +       rtw_write8_mask(rtwdev, REG_8723BS_BT_COEX_CTRL, BIT(3), 0x1);
> +       rtw_write8(rtwdev, REG_8723BS_BB_ANT_BUF, 0xff);
> +       rtw_write8_mask(rtwdev, REG_8723BS_BB_ANT_CFG1, 0x3, 0x3);
> +       rtw_write8(rtwdev, REG_8723BS_BB_ANT_CFG, 0x77);
> +}
> +
> +static void rtw_coex_8723bs_apply_scan_table(struct rtw_dev *rtwdev)
> +{
> +       rtwdev->coex.dm.cur_table = 2;
> +       rtw_coex_set_table(rtwdev, true, 0x5a5a5a5a, 0x5a5a5a5a);
> +}
> +
> +/* Non-connected scan/auth workaround: PS-TDMA type 8 off, PTA antenna path,
> + * coex table type 2 (matches the vendor non-connected arbitration).
> + */
> +void rtw_coex_8723bs_scan_workaround(struct rtw_dev *rtwdev)
> +{
> +       struct rtw_coex_dm *coex_dm = &rtwdev->coex.dm;
> +       struct rtw_coex_stat *coex_stat = &rtwdev->coex.stat;
> +
> +       if (!rtw_is_8723bs(rtwdev))

Caller check this already.

> +               return;
> +
> +       coex_dm->cur_ps_tdma_on = false;
> +       coex_dm->cur_ps_tdma = 8;
> +       coex_dm->ps_tdma_para[0] = 0x08;
> +       coex_dm->ps_tdma_para[1] = 0x00;
> +       coex_dm->ps_tdma_para[2] = 0x00;
> +       coex_dm->ps_tdma_para[3] = 0x00;
> +       coex_dm->ps_tdma_para[4] = 0x00;
> +
> +       rtw_fw_coex_tdma_type(rtwdev, 0x08, 0x00, 0x00, 0x00, 0x00);
> +       rtw_coex_8723bs_fw_gnt_bt_low(rtwdev);
> +       rtw_coex_set_ant_path(rtwdev, true, COEX_SET_ANT_2G);
> +       rtw_coex_8723bs_reassert_ant_buffer(rtwdev);
> +       rtw_coex_8723bs_apply_scan_table(rtwdev);
> +       if (coex_stat->bt_disabled)

It looks like callers check this already. 

> +               rtw_coex_8723bs_set_cck_pri(rtwdev, true);
> +       rtw_coex_8723bs_reassert_pta_ant(rtwdev);
> +       rtw_coex_8723bs_restore_pad_ctrl(rtwdev, true);

I wonder how you can make this workaround? There are so many arguments.

> +}
> +
>  static const char *rtw_coex_get_algo_string(u8 algo)
>  {
>         switch (algo) {
> @@ -2770,7 +2920,7 @@ void rtw_coex_power_on_setting(struct rtw_dev *rtwdev)
>         coex->stop_dm = true;
>         coex->wl_rf_off = false;
> 
> -       /* enable BB, we can write 0x948 */
> +       /* enable BB, so BB_SEL_BTG is writable */
>         rtw_write8_set(rtwdev, REG_SYS_FUNC_EN,
>                        BIT_FEN_BB_GLB_RST | BIT_FEN_BB_RSTB);
> 
> @@ -2877,6 +3027,33 @@ void rtw_coex_scan_notify(struct rtw_dev *rtwdev, u8 type)
>         coex->freeze = false;
>         rtw_coex_write_scbd(rtwdev, COEX_SCBD_ACTIVE | COEX_SCBD_ONOFF, true);
> 
> +       /* 8723BS SDIO BT-disabled: keep the scan/auth PTA antenna state the
> +        * vendor uses and skip the generic coex run. At scan start (firmware
> +        * has been up long enough for stable RX DMA) replay the vendor BT_MP /
> +        * BT_INFO queries, then re-establish the scan-path PTA setup.
> +        */
> +       if (rtw_coex_8723bs_bt_disabled(rtwdev)) {
> +               if (type == COEX_SCAN_START_2G || type == COEX_SCAN_START) {
> +                       struct rtw_coex_info_req req = {};
> +
> +                       coex_stat->cnt_wl[COEX_CNT_WL_SCANAP] = 0;
> +                       coex_stat->wl_hi_pri_task2 = true;
> +
> +                       req.seq = 0x0e;
> +                       req.op_code = BT_MP_INFO_OP_SUPP_VER;
> +                       rtw_fw_query_bt_mp_info(rtwdev, &req);
> +                       req.seq = 0x0f;
> +                       req.op_code = BT_MP_INFO_OP_PATCH_VER;
> +                       rtw_fw_query_bt_mp_info(rtwdev, &req);

These two are debug purpose. When you cat debugfs, it will try to query the
version, won't it?

> +                       rtw_fw_query_bt_info(rtwdev);

It seems to be called at initial. Is it necessary?

> +
> +                       rtw_coex_8723bs_scan_workaround(rtwdev);
> +               } else {
> +                       coex_stat->wl_hi_pri_task2 = false;
> +               }
> +               return;

Since RTL8723BS does special handle, just move all into a function, but put
in common flow.

> +       }
> +
>         if (type == COEX_SCAN_START_5G) {
>                 rtw_dbg(rtwdev, RTW_DBG_COEX,
>                         "[BTCoex], SCAN START notify (5G)\n");
> diff --git a/drivers/net/wireless/realtek/rtw88/coex.h b/drivers/net/wireless/realtek/rtw88/coex.h
> index c398be8391f7..72b1353c9313 100644
> --- a/drivers/net/wireless/realtek/rtw88/coex.h
> +++ b/drivers/net/wireless/realtek/rtw88/coex.h
> @@ -430,4 +430,6 @@ static inline void rtw_coex_active_query_bt_info(struct rtw_dev *rtwdev)
>                 rtw_coex_query_bt_info(rtwdev);
>  }
> 
> +void rtw_coex_8723bs_scan_workaround(struct rtw_dev *rtwdev);
> +

Just static no need to export.

>  #endif