> -----Original Message-----
> From: Fiona Klute <fiona.klute@xxxxxx>
> Sent: Friday, February 2, 2024 8:11 PM
> To: linux-wireless@xxxxxxxxxxxxxxx; Ping-Ke Shih <pkshih@xxxxxxxxxxx>
> Cc: Kalle Valo <kvalo@xxxxxxxxxx>; Ulf Hansson <ulf.hansson@xxxxxxxxxx>; linux-mmc@xxxxxxxxxxxxxxx; Pavel
> Machek <pavel@xxxxxx>; Ondřej Jirman <megi@xxxxxx>; Fiona Klute <fiona.klute@xxxxxx>
> Subject: [PATCH 5/9] wifi: rtw88: Add rtw8703b.c
>
> This is the main source for the new rtw88_8703b chip driver.
>
> Signed-off-by: Fiona Klute <fiona.klute@xxxxxx>
> ---
>
> rtw8703b_read_efuse retrieves the MAC address from DT, if
> available. The code is not tied to any particular hardware, but
> required to get a persistent address on the Pinephone. Do I need to
> add a DT binding for this? Also, I think this could be moved into
> rtw_chip_efuse_info_setup(), in preference to falling back to a random
> MAC if there isn't a valid one in EFUSE. Would that be acceptable? If
> yes, should EFUSE or DT take priority?
>
> All the RTL8723CS EFUSE samples I've seen so far contain almost
> entirely invalid data (all 0xff, except rtl_id, afe, and
> thermal_meter), so I've added fallbacks in the EFUSE parser. In some
> cases they alter specific bits so parsing in rtw_chip_efuse_info_setup
> will get the right results. I'm not sure if this is the best approach:
> The good part is that it works without changing the core EFUSE code,
> the negative is that it's not visible to the core code that a fallback
> has been applied. What do you think?
I think efuse take priority, but you have said most are invalid data, so
you can write a rule to determine efuse is valid before using them. If
invalid, just use DT.
Sorry, I'm not familiar with DT, could you show me an example of DT node?
>
> drivers/net/wireless/realtek/rtw88/rtw8703b.c | 2106 +++++++++++++++++
> 1 file changed, 2106 insertions(+)
> create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8703b.c
>
> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8703b.c
> b/drivers/net/wireless/realtek/rtw88/rtw8703b.c
> new file mode 100644
> index 0000000000..ac9b1bf6ea
> --- /dev/null
> +++ b/drivers/net/wireless/realtek/rtw88/rtw8703b.c
> @@ -0,0 +1,2106 @@
> +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> +/* Copyright Fiona Klute <fiona.klute@xxxxxx> */
> +
> +#include <linux/of_net.h>
> +#include "main.h"
> +#include "coex.h"
> +#include "debug.h"
> +#include "mac.h"
> +#include "phy.h"
> +#include "reg.h"
> +#include "rx.h"
> +#include "rtw8703b.h"
> +#include "rtw8703b_tables.h"
> +#include "rtw8723x.h"
> +
> +#define GET_RX_DESC_BW(rxdesc) \
> + (le32_get_bits(*((__le32 *)(rxdesc) + 0x04), GENMASK(31, 24)))
use struct and le32_get_bits() directly.
> +
> +#define BIT_MASK_TXQ_INIT (BIT(7))
> +#define WLAN_RL_VAL 0x3030
> +/* disable BAR */
> +#define WLAN_BAR_VAL 0x0201ffff
> +#define WLAN_PIFS_VAL 0
> +#define WLAN_RX_PKT_LIMIT 0x18
> +#define WLAN_SLOT_TIME 0x09
> +#define WLAN_SPEC_SIFS 0x100a
> +#define WLAN_MAX_AGG_NR 0x1f
> +#define WLAN_AMPDU_MAX_TIME 0x70
> +
> +/* unit is 32us */
> +#define TBTT_PROHIBIT_SETUP_TIME 0x04
> +#define TBTT_PROHIBIT_HOLD_TIME 0x80
> +#define TBTT_PROHIBIT_HOLD_TIME_STOP_BCN 0x64
> +
> +/* raw pkt_stat->drv_info_sz is in unit of 8-bytes */
> +#define RX_DRV_INFO_SZ_UNIT_8703B 8
> +
> +#define TRANS_SEQ_END \
> + { \
> + 0xFFFF, \
> + RTW_PWR_CUT_ALL_MSK, \
> + RTW_PWR_INTF_ALL_MSK, \
> + 0, \
> + RTW_PWR_CMD_END, 0, 0}
nit: This looks a little odd, not like others of trans_pre_enable_8703b[].
How about this:
#define TRANS_SEQ_END \
{0xFFFF, \
RTW_PWR_CUT_ALL_MSK, \
RTW_PWR_INTF_ALL_MSK, \
0, \
RTW_PWR_CMD_END, 0, 0}
[...]
> +static const u8 rtw8703b_cck_swing_table[][16] = {
> + {0x44, 0x42, 0x3C, 0x33, 0x28, 0x1C, 0x13, 0x0B, 0x05, 0x02,
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}, /*-16dB*/
nit: align "{" ?
{0x44, 0x42, 0x3C, 0x33, 0x28, 0x1C, 0x13, 0x0B, 0x05, 0x02,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00}, /*-16dB*/
[...]
> +
> +/* Default power index table for RTL8703B, used if EFUSE does not
> + * contain valid data. Replaces EFUSE data from offset 0x10 (start of
> + * txpwr_idx_table).
> + */
> +static const u8 rtw8703b_txpwr_idx_table[] = {
> + 0x2D, 0x2D, 0x2D, 0x2D, 0x2D, 0x2D,
> + 0x2D, 0x2D, 0x2D, 0x2D, 0x2D, 0x02
> +};
> +
> +#define RTW8703B_TXPWR_IDX_TABLE_LEN ARRAY_SIZE(rtw8703b_txpwr_idx_table)
nit: This definition seems not save much.
> +
> +#define DBG_EFUSE_FIX(name) \
suggest to not hide 'rtwdev'
> + rtw_dbg(rtwdev, RTW_DBG_EFUSE, "Fixed invalid EFUSE value: " \
> + # name "=0x%x\n", rtwdev->efuse.name)
a blank line?
> +static int rtw8703b_read_efuse(struct rtw_dev *rtwdev, u8 *log_map)
> +{
> + struct rtw_efuse *efuse = &rtwdev->efuse;
> + int ret = rtw8723x_read_efuse(rtwdev, log_map);
I prefer
int ret;
ret = rtw8723x_read_efuse(rtwdev, log_map);
if (ret)
return ret;
> +
> + if (ret != 0)
> + return ret;
> +
> +#ifdef CONFIG_OF
> + /* Prefer MAC from DT, if available. On some devices like the
> + * Pinephone that might be the only way to get a valid MAC.
> + */
> + struct device_node *node = rtwdev->dev->of_node;
Should move this statement to topmost of this function? no compiler warning?
Or, make an individual function to read mac addr from DT?
> +
> + if (node) {
> + ret = of_get_mac_address(node, efuse->addr);
> + if (ret == 0) {
> + rtw_dbg(rtwdev, RTW_DBG_EFUSE,
> + "got wifi mac address from DT: %pM\n",
> + efuse->addr);
> + }
> + }
> +#endif /* CONFIG_OF */
> +
> + /* If TX power index table in EFUSE is invalid, fall back to
> + * built-in table.
> + */
> + u8 *pwr = (u8 *)efuse->txpwr_idx_table;
> + bool valid = false;
I tend to move these declaration to top of this function too, but not sure why
compiler also doesn't warn this in my side. Seemingly kernel changes default
compiler flags?
[...]
> +static void rtw8703b_set_channel_bb(struct rtw_dev *rtwdev, u8 channel, u8 bw,
> + u8 primary_ch_idx)
> +{
> + const struct rtw_backup_info *cck_dfir;
> + int i;
> +
> + cck_dfir = channel <= 13 ? cck_dfir_cfg[0] : cck_dfir_cfg[1];
> +
> + for (i = 0; i < CCK_DFIR_NR_8703B; i++, cck_dfir++)
> + rtw_write32(rtwdev, cck_dfir->reg, cck_dfir->val);
> +
> + switch (bw) {
> + case RTW_CHANNEL_WIDTH_20:
> + rtw_write32_mask(rtwdev, REG_FPGA0_RFMOD, BIT_MASK_RFMOD, 0x0);
> + rtw_write32_mask(rtwdev, REG_FPGA1_RFMOD, BIT_MASK_RFMOD, 0x0);
> + rtw_write32_mask(rtwdev, REG_OFDM0_TX_PSD_NOISE,
> + GENMASK(31, 20), 0x0);
> + rtw_write32(rtwdev, REG_BBRX_DFIR, 0x4A880000);
> + rtw_write32(rtwdev, REG_OFDM0_A_TX_AFE, 0x19F60000);
> + break;
> + case RTW_CHANNEL_WIDTH_40:
> + rtw_write32_mask(rtwdev, REG_FPGA0_RFMOD, BIT_MASK_RFMOD, 0x1);
> + rtw_write32_mask(rtwdev, REG_FPGA1_RFMOD, BIT_MASK_RFMOD, 0x1);
> + rtw_write32(rtwdev, REG_BBRX_DFIR, 0x40100000);
> + rtw_write32(rtwdev, REG_OFDM0_A_TX_AFE, 0x51F60000);
> + rtw_write32_mask(rtwdev, REG_CCK0_SYS, BIT_CCK_SIDE_BAND,
> + (primary_ch_idx == RTW_SC_20_UPPER ? 1 : 0));
unnecessary parenthesis around ? :
> + rtw_write32_mask(rtwdev, REG_OFDM_FA_RSTD_11N, 0xC00,
> + primary_ch_idx == RTW_SC_20_UPPER ? 2 : 1);
> +
> + rtw_write32_mask(rtwdev, REG_BB_PWR_SAV5_11N, GENMASK(27, 26),
> + primary_ch_idx == RTW_SC_20_UPPER ? 1 : 2);
> + break;
> + default:
> + break;
> + }
> +}
> +
[...]
> +static void rtw8703b_query_rx_desc(struct rtw_dev *rtwdev, u8 *rx_desc,
> + struct rtw_rx_pkt_stat *pkt_stat,
> + struct ieee80211_rx_status *rx_status)
> +{
> + struct ieee80211_hdr *hdr;
> + u32 desc_sz = rtwdev->chip->rx_pkt_desc_sz;
> + u8 *phy_status = NULL;
> +
> + memset(pkt_stat, 0, sizeof(*pkt_stat));
> +
> + pkt_stat->phy_status = GET_RX_DESC_PHYST(rx_desc);
> + pkt_stat->icv_err = GET_RX_DESC_ICV_ERR(rx_desc);
> + pkt_stat->crc_err = GET_RX_DESC_CRC32(rx_desc);
> + pkt_stat->decrypted = !GET_RX_DESC_SWDEC(rx_desc) &&
> + GET_RX_DESC_ENC_TYPE(rx_desc) != RX_DESC_ENC_NONE;
> + pkt_stat->is_c2h = GET_RX_DESC_C2H(rx_desc);
> + pkt_stat->pkt_len = GET_RX_DESC_PKT_LEN(rx_desc);
> + pkt_stat->drv_info_sz = GET_RX_DESC_DRV_INFO_SIZE(rx_desc);
> + pkt_stat->shift = GET_RX_DESC_SHIFT(rx_desc);
> + pkt_stat->rate = GET_RX_DESC_RX_RATE(rx_desc);
> + pkt_stat->cam_id = GET_RX_DESC_MACID(rx_desc);
> + pkt_stat->ppdu_cnt = 0;
> + pkt_stat->tsf_low = GET_RX_DESC_TSFL(rx_desc);
Could you add a separate patch to convert these macros to struct style?
It is fine to keep as it was, and do this conversion afterward.
[...]
> +static
> +void rtw8703b_iqk_fill_a_matrix(struct rtw_dev *rtwdev, const s32 result[])
> +{
> + s32 oldval_1;
> + s32 x, y;
> + s32 tx1_a, tx1_a_ext;
> + s32 tx1_c, tx1_c_ext;
> + u32 tmp_rx_iqi = 0x40000100 & GENMASK(31, 16);
reverse X'mas tree order
> +
> +static void rtw8703b_phy_calibration(struct rtw_dev *rtwdev)
> +{
> + /* For some reason path A is called S1 and B S0 in shared
> + * rtw88 calibration data.
> + */
> + struct rtw_dm_info *dm_info = &rtwdev->dm_info;
> + s32 result[IQK_ROUND_SIZE][IQK_NR];
> + struct rtw8723x_iqk_backup_regs backup;
> + u8 i, j;
> + u8 final_candidate = IQK_ROUND_INVALID;
> + bool good;
reverse X'mas tree order
> +MODULE_FIRMWARE("rtw88/rtw8703b_wow_fw.bin");
Just curious. Have you tried WOW for this chip?
