Re: [PATCH net-next v1 2/4] r8169: add sfp mode for RTL8116af

[Heiner Kallweit <hkallweit1@gmail.com>]

On 17.03.2026 04:14, Javen wrote:
>> On 13.03.2026 08:25, Javen wrote:
>>>> On 02.03.2026 07:32, javen wrote:
>>>>> From: Javen Xu <javen_xu@realsil.com.cn>
>>>>>
>>>>> RTL8116af is a variation of RTL8168fp. It uses SerDes instead of PHY.
>>>>> But SerDes status will not relect to PHY. So it needs to add sfp
>>>>> mode for quirk to help reflect SerDes status during PHY read.
>>>>>
>>>>
>>>> Is there any mass market device using this chip version? As far as
>>>> possible I'd like to avoid adding support for chip versions that
>>>> never make it to the mass market. This just makes driver maintenance
>> harder.
>>>>
>>>> The patch includes support for 100Mbps fiber mode. Is there any use
>>>> case for such legacy modes?
>>>>
>>>>
>>>>> Signed-off-by: Javen Xu <javen_xu@realsil.com.cn>
>>>>> ---
>>>>>  drivers/net/ethernet/realtek/r8169_main.c | 71
>>>>> ++++++++++++++++++++---
>>>>>  1 file changed, 62 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c
>>>>> b/drivers/net/ethernet/realtek/r8169_main.c
>>>>> index fb2247a20c36..a5c0d3995328 100644
>>>>> --- a/drivers/net/ethernet/realtek/r8169_main.c
>>>>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
>>>>> @@ -726,6 +726,12 @@ enum rtl_dash_type {
>>>>>       RTL_DASH_25_BP,
>>>>>  };
>>>>>
>>>>> +enum rtl_sfp_mode {
>>>>> +     RTL_SFP_NONE,
>>>>> +     RTL_SFP_8168_AF,
>>>>> +     RTL_SFP_8127_ATF,
>>>>> +};
>>>>> +
>>>>>  struct rtl8169_private {
>>>>>       void __iomem *mmio_addr;        /* memory map physical address */
>>>>>       struct pci_dev *pci_dev;
>>>>> @@ -734,6 +740,7 @@ struct rtl8169_private {
>>>>>       struct napi_struct napi;
>>>>>       enum mac_version mac_version;
>>>>>       enum rtl_dash_type dash_type;
>>>>> +     enum rtl_sfp_mode sfp_mode;
>>>>>       u32 cur_rx; /* Index into the Rx descriptor buffer of next Rx pkt. */
>>>>>       u32 cur_tx; /* Index into the Tx descriptor buffer of next Rx pkt. */
>>>>>       u32 dirty_tx;
>>>>> @@ -760,7 +767,6 @@ struct rtl8169_private {
>>>>>       unsigned supports_gmii:1;
>>>>>       unsigned aspm_manageable:1;
>>>>>       unsigned dash_enabled:1;
>>>>> -     bool sfp_mode:1;
>>>>>       dma_addr_t counters_phys_addr;
>>>>>       struct rtl8169_counters *counters;
>>>>>       struct rtl8169_tc_offsets tc_offset; @@ -1126,7 +1132,7 @@
>>>>> static int r8168_phy_ocp_read(struct rtl8169_private *tp, u32 reg)
>>>>>               return 0;
>>>>>
>>>>>       /* Return dummy MII_PHYSID2 in SFP mode to match SFP PHY driver
>> */
>>>>> -     if (tp->sfp_mode && reg == (OCP_STD_PHY_BASE + 2 * MII_PHYSID2))
>>>>> +     if (tp->sfp_mode == RTL_SFP_8127_ATF && reg ==
>>>> (OCP_STD_PHY_BASE
>>>>> + + 2 * MII_PHYSID2))
>>>>>               return PHY_ID_RTL_DUMMY_SFP & 0xffff;
>>>>>
>>>>>       RTL_W32(tp, GPHY_OCP, reg << 15); @@ -1270,6 +1276,34 @@
>>>>> static int r8168g_mdio_read(struct rtl8169_private *tp, int reg)
>>>>>       return r8168_phy_ocp_read(tp, tp->ocp_base + reg * 2);  }
>>>>>
>>>>> +/* The quirk reflects RTL8116af SerDes status. */ static int
>>>>> +r8116af_mdio_read_quirk(struct rtl8169_private *tp, int reg) {
>>>>> +     u8 phyStatus = RTL_R8(tp, PHYstatus);
>>>>> +
>>>>> +     if (!(phyStatus & LinkStatus))
>>>>> +             return 0;
>>>>> +
>>>>> +     /* BMSR */
>>>>> +     if (tp->ocp_base == OCP_STD_PHY_BASE && reg == MII_BMSR)
>>>>> +             return BMSR_ANEGCOMPLETE | BMSR_LSTATUS;
>>>>> +
>>>>> +     /* PHYSR */
>>>>> +     if (tp->ocp_base == 0xa430 && reg == 0x12) {
>>>>> +             if (phyStatus & _1000bpsF)
>>>>> +                     return 0x0028;
>>>>> +             else if (phyStatus & _100bps)
>>>>> +                     return 0x0018;
>>>>
>>>> This is a complete hack. Any means to access the SerDes directly?
>>>
>>> Hi, Heiner
>>> Now we find a indirect access to serdes reg. The serdes reg is designed
>> according to IEEE 802.3.
>>> BIT 2 means Link Status and BIT 5 means ANEGCOMPLETE. We can access it
>>> through read mac ocp 0xeb14, I wonder whether this indirect access can be
>> accepted?
>>>
>> Based on the bit numbers this seems to be the BMSR register. Can all clause
>> 22 registers be accessed in a similar way so that a MII bus can be implemented
>> for accessing the SerDes?
> 
> We checked our chip documentation and confirmed that the SerDes register file maps
> all clause 22 registers(e.g. WR 0xeb10<reg_addr>, RD 0xeb14. reg_addr 0x40 is for BMCR,
> 0x41 for BMSR, 0x42 for PHYSID1, 0x43 for PHYSID2, etc.)
> As you suggested, I wonder whether the proper approach is to implement a virtual mii_bus
> and map the standard serdes reg in the new map.
> 
We'd need to know how the internal PHY and the SerDes play together.
Whether the usual internal PHY plays any role in communication between MAC and SerDes.
How interface modes are controlled on MAC and SerDes side, etc.
If Realtek doesn't implement proper layering in hw, and doesn't have enough knowledge
about mainline network driver structures, and on the other hand doesn't want to provide
chip documentation, then the situation somewhat deadlocks.

> Thanks,
> Javen Xu
> 
>> In general it's hard to provide a statement w/o knowing the internal layout of
>> the IP blocks.
>> Can you provide any chip documentation / datasheet?
>> For proper support of such a component chain between MAC and SFP port
>> the driver would have to be converted to use phylink, what requires access to
>> the involved components, incl. the SFP I2C bus.
>>
>>
>>> Thanks,
>>> Javen Xu
>>>
>>>>
>>>>> +     }
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static int r8116af_mdio_read(struct rtl8169_private *tp, int reg) {
>>>>> +     return r8168g_mdio_read(tp, reg) | r8116af_mdio_read_quirk(tp,
>>>>> +reg); }
>>>>> +
>>>>>  static void mac_mcu_write(struct rtl8169_private *tp, int reg, int
>>>>> value)  {
>>>>>       if (reg == 0x1f) {
>>>>> @@ -1280,6 +1314,13 @@ static void mac_mcu_write(struct
>>>>> rtl8169_private
>>>> *tp, int reg, int value)
>>>>>       r8168_mac_ocp_write(tp, tp->ocp_base + reg, value);  }
>>>>>
>>>>> +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; }
>>>>> +
>>>>>  static int mac_mcu_read(struct rtl8169_private *tp, int reg)  {
>>>>>       return r8168_mac_ocp_read(tp, tp->ocp_base + reg); @@ -1386,7
>>>>> +1427,10 @@ static int rtl_readphy(struct rtl8169_private *tp, int
>>>>> +location)
>>>>>       case RTL_GIGA_MAC_VER_31:
>>>>>               return r8168dp_2_mdio_read(tp, location);
>>>>>       case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_LAST:
>>>>> -             return r8168g_mdio_read(tp, location);
>>>>> +             if (tp->sfp_mode == RTL_SFP_8168_AF)
>>>>> +                     return r8116af_mdio_read(tp, location);
>>>>> +             else
>>>>> +                     return r8168g_mdio_read(tp, location);
>>>>>       default:
>>>>>               return r8169_mdio_read(tp, location);
>>>>>       }
>>>>> @@ -1575,6 +1619,20 @@ static bool rtl_dash_is_enabled(struct
>>>> rtl8169_private *tp)
>>>>>       }
>>>>>  }
>>>>>
>>>>> +static enum rtl_sfp_mode rtl_get_sfp_mode(struct rtl8169_private
>>>>> +*tp) {
>>>>> +     if (rtl_is_8125(tp)) {
>>>>> +             u16 data = r8168_mac_ocp_read(tp, 0xd006);
>>>>> +
>>>>> +             if ((data & 0xff) == 0x07)
>>>>> +                     return RTL_SFP_8127_ATF;
>>>>> +     } else if (rtl_is_8116af(tp)) {
>>>>> +             return RTL_SFP_8168_AF;
>>>>> +     }
>>>>> +
>>>>> +     return RTL_SFP_NONE;
>>>>> +}
>>>>> +
>>>>>  static enum rtl_dash_type rtl_get_dash_type(struct rtl8169_private
>>>>> *tp)  {
>>>>>       switch (tp->mac_version) {
>>>>> @@ -5693,12 +5751,7 @@ static int rtl_init_one(struct pci_dev *pdev,
>>>>> const
>>>> struct pci_device_id *ent)
>>>>>       }
>>>>>       tp->aspm_manageable = !rc;
>>>>>
>>>>> -     if (rtl_is_8125(tp)) {
>>>>> -             u16 data = r8168_mac_ocp_read(tp, 0xd006);
>>>>> -
>>>>> -             if ((data & 0xff) == 0x07)
>>>>> -                     tp->sfp_mode = true;
>>>>> -     }
>>>>> +     tp->sfp_mode = rtl_get_sfp_mode(tp);
>>>>>
>>>>>       tp->dash_type = rtl_get_dash_type(tp);
>>>>>       tp->dash_enabled = rtl_dash_is_enabled(tp);
>>>
>