[PATCH net-next 0/3] improve clause 45 support in phylink

[PATCH net-next 0/3] improve clause 45 support in phylink

[Russell King - ARM Linux admin]

Hi,

These three patches improve the clause 45 support in phylink, fixing
some corner cases that have been noticed with the addition of SFP+
NBASE-T modules, but are actually a little more wisespread than I
initially realised.

The first issue was spotted with a NBASE-T PHY on a SFP+ module plugged
into a mvneta platform. When these PHYs are not operating in USXGMII
mode, but are in a single-lane Serdes mode, they will switch between
one of several different PHY interface modes.

If we call the MAC validate() function with the current PHY interface
mode, we will restrict the supported and advertising masks to the link
modes that the current PHY interface mode supports. For example, if we
determine that we want to start the PHY with an interface mode of
2500BASE-X, then this setup will restrict the advertisement and
supported masks to 2.5G speed link modes.

What we actually want for these PHYs is to allow them to support any
link modes that the PHY supports _and_ the MAC is also capable of
supporting. Without knowing the details of the PHY interface modes that
may be used, we can do this by using PHY_INTERFACE_MODE_NA to validate
and restrict the link modes to any that the MAC supports.

mvpp2 with the 88X3310 PHY avoids this problem, because the validate()
implementation allows all MAC supported speeds not only for
PHY_INTERFACE_MODE_NA, but also for XAUI and 10GKR modes.

The first patch addresses this; current MAC drivers should continue to
work as-is, but there will be a follow-on patch to fixup at least
mvpp2.

The second issue addresses a very similar problem that occurs when
trying to use ethtool to alter the advertisement mask - we call
the MAC validate() function with the current interface mode, the
current support and requested advertisement masks. This immediately
restricts the advertisement in the same way as the above.

This patch series addresses both issues, although the patches are not
in the above order.

Antoine - please can you check that there are no other reasons for
the mvpp2 code to be the way it was?  Thanks.

 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c |  20 ++++-
 drivers/net/phy/phylink.c                       | 106 +++++++++++++++---------
 2 files changed, 82 insertions(+), 44 deletions(-)

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

[PATCH net-next 1/3] net: phylink: improve clause 45 PHY ksettings_set implementation

[Russell King]

While testing ethtool with the Methode DM7052 module, it was noticed
that attempting to set the advertising mask results in the mask being
truncated to the support offered by the currently chosen PHY interface
mode.

When a PHY dynamically changes the PHY interface mode, limiting the
advertising mask in this way is not correct - if the PHY happened to
negotiate 10GBASE-T, and selected 10GBASE-R as the host interface, we
don't want to restrict the advertisement to just 10GBASE-* modes.

Rework setting the advertisement to take account of this; do not pass
the requested advertisement through phylink_validate(), but rely on
the advertisement restriction (supported mask) set when the PHY was
initially setup.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 84 ++++++++++++++++++++++++---------------
 1 file changed, 53 insertions(+), 31 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 8d20cf3ba0b7..2e5bc63c1dfa 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1229,44 +1229,66 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
 		__set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, config.advertising);
 	}
 
-	if (phylink_validate(pl, support, &config))
-		return -EINVAL;
-
-	/* If autonegotiation is enabled, we must have an advertisement */
-	if (config.an_enabled && phylink_is_empty_linkmode(config.advertising))
-		return -EINVAL;
-
-	our_kset = *kset;
-	linkmode_copy(our_kset.link_modes.advertising, config.advertising);
-	our_kset.base.speed = config.speed;
-	our_kset.base.duplex = config.duplex;
-
-	/* If we have a PHY, configure the phy */
 	if (pl->phydev) {
+		/* If we have a PHY, we process the kset change via phylib.
+		 * phylib will call our link state function if the PHY
+		 * parameters have changed, which will trigger a resolve
+		 * and update the MAC configuration.
+		 */
+		our_kset = *kset;
+		linkmode_copy(our_kset.link_modes.advertising,
+			      config.advertising);
+		our_kset.base.speed = config.speed;
+		our_kset.base.duplex = config.duplex;
+
 		ret = phy_ethtool_ksettings_set(pl->phydev, &our_kset);
 		if (ret)
 			return ret;
-	}
 
