| Message ID | 20250221071828.12323-442-nic_swsd@realtek.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | r8169: enable more devices ASPM and LTR support | expand |
On 21.02.2025 08:18, ChunHao Lin wrote: > Disable it due to it dose not meet ZRX-DC specification. If it is enabled, > device will exit L1 substate every 100ms. Disable it for saving more power > in L1 substate. > Is this compliant with the PCIe spec? Not being an expert on this topic, but when I read e.g. the following then my understanding is that this wakeup every 100ms is the expected behavior. https://lore.kernel.org/all/1610033323-10560-4-git-send-email-shradha.t@samsung.com/T/ > Signed-off-by: ChunHao Lin <hau@realtek.com> > --- > drivers/net/ethernet/realtek/r8169_main.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c > index 9953eaa01c9d..7a5b99d54e12 100644 > --- a/drivers/net/ethernet/realtek/r8169_main.c > +++ b/drivers/net/ethernet/realtek/r8169_main.c > @@ -2851,6 +2851,21 @@ static u32 rtl_csi_read(struct rtl8169_private *tp, int addr) > RTL_R32(tp, CSIDR) : ~0; > } > > +static void rtl_disable_zrxdc_timeout(struct rtl8169_private *tp) > +{ > + struct pci_dev *pdev = tp->pci_dev; > + u8 val; > + > + if (pdev->cfg_size > 0x0890 && > + pci_read_config_byte(pdev, 0x0890, &val) == PCIBIOS_SUCCESSFUL && > + pci_write_config_byte(pdev, 0x0890, val & ~BIT(0)) == PCIBIOS_SUCCESSFUL) > + return; > + > + netdev_notice_once(tp->dev, > + "No native access to PCI extended config space, falling back to CSI\n"); > + rtl_csi_write(tp, 0x0890, rtl_csi_read(tp, 0x0890) & ~BIT(0)); > +} > + > static void rtl_set_aspm_entry_latency(struct rtl8169_private *tp, u8 val) > { > struct pci_dev *pdev = tp->pci_dev; > @@ -3930,6 +3945,7 @@ static void rtl_hw_start_8125d(struct rtl8169_private *tp) > > static void rtl_hw_start_8126a(struct rtl8169_private *tp) > { > + rtl_disable_zrxdc_timeout(tp); > rtl_set_def_aspm_entry_latency(tp); > rtl_hw_start_8125_common(tp); > }
On Fri, Feb 21, 2025 at 03:18:28PM +0800, ChunHao Lin wrote: > Disable it due to it dose not meet ZRX-DC specification. If it is enabled, > device will exit L1 substate every 100ms. Disable it for saving more power > in L1 substate. s/dose/does/ Is ZRX-DC a PCIe spec? A Google search suggests that it might not be completely Realtek-specific? > +static void rtl_disable_zrxdc_timeout(struct rtl8169_private *tp) > +{ > + struct pci_dev *pdev = tp->pci_dev; > + u8 val; > + > + if (pdev->cfg_size > 0x0890 && > + pci_read_config_byte(pdev, 0x0890, &val) == PCIBIOS_SUCCESSFUL && > + pci_write_config_byte(pdev, 0x0890, val & ~BIT(0)) == PCIBIOS_SUCCESSFUL) Is this a standard PCIe extended capability? If so, it would be nice to search for it with pci_find_ext_capability() and use standard #defines. Bjorn
On 21.02.2025 21:01, Bjorn Helgaas wrote: > On Fri, Feb 21, 2025 at 03:18:28PM +0800, ChunHao Lin wrote: >> Disable it due to it dose not meet ZRX-DC specification. If it is enabled, >> device will exit L1 substate every 100ms. Disable it for saving more power >> in L1 substate. > > s/dose/does/ > > Is ZRX-DC a PCIe spec? A Google search suggests that it might not be > completely Realtek-specific? > ZRX-DC is the receiver DC impedance as specified in the PCIe Base Specification. From what I've found after a quick search ASPM restrictions apply if this impedance isn't in the range 40-60 ohm. >> +static void rtl_disable_zrxdc_timeout(struct rtl8169_private *tp) >> +{ >> + struct pci_dev *pdev = tp->pci_dev; >> + u8 val; >> + >> + if (pdev->cfg_size > 0x0890 && >> + pci_read_config_byte(pdev, 0x0890, &val) == PCIBIOS_SUCCESSFUL && >> + pci_write_config_byte(pdev, 0x0890, val & ~BIT(0)) == PCIBIOS_SUCCESSFUL) > > Is this a standard PCIe extended capability? If so, it would be nice > to search for it with pci_find_ext_capability() and use standard > #defines. > > Bjorn
On Fri, Feb 21, 2025 at 09:12:09PM +0100, Heiner Kallweit wrote: > On 21.02.2025 21:01, Bjorn Helgaas wrote: > > On Fri, Feb 21, 2025 at 03:18:28PM +0800, ChunHao Lin wrote: > >> Disable it due to it dose not meet ZRX-DC specification. If it is enabled, > >> device will exit L1 substate every 100ms. Disable it for saving more power > >> in L1 substate. > > > > s/dose/does/ > > > > Is ZRX-DC a PCIe spec? A Google search suggests that it might not be > > completely Realtek-specific? > > > ZRX-DC is the receiver DC impedance as specified in the PCIe Base > Specification. From what I've found after a quick search ASPM > restrictions apply if this impedance isn't in the range 40-60 ohm. Ah, so it looks like PCIe r6.0, sec 4.2.7.6.1.2, is the sort of thing this refers to: 4.2.7.6.1.2 Rx_L0s.Idle § - Next state is Rx_L0s.FTS if the Receiver detects an exit from Electrical Idle on any Lane of the configured Link. - Next state is Rx_L0s.FTS after a 100 ms timeout if the current data rate is 8.0 GT/s or higher and the Port’s Receivers do not meet the ZRX-DC specification for 2.5 GT/s (see § Table 8-11). All Ports are permitted to implement the timeout and transition to Rx_L0s.FTS when the data rate is 8.0 GT/s or higher. > >> +static void rtl_disable_zrxdc_timeout(struct rtl8169_private *tp) > >> +{ > >> + struct pci_dev *pdev = tp->pci_dev; > >> + u8 val; > >> + > >> + if (pdev->cfg_size > 0x0890 && > >> + pci_read_config_byte(pdev, 0x0890, &val) == PCIBIOS_SUCCESSFUL && > >> + pci_write_config_byte(pdev, 0x0890, val & ~BIT(0)) == PCIBIOS_SUCCESSFUL) > > > > Is this a standard PCIe extended capability? If so, it would be nice > > to search for it with pci_find_ext_capability() and use standard > > #defines. I guess we could tell from "sudo lspci -vv" output whether this is a standard capability. Bjorn
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c index 9953eaa01c9d..7a5b99d54e12 100644 --- a/drivers/net/ethernet/realtek/r8169_main.c +++ b/drivers/net/ethernet/realtek/r8169_main.c @@ -2851,6 +2851,21 @@ static u32 rtl_csi_read(struct rtl8169_private *tp, int addr) RTL_R32(tp, CSIDR) : ~0; } +static void rtl_disable_zrxdc_timeout(struct rtl8169_private *tp) +{ + struct pci_dev *pdev = tp->pci_dev; + u8 val; + + if (pdev->cfg_size > 0x0890 && + pci_read_config_byte(pdev, 0x0890, &val) == PCIBIOS_SUCCESSFUL && + pci_write_config_byte(pdev, 0x0890, val & ~BIT(0)) == PCIBIOS_SUCCESSFUL) + return; + + netdev_notice_once(tp->dev, + "No native access to PCI extended config space, falling back to CSI\n"); + rtl_csi_write(tp, 0x0890, rtl_csi_read(tp, 0x0890) & ~BIT(0)); +} + static void rtl_set_aspm_entry_latency(struct rtl8169_private *tp, u8 val) { struct pci_dev *pdev = tp->pci_dev; @@ -3930,6 +3945,7 @@ static void rtl_hw_start_8125d(struct rtl8169_private *tp) static void rtl_hw_start_8126a(struct rtl8169_private *tp) { + rtl_disable_zrxdc_timeout(tp); rtl_set_def_aspm_entry_latency(tp); rtl_hw_start_8125_common(tp); }
Disable it due to it dose not meet ZRX-DC specification. If it is enabled, device will exit L1 substate every 100ms. Disable it for saving more power in L1 substate. Signed-off-by: ChunHao Lin <hau@realtek.com> --- drivers/net/ethernet/realtek/r8169_main.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)