[PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY

[PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY

[Revanth Kumar Uppala]

From: Narayan Reddy <narayanr@nvidia.com>

Enable flow control support using pause frames in aquantia phy driver.

Signed-off-by: Narayan Reddy <narayanr@nvidia.com>
Signed-off-by: Revanth Kumar Uppala <ruppala@nvidia.com>
---
 drivers/net/phy/aquantia_main.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
index 334a6904ca5a..c99b9d066463 100644
--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c
@@ -26,6 +26,8 @@
 #define PHY_ID_AQR412	0x03a1b712
 #define PHY_ID_AQR113C	0x31c31c12
 
+#define MDIO_AN_ADVT		0x0010
+
 #define MDIO_PHYXS_VEND_IF_STATUS		0xe812
 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_MASK	GENMASK(7, 3)
 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_KR	0
@@ -583,6 +585,17 @@ static int aqr107_config_init(struct phy_device *phydev)
 	if (!ret)
 		aqr107_chip_info(phydev);
 
+	/* Advertize flow control */
+	linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev->supported);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev->supported);
+	linkmode_copy(phydev->advertising, phydev->supported);
+
+	/* Configure flow control */
+	ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN, MDIO_AN_ADVT, ADVERTISE_PAUSE_CAP |
+			       ADVERTISE_PAUSE_ASYM);
+	if (ret < 0)
+		return ret;
+
 	return aqr107_set_downshift(phydev, MDIO_AN_VEND_PROV_DOWNSHIFT_DFLT);
 }
 
-- 
2.17.1

[PATCH 2/4] net: phy: aquantia: Enable MAC Controlled EEE

[Revanth Kumar Uppala]

Enable MAC controlled energy efficient ethernet (EEE) so that MAC can
keep the PHY in EEE sleep mode when link utilization is low to reduce
energy consumption.

Signed-off-by: Narayan Reddy <narayanr@nvidia.com>
Signed-off-by: Revanth Kumar Uppala <ruppala@nvidia.com>
---
 drivers/net/phy/aquantia_main.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
index c99b9d066463..faca2a0b1d49 100644
--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c
@@ -28,6 +28,9 @@
 
 #define MDIO_AN_ADVT		0x0010
 
+#define VEND1_SEC_INGRESS_CNTRL_REG1    0x7001
+#define MAC_CNTRL_EEE		(BIT(8) | BIT(12))
+
 #define MDIO_PHYXS_VEND_IF_STATUS		0xe812
 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_MASK	GENMASK(7, 3)
 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_KR	0
@@ -596,6 +599,12 @@ static int aqr107_config_init(struct phy_device *phydev)
 	if (ret < 0)
 		return ret;
 
+	/* Enable MAC Controlled EEE */
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
+			    VEND1_SEC_INGRESS_CNTRL_REG1, MAC_CNTRL_EEE);
+	if (ret < 0)
+		return ret;
+
 	return aqr107_set_downshift(phydev, MDIO_AN_VEND_PROV_DOWNSHIFT_DFLT);
 }
 
-- 
2.17.1

Re: [PATCH 2/4] net: phy: aquantia: Enable MAC Controlled EEE

[Andrew Lunn]

On Wed, Jun 28, 2023 at 06:13:24PM +0530, Revanth Kumar Uppala wrote:
> Enable MAC controlled energy efficient ethernet (EEE) so that MAC can
> keep the PHY in EEE sleep mode when link utilization is low to reduce
> energy consumption.

This needs more explanation. Is this 'SmartEEE', in that the PHY is
doing EEE without the SoC MAC being involved?

Ideally, you should only do SmartEEE, if the SoC MAC is dumb and does
not have EEE itself. I guess if you are doing rate adaptation, or
MACSEC in the PHY, then you might be forced to use SmartEEE since the
SoC MAC is somewhat decoupled from the PHY.

At the moment, we don't have a good story for SmartEEE. It should be
configured in the same way as normal EEE, ethtool --set-eee etc. I've
got a rewrite of normal EEE in the works. Once that is merged i hope
SmartEEE will be next.

    Andrew

RE: [PATCH 2/4] net: phy: aquantia: Enable MAC Controlled EEE

[Revanth Kumar Uppala]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Wednesday, June 28, 2023 7:24 PM
> To: Revanth Kumar Uppala <ruppala@nvidia.com>
> Cc: linux@armlinux.org.uk; hkallweit1@gmail.com; netdev@vger.kernel.org;
> linux-tegra@vger.kernel.org; Narayan Reddy <narayanr@nvidia.com>
> Subject: Re: [PATCH 2/4] net: phy: aquantia: Enable MAC Controlled EEE
> 
> External email: Use caution opening links or attachments
> 
> 
> On Wed, Jun 28, 2023 at 06:13:24PM +0530, Revanth Kumar Uppala wrote:
> > Enable MAC controlled energy efficient ethernet (EEE) so that MAC can
> > keep the PHY in EEE sleep mode when link utilization is low to reduce
> > energy consumption.
> 
> This needs more explanation. Is this 'SmartEEE', in that the PHY is doing EEE
> without the SoC MAC being involved?
No, this is not Smart EEE.
> 
> Ideally, you should only do SmartEEE, if the SoC MAC is dumb and does not have
> EEE itself. I guess if you are doing rate adaptation, or MACSEC in the PHY, then
> you might be forced to use SmartEEE since the SoC MAC is somewhat decoupled
> from the PHY.
> 
> At the moment, we don't have a good story for SmartEEE. It should be
> configured in the same way as normal EEE, ethtool --set-eee etc. I've got a
> rewrite of normal EEE in the works. Once that is merged i hope SmartEEE will be
> next.
"ethtool --set-eee" is a dynamic way of enabling normal EEE and here we are doing the same normal EEE but configuring it by default in aqr107_config_init() instead of doing it dynamically.
So, is there any concern for this?
Thanks,
Revanth Uppala
> 
>     Andrew

Re: [PATCH 2/4] net: phy: aquantia: Enable MAC Controlled EEE

[Russell King (Oracle)]

On Mon, Jul 24, 2023 at 11:29:28AM +0000, Revanth Kumar Uppala wrote:
> > Ideally, you should only do SmartEEE, if the SoC MAC is dumb and does not have
> > EEE itself. I guess if you are doing rate adaptation, or MACSEC in the PHY, then
> > you might be forced to use SmartEEE since the SoC MAC is somewhat decoupled
> > from the PHY.
> > 
> > At the moment, we don't have a good story for SmartEEE. It should be
> > configured in the same way as normal EEE, ethtool --set-eee etc. I've got a
> > rewrite of normal EEE in the works. Once that is merged i hope SmartEEE will be
> > next.
> "ethtool --set-eee" is a dynamic way of enabling normal EEE and here we are doing the same normal EEE but configuring it by default in aqr107_config_init() instead of doing it dynamically.

So, setting the MAC_CNTRL_EEE bits is just enabling the standard IEEE
paths in the PHY to allow the IEEE defined EEE architecture to work?

If that's all its doing, I wonder why they aren't set by default...
seems rather strange.

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

[PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side

[Revanth Kumar Uppala]

Lane bring-up failures are seen during interface up, as interface
speed settings are configured while the PHY is still initializing.

So, poll until PHY system side interface gets ready and the interface
speed settings are configured.

Signed-off-by: Revanth Kumar Uppala <ruppala@nvidia.com>
---
 drivers/net/phy/aquantia_main.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
index faca2a0b1d49..a27ff4733050 100644
--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c
@@ -41,6 +41,7 @@
 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_SGMII	6
 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_RXAUI	7
 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_OCSGMII	10
+#define MDIO_PHYXS_VEND_IF_STATUS_TX_READY	BIT(12)
 
 #define MDIO_AN_VEND_PROV			0xc400
 #define MDIO_AN_VEND_PROV_1000BASET_FULL	BIT(15)
@@ -467,6 +468,19 @@ static int aqr107_read_status(struct phy_device *phydev)
 		break;
 	}
 
+	/* Lane bring-up failures are seen during interface up, as interface
+	 * speed settings are configured while the PHY is still initializing.
+	 * To resolve this, poll until PHY system side interface gets ready
+	 * and the interface speed settings are configured.
+	 */
+	ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_PHYXS, MDIO_PHYXS_VEND_IF_STATUS,
+					val, (val & MDIO_PHYXS_VEND_IF_STATUS_TX_READY),
+					20000, 2000000, false);
+	if (ret) {
+		phydev_err(phydev, "PHY system interface is not yet ready\n");
+		return ret;
+	}
+
 	/* Read possibly downshifted rate from vendor register */
 	return aqr107_read_rate(phydev);
 }
-- 
2.17.1

