Re: [PATCH net-next v5 4/4] phy: aquantia: Determine rate adaptation support from registers

[Sean Anderson]

On 1/5/23 09:04, Vladimir Oltean wrote:
> On Tue, Jan 03, 2023 at 05:05:11PM -0500, Sean Anderson wrote:
>>  static int aqr107_get_rate_matching(struct phy_device *phydev,
>>  				    phy_interface_t iface)
>>  {
>> -	if (iface == PHY_INTERFACE_MODE_10GBASER ||
>> -	    iface == PHY_INTERFACE_MODE_2500BASEX ||
>> -	    iface == PHY_INTERFACE_MODE_NA)
>> -		return RATE_MATCH_PAUSE;
>> -	return RATE_MATCH_NONE;
>> +	static const struct aqr107_link_speed_cfg speed_table[] = {
>> +		{
>> +			.speed = SPEED_10,
>> +			.reg = VEND1_GLOBAL_CFG_10M,
>> +			.speed_bit = MDIO_PMA_SPEED_10,
>> +		},
>> +		{
>> +			.speed = SPEED_100,
>> +			.reg = VEND1_GLOBAL_CFG_100M,
>> +			.speed_bit = MDIO_PMA_SPEED_100,
>> +		},
>> +		{
>> +			.speed = SPEED_1000,
>> +			.reg = VEND1_GLOBAL_CFG_1G,
>> +			.speed_bit = MDIO_PMA_SPEED_1000,
>> +		},
>> +		{
>> +			.speed = SPEED_2500,
>> +			.reg = VEND1_GLOBAL_CFG_2_5G,
>> +			.speed_bit = MDIO_PMA_SPEED_2_5G,
>> +		},
>> +		{
>> +			.speed = SPEED_5000,
>> +			.reg = VEND1_GLOBAL_CFG_5G,
>> +			.speed_bit = MDIO_PMA_SPEED_5G,
>> +		},
>> +		{
>> +			.speed = SPEED_10000,
>> +			.reg = VEND1_GLOBAL_CFG_10G,
>> +			.speed_bit = MDIO_PMA_SPEED_10G,
>> +		},
>> +	};
>> +	int speed = phy_interface_max_speed(iface);
>> +	bool got_one = false;
>> +	int i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(speed_table) &&
>> +		    speed_table[i].speed <= speed; i++) {
>> +		if (!aqr107_rate_adapt_ok(phydev, speed, &speed_table[i]))
>> +			return RATE_MATCH_NONE;
>> +		got_one = true;
>> +	}
> 
> Trying to wrap my head around the API for rate matching that was
> originally proposed and how it applies to what we read from Aquantia
> registers now.
> 
> IIUC, phylink (via the PHY library) asks "what kind of rate matching is
> supported for this SERDES protocol?". It doesn't ask "via what kind of
> rate matching can this SERDES protocol support this particular media
> side speed?".

We ask "What kind of rate matching is supported for this phy interface?"

> Your code walks through the speed_table[] of media speeds (from 10M up
> until the max speed of the SERDES) and sees whether the PHY was
> provisioned, for that speed, to use PAUSE rate adaptation.

This is because we assume that if a phy supports rate matching for a phy
interface mode, then it supports rate matching to all slower speeds that
it otherwise supports. This seemed like a pretty reasonable assumption
when I wrote the code, but it turns out that some firmwares don't abide
by this. This is firstly a problem with the firmware (as it should be
configured so that Linux can use the phy's features), but we have to be
careful not to end up with an unsupported combination.

> If the PHY firmware uses a combination like this: 10GBASE-R/XFI for
> media speeds of 10G, 5G, 2.5G (rate adapted), and SGMII for 1G, 100M
> and 10M, a call to your implementation of
> aqr107_get_rate_matching(PHY_INTERFACE_MODE_10GBASER) would return
> RATE_MATCH_NONE, right?

Correct.

> So only ETHTOOL_LINK_MODE_10000baseT_Full_BIT
> would be advertised on the media side?


If the host only supports 10GBASE-R and nothing else. If the host
supports SGMII as well, we will advertise 10G, 1G, 100M, and 10M. But
really, this is a problem with the firmware, since if the host supports
only 10GBASE-R, then the firmware should be set up to rate adapt to all
speeds.

> Shouldn't you take into consideration in your aqr107_rate_adapt_ok()
> function only the media side link speeds for which the PHY was actually
> *configured* to use the SERDES protocol @iface?

No, because we don't know what phy interface modes are actually going to
be used. The phy doesn't know whether e.g. the host supports both
10GBASE-R and SGMII or whether it only supports 10GBASE-R. With the
current API we cannot say "I support 5G" without also saying "I support
1G". If you don't like this, please send a patch for an API returning
supported speeds for a phy interface mode.

--Sean