| Message ID | 20211004191527.1610759-9-sean.anderson@seco.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add support for Xilinx PCS | expand |
On Mon, Oct 04, 2021 at 03:15:19PM -0400, Sean Anderson wrote: > 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. No functional change intended. This doesn't look right to me. > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > index e2730b3e1a57..18afa544b623 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -510,32 +510,55 @@ static void macb_validate(struct phylink_config *config, > unsigned long *supported, > struct phylink_link_state *state) > { > + bool one = state->interface == PHY_INTERFACE_MODE_NA; Shouldn't this be != ? Since PHY_INTERFACE_MODE_NA is supposed to return all capabilities.
On 10/4/21 7:04 PM, Russell King (Oracle) wrote: > On Mon, Oct 04, 2021 at 03:15:19PM -0400, Sean Anderson wrote: >> 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. No functional change intended. > > This doesn't look right to me. > >> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c >> index e2730b3e1a57..18afa544b623 100644 >> --- a/drivers/net/ethernet/cadence/macb_main.c >> +++ b/drivers/net/ethernet/cadence/macb_main.c >> @@ -510,32 +510,55 @@ static void macb_validate(struct phylink_config *config, >> unsigned long *supported, >> struct phylink_link_state *state) >> { >> + bool one = state->interface == PHY_INTERFACE_MODE_NA; > > Shouldn't this be != ? > > Since PHY_INTERFACE_MODE_NA is supposed to return all capabilities. > Ah, yes it should. --Sean
On 04/10/2021 at 21:15, Sean Anderson wrote: > While we're on the subject, could someone clarify the relationship > between the various speed capabilities? What's the difference between > MACB_CAPS_GIGABIT_MODE_AVAILABLE, MACB_CAPS_HIGH_SPEED, MACB_CAPS_PCS, > and macb_is_gem()? Would there ever be a GEM without GIGABIT_MODE? Yes. GEM is a new revision of the IP that is capable of doing Gigabit mode or not. sama7g5_emac_config is typically one of those doing only 10/100. > HIGH_SPEED without PCS? Why doesn't SGMII care if we're a gem (I think > this one is a bug, because it cares later on)? MACB_CAPS_HIGH_SPEED and MACB_CAPS_PCS were added by e4e143e26ce8f5f57c60a994bdc63d0ddce3a823 ("net: macb: add support for high speed interface"). In this commit it is said that "This controller has separate MAC's and PCS'es for low and high speed paths." Maybe it's a hint. Best regards, Nicolas
Hi Nicolas, On 10/7/21 9:22 AM, Nicolas Ferre wrote: > On 04/10/2021 at 21:15, Sean Anderson wrote: >> While we're on the subject, could someone clarify the relationship >> between the various speed capabilities? What's the difference between >> MACB_CAPS_GIGABIT_MODE_AVAILABLE, MACB_CAPS_HIGH_SPEED, MACB_CAPS_PCS, >> and macb_is_gem()? Would there ever be a GEM without GIGABIT_MODE? > > Yes. GEM is a new revision of the IP that is capable of doing Gigabit > mode or not. sama7g5_emac_config is typically one of those doing only > 10/100. Thanks for pointing that out. But even that config still has MACB_CAPS_GIGABIT_MODE_AVAILABLE. So presumably you can use it for gigabit speed if you don't use MII-on-RGMII. I suppose that sama7g5_emac_config is not a GEM? >> HIGH_SPEED without PCS? Why doesn't SGMII care if we're a gem (I think >> this one is a bug, because it cares later on)? > > MACB_CAPS_HIGH_SPEED and MACB_CAPS_PCS were added by > e4e143e26ce8f5f57c60a994bdc63d0ddce3a823 ("net: macb: add support for > high speed interface"). In this commit it is said that "This > controller has separate MAC's and PCS'es for low and high speed > paths." Maybe it's a hint. Ah, so perhaps HIGH_SPEED only represents the capability for a high-speed PCS. Which of course is a bit strange considering that we check for both it, PCS, and GIGABIT_MODE when determining whether we can do 10G. --Sean
Sean, On 08/10/2021 at 02:20, Sean Anderson wrote: > > On 10/7/21 9:22 AM, Nicolas Ferre wrote: >> On 04/10/2021 at 21:15, Sean Anderson wrote: >>> While we're on the subject, could someone clarify the relationship >>> between the various speed capabilities? What's the difference between >>> MACB_CAPS_GIGABIT_MODE_AVAILABLE, MACB_CAPS_HIGH_SPEED, MACB_CAPS_PCS, >>> and macb_is_gem()? Would there ever be a GEM without GIGABIT_MODE? >> >> Yes. GEM is a new revision of the IP that is capable of doing Gigabit >> mode or not. sama7g5_emac_config is typically one of those doing only >> 10/100. > > Thanks for pointing that out. But even that config still has > MACB_CAPS_GIGABIT_MODE_AVAILABLE. So presumably you can use it for > gigabit speed if you don't use MII-on-RGMII. I suppose that > sama7g5_emac_config is not a GEM? There must be a confusion between sama7g5_gem_config and sama7g5_emac_config here. The later one doesn't have this MACB_CAPS_GIGABIT_MODE_AVAILABLE capability. Both are flavors of GEM and identified as such in the driver. Best regards, Nicolas
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index e2730b3e1a57..18afa544b623 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -510,32 +510,55 @@ 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 || - 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)) { + 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(mask, 10000baseCR_Full); + phylink_set(mask, 10000baseER_Full); + phylink_set(mask, 10000baseKR_Full); + phylink_set(mask, 10000baseLR_Full); + phylink_set(mask, 10000baseLRM_Full); + phylink_set(mask, 10000baseSR_Full); + phylink_set(mask, 10000baseT_Full); + } else if (one) { + goto none; + } + 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: + if (!macb_is_gem(bp) && one) + goto none; + fallthrough; + case PHY_INTERFACE_MODE_SGMII: + 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); + } + 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; + none: + default: bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS); return; } @@ -543,38 +566,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(mask, 10000baseCR_Full); - phylink_set(mask, 10000baseER_Full); - phylink_set(mask, 10000baseKR_Full); - phylink_set(mask, 10000baseLR_Full); - phylink_set(mask, 10000baseLRM_Full); - phylink_set(mask, 10000baseSR_Full); - phylink_set(mask, 10000baseT_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. No functional change intended. While we're on the subject, could someone clarify the relationship between the various speed capabilities? What's the difference between MACB_CAPS_GIGABIT_MODE_AVAILABLE, MACB_CAPS_HIGH_SPEED, MACB_CAPS_PCS, and macb_is_gem()? Would there ever be a GEM without GIGABIT_MODE? HIGH_SPEED without PCS? Why doesn't SGMII care if we're a gem (I think this one is a bug, because it cares later on)? Signed-off-by: Sean Anderson <sean.anderson@seco.com> --- drivers/net/ethernet/cadence/macb_main.c | 99 +++++++++++------------- 1 file changed, 45 insertions(+), 54 deletions(-)