Re: [PATCH] net: dsa: mv88e6xxx: prevent 2500BASEX mode override

On Mon, 15 Feb 2021 14:57:57 +0000
Russell King - ARM Linux admin [off-list ref] wrote:

On Mon, Feb 15, 2021 at 02:47:56PM +0100, Marek Behun wrote:

quoted

On Mon, 15 Feb 2021 06:15:59 +0000
Nathan Rossi [off-list ref] wrote:
  

quoted

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 54aa942eed..5c52906b29 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c

@@ -650,6 +650,13 @@ static void mv88e6xxx_validate(struct dsa_switch *ds, int port,
 	if (chip->info->ops->phylink_validate)
 		chip->info->ops->phylink_validate(chip, port, mask, state);
 
+	/* Advertise 2500BASEX only if 1000BASEX is not configured, this
+	 * prevents phylink_helper_basex_speed from always overriding the
+	 * 1000BASEX mode since auto negotiation is always enabled.
+	 */
+	if (state->interface == PHY_INTERFACE_MODE_1000BASEX)
+		phylink_clear(mask, 2500baseX_Full);
+  

I don't quite like this. This problem should be either solved in
phylink_helper_basex_speed() or somewhere in the mv88e6xxx code, but near
the call to phylink_helper_basex_speed().

Putting a solution to the behaviour of phylink_helper_basex_speed() it
into the validate() method when phylink_helper_basex_speed() is called
from a different place will complicate debugging in the future. If
we start solving problems in this kind of way, the driver will become
totally unreadable, IMO.  

If we can't switch between 1000base-X and 2500base-X, then we should
not be calling phylink_helper_basex_speed() - and only one of those
two capabilities should be set in the validation callback. I thought
there were DSA switches where we could program the CMODE to switch
between these two...

There are. At least Peridot, Topaz and Amethyst support switching
between these modes. But only on some ports.

This problem happnes on Peridot X, I think.

On Peridot X there are
- port 0: RGMII
- ports 9-10: capable of 1, 2.5 and 10G SerDes (10G via
  XAUI/RXAUI, so multiple lanes)
- ports 1-8: with copper PHYs
  - some of these can instead be set to use the unused SerDes lanes
    of ports 9-10, but only in 1000base-x mode

So the problem can happen if you set port 9 or 10 to only use one
SerDes lane, and use the spare lanes for the 1G ports.
On these ports 2500base-x is not supported, only 1000base-x (maybe
sgmii, I don't remember)

Marek