Message-ID: <Zwa-j1LKB3V2o2r9@shell.armlinux.org.uk>
Date: Wed, 9 Oct 2024 18:34:07 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Daniel Golle <daniel@...rotopia.org>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] net: phy: populate host_interfaces when
 attaching PHY

On Wed, Oct 09, 2024 at 12:52:13PM +0100, Daniel Golle wrote:
> Hi Russell,
> 
> On Wed, Oct 09, 2024 at 10:01:09AM +0100, Russell King (Oracle) wrote:
> > On Wed, Oct 09, 2024 at 02:57:03AM +0100, Daniel Golle wrote:
> > > Use bitmask of interfaces supported by the MAC for the PHY to choose
> > > from if the declared interface mode is among those using a single pair
> > > of SerDes lanes.
> > > This will allow 2500Base-T PHYs to switch to SGMII on most hosts, which
> > > results in half-duplex being supported in case the MAC supports that.
> > > Without this change, 2500Base-T PHYs will always operate in 2500Base-X
> > > mode with rate-matching, which is not only wasteful in terms of energy
> > > consumption, but also limits the supported interface modes to
> > > full-duplex only.
> > 
> > We've had a similar patch before, and it's been NAK'd. The problem is
> > that supplying the host_interfaces for built-in PHYs means that the
> > hardware strapping for the PHY interface mode becomes useless, as does
> > the DT property specifying it - and thus we may end up selecting a
> > mode that both the MAC and PHY support, but the hardware design
> > doesn't (e.g. signals aren't connected, signal speed to fast.)
> > 
> > For example, take a board designed to use RXAUI and the host supports
> > 10GBASE-R. The first problem is, RXAUI is not listed in the SFP
> > interface list because it's not usable over a SFP cage.
> 
> I thought about that, also boards configured for RGMII but both MAC
> and PHY supporting SGMII or even 2500Base-X would be such a case.
> In order to make sure we don't switch to link modes not supported
> by the design I check if the interface mode configured in DT is
> among those suitable for use with an SFP (ie. using a single pair
> of SerDes lanes):
> if (test_bit(pl->link_interface, phylink_sfp_interfaces))
> 	phy_interface_and(phy_dev->host_interfaces, phylink_sfp_interfaces,
> 			  pl->config->supported_interfaces);
> 
> Neither RXAUI nor RGMII modes are among phylink_sfp_interfaces, so
> cases in which those modes are configured in DT are already excluded.

This still won't work. There are drivers (boo, hiss, stmmac crap which
is resistant to cleanup and fixing but there's others too) that don't
do the phylink interface switching.

For example, stmmac sets the mode specified in DT and also if there
is a Synopsys XPCS, then the supported interfaces also gets USXGMII,
10GKR, XLGMII, 10GBASER, SGMII, 1000BASEX and 2500BASEX. If DT says
10GBASER, then the PHY must not switch to USXGMII, but if an
88x3310 were to be thrown in, the PHY driver _would_ decide to use
USXGMII against the DT configuration.

phydev->host_interfaces is there to allow PHYs on SFPs be properly
configured according to the host interface, where there is no DT
description for the module. It is not meant for built-in PHYs.

> > So, the
> > host_interfaces excludes that, and thus the PHY thinks that's not
> > supported. It looks at the mask and sees only 10GBASE-R, and
> > decides to use that instead with rate matching. The MAC doesn't have
> > support for flow control, and thus can't use rate matching.
> > 
> > Not only have the electrical charateristics been violated by selecting
> > a faster interface than the hardware was designed for, but we now have
> > rate matching being used when it shouldn't be.
> 
> As we are also using using rate matching right now in cases when it
> should not (and thereby inhibiting support for half-duplex modes), I
> suppose the only good solution would be to allow a set of interface
> modes in DT instead of only a single one.

Two issues... why was the PHY configured via firmware to use rate
matching if that brings with it this restriction, and it's possible
not to?

Second, aqr107_get_rate_matching() is rather basic based on what people
want. It doesn't actually ask the PHY what its going to do. I know
there's a bunch of VEND1_GLOBAL_CFG registers that give the serdes
mode and rate adaption for each speed, and these can be set not only
by firmware configuration, but changed by the host.

So, aqr107_get_rate_matching() should work out whether rate matching
will be used for the interface mode by scanning these registers.

Something like:

	u16 cfg_regs[] = {
		VEND1_GLOBAL_CFG_10M,
		VEND1_GLOBAL_CFG_100M,
		VEND1_GLOBAL_CFG_1G,
		VEND1_GLOBAL_CFG_2_5G,
		VEND1_GLOBAL_CFG_5G,
		VEND1_GLOBAL_CFG_10G
	};
	int i, val;
	u8 mode;

	switch (interface) {
	case PHY_INTERFACE_MODE_10GBASER:
		mode = VEND1_GLOBAL_CFG_SERDES_MODE_XFI;
		break;
	
	case PHY_INTERFACE_MODE_2500BASEX:
		mode = VEND1_GLOBAL_CFG_SERDES_MODE_OCSGMII;
		break;

	case PHY_INTERFACE_MODE_5GBASER:
		mode = VEND1_GLOBAL_CFG_SERDES_MODE_XFI5G;
		break;

	default:
		return RATE_MATCH_NONE;
	}

	/* If any speed corresponds to the interface mode and uses pause rate
	 * matching, indicate that this interface mode uses pause rate
	 * matching.
	 */
	for (i = 0; i < ARRAY_SIZE(cfg_regs); i++) {
		val = phy_read_mmd(phydev, MDIO_MMD_VEND1, cfg_regs[i]);
		if (val < 0)
			return val;

		if (FIELD_GET(VEND1_GLOBAL_CFG_SERDES_MODE, val) == mode) {
			if (FIELD_GET(VEND1_GLOBAL_CFG_RATE_ADAPT, val) ==
			    VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE)
				return RATE_MATCH_PAUSE;
		}
	}

	return RATE_MATCH_NONE;

Now, while phylink restricts RATE_MATCH_PAUSE to being full-duplex only,
I'm not sure that is correct. I didn't contribute this support, and I
don't have any platforms that support this, and I don't have any
experience of it.

What I do have is the data sheet for 88x3310, and that doesn't mention
any restriction such as "only full duplex is supported in rate matching
mode".

It is true that to use pause frames, the MAC/PCS must be in full-duplex
mode, but if the PHY supports half-duplex on the media to full-duplex
on the MAC side link, then why should phylink restrict this to be
full-duplex only?

I suspect phylink_get_capabilities() handling for RATE_MATCH_PAUSE is
not correct - or maybe not versatile enough.

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

Powered by blists - more mailing lists