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

2019-09-16 Thread Ardelean, Alexandru 
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

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

2019-09-15 Thread Florian Fainelli 



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 
-- 
Florian

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

2019-09-14 Thread Andrew Lunn 
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

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

2019-09-12 Thread Alexandru Ardelean 
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 
---
 drivers/net/phy/adin.c | 61 ++
 1 file changed, 61 insertions(+)

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_CNT0x0014
 
+#define ADIN1300_PHY_CTRL_STATUS2  0x0015
+#define   ADIN1300_NRG_PD_EN   BIT(3)
+#define   ADIN1300_NRG_PD_TX_ENBIT(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));
 
-- 
2.20.1