1. Patchew
  2. linux
  3. [PATCH net-next v7 0/8] net: ethernet: Rework EEE
  4. View patch

[PATCH net-next v8 3/8] net: phy: Add helper to set EEE Clock stop enable bit

Oleksij Rempel posted 8 patches 1 year, 2 months ago
There is a newer version of this series
[PATCH net-next v8 3/8] net: phy: Add helper to set EEE Clock stop enable bit
Posted by Oleksij Rempel 1 year, 2 months ago 
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);
+
 /**
  * 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);
-- 
2.39.2
Re: [PATCH net-next v8 3/8] net: phy: Add helper to set EEE Clock stop enable bit
Posted by Heiner Kallweit 1 year, 2 months ago 
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);
Re: [PATCH net-next v8 3/8] net: phy: Add helper to set EEE Clock stop enable bit
Posted by Russell King (Oracle) 1 year, 2 months ago 
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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH net-next v8 3/8] net: phy: Add helper to set EEE Clock stop enable bit
Posted by Andrew Lunn 1 year, 2 months ago 
> 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
Re: [PATCH net-next v8 3/8] net: phy: Add helper to set EEE Clock stop enable bit
Posted by Oleksij Rempel 1 year, 2 months ago 
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
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.