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

[PATCH net-next 0/7] 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.
Some of the patches (7/7) were done at the request of Russell King
(https://patchwork.kernel.org/cover/11386917/), while others are simply
cleanup. There is also one new feature being added: half-duplex for
10/100/1000 link speeds (2/7).

Vladimir Oltean (7):
  net: dsa: felix: stop writing to read-only fields in 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: disable in-band AN in MII_BMCR without reset
  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 net-next 1/7] net: dsa: felix: stop writing to read-only fields in MII_BMCR

[Vladimir Oltean]

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

It looks like BMCR_SPEED and BMCR_DUPLEX are read-only, since they are
actually configured through the vendor-specific IF_MODE (0x14) register.
So, don't perform bogus writes to these fields, giving the impression
that those writes do something.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/ocelot/felix_vsc9959.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 2067776773f7..3269c76b59ff 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -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_write(pcs, MII_BMCR, BMCR_RESET);
 	}
 }
 
@@ -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_write(pcs, MII_BMCR, BMCR_RESET);
 }
 
 static void vsc9959_pcs_init_usxgmii(struct phy_device *pcs,
-- 
2.25.1

Re: [PATCH net-next 1/7] net: dsa: felix: stop writing to read-only fields in MII_BMCR

[Russell King - ARM Linux admin]

On Thu, Jun 25, 2020 at 06:23:25PM +0300, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> It looks like BMCR_SPEED and BMCR_DUPLEX are read-only, since they are
> actually configured through the vendor-specific IF_MODE (0x14) register.
> So, don't perform bogus writes to these fields, giving the impression
> that those writes do something.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Is this patch really worth the churn?

If the bits are read-only, and are ones, writing ones back to them seems
at least to me to be the sane thing to be doing, rather than writing
zeros.  It isn't giving a false impression IMHO.

Also note that these are documented as being used in 1000base-X mode.

"Read only bit always set to '1' to indicate that the Core (when used
as 1000Base-X) only supports Full Duplex mode of operation and does not
support HalfDuplex. Note: the SGMII mode is controlled with register
IF_MODE."

"Read only bits that define that the Core only operates in Gigabit
mode(1000Base-X): Bit 13 set to '0', Bit 6 set to '1'. Note: the SGMII
speed is controlled with register IF_MODE."

So, I think definitely for the 2500BASE-X case (which is merely
1000BASE-X clocked 2.5x faster) we certainly want to keep writing
these settings correctly as if they were writable.

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

[PATCH net-next 2/7] 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>
---
Repost of:
https://patchwork.ozlabs.org/project/netdev/patch/20200624155926.3379373-1-olteanv@gmail.com/
Changed:
In the "forced link" scenario (not previously tested, just in-band), we
need to configure half duplex through the IF_MODE register, not BMCR.

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

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 25046777c993..25b340e0a6dd 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 3269c76b59ff..c1220b488f9c 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_write(pcs, MII_BMCR, BMCR_ANRESTART | 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,10 +838,11 @@ 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_write(pcs, ENETC_PCS_IF_MODE, if_mode);
 		phy_write(pcs, MII_BMCR, BMCR_RESET);
 	}
 }
@@ -870,15 +868,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_write(pcs, MII_BMCR, BMCR_RESET);
 }
 
@@ -919,8 +920,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

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

[Russell King - ARM Linux admin]

On Thu, Jun 25, 2020 at 06:23:26PM +0300, Vladimir Oltean wrote:
> 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>

Ping isn't really a good test of whether half duplex mode is operating
correctly.  However, apart from that detail, and as this reflects the
functionality I implemented with the LX2160A version of this PCS:

Reviewed-by: Russell King <rmk+kernel@armlinux.org.uk>

> ---
> Repost of:
> https://patchwork.ozlabs.org/project/netdev/patch/20200624155926.3379373-1-olteanv@gmail.com/
> Changed:
> In the "forced link" scenario (not previously tested, just in-band), we
> need to configure half duplex through the IF_MODE register, not BMCR.
> 
>  drivers/net/dsa/ocelot/felix.c         |  4 +++-
>  drivers/net/dsa/ocelot/felix_vsc9959.c | 24 ++++++++++++++----------
>  include/linux/fsl/enetc_mdio.h         |  1 +
>  3 files changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
> index 25046777c993..25b340e0a6dd 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 3269c76b59ff..c1220b488f9c 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_write(pcs, MII_BMCR, BMCR_ANRESTART | 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,10 +838,11 @@ 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_write(pcs, ENETC_PCS_IF_MODE, if_mode);
>  		phy_write(pcs, MII_BMCR, BMCR_RESET);
>  	}
>  }
> @@ -870,15 +868,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_write(pcs, MII_BMCR, BMCR_RESET);
>  }
>  
> @@ -919,8 +920,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
> 
> 

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

