[PATCH net 0/3] net: dsa: mv88e6xxx serdes fixes

[PATCH net 0/3] net: dsa: mv88e6xxx serdes fixes

[Russell King (Oracle)]

Hi,

This series addresses a number of deficiencies in the mv88e6xxx serdes
handling, namely:

1) incorrectly filling out state->an_complete - which was incorrectly
fixed due to confusion with the ANEGCAPABLE bit - patch from Marek to
fix this.

2) make the error print in patch 1 consistent with all the other error
prints.

3) ensure that we always capture a link-drop event by reading the link
status from the BMSR as well, and report link down if BMSR says the link
failed.

 drivers/net/dsa/mv88e6xxx/serdes.c | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

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

[PATCH net 1/3] net: dsa: mv88e6xxx: use BMSR_ANEGCOMPLETE bit for filling an_complete

[Russell King]

From: =?UTF-8?q?Marek=20Beh=C3=BAn?= <kabel@kernel.org>

Commit ede359d8843a ("net: dsa: mv88e6xxx: Link in pcs_get_state() if AN
is bypassed") added the ability to link if AN was bypassed, and added
filling of state->an_complete field, but set it to true if AN was
enabled in BMCR, not when AN was reported complete in BMSR.

This was done because for some reason, when I wanted to use BMSR value
to infer an_complete, I was looking at BMSR_ANEGCAPABLE bit (which was
always 1), instead of BMSR_ANEGCOMPLETE bit.

Use BMSR_ANEGCOMPLETE for filling state->an_complete.

Fixes: ede359d8843a ("net: dsa: mv88e6xxx: Link in pcs_get_state() if AN is bypassed")
Signed-off-by: Marek Behún <kabel@kernel.org>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/mv88e6xxx/serdes.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index 7b37d45bc9fb..1a19c5284f2c 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -50,22 +50,17 @@ static int mv88e6390_serdes_write(struct mv88e6xxx_chip *chip,
 }
 
 static int mv88e6xxx_serdes_pcs_get_state(struct mv88e6xxx_chip *chip,
-					  u16 ctrl, u16 status, u16 lpa,
+					  u16 bmsr, u16 lpa, u16 status,
 					  struct phylink_link_state *state)
 {
 	state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK);
+	state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
 
 	if (status & MV88E6390_SGMII_PHY_STATUS_SPD_DPL_VALID) {
 		/* The Spped and Duplex Resolved register is 1 if AN is enabled
 		 * and complete, or if AN is disabled. So with disabled AN we
-		 * still get here on link up. But we want to set an_complete
-		 * only if AN was enabled, thus we look at BMCR_ANENABLE.
-		 * (According to 802.3-2008 section 22.2.4.2.10, we should be
-		 *  able to get this same value from BMSR_ANEGCAPABLE, but tests
-		 *  show that these Marvell PHYs don't conform to this part of
-		 *  the specificaion - BMSR_ANEGCAPABLE is simply always 1.)
+		 * still get here on link up.
 		 */
-		state->an_complete = !!(ctrl & BMCR_ANENABLE);
 		state->duplex = status &
 				MV88E6390_SGMII_PHY_STATUS_DUPLEX_FULL ?
 			                         DUPLEX_FULL : DUPLEX_HALF;
@@ -191,12 +186,12 @@ 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, ctrl;
+	u16 bmsr, lpa, status;
 	int err;
 
-	err = mv88e6352_serdes_read(chip, MII_BMCR, &ctrl);
+	err = mv88e6352_serdes_read(chip, MII_BMSR, &bmsr);
 	if (err) {
-		dev_err(chip->dev, "can't read Serdes PHY control: %d\n", err);
+		dev_err(chip->dev, "can't read Serdes BMSR: %d\n", err);
 		return err;
 	}
 
@@ -212,7 +207,7 @@ int mv88e6352_serdes_pcs_get_state(struct mv88e6xxx_chip *chip, int port,
 		return err;
 	}
 
-	return mv88e6xxx_serdes_pcs_get_state(chip, ctrl, status, lpa, state);
+	return mv88e6xxx_serdes_pcs_get_state(chip, bmsr, lpa, status, state);
 }
 
 int mv88e6352_serdes_pcs_an_restart(struct mv88e6xxx_chip *chip, int port,
@@ -918,13 +913,13 @@ 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, ctrl;
+	u16 bmsr, lpa, status;
 	int err;
 
 	err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
-				    MV88E6390_SGMII_BMCR, &ctrl);
+				    MV88E6390_SGMII_BMSR, &bmsr);
 	if (err) {
-		dev_err(chip->dev, "can't read Serdes PHY control: %d\n", err);
+		dev_err(chip->dev, "can't read Serdes PHY BMSR: %d\n", err);
 		return err;
 	}
 
