Re: [PATCH net-next v2 2/4] r8152: Add support for the RTL8159 chip
From: Jakub Kicinski
Date: Thu Apr 30 2026 - 21:16:55 EST
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?
> 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.
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?
[ ... ]
> @@ -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 ... ]
> + 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?
> @@ -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.
[ ... ]
> @@ -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?
[ ... ]
> @@ -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)?
[ ... ]