Skip to contentSkip to conversation

phy: keep the BCMR_LOOPBACK bit while setup forced mode

PATCH RESEND In review Weidong Wang 4 participants 7 messages Apr 13, 2016 – Apr 16, 2016

Subsystems (3)
Networking Network drivers Ethernet PHY
lore
Share & tools
lore

Weidong Wang

author
#0 lore raw

PATCH RESEND 1/1 phy: keep the BCMR_LOOPBACK bit while setup forced mode

When tested the PHY SGMII Loopback,:
1.set the LOOPBACK bit,
2.set the autoneg to AUTONEG_DISABLE, it calls the genphy_setup_forced which will clear the bit.

So just keep the LOOPBACK bit while setup forced mode.

Signed-off-by Weidong Wang <[email protected]>
---
 drivers/net/phy/phy_device.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/net/phy/phy_device.c +2 -0
index e551f3a..8da4b80 100644
@@ -1124,7 +1124,9 @@ static int genphy_config_advert(struct phy_device *phydev)
 int genphy_setup_forced(struct phy_device *phydev)
 {
 	int ctl = 0;
+	int val = phy_read(phydev, MII_BMCR);
 
+	ctl |= val & BMCR_LOOPBACK;
 	phydev->pause = 0;
 	phydev->asym_pause = 0;

-- 2.7.0

#1 lore raw

Hello.

Quoted 17 lines · Weidong Wang

When tested the PHY SGMII Loopback,:
1.set the LOOPBACK bit,
2.set the autoneg to AUTONEG_DISABLE, it calls the genphy_setup_forced which will clear the bit.

So just keep the LOOPBACK bit while setup forced mode.

Signed-off-by Weidong Wang <[email protected]>
---
  drivers/net/phy/phy_device.c | 2 ++
  1 file changed, 2 insertions(+)
drivers/net/phy/phy_device.c +0 -0
index e551f3a..8da4b80 100644

drivers/net/phy/phy_device.c static int genphy_config_advert(struct phy_device *phydev) · line 1127

Quoted 4 lines
drivers/net/phy/phy_device.c +1 -0
@@ -1124,7 +1124,9 @@ static int genphy_config_advert(struct phy_device *phydev)
  int genphy_setup_forced(struct phy_device *phydev)
  {
  	int ctl = 0;
+	int val = phy_read(phydev, MII_BMCR);

Please place this declaration first, DaveM prefers declarations to be
sorted from longest to shortest.

drivers/net/phy/phy_device.c static int genphy_config_advert(struct phy_device *phydev) · line 1129

Quoted 2 lines
drivers/net/phy/phy_device.c +1 -0
+	ctl |= val & BMCR_LOOPBACK;

Just =, removing the 'ctl' initializer, please.

[...]

MBR, Sergei

author
#2 lore raw
Quoted 29 lines · Sergei Shtylyov

Hello.

Quoted 17 lines · Weidong Wang

When tested the PHY SGMII Loopback,:
1.set the LOOPBACK bit,
2.set the autoneg to AUTONEG_DISABLE, it calls the genphy_setup_forced which will clear the bit.

So just keep the LOOPBACK bit while setup forced mode.

Signed-off-by Weidong Wang <[email protected]>
---
  drivers/net/phy/phy_device.c | 2 ++
  1 file changed, 2 insertions(+)
drivers/net/phy/phy_device.c +0 -0
index e551f3a..8da4b80 100644

drivers/net/phy/phy_device.c static int genphy_config_advert(struct phy_device *phydev) · line 1127

Quoted 4 lines
drivers/net/phy/phy_device.c +1 -0
@@ -1124,7 +1124,9 @@ static int genphy_config_advert(struct phy_device *phydev)
  int genphy_setup_forced(struct phy_device *phydev)
  {
      int ctl = 0;
+    int val = phy_read(phydev, MII_BMCR);

Please place this declaration first, DaveM prefers declarations to be sorted from longest to shortest.

Quoted 1 line
Quoted 8 lines
Quoted 1 line

+ ctl |= val & BMCR_LOOPBACK;

Just =, removing the 'ctl' initializer, please.

[...]

MBR, Sergei

Got it.

Regards,
Weidong

Quoted 2 lines
#3 lore raw
In reply to Weidong Wang
Quoted 6 lines · Weidong Wang

When tested the PHY SGMII Loopback,:
1.set the LOOPBACK bit,
2.set the autoneg to AUTONEG_DISABLE, it calls the genphy_setup_forced which will clear the bit.

So just keep the LOOPBACK bit while setup forced mode.

Humm, it makes sense why we want this one, but maybe we want other bits to be preserved too, like MII_ISOLATE for instance?

Or maybe we should have a separate way to put the PHY into loopback mode which is deterministic and takes care of forcing the link at the same time?

Quoted 22 lines
Signed-off-by Weidong Wang <[email protected]>
---
 drivers/net/phy/phy_device.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/net/phy/phy_device.c +2 -1
index e551f3a..8da4b80 100644
@@ -1124,7 +1124,9 @@ static int genphy_config_advert(struct phy_device *phydev)
 int genphy_setup_forced(struct phy_device *phydev)
 {
 	int ctl = 0;
+	int val = phy_read(phydev, MII_BMCR);
 
+	ctl |= val & BMCR_LOOPBACK;
 	phydev->pause = 0;
 	phydev->asym_pause = 0;
 
-- 2.7.0
Signature · 1 line
Florian
author
#4 lore raw
Quoted 11 lines · Florian Fainelli
Quoted 6 lines · Weidong Wang

When tested the PHY SGMII Loopback,:
1.set the LOOPBACK bit,
2.set the autoneg to AUTONEG_DISABLE, it calls the genphy_setup_forced which will clear the bit.

So just keep the LOOPBACK bit while setup forced mode.

Humm, it makes sense why we want this one, but maybe we want other bits to be preserved too, like MII_ISOLATE for instance?

I will add the MII_ISOLATE as well.

Quoted 3 lines

Or maybe we should have a separate way to put the PHY into loopback mode which is deterministic and takes care of forcing the link at the same time?

I test with the loopback mode, and the link status is undeterminable.

Quoted 24 lines
Quoted 22 lines
Signed-off-by Weidong Wang <[email protected]>
---
 drivers/net/phy/phy_device.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/net/phy/phy_device.c +2 -1
index e551f3a..8da4b80 100644
@@ -1124,7 +1124,9 @@ static int genphy_config_advert(struct phy_device *phydev)
 int genphy_setup_forced(struct phy_device *phydev)
 {
 	int ctl = 0;
+	int val = phy_read(phydev, MII_BMCR);
 
+	ctl |= val & BMCR_LOOPBACK;
 	phydev->pause = 0;
 	phydev->asym_pause = 0;
 
-- 2.7.0
author
#5 lore raw

PATCH 1/1 phy: make some bits preserved while setup forced mode

In reply to Weidong Wang

When tested the PHY SGMII Loopback:
1.set the LOOPBACK bit,
2.set the autoneg to AUTONEG_DISABLE, it calls the genphy_setup_forced which will clear the bit.

The BMCR_LOOPBACK bit should be preserved.

As Florian pointed out that other bits should be preserved too. So I make the BMCR_ISOLATE and BMCR_PDOWN as well.

Signed-off-by Weidong Wang <[email protected]>

---
the patch is evoluted from "phy: keep the BCMR_LOOPBACK bit while setup forced mode".

---
 drivers/net/phy/phy_device.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/net/phy/phy_device.c +2 -1
index d551df6..9b24d7e 100644
@@ -818,8 +818,9 @@ static int genphy_config_advert(struct phy_device *phydev)
  */
 int genphy_setup_forced(struct phy_device *phydev)
 {
-	int ctl = 0;
+	int ctl = phy_read(phydev, MII_BMCR);
 
+	ctl &= BMCR_LOOPBACK | BMCR_ISOLATE | BMCR_PDOWN;
 	phydev->pause = 0;
 	phydev->asym_pause = 0;
Signature · 1 line
2.7.0
maintainer Applied
#6 lore raw

Re: [PATCH net-next] phy: make some bits preserved while setup forced mode

From: Weidong Wang <[email protected]>
Date: Thu, 14 Apr 2016 15:43:52 +0800

Quoted 11 lines

When tested the PHY SGMII Loopback:
1.set the LOOPBACK bit,
2.set the autoneg to AUTONEG_DISABLE, it calls the genphy_setup_forced which will clear the bit.

The BMCR_LOOPBACK bit should be preserved.

As Florian pointed out that other bits should be preserved too. So I make the BMCR_ISOLATE and BMCR_PDOWN as well.

Signed-off-by Weidong Wang <[email protected]>

Applied, thank you.