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

[Sean Anderson]

Hi Russell,

On 10/5/21 6:06 AM, Russell King (Oracle) wrote:
> 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.

Ok, so instead I should be doing something like

if (gem_readl(bp, NCFGR) & GEM_BIT(SGMIIEN))
	interface = PHY_INTERFACE_MODE_SGMII;
else
	interface = PHY_INTERFACE_MODE_1000BASEX;

if (interface != state->interface) {
	state->link = 0;
	return;
}

?

> > +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.

Ok.

> > +		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.

This logic is taken from phylink_mii_c22_pcs_config. Maybe I should add
another _encode variant? I hadn't done this here because the logic was
only one if statement.

> I have a patch that adds that.

Have you posted it?

> > +	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.

Ok, so where can the PCS NAK an interface? This is the only callback
which has a return code, so I assumed this was the correct place to say
"no, we don't support this." This is what lynx_pcs_config does as well.

> >  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.

Hm, I didn't realized I had removed this, so I add it back in the next
patch. I will merge that one into this one.


> 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.

Ok, perhaps I can add some sanity checks for this in pcs_register.

--Sean

> That may also help the diff algorithm to produce a cleaner
> patch too.