[PATCH net-next 0/5] net: dsa: b53: convert to phylink_generic_validate() and mark as non-legacy

[PATCH net-next 0/5] net: dsa: b53: convert to phylink_generic_validate() and mark as non-legacy

[Russell King (Oracle)]

Hi,

This series converts b53 to use phylink_generic_validate() and also
marks this driver as non-legacy.

Patch 1 cleans up an if() condition to be more readable before we
proceed with the conversion.

Patch 2 populates the supported_interfaces and mac_capabilities members
of phylink_config.

Patch 3 drops the use of phylink_helper_basex_speed() which is now not
necessary.

Patch 4 switches the driver to use phylink_generic_validate()

Patch 5 marks the driver as non-legacy.

 drivers/net/dsa/b53/b53_common.c | 68 +++++++++++++++++++++-------------------
 drivers/net/dsa/b53/b53_priv.h   |  8 ++---
 drivers/net/dsa/b53/b53_serdes.c | 19 +++++++----
 drivers/net/dsa/b53/b53_serdes.h |  5 ++-
 drivers/net/dsa/b53/b53_srab.c   | 35 ++++++++++++++++++++-
 5 files changed, 87 insertions(+), 48 deletions(-)

-- 
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 1/5] net: dsa: b53: clean up if() condition to be more readable

[Russell King (Oracle)]

I've stared at this if() statement for a while trying to work out if
it really does correspond with the comment above, and it does seem to.
However, let's make it more readable and phrase it in the same way as
the comment.

