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

[Revanth Kumar Uppala <ruppala@nvidia.com>]

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