diff mbox series

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

Message ID E1negFc-005tSn-BX@rmk-PC.armlinux.org.uk (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net: dsa: mv88e6xxx serdes fixes | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 1 maintainers not CCed: linux@armlinux.org.uk
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 72 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Russell King (Oracle) April 13, 2022, 4:53 p.m. UTC 
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(-)

Comments

Andrew Lunn April 13, 2022, 6:46 p.m. UTC | #1 
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
Russell King (Oracle) April 13, 2022, 7:29 p.m. UTC | #2 
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.
diff mbox series

Patch

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,