[PATCH RFC 00/18] Rework MAC drivers EEE support

[PATCH RFC 00/18] Rework MAC drivers EEE support

[Andrew Lunn]

phy_init_eee() is supposed to be called once auto-neg has been
completed to determine if EEE should be used with the current link
mode. The MAC hardware should then be configured to either enable or
disable EEE. Many drivers get this wrong, calling phy_init_eee() once,
or only in the ethtool set_eee callback.

This patchset changes the API, such that EEE becomes the same as other
parameters which are determined by auto-neg. As will speed and duplex,
active EEE is now indicated in the phydev structure, and the
adjust_link callbacks have been modified to act upon its value.

eee_set and eee_get have been simplified, given that the phylib
functions act upon most of the data in eee_set, and fill in most of
the information needed for eee_set.

Andrew Lunn (18):
  net: phy: Add phydev->eee_active to simplify adjust link callbacks
  net: phy: Add helper to set EEE Clock stop enable bit
  net: marvell: mvneta: Simplify EEE configuration
  net: stmmac: Drop usage of phy_init_eee()
  net: stmmac: Simplify ethtool get eee
  net: lan743x: Fixup EEE
  net: fec: Move fec_enet_eee_mode_set() and helper earlier
  net: FEC: Fixup EEE
  net: genet: Fixup EEE
  net: sxgdb: Fixup EEE
  net: dsa: mt7530: Swap to using phydev->eee_active
  net: dsa: mt7530: Call phylib for set_eee and get_eee
  net: dsa: b53: Swap to using phydev->eee_active
  net: dsa: b53: Call phylib for set_eee and get_eee
  net: phylink: Remove unused phylink_init_eee()
  net: phylink: Update comment about configuring EEE in mac_link_up()
  net: phy: remove unused phy_init_eee()
  net: usb: lan78xx: Fixup EEE

 drivers/net/dsa/b53/b53_common.c              | 11 ++-
 drivers/net/dsa/mt7530.c                      |  8 +-
 .../net/ethernet/broadcom/genet/bcmgenet.c    | 31 +++----
 .../net/ethernet/broadcom/genet/bcmgenet.h    |  1 +
 drivers/net/ethernet/broadcom/genet/bcmmii.c  |  1 +
 drivers/net/ethernet/freescale/fec_main.c     | 84 ++++++++-----------
 drivers/net/ethernet/marvell/mvneta.c         | 12 +--
 .../net/ethernet/microchip/lan743x_ethtool.c  | 20 -----
 drivers/net/ethernet/microchip/lan743x_main.c |  7 ++
 .../net/ethernet/samsung/sxgbe/sxgbe_common.h |  4 +-
 .../ethernet/samsung/sxgbe/sxgbe_ethtool.c    | 23 +----
 .../net/ethernet/samsung/sxgbe/sxgbe_main.c   | 37 +++-----
 .../ethernet/stmicro/stmmac/stmmac_ethtool.c  |  2 -
 .../net/ethernet/stmicro/stmmac/stmmac_main.c |  5 +-
 drivers/net/phy/phy.c                         | 36 +++-----
 drivers/net/phy/phylink.c                     | 18 ----
 drivers/net/usb/lan78xx.c                     | 36 ++++----
 include/linux/phy.h                           |  4 +-
 include/linux/phylink.h                       |  7 +-
 19 files changed, 121 insertions(+), 226 deletions(-)

-- 
2.39.1

[PATCH RFC 01/18] net: phy: Add phydev->eee_active to simplify adjust link callbacks

[Andrew Lunn]

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);
 		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;
-- 
2.39.1

Re: [PATCH RFC 01/18] net: phy: Add phydev->eee_active to simplify adjust link callbacks

[Oleksij Rempel]

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.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH RFC 01/18] net: phy: Add phydev->eee_active to simplify adjust link callbacks

[Andrew Lunn]

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

Re: [PATCH RFC 01/18] net: phy: Add phydev->eee_active to simplify adjust link callbacks

[Russell King (Oracle)]

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!

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

Re: [PATCH RFC 01/18] net: phy: Add phydev->eee_active to simplify adjust link callbacks

[Andrew Lunn]

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

[PATCH RFC 02/18] net: phy: Add helper to set EEE Clock stop enable bit

[Andrew Lunn]

The MAC driver can request that the PHY stops the clock during EEE
LPI. This has normally been does as part of phy_init_eee(), however
that function is overly complex and often wrongly used. Add a
standalone helper, to aid removing phy_init_eee().

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/phy.c | 19 +++++++++++++++++++
 include/linux/phy.h   |  1 +
 2 files changed, 20 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 1e6df250d0d0..b25e0946405b 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1475,6 +1475,25 @@ void phy_mac_interrupt(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_mac_interrupt);
 
+/**
+ * phy_eee_clk_stop_enable - Clock should stop during LIP
+ * @phydev: target phy_device struct
+ *
+ * Description: Program the MMD register 3.0 setting the "Clock stop enable"
+ * bit.
+ */
+int phy_eee_clk_stop_enable(struct phy_device *phydev)
+{
+	int ret;
+
+	mutex_lock(&phydev->lock);
+	ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1,
+			       MDIO_PCS_CTRL1_CLKSTOP_EN);
+	mutex_unlock(&phydev->lock);
+
+	return ret;
+}
+
 /**
  * phy_init_eee - init and check the EEE feature
  * @phydev: target phy_device struct
diff --git a/include/linux/phy.h b/include/linux/phy.h
index b1fbb52ac5a4..4f72ffcb8b02 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1840,6 +1840,7 @@ int phy_unregister_fixup_for_id(const char *bus_id);
 int phy_unregister_fixup_for_uid(u32 phy_uid, u32 phy_uid_mask);
 
 int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable);
+int phy_eee_clk_stop_enable(struct phy_device *phydev);
 int phy_get_eee_err(struct phy_device *phydev);
 int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_eee *data);
 int phy_ethtool_get_eee(struct phy_device *phydev, struct ethtool_eee *data);
-- 
2.39.1

Re: [PATCH RFC 02/18] net: phy: Add helper to set EEE Clock stop enable bit

[Andrew Lunn]

On Fri, Feb 17, 2023 at 04:42:14AM +0100, Andrew Lunn wrote:
> The MAC driver can request that the PHY stops the clock during EEE
> LPI. This has normally been does as part of phy_init_eee(), however
> that function is overly complex and often wrongly used. Add a
> standalone helper, to aid removing phy_init_eee().
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/net/phy/phy.c | 19 +++++++++++++++++++
>  include/linux/phy.h   |  1 +
>  2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 1e6df250d0d0..b25e0946405b 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -1475,6 +1475,25 @@ void phy_mac_interrupt(struct phy_device *phydev)
>  }
>  EXPORT_SYMBOL(phy_mac_interrupt);
>  
> +/**
> + * phy_eee_clk_stop_enable - Clock should stop during LIP
> + * @phydev: target phy_device struct
> + *
> + * Description: Program the MMD register 3.0 setting the "Clock stop enable"
> + * bit.
> + */
> +int phy_eee_clk_stop_enable(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	mutex_lock(&phydev->lock);
> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1,
> +			       MDIO_PCS_CTRL1_CLKSTOP_EN);
> +	mutex_unlock(&phydev->lock);
> +
> +	return ret;
> +}

There is a missing EXPORT_SYMBOL_GPL() here, so modular builds don't
work.

	Andrew

[PATCH RFC 03/18] net: marvell: mvneta: Simplify EEE configuration

[Andrew Lunn]

phylib already does most of the work. It will track eee_enabled and
eee_active and correctly set them in the ethtool_get_eee callback.

Replace the call to phy_init_eee() by looking at the
phydev->eee_active member.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/ethernet/marvell/mvneta.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 0e39d199ff06..519a08354442 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -536,8 +536,6 @@ struct mvneta_port {
 	struct mvneta_bm_pool *pool_short;
 	int bm_win_id;
 
-	bool eee_enabled;
-	bool eee_active;
 	bool tx_lpi_enabled;
 
 	u64 ethtool_stats[ARRAY_SIZE(mvneta_statistics)];
@@ -4170,7 +4168,6 @@ static void mvneta_mac_link_down(struct phylink_config *config,
 		mvreg_write(pp, MVNETA_GMAC_AUTONEG_CONFIG, val);
 	}
 
-	pp->eee_active = false;
 	mvneta_set_eee(pp, false);
 }
 
