Message-ID: <4a039a07-ab73-efa3-96d5-d109438f4575@seco.com>
Date:   Fri, 18 Nov 2022 10:11:06 -0500
From:   Sean Anderson <sean.anderson@...o.com>
To:     Vladimir Oltean <vladimir.oltean@....com>, netdev@...r.kernel.org
Cc:     "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Russell King <linux@...linux.org.uk>,
        Florian Fainelli <f.fainelli@...il.com>,
        UNGLinuxDriver@...rochip.com,
        bcm-kernel-feedback-list@...adcom.com,
        Madalin Bucur <madalin.bucur@....nxp.com>,
        Camelia Groza <camelia.groza@....com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Ioana Ciornei <ioana.ciornei@....com>,
        Maxim Kochetkov <fido_max@...ox.ru>,
        Antoine Tenart <atenart@...nel.org>,
        Michael Walle <michael@...le.cc>,
        Raag Jadav <raagjadav@...il.com>,
        Siddharth Vadapalli <s-vadapalli@...com>,
        Ong Boon Leong <boon.leong.ong@...el.com>,
        Colin Foster <colin.foster@...advantage.com>,
        Marek Behun <marek.behun@....cz>
Subject: Re: [PATCH v4 net-next 2/8] net: phylink: introduce generic method to
 query PHY in-band autoneg capability

