| Message ID | 20230327170201.2036708-4-andrew@lunn.ch (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ethernet: Rework EEE | expand |
On Mon, Mar 27, 2023 at 07:01:41PM +0200, Andrew Lunn wrote: > 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 <andrew@lunn.ch> Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Thanks!
On Mon, Mar 27, 2023 at 07:01:41PM +0200, Andrew Lunn wrote: > 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 <andrew@lunn.ch> > --- > v2: Add missing EXPORT_SYMBOL_GPL > --- > drivers/net/phy/phy.c | 20 ++++++++++++++++++++ > include/linux/phy.h | 1 + > 2 files changed, 21 insertions(+) > > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > index 68e1ce942dd6..d3d6ff4ed488 100644 > --- a/drivers/net/phy/phy.c > +++ b/drivers/net/phy/phy.c > @@ -1503,6 +1503,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) this function should go to drivers/net/phy/phy-c45.c and renamed to genphy_c45_eee_clk_stop_enable() > +{ > + int ret; > + > + mutex_lock(&phydev->lock); /* IEEE 802.3-2018 45.2.3.1.4 Clock stop enable (3.0.10) */ > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1, > + MDIO_PCS_CTRL1_CLKSTOP_EN); It would be better to write it conditionally. Only if EEE is supported and only if this bit is supported as well. Support is indicated by the IEEE 802.3:2018 - 45.2.3.2.6 Clock stop capable (3.1.6) It looks like there are other registers for same functionality too but other types of PHYs: 45.2.4.1.4 Clock stop enable (4.0.10) 45.2.4.2.6 Clock stop capable (4.1.6) 45.2.5.1.4 Clock stop enable (5.0.10) 45.2.5.2.6 Clock stop capable (5.1.6) If I see it correctly, Clock-stop is possible only for GMII/RGMII. Integrated PHYs or EEE capable PHYs with RMII do not support it. For example KSZ8091RNA with RMII: https://ww1.microchip.com/downloads/en/DeviceDoc/KSZ8091RNA-RND-10BASE-T-100BASE-TX-PHY-with-RMII-and-EEE-Support-DS00002298B.pdf KSZ9477 switch with integrated PHYs: https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/KSZ9477S-Data-Sheet-DS00002392C.pdf > + 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 2508f1d99777..12addd1c29f2 100644 > --- a/include/linux/phy.h > +++ b/include/linux/phy.h > @@ -1846,6 +1846,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_eee *data); > int phy_ethtool_get_eee(struct phy_device *phydev, struct ethtool_eee *data); > -- > 2.39.2 > >
On Tue, Mar 28, 2023 at 07:03:27AM +0200, Oleksij Rempel wrote: > On Mon, Mar 27, 2023 at 07:01:41PM +0200, Andrew Lunn wrote: > > 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 <andrew@lunn.ch> > > --- > > v2: Add missing EXPORT_SYMBOL_GPL > > --- > > drivers/net/phy/phy.c | 20 ++++++++++++++++++++ > > include/linux/phy.h | 1 + > > 2 files changed, 21 insertions(+) > > > > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > > index 68e1ce942dd6..d3d6ff4ed488 100644 > > --- a/drivers/net/phy/phy.c > > +++ b/drivers/net/phy/phy.c > > @@ -1503,6 +1503,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) > > this function should go to drivers/net/phy/phy-c45.c > and renamed to genphy_c45_eee_clk_stop_enable() > > +{ > > + int ret; > > + > > + mutex_lock(&phydev->lock); > > /* IEEE 802.3-2018 45.2.3.1.4 Clock stop enable (3.0.10) */ > > > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1, > > + MDIO_PCS_CTRL1_CLKSTOP_EN); > > It would be better to write it conditionally. Only if EEE is supported > and only if this bit is supported as well. Support is indicated by the > IEEE 802.3:2018 - 45.2.3.2.6 Clock stop capable (3.1.6) > > It looks like there are other registers for same functionality too but > other types of PHYs: > 45.2.4.1.4 Clock stop enable (4.0.10) > 45.2.4.2.6 Clock stop capable (4.1.6) > 45.2.5.1.4 Clock stop enable (5.0.10) > 45.2.5.2.6 Clock stop capable (5.1.6) > > If I see it correctly, Clock-stop is possible only for GMII/RGMII. > Integrated PHYs or EEE capable PHYs with RMII do not support it. > For example KSZ8091RNA with RMII: > https://ww1.microchip.com/downloads/en/DeviceDoc/KSZ8091RNA-RND-10BASE-T-100BASE-TX-PHY-with-RMII-and-EEE-Support-DS00002298B.pdf > KSZ9477 switch with integrated PHYs: > https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/KSZ9477S-Data-Sheet-DS00002392C.pdf One more PHY, with RGMII support, but without clock-stop, which is probably indicated by 3.1.6 bit. https://ww1.microchip.com/downloads/aemDocuments/documents/AERO/ProductDocuments/DataSheets/VSC8540ET_Extended_Temperature_Single_Port_Fast_Ethernet_Copper_PHY_DS60001648.pdf
> > + */ > > +int phy_eee_clk_stop_enable(struct phy_device *phydev) > > this function should go to drivers/net/phy/phy-c45.c > and renamed to genphy_c45_eee_clk_stop_enable() > > +{ > > + int ret; > > + > > + mutex_lock(&phydev->lock); > > /* IEEE 802.3-2018 45.2.3.1.4 Clock stop enable (3.0.10) */ > > > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1, > > + MDIO_PCS_CTRL1_CLKSTOP_EN); > > It would be better to write it conditionally. Only if EEE is supported > and only if this bit is supported as well. Support is indicated by the > IEEE 802.3:2018 - 45.2.3.2.6 Clock stop capable (3.1.6) O.K, i was too lazy. I just took the existing code in phy_eee_init() and moved it here. I can rework it as requested. > It looks like there are other registers for same functionality too but > other types of PHYs: > 45.2.4.1.4 Clock stop enable (4.0.10) > 45.2.4.2.6 Clock stop capable (4.1.6) > 45.2.5.1.4 Clock stop enable (5.0.10) > 45.2.5.2.6 Clock stop capable (5.1.6) > > If I see it correctly, Clock-stop is possible only for GMII/RGMII. > Integrated PHYs or EEE capable PHYs with RMII do not support it. For the existing code, it is up to the MAC to decide if the clock should be stopped. It is not clear why, but we do see some MACs where the DMA engine stops working when the clock is stopped. So i don't want to be overly eager to stop clocks and introduce regressions. So i will keep with just one clock above. But make it conditional on the capability. Andrew
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 68e1ce942dd6..d3d6ff4ed488 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -1503,6 +1503,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 2508f1d99777..12addd1c29f2 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -1846,6 +1846,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_eee *data); int phy_ethtool_get_eee(struct phy_device *phydev, struct ethtool_eee *data);
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 <andrew@lunn.ch> --- v2: Add missing EXPORT_SYMBOL_GPL --- drivers/net/phy/phy.c | 20 ++++++++++++++++++++ include/linux/phy.h | 1 + 2 files changed, 21 insertions(+)