diff mbox series

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

Message ID E1qA8De-00EaFA-Ht@rmk-PC.armlinux.org.uk (mailing list archive)
State Accepted
Commit f99d471afa03f770149f1cc60a288b9a08285903
Delegated to: Netdev Maintainers
Headers show
Series Add and use helper for PCS negotiation modes | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 98 this patch: 98
netdev/cc_maintainers success CCed 12 of 12 maintainers
netdev/build_clang success Errors and warnings before: 13 this patch: 13
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 98 this patch: 98
netdev/checkpatch warning CHECK: Alignment should match open parenthesis WARNING: line length of 81 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Russell King (Oracle) June 16, 2023, 12:06 p.m. UTC 
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>
---
 drivers/net/phy/phylink.c |  45 +++++++++++++----
 include/linux/phylink.h   | 104 +++++++++++++++++++++++++++++++++++---
 2 files changed, 132 insertions(+), 17 deletions(-)

Comments

Simon Horman June 16, 2023, 3:51 p.m. UTC | #1 
On Fri, Jun 16, 2023 at 01:06:22PM +0100, Russell King (Oracle) wrote:

Hi Russell,

some minor feedback from my side.

> @@ -1149,12 +1159,20 @@ static int phylink_change_inband_advert(struct phylink *pl)
>  		    __ETHTOOL_LINK_MODE_MASK_NBITS, pl->link_config.advertising,
>  		    pl->link_config.pause);
>  
> +	/* Recompute the PCS neg mode */
> +	pl->pcs_neg_mode = phylink_pcs_neg_mode(pl->cur_link_an_mode,
> +					pl->link_config.interface,
> +					pl->link_config.advertising);

nit: the indentation of the above two lines seems off.

> +
> +	neg_mode = pl->cur_link_an_mode;
> +	if (pl->pcs->neg_mode)
> +		neg_mode = pl->pcs_neg_mode;
> +

Smatch is unhappy that previously it was thought that
pl->pcs could be NULL.

I assume it is taking into account the following, which appears slightly
above this hunk:

	if (!pl->pcs && pl->config->legacy_pre_march2020) {
		...
		return 0;
	}

Could it be the case that pl->pcs is NULL and
pl->config->legacy_pre_march2020 is false?



>  	/* Modern PCS-based method; update the advert at the PCS, and
>  	 * restart negotiation if the pcs_config() helper indicates that
>  	 * the programmed advertisement has changed.
>  	 */
> -	ret = phylink_pcs_config(pl->pcs, pl->cur_link_an_mode,
> -				 &pl->link_config,
> +	ret = phylink_pcs_config(pl->pcs, neg_mode, &pl->link_config,
>  				 !!(pl->link_config.pause & MLO_PAUSE_AN));
>  	if (ret < 0)
>  		return ret;

...
Vladimir Oltean June 20, 2023, 11:34 a.m. UTC | #2 
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?

> +	 *  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?

> +	PHYLINK_PCS_NEG_OUTBAND = BIT(5),
> +	PHYLINK_PCS_NEG_INBAND = BIT(6),
> +	PHYLINK_PCS_NEG_INBAND_DISABLED = PHYLINK_PCS_NEG_INBAND,
> +	PHYLINK_PCS_NEG_INBAND_ENABLED = PHYLINK_PCS_NEG_INBAND |
> +					 PHYLINK_PCS_NEG_ENABLED,
> +
>  	/* MAC_SYM_PAUSE and MAC_ASYM_PAUSE are used when configuring our
>  	 * autonegotiation advertisement. They correspond to the PAUSE and
>  	 * ASM_DIR bits defined by 802.3, respectively.
> @@ -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"?
Vladimir Oltean June 20, 2023, 11:37 a.m. UTC | #3 
On Fri, Jun 16, 2023 at 01:06:22PM +0100, Russell King (Oracle) wrote:
> @@ -443,6 +526,7 @@ struct phylink_pcs_ops;
>   */
>  struct phylink_pcs {
>  	const struct phylink_pcs_ops *ops;
> +	bool neg_mode;
>  	bool poll;
>  };

I deleted one of my own comments while trimming the email... Yay me :)

