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

-----Original Message-----
From: Andrew Lunn <andrew@lunn.ch>
Sent: Wednesday, June 28, 2023 7:48 PM
To: Revanth Kumar Uppala <redacted>
Cc: linux@armlinux.org.uk; hkallweit1@gmail.com; netdev@vger.kernel.org;
linux-tegra@vger.kernel.org; Narayan Reddy [off-list ref]
Subject: Re: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL)

External email: Use caution opening links or attachments

quoted

+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,

quoted

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

quoted

@@ -619,6 +784,31 @@ static int aqr107_config_init(struct phy_device

*phydev)

quoted

      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,

quoted

+                         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

quoted

+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);

quoted

+     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