Also add a FIXME into the comment - we appear to deny Gigabit modes for
802.3z interface modes, but 802.3z interface modes only operate at
gigabit and above.

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>                            Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/b53/b53_common.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index a3b98992f180..7d62b0aeaae9 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1327,11 +1327,14 @@ void b53_phylink_validate(struct dsa_switch *ds, int port,
 
 	/* With the exclusion of 5325/5365, MII, Reverse MII and 802.3z, we
 	 * support Gigabit, including Half duplex.
+	 *
+	 * FIXME: this is weird - 802.3z is always Gigabit, but we exclude
+	 * it here. Why? This makes no sense.
 	 */
-	if (state->interface != PHY_INTERFACE_MODE_MII &&
-	    state->interface != PHY_INTERFACE_MODE_REVMII &&
-	    !phy_interface_mode_is_8023z(state->interface) &&
-	    !(is5325(dev) || is5365(dev))) {
+	if (!(state->interface == PHY_INTERFACE_MODE_MII ||
+	      state->interface == PHY_INTERFACE_MODE_REVMII ||
+	      phy_interface_mode_is_8023z(state->interface) ||
+	      is5325(dev) || is5365(dev))) {
 		phylink_set(mask, 1000baseT_Full);
 		phylink_set(mask, 1000baseT_Half);
 	}
-- 
2.30.2

[PATCH net-next 2/5] net: dsa: b53: populate supported_interfaces and mac_capabilities

[Russell King (Oracle)]

Populate the supported interfaces and MAC capabilities for the Broadcom
B53 DSA switches in preparation to using these for the generic
validation functionality.

The interface modes are derived from:
- b53_serdes_phylink_validate()
- SRAB mux configuration

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/b53/b53_common.c | 28 ++++++++++++++++++++++++++
 drivers/net/dsa/b53/b53_priv.h   |  2 ++
 drivers/net/dsa/b53/b53_serdes.c | 29 +++++++++++++++++++++++++++
 drivers/net/dsa/b53/b53_serdes.h |  2 ++
 drivers/net/dsa/b53/b53_srab.c   | 34 ++++++++++++++++++++++++++++++++
 5 files changed, 95 insertions(+)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 7d62b0aeaae9..211c0e499370 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1353,6 +1353,33 @@ void b53_phylink_validate(struct dsa_switch *ds, int port,
 }
 EXPORT_SYMBOL(b53_phylink_validate);
 
+static void b53_phylink_get_caps(struct dsa_switch *ds, int port,
+				 struct phylink_config *config)
+{
+	struct b53_device *dev = ds->priv;
+
+	/* Internal ports need GMII for PHYLIB */
+	__set_bit(PHY_INTERFACE_MODE_GMII, config->supported_interfaces);
+
+	/* These switches appear to support MII and RevMII too, but beyond
+	 * this, the code gives very few clues. FIXME: We probably need more
+	 * interface modes here.
+	 */
+	__set_bit(PHY_INTERFACE_MODE_MII, config->supported_interfaces);
+	__set_bit(PHY_INTERFACE_MODE_REVMII, config->supported_interfaces);
+
+	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
+		MAC_10 | MAC_100;
+
+	/* 5325/5365 are not capable of gigabit speeds, everything else is */
+	if (!(is5325(dev) || is5365(dev)))
+		config->mac_capabilities |= MAC_1000;
+
+	/* Get the implementation specific capabilities */
+	if (dev->ops->phylink_get_caps)
+		dev->ops->phylink_get_caps(dev, port, config);
+}
+
 int b53_phylink_mac_link_state(struct dsa_switch *ds, int port,
 			       struct phylink_link_state *state)
 {
@@ -2262,6 +2289,7 @@ static const struct dsa_switch_ops b53_switch_ops = {
 	.phy_read		= b53_phy_read16,
 	.phy_write		= b53_phy_write16,
 	.adjust_link		= b53_adjust_link,
+	.phylink_get_caps	= b53_phylink_get_caps,
 	.phylink_validate	= b53_phylink_validate,
 	.phylink_mac_link_state	= b53_phylink_mac_link_state,
 	.phylink_mac_config	= b53_phylink_mac_config,
diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index b41dc8ac2ca8..b9d1b4819c5f 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -46,6 +46,8 @@ struct b53_io_ops {
 	int (*phy_write16)(struct b53_device *dev, int addr, int reg, u16 value);
 	int (*irq_enable)(struct b53_device *dev, int port);
 	void (*irq_disable)(struct b53_device *dev, int port);
+	void (*phylink_get_caps)(struct b53_device *dev, int port,
+				 struct phylink_config *config);
 	u8 (*serdes_map_lane)(struct b53_device *dev, int port);
 	int (*serdes_link_state)(struct b53_device *dev, int port,
 				 struct phylink_link_state *state);
diff --git a/drivers/net/dsa/b53/b53_serdes.c b/drivers/net/dsa/b53/b53_serdes.c
index 5ae3d9783b68..e4cc6442c113 100644
--- a/drivers/net/dsa/b53/b53_serdes.c
+++ b/drivers/net/dsa/b53/b53_serdes.c
@@ -180,6 +180,35 @@ void b53_serdes_phylink_validate(struct b53_device *dev, int port,
 }
 EXPORT_SYMBOL(b53_serdes_phylink_validate);
 
+void b53_serdes_phylink_get_caps(struct b53_device *dev, int port,
+				 struct phylink_config *config)
+{
+	u8 lane = b53_serdes_map_lane(dev, port);
+
+	if (lane == B53_INVALID_LANE)
+		return;
+
+	switch (lane) {
+	case 0:
+		/* It appears lane 0 supports 2500base-X and 1000base-X */
+		__set_bit(PHY_INTERFACE_MODE_2500BASEX,
+			  config->supported_interfaces);
+		config->mac_capabilities |= MAC_2500FD;
+		fallthrough;
+	case 1:
+		/* It appears lane 1 only supports 1000base-X and SGMII */
+		__set_bit(PHY_INTERFACE_MODE_1000BASEX,
+			  config->supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_SGMII,
+			  config->supported_interfaces);
+		config->mac_capabilities |= MAC_1000FD;
+		break;
+	default:
+		break;
+	}
+}
+EXPORT_SYMBOL(b53_serdes_phylink_get_caps);
+
 int b53_serdes_init(struct b53_device *dev, int port)
 {
 	u8 lane = b53_serdes_map_lane(dev, port);
diff --git a/drivers/net/dsa/b53/b53_serdes.h b/drivers/net/dsa/b53/b53_serdes.h
index 55d280fe38e4..8fa24f7001aa 100644
--- a/drivers/net/dsa/b53/b53_serdes.h
+++ b/drivers/net/dsa/b53/b53_serdes.h
@@ -115,6 +115,8 @@ void b53_serdes_config(struct b53_device *dev, int port, unsigned int mode,
 void b53_serdes_an_restart(struct b53_device *dev, int port);
 void b53_serdes_link_set(struct b53_device *dev, int port, unsigned int mode,
 			 phy_interface_t interface, bool link_up);
+void b53_serdes_phylink_get_caps(struct b53_device *dev, int port,
+				 struct phylink_config *config);
 void b53_serdes_phylink_validate(struct b53_device *dev, int port,
 				unsigned long *supported,
 				struct phylink_link_state *state);
diff --git a/drivers/net/dsa/b53/b53_srab.c b/drivers/net/dsa/b53/b53_srab.c
index 4591bb1c05d2..7d72f3b293d3 100644
--- a/drivers/net/dsa/b53/b53_srab.c
+++ b/drivers/net/dsa/b53/b53_srab.c
@@ -443,6 +443,39 @@ static void b53_srab_irq_disable(struct b53_device *dev, int port)
 	}
 }
 
+static void b53_srab_phylink_get_caps(struct b53_device *dev, int port,
+				      struct phylink_config *config)
+{
+	struct b53_srab_priv *priv = dev->priv;
+	struct b53_srab_port_priv *p = &priv->port_intrs[port];
+
+	switch (p->mode) {
+	case PHY_INTERFACE_MODE_SGMII:
+#if IS_ENABLED(CONFIG_B53_SERDES)
+		/* If p->mode indicates SGMII mode, that essentially means we
+		 * are using a serdes. As the serdes for the capabilities.
+		 */
+		b53_serdes_phylink_get_caps(dev, port, config);
+#endif
+		break;
+
+	case PHY_INTERFACE_MODE_NA:
+		break;
+
+	case PHY_INTERFACE_MODE_RGMII:
+		/* If we support RGMII, support all RGMII modes, since
+		 * that dictates the PHY delay settings.
+		 */
+		phy_interface_set_rgmii(config->supported_interfaces);
+		break;
+
+	default:
+		/* Some other mode (e.g. MII, GMII etc) */
+		__set_bit(p->mode, config->supported_interfaces);
+		break;
+	}
+}
+
 static const struct b53_io_ops b53_srab_ops = {
 	.read8 = b53_srab_read8,
 	.read16 = b53_srab_read16,
@@ -456,6 +489,7 @@ static const struct b53_io_ops b53_srab_ops = {
 	.write64 = b53_srab_write64,
 	.irq_enable = b53_srab_irq_enable,
 	.irq_disable = b53_srab_irq_disable,
+	.phylink_get_caps = b53_srab_phylink_get_caps,
 #if IS_ENABLED(CONFIG_B53_SERDES)
 	.serdes_map_lane = b53_srab_serdes_map_lane,
 	.serdes_link_state = b53_serdes_link_state,
-- 
2.30.2

[PATCH net-next 3/5] net: dsa: b53: drop use of phylink_helper_basex_speed()

[Russell King (Oracle)]

Now that we have a better method to select SFP interface modes, we
no longer need to use phylink_helper_basex_speed() in a driver's
validation function.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/b53/b53_common.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 211c0e499370..a637e44bce0b 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1348,8 +1348,6 @@ void b53_phylink_validate(struct dsa_switch *ds, int port,
 
 	linkmode_and(supported, supported, mask);
 	linkmode_and(state->advertising, state->advertising, mask);
-
-	phylink_helper_basex_speed(state);
 }
 EXPORT_SYMBOL(b53_phylink_validate);
 
-- 
2.30.2

[PATCH net-next 4/5] net: dsa: b53: switch to using phylink_generic_validate()

[Russell King (Oracle)]

Switch the Broadcom b53 driver to using the phylink_generic_validate()
implementation by removing its own .phylink_validate method and
associated code.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/b53/b53_common.c | 57 ++++++++------------------------
 drivers/net/dsa/b53/b53_priv.h   |  6 ----
 drivers/net/dsa/b53/b53_serdes.c | 22 ------------
 drivers/net/dsa/b53/b53_serdes.h |  3 --
 drivers/net/dsa/b53/b53_srab.c   |  1 -
 5 files changed, 13 insertions(+), 76 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index a637e44bce0b..50a372dc32ae 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1309,48 +1309,6 @@ void b53_port_event(struct dsa_switch *ds, int port)
 }
 EXPORT_SYMBOL(b53_port_event);
 
-void b53_phylink_validate(struct dsa_switch *ds, int port,
-			  unsigned long *supported,
-			  struct phylink_link_state *state)
-{
-	struct b53_device *dev = ds->priv;
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
-
-	if (dev->ops->serdes_phylink_validate)
-		dev->ops->serdes_phylink_validate(dev, port, mask, state);
-
-	/* Allow all the expected bits */
-	phylink_set(mask, Autoneg);
-	phylink_set_port_modes(mask);
-	phylink_set(mask, Pause);
-	phylink_set(mask, Asym_Pause);
-
-	/* With the exclusion of 5325/5365, MII, Reverse MII and 802.3z, we
-	 * support Gigabit, including Half duplex.
-	 *
-	 * FIXME: this is weird - 802.3z is always Gigabit, but we exclude
-	 * it here. Why? This makes no sense.
-	 */
-	if (!(state->interface == PHY_INTERFACE_MODE_MII ||
-	      state->interface == PHY_INTERFACE_MODE_REVMII ||
-	      phy_interface_mode_is_8023z(state->interface) ||
-	      is5325(dev) || is5365(dev))) {
-		phylink_set(mask, 1000baseT_Full);
-		phylink_set(mask, 1000baseT_Half);
-	}
-
-	if (!phy_interface_mode_is_8023z(state->interface)) {
-		phylink_set(mask, 10baseT_Half);
-		phylink_set(mask, 10baseT_Full);
-		phylink_set(mask, 100baseT_Half);
-		phylink_set(mask, 100baseT_Full);
-	}
-
-	linkmode_and(supported, supported, mask);
-	linkmode_and(state->advertising, state->advertising, mask);
-}
-EXPORT_SYMBOL(b53_phylink_validate);
-
 static void b53_phylink_get_caps(struct dsa_switch *ds, int port,
 				 struct phylink_config *config)
 {
@@ -1362,6 +1320,13 @@ static void b53_phylink_get_caps(struct dsa_switch *ds, int port,
 	/* These switches appear to support MII and RevMII too, but beyond
 	 * this, the code gives very few clues. FIXME: We probably need more
 	 * interface modes here.
+	 *
+	 * According to b53_srab_mux_init(), ports 3..5 can support:
+	 *  SGMII, MII, GMII, RGMII or INTERNAL depending on the MUX setting.
+	 * However, the interface mode read from the MUX configuration is
+	 * not passed back to DSA, so phylink uses NA.
+	 * DT can specify RGMII for ports 0, 1.
+	 * For MDIO, port 8 can be RGMII_TXID.
 	 */
 	__set_bit(PHY_INTERFACE_MODE_MII, config->supported_interfaces);
 	__set_bit(PHY_INTERFACE_MODE_REVMII, config->supported_interfaces);
@@ -1369,7 +1334,12 @@ static void b53_phylink_get_caps(struct dsa_switch *ds, int port,
 	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
 		MAC_10 | MAC_100;
 
-	/* 5325/5365 are not capable of gigabit speeds, everything else is */
+	/* 5325/5365 are not capable of gigabit speeds, everything else is.
+	 * Note: the original code also exclulded Gigagbit for MII, RevMII
+	 * and 802.3z modes. MII and RevMII are not able to work above 100M,
+	 * so will be excluded by the generic validator implementation.
+	 * However, the exclusion of Gigabit for 802.3z just seems wrong.
+	 */
 	if (!(is5325(dev) || is5365(dev)))
 		config->mac_capabilities |= MAC_1000;
 
@@ -2288,7 +2258,6 @@ static const struct dsa_switch_ops b53_switch_ops = {
 	.phy_write		= b53_phy_write16,
 	.adjust_link		= b53_adjust_link,
 	.phylink_get_caps	= b53_phylink_get_caps,
-	.phylink_validate	= b53_phylink_validate,
 	.phylink_mac_link_state	= b53_phylink_mac_link_state,
 	.phylink_mac_config	= b53_phylink_mac_config,
 	.phylink_mac_an_restart	= b53_phylink_mac_an_restart,
diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index b9d1b4819c5f..a6b339fcb17e 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -58,9 +58,6 @@ struct b53_io_ops {
 	void (*serdes_link_set)(struct b53_device *dev, int port,
 				unsigned int mode, phy_interface_t interface,
 				bool link_up);
-	void (*serdes_phylink_validate)(struct b53_device *dev, int port,
-					unsigned long *supported,
-					struct phylink_link_state *state);
 };
 
 #define B53_INVALID_LANE	0xff
@@ -339,9 +336,6 @@ int b53_br_flags(struct dsa_switch *ds, int port,
 		 struct netlink_ext_ack *extack);
 int b53_setup_devlink_resources(struct dsa_switch *ds);
 void b53_port_event(struct dsa_switch *ds, int port);
-void b53_phylink_validate(struct dsa_switch *ds, int port,
-			  unsigned long *supported,
-			  struct phylink_link_state *state);
 int b53_phylink_mac_link_state(struct dsa_switch *ds, int port,
 			       struct phylink_link_state *state);
 void b53_phylink_mac_config(struct dsa_switch *ds, int port,
diff --git a/drivers/net/dsa/b53/b53_serdes.c b/drivers/net/dsa/b53/b53_serdes.c
index e4cc6442c113..555e5b372321 100644
--- a/drivers/net/dsa/b53/b53_serdes.c
+++ b/drivers/net/dsa/b53/b53_serdes.c
@@ -158,28 +158,6 @@ void b53_serdes_link_set(struct b53_device *dev, int port, unsigned int mode,
 }
 EXPORT_SYMBOL(b53_serdes_link_set);
 
-void b53_serdes_phylink_validate(struct b53_device *dev, int port,
-				 unsigned long *supported,
-				 struct phylink_link_state *state)
-{
-	u8 lane = b53_serdes_map_lane(dev, port);
-
-	if (lane == B53_INVALID_LANE)
-		return;
-
-	switch (lane) {
-	case 0:
-		phylink_set(supported, 2500baseX_Full);
-		fallthrough;
-	case 1:
-		phylink_set(supported, 1000baseX_Full);
-		break;
-	default:
-		break;
-	}
-}
-EXPORT_SYMBOL(b53_serdes_phylink_validate);
-
 void b53_serdes_phylink_get_caps(struct b53_device *dev, int port,
 				 struct phylink_config *config)
 {
diff --git a/drivers/net/dsa/b53/b53_serdes.h b/drivers/net/dsa/b53/b53_serdes.h
index 8fa24f7001aa..f47d5caa7557 100644
--- a/drivers/net/dsa/b53/b53_serdes.h
+++ b/drivers/net/dsa/b53/b53_serdes.h
@@ -117,9 +117,6 @@ void b53_serdes_link_set(struct b53_device *dev, int port, unsigned int mode,
 			 phy_interface_t interface, bool link_up);
 void b53_serdes_phylink_get_caps(struct b53_device *dev, int port,
 				 struct phylink_config *config);
-void b53_serdes_phylink_validate(struct b53_device *dev, int port,
-				unsigned long *supported,
-				struct phylink_link_state *state);
 #if IS_ENABLED(CONFIG_B53_SERDES)
 int b53_serdes_init(struct b53_device *dev, int port);
 #else
diff --git a/drivers/net/dsa/b53/b53_srab.c b/drivers/net/dsa/b53/b53_srab.c
index 7d72f3b293d3..c51b716657db 100644
--- a/drivers/net/dsa/b53/b53_srab.c
+++ b/drivers/net/dsa/b53/b53_srab.c
@@ -496,7 +496,6 @@ static const struct b53_io_ops b53_srab_ops = {
 	.serdes_config = b53_serdes_config,
 	.serdes_an_restart = b53_serdes_an_restart,
 	.serdes_link_set = b53_serdes_link_set,
-	.serdes_phylink_validate = b53_serdes_phylink_validate,
 #endif
 };
 
-- 
2.30.2

[PATCH net-next 5/5] net: dsa: b53: mark as non-legacy

[Russell King (Oracle)]

The B53 driver does not make use of the speed, duplex, pause or
advertisement in its phylink_mac_config() implementation, so it can be
marked as a non-legacy driver.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/b53/b53_common.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 50a372dc32ae..83bf30349c26 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1346,6 +1346,12 @@ static void b53_phylink_get_caps(struct dsa_switch *ds, int port,
 	/* Get the implementation specific capabilities */
 	if (dev->ops->phylink_get_caps)
 		dev->ops->phylink_get_caps(dev, port, config);
+
+	/* This driver does not make use of the speed, duplex, pause or the
+	 * advertisement in its mac_config, so it is safe to mark this driver
+	 * as non-legacy.
+	 */
+	config->legacy_pre_march2020 = false;
 }
 
 int b53_phylink_mac_link_state(struct dsa_switch *ds, int port,
-- 
2.30.2

Re: [PATCH net-next 0/5] net: dsa: b53: convert to phylink_generic_validate() and mark as non-legacy

[Florian Fainelli]

On 2/22/2022 2:14 AM, Russell King (Oracle) wrote:
> Hi,
> 
> This series converts b53 to use phylink_generic_validate() and also
> marks this driver as non-legacy.
> 
> Patch 1 cleans up an if() condition to be more readable before we
> proceed with the conversion.
> 
> Patch 2 populates the supported_interfaces and mac_capabilities members
> of phylink_config.
> 
> Patch 3 drops the use of phylink_helper_basex_speed() which is now not
> necessary.
> 
> Patch 4 switches the driver to use phylink_generic_validate()
> 
> Patch 5 marks the driver as non-legacy.

It has been applied anyway, but still:

Tested-by: Florian Fainelli <f.fainelli@gmail.com>

David, would you please allow reviewers 12 hours at least, 24 hours 
ideally just to account for time zone differences?

Thanks.
-- 
Florian

Re: [PATCH net-next 5/5] net: dsa: b53: mark as non-legacy

[Florian Fainelli]

Hi Russell,

On 2/22/22 02:16, Russell King (Oracle) wrote:
> The B53 driver does not make use of the speed, duplex, pause or
> advertisement in its phylink_mac_config() implementation, so it can be
> marked as a non-legacy driver.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>   drivers/net/dsa/b53/b53_common.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index 50a372dc32ae..83bf30349c26 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -1346,6 +1346,12 @@ static void b53_phylink_get_caps(struct dsa_switch *ds, int port,
>   	/* Get the implementation specific capabilities */
>   	if (dev->ops->phylink_get_caps)
>   		dev->ops->phylink_get_caps(dev, port, config);
> +
> +	/* This driver does not make use of the speed, duplex, pause or the
> +	 * advertisement in its mac_config, so it is safe to mark this driver
> +	 * as non-legacy.
> +	 */
> +	config->legacy_pre_march2020 = false;

This patch appears to cause a regression for me, I am not sure why I did 
not notice it back when I tested it but I suspect it had to do with me 
testing only with a copper module and not with a fiber module.

Now that I tested it again, the SFP port (port 5 in my set-up) link up 
interrupt does not fire up when setting config->legacy_pre_march2020 to 
false.

Here is a working log with phylink debugging enabled:

# udhcpc -i sfp
udhcpc: started, v1.35.0
[   49.479637] bgmac-enet 18024000.ethernet eth2: Link is Up - 
1Gbps/Full - flow control off
[   49.488139] Generic PHY fixed-0:02: PHY state change UP -> RUNNING
[   49.488256] b53-srab-switch 18036000.ethernet-switch sfp: configuring 
for inband/1000base-x link mode
[   49.504062] b53-srab-switch 18036000.ethernet-switch sfp: major 
config 1000base-x
[   49.511800] b53-srab-switch 18036000.ethernet-switch sfp: 
phylink_mac_config: mode=inband/1000base-x/Unknown/Unknown 
adv=0000000,00000201
[   49.527504] b53-srab-switch 18036000.ethernet-switch sfp: mac link down
[   49.535044] sfp sfp: SM: enter present:down:down event dev_up
[   49.541006] sfp sfp: tx disable 1 -> 0
[   49.544897] sfp sfp: SM: exit present:up:wait
[   49.549509] IPv6: ADDRCONF(NETDEV_CHANGE): eth2: link becomes ready
udhcpc: broadcasting discover
[   49.595185] sfp sfp: SM: enter present:up:wait event timeout
[   49.601064] sfp sfp: SM: exit present:up:link_up
[   52.388917] b53-srab-switch 18036000.ethernet-switch sfp: mac link up
[   52.396513] b53-srab-switch 18036000.ethernet-switch sfp: Link is Up 
- 1Gbps/Full - flow control rx/tx
[   52.406145] IPv6: ADDRCONF(NETDEV_CHANGE): sfp: link becomes ready
udhcpc: broadcasting discover
udhcpc: broadcasting select for 192.168.3.156, server 192.168.3.1
udhcpc: lease of 192.168.3.156 obtained from 192.168.3.1, lease time 600
deleting routers
adding dns 192.168.1.1

and one that is not working with phylink debugging enabled:

# udhcpc -i sfp
udhcpc: started, v1.35.0
[   27.863529] bgmac-enet 18024000.ethernet eth2: Link is Up - 
1Gbps/Full - flow control off
[   27.872021] Generic PHY fixed-0:02: PHY state change UP -> RUNNING
[   27.872120] b53-srab-switch 18036000.ethernet-switch sfp: configuring 
for inband/1000base-x link mode
[   27.887952] b53-srab-switch 18036000.ethernet-switch sfp: major 
config 1000base-x
[   27.895689] b53-srab-switch 18036000.ethernet-switch sfp: 
phylink_mac_config: mode=inband/1000base-x/Unknown/Unknown 
adv=0000000,00000201
[   27.895802] b53-srab-switch 18036000.ethernet-switch sfp: mac link down
[   27.911945] sfp sfp: SM: enter present:down:down event dev_up
[   27.923947] sfp sfp: tx disable 1 -> 0
[   27.927835] sfp sfp: SM: exit present:up:wait
[   27.932442] IPv6: ADDRCONF(NETDEV_CHANGE): eth2: link becomes ready
udhcpc: broadcasting discover
[   27.978181] sfp sfp: SM: enter present:up:wait event timeout
[   27.984056] sfp sfp: SM: exit present:up:link_up
[   30.686440] b53-srab-switch 18036000.ethernet-switch sfp: mac link up
udhcpc: broadcasting discover
udhcpc: broadcasting discover

The mac side appears to be UP but not no carrier is set to the sfp 
network device. Do you have any idea why that would happen?
-- 
Florian

Re: [PATCH net-next 5/5] net: dsa: b53: mark as non-legacy

[Russell King (Oracle)]

On Thu, Apr 21, 2022 at 03:31:26PM -0700, Florian Fainelli wrote:
> Hi Russell,
> 
> On 2/22/22 02:16, Russell King (Oracle) wrote:
> > The B53 driver does not make use of the speed, duplex, pause or
> > advertisement in its phylink_mac_config() implementation, so it can be
> > marked as a non-legacy driver.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >   drivers/net/dsa/b53/b53_common.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> > index 50a372dc32ae..83bf30349c26 100644
> > --- a/drivers/net/dsa/b53/b53_common.c
> > +++ b/drivers/net/dsa/b53/b53_common.c
> > @@ -1346,6 +1346,12 @@ static void b53_phylink_get_caps(struct dsa_switch *ds, int port,
> >   	/* Get the implementation specific capabilities */
> >   	if (dev->ops->phylink_get_caps)
> >   		dev->ops->phylink_get_caps(dev, port, config);
> > +
> > +	/* This driver does not make use of the speed, duplex, pause or the
> > +	 * advertisement in its mac_config, so it is safe to mark this driver
> > +	 * as non-legacy.
> > +	 */
> > +	config->legacy_pre_march2020 = false;
> 
> This patch appears to cause a regression for me, I am not sure why I did not
> notice it back when I tested it but I suspect it had to do with me testing
> only with a copper module and not with a fiber module.
> 
> Now that I tested it again, the SFP port (port 5 in my set-up) link up
> interrupt does not fire up when setting config->legacy_pre_march2020 to
> false.
> 
> Here is a working log with phylink debugging enabled:
> 
> # udhcpc -i sfp
> udhcpc: started, v1.35.0
> [   49.479637] bgmac-enet 18024000.ethernet eth2: Link is Up - 1Gbps/Full -
> flow control off
> [   49.488139] Generic PHY fixed-0:02: PHY state change UP -> RUNNING
> [   49.488256] b53-srab-switch 18036000.ethernet-switch sfp: configuring for
> inband/1000base-x link mode
> [   49.504062] b53-srab-switch 18036000.ethernet-switch sfp: major config
> 1000base-x
> [   49.511800] b53-srab-switch 18036000.ethernet-switch sfp:
> phylink_mac_config: mode=inband/1000base-x/Unknown/Unknown
> adv=0000000,00000201
> [   49.527504] b53-srab-switch 18036000.ethernet-switch sfp: mac link down
> [   49.535044] sfp sfp: SM: enter present:down:down event dev_up
> [   49.541006] sfp sfp: tx disable 1 -> 0
> [   49.544897] sfp sfp: SM: exit present:up:wait
> [   49.549509] IPv6: ADDRCONF(NETDEV_CHANGE): eth2: link becomes ready
> udhcpc: broadcasting discover
> [   49.595185] sfp sfp: SM: enter present:up:wait event timeout
> [   49.601064] sfp sfp: SM: exit present:up:link_up
> [   52.388917] b53-srab-switch 18036000.ethernet-switch sfp: mac link up
> [   52.396513] b53-srab-switch 18036000.ethernet-switch sfp: Link is Up -
> 1Gbps/Full - flow control rx/tx
> [   52.406145] IPv6: ADDRCONF(NETDEV_CHANGE): sfp: link becomes ready
> udhcpc: broadcasting discover
> udhcpc: broadcasting select for 192.168.3.156, server 192.168.3.1
> udhcpc: lease of 192.168.3.156 obtained from 192.168.3.1, lease time 600
> deleting routers
> adding dns 192.168.1.1
> 
> and one that is not working with phylink debugging enabled:
> 
> # udhcpc -i sfp
> udhcpc: started, v1.35.0
> [   27.863529] bgmac-enet 18024000.ethernet eth2: Link is Up - 1Gbps/Full -
> flow control off
> [   27.872021] Generic PHY fixed-0:02: PHY state change UP -> RUNNING
> [   27.872120] b53-srab-switch 18036000.ethernet-switch sfp: configuring for
> inband/1000base-x link mode
> [   27.887952] b53-srab-switch 18036000.ethernet-switch sfp: major config
> 1000base-x
> [   27.895689] b53-srab-switch 18036000.ethernet-switch sfp:
> phylink_mac_config: mode=inband/1000base-x/Unknown/Unknown
> adv=0000000,00000201
> [   27.895802] b53-srab-switch 18036000.ethernet-switch sfp: mac link down
> [   27.911945] sfp sfp: SM: enter present:down:down event dev_up
> [   27.923947] sfp sfp: tx disable 1 -> 0
> [   27.927835] sfp sfp: SM: exit present:up:wait
> [   27.932442] IPv6: ADDRCONF(NETDEV_CHANGE): eth2: link becomes ready
> udhcpc: broadcasting discover
> [   27.978181] sfp sfp: SM: enter present:up:wait event timeout
> [   27.984056] sfp sfp: SM: exit present:up:link_up
> [   30.686440] b53-srab-switch 18036000.ethernet-switch sfp: mac link up
> udhcpc: broadcasting discover
> udhcpc: broadcasting discover
> 
> The mac side appears to be UP but not no carrier is set to the sfp network
> device. Do you have any idea why that would happen?

Oh, it's because setting that flag means we're wanting the PCS methods
rather than the legacy MAC methods for an_restart and getting the PCS
link state - so the patch in question was submitted too early (it
should have been _after_ the conversion to PCS.)

If we get the patch reverted in net-next, and then convert b53 to use
PCS support, we'll then be putting the patch back, so I wonder if it
would just make sense to apply the PCS conversion patch, possibly
adding a comment in the commit message pointing out that this fixes
the b53 legacy_pre_march2020 patch. Thoughts?

-- 
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/5] net: dsa: b53: mark as non-legacy

[Florian Fainelli]

On 4/22/22 00:39, Russell King (Oracle) wrote:
> On Thu, Apr 21, 2022 at 03:31:26PM -0700, Florian Fainelli wrote:
>> Hi Russell,
>>
>> On 2/22/22 02:16, Russell King (Oracle) wrote:
>>> The B53 driver does not make use of the speed, duplex, pause or
>>> advertisement in its phylink_mac_config() implementation, so it can be
>>> marked as a non-legacy driver.
>>>
>>> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
>>> ---
>>>    drivers/net/dsa/b53/b53_common.c | 6 ++++++
>>>    1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
>>> index 50a372dc32ae..83bf30349c26 100644
>>> --- a/drivers/net/dsa/b53/b53_common.c
>>> +++ b/drivers/net/dsa/b53/b53_common.c
>>> @@ -1346,6 +1346,12 @@ static void b53_phylink_get_caps(struct dsa_switch *ds, int port,
>>>    	/* Get the implementation specific capabilities */
>>>    	if (dev->ops->phylink_get_caps)
>>>    		dev->ops->phylink_get_caps(dev, port, config);
>>> +
>>> +	/* This driver does not make use of the speed, duplex, pause or the
>>> +	 * advertisement in its mac_config, so it is safe to mark this driver
>>> +	 * as non-legacy.
>>> +	 */
>>> +	config->legacy_pre_march2020 = false;
>>
>> This patch appears to cause a regression for me, I am not sure why I did not
>> notice it back when I tested it but I suspect it had to do with me testing
>> only with a copper module and not with a fiber module.
>>
>> Now that I tested it again, the SFP port (port 5 in my set-up) link up
>> interrupt does not fire up when setting config->legacy_pre_march2020 to
>> false.
>>
>> Here is a working log with phylink debugging enabled:
>>
>> # udhcpc -i sfp
>> udhcpc: started, v1.35.0
>> [   49.479637] bgmac-enet 18024000.ethernet eth2: Link is Up - 1Gbps/Full -
>> flow control off
>> [   49.488139] Generic PHY fixed-0:02: PHY state change UP -> RUNNING
>> [   49.488256] b53-srab-switch 18036000.ethernet-switch sfp: configuring for
>> inband/1000base-x link mode
>> [   49.504062] b53-srab-switch 18036000.ethernet-switch sfp: major config
>> 1000base-x
>> [   49.511800] b53-srab-switch 18036000.ethernet-switch sfp:
>> phylink_mac_config: mode=inband/1000base-x/Unknown/Unknown
>> adv=0000000,00000201
>> [   49.527504] b53-srab-switch 18036000.ethernet-switch sfp: mac link down
>> [   49.535044] sfp sfp: SM: enter present:down:down event dev_up
>> [   49.541006] sfp sfp: tx disable 1 -> 0
>> [   49.544897] sfp sfp: SM: exit present:up:wait
>> [   49.549509] IPv6: ADDRCONF(NETDEV_CHANGE): eth2: link becomes ready
>> udhcpc: broadcasting discover
>> [   49.595185] sfp sfp: SM: enter present:up:wait event timeout
>> [   49.601064] sfp sfp: SM: exit present:up:link_up
>> [   52.388917] b53-srab-switch 18036000.ethernet-switch sfp: mac link up
>> [   52.396513] b53-srab-switch 18036000.ethernet-switch sfp: Link is Up -
>> 1Gbps/Full - flow control rx/tx
>> [   52.406145] IPv6: ADDRCONF(NETDEV_CHANGE): sfp: link becomes ready
>> udhcpc: broadcasting discover
>> udhcpc: broadcasting select for 192.168.3.156, server 192.168.3.1
>> udhcpc: lease of 192.168.3.156 obtained from 192.168.3.1, lease time 600
>> deleting routers
>> adding dns 192.168.1.1
>>
>> and one that is not working with phylink debugging enabled:
>>
>> # udhcpc -i sfp
>> udhcpc: started, v1.35.0
>> [   27.863529] bgmac-enet 18024000.ethernet eth2: Link is Up - 1Gbps/Full -
>> flow control off
>> [   27.872021] Generic PHY fixed-0:02: PHY state change UP -> RUNNING
>> [   27.872120] b53-srab-switch 18036000.ethernet-switch sfp: configuring for
>> inband/1000base-x link mode
>> [   27.887952] b53-srab-switch 18036000.ethernet-switch sfp: major config
>> 1000base-x
>> [   27.895689] b53-srab-switch 18036000.ethernet-switch sfp:
>> phylink_mac_config: mode=inband/1000base-x/Unknown/Unknown
>> adv=0000000,00000201
>> [   27.895802] b53-srab-switch 18036000.ethernet-switch sfp: mac link down
>> [   27.911945] sfp sfp: SM: enter present:down:down event dev_up
>> [   27.923947] sfp sfp: tx disable 1 -> 0
>> [   27.927835] sfp sfp: SM: exit present:up:wait
>> [   27.932442] IPv6: ADDRCONF(NETDEV_CHANGE): eth2: link becomes ready
>> udhcpc: broadcasting discover
>> [   27.978181] sfp sfp: SM: enter present:up:wait event timeout
>> [   27.984056] sfp sfp: SM: exit present:up:link_up
>> [   30.686440] b53-srab-switch 18036000.ethernet-switch sfp: mac link up
>> udhcpc: broadcasting discover
>> udhcpc: broadcasting discover
>>
>> The mac side appears to be UP but not no carrier is set to the sfp network
>> device. Do you have any idea why that would happen?
> 
> Oh, it's because setting that flag means we're wanting the PCS methods
> rather than the legacy MAC methods for an_restart and getting the PCS
> link state - so the patch in question was submitted too early (it
> should have been _after_ the conversion to PCS.)

Meh, sorry I was really slow on this and did not even connect the dots. 
Indeed that is what it is.

> 
> If we get the patch reverted in net-next, and then convert b53 to use
> PCS support, we'll then be putting the patch back, so I wonder if it
> would just make sense to apply the PCS conversion patch, possibly
> adding a comment in the commit message pointing out that this fixes
> the b53 legacy_pre_march2020 patch. Thoughts?

I just responded to your other patch "net: dsa: b53: convert to 
phylink_pcs", so if we target that one for 'net' I think we should be 
good to go.

Thanks!
-- 
Florian