[PATCH net 0/6] mv88e6xxx fixes (mainly 88E6393X family)

[PATCH net 0/6] mv88e6xxx fixes (mainly 88E6393X family)

[Marek Behún]

Hello,

so I managed to discovered how to fix inband AN for 2500base-x mode on
88E6393x (Amethyst) family.

This series fixes application of erratum 4.8, adds fix for erratum 5.2,
adds support for completely disablign SerDes receiver / transmitter,
fixes inband AN for 2500base-x mode by using 1000base-x mode and simply
changing frequeny to 3.125 GHz, all this for 88E6393X.

The last commit fixes linking when link partner has AN disabled and the
device invokes the AN bypass feature. Currently we fail to link in this
case.

Marek

Marek Behún (6):
  net: dsa: mv88e6xxx: Fix application of erratum 4.8 for 88E6393X
  net: dsa: mv88e6xxx: Drop unnecessary check in
    mv88e6393x_serdes_erratum_4_6()
  net: dsa: mv88e6xxx: Save power by disabling SerDes trasmitter and
    receiver
  net: dsa: mv88e6xxx: Add fix for erratum 5.2 of 88E6393X family
  net: dsa: mv88e6xxx: Fix inband AN for 2500base-x on 88E6393X family
  net: dsa: mv88e6xxx: Link in pcs_get_state() if AN is bypassed

 drivers/net/dsa/mv88e6xxx/serdes.c | 240 +++++++++++++++++++++++++----
 drivers/net/dsa/mv88e6xxx/serdes.h |   4 +
 2 files changed, 213 insertions(+), 31 deletions(-)

-- 
2.32.0

[PATCH net 1/6] net: dsa: mv88e6xxx: Fix application of erratum 4.8 for 88E6393X

[Marek Behún]

According to SERDES scripts for 88E6393X, erratum 4.8 has to be applied
every time before SerDes is powered on.

Split the code for erratum 4.8 into separate function and call it in
mv88e6393x_serdes_power().

Fixes: de776d0d316f ("net: dsa: mv88e6xxx: add support for mv88e6393x family")
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/net/dsa/mv88e6xxx/serdes.c | 53 +++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 20 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index 6ea003678798..0658ee3b014c 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -1271,9 +1271,9 @@ void mv88e6390_serdes_get_regs(struct mv88e6xxx_chip *chip, int port, void *_p)
 	}
 }
 
