Re: [PATCH 2/2] net: phy: Add driver for Motorcomm yt8821 2.5G ethernet phy

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

On Sat, Jul 27, 2024 at 02:20:31AM -0700, Frank.Sae wrote:
> +/**
> + * yt8821_config_init() - phy initializatioin
> + * @phydev: a pointer to a &struct phy_device
> + *
> + * returns 0 or negative errno code
> + */
> +static int yt8821_config_init(struct phy_device *phydev)
> +{
> +	struct yt8821_priv *priv = phydev->priv;
> +	int ret, val;
> +
> +	phydev->irq = PHY_POLL;
> +
> +	val = ytphy_read_ext_with_lock(phydev, YT8521_CHIP_CONFIG_REG);
> +	if (priv->chip_mode == YT8821_CHIP_MODE_AUTO_BX2500_SGMII) {
> +		ret = ytphy_modify_ext_with_lock(phydev,
> +						 YT8521_CHIP_CONFIG_REG,
> +						 YT8521_CCR_MODE_SEL_MASK,
> +						 FIELD_PREP(YT8521_CCR_MODE_SEL_MASK, 0));
> +		if (ret < 0)
> +			return ret;
> +
> +		__assign_bit(PHY_INTERFACE_MODE_2500BASEX,
> +			     phydev->possible_interfaces,
> +			     true);
> +		__assign_bit(PHY_INTERFACE_MODE_SGMII,
> +			     phydev->possible_interfaces,
> +			     true);

Before each and every call to .config_init, phydev->possible_interfaces
will be cleared. So, please use __set_bit() here.

> +static int yt8821_read_status(struct phy_device *phydev)
> +{
> +	struct yt8821_priv *priv = phydev->priv;
> +	int old_page;
> +	int ret = 0;
> +	int link;
> +	int val;
> +
> +	if (phydev->autoneg == AUTONEG_ENABLE) {
> +		int lpadv = phy_read_mmd(phydev,
> +					 MDIO_MMD_AN, MDIO_AN_10GBT_STAT);
> +
> +		if (lpadv < 0)
> +			return lpadv;
> +
> +		mii_10gbt_stat_mod_linkmode_lpa_t(phydev->lp_advertising,
> +						  lpadv);
> +	}
> +
> +	ret = ytphy_write_ext_with_lock(phydev,
> +					YT8521_REG_SPACE_SELECT_REG,
> +					YT8521_RSSR_UTP_SPACE);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = genphy_read_status(phydev);
> +	if (ret < 0)
> +		return ret;
> +
> +	old_page = phy_select_page(phydev, YT8521_RSSR_UTP_SPACE);
> +	if (old_page < 0)
> +		goto err_restore_page;
> +
> +	val = __phy_read(phydev, YTPHY_SPECIFIC_STATUS_REG);
> +	if (val < 0) {
> +		ret = val;
> +		goto err_restore_page;
> +	}
> +
> +	link = val & YTPHY_SSR_LINK;

What link status is this reporting? For interface switching to work,
phydev->link must _only_ indicate whether the _media_ side interface
is up or down. It must _not_ include the status of the MAC facing
interface from the PHY.

Why? The interface configuration of the MAC is only performed when
the _media_ link comes up, denoted by phydev->link becoming true.
If the MAC interface configuration mismatches the PHY interface
configuration, then the MAC facing interface of the PHY will
remain down, and if phydev->link is forced to false, then the link
will never come up.

So, I hope that this isn't testing the MAC facing interface status
of the PHY!

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