On 11/17/22 19:01, Vladimir Oltean wrote:
> Currently, phylink requires that fwnodes with links to SFP cages have
> the 'managed = "in-band-status"' property, and based on this, the
> initial pl->cfg_link_an_mode gets set to MLO_AN_INBAND.
> 
> However, some PHYs on SFP modules may have broken in-band autoneg, and
> in that case, phylink selects a pl->cur_link_an_mode which is MLO_AN_PHY,
> to tell the MAC/PCS side to disable in-band autoneg (link speed/status
> will come over the MDIO side channel).
> 
> The check for PHY in-band autoneg capability is currently open-coded
> based on a PHY ID comparison against the BCM84881. But the same problem
> will also be need to solved in another case, where syncing in-band
> autoneg will be desired between the MAC/PCS and an on-board PHY.
> So the approach needs to be generalized, and eventually what is done for
> the BCM84881 needs to be replaced with a more generic solution.
> 
> Add new API to the PHY device structure which allows it to report what
> it supports in terms of in-band autoneg (whether it can operate with it
> on, and whether it can operate with it off). The assumption is that
> there is a Clause 37 compatible state machine in the PHY's PCS, and it
> requires that the autoneg process completes before the lane transitions
> to data mode. If we have a mismatch between in-band autoneg modes, the
> system side link will be broken.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
> v3->v4:
> - split the SFP cur_link_an_mode fixup to separate patch (this one)
> - s/inband_aneg/an_inband/ to be more in line with phylink terminology
> - clearer documentation, added kerneldocs
> - don't return -EIO in phy_validate_an_inband(), this breaks with the
>   Generic PHY driver because the expected return code is a bit mask, not
>   a negative integer
> 
>  drivers/net/phy/phy.c     | 25 +++++++++++++++++++++++++
>  drivers/net/phy/phylink.c | 20 +++++++++++++++++---
>  include/linux/phy.h       | 17 +++++++++++++++++
>  3 files changed, 59 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index e5b6cb1a77f9..2abbacf2c7cb 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -733,6 +733,31 @@ static int phy_check_link_status(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +/**
> + * phy_validate_an_inband - validate which in-band autoneg modes are supported
> + * @phydev: the phy_device struct
> + * @interface: the MAC-side interface type
> + *
> + * Returns @PHY_AN_INBAND_UNKNOWN if it is unknown what in-band autoneg setting
> + * is required for the given PHY mode, or a bit mask of @PHY_AN_INBAND_OFF (if
> + * the PHY is able to work with in-band AN turned off) and @PHY_AN_INBAND_ON
> + * (if it works with the feature turned on). With the Generic PHY driver, the
> + * result will always be @PHY_AN_INBAND_UNKNOWN.
> + */
> +int phy_validate_an_inband(struct phy_device *phydev,
> +			   phy_interface_t interface)
> +{
> +	/* We may be called before phy_attach_direct() force-binds the
> +	 * generic PHY driver to this device. In that case, report an unknown
> +	 * setting rather than -EIO as most other functions do.
> +	 */
> +	if (!phydev->drv || !phydev->drv->validate_an_inband)
> +		return PHY_AN_INBAND_UNKNOWN;
> +
> +	return phydev->drv->validate_an_inband(phydev, interface);
> +}
> +EXPORT_SYMBOL_GPL(phy_validate_an_inband);
> +
>  /**
>   * _phy_start_aneg - start auto-negotiation for this PHY device
>   * @phydev: the phy_device struct
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 9e4b2dfc98d8..40b7e730fb33 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -2936,10 +2936,24 @@ static int phylink_sfp_config_phy(struct phylink *pl, struct phy_device *phy)
>  		return -EINVAL;
>  	}
>  
> -	if (phylink_phy_no_inband(phy))
> -		mode = MLO_AN_PHY;
> -	else
> +	/* Select whether to operate in in-band mode or not, based on the
> +	 * capability of the PHY in the current link mode.
> +	 */
> +	ret = phy_validate_an_inband(phy, iface);
> +	if (ret == PHY_AN_INBAND_UNKNOWN) {
> +		if (phylink_phy_no_inband(phy))
> +			mode = MLO_AN_PHY;
> +		else
> +			mode = MLO_AN_INBAND;
> +
> +		phylink_dbg(pl,
> +			    "PHY driver does not report in-band autoneg capability, assuming %s\n",
> +			    phylink_autoneg_inband(mode) ? "true" : "false");
> +	} else if (ret & PHY_AN_INBAND_ON) {
>  		mode = MLO_AN_INBAND;
> +	} else {
> +		mode = MLO_AN_PHY;
> +	}
>  
>  	config.interface = iface;
>  	linkmode_copy(support1, support);
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 9a3752c0c444..56a431d88dd9 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -761,6 +761,12 @@ struct phy_tdr_config {
>  };
>  #define PHY_PAIR_ALL -1
>  
> +enum phy_an_inband {
> +	PHY_AN_INBAND_UNKNOWN		= BIT(0),

Shouldn't this be something like

	PHY_AN_INBAND_UNKNOWN		= 0,

? What does it mean if a phy returns e.g. 0b101?

--Sean

> +	PHY_AN_INBAND_OFF		= BIT(1),
> +	PHY_AN_INBAND_ON		= BIT(2),
> +};
> +
>  /**
>   * struct phy_driver - Driver structure for a particular PHY type
>   *
> @@ -845,6 +851,15 @@ struct phy_driver {
>  	 */
>  	int (*config_aneg)(struct phy_device *phydev);
>  
> +	/**
> +	 * @validate_an_inband: Report what types of in-band auto-negotiation
> +	 * are available for the given PHY interface type. Returns a bit mask
> +	 * of type enum phy_an_inband. Returning negative error codes is not
> +	 * permitted.
> +	 */
> +	int (*validate_an_inband)(struct phy_device *phydev,
> +				  phy_interface_t interface);
> +
>  	/** @aneg_done: Determines the auto negotiation result */
>  	int (*aneg_done)(struct phy_device *phydev);
>  
> @@ -1540,6 +1555,8 @@ void phy_stop(struct phy_device *phydev);
>  int phy_config_aneg(struct phy_device *phydev);
>  int phy_start_aneg(struct phy_device *phydev);
>  int phy_aneg_done(struct phy_device *phydev);
> +int phy_validate_an_inband(struct phy_device *phydev,
> +			   phy_interface_t interface);
>  int phy_speed_down(struct phy_device *phydev, bool sync);
>  int phy_speed_up(struct phy_device *phydev);
>  

Powered by blists - more mailing lists