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 09:08:33PM +0000, Russell King (Oracle) wrote:
> On Wed, Nov 24, 2021 at 07:53:40PM +0000, Vladimir Oltean wrote:
> > 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.
> 
> Your patch is combining two changes into one patch. Specifically, the
> there are two logical changes in your patch:
> 
> 1) changing the existing behaviour of the validate() function by
> allowing switching between PHY_INTERFACE_MODE_SGMII and
> PHY_INTERFACE_MODE_2500BASEX, which was not permitted before with the
> sja1105_phy_mode_mismatch() check.
> 
> 2) converting to supported_interfaces / mac_capabilities way of defining
> what is supported.
> 
> Combining the two changes makes the patch harder to review, and it
> becomes less obvious that it is actually correct. I would recommend
> changing the existing behaviour prior to the conversion, but ultimately
> that is your decision.
> 
> For more information please see the "Separate your changes" section in
> Documentation/process/submitting-patches.rst
> 
> Thanks.

-- >8 --
From febedc56cf0e269556e7483a70a3e6cb8d0d5cc3 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 (none for
now), and the MAC capabilities so it can limit the advertisement and
supported masks of the PHY.

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.

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 | 78 ++++++++------------------
 1 file changed, 24 insertions(+), 54 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index c343effe2e96..0db590daa3d9 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,37 @@ 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.
+	phy_interface_t phy_mode;
+
+	phy_mode = priv->phy_mode[port];
+
+	/* 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.
+	 * On the other hand, changing the PHY mode on SERDES ports is
+	 * possible and makes sense, because that is done through the
+	 * XPCS. We could allow changes between SGMII and 2500base-X
+	 * (it is unknown whether 1000base-X is supported), but that
+	 * is left for a future time.
 	 */
-	if (state->interface != PHY_INTERFACE_MODE_NA &&
-	    sja1105_phy_mode_mismatch(priv, port, state->interface)) {
-		linkmode_zero(supported);
-		return;
-	}
+	__set_bit(phy_mode, config->supported_interfaces);
 
 	/* 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 (phy_mode == PHY_INTERFACE_MODE_2500BASEX) {
+		config->mac_capabilities = MAC_2500FD;
+	} else if (phy_interface_mode_is_rgmii(phy_mode) ||
+		   phy_mode == PHY_INTERFACE_MODE_SGMII) {
+		config->mac_capabilities = MAC_10FD | MAC_100FD | MAC_1000FD;
+	} else {
+		config->mac_capabilities = MAC_10FD | MAC_100FD;
 	}
-
-	linkmode_and(supported, supported, mask);
-	linkmode_and(state->advertising, state->advertising, mask);
 }
 
 static int
@@ -3189,7 +3159,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 --