Message-ID: <20210701155923.GC1350@shell.armlinux.org.uk>
Date:   Thu, 1 Jul 2021 16:59:23 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Robert Hancock <robert.hancock@...ian.com>
Cc:     andrew@...n.ch, hkallweit1@...il.com, davem@...emloft.net,
        kuba@...nel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: phylink: Support disabling autonegotiation
 for PCS

On Thu, Jul 01, 2021 at 03:52:22PM +0100, Russell King (Oracle) wrote:
> On Wed, Jun 30, 2021 at 11:49:27AM -0600, Robert Hancock wrote:
> > The auto-negotiation state in the PCS as set by
> > phylink_mii_c22_pcs_config was previously always enabled when the driver is
> > configured for in-band autonegotiation, even if autonegotiation was
> > disabled on the interface with ethtool. Update the code to set the
> > BMCR_ANENABLE bit based on the interface's autonegotiation enabled
> > state.
> > 
> > Update phylink_mii_c22_pcs_get_state to not check
> > autonegotiation-related fields when autonegotiation is disabled.
> > 
> > Update phylink_mac_pcs_get_state to initialize the state based on the
> > interface's configured speed, duplex and pause parameters rather than to
> > unknown when autonegotiation is disabled, before calling the driver's
> > pcs_get_state functions, as they are not likely to provide meaningful data
> > for these fields when autonegotiation is disabled. In this case the
> > driver is really just filling in the link state field.
> > 
> > Note that in cases where there is a downstream PHY connected, such as
> > with SGMII and a copper PHY, the configuration set by ethtool is handled by
> > phy_ethtool_ksettings_set and not propagated to the PCS. This is correct
> > since SGMII or 1000Base-X autonegotiation with the PCS should normally
> > still be used even if the copper side has disabled it.
> 
> In theory, this seems to be correct, but...
> 
> We do have some cases where, if a port is in 1000Base-X mode, the
> documentation explicitly states that AN must be enabled. So, I think
> if we are introducing the possibility to disable the negotiation in
> 1000Base-X mode, we need to give an option to explicitly reject that
> configuration attempt.
> 
> We also need this to be consistently applied over all the existing
> phylink-using drivers that support 1000Base-X without AN - we shouldn't
> end up in the situation where we have different behaviours with
> different network drivers.
> 
> So, we need mvneta and mvpp2 to reject such a configuration - with
> these ports in 1000Base-X mode, the documentation states:
> 
> "Bit 2 Field InBandAnEn In-band Auto-Negotiation enable. ...
> When <PortType> = 1 (1000BASE-X) this field must be set to 1."
> 
> We should be aware that there may be other hardware out there which
> may not support 1000BASE-X without inband.

Incidentally, this also means that when we're in 2500BASE-X mode on
mvneta and mvpp2, PortType is 1, and we must use autonegotiation.

I think we _really_ need to have a better discussion about the
presence of AN or not with 2500BASE-X as far as the kernel is concerned
because we have ended up in the situation where mvneta and mvpp2 always
enable it (through need) for 1000BASE-X and 2500BASE-X, whereas others
always disable it in 2500BASE-X. Meanwhile, Xilinx allows it to be
configured. We seem to have headed into a situation where different
SoCs from different manufacturers disagree on whether 2500BASE-X does
negotiation, and thus we've ended up with different kernel behaviours.
This is not sane.

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