Message-ID: <YtqjFKUTsH4CK0L+@shell.armlinux.org.uk>
Date:   Fri, 22 Jul 2022 14:16:04 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Marek Behún <kabel@...nel.org>,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Alvin __ipraga <alsi@...g-olufsen.dk>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Daniel Scally <djrscally@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        DENG Qingfang <dqfext@...il.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        George McCollister <george.mccollister@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Hauke Mehrtens <hauke@...ke-m.de>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Kurt Kanzenbach <kurt@...utronix.de>,
        Landen Chao <Landen.Chao@...iatek.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        linux-acpi@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org,
        Matthias Brugger <matthias.bgg@...il.com>,
        netdev@...r.kernel.org, Paolo Abeni <pabeni@...hat.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Sean Wang <sean.wang@...iatek.com>,
        UNGLinuxDriver@...rochip.com,
        Vivien Didelot <vivien.didelot@...il.com>,
        Woojung Huh <woojung.huh@...rochip.com>
Subject: Re: [PATCH net-next 3/6] net: dsa: add support for retrieving the
 interface mode

On Fri, Jul 22, 2022 at 03:46:29PM +0300, Vladimir Oltean wrote:
> On Fri, Jul 22, 2022 at 12:44:17PM +0100, Russell King (Oracle) wrote:
> > Today, there is no guarantee - because it depends on how people have
> > chosen to implement 2500base-X, and whether the hardware requires the
> > use of in-band AN or prohibits it. This is what happens when stuff
> > isn't standardised - one ends up with differing implementations doing
> > different things, and this has happened not _only_ at hardware level
> > but also software level as well.
> > 
> > You have to also throw into this that various implementations also have
> > an "AN bypass" flag, which means if they see what looks like a valid
> > SERDES data stream, but do not see the AN data, after a certain timeout
> > they allow the link to come up - and again, whether that is enabled or
> > not is pot luck today.
> 
> Interesting. After the timeout expires, does the lane ever transition
> back into the encoding required for AN mode, in case there appears at a
> later time someone willing to negotiate?

They don't document that it does.