-static int mv88e6393x_serdes_port_errata(struct mv88e6xxx_chip *chip, int lane)
+static int mv88e6393x_serdes_erratum_4_6(struct mv88e6xxx_chip *chip, int lane)
 {
-	u16 reg, pcs;
+	u16 reg;
 	int err;
 
 	/* mv88e6393x family errata 4.6:
@@ -1300,11 +1300,32 @@ static int mv88e6393x_serdes_port_errata(struct mv88e6xxx_chip *chip, int lane)
 		if (err)
 			return err;
 
-		err = mv88e6390_serdes_power_sgmii(chip, lane, false);
-		if (err)
-			return err;
+		return mv88e6390_serdes_power_sgmii(chip, lane, false);
 	}
 
+	return 0;
+}
+
+int mv88e6393x_serdes_setup_errata(struct mv88e6xxx_chip *chip)
+{
+	int err;
+
+	err = mv88e6393x_serdes_erratum_4_6(chip, MV88E6393X_PORT0_LANE);
+	if (err)
+		return err;
+
+	err = mv88e6393x_serdes_erratum_4_6(chip, MV88E6393X_PORT9_LANE);
+	if (err)
+		return err;
+
+	return mv88e6393x_serdes_erratum_4_6(chip, MV88E6393X_PORT10_LANE);
+}
+
+static int mv88e6393x_serdes_erratum_4_8(struct mv88e6xxx_chip *chip, int lane)
+{
+	u16 reg, pcs;
+	int err;
+
 	/* mv88e6393x family errata 4.8:
 	 * When a SERDES port is operating in 1000BASE-X or SGMII mode link may
 	 * not come up after hardware reset or software reset of SERDES core.
@@ -1334,29 +1355,21 @@ static int mv88e6393x_serdes_port_errata(struct mv88e6xxx_chip *chip, int lane)
 				      MV88E6393X_ERRATA_4_8_REG, 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);
-}
-
 int mv88e6393x_serdes_power(struct mv88e6xxx_chip *chip, int port, int lane,
 			    bool on)
 {
 	u8 cmode = chip->ports[port].cmode;
+	int err;
 
 	if (port != 0 && port != 9 && port != 10)
 		return -EOPNOTSUPP;
 
+	if (on) {
+		err = mv88e6393x_serdes_erratum_4_8(chip, lane);
+		if (err)
+			return err;
+	}
+
 	switch (cmode) {
 	case MV88E6XXX_PORT_STS_CMODE_SGMII:
 	case MV88E6XXX_PORT_STS_CMODE_1000BASEX:
-- 
2.32.0

[PATCH net 2/6] net: dsa: mv88e6xxx: Drop unnecessary check in mv88e6393x_serdes_erratum_4_6()

[Marek Behún]

The check for lane is unnecessary, since the function is called only
with allowed lane argument.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/net/dsa/mv88e6xxx/serdes.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index 0658ee3b014c..3a6244596a67 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -1284,26 +1284,20 @@ static int mv88e6393x_serdes_erratum_4_6(struct mv88e6xxx_chip *chip, int lane)
 	 * It seems that after this workaround the SERDES is automatically
 	 * powered up (the bit is cleared), so power it down.
 	 */
-	if (lane == MV88E6393X_PORT0_LANE || lane == MV88E6393X_PORT9_LANE ||
-	    lane == MV88E6393X_PORT10_LANE) {
-		err = mv88e6390_serdes_read(chip, lane,
-					    MDIO_MMD_PHYXS,
-					    MV88E6393X_SERDES_POC, &reg);
-		if (err)
-			return err;
+	err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
+				    MV88E6393X_SERDES_POC, &reg);
+	if (err)
+		return err;
 
-		reg &= ~MV88E6393X_SERDES_POC_PDOWN;
-		reg |= MV88E6393X_SERDES_POC_RESET;
+	reg &= ~MV88E6393X_SERDES_POC_PDOWN;
+	reg |= MV88E6393X_SERDES_POC_RESET;
 
-		err = mv88e6390_serdes_write(chip, lane, MDIO_MMD_PHYXS,
-					     MV88E6393X_SERDES_POC, reg);
-		if (err)
-			return err;
-
-		return mv88e6390_serdes_power_sgmii(chip, lane, false);
-	}
+	err = mv88e6390_serdes_write(chip, lane, MDIO_MMD_PHYXS,
+				     MV88E6393X_SERDES_POC, reg);
+	if (err)
+		return err;
 
-	return 0;
+	return mv88e6390_serdes_power_sgmii(chip, lane, false);
 }
 
 int mv88e6393x_serdes_setup_errata(struct mv88e6xxx_chip *chip)
-- 
2.32.0

[PATCH net 3/6] net: dsa: mv88e6xxx: Save power by disabling SerDes trasmitter and receiver

[Marek Behún]

Save power on 88E6393X by disabling SerDes receiver and transmitter
after SerDes is SerDes is disabled.

Signed-off-by: Marek Behún <kabel@kernel.org>
Cc: stable@vger.kernel.org # de776d0d316f ("net: dsa: mv88e6xxx: add support for mv88e6393x family")
---
 drivers/net/dsa/mv88e6xxx/serdes.c | 44 ++++++++++++++++++++++++++++--
 drivers/net/dsa/mv88e6xxx/serdes.h |  3 ++
 2 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index 3a6244596a67..0d92bd814f3a 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -1271,6 +1271,28 @@ void mv88e6390_serdes_get_regs(struct mv88e6xxx_chip *chip, int port, void *_p)
 	}
 }
 
+static int mv88e6393x_serdes_power_lane(struct mv88e6xxx_chip *chip, int lane,
+					bool on)
+{
+	u16 reg;
+	int err;
+
+	err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
+				    MV88E6393X_SERDES_CTRL1, &reg);
+	if (err)
+		return err;
+
+	if (on)
+		reg &= !(MV88E6393X_SERDES_CTRL1_TX_PDOWN |
+			 MV88E6393X_SERDES_CTRL1_RX_PDOWN);
+	else
+		reg |= MV88E6393X_SERDES_CTRL1_TX_PDOWN |
+		       MV88E6393X_SERDES_CTRL1_RX_PDOWN;
+
+	return mv88e6390_serdes_write(chip, lane, MDIO_MMD_PHYXS,
+				      MV88E6393X_SERDES_CTRL1, reg);
+}
+
 static int mv88e6393x_serdes_erratum_4_6(struct mv88e6xxx_chip *chip, int lane)
 {
 	u16 reg;
@@ -1297,7 +1319,11 @@ static int mv88e6393x_serdes_erratum_4_6(struct mv88e6xxx_chip *chip, int lane)
 	if (err)
 		return err;
 
-	return mv88e6390_serdes_power_sgmii(chip, lane, false);
+	err = mv88e6390_serdes_power_sgmii(chip, lane, false);
+	if (err)
+		return err;
+
+	return mv88e6393x_serdes_power_lane(chip, lane, false);
 }
 
 int mv88e6393x_serdes_setup_errata(struct mv88e6xxx_chip *chip)