[PATCH net-next 3/7] 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>
---
 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 25b340e0a6dd..d229cb5d5f9e 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 net-next 4/7] 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>
---
 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 d229cb5d5f9e..da337c63e7ca 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

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

[Russell King - ARM Linux admin]

On Thu, Jun 25, 2020 at 06:23:28PM +0300, Vladimir Oltean wrote:
> 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>
> ---
>  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 d229cb5d5f9e..da337c63e7ca 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);

I'm not that happy with the persistence of felix using the legacy
method; I can bring a horse to water but can't make it drink comes
to mind.  I'm at the point where I don't care _if_ my phylink changes
break code that I've asked people to convert and they haven't yet,
and I'm planning to send the series that /may/ cause breakage in
that regard pretty soon, so the lynx PCS changes can move forward.

I even tried to move felix forward by sending a patch for it, and I
just gave up with that approach based on your comment.

Shrug.

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

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

[Vladimir Oltean]

On Thu, 25 Jun 2020 at 19:37, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Thu, Jun 25, 2020 at 06:23:28PM +0300, Vladimir Oltean wrote:
> > 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>
> > ---
> >  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 d229cb5d5f9e..da337c63e7ca 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);
>
> I'm not that happy with the persistence of felix using the legacy
> method; I can bring a horse to water but can't make it drink comes
> to mind.  I'm at the point where I don't care _if_ my phylink changes
> break code that I've asked people to convert and they haven't yet,
> and I'm planning to send the series that /may/ cause breakage in
> that regard pretty soon, so the lynx PCS changes can move forward.
>
> I even tried to move felix forward by sending a patch for it, and I
> just gave up with that approach based on your comment.
>
> Shrug.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

The callbacks clearly work the way they are, given the current
structure of phylink, as proven by the users of this driver. Code that
isn't there yet simply isn't there yet.

What you consider "useless churn" and what I consider "useless churn"
are pretty different things.
To me, patch 7/7 is the useless churn, that's why it's at the end.
Patches 1-6 are structured in a way that is compatible with
backporting to a stable 5.4 tree. Patch 7, not so much.

The fact that you have such an attitude and you make the effort to
actually send an email complaining about me using state->speed in
.mac_config(), when literally 3 patches later I'm moving this
configuration where you want it to be, is pretty annoying.

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

[Russell King - ARM Linux admin]

On Thu, Jun 25, 2020 at 07:48:23PM +0300, Vladimir Oltean wrote:
> On Thu, 25 Jun 2020 at 19:37, Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> > On Thu, Jun 25, 2020 at 06:23:28PM +0300, Vladimir Oltean wrote:
> > > 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>
> > > ---
> > >  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 d229cb5d5f9e..da337c63e7ca 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);
> >
> > I'm not that happy with the persistence of felix using the legacy
> > method; I can bring a horse to water but can't make it drink comes
> > to mind.  I'm at the point where I don't care _if_ my phylink changes
> > break code that I've asked people to convert and they haven't yet,
> > and I'm planning to send the series that /may/ cause breakage in
> > that regard pretty soon, so the lynx PCS changes can move forward.
> >
> > I even tried to move felix forward by sending a patch for it, and I
> > just gave up with that approach based on your comment.
> >
> > Shrug.
> >
> > --
> > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
> 
> The callbacks clearly work the way they are, given the current
> structure of phylink, as proven by the users of this driver. Code that
> isn't there yet simply isn't there yet.
> 
> What you consider "useless churn" and what I consider "useless churn"
> are pretty different things.
> To me, patch 7/7 is the useless churn, that's why it's at the end.
> Patches 1-6 are structured in a way that is compatible with
> backporting to a stable 5.4 tree. Patch 7, not so much.
> 
> The fact that you have such an attitude and you make the effort to
> actually send an email complaining about me using state->speed in
> .mac_config(), when literally 3 patches later I'm moving this
> configuration where you want it to be, is pretty annoying.

