[PATCH net-next 00/10] net: add phylink managed EEE support

[PATCH net-next 00/10] net: add phylink managed EEE support

[Russell King (Oracle)]

Hi,

Adding managed EEE support to phylink has been on the cards ever since
the idea in phylib was mooted. This overly large series attempts to do
so. I've included all the patches as it's important to get the driver
patches out there.

Patch 1 adds a definition for the clock stop capable bit in the PCS
MMD status register.

Patch 2 adds a phylib API to query whether the PHY allows the transmit
xMII clock to be stopped while in LPI mode. This capability is for MAC
drivers to save power when LPI is active, to allow them to stop their
transmit clock.

Patch 3 adds another phylib API to configure whether the receive xMII
clock may be disabled by the PHY. We do have an existing API,
phy_init_eee(), but... it only allows the control bit to be set which
is weird - what if a boot firmware or previous kernel has set this bit
and we want it clear?

Patch 4 starts on the phylink parts of this, extracting from
phylink_resolve() the detection of link-up. (Yes, okay, I could've
dropped this patch, but with 23 patches, it's not going to make that
much difference.)

Patch 5 adds phylink managed EEE support. Two new MAC APIs are added,
to enable and disable LPI. The enable method is passed the LPI timer
setting which it is expected to program into the hardware, and also a
flag ehther the transmit clock should be stopped.

 *** There are open questions here. Eagle eyed reviewers will notice
   pl->config->lpi_interfaces. There are MACs out there which only
   support LPI signalling on a subset of their interface types. Phylib
   doesn't understand this. I'm handling this at the moment by simply
   not activating LPI at the MAC, but that leads to ethtool --show-eee
   suggesting that EEE is active when it isn't.
 *** Should we pass the phy_interface_t to these functions?
 *** Should mac_enable_tx_lpi() be allowed to fail if the MAC doesn't
   support the interface mode?

The above questions remain unanswered from the RFC posting of this
series.

A change that has been included over the RFC version is the addition
of the mac_validate_tx_lpi() method, which allows MAC drivers to
validate the parameters to the ethtool set_eee() method. Implementations
of this are in mvneta and mvpp2.

An example of a MAC that this is the case are the Marvell ones - both
NETA and PP2 only support LPI signalling when connected via SGMII,
which makes being connected to a PHY which changes its link mode
problematical.

The remainder of the patches address the driver sides, which are
necessary to actually test phylink managed EEE.

 drivers/net/ethernet/marvell/mvneta.c            | 127 +++++++++++--------
 drivers/net/ethernet/marvell/mvpp2/mvpp2.h       |   5 +
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c  |  98 +++++++++++++++
 drivers/net/ethernet/microchip/lan743x_ethtool.c |  21 ----
 drivers/net/ethernet/microchip/lan743x_main.c    |  39 ++++--
 drivers/net/ethernet/microchip/lan743x_main.h    |   1 -
 drivers/net/phy/phy.c                            |  47 ++++++-
 drivers/net/phy/phylink.c                        | 150 +++++++++++++++++++++--
 include/linux/phy.h                              |   2 +
 include/linux/phylink.h                          |  59 +++++++++
 include/uapi/linux/mdio.h                        |   1 +
 11 files changed, 458 insertions(+), 92 deletions(-)

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

[PATCH net-next 01/10] net: mdio: add definition for clock stop capable bit

[Russell King (Oracle)]

Add a definition for the clock stop capable bit in the PCS MMD. This
bit indicates whether the MAC is able to stop the transmit xMII clock
while it is signalling LPI.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 include/uapi/linux/mdio.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h
index f0d3f268240d..6975f182b22c 100644
--- a/include/uapi/linux/mdio.h
+++ b/include/uapi/linux/mdio.h
@@ -125,6 +125,7 @@
 #define MDIO_STAT1_LPOWERABLE		0x0002	/* Low-power ability */
 #define MDIO_STAT1_LSTATUS		BMSR_LSTATUS
 #define MDIO_STAT1_FAULT		0x0080	/* Fault */
+#define MDIO_PCS_STAT1_CLKSTOP_CAP	0x0040
 #define MDIO_AN_STAT1_LPABLE		0x0001	/* Link partner AN ability */
 #define MDIO_AN_STAT1_ABLE		BMSR_ANEGCAPABLE
 #define MDIO_AN_STAT1_RFAULT		BMSR_RFAULT
-- 
2.30.2

Re: [PATCH net-next 01/10] net: mdio: add definition for clock stop capable bit

[Andrew Lunn]

On Mon, Dec 09, 2024 at 02:23:18PM +0000, Russell King (Oracle) wrote:
> Add a definition for the clock stop capable bit in the PCS MMD. This
> bit indicates whether the MAC is able to stop the transmit xMII clock
> while it is signalling LPI.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next 02/10] net: phy: add support for querying PHY clock stop capability

[Russell King (Oracle)]

Add support for querying whether the PHY allows the transmit xMII clock
to be stopped while in LPI mode. This will be used by phylink to pass
to the MAC driver so it can configure the generation of the xMII clock
appropriately.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy.c | 20 ++++++++++++++++++++
 include/linux/phy.h   |  1 +
 2 files changed, 21 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index e4b04cdaa995..8fb4224c7576 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1640,6 +1640,26 @@ void phy_mac_interrupt(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_mac_interrupt);
 
