Received-SPF: pass (google.com: domain of linux-kernel+bounces-158441-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Thu, 25 Apr 2024 13:24:51 +0200
From: Stefan Eichenberger <eichest@gmail.com>
To: "Russell King (Oracle)" <linux@armlinux.org.uk>
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: <Zio9g9+wsFX39Vkx@eichest-laptop>
References: <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>
 <ZilLz8f6vQQCg4NB@shell.armlinux.org.uk>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZilLz8f6vQQCg4NB@shell.armlinux.org.uk>

On Wed, Apr 24, 2024 at 07:13:35PM +0100, Russell King (Oracle) wrote:
> 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.

What I changed in mvpp2 is to allow turing off inband in 2500base-x. The
mvpp2 driver can handle this use case in pcs_config, it will turn off AN
and force the link up. I think it should also work for mvneta, at least
the code looks almost the same. I get the following for the Port
Auto-Negotiation Configuration Register:

For 1Gbit/s it switches to SGMII and enables inband AN:
Memory mapped at address 0xffffa0112000.
Value at address 0xF2132E0C (0xffffa0112e0c): 0xB0C6

For 2.5Gbit/s it disables inband AN and forces the link to be up:
Memory mapped at address 0xffffaff88000.
Value at address 0xF2132E0C (0xffffaff88e0c): 0x9042

Then the status register shows also link up for 2.5Gbit/s:
Memory mapped at address 0xffffa5fe2000.
Value at address 0xF2132E10 (0xffffa5fe2e10): 0x683B

What might be confusing is that the port type, bit 1 in MAC Control
Register0, is set to SGMII for 2.5Gbit/s, because we can only turn off
autonegotiation for SGMII:
Memory mapped at address 0xffff8c26c000.
Value at address 0xF2132E00 (0xffff8c26ce00): 0x8BFD

My example patch still uses the old macros:

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 97e38f61ac65..15fadfd61313 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -6223,9 +6223,12 @@ static unsigned int mvpp2_gmac_pcs_query_inband(struct phylink_pcs *pcs,
 	 * When <PortType> = 1 (1000BASE-X) this field must be set to 1.
 	 * Therefore, inband is "required".
 	 */
-	if (phy_interface_mode_is_8023z(interface))
+	if (interface == PHY_INTERFACE_MODE_1000BASEX)
 		return LINK_INBAND_VALID | LINK_INBAND_REQUIRED;
 
+	if (interface == PHY_INTERFACE_MODE_2500BASEX)
+		return LINK_INBAND_VALID | LINK_INBAND_POSSIBLE;
+
 	/* SGMII and RGMII can be configured to use inband signalling of the
 	 * AN result. Indicate these as "possible".
 	 */