> > > similarly, there is a good chance that the DT description below might
> > > result in a functional link:
> > > 
> > > 	&eth0 {
> > > 		phy-mode = "2500base-x";
> > > 		managed = "in-band-status";
> > > 	};
> > > 
> > > 	&switch_cpu_port {
> > > 		ethernet = <&eth0>;
> > > 		phy-mode = "25000base-x";
> > > 
> > > 		fixed-link {
> > > 			speed = <2500>;
> > > 			full-duplex;
> > > 		};
> > > 	};
> > > 
> > > There is no expectation from either DT description to use in-band
> > > autoneg or not.
> > > 
> > > The fact that of_phy_is_fixed_link() was made by Stas Sergeev to say
> > > that a 'managed' link with the value != 'auto' is fixed prompted me to
> > > study exactly what those changes were about.
> > 
> > From what I can see, there is no formal definition of "in-band-status"
> > beyond what it says on the tin. The description in the DT binding
> > specification, which is really where this should be formally documented,
> > is totally lacking.
> > 
> > >     This patch introduces the new string property 'managed' that allows
> > >     the user to set the management type explicitly.
> > >     The supported values are:
> > >     "auto" - default. Uses either MDIO or nothing, depending on the presence
> > >     of the fixed-link node
> > >     "in-band-status" - use in-band status
> > 
> > This, and how this is implemented by mvneta, is the best we have to go
> > on for the meaning of this.
> > 
> > > This is why I am asking whether there is any formal definition of what
> > > managed = "in-band-status" means. You've said it means about retrieving
> > > link status from the PCS. What are you basing upon when you are saying that?
> > 
> > Given that this managed property was introduced for mvneta, mvneta's
> > implementation of it is the best reference we have to work out what
> > the intentions of it were beyond the commit text.
> > 
> > With in-band mode enabled, mvneta makes use of a fixed-link PHY, and
> > updates the fixed-link PHY with the status from its GMAC block (which
> > is the combined PCS+MAC).
> > 
> > So, when in-band mode is specified, the results from SGMII or 1000base-X
> > negotiation are read from the MAC side of the link, pushed into the
> > fixed-PHY, which then are reflected back into the driver via the usual
> > phylib adjust_link().
> > 
> > Have a read through mvneta's code at this commit:
> > 
> > git show 2eecb2e04abb62ef8ea7b43e1a46bdb5b99d1bf8:drivers/net/ethernet/marvell/mvneta.c
> > 
> > specifically, mvneta_fixed_link_update() and mvneta_adjust_link().
> > Note that when operating in in-band mode, there is actually no need
> > for the configuration of MVNETA_GMAC_AUTONEG_CONFIG to be touched
> > in any way since the values read from the MVNETA_GMAC_STATUS register
> > indicate what parameters the MAC is actually using. (The speed,
> > duplex, and pause bits in AUTONEG_CONFIG are ignored anyway if AN
> > is enabled.)
> 
> I view this as just an implementation detail and not as something that
> influences what managed = "in-band-status" is supposed to mean.
> 
> > I know this is rather wooly, but not everything is defined in black and
> > white, and we need to do the best we can with the information that is
> > available.
> 
> So mvneta at the stage of the commit you've mentioned calls
> mvneta_set_autoneg() with the value of pp->use_inband_status. There is
> then the exception to be made for the PCS being what's exposed to the
> medium, and in that case, ethtool may also override the pp->use_inband_status
> variable (which in turn affects the autoneg).
> 
> So if we take mvneta at this commit as the reference, what we learn is
> that using in-band status essentially depends on using in-band autoneg
> in the first place.
> 
> What is hard for me to comprehend is how we ever came to conclude that
> for SERDES protocols where clause 37 is possible (2500base-x should be
> part of this group), managed = "in-band-status" does not imply in-band
> autoneg, considering the mvneta precedent.

That is a recent addition, since the argument was made that when using
a 1000base-X fibre transceiver, using ethtool to disable autoneg is a
reasonable thing to do - and something that was supported with
mvneta_ethtool_set_link_ksettings() as it stands at the point in the
commit above.

> And why would we essentially redefine its meaning by stating that no,
> it is only about the status, not about the autoneg, even though the
> status comes from the autoneg for these protocols.

I'm not sure I understand what you're getting at there.

Going back to the mvneta combined PCS+MAC implementation, we read the
link parameters from the PCS when operating in in-band mode and throw
them at the fixed PHY so that ethtool works, along with all the usual
link up/down state reporting, carrier etc.

If autoneg is disabled, then we effectively operate in fixed-link mode
(use_inband_status becomes false, and we start forcing the link up/down
and also force the speed and duplex parameters by disabling autoneg.)

Note that this version of mvneta does not support 1000base-X mode, only
SGMII is actually supported.

There's a few things that are rather confusing in the driver:

MVNETA_GMAC_INBAND_AN_ENABLE - this controls whether in-band negotiation
is performed or not.
MVNETA_GMAC_AN_SPEED_EN - this controls whether the result of in-band
negotiation for speed is used, or the manually programmed speed in this
register.
MVNETA_GMAC_AN_DUPLEX_EN - same for duplex.
MVNETA_GMAC_AN_FLOW_CTRL_EN - same for pause (only symmetric pause is
supported)

MVNETA_GMAC2_INBAND_AN_ENABLE - misnamed, it selects whether SGMII (set)
or 1000base-X (unset) format for the 16-bit control word is used.

There is another bit in MVNETA_GMAC_CTRL_0 that selects between
1000base-X and SGMII operation mode, and when this bit is set for
1000base-X. This version of the driver doesn't support 1000base-X,
so this bit is never set.

-- 
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