+/**
+ * phy_eee_tx_clock_stop_capable() - indicate whether the MAC can stop tx clock
+ * @phydev: target phy_device struct
+ *
+ * Indicate whether the MAC can disable the transmit xMII clock while in LPI
+ * state. Returns 1 if the MAC may stop the transmit clock, 0 if the MAC must
+ * not stop the transmit clock, or negative error.
+ */
+int phy_eee_tx_clock_stop_capable(struct phy_device *phydev)
+{
+	int stat1;
+
+	stat1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_STAT1);
+	if (stat1 < 0)
+		return stat1;
+
+	return !!(stat1 & MDIO_PCS_STAT1_CLKSTOP_CAP);
+}
+EXPORT_SYMBOL_GPL(phy_eee_tx_clock_stop_capable);
+
 /**
  * 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 bb157136351e..267de08c227c 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -2072,6 +2072,7 @@ 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_eee_tx_clock_stop_capable(struct phy_device *phydev);
 int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable);
 int phy_get_eee_err(struct phy_device *phydev);
 int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_keee *data);
-- 
2.30.2

Re: [PATCH net-next 02/10] net: phy: add support for querying PHY clock stop capability

[Andrew Lunn]

On Mon, Dec 09, 2024 at 02:23:23PM +0000, Russell King (Oracle) wrote:
> Add support for querying whether the PHY allows the transmit xMII clock
> to be stopped while in LPI mode. This will be used by phylink to pass
> to the MAC driver so it can configure the generation of the xMII clock
> appropriately.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next 03/10] net: phy: add configuration of rx clock stop mode

[Russell King (Oracle)]

Add a function to allow configuration of the PCS's clock stop enable
bit, used to configure whether the xMII receive clock can be stopped
during LPI mode.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy.c | 27 ++++++++++++++++++++++-----
 include/linux/phy.h   |  1 +
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 8fb4224c7576..caf472b5d4e5 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1660,6 +1660,27 @@ int phy_eee_tx_clock_stop_capable(struct phy_device *phydev)
 }
 EXPORT_SYMBOL_GPL(phy_eee_tx_clock_stop_capable);
 
+/**
+ * phy_eee_rx_clock_stop() - configure PHY receive clock in LPI
+ * @phydev: target phy_device struct
+ * @clk_stop_enable: flag to indicate whether the clock can be stopped
+ *
+ * Configure whether the PHY can disable its receive clock during LPI mode,
+ * See IEEE 802.3 sections 22.2.2.2, 35.2.2.10, and 45.2.3.1.4.
+ *
+ * Returns: 0 or negative error.
+ */
+int phy_eee_rx_clock_stop(struct phy_device *phydev, bool clk_stop_enable)
+{
+	/* Configure the PHY to stop receiving xMII
+	 * clock while it is signaling LPI.
+	 */
+	return phy_modify_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1,
+			      MDIO_PCS_CTRL1_CLKSTOP_EN,
+			      clk_stop_enable ? MDIO_PCS_CTRL1_CLKSTOP_EN : 0);
+}
+EXPORT_SYMBOL_GPL(phy_eee_rx_clock_stop);
+
 /**
  * phy_init_eee - init and check the EEE feature
  * @phydev: target phy_device struct
@@ -1684,11 +1705,7 @@ int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable)
 		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);
+		ret = phy_eee_rx_clock_stop(phydev, true);
 
 	return ret < 0 ? ret : 0;
 }
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 267de08c227c..f865a8fc56c3 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -2073,6 +2073,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_eee_tx_clock_stop_capable(struct phy_device *phydev);
+int phy_eee_rx_clock_stop(struct phy_device *phydev, bool clk_stop_enable);
 int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable);
 int phy_get_eee_err(struct phy_device *phydev);
 int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_keee *data);
-- 
2.30.2

Re: [PATCH net-next 03/10] net: phy: add configuration of rx clock stop mode

[Andrew Lunn]

On Mon, Dec 09, 2024 at 02:23:28PM +0000, Russell King (Oracle) wrote:
> Add a function to allow configuration of the PCS's clock stop enable
> bit, used to configure whether the xMII receive clock can be stopped
> during LPI mode.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 03/10] net: phy: add configuration of rx clock stop mode

[Andrew Lunn]

> @@ -2073,6 +2073,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_eee_tx_clock_stop_capable(struct phy_device *phydev);
> +int phy_eee_rx_clock_stop(struct phy_device *phydev, bool clk_stop_enable);

Hi Russell

Do you have patches to MAC drivers using phylib, not phylink, using
these two new calls?

We see phylib users get EEE wrong. I'm worried phylib users are going
to try to use these new API methods and make an even bigger mess. If
we think these should only be used by phylink, maybe they should be
put into a header in drivers/net/phy to stop MAC drivers using them?

	Andrew

Re: [PATCH net-next 03/10] net: phy: add configuration of rx clock stop mode

[Russell King (Oracle)]

On Tue, Dec 10, 2024 at 04:11:09AM +0100, Andrew Lunn wrote:
> > @@ -2073,6 +2073,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_eee_tx_clock_stop_capable(struct phy_device *phydev);
> > +int phy_eee_rx_clock_stop(struct phy_device *phydev, bool clk_stop_enable);
> 
> Hi Russell
> 
> Do you have patches to MAC drivers using phylib, not phylink, using
> these two new calls?
> 
> We see phylib users get EEE wrong. I'm worried phylib users are going
> to try to use these new API methods and make an even bigger mess. If
> we think these should only be used by phylink, maybe they should be
> put into a header in drivers/net/phy to stop MAC drivers using them?

If we want to fix the current phylib-using drivers, then we do need
at minimum phy_eee_rx_clock_stop() to do that - we have drivers that
call phy_init_eee(..., true) which would need to call this.

It's quite rare that drivers allow the PHY to stop the clock. There
may be several reasons for this:

1) the MAC doesn't support EEE on the MII link type(s) that have a
   clock. (e.g. supporting EEE on SGMII but not RGMII.)

2) the documentation for the MAC doesn't comment on this aspect
   (so the safest thing is to always keep the clock running.)

3) the driver developer hasn't understood the feature and the safest
   approach is to pass phy_init_eee() with a value of zero/false
   which leaves the setting unchanged.

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

Re: [PATCH net-next 03/10] net: phy: add configuration of rx clock stop mode

[Andrew Lunn]

On Tue, Dec 10, 2024 at 09:51:54AM +0000, Russell King (Oracle) wrote:
> On Tue, Dec 10, 2024 at 04:11:09AM +0100, Andrew Lunn wrote:
> > > @@ -2073,6 +2073,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_eee_tx_clock_stop_capable(struct phy_device *phydev);
> > > +int phy_eee_rx_clock_stop(struct phy_device *phydev, bool clk_stop_enable);
> > 
> > Hi Russell
> > 
> > Do you have patches to MAC drivers using phylib, not phylink, using
> > these two new calls?
> > 
> > We see phylib users get EEE wrong. I'm worried phylib users are going
> > to try to use these new API methods and make an even bigger mess. If
> > we think these should only be used by phylink, maybe they should be
> > put into a header in drivers/net/phy to stop MAC drivers using them?
> 
> If we want to fix the current phylib-using drivers, then we do need
> at minimum phy_eee_rx_clock_stop() to do that - we have drivers that
> call phy_init_eee(..., true) which would need to call this.

phy_init_eee() needs to die, so we need this one. 

But we should consider the other one, do we want to make it private?

	Andrew

[PATCH net-next 04/10] net: phylink: add phylink_link_is_up() helper

[Russell King (Oracle)]

Add a helper to determine whether the link is up or down. Currently
this is only used in one location, but becomes necessary to test
when reconfiguring EEE.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 95fbc363f9a6..03509fdaa1ec 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1630,20 +1630,21 @@ static void phylink_link_down(struct phylink *pl)
 	phylink_info(pl, "Link is Down\n");
 }
 
+static bool phylink_link_is_up(struct phylink *pl)
+{
+	return pl->netdev ? netif_carrier_ok(pl->netdev) : pl->old_link_state;
+}
+
 static void phylink_resolve(struct work_struct *w)
 {
 	struct phylink *pl = container_of(w, struct phylink, resolve);
 	struct phylink_link_state link_state;
-	struct net_device *ndev = pl->netdev;
 	bool mac_config = false;
 	bool retrigger = false;
 	bool cur_link_state;
 
 	mutex_lock(&pl->state_mutex);
-	if (pl->netdev)
-		cur_link_state = netif_carrier_ok(ndev);
-	else
-		cur_link_state = pl->old_link_state;
+	cur_link_state = phylink_link_is_up(pl);
 
 	if (pl->phylink_disable_state) {
 		pl->link_failed = false;
-- 
2.30.2

Re: [PATCH net-next 04/10] net: phylink: add phylink_link_is_up() helper

[Andrew Lunn]

On Mon, Dec 09, 2024 at 02:23:33PM +0000, Russell King (Oracle) wrote:
> Add a helper to determine whether the link is up or down. Currently
> this is only used in one location, but becomes necessary to test
> when reconfiguring EEE.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next 05/10] net: phylink: add EEE management

[Russell King (Oracle)]

Add EEE management to phylink, making use of the phylib implementation.
This will only be used where a MAC driver populates the methods and
capabilities bitfield, otherwise we keep our old behaviour.

Phylink will keep track of the EEE configuration, including the clock
stop abilities at each end of the MAC to PHY link, programming the PHY
appropriately and preserving the EEE configuration should the PHY go
away.

It will also call into the MAC driver when LPI needs to be enabled or
disabled, with the expectation that the MAC have LPI disabled during
probe.

Support for phylink managed EEE is enabled by populating both tx_lpi
MAC operations method pointers.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 123 ++++++++++++++++++++++++++++++++++++--
 include/linux/phylink.h   |  44 ++++++++++++++
 2 files changed, 163 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 03509fdaa1ec..750356b6a2e9 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -81,12 +81,19 @@ struct phylink {
 	unsigned int pcs_state;
 
 	bool link_failed;
+	bool mac_supports_eee;
+	bool phy_enable_tx_lpi;
+	bool mac_enable_tx_lpi;
+	bool mac_tx_clk_stop;
+	u32 mac_tx_lpi_timer;
 
 	struct sfp_bus *sfp_bus;
 	bool sfp_may_have_phy;
 	DECLARE_PHY_INTERFACE_MASK(sfp_interfaces);
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(sfp_support);
 	u8 sfp_port;
+
+	struct eee_config eee_cfg;
 };
 
 #define phylink_printk(level, pl, fmt, ...) \
@@ -1563,6 +1570,47 @@ static const char *phylink_pause_to_str(int pause)
 	}
 }
 
+static void phylink_deactivate_lpi(struct phylink *pl)
+{
+	if (pl->mac_enable_tx_lpi) {
+		pl->mac_enable_tx_lpi = false;
+
+		phylink_dbg(pl, "disabling LPI\n");
+
+		pl->mac_ops->mac_disable_tx_lpi(pl->config);
+	}
+}
+
+static void phylink_activate_lpi(struct phylink *pl)
+{
+	if (!test_bit(pl->cur_interface, pl->config->lpi_interfaces)) {
+		phylink_dbg(pl, "MAC does not support LPI with %s\n",
+			    phy_modes(pl->cur_interface));
+		return;
+	}
+
+	phylink_dbg(pl, "LPI timer %uus, tx clock stop %u\n",
+		    pl->mac_tx_lpi_timer, pl->mac_tx_clk_stop);
+
+	pl->mac_ops->mac_enable_tx_lpi(pl->config, pl->mac_tx_lpi_timer,
+				       pl->mac_tx_clk_stop);
+
+	pl->mac_enable_tx_lpi = true;
+}
+
+static void phylink_phy_restrict_eee(struct phylink *pl, struct phy_device *phy)
+{
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(eee_supported);
+
+	/* Convert the MAC's LPI capabilities to linkmodes */
+	linkmode_zero(eee_supported);
+	phylink_caps_to_linkmodes(eee_supported, pl->config->lpi_capabilities);
+
+	/* Mask out EEE modes that are not supported */
+	linkmode_and(phy->supported_eee, phy->supported_eee, eee_supported);
+	linkmode_and(phy->advertising_eee, phy->advertising_eee, eee_supported);
+}
+
 static void phylink_link_up(struct phylink *pl,
 			    struct phylink_link_state link_state)
 {
@@ -1609,6 +1657,9 @@ static void phylink_link_up(struct phylink *pl,
 				 pl->cur_interface, speed, duplex,
 				 !!(link_state.pause & MLO_PAUSE_TX), rx_pause);
 
+	if (pl->mac_supports_eee && pl->phy_enable_tx_lpi)
+		phylink_activate_lpi(pl);
+
 	if (ndev)
 		netif_carrier_on(ndev);
 
@@ -1625,6 +1676,9 @@ static void phylink_link_down(struct phylink *pl)
 
 	if (ndev)
 		netif_carrier_off(ndev);
+
+	phylink_deactivate_lpi(pl);
+
 	pl->mac_ops->mac_link_down(pl->config, pl->act_link_an_mode,
 				   pl->cur_interface);
 	phylink_info(pl, "Link is Down\n");
@@ -1888,6 +1942,14 @@ struct phylink *phylink_create(struct phylink_config *config,
 		return ERR_PTR(-EINVAL);
 	}
 
+	pl->mac_supports_eee = mac_ops->mac_disable_tx_lpi &&
+			       mac_ops->mac_enable_tx_lpi;
+
+	/* Set the default EEE configuration */
+	pl->eee_cfg.eee_enabled = pl->config->eee_enabled_default;
+	pl->eee_cfg.tx_lpi_enabled = pl->eee_cfg.eee_enabled;
+	pl->eee_cfg.tx_lpi_timer = pl->config->lpi_timer_default;
+
 	pl->phy_state.interface = iface;
 	pl->link_interface = iface;
 	if (iface == PHY_INTERFACE_MODE_MOCA)
@@ -1992,16 +2054,22 @@ static void phylink_phy_change(struct phy_device *phydev, bool up)
 	pl->phy_state.link = up;
 	if (!up)
 		pl->link_failed = true;
+
+	/* Get the LPI state from phylib */
+	pl->phy_enable_tx_lpi = phydev->enable_tx_lpi;
+	pl->mac_tx_lpi_timer = phydev->eee_cfg.tx_lpi_timer;
 	mutex_unlock(&pl->state_mutex);
 
 	phylink_run_resolve(pl);
 
-	phylink_dbg(pl, "phy link %s %s/%s/%s/%s/%s\n", up ? "up" : "down",
+	phylink_dbg(pl, "phy link %s %s/%s/%s/%s/%s/%slpi\n",
+		    up ? "up" : "down",
 		    phy_modes(phydev->interface),
 		    phy_speed_to_str(phydev->speed),
 		    phy_duplex_to_str(phydev->duplex),
 		    phy_rate_matching_to_str(phydev->rate_matching),
-		    phylink_pause_to_str(pl->phy_state.pause));
+		    phylink_pause_to_str(pl->phy_state.pause),
+		    phydev->enable_tx_lpi ? "" : "no");
 }
 
 static int phylink_validate_phy(struct phylink *pl, struct phy_device *phy,
@@ -2131,6 +2199,28 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 
 	/* Restrict the phy advertisement according to the MAC support. */
 	linkmode_copy(phy->advertising, config.advertising);
+
+	/* If the MAC supports phylink managed EEE, restrict the EEE
+	 * advertisement according to the MAC's LPI capabilities.
+	 */
+	if (pl->mac_supports_eee) {
+		phy->eee_cfg.eee_enabled = pl->eee_cfg.eee_enabled;
+
+		/* If EEE is enabled, then we need to call phy_support_eee()
+		 * to ensure that the advertising mask is appropriately set.
+		 */
+		if (pl->eee_cfg.eee_enabled)
+			phy_support_eee(phy);
+
+		phy->eee_cfg.tx_lpi_enabled = pl->eee_cfg.tx_lpi_enabled;
+		phy->eee_cfg.tx_lpi_timer = pl->eee_cfg.tx_lpi_timer;
+
+		/* Restrict the PHYs EEE support/advertisement to the modes
+		 * that the MAC supports.
+		 */
+		phylink_phy_restrict_eee(pl, phy);
+	}
+
 	mutex_unlock(&pl->state_mutex);
 	mutex_unlock(&phy->lock);
 
@@ -2146,7 +2236,13 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 	if (pl->config->mac_managed_pm)
 		phy->mac_managed_pm = true;
 
-	return 0;
+	/* Allow the MAC to stop its clock if the PHY has the capability */
+	pl->mac_tx_clk_stop = phy_eee_tx_clock_stop_capable(phy) > 0;
+
+	/* Explicitly configure whether the PHY is allowed to stop it's
+	 * receive clock.
+	 */
+	return phy_eee_rx_clock_stop(phy, pl->config->eee_rx_clk_stop_enable);
 }
 
 static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy,
@@ -2303,6 +2399,8 @@ void phylink_disconnect_phy(struct phylink *pl)
 		mutex_lock(&phy->lock);
 		mutex_lock(&pl->state_mutex);
 		pl->phydev = NULL;
+		pl->phy_enable_tx_lpi = false;
+		pl->mac_tx_clk_stop = false;
 		mutex_unlock(&pl->state_mutex);
 		mutex_unlock(&phy->lock);
 		flush_work(&pl->resolve);
@@ -3071,12 +3169,29 @@ EXPORT_SYMBOL_GPL(phylink_ethtool_get_eee);
  */
 int phylink_ethtool_set_eee(struct phylink *pl, struct ethtool_keee *eee)
 {
+	bool mac_eee = pl->mac_supports_eee;
 	int ret = -EOPNOTSUPP;
 
 	ASSERT_RTNL();
 
-	if (pl->phydev)
+	phylink_dbg(pl, "mac %s phylink EEE%s, adv %*pbl, LPI%s timer %uus\n",
+		    mac_eee ? "supports" : "does not support",
+		    eee->eee_enabled ? ", enabled" : "",
+		    __ETHTOOL_LINK_MODE_MASK_NBITS, eee->advertised,
+		    eee->tx_lpi_enabled ? " enabled" : "", eee->tx_lpi_timer);
+
+	/* Clamp the LPI timer maximum value */
+	if (mac_eee && eee->tx_lpi_timer > pl->config->lpi_timer_limit_us) {
+		eee->tx_lpi_timer = pl->config->lpi_timer_limit_us;
+		phylink_dbg(pl, "LPI timer limited to %uus\n",
+			    eee->tx_lpi_timer);
+	}
+
+	if (pl->phydev) {
 		ret = phy_ethtool_set_eee(pl->phydev, eee);
+		if (ret == 0)
+			eee_to_eeecfg(&pl->eee_cfg, eee);
+	}
 
 	return ret;
 }
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 5462cc6a37dc..f72f0a1feb89 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -5,6 +5,8 @@
 #include <linux/spinlock.h>
 #include <linux/workqueue.h>
 
+#include <net/eee.h>
+
 struct device_node;
 struct ethtool_cmd;
 struct fwnode_handle;
@@ -143,11 +145,17 @@ enum phylink_op_type {
  *                    possible and avoid stopping it during suspend events.
  * @default_an_inband: if true, defaults to MLO_AN_INBAND rather than
  *		       MLO_AN_PHY. A fixed-link specification will override.
+ * @eee_rx_clk_stop_enable: if true, PHY can stop the receive clock during LPI
  * @get_fixed_state: callback to execute to determine the fixed link state,
  *		     if MAC link is at %MLO_AN_FIXED mode.
  * @supported_interfaces: bitmap describing which PHY_INTERFACE_MODE_xxx
  *                        are supported by the MAC/PCS.
+ * @lpi_interfaces: bitmap describing which PHY interface modes can support
+ *		    LPI signalling.
  * @mac_capabilities: MAC pause/speed/duplex capabilities.
+ * @lpi_capabilities: MAC speeds which can support LPI signalling
+ * @eee: default EEE configuration.
+ * @lpi_timer_limit_us: Maximum (inclusive) value of the EEE LPI timer.
  */
 struct phylink_config {
 	struct device *dev;
@@ -156,10 +164,16 @@ struct phylink_config {
 	bool mac_managed_pm;
 	bool mac_requires_rxc;
 	bool default_an_inband;
+	bool eee_rx_clk_stop_enable;
 	void (*get_fixed_state)(struct phylink_config *config,
 				struct phylink_link_state *state);
 	DECLARE_PHY_INTERFACE_MASK(supported_interfaces);
+	DECLARE_PHY_INTERFACE_MASK(lpi_interfaces);
 	unsigned long mac_capabilities;
+	unsigned long lpi_capabilities;
+	u32 lpi_timer_limit_us;
+	u32 lpi_timer_default;
+	bool eee_enabled_default;
 };
 
 void phylink_limit_mac_speed(struct phylink_config *config, u32 max_speed);
@@ -173,6 +187,8 @@ void phylink_limit_mac_speed(struct phylink_config *config, u32 max_speed);
  * @mac_finish: finish a major reconfiguration of the interface.
  * @mac_link_down: take the link down.
  * @mac_link_up: allow the link to come up.
+ * @mac_disable_tx_lpi: disable LPI.
+ * @mac_enable_tx_lpi: enable and configure LPI.
  *
  * The individual methods are described more fully below.
  */
@@ -193,6 +209,9 @@ struct phylink_mac_ops {
 			    struct phy_device *phy, unsigned int mode,
 			    phy_interface_t interface, int speed, int duplex,
 			    bool tx_pause, bool rx_pause);
+	void (*mac_disable_tx_lpi)(struct phylink_config *config);
+	void (*mac_enable_tx_lpi)(struct phylink_config *config, u32 timer,
+				  bool tx_clk_stop);
 };
 
 #if 0 /* For kernel-doc purposes only. */
@@ -387,6 +406,31 @@ void mac_link_down(struct phylink_config *config, unsigned int mode,
 void mac_link_up(struct phylink_config *config, struct phy_device *phy,
 		 unsigned int mode, phy_interface_t interface,
 		 int speed, int duplex, bool tx_pause, bool rx_pause);
+
+/**
+ * mac_disable_tx_lpi() - disable LPI generation at the MAC
+ * @config: a pointer to a &struct phylink_config.
+ *
+ * Disable generation of LPI at the MAC, effectively preventing the MAC
+ * from indicating that it is idle.
+ */
+void mac_disable_tx_lpi(struct phylink_config *config);
+
+/**
+ * mac_enable_tx_lpi() - configure and enable LPI generation at the MAC
+ * @config: a pointer to a &struct phylink_config.
+ * @timer: LPI timeout in microseconds.
+ * @tx_clk_stop: allow xMII transmit clock to be stopped during LPI
+ *
+ * Configure the LPI timeout accordingly. This will only be called when
+ * the link is already up, to cater for situations where the hardware
+ * needs to be programmed according to the link speed.
+ *
+ * Enable LPI generation at the MAC, and configure whether the xMII transmit
+ * clock may be stopped.
+ */
+void mac_enable_tx_lpi(struct phylink_config *config, u32 timer,
+		       bool tx_clk_stop);
 #endif
 
 struct phylink_pcs_ops;
-- 
2.30.2

Re: [PATCH net-next 05/10] net: phylink: add EEE management

[Andrew Lunn]

> It will also call into the MAC driver when LPI needs to be enabled or
> disabled, with the expectation that the MAC have LPI disabled during
> probe.

How important is this expectation? I don't see it documented anywhere.

	Andrew

Re: [PATCH net-next 05/10] net: phylink: add EEE management

[Simon Horman]

On Mon, Dec 09, 2024 at 02:23:38PM +0000, Russell King (Oracle) wrote:
> Add EEE management to phylink, making use of the phylib implementation.
> This will only be used where a MAC driver populates the methods and
> capabilities bitfield, otherwise we keep our old behaviour.
> 
> Phylink will keep track of the EEE configuration, including the clock
> stop abilities at each end of the MAC to PHY link, programming the PHY
> appropriately and preserving the EEE configuration should the PHY go
> away.
> 
> It will also call into the MAC driver when LPI needs to be enabled or
> disabled, with the expectation that the MAC have LPI disabled during
> probe.
> 
> Support for phylink managed EEE is enabled by populating both tx_lpi
> MAC operations method pointers.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

...

> diff --git a/include/linux/phylink.h b/include/linux/phylink.h

...

> @@ -143,11 +145,17 @@ enum phylink_op_type {
>   *                    possible and avoid stopping it during suspend events.
>   * @default_an_inband: if true, defaults to MLO_AN_INBAND rather than
>   *		       MLO_AN_PHY. A fixed-link specification will override.
> + * @eee_rx_clk_stop_enable: if true, PHY can stop the receive clock during LPI
>   * @get_fixed_state: callback to execute to determine the fixed link state,
>   *		     if MAC link is at %MLO_AN_FIXED mode.
>   * @supported_interfaces: bitmap describing which PHY_INTERFACE_MODE_xxx
>   *                        are supported by the MAC/PCS.
> + * @lpi_interfaces: bitmap describing which PHY interface modes can support
> + *		    LPI signalling.
>   * @mac_capabilities: MAC pause/speed/duplex capabilities.
> + * @lpi_capabilities: MAC speeds which can support LPI signalling
> + * @eee: default EEE configuration.
> + * @lpi_timer_limit_us: Maximum (inclusive) value of the EEE LPI timer.
>   */
>  struct phylink_config {
>  	struct device *dev;
> @@ -156,10 +164,16 @@ struct phylink_config {
>  	bool mac_managed_pm;
>  	bool mac_requires_rxc;
>  	bool default_an_inband;
> +	bool eee_rx_clk_stop_enable;
>  	void (*get_fixed_state)(struct phylink_config *config,
>  				struct phylink_link_state *state);
>  	DECLARE_PHY_INTERFACE_MASK(supported_interfaces);
> +	DECLARE_PHY_INTERFACE_MASK(lpi_interfaces);
>  	unsigned long mac_capabilities;
> +	unsigned long lpi_capabilities;
> +	u32 lpi_timer_limit_us;
> +	u32 lpi_timer_default;
> +	bool eee_enabled_default;
>  };

Hi Russell,

A minor nit from my side.

The Kernel doc updates don't correspond entirely to the structure updates:
- @eee is documented but not present in the structure
- Conversely, @lpi_timer_default and @ee_enabled_default are
  present in the structure but not documented.

...

Re: [PATCH net-next 05/10] net: phylink: add EEE management

[Heiner Kallweit]

On 09.12.2024 15:23, Russell King (Oracle) wrote:
> Add EEE management to phylink, making use of the phylib implementation.
> This will only be used where a MAC driver populates the methods and
> capabilities bitfield, otherwise we keep our old behaviour.
> 
> Phylink will keep track of the EEE configuration, including the clock
> stop abilities at each end of the MAC to PHY link, programming the PHY
> appropriately and preserving the EEE configuration should the PHY go
> away.
> 
> It will also call into the MAC driver when LPI needs to be enabled or
> disabled, with the expectation that the MAC have LPI disabled during
> probe.
> 
> Support for phylink managed EEE is enabled by populating both tx_lpi
> MAC operations method pointers.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/phy/phylink.c | 123 ++++++++++++++++++++++++++++++++++++--
>  include/linux/phylink.h   |  44 ++++++++++++++
>  2 files changed, 163 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 03509fdaa1ec..750356b6a2e9 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -81,12 +81,19 @@ struct phylink {
>  	unsigned int pcs_state;
>  
>  	bool link_failed;
> +	bool mac_supports_eee;
> +	bool phy_enable_tx_lpi;
> +	bool mac_enable_tx_lpi;
> +	bool mac_tx_clk_stop;
> +	u32 mac_tx_lpi_timer;
>  
>  	struct sfp_bus *sfp_bus;
>  	bool sfp_may_have_phy;
>  	DECLARE_PHY_INTERFACE_MASK(sfp_interfaces);
>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(sfp_support);
>  	u8 sfp_port;
> +
> +	struct eee_config eee_cfg;
>  };
>  
>  #define phylink_printk(level, pl, fmt, ...) \
> @@ -1563,6 +1570,47 @@ static const char *phylink_pause_to_str(int pause)
>  	}
>  }
>  
> +static void phylink_deactivate_lpi(struct phylink *pl)
> +{
> +	if (pl->mac_enable_tx_lpi) {
> +		pl->mac_enable_tx_lpi = false;
> +
> +		phylink_dbg(pl, "disabling LPI\n");
> +
> +		pl->mac_ops->mac_disable_tx_lpi(pl->config);
> +	}
> +}
> +
> +static void phylink_activate_lpi(struct phylink *pl)
> +{
> +	if (!test_bit(pl->cur_interface, pl->config->lpi_interfaces)) {
> +		phylink_dbg(pl, "MAC does not support LPI with %s\n",
> +			    phy_modes(pl->cur_interface));
> +		return;
> +	}
> +
> +	phylink_dbg(pl, "LPI timer %uus, tx clock stop %u\n",
> +		    pl->mac_tx_lpi_timer, pl->mac_tx_clk_stop);
> +
> +	pl->mac_ops->mac_enable_tx_lpi(pl->config, pl->mac_tx_lpi_timer,
> +				       pl->mac_tx_clk_stop);
> +
> +	pl->mac_enable_tx_lpi = true;
> +}
> +
> +static void phylink_phy_restrict_eee(struct phylink *pl, struct phy_device *phy)
> +{
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(eee_supported);
> +
> +	/* Convert the MAC's LPI capabilities to linkmodes */
> +	linkmode_zero(eee_supported);
> +	phylink_caps_to_linkmodes(eee_supported, pl->config->lpi_capabilities);
> +
> +	/* Mask out EEE modes that are not supported */
> +	linkmode_and(phy->supported_eee, phy->supported_eee, eee_supported);
> +	linkmode_and(phy->advertising_eee, phy->advertising_eee, eee_supported);
> +}
> +

Something similar we may need in phylib too. An example is cpsw MAC driver which
doesn't support EEE. Issues have been reported if the PHY's on both sides negotiate
EEE, workaround is to use property eee-broken-xxx in the respective DT's to disable
PHY EEE advertisement. I'm thinking of adding a simple phy_disable_eee() which can
be called by MAC drivers to clear EEE supported and advertising bitmaps.

A similar case is enetc (using phylink) which doesn't support EEE. See following in
enetc.c:

/* disable EEE autoneg, until ENETC driver supports it */
memset(&edata, 0, sizeof(struct ethtool_keee));
phylink_ethtool_set_eee(priv->phylink, &edata);

Russell, do you plan to change this driver too, based on phylink extensions?
I think already now, based on the quoted code piece, several (all?) eee-broken-xxx
properties can be removed under arch/arm64/boot/dts/freescale .

Re: [PATCH net-next 05/10] net: phylink: add EEE management

[Russell King (Oracle)]

On Sun, Dec 15, 2024 at 12:38:24AM +0100, Heiner Kallweit wrote:
> On 09.12.2024 15:23, Russell King (Oracle) wrote:
> > Add EEE management to phylink, making use of the phylib implementation.
> > This will only be used where a MAC driver populates the methods and
> > capabilities bitfield, otherwise we keep our old behaviour.
> > 
> > Phylink will keep track of the EEE configuration, including the clock
> > stop abilities at each end of the MAC to PHY link, programming the PHY
> > appropriately and preserving the EEE configuration should the PHY go
> > away.
> > 
> > It will also call into the MAC driver when LPI needs to be enabled or
> > disabled, with the expectation that the MAC have LPI disabled during
> > probe.
> > 
> > Support for phylink managed EEE is enabled by populating both tx_lpi
> > MAC operations method pointers.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/net/phy/phylink.c | 123 ++++++++++++++++++++++++++++++++++++--
> >  include/linux/phylink.h   |  44 ++++++++++++++
> >  2 files changed, 163 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index 03509fdaa1ec..750356b6a2e9 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -81,12 +81,19 @@ struct phylink {
> >  	unsigned int pcs_state;
> >  
> >  	bool link_failed;
> > +	bool mac_supports_eee;
> > +	bool phy_enable_tx_lpi;
> > +	bool mac_enable_tx_lpi;
> > +	bool mac_tx_clk_stop;
> > +	u32 mac_tx_lpi_timer;
> >  
> >  	struct sfp_bus *sfp_bus;
> >  	bool sfp_may_have_phy;
> >  	DECLARE_PHY_INTERFACE_MASK(sfp_interfaces);
> >  	__ETHTOOL_DECLARE_LINK_MODE_MASK(sfp_support);
> >  	u8 sfp_port;
> > +
> > +	struct eee_config eee_cfg;
> >  };
> >  
> >  #define phylink_printk(level, pl, fmt, ...) \
> > @@ -1563,6 +1570,47 @@ static const char *phylink_pause_to_str(int pause)
> >  	}
> >  }
> >  
> > +static void phylink_deactivate_lpi(struct phylink *pl)
> > +{
> > +	if (pl->mac_enable_tx_lpi) {
> > +		pl->mac_enable_tx_lpi = false;
> > +
> > +		phylink_dbg(pl, "disabling LPI\n");
> > +
> > +		pl->mac_ops->mac_disable_tx_lpi(pl->config);
> > +	}
> > +}
> > +
> > +static void phylink_activate_lpi(struct phylink *pl)
> > +{
> > +	if (!test_bit(pl->cur_interface, pl->config->lpi_interfaces)) {
> > +		phylink_dbg(pl, "MAC does not support LPI with %s\n",
> > +			    phy_modes(pl->cur_interface));
> > +		return;
> > +	}
> > +
> > +	phylink_dbg(pl, "LPI timer %uus, tx clock stop %u\n",
> > +		    pl->mac_tx_lpi_timer, pl->mac_tx_clk_stop);
> > +
> > +	pl->mac_ops->mac_enable_tx_lpi(pl->config, pl->mac_tx_lpi_timer,
> > +				       pl->mac_tx_clk_stop);
> > +
> > +	pl->mac_enable_tx_lpi = true;
> > +}
> > +
> > +static void phylink_phy_restrict_eee(struct phylink *pl, struct phy_device *phy)
> > +{
> > +	__ETHTOOL_DECLARE_LINK_MODE_MASK(eee_supported);
> > +
> > +	/* Convert the MAC's LPI capabilities to linkmodes */
> > +	linkmode_zero(eee_supported);
> > +	phylink_caps_to_linkmodes(eee_supported, pl->config->lpi_capabilities);
> > +
> > +	/* Mask out EEE modes that are not supported */
> > +	linkmode_and(phy->supported_eee, phy->supported_eee, eee_supported);
> > +	linkmode_and(phy->advertising_eee, phy->advertising_eee, eee_supported);
> > +}
> > +
> 
> Something similar we may need in phylib too. An example is cpsw MAC driver which
> doesn't support EEE. Issues have been reported if the PHY's on both sides negotiate
> EEE, workaround is to use property eee-broken-xxx in the respective DT's to disable
> PHY EEE advertisement. I'm thinking of adding a simple phy_disable_eee() which can
> be called by MAC drivers to clear EEE supported and advertising bitmaps.
> 
> A similar case is enetc (using phylink) which doesn't support EEE. See following in
> enetc.c:
> 
> /* disable EEE autoneg, until ENETC driver supports it */
> memset(&edata, 0, sizeof(struct ethtool_keee));
> phylink_ethtool_set_eee(priv->phylink, &edata);
> 
> Russell, do you plan to change this driver too, based on phylink extensions?
> I think already now, based on the quoted code piece, several (all?) eee-broken-xxx
> properties can be removed under arch/arm64/boot/dts/freescale .

