Message ID | 20190912162812.402-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 Thu, Sep 12, 2019 at 07:28:12PM +0300, Alexandru Ardelean wrote: > +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; > + > + switch (tx_interval) { > + case 1000: /* 1 second */ > + /* fallthrough */ > + case ETHTOOL_PHY_EDPD_DFLT_TX_MSECS: > + val |= ADIN1300_NRG_PD_TX_EN; > + /* fallthrough */ > + case ETHTOOL_PHY_EDPD_NO_TX: > + break; > + default: > + return -EINVAL; > + } > + > + return phy_modify(phydev, ADIN1300_PHY_CTRL_STATUS2, > + (ADIN1300_NRG_PD_EN | ADIN1300_NRG_PD_TX_EN), > + val); > +} > + > > + rc = adin_set_edpd(phydev, 1); > + if (rc < 0) > + return rc; Hi Alexandru Shouldn't this be adin_set_edpd(phydev, 1000); Andrew
On 9/14/2019 8:29 AM, Andrew Lunn wrote: > On Thu, Sep 12, 2019 at 07:28:12PM +0300, Alexandru Ardelean wrote: > >> +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; >> + >> + switch (tx_interval) { >> + case 1000: /* 1 second */ >> + /* fallthrough */ >> + case ETHTOOL_PHY_EDPD_DFLT_TX_MSECS: >> + val |= ADIN1300_NRG_PD_TX_EN; >> + /* fallthrough */ >> + case ETHTOOL_PHY_EDPD_NO_TX: >> + break; >> + default: >> + return -EINVAL; >> + } >> + >> + return phy_modify(phydev, ADIN1300_PHY_CTRL_STATUS2, >> + (ADIN1300_NRG_PD_EN | ADIN1300_NRG_PD_TX_EN), >> + val); >> +} >> + > >> >> + rc = adin_set_edpd(phydev, 1); >> + if (rc < 0) >> + return rc; > > Hi Alexandru > > Shouldn't this be adin_set_edpd(phydev, 1000); That does sound like the intended use, or use ETHTOOL_PHY_EDPD_DFLT_TX_MSECS, with that fixed: Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
On Sun, 2019-09-15 at 08:11 -0700, Florian Fainelli wrote: > [External] > > > > On 9/14/2019 8:29 AM, Andrew Lunn wrote: > > On Thu, Sep 12, 2019 at 07:28:12PM +0300, Alexandru Ardelean wrote: > > > > > +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; > > > + > > > + switch (tx_interval) { > > > + case 1000: /* 1 second */ > > > + /* fallthrough */ > > > + case ETHTOOL_PHY_EDPD_DFLT_TX_MSECS: > > > + val |= ADIN1300_NRG_PD_TX_EN; > > > + /* fallthrough */ > > > + case ETHTOOL_PHY_EDPD_NO_TX: > > > + break; > > > + default: > > > + return -EINVAL; > > > + } > > > + > > > + return phy_modify(phydev, ADIN1300_PHY_CTRL_STATUS2, > > > + (ADIN1300_NRG_PD_EN | ADIN1300_NRG_PD_TX_EN), > > > + val); > > > +} > > > + > > > > > > + rc = adin_set_edpd(phydev, 1); > > > + if (rc < 0) > > > + return rc; > > > > Hi Alexandru > > > > Shouldn't this be adin_set_edpd(phydev, 1000); > > That does sound like the intended use, or use > ETHTOOL_PHY_EDPD_DFLT_TX_MSECS, with that fixed: Ack. Many thanks for catching this. I missed it when re-spinning. > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c index 4dec83df048d..9afeee67675b 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,62 @@ 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) + /* default is 1 second */ + *tx_interval = ETHTOOL_PHY_EDPD_DFLT_TX_MSECS; + 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; + + switch (tx_interval) { + case 1000: /* 1 second */ + /* fallthrough */ + case ETHTOOL_PHY_EDPD_DFLT_TX_MSECS: + val |= ADIN1300_NRG_PD_TX_EN; + /* fallthrough */ + case ETHTOOL_PHY_EDPD_NO_TX: + break; + default: + return -EINVAL; + } + + 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 +400,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 +425,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. The ADIN PHY supports only fixed 1 second intervals; they cannot be configured. That is why the acceptable values are 1, ETHTOOL_PHY_EDPD_DFLT_TX_MSECS and ETHTOOL_PHY_EDPD_NO_TX (which disables TX pulses). Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> --- drivers/net/phy/adin.c | 61 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+)