Fix 88x3310 leaving power save mode

PATCH Superseded Russell King - ARM Linux admin 3 participants 6 messages Apr 14, 2020

Subsystems (4)

Networking Network drivers Ethernet PHY Marvell MV88X3310 PHY driver

lore

Share & tools

lore

Russell King - ARM Linux admin

author maintainer

2020-04-14 18:29 UTC #0 lore raw

cover Fix 88x3310 leaving power save mode

Hi,

This series fixes a problem with the 88x3310 PHY on Macchiatobin coming out of powersave mode noticed by Matteo Croce. It seems that certain PHY firmwares do not properly exit powersave mode, resulting in a fibre link not coming up.

The solution appears to be to soft-reset the PHY after clearing the powersave bit.

We add support for reporting the PHY firmware version to the kernel log, and use it to trigger this new behaviour if we have v0.3.x.x or more recent firmware on the PHY. This, however, is a guess as the firmware revision documentation does not mention this issue, and we know that v0.2.1.0 works without this fix but v0.3.3.0 and later does not.

 drivers/net/phy/marvell10g.c | 39 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)

Signature · 2 lines

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Russell King

2020-04-14 18:30 UTC #1 lore raw

PATCH 1/2 net: marvell10g: report firmware version

Report the firmware version when probing the PHY to allow issues attributable to firmware to be diagnosed.

Tested-by Matteo Croce <[email protected]>

Signed-off-by Russell King <[email protected]>

---
 drivers/net/phy/marvell10g.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

drivers/net/phy/marvell10g.c +21 -0

index 7621badae64d..ee60417cdc55 100644
@@ -33,6 +33,8 @@
 #define MV_PHY_ALASKA_NBT_QUIRK_REV	(MARVELL_PHY_ID_88X3310 | 0xa)
 
 enum {
+	MV_PMA_FW_VER0		= 0xc011,
+	MV_PMA_FW_VER1		= 0xc012,
 	MV_PMA_BOOT		= 0xc050,
 	MV_PMA_BOOT_FATAL	= BIT(0),

@@ -83,6 +85,8 @@ enum {
 };
 
 struct mv3310_priv {
+	u32 firmware_ver;
+
 	struct device *hwmon_dev;
 	char *hwmon_name;
 };

@@ -355,6 +359,23 @@ static int mv3310_probe(struct phy_device *phydev)
 
 	dev_set_drvdata(&phydev->mdio.dev, priv);
 
+	ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_FW_VER0);
+	if (ret < 0)
+		return ret;
+
+	priv->firmware_ver = ret << 16;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_FW_VER1);
+	if (ret < 0)
+		return ret;
+
+	priv->firmware_ver |= ret;
+
+	dev_info(&phydev->mdio.dev,
+		 "Firmware version %u.%u.%u.%u\n",
+		 priv->firmware_ver >> 24, (priv->firmware_ver >> 16) & 255,
+		 (priv->firmware_ver >> 8) & 255, priv->firmware_ver & 255);
+
 	/* Powering down the port when not in use saves about 600mW */
 	ret = mv3310_power_down(phydev);
 	if (ret)

Signature · 1 line

2.20.1

Russell King

2020-04-14 18:30 UTC #4 lore raw

PATCH 2/2 net: marvell10g: soft-reset the PHY when coming out of low power

In reply to Russell King - ARM Linux admin

Soft-reset the PHY when coming out of low power mode, which seems to be necessary with firmware versions 0.3.3.0 and 0.3.10.0.

This depends on ("net: marvell10g: report firmware version")

Fixes c9cc1c815d36 ("net: phy: marvell10g: place in powersave mode at probe")

Reported-by Matteo Croce <[email protected]>

Tested-by Matteo Croce <[email protected]>

Signed-off-by Russell King <[email protected]>

---
 drivers/net/phy/marvell10g.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

drivers/net/phy/marvell10g.c +15 -3

index ee60417cdc55..5865b563cc94 100644
@@ -75,7 +75,8 @@ enum {
 
 	/* Vendor2 MMD registers */
 	MV_V2_PORT_CTRL		= 0xf001,
-	MV_V2_PORT_CTRL_PWRDOWN = 0x0800,
+	MV_V2_PORT_CTRL_SWRST	= BIT(15),
+	MV_V2_PORT_CTRL_PWRDOWN = BIT(11),
 	MV_V2_TEMP_CTRL		= 0xf08a,
 	MV_V2_TEMP_CTRL_MASK	= 0xc000,
 	MV_V2_TEMP_CTRL_SAMPLE	= 0x0000,

@@ -239,8 +240,19 @@ static int mv3310_power_down(struct phy_device *phydev)
 
 static int mv3310_power_up(struct phy_device *phydev)
 {
-	return phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,
-				  MV_V2_PORT_CTRL_PWRDOWN);
+	struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev);
+	int ret;
+
+	ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,
+				 MV_V2_PORT_CTRL_PWRDOWN);
+
+	if (priv->firmware_ver < 0x00030000)
+		return ret;
+
+	ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,
+			       MV_V2_PORT_CTRL_SWRST);
+
+	return ret;
 }
 
 static int mv3310_reset(struct phy_device *phydev, u32 unit)

Signature · 1 line

2.20.1

Andrew Lunn

maintainer

2020-04-14 18:47 UTC #2 lore raw

