Re: [PATCH RFC net-next 11/12] net: dsa: sja1105: convert to phylink_generic_validate()

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Wed, Nov 24, 2021 at 05:53:19PM +0000, Russell King (Oracle) wrote:
> Populate the supported interfaces and MAC capabilities for the SJA1105
> DSA switch and remove the old validate implementation to allow DSA to
> use phylink_generic_validate() for this switch driver.
> 
> This switch only supports a static model of configuration, so we
> restrict the interface modes to the configured setting, and pass the
> MAC capabilities. As it is unclear which interface modes support 1G
> speeds, we keep the setting of MAC_1000FD conditional on the configured
> interface mode.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

Please use this patch for sja1105. Thanks.

-- >8 --
From cc8a64e9aec8afeae5005dd34636fdccde22c1ac Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Wed, 24 Nov 2021 21:02:43 +0200
Subject: [PATCH] net: dsa: sja1105: convert to phylink_generic_validate()

Provide a ->phylink_get_caps() implementation in order to tell phylink
what are the PHY modes between which each port can change, and the MAC
capabilities so it can limit the advertisement and supported masks of
the PHY using the generic validation method.

Now that we populate phylink_config->supported_interfaces, it is
phylink's responsibility to not attempt a PHY mode change towards
something which we do not support, so we can delete the logic from
sja1105_phy_mode_mismatch() and move the essence of it into
sja1105_phylink_get_caps(), which happens much earlier. By removing the
PHY mode mismatch checks, we now effectively allow SERDES protocol
changes between SGMII and 2500base-X, while still denying PHY mode
changes between one xMII kind and another, which did not make sense.

This patch also fixes an inconsequential bug, which was that for ports
which support 2500base-X, we used to keep advertising the gigabit and
lower speeds. We should not have done this, because 2500base-X operates
only at 2500Mbps (and we do not support PAUSE frames in order for the
lower media speeds to work via rate adaptation). Nonetheless, the only
SJA1110 boards which use 2500base-X use it in a SERDES-to-SERDES fixed
link, so there isn't any PHY whose advertisement matters there.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_main.c | 105 +++++++++++++------------
 1 file changed, 53 insertions(+), 52 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index c343effe2e96..86883291e71d 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -1357,19 +1357,6 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port,
 	return sja1105_clocking_setup_port(priv, port);
 }
 
-/* The SJA1105 MAC programming model is through the static config (the xMII
- * Mode table cannot be dynamically reconfigured), and we have to program
- * that early (earlier than PHYLINK calls us, anyway).
- * So just error out in case the connected PHY attempts to change the initial
- * system interface MII protocol from what is defined in the DT, at least for
- * now.
- */
-static bool sja1105_phy_mode_mismatch(struct sja1105_private *priv, int port,
-				      phy_interface_t interface)
-{
-	return priv->phy_mode[port] != interface;
-}
-
 static void sja1105_mac_config(struct dsa_switch *ds, int port,
 			       unsigned int mode,
 			       const struct phylink_link_state *state)
@@ -1378,12 +1365,6 @@ static void sja1105_mac_config(struct dsa_switch *ds, int port,
 	struct sja1105_private *priv = ds->priv;
 	struct dw_xpcs *xpcs;
 
-	if (sja1105_phy_mode_mismatch(priv, port, state->interface)) {
-		dev_err(ds->dev, "Changing PHY mode to %s not supported!\n",
-			phy_modes(state->interface));
-		return;
-	}
-
 	xpcs = priv->xpcs[port];
 
 	if (xpcs)
@@ -1411,48 +1392,68 @@ static void sja1105_mac_link_up(struct dsa_switch *ds, int port,
 	sja1105_inhibit_tx(priv, BIT(port), false);
 }
 
-static void sja1105_phylink_validate(struct dsa_switch *ds, int port,
-				     unsigned long *supported,
-				     struct phylink_link_state *state)
+static void sja1105_phylink_get_caps(struct dsa_switch *ds, int port,
+				     struct phylink_config *config)
 {
-	/* Construct a new mask which exhaustively contains all link features
-	 * supported by the MAC, and then apply that (logical AND) to what will
-	 * be sent to the PHY for "marketing".
-	 */
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
 	struct sja1105_private *priv = ds->priv;
-	struct sja1105_xmii_params_entry *mii;
 
-	mii = priv->static_config.tables[BLK_IDX_XMII_PARAMS].entries;
-
-	/* include/linux/phylink.h says:
-	 *     When @state->interface is %PHY_INTERFACE_MODE_NA, phylink
-	 *     expects the MAC driver to return all supported link modes.
-	 */
-	if (state->interface != PHY_INTERFACE_MODE_NA &&
-	    sja1105_phy_mode_mismatch(priv, port, state->interface)) {
-		linkmode_zero(supported);
+	switch (priv->phy_mode[port]) {
+	case PHY_INTERFACE_MODE_MII:
+	case PHY_INTERFACE_MODE_REVMII:
+	case PHY_INTERFACE_MODE_RMII:
+	case PHY_INTERFACE_MODE_REVRMII:
+	case PHY_INTERFACE_MODE_RGMII:
+	case PHY_INTERFACE_MODE_RGMII_ID:
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+	case PHY_INTERFACE_MODE_INTERNAL:
+		/* Changing the PHY mode of xMII (parallel) ports is possible,
+		 * but requires a switch reset, and on top of that, will never
+		 * be needed in real life. So these ports support only the PHY
+		 * mode declared in the device tree.
+		 */
+		__set_bit(priv->phy_mode[port], config->supported_interfaces);
+		break;
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_2500BASEX:
+		/* Changing the PHY mode on SERDES ports is possible and makes
+		 * sense, because that is done through the XPCS. We allow
+		 * changes between SGMII and 2500base-X (it is unknown whether
+		 * 1000base-X is supported).
+		 */
+		if (priv->info->supports_sgmii[port]) {
+			__set_bit(PHY_INTERFACE_MODE_SGMII,
+				  config->supported_interfaces);
+		}
+		if (priv->info->supports_2500basex[port]) {
+			__set_bit(PHY_INTERFACE_MODE_2500BASEX,
+				  config->supported_interfaces);
+		}
+		break;
+	default:
 		return;
 	}
 
 	/* The MAC does not support pause frames, and also doesn't
 	 * support half-duplex traffic modes.
 	 */
-	phylink_set(mask, Autoneg);
-	phylink_set(mask, MII);
-	phylink_set(mask, 10baseT_Full);
-	phylink_set(mask, 100baseT_Full);
-	phylink_set(mask, 100baseT1_Full);
-	if (mii->xmii_mode[port] == XMII_MODE_RGMII ||
-	    mii->xmii_mode[port] == XMII_MODE_SGMII)
-		phylink_set(mask, 1000baseT_Full);
-	if (priv->info->supports_2500basex[port]) {
-		phylink_set(mask, 2500baseT_Full);
-		phylink_set(mask, 2500baseX_Full);
+	if (priv->info->supports_rgmii[port] ||
+	    priv->info->supports_sgmii[port]) {
+		/* Ports can be gigabit-capable either because they are xMII or
+		 * because they are SERDES ports.
+		 */
+		config->mac_capabilities = MAC_10FD | MAC_100FD | MAC_1000FD;
+		/* Some SERDES ports also support the 2500Mbps speed */
+		if (priv->info->supports_2500basex)
+			config->mac_capabilities |= MAC_2500FD;
+	} else {
+		/* As per the "Port compatibility matrix" chapter in
+		 * Documentation/networking/dsa/sja1105.rst, ports that don't
+		 * support xMII or SERDES go to the internal 100base-T1 or
+		 * 100base-TX ports, which aren't gigabit capable.
+		 */
+		config->mac_capabilities = MAC_10FD | MAC_100FD | MAC_1000FD;
 	}
-
-	linkmode_and(supported, supported, mask);
-	linkmode_and(state->advertising, state->advertising, mask);
 }
 
 static int
@@ -3189,7 +3190,7 @@ static const struct dsa_switch_ops sja1105_switch_ops = {
 	.set_ageing_time	= sja1105_set_ageing_time,
 	.port_change_mtu	= sja1105_change_mtu,
 	.port_max_mtu		= sja1105_get_max_mtu,
-	.phylink_validate	= sja1105_phylink_validate,
+	.phylink_get_caps	= sja1105_phylink_get_caps,
 	.phylink_mac_config	= sja1105_mac_config,
 	.phylink_mac_link_up	= sja1105_mac_link_up,
 	.phylink_mac_link_down	= sja1105_mac_link_down,
-- >8 --