RE: [PATCH net-next v4 4/8] r8152: split r8157_hw_phy_cfg into RTL8157 and RTL8159 variants

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 3 · Low: 1

If you believe none of the issues are legitimate, please reply to each issue of
Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork know
by including "pw-bot: cr" as a separate line at the end of your reply (one such
reply per series is enough).

- [Medium] r8159_hw_phy_cfg() (drivers/net/usb/r8152.c:8309) contains
  functional PHY programming that the commit message does not disclose.
- [Medium] r8159_hw_phy_cfg() (drivers/net/usb/r8152.c:8309) drops the
  unconditional rtl_eee_enable(tp, false) that r8157_hw_phy_cfg() keeps…
- [Medium] The preceding patch in this series, "r8152: split RTL_VER_17
  into QFN68 and QFN100 package variants", justifies the new package…
- [Low] The commit message states the split was done "instead of
  switching on version mid-function", but both resulting functions still…

--

r8152: split r8157_hw_phy_cfg into RTL8157 and RTL8159 variants

This splits r8157_hw_phy_cfg() into an RTL8157-only version and a new
r8159_hw_phy_cfg() for RTL_VER_17_QFN68/QFN100, each carrying its own
copy of the shared prologue and epilogue.  It also adds sram2_set_bits()/
sram2_clr_bits() helpers and replaces a raw 0xa430 write with the named
OCP_POWER_CFG/EN_10M_PLLOFF/EN_ALDPS_PLLOFF macros.

No Link: tags in the commit header.

A few questions below about the new function and the commit message.

quoted

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index

c513c286c5499..d30031dbf2d38 100644

--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c

[ ... ]

quoted

@@ -8257,12 +8271,85 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)
              sram2_write_w0w1(tp, 0x807c, 0xff00, 0x5000);
              sram2_write_w0w1(tp, 0x809d, 0xff00, 0x5000);
              break;
+     default:
+             break;
+     }

[ ... copy of the r8157 epilogue snipped ... ]

quoted

