Re: [PATCH net-next 2/3] r8169: enable RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126 LTR support

[Heiner Kallweit <hkallweit1@gmail.com>]

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)