RE: [PATCH net-next v7 05/10] r8152: fix up and down register settings for RTL8156/8156B/8157/8159

[Chih Kai Hsu]

On Wednesday, October 21, 2026 8:52 AM, Kicinski wrote:
> 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: fix up and down register settings for RTL8156/8156B/8157/8159
> 
> This commit corrects several up/down and OOB register settings for the
> RTL8156, RTL8156B, RTL8157 and RTL8159 chips, following the earlier
> per-generation split of the up/down callbacks. It changes the PLA_RCR1,
> USB_SPEED_OPTION, Clear_SDR, PLA_BDC_CR/PLA_OOB_CTRL and OOB
> PLA_RMS/PLA_MTPS settings, and drops PLA_MCU_SPDWN_EN handling from
> rtl8157_up()/rtl8157_down().
> 
> > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index
> > b245b63fb6aaf..045460a35f383 100644
> > --- a/drivers/net/usb/r8152.c
> > +++ b/drivers/net/usb/r8152.c
> 
> [ ... ]
> 
> > @@ -6938,8 +6937,18 @@ static void r8156_enter_oob(struct r8152 *tp)
> >       rtl_disable(tp);
> >       rtl_reset_bmu(tp);
> >
> > -     ocp_write_word(tp, MCU_TYPE_PLA, PLA_RMS, 1522);
> > -     ocp_write_byte(tp, MCU_TYPE_PLA, PLA_MTPS, MTPS_DEFAULT);
> > +     switch (tp->version) {
> > +     case RTL_VER_16:
> > +     case RTL_VER_17_QFN68:
> > +     case RTL_VER_17_QFN100:
> 
> [Severity: Medium]
> This patch has three Fixes: tags:
> 
>   Fixes: 195aae321c82 ("r8152: support new chips")
>   Fixes: fd3c7d080df5 ("r8152: Add support for the RTL8157 hardware")
>   Fixes: ad2a55b63b41 ("r8152: Add support for the RTL8159 chip")
> 
> Will it apply and build on the trees those tags point to?
> 
> RTL_VER_17_QFN68 and RTL_VER_17_QFN100 come from an earlier patch in
> this series, "r8152: split RTL_VER_17 into QFN68 and QFN100 package
> variants".
> Before that patch the enum only has RTL_VER_17.
> 
> rtl8157_up() and rtl8157_down() also only exist after "r8152: split
> rtl8156_enable/up/down into per-generation functions", which is described as
> no functional change. Before that patch there is one shared
> rtl8156_up() with tp->version >= RTL_VER_16 and tp->version < RTL_VER_16
> checks.
> 
> So a stable backport would either conflict or need both non-fix patches as
> prerequisites. One of them is the package detection change, which makes an
> unrecognised PLA_PKG_DET value fall back to RTL_VER_UNKNOWN.
> 
> Could these fixes go to net separately, written against the pre-refactor code? Or
> could the commit message at least name the prerequisites?

I will reorder the series so the fixes come first, written against the pre-refactor code, 
with the refactor rebased on top.

> 
> > +             ocp_write_word(tp, MCU_TYPE_PLA, PLA_RMS, 1526);
> > +             ocp_write_byte(tp, MCU_TYPE_PLA, PLA_MTPS, 10 * 1024 /
> 64);
> > +             break;
> 
> [ ... ]
> 
> > @@ -6997,6 +7008,11 @@ static void rtl8157_up(struct r8152 *tp)
> >       }
> >
> >       r8153_aldps_en(tp, true);
> > +
> > +     /* Enable Clear_SDR */
> > +     ocp_byte_set_bits(tp, MCU_TYPE_USB, 0xd378, BIT(7));
> > +     /* Clear SDR bit */
> > +     ocp_word_clr_bits(tp, MCU_TYPE_USB, 0xcd06, BIT(15));
> >  }
> 
> [Severity: High]
> The commit message says:
> 
>   - RTL8156B (VER_13/15) need Clear_SDR enabled on up, which nothing
>     ever did for them.
> 
> Is the RTL8156B part missing from this patch? These are the only Clear_SDR
> writes the patch adds. rtl8157_up() is only used for RTL_VER_16,
> RTL_VER_17_QFN68 and RTL_VER_17_QFN100.
> 
> In rtl_ops_init(), RTL_VER_13 and RTL_VER_15 are bound to rtl8156_up():
> 
>         case RTL_VER_12:
>         case RTL_VER_13:
>                 tp->support_2500full    = 1;
>                 fallthrough;
>         case RTL_VER_15:
>                 ...
>                 ops->up                 = rtl8156_up;
> 
> This patch does not change rtl8156_up(), and it still writes no Clear_SDR
> register:
> 
>         r8156_exit_oob(tp);
> 
>         ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3,
> PLA_MCU_SPDWN_EN);
> 
>         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);
>         ...
>         r8153_aldps_en(tp, true);
>         r8153_u2p3en(tp, true);
> 
> So every open or resume on an RTL8156B goes through ops->up ->
> rtl8156_up() without the Clear_SDR enable. The later patches in the series don't
> add it to rtl8156_up() either.
> 
> Should rtl8156_up() get the RTL8156B Clear_SDR writes under an
> RTL_VER_13/RTL_VER_15 check, or should this bullet come out of the commit
> message? The 0xd378/0xcd06 writes used for RTL8157/8159 can't be assumed
> to apply to RTL8156B.

I accidentally dropped the corresponding code; I will add it back.

Best Regards,
Chih-Kai