I have asked you to update felix _prior_ to my patch update, because
I forsee the possibility for there to be breakage from pushing the
PCS support further forward.  In other words, I see there to be a
dependency between moving forward with PCS support and drivers that
still use the legacy method.

You don't see that, so you constantly bitch and moan.

Please stop being a problem.

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

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

[Vladimir Oltean]

On Thu, 25 Jun 2020 at 19:58, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Thu, Jun 25, 2020 at 07:48:23PM +0300, Vladimir Oltean wrote:
> > On Thu, 25 Jun 2020 at 19:37, Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> wrote:
> > >
> > > On Thu, Jun 25, 2020 at 06:23:28PM +0300, Vladimir Oltean wrote:
> > > > 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>
> > > > ---
> > > >  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 d229cb5d5f9e..da337c63e7ca 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);
> > >
> > > I'm not that happy with the persistence of felix using the legacy
> > > method; I can bring a horse to water but can't make it drink comes
> > > to mind.  I'm at the point where I don't care _if_ my phylink changes
> > > break code that I've asked people to convert and they haven't yet,
> > > and I'm planning to send the series that /may/ cause breakage in
> > > that regard pretty soon, so the lynx PCS changes can move forward.
> > >
> > > I even tried to move felix forward by sending a patch for it, and I
> > > just gave up with that approach based on your comment.
> > >
> > > Shrug.
> > >
> > > --
> > > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
> >
> > The callbacks clearly work the way they are, given the current
> > structure of phylink, as proven by the users of this driver. Code that
> > isn't there yet simply isn't there yet.
> >
> > What you consider "useless churn" and what I consider "useless churn"
> > are pretty different things.
> > To me, patch 7/7 is the useless churn, that's why it's at the end.
> > Patches 1-6 are structured in a way that is compatible with
> > backporting to a stable 5.4 tree. Patch 7, not so much.
> >
> > The fact that you have such an attitude and you make the effort to
> > actually send an email complaining about me using state->speed in
> > .mac_config(), when literally 3 patches later I'm moving this
> > configuration where you want it to be, is pretty annoying.
>
> I have asked you to update felix _prior_ to my patch update, because
> I forsee the possibility for there to be breakage from pushing the
> PCS support further forward.  In other words, I see there to be a
> dependency between moving forward with PCS support and drivers that
> still use the legacy method.
>
> You don't see that, so you constantly bitch and moan.
>

And that's what this patch series is, no?
Some cleanup needed to be done, and it needed to be done _before_ the
mac_link_up() conversion. And for things to go quicker, the cleanup
and the conversion are part of the same series.

> Please stop being a problem.
>

Come again, please?

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

[Russell King - ARM Linux admin]

On Thu, Jun 25, 2020 at 08:06:08PM +0300, Vladimir Oltean wrote:
> On Thu, 25 Jun 2020 at 19:58, Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> > On Thu, Jun 25, 2020 at 07:48:23PM +0300, Vladimir Oltean wrote:
> > > On Thu, 25 Jun 2020 at 19:37, Russell King - ARM Linux admin
> > > <linux@armlinux.org.uk> wrote:
> > > >
> > > > On Thu, Jun 25, 2020 at 06:23:28PM +0300, Vladimir Oltean wrote:
> > > > > 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>
> > > > > ---
> > > > >  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 d229cb5d5f9e..da337c63e7ca 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);
> > > >
> > > > I'm not that happy with the persistence of felix using the legacy
> > > > method; I can bring a horse to water but can't make it drink comes
> > > > to mind.  I'm at the point where I don't care _if_ my phylink changes
> > > > break code that I've asked people to convert and they haven't yet,
> > > > and I'm planning to send the series that /may/ cause breakage in
> > > > that regard pretty soon, so the lynx PCS changes can move forward.
> > > >
> > > > I even tried to move felix forward by sending a patch for it, and I
> > > > just gave up with that approach based on your comment.
> > > >
> > > > Shrug.
> > > >
> > > > --
> > > > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > > > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
> > >
> > > The callbacks clearly work the way they are, given the current
> > > structure of phylink, as proven by the users of this driver. Code that
> > > isn't there yet simply isn't there yet.
> > >
> > > What you consider "useless churn" and what I consider "useless churn"
> > > are pretty different things.
> > > To me, patch 7/7 is the useless churn, that's why it's at the end.
> > > Patches 1-6 are structured in a way that is compatible with
> > > backporting to a stable 5.4 tree. Patch 7, not so much.
> > >
> > > The fact that you have such an attitude and you make the effort to
> > > actually send an email complaining about me using state->speed in
> > > .mac_config(), when literally 3 patches later I'm moving this
> > > configuration where you want it to be, is pretty annoying.
> >
> > I have asked you to update felix _prior_ to my patch update, because
> > I forsee the possibility for there to be breakage from pushing the
> > PCS support further forward.  In other words, I see there to be a
> > dependency between moving forward with PCS support and drivers that
> > still use the legacy method.
> >
> > You don't see that, so you constantly bitch and moan.
> >
> 
> And that's what this patch series is, no?
> Some cleanup needed to be done, and it needed to be done _before_ the
> mac_link_up() conversion. And for things to go quicker, the cleanup
> and the conversion are part of the same series.