+static void r8159_hw_phy_cfg(struct r8152 *tp) {
+     u16 data;
+
+     r8156b_wait_loading_flash(tp);
+
+     ocp_word_test_and_clr_bits(tp, MCU_TYPE_USB, USB_MISC_0,
+ PCUT_STATUS);
+
+     data = r8153_phy_status(tp, 0);
+     switch (data) {
+     case PHY_STAT_EXT_INIT:
+             rtl8152_apply_firmware(tp, true);
+             ocp_reg_clr_bits(tp, 0xa466, BIT(0));
+             ocp_reg_clr_bits(tp, 0xa468, BIT(3) | BIT(1));
+             break;
+     case PHY_STAT_LAN_ON:
+     case PHY_STAT_PWRDN:
+     default:
+             rtl8152_apply_firmware(tp, false);
+             break;
+     }
+
+     r8152_mdio_test_and_clr_bit(tp, MII_BMCR, BMCR_PDOWN);
+
+     r8153_aldps_en(tp, false);

[Severity: Medium]
Here the rtl_eee_enable(tp, false) call is gone, while the epilogue of
r8159_hw_phy_cfg() still ends with:

        if (tp->eee_en)
                rtl_eee_enable(tp, true);

with no else branch.

When tp->eee_en is false, is there anything left in this callback that programs
the EEE registers or updates tp->ups_info.eee?  r8157_hw_phy_cfg(),
r8153_hw_phy_cfg(), r8153b_hw_phy_cfg() and the r8156/r8156b variants all
keep the unconditional disable in the same spot.

tp->eee_en is user settable through r8152_set_eee(), which does eee_en =
tp->eee->eee_enabled followed by rtl_eee_enable(tp, tp->eee_en),
i.e. both states get programmed there.  hw_phy_cfg() is what re-establishes
that policy after the PHY goes back to defaults: it is re-run from the
PCUT_STATUS power-cut recovery paths and from rtl_hw_phy_work_func_t,
and
rtl8152_apply_firmware() runs just above.

So after "ethtool --set-eee <dev> eee off" and a later power-cut recovery on
RTL8159, can EEE come back enabled/advertised in hardware while ethtool still
reports eee_enabled == 0 and tp->ups_info.eee (used to build USB_UPS_FLAGS)
stays stale?

The EEE settings are properly recovered across runtime suspend, not reset to default.
During suspend, r8156_ups_flags() (called via r8157_ups_en()) saves the current PHY settings,
including EEE, into USB_UPS_FLAGS.

Upon resume, the firmware relies on USB_UPS_FLAGS to restore the exact PHY state.

The commit message says "RTL8159 doesn't need EEE disabled before
reprogramming PHY parameters", which covers the ordering role of that call.
Does it also cover its second role of enforcing the requested EEE state?

quoted

+
+     data = r8153_phy_status(tp, PHY_STAT_LAN_ON);
+     WARN_ON_ONCE(data != PHY_STAT_LAN_ON);
+
+     /* PFM mode */
+     ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_PHY_PWR,
+ PFM_PWM_SWITCH);
+
+     /* Advanced Power Saving parameter */
+     ocp_reg_set_bits(tp, OCP_POWER_CFG, EN_10M_PLLOFF |
+ EN_ALDPS_PLLOFF);
+
+     /* Disable ALDPS force mode */
+     ocp_reg_clr_bits(tp, 0xa44a, BIT(2));
+
+     /* Disable bypass_turn_off_clk_in_aldps */
+     ocp_byte_clr_bits(tp, MCU_TYPE_PLA, 0xd3c8, BIT(0));
+
+     switch (tp->version) {

[Severity: Low]
The commit message says the split was done "instead of switching on version
mid-function", but both resulting functions still switch on tp->version with only
one non-default case each.

rtl_ops_init() installs ops->hw_phy_cfg = r8159_hw_phy_cfg only under case
RTL_VER_17_QFN68 / case RTL_VER_17_QFN100, and
ops->hw_phy_cfg = r8157_hw_phy_cfg only under case RTL_VER_16, so
ops->neither
default arm is reachable.

Can both switches (and their default: break; arms) simply be dropped?

quoted

      case RTL_VER_17_QFN68:
      case RTL_VER_17_QFN100:

[Severity: Medium]
The preceding patch in this series, "r8152: split RTL_VER_17 into QFN68 and
QFN100 package variants", justifies the new package detection with:

    "RTL8159 is sold in two packages, QFN68 and QFN100, which need
     different PHY tuning (added in a later patch) and a package-specific
     u2phy backup-restore patch"

This is the patch that adds the RTL8159 PHY tuning, yet both package versions
fall into the same case and get identical power-level, TX filter, CMP timer, PLL
and SRAM/SRAM2 values.  By the end of the series the only QFN68-specific
code is the u2phy backup-restore write (0xb99c BIT(0)) in r8159_init(), the UPS
enable/disable selection, and the 10G EEE UPS flag.

Is one of the two packages being programmed with the other package's PHY
parameters here, or was the per-package PHY tuning rationale in the previous
patch inaccurate?

I will fix the commit message in the previous patch. Besides, the two packages 
will apply different patches; I will mention this in the previous patch.

quoted

-             /* Disable bypass turn off clk in ALDPS */
-             ocp_byte_clr_bits(tp, MCU_TYPE_PLA, 0xd3c8, BIT(0));
-
              /* Power level tuning
               * test mode power level
               */

@@ -8272,22 +8359,35 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)
              sram_write_w0w1(tp, 0x81ae, 0xff00, 0x0f00);
              sram_write_w0w1(tp, 0x81b9, 0xff00, 0xb900);
              /* normal link TX filter */
-             sram2_write_w0w1(tp, 0x83b0, 0x0e00, 0);
-             sram2_write_w0w1(tp, 0x83c5, 0x0e00, 0);
-             sram2_write_w0w1(tp, 0x83da, 0x0e00, 0);
-             sram2_write_w0w1(tp, 0x83ef, 0x0e00, 0);
+             sram2_clr_bits(tp, 0x83b0, 0x0e00);
+             sram2_clr_bits(tp, 0x83c5, 0x0e00);
+             sram2_clr_bits(tp, 0x83da, 0x0e00);
+             sram2_clr_bits(tp, 0x83ef, 0x0e00);
+
+             ocp_reg_w0w1(tp, 0xbf38, 0x01f0, 0x0160);
+             ocp_reg_w0w1(tp, 0xbf3a, 0x001f, 0x0014);
+             /* shorten CLKS latency */
+             ocp_reg_clr_bits(tp, 0xbf28, BIT(14) | BIT(13));
+             ocp_reg_clr_bits(tp, 0xbf2c, BIT(15) | BIT(14));
+             /* CMP_Timer on MP_Timer=333
+              * GPHY OCP 0xbf28 bit[0] = 0x1
+              * GPHY OCP 0xbf28 bit[6:1] = 0x3
+              * GPHY OCP 0xbf28 bit[12:7] = 0x3
+              */
+             ocp_reg_w0w1(tp, 0xbf28, 0x1fff, 0x0187);
+             ocp_reg_w0w1(tp, 0xbf2a, 0x3f, 0x03);

