This patch will enable RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126 LTR
support on the platforms that have tested with LTR enabled.
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);
+ }
+}
+
+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);
rtl_prepare_power_down(tp);
if (tp->dash_type != RTL_DASH_NONE)
--
2.43.0On 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)
>
> 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)
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)
>
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
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
> -----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
© 2016 - 2025 Red Hat, Inc.