Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 5 Jan 2023 14:40:50 +0000
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Vladimir Oltean <olteanv@gmail.com>
Cc:     Sean Anderson <sean.anderson@seco.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>, netdev@vger.kernel.org,
        "David S . Miller" <davem@davemloft.net>,
        Paolo Abeni <pabeni@redhat.com>, linux-kernel@vger.kernel.org,
        Jakub Kicinski <kuba@kernel.org>,
        Eric Dumazet <edumazet@google.com>,
        Tim Harvey <tharvey@gateworks.com>
Subject: Re: [PATCH net-next v5 4/4] phy: aquantia: Determine rate adaptation
 support from registers
Message-ID: <Y7bhctPZoyNnw1ay@shell.armlinux.org.uk>
References: <20230103220511.3378316-1-sean.anderson@seco.com>
 <20230103220511.3378316-5-sean.anderson@seco.com>
 <20230105140421.bqd2aed6du5mtxn4@skbuf>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230105140421.bqd2aed6du5mtxn4@skbuf>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Thu, Jan 05, 2023 at 04:04:21PM +0200, 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?".
> 
> 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?

No, beause of the special condition in phylink that if it's a clause 45
PHY and we use something like 10GBASE-R, we don't limit to just 10G
speed, but try all interface modes - on the assumption that the PHY
will switch its host interface.

RATE_MATCH_NONE doesn't state anything about whether the PHY operates
in a single interface mode or not - with 10G PHYs (and thus clause 45
PHYs) it seems very common from current observations for
implementations to do this kind of host-interface switching.

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