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

[luka . gejak]

From: Luka Gejak <luka.gejak@xxxxxxxxx>

On 27/07/2026 07:58, Ping-Ke Shih wrote:
>> +/* 8723BS SDIO WiFi/BT coexistence antenna handling. On BT-disabled boards the
> 
> comment style.

Fixed here and everywhere else in the series.

>> +#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

Done for the registers. 0x0974 already existed as REG_RFE_CTRL_E, so I
use that. The other three are now

  #define REG_PWR_DATA		0x0038
  #define BIT_EEPRPAD_RFE_CTRL_EN	BIT(11)
  #define REG_RFE_CTRL_ANTA_SRC	0x0930
  #define REG_RFE_CTRL_ANT_SW	0x0944
  #define BIT_RFE_CTRL_ANT_SW_SEL	GENMASK(1, 0)

0x0039 is the second byte of REG_PWR_DATA and the bit the vendor sets
there is bit 11 of the register, which rtl8xxxu calls
PWR_DATA_EEPRPAD_RFE_CTRL_EN, so I used the same name with rtw88's
BIT_ prefix. 0x0930 is rfe_ctrl_anta_src in the vendor PHY register
header. 0x0944 has no vendor name I could find, so I named it after
what the write does; happy to take a better one if you have it.

0x1800101b I left in coex.c: it is a value written to
REG_BT_COEX_TABLE_H, not a register, and coex.c already keeps its other
table values inline. Say the word and I will move it.

>> +/* Write BB_SEL_BTG, retrying once with SYS_FUNC BB reset if the first
> 
> With the given name, this comment seems not necessary.

Removed.

>> +       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.

Now BIT_FEN_BB_GLB_RST | BIT_FEN_BB_RSTB. The same open-coded sequence
appeared twice, so it is factored into rtw_coex_8723bs_enable_bb().

>> +static u32 rtw_coex_8723bs_reassert_pta_ant(struct rtw_dev *rtwdev)
> 
> Just call rtw_coex_8723bs_write_bb_sel_btg() ?

Dropped. rtw_coex_8723bs_write_bb_sel_btg() now takes no value and
computes the PTA path itself, since that was the only value ever
written.

>> +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)
[...]
>> +       if (!rtw_is_8723bs(rtwdev))
> 
> Caller check this already.
[...]
>> +       if (coex_stat->bt_disabled)
> 
> It looks like callers check this already.

All of these are gone. There are now exactly two entry points from the
common flow, rtw_coex_8723bs_scan_notify() and
rtw_coex_8723bs_connect_notify(), which test the chip and bt_disabled
once and return true when they have handled the notification.
Everything below them assumes both. rtw_coex_8723bs_bt_disabled() is
gone with them.

While doing that I found rtw_coex_8723bs_set_cck_pri() was only ever
called with high=true, so the argument and its dead else branch are
removed too.

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

It is halbtc8723b1ant_SetAntPath() for the wifi-only / BT-off case,
plus the vendor's non-connected coex table and PS-TDMA type 8. Every
register write in the function maps one to one onto a vendor write in
that path; I did not invent any of the values. I then removed them one
at a time on hardware to check each was actually load bearing, which is
how the ones below went.

>> +                       req.op_code = BT_MP_INFO_OP_SUPP_VER;
>> +                       rtw_fw_query_bt_mp_info(rtwdev, &req);
>> +                       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?

You are right on both counts, all three are removed.

Measured over 10 cold module reloads, three scans each, with and
without them:

  with:     24/24 scans saw the target AP, 8/8 associations
  without:  29/30 scans saw the target AP, 10/10 associations

The single miss is one scan pass not catching a beacon, not a
regression. The pre-auth rtw_fw_query_bt_info() in patch 06 went the
same way, together with two of the three repeated PS-TDMA H2Cs there.

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

Done, that is rtw_coex_8723bs_scan_notify() above.

>> +void rtw_coex_8723bs_scan_workaround(struct rtw_dev *rtwdev);
> 
> Just static no need to export.

Static now, and the coex.h declaration is gone.

Best regards,
Luka Gejak