At the moment, phylink-managed EEE is opt-in, so what you get without
this patch is what you get with the patch but no driver changes. This
allows existing drivers that support EEE and that use phylink to
continue to support EEE, and those that don't to continue in their
current situation (if they use work-arounds to disable EEE, then
those will continue to work.)

Rather than adding something explicit to phylink to disable EEE, I
would much rather we work towards a situation where, if a driver
uses phylink, then if EEE is supported, the EEE methods and other
configuration get populated. If not, then phylink disables EEE on
the PHY.

However, we can only get there once we have EEE implemented on all
the phylink-using drivers that currently support EEE.

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

[PATCH net-next 06/10] net: phylink: allow MAC driver to validate eee params

[Russell King (Oracle)]

Allow the MAC driver to validate EEE parameters before accepting the
set_eee() call.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 16 +++++++++++++++-
 include/linux/phylink.h   | 15 +++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 750356b6a2e9..995bfbbd18e9 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1570,6 +1570,14 @@ static const char *phylink_pause_to_str(int pause)
 	}
 }
 
+static int phylink_validate_tx_lpi(struct phylink *pl, struct ethtool_keee *eee)
+{
+	if (!pl->mac_ops->mac_validate_tx_lpi)
+		return 0;
+
+	return pl->mac_ops->mac_validate_tx_lpi(pl->config, eee);
+}
+
 static void phylink_deactivate_lpi(struct phylink *pl)
 {
 	if (pl->mac_enable_tx_lpi) {
@@ -3170,7 +3178,7 @@ EXPORT_SYMBOL_GPL(phylink_ethtool_get_eee);
 int phylink_ethtool_set_eee(struct phylink *pl, struct ethtool_keee *eee)
 {
 	bool mac_eee = pl->mac_supports_eee;
-	int ret = -EOPNOTSUPP;
+	int ret;
 
 	ASSERT_RTNL();
 
@@ -3187,6 +3195,12 @@ int phylink_ethtool_set_eee(struct phylink *pl, struct ethtool_keee *eee)
 			    eee->tx_lpi_timer);
 	}
 
+	ret = phylink_validate_tx_lpi(pl, eee);
+	if (ret)
+		return ret;
+
+	ret = -EOPNOTSUPP;
+
 	if (pl->phydev) {
 		ret = phy_ethtool_set_eee(pl->phydev, eee);
 		if (ret == 0)
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index f72f0a1feb89..03e790579203 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -187,6 +187,7 @@ void phylink_limit_mac_speed(struct phylink_config *config, u32 max_speed);
  * @mac_finish: finish a major reconfiguration of the interface.
  * @mac_link_down: take the link down.
  * @mac_link_up: allow the link to come up.
+ * @mac_validate_tx_lpi: validate LPI configuration.
  * @mac_disable_tx_lpi: disable LPI.
  * @mac_enable_tx_lpi: enable and configure LPI.
  *
@@ -209,6 +210,8 @@ struct phylink_mac_ops {
 			    struct phy_device *phy, unsigned int mode,
 			    phy_interface_t interface, int speed, int duplex,
 			    bool tx_pause, bool rx_pause);
+	int (*mac_validate_tx_lpi)(struct phylink_config *config,
+				   struct ethtool_keee *e);
 	void (*mac_disable_tx_lpi)(struct phylink_config *config);
 	void (*mac_enable_tx_lpi)(struct phylink_config *config, u32 timer,
 				  bool tx_clk_stop);
@@ -407,6 +410,18 @@ void mac_link_up(struct phylink_config *config, struct phy_device *phy,
 		 unsigned int mode, phy_interface_t interface,
 		 int speed, int duplex, bool tx_pause, bool rx_pause);
 
+/**
+ * mac_validate_tx_lpi() - validate the LPI parameters in EEE
+ * @config: a pointer to a &struct phylink_config.
+ * @e: EEE configuration to be validated.
+ *
+ * Validate the LPI configuration parameters in @e, returning an appropriate
+ * error. This will be called prior to any changes being made, and must not
+ * make any changes to existing configuration. Returns zero on success.
+ */
+int (*mac_validate_tx_lpi)(struct phylink_config *config,
+			   struct ethtool_keee *e);
+
 /**
  * mac_disable_tx_lpi() - disable LPI generation at the MAC
  * @config: a pointer to a &struct phylink_config.
-- 
2.30.2

Re: [PATCH net-next 06/10] net: phylink: allow MAC driver to validate eee params

[Andrew Lunn]

> +/**
> + * mac_validate_tx_lpi() - validate the LPI parameters in EEE
> + * @config: a pointer to a &struct phylink_config.
> + * @e: EEE configuration to be validated.
> + *
> + * Validate the LPI configuration parameters in @e, returning an appropriate
> + * error. This will be called prior to any changes being made, and must not
> + * make any changes to existing configuration. Returns zero on success.

Maybe suggest -EOPNOTSUPP? We might then get more uniform error codes
from MAC drivers?

	Andrew

Re: [PATCH net-next 06/10] net: phylink: allow MAC driver to validate eee params

[Russell King (Oracle)]

On Tue, Dec 10, 2024 at 04:21:51AM +0100, Andrew Lunn wrote:
> > +/**
> > + * mac_validate_tx_lpi() - validate the LPI parameters in EEE
> > + * @config: a pointer to a &struct phylink_config.
> > + * @e: EEE configuration to be validated.
> > + *
> > + * Validate the LPI configuration parameters in @e, returning an appropriate
> > + * error. This will be called prior to any changes being made, and must not
> > + * make any changes to existing configuration. Returns zero on success.
> 
> Maybe suggest -EOPNOTSUPP? We might then get more uniform error codes
> from MAC drivers?

-EOPNOTSUPP would be appropriate if the driver doesn't support EEE at
all. Other errors are more appropriate when validating the value of
the parameters (e.g. tx_lpi_timer.)

However, we probably need to have a discussion about the best way to
handle tx_lpi_timer values that the hardware can't deal with. Should we
handle it with a hrtimer? If so, we probably need mac_enable_tx_lpi() to
return an error code, so that the core knows that the hardware can't
cope with the value at a particular speed, and needs to switch to a
software timer (I'm not currently sure how complex that would be to
implement, but I think stmmac takes this approach.)

That could make mac_validate_tx_lpi() redundant, but then I have a
stack of DSA patches that could use this callback to indicate that
EEE is not supported.

I don't have all the answers for this, so I was hoping to kick off a
discussion in the RFC patchset before submitting something that could
be merged, but that didn't happen.

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

Re: [PATCH net-next 06/10] net: phylink: allow MAC driver to validate eee params

[Russell King (Oracle)]

On Tue, Dec 10, 2024 at 09:58:40AM +0000, Russell King (Oracle) wrote:
> That could make mac_validate_tx_lpi() redundant, but then I have a
> stack of DSA patches that could use this callback to indicate that
> EEE is not supported.

Scratch that idea - DSA also needs the ethtool get_eee() method to
return -EOPNOTSUPP, which mac_validate_tx_lpi() wouldn't cover.

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

[PATCH net-next 07/10] net: mvneta: convert to phylink EEE implementation

[Russell King (Oracle)]

Convert mvneta to use phylink's EEE implementation, which means we just
need to implement the two methods for LPI control, and adding the
initial configuration.

Disabling LPI requires clearing a single bit. Enabling LPI needs a full
configuration of several values, as the timer values are dependent on
the MAC operating speed.

As Armada 388 states that EEE is only supported in "SGMII" modes, mark
this in lpi_interfaces. Testing with RGMII on the Clearfog platform
indicates that the receive path fails to detect LPI over RGMII.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/marvell/mvneta.c | 127 ++++++++++++++++----------
 1 file changed, 79 insertions(+), 48 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index fe6261b81540..2b0758c11664 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -284,8 +284,12 @@
 					  MVNETA_TXQ_BUCKET_REFILL_PERIOD))
 
 #define MVNETA_LPI_CTRL_0                        0x2cc0
+#define      MVNETA_LPI_CTRL_0_TS                (0xff << 8)
 #define MVNETA_LPI_CTRL_1                        0x2cc4
-#define      MVNETA_LPI_REQUEST_ENABLE           BIT(0)
+#define      MVNETA_LPI_CTRL_1_REQUEST_ENABLE    BIT(0)
+#define      MVNETA_LPI_CTRL_1_REQUEST_FORCE     BIT(1)
+#define      MVNETA_LPI_CTRL_1_MANUAL_MODE       BIT(2)
+#define      MVNETA_LPI_CTRL_1_TW                (0xfff << 4)
 #define MVNETA_LPI_CTRL_2                        0x2cc8
 #define MVNETA_LPI_STATUS                        0x2ccc
 
@@ -541,10 +545,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)];
 
 	u32 indir[MVNETA_RSS_LU_TABLE_SIZE];
@@ -1354,6 +1354,13 @@ static void mvneta_port_enable(struct mvneta_port *pp)
 	val = mvreg_read(pp, MVNETA_GMAC_CTRL_0);
 	val |= MVNETA_GMAC0_PORT_ENABLE;
 	mvreg_write(pp, MVNETA_GMAC_CTRL_0, val);
+
+	/* Ensure LPI is disabled */
+	val = mvreg_read(pp, MVNETA_LPI_CTRL_1);
+	val &= ~(MVNETA_LPI_CTRL_1_REQUEST_ENABLE |
+		 MVNETA_LPI_CTRL_1_REQUEST_FORCE |
+		 MVNETA_LPI_CTRL_1_MANUAL_MODE);
+	mvreg_write(pp, MVNETA_LPI_CTRL_1, val);
 }
 
 /* Disable the port and wait for about 200 usec before retuning */
@@ -4213,18 +4220,6 @@ static int mvneta_mac_finish(struct phylink_config *config, unsigned int mode,
 	return 0;
 }
 
-static void mvneta_set_eee(struct mvneta_port *pp, bool enable)
-{
-	u32 lpi_ctl1;
-
-	lpi_ctl1 = mvreg_read(pp, MVNETA_LPI_CTRL_1);
-	if (enable)
-		lpi_ctl1 |= MVNETA_LPI_REQUEST_ENABLE;
-	else
-		lpi_ctl1 &= ~MVNETA_LPI_REQUEST_ENABLE;
-	mvreg_write(pp, MVNETA_LPI_CTRL_1, lpi_ctl1);
-}
-
 static void mvneta_mac_link_down(struct phylink_config *config,
 				 unsigned int mode, phy_interface_t interface)
 {
@@ -4240,9 +4235,6 @@ static void mvneta_mac_link_down(struct phylink_config *config,
 		val |= MVNETA_GMAC_FORCE_LINK_DOWN;
 		mvreg_write(pp, MVNETA_GMAC_AUTONEG_CONFIG, val);
 	}
-
-	pp->eee_active = false;
-	mvneta_set_eee(pp, false);
 }
 
 static void mvneta_mac_link_up(struct phylink_config *config,
@@ -4291,11 +4283,61 @@ static void mvneta_mac_link_up(struct phylink_config *config,
 	}
 
 	mvneta_port_up(pp);
+}
+
+static int mvneta_mac_validate_tx_lpi(struct phylink_config *config,
+				      struct ethtool_keee *eee)
+{
+	if (eee->tx_lpi_timer > 255)
+		return -EINVAL;
+
+	return 0;
+}
+
+static void mvneta_mac_disable_tx_lpi(struct phylink_config *config)
+{
+	struct mvneta_port *pp = netdev_priv(to_net_dev(config->dev));
+	u32 lpi1;
+
+	lpi1 = mvreg_read(pp, MVNETA_LPI_CTRL_1);
+	lpi1 &= ~MVNETA_LPI_CTRL_1_REQUEST_ENABLE;
+	mvreg_write(pp, MVNETA_LPI_CTRL_1, lpi1);
+}
 
-	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);
+static void mvneta_mac_enable_tx_lpi(struct phylink_config *config, u32 timer,
+				     bool tx_clk_stop)
+{
+	struct mvneta_port *pp = netdev_priv(to_net_dev(config->dev));
+	u32 ts, tw, lpi0, lpi1, status;
+
+	status = mvreg_read(pp, MVNETA_GMAC_STATUS);
+	if (status & MVNETA_GMAC_SPEED_1000) {
+		/* At 1G speeds, the timer resolution are 1us, and
+		 * 802.3 says tw is 16.5us. Round up to 17us.
+		 */
+		tw = 17;
+		ts = timer;
+	} else {
+		/* At 100M speeds, the timer resolutions are 10us, and
+		 * 802.3 says tw is 30us.
+		 */
+		tw = 3;
+		ts = DIV_ROUND_UP(timer, 10);
 	}
+
+	if (ts > 255)
+		ts = 255;
+
+	/* Configure ts */
+	lpi0 = mvreg_read(pp, MVNETA_LPI_CTRL_0);
+	lpi0 = u32_replace_bits(lpi0, MVNETA_LPI_CTRL_0_TS, ts);
+	mvreg_write(pp, MVNETA_LPI_CTRL_0, lpi0);
+
+	/* Configure tw and enable LPI generation */
+	lpi1 = mvreg_read(pp, MVNETA_LPI_CTRL_1);
+	lpi1 = u32_replace_bits(lpi1, MVNETA_LPI_CTRL_1_TW, tw);
+	lpi1 |= MVNETA_LPI_CTRL_1_REQUEST_ENABLE;
+	mvreg_write(pp, MVNETA_LPI_CTRL_1, lpi1);
 }
 
 static const struct phylink_mac_ops mvneta_phylink_ops = {
@@ -4305,6 +4347,9 @@ static const struct phylink_mac_ops mvneta_phylink_ops = {
 	.mac_finish = mvneta_mac_finish,
 	.mac_link_down = mvneta_mac_link_down,
 	.mac_link_up = mvneta_mac_link_up,
+	.mac_validate_tx_lpi = mvneta_mac_validate_tx_lpi,
+	.mac_disable_tx_lpi = mvneta_mac_disable_tx_lpi,
+	.mac_enable_tx_lpi = mvneta_mac_enable_tx_lpi,
 };
 
 static int mvneta_mdio_probe(struct mvneta_port *pp)
@@ -5106,14 +5151,6 @@ static int mvneta_ethtool_get_eee(struct net_device *dev,
 				  struct ethtool_keee *eee)
 {
 	struct mvneta_port *pp = netdev_priv(dev);
-	u32 lpi_ctl0;
-
-	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;
 
 	return phylink_ethtool_get_eee(pp->phylink, eee);
 }
@@ -5122,23 +5159,6 @@ static int mvneta_ethtool_set_eee(struct net_device *dev,
 				  struct ethtool_keee *eee)
 {
 	struct mvneta_port *pp = netdev_priv(dev);
-	u32 lpi_ctl0;
-
-	/* The Armada 37x documents do not give limits for this other than
-	 * it being an 8-bit register.
-	 */
-	if (eee->tx_lpi_enabled && eee->tx_lpi_timer > 255)
-		return -EINVAL;
-
-	lpi_ctl0 = mvreg_read(pp, MVNETA_LPI_CTRL_0);
-	lpi_ctl0 &= ~(0xff << 8);
-	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);
 
 	return phylink_ethtool_set_eee(pp->phylink, eee);
 }