Re: [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side

[Russell King (Oracle)]

On Wed, Jun 28, 2023 at 06:13:25PM +0530, Revanth Kumar Uppala wrote:
> +	/* Lane bring-up failures are seen during interface up, as interface
> +	 * speed settings are configured while the PHY is still initializing.
> +	 * To resolve this, poll until PHY system side interface gets ready
> +	 * and the interface speed settings are configured.
> +	 */
> +	ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_PHYXS, MDIO_PHYXS_VEND_IF_STATUS,
> +					val, (val & MDIO_PHYXS_VEND_IF_STATUS_TX_READY),
> +					20000, 2000000, false);

What does this actually mean when the condition succeeds? Does it mean
that the system interface is now fully configured (but may or may not
have link)?

If that's correct, then that's fine. If it doesn't succeed because
the system interface doesn't have link, then that would be very bad,
because _this_ function needs to return so the MAC side can then be
configured to gain link with the PHY with the appropriate link
parameters.

The comment doesn't make it clear which it is.

Thanks.

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

RE: [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side

[Revanth Kumar Uppala]

> -----Original Message-----
> From: Russell King <linux@armlinux.org.uk>
> Sent: Wednesday, June 28, 2023 7:04 PM
> To: Revanth Kumar Uppala <ruppala@nvidia.com>
> Cc: andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org; linux-
> tegra@vger.kernel.org
> Subject: Re: [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side
> 
> External email: Use caution opening links or attachments
> 
> 
> On Wed, Jun 28, 2023 at 06:13:25PM +0530, Revanth Kumar Uppala wrote:
> > +     /* Lane bring-up failures are seen during interface up, as interface
> > +      * speed settings are configured while the PHY is still initializing.
> > +      * To resolve this, poll until PHY system side interface gets ready
> > +      * and the interface speed settings are configured.
> > +      */
> > +     ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_PHYXS,
> MDIO_PHYXS_VEND_IF_STATUS,
> > +                                     val, (val & MDIO_PHYXS_VEND_IF_STATUS_TX_READY),
> > +                                     20000, 2000000, false);
> 
> What does this actually mean when the condition succeeds? Does it mean that
> the system interface is now fully configured (but may or may not have link)?
Yes, your understanding is correct.
It means that the system interface is now fully configured and has the link.
> 
> If that's correct, then that's fine. If it doesn't succeed because the system
> interface doesn't have link, then that would be very bad, because _this_ function
> needs to return so the MAC side can then be configured to gain link with the PHY
> with the appropriate link parameters.
> 
> The comment doesn't make it clear which it is.
I will add the comment more clearly in V2 series.
> 
> Thanks.
> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side

[Russell King (Oracle)]

On Mon, Jul 24, 2023 at 11:29:33AM +0000, Revanth Kumar Uppala wrote:
> > -----Original Message-----
> > From: Russell King <linux@armlinux.org.uk>
> > Sent: Wednesday, June 28, 2023 7:04 PM
> > To: Revanth Kumar Uppala <ruppala@nvidia.com>
> > Cc: andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org; linux-
> > tegra@vger.kernel.org
> > Subject: Re: [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system side
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Wed, Jun 28, 2023 at 06:13:25PM +0530, Revanth Kumar Uppala wrote:
> > > +     /* Lane bring-up failures are seen during interface up, as interface
> > > +      * speed settings are configured while the PHY is still initializing.
> > > +      * To resolve this, poll until PHY system side interface gets ready
> > > +      * and the interface speed settings are configured.
> > > +      */
> > > +     ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_PHYXS,
> > MDIO_PHYXS_VEND_IF_STATUS,
> > > +                                     val, (val & MDIO_PHYXS_VEND_IF_STATUS_TX_READY),
> > > +                                     20000, 2000000, false);
> > 
> > What does this actually mean when the condition succeeds? Does it mean that
> > the system interface is now fully configured (but may or may not have link)?
> Yes, your understanding is correct.
> It means that the system interface is now fully configured and has the link.

As you indicate that it also indicates that the system interface has
link, then you leave me no option but to NAK this patch, sorry. The
reason is:

> > ... If it doesn't succeed because the system
> > interface doesn't have link, then that would be very bad, because _this_ function
> > needs to return so the MAC side can then be configured to gain link with the PHY
> > with the appropriate link parameters.

Essentially, if the PHY changes its host interface because the media
side has changed, we *need* the read_status() function to succeed, tell
us that the link is up, and what the parameters are for the media side
link _and_ the host side interface.

At this point, if the PHY has changed its host-side interface, then the
link with the host MAC will be _down_ because the MAC driver is not yet
aware of the new parameters for the link. read_status() has to succeed
and report the new parameters to the MAC so that the MAC (or phylink)
can reconfigure the MAC and PCS for the PHY's new operating mode.

Sorry, but NAK.

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

[PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL)

[Revanth Kumar Uppala]

Configure the WOL settings in the PHY to allow the PHY to detect magic
packets and generate an interrupt to wake the device from suspend.

The PHY configuration is restored back to XFI once the PHY received the
WOL magic packet.

Note that it is not necessary to poll the PHY status when WOL is enabled
because the interface speed is not being re-configured

Signed-off-by: Narayan Reddy <narayanr@nvidia.com>
Signed-off-by: Revanth Kumar Uppala <ruppala@nvidia.com>
---
 drivers/net/phy/aquantia_main.c | 235 +++++++++++++++++++++++++++++++-
 include/uapi/linux/mdio.h       |   1 +
 2 files changed, 229 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
index a27ff4733050..5c69ecd2cf9f 100644
--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c
@@ -12,6 +12,7 @@
 #include <linux/delay.h>
 #include <linux/bitfield.h>
 #include <linux/phy.h>
+#include <linux/netdevice.h>
 
 #include "aquantia.h"
 
@@ -48,10 +49,13 @@
 #define MDIO_AN_VEND_PROV_1000BASET_HALF	BIT(14)
 #define MDIO_AN_VEND_PROV_5000BASET_FULL	BIT(11)
 #define MDIO_AN_VEND_PROV_2500BASET_FULL	BIT(10)
+#define MDIO_AN_VEND_PROV_AQRATE_DWN_SHFT_CAP	BIT(12)
 #define MDIO_AN_VEND_PROV_DOWNSHIFT_EN		BIT(4)
 #define MDIO_AN_VEND_PROV_DOWNSHIFT_MASK	GENMASK(3, 0)
 #define MDIO_AN_VEND_PROV_DOWNSHIFT_DFLT	4
+#define MDIO_AN_VEND_MASK			0xF0FF
 
+#define MDIO_AN_RSVD_VEND_PROV1			0xc410
 #define MDIO_AN_TX_VEND_STATUS1			0xc800
 #define MDIO_AN_TX_VEND_STATUS1_RATE_MASK	GENMASK(3, 1)
 #define MDIO_AN_TX_VEND_STATUS1_10BASET		0
@@ -61,6 +65,9 @@
 #define MDIO_AN_TX_VEND_STATUS1_2500BASET	4
 #define MDIO_AN_TX_VEND_STATUS1_5000BASET	5
 #define MDIO_AN_TX_VEND_STATUS1_FULL_DUPLEX	BIT(0)
+#define MDIO_MMD_AN_WOL_ENABLE			BIT(6)
+
+#define MDIO_AN_RSVD_VEND_STATUS3		0xc812
 
 #define MDIO_AN_TX_VEND_INT_STATUS1		0xcc00
 #define MDIO_AN_TX_VEND_INT_STATUS1_DOWNSHIFT	BIT(1)
@@ -86,6 +93,19 @@
 #define MDIO_AN_RX_VEND_STAT3_AFR		BIT(0)
 
 /* MDIO_MMD_C22EXT */
+#define MDIO_C22EXT_MAGIC_PKT_PATTERN_0_2_15		0xc339
+#define MDIO_C22EXT_MAGIC_PKT_PATTERN_16_2_31		0xc33a
+#define MDIO_C22EXT_MAGIC_PKT_PATTERN_32_2_47		0xc33b
+
+#define MDIO_C22EXT_GBE_PHY_RSI1_CTRL6			0xc355
+#define MDIO_C22EXT_RSI_WAKE_UP_FRAME_DETECTION         BIT(0)
+
+#define MDIO_C22EXT_GBE_PHY_RSI1_CTRL7			0xc356
+#define MDIO_C22EXT_RSI_MAGIC_PKT_FRAME_DETECTION	BIT(0)
+
+#define MDIO_C22EXT_GBE_PHY_RSI1_CTRL8			0xc357
+#define MDIO_C22EXT_RSI_WOL_FCS_MONITOR_MODE		BIT(15)
+
 #define MDIO_C22EXT_STAT_SGMII_RX_GOOD_FRAMES		0xd292
 #define MDIO_C22EXT_STAT_SGMII_RX_BAD_FRAMES		0xd294
 #define MDIO_C22EXT_STAT_SGMII_RX_FALSE_CARRIER		0xd297
@@ -96,6 +116,11 @@
 #define MDIO_C22EXT_STAT_SGMII_TX_LINE_COLLISIONS	0xd319
 #define MDIO_C22EXT_STAT_SGMII_TX_FRAME_ALIGN_ERR	0xd31a
 #define MDIO_C22EXT_STAT_SGMII_TX_RUNT_FRAMES		0xd31b
+#define MDIO_C22EXT_GBE_PHY_SGMII_TX_ALARM1		0xec20
+
+#define MDIO_C22EXT_GBE_PHY_SGMII_TX_INT_MASK1		0xf420
+#define MDIO_C22EXT_SGMII0_WAKE_UP_FRAME_MASK		BIT(4)
+#define MDIO_C22EXT_SGMII0_MAGIC_PKT_FRAME_MASK		BIT(5)
 
 /* Vendor specific 1, MDIO_MMD_VEND1 */
 #define VEND1_GLOBAL_FW_ID			0x0020
@@ -109,6 +134,10 @@
 #define VEND1_GLOBAL_CFG_10M			0x0310
 #define VEND1_GLOBAL_CFG_100M			0x031b
 #define VEND1_GLOBAL_CFG_1G			0x031c
+#define VEND1_GLOBAL_SYS_CONFIG_SGMII   (BIT(0) | BIT(1))
+#define VEND1_GLOBAL_SYS_CONFIG_AN      BIT(3)
+#define VEND1_GLOBAL_SYS_CONFIG_XFI     BIT(8)
+
 #define VEND1_GLOBAL_CFG_2_5G			0x031d
 #define VEND1_GLOBAL_CFG_5G			0x031e
 #define VEND1_GLOBAL_CFG_10G			0x031f
@@ -181,6 +210,7 @@ static const struct aqr107_hw_stat aqr107_hw_stats[] = {
 
 struct aqr107_priv {
 	u64 sgmii_stats[AQR107_SGMII_STAT_SZ];
+	int wol_status;
 };
 
 static int aqr107_get_sset_count(struct phy_device *phydev)
@@ -327,9 +357,139 @@ static int aqr_config_intr(struct phy_device *phydev)
 	return 0;
 }
 
+static int aqr113c_wol_enable(struct phy_device *phydev)
+{
+	struct aqr107_priv *priv = phydev->priv;
+	u16 val;
+	int ret;
+
+	/* Disables all advertised speeds except for the WoL
+	 * speed (100BASE-TX FD or 1000BASE-T)
+	 * This is set as per the APP note from Marvel
+	 */
+	ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL,
+			       MDIO_AN_LD_LOOP_TIMING_ABILITY);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_VEND_PROV);
+	if (ret < 0)
+		return ret;
+
+	val = (ret & MDIO_AN_VEND_MASK) |
+	      (MDIO_AN_VEND_PROV_AQRATE_DWN_SHFT_CAP | MDIO_AN_VEND_PROV_1000BASET_FULL);
+	ret = phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_VEND_PROV, val);
+	if (ret < 0)
+		return ret;
+
+	/* Enable the magic frame and wake up frame detection for the PHY */
+	ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_GBE_PHY_RSI1_CTRL6,
+			       MDIO_C22EXT_RSI_WAKE_UP_FRAME_DETECTION);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_GBE_PHY_RSI1_CTRL7,
+			       MDIO_C22EXT_RSI_MAGIC_PKT_FRAME_DETECTION);
+	if (ret < 0)
+		return ret;
+
+	/* Set the WoL enable bit */
+	ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN, MDIO_AN_RSVD_VEND_PROV1,
+			       MDIO_MMD_AN_WOL_ENABLE);
+	if (ret < 0)
+		return ret;
+
+	/* Set the WoL INT_N trigger bit */
+	ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_GBE_PHY_RSI1_CTRL8,
+			       MDIO_C22EXT_RSI_WOL_FCS_MONITOR_MODE);
+	if (ret < 0)
+		return ret;
+
+	/* Enable Interrupt INT_N Generation at pin level */
+	ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_GBE_PHY_SGMII_TX_INT_MASK1,
+			       MDIO_C22EXT_SGMII0_WAKE_UP_FRAME_MASK |
+			       MDIO_C22EXT_SGMII0_MAGIC_PKT_FRAME_MASK);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_INT_STD_MASK,
+			       VEND1_GLOBAL_INT_STD_MASK_ALL);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_INT_VEND_MASK,
+			       VEND1_GLOBAL_INT_VEND_MASK_GBE);
+	if (ret < 0)
+		return ret;
+
+	/* Set the system interface to SGMII */
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
+			    VEND1_GLOBAL_CFG_100M, VEND1_GLOBAL_SYS_CONFIG_SGMII |
+			    VEND1_GLOBAL_SYS_CONFIG_AN);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
+			    VEND1_GLOBAL_CFG_1G, VEND1_GLOBAL_SYS_CONFIG_SGMII |
+			    VEND1_GLOBAL_SYS_CONFIG_AN);
+	if (ret < 0)
+		return ret;
+
+	/* restart auto-negotiation */
+	genphy_c45_restart_aneg(phydev);
+	priv->wol_status = 1;
+
+	return 0;
+}
+
+static int aqr113c_wol_disable(struct phy_device *phydev)
+{
+	struct aqr107_priv *priv = phydev->priv;
+	int ret;
+
+	/* Disable the WoL enable bit */
+	ret = phy_clear_bits_mmd(phydev, MDIO_MMD_AN, MDIO_AN_RSVD_VEND_PROV1,
+				 MDIO_MMD_AN_WOL_ENABLE);
+	if (ret < 0)
+		return ret;
+
+	/* Restore the SERDES/System Interface back to the XFI mode */
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
+			    VEND1_GLOBAL_CFG_100M, VEND1_GLOBAL_SYS_CONFIG_XFI);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
+			    VEND1_GLOBAL_CFG_1G, VEND1_GLOBAL_SYS_CONFIG_XFI);
+	if (ret < 0)
+		return ret;
+
+	/* restart auto-negotiation */
+	genphy_c45_restart_aneg(phydev);
+	priv->wol_status = 0;
+
+	return 0;
+}
+
 static irqreturn_t aqr_handle_interrupt(struct phy_device *phydev)
 {
+	struct aqr107_priv *priv = phydev->priv;
 	int irq_status;
+	int ret;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_GBE_PHY_SGMII_TX_ALARM1);
+	if (ret < 0) {
+		phy_error(phydev);
+		return IRQ_NONE;
+	}
+
+	if ((ret & MDIO_C22EXT_SGMII0_MAGIC_PKT_FRAME_MASK) ==
+	    MDIO_C22EXT_SGMII0_MAGIC_PKT_FRAME_MASK) {
+		/* Disable the WoL */
+		ret = aqr113c_wol_disable(phydev);
+		if (ret < 0)
+			return IRQ_NONE;
+	}
 
 	irq_status = phy_read_mmd(phydev, MDIO_MMD_AN,
 				  MDIO_AN_TX_VEND_INT_STATUS2);