Would it be more appropriate to name this "bool pass_neg_mode" to avoid
a naming collision between "bool neg_mode" and "unsigned int neg_mode"?
Russell King (Oracle) June 20, 2023, 3:42 p.m. UTC | #4 
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.
Russell King (Oracle) June 20, 2023, 3:51 p.m. UTC | #5 
On Tue, Jun 20, 2023 at 02:37:30PM +0300, Vladimir Oltean wrote:
> On Fri, Jun 16, 2023 at 01:06:22PM +0100, Russell King (Oracle) wrote:
> > @@ -443,6 +526,7 @@ struct phylink_pcs_ops;
> >   */
> >  struct phylink_pcs {
> >  	const struct phylink_pcs_ops *ops;
> > +	bool neg_mode;
> >  	bool poll;
> >  };
> 
> I deleted one of my own comments while trimming the email... Yay me :)
> 
> Would it be more appropriate to name this "bool pass_neg_mode" to avoid
> a naming collision between "bool neg_mode" and "unsigned int neg_mode"?

I'd entertain "want_neg_mode" but I don't think there's much scope for
confusion between the two - PCS drivers only get to set this flag
during the creation of the PCS.

In any case, I don't want this "neg_mode" to hang around for ages
and I see it as a transitory mechanism that will go away in a max of
a couple of kernel releases, especially as this patch set ensures
that all current users are converted. At the moment, it will catch
the case where some new PCS driver gets merged that doesn't use the
new "neg_mode" (and thus doesn't set this boolean flag.)

I know it's heresy, but it also helps trees like OpenWRT deal with
the interface change - which after all will _not_ generate any
compiler diagnostics between a converted PCS driver and an
unconverted PCS driver.
diff mbox series

Patch

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 1ae7868d2137..e2169ca00979 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -71,6 +71,7 @@  struct phylink {
 	struct mutex state_mutex;
 	struct phylink_link_state phy_state;
 	struct work_struct resolve;
+	unsigned int pcs_neg_mode;
 
 	bool mac_link_dropped;
 	bool using_mac_select_pcs;
@@ -992,23 +993,23 @@  static void phylink_resolve_an_pause(struct phylink_link_state *state)
 	}
 }
 
-static int phylink_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
+static int phylink_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
 			      const struct phylink_link_state *state,
 			      bool permit_pause_to_mac)
 {
 	if (!pcs)
 		return 0;
 
-	return pcs->ops->pcs_config(pcs, mode, state->interface,
+	return pcs->ops->pcs_config(pcs, neg_mode, state->interface,
 				    state->advertising, permit_pause_to_mac);
 }
 
-static void phylink_pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
+static void phylink_pcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
 				phy_interface_t interface, int speed,
 				int duplex)
 {
 	if (pcs && pcs->ops->pcs_link_up)
-		pcs->ops->pcs_link_up(pcs, mode, interface, speed, duplex);
+		pcs->ops->pcs_link_up(pcs, neg_mode, interface, speed, duplex);
 }
 
 static void phylink_pcs_poll_stop(struct phylink *pl)
