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

On Thu, Apr 25, 2024 at 01:24:51PM +0200, Stefan Eichenberger wrote:
> 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.

pcs_config can't 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

So here the link is forced up which is wrong for inband, because then
we have no way to detect the link going down.

> 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

Control register 0 bit 1 is the port type bit, which controls whether
the port is in "1000base-X" mode or SGMII mode. This has no effect on
the interpretation of the inband control word.

Control register 2 bit 0 controls whether the port uses 802.3z
format control words or SGMII format control words.

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

This is not correct though. If we set PortType = 1, then this applies:

  Bit   Field        Type / InitVal   Description

  2     InBandAnEn   RW               In-band Auto-Negotiation enable.
..
                                      When <PortType> = 1 (1000BASE-X)
                                      this field must be set to 1.
                                                                                                                      When <PortType> = 0 (SGMII) and this
                                      field is 1, in-band Flow Control not
				      supported.

So, if we have the port in 1000base-X mode (PortType = 1) then bit 2
of the Autoneg configuration register needs to be set to be compliant
with the documentation. Therefore, since we set PortType = 1 for
2500BASE-X (and note that I _do_ run 2500BASE-X with inband AN enabled
over fibre transceivers here, so this is needed), we also need to
enable InBandAnEn.

This is exactly where the difficulty comes - because what you're
suggesting will break currently working setups such as what I have
here.

Switching the port to SGMII mode for 2500base-X doesn't sound like a
sensible thing to do.

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