1. Patchew
  2. linux
  3. [PATCH net-next 0/3] r8169: enable more devices ASPM and LTR support
  4. View patch

[PATCH net-next 3/3] r8169: disable RTL8126 ZRX-DC timeout

ChunHao Lin posted 3 patches 9 months, 4 weeks ago
[PATCH net-next 3/3] r8169: disable RTL8126 ZRX-DC timeout
Posted by ChunHao Lin 9 months, 4 weeks ago 
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(+)

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);
 }
-- 
2.43.0
Re: [PATCH net-next 3/3] r8169: disable RTL8126 ZRX-DC timeout
Posted by Bjorn Helgaas 9 months, 3 weeks ago 
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
Re: [PATCH net-next 3/3] r8169: disable RTL8126 ZRX-DC timeout
Posted by Heiner Kallweit 9 months, 3 weeks ago 
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
Re: [PATCH net-next 3/3] r8169: disable RTL8126 ZRX-DC timeout
Posted by Bjorn Helgaas 9 months, 3 weeks ago 
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
Re: [PATCH net-next 3/3] r8169: disable RTL8126 ZRX-DC timeout
Posted by Heiner Kallweit 9 months, 3 weeks ago 
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);
>  }
RE: [PATCH net-next 3/3] r8169: disable RTL8126 ZRX-DC timeout
Posted by Hau 9 months, 3 weeks ago 
> 
> 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 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/
>
ZRX-DC timeout is for L1.Idle, not for L1 Substate. And according to the spec below, because it will reduce power saving, it is not encouraged to implement.
So we disable it for saving power.

Next state is Recovery 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). All Ports are permitted, but not encouraged, to
implement the timeout and transition to Recovery when the data rate is 8.0 GT/s or higher.

> > 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);
> >  }

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.