Re: [PATCH RFC] Providing a helper for PCS inband negotiation

From: Vladimir Oltean <olteanv@gmail.com>
To: "Russell King (Oracle)" <linux@armlinux.org.uk>
Cc: Andrew Lunn <andrew@lunn.ch>,
	AngeloGioacchino Del Regno
	<angelogioacchino.delregno@collabora.com>,
	Daniel Machon <daniel.machon@microchip.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>, Felix Fietkau <nbd@nbd.name>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	Horatiu Vultur <horatiu.vultur@microchip.com>,
	Ioana Ciornei <ioana.ciornei@nxp.com>,
	Jakub Kicinski <kuba@kernel.org>, John Crispin <john@phrozen.org>,
	Jose Abreu <Jose.Abreu@synopsys.com>,
	Lars Povlsen <lars.povlsen@microchip.com>,
	Lorenzo Bianconi <lorenzo@kernel.org>,
	Marcin Wojtas <mw@semihalf.com>,
	Mark Lee <Mark-MC.Lee@mediatek.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Maxime Chevallier <maxime.chevallier@bootlin.com>,
	Paolo Abeni <pabeni@redhat.com>,
	Sean Wang <sean.wang@mediatek.com>,
	Steen Hegelund <Steen.Hegelund@microchip.com>,
	Taras Chornyi <taras.chornyi@plvision.eu>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	UNGLinuxDriver@microchip.com,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org, netdev@vger.kernel.org
Subject: Re: [PATCH RFC] Providing a helper for PCS inband negotiation
Date: Tue, 16 May 2023 17:15:44 +0300	[thread overview]
Message-ID: <20230516141544.t2e3ll3snrbi3oqq@skbuf> (raw)
In-Reply-To: <ZGNt2MFeRolKGFck@shell.armlinux.org.uk>

On Tue, May 16, 2023 at 12:49:44PM +0100, Russell King (Oracle) wrote:
> What I'm getting at is if the interface mode is
> PHY_INTERFACE_MODE_USXGMII, then... okay... we _may_ wish to do
> clause 73 negotiation advertising 10GBASE-KR and then do clause 73
                                                                  ~~
                                                                  37

> for the USXGMII control word - but the driver doesn't do this as far
> as I can see. If C73 AN is being used, it merely reads the C73
> state and returns the resolution from that. Any speed information that
> a USXGMII PHY passes back over the C37 inband signalling would be
> ignored because there seems to be no provision for the USXGMII
> inband signalling.
> 
> So I'm confused what the xpcs driver _actually_ does when USXGMII
> mode is selected by PHY_INTERFACE_MODE_USXGMII, because looking at
> the driver, it doesn't look like it's USXGMII at all.

If what you're looking for is strictly the USXGMII in-band autoneg code
word derived from C37, then the short answer is that you won't find it,
even when going back to the initial commit fcb26bd2b6ca ("net: phy: Add
Synopsys DesignWare XPCS MDIO module").

To the larger question 'what does XPCS actually do in phy-mode "usxgmii"',
I guess the simple answer looking at the aforementioned initial commit
is 'it violates the advertised coding scheme by using BASE-R instead of
BASE-X for speeds lower than 10G', and 'if you don't want the USXGMII
replicator just use phy-mode "10gbase-kr" which behaves basically the
same except it doesn't advertise the rate-adapted speeds'. Disclaimer:
I'm saying this based solely on the code and documentation.

> If we want to change that back to the old behaviour, that needs to
> be:
> 		if (test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, advertising)) {
> 			...
> 		}
> 		break;

Ok. I should send a patch fixing this, since I introduced the behavior
change. I'll add your suggested tag.

> but that wouldn't ever have been sufficient, even when the code was
> using an_enabled, because both of these reflect the user configuration.
> (an_enabled was just a proxy for this Autoneg bit). I'm going to call
> both of these an "AN indicator" in the question below.
> 
> Isn't it rather perverse that the driver configures AN if this AN
> indicator is set, but then does nothing if it isn't?

Maybe. My copy of the databook is parameterized based on
instantiation-dependent variables, and I can't really tell what are the
out-of-reset register values for hardware I don't have, so it is hard
for me to infer what is the behavior when AN is not enabled.

> As this is the only phylink-using implementation that involves clause
> 73 at present, I would like to ensure that there's a clear resolution
> of the expected behaviour before we get further implementations, and
> preferably document what's expected.

+1
that's where I would like this to go, too.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel