[PATCH] net: phy: marvell10g: report if the PHY fails to boot firmware

[PATCH] net: phy: marvell10g: report if the PHY fails to boot firmware

[Russell King]

Some boards do not have the PHY firmware programmed in the 3310's flash,
which leads to the PHY not working as expected.  Warn the user when the
PHY fails to boot the firmware and refuse to initialise.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
I think this patch needs testing with the Marvell 88x2110 PHY before
this can be merged into mainline, but I think it should go into -rc
and be back-ported to stable trees to avoid user frustration. I spent
some time last night debugging one such instance, and the user
afterwards indicated that they'd had the problem for a long time, and
had thought of throwing the hardware out the window!  Clearly not a
good user experience.

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

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 100b401b1f4a..754cde873dde 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -31,6 +31,9 @@
 #define MV_PHY_ALASKA_NBT_QUIRK_REV	(MARVELL_PHY_ID_88X3310 | 0xa)
 
 enum {
+	MV_PMA_BOOT		= 0xc050,
+	MV_PMA_BOOT_FATAL	= BIT(0),
+
 	MV_PCS_BASE_T		= 0x0000,
 	MV_PCS_BASE_R		= 0x1000,
 	MV_PCS_1000BASEX	= 0x2000,
@@ -211,6 +214,16 @@ static int mv3310_probe(struct phy_device *phydev)
 	    (phydev->c45_ids.devices_in_package & mmd_mask) != mmd_mask)
 		return -ENODEV;
 
+	ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT);
+	if (ret < 0)
+		return ret;
+
+	if (ret & MV_PMA_BOOT_FATAL) {
+		dev_warn(&phydev->mdio.dev,
+			 "PHY failed to boot firmware, status=%04x\n", ret);
+		return -ENODEV;
+	}
+
 	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
-- 
2.7.4

Re: [PATCH] net: phy: marvell10g: report if the PHY fails to boot firmware

[Maxime Chevallier]

Hello Russell,

On Tue, 28 May 2019 10:34:42 +0100
Russell King <rmk+kernel@armlinux.org.uk> wrote:

>Some boards do not have the PHY firmware programmed in the 3310's flash,
>which leads to the PHY not working as expected.  Warn the user when the
>PHY fails to boot the firmware and refuse to initialise.
>
>Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
>---
>I think this patch needs testing with the Marvell 88x2110 PHY before
>this can be merged into mainline, but I think it should go into -rc
>and be back-ported to stable trees to avoid user frustration

I've tested this on a 88x2110 and a 88e2010, who both have the same
register at the same location, at are potentially subject to the same
issue.

Tested-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Thanks,

Maxime

Re: [PATCH] net: phy: marvell10g: report if the PHY fails to boot firmware

[Andrew Lunn]

On Tue, May 28, 2019 at 10:34:42AM +0100, Russell King wrote:
> Some boards do not have the PHY firmware programmed in the 3310's flash,
> which leads to the PHY not working as expected.  Warn the user when the
> PHY fails to boot the firmware and refuse to initialise.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Fixes: 20b2af32ff3f ("net: phy: add Marvell Alaska X 88X3310 10Gigabit PHY support")
Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH] net: phy: marvell10g: report if the PHY fails to boot firmware

[Russell King - ARM Linux admin]

On Tue, May 28, 2019 at 10:34:42AM +0100, Russell King wrote:
> Some boards do not have the PHY firmware programmed in the 3310's flash,
> which leads to the PHY not working as expected.  Warn the user when the
> PHY fails to boot the firmware and refuse to initialise.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> I think this patch needs testing with the Marvell 88x2110 PHY before
> this can be merged into mainline, but I think it should go into -rc
> and be back-ported to stable trees to avoid user frustration. I spent
> some time last night debugging one such instance, and the user
> afterwards indicated that they'd had the problem for a long time, and
> had thought of throwing the hardware out the window!  Clearly not a
> good user experience.
> 
>  drivers/net/phy/marvell10g.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> index 100b401b1f4a..754cde873dde 100644
> --- a/drivers/net/phy/marvell10g.c
> +++ b/drivers/net/phy/marvell10g.c
> @@ -31,6 +31,9 @@
>  #define MV_PHY_ALASKA_NBT_QUIRK_REV	(MARVELL_PHY_ID_88X3310 | 0xa)
>  
>  enum {
> +	MV_PMA_BOOT		= 0xc050,
> +	MV_PMA_BOOT_FATAL	= BIT(0),
> +
>  	MV_PCS_BASE_T		= 0x0000,
>  	MV_PCS_BASE_R		= 0x1000,
>  	MV_PCS_1000BASEX	= 0x2000,
> @@ -211,6 +214,16 @@ static int mv3310_probe(struct phy_device *phydev)
>  	    (phydev->c45_ids.devices_in_package & mmd_mask) != mmd_mask)
>  		return -ENODEV;
>  
> +	ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret & MV_PMA_BOOT_FATAL) {
> +		dev_warn(&phydev->mdio.dev,
> +			 "PHY failed to boot firmware, status=%04x\n", ret);
> +		return -ENODEV;

One question: are we happy with failing the probe like this, or would it
be better to allow the probe to suceed?

As has been pointed out in the C45 MII access patch, we need the PHY
to bind to the network driver for the MII bus to be accessible to
userspace, so if we're going to have userspace tools to upload the
firmware, rather than using u-boot, we need the PHY to be present and
bound to the network interface.

> +	}
> +
>  	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
> -- 
> 2.7.4
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH] net: phy: marvell10g: report if the PHY fails to boot firmware

