[PATCH net-next] net: phy: marvell10g: enable WoL for mv2110

[PATCH net-next] net: phy: marvell10g: enable WoL for mv2110

[Ling Pei Lee]

From: Voon Weifeng <weifeng.voon@intel.com>

Basically it is just to enable to WoL interrupt and enable WoL detection.
Then, configure the MAC address into address detection register.

Signed-off-by: Voon Weifeng <weifeng.voon@intel.com>
Signed-off-by: Ling PeiLee <pei.lee.ling@intel.com>
---
 drivers/net/phy/marvell10g.c | 102 +++++++++++++++++++++++++++++++++++
 1 file changed, 102 insertions(+)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index bbbc6ac8fa82..93410ece83af 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -28,6 +28,7 @@
 #include <linux/marvell_phy.h>
 #include <linux/phy.h>
 #include <linux/sfp.h>
+#include <linux/netdevice.h>
 
 #define MV_PHY_ALASKA_NBT_QUIRK_MASK	0xfffffffe
 #define MV_PHY_ALASKA_NBT_QUIRK_REV	(MARVELL_PHY_ID_88X3310 | 0xa)
@@ -106,6 +107,17 @@ enum {
 	MV_V2_TEMP_CTRL_DISABLE	= 0xc000,
 	MV_V2_TEMP		= 0xf08c,
 	MV_V2_TEMP_UNKNOWN	= 0x9600, /* unknown function */
+	MV_V2_MAGIC_PKT_WORD0	= 0xf06b,
+	MV_V2_MAGIC_PKT_WORD1	= 0xf06c,
+	MV_V2_MAGIC_PKT_WORD2	= 0xf06d,
+	/* Wake on LAN registers */
+	MV_V2_WOL_CTRL		= 0xf06e,
+	MV_V2_WOL_STS		= 0xf06f,
+	MV_V2_WOL_CLEAR_STS	= BIT(15),
+	MV_V2_WOL_MAGIC_PKT_EN	= BIT(0),
+	MV_V2_PORT_INTR_STS	= 0xf040,
+	MV_V2_PORT_INTR_MASK	= 0xf043,
+	MV_V2_WOL_INTR_EN	= BIT(8),
 };
 
 struct mv3310_chip {
@@ -991,6 +1003,94 @@ static int mv2111_match_phy_device(struct phy_device *phydev)
 	return mv211x_match_phy_device(phydev, false);
 }
 
