From 341b672c99b9e69d7e8695979cb8a93f6ae58500 Mon Sep 17 00:00:00 2001 From: "Russell King (Oracle)" Date: Tue, 22 Feb 2022 10:15:57 +0000 Subject: [PATCH 1/5] net: dsa: b53: clean up if() condition to be more readable 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 Signed-off-by: Russell King (Oracle) Signed-off-by: David S. Miller --- 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); } From dda1c25751fa22ecbfa8eaa17efb296ff5017408 Mon Sep 17 00:00:00 2001 From: "Russell King (Oracle)" Date: Tue, 22 Feb 2022 10:16:02 +0000 Subject: [PATCH 2/5] net: dsa: b53: populate supported_interfaces and mac_capabilities 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) Signed-off-by: David S. Miller --- 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, From cc6b68f5f10e9fd2af083a70a0a007d2eeb989d8 Mon Sep 17 00:00:00 2001 From: "Russell King (Oracle)" Date: Tue, 22 Feb 2022 10:16:07 +0000 Subject: [PATCH 3/5] net: dsa: b53: drop use of phylink_helper_basex_speed() 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) Signed-off-by: David S. Miller --- 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); From 25179f8f34424208183341595ac8ea06d9900b9c Mon Sep 17 00:00:00 2001 From: "Russell King (Oracle)" Date: Tue, 22 Feb 2022 10:16:13 +0000 Subject: [PATCH 4/5] net: dsa: b53: switch to using phylink_generic_validate() 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) Signed-off-by: David S. Miller --- 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 }; From 81c1681cbb9f462eea5282942ea12faf40eaeef3 Mon Sep 17 00:00:00 2001 From: "Russell King (Oracle)" Date: Tue, 22 Feb 2022 10:16:18 +0000 Subject: [PATCH 5/5] net: dsa: b53: mark as non-legacy 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) Signed-off-by: David S. Miller --- 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,