[Andrew Lunn]

> One question: are we happy with failing the probe like this, or would it
> be better to allow the probe to suceed?
> 
> As has been pointed out in the C45 MII access patch, we need the PHY
> to bind to the network driver for the MII bus to be accessible to
> userspace, so if we're going to have userspace tools to upload the
> firmware, rather than using u-boot, we need the PHY to be present and
> bound to the network interface.

Hi Russell

It is an interesting question. Failing the probe is the simple
solution. If we don't fail the probe, we then need to allow the
attach, but fail all normal operations, with a noisy kernel log.  That
probably means adding a new state to the state machine, PHY_BROKEN.
Enter that state if phy_start_aneg() returns an error?

      Andrew

Re: [PATCH] net: phy: marvell10g: report if the PHY fails to boot firmware

[Russell King - ARM Linux admin]

On Tue, May 28, 2019 at 06:11:39PM +0200, Andrew Lunn wrote:
> > One question: are we happy with failing the probe like this, or would it
> > be better to allow the probe to suceed?
> > 
> > As has been pointed out in the C45 MII access patch, we need the PHY
> > to bind to the network driver for the MII bus to be accessible to
> > userspace, so if we're going to have userspace tools to upload the
> > firmware, rather than using u-boot, we need the PHY to be present and
> > bound to the network interface.
> 
> Hi Russell
> 
> It is an interesting question. Failing the probe is the simple
> solution. If we don't fail the probe, we then need to allow the
> attach, but fail all normal operations, with a noisy kernel log.  That
> probably means adding a new state to the state machine, PHY_BROKEN.
> Enter that state if phy_start_aneg() returns an error?

Hi Andrew,

I don't think we need a new state - I think we can trap it in
the link_change_notify() method, and force phydev->state to
PHY_HALTED if it's in phy_is_started() mode.

Maybe something like:

	if (phy_is_started(phydev) && priv->firmware_failed) {
		dev_warn(&phydev->mdio.dev,
			 "PHY firmware failure: link forced down");
		phydev->state = PHY_HALTED;
	}

Or maybe we just need to do that if phydev->state == PHY_UP or
PHY_RESUMING (the two states entered from phy_start())?

Russell.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH] net: phy: marvell10g: report if the PHY fails to boot firmware

[Russell King - ARM Linux admin]

On Tue, May 28, 2019 at 05:23:56PM +0100, Russell King - ARM Linux admin wrote:
> On Tue, May 28, 2019 at 06:11:39PM +0200, Andrew Lunn wrote:
> > > One question: are we happy with failing the probe like this, or would it
> > > be better to allow the probe to suceed?
> > > 
> > > As has been pointed out in the C45 MII access patch, we need the PHY
> > > to bind to the network driver for the MII bus to be accessible to
> > > userspace, so if we're going to have userspace tools to upload the
> > > firmware, rather than using u-boot, we need the PHY to be present and
> > > bound to the network interface.
> > 
> > Hi Russell
> > 
> > It is an interesting question. Failing the probe is the simple
> > solution. If we don't fail the probe, we then need to allow the
> > attach, but fail all normal operations, with a noisy kernel log.  That
> > probably means adding a new state to the state machine, PHY_BROKEN.
> > Enter that state if phy_start_aneg() returns an error?
> 
> Hi Andrew,
> 
> I don't think we need a new state - I think we can trap it in
> the link_change_notify() method, and force phydev->state to
> PHY_HALTED if it's in phy_is_started() mode.
> 
> Maybe something like:
> 
> 	if (phy_is_started(phydev) && priv->firmware_failed) {
> 		dev_warn(&phydev->mdio.dev,
> 			 "PHY firmware failure: link forced down");
> 		phydev->state = PHY_HALTED;
> 	}
> 
> Or maybe we just need to do that if phydev->state == PHY_UP or
> PHY_RESUMING (the two states entered from phy_start())?

Here's the fuller patch for what I'm suggesting:

 drivers/net/phy/marvell10g.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 754cde873dde..86333d98b384 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -60,6 +60,8 @@ enum {
 };
 
 struct mv3310_priv {
+	bool firmware_failed;
+
 	struct device *hwmon_dev;
 	char *hwmon_name;
 };
@@ -214,6 +216,10 @@ static int mv3310_probe(struct phy_device *phydev)
 	    (phydev->c45_ids.devices_in_package & mmd_mask) != mmd_mask)
 		return -ENODEV;
 