+static void mv2110_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
+{
+	int ret = 0;
+
+	wol->supported = WAKE_MAGIC;
+	wol->wolopts = 0;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, MV_V2_WOL_CTRL);
+
+	if (ret & MV_V2_WOL_MAGIC_PKT_EN)
+		wol->wolopts |= WAKE_MAGIC;
+}
+
+static int mv2110_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
+{
+	int ret = 0;
+
+	if (wol->wolopts & WAKE_MAGIC) {
+		/* Enable the WOL interrupt */
+		ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND2,
+				       MV_V2_PORT_INTR_MASK,
+				       MV_V2_WOL_INTR_EN);
+
+		if (ret < 0)
+			return ret;
+
+		/* Store the device address for the magic packet */
+		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
+				    MV_V2_MAGIC_PKT_WORD2,
+				    ((phydev->attached_dev->dev_addr[5] << 8) |
+				    phydev->attached_dev->dev_addr[4]));
+
+		if (ret < 0)
+			return ret;
+
+		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
+				    MV_V2_MAGIC_PKT_WORD1,
+				    ((phydev->attached_dev->dev_addr[3] << 8) |
+				    phydev->attached_dev->dev_addr[2]));
+
+		if (ret < 0)
+			return ret;
+
+		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
+				    MV_V2_MAGIC_PKT_WORD0,
+				    ((phydev->attached_dev->dev_addr[1] << 8) |
+				    phydev->attached_dev->dev_addr[0]));
+
+		if (ret < 0)
+			return ret;
+
+		/* Clear WOL status and enable magic packet matching */
+		ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND2,
+				       MV_V2_WOL_CTRL,
+				       MV_V2_WOL_MAGIC_PKT_EN |
+				       MV_V2_WOL_CLEAR_STS);
+
+		if (ret < 0)
+			return ret;
+
+		/* Reset the clear WOL status bit as it does not self-clear */
+		ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2,
+					 MV_V2_WOL_CTRL,
+					 MV_V2_WOL_CLEAR_STS);
+
+		if (ret < 0)
+			return ret;
+	} else {
+		/* Disable magic packet matching & reset WOL status bit */
+		ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2,
+				     MV_V2_WOL_CTRL,
+				     MV_V2_WOL_MAGIC_PKT_EN,
+				     MV_V2_WOL_CLEAR_STS);
+
+		if (ret < 0)
+			return ret;
+
+		ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2,
+					 MV_V2_WOL_CTRL,
+					 MV_V2_WOL_CLEAR_STS);
+
+		if (ret < 0)
+			return ret;
+	}
+
+	return ret;
+}
+
 static struct phy_driver mv3310_drivers[] = {
 	{
 		.phy_id		= MARVELL_PHY_ID_88X3310,
@@ -1045,6 +1145,8 @@ static struct phy_driver mv3310_drivers[] = {
 		.set_tunable	= mv3310_set_tunable,
 		.remove		= mv3310_remove,
 		.set_loopback	= genphy_c45_loopback,
+		.get_wol	= mv2110_get_wol,
+		.set_wol	= mv2110_set_wol,
 	},
 	{
 		.phy_id		= MARVELL_PHY_ID_88E2110,
-- 
2.25.1

Re: [PATCH net-next] net: phy: marvell10g: enable WoL for mv2110

[Russell King (Oracle)]

On Wed, Jun 23, 2021 at 09:09:29PM +0800, Ling Pei Lee wrote:
> +static void mv2110_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
> +{
> +	int ret = 0;

This initialiser doesn't do anything.

> +
> +	wol->supported = WAKE_MAGIC;
> +	wol->wolopts = 0;
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, MV_V2_WOL_CTRL);
> +
> +	if (ret & MV_V2_WOL_MAGIC_PKT_EN)
> +		wol->wolopts |= WAKE_MAGIC;

You need to check whether "ret" is a negative number - if phy_read_mmd()
returns an error, this test could be true or false. It would be better
to have well defined behaviour (e.g. reporting that WOL is disabled?)

> +		/* Reset the clear WOL status bit as it does not self-clear */
> +		ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2,
> +					 MV_V2_WOL_CTRL,
> +					 MV_V2_WOL_CLEAR_STS);
> +
> +		if (ret < 0)
> +			return ret;
> +	} else {
> +		/* Disable magic packet matching & reset WOL status bit */
> +		ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2,
> +				     MV_V2_WOL_CTRL,
> +				     MV_V2_WOL_MAGIC_PKT_EN,
> +				     MV_V2_WOL_CLEAR_STS);
> +
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2,
> +					 MV_V2_WOL_CTRL,
> +					 MV_V2_WOL_CLEAR_STS);
> +
> +		if (ret < 0)
> +			return ret;

This phy_clear_bits_mmd() is the same as the tail end of the other part
of the if() clause. Consider moving it after the if () { } else { }
statement...

> +	}
> +
> +	return ret;

and as all paths return "ret" just do:

	return phy_clear_bits_mmd(...

I will also need to check whether this is the same as the 88x3310, but
I'm afraid I don't have the energy this evening - please email me a
remind to look at this tomorrow. Thanks.

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

Re: [PATCH net-next] net: phy: marvell10g: enable WoL for mv2110

[Marek Behun]

On Wed, 23 Jun 2021 21:09:29 +0800
Ling Pei Lee <pei.lee.ling@intel.com> wrote:

> +		/* Enable the WOL interrupt */
> +		ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND2,
> +				       MV_V2_PORT_INTR_MASK,
> +				       MV_V2_WOL_INTR_EN);
> +
> +		if (ret < 0)
> +			return ret;

Hi, in addition to what Russell said, please remove the extra newline
between function call and return value check, i.e. instead of
  ret = phy_xyz(...);

  if (ret)
     return ret;

  ret = phy_xyz(...);

  if (ret)
     return ret;

do
  ret = phy_xyz(...);
  if (ret)
     return ret;

  ret = phy_xyz(...);
  if (ret)
     return ret;

This is how this driver does this everywhere else.

Do you have a device that uses this WoL feature?

Marek

Re: [PATCH net-next] net: phy: marvell10g: enable WoL for mv2110

[Wong Vee Khee]

Hi Marek,

On Wed, Jun 23, 2021 at 11:38:54PM +0200, Marek Behun wrote:
> On Wed, 23 Jun 2021 21:09:29 +0800
> Ling Pei Lee <pei.lee.ling@intel.com> wrote:
> 
> > +		/* Enable the WOL interrupt */
> > +		ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND2,
> > +				       MV_V2_PORT_INTR_MASK,
> > +				       MV_V2_WOL_INTR_EN);
> > +
> > +		if (ret < 0)
> > +			return ret;
> 
> Hi, in addition to what Russell said, please remove the extra newline
> between function call and return value check, i.e. instead of
>   ret = phy_xyz(...);
> 
>   if (ret)
>      return ret;
> 
>   ret = phy_xyz(...);
> 
>   if (ret)
>      return ret;
> 
> do
>   ret = phy_xyz(...);
>   if (ret)
>      return ret;
> 
>   ret = phy_xyz(...);
>   if (ret)
>      return ret;
> 
> This is how this driver does this everywhere else.
> 
> Do you have a device that uses this WoL feature?
>

Yes. We have Intel Elkhart Lake platform with Integrated Sypnosys MAC
controller(STMMAC) paired with External PHY device (in this case the
Marvell Alaska 88E2110).

BR,
 VK

Re: [PATCH net-next] net: phy: marvell10g: enable WoL for mv2110

[Wong Vee Khee]

Hi Russell,

On Wed, Jun 23, 2021 at 09:06:18PM +0100, Russell King (Oracle) wrote:
> On Wed, Jun 23, 2021 at 09:09:29PM +0800, Ling Pei Lee wrote:
> > +static void mv2110_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
> > +{
> > +	int ret = 0;
> 
> This initialiser doesn't do anything.
> 
> > +
> > +	wol->supported = WAKE_MAGIC;
> > +	wol->wolopts = 0;
> > +
> > +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, MV_V2_WOL_CTRL);
> > +
> > +	if (ret & MV_V2_WOL_MAGIC_PKT_EN)
> > +		wol->wolopts |= WAKE_MAGIC;
> 
> You need to check whether "ret" is a negative number - if phy_read_mmd()
> returns an error, this test could be true or false. It would be better
> to have well defined behaviour (e.g. reporting that WOL is disabled?)
> 
> > +		/* Reset the clear WOL status bit as it does not self-clear */
> > +		ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2,
> > +					 MV_V2_WOL_CTRL,
> > +					 MV_V2_WOL_CLEAR_STS);
> > +
> > +		if (ret < 0)
> > +			return ret;
> > +	} else {
> > +		/* Disable magic packet matching & reset WOL status bit */
> > +		ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2,
> > +				     MV_V2_WOL_CTRL,
> > +				     MV_V2_WOL_MAGIC_PKT_EN,
> > +				     MV_V2_WOL_CLEAR_STS);
> > +
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2,
> > +					 MV_V2_WOL_CTRL,
> > +					 MV_V2_WOL_CLEAR_STS);
> > +
> > +		if (ret < 0)
> > +			return ret;
> 
> This phy_clear_bits_mmd() is the same as the tail end of the other part
> of the if() clause. Consider moving it after the if () { } else { }
> statement...
> 
> > +	}
> > +
> > +	return ret;
> 
> and as all paths return "ret" just do:
> 
> 	return phy_clear_bits_mmd(...
> 
> I will also need to check whether this is the same as the 88x3310, but
> I'm afraid I don't have the energy this evening - please email me a
> remind to look at this tomorrow. Thanks.
>

Shall we add this for the 88x3310 as well?

BR,
 VK 

Re: [PATCH net-next] net: phy: marvell10g: enable WoL for mv2110

[Russell King (Oracle)]

On Wed, Jun 23, 2021 at 09:09:29PM +0800, Ling Pei Lee wrote:
> @@ -106,6 +107,17 @@ enum {
>  	MV_V2_TEMP_CTRL_DISABLE	= 0xc000,
>  	MV_V2_TEMP		= 0xf08c,
>  	MV_V2_TEMP_UNKNOWN	= 0x9600, /* unknown function */
> +	MV_V2_MAGIC_PKT_WORD0	= 0xf06b,
> +	MV_V2_MAGIC_PKT_WORD1	= 0xf06c,
> +	MV_V2_MAGIC_PKT_WORD2	= 0xf06d,
> +	/* Wake on LAN registers */
> +	MV_V2_WOL_CTRL		= 0xf06e,
> +	MV_V2_WOL_STS		= 0xf06f,
> +	MV_V2_WOL_CLEAR_STS	= BIT(15),
> +	MV_V2_WOL_MAGIC_PKT_EN	= BIT(0),
> +	MV_V2_PORT_INTR_STS	= 0xf040,
> +	MV_V2_PORT_INTR_MASK	= 0xf043,
> +	MV_V2_WOL_INTR_EN	= BIT(8),

Please put these new register definitions in address order. This list is
first sorted by MMD and then by address. So these should be before the
definition of MV_V2_TEMP_CTRL.

As I suspected, the 88x3310 shares this same register layout for the WOL
and at least bit 8 of the interrupt status and enable registers.

Thanks, and thanks for reminding me to look at this today!

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