Re: [PATCH] net: phy: add Marvell 88X2222 transceiver support

[Andrew Lunn <andrew@lunn.ch>]

> +static int mv2222_config_init(struct phy_device *phydev)
> +{
> +	linkmode_zero(phydev->supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT, phydev->supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT, phydev->supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseKR_Full_BIT, phydev->supported);
> +
> +	phydev->pause = 0;
> +	phydev->asym_pause = 0;
> +	phydev->duplex = DUPLEX_FULL;
> +	phydev->autoneg = AUTONEG_DISABLE;
> +
> +	return 0;
> +}
> +
> +static void mv2222_update_interface(struct phy_device *phydev)
> +{
> +	if ((phydev->speed == SPEED_1000 ||
> +	     phydev->speed == SPEED_100 ||
> +	     phydev->speed == SPEED_10) &&
> +	    phydev->interface != PHY_INTERFACE_MODE_1000BASEX) {
> +		phydev->interface = PHY_INTERFACE_MODE_1000BASEX;

The speeds 10 and 100 seem odd here. 1000BaseX only supports 1G. It
would have to be SGMII in order to also support 10Mbps and 100Mbps.
Plus you are not listing 10 and 100 as a supported value.

> +/* Returns negative on error, 0 if link is down, 1 if link is up */
> +static int mv2222_read_status_1g(struct phy_device *phydev)
> +{
> +	int val, link = 0;
> +
> +	val = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_1GBX_STAT);
> +	if (val < 0)
> +		return val;
> +
> +	if (!(val & MDIO_STAT1_LSTATUS) ||
> +	    (phydev->autoneg == AUTONEG_ENABLE && !(val & MDIO_AN_STAT1_COMPLETE)))
> +		return 0;
> +
> +	link = 1;
> +
> +	if (phydev->autoneg == AUTONEG_DISABLE) {
> +		phydev->speed = SPEED_1000;
> +		phydev->duplex = DUPLEX_FULL;
> +
> +		return link;
> +	}
> +
> +	val = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_1GBX_PHY_STAT);
> +	if (val < 0)
> +		return val;
> +
> +	if (val & MV_1GBX_PHY_STAT_AN_RESOLVED) {
> +		if (val & MV_1GBX_PHY_STAT_DUPLEX)
> +			phydev->duplex = DUPLEX_FULL;
> +		else
> +			phydev->duplex = DUPLEX_HALF;
> +
> +		if (val & MV_1GBX_PHY_STAT_SPEED1000)
> +			phydev->speed = SPEED_1000;
> +		else if (val & MV_1GBX_PHY_STAT_SPEED100)
> +			phydev->speed = SPEED_100;
> +		else
> +			phydev->speed = SPEED_10;

Are you sure it is not doing SGMII? Maybe it can do both, 1000BaseX
and SGMII? You would generally use 1000BaseX to connect to a fibre
SFP, and SGMII to connect to a copper SFP. So ideally you want to be
able to swap between these modes as needed.

> +static int mv2222_read_status(struct phy_device *phydev)
> +{
> +	int link;
> +
> +	linkmode_zero(phydev->lp_advertising);
> +	phydev->pause = 0;
> +	phydev->asym_pause = 0;
> +
> +	switch (phydev->interface) {
> +	case PHY_INTERFACE_MODE_10GBASER:
> +		link = mv2222_read_status_10g(phydev);
> +		break;
> +	case PHY_INTERFACE_MODE_1000BASEX:
> +	default:
> +		link = mv2222_read_status_1g(phydev);
> +		break;
> +	}
> +
> +	if (link < 0)
> +		return link;
> +
> +	phydev->link = link;
> +
> +	if (phydev->link)
> +		mv2222_link_led_on(phydev);
> +	else
> +		mv2222_link_led_off(phydev);

You have to manually control the LED? That is odd for a PHY. Normally
you just select a mode and it will do it all in hardware.

    Andrew