Received-SPF: pass (google.com: domain of linux-kernel+bounces-157449-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 24 Apr 2024 19:13:35 +0100
From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Stefan Eichenberger <eichest@gmail.com>
Cc: Andrew Lunn <andrew@lunn.ch>, davem@davemloft.net, edumazet@google.com,
	kuba@kernel.org, pabeni@redhat.com, robh@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
	lxu@maxlinear.com, hkallweit1@gmail.com, michael@walle.cc,
	netdev@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property
 to disable SGMII autoneg
Message-ID: <ZilLz8f6vQQCg4NB@shell.armlinux.org.uk>
References: <3f7f278f-e490-47f1-971c-ecf44a70cee4@lunn.ch>
 <Zh6clAtI3NO+nMEi@eichest-laptop>
 <5ed39628-4ac0-4c4e-9a16-fd4bf9a6db29@lunn.ch>
 <Zh6mIv1Ee+1h21Xo@shell.armlinux.org.uk>
 <Zh6z90iCpLqF4fla@eichest-laptop>
 <Zh6/oVHUvnOVtHaC@shell.armlinux.org.uk>
 <Zh94yqo2EHRq8eEq@eichest-laptop>
 <ZiE156+BPpx/ciL6@shell.armlinux.org.uk>
 <Zikd+GxuwMRC+5Ae@shell.armlinux.org.uk>
 <Zikrv5UOWvSGjgcv@eichest-laptop>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Zikrv5UOWvSGjgcv@eichest-laptop>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>

On Wed, Apr 24, 2024 at 05:56:47PM +0200, Stefan Eichenberger wrote:
> On Wed, Apr 24, 2024 at 03:58:00PM +0100, Russell King (Oracle) wrote:
> > On Thu, Apr 18, 2024 at 04:01:59PM +0100, Russell King (Oracle) wrote:
> > > On Wed, Apr 17, 2024 at 09:22:50AM +0200, Stefan Eichenberger wrote:
> > > > I also checked the datasheet and you are right about the 1000base-X mode
> > > > and in-band AN. What worked for us so far was to use SGMII mode even for
> > > > 2.5Gbps and disable in-band AN (which is possible for SGMII). I think
> > > > this works because as you wrote, the genphy just multiplies the clock by
> > > > 2.5 and doesn't care if it's 1000base-X or SGMII. With your patches we
> > > > might even be able to use in-band autonegoation for 10,100 and 1000Mbps
> > > > and then just disable it for 2.5Gbps. I need to test it, but I have hope
> > > > that this should work.
> > > 
> > > There is another way we could address this. If the querying support
> > > had a means to identify that the endpoint supports bypass mode, we
> > > could then have phylink identify that, and arrange to program the
> > > mvpp2 end to be in 1000base-X + x2.5 clock + AN bypass, which would
> > > mean it wouldn't require the inband 16-bit word to be present.
> > > 
> > > I haven't fully thought it through yet - for example, I haven't
> > > considered how we should indicate to the PCS that AN bypass mode
> > > should be enabled or disabled via the pcs_config() method.
> > 
> > Okay, I've been trying to put more effort into this, but it's been slow
> > progress (sorry).
> > 
> > My thoughts from a design point of view were that we could just switch
> > to PHYLINK_PCS_NEG_OUTBAND instead of PHYLINK_PCS_NEG_INBAND_* and
> > everything at the PCS layer should be able to cope, but this is not the
> > case, especially with mvneta/mvpp2.
> > 
> > The problem is that mvneta/mvpp2 (and probably more) expect that
> > 
> > 1) MLO_AN_INBAND means that the PCS will be using inband, and that
> >    means the link up/down state won't be forced. This basically implies
> >    that only PHYLINK_PCS_NEG_INBAND_* can be used can be used for the
> >    PCS.
> > 
> > 2) !MLO_AN_INBAND means that an out-of-band mechanism will be used and
> >    that means that the link needs to be forced (since there's no way
> >    for the hardware to know whether the link should be up or down.)
> >    It's therefore expected that only PHYLINK_PCS_NEG_OUTBAND will be
> >    used for the PCS.
> > 
> > So, attempting to put a resolution of the PHY and PCS abilities into
> > phylink_pcs_neg_mode() and select the appropriate PHYLINK_PCS_NEG_*
> > mode alone just doesn't work. Yet... we need to do that in there when
> > considering whether inband can be enabled or not for non-PHY links.
> > 
> > Basically, it needs a re-think how to solve this...
> 
> Today I was playing around with my combination of mxl-gpy and mvpp2 and
> I got it working again with your patches applied. However, I hacked the
> phylink driver to only rely on what the phy and pcs support. I know this
> is not a proper solution, but it allowed me to verify the other changes.
> My idea was if the phy and pcs support inband then use it, otherwise use
> outband and ignore the rest.
> 
> Here is how my minimal phylink_pcs_neg_mode test function looks like:
> 
> static unsigned int phylink_pcs_neg_mode(struct phylink *pl,
> 					 struct phylink_pcs *pcs,
> 					 unsigned int mode,
> 					 phy_interface_t interface,
> 					 const unsigned long *advertising)
> {
> 	unsigned int phy_link_mode = 0;
> 	unsigned int pcs_link_mode;
> 
> 	pcs_link_mode = phylink_pcs_query_inband(pcs, interface);
> 	if (pl->phydev)
> 		phy_link_mode = phy_query_inband(pl->phydev, interface);
> 
> 	/* If the PCS or PHY can not provide inband, then use
> 	 * outband.
> 	 */
> 	if (!(pcs_link_mode & LINK_INBAND_VALID) ||
> 	    !(phy_link_mode & LINK_INBAND_VALID))
> 		return PHYLINK_PCS_NEG_OUTBAND;
> 
> 	return PHYLINK_PCS_NEG_INBAND_ENABLED;
> }

