| Message ID | 20190904162322.17542-2-alexandru.ardelean@analog.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | ethtool: implement Energy Detect Powerdown support via phy-tunable | expand |
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
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
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?
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; }
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(+)