diff mbox series

[v3,net-next,1/6] net: dsa: felix: clarify the intention of writes to MII_BMCR

Message ID 20200705161626.3797968-2-olteanv@gmail.com
State Accepted
Delegated to: David Miller
Headers show
Series Phylink integration improvements for Felix DSA driver | expand

Commit Message

Vladimir Oltean July 5, 2020, 4:16 p.m. UTC 
From: Vladimir Oltean <vladimir.oltean@nxp.com>

The driver appears to write to BMCR_SPEED and BMCR_DUPLEX, fields which
are read-only, since they are actually configured through the
vendor-specific IF_MODE (0x14) register.

But the reason we're writing back the read-only values of MII_BMCR is to
alter these writable fields:

BMCR_RESET
BMCR_LOOPBACK
BMCR_ANENABLE
BMCR_PDOWN
BMCR_ISOLATE
BMCR_ANRESTART

In particular, the only field which is really relevant to this driver is
BMCR_ANENABLE. Clarify that intention by spelling it out, using
phy_set_bits and phy_clear_bits.

The driver also made a few writes to BMCR_RESET and BMCR_ANRESTART which
are unnecessary and may temporarily disrupt the link to the PHY. Remove
them.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Changes in v3:
None.

Changes in v2:
Patch is new.

 drivers/net/dsa/ocelot/felix_vsc9959.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

Comments

Florian Fainelli July 5, 2020, 9:11 p.m. UTC | #1 
On 7/5/2020 9:16 AM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> The driver appears to write to BMCR_SPEED and BMCR_DUPLEX, fields which
> are read-only, since they are actually configured through the
> vendor-specific IF_MODE (0x14) register.
> 
> But the reason we're writing back the read-only values of MII_BMCR is to
> alter these writable fields:
> 
> BMCR_RESET
> BMCR_LOOPBACK
> BMCR_ANENABLE
> BMCR_PDOWN
> BMCR_ISOLATE
> BMCR_ANRESTART
> 
> In particular, the only field which is really relevant to this driver is
> BMCR_ANENABLE. Clarify that intention by spelling it out, using
> phy_set_bits and phy_clear_bits.
> 
> The driver also made a few writes to BMCR_RESET and BMCR_ANRESTART which
> are unnecessary and may temporarily disrupt the link to the PHY. Remove
> them.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
diff mbox series

Patch

diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 2067776773f7..9f4c8343652f 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -815,7 +815,7 @@  static void vsc9959_pcs_init_sgmii(struct phy_device *pcs,
 		phy_write(pcs, ENETC_PCS_LINK_TIMER2,
 			  ENETC_PCS_LINK_TIMER2_VAL);
 
-		phy_write(pcs, MII_BMCR, BMCR_ANRESTART | BMCR_ANENABLE);
+		phy_set_bits(pcs, MII_BMCR, BMCR_ANENABLE);
 	} else {
 		int speed;
 
@@ -845,10 +845,7 @@  static void vsc9959_pcs_init_sgmii(struct phy_device *pcs,
 			  ENETC_PCS_IF_MODE_SGMII_EN |
 			  ENETC_PCS_IF_MODE_SGMII_SPEED(speed));
 
-		/* Yes, not a mistake: speed is given by IF_MODE. */
-		phy_write(pcs, MII_BMCR, BMCR_RESET |
-					 BMCR_SPEED1000 |
-					 BMCR_FULLDPLX);
+		phy_clear_bits(pcs, MII_BMCR, BMCR_ANENABLE);
 	}
 }
 
@@ -882,9 +879,7 @@  static void vsc9959_pcs_init_2500basex(struct phy_device *pcs,
 		  ENETC_PCS_IF_MODE_SGMII_EN |
 		  ENETC_PCS_IF_MODE_SGMII_SPEED(ENETC_PCS_SPEED_2500));
 
-	phy_write(pcs, MII_BMCR, BMCR_SPEED1000 |
-				 BMCR_FULLDPLX |
-				 BMCR_RESET);
+	phy_clear_bits(pcs, MII_BMCR, BMCR_ANENABLE);
 }
 
 static void vsc9959_pcs_init_usxgmii(struct phy_device *pcs,