[PATCH net-next] net: phy: marvell: Fix 88e1510 downshift counter errata

[PATCH net-next] net: phy: marvell: Fix 88e1510 downshift counter errata

[Rohan G Thomas]

The 88e1510 PHY has an erratum where the phy downshift counter is not
cleared on a link power down/up. This can cause the gigabit link to
intermittently downshift to a lower speed.

Disabling and re-enabling the downshift feature clears the counter,
allowing the PHY to retry gigabit link negotiation up to the programmed
retry count times before downshifting. This behavior has been observed
on copper links.

Signed-off-by: Rohan G Thomas <rohan.g.thomas@altera.com>
Reviewed-by: Matthew Gerlach <matthew.gerlach@altera.com>
---
 drivers/net/phy/marvell.c | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 623292948fa706a2b0d8b98919ead8b609bbd949..4c3d5fbcfda0a960f6c1284f07f16061d9fa0229 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -1902,6 +1902,43 @@ static int marvell_resume(struct phy_device *phydev)
 	return err;
 }
 
+/* m88e1510_resume
+ *
+ * The 88e1510 PHY has an erratum where the phy downshift counter is not cleared
+ * during a link power down/up. This can cause the link to intermittently
+ * downshift to a lower speed.
+ *
+ * Disabling and re-enabling the downshift feature clears the counter, allowing
+ * the PHY to retry gigabit link negotiation up to the programmed retry count
+ * before downshifting. This behavior has been observed on copper links.
+ */
+static int m88e1510_resume(struct phy_device *phydev)
+{
+	int err;
+	u8 cnt = 0;
+
+	err = marvell_resume(phydev);
+	if (err < 0)
+		return err;
+
+	/* read downshift counter value */
+	err = m88e1011_get_downshift(phydev, &cnt);
+	if (err < 0)
+		return err;
+
+	if (cnt) {
+		/* downshift disabled */
+		err = m88e1011_set_downshift(phydev, 0);
+		if (err < 0)
+			return err;
+
+		/* downshift enabled, with previous counter value */
+		err = m88e1011_set_downshift(phydev, cnt);
+	}
+
+	return err;
+}
+
 static int marvell_aneg_done(struct phy_device *phydev)
 {
 	int retval = phy_read(phydev, MII_M1011_PHY_STATUS);
@@ -3923,7 +3960,7 @@ static struct phy_driver marvell_drivers[] = {
 		.handle_interrupt = marvell_handle_interrupt,
 		.get_wol = m88e1318_get_wol,
 		.set_wol = m88e1318_set_wol,
-		.resume = marvell_resume,
+		.resume = m88e1510_resume,
 		.suspend = marvell_suspend,
 		.read_page = marvell_read_page,
 		.write_page = marvell_write_page,

---
base-commit: 2fd4161d0d2547650d9559d57fc67b4e0a26a9e3
change-id: 20250902-marvell_fix-3f2a3d5e0fca

Best regards,
-- 
Rohan G Thomas <rohan.g.thomas@altera.com>

[PATCH net-next] net: phy: marvell: Fix 88e1510 downshift counter errata

[Rohan G Thomas via B4 Relay]

From: Rohan G Thomas <rohan.g.thomas@altera.com>

The 88e1510 PHY has an erratum where the phy downshift counter is not
cleared on a link power down/up. This can cause the gigabit link to
intermittently downshift to a lower speed.

Disabling and re-enabling the downshift feature clears the counter,
allowing the PHY to retry gigabit link negotiation up to the programmed
retry count times before downshifting. This behavior has been observed
on copper links.

Signed-off-by: Rohan G Thomas <rohan.g.thomas@altera.com>
Reviewed-by: Matthew Gerlach <matthew.gerlach@altera.com>
---
 drivers/net/phy/marvell.c | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 623292948fa706a2b0d8b98919ead8b609bbd949..4c3d5fbcfda0a960f6c1284f07f16061d9fa0229 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -1902,6 +1902,43 @@ static int marvell_resume(struct phy_device *phydev)
 	return err;
 }
 
+/* m88e1510_resume
+ *
+ * The 88e1510 PHY has an erratum where the phy downshift counter is not cleared
+ * during a link power down/up. This can cause the link to intermittently
+ * downshift to a lower speed.
+ *
+ * Disabling and re-enabling the downshift feature clears the counter, allowing
+ * the PHY to retry gigabit link negotiation up to the programmed retry count
+ * before downshifting. This behavior has been observed on copper links.
+ */
+static int m88e1510_resume(struct phy_device *phydev)
+{
+	int err;
+	u8 cnt = 0;
+
+	err = marvell_resume(phydev);
+	if (err < 0)
+		return err;
+
+	/* read downshift counter value */
+	err = m88e1011_get_downshift(phydev, &cnt);
+	if (err < 0)
+		return err;
+
+	if (cnt) {
+		/* downshift disabled */
+		err = m88e1011_set_downshift(phydev, 0);
+		if (err < 0)
+			return err;
+
+		/* downshift enabled, with previous counter value */
+		err = m88e1011_set_downshift(phydev, cnt);
+	}
+
+	return err;
+}
+
 static int marvell_aneg_done(struct phy_device *phydev)
 {
 	int retval = phy_read(phydev, MII_M1011_PHY_STATUS);
@@ -3923,7 +3960,7 @@ static struct phy_driver marvell_drivers[] = {
 		.handle_interrupt = marvell_handle_interrupt,
 		.get_wol = m88e1318_get_wol,
 		.set_wol = m88e1318_set_wol,
-		.resume = marvell_resume,
+		.resume = m88e1510_resume,
 		.suspend = marvell_suspend,
 		.read_page = marvell_read_page,
 		.write_page = marvell_write_page,

---
base-commit: 2fd4161d0d2547650d9559d57fc67b4e0a26a9e3
change-id: 20250902-marvell_fix-3f2a3d5e0fca

Best regards,
-- 
Rohan G Thomas <rohan.g.thomas@altera.com>

Re: [PATCH net-next] net: phy: marvell: Fix 88e1510 downshift counter errata

[Andrew Lunn]

On Tue, Sep 02, 2025 at 01:59:57PM +0800, Rohan G Thomas via B4 Relay wrote:
> From: Rohan G Thomas <rohan.g.thomas@altera.com>
> 
> The 88e1510 PHY has an erratum where the phy downshift counter is not
> cleared on a link power down/up. This can cause the gigabit link to
> intermittently downshift to a lower speed.
> 
> Disabling and re-enabling the downshift feature clears the counter,
> allowing the PHY to retry gigabit link negotiation up to the programmed
> retry count times before downshifting. This behavior has been observed
> on copper links.
> 
> Signed-off-by: Rohan G Thomas <rohan.g.thomas@altera.com>
> Reviewed-by: Matthew Gerlach <matthew.gerlach@altera.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next] net: phy: marvell: Fix 88e1510 downshift counter errata

[Russell King (Oracle)]

On Tue, Sep 02, 2025 at 01:59:57PM +0800, Rohan G Thomas via B4 Relay wrote:
> From: Rohan G Thomas <rohan.g.thomas@altera.com>
> 
> The 88e1510 PHY has an erratum where the phy downshift counter is not
> cleared on a link power down/up. This can cause the gigabit link to
> intermittently downshift to a lower speed.

Does this apply to all 88e1510 PHYs or just some revisions?

Also, what is a "link power down/up" ? Are you referring to setting
BMCR_PDOWN and then clearing it? (please update the commit description
and repost after 24 hours, thanks.)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next] net: phy: marvell: Fix 88e1510 downshift counter errata

[G Thomas, Rohan]

Hi Russell King,

Thanks for reviewing the patch.

On 9/2/2025 7:05 PM, Russell King (Oracle) wrote:
> On Tue, Sep 02, 2025 at 01:59:57PM +0800, Rohan G Thomas via B4 Relay wrote:
>> From: Rohan G Thomas <rohan.g.thomas@altera.com>
>>
>> The 88e1510 PHY has an erratum where the phy downshift counter is not
>> cleared on a link power down/up. This can cause the gigabit link to
>> intermittently downshift to a lower speed.
> 
> Does this apply to all 88e1510 PHYs or just some revisions?

I'm not entirely sure on this. But, based on 88E151x A0 "Errata and
Hardware Release Notes" from Marvell, no revision plans or fixes
mentioned for this issue but only the workaround is suggested for the
downshift feature. So, I think it is applicable for all 88e151x PHY
revisions.

> 
> Also, what is a "link power down/up" ? Are you referring to setting
> BMCR_PDOWN and then clearing it? (please update the commit description
> and repost after 24 hours, thanks.)
> 

Yes, I'm referring to setting and the clearing BMCR_PDOWN. Will update
the commit description in the next version.

Best Regards,
Rohan

Re: [PATCH net-next] net: phy: marvell: Fix 88e1510 downshift counter errata

[Andrew Lunn]

> > Also, what is a "link power down/up" ? Are you referring to setting
> > BMCR_PDOWN and then clearing it? (please update the commit description
> > and repost after 24 hours, thanks.)
> > 
> 
> Yes, I'm referring to setting and the clearing BMCR_PDOWN. Will update
> the commit description in the next version.

So it could be the bootloader left the PHY in powered down state, when
booting.

There is some not so nice logic in phy_attach_direct() which calls
phy_resume() when the MAC attaches to the PHY. So this case is covered
by that. But it would be good to comment in the commit message, and
maybe the resume function, that you are relying on this behaviour.

I do wounder if it should be more explicit, m88e1510_config_init()
also calls the workaround? You would then not need the comment.

	Andrew

Re: [PATCH net-next] net: phy: marvell: Fix 88e1510 downshift counter errata

[G Thomas, Rohan]

Hi Andrew,

Thanks for the review comments.

On 9/2/2025 8:38 PM, Andrew Lunn wrote:
>>> Also, what is a "link power down/up" ? Are you referring to setting
>>> BMCR_PDOWN and then clearing it? (please update the commit description
>>> and repost after 24 hours, thanks.)
>>>
>>
>> Yes, I'm referring to setting and the clearing BMCR_PDOWN. Will update
>> the commit description in the next version.
> 
> So it could be the bootloader left the PHY in powered down state, when
> booting.
> 

It is not just during initial boot, but whenever the PHY is resumed by
clearing BMCR_PDOWN and goes for autoneg it is recommended to clear the
downshift counter by disabling and re-enabling the downshift feature.

> There is some not so nice logic in phy_attach_direct() which calls
> phy_resume() when the MAC attaches to the PHY. So this case is covered
> by that. But it would be good to comment in the commit message, and
> maybe the resume function, that you are relying on this behaviour.
> 

For m88e1510_resume(), description is added. Will update the commit
message and function description in the next version.

> I do wounder if it should be more explicit, m88e1510_config_init()
> also calls the workaround? You would then not need the comment.
> 
> 	Andrew

Best Regards,
Rohan