Re: [PATCH net v2] net: stmmac: intel: gate SerDes reconfig on rate

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi Markus,

On 7/9/26 21:03, Markus Breitenberger wrote:
> From: Markus Breitenberger <bre@keba.com>
> 
> intel_mac_finish() is registered as the phylink mac_finish()
> callback for the Elkhart Lake SGMII ports. phylink calls it at
> the end of every major link reconfiguration, including the
> initial one during probe.
> 
> The callback selects the PMC ModPHY LCPLL programming for the
> requested MAC-side interface and then power-cycles the SerDes.
> On Elkhart Lake that ModPHY is also used by the on-die AHCI
> SATA PHY. Reapplying the programming during the initial
> boot-time link-up disturbs the shared analog block while it is
> still driving SATA, so the SATA link fails to train:
> 
>   ata1: SATA link down (SStatus 1 SControl 300)
> 
> The disk carrying the root filesystem is never detected and the
> system hangs at rootwait. Ethernet itself comes up normally,
> which makes the failure look unrelated to the network driver.
> 
> Before mac_finish() runs, the legacy SerDes power-up path has
> already programmed SERDES_GCR0 for the current interface. The
> 1G and 2.5G ModPHY tables selected by mac_finish() correspond
> to the SerDes lane rate, so read that rate back from SERDES_GCR0
> and skip the PMC reprogramming and SerDes power-cycle when it
> already matches the selected interface.
> 
> This keeps the disruptive reprogramming out of the boot path
> when the SerDes is configured correctly, while preserving the
> previous behavior when a real SGMII/1000BASE-X to 2500BASE-X
> rate change is needed. If the register read fails, reconfigure
> as before.
> 
> Fixes: a42f6b3f1cc1 ("net: stmmac: configure SerDes according to the interface mode")
> Cc: stable@vger.kernel.org
> Assisted-by: GitHub-Copilot:claude-opus-4.8
> Signed-off-by: Markus Breitenberger <bre@keba.com>
> ---
> v2:
>   - Read the current SerDes lane rate from SERDES_GCR0 instead of
>     comparing against cached phy_interface state.
>   - Rework the commit message to clarify the SerDes power-up path and
>     the rate readback check.
>   - Keep the previous reconfiguration behavior if the SERDES_GCR0 read
>     fails.
> 
> v1: https://lore.kernel.org/netdev/20260706061954.94842-1-bre@breiti.cc/
> 
>  .../net/ethernet/stmicro/stmmac/dwmac-intel.c | 28 +++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> index b8d467ba6d72..fa0113597c97 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> @@ -525,6 +525,31 @@ static int intel_set_reg_access(const struct pmc_serdes_regs *regs, int max_regs
>  	return ret;
>  }
>  
> +/* Return true if the SerDes lane rate must change to serve @interface.
> + * If the current rate cannot be determined, reconfigure as before.
> + */
> +static bool intel_serdes_needs_reconfig(struct stmmac_priv *priv,
> +					struct intel_priv_data *intel_priv,
> +					phy_interface_t interface)
> +{
> +	u32 cur_rate, want_rate;
> +	int data;
> +
> +	if (!intel_priv->mdio_adhoc_addr)
> +		return true;
> +
> +	data = mdiobus_read(priv->mii, intel_priv->mdio_adhoc_addr,
> +			    SERDES_GCR0);
> +	if (data < 0)
> +		return true;
> +
> +	cur_rate = (data & SERDES_RATE_MASK) >> SERDES_RATE_PCIE_SHIFT;
> +	want_rate = interface == PHY_INTERFACE_MODE_2500BASEX ?
> +			SERDES_RATE_PCIE_GEN2 : SERDES_RATE_PCIE_GEN1;
> +
> +	return cur_rate != want_rate;
> +}
> +
>  static int intel_mac_finish(struct net_device *ndev,
>  			    void *intel_data,
>  			    unsigned int mode,
> @@ -536,6 +561,9 @@ static int intel_mac_finish(struct net_device *ndev,
>  	int max_regs = 0;
>  	int ret = 0;
>  
> +	if (!intel_serdes_needs_reconfig(priv, intel_priv, interface))
> +		return 0;

You're returning a bit too early, make sure that you still update
priv->plat->phy_interface as you may be switching between 1000BaseX and
SGMII, so no rate change needed, but an interface change still :)

Maxime