Re: [PATCH net v1 1/2] net: phy: set eee_cfg based on PHY configuration

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Thu, Nov 14, 2024 at 04:16:52PM +0800, Choong Yong Liang wrote:
> Not all PHYs have EEE enabled by default. For example, Marvell PHYs are
> designed to have EEE hardware disabled during the initial state, and it
> needs to be configured to turn it on again.
> 
> This patch reads the PHY configuration and sets it as the initial value for
> eee_cfg.tx_lpi_enabled and eee_cfg.eee_enabled instead of having them set to
> true by default.

eee_cfg.tx_lpi_enabled is something phylib tracks, and it merely means
that LPI needs to be enabled at the MAC if EEE was negotiated:

 * @tx_lpi_enabled: Whether the interface should assert its tx lpi, given
 *      that eee was negotiated.

eee_cfg.eee_enabled means that EEE mode was enabled - which is user
configuration:

 * @eee_enabled: EEE configured mode (enabled/disabled).

phy_probe() reads the initial PHY state and sets things up
appropriately.

However, there is a point where the EEE configuration (advertisement,
and therefore eee_enabled state) is written to the PHY, and that should
be config_aneg(). Looking at the Marvell driver, it's calling
genphy_config_aneg() which eventually calls
genphy_c45_an_config_eee_aneg() which does this (via
__genphy_config_aneg()).

Please investigate why the hardware state is going out of sync with the
software state.

Thanks.

>  void phy_support_eee(struct phy_device *phydev)
>  {
> +	bool is_enabled = true;
> +
> +	genphy_c45_eee_is_active(phydev, NULL, NULL, &is_enabled);
>  	linkmode_copy(phydev->advertising_eee, phydev->supported_eee);
> -	phydev->eee_cfg.tx_lpi_enabled = true;
> -	phydev->eee_cfg.eee_enabled = true;
> +	phydev->eee_cfg.tx_lpi_enabled = is_enabled;
> +	phydev->eee_cfg.eee_enabled = is_enabled;

This is almost certainly incorrect, because eee_enabled should only
be set when phydev->advertising_eee (which should track the hardware
EEE advertisement programmed into the PHY) is non-zero.

Note that phy_support_eee() must be called _before_ phy_start(). I
haven't checked whether stmmac does this.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!