Re: [PATCH net-next v2 07/10] net: phy: marvell10g: Add support for 2.5GBASET

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

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
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up