[PATCH v2 0/5] wifi: rtw89: correct MAC and PCI settings

[PATCH v2 0/5] wifi: rtw89: correct MAC and PCI settings

[Ping-Ke Shih]

In internal test, we found some problems containing MAC and PCI related
things, so correct them by this patchset.

v2: replace indirect access MIO with Linux standard API to access
    PCI configuration space.

Chia-Yuan Li (2):
  wifi: rtw89: 8852c: set TBTT shift configuration
  wifi: rtw89: pci: fix PCI PHY auto adaption by using software restore

Chin-Yen Lee (3):
  wifi: rtw89: add retry to change power_mode state
  wifi: rtw89: pci: enable CLK_REQ, ASPM, L1 and L1ss for 8852c
  wifi: rtw89: pci: correct suspend/resume setting for variant chips

 drivers/net/wireless/realtek/rtw89/mac.c |  44 +++++-
 drivers/net/wireless/realtek/rtw89/mac.h |   1 +
 drivers/net/wireless/realtek/rtw89/pci.c | 193 +++++++++++++++++++----
 drivers/net/wireless/realtek/rtw89/pci.h |  41 ++++-
 drivers/net/wireless/realtek/rtw89/reg.h |  14 ++
 5 files changed, 253 insertions(+), 40 deletions(-)

-- 
2.25.1

[PATCH v2 1/5] wifi: rtw89: add retry to change power_mode state

[Ping-Ke Shih]

From: Chin-Yen Lee <timlee@realtek.com>

When starting to send heavy traffic in low power mode,
driver will call multiple tx wake notify to wake firmware
within a short time. In this situation, firmware may miss
power mode change request from driver and leads to status
error. So we change driver to call power_mode_change at most
three times to make sure firmware could get the request.

Signed-off-by: Chin-Yen Lee <timlee@realtek.com>
Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
---
 drivers/net/wireless/realtek/rtw89/mac.c | 23 +++++++++++++++++------
 drivers/net/wireless/realtek/rtw89/mac.h |  1 +
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw89/mac.c b/drivers/net/wireless/realtek/rtw89/mac.c
index 93124b815825f..61c7e79714925 100644
--- a/drivers/net/wireless/realtek/rtw89/mac.c
+++ b/drivers/net/wireless/realtek/rtw89/mac.c
@@ -1053,18 +1053,29 @@ void rtw89_mac_power_mode_change(struct rtw89_dev *rtwdev, bool enter)
 	enum rtw89_rpwm_req_pwr_state state;
 	unsigned long delay = enter ? 10 : 150;
 	int ret;
+	int i;
 
 	if (enter)
 		state = rtw89_mac_get_req_pwr_state(rtwdev);
 	else
 		state = RTW89_MAC_RPWM_REQ_PWR_STATE_ACTIVE;
 
-	rtw89_mac_send_rpwm(rtwdev, state, false);
-	ret = read_poll_timeout_atomic(rtw89_mac_check_cpwm_state, ret, !ret,
-				       delay, 15000, false, rtwdev, state);
-	if (ret)
-		rtw89_err(rtwdev, "firmware failed to ack for %s ps mode\n",
-			  enter ? "entering" : "leaving");
+	for (i = 0; i < RPWM_TRY_CNT; i++) {
+		rtw89_mac_send_rpwm(rtwdev, state, false);
+		ret = read_poll_timeout_atomic(rtw89_mac_check_cpwm_state, ret,
+					       !ret, delay, 15000, false,
+					       rtwdev, state);
+		if (!ret)
+			break;
+
+		if (i == RPWM_TRY_CNT - 1)
+			rtw89_err(rtwdev, "firmware failed to ack for %s ps mode\n",
+				  enter ? "entering" : "leaving");
+		else
+			rtw89_debug(rtwdev, RTW89_DBG_UNEXP,
+				    "%d time firmware failed to ack for %s ps mode\n",
+				    i + 1, enter ? "entering" : "leaving");
+	}
 }
 
 void rtw89_mac_notify_wake(struct rtw89_dev *rtwdev)
diff --git a/drivers/net/wireless/realtek/rtw89/mac.h b/drivers/net/wireless/realtek/rtw89/mac.h
index f66619354734d..986e359a82237 100644
--- a/drivers/net/wireless/realtek/rtw89/mac.h
+++ b/drivers/net/wireless/realtek/rtw89/mac.h
@@ -11,6 +11,7 @@
 #define ADDR_CAM_ENT_SIZE  0x40
 #define BSSID_CAM_ENT_SIZE 0x08
 #define HFC_PAGE_UNIT 64
+#define RPWM_TRY_CNT 3
 
 enum rtw89_mac_hwmod_sel {
 	RTW89_DMAC_SEL = 0,
-- 
2.25.1

[PATCH v2 2/5] wifi: rtw89: 8852c: set TBTT shift configuration

[Ping-Ke Shih]

From: Chia-Yuan Li <leo.li@realtek.com>

It is found that 8852ce loses some beacon after
enabling deep ps mode. We set TBTT shift to wake up
firmware early to open RF/BB for receiving beacon in time.

Signed-off-by: Chia-Yuan Li <leo.li@realtek.com>
Signed-off-by: Po-Hao Huang <phhuang@realtek.com>
Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
---
 drivers/net/wireless/realtek/rtw89/mac.c | 21 +++++++++++++++++++++
 drivers/net/wireless/realtek/rtw89/reg.h |  2 ++
 2 files changed, 23 insertions(+)

diff --git a/drivers/net/wireless/realtek/rtw89/mac.c b/drivers/net/wireless/realtek/rtw89/mac.c
index 61c7e79714925..c27b1e1aed37d 100644
--- a/drivers/net/wireless/realtek/rtw89/mac.c
+++ b/drivers/net/wireless/realtek/rtw89/mac.c
@@ -3535,6 +3535,26 @@ static void rtw89_mac_port_cfg_bcn_early(struct rtw89_dev *rtwdev,
 				BCN_ERLY_DEF);
 }
 
