[PATCH net-next 0/15] Add and use helper for PCS negotiation modes

[PATCH net-next 0/15] Add and use helper for PCS negotiation modes

[Russell King (Oracle)]

Hi,

Earlier this month, I proposed a helper for deciding whether a PCS
should use inband negotiation modes or not. There was some discussion
around this topic, and I believe there was no disagreement about
providing the helper.

The initial discussion can be found at:

https://lore.kernel.org/r/ZGIkGmyL8yL1q1zp@shell.armlinux.org.uk

Subsequently, I posted a RFC series back in May:

https://lore.kernel.org/r/ZGzhvePzPjJ0v2En@shell.armlinux.org.uk

that added a helper, phylink_pcs_neg_mode() which PCS drivers could use
to parse the state, and updated a bunch of drivers to use it. I got
a couple of bits of feedback to it, including some ACKs.

However, I've decided to take this slightly further and change the
"mode" parameter to both the pcs_config() and pcs_link_up() methods
when a PCS driver opts in to this (by setting "neg_mode" in the
phylink_pcs structure.) If this is not set, we default to the old
behaviour. That said, this series converts all the PCS implementations
I can find currently in net-next.

Doing this has the added benefit that the negotiation mode parameter
is also available to the pcs_link_up() function, which can now know
whether inband negotiation was in fact enabled or not at pcs_config()
time.

It has been posted as RFC at:

https://lore.kernel.org/r/ZIh/CLQ3z89g0Ua0@shell.armlinux.org.uk

and received one reply, thanks Elad, which is a similar amount of
interest to previous postings. Let's post it as non-RFC and see
whether we get more reaction.

 drivers/net/dsa/qca/qca8k-8xxx.c                   |  13 ++-
 drivers/net/dsa/sja1105/sja1105_main.c             |  14 ++-
 drivers/net/ethernet/freescale/fman/fman_dtsec.c   |   7 +-
 drivers/net/ethernet/marvell/mvneta.c              |   7 +-
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c    |  14 +--
 .../net/ethernet/marvell/prestera/prestera_main.c  |  11 +--
 .../net/ethernet/microchip/lan966x/lan966x_main.c  |   1 +
 .../ethernet/microchip/lan966x/lan966x_phylink.c   |   7 +-
 .../net/ethernet/microchip/sparx5/sparx5_main.c    |   1 +
 .../net/ethernet/microchip/sparx5/sparx5_phylink.c |   8 +-
 drivers/net/ethernet/xilinx/xilinx_axienet_main.c  |   6 +-
 drivers/net/pcs/pcs-lynx.c                         |  48 +++++----
 drivers/net/pcs/pcs-mtk-lynxi.c                    |  39 +++-----
 drivers/net/pcs/pcs-xpcs.c                         |  43 ++++----
 drivers/net/phy/phylink.c                          |  59 +++++++----
 include/linux/pcs/pcs-xpcs.h                       |   4 +-
 include/linux/phylink.h                            | 109 +++++++++++++++++++--
 17 files changed, 253 insertions(+), 138 deletions(-)

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

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

[Russell King (Oracle)]

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(-)

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

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

[Simon Horman]

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;

...

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

[Vladimir Oltean]

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"?

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

[Russell King (Oracle)]

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!

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

[Vladimir Oltean]

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"?

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

[Russell King (Oracle)]

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.

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

[PATCH net-next 02/15] net: phylink: convert phylink_mii_c22_pcs_config() to neg_mode

[Russell King (Oracle)]

Use phylink_pcs_neg_mode() for phylink_mii_c22_pcs_config(). This
results in no functional change.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index e2169ca00979..601d64f57e33 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -3521,6 +3521,7 @@ int phylink_mii_c22_pcs_config(struct mdio_device *pcs, unsigned int mode,
 			       phy_interface_t interface,
 			       const unsigned long *advertising)
 {
+	unsigned int neg_mode;
 	bool changed = 0;
 	u16 bmcr;
 	int ret, adv;
@@ -3534,15 +3535,13 @@ int phylink_mii_c22_pcs_config(struct mdio_device *pcs, unsigned int mode,
 		changed = ret;
 	}
 
-	/* Ensure ISOLATE bit is disabled */
-	if (mode == MLO_AN_INBAND &&
-	    (interface == PHY_INTERFACE_MODE_SGMII ||
-	     interface == PHY_INTERFACE_MODE_QSGMII ||
-	     linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, advertising)))
+	neg_mode = phylink_pcs_neg_mode(mode, interface, advertising);
+	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
 		bmcr = BMCR_ANENABLE;
 	else
 		bmcr = 0;
 
+	/* Configure the inband state. Ensure ISOLATE bit is disabled */
 	ret = mdiodev_modify(pcs, MII_BMCR, BMCR_ANENABLE | BMCR_ISOLATE, bmcr);
 	if (ret < 0)
 		return ret;
-- 
2.30.2

[PATCH net-next 03/15] net: phylink: pass neg_mode into phylink_mii_c22_pcs_config()

[Russell King (Oracle)]

Convert fman_dtsec, xilinx_axienet and pcs-lynx to pass the neg_mode
into phylink_mii_c22_pcs_config(). Where appropriate, drivers are
updated to have neg_mode passed into their pcs_config() and
pcs_link_up() functions. For other drivers, we just hoist the call
to phylink_pcs_neg_mode() to their pcs_config() method out of
phylink_mii_c22_pcs_config().

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 .../net/ethernet/freescale/fman/fman_dtsec.c   |  7 ++++---
 .../net/ethernet/xilinx/xilinx_axienet_main.c  |  6 ++++--
 drivers/net/pcs/pcs-lynx.c                     | 18 ++++++++++++------
 drivers/net/phy/phylink.c                      |  9 ++++-----
 include/linux/phylink.h                        |  5 +++--
 5 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fman/fman_dtsec.c b/drivers/net/ethernet/freescale/fman/fman_dtsec.c
index d528ca681b6f..3088da7adf0f 100644
--- a/drivers/net/ethernet/freescale/fman/fman_dtsec.c
+++ b/drivers/net/ethernet/freescale/fman/fman_dtsec.c
@@ -763,15 +763,15 @@ static void dtsec_pcs_get_state(struct phylink_pcs *pcs,
 	phylink_mii_c22_pcs_get_state(dtsec->tbidev, state);
 }
 
-static int dtsec_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
+static int dtsec_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
 			    phy_interface_t interface,
 			    const unsigned long *advertising,
 			    bool permit_pause_to_mac)
 {
 	struct fman_mac *dtsec = pcs_to_dtsec(pcs);
 
-	return phylink_mii_c22_pcs_config(dtsec->tbidev, mode, interface,
-					  advertising);
+	return phylink_mii_c22_pcs_config(dtsec->tbidev, interface,
+					  advertising, neg_mode);
 }
 
 static void dtsec_pcs_an_restart(struct phylink_pcs *pcs)
@@ -1447,6 +1447,7 @@ int dtsec_initialization(struct mac_device *mac_dev,
 		goto _return_fm_mac_free;
 	}
 	dtsec->pcs.ops = &dtsec_pcs_ops;
+	dtsec->pcs.neg_mode = true;
 	dtsec->pcs.poll = true;
 
 	supported = mac_dev->phylink_config.supported_interfaces;
diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 3e310b55bce2..ae7b9af7b7d7 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -1631,7 +1631,7 @@ static void axienet_pcs_an_restart(struct phylink_pcs *pcs)
 	phylink_mii_c22_pcs_an_restart(pcs_phy);
 }
 
-static int axienet_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
+static int axienet_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
 			      phy_interface_t interface,
 			      const unsigned long *advertising,
 			      bool permit_pause_to_mac)
@@ -1653,7 +1653,8 @@ static int axienet_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
 		}
 	}
 
-	ret = phylink_mii_c22_pcs_config(pcs_phy, mode, interface, advertising);
+	ret = phylink_mii_c22_pcs_config(pcs_phy, interface, advertising,
+					 neg_mode);
 	if (ret < 0)
 		netdev_warn(ndev, "Failed to configure PCS: %d\n", ret);
 
@@ -2129,6 +2130,7 @@ static int axienet_probe(struct platform_device *pdev)
 		}
 		of_node_put(np);
 		lp->pcs.ops = &axienet_pcs_ops;
+		lp->pcs.neg_mode = true;
 		lp->pcs.poll = true;
 	}
 
diff --git a/drivers/net/pcs/pcs-lynx.c b/drivers/net/pcs/pcs-lynx.c
index fca48ebf0b81..25bd4b45eb7b 100644
--- a/drivers/net/pcs/pcs-lynx.c
+++ b/drivers/net/pcs/pcs-lynx.c
@@ -112,9 +112,10 @@ static void lynx_pcs_get_state(struct phylink_pcs *pcs,
 		state->link, state->an_complete);
 }
 
