[PATCH net-next v4 0/8] r8152: refactor and extend RTL8157/8159 support

[PATCH net-next v4 0/8] r8152: refactor and extend RTL8157/8159 support

[Chih Kai Hsu]

RTL8157 and RTL8159 have different init, enable, up/down, hw_phy_cfg,
and PHY access sequences from RTL8156.  This series splits the shared
callbacks into per-chip variants and adds the missing RTL8157/8159
functionality: dedicated unload and change_mtu callbacks, TGPHY register
access via phy_read/phy_write pointers in struct rtl_ops, flow control
patch support through rtl_fc_pause_pkt_en(), and UPS support via
r8157_ups_en().

v4:
- Patch #1: make wait_autoload_done() bail out immediately on
  RTL8152_INACCESSIBLE instead of waiting out the full 10 second
  timeout.
- Patch #6: propagate PHY access failures instead of ignoring them.
  ocp_reg_read()/ocp_reg_write() and every helper built on top of them
  (sram_read/write, sram2_read/write, the ocp_reg_*_bits and MDIO bit
  helpers) now return int and stop silently operating on stale data
  when a TGPHY access times out.

v3:
Rebase on top of latest net-next to resolve merge conflicts.

v2:
- For patch #1, use read_poll_timeout() to replace the manual
  for-loop polling.
- For patch #2 and #3, rewrite the commit message.

v1:
RTL8157 and RTL8159 have different init, enable, up/down, hw_phy_cfg,
and PHY access sequences from RTL8156.  This series splits the shared
callbacks into per-chip variants and adds the missing RTL8157/8159
functionality: dedicated unload and change_mtu callbacks, TGPHY register
access via phy_read/phy_write pointers in struct rtl_ops, flow control
patch support through rtl_fc_pause_pkt_en(), and UPS support via
r8157_ups_en().

Chih Kai Hsu (8):
  r8152: split r8156_init per chip and add missing init writes
  r8152: split RTL_VER_17 into QFN68 and QFN100 package variants
  r8152: split rtl8156_enable/up/down into per-chip-family functions
  r8152: split r8157_hw_phy_cfg into RTL8157 and RTL8159 variants
  r8152: add rtl8157_unload and rtl8157_change_mtu
  r8152: add TGPHY register access for RTL8157 and RTL8159
  r8152: extract rtl_fc_pause_pkt_en() and apply it to RTL8156/8157/8159
  r8152: enable UPS for RTL8157 and RTL8159 QFN68

 drivers/net/usb/r8152.c | 1655 ++++++++++++++++++++++++++++++---------
 1 file changed, 1306 insertions(+), 349 deletions(-)

-- 
2.34.1

[PATCH net-next v4 1/8] r8152: split r8156_init per chip and add missing init writes

[Chih Kai Hsu]

RTL8156, RTL8156B, RTL8157 and RTL8159 shared a single r8156_init(),
gated by a dense set of tp->version range checks. That made the init
sequence hard to follow, and some checks (e.g. the FC-timer block,
which was fenced to "RTL_VER_12 <= version <= RTL_VER_15") silently
skipped RTL8156 (VER_10/11) and RTL8157/RTL8159 (VER_16/17)
altogether even though nothing chip-specific justified the gap.

Split r8156_init() into one function per chip family:
- r8156_init  (VER_10/11)
- r8156b_init (VER_12/13/15)
- r8157_init  (VER_16)
- r8159_init  (VER_17)

While splitting:

- Factor the repeated "poll PLA_BOOT_CTRL for AUTOLOAD_DONE" loop
  into wait_autoload_done(), reusing read_poll_timeout() instead of
  the ad-hoc 500 * msleep(20) busy loops. r8153_init(), r8153b_init(),
  r8153c_init(), r8153b_ups_en() and r8153c_ups_en() are converted to
  use it too.

  wait_autoload_done() only reports failure (nonzero) when
  RTL8152_INACCESSIBLE is set; a plain AUTOLOAD_DONE timeout is
  logged but otherwise treated as success, same as every one of the
  loops it replaces, which only bailed out early on
  RTL8152_INACCESSIBLE and simply fell through to the rest of the
  init sequence on their own timeout. This matters because the two
  failure modes are not equivalent: once RTL8152_INACCESSIBLE is
  set, generic_ocp_read()/generic_ocp_write() short-circuit to
  -ENODEV without touching the device, so every OCP access in the
  remaining init sequence becomes a no-op and returning early costs
  nothing. A plain timeout, on the other hand, means the device is
  still reachable, so skipping the rest of a chip bring-up sequence
  would leave real hardware state half-configured -- keeping going
  and letting each subsequent access fail or succeed on its own is
  safer than aborting mid-sequence.

- Add PLA_RMT_WAKE (RMT_WAKE_EN) and PLA_INDICATE_FALG's
  PREBOOT_OPTION bit, and clear both from every split init function.
  Neither bit was ever touched by the previous shared r8156_init().

- Add r8156b_u2phy_backup(), a new sequence of OCP writes to
  0xd3ce/0xd3d0/0xd3d4/0xd3d8/0xd3dc, called only from r8156b_init().

- Explicitly clear AUTO_SPEEDUP as its own write in r8156b_init(),
  r8157_init() and r8159_init(), instead of folding it into the
  FLOW_CTRL_PATCH_2 read-modify-write that only ran for VER_12-15.
  This means RTL8157/RTL8159 (r8157_init/r8159_init) now also clear
  AUTO_SPEEDUP during init, which the old shared function never did
  for them.

- Add "disable Interrupt Mitigation" (0xcf04, bits 0/1/2/7) to
  r8157_init() and r8159_init(). This write previously only existed
  in rtl8153_unload() for VER_16/17; it is now also applied during
  init.

- Add "clear Warm RST / Bus RST event flag" (0xcd06 BIT(11)) to
  r8157_init() and r8159_init(), and r8159_init()-only "disable FW
  u1u2 patch option" (0xb9a6 BIT(0)) and "enable u3phy patch backup"
  (0xb9a2 = 0x0448). None of these existed in the previous shared
  function.

Signed-off-by: Chih Kai Hsu <hsu.chih.kai@realtek.com>
---
 drivers/net/usb/r8152.c | 489 ++++++++++++++++++++++++++++------------
 1 file changed, 350 insertions(+), 139 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index af3d7dcb2f141..00b0fda306c30 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -78,6 +78,7 @@
 #define PLA_EEE_TXTWSYS_2P5G	0xe058
 #define PLA_EEEP_CR		0xe080
 #define PLA_MAC_PWR_CTRL	0xe0c0
+#define PLA_RMT_WAKE		0xe0c8
 #define PLA_MAC_PWR_CTRL2	0xe0ca
 #define PLA_MAC_PWR_CTRL3	0xe0cc
 #define PLA_MAC_PWR_CTRL4	0xe0ce
@@ -371,6 +372,9 @@
 #define MCU_CLK_RATIO_MASK	0x0f0f0f0f
 #define ALDPS_SPDWN_RATIO	0x0f87
 
+/* PLA_RMT_WAKE */
+#define RMT_WAKE_EN		BIT(0)
+
 /* PLA_MAC_PWR_CTRL2 */
 #define EEE_SPDWN_RATIO		0x8007
 #define MAC_CLK_SPDWN_EN	BIT(15)
@@ -417,6 +421,7 @@
 
 /* PLA_INDICATE_FALG */
 #define UPCOMING_RUNTIME_D3	BIT(0)
+#define PREBOOT_OPTION		BIT(1)
 
 /* PLA_MACDBG_PRE and PLA_MACDBG_POST */
 #define DEBUG_OE		BIT(0)
@@ -542,6 +547,7 @@
 #define RX_AGG_DISABLE		0x0010
 #define RX_ZERO_EN		0x0080
 #define RX_DESC_16B		0x0400
+#define RX_END_TRANSFER_EN	BIT(11)
 
 /* USB_U2P3_CTRL */
 #define U2P3_ENABLE		0x0001
@@ -4172,6 +4178,23 @@ static u16 r8153_phy_status(struct r8152 *tp, u16 desired)
 	return data;
 }
 
+static int wait_autoload_done(struct r8152 *tp)
+{
+	u16 ocp_data;
+	int ret;
+
+	ret = read_poll_timeout(ocp_read_word, ocp_data,
+				test_bit(RTL8152_INACCESSIBLE, &tp->flags) ||
+				(ocp_data & AUTOLOAD_DONE),
+				20000, 10 * USEC_PER_SEC, false, tp,
+				MCU_TYPE_PLA, PLA_BOOT_CTRL);
+
+	if (ret)
+		dev_err(&tp->intf->dev, "autoload done timeout\n");
+
+	return test_bit(RTL8152_INACCESSIBLE, &tp->flags) ? -ENODEV : 0;
+}
+
 static void r8153b_ups_en(struct r8152 *tp, bool enable)
 {
 	if (enable) {
@@ -4190,16 +4213,8 @@ static void r8153b_ups_en(struct r8152 *tp, bool enable)
 				  UPS_FORCE_PWR_DOWN);
 
 		if (ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0) & PCUT_STATUS) {
-			int i;
-
-			for (i = 0; i < 500; i++) {
-				if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
-					return;
-				if (ocp_read_word(tp, MCU_TYPE_PLA, PLA_BOOT_CTRL) &
-				    AUTOLOAD_DONE)
-					break;
-				msleep(20);
-			}
+			if (wait_autoload_done(tp))
+				return;
 
 			tp->rtl_ops.hw_phy_cfg(tp);
 
@@ -4227,16 +4242,8 @@ static void r8153c_ups_en(struct r8152 *tp, bool enable)
 				  UPS_FORCE_PWR_DOWN);
 
 		if (ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0) & PCUT_STATUS) {
-			int i;
-
-			for (i = 0; i < 500; i++) {
-				if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
-					return;
-				if (ocp_read_word(tp, MCU_TYPE_PLA, PLA_BOOT_CTRL) &
-				    AUTOLOAD_DONE)
-					break;
-				msleep(20);
-			}
+			if (wait_autoload_done(tp))
+				return;
 
 			tp->rtl_ops.hw_phy_cfg(tp);
 
@@ -7225,22 +7232,14 @@ static void r8152b_init(struct r8152 *tp)
 static void r8153_init(struct r8152 *tp)
 {
 	u32 ocp_data;
-	int i;
 
 	if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
 		return;
 
 	r8153_u1u2en(tp, false);
 
-	for (i = 0; i < 500; i++) {
-		if (ocp_read_word(tp, MCU_TYPE_PLA, PLA_BOOT_CTRL) &
-		    AUTOLOAD_DONE)
-			break;
-
-		msleep(20);
-		if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
-			break;
-	}
+	if (wait_autoload_done(tp))
+		return;
 
 	r8153_phy_status(tp, 0);
 
@@ -7341,22 +7340,13 @@ static void r8153_init(struct r8152 *tp)
 
 static void r8153b_init(struct r8152 *tp)
 {
-	int i;
-
 	if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
 		return;
 
 	r8153b_u1u2en(tp, false);
 
-	for (i = 0; i < 500; i++) {
-		if (ocp_read_word(tp, MCU_TYPE_PLA, PLA_BOOT_CTRL) &
-		    AUTOLOAD_DONE)
-			break;
-
-		msleep(20);
-		if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
-			break;
-	}
+	if (wait_autoload_done(tp))
+		return;
 
 	r8153_phy_status(tp, 0);
 
@@ -7411,8 +7401,6 @@ static void r8153b_init(struct r8152 *tp)
 
 static void r8153c_init(struct r8152 *tp)
 {
-	int i;
-
 	if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
 		return;
 
@@ -7425,15 +7413,8 @@ static void r8153c_init(struct r8152 *tp)
 
 	ocp_word_set_bits(tp, MCU_TYPE_USB, 0xcbf0, BIT(1));
 
-	for (i = 0; i < 500; i++) {
-		if (ocp_read_word(tp, MCU_TYPE_PLA, PLA_BOOT_CTRL) &
-		    AUTOLOAD_DONE)
-			break;
-
-		msleep(20);
-		if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
-			return;
-	}
+	if (wait_autoload_done(tp))
+		return;
 
 	r8153_phy_status(tp, 0);
 
@@ -8330,90 +8311,132 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)
 	set_bit(PHY_RESET, &tp->flags);
 }
 
-static int r8159_wait_backup_restore(struct r8152 *tp)
+static void r8156_init(struct r8152 *tp)
 {
-	u32 ocp_data;
+	u16 data;
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0);
-	if (!(ocp_data & PCUT_STATUS))
-		return 0;
+	if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
+		return;
 
-	return poll_timeout_us(ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_GPHY_CTRL),
-			       ocp_data & BACKUP_RESTRORE, 200, 20000, false);
+	ocp_byte_clr_bits(tp, MCU_TYPE_USB, USB_ECM_OP, EN_ALL_SPEED);
+
+	ocp_write_word(tp, MCU_TYPE_USB, USB_SPEED_OPTION, 0);
+
+	ocp_word_set_bits(tp, MCU_TYPE_USB, USB_ECM_OPTION, BYPASS_MAC_RESET);
+
+	r8153b_u1u2en(tp, false);
+
+	if (wait_autoload_done(tp))
+		return;
+
+	data = r8153_phy_status(tp, 0);
+	if (data == PHY_STAT_EXT_INIT)
+		ocp_reg_clr_bits(tp, 0xa468, BIT(3) | BIT(1));
+
+	r8152_mdio_test_and_clr_bit(tp, MII_BMCR, BMCR_PDOWN);
+
+	data = r8153_phy_status(tp, PHY_STAT_LAN_ON);
+
+	r8153_u2p3en(tp, false);
+
+	/* MSC timer = 0xfff * 8ms = 32760 ms */
+	ocp_write_word(tp, MCU_TYPE_USB, USB_MSC_TIMER, 0x0fff);
+
+	/* U1/U2/L1 idle timer. 500 us */
+	ocp_write_word(tp, MCU_TYPE_USB, USB_U1U2_TIMER, 500);
+
+	r8153b_power_cut_en(tp, false);
+	r8156_ups_en(tp, false);
+	r8153_queue_wake(tp, false);
+	rtl_runtime_suspend_enable(tp, false);
+
+	ocp_byte_clr_bits(tp, MCU_TYPE_PLA, PLA_INDICATE_FALG, PREBOOT_OPTION);
+
+	ocp_byte_clr_bits(tp, MCU_TYPE_PLA, PLA_RMT_WAKE, RMT_WAKE_EN);
+
+	if (tp->udev->speed >= USB_SPEED_SUPER)
+		r8153b_u1u2en(tp, true);
+
+	usb_enable_lpm(tp->udev);
+
+	r8156_mac_clk_spd(tp, true);
+
+	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3,
+			  PLA_MCU_SPDWN_EN);
+
+	if (rtl8152_get_speed(tp) & LINK_STATUS)
+		ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_EXTRA_STATUS,
+				  CUR_LINK_OK | POLL_LINK_CHG);
+	else
+		ocp_word_w0w1(tp, MCU_TYPE_PLA, PLA_EXTRA_STATUS, CUR_LINK_OK,
+			      POLL_LINK_CHG);
+
+	set_bit(GREEN_ETHERNET, &tp->flags);
+
+	ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL,
+			  RX_AGG_DISABLE | RX_ZERO_EN);
+
+	ocp_byte_set_bits(tp, MCU_TYPE_USB, USB_BMU_CONFIG, ACT_ODMA);
+
+	r8156_mdio_force_mode(tp);
+	rtl_tally_reset(tp);
+
+	tp->coalesce = 15000; /* 15 us */
 }
 
-static void r8156_init(struct r8152 *tp)
+static void r8156b_u2phy_backup(struct r8152 *tp)
+{
+	ocp_write_word(tp, MCU_TYPE_USB, 0xd3ce, 0x181b);
+	ocp_write_dword(tp, MCU_TYPE_USB, 0xd3d0, 0x616ccd99);
+	ocp_write_dword(tp, MCU_TYPE_USB, 0xd3d4, 0x08fc8101);
+	ocp_write_dword(tp, MCU_TYPE_USB, 0xd3d8, 0x159b1100);
+	ocp_write_word(tp, MCU_TYPE_USB, 0xd3dc, 0x0a00);
+}
+
+static void r8156b_init(struct r8152 *tp)
 {
 	u32 ocp_data;
 	u16 data;
-	int i;
 
 	if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
 		return;
 
-	if (tp->version == RTL_VER_16 || tp->version == RTL_VER_17) {
-		ocp_byte_set_bits(tp, MCU_TYPE_USB, 0xcffe, BIT(3));
-		ocp_byte_clr_bits(tp, MCU_TYPE_USB, 0xd3ca, BIT(0));
-	}
-
 	ocp_byte_clr_bits(tp, MCU_TYPE_USB, USB_ECM_OP, EN_ALL_SPEED);
 
-	if (tp->version < RTL_VER_16)
-		ocp_write_word(tp, MCU_TYPE_USB, USB_SPEED_OPTION, 0);
+	ocp_write_word(tp, MCU_TYPE_USB, USB_SPEED_OPTION, 0);
 
 	ocp_word_set_bits(tp, MCU_TYPE_USB, USB_ECM_OPTION, BYPASS_MAC_RESET);
 
-	if (tp->version >= RTL_VER_12 && tp->version <= RTL_VER_15)
-		ocp_word_set_bits(tp, MCU_TYPE_USB, USB_U2P3_CTRL, RX_DETECT8);
+	ocp_word_set_bits(tp, MCU_TYPE_USB, USB_U2P3_CTRL, RX_DETECT8);
 
 	r8153b_u1u2en(tp, false);
 
 	switch (tp->version) {
 	case RTL_VER_13:
 	case RTL_VER_15:
-	case RTL_VER_16:
-	case RTL_VER_17:
 		r8156b_wait_loading_flash(tp);
 		break;
 	default:
 		break;
 	}
 
-	for (i = 0; i < 500; i++) {
-		if (ocp_read_word(tp, MCU_TYPE_PLA, PLA_BOOT_CTRL) &
-		    AUTOLOAD_DONE)
-			break;
-
-		msleep(20);
-		if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
-			return;
-	}
-
-	if (tp->version == RTL_VER_17 && r8159_wait_backup_restore(tp)) {
-		rtl_set_inaccessible(tp);
-		dev_err(&tp->intf->dev, "init failed, backup-restore timed out\n");
+	if (wait_autoload_done(tp))
 		return;
-	}
 
 	data = r8153_phy_status(tp, 0);
 	if (data == PHY_STAT_EXT_INIT) {
 		ocp_reg_clr_bits(tp, 0xa468, BIT(3) | BIT(1));
-		if (tp->version >= RTL_VER_12)
-			ocp_reg_clr_bits(tp, 0xa466, BIT(0));
+		ocp_reg_clr_bits(tp, 0xa466, BIT(0));
 	}
 
-	data = r8152_mdio_read(tp, MII_BMCR);
-	if (data & BMCR_PDOWN) {
-		data &= ~BMCR_PDOWN;
-		r8152_mdio_write(tp, MII_BMCR, data);
-	}
+	r8152_mdio_test_and_clr_bit(tp, MII_BMCR, BMCR_PDOWN);
 
 	data = r8153_phy_status(tp, PHY_STAT_LAN_ON);
 
-	if (tp->version >= RTL_VER_16)
-		r8157_u2p3en(tp, false);
-	else
-		r8153_u2p3en(tp, false);
+	r8153_u2p3en(tp, false);
+
+	/* Disable Auto Speed up */
+	ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_FW_CTRL, AUTO_SPEEDUP);
 
 	/* MSC timer = 0xfff * 8ms = 32760 ms */
 	ocp_write_word(tp, MCU_TYPE_USB, USB_MSC_TIMER, 0x0fff);
@@ -8421,73 +8444,261 @@ static void r8156_init(struct r8152 *tp)
 	/* U1/U2/L1 idle timer. 500 us */
 	ocp_write_word(tp, MCU_TYPE_USB, USB_U1U2_TIMER, 500);
 
-	if (tp->version >= RTL_VER_16)
-		r8157_power_cut_en(tp, false);
-	else
-		r8153b_power_cut_en(tp, false);
+	r8156b_u2phy_backup(tp);
 
+	r8153b_power_cut_en(tp, false);
 	r8156_ups_en(tp, false);
 	r8153_queue_wake(tp, false);
 	rtl_runtime_suspend_enable(tp, false);
 
+	ocp_byte_clr_bits(tp, MCU_TYPE_PLA, PLA_INDICATE_FALG, PREBOOT_OPTION);
+
+	ocp_byte_clr_bits(tp, MCU_TYPE_PLA, PLA_RMT_WAKE, RMT_WAKE_EN);
+
 	if (tp->udev->speed >= USB_SPEED_SUPER)
 		r8153b_u1u2en(tp, true);
 
 	usb_enable_lpm(tp->udev);
 
-	if (tp->version >= RTL_VER_12 && tp->version <= RTL_VER_15) {
-		ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR, SLOT_EN);
+	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR, SLOT_EN);
 
-		ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_CPCR, FLOW_CTRL_EN);
+	ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_CPCR, FLOW_CTRL_EN);
 
-		/* enable fc timer and set timer to 600 ms. */
-		ocp_write_word(tp, MCU_TYPE_USB, USB_FC_TIMER, CTRL_TIMER_EN | (600 / 8));
+	/* enable fc timer and set timer to 600 ms. */
+	ocp_write_word(tp, MCU_TYPE_USB, USB_FC_TIMER,
+		       CTRL_TIMER_EN | (600 / 8));
 
-		ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_FW_CTRL);
-		if (!(ocp_read_word(tp, MCU_TYPE_PLA, PLA_POL_GPIO_CTRL) & DACK_DET_EN))
-			ocp_data |= FLOW_CTRL_PATCH_2;
-		ocp_data &= ~AUTO_SPEEDUP;
-		ocp_write_word(tp, MCU_TYPE_USB, USB_FW_CTRL, ocp_data);
+	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_POL_GPIO_CTRL);
+	if (!(ocp_data & DACK_DET_EN))
+		ocp_word_set_bits(tp, MCU_TYPE_USB, USB_FW_CTRL,
+				  FLOW_CTRL_PATCH_2);
 
-		ocp_word_set_bits(tp, MCU_TYPE_USB, USB_FW_TASK, FC_PATCH_TASK);
-	}
+	ocp_word_set_bits(tp, MCU_TYPE_USB, USB_FW_TASK, FC_PATCH_TASK);
 
 	r8156_mac_clk_spd(tp, true);
 
-	if (tp->version < RTL_VER_16)
-		ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3, PLA_MCU_SPDWN_EN);
+	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3,
+			  PLA_MCU_SPDWN_EN);
 
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_EXTRA_STATUS);
 	if (rtl8152_get_speed(tp) & LINK_STATUS)
-		ocp_data |= CUR_LINK_OK;
+		ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_EXTRA_STATUS,
+				  CUR_LINK_OK | POLL_LINK_CHG);
 	else
-		ocp_data &= ~CUR_LINK_OK;
-	ocp_data |= POLL_LINK_CHG;
-	ocp_write_word(tp, MCU_TYPE_PLA, PLA_EXTRA_STATUS, ocp_data);
+		ocp_word_w0w1(tp, MCU_TYPE_PLA, PLA_EXTRA_STATUS,
+			      CUR_LINK_OK, POLL_LINK_CHG);
 
 	set_bit(GREEN_ETHERNET, &tp->flags);
 
-	/* RX aggregation / 16 bytes RX descriptor
-	 * BIT(11) is specific to RTL8159, with unknown meaning
-	 */
-	if (tp->version == RTL_VER_17)
-		ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL,
-				  RX_AGG_DISABLE | RX_DESC_16B | BIT(11));
-	else if (tp->version == RTL_VER_16)
-		ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL, RX_AGG_DISABLE | RX_DESC_16B);
+	ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL,
+			  RX_AGG_DISABLE | RX_ZERO_EN);
+
+	r8156_mdio_force_mode(tp);
+	rtl_tally_reset(tp);
+
+	tp->coalesce = 15000;	/* 15 us */
+}
+
+static void r8157_init(struct r8152 *tp)
+{
+	u16 data;
+
+	if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
+		return;
+
+	/* Enable SW reset */
+	ocp_byte_set_bits(tp, MCU_TYPE_USB, 0xcffe, BIT(3));
+	ocp_byte_clr_bits(tp, MCU_TYPE_USB, 0xd3ca, BIT(0));
+
+	ocp_byte_clr_bits(tp, MCU_TYPE_USB, USB_ECM_OP, EN_ALL_SPEED);
+
+	ocp_word_set_bits(tp, MCU_TYPE_USB, USB_ECM_OPTION, BYPASS_MAC_RESET);
+
+	r8153b_u1u2en(tp, false);
+
+	if (wait_autoload_done(tp))
+		return;
+
+	r8156b_wait_loading_flash(tp);
+
+	data = r8153_phy_status(tp, 0);
+	if (data == PHY_STAT_EXT_INIT) {
+		ocp_reg_clr_bits(tp, 0xa468, BIT(3) | BIT(1));
+		ocp_reg_clr_bits(tp, 0xa466, BIT(0));
+	}
+
+	r8152_mdio_test_and_clr_bit(tp, MII_BMCR, BMCR_PDOWN);
+
+	data = r8153_phy_status(tp, PHY_STAT_LAN_ON);
+
+	r8157_u2p3en(tp, false);
+
+	/* Disable Interrupt Mitigation */
+	ocp_byte_clr_bits(tp, MCU_TYPE_USB, 0xcf04,
+			  BIT(0) | BIT(1) | BIT(2) | BIT(7));
+
+	/* Disable Auto Speed up */
+	ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_FW_CTRL, AUTO_SPEEDUP);
+
+	/* MSC timer = 0xfff * 8ms = 32760 ms */
+	ocp_write_word(tp, MCU_TYPE_USB, USB_MSC_TIMER, 0x0fff);
+
+	/* U1/U2/L1 idle timer. 500 us */
+	ocp_write_word(tp, MCU_TYPE_USB, USB_U1U2_TIMER, 500);
+
+	r8157_power_cut_en(tp, false);
+	r8156_ups_en(tp, false);
+	r8153_queue_wake(tp, false);
+	rtl_runtime_suspend_enable(tp, false);
+
+	ocp_byte_clr_bits(tp, MCU_TYPE_PLA, PLA_INDICATE_FALG, PREBOOT_OPTION);
+
+	ocp_byte_clr_bits(tp, MCU_TYPE_PLA, PLA_RMT_WAKE, RMT_WAKE_EN);
+
+	/* Clear Warm RST / Bus RST event flag */
+	ocp_word_clr_bits(tp, MCU_TYPE_USB, 0xcd06, BIT(11));
+
+	if (tp->udev->speed >= USB_SPEED_SUPER)
+		r8153b_u1u2en(tp, true);
+
+	usb_enable_lpm(tp->udev);
+
+	r8156_mac_clk_spd(tp, true);
+
+	if (rtl8152_get_speed(tp) & LINK_STATUS)
+		ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_EXTRA_STATUS,
+				  CUR_LINK_OK | POLL_LINK_CHG);
 	else
-		ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL, RX_AGG_DISABLE | RX_ZERO_EN);
+		ocp_word_w0w1(tp, MCU_TYPE_PLA, PLA_EXTRA_STATUS,
+			      CUR_LINK_OK, POLL_LINK_CHG);
 
-	if (tp->version < RTL_VER_12)
-		ocp_byte_set_bits(tp, MCU_TYPE_USB, USB_BMU_CONFIG, ACT_ODMA);
+	set_bit(GREEN_ETHERNET, &tp->flags);
 
