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.
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 (7):
net: phy: Add phydev->enable_tx_lpi to simplify adjust link callbacks
net: phy: Add helper to set EEE Clock stop enable bit
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 | 11 ++-
drivers/net/phy/phy.c | 55 ++++++++++++++-
drivers/net/phy/phy_device.c | 18 +++++
include/linux/phy.h | 9 ++-
include/net/eee.h | 38 ++++++++++
6 files changed, 160 insertions(+), 55 deletions(-)
create mode 100644 include/net/eee.h
--
2.39.2
From: Andrew Lunn <[email protected]>
The MAC driver changes its EEE hardware configuration in its
adjust_link callback. This is called when auto-neg
completes. Disabling EEE via eee_enabled false will trigger an
autoneg, and as a result the adjust_link callback will be called with
phydev->enable_tx_lpi set to false. Similarly, eee_enabled set to true
and with a change of advertised link modes will result in a new
autoneg, and a call the adjust_link call.
If set_eee is called with only a change to tx_lpi_enabled which does
not trigger an auto-neg, it is necessary to call the adjust_link
callback so that the MAC is reconfigured to take this change into
account.
When setting phydev->enable_tx_lpi, take both eee_enabled and
tx_lpi_enabled into account, so the MAC drivers just needs to act on
phydev->enable_tx_lpi and not the whole EEE configuration.
Signed-off-by: Andrew Lunn <[email protected]>
Signed-off-by: Oleksij Rempel <[email protected]>
---
drivers/net/phy/phy-c45.c | 11 ++++++++---
drivers/net/phy/phy.c | 25 ++++++++++++++++++++++---
2 files changed, 30 insertions(+), 6 deletions(-)
diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index c69568e7695e..217d4df59eb6 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -1549,6 +1549,8 @@ EXPORT_SYMBOL(genphy_c45_ethtool_get_eee);
* advertised, but the previously advertised link modes are
* retained. This allows EEE to be enabled/disabled in a
* non-destructive way.
+ * Returns either error code, 0 if there was no change, or positive
+ * value if there was a change which triggered auto-neg.
*/
int genphy_c45_ethtool_set_eee(struct phy_device *phydev,
struct ethtool_keee *data)
@@ -1580,9 +1582,12 @@ int genphy_c45_ethtool_set_eee(struct phy_device *phydev,
ret = genphy_c45_an_config_eee_aneg(phydev);
if (ret < 0)
return ret;
- if (ret > 0)
- return phy_restart_aneg(phydev);
-
+ if (ret > 0) {
+ ret = phy_restart_aneg(phydev);
+ if (ret < 0)
+ return ret;
+ return 1;
+ }
return 0;
}
EXPORT_SYMBOL(genphy_c45_ethtool_set_eee);
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 7f3629d56503..dad827717ad9 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -988,7 +988,8 @@ static int phy_check_link_status(struct phy_device *phydev)
if (err < 0)
phydev->enable_tx_lpi = false;
else
- phydev->enable_tx_lpi = err;
+ phydev->enable_tx_lpi = (err & phydev->eee_cfg.tx_lpi_enabled);
+
phy_link_up(phydev);
} else if (!phydev->link && phydev->state != PHY_NOLINK) {
phydev->state = PHY_NOLINK;
@@ -1679,6 +1680,21 @@ int phy_ethtool_get_eee(struct phy_device *phydev, struct ethtool_keee *data)
}
EXPORT_SYMBOL(phy_ethtool_get_eee);
+/* auto-neg not triggered, directly inform the MAC if something
+ * changed'
+ */
+static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
+ struct ethtool_keee *data)
+{
+ if (phydev->eee_cfg.tx_lpi_enabled !=
+ data->tx_lpi_enabled) {
+ eee_to_eeecfg(data, &phydev->eee_cfg);
+ phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg);
+ if (phydev->link)
+ phy_link_up(phydev);
+ }
+}
+
/**
* phy_ethtool_set_eee - set EEE supported and status
* @phydev: target phy_device struct
@@ -1695,11 +1711,14 @@ 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)
+ if (ret >= 0) {
+ if (ret == 0)
+ phy_ethtool_set_eee_noneg(phydev, data);
eee_to_eeecfg(data, &phydev->eee_cfg);
+ }
mutex_unlock(&phydev->lock);
- return ret;
+ return (ret < 0 ? ret : 0);
}
EXPORT_SYMBOL(phy_ethtool_set_eee);
--
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]>
---
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 a2c786550342..d7693fdf640d 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]>
The MAC driver can request that the PHY stops the clock during EEE
LPI. This has normally been does as part of phy_init_eee(), however
that function is overly complex and often wrongly used. Add a
standalone helper, to aid removing phy_init_eee().
Signed-off-by: Andrew Lunn <[email protected]>
Signed-off-by: Oleksij Rempel <[email protected]>
---
v2: Add missing EXPORT_SYMBOL_GPL
---
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 a54b1daf5d5f..207e68b0eec6 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1579,6 +1579,26 @@ void phy_mac_interrupt(struct phy_device *phydev)
}
EXPORT_SYMBOL(phy_mac_interrupt);
+/**
+ * phy_eee_clk_stop_enable - Clock should stop during LIP
+ * @phydev: target phy_device struct
+ *
+ * Description: Program the MMD register 3.0 setting the "Clock stop enable"
+ * bit.
+ */
+int phy_eee_clk_stop_enable(struct phy_device *phydev)
+{
+ int ret;
+
+ mutex_lock(&phydev->lock);
+ ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1,
+ MDIO_PCS_CTRL1_CLKSTOP_EN);
+ mutex_unlock(&phydev->lock);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(phy_eee_clk_stop_enable);
+
/**
* 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 a880f6d7170e..432c561f5809 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1995,6 +1995,7 @@ int phy_unregister_fixup_for_id(const char *bus_id);
int phy_unregister_fixup_for_uid(u32 phy_uid, u32 phy_uid_mask);
int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable);
+int phy_eee_clk_stop_enable(struct phy_device *phydev);
int phy_get_eee_err(struct phy_device *phydev);
int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_keee *data);
int phy_ethtool_get_eee(struct phy_device *phydev, struct ethtool_keee *data);
--
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
---
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 14224e06d69f..a54b1daf5d5f 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 e3ab2c347a59..a880f6d7170e 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]>
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]>
---
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 207f1f66c117..a2c786550342 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
From: Andrew Lunn <[email protected]>
In order for EEE to operate, both the MAC and the PHY need to support
it, similar to how pause works. Copy the pause concept and add the
call phy_support_eee() which the MAC makes after connecting the PHY to
indicate it supports EEE. phylib will then advertise EEE when auto-neg
is performed.
Signed-off-by: Andrew Lunn <[email protected]>
Signed-off-by: Oleksij Rempel <[email protected]>
---
drivers/net/phy/phy_device.c | 18 ++++++++++++++++++
include/linux/phy.h | 3 ++-
2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 2eefee970851..269d3c7f0849 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2910,6 +2910,24 @@ void phy_advertise_eee_all(struct phy_device *phydev)
}
EXPORT_SYMBOL_GPL(phy_advertise_eee_all);
+/**
+ * phy_support_eee - Enable support of EEE
+ * @phydev: target phy_device struct
+ *
+ * Description: Called by the MAC to indicate is supports Energy
+ * Efficient Ethernet. This should be called before phy_start() in
+ * order that EEE is negotiated when the link comes up as part of
+ * phy_start(). EEE is enabled by default when the hardware supports
+ * it.
+ */
+void phy_support_eee(struct phy_device *phydev)
+{
+ linkmode_copy(phydev->advertising_eee, phydev->supported_eee);
+ phydev->eee_cfg.tx_lpi_enabled = true;
+ phydev->eee_cfg.eee_enabled = true;
+}
+EXPORT_SYMBOL(phy_support_eee);
+
/**
* phy_support_sym_pause - Enable support of symmetrical pause
* @phydev: target phy_device struct
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 356916695a26..b6c5dee143d1 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -706,7 +706,7 @@ struct phy_device {
__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising);
/* used with phy_speed_down */
__ETHTOOL_DECLARE_LINK_MODE_MASK(adv_old);
- /* used for eee validation */
+ /* used for eee validation and configuration*/
__ETHTOOL_DECLARE_LINK_MODE_MASK(supported_eee);
__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising_eee);
bool eee_enabled;
@@ -1973,6 +1973,7 @@ void phy_advertise_supported(struct phy_device *phydev);
void phy_advertise_eee_all(struct phy_device *phydev);
void phy_support_sym_pause(struct phy_device *phydev);
void phy_support_asym_pause(struct phy_device *phydev);
+void phy_support_eee(struct phy_device *phydev);
void phy_set_sym_pause(struct phy_device *phydev, bool rx, bool tx,
bool autoneg);
void phy_set_asym_pause(struct phy_device *phydev, bool rx, bool tx);
--
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]>
Signed-off-by: Oleksij Rempel <[email protected]>
---
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 207e68b0eec6..7f3629d56503 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1660,8 +1660,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)
{
@@ -1672,6 +1672,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(&phydev->eee_cfg, data);
mutex_unlock(&phydev->lock);
return ret;
@@ -1694,6 +1695,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(data, &phydev->eee_cfg);
mutex_unlock(&phydev->lock);
return ret;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 432c561f5809..356916695a26 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]>
Signed-off-by: Oleksij Rempel <[email protected]>
---
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 000000000000..1232658b32f4
--- /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(const struct eee_config *eeecfg,
+ struct ethtool_keee *eee)
+{
+ 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(const struct ethtool_keee *eee,
+ struct eee_config *eeecfg)
+{
+ 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
On Wed, 21 Feb 2024 07:21:03 +0100 Oleksij Rempel wrote:
> @@ -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
missing @ in front of the name here
> * @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;
--
pw-bot: cr
On 2/20/2024 10:21 PM, Oleksij Rempel wrote:
> From: Andrew Lunn <[email protected]>
>
> The MAC driver can request that the PHY stops the clock during EEE
> LPI. This has normally been does as part of phy_init_eee(), however
> that function is overly complex and often wrongly used. Add a
> standalone helper, to aid removing phy_init_eee().
>
> Signed-off-by: Andrew Lunn <[email protected]>
> Signed-off-by: Oleksij Rempel <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
It would be useful to also read whether the PHY is capable of stopping
its clock, this has IMHO always been missing. Clause 45 IEEE PCS Status
1 Register (3.1) bit 6 reflects whether the PHY is capable of stopping
its clock.
--
Florian
On 2/20/2024 10:21 PM, Oleksij Rempel wrote:
> From: Andrew Lunn <[email protected]>
>
> The MAC driver changes its EEE hardware configuration in its
> adjust_link callback. This is called when auto-neg
> completes. Disabling EEE via eee_enabled false will trigger an
> autoneg, and as a result the adjust_link callback will be called with
> phydev->enable_tx_lpi set to false. Similarly, eee_enabled set to true
> and with a change of advertised link modes will result in a new
> autoneg, and a call the adjust_link call.
>
> If set_eee is called with only a change to tx_lpi_enabled which does
> not trigger an auto-neg, it is necessary to call the adjust_link
> callback so that the MAC is reconfigured to take this change into
> account.
>
> When setting phydev->enable_tx_lpi, take both eee_enabled and
> tx_lpi_enabled into account, so the MAC drivers just needs to act on
> phydev->enable_tx_lpi and not the whole EEE configuration.
>
> Signed-off-by: Andrew Lunn <[email protected]>
> Signed-off-by: Oleksij Rempel <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
Definitively an improvement! Minor nits below
> ---
> drivers/net/phy/phy-c45.c | 11 ++++++++---
> drivers/net/phy/phy.c | 25 ++++++++++++++++++++++---
> 2 files changed, 30 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> index c69568e7695e..217d4df59eb6 100644
> --- a/drivers/net/phy/phy-c45.c
> +++ b/drivers/net/phy/phy-c45.c
> @@ -1549,6 +1549,8 @@ EXPORT_SYMBOL(genphy_c45_ethtool_get_eee);
> * advertised, but the previously advertised link modes are
> * retained. This allows EEE to be enabled/disabled in a
> * non-destructive way.
> + * Returns either error code, 0 if there was no change, or positive
> + * value if there was a change which triggered auto-neg.
> */
> int genphy_c45_ethtool_set_eee(struct phy_device *phydev,
> struct ethtool_keee *data)
> @@ -1580,9 +1582,12 @@ int genphy_c45_ethtool_set_eee(struct phy_device *phydev,
> ret = genphy_c45_an_config_eee_aneg(phydev);
> if (ret < 0)
> return ret;
> - if (ret > 0)
> - return phy_restart_aneg(phydev);
> -
> + if (ret > 0) {
> + ret = phy_restart_aneg(phydev);
> + if (ret < 0)
> + return ret;
> + return 1;
> + }
> return 0;
> }
> EXPORT_SYMBOL(genphy_c45_ethtool_set_eee);
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 7f3629d56503..dad827717ad9 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -988,7 +988,8 @@ static int phy_check_link_status(struct phy_device *phydev)
> if (err < 0)
> phydev->enable_tx_lpi = false;
> else
> - phydev->enable_tx_lpi = err;
> + phydev->enable_tx_lpi = (err & phydev->eee_cfg.tx_lpi_enabled);
> +
> phy_link_up(phydev);
> } else if (!phydev->link && phydev->state != PHY_NOLINK) {
> phydev->state = PHY_NOLINK;
> @@ -1679,6 +1680,21 @@ int phy_ethtool_get_eee(struct phy_device *phydev, struct ethtool_keee *data)
> }
> EXPORT_SYMBOL(phy_ethtool_get_eee);
>
> +/* auto-neg not triggered, directly inform the MAC if something
> + * changed'
Stray ' character at the end here.
> + */
> +static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
> + struct ethtool_keee *data)
> +{
> + if (phydev->eee_cfg.tx_lpi_enabled !=
> + data->tx_lpi_enabled) {
> + eee_to_eeecfg(data, &phydev->eee_cfg);
> + phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg);
> + if (phydev->link)
> + phy_link_up(phydev);
> + }
> +}
> +
> /**
> * phy_ethtool_set_eee - set EEE supported and status
> * @phydev: target phy_device struct
> @@ -1695,11 +1711,14 @@ 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)
> + if (ret >= 0) {
> + if (ret == 0)
> + phy_ethtool_set_eee_noneg(phydev, data);
> eee_to_eeecfg(data, &phydev->eee_cfg);
> + }
> mutex_unlock(&phydev->lock);
>
> - return ret;
> + return (ret < 0 ? ret : 0);
Don't think the parenthesis are needed but does not hurt.
--
Florian
On 2/20/2024 10:21 PM, Oleksij Rempel wrote:
> From: Andrew Lunn <[email protected]>
>
> In order for EEE to operate, both the MAC and the PHY need to support
> it, similar to how pause works.
Kinda, a number of PHYs have added support for SmartEEE or AutoGrEEEn in
order to provide some EEE-like power savings with non-EEE capable MACs.
Oleksij did not you have a patch series at some point that introduced a
smarteee field in the phy_device structure to reflect that? I thought
that had been accepted, but maybe not.
> Copy the pause concept and add the
> call phy_support_eee() which the MAC makes after connecting the PHY to
> indicate it supports EEE. phylib will then advertise EEE when auto-neg
> is performed.
>
> Signed-off-by: Andrew Lunn <[email protected]>
> Signed-off-by: Oleksij Rempel <[email protected]>
> ---
> drivers/net/phy/phy_device.c | 18 ++++++++++++++++++
> include/linux/phy.h | 3 ++-
> 2 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 2eefee970851..269d3c7f0849 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -2910,6 +2910,24 @@ void phy_advertise_eee_all(struct phy_device *phydev)
> }
> EXPORT_SYMBOL_GPL(phy_advertise_eee_all);
>
> +/**
> + * phy_support_eee - Enable support of EEE
> + * @phydev: target phy_device struct
> + *
> + * Description: Called by the MAC to indicate is supports Energy
> + * Efficient Ethernet. This should be called before phy_start() in
> + * order that EEE is negotiated when the link comes up as part of
> + * phy_start(). EEE is enabled by default when the hardware supports
> + * it.
That comment is a bit confusing without mentioning how the hardware
default state wrt. EEE is being factored in, can we have some details here?
--
Florian
On 2/20/2024 10:21 PM, Oleksij Rempel wrote:
> 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]>
> Signed-off-by: Oleksij Rempel <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 2/20/2024 10:21 PM, Oleksij Rempel wrote:
> 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]>
> Signed-off-by: Oleksij Rempel <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
> -----Original Message-----
> From: Oleksij Rempel <[email protected]>
> Sent: 2024??2??21?? 14:21
> To: Wei Fang <[email protected]>; David S. Miller <[email protected]>;
> Eric Dumazet <[email protected]>; Jakub Kicinski <[email protected]>;
> Paolo Abeni <[email protected]>; Andrew Lunn <[email protected]>;
> Heiner Kallweit <[email protected]>; Russell King
> <[email protected]>
> Cc: Florian Fainelli <[email protected]>; Oleksij Rempel
> <[email protected]>; [email protected];
> [email protected]; [email protected]; Shenwei Wang
> <[email protected]>; Clark Wang <[email protected]>;
> dl-linux-imx <[email protected]>
> Subject: [PATCH net-next v5 2/8] net: phy: Add phydev->enable_tx_lpi to
> simplify adjust link callbacks
>
> 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
> ---
> 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
> 14224e06d69f..a54b1daf5d5f 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;
phydev->enable_tx_lpi = !!err; Is this better?
> 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
> e3ab2c347a59..a880f6d7170e 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
On Thu, Feb 22, 2024 at 08:52:25PM -0800, Florian Fainelli wrote:
>
>
> On 2/20/2024 10:21 PM, Oleksij Rempel wrote:
> > From: Andrew Lunn <[email protected]>
> >
> > In order for EEE to operate, both the MAC and the PHY need to support
> > it, similar to how pause works.
>
> Kinda, a number of PHYs have added support for SmartEEE or AutoGrEEEn in
> order to provide some EEE-like power savings with non-EEE capable MACs.
Will reword it.
> Oleksij did not you have a patch series at some point that introduced a
> smarteee field in the phy_device structure to reflect that? I thought that
> had been accepted, but maybe not.
Ack. They are pending at the end of EEE refactoring queue :)
> > Copy the pause concept and add the
> > call phy_support_eee() which the MAC makes after connecting the PHY to
> > indicate it supports EEE. phylib will then advertise EEE when auto-neg
> > is performed.
> >
> > Signed-off-by: Andrew Lunn <[email protected]>
> > Signed-off-by: Oleksij Rempel <[email protected]>
> > ---
> > drivers/net/phy/phy_device.c | 18 ++++++++++++++++++
> > include/linux/phy.h | 3 ++-
> > 2 files changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index 2eefee970851..269d3c7f0849 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -2910,6 +2910,24 @@ void phy_advertise_eee_all(struct phy_device *phydev)
> > }
> > EXPORT_SYMBOL_GPL(phy_advertise_eee_all);
> > +/**
> > + * phy_support_eee - Enable support of EEE
> > + * @phydev: target phy_device struct
> > + *
> > + * Description: Called by the MAC to indicate is supports Energy
> > + * Efficient Ethernet. This should be called before phy_start() in
> > + * order that EEE is negotiated when the link comes up as part of
> > + * phy_start(). EEE is enabled by default when the hardware supports
> > + * it.
>
> That comment is a bit confusing without mentioning how the hardware default
> state wrt. EEE is being factored in, can we have some details here?
If I see it correctly, this function set initial EEE policy for the PHY.
It should be called only once at PHY registration by the MAC and/or by
the PHY in case of SmartEEE or AutoGrEEEn PHY.
The advertisement configuration will be based on already filtered set of
supported modes.
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
> -----Original Message-----
> From: Oleksij Rempel <[email protected]>
> Sent: 2024??2??21?? 14:21
> To: Wei Fang <[email protected]>; David S. Miller <[email protected]>;
> Eric Dumazet <[email protected]>; Jakub Kicinski <[email protected]>;
> Paolo Abeni <[email protected]>; Andrew Lunn <[email protected]>;
> Heiner Kallweit <[email protected]>; Russell King
> <[email protected]>
> Cc: Oleksij Rempel <[email protected]>; [email protected];
> [email protected]; [email protected]; Shenwei Wang
> <[email protected]>; Clark Wang <[email protected]>;
> dl-linux-imx <[email protected]>
> Subject: [PATCH net-next v5 7/8] net: fec: Move fec_enet_eee_mode_set()
> and helper earlier
>
> 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]>
> ---
> 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 207f1f66c117..a2c786550342 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
Reviewed-by: Wei Fang <[email protected]>
> -----Original Message-----
> From: Oleksij Rempel <[email protected]>
> Sent: 2024??2??21?? 14:21
> To: Wei Fang <[email protected]>; David S. Miller <[email protected]>;
> Eric Dumazet <[email protected]>; Jakub Kicinski <[email protected]>;
> Paolo Abeni <[email protected]>; Andrew Lunn <[email protected]>;
> Heiner Kallweit <[email protected]>; Russell King
> <[email protected]>
> Cc: Oleksij Rempel <[email protected]>; [email protected];
> [email protected]; [email protected]; Shenwei Wang
> <[email protected]>; Clark Wang <[email protected]>;
> dl-linux-imx <[email protected]>
> Subject: [PATCH net-next v5 8/8] net: fec: Fixup EEE
>
> 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]>
> ---
> 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 a2c786550342..d7693fdf640d 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
Reviewed-by: Wei Fang <[email protected]>
On 23.02.2024 06:36, Wei Fang wrote:
>> -----Original Message-----
>> From: Oleksij Rempel <[email protected]>
>> Sent: 2024年2月21日 14:21
>> To: Wei Fang <[email protected]>; David S. Miller <[email protected]>;
>> Eric Dumazet <[email protected]>; Jakub Kicinski <[email protected]>;
>> Paolo Abeni <[email protected]>; Andrew Lunn <[email protected]>;
>> Heiner Kallweit <[email protected]>; Russell King
>> <[email protected]>
>> Cc: Florian Fainelli <[email protected]>; Oleksij Rempel
>> <[email protected]>; [email protected];
>> [email protected]; [email protected]; Shenwei Wang
>> <[email protected]>; Clark Wang <[email protected]>;
>> dl-linux-imx <[email protected]>
>> Subject: [PATCH net-next v5 2/8] net: phy: Add phydev->enable_tx_lpi to
>> simplify adjust link callbacks
>>
>> 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
>> ---
>> 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
>> 14224e06d69f..a54b1daf5d5f 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;
>
> phydev->enable_tx_lpi = !!err; Is this better?
>
I don't think so. Effectively err is a bool value, and it's implicitly
converted.
>> 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
>> e3ab2c347a59..a880f6d7170e 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
>
On Thu, Feb 22, 2024 at 08:47:58PM -0800, Florian Fainelli wrote:
>
>
> On 2/20/2024 10:21 PM, Oleksij Rempel wrote:
> > From: Andrew Lunn <[email protected]>
> >
> > The MAC driver can request that the PHY stops the clock during EEE
> > LPI. This has normally been does as part of phy_init_eee(), however
> > that function is overly complex and often wrongly used. Add a
> > standalone helper, to aid removing phy_init_eee().
> >
> > Signed-off-by: Andrew Lunn <[email protected]>
> > Signed-off-by: Oleksij Rempel <[email protected]>
>
> Reviewed-by: Florian Fainelli <[email protected]>
>
> It would be useful to also read whether the PHY is capable of stopping its
> clock, this has IMHO always been missing. Clause 45 IEEE PCS Status 1
> Register (3.1) bit 6 reflects whether the PHY is capable of stopping its
> clock.
Agreed, there is a extra set of challenges with this functionality. For
example stmmac will fail to reset DMA engine if PHY disabled clock. It will be
good to handle it in a separate patch set.
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Thu, Feb 22, 2024 at 08:52:25PM -0800, Florian Fainelli wrote:
>
>
> On 2/20/2024 10:21 PM, Oleksij Rempel wrote:
> > From: Andrew Lunn <[email protected]>
> >
> > In order for EEE to operate, both the MAC and the PHY need to support
> > it, similar to how pause works.
>
> Kinda, a number of PHYs have added support for SmartEEE or AutoGrEEEn in
> order to provide some EEE-like power savings with non-EEE capable MACs.
>
> Oleksij did not you have a patch series at some point that introduced a
> smarteee field in the phy_device structure to reflect that? I thought that
> had been accepted, but maybe not.
I have some similar hacks for the Atheros SmartEEE in my tree that I've
never published.
For SmartEEE, we need two things to happen:
1) MAC drivers must not fail set_eee()/get_eee() just because they
themselves do not support EEE.
2) MAC drivers must not attempt to modify the EEE parameters passed
to phylib.
Whether a MAC driver should be configuring the hardware in set_eee()
at all is another question - because in the case of using SmartEEE
the MAC side is irrelevant. So maybe phylib should have a callback to
set the EEE TX LPI parameters? In phylink, my model was to add two
new functions (one to enable and another to disable TX LPI) and the
enable function always gets passed the TX LPI timeout.
If we did the same in phylib, we would eliminate the need for MAC
drivers to conditionalise based on SmartEEE - that could be handled
entirely within phylib.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!