[PATCH v4 net-next 0/8] Let phylink manage in-band AN for the PHY

[PATCH v4 net-next 0/8] Let phylink manage in-band AN for the PHY

[Vladimir Oltean]

Problem statement
~~~~~~~~~~~~~~~~~

The on-board SERDES link between an NXP (Lynx) PCS and a PHY may not
work, depending on whether U-Boot networking was used on that port or not.

There is no mechanism in Linux (with phylib/phylink, at least) to ensure
that the MAC driver and the PHY driver have synchronized settings for
in-band autoneg. It all depends on the 'managed = "in-band-status"'
device tree property, which does not reflect a stable and unchanging
reality, and furthermore, some (older) device trees may have this
property missing when they shouldn't.

Proposed solution
~~~~~~~~~~~~~~~~~

Extend the phy_device API with 2 new methods:
- phy_validate_an_inband()
- phy_config_an_inband()

Extend phylink with an opt-in bool sync_an_inband which makes sure that
the configured "unsigned int mode" (MLO_AN_PHY/MLO_AN_INBAND) is both
supported by the PHY, and actually applied to the PHY.

Make NXP drivers which use phylink and the Lynx PCS driver opt into the
new behavior. Other drivers can trivially do this as well, by setting
struct phylink_config :: sync_an_inband to true.

Compared to other solutions
~~~~~~~~~~~~~~~~~~~~~~~~~~~

Sean Anderson, in commit 5d93cfcf7360 ("net: dpaa: Convert to phylink"),
sets phylink_config :: ovr_an_inband to true. This doesn't quite solve
all problems, because we don't *know* that the PHY is set for in-band
autoneg. For example with the VSC8514, it all depends on what the
bootloader has/has not done. This solution eliminates the bootloader
dependency by actually programming in-band autoneg in the VSC8514 PHY.

Change log
~~~~~~~~~~

Changes in v4:
Make all new behavior opt-in.
Fix bug when Generic PHY driver is used.
Dropped support for PHY_AN_INBAND_OFF in at803x.

Changes in v3:
Added patch for the Atheros PHY family.
v3 at:
https://patchwork.kernel.org/project/netdevbpf/cover/20210922181446.2677089-1-vladimir.oltean@nxp.com/

Changes in v2:
Incorporated feedback from Russell, which was to consider PHYs on SFP
modules too, and unify phylink's detection of PHYs with broken in-band
autoneg with the newly introduced PHY driver methods.
v2 at:
https://patchwork.kernel.org/project/netdevbpf/cover/20210212172341.3489046-1-olteanv@gmail.com/

Vladimir Oltean (8):
  net: phylink: let phylink_sfp_config_phy() determine the MLO_AN_* mode
    to use
  net: phylink: introduce generic method to query PHY in-band autoneg
    capability
  net: phy: bcm84881: move the in-band capability check where it belongs
  net: phylink: add option to sync in-band autoneg setting between PCS
    and PHY
  net: phylink: explicitly configure in-band autoneg for on-board PHYs
  net: phy: mscc: configure in-band auto-negotiation for VSC8514
  net: phy: at803x: validate in-band autoneg for AT8031/AT8033
  net: opt MAC drivers which use Lynx PCS into phylink sync_an_inband

 drivers/net/dsa/ocelot/felix.c                |  2 +
 .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  |  1 +
 .../net/ethernet/freescale/enetc/enetc_pf.c   |  1 +
 .../net/ethernet/freescale/fman/fman_memac.c  | 16 +--
 drivers/net/phy/at803x.c                      | 10 ++
 drivers/net/phy/bcm84881.c                    | 10 ++
 drivers/net/phy/mscc/mscc.h                   |  2 +
 drivers/net/phy/mscc/mscc_main.c              | 21 ++++
 drivers/net/phy/phy.c                         | 51 ++++++++++
 drivers/net/phy/phylink.c                     | 97 +++++++++++++++----
 include/linux/phy.h                           | 27 ++++++
 include/linux/phylink.h                       |  7 ++
 12 files changed, 212 insertions(+), 33 deletions(-)

-- 
2.34.1

[PATCH v4 net-next 1/8] net: phylink: let phylink_sfp_config_phy() determine the MLO_AN_* mode to use

[Vladimir Oltean]

For now, the "mode" is assumed to be MLO_AN_INBAND unless there is a PHY,
and that PHY has broken inband capabilities. So, since phylink_sfp_config()
already has the PHY pointer, we can drop the "mode" argument and deduce
it locally.

We'll want to make the in-band capability determination also based on
the interface mode in use. Move the phy_no_inband() check inside
phylink_sfp_config(), right after the PHY mode was determined by
sfp_select_interface().

To avoid a forward-declaration, this change also moves
phylink_phy_no_inband() above phylink_sfp_config_phy().

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v3->v4:
phylink_sfp_config() got split into phylink_sfp_config_phy() and
phylink_sfp_config_optical(), we now move the code to _phy()

 drivers/net/phy/phylink.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 25feab1802ee..9e4b2dfc98d8 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2811,6 +2811,15 @@ int phylink_speed_up(struct phylink *pl)
 }
 EXPORT_SYMBOL_GPL(phylink_speed_up);
 
