Re: [PATCH net-next v14 5/6] net: dsa: mv88e6xxx: Add support for mv88e6393x family of Marvell

On Mon, Jan 11, 2021 at 02:21:55AM +0100, Marek Behún wrote:

From: Pavana Sharma <redacted>

The Marvell 88E6393X device is a single-chip integration of a 11-port
Ethernet switch with eight integrated Gigabit Ethernet (GbE)
transceivers and three 10-Gigabit interfaces.

This patch adds functionalities specific to mv88e6393x family (88E6393X,
88E6193X and 88E6191X).

The main differences between previous devices and this one are:
- port 0 can be a SERDES port
- all SERDESes are one-lane, eg. no XAUI nor RXAUI
- on the other hand the SERDESes can do USXGMII, 10GBASER and 5GBASER
  (on 6191X only one SERDES is capable of more than 1g; USXGMII is not
  yet supported with this change)
- Port Policy CTL register is changed to Port Policy MGMT CTL register,
  via which serveral more registers can be accessed indirectly

              ~~~~~~~~
              several

- egress monitor port is configured differently
- ingress monitor/CPU/mirror ports are configured differently and can be
  configured per port (ie. each port can have different ingress monitor
  port, for example)
- port speed AltBit works differently than previously
- PHY registers can be also accessed via MDIO address 0x18 and 0x19
  (on previous devices they could be accessed only via Global 2 offsets
   0x18 and 0x19, which means two indirections; this feature is not yet
   leveraged with this patch)

Co-developed-by: Ashkan Boldaji <redacted>
Signed-off-by: Ashkan Boldaji <redacted>
Signed-off-by: Pavana Sharma <redacted>
Co-developed-by: Marek Behún <kabel@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
+static void mv88e6393x_phylink_validate(struct mv88e6xxx_chip *chip, int port,
+					unsigned long *mask,
+					struct phylink_link_state *state)
+{
+	if (port == 0 || port == 9 || port == 10) {
+		phylink_set(mask, 10000baseT_Full);
+		phylink_set(mask, 10000baseKR_Full);

I think I understand the reason for declaring 10GBase-KR support in
phylink_validate, in case the PHY supports that link mode on the media
side, but...

Does the SERDES actually support 10GBase-KR (aka 10GBase-R for copper
backplanes)? It is different than plain 10GBase-R (abusingly called XFI)
by the need of a link training procedure to negotiate SERDES eye
parameters. There have been discussion in the past where it turned out
that drivers which didn't really support 10GBase-KR incorrectly reported
that they did.

I think the assignment fits on the same line as the declaration?

+
+	return mv88e6xxx_port_write(chip, port, MV88E6393X_PORT_POLICY_MGMT_CTL,
+				    reg);
+}
+
+int mv88e6393x_serdes_setup_errata(struct mv88e6xxx_chip *chip)
+{
+	int err;
+
+	err = mv88e6393x_serdes_port_errata(chip, MV88E6393X_PORT0_LANE);
+	if (err)
+		return err;
+
+	err = mv88e6393x_serdes_port_errata(chip, MV88E6393X_PORT9_LANE);
+	if (err)
+		return err;
+
+	return mv88e6393x_serdes_port_errata(chip, MV88E6393X_PORT10_LANE);
+}
+
+static int mv88e6393x_serdes_port_config(struct mv88e6xxx_chip *chip, int lane,
+					 bool on)
+{
+	u8 cmode = chip->ports[lane].cmode;
+	u16 reg, pcs;
+	int err;
+
+	if (on) {

And if "on" is false? Nothing? Why even pass it as an argument then? Why
even call mv88e6393x_serdes_port_config?

+		switch (cmode) {
+		case MV88E6XXX_PORT_STS_CMODE_SGMII:
+			pcs = MV88E6393X_PCS_SELECT_SGMII_MAC;
+			break;
+		case MV88E6XXX_PORT_STS_CMODE_1000BASEX:
+			pcs = MV88E6393X_PCS_SELECT_1000BASEX;
+			break;
+		case MV88E6XXX_PORT_STS_CMODE_2500BASEX:
+			pcs = MV88E6393X_PCS_SELECT_2500BASEX;
+			break;
+		case MV88E6393X_PORT_STS_CMODE_5GBASER:
+			pcs = MV88E6393X_PCS_SELECT_5GBASER;
+			break;
+		case MV88E6393X_PORT_STS_CMODE_10GBASER:
+			pcs = MV88E6393X_PCS_SELECT_10GBASER;
+			break;
+		default:
+			pcs = MV88E6393X_PCS_SELECT_1000BASEX;
+			break;
+		}
+
+		/* mv88e6393x family errata 3.6 :
+		 * When changing c_mode on Port 0 from [x]MII mode to any
+		 * SERDES mode SERDES will not be operational.
+		 * Workaround: Set Port0 SERDES register 4.F002.5=0
+		 */
+		err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
+					    MV88E6393X_SERDES_POC, &reg);
+		if (err)
+			return err;
+
+		reg &= ~(MV88E6393X_SERDES_POC_PCS_MODE_MASK |
+			 MV88E6393X_SERDES_POC_PDOWN);
+		reg |= pcs;
+
+		err = mv88e6390_serdes_write(chip, lane, MDIO_MMD_PHYXS,
+					     MV88E6393X_SERDES_POC, reg);
+		if (err)
+			return err;
+
+		reg |= MV88E6393X_SERDES_POC_RESET;
+		err = mv88e6390_serdes_write(chip, lane, MDIO_MMD_PHYXS,
+					     MV88E6393X_SERDES_POC, reg);
+		if (err)
+			return err;
+
+		/* mv88e6393x family errata 3.7 :
+		 * When changing cmode on SERDES port from any other mode to
+		 * 1000BASE-X mode the link may not come up due to invalid
+		 * 1000BASE-X advertisement.
+		 * Workaround: Correct advertisement and reset PHY core.
+		 */
+		if (cmode == MV88E6XXX_PORT_STS_CMODE_1000BASEX) {
+			reg = MV88E6390_SGMII_ANAR_1000BASEX_FD;
+			err = mv88e6390_serdes_write(chip, lane, MDIO_MMD_PHYXS,
+						     MV88E6390_SGMII_ANAR, reg);
+			if (err)
+				return err;
+
+			/* soft reset the PCS/PMA */
+			err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
+						    MV88E6390_SGMII_CONTROL,
+						    &reg);
+			if (err)
+				return err;
+
+			reg |= MV88E6390_SGMII_CONTROL_RESET;
+			err = mv88e6390_serdes_write(chip, lane, MDIO_MMD_PHYXS,
+						     MV88E6390_SGMII_CONTROL,
+						     reg);
+			if (err)
+				return err;
+		}
+	}
+
+	return 0;
+}
+
+int mv88e6393x_serdes_power(struct mv88e6xxx_chip *chip, int port, int lane,
+			    bool on)
+{
+	u8 cmode = chip->ports[port].cmode;
+
+	if (port != 0 && port != 9 && port != 10)
+		return -EOPNOTSUPP;
+
+	mv88e6393x_serdes_port_config(chip, lane, on);
+
+	switch (cmode) {
+	case MV88E6XXX_PORT_STS_CMODE_SGMII:
+	case MV88E6XXX_PORT_STS_CMODE_1000BASEX:
+	case MV88E6XXX_PORT_STS_CMODE_2500BASEX:
+		return mv88e6390_serdes_power_sgmii(chip, lane, on);
+	case MV88E6393X_PORT_STS_CMODE_5GBASER:
+	case MV88E6393X_PORT_STS_CMODE_10GBASER:
+		return mv88e6390_serdes_power_10g(chip, lane, on);
+	}
+
+	return 0;
+}