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

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

Luka Gejak <luka.gejak@linux.dev> 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.
> 
> This depends on the shared core handling sent as a separate series,
> "wifi: rtw88: preparations for RTL8723B/RTL8723BS". The driver is not
> built yet; the Kconfig and Makefile entries follow.

No need this paragraph.

> 
> The RTL8723B chip support is based on the initial work by
> Michael Straube <straube.linux@gmail.com>.
> Link: https://github.com/mistraube/rtw88/tree/rtl8723bs
> 
> Co-developed-by: Michael Straube <straube.linux@gmail.com>
> Signed-off-by: Michael Straube <straube.linux@gmail.com>
> Signed-off-by: Luka Gejak <luka.gejak@linux.dev>
> ---
>  drivers/net/wireless/realtek/rtw88/rtw8723b.c | 2938 +++++++++++++++++

As LOC is large, I think I can't address all flaws in single one review
iteration. Please also review patches yourself before sending out.
I think you're quite experienced now.

>  drivers/net/wireless/realtek/rtw88/rtw8723b.h |   12 +
>  2 files changed, 2950 insertions(+)
>  create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8723b.c
>  create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8723b.h
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8723b.c
> b/drivers/net/wireless/realtek/rtw88/rtw8723b.c
> new file mode 100644
> index 000000000000..9e53ba49def3
> --- /dev/null
> +++ b/drivers/net/wireless/realtek/rtw88/rtw8723b.c
> @@ -0,0 +1,2938 @@
> +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> +/*
> + * Copyright(c) 2007-2017  Realtek Corporation
> + * Copyright(c) Michael Straube <straube.linux@gmail.com>
> + * Copyright(c) 2024-2026 Luka Gejak <luka.gejak@linux.dev>
> + */
> +
> +#include <linux/unaligned.h>
> +#include "main.h"
> +#include "coex.h"
> +#include "fw.h"
> +#include "mac.h"
> +#include "phy.h"
> +/*
> + * Shares the receive PHY status layout, the SDIO aggregation burst fields
> + * and a few baseband registers with the RTL8703B; reuse that header.
> + */
> +#include "rtw8703b.h"
> +#include "rtw8723b.h"
> +#include "rtw8723b_table.h"
> +#include "sdio.h"
> +#include "tx.h"

In increasing alphabet order. 

[...]

> +/* 0x03a05611 is the normal RX path (staging and the BB table agree);
> + * 0x03a05600 is only an IQK temporary value and must not be reasserted.
> + */

Keep first line of block comment blank.

> +#define WLAN_RX_PATH_A_8723B                   0x03a05611
> +
> +#define ADDA_ON_VAL_8723B                      0x01c00014
> +
> +#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?

> +
> +#define WLAN_RX_FILTER0                        0xFFFF
> +#define WLAN_RX_FILTER1                        0x400
> +#define WLAN_RX_FILTER2                        0xFFFF
> +/* Keep BIT_APP_FCS: rtw88 advertises RX_INCLUDES_FCS for every chip, and
> + * without it mac80211 trims four bytes of real frame data.
> + */

