| Message ID | 20230217034230.1249661-2-andrew@lunn.ch (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Rework MAC drivers EEE support | expand |
On Fri, Feb 17, 2023 at 04:42:13AM +0100, Andrew Lunn wrote: > MAC drivers which support EEE need to know the results of the EEE > auto-neg in order to program the hardware to perform EEE or not. The > oddly named phy_init_eee() can be used to determine this, it returns 0 > if EEE should be used, or a negative error code, > e.g. -EOPPROTONOTSUPPORT if the PHY does not support EEE or negotiate > resulted in it not being used. > > However, many MAC drivers get this wrong. Add phydev->eee_active which > indicates the result of the autoneg for EEE, including if EEE is > administratively disabled with ethtool. The MAC driver can then access > this in the same way as link speed and duplex in the adjust link > callback. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > --- > drivers/net/phy/phy.c | 3 +++ > include/linux/phy.h | 2 ++ > 2 files changed, 5 insertions(+) > > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > index b33e55a7364e..1e6df250d0d0 100644 > --- a/drivers/net/phy/phy.c > +++ b/drivers/net/phy/phy.c > @@ -916,9 +916,12 @@ static int phy_check_link_status(struct phy_device *phydev) > if (phydev->link && phydev->state != PHY_RUNNING) { > phy_check_downshift(phydev); > phydev->state = PHY_RUNNING; > + phydev->eee_active = genphy_c45_eee_is_active(phydev, > + NULL, NULL, NULL); genphy_c45_eee_is_active() may return an error.
On Fri, Feb 17, 2023 at 04:42:13AM +0100, Andrew Lunn wrote: > MAC drivers which support EEE need to know the results of the EEE > auto-neg in order to program the hardware to perform EEE or not. The > oddly named phy_init_eee() can be used to determine this, it returns 0 > if EEE should be used, or a negative error code, > e.g. -EOPPROTONOTSUPPORT if the PHY does not support EEE or negotiate > resulted in it not being used. > > However, many MAC drivers get this wrong. Add phydev->eee_active which > indicates the result of the autoneg for EEE, including if EEE is > administratively disabled with ethtool. The MAC driver can then access > this in the same way as link speed and duplex in the adjust link > callback. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> Yay! Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Thanks!
On Fri, Feb 17, 2023 at 10:09:19AM +0100, Oleksij Rempel wrote: > On Fri, Feb 17, 2023 at 04:42:13AM +0100, Andrew Lunn wrote: > > MAC drivers which support EEE need to know the results of the EEE > > auto-neg in order to program the hardware to perform EEE or not. The > > oddly named phy_init_eee() can be used to determine this, it returns 0 > > if EEE should be used, or a negative error code, > > e.g. -EOPPROTONOTSUPPORT if the PHY does not support EEE or negotiate > > resulted in it not being used. > > > > However, many MAC drivers get this wrong. Add phydev->eee_active which > > indicates the result of the autoneg for EEE, including if EEE is > > administratively disabled with ethtool. The MAC driver can then access > > this in the same way as link speed and duplex in the adjust link > > callback. > > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > --- > > drivers/net/phy/phy.c | 3 +++ > > include/linux/phy.h | 2 ++ > > 2 files changed, 5 insertions(+) > > > > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > > index b33e55a7364e..1e6df250d0d0 100644 > > --- a/drivers/net/phy/phy.c > > +++ b/drivers/net/phy/phy.c > > @@ -916,9 +916,12 @@ static int phy_check_link_status(struct phy_device *phydev) > > if (phydev->link && phydev->state != PHY_RUNNING) { > > phy_check_downshift(phydev); > > phydev->state = PHY_RUNNING; > > + phydev->eee_active = genphy_c45_eee_is_active(phydev, > > + NULL, NULL, NULL); > > genphy_c45_eee_is_active() may return an error. Yep. So we want eee_active false on error. Thanks Andrew
On Fri, Feb 17, 2023 at 11:36:31AM +0000, Russell King (Oracle) wrote: > On Fri, Feb 17, 2023 at 04:42:13AM +0100, Andrew Lunn wrote: > > MAC drivers which support EEE need to know the results of the EEE > > auto-neg in order to program the hardware to perform EEE or not. The > > oddly named phy_init_eee() can be used to determine this, it returns 0 > > if EEE should be used, or a negative error code, > > e.g. -EOPPROTONOTSUPPORT if the PHY does not support EEE or negotiate > > resulted in it not being used. > > > > However, many MAC drivers get this wrong. Add phydev->eee_active which > > indicates the result of the autoneg for EEE, including if EEE is > > administratively disabled with ethtool. The MAC driver can then access > > this in the same way as link speed and duplex in the adjust link > > callback. > > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > Yay! > > Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Hi Russell I did wonder about making this a parameter for phylink mac up callback, rather than having the driver look inside phydev. It is something which can be done later. Andrew
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index b33e55a7364e..1e6df250d0d0 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -916,9 +916,12 @@ static int phy_check_link_status(struct phy_device *phydev) if (phydev->link && phydev->state != PHY_RUNNING) { phy_check_downshift(phydev); phydev->state = PHY_RUNNING; + phydev->eee_active = genphy_c45_eee_is_active(phydev, + NULL, NULL, NULL); phy_link_up(phydev); } else if (!phydev->link && phydev->state != PHY_NOLINK) { phydev->state = PHY_NOLINK; + phydev->eee_active = false; phy_link_down(phydev); } diff --git a/include/linux/phy.h b/include/linux/phy.h index 727bff531a14..b1fbb52ac5a4 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -575,6 +575,7 @@ struct macsec_ops; * @advertising: Currently advertised linkmodes * @adv_old: Saved advertised while power saving for WoL * @supported_eee: supported PHY EEE linkmodes + * @eee_active: EEE is active for the current link mode * @lp_advertising: Current link partner advertised linkmodes * @host_interfaces: PHY interface modes supported by host * @eee_broken_modes: Energy efficient ethernet modes which should be prohibited @@ -687,6 +688,7 @@ struct phy_device { /* Energy efficient ethernet modes which should be prohibited */ u32 eee_broken_modes; + bool eee_active; #ifdef CONFIG_LED_TRIGGER_PHY struct phy_led_trigger *phy_led_triggers;
MAC drivers which support EEE need to know the results of the EEE auto-neg in order to program the hardware to perform EEE or not. The oddly named phy_init_eee() can be used to determine this, it returns 0 if EEE should be used, or a negative error code, e.g. -EOPPROTONOTSUPPORT if the PHY does not support EEE or negotiate resulted in it not being used. However, many MAC drivers get this wrong. Add phydev->eee_active which indicates the result of the autoneg for EEE, including if EEE is administratively disabled with ethtool. The MAC driver can then access this in the same way as link speed and duplex in the adjust link callback. Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- drivers/net/phy/phy.c | 3 +++ include/linux/phy.h | 2 ++ 2 files changed, 5 insertions(+)