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

[Javen]

Hi, Andrew, Maxime

>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?

For our PCIe nics, they are single chips. r8169 driver is for the entire integrated chip. Some internal phy shared the same PHY id, but the required PHY parameters differ depending on the specific MAC/Chip it is integrated with. So I think firmware here can not be moved.

>
>> >> -     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?

UPS mode TX-link-pulse timing calibration. We read rg_saw_cnt from the GPHY page 0x0C42, register 0x13. This value is the SAW calibration result burned during mass production. If rg_saw_cnt == 0, it means the MP did not burn the calibration result, so we do nothing. If calibrated (rg_saw_cnt > 0), we calculate the 1ms timer counter: sw_cnt_1ms_ini = (16000000 / rg_saw_cnt). And then write it into the MAC OCP register (0xD412). This requires reading a hardware-specific value from the PHY to configure a timer register in the MAC. So I think it can not be moved too.

How do you recommend handling this kind of SoC-specific tight coupling within the phylink framework? Is it acceptable to retain a local phydev pointer in the MAC driver solely for these hardware-specific initialization quirks, or is there a preferred approach for single-chip devices?

Any guidance for the migration would be greatly appreciated!

Thanks,
BRs,
Javen
>
>        Andrew