* Re: [PATCH net-next 2/3] r8169: enable RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126 LTR support
[not found] ` <20250221071828.12323-441-nic_swsd@realtek.com>
@ 2025-02-21 19:39 ` Heiner Kallweit
2025-02-24 16:33 ` Hau
0 siblings, 1 reply; 8+ messages in thread
From: Heiner Kallweit @ 2025-02-21 19:39 UTC (permalink / raw)
To: ChunHao Lin, nic_swsd, andrew+netdev, davem, edumazet, kuba,
pabeni, Bjorn Helgaas
Cc: netdev, linux-kernel, linux-pci@vger.kernel.org
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?
> 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?
+Bjorn and PCI list, to get an opinion from the PCI folks.
> + }
> +}
> +
> +static void rtl_enable_ltr(struct rtl8169_private *tp)
> +{
> + switch (tp->mac_version) {
> + case RTL_GIGA_MAC_VER_61 ... RTL_GIGA_MAC_VER_71:
> + r8168_mac_ocp_modify(tp, 0xe034, 0x0000, 0xc000);
> + r8168_mac_ocp_modify(tp, 0xe0a2, 0x0000, BIT(0));
> + r8168_mac_ocp_modify(tp, 0xe032, 0x0000, BIT(14));
> + break;
> + case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_48:
> + case RTL_GIGA_MAC_VER_52 ... RTL_GIGA_MAC_VER_53:
> + r8168_mac_ocp_modify(tp, 0xe0a2, 0x0000, BIT(0));
> + RTL_W8(tp, 0xb6, RTL_R8(tp, 0xb6) | BIT(0));
> + fallthrough;
> + case RTL_GIGA_MAC_VER_51:
> + r8168_mac_ocp_modify(tp, 0xe034, 0x0000, 0xc000);
> + r8168_mac_ocp_write(tp, 0xe02c, 0x1880);
> + r8168_mac_ocp_write(tp, 0xe02e, 0x4880);
> + break;
> + default:
> + return;
> + }
> +
> + rtl_set_ltr_latency(tp);
> +
> + /* chip can trigger LTR */
> + r8168_mac_ocp_modify(tp, 0xe032, 0x0003, BIT(0));
> +
> + /* reset LTR to notify host */
> + rtl_reset_pci_ltr(tp);
> +}
> +
> +static void rtl_disable_ltr(struct rtl8169_private *tp)
> +{
> + switch (tp->mac_version) {
> + case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_71:
> + r8168_mac_ocp_modify(tp, 0xe032, 0x0003, 0);
> + break;
> + default:
> + break;
> + }
> +}
> +
> static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
> {
> u8 val8;
> @@ -2971,6 +3076,8 @@ static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
> tp->mac_version == RTL_GIGA_MAC_VER_43)
> return;
>
> + rtl_enable_ltr(tp);
> +
> rtl_mod_config5(tp, 0, ASPM_en);
> switch (tp->mac_version) {
> case RTL_GIGA_MAC_VER_70:
> @@ -4821,6 +4928,7 @@ static void rtl8169_down(struct rtl8169_private *tp)
>
> rtl8169_cleanup(tp);
> rtl_disable_exit_l1(tp);
> + rtl_disable_ltr(tp);
Any specific reason why LTR isn't configured just once, on driver load?
> rtl_prepare_power_down(tp);
>
> if (tp->dash_type != RTL_DASH_NONE)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 3/3] r8169: disable RTL8126 ZRX-DC timeout
[not found] ` <20250221071828.12323-442-nic_swsd@realtek.com>
@ 2025-02-21 20:01 ` Heiner Kallweit
2025-02-24 16:09 ` Hau
0 siblings, 1 reply; 8+ messages in thread
From: Heiner Kallweit @ 2025-02-21 20:01 UTC (permalink / raw)
To: ChunHao Lin, nic_swsd, andrew+netdev, davem, edumazet, kuba,
pabeni, Bjorn Helgaas
Cc: netdev, linux-kernel, linux-pci@vger.kernel.org
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);
> }
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH net-next 3/3] r8169: disable RTL8126 ZRX-DC timeout
2025-02-21 20:01 ` [PATCH net-next 3/3] r8169: disable RTL8126 ZRX-DC timeout Heiner Kallweit
@ 2025-02-24 16:09 ` Hau
0 siblings, 0 replies; 8+ messages in thread
From: Hau @ 2025-02-24 16:09 UTC (permalink / raw)
To: Heiner Kallweit, nic_swsd, andrew+netdev@lunn.ch,
davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
pabeni@redhat.com, Bjorn Helgaas
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-pci@vger.kernel.org
>
> 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);
> > }
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH net-next 2/3] r8169: enable RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126 LTR support
2025-02-21 19:39 ` [PATCH net-next 2/3] r8169: enable RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126 LTR support Heiner Kallweit
@ 2025-02-24 16:33 ` Hau
2025-02-24 19:00 ` Bjorn Helgaas
2025-03-01 15:02 ` Heiner Kallweit
0 siblings, 2 replies; 8+ messages in thread
From: Hau @ 2025-02-24 16:33 UTC (permalink / raw)
To: Heiner Kallweit, nic_swsd, andrew+netdev@lunn.ch,
davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
pabeni@redhat.com, Bjorn Helgaas
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-pci@vger.kernel.org
>
> 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:
> > 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.
> +Bjorn and PCI list, to get an opinion from the PCI folks.
>
> > + }
> > +}
> > +
> > +static void rtl_enable_ltr(struct rtl8169_private *tp) {
> > + switch (tp->mac_version) {
> > + case RTL_GIGA_MAC_VER_61 ... RTL_GIGA_MAC_VER_71:
> > + r8168_mac_ocp_modify(tp, 0xe034, 0x0000, 0xc000);
> > + r8168_mac_ocp_modify(tp, 0xe0a2, 0x0000, BIT(0));
> > + r8168_mac_ocp_modify(tp, 0xe032, 0x0000, BIT(14));
> > + break;
> > + case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_48:
> > + case RTL_GIGA_MAC_VER_52 ... RTL_GIGA_MAC_VER_53:
> > + r8168_mac_ocp_modify(tp, 0xe0a2, 0x0000, BIT(0));
> > + RTL_W8(tp, 0xb6, RTL_R8(tp, 0xb6) | BIT(0));
> > + fallthrough;
> > + case RTL_GIGA_MAC_VER_51:
> > + r8168_mac_ocp_modify(tp, 0xe034, 0x0000, 0xc000);
> > + r8168_mac_ocp_write(tp, 0xe02c, 0x1880);
> > + r8168_mac_ocp_write(tp, 0xe02e, 0x4880);
> > + break;
> > + default:
> > + return;
> > + }
> > +
> > + rtl_set_ltr_latency(tp);
> > +
> > + /* chip can trigger LTR */
> > + r8168_mac_ocp_modify(tp, 0xe032, 0x0003, BIT(0));
> > +
> > + /* reset LTR to notify host */
> > + rtl_reset_pci_ltr(tp);
> > +}
> > +
> > +static void rtl_disable_ltr(struct rtl8169_private *tp) {
> > + switch (tp->mac_version) {
> > + case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_71:
> > + r8168_mac_ocp_modify(tp, 0xe032, 0x0003, 0);
> > + break;
> > + default:
> > + break;
> > + }
> > +}
> > +
> > static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp,
> > bool enable) {
> > u8 val8;
> > @@ -2971,6 +3076,8 @@ static void rtl_hw_aspm_clkreq_enable(struct
> rtl8169_private *tp, bool enable)
> > tp->mac_version == RTL_GIGA_MAC_VER_43)
> > return;
> >
> > + rtl_enable_ltr(tp);
> > +
> > rtl_mod_config5(tp, 0, ASPM_en);
> > switch (tp->mac_version) {
> > case RTL_GIGA_MAC_VER_70:
> > @@ -4821,6 +4928,7 @@ static void rtl8169_down(struct rtl8169_private
> > *tp)
> >
> > rtl8169_cleanup(tp);
> > rtl_disable_exit_l1(tp);
> > + rtl_disable_ltr(tp);
>
> Any specific reason why LTR isn't configured just once, on driver load?
>
It is for device compatibility, I will check internally to see if we can remove it.
> > rtl_prepare_power_down(tp);
> >
> > if (tp->dash_type != RTL_DASH_NONE)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 2/3] r8169: enable RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126 LTR support
2025-02-24 16:33 ` Hau
@ 2025-02-24 19:00 ` Bjorn Helgaas
2025-02-28 22:58 ` Heiner Kallweit
2025-03-01 15:02 ` Heiner Kallweit
1 sibling, 1 reply; 8+ messages in thread
From: Bjorn Helgaas @ 2025-02-24 19:00 UTC (permalink / raw)
To: Hau
Cc: Heiner Kallweit, nic_swsd, andrew+netdev@lunn.ch,
davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
pabeni@redhat.com, Bjorn Helgaas, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-pci@vger.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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 2/3] r8169: enable RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126 LTR support
2025-02-24 19:00 ` Bjorn Helgaas
@ 2025-02-28 22:58 ` Heiner Kallweit
2025-03-06 16:51 ` Hau
0 siblings, 1 reply; 8+ messages in thread
From: Heiner Kallweit @ 2025-02-28 22:58 UTC (permalink / raw)
To: Bjorn Helgaas, Hau
Cc: nic_swsd, andrew+netdev@lunn.ch, davem@davemloft.net,
edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
Bjorn Helgaas, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org
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@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.
>
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?
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.
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 2/3] r8169: enable RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126 LTR support
2025-02-24 16:33 ` Hau
2025-02-24 19:00 ` Bjorn Helgaas
@ 2025-03-01 15:02 ` Heiner Kallweit
1 sibling, 0 replies; 8+ messages in thread
From: Heiner Kallweit @ 2025-03-01 15:02 UTC (permalink / raw)
To: Hau, nic_swsd, andrew+netdev@lunn.ch, davem@davemloft.net,
edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
Bjorn Helgaas
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-pci@vger.kernel.org
On 24.02.2025 17:33, Hau wrote:
>>
>> 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:
>>> 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.
>
>> +Bjorn and PCI list, to get an opinion from the PCI folks.
>>
>>> + }
>>> +}
>>> +
>>> +static void rtl_enable_ltr(struct rtl8169_private *tp) {
>>> + switch (tp->mac_version) {
>>> + case RTL_GIGA_MAC_VER_61 ... RTL_GIGA_MAC_VER_71:
>>> + r8168_mac_ocp_modify(tp, 0xe034, 0x0000, 0xc000);
>>> + r8168_mac_ocp_modify(tp, 0xe0a2, 0x0000, BIT(0));
>>> + r8168_mac_ocp_modify(tp, 0xe032, 0x0000, BIT(14));
>>> + break;
>>> + case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_48:
>>> + case RTL_GIGA_MAC_VER_52 ... RTL_GIGA_MAC_VER_53:
>>> + r8168_mac_ocp_modify(tp, 0xe0a2, 0x0000, BIT(0));
>>> + RTL_W8(tp, 0xb6, RTL_R8(tp, 0xb6) | BIT(0));
>>> + fallthrough;
>>> + case RTL_GIGA_MAC_VER_51:
>>> + r8168_mac_ocp_modify(tp, 0xe034, 0x0000, 0xc000);
>>> + r8168_mac_ocp_write(tp, 0xe02c, 0x1880);
>>> + r8168_mac_ocp_write(tp, 0xe02e, 0x4880);
>>> + break;
>>> + default:
>>> + return;
>>> + }
>>> +
>>> + rtl_set_ltr_latency(tp);
>>> +
>>> + /* chip can trigger LTR */
>>> + r8168_mac_ocp_modify(tp, 0xe032, 0x0003, BIT(0));
>>> +
>>> + /* reset LTR to notify host */
>>> + rtl_reset_pci_ltr(tp);
>>> +}
>>> +
>>> +static void rtl_disable_ltr(struct rtl8169_private *tp) {
>>> + switch (tp->mac_version) {
>>> + case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_71:
>>> + r8168_mac_ocp_modify(tp, 0xe032, 0x0003, 0);
>>> + break;
>>> + default:
>>> + break;
>>> + }
>>> +}
>>> +
>>> static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp,
>>> bool enable) {
>>> u8 val8;
>>> @@ -2971,6 +3076,8 @@ static void rtl_hw_aspm_clkreq_enable(struct
>> rtl8169_private *tp, bool enable)
>>> tp->mac_version == RTL_GIGA_MAC_VER_43)
>>> return;
>>>
>>> + rtl_enable_ltr(tp);
>>> +
>>> rtl_mod_config5(tp, 0, ASPM_en);
>>> switch (tp->mac_version) {
>>> case RTL_GIGA_MAC_VER_70:
>>> @@ -4821,6 +4928,7 @@ static void rtl8169_down(struct rtl8169_private
>>> *tp)
>>>
>>> rtl8169_cleanup(tp);
>>> rtl_disable_exit_l1(tp);
>>> + rtl_disable_ltr(tp);
>>
>> Any specific reason why LTR isn't configured just once, on driver load?
>>
> It is for device compatibility, I will check internally to see if we can remove it.
>
Thanks. Complementing what I wrote before:
I would understand that reconfiguring LTR may be needed after a hw reset, when chip
"forgets" settings. But is there a reason to disable the internal LTR config?
IOW: What could happen if we omit rtl_disable_ltr()?
>
>>> rtl_prepare_power_down(tp);
>>>
>>> if (tp->dash_type != RTL_DASH_NONE)
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH net-next 2/3] r8169: enable RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126 LTR support
2025-02-28 22:58 ` Heiner Kallweit
@ 2025-03-06 16:51 ` Hau
0 siblings, 0 replies; 8+ messages in thread
From: Hau @ 2025-03-06 16:51 UTC (permalink / raw)
To: Heiner Kallweit, Bjorn Helgaas
Cc: nic_swsd, andrew+netdev@lunn.ch, davem@davemloft.net,
edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
Bjorn Helgaas, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org
> -----Original Message-----
> From: Heiner Kallweit [mailto:hkallweit1@gmail.com]
> Sent: Saturday, March 1, 2025 6:58 AM
> To: Bjorn Helgaas <helgaas@kernel.org>; Hau <hau@realtek.com>
> Cc: nic_swsd <nic_swsd@realtek.com>; andrew+netdev@lunn.ch;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; Bjorn Helgaas <bhelgaas@google.com>;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> pci@vger.kernel.org
> 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@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.
> >
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-03-06 16:51 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20250221071828.12323-439-nic_swsd@realtek.com>
[not found] ` <20250221071828.12323-441-nic_swsd@realtek.com>
2025-02-21 19:39 ` [PATCH net-next 2/3] r8169: enable RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126 LTR support Heiner Kallweit
2025-02-24 16:33 ` Hau
2025-02-24 19:00 ` Bjorn Helgaas
2025-02-28 22:58 ` Heiner Kallweit
2025-03-06 16:51 ` Hau
2025-03-01 15:02 ` Heiner Kallweit
[not found] ` <20250221071828.12323-442-nic_swsd@realtek.com>
2025-02-21 20:01 ` [PATCH net-next 3/3] r8169: disable RTL8126 ZRX-DC timeout Heiner Kallweit
2025-02-24 16:09 ` Hau
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox