Re: [PATCH net-next v5 5/5] net: phy: add driver for built-in 2.5G ethernet PHY on MT7988

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

On Thu, May 30, 2024 at 11:48:44AM +0800, Sky Huang wrote:
> +static int mt798x_2p5ge_phy_config_aneg(struct phy_device *phydev)
> +{
> +	bool changed = false;
> +	u32 adv;
> +	int ret;
> +
> +	/* In fact, if we disable autoneg, we can't link up correctly:
> +	 *  2.5G/1G: Need AN to exchange master/slave information.
> +	 *  100M: Without AN, link starts at half duplex(According to IEEE 802.3-2018),
> +	 *        which this phy doesn't support.
> +	 *   10M: Deprecated in this ethernet phy.
> +	 */
> +	if (phydev->autoneg == AUTONEG_DISABLE)
> +		return -EOPNOTSUPP;

We have another driver (stmmac) where a platform driver is wanting to
put a hack in the ksettings_set() ethtool path to error out on
disabling AN for 1G speeds. This sounds like something that is
applicable to more than one hardware (and I've been wondering whether
it is universally true that 1G copper links and faster all require
AN to function.)

Thus, I'm wondering whether this is something that the core code should
be doing.

> +	/* This phy can't handle collision, and neither can (XFI)MAC it's connected to.
> +	 * Although it can do HDX handshake, it doesn't support CSMA/CD that HDX requires.
> +	 */

What the MAC can and can't do really has little bearing on what link
modes the PHY driver should be providing. It is the responsibility of
the MAC driver to appropriately change what is supported when attaching
to the PHY. If using phylink, this is done by phylink via the MAC driver
telling phylink what it is capable of via mac_capabilities.

> +static int mt798x_2p5ge_phy_get_rate_matching(struct phy_device *phydev,
> +					      phy_interface_t iface)
> +{
> +	if (iface == PHY_INTERFACE_MODE_XGMII)
> +		return RATE_MATCH_PAUSE;

You mention above XFI...

XFI is 10GBASE-R protocol to XFP module electrical standards.
SFI is 10GBASE-R protocol to SFP+ module electrical standards.

phy_interface_t is interested in the protocol. So, given that you
mention XFI, why doesn't this test for PHY_INTERFACE_MODE_10GBASER?

> +static int mt798x_2p5ge_phy_probe(struct phy_device *phydev)
> +{
> +	struct mtk_i2p5ge_phy_priv *priv;
> +
> +	priv = devm_kzalloc(&phydev->mdio.dev,
> +			    sizeof(struct mtk_i2p5ge_phy_priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	switch (phydev->drv->phy_id) {
> +	case MTK_2P5GPHY_ID_MT7988:
> +		/* The original hardware only sets MDIO_DEVS_PMAPMD */
> +		phydev->c45_ids.mmds_present |= (MDIO_DEVS_PCS | MDIO_DEVS_AN |
> +						 MDIO_DEVS_VEND1 | MDIO_DEVS_VEND2);

No need for parens on the RHS. The RHS is an expression in its own
right, and there's no point in putting parens around the expression
to turn it into another expression!

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