-	if (tp->version >= RTL_VER_16) {
-		/* Disable Rx Zero Len */
-		rtl_bmu_clr_bits(tp, 0x2300, BIT(3));
-		/* TX descriptor Signature */
-		ocp_byte_clr_bits(tp, MCU_TYPE_USB, 0xd4ae, BIT(1));
+	/* RX aggregation / 16 bytes RX descriptor / Bulk In End transfer */
+	ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL,
+			  RX_AGG_DISABLE | RX_DESC_16B | RX_END_TRANSFER_EN);
+
+	/* Disable Rx Zero Len */
+	rtl_bmu_clr_bits(tp, 0x2300, BIT(3));
+
+	/* TX descriptor Signature */
+	ocp_byte_clr_bits(tp, MCU_TYPE_USB, 0xd4ae, BIT(1));
+
+	r8156_mdio_force_mode(tp);
+	rtl_tally_reset(tp);
+
+	tp->coalesce = 15000;	/* 15 us */
+}
+
+static int r8159_wait_backup_restore(struct r8152 *tp)
+{
+	u32 ocp_data;
+
+	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0);
+	if (!(ocp_data & PCUT_STATUS))
+		return 0;
+
+	return poll_timeout_us(ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_GPHY_CTRL),
+			       ocp_data & BACKUP_RESTRORE, 200, 20000, false);
+}
+
+static void r8159_init(struct r8152 *tp)
+{
+	u16 data;
+
+	if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
+		return;
+
+	/* Enable SW reset */
+	ocp_byte_set_bits(tp, MCU_TYPE_USB, 0xcffe, BIT(3));
+	ocp_byte_clr_bits(tp, MCU_TYPE_USB, 0xd3ca, BIT(0));
+
+	ocp_byte_clr_bits(tp, MCU_TYPE_USB, USB_ECM_OP, EN_ALL_SPEED);
+
+	ocp_word_set_bits(tp, MCU_TYPE_USB, USB_ECM_OPTION, BYPASS_MAC_RESET);
+
+	r8153b_u1u2en(tp, false);
+
+	if (wait_autoload_done(tp))
+		return;
+
+	if (r8159_wait_backup_restore(tp)) {
+		rtl_set_inaccessible(tp);
+		dev_err(&tp->intf->dev,
+			"init failed, backup-restore timed out\n");
+		return;
 	}
 
+	r8156b_wait_loading_flash(tp);
+
+	data = r8153_phy_status(tp, 0);
+	if (data == PHY_STAT_EXT_INIT) {
+		ocp_reg_clr_bits(tp, 0xa468, BIT(3) | BIT(1));
+		ocp_reg_clr_bits(tp, 0xa466, BIT(0));
+	}
+
+	r8152_mdio_test_and_clr_bit(tp, MII_BMCR, BMCR_PDOWN);
+
+	data = r8153_phy_status(tp, PHY_STAT_LAN_ON);
+
+	r8157_u2p3en(tp, false);
+
+	/* Disable Interrupt Mitigation */
+	ocp_byte_clr_bits(tp, MCU_TYPE_USB, 0xcf04,
+			  BIT(0) | BIT(1) | BIT(2) | BIT(7));
+
+	/* Disable Auto Speed up */
+	ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_FW_CTRL, AUTO_SPEEDUP);
+
+	/* MSC timer = 0xfff * 8ms = 32760 ms */
+	ocp_write_word(tp, MCU_TYPE_USB, USB_MSC_TIMER, 0x0fff);
+
+	/* U1/U2/L1 idle timer. 500 us */
+	ocp_write_word(tp, MCU_TYPE_USB, USB_U1U2_TIMER, 500);
+
+	r8157_power_cut_en(tp, false);
+	r8156_ups_en(tp, false);
+	r8153_queue_wake(tp, false);
+	rtl_runtime_suspend_enable(tp, false);
+
+	ocp_byte_clr_bits(tp, MCU_TYPE_PLA, PLA_INDICATE_FALG, PREBOOT_OPTION);
+
+	ocp_byte_clr_bits(tp, MCU_TYPE_PLA, PLA_RMT_WAKE, RMT_WAKE_EN);
+
+	/* Clear Warm RST / Bus RST event flag */
+	ocp_word_clr_bits(tp, MCU_TYPE_USB, 0xcd06, BIT(11));
+
+	/* Disable FW u1u2 patch option */
+	ocp_byte_clr_bits(tp, MCU_TYPE_USB, 0xb9a6, BIT(0));
+
+	if (tp->udev->speed >= USB_SPEED_SUPER)
+		r8153b_u1u2en(tp, true);
+
+	usb_enable_lpm(tp->udev);
+
+	r8156_mac_clk_spd(tp, true);
+
+	if (rtl8152_get_speed(tp) & LINK_STATUS)
+		ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_EXTRA_STATUS,
+				  CUR_LINK_OK | POLL_LINK_CHG);
+	else
+		ocp_word_w0w1(tp, MCU_TYPE_PLA, PLA_EXTRA_STATUS,
+			      CUR_LINK_OK, POLL_LINK_CHG);
+
+	set_bit(GREEN_ETHERNET, &tp->flags);
+
+	/* RX aggregation / 16 bytes RX descriptor / Bulk In End transfer */
+	ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL,
+			  RX_AGG_DISABLE | RX_DESC_16B | RX_END_TRANSFER_EN);
+
+	/* Disable Rx Zero Len */
+	rtl_bmu_clr_bits(tp, 0x2300, BIT(3));
+
+	/* TX descriptor Signature */
+	ocp_byte_clr_bits(tp, MCU_TYPE_USB, 0xd4ae, BIT(1));
+
+	/* Enable u3phy patch backup */
+	ocp_write_word(tp, MCU_TYPE_USB, 0xb9a2, 0x0448);
+
 	r8156_mdio_force_mode(tp);
 	rtl_tally_reset(tp);
 
@@ -9812,7 +10023,7 @@ static int rtl_ops_init(struct r8152 *tp)
 		tp->eee_en		= true;
 		tp->eee_adv		= MDIO_EEE_1000T | MDIO_EEE_100TX;
 		tp->eee_adv2		= MDIO_EEE_2_5GT;
-		ops->init		= r8156_init;
+		ops->init		= r8156b_init;
 		ops->enable		= rtl8156_enable;
 		ops->disable		= rtl8153_disable;
 		ops->up			= rtl8156_up;
@@ -9851,7 +10062,7 @@ static int rtl_ops_init(struct r8152 *tp)
 		tp->eee_en		= true;
 		tp->eee_adv		= MDIO_EEE_1000T | MDIO_EEE_100TX;
 		tp->eee_adv2		= MDIO_EEE_2_5GT | MDIO_EEE_5GT;
-		ops->init		= r8156_init;
+		ops->init		= r8157_init;
 		ops->enable		= rtl8156_enable;
 		ops->disable		= rtl8153_disable;
 		ops->up			= rtl8156_up;
@@ -9873,7 +10084,7 @@ static int rtl_ops_init(struct r8152 *tp)
 		tp->eee_en		= true;
 		tp->eee_adv		= MDIO_EEE_100TX | MDIO_EEE_1000T | MDIO_EEE_10GT;
 		tp->eee_adv2		= MDIO_EEE_2_5GT | MDIO_EEE_5GT;
-		ops->init		= r8156_init;
+		ops->init		= r8159_init;
 		ops->enable		= rtl8156_enable;
 		ops->disable		= rtl8153_disable;
 		ops->up			= rtl8156_up;
-- 
2.34.1

[PATCH net-next v4 2/8] r8152: split RTL_VER_17 into QFN68 and QFN100 package variants

[Chih Kai Hsu]

RTL8159 is sold in two packages, QFN68 and QFN100, which need
different PHY tuning (added in a later patch) and a package-specific
u2phy backup-restore patch. There was previously no way to tell the
two apart at runtime, since both used the same RTL_VER_17.

Split RTL_VER_17 into RTL_VER_17_QFN68 and RTL_VER_17_QFN100 and
update every switch/case on RTL_VER_17 in the driver to handle both.
Detect the package by reading the new PLA_PKG_DET register
(0xdc48, masked with PKG_MASK = 0x1e) via a new __rtl_get_pkg_det()
helper, called from __rtl_get_hw_ver() when the chip ID is 0x2020:
pkg_det value 0x1e/0x1c selects QFN68, 0x18/0x1a selects QFN100, and
any other value logs "Unknown package" and falls back to
RTL_VER_UNKNOWN.

r8159_init() gains a QFN68-only write ("Enable u2phy backup restore
patch", 0xb99c BIT(0)) that does not apply to QFN100.

Signed-off-by: Chih Kai Hsu <hsu.chih.kai@realtek.com>
---
 drivers/net/usb/r8152.c | 93 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 78 insertions(+), 15 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 00b0fda306c30..9ce072c38b5b5 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -64,6 +64,7 @@
 #define PLA_MACDBG_POST		0xd38e	/* RTL_VER_04 only */
 #define PLA_EXTRA_STATUS	0xd398
 #define PLA_GPHY_CTRL		0xd3ae
+#define PLA_PKG_DET		0xdc48
 #define PLA_POL_GPIO_CTRL	0xdc6a
 #define PLA_EFUSE_DATA		0xdd00
 #define PLA_EFUSE_CMD		0xdd02
@@ -290,6 +291,9 @@
 #define IFG_144NS		BIT(9)
 #define IFG_96NS		(BIT(9) | BIT(8))
 
+/* PLA_PKG_DET */
+#define PKG_MASK		0x1e
+
 /* PLA_MTPS */
 #define MTPS_JUMBO		(12 * 1024 / 64)
 #define MTPS_DEFAULT		(6 * 1024 / 64)
@@ -1253,7 +1257,8 @@ enum rtl_version {
 	RTL_VER_14,
 	RTL_VER_15,
 	RTL_VER_16,
-	RTL_VER_17,
+	RTL_VER_17_QFN68,
+	RTL_VER_17_QFN100,
 
 	RTL_VER_MAX
 };
@@ -3418,7 +3423,8 @@ static void rtl8152_nic_reset(struct r8152 *tp)
 		break;
 
 	case RTL_VER_16:
-	case RTL_VER_17:
+	case RTL_VER_17_QFN68:
+	case RTL_VER_17_QFN100:
 		ocp_byte_clr_bits(tp, MCU_TYPE_PLA, PLA_CR, CR_RE | CR_TE);
 		break;
 
@@ -3458,7 +3464,7 @@ static void rtl_eee_plus_en(struct r8152 *tp, bool enable)
 
 static void rtl_set_eee_plus(struct r8152 *tp)
 {
-	if (tp->version == RTL_VER_17)
+	if (tp->version == RTL_VER_17_QFN68 || tp->version == RTL_VER_17_QFN100)
 		return rtl_eee_plus_en(tp, false);
 
 	if (rtl8152_get_speed(tp) & _10bps)
@@ -3646,7 +3652,8 @@ static void r8153_set_rx_early_timeout(struct r8152 *tp)
 	case RTL_VER_13:
 	case RTL_VER_15:
 	case RTL_VER_16:
-	case RTL_VER_17:
+	case RTL_VER_17_QFN68:
+	case RTL_VER_17_QFN100:
 		ocp_write_word(tp, MCU_TYPE_USB, USB_RX_EARLY_TIMEOUT,
 			       640 / 8);
 		ocp_write_word(tp, MCU_TYPE_USB, USB_RX_EXTRA_AGGR_TMR,
@@ -3691,7 +3698,8 @@ static void r8153_set_rx_early_size(struct r8152 *tp)
 			       ocp_data / 8);
 		break;
 	case RTL_VER_16:
-	case RTL_VER_17:
+	case RTL_VER_17_QFN68:
+	case RTL_VER_17_QFN100:
 		ocp_write_word(tp, MCU_TYPE_USB, USB_RX_EARLY_SIZE,
 			       ocp_data / 16);
 		break;
@@ -3807,6 +3815,8 @@ static void rtl_rx_vlan_en(struct r8152 *tp, bool enable)
 	case RTL_VER_13:
 	case RTL_VER_15:
 	case RTL_VER_16:
+	case RTL_VER_17_QFN68:
+	case RTL_VER_17_QFN100:
 	default:
 		if (enable)
 			ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_RCR1,
@@ -4487,6 +4497,8 @@ static void r8153_teredo_off(struct r8152 *tp)
 	case RTL_VER_14:
 	case RTL_VER_15:
 	case RTL_VER_16:
+	case RTL_VER_17_QFN68:
+	case RTL_VER_17_QFN100:
 	default:
 		/* The bit 0 ~ 7 are relative with teredo settings. They are
 		 * W1C (write 1 to clear), so set all 1 to disable it.
@@ -4541,7 +4553,8 @@ static void rtl_clear_bp(struct r8152 *tp, u16 type)
 		break;
 	case RTL_VER_14:
 	case RTL_VER_16:
-	case RTL_VER_17:
+	case RTL_VER_17_QFN68:
+	case RTL_VER_17_QFN100:
 	default:
 		ocp_write_word(tp, type, USB_BP2_EN, 0);
 		bp_num = 16;
@@ -4653,7 +4666,8 @@ static bool rtl8152_is_fw_phy_speed_up_ok(struct r8152 *tp, struct fw_phy_speed_
 	case RTL_VER_13:
 	case RTL_VER_15:
 	case RTL_VER_16:
-	case RTL_VER_17:
+	case RTL_VER_17_QFN68:
+	case RTL_VER_17_QFN100:
 	default:
 		break;
 	}
@@ -5813,7 +5827,8 @@ static void rtl_eee_enable(struct r8152 *tp, bool enable)
 	case RTL_VER_13:
 	case RTL_VER_15:
 	case RTL_VER_16:
-	case RTL_VER_17:
+	case RTL_VER_17_QFN68:
+	case RTL_VER_17_QFN100:
 		if (enable) {
 			r8156_eee_en(tp, true);
 			ocp_reg_write(tp, OCP_EEE_ADV, tp->eee_adv);
@@ -6404,8 +6419,15 @@ static int rtl8156_enable(struct r8152 *tp)
 	set_tx_qlen(tp);
 	rtl_set_eee_plus(tp);
 
-	if (tp->version >= RTL_VER_12 && tp->version <= RTL_VER_17)
-		ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_RX_AGGR_NUM, RX_AGGR_NUM_MASK);
+	switch (tp->version) {
+	case RTL_VER_10:
+	case RTL_VER_11:
+		break;
+	default:
+		ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_RX_AGGR_NUM,
+				  RX_AGGR_NUM_MASK);
+		break;
+	}
 
 	r8153_set_rx_early_timeout(tp);
 	r8153_set_rx_early_size(tp);
@@ -8093,7 +8115,8 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)
 		sram2_write_w0w1(tp, 0x809d, 0xff00, 0x5000);
 		break;
 
-	case RTL_VER_17:
+	case RTL_VER_17_QFN68:
+	case RTL_VER_17_QFN100:
 		/* Disable bypass turn off clk in ALDPS */
 		ocp_byte_clr_bits(tp, MCU_TYPE_PLA, 0xd3c8, BIT(0));
 
@@ -8696,6 +8719,10 @@ static void r8159_init(struct r8152 *tp)
 	/* TX descriptor Signature */
 	ocp_byte_clr_bits(tp, MCU_TYPE_USB, 0xd4ae, BIT(1));
 
+	/* Enable u2phy backup restore patch */
+	if (tp->version == RTL_VER_17_QFN68)
+		ocp_byte_set_bits(tp, MCU_TYPE_USB, 0xb99c, BIT(0));
+
 	/* Enable u3phy patch backup */
 	ocp_write_word(tp, MCU_TYPE_USB, 0xb9a2, 0x0448);
 
@@ -10080,7 +10107,8 @@ static int rtl_ops_init(struct r8152 *tp)
 		r8157_desc_init(tp);
 		break;
 
-	case RTL_VER_17:
+	case RTL_VER_17_QFN68:
+	case RTL_VER_17_QFN100:
 		tp->eee_en		= true;
 		tp->eee_adv		= MDIO_EEE_100TX | MDIO_EEE_1000T | MDIO_EEE_10GT;
 		tp->eee_adv2		= MDIO_EEE_2_5GT | MDIO_EEE_5GT;
@@ -10172,7 +10200,8 @@ static int rtl_fw_init(struct r8152 *tp)
 	case RTL_VER_16:
 		rtl_fw->fw_name		= FIRMWARE_8157_1;
 		break;
-	case RTL_VER_17:
+	case RTL_VER_17_QFN68:
+	case RTL_VER_17_QFN100:
 		rtl_fw->fw_name		= FIRMWARE_8159_1;
 		break;
 	default:
@@ -10182,9 +10211,33 @@ static int rtl_fw_init(struct r8152 *tp)
 	return 0;
 }
 
+static u32 __rtl_get_pkg_det(struct usb_device *udev)
+{
+	u32 pkg_det = 0;
+	__le32 *tmp;
+	int ret, i;
+
+	tmp = kmalloc_obj(*tmp);
+	if (!tmp)
+		return 0;
+
+	for (i = 0, ret = 0; i < 3 && ret != 4; i++)
+		ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
+				      RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
+				      PLA_PKG_DET, MCU_TYPE_PLA, tmp,
+				      sizeof(*tmp), USB_CTRL_GET_TIMEOUT);
+
+	if (ret > 0)
+		pkg_det = __le32_to_cpu(*tmp) & PKG_MASK;
+
+	kfree(tmp);
+	return pkg_det;
+}
+
 static u8 __rtl_get_hw_ver(struct usb_device *udev)
 {
 	u32 ocp_data = 0;
+	u32 pkg_det = 0;
 	__le32 *tmp;
 	u8 version;
 	int ret;
@@ -10267,7 +10320,16 @@ static u8 __rtl_get_hw_ver(struct usb_device *udev)
 		version = RTL_VER_16;
 		break;
 	case 0x2020:
-		version = RTL_VER_17;
+		pkg_det = __rtl_get_pkg_det(udev);
+		if (pkg_det == 0x1e || pkg_det == 0x1c) {
+			version = RTL_VER_17_QFN68;
+		} else if (pkg_det == 0x18 || pkg_det == 0x1a) {
+			version = RTL_VER_17_QFN100;
+		} else {
+			version = RTL_VER_UNKNOWN;
+			dev_info(&udev->dev, "Unknown package %#02x\n",
+				 pkg_det);
+		}
 		break;
 	default:
 		version = RTL_VER_UNKNOWN;
@@ -10426,7 +10488,8 @@ static int rtl8152_probe_once(struct usb_interface *intf,
 	case RTL_VER_13:
 	case RTL_VER_15:
 	case RTL_VER_16:
-	case RTL_VER_17:
+	case RTL_VER_17_QFN68:
+	case RTL_VER_17_QFN100:
 		netdev->max_mtu = size_to_mtu(16 * 1024);
 		break;
 	case RTL_VER_01:
-- 
2.34.1

[PATCH net-next v4 3/8] r8152: split rtl8156_enable/up/down into per-chip-family functions

[Chih Kai Hsu]

rtl8156_enable(), rtl8156_up() and rtl8156_down() had accumulated
tp->version range checks to cover RTL8156, RTL8156B, and
RTL8157/RTL8159, including an early bail-out in rtl8156_enable()
that skipped most of the function body for RTL8157/RTL8159.

Add rtl8156b_enable() (VER_12/13/15) and rtl8157_enable()/
rtl8157_up()/rtl8157_down() (VER_16/17) as separate ops callbacks.
rtl8156_up()/rtl8156_down() keep serving RTL8156 and RTL8156B
(VER_10/11/12/13/15).

Per the datasheet, this uncovers real register differences that the
old version guards had wrong or missing:

- RTL8156/8156B now also clear PLA_RCR1 BIT(3) on up, previously
  applied only to RTL8157/8159's share of the old function.
- RTL8156B (VER_13/15) gains a Clear_SDR enable (USB 0xd3ca BIT(15))
  on up.
- RTL8157/8159 now also clear USB_SPEED_OPTION's
  RG_PWRDN_EN/ALL_SPEED_OFF on up, previously excluded for them, and
  gain their own Clear_SDR enables (USB 0xd378 BIT(7), USB 0xcd06
  BIT(15)).
- RTL8156/8156B/8157/8159 OOB entry (down) gains PLA_BDC_CR's
  ALDPS_PROXY_MODE and PLA_OOB_CTRL's DIS_MCU_CLROOB, aligning with
  r8152b_enter_oob()/r8153_enter_oob().
- RTL8157/8159 OOB entry now uses PLA_RMS=1526/PLA_MTPS=10K instead
  of 1522/6K.

While extracting rtl8156b_enable(), also drop the
"if (tp->version < RTL_VER_12) r8156_fc_parameter(tp)" guard: since
this function only runs for VER_12/13/15 chips, the call was always
skipped and is removed rather than carried over.

Signed-off-by: Chih Kai Hsu <hsu.chih.kai@realtek.com>
---
 drivers/net/usb/r8152.c | 235 ++++++++++++++++++++++++++++++++--------
 1 file changed, 189 insertions(+), 46 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 9ce072c38b5b5..c513c286c5499 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -6413,31 +6413,16 @@ static int rtl8156_enable(struct r8152 *tp)
 	if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
 		return -ENODEV;
 
-	if (tp->version < RTL_VER_12)
-		r8156_fc_parameter(tp);
-
+	r8156_fc_parameter(tp);
 	set_tx_qlen(tp);
 	rtl_set_eee_plus(tp);
 
-	switch (tp->version) {
-	case RTL_VER_10:
-	case RTL_VER_11:
-		break;
-	default:
-		ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_RX_AGGR_NUM,
-				  RX_AGGR_NUM_MASK);
-		break;
-	}
-
 	r8153_set_rx_early_timeout(tp);
 	r8153_set_rx_early_size(tp);
 
 	speed = rtl8152_get_speed(tp);
 	rtl_set_ifg(tp, speed);
 
-	if (tp->version >= RTL_VER_16)
-		return rtl_enable(tp);
-
 	if (speed & _2500bps)
 		ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL4,
 				  IDLE_SPDWN_EN);
@@ -6445,12 +6430,10 @@ static int rtl8156_enable(struct r8152 *tp)
 		ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL4,
 				  IDLE_SPDWN_EN);
 
-	if (tp->version < RTL_VER_12) {
-		if (speed & _1000bps)
-			ocp_write_word(tp, MCU_TYPE_PLA, PLA_EEE_TXTWSYS, 0x11);
-		else if (speed & _500bps)
-			ocp_write_word(tp, MCU_TYPE_PLA, PLA_EEE_TXTWSYS, 0x3d);
-	}
+	if (speed & _1000bps)
+		ocp_write_word(tp, MCU_TYPE_PLA, PLA_EEE_TXTWSYS, 0x11);
+	else if (speed & _500bps)
+		ocp_write_word(tp, MCU_TYPE_PLA, PLA_EEE_TXTWSYS, 0x3d);
 
 	if (tp->udev->speed == USB_SPEED_HIGH) {
 		/* USB 0xb45e[3:0] l1_nyet_hird */
@@ -6475,6 +6458,67 @@ static void rtl8156_disable(struct r8152 *tp)
 	rtl8153_disable(tp);
 }
 
+static int rtl8156b_enable(struct r8152 *tp)
+{
+	u16 speed;
+
+	if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
+		return -ENODEV;
+
+	set_tx_qlen(tp);
+	rtl_set_eee_plus(tp);
+
+	ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_RX_AGGR_NUM, RX_AGGR_NUM_MASK);
+
+	r8153_set_rx_early_timeout(tp);
+	r8153_set_rx_early_size(tp);
+
+	speed = rtl8152_get_speed(tp);
+	rtl_set_ifg(tp, speed);
+
+	if (speed & _2500bps)
+		ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL4,
+				  IDLE_SPDWN_EN);
+	else
+		ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL4,
+				  IDLE_SPDWN_EN);
+
+	if (tp->udev->speed == USB_SPEED_HIGH) {
+		/* USB 0xb45e[3:0] l1_nyet_hird */
+		if (is_flow_control(speed))
+			ocp_word_w0w1(tp, MCU_TYPE_USB, USB_L1_CTRL, 0xf, 0xf);
+		else
+			ocp_word_w0w1(tp, MCU_TYPE_USB, USB_L1_CTRL, 0xf, 0x1);
+	}
+
+	ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_FW_TASK, FC_PATCH_TASK);
+	usleep_range(1000, 2000);
+	ocp_word_set_bits(tp, MCU_TYPE_USB, USB_FW_TASK, FC_PATCH_TASK);
+
+	return rtl_enable(tp);
+}
+
+static int rtl8157_enable(struct r8152 *tp)
+{
+	u16 speed;
+
+	if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
+		return -ENODEV;
+
+	set_tx_qlen(tp);
+	rtl_set_eee_plus(tp);
+
+	ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_RX_AGGR_NUM, RX_AGGR_NUM_MASK);
+
+	r8153_set_rx_early_timeout(tp);
+	r8153_set_rx_early_size(tp);
+
+	speed = rtl8152_get_speed(tp);
+	rtl_set_ifg(tp, speed);
+
+	return rtl_enable(tp);
+}
+
 static int rtl8152_set_speed(struct r8152 *tp, u8 autoneg, u32 speed, u8 duplex,
 			     u32 advertising)
 {
@@ -6835,8 +6879,7 @@ static void rtl8156_up(struct r8152 *tp)
 		return;
 
 	r8153b_u1u2en(tp, false);
-	if (tp->version < RTL_VER_16)
-		r8153_u2p3en(tp, false);
+	r8153_u2p3en(tp, false);
 	r8153_aldps_en(tp, false);
 
 	rxdy_gated_en(tp, true);
@@ -6849,8 +6892,7 @@ static void rtl8156_up(struct r8152 *tp)
 
 	ocp_byte_clr_bits(tp, MCU_TYPE_PLA, PLA_OOB_CTRL, NOW_IS_OOB);
 
-	if (tp->version >= RTL_VER_16)
-		ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR1, BIT(3));
+	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR1, BIT(3));
 
 	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_SFF_STS_7, MCU_BORW_EN);
 
@@ -6872,11 +6914,11 @@ static void rtl8156_up(struct r8152 *tp)
 	ocp_word_w0w1(tp, MCU_TYPE_PLA, PLA_RXFIFO_FULL, RXFIFO_FULL_MASK,
 		      0x08);
 
-	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3, PLA_MCU_SPDWN_EN);
+	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3,
+			  PLA_MCU_SPDWN_EN);
 
-	if (tp->version < RTL_VER_16)
-		ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_SPEED_OPTION,
-				  RG_PWRDN_EN | ALL_SPEED_OFF);
+	ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_SPEED_OPTION,
+			  RG_PWRDN_EN | ALL_SPEED_OFF);
 
 	ocp_write_dword(tp, MCU_TYPE_USB, USB_RX_BUF_TH, 0x00600400);
 
@@ -6886,10 +6928,19 @@ static void rtl8156_up(struct r8152 *tp)
 	}
 
 	r8153_aldps_en(tp, true);
-	if (tp->version < RTL_VER_16)
-		r8153_u2p3en(tp, true);
+	r8153_u2p3en(tp, true);
 
-	if (tp->version < RTL_VER_16 && tp->udev->speed >= USB_SPEED_SUPER)
+	switch (tp->version) {
+	case RTL_VER_13:
+	case RTL_VER_15:
+		/* Enable Clear_SDR */
+		ocp_word_set_bits(tp, MCU_TYPE_USB, 0xd3ca, BIT(15));
+		break;
+	default:
+		break;
+	}
+
+	if (tp->udev->speed >= USB_SPEED_SUPER)
 		r8153b_u1u2en(tp, true);
 }
 
@@ -6902,12 +6953,9 @@ static void rtl8156_down(struct r8152 *tp)
 
 	ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3,
 			  PLA_MCU_SPDWN_EN);
-
 	r8153b_u1u2en(tp, false);
-	if (tp->version < RTL_VER_16) {
-		r8153_u2p3en(tp, false);
-		r8153b_power_cut_en(tp, false);
-	}
+	r8153_u2p3en(tp, false);
+	r8153b_power_cut_en(tp, false);
 	r8153_aldps_en(tp, false);
 
 	ocp_byte_clr_bits(tp, MCU_TYPE_PLA, PLA_OOB_CTRL, NOW_IS_OOB);
@@ -6929,7 +6977,102 @@ static void rtl8156_down(struct r8152 *tp)
 	 */
 	ocp_write_word(tp, MCU_TYPE_PLA, PLA_TEREDO_WAKE_BASE, 0x00ff);
 
-	ocp_byte_set_bits(tp, MCU_TYPE_PLA, PLA_OOB_CTRL, NOW_IS_OOB);
+	ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_BDC_CR, ALDPS_PROXY_MODE);
+
+	ocp_byte_set_bits(tp, MCU_TYPE_PLA, PLA_OOB_CTRL,
+			  NOW_IS_OOB | DIS_MCU_CLROOB);
+
+	ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_SFF_STS_7, MCU_BORW_EN);
+
+	rtl_rx_vlan_en(tp, true);
+	rxdy_gated_en(tp, false);
+
+	ocp_dword_set_bits(tp, MCU_TYPE_PLA, PLA_RCR,
+			   RCR_APM | RCR_AM | RCR_AB);
+
+	r8153_aldps_en(tp, true);
+}
+
+static void rtl8157_up(struct r8152 *tp)
+{
+	if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
+		return;
+
+	r8153b_u1u2en(tp, false);
+	r8153_aldps_en(tp, false);
+
+	rxdy_gated_en(tp, true);
+	r8153_teredo_off(tp);
+
+	ocp_dword_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR, RCR_ACPT_ALL);
+
+	rtl8152_nic_reset(tp);
+	rtl_reset_bmu(tp);
+
+	ocp_byte_clr_bits(tp, MCU_TYPE_PLA, PLA_OOB_CTRL, NOW_IS_OOB);
+
+	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR1, BIT(3));
+
+	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_SFF_STS_7, MCU_BORW_EN);
+
+	rtl_rx_vlan_en(tp, tp->netdev->features & NETIF_F_HW_VLAN_CTAG_RX);
+
+	rtl8156_change_mtu(tp);
+
+	/* share FIFO settings */
+	ocp_word_w0w1(tp, MCU_TYPE_PLA, PLA_RXFIFO_FULL, RXFIFO_FULL_MASK,
+		      0x08);
+
+	ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_SPEED_OPTION,
+			  RG_PWRDN_EN | ALL_SPEED_OFF);
+
+	ocp_write_dword(tp, MCU_TYPE_USB, USB_RX_BUF_TH, 0x00600400);
+
+	if (tp->saved_wolopts != __rtl_get_wol(tp)) {
+		netif_warn(tp, ifup, tp->netdev, "wol setting is changed\n");
+		__rtl_set_wol(tp, tp->saved_wolopts);
+	}
+
+	r8153_aldps_en(tp, true);
+
+	/* Clear_SDR */
+	ocp_byte_set_bits(tp, MCU_TYPE_USB, 0xd378, BIT(7));
+	ocp_word_clr_bits(tp, MCU_TYPE_USB, 0xcd06, BIT(15));
+}
+
+static void rtl8157_down(struct r8152 *tp)
+{
+	if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) {
+		rtl_drop_queued_tx(tp);
+		return;
+	}
+
+	r8153b_u1u2en(tp, false);
+	r8153_aldps_en(tp, false);
+
+	ocp_byte_clr_bits(tp, MCU_TYPE_PLA, PLA_OOB_CTRL, NOW_IS_OOB);
+
+	/* RX FIFO settings for OOB */
+	ocp_write_word(tp, MCU_TYPE_PLA, PLA_RXFIFO_FULL, 64 / 16);
+	ocp_write_word(tp, MCU_TYPE_PLA, PLA_RX_FIFO_FULL, 1024 / 16);
+	ocp_write_word(tp, MCU_TYPE_PLA, PLA_RX_FIFO_EMPTY, 4096 / 16);
+
+	rtl_disable(tp);
+	rtl_reset_bmu(tp);
+
+	ocp_write_word(tp, MCU_TYPE_PLA, PLA_RMS, 1526);
+	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_MTPS, 10 * 1024 / 64);
+
+	/* Clear teredo wake event. bit[15:8] is the teredo wakeup
+	 * type. Set it to zero. bits[7:0] are the W1C bits about
+	 * the events. Set them to all 1 to clear them.
+	 */
+	ocp_write_word(tp, MCU_TYPE_PLA, PLA_TEREDO_WAKE_BASE, 0x00ff);
+
+	ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_BDC_CR, ALDPS_PROXY_MODE);
+
+	ocp_byte_set_bits(tp, MCU_TYPE_PLA, PLA_OOB_CTRL,
+			  NOW_IS_OOB | DIS_MCU_CLROOB);
 
 	ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_SFF_STS_7, MCU_BORW_EN);
 
@@ -10051,7 +10194,7 @@ static int rtl_ops_init(struct r8152 *tp)
 		tp->eee_adv		= MDIO_EEE_1000T | MDIO_EEE_100TX;
 		tp->eee_adv2		= MDIO_EEE_2_5GT;
 		ops->init		= r8156b_init;
-		ops->enable		= rtl8156_enable;
+		ops->enable		= rtl8156b_enable;
 		ops->disable		= rtl8153_disable;
 		ops->up			= rtl8156_up;
 		ops->down		= rtl8156_down;
@@ -10090,10 +10233,10 @@ static int rtl_ops_init(struct r8152 *tp)
 		tp->eee_adv		= MDIO_EEE_1000T | MDIO_EEE_100TX;
 		tp->eee_adv2		= MDIO_EEE_2_5GT | MDIO_EEE_5GT;
 		ops->init		= r8157_init;
-		ops->enable		= rtl8156_enable;
+		ops->enable		= rtl8157_enable;
 		ops->disable		= rtl8153_disable;
-		ops->up			= rtl8156_up;
-		ops->down		= rtl8156_down;
+		ops->up			= rtl8157_up;
+		ops->down		= rtl8157_down;
 		ops->unload		= rtl8153_unload;
 		ops->eee_get		= r8153_get_eee;
 		ops->eee_set		= r8152_set_eee;
@@ -10113,10 +10256,10 @@ static int rtl_ops_init(struct r8152 *tp)
 		tp->eee_adv		= MDIO_EEE_100TX | MDIO_EEE_1000T | MDIO_EEE_10GT;
 		tp->eee_adv2		= MDIO_EEE_2_5GT | MDIO_EEE_5GT;
 		ops->init		= r8159_init;
-		ops->enable		= rtl8156_enable;
+		ops->enable		= rtl8157_enable;
 		ops->disable		= rtl8153_disable;
-		ops->up			= rtl8156_up;
-		ops->down		= rtl8156_down;
+		ops->up			= rtl8157_up;
+		ops->down		= rtl8157_down;
 		ops->unload		= rtl8153_unload;
 		ops->eee_get		= r8153_get_eee;
 		ops->eee_set		= r8152_set_eee;
-- 
2.34.1

[PATCH net-next v4 4/8] r8152: split r8157_hw_phy_cfg into RTL8157 and RTL8159 variants

[Chih Kai Hsu]

r8157_hw_phy_cfg() served both RTL8157 (VER_16) and RTL8159
(VER_17_QFN68/QFN100) via a tp->version switch inside a single
function, but per the datasheet RTL8159's PHY tuning differs from
RTL8157's in more than the per-version blocks already captured.

Split it into r8157_hw_phy_cfg() (VER_16 only) and a new
r8159_hw_phy_cfg() (VER_17_QFN68/QFN100), each with its own copy of
the shared prologue/epilogue instead of switching on version
mid-function. Per the datasheet, RTL8159 diverges from RTL8157 in
this split in several ways:

- RTL8159 doesn't need EEE disabled before reprogramming PHY
  parameters, so r8159_hw_phy_cfg() drops the rtl_eee_enable(tp,
  false) call that r8157_hw_phy_cfg() keeps.
- RTL8159's EEE clock-divider and 0xa5ea encoding differ from
  RTL8157's: r8159_hw_phy_cfg() clears EN_10M_CLKDIV (ups_info.
  _10m_ckdiv = false) instead of setting it, and clears an extra bit
  (BIT(2)) of 0xa5ea.
- "Disable bypass_turn_off_clk_in_aldps" (0xd3c8 BIT(0)) is needed by
  both chips, so it moves from the VER_17-only switch case into the
  shared prologue and now also runs for RTL8157.

Along the way, add sram2_set_bits()/sram2_clr_bits() helpers
mirroring the existing sram_set_bits()/sram_clr_bits(), and use the
named OCP_POWER_CFG/EN_10M_PLLOFF/EN_ALDPS_PLLOFF macros in place of
the raw "ocp_reg_set_bits(tp, 0xa430, BIT(0) | BIT(1))".

Signed-off-by: Chih Kai Hsu <hsu.chih.kai@realtek.com>
---
 drivers/net/usb/r8152.c | 165 +++++++++++++++++++++++++++++++++-------
 1 file changed, 136 insertions(+), 29 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index c513c286c5499..d30031dbf2d38 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -655,6 +655,7 @@ enum spd_duplex {
 /* OCP_POWER_CFG */
 #define EEE_CLKDIV_EN		0x8000
 #define EN_ALDPS		0x0004
+#define EN_ALDPS_PLLOFF         0x0002
 #define EN_10M_PLLOFF		0x0001
 
 /* OCP_EEE_CONFIG1 */
@@ -1964,6 +1965,16 @@ static void sram2_write_w0w1(struct r8152 *tp, u16 addr, u16 clear, u16 set)
 	ocp_reg_write(tp, OCP_SRAM2_DATA, data);
 }
 
+static void sram2_set_bits(struct r8152 *tp, u16 addr, u16 set)
+{
+	sram2_write_w0w1(tp, addr, 0, set);
+}
+
+static void sram2_clr_bits(struct r8152 *tp, u16 addr, u16 clear)
+{
+	sram2_write_w0w1(tp, addr, clear, 0);
+}
+
 static void r8152_mdio_clr_bit(struct r8152 *tp, u16 addr, u16 clear)
 {
 	int data;
@@ -8131,11 +8142,14 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)
 	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_PHY_PWR, PFM_PWM_SWITCH);
 
 	/* Advanced Power Saving parameter */
-	ocp_reg_set_bits(tp, 0xa430, BIT(0) | BIT(1));
+	ocp_reg_set_bits(tp, OCP_POWER_CFG, EN_10M_PLLOFF | EN_ALDPS_PLLOFF);
 
 	/* Disable ALDPS force mode */
 	ocp_reg_clr_bits(tp, 0xa44a, BIT(2));
 
+	/* Disable bypass_turn_off_clk_in_aldps */
+	ocp_byte_clr_bits(tp, MCU_TYPE_PLA, 0xd3c8, BIT(0));
+
 	switch (tp->version) {
 	case RTL_VER_16:
 		/* XG_INRX parameter */
@@ -8151,7 +8165,7 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)
 		sram2_write_w0w1(tp, 0x8078, 0xff00, 0x3000);
 
 		/* green mode */
-		sram2_write_w0w1(tp, 0x89e9, 0xff00, 0);
+		sram2_clr_bits(tp, 0x89e9, 0xff00);
 		sram2_write_w0w1(tp, 0x8ffd, 0xff00, 0x0100);
 		sram2_write_w0w1(tp, 0x8ffe, 0xff00, 0x0200);
 		sram2_write_w0w1(tp, 0x8fff, 0xff00, 0x0400);
@@ -8257,12 +8271,85 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)
 		sram2_write_w0w1(tp, 0x807c, 0xff00, 0x5000);
 		sram2_write_w0w1(tp, 0x809d, 0xff00, 0x5000);
 		break;
+	default:
+		break;
+	}
 
+	if (rtl_phy_patch_request(tp, true, true))
+		return;
+
+	ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL4, EEE_SPDWN_EN);
+
+	ocp_reg_w0w1(tp, OCP_DOWN_SPEED, EN_EEE_100 | EN_EEE_1000, EN_10M_CLKDIV);
+
+	tp->ups_info._10m_ckdiv = true;
+	tp->ups_info.eee_plloff_100 = false;
+	tp->ups_info.eee_plloff_giga = false;
+
+	ocp_reg_set_bits(tp, OCP_POWER_CFG, EEE_CLKDIV_EN);
+	tp->ups_info.eee_ckdiv = true;
+
+	rtl_phy_patch_request(tp, false, true);
+
+	rtl_green_en(tp, test_bit(GREEN_ETHERNET, &tp->flags));
+
+	ocp_reg_clr_bits(tp, 0xa428, BIT(9));
+	ocp_reg_clr_bits(tp, 0xa5ea, BIT(0) | BIT(1));
+	tp->ups_info.lite_mode = 0;
+
+	if (tp->eee_en)
+		rtl_eee_enable(tp, true);
+
+	r8153_aldps_en(tp, true);
+	r8152b_enable_fc(tp);
+
+	set_bit(PHY_RESET, &tp->flags);
+}
+
+static void r8159_hw_phy_cfg(struct r8152 *tp)
+{
+	u16 data;
+
+	r8156b_wait_loading_flash(tp);
+
+	ocp_word_test_and_clr_bits(tp, MCU_TYPE_USB, USB_MISC_0, PCUT_STATUS);
+
+	data = r8153_phy_status(tp, 0);
+	switch (data) {
+	case PHY_STAT_EXT_INIT:
+		rtl8152_apply_firmware(tp, true);
+		ocp_reg_clr_bits(tp, 0xa466, BIT(0));
+		ocp_reg_clr_bits(tp, 0xa468, BIT(3) | BIT(1));
+		break;
+	case PHY_STAT_LAN_ON:
+	case PHY_STAT_PWRDN:
+	default:
+		rtl8152_apply_firmware(tp, false);
+		break;
+	}
+
+	r8152_mdio_test_and_clr_bit(tp, MII_BMCR, BMCR_PDOWN);
+
+	r8153_aldps_en(tp, false);
+
+	data = r8153_phy_status(tp, PHY_STAT_LAN_ON);
+	WARN_ON_ONCE(data != PHY_STAT_LAN_ON);
+
+	/* PFM mode */
+	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_PHY_PWR, PFM_PWM_SWITCH);
+
+	/* Advanced Power Saving parameter */
+	ocp_reg_set_bits(tp, OCP_POWER_CFG, EN_10M_PLLOFF | EN_ALDPS_PLLOFF);
+
+	/* Disable ALDPS force mode */
+	ocp_reg_clr_bits(tp, 0xa44a, BIT(2));
+
+	/* Disable bypass_turn_off_clk_in_aldps */
+	ocp_byte_clr_bits(tp, MCU_TYPE_PLA, 0xd3c8, BIT(0));
+
+	switch (tp->version) {
 	case RTL_VER_17_QFN68:
 	case RTL_VER_17_QFN100:
-		/* Disable bypass turn off clk in ALDPS */
-		ocp_byte_clr_bits(tp, MCU_TYPE_PLA, 0xd3c8, BIT(0));
-
 		/* Power level tuning
 		 * test mode power level
 		 */
@@ -8272,22 +8359,35 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)
 		sram_write_w0w1(tp, 0x81ae, 0xff00, 0x0f00);
 		sram_write_w0w1(tp, 0x81b9, 0xff00, 0xb900);
 		/* normal link TX filter */
-		sram2_write_w0w1(tp, 0x83b0, 0x0e00, 0);
-		sram2_write_w0w1(tp, 0x83c5, 0x0e00, 0);
-		sram2_write_w0w1(tp, 0x83da, 0x0e00, 0);
-		sram2_write_w0w1(tp, 0x83ef, 0x0e00, 0);
+		sram2_clr_bits(tp, 0x83b0, 0x0e00);
+		sram2_clr_bits(tp, 0x83c5, 0x0e00);
+		sram2_clr_bits(tp, 0x83da, 0x0e00);
+		sram2_clr_bits(tp, 0x83ef, 0x0e00);
+
+		ocp_reg_w0w1(tp, 0xbf38, 0x01f0, 0x0160);
+		ocp_reg_w0w1(tp, 0xbf3a, 0x001f, 0x0014);
+		/* shorten CLKS latency */
+		ocp_reg_clr_bits(tp, 0xbf28, BIT(14) | BIT(13));
+		ocp_reg_clr_bits(tp, 0xbf2c, BIT(15) | BIT(14));
+		/* CMP_Timer on MP_Timer=333
+		 * GPHY OCP 0xbf28 bit[0] = 0x1
+		 * GPHY OCP 0xbf28 bit[6:1] = 0x3
+		 * GPHY OCP 0xbf28 bit[12:7] = 0x3
+		 */
+		ocp_reg_w0w1(tp, 0xbf28, 0x1fff, 0x0187);
+		ocp_reg_w0w1(tp, 0xbf2a, 0x3f, 0x03);
 
 		/* AFE power saving for 2.5G & 5G */
 		sram_write(tp, 0x8173, 0x8620);
 		sram_write(tp, 0x8175, 0x8671);
 
-		sram_write_w0w1(tp, 0x817c, 0, BIT(13));
-		sram_write_w0w1(tp, 0x8187, 0, BIT(13));
-		sram_write_w0w1(tp, 0x8192, 0, BIT(13));
-		sram_write_w0w1(tp, 0x819d, 0, BIT(13));
-		sram_write_w0w1(tp, 0x81a8, BIT(13), 0);
-		sram_write_w0w1(tp, 0x81b3, BIT(13), 0);
-		sram_write_w0w1(tp, 0x81be, 0, BIT(13));
+		sram_set_bits(tp, 0x817c, BIT(13));
+		sram_set_bits(tp, 0x8187, BIT(13));
+		sram_set_bits(tp, 0x8192, BIT(13));
+		sram_set_bits(tp, 0x819d, BIT(13));
+		sram_clr_bits(tp, 0x81a8, BIT(13));
+		sram_clr_bits(tp, 0x81b3, BIT(13));
+		sram_set_bits(tp, 0x81be, BIT(13));
 
 		sram_write_w0w1(tp, 0x817d, 0xff00, 0xa600);
 		sram_write_w0w1(tp, 0x8188, 0xff00, 0xa600);
@@ -8351,10 +8451,10 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)
 		sram2_write_w0w1(tp, 0x84b2, 0xff00, 0x6000);
 		/* Training AAGC PAR (with uc2 patch) */
 		sram2_write(tp, 0x8ffc, 0x6008);
-		sram2_write(tp, 0x8ffe, 0xf450);
+		sram2_write(tp, 0x8ffe, 0xf4ff);
 		/* DAC BGK */
-		sram2_write_w0w1(tp, 0x8015, 0, BIT(9));
-		sram2_write_w0w1(tp, 0x8016, 0, BIT(11));
+		sram2_set_bits(tp, 0x8015, BIT(9));
+		sram2_set_bits(tp, 0x8016, BIT(11));
 		sram2_write_w0w1(tp, 0x8fe6, 0xff00, 0x0800);
 		sram2_write(tp, 0x8fe4, 0x2114);
 		/* 10G PBO table */
@@ -8363,14 +8463,14 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)
 		sram2_write_w0w1(tp, 0x864b, 0xff00, 0xdc00);
 		/* 2.5G ado power window size */
 		sram2_write_w0w1(tp, 0x8154, 0xc000, 0x4000);
-		sram2_write_w0w1(tp, 0x8158, 0xc000, 0);
+		sram2_clr_bits(tp, 0x8158, 0xc000);
 		/* 10G lock far */
 		sram2_write(tp, 0x826c, 0xffff);
 		sram2_write(tp, 0x826e, 0xffff);
 		/* XG INRX parameter */
 		sram2_write_w0w1(tp, 0x8872, 0xff00, 0x0e00);
-		sram_write_w0w1(tp, 0x8012, 0, BIT(11));
-		sram_write_w0w1(tp, 0x8012, 0, BIT(14));
+		sram_set_bits(tp, 0x8012, BIT(11));
+		sram_set_bits(tp, 0x8012, BIT(14));
 		ocp_reg_set_bits(tp, 0xb576, BIT(0));
 		sram_write_w0w1(tp, 0x834a, 0xff00, 0x0700);
 		sram2_write_w0w1(tp, 0x8217, 0x3f00, 0x2a00);
@@ -8381,7 +8481,7 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)
 		/* improve UBE */
 		ocp_reg_set_bits(tp, 0xbf0c, 0x7 << 11);
 		/* close Sparse NEC, improve connect 5EUU cable performance */
-		sram2_write_w0w1(tp, 0x88de, 0xff00, 0);
+		sram2_clr_bits(tp, 0x88de, 0xff00);
 		/* 5G slave compatibility issue */
 		sram2_write(tp, 0x80b4, 0x5195);
 
@@ -8440,8 +8540,15 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)
 		sram2_write(tp, 0x8ff8, 0xaa5a);
 
 		sram2_write_w0w1(tp, 0x88d5, 0xff00, 0x0200);
-		break;
 
+		/* spdchg_pga1_lpf_cap */
+		sram_write_w0w1(tp, 0x84bb, 0xff00, 0x0a00);
+		sram_write_w0w1(tp, 0x84c0, 0xff00, 0x1600);
+
+		/* ENET PLL jitter improvement */
+		ocp_reg_w0w1(tp, 0xbf8a, 0xfc00, 0x2000);
+		ocp_reg_set_bits(tp, 0xbf88, BIT(2));
+		break;
 	default:
 		break;
 	}
@@ -8451,9 +8558,9 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)
 
 	ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL4, EEE_SPDWN_EN);
 
-	ocp_reg_w0w1(tp, OCP_DOWN_SPEED, EN_EEE_100 | EN_EEE_1000, EN_10M_CLKDIV);
-
-	tp->ups_info._10m_ckdiv = true;
+	ocp_reg_clr_bits(tp, OCP_DOWN_SPEED,
+			 EN_EEE_100 | EN_EEE_1000 | EN_10M_CLKDIV);
+	tp->ups_info._10m_ckdiv = false;
 	tp->ups_info.eee_plloff_100 = false;
 	tp->ups_info.eee_plloff_giga = false;
 
@@ -8465,7 +8572,7 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)
 	rtl_green_en(tp, test_bit(GREEN_ETHERNET, &tp->flags));
 
 	ocp_reg_clr_bits(tp, 0xa428, BIT(9));
-	ocp_reg_clr_bits(tp, 0xa5ea, BIT(0) | BIT(1));
+	ocp_reg_clr_bits(tp, 0xa5ea, BIT(0) | BIT(1) | BIT(2));
 	tp->ups_info.lite_mode = 0;
 
 	if (tp->eee_en)
@@ -10264,7 +10371,7 @@ static int rtl_ops_init(struct r8152 *tp)
 		ops->eee_get		= r8153_get_eee;
 		ops->eee_set		= r8152_set_eee;
 		ops->in_nway		= rtl8153_in_nway;
-		ops->hw_phy_cfg		= r8157_hw_phy_cfg;
+		ops->hw_phy_cfg		= r8159_hw_phy_cfg;
 		ops->autosuspend_en	= rtl8157_runtime_enable;
 		ops->change_mtu		= rtl8156_change_mtu;
 		tp->rx_buf_sz		= 48 * 1024;
-- 
2.34.1

[PATCH net-next v4 5/8] r8152: add rtl8157_unload and rtl8157_change_mtu

[Chih Kai Hsu]

RTL8157 and RTL8159 used rtl8153_unload() and rtl8156_change_mtu(),
which program power-cut and MTPS the way earlier chips require. Per
the datasheet, RTL8157/RTL8159 need r8157_power_cut_en() (already
used by r8157_init()/r8159_init()) instead of r8153_power_cut_en(),
and an MTPS scaled from the configured MTU (10K-16K, max_pkt_size /
64 capped at 0xff) instead of a fixed MTPS_JUMBO/MTPS_DEFAULT.

Add rtl8157_unload(), used only by RTL8157/RTL8159: calls
r8157_power_cut_en(tp, false), then the Interrupt Mitigation disable
that rtl8153_unload() used to gate on "tp->version >= RTL_VER_16"
unconditionally, since this function now only runs for those
versions.

Add rtl8157_change_mtu(), also RTL8157/RTL8159-only, and wire it up
via ops->change_mtu in place of rtl8156_change_mtu().

Signed-off-by: Chih Kai Hsu <hsu.chih.kai@realtek.com>
---
 drivers/net/usb/r8152.c | 47 ++++++++++++++++++++++++++++++++---------
 1 file changed, 37 insertions(+), 10 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index d30031dbf2d38..f44dcb88b5c6a 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -7004,6 +7004,28 @@ static void rtl8156_down(struct r8152 *tp)
 	r8153_aldps_en(tp, true);
 }
 
+static void rtl8157_change_mtu(struct r8152 *tp)
+{
+	u32 max_pkt_size = mtu_to_size(tp->netdev->mtu);
+	u32 ocp_data;
+
+	ocp_write_word(tp, MCU_TYPE_PLA, PLA_RMS, max_pkt_size);
+
+	/* Use at least 10K for MTPS */
+	ocp_data = max_t(u32, max_pkt_size, 10 * 1024) / 64;
+
+	/* 16 * 1024 / 64 = 0x100, so the max is 0xff for 8 bits data */
+	ocp_data = min_t(u32, ocp_data, 0xff);
+
+	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_MTPS, ocp_data);
+	r8156_fc_parameter(tp);
+
+	/* TX share fifo free credit full threshold */
+	ocp_write_word(tp, MCU_TYPE_PLA, PLA_TXFIFO_CTRL, 512 / 64);
+	ocp_write_word(tp, MCU_TYPE_PLA, PLA_TXFIFO_FULL,
+		       ALIGN(max_pkt_size + tp->tx_desc.size, 1024) / 16);
+}
+
 static void rtl8157_up(struct r8152 *tp)
 {
 	if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
@@ -7028,7 +7050,7 @@ static void rtl8157_up(struct r8152 *tp)
 
 	rtl_rx_vlan_en(tp, tp->netdev->features & NETIF_F_HW_VLAN_CTAG_RX);
 
-	rtl8156_change_mtu(tp);
+	rtl8157_change_mtu(tp);
 
 	/* share FIFO settings */
 	ocp_word_w0w1(tp, MCU_TYPE_PLA, PLA_RXFIFO_FULL, RXFIFO_FULL_MASK,
@@ -10151,11 +10173,6 @@ static void rtl8153_unload(struct r8152 *tp)
 		return;
 
 	r8153_power_cut_en(tp, false);
-
-	if (tp->version >= RTL_VER_16) {
-		/* Disable Interrupt Mitigation */
-		ocp_byte_clr_bits(tp, MCU_TYPE_USB, 0xcf04, BIT(0) | BIT(1) | BIT(2) | BIT(7));
-	}
 }
 
 static void rtl8153b_unload(struct r8152 *tp)
@@ -10166,6 +10183,16 @@ static void rtl8153b_unload(struct r8152 *tp)
 	r8153b_power_cut_en(tp, false);
 }
 
+static void rtl8157_unload(struct r8152 *tp)
+{
+	if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
+		return;
+
+	r8157_power_cut_en(tp, false);
+	ocp_byte_clr_bits(tp, MCU_TYPE_USB, 0xcf04,
+			  BIT(0) | BIT(1) | BIT(2) | BIT(7));
+}
+
 static int r8152_desc_init(struct r8152 *tp)
 {
 	tp->rx_desc.size = sizeof(struct rx_desc);
@@ -10344,13 +10371,13 @@ static int rtl_ops_init(struct r8152 *tp)
 		ops->disable		= rtl8153_disable;
 		ops->up			= rtl8157_up;
 		ops->down		= rtl8157_down;
-		ops->unload		= rtl8153_unload;
+		ops->unload		= rtl8157_unload;
 		ops->eee_get		= r8153_get_eee;
 		ops->eee_set		= r8152_set_eee;
 		ops->in_nway		= rtl8153_in_nway;
 		ops->hw_phy_cfg		= r8157_hw_phy_cfg;
 		ops->autosuspend_en	= rtl8157_runtime_enable;
-		ops->change_mtu		= rtl8156_change_mtu;
+		ops->change_mtu		= rtl8157_change_mtu;
 		tp->rx_buf_sz		= 32 * 1024;
 		tp->support_2500full	= 1;
 		tp->support_5000full	= 1;
@@ -10367,13 +10394,13 @@ static int rtl_ops_init(struct r8152 *tp)
 		ops->disable		= rtl8153_disable;
 		ops->up			= rtl8157_up;
 		ops->down		= rtl8157_down;
-		ops->unload		= rtl8153_unload;
+		ops->unload		= rtl8157_unload;
 		ops->eee_get		= r8153_get_eee;
 		ops->eee_set		= r8152_set_eee;
 		ops->in_nway		= rtl8153_in_nway;
 		ops->hw_phy_cfg		= r8159_hw_phy_cfg;
 		ops->autosuspend_en	= rtl8157_runtime_enable;
-		ops->change_mtu		= rtl8156_change_mtu;
+		ops->change_mtu		= rtl8157_change_mtu;
 		tp->rx_buf_sz		= 48 * 1024;
 		tp->support_2500full	= 1;
 		tp->support_5000full	= 1;
-- 
2.34.1

[PATCH net-next v4 6/8] r8152: add TGPHY register access for RTL8157 and RTL8159

[Chih Kai Hsu]

RTL8157 and RTL8159 read/write PHY registers through a new TGPHY
command interface (USB_TGPHY_ADDR/DATA/CMD) instead of the
OCP-base-switching path used by every earlier chip: ocp_reg_read/
write() reprograms PLA_OCP_GPHY_BASE whenever the top nibble of the
requested address changes. Support for these chips needs a second
PHY access path, so ocp_reg_read()/ocp_reg_write() can no longer
hardcode the single implementation shared by all versions.

Rename the existing implementations to r8152_phy_read()/
r8152_phy_write(), and turn ocp_reg_read()/ocp_reg_write() into
dispatch wrappers over new tp->rtl_ops.phy_read/phy_write function
pointers, set per-chip in rtl_ops_init(). Add r8157_phy_read()/
r8157_phy_write(), built on rtl_tgphy_access(), which polls
TGPHY_CMD_BUSY the same way wait_autoload_done() polls for
autoload completion.

Unlike wait_autoload_done() though, rtl_tgphy_access() aborts the
remaining steps of the access on a busy-wait failure instead of
falling through: USB_TGPHY_ADDR/CMD/DATA form a single hardware
transaction gated by TGPHY_CMD_BUSY, not a sequence of independent
register writes, so proceeding past an unresolved busy bit would
race the in-flight command rather than just leaving an unrelated
register unset.

Since ocp_reg_read()/ocp_reg_write() can now fail on a TGPHY timeout,
every helper built on top of them (sram_read/write, sram2_read/write,
the ocp_reg_*_bits and MDIO bit helpers) is converted from void/raw-
value returns to int, propagating the failure to callers instead of
silently operating on stale data.

Signed-off-by: Chih Kai Hsu <hsu.chih.kai@realtek.com>
---
 drivers/net/usb/r8152.c | 430 +++++++++++++++++++++++++++++++---------
 1 file changed, 334 insertions(+), 96 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index f44dcb88b5c6a..3473061af129a 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -162,6 +162,9 @@
 #define USB_ADV_ADDR		0xd5d6
 #define USB_ADV_DATA		0xd5d8
 #define USB_ADV_CMD		0xd5dc
+#define USB_TGPHY_ADDR		0xd630
+#define USB_TGPHY_DATA		0xd632
+#define USB_TGPHY_CMD		0xd634
 #define USB_UPS_CTRL		0xd800
 #define USB_POWER_CUT		0xd80a
 #define USB_MISC_0		0xd81a
@@ -511,6 +514,10 @@
 #define ADV_CMD_WR		BIT(1)
 #define ADV_CMD_IP		BIT(2)
 
+/* USB_TGPHY_CMD */
+#define TGPHY_CMD_BUSY		BIT(0)
+#define TGPHY_CMD_WR		BIT(1)
+
 /* USB_UPS_CTRL */
 #define POWER_CUT		0x0100
 
@@ -959,6 +966,8 @@ struct r8152 {
 		void (*hw_phy_cfg)(struct r8152 *tp);
 		void (*autosuspend_en)(struct r8152 *tp, bool enable);
 		void (*change_mtu)(struct r8152 *tp);
+		int (*phy_read)(struct r8152 *tp, u16 addr, u16 *data);
+		int (*phy_write)(struct r8152 *tp, u16 addr, u16 data);
 	} rtl_ops;
 
 	struct ups_info {
@@ -1638,7 +1647,7 @@ static void ocp_write_byte(struct r8152 *tp, u16 type, u16 index, u32 data)
 	generic_ocp_write(tp, index, byen, sizeof(tmp), &tmp, type);
 }
 
-static u16 ocp_reg_read(struct r8152 *tp, u16 addr)
+static int r8152_phy_read(struct r8152 *tp, u16 addr, u16 *data)
 {
 	u16 ocp_base, ocp_index;
 
@@ -1649,10 +1658,12 @@ static u16 ocp_reg_read(struct r8152 *tp, u16 addr)
 	}
 
 	ocp_index = (addr & 0x0fff) | 0xb000;
-	return ocp_read_word(tp, MCU_TYPE_PLA, ocp_index);
+	*data = ocp_read_word(tp, MCU_TYPE_PLA, ocp_index);
+
+	return 0;
 }
 
-static void ocp_reg_write(struct r8152 *tp, u16 addr, u16 data)
+static int r8152_phy_write(struct r8152 *tp, u16 addr, u16 data)
 {
 	u16 ocp_base, ocp_index;
 
@@ -1664,16 +1675,33 @@ static void ocp_reg_write(struct r8152 *tp, u16 addr, u16 data)
 
 	ocp_index = (addr & 0x0fff) | 0xb000;
 	ocp_write_word(tp, MCU_TYPE_PLA, ocp_index, data);
+
+	return 0;
+}
+
+static int ocp_reg_read(struct r8152 *tp, u16 addr, u16 *data)
+{
+	return tp->rtl_ops.phy_read(tp, addr, data);
+}
+
+static int ocp_reg_write(struct r8152 *tp, u16 addr, u16 data)
+{
+	return tp->rtl_ops.phy_write(tp, addr, data);
 }
 
-static inline void r8152_mdio_write(struct r8152 *tp, u32 reg_addr, u32 value)
+static inline int r8152_mdio_write(struct r8152 *tp, u32 reg_addr, u32 value)
 {
-	ocp_reg_write(tp, OCP_BASE_MII + reg_addr * 2, value);
+	return ocp_reg_write(tp, OCP_BASE_MII + reg_addr * 2, value);
 }
 
 static inline int r8152_mdio_read(struct r8152 *tp, u32 reg_addr)
 {
-	return ocp_reg_read(tp, OCP_BASE_MII + reg_addr * 2);
+	u16 data;
+	int ret;
+
+	ret = ocp_reg_read(tp, OCP_BASE_MII + reg_addr * 2, &data);
+
+	return ret < 0 ? ret : data;
 }
 
 static int wait_cmd_ready(struct r8152 *tp, u16 cmd)
@@ -1792,16 +1820,32 @@ static int rtl_ip_set_bits(struct r8152 *tp, u16 addr, u32 set)
 	return rtl_ip_w0w1(tp, addr, 0, set);
 }
 
-static void sram_write(struct r8152 *tp, u16 addr, u16 data)
+static int sram_write(struct r8152 *tp, u16 addr, u16 data)
 {
-	ocp_reg_write(tp, OCP_SRAM_ADDR, addr);
+	int ret;
+
+	ret = ocp_reg_write(tp, OCP_SRAM_ADDR, addr);
+	if (ret < 0)
+		goto out;
+
 	ocp_reg_write(tp, OCP_SRAM_DATA, data);
+
+out:
+	return ret;
 }
 
-static u16 sram_read(struct r8152 *tp, u16 addr)
+static int sram_read(struct r8152 *tp, u16 addr, u16 *data)
 {
-	ocp_reg_write(tp, OCP_SRAM_ADDR, addr);
-	return ocp_reg_read(tp, OCP_SRAM_DATA);
+	int ret;
+
+	ret = ocp_reg_write(tp, OCP_SRAM_ADDR, addr);
+	if (ret < 0)
+		goto out;
+
+	ret = ocp_reg_read(tp, OCP_SRAM_DATA, data);
+
+out:
+	return ret;
 }
 
 static int read_mii_word(struct net_device *netdev, int phy_id, int reg)
@@ -1906,100 +1950,217 @@ static void ocp_byte_set_bits(struct r8152 *tp, u16 type, u16 index, u8 set)
 	ocp_byte_w0w1(tp, type, index, 0, set);
 }
 
-static void ocp_reg_w0w1(struct r8152 *tp, u16 addr, u16 clear, u16 set)
+static int ocp_reg_w0w1(struct r8152 *tp, u16 addr, u16 clear, u16 set)
 {
 	u16 data;
+	int ret;
+
+	ret = ocp_reg_read(tp, addr, &data);
+	if (ret < 0)
+		goto out;
 
-	data = ocp_reg_read(tp, addr);
 	data = (data & ~clear) | set;
-	ocp_reg_write(tp, addr, data);
+	ret = ocp_reg_write(tp, addr, data);
+
+out:
+	return ret;
 }
 
-static void ocp_reg_clr_bits(struct r8152 *tp, u16 addr, u16 clear)
+static int ocp_reg_clr_bits(struct r8152 *tp, u16 addr, u16 clear)
 {
-	ocp_reg_w0w1(tp, addr, clear, 0);
+	return ocp_reg_w0w1(tp, addr, clear, 0);
 }
 
-static void ocp_reg_set_bits(struct r8152 *tp, u16 addr, u16 set)
+static int ocp_reg_set_bits(struct r8152 *tp, u16 addr, u16 set)
 {
-	ocp_reg_w0w1(tp, addr, 0, set);
+	return ocp_reg_w0w1(tp, addr, 0, set);
 }
 
-static void sram_write_w0w1(struct r8152 *tp, u16 addr, u16 clear, u16 set)
+static int sram_write_w0w1(struct r8152 *tp, u16 addr, u16 clear, u16 set)
 {
 	u16 data;
+	int ret;
+
+	ret = sram_read(tp, addr, &data);
+	if (ret < 0)
+		goto out;
 
-	data = sram_read(tp, addr);
 	data = (data & ~clear) | set;
-	ocp_reg_write(tp, OCP_SRAM_DATA, data);
+	ret = ocp_reg_write(tp, OCP_SRAM_DATA, data);
+
+out:
+	return ret;
 }
 
-static void sram_clr_bits(struct r8152 *tp, u16 addr, u16 clear)
+static int sram_clr_bits(struct r8152 *tp, u16 addr, u16 clear)
 {
-	sram_write_w0w1(tp, addr, clear, 0);
+	return sram_write_w0w1(tp, addr, clear, 0);
 }
 
-static void sram_set_bits(struct r8152 *tp, u16 addr, u16 set)
+static int sram_set_bits(struct r8152 *tp, u16 addr, u16 set)
 {
-	sram_write_w0w1(tp, addr, 0, set);
+	return sram_write_w0w1(tp, addr, 0, set);
 }
 
-static void sram2_write(struct r8152 *tp, u16 addr, u16 data)
+static int sram2_write(struct r8152 *tp, u16 addr, u16 data)
 {
-	ocp_reg_write(tp, OCP_SRAM2_ADDR, addr);
-	ocp_reg_write(tp, OCP_SRAM2_DATA, data);
+	int ret;
+
+	ret = ocp_reg_write(tp, OCP_SRAM2_ADDR, addr);
+	if (ret < 0)
+		goto out;
+
+	ret = ocp_reg_write(tp, OCP_SRAM2_DATA, data);
+
+out:
+	return ret;
 }
 
-static u16 sram2_read(struct r8152 *tp, u16 addr)
+static int sram2_read(struct r8152 *tp, u16 addr, u16 *data)
 {
-	ocp_reg_write(tp, OCP_SRAM2_ADDR, addr);
-	return ocp_reg_read(tp, OCP_SRAM2_DATA);
+	int ret;
+
+	ret = ocp_reg_write(tp, OCP_SRAM2_ADDR, addr);
+	if (ret < 0)
+		goto out;
+
+	ret = ocp_reg_read(tp, OCP_SRAM2_DATA, data);
+
+out:
+	return ret;
 }
 
-static void sram2_write_w0w1(struct r8152 *tp, u16 addr, u16 clear, u16 set)
+static int sram2_write_w0w1(struct r8152 *tp, u16 addr, u16 clear, u16 set)
 {
 	u16 data;
+	int ret;
+
+	ret = sram2_read(tp, addr, &data);
+	if (ret < 0)
+		goto out;
 
-	data = sram2_read(tp, addr);
 	data = (data & ~clear) | set;
-	ocp_reg_write(tp, OCP_SRAM2_DATA, data);
+	ret = ocp_reg_write(tp, OCP_SRAM2_DATA, data);
+
+out:
+	return ret;
 }
 
-static void sram2_set_bits(struct r8152 *tp, u16 addr, u16 set)
+static int sram2_set_bits(struct r8152 *tp, u16 addr, u16 set)
 {
-	sram2_write_w0w1(tp, addr, 0, set);
+	return sram2_write_w0w1(tp, addr, 0, set);
 }
 
-static void sram2_clr_bits(struct r8152 *tp, u16 addr, u16 clear)
+static int sram2_clr_bits(struct r8152 *tp, u16 addr, u16 clear)
 {
-	sram2_write_w0w1(tp, addr, clear, 0);
+	return sram2_write_w0w1(tp, addr, clear, 0);
 }
 
-static void r8152_mdio_clr_bit(struct r8152 *tp, u16 addr, u16 clear)
+static int r8152_mdio_clr_bit(struct r8152 *tp, u16 addr, u16 clear)
 {
 	int data;
 
 	data = r8152_mdio_read(tp, addr);
-	r8152_mdio_write(tp, addr, data & ~clear);
+	if (data < 0)
+		goto out;
+
+	data = r8152_mdio_write(tp, addr, data & ~clear);
+
+out:
+	return data;
 }
 
-static void r8152_mdio_set_bit(struct r8152 *tp, u16 addr, u16 set)
+static int r8152_mdio_set_bit(struct r8152 *tp, u16 addr, u16 set)
 {
 	int data;
 
 	data = r8152_mdio_read(tp, addr);
-	r8152_mdio_write(tp, addr, data | set);
+	if (data < 0)
+		goto out;
+
+	data = r8152_mdio_write(tp, addr, data | set);
+
+out:
+	return data;
 }
 
 static int r8152_mdio_test_and_clr_bit(struct r8152 *tp, u16 addr, u16 clear)
 {
-	int data;
+	int data, ret;
 
-	data = r8152_mdio_read(tp, addr);
-	if (data & clear)
-		r8152_mdio_write(tp, addr, data & ~clear);
+	ret = r8152_mdio_read(tp, addr);
+	if (ret < 0)
+		goto out;
 
-	return data & clear;
+	data = ret;
+	if (data & clear) {
+		ret = r8152_mdio_write(tp, addr, data & ~clear);
+		if (ret < 0)
+			goto out;
+	}
+
+	ret = !!(data & clear);
+
+out:
+	return ret;
+}
+
+static int wait_tgphy_cmd_ready(struct r8152 *tp)
+{
+	u16 ocp_data;
+	int ret;
+
+	ret = read_poll_timeout(ocp_read_word, ocp_data,
+				test_bit(RTL8152_INACCESSIBLE, &tp->flags) ||
+				!(ocp_data & TGPHY_CMD_BUSY),
+				2000, 20000, false, tp,
+				MCU_TYPE_USB, USB_TGPHY_CMD);
+
+	if (ret)
+		dev_err(&tp->intf->dev, "TGPHY cmd busy timeout\n");
+
+	return test_bit(RTL8152_INACCESSIBLE, &tp->flags) ? -ENODEV : ret;
+}
+
+static int rtl_tgphy_access(struct r8152 *tp, u16 addr, u16 *data, bool write)
+{
+	u16 cmd = 0;
+	int ret;
+
+	ret = wait_tgphy_cmd_ready(tp);
+	if (ret < 0)
+		goto out;
+
+	if (write) {
+		cmd |= TGPHY_CMD_WR;
+		ocp_write_word(tp, MCU_TYPE_USB, USB_TGPHY_DATA, *data);
+	}
+
+	ocp_write_word(tp, MCU_TYPE_USB, USB_TGPHY_ADDR, addr);
+
+	cmd |= TGPHY_CMD_BUSY;
+	ocp_write_word(tp, MCU_TYPE_USB, USB_TGPHY_CMD, cmd);
+
+	if (!write) {
+		ret = wait_tgphy_cmd_ready(tp);
+		if (ret < 0)
+			goto out;
+
+		*data = ocp_read_word(tp, MCU_TYPE_USB, USB_TGPHY_DATA);
+	}
+
+out:
+	return ret;
+}
+
+static int r8157_phy_read(struct r8152 *tp, u16 addr, u16 *data)
+{
+	return rtl_tgphy_access(tp, addr, data, false);
+}
+
+static int r8157_phy_write(struct r8152 *tp, u16 addr, u16 data)
+{
+	return rtl_tgphy_access(tp, addr, &data, true);
 }
 
 static int