+/* The Broadcom BCM84881 in the Methode DM7052 is unable to provide a SGMII
+ * or 802.3z control word, so inband will not work.
+ */
+static bool phylink_phy_no_inband(struct phy_device *phy)
+{
+	return phy->is_c45 &&
+		(phy->c45_ids.device_ids[1] & 0xfffffff0) == 0xae025150;
+}
+
 static void phylink_sfp_attach(void *upstream, struct sfp_bus *bus)
 {
 	struct phylink *pl = upstream;
@@ -2891,13 +2900,13 @@ static void phylink_sfp_set_config(struct phylink *pl, u8 mode,
 		phylink_mac_initial_config(pl, false);
 }
 
-static int phylink_sfp_config_phy(struct phylink *pl, u8 mode,
-				  struct phy_device *phy)
+static int phylink_sfp_config_phy(struct phylink *pl, struct phy_device *phy)
 {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(support1);
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(support);
 	struct phylink_link_state config;
 	phy_interface_t iface;
+	u8 mode;
 	int ret;
 
 	linkmode_copy(support, phy->supported);
@@ -2927,6 +2936,11 @@ static int phylink_sfp_config_phy(struct phylink *pl, u8 mode,
 		return -EINVAL;
 	}
 
+	if (phylink_phy_no_inband(phy))
+		mode = MLO_AN_PHY;
+	else
+		mode = MLO_AN_INBAND;
+
 	config.interface = iface;
 	linkmode_copy(support1, support);
 	ret = phylink_validate(pl, support1, &config);
@@ -3082,20 +3096,10 @@ static void phylink_sfp_link_up(void *upstream)
 	phylink_enable_and_run_resolve(pl, PHYLINK_DISABLE_LINK);
 }
 
-/* The Broadcom BCM84881 in the Methode DM7052 is unable to provide a SGMII
- * or 802.3z control word, so inband will not work.
- */
-static bool phylink_phy_no_inband(struct phy_device *phy)
-{
-	return phy->is_c45 &&
-		(phy->c45_ids.device_ids[1] & 0xfffffff0) == 0xae025150;
-}
-
 static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
 {
 	struct phylink *pl = upstream;
 	phy_interface_t interface;
-	u8 mode;
 	int ret;
 
 	/*
@@ -3107,17 +3111,12 @@ static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
 	 */
 	phy_support_asym_pause(phy);
 
-	if (phylink_phy_no_inband(phy))
-		mode = MLO_AN_PHY;
-	else
-		mode = MLO_AN_INBAND;
-
 	/* Set the PHY's host supported interfaces */
 	phy_interface_and(phy->host_interfaces, phylink_sfp_interfaces,
 			  pl->config->supported_interfaces);
 
 	/* Do the initial configuration */
-	ret = phylink_sfp_config_phy(pl, mode, phy);
+	ret = phylink_sfp_config_phy(pl, phy);
 	if (ret < 0)
 		return ret;
 
-- 
2.34.1

[PATCH v4 net-next 2/8] net: phylink: introduce generic method to query PHY in-band autoneg capability

[Vladimir Oltean]

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.

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(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index e5b6cb1a77f9..2abbacf2c7cb 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)
 	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)
+{
+	/* 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);
+}
+EXPORT_SYMBOL_GPL(phy_validate_an_inband);
+
 /**
  * _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
--- 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)
 		return -EINVAL;
 	}
 
-	if (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.
+	 */
+	ret = phy_validate_an_inband(phy, iface);
+	if (ret == PHY_AN_INBAND_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_AN_INBAND_ON) {
 		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
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -761,6 +761,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),
+};
+
 /**
  * struct phy_driver - Driver structure for a particular PHY type
  *
@@ -845,6 +851,15 @@ struct phy_driver {
 	 */
 	int (*config_aneg)(struct phy_device *phydev);
 
+	/**
+	 * @validate_an_inband: 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.
+	 */
+	int (*validate_an_inband)(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);
 int phy_config_aneg(struct phy_device *phydev);
 int phy_start_aneg(struct phy_device *phydev);
 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

Re: [PATCH v4 net-next 2/8] net: phylink: introduce generic method to query PHY in-band autoneg capability

[Sean Anderson]

On 11/17/22 19:01, Vladimir Oltean wrote:
> 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.
> 
> 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(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index e5b6cb1a77f9..2abbacf2c7cb 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)
>  	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)
> +{
> +	/* 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);
> +}
> +EXPORT_SYMBOL_GPL(phy_validate_an_inband);
> +
>  /**
>   * _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
> --- 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)
>  		return -EINVAL;
>  	}
>  
> -	if (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.
> +	 */
> +	ret = phy_validate_an_inband(phy, iface);
> +	if (ret == PHY_AN_INBAND_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_AN_INBAND_ON) {
>  		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
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -761,6 +761,12 @@ struct phy_tdr_config {
>  };
>  #define PHY_PAIR_ALL -1
>  
> +enum phy_an_inband {
> +	PHY_AN_INBAND_UNKNOWN		= BIT(0),

Shouldn't this be something like

	PHY_AN_INBAND_UNKNOWN		= 0,

? What does it mean if a phy returns e.g. 0b101?

--Sean

> +	PHY_AN_INBAND_OFF		= BIT(1),
> +	PHY_AN_INBAND_ON		= BIT(2),
> +};
> +
>  /**
>   * struct phy_driver - Driver structure for a particular PHY type
>   *
> @@ -845,6 +851,15 @@ struct phy_driver {
>  	 */
>  	int (*config_aneg)(struct phy_device *phydev);
>  
> +	/**
> +	 * @validate_an_inband: 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.
> +	 */
> +	int (*validate_an_inband)(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);
>  int phy_config_aneg(struct phy_device *phydev);
>  int phy_start_aneg(struct phy_device *phydev);
>  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);
>  

Re: [PATCH v4 net-next 2/8] net: phylink: introduce generic method to query PHY in-band autoneg capability

[Vladimir Oltean]

On Fri, Nov 18, 2022 at 10:11:06AM -0500, Sean Anderson wrote:
> > +enum phy_an_inband {
> > +	PHY_AN_INBAND_UNKNOWN		= BIT(0),
> 
> Shouldn't this be something like
> 
> 	PHY_AN_INBAND_UNKNOWN		= 0,
> 
> ?

Could be 0 as well. The code explicitly tests against PHY_AN_INBAND_UNKNOWN
everywhere, so the precise value doesn't matter too much.

> What does it mean if a phy returns e.g. 0b101?

You mean PHY_AN_INBAND_ON | PHY_AN_INBAND_UNKNOWN. Well, it doesn't mean
anything, it's not a valid return code. I didn't make the code too defensive
in this regard, because I didn't see a reason for making some pieces of
code defend themselves against other pieces of code. It's a bit mask of
3 bits where not all combinations are valid. Even if PHY_AN_INBAND_UNKNOWN
was defined as 0 instead of BIT(0), it would still be just as logically
invalid to return PHY_AN_INBAND_ON | PHY_AN_INBAND_UNKNOWN, but this
would be indistinguishable in machine code from just PHY_AN_INBAND_ON.

I don't know, I don't see a practical reason to make a change here.

Re: [PATCH v4 net-next 2/8] net: phylink: introduce generic method to query PHY in-band autoneg capability

[Sean Anderson]

On 11/18/22 10:42, Vladimir Oltean wrote:
> On Fri, Nov 18, 2022 at 10:11:06AM -0500, Sean Anderson wrote:
>> > +enum phy_an_inband {
>> > +	PHY_AN_INBAND_UNKNOWN		= BIT(0),
>> 
>> Shouldn't this be something like
>> 
>> 	PHY_AN_INBAND_UNKNOWN		= 0,
>> 
>> ?
> 
> Could be 0 as well. The code explicitly tests against PHY_AN_INBAND_UNKNOWN
> everywhere, so the precise value doesn't matter too much.
> 
>> What does it mean if a phy returns e.g. 0b101?
> 
> You mean PHY_AN_INBAND_ON | PHY_AN_INBAND_UNKNOWN. Well, it doesn't mean
> anything, it's not a valid return code. I didn't make the code too defensive
> in this regard, because I didn't see a reason for making some pieces of
> code defend themselves against other pieces of code. It's a bit mask of
> 3 bits where not all combinations are valid. Even if PHY_AN_INBAND_UNKNOWN
> was defined as 0 instead of BIT(0), it would still be just as logically
> invalid to return PHY_AN_INBAND_ON | PHY_AN_INBAND_UNKNOWN, but this
> would be indistinguishable in machine code from just PHY_AN_INBAND_ON.
> 
> I don't know, I don't see a practical reason to make a change here.

If we have the opportunity, we should try to make invalid return codes
inexpressible. If we remove the extra bit, then all the combinations we
would like to have:

- I don't know what I support
- In-band must be enabled
- In-band must be disabled
- I can support either

are exactly the combinations supported by the underlying data.

--Sean

Re: [PATCH v4 net-next 2/8] net: phylink: introduce generic method to query PHY in-band autoneg capability

[Vladimir Oltean]

On Fri, Nov 18, 2022 at 10:49:30AM -0500, Sean Anderson wrote:
> If we have the opportunity, we should try to make invalid return codes
> inexpressible. If we remove the extra bit, then all the combinations we
> would like to have:
> 
> - I don't know what I support
> - In-band must be enabled
> - In-band must be disabled
> - I can support either
> 
> are exactly the combinations supported by the underlying data.

So the change request is to make the enum look like this?

enum phy_an_inband {
	PHY_AN_INBAND_UNKNOWN		= 0,
	PHY_AN_INBAND_OFF		= BIT(0),
	PHY_AN_INBAND_ON		= BIT(1),
};

Re: [PATCH v4 net-next 2/8] net: phylink: introduce generic method to query PHY in-band autoneg capability

[Sean Anderson]

On 11/18/22 10:56, Vladimir Oltean wrote:
> On Fri, Nov 18, 2022 at 10:49:30AM -0500, Sean Anderson wrote:
>> If we have the opportunity, we should try to make invalid return codes
>> inexpressible. If we remove the extra bit, then all the combinations we
>> would like to have:
>> 
>> - I don't know what I support
>> - In-band must be enabled
>> - In-band must be disabled
>> - I can support either
>> 
>> are exactly the combinations supported by the underlying data.
> 
> So the change request is to make the enum look like this?
> 
> enum phy_an_inband {
> 	PHY_AN_INBAND_UNKNOWN		= 0,
> 	PHY_AN_INBAND_OFF		= BIT(0),
> 	PHY_AN_INBAND_ON		= BIT(1),
> };

Yes.

--Sean

Re: [PATCH v4 net-next 2/8] net: phylink: introduce generic method to query PHY in-band autoneg capability

[Vladimir Oltean]

On Fri, Nov 18, 2022 at 10:57:13AM -0500, Sean Anderson wrote:
> On 11/18/22 10:56, Vladimir Oltean wrote:
> > So the change request is to make the enum look like this?
> > 
> > enum phy_an_inband {
> > 	PHY_AN_INBAND_UNKNOWN		= 0,
> > 	PHY_AN_INBAND_OFF		= BIT(0),
> > 	PHY_AN_INBAND_ON		= BIT(1),
> > };
> 
> Yes.

Ok, I'll make this change and wait for more feedback, and if nothing
else, will resubmit on Monday.

Re: [PATCH v4 net-next 2/8] net: phylink: introduce generic method to query PHY in-band autoneg capability

[Russell King (Oracle)]

On Fri, Nov 18, 2022 at 02:01:18AM +0200, Vladimir Oltean wrote:
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 9a3752c0c444..56a431d88dd9 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -761,6 +761,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),
> +};

There is another option here:

- unknown (basically, PHY driver doesn't implement the function or
  can't report)
- off (PHY driver knows for certain that in-band isn't used)
- on (PHY driver knows that in-band is required and must be
  acknowledged)
- on-but-not-required (PHY driver knows that in-band can be used, but
  the PHY has hardware support for timing out waiting for the in-band
  acknowledgement - Marvell PHYs support this.)

Maybe the fourth state can be indicated by setting both _OFF and _ON ?

Given that there's four states, does it make sense for this to be a
bitfield?

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

Re: [PATCH v4 net-next 2/8] net: phylink: introduce generic method to query PHY in-band autoneg capability

[Vladimir Oltean]

On Tue, Nov 22, 2022 at 09:21:36AM +0000, Russell King (Oracle) wrote:
> > +enum phy_an_inband {
> > +	PHY_AN_INBAND_UNKNOWN		= BIT(0),
> > +	PHY_AN_INBAND_OFF		= BIT(1),
> > +	PHY_AN_INBAND_ON		= BIT(2),
> > +};
> 
> There is another option here:
> 
> - unknown (basically, PHY driver doesn't implement the function or
>   can't report)
> - off (PHY driver knows for certain that in-band isn't used)
> - on (PHY driver knows that in-band is required and must be
>   acknowledged)
> - on-but-not-required (PHY driver knows that in-band can be used, but
>   the PHY has hardware support for timing out waiting for the in-band
>   acknowledgement - Marvell PHYs support this.)
> 
> Maybe the fourth state can be indicated by setting both _OFF and _ON ?
> 
> Given that there's four states, does it make sense for this to be a
> bitfield?

Setting both _OFF and _ON in the capability report would already have
the meaning that it's configurable via a subsequent call to
phy_config_an_inband(). It's really configurable in VSC8514. I suppose
introducing PHY_AN_INBAND_ON_TIMEOUT = BIT(3) could make sense as
another option for the capability, orthogonal to the other 2.

Maybe it would be useful in itself if the MAC cannot support MLO_AN_INBAND,
like the Lynx PCS in 2500base-x, and the PHY only reports PHY_AN_INBAND_ON |
PHY_AN_INBAND_ON_TIMEOUT (hypothetical example). Phylink would pick
PHY_AN_INBAND_ON_TIMEOUT.

Given that I don't have a use case for this, should I add PHY_AN_INBAND_ON_TIMEOUT
to this patch set or should that be done by someone for whom it makes a difference?

The relevant implication for this patch set is the function prototype of
phy_config_an_enabled(struct phy_device *phydev, bool enabled). It
shouldn't take a bool enabled, but an enum phy_an_inband for future
extensibility (and reject/ignore PHY_AN_INBAND_UNKNOWN).

Re: [PATCH v4 net-next 2/8] net: phylink: introduce generic method to query PHY in-band autoneg capability

[Vladimir Oltean]

On Tue, Nov 22, 2022 at 11:41:31AM +0200, Vladimir Oltean wrote:
> Maybe it would be useful in itself if the MAC cannot support MLO_AN_INBAND,
> like the Lynx PCS in 2500base-x, and the PHY only reports PHY_AN_INBAND_ON |
> PHY_AN_INBAND_ON_TIMEOUT (hypothetical example). Phylink would pick
> PHY_AN_INBAND_ON_TIMEOUT.

There's a separate but very much related issue which is that phylink
doesn't know that the Lynx PCS doesn't support MLO_AN_INBAND with
2500base-x. So it couldn't make the determination to select
PHY_AN_INBAND_ON_TIMEOUT rather than PHY_AN_INBAND_ON right now.

[PATCH v4 net-next 3/8] net: phy: bcm84881: move the in-band capability check where it belongs

[Vladimir Oltean]

Now that there is a generic interface through which phylink can query
PHY drivers whether they support various forms of in-band autoneg, use
that and delete the special case from phylink.c.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v3->v4:
- since we are now in phylink_sfp_config_phy() rather than
  phylink_sfp_config(), drop "if (phy)" check
- s/inband_aneg/an_inband/

 drivers/net/phy/bcm84881.c | 10 ++++++++++
 drivers/net/phy/phylink.c  | 17 ++---------------
 2 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/net/phy/bcm84881.c b/drivers/net/phy/bcm84881.c
index 9717a1626f3f..ab03acee77ea 100644
--- a/drivers/net/phy/bcm84881.c
+++ b/drivers/net/phy/bcm84881.c
@@ -223,6 +223,15 @@ static int bcm84881_read_status(struct phy_device *phydev)
 	return genphy_c45_read_mdix(phydev);
 }
 
+/* The Broadcom BCM84881 in the Methode DM7052 is unable to provide a SGMII
+ * or 802.3z control word, so inband will not work.
+ */
+static int bcm84881_validate_an_inband(struct phy_device *phydev,
+				       phy_interface_t interface)
+{
+	return PHY_AN_INBAND_OFF;
+}
+
 static struct phy_driver bcm84881_drivers[] = {
 	{
 		.phy_id		= 0xae025150,
@@ -234,6 +243,7 @@ static struct phy_driver bcm84881_drivers[] = {
 		.config_aneg	= bcm84881_config_aneg,
 		.aneg_done	= bcm84881_aneg_done,
 		.read_status	= bcm84881_read_status,
+		.validate_an_inband = bcm84881_validate_an_inband,
 	},
 };
 
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 40b7e730fb33..bf2a5ebfc4f4 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2811,15 +2811,6 @@ int phylink_speed_up(struct phylink *pl)
 }
 EXPORT_SYMBOL_GPL(phylink_speed_up);
 
-/* The Broadcom BCM84881 in the Methode DM7052 is unable to provide a SGMII
- * or 802.3z control word, so inband will not work.
- */
-static bool phylink_phy_no_inband(struct phy_device *phy)
-{
-	return phy->is_c45 &&
-		(phy->c45_ids.device_ids[1] & 0xfffffff0) == 0xae025150;
-}
-
 static void phylink_sfp_attach(void *upstream, struct sfp_bus *bus)
 {
 	struct phylink *pl = upstream;
@@ -2941,14 +2932,10 @@ static int phylink_sfp_config_phy(struct phylink *pl, struct phy_device *phy)
 	 */
 	ret = phy_validate_an_inband(phy, iface);
 	if (ret == PHY_AN_INBAND_UNKNOWN) {
-		if (phylink_phy_no_inband(phy))
-			mode = MLO_AN_PHY;
-		else
-			mode = MLO_AN_INBAND;
+		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");
+			    "PHY driver does not report in-band autoneg capability, assuming true\n");
 	} else if (ret & PHY_AN_INBAND_ON) {
 		mode = MLO_AN_INBAND;
 	} else {
-- 
2.34.1

Re: [PATCH v4 net-next 3/8] net: phy: bcm84881: move the in-band capability check where it belongs

[Russell King (Oracle)]

On Fri, Nov 18, 2022 at 02:01:19AM +0200, Vladimir Oltean wrote:
> Now that there is a generic interface through which phylink can query
> PHY drivers whether they support various forms of in-band autoneg, use
> that and delete the special case from phylink.c.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

I think I'd prefer to see patch 2 and patch 3 first in this series
(patch 3 without the phylink change). Possibly followed by other PHY
driver patches adding the validate_an_inband function, but that's not
important. Then the next patch can be patch 1 and the phylink part of
this patch combined - which makes the changes to phylink smaller as
there's no need to move the phylink_phy_no_inband() function and then
delete it a few patches later.

Also, if we get the Marvell driver implementing validate_an_inband()
then I believe we can get rid of other parts of this patch - 88E1111 is
the commonly used accessible PHY on gigabit SFPs, as this PHY implements
I2C access natively. As I mentioned, Marvell PHYs can be set to no
inband, requiring inband, or inband with bypass mode enabled. So we
need to decide how we deal with that - especially if we're going to be
changing the mode from 1000base-X to SGMII (which we do on some SFP
modules so they work at 10/100/1000.)

In that regard, I'm not entirely convinced that validate_an_inband()
covers the functionality we need - as reading the config register on
Marvell hardware doesn't guarantee that we're reading the right mode -
the PHY may be in 1000base-X, and we might change it to
SGMII-with-bypass - I'd need to go through the PHY datasheets to check
what we actually do.

Changing what the PHY driver does would be a recipe for regressions,
especially for drivers that do not use phylink.

Sorry, the above is a bit rambling, but are my thoughts on the current
approach.

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

Re: [PATCH v4 net-next 3/8] net: phy: bcm84881: move the in-band capability check where it belongs

[Vladimir Oltean]

On Tue, Nov 22, 2022 at 09:38:43AM +0000, Russell King (Oracle) wrote:
> Also, if we get the Marvell driver implementing validate_an_inband()
> then I believe we can get rid of other parts of this patch - 88E1111 is
> the commonly used accessible PHY on gigabit SFPs, as this PHY implements
> I2C access natively. As I mentioned, Marvell PHYs can be set to no
> inband, requiring inband, or inband with bypass mode enabled. So we
> need to decide how we deal with that - especially if we're going to be
> changing the mode from 1000base-X to SGMII (which we do on some SFP
> modules so they work at 10/100/1000.)

Not clear which parts of this patch we could ged rid of, if we
implemented in-band AN reporting/configuration for the 88E1111.
Based on your previous description, it sounds like it would be just more
functionality for software rather than less.

> In that regard, I'm not entirely convinced that validate_an_inband()
> covers the functionality we need - as reading the config register on
> Marvell hardware doesn't guarantee that we're reading the right mode -
> the PHY may be in 1000base-X, and we might change it to
> SGMII-with-bypass - I'd need to go through the PHY datasheets to check
> what we actually do.
> 
> Changing what the PHY driver does would be a recipe for regressions,
> especially for drivers that do not use phylink.

I believe that currently, the "interface" passed to phy_validate_an_inband()
and phy_config_an_inband() is always also the phydev->interface.
We could strive to keep that being the case, and put a phydev_warn() in
the Marvell PHY driver if we get a query for interface != phydev->interface,
and report PHY_AN_INBAND_UNKNOWN.

It's also one of the reasons why I didn't yet want to jump right into
figuring out what should be done with PHYs that change SERDES protocols,
and when exactly we query the in-band capability for the new one. Right
now, phylink will assume that the in-band capability reported for the
first SERDES protocol will continue to be the same for all subsequent
protocols. Obviously this might not be the case.

Re: [PATCH v4 net-next 3/8] net: phy: bcm84881: move the in-band capability check where it belongs

[Russell King (Oracle)]

On Tue, Nov 22, 2022 at 09:38:43AM +0000, Russell King (Oracle) wrote:
> Also, if we get the Marvell driver implementing validate_an_inband()
> then I believe we can get rid of other parts of this patch - 88E1111 is
> the commonly used accessible PHY on gigabit SFPs, as this PHY implements
> I2C access natively. As I mentioned, Marvell PHYs can be set to no
> inband, requiring inband, or inband with bypass mode enabled. So we
> need to decide how we deal with that - especially if we're going to be
> changing the mode from 1000base-X to SGMII (which we do on some SFP
> modules so they work at 10/100/1000.)

For the Marvell 88E1111:

- If switching into 1000base-X mode, then bypass mode is enabled by
m88e1111_config_init_1000basex(). However, if AN is disabled in the
fibre page BMCR (e.g. by firmware), then AN won't be used.

- If switching into SGMII mode, then bypass mode is left however it was
originally set by m88e1111_config_init_sgmii() - so it may be allowed
or it may be disallowed, which means it's whatever the hardware
defaulted to or firmware set it as.

For the 88e151x (x=0,2,8) it looks like bypass mode defaults to being
allowed on hardware reset, but firmware may change that.

I don't think we make much of an effort to configure other Marvell
PHYs, relying on their hardware reset defaults or firmware to set
them appropriately.

So, I think for 88e151x, we should implement something like:

	int mode, bmcr, fscr2;

	/* RGMII too? I believe RGMII can signal inband as well, so we
	 * may need to handle that as well.
	 */
	if (interface != PHY_INTERFACE_MODE_SGMII &&
	    interface != PHY_INTERFACE_MODE_1000BASE_X)
		return PHY_AN_INBAND_UNKNOWN;

	bmcr = phy_read_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR);
	if (bmcr < 0)
		return SOME_ERROR?

	mode = PHY_AN_INBAND_OFF;

	if (bmcr & BMCR_ANENABLE) {
		mode = PHY_AN_INBAND_ON;

		fscr2 = phy_read_paged(phydev, MII_MARVELL_FIBER_PAGE,
				       0x1a);
		if (fscr2 & BIT(6))
			mode |= PHY_AN_INBAND_TIMEOUT;
	}

	return mode;

Obviously adding register definitions for BIT(6) and 01a.

For the 88E1111:

	int mode, hwcfg;

	/* If operating in 1000base-X mode, we always turn on inband
	 * and allow bypass.
	 */
	if (interface == PHY_INTERFACE_MODE_1000BASEX)
		return PHY_AN_INBAND_ON | PHY_AN_INBAND_TIMEOUT;

	if (interface == PHY_INTERFACE_MODE_SGMII) {
		hwcfg = phy_read(phydev, MII_M1111_PHY_EXT_SR);
		if (hwcfg < 0)
			return SOME_ERROR?

		mode = PHY_AN_INBAND_ON;
		if (hwcfg & MII_M1111_HWCFG_SERIAL_AN_BYPASS)
			mode |= PHY_AN_INBAND_TIMEOUT;

		return mode;
	}

	return PHY_AN_INBAND_UNKNOWN;

Maybe?

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

Re: [PATCH v4 net-next 3/8] net: phy: bcm84881: move the in-band capability check where it belongs

[Vladimir Oltean]

On Tue, Nov 22, 2022 at 11:16:07AM +0000, Russell King (Oracle) wrote:
> On Tue, Nov 22, 2022 at 09:38:43AM +0000, Russell King (Oracle) wrote:
> > Also, if we get the Marvell driver implementing validate_an_inband()
> > then I believe we can get rid of other parts of this patch - 88E1111 is
> > the commonly used accessible PHY on gigabit SFPs, as this PHY implements
> > I2C access natively. As I mentioned, Marvell PHYs can be set to no
> > inband, requiring inband, or inband with bypass mode enabled. So we
> > need to decide how we deal with that - especially if we're going to be
> > changing the mode from 1000base-X to SGMII (which we do on some SFP
> > modules so they work at 10/100/1000.)
> 
> For the Marvell 88E1111:
> 
> - If switching into 1000base-X mode, then bypass mode is enabled by
> m88e1111_config_init_1000basex(). However, if AN is disabled in the
> fibre page BMCR (e.g. by firmware), then AN won't be used.
> 
> - If switching into SGMII mode, then bypass mode is left however it was
> originally set by m88e1111_config_init_sgmii() - so it may be allowed
> or it may be disallowed, which means it's whatever the hardware
> defaulted to or firmware set it as.
> 
> For the 88e151x (x=0,2,8) it looks like bypass mode defaults to being
> allowed on hardware reset, but firmware may change that.
> 
> I don't think we make much of an effort to configure other Marvell
> PHYs, relying on their hardware reset defaults or firmware to set
> them appropriately.
> 
> So, I think for 88e151x, we should implement something like:
> 
> 	int mode, bmcr, fscr2;
> 
> 	/* RGMII too? I believe RGMII can signal inband as well, so we
> 	 * may need to handle that as well.
> 	 */
> 	if (interface != PHY_INTERFACE_MODE_SGMII &&
> 	    interface != PHY_INTERFACE_MODE_1000BASE_X)
> 		return PHY_AN_INBAND_UNKNOWN;
> 
> 	bmcr = phy_read_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR);
> 	if (bmcr < 0)
> 		return SOME_ERROR?

There's a limitation in the API presented here, you can't return
SOME_ERROR, you have to return PHY_AN_INBAND_UNKNOWN and maybe log the
error to the console. If the error persists, other PHY methods will
eventually catch it.

> 
> 	mode = PHY_AN_INBAND_OFF;
> 
> 	if (bmcr & BMCR_ANENABLE) {
> 		mode = PHY_AN_INBAND_ON;
> 
> 		fscr2 = phy_read_paged(phydev, MII_MARVELL_FIBER_PAGE,
> 				       0x1a);
> 		if (fscr2 & BIT(6))
> 			mode |= PHY_AN_INBAND_TIMEOUT;
> 	}
> 
> 	return mode;
> 
> Obviously adding register definitions for BIT(6) and 01a.
> 
> For the 88E1111:
> 
> 	int mode, hwcfg;
> 
> 	/* If operating in 1000base-X mode, we always turn on inband
> 	 * and allow bypass.
> 	 */
> 	if (interface == PHY_INTERFACE_MODE_1000BASEX)
> 		return PHY_AN_INBAND_ON | PHY_AN_INBAND_TIMEOUT;
> 
> 	if (interface == PHY_INTERFACE_MODE_SGMII) {
> 		hwcfg = phy_read(phydev, MII_M1111_PHY_EXT_SR);
> 		if (hwcfg < 0)
> 			return SOME_ERROR?
> 
> 		mode = PHY_AN_INBAND_ON;
> 		if (hwcfg & MII_M1111_HWCFG_SERIAL_AN_BYPASS)
> 			mode |= PHY_AN_INBAND_TIMEOUT;
> 
> 		return mode;
> 	}
> 
> 	return PHY_AN_INBAND_UNKNOWN;
> 
> Maybe?

Hmm, not quite (neither for 88E151x not 88E1111). The intention with the
validate()/config() split is that you either implement just validate(),
or both. If you implement just validate(), you should report just one
bit, corresponding to what the hardware is configured for (so either
PHY_AN_INBAND_ON, *or* PHY_AN_INBAND_TIMEOUT). This is because you'd
otherwise tell phylink that 2 modes are supported, but provide no way to
choose between them, and you don't make it clear which one is in use
either. This will force phylink to adapt to MLO_AN_PHY or MLO_AN_INBAND,
depending on what has a chance of working.

If you implement config_an_inband() too, then the validate procedure
becomes a simple report of what can be configured for that PHY
(OFF | ON | ON_TIMEOUT for 88E151x, and ON | ON_TIMEOUT for 88E1111).
It's then the config_an_inband() procedure that applies to hardware the
mode that is selected by phylink. From config_an_inband() you can return
a negative error code on PHY I/O failure.

If you can prepare some more formal patches for these PHYs for which I
don't have documentation, I think I have a copper SFP module which uses
SGMII and 88E1111, and I can plug it into the Honeycomb and see what
happens.

Re: [PATCH v4 net-next 3/8] net: phy: bcm84881: move the in-band capability check where it belongs

[Russell King (Oracle)]

On Tue, Nov 22, 2022 at 02:11:22PM +0200, Vladimir Oltean wrote:
> On Tue, Nov 22, 2022 at 11:16:07AM +0000, Russell King (Oracle) wrote:
> > On Tue, Nov 22, 2022 at 09:38:43AM +0000, Russell King (Oracle) wrote:
> > > Also, if we get the Marvell driver implementing validate_an_inband()
> > > then I believe we can get rid of other parts of this patch - 88E1111 is
> > > the commonly used accessible PHY on gigabit SFPs, as this PHY implements
> > > I2C access natively. As I mentioned, Marvell PHYs can be set to no
> > > inband, requiring inband, or inband with bypass mode enabled. So we
> > > need to decide how we deal with that - especially if we're going to be
> > > changing the mode from 1000base-X to SGMII (which we do on some SFP
> > > modules so they work at 10/100/1000.)
> > 
> > For the Marvell 88E1111:
> > 
> > - If switching into 1000base-X mode, then bypass mode is enabled by
> > m88e1111_config_init_1000basex(). However, if AN is disabled in the
> > fibre page BMCR (e.g. by firmware), then AN won't be used.
> > 
> > - If switching into SGMII mode, then bypass mode is left however it was
> > originally set by m88e1111_config_init_sgmii() - so it may be allowed
> > or it may be disallowed, which means it's whatever the hardware
> > defaulted to or firmware set it as.
> > 
> > For the 88e151x (x=0,2,8) it looks like bypass mode defaults to being
> > allowed on hardware reset, but firmware may change that.
> > 
> > I don't think we make much of an effort to configure other Marvell
> > PHYs, relying on their hardware reset defaults or firmware to set
> > them appropriately.
> > 
> > So, I think for 88e151x, we should implement something like:
> > 
> > 	int mode, bmcr, fscr2;
> > 
> > 	/* RGMII too? I believe RGMII can signal inband as well, so we
> > 	 * may need to handle that as well.
> > 	 */
> > 	if (interface != PHY_INTERFACE_MODE_SGMII &&
> > 	    interface != PHY_INTERFACE_MODE_1000BASE_X)
> > 		return PHY_AN_INBAND_UNKNOWN;
> > 
> > 	bmcr = phy_read_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR);
> > 	if (bmcr < 0)
> > 		return SOME_ERROR?
> 
> There's a limitation in the API presented here, you can't return
> SOME_ERROR, you have to return PHY_AN_INBAND_UNKNOWN and maybe log the
> error to the console. If the error persists, other PHY methods will
> eventually catch it.

Right.

> > 
> > 	mode = PHY_AN_INBAND_OFF;
> > 
> > 	if (bmcr & BMCR_ANENABLE) {
> > 		mode = PHY_AN_INBAND_ON;
> > 
> > 		fscr2 = phy_read_paged(phydev, MII_MARVELL_FIBER_PAGE,
> > 				       0x1a);
> > 		if (fscr2 & BIT(6))
> > 			mode |= PHY_AN_INBAND_TIMEOUT;
> > 	}
> > 
> > 	return mode;
> > 
> > Obviously adding register definitions for BIT(6) and 01a.
> > 
> > For the 88E1111:
> > 
> > 	int mode, hwcfg;
> > 
> > 	/* If operating in 1000base-X mode, we always turn on inband
> > 	 * and allow bypass.
> > 	 */
> > 	if (interface == PHY_INTERFACE_MODE_1000BASEX)
> > 		return PHY_AN_INBAND_ON | PHY_AN_INBAND_TIMEOUT;
> > 
> > 	if (interface == PHY_INTERFACE_MODE_SGMII) {
> > 		hwcfg = phy_read(phydev, MII_M1111_PHY_EXT_SR);
> > 		if (hwcfg < 0)
> > 			return SOME_ERROR?
> > 
> > 		mode = PHY_AN_INBAND_ON;
> > 		if (hwcfg & MII_M1111_HWCFG_SERIAL_AN_BYPASS)
> > 			mode |= PHY_AN_INBAND_TIMEOUT;
> > 
> > 		return mode;
> > 	}
> > 
> > 	return PHY_AN_INBAND_UNKNOWN;
> > 
> > Maybe?
> 
> Hmm, not quite (neither for 88E151x not 88E1111). The intention with the
> validate()/config() split is that you either implement just validate(),
> or both.

The intention of the above is to report how the PHY is going to behave
with the current code when the PHY is in operation, which I believe to
be the intention of the validate callback. I'm not proposing at the
moment to add the config() part, although that can be done later.

As I stated, with the 88e1111, if we are asked to operate in 1000base-X
mode, when we configure the PHY we will allow bypass mode and I believe
in-band will be enabled, because this is what config_init() does today
when operating in 1000base-X mode. If we add support for your config()
method, then we will need a way to prevent a later config_init()
changing that configuration.

For SGMII, things are a lot more complicated, the result depends on how
the PHY has been setup by firmware or possibly reset pin strapping, so
we need to read registers to work out how it's going to behave. So,
unless we do that, we just can't report anything with certainty. We
probably ought to be reading a register to check that in-band is indeed
enabled.

Note that a soft-reset doesn't change any of this - it won't enable
in-band if it was disabled, and it won't disable it if it was previously
set to be enabled.

> If you implement just validate(), you should report just one
> bit, corresponding to what the hardware is configured for (so either
> PHY_AN_INBAND_ON, *or* PHY_AN_INBAND_TIMEOUT). This is because you'd
> otherwise tell phylink that 2 modes are supported, but provide no way to
> choose between them, and you don't make it clear which one is in use
> either. This will force phylink to adapt to MLO_AN_PHY or MLO_AN_INBAND,
> depending on what has a chance of working.

Don't we have the same problem with PHY_AN_INBAND_TIMEOUT? If a PHY
reports that, do we use MLO_AN_INBAND or MLO_AN_PHY?

> If you implement config_an_inband() too, then the validate procedure
> becomes a simple report of what can be configured for that PHY
> (OFF | ON | ON_TIMEOUT for 88E151x, and ON | ON_TIMEOUT for 88E1111).
> It's then the config_an_inband() procedure that applies to hardware the
> mode that is selected by phylink. From config_an_inband() you can return
> a negative error code on PHY I/O failure.

So it sounds like the decision about which mode to use needs to be
coupled with "does the PHY driver implement config_an_inband()"

> If you can prepare some more formal patches for these PHYs for which I
> don't have documentation, I think I have a copper SFP module which uses
> SGMII and 88E1111, and I can plug it into the Honeycomb and see what
> happens.

I'm away from home at the moment, which means I don't have a way to
do any in-depth tests other than with the SFPs that are plugged into
my Honeycomb - which does include some copper SFPs but they're not
connected to anything. So I can't test to see if data passes until
I'm back home next week.

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

Re: [PATCH v4 net-next 3/8] net: phy: bcm84881: move the in-band capability check where it belongs

[Vladimir Oltean]

On Tue, Nov 22, 2022 at 04:58:45PM +0000, Russell King (Oracle) wrote:
> The intention of the above is to report how the PHY is going to behave
> with the current code when the PHY is in operation, which I believe to
> be the intention of the validate callback. I'm not proposing at the
> moment to add the config() part, although that can be done later.

Again, all I'm saying is that with validate() but no config(), you should
report a single bit, not PHY_AN_INBAND_ON | PHY_AN_INBAND_TIMEOUT.
The code which you posted does report multiple bits, and we won't know
how to make sense out of them. We'd select what is the most convenient
to us, and we'll call phy_config_an_inband() with that, but it'll return
-EOPNOTSUPP, which will be perfectly reasonable to us. Except that the
PHY may actually not currently operate in the mode it reported as a
supported capability. So things are broken.

> As I stated, with the 88e1111, if we are asked to operate in 1000base-X
> mode, when we configure the PHY we will allow bypass mode and I believe
> in-band will be enabled, because this is what config_init() does today
> when operating in 1000base-X mode. If we add support for your config()
> method, then we will need a way to prevent a later config_init()
> changing that configuration.

If config_init() is going to be kept being called only from
phy_attach_direct() -> phy_init_hw(), then, at least with phylink, we
only call phy_config_an_inband() from phylink_bringup_phy(), so after
the phy_attach_direct() call. So we overwrite what the driver does by
default.

The problem is not phy_config_an_inband() but phy_validate_an_inband().
We call that earlier than phy_attach_direct(), so if the PHY driver is
going to read a register from HW which hasn't yet been written, we get
an incorrect report of the current capabilities.

I'll see if I can fix that and delay phy_validate_an_inband() a bit,
maybe up until before right before phy_config_an_inband().

> 
> For SGMII, things are a lot more complicated, the result depends on how
> the PHY has been setup by firmware or possibly reset pin strapping, so
> we need to read registers to work out how it's going to behave. So,
> unless we do that, we just can't report anything with certainty. We
> probably ought to be reading a register to check that in-band is indeed
> enabled.
> 
> Note that a soft-reset doesn't change any of this - it won't enable
> in-band if it was disabled, and it won't disable it if it was previously
> set to be enabled.

Similar to VSC8514 and the U-Boot preconfiguration issue. Soft reset,
but the setting sticks.

> > If you implement just validate(), you should report just one
> > bit, corresponding to what the hardware is configured for (so either
> > PHY_AN_INBAND_ON, *or* PHY_AN_INBAND_TIMEOUT). This is because you'd
> > otherwise tell phylink that 2 modes are supported, but provide no way to
> > choose between them, and you don't make it clear which one is in use
> > either. This will force phylink to adapt to MLO_AN_PHY or MLO_AN_INBAND,
> > depending on what has a chance of working.
> 
> Don't we have the same problem with PHY_AN_INBAND_TIMEOUT? If a PHY
> reports that, do we use MLO_AN_INBAND or MLO_AN_PHY?

Well, I haven't yet written any logic for it.

To your question: "PHY_AN_INBAND_ON_TIMEOUT => MLO_AN_PHY or MLO_AN_INBAND"?
I'd say either depending on situation, since my expectation is that it's
compatible with both.

Always give preference to what's in the device tree if it can work
somehow. If it can work in fully compatible modes (MLO_AN_PHY with
PHY_AN_INBAND_OFF; MLO_AN_INBAND with PHY_AN_INBAND_ON), perfect.
If not, but what's in the device tree can work with PHY_AN_INBAND_ON_TIMEOUT,
also good => use ON_TIMEOUT.

If what's in the device tree needs to be changed, it pretty much means
that ON_TIMEOUT isn't supported by the PHY.

Concretely, I would propose something like this (a modification of the
function added by the patch set, notice the extra "an_inband" argument,
as well as the new checks for PHY_AN_INBAND_ON_TIMEOUT):

static void phylink_sync_an_inband(struct phylink *pl, struct phy_device *phy,
				   enum phy_an_inband *an_inband)
{
	unsigned int mode = pl->cfg_link_an_mode;
	int ret;

	if (!pl->config->sync_an_inband)
		return;

	ret = phy_validate_an_inband(phy, pl->link_config.interface);
	if (ret == PHY_AN_INBAND_UNKNOWN) {
		phylink_dbg(pl,
			    "PHY driver does not report in-band autoneg capability, assuming %s\n",
			    phylink_autoneg_inband(mode) ? "true" : "false");

		*an_inband = PHY_AN_INBAND_UNKNOWN;
	} else if (phylink_autoneg_inband(mode) &&
		   !(ret & PHY_AN_INBAND_ON) &&
		   !(ret & PHY_AN_INBAND_ON_TIMEOUT)) {
		phylink_err(pl,
			    "Requested in-band autoneg but driver does not support this, disabling it.\n");

		mode = MLO_AN_PHY;
		*an_inband = PHY_AN_INBAND_OFF;
	} else if (!phylink_autoneg_inband(mode) &&
		   !(ret & PHY_AN_INBAND_OFF) &&
		   !(ret & PHY_AN_INBAND_ON_TIMEOUT)) {
		phylink_dbg(pl,
			    "PHY driver requests in-band autoneg, force-enabling it.\n");

		mode = MLO_AN_INBAND;
		*an_inband = PHY_AN_INBAND_ON;
	} else {
		/* For the checks below, we've found a common operating
		 * mode with the PHY, just need to figure out if we
		 * agree fully or if we have to rely on the PHY's
		 * timeout ability
		 */
		if (phylink_autoneg_inband(mode)) {
			*an_inband = !!(ret & PHY_AN_INBAND_ON) ? PHY_AN_INBAND_ON :
					PHY_AN_INBAND_ON_TIMEOUT;
		} else {
			*an_inband = !!(ret & PHY_AN_INBAND_OFF) ? PHY_AN_INBAND_OFF :
					PHY_AN_INBAND_ON_TIMEOUT;
		}
	}

	pl->cur_link_an_mode = mode;
}

then call phy_config_an_inband() with "an_inband" as the mode to use.

As per Sean's feedback, we force the PHY to report at least one valid
capability, otherwise, 0 is PHY_AN_INBAND_UNKNOWN and it's also treated
correctly.

> > If you implement config_an_inband() too, then the validate procedure
> > becomes a simple report of what can be configured for that PHY
> > (OFF | ON | ON_TIMEOUT for 88E151x, and ON | ON_TIMEOUT for 88E1111).
> > It's then the config_an_inband() procedure that applies to hardware the
> > mode that is selected by phylink. From config_an_inband() you can return
> > a negative error code on PHY I/O failure.
> 
> So it sounds like the decision about which mode to use needs to be
> coupled with "does the PHY driver implement config_an_inband()"

So do you recommend that I should put a WARN_ON() somewhere, which
asserts something like this?

"if the weight (number of bits set) in the return code of
phy_validate_an_inband() is larger than 1, then phydev->drv->phy_config_an_inband()
must be a non-NULL pointer, to allow selecting between them"

> > If you can prepare some more formal patches for these PHYs for which I
> > don't have documentation, I think I have a copper SFP module which uses
> > SGMII and 88E1111, and I can plug it into the Honeycomb and see what
> > happens.
> 
> I'm away from home at the moment, which means I don't have a way to
> do any in-depth tests other than with the SFPs that are plugged into
> my Honeycomb - which does include some copper SFPs but they're not
> connected to anything. So I can't test to see if data passes until
> I'm back home next week.

I actually meant that I can test on a Solidrun Honeycomb board that I
happen to have access to, if you have some Marvell PHY code, even untested,
that I could try out. I'm pretty much in the dark when it comes to their
hardware documentation.

Re: [PATCH v4 net-next 3/8] net: phy: bcm84881: move the in-band capability check where it belongs

[Vladimir Oltean]

On Tue, Nov 22, 2022 at 07:56:25PM +0200, Vladimir Oltean wrote:
> On Tue, Nov 22, 2022 at 04:58:45PM +0000, Russell King (Oracle) wrote:
> > > I think I have a copper SFP module which uses SGMII and 88E1111,
> > > and I can plug it into the Honeycomb and see what happens.
> > 
> > I don't have a way to do any in-depth tests other than with the SFPs
> > that are plugged into my Honeycomb
> 
> I actually meant that I can test on a Solidrun Honeycomb board that I
> happen to have access to

I may have misunderstood what you said here. Subconsciously I thought
that you replied to what I said, but it looks unrelated. Disregard.

Anyway, considering I can't really patch the Marvell PHY driver
(knowing what I'm doing), not sure what to choose between waiting for
some help there (to have more coverage => less chances for regression)
and simply not converting dpaa1's ovr_an_inband to sync_an_inband.
I don't think the other NXP drivers are quite in the same risk of
regressions as dpaa1, as their device trees were introduced much more
recently compared to their conversion to phylink.

Re: [PATCH v4 net-next 3/8] net: phy: bcm84881: move the in-band capability check where it belongs

[Russell King (Oracle)]

On Tue, Nov 22, 2022 at 07:56:25PM +0200, Vladimir Oltean wrote:
> The problem is not phy_config_an_inband() but phy_validate_an_inband().
> We call that earlier than phy_attach_direct(), so if the PHY driver is
> going to read a register from HW which hasn't yet been written, we get
> an incorrect report of the current capabilities.

Why would it be "incorrect" ?

What the code I'm proposing correctly reports back what inband mode(s)
will be in use should we select the proposed interface mode. Let's
ignore whether we report the TIMEOUT or not for that statement, because
I think that's confusing the discussion.

If we _do_ want to report whether the TIMEOUT mode is going to be used
or not, the code I proposed is what will be necessary, because it
depends on (a) how the PHY is strapped and (b) how firmware or external
EEPROM has setup the device. If we want a single bit, then we would
report just _ON_TIMEOUT if bypass is enabled - but we still need to
read registers to come to a conclusion about whether it's enabled or
not. As I say, we can't blindly say "if interface is X, then bypass
will be enabled" for any X - and what may be correct for one board will
not be correct for another.

Moreover, in the 88e1111 case on a SFP, what's right for one SFP is not
right for another - there are SFPs where the 88e1111 registers are
preloaded from an EEPROM, so whether bypass is enabled or not in SGMII
mode is up to the contents of the EEPROM - the marvell PHY driver does
not interfere with that setting for SGMII.

Hence, to report how the PHY will behave in SGMII mode, with lack of
explicit configuration, we _have_ to read registers and use them to
determine the outcome.

> > > If you implement just validate(), you should report just one
> > > bit, corresponding to what the hardware is configured for (so either
> > > PHY_AN_INBAND_ON, *or* PHY_AN_INBAND_TIMEOUT). This is because you'd
> > > otherwise tell phylink that 2 modes are supported, but provide no way to
> > > choose between them, and you don't make it clear which one is in use
> > > either. This will force phylink to adapt to MLO_AN_PHY or MLO_AN_INBAND,
> > > depending on what has a chance of working.
> > 
> > Don't we have the same problem with PHY_AN_INBAND_TIMEOUT? If a PHY
> > reports that, do we use MLO_AN_INBAND or MLO_AN_PHY?
> 
> Well, I haven't yet written any logic for it.
> 
> To your question: "PHY_AN_INBAND_ON_TIMEOUT => MLO_AN_PHY or MLO_AN_INBAND"?
> I'd say either depending on situation, since my expectation is that it's
> compatible with both.

Yes, it would be compatible with both.

> Always give preference to what's in the device tree if it can work
> somehow. If it can work in fully compatible modes (MLO_AN_PHY with
> PHY_AN_INBAND_OFF; MLO_AN_INBAND with PHY_AN_INBAND_ON), perfect.
> If not, but what's in the device tree can work with PHY_AN_INBAND_ON_TIMEOUT,
> also good => use ON_TIMEOUT.

What do we do for a SFP module with a Marvell PHY on - we need to cover
that in this thought process, especially as 88e1111 is one of the most
popular PHYs on Gigabit copper SFPs. We can't really say "whatever
DT/ACPI firmware says" because that's not relevant to SFPs (we always
override firmware for SFPs.)

> If what's in the device tree needs to be changed, it pretty much means
> that ON_TIMEOUT isn't supported by the PHY.
> 
> Concretely, I would propose something like this (a modification of the
> function added by the patch set, notice the extra "an_inband" argument,
> as well as the new checks for PHY_AN_INBAND_ON_TIMEOUT):
> 
> static void phylink_sync_an_inband(struct phylink *pl, struct phy_device *phy,
> 				   enum phy_an_inband *an_inband)
> {
> 	unsigned int mode = pl->cfg_link_an_mode;
> 	int ret;
> 
> 	if (!pl->config->sync_an_inband)
> 		return;
> 
> 	ret = phy_validate_an_inband(phy, pl->link_config.interface);
> 	if (ret == PHY_AN_INBAND_UNKNOWN) {
> 		phylink_dbg(pl,
> 			    "PHY driver does not report in-band autoneg capability, assuming %s\n",
> 			    phylink_autoneg_inband(mode) ? "true" : "false");
> 
> 		*an_inband = PHY_AN_INBAND_UNKNOWN;
> 	} else if (phylink_autoneg_inband(mode) &&
> 		   !(ret & PHY_AN_INBAND_ON) &&
> 		   !(ret & PHY_AN_INBAND_ON_TIMEOUT)) {
> 		phylink_err(pl,
> 			    "Requested in-band autoneg but driver does not support this, disabling it.\n");
> 
> 		mode = MLO_AN_PHY;
> 		*an_inband = PHY_AN_INBAND_OFF;
> 	} else if (!phylink_autoneg_inband(mode) &&
> 		   !(ret & PHY_AN_INBAND_OFF) &&
> 		   !(ret & PHY_AN_INBAND_ON_TIMEOUT)) {
> 		phylink_dbg(pl,
> 			    "PHY driver requests in-band autoneg, force-enabling it.\n");
> 
> 		mode = MLO_AN_INBAND;
> 		*an_inband = PHY_AN_INBAND_ON;
> 	} else {
> 		/* For the checks below, we've found a common operating
> 		 * mode with the PHY, just need to figure out if we
> 		 * agree fully or if we have to rely on the PHY's
> 		 * timeout ability
> 		 */
> 		if (phylink_autoneg_inband(mode)) {
> 			*an_inband = !!(ret & PHY_AN_INBAND_ON) ? PHY_AN_INBAND_ON :
> 					PHY_AN_INBAND_ON_TIMEOUT;
> 		} else {
> 			*an_inband = !!(ret & PHY_AN_INBAND_OFF) ? PHY_AN_INBAND_OFF :
> 					PHY_AN_INBAND_ON_TIMEOUT;
> 		}
> 	}
> 
> 	pl->cur_link_an_mode = mode;
> }
> 
> then call phy_config_an_inband() with "an_inband" as the mode to use.
> 
> As per Sean's feedback, we force the PHY to report at least one valid
> capability, otherwise, 0 is PHY_AN_INBAND_UNKNOWN and it's also treated
> correctly.
> 
> > > If you implement config_an_inband() too, then the validate procedure
> > > becomes a simple report of what can be configured for that PHY
> > > (OFF | ON | ON_TIMEOUT for 88E151x, and ON | ON_TIMEOUT for 88E1111).
> > > It's then the config_an_inband() procedure that applies to hardware the
> > > mode that is selected by phylink. From config_an_inband() you can return
> > > a negative error code on PHY I/O failure.
> > 
> > So it sounds like the decision about which mode to use needs to be
> > coupled with "does the PHY driver implement config_an_inband()"
> 
> So do you recommend that I should put a WARN_ON() somewhere, which
> asserts something like this?
> 
> "if the weight (number of bits set) in the return code of
> phy_validate_an_inband() is larger than 1, then phydev->drv->phy_config_an_inband()
> must be a non-NULL pointer, to allow selecting between them"

I think that would be a good idea, otherwise we are going to have to
rely on reviewers spotting this error, which given the way patches are
picked up on netdev, is prone to being missed. So, I think the more we
can encourage people to do it correctly first time, the better.

> > > If you can prepare some more formal patches for these PHYs for which I
> > > don't have documentation, I think I have a copper SFP module which uses
> > > SGMII and 88E1111, and I can plug it into the Honeycomb and see what
> > > happens.
> > 
> > I'm away from home at the moment, which means I don't have a way to
> > do any in-depth tests other than with the SFPs that are plugged into
> > my Honeycomb - which does include some copper SFPs but they're not
> > connected to anything. So I can't test to see if data passes until
> > I'm back home next week.
> 
> I actually meant that I can test on a Solidrun Honeycomb board that I
> happen to have access to, if you have some Marvell PHY code, even untested,
> that I could try out. I'm pretty much in the dark when it comes to their
> hardware documentation.

If we can agree on the reading-registers approach I suggested (with
the multi-bit return values corrected), then I can turn that into a
patch, but I think we need to come to agreement on that first.

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

Re: [PATCH v4 net-next 3/8] net: phy: bcm84881: move the in-band capability check where it belongs

[Vladimir Oltean]

On Tue, Nov 22, 2022 at 06:28:38PM +0000, Russell King (Oracle) wrote:
> On Tue, Nov 22, 2022 at 07:56:25PM +0200, Vladimir Oltean wrote:
> > The problem is not phy_config_an_inband() but phy_validate_an_inband().
> > We call that earlier than phy_attach_direct(), so if the PHY driver is
> > going to read a register from HW which hasn't yet been written, we get
> > an incorrect report of the current capabilities.
> 
> Why would it be "incorrect" ?
> 
> What the code I'm proposing correctly reports back what inband mode(s)
> will be in use should we select the proposed interface mode. Let's
> ignore whether we report the TIMEOUT or not for that statement, because
> I think that's confusing the discussion.
> 
> If we _do_ want to report whether the TIMEOUT mode is going to be used
> or not, the code I proposed is what will be necessary, because it
> depends on (a) how the PHY is strapped and (b) how firmware or external
> EEPROM has setup the device. If we want a single bit, then we would
> report just _ON_TIMEOUT if bypass is enabled - but we still need to
> read registers to come to a conclusion about whether it's enabled or
> not. As I say, we can't blindly say "if interface is X, then bypass
> will be enabled" for any X - and what may be correct for one board will
> not be correct for another.
> 
> Moreover, in the 88e1111 case on a SFP, what's right for one SFP is not
> right for another - there are SFPs where the 88e1111 registers are
> preloaded from an EEPROM, so whether bypass is enabled or not in SGMII
> mode is up to the contents of the EEPROM - the marvell PHY driver does
> not interfere with that setting for SGMII.
> 
> Hence, to report how the PHY will behave in SGMII mode, with lack of
> explicit configuration, we _have_ to read registers and use them to
> determine the outcome.
> 

I'll re-read this tomorrow to make sure I didn't miss something because
of being tired.

I may have mixed up interface modes in the validate() code for MV88E1111
that you posted. I was under the impression that PHY_AN_INBAND_TIMEOUT
always gets reported based on reading a hardware register, the same
hardware register that gets overwritten to MII_M1111_HWCFG_SERIAL_AN_BYPASS
in m88e1111_config_init_1000basex().

But your proposed code is actually a mix between reading the existing
hardware configuration for SGMII, and returning something hardcoded for
1000base-x. For 1000base-x, we will return PHY_AN_INBAND_TIMEOUT, not
because the hardware is currently configured like that, but because it
will be, later. And the timing of the validate() call isn't going to be
a problem, so there isn't a reason to move it.

I'm okay with that, I just didn't understand.

> > Always give preference to what's in the device tree if it can work
> > somehow. If it can work in fully compatible modes (MLO_AN_PHY with
> > PHY_AN_INBAND_OFF; MLO_AN_INBAND with PHY_AN_INBAND_ON), perfect.
> > If not, but what's in the device tree can work with PHY_AN_INBAND_ON_TIMEOUT,
> > also good => use ON_TIMEOUT.
> 
> What do we do for a SFP module with a Marvell PHY on - we need to cover
> that in this thought process, especially as 88e1111 is one of the most
> popular PHYs on Gigabit copper SFPs. We can't really say "whatever
> DT/ACPI firmware says" because that's not relevant to SFPs (we always
> override firmware for SFPs.)

Ok, I only answered to part of the question - which is how do we
interpret phy_validate_an_inband()'s result from phylink_sync_an_inband() -
the on-board PHY code path.

If the code path we're talking about is from phylink_sfp_config_phy(),
then the modified code, to account for TIMEOUT, would look like this:

	/* Select whether to operate in in-band mode or not, based on the
	 * capability of the PHY in the current link mode.
	 */
	ret = phy_validate_an_inband(phy, iface);
	if (ret == PHY_AN_INBAND_UNKNOWN) {
		mode = MLO_AN_INBAND;

		phylink_dbg(pl,
			    "PHY driver does not report in-band autoneg capability, assuming true\n");
	} else if (ret & (PHY_AN_INBAND_ON | PHY_AN_INBAND_ON_TIMEOUT)) {
		mode = MLO_AN_INBAND;
	} else {
		mode = MLO_AN_PHY;
	}

or in words, essentially prefer MLO_AN_INBAND except when the PHY driver
says that it requires in-band disabled.

At least that's for now, because we assume that the PCS always supports
MLO_AN_INBAND. For the purpose of this series, let's assume that's a given.

> > > > If you can prepare some more formal patches for these PHYs for which I
> > > > don't have documentation, I think I have a copper SFP module which uses
> > > > SGMII and 88E1111, and I can plug it into the Honeycomb and see what
> > > > happens.
> > > 
> > > I'm away from home at the moment, which means I don't have a way to
> > > do any in-depth tests other than with the SFPs that are plugged into
> > > my Honeycomb - which does include some copper SFPs but they're not
> > > connected to anything. So I can't test to see if data passes until
> > > I'm back home next week.
> > 
> > I actually meant that I can test on a Solidrun Honeycomb board that I
> > happen to have access to, if you have some Marvell PHY code, even untested,
> > that I could try out. I'm pretty much in the dark when it comes to their
> > hardware documentation.
> 
> If we can agree on the reading-registers approach I suggested (with
> the multi-bit return values corrected), then I can turn that into a
> patch, but I think we need to come to agreement on that first.

I think we're in agreement, but please let's wait until tomorrow, I need
to take a break for today.

Re: [PATCH v4 net-next 3/8] net: phy: bcm84881: move the in-band capability check where it belongs

[Russell King (Oracle)]

On Tue, Nov 22, 2022 at 09:36:59PM +0200, Vladimir Oltean wrote:
> I think we're in agreement, but please let's wait until tomorrow, I need
> to take a break for today.

I think we do have a sort of agreement... but lets give this a go. The
following should be sufficient for copper SFP modules using the 88E1111
PHY. However, I haven't build-tested this patch yet.

Reading through the documentation has brought up some worms in this
area. :(

It may be worth printing the fiber page BMCR and extsr at various
strategic points in this driver and reporting back if things don't
seem to be working right for your modules. In the mean time, I'll try
to see how the modules in the Honeycomb appear to be setup at power-up
and after the driver has configured the PHY... assuming I left both
MicroUSBs connected and the board has a network connection via the
main ethernet jack.

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 3c54d7d0f17f..9d537a2bccda 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -669,6 +669,54 @@ static int marvell_config_aneg_fiber(struct phy_device *phydev)
 	return genphy_check_and_restart_aneg(phydev, changed);
 }
 
+static int m88e1111_validate_an_inband(struct phy_device *phydev,
+				       phy_interface_t interface)
+{
+	int hwcfg_mode, extsr, bmcr;
+
+	if (interface != PHY_INTERFACE_MODE_1000BASEX &&
+	    interface != PHY_INTERFACE_MODE_SGMII)
+		return PHY_AN_INBAND_UNKNOWN;
+
+	extsr = phy_read(phydev, MII_M1111_PHY_EXT_SR);
+	if (extsr < 0)
+		return PHY_AN_INBAND_UNKNOWN;
+
+	hwcfg_mode = extsr & MII_M1111_HWCFG_MODE_MASK;
+
+	/* If we are in 1000base-X no-AN hwcfg_mode,
+	 * m88e1111_config_init_1000basex() will allegedly enable AN and
+	 * allow AN bypass - but there is a question over whether that is
+	 * actually the case. Let's follow what the comment says, and assume
+	 * that it was actually tested.
+	 */
+	if (interface == PHY_INTERFACE_MODE_1000BASEX &&
+	    hwcfg_mode == MII_M1111_HWCFG_MODE_COPPER_1000X_NOAN)
+		return PHY_AN_INBAND_ON_TIMEOUT;
+
+	/* Otherwise, we leave the AN enable bit and the AN bypass bit
+	 * alone, so we need to read the registers to determine how the
+	 * MAC facing side of the PHY has been setup by firmware and/or
+	 * hardware reset.
+	 *
+	 * If the AN enable bit is clear, then all in-band signalling
+	 * on the SGMII/1000base-X side is disabled. Otherwise, AN is
+	 * enabled. If the bypass bit is set, AN can complete without
+	 * a response from the partner (MAC).
+	 */
+	bmcr = phy_read_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR);
+	if (bmcr < 0)
+		return PHY_AN_INBAND_UNKNOWN;
+
+	if (!(bmcr & BMCR_ANENABLE))
+		return PHY_AN_INBAND_OFF;
+
+	if (extsr & MII_M1111_HWCFG_SERIAL_AN_BYPASS)
+		return PHY_AN_INBAND_ON_TIMEOUT;
+
+	return PHY_AN_INBAND_ON;
+}
+
 static int m88e1111_config_aneg(struct phy_device *phydev)
 {
 	int extsr = phy_read(phydev, MII_M1111_PHY_EXT_SR);
@@ -915,7 +963,10 @@ static int m88e1111_config_init_1000basex(struct phy_device *phydev)
 	if (extsr < 0)
 		return extsr;
 
-	/* If using copper mode, ensure 1000BaseX auto-negotiation is enabled */
+	/* If using copper mode, ensure 1000BaseX auto-negotiation is enabled.
+	 * FIXME: does this actually enable 1000BaseX auto-negotiation if it
+	 * was previously disabled in the Fiber BMCR? 2.3.1.6 suggests not!
+	 */
 	mode = extsr & MII_M1111_HWCFG_MODE_MASK;
 	if (mode == MII_M1111_HWCFG_MODE_COPPER_1000X_NOAN) {
 		err = phy_modify(phydev, MII_M1111_PHY_EXT_SR,
@@ -2997,6 +3048,7 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1111_get_tunable,
 		.set_tunable = m88e1111_set_tunable,
+		.validate_an_inband = m88e1111_validate_an_inband,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1111_FINISAR,

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

Re: [PATCH v4 net-next 3/8] net: phy: bcm84881: move the in-band capability check where it belongs

[Russell King (Oracle)]

On Wed, Nov 23, 2022 at 12:08:11PM +0000, Russell King (Oracle) wrote:
> On Tue, Nov 22, 2022 at 09:36:59PM +0200, Vladimir Oltean wrote:
> > I think we're in agreement, but please let's wait until tomorrow, I need
> > to take a break for today.
> 
> I think we do have a sort of agreement... but lets give this a go. The
> following should be sufficient for copper SFP modules using the 88E1111
> PHY. However, I haven't build-tested this patch yet.
> 
> Reading through the documentation has brought up some worms in this
> area. :(
> 
> It may be worth printing the fiber page BMCR and extsr at various
> strategic points in this driver and reporting back if things don't
> seem to be working right for your modules. In the mean time, I'll try
> to see how the modules in the Honeycomb appear to be setup at power-up
> and after the driver has configured the PHY... assuming I left both
> MicroUSBs connected and the board has a network connection via the
> main ethernet jack.

Unfortunately, I don't have a SFP with an 88e1111 plugged in, only the
bcm84881, so I can't test my patch remotely. However, it builds fine
when the appropriate TIMEOUT definition is added.

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

Re: [PATCH v4 net-next 3/8] net: phy: bcm84881: move the in-band capability check where it belongs

[Vladimir Oltean]

Hi Russell,

On Wed, Nov 23, 2022 at 01:11:23PM +0000, Russell King (Oracle) wrote:
> On Wed, Nov 23, 2022 at 12:08:11PM +0000, Russell King (Oracle) wrote:
> > On Tue, Nov 22, 2022 at 09:36:59PM +0200, Vladimir Oltean wrote:
> > > I think we're in agreement, but please let's wait until tomorrow, I need
> > > to take a break for today.
> > 
> > I think we do have a sort of agreement... but lets give this a go. The
> > following should be sufficient for copper SFP modules using the 88E1111
> > PHY. However, I haven't build-tested this patch yet.
> > 
> > Reading through the documentation has brought up some worms in this
> > area. :(
> > 
> > It may be worth printing the fiber page BMCR and extsr at various
> > strategic points in this driver and reporting back if things don't
> > seem to be working right for your modules. In the mean time, I'll try
> > to see how the modules in the Honeycomb appear to be setup at power-up
> > and after the driver has configured the PHY... assuming I left both
> > MicroUSBs connected and the board has a network connection via the
> > main ethernet jack.
> 
> Unfortunately, I don't have a SFP with an 88e1111 plugged in, only the
> bcm84881, so I can't test my patch remotely. However, it builds fine
> when the appropriate TIMEOUT definition is added.

Sorry for the delay. Had to do something else yesterday and the day before.

I tested the patch and it does detect the operating mode of my PHY.

My modules are these:

[    6.465788] sfp sfp-0: module UBNT  UF-RJ45-1G  rev 1.0  sn X20072804742  dc 200617
ethtool -m dpmac7
        Identifier              : 0x03 (SFP)
        Extended identifier     : 0x04 (GBIC/SFP defined by 2-wire interface ID)
        Connector               : 0x00 (unknown or unspecified)
        Transceiver codes       : 0x00 0x00 0x00 0x08 0x00 0x00 0x00 0x00 0x00
        Transceiver type        : Ethernet: 1000BASE-T
        Encoding                : 0x01 (8B/10B)
        BR, Nominal             : 1300MBd
        Rate identifier         : 0x00 (unspecified)
        Length (SMF,km)         : 0km
        Length (SMF)            : 0m
        Length (50um)           : 0m
        Length (62.5um)         : 0m
        Length (Copper)         : 100m
        Length (OM3)            : 0m
        Laser wavelength        : 0nm
        Vendor name             : UBNT
        Vendor OUI              : 00:00:00
        Vendor PN               : UF-RJ45-1G
        Vendor rev              : 1.0
        Option values           : 0x00 0x1a
        Option                  : RX_LOS implemented
        Option                  : TX_FAULT implemented
        Option                  : TX_DISABLE implemented
        BR margin, max          : 0%
        BR margin, min          : 0%
        Vendor SN               : X20072804742
        Date code               : 200617

Here is how the PHY driver does a few things:

[ 3079.596985] fsl_dpaa2_eth dpni.1 dpmac7: configuring for inband/sgmii link mode
[ 3079.689892] fsl_dpaa2_eth dpni.1 dpmac7: PHY driver reported AN inband 0x4 // PHY_AN_INBAND_ON_TIMEOUT
[ 3079.696826] fsl_dpaa2_eth dpni.1 dpmac7: switched to phy/sgmii link mode
[ 3079.779656] Marvell 88E1111 i2c:sfp-0:16: m88e1111_config_init_sgmii: EXT_SR before 0x9088 after 0x9084, fiber page BMCR 0x1140
[ 3079.865386] fsl_dpaa2_eth dpni.1 dpmac7: PHY [i2c:sfp-0:16] driver [Marvell 88E1111] (irq=POLL)

So the default EXT_SR is being changed by the PHY driver from 0x9088 to
0x9084 (MII_M1111_HWCFG_MODE_COPPER_1000X_AN -> MII_M1111_HWCFG_MODE_SGMII_NO_CLK).

I don't know if it's possible to force these modules to operate in
1000BASE-X mode. If you're interested in the results there, please give
me some guidance.

I was curious if the fiber page BMCR has an effect for in-band autoneg,
and at least for SGMII it surely does. If I add this to m88e1111_config_init_sgmii():

	phy_modify_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR,
			 BMCR_ANENABLE, 0);

(and force an intentional mismatch) then I am able to break the link
with my Lynx PCS.

If my hunch is correct that this also holds true for 1000BASE-X, then
you are also correct that m88e1111_config_init_1000basex() only allows
AN bypass but does not seem to enable in-band AN in itself, if it wasn't
enabled.

The implication here is that maybe we should test for the fiber page
BMCR in both SGMII and 1000BASE-X modes?

Should we call m88e1111_validate_an_inband() also for the Finisar
variant of the 88E1111? What about 88E1112?

Re: [PATCH v4 net-next 3/8] net: phy: bcm84881: move the in-band capability check where it belongs

[Russell King (Oracle)]

Hi Vladimir,

On Fri, Nov 25, 2022 at 02:30:42PM +0200, Vladimir Oltean wrote:
> Hi Russell,
> 
> Sorry for the delay. Had to do something else yesterday and the day before.

I think there was some kind of celebration going on in the US for at
least one of those days...

> I tested the patch and it does detect the operating mode of my PHY.

Thanks for testing.

> My modules are these:
> 
> [    6.465788] sfp sfp-0: module UBNT  UF-RJ45-1G  rev 1.0  sn X20072804742  dc 200617
> ethtool -m dpmac7
>         Identifier              : 0x03 (SFP)
>         Extended identifier     : 0x04 (GBIC/SFP defined by 2-wire interface ID)
>         Connector               : 0x00 (unknown or unspecified)
>         Transceiver codes       : 0x00 0x00 0x00 0x08 0x00 0x00 0x00 0x00 0x00
>         Transceiver type        : Ethernet: 1000BASE-T
>         Encoding                : 0x01 (8B/10B)
>         BR, Nominal             : 1300MBd
>         Rate identifier         : 0x00 (unspecified)
>         Length (SMF,km)         : 0km
>         Length (SMF)            : 0m
>         Length (50um)           : 0m
>         Length (62.5um)         : 0m
>         Length (Copper)         : 100m
>         Length (OM3)            : 0m
>         Laser wavelength        : 0nm
>         Vendor name             : UBNT
>         Vendor OUI              : 00:00:00
>         Vendor PN               : UF-RJ45-1G
>         Vendor rev              : 1.0
>         Option values           : 0x00 0x1a
>         Option                  : RX_LOS implemented
>         Option                  : TX_FAULT implemented
>         Option                  : TX_DISABLE implemented
>         BR margin, max          : 0%
>         BR margin, min          : 0%
>         Vendor SN               : X20072804742
>         Date code               : 200617
> 
> Here is how the PHY driver does a few things:
> 
> [ 3079.596985] fsl_dpaa2_eth dpni.1 dpmac7: configuring for inband/sgmii link mode
> [ 3079.689892] fsl_dpaa2_eth dpni.1 dpmac7: PHY driver reported AN inband 0x4 // PHY_AN_INBAND_ON_TIMEOUT
> [ 3079.696826] fsl_dpaa2_eth dpni.1 dpmac7: switched to phy/sgmii link mode
> [ 3079.779656] Marvell 88E1111 i2c:sfp-0:16: m88e1111_config_init_sgmii: EXT_SR before 0x9088 after 0x9084, fiber page BMCR 0x1140
> [ 3079.865386] fsl_dpaa2_eth dpni.1 dpmac7: PHY [i2c:sfp-0:16] driver [Marvell 88E1111] (irq=POLL)
> 
> So the default EXT_SR is being changed by the PHY driver from 0x9088 to
> 0x9084 (MII_M1111_HWCFG_MODE_COPPER_1000X_AN -> MII_M1111_HWCFG_MODE_SGMII_NO_CLK).
> 
> I don't know if it's possible to force these modules to operate in
> 1000BASE-X mode. If you're interested in the results there, please give
> me some guidance.

The value of "EXT_SR before" is 1000base-X, so if you change sfp-bus.c::
sfp_select_interface() to use 1000BASEX instead of SGMII then you'll be
using 1000BASEX instead (and it should work, although at fixed 1G
speeds). The only reason the module is working in SGMII mode is because,
as you've noticed above, we switch it to SGMII mode in
m88e1111_config_init_sgmii().

> I was curious if the fiber page BMCR has an effect for in-band autoneg,
> and at least for SGMII it surely does. If I add this to
> m88e1111_config_init_sgmii():
> 
> 	phy_modify_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR,
> 			 BMCR_ANENABLE, 0);
> 
> (and force an intentional mismatch) then I am able to break the link
> with my Lynx PCS.

Yes, the fiber page is re-used for the host side of the link when
operating in SGMII and 1000baseX modes, so changes there have the
expected effect.

> If my hunch is correct that this also holds true for 1000BASE-X, then
> you are also correct that m88e1111_config_init_1000basex() only allows
> AN bypass but does not seem to enable in-band AN in itself, if it wasn't
> enabled.
> 
> The implication here is that maybe we should test for the fiber page
> BMCR in both SGMII and 1000BASE-X modes?

I think a more comprehensive test would be to write the fiber page
BMCR with 0x140 before changing the mode from 1000baseX to SGMII and
see whether the BMCR changes value. My suspicion is it won't, and
the hwcfg_mode only has an effect on the settings in the fiber page
under hardware reset conditions, and mode changes have no effect on
the fiber page.

If that is correct, then I think we have two options:
1. make m88e1111_config_init_1000basex() actually do what the comment
   says, and enable AN in the fiber page (risky). That would make it
   conform with how I implemented the validate_an_inband() function.
2. update the comment in m88e1111_config_init_1000basex() to reflect
   what's actually happening with the hardware, and make
   validate_an_inband() read the fiber page BMCR for 1000base-X too.

> Should we call m88e1111_validate_an_inband() also for the Finisar
> variant of the 88E1111? What about 88E1112?

Entirely probable - this patch is minimalist in order for your tests,
I didn't bother with the 151x devices. I'll prepare a more
comprehensive patch next week.

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

Re: [PATCH v4 net-next 3/8] net: phy: bcm84881: move the in-band capability check where it belongs

[Vladimir Oltean]

On Fri, Nov 25, 2022 at 01:43:34PM +0000, Russell King (Oracle) wrote:
> Hi Vladimir,
> 
> On Fri, Nov 25, 2022 at 02:30:42PM +0200, Vladimir Oltean wrote:
> > Hi Russell,
> > 
> > Sorry for the delay. Had to do something else yesterday and the day before.
> 
> I think there was some kind of celebration going on in the US for at
> least one of those days...

Yeah, but I don't celebrate that. I had to write some documentation.

> > So the default EXT_SR is being changed by the PHY driver from 0x9088 to
> > 0x9084 (MII_M1111_HWCFG_MODE_COPPER_1000X_AN -> MII_M1111_HWCFG_MODE_SGMII_NO_CLK).
> > 
> > I don't know if it's possible to force these modules to operate in
> > 1000BASE-X mode. If you're interested in the results there, please give
> > me some guidance.
> 
> The value of "EXT_SR before" is 1000base-X, so if you change sfp-bus.c::
> sfp_select_interface() to use 1000BASEX instead of SGMII then you'll be
> using 1000BASEX instead (and it should work, although at fixed 1G
> speeds). The only reason the module is working in SGMII mode is because,
> as you've noticed above, we switch it to SGMII mode in
> m88e1111_config_init_sgmii().
> 

Which is an interesting thing, because m88e1111_config_init_1000basex()
does not change the HWCFG_MODE_MASK to something with 1000X in it.

Anyway, with sfp_select_interface() hacked, I can confirm it works in
1000BASE-X with AN enabled too.

[   69.746643] fsl_dpaa2_eth dpni.1 dpmac7: configuring for inband/sgmii link mode
[   69.845784] fsl_dpaa2_eth dpni.1 dpmac7: PHY driver reported AN inband 0x4 // PHY_AN_INBAND_ON_TIMEOUT
[   69.852764] fsl_dpaa2_eth dpni.1 dpmac7: switched to inband/1000base-x link mode // MLO_AN_INBAND
[   69.934191] Marvell 88E1111 i2c:sfp-0:16: m88e1111_config_init_1000basex: EXT_SR before 0x9088 after 0x9088, fiber page BMCR before 0x1140 after 0x1140
[   70.015735] fsl_dpaa2_eth dpni.1 dpmac7: PHY [i2c:sfp-0:16] driver [Marvell 88E1111] (irq=POLL)

ping 192.168.100.2
PING 192.168.100.2 (192.168.100.2) 56(84) bytes of data.
64 bytes from 192.168.100.2: icmp_seq=1 ttl=64 time=0.874 ms
64 bytes from 192.168.100.2: icmp_seq=2 ttl=64 time=0.225 ms
64 bytes from 192.168.100.2: icmp_seq=3 ttl=64 time=0.216 ms
^C
--- 192.168.100.2 ping statistics ---
3 packets transmitted, 3 received, 0% packet loss, time 2019ms
rtt min/avg/max/mdev = 0.216/0.438/0.874/0.308 ms

printed with code:

static int m88e1111_config_init_1000basex(struct phy_device *phydev)
{
	int extsr = phy_read(phydev, MII_M1111_PHY_EXT_SR);
	int fiber_bmcr_before, fiber_bmcr_after;
	int ext_sr_after;
	int err, mode;

	if (extsr < 0)
		return extsr;

	fiber_bmcr_before = phy_read_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR);
	if (fiber_bmcr_before < 0)
		return fiber_bmcr_before;

	/* If using copper mode, ensure 1000BaseX auto-negotiation is enabled.
	 * FIXME: does this actually enable 1000BaseX auto-negotiation if it
	 * was previously disabled in the Fiber BMCR? 2.3.1.6 suggests not!
	 */
	mode = extsr & MII_M1111_HWCFG_MODE_MASK;
	if (mode == MII_M1111_HWCFG_MODE_COPPER_1000X_NOAN) {
		err = phy_modify(phydev, MII_M1111_PHY_EXT_SR,
				 MII_M1111_HWCFG_MODE_MASK |
				 MII_M1111_HWCFG_SERIAL_AN_BYPASS,
				 MII_M1111_HWCFG_MODE_COPPER_1000X_AN |
				 MII_M1111_HWCFG_SERIAL_AN_BYPASS);
		if (err < 0)
			return err;
	}

	ext_sr_after = phy_read(phydev, MII_M1111_PHY_EXT_SR);
	if (ext_sr_after < 0)
		return ext_sr_after;

	fiber_bmcr_after = phy_read_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR);
	if (fiber_bmcr_after < 0)
		return fiber_bmcr_after;

	phydev_err(phydev, "%s: EXT_SR before 0x%x after 0x%x, fiber page BMCR before 0x%x after 0x%x\n",
		   __func__, extsr, ext_sr_after,
		   fiber_bmcr_before, fiber_bmcr_after);
	return 0;
}

Furthermore, I can confirm that if the fiber page BMCR has BMCR_ANENABLE
disabled, the link with my Lynx PCS in MLO_AN_INBAND is broken (and that
the write to EXT_SR doesn't change the value of the BMCR).



But there's actually a problem (or maybe two problems).

First is that if I make phylink treat the ON_TIMEOUT capability by using
MLO_AN_PHY (basically like this):

phylink_sfp_config_phy():

	/* Select whether to operate in in-band mode or not, based on the
	 * capability of the PHY in the current link mode.
	 */
	ret = phy_validate_an_inband(phy, iface);
	phylink_err(pl, "PHY driver reported AN inband 0x%x\n", ret);
	if (ret == PHY_AN_INBAND_UNKNOWN) {
		mode = MLO_AN_INBAND;

		phylink_dbg(pl,
			    "PHY driver does not report in-band autoneg capability, assuming true\n");
//	} else if (ret & (PHY_AN_INBAND_ON | PHY_AN_INBAND_ON_TIMEOUT)) {
	} else if (ret & PHY_AN_INBAND_ON) {
		mode = MLO_AN_INBAND;
	} else {
		mode = MLO_AN_PHY;
	}

[   30.059923] fsl_dpaa2_eth dpni.1 dpmac7: PHY driver reported AN inband 0x4 // PHY_AN_INBAND_ON_TIMEOUT
[   30.066867] fsl_dpaa2_eth dpni.1 dpmac7: switched to phy/1000base-x link mode // MLO_AN_PHY
[   30.153350] Marvell 88E1111 i2c:sfp-0:16: m88e1111_config_init_1000basex: EXT_SR before 0x9088 after 0x9088, fiber page BMCR before 0x1140 after 0x1140
[   30.238970] fsl_dpaa2_eth dpni.1 dpmac7: PHY [i2c:sfp-0:16] driver [Marvell 88E1111] (irq=POLL)

then pinging is broken with mismatched in-band AN settings ("TIMEOUT" in
PHY, "OFF" in PCS). I triple-checked this.

ping 192.168.100.2
PING 192.168.100.2 (192.168.100.2) 56(84) bytes of data.
From 192.168.100.1 icmp_seq=1 Destination Host Unreachable
From 192.168.100.1 icmp_seq=2 Destination Host Unreachable
From 192.168.100.1 icmp_seq=3 Destination Host Unreachable
From 192.168.100.1 icmp_seq=4 Destination Host Unreachable
From 192.168.100.1 icmp_seq=5 Destination Host Unreachable
From 192.168.100.1 icmp_seq=6 Destination Host Unreachable
^C
--- 192.168.100.2 ping statistics ---
9 packets transmitted, 0 received, +6 errors, 100% packet loss, time 8170ms


However, if using the same phylink code (to force a mismatch), I unhack
sfp_select_interface() and use SGMII mode, the timeout feature does
actually work:

[   30.262979] fsl_dpaa2_eth dpni.1 dpmac7: PHY driver reported AN inband 0x4 // PHY_AN_INBAND_ON_TIMEOUT
[   30.270349] fsl_dpaa2_eth dpni.1 dpmac7: switched to phy/sgmii link mode // MLO_AN_PHY
[   30.351066] Marvell 88E1111 i2c:sfp-0:16: m88e1111_config_init_sgmii: EXT_SR before 0x9088 after 0x9084, fiber page BMCR before 0x1140 after 0x1140
[   30.433236] fsl_dpaa2_eth dpni.1 dpmac7: PHY [i2c:sfp-0:16] driver [Marvell 88E1111] (irq=POLL)

this is a functional link despite the mismatched settings.

ping 192.168.100.2
PING 192.168.100.2 (192.168.100.2) 56(84) bytes of data.
64 bytes from 192.168.100.2: icmp_seq=1 ttl=64 time=0.885 ms
64 bytes from 192.168.100.2: icmp_seq=2 ttl=64 time=0.221 ms
64 bytes from 192.168.100.2: icmp_seq=3 ttl=64 time=0.216 ms
64 bytes from 192.168.100.2: icmp_seq=4 ttl=64 time=0.217 ms
64 bytes from 192.168.100.2: icmp_seq=5 ttl=64 time=0.238 ms
^C
--- 192.168.100.2 ping statistics ---
5 packets transmitted, 5 received, 0% packet loss, time 4062ms
rtt min/avg/max/mdev = 0.216/0.355/0.885/0.264 ms


The second problem is that not even *matched* settings work if I turn
off BMCR_ANENABLE in the PHY fiber page.

[   30.809869] fsl_dpaa2_eth dpni.1 dpmac7: configuring for inband/sgmii link mode
[   30.817936] mdio_bus 0x0000000008c1f000:00: MII_BMCR 0x1140 MII_BMSR 0x9 MII_ADVERTISE 0x1 MII_LPA 0x0 IF_MODE 0x3 // PCS registers at the end of lynx_pcs_config_giga()
[   30.917651] fsl_dpaa2_eth dpni.1 dpmac7: PHY driver reported AN inband 0x4 // ignore; m88e1111_validate_an_inband() is hardcoded for this and does not detect BMCR for BASE-X
[   30.924571] fsl_dpaa2_eth dpni.1 dpmac7: switched to phy/1000base-x link mode
[   30.932441] mdio_bus 0x0000000008c1f000:00: MII_BMCR 0x140 MII_BMSR 0xd MII_ADVERTISE 0x1 MII_LPA 0x0 IF_MODE 0x1
[   31.032547] Marvell 88E1111 i2c:sfp-0:16: m88e1111_config_init_1000basex: EXT_SR before 0x9088 after 0x9088, fiber page BMCR before 0x140 after 0x140
[   31.117668] fsl_dpaa2_eth dpni.1 dpmac7: PHY [i2c:sfp-0:16] driver [Marvell 88E1111] (irq=POLL)

ping 192.168.100.2
PING 192.168.100.2 (192.168.100.2) 56(84) bytes of data.
^C
--- 192.168.100.2 ping statistics ---
4 packets transmitted, 0 received, 100% packet loss, time 3058ms

What's common is that if in-band autoneg is turned off (either forced
off or via timeout), 1000BASE-X between the Lynx PCS and the 88E1111
simply doesn't work.

> > I was curious if the fiber page BMCR has an effect for in-band autoneg,
> > and at least for SGMII it surely does. If I add this to
> > m88e1111_config_init_sgmii():
> > 
> > 	phy_modify_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR,
> > 			 BMCR_ANENABLE, 0);
> > 
> > (and force an intentional mismatch) then I am able to break the link
> > with my Lynx PCS.
> 
> Yes, the fiber page is re-used for the host side of the link when
> operating in SGMII and 1000baseX modes, so changes there have the
> expected effect.
> 
> > If my hunch is correct that this also holds true for 1000BASE-X, then
> > you are also correct that m88e1111_config_init_1000basex() only allows
> > AN bypass but does not seem to enable in-band AN in itself, if it wasn't
> > enabled.
> > 
> > The implication here is that maybe we should test for the fiber page
> > BMCR in both SGMII and 1000BASE-X modes?
> 
> I think a more comprehensive test would be to write the fiber page
> BMCR with 0x140 before changing the mode from 1000baseX to SGMII and
> see whether the BMCR changes value. My suspicion is it won't, and
> the hwcfg_mode only has an effect on the settings in the fiber page
> under hardware reset conditions, and mode changes have no effect on
> the fiber page.