-static int lynx_pcs_config_giga(struct mdio_device *pcs, unsigned int mode,
+static int lynx_pcs_config_giga(struct mdio_device *pcs,
 				phy_interface_t interface,
-				const unsigned long *advertising)
+				const unsigned long *advertising,
+				unsigned int neg_mode)
 {
 	int link_timer_ns;
 	u32 link_timer;
@@ -132,8 +133,9 @@ static int lynx_pcs_config_giga(struct mdio_device *pcs, unsigned int mode,
 	if (interface == PHY_INTERFACE_MODE_1000BASEX) {
 		if_mode = 0;
 	} else {
+		/* SGMII and QSGMII */
 		if_mode = IF_MODE_SGMII_EN;
-		if (mode == MLO_AN_INBAND)
+		if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
 			if_mode |= IF_MODE_USE_SGMII_AN;
 	}
 
@@ -143,7 +145,8 @@ static int lynx_pcs_config_giga(struct mdio_device *pcs, unsigned int mode,
 	if (err)
 		return err;
 
-	return phylink_mii_c22_pcs_config(pcs, mode, interface, advertising);
+	return phylink_mii_c22_pcs_config(pcs, interface, advertising,
+					  neg_mode);
 }
 
 static int lynx_pcs_config_usxgmii(struct mdio_device *pcs, unsigned int mode,
@@ -170,13 +173,16 @@ static int lynx_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
 			   bool permit)
 {
 	struct lynx_pcs *lynx = phylink_pcs_to_lynx(pcs);
+	unsigned int neg_mode;
+
+	neg_mode = phylink_pcs_neg_mode(mode, ifmode, advertising);
 
 	switch (ifmode) {
 	case PHY_INTERFACE_MODE_1000BASEX:
 	case PHY_INTERFACE_MODE_SGMII:
 	case PHY_INTERFACE_MODE_QSGMII:
-		return lynx_pcs_config_giga(lynx->mdio, mode, ifmode,
-					    advertising);
+		return lynx_pcs_config_giga(lynx->mdio, ifmode, advertising,
+					    neg_mode);
 	case PHY_INTERFACE_MODE_2500BASEX:
 		if (phylink_autoneg_inband(mode)) {
 			dev_err(&lynx->mdio->dev,
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 601d64f57e33..414508ed5512 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -3508,20 +3508,20 @@ EXPORT_SYMBOL_GPL(phylink_mii_c22_pcs_encode_advertisement);
 /**
  * phylink_mii_c22_pcs_config() - configure clause 22 PCS
  * @pcs: a pointer to a &struct mdio_device.
- * @mode: link autonegotiation mode
  * @interface: the PHY interface mode being configured
  * @advertising: the ethtool advertisement mask
+ * @neg_mode: PCS negotiation mode
  *
  * Configure a Clause 22 PCS PHY with the appropriate negotiation
  * parameters for the @mode, @interface and @advertising parameters.
  * Returns negative error number on failure, zero if the advertisement
  * has not changed, or positive if there is a change.
  */
-int phylink_mii_c22_pcs_config(struct mdio_device *pcs, unsigned int mode,
+int phylink_mii_c22_pcs_config(struct mdio_device *pcs,
 			       phy_interface_t interface,
-			       const unsigned long *advertising)
+			       const unsigned long *advertising,
+			       unsigned int neg_mode)
 {
-	unsigned int neg_mode;
 	bool changed = 0;
 	u16 bmcr;
 	int ret, adv;
@@ -3535,7 +3535,6 @@ int phylink_mii_c22_pcs_config(struct mdio_device *pcs, unsigned int mode,
 		changed = ret;
 	}
 
-	neg_mode = phylink_pcs_neg_mode(mode, interface, advertising);
 	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
 		bmcr = BMCR_ANENABLE;
 	else
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 2b322d7fa51a..516240f1e950 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -743,9 +743,10 @@ void phylink_mii_c22_pcs_get_state(struct mdio_device *pcs,
 				   struct phylink_link_state *state);
 int phylink_mii_c22_pcs_encode_advertisement(phy_interface_t interface,
 					     const unsigned long *advertising);
-int phylink_mii_c22_pcs_config(struct mdio_device *pcs, unsigned int mode,
+int phylink_mii_c22_pcs_config(struct mdio_device *pcs,
 			       phy_interface_t interface,
-			       const unsigned long *advertising);
+			       const unsigned long *advertising,
+			       unsigned int neg_mode);
 void phylink_mii_c22_pcs_an_restart(struct mdio_device *pcs);
 
 void phylink_resolve_c73(struct phylink_link_state *state);
-- 
2.30.2

[PATCH net-next 04/15] net: pcs: xpcs: update PCS driver to use neg_mode

[Russell King (Oracle)]

Update xpcs to use neg_mode to configure whether inband negotiation
should be used. We need to update sja1105 as well as that directly
calls into the XPCS driver's config function.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/sja1105/sja1105_main.c | 14 ++++-----
 drivers/net/pcs/pcs-xpcs.c             | 43 ++++++++++++++------------
 include/linux/pcs/pcs-xpcs.h           |  4 +--
 3 files changed, 31 insertions(+), 30 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index b70dcf32a26d..a55a6436fc05 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -2314,7 +2314,7 @@ int sja1105_static_config_reload(struct sja1105_private *priv,
 
 	for (i = 0; i < ds->num_ports; i++) {
 		struct dw_xpcs *xpcs = priv->xpcs[i];
-		unsigned int mode;
+		unsigned int neg_mode;
 
 		rc = sja1105_adjust_port_config(priv, i, speed_mbps[i]);
 		if (rc < 0)
@@ -2324,17 +2324,15 @@ int sja1105_static_config_reload(struct sja1105_private *priv,
 			continue;
 
 		if (bmcr[i] & BMCR_ANENABLE)
-			mode = MLO_AN_INBAND;
-		else if (priv->fixed_link[i])
-			mode = MLO_AN_FIXED;
+			neg_mode = PHYLINK_PCS_NEG_INBAND_ENABLED;
 		else
-			mode = MLO_AN_PHY;
+			neg_mode = PHYLINK_PCS_NEG_OUTBAND;
 
-		rc = xpcs_do_config(xpcs, priv->phy_mode[i], mode, NULL);
+		rc = xpcs_do_config(xpcs, priv->phy_mode[i], NULL, neg_mode);
 		if (rc < 0)
 			goto out;
 
-		if (!phylink_autoneg_inband(mode)) {
+		if (neg_mode == PHYLINK_PCS_NEG_OUTBAND) {
 			int speed = SPEED_UNKNOWN;
 
 			if (priv->phy_mode[i] == PHY_INTERFACE_MODE_2500BASEX)
@@ -2346,7 +2344,7 @@ int sja1105_static_config_reload(struct sja1105_private *priv,
 			else
 				speed = SPEED_10;
 
-			xpcs_link_up(&xpcs->pcs, mode, priv->phy_mode[i],
+			xpcs_link_up(&xpcs->pcs, neg_mode, priv->phy_mode[i],
 				     speed, DUPLEX_FULL);
 		}
 	}
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index e4e59aa9faf7..44b037646865 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -657,7 +657,8 @@ int xpcs_config_eee(struct dw_xpcs *xpcs, int mult_fact_100ns, int enable)
 }
 EXPORT_SYMBOL_GPL(xpcs_config_eee);
 
-static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode)
+static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs,
+				      unsigned int neg_mode)
 {
 	int ret, mdio_ctrl;
 
@@ -707,7 +708,7 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode)
 	if (ret < 0)
 		return ret;
 
-	if (phylink_autoneg_inband(mode))
+	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
 		ret |= DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW;
 	else
 		ret &= ~DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW;
@@ -716,14 +717,15 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode)
 	if (ret < 0)
 		return ret;
 
-	if (phylink_autoneg_inband(mode))
+	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
 		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL,
 				 mdio_ctrl | AN_CL37_EN);
 
 	return ret;
 }
 
-static int xpcs_config_aneg_c37_1000basex(struct dw_xpcs *xpcs, unsigned int mode,
+static int xpcs_config_aneg_c37_1000basex(struct dw_xpcs *xpcs,
+					  unsigned int neg_mode,
 					  const unsigned long *advertising)
 {
 	phy_interface_t interface = PHY_INTERFACE_MODE_1000BASEX;
@@ -774,8 +776,7 @@ static int xpcs_config_aneg_c37_1000basex(struct dw_xpcs *xpcs, unsigned int mod
 	if (ret < 0)
 		return ret;
 
-	if (phylink_autoneg_inband(mode) &&
-	    linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, advertising)) {
+	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {
 		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL,
 				 mdio_ctrl | AN_CL37_EN);
 		if (ret < 0)
@@ -808,7 +809,7 @@ static int xpcs_config_2500basex(struct dw_xpcs *xpcs)
 }
 
 int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface,
-		   unsigned int mode, const unsigned long *advertising)
+		   const unsigned long *advertising, unsigned int neg_mode)
 {
 	const struct xpcs_compat *compat;
 	int ret;
@@ -821,19 +822,19 @@ int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface,
 	case DW_10GBASER:
 		break;
 	case DW_AN_C73:
-		if (test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, advertising)) {
+		if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {
 			ret = xpcs_config_aneg_c73(xpcs, compat);
 			if (ret)
 				return ret;
 		}
 		break;
 	case DW_AN_C37_SGMII:
-		ret = xpcs_config_aneg_c37_sgmii(xpcs, mode);
+		ret = xpcs_config_aneg_c37_sgmii(xpcs, neg_mode);
 		if (ret)
 			return ret;
 		break;
 	case DW_AN_C37_1000BASEX:
-		ret = xpcs_config_aneg_c37_1000basex(xpcs, mode,
+		ret = xpcs_config_aneg_c37_1000basex(xpcs, neg_mode,
 						     advertising);
 		if (ret)
 			return ret;
@@ -857,14 +858,14 @@ int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface,
 }
 EXPORT_SYMBOL_GPL(xpcs_do_config);
 
-static int xpcs_config(struct phylink_pcs *pcs, unsigned int mode,
+static int xpcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
 		       phy_interface_t interface,
 		       const unsigned long *advertising,
 		       bool permit_pause_to_mac)
 {
 	struct dw_xpcs *xpcs = phylink_pcs_to_xpcs(pcs);
 
-	return xpcs_do_config(xpcs, interface, mode, advertising);
+	return xpcs_do_config(xpcs, interface, advertising, neg_mode);
 }
 
 static int xpcs_get_state_c73(struct dw_xpcs *xpcs,
@@ -898,7 +899,8 @@ static int xpcs_get_state_c73(struct dw_xpcs *xpcs,
 
 		state->link = 0;
 
-		return xpcs_do_config(xpcs, state->interface, MLO_AN_INBAND, NULL);
+		return xpcs_do_config(xpcs, state->interface, NULL,
+				      PHYLINK_PCS_NEG_INBAND_ENABLED);
 	}
 
 	/* There is no point doing anything else if the link is down. */
@@ -1046,12 +1048,12 @@ static void xpcs_get_state(struct phylink_pcs *pcs,
 	}
 }
 
-static void xpcs_link_up_sgmii(struct dw_xpcs *xpcs, unsigned int mode,
+static void xpcs_link_up_sgmii(struct dw_xpcs *xpcs, unsigned int neg_mode,
 			       int speed, int duplex)
 {
 	int val, ret;
 
-	if (phylink_autoneg_inband(mode))
+	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
 		return;
 
 	val = mii_bmcr_encode_fixed(speed, duplex);
@@ -1060,12 +1062,12 @@ static void xpcs_link_up_sgmii(struct dw_xpcs *xpcs, unsigned int mode,
 		pr_err("%s: xpcs_write returned %pe\n", __func__, ERR_PTR(ret));
 }
 
-static void xpcs_link_up_1000basex(struct dw_xpcs *xpcs, unsigned int mode,
+static void xpcs_link_up_1000basex(struct dw_xpcs *xpcs, unsigned int neg_mode,
 				   int speed, int duplex)
 {
 	int val, ret;
 
-	if (phylink_autoneg_inband(mode))
+	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
 		return;
 
 	switch (speed) {
@@ -1089,7 +1091,7 @@ static void xpcs_link_up_1000basex(struct dw_xpcs *xpcs, unsigned int mode,
 		pr_err("%s: xpcs_write returned %pe\n", __func__, ERR_PTR(ret));
 }
 
-void xpcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
+void xpcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
 		  phy_interface_t interface, int speed, int duplex)
 {
 	struct dw_xpcs *xpcs = phylink_pcs_to_xpcs(pcs);
@@ -1097,9 +1099,9 @@ void xpcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
 	if (interface == PHY_INTERFACE_MODE_USXGMII)
 		return xpcs_config_usxgmii(xpcs, speed);
 	if (interface == PHY_INTERFACE_MODE_SGMII)
-		return xpcs_link_up_sgmii(xpcs, mode, speed, duplex);
+		return xpcs_link_up_sgmii(xpcs, neg_mode, speed, duplex);
 	if (interface == PHY_INTERFACE_MODE_1000BASEX)
-		return xpcs_link_up_1000basex(xpcs, mode, speed, duplex);
+		return xpcs_link_up_1000basex(xpcs, neg_mode, speed, duplex);
 }
 EXPORT_SYMBOL_GPL(xpcs_link_up);
 
@@ -1283,6 +1285,7 @@ static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
 		}
 
 		xpcs->pcs.ops = &xpcs_phylink_ops;
+		xpcs->pcs.neg_mode = true;
 		if (compat->an_mode == DW_10GBASER)
 			return xpcs;
 
diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h
index ec8175b847cc..ff99cf7a5d0d 100644
--- a/include/linux/pcs/pcs-xpcs.h
+++ b/include/linux/pcs/pcs-xpcs.h
@@ -29,10 +29,10 @@ struct dw_xpcs {
 };
 
 int xpcs_get_an_mode(struct dw_xpcs *xpcs, phy_interface_t interface);
-void xpcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
+void xpcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
 		  phy_interface_t interface, int speed, int duplex);
 int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface,
-		   unsigned int mode, const unsigned long *advertising);
+		   const unsigned long *advertising, unsigned int neg_mode);
 void xpcs_get_interfaces(struct dw_xpcs *xpcs, unsigned long *interfaces);
 int xpcs_config_eee(struct dw_xpcs *xpcs, int mult_fact_100ns,
 		    int enable);
-- 
2.30.2

[PATCH net-next 05/15] net: pcs: lynxi: update PCS driver to use neg_mode

[Russell King (Oracle)]

Update the Lynxi PCS driver to use neg_mode rather than the mode
argument. This ensures that the link_up() method will always program
the speed and duplex when negotiation is disabled.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/pcs/pcs-mtk-lynxi.c | 39 ++++++++++++++-------------------
 1 file changed, 16 insertions(+), 23 deletions(-)

diff --git a/drivers/net/pcs/pcs-mtk-lynxi.c b/drivers/net/pcs/pcs-mtk-lynxi.c
index 888452325edc..b0f3ede945d9 100644
--- a/drivers/net/pcs/pcs-mtk-lynxi.c
+++ b/drivers/net/pcs/pcs-mtk-lynxi.c
@@ -102,13 +102,13 @@ static void mtk_pcs_lynxi_get_state(struct phylink_pcs *pcs,
 					 FIELD_GET(SGMII_LPA, adv));
 }
 
-static int mtk_pcs_lynxi_config(struct phylink_pcs *pcs, unsigned int mode,
+static int mtk_pcs_lynxi_config(struct phylink_pcs *pcs, unsigned int neg_mode,
 				phy_interface_t interface,
 				const unsigned long *advertising,
 				bool permit_pause_to_mac)
 {
 	struct mtk_pcs_lynxi *mpcs = pcs_to_mtk_pcs_lynxi(pcs);
-	bool mode_changed = false, changed, use_an;
+	bool mode_changed = false, changed;
 	unsigned int rgc3, sgm_mode, bmcr;
 	int advertise, link_timer;
 
@@ -121,30 +121,21 @@ static int mtk_pcs_lynxi_config(struct phylink_pcs *pcs, unsigned int mode,
 	 * we assume that fixes it's speed at bitrate = line rate (in
 	 * other words, 1000Mbps or 2500Mbps).
 	 */
-	if (interface == PHY_INTERFACE_MODE_SGMII) {
+	if (interface == PHY_INTERFACE_MODE_SGMII)
 		sgm_mode = SGMII_IF_MODE_SGMII;
-		if (phylink_autoneg_inband(mode)) {
-			sgm_mode |= SGMII_REMOTE_FAULT_DIS |
-				    SGMII_SPEED_DUPLEX_AN;
-			use_an = true;
-		} else {
-			use_an = false;
-		}
-	} else if (phylink_autoneg_inband(mode)) {
-		/* 1000base-X or 2500base-X autoneg */
-		sgm_mode = SGMII_REMOTE_FAULT_DIS;
-		use_an = linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
-					   advertising);
-	} else {
-		/* 1000base-X or 2500base-X without autoneg */
+	else
 		sgm_mode = 0;
-		use_an = false;
-	}
 
-	if (use_an)
+	if (neg_mode & PHYLINK_PCS_NEG_INBAND)
+		sgm_mode |= SGMII_REMOTE_FAULT_DIS;
+
+	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {
+		if (interface == PHY_INTERFACE_MODE_SGMII)
+			sgm_mode |= SGMII_SPEED_DUPLEX_AN;
 		bmcr = BMCR_ANENABLE;
-	else
+	} else {
 		bmcr = 0;
+	}
 
 	if (mpcs->interface != interface) {
 		link_timer = phylink_get_link_timer_ns(interface);
@@ -216,14 +207,15 @@ static void mtk_pcs_lynxi_restart_an(struct phylink_pcs *pcs)
 	regmap_set_bits(mpcs->regmap, SGMSYS_PCS_CONTROL_1, BMCR_ANRESTART);
 }
 
-static void mtk_pcs_lynxi_link_up(struct phylink_pcs *pcs, unsigned int mode,
+static void mtk_pcs_lynxi_link_up(struct phylink_pcs *pcs,
+				  unsigned int neg_mode,
 				  phy_interface_t interface, int speed,
 				  int duplex)
 {
 	struct mtk_pcs_lynxi *mpcs = pcs_to_mtk_pcs_lynxi(pcs);
 	unsigned int sgm_mode;
 
-	if (!phylink_autoneg_inband(mode)) {
+	if (neg_mode != PHYLINK_PCS_NEG_INBAND_ENABLED) {
 		/* Force the speed and duplex setting */
 		if (speed == SPEED_10)
 			sgm_mode = SGMII_SPEED_10;
@@ -286,6 +278,7 @@ struct phylink_pcs *mtk_pcs_lynxi_create(struct device *dev,
 	mpcs->regmap = regmap;
 	mpcs->flags = flags;
 	mpcs->pcs.ops = &mtk_pcs_lynxi_ops;
+	mpcs->pcs.neg_mode = true;
 	mpcs->pcs.poll = true;
 	mpcs->interface = PHY_INTERFACE_MODE_NA;
 
-- 
2.30.2

[PATCH net-next 06/15] net: pcs: lynx: update PCS driver to use neg_mode

[Russell King (Oracle)]

Update the Lynx PCS driver to use neg_mode rather than the mode
argument. This ensures that the link_up() method will always program
the speed and duplex when negotiation is disabled.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/pcs/pcs-lynx.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/net/pcs/pcs-lynx.c b/drivers/net/pcs/pcs-lynx.c
index 25bd4b45eb7b..9021b96d4f9d 100644
--- a/drivers/net/pcs/pcs-lynx.c
+++ b/drivers/net/pcs/pcs-lynx.c
@@ -149,13 +149,14 @@ static int lynx_pcs_config_giga(struct mdio_device *pcs,
 					  neg_mode);
 }
 
-static int lynx_pcs_config_usxgmii(struct mdio_device *pcs, unsigned int mode,
-				   const unsigned long *advertising)
+static int lynx_pcs_config_usxgmii(struct mdio_device *pcs,
+				   const unsigned long *advertising,
+				   unsigned int neg_mode)
 {
 	struct mii_bus *bus = pcs->bus;
 	int addr = pcs->addr;
 
-	if (!phylink_autoneg_inband(mode)) {
+	if (neg_mode != PHYLINK_PCS_NEG_INBAND_ENABLED) {
 		dev_err(&pcs->dev, "USXGMII only supports in-band AN for now\n");
 		return -EOPNOTSUPP;
 	}
@@ -167,15 +168,11 @@ static int lynx_pcs_config_usxgmii(struct mdio_device *pcs, unsigned int mode,
 				 ADVERTISE_SGMII | ADVERTISE_LPACK);
 }
 
-static int lynx_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
+static int lynx_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
 			   phy_interface_t ifmode,
-			   const unsigned long *advertising,
-			   bool permit)
+			   const unsigned long *advertising, bool permit)
 {
 	struct lynx_pcs *lynx = phylink_pcs_to_lynx(pcs);
-	unsigned int neg_mode;
-
-	neg_mode = phylink_pcs_neg_mode(mode, ifmode, advertising);
 
 	switch (ifmode) {
 	case PHY_INTERFACE_MODE_1000BASEX:
@@ -184,14 +181,15 @@ static int lynx_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
 		return lynx_pcs_config_giga(lynx->mdio, ifmode, advertising,
 					    neg_mode);
 	case PHY_INTERFACE_MODE_2500BASEX:
-		if (phylink_autoneg_inband(mode)) {
+		if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {
 			dev_err(&lynx->mdio->dev,
 				"AN not supported on 3.125GHz SerDes lane\n");
 			return -EOPNOTSUPP;
 		}
 		break;
 	case PHY_INTERFACE_MODE_USXGMII:
-		return lynx_pcs_config_usxgmii(lynx->mdio, mode, advertising);
+		return lynx_pcs_config_usxgmii(lynx->mdio, advertising,
+					       neg_mode);
 	case PHY_INTERFACE_MODE_10GBASER:
 		/* Nothing to do here for 10GBASER */
 		break;
@@ -209,7 +207,8 @@ static void lynx_pcs_an_restart(struct phylink_pcs *pcs)
 	phylink_mii_c22_pcs_an_restart(lynx->mdio);
 }
 
-static void lynx_pcs_link_up_sgmii(struct mdio_device *pcs, unsigned int mode,
+static void lynx_pcs_link_up_sgmii(struct mdio_device *pcs,
+				   unsigned int neg_mode,
 				   int speed, int duplex)
 {
 	u16 if_mode = 0, sgmii_speed;
@@ -217,7 +216,7 @@ static void lynx_pcs_link_up_sgmii(struct mdio_device *pcs, unsigned int mode,
 	/* The PCS needs to be configured manually only
 	 * when not operating on in-band mode
 	 */
-	if (mode == MLO_AN_INBAND)
+	if (neg_mode != PHYLINK_PCS_NEG_INBAND_ENABLED)
 		return;
 
 	if (duplex == DUPLEX_HALF)
@@ -264,12 +263,12 @@ static void lynx_pcs_link_up_sgmii(struct mdio_device *pcs, unsigned int mode,
  * 2500 Mbps and we do rate adaptation through pause frames.
  */
 static void lynx_pcs_link_up_2500basex(struct mdio_device *pcs,
-				       unsigned int mode,
+				       unsigned int neg_mode,
 				       int speed, int duplex)
 {
 	u16 if_mode = 0;
 
-	if (mode == MLO_AN_INBAND) {
+	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {
 		dev_err(&pcs->dev, "AN not supported for 2500BaseX\n");
 		return;
 	}
@@ -283,7 +282,7 @@ static void lynx_pcs_link_up_2500basex(struct mdio_device *pcs,
 		       if_mode);
 }
 
-static void lynx_pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
+static void lynx_pcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
 			     phy_interface_t interface,
 			     int speed, int duplex)
 {
@@ -292,10 +291,10 @@ static void lynx_pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
 	switch (interface) {
 	case PHY_INTERFACE_MODE_SGMII:
 	case PHY_INTERFACE_MODE_QSGMII:
-		lynx_pcs_link_up_sgmii(lynx->mdio, mode, speed, duplex);
+		lynx_pcs_link_up_sgmii(lynx->mdio, neg_mode, speed, duplex);
 		break;
 	case PHY_INTERFACE_MODE_2500BASEX:
-		lynx_pcs_link_up_2500basex(lynx->mdio, mode, speed, duplex);
+		lynx_pcs_link_up_2500basex(lynx->mdio, neg_mode, speed, duplex);
 		break;
 	case PHY_INTERFACE_MODE_USXGMII:
 		/* At the moment, only in-band AN is supported for USXGMII
@@ -325,6 +324,7 @@ static struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio)
 	mdio_device_get(mdio);
 	lynx->mdio = mdio;
 	lynx->pcs.ops = &lynx_pcs_phylink_ops;
+	lynx->pcs.neg_mode = true;
 	lynx->pcs.poll = true;
 
 	return lynx_to_phylink_pcs(lynx);
-- 
2.30.2

[PATCH net-next 07/15] net: lan966x: update PCS driver to use neg_mode

[Russell King (Oracle)]

Update lan966x's embedded PCS driver to use neg_mode rather than the
mode argument. As there is no pcs_link_up() method, this only affects
the pcs_config() method.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/microchip/lan966x/lan966x_main.c    | 1 +
 drivers/net/ethernet/microchip/lan966x/lan966x_phylink.c | 7 +++----
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
index f6931dfb3e68..fbb0bb4594cd 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
@@ -818,6 +818,7 @@ static int lan966x_probe_port(struct lan966x *lan966x, u32 p,
 	port->phylink_config.type = PHYLINK_NETDEV;
 	port->phylink_pcs.poll = true;
 	port->phylink_pcs.ops = &lan966x_phylink_pcs_ops;
+	port->phylink_pcs.neg_mode = true;
 
 	port->phylink_config.mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
 		MAC_10 | MAC_100 | MAC_1000FD | MAC_2500FD;
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_phylink.c b/drivers/net/ethernet/microchip/lan966x/lan966x_phylink.c
index c5f9803e6e63..1d63903f9006 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_phylink.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_phylink.c
@@ -95,8 +95,7 @@ static void lan966x_pcs_get_state(struct phylink_pcs *pcs,
 	lan966x_port_status_get(port, state);
 }
 
-static int lan966x_pcs_config(struct phylink_pcs *pcs,
-			      unsigned int mode,
+static int lan966x_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
 			      phy_interface_t interface,
 			      const unsigned long *advertising,
 			      bool permit_pause_to_mac)
@@ -107,8 +106,8 @@ static int lan966x_pcs_config(struct phylink_pcs *pcs,
 
 	config = port->config;
 	config.portmode = interface;
-	config.inband = phylink_autoneg_inband(mode);
-	config.autoneg = phylink_test(advertising, Autoneg);
+	config.inband = neg_mode & PHYLINK_PCS_NEG_INBAND;
+	config.autoneg = neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED;
 	config.advertising = advertising;
 
 	ret = lan966x_port_pcs_set(port, &config);
-- 
2.30.2

[PATCH net-next 08/15] net: mvneta: update PCS driver to use neg_mode

[Russell King (Oracle)]

Update mvneta's embedded PCS driver to use neg_mode rather than the
mode argument. As there is no pcs_link_up() method, this only affects
the pcs_config() method.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/marvell/mvneta.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index e2abc00d0472..ff5647bcdfca 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -4002,8 +4002,8 @@ static void mvneta_pcs_get_state(struct phylink_pcs *pcs,
 		state->pause |= MLO_PAUSE_TX;
 }
 
-static int mvneta_pcs_config(struct phylink_pcs *pcs,
-			     unsigned int mode, phy_interface_t interface,
+static int mvneta_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
+			     phy_interface_t interface,
 			     const unsigned long *advertising,
 			     bool permit_pause_to_mac)
 {
@@ -4016,7 +4016,7 @@ static int mvneta_pcs_config(struct phylink_pcs *pcs,
 	       MVNETA_GMAC_AN_FLOW_CTRL_EN |
 	       MVNETA_GMAC_AN_DUPLEX_EN;
 
-	if (phylink_autoneg_inband(mode)) {
+	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {
 		mask |= MVNETA_GMAC_CONFIG_MII_SPEED |
 			MVNETA_GMAC_CONFIG_GMII_SPEED |
 			MVNETA_GMAC_CONFIG_FULL_DUPLEX;
@@ -5518,6 +5518,7 @@ static int mvneta_probe(struct platform_device *pdev)
 		clk_prepare_enable(pp->clk_bus);
 
 	pp->phylink_pcs.ops = &mvneta_phylink_pcs_ops;
+	pp->phylink_pcs.neg_mode = true;
 
 	pp->phylink_config.dev = &dev->dev;
 	pp->phylink_config.type = PHYLINK_NETDEV;
-- 
2.30.2

[PATCH net-next 09/15] net: mvpp2: update PCS driver to use neg_mode

[Russell King (Oracle)]

Update mvpp2's embedded PCS drivers to use neg_mode rather than the
mode argument, remembering to update the ACPI path as well. As there
are no pcs_link_up() methods, this only affects the two pcs_config()
methods.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index adc953611913..1fec84b4c068 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -6168,8 +6168,7 @@ static void mvpp2_xlg_pcs_get_state(struct phylink_pcs *pcs,
 		state->pause |= MLO_PAUSE_RX;
 }
 
-static int mvpp2_xlg_pcs_config(struct phylink_pcs *pcs,
-				unsigned int mode,
+static int mvpp2_xlg_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
 				phy_interface_t interface,
 				const unsigned long *advertising,
 				bool permit_pause_to_mac)
@@ -6232,7 +6231,7 @@ static void mvpp2_gmac_pcs_get_state(struct phylink_pcs *pcs,
 		state->pause |= MLO_PAUSE_TX;
 }
 
-static int mvpp2_gmac_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
+static int mvpp2_gmac_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
 				 phy_interface_t interface,
 				 const unsigned long *advertising,
 				 bool permit_pause_to_mac)
@@ -6246,7 +6245,7 @@ static int mvpp2_gmac_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
 	       MVPP2_GMAC_FLOW_CTRL_AUTONEG |
 	       MVPP2_GMAC_AN_DUPLEX_EN;
 
-	if (phylink_autoneg_inband(mode)) {
+	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {
 		mask |= MVPP2_GMAC_CONFIG_MII_SPEED |
 			MVPP2_GMAC_CONFIG_GMII_SPEED |
 			MVPP2_GMAC_CONFIG_FULL_DUPLEX;
@@ -6649,8 +6648,9 @@ static void mvpp2_acpi_start(struct mvpp2_port *port)
 	mvpp2_mac_prepare(&port->phylink_config, MLO_AN_INBAND,
 			  port->phy_interface);
 	mvpp2_mac_config(&port->phylink_config, MLO_AN_INBAND, &state);
-	pcs->ops->pcs_config(pcs, MLO_AN_INBAND, port->phy_interface,
-			     state.advertising, false);
+	pcs->ops->pcs_config(pcs, PHYLINK_PCS_NEG_INBAND_ENABLED,
+			     port->phy_interface, state.advertising,
+			     false);
 	mvpp2_mac_finish(&port->phylink_config, MLO_AN_INBAND,
 			 port->phy_interface);
 	mvpp2_mac_link_up(&port->phylink_config, NULL,
@@ -6896,7 +6896,9 @@ static int mvpp2_port_probe(struct platform_device *pdev,
 	dev->dev.of_node = port_node;
 
 	port->pcs_gmac.ops = &mvpp2_phylink_gmac_pcs_ops;
+	port->pcs_gmac.neg_mode = true;
 	port->pcs_xlg.ops = &mvpp2_phylink_xlg_pcs_ops;
+	port->pcs_xlg.neg_mode = true;
 
 	if (!mvpp2_use_acpi_compat_mode(port_fwnode)) {
 		port->phylink_config.dev = &dev->dev;
-- 
2.30.2

[PATCH net-next 10/15] net: prestera: update PCS driver to use neg_mode

[Russell King (Oracle)]

Update prestera's embedded PCS driver to use neg_mode rather than the
mode argument. As there is no pcs_link_up() method, this only affects
the pcs_config() method.

Acked-by: Elad Nachman <enachman@marvell.com>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/marvell/prestera/prestera_main.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/marvell/prestera/prestera_main.c b/drivers/net/ethernet/marvell/prestera/prestera_main.c
index 9d504142e51a..4fb886c57cd7 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_main.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_main.c
@@ -300,8 +300,7 @@ static void prestera_pcs_get_state(struct phylink_pcs *pcs,
 	}
 }
 
-static int prestera_pcs_config(struct phylink_pcs *pcs,
-			       unsigned int mode,
+static int prestera_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
 			       phy_interface_t interface,
 			       const unsigned long *advertising,
 			       bool permit_pause_to_mac)
@@ -316,30 +315,25 @@ static int prestera_pcs_config(struct phylink_pcs *pcs,
 
 	cfg_mac.admin = true;
 	cfg_mac.fec = PRESTERA_PORT_FEC_OFF;
+	cfg_mac.inband = neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED;
 
 	switch (interface) {
 	case PHY_INTERFACE_MODE_10GBASER:
 		cfg_mac.speed = SPEED_10000;
-		cfg_mac.inband = 0;
 		cfg_mac.mode = PRESTERA_MAC_MODE_SR_LR;
 		break;
 	case PHY_INTERFACE_MODE_2500BASEX:
 		cfg_mac.speed = SPEED_2500;
 		cfg_mac.duplex = DUPLEX_FULL;
-		cfg_mac.inband = test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
-					  advertising);
 		cfg_mac.mode = PRESTERA_MAC_MODE_SGMII;
 		break;
 	case PHY_INTERFACE_MODE_SGMII:
-		cfg_mac.inband = 1;
 		cfg_mac.mode = PRESTERA_MAC_MODE_SGMII;
 		break;
 	case PHY_INTERFACE_MODE_1000BASEX:
 	default:
 		cfg_mac.speed = SPEED_1000;
 		cfg_mac.duplex = DUPLEX_FULL;
-		cfg_mac.inband = test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
-					  advertising);
 		cfg_mac.mode = PRESTERA_MAC_MODE_1000BASE_X;
 		break;
 	}
@@ -401,6 +395,7 @@ static int prestera_port_sfp_bind(struct prestera_port *port)
 			continue;
 
 		port->phylink_pcs.ops = &prestera_pcs_ops;
+		port->phylink_pcs.neg_mode = true;
 
 		port->phy_config.dev = &port->dev->dev;
 		port->phy_config.type = PHYLINK_NETDEV;
-- 
2.30.2

[PATCH net-next 11/15] net: qca8k: update PCS driver to use neg_mode

[Russell King (Oracle)]

Update qca8k's embedded PCS driver to use neg_mode rather than the
mode argument. As there is no pcs_link_up() method, this only affects
the pcs_config() method.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/qca/qca8k-8xxx.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index dee7b6579916..f7d7cfb2fd86 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -1493,7 +1493,7 @@ static void qca8k_pcs_get_state(struct phylink_pcs *pcs,
 		state->pause |= MLO_PAUSE_TX;
 }
 
-static int qca8k_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
+static int qca8k_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
 			    phy_interface_t interface,
 			    const unsigned long *advertising,
 			    bool permit_pause_to_mac)
@@ -1520,14 +1520,12 @@ static int qca8k_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
 	}
 
 	/* Enable/disable SerDes auto-negotiation as necessary */
-	ret = qca8k_read(priv, QCA8K_REG_PWS, &val);
+	val = neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED ?
+		0 : QCA8K_PWS_SERDES_AEN_DIS;
+
+	ret = qca8k_rmw(priv, QCA8K_REG_PWS, QCA8K_PWS_SERDES_AEN_DIS, val);
 	if (ret)
 		return ret;
-	if (phylink_autoneg_inband(mode))
-		val &= ~QCA8K_PWS_SERDES_AEN_DIS;
-	else
-		val |= QCA8K_PWS_SERDES_AEN_DIS;
-	qca8k_write(priv, QCA8K_REG_PWS, val);
 
 	/* Configure the SGMII parameters */
 	ret = qca8k_read(priv, QCA8K_REG_SGMII_CTRL, &val);
@@ -1598,6 +1596,7 @@ static void qca8k_setup_pcs(struct qca8k_priv *priv, struct qca8k_pcs *qpcs,
 			    int port)
 {
 	qpcs->pcs.ops = &qca8k_pcs_ops;
+	qpcs->pcs.neg_mode = true;
 
 	/* We don't have interrupts for link changes, so we need to poll */
 	qpcs->pcs.poll = true;
-- 
2.30.2

Re: [PATCH net-next 11/15] net: qca8k: update PCS driver to use neg_mode

[Russell King (Oracle)]

On Fri, Jun 16, 2023 at 01:07:14PM +0100, Russell King (Oracle) wrote:
> Update qca8k's embedded PCS driver to use neg_mode rather than the
> mode argument. As there is no pcs_link_up() method, this only affects
> the pcs_config() method.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

I see netdevbpf patchwork is complaining that I didn't Cc a maintainer
for this patch (ansuelsmth@gmail.com). Why is it complaining? This
address is *not* in the MAINTAINERS file in the net-next tree neither
for the version I generated the patch against (tip on submission date),
today's tip, nor the net tree.

Is patchwork using an outdated MAINTAINERS file?

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

Re: [PATCH net-next 11/15] net: qca8k: update PCS driver to use neg_mode

[Vladimir Oltean]

On Tue, Jun 20, 2023 at 10:18:13AM +0100, Russell King (Oracle) wrote:
> On Fri, Jun 16, 2023 at 01:07:14PM +0100, Russell King (Oracle) wrote:
> > Update qca8k's embedded PCS driver to use neg_mode rather than the
> > mode argument. As there is no pcs_link_up() method, this only affects
> > the pcs_config() method.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> 
> I see netdevbpf patchwork is complaining that I didn't Cc a maintainer
> for this patch (ansuelsmth@gmail.com). Why is it complaining? This
> address is *not* in the MAINTAINERS file in the net-next tree neither
> for the version I generated the patch against (tip on submission date),
> today's tip, nor the net tree.
> 
> Is patchwork using an outdated MAINTAINERS file?
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

I can presume that patchwork runs scripts/get_maintainer.pl, which looks
not only at the MAINTAINERS file, but also at the recent authors and
sign offs from git for a certain file path.

Re: [PATCH net-next 11/15] net: qca8k: update PCS driver to use neg_mode

[Jakub Kicinski]

On Tue, 20 Jun 2023 14:28:58 +0300 Vladimir Oltean wrote:
> On Tue, Jun 20, 2023 at 10:18:13AM +0100, Russell King (Oracle) wrote:
> > I see netdevbpf patchwork is complaining that I didn't Cc a maintainer
> > for this patch (ansuelsmth@gmail.com). Why is it complaining? This
> > address is *not* in the MAINTAINERS file in the net-next tree neither
> > for the version I generated the patch against (tip on submission date),
> > today's tip, nor the net tree.
> > 
> > Is patchwork using an outdated MAINTAINERS file?
> > 
> > -- 
> > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!  
> 
> I can presume that patchwork runs scripts/get_maintainer.pl, which looks
> not only at the MAINTAINERS file, but also at the recent authors and
> sign offs from git for a certain file path.

The exact incantation it uses is:

./scripts/get_maintainer.pl --git-min-percent 25

But it's just a warning because get_maintainer sometimes reports 
stupid stuff.

[PATCH net-next 12/15] net: sparx5: update PCS driver to use neg_mode

[Russell King (Oracle)]

Update Sparx5's embedded PCS driver to use neg_mode rather than the
mode argument. As there is no pcs_link_up() method, this only affects
the pcs_config() method.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/microchip/sparx5/sparx5_main.c    | 1 +
 drivers/net/ethernet/microchip/sparx5/sparx5_phylink.c | 8 ++++----
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
index a7edf524eedb..dc9af480bfea 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
@@ -281,6 +281,7 @@ static int sparx5_create_port(struct sparx5 *sparx5,
 	spx5_port->custom_etype = 0x8880; /* Vitesse */
 	spx5_port->phylink_pcs.poll = true;
 	spx5_port->phylink_pcs.ops = &sparx5_phylink_pcs_ops;
+	spx5_port->phylink_pcs.neg_mode = true;
 	spx5_port->is_mrouter = false;
 	INIT_LIST_HEAD(&spx5_port->tc_templates);
 	sparx5->ports[config->portno] = spx5_port;
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_phylink.c b/drivers/net/ethernet/microchip/sparx5/sparx5_phylink.c
index bb97d27a1da4..f8562c1a894d 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_phylink.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_phylink.c
@@ -91,8 +91,7 @@ static void sparx5_pcs_get_state(struct phylink_pcs *pcs,
 	state->pause = status.pause;
 }
 
-static int sparx5_pcs_config(struct phylink_pcs *pcs,
-			     unsigned int mode,
+static int sparx5_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
 			     phy_interface_t interface,
 			     const unsigned long *advertising,
 			     bool permit_pause_to_mac)
@@ -104,8 +103,9 @@ static int sparx5_pcs_config(struct phylink_pcs *pcs,
 	conf = port->conf;
 	conf.power_down = false;
 	conf.portmode = interface;
-	conf.inband = phylink_autoneg_inband(mode);
-	conf.autoneg = phylink_test(advertising, Autoneg);
+	conf.inband = neg_mode == PHYLINK_PCS_NEG_INBAND_DISABLED ||
+		      neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED;
+	conf.autoneg = neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED;
 	conf.pause_adv = 0;
 	if (phylink_test(advertising, Pause))
 		conf.pause_adv |= ADVERTISE_1000XPAUSE;
-- 
2.30.2

[PATCH net-next 13/15] net: dsa: b53: update PCS driver to use neg_mode

[Russell King (Oracle)]

Update B53's embedded PCS driver to use neg_mode, even though it makes
no use of it or the "mode" argument. This makes the driver consistent
with converted drivers.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/b53/b53_serdes.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/b53/b53_serdes.c b/drivers/net/dsa/b53/b53_serdes.c
index 0690210770ff..b0ccebcd3ffa 100644
--- a/drivers/net/dsa/b53/b53_serdes.c
+++ b/drivers/net/dsa/b53/b53_serdes.c
@@ -65,7 +65,7 @@ static u16 b53_serdes_read(struct b53_device *dev, u8 lane,
 	return b53_serdes_read_blk(dev, offset, block);
 }
 
-static int b53_serdes_config(struct phylink_pcs *pcs, unsigned int mode,
+static int b53_serdes_config(struct phylink_pcs *pcs, unsigned int neg_mode,
 			     phy_interface_t interface,
 			     const unsigned long *advertising,
 			     bool permit_pause_to_mac)
@@ -239,6 +239,7 @@ int b53_serdes_init(struct b53_device *dev, int port)
 	pcs->dev = dev;
 	pcs->lane = lane;
 	pcs->pcs.ops = &b53_pcs_ops;
+	pcs->pcs.neg_mode = true;
 
 	return 0;
 }
-- 
2.30.2

Re: [PATCH net-next 13/15] net: dsa: b53: update PCS driver to use neg_mode

[Florian Fainelli]

On 6/16/2023 1:07 PM, Russell King (Oracle) wrote:
> Update B53's embedded PCS driver to use neg_mode, even though it makes
> no use of it or the "mode" argument. This makes the driver consistent
> with converted drivers.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Acked-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

[PATCH net-next 14/15] net: dsa: mt7530: update PCS driver to use neg_mode

[Russell King (Oracle)]

Update mt7530's embedded PCS driver to use neg_mode, even though it
makes no use of it or the "mode" argument. This makes the driver
consistent with converted drivers.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/mt7530.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 9bc54e1348cb..cf95ccbba654 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2978,7 +2978,7 @@ static void mt7530_pcs_get_state(struct phylink_pcs *pcs,
 		state->pause |= MLO_PAUSE_TX;
 }
 
-static int mt753x_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
+static int mt753x_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
 			     phy_interface_t interface,
 			     const unsigned long *advertising,
 			     bool permit_pause_to_mac)
@@ -3006,6 +3006,7 @@ mt753x_setup(struct dsa_switch *ds)
 	/* Initialise the PCS devices */
 	for (i = 0; i < priv->ds->num_ports; i++) {
 		priv->pcs[i].pcs.ops = priv->info->pcs_ops;
+		priv->pcs[i].pcs.neg_mode = true;
 		priv->pcs[i].priv = priv;
 		priv->pcs[i].port = i;
 	}
-- 
2.30.2

[PATCH net-next 15/15] net: macb: update PCS driver to use neg_mode

[Russell King (Oracle)]

Update macb's embedded PCS drivers to use neg_mode, even though it
makes no use of it or the "mode" argument. This makes the driver
consistent with converted drivers.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/cadence/macb_main.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 50a4b04315e9..e2640c81fe8c 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -563,7 +563,7 @@ static void macb_set_tx_clk(struct macb *bp, int speed)
 		netdev_err(bp->dev, "adjusting tx_clk failed.\n");
 }
 
-static void macb_usx_pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
+static void macb_usx_pcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
 				 phy_interface_t interface, int speed,
 				 int duplex)
 {
@@ -596,7 +596,7 @@ static void macb_usx_pcs_get_state(struct phylink_pcs *pcs,
 }
 
 static int macb_usx_pcs_config(struct phylink_pcs *pcs,
-			       unsigned int mode,
+			       unsigned int neg_mode,
 			       phy_interface_t interface,
 			       const unsigned long *advertising,
 			       bool permit_pause_to_mac)
@@ -621,7 +621,7 @@ static void macb_pcs_an_restart(struct phylink_pcs *pcs)
 }
 
 static int macb_pcs_config(struct phylink_pcs *pcs,
-			   unsigned int mode,
+			   unsigned int neg_mode,
 			   phy_interface_t interface,
 			   const unsigned long *advertising,
 			   bool permit_pause_to_mac)
@@ -862,7 +862,9 @@ static int macb_mii_probe(struct net_device *dev)
 	struct macb *bp = netdev_priv(dev);
 
 	bp->phylink_sgmii_pcs.ops = &macb_phylink_pcs_ops;
+	bp->phylink_sgmii_pcs.neg_mode = true;
 	bp->phylink_usx_pcs.ops = &macb_phylink_usx_pcs_ops;
+	bp->phylink_usx_pcs.neg_mode = true;
 
 	bp->phylink_config.dev = &dev->dev;
 	bp->phylink_config.type = PHYLINK_NETDEV;
-- 
2.30.2

Re: [PATCH net-next 0/15] Add and use helper for PCS negotiation modes

[Vladimir Oltean]

On Fri, Jun 16, 2023 at 01:05:52PM +0100, Russell King (Oracle) wrote:
> Hi,
> 
> Earlier this month, I proposed a helper for deciding whether a PCS
> should use inband negotiation modes or not. There was some discussion
> around this topic, and I believe there was no disagreement about
> providing the helper.
> 
> The initial discussion can be found at:
> 
> https://lore.kernel.org/r/ZGIkGmyL8yL1q1zp@shell.armlinux.org.uk
> 
> Subsequently, I posted a RFC series back in May:
> 
> https://lore.kernel.org/r/ZGzhvePzPjJ0v2En@shell.armlinux.org.uk
> 
> that added a helper, phylink_pcs_neg_mode() which PCS drivers could use
> to parse the state, and updated a bunch of drivers to use it. I got
> a couple of bits of feedback to it, including some ACKs.
> 
> However, I've decided to take this slightly further and change the
> "mode" parameter to both the pcs_config() and pcs_link_up() methods
> when a PCS driver opts in to this (by setting "neg_mode" in the
> phylink_pcs structure.) If this is not set, we default to the old
> behaviour. That said, this series converts all the PCS implementations
> I can find currently in net-next.
> 
> Doing this has the added benefit that the negotiation mode parameter
> is also available to the pcs_link_up() function, which can now know
> whether inband negotiation was in fact enabled or not at pcs_config()
> time.
> 
> It has been posted as RFC at:
> 
> https://lore.kernel.org/r/ZIh/CLQ3z89g0Ua0@shell.armlinux.org.uk
> 
> and received one reply, thanks Elad, which is a similar amount of
> interest to previous postings. Let's post it as non-RFC and see
> whether we get more reaction.

Sorry, I was in the process of reviewing the RFC, but I'm not sure I
know what to ask to make sure that I understand the motivation :-/
Here's a question that might or might not result in a code change.

In the single-patch RFC at:
https://lore.kernel.org/all/ZGIkGmyL8yL1q1zp@shell.armlinux.org.uk/
you bring sparx5 and lan966x as a motivation for introducing
PHYLINK_PCS_NEG_OUTBAND as separate from PHYLINK_PCS_NEG_INBAND_DISABLED,
with both meaning that in-band autoneg isn't used, but in the former
case it's not enabled at all, while in the latter it's disabled through
ethtool (if I get that right?).

I've opened the Sparx5 documentation at:
https://ww1.microchip.com/downloads/en/DeviceDoc/SparX-5_Family_L2L3_Enterprise_10G_Ethernet_Switches_Datasheet_00003822B.pdf
and also cross-checked with the PCS1G documentation from VSC7514
(Ocelot: https://ww1.microchip.com/downloads/en/DeviceDoc/VMDS-10491.pdf,
there's another embedded PDF with registers at page 283), trying to find
exactly what the PCS1G_MODE_CFG.SGMII_MODE_ENA field does (which is
controlled in sparx5 and lan966x based on the presence or absence of the
managed = "in-band-status" property).

Do you know for sure what this bit does and whether it makes sense for
drivers to even distinguish between OUTBAND and INBAND_DISABLED in the
way that this series is proposing?

It's hard to know for sure, not having the hardware, but I believe that
the bit selects between the SGMII and the 1000Base-X control word
format (so, even though there's a dedicated and fully programmable
PCS1G_ANEG_CFG.ADV_ABILITY register, the link partner ability is still
decoded as per the programmed expected format). The documents talk about
using the PCS in "SGMII mode" vs "1000BASE-X SERDES mode".

If that's the case, then it is selecting between those 2 based on
phylink_autoneg_inband(mode) and irrespective of the phy-mode, i.e.:

- enabling the SGMII control word format for phy-mode = "1000base-x" and
  no managed = "in-band-status", or
- enabling the 1000Base-X control word format for phy-mode = "sgmii" and
  managed = "in-band-status"

...is that a model to follow?

Re: [PATCH net-next 0/15] Add and use helper for PCS negotiation modes

[Russell King (Oracle)]

On Fri, Jun 16, 2023 at 06:00:55PM +0300, Vladimir Oltean wrote:
> On Fri, Jun 16, 2023 at 01:05:52PM +0100, Russell King (Oracle) wrote:
> > Hi,
> > 
> > Earlier this month, I proposed a helper for deciding whether a PCS
> > should use inband negotiation modes or not. There was some discussion
> > around this topic, and I believe there was no disagreement about
> > providing the helper.
> > 
> > The initial discussion can be found at:
> > 
> > https://lore.kernel.org/r/ZGIkGmyL8yL1q1zp@shell.armlinux.org.uk
> > 
> > Subsequently, I posted a RFC series back in May:
> > 
> > https://lore.kernel.org/r/ZGzhvePzPjJ0v2En@shell.armlinux.org.uk
> > 
> > that added a helper, phylink_pcs_neg_mode() which PCS drivers could use
> > to parse the state, and updated a bunch of drivers to use it. I got
> > a couple of bits of feedback to it, including some ACKs.
> > 
> > However, I've decided to take this slightly further and change the
> > "mode" parameter to both the pcs_config() and pcs_link_up() methods
> > when a PCS driver opts in to this (by setting "neg_mode" in the
> > phylink_pcs structure.) If this is not set, we default to the old
> > behaviour. That said, this series converts all the PCS implementations
> > I can find currently in net-next.
> > 
> > Doing this has the added benefit that the negotiation mode parameter
> > is also available to the pcs_link_up() function, which can now know
> > whether inband negotiation was in fact enabled or not at pcs_config()
> > time.
> > 
> > It has been posted as RFC at:
> > 
> > https://lore.kernel.org/r/ZIh/CLQ3z89g0Ua0@shell.armlinux.org.uk
> > 
> > and received one reply, thanks Elad, which is a similar amount of
> > interest to previous postings. Let's post it as non-RFC and see
> > whether we get more reaction.
> 
> Sorry, I was in the process of reviewing the RFC, but I'm not sure I
> know what to ask to make sure that I understand the motivation :-/
> Here's a question that might or might not result in a code change.
> 
> In the single-patch RFC at:
> https://lore.kernel.org/all/ZGIkGmyL8yL1q1zp@shell.armlinux.org.uk/
> you bring sparx5 and lan966x as a motivation for introducing
> PHYLINK_PCS_NEG_OUTBAND as separate from PHYLINK_PCS_NEG_INBAND_DISABLED,
> with both meaning that in-band autoneg isn't used, but in the former
> case it's not enabled at all, while in the latter it's disabled through
> ethtool (if I get that right?).

Correct.

conf.inband is set true if phylink_autoneg_inband(mode) is true, which
equates to MLO_AN_INBAND.

conf.autoneg is set true if the ethtool Autoneg flag in the advertising
mask is set.

That goes through some incomprehensible logic in
sparx5_port_pcs_low_set() which I'm not going to even try to unpick
because it looks buggy to me, except that conf.autoneg is only looked
at if conf.inband were true at some point in the past.

So, what I care about here is keeping the behaviour pretty much the
same, especially as far as conf.inband.

With the new neg_mode:

conf.inband is set when we have one of the NEG_INBAND states. These are
set when in 1000BASE-X, 2500BASE-X or one of the SGMII family and
phylink_autoneg_inband(mode) is true. So, 100% identical when one
considers that the driver only supports SGMII, QSGMII, 1000BASE-X and
2500BASE-X for this path.

conf.autoneg will only be set when we have NEG_INBAND_ENABLED state,
and that is only set when in SGMII mode (irrespective of Autoneg) or
in *BASE-X, we're in in-band mode (so conf.inband is set) and the
advertising mask has the Autoneg bit set. As this is only looked at
if conf.inband was set the _last_ time around (which seems like a
bug in the driver...) and we're in 1000BASE-X mode, this is identical
logic where it matters.

So, conf.inband is 100% identical logic, and conf.autoneg is very
similar and for how it's actually used, completely identical.

> ... trying to find
> exactly what the PCS1G_MODE_CFG.SGMII_MODE_ENA field does (which is
> controlled in sparx5 and lan966x based on the presence or absence of the
> managed = "in-band-status" property).
> 
> Do you know for sure what this bit does and whether it makes sense for
> drivers to even distinguish between OUTBAND and INBAND_DISABLED in the
> way that this series is proposing?

I have no idea, and I didn't bother investigating - I don't want to go
around trying to disect drivers to figure out whether they're buggy or
not.

However, what I would say is that this is not where these modes came
from. They came from me asking myself the question "what would be the
logical set of information to give a PCS driver about the negotiation
state of the link?" and that's what I came up with _without_ reference
to this driver. The states are all documented in the first patch and
what they mean.

So, no, the Microchip driver code is not the reason why these
definitions were chosen. They were chosen because it's the logical
set that gives PCS drivers what they need to know.

Start from inband + autoneg. Then inband + !autoneg. Then inband
possible but not being used. Then "there's no inband possible for this
mode". That's the four states.

I think having this level of detail is important if we want to think
about those pesky inband-AN bypass modes, which make sense for only
really the PHYLINK_PCS_NEG_INBAND_DISABLED state and not OUTBAND nor
NONE state. Bypass mode doesn't make sense for e.g. SGMII because
one needs to know the speed for the link to come up, and if you're
getting that through an out-of-band mechanism, you're into forcing
the configuration at the PCS end.

Makes sense?

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

Re: [PATCH net-next 0/15] Add and use helper for PCS negotiation modes

[Russell King (Oracle)]

On Fri, Jun 16, 2023 at 04:46:39PM +0100, Russell King (Oracle) wrote:
> On Fri, Jun 16, 2023 at 06:00:55PM +0300, Vladimir Oltean wrote:
> > Do you know for sure what this bit does and whether it makes sense for
> > drivers to even distinguish between OUTBAND and INBAND_DISABLED in the
> > way that this series is proposing?
> 
> I have no idea, and I didn't bother investigating - I don't want to go
> around trying to disect drivers to figure out whether they're buggy or
> not.
> 
> However, what I would say is that this is not where these modes came
> from. They came from me asking myself the question "what would be the
> logical set of information to give a PCS driver about the negotiation
> state of the link?" and that's what I came up with _without_ reference
> to this driver. The states are all documented in the first patch and
> what they mean.
> 
> So, no, the Microchip driver code is not the reason why these
> definitions were chosen. They were chosen because it's the logical
> set that gives PCS drivers what they need to know.
> 
> Start from inband + autoneg. Then inband + !autoneg. Then inband
> possible but not being used. Then "there's no inband possible for this
> mode". That's the four states.
> 
> I think having this level of detail is important if we want to think
> about those pesky inband-AN bypass modes, which make sense for only
> really the PHYLINK_PCS_NEG_INBAND_DISABLED state and not OUTBAND nor
> NONE state. Bypass mode doesn't make sense for e.g. SGMII because
> one needs to know the speed for the link to come up, and if you're
> getting that through an out-of-band mechanism, you're into forcing
> the configuration at the PCS end.

I should also add... yes, I did _then_ subsequently use the microchip
driver as a justification for it. I probably should've explained it
without using that as justification.

I could also have used the sja1105 driver as well, since:

	MLO_AN_INBAND => PHYLINK_PCS_NEG_INBAND_ENABLED
	MLO_AN_FIXED || MLO_AN_PHY => PHYLINK_PCS_NEG_OUTBAND

are the conversions done there, which fits with:

-               if (!phylink_autoneg_inband(mode)) {
+               if (neg_mode == PHYLINK_PCS_NEG_OUTBAND) {

since the opposite of !inband is outband.

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

Re: [PATCH net-next 0/15] Add and use helper for PCS negotiation modes

[Vladimir Oltean]

On Fri, Jun 16, 2023 at 04:52:21PM +0100, Russell King (Oracle) wrote:
> I should also add... yes, I did _then_ subsequently use the microchip
> driver as a justification for it. I probably should've explained it
> without using that as justification.
> 
> I could also have used the sja1105 driver as well, since:
> 
> 	MLO_AN_INBAND => PHYLINK_PCS_NEG_INBAND_ENABLED

Technically this should have been:

	MLO_AN_INBAND => neg_mode & PHYLINK_PCS_NEG_INBAND, which
	includes both INBAND_DISABLED and INBAND_ENABLED, right?

> 	MLO_AN_FIXED || MLO_AN_PHY => PHYLINK_PCS_NEG_OUTBAND
> 
> are the conversions done there, which fits with:
> 
> -               if (!phylink_autoneg_inband(mode)) {
> +               if (neg_mode == PHYLINK_PCS_NEG_OUTBAND) {
> 
> since the opposite of !inband is outband.

The conversion is correct - no doubt about it.

Maybe the SJA1105 and its use of the XPCS is also not the best example,
because it doesn't support 1000BASE-X. So it doesn't have to handle the
INBAND_DISABLED state. If it did, the !phylink_autoneg_inband(mode)
check would have been incorrect (insufficient to detect the xpcs state
that it's restoring).

Re: [PATCH net-next 0/15] Add and use helper for PCS negotiation modes

[Vladimir Oltean]

On Fri, Jun 16, 2023 at 04:46:39PM +0100, Russell King (Oracle) wrote:
> So, no, the Microchip driver code is not the reason why these
> definitions were chosen. They were chosen because it's the logical
> set that gives PCS drivers what they need to know.
> 
> Start from inband + autoneg. Then inband + !autoneg. Then inband
> possible but not being used. Then "there's no inband possible for this
> mode". That's the four states.
> 
> I think having this level of detail is important if we want to think
> about those pesky inband-AN bypass modes, which make sense for only
> really the PHYLINK_PCS_NEG_INBAND_DISABLED state and not OUTBAND nor
             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

don't you mean PHYLINK_PCS_NEG_INBAND_ENABLED? I fail to see why would
the bypass make any difference for INBAND_DISABLED, where presumably the
fiber BMCR of the attached PHY would have BMCR_ANENABLE unset.

And in that case, I still don't understand the need for distinguishing
between INBAND_DISABLED, OUTBAND, NONE. Sorry, slow-witted :)

> NONE state. Bypass mode doesn't make sense for e.g. SGMII because
> one needs to know the speed for the link to come up, and if you're
> getting that through an out-of-band mechanism, you're into forcing
> the configuration at the PCS end.
> 
> Makes sense?

I refreshed my memory with this thread
https://patchwork.kernel.org/project/netdevbpf/patch/20221118000124.2754581-4-vladimir.oltean@nxp.com/
regarding in-band AN bypass on m88e1011, and the fact that enabling
in-band AN bypass with SGMII forces an advertisement of only
1000baseT/Half and 1000baseT/Full on the media side.

So.. correct, but I still don't get the overall answer to the question
I have, which is "why would drivers want to make any legitimate
distinction between INBAND_DISABLED and OUTBAND, when for all intents
and purposes, those 2 modes are nothing but the same physical state,
reached from 2 different phylink configuration path"?

Re: [PATCH net-next 0/15] Add and use helper for PCS negotiation modes

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 16 Jun 2023 13:05:52 +0100 you wrote:
> Hi,
> 
> Earlier this month, I proposed a helper for deciding whether a PCS
> should use inband negotiation modes or not. There was some discussion
> around this topic, and I believe there was no disagreement about
> providing the helper.
> 
> [...]

Here is the summary with links:
  - [net-next,01/15] net: phylink: add PCS negotiation mode
    https://git.kernel.org/netdev/net-next/c/f99d471afa03
  - [net-next,02/15] net: phylink: convert phylink_mii_c22_pcs_config() to neg_mode
    https://git.kernel.org/netdev/net-next/c/cdb08aa04737
  - [net-next,03/15] net: phylink: pass neg_mode into phylink_mii_c22_pcs_config()
    https://git.kernel.org/netdev/net-next/c/febf2aaf0564
  - [net-next,04/15] net: pcs: xpcs: update PCS driver to use neg_mode
    https://git.kernel.org/netdev/net-next/c/a3a47cfb88fc
  - [net-next,05/15] net: pcs: lynxi: update PCS driver to use neg_mode
    https://git.kernel.org/netdev/net-next/c/3b2de56a146f
  - [net-next,06/15] net: pcs: lynx: update PCS driver to use neg_mode
    https://git.kernel.org/netdev/net-next/c/c689a6528c22
  - [net-next,07/15] net: lan966x: update PCS driver to use neg_mode
    https://git.kernel.org/netdev/net-next/c/a0e93cfdac4c
  - [net-next,08/15] net: mvneta: update PCS driver to use neg_mode
    https://git.kernel.org/netdev/net-next/c/140d1002e2a3
  - [net-next,09/15] net: mvpp2: update PCS driver to use neg_mode
    https://git.kernel.org/netdev/net-next/c/d5b16264fffe
  - [net-next,10/15] net: prestera: update PCS driver to use neg_mode
    https://git.kernel.org/netdev/net-next/c/d5a052993062
  - [net-next,11/15] net: qca8k: update PCS driver to use neg_mode
    https://git.kernel.org/netdev/net-next/c/bfa0a3ac05b6
  - [net-next,12/15] net: sparx5: update PCS driver to use neg_mode
    https://git.kernel.org/netdev/net-next/c/6e5bb3da9842
  - [net-next,13/15] net: dsa: b53: update PCS driver to use neg_mode
    https://git.kernel.org/netdev/net-next/c/772c476dd1d4
  - [net-next,14/15] net: dsa: mt7530: update PCS driver to use neg_mode
    https://git.kernel.org/netdev/net-next/c/6c1e4eca0b4e
  - [net-next,15/15] net: macb: update PCS driver to use neg_mode
    https://git.kernel.org/netdev/net-next/c/f40df95d375d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html