@@ -5453,6 +5473,9 @@ static int mvneta_port_power_up(struct mvneta_port *pp, int phy_mode)
 	    !phy_interface_mode_is_rgmii(phy_mode))
 		return -EINVAL;
 
+	/* Ensure LPI is disabled */
+	mvneta_mac_disable_tx_lpi(&pp->phylink_config);
+
 	return 0;
 }
 
@@ -5544,6 +5567,14 @@ static int mvneta_probe(struct platform_device *pdev)
 	pp->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_10 |
 		MAC_100 | MAC_1000FD | MAC_2500FD;
 
+	/* Setup EEE. Choose 250us idle. Only supported in SGMII modes. */
+	__set_bit(PHY_INTERFACE_MODE_QSGMII, pp->phylink_config.lpi_interfaces);
+	__set_bit(PHY_INTERFACE_MODE_SGMII, pp->phylink_config.lpi_interfaces);
+	pp->phylink_config.lpi_capabilities = MAC_100FD | MAC_1000FD;
+	pp->phylink_config.lpi_timer_limit_us = 255;
+	pp->phylink_config.lpi_timer_default = 250;
+	pp->phylink_config.eee_enabled_default = true;
+
 	phy_interface_set_rgmii(pp->phylink_config.supported_interfaces);
 	__set_bit(PHY_INTERFACE_MODE_QSGMII,
 		  pp->phylink_config.supported_interfaces);
-- 
2.30.2

Re: [PATCH net-next 07/10] net: mvneta: convert to phylink EEE implementation

[Andrew Lunn]

On Mon, Dec 09, 2024 at 02:23:48PM +0000, Russell King (Oracle) wrote:
> Convert mvneta to use phylink's EEE implementation, which means we just
> need to implement the two methods for LPI control, and adding the
> initial configuration.
> 
> Disabling LPI requires clearing a single bit. Enabling LPI needs a full
> configuration of several values, as the timer values are dependent on
> the MAC operating speed.
> 
> As Armada 388 states that EEE is only supported in "SGMII" modes, mark
> this in lpi_interfaces. Testing with RGMII on the Clearfog platform
> indicates that the receive path fails to detect LPI over RGMII.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 07/10] net: mvneta: convert to phylink EEE implementation

[Simon Horman]

On Mon, Dec 09, 2024 at 02:23:48PM +0000, Russell King (Oracle) wrote:
> Convert mvneta to use phylink's EEE implementation, which means we just
> need to implement the two methods for LPI control, and adding the
> initial configuration.
> 
> Disabling LPI requires clearing a single bit. Enabling LPI needs a full
> configuration of several values, as the timer values are dependent on
> the MAC operating speed.
> 
> As Armada 388 states that EEE is only supported in "SGMII" modes, mark
> this in lpi_interfaces. Testing with RGMII on the Clearfog platform
> indicates that the receive path fails to detect LPI over RGMII.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/ethernet/marvell/mvneta.c | 127 ++++++++++++++++----------
>  1 file changed, 79 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c

...

> +static void mvneta_mac_enable_tx_lpi(struct phylink_config *config, u32 timer,
> +				     bool tx_clk_stop)
> +{
> +	struct mvneta_port *pp = netdev_priv(to_net_dev(config->dev));
> +	u32 ts, tw, lpi0, lpi1, status;
> +
> +	status = mvreg_read(pp, MVNETA_GMAC_STATUS);
> +	if (status & MVNETA_GMAC_SPEED_1000) {
> +		/* At 1G speeds, the timer resolution are 1us, and
> +		 * 802.3 says tw is 16.5us. Round up to 17us.
> +		 */
> +		tw = 17;
> +		ts = timer;
> +	} else {
> +		/* At 100M speeds, the timer resolutions are 10us, and
> +		 * 802.3 says tw is 30us.
> +		 */
> +		tw = 3;
> +		ts = DIV_ROUND_UP(timer, 10);
>  	}
> +
> +	if (ts > 255)
> +		ts = 255;
> +
> +	/* Configure ts */
> +	lpi0 = mvreg_read(pp, MVNETA_LPI_CTRL_0);
> +	lpi0 = u32_replace_bits(lpi0, MVNETA_LPI_CTRL_0_TS, ts);

Hi Russell,

I think that the val and field arguments to u32_replace_bits() are
inverted here and this should be:

	lpi0 = u32_replace_bits(lpi0, ts, MVNETA_LPI_CTRL_0_TS);

> +	mvreg_write(pp, MVNETA_LPI_CTRL_0, lpi0);
> +
> +	/* Configure tw and enable LPI generation */
> +	lpi1 = mvreg_read(pp, MVNETA_LPI_CTRL_1);
> +	lpi1 = u32_replace_bits(lpi1, MVNETA_LPI_CTRL_1_TW, tw);

Ditto.

> +	lpi1 |= MVNETA_LPI_CTRL_1_REQUEST_ENABLE;
> +	mvreg_write(pp, MVNETA_LPI_CTRL_1, lpi1);
>  }
>  
>  static const struct phylink_mac_ops mvneta_phylink_ops = {

Flagged by clang-19 and gcc-14 W=1 builds.

...

Re: [PATCH net-next 07/10] net: mvneta: convert to phylink EEE implementation

[Simon Horman]

On Fri, Dec 13, 2024 at 10:04:15AM +0000, Simon Horman wrote:
> On Mon, Dec 09, 2024 at 02:23:48PM +0000, Russell King (Oracle) wrote:
> > Convert mvneta to use phylink's EEE implementation, which means we just
> > need to implement the two methods for LPI control, and adding the
> > initial configuration.
> > 
> > Disabling LPI requires clearing a single bit. Enabling LPI needs a full
> > configuration of several values, as the timer values are dependent on
> > the MAC operating speed.
> > 
> > As Armada 388 states that EEE is only supported in "SGMII" modes, mark
> > this in lpi_interfaces. Testing with RGMII on the Clearfog platform
> > indicates that the receive path fails to detect LPI over RGMII.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/net/ethernet/marvell/mvneta.c | 127 ++++++++++++++++----------
> >  1 file changed, 79 insertions(+), 48 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> 
> ...
> 
> > +static void mvneta_mac_enable_tx_lpi(struct phylink_config *config, u32 timer,
> > +				     bool tx_clk_stop)
> > +{
> > +	struct mvneta_port *pp = netdev_priv(to_net_dev(config->dev));
> > +	u32 ts, tw, lpi0, lpi1, status;
> > +
> > +	status = mvreg_read(pp, MVNETA_GMAC_STATUS);
> > +	if (status & MVNETA_GMAC_SPEED_1000) {
> > +		/* At 1G speeds, the timer resolution are 1us, and
> > +		 * 802.3 says tw is 16.5us. Round up to 17us.
> > +		 */
> > +		tw = 17;
> > +		ts = timer;
> > +	} else {
> > +		/* At 100M speeds, the timer resolutions are 10us, and
> > +		 * 802.3 says tw is 30us.
> > +		 */
> > +		tw = 3;
> > +		ts = DIV_ROUND_UP(timer, 10);
> >  	}
> > +
> > +	if (ts > 255)
> > +		ts = 255;
> > +
> > +	/* Configure ts */
> > +	lpi0 = mvreg_read(pp, MVNETA_LPI_CTRL_0);
> > +	lpi0 = u32_replace_bits(lpi0, MVNETA_LPI_CTRL_0_TS, ts);
> 
> Hi Russell,
> 
> I think that the val and field arguments to u32_replace_bits() are
> inverted here and this should be:
> 
> 	lpi0 = u32_replace_bits(lpi0, ts, MVNETA_LPI_CTRL_0_TS);
> 
> > +	mvreg_write(pp, MVNETA_LPI_CTRL_0, lpi0);
> > +
> > +	/* Configure tw and enable LPI generation */
> > +	lpi1 = mvreg_read(pp, MVNETA_LPI_CTRL_1);
> > +	lpi1 = u32_replace_bits(lpi1, MVNETA_LPI_CTRL_1_TW, tw);
> 
> Ditto.
> 
> > +	lpi1 |= MVNETA_LPI_CTRL_1_REQUEST_ENABLE;
> > +	mvreg_write(pp, MVNETA_LPI_CTRL_1, lpi1);
> >  }
> >  
> >  static const struct phylink_mac_ops mvneta_phylink_ops = {
> 
> Flagged by clang-19 and gcc-14 W=1 builds.
> 
> ...

Sorry for more noise, and perhaps this is obvious.
But a similar problem seems to also exists in the following patch,
[PATCH] net: mvpp2: add EEE implementation.

Re: [PATCH net-next 07/10] net: mvneta: convert to phylink EEE implementation

[Russell King (Oracle)]

On Fri, Dec 13, 2024 at 10:22:11AM +0000, Simon Horman wrote:
> > Hi Russell,
> > 
> > I think that the val and field arguments to u32_replace_bits() are
> > inverted here and this should be:
> > 
> > 	lpi0 = u32_replace_bits(lpi0, ts, MVNETA_LPI_CTRL_0_TS);
> > 
> > > +	mvreg_write(pp, MVNETA_LPI_CTRL_0, lpi0);
> > > +
> > > +	/* Configure tw and enable LPI generation */
> > > +	lpi1 = mvreg_read(pp, MVNETA_LPI_CTRL_1);
> > > +	lpi1 = u32_replace_bits(lpi1, MVNETA_LPI_CTRL_1_TW, tw);
> > 
> > Ditto.
> > 
> > > +	lpi1 |= MVNETA_LPI_CTRL_1_REQUEST_ENABLE;
> > > +	mvreg_write(pp, MVNETA_LPI_CTRL_1, lpi1);
> > >  }
> > >  
> > >  static const struct phylink_mac_ops mvneta_phylink_ops = {
> > 
> > Flagged by clang-19 and gcc-14 W=1 builds.
> > 
> > ...
> 
> Sorry for more noise, and perhaps this is obvious.
> But a similar problem seems to also exists in the following patch,
> [PATCH] net: mvpp2: add EEE implementation.

Thanks Simon, the 0-day bot flagged them and have already been fixed.

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

[PATCH net-next 08/10] net: mvpp2: add EEE implementation

[Russell King (Oracle)]

Add EEE support for mvpp2, using phylink's EEE implementation, which
means we just need to implement the two methods for LPI control, and
with the initial configuration. Only SGMII mode is supported, so only
100M and 1G speeds.

Disabling LPI requires clearing a single bit. Enabling LPI needs a full
configuration of several values, as the timer values are dependent on
the MAC operating speed.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/marvell/mvpp2/mvpp2.h    |  5 +
 .../net/ethernet/marvell/mvpp2/mvpp2_main.c   | 98 +++++++++++++++++++
 2 files changed, 103 insertions(+)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
index 9e02e4367bec..364d038da7ea 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
@@ -481,6 +481,11 @@
 #define MVPP22_GMAC_INT_SUM_MASK		0xa4
 #define     MVPP22_GMAC_INT_SUM_MASK_LINK_STAT	BIT(1)
 #define	    MVPP22_GMAC_INT_SUM_MASK_PTP	BIT(2)
+#define MVPP2_GMAC_LPI_CTRL0			0xc0
+#define     MVPP2_GMAC_LPI_CTRL0_TS_MASK	GENMASK(8, 8)
+#define MVPP2_GMAC_LPI_CTRL1			0xc4
+#define     MVPP2_GMAC_LPI_CTRL1_REQ_EN		BIT(0)
+#define     MVPP2_GMAC_LPI_CTRL1_TW_MASK	GENMASK(15, 4)
 
 /* Per-port XGMAC registers. PPv2.2 and PPv2.3, only for GOP port 0,
  * relative to port->base.
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index f85229a30844..cb5f8e5965d9 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -5757,6 +5757,28 @@ static int mvpp2_ethtool_set_rxfh(struct net_device *dev,
 	return mvpp2_modify_rxfh_context(dev, NULL, rxfh, extack);
 }
 
+static int mvpp2_ethtool_get_eee(struct net_device *dev,
+				 struct ethtool_keee *eee)
+{
+	struct mvpp2_port *port = netdev_priv(dev);
+
+	if (!port->phylink)
+		return -EOPNOTSUPP;
+
+	return phylink_ethtool_get_eee(port->phylink, eee);
+}
+
+static int mvpp2_ethtool_set_eee(struct net_device *dev,
+				 struct ethtool_keee *eee)
+{
+	struct mvpp2_port *port = netdev_priv(dev);
+
+	if (!port->phylink)
+		return -EOPNOTSUPP;
+
+	return phylink_ethtool_set_eee(port->phylink, eee);
+}
+
 /* Device ops */
 
 static const struct net_device_ops mvpp2_netdev_ops = {
@@ -5802,6 +5824,8 @@ static const struct ethtool_ops mvpp2_eth_tool_ops = {
 	.create_rxfh_context	= mvpp2_create_rxfh_context,
 	.modify_rxfh_context	= mvpp2_modify_rxfh_context,
 	.remove_rxfh_context	= mvpp2_remove_rxfh_context,
+	.get_eee		= mvpp2_ethtool_get_eee,
+	.set_eee		= mvpp2_ethtool_set_eee,
 };
 
 /* Used for PPv2.1, or PPv2.2 with the old Device Tree binding that
@@ -6672,6 +6696,65 @@ static void mvpp2_mac_link_down(struct phylink_config *config,
 	mvpp2_port_disable(port);
 }
 
+static int mvpp2_mac_validate_tx_lpi(struct phylink_config *config,
+				     struct ethtool_keee *eee)
+{
+	if (eee->tx_lpi_timer > 255)
+		return -EINVAL;
+
+	return 0;
+}
+
+static void mvpp2_mac_disable_tx_lpi(struct phylink_config *config)
+{
+	struct mvpp2_port *port = mvpp2_phylink_to_port(config);
+
+	mvpp2_modify(port->base + MVPP2_GMAC_LPI_CTRL1,
+		     MVPP2_GMAC_LPI_CTRL1_REQ_EN, 0);
+}
+
+static void mvpp2_mac_enable_tx_lpi(struct phylink_config *config, u32 timer,
+				    bool tx_clk_stop)
+{
+	struct mvpp2_port *port = mvpp2_phylink_to_port(config);
+	u32 ts, tw, lpi1, status;
+
+	status = readl(port->base + MVPP2_GMAC_STATUS0);
+	if (status & MVPP2_GMAC_STATUS0_GMII_SPEED) {
+		/* At 1G speeds, the timer resolution are 1us, and
+		 * 802.3 says tw is 16.5us. Round up to 17us.
+		 */
+		tw = 17;
+		ts = timer;
+	} else {
+		/* At 100M speeds, the timer resolutions are 10us, and
+		 * 802.3 says tw is 30us.
+		 */
+		tw = 3;
+		ts = DIV_ROUND_UP(timer, 10);
+	}
+
+	if (ts > 255)
+		ts = 255;
+
+	/* Ensure LPI generation is disabled */
+	lpi1 = readl(port->base + MVPP2_GMAC_LPI_CTRL1);
+	writel(lpi1 & ~MVPP2_GMAC_LPI_CTRL1_REQ_EN,
+	       port->base + MVPP2_GMAC_LPI_CTRL1);
+
+	/* Configure ts */
+	mvpp2_modify(port->base + MVPP2_GMAC_LPI_CTRL0,
+		     MVPP2_GMAC_LPI_CTRL0_TS_MASK,
+		     FIELD_PREP(MVPP2_GMAC_LPI_CTRL0_TS_MASK, ts));
+
+	/* Configure tw */
+	lpi1 = u32_replace_bits(lpi1, MVPP2_GMAC_LPI_CTRL1_TW_MASK, tw);
+
+	/* Enable LPI generation */
+	writel(lpi1 | MVPP2_GMAC_LPI_CTRL1_REQ_EN,
+	       port->base + MVPP2_GMAC_LPI_CTRL1);
+}
+
 static const struct phylink_mac_ops mvpp2_phylink_ops = {
 	.mac_select_pcs = mvpp2_select_pcs,
 	.mac_prepare = mvpp2_mac_prepare,
@@ -6679,6 +6762,9 @@ static const struct phylink_mac_ops mvpp2_phylink_ops = {
 	.mac_finish = mvpp2_mac_finish,
 	.mac_link_up = mvpp2_mac_link_up,
 	.mac_link_down = mvpp2_mac_link_down,
+	.mac_validate_tx_lpi = mvpp2_mac_validate_tx_lpi,
+	.mac_enable_tx_lpi = mvpp2_mac_enable_tx_lpi,
+	.mac_disable_tx_lpi = mvpp2_mac_disable_tx_lpi,
 };
 
 /* Work-around for ACPI */
@@ -6957,6 +7043,16 @@ static int mvpp2_port_probe(struct platform_device *pdev,
 		port->phylink_config.mac_capabilities =
 			MAC_2500FD | MAC_1000FD | MAC_100 | MAC_10;
 
+		__set_bit(PHY_INTERFACE_MODE_SGMII,
+			  port->phylink_config.lpi_interfaces);
+
+		port->phylink_config.lpi_capabilities = MAC_1000FD | MAC_100FD;
+
+		/* Setup EEE.  Choose 250us idle. */
+		port->phylink_config.lpi_timer_limit_us = 255;
+		port->phylink_config.lpi_timer_default = 250;
+		port->phylink_config.eee_enabled_default = true;
+
 		if (port->priv->global_tx_fc)
 			port->phylink_config.mac_capabilities |=
 				MAC_SYM_PAUSE | MAC_ASYM_PAUSE;
@@ -7031,6 +7127,8 @@ static int mvpp2_port_probe(struct platform_device *pdev,
 			goto err_free_port_pcpu;
 		}
 		port->phylink = phylink;
+
+		mvpp2_mac_disable_tx_lpi(&port->phylink_config);
 	} else {
 		dev_warn(&pdev->dev, "Use link irqs for port#%d. FW update required\n", port->id);
 		port->phylink = NULL;
-- 
2.30.2

Re: [PATCH net-next 08/10] net: mvpp2: add EEE implementation

[Andrew Lunn]

On Mon, Dec 09, 2024 at 02:23:53PM +0000, Russell King (Oracle) wrote:
> Add EEE support for mvpp2, using phylink's EEE implementation, which
> means we just need to implement the two methods for LPI control, and
> with the initial configuration. Only SGMII mode is supported, so only
> 100M and 1G speeds.
> 
> Disabling LPI requires clearing a single bit. Enabling LPI needs a full
> configuration of several values, as the timer values are dependent on
> the MAC operating speed.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next 09/10] net: lan743x: use netdev in lan743x_phylink_mac_link_down()

[Russell King (Oracle)]

Use the netdev that we already have in lan743x_phylink_mac_link_down().

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/microchip/lan743x_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index 4dc5adcda6a3..8d7ad021ac70 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -3029,7 +3029,7 @@ static void lan743x_phylink_mac_link_down(struct phylink_config *config,
 	struct net_device *netdev = to_net_dev(config->dev);
 	struct lan743x_adapter *adapter = netdev_priv(netdev);
 
-	netif_tx_stop_all_queues(to_net_dev(config->dev));
+	netif_tx_stop_all_queues(netdev);
 	lan743x_mac_eee_enable(adapter, false);
 }
 
-- 
2.30.2

Re: [PATCH net-next 09/10] net: lan743x: use netdev in lan743x_phylink_mac_link_down()

[Andrew Lunn]

On Mon, Dec 09, 2024 at 02:23:59PM +0000, Russell King (Oracle) wrote:
> Use the netdev that we already have in lan743x_phylink_mac_link_down().
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next 10/10] net: lan743x: convert to phylink managed EEE

[Russell King (Oracle)]

Convert lan743x to phylink managed EEE:
- Set the lpi_capabilties.
- Move the call to lan743x_mac_eee_enable() into the enable/disable
  tx_lpi functions.
- Ensure that EEEEN is clear during probe.
- Move the setting of the LPI timer into mac_enable_tx_lpi().
- Move reading of LPI timer to phylink initialisation to set the
  default timer value.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 .../net/ethernet/microchip/lan743x_ethtool.c  | 21 -----------
 drivers/net/ethernet/microchip/lan743x_main.c | 37 ++++++++++++++++---
 drivers/net/ethernet/microchip/lan743x_main.h |  1 -
 3 files changed, 31 insertions(+), 28 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c
index 1a1cbd034eda..1459acfb1e61 100644
--- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
+++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
@@ -1055,9 +1055,6 @@ static int lan743x_ethtool_get_eee(struct net_device *netdev,
 {
 	struct lan743x_adapter *adapter = netdev_priv(netdev);
 
-	eee->tx_lpi_timer = lan743x_csr_read(adapter,
-					     MAC_EEE_TX_LPI_REQ_DLY_CNT);
-
 	return phylink_ethtool_get_eee(adapter->phylink, eee);
 }
 
@@ -1065,24 +1062,6 @@ static int lan743x_ethtool_set_eee(struct net_device *netdev,
 				   struct ethtool_keee *eee)
 {
 	struct lan743x_adapter *adapter = netdev_priv(netdev);
-	u32 tx_lpi_timer;
-
-	tx_lpi_timer = lan743x_csr_read(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT);
-	if (tx_lpi_timer != eee->tx_lpi_timer) {
-		u32 mac_cr = lan743x_csr_read(adapter, MAC_CR);
-
-		/* Software should only change this field when Energy Efficient
-		 * Ethernet Enable (EEEEN) is cleared.
-		 * This function will trigger an autonegotiation restart and
-		 * eee will be reenabled during link up if eee was negotiated.
-		 */
-		lan743x_mac_eee_enable(adapter, false);
-		lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT,
-				  eee->tx_lpi_timer);
-
-		if (mac_cr & MAC_CR_EEE_EN_)
-			lan743x_mac_eee_enable(adapter, true);
-	}
 
 	return phylink_ethtool_set_eee(adapter->phylink, eee);
 }
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index 8d7ad021ac70..25d37a2cb4a6 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -2966,7 +2966,7 @@ static int lan743x_phylink_2500basex_config(struct lan743x_adapter *adapter)
 	return lan743x_pcs_power_reset(adapter);
 }
 
