Messages in this thread | | | Date | Sat, 20 Feb 2021 18:51:31 +0300 | From | Ivan Bornyakov <> | Subject | Re: [PATCH v2] net: phy: add Marvell 88X2222 transceiver support |
| |
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.
| |