[PATCH v2 net-next 0/6] PHYLINK integration improvements for Felix DSA driver

[PATCH v2 net-next 0/6] PHYLINK integration improvements for Felix DSA driver

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

This is an overhaul of the Felix switch driver's PHYLINK operations.

Patches 1, 3, 4 and 5 are cleanup, patch 2 is adding a new feature and
and patch 6 is adaptation to the new format of an existing phylink API
(mac_link_up).

Changes since v1:
- Now using phy_clear_bits and phy_set_bits instead of plain writes to
  MII_BMCR. This combines former patches 1/7 and 6/7 into a single new
  patch 1/6.
- Updated commit message of patch 5/6.

Vladimir Oltean (6):
  net: dsa: felix: clarify the intention of writes to MII_BMCR
  net: dsa: felix: support half-duplex link modes
  net: dsa: felix: unconditionally configure MAC speed to 1000Mbps
  net: dsa: felix: set proper pause frame timers based on link speed
  net: dsa: felix: delete .phylink_mac_an_restart code
  net: dsa: felix: use resolved link config in mac_link_up()

 drivers/net/dsa/ocelot/felix.c         | 108 +++++----
 drivers/net/dsa/ocelot/felix.h         |  11 +-
 drivers/net/dsa/ocelot/felix_vsc9959.c | 298 ++++++++++++-------------
 include/linux/fsl/enetc_mdio.h         |   1 +
 4 files changed, 213 insertions(+), 205 deletions(-)

-- 
2.25.1

[PATCH v2 net-next 1/6] net: dsa: felix: clarify the intention of writes to MII_BMCR

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

The driver appears to write to BMCR_SPEED and BMCR_DUPLEX, fields which
are read-only, since they are actually configured through the
vendor-specific IF_MODE (0x14) register.

But the reason we're writing back the read-only values of MII_BMCR is to
alter these writable fields:

BMCR_RESET
BMCR_LOOPBACK
BMCR_ANENABLE
BMCR_PDOWN
BMCR_ISOLATE
BMCR_ANRESTART

In particular, the only field which is really relevant to this driver is
BMCR_ANENABLE. Clarify that intention by spelling it out, using
phy_set_bits and phy_clear_bits.

The driver also made a few writes to BMCR_RESET and BMCR_ANRESTART which
are unnecessary and may temporarily disrupt the link to the PHY. Remove
them.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Changes in v2:
Patch is new.

 drivers/net/dsa/ocelot/felix_vsc9959.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 2067776773f7..9f4c8343652f 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -815,7 +815,7 @@ static void vsc9959_pcs_init_sgmii(struct phy_device *pcs,
 		phy_write(pcs, ENETC_PCS_LINK_TIMER2,
 			  ENETC_PCS_LINK_TIMER2_VAL);
 
-		phy_write(pcs, MII_BMCR, BMCR_ANRESTART | BMCR_ANENABLE);
+		phy_set_bits(pcs, MII_BMCR, BMCR_ANENABLE);
 	} else {
 		int speed;
 
@@ -845,10 +845,7 @@ static void vsc9959_pcs_init_sgmii(struct phy_device *pcs,
 			  ENETC_PCS_IF_MODE_SGMII_EN |
 			  ENETC_PCS_IF_MODE_SGMII_SPEED(speed));
 
-		/* Yes, not a mistake: speed is given by IF_MODE. */
-		phy_write(pcs, MII_BMCR, BMCR_RESET |
-					 BMCR_SPEED1000 |
-					 BMCR_FULLDPLX);
+		phy_clear_bits(pcs, MII_BMCR, BMCR_ANENABLE);
 	}
 }
 
@@ -882,9 +879,7 @@ static void vsc9959_pcs_init_2500basex(struct phy_device *pcs,
 		  ENETC_PCS_IF_MODE_SGMII_EN |
 		  ENETC_PCS_IF_MODE_SGMII_SPEED(ENETC_PCS_SPEED_2500));
 
-	phy_write(pcs, MII_BMCR, BMCR_SPEED1000 |
-				 BMCR_FULLDPLX |
-				 BMCR_RESET);
+	phy_clear_bits(pcs, MII_BMCR, BMCR_ANENABLE);
 }
 
 static void vsc9959_pcs_init_usxgmii(struct phy_device *pcs,
-- 
2.25.1

[PATCH v2 net-next 2/6] net: dsa: felix: support half-duplex link modes

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

Ping tested:

  [   11.808455] mscc_felix 0000:00:00.5 swp0: Link is Up - 1Gbps/Full - flow control rx/tx
  [   11.816497] IPv6: ADDRCONF(NETDEV_CHANGE): swp0: link becomes ready

  [root@LS1028ARDB ~] # ethtool -s swp0 advertise 0x4
  [   18.844591] mscc_felix 0000:00:00.5 swp0: Link is Down
  [   22.048337] mscc_felix 0000:00:00.5 swp0: Link is Up - 100Mbps/Half - flow control off

  [root@LS1028ARDB ~] # ip addr add 192.168.1.1/24 dev swp0

  [root@LS1028ARDB ~] # ping 192.168.1.2
  PING 192.168.1.2 (192.168.1.2): 56 data bytes
  (...)
  ^C--- 192.168.1.2 ping statistics ---
  3 packets transmitted, 3 packets received, 0% packet loss
  round-trip min/avg/max = 0.383/0.611/1.051 ms

  [root@LS1028ARDB ~] # ethtool -s swp0 advertise 0x10
  [  355.637747] mscc_felix 0000:00:00.5 swp0: Link is Down
  [  358.788034] mscc_felix 0000:00:00.5 swp0: Link is Up - 1Gbps/Half - flow control off

  [root@LS1028ARDB ~] # ping 192.168.1.2
  PING 192.168.1.2 (192.168.1.2): 56 data bytes
  (...)
  ^C
  --- 192.168.1.2 ping statistics ---
  16 packets transmitted, 16 packets received, 0% packet loss
  round-trip min/avg/max = 0.301/0.384/1.138 ms

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Russell King <rmk+kernel@armlinux.org.uk>
---
Changes in v2:
None.

Note: this patch is still using state->duplex from mac_config(),
although that will be changed in 6/6.

 drivers/net/dsa/ocelot/felix.c         |  4 +++-
 drivers/net/dsa/ocelot/felix_vsc9959.c | 23 +++++++++++++----------
 include/linux/fsl/enetc_mdio.h         |  1 +
 3 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 75020af7f7a4..f54648dff0ec 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -194,13 +194,15 @@ static void felix_phylink_validate(struct dsa_switch *ds, int port,
 		return;
 	}
 
-	/* No half-duplex. */
 	phylink_set_port_modes(mask);
 	phylink_set(mask, Autoneg);
 	phylink_set(mask, Pause);
 	phylink_set(mask, Asym_Pause);
+	phylink_set(mask, 10baseT_Half);
 	phylink_set(mask, 10baseT_Full);
+	phylink_set(mask, 100baseT_Half);
 	phylink_set(mask, 100baseT_Full);
+	phylink_set(mask, 1000baseT_Half);
 	phylink_set(mask, 1000baseT_Full);
 
 	if (state->interface == PHY_INTERFACE_MODE_INTERNAL ||
diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 9f4c8343652f..94e946b26f90 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -817,12 +817,9 @@ static void vsc9959_pcs_init_sgmii(struct phy_device *pcs,
 
 		phy_set_bits(pcs, MII_BMCR, BMCR_ANENABLE);
 	} else {
+		u16 if_mode = ENETC_PCS_IF_MODE_SGMII_EN;
 		int speed;
 
-		if (state->duplex == DUPLEX_HALF) {
-			phydev_err(pcs, "Half duplex not supported\n");
-			return;
-		}
 		switch (state->speed) {
 		case SPEED_1000:
 			speed = ENETC_PCS_SPEED_1000;
@@ -841,9 +838,9 @@ static void vsc9959_pcs_init_sgmii(struct phy_device *pcs,
 			return;
 		}
 
-		phy_write(pcs, ENETC_PCS_IF_MODE,
-			  ENETC_PCS_IF_MODE_SGMII_EN |
-			  ENETC_PCS_IF_MODE_SGMII_SPEED(speed));
+		if_mode |= ENETC_PCS_IF_MODE_SGMII_SPEED(speed);
+		if (state->duplex == DUPLEX_HALF)
+			if_mode |= ENETC_PCS_IF_MODE_DUPLEX_HALF;
 
 		phy_clear_bits(pcs, MII_BMCR, BMCR_ANENABLE);
 	}
@@ -870,15 +867,18 @@ static void vsc9959_pcs_init_2500basex(struct phy_device *pcs,
 				       unsigned int link_an_mode,
 				       const struct phylink_link_state *state)
 {
+	u16 if_mode = ENETC_PCS_IF_MODE_SGMII_SPEED(ENETC_PCS_SPEED_2500) |
+		      ENETC_PCS_IF_MODE_SGMII_EN;
+
 	if (link_an_mode == MLO_AN_INBAND) {
 		phydev_err(pcs, "AN not supported on 3.125GHz SerDes lane\n");
 		return;
 	}
 
-	phy_write(pcs, ENETC_PCS_IF_MODE,
-		  ENETC_PCS_IF_MODE_SGMII_EN |
-		  ENETC_PCS_IF_MODE_SGMII_SPEED(ENETC_PCS_SPEED_2500));
+	if (state->duplex == DUPLEX_HALF)
+		if_mode |= ENETC_PCS_IF_MODE_DUPLEX_HALF;
 
+	phy_write(pcs, ENETC_PCS_IF_MODE, if_mode);
 	phy_clear_bits(pcs, MII_BMCR, BMCR_ANENABLE);
 }
 
@@ -919,8 +919,11 @@ static void vsc9959_pcs_init(struct ocelot *ocelot, int port,
 	linkmode_set_bit_array(phy_basic_ports_array,
 			       ARRAY_SIZE(phy_basic_ports_array),
 			       pcs->supported);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, pcs->supported);
 	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, pcs->supported);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, pcs->supported);
 	linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, pcs->supported);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, pcs->supported);
 	linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, pcs->supported);
 	if (pcs->interface == PHY_INTERFACE_MODE_2500BASEX ||
 	    pcs->interface == PHY_INTERFACE_MODE_USXGMII)
