Re: [RFC net-next PATCH 10/16] net: macb: Move PCS settings to PCS callbacks

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Mon, Oct 04, 2021 at 03:15:21PM -0400, Sean Anderson wrote:
> +static void macb_pcs_get_state(struct phylink_pcs *pcs,
> +			       struct phylink_link_state *state)
> +{
> +	struct macb *bp = pcs_to_macb(pcs);
> +
> +	if (gem_readl(bp, NCFGR) & GEM_BIT(SGMIIEN))
> +		state->interface = PHY_INTERFACE_MODE_SGMII;
> +	else
> +		state->interface = PHY_INTERFACE_MODE_1000BASEX;

There is no requirement to set state->interface here. Phylink doesn't
cater for interface changes when reading the state. As documented,
phylink will set state->interface already before calling this function
to indicate what interface mode it is currently expecting from the
hardware.

> +static int macb_pcs_config_an(struct macb *bp, unsigned int mode,
> +			      phy_interface_t interface,
> +			      const unsigned long *advertising)
> +{
> +	bool changed = false;
> +	u16 old, new;
> +
> +	old = gem_readl(bp, PCSANADV);
> +	new = phylink_mii_c22_pcs_encode_advertisement(interface, advertising,
> +						       old);
> +	if (old != new) {
> +		changed = true;
> +		gem_writel(bp, PCSANADV, new);
> +	}
> +
> +	old = new = gem_readl(bp, PCSCNTRL);
> +	if (mode == MLO_AN_INBAND)

Please use phylink_autoneg_inband(mode) here.

> +		new |= BMCR_ANENABLE;
> +	else
> +		new &= ~BMCR_ANENABLE;
> +	if (old != new) {
> +		changed = true;
> +		gem_writel(bp, PCSCNTRL, new);
> +	}

There has been the suggestion that we should allow in-band AN to be
disabled in 1000base-X if we're in in-band mode according to the
ethtool state. I have a patch that adds that.

> +	return changed;
> +}
> +
> +static int macb_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
> +			   phy_interface_t interface,
> +			   const unsigned long *advertising,
> +			   bool permit_pause_to_mac)
> +{
> +	bool changed = false;
> +	struct macb *bp = pcs_to_macb(pcs);
> +	u16 old, new;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&bp->lock, flags);
> +	old = new = gem_readl(bp, NCFGR);
> +	if (interface == PHY_INTERFACE_MODE_SGMII) {
> +		new |= GEM_BIT(SGMIIEN);
> +	} else if (interface == PHY_INTERFACE_MODE_1000BASEX) {
> +		new &= ~GEM_BIT(SGMIIEN);
> +	} else {
> +		spin_lock_irqsave(&bp->lock, flags);
> +		return -EOPNOTSUPP;

You can't actually abort at this point - phylink will print the error
and carry on regardless. The checking is all done via the validate()
callback and if that indicates the interface mode is acceptable, then
it should be accepted.

>  static const struct phylink_pcs_ops macb_phylink_usx_pcs_ops = {
>  	.pcs_get_state = macb_usx_pcs_get_state,
>  	.pcs_config = macb_usx_pcs_config,
> -	.pcs_link_up = macb_usx_pcs_link_up,
>  };
>  
>  static const struct phylink_pcs_ops macb_phylink_pcs_ops = {
>  	.pcs_get_state = macb_pcs_get_state,
> -	.pcs_an_restart = macb_pcs_an_restart,

You don't want to remove this. When operating in 1000BASE-X mode, it
will be called if a restart is required (e.g. macb_pcs_config()
returning positive, or an ethtool request.) You need to keep the empty
function. That may also help the diff algorithm to produce a cleaner
patch too.

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