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

[Ping-Ke Shih]

Luka Gejak <luka.gejak@xxxxxxxxx> wrote:
> >> +#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.

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.

> 
> >> +       /* 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.
> 
> 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.
> 
> 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.

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

> 
> >> +       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.

Can you reuse the existing since they are the same?

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.)

> 
> 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.

(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.

Ping-Ke