| Message ID | 20211012194644.3182475-2-sean.anderson@seco.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v3,1/2] net: macb: Allow SGMII only if we are a GEM in mac_validate | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Single patches do not need cover letters |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 25 this patch: 25 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/checkpatch | warning | WARNING: Prefer 'fallthrough;' over fallthrough comment |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 25 this patch: 25 |
| netdev/header_inline | success | No static functions without inline keyword in header files |
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
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 >
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);
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(-)