Re: [PATCH net-next v2 2/4] r8152: Add support for the RTL8159 chip
From: Birger Koblitz
Date: Sat May 02 2026 - 00:40:03 EST
On 01/05/2026 3:15 am, Jakub Kicinski wrote:
The commit subject says RTL8159 is being added, but the body reads
"The RTL8157 re-uses the packet descriptor format introduced with the
RTL8157 ..." and later "Add support for the USB device ID of Realtek
RTL8157-based adapters." Should these occurrences of RTL8157 actually
be RTL8159?
Both are typos and RTL8159 was meant. Will be fixed in v3.
Most of the addiitonal code is added in r8157_hw_phy_cfg() to configure^^^^^^^^^^
Typo: addiitonal.
Will fix.
Add support for the USB device ID of Realtek RTL8157-based adapters. Detect
the RTL8159 as RTL_VER_17 and set it up.
The new entry in rtl8152_table is 0x815a, which breaks the earlier
0x815N-for-RTL815N naming convention (0x8157 for RTL8157, etc.). Could
the commit message explain why the RTL8159 ships with product id
0x815a?
Will fix.
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 01e65d845f87..2a07dde289e2 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
[ ... ]
@@ -6894,7 +6903,7 @@ static void rtl8156_down(struct r8152 *tp)
PLA_MCU_SPDWN_EN);
r8153b_u1u2en(tp, false);
- if (tp->version != RTL_VER_16) {
+ if (tp->version < RTL_VER_16) {
r8153_u2p3en(tp, false);
r8153b_power_cut_en(tp, false);
}
rtl8156_down() is converted from "!= RTL_VER_16" to "< RTL_VER_16" so
that RTL_VER_17 takes the same path as RTL_VER_16. rtl8156_up() is
wired as ops->up for RTL_VER_17 in rtl_ops_init() but still uses
"!= RTL_VER_16" / "== RTL_VER_16" checks that were not converted.
This was simply forgotten. The code-path for the RTL8159, i.e. RTL_VER_17 should be the same as for the RTL8157 (RTL_VER_16), will fix rtl8156_up() accordingly.
The rest of the driver organizes helpers by chip family (r8152_,
r8153_, r8156_, r8157_) and the commit message describes the RTL8159
as reusing the RTL8157 scaffolding. Is the r8159_ prefix on this
single helper intentional, or would r8157_wait_backup_restore() (or
similar) fit better?
The prefix is intentional, since the function is only used for the
RTL8159.
Also, poll_timeout_us() is invoked with a 200 us poll interval and a
2000 us overall timeout, giving roughly ten iterations before the
error path fires. Other GPHY-related waits in the driver allow up to
the order of seconds. Is 2 ms known to be sufficient for the
backup-restore handshake on RTL8159 across suspend/resume and on
slower hosts?
Experimentally, the function returns always on the first iteration, which is the reason for the short poll interval. A safety factor of 10 appears to be relatively generous. The out-of-tree code was busy waiting for 100 iterations without any delay. The polling in wait_cmd_ready() waits for up-to 20ms. Since it does not hurt and to be on the safe side, I will change the total poll time to 20ms, here, too.
@@ -8184,14 +8389,14 @@ static void r8156_init(struct r8152 *tp)
Later in the same function:
if (tp->version != RTL_VER_16)
ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3,
PLA_MCU_SPDWN_EN);
Was this site meant to be converted to "< RTL_VER_16" along with the
others? As written, RTL_VER_17 clears PLA_MCU_SPDWN_EN during init,
while RTL_VER_16 does not.
Indeed, this was an oversight, it should have been also converted to "<". Will fix.
[ ... ]
@@ -8294,7 +8505,10 @@ static void r8156_init(struct r8152 *tp)
set_bit(GREEN_ETHERNET, &tp->flags);
/* rx aggregation / 16 bytes Rx descriptor */
- if (tp->version == RTL_VER_16)
+ 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);
Every other USB_USB_CTRL bit written in this driver uses a named
macro (CDC_ECM_EN, RX_AGG_DISABLE, RX_ZERO_EN, RX_DESC_16B). Could
BIT(11) be given a symbolic name so future multi-version handling of
this register stays self-describing?
Unfortunately, BIT(11) just appears here, for the RTL8159 in the out-of-tree code, without any further explanation. For BIT(10) = RX_DESC_16B I guessed the name from the comment. That bit appears for both chips, but BIT(11) just is there for the RTL8159, while the comment stays the same. Any guess could turn out to be misleading.
[ ... ]
@@ -9690,6 +9904,29 @@ static int rtl_ops_init(struct r8152 *tp)
r8157_desc_init(tp);
break;
+ case RTL_VER_17:
+ 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->enable = rtl8156_enable;
+ ops->disable = rtl8153_disable;
+ ops->up = rtl8156_up;
+ ops->down = rtl8156_down;
ops->up is wired to rtl8156_up() here, but rtl8156_up() still checks
for RTL_VER_16 exclusively at the sites noted above. See the
rtl8156_down() hunk for the pattern that appears to be intended.
This is fixed, see above.
RTL_VER_17 sets rx_buf_sz to 48 KiB and enables support_10000full,
but the max_mtu switch in rtl8152_probe() does not include
RTL_VER_17:
case RTL_VER_12:
case RTL_VER_13:
case RTL_VER_15:
case RTL_VER_16:
netdev->max_mtu = size_to_mtu(16 * 1024);
break;
...
default:
netdev->max_mtu = ETH_DATA_LEN;
break;
Should RTL_VER_17 also select the 16 KiB max_mtu case so jumbo frames
are available on the new 10 Gbit chip?
Yes, this was an oversight. Will be fixed.
RTL_VER_17 should be included in that range. Will fix.
Separately, rtl8156_enable() is set as ops->enable for RTL_VER_17 but
the USB_RX_AGGR_NUM reset is guarded by
"tp->version >= RTL_VER_12 && tp->version <= RTL_VER_16", so
RTL_VER_17 does not execute it. Is that intended given RTL_VER_17
otherwise shares the RTL_VER_16 descriptor/aggregation plumbing
(r8157_desc_init(), and the USB_USB_CTRL aggregation bits cleared in
r8156_init() for RTL_VER_17)?
[ ... ]