Confirmed that changes to the EXT_SR register don't cause changes to the
MII_BMCR register:

[   28.587838] Marvell 88E1111 i2c:sfp-0:16: m88e1111_config_init_sgmii: EXT_SR before 0x9088 after 0x9084, fiber page BMCR before 0x140 after 0x140

generated by:

static int m88e1111_config_init_sgmii(struct phy_device *phydev)
{
	int fiber_bmcr_before, fiber_bmcr_after;
	int ext_sr_before, ext_sr_after;
	int err;

	ext_sr_before = phy_read(phydev, MII_M1111_PHY_EXT_SR);
	if (ext_sr_before < 0)
		return ext_sr_before;

	err = phy_modify_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR,
			       BMCR_ANENABLE, 0);
	if (err < 0)
		return err;

	fiber_bmcr_before = phy_read_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR);
	if (fiber_bmcr_before < 0)
		return fiber_bmcr_before;

	err = m88e1111_config_init_hwcfg_mode(
		phydev,
		MII_M1111_HWCFG_MODE_SGMII_NO_CLK,
		MII_M1111_HWCFG_FIBER_COPPER_AUTO);
	if (err < 0)
		return err;

	ext_sr_after = phy_read(phydev, MII_M1111_PHY_EXT_SR);
	if (ext_sr_after < 0)
		return ext_sr_after;

	fiber_bmcr_after = phy_read_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR);
	if (fiber_bmcr_after < 0)
		return fiber_bmcr_after;

	phydev_err(phydev, "%s: EXT_SR before 0x%x after 0x%x, fiber page BMCR before 0x%x after 0x%x\n",
		   __func__, ext_sr_before, ext_sr_after,
		   fiber_bmcr_before, fiber_bmcr_after);

	/* make sure copper is selected */
	return marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
}

