| Message ID | 20230817150644.3605105-3-vladimir.oltean@nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add C72/C73 copper backplane support for LX2160 | expand |
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 > In particular to the Lynx SerDes, it can be used (as the PMA/PMD layer) > in conjunction with a separate backplane AN/LT block to form a > full-fledged copper backplane Ethernet PHY. The configuration of the > lanes is relatively similar to what is done for a typical MAC-to-PHY > link, except that we allow tuning the electrical equalization parameters > of the link (support for which will come as a separate change). > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > include/linux/phy/phy.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h > index 456d21c67e4f..7e10761303fc 100644 > --- a/include/linux/phy/phy.h > +++ b/include/linux/phy/phy.h > @@ -39,6 +39,7 @@ enum phy_mode { > PHY_MODE_UFS_HS_B, > PHY_MODE_PCIE, > PHY_MODE_ETHERNET, > + PHY_MODE_ETHERNET_PHY, > PHY_MODE_MIPI_DPHY, > PHY_MODE_SATA, > PHY_MODE_LVDS,
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.
On Thu, Aug 17, 2023 at 06:06:38PM +0300, 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. This doesn't seem to be correct. I think one of your PHY_MODE_ETHERNET above should be PHY_MODE_ETHERNET_PHY - but maybe instead it should be PHY_MODE_ETHTOOL so that it's clearly different and doesn't get confused with PHY_MODE_ETHERNET ?
On Mon, Aug 21, 2023 at 07:14:04PM +0100, Russell King (Oracle) wrote: > On Thu, Aug 17, 2023 at 06:06:38PM +0300, 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. > > This doesn't seem to be correct. I think one of your PHY_MODE_ETHERNET > above should be PHY_MODE_ETHERNET_PHY - but maybe instead it should > be PHY_MODE_ETHTOOL so that it's clearly different and doesn't get > confused with PHY_MODE_ETHERNET ? > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! Correct, sorry for the incorrect commit description. PHY_MODE_ETHTOOL would also work perfectly fine to me, and is more explicit.
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
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h index 456d21c67e4f..7e10761303fc 100644 --- a/include/linux/phy/phy.h +++ b/include/linux/phy/phy.h @@ -39,6 +39,7 @@ enum phy_mode { PHY_MODE_UFS_HS_B, PHY_MODE_PCIE, PHY_MODE_ETHERNET, + PHY_MODE_ETHERNET_PHY, PHY_MODE_MIPI_DPHY, PHY_MODE_SATA, PHY_MODE_LVDS,
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. In particular to the Lynx SerDes, it can be used (as the PMA/PMD layer) in conjunction with a separate backplane AN/LT block to form a full-fledged copper backplane Ethernet PHY. The configuration of the lanes is relatively similar to what is done for a typical MAC-to-PHY link, except that we allow tuning the electrical equalization parameters of the link (support for which will come as a separate change). Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- include/linux/phy/phy.h | 1 + 1 file changed, 1 insertion(+)