-	mutex_lock(&pl->state_mutex);
-	/* Configure the MAC to match the new settings */
-	linkmode_copy(pl->link_config.advertising, our_kset.link_modes.advertising);
-	pl->link_config.interface = config.interface;
-	pl->link_config.speed = our_kset.base.speed;
-	pl->link_config.duplex = our_kset.base.duplex;
-	pl->link_config.an_enabled = our_kset.base.autoneg != AUTONEG_DISABLE;
+		mutex_lock(&pl->state_mutex);
+		/* Save the new configuration */
+		linkmode_copy(pl->link_config.advertising,
+			      our_kset.link_modes.advertising);
+		pl->link_config.interface = config.interface;
+		pl->link_config.speed = our_kset.base.speed;
+		pl->link_config.duplex = our_kset.base.duplex;
+		pl->link_config.an_enabled = our_kset.base.autoneg !=
+					     AUTONEG_DISABLE;
+		mutex_unlock(&pl->state_mutex);
+	} else {
+		/* For a fixed link, this isn't able to change any parameters,
+		 * which just leaves inband mode.
+		 */
+		if (phylink_validate(pl, support, &config))
+			return -EINVAL;
 
-	/* If we have a PHY, phylib will call our link state function if the
-	 * mode has changed, which will trigger a resolve and update the MAC
-	 * configuration. For a fixed link, this isn't able to change any
-	 * parameters, which just leaves inband mode.
-	 */
-	if (pl->cur_link_an_mode == MLO_AN_INBAND &&
-	    !test_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state)) {
-		phylink_mac_config(pl, &pl->link_config);
-		phylink_mac_an_restart(pl);
+		/* If autonegotiation is enabled, we must have an advertisement */
+		if (config.an_enabled &&
+		    phylink_is_empty_linkmode(config.advertising))
+			return -EINVAL;
+
+		mutex_lock(&pl->state_mutex);
+		linkmode_copy(pl->link_config.advertising, config.advertising);
+		pl->link_config.interface = config.interface;
+		pl->link_config.speed = config.speed;
+		pl->link_config.duplex = config.duplex;
+		pl->link_config.an_enabled = kset->base.autoneg !=
+					     AUTONEG_DISABLE;
+
+		if (pl->cur_link_an_mode == MLO_AN_INBAND &&
+		    !test_bit(PHYLINK_DISABLE_STOPPED,
+			      &pl->phylink_disable_state)) {
+			/* If in 802.3z mode, this updates the advertisement.
+			 *
+			 * If we are in SGMII mode without a PHY, there is no
+			 * advertisement; the only thing we have is the pause
+			 * modes which can only come from a PHY.
+			 */
+			phylink_mac_config(pl, &pl->link_config);
+			phylink_mac_an_restart(pl);
+		}
+		mutex_unlock(&pl->state_mutex);
 	}
-	mutex_unlock(&pl->state_mutex);
 
 	return 0;
 }
-- 
2.20.1

[PATCH net-next 2/3] net: phylink: extend clause 45 PHY validation workaround

[Russell King]

