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

[Vladimir Oltean]

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?".

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.

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? So only ETHTOOL_LINK_MODE_10000baseT_Full_BIT
would be advertised on the media side?

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?

> +
> +	/* Must match at least one speed */
> +	return got_one ? RATE_MATCH_PAUSE : RATE_MATCH_NONE;
>  }