* [PATCH v3 1/2] net: macb: Allow SGMII only if we are a GEM in mac_validate
@ 2021-10-12 19:46 Sean Anderson
2021-10-12 19:46 ` [PATCH v3 2/2] net: macb: Clean up macb_validate Sean Anderson
0 siblings, 1 reply; 4+ messages in thread
From: Sean Anderson @ 2021-10-12 19:46 UTC (permalink / raw)
To: netdev, David S . Miller, Jakub Kicinski
Cc: Claudiu Beznea, Nicolas Ferre, Antoine Tenart, Russell King,
Sean Anderson
This aligns mac_validate with mac_config. In mac_config, SGMII is only
enabled if macb_is_gem. Validate should care if the mac is a gem as
well.
Fixes: 7897b071ac3b ("net: macb: convert to phylink")
Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---
Changes in v3:
- Order bugfix patch first
Changes in v2:
- New
drivers/net/ethernet/cadence/macb_main.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 683f14665c2c..cb0f86544955 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -528,6 +528,7 @@ static void macb_validate(struct phylink_config *config,
if (!macb_is_gem(bp) &&
(state->interface == PHY_INTERFACE_MODE_GMII ||
+ state->interface == PHY_INTERFACE_MODE_SGMII ||
phy_interface_mode_is_rgmii(state->interface))) {
bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
return;
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH v3 2/2] net: macb: Clean up macb_validate 2021-10-12 19:46 [PATCH v3 1/2] net: macb: Allow SGMII only if we are a GEM in mac_validate Sean Anderson @ 2021-10-12 19:46 ` Sean Anderson 2021-10-13 8:29 ` Antoine Tenart 0 siblings, 1 reply; 4+ messages in thread From: Sean Anderson @ 2021-10-12 19:46 UTC (permalink / raw) To: netdev, David S . Miller, Jakub Kicinski Cc: Claudiu Beznea, Nicolas Ferre, Antoine Tenart, Russell King, Sean Anderson As the number of interfaces grows, the number of if statements grows ever more unweildy. Clean everything up a bit by using a switch statement. This reduces the number of if statements, and the number of times the same condition is checked. No functional change intended. Signed-off-by: Sean Anderson <sean.anderson@seco.com> --- This patch was originally submitted as [1]. There is another approach which could be used here. We could do something like macb_set_mode(bp, mask, iface) { switch (iface) { case 10GBASER: if (...) phylink_set_10g_modes(mask); else return -EINVAL; break; case GMII: /* etc etc */ } return 0; } macb_validate(...) { if (state->interface == PHY_INTERFACE_MODE_NA) { macb_set_mode(bp, mask, PHY_INTERFACE_MODE_10GBASER); macb_set_mode(bp, mask, PHY_INTERFACE_MODE_GMII); macb_set_mode(bp, mask, PHY_INTERFACE_MODE_MII); } else if (macb_set_mode(bp, mask, state->interface)) { bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS); return; } /* etc etc */ } which has the advantage of much cleaner logic in the switch statement. [1] https://lore.kernel.org/netdev/20211004191527.1610759-9-sean.anderson@seco.com/ Changes in v3: - Remove labels/gotos in favor of explicit zeroing. Hopefully this better illustrates the "linear" flow of the logic. - Add comment with overview of the flow of the logic. Changes in v2: - Fix polarity of `one` being inverted - Only set gigabit modes for NA if macb_is_gem() drivers/net/ethernet/cadence/macb_main.c | 105 ++++++++++++----------- 1 file changed, 55 insertions(+), 50 deletions(-) diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index cb0f86544955..9b2173c37edb 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -510,33 +510,65 @@ static void macb_validate(struct phylink_config *config, unsigned long *supported, struct phylink_link_state *state) { + bool one = state->interface != PHY_INTERFACE_MODE_NA; struct net_device *ndev = to_net_dev(config->dev); __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, }; struct macb *bp = netdev_priv(ndev); - /* We only support MII, RMII, GMII, RGMII & SGMII. */ - if (state->interface != PHY_INTERFACE_MODE_NA && - state->interface != PHY_INTERFACE_MODE_MII && - state->interface != PHY_INTERFACE_MODE_RMII && - state->interface != PHY_INTERFACE_MODE_GMII && - state->interface != PHY_INTERFACE_MODE_SGMII && - state->interface != PHY_INTERFACE_MODE_10GBASER && - !phy_interface_mode_is_rgmii(state->interface)) { - bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS); - return; - } - - if (!macb_is_gem(bp) && - (state->interface == PHY_INTERFACE_MODE_GMII || - state->interface == PHY_INTERFACE_MODE_SGMII || - phy_interface_mode_is_rgmii(state->interface))) { - bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS); - return; - } - - if (state->interface == PHY_INTERFACE_MODE_10GBASER && - !(bp->caps & MACB_CAPS_HIGH_SPEED && - bp->caps & MACB_CAPS_PCS)) { + /* There are three major types of interfaces we support: + * - (R)MII supporting 10/100 Mbit/s + * - GMII, RGMII, and SGMII supporting 10/100/1000 Mbit/s + * - 10GBASER supporting 10 Gbit/s only + * Because GMII and MII both support 10/100, GMII falls through to MII. + * + * If we can't support an interface mode, we just clear the supported + * mask and return. The major complication is that if we get + * PHY_INTERFACE_MODE_NA, we must return all modes we support. Because + * of this, NA starts at the top of the switch and falls all the way to + * the bottom, and doesn't return early if we don't support a + * particular mode. + */ + switch (state->interface) { + case PHY_INTERFACE_MODE_NA: + case PHY_INTERFACE_MODE_10GBASER: + if (bp->caps & MACB_CAPS_HIGH_SPEED && + bp->caps & MACB_CAPS_PCS && + bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) { + phylink_set_10g_modes(mask); + phylink_set(mask, 10000baseKR_Full); + } else if (one) { + bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS); + return; + } + if (one) + break; + fallthrough; + case PHY_INTERFACE_MODE_GMII: + 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_SGMII: + if (macb_is_gem(bp)) { + if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) { + phylink_set(mask, 1000baseT_Full); + phylink_set(mask, 1000baseX_Full); + if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF)) + phylink_set(mask, 1000baseT_Half); + } + } else if (one) { + bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS); + return; + } + fallthrough; + case PHY_INTERFACE_MODE_MII: + case PHY_INTERFACE_MODE_RMII: + phylink_set(mask, 10baseT_Half); + phylink_set(mask, 10baseT_Full); + phylink_set(mask, 100baseT_Half); + phylink_set(mask, 100baseT_Full); + break; + default: bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS); return; } @@ -544,33 +576,6 @@ static void macb_validate(struct phylink_config *config, phylink_set_port_modes(mask); phylink_set(mask, Autoneg); phylink_set(mask, Asym_Pause); - - if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE && - (state->interface == PHY_INTERFACE_MODE_NA || - state->interface == PHY_INTERFACE_MODE_10GBASER)) { - phylink_set_10g_modes(mask); - phylink_set(mask, 10000baseKR_Full); - if (state->interface != PHY_INTERFACE_MODE_NA) - goto out; - } - - phylink_set(mask, 10baseT_Half); - phylink_set(mask, 10baseT_Full); - phylink_set(mask, 100baseT_Half); - phylink_set(mask, 100baseT_Full); - - if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE && - (state->interface == PHY_INTERFACE_MODE_NA || - state->interface == PHY_INTERFACE_MODE_GMII || - state->interface == PHY_INTERFACE_MODE_SGMII || - phy_interface_mode_is_rgmii(state->interface))) { - phylink_set(mask, 1000baseT_Full); - phylink_set(mask, 1000baseX_Full); - - if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF)) - phylink_set(mask, 1000baseT_Half); - } -out: bitmap_and(supported, supported, mask, __ETHTOOL_LINK_MODE_MASK_NBITS); bitmap_and(state->advertising, state->advertising, mask, __ETHTOOL_LINK_MODE_MASK_NBITS); -- 2.25.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3 2/2] net: macb: Clean up macb_validate 2021-10-12 19:46 ` [PATCH v3 2/2] net: macb: Clean up macb_validate Sean Anderson @ 2021-10-13 8:29 ` Antoine Tenart 2021-10-14 16:01 ` Sean Anderson 0 siblings, 1 reply; 4+ messages in thread From: Antoine Tenart @ 2021-10-13 8:29 UTC (permalink / raw) To: David S . Miller, Jakub Kicinski, Sean Anderson, netdev Cc: Claudiu Beznea, Nicolas Ferre, Russell King, Sean Anderson Quoting Sean Anderson (2021-10-12 21:46:44) > + /* There are three major types of interfaces we support: > + * - (R)MII supporting 10/100 Mbit/s > + * - GMII, RGMII, and SGMII supporting 10/100/1000 Mbit/s > + * - 10GBASER supporting 10 Gbit/s only > + * Because GMII and MII both support 10/100, GMII falls through to MII. > + * > + * If we can't support an interface mode, we just clear the supported > + * mask and return. The major complication is that if we get > + * PHY_INTERFACE_MODE_NA, we must return all modes we support. Because > + * of this, NA starts at the top of the switch and falls all the way to > + * the bottom, and doesn't return early if we don't support a > + * particular mode. > + */ > + switch (state->interface) { > + case PHY_INTERFACE_MODE_NA: > + case PHY_INTERFACE_MODE_10GBASER: > + if (bp->caps & MACB_CAPS_HIGH_SPEED && > + bp->caps & MACB_CAPS_PCS && > + bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) { > + phylink_set_10g_modes(mask); > + phylink_set(mask, 10000baseKR_Full); > + } else if (one) { > + bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS); > + return; > + } > + if (one) > + break; This can go in the first if block. > + fallthrough; > + case PHY_INTERFACE_MODE_GMII: > + 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_SGMII: > + if (macb_is_gem(bp)) { > + if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) { Is not having MACB_CAPS_GIGABIT_MODE_AVAILABLE acceptable here, or should the two above checks be merged? > + phylink_set(mask, 1000baseT_Full); > + phylink_set(mask, 1000baseX_Full); > + if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF)) > + phylink_set(mask, 1000baseT_Half); > + } > + } else if (one) { > + bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS); > + return; > + } > + fallthrough; > + case PHY_INTERFACE_MODE_MII: > + case PHY_INTERFACE_MODE_RMII: > + phylink_set(mask, 10baseT_Half); > + phylink_set(mask, 10baseT_Full); > + phylink_set(mask, 100baseT_Half); > + phylink_set(mask, 100baseT_Full); > + break; > + default: > bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS); > return; > } (For readability, it's not for me to decide in the end). Thanks, Antoine ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3 2/2] net: macb: Clean up macb_validate 2021-10-13 8:29 ` Antoine Tenart @ 2021-10-14 16:01 ` Sean Anderson 0 siblings, 0 replies; 4+ messages in thread From: Sean Anderson @ 2021-10-14 16:01 UTC (permalink / raw) To: Antoine Tenart, David S . Miller, Jakub Kicinski, netdev Cc: Claudiu Beznea, Nicolas Ferre, Russell King On 10/13/21 4:29 AM, Antoine Tenart wrote: > Quoting Sean Anderson (2021-10-12 21:46:44) >> + /* There are three major types of interfaces we support: >> + * - (R)MII supporting 10/100 Mbit/s >> + * - GMII, RGMII, and SGMII supporting 10/100/1000 Mbit/s >> + * - 10GBASER supporting 10 Gbit/s only >> + * Because GMII and MII both support 10/100, GMII falls through to MII. >> + * >> + * If we can't support an interface mode, we just clear the supported >> + * mask and return. The major complication is that if we get >> + * PHY_INTERFACE_MODE_NA, we must return all modes we support. Because >> + * of this, NA starts at the top of the switch and falls all the way to >> + * the bottom, and doesn't return early if we don't support a >> + * particular mode. >> + */ >> + switch (state->interface) { >> + case PHY_INTERFACE_MODE_NA: >> + case PHY_INTERFACE_MODE_10GBASER: >> + if (bp->caps & MACB_CAPS_HIGH_SPEED && >> + bp->caps & MACB_CAPS_PCS && >> + bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) { >> + phylink_set_10g_modes(mask); >> + phylink_set(mask, 10000baseKR_Full); >> + } else if (one) { >> + bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS); >> + return; >> + } >> + if (one) >> + break; > > This can go in the first if block. OK. >> + fallthrough; >> + case PHY_INTERFACE_MODE_GMII: >> + 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_SGMII: >> + if (macb_is_gem(bp)) { >> + if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) { > > Is not having MACB_CAPS_GIGABIT_MODE_AVAILABLE acceptable here, or > should the two above checks be merged? As I understand it, macb_is_gem does not imply GIGABIT_MODE. I'm not sure if GIGABIT_MODE implies macb_is_gem. The logic here is mostly to match that in prepare()/config(). From what I can gather, all accesses to GEM registers are protected by macb_is_gem. --Sean >> + phylink_set(mask, 1000baseT_Full); >> + phylink_set(mask, 1000baseX_Full); >> + if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF)) >> + phylink_set(mask, 1000baseT_Half); >> + } >> + } else if (one) { >> + bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS); >> + return; >> + } >> + fallthrough; >> + case PHY_INTERFACE_MODE_MII: >> + case PHY_INTERFACE_MODE_RMII: >> + phylink_set(mask, 10baseT_Half); >> + phylink_set(mask, 10baseT_Full); >> + phylink_set(mask, 100baseT_Half); >> + phylink_set(mask, 100baseT_Full); >> + break; >> + default: >> bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS); >> return; >> } > > (For readability, it's not for me to decide in the end). > > Thanks, > Antoine > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-10-14 16:01 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-10-12 19:46 [PATCH v3 1/2] net: macb: Allow SGMII only if we are a GEM in mac_validate Sean Anderson 2021-10-12 19:46 ` [PATCH v3 2/2] net: macb: Clean up macb_validate Sean Anderson 2021-10-13 8:29 ` Antoine Tenart 2021-10-14 16:01 ` Sean Anderson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox