| Message ID | E1tKefY-006SMd-Af@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: add phylink managed EEE support | expand |
On Mon, Dec 09, 2024 at 02:23:28PM +0000, Russell King (Oracle) wrote: > Add a function to allow configuration of the PCS's clock stop enable > bit, used to configure whether the xMII receive clock can be stopped > during LPI mode. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
> @@ -2073,6 +2073,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_eee_tx_clock_stop_capable(struct phy_device *phydev); > +int phy_eee_rx_clock_stop(struct phy_device *phydev, bool clk_stop_enable); Hi Russell Do you have patches to MAC drivers using phylib, not phylink, using these two new calls? We see phylib users get EEE wrong. I'm worried phylib users are going to try to use these new API methods and make an even bigger mess. If we think these should only be used by phylink, maybe they should be put into a header in drivers/net/phy to stop MAC drivers using them? Andrew
On Tue, Dec 10, 2024 at 04:11:09AM +0100, Andrew Lunn wrote: > > @@ -2073,6 +2073,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_eee_tx_clock_stop_capable(struct phy_device *phydev); > > +int phy_eee_rx_clock_stop(struct phy_device *phydev, bool clk_stop_enable); > > Hi Russell > > Do you have patches to MAC drivers using phylib, not phylink, using > these two new calls? > > We see phylib users get EEE wrong. I'm worried phylib users are going > to try to use these new API methods and make an even bigger mess. If > we think these should only be used by phylink, maybe they should be > put into a header in drivers/net/phy to stop MAC drivers using them? If we want to fix the current phylib-using drivers, then we do need at minimum phy_eee_rx_clock_stop() to do that - we have drivers that call phy_init_eee(..., true) which would need to call this. It's quite rare that drivers allow the PHY to stop the clock. There may be several reasons for this: 1) the MAC doesn't support EEE on the MII link type(s) that have a clock. (e.g. supporting EEE on SGMII but not RGMII.) 2) the documentation for the MAC doesn't comment on this aspect (so the safest thing is to always keep the clock running.) 3) the driver developer hasn't understood the feature and the safest approach is to pass phy_init_eee() with a value of zero/false which leaves the setting unchanged.
On Tue, Dec 10, 2024 at 09:51:54AM +0000, Russell King (Oracle) wrote: > On Tue, Dec 10, 2024 at 04:11:09AM +0100, Andrew Lunn wrote: > > > @@ -2073,6 +2073,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_eee_tx_clock_stop_capable(struct phy_device *phydev); > > > +int phy_eee_rx_clock_stop(struct phy_device *phydev, bool clk_stop_enable); > > > > Hi Russell > > > > Do you have patches to MAC drivers using phylib, not phylink, using > > these two new calls? > > > > We see phylib users get EEE wrong. I'm worried phylib users are going > > to try to use these new API methods and make an even bigger mess. If > > we think these should only be used by phylink, maybe they should be > > put into a header in drivers/net/phy to stop MAC drivers using them? > > If we want to fix the current phylib-using drivers, then we do need > at minimum phy_eee_rx_clock_stop() to do that - we have drivers that > call phy_init_eee(..., true) which would need to call this. phy_init_eee() needs to die, so we need this one. But we should consider the other one, do we want to make it private? Andrew
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 8fb4224c7576..caf472b5d4e5 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -1660,6 +1660,27 @@ int phy_eee_tx_clock_stop_capable(struct phy_device *phydev) } 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 + * @clk_stop_enable: flag to indicate whether the clock can be stopped + * + * Configure whether the PHY can disable its receive clock during LPI mode, + * See IEEE 802.3 sections 22.2.2.2, 35.2.2.10, and 45.2.3.1.4. + * + * Returns: 0 or negative error. + */ +int phy_eee_rx_clock_stop(struct phy_device *phydev, bool clk_stop_enable) +{ + /* Configure the PHY to stop receiving xMII + * clock while it is signaling LPI. + */ + return phy_modify_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1, + MDIO_PCS_CTRL1_CLKSTOP_EN, + clk_stop_enable ? MDIO_PCS_CTRL1_CLKSTOP_EN : 0); +} +EXPORT_SYMBOL_GPL(phy_eee_rx_clock_stop); + /** * phy_init_eee - init and check the EEE feature * @phydev: target phy_device struct @@ -1684,11 +1705,7 @@ int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable) return -EPROTONOSUPPORT; if (clk_stop_enable) - /* Configure the PHY to stop receiving xMII - * clock while it is signaling LPI. - */ - ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1, - MDIO_PCS_CTRL1_CLKSTOP_EN); + ret = phy_eee_rx_clock_stop(phydev, true); return ret < 0 ? ret : 0; } diff --git a/include/linux/phy.h b/include/linux/phy.h index 267de08c227c..f865a8fc56c3 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -2073,6 +2073,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_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); int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_keee *data);
Add a function to allow configuration of the PCS's clock stop enable bit, used to configure whether the xMII receive clock can be stopped during LPI mode. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/phy/phy.c | 27 ++++++++++++++++++++++----- include/linux/phy.h | 1 + 2 files changed, 23 insertions(+), 5 deletions(-)