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