diff --git a/include/linux/fsl/enetc_mdio.h b/include/linux/fsl/enetc_mdio.h
index 4875dd38af7e..2d9203314865 100644
--- a/include/linux/fsl/enetc_mdio.h
+++ b/include/linux/fsl/enetc_mdio.h
@@ -15,6 +15,7 @@
 #define ENETC_PCS_IF_MODE_SGMII_EN		BIT(0)
 #define ENETC_PCS_IF_MODE_USE_SGMII_AN		BIT(1)
 #define ENETC_PCS_IF_MODE_SGMII_SPEED(x)	(((x) << 2) & GENMASK(3, 2))
+#define ENETC_PCS_IF_MODE_DUPLEX_HALF		BIT(3)
 
 /* Not a mistake, the SerDes PLL needs to be set at 3.125 GHz by Reset
  * Configuration Word (RCW, outside Linux control) for 2.5G SGMII mode. The PCS
-- 
2.25.1

[PATCH v2 net-next 3/6] net: dsa: felix: unconditionally configure MAC speed to 1000Mbps

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

In VSC9959, the PCS is the one who performs rate adaptation (symbol
duplication) to the speed negotiated by the PHY. The MAC is unaware of
that and must remain configured for gigabit. If it is configured at
OCELOT_SPEED_10 or OCELOT_SPEED_100, it'll start transmitting PAUSE
frames out of control and never recover, _even if_ we then reconfigure
it at OCELOT_SPEED_1000 afterwards.

This patch fixes a bug that luckily did not have any functional impact.
We were writing 10, 100, 1000 etc into this 2-bit field in
DEV_CLOCK_CFG, but the hardware expects values in the range 0, 1, 2, 3.
So all speed values were getting truncated to 0, which is
OCELOT_SPEED_2500, and which also appears to be fine.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Changes in v2:
None.

 drivers/net/dsa/ocelot/felix.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index f54648dff0ec..4d819cc45bed 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -240,9 +240,14 @@ static void felix_phylink_mac_config(struct dsa_switch *ds, int port,
 	u32 mac_fc_cfg;
 
 	/* Take port out of reset by clearing the MAC_TX_RST, MAC_RX_RST and
-	 * PORT_RST bits in CLOCK_CFG
+	 * PORT_RST bits in DEV_CLOCK_CFG. Note that the way this system is
+	 * integrated is that the MAC speed is fixed and it's the PCS who is
+	 * performing the rate adaptation, so we have to write "1000Mbps" into
+	 * the LINK_SPEED field of DEV_CLOCK_CFG (which is also its default
+	 * value).
 	 */
-	ocelot_port_writel(ocelot_port, DEV_CLOCK_CFG_LINK_SPEED(state->speed),
+	ocelot_port_writel(ocelot_port,
+			   DEV_CLOCK_CFG_LINK_SPEED(OCELOT_SPEED_1000),
 			   DEV_CLOCK_CFG);
 
 	/* Flow control. Link speed is only used here to evaluate the time
-- 
2.25.1

[PATCH v2 net-next 4/6] net: dsa: felix: set proper pause frame timers based on link speed

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

state->speed holds a value of 10, 100, 1000 or 2500, but
SYS_MAC_FC_CFG_FC_LINK_SPEED expects a value in the range 0, 1, 2 or 3.

So set the correct speed encoding into this register.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Changes in v2:
None.

Note: this patch is still using state->speed from mac_config(), although
that will be changed in 6/6.

 drivers/net/dsa/ocelot/felix.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 4d819cc45bed..4684339012c5 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -250,10 +250,25 @@ static void felix_phylink_mac_config(struct dsa_switch *ds, int port,
 			   DEV_CLOCK_CFG_LINK_SPEED(OCELOT_SPEED_1000),
 			   DEV_CLOCK_CFG);
 
-	/* Flow control. Link speed is only used here to evaluate the time
-	 * specification in incoming pause frames.
-	 */
-	mac_fc_cfg = SYS_MAC_FC_CFG_FC_LINK_SPEED(state->speed);
+	switch (state->speed) {
+	case SPEED_10:
+		mac_fc_cfg = SYS_MAC_FC_CFG_FC_LINK_SPEED(3);
+		break;
+	case SPEED_100:
+		mac_fc_cfg = SYS_MAC_FC_CFG_FC_LINK_SPEED(2);
+		break;
+	case SPEED_1000:
+	case SPEED_2500:
+		mac_fc_cfg = SYS_MAC_FC_CFG_FC_LINK_SPEED(1);
+		break;
+	case SPEED_UNKNOWN:
+		mac_fc_cfg = SYS_MAC_FC_CFG_FC_LINK_SPEED(0);
+		break;
+	default:
+		dev_err(ocelot->dev, "Unsupported speed on port %d: %d\n",
+			port, state->speed);
+		return;
+	}
 
 	/* handle Rx pause in all cases, with 2500base-X this is used for rate
 	 * adaptation.
@@ -265,6 +280,10 @@ static void felix_phylink_mac_config(struct dsa_switch *ds, int port,
 			      SYS_MAC_FC_CFG_PAUSE_VAL_CFG(0xffff) |
 			      SYS_MAC_FC_CFG_FC_LATENCY_CFG(0x7) |
 			      SYS_MAC_FC_CFG_ZERO_PAUSE_ENA;
+
+	/* Flow control. Link speed is only used here to evaluate the time
+	 * specification in incoming pause frames.
+	 */
 	ocelot_write_rix(ocelot, mac_fc_cfg, SYS_MAC_FC_CFG, port);
 
 	ocelot_write_rix(ocelot, 0, ANA_POL_FLOWC, port);
-- 
2.25.1

[PATCH v2 net-next 5/6] net: dsa: felix: delete .phylink_mac_an_restart code

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

The Cisco SGMII and USXGMII standards specify control information
exchange to be "achieved by using the Auto-Negotiation functionality
defined in Clause 37 of the IEEE Specification 802.3z".

The differences to clause 37 auto-negotiation are specified by the
respective standards. In the case of SGMII, the differences are spelled
out as being:

- A reduction of the link timer value, from 10 ms to 1.6 ms.
- A customization of the tx_config_reg[15:0], mostly to allow
  propagation of speed information.

A similar situation is going on for USXGMII as well: "USXGMII Auto-neg
mechanism is based on Clause 37 (Figure 37-6) plus additional management
control to select USXGMII mode".

The point is, both Cisco standards make explicit reference that they
require an auto-negotiation state machine implemented as per "Figure
37-6-Auto-Negotiation state diagram" from IEEE 802.3. In the SGMII spec,
it is very clearly pointed out that both the MAC PCS (Figure 3 MAC
Functional Block) and the PHY PCS (Figure 2 PHY Functional Block)
contain an auto-negotiation block defined by "Auto-Negotiation Figure
37-6".

Since both ends of the SGMII/USXGMII link implement the same state
machine (just carry different tx_config_reg payloads, which they convey
to their link partner via /C/ ordered sets), naturally the ability to
restart auto-negotiation is symmetrical. The state machine in IEEE 802.3
Figure 37-6 specifies the signal that triggers an auto-negotiation
restart as being "mr_restart_an=TRUE".

Furthermore, clause "37.2.5.1.9 State diagram variable to management
register mapping", through its "Table 37-8-PCS state diagram variable to
management register mapping", requires a PCS compliant to clause 37 to
expose the mr_restart_an signal to management through MDIO register "0.9
Auto-Negotiation restart", aka BMCR_ANRESTART in Linux terms.

The Felix PCS for SGMII and USXGMII is compliant to clause 37, so it
exposes BMCR_ANRESTART to the operating system. When this bit is
asserted, the following happens:

1. STATUS[Auto_Negotiation_Complete] goes from 1->0.
2. The PCS starts sending AN sequences instead of packets or IDLEs.
3. The PCS waits to receive AN sequences from PHY and matches them.
4. Once it has received  matching AN sequences and a PHY acknowledge,
   STATUS[Auto_Negotiation_Complete] goes from 0->1.
5. Normal packet transmission restarts.

Otherwise stated, the MAC PCS has the ability to re-trigger a switch of
the lane from data mode into configuration mode, then control
information exchange takes place, then the lane is switched back into
data mode. These 5 steps are collectively described as "restart AN state
machine" by the PCS documentation.
This is all as per IEEE 802.3 Clause 37 AN state machine, which SGMII
and USXGMII do not touch at this fundamental level.

Now, it is true that the Cisco SGMII and USXGMII specs mention that the
control information exchange has a unidirectional meaning. That is, the
PHY restarts the clause 37 auto-negotiation upon any change in MDI
auto-negotiation parameters.

PHYLINK takes this fact a bit further, and since the fact that for
SGMII/USXGMII, the MAC PCS conveys no new information to the PHY PCS
(beyond acknowledging the received config word), does not have any use
for permitting the MAC PCS to trigger a restart of the clause 37
auto-negotiation.

The only SERDES protocols for which PHYLINK allows that are 1000Base-X
and 2500Base-X. For those, the control information exchange _is_
bidirectional (local PCS specifies its duplex and flow control
abilities) since the link partner is at the other side of the media.

For any other SERDES protocols, the .phylink_mac_an_restart callback is
dead code. This is probably OK, I can't come up with a situation where
it might be useful for the MAC PCS to clear its cache of link state and
ask for a new tx_config_reg.

So remove this code.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Changes in v2:
Update commit message to be more clear.

 drivers/net/dsa/ocelot/felix.c         | 10 -------
 drivers/net/dsa/ocelot/felix.h         |  1 -
 drivers/net/dsa/ocelot/felix_vsc9959.c | 37 --------------------------
 3 files changed, 48 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 4684339012c5..57c400a67f16 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -296,15 +296,6 @@ static void felix_phylink_mac_config(struct dsa_switch *ds, int port,
 						  state->speed);
 }
 
