Re: [PATCH net-next v2 5/6] r8169: add ltr support for RTL8116af

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
r8169: add ltr support for RTL8116af

This patch adds LTR programming for RTL8116af so the link can enter L1.2,
which is needed for the system to reach C10. The change extends
rtl_enable_ltr() with a new case for RTL_GIGA_MAC_VER_52, adds a global
SEND_LTR_MSG write at the end of rtl_enable_ltr(), and modifies several
unrelated registers in rtl_hw_start_8117().

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index a73c0215b240..a49e7a8d8f4d 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -349,11 +349,13 @@ enum rtl_registers {
>  	ALDPS_LTR	= 0xe0a2,
>  	LTR_OBFF_LOCK	= 0xe032,
>  	LTR_SNOOP	= 0xe034,
> +	SEND_LTR_MSG	= 0xe038,
>  
>  #define ALDPS_LTR_EN			BIT(0)
>  #define LTR_OBFF_LOCK_EN		BIT(0)
>  #define LINK_SPEED_CHANGE_EN		BIT(14)
>  #define LTR_SNOOP_EN			GENMASK(15, 14)
> +#define LTR_MSG_EN			BIT(0)
>  };
>  
>  enum rtl8168_8101_registers {
> @@ -3153,8 +3155,22 @@ static void rtl_enable_ltr(struct rtl8169_private *tp)
>  		r8168_mac_ocp_write(tp, 0xcdf2, 0x9003);
>  		r8168_mac_ocp_modify(tp, LTR_OBFF_LOCK, 0x0000, LINK_SPEED_CHANGE_EN);
>  		break;
> -	case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_48:
>  	case RTL_GIGA_MAC_VER_52:
> +		r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);
> +		r8168_mac_ocp_modify(tp, LTR_SNOOP, 0x0000, LTR_SNOOP_EN);
> +		r8168_mac_ocp_write(tp, 0xe02c, 0x1880);
> +		r8168_mac_ocp_write(tp, 0xe02e, 0x4880);
> +		r8168_mac_ocp_modify(tp, ALDPS_LTR, 0x0000, ALDPS_LTR_EN);
> +		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);
> +		r8168_mac_ocp_write(tp, 0xe0a6, 0x9003);
> +		r8168_mac_ocp_write(tp, 0xe0a8, 0x9003);
> +		break;

[High]
The new case is selected purely on tp->mac_version == RTL_GIGA_MAC_VER_52,
but per the chip_info table that mac_version covers both the existing
RTL8168fp/RTL8117 and the new RTL8116af. Should this case use
rtl_is_8116af() to distinguish the two, the way other 8116af-specific
paths in the driver already do?

Two behavioural deltas show up for RTL8117 with this change:

  - the previous (VER_46..VER_48 | VER_52) fallthrough caused VER_52
    to execute

        RTL_W8(tp, COMBO_LTR_EXTEND,
               RTL_R8(tp, COMBO_LTR_EXTEND) | COMBO_LTR_EXTEND_EN);

    which is silently dropped for VER_52 here.

  - new writes to OCP 0xe0a6 and 0xe0a8 are now performed on every
    VER_52 device, including pre-existing RTL8117 hardware that the
    old code never poked.

Is the intent that the new sequence supersedes the old RTL8117 LTR
programming for all VER_52 chips, or only for RTL8116af? If the former,
could the commit message explain why the existing RTL8117 sequence was
wrong, and should this be a separate patch?

> +	case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_48:
>  		r8168_mac_ocp_modify(tp, ALDPS_LTR, 0x0000, ALDPS_LTR_EN);
>  		RTL_W8(tp, COMBO_LTR_EXTEND, RTL_R8(tp, COMBO_LTR_EXTEND) | COMBO_LTR_EXTEND_EN);
>  		fallthrough;
> @@ -3174,6 +3190,7 @@ static void rtl_enable_ltr(struct rtl8169_private *tp)
>  	}
>  	/* chip can trigger LTR */
>  	r8168_mac_ocp_modify(tp, LTR_OBFF_LOCK, 0x0003, LTR_OBFF_LOCK_EN);
> +	r8168_mac_ocp_modify(tp, SEND_LTR_MSG, 0x0000, LTR_MSG_EN);
>  }

[Medium]
This new write sits after the switch, so it runs for every case that
does not hit the default return: VER_46..VER_48, VER_51, VER_52,
VER_61..VER_66, VER_70 and VER_80. The commit message describes the
change as RTL8116af-specific, but the resulting behaviour pokes
SEND_LTR_MSG bit 0 on every modern Realtek MAC handled by this driver.

Does OCP 0xe038 bit 0 have identical "send LTR message enable"
semantics across the entire MAC_VER_46..LAST range, or has this only
been validated on RTL8116af? If the former, could that be stated in
the commit message; if the latter, should the write be gated on the
chip family it was tested on?

[ ... ]

> @@ -3732,7 +3751,9 @@ static void rtl_hw_start_8117(struct rtl8169_private *tp)
>  
>  	rtl_eri_set_bits(tp, 0xd4, 0x0010);
>  
> -	rtl_eri_write(tp, 0x5f0, ERIAR_MASK_0011, 0x4f87);
> +	rtl_eri_write(tp, 0x5f0, ERIAR_MASK_0011, 0x4000);
> +
> +	r8168_mac_ocp_write(tp, 0xe098, 0xc302);
>  
>  	rtl_disable_rxdvgate(tp);

[High]
rtl_hw_start_8117() is dispatched for the entire VER_52 family
without any rtl_is_8116af() gating, so these changes affect existing
RTL8168fp/RTL8117 hardware and not just RTL8116af. None of the
modified registers (0x5f0, 0xe098, 0xea80, 0xe052, 0xd420) appear to
be LTR registers, yet the commit subject and body only discuss LTR
support for RTL8116af.

Could the commit message explain what each of these register changes
does and confirm they have been tested on existing RTL8117 hardware?
Should they be split into separate patches with their own
justifications?

>  
> @@ -3759,9 +3780,9 @@ static void rtl_hw_start_8117(struct rtl8169_private *tp)
>  	}
>  
>  	r8168_mac_ocp_modify(tp, 0xe056, 0x00f0, 0x0000);
> -	r8168_mac_ocp_write(tp, 0xea80, 0x0003);
> -	r8168_mac_ocp_modify(tp, 0xe052, 0x0000, 0x0009);
> -	r8168_mac_ocp_modify(tp, 0xd420, 0x0fff, 0x045f);
> +	r8168_mac_ocp_write(tp, 0xea80, 0x0000);
> +	r8168_mac_ocp_modify(tp, 0xe052, 0x0009, 0x0000);
> +	r8168_mac_ocp_modify(tp, 0xd420, 0x0fff, 0x045f);

[High]
The 0xe052 modify call has its mask and set arguments swapped:

    -    r8168_mac_ocp_modify(tp, 0xe052, 0x0000, 0x0009);
    +    r8168_mac_ocp_modify(tp, 0xe052, 0x0009, 0x0000);

r8168_mac_ocp_modify() applies (data & ~mask) | set, so the old call
set bits 0 and 3 of 0xe052 while the new call clears them. Is this
semantic inversion intentional, and if so could the commit message
say so explicitly? On what hardware was the new behaviour validated,
given that VER_52 covers both RTL8168fp/RTL8117 and the new
RTL8116af?

>  
>  	r8168_mac_ocp_write(tp, 0xe63e, 0x0001);
>  	r8168_mac_ocp_write(tp, 0xe63e, 0x0000);