@@ -425,6 +585,7 @@ static int aqr107_read_rate(struct phy_device *phydev)
 
 static int aqr107_read_status(struct phy_device *phydev)
 {
+	struct aqr107_priv *priv = phydev->priv;
 	int val, ret;
 
 	ret = aqr_read_status(phydev);
@@ -471,14 +632,18 @@ static int aqr107_read_status(struct phy_device *phydev)
 	/* Lane bring-up failures are seen during interface up, as interface
 	 * speed settings are configured while the PHY is still initializing.
 	 * To resolve this, poll until PHY system side interface gets ready
-	 * and the interface speed settings are configured.
+	 * and the interface speed settings are configured.Polling is skipped
+	 * when WoL is enabled because interface speed settings are not
+	 * configured at that time.
 	 */
-	ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_PHYXS, MDIO_PHYXS_VEND_IF_STATUS,
-					val, (val & MDIO_PHYXS_VEND_IF_STATUS_TX_READY),
-					20000, 2000000, false);
-	if (ret) {
-		phydev_err(phydev, "PHY system interface is not yet ready\n");
-		return ret;
+	if (!priv->wol_status) {
+		ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_PHYXS, MDIO_PHYXS_VEND_IF_STATUS,
+						val, (val & MDIO_PHYXS_VEND_IF_STATUS_TX_READY),
+						20000, 2000000, false);
+		if (ret) {
+			phydev_err(phydev, "PHY system interface is not yet ready\n");
+			return ret;
+		}
 	}
 
 	/* Read possibly downshifted rate from vendor register */
@@ -619,6 +784,31 @@ static int aqr107_config_init(struct phy_device *phydev)
 	if (ret < 0)
 		return ret;
 