@@ -1058,10 +1059,15 @@  static void phylink_major_config(struct phylink *pl, bool restart,
 	struct phylink_pcs *pcs = NULL;
 	bool pcs_changed = false;
 	unsigned int rate_kbd;
+	unsigned int neg_mode;
 	int err;
 
 	phylink_dbg(pl, "major config %s\n", phy_modes(state->interface));
 
+	pl->pcs_neg_mode = phylink_pcs_neg_mode(pl->cur_link_an_mode,
+						state->interface,
+						state->advertising);
+
 	if (pl->using_mac_select_pcs) {
 		pcs = pl->mac_ops->mac_select_pcs(pl->config, state->interface);
 		if (IS_ERR(pcs)) {
@@ -1094,9 +1100,12 @@  static void phylink_major_config(struct phylink *pl, bool restart,
 
 	phylink_mac_config(pl, state);
 
-	err = phylink_pcs_config(pl->pcs, pl->cur_link_an_mode, state,
-				 !!(pl->link_config.pause &
-				    MLO_PAUSE_AN));
+	neg_mode = pl->cur_link_an_mode;
+	if (pl->pcs && pl->pcs->neg_mode)
+		neg_mode = pl->pcs_neg_mode;
+
+	err = phylink_pcs_config(pl->pcs, neg_mode, state,
+				 !!(pl->link_config.pause & MLO_PAUSE_AN));
 	if (err < 0)
 		phylink_err(pl, "pcs_config failed: %pe\n",
 			    ERR_PTR(err));
@@ -1131,6 +1140,7 @@  static void phylink_major_config(struct phylink *pl, bool restart,
  */
 static int phylink_change_inband_advert(struct phylink *pl)
 {
+	unsigned int neg_mode;
 	int ret;
 
 	if (test_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state))
@@ -1149,12 +1159,20 @@  static int phylink_change_inband_advert(struct phylink *pl)
 		    __ETHTOOL_LINK_MODE_MASK_NBITS, pl->link_config.advertising,
 		    pl->link_config.pause);
 
+	/* Recompute the PCS neg mode */
+	pl->pcs_neg_mode = phylink_pcs_neg_mode(pl->cur_link_an_mode,
+					pl->link_config.interface,
+					pl->link_config.advertising);
+
+	neg_mode = pl->cur_link_an_mode;
+	if (pl->pcs->neg_mode)
+		neg_mode = pl->pcs_neg_mode;
+
 	/* Modern PCS-based method; update the advert at the PCS, and
 	 * restart negotiation if the pcs_config() helper indicates that
 	 * the programmed advertisement has changed.
 	 */
-	ret = phylink_pcs_config(pl->pcs, pl->cur_link_an_mode,
-				 &pl->link_config,
+	ret = phylink_pcs_config(pl->pcs, neg_mode, &pl->link_config,
 				 !!(pl->link_config.pause & MLO_PAUSE_AN));
 	if (ret < 0)
 		return ret;
@@ -1257,6 +1275,7 @@  static void phylink_link_up(struct phylink *pl,
 			    struct phylink_link_state link_state)
 {
 	struct net_device *ndev = pl->netdev;
+	unsigned int neg_mode;
 	int speed, duplex;
 	bool rx_pause;
 
@@ -1287,8 +1306,12 @@  static void phylink_link_up(struct phylink *pl,
 
 	pl->cur_interface = link_state.interface;
 
-	phylink_pcs_link_up(pl->pcs, pl->cur_link_an_mode, pl->cur_interface,
-			    speed, duplex);
+	neg_mode = pl->cur_link_an_mode;
+	if (pl->pcs && pl->pcs->neg_mode)
+		neg_mode = pl->pcs_neg_mode;
+
+	phylink_pcs_link_up(pl->pcs, neg_mode, pl->cur_interface, speed,
+			    duplex);
 
 	pl->mac_ops->mac_link_up(pl->config, pl->phydev, pl->cur_link_an_mode,
 				 pl->cur_interface, speed, duplex,
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
+	 *  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),
+	PHYLINK_PCS_NEG_OUTBAND = BIT(5),
+	PHYLINK_PCS_NEG_INBAND = BIT(6),
+	PHYLINK_PCS_NEG_INBAND_DISABLED = PHYLINK_PCS_NEG_INBAND,
+	PHYLINK_PCS_NEG_INBAND_ENABLED = PHYLINK_PCS_NEG_INBAND |
+					 PHYLINK_PCS_NEG_ENABLED,
+
 	/* MAC_SYM_PAUSE and MAC_ASYM_PAUSE are used when configuring our
 	 * autonegotiation advertisement. They correspond to the PAUSE and
 	 * ASM_DIR bits defined by 802.3, respectively.
@@ -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
+ * @mode: one of %MLO_AN_FIXED, %MLO_AN_PHY, %MLO_AN_INBAND.
+ * @interface: interface mode to be used
+ * @advertising: adertisement ethtool link mode mask
+ *
+ * Determines the negotiation mode to be used by the PCS, and returns
+ * one of:
+ * %PHYLINK_PCS_NEG_NONE: interface mode does not support inband
+ * %PHYLINK_PCS_NEG_OUTBAND: an out of band mode (e.g. reading the PHY)
+ *   will be used.
+ * %PHYLINK_PCS_NEG_INBAND_DISABLED: inband mode selected but autoneg disabled
+ * %PHYLINK_PCS_NEG_INBAND_ENABLED: inband mode selected and autoneg enabled
+ *
+ * Note: this is for cases where the PCS itself is involved in negotiation
+ * (e.g. Clause 37, SGMII and similar) not Clause 73.
+ */
+static inline unsigned int phylink_pcs_neg_mode(unsigned int mode,
+						phy_interface_t interface,
+						const unsigned long *advertising)
+{
+	unsigned int neg_mode;
+
+	switch (interface) {
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_QSGMII:
+	case PHY_INTERFACE_MODE_QUSGMII:
+	case PHY_INTERFACE_MODE_USXGMII:
+		/* These protocols are designed for use with a PHY which
+		 * communicates its negotiation result back to the MAC via
+		 * inband communication. Note: there exist PHYs that run
+		 * with SGMII but do not send the inband data.
+		 */
+		if (!phylink_autoneg_inband(mode))
+			neg_mode = PHYLINK_PCS_NEG_OUTBAND;
+		else
+			neg_mode = PHYLINK_PCS_NEG_INBAND_ENABLED;
+		break;
+
+	case PHY_INTERFACE_MODE_1000BASEX:
+	case PHY_INTERFACE_MODE_2500BASEX:
+		/* 1000base-X is designed for use media-side for Fibre
+		 * connections, and thus the Autoneg bit needs to be
+		 * taken into account. We also do this for 2500base-X
+		 * as well, but drivers may not support this, so may
+		 * need to override this.
+		 */
+		if (!phylink_autoneg_inband(mode))
+			neg_mode = PHYLINK_PCS_NEG_OUTBAND;
+		else if (linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
+					   advertising))
+			neg_mode = PHYLINK_PCS_NEG_INBAND_ENABLED;
+		else
+			neg_mode = PHYLINK_PCS_NEG_INBAND_DISABLED;
+		break;
+
+	default:
+		neg_mode = PHYLINK_PCS_NEG_NONE;
+		break;
+	}
+
+	return neg_mode;
+}
+
 /**
  * struct phylink_link_state - link state structure
  * @advertising: ethtool bitmask containing advertised link modes
@@ -436,6 +518,7 @@  struct phylink_pcs_ops;
 /**
  * struct phylink_pcs - PHYLINK PCS instance
  * @ops: a pointer to the &struct phylink_pcs_ops structure
+ * @neg_mode: provide PCS neg mode via "mode" argument
  * @poll: poll the PCS for link changes
  *
  * This structure is designed to be embedded within the PCS private data,
@@ -443,6 +526,7 @@  struct phylink_pcs_ops;
  */
 struct phylink_pcs {
 	const struct phylink_pcs_ops *ops;
+	bool neg_mode;
 	bool poll;
 };
 
@@ -460,12 +544,12 @@  struct phylink_pcs_ops {
 			    const struct phylink_link_state *state);
 	void (*pcs_get_state)(struct phylink_pcs *pcs,
 			      struct phylink_link_state *state);
-	int (*pcs_config)(struct phylink_pcs *pcs, unsigned int mode,
+	int (*pcs_config)(struct phylink_pcs *pcs, unsigned int neg_mode,
 			  phy_interface_t interface,
 			  const unsigned long *advertising,
 			  bool permit_pause_to_mac);
 	void (*pcs_an_restart)(struct phylink_pcs *pcs);
-	void (*pcs_link_up)(struct phylink_pcs *pcs, unsigned int mode,
+	void (*pcs_link_up)(struct phylink_pcs *pcs, unsigned int neg_mode,
 			    phy_interface_t interface, int speed, int duplex);
 };
 
@@ -508,7 +592,7 @@  void pcs_get_state(struct phylink_pcs *pcs,
 /**
  * pcs_config() - Configure the PCS mode and advertisement
  * @pcs: a pointer to a &struct phylink_pcs.
- * @mode: one of %MLO_AN_FIXED, %MLO_AN_PHY, %MLO_AN_INBAND.
+ * @neg_mode: link negotiation mode (see below)
  * @interface: interface mode to be used
  * @advertising: adertisement ethtool link mode mask
  * @permit_pause_to_mac: permit forwarding pause resolution to MAC
@@ -526,8 +610,12 @@  void pcs_get_state(struct phylink_pcs *pcs,
  * For 1000BASE-X, the advertisement should be programmed into the PCS.
  *
  * For most 10GBASE-R, there is no advertisement.
+ *
+ * The %neg_mode argument should be tested via the phylink_mode_*() family of
+ * functions, or for PCS that set pcs->neg_mode true, should be tested
+ * against the %PHYLINK_PCS_NEG_* definitions.
  */
-int pcs_config(struct phylink_pcs *pcs, unsigned int mode,
+int pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
 	       phy_interface_t interface, const unsigned long *advertising,
 	       bool permit_pause_to_mac);
 
@@ -543,7 +631,7 @@  void pcs_an_restart(struct phylink_pcs *pcs);
 /**
  * pcs_link_up() - program the PCS for the resolved link configuration
  * @pcs: a pointer to a &struct phylink_pcs.
- * @mode: link autonegotiation mode
+ * @neg_mode: link negotiation mode (see below)
  * @interface: link &typedef phy_interface_t mode
  * @speed: link speed
  * @duplex: link duplex
@@ -552,8 +640,12 @@  void pcs_an_restart(struct phylink_pcs *pcs);
  * the resolved link parameters. For example, a PCS operating in SGMII
  * mode without in-band AN needs to be manually configured for the link
  * and duplex setting. Otherwise, this should be a no-op.
+ *
+ * The %mode argument should be tested via the phylink_mode_*() family of
+ * functions, or for PCS that set pcs->neg_mode true, should be tested
+ * against the %PHYLINK_PCS_NEG_* definitions.
  */
-void pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
+void pcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
 		 phy_interface_t interface, int speed, int duplex);
 #endif