Message-ID: <984fec49-4c08-9d5a-d62f-c59f106f8fe5@seco.com>
Date:   Sat, 16 Jul 2022 18:29:54 -0400
From:   Sean Anderson <sean.anderson@...o.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Madalin Bucur <madalin.bucur@....com>, netdev@...r.kernel.org,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>,
        linux-arm-kernel@...ts.infradead.org,
        Russell King <linux@...linux.org.uk>,
        linux-kernel@...r.kernel.org,
        Alexandru Marginean <alexandru.marginean@....com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Vladimir Oltean <olteanv@...il.com>
Subject: Re: [PATCH net-next v3 08/47] net: phylink: Support differing link
 speeds and interface speeds

On 7/16/22 4:06 PM, Andrew Lunn wrote:
>> +/**
>> + * phy_interface_speed() - get the speed of a phy interface
>> + * @interface: phy interface mode defined by &typedef phy_interface_t
>> + * @link_speed: the speed of the link
>> + *
>> + * Some phy interfaces modes adapt to the speed of the underlying link (such as
>> + * by duplicating data or changing the clock rate). Others, however, are fixed
>> + * at a particular rate. Determine the speed of a phy interface mode for a
>> + * particular link speed.
>> + *
>> + * Return: The speed of @interface
>> + */
>> +static int phy_interface_speed(phy_interface_t interface, int link_speed)
>> +{
>> +	switch (interface) {
>> +	case PHY_INTERFACE_MODE_100BASEX:
>> +		return SPEED_100;
>> +
>> +	case PHY_INTERFACE_MODE_TBI:
>> +	case PHY_INTERFACE_MODE_MOCA:
>> +	case PHY_INTERFACE_MODE_RTBI:
>> +	case PHY_INTERFACE_MODE_1000BASEX:
>> +	case PHY_INTERFACE_MODE_1000BASEKX:
>> +	case PHY_INTERFACE_MODE_TRGMII:
>> +		return SPEED_1000;
>> +
>> +	case PHY_INTERFACE_MODE_2500BASEX:
>> +		return SPEED_2500;
>> +
>> +	case PHY_INTERFACE_MODE_5GBASER:
>> +		return SPEED_5000;
>> +
>> +	case PHY_INTERFACE_MODE_XGMII:
>> +	case PHY_INTERFACE_MODE_RXAUI:
>> +	case PHY_INTERFACE_MODE_XAUI:
>> +	case PHY_INTERFACE_MODE_10GBASER:
>> +	case PHY_INTERFACE_MODE_10GKR:
>> +		return SPEED_10000;
>> +
>> +	case PHY_INTERFACE_MODE_25GBASER:
>> +		return SPEED_25000;
>> +
>> +	case PHY_INTERFACE_MODE_XLGMII:
>> +		return SPEED_40000;
>> +
>> +	case PHY_INTERFACE_MODE_USXGMII:
>> +	case PHY_INTERFACE_MODE_RGMII_TXID:
>> +	case PHY_INTERFACE_MODE_RGMII_RXID:
>> +	case PHY_INTERFACE_MODE_RGMII_ID:
>> +	case PHY_INTERFACE_MODE_RGMII:
>> +	case PHY_INTERFACE_MODE_QSGMII:
>> +	case PHY_INTERFACE_MODE_SGMII:
>> +	case PHY_INTERFACE_MODE_GMII:
>> +	case PHY_INTERFACE_MODE_REVRMII:
>> +	case PHY_INTERFACE_MODE_RMII:
>> +	case PHY_INTERFACE_MODE_SMII:
>> +	case PHY_INTERFACE_MODE_REVMII:
>> +	case PHY_INTERFACE_MODE_MII:
>> +	case PHY_INTERFACE_MODE_INTERNAL:
>> +		return link_speed;
>> +
>> +	case PHY_INTERFACE_MODE_NA:
>> +	case PHY_INTERFACE_MODE_MAX:
>> +		break;
>> +	}
>> +
>> +	return SPEED_UNKNOWN;
> 
> This seem error prone when new PHY_INTERFACE_MODES are added. I would
> prefer a WARN_ON_ONCE() in the default: so we get to know about such
> problems.

Actually, this is the reason I did not add a default: clause to the
switch (and instead listed everything out). If a new interface mode is
added, there will be a warning (as I discovered when preparing this
patch). I can still add a warning here if you'd like; the return there
should effectively be dead code.

> I'm also wondering if we need a sanity check here. I've seen quite a
> few boards a Fast Ethernet MAC, but a 1G PHY because they are
> cheap. In such cases, the MAC is supposed to call phy_set_max_speed()
> to indicate it can only do 100Mbs. PHY_INTERFACE_MODE_MII but a
> link_speed of 1G is clearly wrong. Are there other cases where we
> could have a link speed faster than what the interface mode allows?

AFAIK the phy must report SPEED_100 here, since many MACs set their
configuration based on the resolved speed. So if a phy reported
SPEED_1000 then the MAC would be confused.

> Bike shedding a bit, but would it be better to use host_side_speed and
> line_side_speed? When you say link_speed, which link are your
> referring to? Since we are talking about the different sides of the
> PHY doing different speeds, the naming does need to be clear.
When I say "link" I mean the thing that the PMD speaks. That is, one of
the ethtool link mode bits. I am thinking of a topology like


MAC (+PCS) <-- phy interface mode (MII) --> phy <-- link mode --> far-end phy

The way it has been done up to now, the phy interface mode and the link
mode have the same speed. For some MIIs, (such as MII or GMII) this is
actually the case, since the data clock changes depending on the data
speed. For others (SGMII/USXGMII) the data is repeated, but the clock
rate stays the same. In particular, the MAC doesn't care what the actual
link speed is, just what configuration it has to use (so it selects the
right clock etc).

The exception to the above is when you have no phy (such as for
1000BASE-X):

MAC (+PCS) <-- MDI --> PMD <-- link mode --> far-end PMD

All of the phy interface modes which can be used this way are
"non-adaptive." That is, in the above case they have a fixed speed.

That said, I would like to keep the "phy interface mode speed" named
"speed" so I don't have to write up a semantic patch to rename it in all
the drivers.

---

One thing I thought about is that it might be better to set this based
on the phy adaptation as well. Something like

static void phylink_set_speed(struct phylink_link_state *state)
{
	if (state->rate_adaptation == RATE_ADAPT_NONE) {
		state->speed = state->link_speed;
		return;
	}

	switch (state->interface) {
	case PHY_INTERFACE_MODE_REVRMII:
	case PHY_INTERFACE_MODE_RMII:
	case PHY_INTERFACE_MODE_SMII:
	case PHY_INTERFACE_MODE_REVMII:
	case PHY_INTERFACE_MODE_MII:
	case PHY_INTERFACE_MODE_100BASEX:
		state->speed = SPEED_100;
		return;

	case PHY_INTERFACE_MODE_RGMII_TXID:
	case PHY_INTERFACE_MODE_RGMII_RXID:
	case PHY_INTERFACE_MODE_RGMII_ID:
	case PHY_INTERFACE_MODE_RGMII:
	case PHY_INTERFACE_MODE_QSGMII:
	case PHY_INTERFACE_MODE_SGMII:
	case PHY_INTERFACE_MODE_GMII:
	case PHY_INTERFACE_MODE_TBI:
	case PHY_INTERFACE_MODE_MOCA:
	case PHY_INTERFACE_MODE_RTBI:
	case PHY_INTERFACE_MODE_1000BASEX:
	case PHY_INTERFACE_MODE_1000BASEKX:
	case PHY_INTERFACE_MODE_TRGMII:
		state->speed = SPEED_1000;
		return;

	case PHY_INTERFACE_MODE_2500BASEX:
		state->speed = SPEED_2500;
		return;

	case PHY_INTERFACE_MODE_5GBASER:
		state->speed = SPEED_5000;
		return;

	case PHY_INTERFACE_MODE_USXGMII:
	case PHY_INTERFACE_MODE_XGMII:
	case PHY_INTERFACE_MODE_RXAUI:
	case PHY_INTERFACE_MODE_XAUI:
	case PHY_INTERFACE_MODE_10GBASER:
	case PHY_INTERFACE_MODE_10GKR:
		state->speed = SPEED_10000;
		return;

	case PHY_INTERFACE_MODE_25GBASER:
		state->speed = SPEED_25000;
		return;

	case PHY_INTERFACE_MODE_XLGMII:
		state->speed = SPEED_40000;
		return;

	case PHY_INTERFACE_MODE_INTERNAL:
		state->speed = link_speed;
		return;

	case PHY_INTERFACE_MODE_NA:
	case PHY_INTERFACE_MODE_MAX:
		state->speed = SPEED_UNKNOWN;
		return;
	}

	WARN();
}

The reason being that this would allow for rate adaptation for "rate
adapting" phy interface modes such as MII. This would be necessary for
things like RATE_ADAPT_CRS modes like 10PASS-TS which always use 100M
MII, but have a variable link speed.

--Sean

Powered by blists - more mailing lists