@@ -4177,11 +4338,13 @@ static void r8153b_green_en(struct r8152 *tp, bool enable)
 
 static u16 r8153_phy_status(struct r8152 *tp, u16 desired)
 {
-	u16 data;
+	u16 data = 0;
 	int i;
 
 	for (i = 0; i < 500; i++) {
-		data = ocp_reg_read(tp, OCP_PHY_STATUS);
+		if (ocp_reg_read(tp, OCP_PHY_STATUS, &data) < 0)
+			break;
+
 		data &= PHY_STAT_MASK;
 		if (desired) {
 			if (data == desired)
@@ -4587,7 +4750,8 @@ static inline void rtl_reset_ocp_base(struct r8152 *tp)
 static int rtl_phy_patch_request(struct r8152 *tp, bool request, bool wait)
 {
 	u16 check;
-	int i;
+	u16 ocp_data = 0;
+	int i, ret;
 
 	if (request) {
 		ocp_reg_set_bits(tp, OCP_PHY_PATCH_CMD, PATCH_REQUEST);
@@ -4598,25 +4762,23 @@ static int rtl_phy_patch_request(struct r8152 *tp, bool request, bool wait)
 	}
 
 	for (i = 0; wait && i < 5000; i++) {
-		u32 ocp_data;
-
 		if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
 			return -ENODEV;
 
 		usleep_range(1000, 2000);
-		ocp_data = ocp_reg_read(tp, OCP_PHY_PATCH_STAT);
-		if ((ocp_data & PATCH_READY) ^ check)
+		ret = ocp_reg_read(tp, OCP_PHY_PATCH_STAT, &ocp_data);
+		if (ret < 0 || (ocp_data & PATCH_READY) ^ check)
 			break;
 	}
 
-	if (request && wait &&
-	    !(ocp_reg_read(tp, OCP_PHY_PATCH_STAT) & PATCH_READY)) {
+	ret = ocp_reg_read(tp, OCP_PHY_PATCH_STAT, &ocp_data);
+	if (request && wait && (ret < 0 || !(ocp_data & PATCH_READY))) {
 		dev_err(&tp->intf->dev, "PHY patch request fail\n");
 		rtl_phy_patch_request(tp, false, false);
 		return -ETIME;
-	} else {
-		return 0;
 	}
+
+	return 0;
 }
 
 static void rtl_patch_key_set(struct r8152 *tp, u16 key_addr, u16 patch_key)
@@ -5331,10 +5493,12 @@ static void rtl_ram_code_speed_up(struct r8152 *tp, struct fw_phy_speed_up *phy,
 {
 	u32 len;
 	u8 *data;
+	u16 ver = 0;
 
 	rtl_reset_ocp_base(tp);
 
-	if (sram_read(tp, SRAM_GPHY_FW_VER) >= __le16_to_cpu(phy->version)) {
+	sram_read(tp, SRAM_GPHY_FW_VER, &ver);
+	if (ver >= __le16_to_cpu(phy->version)) {
 		dev_dbg(&tp->intf->dev, "PHY firmware has been the newest\n");
 		return;
 	}
@@ -5381,7 +5545,9 @@ static void rtl_ram_code_speed_up(struct r8152 *tp, struct fw_phy_speed_up *phy,
 
 	rtl_phy_patch_request(tp, false, wait);
 
-	if (sram_read(tp, SRAM_GPHY_FW_VER) == __le16_to_cpu(phy->version))
+	ver = 0;
+	sram_read(tp, SRAM_GPHY_FW_VER, &ver);
+	if (ver == __le16_to_cpu(phy->version))
 		dev_dbg(&tp->intf->dev, "successfully applied %s\n", phy->info);
 	else
 		dev_err(&tp->intf->dev, "ram code speedup mode fail\n");
@@ -5389,14 +5555,15 @@ static void rtl_ram_code_speed_up(struct r8152 *tp, struct fw_phy_speed_up *phy,
 
 static int rtl8152_fw_phy_ver(struct r8152 *tp, struct fw_phy_ver *phy_ver)
 {
-	u16 ver_addr, ver;
+	u16 ver_addr, ver, cur_ver = 0;
 
 	ver_addr = __le16_to_cpu(phy_ver->ver.addr);
 	ver = __le16_to_cpu(phy_ver->ver.data);
 
 	rtl_reset_ocp_base(tp);
 
-	if (sram_read(tp, ver_addr) >= ver) {
+	sram_read(tp, ver_addr, &cur_ver);
+	if (cur_ver >= ver) {
 		dev_dbg(&tp->intf->dev, "PHY firmware has been the newest\n");
 		return 0;
 	}
@@ -5415,7 +5582,8 @@ static void rtl8152_fw_phy_fixup(struct r8152 *tp, struct fw_phy_fixup *fix)
 	rtl_reset_ocp_base(tp);
 
 	addr = __le16_to_cpu(fix->setting.addr);
-	data = ocp_reg_read(tp, addr);
+	if (ocp_reg_read(tp, addr, &data) < 0)
+		return;
 
 	switch (__le16_to_cpu(fix->bit_cmd)) {
 	case FW_FIXUP_AND:
@@ -5719,10 +5887,10 @@ static inline void r8152_mmd_indirect(struct r8152 *tp, u16 dev, u16 reg)
 
 static u16 r8152_mmd_read(struct r8152 *tp, u16 dev, u16 reg)
 {
-	u16 data;
+	u16 data = 0;
 
 	r8152_mmd_indirect(tp, dev, reg);
-	data = ocp_reg_read(tp, OCP_EEE_DATA);
+	ocp_reg_read(tp, OCP_EEE_DATA, &data);
 	ocp_reg_write(tp, OCP_EEE_AR, 0x0000);
 
 	return data;
@@ -5787,7 +5955,8 @@ static void r8156_eee_en(struct r8152 *tp, bool enable)
 
 	r8153_eee_en(tp, enable);
 
-	config = ocp_reg_read(tp, OCP_EEE_ADV2);
+	if (ocp_reg_read(tp, OCP_EEE_ADV2, &config) < 0)
+		return;
 
 	if (enable && (tp->eee_adv2 & MDIO_EEE_2_5GT))
 		config |= MDIO_EEE_2_5GT;
@@ -6243,8 +6412,8 @@ static void r8153b_hw_phy_cfg(struct r8152 *tp)
 	 * rg_saw_cnt = OCP reg 0xC426 Bit[13:0]
 	 * swr_cnt_1ms_ini = 16000000 / rg_saw_cnt
 	 */
-	ocp_data = ocp_reg_read(tp, 0xc426);
-	ocp_data &= 0x3fff;
+	ocp_reg_read(tp, 0xc426, &data);
+	ocp_data = data & 0x3fff;
 	if (ocp_data) {
 		u32 swr_cnt_1ms_ini;
 
@@ -6601,7 +6770,11 @@ static int rtl8152_set_speed(struct r8152 *tp, u8 autoneg, u32 speed, u8 duplex,
 		if (!advertising)
 			return -EINVAL;
 
-		orig = r8152_mdio_read(tp, MII_ADVERTISE);
+		ret = r8152_mdio_read(tp, MII_ADVERTISE);
+		if (ret < 0)
+			goto out;
+
+		orig = ret;
 		new1 = orig & ~(ADVERTISE_10HALF | ADVERTISE_10FULL |
 				ADVERTISE_100HALF | ADVERTISE_100FULL);
 		if (advertising & RTL_ADVERTISED_10_HALF) {
@@ -6628,7 +6801,11 @@ static int rtl8152_set_speed(struct r8152 *tp, u8 autoneg, u32 speed, u8 duplex,
 		}
 
 		if (tp->mii.supports_gmii) {
-			orig = r8152_mdio_read(tp, MII_CTRL1000);
+			ret = r8152_mdio_read(tp, MII_CTRL1000);
+			if (ret < 0)
+				goto out;
+
+			orig = ret;
 			new1 = orig & ~(ADVERTISE_1000FULL |
 					ADVERTISE_1000HALF);
 
@@ -6642,7 +6819,10 @@ static int rtl8152_set_speed(struct r8152 *tp, u8 autoneg, u32 speed, u8 duplex,
 		}
 
 		if (tp->support_2500full || tp->support_5000full || tp->support_10000full) {
-			orig = ocp_reg_read(tp, OCP_10GBT_CTRL);
+			ret = ocp_reg_read(tp, OCP_10GBT_CTRL, &orig);
+			if (ret < 0)
+				goto out;
+
 			new1 = orig & ~(MDIO_AN_10GBT_CTRL_ADV2_5G | MDIO_AN_10GBT_CTRL_ADV5G
 					| MDIO_AN_10GBT_CTRL_ADV10G);
 
@@ -6686,7 +6866,7 @@ static int rtl8152_set_speed(struct r8152 *tp, u8 autoneg, u32 speed, u8 duplex,
 	}
 
 out:
-	return ret;
+	return ret < 0 ? ret : 0;
 }
 
 static void rtl8152_up(struct r8152 *tp)
@@ -7136,7 +7316,12 @@ static bool rtl8152_in_nway(struct r8152 *tp)
 
 static bool rtl8153_in_nway(struct r8152 *tp)
 {
-	u16 phy_state = ocp_reg_read(tp, OCP_PHY_STATE) & 0xff;
+	u16 phy_state;
+
+	if (ocp_reg_read(tp, OCP_PHY_STATE, &phy_state) < 0)
+		return false;
+
+	phy_state &= 0xff;
 
 	if (phy_state == TXDIS_STATE || phy_state == ABD_STATE)
 		return false;
@@ -7152,7 +7337,9 @@ static void r8156_mdio_force_mode(struct r8152 *tp)
 	 * 0: MDIO force mode
 	 * 1: MMD force mode
 	 */
-	data = ocp_reg_read(tp, 0xa5b4);
+	if (ocp_reg_read(tp, 0xa5b4, &data) < 0)
+		return;
+
 	if (data & BIT(15)) {
 		data &= ~BIT(15);
 		ocp_reg_write(tp, 0xa5b4, data);
@@ -7791,19 +7978,20 @@ static void r8156_hw_phy_cfg(struct r8152 *tp)
 		ocp_reg_clr_bits(tp, 0xa86a, BIT(0));
 
 		/* MDI SWAP */
+		ocp_reg_read(tp, 0xd068, &data);
 		if ((ocp_read_word(tp, MCU_TYPE_USB, USB_UPS_CFG) & MID_REVERSE) &&
-		    (ocp_reg_read(tp, 0xd068) & BIT(1))) {
+		    (data & BIT(1))) {
 			u16 swap_a, swap_b;
 
-			data = ocp_reg_read(tp, 0xd068);
+			ocp_reg_read(tp, 0xd068, &data);
 			data &= ~0x1f;
 			data |= 0x1; /* p0 */
 			ocp_reg_write(tp, 0xd068, data);
-			swap_a = ocp_reg_read(tp, 0xd06a);
+			ocp_reg_read(tp, 0xd06a, &swap_a);
 			data &= ~0x18;
 			data |= 0x18; /* p3 */
 			ocp_reg_write(tp, 0xd068, data);
-			swap_b = ocp_reg_read(tp, 0xd06a);
+			ocp_reg_read(tp, 0xd06a, &swap_b);
 			data &= ~0x18; /* p0 */
 			ocp_reg_write(tp, 0xd068, data);
 			ocp_reg_write(tp, 0xd06a,
@@ -7815,11 +8003,11 @@ static void r8156_hw_phy_cfg(struct r8152 *tp)
 			data &= ~0x18;
 			data |= 0x08; /* p1 */
 			ocp_reg_write(tp, 0xd068, data);
-			swap_a = ocp_reg_read(tp, 0xd06a);
+			ocp_reg_read(tp, 0xd06a, &swap_a);
 			data &= ~0x18;
 			data |= 0x10; /* p2 */
 			ocp_reg_write(tp, 0xd068, data);
-			swap_b = ocp_reg_read(tp, 0xd06a);
+			ocp_reg_read(tp, 0xd06a, &swap_b);
 			data &= ~0x18;
 			data |= 0x08; /* p1 */
 			ocp_reg_write(tp, 0xd068, data);
@@ -7830,16 +8018,16 @@ static void r8156_hw_phy_cfg(struct r8152 *tp)
 			ocp_reg_write(tp, 0xd068, data);
 			ocp_reg_write(tp, 0xd06a,
 				      (swap_b & ~0x7ff) | (swap_a & 0x7ff));
-			swap_a = ocp_reg_read(tp, 0xbd5a);
-			swap_b = ocp_reg_read(tp, 0xbd5c);
+			ocp_reg_read(tp, 0xbd5a, &swap_a);
+			ocp_reg_read(tp, 0xbd5c, &swap_b);
 			ocp_reg_write(tp, 0xbd5a, (swap_a & ~0x1f1f) |
 				      ((swap_b & 0x1f) << 8) |
 				      ((swap_b >> 8) & 0x1f));
 			ocp_reg_write(tp, 0xbd5c, (swap_b & ~0x1f1f) |
 				      ((swap_a & 0x1f) << 8) |
 				      ((swap_a >> 8) & 0x1f));
-			swap_a = ocp_reg_read(tp, 0xbc18);
-			swap_b = ocp_reg_read(tp, 0xbc1a);
+			ocp_reg_read(tp, 0xbc18, &swap_a);
+			ocp_reg_read(tp, 0xbc1a, &swap_b);
 			ocp_reg_write(tp, 0xbc18, (swap_a & ~0x1f1f) |
 				      ((swap_b & 0x1f) << 8) |
 				      ((swap_b >> 8) & 0x1f));
@@ -9436,8 +9624,16 @@ int rtl8152_get_link_ksettings(struct net_device *netdev,
 			 cmd->link_modes.supported, tp->support_10000full);
 
 	if (tp->support_2500full || tp->support_5000full || tp->support_10000full) {
-		u16 ocp_10gbt_ctrl = ocp_reg_read(tp, OCP_10GBT_CTRL);
-		u16 ocp_10gbt_stat = ocp_reg_read(tp, OCP_10GBT_STAT);
+		u16 ocp_10gbt_ctrl;
+		u16 ocp_10gbt_stat;
+
+		ret = ocp_reg_read(tp, OCP_10GBT_CTRL, &ocp_10gbt_ctrl);
+		if (ret < 0)
+			goto out_unlock;
+
+		ret = ocp_reg_read(tp, OCP_10GBT_STAT, &ocp_10gbt_stat);
+		if (ret < 0)
+			goto out_unlock;
 
 		if (tp->support_2500full) {
 			linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
@@ -9479,12 +9675,13 @@ int rtl8152_get_link_ksettings(struct net_device *netdev,
 		}
 	}
 
+out_unlock:
 	mutex_unlock(&tp->control);
 
 	usb_autopm_put_interface(tp->intf);
 
 out:
-	return ret;
+	return ret < 0 ? ret : 0;
 }
 
 static int rtl8152_set_link_ksettings(struct net_device *dev,
@@ -9665,21 +9862,37 @@ static int r8153_get_eee(struct r8152 *tp, struct ethtool_keee *eee)
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(common) = {};
 	u16 speed = rtl8152_get_speed(tp);
 	u16 val;
+	int ret;
+
+	ret = ocp_reg_read(tp, OCP_EEE_ABLE, &val);
+	if (ret < 0)
+		goto out;
 
-	val = ocp_reg_read(tp, OCP_EEE_ABLE);
 	mii_eee_cap1_mod_linkmode_t(eee->supported, val);
 
-	val = ocp_reg_read(tp, OCP_EEE_ADV);
+	ret = ocp_reg_read(tp, OCP_EEE_ADV, &val);
+	if (ret < 0)
+		goto out;
+
 	mii_eee_cap1_mod_linkmode_t(eee->advertised, val);
 
-	val = ocp_reg_read(tp, OCP_EEE_LPABLE);
+	ret = ocp_reg_read(tp, OCP_EEE_LPABLE, &val);
+	if (ret < 0)
+		goto out;
+
 	mii_eee_cap1_mod_linkmode_t(eee->lp_advertised, val);
 
 	if (tp->support_2500full || tp->support_5000full) {
-		val = ocp_reg_read(tp, OCP_EEE_ADV2);
+		ret = ocp_reg_read(tp, OCP_EEE_ADV2, &val);
+		if (ret < 0)
+			goto out;
+
 		mii_eee_cap2_mod_linkmode_adv_t(eee->advertised, val);
 
-		val = ocp_reg_read(tp, OCP_EEE_LPABLE2);
+		ret = ocp_reg_read(tp, OCP_EEE_LPABLE2, &val);
+		if (ret < 0)
+			goto out;
+
 		mii_eee_cap2_mod_linkmode_adv_t(eee->lp_advertised, val);
 	}
 
@@ -9715,7 +9928,8 @@ static int r8153_get_eee(struct r8152 *tp, struct ethtool_keee *eee)
 	linkmode_and(common, common, eee->lp_advertised);
 	eee->eee_active = !linkmode_empty(common);
 
-	return 0;
+out:
+	return ret < 0 ? ret : 0;
 }
 
 static int
@@ -10002,7 +10216,11 @@ static int rtl8152_set_pauseparam(struct net_device *netdev, struct ethtool_paus
 
 	mutex_lock(&tp->control);
 
-	if (pause->autoneg && !(r8152_mdio_read(tp, MII_BMCR) & BMCR_ANENABLE)) {
+	ret = r8152_mdio_read(tp, MII_BMCR);
+	if (ret < 0)
+		goto out;
+
+	if (pause->autoneg && !(ret & BMCR_ANENABLE)) {
 		ret = -EINVAL;
 		goto out;
 	}
@@ -10013,7 +10231,11 @@ static int rtl8152_set_pauseparam(struct net_device *netdev, struct ethtool_paus
 	if (pause->tx_pause)
 		cap |= FLOW_CTRL_TX;
 
-	old = r8152_mdio_read(tp, MII_ADVERTISE);
+	ret = r8152_mdio_read(tp, MII_ADVERTISE);
+	if (ret < 0)
+		goto out;
+
+	old = ret;
 	new1 = (old & ~(ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM)) | mii_advertise_flowctrl(cap);
 	if (old != new1)
 		r8152_mdio_write(tp, MII_ADVERTISE, new1);
@@ -10022,7 +10244,7 @@ static int rtl8152_set_pauseparam(struct net_device *netdev, struct ethtool_paus
 	mutex_unlock(&tp->control);
 	usb_autopm_put_interface(tp->intf);
 
-	return ret;
+	return ret < 0 ? ret : 0;
 }
 
 static const struct ethtool_ops ops = {
@@ -10245,6 +10467,8 @@ static int rtl_ops_init(struct r8152 *tp)
 		ops->in_nway		= rtl8152_in_nway;
 		ops->hw_phy_cfg		= r8152b_hw_phy_cfg;
 		ops->autosuspend_en	= rtl_runtime_suspend_enable;
+		ops->phy_read		= r8152_phy_read;
+		ops->phy_write		= r8152_phy_write;
 		tp->rx_buf_sz		= 16 * 1024;
 		tp->eee_en		= true;
 		tp->eee_adv		= MDIO_EEE_100TX;
@@ -10267,6 +10491,8 @@ static int rtl_ops_init(struct r8152 *tp)
 		ops->hw_phy_cfg		= r8153_hw_phy_cfg;
 		ops->autosuspend_en	= rtl8153_runtime_enable;
 		ops->change_mtu		= rtl8153_change_mtu;
+		ops->phy_read		= r8152_phy_read;
+		ops->phy_write		= r8152_phy_write;
 		if (tp->udev->speed < USB_SPEED_SUPER)
 			tp->rx_buf_sz	= 16 * 1024;
 		else
@@ -10290,6 +10516,8 @@ static int rtl_ops_init(struct r8152 *tp)
 		ops->hw_phy_cfg		= r8153b_hw_phy_cfg;
 		ops->autosuspend_en	= rtl8153b_runtime_enable;
 		ops->change_mtu		= rtl8153_change_mtu;
+		ops->phy_read		= r8152_phy_read;
+		ops->phy_write		= r8152_phy_write;
 		tp->rx_buf_sz		= 32 * 1024;
 		tp->eee_en		= true;
 		tp->eee_adv		= MDIO_EEE_1000T | MDIO_EEE_100TX;
@@ -10314,6 +10542,8 @@ static int rtl_ops_init(struct r8152 *tp)
 		ops->hw_phy_cfg		= r8156_hw_phy_cfg;
 		ops->autosuspend_en	= rtl8156_runtime_enable;
 		ops->change_mtu		= rtl8156_change_mtu;
+		ops->phy_read		= r8152_phy_read;
+		ops->phy_write		= r8152_phy_write;
 		tp->rx_buf_sz		= 48 * 1024;
 		tp->support_2500full	= 1;
 		r8152_desc_init(tp);
@@ -10339,6 +10569,8 @@ static int rtl_ops_init(struct r8152 *tp)
 		ops->hw_phy_cfg		= r8156b_hw_phy_cfg;
 		ops->autosuspend_en	= rtl8156_runtime_enable;
 		ops->change_mtu		= rtl8156_change_mtu;
+		ops->phy_read		= r8152_phy_read;
+		ops->phy_write		= r8152_phy_write;
 		tp->rx_buf_sz		= 48 * 1024;
 		r8152_desc_init(tp);
 		break;
@@ -10356,6 +10588,8 @@ static int rtl_ops_init(struct r8152 *tp)
 		ops->hw_phy_cfg		= r8153c_hw_phy_cfg;
 		ops->autosuspend_en	= rtl8153c_runtime_enable;
 		ops->change_mtu		= rtl8153c_change_mtu;
+		ops->phy_read		= r8152_phy_read;
+		ops->phy_write		= r8152_phy_write;
 		tp->rx_buf_sz		= 32 * 1024;
 		tp->eee_en		= true;
 		tp->eee_adv		= MDIO_EEE_1000T | MDIO_EEE_100TX;
@@ -10378,6 +10612,8 @@ static int rtl_ops_init(struct r8152 *tp)
 		ops->hw_phy_cfg		= r8157_hw_phy_cfg;
 		ops->autosuspend_en	= rtl8157_runtime_enable;
 		ops->change_mtu		= rtl8157_change_mtu;
+		ops->phy_read		= r8157_phy_read;
+		ops->phy_write		= r8157_phy_write;
 		tp->rx_buf_sz		= 32 * 1024;
 		tp->support_2500full	= 1;
 		tp->support_5000full	= 1;
@@ -10401,6 +10637,8 @@ static int rtl_ops_init(struct r8152 *tp)
 		ops->hw_phy_cfg		= r8159_hw_phy_cfg;
 		ops->autosuspend_en	= rtl8157_runtime_enable;
 		ops->change_mtu		= rtl8157_change_mtu;
+		ops->phy_read		= r8157_phy_read;
+		ops->phy_write		= r8157_phy_write;
 		tp->rx_buf_sz		= 48 * 1024;
 		tp->support_2500full	= 1;
 		tp->support_5000full	= 1;
-- 
2.34.1

[PATCH net-next v4 7/8] r8152: extract rtl_fc_pause_pkt_en() and apply it to RTL8156/8157/8159

[Chih Kai Hsu]

The flow-control patch in r8156b_init() only ran for RTL8156B
(VER_12/13/15). RTL8156A (VER_10/11) and RTL8157/RTL8159 (VER_16/17)
never applied any flow-control patch at all.

Extract it into rtl_fc_pause_pkt_en(tp, speed) and add the missing
cases:

- VER_10/11: same FC timer + FLOW_CTRL_PATCH_OPT sequence RTL8153B
  (VER_09) already applies via its pre_fw/post_fw hooks, applied
  directly here instead.

- VER_16/17_QFN68/QFN100: RTL8157/RTL8159 use a different mechanism.
  The datasheet gives a fixed pause-packet count per link speed via
  the new PLA_PAUSE_LIMIT register; num_pause_pkts is computed with
  a log2-ratio formula that reproduces that table. Since the count
  depends on the negotiated speed, call rtl_fc_pause_pkt_en() from
  rtl8157_enable() instead of from init.

Signed-off-by: Chih Kai Hsu <hsu.chih.kai@realtek.com>
---
 drivers/net/usb/r8152.c | 112 ++++++++++++++++++++++++++++++++++------
 1 file changed, 97 insertions(+), 15 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 3473061af129a..05b2b79ab3e31 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -89,6 +89,7 @@
 #define PLA_MTPS		0xe615
 #define PLA_TXFIFO_CTRL		0xe618
 #define PLA_TXFIFO_FULL		0xe61a
+#define PLA_PAUSE_LIMIT         0xe61e
 #define PLA_RSTTALLY		0xe800
 #define PLA_CR			0xe813
 #define PLA_CRWECR		0xe81c
@@ -301,6 +302,10 @@
 #define MTPS_JUMBO		(12 * 1024 / 64)
 #define MTPS_DEFAULT		(6 * 1024 / 64)
 
+/* PLA_PAUSE_LIMIT */
+#define PAUSE_LIMIT_EN		BIT(3)
+#define PAUSE_LIMIT_MASK	0xf0
+
 /* PLA_RSTTALLY */
 #define TALLY_RESET		0x0001
 
@@ -6163,6 +6168,93 @@ static void r8152b_enter_oob(struct r8152 *tp)
 			   RCR_APM | RCR_AM | RCR_AB);
 }
 
+static void rtl_fc_pause_pkt_en(struct r8152 *tp, u16 speed)
+{
+	int log2_ratio, ratio;
+	u16 num_pause_pkts;
+	u32 ocp_data;
+
+	switch (tp->version) {
+	case RTL_VER_10:
+	case RTL_VER_11:
+		ocp_write_word(tp, MCU_TYPE_USB, USB_FC_TIMER,
+			       CTRL_TIMER_EN | (1000 / 8));
+
+		ocp_word_set_bits(tp, MCU_TYPE_USB, USB_FW_CTRL,
+				  FLOW_CTRL_PATCH_OPT);
+
+		ocp_word_set_bits(tp, MCU_TYPE_USB, USB_FW_TASK, FC_PATCH_TASK);
+		break;
+	case RTL_VER_12:
+	case RTL_VER_13:
+	case RTL_VER_15:
+		ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR, SLOT_EN);
+
+		ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_CPCR, FLOW_CTRL_EN);
+
+		/* enable fc timer and set timer to 600 ms. */
+		ocp_write_word(tp, MCU_TYPE_USB, USB_FC_TIMER,
+			       CTRL_TIMER_EN | (600 / 8));
+
+		ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_POL_GPIO_CTRL);
+		if (!(ocp_data & DACK_DET_EN))
+			ocp_word_set_bits(tp, MCU_TYPE_USB, USB_FW_CTRL,
+					  FLOW_CTRL_PATCH_2);
+
+		ocp_word_set_bits(tp, MCU_TYPE_USB, USB_FW_TASK, FC_PATCH_TASK);
+		break;
+	case RTL_VER_16:
+	case RTL_VER_17_QFN68:
+	case RTL_VER_17_QFN100:
+		ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR, SLOT_EN);
+
+		num_pause_pkts = 0xa;
+		ratio = 10000;
+
+		if (!(speed & LINK_STATUS)) {
+			dev_dbg(&tp->intf->dev, "No link\n");
+			goto no_link;
+		} else if (speed & _10bps) {
+			ratio /= 10;
+		} else if (speed & _100bps) {
+			ratio /= 100;
+		} else if (speed & _1000bps) {
+			ratio /= 1000;
+		} else if (speed & _2500bps) {
+			ratio /= 2500;
+		} else if (speed & _5000bps) {
+			ratio /= 5000;
+		} else if (speed & _10000bps) {
+			ratio /= 10000;
+		} else {
+			dev_err(&tp->intf->dev, "Unknown link speed\n");
+			goto no_link;
+		}
+
+		log2_ratio = ilog2(ratio);
+		num_pause_pkts -= log2_ratio;
+
+		/* Round up if ratio is more than halfway to the next power of 2.
+		 * Floating-point is avoided by rewriting
+		 * ratio > 1.5 * 2^log2_ratio as
+		 * 2 * ratio > 3 * 2^log2_ratio
+		 */
+		if (2 * ratio > 3 * (1 << log2_ratio))
+			num_pause_pkts--;
+
+no_link:
+		ocp_word_w0w1(tp, MCU_TYPE_PLA, PLA_PAUSE_LIMIT,
+			      PAUSE_LIMIT_MASK | PAUSE_LIMIT_EN,
+			      num_pause_pkts << 4);
+
+		ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_PAUSE_LIMIT,
+				  PAUSE_LIMIT_EN);
+		break;
+	default:
+		break;
+	}
+}
+
 static int r8153_pre_firmware_1(struct r8152 *tp)
 {
 	int i;
@@ -6694,6 +6786,8 @@ static int rtl8157_enable(struct r8152 *tp)
 	r8153_set_rx_early_size(tp);
 
 	speed = rtl8152_get_speed(tp);
+	rtl_fc_pause_pkt_en(tp, speed);
+
 	rtl_set_ifg(tp, speed);
 
 	return rtl_enable(tp);
@@ -8842,6 +8936,8 @@ static void r8156_init(struct r8152 *tp)
 
 	usb_enable_lpm(tp->udev);
 
+	rtl_fc_pause_pkt_en(tp, 0);
+
 	r8156_mac_clk_spd(tp, true);
 
 	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3,
@@ -8878,7 +8974,6 @@ static void r8156b_u2phy_backup(struct r8152 *tp)
 
 static void r8156b_init(struct r8152 *tp)
 {
-	u32 ocp_data;
 	u16 data;
 
 	if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
@@ -8943,20 +9038,7 @@ static void r8156b_init(struct r8152 *tp)
 
 	usb_enable_lpm(tp->udev);
 
-	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR, SLOT_EN);
-
-	ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_CPCR, FLOW_CTRL_EN);
-
-	/* enable fc timer and set timer to 600 ms. */
-	ocp_write_word(tp, MCU_TYPE_USB, USB_FC_TIMER,
-		       CTRL_TIMER_EN | (600 / 8));
-
-	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_POL_GPIO_CTRL);
-	if (!(ocp_data & DACK_DET_EN))
-		ocp_word_set_bits(tp, MCU_TYPE_USB, USB_FW_CTRL,
-				  FLOW_CTRL_PATCH_2);
-
-	ocp_word_set_bits(tp, MCU_TYPE_USB, USB_FW_TASK, FC_PATCH_TASK);
+	rtl_fc_pause_pkt_en(tp, 0);
 
 	r8156_mac_clk_spd(tp, true);
 
-- 
2.34.1

[PATCH net-next v4 8/8] r8152: enable UPS for RTL8157 and RTL8159 QFN68

[Chih Kai Hsu]

RTL8157 and RTL8159 QFN68 support UPS, but rtl8157_runtime_enable()
never toggled it on runtime suspend/resume, and r8156_ups_flags()
used the older per-chip EEE/lite_mode encoding instead of the
per-speed EEE flags their UPS firmware expects. RTL8159 QFN100 does
not support UPS.

Add r8157_ups_en(), mirroring r8156_ups_en(), and call it from
rtl8157_runtime_enable() for VER_16/QFN68 only; QFN100 keeps its
existing no-UPS behaviour there. Use r8157_ups_en(tp, false) in
r8157_init()/r8159_init() in place of r8156_ups_en(tp, false).

Extend r8156_ups_flags() with per-speed EEE flags
(UPS_FLAGS_EN_100M_EEE/1000M_EEE/2500M_EEE/5000M_EEE/10G_EEE) and
skip the lite_mode bits[6:5] encoding for VER_16/QFN68. Add
ups_flags_speed() entries 10 and 11 for NWAY_5000M_FULL and
NWAY_10000M_FULL, previously unmapped.

Add the USB_FW_USE_VER register (0xcfd7), used by the new
r8157_ups_en() disable path.

Signed-off-by: Chih Kai Hsu <hsu.chih.kai@realtek.com>
---
 drivers/net/usb/r8152.c | 110 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 98 insertions(+), 12 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 05b2b79ab3e31..ebe9cc9322b3c 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -132,6 +132,7 @@
 #define USB_BURST_SIZE		0xcfc0
 #define USB_FW_FIX_EN0		0xcfca
 #define USB_FW_FIX_EN1		0xcfcc
+#define USB_FW_USE_VER          0xcfd7
 #define USB_LPM_CONFIG		0xcfd8
 #define USB_ECM_OPTION		0xcfee
 #define USB_CSTMR		0xcfef	/* RTL8153A */
@@ -619,6 +620,11 @@
 #define UPS_FLAGS_250M_CKDIV		BIT(2)
 #define UPS_FLAGS_EN_ALDPS		BIT(3)
 #define UPS_FLAGS_CTAP_SHORT_DIS	BIT(4)
+#define UPS_FLAGS_EN_100M_EEE		BIT(9)
+#define UPS_FLAGS_EN_1000M_EEE		BIT(10)
+#define UPS_FLAGS_EN_2500M_EEE		BIT(11)
+#define UPS_FLAGS_EN_5000M_EEE		BIT(12)
+#define UPS_FLAGS_EN_10G_EEE		BIT(13)
 #define UPS_FLAGS_SPEED_MASK		(0xf << 16)
 #define ups_flags_speed(x)		((x) << 16)
 #define UPS_FLAGS_EN_EEE		BIT(20)
@@ -4244,8 +4250,27 @@ static void r8156_ups_flags(struct r8152 *tp)
 	if (tp->ups_info.aldps)
 		ups_flags |= UPS_FLAGS_EN_ALDPS;
 
-	if (tp->ups_info.eee)
-		ups_flags |= UPS_FLAGS_EN_EEE;
+	if (tp->ups_info.eee) {
+		switch (tp->version) {
+		case RTL_VER_17_QFN68:
+			if (tp->eee_adv & MDIO_EEE_10GT)
+				ups_flags |= UPS_FLAGS_EN_10G_EEE;
+			fallthrough;
+		case RTL_VER_16:
+			if (tp->eee_adv & MDIO_EEE_100TX)
+				ups_flags |= UPS_FLAGS_EN_100M_EEE;
+			if (tp->eee_adv & MDIO_EEE_1000T)
+				ups_flags |= UPS_FLAGS_EN_1000M_EEE;
+			if (tp->eee_adv2 & MDIO_EEE_2_5GT)
+				ups_flags |= UPS_FLAGS_EN_2500M_EEE;
+			if (tp->eee_adv2 & MDIO_EEE_5GT)
+				ups_flags |= UPS_FLAGS_EN_5000M_EEE;
+			break;
+		default:
+			ups_flags |= UPS_FLAGS_EN_EEE;
+			break;
+		}
+	}
 
 	if (tp->ups_info.flow_control)
 		ups_flags |= UPS_FLAGS_EN_FLOW_CTR;
@@ -4296,20 +4321,33 @@ static void r8156_ups_flags(struct r8152 *tp)
 	case NWAY_2500M_FULL:
 		ups_flags |= ups_flags_speed(9);
 		break;
+	case NWAY_5000M_FULL:
+		ups_flags |= ups_flags_speed(10);
+		break;
+	case NWAY_10000M_FULL:
+		ups_flags |= ups_flags_speed(11);
+		break;
 	default:
 		break;
 	}
 
-	switch (tp->ups_info.lite_mode) {
-	case 1:
-		ups_flags |= 0 << 5;
-		break;
-	case 2:
-		ups_flags |= 2 << 5;
+	switch (tp->version) {
+	case RTL_VER_16:
+	case RTL_VER_17_QFN68:
 		break;
-	case 0:
 	default:
-		ups_flags |= 1 << 5;
+		switch (tp->ups_info.lite_mode) {
+		case 1:
+			ups_flags |= 0 << 5;
+			break;
+		case 2:
+			ups_flags |= 2 << 5;
+			break;
+		case 0:
+		default:
+			ups_flags |= 1 << 5;
+			break;
+		}
 		break;
 	}
 
@@ -4484,6 +4522,35 @@ static void r8156_ups_en(struct r8152 *tp, bool enable)
 	}
 }
 
+static void r8157_ups_en(struct r8152 *tp, bool enable)
+{
+	if (enable) {
+		r8156_ups_flags(tp);
+
+		ocp_byte_set_bits(tp, MCU_TYPE_USB, USB_POWER_CUT,
+				  UPS_EN | USP_PREWAKE | PHASE2_EN);
+
+		ocp_byte_set_bits(tp, MCU_TYPE_USB, USB_MISC_2,
+				  UPS_FORCE_PWR_DOWN);
+	} else {
+		ocp_byte_clr_bits(tp, MCU_TYPE_USB, USB_POWER_CUT,
+				  UPS_EN | USP_PREWAKE);
+
+		ocp_byte_clr_bits(tp, MCU_TYPE_USB, USB_MISC_2,
+				  UPS_FORCE_PWR_DOWN);
+
+		if (ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0) & PCUT_STATUS) {
+			/* clear USB fw_ver_reg */
+			ocp_write_byte(tp, MCU_TYPE_USB, USB_FW_USE_VER, 0);
+
+			tp->rtl_ops.hw_phy_cfg(tp);
+
+			rtl8152_set_speed(tp, tp->autoneg, tp->speed,
+					  tp->duplex, tp->advertising);
+		}
+	}
+}
+
 static void r8153_power_cut_en(struct r8152 *tp, bool enable)
 {
 	if (enable)
@@ -4642,9 +4709,28 @@ static void rtl8157_runtime_enable(struct r8152 *tp, bool enable)
 		r8153b_u1u2en(tp, false);
 		r8157_u2p3en(tp, false);
 		rtl_runtime_suspend_enable(tp, true);
+
+		switch (tp->version) {
+		case RTL_VER_16:
+		case RTL_VER_17_QFN68:
+			r8157_ups_en(tp, true);
+			break;
+		default:
+			break;
+		}
 	} else {
 		r8153_queue_wake(tp, false);
 		rtl_runtime_suspend_enable(tp, false);
+
+		switch (tp->version) {
+		case RTL_VER_16:
+		case RTL_VER_17_QFN68:
+			r8157_ups_en(tp, false);
+			break;
+		default:
+			break;
+		}
+
 		r8157_u2p3en(tp, true);
 		if (tp->udev->speed >= USB_SPEED_SUPER)
 			r8153b_u1u2en(tp, true);
@@ -9111,7 +9197,7 @@ static void r8157_init(struct r8152 *tp)
 	ocp_write_word(tp, MCU_TYPE_USB, USB_U1U2_TIMER, 500);
 
 	r8157_power_cut_en(tp, false);
-	r8156_ups_en(tp, false);
+	r8157_ups_en(tp, false);
 	r8153_queue_wake(tp, false);
 	rtl_runtime_suspend_enable(tp, false);
 
@@ -9221,7 +9307,7 @@ static void r8159_init(struct r8152 *tp)
 	ocp_write_word(tp, MCU_TYPE_USB, USB_U1U2_TIMER, 500);
 
 	r8157_power_cut_en(tp, false);
-	r8156_ups_en(tp, false);
+	r8157_ups_en(tp, false);
 	r8153_queue_wake(tp, false);
 	rtl_runtime_suspend_enable(tp, false);
 
-- 
2.34.1

Re: [PATCH net-next v4 1/8] r8152: split r8156_init per chip and add missing init writes

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 (1) · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Undocumented hardware behaviour change during the split:
  r8157_init() (installed as ops->init for RTL_VER_16 in rtl_ops_init())…
- [Medium] Silent reordering of hardware handshakes for RTL8157/RTL8159.
- [Low] Commit message vs. code: the message names the FC-timer block
  ("fenced to RTL_VER_12 <= version <= RTL_VER_15") as the example of a…

Pre-existing issues:
- [Medium] An RTL8159 backup-restore timeout marks the device
  inaccessible but cannot report the failure to its caller: r8159_init()…

--

r8152: split r8156_init per chip and add missing init writes

This splits the shared r8156_init() into r8156_init(), r8156b_init(),
r8157_init() and r8159_init(), factors the AUTOLOAD_DONE polling loops into
wait_autoload_done(), and adds a number of new OCP writes to the per-chip
init sequences.  A few observations below.

First, about the motivation in the changelog:

> RTL8156, RTL8156B, RTL8157 and RTL8159 shared a single r8156_init(),
> gated by a dense set of tp->version range checks. That made the init
> sequence hard to follow, and some checks (e.g. the FC-timer block,
> which was fenced to "RTL_VER_12 <= version <= RTL_VER_15") silently
> skipped RTL8156 (VER_10/11) and RTL8157/RTL8159 (VER_16/17)
> altogether even though nothing chip-specific justified the gap.

[Severity: Low]

This isn't a bug, but after the split the FC-timer block named here still
exists only in r8156b_init().  The PLA_RCR SLOT_EN clear, PLA_CPCR
FLOW_CTRL_EN, USB_FC_TIMER, FLOW_CTRL_PATCH_2 and USB_FW_TASK FC_PATCH_TASK
writes are absent from r8156_init() (VER_10/11), r8157_init() (VER_16) and
r8159_init() (VER_17), i.e. the exact gap the paragraph complains about is
unchanged by this patch, and the "While splitting:" list does not claim to
close it.

The gap is closed later in the series by "r8152: extract
rtl_fc_pause_pkt_en() and apply it to RTL8156/8157/8159", which factors the
block into rtl_fc_pause_pkt_en() with cases for RTL_VER_10/11,
RTL_VER_12/13/15 and RTL_VER_16/17 and calls it from r8156_init(),
r8156b_init() and rtl8157_enable().

Could this paragraph say that the FC-timer gap is addressed by a later patch
in the series rather than by this one?

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index af3d7dcb2f141..00b0fda306c30 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

> @@ -542,6 +547,7 @@
>  #define RX_AGG_DISABLE		0x0010
>  #define RX_ZERO_EN		0x0080
>  #define RX_DESC_16B		0x0400
> +#define RX_END_TRANSFER_EN	BIT(11)
>  
>  /* USB_U2P3_CTRL */
>  #define U2P3_ENABLE		0x0001

[ ... ]

> @@ -8330,90 +8311,132 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)

[ ... ]

>  	switch (tp->version) {
>  	case RTL_VER_13:
>  	case RTL_VER_15:
> -	case RTL_VER_16:
> -	case RTL_VER_17:
>  		r8156b_wait_loading_flash(tp);
>  		break;
>  	default:
>  		break;
>  	}
>  
> -	for (i = 0; i < 500; i++) {
> -		if (ocp_read_word(tp, MCU_TYPE_PLA, PLA_BOOT_CTRL) &
> -		    AUTOLOAD_DONE)
> -			break;
> -
> -		msleep(20);
> -		if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
> -			return;
> -	}
> -
> -	if (tp->version == RTL_VER_17 && r8159_wait_backup_restore(tp)) {
> -		rtl_set_inaccessible(tp);
> -		dev_err(&tp->intf->dev, "init failed, backup-restore timed out\n");
> +	if (wait_autoload_done(tp))
>  		return;
> -	}

[ ... ]

> @@ -8421,73 +8444,261 @@ static void r8156_init(struct r8152 *tp)

[ ... ]

>  	set_bit(GREEN_ETHERNET, &tp->flags);
>  
> -	/* RX aggregation / 16 bytes RX descriptor
> -	 * BIT(11) is specific to RTL8159, with unknown meaning
> -	 */
> -	if (tp->version == RTL_VER_17)
> -		ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL,
> -				  RX_AGG_DISABLE | RX_DESC_16B | BIT(11));
> -	else if (tp->version == RTL_VER_16)
> -		ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL, RX_AGG_DISABLE | RX_DESC_16B);
> +	ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL,
> +			  RX_AGG_DISABLE | RX_ZERO_EN);

[ ... ]

> +static void r8157_init(struct r8152 *tp)
> +{
> +	u16 data;
> +
> +	if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
> +		return;
> +
> +	/* Enable SW reset */
> +	ocp_byte_set_bits(tp, MCU_TYPE_USB, 0xcffe, BIT(3));
> +	ocp_byte_clr_bits(tp, MCU_TYPE_USB, 0xd3ca, BIT(0));
> +
> +	ocp_byte_clr_bits(tp, MCU_TYPE_USB, USB_ECM_OP, EN_ALL_SPEED);
> +
> +	ocp_word_set_bits(tp, MCU_TYPE_USB, USB_ECM_OPTION, BYPASS_MAC_RESET);
> +
> +	r8153b_u1u2en(tp, false);
> +
> +	if (wait_autoload_done(tp))
> +		return;
> +
> +	r8156b_wait_loading_flash(tp);

[Severity: Medium]

Is this reordering deliberate?  In the shared r8156_init() the flash-patch
wait for VER_16/17 ran right after r8153b_u1u2en(tp, false) and before the
AUTOLOAD_DONE poll:

	switch (tp->version) {
	case RTL_VER_13:
	case RTL_VER_15:
	case RTL_VER_16:
	case RTL_VER_17:
		r8156b_wait_loading_flash(tp);
		break;
	...
	for (i = 0; i < 500; i++) {
		if (ocp_read_word(tp, MCU_TYPE_PLA, PLA_BOOT_CTRL) &
		    AUTOLOAD_DONE)
			break;

r8156b_init() keeps that pre-poll placement for VER_13/15, so only the
VER_16 and VER_17 paths change order here.

r8159_init() moves r8156b_wait_loading_flash() after
r8159_wait_backup_restore() as well.  Both helpers poll USB_GPHY_CTRL
(GPHY_PATCH_DONE versus BACKUP_RESTRORE), and the backup-restore poll has a
20 ms budget:

	return poll_timeout_us(ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_GPHY_CTRL),
			       ocp_data & BACKUP_RESTRORE, 200, 20000, false);

while r8156b_wait_loading_flash() allows roughly 100 ms for the patch load.
If a pending flash load can delay BACKUP_RESTRORE, can this poll now time
out where it previously could not, given that the flash wait used to
complete first?  On timeout r8159_init() calls rtl_set_inaccessible() and
abandons the rest of the bring-up.

> +
> +	data = r8153_phy_status(tp, 0);
> +	if (data == PHY_STAT_EXT_INIT) {
> +		ocp_reg_clr_bits(tp, 0xa468, BIT(3) | BIT(1));
> +		ocp_reg_clr_bits(tp, 0xa466, BIT(0));
> +	}

[ ... ]

> +	set_bit(GREEN_ETHERNET, &tp->flags);
>  
> +	/* RX aggregation / 16 bytes RX descriptor / Bulk In End transfer */
> +	ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL,
> +			  RX_AGG_DISABLE | RX_DESC_16B | RX_END_TRANSFER_EN);

[Severity: Medium]

Should r8157_init() clear USB_USB_CTRL bit 11?  Before the split that bit
was restricted to RTL_VER_17, with a comment saying it was RTL8159
specific:

	/* RX aggregation / 16 bytes RX descriptor
	 * BIT(11) is specific to RTL8159, with unknown meaning
	 */
	if (tp->version == RTL_VER_17)
		ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL,
				  RX_AGG_DISABLE | RX_DESC_16B | BIT(11));
	else if (tp->version == RTL_VER_16)
		ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL, RX_AGG_DISABLE | RX_DESC_16B);

rtl_ops_init() installs r8157_init() as ops->init for RTL_VER_16, so every
RTL8157 now has bit 11 (newly named RX_END_TRANSFER_EN) cleared on each
probe and reset_resume.

Every other per-version divergence of the old function was reproduced
exactly in the split (USB_SPEED_OPTION for versions below VER_16, RX_DETECT8
for VER_12 to VER_15, PLA_MCU_SPDWN_EN for versions below VER_16, ACT_ODMA
for versions below VER_12, r8153_u2p3en() versus r8157_u2p3en(),
r8153b_power_cut_en() versus r8157_power_cut_en()), and the changelog
enumerates each intentionally added write without mentioning this one.

Can the changelog describe what bit 11 does on RTL8157, or keep the write
limited to RTL8159 as before?

[ ... ]

> +static int r8159_wait_backup_restore(struct r8152 *tp)
> +{
> +	u32 ocp_data;
> +
> +	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0);
> +	if (!(ocp_data & PCUT_STATUS))
> +		return 0;
> +
> +	return poll_timeout_us(ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_GPHY_CTRL),
> +			       ocp_data & BACKUP_RESTRORE, 200, 20000, false);
> +}
> +
> +static void r8159_init(struct r8152 *tp)
> +{

[ ... ]

> +	if (wait_autoload_done(tp))
> +		return;
> +
> +	if (r8159_wait_backup_restore(tp)) {
> +		rtl_set_inaccessible(tp);
> +		dev_err(&tp->intf->dev,
> +			"init failed, backup-restore timed out\n");
> +		return;
>  	}

[Severity: Medium]

This is a pre-existing issue and not introduced by this patch, since the
same abort path was in the shared r8156_init(), but as it is being moved
here: how does this failure reach the caller?

rtl_set_inaccessible() only sets one flag:

	static void rtl_set_inaccessible(struct r8152 *tp)
	{
		set_bit(RTL8152_INACCESSIBLE, &tp->flags);
		smp_mb__after_atomic();
	}

PROBE_SHOULD_RETRY is set only by r8152_control_msg() on a failed control
transfer, which a BACKUP_RESTRORE poll timeout is not, and
rtl8152_probe_once() checks only that flag after calling
tp->rtl_ops.init(tp):

	if (test_bit(PROBE_SHOULD_RETRY, &tp->flags))
		goto out2;

So probe continues through register_netdev() and returns success while the
rest of r8159_init() was skipped and every subsequent
generic_ocp_read()/generic_ocp_write() short-circuits to -ENODEV, leaving a
registered netdev that cannot pass traffic until re-plug.  Would it make
sense for this path to propagate an error or queue a device reset?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917070637.62827-1-nic_swsd%40realtek.com

Re: [PATCH net-next v4 2/8] r8152: split RTL_VER_17 into QFN68 and QFN100 package variants

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] A failed or unrecognised PLA_PKG_DET read makes an already-
  identified RTL8159 (chip ID 0x2020) unusable.
- [Low] The commit message states the patch splits the enum and updates
  'every switch/case on RTL_VER_17', but it does not mention that the…
- [Low] The new diagnostic in __rtl_get_hw_ver() uses
  `dev_info(&udev->dev, "Unknown package %#02x\n", pkg_det)`.

--

r8152: split RTL_VER_17 into QFN68 and QFN100 package variants

This splits RTL_VER_17 into RTL_VER_17_QFN68 and RTL_VER_17_QFN100 and
updates the version switches accordingly. The package is detected by
reading the new PLA_PKG_DET register through a new __rtl_get_pkg_det()
helper called from __rtl_get_hw_ver() for chip ID 0x2020.

A couple of questions below.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 00b0fda306c30..9ce072c38b5b5 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

> @@ -6404,8 +6419,15 @@ static int rtl8156_enable(struct r8152 *tp)
>  	set_tx_qlen(tp);
>  	rtl_set_eee_plus(tp);
>  
> -	if (tp->version >= RTL_VER_12 && tp->version <= RTL_VER_17)
> -		ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_RX_AGGR_NUM, RX_AGGR_NUM_MASK);
> +	switch (tp->version) {
> +	case RTL_VER_10:
> +	case RTL_VER_11:
> +		break;
> +	default:
> +		ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_RX_AGGR_NUM,
> +				  RX_AGGR_NUM_MASK);
> +		break;
> +	}

[Severity: Low]
This isn't a bug, but the changelog only says "update every switch/case on
RTL_VER_17 in the driver to handle both", and this hunk does something
else: the positive range test

	if (tp->version >= RTL_VER_12 && tp->version <= RTL_VER_17)

becomes an exclusion list, so every version other than RTL_VER_10 and
RTL_VER_11 that reaches rtl8156_enable() now clears RX_AGGR_NUM_MASK by
default.

With the versions rtl_ops_init() currently routes to rtl8156_enable() the
resulting register writes are identical, and the later patch "r8152: split
rtl8156_enable/up/down into per-chip-family functions" removes this switch
again, so there is no behavioural change in the tree.

Could the changelog mention the inverted version gate, since it is not just
an enum rename?

[ ... ]

> @@ -10182,9 +10211,33 @@ static int rtl_fw_init(struct r8152 *tp)
>  	return 0;
>  }
>  
> +static u32 __rtl_get_pkg_det(struct usb_device *udev)
> +{
> +	u32 pkg_det = 0;
> +	__le32 *tmp;
> +	int ret, i;
> +
> +	tmp = kmalloc_obj(*tmp);
> +	if (!tmp)
> +		return 0;
> +
> +	for (i = 0, ret = 0; i < 3 && ret != 4; i++)
> +		ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
> +				      RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
> +				      PLA_PKG_DET, MCU_TYPE_PLA, tmp,
> +				      sizeof(*tmp), USB_CTRL_GET_TIMEOUT);
> +
> +	if (ret > 0)
> +		pkg_det = __le32_to_cpu(*tmp) & PKG_MASK;
> +
> +	kfree(tmp);
> +	return pkg_det;
> +}

[Severity: High]
Can __rtl_get_pkg_det() report a read failure to its caller?

Both the allocation failure path and the "all three control transfers
failed" path return 0, which is also a legal register value (PKG_DET bits
[4:1] all zero), so the caller cannot tell an -EPIPE/-ETIMEDOUT/-ENOMEM
from real strap data.

Two smaller points in the same helper:

The loop's success condition is ret == 4, but the value is consumed when
ret > 0, so a short control transfer would be accepted and the unread
bytes of the kmalloc'ed (non-zeroed) buffer would be folded into pkg_det.
Should this test be ret == sizeof(*tmp), matching the loop?

The version read a few lines above retries and complains with
dev_warn("Needed %d retries to read version") when the transfer misbehaves.
Would it be worth propagating an error here in the same spirit, e.g.
returning the usb_control_msg() error via an out parameter?

>  static u8 __rtl_get_hw_ver(struct usb_device *udev)
>  {
>  	u32 ocp_data = 0;
> +	u32 pkg_det = 0;
>  	__le32 *tmp;
>  	u8 version;
>  	int ret;

[ ... ]

> @@ -10267,7 +10320,16 @@ static u8 __rtl_get_hw_ver(struct usb_device *udev)
>  		version = RTL_VER_16;
>  		break;
>  	case 0x2020:
> -		version = RTL_VER_17;
> +		pkg_det = __rtl_get_pkg_det(udev);
> +		if (pkg_det == 0x1e || pkg_det == 0x1c) {
> +			version = RTL_VER_17_QFN68;
> +		} else if (pkg_det == 0x18 || pkg_det == 0x1a) {
> +			version = RTL_VER_17_QFN100;
> +		} else {
> +			version = RTL_VER_UNKNOWN;

[Severity: High]
Following on from the helper above: does a single failed PLA_PKG_DET read
make an already identified RTL8159 unusable?

The chip has been positively identified from PLA_TCR0/TCR1 as 0x2020 at
this point, but a failed read yields pkg_det == 0, which lands in the else
branch and becomes RTL_VER_UNKNOWN. In rtl8152_probe():

	version = rtl8152_get_version(intf);
	if (version == RTL_VER_UNKNOWN)
		return -ENODEV;

	for (i = 0; i < RTL8152_PROBE_TRIES; i++) {

so the -ENODEV happens before the RTL8152_PROBE_TRIES retry loop and a
transient control-transfer error is not retried at probe level either.

rtl8152_cfgselector_choose_configuration() has the same test:

	/* Switch the device to vendor mode, if and only if the vendor mode
	 * driver supports it.
	 */
	if (__rtl_get_hw_ver(udev) == RTL_VER_UNKNOWN)
		return -ENODEV;

which leaves the device in its default CDC configuration, and
rtl8153_ecm_probe() then treats the zero version as "r8152 does not
support this device":

drivers/net/usb/r8153_ecm.c:
	if (rtl8152_get_version(intf))
		return -ENODEV;

Before this patch chip ID 0x2020 unconditionally bound as RTL_VER_17.
Would it make sense to keep binding when the PKG_DET read itself fails,
and only fall back to RTL_VER_UNKNOWN for a value that was actually read?

The same else branch is also taken for the twelve other encodings
PKG_MASK = 0x1e can produce. Is rejecting the device the intended
behaviour for those, or should one of the two packages be the default?

> +			dev_info(&udev->dev, "Unknown package %#02x\n",
> +				 pkg_det);

[Severity: Low]
Does this format string produce the intended padding? The kernel
vsnprintf() counts the "0x" emitted by the '#' flag inside the field
width, so "%#02x" behaves like "%#x": pkg_det == 0 prints as 0x0 rather
than 0x00. The sibling message in the default case of the same switch uses

	dev_info(&udev->dev, "Unknown version 0x%04x\n", ocp_data);

Would "%#04x" be what was meant here?

> +		}
>  		break;
>  	default:
>  		version = RTL_VER_UNKNOWN;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917070637.62827-1-nic_swsd%40realtek.com

Re: [PATCH net-next v4 3/8] r8152: split rtl8156_enable/up/down into per-chip-family functions

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 3 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] For RTL8157/RTL8159 (RTL_VER_16, RTL_VER_17_QFN68,
  RTL_VER_17_QFN100) the newly extracted rtl8157_up() / rtl8157_down()…
- [Medium] The patch is presented as a refactor ("split
  rtl8156_enable/up/down into per-chip-family functions") but it…
- [Medium] Suspicious bitwise clear for Clear_SDR feature in rtl8157_up()

--

r8152: split rtl8156_enable/up/down into per-chip-family functions

This splits the version-guarded rtl8156_enable()/rtl8156_up()/rtl8156_down()
into per-chip-family callbacks: rtl8156b_enable() for VER_12/13/15 and
rtl8157_enable()/rtl8157_up()/rtl8157_down() for VER_16/17, while
rtl8156_up()/rtl8156_down() keep serving VER_10..VER_15. Along the way
several register writes that were previously version guarded become
unconditional for one family or another.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 9ce072c38b5b5..c513c286c5499 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

> @@ -6849,8 +6892,7 @@ static void rtl8156_up(struct r8152 *tp)
>  
>  	ocp_byte_clr_bits(tp, MCU_TYPE_PLA, PLA_OOB_CTRL, NOW_IS_OOB);
>  
> -	if (tp->version >= RTL_VER_16)
> -		ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR1, BIT(3));
> +	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR1, BIT(3));

[Severity: Medium]

Should the behaviour changes to already-supported chips be a separate
patch from the mechanical split?

The subject describes a split, but this hunk starts clearing PLA_RCR1
BIT(3) on every ifup for RTL8156/8156B, which previously only happened
for VER_16 and later. The same patch also:

- adds a Clear_SDR write for VER_13/15 (USB 0xd3ca BIT(15)) in
  rtl8156_up()
- clears USB_SPEED_OPTION RG_PWRDN_EN|ALL_SPEED_OFF for RTL8157/8159 in
  rtl8157_up(), which was excluded for them before
- adds PLA_BDC_CR ALDPS_PROXY_MODE and PLA_OOB_CTRL DIS_MCU_CLROOB to OOB
  entry in rtl8156_down() and rtl8157_down()
- changes RTL8157/8159 OOB sizing to PLA_RMS=1526 and PLA_MTPS=10K in
  rtl8157_down()

The commit message says:

  "Per the datasheet, this uncovers real register differences that the
   old version guards had wrong or missing"

If the old guards were wrong, then shipping RTL8156/8156B dongles are
mis-programmed today, in particular for the OOB/wake-on-LAN path after
ifdown. Can the message name the observed symptom, add Fixes: tags for
the commits that introduced the wrong guards (195aae321c82, fd3c7d080df5,
ad2a55b63b41), and state which chips were tested?

Splitting the refactor from the register fixes would also let the fixes be
backported and bisected on their own.

>  
>  	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_SFF_STS_7, MCU_BORW_EN);
>  
> @@ -6872,11 +6914,11 @@ static void rtl8156_up(struct r8152 *tp)
>  	ocp_word_w0w1(tp, MCU_TYPE_PLA, PLA_RXFIFO_FULL, RXFIFO_FULL_MASK,
>  		      0x08);
>  
> -	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3, PLA_MCU_SPDWN_EN);
> +	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3,
> +			  PLA_MCU_SPDWN_EN);
>  
> -	if (tp->version < RTL_VER_16)
> -		ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_SPEED_OPTION,
> -				  RG_PWRDN_EN | ALL_SPEED_OFF);
> +	ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_SPEED_OPTION,
> +			  RG_PWRDN_EN | ALL_SPEED_OFF);

