diff mbox series

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

Message ID 66446a75-8087-10f4-fc37-b97e13b88c27@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: phy: smsc: add support for edpd tunable | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 18 this patch: 18
netdev/cc_maintainers warning 1 maintainers not CCed: healych@amazon.com
netdev/build_clang success Errors and warnings before: 18 this patch: 18
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 18 this patch: 18
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 104 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Heiner Kallweit April 2, 2023, 9:47 a.m. UTC 
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(+)

Comments

Simon Horman April 2, 2023, 12:08 p.m. UTC | #1 
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
> 
>
Heiner Kallweit April 2, 2023, 1:52 p.m. UTC | #2 
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
>>
>>
diff mbox series

Patch

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__ */