+	/* Configure Magic packet frame pattern (MAC address) */
+	ret = phy_write_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_MAGIC_PKT_PATTERN_0_2_15,
+			    phydev->attached_dev->dev_addr[0] |
+			    (phydev->attached_dev->dev_addr[1] << 8));
+	if (ret < 0) {
+		phydev_err(phydev, "Error setting magic packet frame of 0/1st byte\n");
+		return ret;
+	}
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_MAGIC_PKT_PATTERN_16_2_31,
+			    phydev->attached_dev->dev_addr[2] |
+			    (phydev->attached_dev->dev_addr[3] << 8));
+	if (ret < 0) {
+		phydev_err(phydev, "Error setting magic packet frame of 2/3rd byte\n");
+		return ret;
+	}
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_MAGIC_PKT_PATTERN_32_2_47,
+			    phydev->attached_dev->dev_addr[4] |
+			    (phydev->attached_dev->dev_addr[5] << 8));
+	if (ret < 0) {
+		phydev_err(phydev, "Error setting magic packet frame of 4/5th byte\n");
+		return ret;
+	}
+
 	return aqr107_set_downshift(phydev, MDIO_AN_VEND_PROV_DOWNSHIFT_DFLT);
 }
 
@@ -757,6 +947,35 @@ static int aqr107_probe(struct phy_device *phydev)
 	return aqr_hwmon_probe(phydev);
 }
 
+static void aqr113c_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
+{
+	int val;
+
+	val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_RSVD_VEND_STATUS3);
+	if (val < 0)
+		return;
+
+	wol->supported = WAKE_MAGIC;
+	if (val & 0x1)
+		wol->wolopts = WAKE_MAGIC;
+}
+
+static int aqr113c_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
+{
+	struct aqr107_priv *priv = phydev->priv;
+
+	/* Return success if WOL is already set. Don't entertain duplicate setting of WOL */
+	if (!(priv->wol_status ^ wol->wolopts))
+		return 0;
+
+	if (wol->wolopts & WAKE_MAGIC)
+		return aqr113c_wol_enable(phydev);
+	else
+		return aqr113c_wol_disable(phydev);
+
+	return 0;
+}
+
 static struct phy_driver aqr_driver[] = {
 {
 	PHY_ID_MATCH_MODEL(PHY_ID_AQ1202),
@@ -892,6 +1111,8 @@ static struct phy_driver aqr_driver[] = {
 	.get_strings    = aqr107_get_strings,
 	.get_stats      = aqr107_get_stats,
 	.link_change_notify = aqr107_link_change_notify,
+	.get_wol        = &aqr113c_get_wol,
+	.set_wol        = &aqr113c_set_wol,
 },
 };
 
diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h
index b826598d1e94..07ca44891378 100644
--- a/include/uapi/linux/mdio.h
+++ b/include/uapi/linux/mdio.h
@@ -298,6 +298,7 @@
 #define MDIO_AN_10GBT_CTRL_ADV5G	0x0100	/* Advertise 5GBASE-T */
 #define MDIO_AN_10GBT_CTRL_ADV10G	0x1000	/* Advertise 10GBASE-T */
 
+#define MDIO_AN_LD_LOOP_TIMING_ABILITY	0x0001
 /* AN 10GBASE-T status register. */
 #define MDIO_AN_10GBT_STAT_LP2_5G	0x0020  /* LP is 2.5GBT capable */
 #define MDIO_AN_10GBT_STAT_LP5G		0x0040  /* LP is 5GBT capable */
-- 
2.17.1

Re: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL)

[Russell King (Oracle)]

On Wed, Jun 28, 2023 at 06:13:26PM +0530, Revanth Kumar Uppala wrote:
> @@ -109,6 +134,10 @@
>  #define VEND1_GLOBAL_CFG_10M			0x0310
>  #define VEND1_GLOBAL_CFG_100M			0x031b
>  #define VEND1_GLOBAL_CFG_1G			0x031c
> +#define VEND1_GLOBAL_SYS_CONFIG_SGMII   (BIT(0) | BIT(1))
> +#define VEND1_GLOBAL_SYS_CONFIG_AN      BIT(3)
> +#define VEND1_GLOBAL_SYS_CONFIG_XFI     BIT(8)

My understanding is that bits 2:0 are a _bitfield_ and not individual
bits, which contain the following values:

0 - 10GBASE-R (XFI if you really want to call it that)
3 - SGMII
4 - OCSGMII (2.5G)
6 - 5GBASE-R (XFI5G if you really want to call it that)

Bit 3 controls whether the SGMII control word is used, and this is the
only applicable mode.

Bit 8 is already defined - it's part of the rate adaption mode field,
see VEND1_GLOBAL_CFG_RATE_ADAPT and VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE.

These bits apply to all the VEND1_GLOBAL_CFG_* registers, so these
should be defined after the last register (0x031f).

> +static int aqr113c_wol_enable(struct phy_device *phydev)
> +{
> +	struct aqr107_priv *priv = phydev->priv;
> +	u16 val;
> +	int ret;
> +
> +	/* Disables all advertised speeds except for the WoL
> +	 * speed (100BASE-TX FD or 1000BASE-T)
> +	 * This is set as per the APP note from Marvel
> +	 */
> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL,
> +			       MDIO_AN_LD_LOOP_TIMING_ABILITY);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_VEND_PROV);
> +	if (ret < 0)
> +		return ret;
> +
> +	val = (ret & MDIO_AN_VEND_MASK) |
> +	      (MDIO_AN_VEND_PROV_AQRATE_DWN_SHFT_CAP | MDIO_AN_VEND_PROV_1000BASET_FULL);
> +	ret = phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_VEND_PROV, val);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Enable the magic frame and wake up frame detection for the PHY */
> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_GBE_PHY_RSI1_CTRL6,
> +			       MDIO_C22EXT_RSI_WAKE_UP_FRAME_DETECTION);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_GBE_PHY_RSI1_CTRL7,
> +			       MDIO_C22EXT_RSI_MAGIC_PKT_FRAME_DETECTION);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Set the WoL enable bit */
> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN, MDIO_AN_RSVD_VEND_PROV1,
> +			       MDIO_MMD_AN_WOL_ENABLE);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Set the WoL INT_N trigger bit */
> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_GBE_PHY_RSI1_CTRL8,
> +			       MDIO_C22EXT_RSI_WOL_FCS_MONITOR_MODE);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Enable Interrupt INT_N Generation at pin level */
> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_GBE_PHY_SGMII_TX_INT_MASK1,
> +			       MDIO_C22EXT_SGMII0_WAKE_UP_FRAME_MASK |
> +			       MDIO_C22EXT_SGMII0_MAGIC_PKT_FRAME_MASK);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_INT_STD_MASK,
> +			       VEND1_GLOBAL_INT_STD_MASK_ALL);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_INT_VEND_MASK,
> +			       VEND1_GLOBAL_INT_VEND_MASK_GBE);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Set the system interface to SGMII */
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> +			    VEND1_GLOBAL_CFG_100M, VEND1_GLOBAL_SYS_CONFIG_SGMII |
> +			    VEND1_GLOBAL_SYS_CONFIG_AN);

How do you know that SGMII should be used for 100M?

> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> +			    VEND1_GLOBAL_CFG_1G, VEND1_GLOBAL_SYS_CONFIG_SGMII |
> +			    VEND1_GLOBAL_SYS_CONFIG_AN);

How do you know that SGMII should be used for 1G?

Doesn't this depend on the configuration of the host MAC and the
capabilities of it? If the host MAC only supports 10G, doesn't this
break stuff?

> +	if (ret < 0)
> +		return ret;
> +
> +	/* restart auto-negotiation */
> +	genphy_c45_restart_aneg(phydev);
> +	priv->wol_status = 1;
> +
> +	return 0;
> +}
> +
> +static int aqr113c_wol_disable(struct phy_device *phydev)
> +{
> +	struct aqr107_priv *priv = phydev->priv;
> +	int ret;
> +
> +	/* Disable the WoL enable bit */
> +	ret = phy_clear_bits_mmd(phydev, MDIO_MMD_AN, MDIO_AN_RSVD_VEND_PROV1,
> +				 MDIO_MMD_AN_WOL_ENABLE);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Restore the SERDES/System Interface back to the XFI mode */
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> +			    VEND1_GLOBAL_CFG_100M, VEND1_GLOBAL_SYS_CONFIG_XFI);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> +			    VEND1_GLOBAL_CFG_1G, VEND1_GLOBAL_SYS_CONFIG_XFI);
> +	if (ret < 0)
> +		return ret;

Conversely, how do you know that configuring 100M/1G to use 10GBASE-R on
the host interface is how the PHY was provisioned in firmware? I think
at the very least, you should be leaving these settings alone until you
know that the system is entering a low power mode, saving the settings,
and restoring them when you wake up.

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

RE: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL)

[Revanth Kumar Uppala]

