> -----Original Message----- > From: Heiner Kallweit [mailto:hkallweit1@xxxxxxxxx] > Sent: Saturday, March 1, 2025 6:58 AM > To: Bjorn Helgaas <helgaas@xxxxxxxxxx>; Hau <hau@xxxxxxxxxxx> > Cc: nic_swsd <nic_swsd@xxxxxxxxxxx>; andrew+netdev@xxxxxxx; > davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; > pabeni@xxxxxxxxxx; Bjorn Helgaas <bhelgaas@xxxxxxxxxx>; > netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux- > pci@xxxxxxxxxxxxxxx > Subject: Re: [PATCH net-next 2/3] r8169: enable > RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126 LTR support > > > External mail : This email originated from outside the organization. Do not > reply, click links, or open attachments unless you recognize the sender and > know the content is safe. > > > > On 24.02.2025 20:00, Bjorn Helgaas wrote: > > 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@xxxxxxxxxxx> > >>>> --- > >>>> 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. > > > I interpret this in a way that the chip triggers some internal LTR configuration > activity if it detects bit PCI_EXP_DEVCTL2_LTR_EN changing from 0 to 1. And > this needed activity isn't triggered if PCI_EXP_DEVCTL2_LTR_EN is set already > and doesn't change. > Hau, is this correct? > It may be the reason for the LTR of these platforms to work. But I will remove this quirk from next version patch. > So the PCI_EXP_DEVCTL2_LTR_EN reset is some kind of needed quirk. > However PCI quirks are applied too early, before we even detected the chip > version in probe(). Therefore I also think a helper for this reset in PCI core > would be best. > > And what hasn't been mentioned yet: We have to skip the chip-specific LTR > configuration if pci_dev->ltr_path isn't set. > I will add checking pci_dev->ltr_path in next version patch. > > 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
