Re: [PATCH net-next 01/15] net: phylink: add PCS negotiation mode

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

On Tue, Jun 20, 2023 at 02:34:29PM +0300, Vladimir Oltean wrote:
> On Fri, Jun 16, 2023 at 01:06:22PM +0100, Russell King (Oracle) wrote:
> > PCS have to work out whether they should enable PCS negotiation by
> > looking at the "mode" and "interface" arguments, and the Autoneg bit
> > in the advertising mask.
> > 
> > This leads to some complex logic, so lets pull that out into phylink
> > and instead pass a "neg_mode" argument to the PCS configuration and
> > link up methods, instead of the "mode" argument.
> > 
> > In order to transition drivers, add a "neg_mode" flag to the phylink
> > PCS structure to PCS can indicate whether they want to be passed the
> > neg_mode or the old mode argument.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> > diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> > index 0cf07d7d11b8..2b322d7fa51a 100644
> > --- a/include/linux/phylink.h
> > +++ b/include/linux/phylink.h
> > @@ -21,6 +21,24 @@ enum {
> >  	MLO_AN_FIXED,	/* Fixed-link mode */
> >  	MLO_AN_INBAND,	/* In-band protocol */
> >  
> > +	/* PCS "negotiation" mode.
> > +	 *  PHYLINK_PCS_NEG_NONE - protocol has no inband capability
> > +	 *  PHYLINK_PCS_NEG_OUTBAND - some out of band or fixed link setting
> 
> Would OUTBAND be more clearly named as FORCED maybe?

I don't see how FORCED would improve anything.

> > +	 *  PHYLINK_PCS_NEG_INBAND_DISABLED - inband mode disabled, e.g.
> > +	 *				      1000base-X with autoneg off
> > +	 *  PHYLINK_PCS_NEG_INBAND_ENABLED - inband mode enabled
> > +	 * Additionally, this can be tested using bitmasks:
> > +	 *  PHYLINK_PCS_NEG_INBAND - inband mode selected
> > +	 *  PHYLINK_PCS_NEG_ENABLED - negotiation mode enabled
> > +	 */
> > +	PHYLINK_PCS_NEG_NONE = 0,
> > +	PHYLINK_PCS_NEG_ENABLED = BIT(4),
> 
> Why do we start the enum values from BIT(4)? What are we colliding with,
> in the range of lower values?

I want to make sure that they can't be confused with MLO_AN_* values,
especially since we pass them through the same interface that MLO_AN_*
was using.

> > @@ -79,6 +97,70 @@ static inline bool phylink_autoneg_inband(unsigned int mode)
> >  	return mode == MLO_AN_INBAND;
> >  }
> >  
> > +/**
> > + * phylink_pcs_neg_mode() - helper to determine PCS inband mode
> 
> I think you are naming it "neg_mode" rather than "aneg_mode" because
> with OUTBAND/NONE, there's nothing "automatic" about the negotiation.
> However, "neg" rather than "aneg" sounds more like a shorthand form of
> "negation" or "negative". Would you oppose renaming it to "aneg_mode"?

I want to try to get away from "auto-negotiation" for this because
that has connotations of there being some kind of agreement reached
between both ends, like what happens with baseT links. However, that
is not the case with the SGMII family, which are more a form of
inband signalling.

I can't find any other term for the inband signalling that covers
all cases. "Negotiation" on its own covers both "negotiation result"
for SGMII as well as in-band negotiation for base-X cases. I don't
think "auto-negotiation" covers SGMII, and "in-band signalling"
wouldn't really cover base-X.

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