Commit e45d1f5288b8 ("net: phylink: support Clause 45 PHYs on SFP+
modules") added a workaround to support clause 45 PHYs which
dynamically switch their interface mode on SFP+ modules.  This was
implemented by validating the PHYs supported/advertising using
PHY_INTERFACE_MODE_NA, rather than the specific interface mode that
we attached the PHY with.

However, we already have a situation where phylink is used to connect
a Marvell 88X3310 PHY which also behaves in exactly the same way, but
which seemingly doesn't need this.  The reason seems to be that the
mvpp2 driver sets a whole bunch of link modes for
PHY_INTERFACE_MODE_10GKR down to 10Mb/s, despite 10GBASE-R not actually
supporting anything but 10Gb/s speeds.

When testing with drivers that (correctly) take the mvneta approach,
where the validate() method only returns what can be supported /
advertised for the specified link mode, we find that Clause 45 PHYs do
not behave as we expect: their advertisement is restricted to what
the current link will support, rather than what the PHY supports
through its dynamic switching.

Extend this workaround to all such cases; if we have a Clause 45 PHY
attaching via any means, except in USXGMII, XAUI and RXAUI which are
all unable to support this dynamic switching or have other solutions
to it, then we need to validate using PHY_INTERFACE_MODE_NA.

This should allow mvpp2 to switch to a more conformant validate()
implementation.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 2e5bc63c1dfa..896772694bf4 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -735,7 +735,19 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 	memset(&config, 0, sizeof(config));
 	linkmode_copy(supported, phy->supported);
 	linkmode_copy(config.advertising, phy->advertising);
-	config.interface = interface;
+
+	/* Clause 45 PHYs switch their Serdes lane between several different
+	 * modes, normally 10GBASE-R, SGMII. Some use 2500BASE-X for 2.5G
+	 * speeds. We really need to know which interface modes the PHY and
+	 * MAC supports to properly work out which linkmodes can be supported.
+	 */
+	if (phy->is_c45 &&
+	    interface != PHY_INTERFACE_MODE_RXAUI &&
+	    interface != PHY_INTERFACE_MODE_XAUI &&
+	    interface != PHY_INTERFACE_MODE_USXGMII)
+		config.interface = PHY_INTERFACE_MODE_NA;
+	else
+		config.interface = interface;
 
 	ret = phylink_validate(pl, supported, &config);
 	if (ret)
@@ -1904,14 +1916,6 @@ static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
 	if (ret < 0)
 		return ret;
 
-	/* Clause 45 PHYs switch their Serdes lane between several different
-	 * modes, normally 10GBASE-R, SGMII. Some use 2500BASE-X for 2.5G
-	 * speeds.  We really need to know which interface modes the PHY and
-	 * MAC supports to properly work out which linkmodes can be supported.
-	 */
-	if (phy->is_c45)
-		interface = PHY_INTERFACE_MODE_NA;
-
 	ret = phylink_bringup_phy(pl, phy, interface);
 	if (ret)
 		phy_detach(phy);
-- 
2.20.1

[PATCH net-next 3/3] net: mvpp2: update mvpp2 validate() implementation

[Russell King]

Fix up the mvpp2 validate implementation to adopt the same behaviour as
mvneta:
- only allow the link modes that the specified PHY interface mode
  supports with the exception of 1000base-X and 2500base-X.
- use the basex helper to deal with SFP modules that can be switched
  between 1000base-X vs 2500base-X.

This gives consistent behaviour between mvneta and mvpp2.

This commit depends on "net: phylink: extend clause 45 PHY validation
workaround" so is not marked for backporting to stable kernels.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 .../net/ethernet/marvell/mvpp2/mvpp2_main.c   | 20 +++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 111b3b8239e1..fddd856338b4 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -4786,6 +4786,8 @@ static void mvpp2_phylink_validate(struct phylink_config *config,
 			phylink_set(mask, 10000baseER_Full);
 			phylink_set(mask, 10000baseKR_Full);
 		}
+		if (state->interface != PHY_INTERFACE_MODE_NA)
+			break;
 		/* Fall-through */
 	case PHY_INTERFACE_MODE_RGMII:
 	case PHY_INTERFACE_MODE_RGMII_ID:
