[PATCH v2 0/2] ethtool: implement Energy Detect Powerdown support via phy-tunable

[PATCH v2 0/2] ethtool: implement Energy Detect Powerdown support via phy-tunable

[Alexandru Ardelean]

This changeset proposes a new control for PHY tunable to control Energy
Detect Power Down.

The `phy_tunable_id` has been named `ETHTOOL_PHY_EDPD` since it looks like
this feature is common across other PHYs (like EEE), and defining
`ETHTOOL_PHY_ENERGY_DETECT_POWER_DOWN` seems too long.
    
The way EDPD works, is that the RX block is put to a lower power mode,
except for link-pulse detection circuits. The TX block is also put to low
power mode, but the PHY wakes-up periodically to send link pulses, to avoid
lock-ups in case the other side is also in EDPD mode.
    
Currently, there are 2 PHY drivers that look like they could use this new
PHY tunable feature: the `adin` && `micrel` PHYs.

This series updates only the `adin` PHY driver to support this new feature,
as this chip has been tested. A change for `micrel` can be proposed after a
discussion of the PHY-tunable API is resolved.

Alexandru Ardelean (2):
  ethtool: implement Energy Detect Powerdown support via phy-tunable
  net: phy: adin: implement Energy Detect Powerdown mode via phy-tunable

 drivers/net/phy/adin.c       | 50 ++++++++++++++++++++++++++++++++++++
 include/uapi/linux/ethtool.h |  5 ++++
 net/core/ethtool.c           |  6 +++++
 3 files changed, 61 insertions(+)

Changelog v1 -> v2:
* initial series was made up of 2 sub-series: 1 for kernel & 1 for ethtool
  in userspace; v2 contains only the kernel series

-- 
2.20.1

[PATCH v2 1/2] ethtool: implement Energy Detect Powerdown support via phy-tunable

[Alexandru Ardelean]

The `phy_tunable_id` has been named `ETHTOOL_PHY_EDPD` since it looks like
this feature is common across other PHYs (like EEE), and defining
`ETHTOOL_PHY_ENERGY_DETECT_POWER_DOWN` seems too long.

The way EDPD works, is that the RX block is put to a lower power mode,
except for link-pulse detection circuits. The TX block is also put to low
power mode, but the PHY wakes-up periodically to send link pulses, to avoid
lock-ups in case the other side is also in EDPD mode.

Currently, there are 2 PHY drivers that look like they could use this new
PHY tunable feature: the `adin` && `micrel` PHYs.

The ADIN's datasheet mentions that TX pulses are at intervals of 1 second
default each, and they can be disabled. For the Micrel KSZ9031 PHY, the
datasheet does not mention whether they can be disabled, but mentions that
they can modified.

The way this change is structured, is similar to the PHY tunable downshift
control:
* a `ETHTOOL_PHY_EDPD_DFLT_TX_INTERVAL` value is exposed to cover a default
  TX interval; some PHYs could specify a certain value that makes sense
* `ETHTOOL_PHY_EDPD_NO_TX` would disable TX when EDPD is enabled
* `ETHTOOL_PHY_EDPD_DISABLE` will disable EDPD

This should allow PHYs to:
* enable EDPD and not enable TX pulses (interval would be 0)
* enable EDPD and configure TX pulse interval; note that TX interval units
  would be PHY specific; we could consider `seconds` as units, but it could
  happen that some PHYs would be prefer 500 milliseconds as a unit;
  a maximum of 32766 units should be sufficient
