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

[Sean Anderson]

On 8/21/23 14:13, Vladimir Oltean wrote:
> 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.

Yes. The intent for this interface mode is for the PCS to select the
appropriate autonegotiation. So if you use the 1000Base-KX link mode,
you should also use the 1000BASEKX phy mode (unless you have a separate
phy doing the conversion).

> 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,
> };

Well, I suppose you really do have "electrically-incompatible modes
which shared the same signalling". So I think it's reasonable to go this
route.

I considered something like this for lynx10g, but that serdes only
supports 10GBase-KR, so there was no need to differentiate that from
10GBase-KX4 (or 10GBase-CR).

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

I think the commit message could better motivate this patch by drawing a
distinction between a serdes which is talking to a phy or sfp which will
convert the signals to the final link mode, and a serdes which is acting
as the final connection to the far end, with nothing intervening.

--Sean