Received-SPF: pass (google.com: domain of linux-kernel+bounces-158907-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 17:30:50 +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: <ZiqFOko7zFjfTdz4@shell.armlinux.org.uk>
References: <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>
 <ZippHJrnvzXsTiK4@shell.armlinux.org.uk>
 <Zip8Hd/ozP3R8ASS@eichest-laptop>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Zip8Hd/ozP3R8ASS@eichest-laptop>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>

On Thu, Apr 25, 2024 at 05:51:57PM +0200, Stefan Eichenberger wrote:
> On Thu, Apr 25, 2024 at 03:30:52PM +0100, Russell King (Oracle) wrote:
> > On Thu, Apr 25, 2024 at 01:24:51PM +0200, Stefan Eichenberger wrote:
> > > 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.
> 
> Yes I also saw this and didn't understand it. When I clear the force bit
> it will be set back to 1 again when AN is enabled. I thought this might
> be a bug of the controller.

No it isn't, the hardware never changes the value in this register.
The difference will be because of what I explained previously.

Because the mvneta and mvpp2 hardware is "weird" in that these two
registers provide both PCS and MAC functions, we have to deal with
them in a way that is split between the phylink PCS and MAC
operations.

This code was written at a time when all we had was MLO_AN_* and
none of the PHYLINK_PCS_NEG_* stuff. PCSes got passed the MLO_AN_*
constants which were the same as what was passed via the MAC
operations. This made the implementation entirely consistent.

However, that lead PCS implementers to go off and do their own
things, so we ended up with a range of different PCS behaviours
depending on the implementation (because the way people interpreted
MLO_AN_INBAND, interface mode, and the Autoneg bit in the advertising
mask was all different.

So to bring some consistency, I changed the PCS interface to what we
have now, in the belief that it would later allow us to solve the
2500base-X problem.

However, I'd forgotten about the mvneta/mvpp2 details, but that was
fine at the time of this change because everything was still
consistent - we would only ever use PHYLINK_PCS_NEG_OUTBAND or
PHYLINK_PCS_NEG_NONE for non-MLO_AN_INBAND modes, and
PHYLINK_PCS_NEG_INBAND_* for interface modes that support inband
when using MLO_AN_INBAND.

Now, when trying to solve the 2500base-X problem which needs us to
use PHYLINK_PCS_NEG_OUTBAND in some situations, this means we can
end up with MLO_AN_INBAND + PHYLINK_PCS_NEG_OUTBAND, and this is
what is causing me problems (the link isn't forced up.)

Conversely, I suspect you have the situation where you have MLO_AN_PHY
or MLO_AN_FIXED being passed to the mac_config() and mac_link_up()
operations, but the PCS is being configured for a different mode.

I am wondering whether we should at the very least move the
configuration of the control register 0 and 2 to the pcs_config()
method so at least that's consistent with the PHYLINK_PCS_NEG_*
value passed to the PCS and thus the values programmed into the
autoneg config register. However, that still leaves a big hole in
how we handle the link forcing - which is necessary if inband AN
is disabled (in other words, if we need to read the link status
from the PHY as is done in MLO_AN_PHY mode.)

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