diff mbox series

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

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

Commit Message

Alexandru Ardelean Sept. 12, 2019, 4:28 p.m. UTC 
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(+)

Comments

Andrew Lunn Sept. 14, 2019, 3:29 p.m. UTC | #1 
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
Florian Fainelli Sept. 15, 2019, 3:11 p.m. UTC | #2 
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>
Alexandru Ardelean Sept. 16, 2019, 6:44 a.m. UTC | #3 
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 mbox series

Patch

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));