[PATCH RFC] Providing a helper for PCS inband negotiation

[Vladimir Oltean]

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.