Skip to contentSkip to conversation

Fix 88x3310 leaving power save mode

PATCH In review Russell King - ARM Linux admin 7 participants 12 messages Apr 14, 2020 – May 9, 2020

Subsystems (4)
Networking Network drivers Ethernet PHY Marvell MV88X3310 PHY driver
lore
Share & tools
lore

Russell King - ARM Linux admin

author maintainer
#0 lore raw

v2 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 | 36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 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
#1 lore raw

PATCH v2 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 | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
drivers/net/phy/marvell10g.c +20 -0
index 7621badae64d..748532d9e1ae 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,22 @@ 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;
+
+	phydev_info(phydev, "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
#4 lore raw

PATCH v2 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]>
Reviewed-by Andrew Lunn <[email protected]>
Signed-off-by Russell King <[email protected]>
---
 drivers/net/phy/marvell10g.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)
drivers/net/phy/marvell10g.c +13 -3
index 748532d9e1ae..95e3f4644aeb 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,17 @@ 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;
+
+	return phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,
+				MV_V2_PORT_CTRL_SWRST);
 }
 
 static int mv3310_reset(struct phy_device *phydev, u32 unit)
Signature · 1 line
2.20.1
Reviewed-by on v2 2/2
#5 lore raw

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

In reply to Russell King
Quoted 10 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]>
Reviewed-by Andrew Lunn <[email protected]>
Signed-off-by Russell King <[email protected]>
Reviewed-by Florian Fainelli <[email protected]>
Signature · 1 line
Florian
maintainer
#6 lore raw
In reply to Russell King - ARM Linux admin

From: Russell King - ARM Linux admin <[email protected]>
Date: Tue, 14 Apr 2020 20:47:53 +0100

Quoted 8 lines

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.

Quoted 6 lines

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.

Series applied, thanks.

#7 lore raw
Quoted 12 lines · David Miller

From: Russell King - ARM Linux admin <[email protected]>
Date: Tue, 14 Apr 2020 20:47:53 +0100

Quoted 8 lines

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.

Quoted 9 lines
Quoted 6 lines

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.

Series applied, thanks.

Hi,

should we queue this to -stable?
The 10 gbit ports don't work without this fix.

Regards,

Signature · 2 lines
Matteo Croce
per aspera ad upstream
author maintainer
#8 lore raw
Quoted 20 lines · Matteo Croce
Quoted 19 lines · David Miller

From: Russell King - ARM Linux admin <[email protected]>
Date: Tue, 14 Apr 2020 20:47:53 +0100

Quoted 14 lines

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.

Quoted 7 lines
Quoted 2 lines

Series applied, thanks.

Hi,

should we queue this to -stable?
The 10 gbit ports don't work without this fix.

It has a "Fixes:" tag, so it should be backported automatically.

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
#9 lore raw
Quoted 24 lines · Russell King - ARM Linux admin
Quoted 23 lines · Matteo Croce
Quoted 21 lines · David Miller

From: Russell King - ARM Linux admin <[email protected]>
Date: Tue, 14 Apr 2020 20:47:53 +0100

Quoted 8 lines

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.

Quoted 6 lines

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.

Series applied, thanks.

Quoted 6 lines
Quoted 4 lines

Hi,

should we queue this to -stable?
The 10 gbit ports don't work without this fix.

It has a "Fixes:" tag, so it should be backported automatically.

That is a wild guess that it might happen sometime in the future. Please use the cc: stable@ tag as is documented for the past 15+ years instead of relying on us to randomly notice a Fixes: tag.

thanks,

greg k-h

author maintainer
#10 lore raw
Quoted 27 lines · Greg Kroah-Hartman
Quoted 26 lines · Russell King - ARM Linux admin
Quoted 25 lines · Matteo Croce
Quoted 12 lines · David Miller

From: Russell King - ARM Linux admin <[email protected]>
Date: Tue, 14 Apr 2020 20:47:53 +0100

Quoted 8 lines

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.

Quoted 9 lines
Quoted 6 lines

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.

Series applied, thanks.

Hi,

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

should we queue this to -stable?
The 10 gbit ports don't work without this fix.

It has a "Fixes:" tag, so it should be backported automatically.

That is a wild guess that it might happen sometime in the future. Please use the cc: stable@ tag as is documented for the past 15+ years instead of relying on us to randomly notice a Fixes: tag.

Not for netdev material. Netdev has its own rules:

https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt

Q: I see a network patch and I think it should be backported to stable.
various stable releases?

A: Normally Greg Kroah-Hartman collects stable commits himself, but
for networking, Dave collects up patches he deems critical for the networking subsystem, and then hands them off to Greg.
...

Q: I see a network patch and I think it should be backported to stable.
Should I request it via "[email protected]" like the references in the kernel's Documentation/process/stable-kernel-rules.rst file say?

A: No, not for networking. Check the stable queues as per above 1st to see
if it is already queued. If not, then send a mail to netdev, listing the upstream commit ID and why you think it should be a stable candidate.
...

Q: I have created a network patch and I think it should be backported to
stable. Should I add a "Cc: [email protected]" like the references in the kernel's Documentation/ directory say?

A: No. See above answer. In short, if you think it really belongs in
stable, then ensure you write a decent commit log that describes who gets impacted by the bugfix and how it manifests itself, and when the bug was introduced. If you do that properly, then the commit will get handled appropriately and most likely get put in the patchworks stable queue if it really warrants it.
...

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
#11 lore raw
Quoted 33 lines · Russell King - ARM Linux admin
Quoted 32 lines · Greg Kroah-Hartman
Quoted 30 lines · Russell King - ARM Linux admin
Quoted 20 lines · Matteo Croce
Quoted 19 lines · David Miller

From: Russell King - ARM Linux admin <[email protected]>
Date: Tue, 14 Apr 2020 20:47:53 +0100

Quoted 14 lines

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.

Quoted 7 lines
Quoted 2 lines

Series applied, thanks.

Hi,

should we queue this to -stable?
The 10 gbit ports don't work without this fix.

It has a "Fixes:" tag, so it should be backported automatically.

Quoted 7 lines
Quoted 3 lines

That is a wild guess that it might happen sometime in the future. Please use the cc: stable@ tag as is documented for the past 15+ years instead of relying on us to randomly notice a Fixes: tag.

Not for netdev material. Netdev has its own rules:

https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt

<snip>

Sorry, missed that this was netdev, my fault.

greg k-h