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

[Heiner Kallweit]

On 04.03.2026 09:16, 许俊伟 wrote:
>> On 02.03.2026 07:32, javen wrote:
>>> From: Javen Xu <javen_xu@xxxxxxxxxxxxxx>
>>>
>>> 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?
>>
> 
> Our customer platform (using RTL8116af) requires support for entering c10
> power-saving state, so we need to add support for this chip.
> 
What do you mean with "customer platform"? That what other manufacturers
would call a board support package (BSP)?

>>
>>> Signed-off-by: Javen Xu <javen_xu@xxxxxxxxxxxxxx>
>>> ---
>>>  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?
> 
> Unlike RTL8127atf where the SerDes status is automatically reflected in the
> Standard PHY register, RTL8116af does not sync this status to the PHY register.
> Maybe this hardware design is to save power. So we have no choice but to
> upstream such hack code to get link status and speed. 
> Given this hardware behavior, do you have any suggestions for a cleaner approach,
> or would this workaround be acceptable with some explaining comments?
> 
Even RTL8127atf requires ugly workarounds because there's no direct access to
the SerDes and the SFP I2C bus.
Realtek's wish to upstream support for their chips collides with their decision
to hide several essential components behind firmware. For proper mainline support
Realtek should expose components like SerDes and SFP I2C, so that the kernel
can access them, and phylink, sfp, and friends can do their job.

If use case is just Realtek's customer platform, then RTL8116af support could
be implemented downstream, avoiding hacks in mainline.


> 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);
>