+	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
 	ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT);
 	if (ret < 0)
 		return ret;
@@ -221,13 +227,9 @@ static int mv3310_probe(struct phy_device *phydev)
 	if (ret & MV_PMA_BOOT_FATAL) {
 		dev_warn(&phydev->mdio.dev,
 			 "PHY failed to boot firmware, status=%04x\n", ret);
-		return -ENODEV;
+		priv->firmware_failed = true;
 	}
 
-	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
-
 	dev_set_drvdata(&phydev->mdio.dev, priv);
 
 	ret = mv3310_hwmon_probe(phydev);
@@ -247,6 +249,19 @@ static int mv3310_resume(struct phy_device *phydev)
 	return mv3310_hwmon_config(phydev, true);
 }
 
+static void mv3310_link_change_notify(struct phy_device *phydev)
+{
+	struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev);
+	enum phy_state state = phydev->state;
+
+	if (priv->firmware_failed &&
+	    (state == PHY_UP || state == PHY_RESUMING)) {
+		dev_warn(&phydev->mdio.dev,
+			 "PHY firmware failure: link forced down");
+		phydev->state = state = PHY_HALTED;
+	}
+}
+
 /* Some PHYs in the Alaska family such as the 88X3310 and the 88E2010
  * don't set bit 14 in PMA Extended Abilities (1.11), although they do
  * support 2.5GBASET and 5GBASET. For these models, we can still read their
-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH] net: phy: marvell10g: report if the PHY fails to boot firmware

[Andrew Lunn]

> Here's the fuller patch for what I'm suggesting:
> 
>  drivers/net/phy/marvell10g.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> index 754cde873dde..86333d98b384 100644
> --- a/drivers/net/phy/marvell10g.c
> +++ b/drivers/net/phy/marvell10g.c
> @@ -60,6 +60,8 @@ enum {
>  };
>  
>  struct mv3310_priv {
> +	bool firmware_failed;
> +
>  	struct device *hwmon_dev;
>  	char *hwmon_name;
>  };
> @@ -214,6 +216,10 @@ static int mv3310_probe(struct phy_device *phydev)
>  	    (phydev->c45_ids.devices_in_package & mmd_mask) != mmd_mask)
>  		return -ENODEV;
>  
> +	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
>  	ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT);
>  	if (ret < 0)
>  		return ret;
> @@ -221,13 +227,9 @@ static int mv3310_probe(struct phy_device *phydev)
>  	if (ret & MV_PMA_BOOT_FATAL) {
>  		dev_warn(&phydev->mdio.dev,
>  			 "PHY failed to boot firmware, status=%04x\n", ret);
> -		return -ENODEV;
> +		priv->firmware_failed = true;
>  	}
>  
> -	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
> -	if (!priv)
> -		return -ENOMEM;
> -
>  	dev_set_drvdata(&phydev->mdio.dev, priv);
>  
>  	ret = mv3310_hwmon_probe(phydev);
> @@ -247,6 +249,19 @@ static int mv3310_resume(struct phy_device *phydev)
>  	return mv3310_hwmon_config(phydev, true);
>  }
>  
> +static void mv3310_link_change_notify(struct phy_device *phydev)
> +{
> +	struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev);
> +	enum phy_state state = phydev->state;
> +
> +	if (priv->firmware_failed &&
> +	    (state == PHY_UP || state == PHY_RESUMING)) {
> +		dev_warn(&phydev->mdio.dev,
> +			 "PHY firmware failure: link forced down");
> +		phydev->state = state = PHY_HALTED;
> +	}
> +}

Hi Russell

This looks good.

     Andrew

Re: [PATCH] net: phy: marvell10g: report if the PHY fails to boot firmware

[David Miller]

From: Russell King <rmk+kernel@armlinux.org.uk>
Date: Tue, 28 May 2019 10:34:42 +0100

> Some boards do not have the PHY firmware programmed in the 3310's flash,
> which leads to the PHY not working as expected.  Warn the user when the
> PHY fails to boot the firmware and refuse to initialise.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Applied and queued up for -stable, thanks.

Longer term in net-next we can do the PHY_HALTED thing so that the PHY
is accessible to update/fix the broken firmware.

Re: [PATCH] net: phy: marvell10g: report if the PHY fails to boot firmware

[Russell King - ARM Linux admin]

On Wed, May 29, 2019 at 02:26:34PM -0700, David Miller wrote:
> From: Russell King <rmk+kernel@armlinux.org.uk>
> Date: Tue, 28 May 2019 10:34:42 +0100
> 
> > Some boards do not have the PHY firmware programmed in the 3310's flash,
> > which leads to the PHY not working as expected.  Warn the user when the
> > PHY fails to boot the firmware and refuse to initialise.
> > 
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> 
> Applied and queued up for -stable, thanks.
> 
> Longer term in net-next we can do the PHY_HALTED thing so that the PHY
> is accessible to update/fix the broken firmware.

Thanks David.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up