RE: [RFC PATCH v1 0/9] wifi: rtw88: add RTL8723B/RTL8723BS support

[Luka Gejak <luka.gejak@linux.dev>]

Hi Ping-Ke,

Thanks, and noted on stopping the v1 review. Before I send v2 I want to
report two things: the firmware experiment you suggested, and what I
found when I tried to work out which patches are actually droppable.

1. Firmware

I tried all of them, plus the staging blob:

  rtlwifi/rtl8723befw.bin      loads, version 15.17.0, associates
  rtlwifi/rtl8723befw_36.bin   loads, version 36.0.0,  associates
  rtlwifi/rtl8723bs_nic.bin    loads, version 35.0.0,  associates

So the driver runs on all of them. But the coex reading does not change:
BT Rpt stays 0x00000069 on every one, byte for byte. That makes sense in
hindsight, since the version is reported by the BT firmware
(rtl_bt/rtl8723bs_fw.bin) and relayed through, so no WiFi firmware can
move it. rtl8723bs_nic.bin additionally produced 72 RA report WARNs,
because the driver sets c2h_ra_report_size = 4 for the v41 report format
where the siblings use 7.

Since the driver was developed and tuned against v41 and swapping the
blob does not help coex, I am keeping v41 and leaving the linux-firmware
submission as it is rather than withdrawing it. If you would still
rather I hold it, say so and I will.
,
I have not closed the coex question, and I no longer think it can be
closed from this driver.

The driver does ask: rtw_fw_query_bt_info() goes out on the connect and
scan paths and at power on. The BT side answers, so this is not a
missing or timed out request. The answer is the problem. With A2DP
streaming it reports conn-idle and an empty profile list,
BT_RSP = 01 01 40 12, while the same dump shows 440 low priority packets
each way. The profile byte stays 0x01 (CONNECTION) and never sets A2DP
or ACL_BUSY. I checked the decode against the vendor driver, which reads
the same byte, so this is not a parsing error on my side, and the coex
algorithm itself is clearly running: TDMA case-3, table 10, reason
BTINFO.

Those profile bits originate in the BT firmware and reach the WiFi side
over the chip internal link, so no change in this driver can supply
them. I am not going to infer a profile or pin a TDMA case to
compensate, because that means guessing at BT state I cannot observe,
and it is the same kind of unjustified special case I have just spent
this round removing. I will document the limitation in the cover letter
instead. I can say the empty profile list would explain why WiFi is left
with almost nothing, but I have not proven that it is the cause.

2. Which patches are hard required

You suggested keeping the first version minimal and adding special cases
after it merges, so I tried to find out what the minimum actually is,
by building reduced sets and testing each on hardware.

The result is that there is less to defer than I expected.

Building only the chip body plus Kconfig and MAINTAINERS compiles clean
with W=1, the firmware loads, and RX works: a scan finds the APs around
me. But it never associates, because nothing implements the SDIO TX
path.

Adding that back pulls in the rest, because the shared code is
interlocked:

  SDIO transfer contract  needs  the association sequence
                                 (rtw_sdio_rx_skb calls into it)
  association sequence    needs  the coex antenna handling, the H2C
                                 interface and the scan/calibration
                                 patch
  scan/calibration        needs  the H2C interface

Taking any one of them without the others does not build. Measured on
hardware, with all of them in but without the TX retry patch:

  uplink 11.9 Mbit/s, 204 TCP retransmits

and with the TX retry patch added:

  uplink 20.1 Mbit/s, 2 TCP retransmits

Downlink is ~40 Mbit/s either way. So the retry patch is not a
refinement, it is worth most of the uplink on this hardware.

That leaves exactly one patch I can defer, the TX report purge timeout,
and it is this:

  -	if (rtwdev->chip->id == RTW_CHIP_TYPE_8723D &&
  -	    rtwdev->hci.type == RTW_HCI_TYPE_USB)
  +	if ((rtwdev->chip->id == RTW_CHIP_TYPE_8723D &&
  +	     rtwdev->hci.type == RTW_HCI_TYPE_USB) ||
  +	    rtw_is_8723bs(rtwdev))
   		timeout = msecs_to_jiffies(2500);

Three lines, and it only adds one condition to a case that is already in
tree. Without it the "failed to get tx report" warning comes back on
slower SDIO hosts, which is what Peter reported on his Pine64 boards. I
would rather keep it than defer it, but I will drop it if you disagree.

One correction to something I told you earlier. I said the auth_sync
mechanism tested removable, and its wait does. But the SDIO RX path
calls rtw8723bs_auth_sync_rx(), so it cannot simply be deferred without
also changing the SDIO patch. If you want it gone I will remove the hook
with it, I just did not want to imply it was free.

3. Shape of v2

Unless you prefer otherwise, I will restructure to what you sketched:

  1 add hard required handling for RTL8723BS   (the shared code: main.h,
    reg.h, sec.h, fw.c/h, coex.c/h, main.c, mac80211.c, sdio.c/h)
  2 add RTL8723B main body                     (rtw8723b.c/h, tables,
    rtw8723bs.c)
  3 add Kconfig and Makefile
  4 MAINTAINERS

That means splitting the current chip patch, which today carries both
the shared definitions and the chip body, and squashing the shared code
patches together. No code is removed by this, it is only re-shaping.

The one thing I would like your opinion on before I do it: patch 1 above
touches six shared files and will be large. Would you rather have it as
one patch, or split into two or three smaller ones, for example the H2C
interface, then coex, then the SDIO path? I will go with a single patch
if I do not hear otherwise, since the dependency analysis says they
belong together, but it is easier to review split up.

Peter has said he will retest v2 on his Pine64 boards and get others
with the hardware to test as well. That covers the main gap in what I
can measure here, since his hosts are slower than mine and are where the
timing sensitive paths actually show up.

Best regards,
Luka Gejak