| Message ID | 20190904162322.17542-3-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: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
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
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));
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(+)