Right, which is what I find when I eventually get to patch 7, the patch
that you called above "useless churn."  I'm not going to review patch 7
tonight because of this fiasco.  To pissed off to do a decent job, so
you're just going to have to wait.

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

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

[Vladimir Oltean]

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

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.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 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 da337c63e7ca..4ec05090121c 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)
@@ -812,7 +803,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 c1220b488f9c..7d2673dab7d3 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
@@ -1412,7 +1376,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 net-next 5/7] net: dsa: felix: delete .phylink_mac_an_restart code

[Russell King - ARM Linux admin]

On Thu, Jun 25, 2020 at 06:23:29PM +0300, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> 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".

This is not "a different interpretation".

The LX2160A documentation for this PHY says:

  9             Restart Auto Negotiation
 Restart_Auto_N Self-clearing Read / Write command bit, set to '1' to
                restart an auto negotiation sequence. Set to '0'
		(Reset value) in normal operation mode. Note: Controls
		the Clause 37 1000Base-X Auto-negotiation.

It doesn't say anything about clearing anything for SGMII.

Also, the Cisco SGMII specification does not indicate that it is
possible to restart the "autonegotiation" - the PHY is the controlling
end of the SGMII link.  There is no clause in the SGMII specification
that indicates that changing the MAC's tx_config word to the PHY will
have any effect on the PHY once the data path has been established.

Finally, when a restart of negotiation is requested, and we have a PHY
attached in SGMII mode, we will talk to that PHY to cause a restart of
negotiation on the media side, which will implicitly cause the link
to drop and re-establish, causing the SGMII side to indicate link down
and subsequently re-establish according to the media side results.

So, please, lay off your phylink bashing in your commit messages.

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

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

[Vladimir Oltean]

Hi Russell,

On Thu, 25 Jun 2020 at 19:53, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Thu, Jun 25, 2020 at 06:23:29PM +0300, Vladimir Oltean wrote:
> > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> >
> > 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".
>
> This is not "a different interpretation".
>
> The LX2160A documentation for this PHY says:
>
>   9             Restart Auto Negotiation
>  Restart_Auto_N Self-clearing Read / Write command bit, set to '1' to
>                 restart an auto negotiation sequence. Set to '0'
>                 (Reset value) in normal operation mode. Note: Controls
>                 the Clause 37 1000Base-X Auto-negotiation.
>
> It doesn't say anything about clearing anything for SGMII.
>
> Also, the Cisco SGMII specification does not indicate that it is
> possible to restart the "autonegotiation" - the PHY is the controlling
> end of the SGMII link.  There is no clause in the SGMII specification
> that indicates that changing the MAC's tx_config word to the PHY will
> have any effect on the PHY once the data path has been established.
>
> Finally, when a restart of negotiation is requested, and we have a PHY
> attached in SGMII mode, we will talk to that PHY to cause a restart of
> negotiation on the media side, which will implicitly cause the link
> to drop and re-establish, causing the SGMII side to indicate link down
> and subsequently re-establish according to the media side results.
>
> So, please, lay off your phylink bashing in your commit messages.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Sorry, I was in a bit of a hurry when writing this commit message, so it is a
bit imprecise as you point out. How about:

net: dsa: felix: delete .phylink_mac_an_restart code

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 the
MAC PCS conveys no new information to the PHY PCS (beyond acknowledging the
received config word), does not permit the MAC PCS to trigger a restart of the
clause 37 auto-negotiation for any other SERDES protocols than 1000Base-X and
2500Base-X. For those, the control information exchange _is_ bidirectional
(local PCS specifies its duplex and flow control abilities). 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.

Thanks,
-Vladimir

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

[Russell King - ARM Linux admin]