[ ... ]

> @@ -6929,7 +6977,102 @@ static void rtl8156_down(struct r8152 *tp)

[ ... ]

> +static void rtl8157_up(struct r8152 *tp)
> +{
> +	if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
> +		return;
> +
> +	r8153b_u1u2en(tp, false);
> +	r8153_aldps_en(tp, false);

[ ... ]

> +	/* share FIFO settings */
> +	ocp_word_w0w1(tp, MCU_TYPE_PLA, PLA_RXFIFO_FULL, RXFIFO_FULL_MASK,
> +		      0x08);
> +
> +	ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_SPEED_OPTION,
> +			  RG_PWRDN_EN | ALL_SPEED_OFF);

[Severity: Medium]

Is dropping the PLA_MCU_SPDWN_EN programming intentional for
RTL8157/RTL8159?

Before the split, VER_16/VER_17 ran rtl8156_up(), which cleared the bit
unconditionally at exactly this spot in the sequence:

	/* share FIFO settings */
	ocp_word_w0w1(tp, MCU_TYPE_PLA, PLA_RXFIFO_FULL, RXFIFO_FULL_MASK,
		      0x08);

	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3, PLA_MCU_SPDWN_EN);

	if (tp->version < RTL_VER_16)
		ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_SPEED_OPTION,
				  RG_PWRDN_EN | ALL_SPEED_OFF);

and the old rtl8156_down() had

	ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3,
			  PLA_MCU_SPDWN_EN);

as its first register write for these chips. Neither rtl8157_up() nor
rtl8157_down() touches PLA_MAC_PWR_CTRL3.

Grepping PLA_MCU_SPDWN_EN after the patch leaves only rtl8153b_up/down,
rtl8153c_up, rtl8156_up/down, r8153b_init, r8156_init and r8156b_init.
r8157_init() and r8159_init() do not clear it either, and
r8156_mac_clk_spd(), which r8157_init() does call, only writes
PLA_MAC_PWR_CTRL and PLA_MAC_PWR_CTRL2.

So for VER_16 and both VER_17 variants the MCU speed-down enable is now
never written: while the interface is up it keeps whatever the ROM/OOB
firmware or a previous driver instance left, and the set on OOB entry is
gone. The commit message enumerates the other register deltas but not
this one. Was the paired clear-on-up / set-on-down meant to be carried
over into rtl8157_up()/rtl8157_down()?

The omission is also still present at the end of the series
(eac8e24403901016050a115a3102a0c0b46ddbbd), where the later added
rtl8157_unload() does not add it back.

> +
> +	ocp_write_dword(tp, MCU_TYPE_USB, USB_RX_BUF_TH, 0x00600400);
> +
> +	if (tp->saved_wolopts != __rtl_get_wol(tp)) {
> +		netif_warn(tp, ifup, tp->netdev, "wol setting is changed\n");
> +		__rtl_set_wol(tp, tp->saved_wolopts);
> +	}
> +
> +	r8153_aldps_en(tp, true);
> +
> +	/* Clear_SDR */
> +	ocp_byte_set_bits(tp, MCU_TYPE_USB, 0xd378, BIT(7));
> +	ocp_word_clr_bits(tp, MCU_TYPE_USB, 0xcd06, BIT(15));
> +}
> +
> +static void rtl8157_down(struct r8152 *tp)
> +{
> +	if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) {
> +		rtl_drop_queued_tx(tp);
> +		return;
> +	}
> +
> +	r8153b_u1u2en(tp, false);
> +	r8153_aldps_en(tp, false);

[Severity: Medium]
Is the clear (rather than set) of USB 0xcd06 BIT(15) here what you
meant?

The commit message says RTL8157/8159 "gain their own Clear_SDR enables
(USB 0xd378 BIT(7), USB 0xcd06 BIT(15))", and the comment above the two
writes just says "Clear_SDR". The first write uses ocp_byte_set_bits()
to set 0xd378 BIT(7), which matches "enable". The second uses
ocp_word_clr_bits() to zero 0xcd06 BIT(15), which is the opposite
direction, and it is the only Clear_SDR write in the series that clears
rather than sets: the RTL8156B path added in this same patch does

	ocp_word_set_bits(tp, MCU_TYPE_USB, 0xd3ca, BIT(15));

for its Clear_SDR enable. If 0xcd06 BIT(15) is an active-low or
disable-type bit, that would explain the asymmetry, but nothing in the
code or the changelog says so, and a plain clr/set typo here would
silently leave Clear_SDR off on RTL8157/8159 with no visible error.

Since these are bare magic addresses with no register defines, please
either give the bit a name (a #define alongside the other USB_*
registers) or extend the comment to state the polarity, e.g. that
BIT(15) of 0xcd06 must be 0 to enable Clear_SDR. Can you confirm
against the datasheet which of set/clear the hardware needs for that
bit?

[ ... ]

> +	ocp_write_word(tp, MCU_TYPE_PLA, PLA_RMS, 1526);
> +	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_MTPS, 10 * 1024 / 64);

[ ... ]

> @@ -10090,10 +10233,10 @@ static int rtl_ops_init(struct r8152 *tp)
>  		tp->eee_adv		= MDIO_EEE_1000T | MDIO_EEE_100TX;
>  		tp->eee_adv2		= MDIO_EEE_2_5GT | MDIO_EEE_5GT;
>  		ops->init		= r8157_init;
> -		ops->enable		= rtl8156_enable;
> +		ops->enable		= rtl8157_enable;
>  		ops->disable		= rtl8153_disable;
> -		ops->up			= rtl8156_up;
> -		ops->down		= rtl8156_down;
> +		ops->up			= rtl8157_up;
> +		ops->down		= rtl8157_down;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917070637.62827-1-nic_swsd%40realtek.com

Re: [PATCH net-next v4 4/8] r8152: split r8157_hw_phy_cfg into RTL8157 and RTL8159 variants

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 3 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] r8159_hw_phy_cfg() (drivers/net/usb/r8152.c:8309) contains
  functional PHY programming that the commit message does not disclose.
- [Medium] r8159_hw_phy_cfg() (drivers/net/usb/r8152.c:8309) drops the
  unconditional rtl_eee_enable(tp, false) that r8157_hw_phy_cfg() keeps…
- [Medium] The preceding patch in this series, "r8152: split RTL_VER_17
  into QFN68 and QFN100 package variants", justifies the new package…
- [Low] The commit message states the split was done "instead of
  switching on version mid-function", but both resulting functions still…

--

r8152: split r8157_hw_phy_cfg into RTL8157 and RTL8159 variants

This splits r8157_hw_phy_cfg() into an RTL8157-only version and a new
r8159_hw_phy_cfg() for RTL_VER_17_QFN68/QFN100, each carrying its own copy of
the shared prologue and epilogue.  It also adds sram2_set_bits()/
sram2_clr_bits() helpers and replaces a raw 0xa430 write with the named
OCP_POWER_CFG/EN_10M_PLLOFF/EN_ALDPS_PLLOFF macros.

No Link: tags in the commit header.

A few questions below about the new function and the commit message.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index c513c286c5499..d30031dbf2d38 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

> @@ -8257,12 +8271,85 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)
>  		sram2_write_w0w1(tp, 0x807c, 0xff00, 0x5000);
>  		sram2_write_w0w1(tp, 0x809d, 0xff00, 0x5000);
>  		break;
> +	default:
> +		break;
> +	}
>  

[ ... copy of the r8157 epilogue snipped ... ]

> +static void r8159_hw_phy_cfg(struct r8152 *tp)
> +{
> +	u16 data;
> +
> +	r8156b_wait_loading_flash(tp);
> +
> +	ocp_word_test_and_clr_bits(tp, MCU_TYPE_USB, USB_MISC_0, PCUT_STATUS);
> +
> +	data = r8153_phy_status(tp, 0);
> +	switch (data) {
> +	case PHY_STAT_EXT_INIT:
> +		rtl8152_apply_firmware(tp, true);
> +		ocp_reg_clr_bits(tp, 0xa466, BIT(0));
> +		ocp_reg_clr_bits(tp, 0xa468, BIT(3) | BIT(1));
> +		break;
> +	case PHY_STAT_LAN_ON:
> +	case PHY_STAT_PWRDN:
> +	default:
> +		rtl8152_apply_firmware(tp, false);
> +		break;
> +	}
> +
> +	r8152_mdio_test_and_clr_bit(tp, MII_BMCR, BMCR_PDOWN);
> +
> +	r8153_aldps_en(tp, false);

[Severity: Medium]
Here the rtl_eee_enable(tp, false) call is gone, while the epilogue of
r8159_hw_phy_cfg() still ends with:

	if (tp->eee_en)
		rtl_eee_enable(tp, true);

with no else branch.

When tp->eee_en is false, is there anything left in this callback that
programs the EEE registers or updates tp->ups_info.eee?  r8157_hw_phy_cfg(),
r8153_hw_phy_cfg(), r8153b_hw_phy_cfg() and the r8156/r8156b variants all
keep the unconditional disable in the same spot.

tp->eee_en is user settable through r8152_set_eee(), which does
tp->eee_en = eee->eee_enabled followed by rtl_eee_enable(tp, tp->eee_en),
i.e. both states get programmed there.  hw_phy_cfg() is what re-establishes
that policy after the PHY goes back to defaults: it is re-run from the
PCUT_STATUS power-cut recovery paths and from rtl_hw_phy_work_func_t, and
rtl8152_apply_firmware() runs just above.

So after "ethtool --set-eee <dev> eee off" and a later power-cut recovery on
RTL8159, can EEE come back enabled/advertised in hardware while ethtool still
reports eee_enabled == 0 and tp->ups_info.eee (used to build USB_UPS_FLAGS)
stays stale?

The commit message says "RTL8159 doesn't need EEE disabled before
reprogramming PHY parameters", which covers the ordering role of that call.
Does it also cover its second role of enforcing the requested EEE state?

> +
> +	data = r8153_phy_status(tp, PHY_STAT_LAN_ON);
> +	WARN_ON_ONCE(data != PHY_STAT_LAN_ON);
> +
> +	/* PFM mode */
> +	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_PHY_PWR, PFM_PWM_SWITCH);
> +
> +	/* Advanced Power Saving parameter */
> +	ocp_reg_set_bits(tp, OCP_POWER_CFG, EN_10M_PLLOFF | EN_ALDPS_PLLOFF);
> +
> +	/* Disable ALDPS force mode */
> +	ocp_reg_clr_bits(tp, 0xa44a, BIT(2));
> +
> +	/* Disable bypass_turn_off_clk_in_aldps */
> +	ocp_byte_clr_bits(tp, MCU_TYPE_PLA, 0xd3c8, BIT(0));
> +
> +	switch (tp->version) {

[Severity: Low]
The commit message says the split was done "instead of switching on version
mid-function", but both resulting functions still switch on tp->version with
only one non-default case each.

rtl_ops_init() installs ops->hw_phy_cfg = r8159_hw_phy_cfg only under
case RTL_VER_17_QFN68 / case RTL_VER_17_QFN100, and
ops->hw_phy_cfg = r8157_hw_phy_cfg only under case RTL_VER_16, so neither
default arm is reachable.

Can both switches (and their default: break; arms) simply be dropped?

>  	case RTL_VER_17_QFN68:
>  	case RTL_VER_17_QFN100:

[Severity: Medium]
The preceding patch in this series, "r8152: split RTL_VER_17 into QFN68 and
QFN100 package variants", justifies the new package detection with:

    "RTL8159 is sold in two packages, QFN68 and QFN100, which need
     different PHY tuning (added in a later patch) and a package-specific
     u2phy backup-restore patch"

This is the patch that adds the RTL8159 PHY tuning, yet both package
versions fall into the same case and get identical power-level, TX filter,
CMP timer, PLL and SRAM/SRAM2 values.  By the end of the series the only
QFN68-specific code is the u2phy backup-restore write (0xb99c BIT(0)) in
r8159_init(), the UPS enable/disable selection, and the 10G EEE UPS flag.

Is one of the two packages being programmed with the other package's PHY
parameters here, or was the per-package PHY tuning rationale in the previous
patch inaccurate?

> -		/* Disable bypass turn off clk in ALDPS */
> -		ocp_byte_clr_bits(tp, MCU_TYPE_PLA, 0xd3c8, BIT(0));
> -
>  		/* Power level tuning
>  		 * test mode power level
>  		 */
> @@ -8272,22 +8359,35 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)
>  		sram_write_w0w1(tp, 0x81ae, 0xff00, 0x0f00);
>  		sram_write_w0w1(tp, 0x81b9, 0xff00, 0xb900);
>  		/* normal link TX filter */
> -		sram2_write_w0w1(tp, 0x83b0, 0x0e00, 0);
> -		sram2_write_w0w1(tp, 0x83c5, 0x0e00, 0);
> -		sram2_write_w0w1(tp, 0x83da, 0x0e00, 0);
> -		sram2_write_w0w1(tp, 0x83ef, 0x0e00, 0);
> +		sram2_clr_bits(tp, 0x83b0, 0x0e00);
> +		sram2_clr_bits(tp, 0x83c5, 0x0e00);
> +		sram2_clr_bits(tp, 0x83da, 0x0e00);
> +		sram2_clr_bits(tp, 0x83ef, 0x0e00);
> +
> +		ocp_reg_w0w1(tp, 0xbf38, 0x01f0, 0x0160);
> +		ocp_reg_w0w1(tp, 0xbf3a, 0x001f, 0x0014);
> +		/* shorten CLKS latency */
> +		ocp_reg_clr_bits(tp, 0xbf28, BIT(14) | BIT(13));
> +		ocp_reg_clr_bits(tp, 0xbf2c, BIT(15) | BIT(14));
> +		/* CMP_Timer on MP_Timer=333
> +		 * GPHY OCP 0xbf28 bit[0] = 0x1
> +		 * GPHY OCP 0xbf28 bit[6:1] = 0x3
> +		 * GPHY OCP 0xbf28 bit[12:7] = 0x3
> +		 */
> +		ocp_reg_w0w1(tp, 0xbf28, 0x1fff, 0x0187);
> +		ocp_reg_w0w1(tp, 0xbf2a, 0x3f, 0x03);