> -----Original Message-----
> From: Russell King <linux@armlinux.org.uk>
> Sent: Wednesday, June 28, 2023 7:13 PM
> To: Revanth Kumar Uppala <ruppala@nvidia.com>
> Cc: andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org; linux-
> tegra@vger.kernel.org; Narayan Reddy <narayanr@nvidia.com>
> Subject: Re: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL)
> 
> External email: Use caution opening links or attachments
> 
> 
> On Wed, Jun 28, 2023 at 06:13:26PM +0530, Revanth Kumar Uppala wrote:
> > @@ -109,6 +134,10 @@
> >  #define VEND1_GLOBAL_CFG_10M                 0x0310
> >  #define VEND1_GLOBAL_CFG_100M                        0x031b
> >  #define VEND1_GLOBAL_CFG_1G                  0x031c
> > +#define VEND1_GLOBAL_SYS_CONFIG_SGMII   (BIT(0) | BIT(1))
> > +#define VEND1_GLOBAL_SYS_CONFIG_AN      BIT(3)
> > +#define VEND1_GLOBAL_SYS_CONFIG_XFI     BIT(8)
> 
> My understanding is that bits 2:0 are a _bitfield_ and not individual bits, which
> contain the following values:
I will define bitfield instead of defining individual bits in V2 series
> 
> 0 - 10GBASE-R (XFI if you really want to call it that)
> 3 - SGMII
> 4 - OCSGMII (2.5G)
> 6 - 5GBASE-R (XFI5G if you really want to call it that)
> 
> Bit 3 controls whether the SGMII control word is used, and this is the only
> applicable mode.
> 
> Bit 8 is already defined - it's part of the rate adaption mode field, see
> VEND1_GLOBAL_CFG_RATE_ADAPT and
> VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE.
Sure, I will use above mentioned macros and will set the register values with help of FIELD_PREP in V2 series
> 
> These bits apply to all the VEND1_GLOBAL_CFG_* registers, so these should be
> defined after the last register (0x031f).
Will take care of this.
> 
> > +static int aqr113c_wol_enable(struct phy_device *phydev) {
> > +     struct aqr107_priv *priv = phydev->priv;
> > +     u16 val;
> > +     int ret;
> > +
> > +     /* Disables all advertised speeds except for the WoL
> > +      * speed (100BASE-TX FD or 1000BASE-T)
> > +      * This is set as per the APP note from Marvel
> > +      */
> > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN,
> MDIO_AN_10GBT_CTRL,
> > +                            MDIO_AN_LD_LOOP_TIMING_ABILITY);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_VEND_PROV);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     val = (ret & MDIO_AN_VEND_MASK) |
> > +           (MDIO_AN_VEND_PROV_AQRATE_DWN_SHFT_CAP |
> MDIO_AN_VEND_PROV_1000BASET_FULL);
> > +     ret = phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_VEND_PROV,
> val);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     /* Enable the magic frame and wake up frame detection for the PHY */
> > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT,
> MDIO_C22EXT_GBE_PHY_RSI1_CTRL6,
> > +                            MDIO_C22EXT_RSI_WAKE_UP_FRAME_DETECTION);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT,
> MDIO_C22EXT_GBE_PHY_RSI1_CTRL7,
> > +                            MDIO_C22EXT_RSI_MAGIC_PKT_FRAME_DETECTION);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     /* Set the WoL enable bit */
> > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN,
> MDIO_AN_RSVD_VEND_PROV1,
> > +                            MDIO_MMD_AN_WOL_ENABLE);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     /* Set the WoL INT_N trigger bit */
> > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT,
> MDIO_C22EXT_GBE_PHY_RSI1_CTRL8,
> > +                            MDIO_C22EXT_RSI_WOL_FCS_MONITOR_MODE);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     /* Enable Interrupt INT_N Generation at pin level */
> > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT,
> MDIO_C22EXT_GBE_PHY_SGMII_TX_INT_MASK1,
> > +                            MDIO_C22EXT_SGMII0_WAKE_UP_FRAME_MASK |
> > +                            MDIO_C22EXT_SGMII0_MAGIC_PKT_FRAME_MASK);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1,
> VEND1_GLOBAL_INT_STD_MASK,
> > +                            VEND1_GLOBAL_INT_STD_MASK_ALL);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1,
> VEND1_GLOBAL_INT_VEND_MASK,
> > +                            VEND1_GLOBAL_INT_VEND_MASK_GBE);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     /* Set the system interface to SGMII */
> > +     ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > +                         VEND1_GLOBAL_CFG_100M,
> VEND1_GLOBAL_SYS_CONFIG_SGMII |
> > +                         VEND1_GLOBAL_SYS_CONFIG_AN);
> 
> How do you know that SGMII should be used for 100M?
> 
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > +                         VEND1_GLOBAL_CFG_1G,
> VEND1_GLOBAL_SYS_CONFIG_SGMII |
> > +                         VEND1_GLOBAL_SYS_CONFIG_AN);
> 
> How do you know that SGMII should be used for 1G?
> 
> Doesn't this depend on the configuration of the host MAC and the capabilities of
> it? If the host MAC only supports 10G, doesn't this break stuff?
> 
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     /* restart auto-negotiation */
> > +     genphy_c45_restart_aneg(phydev);
> > +     priv->wol_status = 1;
> > +
> > +     return 0;
> > +}
> > +
> > +static int aqr113c_wol_disable(struct phy_device *phydev) {
> > +     struct aqr107_priv *priv = phydev->priv;
> > +     int ret;
> > +
> > +     /* Disable the WoL enable bit */
> > +     ret = phy_clear_bits_mmd(phydev, MDIO_MMD_AN,
> MDIO_AN_RSVD_VEND_PROV1,
> > +                              MDIO_MMD_AN_WOL_ENABLE);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     /* Restore the SERDES/System Interface back to the XFI mode */
> > +     ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > +                         VEND1_GLOBAL_CFG_100M,
> VEND1_GLOBAL_SYS_CONFIG_XFI);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > +                         VEND1_GLOBAL_CFG_1G, VEND1_GLOBAL_SYS_CONFIG_XFI);
> > +     if (ret < 0)
> > +             return ret;
> 
> Conversely, how do you know that configuring 100M/1G to use 10GBASE-R on
> the host interface is how the PHY was provisioned in firmware? I think at the
> very least, you should be leaving these settings alone until you know that the
> system is entering a low power mode, saving the settings, and restoring them
> when you wake up.

Regarding all the above comments ,
We are following the app note AN-N4209 by Marvell semiconductors for enabling and disabling of WOL.
Below are the steps in brief as mentioned in app note
For enabling the WOL,
1. Set the MAC address for the PHY. Make sure the MAC address is a legal address 
2. Disables all advertised speeds except for the WoL speed (100BASE-TX FD or 1000BASE-T)
3. Enable the magic frame and wake up frame detection for the PHY
4. Set the WoL enable bit
5. Set the WoL INT_N trigger bit
6. Optional: Enable Interrupt INT_N Generation at pin level
7. Set the system interface to SGMII by writing into below registers
MDIO Write 1e.31b = 0x0b (Sets SGMII for 100M) 
MDIO Write 1e.31c = 0x0b (Sets SGMII for 1G)
8. Perform a link re-negotiation/auto-negotiation
9. The WoL status bit should be 1 which indicates that the WoL is active. The PHY now is in sleep mode

For remote WAKEUP via magic packet,
1.MAC detects INT from PHY and confirm Wake request.
2. Disable the WoL mode by unsetting the WoL enable bit.
3. Restore the SERDES/System Interface back to the original mode before WoL was initialized using SGMII mode i.e; back to XFI mode.
MDIO write 1e.31b = 0x100 (Reverts the 100M setup to original mode)
MDIO write 1e.31c = 0x100 (Reverts the 1G setup to original mode
4. Perform an auto-negotiation for the register values to take effect and establish a proper link

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

Re: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL)

[Russell King (Oracle)]

