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.
