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

[PATCH net-next V2] 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.

Change Log:
 V2:
 (1) Reviewer Marek request to rorganize code to readable way.
 (2) Reviewer Rusell request to put phy_clear_bits_mmd() outside of if(){}else{}
     and modify return ret to return phy_clear_bits_mmd().
 (3) Reviewer Rusell request to add return on phy_read_mmd() in set_wol().
 (4) Reorganize register layout to be put before MV_V2_TEMP_CTRL.
 (5) Add the .{get|set}_wol for 88E3110 too as per feedback from Russell.

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

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index bbbc6ac8fa82..5bddf4682127 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)
@@ -99,6 +100,17 @@ enum {
 	MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER_NO_SGMII_AN	= 0x5,
 	MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER_RATE_MATCH	= 0x6,
 	MV_V2_33X0_PORT_CTRL_MACTYPE_USXGMII			= 0x7,
+	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),
 	/* Temperature control/read registers (88X3310 only) */
 	MV_V2_TEMP_CTRL		= 0xf08a,
 	MV_V2_TEMP_CTRL_MASK	= 0xc000,
@@ -991,6 +1003,78 @@ 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;
+
+	wol->supported = WAKE_MAGIC;
+	wol->wolopts = 0;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, MV_V2_WOL_CTRL);
+	if (ret < 0)
+		return;
+
+	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;
+
+	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;
+	} 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;
+	}
+
+	/* Reset the clear WOL status bit as it does not self-clear */
+	return phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2,
+					MV_V2_WOL_CTRL,
+					MV_V2_WOL_CLEAR_STS);
+}
+
 static struct phy_driver mv3310_drivers[] = {
 	{
 		.phy_id		= MARVELL_PHY_ID_88X3310,
@@ -1009,6 +1093,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_88X3340,
@@ -1045,6 +1131,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 V2] net: phy: marvell10g: enable WoL for mv2110

[Russell King (Oracle)]

On Tue, Jun 29, 2021 at 06:55:54PM +0800, Ling Pei Lee wrote:
> 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.
> 
> Change Log:
>  V2:
>  (1) Reviewer Marek request to rorganize code to readable way.
>  (2) Reviewer Rusell request to put phy_clear_bits_mmd() outside of if(){}else{}
>      and modify return ret to return phy_clear_bits_mmd().
>  (3) Reviewer Rusell request to add return on phy_read_mmd() in set_wol().
>  (4) Reorganize register layout to be put before MV_V2_TEMP_CTRL.

I actually said:

"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."

Which you have partially done.

> @@ -99,6 +100,17 @@ enum {
>  	MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER_NO_SGMII_AN	= 0x5,
>  	MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER_RATE_MATCH	= 0x6,
>  	MV_V2_33X0_PORT_CTRL_MACTYPE_USXGMII			= 0x7,
> +	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),

Clearly MV_V2_PORT_INTR_STS is at a lower address than
MV_V2_MAGIC_PKT_WORD0, so the list is not sorted as it originally was.

Apart from that, the rest of the patch looks good, 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 V2] net: phy: marvell10g: enable WoL for mv2110

[Marek Behún]

Hi Ling,

some more things, please look at these.

First, the subject line should be
  net: phy: marvell10g: enable WoL for 88X3310 and 88E2110
since you are also implementing it for 3310.

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

"Implement Wake-on-LAN feature for 88X3310 and 88E2110.

 This is done by enabling WoL interrupt and WoL detection and
 configuring MAC address into WoL magic packet registers."

> Change Log:
>  V2:
>  (1) Reviewer Marek request to rorganize code to readable way.
>  (2) Reviewer Rusell request to put phy_clear_bits_mmd() outside of
> if(){}else{} and modify return ret to return phy_clear_bits_mmd().
>  (3) Reviewer Rusell request to add return on phy_read_mmd() in
> set_wol(). (4) Reorganize register layout to be put before
> MV_V2_TEMP_CTRL. (5) Add the .{get|set}_wol for 88E3110 too as per
> feedback from Russell.

We do not put changelogs into the commit message normally. Mostly we
put it into cover letters or after the "--" line so that it is not
included in the commit message in git history (merge commits are one
of the exceptions).

As Russell wrote, you have only partially reorganized register
constants. The constants should be defined in this order
+	MV_V2_PORT_INTR_STS	= 0xf040,
+	MV_V2_PORT_INTR_MASK	= 0xf043,
+	MV_V2_WOL_INTR_EN	= BIT(8),
+	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_CLEAR_STS	= BIT(15),
+	MV_V2_WOL_MAGIC_PKT_EN	= BIT(0),
+	MV_V2_WOL_STS		= 0xf06f,

Also:
- MV_V2_WOL_STS is not used, you should read the register before
  cleaning or don't define the constant at all. (IMO you should read
  it).
- register value constants should be prefixed with whole register names,
  so:
    MV_V2_WOL_INTR_EN  rename to  MV_V2_PORT_INTR_STS_WOL_EN
    MV_V2_WOL_CLEAR_STS  rename to  MV_V2_WOL_CTRL_CLEAR_STS
    MV_V2_WOL_MAGIC_PKT_EN  rename to  MV_V2_WOL_CTRL_MAGIC_PKT_EN

+	/* Reset the clear WOL status bit as it does not self-clear */
+	return phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2,
+					MV_V2_WOL_CTRL,
+					MV_V2_WOL_CLEAR_STS);

This has wrong indentation, the arguments in new lines should start at
the position of first argument.

Marek