Re: [PATCH 3/3] net: phy: mediatek: add support for built-in 2.5G ethernet PHY on MT7988

[Andrew Lunn]

> +static int mt7988_2p5ge_phy_config_init(struct phy_device *phydev)
> +{
> +	int ret, i;
> +	const struct firmware *fw;
> +	struct device *dev = &phydev->mdio.dev;
> +	struct device_node *np;
> +	void __iomem *pmb_addr;
> +	void __iomem *md32_en_cfg_base;
> +	struct mtk_i2p5ge_phy_priv *priv = phydev->priv;
> +	u16 reg;
> +	struct pinctrl *pinctrl;

Reverse Christmas Tree please.

> +static int mt7988_2p5ge_phy_config_aneg(struct phy_device *phydev)
> +{
> +	bool changed = false;
> +	u32 adv;
> +	int ret;
> +
> +	if (phydev->autoneg == AUTONEG_DISABLE) {
> +		/* Configure half duplex with genphy_setup_forced,
> +		 * because genphy_c45_pma_setup_forced does not support.
> +		 */

The English in that comment is wrong.

Ah, it appears to be a copy/paste, e.g. from mxl-gpy.c. In fact, a lot
of this function is identical to gpy_config_aneg(). Maybe it should be
pulled out into a helper.

> +static int mt7988_2p5ge_phy_get_features(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = genphy_read_abilities(phydev);
> +	if (ret)
> +		return ret;
> +
> +	/* We don't support HDX at MAC layer on mt7988.
> +	 * So mask phy's HDX capabilities, too.
> +	 */

This comment seems to contradict the comment above? 

Also, this code does not mask anything, it sets bits.  What does
genphy_read_abilities() find out about the device? If the hardware
confirms to the standard, it should not indicate is supports 1/2
duplex, genphy_read_abilities() should then not return those link
modes, but it should return 10, 100 and 1000 full duplex.

> +	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT,
> +			 phydev->supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
> +			 phydev->supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
> +			 phydev->supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
> +			 phydev->supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->supported);

What does genphy_c45_pma_read_abilities() report about the device?

> +static int mt7988_2p5ge_phy_read_status(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = genphy_update_link(phydev);
> +	if (ret)
> +		return ret;
> +
> +	phydev->speed = SPEED_UNKNOWN;
> +	phydev->duplex = DUPLEX_UNKNOWN;
> +	phydev->pause = 0;
> +	phydev->asym_pause = 0;
> +
> +	if (phydev->autoneg == AUTONEG_ENABLE) {
> +		if (phydev->autoneg_complete) {
> +			ret = genphy_c45_read_lpa(phydev);
> +			if (ret < 0)
> +				return ret;
> +
> +			/* Read the link partner's 1G advertisement */
> +			ret = phy_read(phydev, MII_STAT1000);
> +			if (ret < 0)
> +				return ret;
> +			mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising, ret);

> +		} else if (!linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
> +					      phydev->advertising) &&
> +			   linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
> +					     phydev->advertising)) {
> +			extend_an_new_lp_cnt_limit(phydev);
> +		}

A comment about what is happening here would be good.

> +	} else if (phydev->autoneg == AUTONEG_DISABLE) {
> +		linkmode_zero(phydev->lp_advertising);
> +	}

This if does not make much sense.

> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_LINK_STATUS_MISC);
> +	if (ret < 0)
> +		return ret;
> +	phydev->duplex = (ret & MTK_PHY_FDX_ENABLE) ? DUPLEX_FULL : DUPLEX_HALF;

Do does it support half duplex or not?

   Andrew