On Fri, Jun 26, 2020 at 11:53:24AM +0300, Vladimir Oltean wrote:
> Hi Russell,
> 
> On Thu, 25 Jun 2020 at 19:53, Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> > On Thu, Jun 25, 2020 at 06:23:29PM +0300, Vladimir Oltean wrote:
> > > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > >
> > > 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".
> >
> > This is not "a different interpretation".
> >
> > The LX2160A documentation for this PHY says:
> >
> >   9             Restart Auto Negotiation
> >  Restart_Auto_N Self-clearing Read / Write command bit, set to '1' to
> >                 restart an auto negotiation sequence. Set to '0'
> >                 (Reset value) in normal operation mode. Note: Controls
> >                 the Clause 37 1000Base-X Auto-negotiation.
> >
> > It doesn't say anything about clearing anything for SGMII.
> >
> > Also, the Cisco SGMII specification does not indicate that it is
> > possible to restart the "autonegotiation" - the PHY is the controlling
> > end of the SGMII link.  There is no clause in the SGMII specification
> > that indicates that changing the MAC's tx_config word to the PHY will
> > have any effect on the PHY once the data path has been established.
> >
> > Finally, when a restart of negotiation is requested, and we have a PHY
> > attached in SGMII mode, we will talk to that PHY to cause a restart of
> > negotiation on the media side, which will implicitly cause the link
> > to drop and re-establish, causing the SGMII side to indicate link down
> > and subsequently re-establish according to the media side results.
> >
> > So, please, lay off your phylink bashing in your commit messages.
> >
> > --
> > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
> 
> Sorry, I was in a bit of a hurry when writing this commit message, so it is a
> bit imprecise as you point out. How about:

This is going over the top - most of this content should have been in a
discussion on the topic several months ago when this was first raised.
I accept that the SGMII link can be renegotiated from the MAC end, but
I still assert that it is pointless.

I've tried it here on the LX2160 with a Copper SFP plugged in that has a
Marvell 88E1111 PHY on-board, which I can monitor both the media and
SGMII side at the 88E1111 PHY.

Sure enough, if I set bit 9 in the LX2160 PCS, and monitor the "fiber"
page in the 88E1111, it reports that the link did drop.  However, that's
as far as the "renegotiation" goes - the link on the media side (as I've
said multiple times) does not renegotiate.

People expect the media side to renegotiate when they issue
"ethtool -r $IFACE" and this is what the phylink_mac_an_restart() method
is there for.  It caters for the case where is no other PHY as there is
with a copper link, the media terminating PHY is the MAC PCS PHY, as it
is for 1000BASE-X, and therefore the only place that media negotiation
can be restarted is the MAC PCS PHY.

Also as I've said multiple times, if you trigger a renegotiation at the
PHY end, then you end up with the media side renegotiating, and a fresh
exchange (in fact, two exchanges) of link information over the SGMII
link.  This is what the user expects.

If we don't have access to the PHY, then restarting the SGMII link
config exchange doesn't provide any benefit - the inaccessible PHY
_still_ doesn't renegotiate on the media side just because the SGMII
side wanted to re-exchange the config information.

Everything I've covered above with respect to the usefulness of
restarting the SGMII exchange are points that I've previously brought
up.

So, what practical use does triggering a fresh exchange of the SGMII
link configuration from the MAC side have?  The only use I can see is
if the SGMII MAC PCS is unreliable due to some hardware issue, doesn't
receive the link information correctly from the PHY, and needs manual
intervention from the MAC PCS side to "pursuade" it to get the correct
link information.  At that point we're into a severly unreliable
implementation that would likely be unsuitable for production systems.

I have no problem allowing other interface types to pass through _if_ it
can be shown that there is a proper practical use and benefit for doing
so, rather than just "the hardware lets us do this".

If we start allowing it for SGMII, we would be triggering a restart of
that configuration passing from both ends of the SGMII at a very similar
time - we would request the PHY to renegotiate, which triggers a fresh
exchange on the SGMII side when the media link fails, and we will also
be hitting the MAC PCS with its own "AN restart".  That doesn't sound
sane.  Depending on the timing, that could mean we end up with the MAC
PCS reporting _three_ different results: one from the local AN restart,
one from the link dropping, and one from the subsequent link
re-establishment.

So, I ask again, what practical use and benefit does restarting the
configuration exchange on a SGMII or USXGMII link have?  Give me a real
life use case where there's a problem with a link that this can solve.

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

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

[Vladimir Oltean]

On Fri, 26 Jun 2020 at 14:08, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>

[snip]

>
> So, I ask again, what practical use and benefit does restarting the
> configuration exchange on a SGMII or USXGMII link have?  Give me a real
> life use case where there's a problem with a link that this can solve.
>

You are pushing the discussion in an area that to me is pretty
insignificant, and where I did _not_ want to go. I said:

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

I was going to remove this code in the first place, it's just that you
didn't like the justification in the initial commit message. Fine. So
I asked you if this new commit message is OK. You said:

> This is going over the top

So let's cut this short: we agree about everything now, hardware
behavior and software behavior. Could you edit my commit message in a
way that you agree with, and paste it here so that I could include it
in v2?

Thanks,
-Vladimir

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

[Russell King - ARM Linux admin]

On Fri, Jun 26, 2020 at 02:19:42PM +0300, Vladimir Oltean wrote:
> On Fri, 26 Jun 2020 at 14:08, Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> 
> [snip]
> 
> >
> > So, I ask again, what practical use and benefit does restarting the
> > configuration exchange on a SGMII or USXGMII link have?  Give me a real
> > life use case where there's a problem with a link that this can solve.
> >
> 
> You are pushing the discussion in an area that to me is pretty
> insignificant, and where I did _not_ want to go. I said:
> 
> > 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.
> 
> I was going to remove this code in the first place, it's just that you
> didn't like the justification in the initial commit message. Fine. So
> I asked you if this new commit message is OK. You said:
> 
> > This is going over the top
> 
> So let's cut this short: we agree about everything now, hardware
> behavior and software behavior. Could you edit my commit message in a
> way that you agree with, and paste it here so that I could include it
> in v2?

No.  Too busy.

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

[PATCH net-next 6/7] net: dsa: felix: disable in-band AN in MII_BMCR without reset

[Vladimir Oltean]

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

The PCS doesn't need to be reset, we just need to clear BMCR_ANENABLE.
Since the only writable fields of MII_BMCR in this PCS are:

- BMCR_RESET
- BMCR_LOOPBACK
- BMCR_ANENABLE
- BMCR_PDOWN
- BMCR_ISOLATE
- BMCR_ANRESTART

it's safe to write zero to disable in-band AN.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/ocelot/felix_vsc9959.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 7d2673dab7d3..dba62c609efc 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -807,7 +807,7 @@ static void vsc9959_pcs_init_sgmii(struct phy_device *pcs,
 			if_mode |= ENETC_PCS_IF_MODE_DUPLEX_HALF;
 
 		phy_write(pcs, ENETC_PCS_IF_MODE, if_mode);
-		phy_write(pcs, MII_BMCR, BMCR_RESET);
+		phy_write(pcs, MII_BMCR, 0);
 	}
 }
 
@@ -844,7 +844,7 @@ static void vsc9959_pcs_init_2500basex(struct phy_device *pcs,
 		if_mode |= ENETC_PCS_IF_MODE_DUPLEX_HALF;
 
 	phy_write(pcs, ENETC_PCS_IF_MODE, if_mode);
-	phy_write(pcs, MII_BMCR, BMCR_RESET);
+	phy_write(pcs, MII_BMCR, 0);
 }
 
 static void vsc9959_pcs_init_usxgmii(struct phy_device *pcs,
-- 
2.25.1

[PATCH net-next 7/7] 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>
---
 drivers/net/dsa/ocelot/felix.c         |  76 ++++----
 drivers/net/dsa/ocelot/felix.h         |  10 +-
 drivers/net/dsa/ocelot/felix_vsc9959.c | 258 ++++++++++++++-----------
 3 files changed, 187 insertions(+), 157 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 4ec05090121c..0ac875ac1d65 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 dba62c609efc..f9ddac7f48ae 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -737,125 +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_write(pcs, MII_BMCR, BMCR_ANRESTART | 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_write(pcs, ENETC_PCS_IF_MODE, if_mode);
-		phy_write(pcs, MII_BMCR, 0);
-	}
-}
+	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_write(pcs, MII_BMCR, 0);
+	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_write(pcs, MII_BMCR, BMCR_ANRESTART | 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) |
@@ -865,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];
@@ -899,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_write(pcs, MII_BMCR, 0);
+}
+
+/* 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_write(pcs, MII_BMCR, 0);
+}
+
+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",
@@ -1375,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