Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx> wrote:
> On 16/08/2024 09:06, Ping-Ke Shih wrote:
> > Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx> wrote:
> >> + 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);
> >> + pkt_stat->bw = GET_RX_DESC_BW(rx_desc);
> >
> > More and more chips use these macros. Please add a patch using struct to
> > access these fields. More, query_rx_desc() are very similar across chips,
> > please move them to mac.c or phy.c as a common function.
> >
>
> Why not rx.c?
Also fine to me.
> > Can you collect undefined register addresses? I can try to lookup them in one go.
> >
>
> Here are the addresses I could find and a few bits/bit masks:
>
> 0x765
#define REG_GNT_BT 0x765
#define BIT_PTA_SW_CTL GENMASK(4, 3)
> 0x90c
#define REG_DAC_RSTB 0x90c
> 0x978
> 31, 0x03ff8000, 0x000007ff
> 0x97c
> 31
> 0x980
> 0x984
#define REG_IQK_COM00 0x978
#define REG_IQK_COM32 0x97c
#define REG_IQK_COM64 0x980
#define REG_IQK_COM96 0x984
> 0xa0a
#define REG_CCK_PD_TH 0xa0a
> 0xc00
> 0xf
> 0xe00
#define REG_3WIRE_SWA 0xc00
#define REG_3WIRE_SWB 0xe00
(0xc00 page for path A, 0xe00 page for path B)
> 0xc5c
> 0xe5c
> GENMASK(26, 24)
#define REG_CK_MONHA 0xc5c
#define REG_CK_MONHB 0xe5c
> 0xce8
> 31, 0x3fff0000
> 0xee8
#define REG_INTPO_SETA 0xce8
#define REG_INTPO_SETB 0xee8
> 0xd00
> 10, 11, 12, 0x07ff0000
#define REG_IQKA_END 0xd00
> 0xd40
> 10, 11, 12
#define REG_IQKB_END 0xd40
> GENMASK(26, 24)
> 0xe80
#define REG_TXTONEB 0xe80
> 0xe84
#define REG_RXTONEB 0xe84
> 0xe88
#define REG_TXPITMB 0xe88
> 0xe8c
#define REG_RXPITMB 0xe8c
> 0xe90
#define REG_PREDISTB 0xe90
> 7
> 0xe94
#define REG_INIDLYB 0xe94
> 0
> 0xec4
> 18, 29
#define REG_BPBDB 0xec4
> 0xec8
> 29
#define REG_PHYTXONB 0xec8
> 0xecc
#define REG_IQKYB 0xecc
> 0xed4
#define REG_IQKXB 0xed4
> 0xf008
> 3, 4
> 0xf050
I can't find these two, but just follow comments above.
#define REG_USB_MOD 0xf008
#define REG_USB3_RXITV 0xf050
>
> And some RF registers: 0x8, 0x58, 0x65, 0x8f
>
#define RF_DTXLOK 0x08 (already existing)
#define RF_TXMOD 0x58
#define RF_TXA_PREPAD 0x65
#define RF_RXBB2 0x8f
> [...]
>
> >> +
> >> +const struct rtw_chip_info rtw8812a_hw_spec = {
> >
> > Is it possible moving 8812a to individual file?
> > Since you have rtw8812au.c and rtw8821au.c.
> >
>
> I think it is possible. But most of the code is common to both chips.
> Only the IQ calibration could be moved.
Yep, depend on how much IQK code echo chip has.
>
> Another possibility is to move rtw8812au.c into rtw8821au.c and have
> only one module handle both chips.
Prefer two modules as was. That is clear to people we have two chips.
> >> +
> >> +#define WLAN_TBTT_TIME (WLAN_TBTT_PROHIBIT |\
> >> + (WLAN_TBTT_HOLD_TIME << BIT_SHIFT_TBTT_HOLD_TIME_AP))
> >> +
> >> +#define WLAN_NAV_CFG (WLAN_RDG_NAV | (WLAN_TXOP_NAV << 16))
> >> +#define WLAN_RX_TSF_CFG (WLAN_CCK_RX_TSF | (WLAN_OFDM_RX_TSF) << 8)
> >> +#define WLAN_PRE_TXCNT_TIME_TH 0x1E4
> >> +
> >> +struct rtw8821a_phy_status_rpt {
> >> + /* DWORD 0 */
> >> + u8 gain_trsw[2]; /* path-A and path-B { TRSW, gain[6:0] } */
> >> + u8 chl_num_lsb; /* channel number[7:0] */
> >> +#ifdef __LITTLE_ENDIAN
> >> + u8 chl_num_msb:2; /* channel number[9:8] */
> >> + u8 sub_chnl:4; /* sub-channel location[3:0] */
> >> + u8 r_rfmod:2; /* RF mode[1:0] */
> >> +#else
> >> + u8 r_rfmod:2;
> >> + u8 sub_chnl:4;
> >> + u8 chl_num_msb:2;
> >> +#endif
> >
> > Not prefer "#ifdef __LITTLE_ENDIAN" style, because we have never verified
> > big endian part. Prefer to define mask and access them via u8_get_bits() and
> > its friends.
> >
>
> None of the members inside #ifdef __LITLLE_ENDIAN are used
> in this driver. Do I still have to define all the masks?
I prefer to define all masks, because the names have been there. Adding a name
in the future may be painful.
>
> >
> >> +
> >> + /* DWORD 1 */
> >> + u8 pwdb_all; /* CCK signal quality / OFDM pwdb all */
> >> + s8 cfosho[2]; /* CCK AGC report and CCK_BB_Power */
> >> + /* OFDM path-A and path-B short CFO */
> >> +#ifdef __LITTLE_ENDIAN
> >> + u8 resvd_0:6;
> >> + u8 bt_rf_ch_msb:2; /* 8812A: 2'b0 8814A: bt rf channel keep[7:6] */
> >
> > Will you share this struct with 8814A? If not please remove comments related to 8814A.
> >
>
> Yes, it uses the same struct.
But struct name is rtw8821a_phy_status_rpt that prefix is rtw8821a.
> >> +
> >> +#define REG_SYS_CTRL 0x000
> >> +#define BIT_FEN_EN BIT(26)
> >> +#define REG_APS_FSMCO 0x04
> >> +#define APS_FSMCO_MAC_ENABLE BIT(8)
> >> +#define APS_FSMCO_MAC_OFF BIT(9)
> >> +#define APS_FSMCO_HW_POWERDOWN BIT(15)
> >> +#define REG_ACLK_MON 0x3e
> >> +#define REG_RF_B_CTRL 0x76
> >> +#define REG_HIMR0 0xb0
> >> +#define REG_HISR0 0xb4
> >> +#define REG_HIMR1 0xb8
> >> +#define REG_HISR1 0xbc
> >
> > [...]
> >
> > Can't we move all of these registers (including what I delete) into reg.h?
> >
>
> I think so.
I remember we were afraid that definitions of registers for each are different,
so put them in chip specific header file. But now I feel most registers can be
shared, and easy to read code because we need spending extra time to disambiguate
duplicate definitions.
