Re: [PATCH net-next v4 02/23] net: phy: add genphy_c45_read_eee_abilities() function

[Vladimir Oltean <olteanv@gmail.com>]

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)