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

[Russell King (Oracle)]

On Thu, Feb 19, 2026 at 09:01:11AM -0800, Charles Perry wrote:
> @@ -574,6 +574,25 @@ 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);
> +	unsigned long flags;
> +	u32 old, new;
> +
> +	spin_lock_irqsave(&bp->lock, flags);
> +	old = gem_readl(bp, PCSANADV);
> +	new = phylink_mii_c22_pcs_encode_advertisement(interface, advertising);
> +	if (new != -EINVAL && old != new)
> +		gem_writel(bp, PCSANADV, new);
> +
> +	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);
> +
> +	spin_unlock_irqrestore(&bp->lock, flags);

What purpose does the spinlock serve?

phylink will call this under the phylink state_mutex lock, which will
guarantee that two threads can't execute this code simultaneously.

>  	return 0;
>  }
>  
> @@ -628,22 +647,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.

This looks like a key piece of information that is being removed from
the driver. Might be worth keeping it as a note that macb_pcs_config()
will be called after PCSSEL has been set in NCFGR?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!