[Severity: Medium]
These writes to 0xbf38, 0xbf3a, 0xbf28, 0xbf2c and 0xbf2a have no matching
removal anywhere in the diff, so they are new PHY programming rather than
moved code.

The commit message enumerates exactly three RTL8159 divergences (dropping
rtl_eee_enable(tp, false), clearing EN_10M_CLKDIV instead of setting it, and the
extra 0xa5ea BIT(2)) and then frames the rest as "add
sram2_set_bits()/sram2_clr_bits() helpers ... and use the named
OCP_POWER_CFG/EN_10M_PLLOFF/EN_ALDPS_PLLOFF macros".

Can the "shorten CLKS latency" and "CMP_Timer on MP_Timer=333"
programming be mentioned in the commit message as well?

Sure. Will do.

quoted

              /* AFE power saving for 2.5G & 5G */
              sram_write(tp, 0x8173, 0x8620);
              sram_write(tp, 0x8175, 0x8671);

-             sram_write_w0w1(tp, 0x817c, 0, BIT(13));
-             sram_write_w0w1(tp, 0x8187, 0, BIT(13));
-             sram_write_w0w1(tp, 0x8192, 0, BIT(13));
-             sram_write_w0w1(tp, 0x819d, 0, BIT(13));
-             sram_write_w0w1(tp, 0x81a8, BIT(13), 0);
-             sram_write_w0w1(tp, 0x81b3, BIT(13), 0);
-             sram_write_w0w1(tp, 0x81be, 0, BIT(13));
+             sram_set_bits(tp, 0x817c, BIT(13));
+             sram_set_bits(tp, 0x8187, BIT(13));
+             sram_set_bits(tp, 0x8192, BIT(13));
+             sram_set_bits(tp, 0x819d, BIT(13));
+             sram_clr_bits(tp, 0x81a8, BIT(13));
+             sram_clr_bits(tp, 0x81b3, BIT(13));
+             sram_set_bits(tp, 0x81be, BIT(13));

              sram_write_w0w1(tp, 0x817d, 0xff00, 0xa600);
              sram_write_w0w1(tp, 0x8188, 0xff00, 0xa600); @@

-8351,10

quoted

+8451,10 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)
              sram2_write_w0w1(tp, 0x84b2, 0xff00, 0x6000);
              /* Training AAGC PAR (with uc2 patch) */
              sram2_write(tp, 0x8ffc, 0x6008);
-             sram2_write(tp, 0x8ffe, 0xf450);
+             sram2_write(tp, 0x8ffe, 0xf4ff);

[Severity: Medium]
The neighbouring lines in this block are pure sram2_set_bits()/
sram2_clr_bits() conversions, but this one changes the value written to 0x8ffe
from 0xf450 to 0xf4ff.  Is this value change intended, and can it be called out in
the commit message?

Yes, it is. I will mention it in the next version.

quoted

              /* DAC BGK */
-             sram2_write_w0w1(tp, 0x8015, 0, BIT(9));
-             sram2_write_w0w1(tp, 0x8016, 0, BIT(11));
+             sram2_set_bits(tp, 0x8015, BIT(9));
+             sram2_set_bits(tp, 0x8016, BIT(11));

[ ... ]

quoted

@@ -8440,8 +8540,15 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)
              sram2_write(tp, 0x8ff8, 0xaa5a);

              sram2_write_w0w1(tp, 0x88d5, 0xff00, 0x0200);
-             break;

+             /* spdchg_pga1_lpf_cap */
+             sram_write_w0w1(tp, 0x84bb, 0xff00, 0x0a00);
+             sram_write_w0w1(tp, 0x84c0, 0xff00, 0x1600);
+
+             /* ENET PLL jitter improvement */
+             ocp_reg_w0w1(tp, 0xbf8a, 0xfc00, 0x2000);
+             ocp_reg_set_bits(tp, 0xbf88, BIT(2));
+             break;

[Severity: Medium]
The spdchg_pga1_lpf_cap writes (0x84bb, 0x84c0) and the ENET PLL jitter
writes (0xbf8a, 0xbf88) are also new, with no counterpart removal in the diff.
Should the commit message list these among the RTL8159 changes too?

Sure.

quoted

      default:
              break;
      }

[ ... ]

quoted

@@ -10264,7 +10371,7 @@ static int rtl_ops_init(struct r8152 *tp)
              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->hw_phy_cfg         = r8159_hw_phy_cfg;
              ops->autosuspend_en     = rtl8157_runtime_enable;
              ops->change_mtu         = rtl8156_change_mtu;
              tp->rx_buf_sz           = 48 * 1024;

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

Best Regards,
Chih-Kai