On Mon, Jul 24, 2023 at 11:29:39AM +0000, Revanth Kumar Uppala wrote:
> 
> 
> > -----Original Message-----
> > From: Russell King <linux@armlinux.org.uk>
> > Sent: Wednesday, June 28, 2023 7:13 PM
> > To: Revanth Kumar Uppala <ruppala@nvidia.com>
> > Cc: andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org; linux-
> > tegra@vger.kernel.org; Narayan Reddy <narayanr@nvidia.com>
> > Subject: Re: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL)
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Wed, Jun 28, 2023 at 06:13:26PM +0530, Revanth Kumar Uppala wrote:
> > > @@ -109,6 +134,10 @@
> > >  #define VEND1_GLOBAL_CFG_10M                 0x0310
> > >  #define VEND1_GLOBAL_CFG_100M                        0x031b
> > >  #define VEND1_GLOBAL_CFG_1G                  0x031c
> > > +#define VEND1_GLOBAL_SYS_CONFIG_SGMII   (BIT(0) | BIT(1))
> > > +#define VEND1_GLOBAL_SYS_CONFIG_AN      BIT(3)
> > > +#define VEND1_GLOBAL_SYS_CONFIG_XFI     BIT(8)
> > 
> > My understanding is that bits 2:0 are a _bitfield_ and not individual bits, which
> > contain the following values:
> I will define bitfield instead of defining individual bits in V2 series
> > 
> > 0 - 10GBASE-R (XFI if you really want to call it that)
> > 3 - SGMII
> > 4 - OCSGMII (2.5G)
> > 6 - 5GBASE-R (XFI5G if you really want to call it that)
> > 
> > Bit 3 controls whether the SGMII control word is used, and this is the only
> > applicable mode.
> > 
> > Bit 8 is already defined - it's part of the rate adaption mode field, see
> > VEND1_GLOBAL_CFG_RATE_ADAPT and
> > VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE.
> Sure, I will use above mentioned macros and will set the register values with help of FIELD_PREP in V2 series
> > 
> > These bits apply to all the VEND1_GLOBAL_CFG_* registers, so these should be
> > defined after the last register (0x031f).
> Will take care of this.
> > 
> > > +static int aqr113c_wol_enable(struct phy_device *phydev) {
> > > +     struct aqr107_priv *priv = phydev->priv;
> > > +     u16 val;
> > > +     int ret;
> > > +
> > > +     /* Disables all advertised speeds except for the WoL
> > > +      * speed (100BASE-TX FD or 1000BASE-T)
> > > +      * This is set as per the APP note from Marvel
> > > +      */
> > > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN,
> > MDIO_AN_10GBT_CTRL,
> > > +                            MDIO_AN_LD_LOOP_TIMING_ABILITY);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_VEND_PROV);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     val = (ret & MDIO_AN_VEND_MASK) |
> > > +           (MDIO_AN_VEND_PROV_AQRATE_DWN_SHFT_CAP |
> > MDIO_AN_VEND_PROV_1000BASET_FULL);
> > > +     ret = phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_VEND_PROV,
> > val);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     /* Enable the magic frame and wake up frame detection for the PHY */
> > > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT,
> > MDIO_C22EXT_GBE_PHY_RSI1_CTRL6,
> > > +                            MDIO_C22EXT_RSI_WAKE_UP_FRAME_DETECTION);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT,
> > MDIO_C22EXT_GBE_PHY_RSI1_CTRL7,
> > > +                            MDIO_C22EXT_RSI_MAGIC_PKT_FRAME_DETECTION);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     /* Set the WoL enable bit */
> > > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN,
> > MDIO_AN_RSVD_VEND_PROV1,
> > > +                            MDIO_MMD_AN_WOL_ENABLE);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     /* Set the WoL INT_N trigger bit */
> > > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT,
> > MDIO_C22EXT_GBE_PHY_RSI1_CTRL8,
> > > +                            MDIO_C22EXT_RSI_WOL_FCS_MONITOR_MODE);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     /* Enable Interrupt INT_N Generation at pin level */
> > > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_C22EXT,
> > MDIO_C22EXT_GBE_PHY_SGMII_TX_INT_MASK1,
> > > +                            MDIO_C22EXT_SGMII0_WAKE_UP_FRAME_MASK |
> > > +                            MDIO_C22EXT_SGMII0_MAGIC_PKT_FRAME_MASK);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1,
> > VEND1_GLOBAL_INT_STD_MASK,
> > > +                            VEND1_GLOBAL_INT_STD_MASK_ALL);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1,
> > VEND1_GLOBAL_INT_VEND_MASK,
> > > +                            VEND1_GLOBAL_INT_VEND_MASK_GBE);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     /* Set the system interface to SGMII */
> > > +     ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > > +                         VEND1_GLOBAL_CFG_100M,
> > VEND1_GLOBAL_SYS_CONFIG_SGMII |
> > > +                         VEND1_GLOBAL_SYS_CONFIG_AN);
> > 
> > How do you know that SGMII should be used for 100M?
> > 
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > > +                         VEND1_GLOBAL_CFG_1G,
> > VEND1_GLOBAL_SYS_CONFIG_SGMII |
> > > +                         VEND1_GLOBAL_SYS_CONFIG_AN);
> > 
> > How do you know that SGMII should be used for 1G?
> > 
> > Doesn't this depend on the configuration of the host MAC and the capabilities of
> > it? If the host MAC only supports 10G, doesn't this break stuff?
> > 
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     /* restart auto-negotiation */
> > > +     genphy_c45_restart_aneg(phydev);
> > > +     priv->wol_status = 1;
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int aqr113c_wol_disable(struct phy_device *phydev) {
> > > +     struct aqr107_priv *priv = phydev->priv;
> > > +     int ret;
> > > +
> > > +     /* Disable the WoL enable bit */
> > > +     ret = phy_clear_bits_mmd(phydev, MDIO_MMD_AN,
> > MDIO_AN_RSVD_VEND_PROV1,
> > > +                              MDIO_MMD_AN_WOL_ENABLE);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     /* Restore the SERDES/System Interface back to the XFI mode */
> > > +     ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > > +                         VEND1_GLOBAL_CFG_100M,
> > VEND1_GLOBAL_SYS_CONFIG_XFI);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > > +                         VEND1_GLOBAL_CFG_1G, VEND1_GLOBAL_SYS_CONFIG_XFI);
> > > +     if (ret < 0)
> > > +             return ret;
> > 
> > Conversely, how do you know that configuring 100M/1G to use 10GBASE-R on
> > the host interface is how the PHY was provisioned in firmware? I think at the
> > very least, you should be leaving these settings alone until you know that the
> > system is entering a low power mode, saving the settings, and restoring them
> > when you wake up.
> 
> Regarding all the above comments ,
> We are following the app note AN-N4209 by Marvell semiconductors for enabling and disabling of WOL.
> Below are the steps in brief as mentioned in app note

So basically what I gather is that the answer to "how do you know that
configuring 100M/1G to use 10GBASE-R the host interface is how the PHY
was provisioned in firmware?" is that you don't know, and you're just
blindly following what someone's thrown into an application note but
haven't thought enough about it.

> For remote WAKEUP via magic packet,
> 1.MAC detects INT from PHY and confirm Wake request.
> 2. Disable the WoL mode by unsetting the WoL enable bit.
> 3. Restore the SERDES/System Interface back to the original mode before WoL was initialized using SGMII mode i.e; back to XFI mode.
> MDIO write 1e.31b = 0x100 (Reverts the 100M setup to original mode)
> MDIO write 1e.31c = 0x100 (Reverts the 1G setup to original mode

I think you have misunderstood step 3. "Restore ... back to the
original mode" when interpreting the application note.

Since these registers are set during the provisioning on the PHY,
there is *no* guarantee that they were originally in XFI mode before
WoL was enabled. Hence, in order to "restore" their state, you need
to "save" their state at some point, and it would probably be a good
idea to do that when:

1) the PHY is probed to get the power-up status.
2) update the saved registers whenever the driver reconfigures the PHY
   for a different interface mode (I don't think it does this.)
3) use this saved information to restore these registers when WoL is
   disabled, _or_ when the PHY device is detached from the PHY driver
   i.o.w. when the ->remove method is called, so that if the driver
   re-probes, it can get at the _original_ information.

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

Re: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL)

[Andrew Lunn]

> +static int aqr113c_wol_enable(struct phy_device *phydev)
> +{
> +	struct aqr107_priv *priv = phydev->priv;
> +	u16 val;
> +	int ret;
> +
> +	/* Disables all advertised speeds except for the WoL
> +	 * speed (100BASE-TX FD or 1000BASE-T)
> +	 * This is set as per the APP note from Marvel
> +	 */
> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL,
> +			       MDIO_AN_LD_LOOP_TIMING_ABILITY);
> +	if (ret < 0)
> +		return ret;

Please take a look at phylink_speed_down() and
phylink_speed_up(). Assuming the PHY is not reporting it can do 10Full
and 10Half, it should end up in 100BaseFull. Assuming the link partner
can do 100BaseFull....

Russell points out you are making a lot of assumptions about the
system side link. Ideally, you want to leave that to the PHY. Once the
auto-neg at the lower speed has completed, it might change the system
side link, e.g. to SGMII and the normal machinery should pass that
onto the MAC, so it can follow. I would not force anything.

> @@ -619,6 +784,31 @@ static int aqr107_config_init(struct phy_device *phydev)
>  	if (ret < 0)
>  		return ret;
>  
> +	/* Configure Magic packet frame pattern (MAC address) */
> +	ret = phy_write_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_MAGIC_PKT_PATTERN_0_2_15,
> +			    phydev->attached_dev->dev_addr[0] |
> +			    (phydev->attached_dev->dev_addr[1] << 8));

I think most PHY drivers do this as part of enabling WOL. Doing it in
aqr107_config_init() is early, is the MAC address stable yet? The user
could change it. It could still be changed after wol is enabled, but
at least the user has a clear point in time when WoL configuration
happens.

