Hi, I hadn't responded earlier because I wanted to think about it more, but then forgot about this email.
On Thu, Mar 25, 2021 at 11:36:26AM -0500, George McCollister wrote: > When I set port 9 on an mv88e6390, a cpu facing port to use 1000base-x > (it also supports 2500base-x) in device-tree I find that > phylink_helper_basex_speed() changes interface to > PHY_INTERFACE_MODE_2500BASEX. If both 2500base-X and 1000base-X are reported as being advertised, then yes, it will. This is to support SFPs that can operate in either mode. The key thing here is that both speeds are being advertised and we're in either 2500base-X or 1000base-X mode. This gives userspace a way to switch between those two supported modes on the SFP. > The Ethernet adapter connecting to this > switch port doesn't support 2500BASEX so it never establishes a link. You mean the remote side only supports 1000base-X? > If I hack up the code to force PHY_INTERFACE_MODE_1000BASEX it works > fine. > > state->an_enabled is true when phylink_helper_basex_speed() is called > even when configured with fixed-link. This causes it to change the > interface to PHY_INTERFACE_MODE_2500BASEX if 2500BaseX_Full is in > state->advertising which it always is on the first call because > phylink_create calls bitmap_fill(pl->supported, > __ETHTOOL_LINK_MODE_MASK_NBITS) beforehand. Should state->an_enabled > be true with MLO_AN_FIXED? Historically, it has been (by the original fixed-phy implementation) and I don't wish to change that as that would be a user-visible change. Turning off state->an_enabled will make the interface depend on state->speed instead. > I've also noticed that phylink_validate (which ends up calling > phylink_helper_basex_speed) is called before phylink_parse_mode in > phylink_create. If phylink_helper_basex_speed changes the interface > mode this influences whether phylink_parse_mode (for MLO_AN_INBAND) > sets 1000baseX_Full or 2500baseX_Full in pl->supported (which is then > copied to pl->advertising). phylink_helper_basex_speed is then called > again (via phylink_validate) which uses advertising to decide how to > set interface. This seems like circular logic. I'm wondering if we should postpone the initial call to phylink_validate() to just before the "pl->cur_link_an_mode = pl->cfg_link_an_mode;" in there, and only if we're still in MLO_AN_PHY mode - it will already have been called via the other methods. Would that help to solve the problem? > To make matters even more confusing I see that > mv88e6xxx_serdes_dcs_get_state uses state->interface to decide whether > to set state->speed to SPEED_1000 or SPEED_2500. There is no real report from the hardware to indicate the speed - 2500base-X looks like 1000base-X except for the different interface mode. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
