Re: [PATCH net-next v4 06/10] net: pcs: xpcs: add ANRESTART support for SGMII link recovery - sashiko-bot

From: sashiko-bot@kernel.org
To: "Coia Prant" <coiaprant@gmail.com>
Cc: linux-phy@lists.infradead.org, devicetree@vger.kernel.org,
	vkoul@kernel.org, olteanv@gmail.com, robh@kernel.org,
	neil.armstrong@linaro.org, wsa+renesas@sang-engineering.com,
	conor+dt@kernel.org
Subject: Re: [PATCH net-next v4 06/10] net: pcs: xpcs: add ANRESTART support for SGMII link recovery
Date: Tue, 08 Sep 2026 15:40:26 +0000	[thread overview]
Message-ID: <20260908154050.665AF1F00A3D@smtp.kernel.org> (raw)
In-Reply-To: <20260907153713.2572547-8-coiaprant@gmail.com>

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Stale comment contradicts new ANRESTART behavior.
--

commit 553a0aad47033d8800155c135475e03ebbeacf26
Author: Coia Prant <coiaprant@gmail.com>

net: pcs: xpcs: add ANRESTART support for SGMII link recovery

This commit modifies the XPCS driver to trigger ANRESTART alongside ANENABLE
to ensure auto-negotiation correctly restarts on link state changes for
certain hardware. It applies this both when starting AN and when the link
is down but AN completion is detected.

> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index 0337e2bcc0125..50b9cdf94fdaa 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
> @@ -816,9 +816,14 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs,
[ ... ]
>  	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
>  		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR,
> -				 mdio_ctrl | BMCR_ANENABLE);
> +				 mdio_ctrl | BMCR_ANENABLE | BMCR_ANRESTART);

[Severity: Low]
Since this change explicitly triggers AN restart here, does the comment
earlier in xpcs_config_aneg_c37_sgmii() need to be updated?

The block comment at the top of the function currently states:

        /* For AN for C37 SGMII mode, the settings are :-
[ ... ]
         * Note: Since it is MAC side SGMII, there is no need to set
         *       SR_MII_AN_ADV. MAC side SGMII receives AN Tx Config from
         *       PHY about the link state change after C28 AN is completed
         *       between PHY and Link Partner. There is also no need to
         *       trigger AN restart for MAC-side SGMII.
         */

It seems the new code logic contradicts the last sentence of this comment.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907153713.2572547-2-coiaprant@gmail.com?part=6

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy