On Tue, Oct 26, 2021 at 11:30:08AM -0400, Sean Anderson wrote:
I don't know if it's a "fix" per se. The current logic isn't wrong,
since I believe that the configurations where the above patch would make
a difference do not exist. However, as noted in the commit message, this
makes refactoring difficult. For example, one might want to implement
supported_interfaces like
if (bp->caps & MACB_CAPS_HIGH_SPEED &&
bp->caps & MACB_CAPS_PCS)
__set_bit(PHY_INTERFACE_MODE_10GBASER, supported);
if (macb_is_gem(bp) && bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
__set_bit(PHY_INTERFACE_MODE_GMII, supported);
phy_interface_set_rgmii(supported);
if (bp->caps & MACB_CAPS_PCS)
__set_bit(PHY_INTERFACE_MODE_SGMII, supported);
}
__set_bit(PHY_INTERFACE_MODE_MII, supported);
__set_bit(PHY_INTERFACE_MODE_RMII, supported);
but then you still need to check for GIGABIT_MODE in validate to
determine whether 10GBASER should "support" 10/100. See [1] for more
discussion.
However, 10GBASE-R doesn't support anything except 10G speeds, except
if the PHY itself does rate matching to achieve the slower speeds.
Then you need pause frames between the MAC and PHY to control the MAC
sending rate - which isn't something that the phylib model supports
particularly well.
The current code and your patched code conforms to this when
state->interface is 10GBASE-R _and_ MACB_CAPS_GIGABIT_MODE_AVAILABLE
is set, then we only end up with 10G linkmodes being allowed.
The problem case occurs in current code when
MACB_CAPS_GIGABIT_MODE_AVAILABLE is _not_ set, but state->interface
is 10GBASE-R. Current code ends up saying that 10GBASE-R supports
10/100 link modes, which is wrong.
The existing code:
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);
Would have been better written as:
if (state->interface == PHY_INTERFACE_MODE_NA ||
state->interface == PHY_INTERFACE_MODE_10GBASER) {
if (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);
}
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);
which would have avoided getting to the code that sets 10/100 link
modes when state->interface is 10GBASE-R.
The same transformation is probably applicable to the next if ()
block as well.
If I truely understood exactly what MACB_CAPS_GIGABIT_MODE_AVAILABLE,
MACB_CAPS_HIGH_SPEED, and MACB_CAPS_PCS were indicating and how they
relate to what is supported, I might be tempted to come up with a
better implementation myself. However, every time I look at the
existing code, it just confuses me - it seems to me that the use of
those capability bits is entirely inconsistent in the current
macb_validate() implementation.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!