> +static void aqr113c_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
> +{
> +	int val;
> +
> +	val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_RSVD_VEND_STATUS3);
> +	if (val < 0)
> +		return;
> +
> +	wol->supported = WAKE_MAGIC;
> +	if (val & 0x1)
> +		wol->wolopts = WAKE_MAGIC;

WoL seems to be tried to interrupts. So maybe you should actually
check an interrupt is available? This is not going to work if the PHY
is being polled. It does however get a bit messy, some boards might
connect the 'interrupt' pin to PMIC. So there is not a true interrupt,
but the PMIC can turn the power back on.

    Andrew

RE: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL)

[Revanth Kumar Uppala]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Wednesday, June 28, 2023 7:48 PM
> To: Revanth Kumar Uppala <ruppala@nvidia.com>
> Cc: linux@armlinux.org.uk; hkallweit1@gmail.com; netdev@vger.kernel.org;
> linux-tegra@vger.kernel.org; Narayan Reddy <narayanr@nvidia.com>
> Subject: Re: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL)
> 
> External email: Use caution opening links or attachments
> 
> 
> > +static int aqr113c_wol_enable(struct phy_device *phydev) {
> > +     struct aqr107_priv *priv = phydev->priv;
> > +     u16 val;
> > +     int ret;
> > +
> > +     /* Disables all advertised speeds except for the WoL
> > +      * speed (100BASE-TX FD or 1000BASE-T)
> > +      * This is set as per the APP note from Marvel
> > +      */
> > +     ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN,
> MDIO_AN_10GBT_CTRL,
> > +                            MDIO_AN_LD_LOOP_TIMING_ABILITY);
> > +     if (ret < 0)
> > +             return ret;
> 
> Please take a look at phylink_speed_down() and phylink_speed_up(). Assuming
> the PHY is not reporting it can do 10Full and 10Half, it should end up in
> 100BaseFull. Assuming the link partner can do 100BaseFull....
> 
> Russell points out you are making a lot of assumptions about the system side
> link. Ideally, you want to leave that to the PHY. Once the auto-neg at the lower
> speed has completed, it might change the system side link, e.g. to SGMII and the
> normal machinery should pass that onto the MAC, so it can follow. I would not
> force anything.
As per my reply to Russel's comment, we are following the app note AN-N4209 by Marvell semiconductors for enabling and disabling of WOL and above logic is part of the same app note.
> 
> > @@ -619,6 +784,31 @@ static int aqr107_config_init(struct phy_device
> *phydev)
> >       if (ret < 0)
> >               return ret;
> >
> > +     /* Configure Magic packet frame pattern (MAC address) */
> > +     ret = phy_write_mmd(phydev, MDIO_MMD_C22EXT,
> MDIO_C22EXT_MAGIC_PKT_PATTERN_0_2_15,
> > +                         phydev->attached_dev->dev_addr[0] |
> > +                         (phydev->attached_dev->dev_addr[1] << 8));
> 
> I think most PHY drivers do this as part of enabling WOL. Doing it in
> aqr107_config_init() is early, is the MAC address stable yet? The user could
> change it. It could still be changed after wol is enabled, but at least the user has
> a clear point in time when WoL configuration happens.
Yes, your assumption is correct.
Will move this logic to aqr113c_wol_enable() function to take care of above scenario.
Thanks,
Revanth Uppala

> 
> > +static void aqr113c_get_wol(struct phy_device *phydev, struct
> > +ethtool_wolinfo *wol) {
> > +     int val;
> > +
> > +     val = phy_read_mmd(phydev, MDIO_MMD_AN,
> MDIO_AN_RSVD_VEND_STATUS3);
> > +     if (val < 0)
> > +             return;
> > +
> > +     wol->supported = WAKE_MAGIC;
> > +     if (val & 0x1)
> > +             wol->wolopts = WAKE_MAGIC;
> 
> WoL seems to be tried to interrupts. So maybe you should actually check an
> interrupt is available? This is not going to work if the PHY is being polled. It does
> however get a bit messy, some boards might connect the 'interrupt' pin to PMIC.
> So there is not a true interrupt, but the PMIC can turn the power back on.
> 
>     Andrew

Re: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL)

[kernel test robot]

Hi Revanth,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net/main]
[also build test WARNING on net-next/main linus/master horms-ipvs/master v6.4 next-20230628]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Revanth-Kumar-Uppala/net-phy-aquantia-Enable-MAC-Controlled-EEE/20230628-204746
base:   net/main
patch link:    https://lore.kernel.org/r/20230628124326.55732-4-ruppala%40nvidia.com
patch subject: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL)
config: i386-randconfig-i013-20230628 (https://download.01.org/0day-ci/archive/20230629/202306290253.b8D3gQf8-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230629/202306290253.b8D3gQf8-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306290253.b8D3gQf8-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/net/phy/aquantia_main.c: In function 'aqr_handle_interrupt':
>> drivers/net/phy/aquantia_main.c:476:29: warning: unused variable 'priv' [-Wunused-variable]
     476 |         struct aqr107_priv *priv = phydev->priv;
         |                             ^~~~


vim +/priv +476 drivers/net/phy/aquantia_main.c

   473	
   474	static irqreturn_t aqr_handle_interrupt(struct phy_device *phydev)
   475	{
 > 476		struct aqr107_priv *priv = phydev->priv;
   477		int irq_status;
   478		int ret;
   479	
   480		ret = phy_read_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_GBE_PHY_SGMII_TX_ALARM1);
   481		if (ret < 0) {
   482			phy_error(phydev);
   483			return IRQ_NONE;
   484		}
   485	
   486		if ((ret & MDIO_C22EXT_SGMII0_MAGIC_PKT_FRAME_MASK) ==
   487		    MDIO_C22EXT_SGMII0_MAGIC_PKT_FRAME_MASK) {
   488			/* Disable the WoL */
   489			ret = aqr113c_wol_disable(phydev);
   490			if (ret < 0)
   491				return IRQ_NONE;
   492		}
   493	
   494		irq_status = phy_read_mmd(phydev, MDIO_MMD_AN,
   495					  MDIO_AN_TX_VEND_INT_STATUS2);
   496		if (irq_status < 0) {
   497			phy_error(phydev);
   498			return IRQ_NONE;
   499		}
   500	
   501		if (!(irq_status & MDIO_AN_TX_VEND_INT_STATUS2_MASK))
   502			return IRQ_NONE;
   503	
   504		phy_trigger_machine(phydev);
   505	
   506		return IRQ_HANDLED;
   507	}
   508	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY

[Russell King (Oracle)]

On Wed, Jun 28, 2023 at 06:13:23PM +0530, Revanth Kumar Uppala wrote:
> From: Narayan Reddy <narayanr@nvidia.com>
> 
> Enable flow control support using pause frames in aquantia phy driver.
> 
> Signed-off-by: Narayan Reddy <narayanr@nvidia.com>
> Signed-off-by: Revanth Kumar Uppala <ruppala@nvidia.com>

I think this is over-complex.

>  #define MDIO_PHYXS_VEND_IF_STATUS		0xe812
>  #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_MASK	GENMASK(7, 3)
>  #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_KR	0
> @@ -583,6 +585,17 @@ static int aqr107_config_init(struct phy_device *phydev)
>  	if (!ret)
>  		aqr107_chip_info(phydev);
>  
> +	/* Advertize flow control */
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev->supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev->supported);
> +	linkmode_copy(phydev->advertising, phydev->supported);

This is the wrong place to be doing this, since pause support depends
not only on the PHY but also on the MAC. There are phylib interfaces
that MACs should call so that phylib knows that the MAC supports pause
frames.

Secondly, the PHY driver needs to tell phylib that the PHY supports
pause frames, and that's done through either setting the .features
member in the PHY driver, or by providing a .get_features
implementation.

Configuration of the pause advertisement should already be happening
through the core phylib code.

Thanks.

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

Re: [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY

[Andrew Lunn]

On Wed, Jun 28, 2023 at 02:30:48PM +0100, Russell King (Oracle) wrote:
> On Wed, Jun 28, 2023 at 06:13:23PM +0530, Revanth Kumar Uppala wrote:
> > From: Narayan Reddy <narayanr@nvidia.com>
> > 
> > Enable flow control support using pause frames in aquantia phy driver.
> > 
> > Signed-off-by: Narayan Reddy <narayanr@nvidia.com>
> > Signed-off-by: Revanth Kumar Uppala <ruppala@nvidia.com>
> 
> I think this is over-complex.
> 
> >  #define MDIO_PHYXS_VEND_IF_STATUS		0xe812
> >  #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_MASK	GENMASK(7, 3)
> >  #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_KR	0
> > @@ -583,6 +585,17 @@ static int aqr107_config_init(struct phy_device *phydev)
> >  	if (!ret)
> >  		aqr107_chip_info(phydev);
> >  
> > +	/* Advertize flow control */
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev->supported);
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev->supported);
> > +	linkmode_copy(phydev->advertising, phydev->supported);
> 
> This is the wrong place to be doing this, since pause support depends
> not only on the PHY but also on the MAC. There are phylib interfaces
> that MACs should call so that phylib knows that the MAC supports pause
> frames.
> 
> Secondly, the PHY driver needs to tell phylib that the PHY supports
> pause frames, and that's done through either setting the .features
> member in the PHY driver, or by providing a .get_features
> implementation.
> 
> Configuration of the pause advertisement should already be happening
> through the core phylib code.

