Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 14 Aug 2021 18:26:56 +0100
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Song Yoong Siang <yoong.siang.song@intel.com>
Cc:     Marek =?iso-8859-1?Q?Beh=FAn?= <kabel@kernel.org>,
        Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        "David S . Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next 1/1] net: phy: marvell10g: Add WAKE_PHY support
 to WOL event
Message-ID: <20210814172656.GA22278@shell.armlinux.org.uk>
References: <20210813084536.182381-1-yoong.siang.song@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210813084536.182381-1-yoong.siang.song@intel.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Fri, Aug 13, 2021 at 04:45:36PM +0800, Song Yoong Siang wrote:
> Add Wake-on-PHY feature support by enabling the Link Status Changed
> interrupt.
> 
> Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com>
> ---
>  drivers/net/phy/marvell10g.c | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> index 0b7cae118ad7..d46761c225f0 100644
> --- a/drivers/net/phy/marvell10g.c
> +++ b/drivers/net/phy/marvell10g.c
> @@ -76,6 +76,11 @@ enum {
>  	MV_PCS_CSSR1_SPD2_2500	= 0x0004,
>  	MV_PCS_CSSR1_SPD2_10000	= 0x0000,
>  
> +	/* Copper Specific Interrupt registers */
> +	MV_PCS_INTR_ENABLE	= 0x8010,
> +	MV_PCS_INTR_ENABLE_LSC	= BIT(10),
> +	MV_PCS_INTR_STS		= 0x8011,
> +
>  	/* Temperature read register (88E2110 only) */
>  	MV_PCS_TEMP		= 0x8042,
>  
> @@ -1036,7 +1041,7 @@ static void mv3110_get_wol(struct phy_device *phydev,
>  {
>  	int ret;
>  
> -	wol->supported = WAKE_MAGIC;
> +	wol->supported = WAKE_MAGIC | WAKE_PHY;
>  	wol->wolopts = 0;
>  
>  	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, MV_V2_WOL_CTRL);
> @@ -1045,6 +1050,13 @@ static void mv3110_get_wol(struct phy_device *phydev,
>  
>  	if (ret & MV_V2_WOL_CTRL_MAGIC_PKT_EN)
>  		wol->wolopts |= WAKE_MAGIC;
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_INTR_ENABLE);
> +	if (ret < 0)
> +		return;
> +
> +	if (ret & MV_PCS_INTR_ENABLE_LSC)
> +		wol->wolopts |= WAKE_PHY;
>  }
>  
>  static int mv3110_set_wol(struct phy_device *phydev,
> @@ -1099,6 +1111,25 @@ static int mv3110_set_wol(struct phy_device *phydev,
>  			return ret;
>  	}
>  
> +	if (wol->wolopts & WAKE_PHY) {
> +		/* Enable the link status changed interrupt */
> +		ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS,
> +				       MV_PCS_INTR_ENABLE,
> +				       MV_PCS_INTR_ENABLE_LSC);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* Clear the interrupt status register */
> +		ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_INTR_STS);
> +	} else {
> +		/* Disable the link status changed interrupt */
> +		ret = phy_clear_bits_mmd(phydev, MDIO_MMD_PCS,
> +					 MV_PCS_INTR_ENABLE,
> +					 MV_PCS_INTR_ENABLE_LSC);
> +		if (ret < 0)
> +			return ret;
> +	}
> +

How does this work if the driver has no interrupt support? What is
the hardware setup this has been tested with?

What if we later want to add interrupt support to this driver to
support detecting changes in link state - isn't using this bit
in the interrupt enable register going to confict with that?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!