net: add phylink managed EEE support
Subsystems (10)
Russell King (Oracle)
maintainercover net: add phylink managed EEE support
Hi,
Adding managed EEE support to phylink has been on the cards ever since the idea in phylib was mooted. This overly large series attempts to do so. I've included all the patches as it's important to get the driver patches out there.
Patch 1 adds a definition for the clock stop capable bit in the PCS MMD status register.
Patch 2 adds a phylib API to query whether the PHY allows the transmit xMII clock to be stopped while in LPI mode. This capability is for MAC drivers to save power when LPI is active, to allow them to stop their transmit clock.
Patch 3 extracts a phylink internal helper for determining whether the link is up.
Patch 4 adds basic phylink managed EEE support. Two new MAC APIs are added, to enable and disable LPI. The enable method is passed the LPI timer setting which it is expected to program into the hardware, and also a flag ehther the transmit clock should be stopped.
I have taken the decision to make enable_tx_lpi() to return an error code, but not do much with it other than report it - the intention being that we can later use it to extend functionality if needed without reworking loads of drivers.
I have also dropped the validation/limitation of the LPI timer, and left that in the driver code prior to calling phylink_ethtool_set_eee().
The remainder of the patches convert mvneta, lan743x and stmmac, add support for mvneta, and add the basics that will be necessary into the DSA code for DSA drivers to make use of this.
I would like to get patches 1 through 9 into net-next before the merge window, but we're running out of time for that.
drivers/net/ethernet/marvell/mvneta.c | 107 +++++++++------- drivers/net/ethernet/marvell/mvpp2/mvpp2.h | 5 + drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 86 +++++++++++++ drivers/net/ethernet/microchip/lan743x_ethtool.c | 21 ---- drivers/net/ethernet/microchip/lan743x_main.c | 46 +++++-- drivers/net/ethernet/microchip/lan743x_main.h | 1 - drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 57 +++++++-- drivers/net/phy/phy.c | 20 +++ drivers/net/phy/phylink.c | 144 ++++++++++++++++++++-- include/linux/phy.h | 1 + include/linux/phylink.h | 45 +++++++ include/uapi/linux/mdio.h | 1 + net/dsa/user.c | 25 ++-- 13 files changed, 458 insertions(+), 101 deletions(-)
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
RFC PATCH 1/10 net: mdio: add definition for clock stop capable bit
Add a definition for the clock stop capable bit in the PCS MMD. This bit indicates whether the MAC is able to stop the transmit xMII clock while it is signalling LPI.
--- include/uapi/linux/mdio.h | 1 + 1 file changed, 1 insertion(+)
include/uapi/linux/mdio.h +1 -0
@@ -125,6 +125,7 @@#define MDIO_STAT1_LPOWERABLE 0x0002/* Low-power ability */ #define MDIO_STAT1_LSTATUS BMSR_LSTATUS#define MDIO_STAT1_FAULT 0x0080/* Fault */ +#define MDIO_PCS_STAT1_CLKSTOP_CAP 0x0040#define MDIO_AN_STAT1_LPABLE 0x0001/* Link partner AN ability */ #define MDIO_AN_STAT1_ABLE BMSR_ANEGCAPABLE#define MDIO_AN_STAT1_RFAULT BMSR_RFAULT
Signature · 1 line
2.30.2
RFC PATCH 2/10 net: phy: add support for querying PHY clock stop capability
In reply to Russell King (Oracle)Add support for querying whether the PHY allows the transmit xMII clock to be stopped while in LPI mode. This will be used by phylink to pass to the MAC driver so it can configure the generation of the xMII clock appropriately.
--- drivers/net/phy/phy.c | 20 ++++++++++++++++++++ include/linux/phy.h | 1 + 2 files changed, 21 insertions(+)
drivers/net/phy/phy.c +20 -0
@@ -1640,6 +1640,26 @@ void phy_mac_interrupt(struct phy_device *phydev) }EXPORT_SYMBOL (phy_mac_interrupt); +/** +* phy_eee_tx_clock_stop_capable() - indicate whether the MAC can stop tx clock +* @phydev: target phy_device struct +* +* Indicate whether the MAC can disable the transmit xMII clock while in LPI +* state. Returns 1 if the MAC may stop the transmit clock, 0 if the MAC must +* not stop the transmit clock, or negative error. +*/ +int phy_eee_tx_clock_stop_capable (struct phy_device *phydev) +{ +int stat1; + + stat1 =phy_read_mmd (phydev,MDIO_MMD_PCS ,MDIO_STAT1 ); +if (stat1 <0 ) +return stat1; + +return !!(stat1 &MDIO_PCS_STAT1_CLKSTOP_CAP ); +} +EXPORT_SYMBOL_GPL (phy_eee_tx_clock_stop_capable); +/** * phy_eee_rx_clock_stop() - configure PHY receive clock in LPI * @phydev: target phy_device struct
include/linux/phy.h +1 -0
@@ -2096,6 +2096,7 @@ int phy_unregister_fixup(const char *bus_id, u32 phy_uid, u32 phy_uid_mask);int phy_unregister_fixup_for_id (const char *bus_id);int phy_unregister_fixup_for_uid (u32 phy_uid,u32 phy_uid_mask); +int phy_eee_tx_clock_stop_capable (struct phy_device *phydev);int phy_eee_rx_clock_stop (struct phy_device *phydev,bool clk_stop_enable);int phy_init_eee (struct phy_device *phydev,bool clk_stop_enable);int phy_get_eee_err (struct phy_device *phydev);
Signature · 1 line
2.30.2
RFC PATCH 3/10 net: phylink: add phylink_link_is_up() helper
In reply to Russell King (Oracle)Add a helper to determine whether the link is up or down. Currently this is only used in one location, but becomes necessary to test when reconfiguring EEE.
--- drivers/net/phy/phylink.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
drivers/net/phy/phylink.c +6 -5
@@ -1641,20 +1641,21 @@ static void phylink_link_down(struct phylink *pl)phylink_info (pl,"Link is Down\n" ); } +static bool phylink_link_is_up (struct phylink *pl) +{ +return pl->netdev ?netif_carrier_ok (pl->netdev) : pl->old_link_state; +} +static void phylink_resolve (struct work_struct *w) { struct phylink *pl = container_of(w, struct phylink, resolve);struct phylink_link_state link_state; -struct net_device *ndev = pl->netdev;bool mac_config = false;bool retrigger = false;bool cur_link_state;mutex_lock (&pl->state_mutex); -if (pl->netdev) - cur_link_state =netif_carrier_ok (ndev); -else - cur_link_state = pl->old_link_state; + cur_link_state =phylink_link_is_up (pl);if (pl->phylink_disable_state) { pl->link_failed = false;
Signature · 1 line
2.30.2
RFC PATCH 4/10 net: phylink: add EEE management
In reply to Russell King (Oracle)Add EEE management to phylink, making use of the phylib implementation. This will only be used where a MAC driver populates the methods and capabilities bitfield, otherwise we keep our old behaviour.
Phylink will keep track of the EEE configuration, including the clock stop abilities at each end of the MAC to PHY link, programming the PHY appropriately and preserving the LPI configuration should the PHY go away.
Phylink will call into the MAC driver when LPI needs to be enabled or disabled, with the requirement that the MAC have LPI disabled prior to the netdev being brought up (in other words, it will only call mac_disable_tx_lpi() if it has already called mac_enable_tx_lpi().)
Support for phylink managed EEE is enabled by populating both tx_lpi MAC operations method pointers, and filling in both LPI interfaces and capabilities. If the methods are provided but the LPI interfaces or capabilities remain empty, this indicates to phylink that EEE is implemented by the driver but the hardware it is driving does not support EEE, and thus the ethtool set_eee() and get_eee() methods will return EOPNOTSUPP.
No validation of the LPI timer value is performed by this patch.
--- drivers/net/phy/phylink.c | 133 ++++++++++++++++++++++++++++++++++++-- include/linux/phylink.h | 45 +++++++++++++ 2 files changed, 174 insertions(+), 4 deletions(-)
drivers/net/phy/phylink.c +129 -4
@@ -81,12 +81,20 @@ struct phylink {unsigned int pcs_state;bool link_failed; +bool mac_supports_eee_ops; +bool mac_supports_eee; +bool phy_enable_tx_lpi; +bool mac_enable_tx_lpi; +bool mac_tx_clk_stop; +u32 mac_tx_lpi_timer;struct sfp_bus *sfp_bus;bool sfp_may_have_phy;DECLARE_PHY_INTERFACE_MASK (sfp_interfaces);__ETHTOOL_DECLARE_LINK_MODE_MASK (sfp_support);u8 sfp_port; + +struct eee_config eee_cfg; };#define phylink_printk (level, pl, fmt, ...) \
@@ -1574,6 +1582,52 @@ static const char *phylink_pause_to_str(int pause) } } +static void phylink_deactivate_lpi (struct phylink *pl) +{ +if (pl->mac_enable_tx_lpi) { + pl->mac_enable_tx_lpi = false; + +phylink_dbg (pl,"disabling LPI\n" ); + + pl->mac_ops->mac_disable_tx_lpi (pl->config); + } +} + +static void phylink_activate_lpi (struct phylink *pl) +{ +int err; + +if (!test_bit (pl->cur_interface, pl->config->lpi_interfaces)) { +phylink_dbg (pl,"MAC does not support LPI with %s\n" , +phy_modes (pl->cur_interface)); +return ; + } + +phylink_dbg (pl,"LPI timer %uus, tx clock stop %u\n" , + pl->mac_tx_lpi_timer, pl->mac_tx_clk_stop); + + err = pl->mac_ops->mac_enable_tx_lpi (pl->config, pl->mac_tx_lpi_timer, + pl->mac_tx_clk_stop); +if (!err) + pl->mac_enable_tx_lpi = true; +else +phylink_err (pl,"%ps() failed: %pe\n" , + pl->mac_ops->mac_enable_tx_lpi,ERR_PTR (err)); +} + +static void phylink_phy_restrict_eee (struct phylink *pl,struct phy_device *phy) +{ +__ETHTOOL_DECLARE_LINK_MODE_MASK (eee_supported); + +/* Convert the MAC's LPI capabilities to linkmodes */ +linkmode_zero (eee_supported); +phylink_caps_to_linkmodes (eee_supported, pl->config->lpi_capabilities); + +/* Mask out EEE modes that are not supported */ +linkmode_and (phy->supported_eee, phy->supported_eee, eee_supported); +linkmode_and (phy->advertising_eee, phy->advertising_eee, eee_supported); +} +static void phylink_link_up (struct phylink *pl,struct phylink_link_state link_state) {
@@ -1620,6 +1674,9 @@ static void phylink_link_up(struct phylink *pl, pl->cur_interface, speed, duplex, !!(link_state.pause & MLO_PAUSE_TX), rx_pause); +if (pl->mac_supports_eee && pl->phy_enable_tx_lpi) +phylink_activate_lpi (pl); +if (ndev)netif_carrier_on (ndev);
@@ -1636,6 +1693,9 @@ static void phylink_link_down(struct phylink *pl)if (ndev)netif_carrier_off (ndev); + +phylink_deactivate_lpi (pl); + pl->mac_ops->mac_link_down (pl->config, pl->act_link_an_mode, pl->cur_interface);phylink_info (pl,"Link is Down\n" );
@@ -1899,6 +1959,17 @@ struct phylink *phylink_create(struct phylink_config *config,return ERR_PTR (-EINVAL ); } + pl->mac_supports_eee_ops = mac_ops->mac_disable_tx_lpi && + mac_ops->mac_enable_tx_lpi; + pl->mac_supports_eee = pl->mac_supports_eee_ops && + pl->config->lpi_capabilities && + !phy_interface_empty (pl->config->lpi_interfaces); + +/* Set the default EEE configuration */ + pl->eee_cfg.eee_enabled = pl->config->eee_enabled_default; + pl->eee_cfg.tx_lpi_enabled = pl->eee_cfg.eee_enabled; + pl->eee_cfg.tx_lpi_timer = pl->config->lpi_timer_default; + pl->phy_state.interface = iface; pl->link_interface = iface;if (iface ==PHY_INTERFACE_MODE_MOCA )
@@ -2003,16 +2074,22 @@ static void phylink_phy_change(struct phy_device *phydev, bool up) pl->phy_state.link = up;if (!up) pl->link_failed = true; + +/* Get the LPI state from phylib */ + pl->phy_enable_tx_lpi = phydev->enable_tx_lpi; + pl->mac_tx_lpi_timer = phydev->eee_cfg.tx_lpi_timer;mutex_unlock (&pl->state_mutex);phylink_run_resolve (pl); -phylink_dbg (pl,"phy link %s %s/%s/%s/%s/%s\n" , up ?"up" :"down" , +phylink_dbg (pl,"phy link %s %s/%s/%s/%s/%s/%slpi\n" , + up ?"up" :"down" ,phy_modes (phydev->interface),phy_speed_to_str (phydev->speed),phy_duplex_to_str (phydev->duplex),phy_rate_matching_to_str (phydev->rate_matching), -phylink_pause_to_str (pl->phy_state.pause)); +phylink_pause_to_str (pl->phy_state.pause), + phydev->enable_tx_lpi ?"" :"no" ); } static int phylink_validate_phy(struct phylink *pl, struct phy_device *phy,
@@ -2142,6 +2219,30 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,/* Restrict the phy advertisement according to the MAC support. */ linkmode_copy (phy->advertising, config.advertising); + +/* If the MAC supports phylink managed EEE, restrict the EEE +* advertisement according to the MAC's LPI capabilities. +*/ +if (pl->mac_supports_eee) { +/* If EEE is enabled, then we need to call phy_support_eee() +* to ensure that the advertising mask is appropriately set. +* This also enables EEE at the PHY. +*/ +if (pl->eee_cfg.eee_enabled) +phy_support_eee (phy); + + phy->eee_cfg.tx_lpi_enabled = pl->eee_cfg.tx_lpi_enabled; + phy->eee_cfg.tx_lpi_timer = pl->eee_cfg.tx_lpi_timer; + +/* Restrict the PHYs EEE support/advertisement to the modes +* that the MAC supports. +*/ +phylink_phy_restrict_eee (pl, phy); + }else if (pl->mac_supports_eee_ops) { +/* MAC supports phylink EEE, but wants EEE always disabled. */ +phy_disable_eee (phy); + } +mutex_unlock (&pl->state_mutex);mutex_unlock (&phy->lock);
@@ -2157,7 +2258,13 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,if (pl->config->mac_managed_pm) phy->mac_managed_pm = true; -return 0 ; +/* Allow the MAC to stop its clock if the PHY has the capability */ + pl->mac_tx_clk_stop =phy_eee_tx_clock_stop_capable (phy) >0 ; + +/* Explicitly configure whether the PHY is allowed to stop it's +* receive clock. +*/ +return phy_eee_rx_clock_stop (phy, pl->config->eee_rx_clk_stop_enable); } static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy,
@@ -2314,6 +2421,8 @@ void phylink_disconnect_phy(struct phylink *pl)mutex_lock (&phy->lock);mutex_lock (&pl->state_mutex); pl->phydev =NULL ; + pl->phy_enable_tx_lpi = false; + pl->mac_tx_clk_stop = false;mutex_unlock (&pl->state_mutex);mutex_unlock (&phy->lock);flush_work (&pl->resolve);
@@ -3068,6 +3177,9 @@ int phylink_ethtool_get_eee(struct phylink *pl, struct ethtool_keee *eee)ASSERT_RTNL (); +if (pl->mac_supports_eee_ops && !pl->mac_supports_eee) +return ret; +if (pl->phydev) ret =phy_ethtool_get_eee (pl->phydev, eee);
@@ -3082,12 +3194,25 @@ EXPORT_SYMBOL_GPL(phylink_ethtool_get_eee);*/ int phylink_ethtool_set_eee (struct phylink *pl,struct ethtool_keee *eee) { +bool mac_eee = pl->mac_supports_eee;int ret = -EOPNOTSUPP ;ASSERT_RTNL (); -if (pl->phydev) +phylink_dbg (pl,"mac %s phylink EEE%s, adv %*pbl, LPI%s timer %uus\n" , + mac_eee ?"supports" :"does not support" , + eee->eee_enabled ?", enabled" :"" , + __ETHTOOL_LINK_MODE_MASK_NBITS, eee->advertised, + eee->tx_lpi_enabled ?" enabled" :"" , eee->tx_lpi_timer); + +if (pl->mac_supports_eee_ops && !mac_eee) +return ret; + +if (pl->phydev) { ret =phy_ethtool_set_eee (pl->phydev, eee); +if (ret ==0 ) +eee_to_eeecfg (&pl->eee_cfg, eee); + }return ret; }
include/linux/phylink.h +45 -0
90 lines not on this page · raw message
Signature · 1 line
2.30.2
Russell King (Oracle)
maintainerRe: [PATCH RFC net-next 04/10] net: phylink: add EEE management
Quoted 22 lines · Russell King (Oracle)
Add EEE management to phylink, making use of the phylib implementation. This will only be used where a MAC driver populates the methods and capabilities bitfield, otherwise we keep our old behaviour.
Phylink will keep track of the EEE configuration, including the clock stop abilities at each end of the MAC to PHY link, programming the PHY appropriately and preserving the LPI configuration should the PHY go away.
Phylink will call into the MAC driver when LPI needs to be enabled or disabled, with the requirement that the MAC have LPI disabled prior to the netdev being brought up (in other words, it will only call
mac_disable_tx_lpi()if it has already calledmac_enable_tx_lpi().)Support for phylink managed EEE is enabled by populating both
tx_lpiMAC operations method pointers, and filling in both LPI interfaces and capabilities. If the methods are provided but the LPI interfaces or capabilities remain empty, this indicates to phylink that EEE is implemented by the driver but the hardware it is driving does not support EEE, and thus the ethtoolset_eee()andget_eee()methods will return EOPNOTSUPP.
Quoted 7 lines
No validation of the LPI timer value is performed by this patch.
Signed-off-by Russell King (Oracle) <[email protected]>--- drivers/net/phy/phylink.c | 133 ++++++++++++++++++++++++++++++++++++-- include/linux/phylink.h | 45 +++++++++++++ 2 files changed, 174 insertions(+), 4 deletions(-)
In discussion with Herner, it came to my attention that the way I'm restricting EEE modes in this patch is actually buggy. I've been intending to update this patch once Herner's patch set has been merged, but this morning, Jakub asked for changes to it.
So, rather than waiting for that, I'm intending to use a different solution - rather than waiting for Herner's patch set and adapting mine to it. So, the patch below will change the way EEE modes are restricted in such a way that this is independent of Herner's series. We basically apply our own restriction to the supported modes after calling phylib's get_eee() method, and restrict the advertised modes before attaching the PHY and before calling phylib's set_eee() method.
Once Herner's patch set is merged, maybe I can manipulate the eee disabled modes mask instead, and leave this to phylib to deal with.
drivers/net/phy/phylink.c +18 -15
@@ -60,6 +60,7 @@ struct phylink {u8 act_link_an_mode;/* Active MLO_AN_xxx mode */ u8 link_port;/* The current non-phy ethtool port */ __ETHTOOL_DECLARE_LINK_MODE_MASK (supported); +__ETHTOOL_DECLARE_LINK_MODE_MASK (supported_lpi);/* The link configuration settings */ struct phylink_link_state link_config;
@@ -1623,19 +1624,6 @@ static void phylink_activate_lpi(struct phylink *pl) pl->mac_ops->mac_enable_tx_lpi, ERR_PTR(err)); } -static void phylink_phy_restrict_eee (struct phylink *pl,struct phy_device *phy) -{ -__ETHTOOL_DECLARE_LINK_MODE_MASK (eee_supported); - -/* Convert the MAC's LPI capabilities to linkmodes */ -linkmode_zero (eee_supported); -phylink_caps_to_linkmodes (eee_supported, pl->config->lpi_capabilities); - -/* Mask out EEE modes that are not supported */ -linkmode_and (phy->supported_eee, phy->supported_eee, eee_supported); -linkmode_and (phy->advertising_eee, phy->advertising_eee, eee_supported); -} -static void phylink_link_up (struct phylink *pl,struct phylink_link_state link_state) {
@@ -2242,10 +2230,16 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy, phy->eee_cfg.tx_lpi_enabled = pl->eee_cfg.tx_lpi_enabled; phy->eee_cfg.tx_lpi_timer = pl->eee_cfg.tx_lpi_timer; +/* Convert the MAC's LPI capabilities to linkmodes */ +linkmode_zero (pl->supported_lpi); +phylink_caps_to_linkmodes (pl->supported_lpi, + pl->config->lpi_capabilities); +/* Restrict the PHYs EEE support/advertisement to the modes * that the MAC supports. */ -phylink_phy_restrict_eee (pl, phy); +linkmode_and (phy->advertising_eee, phy->advertising_eee, + pl->supported_lpi); }else if (pl->mac_supports_eee_ops) {/* MAC supports phylink EEE, but wants EEE always disabled. */ phy_disable_eee (phy);
@@ -3188,8 +3182,13 @@ int phylink_ethtool_get_eee(struct phylink *pl, struct ethtool_keee *eee)if (pl->mac_supports_eee_ops && !pl->mac_supports_eee)return ret; -if (pl->phydev) +if (pl->phydev) { ret =phy_ethtool_get_eee (pl->phydev, eee); +/* Restrict supported linkmode mask */ +if (ret ==0 && pl->mac_supports_eee_ops) +linkmode_and (eee->supported, eee->supported, + pl->supported_lpi); + }return ret; }
@@ -3232,6 +3231,10 @@ int phylink_ethtool_set_eee(struct phylink *pl, struct ethtool_keee *eee) ret = -EOPNOTSUPP ;if (pl->phydev) { +/* Restrict advertisement mask */ +if (pl->mac_supports_eee_ops) +linkmode_and (eee->advertised, eee->advertised, + pl->supported_lpi); ret =phy_ethtool_set_eee (pl->phydev, eee);if (ret ==0 )eee_to_eeecfg (&pl->eee_cfg, eee);
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
RFC PATCH 5/10 net: mvneta: convert to phylink EEE implementation
In reply to Russell King (Oracle)Convert mvneta to use phylink's EEE implementation by implementing the two LPI control methods, and adding the initial configuration and capabilities.
Although disabling LPI requires clearing a single bit, for safety we clear the manual mode and force bits to ensure that auto mode will be used.
Enabling LPI needs a full configuration of several values, as the timer values are dependent on the MAC operating speed, as per the original code.
As Armada 388 states that EEE is only supported in "SGMII" modes, mark this in lpi_interfaces. Testing with RGMII on the Clearfog platform indicates that the receive path fails to detect LPI over RGMII.
--
v2: correct argument order to u32_replace_bits()
v3: split out validation and limitation of the LPI timer.
--- drivers/net/ethernet/marvell/mvneta.c | 107 ++++++++++++++++---------- 1 file changed, 65 insertions(+), 42 deletions(-)
drivers/net/ethernet/marvell/mvneta.c +65 -42
@@ -284,8 +284,12 @@ MVNETA_TXQ_BUCKET_REFILL_PERIOD))#define MVNETA_LPI_CTRL_0 0x2cc0 +#define MVNETA_LPI_CTRL_0_TS (0xff << 8)#define MVNETA_LPI_CTRL_1 0x2cc4 -#define MVNETA_LPI_REQUEST_ENABLE BIT(0) +#define MVNETA_LPI_CTRL_1_REQUEST_ENABLE BIT(0) +#define MVNETA_LPI_CTRL_1_REQUEST_FORCE BIT(1) +#define MVNETA_LPI_CTRL_1_MANUAL_MODE BIT(2) +#define MVNETA_LPI_CTRL_1_TW (0xfff << 4)#define MVNETA_LPI_CTRL_2 0x2cc8#define MVNETA_LPI_STATUS 0x2ccc
@@ -541,10 +545,6 @@ struct mvneta_port {struct mvneta_bm_pool *pool_short;int bm_win_id; -bool eee_enabled; -bool eee_active; -bool tx_lpi_enabled; -u64 ethtool_stats[ARRAY_SIZE (mvneta_statistics)];u32 indir[MVNETA_RSS_LU_TABLE_SIZE ];
@@ -4213,18 +4213,6 @@ static int mvneta_mac_finish(struct phylink_config *config, unsigned int mode,return 0 ; } -static void mvneta_set_eee (struct mvneta_port *pp,bool enable) -{ -u32 lpi_ctl1; - - lpi_ctl1 =mvreg_read (pp,MVNETA_LPI_CTRL_1 ); -if (enable) - lpi_ctl1 |=MVNETA_LPI_REQUEST_ENABLE ; -else - lpi_ctl1 &= ~MVNETA_LPI_REQUEST_ENABLE ; -mvreg_write (pp,MVNETA_LPI_CTRL_1 , lpi_ctl1); -} -static void mvneta_mac_link_down (struct phylink_config *config,unsigned int mode,phy_interface_t interface) {
@@ -4240,9 +4228,6 @@ static void mvneta_mac_link_down(struct phylink_config *config, val |=MVNETA_GMAC_FORCE_LINK_DOWN ;mvreg_write (pp,MVNETA_GMAC_AUTONEG_CONFIG , val); } - - pp->eee_active = false; -mvneta_set_eee (pp, false); } static void mvneta_mac_link_up(struct phylink_config *config,
@@ -4291,11 +4276,56 @@ static void mvneta_mac_link_up(struct phylink_config *config, }mvneta_port_up (pp); +} -if (phy && pp->eee_enabled) { - pp->eee_active =phy_init_eee (phy, false) >=0 ; -mvneta_set_eee (pp, pp->eee_active && pp->tx_lpi_enabled); +static void mvneta_mac_disable_tx_lpi (struct phylink_config *config) +{ +struct mvneta_port *pp =netdev_priv (to_net_dev (config->dev)); +u32 lpi1; + + lpi1 =mvreg_read (pp,MVNETA_LPI_CTRL_1 ); + lpi1 &= ~(MVNETA_LPI_CTRL_1_REQUEST_ENABLE | +MVNETA_LPI_CTRL_1_REQUEST_FORCE | +MVNETA_LPI_CTRL_1_MANUAL_MODE ); +mvreg_write (pp,MVNETA_LPI_CTRL_1 , lpi1); +} + +static int mvneta_mac_enable_tx_lpi (struct phylink_config *config,u32 timer, +bool tx_clk_stop) +{ +struct mvneta_port *pp =netdev_priv (to_net_dev (config->dev)); +u32 ts, tw, lpi0, lpi1, status; + + status =mvreg_read (pp,MVNETA_GMAC_STATUS ); +if (status &MVNETA_GMAC_SPEED_1000 ) { +/* At 1G speeds, the timer resolution are 1us, and +* 802.3 says tw is 16.5us. Round up to 17us. +*/ + tw =17 ; + ts = timer; + }else { +/* At 100M speeds, the timer resolutions are 10us, and +* 802.3 says tw is 30us. +*/ + tw =3 ; + ts =DIV_ROUND_UP (timer,10 ); } + +if (ts >255 ) + ts =255 ; + +/* Configure ts */ + lpi0 =mvreg_read (pp,MVNETA_LPI_CTRL_0 ); + lpi0 =u32_replace_bits (lpi0, ts,MVNETA_LPI_CTRL_0_TS ); +mvreg_write (pp,MVNETA_LPI_CTRL_0 , lpi0); + +/* Configure tw and enable LPI generation */ + lpi1 =mvreg_read (pp,MVNETA_LPI_CTRL_1 ); + lpi1 =u32_replace_bits (lpi1, tw,MVNETA_LPI_CTRL_1_TW ); + lpi1 |=MVNETA_LPI_CTRL_1_REQUEST_ENABLE ; +mvreg_write (pp,MVNETA_LPI_CTRL_1 , lpi1); + +return 0 ; }static const struct phylink_mac_ops mvneta_phylink_ops = {
@@ -4305,6 +4335,8 @@ static const struct phylink_mac_ops mvneta_phylink_ops = { .mac_finish = mvneta_mac_finish, .mac_link_down = mvneta_mac_link_down, .mac_link_up = mvneta_mac_link_up, + .mac_disable_tx_lpi = mvneta_mac_disable_tx_lpi, + .mac_enable_tx_lpi = mvneta_mac_enable_tx_lpi, };static int mvneta_mdio_probe (struct mvneta_port *pp)
@@ -5106,14 +5138,6 @@ static int mvneta_ethtool_get_eee(struct net_device *dev, struct ethtool_keee *eee) {struct mvneta_port *pp =netdev_priv (dev); -u32 lpi_ctl0; - - lpi_ctl0 =mvreg_read (pp,MVNETA_LPI_CTRL_0 ); - - eee->eee_enabled = pp->eee_enabled; - eee->eee_active = pp->eee_active; - eee->tx_lpi_enabled = pp->tx_lpi_enabled; - eee->tx_lpi_timer = (lpi_ctl0) >>8 ;// * scale; return phylink_ethtool_get_eee (pp->phylink, eee); }
@@ -5122,7 +5146,6 @@ static int mvneta_ethtool_set_eee(struct net_device *dev, struct ethtool_keee *eee) {struct mvneta_port *pp =netdev_priv (dev); -u32 lpi_ctl0;/* The Armada 37x documents do not give limits for this other than * it being an 8-bit register.
@@ -5130,16 +5153,6 @@ static int mvneta_ethtool_set_eee(struct net_device *dev,if (eee->tx_lpi_enabled && eee->tx_lpi_timer >255 )return -EINVAL ; - lpi_ctl0 =mvreg_read (pp,MVNETA_LPI_CTRL_0 ); - lpi_ctl0 &= ~(0xff <<8 ); - lpi_ctl0 |= eee->tx_lpi_timer <<8 ; -mvreg_write (pp,MVNETA_LPI_CTRL_0 , lpi_ctl0); - - pp->eee_enabled = eee->eee_enabled; - pp->tx_lpi_enabled = eee->tx_lpi_enabled; - -mvneta_set_eee (pp, eee->tx_lpi_enabled && eee->eee_enabled); -return phylink_ethtool_set_eee (pp->phylink, eee); }
@@ -5453,6 +5466,9 @@ static int mvneta_port_power_up(struct mvneta_port *pp, int phy_mode) !phy_interface_mode_is_rgmii(phy_mode))return -EINVAL ; +/* Ensure LPI is disabled */ +mvneta_mac_disable_tx_lpi (&pp->phylink_config); +return 0 ; }
@@ -5544,6 +5560,13 @@ static int mvneta_probe(struct platform_device *pdev) pp->phylink_config.mac_capabilities =MAC_SYM_PAUSE |MAC_10 |MAC_100 |MAC_1000FD |MAC_2500FD ; +/* Setup EEE. Choose 250us idle. Only supported in SGMII modes. */ +__set_bit (PHY_INTERFACE_MODE_QSGMII , pp->phylink_config.lpi_interfaces); +__set_bit (PHY_INTERFACE_MODE_SGMII , pp->phylink_config.lpi_interfaces); + pp->phylink_config.lpi_capabilities =MAC_100FD |MAC_1000FD ; + pp->phylink_config.lpi_timer_default =250 ; + pp->phylink_config.eee_enabled_default = true; +phy_interface_set_rgmii (pp->phylink_config.supported_interfaces);__set_bit (PHY_INTERFACE_MODE_QSGMII , pp->phylink_config.supported_interfaces);
Signature · 1 line
2.30.2
RFC PATCH 6/10 net: mvpp2: add EEE implementation
In reply to Russell King (Oracle)Add EEE support for mvpp2, using phylink's EEE implementation, which means we just need to implement the two methods for LPI control, and with the initial configuration. Only SGMII mode is supported, so only 100M and 1G speeds.
Disabling LPI requires clearing a single bit. Enabling LPI needs a full configuration of several values, as the timer values are dependent on the MAC operating speed.
--
v3: split LPI timer limit and validation into separate patches
--- drivers/net/ethernet/marvell/mvpp2/mvpp2.h | 5 ++ .../net/ethernet/marvell/mvpp2/mvpp2_main.c | 86 +++++++++++++++++++ 2 files changed, 91 insertions(+)
drivers/net/ethernet/marvell/mvpp2/mvpp2.h +5 -0
@@ -481,6 +481,11 @@#define MVPP22_GMAC_INT_SUM_MASK 0xa4#define MVPP22_GMAC_INT_SUM_MASK_LINK_STAT BIT(1)#define MVPP22_GMAC_INT_SUM_MASK_PTP BIT(2) +#define MVPP2_GMAC_LPI_CTRL0 0xc0 +#define MVPP2_GMAC_LPI_CTRL0_TS_MASK GENMASK(8, 8) +#define MVPP2_GMAC_LPI_CTRL1 0xc4 +#define MVPP2_GMAC_LPI_CTRL1_REQ_EN BIT(0) +#define MVPP2_GMAC_LPI_CTRL1_TW_MASK GENMASK(15, 4)/* Per-port XGMAC registers. PPv2.2 and PPv2.3, only for GOP port 0, * relative to port->base.
drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c +86 -0
122 lines not on this page · raw message
Signature · 1 line
2.30.2
Maxime Chevallier
maintainerRe: [PATCH RFC net-next 06/10] net: mvpp2: add EEE implementation
Hello Russell,
Quoted 22 lines · On Tue, 14 Jan 2025 14:02:29 +0000 "Russell King (Oracle)"…
Add EEE support for mvpp2, using phylink's EEE implementation, which means we just need to implement the two methods for LPI control, and with the initial configuration. Only SGMII mode is supported, so only 100M and 1G speeds.
Disabling LPI requires clearing a single bit. Enabling LPI needs a full configuration of several values, as the timer values are dependent on the MAC operating speed.
Signed-off-by Russell King (Oracle) <[email protected]>--
v3: split LPI timer limit and validation into separate patches--- drivers/net/ethernet/marvell/mvpp2/mvpp2.h | 5 ++ .../net/ethernet/marvell/mvpp2/mvpp2_main.c | 86 +++++++++++++++++++ 2 files changed, 91 insertions(+)drivers/net/ethernet/marvell/mvpp2/mvpp2.h +0
-0
drivers/net/ethernet/marvell/mvpp2/mvpp2.h line 485
Quoted 5 lines
drivers/net/ethernet/marvell/mvpp2/mvpp2.h +2
-0@@ -481,6 +481,11 @@#define MVPP22_GMAC_INT_SUM_MASK 0xa4#define MVPP22_GMAC_INT_SUM_MASK_LINK_STAT BIT(1)#define MVPP22_GMAC_INT_SUM_MASK_PTP BIT(2) +#define MVPP2_GMAC_LPI_CTRL0 0xc0 +#define MVPP2_GMAC_LPI_CTRL0_TS_MASK GENMASK(8, 8)
I think this should be GENMASK(15, 8) :)
The rest looks good to me,
Thanks,
Maxime
Russell King (Oracle)
maintainerRe: [PATCH RFC net-next 06/10] net: mvpp2: add EEE implementation
Quoted 22 lines · Maxime Chevallier
Hello Russell,
Quoted 17 lines · On Tue, 14 Jan 2025 14:02:29 +0000 "Russell King (Oracle)"…
Add EEE support for mvpp2, using phylink's EEE implementation, which means we just need to implement the two methods for LPI control, and with the initial configuration. Only SGMII mode is supported, so only 100M and 1G speeds.
Disabling LPI requires clearing a single bit. Enabling LPI needs a full configuration of several values, as the timer values are dependent on the MAC operating speed.
Signed-off-by Russell King (Oracle) <[email protected]>--
v3: split LPI timer limit and validation into separate patches--- drivers/net/ethernet/marvell/mvpp2/mvpp2.h | 5 ++ .../net/ethernet/marvell/mvpp2/mvpp2_main.c | 86 +++++++++++++++++++ 2 files changed, 91 insertions(+)
Quoted 12 lines
drivers/net/ethernet/marvell/mvpp2/mvpp2.h line 485
Quoted 10 lines
drivers/net/ethernet/marvell/mvpp2/mvpp2.h +2
-0@@ -481,6 +481,11 @@#define MVPP22_GMAC_INT_SUM_MASK 0xa4#define MVPP22_GMAC_INT_SUM_MASK_LINK_STAT BIT(1)#define MVPP22_GMAC_INT_SUM_MASK_PTP BIT(2) +#define MVPP2_GMAC_LPI_CTRL0 0xc0 +#define MVPP2_GMAC_LPI_CTRL0_TS_MASK GENMASK(8, 8)I think this should be
GENMASK(15, 8):)
Bah.
Thanks for spotting... and people say GENMASK() is better than using hex. I've disagreed with that, but tried to conform, but I keep making mistakes with GENMASK(), but never have with hex notation. Maybe I'm just different from most people, having been programming computers in assembly since the 1980s, where the choices were decimal or hex!
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
RFC PATCH 7/10 net: lan743x: use netdev in lan743x_phylink_mac_link_down()
In reply to Russell King (Oracle)Use the netdev that we already have in lan743x_phylink_mac_link_down().
--- drivers/net/ethernet/microchip/lan743x_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/microchip/lan743x_main.c +1 -1
@@ -3029,7 +3029,7 @@ static void lan743x_phylink_mac_link_down(struct phylink_config *config,struct net_device *netdev =to_net_dev (config->dev);struct lan743x_adapter *adapter =netdev_priv (netdev); -netif_tx_stop_all_queues (to_net_dev (config->dev)); +netif_tx_stop_all_queues (netdev);lan743x_mac_eee_enable (adapter, false); }
Signature · 1 line
2.30.2
RFC PATCH 8/10 net: lan743x: convert to phylink managed EEE
In reply to Russell King (Oracle)Convert lan743x to phylink managed EEE:
- Set the lpi_capabilties.
- Move the call to lan743x_mac_eee_enable() into the enable/disable tx_lpi functions.
- Ensure that EEEEN is clear during probe.
- Move the setting of the LPI timer into mac_enable_tx_lpi().
- Move reading of LPI timer to phylink initialisation to set the default timer value.
--- .../net/ethernet/microchip/lan743x_ethtool.c | 21 --------- drivers/net/ethernet/microchip/lan743x_main.c | 44 ++++++++++++++++--- drivers/net/ethernet/microchip/lan743x_main.h | 1 - 3 files changed, 38 insertions(+), 28 deletions(-)
drivers/net/ethernet/microchip/lan743x_ethtool.c +0 -21
@@ -1055,9 +1055,6 @@ static int lan743x_ethtool_get_eee(struct net_device *netdev, {struct lan743x_adapter *adapter =netdev_priv (netdev); - eee->tx_lpi_timer =lan743x_csr_read (adapter, -MAC_EEE_TX_LPI_REQ_DLY_CNT ); -return phylink_ethtool_get_eee (adapter->phylink, eee); }
@@ -1065,24 +1062,6 @@ static int lan743x_ethtool_set_eee(struct net_device *netdev, struct ethtool_keee *eee) {struct lan743x_adapter *adapter =netdev_priv (netdev); -u32 tx_lpi_timer; - - tx_lpi_timer =lan743x_csr_read (adapter,MAC_EEE_TX_LPI_REQ_DLY_CNT ); -if (tx_lpi_timer != eee->tx_lpi_timer) { -u32 mac_cr =lan743x_csr_read (adapter,MAC_CR ); - -/* Software should only change this field when Energy Efficient -* Ethernet Enable (EEEEN) is cleared. -* This function will trigger an autonegotiation restart and -* eee will be reenabled during link up if eee was negotiated. -*/ -lan743x_mac_eee_enable (adapter, false); -lan743x_csr_write (adapter,MAC_EEE_TX_LPI_REQ_DLY_CNT , - eee->tx_lpi_timer); - -if (mac_cr &MAC_CR_EEE_EN_ ) -lan743x_mac_eee_enable (adapter, true); - }return phylink_ethtool_set_eee (adapter->phylink, eee); }
drivers/net/ethernet/microchip/lan743x_main.c +38 -6
89 lines not on this page · raw message
drivers/net/ethernet/microchip/lan743x_main.h +0 -1
@@ -1206,6 +1206,5 @@ void lan743x_hs_syslock_release(struct lan743x_adapter *adapter);void lan743x_mac_flow_ctrl_set_enables (struct lan743x_adapter *adapter,bool tx_enable,bool rx_enable);int lan743x_sgmii_read (struct lan743x_adapter *adapter,u8 mmd,u16 addr); -void lan743x_mac_eee_enable (struct lan743x_adapter *adapter,bool enable);#endif /* _LAN743X_H */
Signature · 1 line
2.30.2
RFC PATCH 9/10 net: stmmac: convert to phylink managed EEE support
In reply to Russell King (Oracle)Convert stmmac to use phylink managed EEE support rather than delving into phylib:
1. Move the stmmac_eee_init() calls out of mac_link_down() and mac_link_up() methods into the new mac_{enable,disable}_lpi() methods. We leave the calls to stmmac_set_eee_pls() in place as these change bits which tell the EEE hardware when the link came up or down, and is used for a separate hardware timer. However, symmetrically conditionalise this with priv->dma_cap.eee.
2. Update the current LPI timer each time LPI is enabled - which we need for software-timed LPI.
3. With phylink managed EEE, phylink manages the receive clock stop configuration via phylink_config.eee_rx_clk_stop_enable. Set this appropriately which makes the call to phy_eee_rx_clock_stop() redundant.
4. From what I can work out, all supported interfaces support LPI signalling on stmmac (there's no restriction implemented.) It also appears to support LPI at all full duplex speeds at or over 100M. Set these capabilities.
5. The default timer appears to be derived from a module parameter. Set this the same, although we keep code that reconfigures the timer in stmmac_init_phy().
6. Remove the direct call to phy_support_eee(), which phylink will do on the drivers behalf if phylink_config.eee_enabled_default is set.
--- .../net/ethernet/stmicro/stmmac/stmmac_main.c | 57 +++++++++++++++---- 1 file changed, 45 insertions(+), 12 deletions(-)
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +45 -12
@@ -988,8 +988,8 @@ static void stmmac_mac_link_down(struct phylink_config *config,struct stmmac_priv *priv =netdev_priv (to_net_dev (config->dev));stmmac_mac_set (priv, priv->ioaddr, false); -stmmac_eee_init (priv, false); -stmmac_set_eee_pls (priv, priv->hw, false); +if (priv->dma_cap.eee) +stmmac_set_eee_pls (priv, priv->hw, false);if (stmmac_fpe_supported (priv))stmmac_fpe_link_state_handle (priv, false);
@@ -1096,13 +1096,8 @@ static void stmmac_mac_link_up(struct phylink_config *config,writel (ctrl, priv->ioaddr +MAC_CTRL_REG );stmmac_mac_set (priv, priv->ioaddr, true); -if (phy && priv->dma_cap.eee) { -phy_eee_rx_clock_stop (phy, !(priv->plat->flags & -STMMAC_FLAG_RX_CLK_RUNS_IN_LPI )); - priv->tx_lpi_timer = phy->eee_cfg.tx_lpi_timer; -stmmac_eee_init (priv, phy->enable_tx_lpi); +if (priv->dma_cap.eee)stmmac_set_eee_pls (priv, priv->hw, true); - }if (stmmac_fpe_supported (priv))stmmac_fpe_link_state_handle (priv, true);
@@ -1111,12 +1106,32 @@ static void stmmac_mac_link_up(struct phylink_config *config,stmmac_hwtstamp_correct_latency (priv, priv); } +static void stmmac_mac_disable_tx_lpi (struct phylink_config *config) +{ +struct stmmac_priv *priv =netdev_priv (to_net_dev (config->dev)); + +stmmac_eee_init (priv, false); +} + +static int stmmac_mac_enable_tx_lpi (struct phylink_config *config,u32 timer, +bool tx_clk_stop) +{ +struct stmmac_priv *priv =netdev_priv (to_net_dev (config->dev)); + + priv->tx_lpi_timer = timer; +stmmac_eee_init (priv, true); + +return 0 ; +} +static const struct phylink_mac_ops stmmac_phylink_mac_ops = { .mac_get_caps = stmmac_mac_get_caps, .mac_select_pcs = stmmac_mac_select_pcs, .mac_config = stmmac_mac_config, .mac_link_down = stmmac_mac_link_down, .mac_link_up = stmmac_mac_link_up, + .mac_disable_tx_lpi = stmmac_mac_disable_tx_lpi, + .mac_enable_tx_lpi = stmmac_mac_enable_tx_lpi, };/**
@@ -1189,9 +1204,6 @@ static int stmmac_init_phy(struct net_device *dev)return -ENODEV ; } -if (priv->dma_cap.eee) -phy_support_eee (phydev); - ret =phylink_connect_phy (priv->phylink, phydev); }else {fwnode_handle_put (phy_fwnode);
@@ -1201,7 +1213,12 @@ static int stmmac_init_phy(struct net_device *dev)if (ret ==0 ) {struct ethtool_keee eee; -/* Configure phylib's copy of the LPI timer */ +/* Configure phylib's copy of the LPI timer. Normally, +* phylink_config.lpi_timer_default would do this, but there is +* a chance that userspace could change the eee_timer setting +* via sysfs before the first open. Thus, preserve existing +* behaviour. +*/ if (!phylink_ethtool_get_eee (priv->phylink, &eee)) { eee.tx_lpi_timer = priv->tx_lpi_timer;phylink_ethtool_set_eee (priv->phylink, &eee);
@@ -1234,6 +1251,9 @@ static int stmmac_phy_setup(struct stmmac_priv *priv)/* Stmmac always requires an RX clock for hardware initialization */ priv->phylink_config.mac_requires_rxc = true; +if (!(priv->plat->flags &STMMAC_FLAG_RX_CLK_RUNS_IN_LPI )) + priv->phylink_config.eee_rx_clk_stop_enable = true; + mdio_bus_data = priv->plat->mdio_bus_data;if (mdio_bus_data) priv->phylink_config.default_an_inband =
@@ -1255,6 +1275,19 @@ static int stmmac_phy_setup(struct stmmac_priv *priv) priv->phylink_config.supported_interfaces, pcs->supported_interfaces); +if (priv->dma_cap.eee) { +/* Assume all supported interfaces also support LPI */ +memcpy (priv->phylink_config.lpi_interfaces, + priv->phylink_config.supported_interfaces, +sizeof (priv->phylink_config.lpi_interfaces)); + +/* All full duplex speeds above 100Mbps are supported */ + priv->phylink_config.lpi_capabilities = ~(MAC_1000FD -1 ) | +MAC_100FD ; + priv->phylink_config.lpi_timer_default = eee_timer *1000 ; + priv->phylink_config.eee_enabled_default = true; + } + fwnode = priv->plat->port_node;if (!fwnode) fwnode =dev_fwnode (priv->device);
Signature · 1 line
2.30.2
RFC PATCH 10/10 net: dsa: allow use of phylink managed EEE support
In reply to Russell King (Oracle)In order to allow DSA drivers to use phylink managed EEE, changes are necessary to the DSA .set_eee() and .get_eee() methods. Where drivers make use of phylink managed EEE, these should just pass the method on to their phylink implementation without calling the DSA specific operations.
--- net/dsa/user.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)
net/dsa/user.c +16 -9
@@ -1233,16 +1233,23 @@ static int dsa_user_set_eee(struct net_device *dev, struct ethtool_keee *e)if (!ds->ops->support_eee || !ds->ops->support_eee (ds, dp->index))return -EOPNOTSUPP ; -/* Port's PHY and MAC both need to be EEE capable */ -if (!dev->phydev) -return -ENODEV ; - -if (!ds->ops->set_mac_eee) -return -EOPNOTSUPP ; +/* If the port is using phylink managed EEE, then get_mac_eee is +* unnecessary. +*/ +if (!ds->phylink_mac_ops || + !ds->phylink_mac_ops->mac_disable_tx_lpi || + !ds->phylink_mac_ops->mac_enable_tx_lpi) { +/* Port's PHY and MAC both need to be EEE capable */ +if (!dev->phydev) +return -ENODEV ; + +if (!ds->ops->set_mac_eee) +return -EOPNOTSUPP ; - ret = ds->ops->set_mac_eee (ds, dp->index, e); -if (ret) -return ret; + ret = ds->ops->set_mac_eee (ds, dp->index, e); +if (ret) +return ret; + }return phylink_ethtool_set_eee (dp->pl, e); }
Signature · 1 line
2.30.2
Vladimir Oltean
maintainerRe: [PATCH RFC net-next 10/10] net: dsa: allow use of phylink managed EEE support
Quoted 8 lines · Russell King (Oracle)
In order to allow DSA drivers to use phylink managed EEE, changes are necessary to the DSA .set_eee() and .get_eee() methods. Where drivers make use of phylink managed EEE, these should just pass the method on to their phylink implementation without calling the DSA specific operations.
Signed-off-by Russell King (Oracle) <[email protected]>---
What is the reason for including this patch with this set, where it is of no use until at least one DSA driver provides the new API implementations?
Quoted 8 lines
net/dsa/user.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)net/dsa/user.c +0
-0
net/dsa/user.c static int dsa_user_set_eee(struct net_device *dev, struct ethtool_keee *e) · line 1236
Quoted 10 lines
net/dsa/user.c +1
-6@@ -1233,16 +1233,23 @@ static int dsa_user_set_eee(struct net_device *dev, struct ethtool_keee *e)if (!ds->ops->support_eee || !ds->ops->support_eee (ds, dp->index))return -EOPNOTSUPP ; -/* Port's PHY and MAC both need to be EEE capable */ -if (!dev->phydev) -return -ENODEV ; - -if (!ds->ops->set_mac_eee) -return -EOPNOTSUPP ; +/* If the port is using phylink managed EEE, then get_mac_eee is
set_mac_eee() is what is unnecessary.
net/dsa/user.c static int dsa_user_set_eee(struct net_device *dev, struct ethtool_keee *e) · line 1241
Quoted 5 lines
net/dsa/user.c +5
-0+* unnecessary. +*/ +if (!ds->phylink_mac_ops || + !ds->phylink_mac_ops->mac_disable_tx_lpi || + !ds->phylink_mac_ops->mac_enable_tx_lpi) {
Does it make sense to export pl->mac_supports_eee_ops wrapped into a helper function and call that here? To avoid making DSA too tightly coupled with the phylink MAC operation names.
Quoted 20 lines
net/dsa/user.c +10
-3+/* Port's PHY and MAC both need to be EEE capable */ +if (!dev->phydev) +return -ENODEV ; + +if (!ds->ops->set_mac_eee) +return -EOPNOTSUPP ; - ret = ds->ops->set_mac_eee (ds, dp->index, e); -if (ret) -return ret; + ret = ds->ops->set_mac_eee (ds, dp->index, e); +if (ret) +return ret; + }return phylink_ethtool_set_eee (dp->pl, e); }Signature · 1 line
2.30.2
Russell King (Oracle)
maintainerRe: [PATCH RFC net-next 10/10] net: dsa: allow use of phylink managed EEE support
Quoted 7 lines · Vladimir Oltean
Quoted 6 lines · Russell King (Oracle)
In order to allow DSA drivers to use phylink managed EEE, changes are necessary to the DSA .set_eee() and .get_eee() methods. Where drivers make use of phylink managed EEE, these should just pass the method on to their phylink implementation without calling the DSA specific operations.
Quoted 6 lines
Quoted 2 lines
Signed-off-by Russell King (Oracle) <[email protected]>---
What is the reason for including this patch with this set, where it is of no use until at least one DSA driver provides the new API implementations?
No criticism against you, but I guess you didn't read the cover message? I tend not to read cover messages. I've been wondering for a while now whether anyone actually does and thus whether they are worth the effort of writing anything beyond providing a message ID to tie a series together and a diffstat for the series.
Here's the relevant bit:
"The remainder of the patches convert mvneta, lan743x and stmmac, add support for mvneta, and add the basics that will be necessary into the DSA code for DSA drivers to make use of this.
"I would like to get patches 1 through 9 into net-next before the merge window, but we're running out of time for that."
So, it's included in this RFC series not because I'm intending it to be merged, but so that people can see what DSA requires to make it functional there, and provide review comments if they see fit - which you have done, thanks.
I'm sure if I'd said "I have patches for DSA" you'd have responded asking to see them!
Of course, I do have changes that will require this - mt753x - but that patch is not quite ready because this series I've posted has seen a few changes recently to remove stuff that was never settled (the LPI timer questions, whether it should be validated, clamped, should phylink provide a software timer if the LPI timer is out of range, etc.) That's more proof that text in cover messages is utterly useless!
Quoted 11 lines
Quoted 11 lines
net/dsa/user.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)net/dsa/user.c +0
-0@@ -1233,16 +1233,23 @@ static int dsa_user_set_eee(struct net_device *dev, struct ethtool_keee *e)if (!ds->ops->support_eee || !ds->ops->support_eee (ds, dp->index))return -EOPNOTSUPP ;
Quoted 9 lines
net/dsa/user.c static int dsa_user_set_eee(struct net_device *dev, struct ethtool_keee *e) · line 1236
Quoted 7 lines
net/dsa/user.c +1
-6-/* Port's PHY and MAC both need to be EEE capable */ -if (!dev->phydev) -return -ENODEV ; - -if (!ds->ops->set_mac_eee) -return -EOPNOTSUPP ; +/* If the port is using phylink managed EEE, then get_mac_eee is
set_mac_eee()is what is unnecessary.
Thanks for spotting that.
Quoted 9 lines
net/dsa/user.c static int dsa_user_set_eee(struct net_device *dev, struct ethtool_keee *e) · line 1241
Quoted 5 lines
net/dsa/user.c +5
-0+* unnecessary. +*/ +if (!ds->phylink_mac_ops || + !ds->phylink_mac_ops->mac_disable_tx_lpi || + !ds->phylink_mac_ops->mac_enable_tx_lpi) {Does it make sense to export
pl->mac_supports_eee_opswrapped into a helper function and call that here? To avoid making DSA too tightly coupled with the phylink MAC operation names.
I could wrap the test up in an inline function which would avoid the duplication. Thanks for the suggestion.
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Vladimir Oltean
maintainerRe: [PATCH RFC net-next 10/10] net: dsa: allow use of phylink managed EEE support
Quoted 27 lines · Russell King (Oracle)
Quoted 3 lines
What is the reason for including this patch with this set, where it is of no use until at least one DSA driver provides the new API implementations?
No criticism against you, but I guess you didn't read the cover message? I tend not to read cover messages. I've been wondering for a while now whether anyone actually does and thus whether they are worth the effort of writing anything beyond providing a message ID to tie a series together and a diffstat for the series.
Here's the relevant bit:
"The remainder of the patches convert mvneta, lan743x and stmmac, add support for mvneta, and add the basics that will be necessary into the DSA code for DSA drivers to make use of this.
"I would like to get patches 1 through 9 into net-next before the merge window, but we're running out of time for that."
So, it's included in this RFC series not because I'm intending it to be merged, but so that people can see what DSA requires to make it functional there, and provide review comments if they see fit - which you have done, thanks.
I'm sure if I'd said "I have patches for DSA" you'd have responded asking to see them!
Quoted 7 lines
Of course, I do have changes that will require this - mt753x - but that patch is not quite ready because this series I've posted has seen a few changes recently to remove stuff that was never settled (the LPI timer questions, whether it should be validated, clamped, should phylink provide a software timer if the LPI timer is out of range, etc.) That's more proof that text in cover messages is utterly useless!
Ok, for your information, I had read the cover letter in its entirety, including the paragraph which you are highlighting here again.
I still find it of limited usefulness for me to see and not be able to leave meaningful feedback on a DSA conversion broken down into sets which offer an incomplete image, especially when the API itself is still subject to change. I wanted to leave an Acked-by but I didn't fully understand where the conversion is going, so I asked the question instead. I wonder what you would have done in my position.
I wish you a beautiful evening.