RE: [PATCH net-next v1 4/4] r8169: enable system enter c10 with RTL8116af

[Javen]

>On 04.03.2026 10:43, 许俊伟 wrote:
>>> On 02.03.2026 07:32, javen wrote:
>>>> From: Javen Xu <javen_xu@xxxxxxxxxxxxxx>
>>>>
>>>> RTL8116af is a multi function chip. Function 1 is load with r8169
>>>> driver. Function 0 is bmc virual driver which is used for power
>>>> management. This patch set Function 2 to 7 into d3 state when config
>>>> hw_config. This helps the whole system enter c10.
>>>>
>>>> Signed-off-by: Javen Xu <javen_xu@xxxxxxxxxxxxxx>
>>>> ---
>>>>  drivers/net/ethernet/realtek/r8169_main.c | 95
>>>> +++++++++++++++++++++++
>>>>  1 file changed, 95 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c
>>>> b/drivers/net/ethernet/realtek/r8169_main.c
>>>> index 787859b0ab68..d8ffc76186b2 100644
>>>> --- a/drivers/net/ethernet/realtek/r8169_main.c
>>>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
>>>> @@ -1121,6 +1121,100 @@ DECLARE_RTL_COND(rtl_ocp_gphy_cond)
>>>>       return RTL_R32(tp, GPHY_OCP) & OCPAR_FLAG;  }
>>>>
>>>> +static u32 rtl_other_csi_read(struct rtl8169_private *tp, u8
>>>> +multi_fun_sel_bit, int addr) {
>>>> +     RTL_W32(tp, CSIAR, (addr & CSIAR_ADDR_MASK) |
>>>> +multi_fun_sel_bit
>>> << 16 |
>>>> +             CSIAR_BYTE_ENABLE);
>>>> +
>>>> +     return rtl_loop_wait_high(tp, &rtl_csiar_cond, 10, 100) ?
>>>> +             RTL_R32(tp, CSIDR) : ~0; }
>>>> +
>>>> +static void rtl_other_csi_write(struct rtl8169_private *tp,
>>>> +                             u8 multi_fun_sel_bit,
>>>> +                             int addr,
>>>> +                             int value) {
>>>> +     RTL_W32(tp, CSIDR, value);
>>>> +     RTL_W32(tp, CSIAR, CSIAR_WRITE_CMD | (addr &
>CSIAR_ADDR_MASK)
>>> |
>>>> +             CSIAR_BYTE_ENABLE | multi_fun_sel_bit << 16);
>>>> +
>>>> +     rtl_loop_wait_low(tp, &rtl_csiar_cond, 10, 100); }
>>>> +
>>>> +static void rtl8168_clear_and_set_other_fun_pci_bit(struct
>>>> +rtl8169_private
>>> *tp,
>>>> +                                                 u8 multi_fun_sel_bit,
>>>> +                                                 u32 addr,
>>>> +                                                 u32 clearmask,
>>>> +                                                 u32 setmask) {
>>>> +     u32 val;
>>>> +
>>>> +     val = rtl_other_csi_read(tp, multi_fun_sel_bit, addr);
>>>> +     val &= ~clearmask;
>>>> +     val |= setmask;
>>>> +     rtl_other_csi_write(tp, multi_fun_sel_bit, addr, val); }
>>>> +
>>>> +static void rtl8168_other_fun_dev_pci_setting(struct rtl8169_private *tp,
>>>> +                                           u32 addr,
>>>> +                                           u32 clearmask,
>>>> +                                           u32 setmask,
>>>> +                                           u8 multi_fun_sel_bit) {
>>>> +     u32 val;
>>>> +     u8 i;
>>>> +     u8 FunBit;
>>>> +     /* 0: BMC, 1: NIC, 2: TCR, 3: VGA/PCIE_TO_USB, 4: EHCI, 5:
>>>> +WIFI, 6: WIFI,
>>> 7: KCS */
>>>> +     for (i = 0; i < 8; i++) {
>>>> +             FunBit = (1 << i);
>>>> +             if (FunBit & multi_fun_sel_bit) {
>>>> +                     u8 set_other_fun = true;
>>>> +
>>>> +                     if (i == 0) {
>>>> +                             set_other_fun = true;
>>>> +                     } else if (i == 5 || i == 6) {
>>>> +                             if (tp->dash_enabled) {
>>>> +                                     val = rtl_eri_read(tp, 0x184);
>>>> +                                     if (val & BIT(26))
>>>> +                                             set_other_fun = false;
>>>> +                                     else
>>>> +                                             set_other_fun = true;
>>>> +                             }
>>>> +                     } else {
>>>> +                             val = rtl_other_csi_read(tp, i, 0x00);
>>>> +                             if (val == 0xffffffff)
>>>> +                                     set_other_fun = true;
>>>> +                             else
>>>> +                                     set_other_fun = false;
>>>> +                     }
>>>> +                     if (set_other_fun)
>>>> +                             rtl8168_clear_and_set_other_fun_pci_bit(tp, i, addr,
>>>> +                                                                     clearmask, setmask);
>>>> +             }
>>>> +     }
>>>> +}
>>>> +
>>>> +static void rtl8168_set_dash_other_fun_dev_state_change(struct
>>> rtl8169_private *tp,
>>>> +                                                     u8 dev_state,
>>>> +                                                     u8
>>>> +multi_fun_sel_bit) {
>>>> +     u32 clearmask;
>>>> +     u32 setmask;
>>>> +
>>>> +     if (dev_state == 0) {
>>>> +             clearmask = (BIT(0) | BIT(1));
>>>> +             setmask = 0;
>>>> +             rtl8168_other_fun_dev_pci_setting(tp, 0x44, clearmask,
>>>> +                                               setmask, multi_fun_sel_bit);
>>>> +     } else {
>>>> +             clearmask = 0;
>>>> +             setmask = (BIT(0) | BIT(1));
>>>> +             rtl8168_other_fun_dev_pci_setting(tp, 0x44, clearmask,
>>>> +                                               setmask, multi_fun_sel_bit);
>>>> +     }
>>>> +}
>>>> +
>>>>  static void r8168_phy_ocp_write(struct rtl8169_private *tp, u32
>>>> reg,
>>>> u32 data)  {
>>>>       if (rtl_ocp_reg_failure(reg))
>>>> @@ -3785,6 +3879,7 @@ static void rtl_hw_start_8117(struct
>>> rtl8169_private *tp)
>>>>       r8168_mac_ocp_write(tp, 0xc094, 0x0000);
>>>>       r8168_mac_ocp_write(tp, 0xc09e, 0x0000);
>>>>
>>>> +     rtl8168_set_dash_other_fun_dev_state_change(tp, 3, 0xfc);
>>>>       /* firmware is for MAC only */
>>>>       r8169_apply_firmware(tp);
>>>>  }
>>>
>>> This is the type of code that would make r8169 become an
>>> unmaintainable mess like the vendor drivers. Why not use standard
>>> means like
>>> pci_bus_read_config_word() et al?
>>
>> Thanks for the suggestion.
>> I first tried to use lspci, but I can't find function 2-7 in pci
>> system. Then I tried to use pci_bus_read_config_word() to access Function
>2-7 and get result as below.
>>
>> [21254.984983] RTL8168_DEBUG: Slot 0, Function 2, Read Vendor ID:
>> 0xffffffff
>>
>> This confirms that these funcitons are physically hidden from the PCI
>> bus. They do not respond to standard PCI configuration cycles, this
>> maybe the reason why standard PCI APIs cannot be used here.
>> These functions are important for power state issue. When system
>> suspends, linux PCI core automatically handles the power transition
>> for the visible functions. But since function 2-7 is invisible, the
>> kernel cannot manage it. As a result, we have no choice but to use the
>vendor-specific indirect access mechanism(via CSIAR register).
>>
>I understand your pain adding mainline support for broken hw designs. I
>consider the chip design fundamentally broken as PCI functions 2-7 are hidden
>and the ethernet driver is expected to set power management states for e.g.
>the EHCI and WiFi functions.
>As stated in another comment, the best option seems to me to add support
>for this chip downstream only.

Thanks for your suggestion, I will take it into consideration.

Thanks,
Javen Xu