[Severity: Medium]
These writes to 0xbf38, 0xbf3a, 0xbf28, 0xbf2c and 0xbf2a have no matching
removal anywhere in the diff, so they are new PHY programming rather than
moved code.

The commit message enumerates exactly three RTL8159 divergences (dropping
rtl_eee_enable(tp, false), clearing EN_10M_CLKDIV instead of setting it, and
the extra 0xa5ea BIT(2)) and then frames the rest as
"add sram2_set_bits()/sram2_clr_bits() helpers ... and use the named
OCP_POWER_CFG/EN_10M_PLLOFF/EN_ALDPS_PLLOFF macros".

Can the "shorten CLKS latency" and "CMP_Timer on MP_Timer=333" programming be
mentioned in the commit message as well?

>  
>  		/* AFE power saving for 2.5G & 5G */
>  		sram_write(tp, 0x8173, 0x8620);
>  		sram_write(tp, 0x8175, 0x8671);
>  
> -		sram_write_w0w1(tp, 0x817c, 0, BIT(13));
> -		sram_write_w0w1(tp, 0x8187, 0, BIT(13));
> -		sram_write_w0w1(tp, 0x8192, 0, BIT(13));
> -		sram_write_w0w1(tp, 0x819d, 0, BIT(13));
> -		sram_write_w0w1(tp, 0x81a8, BIT(13), 0);
> -		sram_write_w0w1(tp, 0x81b3, BIT(13), 0);
> -		sram_write_w0w1(tp, 0x81be, 0, BIT(13));
> +		sram_set_bits(tp, 0x817c, BIT(13));
> +		sram_set_bits(tp, 0x8187, BIT(13));
> +		sram_set_bits(tp, 0x8192, BIT(13));
> +		sram_set_bits(tp, 0x819d, BIT(13));
> +		sram_clr_bits(tp, 0x81a8, BIT(13));
> +		sram_clr_bits(tp, 0x81b3, BIT(13));
> +		sram_set_bits(tp, 0x81be, BIT(13));
>  
>  		sram_write_w0w1(tp, 0x817d, 0xff00, 0xa600);
>  		sram_write_w0w1(tp, 0x8188, 0xff00, 0xa600);
> @@ -8351,10 +8451,10 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)
>  		sram2_write_w0w1(tp, 0x84b2, 0xff00, 0x6000);
>  		/* Training AAGC PAR (with uc2 patch) */
>  		sram2_write(tp, 0x8ffc, 0x6008);
> -		sram2_write(tp, 0x8ffe, 0xf450);
> +		sram2_write(tp, 0x8ffe, 0xf4ff);

[Severity: Medium]
The neighbouring lines in this block are pure sram2_set_bits()/
sram2_clr_bits() conversions, but this one changes the value written to
0x8ffe from 0xf450 to 0xf4ff.  Is this value change intended, and can it be
called out in the commit message?

>  		/* DAC BGK */
> -		sram2_write_w0w1(tp, 0x8015, 0, BIT(9));
> -		sram2_write_w0w1(tp, 0x8016, 0, BIT(11));
> +		sram2_set_bits(tp, 0x8015, BIT(9));
> +		sram2_set_bits(tp, 0x8016, BIT(11));

[ ... ]

> @@ -8440,8 +8540,15 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)
>  		sram2_write(tp, 0x8ff8, 0xaa5a);
>  
>  		sram2_write_w0w1(tp, 0x88d5, 0xff00, 0x0200);
> -		break;
>  
> +		/* spdchg_pga1_lpf_cap */
> +		sram_write_w0w1(tp, 0x84bb, 0xff00, 0x0a00);
> +		sram_write_w0w1(tp, 0x84c0, 0xff00, 0x1600);
> +
> +		/* ENET PLL jitter improvement */
> +		ocp_reg_w0w1(tp, 0xbf8a, 0xfc00, 0x2000);
> +		ocp_reg_set_bits(tp, 0xbf88, BIT(2));
> +		break;

[Severity: Medium]
The spdchg_pga1_lpf_cap writes (0x84bb, 0x84c0) and the ENET PLL jitter
writes (0xbf8a, 0xbf88) are also new, with no counterpart removal in the
diff.  Should the commit message list these among the RTL8159 changes too?

>  	default:
>  		break;
>  	}

[ ... ]

> @@ -10264,7 +10371,7 @@ static int rtl_ops_init(struct r8152 *tp)
>  		ops->eee_get		= r8153_get_eee;
>  		ops->eee_set		= r8152_set_eee;
>  		ops->in_nway		= rtl8153_in_nway;
> -		ops->hw_phy_cfg		= r8157_hw_phy_cfg;
> +		ops->hw_phy_cfg		= r8159_hw_phy_cfg;
>  		ops->autosuspend_en	= rtl8157_runtime_enable;
>  		ops->change_mtu		= rtl8156_change_mtu;
>  		tp->rx_buf_sz		= 48 * 1024;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917070637.62827-1-nic_swsd%40realtek.com

Re: [PATCH net-next v4 5/8] r8152: add rtl8157_unload and rtl8157_change_mtu

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Commit message / Fixes: tag mismatch for a substantive
  hardware-programming correction.

--

r8152: add rtl8157_unload and rtl8157_change_mtu

Adds rtl8157_unload(), which uses r8157_power_cut_en() plus the
Interrupt Mitigation disable that rtl8153_unload() previously gated on
"tp->version >= RTL_VER_16", and rtl8157_change_mtu(), which scales
PLA_MTPS from the configured MTU. Both are wired to RTL8157 and RTL8159
through ops->unload and ops->change_mtu.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index d30031dbf2d38..f44dcb88b5c6a 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -7004,6 +7004,28 @@ static void rtl8156_down(struct r8152 *tp)
>  	r8153_aldps_en(tp, true);
>  }
>  
> +static void rtl8157_change_mtu(struct r8152 *tp)
> +{
> +	u32 max_pkt_size = mtu_to_size(tp->netdev->mtu);
> +	u32 ocp_data;
> +
> +	ocp_write_word(tp, MCU_TYPE_PLA, PLA_RMS, max_pkt_size);
> +
> +	/* Use at least 10K for MTPS */
> +	ocp_data = max_t(u32, max_pkt_size, 10 * 1024) / 64;
> +
> +	/* 16 * 1024 / 64 = 0x100, so the max is 0xff for 8 bits data */
> +	ocp_data = min_t(u32, ocp_data, 0xff);
> +
> +	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_MTPS, ocp_data);
> +	r8156_fc_parameter(tp);
> +
> +	/* TX share fifo free credit full threshold */
> +	ocp_write_word(tp, MCU_TYPE_PLA, PLA_TXFIFO_CTRL, 512 / 64);
> +	ocp_write_word(tp, MCU_TYPE_PLA, PLA_TXFIFO_FULL,
> +		       ALIGN(max_pkt_size + tp->tx_desc.size, 1024) / 16);
> +}
> +

[ ... ]

> @@ -10151,11 +10173,6 @@ static void rtl8153_unload(struct r8152 *tp)
>  		return;
>  
>  	r8153_power_cut_en(tp, false);
> -
> -	if (tp->version >= RTL_VER_16) {
> -		/* Disable Interrupt Mitigation */
> -		ocp_byte_clr_bits(tp, MCU_TYPE_USB, 0xcf04, BIT(0) | BIT(1) | BIT(2) | BIT(7));
> -	}
>  }

[ ... ]

> +static void rtl8157_unload(struct r8152 *tp)
> +{
> +	if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
> +		return;
> +
> +	r8157_power_cut_en(tp, false);
> +	ocp_byte_clr_bits(tp, MCU_TYPE_USB, 0xcf04,
> +			  BIT(0) | BIT(1) | BIT(2) | BIT(7));
> +}

[ ... ]

> @@ -10344,13 +10371,13 @@ static int rtl_ops_init(struct r8152 *tp)
>  		ops->disable		= rtl8153_disable;
>  		ops->up			= rtl8157_up;
>  		ops->down		= rtl8157_down;
> -		ops->unload		= rtl8153_unload;
> +		ops->unload		= rtl8157_unload;
>  		ops->eee_get		= r8153_get_eee;
>  		ops->eee_set		= r8152_set_eee;
>  		ops->in_nway		= rtl8153_in_nway;
>  		ops->hw_phy_cfg		= r8157_hw_phy_cfg;
>  		ops->autosuspend_en	= rtl8157_runtime_enable;
> -		ops->change_mtu		= rtl8156_change_mtu;
> +		ops->change_mtu		= rtl8157_change_mtu;
>  		tp->rx_buf_sz		= 32 * 1024;
>  		tp->support_2500full	= 1;
>  		tp->support_5000full	= 1;

[Severity: Medium]
The subject and changelog read as purely additive, but the two ops
rewirings here (and the identical pair in the RTL_VER_17_QFN68 and
RTL_VER_17_QFN100 branches further down) change register programming for
hardware that is already supported in released kernels. Would a Fixes:
tag and a note about the user-visible symptom help here, at least
fd3c7d080df5 ("r8152: Add support for the RTL8157 hardware") plus the
RTL8159 support commit?

Two concrete deltas, first the power-cut sequence. rtl8153_unload() used:

	r8153_power_cut_en(tp, false)
	    ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_POWER_CUT, PWR_EN | PHASE2_EN);
	    ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_MISC_0, PCUT_STATUS);

while rtl8157_unload() now uses:

	r8157_power_cut_en(tp, false)
	    ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_POWER_CUT, PWR_EN);
	    ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_MISC_0, PCUT_STATUS);
	    ocp_byte_clr_bits(tp, MCU_TYPE_USB, USB_MISC_2, BIT(1));

So PHASE2_EN is now left set and USB_MISC_2 BIT(1) is now cleared. These
writes really execute on unbind, since rtl8152_disconnect() calls
rtl_set_unplug() first but rtl_set_unplug() only sets
RTL8152_INACCESSIBLE when tp->udev->state == USB_STATE_NOTATTACHED, and
they also execute on the probe error path. Was the old sequence leaving
the device in a bad state after power-cut or resume?

Second, PLA_MTPS at the default MTU. rtl8156_change_mtu() wrote
MTPS_JUMBO (12 * 1024 / 64 = 192) unconditionally, while
rtl8157_change_mtu() writes max_t(u32, mtu_to_size(1500), 10 * 1024) / 64
= 160 for a 1500 byte MTU. Every RTL8157/RTL8159 user therefore gets a
different TX threshold after this patch, both from
rtl8157_up() -> rtl8157_change_mtu() and from ndo_change_mtu. Is that
intentional for existing setups, and is it something that should be
backported?

Also on the changelog wording:

    and an MTPS scaled from the configured MTU (10K-16K, max_pkt_size /
    64 capped at 0xff) instead of a fixed MTPS_JUMBO/MTPS_DEFAULT.

The replaced callback, rtl8156_change_mtu(), only ever writes MTPS_JUMBO.
MTPS_DEFAULT is written by rtl8156_down(), which these chips no longer
use after the earlier split into per-chip-family functions; rtl8157_down()
already writes 10 * 1024 / 64. Could the description be adjusted so it
matches the code being replaced?

Neither this message nor any later patch in the series changes
rtl8157_unload()/rtl8157_change_mtu(), so the tags and description would
need to be corrected here.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917070637.62827-1-nic_swsd%40realtek.com

