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

From: Voon Weifeng <redacted>

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 <redacted>
Signed-off-by: Ling PeiLee <redacted>
---
 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

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!

On Wed, 23 Jun 2021 21:09:29 +0800
Ling Pei Lee [off-list ref] 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

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 [off-list ref] wrote:

quoted

+		/* 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

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:

quoted

+static void mv2110_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
+{
+	int ret = 0;

This initialiser doesn't do anything.

quoted

+
+	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?)

quoted

+		/* 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...

quoted

+	}
+
+	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 

On Wed, Jun 23, 2021 at 09:09:29PM +0800, Ling Pei Lee wrote:

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!