On Mon, Jul 24, 2023 at 11:29:39AM +0000, Revanth Kumar Uppala wrote:
>
>
> > -----Original Message-----
> > From: Russell King <linux@xxxxxxxxxxxxxxx>
> > Sent: Wednesday, June 28, 2023 7:13 PM
> > To: Revanth Kumar Uppala <ruppala@xxxxxxxxxx>
> > Cc: andrew@xxxxxxx; hkallweit1@xxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-
> > tegra@xxxxxxxxxxxxxxx; Narayan Reddy <narayanr@xxxxxxxxxx>
> > 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!
