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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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:

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.

These bits apply to all the VEND1_GLOBAL_CFG_* registers, so these
should be defined after the last register (0x031f).

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

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!