Re: [PATCH v4 net-next 3/8] net: phy: bcm84881: move the in-band capability check where it belongs

[Russell King (Oracle)]

On Fri, Nov 25, 2022 at 05:35:55PM +0200, Vladimir Oltean wrote:
> On Fri, Nov 25, 2022 at 01:43:34PM +0000, Russell King (Oracle) wrote:
> > The value of "EXT_SR before" is 1000base-X, so if you change sfp-bus.c::
> > sfp_select_interface() to use 1000BASEX instead of SGMII then you'll be
> > using 1000BASEX instead (and it should work, although at fixed 1G
> > speeds). The only reason the module is working in SGMII mode is because,
> > as you've noticed above, we switch it to SGMII mode in
> > m88e1111_config_init_sgmii().
> 
> Which is an interesting thing, because m88e1111_config_init_1000basex()
> does not change the HWCFG_MODE_MASK to something with 1000X in it.

It only changes the hwcfg mode if it was using 1000base-X no-AN -
switching it instead to be 1000base-X with AN, but as we've established
the comment above that code describes something which doesn't happen,
as the fibre page BMCR is unaffected by this change.

Anyway, with my SourcePhotonics SPGBTXCNFC module (which is a SGMII
module) I get:

Marvell 88E1111 i2c:sfp-3:16: extsr: 8084 fiber bmcr: 1140

although the first time I plugged it in, BMCR was 1940 (pdown set).
Key thing is this module doesn't have bypass permitted.

> But there's actually a problem (or maybe two problems).
> 
> First is that if I make phylink treat the ON_TIMEOUT capability by using
> MLO_AN_PHY (basically like this):
> 
> phylink_sfp_config_phy():
> 
> 	/* Select whether to operate in in-band mode or not, based on the
> 	 * capability of the PHY in the current link mode.
> 	 */
> 	ret = phy_validate_an_inband(phy, iface);
> 	phylink_err(pl, "PHY driver reported AN inband 0x%x\n", ret);
> 	if (ret == PHY_AN_INBAND_UNKNOWN) {
> 		mode = MLO_AN_INBAND;
> 
> 		phylink_dbg(pl,
> 			    "PHY driver does not report in-band autoneg capability, assuming true\n");
> //	} else if (ret & (PHY_AN_INBAND_ON | PHY_AN_INBAND_ON_TIMEOUT)) {
> 	} else if (ret & PHY_AN_INBAND_ON) {
> 		mode = MLO_AN_INBAND;
> 	} else {
> 		mode = MLO_AN_PHY;
> 	}
> 
> [   30.059923] fsl_dpaa2_eth dpni.1 dpmac7: PHY driver reported AN inband 0x4 // PHY_AN_INBAND_ON_TIMEOUT
> [   30.066867] fsl_dpaa2_eth dpni.1 dpmac7: switched to phy/1000base-x link mode // MLO_AN_PHY
> [   30.153350] Marvell 88E1111 i2c:sfp-0:16: m88e1111_config_init_1000basex: EXT_SR before 0x9088 after 0x9088, fiber page BMCR before 0x1140 after 0x1140
> [   30.238970] fsl_dpaa2_eth dpni.1 dpmac7: PHY [i2c:sfp-0:16] driver [Marvell 88E1111] (irq=POLL)
> 
> then pinging is broken with mismatched in-band AN settings ("TIMEOUT" in
> PHY, "OFF" in PCS). I triple-checked this.
> 
> ping 192.168.100.2
> PING 192.168.100.2 (192.168.100.2) 56(84) bytes of data.
> From 192.168.100.1 icmp_seq=1 Destination Host Unreachable
> From 192.168.100.1 icmp_seq=2 Destination Host Unreachable
> From 192.168.100.1 icmp_seq=3 Destination Host Unreachable
> From 192.168.100.1 icmp_seq=4 Destination Host Unreachable
> From 192.168.100.1 icmp_seq=5 Destination Host Unreachable
> From 192.168.100.1 icmp_seq=6 Destination Host Unreachable
> ^C
> --- 192.168.100.2 ping statistics ---
> 9 packets transmitted, 0 received, +6 errors, 100% packet loss, time 8170ms
> 
> 
> However, if using the same phylink code (to force a mismatch), I unhack
> sfp_select_interface() and use SGMII mode, the timeout feature does
> actually work:
> 
> [   30.262979] fsl_dpaa2_eth dpni.1 dpmac7: PHY driver reported AN inband 0x4 // PHY_AN_INBAND_ON_TIMEOUT
> [   30.270349] fsl_dpaa2_eth dpni.1 dpmac7: switched to phy/sgmii link mode // MLO_AN_PHY
> [   30.351066] Marvell 88E1111 i2c:sfp-0:16: m88e1111_config_init_sgmii: EXT_SR before 0x9088 after 0x9084, fiber page BMCR before 0x1140 after 0x1140
> [   30.433236] fsl_dpaa2_eth dpni.1 dpmac7: PHY [i2c:sfp-0:16] driver [Marvell 88E1111] (irq=POLL)
> 
> this is a functional link despite the mismatched settings.
> 
> ping 192.168.100.2
> PING 192.168.100.2 (192.168.100.2) 56(84) bytes of data.
> 64 bytes from 192.168.100.2: icmp_seq=1 ttl=64 time=0.885 ms
> 64 bytes from 192.168.100.2: icmp_seq=2 ttl=64 time=0.221 ms
> 64 bytes from 192.168.100.2: icmp_seq=3 ttl=64 time=0.216 ms
> 64 bytes from 192.168.100.2: icmp_seq=4 ttl=64 time=0.217 ms
> 64 bytes from 192.168.100.2: icmp_seq=5 ttl=64 time=0.238 ms
> ^C
> --- 192.168.100.2 ping statistics ---
> 5 packets transmitted, 5 received, 0% packet loss, time 4062ms
> rtt min/avg/max/mdev = 0.216/0.355/0.885/0.264 ms
> 
> 
> The second problem is that not even *matched* settings work if I turn
> off BMCR_ANENABLE in the PHY fiber page.
> 
> [   30.809869] fsl_dpaa2_eth dpni.1 dpmac7: configuring for inband/sgmii link mode
> [   30.817936] mdio_bus 0x0000000008c1f000:00: MII_BMCR 0x1140 MII_BMSR 0x9 MII_ADVERTISE 0x1 MII_LPA 0x0 IF_MODE 0x3 // PCS registers at the end of lynx_pcs_config_giga()
> [   30.917651] fsl_dpaa2_eth dpni.1 dpmac7: PHY driver reported AN inband 0x4 // ignore; m88e1111_validate_an_inband() is hardcoded for this and does not detect BMCR for BASE-X
> [   30.924571] fsl_dpaa2_eth dpni.1 dpmac7: switched to phy/1000base-x link mode
> [   30.932441] mdio_bus 0x0000000008c1f000:00: MII_BMCR 0x140 MII_BMSR 0xd MII_ADVERTISE 0x1 MII_LPA 0x0 IF_MODE 0x1
> [   31.032547] Marvell 88E1111 i2c:sfp-0:16: m88e1111_config_init_1000basex: EXT_SR before 0x9088 after 0x9088, fiber page BMCR before 0x140 after 0x140
> [   31.117668] fsl_dpaa2_eth dpni.1 dpmac7: PHY [i2c:sfp-0:16] driver [Marvell 88E1111] (irq=POLL)
> 
> ping 192.168.100.2
> PING 192.168.100.2 (192.168.100.2) 56(84) bytes of data.
> ^C
> --- 192.168.100.2 ping statistics ---
> 4 packets transmitted, 0 received, 100% packet loss, time 3058ms
> 
> What's common is that if in-band autoneg is turned off (either forced
> off or via timeout), 1000BASE-X between the Lynx PCS and the 88E1111
> simply doesn't work.

I've just tried an experiment here with my SourcePhotonics module.

I made m88e1111_validate_an_inband() set the SERIAL_AN_BYPASS bit,
and then the bit I think you're probably unaware of - the PHY needs
to be soft-reset in order for that change to take effect. Calling
genphy_soft_reset() is sufficient.

Then I made m88e1111_validate_an_inband() return PHY_AN_INBAND_OFF.
So we now have the PHY setup with BMCR=1140 and EXTSR=9084.

# ping -I eth4 fe80::222:68ff:fe15:37dd
ping: Warning: source address might be selected on device other than: eth4
PING fe80::222:68ff:fe15:37dd(fe80::222:68ff:fe15:37dd) from :: eth4: 56 data bytes
64 bytes from fe80::222:68ff:fe15:37dd%eth4: icmp_seq=1 ttl=64 time=0.281 ms
^C
--- fe80::222:68ff:fe15:37dd ping statistics ---
1 packets transmitted, 1 received, 0% packet loss, time 0ms
rtt min/avg/max/mdev = 0.281/0.281/0.281/0.000 ms

(yes, it did use eth4's source address, I checked with tcpdump on the
target machine.)

So the link appears to be functional. Using a highly modified mii-diag
tool that allows me to read/write registers on the PHY, if I read the
EXT_SR register, it now contains:

Reading 0x001b=0x9884

and bit 11 being set means the PHY went into bypass mode. In other
words, it didn't see the SGMII acknowledgement from the MAC and
decided to bring the link up in bypass mode.

However, I've just tripped over some information in the 88E1111
manual which states that in SGMII mode, if bypass mode is used, then
the PHY will apparently renegotiate on the copper side advertising
1000baseT HD and FD only, no pause. So I checked what my link partner
is seeing, and it was seeing the original advertisement.

So I then triggered a renegotiate from the partner, and it now shows
only 1000baseT/Half 1000baseT/Full being advertised by the 88E1111.
Reading the advertisement register, it still contains 0x0d41, which
shows pause modes, 100FD, 10FD - so the advertisement register doesn't
reflect what was actually adfertised in this case.

Also, presumably, based on this observation, it will only renegotiate
if the copper side hadn't resolved to gigabit. If correct, what this
means is that when operating in SGMII mode, the the PHY becomes
gigabit-only if bypass mode gets used.

Given this behaviour, the fact that it switches to gigabit only when
the SGMII side enters bypass mode, I think we should _positively_ be
disabling inband bypass in SGMII mode. This change in advertisement
is not what phylib would expect, and I suspect could lead to surprises
e.g. if phylib was told to advertise non-gigabit speeds only.

However, I'll try this test with 1000base-X mode tomorrow.

> > I think a more comprehensive test would be to write the fiber page
> > BMCR with 0x140 before changing the mode from 1000baseX to SGMII and
> > see whether the BMCR changes value. My suspicion is it won't, and
> > the hwcfg_mode only has an effect on the settings in the fiber page
> > under hardware reset conditions, and mode changes have no effect on
> > the fiber page.
> 
> Confirmed that changes to the EXT_SR register don't cause changes to the
> MII_BMCR register:
> 
> [   28.587838] Marvell 88E1111 i2c:sfp-0:16: m88e1111_config_init_sgmii: EXT_SR before 0x9088 after 0x9084, fiber page BMCR before 0x140 after 0x140
> 
> generated by:
> 
> static int m88e1111_config_init_sgmii(struct phy_device *phydev)
> {
> 	int fiber_bmcr_before, fiber_bmcr_after;
> 	int ext_sr_before, ext_sr_after;
> 	int err;
> 
> 	ext_sr_before = phy_read(phydev, MII_M1111_PHY_EXT_SR);
> 	if (ext_sr_before < 0)
> 		return ext_sr_before;
> 
> 	err = phy_modify_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR,
> 			       BMCR_ANENABLE, 0);
> 	if (err < 0)
> 		return err;
> 
> 	fiber_bmcr_before = phy_read_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR);
> 	if (fiber_bmcr_before < 0)
> 		return fiber_bmcr_before;
> 
> 	err = m88e1111_config_init_hwcfg_mode(
> 		phydev,
> 		MII_M1111_HWCFG_MODE_SGMII_NO_CLK,
> 		MII_M1111_HWCFG_FIBER_COPPER_AUTO);
> 	if (err < 0)
> 		return err;
> 
> 	ext_sr_after = phy_read(phydev, MII_M1111_PHY_EXT_SR);
> 	if (ext_sr_after < 0)
> 		return ext_sr_after;
> 
> 	fiber_bmcr_after = phy_read_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR);
> 	if (fiber_bmcr_after < 0)
> 		return fiber_bmcr_after;
> 
> 	phydev_err(phydev, "%s: EXT_SR before 0x%x after 0x%x, fiber page BMCR before 0x%x after 0x%x\n",
> 		   __func__, ext_sr_before, ext_sr_after,
> 		   fiber_bmcr_before, fiber_bmcr_after);
> 
> 	/* make sure copper is selected */
> 	return marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
> }

Thanks for testing. So that means m88e1111_config_init_sgmii() will not
enable in-band if it was previously disabled. So we need to check the
fiber ANENABLE bit and unconditionally return PHY_AN_INBAND_OFF if it is
clear before evaluating anything else.

Also, given this behaviour of bypass mode, it seems it would only make
sense if the PHY were operating in 1000base-X mode, which we don't do
with SFPs, so maybe it makes no sense to support the ON_TIMEOUT as an
option right now - and as I say above, maybe we should be focing the
AN bypass allow bit to be clear in SGMII mode.

I think maybe Andrew needs to be involved in that last bit though.

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

Re: [PATCH v4 net-next 3/8] net: phy: bcm84881: move the in-band capability check where it belongs

[Russell King (Oracle)]