Re: [PATCH net 1/2] net: marvell10g: report firmware version

In reply to Russell King

Quoted 42 lines · Russell King

Report the firmware version when probing the PHY to allow issues attributable to firmware to be diagnosed.

Tested-by Matteo Croce <[email protected]>

Signed-off-by Russell King <[email protected]>

---
 drivers/net/phy/marvell10g.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

drivers/net/phy/marvell10g.c +11 -0

index 7621badae64d..ee60417cdc55 100644
@@ -33,6 +33,8 @@
 #define MV_PHY_ALASKA_NBT_QUIRK_REV	(MARVELL_PHY_ID_88X3310 | 0xa)
 
 enum {
+	MV_PMA_FW_VER0		= 0xc011,
+	MV_PMA_FW_VER1		= 0xc012,
 	MV_PMA_BOOT		= 0xc050,
 	MV_PMA_BOOT_FATAL	= BIT(0),

@@ -83,6 +85,8 @@ enum {
 };
 
 struct mv3310_priv {
+	u32 firmware_ver;
+
 	struct device *hwmon_dev;
 	char *hwmon_name;
 };

@@ -355,6 +359,23 @@ static int mv3310_probe(struct phy_device *phydev)
 
 	dev_set_drvdata(&phydev->mdio.dev, priv);
 
+	ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_FW_VER0);
+	if (ret < 0)
+		return ret;
+
+	priv->firmware_ver = ret << 16;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_FW_VER1);

drivers/net/phy/marvell10g.c static int mv3310_probe(struct phy_device *phydev) · line 378

Quoted 10 lines

drivers/net/phy/marvell10g.c +10 -0

+	if (ret < 0)
+		return ret;
+
+	priv->firmware_ver |= ret;
+
+	dev_info(&phydev->mdio.dev,
+		 "Firmware version %u.%u.%u.%u\n",
+		 priv->firmware_ver >> 24, (priv->firmware_ver >> 16) & 255,
+		 (priv->firmware_ver >> 8) & 255, priv->firmware_ver & 255);
+

Hi Russell

Did you consider using phydev_info()? Or is it too early, and you don't get a sensible name?

Andrew

Andrew Lunn

maintainer Reviewed-by on v1 2/2

2020-04-14 18:49 UTC #5 lore raw

Re: [PATCH net 2/2] net: marvell10g: soft-reset the PHY when coming out of low power

In reply to Russell King

Quoted 9 lines · Russell King

Soft-reset the PHY when coming out of low power mode, which seems to be necessary with firmware versions 0.3.3.0 and 0.3.10.0.
This depends on ("net: marvell10g: report firmware version")
Fixes c9cc1c815d36 ("net: phy: marvell10g: place in powersave mode at probe")
Reported-by Matteo Croce <[email protected]>
Tested-by Matteo Croce <[email protected]>
Signed-off-by Russell King <[email protected]>

Reviewed-by Andrew Lunn <[email protected]>

Andrew

Russell King - ARM Linux admin

author maintainer

2020-04-14 19:48 UTC #3 lore raw

Re: [PATCH net 1/2] net: marvell10g: report firmware version

In reply to Andrew Lunn

Quoted 56 lines · Andrew Lunn

Quoted 42 lines · Russell King

Report the firmware version when probing the PHY to allow issues attributable to firmware to be diagnosed.

Tested-by Matteo Croce <[email protected]>

Signed-off-by Russell King <[email protected]>

---
 drivers/net/phy/marvell10g.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

drivers/net/phy/marvell10g.c +11 -0

index 7621badae64d..ee60417cdc55 100644
@@ -33,6 +33,8 @@
 #define MV_PHY_ALASKA_NBT_QUIRK_REV	(MARVELL_PHY_ID_88X3310 | 0xa)
 
 enum {
+	MV_PMA_FW_VER0		= 0xc011,
+	MV_PMA_FW_VER1		= 0xc012,
 	MV_PMA_BOOT		= 0xc050,
 	MV_PMA_BOOT_FATAL	= BIT(0),

@@ -83,6 +85,8 @@ enum {
 };
 
 struct mv3310_priv {
+	u32 firmware_ver;
+
 	struct device *hwmon_dev;
 	char *hwmon_name;
 };

@@ -355,6 +359,23 @@ static int mv3310_probe(struct phy_device *phydev)
 
 	dev_set_drvdata(&phydev->mdio.dev, priv);
 
+	ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_FW_VER0);
+	if (ret < 0)
+		return ret;
+
+	priv->firmware_ver = ret << 16;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_FW_VER1);

drivers/net/phy/marvell10g.c static int mv3310_probe(struct phy_device *phydev) · line 378

Quoted 10 lines

drivers/net/phy/marvell10g.c +10 -0

+	if (ret < 0)
+		return ret;
+
+	priv->firmware_ver |= ret;
+
+	dev_info(&phydev->mdio.dev,
+		 "Firmware version %u.%u.%u.%u\n",
+		 priv->firmware_ver >> 24, (priv->firmware_ver >> 16) & 255,
+		 (priv->firmware_ver >> 8) & 255, priv->firmware_ver & 255);
+

Hi Russell

Quoted 2 lines

Did you consider using phydev_info()? Or is it too early, and you don't get a sensible name?

No, I keep forgetting those exist! I'll resend shortly.

Signature · 2 lines

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up