[PATCH net v1 1/2] net: phy: Introduce phy_update_eee() to update eee_cfg values
Russell King (Oracle)
linux at armlinux.org.uk
Thu Nov 14 02:15:42 PST 2024
On Thu, Nov 14, 2024 at 09:12:06AM +0000, Russell King (Oracle) wrote:
> On Thu, Nov 14, 2024 at 09:02:47AM +0000, Russell King (Oracle) wrote:
> > On Wed, Nov 13, 2024 at 06:10:55PM +0800, Choong Yong Liang wrote:
> > > On 12/11/2024 9:04 pm, Andrew Lunn wrote:
> > > > On Tue, Nov 12, 2024 at 12:03:15PM +0100, Heiner Kallweit wrote:
> > > > > In stmmac_ethtool_op_get_eee() you have the following:
> > > > >
> > > > > edata->tx_lpi_timer = priv->tx_lpi_timer;
> > > > > edata->tx_lpi_enabled = priv->tx_lpi_enabled;
> > > > > return phylink_ethtool_get_eee(priv->phylink, edata);
> > > > >
> > > > > You have to call phylink_ethtool_get_eee() first, otherwise the manually
> > > > > set values will be overridden. However setting tx_lpi_enabled shouldn't
> > > > > be needed if you respect phydev->enable_tx_lpi.
> > > >
> > > > I agree with Heiner here, this sounds like a bug somewhere, not
> > > > something which needs new code in phylib. Lets understand why it gives
> > > > the wrong results.
> > > >
> > > > Andrew
> > > Hi Russell, Andrew, and Heiner, thanks a lot for your valuable feedback.
> > >
> > > The current implementation of the 'ethtool --show-eee' command heavily
> > > relies on the phy_ethtool_get_eee() in phy.c. The eeecfg values are set by
> > > the 'ethtool --set-eee' command and the phy_support_eee() during the initial
> > > state. The phy_ethtool_get_eee() calls eeecfg_to_eee(), which returns the
> > > eeecfg containing tx_lpi_timer, tx_lpi_enabled, and eee_enable for the
> > > 'ethtool --show-eee' command.
> >
> > These three members you mention are user configuration members.
> >
> > > The tx_lpi_timer and tx_lpi_enabled values stored in the MAC or PHY driver
> > > are not retrieved by the 'ethtool --show-eee' command.
> >
> > tx_lpi_timer is the only thing that the MAC driver should be concerned
> > with - it needs to program the MAC according to the timer value
> > specified. Whether LPI is enabled or not is determined by
> > phydev->enable_tx_lpi. The MAC should be using nothing else.
> >
> > > Currently, we are facing 3 issues:
> > > 1. When we boot up our system and do not issue the 'ethtool --set-eee'
> > > command, and then directly issue the 'ethtool --show-eee' command, it always
> > > shows that EEE is disabled due to the eeecfg values not being set. However,
> > > in the Maxliner GPY PHY, the driver EEE is enabled.
> >
> > So the software state is out of sync with the hardware state. This is a
> > bug in the GPY PHY driver.
> >
> > If we look at the generic code, we can see that genphy_config_aneg()
> > calls __genphy_config_aneg() which then goes on to call
> > genphy_c45_an_config_eee_aneg(). genphy_c45_an_config_eee_aneg()
> > writes the current EEE configuration to the PHY.
> >
> > Now if we look at gpy_config_aneg(), it doesn't do this. Therefore,
> > the GPY PHY is retaining its hardware state which is different from
> > the software state. This is wrong.
>
> Also note that phy_probe() reads the current configuration from the
> PHY. The supported mask is set via phydev->drv->get_features,
> which calls genphy_c45_pma_read_abilities() via the GPY driver and
> genphy_c45_read_eee_abilities().
>
> phy_probe() then moved on to genphy_c45_read_eee_adv(), which reads
> the advertisement mask. If the advertising mask is non-zero, then
> EEE is set as enabled.
>
> From your description, it sounds like this isn't working right, and
> needs to be debugged. For example, is the PHY changing its EEE
> advertisement between phy_probe() and when it is up and running?
For the benefit of this thread - phylib definitely has a problem. It's
got one too many things called eee_enabled, leading to confusion about
which should be used. I've now built this patch, and am testing it
with a Marvell PHY.
Without a call to phy_support_eee():
EEE settings for eth2:
EEE status: disabled
Tx LPI: disabled
Supported EEE link modes: 100baseT/Full
1000baseT/Full
Advertised EEE link modes: Not reported
Link partner advertised EEE link modes: 100baseT/Full
1000baseT/Full
With a call to phy_support_eee():
EEE settings for eth2:
EEE status: enabled - active
Tx LPI: 0 (us)
Supported EEE link modes: 100baseT/Full
1000baseT/Full
Advertised EEE link modes: 100baseT/Full
1000baseT/Full
Link partner advertised EEE link modes: 100baseT/Full
1000baseT/Full
So the EEE status is now behaving correctly, and the Marvell PHY is
being programmed with the advertisement correctly.
diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index c1b3576c307f..2d64d3f293e5 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -943,7 +943,7 @@ EXPORT_SYMBOL_GPL(genphy_c45_read_eee_abilities);
*/
int genphy_c45_an_config_eee_aneg(struct phy_device *phydev)
{
- if (!phydev->eee_enabled) {
+ if (!phydev->eee_cfg.eee_enabled) {
__ETHTOOL_DECLARE_LINK_MODE_MASK(adv) = {};
return genphy_c45_write_eee_adv(phydev, adv);
@@ -1576,8 +1576,6 @@ int genphy_c45_ethtool_set_eee(struct phy_device *phydev,
}
}
- phydev->eee_enabled = data->eee_enabled;
-
ret = genphy_c45_an_config_eee_aneg(phydev);
if (ret > 0) {
ret = phy_restart_aneg(phydev);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index bc24c9f2786b..b26bb33cd1d4 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3589,12 +3589,12 @@ static int phy_probe(struct device *dev)
/* There is no "enabled" flag. If PHY is advertising, assume it is
* kind of enabled.
*/
- phydev->eee_enabled = !linkmode_empty(phydev->advertising_eee);
+ phydev->eee_cfg.eee_enabled = !linkmode_empty(phydev->advertising_eee);
/* Some PHYs may advertise, by default, not support EEE modes. So,
* we need to clean them.
*/
- if (phydev->eee_enabled)
+ if (phydev->eee_cfg.eee_enabled)
linkmode_and(phydev->advertising_eee, phydev->supported_eee,
phydev->advertising_eee);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 1e4127c495c0..33905e9672a7 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -601,7 +601,6 @@ struct macsec_ops;
* @adv_old: Saved advertised while power saving for WoL
* @supported_eee: supported PHY EEE linkmodes
* @advertising_eee: Currently advertised EEE linkmodes
- * @eee_enabled: Flag indicating whether the EEE feature is enabled
* @enable_tx_lpi: When True, MAC should transmit LPI to PHY
* @eee_cfg: User configuration of EEE
* @lp_advertising: Current link partner advertised linkmodes
@@ -721,7 +720,6 @@ struct phy_device {
/* used for eee validation and configuration*/
__ETHTOOL_DECLARE_LINK_MODE_MASK(supported_eee);
__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising_eee);
- bool eee_enabled;
/* Host supported PHY interface types. Should be ignored if empty. */
DECLARE_PHY_INTERFACE_MASK(host_interfaces);
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
More information about the linux-arm-kernel
mailing list