Note that I've changed the flags that get reported to be disable (bit 0)/
enable (bit 1) rather than valid/possible/required because the former
makes the resolution easier.

The problem is that merely returning outband doesn't cause mvneta/mvpp2
to force the link up. So for example, here's a SFP module which doesn't
support any inband for 2500base-X nor SGMII:

mvneta f1034000.ethernet eno2: copper SFP: interfaces=[mac=4,9-12,19,22-23, sfp=
4,23,27]
mvneta f1034000.ethernet eno2: copper SFP: chosen 2500base-x interface
mvneta f1034000.ethernet eno2: PHY i2c:sfp:16 uses interfaces 4,23,27, validatin
g 4,23
mvneta f1034000.ethernet eno2:  interface 4 (sgmii) rate match none supports 2-3
,5-6,13
mvneta f1034000.ethernet eno2:  interface 23 (2500base-x) rate match none suppor
ts 6,13,47
mvneta f1034000.ethernet eno2: PHY [i2c:sfp:16] driver [Broadcom BCM84881] (irq=
POLL)
mvneta f1034000.ethernet eno2: phy: 2500base-x setting supported 00,00000000,000
08000,0000206c advertising 00,00000000,00008000,0000206c
mvneta f1034000.ethernet eno2: copper SFP: PHY link in-band modes 0x1
mvneta f1034000.ethernet eno2: major config 2500base-x
mvneta f1034000.ethernet eno2: link modes: pcs=02 phy=01
mvneta f1034000.ethernet eno2: phylink_mac_config: mode=inband/2500base-x/none a
dv=00,00000000,00008000,0000206c pause=04
mvneta f1034000.ethernet eno2: phylink_sfp_module_start()
mvneta f1034000.ethernet eno2: phylink_sfp_link_up()
mvneta f1034000.ethernet eno2: phy link down 2500base-x/Unknown/Unknown/none/off
mvneta f1034000.ethernet eno2: phy link up sgmii/1Gbps/Full/none/off
mvneta f1034000.ethernet eno2: major config sgmii
mvneta f1034000.ethernet eno2: link modes: pcs=03 phy=01
mvneta f1034000.ethernet eno2: phylink_mac_config: mode=inband/sgmii/none adv=00,00000000,00008000,0000206c pause=00
mvneta f1034000.ethernet eno2: pcs link down
mvneta f1034000.ethernet eno2: pcs link down
mvneta f1034000.ethernet eno2: can LPI, EEE enabled, active
mvneta f1034000.ethernet eno2: enabling tx_lpi, timer 250us
mvneta f1034000.ethernet eno2: Link is Up - 1Gbps/Full - flow control off

This looks like the link is up, but it isn't - note "pcs link down".
If we look at the value of the GMAC AN status register:

Value at address 0xf1036c10: 0x0000600a

which indicates that the link is down, so no packets will pass.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!