--- v4
+++ v3
@@ -1,152 +1,216 @@
-Currently, phylink requires that fwnodes with links to SFP cages have
-the 'managed = "in-band-status"' property, and based on this, the
-initial pl->cfg_link_an_mode gets set to MLO_AN_INBAND.
-
-However, some PHYs on SFP modules may have broken in-band autoneg, and
-in that case, phylink selects a pl->cur_link_an_mode which is MLO_AN_PHY,
-to tell the MAC/PCS side to disable in-band autoneg (link speed/status
-will come over the MDIO side channel).
-
-The check for PHY in-band autoneg capability is currently open-coded
-based on a PHY ID comparison against the BCM84881. But the same problem
-will also be need to solved in another case, where syncing in-band
-autoneg will be desired between the MAC/PCS and an on-board PHY.
-So the approach needs to be generalized, and eventually what is done for
-the BCM84881 needs to be replaced with a more generic solution.
-
-Add new API to the PHY device structure which allows it to report what
-it supports in terms of in-band autoneg (whether it can operate with it
-on, and whether it can operate with it off). The assumption is that
-there is a Clause 37 compatible state machine in the PHY's PCS, and it
-requires that the autoneg process completes before the lane transitions
-to data mode. If we have a mismatch between in-band autoneg modes, the
-system side link will be broken.
+Phylink parses the firmware node for 'managed = "in-band-status"' and
+populates the initial pl->cfg_link_an_mode to MLO_AN_PHY or MLO_AN_INBAND
+accordingly, but sometimes things do not really work out at runtime, and
+the pl->cur_link_an_mode may change.
+
+The most notable case is when an SFP module with a PHY that has broken
+in-band autoneg is attached. Phylink currently open-codes a check for
+the BCM84881 PHY ID and updates pl->cur_link_an_mode from MLO_AN_INBAND
+to MLO_AN_PHY.
+
+There is an additional degree of freedom I would like to add. This has
+to do with the on-board PHY case (not on SFP). Sometimes, a PHY can only
+operate with in-band autoneg enabled, but the MAC driver does not
+declare 'managed = "in-band-status"' in the firmware node (say it was
+recently converted from phylib to phylink). If the MAC driver is strict
+in its phylink ops implementation, it will disable in-band autoneg and
+thus the connection to the PHY will be broken.
+
+The firmware can (and should) be updated, but if the PHY driver is
+patched to report that it only supports in-band autoneg, then the
+pl->cur_link_an_mode can be fixed up to request in-band autoneg from the
+MAC driver, even if the firmware node does not. While I do not expect
+production systems to rely on this feature, it seems sensible to have it
+as long as it is not difficult to implement (the PHY driver should be
+updated with a small .validate_inband_aneg method), and it can even ease
+the transition from phylib to phylink.
+
+There is also the reverse case: the firmware node reports MLO_AN_INBAND
+but the on-board PHY doesn't support that. That sounds like a serious
+bug, so while we still do attempt to fix it up (it seems within our
+reach to handle it, and worth it), we print to the kernel log on a more
+severe tone and not just at the debug level.
+
+So if the 3 code paths:
+- phylink_sfp_config
+- phylink_connect_phy
+- phylink_fwnode_phy_connect
+
+do more or less the same thing (adapt pl->cur_link_an_mode based on the
+capability reported by the PHY), the intention is different. With SFP
+modules this behavior is absolutely to be expected, and pl->cfg_link_an_mode
+only denotes the initial operating mode. On the other hand, when the PHY
+is on-board, the initial link AN mode should ideally also be the final
+one. So the implementations for the three are different.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
-v3->v4:
-- split the SFP cur_link_an_mode fixup to separate patch (this one)
-- s/inband_aneg/an_inband/ to be more in line with phylink terminology
-- clearer documentation, added kerneldocs
-- don't return -EIO in phy_validate_an_inband(), this breaks with the
- Generic PHY driver because the expected return code is a bit mask, not
- a negative integer
-
- drivers/net/phy/phy.c | 25 +++++++++++++++++++++++++
- drivers/net/phy/phylink.c | 20 +++++++++++++++++---
- include/linux/phy.h | 17 +++++++++++++++++
- 3 files changed, 59 insertions(+), 3 deletions(-)
+ drivers/net/phy/phy.c | 13 ++++++++
+ drivers/net/phy/phylink.c | 63 +++++++++++++++++++++++++++++++++++++--
+ include/linux/phy.h | 16 ++++++++++
+ 3 files changed, 89 insertions(+), 3 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
-index e5b6cb1a77f9..2abbacf2c7cb 100644
+index f124a8a58bd4..975ae3595f8f 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
-@@ -733,6 +733,31 @@ static int phy_check_link_status(struct phy_device *phydev)
+@@ -750,6 +750,19 @@ static int phy_check_link_status(struct phy_device *phydev)
return 0;
}
-+/**
-+ * phy_validate_an_inband - validate which in-band autoneg modes are supported
-+ * @phydev: the phy_device struct
-+ * @interface: the MAC-side interface type
-+ *
-+ * Returns @PHY_AN_INBAND_UNKNOWN if it is unknown what in-band autoneg setting
-+ * is required for the given PHY mode, or a bit mask of @PHY_AN_INBAND_OFF (if
-+ * the PHY is able to work with in-band AN turned off) and @PHY_AN_INBAND_ON
-+ * (if it works with the feature turned on). With the Generic PHY driver, the
-+ * result will always be @PHY_AN_INBAND_UNKNOWN.
-+ */
-+int phy_validate_an_inband(struct phy_device *phydev,
-+ phy_interface_t interface)
++int phy_validate_inband_aneg(struct phy_device *phydev,
++ phy_interface_t interface)
+{
-+ /* We may be called before phy_attach_direct() force-binds the
-+ * generic PHY driver to this device. In that case, report an unknown
-+ * setting rather than -EIO as most other functions do.
-+ */
-+ if (!phydev->drv || !phydev->drv->validate_an_inband)
-+ return PHY_AN_INBAND_UNKNOWN;
-+
-+ return phydev->drv->validate_an_inband(phydev, interface);
++ if (!phydev->drv)
++ return -EIO;
++
++ if (!phydev->drv->validate_inband_aneg)
++ return PHY_INBAND_ANEG_UNKNOWN;
++
++ return phydev->drv->validate_inband_aneg(phydev, interface);
+}
-+EXPORT_SYMBOL_GPL(phy_validate_an_inband);
++EXPORT_SYMBOL_GPL(phy_validate_inband_aneg);
+
/**
- * _phy_start_aneg - start auto-negotiation for this PHY device
+ * phy_start_aneg - start auto-negotiation for this PHY device
* @phydev: the phy_device struct
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
-index 9e4b2dfc98d8..40b7e730fb33 100644
+index fd02ec265a39..f9a7c999821b 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
-@@ -2936,10 +2936,24 @@ static int phylink_sfp_config_phy(struct phylink *pl, struct phy_device *phy)
+@@ -1043,6 +1043,39 @@ static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy,
+ return phy_attach_direct(pl->netdev, phy, 0, interface);
+ }
+
++static unsigned int phylink_fixup_inband_aneg(struct phylink *pl,
++ struct phy_device *phy,
++ unsigned int mode)
++{
++ int ret;
++
++ ret = phy_validate_inband_aneg(phy, pl->link_interface);
++ if (ret == PHY_INBAND_ANEG_UNKNOWN) {
++ phylink_dbg(pl,
++ "PHY driver does not report in-band autoneg capability, assuming %s\n",
++ phylink_autoneg_inband(mode) ? "true" : "false");
++
++ return mode;
++ }
++
++ if (phylink_autoneg_inband(mode) && !(ret & PHY_INBAND_ANEG_ON)) {
++ phylink_err(pl,
++ "Requested in-band autoneg but driver does not support this, disabling it.\n");
++
++ return MLO_AN_PHY;
++ }
++
++ if (!phylink_autoneg_inband(mode) && !(ret & PHY_INBAND_ANEG_OFF)) {
++ phylink_dbg(pl,
++ "PHY driver requests in-band autoneg, force-enabling it.\n");
++
++ mode = MLO_AN_INBAND;
++ }
++
++ /* Peaceful agreement, isn't it great? */
++ return mode;
++}
++
+ /**
+ * phylink_connect_phy() - connect a PHY to the phylink instance
+ * @pl: a pointer to a &struct phylink returned from phylink_create()
+@@ -1062,6 +1095,9 @@ int phylink_connect_phy(struct phylink *pl, struct phy_device *phy)
+ {
+ int ret;
+
++ pl->cur_link_an_mode = phylink_fixup_inband_aneg(pl, phy,
++ pl->cfg_link_an_mode);
++
+ /* Use PHY device/driver interface */
+ if (pl->link_interface == PHY_INTERFACE_MODE_NA) {
+ pl->link_interface = phy->interface;
+@@ -1137,6 +1173,9 @@ int phylink_fwnode_phy_connect(struct phylink *pl,
+ if (!phy_dev)
+ return -ENODEV;
+
++ pl->cur_link_an_mode = phylink_fixup_inband_aneg(pl, phy_dev,
++ pl->cfg_link_an_mode);
++
+ ret = phy_attach_direct(pl->netdev, phy_dev, flags,
+ pl->link_interface);
+ if (ret) {
+@@ -2207,10 +2246,28 @@ static int phylink_sfp_config(struct phylink *pl, struct phy_device *phy,
return -EINVAL;
}
-- if (phylink_phy_no_inband(phy))
+- if (phy && phylink_phy_no_inband(phy))
- mode = MLO_AN_PHY;
- else
+ /* Select whether to operate in in-band mode or not, based on the
-+ * capability of the PHY in the current link mode.
++ * presence and capability of the PHY in the current link mode.
+ */
-+ ret = phy_validate_an_inband(phy, iface);
-+ if (ret == PHY_AN_INBAND_UNKNOWN) {
-+ if (phylink_phy_no_inband(phy))
++ if (phy) {
++ ret = phy_validate_inband_aneg(phy, iface);
++ if (ret == PHY_INBAND_ANEG_UNKNOWN) {
++ if (phylink_phy_no_inband(phy))
++ mode = MLO_AN_PHY;
++ else
++ mode = MLO_AN_INBAND;
++
++ phylink_dbg(pl,
++ "PHY driver does not report in-band autoneg capability, assuming %s\n",
++ phylink_autoneg_inband(mode) ? "true" : "false");
++ } else if (ret & PHY_INBAND_ANEG_ON) {
++ mode = MLO_AN_INBAND;
++ } else {
+ mode = MLO_AN_PHY;
-+ else
-+ mode = MLO_AN_INBAND;
-+
-+ phylink_dbg(pl,
-+ "PHY driver does not report in-band autoneg capability, assuming %s\n",
-+ phylink_autoneg_inband(mode) ? "true" : "false");
-+ } else if (ret & PHY_AN_INBAND_ON) {
++ }
++ } else {
mode = MLO_AN_INBAND;
-+ } else {
-+ mode = MLO_AN_PHY;
+ }
config.interface = iface;
linkmode_copy(support1, support);
diff --git a/include/linux/phy.h b/include/linux/phy.h
-index 9a3752c0c444..56a431d88dd9 100644
+index 736e1d1a47c4..4ac876f988ca 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
-@@ -761,6 +761,12 @@ struct phy_tdr_config {
+@@ -698,6 +698,12 @@ struct phy_tdr_config {
};
#define PHY_PAIR_ALL -1
-+enum phy_an_inband {
-+ PHY_AN_INBAND_UNKNOWN = BIT(0),
-+ PHY_AN_INBAND_OFF = BIT(1),
-+ PHY_AN_INBAND_ON = BIT(2),
++enum phy_inband_aneg {
++ PHY_INBAND_ANEG_UNKNOWN = BIT(0),
++ PHY_INBAND_ANEG_OFF = BIT(1),
++ PHY_INBAND_ANEG_ON = BIT(2),
+};
+
/**
* struct phy_driver - Driver structure for a particular PHY type
*
-@@ -845,6 +851,15 @@ struct phy_driver {
+@@ -767,6 +773,14 @@ struct phy_driver {
*/
int (*config_aneg)(struct phy_device *phydev);
+ /**
-+ * @validate_an_inband: Report what types of in-band auto-negotiation
++ * @validate_inband_aneg: Report what types of in-band auto-negotiation
+ * are available for the given PHY interface type. Returns a bit mask
-+ * of type enum phy_an_inband. Returning negative error codes is not
-+ * permitted.
++ * of type enum phy_inband_aneg.
+ */
-+ int (*validate_an_inband)(struct phy_device *phydev,
-+ phy_interface_t interface);
++ int (*validate_inband_aneg)(struct phy_device *phydev,
++ phy_interface_t interface);
+
/** @aneg_done: Determines the auto negotiation result */
int (*aneg_done)(struct phy_device *phydev);
-@@ -1540,6 +1555,8 @@ void phy_stop(struct phy_device *phydev);
+@@ -1458,6 +1472,8 @@ void phy_start(struct phy_device *phydev);
+ void phy_stop(struct phy_device *phydev);
int phy_config_aneg(struct phy_device *phydev);
int phy_start_aneg(struct phy_device *phydev);
++int phy_validate_inband_aneg(struct phy_device *phydev,
++ phy_interface_t interface);
int phy_aneg_done(struct phy_device *phydev);
-+int phy_validate_an_inband(struct phy_device *phydev,
-+ phy_interface_t interface);
int phy_speed_down(struct phy_device *phydev, bool sync);
int phy_speed_up(struct phy_device *phydev);
-
--
-2.34.1
-
+2.25.1
+