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.ardel...@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
