Hi George, 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. The Ethernet adapter connecting to this > switch port doesn't support 2500BASEX so it never establishes a link. > 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? > > 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. > > 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. > > I've been thinking through how to get the desired behavior but I'm not > even sure what the desired behavior is. If you set phy-mode to > "1000base-x" in device-tree do you ever want interface to be set to > PHY_INTERFACE_MODE_2500BASEX? If so just for MLO_AN_INBAND or also for > ML_AN_FIXED? Do we want phylink_validate called in phylink_create even > though it gets called anyway for MLO_AN_INBAND and ML_AN_FIXED later?
I think these are good and valid questions. If I had a good justification for the current phylink behavior I would have answered them, but I don't.
