| Message ID | 20190118152352.26417-5-maxime.chevallier@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: phy: Add support for 2.5GBASET PHYs | expand |
On Fri, Jan 18, 2019 at 04:23:49PM +0100, Maxime Chevallier wrote: > @@ -264,8 +265,10 @@ static int mv3310_config_init(struct phy_device *phydev) > if (ret) > return ret; > > - linkmode_and(phydev->advertising, phydev->advertising, > - phydev->supported); > + /* Make sure we advertise all the supported modes, and not just the > + * default one specified in the driver's .features. > + */ > + linkmode_copy(phydev->advertising, phydev->supported); This looks wrong to me, it tramples over the work that phy_probe() did, specifically with regard to the pause mode bits. Please see the comments in that function about the pause mode bits.
> @@ -264,8 +265,10 @@ static int mv3310_config_init(struct phy_device *phydev) > if (ret) > return ret; > > - linkmode_and(phydev->advertising, phydev->advertising, > - phydev->supported); > + /* Make sure we advertise all the supported modes, and not just the > + * default one specified in the driver's .features. > + */ > + linkmode_copy(phydev->advertising, phydev->supported); Hi Maxime What Russell is referring to is: static int phy_probe(struct device *dev) { .. /* Start out supporting everything. Eventually, * a controller will attach, and may modify one * or both of these values */ linkmode_copy(phydev->supported, phydrv->features); of_set_phy_supported(phydev); linkmode_copy(phydev->advertising, phydev->supported); /* Get the EEE modes we want to prohibit. We will ask * the PHY stop advertising these mode later on */ of_set_phy_eee_broken(phydev); /* The Pause Frame bits indicate that the PHY can support passing * pause frames. During autonegotiation, the PHYs will determine if * they should allow pause frames to pass. The MAC driver should then * use that result to determine whether to enable flow control via * pause frames. * * Normally, PHY drivers should not set the Pause bits, and instead * allow phylib to do that. However, there may be some situations * (e.g. hardware erratum) where the driver wants to set only one * of these bits. */ if (test_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydrv->features) || test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydrv->features)) { linkmode_clear_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev->supported); linkmode_clear_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev->supported); if (test_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydrv->features)) linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev->supported); if (test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydrv->features)) linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev->supported); } else { linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev->supported); linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev->supported); } So by doing a copy of supported into advertising, you can stomping over any restrictions applied via of_set_phy_supported(), of_set_phy_eee_broken(phydev), and any pause control settings which might of happened. What might make sense here is that a PHY driver can replace its .features member at run time, in its config_init() call. The core then needs to perform these evaluations. So i'm guessing we need to split this code out of probe() and move it into phy_init_hw()? Heiner, you know this code better than anybody. What do you think? Andrew
Hello Andrew, Russell, On Mon, 21 Jan 2019 21:17:15 +0100 Andrew Lunn <andrew@lunn.ch> wrote: >> @@ -264,8 +265,10 @@ static int mv3310_config_init(struct phy_device *phydev) >> if (ret) >> return ret; >> >> - linkmode_and(phydev->advertising, phydev->advertising, >> - phydev->supported); >> + /* Make sure we advertise all the supported modes, and not just the >> + * default one specified in the driver's .features. >> + */ >> + linkmode_copy(phydev->advertising, phydev->supported); > >So by doing a copy of supported into advertising, you can stomping >over any restrictions applied via of_set_phy_supported(), >of_set_phy_eee_broken(phydev), and any pause control settings which >might of happened. Thanks for the explanations, this is indeed clearly not a good solution. >What might make sense here is that a PHY driver can replace its >.features member at run time, in its config_init() call. The core then >needs to perform these evaluations. So i'm guessing we need to split >this code out of probe() and move it into phy_init_hw()? So the .features won't be read-only anymore ? We could also simply make a helper that would add a mode to both the supported and advertising modes list, that would be used in the 'genphy_c45_pma_read_abilities' and config_init ? I lack the big picture of the PHY init sequence, there seems to be a lot of quirks and complex cases that we need to take into account, so I'll let you decide :) >Heiner, you know this code better than anybody. What do you think? > > Andrew Thanks for the feedback, Maxime
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c index f2a6d6e7041a..f45ddf3bc138 100644 --- a/drivers/net/phy/marvell10g.c +++ b/drivers/net/phy/marvell10g.c @@ -255,6 +255,7 @@ static int mv3310_config_init(struct phy_device *phydev) /* Check that the PHY interface type is compatible */ if (phydev->interface != PHY_INTERFACE_MODE_SGMII && + phydev->interface != PHY_INTERFACE_MODE_2500BASEX && phydev->interface != PHY_INTERFACE_MODE_XAUI && phydev->interface != PHY_INTERFACE_MODE_RXAUI && phydev->interface != PHY_INTERFACE_MODE_10GKR) @@ -264,8 +265,10 @@ static int mv3310_config_init(struct phy_device *phydev) if (ret) return ret; - linkmode_and(phydev->advertising, phydev->advertising, - phydev->supported); + /* Make sure we advertise all the supported modes, and not just the + * default one specified in the driver's .features. + */ + linkmode_copy(phydev->advertising, phydev->supported); return 0; } @@ -314,8 +317,17 @@ static int mv3310_config_aneg(struct phy_device *phydev) else reg = 0; + if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, + phydev->advertising)) + reg |= MDIO_AN_10GBT_CTRL_ADV2_5G; + if (linkmode_test_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, + phydev->advertising)) + reg |= MDIO_AN_10GBT_CTRL_ADV5G; + ret = mv3310_modify(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL, - MDIO_AN_10GBT_CTRL_ADV10G, reg); + MDIO_AN_10GBT_CTRL_ADV10G | + MDIO_AN_10GBT_CTRL_ADV5G | + MDIO_AN_10GBT_CTRL_ADV2_5G, reg); if (ret < 0) return ret; if (ret > 0) @@ -344,17 +356,20 @@ static int mv3310_aneg_done(struct phy_device *phydev) static void mv3310_update_interface(struct phy_device *phydev) { if ((phydev->interface == PHY_INTERFACE_MODE_SGMII || + phydev->interface == PHY_INTERFACE_MODE_2500BASEX || phydev->interface == PHY_INTERFACE_MODE_10GKR) && phydev->link) { /* The PHY automatically switches its serdes interface (and - * active PHYXS instance) between Cisco SGMII and 10GBase-KR - * modes according to the speed. Florian suggests setting - * phydev->interface to communicate this to the MAC. Only do - * this if we are already in either SGMII or 10GBase-KR mode. + * active PHYXS instance) between Cisco SGMII, 10GBase-KR and + * 2500BaseX modes according to the speed. Florian suggests + * setting phydev->interface to communicate this to the MAC. + * Only do this if we are already in one of the above modes. */ if (phydev->speed == SPEED_10000) phydev->interface = PHY_INTERFACE_MODE_10GKR; + else if (phydev->speed == SPEED_2500) + phydev->interface = PHY_INTERFACE_MODE_2500BASEX; else if (phydev->speed >= SPEED_10 && - phydev->speed < SPEED_10000) + phydev->speed < SPEED_2500) phydev->interface = PHY_INTERFACE_MODE_SGMII; } }
The Marvell Alaska family of PHYs supports 2.5GBaseT and 5GBaseT modes, as defined in the 802.3bz specification. When the link partner requests a 2.5GBASET link, the PHY will reconfigure it's MII interface to 2500BASEX. At 5G, the PHY will reconfigure it's interface to 5GBASE-R, but this mode isn't supported by any MAC for now. This was tested with : - The 88X3310, which is on the MacchiatoBin - The 88E2010, an Alaska PHY that has no fiber interfaces, and is limited to 5G maximum speed. Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> --- drivers/net/phy/marvell10g.c | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-)