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

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

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