Am 05.02.24 um 04:01 schrieb Ping-Ke Shih:
-----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.
That's no problem, I could use is_valid_ether_addr() like
rtw_chip_efuse_info_setup() in main.c already does. That's why I'm
wondering if it'd make sense to move getting the MAC from DT to there,
and use MAC from EFUSE, DT, or random in that order of preference for
all rtw88 devices.
Sorry, I'm not familiar with DT, could you show me an example of DT node?
I'm afraid I'm not familiar with the details either, but this is how the
SDIO wifi device for the Pinephone is defined (in
arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi, as of v6.8-rc2):
&mmc1 {
pinctrl-names = "default";
pinctrl-0 = <&mmc1_pins>;
vmmc-supply = <®_vbat_wifi>;
vqmmc-supply = <®_dldo4>;
bus-width = <4>;
non-removable;
status = "okay";
rtl8723cs: wifi@1 {
reg = <1>;
};
};
As far as I understand the "reg = <1>;" line implies that the bootloader
can provide some setup (like the MAC address). I hope someone with more
knowledge can correct me or add missing details.
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.
Do you mean to create a struct to represent the RX descriptor and then
use le*_get_bits() on the fields to get the values? I could try, but I'd
have to replace all the other GET_RX_DESC_* macros defined in rx.h (and
shared by the other chip drivers), too, or it won't really make a
difference (more below).
[...]
+
+ 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?
I can move that to a separate function if you prefer, see below for the
compiler warning.
+
+ 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?
Yes, I learned about that while working on this driver. First the move
to gnu11, and then removing -Wdeclaration-after-statement with
b5ec6fd286dfa466f64cb0e56ed768092d0342ae in 6.5. The commit message says
"It will still be recommeneded [sic!] to place declarations at the start
of a scope where possible, but it will no longer be enforced", so I'll
move these up.
For the struct device_node pointer I think it makes sense to leave the
declaration within the #ifdef CONFIG_OF section (unless we restructure
that into a separate function) because it's unused otherwise.
[...]
+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.
In principle yes, but as I mentioned above I'd basically have to
reinvent all the definitions from rx.h to make it work, I'm not sure if
that really simplifies things. If you want to refactor those I think
it'd be best to do it for all chip drivers together.
The GET_PHY_STAT_* macros are a different matter. The PHY status
structure is different between 8703b and the other supported chips, so
those could be replaced with a struct without duplication. Or at least
mostly, some elements are bit fields or values with < 8 bits, where I
think a macro is simpler than a struct with different definitions
depending on endianess. I am worried about introducing an endianess
error though, so I'd have to ask for careful review of such a patch.
[...]
+MODULE_FIRMWARE("rtw88/rtw8703b_wow_fw.bin");
Just curious. Have you tried WOW for this chip?
Kind of. It definitely doesn't work with this code, that's why I haven't
filled wowlan_stub yet. I already know the check if WOW has been enabled
would have to be changed in wow.c (I'd probably add a "legacy" function
like for the firmware download), but there must be some other details
because I couldn't get it to work yet. But others got it to work with
the out-of-tree driver, so it must be possible.
I've applied the other minor changes you suggested.
