Received-SPF: pass (google.com: domain of linux-kernel+bounces-157266-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 24 Apr 2024 17:56:47 +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: <Zikrv5UOWvSGjgcv@eichest-laptop>
References: <20240416121032.52108-3-eichest@gmail.com>
 <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>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Zikd+GxuwMRC+5Ae@shell.armlinux.org.uk>

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