+static void rtw89_mac_port_cfg_tbtt_shift(struct rtw89_dev *rtwdev,
+					  struct rtw89_vif *rtwvif)
+{
+	const struct rtw89_port_reg *p = &rtw_port_base;
+	u16 val;
+
+	if (rtwdev->chip->chip_id != RTL8852C)
+		return;
+
+	if (rtwvif->wifi_role != RTW89_WIFI_ROLE_P2P_CLIENT &&
+	    rtwvif->wifi_role != RTW89_WIFI_ROLE_STATION)
+		return;
+
+	val = FIELD_PREP(B_AX_TBTT_SHIFT_OFST_MAG, 1) |
+			 B_AX_TBTT_SHIFT_OFST_SIGN;
+
+	rtw89_write16_port_mask(rtwdev, rtwvif, p->tbtt_shift,
+				B_AX_TBTT_SHIFT_OFST_MASK, val);
+}
+
 int rtw89_mac_vif_init(struct rtw89_dev *rtwdev, struct rtw89_vif *rtwvif)
 {
 	int ret;
@@ -3609,6 +3629,7 @@ int rtw89_mac_port_update(struct rtw89_dev *rtwdev, struct rtw89_vif *rtwvif)
 	rtw89_mac_port_cfg_bcn_hold_time(rtwdev, rtwvif);
 	rtw89_mac_port_cfg_bcn_mask_area(rtwdev, rtwvif);
 	rtw89_mac_port_cfg_tbtt_early(rtwdev, rtwvif);
+	rtw89_mac_port_cfg_tbtt_shift(rtwdev, rtwvif);
 	rtw89_mac_port_cfg_bss_color(rtwdev, rtwvif);
 	rtw89_mac_port_cfg_mbssid(rtwdev, rtwvif);
 	rtw89_mac_port_cfg_func_en(rtwdev, rtwvif);
diff --git a/drivers/net/wireless/realtek/rtw89/reg.h b/drivers/net/wireless/realtek/rtw89/reg.h
index 76d3d9aa8745b..497c1e9263fc0 100644
--- a/drivers/net/wireless/realtek/rtw89/reg.h
+++ b/drivers/net/wireless/realtek/rtw89/reg.h
@@ -2093,6 +2093,8 @@
 #define R_AX_TBTT_SHIFT_P3 0xC4E8
 #define R_AX_TBTT_SHIFT_P4 0xC528
 #define B_AX_TBTT_SHIFT_OFST_MASK GENMASK(11, 0)
+#define B_AX_TBTT_SHIFT_OFST_SIGN BIT(11)
+#define B_AX_TBTT_SHIFT_OFST_MAG GENMASK(10, 0)
 
 #define R_AX_BCN_CNT_TMR_P0 0xC434
 #define R_AX_BCN_CNT_TMR_P1 0xC474
-- 
2.25.1

[PATCH v2 3/5] wifi: rtw89: pci: fix PCI PHY auto adaption by using software restore

[Ping-Ke Shih]

From: Chia-Yuan Li <leo.li@realtek.com>

There is chance that PCI PHY auto adaption fail. When first time boot up,
software restore the right adaption value and close PHY auto adaption
mechanism.

Signed-off-by: Chia-Yuan Li <leo.li@realtek.com>
Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
---
 drivers/net/wireless/realtek/rtw89/pci.c | 71 ++++++++++++++++++++++++
 drivers/net/wireless/realtek/rtw89/pci.h | 29 +++++++++-
 drivers/net/wireless/realtek/rtw89/reg.h | 12 ++++
 3 files changed, 110 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw89/pci.c b/drivers/net/wireless/realtek/rtw89/pci.c
index c68fec9eb5a64..c09d2ffc5005f 100644
--- a/drivers/net/wireless/realtek/rtw89/pci.c
+++ b/drivers/net/wireless/realtek/rtw89/pci.c
@@ -3219,6 +3219,76 @@ static void rtw89_pci_free_irq(struct rtw89_dev *rtwdev,
 	pci_free_irq_vectors(pdev);
 }
 
+static u16 gray_code_to_bin(u16 gray_code, u32 bit_num)
+{
+	u16 bin = 0, gray_bit;
+	u32 bit_idx;
+
+	for (bit_idx = 0; bit_idx < bit_num; bit_idx++) {
+		gray_bit = (gray_code >> bit_idx) & 0x1;
+		if (bit_num - bit_idx > 1)
+			gray_bit ^= (gray_code >> (bit_idx + 1)) & 0x1;
+		bin |= (gray_bit << bit_idx);
+	}
+
+	return bin;
+}
+
+static int rtw89_pci_filter_out(struct rtw89_dev *rtwdev)
+{
+	struct rtw89_pci *rtwpci = (struct rtw89_pci *)rtwdev->priv;
+	struct pci_dev *pdev = rtwpci->pdev;
+	u16 val16, filter_out_val;
+	u32 val, phy_offset;
+	int ret;
+
+	if (rtwdev->chip->chip_id != RTL8852C)
+		return 0;
+
+	val = rtw89_read32_mask(rtwdev, R_AX_PCIE_MIX_CFG_V1, B_AX_ASPM_CTRL_MASK);
+	if (val == B_AX_ASPM_CTRL_L1)
+		return 0;
+
+	ret = pci_read_config_dword(pdev, RTW89_PCIE_L1_STS_V1, &val);
+	if (ret)
+		return ret;
+
+	val = FIELD_GET(RTW89_BCFG_LINK_SPEED_MASK, val);
+	if (val == RTW89_PCIE_GEN1_SPEED) {
+		phy_offset = R_RAC_DIRECT_OFFSET_G1;
+	} else if (val == RTW89_PCIE_GEN2_SPEED) {
+		phy_offset = R_RAC_DIRECT_OFFSET_G2;
+		val16 = rtw89_read16(rtwdev, phy_offset + RAC_ANA10 * RAC_MULT);
+		rtw89_write16_set(rtwdev, phy_offset + RAC_ANA10 * RAC_MULT,
+				  val16 | B_PCIE_BIT_PINOUT_DIS);
+		rtw89_write16_set(rtwdev, phy_offset + RAC_ANA19 * RAC_MULT,
+				  val16 & ~B_PCIE_BIT_RD_SEL);
+
+		val16 = rtw89_read16_mask(rtwdev,
+					  phy_offset + RAC_ANA1F * RAC_MULT,
+					  FILTER_OUT_EQ_MASK);
+		val16 = gray_code_to_bin(val16, hweight16(val16));
+		filter_out_val = rtw89_read16(rtwdev, phy_offset + RAC_ANA24 *
+					      RAC_MULT);
+		filter_out_val &= ~REG_FILTER_OUT_MASK;
+		filter_out_val |= FIELD_PREP(REG_FILTER_OUT_MASK, val16);
+
+		rtw89_write16(rtwdev, phy_offset + RAC_ANA24 * RAC_MULT,
+			      filter_out_val);
+		rtw89_write16_set(rtwdev, phy_offset + RAC_ANA0A * RAC_MULT,
+				  B_BAC_EQ_SEL);
+		rtw89_write16_set(rtwdev,
+				  R_RAC_DIRECT_OFFSET_G1 + RAC_ANA0C * RAC_MULT,
+				  B_PCIE_BIT_PSAVE);
+	} else {
+		return -EOPNOTSUPP;
+	}
+	rtw89_write16_set(rtwdev, phy_offset + RAC_ANA0C * RAC_MULT,
+			  B_PCIE_BIT_PSAVE);
+
+	return 0;
+}
+
 static void rtw89_pci_clkreq_set(struct rtw89_dev *rtwdev, bool enable)
 {
 	int ret;
@@ -3667,6 +3737,7 @@ int rtw89_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		goto err_clear_resource;
 	}
 
+	rtw89_pci_filter_out(rtwdev);
 	rtw89_pci_link_cfg(rtwdev);
 	rtw89_pci_l1ss_cfg(rtwdev);
 
diff --git a/drivers/net/wireless/realtek/rtw89/pci.h b/drivers/net/wireless/realtek/rtw89/pci.h
index a118647213e35..e80380271cd35 100644
--- a/drivers/net/wireless/realtek/rtw89/pci.h
+++ b/drivers/net/wireless/realtek/rtw89/pci.h
@@ -11,11 +11,18 @@
 #define MDIO_PG1_G1 1
 #define MDIO_PG0_G2 2
 #define MDIO_PG1_G2 3
+#define RAC_CTRL_PPR			0x00
+#define RAC_ANA0A			0x0A
+#define B_BAC_EQ_SEL			BIT(5)
+#define RAC_ANA0C			0x0C
+#define B_PCIE_BIT_PSAVE		BIT(15)
 #define RAC_ANA10			0x10
+#define B_PCIE_BIT_PINOUT_DIS		BIT(3)
 #define RAC_REG_REV2			0x1B
 #define BAC_CMU_EN_DLY_MASK		GENMASK(15, 12)
 #define PCIE_DPHY_DLY_25US		0x1
 #define RAC_ANA19			0x19
+#define B_PCIE_BIT_RD_SEL		BIT(2)
 #define RAC_ANA1F			0x1F
 #define RAC_ANA24			0x24
 #define B_AX_DEGLITCH			GENMASK(11, 8)
@@ -45,6 +52,16 @@
 #define B_AX_SEL_REQ_ENTR_L1		BIT(2)
 #define B_AX_SEL_REQ_EXIT_L1		BIT(0)
 
+#define R_AX_PCIE_MIX_CFG_V1		0x300C
+#define B_AX_ASPM_CTRL_L1		BIT(17)
+#define B_AX_ASPM_CTRL_L0		BIT(16)
+#define B_AX_ASPM_CTRL_MASK		GENMASK(17, 16)
+#define B_AX_XFER_PENDING_FW		BIT(11)
+#define B_AX_XFER_PENDING		BIT(10)
+#define B_AX_REQ_EXIT_L1		BIT(9)
+#define B_AX_REQ_ENTR_L1		BIT(8)
+#define B_AX_L1SUB_DISABLE		BIT(0)
+
 #define R_AX_PCIE_BG_CLR		0x303C
 #define B_AX_BG_CLR_ASYNC_M3		BIT(4)
 
@@ -88,7 +105,10 @@
 #define B_AX_PCIE_WDT_TIMER_S1_MASK GENMASK(31, 0)
 
 #define R_RAC_DIRECT_OFFSET_G1 0x3800
+#define FILTER_OUT_EQ_MASK GENMASK(14, 10)
 #define R_RAC_DIRECT_OFFSET_G2 0x3880
+#define REG_FILTER_OUT_MASK GENMASK(6, 2)
+#define RAC_MULT 2
 
 #define RTW89_PCI_WR_RETRY_CNT		20
 
@@ -505,6 +525,12 @@
 #define RTW89_PCI_MULTITAG		8
 
 /* PCIE CFG register */
+#define RTW89_PCIE_L1_STS_V1		0x80
+#define RTW89_BCFG_LINK_SPEED_MASK	GENMASK(19, 16)
+#define RTW89_PCIE_GEN1_SPEED		0x01
+#define RTW89_PCIE_GEN2_SPEED		0x02
+#define RTW89_PCIE_PHY_RATE		0x82
+#define RTW89_PCIE_PHY_RATE_MASK	GENMASK(1, 0)
 #define RTW89_PCIE_ASPM_CTRL		0x070F
 #define RTW89_L1DLY_MASK		GENMASK(5, 3)
 #define RTW89_L0DLY_MASK		GENMASK(2, 0)
@@ -516,8 +542,7 @@
 #define RTW89_PCIE_CLK_CTRL		0x0725
 #define RTW89_PCIE_RST_MSTATE		0x0B48
 #define RTW89_PCIE_BIT_CFG_RST_MSTATE	BIT(0)
-#define RTW89_PCIE_PHY_RATE		0x82
-#define RTW89_PCIE_PHY_RATE_MASK	GENMASK(1, 0)
+
 #define INTF_INTGRA_MINREF_V1	90
 #define INTF_INTGRA_HOSTREF_V1	100
 
diff --git a/drivers/net/wireless/realtek/rtw89/reg.h b/drivers/net/wireless/realtek/rtw89/reg.h
index 497c1e9263fc0..f55ea3f834f89 100644
--- a/drivers/net/wireless/realtek/rtw89/reg.h
+++ b/drivers/net/wireless/realtek/rtw89/reg.h
@@ -143,6 +143,18 @@
 #define R_AX_PMC_DBG_CTRL2 0x00CC
 #define B_AX_SYSON_DIS_PMCR_AX_WRMSK BIT(2)
 
+#define R_AX_PCIE_MIO_INTF 0x00E4
+#define B_AX_PCIE_MIO_ADDR_PAGE_V1_MASK GENMASK(20, 16)
+#define B_AX_PCIE_MIO_BYIOREG BIT(13)
+#define B_AX_PCIE_MIO_RE BIT(12)
+#define B_AX_PCIE_MIO_WE_MASK GENMASK(11, 8)
+#define MIO_WRITE_BYTE_ALL 0xF
+#define B_AX_PCIE_MIO_ADDR_MASK GENMASK(7, 0)
+#define MIO_ADDR_PAGE_MASK GENMASK(12, 8)
+
+#define R_AX_PCIE_MIO_INTD 0x00E8
+#define B_AX_PCIE_MIO_DATA_MASK GENMASK(31, 0)
+
 #define R_AX_SYS_CFG1 0x00F0
 #define B_AX_CHIP_VER_MASK GENMASK(15, 12)
 
-- 
2.25.1

[PATCH v2 4/5] wifi: rtw89: pci: enable CLK_REQ, ASPM, L1 and L1ss for 8852c

[Ping-Ke Shih]

From: Chin-Yen Lee <timlee@realtek.com>

8852CE controls CLKREQ, ASPM L1, L1ss via wifi registers
instead, so change them accordingly.

Signed-off-by: Chin-Yen Lee <timlee@realtek.com>
Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
---
 drivers/net/wireless/realtek/rtw89/pci.c | 91 +++++++++++++++++-------
 drivers/net/wireless/realtek/rtw89/pci.h | 12 ++++
 2 files changed, 79 insertions(+), 24 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw89/pci.c b/drivers/net/wireless/realtek/rtw89/pci.c
index c09d2ffc5005f..67baf495a6986 100644
--- a/drivers/net/wireless/realtek/rtw89/pci.c
+++ b/drivers/net/wireless/realtek/rtw89/pci.c
@@ -3291,6 +3291,7 @@ static int rtw89_pci_filter_out(struct rtw89_dev *rtwdev)
 
 static void rtw89_pci_clkreq_set(struct rtw89_dev *rtwdev, bool enable)
 {
+	enum rtw89_core_chip_id chip_id = rtwdev->chip->chip_id;
 	int ret;
 
 	if (rtw89_pci_disable_clkreq)
@@ -3301,19 +3302,33 @@ static void rtw89_pci_clkreq_set(struct rtw89_dev *rtwdev, bool enable)
 	if (ret)
 		rtw89_err(rtwdev, "failed to set CLKREQ Delay\n");
 
-	if (enable)
-		ret = rtw89_pci_config_byte_set(rtwdev, RTW89_PCIE_L1_CTRL,
-						RTW89_PCIE_BIT_CLK);
-	else
-		ret = rtw89_pci_config_byte_clr(rtwdev, RTW89_PCIE_L1_CTRL,
-						RTW89_PCIE_BIT_CLK);
-	if (ret)
-		rtw89_err(rtwdev, "failed to %s CLKREQ_L1, ret=%d",
-			  enable ? "set" : "unset", ret);
+	if (chip_id == RTL8852A) {
+		if (enable)
+			ret = rtw89_pci_config_byte_set(rtwdev,
+							RTW89_PCIE_L1_CTRL,
+							RTW89_PCIE_BIT_CLK);
+		else
+			ret = rtw89_pci_config_byte_clr(rtwdev,
+							RTW89_PCIE_L1_CTRL,
+							RTW89_PCIE_BIT_CLK);
+		if (ret)
+			rtw89_err(rtwdev, "failed to %s CLKREQ_L1, ret=%d",
+				  enable ? "set" : "unset", ret);
+	} else if (chip_id == RTL8852C) {
+		rtw89_write32_set(rtwdev, R_AX_PCIE_LAT_CTRL,
+				  B_AX_CLK_REQ_SEL_OPT | B_AX_CLK_REQ_SEL);
+		if (enable)
+			rtw89_write32_set(rtwdev, R_AX_L1_CLK_CTRL,
+					  B_AX_CLK_REQ_N);
+		else
+			rtw89_write32_clr(rtwdev, R_AX_L1_CLK_CTRL,
+					  B_AX_CLK_REQ_N);
+	}
 }
 
 static void rtw89_pci_aspm_set(struct rtw89_dev *rtwdev, bool enable)
 {
+	enum rtw89_core_chip_id chip_id = rtwdev->chip->chip_id;
 	u8 value = 0;
 	int ret;
 
@@ -3332,12 +3347,23 @@ static void rtw89_pci_aspm_set(struct rtw89_dev *rtwdev, bool enable)
 	if (ret)
 		rtw89_err(rtwdev, "failed to read ASPM Delay\n");
 
-	if (enable)
-		ret = rtw89_pci_config_byte_set(rtwdev, RTW89_PCIE_L1_CTRL,
-						RTW89_PCIE_BIT_L1);
-	else
-		ret = rtw89_pci_config_byte_clr(rtwdev, RTW89_PCIE_L1_CTRL,
-						RTW89_PCIE_BIT_L1);
+	if (chip_id == RTL8852A || chip_id == RTL8852B) {
+		if (enable)
+			ret = rtw89_pci_config_byte_set(rtwdev,
+							RTW89_PCIE_L1_CTRL,
+							RTW89_PCIE_BIT_L1);
+		else
+			ret = rtw89_pci_config_byte_clr(rtwdev,
+							RTW89_PCIE_L1_CTRL,
+							RTW89_PCIE_BIT_L1);
+	} else if (chip_id == RTL8852C) {
+		if (enable)
+			rtw89_write32_set(rtwdev, R_AX_PCIE_MIX_CFG_V1,
+					  B_AX_ASPM_CTRL_L1);
+		else
+			rtw89_write32_clr(rtwdev, R_AX_PCIE_MIX_CFG_V1,
+					  B_AX_ASPM_CTRL_L1);
+	}
 	if (ret)
 		rtw89_err(rtwdev, "failed to %s ASPM L1, ret=%d",
 			  enable ? "set" : "unset", ret);
@@ -3398,17 +3424,34 @@ static void rtw89_pci_link_cfg(struct rtw89_dev *rtwdev)
 
 static void rtw89_pci_l1ss_set(struct rtw89_dev *rtwdev, bool enable)
 {
+	enum rtw89_core_chip_id chip_id = rtwdev->chip->chip_id;
 	int ret;
 
-	if (enable)
-		ret = rtw89_pci_config_byte_set(rtwdev, RTW89_PCIE_TIMER_CTRL,
-						RTW89_PCIE_BIT_L1SUB);
-	else
-		ret = rtw89_pci_config_byte_clr(rtwdev, RTW89_PCIE_TIMER_CTRL,
-						RTW89_PCIE_BIT_L1SUB);
-	if (ret)
-		rtw89_err(rtwdev, "failed to %s L1SS, ret=%d",
-			  enable ? "set" : "unset", ret);
+	if (chip_id == RTL8852A || chip_id == RTL8852B) {
+		if (enable)
+			ret = rtw89_pci_config_byte_set(rtwdev,
+							RTW89_PCIE_TIMER_CTRL,
+							RTW89_PCIE_BIT_L1SUB);
+		else
+			ret = rtw89_pci_config_byte_clr(rtwdev,
+							RTW89_PCIE_TIMER_CTRL,
+							RTW89_PCIE_BIT_L1SUB);
+		if (ret)
+			rtw89_err(rtwdev, "failed to %s L1SS, ret=%d",
+				  enable ? "set" : "unset", ret);
+	} else if (chip_id == RTL8852C) {
+		ret = rtw89_pci_config_byte_clr(rtwdev, RTW89_PCIE_L1SS_STS_V1,
+						RTW89_PCIE_BIT_ASPM_L11 |
+						RTW89_PCIE_BIT_PCI_L11);
+		if (ret)
+			rtw89_warn(rtwdev, "failed to unset ASPM L1.1, ret=%d", ret);
+		if (enable)
+			rtw89_write32_clr(rtwdev, R_AX_PCIE_MIX_CFG_V1,
+					  B_AX_L1SUB_DISABLE);
+		else
+			rtw89_write32_set(rtwdev, R_AX_PCIE_MIX_CFG_V1,
+					  B_AX_L1SUB_DISABLE);
+	}
 }
 
 static void rtw89_pci_l1ss_cfg(struct rtw89_dev *rtwdev)
diff --git a/drivers/net/wireless/realtek/rtw89/pci.h b/drivers/net/wireless/realtek/rtw89/pci.h
index e80380271cd35..63dc6d4db6022 100644
--- a/drivers/net/wireless/realtek/rtw89/pci.h
+++ b/drivers/net/wireless/realtek/rtw89/pci.h
@@ -62,9 +62,16 @@
 #define B_AX_REQ_ENTR_L1		BIT(8)
 #define B_AX_L1SUB_DISABLE		BIT(0)
 
+#define R_AX_L1_CLK_CTRL		0x3010
+#define B_AX_CLK_REQ_N			BIT(1)
+
 #define R_AX_PCIE_BG_CLR		0x303C
 #define B_AX_BG_CLR_ASYNC_M3		BIT(4)
 
+#define R_AX_PCIE_LAT_CTRL		0x3044
+#define B_AX_CLK_REQ_SEL_OPT		BIT(1)
+#define B_AX_CLK_REQ_SEL		BIT(0)
+
 #define R_AX_PCIE_IO_RCY_M1 0x3100
 #define B_AX_PCIE_IO_RCY_P_M1 BIT(5)
 #define B_AX_PCIE_IO_RCY_WDT_P_M1 BIT(4)
@@ -531,6 +538,11 @@
 #define RTW89_PCIE_GEN2_SPEED		0x02
 #define RTW89_PCIE_PHY_RATE		0x82
 #define RTW89_PCIE_PHY_RATE_MASK	GENMASK(1, 0)
+#define RTW89_PCIE_L1SS_STS_V1		0x0168
+#define RTW89_PCIE_BIT_ASPM_L11		BIT(3)
+#define RTW89_PCIE_BIT_ASPM_L12		BIT(2)
+#define RTW89_PCIE_BIT_PCI_L11		BIT(1)
+#define RTW89_PCIE_BIT_PCI_L12		BIT(0)
 #define RTW89_PCIE_ASPM_CTRL		0x070F
 #define RTW89_L1DLY_MASK		GENMASK(5, 3)
 #define RTW89_L0DLY_MASK		GENMASK(2, 0)
-- 
2.25.1

[PATCH v2 5/5] wifi: rtw89: pci: correct suspend/resume setting for variant chips

[Ping-Ke Shih]

From: Chin-Yen Lee <timlee@realtek.com>

We find that suspend/resume tests cause 8852CE lost, because some pci
registers are changed for 8852CE. So, correct them accordingly.

Signed-off-by: Chin-Yen Lee <timlee@realtek.com>
Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
---
 drivers/net/wireless/realtek/rtw89/pci.c | 31 ++++++++++++++++++------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw89/pci.c b/drivers/net/wireless/realtek/rtw89/pci.c
index 67baf495a6986..ebe74d1bca73b 100644
--- a/drivers/net/wireless/realtek/rtw89/pci.c
+++ b/drivers/net/wireless/realtek/rtw89/pci.c
@@ -3648,14 +3648,20 @@ static int __maybe_unused rtw89_pci_suspend(struct device *dev)
 {
 	struct ieee80211_hw *hw = dev_get_drvdata(dev);
 	struct rtw89_dev *rtwdev = hw->priv;
+	enum rtw89_core_chip_id chip_id = rtwdev->chip->chip_id;
 
-	rtw89_write32_clr(rtwdev, R_AX_SYS_SDIO_CTRL,
-			  B_AX_PCIE_DIS_L2_CTRL_LDO_HCI);
 	rtw89_write32_set(rtwdev, R_AX_RSV_CTRL, B_AX_WLOCK_1C_BIT6);
 	rtw89_write32_set(rtwdev, R_AX_RSV_CTRL, B_AX_R_DIS_PRST);
 	rtw89_write32_clr(rtwdev, R_AX_RSV_CTRL, B_AX_WLOCK_1C_BIT6);
-	rtw89_write32_set(rtwdev, R_AX_PCIE_INIT_CFG1,
-			  B_AX_PCIE_PERST_KEEP_REG | B_AX_PCIE_TRAIN_KEEP_REG);
+	if (chip_id == RTL8852A || chip_id == RTL8852B) {
+		rtw89_write32_clr(rtwdev, R_AX_SYS_SDIO_CTRL,
+				  B_AX_PCIE_DIS_L2_CTRL_LDO_HCI);
+		rtw89_write32_set(rtwdev, R_AX_PCIE_INIT_CFG1,
+				  B_AX_PCIE_PERST_KEEP_REG | B_AX_PCIE_TRAIN_KEEP_REG);
+	} else {
+		rtw89_write32_clr(rtwdev, R_AX_PCIE_PS_CTRL_V1,
+				  B_AX_CMAC_EXIT_L1_EN | B_AX_DMAC0_EXIT_L1_EN);
+	}
 
 	return 0;
 }
@@ -3676,15 +3682,24 @@ static int __maybe_unused rtw89_pci_resume(struct device *dev)
 {
 	struct ieee80211_hw *hw = dev_get_drvdata(dev);
 	struct rtw89_dev *rtwdev = hw->priv;
+	enum rtw89_core_chip_id chip_id = rtwdev->chip->chip_id;
 
-	rtw89_write32_set(rtwdev, R_AX_SYS_SDIO_CTRL,
-			  B_AX_PCIE_DIS_L2_CTRL_LDO_HCI);
 	rtw89_write32_set(rtwdev, R_AX_RSV_CTRL, B_AX_WLOCK_1C_BIT6);
 	rtw89_write32_clr(rtwdev, R_AX_RSV_CTRL, B_AX_R_DIS_PRST);
 	rtw89_write32_clr(rtwdev, R_AX_RSV_CTRL, B_AX_WLOCK_1C_BIT6);
-	rtw89_write32_clr(rtwdev, R_AX_PCIE_INIT_CFG1,
-			  B_AX_PCIE_PERST_KEEP_REG | B_AX_PCIE_TRAIN_KEEP_REG);
+	if (chip_id == RTL8852A || chip_id == RTL8852B) {
+		rtw89_write32_set(rtwdev, R_AX_SYS_SDIO_CTRL,
+				  B_AX_PCIE_DIS_L2_CTRL_LDO_HCI);
+		rtw89_write32_clr(rtwdev, R_AX_PCIE_INIT_CFG1,
+				  B_AX_PCIE_PERST_KEEP_REG | B_AX_PCIE_TRAIN_KEEP_REG);
+	} else {
+		rtw89_write32_set(rtwdev, R_AX_PCIE_PS_CTRL_V1,
+				  B_AX_CMAC_EXIT_L1_EN | B_AX_DMAC0_EXIT_L1_EN);
+		rtw89_write32_clr(rtwdev, R_AX_PCIE_PS_CTRL_V1,
+				  B_AX_SEL_REQ_ENTR_L1);
+	}
 	rtw89_pci_l2_hci_ldo(rtwdev);
+	rtw89_pci_filter_out(rtwdev);
 	rtw89_pci_link_cfg(rtwdev);
 	rtw89_pci_l1ss_cfg(rtwdev);
 
-- 
2.25.1

Re: [PATCH v2 1/5] wifi: rtw89: add retry to change power_mode state

[Kalle Valo]

Ping-Ke Shih <pkshih@realtek.com> wrote:

> From: Chin-Yen Lee <timlee@realtek.com>
> 
> When starting to send heavy traffic in low power mode,
> driver will call multiple tx wake notify to wake firmware
> within a short time. In this situation, firmware may miss
> power mode change request from driver and leads to status
> error. So we change driver to call power_mode_change at most
> three times to make sure firmware could get the request.
> 
> Signed-off-by: Chin-Yen Lee <timlee@realtek.com>
> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>

5 patches applied to wireless-next.git, thanks.

48c0e34755a1 wifi: rtw89: add retry to change power_mode state
704052f55ffe wifi: rtw89: 8852c: set TBTT shift configuration
8f308ae3342c wifi: rtw89: pci: fix PCI PHY auto adaption by using software restore
843059d8193c wifi: rtw89: pci: enable CLK_REQ, ASPM, L1 and L1ss for 8852c
9e3d242fd3b4 wifi: rtw89: pci: correct suspend/resume setting for variant chips

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20220819064811.37700-2-pkshih@realtek.com/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v2 4/5] wifi: rtw89: pci: enable CLK_REQ, ASPM, L1 and L1ss for 8852c

[Bjorn Helgaas]

On Fri, Aug 19, 2022 at 02:48:10PM +0800, Ping-Ke Shih wrote:
> From: Chin-Yen Lee <timlee@realtek.com>
> 
> 8852CE controls CLKREQ, ASPM L1, L1ss via wifi registers
> instead, so change them accordingly.

> ...
>  static void rtw89_pci_l1ss_set(struct rtw89_dev *rtwdev, bool enable)
>  {
> +	enum rtw89_core_chip_id chip_id = rtwdev->chip->chip_id;
>  	int ret;
>  
> -	if (enable)
> -		ret = rtw89_pci_config_byte_set(rtwdev, RTW89_PCIE_TIMER_CTRL,
> -						RTW89_PCIE_BIT_L1SUB);
> -	else
> -		ret = rtw89_pci_config_byte_clr(rtwdev, RTW89_PCIE_TIMER_CTRL,
> -						RTW89_PCIE_BIT_L1SUB);
> -	if (ret)
> -		rtw89_err(rtwdev, "failed to %s L1SS, ret=%d",
> -			  enable ? "set" : "unset", ret);
> +	if (chip_id == RTL8852A || chip_id == RTL8852B) {
> +		if (enable)
> +			ret = rtw89_pci_config_byte_set(rtwdev,
> +							RTW89_PCIE_TIMER_CTRL,
> +							RTW89_PCIE_BIT_L1SUB);
> +		else
> +			ret = rtw89_pci_config_byte_clr(rtwdev,
> +							RTW89_PCIE_TIMER_CTRL,
> +							RTW89_PCIE_BIT_L1SUB);
> +		if (ret)
> +			rtw89_err(rtwdev, "failed to %s L1SS, ret=%d",
> +				  enable ? "set" : "unset", ret);
> +	} else if (chip_id == RTL8852C) {
> +		ret = rtw89_pci_config_byte_clr(rtwdev, RTW89_PCIE_L1SS_STS_V1,
> +						RTW89_PCIE_BIT_ASPM_L11 |
> +						RTW89_PCIE_BIT_PCI_L11);
> +		if (ret)
> +			rtw89_warn(rtwdev, "failed to unset ASPM L1.1, ret=%d", ret);
> +		if (enable)
> +			rtw89_write32_clr(rtwdev, R_AX_PCIE_MIX_CFG_V1,
> +					  B_AX_L1SUB_DISABLE);
> +		else
> +			rtw89_write32_set(rtwdev, R_AX_PCIE_MIX_CFG_V1,
> +					  B_AX_L1SUB_DISABLE);
> +	}
>  }

We get here via this path:

  rtw89_pci_probe
    rtw89_pci_l1ss_cfg
      pci_read_config_dword(pdev, l1ss_cap_ptr + PCI_L1SS_CTL1, &l1ss_ctrl);
      if (l1ss_ctrl & PCI_L1SS_CTL1_L1SS_MASK)
	rtw89_pci_l1ss_set(rtwdev, true);

This looks like it might be a problem because L1SS configuration is
owned by the PCI core, not by the device driver.  The PCI core
provides sysfs user interfaces that can enable and disable L1SS at
run-time without notification to the driver (see [1]).

The user may enable or disable L1SS using those sysfs interfaces, and
this code in the rtw89 driver will not be called.

Bjorn

P.S. rtw89_pci_l1ss_set() is only called from rtw89_pci_l1ss_cfg()
which always supplies "enable == true", so it looks like that
parameter is not needed.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/sysfs-bus-pci?id=v6.1#n410

RE: [PATCH v2 4/5] wifi: rtw89: pci: enable CLK_REQ, ASPM, L1 and L1ss for 8852c

[Ping-Ke Shih]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Wednesday, February 8, 2023 4:50 AM
> To: Ping-Ke Shih <pkshih@realtek.com>
> Cc: Kalle Valo <kvalo@kernel.org>; Leo.Li <leo.li@realtek.com>; Timlee <timlee@realtek.com>; Bernie Huang
> <phhuang@realtek.com>; linux-wireless@vger.kernel.org; linux-pci@vger.kernel.org
> Subject: Re: [PATCH v2 4/5] wifi: rtw89: pci: enable CLK_REQ, ASPM, L1 and L1ss for 8852c
> 
> On Fri, Aug 19, 2022 at 02:48:10PM +0800, Ping-Ke Shih wrote:
> > From: Chin-Yen Lee <timlee@realtek.com>
> >
> > 8852CE controls CLKREQ, ASPM L1, L1ss via wifi registers
> > instead, so change them accordingly.
> 
> > ...
> >  static void rtw89_pci_l1ss_set(struct rtw89_dev *rtwdev, bool enable)
> >  {
> > +	enum rtw89_core_chip_id chip_id = rtwdev->chip->chip_id;
> >  	int ret;
> >
> > -	if (enable)
> > -		ret = rtw89_pci_config_byte_set(rtwdev, RTW89_PCIE_TIMER_CTRL,
> > -						RTW89_PCIE_BIT_L1SUB);
> > -	else
> > -		ret = rtw89_pci_config_byte_clr(rtwdev, RTW89_PCIE_TIMER_CTRL,
> > -						RTW89_PCIE_BIT_L1SUB);
> > -	if (ret)
> > -		rtw89_err(rtwdev, "failed to %s L1SS, ret=%d",
> > -			  enable ? "set" : "unset", ret);
> > +	if (chip_id == RTL8852A || chip_id == RTL8852B) {
> > +		if (enable)
> > +			ret = rtw89_pci_config_byte_set(rtwdev,
> > +							RTW89_PCIE_TIMER_CTRL,
> > +							RTW89_PCIE_BIT_L1SUB);
> > +		else
> > +			ret = rtw89_pci_config_byte_clr(rtwdev,
> > +							RTW89_PCIE_TIMER_CTRL,
> > +							RTW89_PCIE_BIT_L1SUB);
> > +		if (ret)
> > +			rtw89_err(rtwdev, "failed to %s L1SS, ret=%d",
> > +				  enable ? "set" : "unset", ret);
> > +	} else if (chip_id == RTL8852C) {
> > +		ret = rtw89_pci_config_byte_clr(rtwdev, RTW89_PCIE_L1SS_STS_V1,
> > +						RTW89_PCIE_BIT_ASPM_L11 |
> > +						RTW89_PCIE_BIT_PCI_L11);
> > +		if (ret)
> > +			rtw89_warn(rtwdev, "failed to unset ASPM L1.1, ret=%d", ret);
> > +		if (enable)
> > +			rtw89_write32_clr(rtwdev, R_AX_PCIE_MIX_CFG_V1,
> > +					  B_AX_L1SUB_DISABLE);
> > +		else
> > +			rtw89_write32_set(rtwdev, R_AX_PCIE_MIX_CFG_V1,
> > +					  B_AX_L1SUB_DISABLE);
> > +	}
> >  }
> 
> We get here via this path:
> 
>   rtw89_pci_probe
>     rtw89_pci_l1ss_cfg
>       pci_read_config_dword(pdev, l1ss_cap_ptr + PCI_L1SS_CTL1, &l1ss_ctrl);
>       if (l1ss_ctrl & PCI_L1SS_CTL1_L1SS_MASK)
> 	rtw89_pci_l1ss_set(rtwdev, true);
> 
> This looks like it might be a problem because L1SS configuration is
> owned by the PCI core, not by the device driver.  The PCI core
> provides sysfs user interfaces that can enable and disable L1SS at
> run-time without notification to the driver (see [1]).
> 
> The user may enable or disable L1SS using those sysfs interfaces, and
> this code in the rtw89 driver will not be called.

The chunk of code is to configure L1SS of chip specific setting along with
standard PCI capability, and normally the setting and capability are consistent.
An exception is that PCI capability is enabled but chip specific setting is
disabled, when we want to use module parameter to disable chip specific
setting experimentally to resolve interoperability problem on some platforms. 

We don't suggest the use case that L1SS of PCI capability is disabled but 
chip specific setting is enabled, because hardware could get abnormal
occasionally. Also, it could also get unexpected behavior suddenly if we change
L1SS dynamically.

Summary:

   PCI capability      chip specific setting       comment
   --------------      ---------------------       -------
   enabled             enabled                     ok, currently support
   disabled            disabled                    ok, currently support
   enabled             disabled                    experimental case via module parameter
   disabled            enabled                     don't suggest


With above reasons, if users meet problem or unexpected result after changing
L1SS, we may tell them this hardware can't dynamically configure L1SS via
sysfs interfaces. 

Ping-Ke

Re: [PATCH v2 4/5] wifi: rtw89: pci: enable CLK_REQ, ASPM, L1 and L1ss for 8852c

[Bjorn Helgaas]

On Wed, Feb 08, 2023 at 09:15:50AM +0000, Ping-Ke Shih wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <helgaas@kernel.org>
> > On Fri, Aug 19, 2022 at 02:48:10PM +0800, Ping-Ke Shih wrote:
> > > From: Chin-Yen Lee <timlee@realtek.com>
> > >
> > > 8852CE controls CLKREQ, ASPM L1, L1ss via wifi registers
> > > instead, so change them accordingly.
> > ...

> > We get here via this path:
> > 
> >   rtw89_pci_probe
> >     rtw89_pci_l1ss_cfg
> >       pci_read_config_dword(pdev, l1ss_cap_ptr + PCI_L1SS_CTL1, &l1ss_ctrl);
> >       if (l1ss_ctrl & PCI_L1SS_CTL1_L1SS_MASK)
> > 	rtw89_pci_l1ss_set(rtwdev, true);
> > 
> > This looks like it might be a problem because L1SS configuration
> > is owned by the PCI core, not by the device driver.  The PCI core
> > provides sysfs user interfaces that can enable and disable L1SS at
> > run-time without notification to the driver (see [1]).
> > 
> > The user may enable or disable L1SS using those sysfs interfaces,
> > and this code in the rtw89 driver will not be called.
> 
> The chunk of code is to configure L1SS of chip specific setting
> along with standard PCI capability, and normally the setting and
> capability are consistent.  An exception is that PCI capability is
> enabled but chip specific setting is disabled, when we want to use
> module parameter to disable chip specific setting experimentally to
> resolve interoperability problem on some platforms. 

This is a significant usability problem.  An interoperability problem
means the device doesn't work correctly for some users, and there's no
obvious reason *why* it doesn't work, so they don't know how to fix
it.

Module parameters are not a solution because users don't know when
they are needed or how to use them.  This leads to situations like
[1,2,3], where users waste a lot of time flailing around to get the
device to work, and the eventual "solution" is to replace it with
something else:

  After replacing the Realtek card with Intel AX200 I do not have the
  described problem anymore.

> We don't suggest the use case that L1SS of PCI capability is
> disabled but chip specific setting is enabled, because hardware
> could get abnormal occasionally. Also, it could also get unexpected
> behavior suddenly if we change L1SS dynamically.
> 
> Summary:
> 
>    PCI capability      chip specific setting       comment
>    --------------      ---------------------       -------
>    enabled             enabled                     ok, currently support
>    disabled            disabled                    ok, currently support
>    enabled             disabled                    experimental case via module parameter
>    disabled            enabled                     don't suggest

I think the fact that you need chip-specific code here is a hardware
defect in the rtw89 device.  The whole point of L1SS being in the PCIe
spec is so generic software can configure it without having to know
chip-specific details.

> With above reasons, if users meet problem or unexpected result after
> changing L1SS, we may tell them this hardware can't dynamically
> configure L1SS via sysfs interfaces. 

How can we make this better, so the device works and users never have
to specify those module parameters?

Would it help if we had a way to make a quirk that meant "never enable
L1SS for this device"?  Obviously that's not ideal because we want the
power savings of L1SS, but the power saving is only worthwhile if the
device always *works*.

Or maybe we could have a quirk that means "the PCI core will never
change the L1SS configuration for this device"?  Would that help?

Bjorn

[1] https://github.com/lwfinger/rtw89/issues/41
[2] https://bbs.archlinux.org/viewtopic.php?id=273515
[3] https://bugs.launchpad.net/ubuntu/+source/linux-firmware/+bug/1971656

RE: [PATCH v2 4/5] wifi: rtw89: pci: enable CLK_REQ, ASPM, L1 and L1ss for 8852c

[Ping-Ke Shih]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Thursday, February 9, 2023 6:04 AM
> To: Ping-Ke Shih <pkshih@realtek.com>
> Cc: Kalle Valo <kvalo@kernel.org>; Leo.Li <leo.li@realtek.com>; Timlee <timlee@realtek.com>; Bernie Huang
> <phhuang@realtek.com>; linux-wireless@vger.kernel.org; linux-pci@vger.kernel.org
> Subject: Re: [PATCH v2 4/5] wifi: rtw89: pci: enable CLK_REQ, ASPM, L1 and L1ss for 8852c
> 
> On Wed, Feb 08, 2023 at 09:15:50AM +0000, Ping-Ke Shih wrote:
> > > -----Original Message-----
> > > From: Bjorn Helgaas <helgaas@kernel.org>
> > > On Fri, Aug 19, 2022 at 02:48:10PM +0800, Ping-Ke Shih wrote:
> > > > From: Chin-Yen Lee <timlee@realtek.com>
> > > >
> > > > 8852CE controls CLKREQ, ASPM L1, L1ss via wifi registers
> > > > instead, so change them accordingly.
> > > ...
> 
> > > We get here via this path:
> > >
> > >   rtw89_pci_probe
> > >     rtw89_pci_l1ss_cfg
> > >       pci_read_config_dword(pdev, l1ss_cap_ptr + PCI_L1SS_CTL1, &l1ss_ctrl);
> > >       if (l1ss_ctrl & PCI_L1SS_CTL1_L1SS_MASK)
> > > 	rtw89_pci_l1ss_set(rtwdev, true);
> > >
> > > This looks like it might be a problem because L1SS configuration
> > > is owned by the PCI core, not by the device driver.  The PCI core
> > > provides sysfs user interfaces that can enable and disable L1SS at
> > > run-time without notification to the driver (see [1]).
> > >
> > > The user may enable or disable L1SS using those sysfs interfaces,
> > > and this code in the rtw89 driver will not be called.
> >
> > The chunk of code is to configure L1SS of chip specific setting
> > along with standard PCI capability, and normally the setting and
> > capability are consistent.  An exception is that PCI capability is
> > enabled but chip specific setting is disabled, when we want to use
> > module parameter to disable chip specific setting experimentally to
> > resolve interoperability problem on some platforms.
> 
> This is a significant usability problem.  An interoperability problem
> means the device doesn't work correctly for some users, and there's no
> obvious reason *why* it doesn't work, so they don't know how to fix
> it.
> 
> Module parameters are not a solution because users don't know when
> they are needed or how to use them.  This leads to situations like
> [1,2,3], where users waste a lot of time flailing around to get the
> device to work, and the eventual "solution" is to replace it with
> something else:
> 
>   After replacing the Realtek card with Intel AX200 I do not have the
>   described problem anymore.

A cause of interoperability problem could be due to PCI bridge side
configured by BIOS. We have fixed this kind of problem many times before.
Maybe, this device has less tolerance to handle PCI signals. The module
parameter is an alternative way to help users to resolve the problem in
their platforms. If people buy a computer with this device built-in, he
will meet this problem in low probability because ODM will verify this
ahead. If people buy this device themselves to install to their platforms,
it is hard to guarantee it can work well, because cause of interoperability
could be bride side as mentioned in beginning. 

> 
> > We don't suggest the use case that L1SS of PCI capability is
> > disabled but chip specific setting is enabled, because hardware
> > could get abnormal occasionally. Also, it could also get unexpected
> > behavior suddenly if we change L1SS dynamically.
> >
> > Summary:
> >
> >    PCI capability      chip specific setting       comment
> >    --------------      ---------------------       -------
> >    enabled             enabled                     ok, currently support
> >    disabled            disabled                    ok, currently support
> >    enabled             disabled                    experimental case via module parameter
> >    disabled            enabled                     don't suggest
> 
> I think the fact that you need chip-specific code here is a hardware
> defect in the rtw89 device.  The whole point of L1SS being in the PCIe
> spec is so generic software can configure it without having to know
> chip-specific details.
> 
> > With above reasons, if users meet problem or unexpected result after
> > changing L1SS, we may tell them this hardware can't dynamically
> > configure L1SS via sysfs interfaces.
> 
> How can we make this better, so the device works and users never have
> to specify those module parameters?

Normally, users don't need to specify this module parameter. If it's really
needed, we can add a quirk along with DMI vendor and product name to configure
automatically. But, indeed we still need a user to try that module parameter
can work on a certain platform.

> 
> Would it help if we had a way to make a quirk that meant "never enable
> L1SS for this device"?  Obviously that's not ideal because we want the
> power savings of L1SS, but the power saving is only worthwhile if the
> device always *works*.
> 
> Or maybe we could have a quirk that means "the PCI core will never
> change the L1SS configuration for this device"?  Would that help?
> 

In fact, we only don't suggest to change L1SS "dynamically". Initially,
enable or disable L1SS is usable, and driver will set chip-specific 
setting along with standard PCI configuration.

So, I think it would be okay to have a quirk that "never change L1SS dynamically".
But, I'm not sure if switching L1SS is a common option for average users?
I mean L1SS normally is configured by developers only, so restrictions aren't
always good to them, because they should know what they are doing. 

Ping-Ke

Re: [PATCH v2 4/5] wifi: rtw89: pci: enable CLK_REQ, ASPM, L1 and L1ss for 8852c

[Bjorn Helgaas]

On Mon, Feb 13, 2023 at 01:46:51AM +0000, Ping-Ke Shih wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <helgaas@kernel.org>
> > Sent: Thursday, February 9, 2023 6:04 AM
> > To: Ping-Ke Shih <pkshih@realtek.com>
> > Cc: Kalle Valo <kvalo@kernel.org>; Leo.Li <leo.li@realtek.com>; Timlee <timlee@realtek.com>; Bernie Huang
> > <phhuang@realtek.com>; linux-wireless@vger.kernel.org; linux-pci@vger.kernel.org
> > Subject: Re: [PATCH v2 4/5] wifi: rtw89: pci: enable CLK_REQ, ASPM, L1 and L1ss for 8852c

It would be better if your email client allows you to respond without
the unnecessary repetition of the From/To/Cc/Subject lines above.  For
example, this would be sufficient:

  > On Thursday, February 9, 2023 6:04 AM, Bjorn Helgaas wrote:
  > > On Wed, Feb 08, 2023 at 09:15:50AM +0000, Ping-Ke Shih wrote:
  > > > On Fri, Aug 19, 2022 at 02:48:10PM, Ping-Ke Shih wrote:

Then it's shorter and easier to figure out who wrote what.

> > On Wed, Feb 08, 2023 at 09:15:50AM +0000, Ping-Ke Shih wrote:
> > > > -----Original Message-----
> > > > From: Bjorn Helgaas <helgaas@kernel.org>

> > > The chunk of code is to configure L1SS of chip specific setting
> > > along with standard PCI capability, and normally the setting and
> > > capability are consistent.  An exception is that PCI capability is
> > > enabled but chip specific setting is disabled, when we want to use
> > > module parameter to disable chip specific setting experimentally to
> > > resolve interoperability problem on some platforms.
> > 
> > This is a significant usability problem.  An interoperability problem
> > means the device doesn't work correctly for some users, and there's no
> > obvious reason *why* it doesn't work, so they don't know how to fix
> > it.
> > 
> > Module parameters are not a solution because users don't know when
> > they are needed or how to use them.  This leads to situations like
> > [1,2,3], where users waste a lot of time flailing around to get the
> > device to work, and the eventual "solution" is to replace it with
> > something else:
> > 
> >   After replacing the Realtek card with Intel AX200 I do not have the
> >   described problem anymore.
> 
> A cause of interoperability problem could be due to PCI bridge side
> configured by BIOS. We have fixed this kind of problem many times before.
> Maybe, this device has less tolerance to handle PCI signals. The module
> parameter is an alternative way to help users to resolve the problem in
> their platforms. If people buy a computer with this device built-in, he
> will meet this problem in low probability because ODM will verify this
> ahead. If people buy this device themselves to install to their platforms,
> it is hard to guarantee it can work well, because cause of interoperability
> could be bride side as mentioned in beginning. 

The BIOS or PCI core should configure both the bridge and the endpoint
so they are consistent.  If the driver needs to do something, e.g.,
via a module parameter, that means there's a BIOS or PCI core defect
that should be fixed.  It should be fixed in the PCI core, not in the
individual driver.

> > > We don't suggest the use case that L1SS of PCI capability is
> > > disabled but chip specific setting is enabled, because hardware
> > > could get abnormal occasionally. Also, it could also get unexpected
> > > behavior suddenly if we change L1SS dynamically.
> > >
> > > Summary:
> > >
> > >    PCI capability      chip specific setting       comment
> > >    --------------      ---------------------       -------
> > >    enabled             enabled                     ok, currently support
> > >    disabled            disabled                    ok, currently support
> > >    enabled             disabled                    experimental case via module parameter
> > >    disabled            enabled                     don't suggest
> > 
> > I think the fact that you need chip-specific code here is a hardware
> > defect in the rtw89 device.  The whole point of L1SS being in the PCIe
> > spec is so generic software can configure it without having to know
> > chip-specific details.
> > 
> > > With above reasons, if users meet problem or unexpected result after
> > > changing L1SS, we may tell them this hardware can't dynamically
> > > configure L1SS via sysfs interfaces.
> > 
> > How can we make this better, so the device works and users never have
> > to specify those module parameters?
> 
> Normally, users don't need to specify this module parameter. If it's
> really needed, we can add a quirk along with DMI vendor and product
> name to configure automatically. But, indeed we still need a user to
> try that module parameter can work on a certain platform.

The fact that the parameter exists means *some* users do need it.  And
that is a huge problem because those users don't *know* they need it;
they just see a device that doesn't work, and they don't know why.
All they can do is try random combinations of parameters to see what
seems to work.  This just doesn't scale for users that deal with a
dozen different devices in their system.  We can't expect them to
fiddle with module parameters for each.

> > Would it help if we had a way to make a quirk that meant "never enable
> > L1SS for this device"?  Obviously that's not ideal because we want the
> > power savings of L1SS, but the power saving is only worthwhile if the
> > device always *works*.
> > 
> > Or maybe we could have a quirk that means "the PCI core will never
> > change the L1SS configuration for this device"?  Would that help?
> 
> In fact, we only don't suggest to change L1SS "dynamically". Initially,
> enable or disable L1SS is usable, and driver will set chip-specific 
> setting along with standard PCI configuration.

If your device is implemented correctly per the PCIe spec, there
should be no problem with changing L1SS configuration dynamically.  Of
course if there are PCI core defects that mean it doesn't work, those
can and should be fixed.

> So, I think it would be okay to have a quirk that "never change L1SS
> dynamically".  But, I'm not sure if switching L1SS is a common
> option for average users?  I mean L1SS normally is configured by
> developers only, so restrictions aren't always good to them, because
> they should know what they are doing. 

There are sysfs options for changing L1SS configuration, and it's
reasonable for users to change that based on changing workload.  I
expect tools like powertop to take advantage of that eventually.

Bjorn