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

[Frank.Sae]

On 7/29/24 04:41, Russell King (Oracle) wrote:
> 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!
MAC facing interface will be switched according to phy interface. when phy
media side state is link down, the mac facing interface will not be
configured, this refers to Marvell10g.c(mv3310_update_interface) and
Realtek.c(rtl822xb_update_interface).