Re: [PATCH net-next v1 4/6] r8169: add support for RTL8116af

[Andrew Lunn]

On Mon, Jun 08, 2026 at 06:32:19AM +0000, Javen wrote:
> >> +static bool rtl_is_8116af(struct rtl8169_private *tp) {
> >> +     return tp->mac_version == RTL_GIGA_MAC_VER_52 &&
> >> +             (r8168_mac_ocp_read(tp, 0xdc00) & 0x0078) == 0x0030 &&
> >> +             (r8168_mac_ocp_read(tp, 0xd006) & 0x00ff) == 0x0000;
> >
> >Do we know what these magic numbers mean?
> 
> 0xdc00 is a package-detect field. 0xd006 is internal HW id. RTL8116AF shares the same RTL_GIGA_MAC_VER_52 mac_version with other variants. 

Since you know what they are, please add #defines.

> >> @@ -2509,9 +2543,10 @@ void r8169_apply_firmware(struct
> >rtl8169_private *tp)
> >>               tp->ocp_base = OCP_STD_PHY_BASE;
> >>
> >>               /* PHY soft reset may still be in progress */
> >> -             phy_read_poll_timeout(tp->phydev, MII_BMCR, val,
> >> -                                   !(val & BMCR_RESET),
> >> -                                   50000, 600000, true);
> >> +             if (tp->phydev)
> >> +                     phy_read_poll_timeout(tp->phydev, MII_BMCR, val,
> >> +                                           !(val & BMCR_RESET),
> >> +                                           50000, 600000, true);
> >
> >Maybe this all needs to move into the PHY driver?
> 
> This is after firmware application. And PHY_MDIO_CHG opcode switches the access callbacks between PHY and MAC accessors.
> data == 0: phy_read/phy_write
> data != 0: mac_mcu_read/mac_mcu_write
> So the firmware may contain mixed PHY and MAC.

Normally, the MAC does not touch the PHY, it only calls phylib/phylink
API methods. If standard MII interfaces are used, you can connect any
MAC to any PHY.

If it is all integrated into silicon, then there is no choice, you
know the MAC/PHY relationship. However, it is still good practice to
keep MAC code in the MAC driver and PHY code in the PHY driver.

Can firmware programming be moved into the PHY driver?

> >> -     rg_saw_cnt = phy_read_paged(tp->phydev, 0x0c42, 0x13) & 0x3fff;
> >> -     if (rg_saw_cnt > 0) {
> >> -             u16 sw_cnt_1ms_ini;
> >> +     if (tp->phydev) {
> >> +             rg_saw_cnt = phy_read_paged(tp->phydev, 0x0c42, 0x13) & 0x3fff;
> >> +             if (rg_saw_cnt > 0) {
> >> +                     u16 sw_cnt_1ms_ini;
> >>
> >> -             sw_cnt_1ms_ini = (16000000 / rg_saw_cnt) & 0x0fff;
> >> -             r8168_mac_ocp_modify(tp, 0xd412, 0x0fff, sw_cnt_1ms_ini);
> >> +                     sw_cnt_1ms_ini = (16000000 / rg_saw_cnt) & 0x0fff;
> >> +                     r8168_mac_ocp_modify(tp, 0xd412, 0x0fff, sw_cnt_1ms_ini);
> >> +             }
> >
> >Can this move into the PHY driver?
> 
> It reads a counter from PHY, but the calculated value is programmed into MAC OCP register via r8168_mac_ocp_modify, which accesses r8169 through tp->mmio_addr. So I think this can not be moved.

So, big picture, what is this doing? What is rg_saw_cnt?

	Andrew