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.
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 (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 | 14 +++-
drivers/net/phy/phy.c | 70 ++++++++++++++++++-
drivers/net/phy/phy_device.c | 28 ++++++++
include/linux/phy.h | 9 ++-
include/net/eee.h | 38 ++++++++++
6 files changed, 189 insertions(+), 54 deletions(-)
create mode 100644 include/net/eee.h
--
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. With some exception - a number of PHYs
have SmartEEE or AutoGrEEEn support in order to provide some EEE-like
power savings with non-EEE capable MACs.
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]>
---
v6: reword commit message and comment for phy_support_eee()
---
drivers/net/phy/phy_device.c | 28 ++++++++++++++++++++++++++++
include/linux/phy.h | 3 ++-
2 files changed, 30 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 2eefee9708510..72452e6a478c0 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2910,6 +2910,34 @@ void phy_advertise_eee_all(struct phy_device *phydev)
}
EXPORT_SYMBOL_GPL(phy_advertise_eee_all);
+/**
+ * phy_support_eee - Set initial EEE policy configuration
+ * @phydev: Target phy_device struct
+ *
+ * This function configures the initial policy for Energy Efficient Ethernet
+ * (EEE) on the specified PHY device, influencing that EEE capabilities are
+ * advertised before the link is established. It should be called during PHY
+ * registration by the MAC driver and/or the PHY driver (for SmartEEE PHYs)
+ * if MAC supports LPI or PHY is capable to compensate missing LPI functionality
+ * of the MAC.
+ *
+ * The function sets default EEE policy parameters, including preparing the PHY
+ * to advertise EEE capabilities based on hardware support.
+ *
+ * It also sets the expected configuration for Low Power Idle (LPI) in the MAC
+ * driver. If the PHY framework determines that both local and remote
+ * advertisements support EEE, and the negotiated link mode is compatible with
+ * EEE, it will set enable_tx_lpi = true. The MAC driver is expected to act on
+ * this setting by enabling the LPI timer if enable_tx_lpi is set.
+ */
+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 c315928357c8c..661c2c969b191 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]>
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]>
Reviewed-by: Florian Fainelli <[email protected]>
Signed-off-by: Oleksij Rempel <[email protected]>
---
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 2bc0a7d51c63f..ab18b0d9beb47 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 a880f6d7170ea..432c561f58098 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]>
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.
The same check should be done for tx_lpi_timer too.
Signed-off-by: Andrew Lunn <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
Signed-off-by: Oleksij Rempel <[email protected]>
---
v6: remove ' comment and parenthesis or return
v7: - add phy_link_down() before phy_link_up()
- rewrite comment for phy_ethtool_set_eee_noneg()
- add check for changed tx_lpi_timer
v8: update phydev->link value before phy_link_down/up
---
drivers/net/phy/phy-c45.c | 14 ++++++++++++--
drivers/net/phy/phy.c | 40 ++++++++++++++++++++++++++++++++++++---
2 files changed, 49 insertions(+), 5 deletions(-)
diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 3e95b8a15f442..5695935fdce97 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -1550,6 +1550,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)
@@ -1576,8 +1578,16 @@ int genphy_c45_ethtool_set_eee(struct phy_device *phydev,
phydev->eee_enabled = data->eee_enabled;
ret = genphy_c45_an_config_eee_aneg(phydev);
- if (ret > 0)
- return phy_restart_aneg(phydev);
+ if (ret > 0) {
+ ret = phy_restart_aneg(phydev);
+ if (ret < 0)
+ return ret;
+
+ /* explicitly return 1, otherwise (ret > 0) value will be
+ * overwritten by phy_restart_aneg().
+ */
+ return 1;
+ }
return ret;
}
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index f0ed07c74a36e..b6a2af20c1f32 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,36 @@ int phy_ethtool_get_eee(struct phy_device *phydev, struct ethtool_keee *data)
}
EXPORT_SYMBOL(phy_ethtool_get_eee);
+/**
+ * phy_ethtool_set_eee_noneg - Adjusts MAC LPI configuration without PHY
+ * renegotiation
+ * @phydev: pointer to the target PHY device structure
+ * @data: pointer to the ethtool_keee structure containing the new EEE settings
+ *
+ * This function updates the Energy Efficient Ethernet (EEE) configuration
+ * for cases where only the MAC's Low Power Idle (LPI) configuration changes,
+ * without triggering PHY renegotiation. It ensures that the MAC is properly
+ * informed of the new LPI settings by cycling the link down and up, which
+ * is necessary for the MAC to adopt the new configuration. This adjustment
+ * is done only if there is a change in the tx_lpi_enabled or tx_lpi_timer
+ * configuration.
+ */
+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 ||
+ phydev->eee_cfg.tx_lpi_timer != data->tx_lpi_timer) {
+ eee_to_eeecfg(data, &phydev->eee_cfg);
+ phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg);
+ if (phydev->link) {
+ phydev->link = false;
+ phy_link_down(phydev);
+ phydev->link = true;
+ phy_link_up(phydev);
+ }
+ }
+}
+
/**
* phy_ethtool_set_eee - set EEE supported and status
* @phydev: target phy_device struct
@@ -1695,11 +1726,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: 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]>
---
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..1232658b32f40
--- /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
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 ab18b0d9beb47..f0ed07c74a36e 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 432c561f58098..c315928357c8c 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
On Fri, Mar 01, 2024 at 11:01:50AM +0100, 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.
> The same check should be done for tx_lpi_timer too.
>
> Signed-off-by: Andrew Lunn <[email protected]>
> Reviewed-by: Florian Fainelli <[email protected]>
> Signed-off-by: Oleksij Rempel <[email protected]>
> +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 ||
> + phydev->eee_cfg.tx_lpi_timer != data->tx_lpi_timer) {
> + eee_to_eeecfg(data, &phydev->eee_cfg);
> + phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg);
> + if (phydev->link) {
> + phydev->link = false;
> + phy_link_down(phydev);
> + phydev->link = true;
> + phy_link_up(phydev);
> + }
> + }
> +}
> +
Thanks
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
On 01.03.2024 11:01, 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]>
> Reviewed-by: Florian Fainelli <[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 0000000000000..1232658b32f40
> --- /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)
> +{
Typically the argument order is f(dst, src), like for string functions.
Any specific reason handle it differently here?
> + 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
On 01.03.2024 11:01, 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]>
> Reviewed-by: Florian Fainelli <[email protected]>
> Signed-off-by: Oleksij Rempel <[email protected]>
> ---
> 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 2bc0a7d51c63f..ab18b0d9beb47 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);
> +
I don't see a user of this function in the series.
Based on the commit description, wouldn't it be better to
make this patch part of a future series removing
phy_init_eee()?
> /**
> * 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 a880f6d7170ea..432c561f58098 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);
On Sat, Mar 02, 2024 at 06:16:34PM +0100, Heiner Kallweit wrote:
> On 01.03.2024 11:01, 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]>
> > Reviewed-by: Florian Fainelli <[email protected]>
> > Signed-off-by: Oleksij Rempel <[email protected]>
> > ---
> > 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 2bc0a7d51c63f..ab18b0d9beb47 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);
> > +
> I don't see a user of this function in the series.
> Based on the commit description, wouldn't it be better to
> make this patch part of a future series removing
> phy_init_eee()?
That depends who is going to do that work. If it's individual driver
maintainers, then I think we want this to go in along with this series
so that we don't end up with individual driver maintainers having to
carry this patch, and submissions ending up with multiple copies of
this patch or depending on other maintainers submissions.
On the other hand, if someone is going to go through all the network
drivers and update them as one series, then it probably makes more
sense to move this to that series.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> That depends who is going to do that work. If it's individual driver
> maintainers, then I think we want this to go in along with this series
> so that we don't end up with individual driver maintainers having to
> carry this patch, and submissions ending up with multiple copies of
> this patch or depending on other maintainers submissions.
>
> On the other hand, if someone is going to go through all the network
> drivers and update them as one series, then it probably makes more
> sense to move this to that series.
When i did this work, i did convert all drivers to the new API, and
then dropped the old API. There are too many patches for a single
series, so it makes sense to put the API in place along with one
driver conversion to show how it works, then a second series
converting all the remaining drivers, and then a cleanup series.
Andrew
On Sat, Mar 02, 2024 at 07:38:17PM +0100, Andrew Lunn wrote:
> > That depends who is going to do that work. If it's individual driver
> > maintainers, then I think we want this to go in along with this series
> > so that we don't end up with individual driver maintainers having to
> > carry this patch, and submissions ending up with multiple copies of
> > this patch or depending on other maintainers submissions.
> >
> > On the other hand, if someone is going to go through all the network
> > drivers and update them as one series, then it probably makes more
> > sense to move this to that series.
>
> When i did this work, i did convert all drivers to the new API, and
> then dropped the old API. There are too many patches for a single
> series, so it makes sense to put the API in place along with one
> driver conversion to show how it works, then a second series
> converting all the remaining drivers, and then a cleanup series.
In this series we have no driver converted to the new API. I'll move it
to separate patch series.
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 |