Date:   Fri, 2 Apr 2021 11:36:51 -0500
From:   George McCollister <george.mccollister@...il.com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>
Subject: Re: net: phylink: phylink_helper_basex_speed issues with 1000base-x

On Thu, Apr 1, 2021 at 5:33 PM Russell King - ARM Linux admin
<linux@...linux.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!

Powered by blists - more mailing lists