@@ -1362,17 +1388,29 @@ int mv88e6393x_serdes_power(struct mv88e6xxx_chip *chip, int port, int lane,
 		err = mv88e6393x_serdes_erratum_4_8(chip, lane);
 		if (err)
 			return err;
+
+		err = mv88e6393x_serdes_power_lane(chip, lane, true);
+		if (err)
+			return err;
 	}
 
 	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);
+		err = mv88e6390_serdes_power_sgmii(chip, lane, on);
+		break;
 	case MV88E6393X_PORT_STS_CMODE_5GBASER:
 	case MV88E6393X_PORT_STS_CMODE_10GBASER:
-		return mv88e6390_serdes_power_10g(chip, lane, on);
+		err = mv88e6390_serdes_power_10g(chip, lane, on);
+		break;
 	}
 
+	if (err)
+		return err;
+
+	if (!on)
+		return mv88e6393x_serdes_power_lane(chip, lane, false);
+
 	return 0;
 }
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.h b/drivers/net/dsa/mv88e6xxx/serdes.h
index cbb3ba30caea..e9292c8beee4 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.h
+++ b/drivers/net/dsa/mv88e6xxx/serdes.h
@@ -93,6 +93,9 @@
 #define MV88E6393X_SERDES_POC_PCS_MASK		0x0007
 #define MV88E6393X_SERDES_POC_RESET		BIT(15)
 #define MV88E6393X_SERDES_POC_PDOWN		BIT(5)
+#define MV88E6393X_SERDES_CTRL1			0xf003
+#define MV88E6393X_SERDES_CTRL1_TX_PDOWN	BIT(9)
+#define MV88E6393X_SERDES_CTRL1_RX_PDOWN	BIT(8)
 
 #define MV88E6393X_ERRATA_4_8_REG		0xF074
 #define MV88E6393X_ERRATA_4_8_BIT		BIT(14)
-- 
2.32.0

Re: [PATCH net 3/6] net: dsa: mv88e6xxx: Save power by disabling SerDes trasmitter and receiver

[Russell King (Oracle)]

On Mon, Nov 29, 2021 at 08:58:20PM +0100, Marek Behún wrote:
> +static int mv88e6393x_serdes_power_lane(struct mv88e6xxx_chip *chip, int lane,
> +					bool on)
> +{
> +	u16 reg;
> +	int err;
> +
> +	err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
> +				    MV88E6393X_SERDES_CTRL1, &reg);
> +	if (err)
> +		return err;
> +
> +	if (on)
> +		reg &= !(MV88E6393X_SERDES_CTRL1_TX_PDOWN |
> +			 MV88E6393X_SERDES_CTRL1_RX_PDOWN);

Are you sure this is correct? Don't you want that to be ~(...) ?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net 3/6] net: dsa: mv88e6xxx: Save power by disabling SerDes trasmitter and receiver

[Marek Behún]

On Mon, 29 Nov 2021 23:06:38 +0000
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Mon, Nov 29, 2021 at 08:58:20PM +0100, Marek Behún wrote:
> > +static int mv88e6393x_serdes_power_lane(struct mv88e6xxx_chip *chip, int lane,
> > +					bool on)
> > +{
> > +	u16 reg;
> > +	int err;
> > +
> > +	err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
> > +				    MV88E6393X_SERDES_CTRL1, &reg);
> > +	if (err)
> > +		return err;
> > +
> > +	if (on)
> > +		reg &= !(MV88E6393X_SERDES_CTRL1_TX_PDOWN |
> > +			 MV88E6393X_SERDES_CTRL1_RX_PDOWN);  
> 
> Are you sure this is correct? Don't you want that to be ~(...) ?
> 

/o\ How did I not notice this? Thanks.

[PATCH net 4/6] net: dsa: mv88e6xxx: Add fix for erratum 5.2 of 88E6393X family

[Marek Behún]

Add fix for erratum 5.2 of the 88E6393X (Amethyst) family: for 10gbase-r
mode, some undocumented registers need to be written some special
values.

Fixes: de776d0d316f ("net: dsa: mv88e6xxx: add support for mv88e6393x family")
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/net/dsa/mv88e6xxx/serdes.c | 48 ++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index 0d92bd814f3a..9901219780e6 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -1375,6 +1375,50 @@ static int mv88e6393x_serdes_erratum_4_8(struct mv88e6xxx_chip *chip, int lane)
 				      MV88E6393X_ERRATA_4_8_REG, reg);
 }
 