-void lan743x_mac_eee_enable(struct lan743x_adapter *adapter, bool enable)
+static void lan743x_mac_eee_enable(struct lan743x_adapter *adapter, bool enable)
 {
 	u32 mac_cr;
 
@@ -3027,10 +3027,8 @@ static void lan743x_phylink_mac_link_down(struct phylink_config *config,
 					  phy_interface_t interface)
 {
 	struct net_device *netdev = to_net_dev(config->dev);
-	struct lan743x_adapter *adapter = netdev_priv(netdev);
 
 	netif_tx_stop_all_queues(netdev);
-	lan743x_mac_eee_enable(adapter, false);
 }
 
 static void lan743x_phylink_mac_link_up(struct phylink_config *config,
@@ -3072,16 +3070,32 @@ static void lan743x_phylink_mac_link_up(struct phylink_config *config,
 					  cap & FLOW_CTRL_TX,
 					  cap & FLOW_CTRL_RX);
 
-	if (phydev)
-		lan743x_mac_eee_enable(adapter, phydev->enable_tx_lpi);
-
 	netif_tx_wake_all_queues(netdev);
 }
 
+static void mac_disable_tx_lpi(struct phylink_config *config)
+{
+	lan743x_mac_eee_enable(adapter, false);
+}
+
+static void mac_enable_tx_lpi(struct phylink_config *config, u32 timer,
+			      bool tx_clk_stop)
+{
+	/* Software should only change this field when Energy Efficient
+	 * Ethernet Enable (EEEEN) is cleared. We ensure that by clearing
+	 * EEEEN during probe, and phylink itself guarantees that
+	 * mac_disable_tx_lpi() will have been previously called.
+	 */
+	lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT, timer);
+	lan743x_mac_eee_enable(adapter, true);
+}
+
 static const struct phylink_mac_ops lan743x_phylink_mac_ops = {
 	.mac_config = lan743x_phylink_mac_config,
 	.mac_link_down = lan743x_phylink_mac_link_down,
 	.mac_link_up = lan743x_phylink_mac_link_up,
+	.mac_disable_tx_lpi = lan743x_mac_disable_tx_lpi,
+	.mac_enable_tx_lpi = lan743x_mac_enable_tx_lpi,
 };
 
 static int lan743x_phylink_create(struct lan743x_adapter *adapter)
@@ -3095,6 +3109,10 @@ static int lan743x_phylink_create(struct lan743x_adapter *adapter)
 
 	adapter->phylink_config.mac_capabilities = MAC_ASYM_PAUSE |
 		MAC_SYM_PAUSE | MAC_10 | MAC_100 | MAC_1000FD;
+	adapter->phylink_config.lpi_capabilities = MAC_100FD | MAC_1000FD;
+	adapter->phylink_config.lpi_timer_max = U32_MAX;
+	adapter->phylink_config.lpi_timer_default =
+		lan743x_csr_read(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT);
 
 	lan743x_phy_interface_select(adapter);
 
@@ -3120,6 +3138,10 @@ static int lan743x_phylink_create(struct lan743x_adapter *adapter)
 		phy_interface_set_rgmii(adapter->phylink_config.supported_interfaces);
 	}
 
+	memcpy(adapter->phylink_config.lpi_interfaces,
+	       adapter->phylink_config.supported_interfaces,
+	       sizeof(adapter->phylink_config.lpi_interfaces));
+
 	pl = phylink_create(&adapter->phylink_config, NULL,
 			    adapter->phy_interface, &lan743x_phylink_mac_ops);
 
@@ -3517,6 +3539,9 @@ static int lan743x_hardware_init(struct lan743x_adapter *adapter,
 		spin_lock_init(&tx->ring_lock);
 	}
 
+	/* Ensure EEEEN is clear */
+	lan743x_mac_eee_enable(adapter, false);
+
 	return 0;
 }
 
diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
index 8ef897c114d3..7f73d66854be 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.h
+++ b/drivers/net/ethernet/microchip/lan743x_main.h
@@ -1206,6 +1206,5 @@ void lan743x_hs_syslock_release(struct lan743x_adapter *adapter);
 void lan743x_mac_flow_ctrl_set_enables(struct lan743x_adapter *adapter,
 				       bool tx_enable, bool rx_enable);
 int lan743x_sgmii_read(struct lan743x_adapter *adapter, u8 mmd, u16 addr);
-void lan743x_mac_eee_enable(struct lan743x_adapter *adapter, bool enable);
 
 #endif /* _LAN743X_H */
-- 
2.30.2

Re: [PATCH net-next 10/10] net: lan743x: convert to phylink managed EEE

[Andrew Lunn]

> +	adapter->phylink_config.lpi_capabilities = MAC_100FD | MAC_1000FD;

Is EEE not defined for 10Mbps?

	Andrew

Re: [PATCH net-next 10/10] net: lan743x: convert to phylink managed EEE

[Russell King (Oracle)]

On Tue, Dec 10, 2024 at 04:37:27AM +0100, Andrew Lunn wrote:
> > +	adapter->phylink_config.lpi_capabilities = MAC_100FD | MAC_1000FD;
> 
> Is EEE not defined for 10Mbps?

According to 802.3, 10BASE-Te supports it. Phylib only has support for
10BASE-T1L (not mentioned in my 802.3 copy) using it, and I don't think
LAN743x is used with such PHYs.

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

Re: [PATCH net-next 10/10] net: lan743x: convert to phylink managed EEE

[kernel test robot]

Hi Russell,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Russell-King-Oracle/net-mdio-add-definition-for-clock-stop-capable-bit/20241210-022608
base:   net-next/main
patch link:    https://lore.kernel.org/r/E1tKeg8-006SNJ-4Q%40rmk-PC.armlinux.org.uk
patch subject: [PATCH net-next 10/10] net: lan743x: convert to phylink managed EEE
config: powerpc-randconfig-002-20241210 (https://download.01.org/0day-ci/archive/20241210/202412102203.FVir20i4-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 592c0fe55f6d9a811028b5f3507be91458ab2713)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241210/202412102203.FVir20i4-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412102203.FVir20i4-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/net/ethernet/microchip/lan743x_main.c:5:
   In file included from include/linux/pci.h:38:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/powerpc/include/asm/hardirq.h:6:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/powerpc/include/asm/io.h:24:
   In file included from include/linux/mm.h:2223:
   include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     518 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> drivers/net/ethernet/microchip/lan743x_main.c:3078:25: error: use of undeclared identifier 'adapter'
    3078 |         lan743x_mac_eee_enable(adapter, false);
         |                                ^
   drivers/net/ethernet/microchip/lan743x_main.c:3089:20: error: use of undeclared identifier 'adapter'
    3089 |         lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT, timer);
         |                           ^
   drivers/net/ethernet/microchip/lan743x_main.c:3090:25: error: use of undeclared identifier 'adapter'
    3090 |         lan743x_mac_eee_enable(adapter, true);
         |                                ^
>> drivers/net/ethernet/microchip/lan743x_main.c:3097:24: error: use of undeclared identifier 'lan743x_mac_disable_tx_lpi'; did you mean 'mac_disable_tx_lpi'?
    3097 |         .mac_disable_tx_lpi = lan743x_mac_disable_tx_lpi,
         |                               ^~~~~~~~~~~~~~~~~~~~~~~~~~
         |                               mac_disable_tx_lpi
   drivers/net/ethernet/microchip/lan743x_main.c:3076:13: note: 'mac_disable_tx_lpi' declared here
    3076 | static void mac_disable_tx_lpi(struct phylink_config *config)
         |             ^
>> drivers/net/ethernet/microchip/lan743x_main.c:3098:23: error: use of undeclared identifier 'lan743x_mac_enable_tx_lpi'; did you mean 'mac_enable_tx_lpi'?
    3098 |         .mac_enable_tx_lpi = lan743x_mac_enable_tx_lpi,
         |                              ^~~~~~~~~~~~~~~~~~~~~~~~~
         |                              mac_enable_tx_lpi
   drivers/net/ethernet/microchip/lan743x_main.c:3081:13: note: 'mac_enable_tx_lpi' declared here
    3081 | static void mac_enable_tx_lpi(struct phylink_config *config, u32 timer,
         |             ^
>> drivers/net/ethernet/microchip/lan743x_main.c:3113:26: error: no member named 'lpi_timer_max' in 'struct phylink_config'
    3113 |         adapter->phylink_config.lpi_timer_max = U32_MAX;
         |         ~~~~~~~~~~~~~~~~~~~~~~~ ^
   1 warning and 6 errors generated.


vim +/adapter +3078 drivers/net/ethernet/microchip/lan743x_main.c

  3075	
  3076	static void mac_disable_tx_lpi(struct phylink_config *config)
  3077	{
> 3078		lan743x_mac_eee_enable(adapter, false);
  3079	}
  3080	
  3081	static void mac_enable_tx_lpi(struct phylink_config *config, u32 timer,
  3082				      bool tx_clk_stop)
  3083	{
  3084		/* Software should only change this field when Energy Efficient
  3085		 * Ethernet Enable (EEEEN) is cleared. We ensure that by clearing
  3086		 * EEEEN during probe, and phylink itself guarantees that
  3087		 * mac_disable_tx_lpi() will have been previously called.
  3088		 */
  3089		lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT, timer);
  3090		lan743x_mac_eee_enable(adapter, true);
  3091	}
  3092	
  3093	static const struct phylink_mac_ops lan743x_phylink_mac_ops = {
  3094		.mac_config = lan743x_phylink_mac_config,
  3095		.mac_link_down = lan743x_phylink_mac_link_down,
  3096		.mac_link_up = lan743x_phylink_mac_link_up,
> 3097		.mac_disable_tx_lpi = lan743x_mac_disable_tx_lpi,
> 3098		.mac_enable_tx_lpi = lan743x_mac_enable_tx_lpi,
  3099	};
  3100	
  3101	static int lan743x_phylink_create(struct lan743x_adapter *adapter)
  3102	{
  3103		struct net_device *netdev = adapter->netdev;
  3104		struct phylink *pl;
  3105	
  3106		adapter->phylink_config.dev = &netdev->dev;
  3107		adapter->phylink_config.type = PHYLINK_NETDEV;
  3108		adapter->phylink_config.mac_managed_pm = false;
  3109	
  3110		adapter->phylink_config.mac_capabilities = MAC_ASYM_PAUSE |
  3111			MAC_SYM_PAUSE | MAC_10 | MAC_100 | MAC_1000FD;
  3112		adapter->phylink_config.lpi_capabilities = MAC_100FD | MAC_1000FD;
> 3113		adapter->phylink_config.lpi_timer_max = U32_MAX;
  3114		adapter->phylink_config.lpi_timer_default =
  3115			lan743x_csr_read(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT);
  3116	
  3117		lan743x_phy_interface_select(adapter);
  3118	
  3119		switch (adapter->phy_interface) {
  3120		case PHY_INTERFACE_MODE_SGMII:
  3121			__set_bit(PHY_INTERFACE_MODE_SGMII,
  3122				  adapter->phylink_config.supported_interfaces);
  3123			__set_bit(PHY_INTERFACE_MODE_1000BASEX,
  3124				  adapter->phylink_config.supported_interfaces);
  3125			__set_bit(PHY_INTERFACE_MODE_2500BASEX,
  3126				  adapter->phylink_config.supported_interfaces);
  3127			adapter->phylink_config.mac_capabilities |= MAC_2500FD;
  3128			break;
  3129		case PHY_INTERFACE_MODE_GMII:
  3130			__set_bit(PHY_INTERFACE_MODE_GMII,
  3131				  adapter->phylink_config.supported_interfaces);
  3132			break;
  3133		case PHY_INTERFACE_MODE_MII:
  3134			__set_bit(PHY_INTERFACE_MODE_MII,
  3135				  adapter->phylink_config.supported_interfaces);
  3136			break;
  3137		default:
  3138			phy_interface_set_rgmii(adapter->phylink_config.supported_interfaces);
  3139		}
  3140	
  3141		memcpy(adapter->phylink_config.lpi_interfaces,
  3142		       adapter->phylink_config.supported_interfaces,
  3143		       sizeof(adapter->phylink_config.lpi_interfaces));
  3144	
  3145		pl = phylink_create(&adapter->phylink_config, NULL,
  3146				    adapter->phy_interface, &lan743x_phylink_mac_ops);
  3147	
  3148		if (IS_ERR(pl)) {
  3149			netdev_err(netdev, "Could not create phylink (%pe)\n", pl);
  3150			return PTR_ERR(pl);
  3151		}
  3152	
  3153		adapter->phylink = pl;
  3154		netdev_dbg(netdev, "lan743x phylink created");
  3155	
  3156		return 0;
  3157	}
  3158	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next 10/10] net: lan743x: convert to phylink managed EEE

