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

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

Hi Ping-Ke,

September 24, 2026 at 06:11, "Ping-Ke Shih" <pkshih@realtek.com> wrote:
> 
> Luka Gejak <luka.gejak@linux.dev> wrote:

Answers in the order of your mail. The MAINTAINERS point is in a
separate reply.

> Can you also review other RTL8723B specific functions? I didn't review
> them one by one by v3, but I wonder why it needs specific functions,
> not common flow. If any of them is necessary, please point out reasons.
>
> By the way, I didn't only mention these three functions. At here there
> are many specific functions. Please analyze them.
> (Honestly, I don't fully re-examinate your analysis in detail, and
> believe your results.)

I went through the whole file. The part that is already common flow can
be listed exactly, because those ops point at shared code:

  power_on, power_off        rtw_power_on, rtw_power_off
  mac_postinit               rtw8723x_mac_postinit
  set_tx_power_index         rtw8723x_set_tx_power_index
  false_alarm_statistics     rtw8723x_false_alarm_statistics
  read_rf, write_rf          rtw_phy_read_rf_sipi,
                             rtw_phy_write_rf_reg_sipi
  read_efuse                 rtw8723x_read_efuse, plus the hardware
                             capability, because this chip has no
                             hardware feature report: the firmware
                             reports id 0xfd instead of the C2H
  efuse_grant                rtw8723x_efuse_grant, plus the 0x6b BT
                             power cut and output isolation write that
                             the vendor efuse path does

The calls inside rtw8723b_phy_set_param() are the ones you mean. There
are twelve of them, and I have to be straight about one thing first:
there is no shared function to reuse for them. rtw8703b and rtw8723d do
the same work inline in their own phy_set_param(), and rtw8723x has no
equivalent. So "reuse the existing" cannot be done by calling something
that exists today. What is the same, what is not, and the three ways to
fix it follow.

Same as rtw8703b, value for value:

  REG_SPEC_SIFS, REG_MAC_SPEC_SIFS, REG_SIFS, REG_SIFS + 2   0x100a
  REG_EDCA_BE, _BK, _VI, _VO           0x005ea42b, 0x0000a44f,
                                       0x005ea324, 0x002fa226
  REG_ACKTO                                                0x40
  REG_RETRY_LIMIT                                          0x3030
  REG_SLOT, REG_BAR_MODE_CTRL, REG_ATIMWND    0x09, 0x0201ffff, 0x2
  REG_SINGLE_AMPDU_CTRL, REG_RX_PKT_LIMIT, REG_MAX_AGGR_NUM,
  REG_PIFS, REG_AMPDU_MAX_TIME         BIT_EN_SINGLE_APMDU, 0x18,
                                       0x1f, 0, 0x70

8703b has a name for each of these (WLAN_SPEC_SIFS, WLAN_RL_VAL,
WLAN_BAR_VAL, WLAN_SLOT_TIME, WLAN_RX_PKT_LIMIT, WLAN_MAX_AGG_NR,
WLAN_PIFS_VAL, WLAN_AMPDU_MAX_TIME). The chip file writes most of them
as bare numbers, which is why the two look different when they are the
same, and that is on me. 8723d is not the one to compare with here: it
uses 0x17, 0x0a and 0x1c for the aggregation values.

Not shared, and I think they have to stay chip local:

  - REG_RESP_SIFS_CCK and REG_RESP_SIFS_OFDM. Only 8723b and 8822c
    write these in rtw88; neither 8723x sibling does. The comment says
    why they are set.
  - the SDIO and page side: REG_BCNQ_BDNY, REG_MGQ_BDNY,
    REG_WMAC_LBK_BF_HD, REG_TRXFF_BNDY, REG_DWBCN0_CTRL, REG_PBP,
    REG_SDIO_TX_CTRL. The first six exist in the 8812a/8821a family but
    not in either 8723x sibling; REG_SDIO_TX_CTRL is an SDIO register and
    is used from sdio.c and mac.c.
  - REG_NAV_UPPER and the two REG_PKT_*_LIFE_TIME writes, which appear
    nowhere else in rtw88.
  - the rest of the chip's own hardware: the BB and RF configuration
    and the 3-wire setup, where BB_SEL_BTG differs by HCI type; the two
    LCK steps (rtw8723b_lck() is a superset of rtw8723x_lck(), it adds
    the RF_AC standby read and write and the 0xb0 writes around the same
    poll); the phy status page layout used by query_phy_status and
    cck_rx_power; the IQK, where rtw8723x_iqk_* gives the register and
    path control backup and restore and the compare, and the rest of the
    algorithm and its tables are this chip's; the power track tables;
    set_channel_bb and set_channel_rf; and the coex antenna, PTA and RFE
    logic, which each sibling has its own version of.

Here is the accounting for all twelve. init_adaptive_ctrl, init_edca,
init_retry_function and init_burst_pkt_len are your "the same"
comment, see the reuse answer below. init_tx_buffer_boundary,
init_operation_mode and init_beacon_parameters are the chip's own: the
FIFO page boundaries and REG_TRXFF_BNDY, REG_BWOPMODE which appears
nowhere else in rtw88, and a beacon control value that differs from
8703b. init_page_boundary, init_transfer_page_size,
init_driver_info_size and init_antenna_selection are one write each and
have no reason beyond naming the step, and 8723d writes REG_LEDCFG2
inline with the same value. Those four I can fold into phy_set_param()
so they read like the siblings, or leave them named, your call.

The twelfth is init_wmac_setting(), and it is where I found a real
problem:

1. rtw8723b_init_wmac_setting() re-implements part of
   rtw8723x_mac_init(), and one line of it is missing.
   rtwdev->hal.rxfltmap1 is never set. Every other chip sets it, and
   rtw_ops_configure_filter() writes it back to REG_RXFLTMAP1 when
   FIF_CONTROL is turned off, so on this chip that write would put 0
   into the register. The register itself is written correctly here, it
   is the cached value that is missing. I did not spot this in v4.

   The fix I would make is to drop rtw8723b_mac_init() and let the chip
   point at rtw8723x_mac_init(). That writes REG_TCR, RXFLTMAP0/1/2 and
   sets hal.rxfltmap1. It also writes REG_RCR with the vendor 8723x
   value, which is fine for this chip: phy_set_param applies the RCR we
   want after it, and rtw_core_start() applies hal.rcr after that. Then
   rtw8723b_init_wmac_setting() keeps only the RCR and the two REG_MAR
   writes, or disappears entirely if the RCR moves to rtw_core_init(),
   see below.

Outside that block, two more functions carry the same question, and I
would rather answer them now than in a later review.

2. rtw8723b_fill_txdesc_checksum() is the same body as
   fill_txdesc_checksum_common(txdesc, 16), which 8812a, 8821a, 8821c,
   8814a and 8822b already call with the same sixteen words. Only the
   shared 8723x version inverts the sum, which is what the comment here
   points at. If the non-inverted sum is deliberate for this chip, I
   will keep the function and say so; if it is not, the wrapper is the
   shorter answer.

3. rtw8723b_cfg_ldo25() is empty. The reason is in the vendor code:
   Hal_EfusePowerSwitch() only touches the 2.5V LDO when bWrite is
   TRUE, so on a read-only efuse path it is left alone, while the
   shared rtw8723x helper clears BIT_LDO25_EN whenever it is called
   with enable false. That is a hardware reason, so the function stays,
   but I would like the comment to say that instead of what it says
   now.

> I think the better way is to assign proper rtwdev->hal.rcr per chip
> in rtw_core_init().

I agree, and here is why the chip did it by hand, so we can pick how to
do it properly.

The value is the vendor 8723x one, 0x700060ce, plus BIT_APP_FCS, which
is needed because rtw88 advertises RX_INCLUDES_FCS for every chip. The
chip cannot just write the register: rtw_core_start() writes hal.rcr to
REG_RCR after power_on, which runs after phy_set_param(), so a write
that only touches the register is undone. Assigning hal.rcr is what
makes it survive, and fw.c and mac80211.c then keep toggling the same
cached value.

Doing it your way means a per-chip value in rtw_chip_info and the
assignment in rtw_core_init(). Two things I would want to get right:

  - the per-chip value has to keep BIT_APP_FCS, so it is not the same
    constant as the shared 8723x one;
  - rtw8703b and rtw8723d would keep the current default, so this
    changes nothing for them. Today __rtw8723x_mac_init() writes
    0x700060ce to REG_RCR and rtw_core_start() replaces it with the
    default, so that write does not survive anyway. Whether the shared
    value should reach those chips is a separate question and I think
    it needs its own patch, not this series.

If you prefer, I can put the core change in a small patch before the
chip series rather than in patch 3, so the chip series stays free of
core changes.

> Can you reuse the existing since they are the same?

For init_adaptive_ctrl, init_edca and init_retry_function, and also for
init_burst_pkt_len, which is the same block as the packet burst part of
rtw8703b_phy_set_param() down to the last write (8703b names BIT(7) as
BIT_MASK_TXQ_INIT, this file writes BIT(7)): the values are the same,
but there is nothing to call. The three ways:

  a. inline them into rtw8723b_phy_set_param() in the 8703b order, with
     the 8703b macro names. This makes the objection go away and patch 3
     reads like the two siblings. The per-step comments would move or
     be dropped.
  b. keep the named functions and give the reason for each. Smallest
     diff, but it does not really answer "why specific functions".
  c. move the common part into rtw8723x (a shared
     rtw8723x_phy_set_param() helper, or just the WLAN_* macros in
     rtw8723x.h) and call it from 8723b, 8703b and 8723d. Best answer
     to "they are the same", but it edits the two chips that are already
     in.

I lean to (a) or (c). Tell me which and I will do it that way.

> (Please quote the code; to reply to this, I need to switch to your
> patches again).
>
> I can guess you write u32 and then u8 for optimizing stack. Maybe,
> you can rename the names or change types. Just keep reverse X'mas
> order as possible as you can. One exception is dereference prior
> pointer (but still can change the code to fix reverse X'mas order).
> Another exception is put 'struct' declarations first, and then
> fundamental data type.
>
> Please try your ways to fit simple rules. Otherwise, if I accept this,
> it is hard to me to explain the reasons if other people ask me days
> later.

Here is the code you marked, from rtw8723b_reassert_rx_path():

	u32 rf_wlint_before;
	u32 rx_path_before;
	u32 fpga0_before;
	u8 sys_func_before;
	u8 rf_ctrl_before;

You are right about the cause, it is grouped by type and not by line
length. It was not for stack packing, the compiler decides the layout
either way; the grouping just read better to me, which is not a good
enough reason against a rule. The first assignment in the body is
sys_func_before, then rf_ctrl_before, fpga0_before, rx_path_before,
rf_wlint_before, so the block is reverse of use, not reverse of line
length.

Longest line first would put the two u8 names between the u32 names,
because of the length of the names, not the types:

	u32 rf_wlint_before;     21
	u32 rx_path_before;      20
	u8 sys_func_before;      20
	u8 rf_ctrl_before;       19
	u32 fpga0_before;        18

That is why I did not just reorder it. Taking your suggestion of
renaming, the type grouping and the length order can both hold:

	u32 rf_wlint_before;
	u32 rx_path_before;
	u32 fpga0_before;
	u8 sys_before;
	u8 rf_before;

If you prefer the plain longest first order with the current names, I
will do that instead, it is a two line move.

While I was in the file I applied the same check to every declaration
block, not only that one. Ten others are ordered by type rather than by
line length:

  rtw8723b_default_cck_index()       u8 swing before u8 i
  rtw8723b_phy_load_bb_tables()      rfe_def before chip
  rtw8723b_lck()                     ret after val_ctx and rf_val
  rtw8723b_iqk_tx_path_a()           sdio_iqk first
  rtw8723b_iqk_one_round()           a_ok before i
  rtw8723b_phy_calibration()         final_candidate second, after dm_info
  rtw8723b_pwr_track()               dm_info before efuse
  rtw8723b_coex_write_bb_sel_btg()   sys_func_before before readback
  rtw8723b_coex_set_rfe_type()       hci_type first, then aux; coex has
                                     to stay ahead of coex_rfe, which
                                     dereferences it
  rtw8723b_fill_txdesc_checksum()    words before checksum

Two of those (coex_set_rfe_type, and phy_calibration if the structs stay
together) fall under your exceptions and cannot be fully ordered without
renaming. So my question is whether you want all of them converted the
same way as reassert_rx_path(), including renaming where the types and
the length order disagree, or only the ones that can be fixed by moving
lines, and the type grouped ones documented as the exception.

What I would like to send in v5, once you say which way you want each:

  - the MAC init block: (a) inline, or (c) shared helper, and the
    missing hal.rxfltmap1 fixed either way;
  - the RCR in rtw_core_init() as you asked, in patch 3 or as a patch
    before the series, with the shared 8723x behaviour left alone;
  - fill_txdesc_checksum as a wrapper unless the non-inverted sum is
    deliberate;
  - the declaration order converted as far as the simple rules go.

On your last sentence, that the reason is hard to explain later if it is
not written down: whatever we change will carry its reason in the code
or in the commit message, not only in this mail. The same goes for the
two things I am asking to keep, cfg_ldo25() and the RESP_SIFS writes,
which will say why they are there.

I will rebuild, run sparse and smatch, and rerun the hardware suite and
the reload soak on the new tip before sending. The driver is still
station only, the five extra SDIO IDs are still untested, and the
firmware leave LPS stall is still there.

Best regards,
Luka Gejak