> -----Original Message-----
> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: Wednesday, June 28, 2023 7:48 PM
> To: Revanth Kumar Uppala <ruppala@xxxxxxxxxx>
> Cc: linux@xxxxxxxxxxxxxxx; 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
>
>
> > +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