[kernel test robot]

Hi Russell,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Russell-King-Oracle/net-mdio-add-definition-for-clock-stop-capable-bit/20241210-022608
base:   net-next/main
patch link:    https://lore.kernel.org/r/E1tKeg8-006SNJ-4Q%40rmk-PC.armlinux.org.uk
patch subject: [PATCH net-next 10/10] net: lan743x: convert to phylink managed EEE
config: loongarch-allyesconfig (https://download.01.org/0day-ci/archive/20241212/202412120900.DYyJpYUw-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241212/202412120900.DYyJpYUw-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412120900.DYyJpYUw-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/net/ethernet/microchip/lan743x_main.c: In function 'mac_disable_tx_lpi':
   drivers/net/ethernet/microchip/lan743x_main.c:3078:32: error: 'adapter' undeclared (first use in this function)
    3078 |         lan743x_mac_eee_enable(adapter, false);
         |                                ^~~~~~~
   drivers/net/ethernet/microchip/lan743x_main.c:3078:32: note: each undeclared identifier is reported only once for each function it appears in
   drivers/net/ethernet/microchip/lan743x_main.c: In function 'mac_enable_tx_lpi':
   drivers/net/ethernet/microchip/lan743x_main.c:3089:27: error: 'adapter' undeclared (first use in this function)
    3089 |         lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT, timer);
         |                           ^~~~~~~
   drivers/net/ethernet/microchip/lan743x_main.c: At top level:
   drivers/net/ethernet/microchip/lan743x_main.c:3097:31: error: 'lan743x_mac_disable_tx_lpi' undeclared here (not in a function); did you mean 'mac_disable_tx_lpi'?
    3097 |         .mac_disable_tx_lpi = lan743x_mac_disable_tx_lpi,
         |                               ^~~~~~~~~~~~~~~~~~~~~~~~~~
         |                               mac_disable_tx_lpi
   drivers/net/ethernet/microchip/lan743x_main.c:3098:30: error: 'lan743x_mac_enable_tx_lpi' undeclared here (not in a function); did you mean 'mac_enable_tx_lpi'?
    3098 |         .mac_enable_tx_lpi = lan743x_mac_enable_tx_lpi,
         |                              ^~~~~~~~~~~~~~~~~~~~~~~~~
         |                              mac_enable_tx_lpi
   drivers/net/ethernet/microchip/lan743x_main.c: In function 'lan743x_phylink_create':
   drivers/net/ethernet/microchip/lan743x_main.c:3113:33: error: 'struct phylink_config' has no member named 'lpi_timer_max'; did you mean 'lpi_timer_default'?
    3113 |         adapter->phylink_config.lpi_timer_max = U32_MAX;
         |                                 ^~~~~~~~~~~~~
         |                                 lpi_timer_default
   drivers/net/ethernet/microchip/lan743x_main.c: At top level:
>> drivers/net/ethernet/microchip/lan743x_main.c:3081:13: warning: 'mac_enable_tx_lpi' defined but not used [-Wunused-function]
    3081 | static void mac_enable_tx_lpi(struct phylink_config *config, u32 timer,
         |             ^~~~~~~~~~~~~~~~~
>> drivers/net/ethernet/microchip/lan743x_main.c:3076:13: warning: 'mac_disable_tx_lpi' defined but not used [-Wunused-function]
    3076 | static void mac_disable_tx_lpi(struct phylink_config *config)
         |             ^~~~~~~~~~~~~~~~~~


vim +/mac_enable_tx_lpi +3081 drivers/net/ethernet/microchip/lan743x_main.c

  3075	
> 3076	static void mac_disable_tx_lpi(struct phylink_config *config)
  3077	{
  3078		lan743x_mac_eee_enable(adapter, false);
  3079	}
  3080	
> 3081	static void mac_enable_tx_lpi(struct phylink_config *config, u32 timer,
  3082				      bool tx_clk_stop)
  3083	{
  3084		/* Software should only change this field when Energy Efficient
  3085		 * Ethernet Enable (EEEEN) is cleared. We ensure that by clearing
  3086		 * EEEEN during probe, and phylink itself guarantees that
  3087		 * mac_disable_tx_lpi() will have been previously called.
  3088		 */
  3089		lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT, timer);
  3090		lan743x_mac_eee_enable(adapter, true);
  3091	}
  3092	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next 00/10] net: add phylink managed EEE support

[Christian Marangi]

On Mon, Dec 09, 2024 at 02:22:31PM +0000, Russell King (Oracle) wrote:
> Hi,
> 
> Adding managed EEE support to phylink has been on the cards ever since
> the idea in phylib was mooted. This overly large series attempts to do
> so. I've included all the patches as it's important to get the driver
> patches out there.
> 
> Patch 1 adds a definition for the clock stop capable bit in the PCS
> MMD status register.
> 
> Patch 2 adds a phylib API to query whether the PHY allows the transmit
> xMII clock to be stopped while in LPI mode. This capability is for MAC
> drivers to save power when LPI is active, to allow them to stop their
> transmit clock.
> 
> Patch 3 adds another phylib API to configure whether the receive xMII
> clock may be disabled by the PHY. We do have an existing API,
> phy_init_eee(), but... it only allows the control bit to be set which
> is weird - what if a boot firmware or previous kernel has set this bit
> and we want it clear?
> 
> Patch 4 starts on the phylink parts of this, extracting from
> phylink_resolve() the detection of link-up. (Yes, okay, I could've
> dropped this patch, but with 23 patches, it's not going to make that
> much difference.)
> 
> Patch 5 adds phylink managed EEE support. Two new MAC APIs are added,
> to enable and disable LPI. The enable method is passed the LPI timer
> setting which it is expected to program into the hardware, and also a
> flag ehther the transmit clock should be stopped.
> 
>  *** There are open questions here. Eagle eyed reviewers will notice
>    pl->config->lpi_interfaces. There are MACs out there which only
>    support LPI signalling on a subset of their interface types. Phylib
>    doesn't understand this. I'm handling this at the moment by simply
>    not activating LPI at the MAC, but that leads to ethtool --show-eee
>    suggesting that EEE is active when it isn't.
>  *** Should we pass the phy_interface_t to these functions?

Maybe only to validate?

>  *** Should mac_enable_tx_lpi() be allowed to fail if the MAC doesn't
>    support the interface mode?

I'm a bit confused by this... Following principle with other OPs
shouldn't this never happen? Supported interface are validated by
capabilities hence mac_enable_tx_lpi() should never be reached (if not
supported). Or I'm missing something by this idea?

> 
> The above questions remain unanswered from the RFC posting of this
> series.
> 
> A change that has been included over the RFC version is the addition
> of the mac_validate_tx_lpi() method, which allows MAC drivers to
> validate the parameters to the ethtool set_eee() method. Implementations
> of this are in mvneta and mvpp2.
> 
> An example of a MAC that this is the case are the Marvell ones - both
> NETA and PP2 only support LPI signalling when connected via SGMII,
> which makes being connected to a PHY which changes its link mode
> problematical.
> 
> The remainder of the patches address the driver sides, which are
> necessary to actually test phylink managed EEE.
> 
>  drivers/net/ethernet/marvell/mvneta.c            | 127 +++++++++++--------
>  drivers/net/ethernet/marvell/mvpp2/mvpp2.h       |   5 +
>  drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c  |  98 +++++++++++++++
>  drivers/net/ethernet/microchip/lan743x_ethtool.c |  21 ----
>  drivers/net/ethernet/microchip/lan743x_main.c    |  39 ++++--
>  drivers/net/ethernet/microchip/lan743x_main.h    |   1 -
>  drivers/net/phy/phy.c                            |  47 ++++++-
>  drivers/net/phy/phylink.c                        | 150 +++++++++++++++++++++--
>  include/linux/phy.h                              |   2 +
>  include/linux/phylink.h                          |  59 +++++++++
>  include/uapi/linux/mdio.h                        |   1 +
>  11 files changed, 458 insertions(+), 92 deletions(-)
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

-- 
	Ansuel

Re: [PATCH net-next 00/10] net: add phylink managed EEE support

[Russell King (Oracle)]

On Mon, Dec 09, 2024 at 07:35:11PM +0100, Christian Marangi wrote:
> On Mon, Dec 09, 2024 at 02:22:31PM +0000, Russell King (Oracle) wrote:
> > Hi,
> > 
> > Adding managed EEE support to phylink has been on the cards ever since
> > the idea in phylib was mooted. This overly large series attempts to do
> > so. I've included all the patches as it's important to get the driver
> > patches out there.
> > 
> > Patch 1 adds a definition for the clock stop capable bit in the PCS
> > MMD status register.
> > 
> > Patch 2 adds a phylib API to query whether the PHY allows the transmit
> > xMII clock to be stopped while in LPI mode. This capability is for MAC
> > drivers to save power when LPI is active, to allow them to stop their
> > transmit clock.
> > 
> > Patch 3 adds another phylib API to configure whether the receive xMII
> > clock may be disabled by the PHY. We do have an existing API,
> > phy_init_eee(), but... it only allows the control bit to be set which
> > is weird - what if a boot firmware or previous kernel has set this bit
> > and we want it clear?
> > 
> > Patch 4 starts on the phylink parts of this, extracting from
> > phylink_resolve() the detection of link-up. (Yes, okay, I could've
> > dropped this patch, but with 23 patches, it's not going to make that
> > much difference.)
> > 
> > Patch 5 adds phylink managed EEE support. Two new MAC APIs are added,
> > to enable and disable LPI. The enable method is passed the LPI timer
> > setting which it is expected to program into the hardware, and also a
> > flag ehther the transmit clock should be stopped.
> > 
> >  *** There are open questions here. Eagle eyed reviewers will notice
> >    pl->config->lpi_interfaces. There are MACs out there which only
> >    support LPI signalling on a subset of their interface types. Phylib
> >    doesn't understand this. I'm handling this at the moment by simply
> >    not activating LPI at the MAC, but that leads to ethtool --show-eee
> >    suggesting that EEE is active when it isn't.
> >  *** Should we pass the phy_interface_t to these functions?
> 
> Maybe only to validate?

validate doesn't know what interface will be used - at set_eee() time
we can't know that.

> >  *** Should mac_enable_tx_lpi() be allowed to fail if the MAC doesn't
> >    support the interface mode?
> 
> I'm a bit confused by this... Following principle with other OPs
> shouldn't this never happen? Supported interface are validated by
> capabilities hence mac_enable_tx_lpi() should never be reached (if not
> supported). Or I'm missing something by this idea?

This question was asked in the RFC, and before I added lpi_interfaces
which now prevents calling this unless the interface bit is set in
lpi_interfaces. However, does it still make sense to pass the
interface in case e.g. the MAC block that needs to be configured is
dependent on it?

Some network interfaces are made up of multiple MACs for different
speeds, and the MAC is selected by interface. E.g. mvpp2.

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

Re: [PATCH net-next 00/10] net: add phylink managed EEE support

[Russell King (Oracle)]

On Mon, Dec 09, 2024 at 02:22:31PM +0000, Russell King (Oracle) wrote:
> Patch 5 adds phylink managed EEE support. Two new MAC APIs are added,
> to enable and disable LPI. The enable method is passed the LPI timer
> setting which it is expected to program into the hardware, and also a
> flag ehther the transmit clock should be stopped.
> 
>  *** There are open questions here. Eagle eyed reviewers will notice
>    pl->config->lpi_interfaces. There are MACs out there which only
>    support LPI signalling on a subset of their interface types. Phylib
>    doesn't understand this. I'm handling this at the moment by simply
>    not activating LPI at the MAC, but that leads to ethtool --show-eee
>    suggesting that EEE is active when it isn't.
>  *** Should we pass the phy_interface_t to these functions?
>  *** Should mac_enable_tx_lpi() be allowed to fail if the MAC doesn't
>    support the interface mode?
> 
> The above questions remain unanswered from the RFC posting of this
> series.

Given the open questions that still remain and the lack of engagement,
I don't think we're at the point of being able to apply this patch set
(I don't want to rework the method functions in lots of drivers.) So,
I suggest we put this on the back burner until there's a clear way
forward.

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