This patch series is actually 2 series in 1.
First 2 patches implement the kernel support for controlling Energy Detect
Powerdown support via phy-tunable, and the next 2 patches implement the
ethtool user-space control.
Hopefully, this combination of 2 series is an acceptable approach; if not,
I am fine to re-update it based on feedback.
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.
--
2.20.1
This change adds control for enabling/disabling Energy Detect Power Down
mode, as well as configuring wake-up intervals for TX pulses, via the new
ETHTOOL_PHY_EDPD control added in PHY tunable, in the kernel.
Signed-off-by: Alexandru Ardelean <[email protected]>
---
ethtool.8.in | 28 +++++++++++++++++
ethtool.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++---
2 files changed, 111 insertions(+), 4 deletions(-)
diff --git a/ethtool.8.in b/ethtool.8.in
index cd3be91..a32d48b 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -362,11 +362,17 @@ ethtool \- query or control network driver and hardware settings
.A1 on off
.BN msecs
.RB ]
+.RB [
+.B energy\-detect\-power\-down
+.A1 on off
+.BN tx-interval
+.RB ]
.HP
.B ethtool \-\-get\-phy\-tunable
.I devname
.RB [ downshift ]
.RB [ fast-link-down ]
+.RB [ energy-detect-power-down ]
.HP
.B ethtool \-\-reset
.I devname
@@ -1100,6 +1106,24 @@ lB l.
Sets the period after which the link is reported as down. Note that the PHY may choose
the closest supported value. Only on reading back the tunable do you get the actual value.
.TE
+.TP
+.A2 energy-detect-power-down on off
+Specifies whether Energy Detect Power Down (EDPD) should be enabled (if supported).
+This will put the RX and TX circuit blocks into a low power mode, and the PHY will
+wake up periodically to send link pulses to avoid any lock-up situation with a peer
+PHY that may also have EDPD enabled. By default, this setting will also enable the
+periodic transmission of TX pulses.
+.TS
+nokeep;
+lB l.
+.BI tx-interval \ N
+ Some PHYs support configuration of the wake-up interval to send TX pulses.
+ This setting allows the control of this interval, and 0 disables TX pulses
+ if the PHY supports this. Disabling TX pulses can create a lock-up situation
+ where neither of the PHYs wakes the other one. If the PHY supports only
+ a single interval, any non-zero value will enable this.
+.TE
+.TP
.PD
.RE
.TP
@@ -1122,6 +1146,10 @@ Some PHYs support a Fast Link Down Feature and may allow configuration of the de
before a broken link is reported as being down.
Gets the PHY Fast Link Down status / period.
+.TP
+.B energy\-detect\-power\-down
+Gets the current configured setting for Energy Detect Power Down (if supported).
+
.RE
.TP
.B \-\-reset
diff --git a/ethtool.c b/ethtool.c
index c0e2903..c0a18f8 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -4897,6 +4897,30 @@ static int do_get_phy_tunable(struct cmd_context *ctx)
else
fprintf(stdout, "Fast Link Down enabled, %d msecs\n",
cont.msecs);
+ } else if (!strcmp(argp[0], "energy-detect-power-down")) {
+ struct {
+ struct ethtool_tunable ds;
+ u16 tx_interval;
+ } cont;
+
+ cont.ds.cmd = ETHTOOL_PHY_GTUNABLE;
+ cont.ds.id = ETHTOOL_PHY_EDPD;
+ cont.ds.type_id = ETHTOOL_TUNABLE_U16;
+ cont.ds.len = 2;
+ if (send_ioctl(ctx, &cont.ds) < 0) {
+ perror("Cannot Get PHY Energy Detect Power Down value");
+ return 87;
+ }
+
+ if (cont.tx_interval == ETHTOOL_PHY_EDPD_DISABLE)
+ fprintf(stdout, "Energy Detect Power Down: disabled\n");
+ else if (cont.tx_interval == ETHTOOL_PHY_EDPD_NO_TX)
+ fprintf(stdout,
+ "Energy Detect Power Down: enabled, TX disabled\n");
+ else
+ fprintf(stdout,
+ "Energy Detect Power Down: enabled, TX %u intervals\n",
+ cont.tx_interval);
} else {
exit_bad_args();
}
@@ -5018,7 +5042,8 @@ static int parse_named_bool(struct cmd_context *ctx, const char *name, u8 *on)
return 1;
}
-static int parse_named_u8(struct cmd_context *ctx, const char *name, u8 *val)
+static int parse_named_uint(struct cmd_context *ctx, const char *name,
+ void *val, enum tunable_type_id type_id)
{
if (ctx->argc < 2)
return 0;
@@ -5026,7 +5051,16 @@ static int parse_named_u8(struct cmd_context *ctx, const char *name, u8 *val)
if (strcmp(*ctx->argp, name))
return 0;
- *val = get_uint_range(*(ctx->argp + 1), 0, 0xff);
+ switch (type_id) {
+ case ETHTOOL_TUNABLE_U8:
+ *(u8 *)val = get_uint_range(*(ctx->argp + 1), 0, 0xff);
+ break;
+ case ETHTOOL_TUNABLE_U16:
+ *(u16 *)val = get_uint_range(*(ctx->argp + 1), 0, 0xffff);
+ break;
+ default:
+ return 0;
+ }
ctx->argc -= 2;
ctx->argp += 2;
@@ -5034,6 +5068,16 @@ static int parse_named_u8(struct cmd_context *ctx, const char *name, u8 *val)
return 1;
}
+static int parse_named_u8(struct cmd_context *ctx, const char *name, u8 *val)
+{
+ return parse_named_uint(ctx, name, val, ETHTOOL_TUNABLE_U8);
+}
+
+static int parse_named_u16(struct cmd_context *ctx, const char *name, u16 *val)
+{
+ return parse_named_uint(ctx, name, val, ETHTOOL_TUNABLE_U16);
+}
+
static int do_set_phy_tunable(struct cmd_context *ctx)
{
int err = 0;
@@ -5041,6 +5085,8 @@ static int do_set_phy_tunable(struct cmd_context *ctx)
u8 ds_changed = 0, ds_has_cnt = 0, ds_enable = 0;
u8 fld_changed = 0, fld_enable = 0;
u8 fld_msecs = ETHTOOL_PHY_FAST_LINK_DOWN_ON;
+ u8 edpd_changed = 0, edpd_enable = 0;
+ u16 edpd_tx_interval = ETHTOOL_PHY_EDPD_DFLT_TX_INTERVAL;
/* Parse arguments */
if (parse_named_bool(ctx, "downshift", &ds_enable)) {
@@ -5050,6 +5096,11 @@ static int do_set_phy_tunable(struct cmd_context *ctx)
fld_changed = 1;
if (fld_enable)
parse_named_u8(ctx, "msecs", &fld_msecs);
+ } else if (parse_named_bool(ctx, "energy-detect-power-down",
+ &edpd_enable)) {
+ edpd_changed = 1;
+ if (edpd_enable)
+ parse_named_u16(ctx, "tx-interval", &edpd_tx_interval);
} else {
exit_bad_args();
}
@@ -5074,6 +5125,16 @@ static int do_set_phy_tunable(struct cmd_context *ctx)
fld_msecs = ETHTOOL_PHY_FAST_LINK_DOWN_OFF;
else if (fld_msecs == ETHTOOL_PHY_FAST_LINK_DOWN_OFF)
exit_bad_args();
+ } else if (edpd_changed) {
+ if (!edpd_enable)
+ edpd_tx_interval = ETHTOOL_PHY_EDPD_DISABLE;
+ else if (edpd_tx_interval == 0)
+ edpd_tx_interval = ETHTOOL_PHY_EDPD_NO_TX;
+ else if (edpd_tx_interval > ETHTOOL_PHY_EDPD_DFLT_TX_INTERVAL) {
+ fprintf(stderr, "'tx-interval' max value is %d.\n",
+ (ETHTOOL_PHY_EDPD_DFLT_TX_INTERVAL - 1));
+ exit_bad_args();
+ }
}
/* Do it */
@@ -5109,6 +5170,22 @@ static int do_set_phy_tunable(struct cmd_context *ctx)
perror("Cannot Set PHY Fast Link Down value");
err = 87;
}
+ } else if (edpd_changed) {
+ struct {
+ struct ethtool_tunable fld;
+ u16 tx_interval;
+ } cont;
+
+ cont.fld.cmd = ETHTOOL_PHY_STUNABLE;
+ cont.fld.id = ETHTOOL_PHY_EDPD;
+ cont.fld.type_id = ETHTOOL_TUNABLE_U16;
+ cont.fld.len = 2;
+ cont.tx_interval = edpd_tx_interval;
+ err = send_ioctl(ctx, &cont.fld);
+ if (err < 0) {
+ perror("Cannot Set PHY Energy Detect Power Down");
+ err = 87;
+ }
}
return err;
@@ -5361,10 +5438,12 @@ static const struct option {
" [ tx-timer %d ]\n"},
{ "--set-phy-tunable", 1, do_set_phy_tunable, "Set PHY tunable",
" [ downshift on|off [count N] ]\n"
- " [ fast-link-down on|off [msecs N] ]\n"},
+ " [ fast-link-down on|off [msecs N] ]\n"
+ " [ energy-detect-power-down on|off [tx-interval N] ]\n"},
{ "--get-phy-tunable", 1, do_get_phy_tunable, "Get PHY tunable",
" [ downshift ]\n"
- " [ fast-link-down ]\n"},
+ " [ fast-link-down ]\n"
+ " [ energy-detect-power-down ]\n"},
{ "--reset", 1, do_reset, "Reset components",
" [ flags %x ]\n"
" [ mgmt ]\n"
--
2.20.1
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 <[email protected]>
---
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
On Tue, Sep 03, 2019 at 07:06:22PM +0300, Alexandru Ardelean wrote:
> This patch series is actually 2 series in 1.
>
> First 2 patches implement the kernel support for controlling Energy Detect
> Powerdown support via phy-tunable, and the next 2 patches implement the
> ethtool user-space control.
> Hopefully, this combination of 2 series is an acceptable approach; if not,
> I am fine to re-update it based on feedback.
I understand your reasoning, but do keep in mind that userland ethtool
and the kernel are managed in different git trees. Seperate patchsets
would be preferable in general, although in some cases having an
initial userland implementation to show against proposed kernel
changes could be helpful.
It would not be unusual for someone to ask for changes on the kernel
patches. If that happens, just repost the kernel changes until you get
a final merge. Once that happens, then repost the userland patches as
a seperate patchset. But I'll keep an eye here -- if Dave merges the
existing kernel patches as-is, I can take the already posted patchs
(unless problems are found in code review).
John
> 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.
>
> --
> 2.20.1
>
>
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.
From: Alexandru Ardelean <[email protected]>
Date: Tue, 3 Sep 2019 19:06:22 +0300
> First 2 patches implement the kernel support for controlling Energy Detect
> Powerdown support via phy-tunable, and the next 2 patches implement the
> ethtool user-space control.
You should do this as two separate patch series, one for the kernel
portion and one for the ethtool part.