On Thu, Apr 1, 2021 at 5:33 PM Russell King - ARM Linux admin <li...@armlinux.org.uk> wrote: > > 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?
Yes, the Ethernet controller on the board that connects to port 9 on the mv88e6390 doesn't support 2500base-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? I had wondered if precisely this would fix it. I tested it and it does. The question I can't answer is will it break anything else? Should I send a patch? > > > 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!