@@ -942,7 +937,7 @@ static int mv88e6390_serdes_pcs_get_state_sgmii(struct mv88e6xxx_chip *chip,
 		return err;
 	}
 
-	return mv88e6xxx_serdes_pcs_get_state(chip, ctrl, status, lpa, state);
+	return mv88e6xxx_serdes_pcs_get_state(chip, bmsr, lpa, status, state);
 }
 
 static int mv88e6390_serdes_pcs_get_state_10g(struct mv88e6xxx_chip *chip,
-- 
2.30.2

Re: [PATCH net 1/3] net: dsa: mv88e6xxx: use BMSR_ANEGCOMPLETE bit for filling an_complete

[Andrew Lunn]

On Wed, Apr 13, 2022 at 05:53:52PM +0100, Russell King wrote:
> From: =?UTF-8?q?Marek=20Beh=C3=BAn?= <kabel@kernel.org>

Hi Russell

Does git am parse that correctly? At least it is something
Jakub/DaveM/Paolo needs to keep an eye on when they accept the series.

> 
> Commit ede359d8843a ("net: dsa: mv88e6xxx: Link in pcs_get_state() if AN
> is bypassed") added the ability to link if AN was bypassed, and added
> filling of state->an_complete field, but set it to true if AN was
> enabled in BMCR, not when AN was reported complete in BMSR.
> 
> This was done because for some reason, when I wanted to use BMSR value
> to infer an_complete, I was looking at BMSR_ANEGCAPABLE bit (which was
> always 1), instead of BMSR_ANEGCOMPLETE bit.
> 
> Use BMSR_ANEGCOMPLETE for filling state->an_complete.
> 
> Fixes: ede359d8843a ("net: dsa: mv88e6xxx: Link in pcs_get_state() if AN is bypassed")
> Signed-off-by: Marek Behún <kabel@kernel.org>
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/dsa/mv88e6xxx/serdes.c | 27 +++++++++++----------------
>  1 file changed, 11 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
> index 7b37d45bc9fb..1a19c5284f2c 100644
> --- a/drivers/net/dsa/mv88e6xxx/serdes.c
> +++ b/drivers/net/dsa/mv88e6xxx/serdes.c
> @@ -50,22 +50,17 @@ static int mv88e6390_serdes_write(struct mv88e6xxx_chip *chip,
>  }
>  
>  static int mv88e6xxx_serdes_pcs_get_state(struct mv88e6xxx_chip *chip,
> -					  u16 ctrl, u16 status, u16 lpa,
> +					  u16 bmsr, u16 lpa, u16 status,
>  					  struct phylink_link_state *state)
>  {
>  	state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK);
> +	state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
>  
>  	if (status & MV88E6390_SGMII_PHY_STATUS_SPD_DPL_VALID) {
>  		/* The Spped and Duplex Resolved register is 1 if AN is enabled

It looks like there is a typ0 here for speed.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net 1/3] net: dsa: mv88e6xxx: use BMSR_ANEGCOMPLETE bit for filling an_complete

[Russell King (Oracle)]

On Wed, Apr 13, 2022 at 08:46:32PM +0200, Andrew Lunn wrote:
> On Wed, Apr 13, 2022 at 05:53:52PM +0100, Russell King wrote:
> > From: =?UTF-8?q?Marek=20Beh=C3=BAn?= <kabel@kernel.org>
> 
> Hi Russell
> 
> Does git am parse that correctly? At least it is something
> Jakub/DaveM/Paolo needs to keep an eye on when they accept the series.

If it doesn't git is rather brain-dead - the above is generated by
git format-patch... if git produces emails that it can't accept then
there's definitely something wrong with git!

> > Commit ede359d8843a ("net: dsa: mv88e6xxx: Link in pcs_get_state() if AN
> > is bypassed") added the ability to link if AN was bypassed, and added
> > filling of state->an_complete field, but set it to true if AN was
> > enabled in BMCR, not when AN was reported complete in BMSR.
> > 
> > This was done because for some reason, when I wanted to use BMSR value
> > to infer an_complete, I was looking at BMSR_ANEGCAPABLE bit (which was
> > always 1), instead of BMSR_ANEGCOMPLETE bit.
> > 
> > Use BMSR_ANEGCOMPLETE for filling state->an_complete.
> > 
> > Fixes: ede359d8843a ("net: dsa: mv88e6xxx: Link in pcs_get_state() if AN is bypassed")
> > Signed-off-by: Marek Behún <kabel@kernel.org>
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/net/dsa/mv88e6xxx/serdes.c | 27 +++++++++++----------------
> >  1 file changed, 11 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
> > index 7b37d45bc9fb..1a19c5284f2c 100644
> > --- a/drivers/net/dsa/mv88e6xxx/serdes.c
> > +++ b/drivers/net/dsa/mv88e6xxx/serdes.c
> > @@ -50,22 +50,17 @@ static int mv88e6390_serdes_write(struct mv88e6xxx_chip *chip,
> >  }
> >  
> >  static int mv88e6xxx_serdes_pcs_get_state(struct mv88e6xxx_chip *chip,
> > -					  u16 ctrl, u16 status, u16 lpa,
> > +					  u16 bmsr, u16 lpa, u16 status,
> >  					  struct phylink_link_state *state)
> >  {
> >  	state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK);
> > +	state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
> >  
> >  	if (status & MV88E6390_SGMII_PHY_STATUS_SPD_DPL_VALID) {
> >  		/* The Spped and Duplex Resolved register is 1 if AN is enabled
> 
> It looks like there is a typ0 here for speed.

Will fix.

> Reviewed-by: Andrew Lunn <andrew@lunn.ch>

Thanks.

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

[PATCH net 2/3] net: dsa: mv88e6xxx: fix BMSR error to be consistent with others

[Russell King (Oracle)]

Other errors accessing the registers in mv88e6352_serdes_pcs_get_state()
print "PHY " before the register name, except for the BMSR. Make this
consistent with the other error messages.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/mv88e6xxx/serdes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index 1a19c5284f2c..47bf87d530b0 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -191,7 +191,7 @@ int mv88e6352_serdes_pcs_get_state(struct mv88e6xxx_chip *chip, int port,
 
 	err = mv88e6352_serdes_read(chip, MII_BMSR, &bmsr);
 	if (err) {
-		dev_err(chip->dev, "can't read Serdes BMSR: %d\n", err);
+		dev_err(chip->dev, "can't read Serdes PHY BMSR: %d\n", err);
 		return err;
 	}
 
-- 
2.30.2

Re: [PATCH net 2/3] net: dsa: mv88e6xxx: fix BMSR error to be consistent with others

[Andrew Lunn]

On Wed, Apr 13, 2022 at 05:53:57PM +0100, Russell King (Oracle) wrote:
> Other errors accessing the registers in mv88e6352_serdes_pcs_get_state()
> print "PHY " before the register name, except for the BMSR. Make this
> consistent with the other error messages.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net 3/3] net: dsa: mv88e6xxx: correctly report serdes link failure

[Russell King (Oracle)]

Phylink wants to know if the link has dropped since the last time state
was retrieved, and the BMSR gives us that. Read the BMSR and use it when
deciding the link state. Fill in the an_complete member as well for the
emulated PHY state.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/mv88e6xxx/serdes.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index 47bf87d530b0..d94150d8f3f4 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -53,6 +53,14 @@ static int mv88e6xxx_serdes_pcs_get_state(struct mv88e6xxx_chip *chip,
 					  u16 bmsr, u16 lpa, u16 status,
 					  struct phylink_link_state *state)
 {
+	state->link = false;
+
+	/* If the BMSR reports that the link had failed, report this to
+	 * phylink.
+	 */
+	if (!(bmsr & BMSR_LSTATUS))
+		return 0;
+
 	state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK);
 	state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
 
-- 
2.30.2

Re: [PATCH net 3/3] net: dsa: mv88e6xxx: correctly report serdes link failure

[Andrew Lunn]

On Wed, Apr 13, 2022 at 05:54:02PM +0100, Russell King (Oracle) wrote:
> Phylink wants to know if the link has dropped since the last time state
> was retrieved, and the BMSR gives us that. Read the BMSR and use it when
> deciding the link state. Fill in the an_complete member as well for the
> emulated PHY state.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net 0/3] mv88e6xxx: fixes for reading serdes state

[Russell King (Oracle)]

Hi,

These are some low-priority fixes to the mv88e6xxx serdes code.
Patch 1 fixes the reporting of an_complete, which is used in the
emulation of a conventional C22 PHY. Patch from Marek.

Patch 2 makes one of the error messages in patch 2 to be consistent
with the other error messages in this function.

Patch 3 ensures that we do not miss a link-failure event.

 drivers/net/dsa/mv88e6xxx/serdes.c | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

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

[PATCH net 1/3] net: dsa: mv88e6xxx: use BMSR_ANEGCOMPLETE bit for filling an_complete

[Russell King]

From: =?UTF-8?q?Marek=20Beh=C3=BAn?= <kabel@kernel.org>

Commit ede359d8843a ("net: dsa: mv88e6xxx: Link in pcs_get_state() if AN
is bypassed") added the ability to link if AN was bypassed, and added
filling of state->an_complete field, but set it to true if AN was
enabled in BMCR, not when AN was reported complete in BMSR.

This was done because for some reason, when I wanted to use BMSR value
to infer an_complete, I was looking at BMSR_ANEGCAPABLE bit (which was
always 1), instead of BMSR_ANEGCOMPLETE bit.

Use BMSR_ANEGCOMPLETE for filling state->an_complete.

Fixes: ede359d8843a ("net: dsa: mv88e6xxx: Link in pcs_get_state() if AN is bypassed")
Signed-off-by: Marek Behún <kabel@kernel.org>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/mv88e6xxx/serdes.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index 7b37d45bc9fb..1a19c5284f2c 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -50,22 +50,17 @@ static int mv88e6390_serdes_write(struct mv88e6xxx_chip *chip,
 }
 
 static int mv88e6xxx_serdes_pcs_get_state(struct mv88e6xxx_chip *chip,
-					  u16 ctrl, u16 status, u16 lpa,
+					  u16 bmsr, u16 lpa, u16 status,
 					  struct phylink_link_state *state)
 {
 	state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK);
+	state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
 
 	if (status & MV88E6390_SGMII_PHY_STATUS_SPD_DPL_VALID) {
 		/* The Spped and Duplex Resolved register is 1 if AN is enabled
 		 * and complete, or if AN is disabled. So with disabled AN we
-		 * still get here on link up. But we want to set an_complete
-		 * only if AN was enabled, thus we look at BMCR_ANENABLE.
-		 * (According to 802.3-2008 section 22.2.4.2.10, we should be
-		 *  able to get this same value from BMSR_ANEGCAPABLE, but tests
-		 *  show that these Marvell PHYs don't conform to this part of
-		 *  the specificaion - BMSR_ANEGCAPABLE is simply always 1.)
+		 * still get here on link up.
 		 */
-		state->an_complete = !!(ctrl & BMCR_ANENABLE);
 		state->duplex = status &
 				MV88E6390_SGMII_PHY_STATUS_DUPLEX_FULL ?
 			                         DUPLEX_FULL : DUPLEX_HALF;
@@ -191,12 +186,12 @@ 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, ctrl;
+	u16 bmsr, lpa, status;
 	int err;
 
-	err = mv88e6352_serdes_read(chip, MII_BMCR, &ctrl);
+	err = mv88e6352_serdes_read(chip, MII_BMSR, &bmsr);
 	if (err) {
-		dev_err(chip->dev, "can't read Serdes PHY control: %d\n", err);
+		dev_err(chip->dev, "can't read Serdes BMSR: %d\n", err);
 		return err;
 	}
 
@@ -212,7 +207,7 @@ int mv88e6352_serdes_pcs_get_state(struct mv88e6xxx_chip *chip, int port,
 		return err;
 	}
 
-	return mv88e6xxx_serdes_pcs_get_state(chip, ctrl, status, lpa, state);
+	return mv88e6xxx_serdes_pcs_get_state(chip, bmsr, lpa, status, state);
 }
 
 int mv88e6352_serdes_pcs_an_restart(struct mv88e6xxx_chip *chip, int port,
@@ -918,13 +913,13 @@ 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, ctrl;
+	u16 bmsr, lpa, status;
 	int err;
 
 	err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
-				    MV88E6390_SGMII_BMCR, &ctrl);
+				    MV88E6390_SGMII_BMSR, &bmsr);
 	if (err) {
-		dev_err(chip->dev, "can't read Serdes PHY control: %d\n", err);
+		dev_err(chip->dev, "can't read Serdes PHY BMSR: %d\n", err);
 		return err;
 	}
 
@@ -942,7 +937,7 @@ static int mv88e6390_serdes_pcs_get_state_sgmii(struct mv88e6xxx_chip *chip,
 		return err;
 	}
 
-	return mv88e6xxx_serdes_pcs_get_state(chip, ctrl, status, lpa, state);
+	return mv88e6xxx_serdes_pcs_get_state(chip, bmsr, lpa, status, state);
 }
 
 static int mv88e6390_serdes_pcs_get_state_10g(struct mv88e6xxx_chip *chip,
-- 
2.30.2