Re: [PATCH v3 3/6] wifi: rtw88: 8723b: add the RTL8723B chip driver

[Bitterblue Smith]

On 24/09/2026 00:50, Luka Gejak wrote:
> On Wed Sep 23, 2026 at 10:30 AM CEST, Ping-Ke Shih wrote:
>> Luka Gejak <luka.gejak@xxxxxxxxx> wrote:
>>> Add the Realtek RTL8723B 802.11n chip driver: the chip operations, the
>>> power sequences, the efuse layout, the RF and IQ calibration, and the
>>> chip specific coexistence handling.
>>>
> 
> Thanks for the review. Everything that could be turned into a change is
> in v4, and six of your points are worth an answer as much as a change,
> so here they are.
> 
>>> +#include <linux/unaligned.h>
>>> +#include "main.h"
>>> +#include "coex.h"
>> [...]
>>> +#include "tx.h"
>>
>> In increasing alphabet order.
> 
> The list is already in increasing alphabetical order after main.h, and
> main.h cannot be sorted into place. The other rtw88 headers have no
> includes of their own and are written expecting main.h to be first: it is
> what brings in struct rtw_dev and even BIT() and GENMASK(). With mac.h
> ahead of main.h the build stops after 179 error lines, the first of them
> 
>   'struct rtw_dev' declared inside parameter list will not be visible
>   outside of this definition or declaration
>   implicit declaration of function 'GENMASK'
>   passing argument 1 of 'rtw_set_channel_mac' from incompatible pointer
>   type
> 
> I tried the sort and reverted it. A strict order needs main.h added to
> those headers first. I can send that as a separate patch ahead of this
> series, but I would rather not fold a shared header change into the chip
> driver.
> 
>>> +#define MASK_NETTYPE   0x30000
>>> +#define _NETTYPE(x)    (((x) & 0x3) << 16)
>>> +#define NT_LINK_AP     0x2
>>
>> The PORT_SET_NET_TYPE in rtw_vif_port_config() does similar thing
>> relying on static const struct rtw_vif_port rtw_vif_port[].
>>
>> Is that not suitable for RTL8723B? If so, should the common flow
>> avoid RTL8723B?
> 
> It is suitable, and the common flow does not need to avoid RTL8723B.
> PORT_SET_NET_TYPE writes the same field with the same value:
> rtw_vif_port[0].net_type is address 0x0100 with mask 0x30000, which is
> REG_CR bits 17:16, and rtwvif->net_type is RTW_NET_MGD_LINKED, 2, the
> value NT_LINK_AP carried.
> 
> Only the timing differs, and there the common flow is the better one.
> rtw_ops_add_interface() sets net_type from the interface type,
> RTW_NET_NO_LINK for a station, and the association path moves it to
> RTW_NET_MGD_LINKED, writing REG_CR after both. The chip local call ran
> during mac_init and forced the linked value before there was a link,
> and the core's write replaced it a moment later in any case.
> 
> So v4 drops rtw8723b_init_network_type() with MASK_NETTYPE, _NETTYPE()
> and NT_LINK_AP, and the chip relies on rtw_vif_port[] like the others.
> No other rtw88 chip defines a net type value of its own.
> 
>>> +       /* Override the default rcr filter for 8723B */
>>> +       rtwdev->hal.rcr = WLAN_RCR_CFG;
>>
>> Why? The default value doesn't work to RTL8723B?
> 
> Two things say it does not.
> 
> hal.rcr is not only written at init. fw.c clears and restores
> BIT_CBSSID_BCN in it around the beacon filter, and mac80211.c toggles
> BIT_AM, so whatever is left there has to keep those bits.

I don't understand the conclusion...

> 
> And this is not a private filter. It is the RCR that rtw8723x_mac_init()
> writes for the whole 8723x family, 0x700060ce, with BIT_APP_FCS added.
> The generic default is a different set: it has BIT_PKTCTL_DLEN and lacks
> BIT_CBSSID_DATA, BIT_CBSSID_BCN and BIT_AMF, so it drops exactly the bits
> fw.c manipulates on this chip.

That's fine. The value written by rtw8723x_mac_init() gets overwritten
with the default value from rtw_core_init().

> 
> BIT_APP_FCS has to be set because rtw88 advertises RX_INCLUDES_FCS for
> every chip and the shared 8723x value has bit 31 clear; without it
> mac80211 trims four bytes of real frame data. v4 says this at the
> assignment.
> 
>>> +       rtw8723b_init_adaptive_ctrl(rtwdev);
>>> +       rtw8723b_init_edca(rtwdev);
>>> +       rtw8723b_init_retry_function(rtwdev);
>>
>> So RTL8723B is very different from existing chips?
> 
> No, and the siblings write the same registers with the same values.
> 
> rtw8703b_phy_set_param() writes REG_SPEC_SIFS, REG_MAC_SPEC_SIFS, REG_SIFS
> and REG_SIFS + 2 with the same 0x100a, ACKTO is 0x40 in both, and
> REG_RETRY_LIMIT uses the same 0x3030 in both. Its four EDCA registers are
> the same magnitudes as here:
> 
>   8703b  0x002FA226 VO  0x005EA324 VI  0x005EA42B BE  0x0000A44F BK
>   8723b  0x002FA226 VO  0x005EA324 VI  0x005EA42B BE  0x0000A44F BK
> 
> rtw88xxa.c writes REG_RRSR with the same 0xfffff / 0xffff1 pair used here,
> and REG_RESP_SIFS_CCK and REG_RESP_SIFS_OFDM are the pair that only this
> chip and 8822c write. So I read the three helpers as the family pattern
> rather than as something chip specific. Say the word if you would rather
> have them inlined into rtw8723b_phy_set_param() the way 8703b has them;
> that is a smaller diff.
> 
>>> +static bool rtw8723b_sdio_needs_rx_path_fix(struct rtw_dev *rtwdev)
>>
>> What does it mean?
>>
>> In many places using this function are not RX path.
> 
> Right, the name hid what it is, and it is a test the preparation series
> already provides. The function gates the SDIO only register work: the PAD
> mux restore in post_enable_flow, the path control save and restore around
> IQK, and the trailing re-assert in set_channel. v4 drops the local helper
> and calls rtw_is_8723bs() at those seven places, which is what rx.c, tx.c
> and sdio.c already use.
> 
> One more, where the review asked for a change that was already there.
> The declarations in rtw8723b_reassert_rx_path() that you marked for
> reverse X'mas order are already longest first, three u32 lines followed
> by two u8 lines, and they are the same in the sent v3 and in v4. If you
> had a different order in mind, tell me which and I will apply it.
> 
> I re-ran the hardware validation on this exact tip after these changes.
> The suite and the soak both pass: association, WPA2, DHCP, throughput,
> latency, scans under traffic, 20/20 reload/reassociate, link cycles and
> reconnects, with no error, TX-report, H2C, LPS or lockdep lines in dmesg,
> and no regression against the branch that was validated before.
> 
> Best regards,
> Luka Gejak