| Message ID | E1q7Y9R-00DI8g-GF@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | phylink EEE support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 98 this patch: 98 |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/build_clang | success | Errors and warnings before: 13 this patch: 13 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 98 this patch: 98 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 215 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Fri, Jun 09, 2023 at 10:11:21AM +0100, Russell King (Oracle) wrote:
> Add EEE management to phylink.
Hi Russell
I've been working on my EEE patches. I have all pure phylib drivers
converted. I've incorporated these four patches as well, and make use
of the first patch in phylib.
Looking at this patch, i don't see a way for the MAC to indicate it
actually does support EEE. Am i missing it?
What i proposed last time is to add another bit to
pl->config->mac_capabilities. Does that work for you?
Do you have time to respin these patches to address the comments? It
is probably easier for my series to wait until these are merged.
Andrew
On Sun, Jun 11, 2023 at 11:28:32PM +0200, Andrew Lunn wrote: > On Fri, Jun 09, 2023 at 10:11:21AM +0100, Russell King (Oracle) wrote: > > Add EEE management to phylink. > > Hi Russell > > I've been working on my EEE patches. I have all pure phylib drivers > converted. I've incorporated these four patches as well, and make use > of the first patch in phylib. > > Looking at this patch, i don't see a way for the MAC to indicate it > actually does support EEE. Am i missing it? If a MAC doesn't support EEE, it won't have the necessary calls to phylink_*_eee() in its ethtool ops. As can be seen from the mvpp2 patch, mvpp2_ethtool_get_eee() and mvpp2_ethtool_set_eee() are needed to call the phylink methods. Given that a MAC has to provide those hooks, why would we need a capability for EEE? Are you thinking that it may be optional for some MACs? Thinking of the future (not having done a lot of research though) it may be appropriate to have a bitmap of... I was going to say ethtool modes but that doesn't really work... phy interface modes that the MAC can support EEE. I'm thinking of devices such as mvpp2 where EEE is supported by the GMAC (for <=2.5G) but not XLG (for >= 5G). If we use phy interface modes, we somehow need to turn that into ethtool link modes for the media side, which is e.g. PHY dependent. For example, the Aquantia PHYs doing rate adaption to 10G plugged into mvpp2 (which probably doesn't work too well due to the lack of pause support in mvpp2 hardware) won't be able to do EEE at any speed because it'll be only using the XLG, but a PHY that uses SGMII connected to mvpp2 can because that will use GMAC.
On Sun, Jun 11, 2023 at 10:37:55PM +0100, Russell King (Oracle) wrote: > On Sun, Jun 11, 2023 at 11:28:32PM +0200, Andrew Lunn wrote: > > On Fri, Jun 09, 2023 at 10:11:21AM +0100, Russell King (Oracle) wrote: > > > Add EEE management to phylink. > > > > Hi Russell > > > > I've been working on my EEE patches. I have all pure phylib drivers > > converted. I've incorporated these four patches as well, and make use > > of the first patch in phylib. > > > > Looking at this patch, i don't see a way for the MAC to indicate it > > actually does support EEE. Am i missing it? > > If a MAC doesn't support EEE, it won't have the necessary calls to > phylink_*_eee() in its ethtool ops. As can be seen from the mvpp2 > patch, mvpp2_ethtool_get_eee() and mvpp2_ethtool_set_eee() are > needed to call the phylink methods. This is a bit messy for stmmac. Some versions of stmmac have EEE support, some don't. Same is true for r8169, but that is a phylib driver. I expect there are other examples. We also have the same problem with DSA. There is currently one phylink_mac_ops structure which has generic methods for all the callbacks which then call into the switch driver if the switch driver implements the call. And at least with the mv88e6xxx driver, when we get around to adding EEE support, some switches will support it, some won't, so will we need two different switch op structures? Adding to the mac_capabilities just seems easier. Andrew
On Mon, Jun 12, 2023 at 12:25:29AM +0200, Andrew Lunn wrote: > On Sun, Jun 11, 2023 at 10:37:55PM +0100, Russell King (Oracle) wrote: > > On Sun, Jun 11, 2023 at 11:28:32PM +0200, Andrew Lunn wrote: > > > On Fri, Jun 09, 2023 at 10:11:21AM +0100, Russell King (Oracle) wrote: > > > > Add EEE management to phylink. > > > > > > Hi Russell > > > > > > I've been working on my EEE patches. I have all pure phylib drivers > > > converted. I've incorporated these four patches as well, and make use > > > of the first patch in phylib. > > > > > > Looking at this patch, i don't see a way for the MAC to indicate it > > > actually does support EEE. Am i missing it? > > > > If a MAC doesn't support EEE, it won't have the necessary calls to > > phylink_*_eee() in its ethtool ops. As can be seen from the mvpp2 > > patch, mvpp2_ethtool_get_eee() and mvpp2_ethtool_set_eee() are > > needed to call the phylink methods. > > This is a bit messy for stmmac. Some versions of stmmac have EEE > support, some don't. Same is true for r8169, but that is a phylib > driver. I expect there are other examples. I see what you mean, but I think really what's going on there is whether the MAC supports LPI or not coupled with whether the PHY has any "smartEEE" feature meaning overall the system doesn't support any kind of EEE. The reason I pick that level of detail is when one comes to a setup like i.MX6 FEC coupled with an AR8035 with it's SmartEEE, the system as a whole supports EEE but the MAC doesn't. So, while the FEC would have a MAC_CAP_EEE flag clear, we don't actually want to disable EEE support with that setup. > We also have the same problem with DSA. There is currently one > phylink_mac_ops structure which has generic methods for all the > callbacks which then call into the switch driver if the switch driver > implements the call. And at least with the mv88e6xxx driver, when we > get around to adding EEE support, some switches will support it, some > won't, so will we need two different switch op structures? I'd rather not end up with multiple mac_ops. > Adding to the mac_capabilities just seems easier. It does, but I suspect MAC_CAP_EEE is not sufficient because: a) the issue of PHYs with SmartEEE like features such as AR803x. b) an interface which has multiple MACs that it switches between depending on speed may have differing levels of LPI support, so a single property doesn't seem to be suitable. c) rate adapting PHYs that may connect only with e.g. a 10G MAC that doesn't support LPI, but the MAC setup does have a 1G MAC that does. I'm wondering if, rather than adding a bit to mac_capabilities, whether instead: 1) add eee_capabilities and re-use the existing MAC_CAP_* definitions to indicate what speeds the MAC supports LPI. This doesn't seem to solve (c). 2) add a phy interface bitmap indicating which interface modes support LPI generation. Phylib already has similar with its supported_eee link mode bitmap, which presumably MACs can knock out link modes that they know they wouldn't support. Hmm.
> I'm wondering if, rather than adding a bit to mac_capabilities, whether > instead: > > 1) add eee_capabilities and re-use the existing MAC_CAP_* definitions > to indicate what speeds the MAC supports LPI. This doesn't seem to > solve (c). > 2) add a phy interface bitmap indicating which interface modes support > LPI generation. > > Phylib already has similar with its supported_eee link mode bitmap, > which presumably MACs can knock out link modes that they know they > wouldn't support. O.K, I can probably make that work. None of the MAC drivers i've looked at need this flexibility yet, but we can add it now. I do however wounder if it should be called lpi_capabilities, not eee_capabilities. These patches are all about making the core deal with 99% of EEE. All the MAC driver needs to do is enable/disable sending LPI and set the timer value. So we are really talking about the MACs LPI capabilities. Andrew
On Tue, Jun 13, 2023 at 02:26:22PM +0200, Andrew Lunn wrote: > > I'm wondering if, rather than adding a bit to mac_capabilities, whether > > instead: > > > > 1) add eee_capabilities and re-use the existing MAC_CAP_* definitions > > to indicate what speeds the MAC supports LPI. This doesn't seem to > > solve (c). > > 2) add a phy interface bitmap indicating which interface modes support > > LPI generation. > > > > Phylib already has similar with its supported_eee link mode bitmap, > > which presumably MACs can knock out link modes that they know they > > wouldn't support. > > O.K, I can probably make that work. None of the MAC drivers i've > looked at need this flexibility yet, but we can add it now. > > I do however wounder if it should be called lpi_capabilities, not > eee_capabilities. These patches are all about making the core deal > with 99% of EEE. All the MAC driver needs to do is enable/disable > sending LPI and set the timer value. So we are really talking about > the MACs LPI capabilities. No problem with calling it lpi_capabilities. Thanks!
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 1ae7868d2137..d0abae91ceb5 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -80,6 +80,9 @@ struct phylink { DECLARE_PHY_INTERFACE_MASK(sfp_interfaces); __ETHTOOL_DECLARE_LINK_MODE_MASK(sfp_support); u8 sfp_port; + + struct eee_config eee_cfg; + bool eee_active; }; #define phylink_printk(level, pl, fmt, ...) \ @@ -1253,6 +1256,24 @@ static const char *phylink_pause_to_str(int pause) } } +static void phylink_disable_tx_lpi(struct phylink *pl) +{ + if (pl->mac_ops->mac_disable_tx_lpi) + pl->mac_ops->mac_disable_tx_lpi(pl->config); +} + +static void phylink_enable_tx_lpi(struct phylink *pl) +{ + if (pl->mac_ops->mac_enable_tx_lpi) + pl->mac_ops->mac_enable_tx_lpi(pl->config, + pl->eee_cfg.tx_lpi_timer); +} + +static bool phylink_eee_is_active(struct phylink *pl) +{ + return phylink_init_eee(pl, pl->config->eee_clk_stop_enable) >= 0; +} + static void phylink_link_up(struct phylink *pl, struct phylink_link_state link_state) { @@ -1294,6 +1315,12 @@ static void phylink_link_up(struct phylink *pl, pl->cur_interface, speed, duplex, !!(link_state.pause & MLO_PAUSE_TX), rx_pause); + if (eeecfg_mac_can_tx_lpi(&pl->eee_cfg)) { + pl->eee_active = phylink_eee_is_active(pl); + if (pl->eee_active) + phylink_enable_tx_lpi(pl); + } + if (ndev) netif_carrier_on(ndev); @@ -1310,25 +1337,32 @@ static void phylink_link_down(struct phylink *pl) if (ndev) netif_carrier_off(ndev); + + if (eeecfg_mac_can_tx_lpi(&pl->eee_cfg) && pl->eee_active) { + pl->eee_active = false; + phylink_disable_tx_lpi(pl); + } + pl->mac_ops->mac_link_down(pl->config, pl->cur_link_an_mode, pl->cur_interface); 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->mac_link_dropped = false; @@ -1571,6 +1605,9 @@ struct phylink *phylink_create(struct phylink_config *config, linkmode_copy(pl->link_config.advertising, pl->supported); phylink_validate(pl, pl->supported, &pl->link_config); + /* Set the default EEE configuration */ + pl->eee_cfg = pl->config->eee; + ret = phylink_parse_mode(pl, fwnode); if (ret < 0) { kfree(pl); @@ -2589,6 +2626,12 @@ int phylink_ethtool_get_eee(struct phylink *pl, struct ethtool_eee *eee) if (pl->phydev) ret = phy_ethtool_get_eee(pl->phydev, eee); + if (!ret) { + /* Overwrite phylib's interpretation of configuration */ + eeecfg_to_eee(&pl->eee_cfg, eee); + eee->eee_active = pl->eee_active; + } + return ret; } EXPORT_SYMBOL_GPL(phylink_ethtool_get_eee); @@ -2607,6 +2650,35 @@ int phylink_ethtool_set_eee(struct phylink *pl, struct ethtool_eee *eee) if (pl->phydev) ret = phy_ethtool_set_eee(pl->phydev, eee); + if (!ret) { + bool can_lpi, old_can_lpi; + + mutex_lock(&pl->state_mutex); + old_can_lpi = eeecfg_mac_can_tx_lpi(&pl->eee_cfg); + eee_to_eeecfg(eee, &pl->eee_cfg); + can_lpi = eeecfg_mac_can_tx_lpi(&pl->eee_cfg); + + /* IF the link is up, and the configuration changes the + * LPI permissive state, deal with the change at the MAC. + */ + if (phylink_link_is_up(pl) && old_can_lpi != can_lpi) { + if (can_lpi) { + /* If LPI wasn't enabled, eee_active (result + * of any AN) will be false. Update it here. + * If the advertisement has been changed, the + * link will cycle which will update this. + */ + pl->eee_active = phylink_eee_is_active(pl); + if (pl->eee_active) + phylink_enable_tx_lpi(pl); + } else { + phylink_disable_tx_lpi(pl); + } + } + + mutex_unlock(&pl->state_mutex); + } + return ret; } EXPORT_SYMBOL_GPL(phylink_ethtool_set_eee); diff --git a/include/linux/phylink.h b/include/linux/phylink.h index 0cf07d7d11b8..6328a9f481ee 100644 --- a/include/linux/phylink.h +++ b/include/linux/phylink.h @@ -5,6 +5,8 @@ #include <linux/spinlock.h> #include <linux/workqueue.h> +#include <net/eee.h> + struct device_node; struct ethtool_cmd; struct fwnode_handle; @@ -122,11 +124,13 @@ enum phylink_op_type { * if MAC link is at %MLO_AN_FIXED mode. * @mac_managed_pm: if true, indicate the MAC driver is responsible for PHY PM. * @ovr_an_inband: if true, override PCS to MLO_AN_INBAND + * @eee_clk_stop_enable: if true, PHY can stop the receive clock during LPI * @get_fixed_state: callback to execute to determine the fixed link state, * if MAC link is at %MLO_AN_FIXED mode. * @supported_interfaces: bitmap describing which PHY_INTERFACE_MODE_xxx * are supported by the MAC/PCS. * @mac_capabilities: MAC pause/speed/duplex capabilities. + * @eee: default EEE configuration. */ struct phylink_config { struct device *dev; @@ -135,10 +139,12 @@ struct phylink_config { bool poll_fixed_state; bool mac_managed_pm; bool ovr_an_inband; + bool eee_clk_stop_enable; void (*get_fixed_state)(struct phylink_config *config, struct phylink_link_state *state); DECLARE_PHY_INTERFACE_MASK(supported_interfaces); unsigned long mac_capabilities; + struct eee_config eee; }; /** @@ -152,6 +158,8 @@ struct phylink_config { * @mac_an_restart: restart 802.3z BaseX autonegotiation. * @mac_link_down: take the link down. * @mac_link_up: allow the link to come up. + * @mac_disable_tx_lpi: disable LPI. + * @mac_enable_tx_lpi: enable and configure LPI. * * The individual methods are described more fully below. */ @@ -176,6 +184,8 @@ struct phylink_mac_ops { struct phy_device *phy, unsigned int mode, phy_interface_t interface, int speed, int duplex, bool tx_pause, bool rx_pause); + void (*mac_disable_tx_lpi)(struct phylink_config *config); + void (*mac_enable_tx_lpi)(struct phylink_config *config, u32 timer); }; #if 0 /* For kernel-doc purposes only. */ @@ -429,6 +439,28 @@ void mac_link_down(struct phylink_config *config, unsigned int mode, void mac_link_up(struct phylink_config *config, struct phy_device *phy, unsigned int mode, phy_interface_t interface, int speed, int duplex, bool tx_pause, bool rx_pause); + +/** + * mac_disable_tx_lpi() - disable LPI generation at the MAC + * @config: a pointer to a &struct phylink_config. + * + * Disable generation of LPI at the MAC, effectively preventing the MAC + * from indicating that it is idle. + */ +void mac_disable_tx_lpi(struct phylink_config *config); + +/** + * mac_enable_tx_lpi() - configure and enable LPI generation at the MAC + * @config: a pointer to a &struct phylink_config. + * @timer: LPI timeout in microseconds. + * + * Configure the LPI timeout accordingly. This will only be called when + * the link is already up, to cater for situations where the hardware + * needs to be programmed according to the link speed. + * + * Enable LPI generation at the MAC. + */ +void mac_enable_tx_lpi(struct phylink_config *config, u32 timer); #endif struct phylink_pcs_ops;
Add EEE management to phylink. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/phy/phylink.c | 82 ++++++++++++++++++++++++++++++++++++--- include/linux/phylink.h | 32 +++++++++++++++ 2 files changed, 109 insertions(+), 5 deletions(-)