Please review block comment style. (I'd not point out this afterward)

[...]

> +
> +static const struct rtw_pwr_seq_cmd trans_pre_enable_8723b[] = {
> +       /* unlock ISO/CLK/power control register */
> +       {REG_RSV_CTRL,
> +        RTW_PWR_CUT_ALL_MSK,
> +        RTW_PWR_INTF_ALL_MSK,
> +        RTW_PWR_ADDR_MAC,
> +        RTW_PWR_CMD_WRITE, 0xff, 0},
> +

Not prefer the blank line. (apply this rule to tables below)

> +       {TRANS_SEQ_END},
> +};
> +

[...]

> +
> +static void rtw8723b_efuse_grant(struct rtw_dev *rtwdev, bool on)
> +{
> +       /*
> +        * This chip wraps the common efuse power switch in a BT power cut
> +        * (0x6A[14] = 1) when granting access and a BT output isolation
> +        * (0x6A[15] = 1) when releasing it.
> +        */
> +       rtw_write8_set(rtwdev, 0x6b, on ? BIT(6) : BIT(7));

By comment above, I guess you can give proper names for 0x6b and bit 6 and 7.

> +
> +       rtw8723x_efuse_grant(rtwdev, on);
> +}
> +

[...]

> +static u8 rtw8723b_default_ofdm_index(struct rtw_dev *rtwdev)
> +{
> +       u8 i;
> +       u32 val32;
> +       u32 swing;

Reverse x'mas tree order. (Please apply to whole patchset)

> +
> +       swing = rtw_read32_mask(rtwdev, REG_OFDM_0_XA_TX_IQ_IMBALANCE, 0xffc00000);
> +
> +       for (i = 0; i < RTW_OFDM_SWING_TABLE_SIZE; i++) {
> +               val32 = rtw8723b_ofdm_swing_table[i];
> +
> +               if (val32 >= 0x100000)
> +                       val32 >>= 22;

I think shift 22 bits because of 0xffc00000. 

Add a name for 0xffc00000 and use __ffs() to get the shift.

> +
> +               if (val32 == swing)
> +                       break;
> +       }
> +
> +       if (i >= RTW_OFDM_SWING_TABLE_SIZE)
> +               i = 30;

A comment for 30.

/* 30, +0dB */

> +
> +       return i;
> +}
> +
> +static u8 rtw8723b_default_cck_index(struct rtw_dev *rtwdev)
> +{
> +       u8 i;
> +       u8 swing;
> +
> +       swing = rtw_read8(rtwdev, rtw8723b_cck_pwr_regs[0]);
> +
> +       for (i = 0; i < RTW_CCK_SWING_TABLE_SIZE; i++) {
> +               if (rtw8723b_cck_swing_table_ch1_ch13[i][0] == swing)
> +                       break;
> +       }
> +
> +       if (i >= RTW_CCK_SWING_TABLE_SIZE)
> +               i = 20;

A comment for 20.

> +
> +       return i;
> +}
> +
> +static void rtw8723b_pwrtrack_init(struct rtw_dev *rtwdev)
> +{
> +       struct rtw_dm_info *dm_info = &rtwdev->dm_info;
> +       u8 path;
> +
> +       dm_info->default_ofdm_index = rtw8723b_default_ofdm_index(rtwdev);
> +       dm_info->default_cck_index = rtw8723b_default_cck_index(rtwdev);
> +
> +       /* Clear the OFDM remnant per path: tracking keeps one per path. */
> +       for (path = RF_PATH_A; path < rtwdev->hal.rf_path_num; path++) {
> +               ewma_thermal_init(&dm_info->avg_thermal[path]);
> +               dm_info->delta_power_index[path] = 0;
> +               dm_info->txagc_remnant_ofdm[path] = 0;
> +       }
> +       dm_info->pwr_trk_triggered = false;
> +       dm_info->pwr_trk_init_trigger = true;
> +       dm_info->thermal_meter_k = rtwdev->efuse.thermal_meter_k;
> +       dm_info->txagc_remnant_cck = 0;
> +}
> +
> +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.

> +{
> +       return rtw_hci_type(rtwdev) == RTW_HCI_TYPE_SDIO;
> +}
> +

[...]

> +static void rtw8723b_phy_bb_config(struct rtw_dev *rtwdev)
> +{
> +       u8 xtal_cap;
> +
> +       /* Enable BB and RF */
> +       rtw_write16_set(rtwdev, REG_SYS_FUNC_EN,
> +                       BIT_FEN_EN_25_1 | BIT_FEN_BB_GLB_RST | BIT_FEN_BB_RSTB);
> +
> +       if (rtw_hci_type(rtwdev) == RTW_HCI_TYPE_USB)
> +               rtw_write32(rtwdev, REG_BB_SEL_BTG, 0x0);
> +       else
> +               rtw_write32(rtwdev, REG_BB_SEL_BTG, 0x280);
> +
> +       /* Full write: preserving spuriously set bits can wedge the RF bus. */
> +       rtw_write8(rtwdev, REG_RF_CTRL,
> +                  BIT_RF_EN | BIT_RF_RSTB | BIT_RF_SDM_RSTB);
> +       usleep_range(1000, 1100);

just fsleep() ?

> +       rtw_write_rf(rtwdev, RF_PATH_A, RF_WLINT, RFREG_MASK, 0x0780);
> +       rtw_write8(rtwdev, REG_SYS_FUNC_EN,
> +                  BIT_FEN_PPLL | BIT_FEN_PCIEA | BIT_FEN_DIO_PCIE |
> +                  BIT_FEN_BB_GLB_RST | BIT_FEN_BB_RSTB);
> +       rtw_write8(rtwdev, REG_AFE_CTRL1 + 1, 0x80);
> +
> +       xtal_cap = rtwdev->efuse.crystal_cap & 0x3f;
> +       rtw_write32_mask(rtwdev,  REG_AFE_CTRL3, BIT_MASK_XTAL,
> +                        xtal_cap | (xtal_cap << 6));
> +}
> +

[...]

> +
> +static void rtw8723b_phy_rf6052_config(struct rtw_dev *rtwdev)
> +{
> +       struct rtw_hal *hal = &rtwdev->hal;
> +       u32 intf_s, intf_oe, hssi_2;
> +       u32 val32, mask;
> +       u8 path;
> +
> +       for (path = RF_PATH_A; path < hal->rf_path_num; path++) {

[...]

> +       }
> +
> +       /* 3 Configuration of Tx Power Tracking */

This comment looks weird.

> +}
> +

[...]

> +
> +static void rtw8723b_init_network_type(struct rtw_dev *rtwdev)
> +{
> +       u32 val32;
> +
> +       val32 = rtw_read32(rtwdev, REG_CR);
> +       val32 = (val32 & ~MASK_NETTYPE) | _NETTYPE(NT_LINK_AP);
> +       rtw_write32(rtwdev, REG_CR, val32);

This is what I point above. Please reconsider this.

> +}
> +


> +static void rtw8723b_init_wmac_setting(struct rtw_dev *rtwdev)
> +{
> +       /* Override the default rcr filter for 8723B */
> +       rtwdev->hal.rcr = WLAN_RCR_CFG;

Why? The default value doesn't work to RTL8723B?

	rtwdev->hal.rcr = BIT_APP_FCS | BIT_APP_MIC | BIT_APP_ICV |
			  BIT_PKTCTL_DLEN | BIT_HTC_LOC_CTRL | BIT_APP_PHYSTS |
			  BIT_AB | BIT_AM | BIT_APM;


> +       rtw_write32(rtwdev, REG_RCR, rtwdev->hal.rcr);
> +
> +       rtw_write32(rtwdev, REG_MAR, 0xffffffff);
> +       rtw_write32(rtwdev, REG_MAR + 4, 0xffffffff);
> +
> +       rtw_write16(rtwdev, REG_RXFLTMAP2, WLAN_RX_FILTER2);
> +       rtw_write16(rtwdev, REG_RXFLTMAP1, WLAN_RX_FILTER1);
> +       rtw_write16(rtwdev, REG_RXFLTMAP0, WLAN_RX_FILTER0);
> +}
> +

[...]

> +
> +static void rtw8723b_init_antenna_selection(struct rtw_dev *rtwdev)
> +{
> +       /* BIT(7) lets the 8051 control antenna selection, BIT(1) is LED2_CM. */

Just define names for these bits?

> +       rtw_write8(rtwdev, REG_LEDCFG2, BIT(7) | BIT(1));
> +}
> +

[...]

> +static void rtw8723b_phy_set_param(struct rtw_dev *rtwdev)
> +{
> +       const struct rtw_chip_info *chip = rtwdev->chip;
> +       u32 val32;
> +
> +       rtw8723b_post_enable_flow(rtwdev);
> +
> +       rtw_load_table(rtwdev, chip->mac_tbl);
> +       rtw8723b_phy_bb_config(rtwdev);
> +       rtw8723b_phy_load_bb_tables(rtwdev);
> +       rtw8723b_phy_rf_config(rtwdev);
> +
> +       /* enable CCK and OFDM block */
> +       rtw_write32_set(rtwdev, REG_FPGA0_RFMOD, BIT_CCKEN | BIT_OFDMEN);
> +
> +       rtw8723b_init_tx_buffer_boundary(rtwdev);
> +       rtw8723b_init_page_boundary(rtwdev);
> +       rtw8723b_init_transfer_page_size(rtwdev);
> +       rtw8723b_init_driver_info_size(rtwdev);
> +       rtw8723b_init_network_type(rtwdev);
> +
> +       rtw8723b_init_wmac_setting(rtwdev);
> +
> +       rtw8723b_init_adaptive_ctrl(rtwdev);
> +       rtw8723b_init_edca(rtwdev);
> +       rtw8723b_init_retry_function(rtwdev);

So RTL8723B is very different from existing chips?

[...]

> +static void rtw8723b_reassert_rx_path(struct rtw_dev *rtwdev)
> +{
> +       u32 rf_wlint_before;
> +       u32 rx_path_before;
> +       u32 fpga0_before;
> +       u8 sys_func_before;
> +       u8 rf_ctrl_before;

Reverse X'mas tree

> +
> +       if (!rtw8723b_sdio_needs_rx_path_fix(rtwdev))
> +               return;
> +
> +       sys_func_before = rtw_read8(rtwdev, REG_SYS_FUNC_EN);
> +       rf_ctrl_before = rtw_read8(rtwdev, REG_RF_CTRL);
> +       fpga0_before = rtw_read32(rtwdev, REG_FPGA0_RFMOD);
> +       rx_path_before = rtw_read32(rtwdev, REG_BB_RX_PATH_11N);
> +       rf_wlint_before = rtw_read_rf(rtwdev, RF_PATH_A, RF_WLINT, RFREG_MASK);
> +
> +       if ((sys_func_before & WLAN_SYS_FUNC_BB_ENABLE) !=
> +           WLAN_SYS_FUNC_BB_ENABLE) {
> +               rtw_write8_set(rtwdev, REG_SYS_FUNC_EN,
> +                              WLAN_SYS_FUNC_BB_ENABLE);
> +       }

No need brace for single line branch. (apply this to whole patches)

[...]

> +const struct rtw_chip_info rtw8723b_hw_spec = {

Please take rtw8822c_hw_spec as template to align the style including
blank lines and order.

> +       .ops = &rtw8723b_ops,
> +       .id = RTW_CHIP_TYPE_8723B,
> +       .fw_name = "rtw88/rtw8723b_fw.bin",
> +       .wlan_cpu = RTW_WCPU_8051,
> +       .tx_pkt_desc_sz = 40,
> +       .tx_buf_desc_sz = 16,
> +       .rx_pkt_desc_sz = 24,
> +       .rx_buf_desc_sz = 8,
> +       .phy_efuse_size = 512,
> +       .log_efuse_size = 512,
> +       .ptct_efuse_size = 15,
> +
> +       .txff_size = 32768,
> +       .rxff_size = 16384,
> +       .rsvd_drv_pg_num = 8,
> +
> +       .txgi_factor = 1,
> +       .is_pwr_by_rate_dec = true,
> +       .rx_ldpc = false,
> +       .tx_stbc = false,
> +
> +       .max_power_index = 0x3f,
> +
> +       .csi_buf_pg_num = 0,
> +       .band = RTW_BAND_2G,
> +       .page_size = TX_PAGE_SIZE,
> +
> +       .dig_min = 0x20,
> +       .usb_tx_agg_desc_num = 1,
> +
> +       /*
> +        * The firmware reports id 0xfd instead of C2H_HW_FEATURE_REPORT, so
> +        * the hardware feature report is not supported on this chip.
> +        */
> +       .hw_feature_report = false,
> +
> +       .c2h_ra_report_size = 4,        /* rtw88/rtw8723b_fw.bin v41 emits the
> +                                        * legacy 8051 4-byte rate report
> +                                        * (rate_sgi, mac_id, byte2, status).
> +                                        * Setting this to 7, like the upstream
> +                                        * default, caused every C2H_RA_REPORT
> +                                        * to be dropped with
> +                                        * "short ra report c2h length 4
> +                                        * expected 7" on every connect attempt
> +                                        * so the firmware-driven rate adaptation
> +                                        * feedback path
> +                                        * never updated si->ra_report. The
> +                                        * legacy 8051 8723b/8703b/8723d firmware
> +                                        * uses the same 4-byte format as the
> +                                        * older 8821a/8812a chips. byte4..bw fall
> +                                        * back to the per-station defaults in
> +                                        * rtw_fw_ra_report_iter(), which matches
> +                                        * what those chips already do safely.
> +                                        */

I don't know if these comment is actually useful for people?

> +       /* See commit c7706b1173c7 ("wifi: rtw88: Enable data rate fallback for
> +        * older chips").
> +        */

Why do you need this? Just by 'git blame' ?

> +       .old_datarate_fb_limit = true,
> +
> +       .path_div_supported = false,
> +       .ht_supported = true,
> +       .vht_supported = false,
> +       .lps_deep_mode_supported = 0,
> +
> +       .sys_func_en = 0xfd,
> +       .pwr_on_seq = card_enable_flow_8723b,
> +       .pwr_off_seq = card_disable_flow_8723b,
> +       .page_table = page_table_8723b,
> +
> +       .rqpn_table = rqpn_table_8723b,
> +       /* same shared table as the sibling rtw8703b and rtw8723d */
> +       .prioq_addrs = &rtw8723x_common.prioq_addrs,
> +
> +       /* used only in pci.c, not needed for SDIO devices */
> +       .intf_table = NULL,
> +
> +       .dig = rtw8723x_common.dig,
> +       .dig_cck = rtw8723x_common.dig_cck,
> +
> +       .rf_sipi_addr = {0x840, 0x844},
> +       .rf_sipi_read_addr = rtw8723x_common.rf_sipi_addr,
> +
> +       .fix_rf_phy_num = 2,
> +
> +       /* This chip has no LTE coex registers. */
> +       .ltecoex_addr = NULL,
> +
> +       .mac_tbl = &rtw8723b_mac_tbl,
> +       .agc_tbl = &rtw8723b_agc_tbl,
> +       .bb_tbl = &rtw8723b_bb_tbl,
> +       .rf_tbl = {&rtw8723b_rf_a_tbl},
> +
> +       .rfe_defs = rtw8723b_rfe_defs,
> +       .rfe_defs_size = ARRAY_SIZE(rtw8723b_rfe_defs),
> +       .iqk_threshold = 8,
> +       .ampdu_density = IEEE80211_HT_MPDU_DENSITY_16,
> +       .max_scan_ie_len = IEEE80211_MAX_DATA_LEN,
> +
> +       .coex_para_ver = 20180201,      /* glcoex_ver_date_8723b_1ant */
> +       .bt_desired_ver = 0x6d,
> +       .scbd_support = false,
> +       .new_scbd10_def = true,
> +       .ble_hid_profile_support = false,
> +       .wl_mimo_ps_support = false,
> +       .pstdma_type = COEX_PSTDMA_FORCE_LPSOFF,
> +       .bt_rssi_type = COEX_BTRSSI_RATIO,
> +       .ant_isolation = 15,
> +       .rssi_tolerance = 2,
> +       .wl_rssi_step = wl_rssi_step_8723b,
> +       .bt_rssi_step = bt_rssi_step_8723b,
> +       .table_sant_num = ARRAY_SIZE(table_sant_8723b),
> +       .table_sant = table_sant_8723b,
> +       .table_nsant_num = ARRAY_SIZE(table_nsant_8723b),
> +       .table_nsant = table_nsant_8723b,
> +       .tdma_sant_num = ARRAY_SIZE(tdma_sant_8723b),
> +       .tdma_sant = tdma_sant_8723b,
> +       .tdma_nsant_num = ARRAY_SIZE(tdma_nsant_8723b),
> +       .tdma_nsant = tdma_nsant_8723b,
> +       .wl_rf_para_num = ARRAY_SIZE(rf_para_tx_8723b),
> +       .wl_rf_para_tx = rf_para_tx_8723b,
> +       .wl_rf_para_rx = rf_para_rx_8723b,
> +       .bt_afh_span_bw20 = 0x20,
> +       .bt_afh_span_bw40 = 0x30,
> +       .afh_5g_num =  ARRAY_SIZE(afh_5g_8723b),
> +       .afh_5g = afh_5g_8723b,
> +       /* BTG_SEL is driven by the cardemu_to_act power sequence instead. */
> +       .btg_reg = NULL,
> +
> +       .coex_info_hw_regs_num = 0,
> +       .coex_info_hw_regs = NULL,
> +};
> +EXPORT_SYMBOL(rtw8723b_hw_spec);
> +
> +MODULE_FIRMWARE("rtw88/rtw8723b_fw.bin");
> +
> +MODULE_AUTHOR("Luka Gejak <luka.gejak@linux.dev>");
> +MODULE_AUTHOR("Michael Straube <straube.linux@gmail.com>");
> +MODULE_DESCRIPTION("Realtek 802.11n wireless 8723b driver");
> +MODULE_LICENSE("Dual BSD/GPL");
> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8723b.h
> b/drivers/net/wireless/realtek/rtw88/rtw8723b.h
> new file mode 100644
> index 000000000000..2fe00e709620
> --- /dev/null
> +++ b/drivers/net/wireless/realtek/rtw88/rtw8723b.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
> +/* Copyright(c) 2018-2019  Realtek Corporation

Please let copyright consistent. 

> + */
> +
> +#ifndef __RTW8723B_H__
> +#define __RTW8723B_H__
> +
> +#include "rtw8723x.h"
> +
> +extern const struct rtw_chip_info rtw8723b_hw_spec;
> +
> +#endif
> --
> 2.55.0