* disable EDPD

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 include/uapi/linux/ethtool.h | 5 +++++
 net/core/ethtool.c           | 6 ++++++
 2 files changed, 11 insertions(+)

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index dd06302aa93e..0349e9c4350f 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -259,10 +259,15 @@ struct ethtool_tunable {
 #define ETHTOOL_PHY_FAST_LINK_DOWN_ON	0
 #define ETHTOOL_PHY_FAST_LINK_DOWN_OFF	0xff
 
+#define ETHTOOL_PHY_EDPD_DFLT_TX_INTERVAL	0x7fff
+#define ETHTOOL_PHY_EDPD_NO_TX			0x8000
+#define ETHTOOL_PHY_EDPD_DISABLE		0
+
 enum phy_tunable_id {
 	ETHTOOL_PHY_ID_UNSPEC,
 	ETHTOOL_PHY_DOWNSHIFT,
 	ETHTOOL_PHY_FAST_LINK_DOWN,
+	ETHTOOL_PHY_EDPD,
 	/*
 	 * Add your fresh new phy tunable attribute above and remember to update
 	 * phy_tunable_strings[] in net/core/ethtool.c
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 6288e69e94fc..c763106c73fc 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -133,6 +133,7 @@ phy_tunable_strings[__ETHTOOL_PHY_TUNABLE_COUNT][ETH_GSTRING_LEN] = {
 	[ETHTOOL_ID_UNSPEC]     = "Unspec",
 	[ETHTOOL_PHY_DOWNSHIFT]	= "phy-downshift",
 	[ETHTOOL_PHY_FAST_LINK_DOWN] = "phy-fast-link-down",
+	[ETHTOOL_PHY_EDPD]	= "phy-energy-detect-power-down",
 };
 
 static int ethtool_get_features(struct net_device *dev, void __user *useraddr)
@@ -2451,6 +2452,11 @@ static int ethtool_phy_tunable_valid(const struct ethtool_tunable *tuna)
 		    tuna->type_id != ETHTOOL_TUNABLE_U8)
 			return -EINVAL;
 		break;
+	case ETHTOOL_PHY_EDPD:
+		if (tuna->len != sizeof(u16) ||
+		    tuna->type_id != ETHTOOL_TUNABLE_U16)
+			return -EINVAL;
+		break;
 	default:
 		return -EINVAL;
 	}
-- 
2.20.1

Re: [PATCH v2 1/2] ethtool: implement Energy Detect Powerdown support via phy-tunable

[Andrew Lunn]

On Wed, Sep 04, 2019 at 07:23:21PM +0300, Alexandru Ardelean wrote:

Hi Alexandru

Somewhere we need a comment stating what EDPD means. Here would be a
good place.

> +#define ETHTOOL_PHY_EDPD_DFLT_TX_INTERVAL	0x7fff
> +#define ETHTOOL_PHY_EDPD_NO_TX			0x8000
> +#define ETHTOOL_PHY_EDPD_DISABLE		0

I think you are passing a u16. So why not 0xfffe and 0xffff?  We also
need to make it clear what the units are for interval. This file
specifies the contract between the kernel and user space. So we need
to clearly define what we mean here. Lots of comments are better than
no comments.

   Andrew

Re: [PATCH v2 1/2] ethtool: implement Energy Detect Powerdown support via phy-tunable

[Ardelean, Alexandru]

On Wed, 2019-09-04 at 21:53 +0200, Andrew Lunn wrote:
> [External]
> 
> On Wed, Sep 04, 2019 at 07:23:21PM +0300, Alexandru Ardelean wrote:
> 
> Hi Alexandru
> 
> Somewhere we need a comment stating what EDPD means. Here would be a
> good place.

ack

> 
> > +#define ETHTOOL_PHY_EDPD_DFLT_TX_INTERVAL	0x7fff
> > +#define ETHTOOL_PHY_EDPD_NO_TX			0x8000
> > +#define ETHTOOL_PHY_EDPD_DISABLE		0
> 
> I think you are passing a u16. So why not 0xfffe and 0xffff?  We also
> need to make it clear what the units are for interval. This file

I initially thought about keeping this u8 and going with 0xff & 0xfe.
But 254 or 253 could be too small to specify the value of an interval.

Also (maybe due ti all the coding-patterns that I saw over the course of some time), make me feel that I should add a
flag somewhere.

Bottom line is: 0xfffe and 0xffff also work from my side, if it is acceptable (by the community).

Another approach I considered, was to maybe have this EDPD just do enable & disable (which is sufficient for the `adin`
PHY & `micrel` as well).
That would mean that if we would ever want to configure the TX interval (in the future), we would need an extra PHY-
tunable parameter just for that; because changing the enable/disable behavior would be dangerous.
And also, deferring the TX-interval configuration, does not sound like good design/pattern, since it can allow for tons
of PHY-tunable parameters for every little knob.

> specifies the contract between the kernel and user space. So we need
> to clearly define what we mean here. Lots of comments are better than
> no comments.

Will come back with more comments.

> 
>    Andrew

Re: [PATCH v2 1/2] ethtool: implement Energy Detect Powerdown support via phy-tunable

[Florian Fainelli]

On 9/4/19 11:25 PM, Ardelean, Alexandru wrote:
> On Wed, 2019-09-04 at 21:53 +0200, Andrew Lunn wrote:
>> [External]
>>
>> On Wed, Sep 04, 2019 at 07:23:21PM +0300, Alexandru Ardelean wrote:
>>
>> Hi Alexandru
>>
>> Somewhere we need a comment stating what EDPD means. Here would be a
>> good place.
> 
> ack
> 
>>
>>> +#define ETHTOOL_PHY_EDPD_DFLT_TX_INTERVAL	0x7fff
>>> +#define ETHTOOL_PHY_EDPD_NO_TX			0x8000
>>> +#define ETHTOOL_PHY_EDPD_DISABLE		0
>>
>> I think you are passing a u16. So why not 0xfffe and 0xffff?  We also
>> need to make it clear what the units are for interval. This file
> 
> I initially thought about keeping this u8 and going with 0xff & 0xfe.
> But 254 or 253 could be too small to specify the value of an interval.
> 
> Also (maybe due ti all the coding-patterns that I saw over the course of some time), make me feel that I should add a
> flag somewhere.
> 
> Bottom line is: 0xfffe and 0xffff also work from my side, if it is acceptable (by the community).
> 
> Another approach I considered, was to maybe have this EDPD just do enable & disable (which is sufficient for the `adin`
> PHY & `micrel` as well).
> That would mean that if we would ever want to configure the TX interval (in the future), we would need an extra PHY-
> tunable parameter just for that; because changing the enable/disable behavior would be dangerous.
> And also, deferring the TX-interval configuration, does not sound like good design/pattern, since it can allow for tons
> of PHY-tunable parameters for every little knob.

It seems to me that the interval is a better way to deal with that, if
you specify a non zero interval, you enable EDPD, even if your PHY can
only act on an enable/disable bit. For PHYs that do support setting a TX
internal, the non-zero interval can be translated into whatever
appropriate unit. In all cases, a 0 interval means disable.

Andrew, does that work  for you?
-- 
Florian

[PATCH v2 2/2] net: phy: adin: implement Energy Detect Powerdown mode via phy-tunable

[Alexandru Ardelean]

This driver becomes the first user of the kernel's `ETHTOOL_PHY_EDPD`
phy-tunable feature.
EDPD is also enabled by default on PHY config_init, but can be disabled via
the phy-tunable control.

When enabling EDPD, it's also a good idea (for the ADIN PHYs) to enable TX
periodic pulses, so that in case the other PHY is also on EDPD mode, there
is no lock-up situation where both sides are waiting for the other to
transmit.

Via the phy-tunable control, TX pulses can be disabled if specifying 0
`tx-interval` via ethtool.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/net/phy/adin.c | 50 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index 4dec83df048d..742728ab2a5d 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -26,6 +26,11 @@
 
 #define ADIN1300_RX_ERR_CNT			0x0014
 
+#define ADIN1300_PHY_CTRL_STATUS2		0x0015
+#define   ADIN1300_NRG_PD_EN			BIT(3)
+#define   ADIN1300_NRG_PD_TX_EN			BIT(2)
+#define   ADIN1300_NRG_PD_STATUS		BIT(1)
+
 #define ADIN1300_PHY_CTRL2			0x0016
 #define   ADIN1300_DOWNSPEED_AN_100_EN		BIT(11)
 #define   ADIN1300_DOWNSPEED_AN_10_EN		BIT(10)
@@ -328,12 +333,51 @@ static int adin_set_downshift(struct phy_device *phydev, u8 cnt)
 			    ADIN1300_DOWNSPEEDS_EN);
 }
 
+static int adin_get_edpd(struct phy_device *phydev, u16 *tx_interval)
+{
+	int val;
+
+	val = phy_read(phydev, ADIN1300_PHY_CTRL_STATUS2);
+	if (val < 0)
+		return val;
+
+	if (ADIN1300_NRG_PD_EN & val) {
+		if (val & ADIN1300_NRG_PD_TX_EN)
+			*tx_interval = 1;
+		else
+			*tx_interval = ETHTOOL_PHY_EDPD_NO_TX;
+	} else {
+		*tx_interval = ETHTOOL_PHY_EDPD_DISABLE;
+	}
+
+	return 0;
+}
+
+static int adin_set_edpd(struct phy_device *phydev, u16 tx_interval)
+{
+	u16 val;
+
+	if (tx_interval == ETHTOOL_PHY_EDPD_DISABLE)
+		return phy_clear_bits(phydev, ADIN1300_PHY_CTRL_STATUS2,
+				(ADIN1300_NRG_PD_EN | ADIN1300_NRG_PD_TX_EN));
+
+	val = ADIN1300_NRG_PD_EN;
+	if (tx_interval != ETHTOOL_PHY_EDPD_NO_TX)
+		val |= ADIN1300_NRG_PD_TX_EN;
+
+	return phy_modify(phydev, ADIN1300_PHY_CTRL_STATUS2,
+			  (ADIN1300_NRG_PD_EN | ADIN1300_NRG_PD_TX_EN),
+			  val);
+}
+
 static int adin_get_tunable(struct phy_device *phydev,
 			    struct ethtool_tunable *tuna, void *data)
 {
 	switch (tuna->id) {
 	case ETHTOOL_PHY_DOWNSHIFT:
 		return adin_get_downshift(phydev, data);
+	case ETHTOOL_PHY_EDPD:
+		return adin_get_edpd(phydev, data);
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -345,6 +389,8 @@ static int adin_set_tunable(struct phy_device *phydev,
 	switch (tuna->id) {
 	case ETHTOOL_PHY_DOWNSHIFT:
 		return adin_set_downshift(phydev, *(const u8 *)data);
+	case ETHTOOL_PHY_EDPD:
+		return adin_set_edpd(phydev, *(const u16 *)data);
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -368,6 +414,10 @@ static int adin_config_init(struct phy_device *phydev)
 	if (rc < 0)
 		return rc;
 
+	rc = adin_set_edpd(phydev, 1);
+	if (rc < 0)
+		return rc;
+
 	phydev_dbg(phydev, "PHY is using mode '%s'\n",
 		   phy_modes(phydev->interface));
 
-- 
2.20.1

Re: [PATCH v2 2/2] net: phy: adin: implement Energy Detect Powerdown mode via phy-tunable

[Andrew Lunn]

On Wed, Sep 04, 2019 at 07:23:22PM +0300, Alexandru Ardelean wrote:
> This driver becomes the first user of the kernel's `ETHTOOL_PHY_EDPD`
> phy-tunable feature.
> EDPD is also enabled by default on PHY config_init, but can be disabled via
> the phy-tunable control.
> 
> When enabling EDPD, it's also a good idea (for the ADIN PHYs) to enable TX
> periodic pulses, so that in case the other PHY is also on EDPD mode, there
> is no lock-up situation where both sides are waiting for the other to
> transmit.
> 
> Via the phy-tunable control, TX pulses can be disabled if specifying 0
> `tx-interval` via ethtool.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/net/phy/adin.c | 50 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> index 4dec83df048d..742728ab2a5d 100644
> --- a/drivers/net/phy/adin.c
> +++ b/drivers/net/phy/adin.c
> @@ -26,6 +26,11 @@
>  
>  #define ADIN1300_RX_ERR_CNT			0x0014
>  
> +#define ADIN1300_PHY_CTRL_STATUS2		0x0015
> +#define   ADIN1300_NRG_PD_EN			BIT(3)
> +#define   ADIN1300_NRG_PD_TX_EN			BIT(2)
> +#define   ADIN1300_NRG_PD_STATUS		BIT(1)
> +
>  #define ADIN1300_PHY_CTRL2			0x0016
>  #define   ADIN1300_DOWNSPEED_AN_100_EN		BIT(11)
>  #define   ADIN1300_DOWNSPEED_AN_10_EN		BIT(10)
> @@ -328,12 +333,51 @@ static int adin_set_downshift(struct phy_device *phydev, u8 cnt)
>  			    ADIN1300_DOWNSPEEDS_EN);
>  }
>  
> +static int adin_get_edpd(struct phy_device *phydev, u16 *tx_interval)
> +{
> +	int val;
> +
> +	val = phy_read(phydev, ADIN1300_PHY_CTRL_STATUS2);
> +	if (val < 0)
> +		return val;
> +
> +	if (ADIN1300_NRG_PD_EN & val) {
> +		if (val & ADIN1300_NRG_PD_TX_EN)
> +			*tx_interval = 1;

What does 1 mean? 1 pico second, one hour? Anything but zero seconds?
Does the datasheet specify what it actually does? Maybe you should be
using ETHTOOL_PHY_EDPD_DFLT_TX_INTERVAL here, to indicate you actually
have no idea, but it is the default for this PHY?

> +		else
> +			*tx_interval = ETHTOOL_PHY_EDPD_NO_TX;
> +	} else {
> +		*tx_interval = ETHTOOL_PHY_EDPD_DISABLE;
> +	}
> +
> +	return 0;
> +}
> +
> +static int adin_set_edpd(struct phy_device *phydev, u16 tx_interval)
> +{
> +	u16 val;
> +
> +	if (tx_interval == ETHTOOL_PHY_EDPD_DISABLE)
> +		return phy_clear_bits(phydev, ADIN1300_PHY_CTRL_STATUS2,
> +				(ADIN1300_NRG_PD_EN | ADIN1300_NRG_PD_TX_EN));
> +
> +	val = ADIN1300_NRG_PD_EN;
> +	if (tx_interval != ETHTOOL_PHY_EDPD_NO_TX)
> +		val |= ADIN1300_NRG_PD_TX_EN;

So you silently accept any interval? That sounds wrong. You really
should be returning -EINVAL for any value other than, either 1, or
maybe ETHTOOL_PHY_EDPD_DFLT_TX_INTERVAL, if you change the get
function.

      Andrew

Re: [PATCH v2 2/2] net: phy: adin: implement Energy Detect Powerdown mode via phy-tunable

[Ardelean, Alexandru]

On Wed, 2019-09-04 at 22:03 +0200, Andrew Lunn wrote:
> [External]
> 
> On Wed, Sep 04, 2019 at 07:23:22PM +0300, Alexandru Ardelean wrote:
> > This driver becomes the first user of the kernel's `ETHTOOL_PHY_EDPD`
> > phy-tunable feature.
> > EDPD is also enabled by default on PHY config_init, but can be disabled via
> > the phy-tunable control.
> > 
> > When enabling EDPD, it's also a good idea (for the ADIN PHYs) to enable TX
> > periodic pulses, so that in case the other PHY is also on EDPD mode, there
> > is no lock-up situation where both sides are waiting for the other to
> > transmit.
> > 
> > Via the phy-tunable control, TX pulses can be disabled if specifying 0
> > `tx-interval` via ethtool.
> > 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> >  drivers/net/phy/adin.c | 50 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 50 insertions(+)
> > 
> > diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> > index 4dec83df048d..742728ab2a5d 100644
> > --- a/drivers/net/phy/adin.c
> > +++ b/drivers/net/phy/adin.c
> > @@ -26,6 +26,11 @@
> >  
> >  #define ADIN1300_RX_ERR_CNT			0x0014
> >  
> > +#define ADIN1300_PHY_CTRL_STATUS2		0x0015
> > +#define   ADIN1300_NRG_PD_EN			BIT(3)
> > +#define   ADIN1300_NRG_PD_TX_EN			BIT(2)
> > +#define   ADIN1300_NRG_PD_STATUS		BIT(1)
> > +
> >  #define ADIN1300_PHY_CTRL2			0x0016
> >  #define   ADIN1300_DOWNSPEED_AN_100_EN		BIT(11)
> >  #define   ADIN1300_DOWNSPEED_AN_10_EN		BIT(10)
> > @@ -328,12 +333,51 @@ static int adin_set_downshift(struct phy_device *phydev, u8 cnt)
> >  			    ADIN1300_DOWNSPEEDS_EN);
> >  }
> >  
> > +static int adin_get_edpd(struct phy_device *phydev, u16 *tx_interval)
> > +{
> > +	int val;
> > +
> > +	val = phy_read(phydev, ADIN1300_PHY_CTRL_STATUS2);
> > +	if (val < 0)
> > +		return val;
> > +
> > +	if (ADIN1300_NRG_PD_EN & val) {
> > +		if (val & ADIN1300_NRG_PD_TX_EN)
> > +			*tx_interval = 1;
> 
> What does 1 mean? 1 pico second, one hour? Anything but zero seconds?
> Does the datasheet specify what it actually does? Maybe you should be
> using ETHTOOL_PHY_EDPD_DFLT_TX_INTERVAL here, to indicate you actually
> have no idea, but it is the default for this PHY?

This PHY currently has a 1 second TX interval.
As specified by the datasheet (page 22 Energy-Detect Powerdown Mode section):
https://www.analog.com/media/en/technical-documentation/data-sheets/ADIN1300.pdf

"The PHY monitors the line for signal energy and sends a link pulse once every second"

Currently there is no control exposed to configure this interval; but there could be some registers (that are not
exposed yet) that could control this.

Micrel's datasheet mentions (page 27 3.16.1ENERGY-DETECT POWER-DOWN MODE section):
http://ww1.microchip.com/downloads/en/devicedoc/00002117f.pdf

"In EDPD Mode, the KSZ9031RNX shuts down all transceiver blocks, except for the transmitter and energy detect cir-cuits. 
Power can be reduced further by extending the time interval between the transmissions of link pulses to check forthe
presence of a link partner."

I did not check for Micrel to see how that interval is controlled.

The reason for doing this TX interval control was mostly based on this mention from Micrel's datasheet, with
consideration that it could work ADIN & other future PHYs.

> 
> > +		else
> > +			*tx_interval = ETHTOOL_PHY_EDPD_NO_TX;
> > +	} else {
> > +		*tx_interval = ETHTOOL_PHY_EDPD_DISABLE;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int adin_set_edpd(struct phy_device *phydev, u16 tx_interval)
> > +{
> > +	u16 val;
> > +
> > +	if (tx_interval == ETHTOOL_PHY_EDPD_DISABLE)
> > +		return phy_clear_bits(phydev, ADIN1300_PHY_CTRL_STATUS2,
> > +				(ADIN1300_NRG_PD_EN | ADIN1300_NRG_PD_TX_EN));
> > +
> > +	val = ADIN1300_NRG_PD_EN;
> > +	if (tx_interval != ETHTOOL_PHY_EDPD_NO_TX)
> > +		val |= ADIN1300_NRG_PD_TX_EN;
> 
> So you silently accept any interval? That sounds wrong. You really
> should be returning -EINVAL for any value other than, either 1, or
> maybe ETHTOOL_PHY_EDPD_DFLT_TX_INTERVAL, if you change the get
> function.

ack;
will use ETHTOOL_PHY_EDPD_DFLT_TX_INTERVAL, ETHTOOL_PHY_EDPD_NO_TX && 1 as acceptable values here

> 
>       Andrew