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

[Heiner Kallweit <hkallweit1@gmail.com>]

On 04.03.2026 10:43, 许俊伟 wrote:
>> On 02.03.2026 07:32, javen wrote:
>>> From: Javen Xu <javen_xu@realsil.com.cn>
>>>
>>> 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@realsil.com.cn>
>>> ---
>>>  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,
> Javen Xu
>