diff mbox series

[2/9] wifi: rtw88: Debug output for rtw8723x EFUSE

Message ID 20240202121050.977223-3-fiona.klute@gmx.de (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show
Series rtw88: Add support for RTL8723CS/RTL8703B | expand

Commit Message

Fiona Klute Feb. 2, 2024, 12:10 p.m. UTC 
Some 8703b chips contain invalid EFUSE data, getting detailed
information is critical when analyzing issues caused by that.

Signed-off-by: Fiona Klute <fiona.klute@gmx.de>
---
The TX power table debug output function (rtw8723x_debug_txpwr_limit)
isn't specific to the chip family. Should I move it to debug.c
(e.g. as rtw_debug_txpwr_limit_2g)?

 drivers/net/wireless/realtek/rtw88/rtw8723x.c | 159 ++++++++++++++++++
 drivers/net/wireless/realtek/rtw88/rtw8723x.h |  11 ++
 2 files changed, 170 insertions(+)

--
2.43.0

Comments

Ping-Ke Shih Feb. 5, 2024, 2:17 a.m. UTC | #1 
> -----Original Message-----
> From: Fiona Klute <fiona.klute@gmx.de>
> Sent: Friday, February 2, 2024 8:11 PM
> To: linux-wireless@vger.kernel.org; Ping-Ke Shih <pkshih@realtek.com>
> Cc: Kalle Valo <kvalo@kernel.org>; Ulf Hansson <ulf.hansson@linaro.org>; linux-mmc@vger.kernel.org; Pavel
> Machek <pavel@ucw.cz>; Ondřej Jirman <megi@xff.cz>; Fiona Klute <fiona.klute@gmx.de>
> Subject: [PATCH 2/9] wifi: rtw88: Debug output for rtw8723x EFUSE
> 
> Some 8703b chips contain invalid EFUSE data, getting detailed
> information is critical when analyzing issues caused by that.
> 
> Signed-off-by: Fiona Klute <fiona.klute@gmx.de>
> ---
> The TX power table debug output function (rtw8723x_debug_txpwr_limit)
> isn't specific to the chip family. Should I move it to debug.c
> (e.g. as rtw_debug_txpwr_limit_2g)?

I think no need. My another thinking is to add an debugfs entry to read
these stuff out, but if failed to probe we can't get these information. 

> 
>  drivers/net/wireless/realtek/rtw88/rtw8723x.c | 159 ++++++++++++++++++
>  drivers/net/wireless/realtek/rtw88/rtw8723x.h |  11 ++
>  2 files changed, 170 insertions(+)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8723x.c
> b/drivers/net/wireless/realtek/rtw88/rtw8723x.c
> index 2b58064547..bca650c6bb 100644
> --- a/drivers/net/wireless/realtek/rtw88/rtw8723x.c
> +++ b/drivers/net/wireless/realtek/rtw88/rtw8723x.c
> @@ -63,6 +63,163 @@ static void __rtw8723x_lck(struct rtw_dev *rtwdev)
>                 rtw_write8(rtwdev, REG_TXPAUSE, 0x00);
>  }
> 
> +#define DBG_EFUSE_VAL(map, name)                        \

I think we should not hide 'rtwdev'

> +       rtw_dbg(rtwdev, RTW_DBG_EFUSE, # name "=0x%x\n", \
> +               (map)->name)
> +#define DBG_EFUSE_2BYTE(map, name)                        \
> +       rtw_dbg(rtwdev, RTW_DBG_EFUSE, # name "=0x%x%x\n", \

Should format be '%02x%02x' for two bytes?

> +               (map)->name[0], (map)->name[1])
> +
> +static void rtw8723xe_efuse_debug(struct rtw_dev *rtwdev,
> +                                 struct rtw8723x_efuse *map)
> +{
> +       rtw_dbg(rtwdev, RTW_DBG_EFUSE, "mac_addr=%pM\n", map->e.mac_addr);
> +       DBG_EFUSE_2BYTE(map, e.vendor_id);
> +       DBG_EFUSE_2BYTE(map, e.device_id);
> +       DBG_EFUSE_2BYTE(map, e.sub_vendor_id);
> +       DBG_EFUSE_2BYTE(map, e.sub_device_id);
> +}
> +
> +static void rtw8723xu_efuse_debug(struct rtw_dev *rtwdev,
> +                                 struct rtw8723x_efuse *map)
> +{
> +       DBG_EFUSE_2BYTE(map, u.vendor_id);
> +       DBG_EFUSE_2BYTE(map, u.product_id);
> +       DBG_EFUSE_VAL(map, u.usb_option);
> +       rtw_dbg(rtwdev, RTW_DBG_EFUSE, "mac_addr=%pM\n", map->u.mac_addr);

Just curious why 'mac_addr' is the first one in rtw8723xe_efuse_debug(), but
the last one here?


> +}
> +

