Hello all,
with Andrew's permission I'll continue mainlining this patches:
==============================================================
Most MAC drivers get EEE wrong. The API to the PHY is not very
obvious, which is probably why. Rework the API, pushing most of the
EEE handling into phylib core, leaving the MAC drivers to just
enable/disable support for EEE in there change_link call back.
MAC drivers are now expect to indicate to phylib if they support
EEE. This will allow future patches to configure the PHY to advertise
no EEE link modes when EEE is not supported. The information could
also be used to enable SmartEEE if the PHY supports it.
With these changes, the uAPI configuration eee_enable becomes a global
on/off. tx-lpi must also be enabled before EEE is enabled. This fits
the discussion here:
https://lore.kernel.org/netdev/[email protected]/T/
This patchset puts in place all the infrastructure, and converts one
MAC driver to the new API. Following patchsets will convert other MAC
drivers, extend support into phylink, and when all MAC drivers are
converted to the new scheme, clean up some unneeded code.
v9:
--
change dst<>src order for eeecfg_to_eee() and eee_to_eeecfg()
drop phy_eee_clk_stop_enable() patch
v8:
--
update phydev->link value before phy_link_down/up cycle
v7:
--
add phy_link_down() before phy_link_up()
rewrite comment for phy_ethtool_set_eee_noneg()
add check for changed tx_lpi_timer
v6:
--
Reword different comments. See per patch change comments.
v5:
--
Rebase against latest netdev-next
Use keee instead of eee struct
v4
--
Only convert one MAC driver
Drop all phylink code
Conform to the uAPI discision.
v3
--
Rework phylink code to add a new callback.
Rework function to indicate clock should be stopped during LPI
Andrew Lunn (6):
net: phy: Add phydev->enable_tx_lpi to simplify adjust link callbacks
net: phy: Keep track of EEE configuration
net: phy: Immediately call adjust_link if only tx_lpi_enabled changes
net: phy: Add phy_support_eee() indicating MAC support EEE
net: fec: Move fec_enet_eee_mode_set() and helper earlier
net: fec: Fixup EEE
Russell King (1):
net: add helpers for EEE configuration
drivers/net/ethernet/freescale/fec_main.c | 84 ++++++++++-------------
drivers/net/phy/phy-c45.c | 14 +++-
drivers/net/phy/phy.c | 50 +++++++++++++-
drivers/net/phy/phy_device.c | 28 ++++++++
include/linux/phy.h | 8 ++-
include/net/eee.h | 38 ++++++++++
6 files changed, 168 insertions(+), 54 deletions(-)
create mode 100644 include/net/eee.h
--
2.39.2
From: Andrew Lunn <[email protected]>
The enabling/disabling of EEE in the MAC should happen as a result of
auto negotiation. So move the enable/disable into
fec_enet_adjust_link() which gets called by phylib when there is a
change in link status.
fec_enet_set_eee() now just stores away the LPI timer value.
Everything else is passed to phylib, so it can correctly setup the
PHY.
fec_enet_get_eee() relies on phylib doing most of the work,
the MAC driver just adds the LPI timer value.
Call phy_support_eee() if the quirk is present to indicate the MAC
actually supports EEE.
Signed-off-by: Andrew Lunn <[email protected]>
Tested-by: Oleksij Rempel <[email protected]> (On iMX8MP debix)
Signed-off-by: Oleksij Rempel <[email protected]>
Reviewed-by: Wei Fang <[email protected]>
---
v2: Only call fec_enet_eee_mode_set for those that support EEE
v7: update against kernel v6.8-rc4
---
drivers/net/ethernet/freescale/fec_main.c | 23 +++++------------------
1 file changed, 5 insertions(+), 18 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index a2c786550342f..d7693fdf640d5 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -2033,13 +2033,8 @@ static int fec_enet_eee_mode_set(struct net_device *ndev, bool enable)
struct fec_enet_private *fep = netdev_priv(ndev);
struct ethtool_keee *p = &fep->eee;
unsigned int sleep_cycle, wake_cycle;
- int ret = 0;
if (enable) {
- ret = phy_init_eee(ndev->phydev, false);
- if (ret)
- return ret;
-
sleep_cycle = fec_enet_us_to_tx_cycle(ndev, p->tx_lpi_timer);
wake_cycle = sleep_cycle;
} else {
@@ -2047,8 +2042,6 @@ static int fec_enet_eee_mode_set(struct net_device *ndev, bool enable)
wake_cycle = 0;
}
- p->tx_lpi_enabled = enable;
-
writel(sleep_cycle, fep->hwp + FEC_LPI_SLEEP);
writel(wake_cycle, fep->hwp + FEC_LPI_WAKE);
@@ -2094,6 +2087,8 @@ static void fec_enet_adjust_link(struct net_device *ndev)
netif_tx_unlock_bh(ndev);
napi_enable(&fep->napi);
}
+ if (fep->quirks & FEC_QUIRK_HAS_EEE)
+ fec_enet_eee_mode_set(ndev, phy_dev->enable_tx_lpi);
} else {
if (fep->link) {
netif_stop_queue(ndev);
@@ -2453,6 +2448,9 @@ static int fec_enet_mii_probe(struct net_device *ndev)
else
phy_set_max_speed(phy_dev, 100);
+ if (fep->quirks & FEC_QUIRK_HAS_EEE)
+ phy_support_eee(phy_dev);
+
fep->link = 0;
fep->full_duplex = 0;
@@ -3172,7 +3170,6 @@ fec_enet_get_eee(struct net_device *ndev, struct ethtool_keee *edata)
return -ENETDOWN;
edata->tx_lpi_timer = p->tx_lpi_timer;
- edata->tx_lpi_enabled = p->tx_lpi_enabled;
return phy_ethtool_get_eee(ndev->phydev, edata);
}
@@ -3182,7 +3179,6 @@ fec_enet_set_eee(struct net_device *ndev, struct ethtool_keee *edata)
{
struct fec_enet_private *fep = netdev_priv(ndev);
struct ethtool_keee *p = &fep->eee;
- int ret = 0;
if (!(fep->quirks & FEC_QUIRK_HAS_EEE))
return -EOPNOTSUPP;
@@ -3192,15 +3188,6 @@ fec_enet_set_eee(struct net_device *ndev, struct ethtool_keee *edata)
p->tx_lpi_timer = edata->tx_lpi_timer;
- if (!edata->eee_enabled || !edata->tx_lpi_enabled ||
- !edata->tx_lpi_timer)
- ret = fec_enet_eee_mode_set(ndev, false);
- else
- ret = fec_enet_eee_mode_set(ndev, true);
-
- if (ret)
- return ret;
-
return phy_ethtool_set_eee(ndev->phydev, edata);
}
--
2.39.2
From: Andrew Lunn <[email protected]>
MAC drivers which support EEE need to know the results of the EEE
auto-neg in order to program the hardware to perform EEE or not. The
oddly named phy_init_eee() can be used to determine this, it returns 0
if EEE should be used, or a negative error code,
e.g. -EOPPROTONOTSUPPORT if the PHY does not support EEE or negotiate
resulted in it not being used.
However, many MAC drivers get this wrong. Add phydev->enable_tx_lpi
which indicates the result of the autoneg for EEE, including if EEE is
administratively disabled with ethtool. The MAC driver can then access
this in the same way as link speed and duplex in the adjust link
callback. If enable_tx_lpi is true, the MAC should send low power
indications and does not need to consider anything else with respect
to EEE.
Reviewed-by: Florian Fainelli <[email protected]>
Signed-off-by: Andrew Lunn <[email protected]>
Signed-off-by: Oleksij Rempel <[email protected]>
---
v2: Check for errors from genphy_c45_eee_is_active
v5: Rename to enable_tx_lpi to fit better with phylink changes
v6: enable_tx_lpi = !!err
---
drivers/net/phy/phy.c | 7 +++++++
include/linux/phy.h | 2 ++
2 files changed, 9 insertions(+)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 14224e06d69fa..2bc0a7d51c63f 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -983,9 +983,16 @@ static int phy_check_link_status(struct phy_device *phydev)
if (phydev->link && phydev->state != PHY_RUNNING) {
phy_check_downshift(phydev);
phydev->state = PHY_RUNNING;
+ err = genphy_c45_eee_is_active(phydev,
+ NULL, NULL, NULL);
+ if (err < 0)
+ phydev->enable_tx_lpi = false;
+ else
+ phydev->enable_tx_lpi = !!err;
phy_link_up(phydev);
} else if (!phydev->link && phydev->state != PHY_NOLINK) {
phydev->state = PHY_NOLINK;
+ phydev->enable_tx_lpi = false;
phy_link_down(phydev);
}
diff --git a/include/linux/phy.h b/include/linux/phy.h
index e3ab2c347a598..a880f6d7170ea 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -594,6 +594,7 @@ struct macsec_ops;
* @supported_eee: supported PHY EEE linkmodes
* @advertising_eee: Currently advertised EEE linkmodes
* @eee_enabled: Flag indicating whether the EEE feature is enabled
+ * @enable_tx_lpi: When True, MAC should transmit LPI to PHY
* @lp_advertising: Current link partner advertised linkmodes
* @host_interfaces: PHY interface modes supported by host
* @eee_broken_modes: Energy efficient ethernet modes which should be prohibited
@@ -713,6 +714,7 @@ struct phy_device {
/* Energy efficient ethernet modes which should be prohibited */
u32 eee_broken_modes;
+ bool enable_tx_lpi;
#ifdef CONFIG_LED_TRIGGER_PHY
struct phy_led_trigger *phy_led_triggers;
--
2.39.2
From: Andrew Lunn <[email protected]>
Have phylib keep track of the EEE configuration. This simplifies the
MAC drivers, in that they don't need to store it.
Future patches to phylib will also make use of this information to
further simplify the MAC drivers.
Reviewed-by: Russell King (Oracle) <[email protected]>
Signed-off-by: Andrew Lunn <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
Signed-off-by: Oleksij Rempel <[email protected]>
---
v6: add @ in front of eee_cfg
---
drivers/net/phy/phy.c | 7 +++++--
include/linux/phy.h | 3 +++
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 2bc0a7d51c63f..95c4ef5d4e97f 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1640,8 +1640,8 @@ EXPORT_SYMBOL(phy_get_eee_err);
* @phydev: target phy_device struct
* @data: ethtool_keee data
*
- * Description: it reportes the Supported/Advertisement/LP Advertisement
- * capabilities.
+ * Description: reports the Supported/Advertisement/LP Advertisement
+ * capabilities, etc.
*/
int phy_ethtool_get_eee(struct phy_device *phydev, struct ethtool_keee *data)
{
@@ -1652,6 +1652,7 @@ int phy_ethtool_get_eee(struct phy_device *phydev, struct ethtool_keee *data)
mutex_lock(&phydev->lock);
ret = genphy_c45_ethtool_get_eee(phydev, data);
+ eeecfg_to_eee(data, &phydev->eee_cfg);
mutex_unlock(&phydev->lock);
return ret;
@@ -1674,6 +1675,8 @@ int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_keee *data)
mutex_lock(&phydev->lock);
ret = genphy_c45_ethtool_set_eee(phydev, data);
+ if (!ret)
+ eee_to_eeecfg(&phydev->eee_cfg, data);
mutex_unlock(&phydev->lock);
return ret;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index a880f6d7170ea..695e366bd75c1 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -30,6 +30,7 @@
#include <linux/refcount.h>
#include <linux/atomic.h>
+#include <net/eee.h>
#define PHY_DEFAULT_FEATURES (SUPPORTED_Autoneg | \
SUPPORTED_TP | \
@@ -595,6 +596,7 @@ struct macsec_ops;
* @advertising_eee: Currently advertised EEE linkmodes
* @eee_enabled: Flag indicating whether the EEE feature is enabled
* @enable_tx_lpi: When True, MAC should transmit LPI to PHY
+ * @eee_cfg: User configuration of EEE
* @lp_advertising: Current link partner advertised linkmodes
* @host_interfaces: PHY interface modes supported by host
* @eee_broken_modes: Energy efficient ethernet modes which should be prohibited
@@ -715,6 +717,7 @@ struct phy_device {
/* Energy efficient ethernet modes which should be prohibited */
u32 eee_broken_modes;
bool enable_tx_lpi;
+ struct eee_config eee_cfg;
#ifdef CONFIG_LED_TRIGGER_PHY
struct phy_led_trigger *phy_led_triggers;
--
2.39.2
From: Russell King <[email protected]>
Add helpers that phylib and phylink can use to manage EEE configuration
and determine whether the MAC should be permitted to use LPI based on
that configuration.
Signed-off-by: Russell King (Oracle) <[email protected]>
Signed-off-by: Andrew Lunn <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
Signed-off-by: Oleksij Rempel <[email protected]>
---
v8: change dst<>src order
---
include/net/eee.h | 38 ++++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
create mode 100644 include/net/eee.h
diff --git a/include/net/eee.h b/include/net/eee.h
new file mode 100644
index 0000000000000..84837aba3cd9e
--- /dev/null
+++ b/include/net/eee.h
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef _EEE_H
+#define _EEE_H
+
+#include <linux/types.h>
+
+struct eee_config {
+ u32 tx_lpi_timer;
+ bool tx_lpi_enabled;
+ bool eee_enabled;
+};
+
+static inline bool eeecfg_mac_can_tx_lpi(const struct eee_config *eeecfg)
+{
+ /* eee_enabled is the master on/off */
+ if (!eeecfg->eee_enabled || !eeecfg->tx_lpi_enabled)
+ return false;
+
+ return true;
+}
+
+static inline void eeecfg_to_eee(struct ethtool_keee *eee,
+ const struct eee_config *eeecfg)
+{
+ eee->tx_lpi_timer = eeecfg->tx_lpi_timer;
+ eee->tx_lpi_enabled = eeecfg->tx_lpi_enabled;
+ eee->eee_enabled = eeecfg->eee_enabled;
+}
+
+static inline void eee_to_eeecfg(struct eee_config *eeecfg,
+ const struct ethtool_keee *eee)
+{
+ eeecfg->tx_lpi_timer = eee->tx_lpi_timer;
+ eeecfg->tx_lpi_enabled = eee->tx_lpi_enabled;
+ eeecfg->eee_enabled = eee->eee_enabled;
+}
+
+#endif
--
2.39.2
From: Andrew Lunn <[email protected]>
FEC is about to get its EEE code re-written. To allow this, move
fec_enet_eee_mode_set() before fec_enet_adjust_link() which will
need to call it.
Signed-off-by: Andrew Lunn <[email protected]>
Signed-off-by: Oleksij Rempel <[email protected]>
Reviewed-by: Wei Fang <[email protected]>
---
drivers/net/ethernet/freescale/fec_main.c | 75 ++++++++++++-----------
1 file changed, 38 insertions(+), 37 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 207f1f66c117a..a2c786550342f 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -2017,6 +2017,44 @@ static int fec_get_mac(struct net_device *ndev)
/*
* Phy section
*/
+
+/* LPI Sleep Ts count base on tx clk (clk_ref).
+ * The lpi sleep cnt value = X us / (cycle_ns).
+ */
+static int fec_enet_us_to_tx_cycle(struct net_device *ndev, int us)
+{
+ struct fec_enet_private *fep = netdev_priv(ndev);
+
+ return us * (fep->clk_ref_rate / 1000) / 1000;
+}
+
+static int fec_enet_eee_mode_set(struct net_device *ndev, bool enable)
+{
+ struct fec_enet_private *fep = netdev_priv(ndev);
+ struct ethtool_keee *p = &fep->eee;
+ unsigned int sleep_cycle, wake_cycle;
+ int ret = 0;
+
+ if (enable) {
+ ret = phy_init_eee(ndev->phydev, false);
+ if (ret)
+ return ret;
+
+ sleep_cycle = fec_enet_us_to_tx_cycle(ndev, p->tx_lpi_timer);
+ wake_cycle = sleep_cycle;
+ } else {
+ sleep_cycle = 0;
+ wake_cycle = 0;
+ }
+
+ p->tx_lpi_enabled = enable;
+
+ writel(sleep_cycle, fep->hwp + FEC_LPI_SLEEP);
+ writel(wake_cycle, fep->hwp + FEC_LPI_WAKE);
+
+ return 0;
+}
+
static void fec_enet_adjust_link(struct net_device *ndev)
{
struct fec_enet_private *fep = netdev_priv(ndev);
@@ -3121,43 +3159,6 @@ static int fec_enet_set_coalesce(struct net_device *ndev,
return 0;
}
-/* LPI Sleep Ts count base on tx clk (clk_ref).
- * The lpi sleep cnt value = X us / (cycle_ns).
- */
-static int fec_enet_us_to_tx_cycle(struct net_device *ndev, int us)
-{
- struct fec_enet_private *fep = netdev_priv(ndev);
-
- return us * (fep->clk_ref_rate / 1000) / 1000;
-}
-
-static int fec_enet_eee_mode_set(struct net_device *ndev, bool enable)
-{
- struct fec_enet_private *fep = netdev_priv(ndev);
- struct ethtool_keee *p = &fep->eee;
- unsigned int sleep_cycle, wake_cycle;
- int ret = 0;
-
- if (enable) {
- ret = phy_init_eee(ndev->phydev, false);
- if (ret)
- return ret;
-
- sleep_cycle = fec_enet_us_to_tx_cycle(ndev, p->tx_lpi_timer);
- wake_cycle = sleep_cycle;
- } else {
- sleep_cycle = 0;
- wake_cycle = 0;
- }
-
- p->tx_lpi_enabled = enable;
-
- writel(sleep_cycle, fep->hwp + FEC_LPI_SLEEP);
- writel(wake_cycle, fep->hwp + FEC_LPI_WAKE);
-
- return 0;
-}
-
static int
fec_enet_get_eee(struct net_device *ndev, struct ethtool_keee *edata)
{
--
2.39.2
Hello:
This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <[email protected]>:
On Sat, 2 Mar 2024 20:52:59 +0100 you wrote:
> Hello all,
>
> with Andrew's permission I'll continue mainlining this patches:
>
> ==============================================================
>
> Most MAC drivers get EEE wrong. The API to the PHY is not very
> obvious, which is probably why. Rework the API, pushing most of the
> EEE handling into phylib core, leaving the MAC drivers to just
> enable/disable support for EEE in there change_link call back.
>
> [...]
Here is the summary with links:
- [net-next,v9,1/7] net: add helpers for EEE configuration
https://git.kernel.org/netdev/net-next/c/6f2fc8584a46
- [net-next,v9,2/7] net: phy: Add phydev->enable_tx_lpi to simplify adjust link callbacks
https://git.kernel.org/netdev/net-next/c/e3b6876ab850
- [net-next,v9,3/7] net: phy: Keep track of EEE configuration
https://git.kernel.org/netdev/net-next/c/fe0d4fd9285e
- [net-next,v9,4/7] net: phy: Immediately call adjust_link if only tx_lpi_enabled changes
https://git.kernel.org/netdev/net-next/c/3e43b903da04
- [net-next,v9,5/7] net: phy: Add phy_support_eee() indicating MAC support EEE
https://git.kernel.org/netdev/net-next/c/49168d1980e2
- [net-next,v9,6/7] net: fec: Move fec_enet_eee_mode_set() and helper earlier
https://git.kernel.org/netdev/net-next/c/aff1b8c84b44
- [net-next,v9,7/7] net: fec: Fixup EEE
https://git.kernel.org/netdev/net-next/c/6a2495adc0c8
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html