diff mbox series

[RFC] rtlwifi: rtl8821ae: Use inline routines rather than macros for descriptor word 0

Message ID 20190506173916.16486-1-Larry.Finger@lwfinger.net (mailing list archive)
State RFC
Delegated to: Kalle Valo
Headers show
Series [RFC] rtlwifi: rtl8821ae: Use inline routines rather than macros for descriptor word 0 | expand

Commit Message

Larry Finger May 6, 2019, 5:39 p.m. UTC 
The driver uses complicated macros to set parts of word 0 of the TX and RX
descriptors. These are changed into inline routines.

Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---

Kalle,

Based on your comment on how much you dislike those "byte macros", I have
converted a few of them from rtl8821ae into static inline functions.

Is this what you had in mind, and do you consider these changes to
improve the code?

These routines still need to mask the value before the ! operation with
the masked original value.

Thanks,
Larry
---

 .../wireless/realtek/rtlwifi/rtl8821ae/trx.c  | 33 ++++----
 .../wireless/realtek/rtlwifi/rtl8821ae/trx.h  | 80 ++++++++++++++-----
 2 files changed, 78 insertions(+), 35 deletions(-)

Comments

Stanislaw Gruszka May 7, 2019, 12:36 p.m. UTC | #1 
On Mon, May 06, 2019 at 12:39:16PM -0500, Larry Finger wrote:
> The driver uses complicated macros to set parts of word 0 of the TX and RX
> descriptors. These are changed into inline routines.

Why not use le32p_replace_bits() like rtw88 in 
drivers/net/wireless/realtek/rtw88/tx.h ?

Stanislaw
Kalle Valo May 28, 2019, 12:08 p.m. UTC | #2 
Larry Finger <Larry.Finger@lwfinger.net> writes:

> The driver uses complicated macros to set parts of word 0 of the TX and RX
> descriptors. These are changed into inline routines.
>
> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> ---
>
> Kalle,
>
> Based on your comment on how much you dislike those "byte macros", I have
> converted a few of them from rtl8821ae into static inline functions.
>
> Is this what you had in mind, and do you consider these changes to
> improve the code?

[...]

> --- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/trx.h
> +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/trx.h
> @@ -14,25 +14,67 @@
>  #define USB_HWDESC_HEADER_LEN			40
>  #define CRCLENGTH						4
>  
> -#define SET_TX_DESC_PKT_SIZE(__pdesc, __val)		\
> -	SET_BITS_TO_LE_4BYTE(__pdesc, 0, 16, __val)
> -#define SET_TX_DESC_OFFSET(__pdesc, __val)			\
> -	SET_BITS_TO_LE_4BYTE(__pdesc, 16, 8, __val)
> -#define SET_TX_DESC_BMC(__pdesc, __val)				\
> -	SET_BITS_TO_LE_4BYTE(__pdesc, 24, 1, __val)
> -#define SET_TX_DESC_HTC(__pdesc, __val)				\
> -	SET_BITS_TO_LE_4BYTE(__pdesc, 25, 1, __val)
> -#define SET_TX_DESC_LAST_SEG(__pdesc, __val)		\
> -	SET_BITS_TO_LE_4BYTE(__pdesc, 26, 1, __val)
> -#define SET_TX_DESC_FIRST_SEG(__pdesc, __val)		\
> -	SET_BITS_TO_LE_4BYTE(__pdesc, 27, 1, __val)
> -#define SET_TX_DESC_LINIP(__pdesc, __val)			\
> -	SET_BITS_TO_LE_4BYTE(__pdesc, 28, 1, __val)
> -#define SET_TX_DESC_OWN(__pdesc, __val)				\
> -	SET_BITS_TO_LE_4BYTE(__pdesc, 31, 1, __val)
> -
> -#define GET_TX_DESC_OWN(__pdesc)					\
> -	LE_BITS_TO_4BYTE(__pdesc, 31, 1)
> +/* Set packet size (16 bits) in TX descriptor word 0 */
> +static inline void set_tx_desc_pkt_size(__le32 *__pdesc, u16 __val)
> +{
> +	*__pdesc = cpu_to_le32((le32_to_cpu(*__pdesc) & ~GENMASK(0, 15)) |
> +			       __val);
> +}