+static int mv88e6393x_serdes_erratum_5_2(struct mv88e6xxx_chip *chip, int lane,
+					 u8 cmode)
+{
+	static const struct {
+		u16 dev, reg, val, mask;
+	} fixes[] = {
+		{ MDIO_MMD_VEND1, 0x8093, 0xcb5a, 0xffff },
+		{ MDIO_MMD_VEND1, 0x8171, 0x7088, 0xffff },
+		{ MDIO_MMD_VEND1, 0x80c9, 0x311a, 0xffff },
+		{ MDIO_MMD_VEND1, 0x80a2, 0x8000, 0xff7f },
+		{ MDIO_MMD_VEND1, 0x80a9, 0x0000, 0xfff0 },
+		{ MDIO_MMD_VEND1, 0x80a3, 0x0000, 0xf8ff },
+		{ MDIO_MMD_PHYXS, MV88E6393X_SERDES_POC,
+		  MV88E6393X_SERDES_POC_RESET, MV88E6393X_SERDES_POC_RESET },
+	};
+	int err, i;
+	u16 reg;
+
+	/* mv88e6393x family errata 5.2:
+	 * For optimal signal integrity the following sequence should be applied
+	 * to SERDES operating in 10G mode. These registers only apply to 10G
+	 * operation and have no effect on other speeds.
+	 */
+	if (cmode != MV88E6393X_PORT_STS_CMODE_10GBASER)
+		return 0;
+
+	for (i = 0; i < ARRAY_SIZE(fixes); ++i) {
+		err = mv88e6390_serdes_read(chip, lane, fixes[i].dev,
+					    fixes[i].reg, &reg);
+		if (err)
+			return err;
+
+		reg &= ~fixes[i].mask;
+		reg |= fixes[i].val;
+
+		err = mv88e6390_serdes_write(chip, lane, fixes[i].dev,
+					     fixes[i].reg, reg);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
 int mv88e6393x_serdes_power(struct mv88e6xxx_chip *chip, int port, int lane,
 			    bool on)
 {
@@ -1389,6 +1433,10 @@ int mv88e6393x_serdes_power(struct mv88e6xxx_chip *chip, int port, int lane,
 		if (err)
 			return err;
 
+		err = mv88e6393x_serdes_erratum_5_2(chip, lane, cmode);
+		if (err)
+			return err;
+
 		err = mv88e6393x_serdes_power_lane(chip, lane, true);
 		if (err)
 			return err;
-- 
2.32.0

[PATCH net 5/6] net: dsa: mv88e6xxx: Fix inband AN for 2500base-x on 88E6393X family

[Marek Behún]

Inband AN is broken on Amethyst in 2500base-x mode when set by standard
mechanism (via cmode).

(There probably is some weird setting done by default in the switch for
 this mode that make it cycle in some state or something, because when
 the peer is the mvneta controller, it receives link change interrupts
 every ~0.3ms, but the link is always down.)

Get around this by configuring the PCS mode to 1000base-x (where inband
AN works), and then changing the SerDes frequency while SerDes
transmitter and receiver are disabled, before enabling SerDes PHY. After
disabling SerDes PHY, change the PCS mode back to 2500base-x, to avoid
confusing the device (if we leave it at 1000base-x PCS mode but with
different frequency, and then change cmode to sgmii, the device won't
change the frequency because it thinks it already has the correct one).

The register which changes the frequency is undocumented. I discovered
it by going through all registers in the ranges 4.f000-4.f100 and
1e.8000-1e.8200 for all SerDes cmodes (sgmii, 1000base-x, 2500base-x,
5gbase-r, 10gbase-r, usxgmii) and filtering out registers that didn't
make sense (the value was the same for modes which have different
frequency). The result of this was:

    reg   sgmii 1000base-x 2500base-x 5gbase-r 10gbase-r usxgmii
  04.f002  005b       0058       0059     005c      005d    005f
  04.f076  3000       0000       1000     4000      5000    7000
  04.f07c  0950       0950       1850     0550      0150    0150
  1e.8000  0059       0059       0058     0055      0051    0051
  1e.8140  0e20       0e20       0e28     0e21      0e42    0e42

Register 04.f002 is the documented Port Operational Confiuration
register, it's last 3 bits select PCS type, so changing this register
also changes the frequency to the appropriate value.

Registers 04.f076 and 04.f07c are not writable.

Undocumented register 1e.8000 was the one: changing bits 3:0 from 9 to 8
changed SerDes frequency to 3.125 GHz, while leaving the value of PCS
mode in register 04.f002.2:0 at 1000base-x. Inband autonegotiation
started working correctly.

(I didn't try anything with register 1e.8140 since 1e.8000 solved the
 problem.)

Since I don't have documentation for this register 1e.8000.3:0, I am
using the constants without names, but my hypothesis is that this
register selects PHY frequency. If in the future I have access to an
oscilloscope able to handle these frequencies, I will try to test this
hypothesis.

Fixes: de776d0d316f ("net: dsa: mv88e6xxx: add support for mv88e6393x family")
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/net/dsa/mv88e6xxx/serdes.c | 63 +++++++++++++++++++++++++++++-
 drivers/net/dsa/mv88e6xxx/serdes.h |  1 +
 2 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index 9901219780e6..d297131afe21 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -1419,6 +1419,54 @@ static int mv88e6393x_serdes_erratum_5_2(struct mv88e6xxx_chip *chip, int lane,
 	return 0;
 }
 
+static int mv88e6393x_serdes_fix_2500basex_an(struct mv88e6xxx_chip *chip,
+					      int lane, u8 cmode, bool on)
+{
+	u16 reg;
+	int err;
+
+	if (cmode != MV88E6XXX_PORT_STS_CMODE_2500BASEX)
+		return 0;
+
+	/* Inband AN is broken on Amethyst in 2500base-x mode when set by
+	 * standard mechanism (via cmode).
+	 * We can get around this by configuring the PCS mode to 1000base-x
+	 * and then writing value 0x58 to register 1e.8000. (This must be done
+	 * while SerDes receiver and transmitter are disabled, which is, when
+	 * this function is called.)
+	 * It seem that when we do this configuration to 2500base-x mode (by
+	 * changing PCS mode to 1000base-x and frequency to 3.125 GHz from
+	 * 1.25 GHz) and then configure to sgmii or 1000base-x, the device
+	 * thinks that it already has SerDes at 1.25 GHz and does not change
+	 * the 1e.8000 register, leaving SerDes at 3.125 GHz.
+	 * To avoid this, change PCS mode back to 2500base-x when disabling
+	 * SerDes from 2500base-x mode.
+	 */
+	err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
+				    MV88E6393X_SERDES_POC, &reg);
+	if (err)
+		return err;
+
+	reg &= ~(MV88E6393X_SERDES_POC_PCS_MASK | MV88E6393X_SERDES_POC_AN);
+	if (on)
+		reg |= MV88E6393X_SERDES_POC_PCS_1000BASEX |
+		       MV88E6393X_SERDES_POC_AN;
+	else
+		reg |= MV88E6393X_SERDES_POC_PCS_2500BASEX;
+	reg |= MV88E6393X_SERDES_POC_RESET;
+
+	err = mv88e6390_serdes_write(chip, lane, MDIO_MMD_PHYXS,
+				     MV88E6393X_SERDES_POC, reg);
+	if (err)
+		return err;
+
+	err = mv88e6390_serdes_write(chip, lane, MDIO_MMD_VEND1, 0x8000, 0x58);
+	if (err)
+		return err;
+
+	return 0;
+}
+
 int mv88e6393x_serdes_power(struct mv88e6xxx_chip *chip, int port, int lane,
 			    bool on)
 {
@@ -1437,6 +1485,11 @@ int mv88e6393x_serdes_power(struct mv88e6xxx_chip *chip, int port, int lane,
 		if (err)
 			return err;
 
+		err = mv88e6393x_serdes_fix_2500basex_an(chip, lane, cmode,
+							 true);
+		if (err)
+			return err;
+
 		err = mv88e6393x_serdes_power_lane(chip, lane, true);
 		if (err)
 			return err;
@@ -1457,8 +1510,14 @@ int mv88e6393x_serdes_power(struct mv88e6xxx_chip *chip, int port, int lane,
 	if (err)
 		return err;
 
-	if (!on)
-		return mv88e6393x_serdes_power_lane(chip, lane, false);
+	if (!on) {
+		err = mv88e6393x_serdes_power_lane(chip, lane, false);
+		if (err)
+			return err;
+
+		return mv88e6393x_serdes_fix_2500basex_an(chip, lane, cmode,
+							  false);
+	}
 
 	return 0;
 }
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.h b/drivers/net/dsa/mv88e6xxx/serdes.h
index e9292c8beee4..8dd8ed225b45 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.h
+++ b/drivers/net/dsa/mv88e6xxx/serdes.h
@@ -93,6 +93,7 @@
 #define MV88E6393X_SERDES_POC_PCS_MASK		0x0007
 #define MV88E6393X_SERDES_POC_RESET		BIT(15)
 #define MV88E6393X_SERDES_POC_PDOWN		BIT(5)
+#define MV88E6393X_SERDES_POC_AN		BIT(3)
 #define MV88E6393X_SERDES_CTRL1			0xf003
 #define MV88E6393X_SERDES_CTRL1_TX_PDOWN	BIT(9)
 #define MV88E6393X_SERDES_CTRL1_RX_PDOWN	BIT(8)
-- 
2.32.0

[PATCH net 6/6] net: dsa: mv88e6xxx: Link in pcs_get_state() if AN is bypassed

[Marek Behún]

Function mv88e6xxx_serdes_pcs_get_state() currently does not report link
up if AN is enabled, Link bit is set, but Speed and Duplex Resolved bit
is not set, which testing shows is the case for when auto-negotiation
was bypassed (we have enabled AN but link partner does not).

An example of such link partner is Marvell 88X3310 PHY, when put into
the mode where host interface changes between 10gbase-r, 5gbase-r,
2500base-x and sgmii according to copper speed. The 88X3310 does not
enable AN in 2500base-x, and so SerDes on mv88e6xxx currently does not
link with it.

Fix this.

Fixes: a5a6858b793f ("net: dsa: mv88e6xxx: extend phylink to Serdes PHYs")
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/net/dsa/mv88e6xxx/serdes.c | 38 +++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index d297131afe21..a145598905a5 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -50,11 +50,13 @@ static int mv88e6390_serdes_write(struct mv88e6xxx_chip *chip,
 }
 
 static int mv88e6xxx_serdes_pcs_get_state(struct mv88e6xxx_chip *chip,
-					  u16 status, u16 lpa,
+					  u16 ctrl, u16 status, u16 lpa,
 					  struct phylink_link_state *state)
 {
+	state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK);
+
 	if (status & MV88E6390_SGMII_PHY_STATUS_SPD_DPL_VALID) {
-		state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK);
+		state->an_complete = !!(ctrl & BMCR_ANENABLE);
 		state->duplex = status &
 				MV88E6390_SGMII_PHY_STATUS_DUPLEX_FULL ?
 			                         DUPLEX_FULL : DUPLEX_HALF;
@@ -81,6 +83,17 @@ static int mv88e6xxx_serdes_pcs_get_state(struct mv88e6xxx_chip *chip,
 			dev_err(chip->dev, "invalid PHY speed\n");
 			return -EINVAL;
 		}
+	} else if (state->link &&
+		   state->interface != PHY_INTERFACE_MODE_SGMII) {
+		/* The Spped and Duplex Resolved register is always 1 when AN is
+		 * disabled. If it is not set, and link is up, it means that the
+		 * SerDes PHY AN bypass feature was invoked.
+		 */
+		state->duplex = DUPLEX_FULL;
+		if (state->interface == PHY_INTERFACE_MODE_2500BASEX)
+			state->speed = SPEED_2500;
+		else
+			state->speed = SPEED_1000;
 	} else {
 		state->link = false;
 	}
@@ -168,9 +181,15 @@ int mv88e6352_serdes_pcs_config(struct mv88e6xxx_chip *chip, int port,
 int mv88e6352_serdes_pcs_get_state(struct mv88e6xxx_chip *chip, int port,
 				   int lane, struct phylink_link_state *state)
 {
-	u16 lpa, status;
+	u16 lpa, status, ctrl;
 	int err;
 
+	err = mv88e6352_serdes_read(chip, MII_BMCR, &ctrl);
+	if (err) {
+		dev_err(chip->dev, "can't read Serdes PHY control: %d\n", err);
+		return err;
+	}
+
 	err = mv88e6352_serdes_read(chip, 0x11, &status);
 	if (err) {
 		dev_err(chip->dev, "can't read Serdes PHY status: %d\n", err);
@@ -183,7 +202,7 @@ int mv88e6352_serdes_pcs_get_state(struct mv88e6xxx_chip *chip, int port,
 		return err;
 	}
 
-	return mv88e6xxx_serdes_pcs_get_state(chip, status, lpa, state);
+	return mv88e6xxx_serdes_pcs_get_state(chip, ctrl, status, lpa, state);
 }
 
 int mv88e6352_serdes_pcs_an_restart(struct mv88e6xxx_chip *chip, int port,
@@ -883,9 +902,16 @@ int mv88e6390_serdes_pcs_config(struct mv88e6xxx_chip *chip, int port,
 static int mv88e6390_serdes_pcs_get_state_sgmii(struct mv88e6xxx_chip *chip,
 	int port, int lane, struct phylink_link_state *state)
 {
-	u16 lpa, status;
+	u16 lpa, status, ctrl;
 	int err;
 
+	err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
+				    MV88E6390_SGMII_BMCR, &ctrl);
+	if (err) {
+		dev_err(chip->dev, "can't read Serdes PHY control: %d\n", err);
+		return err;
+	}
+
 	err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
 				    MV88E6390_SGMII_PHY_STATUS, &status);
 	if (err) {
@@ -900,7 +926,7 @@ static int mv88e6390_serdes_pcs_get_state_sgmii(struct mv88e6xxx_chip *chip,
 		return err;
 	}
 
-	return mv88e6xxx_serdes_pcs_get_state(chip, status, lpa, state);
+	return mv88e6xxx_serdes_pcs_get_state(chip, ctrl, status, lpa, state);
 }
 
 static int mv88e6390_serdes_pcs_get_state_10g(struct mv88e6xxx_chip *chip,
-- 
2.32.0

Re: [PATCH net 6/6] net: dsa: mv88e6xxx: Link in pcs_get_state() if AN is bypassed

[Russell King (Oracle)]

On Mon, Nov 29, 2021 at 08:58:23PM +0100, Marek Behún wrote:
>  static int mv88e6xxx_serdes_pcs_get_state(struct mv88e6xxx_chip *chip,
> -					  u16 status, u16 lpa,
> +					  u16 ctrl, u16 status, u16 lpa,
>  					  struct phylink_link_state *state)
>  {
> +	state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK);
> +
>  	if (status & MV88E6390_SGMII_PHY_STATUS_SPD_DPL_VALID) {
> -		state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK);
> +		state->an_complete = !!(ctrl & BMCR_ANENABLE);

I think I'd much rather report the value of BMSR_ANEGCAPABLE - since
an_complete controls the BMSR_ANEGCAPABLE bit in the emulated PHY
that userspace sees. Otherwise, an_complete is not used.

state->link is the key that phylink uses to know whether it can
trust the status being reported.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net 6/6] net: dsa: mv88e6xxx: Link in pcs_get_state() if AN is bypassed

[Marek Behún]

On Mon, 29 Nov 2021 23:14:17 +0000
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Mon, Nov 29, 2021 at 08:58:23PM +0100, Marek Behún wrote:
> >  static int mv88e6xxx_serdes_pcs_get_state(struct mv88e6xxx_chip *chip,
> > -					  u16 status, u16 lpa,
> > +					  u16 ctrl, u16 status, u16 lpa,
> >  					  struct phylink_link_state *state)
> >  {
> > +	state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK);
> > +
> >  	if (status & MV88E6390_SGMII_PHY_STATUS_SPD_DPL_VALID) {
> > -		state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK);
> > +		state->an_complete = !!(ctrl & BMCR_ANENABLE);  
> 
> I think I'd much rather report the value of BMSR_ANEGCAPABLE - since
> an_complete controls the BMSR_ANEGCAPABLE bit in the emulated PHY
> that userspace sees. Otherwise, an_complete is not used.
> 
> state->link is the key that phylink uses to know whether it can
> trust the status being reported.

Isn't BMSR_ANEGCAPABLE set to 1 even if aneg is disabled in BMCR?
I will test tomorrow.

The reason why I used BMCR_ANENABLE is that if we don't enable AN, the
PHY will report SPD_DPL_VALID if link is up. But clearly AN is not
completed in that case, because it was never enabled in the first place.

Marek

Re: [PATCH net 6/6] net: dsa: mv88e6xxx: Link in pcs_get_state() if AN is bypassed

[Marek Behún]

On Tue, 30 Nov 2021 01:18:12 +0100
Marek Behún <kabel@kernel.org> wrote:

> On Mon, 29 Nov 2021 23:14:17 +0000
> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> 
> > On Mon, Nov 29, 2021 at 08:58:23PM +0100, Marek Behún wrote:  
> > >  static int mv88e6xxx_serdes_pcs_get_state(struct mv88e6xxx_chip *chip,
> > > -					  u16 status, u16 lpa,
> > > +					  u16 ctrl, u16 status, u16 lpa,
> > >  					  struct phylink_link_state *state)
> > >  {
> > > +	state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK);
> > > +
> > >  	if (status & MV88E6390_SGMII_PHY_STATUS_SPD_DPL_VALID) {
> > > -		state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK);
> > > +		state->an_complete = !!(ctrl & BMCR_ANENABLE);    
> > 
> > I think I'd much rather report the value of BMSR_ANEGCAPABLE - since
> > an_complete controls the BMSR_ANEGCAPABLE bit in the emulated PHY
> > that userspace sees. Otherwise, an_complete is not used.
> > 
> > state->link is the key that phylink uses to know whether it can
> > trust the status being reported.  
> 
> Isn't BMSR_ANEGCAPABLE set to 1 even if aneg is disabled in BMCR?
> I will test tomorrow.
> 
> The reason why I used BMCR_ANENABLE is that if we don't enable AN, the
> PHY will report SPD_DPL_VALID if link is up. But clearly AN is not
> completed in that case, because it was never enabled in the first place.

Seems that BMSR_ANEGCAPABLE is set even if AN is disabled in BMCR.

If phylink_autoneg_inband() is not true mv88e6*_serdes_pcs_config(),
then we aren't enabling AN. But in this case SPD_DPL_VALID is always 1,
so if we use, as you say, an_complete=BMSR_ANEGCAPABLE in
mv88e6xxx_serdes_pcs_get_state(), we will always set an_complete=true, even
if AN wasn't enabled.

I was under the impression that
  state->an_complete
should only be set to true if AN is enabled.

Marek

Re: [PATCH net 6/6] net: dsa: mv88e6xxx: Link in pcs_get_state() if AN is bypassed

[Russell King (Oracle)]

On Tue, Nov 30, 2021 at 05:09:11PM +0100, Marek Behún wrote:
> Seems that BMSR_ANEGCAPABLE is set even if AN is disabled in BMCR.

Hmm, that behaviour goes against 22.2.4.2.10:

A PHY shall return a value of zero in bit 1.5 if Auto-Negotiation is
disabled by clearing bit 0.12. A PHY shall also return a value of zero
in bit 1.5 if it lacks the ability to perform Auto-Negotiation.

> I was under the impression that
>   state->an_complete
> should only be set to true if AN is enabled.

Yes - however as you've stated, the PHY doesn't follow 802.3 behaviour
so I guess we should make the emulation appear compliant by fixing it
like this.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net 6/6] net: dsa: mv88e6xxx: Link in pcs_get_state() if AN is bypassed

[Marek Behún]

On Tue, 30 Nov 2021 16:15:50 +0000
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Tue, Nov 30, 2021 at 05:09:11PM +0100, Marek Behún wrote:
> > Seems that BMSR_ANEGCAPABLE is set even if AN is disabled in BMCR.  
> 
> Hmm, that behaviour goes against 22.2.4.2.10:
> 
> A PHY shall return a value of zero in bit 1.5 if Auto-Negotiation is
> disabled by clearing bit 0.12. A PHY shall also return a value of zero
> in bit 1.5 if it lacks the ability to perform Auto-Negotiation.
> 
> > I was under the impression that
> >   state->an_complete
> > should only be set to true if AN is enabled.  
> 
> Yes - however as you've stated, the PHY doesn't follow 802.3 behaviour
> so I guess we should make the emulation appear compliant by fixing it
> like this.

OK, I will use BMCR_ANENABLE and add a comment explaining that we can't
use BMSR_ANEGCAPABLE because the PHY violates standard. Would that be
okay?

Re: [PATCH net 6/6] net: dsa: mv88e6xxx: Link in pcs_get_state() if AN is bypassed

[Russell King (Oracle)]

On Tue, Nov 30, 2021 at 05:18:59PM +0100, Marek Behún wrote:
> On Tue, 30 Nov 2021 16:15:50 +0000
> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> 
> > On Tue, Nov 30, 2021 at 05:09:11PM +0100, Marek Behún wrote:
> > > Seems that BMSR_ANEGCAPABLE is set even if AN is disabled in BMCR.  
> > 
> > Hmm, that behaviour goes against 22.2.4.2.10:
> > 
> > A PHY shall return a value of zero in bit 1.5 if Auto-Negotiation is
> > disabled by clearing bit 0.12. A PHY shall also return a value of zero
> > in bit 1.5 if it lacks the ability to perform Auto-Negotiation.
> > 
> > > I was under the impression that
> > >   state->an_complete
> > > should only be set to true if AN is enabled.  
> > 
> > Yes - however as you've stated, the PHY doesn't follow 802.3 behaviour
> > so I guess we should make the emulation appear compliant by fixing it
> > like this.
> 
> OK, I will use BMCR_ANENABLE and add a comment explaining that we can't
> use BMSR_ANEGCAPABLE because the PHY violates standard. Would that be
> okay?

Yes, thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!