| Message ID | 20240301100153.927743-4-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ethernet: Rework EEE | expand |
On 01.03.2024 11:01, Oleksij Rempel wrote: > From: Andrew Lunn <andrew@lunn.ch> > > 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: Florian Fainelli <florian.fainelli@broadcom.com> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > 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 <andrew@lunn.ch> > > > > 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: Florian Fainelli <florian.fainelli@broadcom.com> > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > > --- > > 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.
> 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
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);