| Message ID | 20190207094939.27369-8-maxime.chevallier@bootlin.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: phy: Add support for 2.5GBASET PHYs | expand |
> 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; > } Maybe swap to a switch statement? > static struct phy_driver mv3310_drivers[] = { > { > - .phy_id = 0x002b09aa, > + .phy_id = MARVELL_PHY_ID_88X3310, What does the datasheet say about the lower nibble? Often it is a silicon revision field, so you don't match on it. But 0xa is a rather odd revision. Thanks Andrew
On Thu, Feb 07, 2019 at 10:49:36AM +0100, Maxime Chevallier wrote: > 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 Hi Maxime, Looking deeper at this, I think we actually need an additional patch at the beginning of your series. The default AN advertisement in 7.32 is 0x1181 - which includes the 2.5G and 5G modes. We need to clear these bits, so that when the 10G mode disabled via ethtool, we do not switch to 2.5G or 5G speed (both of which are not currently reported as supported.) Such a patch needs backporting to stable kernels. > - 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> > --- > V1 -> V2: Use a #define for the 88X3310 PHY ID, since it's reused in > various places in the code. Rebased on Heiner Kallweit's patch > introducing the phy_modify_mmd accessor. > > drivers/net/phy/marvell10g.c | 30 ++++++++++++++++++++++-------- > include/linux/marvell_phy.h | 1 + > 2 files changed, 23 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c > index 07df87b81369..581b4b6e31e9 100644 > --- a/drivers/net/phy/marvell10g.c > +++ b/drivers/net/phy/marvell10g.c > @@ -238,6 +238,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) > @@ -307,8 +308,18 @@ 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 = phy_modify_mmd(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) > @@ -337,17 +348,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; > } > } > @@ -450,7 +464,7 @@ static int mv3310_read_status(struct phy_device *phydev) > > static struct phy_driver mv3310_drivers[] = { > { > - .phy_id = 0x002b09aa, > + .phy_id = MARVELL_PHY_ID_88X3310, > .phy_id_mask = MARVELL_PHY_ID_MASK, > .name = "mv88x3310", > .features = PHY_10GBIT_FEATURES, > @@ -468,7 +482,7 @@ static struct phy_driver mv3310_drivers[] = { > module_phy_driver(mv3310_drivers); > > static struct mdio_device_id __maybe_unused mv3310_tbl[] = { > - { 0x002b09aa, MARVELL_PHY_ID_MASK }, > + { MARVELL_PHY_ID_88X3310, MARVELL_PHY_ID_MASK }, > { }, > }; > MODULE_DEVICE_TABLE(mdio, mv3310_tbl); > diff --git a/include/linux/marvell_phy.h b/include/linux/marvell_phy.h > index 1eb6f244588d..5851d68d828a 100644 > --- a/include/linux/marvell_phy.h > +++ b/include/linux/marvell_phy.h > @@ -20,6 +20,7 @@ > #define MARVELL_PHY_ID_88E1540 0x01410eb0 > #define MARVELL_PHY_ID_88E1545 0x01410ea0 > #define MARVELL_PHY_ID_88E3016 0x01410e60 > +#define MARVELL_PHY_ID_88X3310 0x002b09aa > > /* The MV88e6390 Ethernet switch contains embedded PHYs. These PHYs do > * not have a model ID. So the switch driver traps reads to the ID2 > -- > 2.19.2 > >
Hello Russell, On Thu, 7 Feb 2019 23:48:24 +0000 Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: >On Thu, Feb 07, 2019 at 10:49:36AM +0100, Maxime Chevallier wrote: >> 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 > >Hi Maxime, > >Looking deeper at this, I think we actually need an additional patch at >the beginning of your series. > >The default AN advertisement in 7.32 is 0x1181 - which includes the >2.5G and 5G modes. We need to clear these bits, so that when the 10G >mode disabled via ethtool, we do not switch to 2.5G or 5G speed (both >of which are not currently reported as supported.) Such a patch needs >backporting to stable kernels. Good catch. The issue seems fixed by Andrew's patch : 3de97f3c6308 ("net: phy: marvell10g: use genphy_c45_an_config_aneg") However, the fix should indeed be backported to the -stable trees, I've been able to repdocude this on 4.20. I'll take care of sending a patch to -net for that. Thanks for reporting this, Maxime
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c index 07df87b81369..581b4b6e31e9 100644 --- a/drivers/net/phy/marvell10g.c +++ b/drivers/net/phy/marvell10g.c @@ -238,6 +238,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) @@ -307,8 +308,18 @@ 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 = phy_modify_mmd(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) @@ -337,17 +348,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; } } @@ -450,7 +464,7 @@ static int mv3310_read_status(struct phy_device *phydev) static struct phy_driver mv3310_drivers[] = { { - .phy_id = 0x002b09aa, + .phy_id = MARVELL_PHY_ID_88X3310, .phy_id_mask = MARVELL_PHY_ID_MASK, .name = "mv88x3310", .features = PHY_10GBIT_FEATURES, @@ -468,7 +482,7 @@ static struct phy_driver mv3310_drivers[] = { module_phy_driver(mv3310_drivers); static struct mdio_device_id __maybe_unused mv3310_tbl[] = { - { 0x002b09aa, MARVELL_PHY_ID_MASK }, + { MARVELL_PHY_ID_88X3310, MARVELL_PHY_ID_MASK }, { }, }; MODULE_DEVICE_TABLE(mdio, mv3310_tbl); diff --git a/include/linux/marvell_phy.h b/include/linux/marvell_phy.h index 1eb6f244588d..5851d68d828a 100644 --- a/include/linux/marvell_phy.h +++ b/include/linux/marvell_phy.h @@ -20,6 +20,7 @@ #define MARVELL_PHY_ID_88E1540 0x01410eb0 #define MARVELL_PHY_ID_88E1545 0x01410ea0 #define MARVELL_PHY_ID_88E3016 0x01410e60 +#define MARVELL_PHY_ID_88X3310 0x002b09aa /* The MV88e6390 Ethernet switch contains embedded PHYs. These PHYs do * not have a model ID. So the switch driver traps reads to the ID2
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> --- V1 -> V2: Use a #define for the 88X3310 PHY ID, since it's reused in various places in the code. Rebased on Heiner Kallweit's patch introducing the phy_modify_mmd accessor. drivers/net/phy/marvell10g.c | 30 ++++++++++++++++++++++-------- include/linux/marvell_phy.h | 1 + 2 files changed, 23 insertions(+), 8 deletions(-)