Re: [PATCH net-next v4 6/8] r8152: add TGPHY register access for RTL8157 and RTL8159

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 12 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 8 · Low: 3

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] r8152_mdio_read() now returns negative errno values (`return ret
  < 0 ?
- [Medium] The commit message presents the TGPHY path as infrastructure
  needed for chip support ("Support for these chips needs a second PHY…
- [Medium] The changelog claims that "every helper built on top of
  [ocp_reg_read/write] ... [is] propagating the failure to callers…
- [Medium] The RTL8157/RTL8159 PHY configuration routines and the PHY
  firmware appliers issue long dependent indirect-access sequences…
- [Medium] rtl8152_set_speed() and rtl8152_set_pauseparam() add read-
  error exits but leave the PHY writes unchecked, so configuration is…
- [Medium] r8153_phy_status() now breaks out of its poll loop when
  ocp_reg_read(OCP_PHY_STATUS, &data) fails and returns the untouched…
- [Medium] rtl_phy_patch_request() implements the driver<->PHY-MCU
  mutual-exclusion handshake (PATCH_REQUEST / PATCH_READY), but the two…
- [Medium] rtl_tgphy_access() builds the new PHY transaction out of
  status-less accessors, so bus-level failures become 'successful' PHY…
- [Medium] r8156_eee_en() gained `if (ocp_reg_read(tp, OCP_EEE_ADV2,
  &config) < 0) return;` after r8153_eee_en(tp, enable) has already…
- [Low] sram_write() is converted to return int but only captures the
  status of the address-phase write; the data-phase ocp_reg_write(tp,…
- [Low] rtl_phy_patch_request() now performs an unconditional
  ocp_reg_read(OCP_PHY_PATCH_STAT) after the wait loop.
- [Low] rtl_ram_code_speed_up() and rtl8152_fw_phy_ver() ignore
  sram_read()'s new error return while pre-initialising the version…

--

r8152: add TGPHY register access for RTL8157 and RTL8159

This turns ocp_reg_read()/ocp_reg_write() into dispatch wrappers over new
tp->rtl_ops.phy_read/phy_write pointers and adds a TGPHY mailbox
implementation (USB_TGPHY_ADDR/DATA/CMD) for RTL8157 and RTL8159.  Because
the new path can time out, the PHY helper layer is converted from
void/raw-value returns to int.

A few questions below about the new error paths and about the changelog.

[Severity: Medium]

> Since ocp_reg_read()/ocp_reg_write() can now fail on a TGPHY timeout,
> every helper built on top of them (sram_read/write, sram2_read/write,
> the ocp_reg_*_bits and MDIO bit helpers) is converted from void/raw-
> value returns to int, propagating the failure to callers instead of
> silently operating on stale data.

Is "propagating the failure to callers" accurate for all of these?  A few
places still swallow or fabricate:

  - sram_write() returns the status of the OCP_SRAM_ADDR write only, the
    OCP_SRAM_DATA write status is dropped.

  - r8153_phy_status() and r8152_mmd_read() return a zero-initialised u16
    on failure, which callers cannot distinguish from a real value.

  - rtl_ram_code_speed_up() and rtl8152_fw_phy_ver() ignore sram_read()
    entirely.

  - the chips that can actually fail here, RTL_VER_16 and RTL_VER_17, run
    r8157_hw_phy_cfg()/r8159_hw_phy_cfg(), which ignore every return
    value.

  - rtl8152_set_speed(), rtl8152_set_pauseparam() and rtl8152_ioctl()
    ignore the write errors and, for SIOCGMIIREG, the read error.

Would it be clearer to scope the sentence to the helpers that were
actually converted?

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index f44dcb88b5c6a..3473061af129a 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

> @@ -1664,16 +1675,33 @@ static void ocp_reg_write(struct r8152 *tp, u16 addr, u16 data)
>  
>  	ocp_index = (addr & 0x0fff) | 0xb000;
>  	ocp_write_word(tp, MCU_TYPE_PLA, ocp_index, data);
> +
> +	return 0;
> +}
> +
> +static int ocp_reg_read(struct r8152 *tp, u16 addr, u16 *data)
> +{
> +	return tp->rtl_ops.phy_read(tp, addr, data);
> +}
> +
> +static int ocp_reg_write(struct r8152 *tp, u16 addr, u16 data)
> +{
> +	return tp->rtl_ops.phy_write(tp, addr, data);
>  }
>  
> -static inline void r8152_mdio_write(struct r8152 *tp, u32 reg_addr, u32 value)
> +static inline int r8152_mdio_write(struct r8152 *tp, u32 reg_addr, u32 value)
>  {
> -	ocp_reg_write(tp, OCP_BASE_MII + reg_addr * 2, value);
> +	return ocp_reg_write(tp, OCP_BASE_MII + reg_addr * 2, value);
>  }
>  
>  static inline int r8152_mdio_read(struct r8152 *tp, u32 reg_addr)
>  {
> -	return ocp_reg_read(tp, OCP_BASE_MII + reg_addr * 2);
> +	u16 data;
> +	int ret;
> +
> +	ret = ocp_reg_read(tp, OCP_BASE_MII + reg_addr * 2, &data);
> +
> +	return ret < 0 ? ret : data;
>  }

[Severity: High]

r8152_mdio_read() now mixes errno values and register contents in the same
int, but the consumers still decode the result as PHY register bits.
read_mii_word() is wired up as tp->mii.mdio_read, and the mii library has
no error channel:

drivers/net/mii.c:mii_nway_restart() {
	bmcr = mii->mdio_read(mii->dev, mii->phy_id, MII_BMCR);

	if (bmcr & BMCR_ANENABLE) {
		bmcr |= BMCR_ANRESTART;
		mii->mdio_write(mii->dev, mii->phy_id, MII_BMCR, bmcr);
	...
}

On a TGPHY busy timeout bmcr becomes 0xff92, which has BMCR_ANENABLE set,
so 0xff92 gets written back into MII_BMCR.  That value also carries
BMCR_RESET, BMCR_LOOPBACK, BMCR_PDOWN and BMCR_ISOLATE.

Can this power down or isolate the PHY?  The path is reachable from
rtl8152_nway_reset() and rtl8152_set_eee().  mii_ethtool_get_link_ksettings()
similarly stores the callback results in u16 bmcr/bmsr and decodes their
bits with no error check.

In rtl8152_ioctl(), SIOCGMIIREG requires no capability and does:

	data->val_out = r8152_mdio_read(tp, data->reg_num);

with res left at 0, so userspace receives the truncated errno (-110 ->
0xff92) as a PHY register value; SIOCSMIIREG drops r8152_mdio_write()'s
new error the same way.  rtl8152_get_pauseparam() puts the three reads
into u16 locals and bit-tests them, so 0xff92 reports autoneg as enabled.

And in rtl8152_set_speed() the reset poll:

		if ((r8152_mdio_read(tp, MII_BMCR) & BMCR_RESET) == 0)
			break;

can never break early, since every negative return has bit 15 set, so it
spins 50 * 20 ms with tp->control held.  Should these callers be taught
about the negative return in the same patch?

>  
>  static int wait_cmd_ready(struct r8152 *tp, u16 cmd)
> @@ -1792,16 +1820,32 @@ static int rtl_ip_set_bits(struct r8152 *tp, u16 addr, u32 set)
>  	return rtl_ip_w0w1(tp, addr, 0, set);
>  }
>  
> -static void sram_write(struct r8152 *tp, u16 addr, u16 data)
> +static int sram_write(struct r8152 *tp, u16 addr, u16 data)
>  {
> -	ocp_reg_write(tp, OCP_SRAM_ADDR, addr);
> +	int ret;
> +
> +	ret = ocp_reg_write(tp, OCP_SRAM_ADDR, addr);
> +	if (ret < 0)
> +		goto out;
> +
>  	ocp_reg_write(tp, OCP_SRAM_DATA, data);
> +
> +out:
> +	return ret;
>  }

[Severity: Low]

Is dropping the OCP_SRAM_DATA write status here intentional?  sram_write()
returns the (successful) OCP_SRAM_ADDR status, so a TGPHY timeout on the
data phase is reported to callers as success.  sram2_write(), converted in
this same patch, does capture it:

	ret = ocp_reg_write(tp, OCP_SRAM2_ADDR, addr);
	if (ret < 0)
		goto out;

	ret = ocp_reg_write(tp, OCP_SRAM2_DATA, data);

No current caller looks at sram_write()'s return value, so there is no
visible effect today.

>  
> -static u16 sram_read(struct r8152 *tp, u16 addr)
> +static int sram_read(struct r8152 *tp, u16 addr, u16 *data)
>  {
> -	ocp_reg_write(tp, OCP_SRAM_ADDR, addr);
> -	return ocp_reg_read(tp, OCP_SRAM_DATA);
> +	int ret;
> +
> +	ret = ocp_reg_write(tp, OCP_SRAM_ADDR, addr);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = ocp_reg_read(tp, OCP_SRAM_DATA, data);
> +
> +out:
> +	return ret;
>  }

[ ... ]

> @@ -1906,100 +1950,217 @@ static void ocp_byte_set_bits(struct r8152 *tp, u16 type, u16 index, u8 set)

[ ... ]

> +static int wait_tgphy_cmd_ready(struct r8152 *tp)
> +{
> +	u16 ocp_data;
> +	int ret;
> +
> +	ret = read_poll_timeout(ocp_read_word, ocp_data,
> +				test_bit(RTL8152_INACCESSIBLE, &tp->flags) ||
> +				!(ocp_data & TGPHY_CMD_BUSY),
> +				2000, 20000, false, tp,
> +				MCU_TYPE_USB, USB_TGPHY_CMD);
> +
> +	if (ret)
> +		dev_err(&tp->intf->dev, "TGPHY cmd busy timeout\n");
> +
> +	return test_bit(RTL8152_INACCESSIBLE, &tp->flags) ? -ENODEV : ret;
> +}
> +
> +static int rtl_tgphy_access(struct r8152 *tp, u16 addr, u16 *data, bool write)
> +{
> +	u16 cmd = 0;
> +	int ret;
> +
> +	ret = wait_tgphy_cmd_ready(tp);
> +	if (ret < 0)
> +		goto out;

[Severity: Medium]

When this busy-wait aborts, the requested access is skipped but the
indirect SRAM programming state is left as it was.  The RTL8157/RTL8159
configuration routines then keep issuing data writes against whatever
address was latched last.  For example r8157_hw_phy_cfg() does:

	ocp_reg_write(tp, OCP_SRAM_ADDR, 0x8f9c);
	ocp_reg_write(tp, OCP_SRAM_DATA, 0x0005);
	ocp_reg_write(tp, OCP_SRAM_DATA, 0x0000);
	...

and r8159_hw_phy_cfg() the same for 0x81c4, while the firmware appliers do:

	ocp_reg_write(tp, OCP_SRAM_ADDR, __le16_to_cpu(phy->fw_reg));
	for (i = 0; i < num; i++)
		ocp_reg_write(tp, OCP_SRAM_DATA, __le16_to_cpu(data[i]));

If the address write aborts on a busy timeout but a data write a few
microseconds later finds the mailbox free, can these words land on the
previously latched SRAM address?  And since successful data writes
auto-advance the destination, would a skipped word shift every following
word of the image?  Both functions continue on to the breakpoint writes,
update the cached ups_info state and finally assert PHY_RESET as if the
whole sequence had been programmed.

> +
> +	if (write) {
> +		cmd |= TGPHY_CMD_WR;
> +		ocp_write_word(tp, MCU_TYPE_USB, USB_TGPHY_DATA, *data);
> +	}
> +
> +	ocp_write_word(tp, MCU_TYPE_USB, USB_TGPHY_ADDR, addr);
> +
> +	cmd |= TGPHY_CMD_BUSY;
> +	ocp_write_word(tp, MCU_TYPE_USB, USB_TGPHY_CMD, cmd);
> +
> +	if (!write) {
> +		ret = wait_tgphy_cmd_ready(tp);
> +		if (ret < 0)
> +			goto out;
> +
> +		*data = ocp_read_word(tp, MCU_TYPE_USB, USB_TGPHY_DATA);
> +	}
> +
> +out:
> +	return ret;
> +}

[Severity: Medium]

The four accesses that make up the transaction all use the status-less
accessors, so bus-level failures come back as successful PHY accesses.
Two cases:

If the USB_TGPHY_DATA or USB_TGPHY_ADDR control transfer fails,
r8152_control_msg() sets RTL8152_INACCESSIBLE and the remaining writes
become -ENODEV no-ops, yet rtl_tgphy_access() still returns 0.  Should the
success path re-check RTL8152_INACCESSIBLE before returning?

set_registers() and get_registers() can also return early, before
r8152_control_msg() runs at all:

	tmp = kmemdup(data, size, GFP_KERNEL);
	if (!tmp)
		return -ENOMEM;

In that case RTL8152_INACCESSIBLE is not set, so the USB_TGPHY_CMD write
can launch a command whose operands were never delivered.

On the read side, get_registers() does:

	if (ret < 0)
		memset(data, 0xff, size);
	else
		memcpy(data, tmp, size);

so *data becomes 0xffff after a failed transfer, and on its -ENOMEM path
ocp_read_word()'s local tmp is never written at all.  Does the read then
publish that value with the success status of the preceding busy poll?

> +
> +static int r8157_phy_read(struct r8152 *tp, u16 addr, u16 *data)
> +{
> +	return rtl_tgphy_access(tp, addr, data, false);
> +}
> +
> +static int r8157_phy_write(struct r8152 *tp, u16 addr, u16 data)
> +{
> +	return rtl_tgphy_access(tp, addr, &data, true);
>  }
>  
>  static int
> @@ -4177,11 +4338,13 @@ static void r8153b_green_en(struct r8152 *tp, bool enable)
>  
>  static u16 r8153_phy_status(struct r8152 *tp, u16 desired)
>  {
> -	u16 data;
> +	u16 data = 0;
>  	int i;
>  
>  	for (i = 0; i < 500; i++) {
> -		data = ocp_reg_read(tp, OCP_PHY_STATUS);
> +		if (ocp_reg_read(tp, OCP_PHY_STATUS, &data) < 0)
> +			break;
> +
>  		data &= PHY_STAT_MASK;
>  		if (desired) {
>  			if (data == desired)

[Severity: Medium]

Since rtl_tgphy_access() leaves *data untouched on a busy-wait failure,
this break returns the initial 0 as an ordinary PHY status, and the u16
signature gives callers no way to tell the difference.  0 matches none of
PHY_STAT_LAN_ON, PHY_STAT_PWRDN or PHY_STAT_EXT_INIT, so in
r8156_hw_phy_cfg(), r8157_hw_phy_cfg() and r8159_hw_phy_cfg():

	data = r8153_phy_status(tp, 0);
	switch (data) {
	...
	default:
		rtl8152_apply_firmware(tp, false);

the whole PHY patch sequence runs against a PHY that is known to be
unreachable.  A little later the same functions do:

	data = r8153_phy_status(tp, PHY_STAT_LAN_ON);
	WARN_ON_ONCE(data != PHY_STAT_LAN_ON);

Would a plain TGPHY timeout now produce a WARN_ON_ONCE splat here?  Should
r8153_phy_status() gain an error return instead?

[ ... ]

> @@ -4587,7 +4750,8 @@ static inline void rtl_reset_ocp_base(struct r8152 *tp)
>  static int rtl_phy_patch_request(struct r8152 *tp, bool request, bool wait)
>  {
>  	u16 check;
> -	int i;
> +	u16 ocp_data = 0;
> +	int i, ret;
>  
>  	if (request) {
>  		ocp_reg_set_bits(tp, OCP_PHY_PATCH_CMD, PATCH_REQUEST);

[Severity: Medium]

These two calls are the ones that actually take and drop the driver/PHY-MCU
handshake bit, and they still discard the newly added error return:

	if (request) {
		ocp_reg_set_bits(tp, OCP_PHY_PATCH_CMD, PATCH_REQUEST);
		check = 0;
	} else {
		ocp_reg_clr_bits(tp, OCP_PHY_PATCH_CMD, PATCH_REQUEST);
		check = PATCH_READY;
	}

With wait == false the polling loop is skipped and the function reaches
"return 0" unconditionally, so a failed acquire is indistinguishable from
success.  That path is reachable via rtl8152_apply_firmware(tp, true) ->
rtl_pre_ram_code(..., wait = !power_cut = false), and the callers use the
zero as permission to proceed:

	if (rtl_phy_patch_request(tp, true, wait))
		return -ETIME;

	rtl_patch_key_set(tp, key_addr, patch_key);

Can PHY RAM code then be streamed while the PHY MCU still owns the PHY?
Symmetrically, a timed-out release leaves PATCH_REQUEST asserted while 0 is
returned.

>  
> @@ -4598,25 +4762,23 @@ static int rtl_phy_patch_request(struct r8152 *tp, bool request, bool wait)
>  	}
>  
>  	for (i = 0; wait && i < 5000; i++) {
> -		u32 ocp_data;
> -
>  		if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
>  			return -ENODEV;
>  
>  		usleep_range(1000, 2000);
> -		ocp_data = ocp_reg_read(tp, OCP_PHY_PATCH_STAT);
> -		if ((ocp_data & PATCH_READY) ^ check)
> +		ret = ocp_reg_read(tp, OCP_PHY_PATCH_STAT, &ocp_data);
> +		if (ret < 0 || (ocp_data & PATCH_READY) ^ check)
>  			break;
>  	}
>  
> -	if (request && wait &&
> -	    !(ocp_reg_read(tp, OCP_PHY_PATCH_STAT) & PATCH_READY)) {
> +	ret = ocp_reg_read(tp, OCP_PHY_PATCH_STAT, &ocp_data);
> +	if (request && wait && (ret < 0 || !(ocp_data & PATCH_READY))) {
>  		dev_err(&tp->intf->dev, "PHY patch request fail\n");
>  		rtl_phy_patch_request(tp, false, false);
>  		return -ETIME;
> -	} else {
> -		return 0;
>  	}
> +
> +	return 0;
>  }

[Severity: Low]

Hoisting the read out of the condition makes it unconditional.  Previously
the short-circuit meant no register access at all when request == false
(rtl_post_ram_code(), the release in rtl_ram_code_speed_up(), and the
internal rtl_phy_patch_request(tp, false, false) rollback) or when
wait == false.  Now every one of those calls costs an extra PHY read, which
on the TGPHY chips is a busy poll plus a command round trip.  Was that
intended?

Also, a read failure is now folded into -ETIME with the "PHY patch request
fail" message rather than reporting the underlying -ENODEV or -ETIMEDOUT.
Should ret be returned directly in that case?

>  
>  static void rtl_patch_key_set(struct r8152 *tp, u16 key_addr, u16 patch_key)
> @@ -5331,10 +5493,12 @@ static void rtl_ram_code_speed_up(struct r8152 *tp, struct fw_phy_speed_up *phy,
>  {
>  	u32 len;
>  	u8 *data;
> +	u16 ver = 0;
>  
>  	rtl_reset_ocp_base(tp);
>  
> -	if (sram_read(tp, SRAM_GPHY_FW_VER) >= __le16_to_cpu(phy->version)) {
> +	sram_read(tp, SRAM_GPHY_FW_VER, &ver);
> +	if (ver >= __le16_to_cpu(phy->version)) {
>  		dev_dbg(&tp->intf->dev, "PHY firmware has been the newest\n");
>  		return;
>  	}

[Severity: Low]

With sram_read()'s return value ignored and ver pre-set to 0, a TGPHY read
failure looks identical to "the running PHY firmware is older", so the
update branch is taken.  Before this change a failed read produced 0xffff
from the 0xff fill in get_registers(), which took the skip path, so the
zero initialisation inverts the outcome.  rtl8152_fw_phy_ver() has the same
shape with cur_ver = 0, and there it also decides patch_phy for the rest of
the image.  Would checking sram_read() and bailing out be preferable?

[ ... ]

> @@ -5389,14 +5555,15 @@ static void rtl_ram_code_speed_up(struct r8152 *tp, struct fw_phy_speed_up *phy,
>  
>  static int rtl8152_fw_phy_ver(struct r8152 *tp, struct fw_phy_ver *phy_ver)
>  {
> -	u16 ver_addr, ver;
> +	u16 ver_addr, ver, cur_ver = 0;
>  
>  	ver_addr = __le16_to_cpu(phy_ver->ver.addr);
>  	ver = __le16_to_cpu(phy_ver->ver.data);
>  
>  	rtl_reset_ocp_base(tp);
>  
> -	if (sram_read(tp, ver_addr) >= ver) {
> +	sram_read(tp, ver_addr, &cur_ver);
> +	if (cur_ver >= ver) {
>  		dev_dbg(&tp->intf->dev, "PHY firmware has been the newest\n");
>  		return 0;
>  	}

[ ... ]

> @@ -5787,7 +5955,8 @@ static void r8156_eee_en(struct r8152 *tp, bool enable)
>  
>  	r8153_eee_en(tp, enable);
>  
> -	config = ocp_reg_read(tp, OCP_EEE_ADV2);
> +	if (ocp_reg_read(tp, OCP_EEE_ADV2, &config) < 0)
> +		return;
>  
>  	if (enable && (tp->eee_adv2 & MDIO_EEE_2_5GT))
>  		config |= MDIO_EEE_2_5GT;

[Severity: Medium]

This early return lands after r8153_eee_en() has already toggled
PLA_EEE_CR EEE_RX_EN/EEE_TX_EN and OCP_EEE_CFG EEE10_EN and set
tp->ups_info.eee, so OCP_EEE_ADV2 keeps its previous 2.5G/5G advertisement
with no rollback.  Since r8156_eee_en() is void, rtl_eee_enable() cannot
see the failure and still writes OCP_EEE_ADV:

	case RTL_VER_16:
	case RTL_VER_17_QFN68:
	case RTL_VER_17_QFN100:
		if (enable) {
			r8156_eee_en(tp, true);
			ocp_reg_write(tp, OCP_EEE_ADV, tp->eee_adv);

Those are exactly the versions whose phy_read is r8157_phy_read(), so can
the MAC/PHY enables, tp->ups_info.eee (which feeds USB_UPS_FLAGS) and the
multi-gig advertisement end up describing different EEE configurations?

[ ... ]

> @@ -6601,7 +6770,11 @@ static int rtl8152_set_speed(struct r8152 *tp, u8 autoneg, u32 speed, u8 duplex,
>  		if (!advertising)
>  			return -EINVAL;
>  
> -		orig = r8152_mdio_read(tp, MII_ADVERTISE);
> +		ret = r8152_mdio_read(tp, MII_ADVERTISE);
> +		if (ret < 0)
> +			goto out;
> +
> +		orig = ret;
>  		new1 = orig & ~(ADVERTISE_10HALF | ADVERTISE_10FULL |
>  				ADVERTISE_100HALF | ADVERTISE_100FULL);
>  		if (advertising & RTL_ADVERTISED_10_HALF) {
> @@ -6628,7 +6801,11 @@ static int rtl8152_set_speed(struct r8152 *tp, u8 autoneg, u32 speed, u8 duplex,
>  		}
>  
>  		if (tp->mii.supports_gmii) {
> -			orig = r8152_mdio_read(tp, MII_CTRL1000);
> +			ret = r8152_mdio_read(tp, MII_CTRL1000);
> +			if (ret < 0)
> +				goto out;
> +
> +			orig = ret;
>  			new1 = orig & ~(ADVERTISE_1000FULL |
>  					ADVERTISE_1000HALF);
>  

[Severity: Medium]

By the time this MII_CTRL1000 read can fail, MII_ADVERTISE has already been
rewritten just above:

		if (orig != new1) {
			r8152_mdio_write(tp, MII_ADVERTISE, new1);
			tp->mii.advertising = new1;
		}

The goto out skips the MII_BMCR write, so does the PHY end up advertising
new 10/100 capabilities mixed with the old 1000/2.5G/5G/10G ones and no
ANRESTART, while tp->mii.advertising and tp->ups_info.speed_duplex have
already been updated?  The same applies to the OCP_10GBT_CTRL read below.

>  @@ -6642,7 +6819,10 @@ static int rtl8152_set_speed(struct r8152 *tp, u8 autoneg, u32 speed, u8 duplex,
>  		}
>  
>  		if (tp->support_2500full || tp->support_5000full || tp->support_10000full) {
> -			orig = ocp_reg_read(tp, OCP_10GBT_CTRL);
> +			ret = ocp_reg_read(tp, OCP_10GBT_CTRL, &orig);
> +			if (ret < 0)
> +				goto out;
> +
>  			new1 = orig & ~(MDIO_AN_10GBT_CTRL_ADV2_5G | MDIO_AN_10GBT_CTRL_ADV5G
>  					| MDIO_AN_10GBT_CTRL_ADV10G);
>  
> @@ -6686,7 +6866,7 @@ static int rtl8152_set_speed(struct r8152 *tp, u8 autoneg, u32 speed, u8 duplex,
>  	}
>  
>  out:
> -	return ret;
> +	return ret < 0 ? ret : 0;
>  }

[Severity: Medium]

The final r8152_mdio_write(tp, MII_BMCR, bmcr) result is not captured, and
this normalisation turns the last successful read into 0.  If the BMCR
write times out, does the function report success even though
autonegotiation was never restarted and a requested BMCR_RESET was already
consumed by test_and_clear_bit(PHY_RESET)?  rtl8152_set_link_ksettings()
then caches the request as applied:

	ret = rtl8152_set_speed(tp, cmd->base.autoneg, cmd->base.speed,
				cmd->base.duplex, advertising);
	if (!ret) {
		tp->autoneg = cmd->base.autoneg;

[ ... ]

> @@ -10002,7 +10216,11 @@ static int rtl8152_set_pauseparam(struct net_device *netdev, struct ethtool_paus
>  
>  	mutex_lock(&tp->control);
>  
> -	if (pause->autoneg && !(r8152_mdio_read(tp, MII_BMCR) & BMCR_ANENABLE)) {
> +	ret = r8152_mdio_read(tp, MII_BMCR);
> +	if (ret < 0)
> +		goto out;
> +
> +	if (pause->autoneg && !(ret & BMCR_ANENABLE)) {
>  		ret = -EINVAL;
>  		goto out;
>  	}
> @@ -10013,7 +10231,11 @@ static int rtl8152_set_pauseparam(struct net_device *netdev, struct ethtool_paus
>  	if (pause->tx_pause)
>  		cap |= FLOW_CTRL_TX;
>  
> -	old = r8152_mdio_read(tp, MII_ADVERTISE);
> +	ret = r8152_mdio_read(tp, MII_ADVERTISE);
> +	if (ret < 0)
> +		goto out;
> +
> +	old = ret;
>  	new1 = (old & ~(ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM)) | mii_advertise_flowctrl(cap);
>  	if (old != new1)
>  		r8152_mdio_write(tp, MII_ADVERTISE, new1);
> @@ -10022,7 +10244,7 @@ static int rtl8152_set_pauseparam(struct net_device *netdev, struct ethtool_paus
>  	mutex_unlock(&tp->control);
>  	usb_autopm_put_interface(tp->intf);
>  
> -	return ret;
> +	return ret < 0 ? ret : 0;
>  }

[Severity: Medium]

Here the reads are checked but the MII_ADVERTISE write status is dropped
and then normalised to 0, so ethtool -A reports success for a pause
advertisement that was never programmed.  Would assigning the write result
to ret before the out label match the set_pauseparam contract better?

[ ... ]

> @@ -10378,6 +10612,8 @@ static int rtl_ops_init(struct r8152 *tp)
>  		ops->hw_phy_cfg		= r8157_hw_phy_cfg;
>  		ops->autosuspend_en	= rtl8157_runtime_enable;
>  		ops->change_mtu		= rtl8157_change_mtu;
> +		ops->phy_read		= r8157_phy_read;
> +		ops->phy_write		= r8157_phy_write;
>  		tp->rx_buf_sz		= 32 * 1024;
>  		tp->support_2500full	= 1;
>  		tp->support_5000full	= 1;

[Severity: Medium]

The changelog says "Support for these chips needs a second PHY access
path", but RTL_VER_16 (RTL8157) has been supported in-tree since
fd3c7d080df5 ("r8152: Add support for the RTL8157 hardware") and RTL_VER_17
since the RTL8159 support commit, and both already ran their PHY accesses
through the OCP-base-switching path.

If the premise is right and these chips only reach their PHY through
USB_TGPHY_ADDR/DATA/CMD, then every PHY access on shipping hardware since
those commits used the wrong interface: the ocp_reg_write() parameter
writes in r8157_hw_phy_cfg()/r8159_hw_phy_cfg(), r8153_phy_status()
polling, the EEE and 10GBT reporting in rtl8152_get_link_ksettings() and
r8153_get_eee(), the advertisement programming in rtl8152_set_speed(),
SIOCGMIIREG/SIOCSMIIREG and rtl8153_in_nway().

That would make this a user-visible fix for released hardware, but there is
no Fixes: tag, no stable Cc and no description of the observed symptom.  If
instead the PLA_OCP_GPHY_BASE window did work on these chips, could the
justification for the second path be restated?  As written it is hard to
tell which case applies for backporting.

>  
[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917070637.62827-1-nic_swsd%40realtek.com

Re: [PATCH net-next v4 7/8] r8152: extract rtl_fc_pause_pkt_en() and apply it to RTL8156/8157/8159

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 3 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] rtl_fc_pause_pkt_en() (drivers/net/usb/r8152.c, RTL_VER_16 /
  RTL_VER_17_QFN68 / RTL_VER_17_QFN100 branch) derives the…
- [Medium] The changelog misstates the pre-patch state and omits the
  impact/Fixes: information for what is a functional change to already-…
- [Medium] In the new RTL_VER_16/RTL_VER_17_* branch of
  rtl_fc_pause_pkt_en() (drivers/net/usb/r8152.c:6206-6252) both…

--

r8152: extract rtl_fc_pause_pkt_en() and apply it to RTL8156/8157/8159

The flow-control setup that used to live inline in r8156b_init() is moved
into a new rtl_fc_pause_pkt_en() helper, and cases are added for RTL8156A
(VER_10/11) and for RTL8157/RTL8159 (VER_16/17), where a per-speed
pause-packet count is programmed into the new PLA_PAUSE_LIMIT register.
The helper is called from r8156_init(), r8156b_init() and rtl8157_enable().

A few questions about the changelog and about the new VER_16/17 branch.

[Severity: Medium]

The changelog says:

    RTL8156A (VER_10/11) and RTL8157/RTL8159 (VER_16/17)
    never applied any flow-control patch at all.

Is the VER_10/11 half of that accurate?  The ops table maps RTL_VER_10 and
RTL_VER_11 to rtl8156_enable(), and rtl8156_enable() already runs the
FC_PATCH_TASK toggle on every enable:

	ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_FW_TASK, FC_PATCH_TASK);
	usleep_range(1000, 2000);
	ocp_word_set_bits(tp, MCU_TYPE_USB, USB_FW_TASK, FC_PATCH_TASK);

What VER_10/11 appears to have been missing is just the USB_FC_TIMER
programming (CTRL_TIMER_EN | 1000 / 8) and the USB_FW_CTRL
FLOW_CTRL_PATCH_OPT bit that the new VER_10/11 case adds.  Could the
message be narrowed to that?

Also, the changelog does not describe an observable symptom (RX FIFO
overflow, drops, throughput loss under back-pressure), and there is no
Fixes: tag even though behaviour changes on already shipping hardware:
r8156_init() gains rtl_fc_pause_pkt_en(tp, 0) for RTL_VER_10/11
(candidate 195aae321c82 "r8152: support new chips"), and rtl8157_enable()
gains the PLA_PAUSE_LIMIT programming for RTL_VER_16 and both RTL_VER_17
variants (candidates fd3c7d080df5, ad2a55b63b41).

Could the impact be stated and the Fixes: tag(s) added, or the wording
adjusted so it does not read as fixing a defect?

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 3473061af129a..05b2b79ab3e31 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

> @@ -301,6 +302,10 @@
>  #define MTPS_JUMBO		(12 * 1024 / 64)
>  #define MTPS_DEFAULT		(6 * 1024 / 64)
>  
> +/* PLA_PAUSE_LIMIT */
> +#define PAUSE_LIMIT_EN		BIT(3)
> +#define PAUSE_LIMIT_MASK	0xf0
> +
>  /* PLA_RSTTALLY */
>  #define TALLY_RESET		0x0001
>  
> @@ -6163,6 +6168,93 @@ static void r8152b_enter_oob(struct r8152 *tp)
>  			   RCR_APM | RCR_AM | RCR_AB);
>  }
>  
> +static void rtl_fc_pause_pkt_en(struct r8152 *tp, u16 speed)
> +{
> +	int log2_ratio, ratio;
> +	u16 num_pause_pkts;
> +	u32 ocp_data;
> +

[ ... ]

> +	case RTL_VER_16:
> +	case RTL_VER_17_QFN68:
> +	case RTL_VER_17_QFN100:
> +		ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR, SLOT_EN);
> +
> +		num_pause_pkts = 0xa;
> +		ratio = 10000;
> +
> +		if (!(speed & LINK_STATUS)) {
> +			dev_dbg(&tp->intf->dev, "No link\n");
> +			goto no_link;
> +		} else if (speed & _10bps) {
> +			ratio /= 10;
> +		} else if (speed & _100bps) {
> +			ratio /= 100;
> +		} else if (speed & _1000bps) {
> +			ratio /= 1000;
> +		} else if (speed & _2500bps) {
> +			ratio /= 2500;
> +		} else if (speed & _5000bps) {
> +			ratio /= 5000;
> +		} else if (speed & _10000bps) {
> +			ratio /= 10000;
> +		} else {
> +			dev_err(&tp->intf->dev, "Unknown link speed\n");
> +			goto no_link;
> +		}
> +
> +		log2_ratio = ilog2(ratio);
> +		num_pause_pkts -= log2_ratio;
> +
> +		/* Round up if ratio is more than halfway to the next power of 2.
> +		 * Floating-point is avoided by rewriting
> +		 * ratio > 1.5 * 2^log2_ratio as
> +		 * 2 * ratio > 3 * 2^log2_ratio
> +		 */
> +		if (2 * ratio > 3 * (1 << log2_ratio))
> +			num_pause_pkts--;
> +

[Severity: Medium]

Can the values this formula produces be spelled out?  The changelog says
"a log2-ratio formula that reproduces that table", but the table itself is
nowhere in the patch, so the programmed encoding cannot be checked against
the datasheet by a reviewer or by whoever touches this next.

Evaluating the arithmetic gives:

	10 Mbit/s   ratio 1000  ilog2 9  0xa - 9 = 1, round-up hits -> 0
	100 Mbit/s  ratio 100   ilog2 6  0xa - 6 = 4, round-up hits -> 3
	1 Gbit/s    ratio 10    ilog2 3  0xa - 3 = 7, no round-up   -> 7
	2.5 Gbit/s  ratio 4     ilog2 2  0xa - 2 = 8, no round-up   -> 8
	5 Gbit/s    ratio 2     ilog2 1  0xa - 1 = 9, no round-up   -> 9
	10 Gbit/s   ratio 1     ilog2 0  0xa - 0 = 10               -> 10

Is num_pause_pkts == 0 at 10 Mbit/s the intended encoding?  The write
below clears PAUSE_LIMIT_MASK | PAUSE_LIMIT_EN, ORs in 0 << 4, and then
PAUSE_LIMIT_EN is set again, so the limiter is armed with a count field of
zero.  PLA_PAUSE_LIMIT was not written at all for these parts before this
patch, so if zero means "no pause frames" this changes 10 Mbit/s
behaviour.

Would a plain switch or lookup table (or the datasheet table quoted in a
comment) make the per-speed values auditable, and can the 10 Mbit/s
boundary value be confirmed against the datasheet?

> +no_link:
> +		ocp_word_w0w1(tp, MCU_TYPE_PLA, PLA_PAUSE_LIMIT,
> +			      PAUSE_LIMIT_MASK | PAUSE_LIMIT_EN,
> +			      num_pause_pkts << 4);
> +
> +		ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_PAUSE_LIMIT,
> +				  PAUSE_LIMIT_EN);
> +		break;

[Severity: Medium]

Both fallback paths reach this label with num_pause_pkts still at its
initialiser 0xa, which is the 10 Gbit/s entry, and the label then writes
that count and sets PAUSE_LIMIT_EN.  Should the no-link and unknown-speed
cases instead leave PLA_PAUSE_LIMIT alone (the pre-patch behaviour), or
clear PAUSE_LIMIT_EN?

The name no_link and the dev_dbg("No link") read as if nothing is
programmed, but the limiter ends up armed with the fastest link's pause
count while the MAC may be running at 10 or 100 Mbit/s, where the intended
counts are 0 and 3.

How is the no-link path reached?  rtl8157_enable() re-reads PLA_PHYSTATUS
after the caller already tested LINK_STATUS:

set_carrier()
	speed = rtl8152_get_speed(tp);
	if (speed & LINK_STATUS)
		... tp->rtl_ops.enable(tp);

rtl8157_enable()
	speed = rtl8152_get_speed(tp);
	rtl_fc_pause_pkt_en(tp, speed);

A link drop between the two reads takes the !(speed & LINK_STATUS) arm.
Since ->enable() only runs again on an observed carrier down/up
transition, and nothing else re-programs PLA_PAUSE_LIMIT, can a later
low-speed link then run for the whole session with the 10 Gbit/s pause
budget?

Separately, the decode chain covers only _10bps, _100bps, _1000bps,
_2500bps, _5000bps and _10000bps, while this driver's own
enum rtl_register_content also defines:

	_2500bps	= BIT(10),
	_1250bps	= BIT(9),
	_500bps		= BIT(8),

and rtl8156_enable() treats _500bps as a real link state:

	if (speed & _1000bps)
		ocp_write_word(tp, MCU_TYPE_PLA, PLA_EEE_TXTWSYS, 0x11);
	else if (speed & _500bps)
		ocp_write_word(tp, MCU_TYPE_PLA, PLA_EEE_TXTWSYS, 0x3d);

If RTL8157/RTL8159 can report one of those, this lands in the final else,
emits dev_err("Unknown link speed") at KERN_ERR on every ops->enable()
call (link-up, ndo_open, change_mtu, set_coalesce), and still arms the
10 Gbit/s limit.  Should the remaining encodings be decoded here too?

> +	default:
> +		break;
> +	}
> +}
> +
>  static int r8153_pre_firmware_1(struct r8152 *tp)
>  {
>  	int i;
> @@ -6694,6 +6786,8 @@ static int rtl8157_enable(struct r8152 *tp)
>  	r8153_set_rx_early_size(tp);
>  
>  	speed = rtl8152_get_speed(tp);
> +	rtl_fc_pause_pkt_en(tp, speed);
> +
>  	rtl_set_ifg(tp, speed);
>  
>  	return rtl_enable(tp);
> @@ -8842,6 +8936,8 @@ static void r8156_init(struct r8152 *tp)
>  
>  	usb_enable_lpm(tp->udev);
>  
> +	rtl_fc_pause_pkt_en(tp, 0);
> +
>  	r8156_mac_clk_spd(tp, true);
>  

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917070637.62827-1-nic_swsd%40realtek.com

Re: [PATCH net-next v4 8/8] r8152: enable UPS for RTL8157 and RTL8159 QFN68

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] r8157_ups_en()'s UPS-exit (disable) path in
  drivers/net/usb/r8152.c starts reprogramming the chip as soon as it…
- [Medium] In rtl8157_runtime_enable()'s disable (resume) branch the new
  UPS exit is sequenced last instead of first: line 4722 calls…

--

r8152: enable UPS for RTL8157 and RTL8159 QFN68

This adds r8157_ups_en() and calls it from rtl8157_runtime_enable() for
RTL_VER_16 and RTL_VER_17_QFN68, so UPS is now toggled across runtime
suspend/resume for those chips.  It also extends r8156_ups_flags() with
per-speed EEE flags and two new ups_flags_speed() entries.

Two questions below about the new UPS exit path.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 05b2b79ab3e31..ebe9cc9322b3c 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

> @@ -4484,6 +4522,35 @@ static void r8156_ups_en(struct r8152 *tp, bool enable)
>  	}
>  }
>  
> +static void r8157_ups_en(struct r8152 *tp, bool enable)
> +{
> +	if (enable) {
> +		r8156_ups_flags(tp);
> +
> +		ocp_byte_set_bits(tp, MCU_TYPE_USB, USB_POWER_CUT,
> +				  UPS_EN | USP_PREWAKE | PHASE2_EN);
> +
> +		ocp_byte_set_bits(tp, MCU_TYPE_USB, USB_MISC_2,
> +				  UPS_FORCE_PWR_DOWN);
> +	} else {
> +		ocp_byte_clr_bits(tp, MCU_TYPE_USB, USB_POWER_CUT,
> +				  UPS_EN | USP_PREWAKE);
> +
> +		ocp_byte_clr_bits(tp, MCU_TYPE_USB, USB_MISC_2,
> +				  UPS_FORCE_PWR_DOWN);
> +
> +		if (ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0) & PCUT_STATUS) {
> +			/* clear USB fw_ver_reg */
> +			ocp_write_byte(tp, MCU_TYPE_USB, USB_FW_USE_VER, 0);
> +
> +			tp->rtl_ops.hw_phy_cfg(tp);
> +
> +			rtl8152_set_speed(tp, tp->autoneg, tp->speed,
> +					  tp->duplex, tp->advertising);
> +		}
> +	}
> +}

[Severity: High]
Should this PCUT_STATUS branch wait for autoload to complete before
reprogramming the PHY?

The two UPS helpers that are actually reached from a runtime resume after a
power cut, r8153b_ups_en() and r8153c_ups_en(), gate the same sequence on
wait_autoload_done():

	if (ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0) & PCUT_STATUS) {
		if (wait_autoload_done(tp))
			return;

		tp->rtl_ops.hw_phy_cfg(tp);

		rtl8152_set_speed(tp, tp->autoneg, tp->speed,
				  tp->duplex, tp->advertising);
	}

r8156_ups_en(), from which the new helper was copied, has no such wait, but
its branch appears to be dead: both callers (r8156_init() and r8156b_init())
run r8153b_power_cut_en(tp, false) first, which clears PCUT_STATUS.  With
this patch the unguarded branch becomes live via

  rtl8152_runtime_resume() -> tp->rtl_ops.autosuspend_en(tp, false)
    -> rtl8157_runtime_enable(tp, false) -> r8157_ups_en(tp, false)

and PCUT_STATUS is exactly the expected state there, because the suspend side
just ran r8157_ups_en(tp, true).  Can hw_phy_cfg() and the following link
renegotiation then run while the chip is still reloading its post-power-cut
configuration?

wait_autoload_done() also returns -ENODEV when RTL8152_INACCESSIBLE is set,
so the peer helpers bail out on a disconnect during resume.  Without that
check, does a disconnect here run the whole hw_phy_cfg() sequence and then
the BMCR_RESET poll in rtl8152_set_speed(), which can spin 50 * msleep(20)
on failing register reads?

For RTL_VER_17_QFN68 there is a second handshake that this path skips.
r8159_init() treats a missing backup-restore as fatal whenever PCUT_STATUS is
set:

	if (r8159_wait_backup_restore(tp)) {
		rtl_set_inaccessible(tp);
		dev_err(&tp->intf->dev,
			"init failed, backup-restore timed out\n");
		return;
	}

r8159_wait_backup_restore() polls USB_GPHY_CTRL for BACKUP_RESTRORE only when
PCUT_STATUS is set.  On the new runtime resume path, r8159_hw_phy_cfg() only
calls r8156b_wait_loading_flash() and then clears PCUT_STATUS before starting
firmware/PHY programming, so nothing waits for BACKUP_RESTRORE.  Is that
handshake needed here too?

>  static void r8153_power_cut_en(struct r8152 *tp, bool enable)
>  {
>  	if (enable)

[ ... ]

> @@ -4642,9 +4709,28 @@ static void rtl8157_runtime_enable(struct r8152 *tp, bool enable)
>  		r8153b_u1u2en(tp, false);
>  		r8157_u2p3en(tp, false);
>  		rtl_runtime_suspend_enable(tp, true);
> +
> +		switch (tp->version) {
> +		case RTL_VER_16:
> +		case RTL_VER_17_QFN68:
> +			r8157_ups_en(tp, true);
> +			break;
> +		default:
> +			break;
> +		}
>  	} else {
>  		r8153_queue_wake(tp, false);
>  		rtl_runtime_suspend_enable(tp, false);
> +
> +		switch (tp->version) {
> +		case RTL_VER_16:
> +		case RTL_VER_17_QFN68:
> +			r8157_ups_en(tp, false);
> +			break;
> +		default:
> +			break;
> +		}
> +

[Severity: Medium]
Is the ordering here intentional?  In the resume branch the UPS exit runs
after the PLA writes, while the other UPS-capable variants exit UPS first.

rtl8153b_runtime_enable():

	} else {
		r8153b_ups_en(tp, false);
		r8153_queue_wake(tp, false);
		rtl_runtime_suspend_enable(tp, false);

rtl8153c_runtime_enable() has the same order with r8153c_ups_en(tp, false)
first.

Here, r8153_queue_wake(tp, false) and rtl_runtime_suspend_enable(tp, false)
touch PLA registers (PLA_INDICATE_FALG/UPCOMING_RUNTIME_D3, PLA_SUSPEND_FLAG,
PLA_EXTRA_STATUS, and the CRWECR-gated PLA_CONFIG34/LINK_OFF_WAKE_EN plus
__rtl_set_wol(tp->saved_wolopts)) while UPS_EN | USP_PREWAKE and
UPS_FORCE_PWR_DOWN are still set, and r8157_ups_en(tp, false) only clears
those bits afterwards.

Since r8153b_ups_en()/r8153c_ups_en() poll PLA_BOOT_CTRL for AUTOLOAD_DONE
right after clearing the same two bits, the PLA block seems to still be
reloading its defaults at that point.  Can the WOL settings restored from
tp->saved_wolopts and the cleared UPCOMING_RUNTIME_D3 flag be overwritten by
that reload, leaving the device unable to wake or with link-off-wake still
armed?  The new helper has no AUTOLOAD_DONE gate either, so nothing in this
resume sequence waits for the reload.

>  		r8157_u2p3en(tp, true);
>  		if (tp->udev->speed >= USB_SPEED_SUPER)
>  			r8153b_u1u2en(tp, true);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917070637.62827-1-nic_swsd%40realtek.com