On Sun, Nov 27, 2022 at 10:14:45PM +0000, Russell King (Oracle) wrote:
> On Fri, Nov 25, 2022 at 05:35:55PM +0200, Vladimir Oltean wrote:
> > On Fri, Nov 25, 2022 at 01:43:34PM +0000, Russell King (Oracle) wrote:
> > > The value of "EXT_SR before" is 1000base-X, so if you change sfp-bus.c::
> > > sfp_select_interface() to use 1000BASEX instead of SGMII then you'll be
> > > using 1000BASEX instead (and it should work, although at fixed 1G
> > > speeds). The only reason the module is working in SGMII mode is because,
> > > as you've noticed above, we switch it to SGMII mode in
> > > m88e1111_config_init_sgmii().
> > 
> > Which is an interesting thing, because m88e1111_config_init_1000basex()
> > does not change the HWCFG_MODE_MASK to something with 1000X in it.
> 
> It only changes the hwcfg mode if it was using 1000base-X no-AN -
> switching it instead to be 1000base-X with AN, but as we've established
> the comment above that code describes something which doesn't happen,
> as the fibre page BMCR is unaffected by this change.
> 
> Anyway, with my SourcePhotonics SPGBTXCNFC module (which is a SGMII
> module) I get:
> 
> Marvell 88E1111 i2c:sfp-3:16: extsr: 8084 fiber bmcr: 1140
> 
> although the first time I plugged it in, BMCR was 1940 (pdown set).
> Key thing is this module doesn't have bypass permitted.
> 
> > But there's actually a problem (or maybe two problems).
> > 
> > First is that if I make phylink treat the ON_TIMEOUT capability by using
> > MLO_AN_PHY (basically like this):
> > 
> > phylink_sfp_config_phy():
> > 
> > 	/* Select whether to operate in in-band mode or not, based on the
> > 	 * capability of the PHY in the current link mode.
> > 	 */
> > 	ret = phy_validate_an_inband(phy, iface);
> > 	phylink_err(pl, "PHY driver reported AN inband 0x%x\n", ret);
> > 	if (ret == PHY_AN_INBAND_UNKNOWN) {
> > 		mode = MLO_AN_INBAND;
> > 
> > 		phylink_dbg(pl,
> > 			    "PHY driver does not report in-band autoneg capability, assuming true\n");
> > //	} else if (ret & (PHY_AN_INBAND_ON | PHY_AN_INBAND_ON_TIMEOUT)) {
> > 	} else if (ret & PHY_AN_INBAND_ON) {
> > 		mode = MLO_AN_INBAND;
> > 	} else {
> > 		mode = MLO_AN_PHY;
> > 	}
> > 
> > [   30.059923] fsl_dpaa2_eth dpni.1 dpmac7: PHY driver reported AN inband 0x4 // PHY_AN_INBAND_ON_TIMEOUT
> > [   30.066867] fsl_dpaa2_eth dpni.1 dpmac7: switched to phy/1000base-x link mode // MLO_AN_PHY
> > [   30.153350] Marvell 88E1111 i2c:sfp-0:16: m88e1111_config_init_1000basex: EXT_SR before 0x9088 after 0x9088, fiber page BMCR before 0x1140 after 0x1140
> > [   30.238970] fsl_dpaa2_eth dpni.1 dpmac7: PHY [i2c:sfp-0:16] driver [Marvell 88E1111] (irq=POLL)
> > 
> > then pinging is broken with mismatched in-band AN settings ("TIMEOUT" in
> > PHY, "OFF" in PCS). I triple-checked this.
> > 
> > ping 192.168.100.2
> > PING 192.168.100.2 (192.168.100.2) 56(84) bytes of data.
> > From 192.168.100.1 icmp_seq=1 Destination Host Unreachable
> > From 192.168.100.1 icmp_seq=2 Destination Host Unreachable
> > From 192.168.100.1 icmp_seq=3 Destination Host Unreachable
> > From 192.168.100.1 icmp_seq=4 Destination Host Unreachable
> > From 192.168.100.1 icmp_seq=5 Destination Host Unreachable
> > From 192.168.100.1 icmp_seq=6 Destination Host Unreachable
> > ^C
> > --- 192.168.100.2 ping statistics ---
> > 9 packets transmitted, 0 received, +6 errors, 100% packet loss, time 8170ms
> > 
> > 
> > However, if using the same phylink code (to force a mismatch), I unhack
> > sfp_select_interface() and use SGMII mode, the timeout feature does
> > actually work:
> > 
> > [   30.262979] fsl_dpaa2_eth dpni.1 dpmac7: PHY driver reported AN inband 0x4 // PHY_AN_INBAND_ON_TIMEOUT
> > [   30.270349] fsl_dpaa2_eth dpni.1 dpmac7: switched to phy/sgmii link mode // MLO_AN_PHY
> > [   30.351066] Marvell 88E1111 i2c:sfp-0:16: m88e1111_config_init_sgmii: EXT_SR before 0x9088 after 0x9084, fiber page BMCR before 0x1140 after 0x1140
> > [   30.433236] fsl_dpaa2_eth dpni.1 dpmac7: PHY [i2c:sfp-0:16] driver [Marvell 88E1111] (irq=POLL)
> > 
> > this is a functional link despite the mismatched settings.
> > 
> > ping 192.168.100.2
> > PING 192.168.100.2 (192.168.100.2) 56(84) bytes of data.
> > 64 bytes from 192.168.100.2: icmp_seq=1 ttl=64 time=0.885 ms
> > 64 bytes from 192.168.100.2: icmp_seq=2 ttl=64 time=0.221 ms
> > 64 bytes from 192.168.100.2: icmp_seq=3 ttl=64 time=0.216 ms
> > 64 bytes from 192.168.100.2: icmp_seq=4 ttl=64 time=0.217 ms
> > 64 bytes from 192.168.100.2: icmp_seq=5 ttl=64 time=0.238 ms
> > ^C
> > --- 192.168.100.2 ping statistics ---
> > 5 packets transmitted, 5 received, 0% packet loss, time 4062ms
> > rtt min/avg/max/mdev = 0.216/0.355/0.885/0.264 ms
> > 
> > 
> > The second problem is that not even *matched* settings work if I turn
> > off BMCR_ANENABLE in the PHY fiber page.
> > 
> > [   30.809869] fsl_dpaa2_eth dpni.1 dpmac7: configuring for inband/sgmii link mode
> > [   30.817936] mdio_bus 0x0000000008c1f000:00: MII_BMCR 0x1140 MII_BMSR 0x9 MII_ADVERTISE 0x1 MII_LPA 0x0 IF_MODE 0x3 // PCS registers at the end of lynx_pcs_config_giga()
> > [   30.917651] fsl_dpaa2_eth dpni.1 dpmac7: PHY driver reported AN inband 0x4 // ignore; m88e1111_validate_an_inband() is hardcoded for this and does not detect BMCR for BASE-X
> > [   30.924571] fsl_dpaa2_eth dpni.1 dpmac7: switched to phy/1000base-x link mode
> > [   30.932441] mdio_bus 0x0000000008c1f000:00: MII_BMCR 0x140 MII_BMSR 0xd MII_ADVERTISE 0x1 MII_LPA 0x0 IF_MODE 0x1
> > [   31.032547] Marvell 88E1111 i2c:sfp-0:16: m88e1111_config_init_1000basex: EXT_SR before 0x9088 after 0x9088, fiber page BMCR before 0x140 after 0x140
> > [   31.117668] fsl_dpaa2_eth dpni.1 dpmac7: PHY [i2c:sfp-0:16] driver [Marvell 88E1111] (irq=POLL)
> > 
> > ping 192.168.100.2
> > PING 192.168.100.2 (192.168.100.2) 56(84) bytes of data.
> > ^C
> > --- 192.168.100.2 ping statistics ---
> > 4 packets transmitted, 0 received, 100% packet loss, time 3058ms
> > 
> > What's common is that if in-band autoneg is turned off (either forced
> > off or via timeout), 1000BASE-X between the Lynx PCS and the 88E1111
> > simply doesn't work.
> 
> I've just tried an experiment here with my SourcePhotonics module.
> 
> I made m88e1111_validate_an_inband() set the SERIAL_AN_BYPASS bit,
> and then the bit I think you're probably unaware of - the PHY needs
> to be soft-reset in order for that change to take effect. Calling
> genphy_soft_reset() is sufficient.
> 
> Then I made m88e1111_validate_an_inband() return PHY_AN_INBAND_OFF.
> So we now have the PHY setup with BMCR=1140 and EXTSR=9084.
> 
> # ping -I eth4 fe80::222:68ff:fe15:37dd
> ping: Warning: source address might be selected on device other than: eth4
> PING fe80::222:68ff:fe15:37dd(fe80::222:68ff:fe15:37dd) from :: eth4: 56 data bytes
> 64 bytes from fe80::222:68ff:fe15:37dd%eth4: icmp_seq=1 ttl=64 time=0.281 ms
> ^C
> --- fe80::222:68ff:fe15:37dd ping statistics ---
> 1 packets transmitted, 1 received, 0% packet loss, time 0ms
> rtt min/avg/max/mdev = 0.281/0.281/0.281/0.000 ms
> 
> (yes, it did use eth4's source address, I checked with tcpdump on the
> target machine.)
> 
> So the link appears to be functional. Using a highly modified mii-diag
> tool that allows me to read/write registers on the PHY, if I read the
> EXT_SR register, it now contains:
> 
> Reading 0x001b=0x9884
> 
> and bit 11 being set means the PHY went into bypass mode. In other
> words, it didn't see the SGMII acknowledgement from the MAC and
> decided to bring the link up in bypass mode.
> 
> However, I've just tripped over some information in the 88E1111
> manual which states that in SGMII mode, if bypass mode is used, then
> the PHY will apparently renegotiate on the copper side advertising
> 1000baseT HD and FD only, no pause. So I checked what my link partner
> is seeing, and it was seeing the original advertisement.
> 
> So I then triggered a renegotiate from the partner, and it now shows
> only 1000baseT/Half 1000baseT/Full being advertised by the 88E1111.
> Reading the advertisement register, it still contains 0x0d41, which
> shows pause modes, 100FD, 10FD - so the advertisement register doesn't
> reflect what was actually adfertised in this case.
> 
> Also, presumably, based on this observation, it will only renegotiate
> if the copper side hadn't resolved to gigabit. If correct, what this
> means is that when operating in SGMII mode, the the PHY becomes
> gigabit-only if bypass mode gets used.
> 
> Given this behaviour, the fact that it switches to gigabit only when
> the SGMII side enters bypass mode, I think we should _positively_ be
> disabling inband bypass in SGMII mode. This change in advertisement
> is not what phylib would expect, and I suspect could lead to surprises
> e.g. if phylib was told to advertise non-gigabit speeds only.
> 
> However, I'll try this test with 1000base-X mode tomorrow.
> 
> > > I think a more comprehensive test would be to write the fiber page
> > > BMCR with 0x140 before changing the mode from 1000baseX to SGMII and
> > > see whether the BMCR changes value. My suspicion is it won't, and
> > > the hwcfg_mode only has an effect on the settings in the fiber page
> > > under hardware reset conditions, and mode changes have no effect on
> > > the fiber page.
> > 
> > Confirmed that changes to the EXT_SR register don't cause changes to the
> > MII_BMCR register:
> > 
> > [   28.587838] Marvell 88E1111 i2c:sfp-0:16: m88e1111_config_init_sgmii: EXT_SR before 0x9088 after 0x9084, fiber page BMCR before 0x140 after 0x140
> > 
> > generated by:
> > 
> > static int m88e1111_config_init_sgmii(struct phy_device *phydev)
> > {
> > 	int fiber_bmcr_before, fiber_bmcr_after;
> > 	int ext_sr_before, ext_sr_after;
> > 	int err;
> > 
> > 	ext_sr_before = phy_read(phydev, MII_M1111_PHY_EXT_SR);
> > 	if (ext_sr_before < 0)
> > 		return ext_sr_before;
> > 
> > 	err = phy_modify_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR,
> > 			       BMCR_ANENABLE, 0);
> > 	if (err < 0)
> > 		return err;
> > 
> > 	fiber_bmcr_before = phy_read_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR);
> > 	if (fiber_bmcr_before < 0)
> > 		return fiber_bmcr_before;
> > 
> > 	err = m88e1111_config_init_hwcfg_mode(
> > 		phydev,
> > 		MII_M1111_HWCFG_MODE_SGMII_NO_CLK,
> > 		MII_M1111_HWCFG_FIBER_COPPER_AUTO);
> > 	if (err < 0)
> > 		return err;
> > 
> > 	ext_sr_after = phy_read(phydev, MII_M1111_PHY_EXT_SR);
> > 	if (ext_sr_after < 0)
> > 		return ext_sr_after;
> > 
> > 	fiber_bmcr_after = phy_read_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR);
> > 	if (fiber_bmcr_after < 0)
> > 		return fiber_bmcr_after;
> > 
> > 	phydev_err(phydev, "%s: EXT_SR before 0x%x after 0x%x, fiber page BMCR before 0x%x after 0x%x\n",
> > 		   __func__, ext_sr_before, ext_sr_after,
> > 		   fiber_bmcr_before, fiber_bmcr_after);
> > 
> > 	/* make sure copper is selected */
> > 	return marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
> > }
> 
> Thanks for testing. So that means m88e1111_config_init_sgmii() will not
> enable in-band if it was previously disabled. So we need to check the
> fiber ANENABLE bit and unconditionally return PHY_AN_INBAND_OFF if it is
> clear before evaluating anything else.
> 
> Also, given this behaviour of bypass mode, it seems it would only make
> sense if the PHY were operating in 1000base-X mode, which we don't do
> with SFPs, so maybe it makes no sense to support the ON_TIMEOUT as an
> option right now - and as I say above, maybe we should be focing the
> AN bypass allow bit to be clear in SGMII mode.
> 
> I think maybe Andrew needs to be involved in that last bit though.

Here's an updated patch.
8<===
From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
Subject: [PATCH] net: phy: marvell: add validate_an_inband() method

Add the validate_an_inband() method for Marvell 88E1111, the Finisar
version of the 88E1111, and 88E1112.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/marvell.c | 54 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 3c54d7d0f17f..1d7e00c4d97a 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -669,6 +669,52 @@ static int marvell_config_aneg_fiber(struct phy_device *phydev)
 	return genphy_check_and_restart_aneg(phydev, changed);
 }
 