[...]
Fiona Klute Feb. 5, 2024, 5:10 p.m. UTC | #2 
Am 05.02.24 um 03:17 schrieb Ping-Ke Shih:
>
>
>> -----Original Message-----
>> From: Fiona Klute <fiona.klute@gmx.de>
>> Sent: Friday, February 2, 2024 8:11 PM
>> To: linux-wireless@vger.kernel.org; Ping-Ke Shih <pkshih@realtek.com>
>> Cc: Kalle Valo <kvalo@kernel.org>; Ulf Hansson <ulf.hansson@linaro.org>; linux-mmc@vger.kernel.org; Pavel
>> Machek <pavel@ucw.cz>; Ondřej Jirman <megi@xff.cz>; Fiona Klute <fiona.klute@gmx.de>
>> Subject: [PATCH 2/9] wifi: rtw88: Debug output for rtw8723x EFUSE
>>
>> Some 8703b chips contain invalid EFUSE data, getting detailed
>> information is critical when analyzing issues caused by that.
>>
>> Signed-off-by: Fiona Klute <fiona.klute@gmx.de>
>> ---
>> The TX power table debug output function (rtw8723x_debug_txpwr_limit)
>> isn't specific to the chip family. Should I move it to debug.c
>> (e.g. as rtw_debug_txpwr_limit_2g)?
>
> I think no need. My another thinking is to add an debugfs entry to read
> these stuff out, but if failed to probe we can't get these information.

Okay, I'll leave it where it is then.

>>
>>   drivers/net/wireless/realtek/rtw88/rtw8723x.c | 159 ++++++++++++++++++
>>   drivers/net/wireless/realtek/rtw88/rtw8723x.h |  11 ++
>>   2 files changed, 170 insertions(+)
>>
>> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8723x.c
>> b/drivers/net/wireless/realtek/rtw88/rtw8723x.c
>> index 2b58064547..bca650c6bb 100644
>> --- a/drivers/net/wireless/realtek/rtw88/rtw8723x.c
>> +++ b/drivers/net/wireless/realtek/rtw88/rtw8723x.c
>> @@ -63,6 +63,163 @@ static void __rtw8723x_lck(struct rtw_dev *rtwdev)
>>                  rtw_write8(rtwdev, REG_TXPAUSE, 0x00);
>>   }
>>
>> +#define DBG_EFUSE_VAL(map, name)                        \
>
> I think we should not hide 'rtwdev'

Okay, I'll change that.

>> +       rtw_dbg(rtwdev, RTW_DBG_EFUSE, # name "=0x%x\n", \
>> +               (map)->name)
>> +#define DBG_EFUSE_2BYTE(map, name)                        \
>> +       rtw_dbg(rtwdev, RTW_DBG_EFUSE, # name "=0x%x%x\n", \
>
> Should format be '%02x%02x' for two bytes?

Right, otherwise the output could be ambiguous. I'll fix that.

>> +               (map)->name[0], (map)->name[1])
>> +
>> +static void rtw8723xe_efuse_debug(struct rtw_dev *rtwdev,
>> +                                 struct rtw8723x_efuse *map)
>> +{
>> +       rtw_dbg(rtwdev, RTW_DBG_EFUSE, "mac_addr=%pM\n", map->e.mac_addr);
>> +       DBG_EFUSE_2BYTE(map, e.vendor_id);
>> +       DBG_EFUSE_2BYTE(map, e.device_id);
>> +       DBG_EFUSE_2BYTE(map, e.sub_vendor_id);
>> +       DBG_EFUSE_2BYTE(map, e.sub_device_id);
>> +}
>> +
>> +static void rtw8723xu_efuse_debug(struct rtw_dev *rtwdev,
>> +                                 struct rtw8723x_efuse *map)
>> +{
>> +       DBG_EFUSE_2BYTE(map, u.vendor_id);
>> +       DBG_EFUSE_2BYTE(map, u.product_id);
>> +       DBG_EFUSE_VAL(map, u.usb_option);
>> +       rtw_dbg(rtwdev, RTW_DBG_EFUSE, "mac_addr=%pM\n", map->u.mac_addr);
>
> Just curious why 'mac_addr' is the first one in rtw8723xe_efuse_debug(), but
> the last one here?

I just followed the order of elements in the EFUSE data, as described in
the rtw8723x[eus]_efuse structs. I don't know why it is that way.
diff mbox series

Patch

diff --git a/drivers/net/wireless/realtek/rtw88/rtw8723x.c b/drivers/net/wireless/realtek/rtw88/rtw8723x.c
index 2b58064547..bca650c6bb 100644
--- a/drivers/net/wireless/realtek/rtw88/rtw8723x.c
+++ b/drivers/net/wireless/realtek/rtw88/rtw8723x.c
@@ -63,6 +63,163 @@  static void __rtw8723x_lck(struct rtw_dev *rtwdev)
 		rtw_write8(rtwdev, REG_TXPAUSE, 0x00);
 }

