Received-SPF: pass (google.com: domain of linux-kernel+bounces-159107-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 25 Apr 2024 21:15:54 +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: <Ziq5+gRXGmqt9bXM@shell.armlinux.org.uk>
References: <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>
 <ZiqFOko7zFjfTdz4@shell.armlinux.org.uk>
 <ZiqUB0lwgw7vIozG@eichest-laptop>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZiqUB0lwgw7vIozG@eichest-laptop>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>

On Thu, Apr 25, 2024 at 07:33:59PM +0200, Stefan Eichenberger wrote:
> Now I got it, thanks a lot for the explanation. So the issue is that
> MLO_AN_INBAND + PHYLINK_PCS_NEG_OUTBAND is happening in my use case and
> therefore the link is not forced up because for that MLO_AN_PHY would be
> needed. I will also try to think about it.

Now that I've moved the setting of PortType and InBandAutoNegMode into
the pcs_config() method, I now have (on mvneta):

Value at address 0xf1036c00: 0x00008bfd	- PortType = 0
					  (SGMII, necessary to be able
					   to set InBandAnEn=0 below)

Value at address 0xf1036c08: 0x0000c018 - InBandAutoNegMode = 0
					  (1000Base-X mode)

Value at address 0xf1036c0c: 0x00009240	- 1000M, FD, unforced link
					  InBandAnEn = 0

Value at address 0xf1036c10: 0x0000600a - Sync, 1000M, FD, but no link

The reason that the link isn't being forced is because
mvneta_mac_link_up() is being called with mode = MLO_AN_INBAND
which expects the link to be controlled as a result of autoneg,
but we've configured autoneg to be off.

I'm wondering whether we need pl->cur_link_an_mode to be the desired
mode for selecting the result from phylink_pcs_neg_mode(), but also
maintain a separate pl->act_link_an_mode which phylink_pcs_neg_mode()
chooses, dependent on whether the PCS is using inband or outband
mode - and pl->act_link_an_mode is what gets passed to the MAC layer.
That would at least keep the MAC MLO_AN_* consistent with what the
PCS layer is using - and also has the advantage that it makes it
clear that pl->act_link_an_mode only gets updated in the "major
config" path.

A quick test of that... seems to work:

mvneta f1034000.ethernet eno2: PHY [i2c:sfp:16] driver [Broadcom BCM84881] (irq=POLL)
mvneta f1034000.ethernet eno2: phy: 2500base-x setting supported 00,00000000,00008000,0000206c advertising 00,00000000,00008000,0000206c
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 adv=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=phy/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

Value at address 0xf1036c00: 0x00008bfd
Value at address 0xf1036c08: 0x0000c018
Value at address 0xf1036c0c: 0x00009242
Value at address 0xf1036c10: 0x0000600b

So we can see in the two phylink_mac_config calls that the mode has
switched from "inband" to "phy" with this PHY (BCM84881) which
doesn't support inband in any interface modes.

However, there's still the issue with:

link modes: pcs=02 phy=01
phylink_mac_config: mode=inband/2500base-x/none adv=00,00000000,00008000,0000206c pause=04

and this is because of the missing code in this part:

	/* PHY present, inband mode depends on the capabilities
	 * of both.
	 */

but there's also the issue that the PCS and PHY capabilities like that
are incompatible. In this case, we're saved by the fact that if we do
this act_link_an_mode thing:

        pl->act_link_an_mode = pl->cur_link_an_mode;
        if (pl->pcs_neg_mode == PHYLINK_PCS_NEG_OUTBAND &&
            pl->act_link_an_mode == MLO_AN_INBAND)
                pl->act_link_an_mode = MLO_AN_PHY;

coupled with the new _behaviour_ of mvneta/mvpp2, we don't actually
end up in the "1000base-X must have AN enabled" trap... but that is
no basis to basing decisions at the phylink layer on.

So, I'm wondering whether we need to be a little more creative here.
Instead of simply passing a few bits, maybe something like:

	31-24: bitfield of "partner" capabilities that are supported
		for inband enabled mode
	23-16: bitfield of "partner" capabilities that are supported
		for inband disabled mode
	15-8: bitfield of "partner" capabilities that are supported
		for outband mode
	2: bypass mode supported
	1: inband enabled mode supported
	0: inband disabled mode supported

Now, a question will come up... what is different between inband
disabled and outband mode?

Consider 1000base-X fibre. 1000base-X is the media interface, and we
need to be able to configure autoneg there, enabling or disabling it.
If we don't support disabling autoneg (as is the case with mvneta
et.al. over fibre) then being able to use ethtool to disable autoneg
can't be used. In both these modes, the 1000base-X is the media side.

However, 1000base-X can be used to connect to a PHY, and the PHY
could do rate matching, so the we need to use an outband way to
access the media side (we need to talk to the PHY.)

Hence why PCS have a distinction between OUTBAND and INBAND_DISABLED.

Now, with 2500base-X we run into the problem that e.g. mvneta
operating in 1000base-X mode upclocked to 2.5G can only support
INBAND_ENABLED and not INBAND_DISABLED (we can't just turn off the
InBandAnEn bit). The change between INBAND_ENABLED and INBAND_DISABLED
can happen with the link up.

However, it can support OUTBAND by disabling the PortType bit and then
turning off InBandAnEn (which can only be done with the link *down*
and that is only guaranteed during a "major config" not through the
ethtool settings API - which is why pcs_config() can't do this for 
INBAND_DISABLED.)

So, a little bit of progress but not at a usable solution yet.

I'm afraid my current tree is in a hacky mess at the moment, I'll see
about updating the published patches as soon as I can.

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