@@ -4796,13 +4798,21 @@ static void mvpp2_phylink_validate(struct phylink_config *config,
 		phylink_set(mask, 10baseT_Full);
 		phylink_set(mask, 100baseT_Half);
 		phylink_set(mask, 100baseT_Full);
+		if (state->interface != PHY_INTERFACE_MODE_NA)
+			break;
 		/* Fall-through */
 	case PHY_INTERFACE_MODE_1000BASEX:
 	case PHY_INTERFACE_MODE_2500BASEX:
-		phylink_set(mask, 1000baseT_Full);
-		phylink_set(mask, 1000baseX_Full);
-		phylink_set(mask, 2500baseT_Full);
-		phylink_set(mask, 2500baseX_Full);
+		if (port->comphy ||
+		    state->interface != PHY_INTERFACE_MODE_2500BASEX) {
+			phylink_set(mask, 1000baseT_Full);
+			phylink_set(mask, 1000baseX_Full);
+		}
+		if (port->comphy ||
+		    state->interface == PHY_INTERFACE_MODE_2500BASEX) {
+			phylink_set(mask, 2500baseT_Full);
+			phylink_set(mask, 2500baseX_Full);
+		}
 		break;
 	default:
 		goto empty_set;
@@ -4811,6 +4821,8 @@ static void mvpp2_phylink_validate(struct phylink_config *config,
 	bitmap_and(supported, supported, mask, __ETHTOOL_LINK_MODE_MASK_NBITS);
 	bitmap_and(state->advertising, state->advertising, mask,
 		   __ETHTOOL_LINK_MODE_MASK_NBITS);
+
+	phylink_helper_basex_speed(state);
 	return;
 
 empty_set:
-- 
2.20.1

Re: [PATCH net-next 3/3] net: mvpp2: update mvpp2 validate() implementation

[Antoine Tenart]

Hello Russell,

On Thu, Dec 12, 2019 at 05:43:46PM +0000, Russell King wrote:
> 
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> index 111b3b8239e1..fddd856338b4 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> @@ -4786,6 +4786,8 @@ static void mvpp2_phylink_validate(struct phylink_config *config,
>  			phylink_set(mask, 10000baseER_Full);
>  			phylink_set(mask, 10000baseKR_Full);
>  		}
> +		if (state->interface != PHY_INTERFACE_MODE_NA)
> +			break;

>  		/* Fall-through */
>  	case PHY_INTERFACE_MODE_RGMII:
>  	case PHY_INTERFACE_MODE_RGMII_ID:
> @@ -4796,13 +4798,21 @@ static void mvpp2_phylink_validate(struct phylink_config *config,
>  		phylink_set(mask, 10baseT_Full);
>  		phylink_set(mask, 100baseT_Half);
>  		phylink_set(mask, 100baseT_Full);
> +		if (state->interface != PHY_INTERFACE_MODE_NA)
> +			break;

The two checks above will break the 10G/1G interfaces on the mcbin
(eth0/eth1) as they can support both 10gbase-kr and 10/100/1000baseT
modes depending on what's connected. With this patch only the modes
related to the one defined in the device tree would be valid, breaking
run-time reconfiguration of the link.

Thanks,
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next 3/3] net: mvpp2: update mvpp2 validate() implementation

[Russell King - ARM Linux admin]

On Fri, Dec 13, 2019 at 05:04:20PM +0100, Antoine Tenart wrote:
> Hello Russell,
> 
> On Thu, Dec 12, 2019 at 05:43:46PM +0000, Russell King wrote:
> > 
> > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > index 111b3b8239e1..fddd856338b4 100644
> > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > @@ -4786,6 +4786,8 @@ static void mvpp2_phylink_validate(struct phylink_config *config,
> >  			phylink_set(mask, 10000baseER_Full);
> >  			phylink_set(mask, 10000baseKR_Full);
> >  		}
> > +		if (state->interface != PHY_INTERFACE_MODE_NA)
> > +			break;
> 
> >  		/* Fall-through */
> >  	case PHY_INTERFACE_MODE_RGMII:
> >  	case PHY_INTERFACE_MODE_RGMII_ID:
> > @@ -4796,13 +4798,21 @@ static void mvpp2_phylink_validate(struct phylink_config *config,
> >  		phylink_set(mask, 10baseT_Full);
> >  		phylink_set(mask, 100baseT_Half);
> >  		phylink_set(mask, 100baseT_Full);
> > +		if (state->interface != PHY_INTERFACE_MODE_NA)
> > +			break;
> 
> The two checks above will break the 10G/1G interfaces on the mcbin
> (eth0/eth1) as they can support both 10gbase-kr and 10/100/1000baseT
> modes depending on what's connected. With this patch only the modes
> related to the one defined in the device tree would be valid, breaking
> run-time reconfiguration of the link.

Exactly which scenario are you talking about?  The mcbin doubleshot
setup, or the singleshot setup?

This patch (when combined with the others) has no effect on the
doubleshot, and should have no effect on the SFP cages on the single
shot.

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

Re: [PATCH net-next 3/3] net: mvpp2: update mvpp2 validate() implementation

[Antoine Tenart]

On Fri, Dec 13, 2019 at 04:11:44PM +0000, Russell King - ARM Linux admin wrote:
> On Fri, Dec 13, 2019 at 05:04:20PM +0100, Antoine Tenart wrote:
> > On Thu, Dec 12, 2019 at 05:43:46PM +0000, Russell King wrote:
> > > 
> > > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > > index 111b3b8239e1..fddd856338b4 100644
> > > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > > @@ -4786,6 +4786,8 @@ static void mvpp2_phylink_validate(struct phylink_config *config,
> > >  			phylink_set(mask, 10000baseER_Full);
> > >  			phylink_set(mask, 10000baseKR_Full);
> > >  		}
> > > +		if (state->interface != PHY_INTERFACE_MODE_NA)
> > > +			break;
> > 
> > >  		/* Fall-through */
> > >  	case PHY_INTERFACE_MODE_RGMII:
> > >  	case PHY_INTERFACE_MODE_RGMII_ID:
> > > @@ -4796,13 +4798,21 @@ static void mvpp2_phylink_validate(struct phylink_config *config,
> > >  		phylink_set(mask, 10baseT_Full);
> > >  		phylink_set(mask, 100baseT_Half);
> > >  		phylink_set(mask, 100baseT_Full);
> > > +		if (state->interface != PHY_INTERFACE_MODE_NA)
> > > +			break;
> > 
> > The two checks above will break the 10G/1G interfaces on the mcbin
> > (eth0/eth1) as they can support both 10gbase-kr and 10/100/1000baseT
> > modes depending on what's connected. With this patch only the modes
> > related to the one defined in the device tree would be valid, breaking
> > run-time reconfiguration of the link.
> 
> Exactly which scenario are you talking about?  The mcbin doubleshot
> setup, or the singleshot setup?

I was thinking about the doubleshot.

> This patch (when combined with the others) has no effect on the
> doubleshot, and should have no effect on the SFP cages on the single
> shot.

You're right, I just tested the series and it the two 10G/1G ports were
able to be reconfigured at runtime, my bad.

However it seems cp1_eth1 is coming up at 100Mbps only. (I haven't
looked into it more than a quick test so far).

  # ethtool eth2
  Settings for eth2:
          Supported ports: [ TP MII FIBRE ]
          Supported link modes:   10baseT/Half 10baseT/Full
                                  100baseT/Half 100baseT/Full
          Supported pause frame use: Symmetric Receive-only
          Supports auto-negotiation: Yes
          Supported FEC modes: Not reported
          Advertised link modes:  10baseT/Half 10baseT/Full
                                  100baseT/Half 100baseT/Full
          Advertised pause frame use: Symmetric Receive-only
          Advertised auto-negotiation: Yes
          Advertised FEC modes: Not reported
          Link partner advertised link modes:  10baseT/Half 10baseT/Full
                                               100baseT/Half 100baseT/Full
          Link partner advertised pause frame use: Symmetric Receive-only
          Link partner advertised auto-negotiation: Yes
          Link partner advertised FEC modes: Not reported
          Speed: 100Mb/s
          Duplex: Full
          Port: MII
          PHYAD: 0
          Transceiver: internal
          Auto-negotiation: on
          Link detected: yes

The link partner however advertises:

          Advertised link modes:  10baseT/Half 10baseT/Full
                                  100baseT/Half 100baseT/Full
                                  1000baseT/Full

Thanks,
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next 3/3] net: mvpp2: update mvpp2 validate() implementation

[Russell King - ARM Linux admin]

On Fri, Dec 13, 2019 at 05:28:08PM +0100, Antoine Tenart wrote:
> On Fri, Dec 13, 2019 at 04:11:44PM +0000, Russell King - ARM Linux admin wrote:
> > On Fri, Dec 13, 2019 at 05:04:20PM +0100, Antoine Tenart wrote:
> > > On Thu, Dec 12, 2019 at 05:43:46PM +0000, Russell King wrote:
> > > > 
> > > > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > > > index 111b3b8239e1..fddd856338b4 100644
> > > > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > > > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > > > @@ -4786,6 +4786,8 @@ static void mvpp2_phylink_validate(struct phylink_config *config,
> > > >  			phylink_set(mask, 10000baseER_Full);
> > > >  			phylink_set(mask, 10000baseKR_Full);
> > > >  		}
> > > > +		if (state->interface != PHY_INTERFACE_MODE_NA)
> > > > +			break;
> > > 
> > > >  		/* Fall-through */
> > > >  	case PHY_INTERFACE_MODE_RGMII:
> > > >  	case PHY_INTERFACE_MODE_RGMII_ID:
> > > > @@ -4796,13 +4798,21 @@ static void mvpp2_phylink_validate(struct phylink_config *config,
> > > >  		phylink_set(mask, 10baseT_Full);
> > > >  		phylink_set(mask, 100baseT_Half);
> > > >  		phylink_set(mask, 100baseT_Full);
> > > > +		if (state->interface != PHY_INTERFACE_MODE_NA)
> > > > +			break;
> > > 
> > > The two checks above will break the 10G/1G interfaces on the mcbin
> > > (eth0/eth1) as they can support both 10gbase-kr and 10/100/1000baseT
> > > modes depending on what's connected. With this patch only the modes
> > > related to the one defined in the device tree would be valid, breaking
> > > run-time reconfiguration of the link.
> > 
> > Exactly which scenario are you talking about?  The mcbin doubleshot
> > setup, or the singleshot setup?
> 
> I was thinking about the doubleshot.
> 
> > This patch (when combined with the others) has no effect on the
> > doubleshot, and should have no effect on the SFP cages on the single
> > shot.
> 
> You're right, I just tested the series and it the two 10G/1G ports were
> able to be reconfigured at runtime, my bad.
> 
> However it seems cp1_eth1 is coming up at 100Mbps only. (I haven't
> looked into it more than a quick test so far).

Oh, looks like I made a mistake in the fallthrough for *GMII modes.
It's probably easier just to add the two 1G modes there, which
should restore eth2 to full functionality.  Thanks for spotting.

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index fddd856338b4..f09fcbe6ea88 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -4798,6 +4798,8 @@ static void mvpp2_phylink_validate(struct phylink_config *config,
 		phylink_set(mask, 10baseT_Full);
 		phylink_set(mask, 100baseT_Half);
 		phylink_set(mask, 100baseT_Full);
+		phylink_set(mask, 1000baseT_Full);
+		phylink_set(mask, 1000baseX_Full);
 		if (state->interface != PHY_INTERFACE_MODE_NA)
 			break;
 		/* Fall-through */

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

Re: [PATCH net-next 3/3] net: mvpp2: update mvpp2 validate() implementation

[Antoine Tenart]

On Fri, Dec 13, 2019 at 04:40:28PM +0000, Russell King - ARM Linux admin wrote:
> On Fri, Dec 13, 2019 at 05:28:08PM +0100, Antoine Tenart wrote:
> > On Fri, Dec 13, 2019 at 04:11:44PM +0000, Russell King - ARM Linux admin wrote:
> > > On Fri, Dec 13, 2019 at 05:04:20PM +0100, Antoine Tenart wrote:
> > > > On Thu, Dec 12, 2019 at 05:43:46PM +0000, Russell King wrote:
> > > > > 
> > > > > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > > > > index 111b3b8239e1..fddd856338b4 100644
> > > > > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > > > > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > > > > @@ -4786,6 +4786,8 @@ static void mvpp2_phylink_validate(struct phylink_config *config,
> > > > >  			phylink_set(mask, 10000baseER_Full);
> > > > >  			phylink_set(mask, 10000baseKR_Full);
> > > > >  		}
> > > > > +		if (state->interface != PHY_INTERFACE_MODE_NA)
> > > > > +			break;
> > > > 
> > > > >  		/* Fall-through */
> > > > >  	case PHY_INTERFACE_MODE_RGMII:
> > > > >  	case PHY_INTERFACE_MODE_RGMII_ID:
> > > > > @@ -4796,13 +4798,21 @@ static void mvpp2_phylink_validate(struct phylink_config *config,
> > > > >  		phylink_set(mask, 10baseT_Full);
> > > > >  		phylink_set(mask, 100baseT_Half);
> > > > >  		phylink_set(mask, 100baseT_Full);
> > > > > +		if (state->interface != PHY_INTERFACE_MODE_NA)
> > > > > +			break;
> > > > 
> > > > The two checks above will break the 10G/1G interfaces on the mcbin
> > > > (eth0/eth1) as they can support both 10gbase-kr and 10/100/1000baseT
> > > > modes depending on what's connected. With this patch only the modes
> > > > related to the one defined in the device tree would be valid, breaking
> > > > run-time reconfiguration of the link.
> > > 
> > > Exactly which scenario are you talking about?  The mcbin doubleshot
> > > setup, or the singleshot setup?
> > 
> > I was thinking about the doubleshot.
> > 
> > > This patch (when combined with the others) has no effect on the
> > > doubleshot, and should have no effect on the SFP cages on the single
> > > shot.
> > 
> > You're right, I just tested the series and it the two 10G/1G ports were
> > able to be reconfigured at runtime, my bad.
> > 
> > However it seems cp1_eth1 is coming up at 100Mbps only. (I haven't
> > looked into it more than a quick test so far).
> 
> Oh, looks like I made a mistake in the fallthrough for *GMII modes.
> It's probably easier just to add the two 1G modes there, which
> should restore eth2 to full functionality.  Thanks for spotting.
> 
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> index fddd856338b4..f09fcbe6ea88 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> @@ -4798,6 +4798,8 @@ static void mvpp2_phylink_validate(struct phylink_config *config,
>  		phylink_set(mask, 10baseT_Full);
>  		phylink_set(mask, 100baseT_Half);
>  		phylink_set(mask, 100baseT_Full);
> +		phylink_set(mask, 1000baseT_Full);
> +		phylink_set(mask, 1000baseX_Full);
>  		if (state->interface != PHY_INTERFACE_MODE_NA)
>  			break;
>  		/* Fall-through */

Agreed, adding the two 1G modes here is easier and more readable.

Thanks,
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com