Re: [PATCH net-next v2 2/4] r8152: Add support for the RTL8159 chip
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-05-01 01:15:59
Also in:
linux-usb, lkml
This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. --- r8152: Add support for the RTL8159 chip This patch adds support for the Realtek RTL8159 as RTL_VER_17, reusing large portions of the existing RTL8157 (RTL_VER_16) initialization and USB power management paths. The bulk of the new code configures the RTL8159 PHY inside r8157_hw_phy_cfg(), and a new USB product id 0x815a is added to the device table.
r8152: Add support for the RTL8159 chip
The RTL8157 re-uses the packet descriptor format introduced with the
RTL8157 and other hardware features of the RTL8157 (RTL_VER_16) such
as the SRAM access. The support therefore consists in expanding the
existing RTL8157 code for initialization and USB power management
to also be used for the RTL8159 (RTL_VER_17).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?
Most of the addiitonal code is added in r8157_hw_phy_cfg() to configure
^^^^^^^^^^ Typo: addiitonal.
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?
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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. In rtl8156_up(): r8153b_u1u2en(tp, false); if (tp->version != RTL_VER_16) r8153_u2p3en(tp, false); ... if (tp->version == RTL_VER_16) ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR1, BIT(3)); ... if (tp->version != RTL_VER_16) ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_SPEED_OPTION, RG_PWRDN_EN | ALL_SPEED_OFF); ... r8153_aldps_en(tp, true); if (tp->version != RTL_VER_16) r8153_u2p3en(tp, true); if (tp->version != RTL_VER_16 && tp->udev->speed >= USB_SPEED_SUPER) r8153b_u1u2en(tp, true); Should these checks also be "< RTL_VER_16" for RTL_VER_17? On RTL_VER_17 the up path will call r8153_u2p3en() (USB_U2P3_CTRL) while r8156_init() and rtl8157_runtime_enable() route through r8157_u2p3en() (USB_U2P3_V2_CTRL) - is that intentional, or should RTL_VER_17 go through r8157_u2p3en() on up/down too? Similarly, is the PLA_RCR1 BIT(3) clear needed on RTL_VER_17, and is the re-enabling of r8153b_u1u2en(true) on up intended for the new chip given the init path deliberately skips it for RTL_VER_16? [ ... ]
quoted hunk ↗ jump to hunk
@@ -8140,6 +8149,190 @@ static void r8157_hw_phy_cfg(struct r8152 *tp) sram2_write_w0w1(tp, 0x807c, 0xff00, 0x5000); sram2_write_w0w1(tp, 0x809d, 0xff00, 0x5000); break; + + case RTL_VER_17:
[ ... snipped, PHY register table ... ]
quoted hunk ↗ jump to hunk
+ break; + default: break; }@@ -8175,6 +8368,18 @@ static void r8157_hw_phy_cfg(struct r8152 *tp) set_bit(PHY_RESET, &tp->flags); } +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, 2000, false); +}
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? 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?
quoted hunk ↗ jump to hunk
@@ -8184,14 +8389,14 @@ static void r8156_init(struct r8152 *tp) if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) return; - if (tp->version == RTL_VER_16) { + 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) + if (tp->version < RTL_VER_16) ocp_write_word(tp, MCU_TYPE_USB, USB_SPEED_OPTION, 0);
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. [ ... ]
quoted hunk ↗ jump to hunk
@@ -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? [ ... ]
quoted hunk ↗ jump to hunk
@@ -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.
+ ops->unload = rtl8153_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; + tp->rx_buf_sz = 48 * 1024; + tp->support_2500full = 1; + tp->support_5000full = 1; + tp->support_10000full = 1; + r8157_desc_init(tp); + break;
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? 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)? [ ... ]