RE: [PATCH net-next v3 1/2] r8152: add helper functions for PLA/USB OCP registers

[Chih Kai Hsu]

On 25/03/26 2:06 am, Simon Horman 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.
> 
> For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
> ---
> r8152: add helper functions for PLA/USB OCP registers
> 
> This commit introduces new helper functions (ocp_dword_w0w1,
> ocp_word_w0w1, ocp_byte_w0w1, and their _clr_bits/_set_bits variants) to
> simplify bitwise operations on PLA/USB OCP registers throughout the
> r8152 driver.
> 
> > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index
> > 3b6d4252d34c..ba6dc4dbfd80 100644
> > --- a/drivers/net/usb/r8152.c
> > +++ b/drivers/net/usb/r8152.c
> 
> [ ... ]
> 
> > @@ -3875,35 +3873,30 @@ static void r8153c_ups_en(struct r8152 *tp,
> > bool enable)
> >
> >  static void r8156_ups_en(struct r8152 *tp, bool enable)  {
> > -     u32 ocp_data = ocp_read_byte(tp, MCU_TYPE_USB,
> USB_POWER_CUT);
> > -
> >       if (enable) {
> >               r8156_ups_flags(tp);
> >
> > -             ocp_data |= UPS_EN | USP_PREWAKE | PHASE2_EN;
> > -             ocp_write_byte(tp, MCU_TYPE_USB, USB_POWER_CUT,
> ocp_data);
> > +             ocp_byte_set_bits(tp, MCU_TYPE_USB, USB_POWER_CUT,
> > +                               UPS_EN | USP_PREWAKE |
> PHASE2_EN);
> >
> > -             ocp_data = ocp_read_byte(tp, MCU_TYPE_USB,
> USB_MISC_2);
> > -             ocp_data |= UPS_FORCE_PWR_DOWN;
> > -             ocp_write_byte(tp, MCU_TYPE_USB, USB_MISC_2,
> ocp_data);
> > +             ocp_byte_set_bits(tp, MCU_TYPE_USB, USB_MISC_2,
> > +                               UPS_FORCE_PWR_DOWN);
> >
> >               switch (tp->version) {
> >               case RTL_VER_13:
> >               case RTL_VER_15:
> > -                     ocp_data = ocp_read_word(tp, MCU_TYPE_USB,
> USB_UPHY_XTAL);
> > -                     ocp_data &= ~OOBS_POLLING;
> > -                     ocp_write_byte(tp, MCU_TYPE_USB,
> USB_UPHY_XTAL, ocp_data);
> > +                     ocp_word_clr_bits(tp, MCU_TYPE_USB,
> USB_UPHY_XTAL,
> > +                                       OOBS_POLLING);
>                               ^^^^
> 
> Does this actually change the hardware behavior for RTL_VER_13 and
> RTL_VER_15?
> 
> The old code read a 16-bit word with ocp_read_word() to get OOBS_POLLING
> (BIT(8)), then cleared BIT(8) in the local variable, but wrote it back using
> ocp_write_byte(). Since ocp_write_byte() masks the data with 0xff, the write
> operation would only touch the low 8 bits and never actually clear BIT(8) in
> the hardware register. This made the old code effectively a no-op for clearing
> OOBS_POLLING.
> 
> The new code uses ocp_word_clr_bits() which calls ocp_word_w0w1(), and
> that performs a proper 16-bit read-modify-write cycle with ocp_write_word().
> This means BIT(8) will now actually be cleared in the hardware.
> 
The OOBS_POLLING (BIT(8)) must be cleared in the USB_UPHY_XTAL register.
The old code using ocp_write_byte() is wrong.

> While this looks like it fixes a latent bug where OOBS_POLLING was never
> being cleared, the commit message describes this as pure refactoring to
> simplify the code with no mention of a behavioral change.
> 
I will mention this fix in the v4 commit message.

Thanks for your review.

Best Regards,
Chih-Kai