@@ -4221,10 +4218,8 @@ static void mvneta_mac_link_up(struct phylink_config *config,
 
 	mvneta_port_up(pp);
 
-	if (phy && pp->eee_enabled) {
-		pp->eee_active = phy_init_eee(phy, false) >= 0;
-		mvneta_set_eee(pp, pp->eee_active && pp->tx_lpi_enabled);
-	}
+	if (phy)
+		mvneta_set_eee(pp, phy->eee_active && pp->tx_lpi_enabled);
 }
 
 static const struct phylink_mac_ops mvneta_phylink_ops = {
@@ -5028,8 +5023,6 @@ static int mvneta_ethtool_get_eee(struct net_device *dev,
 
 	lpi_ctl0 = mvreg_read(pp, MVNETA_LPI_CTRL_0);
 
-	eee->eee_enabled = pp->eee_enabled;
-	eee->eee_active = pp->eee_active;
 	eee->tx_lpi_enabled = pp->tx_lpi_enabled;
 	eee->tx_lpi_timer = (lpi_ctl0) >> 8; // * scale;
 
@@ -5053,7 +5046,6 @@ static int mvneta_ethtool_set_eee(struct net_device *dev,
 	lpi_ctl0 |= eee->tx_lpi_timer << 8;
 	mvreg_write(pp, MVNETA_LPI_CTRL_0, lpi_ctl0);
 
-	pp->eee_enabled = eee->eee_enabled;
 	pp->tx_lpi_enabled = eee->tx_lpi_enabled;
 
 	mvneta_set_eee(pp, eee->tx_lpi_enabled && eee->eee_enabled);
-- 
2.39.1

Re: [PATCH RFC 03/18] net: marvell: mvneta: Simplify EEE configuration

[Russell King (Oracle)]

On Fri, Feb 17, 2023 at 04:42:15AM +0100, Andrew Lunn wrote:
> phylib already does most of the work. It will track eee_enabled and
> eee_active and correctly set them in the ethtool_get_eee callback.
> 
> Replace the call to phy_init_eee() by looking at the
> phydev->eee_active member.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

This is a very nice cleanup, and finally gets rid of that phy_init_eee()
in all those mac_link_up() implementations. Yay!

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks!

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

Re: [PATCH RFC 03/18] net: marvell: mvneta: Simplify EEE configuration

[Russell King (Oracle)]

On Fri, Feb 17, 2023 at 04:42:15AM +0100, Andrew Lunn wrote:
> @@ -4221,10 +4218,8 @@ static void mvneta_mac_link_up(struct phylink_config *config,
>  
>  	mvneta_port_up(pp);
>  
> -	if (phy && pp->eee_enabled) {
> -		pp->eee_active = phy_init_eee(phy, false) >= 0;
> -		mvneta_set_eee(pp, pp->eee_active && pp->tx_lpi_enabled);
> -	}
> +	if (phy)
> +		mvneta_set_eee(pp, phy->eee_active && pp->tx_lpi_enabled);

Thinking about this a bit more, I'm not convinced this is properly safe.
What protects phy->eee_active from changing here? The phydev mutex won't
be held at this point.

As I mentioned in my reply to the cover letter about passing a flag to
mac_link_up() for EEE status, this would mean phylink could save the
EEE active status just like it does with the other phydev parameters
in phylink_phy_change() (which is called under the phydev mutex).

That ensures that we grab all the phylib state under the phydev mutex
and then use that state when bringing the MAC side up without needing
to worry about phydev mutexes.

It's way more invasive, as it requires the mac_link_up() method
signature to change, but I think it would be a cleaner approach
overall.

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

Re: [PATCH RFC 03/18] net: marvell: mvneta: Simplify EEE configuration

[Russell King (Oracle)]

On Fri, Feb 17, 2023 at 11:59:39AM +0000, Russell King (Oracle) wrote:
> On Fri, Feb 17, 2023 at 04:42:15AM +0100, Andrew Lunn wrote:
> > @@ -4221,10 +4218,8 @@ static void mvneta_mac_link_up(struct phylink_config *config,
> >  
> >  	mvneta_port_up(pp);
> >  
> > -	if (phy && pp->eee_enabled) {
> > -		pp->eee_active = phy_init_eee(phy, false) >= 0;
> > -		mvneta_set_eee(pp, pp->eee_active && pp->tx_lpi_enabled);
> > -	}
> > +	if (phy)
> > +		mvneta_set_eee(pp, phy->eee_active && pp->tx_lpi_enabled);
> 
> Thinking about this a bit more, I'm not convinced this is properly safe.
> What protects phy->eee_active from changing here? The phydev mutex won't
> be held at this point.
> 
> As I mentioned in my reply to the cover letter about passing a flag to
> mac_link_up() for EEE status, this would mean phylink could save the
> EEE active status just like it does with the other phydev parameters
> in phylink_phy_change() (which is called under the phydev mutex).

I suppose another option would be to add a new method to
phylink_mac_ops:

	int (*mac_set_eee)(struct phylink_config *config, bool eee,
			   u32 tx_lpi_timer);

and phylink calls this just before mac_link_up() or after
phylink_ethtool_set_eee(). The eee flag would be the effective result
of phydev->eee_active && tx_lpi_enabled, possibly also && tx_lpi_timer
!= 0, since a zero tx_lpi_timer is rather meaningless, unless we
explicitly have phylink_ethtool_set_eee() reject it is invalid.

All that mac_set_eee() implementations should then need to do is to
program the LPI timer in the MAC hardware, and enable or disable it
according to the "eee" flag.

The down-side to another mac_ops method is having to add a wrapper in
net/dsa/port.c for it.

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

Re: [PATCH RFC 03/18] net: marvell: mvneta: Simplify EEE configuration

[Andrew Lunn]

On Fri, Feb 17, 2023 at 11:59:39AM +0000, Russell King (Oracle) wrote:
> On Fri, Feb 17, 2023 at 04:42:15AM +0100, Andrew Lunn wrote:
> > @@ -4221,10 +4218,8 @@ static void mvneta_mac_link_up(struct phylink_config *config,
> >  
> >  	mvneta_port_up(pp);
> >  
> > -	if (phy && pp->eee_enabled) {
> > -		pp->eee_active = phy_init_eee(phy, false) >= 0;
> > -		mvneta_set_eee(pp, pp->eee_active && pp->tx_lpi_enabled);
> > -	}
> > +	if (phy)
> > +		mvneta_set_eee(pp, phy->eee_active && pp->tx_lpi_enabled);
> 
> Thinking about this a bit more, I'm not convinced this is properly safe.
> What protects phy->eee_active from changing here? The phydev mutex won't
> be held at this point.

That is one of the differences between phylib and phylink. For phylib,
the adjust_link callback is performed while holding phydev lock. So it
is guaranteed to be consistent.

> It's way more invasive, as it requires the mac_link_up() method
> signature to change, but I think it would be a cleaner approach
> overall.

Yes, i think it has to happen. But i also think it will have a
beneficial side effect. Very few MAC drivers implement EEE. Having
this parameters will make it much more visible, which could lead to
more MAC drivers adding EEE support.

     Andrew

[PATCH RFC 04/18] net: stmmac: Drop usage of phy_init_eee()

[Andrew Lunn]

Replace this method by looking at the eee_active member of the phydev
structure. Additionally, call the phy_eee_clk_stop_enable() if the
platform indicates the clock should be stopped while LPI is active.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index e4902a7bb61e..23ad855626ee 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1080,8 +1080,9 @@ static void stmmac_mac_link_up(struct phylink_config *config,
 
 	stmmac_mac_set(priv, priv->ioaddr, true);
 	if (phy && priv->dma_cap.eee) {
-		priv->eee_active =
-			phy_init_eee(phy, !priv->plat->rx_clk_runs_in_lpi) >= 0;
+		priv->eee_active = phy->eee_active;
+		if (!priv->plat->rx_clk_runs_in_lpi)
+			phy_eee_clk_stop_enable(phy);
 		priv->eee_enabled = stmmac_eee_init(priv);
 		priv->tx_lpi_enabled = priv->eee_enabled;
 		stmmac_set_eee_pls(priv, priv->hw, true);
-- 
2.39.1

[PATCH RFC 05/18] net: stmmac: Simplify ethtool get eee

[Andrew Lunn]

phylink_ethtool_get_eee() fills in eee_enabled and eee_active.  So
there is no need for the MAC driver to do it as well.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index 35c8dd92d369..efea97e640f4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -782,8 +782,6 @@ static int stmmac_ethtool_op_get_eee(struct net_device *dev,
 	if (!priv->dma_cap.eee)
 		return -EOPNOTSUPP;
 
-	edata->eee_enabled = priv->eee_enabled;
-	edata->eee_active = priv->eee_active;
 	edata->tx_lpi_timer = priv->tx_lpi_timer;
 	edata->tx_lpi_enabled = priv->tx_lpi_enabled;
 
-- 
2.39.1

[PATCH RFC 06/18] net: lan743x: Fixup EEE

[Andrew Lunn]

The enabling/disabling of EEE in the MAC should happen as a result of
auto negotiation. So move the enable/disable into
lan743x_phy_link_status_change() which gets called by phylib when
there is a change in link status.

lan743x_ethtool_set_eee() now just programs the hardware with the LTI
timer value, and passed everything else to phylib, so it can correctly
setup the PHY.

lan743x_ethtool_get_eee() relies on phylib doing most of the work, the
MAC driver just adds the LTI timer value, and tx_lpi_enabled based on
if EEE is active.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 .../net/ethernet/microchip/lan743x_ethtool.c  | 20 -------------------
 drivers/net/ethernet/microchip/lan743x_main.c |  7 +++++++
 2 files changed, 7 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c
index 2db5949b4c7e..044d67a564e0 100644
--- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
+++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
@@ -1073,15 +1073,11 @@ static int lan743x_ethtool_get_eee(struct net_device *netdev,
 
 	buf = lan743x_csr_read(adapter, MAC_CR);
 	if (buf & MAC_CR_EEE_EN_) {
-		eee->eee_enabled = true;
-		eee->eee_active = !!(eee->advertised & eee->lp_advertised);
 		eee->tx_lpi_enabled = true;
 		/* EEE_TX_LPI_REQ_DLY & tx_lpi_timer are same uSec unit */
 		buf = lan743x_csr_read(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT);
 		eee->tx_lpi_timer = buf;
 	} else {
-		eee->eee_enabled = false;
-		eee->eee_active = false;
 		eee->tx_lpi_enabled = false;
 		eee->tx_lpi_timer = 0;
 	}
@@ -1095,7 +1091,6 @@ static int lan743x_ethtool_set_eee(struct net_device *netdev,
 	struct lan743x_adapter *adapter;
 	struct phy_device *phydev;
 	u32 buf = 0;
-	int ret = 0;
 
 	if (!netdev)
 		return -EINVAL;
@@ -1112,23 +1107,8 @@ static int lan743x_ethtool_set_eee(struct net_device *netdev,
 	}
 
 	if (eee->eee_enabled) {
-		ret = phy_init_eee(phydev, false);
-		if (ret) {
-			netif_err(adapter, drv, adapter->netdev,
-				  "EEE initialization failed\n");
-			return ret;
-		}
-
 		buf = (u32)eee->tx_lpi_timer;
 		lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT, buf);
-
-		buf = lan743x_csr_read(adapter, MAC_CR);
-		buf |= MAC_CR_EEE_EN_;
-		lan743x_csr_write(adapter, MAC_CR, buf);
-	} else {
-		buf = lan743x_csr_read(adapter, MAC_CR);
-		buf &= ~MAC_CR_EEE_EN_;
-		lan743x_csr_write(adapter, MAC_CR, buf);
 	}
 
 	return phy_ethtool_set_eee(phydev, eee);
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index 7e0871b631e4..803e83880887 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -1457,6 +1457,13 @@ static void lan743x_phy_link_status_change(struct net_device *netdev)
 		    phydev->interface == PHY_INTERFACE_MODE_1000BASEX ||
 		    phydev->interface == PHY_INTERFACE_MODE_2500BASEX)
 			lan743x_sgmii_config(adapter);
+
+		data = lan743x_csr_read(adapter, MAC_CR);
+		if (phydev->eee_active)
+			data |=  MAC_CR_EEE_EN_;
+		else
+			data &= ~MAC_CR_EEE_EN_;
+		lan743x_csr_write(adapter, MAC_CR, data);
 	}
 }
 
-- 
2.39.1

[PATCH RFC 07/18] net: fec: Move fec_enet_eee_mode_set() and helper earlier

[Andrew Lunn]

FEC is about to get its EEE code re-written. To allow this, move
fec_enet_eee_mode_set() before fec_enet_adjust_link() which will
need to call it.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/ethernet/freescale/fec_main.c | 79 ++++++++++++-----------
 1 file changed, 40 insertions(+), 39 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index c73e25f8995e..195df75ee614 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1919,6 +1919,46 @@ static int fec_get_mac(struct net_device *ndev)
 /*
  * Phy section
  */
+
+/* LPI Sleep Ts count base on tx clk (clk_ref).
+ * The lpi sleep cnt value = X us / (cycle_ns).
+ */
+static int fec_enet_us_to_tx_cycle(struct net_device *ndev, int us)
+{
+	struct fec_enet_private *fep = netdev_priv(ndev);
+
+	return us * (fep->clk_ref_rate / 1000) / 1000;
+}
+
+static int fec_enet_eee_mode_set(struct net_device *ndev, bool enable)
+{
+	struct fec_enet_private *fep = netdev_priv(ndev);
+	struct ethtool_eee *p = &fep->eee;
+	unsigned int sleep_cycle, wake_cycle;
+	int ret = 0;
+
+	if (enable) {
+		ret = phy_init_eee(ndev->phydev, false);
+		if (ret)
+			return ret;
+
+		sleep_cycle = fec_enet_us_to_tx_cycle(ndev, p->tx_lpi_timer);
+		wake_cycle = sleep_cycle;
+	} else {
+		sleep_cycle = 0;
+		wake_cycle = 0;
+	}
+
+	p->tx_lpi_enabled = enable;
+	p->eee_enabled = enable;
+	p->eee_active = enable;
+
+	writel(sleep_cycle, fep->hwp + FEC_LPI_SLEEP);
+	writel(wake_cycle, fep->hwp + FEC_LPI_WAKE);
+
+	return 0;
+}
+
 static void fec_enet_adjust_link(struct net_device *ndev)
 {
 	struct fec_enet_private *fep = netdev_priv(ndev);
@@ -3057,45 +3097,6 @@ static int fec_enet_set_tunable(struct net_device *netdev,
 	return ret;
 }
 
-/* LPI Sleep Ts count base on tx clk (clk_ref).
- * The lpi sleep cnt value = X us / (cycle_ns).
- */
-static int fec_enet_us_to_tx_cycle(struct net_device *ndev, int us)
-{
-	struct fec_enet_private *fep = netdev_priv(ndev);
-
-	return us * (fep->clk_ref_rate / 1000) / 1000;
-}
-
-static int fec_enet_eee_mode_set(struct net_device *ndev, bool enable)
-{
-	struct fec_enet_private *fep = netdev_priv(ndev);
-	struct ethtool_eee *p = &fep->eee;
-	unsigned int sleep_cycle, wake_cycle;
-	int ret = 0;
-
-	if (enable) {
-		ret = phy_init_eee(ndev->phydev, false);
-		if (ret)
-			return ret;
-
-		sleep_cycle = fec_enet_us_to_tx_cycle(ndev, p->tx_lpi_timer);
-		wake_cycle = sleep_cycle;
-	} else {
-		sleep_cycle = 0;
-		wake_cycle = 0;
-	}
-
-	p->tx_lpi_enabled = enable;
-	p->eee_enabled = enable;
-	p->eee_active = enable;
-
-	writel(sleep_cycle, fep->hwp + FEC_LPI_SLEEP);
-	writel(wake_cycle, fep->hwp + FEC_LPI_WAKE);
-
-	return 0;
-}
-
 static int
 fec_enet_get_eee(struct net_device *ndev, struct ethtool_eee *edata)
 {
-- 
2.39.1

[PATCH RFC 08/18] net: FEC: Fixup EEE

[Andrew Lunn]

The enabling/disabling of EEE in the MAC should happen as a result of
auto negotiation. So move the enable/disable into
fec_enet_adjust_link() which gets called by phylib when there is a
change in link status.

fec_enet_set_eee() now just stores away the LTI timer value and if TX
LPI should be enabled. Everything else is passed to phylib, so it can
correctly setup the PHY.

fec_enet_get_eee() relies on phylib doing most of the work,
the MAC driver just adds the LTI timer value and the stored tx_lpi_enabled.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/ethernet/freescale/fec_main.c | 27 ++++-------------------
 1 file changed, 4 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 195df75ee614..5aca705876fe 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1930,18 +1930,13 @@ static int fec_enet_us_to_tx_cycle(struct net_device *ndev, int us)
 	return us * (fep->clk_ref_rate / 1000) / 1000;
 }
 
-static int fec_enet_eee_mode_set(struct net_device *ndev, bool enable)
+static int fec_enet_eee_mode_set(struct net_device *ndev, bool eee_active)
 {
 	struct fec_enet_private *fep = netdev_priv(ndev);
 	struct ethtool_eee *p = &fep->eee;
 	unsigned int sleep_cycle, wake_cycle;
-	int ret = 0;
-
-	if (enable) {
-		ret = phy_init_eee(ndev->phydev, false);
-		if (ret)
-			return ret;
 
+	if (eee_active && p->tx_lpi_enabled) {
 		sleep_cycle = fec_enet_us_to_tx_cycle(ndev, p->tx_lpi_timer);
 		wake_cycle = sleep_cycle;
 	} else {
@@ -1949,10 +1944,6 @@ static int fec_enet_eee_mode_set(struct net_device *ndev, bool enable)
 		wake_cycle = 0;
 	}
 
-	p->tx_lpi_enabled = enable;
-	p->eee_enabled = enable;
-	p->eee_active = enable;
-
 	writel(sleep_cycle, fep->hwp + FEC_LPI_SLEEP);
 	writel(wake_cycle, fep->hwp + FEC_LPI_WAKE);
 
@@ -1997,6 +1988,7 @@ static void fec_enet_adjust_link(struct net_device *ndev)
 			netif_tx_unlock_bh(ndev);
 			napi_enable(&fep->napi);
 		}
+		fec_enet_eee_mode_set(ndev, phy_dev->eee_active);
 	} else {
 		if (fep->link) {
 			napi_disable(&fep->napi);
@@ -3109,8 +3101,6 @@ fec_enet_get_eee(struct net_device *ndev, struct ethtool_eee *edata)
 	if (!netif_running(ndev))
 		return -ENETDOWN;
 
-	edata->eee_enabled = p->eee_enabled;
-	edata->eee_active = p->eee_active;
 	edata->tx_lpi_timer = p->tx_lpi_timer;
 	edata->tx_lpi_enabled = p->tx_lpi_enabled;
 
@@ -3122,7 +3112,6 @@ fec_enet_set_eee(struct net_device *ndev, struct ethtool_eee *edata)
 {
 	struct fec_enet_private *fep = netdev_priv(ndev);
 	struct ethtool_eee *p = &fep->eee;
-	int ret = 0;
 
 	if (!(fep->quirks & FEC_QUIRK_HAS_EEE))
 		return -EOPNOTSUPP;
@@ -3131,15 +3120,7 @@ fec_enet_set_eee(struct net_device *ndev, struct ethtool_eee *edata)
 		return -ENETDOWN;
 
 	p->tx_lpi_timer = edata->tx_lpi_timer;
-
-	if (!edata->eee_enabled || !edata->tx_lpi_enabled ||
-	    !edata->tx_lpi_timer)
-		ret = fec_enet_eee_mode_set(ndev, false);
-	else
-		ret = fec_enet_eee_mode_set(ndev, true);
-
-	if (ret)
-		return ret;
+	p->tx_lpi_enabled = edata->tx_lpi_enabled;
 
 	return phy_ethtool_set_eee(ndev->phydev, edata);
 }
-- 
2.39.1

Re: [PATCH RFC 08/18] net: FEC: Fixup EEE

[Oleksij Rempel]

On Fri, Feb 17, 2023 at 04:42:20AM +0100, Andrew Lunn wrote:
> The enabling/disabling of EEE in the MAC should happen as a result of
> auto negotiation. So move the enable/disable into
> fec_enet_adjust_link() which gets called by phylib when there is a
> change in link status.
> 
> fec_enet_set_eee() now just stores away the LTI timer value and if TX
> LPI should be enabled. Everything else is passed to phylib, so it can
> correctly setup the PHY.
> 
> fec_enet_get_eee() relies on phylib doing most of the work,
> the MAC driver just adds the LTI timer value and the stored tx_lpi_enabled.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 27 ++++-------------------
>  1 file changed, 4 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 195df75ee614..5aca705876fe 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1930,18 +1930,13 @@ static int fec_enet_us_to_tx_cycle(struct net_device *ndev, int us)
>  	return us * (fep->clk_ref_rate / 1000) / 1000;
>  }
>  
> -static int fec_enet_eee_mode_set(struct net_device *ndev, bool enable)
> +static int fec_enet_eee_mode_set(struct net_device *ndev, bool eee_active)
>  {
>  	struct fec_enet_private *fep = netdev_priv(ndev);
>  	struct ethtool_eee *p = &fep->eee;
>  	unsigned int sleep_cycle, wake_cycle;
> -	int ret = 0;
> -
> -	if (enable) {
> -		ret = phy_init_eee(ndev->phydev, false);
> -		if (ret)
> -			return ret;
>  
> +	if (eee_active && p->tx_lpi_enabled) {
>  		sleep_cycle = fec_enet_us_to_tx_cycle(ndev, p->tx_lpi_timer);
>  		wake_cycle = sleep_cycle;
>  	} else {
> @@ -1949,10 +1944,6 @@ static int fec_enet_eee_mode_set(struct net_device *ndev, bool enable)
>  		wake_cycle = 0;
>  	}
>  
> -	p->tx_lpi_enabled = enable;
> -	p->eee_enabled = enable;
> -	p->eee_active = enable;
> -
>  	writel(sleep_cycle, fep->hwp + FEC_LPI_SLEEP);
>  	writel(wake_cycle, fep->hwp + FEC_LPI_WAKE);
>  
> @@ -1997,6 +1988,7 @@ static void fec_enet_adjust_link(struct net_device *ndev)
>  			netif_tx_unlock_bh(ndev);
>  			napi_enable(&fep->napi);
>  		}
> +		fec_enet_eee_mode_set(ndev, phy_dev->eee_active);

Most of iMX variants do not support EEE. It should be something like this:
	if (fep->quirks & FEC_QUIRK_HAS_EEE)
		fec_enet_eee_mode_set(ndev, phy_dev->eee_active);

>  	} else {
>  		if (fep->link) {
>  			napi_disable(&fep->napi);
> @@ -3109,8 +3101,6 @@ fec_enet_get_eee(struct net_device *ndev, struct ethtool_eee *edata)
>  	if (!netif_running(ndev))
>  		return -ENETDOWN;
>  
> -	edata->eee_enabled = p->eee_enabled;
> -	edata->eee_active = p->eee_active;
>  	edata->tx_lpi_timer = p->tx_lpi_timer;
>  	edata->tx_lpi_enabled = p->tx_lpi_enabled;
>  
> @@ -3122,7 +3112,6 @@ fec_enet_set_eee(struct net_device *ndev, struct ethtool_eee *edata)
>  {
>  	struct fec_enet_private *fep = netdev_priv(ndev);
>  	struct ethtool_eee *p = &fep->eee;
> -	int ret = 0;
>  
>  	if (!(fep->quirks & FEC_QUIRK_HAS_EEE))
>  		return -EOPNOTSUPP;
> @@ -3131,15 +3120,7 @@ fec_enet_set_eee(struct net_device *ndev, struct ethtool_eee *edata)
>  		return -ENETDOWN;
>  
>  	p->tx_lpi_timer = edata->tx_lpi_timer;
> -
> -	if (!edata->eee_enabled || !edata->tx_lpi_enabled ||
> -	    !edata->tx_lpi_timer)
> -		ret = fec_enet_eee_mode_set(ndev, false);
> -	else
> -		ret = fec_enet_eee_mode_set(ndev, true);
> -
> -	if (ret)
> -		return ret;
> +	p->tx_lpi_enabled = edata->tx_lpi_enabled;

Hm.. this change have effect only after link restart. Should we do
something like this?

	if (phydev->link)
		fec_enet_eee_mode_set(ndev, phydev->eee_active);

or, execute phy_ethtool_set_eee() first and some how detect if link
changed? Or restart link by phylib on every change?

>  
>  	return phy_ethtool_set_eee(ndev->phydev, edata);
>  }
> -- 
> 2.39.1
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH RFC 08/18] net: FEC: Fixup EEE

[Russell King (Oracle)]

On Fri, Feb 17, 2023 at 09:19:43AM +0100, Oleksij Rempel wrote:
> On Fri, Feb 17, 2023 at 04:42:20AM +0100, Andrew Lunn wrote:
> > @@ -3131,15 +3120,7 @@ fec_enet_set_eee(struct net_device *ndev, struct ethtool_eee *edata)
> >  		return -ENETDOWN;
> >  
> >  	p->tx_lpi_timer = edata->tx_lpi_timer;
> > -
> > -	if (!edata->eee_enabled || !edata->tx_lpi_enabled ||
> > -	    !edata->tx_lpi_timer)
> > -		ret = fec_enet_eee_mode_set(ndev, false);
> > -	else
> > -		ret = fec_enet_eee_mode_set(ndev, true);
> > -
> > -	if (ret)
> > -		return ret;
> > +	p->tx_lpi_enabled = edata->tx_lpi_enabled;
> 
> Hm.. this change have effect only after link restart. Should we do
> something like this?
> 
> 	if (phydev->link)
> 		fec_enet_eee_mode_set(ndev, phydev->eee_active);
> 
> or, execute phy_ethtool_set_eee() first and some how detect if link
> changed? Or restart link by phylib on every change?

Restarting the link on every "change" (even when nothing has changed)
is a dirty hack, and can be very annoying, leading to multiple link
restarts e.g. at boot time which can turn into several seconds of
boot delay depending on how much is configured.

I would suggest as a general principle, we should be keeping link
renegotiations to a minimum - and phylib already tries to do that in
several places.

Also note that reading phydev->eee_active without holding the phydev
mutex can be racy - and I would also ask what would prevent two calls
to fec_enet_eee_mode_set() running concurrently, once by the
adjust_link callback and once via the set_eee method. This applies
to reading phydev->link as well, so it may be best to hold the
phydev mutex around that entire if() clause.

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

Re: [PATCH RFC 08/18] net: FEC: Fixup EEE

[Andrew Lunn]

> > @@ -1997,6 +1988,7 @@ static void fec_enet_adjust_link(struct net_device *ndev)
> >  			netif_tx_unlock_bh(ndev);
> >  			napi_enable(&fep->napi);
> >  		}
> > +		fec_enet_eee_mode_set(ndev, phy_dev->eee_active);
> 
> Most of iMX variants do not support EEE. It should be something like this:
> 	if (fep->quirks & FEC_QUIRK_HAS_EEE)
> 		fec_enet_eee_mode_set(ndev, phy_dev->eee_active);

Yes, i missed that. Thanks. 

> > @@ -3131,15 +3120,7 @@ fec_enet_set_eee(struct net_device *ndev, struct ethtool_eee *edata)
> >  		return -ENETDOWN;
> >  
> >  	p->tx_lpi_timer = edata->tx_lpi_timer;
> > -
> > -	if (!edata->eee_enabled || !edata->tx_lpi_enabled ||
> > -	    !edata->tx_lpi_timer)
> > -		ret = fec_enet_eee_mode_set(ndev, false);
> > -	else
> > -		ret = fec_enet_eee_mode_set(ndev, true);
> > -
> > -	if (ret)
> > -		return ret;
> > +	p->tx_lpi_enabled = edata->tx_lpi_enabled;
> 
> Hm.. this change have effect only after link restart. Should we do
> something like this?
> 
> 	if (phydev->link)
> 		fec_enet_eee_mode_set(ndev, phydev->eee_active);
> 
> or, execute phy_ethtool_set_eee() first and some how detect if link
> changed? Or restart link by phylib on every change?

The whole startup sequence needs looking at, and ties in with the
phy_supports_eee() call we need to add. Given that EEE is broken with
most MAC drivers, i thought we could do that in a follow up patch
series.

As Russell says, we want to avoid multiple auto-neg cycles. Ideally we
want phy_supports_eee() to be called before phy_start() so that EEE
advertisement is set correctly for the first auto-neg. What is missing
from many MAC drivers is a default value from the LPI timer. It seems
like the need eee_set() has to be called with a value, or they are
relying on hardware reset values. Maybe we need to define a default?

We also need to discuss policy of if EEE should be enabled by default
for those systems which support it. As you have pointed out, it can
effect PTP quality.

   Andrew

Re: [PATCH RFC 08/18] net: FEC: Fixup EEE

[Andrew Lunn]

> >  	p->tx_lpi_timer = edata->tx_lpi_timer;
> > -
> > -	if (!edata->eee_enabled || !edata->tx_lpi_enabled ||
> > -	    !edata->tx_lpi_timer)
> > -		ret = fec_enet_eee_mode_set(ndev, false);
> > -	else
> > -		ret = fec_enet_eee_mode_set(ndev, true);
> > -
> > -	if (ret)
> > -		return ret;
> > +	p->tx_lpi_enabled = edata->tx_lpi_enabled;
> 
> Hm.. this change have effect only after link restart. Should we do
> something like this?

I think moving tx_lpi_enabled into phylib will help here. phylib can
track if only this changes, and then call the adjust_link callback
without actually doing an auto neg is only that changes.

	Andrew

[PATCH RFC 09/18] net: genet: Fixup EEE

[Andrew Lunn]

The enabling/disabling of EEE in the MAC should happen as a result of
auto negotiation. So move the enable/disable into bcmgenet_mii_setup()
which gets called by phylib when there is a change in link status.

bcmgenet_set_eee() now just writes the LTI timer value to the hardware
and stores if TX LPI should be enabled. Everything else is passed to
phylib, so it can correctly setup the PHY.

bcmgenet_get_eee() relies on phylib doing most of the work, the MAC
driver just adds the LTI timer value from hardware and the stored
tx_lpi_enabled.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 .../net/ethernet/broadcom/genet/bcmgenet.c    | 31 ++++++-------------
 .../net/ethernet/broadcom/genet/bcmgenet.h    |  1 +
 drivers/net/ethernet/broadcom/genet/bcmmii.c  |  1 +
 3 files changed, 12 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index d937daa8ee88..2793d94ed32c 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1272,12 +1272,17 @@ static void bcmgenet_get_ethtool_stats(struct net_device *dev,
 	}
 }
 
-static void bcmgenet_eee_enable_set(struct net_device *dev, bool enable)
+void bcmgenet_eee_enable_set(struct net_device *dev, bool eee_active)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
-	u32 off = priv->hw_params->tbuf_offset + TBUF_ENERGY_CTRL;
+	struct ethtool_eee *p = &priv->eee;
+	bool enable;
+	u32 off;
 	u32 reg;
 
+	off = priv->hw_params->tbuf_offset + TBUF_ENERGY_CTRL;
+	enable = eee_active && p->tx_lpi_enabled;
+
 	if (enable && !priv->clk_eee_enabled) {
 		clk_prepare_enable(priv->clk_eee);
 		priv->clk_eee_enabled = true;
@@ -1310,9 +1315,6 @@ static void bcmgenet_eee_enable_set(struct net_device *dev, bool enable)
 		clk_disable_unprepare(priv->clk_eee);
 		priv->clk_eee_enabled = false;
 	}
-
-	priv->eee.eee_enabled = enable;
-	priv->eee.eee_active = enable;
 }
 
 static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_eee *e)
@@ -1326,8 +1328,7 @@ static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_eee *e)
 	if (!dev->phydev)
 		return -ENODEV;
 
-	e->eee_enabled = p->eee_enabled;
-	e->eee_active = p->eee_active;
+	e->tx_lpi_enabled = p->tx_lpi_enabled;
 	e->tx_lpi_timer = bcmgenet_umac_readl(priv, UMAC_EEE_LPI_TIMER);
 
 	return phy_ethtool_get_eee(dev->phydev, e);
@@ -1337,7 +1338,6 @@ static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_eee *e)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
 	struct ethtool_eee *p = &priv->eee;
-	int ret = 0;
 
 	if (GENET_IS_V1(priv))
 		return -EOPNOTSUPP;
@@ -1345,20 +1345,9 @@ static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_eee *e)
 	if (!dev->phydev)
 		return -ENODEV;
 
-	p->eee_enabled = e->eee_enabled;
+	p->tx_lpi_enabled = e->tx_lpi_enabled;
 
-	if (!p->eee_enabled) {
-		bcmgenet_eee_enable_set(dev, false);
-	} else {
-		ret = phy_init_eee(dev->phydev, false);
-		if (ret) {
-			netif_err(priv, hw, dev, "EEE initialization failed\n");
-			return ret;
-		}
-
-		bcmgenet_umac_writel(priv, e->tx_lpi_timer, UMAC_EEE_LPI_TIMER);
-		bcmgenet_eee_enable_set(dev, true);
-	}
+	bcmgenet_umac_writel(priv, e->tx_lpi_timer, UMAC_EEE_LPI_TIMER);
 
 	return phy_ethtool_set_eee(dev->phydev, e);
 }
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
index 946f6e283c4e..7458a62afc2c 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
@@ -703,4 +703,5 @@ int bcmgenet_wol_power_down_cfg(struct bcmgenet_priv *priv,
 void bcmgenet_wol_power_up_cfg(struct bcmgenet_priv *priv,
 			       enum bcmgenet_power_mode mode);
 
+void bcmgenet_eee_enable_set(struct net_device *dev, bool eee_active);
 #endif /* __BCMGENET_H__ */
diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index b615176338b2..eb1747503c2e 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -100,6 +100,7 @@ void bcmgenet_mii_setup(struct net_device *dev)
 
 	if (phydev->link) {
 		bcmgenet_mac_config(dev);
+		bcmgenet_eee_enable_set(dev, phydev->eee_active);
 	} else {
 		reg = bcmgenet_ext_readl(priv, EXT_RGMII_OOB_CTRL);
 		reg &= ~RGMII_LINK;
-- 
2.39.1

Re: [PATCH RFC 09/18] net: genet: Fixup EEE

[Florian Fainelli]

On 2/16/2023 7:42 PM, Andrew Lunn wrote:
> The enabling/disabling of EEE in the MAC should happen as a result of
> auto negotiation. So move the enable/disable into bcmgenet_mii_setup()
> which gets called by phylib when there is a change in link status.
> 
> bcmgenet_set_eee() now just writes the LTI timer value to the hardware
> and stores if TX LPI should be enabled. Everything else is passed to
> phylib, so it can correctly setup the PHY.
> 
> bcmgenet_get_eee() relies on phylib doing most of the work, the MAC
> driver just adds the LTI timer value from hardware and the stored
> tx_lpi_enabled.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

This looks similar to a number of patches for GENET that I need to 
resurrect against net-next, or even submit to net. LGTM at first glance, 
I will give you series a test.

> ---
>   .../net/ethernet/broadcom/genet/bcmgenet.c    | 31 ++++++-------------
>   .../net/ethernet/broadcom/genet/bcmgenet.h    |  1 +
>   drivers/net/ethernet/broadcom/genet/bcmmii.c  |  1 +
>   3 files changed, 12 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index d937daa8ee88..2793d94ed32c 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -1272,12 +1272,17 @@ static void bcmgenet_get_ethtool_stats(struct net_device *dev,
>   	}
>   }
>   
> -static void bcmgenet_eee_enable_set(struct net_device *dev, bool enable)
> +void bcmgenet_eee_enable_set(struct net_device *dev, bool eee_active)
>   {
>   	struct bcmgenet_priv *priv = netdev_priv(dev);
> -	u32 off = priv->hw_params->tbuf_offset + TBUF_ENERGY_CTRL;

Seems unnecessary, yes it does not quite abide by the RCT style, but no 
need to fix that yet.

> +	struct ethtool_eee *p = &priv->eee;
> +	bool enable;
> +	u32 off;
>   	u32 reg;
>   
> +	off = priv->hw_params->tbuf_offset + TBUF_ENERGY_CTRL;
> +	enable = eee_active && p->tx_lpi_enabled;
> +
>   	if (enable && !priv->clk_eee_enabled) {
>   		clk_prepare_enable(priv->clk_eee);
>   		priv->clk_eee_enabled = true;
> @@ -1310,9 +1315,6 @@ static void bcmgenet_eee_enable_set(struct net_device *dev, bool enable)
>   		clk_disable_unprepare(priv->clk_eee);
>   		priv->clk_eee_enabled = false;
>   	}
> -
> -	priv->eee.eee_enabled = enable;
> -	priv->eee.eee_active = enable;
>   }
>   
>   static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_eee *e)
> @@ -1326,8 +1328,7 @@ static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_eee *e)
>   	if (!dev->phydev)
>   		return -ENODEV;
>   
> -	e->eee_enabled = p->eee_enabled;
> -	e->eee_active = p->eee_active;
> +	e->tx_lpi_enabled = p->tx_lpi_enabled;
>   	e->tx_lpi_timer = bcmgenet_umac_readl(priv, UMAC_EEE_LPI_TIMER);
>   
>   	return phy_ethtool_get_eee(dev->phydev, e);
> @@ -1337,7 +1338,6 @@ static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_eee *e)
>   {
>   	struct bcmgenet_priv *priv = netdev_priv(dev);
>   	struct ethtool_eee *p = &priv->eee;
> -	int ret = 0;
>   
>   	if (GENET_IS_V1(priv))
>   		return -EOPNOTSUPP;
> @@ -1345,20 +1345,9 @@ static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_eee *e)
>   	if (!dev->phydev)
>   		return -ENODEV;
>   
> -	p->eee_enabled = e->eee_enabled;
> +	p->tx_lpi_enabled = e->tx_lpi_enabled;
>   
> -	if (!p->eee_enabled) {
> -		bcmgenet_eee_enable_set(dev, false);
> -	} else {
> -		ret = phy_init_eee(dev->phydev, false);
> -		if (ret) {
> -			netif_err(priv, hw, dev, "EEE initialization failed\n");
> -			return ret;
> -		}
> -
> -		bcmgenet_umac_writel(priv, e->tx_lpi_timer, UMAC_EEE_LPI_TIMER);
> -		bcmgenet_eee_enable_set(dev, true);
> -	}
> +	bcmgenet_umac_writel(priv, e->tx_lpi_timer, UMAC_EEE_LPI_TIMER);
>   
>   	return phy_ethtool_set_eee(dev->phydev, e);
>   }
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> index 946f6e283c4e..7458a62afc2c 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> @@ -703,4 +703,5 @@ int bcmgenet_wol_power_down_cfg(struct bcmgenet_priv *priv,
>   void bcmgenet_wol_power_up_cfg(struct bcmgenet_priv *priv,
>   			       enum bcmgenet_power_mode mode);
>   
> +void bcmgenet_eee_enable_set(struct net_device *dev, bool eee_active);
>   #endif /* __BCMGENET_H__ */
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
> index b615176338b2..eb1747503c2e 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
> @@ -100,6 +100,7 @@ void bcmgenet_mii_setup(struct net_device *dev)
>   
>   	if (phydev->link) {
>   		bcmgenet_mac_config(dev);
> +		bcmgenet_eee_enable_set(dev, phydev->eee_active);
>   	} else {
>   		reg = bcmgenet_ext_readl(priv, EXT_RGMII_OOB_CTRL);
>   		reg &= ~RGMII_LINK;

-- 
Florian

Re: [PATCH RFC 09/18] net: genet: Fixup EEE

[Andrew Lunn]

> This looks similar to a number of patches for GENET that I need to resurrect
> against net-next, or even submit to net. LGTM at first glance, I will give
> you series a test.

Thanks for testing.

> > --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> > +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> > @@ -1272,12 +1272,17 @@ static void bcmgenet_get_ethtool_stats(struct net_device *dev,
> >   	}
> >   }
> > -static void bcmgenet_eee_enable_set(struct net_device *dev, bool enable)
> > +void bcmgenet_eee_enable_set(struct net_device *dev, bool eee_active)
> >   {
> >   	struct bcmgenet_priv *priv = netdev_priv(dev);
> > -	u32 off = priv->hw_params->tbuf_offset + TBUF_ENERGY_CTRL;
> 
> Seems unnecessary, yes it does not quite abide by the RCT style, but no need
> to fix that yet.

Yes, it is unnecassary. I can drop it.

     Andrew

[PATCH RFC 10/18] net: sxgdb: Fixup EEE

[Andrew Lunn]

The enabling/disabling of EEE in the MAC should happen as a result of
auto negotiation. So rework sxgbe_eee_adjust() to take the result of
negotiation into account.

sxgbe_set_eee() now just stores LTI timer value and
tx_lpi_enabled. Everything else is passed to phylib, so it can
correctly setup the PHY.

sxgbe_get_eee() relies on phylib doing most of the work, the MAC
driver just adds the LTI timer value and tx_lpi_enabled.

The hw_cap.eee is now used to control timers, rather than eee_enabled,
which was wrongly being set based on the value of phy_init_eee()
before auto-neg even completed.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 .../net/ethernet/samsung/sxgbe/sxgbe_common.h |  4 +-
 .../ethernet/samsung/sxgbe/sxgbe_ethtool.c    | 23 ++----------
 .../net/ethernet/samsung/sxgbe/sxgbe_main.c   | 37 ++++++-------------
 3 files changed, 16 insertions(+), 48 deletions(-)

diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_common.h b/drivers/net/ethernet/samsung/sxgbe/sxgbe_common.h
index 0f45107db8dd..6860369ca88f 100644
--- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_common.h
+++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_common.h
@@ -502,9 +502,8 @@ struct sxgbe_priv_data {
 	struct timer_list eee_ctrl_timer;
 	bool tx_path_in_lpi_mode;
 	int lpi_irq;
-	int eee_enabled;
-	int eee_active;
 	int tx_lpi_timer;
+	bool tx_lpi_enabled;
 };
 
 /* Function prototypes */
@@ -528,5 +527,4 @@ int sxgbe_restore(struct net_device *ndev);
 const struct sxgbe_mtl_ops *sxgbe_get_mtl_ops(void);
 
 void sxgbe_disable_eee_mode(struct sxgbe_priv_data * const priv);
-bool sxgbe_eee_init(struct sxgbe_priv_data * const priv);
 #endif /* __SXGBE_COMMON_H__ */
diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c
index 8ba017ec9849..0ec523b24df9 100644
--- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c
+++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c
@@ -140,8 +140,7 @@ static int sxgbe_get_eee(struct net_device *dev,
 	if (!priv->hw_cap.eee)
 		return -EOPNOTSUPP;
 
-	edata->eee_enabled = priv->eee_enabled;
-	edata->eee_active = priv->eee_active;
+	edata->tx_lpi_enabled = priv->tx_lpi_enabled;
 	edata->tx_lpi_timer = priv->tx_lpi_timer;
 
 	return phy_ethtool_get_eee(dev->phydev, edata);
@@ -152,22 +151,8 @@ static int sxgbe_set_eee(struct net_device *dev,
 {
 	struct sxgbe_priv_data *priv = netdev_priv(dev);
 
-	priv->eee_enabled = edata->eee_enabled;
-
-	if (!priv->eee_enabled) {
-		sxgbe_disable_eee_mode(priv);
-	} else {
-		/* We are asking for enabling the EEE but it is safe
-		 * to verify all by invoking the eee_init function.
-		 * In case of failure it will return an error.
-		 */
-		priv->eee_enabled = sxgbe_eee_init(priv);
-		if (!priv->eee_enabled)
-			return -EOPNOTSUPP;
-
-		/* Do not change tx_lpi_timer in case of failure */
-		priv->tx_lpi_timer = edata->tx_lpi_timer;
-	}
+	priv->tx_lpi_timer = edata->tx_lpi_timer;
+	priv->tx_lpi_enabled = edata->tx_lpi_enabled;
 
 	return phy_ethtool_set_eee(dev->phydev, edata);
 }
@@ -230,7 +215,7 @@ static void sxgbe_get_ethtool_stats(struct net_device *dev,
 	int i;
 	char *p;
 
-	if (priv->eee_enabled) {
+	if (dev->phydev->eee_active) {
 		int val = phy_get_eee_err(dev->phydev);
 
 		if (val)
diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
index 9664f029fa16..f9ad232133a4 100644
--- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
+++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
@@ -119,18 +119,10 @@ static void sxgbe_eee_ctrl_timer(struct timer_list *t)
  *  phy can also manage EEE, so enable the LPI state and start the timer
  *  to verify if the tx path can enter in LPI state.
  */
-bool sxgbe_eee_init(struct sxgbe_priv_data * const priv)
+static void sxgbe_eee_init(struct sxgbe_priv_data * const priv)
 {
-	struct net_device *ndev = priv->dev;
-	bool ret = false;
-
 	/* MAC core supports the EEE feature. */
 	if (priv->hw_cap.eee) {
-		/* Check if the PHY supports EEE */
-		if (phy_init_eee(ndev->phydev, true))
-			return false;
-
-		priv->eee_active = 1;
 		timer_setup(&priv->eee_ctrl_timer, sxgbe_eee_ctrl_timer, 0);
 		priv->eee_ctrl_timer.expires = SXGBE_LPI_TIMER(eee_timer);
 		add_timer(&priv->eee_ctrl_timer);
@@ -140,23 +132,16 @@ bool sxgbe_eee_init(struct sxgbe_priv_data * const priv)
 					     priv->tx_lpi_timer);
 
 		pr_info("Energy-Efficient Ethernet initialized\n");
-
-		ret = true;
 	}
-
-	return ret;
 }
 
-static void sxgbe_eee_adjust(const struct sxgbe_priv_data *priv)
+static void sxgbe_eee_adjust(const struct sxgbe_priv_data *priv,
+			     bool eee_active)
 {
-	struct net_device *ndev = priv->dev;
-
-	/* When the EEE has been already initialised we have to
-	 * modify the PLS bit in the LPI ctrl & status reg according
-	 * to the PHY link status. For this reason.
-	 */
-	if (priv->eee_enabled)
-		priv->hw->mac->set_eee_pls(priv->ioaddr, ndev->phydev->link);
+	if (priv->hw_cap.eee)
+		priv->hw->mac->set_eee_pls(priv->ioaddr,
+					   eee_active && priv->tx_lpi_enabled);
+	phy_eee_clk_stop_enable(priv->dev->phydev);
 }
 
 /**
@@ -250,7 +235,7 @@ static void sxgbe_adjust_link(struct net_device *dev)
 		phy_print_status(phydev);
 
 	/* Alter the MAC settings for EEE */
-	sxgbe_eee_adjust(priv);
+	sxgbe_eee_adjust(priv, phydev->eee_active);
 }
 
 /**
@@ -803,7 +788,7 @@ static void sxgbe_tx_all_clean(struct sxgbe_priv_data * const priv)
 		sxgbe_tx_queue_clean(tqueue);
 	}
 
-	if ((priv->eee_enabled) && (!priv->tx_path_in_lpi_mode)) {
+	if (priv->hw_cap.eee && !priv->tx_path_in_lpi_mode) {
 		sxgbe_enable_eee_mode(priv);
 		mod_timer(&priv->eee_ctrl_timer, SXGBE_LPI_TIMER(eee_timer));
 	}
@@ -1181,7 +1166,7 @@ static int sxgbe_open(struct net_device *dev)
 	}
 
 	priv->tx_lpi_timer = SXGBE_DEFAULT_LPI_TIMER;
-	priv->eee_enabled = sxgbe_eee_init(priv);
+	sxgbe_eee_init(priv);
 
 	napi_enable(&priv->napi);
 	netif_start_queue(dev);
@@ -1208,7 +1193,7 @@ static int sxgbe_release(struct net_device *dev)
 {
 	struct sxgbe_priv_data *priv = netdev_priv(dev);
 
-	if (priv->eee_enabled)
+	if (priv->hw_cap.eee)
 		del_timer_sync(&priv->eee_ctrl_timer);
 
 	/* Stop and disconnect the PHY */
-- 
2.39.1

[PATCH RFC 11/18] net: dsa: mt7530: Swap to using phydev->eee_active

[Andrew Lunn]

Rather than calling phy_init_eee() retrieve the same information from
within the phydev structure.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mt7530.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 3a15015bc409..214450378978 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2859,7 +2859,7 @@ static void mt753x_phylink_mac_link_up(struct dsa_switch *ds, int port,
 			mcr |= PMCR_RX_FC_EN;
 	}
 
-	if (mode == MLO_AN_PHY && phydev && phy_init_eee(phydev, false) >= 0) {
+	if (mode == MLO_AN_PHY && phydev && phydev->eee_active) {
 		switch (speed) {
 		case SPEED_1000:
 			mcr |= PMCR_FORCE_EEE1G;
-- 
2.39.1

[PATCH RFC 12/18] net: dsa: mt7530: Call phylib for set_eee and get_eee

[Andrew Lunn]

phylib should be called in order to manage the EEE settings in the
PHY, and to return EEE status such are supported link modes, and what
the link peer supports.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mt7530.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 214450378978..a472353f14f8 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -3124,10 +3124,13 @@ static int mt753x_get_mac_eee(struct dsa_switch *ds, int port,
 {
 	struct mt7530_priv *priv = ds->priv;
 	u32 eeecr = mt7530_read(priv, MT7530_PMEEECR_P(port));
+	struct dsa_port *dp = dsa_to_port(ds, port);
 
 	e->tx_lpi_enabled = !(eeecr & LPI_MODE_EN);
 	e->tx_lpi_timer = GET_LPI_THRESH(eeecr);
 
+	if (dp->slave->phydev)
+		return phy_ethtool_get_eee(dp->slave->phydev, e);
 	return 0;
 }
 
@@ -3136,6 +3139,7 @@ static int mt753x_set_mac_eee(struct dsa_switch *ds, int port,
 {
 	struct mt7530_priv *priv = ds->priv;
 	u32 set, mask = LPI_THRESH_MASK | LPI_MODE_EN;
+	struct dsa_port *dp = dsa_to_port(ds, port);
 
 	if (e->tx_lpi_timer > 0xFFF)
 		return -EINVAL;
@@ -3146,6 +3150,8 @@ static int mt753x_set_mac_eee(struct dsa_switch *ds, int port,
 		set |= LPI_MODE_EN;
 	mt7530_rmw(priv, MT7530_PMEEECR_P(port), mask, set);
 
+	if (dp->slave->phydev)
+		return phy_ethtool_set_eee(dp->slave->phydev, e);
 	return 0;
 }
 
-- 
2.39.1

Re: [PATCH RFC 12/18] net: dsa: mt7530: Call phylib for set_eee and get_eee

[Russell King (Oracle)]

On Fri, Feb 17, 2023 at 04:42:24AM +0100, Andrew Lunn wrote:
> phylib should be called in order to manage the EEE settings in the
> PHY, and to return EEE status such are supported link modes, and what
> the link peer supports.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/net/dsa/mt7530.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 214450378978..a472353f14f8 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -3124,10 +3124,13 @@ static int mt753x_get_mac_eee(struct dsa_switch *ds, int port,
>  {
>  	struct mt7530_priv *priv = ds->priv;
>  	u32 eeecr = mt7530_read(priv, MT7530_PMEEECR_P(port));
> +	struct dsa_port *dp = dsa_to_port(ds, port);
>  
>  	e->tx_lpi_enabled = !(eeecr & LPI_MODE_EN);
>  	e->tx_lpi_timer = GET_LPI_THRESH(eeecr);
>  
> +	if (dp->slave->phydev)
> +		return phy_ethtool_get_eee(dp->slave->phydev, e);

Given that DSA makes use of phylink, is there a reason why we can't use
the phylink wrappers here (which may allow for future EEE improvements,
e.g. moving the gating of EEE with the TX LPI enable) ?

> @@ -3146,6 +3150,8 @@ static int mt753x_set_mac_eee(struct dsa_switch *ds, int port,
>  		set |= LPI_MODE_EN;
>  	mt7530_rmw(priv, MT7530_PMEEECR_P(port), mask, set);
>  
> +	if (dp->slave->phydev)
> +		return phy_ethtool_set_eee(dp->slave->phydev, e);
>  	return 0;


Is this the correct place to do the set_eee operation - I mean, the
register state has been altered (and it looks like it may enable LPI
irrespective of the negotiated state) but what concerns me is that
phy_ethtool_set_eee() can fail, and we return failure to userspace
yet we've modified register state.

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

Re: [PATCH RFC 12/18] net: dsa: mt7530: Call phylib for set_eee and get_eee

[Andrew Lunn]

On Fri, Feb 17, 2023 at 11:48:01AM +0000, Russell King (Oracle) wrote:
> On Fri, Feb 17, 2023 at 04:42:24AM +0100, Andrew Lunn wrote:
> > phylib should be called in order to manage the EEE settings in the
> > PHY, and to return EEE status such are supported link modes, and what
> > the link peer supports.
> > 
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > ---
> >  drivers/net/dsa/mt7530.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > index 214450378978..a472353f14f8 100644
> > --- a/drivers/net/dsa/mt7530.c
> > +++ b/drivers/net/dsa/mt7530.c
> > @@ -3124,10 +3124,13 @@ static int mt753x_get_mac_eee(struct dsa_switch *ds, int port,
> >  {
> >  	struct mt7530_priv *priv = ds->priv;
> >  	u32 eeecr = mt7530_read(priv, MT7530_PMEEECR_P(port));
> > +	struct dsa_port *dp = dsa_to_port(ds, port);
> >  
> >  	e->tx_lpi_enabled = !(eeecr & LPI_MODE_EN);
> >  	e->tx_lpi_timer = GET_LPI_THRESH(eeecr);
> >  
> > +	if (dp->slave->phydev)
> > +		return phy_ethtool_get_eee(dp->slave->phydev, e);
> 
> Given that DSA makes use of phylink, is there a reason why we can't use
> the phylink wrappers here

Ah, yes, phylink_foo would make a lot of sense. I will fix in the next
version.

> > @@ -3146,6 +3150,8 @@ static int mt753x_set_mac_eee(struct dsa_switch *ds, int port,
> >  		set |= LPI_MODE_EN;
> >  	mt7530_rmw(priv, MT7530_PMEEECR_P(port), mask, set);
> >  
> > +	if (dp->slave->phydev)
> > +		return phy_ethtool_set_eee(dp->slave->phydev, e);
> >  	return 0;
> 
> 
> Is this the correct place to do the set_eee operation - I mean, the
> register state has been altered (and it looks like it may enable LPI
> irrespective of the negotiated state) but what concerns me is that
> phy_ethtool_set_eee() can fail, and we return failure to userspace
> yet we've modified register state.

There is currently no consistency here between drivers. Some make this
call last, some do it earlier.

Thinking aloud...

Why would it fail? Most error reports are that phy_read() or
phy_write() failed. If that happens, the hardware is in an
inconsistent state anyway. It could be the PHY does not support
EEE. So an -EOPNOTSUPP or similar could be returned here. So long as
phydev->eee_active is never true, it should not matter if the value of
the LPI timer is changed here, it should never be put into use.  Are
there other cases where it could fail?

Maybe we should make the order in MAC set_eee() function
consistent. It is just a bigger change...

	    Andrew

Re: [PATCH RFC 12/18] net: dsa: mt7530: Call phylib for set_eee and get_eee

[Andrew Lunn]

 On Fri, Feb 17, 2023 at 11:48:01AM +0000, Russell King (Oracle) wrote:
> On Fri, Feb 17, 2023 at 04:42:24AM +0100, Andrew Lunn wrote:
> > phylib should be called in order to manage the EEE settings in the
> > PHY, and to return EEE status such are supported link modes, and what
> > the link peer supports.
> > 
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > ---
> >  drivers/net/dsa/mt7530.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > index 214450378978..a472353f14f8 100644
> > --- a/drivers/net/dsa/mt7530.c
> > +++ b/drivers/net/dsa/mt7530.c
> > @@ -3124,10 +3124,13 @@ static int mt753x_get_mac_eee(struct dsa_switch *ds, int port,
> >  {
> >  	struct mt7530_priv *priv = ds->priv;
> >  	u32 eeecr = mt7530_read(priv, MT7530_PMEEECR_P(port));
> > +	struct dsa_port *dp = dsa_to_port(ds, port);
> >  
> >  	e->tx_lpi_enabled = !(eeecr & LPI_MODE_EN);
> >  	e->tx_lpi_timer = GET_LPI_THRESH(eeecr);
> >  
> > +	if (dp->slave->phydev)
> > +		return phy_ethtool_get_eee(dp->slave->phydev, e);
> 
> Given that DSA makes use of phylink, is there a reason why we can't use
> the phylink wrappers here (which may allow for future EEE improvements,
> e.g. moving the gating of EEE with the TX LPI enable) ?

Turns out this is pointless. dsa_slave_get_eee() and
dsa_slave_set_eee() already make calls to phylink_ethtool_get_eee()
and phylink_ethtool_set_eee() after calling into the DSA driver.

So i will drop this patch, and the b53 change.

   Andrew

[PATCH RFC 13/18] net: dsa: b53: Swap to using phydev->eee_active

[Andrew Lunn]

Rather than calling phy_init_eee() retrieve the same information from
within the phydev structure.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/b53/b53_common.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 59cdfc51ce06..1e8eaf143b65 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -2168,10 +2168,7 @@ EXPORT_SYMBOL(b53_eee_enable_set);
  */
 int b53_eee_init(struct dsa_switch *ds, int port, struct phy_device *phy)
 {
-	int ret;
-
-	ret = phy_init_eee(phy, false);
-	if (ret)
+	if (!phy->eee_active)
 		return 0;
 
 	b53_eee_enable_set(ds, port, true);
-- 
2.39.1

[PATCH RFC 14/18] net: dsa: b53: Call phylib for set_eee and get_eee

[Andrew Lunn]

phylib should be called in order to manage the EEE settings in the
PHY, and to return EEE status such are supported link modes, and what
the link peer supports.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/b53/b53_common.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 1e8eaf143b65..f782a08a9bc9 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -2179,6 +2179,7 @@ EXPORT_SYMBOL(b53_eee_init);
 
 int b53_get_mac_eee(struct dsa_switch *ds, int port, struct ethtool_eee *e)
 {
+	struct dsa_port *dp = dsa_to_port(ds, port);
 	struct b53_device *dev = ds->priv;
 	struct ethtool_eee *p = &dev->ports[port].eee;
 	u16 reg;
@@ -2190,12 +2191,15 @@ int b53_get_mac_eee(struct dsa_switch *ds, int port, struct ethtool_eee *e)
 	e->eee_enabled = p->eee_enabled;
 	e->eee_active = !!(reg & BIT(port));
 
+	if (dp->slave->phydev)
+		return phy_ethtool_get_eee(dp->slave->phydev, e);
 	return 0;
 }
 EXPORT_SYMBOL(b53_get_mac_eee);
 
 int b53_set_mac_eee(struct dsa_switch *ds, int port, struct ethtool_eee *e)
 {
+	struct dsa_port *dp = dsa_to_port(ds, port);
 	struct b53_device *dev = ds->priv;
 	struct ethtool_eee *p = &dev->ports[port].eee;
 
@@ -2205,6 +2209,8 @@ int b53_set_mac_eee(struct dsa_switch *ds, int port, struct ethtool_eee *e)
 	p->eee_enabled = e->eee_enabled;
 	b53_eee_enable_set(ds, port, e->eee_enabled);
 
+	if (dp->slave->phydev)
+		return phy_ethtool_set_eee(dp->slave->phydev, e);
 	return 0;
 }
 EXPORT_SYMBOL(b53_set_mac_eee);
-- 
2.39.1

[PATCH RFC 15/18] net: phylink: Remove unused phylink_init_eee()

[Andrew Lunn]

This is not used in tree, and the phylib equivalent phy_init_eee() is
about to be removed.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/phylink.c | 18 ------------------
 include/linux/phylink.h   |  1 -
 2 files changed, 19 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 1a2f074685fa..f84d92230806 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2501,24 +2501,6 @@ int phylink_get_eee_err(struct phylink *pl)
 }
 EXPORT_SYMBOL_GPL(phylink_get_eee_err);
 
-/**
- * phylink_init_eee() - init and check the EEE features
- * @pl: a pointer to a &struct phylink returned from phylink_create()
- * @clk_stop_enable: allow PHY to stop receive clock
- *
- * Must be called either with RTNL held or within mac_link_up()
- */
-int phylink_init_eee(struct phylink *pl, bool clk_stop_enable)
-{
-	int ret = -EOPNOTSUPP;
-
-	if (pl->phydev)
-		ret = phy_init_eee(pl->phydev, clk_stop_enable);
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(phylink_init_eee);
-
 /**
  * phylink_ethtool_get_eee() - read the energy efficient ethernet parameters
  * @pl: a pointer to a &struct phylink returned from phylink_create()
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index c492c26202b5..d9ea4651426d 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -603,7 +603,6 @@ void phylink_ethtool_get_pauseparam(struct phylink *,
 int phylink_ethtool_set_pauseparam(struct phylink *,
 				   struct ethtool_pauseparam *);
 int phylink_get_eee_err(struct phylink *);
-int phylink_init_eee(struct phylink *, bool);
 int phylink_ethtool_get_eee(struct phylink *, struct ethtool_eee *);
 int phylink_ethtool_set_eee(struct phylink *, struct ethtool_eee *);
 int phylink_mii_ioctl(struct phylink *, struct ifreq *, int);
-- 
2.39.1

Re: [PATCH RFC 15/18] net: phylink: Remove unused phylink_init_eee()

[Russell King (Oracle)]

On Fri, Feb 17, 2023 at 04:42:27AM +0100, Andrew Lunn wrote:
> This is not used in tree, and the phylib equivalent phy_init_eee() is
> about to be removed.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Yay!

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks!

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

[PATCH RFC 16/18] net: phylink: Update comment about configuring EEE in mac_link_up()

[Andrew Lunn]

phy_init_eee() is about to be removed. If eee is active can be
determined directly from the phydev structure. Update the
documentation to indicate this.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 include/linux/phylink.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index d9ea4651426d..78f25990e498 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -424,9 +424,9 @@ void mac_link_down(struct phylink_config *config, unsigned int mode,
  * be allowed when considering the implementation of this method.
  *
  * If in-band negotiation mode is disabled, allow the link to come up. If
- * @phy is non-%NULL, configure Energy Efficient Ethernet by calling
- * phy_init_eee() and perform appropriate MAC configuration for EEE.
- * Interface type selection must be done in mac_config().
+ * @phy is non-%NULL, configure Energy Efficient Ethernet in the MAC if
+ * phy->eee_active is true. Interface type selection must be done in
+ * mac_config().
  */
 void mac_link_up(struct phylink_config *config, struct phy_device *phy,
 		 unsigned int mode, phy_interface_t interface,
-- 
2.39.1

Re: [PATCH RFC 16/18] net: phylink: Update comment about configuring EEE in mac_link_up()

[Russell King (Oracle)]

On Fri, Feb 17, 2023 at 04:42:28AM +0100, Andrew Lunn wrote:
> phy_init_eee() is about to be removed. If eee is active can be
> determined directly from the phydev structure. Update the
> documentation to indicate this.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks!

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

[PATCH RFC 17/18] net: phy: remove unused phy_init_eee()

[Andrew Lunn]

There are no users left of phy_init_eee(), and it is often wrongly
used. So remove it.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/phy.c | 34 ----------------------------------
 include/linux/phy.h   |  1 -
 2 files changed, 35 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index b25e0946405b..135f30d576b2 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1494,40 +1494,6 @@ int phy_eee_clk_stop_enable(struct phy_device *phydev)
 	return ret;
 }
 
-/**
- * phy_init_eee - init and check the EEE feature
- * @phydev: target phy_device struct
- * @clk_stop_enable: PHY may stop the clock during LPI
- *
- * Description: it checks if the Energy-Efficient Ethernet (EEE)
- * is supported by looking at the MMD registers 3.20 and 7.60/61
- * and it programs the MMD register 3.0 setting the "Clock stop enable"
- * bit if required.
- */
-int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable)
-{
-	int ret;
-
-	if (!phydev->drv)
-		return -EIO;
-
-	ret = genphy_c45_eee_is_active(phydev, NULL, NULL, NULL);
-	if (ret < 0)
-		return ret;
-	if (!ret)
-		return -EPROTONOSUPPORT;
-
-	if (clk_stop_enable)
-		/* Configure the PHY to stop receiving xMII
-		 * clock while it is signaling LPI.
-		 */
-		ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1,
-				       MDIO_PCS_CTRL1_CLKSTOP_EN);
-
-	return ret < 0 ? ret : 0;
-}
-EXPORT_SYMBOL(phy_init_eee);
-
 /**
  * phy_get_eee_err - report the EEE wake error count
  * @phydev: target phy_device struct
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 4f72ffcb8b02..593a80d43454 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1839,7 +1839,6 @@ int phy_unregister_fixup(const char *bus_id, u32 phy_uid, u32 phy_uid_mask);
 int phy_unregister_fixup_for_id(const char *bus_id);
 int phy_unregister_fixup_for_uid(u32 phy_uid, u32 phy_uid_mask);
 
-int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable);
 int phy_eee_clk_stop_enable(struct phy_device *phydev);
 int phy_get_eee_err(struct phy_device *phydev);
 int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_eee *data);
-- 
2.39.1

[PATCH RFC 18/18] net: usb: lan78xx: Fixup EEE

[Andrew Lunn]

The enabling/disabling of EEE in the MAC should happen as a result of
auto negotiation. So move the enable/disable into
lan783xx_phy_link_status_change() which gets called by phylib when
there is a change in link status.

lan78xx_set_eee() now just programs the hardware with the LTI
timer value, and passed everything else to phylib, so it can correctly
setup the PHY.

lan743x_get_eee() relies on phylib doing most of the work, the
MAC driver just adds the LTI timer value, and tx_lpi_enabled based on
if EEE is active.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/usb/lan78xx.c | 36 ++++++++++++++++--------------------
 1 file changed, 16 insertions(+), 20 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index f18ab8e220db..a06eac83cc01 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -1690,16 +1690,11 @@ static int lan78xx_get_eee(struct net_device *net, struct ethtool_eee *edata)
 
 	ret = lan78xx_read_reg(dev, MAC_CR, &buf);
 	if (buf & MAC_CR_EEE_EN_) {
-		edata->eee_enabled = true;
-		edata->eee_active = !!(edata->advertised &
-				       edata->lp_advertised);
 		edata->tx_lpi_enabled = true;
 		/* EEE_TX_LPI_REQ_DLY & tx_lpi_timer are same uSec unit */
 		ret = lan78xx_read_reg(dev, EEE_TX_LPI_REQ_DLY, &buf);
 		edata->tx_lpi_timer = buf;
 	} else {
-		edata->eee_enabled = false;
-		edata->eee_active = false;
 		edata->tx_lpi_enabled = false;
 		edata->tx_lpi_timer = 0;
 	}
@@ -1721,24 +1716,16 @@ static int lan78xx_set_eee(struct net_device *net, struct ethtool_eee *edata)
 	if (ret < 0)
 		return ret;
 
-	if (edata->eee_enabled) {
-		ret = lan78xx_read_reg(dev, MAC_CR, &buf);
-		buf |= MAC_CR_EEE_EN_;
-		ret = lan78xx_write_reg(dev, MAC_CR, buf);
-
-		phy_ethtool_set_eee(net->phydev, edata);
-
-		buf = (u32)edata->tx_lpi_timer;
-		ret = lan78xx_write_reg(dev, EEE_TX_LPI_REQ_DLY, buf);
-	} else {
-		ret = lan78xx_read_reg(dev, MAC_CR, &buf);
-		buf &= ~MAC_CR_EEE_EN_;
-		ret = lan78xx_write_reg(dev, MAC_CR, buf);
-	}
+	ret = phy_ethtool_set_eee(net->phydev, edata);
+	if (ret < 0)
+		goto out;
 
+	buf = (u32)edata->tx_lpi_timer;
+	ret = lan78xx_write_reg(dev, EEE_TX_LPI_REQ_DLY, buf);
+out:
 	usb_autopm_put_interface(dev->intf);
 
-	return 0;
+	return ret;
 }
 
 static u32 lan78xx_get_link(struct net_device *net)
@@ -2114,8 +2101,10 @@ static void lan78xx_remove_mdio(struct lan78xx_net *dev)
 
 static void lan78xx_link_status_change(struct net_device *net)
 {
+	struct lan78xx_net *dev = netdev_priv(net);
 	struct phy_device *phydev = net->phydev;
 	int temp;
+	u32 data;
 
 	/* At forced 100 F/H mode, chip may fail to set mode correctly
 	 * when cable is switched between long(~50+m) and short one.
@@ -2142,6 +2131,13 @@ static void lan78xx_link_status_change(struct net_device *net)
 		temp |= LAN88XX_INT_MASK_MDINTPIN_EN_;
 		phy_write(phydev, LAN88XX_INT_MASK, temp);
 	}
+
+	lan78xx_read_reg(dev, MAC_CR, &data);
+	if (phydev->eee_active)
+		data |=  MAC_CR_EEE_EN_;
+	else
+		data &= ~MAC_CR_EEE_EN_;
+	lan78xx_write_reg(dev, MAC_CR, data);
 }
 
 static int irq_map(struct irq_domain *d, unsigned int irq,
-- 
2.39.1

Re: [PATCH RFC 00/18] Rework MAC drivers EEE support

[Russell King (Oracle)]

On Fri, Feb 17, 2023 at 04:42:12AM +0100, Andrew Lunn wrote:
> phy_init_eee() is supposed to be called once auto-neg has been
> completed to determine if EEE should be used with the current link
> mode. The MAC hardware should then be configured to either enable or
> disable EEE. Many drivers get this wrong, calling phy_init_eee() once,
> or only in the ethtool set_eee callback.
> 
> This patchset changes the API, such that EEE becomes the same as other
> parameters which are determined by auto-neg. As will speed and duplex,
> active EEE is now indicated in the phydev structure, and the
> adjust_link callbacks have been modified to act upon its value.
> 
> eee_set and eee_get have been simplified, given that the phylib
> functions act upon most of the data in eee_set, and fill in most of
> the information needed for eee_set.

This is a very nice cleanup, and removes a bunch of logic from MAC
drivers into the phylib core code that should result in more
uniform behaviour across MAC drivers for this feature. Great!

I'm left wondering about the phylink using drivers, whether we could
go a little further, because there's also the tx_lpi_enabled flag
which should also gate whether EEE is enabled at the MAC - and
whether that logic could be handled entirely within phylink too.
That would mean instead of mac_link_up() being passed the phydev
(and EEE is the reason the phydev is passed) we could instead just
pass an "eee" flag to tell the MAC to program itself appropriately.
Then, the only thing which MAC drivers need to concern themselves
with is setting the TX LPI timer to the appropriate value (which
may need to happen in mac_link_up()).

However, for this series, it's definitely a much needed improvement!

Thanks!

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

Re: [PATCH RFC 00/18] Rework MAC drivers EEE support

[Andrew Lunn]

> This is a very nice cleanup, and removes a bunch of logic from MAC
> drivers into the phylib core code that should result in more
> uniform behaviour across MAC drivers for this feature. Great!
> 
> I'm left wondering about the phylink using drivers, whether we could
> go a little further, because there's also the tx_lpi_enabled flag
> which should also gate whether EEE is enabled at the MAC

tx_lpi_enabled is something which i think needs further cleanup. Most
MAC drivers ignore it. I added support to a couple of drivers, when it
was simple to do. But not all.

I'm actually thinking of moving it into phylib. The MAC driver really
does not need to care. All it needs is eee_active in the adjust_link
callback.

I'm currently undecided if to make such a change as part of this
patchset, or do it as a follow up.

	Andrew

Re: [PATCH RFC 00/18] Rework MAC drivers EEE support

[Russell King (Oracle)]

On Fri, Feb 17, 2023 at 04:42:12AM +0100, Andrew Lunn wrote:
> phy_init_eee() is supposed to be called once auto-neg has been
> completed to determine if EEE should be used with the current link
> mode. The MAC hardware should then be configured to either enable or
> disable EEE. Many drivers get this wrong, calling phy_init_eee() once,
> or only in the ethtool set_eee callback.

Looking at some of the recent EEE changes (not related to this patch
set) I've come across:

commit 9b01c885be364526d8c05794f8358b3e563b7ff8
Author: Oleksij Rempel <linux@rempel-privat.de>
Date:   Sat Feb 11 08:41:10 2023 +0100

    net: phy: c22: migrate to genphy_c45_write_eee_adv()

This part of the patch is wrong:

__genphy_config_aneg():
-       if (genphy_config_eee_advert(phydev))
+       err = genphy_c45_write_eee_adv(phydev, phydev->supported_eee);

The problem here is that these are not equivalent.

genphy_config_eee_advert() only clears the broken EEE modes in the
advertisement, it doesn't actually set the advertisement to anything
in particular.

The replacement code _configures_ the advertisement to whatever the
second argument is, which means each time the advertisement is
changed (and thus __genphy_config_aneg() is called) the EEE
advertisement will ignore whatever the user configured via the
set_eee() APIs, and be restored to the full EEE capabilities in the
supported mask.

This is an obvious regression that needs fixing, especially as the
merge window is potentially due to open this weekend.

Moreover, it looks like Oleksij's patch was not Cc'd to me (I can't
find it in my mailbox) and as I'm listed in MAINTAINERS for phylib,
this should have been brought up _before_ Oleksij's patch was
applied to net-next (despite me being unlikely to reply to it due
to covid, it still would be nice to have reviewed it, or even
reply to the damn patch about these concerns.) But I'm having to
pick some other damn series to bring up this concern.

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

Re: [PATCH RFC 00/18] Rework MAC drivers EEE support

[Oleksij Rempel]

On Fri, Feb 17, 2023 at 02:25:57PM +0000, Russell King (Oracle) wrote:
> On Fri, Feb 17, 2023 at 04:42:12AM +0100, Andrew Lunn wrote:
> > phy_init_eee() is supposed to be called once auto-neg has been
> > completed to determine if EEE should be used with the current link
> > mode. The MAC hardware should then be configured to either enable or
> > disable EEE. Many drivers get this wrong, calling phy_init_eee() once,
> > or only in the ethtool set_eee callback.
> 
> Looking at some of the recent EEE changes (not related to this patch
> set) I've come across:
> 
> commit 9b01c885be364526d8c05794f8358b3e563b7ff8
> Author: Oleksij Rempel <linux@rempel-privat.de>
> Date:   Sat Feb 11 08:41:10 2023 +0100
> 
>     net: phy: c22: migrate to genphy_c45_write_eee_adv()
> 
> This part of the patch is wrong:
> 
> __genphy_config_aneg():
> -       if (genphy_config_eee_advert(phydev))
> +       err = genphy_c45_write_eee_adv(phydev, phydev->supported_eee);
> 
> The problem here is that these are not equivalent.
> 
> genphy_config_eee_advert() only clears the broken EEE modes in the
> advertisement, it doesn't actually set the advertisement to anything
> in particular.
> 
> The replacement code _configures_ the advertisement to whatever the
> second argument is, which means each time the advertisement is
> changed (and thus __genphy_config_aneg() is called) the EEE
> advertisement will ignore whatever the user configured via the
> set_eee() APIs, and be restored to the full EEE capabilities in the
> supported mask.
> 
> This is an obvious regression that needs fixing, especially as the
> merge window is potentially due to open this weekend.

You are right :(

I'll be able to come with a fix this Monday.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |