| Message ID | e463bc66-c684-4847-b865-1f59dbadee7e@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: bcmgenet: use genphy_c45_eee_is_active directly, instead of phy_init_eee | expand |
On Tue, Mar 11, 2025 at 07:43:10AM +0100, Heiner Kallweit wrote: > Use genphy_c45_eee_is_active directly instead of phy_init_eee, > this prepares for removing phy_init_eee. With the second > argument being Null, phy_init_eee doesn't initialize anything. bcmgenet_set_eee() is an example where EEE is done wrong. EEE is negotiated, so you cannot configure the MAC until autoneg has completed. So phy_init_eee() should only be called from the adjust link callback. In this driver, it is only being called in the ethtool .set_ee op, which is wrong. So all this patch does is replace one broken things with another broken thing. Please consider my old patch: https://github.com/lunn/linux/commit/c226f4dbe9aa9c51c1308561aba64c722dab04fb It will need updating, but this is how i think this should be solved. Andrew
On 3/11/2025 5:58 AM, Andrew Lunn wrote: > On Tue, Mar 11, 2025 at 07:43:10AM +0100, Heiner Kallweit wrote: >> Use genphy_c45_eee_is_active directly instead of phy_init_eee, >> this prepares for removing phy_init_eee. With the second >> argument being Null, phy_init_eee doesn't initialize anything. > > bcmgenet_set_eee() is an example where EEE is done wrong. EEE is > negotiated, so you cannot configure the MAC until autoneg has > completed. So phy_init_eee() should only be called from the adjust > link callback. In this driver, it is only being called in the ethtool > .set_ee op, which is wrong. So all this patch does is replace one > broken things with another broken thing. > > Please consider my old patch: > > https://github.com/lunn/linux/commit/c226f4dbe9aa9c51c1308561aba64c722dab04fb > > It will need updating, but this is how i think this should be solved. Your old patch was tested and had regressions, and I did not see a follow up to it. The Raspberry Pi 4 systems use this Ethernet controller and the PHY does have EEE available, so it should not be too hard to get a platform to test on.
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c index 3e93f9574..c953559e3 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c @@ -1350,7 +1350,8 @@ static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_keee *e) if (!p->eee_enabled) { bcmgenet_eee_enable_set(dev, false, false); } else { - active = phy_init_eee(dev->phydev, false) >= 0; + active = dev->phydev->drv && + genphy_c45_eee_is_active(dev->phydev, NULL) > 0; bcmgenet_umac_writel(priv, e->tx_lpi_timer, UMAC_EEE_LPI_TIMER); bcmgenet_eee_enable_set(dev, active, e->tx_lpi_enabled); } diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c index c4a3698ce..5aa8e16fe 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c @@ -91,7 +91,7 @@ static void bcmgenet_mac_config(struct net_device *dev) bcmgenet_umac_writel(priv, reg, UMAC_CMD); spin_unlock_bh(&priv->reg_lock); - active = phy_init_eee(phydev, 0) >= 0; + active = phydev->drv && genphy_c45_eee_is_active(phydev, NULL) > 0; bcmgenet_eee_enable_set(dev, priv->eee.eee_enabled && active, priv->eee.tx_lpi_enabled);
Use genphy_c45_eee_is_active directly instead of phy_init_eee, this prepares for removing phy_init_eee. With the second argument being Null, phy_init_eee doesn't initialize anything. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/ethernet/broadcom/genet/bcmgenet.c | 3 ++- drivers/net/ethernet/broadcom/genet/bcmmii.c | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-)