Re: [PATCH net-next 2/3] r8169: enable RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126 LTR support

[Bjorn Helgaas <helgaas@kernel.org>]

On Mon, Feb 24, 2025 at 04:33:50PM +0000, Hau wrote:
> > On 21.02.2025 08:18, ChunHao Lin wrote:
> > > This patch will enable RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126
> > > LTR support on the platforms that have tested with LTR enabled.
> > 
> > Where in the code is the check whether platform has been tested with LTR?
> > 
> LTR is for L1,2. But L1 will be disabled when rtl_aspm_is_safe()
> return false. So LTR needs rtl_aspm_is_safe() to return true.
> 
> > > Signed-off-by: ChunHao Lin <hau@realtek.com>
> > > ---
> > >  drivers/net/ethernet/realtek/r8169_main.c | 108
> > > ++++++++++++++++++++++
> > >  1 file changed, 108 insertions(+)
> > >
> > > diff --git a/drivers/net/ethernet/realtek/r8169_main.c
> > > b/drivers/net/ethernet/realtek/r8169_main.c
> > > index 731302361989..9953eaa01c9d 100644
> > > --- a/drivers/net/ethernet/realtek/r8169_main.c
> > > +++ b/drivers/net/ethernet/realtek/r8169_main.c
> > > @@ -2955,6 +2955,111 @@ static void rtl_disable_exit_l1(struct
> > rtl8169_private *tp)
> > >       }
> > >  }
> > >
> > > +static void rtl_set_ltr_latency(struct rtl8169_private *tp) {
> > > +     switch (tp->mac_version) {
> > > +     case RTL_GIGA_MAC_VER_70:
> > > +     case RTL_GIGA_MAC_VER_71:
> > > +             r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcdd2, 0x8c09);
> > > +             r8168_mac_ocp_write(tp, 0xcdd8, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcdd4, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcdda, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcdd6, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcddc, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcde8, 0x887a);
> > > +             r8168_mac_ocp_write(tp, 0xcdea, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcdec, 0x8c09);
> > > +             r8168_mac_ocp_write(tp, 0xcdee, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcdf0, 0x8a62);
> > > +             r8168_mac_ocp_write(tp, 0xcdf2, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcdf4, 0x883e);
> > > +             r8168_mac_ocp_write(tp, 0xcdf6, 0x9003);
> > > +             break;
> > > +     case RTL_GIGA_MAC_VER_61 ... RTL_GIGA_MAC_VER_66:
> > > +             r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcdd2, 0x889c);
> > > +             r8168_mac_ocp_write(tp, 0xcdd8, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcdd4, 0x8c30);
> > > +             r8168_mac_ocp_write(tp, 0xcdda, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcdd6, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcddc, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcde8, 0x883e);
> > > +             r8168_mac_ocp_write(tp, 0xcdea, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcdec, 0x889c);
> > > +             r8168_mac_ocp_write(tp, 0xcdee, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcdf0, 0x8C09);
> > > +             r8168_mac_ocp_write(tp, 0xcdf2, 0x9003);
> > > +             break;
> > > +     case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_53:
> > > +             r8168_mac_ocp_write(tp, 0xcdd8, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcdda, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcddc, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcdd2, 0x883c);
> > > +             r8168_mac_ocp_write(tp, 0xcdd4, 0x8c12);
> > > +             r8168_mac_ocp_write(tp, 0xcdd6, 0x9003);
> > > +             break;
> > > +     default:
> > > +             break;
> > > +     }
> > > +}
> > > +
> > > +static void rtl_reset_pci_ltr(struct rtl8169_private *tp) {
> > > +     struct pci_dev *pdev = tp->pci_dev;
> > > +     u16 cap;
> > > +
> > > +     pcie_capability_read_word(pdev, PCI_EXP_DEVCTL2, &cap);
> > > +     if (cap & PCI_EXP_DEVCTL2_LTR_EN) {
> > > +             pcie_capability_clear_word(pdev, PCI_EXP_DEVCTL2,
> > > +                                        PCI_EXP_DEVCTL2_LTR_EN);
> > > +             pcie_capability_set_word(pdev, PCI_EXP_DEVCTL2,
> > > +                                      PCI_EXP_DEVCTL2_LTR_EN);
> > 
> > I'd prefer that only PCI core deals with these registers
> > (functions like pci_configure_ltr()). Any specific reason for this
> > reset? Is it something which could be applicable for other devices
> > too, so that the PCI core should be extended?
> > 
> It is for specific platform. On that platform driver needs to do
> this to let LTR works.

This definitely looks like code that should not be in a driver.
Drivers shouldn't need to touch ASPM or LTR configuration unless
there's a device defect to work around, and that should use a PCI core
interface.  Depending on what the defect is, we may need to add a new
interface.

This clear/set of PCI_EXP_DEVCTL2_LTR_EN when it was already set could
work around some kind of device defect, or it could be a hint that
something in the PCI core is broken.  Maybe the core is configuring
ASPM/LTR incorrectly.

Bjorn