[PATCH net-next 0/7] net: phy: smsc: add support for edpd tunable

[PATCH net-next 0/7] net: phy: smsc: add support for edpd tunable

[Heiner Kallweit]

This adds support for the EDPD PHY tunable.
Per default EDPD is disabled in interrupt mode, the tunable can be used
to override this, e.g. if the link partner doesn't use EDPD.
The interval to check for energy can be chosen between 1000ms and
2000ms. Note that this value consists of the 1000ms phylib interval
for state machine runs plus the time to wait for energy being detected.

Heiner Kallweit (7):
  net: phy: smsc: rename flag energy_enable
  net: phy: smsc: add helper smsc_phy_config_edpd
  net: phy: smsc: clear edpd_enable if interrupt mode is used
  net: phy: smsc: add flag edpd_mode_set_by_user
  net: phy: smss: prepare for making edpd wait period configurable
  net: phy: smsc: add edpd tunable support
  net: phy: smsc: enable edpd tunable support

 drivers/net/phy/smsc.c  | 138 ++++++++++++++++++++++++++++++++++++----
 include/linux/smscphy.h |   4 ++
 2 files changed, 131 insertions(+), 11 deletions(-)

-- 
2.40.0

[PATCH net-next 1/7] net: phy: smsc: rename flag energy_enable

[Heiner Kallweit]

Rename the flag to edpd_enable, as we're not enabling energy but
edpd (energy detect power down) mode. In addition change the
type to a bit field member in preparation of adding further flags.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/smsc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index 730964b85..928cf6d8b 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -44,7 +44,7 @@ static struct smsc_hw_stat smsc_hw_stats[] = {
 };
 
 struct smsc_phy_priv {
-	bool energy_enable;
+	unsigned int edpd_enable:1;
 };
 
 static int smsc_phy_ack_interrupt(struct phy_device *phydev)
@@ -102,7 +102,7 @@ int smsc_phy_config_init(struct phy_device *phydev)
 {
 	struct smsc_phy_priv *priv = phydev->priv;
 
-	if (!priv || !priv->energy_enable || phydev->irq != PHY_POLL)
+	if (!priv || !priv->edpd_enable || phydev->irq != PHY_POLL)
 		return 0;
 
 	/* Enable energy detect power down mode */
@@ -198,7 +198,7 @@ int lan87xx_read_status(struct phy_device *phydev)
 	if (err)
 		return err;
 
-	if (!phydev->link && priv && priv->energy_enable &&
+	if (!phydev->link && priv && priv->edpd_enable &&
 	    phydev->irq == PHY_POLL) {
 		/* Disable EDPD to wake up PHY */
 		int rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
@@ -284,10 +284,10 @@ int smsc_phy_probe(struct phy_device *phydev)
 	if (!priv)
 		return -ENOMEM;
 
-	priv->energy_enable = true;
+	priv->edpd_enable = true;
 
 	if (device_property_present(dev, "smsc,disable-energy-detect"))
-		priv->energy_enable = false;
+		priv->edpd_enable = false;
 
 	phydev->priv = priv;
 
-- 
2.40.0

[PATCH net-next 2/7] net: phy: smsc: add helper smsc_phy_config_edpd

[Heiner Kallweit]

Add helper smsc_phy_config_edpd() and explicitly clear bit
MII_LAN83C185_EDPWRDOWN is edpd_enable isn't set.
Boot loader may have left whatever value.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/smsc.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index 928cf6d8b..1b588366e 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -77,6 +77,18 @@ int smsc_phy_config_intr(struct phy_device *phydev)
 }
 EXPORT_SYMBOL_GPL(smsc_phy_config_intr);
 
+static int smsc_phy_config_edpd(struct phy_device *phydev)
+{
+	struct smsc_phy_priv *priv = phydev->priv;
+
+	if (priv->edpd_enable)
+		return phy_set_bits(phydev, MII_LAN83C185_CTRL_STATUS,
+				    MII_LAN83C185_EDPWRDOWN);
+	else
+		return phy_clear_bits(phydev, MII_LAN83C185_CTRL_STATUS,
+				      MII_LAN83C185_EDPWRDOWN);
+}
+
 irqreturn_t smsc_phy_handle_interrupt(struct phy_device *phydev)
 {
 	int irq_status;
@@ -105,9 +117,7 @@ int smsc_phy_config_init(struct phy_device *phydev)
 	if (!priv || !priv->edpd_enable || phydev->irq != PHY_POLL)
 		return 0;
 
-	/* Enable energy detect power down mode */
-	return phy_set_bits(phydev, MII_LAN83C185_CTRL_STATUS,
-			    MII_LAN83C185_EDPWRDOWN);
+	return smsc_phy_config_edpd(phydev);
 }
 EXPORT_SYMBOL_GPL(smsc_phy_config_init);
 
-- 
2.40.0

[PATCH net-next 3/7] net: phy: smsc: clear edpd_enable if interrupt mode is used

[Heiner Kallweit]

Clear edpd_enable if interrupt mode is used, this avoids
having to check for PHY_POLL multiple times.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/smsc.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index 1b588366e..f5ecd8bea 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -114,9 +114,12 @@ int smsc_phy_config_init(struct phy_device *phydev)
 {
 	struct smsc_phy_priv *priv = phydev->priv;
 
-	if (!priv || !priv->edpd_enable || phydev->irq != PHY_POLL)
+	if (!priv)
 		return 0;
 
+	if (phydev->irq != PHY_POLL)
+		priv->edpd_enable = false;
+
 	return smsc_phy_config_edpd(phydev);
 }
 EXPORT_SYMBOL_GPL(smsc_phy_config_init);
@@ -208,8 +211,7 @@ int lan87xx_read_status(struct phy_device *phydev)
 	if (err)
 		return err;
 
-	if (!phydev->link && priv && priv->edpd_enable &&
-	    phydev->irq == PHY_POLL) {
+	if (!phydev->link && priv && priv->edpd_enable) {
 		/* Disable EDPD to wake up PHY */
 		int rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
 		if (rc < 0)
-- 
2.40.0

[PATCH net-next 4/7] net: phy: smsc: add flag edpd_mode_set_by_user

[Heiner Kallweit]

Add flag edpd_mode_set_by_user in preparation of adding edpd phy tunable
support. This flag will allow users to override the default behavior
of edpd being disabled if interrupt mode is used.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/smsc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index f5ecd8bea..25b9cd474 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -45,6 +45,7 @@ static struct smsc_hw_stat smsc_hw_stats[] = {
 
 struct smsc_phy_priv {
 	unsigned int edpd_enable:1;
+	unsigned int edpd_mode_set_by_user:1;
 };
 
 static int smsc_phy_ack_interrupt(struct phy_device *phydev)
@@ -117,7 +118,8 @@ int smsc_phy_config_init(struct phy_device *phydev)
 	if (!priv)
 		return 0;
 
-	if (phydev->irq != PHY_POLL)
+	/* don't use EDPD in irq mode except overridden by user */
+	if (!priv->edpd_mode_set_by_user && phydev->irq != PHY_POLL)
 		priv->edpd_enable = false;
 
 	return smsc_phy_config_edpd(phydev);
-- 
2.40.0

[PATCH net-next 5/7] net: phy: smss: prepare for making edpd wait period configurable

[Heiner Kallweit]

Add a member edpd_max_wait_ms to the private data structure in preparation
of making the wait period configurable by supporting the edpd phy tunable.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/smsc.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index 25b9cd474..0cd433f01 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -33,6 +33,8 @@
 #define SPECIAL_CTRL_STS_AMDIX_ENABLE_	0x4000
 #define SPECIAL_CTRL_STS_AMDIX_STATE_	0x2000
 
+#define EDPD_MAX_WAIT_DFLT		640
+
 struct smsc_hw_stat {
 	const char *string;
 	u8 reg;
@@ -46,6 +48,7 @@ static struct smsc_hw_stat smsc_hw_stats[] = {
 struct smsc_phy_priv {
 	unsigned int edpd_enable:1;
 	unsigned int edpd_mode_set_by_user:1;
+	unsigned int edpd_max_wait_ms;
 };
 
 static int smsc_phy_ack_interrupt(struct phy_device *phydev)
@@ -213,9 +216,13 @@ int lan87xx_read_status(struct phy_device *phydev)
 	if (err)
 		return err;
 
-	if (!phydev->link && priv && priv->edpd_enable) {
+	if (!phydev->link && priv && priv->edpd_enable &&
+	    priv->edpd_max_wait_ms) {
+		unsigned int max_wait = priv->edpd_max_wait_ms * 1000;
+		int rc;
+
 		/* Disable EDPD to wake up PHY */
-		int rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
+		rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
 		if (rc < 0)
 			return rc;
 
@@ -229,7 +236,7 @@ int lan87xx_read_status(struct phy_device *phydev)
 		 */
 		read_poll_timeout(phy_read, rc,
 				  rc & MII_LAN83C185_ENERGYON || rc < 0,
-				  10000, 640000, true, phydev,
+				  10000, max_wait, true, phydev,
 				  MII_LAN83C185_CTRL_STATUS);
 		if (rc < 0)
 			return rc;
@@ -299,6 +306,7 @@ int smsc_phy_probe(struct phy_device *phydev)
 		return -ENOMEM;
 
 	priv->edpd_enable = true;
+	priv->edpd_max_wait_ms = EDPD_MAX_WAIT_DFLT;
 
 	if (device_property_present(dev, "smsc,disable-energy-detect"))
 		priv->edpd_enable = false;
-- 
2.40.0

[PATCH net-next 6/7] net: phy: smsc: add edpd tunable support

[Heiner Kallweit]

This adds support for the EDPD PHY tunable.
Per default EDPD is disabled in interrupt mode, the tunable can be used
to override this, e.g. if the link partner doesn't use EDPD.
The interval to check for energy can be chosen between 1000ms and
2000ms. Note that this value consists of the 1000ms phylib interval
for state machine runs plus the time to wait for energy being detected.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/smsc.c  | 82 +++++++++++++++++++++++++++++++++++++++++
 include/linux/smscphy.h |  4 ++
 2 files changed, 86 insertions(+)

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index 0cd433f01..cca5bf46f 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -34,6 +34,8 @@
 #define SPECIAL_CTRL_STS_AMDIX_STATE_	0x2000
 
 #define EDPD_MAX_WAIT_DFLT		640
+/* interval between phylib state machine runs in ms */
+#define PHY_STATE_MACH_MS		1000
 
 struct smsc_hw_stat {
 	const char *string;
@@ -295,6 +297,86 @@ static void smsc_get_stats(struct phy_device *phydev,
 		data[i] = smsc_get_stat(phydev, i);
 }
 
+static int smsc_phy_get_edpd(struct phy_device *phydev, u16 *edpd)
+{
+	struct smsc_phy_priv *priv = phydev->priv;
+
+	if (!priv)
+		return -EOPNOTSUPP;
+
+	if (!priv->edpd_enable)
+		*edpd = ETHTOOL_PHY_EDPD_DISABLE;
+	else if (!priv->edpd_max_wait_ms)
+		*edpd = ETHTOOL_PHY_EDPD_NO_TX;
+	else
+		*edpd = PHY_STATE_MACH_MS + priv->edpd_max_wait_ms;
+
+	return 0;
+}
+
+static int smsc_phy_set_edpd(struct phy_device *phydev, u16 edpd)
+{
+	struct smsc_phy_priv *priv = phydev->priv;
+	int ret;
+
+	if (!priv)
+		return -EOPNOTSUPP;
+
+	mutex_lock(&phydev->lock);
+
+	switch (edpd) {
+	case ETHTOOL_PHY_EDPD_DISABLE:
+		priv->edpd_enable = false;
+		break;
+	case ETHTOOL_PHY_EDPD_NO_TX:
+		priv->edpd_enable = true;
+		priv->edpd_max_wait_ms = 0;
+		break;
+	case ETHTOOL_PHY_EDPD_DFLT_TX_MSECS:
+		edpd = PHY_STATE_MACH_MS + EDPD_MAX_WAIT_DFLT;
+		fallthrough;
+	default:
+		if (phydev->irq != PHY_POLL)
+			return -EOPNOTSUPP;
+		if (edpd < PHY_STATE_MACH_MS || edpd > PHY_STATE_MACH_MS + 1000)
+			return -EINVAL;
+		priv->edpd_enable = true;
+		priv->edpd_max_wait_ms = edpd - PHY_STATE_MACH_MS;
+	}
+
+	priv->edpd_mode_set_by_user = true;
+
+	ret = smsc_phy_config_edpd(phydev);
+
+	mutex_unlock(&phydev->lock);
+
+	return ret;
+}
+
+int smsc_phy_get_tunable(struct phy_device *phydev,
+			 struct ethtool_tunable *tuna, void *data)
+{
+	switch (tuna->id) {
+	case ETHTOOL_PHY_EDPD:
+		return smsc_phy_get_edpd(phydev, data);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+EXPORT_SYMBOL_GPL(smsc_phy_get_tunable);
+
+int smsc_phy_set_tunable(struct phy_device *phydev,
+			 struct ethtool_tunable *tuna, const void *data)
+{
+	switch (tuna->id) {
+	case ETHTOOL_PHY_EDPD:
+		return smsc_phy_set_edpd(phydev, *(u16 *)data);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+EXPORT_SYMBOL_GPL(smsc_phy_set_tunable);
+
 int smsc_phy_probe(struct phy_device *phydev)
 {
 	struct device *dev = &phydev->mdio.dev;
diff --git a/include/linux/smscphy.h b/include/linux/smscphy.h
index 80f37c1db..e1c886277 100644
--- a/include/linux/smscphy.h
+++ b/include/linux/smscphy.h
@@ -32,6 +32,10 @@ int smsc_phy_config_intr(struct phy_device *phydev);
 irqreturn_t smsc_phy_handle_interrupt(struct phy_device *phydev);
 int smsc_phy_config_init(struct phy_device *phydev);
 int lan87xx_read_status(struct phy_device *phydev);
+int smsc_phy_get_tunable(struct phy_device *phydev,
+			 struct ethtool_tunable *tuna, void *data);
+int smsc_phy_set_tunable(struct phy_device *phydev,
+			 struct ethtool_tunable *tuna, const void *data);
 int smsc_phy_probe(struct phy_device *phydev);
 
 #endif /* __LINUX_SMSCPHY_H__ */
-- 
2.40.0

[PATCH net-next 7/7] net: phy: smsc: enable edpd tunable support

[Heiner Kallweit]

Enable EDPD PHY tunable support for all drivers using
lan87xx_read_status.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/smsc.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index cca5bf46f..a85763c02 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -476,6 +476,9 @@ static struct phy_driver smsc_phy_driver[] = {
 	.get_strings	= smsc_get_strings,
 	.get_stats	= smsc_get_stats,
 
+	.get_tunable	= smsc_phy_get_tunable,
+	.set_tunable	= smsc_phy_set_tunable,
+
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
 }, {
@@ -520,6 +523,9 @@ static struct phy_driver smsc_phy_driver[] = {
 	.get_strings	= smsc_get_strings,
 	.get_stats	= smsc_get_stats,
 
+	.get_tunable	= smsc_phy_get_tunable,
+	.set_tunable	= smsc_phy_set_tunable,
+
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
 }, {
@@ -546,6 +552,9 @@ static struct phy_driver smsc_phy_driver[] = {
 	.get_strings	= smsc_get_strings,
 	.get_stats	= smsc_get_stats,
 
+	.get_tunable	= smsc_phy_get_tunable,
+	.set_tunable	= smsc_phy_set_tunable,
+
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
 }, {
@@ -576,6 +585,9 @@ static struct phy_driver smsc_phy_driver[] = {
 	.get_strings	= smsc_get_strings,
 	.get_stats	= smsc_get_stats,
 
+	.get_tunable	= smsc_phy_get_tunable,
+	.set_tunable	= smsc_phy_set_tunable,
+
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
 } };
-- 
2.40.0

Re: [PATCH net-next 6/7] net: phy: smsc: add edpd tunable support

[Simon Horman]

On Sun, Apr 02, 2023 at 11:47:34AM +0200, Heiner Kallweit wrote:
> This adds support for the EDPD PHY tunable.
> Per default EDPD is disabled in interrupt mode, the tunable can be used
> to override this, e.g. if the link partner doesn't use EDPD.
> The interval to check for energy can be chosen between 1000ms and
> 2000ms. Note that this value consists of the 1000ms phylib interval
> for state machine runs plus the time to wait for energy being detected.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/phy/smsc.c  | 82 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/smscphy.h |  4 ++
>  2 files changed, 86 insertions(+)
> 
> diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
> index 0cd433f01..cca5bf46f 100644
> --- a/drivers/net/phy/smsc.c
> +++ b/drivers/net/phy/smsc.c
> @@ -34,6 +34,8 @@
>  #define SPECIAL_CTRL_STS_AMDIX_STATE_	0x2000
>  
>  #define EDPD_MAX_WAIT_DFLT		640

nit: Maybe this could be EDPD_MAX_WAIT_DFLT_MS for consistency
     with PHY_STATE_MACH_MS.

> +/* interval between phylib state machine runs in ms */
> +#define PHY_STATE_MACH_MS		1000
>  
>  struct smsc_hw_stat {
>  	const char *string;
> @@ -295,6 +297,86 @@ static void smsc_get_stats(struct phy_device *phydev,
>  		data[i] = smsc_get_stat(phydev, i);
>  }
>  
> +static int smsc_phy_get_edpd(struct phy_device *phydev, u16 *edpd)
> +{
> +	struct smsc_phy_priv *priv = phydev->priv;
> +
> +	if (!priv)
> +		return -EOPNOTSUPP;
> +
> +	if (!priv->edpd_enable)
> +		*edpd = ETHTOOL_PHY_EDPD_DISABLE;
> +	else if (!priv->edpd_max_wait_ms)
> +		*edpd = ETHTOOL_PHY_EDPD_NO_TX;
> +	else
> +		*edpd = PHY_STATE_MACH_MS + priv->edpd_max_wait_ms;
> +
> +	return 0;
> +}
> +
> +static int smsc_phy_set_edpd(struct phy_device *phydev, u16 edpd)
> +{
> +	struct smsc_phy_priv *priv = phydev->priv;
> +	int ret;
> +
> +	if (!priv)
> +		return -EOPNOTSUPP;
> +
> +	mutex_lock(&phydev->lock);

I am a little confused by this as by my reasoning this code is called via
the first arm of the following in set_phy_tunable(), and phydev->lock is
already held.

        if (phy_drv_tunable) {
                mutex_lock(&phydev->lock);
                ret = phydev->drv->set_tunable(phydev, &tuna, data);
                mutex_unlock(&phydev->lock);
        } else {
                ret = dev->ethtool_ops->set_phy_tunable(dev, &tuna, data);
        }

> +
> +	switch (edpd) {
> +	case ETHTOOL_PHY_EDPD_DISABLE:
> +		priv->edpd_enable = false;
> +		break;
> +	case ETHTOOL_PHY_EDPD_NO_TX:
> +		priv->edpd_enable = true;
> +		priv->edpd_max_wait_ms = 0;
> +		break;
> +	case ETHTOOL_PHY_EDPD_DFLT_TX_MSECS:
> +		edpd = PHY_STATE_MACH_MS + EDPD_MAX_WAIT_DFLT;
> +		fallthrough;
> +	default:
> +		if (phydev->irq != PHY_POLL)
> +			return -EOPNOTSUPP;

This returns without releasing phydev->lock.
Is that intended?

> +		if (edpd < PHY_STATE_MACH_MS || edpd > PHY_STATE_MACH_MS + 1000)
> +			return -EINVAL;

Ditto.

> +		priv->edpd_enable = true;
> +		priv->edpd_max_wait_ms = edpd - PHY_STATE_MACH_MS;
> +	}
> +
> +	priv->edpd_mode_set_by_user = true;
> +
> +	ret = smsc_phy_config_edpd(phydev);
> +
> +	mutex_unlock(&phydev->lock);
> +
> +	return ret;
> +}
> +
> +int smsc_phy_get_tunable(struct phy_device *phydev,
> +			 struct ethtool_tunable *tuna, void *data)
> +{
> +	switch (tuna->id) {
> +	case ETHTOOL_PHY_EDPD:
> +		return smsc_phy_get_edpd(phydev, data);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(smsc_phy_get_tunable);
> +
> +int smsc_phy_set_tunable(struct phy_device *phydev,
> +			 struct ethtool_tunable *tuna, const void *data)
> +{
> +	switch (tuna->id) {
> +	case ETHTOOL_PHY_EDPD:
> +		return smsc_phy_set_edpd(phydev, *(u16 *)data);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(smsc_phy_set_tunable);
> +
>  int smsc_phy_probe(struct phy_device *phydev)
>  {
>  	struct device *dev = &phydev->mdio.dev;
> diff --git a/include/linux/smscphy.h b/include/linux/smscphy.h
> index 80f37c1db..e1c886277 100644
> --- a/include/linux/smscphy.h
> +++ b/include/linux/smscphy.h
> @@ -32,6 +32,10 @@ int smsc_phy_config_intr(struct phy_device *phydev);
>  irqreturn_t smsc_phy_handle_interrupt(struct phy_device *phydev);
>  int smsc_phy_config_init(struct phy_device *phydev);
>  int lan87xx_read_status(struct phy_device *phydev);
> +int smsc_phy_get_tunable(struct phy_device *phydev,
> +			 struct ethtool_tunable *tuna, void *data);
> +int smsc_phy_set_tunable(struct phy_device *phydev,
> +			 struct ethtool_tunable *tuna, const void *data);
>  int smsc_phy_probe(struct phy_device *phydev);
>  
>  #endif /* __LINUX_SMSCPHY_H__ */
> -- 
> 2.40.0
> 
> 

Re: [PATCH net-next 6/7] net: phy: smsc: add edpd tunable support

[Heiner Kallweit]

On 02.04.2023 14:08, Simon Horman wrote:
> On Sun, Apr 02, 2023 at 11:47:34AM +0200, Heiner Kallweit wrote:
>> This adds support for the EDPD PHY tunable.
>> Per default EDPD is disabled in interrupt mode, the tunable can be used
>> to override this, e.g. if the link partner doesn't use EDPD.
>> The interval to check for energy can be chosen between 1000ms and
>> 2000ms. Note that this value consists of the 1000ms phylib interval
>> for state machine runs plus the time to wait for energy being detected.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/net/phy/smsc.c  | 82 +++++++++++++++++++++++++++++++++++++++++
>>  include/linux/smscphy.h |  4 ++
>>  2 files changed, 86 insertions(+)
>>
>> diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
>> index 0cd433f01..cca5bf46f 100644
>> --- a/drivers/net/phy/smsc.c
>> +++ b/drivers/net/phy/smsc.c
>> @@ -34,6 +34,8 @@
>>  #define SPECIAL_CTRL_STS_AMDIX_STATE_	0x2000
>>  
>>  #define EDPD_MAX_WAIT_DFLT		640
> 
> nit: Maybe this could be EDPD_MAX_WAIT_DFLT_MS for consistency
>      with PHY_STATE_MACH_MS.
> 
Yes, this would be better.

>> +/* interval between phylib state machine runs in ms */
>> +#define PHY_STATE_MACH_MS		1000
>>  
>>  struct smsc_hw_stat {
>>  	const char *string;
>> @@ -295,6 +297,86 @@ static void smsc_get_stats(struct phy_device *phydev,
>>  		data[i] = smsc_get_stat(phydev, i);
>>  }
>>  
>> +static int smsc_phy_get_edpd(struct phy_device *phydev, u16 *edpd)
>> +{
>> +	struct smsc_phy_priv *priv = phydev->priv;
>> +
>> +	if (!priv)
>> +		return -EOPNOTSUPP;
>> +
>> +	if (!priv->edpd_enable)
>> +		*edpd = ETHTOOL_PHY_EDPD_DISABLE;
>> +	else if (!priv->edpd_max_wait_ms)
>> +		*edpd = ETHTOOL_PHY_EDPD_NO_TX;
>> +	else
>> +		*edpd = PHY_STATE_MACH_MS + priv->edpd_max_wait_ms;
>> +
>> +	return 0;
>> +}
>> +
>> +static int smsc_phy_set_edpd(struct phy_device *phydev, u16 edpd)
>> +{
>> +	struct smsc_phy_priv *priv = phydev->priv;
>> +	int ret;
>> +
>> +	if (!priv)
>> +		return -EOPNOTSUPP;
>> +
>> +	mutex_lock(&phydev->lock);
> 
> I am a little confused by this as by my reasoning this code is called via
> the first arm of the following in set_phy_tunable(), and phydev->lock is
> already held.
> 

You're right of course. So we can remove the locking in the driver.

>         if (phy_drv_tunable) {
>                 mutex_lock(&phydev->lock);
>                 ret = phydev->drv->set_tunable(phydev, &tuna, data);
>                 mutex_unlock(&phydev->lock);
>         } else {
>                 ret = dev->ethtool_ops->set_phy_tunable(dev, &tuna, data);
>         }
> 
>> +
>> +	switch (edpd) {
>> +	case ETHTOOL_PHY_EDPD_DISABLE:
>> +		priv->edpd_enable = false;
>> +		break;
>> +	case ETHTOOL_PHY_EDPD_NO_TX:
>> +		priv->edpd_enable = true;
>> +		priv->edpd_max_wait_ms = 0;
>> +		break;
>> +	case ETHTOOL_PHY_EDPD_DFLT_TX_MSECS:
>> +		edpd = PHY_STATE_MACH_MS + EDPD_MAX_WAIT_DFLT;
>> +		fallthrough;
>> +	default:
>> +		if (phydev->irq != PHY_POLL)
>> +			return -EOPNOTSUPP;
> 
> This returns without releasing phydev->lock.
> Is that intended?
> 
>> +		if (edpd < PHY_STATE_MACH_MS || edpd > PHY_STATE_MACH_MS + 1000)
>> +			return -EINVAL;
> 
> Ditto.
> 
>> +		priv->edpd_enable = true;
>> +		priv->edpd_max_wait_ms = edpd - PHY_STATE_MACH_MS;
>> +	}
>> +
>> +	priv->edpd_mode_set_by_user = true;
>> +
>> +	ret = smsc_phy_config_edpd(phydev);
>> +
>> +	mutex_unlock(&phydev->lock);
>> +
>> +	return ret;
>> +}
>> +
>> +int smsc_phy_get_tunable(struct phy_device *phydev,
>> +			 struct ethtool_tunable *tuna, void *data)
>> +{
>> +	switch (tuna->id) {
>> +	case ETHTOOL_PHY_EDPD:
>> +		return smsc_phy_get_edpd(phydev, data);
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +}
>> +EXPORT_SYMBOL_GPL(smsc_phy_get_tunable);
>> +
>> +int smsc_phy_set_tunable(struct phy_device *phydev,
>> +			 struct ethtool_tunable *tuna, const void *data)
>> +{
>> +	switch (tuna->id) {
>> +	case ETHTOOL_PHY_EDPD:
>> +		return smsc_phy_set_edpd(phydev, *(u16 *)data);
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +}
>> +EXPORT_SYMBOL_GPL(smsc_phy_set_tunable);
>> +
>>  int smsc_phy_probe(struct phy_device *phydev)
>>  {
>>  	struct device *dev = &phydev->mdio.dev;
>> diff --git a/include/linux/smscphy.h b/include/linux/smscphy.h
>> index 80f37c1db..e1c886277 100644
>> --- a/include/linux/smscphy.h
>> +++ b/include/linux/smscphy.h
>> @@ -32,6 +32,10 @@ int smsc_phy_config_intr(struct phy_device *phydev);
>>  irqreturn_t smsc_phy_handle_interrupt(struct phy_device *phydev);
>>  int smsc_phy_config_init(struct phy_device *phydev);
>>  int lan87xx_read_status(struct phy_device *phydev);
>> +int smsc_phy_get_tunable(struct phy_device *phydev,
>> +			 struct ethtool_tunable *tuna, void *data);
>> +int smsc_phy_set_tunable(struct phy_device *phydev,
>> +			 struct ethtool_tunable *tuna, const void *data);
>>  int smsc_phy_probe(struct phy_device *phydev);
>>  
>>  #endif /* __LINUX_SMSCPHY_H__ */
>> -- 
>> 2.40.0
>>
>>

Re: [PATCH net-next 0/7] net: phy: smsc: add support for edpd tunable

[Florian Fainelli]

On 4/2/2023 2:43 AM, Heiner Kallweit wrote:
> This adds support for the EDPD PHY tunable.
> Per default EDPD is disabled in interrupt mode, the tunable can be used
> to override this, e.g. if the link partner doesn't use EDPD.
> The interval to check for energy can be chosen between 1000ms and
> 2000ms. Note that this value consists of the 1000ms phylib interval
> for state machine runs plus the time to wait for energy being detected.

AFAIR Chris Healy was trying to get something similar done, maybe you 
can CC him on the next version?
-- 
Florian

Re: [PATCH net-next 0/7] net: phy: smsc: add support for edpd tunable

[Heiner Kallweit]

On 02.04.2023 16:00, Florian Fainelli wrote:
> 
> 
> On 4/2/2023 2:43 AM, Heiner Kallweit wrote:
>> This adds support for the EDPD PHY tunable.
>> Per default EDPD is disabled in interrupt mode, the tunable can be used
>> to override this, e.g. if the link partner doesn't use EDPD.
>> The interval to check for energy can be chosen between 1000ms and
>> 2000ms. Note that this value consists of the 1000ms phylib interval
>> for state machine runs plus the time to wait for energy being detected.
> 
> AFAIR Chris Healy was trying to get something similar done, maybe you can CC him on the next version?

Actually this is based on a conversation between him and me. Chris came up
with a first version and I reworked it from scratch to meet mainline standards
and make it usable on SMSC and Amlogic PHY's.

Re: [PATCH net-next 0/7] net: phy: smsc: add support for edpd tunable

[Chris Healy]

On Sun, Apr 2, 2023 at 7:58 AM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> On 02.04.2023 16:00, Florian Fainelli wrote:
> >
> >
> > On 4/2/2023 2:43 AM, Heiner Kallweit wrote:
> >> This adds support for the EDPD PHY tunable.
> >> Per default EDPD is disabled in interrupt mode, the tunable can be used
> >> to override this, e.g. if the link partner doesn't use EDPD.
> >> The interval to check for energy can be chosen between 1000ms and
> >> 2000ms. Note that this value consists of the 1000ms phylib interval
> >> for state machine runs plus the time to wait for energy being detected.
> >
> > AFAIR Chris Healy was trying to get something similar done, maybe you can CC him on the next version?
>
> Actually this is based on a conversation between him and me. Chris came up
> with a first version and I reworked it from scratch to meet mainline standards
> and make it usable on SMSC and Amlogic PHY's.
>

It's great to see this feature coming together.  I wish I would have
had more time to help out.