-static void felix_phylink_mac_an_restart(struct dsa_switch *ds, int port)
-{
-	struct ocelot *ocelot = ds->priv;
-	struct felix *felix = ocelot_to_felix(ocelot);
-
-	if (felix->info->pcs_an_restart)
-		felix->info->pcs_an_restart(ocelot, port);
-}
-
 static void felix_phylink_mac_link_down(struct dsa_switch *ds, int port,
 					unsigned int link_an_mode,
 					phy_interface_t interface)
@@ -810,7 +801,6 @@ static const struct dsa_switch_ops felix_switch_ops = {
 	.phylink_validate	= felix_phylink_validate,
 	.phylink_mac_link_state	= felix_phylink_mac_pcs_get_state,
 	.phylink_mac_config	= felix_phylink_mac_config,
-	.phylink_mac_an_restart	= felix_phylink_mac_an_restart,
 	.phylink_mac_link_down	= felix_phylink_mac_link_down,
 	.phylink_mac_link_up	= felix_phylink_mac_link_up,
 	.port_enable		= felix_port_enable,
diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h
index a891736ca006..4a4cebcf04a7 100644
--- a/drivers/net/dsa/ocelot/felix.h
+++ b/drivers/net/dsa/ocelot/felix.h
@@ -31,7 +31,6 @@ struct felix_info {
 	void	(*pcs_init)(struct ocelot *ocelot, int port,
 			    unsigned int link_an_mode,
 			    const struct phylink_link_state *state);
-	void	(*pcs_an_restart)(struct ocelot *ocelot, int port);
 	void	(*pcs_link_state)(struct ocelot *ocelot, int port,
 				  struct phylink_link_state *state);
 	int	(*prevalidate_phy_mode)(struct ocelot *ocelot, int port,
diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 94e946b26f90..65f83386bad1 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -728,42 +728,6 @@ static int vsc9959_reset(struct ocelot *ocelot)
 	return 0;
 }
 
-static void vsc9959_pcs_an_restart_sgmii(struct phy_device *pcs)
-{
-	phy_set_bits(pcs, MII_BMCR, BMCR_ANRESTART);
-}
-
-static void vsc9959_pcs_an_restart_usxgmii(struct phy_device *pcs)
-{
-	phy_write_mmd(pcs, MDIO_MMD_VEND2, MII_BMCR,
-		      USXGMII_BMCR_RESET |
-		      USXGMII_BMCR_AN_EN |
-		      USXGMII_BMCR_RST_AN);
-}
-
-static void vsc9959_pcs_an_restart(struct ocelot *ocelot, int port)
-{
-	struct felix *felix = ocelot_to_felix(ocelot);
-	struct phy_device *pcs = felix->pcs[port];
-
-	if (!pcs)
-		return;
-
-	switch (pcs->interface) {
-	case PHY_INTERFACE_MODE_SGMII:
-	case PHY_INTERFACE_MODE_QSGMII:
-		vsc9959_pcs_an_restart_sgmii(pcs);
-		break;
-	case PHY_INTERFACE_MODE_USXGMII:
-		vsc9959_pcs_an_restart_usxgmii(pcs);
-		break;
-	default:
-		dev_err(ocelot->dev, "Invalid PCS interface type %s\n",
-			phy_modes(pcs->interface));
-		break;
-	}
-}
-
 /* We enable SGMII AN only when the PHY has managed = "in-band-status" in the
  * device tree. If we are in MLO_AN_PHY mode, we program directly state->speed
  * into the PCS, which is retrieved out-of-band over MDIO. This also has the
@@ -1411,7 +1375,6 @@ struct felix_info felix_info_vsc9959 = {
 	.mdio_bus_alloc		= vsc9959_mdio_bus_alloc,
 	.mdio_bus_free		= vsc9959_mdio_bus_free,
 	.pcs_init		= vsc9959_pcs_init,
-	.pcs_an_restart		= vsc9959_pcs_an_restart,
 	.pcs_link_state		= vsc9959_pcs_link_state,
 	.prevalidate_phy_mode	= vsc9959_prevalidate_phy_mode,
 	.port_setup_tc          = vsc9959_port_setup_tc,
-- 
2.25.1

Re: [PATCH v2 net-next 5/6] net: dsa: felix: delete .phylink_mac_an_restart code

[Russell King - ARM Linux admin]

On Sat, Jul 04, 2020 at 03:45:06PM +0300, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> The Cisco SGMII and USXGMII standards specify control information
> exchange to be "achieved by using the Auto-Negotiation functionality
> defined in Clause 37 of the IEEE Specification 802.3z".
> 
> The differences to clause 37 auto-negotiation are specified by the
> respective standards. In the case of SGMII, the differences are spelled
> out as being:
> 
> - A reduction of the link timer value, from 10 ms to 1.6 ms.
> - A customization of the tx_config_reg[15:0], mostly to allow
>   propagation of speed information.
> 
> A similar situation is going on for USXGMII as well: "USXGMII Auto-neg
> mechanism is based on Clause 37 (Figure 37-6) plus additional management
> control to select USXGMII mode".
> 
> The point is, both Cisco standards make explicit reference that they
> require an auto-negotiation state machine implemented as per "Figure
> 37-6-Auto-Negotiation state diagram" from IEEE 802.3. In the SGMII spec,
> it is very clearly pointed out that both the MAC PCS (Figure 3 MAC
> Functional Block) and the PHY PCS (Figure 2 PHY Functional Block)
> contain an auto-negotiation block defined by "Auto-Negotiation Figure
> 37-6".
> 
> Since both ends of the SGMII/USXGMII link implement the same state
> machine (just carry different tx_config_reg payloads, which they convey
> to their link partner via /C/ ordered sets), naturally the ability to
> restart auto-negotiation is symmetrical. The state machine in IEEE 802.3
> Figure 37-6 specifies the signal that triggers an auto-negotiation
> restart as being "mr_restart_an=TRUE".
> 
> Furthermore, clause "37.2.5.1.9 State diagram variable to management
> register mapping", through its "Table 37-8-PCS state diagram variable to
> management register mapping", requires a PCS compliant to clause 37 to
> expose the mr_restart_an signal to management through MDIO register "0.9
> Auto-Negotiation restart", aka BMCR_ANRESTART in Linux terms.
> 
> The Felix PCS for SGMII and USXGMII is compliant to clause 37, so it
> exposes BMCR_ANRESTART to the operating system. When this bit is
> asserted, the following happens:
> 
> 1. STATUS[Auto_Negotiation_Complete] goes from 1->0.
> 2. The PCS starts sending AN sequences instead of packets or IDLEs.
> 3. The PCS waits to receive AN sequences from PHY and matches them.
> 4. Once it has received  matching AN sequences and a PHY acknowledge,
>    STATUS[Auto_Negotiation_Complete] goes from 0->1.
> 5. Normal packet transmission restarts.
> 
> Otherwise stated, the MAC PCS has the ability to re-trigger a switch of
> the lane from data mode into configuration mode, then control
> information exchange takes place, then the lane is switched back into
> data mode. These 5 steps are collectively described as "restart AN state
> machine" by the PCS documentation.
> This is all as per IEEE 802.3 Clause 37 AN state machine, which SGMII
> and USXGMII do not touch at this fundamental level.
> 
> Now, it is true that the Cisco SGMII and USXGMII specs mention that the
> control information exchange has a unidirectional meaning. That is, the
> PHY restarts the clause 37 auto-negotiation upon any change in MDI
> auto-negotiation parameters.
> 
> PHYLINK takes this fact a bit further, and since the fact that for
> SGMII/USXGMII, the MAC PCS conveys no new information to the PHY PCS
> (beyond acknowledging the received config word), does not have any use
> for permitting the MAC PCS to trigger a restart of the clause 37
> auto-negotiation.
> 
> The only SERDES protocols for which PHYLINK allows that are 1000Base-X
> and 2500Base-X. For those, the control information exchange _is_
> bidirectional (local PCS specifies its duplex and flow control
> abilities) since the link partner is at the other side of the media.
> 
> For any other SERDES protocols, the .phylink_mac_an_restart callback is
> dead code. This is probably OK, I can't come up with a situation where
> it might be useful for the MAC PCS to clear its cache of link state and
> ask for a new tx_config_reg.
> 
> So remove this code.

NAK for this description.  You know why.

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

Re: [PATCH v2 net-next 5/6] net: dsa: felix: delete .phylink_mac_an_restart code

[Vladimir Oltean]

On Sat, Jul 04, 2020 at 03:56:14PM +0100, Russell King - ARM Linux admin wrote:

[snip]

> 
> NAK for this description.  You know why.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Sorry, I cannot work with "too busy" (your feedback from v1) and "you
know why". If there's anything incorrect in the description of the
patch, please point it out and I will change it.

There seems to be a disconnect between what I thought this phylink
callback does (and hence the reason why the code I'm deleting exists)
and what it really does. That disconnect is explained in enough detail
that even somebody who isn't intimately familiar with phylink and/or
clause 37 AN can understand. Then a justification of why deleting this
code is, at least given what we know now, the right thing to do.

I am really not trying to make any more waves than necessary, so please
help me to formulate the description in a way that is acceptable for
merging into the mainline Linux kernel.

-Vladimir

Re: [PATCH v2 net-next 5/6] net: dsa: felix: delete .phylink_mac_an_restart code

[Russell King - ARM Linux admin]

On Sat, Jul 04, 2020 at 06:50:48PM +0300, Vladimir Oltean wrote:
> On Sat, Jul 04, 2020 at 03:56:14PM +0100, Russell King - ARM Linux admin wrote:
> 
> [snip]
> 
> > 
> > NAK for this description.  You know why.
> > 
> > -- 
> > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
> 
> Sorry, I cannot work with "too busy" (your feedback from v1) and "you
> know why". If there's anything incorrect in the description of the
> patch, please point it out and I will change it.

Let's recap.

I have explained to you on numerous instances that:

- as part of the ethtool program, there is the facility to restart
  negotiation, which users expect to cause the media to renegotiate.

- when dealing with a link that involves a conventional copper PHY,
  irrespective of how that PHY is connected, this has always resulted
  in the copper PHY being requested to restart negotiation on the
  media side.

- in order to provide this same capability for fibre links where
  negotiation is supported, phylink provides the ability to pass that
  request to the PCS, since that is the media facing hardware block
  responsible for on-media negotiation.

- for SGMII, there is no advertisement from the MAC per-se, it is only
  an acknowledgement that the MAC has received the configuration word
  from the PHY.

It is also true that phylink uses this when there may be a change to
the PCS advertisement - again, to support the ability for the user to
change the media-side advertisement.  There is no media side
advertisement in SGMII at the MAC PCS.

There has been code in phylink that avoids calling the an_restart
methods since day one, as a result of the above.

Let's now look at the first version of your commit message:
| In hardware, the AN_RESTART field for these SerDes protocols (SGMII,
| USXGMII) clears the resolved configuration from the PCS's
| auto-negotiation state machine.
| 
| But PHYLINK has a different interpretation of "AN restart". It assumes
| that this Linux system is capable of re-triggering an auto-negotiation
| sequence, something which is only possible with 1000Base-X and
| 2500Base-X, where the auto-negotiation is symmetrical. In SGMII and
| USXGMII, there's an AN master and an AN slave, and it isn't so much of
| "auto-negotiation" as it is "PHY passing the resolved link state on to
| the MAC".
| 
| So, in PHYLINK's interpretation of "AN restart", it doesn't make sense
| to do anything for SGMII and USXGMII. In fact, PHYLINK won't even call
| us for any other SerDes protocol than 1000Base-X and 2500Base-X. But we
| are not supporting those. So just remove this code.

This comes over as blaming phylink for an interpretation of "AN
restart" that does not conform to your ideas.  While it is true that
phylink has a "different interpretation", that interpretation comes
from the interface that this callback is implementing, which is for
the user-level interface.  So, the "blame" that comes over in this
commit message is completely unjustified.

You also capitalised "PHYLINK" throughout this message for some reason,
which comes over as a stressed word (capitals is generally interpreted
as stress or shouting.)  Then there's "this Linux system" which sounds
a bit spiteful.

None of those things belong in a commit message, so I objected to it,
explicitly asking you to (quote) "So, please, lay off your phylink
bashing in your commit messages."

The replacement that you sent was worse - it continues this theme,
taking it further:

| The point is, both Cisco standards make explicit reference that they
| require an auto-negotiation state machine implemented as per "Figure
| 37-6-Auto-Negotiation state diagram" from IEEE 802.3. In the SGMII spec,
| it is very clearly pointed out that both the MAC PCS (Figure 3 MAC
| Functional Block) and the PHY PCS (Figure 2 PHY Functional Block)
| contain an auto-negotiation block defined by "Auto-Negotiation Figure
| 37-6".

Specifically, "The point is, ..." and "very clearly pointed out" are
completely unnecessary in a commit message, it gives a lecturing tone
to this text.  The lecturing tone continues throughout the entire text.

| PHYLINK takes this fact a bit further, and since the fact that for
| SGMII/USXGMII, the MAC PCS conveys no new information to the PHY PCS
| (beyond acknowledging the received config word), does not have any use
| for permitting the MAC PCS to trigger a restart of the clause 37
| auto-negotiation.

Again, it is not phylink that "takes this fact a bit further".  Phylink
is implementing the needs of userspace via this callback, which is to
cause autonegotiation to restart on the media.

| The only SERDES protocols for which PHYLINK allows that are 1000Base-X
| and 2500Base-X. For those, the control information exchange _is_
| bidirectional (local PCS specifies its duplex and flow control
| abilities) since the link partner is at the other side of the media.

This avoids the point that I have been making for a long time now
about what phylink is doing here.

Let me re-cap: phylink implements what is required to support the
network driver in implementing the what the user expects from the APIs
exposed by the kernel. One of the APIs is to restart negotiation, which
is generally accepted to mean the on-media negotiation, rather than
whatever internal negotiation happens within their "network interface".

Hence, it is appropriate that phylink restricts this to situations
where it is known that the media link is terminated on hardware that
phylink is responsible for.

At the moment, the known cases are:
- at the phylib PHY when dealing with conventional twisted pair cabling.
- at the phylib PHY where one is involved in a fibre link.
- at the PCS, where one is involved in a fibre link (which means
  1000base-X or 2500base-X.)

Since SGMII and USXGMII are designed for use between a PHY and the
host system (hence internal to the network interface), rather than over
some user accessible media, there is little point universally making
that call in response to a user request to restart the media
negotiation.

There is two final points to make:

- if we discover a requirement where we need to restart SGMII or
  USXGMII at the MAC PCS end (thank you for showing me that it is
  possible) then, yes, we will have to revisit how we deal with this.
  Yes, we may wish the callback to restart SGMII and USXGMII at that
  point.  However, we do not want to do that if the user requests a
  media side renegotiation.  As I have already explained, restarting
  negotiation on the media side at the PHY will cause a fresh exchange
  - not once, but twice - on the SGMII and USXGMII side anyway, which
  will refresh the configuration.

  The exception to that is if we have a buggy SGMII or USXGMII
  implementation - and, again, when we have such a scenario, that is
  the time to adapt.

- changing the behaviour now that we have several users without good
  reason is inviting regressions - there is the possibility for a state
  machine error if both ends of the link are hit for a renegotiation.
  Yes, I'm being cautious there, but there is always risk to change,
  and if there is no benefit from making that change then it stands to
  reason that there is no net benefit from making that change.

So, to sum up, your commit message _only_ needs to describe the change
you are making.  You should not lecture in a commit message, and you
should use neutral language.

If there is something lacking in the understanding of the callback,
the right place to fix that is in the documentation within the kernel,
not buried in some commit message for some obscure driver that no one
is going to even look at while developing their own driver.  Even so,
such documentation should clearly but briefly explain what is going on.

I have just spent the last 1h40 composing this message - I've put a lot
of thought into it. I obviously do not have the capacity to do that all
the time.

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

Re: [PATCH v2 net-next 5/6] net: dsa: felix: delete .phylink_mac_an_restart code

[Vladimir Oltean]

On Sat, Jul 04, 2020 at 07:14:01PM +0100, Russell King - ARM Linux admin wrote:
> On Sat, Jul 04, 2020 at 06:50:48PM +0300, Vladimir Oltean wrote:
> > On Sat, Jul 04, 2020 at 03:56:14PM +0100, Russell King - ARM Linux admin wrote:
> > 
> > [snip]
> > 
> > > 
> > > NAK for this description.  You know why.
> > > 
> > > -- 
> > > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
> > 
> > Sorry, I cannot work with "too busy" (your feedback from v1) and "you
> > know why". If there's anything incorrect in the description of the
> > patch, please point it out and I will change it.
> 
> Let's recap.
> 
> I have explained to you on numerous instances that:
> 
> - as part of the ethtool program, there is the facility to restart
>   negotiation, which users expect to cause the media to renegotiate.
> 
> - when dealing with a link that involves a conventional copper PHY,
>   irrespective of how that PHY is connected, this has always resulted
>   in the copper PHY being requested to restart negotiation on the
>   media side.
> 
> - in order to provide this same capability for fibre links where
>   negotiation is supported, phylink provides the ability to pass that
>   request to the PCS, since that is the media facing hardware block
>   responsible for on-media negotiation.
> 
> - for SGMII, there is no advertisement from the MAC per-se, it is only
>   an acknowledgement that the MAC has received the configuration word
>   from the PHY.
> 
> It is also true that phylink uses this when there may be a change to
> the PCS advertisement - again, to support the ability for the user to
> change the media-side advertisement.  There is no media side
> advertisement in SGMII at the MAC PCS.
> 

No comment there, my revised commit message confirms indeed that there's
nothing to advertise from MAC side in SGMII/USXGMII mode. The initial
commit message leaves that piece of info up in the air.

> There has been code in phylink that avoids calling the an_restart
> methods since day one, as a result of the above.
> 
> Let's now look at the first version of your commit message:
> | In hardware, the AN_RESTART field for these SerDes protocols (SGMII,
> | USXGMII) clears the resolved configuration from the PCS's
> | auto-negotiation state machine.
> | 
> | But PHYLINK has a different interpretation of "AN restart". It assumes
> | that this Linux system is capable of re-triggering an auto-negotiation
> | sequence, something which is only possible with 1000Base-X and
> | 2500Base-X, where the auto-negotiation is symmetrical. In SGMII and
> | USXGMII, there's an AN master and an AN slave, and it isn't so much of
> | "auto-negotiation" as it is "PHY passing the resolved link state on to
> | the MAC".
> | 
> | So, in PHYLINK's interpretation of "AN restart", it doesn't make sense
> | to do anything for SGMII and USXGMII. In fact, PHYLINK won't even call
> | us for any other SerDes protocol than 1000Base-X and 2500Base-X. But we
> | are not supporting those. So just remove this code.
> 
> This comes over as blaming phylink for an interpretation of "AN
> restart" that does not conform to your ideas.  While it is true that
> phylink has a "different interpretation", that interpretation comes
> from the interface that this callback is implementing, which is for
> the user-level interface.  So, the "blame" that comes over in this
> commit message is completely unjustified.
> 

I also apologized for being imprecise, but you are NACK'ing v2 for v1's
wording here. But, there are also better reasons below.

> You also capitalised "PHYLINK" throughout this message for some reason,
> which comes over as a stressed word (capitals is generally interpreted
> as stress or shouting.)  Then there's "this Linux system" which sounds
> a bit spiteful.
> 

I've been using PHYLINK using capitals for a lot of time now, nothing to
do with shouting, just with the fact that I also spell "Ethernet PHY"
with capitals. I'll change to "Phylink" or "phylink", depending on the
word's location within the phrase, and I'll also ask the people I know
to use this notation.

> None of those things belong in a commit message, so I objected to it,
> explicitly asking you to (quote) "So, please, lay off your phylink
> bashing in your commit messages."
> 
> The replacement that you sent was worse - it continues this theme,
> taking it further:
> 
> | The point is, both Cisco standards make explicit reference that they
> | require an auto-negotiation state machine implemented as per "Figure
> | 37-6-Auto-Negotiation state diagram" from IEEE 802.3. In the SGMII spec,
> | it is very clearly pointed out that both the MAC PCS (Figure 3 MAC
> | Functional Block) and the PHY PCS (Figure 2 PHY Functional Block)
> | contain an auto-negotiation block defined by "Auto-Negotiation Figure
> | 37-6".
> 
> Specifically, "The point is, ..." and "very clearly pointed out" are
> completely unnecessary in a commit message, it gives a lecturing tone
> to this text.  The lecturing tone continues throughout the entire text.
> 

I ramble quite a lot in commit messages, it's nothing personal.
Sometimes, among the rambling I say something useful too. I'll try to
value maintainers' time more by using more succint phrases.

> | PHYLINK takes this fact a bit further, and since the fact that for
> | SGMII/USXGMII, the MAC PCS conveys no new information to the PHY PCS
> | (beyond acknowledging the received config word), does not have any use
> | for permitting the MAC PCS to trigger a restart of the clause 37
> | auto-negotiation.
> 
> Again, it is not phylink that "takes this fact a bit further".  Phylink
> is implementing the needs of userspace via this callback, which is to
> cause autonegotiation to restart on the media.
> 
> | The only SERDES protocols for which PHYLINK allows that are 1000Base-X
> | and 2500Base-X. For those, the control information exchange _is_
> | bidirectional (local PCS specifies its duplex and flow control
> | abilities) since the link partner is at the other side of the media.
> 
> This avoids the point that I have been making for a long time now
> about what phylink is doing here.
> 
> Let me re-cap: phylink implements what is required to support the
> network driver in implementing the what the user expects from the APIs
> exposed by the kernel. One of the APIs is to restart negotiation, which
> is generally accepted to mean the on-media negotiation, rather than
> whatever internal negotiation happens within their "network interface".
> 
> Hence, it is appropriate that phylink restricts this to situations
> where it is known that the media link is terminated on hardware that
> phylink is responsible for.
> 

It doesn't avoid that point. ACK, ethtool -r exists, and .mac_an_restart
can be used to implement it under some circumstances. More below.

> At the moment, the known cases are:
> - at the phylib PHY when dealing with conventional twisted pair cabling.
> - at the phylib PHY where one is involved in a fibre link.
> - at the PCS, where one is involved in a fibre link (which means
>   1000base-X or 2500base-X.)
> 
> Since SGMII and USXGMII are designed for use between a PHY and the
> host system (hence internal to the network interface), rather than over
> some user accessible media, there is little point universally making
> that call in response to a user request to restart the media
> negotiation.
> 
> There is two final points to make:
> 
> - if we discover a requirement where we need to restart SGMII or
>   USXGMII at the MAC PCS end (thank you for showing me that it is
>   possible) then, yes, we will have to revisit how we deal with this.
>   Yes, we may wish the callback to restart SGMII and USXGMII at that
>   point.  However, we do not want to do that if the user requests a
>   media side renegotiation.  As I have already explained, restarting
>   negotiation on the media side at the PHY will cause a fresh exchange
>   - not once, but twice - on the SGMII and USXGMII side anyway, which
>   will refresh the configuration.
> 
>   The exception to that is if we have a buggy SGMII or USXGMII
>   implementation - and, again, when we have such a scenario, that is
>   the time to adapt.
> 
> - changing the behaviour now that we have several users without good
>   reason is inviting regressions - there is the possibility for a state
>   machine error if both ends of the link are hit for a renegotiation.
>   Yes, I'm being cautious there, but there is always risk to change,
>   and if there is no benefit from making that change then it stands to
>   reason that there is no net benefit from making that change.
> 

Yes, I am definitely not suggesting a phylink API change or
reinterpretation of existing API at this point. That would be confusing,
and "confusing" is what I want to avoid, perhaps by using more words
than necessary. I will happily accept that I am the only one who
misunderstood the API on this particular aspect.

> So, to sum up, your commit message _only_ needs to describe the change
> you are making.  You should not lecture in a commit message, and you
> should use neutral language.
> 

Ok, I will try to keep it shorter in v3 and lose the lecturing tone.

> If there is something lacking in the understanding of the callback,
> the right place to fix that is in the documentation within the kernel,
> not buried in some commit message for some obscure driver that no one
> is going to even look at while developing their own driver.  Even so,
> such documentation should clearly but briefly explain what is going on.
> 

There were a number of other points you've made in this text, all of
which boil down to one idea: that restarting SGMII AN from MAC side does
not trigger an MDI-side auto-negotiation process, so it cannot be used
for implementing the behavior expected by the user for "ethtool -r".

I will try to address those points centrally, here, by asking 2
questions.

1. In various topics you have brought up a certain copper SFP module
   from Mikrotik which embeds an inaccessible Atheros SGMII PHY. Mind
   you, I have never interacted with that SFP, but, I have a question
   out of sheer curiosity. How does ethtool -r currently work for such a
   system?

   [ I am not going to use this argument to lean this particular
   discussion in either direction (read: even if my hunch is right and
   restarting AN on the MAC PCS _could_ be the only way to implement
   ethtool -r there, I still don't care enough about that one-off case
   to change the phylink API, for the time being), but I _would_ like
   to know ]

2. There are some 1000Base-T PHYs, such as VSC8234 (which I know from
   first-hand experience, in fact there's even a comment in
   felix_vsc9959.c about it), which restart their MDI-side AN when they
   detect a transition of the system side from data mode to
   configuration mode [ initiated by the MAC ].
   Is this behavior implied by any standard (probably IEEE)? That I
   didn't check. Is this behavior also at least consistent with the
   non-SFP SGMII Atheros PHYs I have? I didn't check that either.
   Anyway, food for thought.

> I have just spent the last 1h40 composing this message - I've put a lot
> of thought into it. I obviously do not have the capacity to do that all
> the time.
> 

Thank you, it shows that you've put some more time and thought into this
reply. Maybe some balance would work a lot better overall?

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

-Vladimir

Re: [PATCH v2 net-next 5/6] net: dsa: felix: delete .phylink_mac_an_restart code

[Russell King - ARM Linux admin]

On Sat, Jul 04, 2020 at 11:29:18PM +0300, Vladimir Oltean wrote:
> I will try to address those points centrally, here, by asking 2
> questions.
> 
> 1. In various topics you have brought up a certain copper SFP module
>    from Mikrotik which embeds an inaccessible Atheros SGMII PHY. Mind
>    you, I have never interacted with that SFP, but, I have a question
>    out of sheer curiosity. How does ethtool -r currently work for such a
>    system?

It does not, but we should probably error out if we're in SGMII mode
and we have no PHY, so userspace knows that the request could not be
satisfied.

>    [ I am not going to use this argument to lean this particular
>    discussion in either direction (read: even if my hunch is right and
>    restarting AN on the MAC PCS _could_ be the only way to implement
>    ethtool -r there, I still don't care enough about that one-off case
>    to change the phylink API, for the time being), but I _would_ like
>    to know ]

Even if we did, it will not cause the media side of the Atheros PHY to
renegotiate - the Atheros PHY makes no mention that restarting the
SGMII exchange has any effect on the media side.  I've just tried it
(again) this time with the module plugged in the LX2160A rather than
a Marvell platform - here's the PCS register dump:

00: 0x1140 0x002d 0x0083 0xe400 0x4001 0xd801 0x0006 0x0000
08: 0x0000 0x0000 0x0000 0x0000 0x0000 0x0000 0x0000 0x0000
10: 0x0000 0x0001 0x0d40 0x0003 0x0003 0xdab6 0x0000 0x0000
18: 0x0000 0x0000 0x0000 0x0000 0x0000 0x0000 0x0000 0x0000

If I write 0x1340 to the BMCR, nothing happens on the media side - it
remains linked with the switch the RJ45 cable is plugged in to, which
means nothing happened on the media side.

> 2. There are some 1000Base-T PHYs, such as VSC8234 (which I know from
>    first-hand experience, in fact there's even a comment in
>    felix_vsc9959.c about it), which restart their MDI-side AN when they
>    detect a transition of the system side from data mode to
>    configuration mode [ initiated by the MAC ].
>    Is this behavior implied by any standard (probably IEEE)? That I
>    didn't check. Is this behavior also at least consistent with the
>    non-SFP SGMII Atheros PHYs I have? I didn't check that either.
>    Anyway, food for thought.

The first thing to straighten out in your comment above is that SGMII
is a Cisco modification of IEEE 802.3 1000BASE-X.  SGMII has not been
incorporated into IEEE 802.3.

I'm going to start with a description of the two - which will aid the
later explanation, so please bear with me.

1000BASE-X deals with gigabit ethernet over Fibre media - and what you
have there (from a practical point, rather than the ISO levels that
are shown in IEEE 802.3) is:

MAC <-> PCS <-> Serdes <-> Optical media <-> Serdes <-> PCS <-> MAC

Each PCS transmits the abilities of its respective end, and receives
the abilities from the remote end. The abilities consists only of
duplex and pause modes, since speed is fixed at gigabit.  Negotiation
can be restarted by either end.

Cisco SGMII took this, and decided to make several modifications to
support a PHY instead of optical media, the most important being:

1) addition of symbol replication for 100M and 10M over the gigabit
   path without changing the bitrate of the path.
2) changed the contents of the configuration word to allow the PHY
   to inform the MAC of the current speed and duplex.

So we end up with:

MAC <-> PCS <-> Serdes <-> PHY <-> Media ...

Given that this is the case, IEEE 802.3 does not cover this setup -
having a PHY attached is beyond the 1000BASE-X specification that
it covers.  So there is nothing in there to mandate that a SGMII PHY
should restart its media side negotiation due to the SGMII side
restarting.

If it were required, it would be in the Cisco SGMII specification. As
it doesn't even make explicit mention of restarting the SGMII exchange
from the MAC end, I really doubt that it would make any comment about
a restart of the SGMII exchange restarting the media side.

So, we're down to the vaguaries of the various PHY manufacturers.

As I've shown, Atheros AR803x do not restart their media side on SGMII
side "negotiation" events.  I've just tested with the Marvell 88E1111,
which is probably the most popular PHY for copper gigabit SFPs out
there, and that also does not restart the media side either.

As for VSC8234, the comment you refer to is:

	Some PHYs like VSC8234 don't like it when AN restarts on
	their system side and they restart line side AN too, going
	into an endless link up/down loop.

However, without knowing in detail what is happening on the SGMII link,
it would be difficult to really know what is going on. It could be that
the implementation in the PHY is fine but has this additional vendor
feature, but the host side always triggers a second exchange of the
configuration word each time that the PHY notifies the MAC PCS of an
updated configuration word.  It could also be a misfeature of the PHY
itself.

It is possible to detect which mode the VSC8234 PHY is in when
connected to the Lynx PCS by looking at the link-partner advertisement
register (register 5) when an AN exchange has completed. Bit 0 is a good
indicator whether the PHY is operating in SGMII mode (1) or 1000BASE-X
mode (0).

There is another possibility, however.  That is the VSC8234 is not in
SGMII mode, but is in 1000BASE-X.  I'm aware of some copper SFPs that
use 1000BASE-X rather than SGMII, where the advertisement from the MAC
PCS to the SFP affects the media side duplex and pause advertisement,
and so any change on the host side causes the media side to restart.
It is, however, unlikely that a PHY configured in 1000BASE-X will be
able to complete negotiation with a host in SGMII mode - the duplex
bits will both be zero leading to an invalid resolution.

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

[PATCH v2 net-next 6/6] net: dsa: felix: use resolved link config in mac_link_up()

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

PHYLINK now requires that parameters established through
auto-negotiation be written into the MAC at the time of the
mac_link_up() callback. In the case of felix, that means taking the port
out of reset, setting the correct timers for PAUSE frames, and
enabling/disabling TX flow control.

This patch also splits the inband and noinband configuration of the
vsc9959 PCS (currently found in a function called "init") into 2
different functions, which have a nomenclature closer to PHYLINK:
"config", for inband setup, and "link_up", for noinband (forced) setup.
This is necessary as a preparation step for giving up control of the PCS
to PHYLINK, which will be done in further patches.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Changes in v2:
Rebase on top of new patch 1/6.

 drivers/net/dsa/ocelot/felix.c         |  76 ++++----
 drivers/net/dsa/ocelot/felix.h         |  10 +-
 drivers/net/dsa/ocelot/felix_vsc9959.c | 257 ++++++++++++++-----------
 3 files changed, 187 insertions(+), 156 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 57c400a67f16..75652ed99b24 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -233,6 +233,32 @@ static int felix_phylink_mac_pcs_get_state(struct dsa_switch *ds, int port,
 static void felix_phylink_mac_config(struct dsa_switch *ds, int port,
 				     unsigned int link_an_mode,
 				     const struct phylink_link_state *state)
+{
+	struct ocelot *ocelot = ds->priv;
+	struct felix *felix = ocelot_to_felix(ocelot);
+
+	if (felix->info->pcs_config)
+		felix->info->pcs_config(ocelot, port, link_an_mode, state);
+}
+
+static void felix_phylink_mac_link_down(struct dsa_switch *ds, int port,
+					unsigned int link_an_mode,
+					phy_interface_t interface)
+{
+	struct ocelot *ocelot = ds->priv;
+	struct ocelot_port *ocelot_port = ocelot->ports[port];
+
+	ocelot_port_writel(ocelot_port, 0, DEV_MAC_ENA_CFG);
+	ocelot_rmw_rix(ocelot, 0, QSYS_SWITCH_PORT_MODE_PORT_ENA,
+		       QSYS_SWITCH_PORT_MODE, port);
+}
+
+static void felix_phylink_mac_link_up(struct dsa_switch *ds, int port,
+				      unsigned int link_an_mode,
+				      phy_interface_t interface,
+				      struct phy_device *phydev,
+				      int speed, int duplex,
+				      bool tx_pause, bool rx_pause)
 {
 	struct ocelot *ocelot = ds->priv;
 	struct ocelot_port *ocelot_port = ocelot->ports[port];
@@ -250,7 +276,7 @@ static void felix_phylink_mac_config(struct dsa_switch *ds, int port,
 			   DEV_CLOCK_CFG_LINK_SPEED(OCELOT_SPEED_1000),
 			   DEV_CLOCK_CFG);
 
-	switch (state->speed) {
+	switch (speed) {
 	case SPEED_10:
 		mac_fc_cfg = SYS_MAC_FC_CFG_FC_LINK_SPEED(3);
 		break;
@@ -261,12 +287,9 @@ static void felix_phylink_mac_config(struct dsa_switch *ds, int port,
 	case SPEED_2500:
 		mac_fc_cfg = SYS_MAC_FC_CFG_FC_LINK_SPEED(1);
 		break;
-	case SPEED_UNKNOWN:
-		mac_fc_cfg = SYS_MAC_FC_CFG_FC_LINK_SPEED(0);
-		break;
 	default:
 		dev_err(ocelot->dev, "Unsupported speed on port %d: %d\n",
-			port, state->speed);
+			port, speed);
 		return;
 	}
 
@@ -275,7 +298,7 @@ static void felix_phylink_mac_config(struct dsa_switch *ds, int port,
 	 */
 	mac_fc_cfg |= SYS_MAC_FC_CFG_RX_FC_ENA;
 
-	if (state->pause & MLO_PAUSE_TX)
+	if (tx_pause)
 		mac_fc_cfg |= SYS_MAC_FC_CFG_TX_FC_ENA |
 			      SYS_MAC_FC_CFG_PAUSE_VAL_CFG(0xffff) |
 			      SYS_MAC_FC_CFG_FC_LATENCY_CFG(0x7) |
@@ -288,37 +311,9 @@ static void felix_phylink_mac_config(struct dsa_switch *ds, int port,
 
 	ocelot_write_rix(ocelot, 0, ANA_POL_FLOWC, port);
 
-	if (felix->info->pcs_init)
-		felix->info->pcs_init(ocelot, port, link_an_mode, state);
-
-	if (felix->info->port_sched_speed_set)
-		felix->info->port_sched_speed_set(ocelot, port,
-						  state->speed);
-}
-
-static void felix_phylink_mac_link_down(struct dsa_switch *ds, int port,
-					unsigned int link_an_mode,
-					phy_interface_t interface)
-{
-	struct ocelot *ocelot = ds->priv;
-	struct ocelot_port *ocelot_port = ocelot->ports[port];
-
-	ocelot_port_writel(ocelot_port, 0, DEV_MAC_ENA_CFG);
-	ocelot_rmw_rix(ocelot, 0, QSYS_SWITCH_PORT_MODE_PORT_ENA,
-		       QSYS_SWITCH_PORT_MODE, port);
-}
-
-static void felix_phylink_mac_link_up(struct dsa_switch *ds, int port,
-				      unsigned int link_an_mode,
-				      phy_interface_t interface,
-				      struct phy_device *phydev,
-				      int speed, int duplex,
-				      bool tx_pause, bool rx_pause)
-{
-	struct ocelot *ocelot = ds->priv;
-	struct ocelot_port *ocelot_port = ocelot->ports[port];
-
-	/* Enable MAC module */
+	/* Undo the effects of felix_phylink_mac_link_down:
+	 * enable MAC module
+	 */
 	ocelot_port_writel(ocelot_port, DEV_MAC_ENA_CFG_RX_ENA |
 			   DEV_MAC_ENA_CFG_TX_ENA, DEV_MAC_ENA_CFG);
 
@@ -335,6 +330,13 @@ static void felix_phylink_mac_link_up(struct dsa_switch *ds, int port,
 			 QSYS_SWITCH_PORT_MODE_SCH_NEXT_CFG(1) |
 			 QSYS_SWITCH_PORT_MODE_PORT_ENA,
 			 QSYS_SWITCH_PORT_MODE, port);
+
+	if (felix->info->pcs_link_up)
+		felix->info->pcs_link_up(ocelot, port, link_an_mode, interface,
+					 speed, duplex);
+
+	if (felix->info->port_sched_speed_set)
+		felix->info->port_sched_speed_set(ocelot, port, speed);
 }
 
 static void felix_port_qos_map_init(struct ocelot *ocelot, int port)
diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h
index 4a4cebcf04a7..00137b64132b 100644
--- a/drivers/net/dsa/ocelot/felix.h
+++ b/drivers/net/dsa/ocelot/felix.h
@@ -28,9 +28,13 @@ struct felix_info {
 	int				imdio_pci_bar;
 	int	(*mdio_bus_alloc)(struct ocelot *ocelot);
 	void	(*mdio_bus_free)(struct ocelot *ocelot);
-	void	(*pcs_init)(struct ocelot *ocelot, int port,
-			    unsigned int link_an_mode,
-			    const struct phylink_link_state *state);
+	void	(*pcs_config)(struct ocelot *ocelot, int port,
+			      unsigned int link_an_mode,
+			      const struct phylink_link_state *state);
+	void	(*pcs_link_up)(struct ocelot *ocelot, int port,
+			       unsigned int link_an_mode,
+			       phy_interface_t interface,
+			       int speed, int duplex);
 	void	(*pcs_link_state)(struct ocelot *ocelot, int port,
 				  struct phylink_link_state *state);
 	int	(*prevalidate_phy_mode)(struct ocelot *ocelot, int port,
diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 65f83386bad1..19614537b1ba 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -737,124 +737,54 @@ static int vsc9959_reset(struct ocelot *ocelot)
  * traffic if SGMII AN is enabled but not completed (acknowledged by us), so
  * setting MLO_AN_INBAND is actually required for those.
  */
-static void vsc9959_pcs_init_sgmii(struct phy_device *pcs,
-				   unsigned int link_an_mode,
-				   const struct phylink_link_state *state)
+static void vsc9959_pcs_config_sgmii(struct phy_device *pcs,
+				     unsigned int link_an_mode,
+				     const struct phylink_link_state *state)
 {
-	if (link_an_mode == MLO_AN_INBAND) {
-		int bmsr, bmcr;
-
-		/* Some PHYs like VSC8234 don't like it when AN restarts on
-		 * their system  side and they restart line side AN too, going
-		 * into an endless link up/down loop.  Don't restart PCS AN if
-		 * link is up already.
-		 * We do check that AN is enabled just in case this is the 1st
-		 * call, PCS detects a carrier but AN is disabled from power on
-		 * or by boot loader.
-		 */
-		bmcr = phy_read(pcs, MII_BMCR);
-		if (bmcr < 0)
-			return;
-
-		bmsr = phy_read(pcs, MII_BMSR);
-		if (bmsr < 0)
-			return;
-
-		if ((bmcr & BMCR_ANENABLE) && (bmsr & BMSR_LSTATUS))
-			return;
-
-		/* SGMII spec requires tx_config_Reg[15:0] to be exactly 0x4001
-		 * for the MAC PCS in order to acknowledge the AN.
-		 */
-		phy_write(pcs, MII_ADVERTISE, ADVERTISE_SGMII |
-					      ADVERTISE_LPACK);
-
-		phy_write(pcs, ENETC_PCS_IF_MODE,
-			  ENETC_PCS_IF_MODE_SGMII_EN |
-			  ENETC_PCS_IF_MODE_USE_SGMII_AN);
-
-		/* Adjust link timer for SGMII */
-		phy_write(pcs, ENETC_PCS_LINK_TIMER1,
-			  ENETC_PCS_LINK_TIMER1_VAL);
-		phy_write(pcs, ENETC_PCS_LINK_TIMER2,
-			  ENETC_PCS_LINK_TIMER2_VAL);
-
-		phy_set_bits(pcs, MII_BMCR, BMCR_ANENABLE);
-	} else {
-		u16 if_mode = ENETC_PCS_IF_MODE_SGMII_EN;
-		int speed;
-
-		switch (state->speed) {
-		case SPEED_1000:
-			speed = ENETC_PCS_SPEED_1000;
-			break;
-		case SPEED_100:
-			speed = ENETC_PCS_SPEED_100;
-			break;
-		case SPEED_10:
-			speed = ENETC_PCS_SPEED_10;
-			break;
-		case SPEED_UNKNOWN:
-			/* Silently don't do anything */
-			return;
-		default:
-			phydev_err(pcs, "Invalid PCS speed %d\n", state->speed);
-			return;
-		}
-
-		if_mode |= ENETC_PCS_IF_MODE_SGMII_SPEED(speed);
-		if (state->duplex == DUPLEX_HALF)
-			if_mode |= ENETC_PCS_IF_MODE_DUPLEX_HALF;
-
-		phy_clear_bits(pcs, MII_BMCR, BMCR_ANENABLE);
-	}
-}
+	int bmsr, bmcr;
+
+	/* Some PHYs like VSC8234 don't like it when AN restarts on
+	 * their system  side and they restart line side AN too, going
+	 * into an endless link up/down loop.  Don't restart PCS AN if
+	 * link is up already.
+	 * We do check that AN is enabled just in case this is the 1st
+	 * call, PCS detects a carrier but AN is disabled from power on
+	 * or by boot loader.
+	 */
+	bmcr = phy_read(pcs, MII_BMCR);
+	if (bmcr < 0)
+		return;
 
-/* 2500Base-X is SerDes protocol 7 on Felix and 6 on ENETC. It is a SerDes lane
- * clocked at 3.125 GHz which encodes symbols with 8b/10b and does not have
- * auto-negotiation of any link parameters. Electrically it is compatible with
- * a single lane of XAUI.
- * The hardware reference manual wants to call this mode SGMII, but it isn't
- * really, since the fundamental features of SGMII:
- * - Downgrading the link speed by duplicating symbols
- * - Auto-negotiation
- * are not there.
- * The speed is configured at 1000 in the IF_MODE and BMCR MDIO registers
- * because the clock frequency is actually given by a PLL configured in the
- * Reset Configuration Word (RCW).
- * Since there is no difference between fixed speed SGMII w/o AN and 802.3z w/o
- * AN, we call this PHY interface type 2500Base-X. In case a PHY negotiates a
- * lower link speed on line side, the system-side interface remains fixed at
- * 2500 Mbps and we do rate adaptation through pause frames.
- */
-static void vsc9959_pcs_init_2500basex(struct phy_device *pcs,
-				       unsigned int link_an_mode,
-				       const struct phylink_link_state *state)
-{
-	u16 if_mode = ENETC_PCS_IF_MODE_SGMII_SPEED(ENETC_PCS_SPEED_2500) |
-		      ENETC_PCS_IF_MODE_SGMII_EN;
+	bmsr = phy_read(pcs, MII_BMSR);
+	if (bmsr < 0)
+		return;
 
-	if (link_an_mode == MLO_AN_INBAND) {
-		phydev_err(pcs, "AN not supported on 3.125GHz SerDes lane\n");
+	if ((bmcr & BMCR_ANENABLE) && (bmsr & BMSR_LSTATUS))
 		return;
-	}
 
-	if (state->duplex == DUPLEX_HALF)
-		if_mode |= ENETC_PCS_IF_MODE_DUPLEX_HALF;
+	/* SGMII spec requires tx_config_Reg[15:0] to be exactly 0x4001
+	 * for the MAC PCS in order to acknowledge the AN.
+	 */
+	phy_write(pcs, MII_ADVERTISE, ADVERTISE_SGMII |
+				      ADVERTISE_LPACK);
 
-	phy_write(pcs, ENETC_PCS_IF_MODE, if_mode);
-	phy_clear_bits(pcs, MII_BMCR, BMCR_ANENABLE);
+	phy_write(pcs, ENETC_PCS_IF_MODE,
+		  ENETC_PCS_IF_MODE_SGMII_EN |
+		  ENETC_PCS_IF_MODE_USE_SGMII_AN);
+
+	/* Adjust link timer for SGMII */
+	phy_write(pcs, ENETC_PCS_LINK_TIMER1,
+		  ENETC_PCS_LINK_TIMER1_VAL);
+	phy_write(pcs, ENETC_PCS_LINK_TIMER2,
+		  ENETC_PCS_LINK_TIMER2_VAL);
+
+	phy_set_bits(pcs, MII_BMCR, BMCR_ANENABLE);
 }
 
-static void vsc9959_pcs_init_usxgmii(struct phy_device *pcs,
-				     unsigned int link_an_mode,
-				     const struct phylink_link_state *state)
+static void vsc9959_pcs_config_usxgmii(struct phy_device *pcs,
+				       unsigned int link_an_mode,
+				       const struct phylink_link_state *state)
 {
-	if (link_an_mode != MLO_AN_INBAND) {
-		phydev_err(pcs, "USXGMII only supports in-band AN for now\n");
-		return;
-	}
-
 	/* Configure device ability for the USXGMII Replicator */
 	phy_write_mmd(pcs, MDIO_MMD_VEND2, MII_ADVERTISE,
 		      USXGMII_ADVERTISE_SPEED(USXGMII_SPEED_2500) |
@@ -864,9 +794,9 @@ static void vsc9959_pcs_init_usxgmii(struct phy_device *pcs,
 		      USXGMII_ADVERTISE_FDX);
 }
 
-static void vsc9959_pcs_init(struct ocelot *ocelot, int port,
-			     unsigned int link_an_mode,
-			     const struct phylink_link_state *state)
+static void vsc9959_pcs_config(struct ocelot *ocelot, int port,
+			       unsigned int link_an_mode,
+			       const struct phylink_link_state *state)
 {
 	struct felix *felix = ocelot_to_felix(ocelot);
 	struct phy_device *pcs = felix->pcs[port];
@@ -898,16 +828,110 @@ static void vsc9959_pcs_init(struct ocelot *ocelot, int port,
 				 pcs->supported);
 	phy_advertise_supported(pcs);
 
+	if (!phylink_autoneg_inband(link_an_mode))
+		return;
+
 	switch (pcs->interface) {
 	case PHY_INTERFACE_MODE_SGMII:
 	case PHY_INTERFACE_MODE_QSGMII:
-		vsc9959_pcs_init_sgmii(pcs, link_an_mode, state);
+		vsc9959_pcs_config_sgmii(pcs, link_an_mode, state);
 		break;
 	case PHY_INTERFACE_MODE_2500BASEX:
-		vsc9959_pcs_init_2500basex(pcs, link_an_mode, state);
+		phydev_err(pcs, "AN not supported on 3.125GHz SerDes lane\n");
 		break;
 	case PHY_INTERFACE_MODE_USXGMII:
-		vsc9959_pcs_init_usxgmii(pcs, link_an_mode, state);
+		vsc9959_pcs_config_usxgmii(pcs, link_an_mode, state);
+		break;
+	default:
+		dev_err(ocelot->dev, "Unsupported link mode %s\n",
+			phy_modes(pcs->interface));
+	}
+}
+
+static void vsc9959_pcs_link_up_sgmii(struct phy_device *pcs,
+				      unsigned int link_an_mode,
+				      int speed, int duplex)
+{
+	u16 if_mode = ENETC_PCS_IF_MODE_SGMII_EN;
+
+	switch (speed) {
+	case SPEED_1000:
+		if_mode |= ENETC_PCS_IF_MODE_SGMII_SPEED(ENETC_PCS_SPEED_1000);
+		break;
+	case SPEED_100:
+		if_mode |= ENETC_PCS_IF_MODE_SGMII_SPEED(ENETC_PCS_SPEED_100);
+		break;
+	case SPEED_10:
+		if_mode |= ENETC_PCS_IF_MODE_SGMII_SPEED(ENETC_PCS_SPEED_10);
+		break;
+	default:
+		phydev_err(pcs, "Invalid PCS speed %d\n", speed);
+		return;
+	}
+
+	if (duplex == DUPLEX_HALF)
+		if_mode |= ENETC_PCS_IF_MODE_DUPLEX_HALF;
+
+	phy_write(pcs, ENETC_PCS_IF_MODE, if_mode);
+	phy_clear_bits(pcs, MII_BMCR, BMCR_ANENABLE);
+}
+
+/* 2500Base-X is SerDes protocol 7 on Felix and 6 on ENETC. It is a SerDes lane
+ * clocked at 3.125 GHz which encodes symbols with 8b/10b and does not have
+ * auto-negotiation of any link parameters. Electrically it is compatible with
+ * a single lane of XAUI.
+ * The hardware reference manual wants to call this mode SGMII, but it isn't
+ * really, since the fundamental features of SGMII:
+ * - Downgrading the link speed by duplicating symbols
+ * - Auto-negotiation
+ * are not there.
+ * The speed is configured at 1000 in the IF_MODE and BMCR MDIO registers
+ * because the clock frequency is actually given by a PLL configured in the
+ * Reset Configuration Word (RCW).
+ * Since there is no difference between fixed speed SGMII w/o AN and 802.3z w/o
+ * AN, we call this PHY interface type 2500Base-X. In case a PHY negotiates a
+ * lower link speed on line side, the system-side interface remains fixed at
+ * 2500 Mbps and we do rate adaptation through pause frames.
+ */
+static void vsc9959_pcs_link_up_2500basex(struct phy_device *pcs,
+					  unsigned int link_an_mode,
+					  int speed, int duplex)
+{
+	u16 if_mode = ENETC_PCS_IF_MODE_SGMII_SPEED(ENETC_PCS_SPEED_2500) |
+		      ENETC_PCS_IF_MODE_SGMII_EN;
+
+	if (duplex == DUPLEX_HALF)
+		if_mode |= ENETC_PCS_IF_MODE_DUPLEX_HALF;
+
+	phy_write(pcs, ENETC_PCS_IF_MODE, if_mode);
+	phy_clear_bits(pcs, MII_BMCR, BMCR_ANENABLE);
+}
+
+static void vsc9959_pcs_link_up(struct ocelot *ocelot, int port,
+				unsigned int link_an_mode,
+				phy_interface_t interface,
+				int speed, int duplex)
+{
+	struct felix *felix = ocelot_to_felix(ocelot);
+	struct phy_device *pcs = felix->pcs[port];
+
+	if (!pcs)
+		return;
+
+	if (phylink_autoneg_inband(link_an_mode))
+		return;
+
+	switch (interface) {
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_QSGMII:
+		vsc9959_pcs_link_up_sgmii(pcs, link_an_mode, speed, duplex);
+		break;
+	case PHY_INTERFACE_MODE_2500BASEX:
+		vsc9959_pcs_link_up_2500basex(pcs, link_an_mode, speed,
+					      duplex);
+		break;
+	case PHY_INTERFACE_MODE_USXGMII:
+		phydev_err(pcs, "USXGMII only supports in-band AN for now\n");
 		break;
 	default:
 		dev_err(ocelot->dev, "Unsupported link mode %s\n",
@@ -1374,7 +1398,8 @@ struct felix_info felix_info_vsc9959 = {
 	.imdio_pci_bar		= 0,
 	.mdio_bus_alloc		= vsc9959_mdio_bus_alloc,
 	.mdio_bus_free		= vsc9959_mdio_bus_free,
-	.pcs_init		= vsc9959_pcs_init,
+	.pcs_config		= vsc9959_pcs_config,
+	.pcs_link_up		= vsc9959_pcs_link_up,
 	.pcs_link_state		= vsc9959_pcs_link_state,
 	.prevalidate_phy_mode	= vsc9959_prevalidate_phy_mode,
 	.port_setup_tc          = vsc9959_port_setup_tc,
-- 
2.25.1

Re: [PATCH v2 net-next 0/6] PHYLINK integration improvements for Felix DSA driver

[David Miller]

From: Vladimir Oltean <olteanv@gmail.com>
Date: Sat,  4 Jul 2020 15:45:01 +0300

> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> This is an overhaul of the Felix switch driver's PHYLINK operations.
> 
> Patches 1, 3, 4 and 5 are cleanup, patch 2 is adding a new feature and
> and patch 6 is adaptation to the new format of an existing phylink API
> (mac_link_up).
> 
> Changes since v1:
> - Now using phy_clear_bits and phy_set_bits instead of plain writes to
>   MII_BMCR. This combines former patches 1/7 and 6/7 into a single new
>   patch 1/6.
> - Updated commit message of patch 5/6.

Series applied, thanks.

Re: [PATCH v2 net-next 0/6] PHYLINK integration improvements for Felix DSA driver

[Russell King - ARM Linux admin]

On Sun, Jul 05, 2020 at 03:26:20PM -0700, David Miller wrote:
> From: Vladimir Oltean <olteanv@gmail.com>
> Date: Sat,  4 Jul 2020 15:45:01 +0300
> 
> > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > 
> > This is an overhaul of the Felix switch driver's PHYLINK operations.
> > 
> > Patches 1, 3, 4 and 5 are cleanup, patch 2 is adding a new feature and
> > and patch 6 is adaptation to the new format of an existing phylink API
> > (mac_link_up).
> > 
> > Changes since v1:
> > - Now using phy_clear_bits and phy_set_bits instead of plain writes to
> >   MII_BMCR. This combines former patches 1/7 and 6/7 into a single new
> >   patch 1/6.
> > - Updated commit message of patch 5/6.
> 
> Series applied, thanks.

v3 was posted yesterday...

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

Re: [PATCH v2 net-next 0/6] PHYLINK integration improvements for Felix DSA driver

[David Miller]

From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Date: Mon, 6 Jul 2020 09:45:04 +0100

> v3 was posted yesterday...

My tree is immutable, so you know what that means :-)

Re: [PATCH v2 net-next 0/6] PHYLINK integration improvements for Felix DSA driver

[Russell King - ARM Linux admin]

On Mon, Jul 06, 2020 at 12:54:54PM -0700, David Miller wrote:
> From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> Date: Mon, 6 Jul 2020 09:45:04 +0100
> 
> > v3 was posted yesterday...
> 
> My tree is immutable, so you know what that means :-)

I was wondering whether there was a reason why you merged v2 when v3 had
already been posted.

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

Re: [PATCH v2 net-next 0/6] PHYLINK integration improvements for Felix DSA driver

[David Miller]

From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Date: Mon, 6 Jul 2020 21:39:24 +0100

> On Mon, Jul 06, 2020 at 12:54:54PM -0700, David Miller wrote:
>> From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
>> Date: Mon, 6 Jul 2020 09:45:04 +0100
>> 
>> > v3 was posted yesterday...
>> 
>> My tree is immutable, so you know what that means :-)
> 
> I was wondering whether there was a reason why you merged v2 when v3 had
> already been posted.

I simply missed it, so relative fixups need to be sent to me.

Re: [PATCH v2 net-next 0/6] PHYLINK integration improvements for Felix DSA driver

[Florian Fainelli]

On 7/6/2020 1:46 PM, David Miller wrote:
> From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> Date: Mon, 6 Jul 2020 21:39:24 +0100
> 
>> On Mon, Jul 06, 2020 at 12:54:54PM -0700, David Miller wrote:
>>> From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
>>> Date: Mon, 6 Jul 2020 09:45:04 +0100
>>>
>>>> v3 was posted yesterday...
>>>
>>> My tree is immutable, so you know what that means :-)
>>
>> I was wondering whether there was a reason why you merged v2 when v3 had
>> already been posted.
> 
> I simply missed it, so relative fixups need to be sent to me.

The changes were more about the commit message than the code itself...
so I am not sure how this can be fixed without a fixup! or squash!
commit which you probably would not do since it would amount to
rewriting history.
-- 
Florian