RE: [PATCH net-next v3 2/8] r8152: support RTL8159 for different packages

[Chih Kai Hsu <hsu.chih.kai@realtek.com>]

> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 0 · Medium: 0 · Low: 1
> 
> - [Low] The changelog describes the patch as a pure enum split ("RTL8159
>   comes in two packages, QFN68 and QFN100, which require different…
> 
> --
> 
> r8152: support RTL8159 for different packages
> 
> This splits RTL_VER_17 into RTL_VER_17_QFN68 and RTL_VER_17_QFN100
> and updates every version switch accordingly.  Probe now reads
> PLA_PKG_DET over a USB control transfer to decide which of the two packages
> is present, and r8159_init() gains one package-specific register write.
> 
> > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index
> > 013e8d1abfc24..f01f442fa71a0 100644
> > --- a/drivers/net/usb/r8152.c
> > +++ b/drivers/net/usb/r8152.c
> 
> [ ... ]
> 
> > @@ -8716,6 +8739,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));
> > +
> 
> [Severity: Low]
> This isn't a bug, but could the changelog mention this write?  The commit
> message describes the change as an enum split:
> 
>     RTL8159 comes in two packages, QFN68 and QFN100, which require
> different
>     handling. Split RTL_VER_17 into RTL_VER_17_QFN68 and
> RTL_VER_17_QFN100.
> 

Sure, I will mention this change in the next version.

> Every other hunk in the patch is a mechanical rename, and this
> ocp_byte_set_bits(tp, MCU_TYPE_USB, 0xb99c, BIT(0)) in r8159_init() is the
> only place where the two packages actually behave differently.
> 
> RTL8159 has been driven as RTL_VER_17 since commit ad2a55b63b41af
> ("r8152:
> Add support for the RTL8159 chip"), so QFN68 parts are running today without
> this u2phy backup restore bit.
> 
> Is this write correcting misbehaviour on QFN68 hardware that is already
> supported, or is it enablement for silicon that was not previously in the field?
> If it is the former, would a Fixes: ad2a55b63b41af ("r8152: Add support for the
> RTL8159 chip") tag be appropriate so it reaches stable, and could the
> changelog describe the symptom it addresses?
> 
> >       /* Enable u3phy patch backup */
> >       ocp_write_word(tp, MCU_TYPE_USB, 0xb9a2, 0x0448);
> >
> 

It is enablement for the chip that was not previously in the field.

> [ ... ]
> 
> > @@ -10287,7 +10340,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;
> 
> [Severity: Low]
> Along the same lines, the changelog does not mention that hw version 0x2020
> now requires an extra PLA_PKG_DET control read at probe time to be classified
> at all.  Could a sentence be added covering that?

Sure, will do.

> 
> Was this new detection path validated against both packages, and which
> PLA_PKG_DET values were observed on each?
> 
> >       default:
> >               version = RTL_VER_UNKNOWN;
> 

Yes. PLA 0xDC48[4:1] is 0b1111 or 0b11110 for QFN68; 0b1100 or 0b1101 for QFN100.

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


Best,
Chih-Kai