Re: [RFC PATCH net-next 2/8] phy: introduce the PHY_MODE_ETHERNET_PHY mode for phy_set_mode_ext()

[Vladimir Oltean <vladimir.oltean@nxp.com>]

Hi Sean,

On Mon, Aug 21, 2023 at 01:30:46PM -0400, Sean Anderson wrote:
> On 8/17/23 11:06, Vladimir Oltean wrote:
> > As opposed to PHY_MODE_ETHERNET which takes a phy_interface_t as is
> > expected to be used by an Ethernet MAC driver, PHY_MODE_ETHERNET takes
> > an enum ethtool_link_mode_bit_indices and expects to be used by an
> > Ethernet PHY driver.
> > 
> > It is true that the phy_interface_t type also contains definitions for
> > PHY_INTERFACE_MODE_10GKR and PHY_INTERFACE_MODE_1000BASEKX, but those
> > were deemed to be mistakes, and shouldn't be used going forward, when
> > 10GBase-KR and 1GBase-KX are really link modes. Thus, I believe that the
> > distinction is necessary, rather than hacking more improper PHY modes.
> 
> 10GBase-KR and 1000Base-KX are both electrically (e.g. link mode) and
> functionally (e.g. phy mode) different from 10GBase-R and 1000Base-X due
> to differing autonegotiation. So the phy modes are still relevant, and
> should still be used to ensure the correct form of autonegotiation is
> selected.
> 
> That said, I do agree that from the phy's (serdes's) point of view,
> there are only electrical differences between these modes.
> 
> However, I'm not sure we need to have a separate mode here. I think this
> would only be necessary if there were electrically-incompatible modes
> which shared the same signalling. E.g. if 802.3 decided that they wanted
> a "long range backplane ethernet" or somesuch with different
> drive/equalization requirements from 1000BASE-KX et al. but with the
> same signalling. Otherwise, we can infer the link mode from the phy
> mode.
> 
> --Sean

Thanks for taking the time to look at this RFC.

I will ask a clarification question. When you say "I'm not sure we need
to have a separate mode here", what do you mean?

The lynx-28g implementation (not shown here) will need to distinguish
between 1000Base-X and 1000Base-KX, and between 10GBase-R and 10GBase-KR
respectively, to configure the number of electrical equalization taps in
the LNmTECR registers, and to allocate memory for the ("K"-specific)
link training algorithm. Also, in the particular case of BaseX vs
BaseKX, we need to modify the PCCR8 register depending on whether the
C22 BaseX PCS or the C45 PCS + AN/LT blocks need to be available over
MDIO.

So, passing PHY_INTERFACE_MODE_1000BASEX when we intend 1000Base-KX is
simply not possible, because the dpaa2-mac consumer already uses
PHY_INTERFACE_MODE_1000BASEX to mean a very different (and legit) thing.

Do you mean instead that we could use the PHY_INTERFACE_MODE_1000BASEKX
that you've added to phy_interface_t? It's not clear that this is what
you're suggesting, so feel free to stop reading here if it isn't.

But mtip_backplane uses linkmode_c73_priority_resolution() (a function
added by me, sure, but nonetheless, it operates in the linkmode namespace,
as a PHY driver helper should) to figure out the proper argument to pass
to phy_set_mode_ext(). That argument has the enum ethtool_link_mode_bit_indices.

So, a translation between enum ethtool_link_mode_bit_indices and
phy_interface_t would be needed. That would be more or less doable for
1000Base-KX and 10GBase-KR, but it needs more phy_interface_t additions
for:

static const enum ethtool_link_mode_bit_indices c73_linkmodes[] = {
	ETHTOOL_LINK_MODE_100000baseCR4_Full_BIT,
	ETHTOOL_LINK_MODE_100000baseKR4_Full_BIT,
	/* ETHTOOL_LINK_MODE_100000baseKP4_Full_BIT not supported */
	/* ETHTOOL_LINK_MODE_100000baseCR10_Full_BIT not supported */
	ETHTOOL_LINK_MODE_40000baseCR4_Full_BIT,
	ETHTOOL_LINK_MODE_40000baseKR4_Full_BIT,
	ETHTOOL_LINK_MODE_25000baseKR_Full_BIT,
	ETHTOOL_LINK_MODE_25000baseCR_Full_BIT,
	/* ETHTOOL_LINK_MODE_25000baseKRS_Full_BIT not supported */
	/* ETHTOOL_LINK_MODE_25000baseCRS_Full_BIT not supported */
	ETHTOOL_LINK_MODE_10000baseKR_Full_BIT,
	ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT,
	ETHTOOL_LINK_MODE_1000baseKX_Full_BIT,
};

I guess that network PHY maintainers will need to chime in and say
whether that's the path forward or not.