Re: [PATCH net-next v2 1/3] net: macb: fix SGMII with inband aneg disabled

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5004 bytes --]

On Tue, Feb 24, 2026 at 12:28:52PM -0800, Charles Perry wrote:
> Make it possible to connect a PHY which does not use inband
> autoneg to a gem MAC using phylink's information.
> 
> The previous implementation relied on whether or not the link
> was a fixed-link to disable SGMII autoneg. This commit extend
> this to all link which are not configured for inband
> autonegotiation.
> 
> Signed-off-by: Charles Perry <charles.perry@microchip.com>

This breaks the macb on mpfs-icicle-kit, I get stuck with:

[    7.189102] mpfs-sys-controller syscontroller: Registered MPFS system controller
[    7.260946] macb 20110000.ethernet eth0: PHY [20112000.ethernet-ffffffff:08] driver [Vitesse VSC8662] (irq=POLL)
[    7.273881] macb 20110000.ethernet eth0: configuring for phy/sgmii link mode
[    7.296580] macb 20110000.ethernet: gem-ptp-timer ptp clock registered.
[    7.345782] macb 20112000.ethernet eth1: PHY [20112000.ethernet-ffffffff:09] driver [Vitesse VSC8662] (irq=POLL)
[    7.358082] macb 20112000.ethernet eth1: configuring for phy/sgmii link mode
[    7.380479] macb 20112000.ethernet: gem-ptp-timer ptp clock registered.
[   11.376763] macb 20110000.ethernet eth0: Link is Up - 1Gbps/Full - flow control off
[   11.398403] Sending DHCP requests .
[   11.472699] macb 20112000.ethernet eth1: Link is Up - 1Gbps/Full - flow control off
[   13.938425] ..... timed out!
[   93.598491] macb 20110000.ethernet eth0: Link is Down
[   93.641823] macb 20110000.ethernet: gem-ptp-timer ptp clock unregistered.
[   93.659433] macb 20112000.ethernet eth1: Link is Down
[   93.691724] macb 20112000.ethernet: gem-ptp-timer ptp clock unregistered.
[   93.703977] IP-Config: Retrying forever (NFS root)...
[   93.758382] macb 20110000.ethernet eth0: PHY [20112000.ethernet-ffffffff:08] driver [Vitesse VSC8662] (irq=POLL)
[   93.770655] macb 20110000.ethernet eth0: configuring for phy/sgmii link mode
[   93.786497] macb 20110000.ethernet eth0: Link is Up - 1Gbps/Full - flow control off
[   93.795840] macb 20110000.ethernet: gem-ptp-timer ptp clock registered.
[   93.844481] macb 20112000.ethernet eth1: PHY [20112000.ethernet-ffffffff:09] driver [Vitesse VSC8662] (irq=POLL)
[   93.856769] macb 20112000.ethernet eth1: configuring for phy/sgmii link mode
[   93.870926] macb 20112000.ethernet eth1: Link is Up - 1Gbps/Full - flow control off
[   93.880302] macb 20112000.ethernet: gem-ptp-timer ptp clock registered.

and because I am NFS root, it breaks boot :)

btw, I don't see an explanation in hte commit message for why the
macb_is_gem(bp) got dropped? Is this now done unconditionally when it
was conditional before?

Cheers,
Conor.

> ---
> 
> Notes:
>     Changes in v2:
>       * Move my notes to the cover letter
>       * Remove the spinlock in macb_pcs_config
>       * Keep the comment about the PCSSEL register
> 
>  drivers/net/ethernet/cadence/macb_main.c | 36 +++++++++++++-----------
>  1 file changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index f72270a39d25..f4250e76cef0 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -574,6 +574,26 @@ static int macb_pcs_config(struct phylink_pcs *pcs,
>  			   const unsigned long *advertising,
>  			   bool permit_pause_to_mac)
>  {
> +	struct macb *bp = container_of(pcs, struct macb, phylink_sgmii_pcs);
> +	u32 old, new;
> +
> +	old = gem_readl(bp, PCSANADV);
> +	new = phylink_mii_c22_pcs_encode_advertisement(interface, advertising);
> +	if (new != -EINVAL && old != new)
> +		gem_writel(bp, PCSANADV, new);
> +
> +	/* Disable AN if it's not to be used, enable otherwise.
> +	 * Must be written after PCSSEL is set in NCFGR which is done in
> +	 * macb_mac_config(), otherwise writes will not take effect.
> +	 */
> +	old = gem_readl(bp, PCSCNTRL);
> +	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
> +		new = old | BMCR_ANENABLE;
> +	else
> +		new = old & ~BMCR_ANENABLE;
> +	if (old != new)
> +		gem_writel(bp, PCSCNTRL, new);
> +
>  	return 0;
>  }
>  
> @@ -628,22 +648,6 @@ static void macb_mac_config(struct phylink_config *config, unsigned int mode,
>  	if (old_ncr ^ ncr)
>  		macb_or_gem_writel(bp, NCR, ncr);
>  
> -	/* Disable AN for SGMII fixed link configuration, enable otherwise.
> -	 * Must be written after PCSSEL is set in NCFGR,
> -	 * otherwise writes will not take effect.
> -	 */
> -	if (macb_is_gem(bp) && state->interface == PHY_INTERFACE_MODE_SGMII) {
> -		u32 pcsctrl, old_pcsctrl;
> -
> -		old_pcsctrl = gem_readl(bp, PCSCNTRL);
> -		if (mode == MLO_AN_FIXED)
> -			pcsctrl = old_pcsctrl & ~GEM_BIT(PCSAUTONEG);
> -		else
> -			pcsctrl = old_pcsctrl | GEM_BIT(PCSAUTONEG);
> -		if (old_pcsctrl != pcsctrl)
> -			gem_writel(bp, PCSCNTRL, pcsctrl);
> -	}
> -
>  	spin_unlock_irqrestore(&bp->lock, flags);
>  }
>  
> -- 
> 2.47.3
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]