From a979a0ab3687f701ef3a8223cbb56ce993a767fb Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Fri, 28 Jun 2019 00:46:35 +0300 Subject: [PATCH 1/3] net: dsa: sja1105: Don't check state->link in phylink_mac_config It has been pointed out that PHYLINK can call mac_config only to update the phy_interface_type and without knowing what the AN results are. Experimentally, when this was observed to happen, state->link was also unset, and therefore was used as a proxy to ignore this call. However it is also suggested that state->link is undefined for this callback and should not be relied upon. So let the previously-dead codepath for SPEED_UNKNOWN be called, and update the comment to make sure the MAC's behavior is sane. Cc: Russell King Signed-off-by: Vladimir Oltean Signed-off-by: David S. Miller --- drivers/net/dsa/sja1105/sja1105_main.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c index caebf76eaa3e..da1736093b06 100644 --- a/drivers/net/dsa/sja1105/sja1105_main.c +++ b/drivers/net/dsa/sja1105/sja1105_main.c @@ -715,7 +715,13 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port, switch (speed_mbps) { case SPEED_UNKNOWN: - /* No speed update requested */ + /* PHYLINK called sja1105_mac_config() to inform us about + * the state->interface, but AN has not completed and the + * speed is not yet valid. UM10944.pdf says that setting + * SJA1105_SPEED_AUTO at runtime disables the port, so that is + * ok for power consumption in case AN will never complete - + * otherwise PHYLINK should come back with a new update. + */ speed = SJA1105_SPEED_AUTO; break; case SPEED_10: @@ -766,9 +772,6 @@ static void sja1105_mac_config(struct dsa_switch *ds, int port, { struct sja1105_private *priv = ds->priv; - if (!state->link) - return; - sja1105_adjust_port_config(priv, port, state->speed); } From 39710229af9b6b69b858b36fe97d9239af0afe78 Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Fri, 28 Jun 2019 00:46:36 +0300 Subject: [PATCH 2/3] net: dsa: sja1105: Check for PHY mode mismatches with what PHYLINK reports PHYLINK being designed with PHYs in mind that can change MII protocol, for correct operation it is necessary to ensure that the PHY interface mode stays the same (otherwise clear the supported bit mask, as required). Because this is just a hypothetical situation for now, we don't bother to check whether we could actually support the new PHY interface mode. Actually we could modify the xMII table, reset the switch and send an updated static configuration, but adding that would just be dead code. Cc: Russell King Signed-off-by: Vladimir Oltean Signed-off-by: David S. Miller --- drivers/net/dsa/sja1105/sja1105_main.c | 44 ++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c index da1736093b06..b366b8e100f8 100644 --- a/drivers/net/dsa/sja1105/sja1105_main.c +++ b/drivers/net/dsa/sja1105/sja1105_main.c @@ -766,12 +766,46 @@ 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) +{ + struct sja1105_xmii_params_entry *mii; + sja1105_phy_interface_t phy_mode; + + mii = priv->static_config.tables[BLK_IDX_XMII_PARAMS].entries; + phy_mode = mii->xmii_mode[port]; + + switch (interface) { + case PHY_INTERFACE_MODE_MII: + return (phy_mode != XMII_MODE_MII); + case PHY_INTERFACE_MODE_RMII: + return (phy_mode != XMII_MODE_RMII); + case PHY_INTERFACE_MODE_RGMII: + case PHY_INTERFACE_MODE_RGMII_ID: + case PHY_INTERFACE_MODE_RGMII_RXID: + case PHY_INTERFACE_MODE_RGMII_TXID: + return (phy_mode != XMII_MODE_RGMII); + default: + return true; + } +} + static void sja1105_mac_config(struct dsa_switch *ds, int port, unsigned int link_an_mode, const struct phylink_link_state *state) { struct sja1105_private *priv = ds->priv; + if (sja1105_phy_mode_mismatch(priv, port, state->interface)) + return; + sja1105_adjust_port_config(priv, port, state->speed); } @@ -804,6 +838,16 @@ static void sja1105_phylink_validate(struct dsa_switch *ds, int port, 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)) { + bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS); + return; + } + /* The MAC does not support pause frames, and also doesn't * support half-duplex traffic modes. */ From 9f971573d96fc55efe9d48ff2ca160c9b95fbe37 Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Fri, 28 Jun 2019 00:46:37 +0300 Subject: [PATCH 3/3] net: dsa: sja1105: Mark in-band AN modes not supported for PHYLINK We need a better way to signal this, perhaps in phylink_validate, but for now just print this error message as guidance for other people looking at this driver's code while trying to rework PHYLINK. Cc: Russell King Signed-off-by: Vladimir Oltean Signed-off-by: David S. Miller --- drivers/net/dsa/sja1105/sja1105_main.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c index b366b8e100f8..32bf3a7cc3b6 100644 --- a/drivers/net/dsa/sja1105/sja1105_main.c +++ b/drivers/net/dsa/sja1105/sja1105_main.c @@ -806,6 +806,11 @@ static void sja1105_mac_config(struct dsa_switch *ds, int port, if (sja1105_phy_mode_mismatch(priv, port, state->interface)) return; + if (link_an_mode == MLO_AN_INBAND) { + dev_err(ds->dev, "In-band AN not supported!\n"); + return; + } + sja1105_adjust_port_config(priv, port, state->speed); }