[PATCH net 1/3] net: dsa: mv88e6xxx: Fix forcing speed & duplex when changing to 2500base-x mode

[PATCH net 1/3] net: dsa: mv88e6xxx: Fix forcing speed & duplex when changing to 2500base-x mode

[Marek Behún]

Commit 64d47d50be7a ("net: dsa: mv88e6xxx: configure interface settings
in mac_config") removed forcing of speed and duplex from
mv88e6xxx_mac_config(), where the link is forced down, and left it only
in mv88e6xxx_mac_link_up(), by which time link is unforced.

It seems that in 2500base-x (at least on 88E6190), if the link is not
forced down, the forcing of new settings for speed and duplex doesn't
take.

Fix this by forcing link down.

Fixes: 64d47d50be7a ("net: dsa: mv88e6xxx: configure interface settings in mac_config")
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index f00cbf5753b9..ddb13cecb3ac 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -785,12 +785,17 @@ static void mv88e6xxx_mac_link_up(struct dsa_switch *ds, int port,
 	if ((!mv88e6xxx_phy_is_internal(ds, port) &&
 	     !mv88e6xxx_port_ppu_updates(chip, port)) ||
 	    mode == MLO_AN_FIXED) {
-		/* FIXME: for an automedia port, should we force the link
-		 * down here - what if the link comes up due to "other" media
-		 * while we're bringing the port up, how is the exclusivity
-		 * handled in the Marvell hardware? E.g. port 2 on 88E6390
-		 * shared between internal PHY and Serdes.
+		/* FIXME: we need to force the link down here, otherwise the
+		 * forcing of link speed and duplex by .port_set_speed_duplex()
+		 * doesn't work for some modes.
+		 * But what if the link comes up due to "other" media while
+		 * we're bringing the port up, how is the exclusivity handled in
+		 * the Marvell hardware? E.g. port 2 on 88E6390 shared between
+		 * internal PHY and Serdes.
 		 */
+		if (ops->port_sync_link)
+			err = ops->port_sync_link(chip, port, mode, false);
+
 		err = mv88e6xxx_serdes_pcs_link_up(chip, port, mode, speed,
 						   duplex);
 		if (err)
-- 
2.32.0

[PATCH net 2/3] net: dsa: mv88e6xxx: Fix reading sgmii link status register

[Marek Behún]

The documentation says this about the SerDes PHY register 4.2001.2:
  This register bit indicates when link was lost since the last
  read. For the current link status, read this register
  back-to-back.

Thus we need to read it twice to get the current value of the register.

The wrong value is read when phylink requests change from sgmii to
2500base-x mode, and link won't come up. This fixes the bug.

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 | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index 6ea003678798..bc198ef06745 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -1041,15 +1041,25 @@ static void mv88e6390_serdes_irq_link_sgmii(struct mv88e6xxx_chip *chip,
 	u16 bmsr;
 	int err;
 
-	/* If the link has dropped, we want to know about it. */
+	/* If the link has dropped, we want to know about it.
+	 * This register bit indicates when link was lost since the last read.
+	 * We need to read it twice to get the current value.
+	 */
 	err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
 				    MV88E6390_SGMII_BMSR, &bmsr);
-	if (err) {
-		dev_err(chip->dev, "can't read Serdes BMSR: %d\n", err);
-		return;
-	}
+	if (err)
+		goto err;
+
+	err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
+				    MV88E6390_SGMII_BMSR, &bmsr);
+	if (err)
+		goto err;
 
 	dsa_port_phylink_mac_change(chip->ds, port, !!(bmsr & BMSR_LSTATUS));
+
+	return;
+err:
+	dev_err(chip->dev, "can't read Serdes BMSR: %d\n", err);
 }
 
 static void mv88e6393x_serdes_irq_link_10g(struct mv88e6xxx_chip *chip,
-- 
2.32.0

[PATCH net 3/3] net: dsa: mv88e6xxx: Link in pcs_get_state() even if LP has AN disabled

[Marek Behún]

Function mv88e6xxx_serdes_pcs_get_state() currently does not report
link up if AN is not complete. This is in contrast to for example the
mvneta's mvneta_mac_pcs_get_state() implementation, where link is simply
taken from link bit.

For 1000base-x and 2500base-x modes, it is possible that the link
partner has autonegotiation disabled. In this case we get zero in the
SPD_DPL_VALID and we won't link, even if we can.

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 | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index bc198ef06745..dd3fba7aab99 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -53,8 +53,11 @@ static int mv88e6xxx_serdes_pcs_get_state(struct mv88e6xxx_chip *chip,
 					  u16 status, u16 lpa,
 					  struct phylink_link_state *state)
 {
-	if (status & MV88E6390_SGMII_PHY_STATUS_SPD_DPL_VALID) {
-		state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK);
+	state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK);
+	state->an_complete = !!(status &
+				MV88E6390_SGMII_PHY_STATUS_SPD_DPL_VALID);
+
+	if (state->an_complete) {
 		state->duplex = status &
 				MV88E6390_SGMII_PHY_STATUS_DUPLEX_FULL ?
 			                         DUPLEX_FULL : DUPLEX_HALF;
@@ -81,8 +84,13 @@ static int mv88e6xxx_serdes_pcs_get_state(struct mv88e6xxx_chip *chip,
 			dev_err(chip->dev, "invalid PHY speed\n");
 			return -EINVAL;
 		}
-	} else {
-		state->link = false;
+	} else if (state->link &&
+		   state->interface != PHY_INTERFACE_MODE_SGMII) {
+		state->duplex = DUPLEX_FULL;
+		if (state->interface == PHY_INTERFACE_MODE_2500BASEX)
+			state->speed = SPEED_2500;
+		else
+			state->speed = SPEED_1000;
 	}
 
 	if (state->interface == PHY_INTERFACE_MODE_2500BASEX)
-- 
2.32.0

Re: [PATCH net 1/3] net: dsa: mv88e6xxx: Fix forcing speed & duplex when changing to 2500base-x mode

[Russell King (Oracle)]

Please wait before applying; I have comments on these patches, but I
won't have time to provide those comments for a couple of days.
Thanks.

On Wed, Nov 10, 2021 at 05:10:08AM +0100, Marek Behún wrote:
> Commit 64d47d50be7a ("net: dsa: mv88e6xxx: configure interface settings
> in mac_config") removed forcing of speed and duplex from
> mv88e6xxx_mac_config(), where the link is forced down, and left it only
> in mv88e6xxx_mac_link_up(), by which time link is unforced.
> 
> It seems that in 2500base-x (at least on 88E6190), if the link is not
> forced down, the forcing of new settings for speed and duplex doesn't
> take.
> 
> Fix this by forcing link down.
> 
> Fixes: 64d47d50be7a ("net: dsa: mv88e6xxx: configure interface settings in mac_config")
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index f00cbf5753b9..ddb13cecb3ac 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -785,12 +785,17 @@ static void mv88e6xxx_mac_link_up(struct dsa_switch *ds, int port,
>  	if ((!mv88e6xxx_phy_is_internal(ds, port) &&
>  	     !mv88e6xxx_port_ppu_updates(chip, port)) ||
>  	    mode == MLO_AN_FIXED) {
> -		/* FIXME: for an automedia port, should we force the link
> -		 * down here - what if the link comes up due to "other" media
> -		 * while we're bringing the port up, how is the exclusivity
> -		 * handled in the Marvell hardware? E.g. port 2 on 88E6390
> -		 * shared between internal PHY and Serdes.
> +		/* FIXME: we need to force the link down here, otherwise the
> +		 * forcing of link speed and duplex by .port_set_speed_duplex()
> +		 * doesn't work for some modes.
> +		 * But what if the link comes up due to "other" media while
> +		 * we're bringing the port up, how is the exclusivity handled in
> +		 * the Marvell hardware? E.g. port 2 on 88E6390 shared between
> +		 * internal PHY and Serdes.
>  		 */
> +		if (ops->port_sync_link)
> +			err = ops->port_sync_link(chip, port, mode, false);
> +
>  		err = mv88e6xxx_serdes_pcs_link_up(chip, port, mode, speed,
>  						   duplex);
>  		if (err)
> -- 
> 2.32.0
> 
> 

-- 
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 1/3] net: dsa: mv88e6xxx: Fix forcing speed & duplex when changing to 2500base-x mode

[Andrew Lunn]

Hi Marek

Please always include a patch 0/X for a patchset explaining what the
patches as a whole do. The text should then be used as the merge
commit for the patchset.

    Andrew

Re: [PATCH net 1/3] net: dsa: mv88e6xxx: Fix forcing speed & duplex when changing to 2500base-x mode

[Marek Behún]

On Wed, 10 Nov 2021 14:18:10 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> Hi Marek
> 
> Please always include a patch 0/X for a patchset explaining what the
> patches as a whole do. The text should then be used as the merge
> commit for the patchset.
> 
>     Andrew

OK, but in this case I will wait till Russell gives his comments before
sending v2.

Marek