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

[Ivan Bornyakov]

On Sat, Feb 20, 2021 at 11:53:04AM +0000, Russell King - ARM Linux admin wrote:
> On Sat, Feb 20, 2021 at 12:46:23PM +0300, Ivan Bornyakov wrote:
> > +
> > +	switch (sfp_interface) {
> > +	case PHY_INTERFACE_MODE_10GBASER:
> > +		phydev->speed = SPEED_10000;
> > +		phy_write_mmd(phydev, MDIO_MMD_VEND2, MV_PCS_CONFIG,
> > +			      MV_PCS_HOST_XAUI | MV_PCS_LINE_10GBR);
> > +		break;
> > +	case PHY_INTERFACE_MODE_1000BASEX:
> > +		phydev->speed = SPEED_1000;
> > +		phy_write_mmd(phydev, MDIO_MMD_VEND2, MV_PCS_CONFIG,
> > +			      MV_PCS_HOST_XAUI | MV_PCS_LINE_1GBX_AN);
> > +		break;
> > +	case PHY_INTERFACE_MODE_SGMII:
> > +		phydev->speed = SPEED_1000;
> > +		phy_write_mmd(phydev, MDIO_MMD_VEND2, MV_PCS_CONFIG,
> > +			      MV_PCS_HOST_XAUI | MV_PCS_LINE_SGMII_AN);
> > +		phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_1GBX_CTRL,
> > +			       BMCR_SPEED1000 | BMCR_SPEED100, BMCR_SPEED1000);
> 
> Isn't this forcing 1000Mbit, but SGMII relies on AN for the slower
> speeds.
> 

It was intended as default, but you have a good point, there is no need
for this, I can just trigger config_aneg() instead.

> > +		break;
> > +	default:
> > +		dev_err(dev, "Incompatible SFP module inserted\n");
> > +
> > +		return -EINVAL;
> > +	}
> 
> I don't think you should set phydev->speed in this function - apart
> from the rtnl lock, there is no other locking here, so this is fragile.
> 
> > +	linkmode_and(phydev->supported, priv->supported, sfp_supported);
> 
> I don't think this is a good idea; phylink does not expect the supported
> mask to change, and I suspect _no_ network device expects it to change.
> One of the things that network drivers and phylink does is to adjust the
> supported mask for a PHY according to the capabilities of the network
> device. For example, if they don't support pause modes, or something
> else. Overriding it in this way has the possibility to re-introduce
> modes that the network driver does not support.
> 

OK, but how can I exclude modes unsupported by inserted SFP?
Or I shouldn't exclude any at all?

> > +/* switch line-side interface between 10GBase-R and 1GBase-X
> > + * according to speed */
> > +static void mv2222_update_interface(struct phy_device *phydev)
> > +{
> > +	struct mv2222_data *priv = phydev->priv;
> > +
> > +	if (phydev->speed == SPEED_10000 &&
> > +	    priv->line_interface == PHY_INTERFACE_MODE_1000BASEX) {
> > +		priv->line_interface = PHY_INTERFACE_MODE_10GBASER;
> > +
> > +		phy_write_mmd(phydev, MDIO_MMD_VEND2, MV_PCS_CONFIG,
> > +			      MV_PCS_HOST_XAUI | MV_PCS_LINE_10GBR);
> > +		mv2222_soft_reset(phydev);
> > +	}
> > +
> > +	if (phydev->speed == SPEED_1000 &&
> > +	    priv->line_interface == PHY_INTERFACE_MODE_10GBASER) {
> > +		priv->line_interface = PHY_INTERFACE_MODE_1000BASEX;
> > +
> > +		phy_write_mmd(phydev, MDIO_MMD_VEND2, MV_PCS_CONFIG,
> > +			      MV_PCS_HOST_XAUI | MV_PCS_LINE_1GBX_AN);
> > +		mv2222_soft_reset(phydev);
> > +	}
> 
> Wouldn't it be better to have a single function to set the line
> interface, used by both this function and your sfp_module_insert
> function? I'm thinking something like:
> 
> static int mv2222_set_line_interface(struct phy_device *phydev,
> 				     phy_interface_t line_interface)
> {
> ...
> }
> 
> and calling that from both these locations to configure the PHY for
> 10GBASE-R, 1000BASE-X and SGMII modes.
> 

I'll think about it, thanks.

> > +
> > +static int mv2222_config_aneg(struct phy_device *phydev)
> > +{
> > +	struct mv2222_data *priv = phydev->priv;
> > +	int ret, adv;
> > +
> > +	/* SFP is not present, do nothing */
> > +	if (priv->line_interface == PHY_INTERFACE_MODE_NA)
> > +		return 0;
> > +
> > +	if (phydev->autoneg == AUTONEG_DISABLE ||
> > +	    phydev->speed == SPEED_10000) {
> > +		if (phydev->speed == SPEED_10000 &&
> > +		    !mv2222_is_10g_capable(phydev))
> > +			return -EINVAL;
> > +
> > +		if (priv->line_interface == PHY_INTERFACE_MODE_SGMII) {
> > +			ret = mv2222_set_sgmii_speed(phydev);
> > +			if (ret < 0)
> > +				return ret;
> > +		} else {
> > +			mv2222_update_interface(phydev);
> > +		}
> > +
> > +		return mv2222_disable_aneg(phydev);
> > +	}
> > +
> > +	/* Try 10G first */
> > +	if (mv2222_is_10g_capable(phydev)) {
> > +		phydev->speed = SPEED_10000;
> > +		mv2222_update_interface(phydev);
> > +
> > +		ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_10GBR_STAT_RT);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		if (ret & MDIO_STAT1_LSTATUS) {
> > +			phydev->autoneg = AUTONEG_DISABLE;
> > +
> > +			return mv2222_disable_aneg(phydev);
> > +		}
> > +
> > +		/* 10G link was not established, switch back to 1G
> > +		 * and proceed with true autonegotiation */
> > +		phydev->speed = SPEED_1000;
> > +		mv2222_update_interface(phydev);
> 
> This doesn't look right. If the user specifies that they want 10G,
> why should we switch back to 1G?
> 

This is for enabled autoneg. Try 10g, if link is established - stay and
disable autoneg, otherwise continue with true autonegotiation.
For explicitly specified 10g mode there is case above.

Thanks again for in-depth review, Russell.