I really should do a LPC netdev talk "Everybody gets pause wrong..."

genphy_c45_an_config_aneg() will configure pause advertisement. The
PHY driver does not need to configure it, if the PHY follows the
standard and has the configuration in the correct place. As Russell
said, please check the PHYs ability to advertise pause is being
reported correctly, by .get_features, of the default implementation of
.get_features if that is being used. And then check your MAC driver is
also indicating it supports pause.

	Andrew

RE: [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY

[Revanth Kumar Uppala]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Wednesday, June 28, 2023 7:17 PM
> To: Russell King (Oracle) <linux@armlinux.org.uk>
> Cc: Revanth Kumar Uppala <ruppala@nvidia.com>; hkallweit1@gmail.com;
> netdev@vger.kernel.org; linux-tegra@vger.kernel.org; Narayan Reddy
> <narayanr@nvidia.com>
> Subject: Re: [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support
> in aquantia PHY
> 
> External email: Use caution opening links or attachments
> 
> 
> On Wed, Jun 28, 2023 at 02:30:48PM +0100, Russell King (Oracle) wrote:
> > On Wed, Jun 28, 2023 at 06:13:23PM +0530, Revanth Kumar Uppala wrote:
> > > From: Narayan Reddy <narayanr@nvidia.com>
> > >
> > > Enable flow control support using pause frames in aquantia phy driver.
> > >
> > > Signed-off-by: Narayan Reddy <narayanr@nvidia.com>
> > > Signed-off-by: Revanth Kumar Uppala <ruppala@nvidia.com>
> >
> > I think this is over-complex.
> >
> > >  #define MDIO_PHYXS_VEND_IF_STATUS          0xe812
> > >  #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_MASK        GENMASK(7, 3)
> > >  #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_KR  0 @@ -583,6 +585,17
> @@
> > > static int aqr107_config_init(struct phy_device *phydev)
> > >     if (!ret)
> > >             aqr107_chip_info(phydev);
> > >
> > > +   /* Advertize flow control */
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev-
> >supported);
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev-
> >supported);
> > > +   linkmode_copy(phydev->advertising, phydev->supported);
> >
> > This is the wrong place to be doing this, since pause support depends
> > not only on the PHY but also on the MAC. There are phylib interfaces
> > that MACs should call so that phylib knows that the MAC supports pause
> > frames.
> >
> > Secondly, the PHY driver needs to tell phylib that the PHY supports
> > pause frames, and that's done through either setting the .features
> > member in the PHY driver, or by providing a .get_features
> > implementation.
> >
> > Configuration of the pause advertisement should already be happening
> > through the core phylib code.
> 
> I really should do a LPC netdev talk "Everybody gets pause wrong..."
> 
> genphy_c45_an_config_aneg() will configure pause advertisement. The PHY
> driver does not need to configure it, if the PHY follows the standard and has the
> configuration in the correct place. As Russell said, please check the PHYs ability
> to advertise pause is being reported correctly, by .get_features, of the default
> implementation of .get_features if that is being used. And then check your MAC
> driver is also indicating it supports pause.
From .get_features, it is not possible to check PHY's ability to advertise pause is being reported as there is no such register present for AQR PHY to check capabilities in its datasheet.
Hence, we are directly configuring the pause frames from  aqr107_config_init().
Thanks,
Revanth Uppala
> 
>         Andrew

Re: [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY

[Russell King (Oracle)]

On Mon, Jul 24, 2023 at 11:29:18AM +0000, Revanth Kumar Uppala wrote:
> 
> 
> > -----Original Message-----
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: Wednesday, June 28, 2023 7:17 PM
> > To: Russell King (Oracle) <linux@armlinux.org.uk>
> > Cc: Revanth Kumar Uppala <ruppala@nvidia.com>; hkallweit1@gmail.com;
> > netdev@vger.kernel.org; linux-tegra@vger.kernel.org; Narayan Reddy
> > <narayanr@nvidia.com>
> > Subject: Re: [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support
> > in aquantia PHY
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Wed, Jun 28, 2023 at 02:30:48PM +0100, Russell King (Oracle) wrote:
> > > On Wed, Jun 28, 2023 at 06:13:23PM +0530, Revanth Kumar Uppala wrote:
> > > > From: Narayan Reddy <narayanr@nvidia.com>
> > > >
> > > > Enable flow control support using pause frames in aquantia phy driver.
> > > >
> > > > Signed-off-by: Narayan Reddy <narayanr@nvidia.com>
> > > > Signed-off-by: Revanth Kumar Uppala <ruppala@nvidia.com>
> > >
> > > I think this is over-complex.
> > >
> > > >  #define MDIO_PHYXS_VEND_IF_STATUS          0xe812
> > > >  #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_MASK        GENMASK(7, 3)
> > > >  #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_KR  0 @@ -583,6 +585,17
> > @@
> > > > static int aqr107_config_init(struct phy_device *phydev)
> > > >     if (!ret)
> > > >             aqr107_chip_info(phydev);
> > > >
> > > > +   /* Advertize flow control */
> > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev-
> > >supported);
> > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev-
> > >supported);
> > > > +   linkmode_copy(phydev->advertising, phydev->supported);
> > >
> > > This is the wrong place to be doing this, since pause support depends
> > > not only on the PHY but also on the MAC. There are phylib interfaces
> > > that MACs should call so that phylib knows that the MAC supports pause
> > > frames.
> > >
> > > Secondly, the PHY driver needs to tell phylib that the PHY supports
> > > pause frames, and that's done through either setting the .features
> > > member in the PHY driver, or by providing a .get_features
> > > implementation.
> > >
> > > Configuration of the pause advertisement should already be happening
> > > through the core phylib code.
> > 
> > I really should do a LPC netdev talk "Everybody gets pause wrong..."
> > 
> > genphy_c45_an_config_aneg() will configure pause advertisement. The PHY
> > driver does not need to configure it, if the PHY follows the standard and has the
> > configuration in the correct place. As Russell said, please check the PHYs ability
> > to advertise pause is being reported correctly, by .get_features, of the default
> > implementation of .get_features if that is being used. And then check your MAC
> > driver is also indicating it supports pause.
> From .get_features, it is not possible to check PHY's ability to advertise pause is being reported as there is no such register present for AQR PHY to check capabilities in its datasheet.
> Hence, we are directly configuring the pause frames from  aqr107_config_init().

... and thus creating a trashy implementation... so NAK.

The first thing to get straight is that in "normal" non-rate adapting
setups, pause frames are no different from normal Ethernet frames as
far as a PHY is concerned. The PHY should be doing absolutely nothing
special with pause frames - it should merely forward them to the MAC,
and it's the MAC that deals with pause frames.

The only thing that a non-rate adapting baseT PHY has to deal with is
the media advertisement and nothing else.

So, whether pause frames are supported has more to do with the MAC
than the PHY.

The way phylib works is that when a PHY is probed, phy_probe() will
set the Pause and Asym_Pause bits in the ->supported bitmap. It is
then up to the MAC driver (or phylink) to call phy_support_asym_pause()
or phy_support_sym_pause() to tell phylib what the MAC supports.

PHY drivers must *not* override this in their config_init() function,
and most certainly must not copy the supported bitmap to the advertising
bitmap there either.

If you need pause-mode rate adaption, this has nothing to do with the
media side, and ->supported and ->advertising are not relevant for
that. Non-phylink based MAC drivers have to use phy_get_rate_matching()
to find out whether the PHY will be using rate adaption and act
accordingly. phylink based MAC drivers have this dealt with for them
and as long as they do what phylink tells them to do, everything
should be fine.

So, basically, do not mess with setting bits in the ->supported bitmap
nor the ->advertising bitmap in config_init. It is wrong, and we will
NAK it.

Thanks.

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

Re: [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY

[Russell King (Oracle)]

Final comments on the series, posted here because there's no cover
message.

1. The series should include a cover message summarising the overall
   purpose of the series is, giving an overview of the changes, and a
   diffstat for the whole series.

2. The subject line should contain the tree that the patches are
   targetting, e.g. "[PATCH net-next 0/4] Add support for WoL to Aquantia
   PHY driver".

3. The tree should be identified in each patch mailed out.

4. The patches should be threaded to the cover message.

Thanks.

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