+static int m88e1111_validate_an_inband(struct phy_device *phydev,
+				       phy_interface_t interface)
+{
+	int hwcfg_mode, extsr, bmcr;
+
+	if (interface != PHY_INTERFACE_MODE_1000BASEX &&
+	    interface != PHY_INTERFACE_MODE_SGMII)
+		return PHY_AN_INBAND_UNKNOWN;
+
+	extsr = phy_read(phydev, MII_M1111_PHY_EXT_SR);
+	bmcr = phy_read_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR);
+	if (extsr < 0 || bmcr < 0)
+		return PHY_AN_INBAND_UNKNOWN;
+
+	/* We make no efforts to enable the ANENABLE bit when switching mode.
+	 * If this bit is clear, then we will not be using in-band signalling.
+	 */
+	if (!(bmcr & BMCR_ANENABLE))
+		return PHY_AN_INBAND_OFF;
+
+	hwcfg_mode = extsr & MII_M1111_HWCFG_MODE_MASK;
+
+	/* If we are in 1000base-X no-AN hwcfg_mode,
+	 * m88e1111_config_init_1000basex() will allow AN bypass, but does not
+	 * enable AN.
+	 */
+	if (interface == PHY_INTERFACE_MODE_1000BASEX &&
+	    hwcfg_mode == MII_M1111_HWCFG_MODE_COPPER_1000X_NOAN)
+		return PHY_AN_INBAND_ON_TIMEOUT;
+
+	/* Otherwise, we leave the AN enable bit and the AN bypass bit
+	 * alone, so we need to read the registers to determine how the
+	 * MAC facing side of the PHY has been setup by firmware and/or
+	 * hardware reset.
+	 *
+	 * If the AN enable bit is clear, then all in-band signalling
+	 * on the SGMII/1000base-X side is disabled. Otherwise, AN is
+	 * enabled. If the bypass bit is set, AN can complete without
+	 * a response from the partner (MAC).
+	 */
+	if (extsr & MII_M1111_HWCFG_SERIAL_AN_BYPASS)
+		return PHY_AN_INBAND_ON_TIMEOUT;
+
+	return PHY_AN_INBAND_ON;
+}
+
 static int m88e1111_config_aneg(struct phy_device *phydev)
 {
 	int extsr = phy_read(phydev, MII_M1111_PHY_EXT_SR);
@@ -915,7 +961,10 @@ static int m88e1111_config_init_1000basex(struct phy_device *phydev)
 	if (extsr < 0)
 		return extsr;
 
-	/* If using copper mode, ensure 1000BaseX auto-negotiation is enabled */
+	/* If using copper mode, ensure 1000BaseX auto-negotiation is enabled.
+	 * FIXME: this does not actually enable 1000BaseX auto-negotiation if
+	 * it was previously disabled in the Fiber BMCR!
+	 */
 	mode = extsr & MII_M1111_HWCFG_MODE_MASK;
 	if (mode == MII_M1111_HWCFG_MODE_COPPER_1000X_NOAN) {
 		err = phy_modify(phydev, MII_M1111_PHY_EXT_SR,
@@ -2978,6 +3027,7 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1011_get_tunable,
 		.set_tunable = m88e1011_set_tunable,
+		.validate_an_inband = m88e1111_validate_an_inband,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1111,
@@ -2999,6 +3049,7 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1111_get_tunable,
 		.set_tunable = m88e1111_set_tunable,
+		.validate_an_inband = m88e1111_validate_an_inband,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1111_FINISAR,
@@ -3020,6 +3071,7 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1111_get_tunable,
 		.set_tunable = m88e1111_set_tunable,
+		.validate_an_inband = m88e1111_validate_an_inband,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1118,
-- 
2.30.2

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

Re: [PATCH v4 net-next 3/8] net: phy: bcm84881: move the in-band capability check where it belongs

[Russell King (Oracle)]

On Tue, Nov 29, 2022 at 01:40:58PM +0000, Russell King (Oracle) wrote:
> Here's an updated patch.
> 8<===
> From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
> Subject: [PATCH] net: phy: marvell: add validate_an_inband() method
> 
> Add the validate_an_inband() method for Marvell 88E1111, the Finisar
> version of the 88E1111, and 88E1112.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

And this is what I was using on top to force a mismatch with bypass
enabled:

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index fbf881cd4a38..c7a0389320cd 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -683,6 +683,14 @@ static int m88e1111_validate_an_inband(struct phy_device *phydev,
 	if (extsr < 0 || bmcr < 0)
 		return PHY_AN_INBAND_UNKNOWN;
 
+	/* <<=== HACK */
+	phydev_info(phydev, "extsr: %04x fiber bmcr: %04x\n", extsr, bmcr);
+	phy_write(phydev, MII_M1111_PHY_EXT_SR, extsr |
+		  MII_M1111_HWCFG_SERIAL_AN_BYPASS);
+	genphy_soft_reset(phydev);
+	return PHY_AN_INBAND_OFF;
+	/* ===>> HACK */
+
 	/* We make no efforts to enable the ANENABLE bit when switching mode.
 	 * If this bit is clear, then we will not be using in-band signalling.
 	 */

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

Re: [PATCH v4 net-next 3/8] net: phy: bcm84881: move the in-band capability check where it belongs

[Andrew Lunn]

> > However, I've just tripped over some information in the 88E1111
> > manual which states that in SGMII mode, if bypass mode is used, then
> > the PHY will apparently renegotiate on the copper side advertising
> > 1000baseT HD and FD only, no pause. So I checked what my link partner
> > is seeing, and it was seeing the original advertisement.
> > 
> > So I then triggered a renegotiate from the partner, and it now shows
> > only 1000baseT/Half 1000baseT/Full being advertised by the 88E1111.
> > Reading the advertisement register, it still contains 0x0d41, which
> > shows pause modes, 100FD, 10FD - so the advertisement register doesn't
> > reflect what was actually adfertised in this case.
> > 
> > Also, presumably, based on this observation, it will only renegotiate
> > if the copper side hadn't resolved to gigabit. If correct, what this
> > means is that when operating in SGMII mode, the the PHY becomes
> > gigabit-only if bypass mode gets used.
> > 
> > Given this behaviour, the fact that it switches to gigabit only when
> > the SGMII side enters bypass mode, I think we should _positively_ be
> > disabling inband bypass in SGMII mode. This change in advertisement
> > is not what phylib would expect, and I suspect could lead to surprises
> > e.g. if phylib was told to advertise non-gigabit speeds only.

Sorry, i've not been reading this thread.

This behaviour is not very nice. So i agree, it should be avoided if
possible.

> > Thanks for testing. So that means m88e1111_config_init_sgmii() will not
> > enable in-band if it was previously disabled. So we need to check the
> > fiber ANENABLE bit and unconditionally return PHY_AN_INBAND_OFF if it is
> > clear before evaluating anything else.
> > 
> > Also, given this behaviour of bypass mode, it seems it would only make
> > sense if the PHY were operating in 1000base-X mode, which we don't do
> > with SFPs, so maybe it makes no sense to support the ON_TIMEOUT as an
> > option right now - and as I say above, maybe we should be focing the
> > AN bypass allow bit to be clear in SGMII mode.
> > 
> > I think maybe Andrew needs to be involved in that last bit though.

As you say, 1000Base-X does not make much sense for a copper
SFP. SGMII does odd things, so just not supporting ON_TIMEOUT seems
reasonable.

	Andrew

Re: [PATCH v4 net-next 3/8] net: phy: bcm84881: move the in-band capability check where it belongs

[Vladimir Oltean]

On Tue, Nov 22, 2022 at 09:38:43AM +0000, Russell King (Oracle) wrote:
> 88E1111 is the commonly used accessible PHY on gigabit SFPs, as this
> PHY implements I2C access natively.

As a side question, I suppose it would be possible to put PHYs on copper
SFP modules even if they don't natively implement I2C access. In that case,
if configuration from the host is at all available, how does that happen,
is there some sort of protocol translator (I2C -> MDIO) on the module?
Do you know of any part number for an I2C controlled MDIO controller?

Re: [PATCH v4 net-next 3/8] net: phy: bcm84881: move the in-band capability check where it belongs

[Russell King (Oracle)]

On Tue, Nov 22, 2022 at 02:24:51PM +0200, Vladimir Oltean wrote:
> On Tue, Nov 22, 2022 at 09:38:43AM +0000, Russell King (Oracle) wrote:
> > 88E1111 is the commonly used accessible PHY on gigabit SFPs, as this
> > PHY implements I2C access natively.
> 
> As a side question, I suppose it would be possible to put PHYs on copper
> SFP modules even if they don't natively implement I2C access. In that case,
> if configuration from the host is at all available, how does that happen,
> is there some sort of protocol translator (I2C -> MDIO) on the module?

Modules that provide access to the PHY seem to do so on I2C address 0x56
and require specific I2C access patterns. I think the 88e1111 set a kind
of "standard" and many follow that or similar.

Some just don't give any kind of access to the PHY - for example, the
Mikrotik modules have an AR803x PHY on them, but there is no way to
access it.

> Do you know of any part number for an I2C controlled MDIO controller?

I think many that don't use a microcontroller - for example, when
someone puts an 88x3310, bcm84881, the protocols are very similar but
timing may vary.

Some of the patches from Marek provided a different way - via the 0x50
or 0x51 "eeprom" addresses, accessing specific addresses with in those
spaces, and if I remember correctly, requiring a "password" to enable
access.

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

[PATCH v4 net-next 4/8] net: phylink: add option to sync in-band autoneg setting between PCS and PHY

[Vladimir Oltean]

In the case of an on-board PHY (not on SFP module), phylink parses the
'managed = "in-band-status"' property (which may or may not be present
in addition to the 'phy-handle' property), and based on this, selects
pl->cfg_link_an_mode to be MLO_AN_PHY or MLO_AN_INBAND.

Drivers which make use of phylink can use MLO_AN_PHY as an indication to
disable in-band autoneg when connected to an on-board PHY, and
MLO_AN_INBAND to enable it. That's what most of the drivers seem to do,
except macb_mac_config() which seems to always force in-band autoneg
except for MLO_AN_FIXED.

If one assumes purely Clause 37 compatible state machines in the
PHY-side PCS and in the MAC-side PCS, then in-band autoneg needs to be
enabled in both places, or disabled in both places, to establish a
successful system-side link. The exception seems to be mvneta, which has
a hardware-based fallback on no-inband when inband autoneg was enabled
but failed. Nonetheless, this is not a generally available feature.

While in the case of an SFP module, in-band autoneg is genuinely useful
in passing the link information through the Ethernet channel when we
lack an I2C/MDIO side channel, in the case of on-board PHYs it is
perhaps less so. Nonetheless, settings must be in sync for a functional
link.

There is currently a lack of information within the kernel as to whether
in-band autoneg should be used between a certain MAC and PHY. We rely on
the fwnode specification for this.

Most of the platforms are seemingly okay with the status quo, but there
are 2 real life scenarios where it has limitations:

- A driver recently converted from phylib to phylink. The device trees
  in circulation will not have the 'managed' property (since it's
  phylink specific), but some PHYs will require in-band autoneg enabled
  in the MAC-side PCS to work with them.

- The PHY in-band autoneg setting is not really fixed but configurable,
  and therefore, the property should not really belong to device tree.
  Matters are made worse when PHY drivers in bootloaders (U-Boot) also
  enable/disable this setting, and this can cause what is specified in
  the device tree to be out of sync with reality. Linux PHY drivers do
  not currently configure in-band autoneg according to the 'managed'
  property of the attached MAC.

This change introduces a new opt-in feature called sync_an_inband which
may override the in-band autoneg setting passed to phylink callbacks,
but not to 'true' as ovr_an_inband does, but rather to what the PHY
reports as supported (with a fallback to what was in the device tree, if
the PHY driver reports nothing).

It's quite possible that one more call to phylink_sync_inband_aneg() is
needed when the PHY changes its pl->phy_state.interface and this results
in a change to pl->link_config.interface. This is the
phylink_major_config() code path, and if the PHY has different in-band
autoneg requirements for the new SERDES protocol, we are currently not
informed about those. Unfortunately I don't have access to any board
which supports SERDES protocol change of an on-board PHY, and I don't
know without testing where that extra sync call should be put, so I
haven't put it anywhere.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v3->v4:
- split the on-board PHY part out of previous patch 2/6 which combines them
- phylink_fixup_inband_aneg() renamed to phylink_sync_an_inband()
- opt-in via phylink_config :: sync_an_inband
- look at pl->link_config.interface rather than pl->link_interface
- clearer comments, add kerneldocs

 drivers/net/phy/phylink.c | 49 +++++++++++++++++++++++++++++++++++++++
 include/linux/phylink.h   |  7 ++++++
 2 files changed, 56 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index bf2a5ebfc4f4..598f5feb661e 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -156,6 +156,45 @@ static const char *phylink_an_mode_str(unsigned int mode)
 	return mode < ARRAY_SIZE(modestr) ? modestr[mode] : "unknown";
 }
 
+/**
+ * phylink_sync_an_inband() - Sync in-band autoneg between PCS and PHY
+ * @pl: a pointer to a &struct phylink returned from phylink_create()
+ * @phy: a pointer to a &struct phy_device
+ *
+ * Query the in-band autoneg capability of an on-board PHY in an attempt to
+ * sync the PCS-side link autoneg mode with the PHY autoneg mode. Set the
+ * current link autoneg mode to the mode configured through the fwnode if the
+ * PHY supports it or if its capabilities are unknown, or to an alternative
+ * mode that the PHY can operate in.
+ */
+static void phylink_sync_an_inband(struct phylink *pl, struct phy_device *phy)
+{
+	unsigned int mode = pl->cfg_link_an_mode;
+	int ret;
+
+	if (!pl->config->sync_an_inband)
+		return;
+
+	ret = phy_validate_an_inband(phy, pl->link_config.interface);
+	if (ret == PHY_AN_INBAND_UNKNOWN) {
+		phylink_dbg(pl,
+			    "PHY driver does not report in-band autoneg capability, assuming %s\n",
+			    phylink_autoneg_inband(mode) ? "true" : "false");
+	} else if (phylink_autoneg_inband(mode) && !(ret & PHY_AN_INBAND_ON)) {
+		phylink_err(pl,
+			    "Requested in-band autoneg but driver does not support this, disabling it.\n");
+
+		mode = MLO_AN_PHY;
+	} else if (!phylink_autoneg_inband(mode) && !(ret & PHY_AN_INBAND_OFF)) {
+		phylink_dbg(pl,
+			    "PHY driver requests in-band autoneg, force-enabling it.\n");
+
+		mode = MLO_AN_INBAND;
+	}
+
+	pl->cur_link_an_mode = mode;
+}
+
 /**
  * phylink_interface_max_speed() - get the maximum speed of a phy interface
  * @interface: phy interface mode defined by &typedef phy_interface_t
@@ -1475,6 +1514,12 @@ struct phylink *phylink_create(struct phylink_config *config,
 	struct phylink *pl;
 	int ret;
 
+	if (config->ovr_an_inband && config->sync_an_inband) {
+		dev_err(config->dev,
+			"phylink: error: ovr_an_inband and sync_an_inband cannot be used simultaneously\n");
+		return ERR_PTR(-EINVAL);
+	}
+
 	if (mac_ops->mac_select_pcs &&
 	    mac_ops->mac_select_pcs(config, PHY_INTERFACE_MODE_NA) !=
 	      ERR_PTR(-EOPNOTSUPP))
@@ -1725,6 +1770,8 @@ int phylink_connect_phy(struct phylink *pl, struct phy_device *phy)
 		pl->link_config.interface = pl->link_interface;
 	}
 
+	phylink_sync_an_inband(pl, phy);
+
 	ret = phylink_attach_phy(pl, phy, pl->link_interface);
 	if (ret < 0)
 		return ret;
@@ -1800,6 +1847,8 @@ int phylink_fwnode_phy_connect(struct phylink *pl,
 		pl->link_config.interface = pl->link_interface;
 	}
 
+	phylink_sync_an_inband(pl, phy_dev);
+
 	ret = phy_attach_direct(pl->netdev, phy_dev, flags,
 				pl->link_interface);
 	if (ret) {
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index c492c26202b5..d4b931bdfdfe 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -124,6 +124,12 @@ enum phylink_op_type {
  *		      if MAC link is at %MLO_AN_FIXED mode.
  * @mac_managed_pm: if true, indicate the MAC driver is responsible for PHY PM.
  * @ovr_an_inband: if true, override PCS to MLO_AN_INBAND
+ * @sync_an_inband: if true, select between %MLO_AN_INBAND and %MLO_AN_PHY
+ *		    according to the capability of the attached on-board PHY
+ *		    (if both modes are supported, the mode deduced from the
+ *		    fwnode specification is used). With PHYs on SFP modules,
+ *		    the automatic selection takes place regardless of this
+ *		    setting. Mutually exclusive with &ovr_an_inband.
  * @get_fixed_state: callback to execute to determine the fixed link state,
  *		     if MAC link is at %MLO_AN_FIXED mode.
  * @supported_interfaces: bitmap describing which PHY_INTERFACE_MODE_xxx
@@ -137,6 +143,7 @@ struct phylink_config {
 	bool poll_fixed_state;
 	bool mac_managed_pm;
 	bool ovr_an_inband;
+	bool sync_an_inband;
 	void (*get_fixed_state)(struct phylink_config *config,
 				struct phylink_link_state *state);
 	DECLARE_PHY_INTERFACE_MASK(supported_interfaces);
-- 
2.34.1

[PATCH v4 net-next 5/8] net: phylink: explicitly configure in-band autoneg for on-board PHYs

[Vladimir Oltean]

Currently Linux has no control over whether a MAC-to-PHY interface uses
in-band signaling or not, even though phylink has the
	managed = "in-band-status";
property which denotes that the MAC expects in-band signaling to be used.

The problem is that if the in-band signaling is configurable in both the
PHY and the MAC, there is a risk that they are out of sync unless
phylink manages them both (setting in PHY may have come from out-of-reset
value, or from bootloader configuration). Most in-band autoneg state
machines follow IEEE 802.3 clause 37, which means that they will not
change the operating mode of the SERDES lane from control to data mode
unless in-band AN completed successfully. Therefore traffic will not
work.

To ensure that systems operate under full control of this particular
setting and not depend on what some other entity has done, let's
introduce a new PHY driver method for configuring in-band autoneg.

The first user will be phylink; the main PHY library does not call
phy_config_inband_autoneg(), because it does not know what to configure
it to. Presumably, individual non-phylink drivers can also call
phy_config_inband_autoneg() individually.

To avoid regressions with board device trees which may rely on fragile
mechanisms, gate the phy_config_inband_autoneg() call with the
bool sync_an_inband opt-in. Also skip doing it for PHYs on SFP modules.
I don't see a need for those, so don't risk making a change there.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v3->v4:
- s/inband_aneg/an_inband/
- clearer comments, added kerneldocs
- opt-in via phylink_config :: sync_an_inband

 drivers/net/phy/phy.c     | 26 ++++++++++++++++++++++++++
 drivers/net/phy/phylink.c | 12 ++++++++++++
 include/linux/phy.h       | 10 ++++++++++
 3 files changed, 48 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 2abbacf2c7cb..37cdd9afd7e1 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -758,6 +758,32 @@ int phy_validate_an_inband(struct phy_device *phydev,
 }
 EXPORT_SYMBOL_GPL(phy_validate_an_inband);
 
+/**
+ * phy_config_an_inband - modify in-band autoneg setting
+ * @phydev: the phy_device struct
+ * @interface: the MAC-side interface type
+ * @enabled: selects whether in-band autoneg is used or not
+ *
+ * Configures the PHY to enable or disable in-band autoneg for the given
+ * interface type. @enabled can be passed as true only if the bit mask returned
+ * by @phy_validate_an_inband() contains @PHY_AN_INBAND_ON, and false only if
+ * it contains @PHY_AN_INBAND_OFF.
+ *
+ * Returns 0 on success, negative error otherwise.
+ */
+int phy_config_an_inband(struct phy_device *phydev, phy_interface_t interface,
+			 bool enabled)
+{
+	if (!phydev->drv)
+		return -EIO;
+
+	if (!phydev->drv->config_an_inband)
+		return -EOPNOTSUPP;
+
+	return phydev->drv->config_an_inband(phydev, interface, enabled);
+}
+EXPORT_SYMBOL_GPL(phy_config_an_inband);
+
 /**
  * _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 598f5feb661e..ca3facc4f1a7 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1691,6 +1691,18 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 		return ret;
 	}
 
+	if (pl->config->sync_an_inband && !phy_on_sfp(phy)) {
+		bool use_inband = phylink_autoneg_inband(pl->cur_link_an_mode);
+
+		ret = phy_config_an_inband(phy, interface, use_inband);
+		if (ret && ret != -EOPNOTSUPP) {
+			phylink_err(pl,
+				    "failed to configure PHY in-band autoneg: %pe\n",
+				    ERR_PTR(ret));
+			return ret;
+		}
+	}
+
 	phy->phylink = pl;
 	phy->phy_link_change = phylink_phy_change;
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 56a431d88dd9..6f8d5765cf0c 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -860,6 +860,14 @@ struct phy_driver {
 	int (*validate_an_inband)(struct phy_device *phydev,
 				  phy_interface_t interface);
 
+	/**
+	 * @config_an_inband: Enable or disable in-band auto-negotiation for
+	 * the system-side interface if the PHY operates in a mode that
+	 * requires it: (Q)SGMII, USXGMII, 1000Base-X, etc.
+	 */
+	int (*config_an_inband)(struct phy_device *phydev,
+				phy_interface_t interface, bool enabled);
+
 	/** @aneg_done: Determines the auto negotiation result */
 	int (*aneg_done)(struct phy_device *phydev);
 
@@ -1557,6 +1565,8 @@ int phy_start_aneg(struct phy_device *phydev);
 int phy_aneg_done(struct phy_device *phydev);
 int phy_validate_an_inband(struct phy_device *phydev,
 			   phy_interface_t interface);
+int phy_config_an_inband(struct phy_device *phydev, phy_interface_t interface,
+			 bool enabled);
 int phy_speed_down(struct phy_device *phydev, bool sync);
 int phy_speed_up(struct phy_device *phydev);
 
-- 
2.34.1

Re: [PATCH v4 net-next 5/8] net: phylink: explicitly configure in-band autoneg for on-board PHYs

[Russell King (Oracle)]

On Fri, Nov 18, 2022 at 02:01:21AM +0200, Vladimir Oltean wrote:
> +	if (pl->config->sync_an_inband && !phy_on_sfp(phy)) {

Hmm, this phy_on_sfp() is new to me, and looking at the git history, I
really don't think this does what it claims to do. This returns the
status of phydev->is_on_sfp_module, which is set by this code:

        phydev->phy_link_change = phy_link_change;
        if (dev) {
                phydev->attached_dev = dev;
                dev->phydev = phydev;

                if (phydev->sfp_bus_attached)
                        dev->sfp_bus = phydev->sfp_bus;
                else if (dev->sfp_bus)
                        phydev->is_on_sfp_module = true;
        }

... which is very wrong. "dev" here is the net_device, and a net_device
will have its sfp_bus member set when there is a SFP cage present,
which may be behind a off-SFP PHY.

This means that when a PHY is attached by the network driver in their
ndo_open, if there is a SFP bus on the interface (such as on the
Macchiatobin board), the above will set is_on_sfp_module true for the
on-board PHY even though it is not in the SFP module.

Essentially, commit b834489bcecc is incorrect, and needs to be fixed
before use is made of phy_on_sfp() outside of the broadcom driver.

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

Re: [PATCH v4 net-next 5/8] net: phylink: explicitly configure in-band autoneg for on-board PHYs

[Vladimir Oltean]

On Fri, Nov 18, 2022 at 10:09:35AM +0000, Russell King (Oracle) wrote:
> On Fri, Nov 18, 2022 at 02:01:21AM +0200, Vladimir Oltean wrote:
> > +	if (pl->config->sync_an_inband && !phy_on_sfp(phy)) {
> 
> Hmm, this phy_on_sfp() is new to me, and looking at the git history, I
> really don't think this does what it claims to do. This returns the
> status of phydev->is_on_sfp_module, which is set by this code:
> 
>         phydev->phy_link_change = phy_link_change;
>         if (dev) {
>                 phydev->attached_dev = dev;
>                 dev->phydev = phydev;
> 
>                 if (phydev->sfp_bus_attached)
>                         dev->sfp_bus = phydev->sfp_bus;
>                 else if (dev->sfp_bus)
>                         phydev->is_on_sfp_module = true;
>         }
> 
> ... which is very wrong. "dev" here is the net_device, and a net_device
> will have its sfp_bus member set when there is a SFP cage present,
> which may be behind a off-SFP PHY.
> 
> This means that when a PHY is attached by the network driver in their
> ndo_open, if there is a SFP bus on the interface (such as on the
> Macchiatobin board), the above will set is_on_sfp_module true for the
> on-board PHY even though it is not in the SFP module.
> 
> Essentially, commit b834489bcecc is incorrect, and needs to be fixed
> before use is made of phy_on_sfp() outside of the broadcom driver.

IIUC, you're saying that if there is an SFP cage after an on-board PHY
X (presumably set using phy_sfp_attach()), then PHY X will be declared
as having phydev->is_on_sfp_module = true despite being on-board?

I don't have such a setup to experiment with. Looking at armada-8040-mcbin.dts,
it's these PHYs, right?

&cp0_xmdio {
	status = "okay";

	phy0: ethernet-phy@0 {
		compatible = "ethernet-phy-ieee802.3-c45";
		reg = <0>;
		sfp = <&sfp_eth0>;
	};

	phy8: ethernet-phy@8 {
		compatible = "ethernet-phy-ieee802.3-c45";
		reg = <8>;
		sfp = <&sfp_eth1>;
	};
};

&cp0_eth0 {
	status = "okay";
	/* Network PHY */
	phy = <&phy0>;
	phy-mode = "10gbase-r";
};

&cp1_eth0 {
	status = "okay";
	/* Network PHY */
	phy = <&phy8>;
	phy-mode = "10gbase-r";
};

But in this case, I believe that phy_sfp_attach() will set
phydev->sfp_bus_attached = true, and this will make the code go through
the first "if" branch and not through the "else" (IOW, the code excludes
the on-board PHYs from the logic)? Or are you describing some
timing/ordering issue which makes this not be the case (something like
the sfp_upstream_ops :: attach() of the on-board PHY being called later
than the phy_attach_direct())?

Could you help me better understand why the code will not enter through
the "if" in this case but will enter through the "else"?

Re: [PATCH v4 net-next 5/8] net: phylink: explicitly configure in-band autoneg for on-board PHYs

[Russell King (Oracle)]

On Fri, Nov 18, 2022 at 01:25:20PM +0200, Vladimir Oltean wrote:
> On Fri, Nov 18, 2022 at 10:09:35AM +0000, Russell King (Oracle) wrote:
> > On Fri, Nov 18, 2022 at 02:01:21AM +0200, Vladimir Oltean wrote:
> > > +	if (pl->config->sync_an_inband && !phy_on_sfp(phy)) {
> > 
> > Hmm, this phy_on_sfp() is new to me, and looking at the git history, I
> > really don't think this does what it claims to do. This returns the
> > status of phydev->is_on_sfp_module, which is set by this code:
> > 
> >         phydev->phy_link_change = phy_link_change;
> >         if (dev) {
> >                 phydev->attached_dev = dev;
> >                 dev->phydev = phydev;
> > 
> >                 if (phydev->sfp_bus_attached)
> >                         dev->sfp_bus = phydev->sfp_bus;
> >                 else if (dev->sfp_bus)
> >                         phydev->is_on_sfp_module = true;
> >         }
> > 
> > ... which is very wrong. "dev" here is the net_device, and a net_device
> > will have its sfp_bus member set when there is a SFP cage present,
> > which may be behind a off-SFP PHY.
> > 
> > This means that when a PHY is attached by the network driver in their
> > ndo_open, if there is a SFP bus on the interface (such as on the
> > Macchiatobin board), the above will set is_on_sfp_module true for the
> > on-board PHY even though it is not in the SFP module.
> > 
> > Essentially, commit b834489bcecc is incorrect, and needs to be fixed
> > before use is made of phy_on_sfp() outside of the broadcom driver.
> 
> IIUC, you're saying that if there is an SFP cage after an on-board PHY
> X (presumably set using phy_sfp_attach()), then PHY X will be declared
> as having phydev->is_on_sfp_module = true despite being on-board?

Having looked more deeply, I don't think it's the problem I thought it
was, so you're all good with using phy_on_sfp(). Sorry for the noise.

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

[PATCH v4 net-next 6/8] net: phy: mscc: configure in-band auto-negotiation for VSC8514

[Vladimir Oltean]

Add the in-band validation and configuration knobs for the VSC8514 quad
PHY. Supporting both ON and OFF means that the mode which gets used in
the end depends squarely on device tree, and the PHY adapts to that.

Tested with QSGMII in-band AN both on and off on NXP LS1028A-RDB and
T1040-RDB.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v3->v4: s/inband_aneg/an_inband/

 drivers/net/phy/mscc/mscc.h      |  2 ++
 drivers/net/phy/mscc/mscc_main.c | 21 +++++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
index a50235fdf7d9..366db1425561 100644
--- a/drivers/net/phy/mscc/mscc.h
+++ b/drivers/net/phy/mscc/mscc.h
@@ -195,6 +195,8 @@ enum rgmii_clock_delay {
 #define MSCC_PHY_EXTENDED_INT_MS_EGR	  BIT(9)
 
 /* Extended Page 3 Registers */
+#define MSCC_PHY_SERDES_PCS_CTRL	  16
+#define MSCC_PHY_SERDES_ANEG		  BIT(7)
 #define MSCC_PHY_SERDES_TX_VALID_CNT	  21
 #define MSCC_PHY_SERDES_TX_CRC_ERR_CNT	  22
 #define MSCC_PHY_SERDES_RX_VALID_CNT	  28
diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
index 8a13b1ad9a33..f136782fd995 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -2189,6 +2189,25 @@ static int vsc85xx_read_status(struct phy_device *phydev)
 	return genphy_read_status(phydev);
 }
 
+static int vsc8514_validate_an_inband(struct phy_device *phydev,
+				      phy_interface_t interface)
+{
+	return PHY_AN_INBAND_OFF | PHY_AN_INBAND_ON;
+}
+
+static int vsc8514_config_an_inband(struct phy_device *phydev,
+				    phy_interface_t interface, bool enabled)
+{
+	int reg_val = 0;
+
+	if (enabled)
+		reg_val = MSCC_PHY_SERDES_ANEG;
+
+	return phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_3,
+				MSCC_PHY_SERDES_PCS_CTRL, MSCC_PHY_SERDES_ANEG,
+				reg_val);
+}
+
 static int vsc8514_probe(struct phy_device *phydev)
 {
 	struct vsc8531_private *vsc8531;
@@ -2395,6 +2414,8 @@ static struct phy_driver vsc85xx_driver[] = {
 	.get_sset_count = &vsc85xx_get_sset_count,
 	.get_strings    = &vsc85xx_get_strings,
 	.get_stats      = &vsc85xx_get_stats,
+	.validate_an_inband = vsc8514_validate_an_inband,
+	.config_an_inband = vsc8514_config_an_inband,
 },
 {
 	.phy_id		= PHY_ID_VSC8530,
-- 
2.34.1

[PATCH v4 net-next 7/8] net: phy: at803x: validate in-band autoneg for AT8031/AT8033

[Vladimir Oltean]

Allow drivers which migrate from phylib to phylink and have old device
tree blobs to work with the AR8031/AT8033 on-board PHY in the SGMII
SERDES side mode.

This would allow DT breakage like the one fixed by commit df392aefe96b
("arm64: dts: fsl-ls1028a-kontron-sl28: specify in-band mode for ENETC")
to be avoided in the future.

We know from experimentation with NXP SoCs that the PHY doesn't pass
traffic if in-band autoneg is enabled but fails to complete. We also
know that it is in principle possible to disable in-band autoneg in the
PHY. This would require disabling autoneg in the fiber page, and then
keeping the fiber and copper page speeds in sync, as explained by
Michael Walle here:
https://patchwork.kernel.org/project/netdevbpf/patch/20210212172341.3489046-2-olteanv@gmail.com/

But since the PHY driver does not currently handle the complexity of
keeping those speeds in sync, we can safely say that no MAC attached to
the AT8031/AT8033 in SGMII mode has in-band autoneg disabled.

I have no motivation to add support for disabled in-band autoneg. I just
need the driver to report that it requires this enabled, which will
make phylink promote a MLO_AN_PHY connection to MLO_AN_INBAND. This is
enough to keep everyone happy.

These PHYs also support RGMII, and for that mode, we report that in-band
AN is unknown, which means that phylink will not change the mode from
the device tree. Since commit d73ffc08824d ("net: phylink: allow
RGMII/RTBI in-band status"), RGMII in-band status is a thing, and I
don't want to meddle with that unless I have a reason for it.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v3->v4:
- s/inband_aneg/an_inband/
- drop unnecessary support for PHY_AN_INBAND_OFF

 drivers/net/phy/at803x.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 349b7b1dbbf2..2ef6ac92fecb 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -1355,6 +1355,15 @@ static int at803x_config_aneg(struct phy_device *phydev)
 	return __genphy_config_aneg(phydev, ret);
 }
 
+static int at803x_validate_an_inband(struct phy_device *phydev,
+				     phy_interface_t interface)
+{
+	if (interface == PHY_INTERFACE_MODE_SGMII)
+		return PHY_AN_INBAND_ON;
+
+	return PHY_AN_INBAND_UNKNOWN;
+}
+
 static int at803x_get_downshift(struct phy_device *phydev, u8 *d)
 {
 	int val;
@@ -2076,6 +2085,7 @@ static struct phy_driver at803x_driver[] = {
 	.set_tunable		= at803x_set_tunable,
 	.cable_test_start	= at803x_cable_test_start,
 	.cable_test_get_status	= at803x_cable_test_get_status,
+	.validate_an_inband	= at803x_validate_an_inband,
 }, {
 	/* Qualcomm Atheros AR8032 */
 	PHY_ID_MATCH_EXACT(ATH8032_PHY_ID),
-- 
2.34.1

[PATCH v4 net-next 8/8] net: opt MAC drivers which use Lynx PCS into phylink sync_an_inband

[Vladimir Oltean]

lynx_pcs_config_giga() enables "SGMII AN" only when used in the
MLO_AN_INBAND mode. If it's connected to a PHY, it expects that the PHY
has the in-band autoneg setting in sync with it.

To fulfill those expectations, set the sync_an_inband field in the
phylink_config structure, to opt into the new phylink behavior which
does the following to help with that:

(1) queries PHY driver to figure out a mode supported by both ends
(2) configures in-band autoneg in the PHY to something supported by both
    ends

The Lynx PCS is integrated in DPAA1 and DPAA2 SoCs, as well as in
LS1028A (ENETC + Felix switch) and in the T1040 Seville switch.

It seems that the DPAA1 phylink conversion already took steps to prevent
breakage with old DT blobs, by using ovr_an_inband. That is partially
sufficient, partially insufficient (there is still no guarantee that PHY
really has in-band AN enabled). Remove that logic and simply set
sync_an_inband instead (setting both isn't allowed anyway).

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v3->v4: patch is new

 drivers/net/dsa/ocelot/felix.c                   |  2 ++
 drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c |  1 +
 drivers/net/ethernet/freescale/enetc/enetc_pf.c  |  1 +
 drivers/net/ethernet/freescale/fman/fman_memac.c | 16 +---------------
 4 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 44e160f32067..6deff681c02d 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -1042,6 +1042,8 @@ static void felix_phylink_get_caps(struct dsa_switch *ds, int port,
 {
 	struct ocelot *ocelot = ds->priv;
 
+	config->sync_an_inband = true;
+
 	/* This driver does not make use of the speed, duplex, pause or the
 	 * advertisement in its mac_config, so it is safe to mark this driver
 	 * as non-legacy.
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
index 51c9da8e1be2..61d31ffb5d97 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
@@ -406,6 +406,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
 	memset(&mac->phylink_config, 0, sizeof(mac->phylink_config));
 	mac->phylink_config.dev = &net_dev->dev;
 	mac->phylink_config.type = PHYLINK_NETDEV;
+	mac->phylink_config.sync_an_inband = true;
 
 	mac->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
 		MAC_10FD | MAC_100FD | MAC_1000FD | MAC_2500FD | MAC_5000FD |
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
index 9f6c4f5c0a6c..c0d4fff00987 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
@@ -1126,6 +1126,7 @@ static int enetc_phylink_create(struct enetc_ndev_priv *priv,
 
 	pf->phylink_config.dev = &priv->ndev->dev;
 	pf->phylink_config.type = PHYLINK_NETDEV;
+	pf->phylink_config.sync_an_inband = true;
 	pf->phylink_config.mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
 		MAC_10 | MAC_100 | MAC_1000 | MAC_2500FD;
 
diff --git a/drivers/net/ethernet/freescale/fman/fman_memac.c b/drivers/net/ethernet/freescale/fman/fman_memac.c
index 9349f841bd06..e4a707a7d7f4 100644
--- a/drivers/net/ethernet/freescale/fman/fman_memac.c
+++ b/drivers/net/ethernet/freescale/fman/fman_memac.c
@@ -1075,7 +1075,6 @@ int memac_initialization(struct mac_device *mac_dev,
 			 struct fman_mac_params *params)
 {
 	int			 err;
-	struct device_node      *fixed;
 	struct phylink_pcs	*pcs;
 	struct fman_mac		*memac;
 	unsigned long		 capabilities;
@@ -1219,6 +1218,7 @@ int memac_initialization(struct mac_device *mac_dev,
 		capabilities &= ~(MAC_10HD | MAC_100HD);
 
 	mac_dev->phylink_config.mac_capabilities = capabilities;
+	mac_dev->phylink_config.sync_an_inband = true;
 
 	/* The T2080 and T4240 don't support half duplex RGMII. There is no
 	 * other way to identify these SoCs, so just use the machine
@@ -1231,20 +1231,6 @@ int memac_initialization(struct mac_device *mac_dev,
 	    of_machine_is_compatible("fsl,T4240RDB"))
 		memac->rgmii_no_half_duplex = true;
 
-	/* Most boards should use MLO_AN_INBAND, but existing boards don't have
-	 * a managed property. Default to MLO_AN_INBAND if nothing else is
-	 * specified. We need to be careful and not enable this if we have a
-	 * fixed link or if we are using MII or RGMII, since those
-	 * configurations modes don't use in-band autonegotiation.
-	 */
-	fixed = of_get_child_by_name(mac_node, "fixed-link");
-	if (!fixed && !of_property_read_bool(mac_node, "fixed-link") &&
-	    !of_property_read_bool(mac_node, "managed") &&
-	    mac_dev->phy_if != PHY_INTERFACE_MODE_MII &&
-	    !phy_interface_mode_is_rgmii(mac_dev->phy_if))
-		mac_dev->phylink_config.ovr_an_inband = true;
-	of_node_put(fixed);
-
 	err = memac_init(mac_dev->fman_mac);
 	if (err < 0)
 		goto _return_fm_mac_free;
-- 
2.34.1

Re: [PATCH v4 net-next 0/8] Let phylink manage in-band AN for the PHY

[Sean Anderson]

Hi Vladimir,

On 11/17/22 19:01, Vladimir Oltean wrote:
> Problem statement
> ~~~~~~~~~~~~~~~~~
> 
> The on-board SERDES link between an NXP (Lynx) PCS and a PHY may not
> work, depending on whether U-Boot networking was used on that port or not.
> 
> There is no mechanism in Linux (with phylib/phylink, at least) to ensure
> that the MAC driver and the PHY driver have synchronized settings for
> in-band autoneg. It all depends on the 'managed = "in-band-status"'
> device tree property, which does not reflect a stable and unchanging
> reality, and furthermore, some (older) device trees may have this
> property missing when they shouldn't.
> 
> Proposed solution
> ~~~~~~~~~~~~~~~~~
> 
> Extend the phy_device API with 2 new methods:
> - phy_validate_an_inband()
> - phy_config_an_inband()
> 
> Extend phylink with an opt-in bool sync_an_inband which makes sure that
> the configured "unsigned int mode" (MLO_AN_PHY/MLO_AN_INBAND) is both
> supported by the PHY, and actually applied to the PHY.
> 
> Make NXP drivers which use phylink and the Lynx PCS driver opt into the
> new behavior. Other drivers can trivially do this as well, by setting
> struct phylink_config :: sync_an_inband to true.
> 
> Compared to other solutions
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Sean Anderson, in commit 5d93cfcf7360 ("net: dpaa: Convert to phylink"),
> sets phylink_config :: ovr_an_inband to true. This doesn't quite solve
> all problems, because we don't *know* that the PHY is set for in-band
> autoneg. For example with the VSC8514, it all depends on what the
> bootloader has/has not done. This solution eliminates the bootloader
> dependency by actually programming in-band autoneg in the VSC8514 PHY.
> 
> Change log
> ~~~~~~~~~~
> 
> Changes in v4:
> Make all new behavior opt-in.
> Fix bug when Generic PHY driver is used.
> Dropped support for PHY_AN_INBAND_OFF in at803x.
> 
> Changes in v3:
> Added patch for the Atheros PHY family.
> v3 at:
> https://patchwork.kernel.org/project/netdevbpf/cover/20210922181446.2677089-1-vladimir.oltean@nxp.com/
> 
> Changes in v2:
> Incorporated feedback from Russell, which was to consider PHYs on SFP
> modules too, and unify phylink's detection of PHYs with broken in-band
> autoneg with the newly introduced PHY driver methods.
> v2 at:
> https://patchwork.kernel.org/project/netdevbpf/cover/20210212172341.3489046-1-olteanv@gmail.com/
> 
> Vladimir Oltean (8):
>   net: phylink: let phylink_sfp_config_phy() determine the MLO_AN_* mode
>     to use
>   net: phylink: introduce generic method to query PHY in-band autoneg
>     capability
>   net: phy: bcm84881: move the in-band capability check where it belongs
>   net: phylink: add option to sync in-band autoneg setting between PCS
>     and PHY
>   net: phylink: explicitly configure in-band autoneg for on-board PHYs
>   net: phy: mscc: configure in-band auto-negotiation for VSC8514
>   net: phy: at803x: validate in-band autoneg for AT8031/AT8033
>   net: opt MAC drivers which use Lynx PCS into phylink sync_an_inband
> 
>  drivers/net/dsa/ocelot/felix.c                |  2 +
>  .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  |  1 +
>  .../net/ethernet/freescale/enetc/enetc_pf.c   |  1 +
>  .../net/ethernet/freescale/fman/fman_memac.c  | 16 +--
>  drivers/net/phy/at803x.c                      | 10 ++
>  drivers/net/phy/bcm84881.c                    | 10 ++
>  drivers/net/phy/mscc/mscc.h                   |  2 +
>  drivers/net/phy/mscc/mscc_main.c              | 21 ++++
>  drivers/net/phy/phy.c                         | 51 ++++++++++
>  drivers/net/phy/phylink.c                     | 97 +++++++++++++++----
>  include/linux/phy.h                           | 27 ++++++
>  include/linux/phylink.h                       |  7 ++
>  12 files changed, 212 insertions(+), 33 deletions(-)
> 

I tested this on an LS1046ARDB. Unfortunately, although the links came
up, the SGMII interfaces could not transfer data:

# dmesg | grep net6
[    3.846249] fsl_dpaa_mac 1aea000.ethernet net6: renamed from eth3
[    5.047334] fsl_dpaa_mac 1aea000.ethernet net6: PHY driver does not report in-band autoneg capability, assuming false
[    5.073739] fsl_dpaa_mac 1aea000.ethernet net6: PHY [0x0000000001afc000:04] driver [RTL8211F Gigabit Ethernet] (irq=POLL)
[    5.073749] fsl_dpaa_mac 1aea000.ethernet net6: phy: sgmii setting supported 0,00000000,00000000,000062ea advertising 0,00000000,00000000,000062ea
[    5.073798] fsl_dpaa_mac 1aea000.ethernet net6: configuring for phy/sgmii link mode
[    5.073803] fsl_dpaa_mac 1aea000.ethernet net6: major config sgmii
[    5.075369] fsl_dpaa_mac 1aea000.ethernet net6: phylink_mac_config: mode=phy/sgmii/Unknown/Unknown/none adv=0,00000000,00000000,00000000 pause=00 link=0 an=0
[    5.102308] fsl_dpaa_mac 1aea000.ethernet net6: phy link down sgmii/Unknown/Unknown/none/off
[    9.186216] fsl_dpaa_mac 1aea000.ethernet net6: phy link up sgmii/1Gbps/Full/none/rx/tx
[    9.186261] fsl_dpaa_mac 1aea000.ethernet net6: Link is Up - 1Gbps/Full - flow control rx/tx

I added a `managed = "in-band-status";` property, and this time data
transfer worked:

# dmesg | grep net6
[    3.826156] fsl_dpaa_mac 1aea000.ethernet net6: renamed from eth3
[    5.062654] fsl_dpaa_mac 1aea000.ethernet net6: PHY driver does not report in-band autoneg capability, assuming true
[    5.089724] fsl_dpaa_mac 1aea000.ethernet net6: PHY [0x0000000001afc000:04] driver [RTL8211F Gigabit Ethernet] (irq=POLL)
[    5.089734] fsl_dpaa_mac 1aea000.ethernet net6: phy: sgmii setting supported 0,00000000,00000000,000062ea advertising 0,00000000,00000000,000062ea
[    5.089782] fsl_dpaa_mac 1aea000.ethernet net6: configuring for inband/sgmii link mode
[    5.089786] fsl_dpaa_mac 1aea000.ethernet net6: major config sgmii
[    5.090951] fsl_dpaa_mac 1aea000.ethernet net6: phylink_mac_config: mode=inband/sgmii/Unknown/Unknown/none adv=0,00000000,00000000,000062ea pause=00 link=0 an=1
[    5.118325] fsl_dpaa_mac 1aea000.ethernet net6: phy link down sgmii/Unknown/Unknown/none/off
[    9.214204] fsl_dpaa_mac 1aea000.ethernet net6: phy link up sgmii/1Gbps/Full/none/rx/tx
[    9.214247] fsl_dpaa_mac 1aea000.ethernet net6: Link is Up - 1Gbps/Full - flow control rx/tx

I believe this is the same issue I ran into before. This is why I
defaulted to in-band.

--Sean

Re: [PATCH v4 net-next 0/8] Let phylink manage in-band AN for the PHY

[Vladimir Oltean]

Hi Sean,

On Mon, Nov 21, 2022 at 01:38:31PM -0500, Sean Anderson wrote:
> On 11/17/22 19:01, Vladimir Oltean wrote:
> > Compared to other solutions
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > Sean Anderson, in commit 5d93cfcf7360 ("net: dpaa: Convert to phylink"),
> > sets phylink_config :: ovr_an_inband to true. This doesn't quite solve
> > all problems, because we don't *know* that the PHY is set for in-band
> > autoneg. For example with the VSC8514, it all depends on what the
> > bootloader has/has not done. This solution eliminates the bootloader
> > dependency by actually programming in-band autoneg in the VSC8514 PHY.
> 
> I tested this on an LS1046ARDB. Unfortunately, although the links came
> up, the SGMII interfaces could not transfer data:
> 
> # dmesg | grep net6
> [    3.846249] fsl_dpaa_mac 1aea000.ethernet net6: renamed from eth3
> [    5.047334] fsl_dpaa_mac 1aea000.ethernet net6: PHY driver does not report in-band autoneg capability, assuming false
> [    5.073739] fsl_dpaa_mac 1aea000.ethernet net6: PHY [0x0000000001afc000:04] driver [RTL8211F Gigabit Ethernet] (irq=POLL)
> [    5.073749] fsl_dpaa_mac 1aea000.ethernet net6: phy: sgmii setting supported 0,00000000,00000000,000062ea advertising 0,00000000,00000000,000062ea
> [    5.073798] fsl_dpaa_mac 1aea000.ethernet net6: configuring for phy/sgmii link mode
> [    5.073803] fsl_dpaa_mac 1aea000.ethernet net6: major config sgmii
> [    5.075369] fsl_dpaa_mac 1aea000.ethernet net6: phylink_mac_config: mode=phy/sgmii/Unknown/Unknown/none adv=0,00000000,00000000,00000000 pause=00 link=0 an=0
> [    5.102308] fsl_dpaa_mac 1aea000.ethernet net6: phy link down sgmii/Unknown/Unknown/none/off
> [    9.186216] fsl_dpaa_mac 1aea000.ethernet net6: phy link up sgmii/1Gbps/Full/none/rx/tx
> [    9.186261] fsl_dpaa_mac 1aea000.ethernet net6: Link is Up - 1Gbps/Full - flow control rx/tx
> 
> I believe this is the same issue I ran into before. This is why I
> defaulted to in-band.

Thanks for testing. Somehow it did not come to me that this kind of
issue might happen when converting a driver that used to use ovr_an_inband
such as dpaa1, but ok, here we are.

The problem, of course, is that the Realtek PHY driver does not report
what the hardware supports, and we're back to trusting the device tree.

I don't think there were that many more PHYs used on NXP evaluation
boards than the Realteks, but of course there are also customer boards
to consider. Considering past history, it might be safer in terms of
regressions to use ovr_an_inband, but eventually, getting regression
reports in is going to make more PHY drivers report their capabilities,
which will improve the situation.

Anyways, we can still keep dpaa1 unconverted for now, and maybe convert
it for the next release cycle.

I also thought of a way of logically combining ovr_an_inband with
sync_an_inband (like say that ovr_an_inband is a "soft" override, and it
only takes place if syncing is not possible), but I'm not sure if that
isn't in fact overkill.

Could you please test the patch below? I only compile-tested it:

-----------------------------[ cut here ]-----------------------------
From 025f8dedf10defa6d5fd10b4e3dd2a505fdbd313 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Mon, 21 Nov 2022 21:34:20 +0200
Subject: [PATCH] net: phy: realtek: validate SGMII in-band autoneg for
 RTL8211FS

Sean Anderson reports that the RTL8211FS on the NXP LS1046A-RDB has
in-band autoneg enabled, and this needs to be detectable by phylink if
the dpaa1 driver is going to use the sync_an_inband mechanism rather
than forcing in-band on via ovr_an_inband.

Reading through the datasheet, it seems like the SGMII Auto-Negotiation
Advertising Register bit 11 (En_Select Link Info) might be responsible
with this.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/phy/realtek.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 3d99fd6664d7..53e7c1a10ab4 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -24,6 +24,10 @@
 
 #define RTL821x_INSR				0x13
 
+#define RTL8211FS_SGMII_ANARSEL			0x14
+
+#define RTL8211FS_SGMII_ANARSEL_EN		BIT(11)
+
 #define RTL821x_EXT_PAGE_SELECT			0x1e
 #define RTL821x_PAGE_SELECT			0x1f
 
@@ -849,6 +853,30 @@ static irqreturn_t rtl9000a_handle_interrupt(struct phy_device *phydev)
 	return IRQ_HANDLED;
 }
 
+/* RTL8211F and RTL8211FS seem to have the same PHY ID. We really only mean to
+ * run this for the S model which supports SGMII, so report unknown for
+ * everything else.
+ */
+static int rtl8211fs_validate_an_inband(struct phy_device *phydev,
+					phy_interface_t interface)
+{
+	int ret;
+
+	if (interface != PHY_INTERFACE_MODE_SGMII)
+		return PHY_AN_INBAND_UNKNOWN;
+
+	ret = phy_read_paged(phydev, 0xd08, RTL8211FS_SGMII_ANARSEL);
+	if (ret < 0)
+		return ret;
+
+	phydev_err(phydev, "%s: SGMII_ANARSEL 0x%x\n", __func__, ret);
+
+	if (ret & RTL8211FS_SGMII_ANARSEL_EN)
+		return PHY_AN_INBAND_ON;
+
+	return PHY_AN_INBAND_OFF;
+}
+
 static struct phy_driver realtek_drvs[] = {
 	{
 		PHY_ID_MATCH_EXACT(0x00008201),
@@ -931,6 +959,7 @@ static struct phy_driver realtek_drvs[] = {
 		.resume		= rtl821x_resume,
 		.read_page	= rtl821x_read_page,
 		.write_page	= rtl821x_write_page,
+		.validate_an_inband = rtl8211fs_validate_an_inband,
 	}, {
 		PHY_ID_MATCH_EXACT(RTL_8211FVD_PHYID),
 		.name		= "RTL8211F-VD Gigabit Ethernet",
-- 
2.34.1

-----------------------------[ cut here ]-----------------------------

Re: [PATCH v4 net-next 0/8] Let phylink manage in-band AN for the PHY

[Sean Anderson]

On 11/21/22 14:44, Vladimir Oltean wrote:
> Hi Sean,
> 
> On Mon, Nov 21, 2022 at 01:38:31PM -0500, Sean Anderson wrote:
>> On 11/17/22 19:01, Vladimir Oltean wrote:
>> > Compared to other solutions
>> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> > 
>> > Sean Anderson, in commit 5d93cfcf7360 ("net: dpaa: Convert to phylink"),
>> > sets phylink_config :: ovr_an_inband to true. This doesn't quite solve
>> > all problems, because we don't *know* that the PHY is set for in-band
>> > autoneg. For example with the VSC8514, it all depends on what the
>> > bootloader has/has not done. This solution eliminates the bootloader
>> > dependency by actually programming in-band autoneg in the VSC8514 PHY.
>> 
>> I tested this on an LS1046ARDB. Unfortunately, although the links came
>> up, the SGMII interfaces could not transfer data:
>> 
>> # dmesg | grep net6
>> [    3.846249] fsl_dpaa_mac 1aea000.ethernet net6: renamed from eth3
>> [    5.047334] fsl_dpaa_mac 1aea000.ethernet net6: PHY driver does not report in-band autoneg capability, assuming false
>> [    5.073739] fsl_dpaa_mac 1aea000.ethernet net6: PHY [0x0000000001afc000:04] driver [RTL8211F Gigabit Ethernet] (irq=POLL)
>> [    5.073749] fsl_dpaa_mac 1aea000.ethernet net6: phy: sgmii setting supported 0,00000000,00000000,000062ea advertising 0,00000000,00000000,000062ea
>> [    5.073798] fsl_dpaa_mac 1aea000.ethernet net6: configuring for phy/sgmii link mode
>> [    5.073803] fsl_dpaa_mac 1aea000.ethernet net6: major config sgmii
>> [    5.075369] fsl_dpaa_mac 1aea000.ethernet net6: phylink_mac_config: mode=phy/sgmii/Unknown/Unknown/none adv=0,00000000,00000000,00000000 pause=00 link=0 an=0
>> [    5.102308] fsl_dpaa_mac 1aea000.ethernet net6: phy link down sgmii/Unknown/Unknown/none/off
>> [    9.186216] fsl_dpaa_mac 1aea000.ethernet net6: phy link up sgmii/1Gbps/Full/none/rx/tx
>> [    9.186261] fsl_dpaa_mac 1aea000.ethernet net6: Link is Up - 1Gbps/Full - flow control rx/tx
>> 
>> I believe this is the same issue I ran into before. This is why I
>> defaulted to in-band.
> 
> Thanks for testing. Somehow it did not come to me that this kind of
> issue might happen when converting a driver that used to use ovr_an_inband
> such as dpaa1, but ok, here we are.
> 
> The problem, of course, is that the Realtek PHY driver does not report
> what the hardware supports, and we're back to trusting the device tree.
> 
> I don't think there were that many more PHYs used on NXP evaluation
> boards than the Realteks, but of course there are also customer boards
> to consider. Considering past history, it might be safer in terms of
> regressions to use ovr_an_inband, but eventually, getting regression
> reports in is going to make more PHY drivers report their capabilities,
> which will improve the situation.

Are you certain this is the cause of the issue? It's also possible that
there is some errata for the PCS which is causing the issue. I have
gotten no review/feedback from NXP regarding the phylink conversion
(aside from acks for the cleanups).

> Anyways, we can still keep dpaa1 unconverted for now, and maybe convert
> it for the next release cycle.
> 
> I also thought of a way of logically combining ovr_an_inband with
> sync_an_inband (like say that ovr_an_inband is a "soft" override, and it
> only takes place if syncing is not possible), but I'm not sure if that
> isn't in fact overkill.
>
> Could you please test the patch below? I only compile-tested it:
> 
> -----------------------------[ cut here ]-----------------------------
> From 025f8dedf10defa6d5fd10b4e3dd2a505fdbd313 Mon Sep 17 00:00:00 2001
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> Date: Mon, 21 Nov 2022 21:34:20 +0200
> Subject: [PATCH] net: phy: realtek: validate SGMII in-band autoneg for
>  RTL8211FS
> 
> Sean Anderson reports that the RTL8211FS on the NXP LS1046A-RDB has
> in-band autoneg enabled, and this needs to be detectable by phylink if
> the dpaa1 driver is going to use the sync_an_inband mechanism rather
> than forcing in-band on via ovr_an_inband.
> 
> Reading through the datasheet, it seems like the SGMII Auto-Negotiation
> Advertising Register bit 11 (En_Select Link Info) might be responsible
> with this.

This is used for SGMII to RGMII bridge mode (figure 4). It doesn't seem
to contain useful information for UTP mode (figure 1).

> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/phy/realtek.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index 3d99fd6664d7..53e7c1a10ab4 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -24,6 +24,10 @@
>  
>  #define RTL821x_INSR				0x13
>  
> +#define RTL8211FS_SGMII_ANARSEL			0x14
> +
> +#define RTL8211FS_SGMII_ANARSEL_EN		BIT(11)
> +
>  #define RTL821x_EXT_PAGE_SELECT			0x1e
>  #define RTL821x_PAGE_SELECT			0x1f
>  
> @@ -849,6 +853,30 @@ static irqreturn_t rtl9000a_handle_interrupt(struct phy_device *phydev)
>  	return IRQ_HANDLED;
>  }
>  
> +/* RTL8211F and RTL8211FS seem to have the same PHY ID. We really only mean to
> + * run this for the S model which supports SGMII, so report unknown for
> + * everything else.
> + */
> +static int rtl8211fs_validate_an_inband(struct phy_device *phydev,
> +					phy_interface_t interface)
> +{
> +	int ret;
> +
> +	if (interface != PHY_INTERFACE_MODE_SGMII)
> +		return PHY_AN_INBAND_UNKNOWN;
> +
> +	ret = phy_read_paged(phydev, 0xd08, RTL8211FS_SGMII_ANARSEL);

That said, you have to use the "Indirect access method" to access this
register (per section 8.5). This is something like

#define RTL8211F_IAAR				0x1b
#define RTL8211F_IADR				0x1c

#define RTL8211F_IAAR_PAGE			GENMASK(15, 4)
#define RTL8211F_IAAR_REG			GENMASK(3, 1)
#define INDIRECT_ADDRESS(page, reg) \
	(FIELD_PREP(RTL8211F_IAAR_PAGE, page) | \
	 FIELD_PREP(RTL8211F_IAAR_REG, reg - 16))

	ret = phy_write_paged(phydev, 0xa43, RTL8211F_IAAR,
			      INDIRECT_ADDRESS(0xd08, RTL8211FS_SGMII_ANARSEL));
	if (ret < 0)
		return ret;

	ret = phy_read_paged(phydev, 0xa43, RTL8211F_IADR);
	if (ret < 0)
		return ret;

I dumped the rest of the serdes registers using this method, but I
didn't see anything interesting (all defaults). I think it would be
better to just return PHY_AN_INBAND_ON when using SGMII.

--Sean

> +	if (ret < 0)
> +		return ret;
> +
> +	phydev_err(phydev, "%s: SGMII_ANARSEL 0x%x\n", __func__, ret);
> +
> +	if (ret & RTL8211FS_SGMII_ANARSEL_EN)
> +		return PHY_AN_INBAND_ON;
> +
> +	return PHY_AN_INBAND_OFF;
> +}
> +
>  static struct phy_driver realtek_drvs[] = {
>  	{
>  		PHY_ID_MATCH_EXACT(0x00008201),
> @@ -931,6 +959,7 @@ static struct phy_driver realtek_drvs[] = {
>  		.resume		= rtl821x_resume,
>  		.read_page	= rtl821x_read_page,
>  		.write_page	= rtl821x_write_page,
> +		.validate_an_inband = rtl8211fs_validate_an_inband,
>  	}, {
>  		PHY_ID_MATCH_EXACT(RTL_8211FVD_PHYID),
>  		.name		= "RTL8211F-VD Gigabit Ethernet",

Re: [PATCH v4 net-next 0/8] Let phylink manage in-band AN for the PHY

[Vladimir Oltean]

On Mon, Nov 21, 2022 at 05:42:44PM -0500, Sean Anderson wrote:
> Are you certain this is the cause of the issue? It's also possible that
> there is some errata for the PCS which is causing the issue. I have
> gotten no review/feedback from NXP regarding the phylink conversion
> (aside from acks for the cleanups).

Erratum which does what out of the ordinary? Your description of the
hardware failure seems consistent with the most plausible explanation
that doesn't involve any bugs.

If you enable C37/SGMII AN in the PCS (of the PHY or of the MAC) and AN
does not complete (because it's not enabled on the other end), that
system side of the link remains down. Which you don't see when you
operate in MLO_AN_PHY mode, because phylink only considers the PCS link
state in MLO_AN_INBAND mode. So this is why you see the link as up but
it doesn't work.

To confirm whether I'm right or wrong, there's a separate SERDES
Interrupt Status Register at page 0xde1 offset 0x12, whose bit 4 is
"SERDES link status change" and bit 0 is "SERDES auto-negotiation error".
These bits should both be set when you double-read them (regardless of
IRQ enable I think) when your link is down with MLO_AN_PHY, but should
be cleared with MLO_AN_INBAND.

> This is used for SGMII to RGMII bridge mode (figure 4). It doesn't seem
> to contain useful information for UTP mode (figure 1).

So it would seem. It was a hasty read last time, sorry. Re-reading, the
field says that when it's set, the SGMII code word being transmitted is
"selected by the register" SGMII ANAR. And in the SGMII ANLPAR, you can
see what the MAC said.

Of course, it doesn't say what happens when the bit for software-driven
SGMII autoneg is *not* set, if the process can be at all bypassed.
I suppose now that it can't, otherwise the ANLPAR register could also be
writable over MDIO, they would have likely reused at least partly the
same mechanisms.

> > +	ret = phy_read_paged(phydev, 0xd08, RTL8211FS_SGMII_ANARSEL);
> 
> That said, you have to use the "Indirect access method" to access this
> register (per section 8.5). This is something like
> 
> #define RTL8211F_IAAR				0x1b
> #define RTL8211F_IADR				0x1c
> 
> #define RTL8211F_IAAR_PAGE			GENMASK(15, 4)
> #define RTL8211F_IAAR_REG			GENMASK(3, 1)
> #define INDIRECT_ADDRESS(page, reg) \
> 	(FIELD_PREP(RTL8211F_IAAR_PAGE, page) | \
> 	 FIELD_PREP(RTL8211F_IAAR_REG, reg - 16))
> 
> 	ret = phy_write_paged(phydev, 0xa43, RTL8211F_IAAR,
> 			      INDIRECT_ADDRESS(0xd08, RTL8211FS_SGMII_ANARSEL));
> 	if (ret < 0)
> 		return ret;
> 
> 	ret = phy_read_paged(phydev, 0xa43, RTL8211F_IADR);
> 	if (ret < 0)
> 		return ret;
> 
> I dumped the rest of the serdes registers using this method, but I
> didn't see anything interesting (all defaults).

I'm _really_ not sure where you got the "Indirect access method" via
registers 0x1b/0x1c from. My datasheet for RTL8211FS doesn't show
offsets 0x1b and 0x1c in page 0xa43. Additionally, I cross-checked with
other registers that are accessed by the driver (like the Interrupt
Enable Register), and the driver access procedure -
phy_write_paged(phydev, 0xa42, RTL821x_INER, val) - seems to be pretty
much in line with what my datasheet shows.

> I think it would be better to just return PHY_AN_INBAND_ON when using
> SGMII.

Well, of course hardcoding PHY_AN_INBAND_ON in the driver is on the
table, if it isn't possible to alter this setting to the best of our
knowledge (or if it's implausible that someone modified it). And this
seems more and more like the case.

Re: [PATCH v4 net-next 0/8] Let phylink manage in-band AN for the PHY

[Sean Anderson]

On 11/21/22 19:17, Vladimir Oltean wrote:
> On Mon, Nov 21, 2022 at 05:42:44PM -0500, Sean Anderson wrote:
>> Are you certain this is the cause of the issue? It's also possible that
>> there is some errata for the PCS which is causing the issue. I have
>> gotten no review/feedback from NXP regarding the phylink conversion
>> (aside from acks for the cleanups).
> 
> Erratum which does what out of the ordinary? Your description of the
> hardware failure seems consistent with the most plausible explanation
> that doesn't involve any bugs.

Well, I don't have a setup which doesn't require in-band AN, so I can't
say one way or the other where the problem lies. To me, the Lynx PCS is
just as opaque as the phy.

> If you enable C37/SGMII AN in the PCS (of the PHY or of the MAC) and AN
> does not complete (because it's not enabled on the other end), that
> system side of the link remains down. Which you don't see when you
> operate in MLO_AN_PHY mode, because phylink only considers the PCS link
> state in MLO_AN_INBAND mode. So this is why you see the link as up but
> it doesn't work.

Actually, I checked the PCS manually in phy mode, and the link was up.
I expected it to be down, so this was a bit surprising to me.

> To confirm whether I'm right or wrong, there's a separate SERDES
> Interrupt Status Register at page 0xde1 offset 0x12, whose bit 4 is
> "SERDES link status change" and bit 0 is "SERDES auto-negotiation error".
> These bits should both be set when you double-read them (regardless of
> IRQ enable I think) when your link is down with MLO_AN_PHY, but should
> be cleared with MLO_AN_INBAND.

This register is always 0s for me...

>> This is used for SGMII to RGMII bridge mode (figure 4). It doesn't seem
>> to contain useful information for UTP mode (figure 1).
> 
> So it would seem. It was a hasty read last time, sorry. Re-reading, the
> field says that when it's set, the SGMII code word being transmitted is
> "selected by the register" SGMII ANAR. And in the SGMII ANLPAR, you can
> see what the MAC said.

... possibly because of this.

That said, ANLPAR is 0x4001 (all reserved bits) when we use in-band:

[    8.191146] RTL8211F Gigabit Ethernet 0x0000000001afc000:04: INER=0000 INSR=0000 ANARSEL=0000 ANAR=0050 ANLPAR=4001

but all zeros without:

[   11.263245] RTL8211F Gigabit Ethernet 0x0000000001afc000:04: INER=0000 INSR=0000 ANARSEL=0000 ANAR=0050 ANLPAR=0000

It's all 1s when using RGMII. These bits are reserved, so it's not that
interesting, but maybe these registers are not as useless as they seem.

> Of course, it doesn't say what happens when the bit for software-driven
> SGMII autoneg is *not* set, if the process can be at all bypassed.
> I suppose now that it can't, otherwise the ANLPAR register could also be
> writable over MDIO, they would have likely reused at least partly the
> same mechanisms.
> 
>> > +	ret = phy_read_paged(phydev, 0xd08, RTL8211FS_SGMII_ANARSEL);
>> 
>> That said, you have to use the "Indirect access method" to access this
>> register (per section 8.5). This is something like
>> 
>> #define RTL8211F_IAAR				0x1b
>> #define RTL8211F_IADR				0x1c
>> 
>> #define RTL8211F_IAAR_PAGE			GENMASK(15, 4)
>> #define RTL8211F_IAAR_REG			GENMASK(3, 1)
>> #define INDIRECT_ADDRESS(page, reg) \
>> 	(FIELD_PREP(RTL8211F_IAAR_PAGE, page) | \
>> 	 FIELD_PREP(RTL8211F_IAAR_REG, reg - 16))
>> 
>> 	ret = phy_write_paged(phydev, 0xa43, RTL8211F_IAAR,
>> 			      INDIRECT_ADDRESS(0xd08, RTL8211FS_SGMII_ANARSEL));
>> 	if (ret < 0)
>> 		return ret;
>> 
>> 	ret = phy_read_paged(phydev, 0xa43, RTL8211F_IADR);
>> 	if (ret < 0)
>> 		return ret;
>> 
>> I dumped the rest of the serdes registers using this method, but I
>> didn't see anything interesting (all defaults).
> 
> I'm _really_ not sure where you got the "Indirect access method" via
> registers 0x1b/0x1c from.

Huh. Looks like this is a second case of differing datasheets. Mine is
revision 1.8 dated 2021-04-21. The documentation for indirect access was
added in revision 1.7 dated 2020-07-08. Although it seems like the
SERDES registers were also added in this revision, so maybe you just
missed this section?

> My datasheet for RTL8211FS doesn't show
> offsets 0x1b and 0x1c in page 0xa43.

Neither does mine. These registers are only documented by reference from
section 8.5. They also aren't named, so the above defines are my own
coinage.

> Additionally, I cross-checked with
> other registers that are accessed by the driver (like the Interrupt
> Enable Register), and the driver access procedure -
> phy_write_paged(phydev, 0xa42, RTL821x_INER, val) - seems to be pretty
> much in line with what my datasheet shows.

| The SERDES related registers should be read and written through indirect
| access method. The registers include Page 0xdc0 to Page 0xdcf and Page
| 0xde0 to Page 0xdf0.

Each register accessed this way also has

| Note: This register requires indirect access.

below the register table.

>> I think it would be better to just return PHY_AN_INBAND_ON when using
>> SGMII.
> 
> Well, of course hardcoding PHY_AN_INBAND_ON in the driver is on the
> table, if it isn't possible to alter this setting to the best of our
> knowledge (or if it's implausible that someone modified it). And this
> seems more and more like the case.

I meant something like

	if (interface == PHY_INTERFACE_MODE_SGMII)
		return PHY_AN_INBAND_ON;

	return PHY_AN_INBAND_UNKNOWN;

Although for RGMII, in-band status is (per MIICR2):

- Enabled by default
- Disablable
- Optional

So maybe we should do (PHY_AN_INBAND_ON | PHY_AN_INBAND_OFF) in that
case. That said, RGMII in-band is not supported by phylink (yet).

--Sean

Re: [PATCH v4 net-next 0/8] Let phylink manage in-band AN for the PHY

[Vladimir Oltean]

On Tue, Nov 22, 2022 at 11:10:03AM -0500, Sean Anderson wrote:
> On 11/21/22 19:17, Vladimir Oltean wrote:
> > On Mon, Nov 21, 2022 at 05:42:44PM -0500, Sean Anderson wrote:
> >> Are you certain this is the cause of the issue? It's also possible that
> >> there is some errata for the PCS which is causing the issue. I have
> >> gotten no review/feedback from NXP regarding the phylink conversion
> >> (aside from acks for the cleanups).
> > 
> > Erratum which does what out of the ordinary? Your description of the
> > hardware failure seems consistent with the most plausible explanation
> > that doesn't involve any bugs.
> 
> Well, I don't have a setup which doesn't require in-band AN, so I can't
> say one way or the other where the problem lies. To me, the Lynx PCS is
> just as opaque as the phy.

ok.

> > If you enable C37/SGMII AN in the PCS (of the PHY or of the MAC) and AN
> > does not complete (because it's not enabled on the other end), that
> > system side of the link remains down. Which you don't see when you
> > operate in MLO_AN_PHY mode, because phylink only considers the PCS link
> > state in MLO_AN_INBAND mode. So this is why you see the link as up but
> > it doesn't work.
> 
> Actually, I checked the PCS manually in phy mode, and the link was up.
> I expected it to be down, so this was a bit surprising to me.

Well, if autoneg is disabled in the Lynx PCS (which it is in MLO_AN_PHY),
then the link should come up right away, as long as it can lock on some
symbols IIRC. It's a different story for the PHY PCS if autoneg is
enabled there. Still nothing surprising here, really.

> > To confirm whether I'm right or wrong, there's a separate SERDES
> > Interrupt Status Register at page 0xde1 offset 0x12, whose bit 4 is
> > "SERDES link status change" and bit 0 is "SERDES auto-negotiation error".
> > These bits should both be set when you double-read them (regardless of
> > IRQ enable I think) when your link is down with MLO_AN_PHY, but should
> > be cleared with MLO_AN_INBAND.
> 
> This register is always 0s for me...
> 
> >> This is used for SGMII to RGMII bridge mode (figure 4). It doesn't seem
> >> to contain useful information for UTP mode (figure 1).
> > 
> > So it would seem. It was a hasty read last time, sorry. Re-reading, the
> > field says that when it's set, the SGMII code word being transmitted is
> > "selected by the register" SGMII ANAR. And in the SGMII ANLPAR, you can
> > see what the MAC said.
> 
> ... possibly because of this.
> 
> That said, ANLPAR is 0x4001 (all reserved bits) when we use in-band:
> 
> [    8.191146] RTL8211F Gigabit Ethernet 0x0000000001afc000:04: INER=0000 INSR=0000 ANARSEL=0000 ANAR=0050 ANLPAR=4001
> 
> but all zeros without:
> 
> [   11.263245] RTL8211F Gigabit Ethernet 0x0000000001afc000:04: INER=0000 INSR=0000 ANARSEL=0000 ANAR=0050 ANLPAR=0000

So enabling in-band autoneg in the Lynx PCS does what you'd expect it to do.
I don't know why you don't get a "SERDES auto-negotiation error" bit in
the interrupt status register. Maybe you need to first enable it in the
interrupt enable register?! Who knows. Not sure how far it's worth
diving into this.

> It's all 1s when using RGMII. These bits are reserved, so it's not that
> interesting, but maybe these registers are not as useless as they seem.

Yeah, with RGMII I don't know if the PHY responds to the SERDES registers
over MDIO. All ones may mean the MDIO bus pull-ups.

> > Of course, it doesn't say what happens when the bit for software-driven
> > SGMII autoneg is *not* set, if the process can be at all bypassed.
> > I suppose now that it can't, otherwise the ANLPAR register could also be
> > writable over MDIO, they would have likely reused at least partly the
> > same mechanisms.
> > 
> >> > +	ret = phy_read_paged(phydev, 0xd08, RTL8211FS_SGMII_ANARSEL);
> >> 
> >> That said, you have to use the "Indirect access method" to access this
> >> register (per section 8.5). This is something like
> >> 
> >> #define RTL8211F_IAAR				0x1b
> >> #define RTL8211F_IADR				0x1c
> >> 
> >> #define RTL8211F_IAAR_PAGE			GENMASK(15, 4)
> >> #define RTL8211F_IAAR_REG			GENMASK(3, 1)
> >> #define INDIRECT_ADDRESS(page, reg) \
> >> 	(FIELD_PREP(RTL8211F_IAAR_PAGE, page) | \
> >> 	 FIELD_PREP(RTL8211F_IAAR_REG, reg - 16))
> >> 
> >> 	ret = phy_write_paged(phydev, 0xa43, RTL8211F_IAAR,
> >> 			      INDIRECT_ADDRESS(0xd08, RTL8211FS_SGMII_ANARSEL));
> >> 	if (ret < 0)
> >> 		return ret;
> >> 
> >> 	ret = phy_read_paged(phydev, 0xa43, RTL8211F_IADR);
> >> 	if (ret < 0)
> >> 		return ret;
> >> 
> >> I dumped the rest of the serdes registers using this method, but I
> >> didn't see anything interesting (all defaults).
> > 
> > I'm _really_ not sure where you got the "Indirect access method" via
> > registers 0x1b/0x1c from.
> 
> Huh. Looks like this is a second case of differing datasheets. Mine is
> revision 1.8 dated 2021-04-21. The documentation for indirect access was
> added in revision 1.7 dated 2020-07-08. Although it seems like the
> SERDES registers were also added in this revision, so maybe you just
> missed this section?

I have Rev. 1.2. dated July 2014. Either that, or I'm holding the book
upside down...

> > My datasheet for RTL8211FS doesn't show
> > offsets 0x1b and 0x1c in page 0xa43.
> 
> Neither does mine. These registers are only documented by reference from
> section 8.5. They also aren't named, so the above defines are my own
> coinage.
> 
> > Additionally, I cross-checked with
> > other registers that are accessed by the driver (like the Interrupt
> > Enable Register), and the driver access procedure -
> > phy_write_paged(phydev, 0xa42, RTL821x_INER, val) - seems to be pretty
> > much in line with what my datasheet shows.
> 
> | The SERDES related registers should be read and written through indirect
> | access method. The registers include Page 0xdc0 to Page 0xdcf and Page
> | 0xde0 to Page 0xdf0.
> 
> Each register accessed this way also has
> 
> | Note: This register requires indirect access.
> 
> below the register table.

Ok, possible. And none of the registers accessed by Linux using
phy_read_paged() / phy_write_paged() have the "indirect access" note?
Maybe it was a documentation update as you say, which I don't have.

> >> I think it would be better to just return PHY_AN_INBAND_ON when using
> >> SGMII.
> > 
> > Well, of course hardcoding PHY_AN_INBAND_ON in the driver is on the
> > table, if it isn't possible to alter this setting to the best of our
> > knowledge (or if it's implausible that someone modified it). And this
> > seems more and more like the case.
> 
> I meant something like
> 
> 	if (interface == PHY_INTERFACE_MODE_SGMII)
> 		return PHY_AN_INBAND_ON;
> 
> 	return PHY_AN_INBAND_UNKNOWN;

Absolutely, I understood the first time. So you confirm that such a
change makes your Lynx PCS promote to MLO_AN_INBAND, which makes the
RTL8211FS work, right?

> Although for RGMII, in-band status is (per MIICR2):
> 
> - Enabled by default
> - Disablable
> - Optional
> 
> So maybe we should do (PHY_AN_INBAND_ON | PHY_AN_INBAND_OFF) in that
> case. That said, RGMII in-band is not supported by phylink (yet).

Well, it kinda is. I even said this in one of the commit messages

|    net: phy: at803x: validate in-band autoneg for AT8031/AT8033
|
|    These PHYs also support RGMII, and for that mode, we report that in-band
|    AN is unknown, which means that phylink will not change the mode from
|    the device tree. Since commit d73ffc08824d ("net: phylink: allow
|    RGMII/RTBI in-band status"), RGMII in-band status is a thing, and I
|    don't want to meddle with that unless I have a reason for it.

Although I'd be much more comfortable for now if we could concentrate on
SERDES protocols. I'm not exactly sure what are the hardware state machines
and responsible standards for RGMII in-band status, what will happen on
settings mismatch (I know that NXP MACs fail to link up if we enable the
feature but we attach a switch and not a PHY to RGMII - see c76a97218dcb
("net: enetc: force the RGMII speed and duplex instead of operating in
inband mode") - but not much more). Essentially I don't know enough
right now to even attempt to make any generalizations. Although I suppose
a discussion could be started about it.

Re: [PATCH v4 net-next 0/8] Let phylink manage in-band AN for the PHY

[Sean Anderson]

On 11/22/22 11:30, Vladimir Oltean wrote:
> On Tue, Nov 22, 2022 at 11:10:03AM -0500, Sean Anderson wrote:
>> On 11/21/22 19:17, Vladimir Oltean wrote:
>> > On Mon, Nov 21, 2022 at 05:42:44PM -0500, Sean Anderson wrote:
>> >> Are you certain this is the cause of the issue? It's also possible that
>> >> there is some errata for the PCS which is causing the issue. I have
>> >> gotten no review/feedback from NXP regarding the phylink conversion
>> >> (aside from acks for the cleanups).
>> > 
>> > Erratum which does what out of the ordinary? Your description of the
>> > hardware failure seems consistent with the most plausible explanation
>> > that doesn't involve any bugs.
>> 
>> Well, I don't have a setup which doesn't require in-band AN, so I can't
>> say one way or the other where the problem lies. To me, the Lynx PCS is
>> just as opaque as the phy.
> 
> ok.
> 
>> > If you enable C37/SGMII AN in the PCS (of the PHY or of the MAC) and AN
>> > does not complete (because it's not enabled on the other end), that
>> > system side of the link remains down. Which you don't see when you
>> > operate in MLO_AN_PHY mode, because phylink only considers the PCS link
>> > state in MLO_AN_INBAND mode. So this is why you see the link as up but
>> > it doesn't work.
>> 
>> Actually, I checked the PCS manually in phy mode, and the link was up.
>> I expected it to be down, so this was a bit surprising to me.
> 
> Well, if autoneg is disabled in the Lynx PCS (which it is in MLO_AN_PHY),
> then the link should come up right away, as long as it can lock on some
> symbols IIRC. It's a different story for the PHY PCS if autoneg is
> enabled there. Still nothing surprising here, really.
> 
>> > To confirm whether I'm right or wrong, there's a separate SERDES
>> > Interrupt Status Register at page 0xde1 offset 0x12, whose bit 4 is
>> > "SERDES link status change" and bit 0 is "SERDES auto-negotiation error".
>> > These bits should both be set when you double-read them (regardless of
>> > IRQ enable I think) when your link is down with MLO_AN_PHY, but should
>> > be cleared with MLO_AN_INBAND.
>> 
>> This register is always 0s for me...
>> 
>> >> This is used for SGMII to RGMII bridge mode (figure 4). It doesn't seem
>> >> to contain useful information for UTP mode (figure 1).
>> > 
>> > So it would seem. It was a hasty read last time, sorry. Re-reading, the
>> > field says that when it's set, the SGMII code word being transmitted is
>> > "selected by the register" SGMII ANAR. And in the SGMII ANLPAR, you can
>> > see what the MAC said.
>> 
>> ... possibly because of this.
>> 
>> That said, ANLPAR is 0x4001 (all reserved bits) when we use in-band:
>> 
>> [    8.191146] RTL8211F Gigabit Ethernet 0x0000000001afc000:04: INER=0000 INSR=0000 ANARSEL=0000 ANAR=0050 ANLPAR=4001
>> 
>> but all zeros without:
>> 
>> [   11.263245] RTL8211F Gigabit Ethernet 0x0000000001afc000:04: INER=0000 INSR=0000 ANARSEL=0000 ANAR=0050 ANLPAR=0000
> 
> So enabling in-band autoneg in the Lynx PCS does what you'd expect it to do.
> I don't know why you don't get a "SERDES auto-negotiation error" bit in
> the interrupt status register. Maybe you need to first enable it in the
> interrupt enable register?! Who knows. Not sure how far it's worth
> diving into this.
> 
>> It's all 1s when using RGMII. These bits are reserved, so it's not that
>> interesting, but maybe these registers are not as useless as they seem.
> 
> Yeah, with RGMII I don't know if the PHY responds to the SERDES registers
> over MDIO. All ones may mean the MDIO bus pull-ups.
> 
>> > Of course, it doesn't say what happens when the bit for software-driven
>> > SGMII autoneg is *not* set, if the process can be at all bypassed.
>> > I suppose now that it can't, otherwise the ANLPAR register could also be
>> > writable over MDIO, they would have likely reused at least partly the
>> > same mechanisms.
>> > 
>> >> > +	ret = phy_read_paged(phydev, 0xd08, RTL8211FS_SGMII_ANARSEL);
>> >> 
>> >> That said, you have to use the "Indirect access method" to access this
>> >> register (per section 8.5). This is something like
>> >> 
>> >> #define RTL8211F_IAAR				0x1b
>> >> #define RTL8211F_IADR				0x1c
>> >> 
>> >> #define RTL8211F_IAAR_PAGE			GENMASK(15, 4)
>> >> #define RTL8211F_IAAR_REG			GENMASK(3, 1)
>> >> #define INDIRECT_ADDRESS(page, reg) \
>> >> 	(FIELD_PREP(RTL8211F_IAAR_PAGE, page) | \
>> >> 	 FIELD_PREP(RTL8211F_IAAR_REG, reg - 16))
>> >> 
>> >> 	ret = phy_write_paged(phydev, 0xa43, RTL8211F_IAAR,
>> >> 			      INDIRECT_ADDRESS(0xd08, RTL8211FS_SGMII_ANARSEL));
>> >> 	if (ret < 0)
>> >> 		return ret;
>> >> 
>> >> 	ret = phy_read_paged(phydev, 0xa43, RTL8211F_IADR);
>> >> 	if (ret < 0)
>> >> 		return ret;
>> >> 
>> >> I dumped the rest of the serdes registers using this method, but I
>> >> didn't see anything interesting (all defaults).
>> > 
>> > I'm _really_ not sure where you got the "Indirect access method" via
>> > registers 0x1b/0x1c from.
>> 
>> Huh. Looks like this is a second case of differing datasheets. Mine is
>> revision 1.8 dated 2021-04-21. The documentation for indirect access was
>> added in revision 1.7 dated 2020-07-08. Although it seems like the
>> SERDES registers were also added in this revision, so maybe you just
>> missed this section?
> 
> I have Rev. 1.2. dated July 2014. Either that, or I'm holding the book
> upside down...

Looks like 1.7 just added SERDES ANSCR and SERDES SSR. So I suppose the
other registers were documented earlier, without the not about indirect
access.

>> > My datasheet for RTL8211FS doesn't show
>> > offsets 0x1b and 0x1c in page 0xa43.
>> 
>> Neither does mine. These registers are only documented by reference from
>> section 8.5. They also aren't named, so the above defines are my own
>> coinage.
>> 
>> > Additionally, I cross-checked with
>> > other registers that are accessed by the driver (like the Interrupt
>> > Enable Register), and the driver access procedure -
>> > phy_write_paged(phydev, 0xa42, RTL821x_INER, val) - seems to be pretty
>> > much in line with what my datasheet shows.
>> 
>> | The SERDES related registers should be read and written through indirect
>> | access method. The registers include Page 0xdc0 to Page 0xdcf and Page
>> | 0xde0 to Page 0xdf0.
>> 
>> Each register accessed this way also has
>> 
>> | Note: This register requires indirect access.
>> 
>> below the register table.
> 
> Ok, possible. And none of the registers accessed by Linux using
> phy_read_paged() / phy_write_paged() have the "indirect access" note?

Correct.

> Maybe it was a documentation update as you say, which I don't have.

Looks like it. Probably to work around some bug.

>> >> I think it would be better to just return PHY_AN_INBAND_ON when using
>> >> SGMII.
>> > 
>> > Well, of course hardcoding PHY_AN_INBAND_ON in the driver is on the
>> > table, if it isn't possible to alter this setting to the best of our
>> > knowledge (or if it's implausible that someone modified it). And this
>> > seems more and more like the case.
>> 
>> I meant something like
>> 
>> 	if (interface == PHY_INTERFACE_MODE_SGMII)
>> 		return PHY_AN_INBAND_ON;
>> 
>> 	return PHY_AN_INBAND_UNKNOWN;
> 
> Absolutely, I understood the first time. So you confirm that such a
> change makes your Lynx PCS promote to MLO_AN_INBAND, which makes the
> RTL8211FS work, right?

Correct.

>> Although for RGMII, in-band status is (per MIICR2):
>> 
>> - Enabled by default
>> - Disablable
>> - Optional
>> 
>> So maybe we should do (PHY_AN_INBAND_ON | PHY_AN_INBAND_OFF) in that
>> case. That said, RGMII in-band is not supported by phylink (yet).
> 
> Well, it kinda is. I even said this in one of the commit messages
> 
> |    net: phy: at803x: validate in-band autoneg for AT8031/AT8033
> |
> |    These PHYs also support RGMII, and for that mode, we report that in-band
> |    AN is unknown, which means that phylink will not change the mode from
> |    the device tree. Since commit d73ffc08824d ("net: phylink: allow
> |    RGMII/RTBI in-band status"), RGMII in-band status is a thing, and I
> |    don't want to meddle with that unless I have a reason for it.
> 
> Although I'd be much more comfortable for now if we could concentrate on
> SERDES protocols. I'm not exactly sure what are the hardware state machines
> and responsible standards for RGMII in-band status, what will happen on
> settings mismatch (I know that NXP MACs fail to link up if we enable the
> feature but we attach a switch and not a PHY to RGMII - see c76a97218dcb
> ("net: enetc: force the RGMII speed and duplex instead of operating in
> inband mode") - but not much more). Essentially I don't know enough
> right now to even attempt to make any generalizations. Although I suppose
> a discussion could be started about it.

I think deferring this is the right thing to do. It's not clear to me if 
in-band RGMII is even necessary (as all hardware I've seen includes MDIO).

--Sean

Re: [PATCH v4 net-next 0/8] Let phylink manage in-band AN for the PHY

[Russell King (Oracle)]

On Tue, Nov 22, 2022 at 11:10:03AM -0500, Sean Anderson wrote:
> So maybe we should do (PHY_AN_INBAND_ON | PHY_AN_INBAND_OFF) in that
> case. That said, RGMII in-band is not supported by phylink (yet).

I think people use it - why do you think it isn't supportable?

Why would it need to be any different from something like SGMII? One
can implement a "phylink_pcs" that responds appropriately when in RGMII
mode (which I believe is what others have done.)

Note that Marvell net drivers always register a PCS no matter what
interface mode is being used - I believe the hardware has the ability
to read the RGMII in-band and in that case it's just the same
registers that one has to read to get the status as SGMII.

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

Re: [PATCH v4 net-next 0/8] Let phylink manage in-band AN for the PHY

[Sean Anderson]

On 11/22/22 12:59, Russell King (Oracle) wrote:
> On Tue, Nov 22, 2022 at 11:10:03AM -0500, Sean Anderson wrote:
>> So maybe we should do (PHY_AN_INBAND_ON | PHY_AN_INBAND_OFF) in that
>> case. That said, RGMII in-band is not supported by phylink (yet).
> 
> I think people use it - why do you think it isn't supportable?

Sorry, I forgot that support had been added recently.

--Sean

> Why would it need to be any different from something like SGMII? One
> can implement a "phylink_pcs" that responds appropriately when in RGMII
> mode (which I believe is what others have done.)
> 
> Note that Marvell net drivers always register a PCS no matter what
> interface mode is being used - I believe the hardware has the ability
> to read the RGMII in-band and in that case it's just the same
> registers that one has to read to get the status as SGMII.
> 

Re: [PATCH v4 net-next 0/8] Let phylink manage in-band AN for the PHY

[Russell King (Oracle)]

On Mon, Nov 21, 2022 at 01:38:31PM -0500, Sean Anderson wrote:
> # dmesg | grep net6
> [    3.826156] fsl_dpaa_mac 1aea000.ethernet net6: renamed from eth3
> [    5.062654] fsl_dpaa_mac 1aea000.ethernet net6: PHY driver does not report in-band autoneg capability, assuming true
> [    5.089724] fsl_dpaa_mac 1aea000.ethernet net6: PHY [0x0000000001afc000:04] driver [RTL8211F Gigabit Ethernet] (irq=POLL)
> [    5.089734] fsl_dpaa_mac 1aea000.ethernet net6: phy: sgmii setting supported 0,00000000,00000000,000062ea advertising 0,00000000,00000000,000062ea
> [    5.089782] fsl_dpaa_mac 1aea000.ethernet net6: configuring for inband/sgmii link mode
> [    5.089786] fsl_dpaa_mac 1aea000.ethernet net6: major config sgmii
> [    5.090951] fsl_dpaa_mac 1aea000.ethernet net6: phylink_mac_config: mode=inband/sgmii/Unknown/Unknown/none adv=0,00000000,00000000,000062ea pause=00 link=0 an=1
> [    5.118325] fsl_dpaa_mac 1aea000.ethernet net6: phy link down sgmii/Unknown/Unknown/none/off
> [    9.214204] fsl_dpaa_mac 1aea000.ethernet net6: phy link up sgmii/1Gbps/Full/none/rx/tx
> [    9.214247] fsl_dpaa_mac 1aea000.ethernet net6: Link is Up - 1Gbps/Full - flow control rx/tx
> 
> I believe this is the same issue I ran into before. This is why I
> defaulted to in-band.

Yes, when operating in SGMII mode, some PHYs:
1) require SGMII in-band to be acknowledged by the MAC
2) provide SGMII in-band but will time out
3) don't provide any SGMII in-band

It sounds like you have case (1), and the options currently are (as
you've identified) to either state in DT that in-band is being used,
or use ovr_an_inband in the driver (if it's a recent conversion that
used in-band mode without needing extra properties.)

Vladimir's patches provide us another way to solve this problem - but
relies upon the PHY drivers being updated to correctly report whether
the hardware will be using in-band. If they don't, then we're lost at
sea and have no idea whether (1) or (3) applies, and in that case we
have to fall back to today's behaviour, which is dependent on
describing it in DT or using ovr_an_inband.

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

Re: [PATCH v4 net-next 0/8] Let phylink manage in-band AN for the PHY

[Maxim Kochetkov]

Hi!

Please add Maxim Kiselev <bigunclemax@gmail.com> to the CC in next 
versions. Because I have no more access to T1040 SoC. Maxim can help 
with testing.

On 18.11.2022 03:01, Vladimir Oltean wrote:
> Problem statement
> ~~~~~~~~~~~~~~~~~
> 
> The on-board SERDES link between an NXP (Lynx) PCS and a PHY may not
> work, depending on whether U-Boot networking was used on that port or not.
> 
> There is no mechanism in Linux (with phylib/phylink, at least) to ensure
> that the MAC driver and the PHY driver have synchronized settings for
> in-band autoneg. It all depends on the 'managed = "in-band-status"'
> device tree property, which does not reflect a stable and unchanging
> reality, and furthermore, some (older) device trees may have this
> property missing when they shouldn't.
> 

Re: [PATCH v4 net-next 0/8] Let phylink manage in-band AN for the PHY

[Vladimir Oltean]

On Fri, Dec 02, 2022 at 03:16:19PM +0300, Maxim Kochetkov wrote:
> Hi!
> 
> Please add Maxim Kiselev <bigunclemax@gmail.com> to the CC in next versions.
> Because I have no more access to T1040 SoC. Maxim can help with testing.

Ok, thanks for your contribution to the T1040 switch driver.