This was not exactly what I had mind. My point was that the firmware
command or event should be handled as a complete structure, not parsed
one (or four) bytes at a time. To show what I mean here's a random
example from iwlwifi:

struct iwl_alive_resp {
	u8 ucode_minor;
	u8 ucode_major;
	__le16 reserved1;
	u8 sw_rev[8];
	u8 ver_type;
	u8 ver_subtype;			/* not "9" for runtime alive */
	__le16 reserved2;
	__le32 log_event_table_ptr;	/* SRAM address for event log */
	__le32 error_event_table_ptr;	/* SRAM address for error log */
	__le32 timestamp;
	__le32 is_valid;
} __packed;

This a nice, clean and robust way both use AND document an event coming
from the firmware. And the driver can pass around 'struct iwl_alive_res
*resp' pointer to other functions to make it easy to understand what
event the buffer contains. Compared to rtlwifi where there's just 'u8
buf[]' and nobody has any clue what it contains, and need to spend at
least five minutes figuring that out everytime they are looking at a
function.

But to be honest I'm not sure if it's worth trying to cleanup rtlwifi,
it really would need a full rewrite to become a clean driver. IMHO much
better to put more effort on rtl8xxxxu and rtw88, which both already are
clean drivers.
diff mbox series

Patch

diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/trx.c b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/trx.c
index 7b6faf38e09c..2ad33cfb1656 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/trx.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/trx.c
@@ -703,8 +703,8 @@  void rtl8821ae_tx_fill_desc(struct ieee80211_hw *hw,
 	if (firstseg) {
 		if (rtlhal->earlymode_enable) {
 			SET_TX_DESC_PKT_OFFSET(pdesc, 1);
-			SET_TX_DESC_OFFSET(pdesc, USB_HWDESC_HEADER_LEN +
-					   EM_HDR_LEN);
+			set_tx_desc_offset((__le32 *)pdesc,
+					   USB_HWDESC_HEADER_LEN + EM_HDR_LEN);
 			if (ptcb_desc->empkt_num) {
 				RT_TRACE(rtlpriv, COMP_SEND, DBG_TRACE,
 					 "Insert 8 byte.pTcb->EMPktNum:%d\n",
@@ -713,7 +713,8 @@  void rtl8821ae_tx_fill_desc(struct ieee80211_hw *hw,
 					 (u8 *)(skb->data));
 			}
 		} else {
-			SET_TX_DESC_OFFSET(pdesc, USB_HWDESC_HEADER_LEN);
+			set_tx_desc_offset((__le32 *)pdesc,
+					   USB_HWDESC_HEADER_LEN);
 		}
 
 
@@ -752,8 +753,8 @@  void rtl8821ae_tx_fill_desc(struct ieee80211_hw *hw,
 		SET_TX_DESC_TX_SUB_CARRIER(pdesc,
 			rtl8821ae_sc_mapping(hw, ptcb_desc));
 
-		SET_TX_DESC_LINIP(pdesc, 0);
-		SET_TX_DESC_PKT_SIZE(pdesc, (u16)skb_len);
+		set_tx_desc_linip((__le32 *)pdesc, 0);
+		set_tx_desc_pkt_size((__le32 *)pdesc, (u16)skb_len);
 		if (sta) {
 			u8 ampdu_density = sta->ht_cap.ampdu_density;
 
@@ -789,15 +790,15 @@  void rtl8821ae_tx_fill_desc(struct ieee80211_hw *hw,
 				RT_TRACE(rtlpriv, COMP_SEND, DBG_TRACE,
 					 "Enable RDG function.\n");
 				SET_TX_DESC_RDG_ENABLE(pdesc, 1);
-				SET_TX_DESC_HTC(pdesc, 1);
+				set_tx_desc_htc((__le32 *)pdesc, 1);
 			}
 		}
 		/* tx report */
 		rtl_set_tx_report(ptcb_desc, pdesc, hw, tx_info);
 	}
 
-	SET_TX_DESC_FIRST_SEG(pdesc, (firstseg ? 1 : 0));
-	SET_TX_DESC_LAST_SEG(pdesc, (lastseg ? 1 : 0));
+	set_tx_desc_first_seg((__le32 *)pdesc, (firstseg ? 1 : 0));
+	set_tx_desc_last_seg((__le32 *)pdesc, (lastseg ? 1 : 0));
 	SET_TX_DESC_TX_BUFFER_SIZE(pdesc, (u16)buf_len);
 	SET_TX_DESC_TX_BUFFER_ADDRESS(pdesc, mapping);
 	/* if (rtlpriv->dm.useramask) { */
@@ -815,7 +816,7 @@  void rtl8821ae_tx_fill_desc(struct ieee80211_hw *hw,
 	SET_TX_DESC_MORE_FRAG(pdesc, (lastseg ? 0 : 1));
 	if (is_multicast_ether_addr(ieee80211_get_DA(hdr)) ||
 	    is_broadcast_ether_addr(ieee80211_get_DA(hdr))) {
-		SET_TX_DESC_BMC(pdesc, 1);
+		set_tx_desc_bmc((__le32 *)pdesc, 1);
 	}
 
 	rtl8821ae_dm_set_tx_ant_by_tx_info(hw, pdesc, ptcb_desc->mac_id);
@@ -841,12 +842,12 @@  void rtl8821ae_tx_fill_cmddesc(struct ieee80211_hw *hw,
 	}
 	CLEAR_PCI_TX_DESC_CONTENT(pdesc, TX_DESC_SIZE);
 
-	SET_TX_DESC_FIRST_SEG(pdesc, 1);
-	SET_TX_DESC_LAST_SEG(pdesc, 1);
+	set_tx_desc_first_seg((__le32 *)pdesc, 1);
+	set_tx_desc_last_seg((__le32 *)pdesc, 1);
 
-	SET_TX_DESC_PKT_SIZE((u8 *)pdesc, (u16)(skb->len));
+	set_tx_desc_pkt_size((__le32 *)pdesc, (u16)(skb->len));
 
-	SET_TX_DESC_OFFSET(pdesc, USB_HWDESC_HEADER_LEN);
+	set_tx_desc_offset((__le32 *)pdesc, USB_HWDESC_HEADER_LEN);
 
 	SET_TX_DESC_USE_RATE(pdesc, 1);
 	SET_TX_DESC_TX_RATE(pdesc, DESC_RATE1M);
@@ -864,7 +865,7 @@  void rtl8821ae_tx_fill_cmddesc(struct ieee80211_hw *hw,
 
 	SET_TX_DESC_MACID(pdesc, 0);
 
-	SET_TX_DESC_OWN(pdesc, 1);
+	set_tx_desc_own((__le32 *)pdesc, 1);
 
 	RT_PRINT_DATA(rtlpriv, COMP_CMD, DBG_LOUD,
 		      "H2C Tx Cmd Content\n",
@@ -877,7 +878,7 @@  void rtl8821ae_set_desc(struct ieee80211_hw *hw, u8 *pdesc,
 	if (istx) {
 		switch (desc_name) {
 		case HW_DESC_OWN:
-			SET_TX_DESC_OWN(pdesc, 1);
+			set_tx_desc_own((__le32 *)pdesc, 1);
 			break;
 		case HW_DESC_TX_NEXTDESC_ADDR:
 			SET_TX_DESC_NEXT_DESC_ADDRESS(pdesc, *(u32 *)val);
@@ -919,7 +920,7 @@  u64 rtl8821ae_get_desc(struct ieee80211_hw *hw,
 	if (istx) {
 		switch (desc_name) {
 		case HW_DESC_OWN:
-			ret = GET_TX_DESC_OWN(pdesc);
+			ret = get_tx_desc_own((__le32 *)pdesc);
 			break;
 		case HW_DESC_TXBUFF_ADDR:
 			ret = GET_TX_DESC_TX_BUFFER_ADDRESS(pdesc);
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/trx.h b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/trx.h
index 861d78a24d05..64deaf4dab23 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/trx.h
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/trx.h
@@ -14,25 +14,67 @@ 
 #define USB_HWDESC_HEADER_LEN			40
 #define CRCLENGTH						4
 
-#define SET_TX_DESC_PKT_SIZE(__pdesc, __val)		\
-	SET_BITS_TO_LE_4BYTE(__pdesc, 0, 16, __val)
-#define SET_TX_DESC_OFFSET(__pdesc, __val)			\
-	SET_BITS_TO_LE_4BYTE(__pdesc, 16, 8, __val)
-#define SET_TX_DESC_BMC(__pdesc, __val)				\
-	SET_BITS_TO_LE_4BYTE(__pdesc, 24, 1, __val)
-#define SET_TX_DESC_HTC(__pdesc, __val)				\
-	SET_BITS_TO_LE_4BYTE(__pdesc, 25, 1, __val)
-#define SET_TX_DESC_LAST_SEG(__pdesc, __val)		\
-	SET_BITS_TO_LE_4BYTE(__pdesc, 26, 1, __val)
-#define SET_TX_DESC_FIRST_SEG(__pdesc, __val)		\
-	SET_BITS_TO_LE_4BYTE(__pdesc, 27, 1, __val)
-#define SET_TX_DESC_LINIP(__pdesc, __val)			\
-	SET_BITS_TO_LE_4BYTE(__pdesc, 28, 1, __val)
-#define SET_TX_DESC_OWN(__pdesc, __val)				\
-	SET_BITS_TO_LE_4BYTE(__pdesc, 31, 1, __val)
-
-#define GET_TX_DESC_OWN(__pdesc)					\
-	LE_BITS_TO_4BYTE(__pdesc, 31, 1)
+/* Set packet size (16 bits) in TX descriptor word 0 */
+static inline void set_tx_desc_pkt_size(__le32 *__pdesc, u16 __val)
+{
+	*__pdesc = cpu_to_le32((le32_to_cpu(*__pdesc) & ~GENMASK(0, 15)) |
+			       __val);
+}
+
+/* Set offset (8 bits) in TX descriptor word 0 */
+static inline void set_tx_desc_offset(__le32 *__pdesc, u8 __val)
+{
+	*__pdesc = cpu_to_le32((le32_to_cpu(*__pdesc) & ~GENMASK(16, 23)) |
+			       __val << 16);
+}
+
+/* Set bmc (1 bit) in TX descriptor word 0 */
+static inline void set_tx_desc_bmc(__le32 *__pdesc, u8 __val)
+{
+	*__pdesc = cpu_to_le32((le32_to_cpu(*__pdesc) & ~GENMASK(24, 24)) |
+			       __val << 24);
+}
+
+/* Set htc (1 bit) in TX descriptor word 0 */
+static inline void set_tx_desc_htc(__le32 *__pdesc, u8 __val)
+{
+	*__pdesc = cpu_to_le32((le32_to_cpu(*__pdesc) & ~GENMASK(25, 25)) |
+			       __val << 25);
+}
+
+/* Set last segment flag (1 bit) in TX descriptor word 0 */
+static inline void set_tx_desc_last_seg(__le32 *__pdesc, u8 __val)
+{
+	*__pdesc = cpu_to_le32((le32_to_cpu(*__pdesc) & ~GENMASK(26, 26)) |
+			       __val << 26);
+}
+
+/* Set first segment iflag (1 bit) in TX descriptor word 0 */
+static inline void set_tx_desc_first_seg(__le32 *__pdesc, u8 __val)
+{
+	*__pdesc = cpu_to_le32((le32_to_cpu(*__pdesc) & ~GENMASK(27, 27)) |
+			       __val << 27);
+}
+
+/* Set linip (1 bit) in TX descriptor word 0 */
+static inline void set_tx_desc_linip(__le32 *__pdesc, u8 __val)
+{
+	*__pdesc = cpu_to_le32((le32_to_cpu(*__pdesc) & ~GENMASK(28, 28)) |
+			       __val << 18);
+}
+
+/* Set own flag (1 bit) in TX descriptor word 0 */
+static inline void set_tx_desc_own(__le32 *__pdesc, u8 __val)
+{
+	*__pdesc = cpu_to_le32((le32_to_cpu(*__pdesc) & ~GENMASK(31, 31)) |
+			       __val << 31);
+}
+
+/* Get own flag (1 bit) from TX descriptor word 0 */
+static inline u8 get_tx_desc_own(__le32 *__pdesc)
+{
+	return (le32_to_cpu(*__pdesc) & ~GENMASK(31, 31)) >> 31;
+}
 
 #define SET_TX_DESC_MACID(__pdesc, __val)			\
 	SET_BITS_TO_LE_4BYTE(__pdesc+4, 0, 7, __val)