| Message ID | 20230201145845.2312060-3-o.rempel@pengutronix.de |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | net: add EEE support for KSZ9477 and AR8035 with i.MX6 | expand |
On Wed, Feb 01, 2023 at 03:58:24PM +0100, Oleksij Rempel wrote: > Add generic function for EEE abilities defined by IEEE 802.3 > specification. For now following registers are supported: > - IEEE 802.3-2018 45.2.3.10 EEE control and capability 1 (Register 3.20) > - IEEE 802.3cg-2019 45.2.1.186b 10BASE-T1L PMA status register > (Register 1.2295) > > Since I was not able to find any flag signaling support of this > registers, we should detect link mode abilities first and then based on > this abilities doing EEE link modes detection. Hi Oleksij There was a discussion along these lines with Chris Healy recently. The meson-gxl PHYs don't have these registers, and reads return 0xffff. The 802.3 2018 standard says the top 2 bits are reserved and should read as 0. Also, it seems unlikely anybody will build a PHY which supports 100GBASE-R deep sleep all the way down to 100BASE-TX EEE. So i would suggest adding a check when reading MDIO_PCS_EEE_ABLE and if it is 0xffff assume EEE is not supported. > + val = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_EEE_ABLE); > + if (val < 0) > + return val; > + > + mii_eee_100_10000_adv_mod_linkmode_t(phydev->supported_eee, val); > + > + /* Some buggy devices claim not supported EEE link modes */ > + linkmode_and(phydev->supported_eee, phydev->supported_eee, > + phydev->supported); That comment could be improved. What i think you mean is /* Some buggy devices indicate EEE link modes in MDIO_PCS_EEE_ABLE which they don't support as indicated by BMSR, ESTATUS etc. */ Andrew
On Wed, Feb 01, 2023 at 03:58:24PM +0100, Oleksij Rempel wrote: > Add generic function for EEE abilities defined by IEEE 802.3 > specification. For now following registers are supported: > - IEEE 802.3-2018 45.2.3.10 EEE control and capability 1 (Register 3.20) > - IEEE 802.3cg-2019 45.2.1.186b 10BASE-T1L PMA status register > (Register 1.2295) > > Since I was not able to find any flag signaling support of this these registers > registers, we should detect link mode abilities first and then based on > this abilities doing EEE link modes detection. these abilities > > Results of EEE ability detection will be stored in to new variable stored into > phydev->supported_eee. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > drivers/net/phy/phy-c45.c | 49 ++++++++++++++++++++++++++++++++++++ > drivers/net/phy/phy_device.c | 16 ++++++++++++ > include/linux/mdio.h | 17 +++++++++++++ > include/linux/phy.h | 5 ++++ > 4 files changed, 87 insertions(+) > > diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c > index 9f9565a4819d..ae87f5856650 100644 > --- a/drivers/net/phy/phy-c45.c > +++ b/drivers/net/phy/phy-c45.c > @@ -661,6 +661,55 @@ int genphy_c45_read_mdix(struct phy_device *phydev) > } > EXPORT_SYMBOL_GPL(genphy_c45_read_mdix); > > +/** > + * genphy_c45_read_eee_abilities - read supported EEE link modes > + * @phydev: target phy_device struct > + * > + * Read supported EEE link modes. > + */ > +int genphy_c45_read_eee_abilities(struct phy_device *phydev) > +{ > + __ETHTOOL_DECLARE_LINK_MODE_MASK(common); > + int val; > + > + linkmode_and(common, phydev->supported, PHY_EEE_100_10000_FEATURES); > + /* There is not indicator if optional register no indicator whether > + * "EEE control and capability 1" (3.20) is supported. Read it only > + * on devices with appropriate linkmodes. > + */ > + if (!linkmode_empty(common)) { if (linkmode_intersects(phydev->supported, PHY_EEE_100_10000_FEATURES))? > + /* IEEE 802.3-2018 45.2.3.10 EEE control and capability 1 > + * (Register 3.20) > + */ > + val = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_EEE_ABLE); > + if (val < 0) > + return val; Might the PHY also return 0xffff for an unsupported register? That would be interpreted as "EEE is supported for all link modes", no? > + > + mii_eee_100_10000_adv_mod_linkmode_t(phydev->supported_eee, val); > + > + /* Some buggy devices claim not supported EEE link modes */ unsupported > + linkmode_and(phydev->supported_eee, phydev->supported_eee, > + phydev->supported); > + } > + > + if (linkmode_test_bit(ETHTOOL_LINK_MODE_10baseT1L_Full_BIT, > + phydev->supported)) { > + /* IEEE 802.3cg-2019 45.2.1.186b 10BASE-T1L PMA status register > + * (Register 1.2295) > + */ > + val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_10T1L_STAT); > + if (val < 0) > + return val; > + > + linkmode_mod_bit(ETHTOOL_LINK_MODE_10baseT1L_Full_BIT, > + phydev->supported_eee, > + val & MDIO_PMA_10T1L_STAT_EEE); > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(genphy_c45_read_eee_abilities); > + > /** > * genphy_c45_pma_read_abilities - read supported link modes from PMA > * @phydev: target phy_device struct > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 9ba8f973f26f..3651f1fd8fc9 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -132,6 +132,18 @@ static const int phy_10gbit_full_features_array[] = { > ETHTOOL_LINK_MODE_10000baseT_Full_BIT, > }; > > +static const int phy_eee_100_10000_features_array[6] = { Don't need array length unless the array is sparse, which isn't the case here. > + ETHTOOL_LINK_MODE_100baseT_Full_BIT, > + ETHTOOL_LINK_MODE_1000baseT_Full_BIT, > + ETHTOOL_LINK_MODE_10000baseT_Full_BIT, > + ETHTOOL_LINK_MODE_1000baseKX_Full_BIT, > + ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT, > + ETHTOOL_LINK_MODE_10000baseKR_Full_BIT, Why stop at 10GBase-KR? Register 3.20 defines EEE abilities up to 100G (for speeds >10G, there seem to be 2 modes, "deep sleep" or "fast wake", with "deep sleep" being essentially equivalent to the only mode available for <=10G modes). > +}; > + > +__ETHTOOL_DECLARE_LINK_MODE_MASK(phy_eee_100_10000_features) __ro_after_init; > +EXPORT_SYMBOL_GPL(phy_eee_100_10000_features); > + > static void features_init(void) > { > /* 10/100 half/full*/ > @@ -213,6 +225,10 @@ static void features_init(void) > linkmode_set_bit_array(phy_10gbit_fec_features_array, > ARRAY_SIZE(phy_10gbit_fec_features_array), > phy_10gbit_fec_features); > + linkmode_set_bit_array(phy_eee_100_10000_features_array, > + ARRAY_SIZE(phy_eee_100_10000_features_array), > + phy_eee_100_10000_features); > + > } > > void phy_device_free(struct phy_device *phydev)
Hi Vladimir, On Sat, Feb 04, 2023 at 02:54:18AM +0200, Vladimir Oltean wrote: > On Wed, Feb 01, 2023 at 03:58:24PM +0100, Oleksij Rempel wrote: [....] > > +static const int phy_eee_100_10000_features_array[6] = { > > Don't need array length unless the array is sparse, which isn't the case here. > > > + ETHTOOL_LINK_MODE_100baseT_Full_BIT, > > + ETHTOOL_LINK_MODE_1000baseT_Full_BIT, > > + ETHTOOL_LINK_MODE_10000baseT_Full_BIT, > > + ETHTOOL_LINK_MODE_1000baseKX_Full_BIT, > > + ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT, > > + ETHTOOL_LINK_MODE_10000baseKR_Full_BIT, > > Why stop at 10GBase-KR? Register 3.20 defines EEE abilities up to 100G > (for speeds >10G, there seem to be 2 modes, "deep sleep" or "fast wake", > with "deep sleep" being essentially equivalent to the only mode > available for <=10G modes). Hm, If i take only deep sleep, missing modes are: 3.20.13 100GBASE-R deep sleep family of Physical Layer devices using 100GBASE-R encoding: 100000baseCR4_Full 100000baseKR4_Full 100000baseCR10_Full (missing) 100000baseSR4_Full 100000baseSR10_Full (missing) 100000baseLR4_ER4_Full 3.20.11 25GBASE-R deep sleep family of Physical Layer devices using 25GBASE-R encoding: 25000baseCR_Full 25000baseER_Full (missing) 25000baseKR_Full 25000baseLR_Full (missing) 25000baseSR_Full 3.20.9 40GBASE-R deep sleep family of Physical Layer devices using 40GBASE-R encoding: 40000baseKR4_Full 40000baseCR4_Full 40000baseSR4_Full 40000baseLR4_Full 3.20.7 40GBASE-T 40000baseT_Full (missing) I have no experience with modes > 1Gbit. Do all of them correct? What should we do with missing modes? Or may be it make sense to implement > 10G modes separately? Regards, Oleksij
On Mon, Feb 06, 2023 at 11:49:55AM +0100, Oleksij Rempel wrote: > > Why stop at 10GBase-KR? Register 3.20 defines EEE abilities up to 100G > > (for speeds >10G, there seem to be 2 modes, "deep sleep" or "fast wake", > > with "deep sleep" being essentially equivalent to the only mode > > available for <=10G modes). > > Hm, > > If i take only deep sleep, missing modes are: > 3.20.13 100GBASE-R deep sleep > family of Physical Layer devices using 100GBASE-R encoding: > 100000baseCR4_Full > 100000baseKR4_Full > 100000baseCR10_Full (missing) > 100000baseSR4_Full > 100000baseSR10_Full (missing) > 100000baseLR4_ER4_Full > > 3.20.11 25GBASE-R deep sleep > family of Physical Layer devices using 25GBASE-R encoding: > 25000baseCR_Full > 25000baseER_Full (missing) > 25000baseKR_Full > 25000baseLR_Full (missing) > 25000baseSR_Full > > 3.20.9 40GBASE-R deep sleep > family of Physical Layer devices using 40GBASE-R encoding: > 40000baseKR4_Full > 40000baseCR4_Full > 40000baseSR4_Full > 40000baseLR4_Full > > 3.20.7 40GBASE-T > 40000baseT_Full (missing) > > I have no experience with modes > 1Gbit. Do all of them correct? What > should we do with missing modes? Or may be it make sense to implement > > 10G modes separately? Given the fact that UAPI needs an extension to cover supported/advertisement bits > 31, I think it makes sense to add these separately. I had not realized this when I commented on this patch. I don't think we want the kernel to advertise EEE for some link modes without user space seeing it.
On Mon, Feb 06, 2023 at 01:22:46PM +0200, Vladimir Oltean wrote: > On Mon, Feb 06, 2023 at 11:49:55AM +0100, Oleksij Rempel wrote: > > > Why stop at 10GBase-KR? Register 3.20 defines EEE abilities up to 100G > > > (for speeds >10G, there seem to be 2 modes, "deep sleep" or "fast wake", > > > with "deep sleep" being essentially equivalent to the only mode > > > available for <=10G modes). > > > > Hm, > > > > If i take only deep sleep, missing modes are: > > 3.20.13 100GBASE-R deep sleep > > family of Physical Layer devices using 100GBASE-R encoding: > > 100000baseCR4_Full > > 100000baseKR4_Full > > 100000baseCR10_Full (missing) > > 100000baseSR4_Full > > 100000baseSR10_Full (missing) > > 100000baseLR4_ER4_Full > > > > 3.20.11 25GBASE-R deep sleep > > family of Physical Layer devices using 25GBASE-R encoding: > > 25000baseCR_Full > > 25000baseER_Full (missing) > > 25000baseKR_Full > > 25000baseLR_Full (missing) > > 25000baseSR_Full > > > > 3.20.9 40GBASE-R deep sleep > > family of Physical Layer devices using 40GBASE-R encoding: > > 40000baseKR4_Full > > 40000baseCR4_Full > > 40000baseSR4_Full > > 40000baseLR4_Full > > > > 3.20.7 40GBASE-T > > 40000baseT_Full (missing) > > > > I have no experience with modes > 1Gbit. Do all of them correct? What > > should we do with missing modes? Or may be it make sense to implement > > > 10G modes separately? > > Given the fact that UAPI needs an extension to cover supported/advertisement > bits > 31, I think it makes sense to add these separately. I had not > realized this when I commented on this patch. I don't think we want the > kernel to advertise EEE for some link modes without user space seeing it. We also don't currently support any PHYs which do more than 10G. So i don't see any need for 40GB and above at the moment. Andrew
diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c index 9f9565a4819d..ae87f5856650 100644 --- a/drivers/net/phy/phy-c45.c +++ b/drivers/net/phy/phy-c45.c @@ -661,6 +661,55 @@ int genphy_c45_read_mdix(struct phy_device *phydev) } EXPORT_SYMBOL_GPL(genphy_c45_read_mdix); +/** + * genphy_c45_read_eee_abilities - read supported EEE link modes + * @phydev: target phy_device struct + * + * Read supported EEE link modes. + */ +int genphy_c45_read_eee_abilities(struct phy_device *phydev) +{ + __ETHTOOL_DECLARE_LINK_MODE_MASK(common); + int val; + + linkmode_and(common, phydev->supported, PHY_EEE_100_10000_FEATURES); + /* There is not indicator if optional register + * "EEE control and capability 1" (3.20) is supported. Read it only + * on devices with appropriate linkmodes. + */ + if (!linkmode_empty(common)) { + /* IEEE 802.3-2018 45.2.3.10 EEE control and capability 1 + * (Register 3.20) + */ + val = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_EEE_ABLE); + if (val < 0) + return val; + + mii_eee_100_10000_adv_mod_linkmode_t(phydev->supported_eee, val); + + /* Some buggy devices claim not supported EEE link modes */ + linkmode_and(phydev->supported_eee, phydev->supported_eee, + phydev->supported); + } + + if (linkmode_test_bit(ETHTOOL_LINK_MODE_10baseT1L_Full_BIT, + phydev->supported)) { + /* IEEE 802.3cg-2019 45.2.1.186b 10BASE-T1L PMA status register + * (Register 1.2295) + */ + val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_10T1L_STAT); + if (val < 0) + return val; + + linkmode_mod_bit(ETHTOOL_LINK_MODE_10baseT1L_Full_BIT, + phydev->supported_eee, + val & MDIO_PMA_10T1L_STAT_EEE); + } + + return 0; +} +EXPORT_SYMBOL_GPL(genphy_c45_read_eee_abilities); + /** * genphy_c45_pma_read_abilities - read supported link modes from PMA * @phydev: target phy_device struct diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 9ba8f973f26f..3651f1fd8fc9 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -132,6 +132,18 @@ static const int phy_10gbit_full_features_array[] = { ETHTOOL_LINK_MODE_10000baseT_Full_BIT, }; +static const int phy_eee_100_10000_features_array[6] = { + ETHTOOL_LINK_MODE_100baseT_Full_BIT, + ETHTOOL_LINK_MODE_1000baseT_Full_BIT, + ETHTOOL_LINK_MODE_10000baseT_Full_BIT, + ETHTOOL_LINK_MODE_1000baseKX_Full_BIT, + ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT, + ETHTOOL_LINK_MODE_10000baseKR_Full_BIT, +}; + +__ETHTOOL_DECLARE_LINK_MODE_MASK(phy_eee_100_10000_features) __ro_after_init; +EXPORT_SYMBOL_GPL(phy_eee_100_10000_features); + static void features_init(void) { /* 10/100 half/full*/ @@ -213,6 +225,10 @@ static void features_init(void) linkmode_set_bit_array(phy_10gbit_fec_features_array, ARRAY_SIZE(phy_10gbit_fec_features_array), phy_10gbit_fec_features); + linkmode_set_bit_array(phy_eee_100_10000_features_array, + ARRAY_SIZE(phy_eee_100_10000_features_array), + phy_eee_100_10000_features); + } void phy_device_free(struct phy_device *phydev) diff --git a/include/linux/mdio.h b/include/linux/mdio.h index c0da30d63b1d..77c324f89b66 100644 --- a/include/linux/mdio.h +++ b/include/linux/mdio.h @@ -402,6 +402,23 @@ static inline u32 linkmode_adv_to_mii_t1_adv_m_t(unsigned long *advertising) return result; } +static inline void mii_eee_100_10000_adv_mod_linkmode_t(unsigned long *adv, + u32 val) +{ + linkmode_mod_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, + adv, val & MDIO_EEE_100TX); + linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, + adv, val & MDIO_EEE_1000T); + linkmode_mod_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, + adv, val & MDIO_EEE_10GT); + linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseKX_Full_BIT, + adv, val & MDIO_EEE_1000KX); + linkmode_mod_bit(ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT, + adv, val & MDIO_EEE_10GKX4); + linkmode_mod_bit(ETHTOOL_LINK_MODE_10000baseKR_Full_BIT, + adv, val & MDIO_EEE_10GKR); +} + int __mdiobus_read(struct mii_bus *bus, int addr, u32 regnum); int __mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val); int __mdiobus_modify_changed(struct mii_bus *bus, int addr, u32 regnum, diff --git a/include/linux/phy.h b/include/linux/phy.h index fbeba4fee8d4..567810f71fb6 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -52,6 +52,7 @@ extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_gbit_all_ports_features) __ro_after_ extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_10gbit_features) __ro_after_init; extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_10gbit_fec_features) __ro_after_init; extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_10gbit_full_features) __ro_after_init; +extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_eee_100_10000_features) __ro_after_init; #define PHY_BASIC_FEATURES ((unsigned long *)&phy_basic_features) #define PHY_BASIC_T1_FEATURES ((unsigned long *)&phy_basic_t1_features) @@ -62,6 +63,7 @@ extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_10gbit_full_features) __ro_after_ini #define PHY_10GBIT_FEATURES ((unsigned long *)&phy_10gbit_features) #define PHY_10GBIT_FEC_FEATURES ((unsigned long *)&phy_10gbit_fec_features) #define PHY_10GBIT_FULL_FEATURES ((unsigned long *)&phy_10gbit_full_features) +#define PHY_EEE_100_10000_FEATURES ((unsigned long *)&phy_eee_100_10000_features) extern const int phy_basic_ports_array[3]; extern const int phy_fibre_port_array[1]; @@ -676,6 +678,8 @@ struct phy_device { __ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising); /* used with phy_speed_down */ __ETHTOOL_DECLARE_LINK_MODE_MASK(adv_old); + /* used for eee validation */ + __ETHTOOL_DECLARE_LINK_MODE_MASK(supported_eee); /* Host supported PHY interface types. Should be ignored if empty. */ DECLARE_PHY_INTERFACE_MASK(host_interfaces); @@ -1737,6 +1741,7 @@ int genphy_c45_an_config_aneg(struct phy_device *phydev); int genphy_c45_an_disable_aneg(struct phy_device *phydev); int genphy_c45_read_mdix(struct phy_device *phydev); int genphy_c45_pma_read_abilities(struct phy_device *phydev); +int genphy_c45_read_eee_abilities(struct phy_device *phydev); int genphy_c45_pma_baset1_read_master_slave(struct phy_device *phydev); int genphy_c45_read_status(struct phy_device *phydev); int genphy_c45_baset1_read_status(struct phy_device *phydev);
Add generic function for EEE abilities defined by IEEE 802.3 specification. For now following registers are supported: - IEEE 802.3-2018 45.2.3.10 EEE control and capability 1 (Register 3.20) - IEEE 802.3cg-2019 45.2.1.186b 10BASE-T1L PMA status register (Register 1.2295) Since I was not able to find any flag signaling support of this registers, we should detect link mode abilities first and then based on this abilities doing EEE link modes detection. Results of EEE ability detection will be stored in to new variable phydev->supported_eee. Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- drivers/net/phy/phy-c45.c | 49 ++++++++++++++++++++++++++++++++++++ drivers/net/phy/phy_device.c | 16 ++++++++++++ include/linux/mdio.h | 17 +++++++++++++ include/linux/phy.h | 5 ++++ 4 files changed, 87 insertions(+)