diff mbox series

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

Message ID 20210623130929.805559-1-pei.lee.ling@intel.com
State New
Headers show
Series [net-next] net: phy: marvell10g: enable WoL for mv2110 | expand

Commit Message

Ling Pei Lee June 23, 2021, 1:09 p.m. UTC 
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(+)

Comments

Russell King (Oracle) June 23, 2021, 8:06 p.m. UTC | #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.
Marek Behún June 23, 2021, 9:38 p.m. UTC | #2 
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
Wong Vee Khee June 24, 2021, 2:56 a.m. UTC | #3 
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
Wong Vee Khee June 24, 2021, 3:27 p.m. UTC | #4 
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
Russell King (Oracle) June 24, 2021, 3:34 p.m. UTC | #5 
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!
diff mbox series

Patch

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,