+#define DBG_EFUSE_VAL(map, name)			 \
+	rtw_dbg(rtwdev, RTW_DBG_EFUSE, # name "=0x%x\n", \
+		(map)->name)
+#define DBG_EFUSE_2BYTE(map, name)			   \
+	rtw_dbg(rtwdev, RTW_DBG_EFUSE, # name "=0x%x%x\n", \
+		(map)->name[0], (map)->name[1])
+
+static void rtw8723xe_efuse_debug(struct rtw_dev *rtwdev,
+				  struct rtw8723x_efuse *map)
+{
+	rtw_dbg(rtwdev, RTW_DBG_EFUSE, "mac_addr=%pM\n", map->e.mac_addr);
+	DBG_EFUSE_2BYTE(map, e.vendor_id);
+	DBG_EFUSE_2BYTE(map, e.device_id);
+	DBG_EFUSE_2BYTE(map, e.sub_vendor_id);
+	DBG_EFUSE_2BYTE(map, e.sub_device_id);
+}
+
+static void rtw8723xu_efuse_debug(struct rtw_dev *rtwdev,
+				  struct rtw8723x_efuse *map)
+{
+	DBG_EFUSE_2BYTE(map, u.vendor_id);
+	DBG_EFUSE_2BYTE(map, u.product_id);
+	DBG_EFUSE_VAL(map, u.usb_option);
+	rtw_dbg(rtwdev, RTW_DBG_EFUSE, "mac_addr=%pM\n", map->u.mac_addr);
+}
+
+static void rtw8723xs_efuse_debug(struct rtw_dev *rtwdev,
+				  struct rtw8723x_efuse *map)
+{
+	rtw_dbg(rtwdev, RTW_DBG_EFUSE, "mac_addr=%pM\n", map->s.mac_addr);
+}
+
+static void __rtw8723x_debug_txpwr_limit(struct rtw_dev *rtwdev,
+					 struct rtw_txpwr_idx *table,
+					 int tx_path_count)
+{
+	if (!rtw_dbg_is_enabled(rtwdev, RTW_DBG_EFUSE))
+		return;
+
+	rtw_dbg(rtwdev, RTW_DBG_EFUSE,
+		"Power index table (2.4G):\n");
+	/* CCK base */
+	rtw_dbg(rtwdev, RTW_DBG_EFUSE, "CCK base\n");
+	rtw_dbg(rtwdev, RTW_DBG_EFUSE, "RF    G0  G1  G2  G3  G4  G5\n");
+	for (int i = 0; i < tx_path_count; i++)
+		rtw_dbg(rtwdev, RTW_DBG_EFUSE,
+			"[%c]: %3u %3u %3u %3u %3u %3u\n",
+			'A' + i,
+			table[i].pwr_idx_2g.cck_base[0],
+			table[i].pwr_idx_2g.cck_base[1],
+			table[i].pwr_idx_2g.cck_base[2],
+			table[i].pwr_idx_2g.cck_base[3],
+			table[i].pwr_idx_2g.cck_base[4],
+			table[i].pwr_idx_2g.cck_base[5]);
+	/* CCK diff */
+	rtw_dbg(rtwdev, RTW_DBG_EFUSE, "CCK diff\n");
+	rtw_dbg(rtwdev, RTW_DBG_EFUSE, "RF   1S 2S 3S 4S\n");
+	for (int i = 0; i < tx_path_count; i++)
+		rtw_dbg(rtwdev, RTW_DBG_EFUSE,
+			"[%c]: %2d %2d %2d %2d\n",
+			'A' + i, 0 /* no diff for 1S */,
+			table[i].pwr_idx_2g.ht_2s_diff.cck,
+			table[i].pwr_idx_2g.ht_3s_diff.cck,
+			table[i].pwr_idx_2g.ht_4s_diff.cck);
+	/* BW40-1S base */
+	rtw_dbg(rtwdev, RTW_DBG_EFUSE, "BW40-1S base\n");
+	rtw_dbg(rtwdev, RTW_DBG_EFUSE, "RF    G0  G1  G2  G3  G4\n");
+	for (int i = 0; i < tx_path_count; i++)
+		rtw_dbg(rtwdev, RTW_DBG_EFUSE,
+			"[%c]: %3u %3u %3u %3u %3u\n",
+			'A' + i,
+			table[i].pwr_idx_2g.bw40_base[0],
+			table[i].pwr_idx_2g.bw40_base[1],
+			table[i].pwr_idx_2g.bw40_base[2],
+			table[i].pwr_idx_2g.bw40_base[3],
+			table[i].pwr_idx_2g.bw40_base[4]);
+	/* OFDM diff */
+	rtw_dbg(rtwdev, RTW_DBG_EFUSE, "OFDM diff\n");
+	rtw_dbg(rtwdev, RTW_DBG_EFUSE, "RF   1S 2S 3S 4S\n");
+	for (int i = 0; i < tx_path_count; i++)
+		rtw_dbg(rtwdev, RTW_DBG_EFUSE,
+			"[%c]: %2d %2d %2d %2d\n",
+			'A' + i,
+			table[i].pwr_idx_2g.ht_1s_diff.ofdm,
+			table[i].pwr_idx_2g.ht_2s_diff.ofdm,
+			table[i].pwr_idx_2g.ht_3s_diff.ofdm,
+			table[i].pwr_idx_2g.ht_4s_diff.ofdm);
+	/* BW20 diff */
+	rtw_dbg(rtwdev, RTW_DBG_EFUSE, "BW20 diff\n");
+	rtw_dbg(rtwdev, RTW_DBG_EFUSE, "RF   1S 2S 3S 4S\n");
+	for (int i = 0; i < tx_path_count; i++)
+		rtw_dbg(rtwdev, RTW_DBG_EFUSE,
+			"[%c]: %2d %2d %2d %2d\n",
+			'A' + i,
+			table[i].pwr_idx_2g.ht_1s_diff.bw20,
+			table[i].pwr_idx_2g.ht_2s_diff.bw20,
+			table[i].pwr_idx_2g.ht_3s_diff.bw20,
+			table[i].pwr_idx_2g.ht_4s_diff.bw20);
+	/* BW40 diff */
+	rtw_dbg(rtwdev, RTW_DBG_EFUSE, "BW40 diff\n");
+	rtw_dbg(rtwdev, RTW_DBG_EFUSE, "RF   1S 2S 3S 4S\n");
+	for (int i = 0; i < tx_path_count; i++)
+		rtw_dbg(rtwdev, RTW_DBG_EFUSE,
+			"[%c]: %2d %2d %2d %2d\n",
+			'A' + i, 0 /* no diff for 1S */,
+			table[i].pwr_idx_2g.ht_2s_diff.bw40,
+			table[i].pwr_idx_2g.ht_3s_diff.bw40,
+			table[i].pwr_idx_2g.ht_4s_diff.bw40);
+}
+
+static void efuse_debug_dump(struct rtw_dev *rtwdev,
+			     struct rtw8723x_efuse *map)
+{
+	if (!rtw_dbg_is_enabled(rtwdev, RTW_DBG_EFUSE))
+		return;
+
+	rtw_dbg(rtwdev, RTW_DBG_EFUSE, "EFUSE raw logical map:\n");
+	print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET, 16, 1,
+		       (u8 *)map, sizeof(struct rtw8723x_efuse), false);
+	rtw_dbg(rtwdev, RTW_DBG_EFUSE, "Parsed rtw8723x EFUSE data:\n");
+	DBG_EFUSE_VAL(map, rtl_id);
+	DBG_EFUSE_VAL(map, afe);
+	rtw8723x_debug_txpwr_limit(rtwdev, map->txpwr_idx_table, 4);
+	DBG_EFUSE_VAL(map, channel_plan);
+	DBG_EFUSE_VAL(map, xtal_k);
+	DBG_EFUSE_VAL(map, thermal_meter);
+	DBG_EFUSE_VAL(map, iqk_lck);
+	DBG_EFUSE_VAL(map, pa_type);
+	DBG_EFUSE_2BYTE(map, lna_type_2g);
+	DBG_EFUSE_2BYTE(map, lna_type_5g);
+	DBG_EFUSE_VAL(map, rf_board_option);
+	DBG_EFUSE_VAL(map, rf_feature_option);
+	DBG_EFUSE_VAL(map, rf_bt_setting);
+	DBG_EFUSE_VAL(map, eeprom_version);
+	DBG_EFUSE_VAL(map, eeprom_customer_id);
+	DBG_EFUSE_VAL(map, tx_bb_swing_setting_2g);
+	DBG_EFUSE_VAL(map, tx_pwr_calibrate_rate);
+	DBG_EFUSE_VAL(map, rf_antenna_option);
+	DBG_EFUSE_VAL(map, rfe_option);
+	DBG_EFUSE_2BYTE(map, country_code);
+
+	switch (rtw_hci_type(rtwdev)) {
+	case RTW_HCI_TYPE_PCIE:
+		rtw8723xe_efuse_debug(rtwdev, map);
+		break;
+	case RTW_HCI_TYPE_USB:
+		rtw8723xu_efuse_debug(rtwdev, map);
+		break;
+	case RTW_HCI_TYPE_SDIO:
+		rtw8723xs_efuse_debug(rtwdev, map);
+		break;
+	default:
+		/* unsupported now */
+		break;
+	}
+}
+
 static void rtw8723xe_efuse_parsing(struct rtw_efuse *efuse,
 				    struct rtw8723x_efuse *map)
 {
@@ -88,6 +245,7 @@  static int __rtw8723x_read_efuse(struct rtw_dev *rtwdev, u8 *log_map)
 	int i;

 	map = (struct rtw8723x_efuse *)log_map;
+	efuse_debug_dump(rtwdev, map);

 	efuse->rfe_option = 0;
 	efuse->rf_board_option = map->rf_board_option;
@@ -553,6 +711,7 @@  const struct rtw8723x_common rtw8723x_common = {
 	.pwrtrack_set_xtal = __rtw8723x_pwrtrack_set_xtal,
 	.coex_cfg_init = __rtw8723x_coex_cfg_init,
 	.fill_txdesc_checksum = __rtw8723x_fill_txdesc_checksum,
+	.debug_txpwr_limit = __rtw8723x_debug_txpwr_limit,
 };
 EXPORT_SYMBOL(rtw8723x_common);

diff --git a/drivers/net/wireless/realtek/rtw88/rtw8723x.h b/drivers/net/wireless/realtek/rtw88/rtw8723x.h
index d3930f1f2c..f5c46b714c 100644
--- a/drivers/net/wireless/realtek/rtw88/rtw8723x.h
+++ b/drivers/net/wireless/realtek/rtw88/rtw8723x.h
@@ -154,6 +154,9 @@  struct rtw8723x_common {
 	void (*fill_txdesc_checksum)(struct rtw_dev *rtwdev,
 				     struct rtw_tx_pkt_info *pkt_info,
 				     u8 *txdesc);
+	void (*debug_txpwr_limit)(struct rtw_dev *rtwdev,
+				  struct rtw_txpwr_idx *table,
+				  int tx_path_count);
 };

 extern const struct rtw8723x_common rtw8723x_common;
@@ -346,6 +349,14 @@  static inline s32 iqk_mult(s32 x, s32 y, s32 *ext)
 	return (t >> 8);	/* Q.16 --> Q.8 */
 }

+static inline
+void rtw8723x_debug_txpwr_limit(struct rtw_dev *rtwdev,
+				struct rtw_txpwr_idx *table,
+				int tx_path_count)
+{
+	rtw8723x_common.debug_txpwr_limit(rtwdev, table, tx_path_count);
+}
+
 static inline void rtw8723x_lck(struct rtw_dev *rtwdev)
 {
 	rtw8723x_common.lck(rtwdev);