[PATCH net-next v2 0/9] Allow isolating PHY devices

[PATCH net-next v2 0/9] Allow isolating PHY devices

[Maxime Chevallier]

Hello,

This is the V2 of a series to add isolation support for PHY devices.

As a remainder, this mode allows a PHY to set its MII lines in
high-impedance mode to avoid interferences on this bus.

So far, I've identified that :

 - Marvell 88e1512 isolation works fine
 - LXT973 claims to support isolation, but it's actually broken
 - Marvell 88x3310 doesn't support isolation, by design
 - Marvell 88e1111 claims to support isolation in GMII, RGMII, TBI
   (untested) but doesn't in SGMII (tested).

Changes in V2 :

 - Removed the loopback mode that was included in the first iteration
 - Added phy_shutdown, to make sure we de-isolate the PHY when rebooting
 - Changes the "PHY_NO_ISOLATE" flag to a phy driver ops. Testing showed
   that some PHYs may or may not support isolation based on the
   interface that's being used.
 - Added isolation support reporting for the Marvell 88e1111 PHY.

V1 : https://lore.kernel.org/netdev/20240911212713.2178943-1-maxime.chevallier@bootlin.com/

Maxime Chevallier (9):
  net: phy: allow isolating PHY devices
  net: phy: Introduce phy_shutdown for device quiescence.
  net: phy: Allow PHY drivers to report isolation support
  net: phy: lxt: Mark LXT973 PHYs as having a broken isolate mode
  net: phy: marvell10g: 88x3310 and 88x3340 don't support isolate mode
  net: phy: marvell: mv88e1111 doesn't support isolate in SGMII mode
  net: phy: introduce ethtool_phy_ops to get and set phy configuration
  net: ethtool: phy: allow reporting and setting the phy isolate status
  netlink: specs: introduce phy-set command along with configurable
    attributes

 Documentation/netlink/specs/ethtool.yaml     |  15 +++
 Documentation/networking/ethtool-netlink.rst |   1 +
 drivers/net/phy/lxt.c                        |   2 +
 drivers/net/phy/marvell.c                    |   9 ++
 drivers/net/phy/marvell10g.c                 |   2 +
 drivers/net/phy/phy.c                        |  44 ++++++++
 drivers/net/phy/phy_device.c                 | 101 +++++++++++++++++--
 include/linux/ethtool.h                      |   8 ++
 include/linux/phy.h                          |  42 ++++++++
 include/uapi/linux/ethtool_netlink.h         |   2 +
 net/ethtool/netlink.c                        |   8 ++
 net/ethtool/netlink.h                        |   1 +
 net/ethtool/phy.c                            |  68 +++++++++++++
 13 files changed, 297 insertions(+), 6 deletions(-)

-- 
2.46.1

[PATCH net-next v2 1/9] net: phy: allow isolating PHY devices

[Maxime Chevallier]

The 802.3 specifications describes the isolation mode as setting the
PHY's MII interface in high-impedance mode, thus isolating the PHY from
that bus. This effectively breaks the link between the MAC and the PHY,
but without necessarily disrupting the link between the PHY and the LP.

This mode can be useful for testing purposes, but also when there are
multiple PHYs on the same MII bus (a case that the 802.3 specification
refers to).

In Isolation mode, the PHY will still continue to respond to MDIO
commands.

Introduce a helper to set the phy in an isolated mode.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
V2 : No change

 drivers/net/phy/phy_device.c | 76 +++++++++++++++++++++++++++++++++---
 include/linux/phy.h          |  4 ++
 2 files changed, 74 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 560e338b307a..c468e72bef4b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2127,6 +2127,38 @@ int phy_loopback(struct phy_device *phydev, bool enable)
 }
 EXPORT_SYMBOL(phy_loopback);
 
+int phy_isolate(struct phy_device *phydev, bool enable)
+{
+	int ret = 0;
+
+	if (!phydev->drv)
+		return -EIO;
+
+	mutex_lock(&phydev->lock);
+
+	if (enable && phydev->isolated) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	if (!enable && !phydev->isolated) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = genphy_isolate(phydev, enable);
+
+	if (ret)
+		goto out;
+
+	phydev->isolated = enable;
+
+out:
+	mutex_unlock(&phydev->lock);
+	return ret;
+}
+EXPORT_SYMBOL(phy_isolate);
+
 /**
  * phy_reset_after_clk_enable - perform a PHY reset if needed
  * @phydev: target phy_device struct
@@ -2280,7 +2312,7 @@ int genphy_setup_forced(struct phy_device *phydev)
 	ctl = mii_bmcr_encode_fixed(phydev->speed, phydev->duplex);
 
 	return phy_modify(phydev, MII_BMCR,
-			  ~(BMCR_LOOPBACK | BMCR_ISOLATE | BMCR_PDOWN), ctl);
+			  ~(BMCR_LOOPBACK | BMCR_PDOWN), ctl);
 }
 EXPORT_SYMBOL(genphy_setup_forced);
 
@@ -2369,8 +2401,11 @@ EXPORT_SYMBOL(genphy_read_master_slave);
  */
 int genphy_restart_aneg(struct phy_device *phydev)
 {
-	/* Don't isolate the PHY if we're negotiating */
-	return phy_modify(phydev, MII_BMCR, BMCR_ISOLATE,
+	u16 mask = phydev->isolated ? 0 : BMCR_ISOLATE;
+	/* Don't isolate the PHY if we're negotiating, unless the PHY is
+	 * explicitly isolated
+	 */
+	return phy_modify(phydev, MII_BMCR, mask,
 			  BMCR_ANENABLE | BMCR_ANRESTART);
 }
 EXPORT_SYMBOL(genphy_restart_aneg);
@@ -2394,7 +2429,8 @@ int genphy_check_and_restart_aneg(struct phy_device *phydev, bool restart)
 		if (ret < 0)
 			return ret;
 
-		if (!(ret & BMCR_ANENABLE) || (ret & BMCR_ISOLATE))
+		if (!(ret & BMCR_ANENABLE) ||
+		    ((ret & BMCR_ISOLATE) && !phydev->isolated))
 			restart = true;
 	}
 
@@ -2495,7 +2531,8 @@ int genphy_c37_config_aneg(struct phy_device *phydev)
 		if (ctl < 0)
 			return ctl;
 
-		if (!(ctl & BMCR_ANENABLE) || (ctl & BMCR_ISOLATE))
+		if (!(ctl & BMCR_ANENABLE) ||
+		    ((ctl & BMCR_ISOLATE) && !phydev->isolated))
 			changed = 1; /* do restart aneg */
 	}
 
@@ -2782,12 +2819,18 @@ EXPORT_SYMBOL(genphy_c37_read_status);
 int genphy_soft_reset(struct phy_device *phydev)
 {
 	u16 res = BMCR_RESET;
+	u16 mask = 0;
 	int ret;
 
 	if (phydev->autoneg == AUTONEG_ENABLE)
 		res |= BMCR_ANRESTART;
 
-	ret = phy_modify(phydev, MII_BMCR, BMCR_ISOLATE, res);
+	if (phydev->isolated)
+		res |= BMCR_ISOLATE;
+	else
+		mask |= BMCR_ISOLATE;
+
+	ret = phy_modify(phydev, MII_BMCR, mask, res);
 	if (ret < 0)
 		return ret;
 
@@ -2912,6 +2955,12 @@ int genphy_loopback(struct phy_device *phydev, bool enable)
 		u16 ctl = BMCR_LOOPBACK;
 		int ret, val;
 
+		/* Isolating and looping-back the MII interface doesn't really
+		 * make sense
+		 */
+		if (phydev->isolated)
+			return -EINVAL;
+
 		ctl |= mii_bmcr_encode_fixed(phydev->speed, phydev->duplex);
 
 		phy_modify(phydev, MII_BMCR, ~0, ctl);
@@ -2924,6 +2973,8 @@ int genphy_loopback(struct phy_device *phydev, bool enable)
 	} else {
 		phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK, 0);
 
+		genphy_isolate(phydev, phydev->isolated);
+
 		phy_config_aneg(phydev);
 	}
 
@@ -2931,6 +2982,19 @@ int genphy_loopback(struct phy_device *phydev, bool enable)
 }
 EXPORT_SYMBOL(genphy_loopback);
 
+int genphy_isolate(struct phy_device *phydev, bool enable)
+{
+	u16 val = 0;
+
+	if (enable)
+		val = BMCR_ISOLATE;
+
+	phy_modify(phydev, MII_BMCR, BMCR_ISOLATE, val);
+
+	return 0;
+}
+EXPORT_SYMBOL(genphy_isolate);
+
 /**
  * phy_remove_link_mode - Remove a supported link mode
  * @phydev: phy_device structure to remove link mode from
diff --git a/include/linux/phy.h b/include/linux/phy.h
index a98bc91a0cde..ae33919aa0f5 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -573,6 +573,7 @@ struct macsec_ops;
  * @mac_managed_pm: Set true if MAC driver takes of suspending/resuming PHY
  * @wol_enabled: Set to true if the PHY or the attached MAC have Wake-on-LAN
  * 		 enabled.
+ * @isolated: Set to true if the PHY's MII has been isolated.
  * @state: State of the PHY for management purposes
  * @dev_flags: Device-specific flags used by the PHY driver.
  *
@@ -676,6 +677,7 @@ struct phy_device {
 	unsigned is_on_sfp_module:1;
 	unsigned mac_managed_pm:1;
 	unsigned wol_enabled:1;
+	unsigned isolated:1;
 
 	unsigned autoneg:1;
 	/* The most recently read link state */
@@ -1781,6 +1783,7 @@ int phy_suspend(struct phy_device *phydev);
 int phy_resume(struct phy_device *phydev);
 int __phy_resume(struct phy_device *phydev);
 int phy_loopback(struct phy_device *phydev, bool enable);
+int phy_isolate(struct phy_device *phydev, bool enable);
 int phy_sfp_connect_phy(void *upstream, struct phy_device *phy);
 void phy_sfp_disconnect_phy(void *upstream, struct phy_device *phy);
 void phy_sfp_attach(void *upstream, struct sfp_bus *bus);
@@ -1894,6 +1897,7 @@ int genphy_read_master_slave(struct phy_device *phydev);
 int genphy_suspend(struct phy_device *phydev);
 int genphy_resume(struct phy_device *phydev);
 int genphy_loopback(struct phy_device *phydev, bool enable);
+int genphy_isolate(struct phy_device *phydev, bool enable);
 int genphy_soft_reset(struct phy_device *phydev);
 irqreturn_t genphy_handle_interrupt_no_ack(struct phy_device *phydev);
 
-- 
2.46.1

[PATCH net-next v2 2/9] net: phy: Introduce phy_shutdown for device quiescence.

[Maxime Chevallier]

When the hardware platform goes down for reboot, there's always the
chance that the PHY device doesn't get reset, or fully reconfigured by
the bootloader, which could rely on strap settings.

Therefore, when special modes from the PHY are being used, which could
make it non-functional at reboot, introduce a phy_shutdown helper to
pull the PHY out of the non-functional mode it is currently in.

Currently, this applies to the isolation mode.

Suggested-by: Florian Fainelli <florian.fainelli@broadcom.com>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
V2 : New patch

 drivers/net/phy/phy_device.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index c468e72bef4b..a0d8ff995024 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3733,6 +3733,17 @@ static int phy_remove(struct device *dev)
 	return 0;
 }
 
+static void phy_shutdown(struct device *dev)
+{
+	struct phy_device *phydev = to_phy_device(dev);
+
+	/* If the PHY isn't reset and reconfigured by the bootloader, the PHY
+	 * will stay isolated. Make sure to un-isolate PHYs during shutdown.
+	 */
+	if (phydev->isolated)
+		phy_isolate(phydev, false);
+}
+
 /**
  * phy_driver_register - register a phy_driver with the PHY layer
  * @new_driver: new phy_driver to register
@@ -3766,6 +3777,7 @@ int phy_driver_register(struct phy_driver *new_driver, struct module *owner)
 	new_driver->mdiodrv.driver.bus = &mdio_bus_type;
 	new_driver->mdiodrv.driver.probe = phy_probe;
 	new_driver->mdiodrv.driver.remove = phy_remove;
+	new_driver->mdiodrv.driver.shutdown = phy_shutdown;
 	new_driver->mdiodrv.driver.owner = owner;
 	new_driver->mdiodrv.driver.probe_type = PROBE_FORCE_SYNCHRONOUS;
 
-- 
2.46.1

[PATCH net-next v2 3/9] net: phy: Allow PHY drivers to report isolation support

[Maxime Chevallier]

Some PHYs have malfunctionning isolation modes, where the MII lines
aren't correctly set in high-impedance, potentially interfering with the
MII bus in unexpected ways. Some other PHYs simply don't support it.

The isolation support may depend on the interface mode being used, so
introduce a new driver callback to report the isolation support in the
current PHY configuration.

As some PHYs just never support isolation, introduce a genphy helper
that can be used for strictly non-isolating PHYs.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
V2 : Moved from flag to callback, introduced genphy helper

 drivers/net/phy/phy_device.c | 11 +++++++++++
 include/linux/phy.h          | 19 +++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index a0d8ff995024..9294b73c391a 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2127,6 +2127,14 @@ int phy_loopback(struct phy_device *phydev, bool enable)
 }
 EXPORT_SYMBOL(phy_loopback);
 
+static bool phy_can_isolate(struct phy_device *phydev)
+{
+	if (phydev->drv && phydev->drv->can_isolate)
+		return phydev->drv->can_isolate(phydev);
+
+	return true;
+}
+
 int phy_isolate(struct phy_device *phydev, bool enable)
 {
 	int ret = 0;
@@ -2134,6 +2142,9 @@ int phy_isolate(struct phy_device *phydev, bool enable)
 	if (!phydev->drv)
 		return -EIO;
 
+	if (!phy_can_isolate(phydev))
+		return -EOPNOTSUPP;
+
 	mutex_lock(&phydev->lock);
 
 	if (enable && phydev->isolated) {
diff --git a/include/linux/phy.h b/include/linux/phy.h
index ae33919aa0f5..e43f7169c57d 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1192,6 +1192,19 @@ struct phy_driver {
 	 */
 	int (*led_polarity_set)(struct phy_device *dev, int index,
 				unsigned long modes);
+
+	/**
+	 * @can_isolate: Query the PHY isolation capability
+	 * @dev: PHY device to query
+	 *
+	 * Although PHY isolation is part of 802.3, not all PHYs support that
+	 * feature. Some PHYs can only support isolation when using a specific
+	 * phy_interface_mode, and some don't support it at all.
+	 *
+	 * Returns true if the PHY can isolate in its current configuration,
+	 * false otherwise.
+	 */
+	bool (*can_isolate)(struct phy_device *dev);
 };
 #define to_phy_driver(d) container_of_const(to_mdio_common_driver(d),		\
 				      struct phy_driver, mdiodrv)
@@ -1910,6 +1923,12 @@ static inline int genphy_no_config_intr(struct phy_device *phydev)
 {
 	return 0;
 }
+
+static inline bool genphy_no_isolate(struct phy_device *phydev)
+{
+	return false;
+}
+
 int genphy_read_mmd_unsupported(struct phy_device *phdev, int devad,
 				u16 regnum);
 int genphy_write_mmd_unsupported(struct phy_device *phdev, int devnum,
-- 
2.46.1

Re: [PATCH net-next v2 3/9] net: phy: Allow PHY drivers to report isolation support

[Oleksij Rempel]

On Fri, Oct 04, 2024 at 06:15:53PM +0200, Maxime Chevallier wrote:
> Some PHYs have malfunctionning isolation modes, where the MII lines
> aren't correctly set in high-impedance, potentially interfering with the
> MII bus in unexpected ways. Some other PHYs simply don't support it.

Do we have in this case multiple isolation variants like high-impedance
and "the other one"? :)  Do the "the other one" is still usable for some
cases like Wake on LAN without shared xMII?

I'm just curios.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v2 3/9] net: phy: Allow PHY drivers to report isolation support

[Maxime Chevallier]

On Fri, 4 Oct 2024 18:46:20 +0200
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> On Fri, Oct 04, 2024 at 06:15:53PM +0200, Maxime Chevallier wrote:
> > Some PHYs have malfunctionning isolation modes, where the MII lines
> > aren't correctly set in high-impedance, potentially interfering with the
> > MII bus in unexpected ways. Some other PHYs simply don't support it.  
> 
> Do we have in this case multiple isolation variants like high-impedance
> and "the other one"? :)  Do the "the other one" is still usable for some
> cases like Wake on LAN without shared xMII?

I don't think there are multiple variants, at least I didn't see any :/

Maxime

Re: [PATCH net-next v2 3/9] net: phy: Allow PHY drivers to report isolation support

[Andrew Lunn]

> +static bool phy_can_isolate(struct phy_device *phydev)
> +{
> +	if (phydev->drv && phydev->drv->can_isolate)
> +		return phydev->drv->can_isolate(phydev);
> +
> +	return true;

Reading Russells comment, and the fact that this feature is nearly
unused, so we have no idea how well PHYs actually support this, i
would flip the logic. Default to false. A PHY driver needs to actively
sign up to supporting isolation, with the understanding it has been
tested on at least one board with two or more PHYs.

	Andrew

Re: [PATCH net-next v2 3/9] net: phy: Allow PHY drivers to report isolation support

[Maxime Chevallier]

On Fri, 4 Oct 2024 20:20:10 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > +static bool phy_can_isolate(struct phy_device *phydev)
> > +{
> > +	if (phydev->drv && phydev->drv->can_isolate)
> > +		return phydev->drv->can_isolate(phydev);
> > +
> > +	return true;  
> 
> Reading Russells comment, and the fact that this feature is nearly
> unused, so we have no idea how well PHYs actually support this, i
> would flip the logic. Default to false. A PHY driver needs to actively
> sign up to supporting isolation, with the understanding it has been
> tested on at least one board with two or more PHYs.

Fair point, I'll reverse the logic.

Thanks,

Maxime

[PATCH net-next v2 4/9] net: phy: lxt: Mark LXT973 PHYs as having a broken isolate mode

[Maxime Chevallier]

Testing showed that PHYs from the LXT973 family have a non-working
isolate mode, where the MII lines aren't set in high-impedance as would
be expected. Prevent isolating these PHYs.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
V2 : Use callback instead of flag

 drivers/net/phy/lxt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/phy/lxt.c b/drivers/net/phy/lxt.c
index e3bf827b7959..45f92451bee8 100644
--- a/drivers/net/phy/lxt.c
+++ b/drivers/net/phy/lxt.c
@@ -334,6 +334,7 @@ static struct phy_driver lxt97x_driver[] = {
 	.read_status	= lxt973a2_read_status,
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
+	.can_isolate	= genphy_no_isolate,
 }, {
 	.phy_id		= 0x00137a10,
 	.name		= "LXT973",
@@ -344,6 +345,7 @@ static struct phy_driver lxt97x_driver[] = {
 	.config_aneg	= lxt973_config_aneg,
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
+	.can_isolate	= genphy_no_isolate,
 } };
 
 module_phy_driver(lxt97x_driver);
-- 
2.46.1

[PATCH net-next v2 5/9] net: phy: marvell10g: 88x3310 and 88x3340 don't support isolate mode

[Maxime Chevallier]

The 88x3310 and 88x3340 PHYs don't support the isolation mode,
regardless of the MII mode being used
(SGMII/1000BaseX/2500BaseX/5GBaseR/10GBaseR). Report this through the
.can_isolate() callback.

This was confirmed on the 88x3310 on a MacchiatoBin.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
V2 : Use callback instead of flag

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

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 6642eb642d4b..c1a471f55cd4 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -1422,6 +1422,7 @@ static struct phy_driver mv3310_drivers[] = {
 		.set_loopback	= genphy_c45_loopback,
 		.get_wol	= mv3110_get_wol,
 		.set_wol	= mv3110_set_wol,
+		.can_isolate	= genphy_no_isolate,
 	},
 	{
 		.phy_id		= MARVELL_PHY_ID_88X3310,
@@ -1441,6 +1442,7 @@ static struct phy_driver mv3310_drivers[] = {
 		.set_tunable	= mv3310_set_tunable,
 		.remove		= mv3310_remove,
 		.set_loopback	= genphy_c45_loopback,
+		.can_isolate	= genphy_no_isolate,
 	},
 	{
 		.phy_id		= MARVELL_PHY_ID_88E2110,
-- 
2.46.1

[PATCH net-next v2 6/9] net: phy: marvell: mv88e1111 doesn't support isolate in SGMII mode

[Maxime Chevallier]

The 88e1111 datasheet indicates that it supports the isolate mode in
GMII, RGMII and TBI modes, but doesn't mention what it does in the other
modes. Testing showed that setting the isolate bit while the PHY is in
SGMII mode has no effect.

Reflect that behaviour in the .can_isolate() driver ops.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
V2 : New patch

 drivers/net/phy/marvell.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 9964bf3dea2f..912b08d9c124 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -1092,6 +1092,14 @@ static int m88e1111_set_tunable(struct phy_device *phydev,
 	}
 }
 
+static bool m88e1111_can_isolate(struct phy_device *phydev)
+{
+	if (phydev->interface == PHY_INTERFACE_MODE_SGMII)
+		return false;
+
+	return true;
+}
+
 static int m88e1011_get_downshift(struct phy_device *phydev, u8 *data)
 {
 	int val, cnt, enable;
@@ -3704,6 +3712,7 @@ static struct phy_driver marvell_drivers[] = {
 		.set_tunable = m88e1111_set_tunable,
 		.cable_test_start = m88e1111_vct_cable_test_start,
 		.cable_test_get_status = m88e1111_vct_cable_test_get_status,
+		.can_isolate = m88e1111_can_isolate,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1111_FINISAR,
-- 
2.46.1

[PATCH net-next v2 7/9] net: phy: introduce ethtool_phy_ops to get and set phy configuration

[Maxime Chevallier]

Expose phy-specific configuration hooks to get and set the state of an
ethernet PHY's internal configuration.

So far, these parameters only include the isolation state of the PHY.

The .get_config() ethtool_phy_ops gets these status information from the
phy_device's internal flags, while the .set_config() operation allows
changing these configuration parameters.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
V2 : Dropped loopback mode

 drivers/net/phy/phy.c        | 44 ++++++++++++++++++++++++++++++++++++
 drivers/net/phy/phy_device.c |  2 ++
 include/linux/ethtool.h      |  8 +++++++
 include/linux/phy.h          | 19 ++++++++++++++++
 4 files changed, 73 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 4f3e742907cb..5a689da060d1 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1811,3 +1811,47 @@ int phy_ethtool_nway_reset(struct net_device *ndev)
 	return ret;
 }
 EXPORT_SYMBOL(phy_ethtool_nway_reset);
+
+/**
+ * phy_get_config - Get PHY configuration parameters
+ * @phydev: the PHY device to act upon
+ * @phy_cfg:  The configuration to apply
+ */
+
+int phy_get_config(struct phy_device *phydev,
+		   struct phy_device_config *phy_cfg)
+{
+	mutex_lock(&phydev->lock);
+	phy_cfg->isolate = phydev->isolated;
+	mutex_unlock(&phydev->lock);
+
+	return 0;
+}
+
+/**
+ * phy_set_config - Set PHY configuration parameters
+ * @phydev: the PHY device to act upon
+ * @phy_cfg: the configuration to apply
+ * @extack: a netlink extack for useful error reporting
+ */
+
+int phy_set_config(struct phy_device *phydev,
+		   const struct phy_device_config *phy_cfg,
+		   struct netlink_ext_ack *extack)
+{
+	bool isolate_change;
+	int ret;
+
+	mutex_lock(&phydev->lock);
+	isolate_change = (phy_cfg->isolate != phydev->isolated);
+	mutex_unlock(&phydev->lock);
+
+	if (!isolate_change)
+		return 0;
+
+	ret = phy_isolate(phydev, phy_cfg->isolate);
+	if (ret)
+		NL_SET_ERR_MSG(extack, "Error while configuring PHY isolation");
+
+	return ret;
+}
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 9294b73c391a..1111a3cbcb02 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3857,6 +3857,8 @@ static const struct ethtool_phy_ops phy_ethtool_phy_ops = {
 	.get_plca_status	= phy_ethtool_get_plca_status,
 	.start_cable_test	= phy_start_cable_test,
 	.start_cable_test_tdr	= phy_start_cable_test_tdr,
+	.get_config		= phy_get_config,
+	.set_config		= phy_set_config,
 };
 
 static const struct phylib_stubs __phylib_stubs = {
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 12f6dc567598..480ee99a69a5 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -1140,6 +1140,7 @@ struct phy_device;
 struct phy_tdr_config;
 struct phy_plca_cfg;
 struct phy_plca_status;
+struct phy_device_config;
 
 /**
  * struct ethtool_phy_ops - Optional PHY device options
@@ -1151,6 +1152,8 @@ struct phy_plca_status;
  * @get_plca_status: Get PLCA configuration.
  * @start_cable_test: Start a cable test
  * @start_cable_test_tdr: Start a Time Domain Reflectometry cable test
+ * @get_config: Retrieve phy device configuration parameters
+ * @set_config: Set phy device configuration parameters
  *
  * All operations are optional (i.e. the function pointer may be set to %NULL)
  * and callers must take this into account. Callers must hold the RTNL lock.
@@ -1172,6 +1175,11 @@ struct ethtool_phy_ops {
 	int (*start_cable_test_tdr)(struct phy_device *phydev,
 				    struct netlink_ext_ack *extack,
 				    const struct phy_tdr_config *config);
+	int (*get_config)(struct phy_device *phydev,
+			  struct phy_device_config *phy_cfg);
+	int (*set_config)(struct phy_device *phydev,
+			  const struct phy_device_config *phy_cfg,
+			  struct netlink_ext_ack *extack);
 };
 
 /**
diff --git a/include/linux/phy.h b/include/linux/phy.h
index e43f7169c57d..662ba57cd0de 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -886,6 +886,20 @@ enum phy_led_modes {
 	__PHY_LED_MODES_NUM,
 };
 
+/**
+ * struct phy_device_config - General PHY device configuration parameters for
+ * status reporting and bulk configuration
+ *
+ * A structure containing generic PHY device information, allowing to expose
+ * internal status to userspace, and perform PHY configuration in a controlled
+ * manner.
+ *
+ * @isolate: The MII-side isolation status of the PHY
+ */
+struct phy_device_config {
+	bool isolate;
+};
+
 /**
  * struct phy_led: An LED driven by the PHY
  *
@@ -2085,6 +2099,11 @@ int phy_ethtool_set_plca_cfg(struct phy_device *phydev,
 			     struct netlink_ext_ack *extack);
 int phy_ethtool_get_plca_status(struct phy_device *phydev,
 				struct phy_plca_status *plca_st);
+int phy_get_config(struct phy_device *phydev,
+		   struct phy_device_config *phy_cfg);
+int phy_set_config(struct phy_device *phydev,
+		   const struct phy_device_config *phy_cfg,
+		   struct netlink_ext_ack *extack);
 
 int __phy_hwtstamp_get(struct phy_device *phydev,
 		       struct kernel_hwtstamp_config *config);
-- 
2.46.1

Re: [PATCH net-next v2 7/9] net: phy: introduce ethtool_phy_ops to get and set phy configuration

[Andrew Lunn]

> +int phy_set_config(struct phy_device *phydev,
> +		   const struct phy_device_config *phy_cfg,
> +		   struct netlink_ext_ack *extack)
> +{
> +	bool isolate_change;
> +	int ret;
> +
> +	mutex_lock(&phydev->lock);
> +	isolate_change = (phy_cfg->isolate != phydev->isolated);
> +	mutex_unlock(&phydev->lock);
> +
> +	if (!isolate_change)
> +		return 0;
> +
> +	ret = phy_isolate(phydev, phy_cfg->isolate);
> +	if (ret)
> +		NL_SET_ERR_MSG(extack, "Error while configuring PHY isolation");

This seems overly simplistic to me. Don't you need to iterate over all
the other PHYs attached to this MAC and ensure they are isolated? Only
one can be unisolated at once.

It is also not clear to me how this is going to work from a MAC
perspective. Does the MAC call phy_connect() multiple times? How does
ndev->phydev work? Who is responsible for the initial configuration,
such that all but one PHY is isolated?

I assume you have a real board that needs this. So i think we need to
see a bit more of the complete solution, including the MAC changes and
the device tree for the board, so we can see the big picture.

	Andrew

Re: [PATCH net-next v2 7/9] net: phy: introduce ethtool_phy_ops to get and set phy configuration

[Russell King (Oracle)]

On Fri, Oct 04, 2024 at 08:42:42PM +0200, Andrew Lunn wrote:
> > +int phy_set_config(struct phy_device *phydev,
> > +		   const struct phy_device_config *phy_cfg,
> > +		   struct netlink_ext_ack *extack)
> > +{
> > +	bool isolate_change;
> > +	int ret;
> > +
> > +	mutex_lock(&phydev->lock);
> > +	isolate_change = (phy_cfg->isolate != phydev->isolated);
> > +	mutex_unlock(&phydev->lock);
> > +
> > +	if (!isolate_change)
> > +		return 0;
> > +
> > +	ret = phy_isolate(phydev, phy_cfg->isolate);
> > +	if (ret)
> > +		NL_SET_ERR_MSG(extack, "Error while configuring PHY isolation");
> 
> This seems overly simplistic to me. Don't you need to iterate over all
> the other PHYs attached to this MAC and ensure they are isolated? Only
> one can be unisolated at once.
> 
> It is also not clear to me how this is going to work from a MAC
> perspective. Does the MAC call phy_connect() multiple times? How does
> ndev->phydev work? Who is responsible for the initial configuration,
> such that all but one PHY is isolated?
> 
> I assume you have a real board that needs this. So i think we need to
> see a bit more of the complete solution, including the MAC changes and
> the device tree for the board, so we can see the big picture.

Also what the ethernet driver looks like too!

One way around the ndev->phydev problem, assuming that we decide that
isolate is a good idea, would be to isolate the current PHY, disconnect
it from the net_device, connect the new PHY, and then clear the isolate
on the new PHY. Essentially, ndev->phydev becomes the currently-active
PHY.

However, I still want to hear whether multiple PHYs can be on the same
MII bus from a functional electrical perspective.

I know that on the Macchiatobin, where the 10G serdes signals go to the
88X3310 on doubleshot boards and to the SFP cage on singleshot boards,
this is controlled by the placement of zero ohm resistors to route the
serdes signals to the appropriate device, thus minimising the unused
stub lengths.

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

Re: [PATCH net-next v2 7/9] net: phy: introduce ethtool_phy_ops to get and set phy configuration

[Maxime Chevallier]

Hello Andrew, Russell,

On Fri, 4 Oct 2024 20:02:05 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

[...]

> > This seems overly simplistic to me. Don't you need to iterate over all
> > the other PHYs attached to this MAC and ensure they are isolated? Only
> > one can be unisolated at once.
> > 
> > It is also not clear to me how this is going to work from a MAC
> > perspective. Does the MAC call phy_connect() multiple times? How does
> > ndev->phydev work? Who is responsible for the initial configuration,
> > such that all but one PHY is isolated?
> > 
> > I assume you have a real board that needs this. So i think we need to
> > see a bit more of the complete solution, including the MAC changes and
> > the device tree for the board, so we can see the big picture.  
> 
> Also what the ethernet driver looks like too!
> 
> One way around the ndev->phydev problem, assuming that we decide that
> isolate is a good idea, would be to isolate the current PHY, disconnect
> it from the net_device, connect the new PHY, and then clear the isolate
> on the new PHY. Essentially, ndev->phydev becomes the currently-active
> PHY.

It seems I am missing details in my cover and the overall work I'm
trying to achieve.

This series focuses on isolating the PHY in the case where only one
PHY is attached to the MAC. I have followup work to support multi-PHY
interfaces. I will do my best to send the RFC this week so that you can
take a look. I'm definitely not saying the current code supports that.

To tell you some details, it indeed works as Russell says, I
detach/re-attach the PHYs, ndev->phydev is the "currently active" PHY.

I'm using a new dedicated "struct phy_mux" for that, which has :

 - Parent ops (that would be filled either by the MAC, or by phylink,
in the same spirit as phylink can be an sfp_upstream), which manages
PHY attach / detach to the netdev, but also the state-machine or the
currently inactive PHY.

 - multiplexer ops, that implement the switching logic, if any (drive a
GPIO, write a register, this is in the case of real multiplexers like
we have on some of the Turris Omnia boards, which the phy_mux framework
would support)

 - child ops, that would be hooks to activate/deactivate a PHY itself
(isoalte/unisolate, power-up/power-down).

I'll send the RFC ASAP, I still have a few rough edges that I will
mention in the cover.

> However, I still want to hear whether multiple PHYs can be on the same
> MII bus from a functional electrical perspective.

Yup, I have that hardware.

Thanks,

Maxime

Re: [PATCH net-next v2 7/9] net: phy: introduce ethtool_phy_ops to get and set phy configuration

[Andrew Lunn]

> It seems I am missing details in my cover and the overall work I'm
> trying to achieve.
> 
> This series focuses on isolating the PHY in the case where only one
> PHY is attached to the MAC.

I can understand implementing building blocks, but this patchset seems
to be more than that, it seems to be a use case of its own. But is
isolating a single PHY a useful use case? Do we want a kAPI for this?

> I have followup work to support multi-PHY
> interfaces. I will do my best to send the RFC this week so that you can
> take a look. I'm definitely not saying the current code supports that.
> 
> To tell you some details, it indeed works as Russell says, I
> detach/re-attach the PHYs, ndev->phydev is the "currently active" PHY.
> 
> I'm using a new dedicated "struct phy_mux" for that, which has :
> 
>  - Parent ops (that would be filled either by the MAC, or by phylink,
> in the same spirit as phylink can be an sfp_upstream), which manages
> PHY attach / detach to the netdev, but also the state-machine or the
> currently inactive PHY.
> 
>  - multiplexer ops, that implement the switching logic, if any (drive a
> GPIO, write a register, this is in the case of real multiplexers like
> we have on some of the Turris Omnia boards, which the phy_mux framework
> would support)
> 
>  - child ops, that would be hooks to activate/deactivate a PHY itself
> (isoalte/unisolate, power-up/power-down).

Does the kAPI for a single PHY get used, and extended, in this setup?

> I'll send the RFC ASAP, I still have a few rough edges that I will
> mention in the cover.
> 
> > However, I still want to hear whether multiple PHYs can be on the same
> > MII bus from a functional electrical perspective.
> 
> Yup, I have that hardware.

Can you talk a bit more about that hardware? What PHYs do you have?
What interface modes are they using?

	Andrew

Re: [PATCH net-next v2 7/9] net: phy: introduce ethtool_phy_ops to get and set phy configuration

[Maxime Chevallier]

Hi Andrew,

On Mon, 7 Oct 2024 15:01:50 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > It seems I am missing details in my cover and the overall work I'm
> > trying to achieve.
> > 
> > This series focuses on isolating the PHY in the case where only one
> > PHY is attached to the MAC.  
> 
> I can understand implementing building blocks, but this patchset seems
> to be more than that, it seems to be a use case of its own. But is
> isolating a single PHY a useful use case? Do we want a kAPI for this?

That's a legit point. I mentioned in the cover for V1 that this in
itself doesn't really bring anything useful. The only point being that
it makes it easy to test if a PHY has a working isolation mode, but
given that we'll assume that it doesn't by default, that whole point
is moot.

I would therefore understand if you consider that having a kAPI for
that isn't very interesting and that I shall include this work as part
of the multi-PHY support.

> > I have followup work to support multi-PHY
> > interfaces. I will do my best to send the RFC this week so that you can
> > take a look. I'm definitely not saying the current code supports that.
> > 
> > To tell you some details, it indeed works as Russell says, I
> > detach/re-attach the PHYs, ndev->phydev is the "currently active" PHY.
> > 
> > I'm using a new dedicated "struct phy_mux" for that, which has :
> > 
> >  - Parent ops (that would be filled either by the MAC, or by phylink,
> > in the same spirit as phylink can be an sfp_upstream), which manages
> > PHY attach / detach to the netdev, but also the state-machine or the
> > currently inactive PHY.
> > 
> >  - multiplexer ops, that implement the switching logic, if any (drive a
> > GPIO, write a register, this is in the case of real multiplexers like
> > we have on some of the Turris Omnia boards, which the phy_mux framework
> > would support)
> > 
> >  - child ops, that would be hooks to activate/deactivate a PHY itself
> > (isoalte/unisolate, power-up/power-down).  
> 
> Does the kAPI for a single PHY get used, and extended, in this setup?

For isolation, no.

> 
> > I'll send the RFC ASAP, I still have a few rough edges that I will
> > mention in the cover.
> >   
> > > However, I still want to hear whether multiple PHYs can be on the same
> > > MII bus from a functional electrical perspective.  
> > 
> > Yup, I have that hardware.  
> 
> Can you talk a bit more about that hardware? What PHYs do you have?
> What interface modes are they using?

Sure thing. There are multiple devices out-there that may have multiple
PHYs accessible from the MAC, through muxers (I'm trying to be generic
enough to address all cases, gpio muxers, mmio-controlled muxers, etc.),
but let me describe the HW I'm working on that's a bit more problematic.

The first such platform I have has an fs_enet MAC, a pair of LXT973
PHYs for which the isolate mode doesn't work, and no on-board circuitry to
perform the isolation. Here, we have to power one PHY down when unused :

                /--- LXT973
fs_enet -- MII--|
                \--- LXT973


The second board has a fs_enet MAC and a pair of KSZ8041 PHYs connected
in MII.

The third one has a pair of KSZ8041 PHYs connected to a
ucc_geth MAC in RMII.

On both these boards, we isolate the PHYs when unused, and we also
drive a GPIO to toggle some on-board circuitry to disconnect the MII
lines as well for the unused PHY. I'd have to run some tests to see if
this circuitry could be enough, without relying at all on PHY
isolation :

                   /--- KSZ8041
                   |
      MAC ------ MUX
                 | | 
  to SoC <-gpio--/ \--- KSZ8041


One point is, if you look at the first case (no mux), we need to know
if the PHYs are able to isolate or not in order to use the proper
switching strategy (isolate or power-down).

I hope this clarifies the approach a little bit ?

Thanks,

Maxime 

Re: [PATCH net-next v2 7/9] net: phy: introduce ethtool_phy_ops to get and set phy configuration

[Russell King (Oracle)]

On Mon, Oct 07, 2024 at 03:48:39PM +0200, Maxime Chevallier wrote:
> Sure thing. There are multiple devices out-there that may have multiple
> PHYs accessible from the MAC, through muxers (I'm trying to be generic
> enough to address all cases, gpio muxers, mmio-controlled muxers, etc.),
> but let me describe the HW I'm working on that's a bit more problematic.
> 
> The first such platform I have has an fs_enet MAC, a pair of LXT973
> PHYs for which the isolate mode doesn't work, and no on-board circuitry to
> perform the isolation. Here, we have to power one PHY down when unused :
> 
>                 /--- LXT973
> fs_enet -- MII--|
>                 \--- LXT973
> 
> 
> The second board has a fs_enet MAC and a pair of KSZ8041 PHYs connected
> in MII.
> 
> The third one has a pair of KSZ8041 PHYs connected to a
> ucc_geth MAC in RMII.
> 
> On both these boards, we isolate the PHYs when unused, and we also
> drive a GPIO to toggle some on-board circuitry to disconnect the MII
> lines as well for the unused PHY. I'd have to run some tests to see if
> this circuitry could be enough, without relying at all on PHY
> isolation :
> 
>                    /--- KSZ8041
>                    |
>       MAC ------ MUX
>                  | | 
>   to SoC <-gpio--/ \--- KSZ8041
> 
> 
> One point is, if you look at the first case (no mux), we need to know
> if the PHYs are able to isolate or not in order to use the proper
> switching strategy (isolate or power-down).
> 
> I hope this clarifies the approach a little bit ?

What I gather from the above is you have these scenarios:

1) two LXT973 on a MII bus (not RMII, RGMII etc but the 802.3 defined
   MII bus with four data lines in each direction, a bunch of control
   signals, clocked at a maximum of 25MHz). In this case, you need to
   power down each PHY so it doesn't interfere on the MII bus as the
   PHY doesn't support isolate mode.

2) two KSZ8041 on a MII bus to a multiplexer who's exact behaviour is
   not yet known which may require the use of the PHYs isolate bit.

I would suggest that spending time adding infrastructure for a rare
scenario, and when it is uncertain whether it needs to be used in
these scenarios is premature.

Please validate on the two KSZ8041 setups whether isolate is
necessary.

Presumably on those two KSZ88041 setups, the idea is to see which PHY
ends up with media link first, and then switch between the two PHYs?

Lastly, I'm a little confused why someone would layout a platform
where there are two identical PHYs connected to one MAC on the same
board. I can see the use case given in 802.3 - where one plugs in
the media specific attachment unit depending on the media being
used - Wikipedia has a photo of the connector on a Sun Ultra 1 -
but to have two PHYs on the same board doesn't make much sense to
me. What is trying to be achieved with these two PHYs on the same
board?

It's got me wondering whether the platform you have is some kind of
development board, and the manufacturer feels the need to provide a
network socket on either end of the board because folk don't have a
long enough ethernet cable to reach the other end! :D

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

Re: [PATCH net-next v2 7/9] net: phy: introduce ethtool_phy_ops to get and set phy configuration

[Maxime Chevallier]

On Mon, 7 Oct 2024 17:10:56 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Mon, Oct 07, 2024 at 03:48:39PM +0200, Maxime Chevallier wrote:
> > Sure thing. There are multiple devices out-there that may have multiple
> > PHYs accessible from the MAC, through muxers (I'm trying to be generic
> > enough to address all cases, gpio muxers, mmio-controlled muxers, etc.),
> > but let me describe the HW I'm working on that's a bit more problematic.
> > 
> > The first such platform I have has an fs_enet MAC, a pair of LXT973
> > PHYs for which the isolate mode doesn't work, and no on-board circuitry to
> > perform the isolation. Here, we have to power one PHY down when unused :
> > 
> >                 /--- LXT973
> > fs_enet -- MII--|
> >                 \--- LXT973
> > 
> > 
> > The second board has a fs_enet MAC and a pair of KSZ8041 PHYs connected
> > in MII.
> > 
> > The third one has a pair of KSZ8041 PHYs connected to a
> > ucc_geth MAC in RMII.
> > 
> > On both these boards, we isolate the PHYs when unused, and we also
> > drive a GPIO to toggle some on-board circuitry to disconnect the MII
> > lines as well for the unused PHY. I'd have to run some tests to see if
> > this circuitry could be enough, without relying at all on PHY
> > isolation :
> > 
> >                    /--- KSZ8041
> >                    |
> >       MAC ------ MUX
> >                  | | 
> >   to SoC <-gpio--/ \--- KSZ8041
> > 
> > 
> > One point is, if you look at the first case (no mux), we need to know
> > if the PHYs are able to isolate or not in order to use the proper
> > switching strategy (isolate or power-down).
> > 
> > I hope this clarifies the approach a little bit ?  
> 
> What I gather from the above is you have these scenarios:
> 
> 1) two LXT973 on a MII bus (not RMII, RGMII etc but the 802.3 defined
>    MII bus with four data lines in each direction, a bunch of control
>    signals, clocked at a maximum of 25MHz). In this case, you need to
>    power down each PHY so it doesn't interfere on the MII bus as the
>    PHY doesn't support isolate mode.

Correct

> 
> 2) two KSZ8041 on a MII bus to a multiplexer who's exact behaviour is
>    not yet known which may require the use of the PHYs isolate bit.

Correct as well

> 
> I would suggest that spending time adding infrastructure for a rare
> scenario, and when it is uncertain whether it needs to be used in
> these scenarios is premature.
> 
> Please validate on the two KSZ8041 setups whether isolate is
> necessary.

I'll do

> Presumably on those two KSZ88041 setups, the idea is to see which PHY
> ends up with media link first, and then switch between the two PHYs?

Indeed. I already have code for that (I was expecting that whole
discussion to happen in the RFC for said code :D )

> Lastly, I'm a little confused why someone would layout a platform
> where there are two identical PHYs connected to one MAC on the same
> board. I can see the use case given in 802.3 - where one plugs in
> the media specific attachment unit depending on the media being
> used - Wikipedia has a photo of the connector on a Sun Ultra 1 -
> but to have two PHYs on the same board doesn't make much sense to
> me. What is trying to be achieved with these two PHYs on the same
> board?

The use-case is redundancy, to switch between the PHYs when the link
goes down on one side. I don't know why bonding isn't used, I suspect
this is because there's not enough MACs on the device to do that. These
are pretty old hardware platforms that have been in use in the field
for quite some time and will continue to be for a while.

I've been trying to decompose support for what is a niche use-case into
something that can benefit other devices :

 - Turris omnia for example uses a MII mux at the serdes level to
switch between a PHY and an SFP bus. We could leverage a phy_mux infra
here to support these related use-cases

 - I've always considered that this is similar enough ( from the
end-user perspective ) to cases like MCBin or other switches that have
2 ports connected to the same MAC. 

In the end, we have 1 netdev, 2 ports, regardless of wether there are 2
PHYs or 1. Of course, the capabilities are not the same, we can't
detect link/power simultaneously in all situations, but I'm keeping
that in mind in the design, and I've talked about this a few weeks ago
at LPC [1].

Thanks,

Maxime

[1] : https://bootlin.com/pub/conferences/2024/lpc/chevallier-phy-port/chevallier-phy-port.pdf

Re: [PATCH net-next v2 7/9] net: phy: introduce ethtool_phy_ops to get and set phy configuration

[Andrew Lunn]

> That's a legit point. I mentioned in the cover for V1 that this in
> itself doesn't really bring anything useful. The only point being that
> it makes it easy to test if a PHY has a working isolation mode, but
> given that we'll assume that it doesn't by default, that whole point
> is moot.
> 
> I would therefore understand if you consider that having a kAPI for
> that isn't very interesting and that I shall include this work as part
> of the multi-PHY support.

kAPI add a lot of Maintenance burden. So we should not add them unless
they are justified. to me, there is not a good justification for this.

> Sure thing. There are multiple devices out-there that may have multiple
> PHYs accessible from the MAC, through muxers (I'm trying to be generic
> enough to address all cases, gpio muxers, mmio-controlled muxers, etc.),
> but let me describe the HW I'm working on that's a bit more problematic.
> 
> The first such platform I have has an fs_enet MAC, a pair of LXT973
> PHYs for which the isolate mode doesn't work, and no on-board circuitry to
> perform the isolation. Here, we have to power one PHY down when unused :
> 
>                 /--- LXT973
> fs_enet -- MII--|
>                 \--- LXT973

So you have at least regulators under Linux control? Is that what you
mean by power down? Pulling the plug and putting it back again is
somewhat different to isolation. All its state is going to be lost,
meaning phylib needs to completely initialise it again. Or can you
hide this using PM? Just suspend/resume it?

> The second board has a fs_enet MAC and a pair of KSZ8041 PHYs connected
> in MII.
> 
> The third one has a pair of KSZ8041 PHYs connected to a
> ucc_geth MAC in RMII.
> 
> On both these boards, we isolate the PHYs when unused, and we also
> drive a GPIO to toggle some on-board circuitry to disconnect the MII
> lines as well for the unused PHY. I'd have to run some tests to see if
> this circuitry could be enough, without relying at all on PHY
> isolation :
> 
>                    /--- KSZ8041
>                    |
>       MAC ------ MUX
>                  | | 
>   to SoC <-gpio--/ \--- KSZ8041
> 
> 
> One point is, if you look at the first case (no mux), we need to know
> if the PHYs are able to isolate or not in order to use the proper
> switching strategy (isolate or power-down).

That explains the hardware, but what are the use cases? How did the
hardware designer envision this hardware being used?

If you need to power the PHY off, you cannot have dynamic behaviour
where the first to have link wins. But if you can have the media side
functional, you can do some dynamic behaviours. Although, is it wise
for the link to come up, yet to be functionally dead because it has no
MAC connected?

There are some Marvell Switches which support both internal Copper
PHYs and a SERDES port. The hardware allows first to get link to have
a functional MAC. But in Linux we have not supported that, and we
leave the unused part down so it does not get link.

Maybe we actually want energy detect, not link, to decide which PHY
should get the MAC?  But i have no real idea what you can do with
energy detect, and it would also mean building out the read_status()
call to report additional things, etc.

	Andrew

Re: [PATCH net-next v2 7/9] net: phy: introduce ethtool_phy_ops to get and set phy configuration

[Maxime Chevallier]

On Mon, 7 Oct 2024 18:37:29 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > That's a legit point. I mentioned in the cover for V1 that this in
> > itself doesn't really bring anything useful. The only point being that
> > it makes it easy to test if a PHY has a working isolation mode, but
> > given that we'll assume that it doesn't by default, that whole point
> > is moot.
> > 
> > I would therefore understand if you consider that having a kAPI for
> > that isn't very interesting and that I shall include this work as part
> > of the multi-PHY support.  
> 
> kAPI add a lot of Maintenance burden. So we should not add them unless
> they are justified. to me, there is not a good justification for this.

That's fine by me.

> 
> > Sure thing. There are multiple devices out-there that may have multiple
> > PHYs accessible from the MAC, through muxers (I'm trying to be generic
> > enough to address all cases, gpio muxers, mmio-controlled muxers, etc.),
> > but let me describe the HW I'm working on that's a bit more problematic.
> > 
> > The first such platform I have has an fs_enet MAC, a pair of LXT973
> > PHYs for which the isolate mode doesn't work, and no on-board circuitry to
> > perform the isolation. Here, we have to power one PHY down when unused :
> > 
> >                 /--- LXT973
> > fs_enet -- MII--|
> >                 \--- LXT973  
> 
> So you have at least regulators under Linux control? Is that what you
> mean by power down? Pulling the plug and putting it back again is
> somewhat different to isolation. All its state is going to be lost,
> meaning phylib needs to completely initialise it again. Or can you
> hide this using PM? Just suspend/resume it?

Ah no, I wasn't referring to regulators but rather the BMCR PDOWN bit to
just shut the PHY down, as in suspend.

Indeed the state is lost. The way I'm supporting this is :

 - If one PHY has the link, it keeps it until link-down
 - When link-down, I round-robin between the 2 phys: 

  - Attach the PHY to the netdev
  - See if it can establish link and negotiate with LP
  - If there's nothing after a given period ( 2 seconds default ), then
I detach the PHY, attach the other one, and start again, until one of
them has link.

That's very limited indeed, we have no way of saying "first that has
link wins".


> > The second board has a fs_enet MAC and a pair of KSZ8041 PHYs connected
> > in MII.
> > 
> > The third one has a pair of KSZ8041 PHYs connected to a
> > ucc_geth MAC in RMII.
> > 
> > On both these boards, we isolate the PHYs when unused, and we also
> > drive a GPIO to toggle some on-board circuitry to disconnect the MII
> > lines as well for the unused PHY. I'd have to run some tests to see if
> > this circuitry could be enough, without relying at all on PHY
> > isolation :
> > 
> >                    /--- KSZ8041
> >                    |
> >       MAC ------ MUX
> >                  | | 
> >   to SoC <-gpio--/ \--- KSZ8041
> > 
> > 
> > One point is, if you look at the first case (no mux), we need to know
> > if the PHYs are able to isolate or not in order to use the proper
> > switching strategy (isolate or power-down).  
> 
> That explains the hardware, but what are the use cases? How did the
> hardware designer envision this hardware being used?

The use-case is link redundancy, if one PHY loses the link, we hope
that we still have link on the other one and switchover. This is one of
the things I discussed at netdev 0x17.

> If you need to power the PHY off, you cannot have dynamic behaviour
> where the first to have link wins. But if you can have the media side
> functional, you can do some dynamic behaviours.

True.

> Although, is it wise
> for the link to come up, yet to be functionally dead because it has no
> MAC connected?

Good point. What would you think ? I already deal with the identified
issue which is that both PHYs are link-up with LP, both connected to
the same switch. When we switch between the active PHYs, we send a
gratuitous ARP on the new PHY to refresh the switch's FDB.

Do you see that as being an issue, having the LP see link-up when the
link cannot actually convey data ? Besides the energy detect feature
you mention, I don't see what other options we can have unfortunately :(

> There are some Marvell Switches which support both internal Copper
> PHYs and a SERDES port. The hardware allows first to get link to have
> a functional MAC. But in Linux we have not supported that, and we
> leave the unused part down so it does not get link.

My plan is to support these as well. For the end-user, it makes no
difference wether the HW internally has 2 PHYs each with one port, or 1
phy with 2 ports. So to me, if we want to support phy_mux, we should
also support the case you mention above. I have some code to support
this, but that's the part where I'm still getting things ironed-out,
this is pretty tricky to represent that properly, especially in DT.

>
> Maybe we actually want energy detect, not link, to decide which PHY
> should get the MAC?  But i have no real idea what you can do with
> energy detect, and it would also mean building out the read_status()
> call to report additional things, etc.

Note that I'm trying to support a bigger set of use-cases besides the
pure 2-PHY setup. One being that we have a MUX within the SoC on the
SERDES lanes, allowing to steer the MII interface between a PHY and an
SFP bus (Turris Omnia has such a setup). Is it possible to have an
equivalent "energy detect" on all kinds of SFPs ?

As a note, I do see that both Russell and you may think you're being
"drip-fed" (I learned that term today) information, that's not my
intent at all, I wasn't expecting this discussion now, sorry about that.

I was saying to Russell that I would start a new thread, but we already
have a discussion going here, let me know if we shall continue the
discussion here or on a new thread.

Thanks,

Maxime

Re: [PATCH net-next v2 7/9] net: phy: introduce ethtool_phy_ops to get and set phy configuration

[Andrew Lunn]

> > So you have at least regulators under Linux control? Is that what you
> > mean by power down? Pulling the plug and putting it back again is
> > somewhat different to isolation. All its state is going to be lost,
> > meaning phylib needs to completely initialise it again. Or can you
> > hide this using PM? Just suspend/resume it?
> 
> Ah no, I wasn't referring to regulators but rather the BMCR PDOWN bit to
> just shut the PHY down, as in suspend.

Ah! I wounder what 802.3 says about PDOWN? Does it say anything about
it being equivalent to ISOLATE? That the pins go HI-Z? Are we talking
about something semi-reliable, or something which just happens to work
for this PHY?

> Indeed the state is lost. The way I'm supporting this is :
> 
>  - If one PHY has the link, it keeps it until link-down
>  - When link-down, I round-robin between the 2 phys: 
> 
>   - Attach the PHY to the netdev
>   - See if it can establish link and negotiate with LP
>   - If there's nothing after a given period ( 2 seconds default ), then
> I detach the PHY, attach the other one, and start again, until one of
> them has link.

This sounds pretty invasive to the MAC driver. I don't think you need
to attach/detach each cycle, since you don't need to send/receive any
packets. You could hide this all in phylib. But that should be
considered as part of the bigger picture.

I assume it is not actually 2 seconds, but some random number in the
range 1-3 seconds, so when both ends are searching they do eventually
find each other?

> > That explains the hardware, but what are the use cases? How did the
> > hardware designer envision this hardware being used?
> 
> The use-case is link redundancy, if one PHY loses the link, we hope
> that we still have link on the other one and switchover. This is one of
> the things I discussed at netdev 0x17.

> > If you need to power the PHY off, you cannot have dynamic behaviour
> > where the first to have link wins. But if you can have the media side
> > functional, you can do some dynamic behaviours.
> 
> True.
> 
> > Although, is it wise
> > for the link to come up, yet to be functionally dead because it has no
> > MAC connected?
> 
> Good point. What would you think ? I already deal with the identified
> issue which is that both PHYs are link-up with LP, both connected to
> the same switch. When we switch between the active PHYs, we send a
> gratuitous ARP on the new PHY to refresh the switch's FDB.

It seems odd to me you have redundant cables going to one switch? I
would have the cables going in opposite directions, to two different
switches, and have the switches in at a minimum a ring, or ideally a
mesh.

I don't think the ARP is necessary. The link peer switch should flush
its tables when the link goes down. But switches further away don't
see such link events, yet they learn about the new location of the
host. I would also expect the host sees a loss of carrier and then the
carrier restored, which probably flushes all its tables, so it is
going to ARP anyway.

> 
> Do you see that as being an issue, having the LP see link-up when the
> link cannot actually convey data ? Besides the energy detect feature
> you mention, I don't see what other options we can have unfortunately :(

Maybe see what 802.3 says about advertising with no link
modes. Autoneg should complete, in that the peers exchange messages,
but the result of the autoneg is that they have no common modes, so
the link won't come up. Is it clearly defined what should happen in
this case? But we are in a corner case, similar to ISOLATE, which i
guess rarely gets tested, so is often broken. I would guess power
detection would be more reliable when implemented. 

> > There are some Marvell Switches which support both internal Copper
> > PHYs and a SERDES port. The hardware allows first to get link to have
> > a functional MAC. But in Linux we have not supported that, and we
> > leave the unused part down so it does not get link.
> 
> My plan is to support these as well. For the end-user, it makes no
> difference wether the HW internally has 2 PHYs each with one port, or 1
> phy with 2 ports. So to me, if we want to support phy_mux, we should
> also support the case you mention above. I have some code to support
> this, but that's the part where I'm still getting things ironed-out,
> this is pretty tricky to represent that properly, especially in DT.
> 
> >
> > Maybe we actually want energy detect, not link, to decide which PHY
> > should get the MAC?  But i have no real idea what you can do with
> > energy detect, and it would also mean building out the read_status()
> > call to report additional things, etc.
> 
> Note that I'm trying to support a bigger set of use-cases besides the
> pure 2-PHY setup. One being that we have a MUX within the SoC on the
> SERDES lanes, allowing to steer the MII interface between a PHY and an
> SFP bus (Turris Omnia has such a setup). Is it possible to have an
> equivalent "energy detect" on all kinds of SFPs ?

The LOS pin, which indicates if there is light entering the SFP.

> As a note, I do see that both Russell and you may think you're being
> "drip-fed" (I learned that term today) information, that's not my
> intent at all, I wasn't expecting this discussion now, sorry about that.

It is a difficult set of problems, and you are addressing it from the
very niche end first using mechanisms which i expect are not reliably
implemented. So we are going to ask lots of questions.

You probably would of got less questions if you have started with the
use cases for the Turris Omnia and Marvell Ethernet switch, which are
more mainstream, and then extended it with your niche device. But i
can understand this order, you probably have a customer with this
niche device...

	Andrew

Re: [PATCH net-next v2 7/9] net: phy: introduce ethtool_phy_ops to get and set phy configuration

[Russell King (Oracle)]

On Tue, Oct 08, 2024 at 03:00:53PM +0200, Andrew Lunn wrote:
> > > So you have at least regulators under Linux control? Is that what you
> > > mean by power down? Pulling the plug and putting it back again is
> > > somewhat different to isolation. All its state is going to be lost,
> > > meaning phylib needs to completely initialise it again. Or can you
> > > hide this using PM? Just suspend/resume it?
> > 
> > Ah no, I wasn't referring to regulators but rather the BMCR PDOWN bit to
> > just shut the PHY down, as in suspend.
> 
> Ah! I wounder what 802.3 says about PDOWN? Does it say anything about
> it being equivalent to ISOLATE? That the pins go HI-Z? Are we talking
> about something semi-reliable, or something which just happens to work
> for this PHY?

"The specific behavior of a PHY in the power-down state is
implementation specific. While in the power-down state, the PHY shall
respond to management transactions. During the transition to the
power-down state and while in the power-down state, the PHY shall not
generate spurious signals on the MII or GMII."

So no, there is no requirement in 802.3 for the MII bus to go into
HI-Z state, the only requirement is to avoid creating spurious
signals. One way to achieve that would be to go into Hi-Z state,
but another way would be to drive the signals to an inactive state.
Thus, as it's not defined, setting 0.11 can't be relied upon to
allow two PHYs to be on the same MII bus.

It seems we're into implementation specifics and not generalities
here.

> > Indeed the state is lost. The way I'm supporting this is :
> > 
> >  - If one PHY has the link, it keeps it until link-down
> >  - When link-down, I round-robin between the 2 phys: 
> > 
> >   - Attach the PHY to the netdev
> >   - See if it can establish link and negotiate with LP
> >   - If there's nothing after a given period ( 2 seconds default ), then
> > I detach the PHY, attach the other one, and start again, until one of
> > them has link.
> 
> This sounds pretty invasive to the MAC driver. I don't think you need
> to attach/detach each cycle, since you don't need to send/receive any
> packets. You could hide this all in phylib. But that should be
> considered as part of the bigger picture.

Given that management transactions are permitted while PDOWN is set,
it seems we're again into an implementation specific behaviour where
setting this bit results in the PHY losing its brains. :(

> > > Although, is it wise
> > > for the link to come up, yet to be functionally dead because it has no
> > > MAC connected?
> > 
> > Good point. What would you think ? I already deal with the identified
> > issue which is that both PHYs are link-up with LP, both connected to
> > the same switch. When we switch between the active PHYs, we send a
> > gratuitous ARP on the new PHY to refresh the switch's FDB.
> 
> It seems odd to me you have redundant cables going to one switch? I
> would have the cables going in opposite directions, to two different
> switches, and have the switches in at a minimum a ring, or ideally a
> mesh.
> 
> I don't think the ARP is necessary. The link peer switch should flush
> its tables when the link goes down. But switches further away don't
> see such link events, yet they learn about the new location of the
> host. I would also expect the host sees a loss of carrier and then the
> carrier restored, which probably flushes all its tables, so it is
> going to ARP anyway.

The ARP will be necessary if you want to have the two links going to two
different switches - otherwise how does the switches upstream of those
two switches know to route packets to the MAC's ethernet address to the
different path... you'd have to wait for the higher level switches to
age their tables.

> > Note that I'm trying to support a bigger set of use-cases besides the
> > pure 2-PHY setup. One being that we have a MUX within the SoC on the
> > SERDES lanes, allowing to steer the MII interface between a PHY and an
> > SFP bus (Turris Omnia has such a setup). Is it possible to have an
> > equivalent "energy detect" on all kinds of SFPs ?
> 
> The LOS pin, which indicates if there is light entering the SFP.

Basically... no. You can't trust anything that SFPs give you. True fibre
SFPs as per the original design are way better at giving a RXLOS signal,
but everything else (including GPON) are a game.

GPON SFPs may even use the RXLOS as their UART transmit pin, wiggling it
at random times.

SFPs are a mess.

(Sorry, for what I feel is another incomplete reply...)

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

Re: [PATCH net-next v2 7/9] net: phy: introduce ethtool_phy_ops to get and set phy configuration

[Maxime Chevallier]

On Tue, 8 Oct 2024 15:00:53 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > > So you have at least regulators under Linux control? Is that what you
> > > mean by power down? Pulling the plug and putting it back again is
> > > somewhat different to isolation. All its state is going to be lost,
> > > meaning phylib needs to completely initialise it again. Or can you
> > > hide this using PM? Just suspend/resume it?  
> > 
> > Ah no, I wasn't referring to regulators but rather the BMCR PDOWN bit to
> > just shut the PHY down, as in suspend.  
> 
> Ah! I wounder what 802.3 says about PDOWN? Does it say anything about
> it being equivalent to ISOLATE? That the pins go HI-Z? Are we talking
> about something semi-reliable, or something which just happens to work
> for this PHY?

The spec doesn't say anything about hi-z on the MII for power-down, it
simply says (22.2.4.1.5 Power down) :

"During the transition to the power-down state and while
in the power-down state, the PHY shall not generate spurious
signals on the MII or GMII"

So my best guess is that it just happens to work for this PHY. It won't
work for serdes links for the same reasons as the isolate mode I guess,
reflections would make it too unstable ?

> > Indeed the state is lost. The way I'm supporting this is :
> > 
> >  - If one PHY has the link, it keeps it until link-down
> >  - When link-down, I round-robin between the 2 phys: 
> > 
> >   - Attach the PHY to the netdev
> >   - See if it can establish link and negotiate with LP
> >   - If there's nothing after a given period ( 2 seconds default ), then
> > I detach the PHY, attach the other one, and start again, until one of
> > them has link.  
> 
> This sounds pretty invasive to the MAC driver. I don't think you need
> to attach/detach each cycle, since you don't need to send/receive any
> packets. You could hide this all in phylib. But that should be
> considered as part of the bigger picture.

Sure, that's what I came-up with so far but that's indeed an implem
problem.

> 
> I assume it is not actually 2 seconds, but some random number in the
> range 1-3 seconds, so when both ends are searching they do eventually
> find each other?

Oleksji pointed that out to me at LPC, that makes sense indeed.

> 
> > > That explains the hardware, but what are the use cases? How did the
> > > hardware designer envision this hardware being used?  
> > 
> > The use-case is link redundancy, if one PHY loses the link, we hope
> > that we still have link on the other one and switchover. This is one of
> > the things I discussed at netdev 0x17.  
> 
> > > If you need to power the PHY off, you cannot have dynamic behaviour
> > > where the first to have link wins. But if you can have the media side
> > > functional, you can do some dynamic behaviours.  
> > 
> > True.
> >   
> > > Although, is it wise
> > > for the link to come up, yet to be functionally dead because it has no
> > > MAC connected?  
> > 
> > Good point. What would you think ? I already deal with the identified
> > issue which is that both PHYs are link-up with LP, both connected to
> > the same switch. When we switch between the active PHYs, we send a
> > gratuitous ARP on the new PHY to refresh the switch's FDB.  
> 
> It seems odd to me you have redundant cables going to one switch? I
> would have the cables going in opposite directions, to two different
> switches, and have the switches in at a minimum a ring, or ideally a
> mesh.
> 
> I don't think the ARP is necessary. The link peer switch should flush
> its tables when the link goes down. But switches further away don't
> see such link events, yet they learn about the new location of the
> host. I would also expect the host sees a loss of carrier and then the
> carrier restored, which probably flushes all its tables, so it is
> going to ARP anyway.

While I would agree with you on the theory, while testing we discovered
that sending that ARP was necessary to reliably update the switch's
tables :/

This is also what bonding does in active-backup mode.

> > 
> > Do you see that as being an issue, having the LP see link-up when the
> > link cannot actually convey data ? Besides the energy detect feature
> > you mention, I don't see what other options we can have unfortunately :(  
> 
> Maybe see what 802.3 says about advertising with no link
> modes. Autoneg should complete, in that the peers exchange messages,
> but the result of the autoneg is that they have no common modes, so
> the link won't come up. Is it clearly defined what should happen in
> this case? But we are in a corner case, similar to ISOLATE, which i
> guess rarely gets tested, so is often broken. I would guess power
> detection would be more reliable when implemented. 

I'll need to perform further tests on that, I haven't looked into
energy detect. Let me take a look :)

> > > There are some Marvell Switches which support both internal Copper
> > > PHYs and a SERDES port. The hardware allows first to get link to have
> > > a functional MAC. But in Linux we have not supported that, and we
> > > leave the unused part down so it does not get link.  
> > 
> > My plan is to support these as well. For the end-user, it makes no
> > difference wether the HW internally has 2 PHYs each with one port, or 1
> > phy with 2 ports. So to me, if we want to support phy_mux, we should
> > also support the case you mention above. I have some code to support
> > this, but that's the part where I'm still getting things ironed-out,
> > this is pretty tricky to represent that properly, especially in DT.
> >   
> > >
> > > Maybe we actually want energy detect, not link, to decide which PHY
> > > should get the MAC?  But i have no real idea what you can do with
> > > energy detect, and it would also mean building out the read_status()
> > > call to report additional things, etc.  
> > 
> > Note that I'm trying to support a bigger set of use-cases besides the
> > pure 2-PHY setup. One being that we have a MUX within the SoC on the
> > SERDES lanes, allowing to steer the MII interface between a PHY and an
> > SFP bus (Turris Omnia has such a setup). Is it possible to have an
> > equivalent "energy detect" on all kinds of SFPs ?  
> 
> The LOS pin, which indicates if there is light entering the SFP.
> 
> > As a note, I do see that both Russell and you may think you're being
> > "drip-fed" (I learned that term today) information, that's not my
> > intent at all, I wasn't expecting this discussion now, sorry about that.  
> 
> It is a difficult set of problems, and you are addressing it from the
> very niche end first using mechanisms which i expect are not reliably
> implemented. So we are going to ask lots of questions.

There's absolutely no problem with that :)

> You probably would of got less questions if you have started with the
> use cases for the Turris Omnia and Marvell Ethernet switch, which are
> more mainstream, and then extended it with your niche device. But i
> can understand this order, you probably have a customer with this
> niche device...

Oh but I plan to add support for the marvell switch, mcbin, and turris
first, as these boards are somewhat easily accessible and allows
converging towards a proper kAPI for that without relying on the boards
only I and a few other folks have.

That's another can of worms though :)

Maxime

Re: [PATCH net-next v2 7/9] net: phy: introduce ethtool_phy_ops to get and set phy configuration

[Russell King (Oracle)]

On Tue, Oct 08, 2024 at 04:57:42PM +0200, Maxime Chevallier wrote:
> Oh but I plan to add support for the marvell switch, mcbin, and turris
> first,

What do you think needs adding for the mcbin?

For the single-shot version, the serdes lines are hard-wired to the
SFP cages, so it's a MAC with a SFP cage directly connected.

For the double-shot, the switching happens dynamically within the
88x3310 PHY, so there's no need to fiddle with any isolate modes.

The only thing that is missing is switching the 88x3310's fibre
interface from the default 10gbase-r to 1000base-X and/or SGMII, and
allowing PHYs to be stacked on top. The former I have untested
patches for but the latter is something that's waiting for
networking/phylib to gain support for stacked PHY.

Switching the interface mode is very disruptive as it needs the PHY
to be software-reset, and if the RJ45 has link but one is simply
plugging in a SFP, hitting the PHY with a software reset will
disrupt that link.

Given that the mcbin has one SFP cage that is capable of 2500base-X,
1000base-X and SGMII, and two SFP cages that can do 10gbase-r, with
a PHY that can do 10/100/1G/2.5G/5G/10G over the RJ45, I'm not sure
adding more complexity really gains us very much other than...
additional complexity.

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

Re: [PATCH net-next v2 7/9] net: phy: introduce ethtool_phy_ops to get and set phy configuration

[Maxime Chevallier]

On Tue, 8 Oct 2024 16:27:43 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Tue, Oct 08, 2024 at 04:57:42PM +0200, Maxime Chevallier wrote:
> > Oh but I plan to add support for the marvell switch, mcbin, and turris
> > first,  
> 
> What do you think needs adding for the mcbin?
> 
> For the single-shot version, the serdes lines are hard-wired to the
> SFP cages, so it's a MAC with a SFP cage directly connected.
> 
> For the double-shot, the switching happens dynamically within the
> 88x3310 PHY, so there's no need to fiddle with any isolate modes.

Nothing related to isolate mode regarding the mcbin :) They aren't
even implemented on the 3310 PHYs anyway :)

> 
> The only thing that is missing is switching the 88x3310's fibre
> interface from the default 10gbase-r to 1000base-X and/or SGMII, and
> allowing PHYs to be stacked on top. The former I have untested
> patches for but the latter is something that's waiting for
> networking/phylib to gain support for stacked PHY.

That's one part of it indeed

> Switching the interface mode is very disruptive as it needs the PHY
> to be software-reset, and if the RJ45 has link but one is simply
> plugging in a SFP, hitting the PHY with a software reset will
> disrupt that link.
> 
> Given that the mcbin has one SFP cage that is capable of 2500base-X,
> 1000base-X and SGMII, and two SFP cages that can do 10gbase-r, with
> a PHY that can do 10/100/1G/2.5G/5G/10G over the RJ45, I'm not sure
> adding more complexity really gains us very much other than...
> additional complexity.

What I mean is the ability for users to see, from tools like ethtool,
that the MCBin doubleshot's eth0 and eth1 interfaces have 2 ports
(copper + sfp), and potentially allow picking which one to use in case
both ports are connected.

There are mutliple devices out-there with such configurations (some
marvell switches for example). Do you not see some value in this ?

This isn't related at all to isolate, but rather the bigger picture of
the type of topology I'm trying to improve support for.

Setups with 2 PHYs connected to the same MAC are similar to the eth0/1
interfaces in the sense that they offer 2 front-facing ports for one
single MAC. IMO it's easier to first deal with the MCBin setup first.

Again that's a whole other topic, but my idea would be to be able, from
ethtool, to see that mcbin eth0 has one port capable of
10/100/1000/2500/5000/10000BaseT, and another capable of
1000BaseX/2500BaseX/5GBaseR/10GBaseR or whatever the plugged SFP module
offers.

I do know that the MCBin has a large variety of interfaces easily
accessible, but it also looks like a good board to introduce such
multi-port support. Many people have it, it works well with an upstream
kernel, making testing and review effort easier IMO.

I know that this whole thing of dealing with 2 PHYs attached to the
same MAC has lots of ramifications (the 1 PHY 2 ports setup I just
mentionned, the phy_link_topology that has been added, the isolate
mode, muxing support, etc.), that's why I tried to cover all the angles
at netdevconf + LPC. I have code for most of that pretty-much ready to
send.

Thanks,

Maxime

Re: [PATCH net-next v2 7/9] net: phy: introduce ethtool_phy_ops to get and set phy configuration

[Russell King (Oracle)]

On Tue, Oct 08, 2024 at 06:41:02PM +0200, Maxime Chevallier wrote:
> On Tue, 8 Oct 2024 16:27:43 +0100
> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> 
> > On Tue, Oct 08, 2024 at 04:57:42PM +0200, Maxime Chevallier wrote:
> > > Oh but I plan to add support for the marvell switch, mcbin, and turris
> > > first,  
> > 
> > What do you think needs adding for the mcbin?
> > 
> > For the single-shot version, the serdes lines are hard-wired to the
> > SFP cages, so it's a MAC with a SFP cage directly connected.
> > 
> > For the double-shot, the switching happens dynamically within the
> > 88x3310 PHY, so there's no need to fiddle with any isolate modes.
> 
> Nothing related to isolate mode regarding the mcbin :) They aren't
> even implemented on the 3310 PHYs anyway :)
> 
> > 
> > The only thing that is missing is switching the 88x3310's fibre
> > interface from the default 10gbase-r to 1000base-X and/or SGMII, and
> > allowing PHYs to be stacked on top. The former I have untested
> > patches for but the latter is something that's waiting for
> > networking/phylib to gain support for stacked PHY.
> 
> That's one part of it indeed
> 
> > Switching the interface mode is very disruptive as it needs the PHY
> > to be software-reset, and if the RJ45 has link but one is simply
> > plugging in a SFP, hitting the PHY with a software reset will
> > disrupt that link.
> > 
> > Given that the mcbin has one SFP cage that is capable of 2500base-X,
> > 1000base-X and SGMII, and two SFP cages that can do 10gbase-r, with
> > a PHY that can do 10/100/1G/2.5G/5G/10G over the RJ45, I'm not sure
> > adding more complexity really gains us very much other than...
> > additional complexity.
> 
> What I mean is the ability for users to see, from tools like ethtool,
> that the MCBin doubleshot's eth0 and eth1 interfaces have 2 ports
> (copper + sfp), and potentially allow picking which one to use in case
> both ports are connected.
> 
> There are mutliple devices out-there with such configurations (some
> marvell switches for example). Do you not see some value in this ?

Many PHYs that have two media facing ports give configuration of the
priority between the two interfaces, and yes, there would definitely be
value in exposing that to userspace, thereby allowing userspace to
configure the policy there.

This would probably be more common than the two-PHY issue that we're
starting with - as I believe the 88e151x PHYs support exactly the same
thing when used with a RGMII host interface. The serdes port becomes
available for "fiber" and it is only 1000base-X there.

I was trying to work out what the motivation was for this platform.

Sorry if you mentioned it at NetdevConf and I've forgotten it all,
it was quite a while ago now!

Thanks!

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

Re: [PATCH net-next v2 7/9] net: phy: introduce ethtool_phy_ops to get and set phy configuration

[Maxime Chevallier]

On Tue, 8 Oct 2024 18:05:05 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> > What I mean is the ability for users to see, from tools like ethtool,
> > that the MCBin doubleshot's eth0 and eth1 interfaces have 2 ports
> > (copper + sfp), and potentially allow picking which one to use in case
> > both ports are connected.
> > 
> > There are mutliple devices out-there with such configurations (some
> > marvell switches for example). Do you not see some value in this ?  
> 
> Many PHYs that have two media facing ports give configuration of the
> priority between the two interfaces, and yes, there would definitely be
> value in exposing that to userspace, thereby allowing userspace to
> configure the policy there.

Great !

> This would probably be more common than the two-PHY issue that we're
> starting with - as I believe the 88e151x PHYs support exactly the same
> thing when used with a RGMII host interface. The serdes port becomes
> available for "fiber" and it is only 1000base-X there.

True, I've seen several setups with this so far indeed, as well as with
PHYs from other vendors.

> I was trying to work out what the motivation was for this platform.

It also turns out that the MCBin is one of the only boards that has a
permanent spot on my desk, as it's a pretty nice platform to experiment
with various PHY aspects.

> 
> Sorry if you mentioned it at NetdevConf and I've forgotten it all,
> it was quite a while ago now!

No worries :)

> 
> Thanks!

Thanks for your feedback on that whole topic,

Maxime 

[PATCH net-next v2 8/9] net: ethtool: phy: allow reporting and setting the phy isolate status

[Maxime Chevallier]

Add the isolate status information to the ETHTOOL_PHY_GET
netlink command attributes, and allow changing this parameter from a
newly-introduced ETHTOOL_PHY_SET command.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
V2 : Dropped loopback mode

 Documentation/networking/ethtool-netlink.rst |  1 +
 include/uapi/linux/ethtool_netlink.h         |  2 +
 net/ethtool/netlink.c                        |  8 +++
 net/ethtool/netlink.h                        |  1 +
 net/ethtool/phy.c                            | 68 ++++++++++++++++++++
 5 files changed, 80 insertions(+)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 295563e91082..800a8812a760 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -2235,6 +2235,7 @@ Kernel response contents:
                                                 bus, the name of this sfp bus
   ``ETHTOOL_A_PHY_DOWNSTREAM_SFP_NAME`` string  if the phy controls an sfp bus,
                                                 the name of the sfp bus
+  ``ETHTOOL_A_PHY_ISOLATE``             u8      The PHY Isolate status
   ===================================== ======  ===============================
 
 When ``ETHTOOL_A_PHY_UPSTREAM_TYPE`` is PHY_UPSTREAM_PHY, the PHY's parent is
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 283305f6b063..bee5bf7ef90d 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -59,6 +59,7 @@ enum {
 	ETHTOOL_MSG_MM_SET,
 	ETHTOOL_MSG_MODULE_FW_FLASH_ACT,
 	ETHTOOL_MSG_PHY_GET,
+	ETHTOOL_MSG_PHY_SET,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_USER_CNT,
@@ -1079,6 +1080,7 @@ enum {
 	ETHTOOL_A_PHY_UPSTREAM_INDEX,		/* u32 */
 	ETHTOOL_A_PHY_UPSTREAM_SFP_NAME,	/* string */
 	ETHTOOL_A_PHY_DOWNSTREAM_SFP_NAME,	/* string */
+	ETHTOOL_A_PHY_ISOLATE,			/* u8 */
 
 	/* add new constants above here */
 	__ETHTOOL_A_PHY_CNT,
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index e3f0ef6b851b..26982f47a934 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -394,6 +394,7 @@ ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
 	[ETHTOOL_MSG_PLCA_GET_STATUS]	= &ethnl_plca_status_request_ops,
 	[ETHTOOL_MSG_MM_GET]		= &ethnl_mm_request_ops,
 	[ETHTOOL_MSG_MM_SET]		= &ethnl_mm_request_ops,
+	[ETHTOOL_MSG_PHY_SET]		= &ethnl_phy_request_ops,
 };
 
 static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback *cb)
@@ -1243,6 +1244,13 @@ static const struct genl_ops ethtool_genl_ops[] = {
 		.policy = ethnl_phy_get_policy,
 		.maxattr = ARRAY_SIZE(ethnl_phy_get_policy) - 1,
 	},
+	{
+		.cmd	= ETHTOOL_MSG_PHY_SET,
+		.flags	= GENL_UNS_ADMIN_PERM,
+		.doit	= ethnl_default_set_doit,
+		.policy = ethnl_phy_set_policy,
+		.maxattr = ARRAY_SIZE(ethnl_phy_set_policy) - 1,
+	},
 };
 
 static const struct genl_multicast_group ethtool_nl_mcgrps[] = {
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 203b08eb6c6f..7ae73e2eab32 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -485,6 +485,7 @@ extern const struct nla_policy ethnl_mm_get_policy[ETHTOOL_A_MM_HEADER + 1];
 extern const struct nla_policy ethnl_mm_set_policy[ETHTOOL_A_MM_MAX + 1];
 extern const struct nla_policy ethnl_module_fw_flash_act_policy[ETHTOOL_A_MODULE_FW_FLASH_PASSWORD + 1];
 extern const struct nla_policy ethnl_phy_get_policy[ETHTOOL_A_PHY_HEADER + 1];
+extern const struct nla_policy ethnl_phy_set_policy[ETHTOOL_A_PHY_MAX + 1];
 
 int ethnl_set_features(struct sk_buff *skb, struct genl_info *info);
 int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info);
diff --git a/net/ethtool/phy.c b/net/ethtool/phy.c
index ed8f690f6bac..b211bd83a3a0 100644
--- a/net/ethtool/phy.c
+++ b/net/ethtool/phy.c
@@ -30,10 +30,13 @@ ethnl_phy_reply_size(const struct ethnl_req_info *req_base,
 	struct phy_req_info *req_info = PHY_REQINFO(req_base);
 	struct phy_device_node *pdn = req_info->pdn;
 	struct phy_device *phydev = pdn->phy;
+	const struct ethtool_phy_ops *ops;
 	size_t size = 0;
 
 	ASSERT_RTNL();
 
+	ops = ethtool_phy_ops;
+
 	/* ETHTOOL_A_PHY_INDEX */
 	size += nla_total_size(sizeof(u32));
 
@@ -66,6 +69,10 @@ ethnl_phy_reply_size(const struct ethnl_req_info *req_base,
 			size += nla_total_size(strlen(sfp_name) + 1);
 	}
 
+	/* ETHTOOL_A_PHY_ISOLATE */
+	if (ops && ops->get_config)
+		size += nla_total_size(sizeof(u8));
+
 	return size;
 }
 
@@ -75,10 +82,20 @@ ethnl_phy_fill_reply(const struct ethnl_req_info *req_base, struct sk_buff *skb)
 	struct phy_req_info *req_info = PHY_REQINFO(req_base);
 	struct phy_device_node *pdn = req_info->pdn;
 	struct phy_device *phydev = pdn->phy;
+	const struct ethtool_phy_ops *ops;
+	struct phy_device_config cfg;
 	enum phy_upstream ptype;
+	int ret;
 
 	ptype = pdn->upstream_type;
 
+	ops = ethtool_phy_ops;
+	if (ops && ops->get_config) {
+		ret = ops->get_config(phydev, &cfg);
+		if (ret)
+			return ret;
+	}
+
 	if (nla_put_u32(skb, ETHTOOL_A_PHY_INDEX, phydev->phyindex) ||
 	    nla_put_string(skb, ETHTOOL_A_PHY_NAME, dev_name(&phydev->mdio.dev)) ||
 	    nla_put_u32(skb, ETHTOOL_A_PHY_UPSTREAM_TYPE, ptype))
@@ -114,6 +131,13 @@ ethnl_phy_fill_reply(const struct ethnl_req_info *req_base, struct sk_buff *skb)
 			return -EMSGSIZE;
 	}
 
+	/* Append PHY configuration, if possible */
+	if (!ops || !ops->get_config)
+		return 0;
+
+	if (nla_put_u8(skb, ETHTOOL_A_PHY_ISOLATE, cfg.isolate))
+		return -EMSGSIZE;
+
 	return 0;
 }
 
@@ -304,3 +328,47 @@ int ethnl_phy_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 
 	return ret;
 }
+
+const struct nla_policy ethnl_phy_set_policy[] = {
+	[ETHTOOL_A_PHY_HEADER]		=
+		NLA_POLICY_NESTED(ethnl_header_policy_phy),
+	[ETHTOOL_A_PHY_ISOLATE]		= NLA_POLICY_MAX(NLA_U8, 1),
+};
+
+static int ethnl_set_phy(struct ethnl_req_info *req_info, struct genl_info *info)
+{
+	struct netlink_ext_ack *extack = info->extack;
+	const struct ethtool_phy_ops *ops;
+	struct nlattr **tb = info->attrs;
+	struct phy_device_config cfg;
+	struct phy_device *phydev;
+	bool mod = false;
+	int ret;
+
+	ops = ethtool_phy_ops;
+	if (!ops || !ops->set_config || !ops->get_config)
+		return -EOPNOTSUPP;
+
+	/* We're running under rtnl */
+	phydev = ethnl_req_get_phydev(req_info, tb[ETHTOOL_A_PHY_HEADER],
+				      extack);
+	if (IS_ERR_OR_NULL(phydev))
+		return -ENODEV;
+
+	ret = ops->get_config(phydev, &cfg);
+	if (ret)
+		return ret;
+
+	ethnl_update_bool(&cfg.isolate, tb[ETHTOOL_A_PHY_ISOLATE], &mod);
+
+	if (!mod)
+		return 0;
+
+	/* Returning 0 is fine as we don't have a notification */
+	return ops->set_config(phydev, &cfg, extack);
+}
+
+const struct ethnl_request_ops ethnl_phy_request_ops = {
+	.hdr_attr		= ETHTOOL_A_PHY_HEADER,
+	.set			= ethnl_set_phy,
+};
-- 
2.46.1

[PATCH net-next v2 9/9] netlink: specs: introduce phy-set command along with configurable attributes

[Maxime Chevallier]

Update the ethnl specification to include the newly introduced isolated
attribute, and describe the newly introduced ETHTOOL_PHY_SET command.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
V2 : Dropped loopback mode

 Documentation/netlink/specs/ethtool.yaml | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index 6a050d755b9c..6f5cdb3af64d 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -1132,6 +1132,9 @@ attribute-sets:
       -
         name: downstream-sfp-name
         type: string
+      -
+        name: isolate
+        type: u8
 
 operations:
   enum-model: directional
@@ -1950,4 +1953,16 @@ operations:
             - upstream-index
             - upstream-sfp-name
             - downstream-sfp-name
+            - isolate
       dump: *phy-get-op
+    -
+      name: phy-set
+      doc: Set configuration attributes for attached PHY devices
+
+      attribute-set: phy
+
+      do:
+        request:
+          attributes:
+            - header
+            - isolate
-- 
2.46.1

Re: [PATCH net-next v2 0/9] Allow isolating PHY devices

[Russell King (Oracle)]

I'm going to ask a very basic question concerning this.

Isolation was present in PHYs early on when speeds were low, and thus
electrical reflections weren't too much of a problem, and thus star
topologies didn't have too much of an effect. A star topology is
multi-drop. Even if the PCB tracks go from MAC to PHY1 and then onto
PHY2, if PHY2 is isolated, there are two paths that the signal will
take, one to MAC and the other to PHY2. If there's no impediance match
at PHY2 (e.g. because it's in high-impedance mode) then that
transmission line is unterminated, and thus will reflect back towards
the MAC.

As speeds get faster, then reflections from unterminated ends become
more of an issue.

I suspect the reason why e.g. 88x3310, 88E1111 etc do not support
isolate mode is because of this - especially when being used in
serdes mode, the topology is essentially point-to-point and any
side branches can end up causing data corruption.

So my questions would be, is adding support for isolation mode in
PHYs given todays network speeds something that is realistic, and
do we have actual hardware out there where there is more than one
PHY in the bus. If there is, it may be useful to include details
of that (such as PHY interface type) in the patch series description.

On Fri, Oct 04, 2024 at 06:15:50PM +0200, Maxime Chevallier wrote:
> Hello,
> 
> This is the V2 of a series to add isolation support for PHY devices.
> 
> As a remainder, this mode allows a PHY to set its MII lines in
> high-impedance mode to avoid interferences on this bus.
> 
> So far, I've identified that :
> 
>  - Marvell 88e1512 isolation works fine
>  - LXT973 claims to support isolation, but it's actually broken
>  - Marvell 88x3310 doesn't support isolation, by design
>  - Marvell 88e1111 claims to support isolation in GMII, RGMII, TBI
>    (untested) but doesn't in SGMII (tested).
> 
> Changes in V2 :
> 
>  - Removed the loopback mode that was included in the first iteration
>  - Added phy_shutdown, to make sure we de-isolate the PHY when rebooting
>  - Changes the "PHY_NO_ISOLATE" flag to a phy driver ops. Testing showed
>    that some PHYs may or may not support isolation based on the
>    interface that's being used.
>  - Added isolation support reporting for the Marvell 88e1111 PHY.
> 
> V1 : https://lore.kernel.org/netdev/20240911212713.2178943-1-maxime.chevallier@bootlin.com/
> 
> Maxime Chevallier (9):
>   net: phy: allow isolating PHY devices
>   net: phy: Introduce phy_shutdown for device quiescence.
>   net: phy: Allow PHY drivers to report isolation support
>   net: phy: lxt: Mark LXT973 PHYs as having a broken isolate mode
>   net: phy: marvell10g: 88x3310 and 88x3340 don't support isolate mode
>   net: phy: marvell: mv88e1111 doesn't support isolate in SGMII mode
>   net: phy: introduce ethtool_phy_ops to get and set phy configuration
>   net: ethtool: phy: allow reporting and setting the phy isolate status
>   netlink: specs: introduce phy-set command along with configurable
>     attributes
> 
>  Documentation/netlink/specs/ethtool.yaml     |  15 +++
>  Documentation/networking/ethtool-netlink.rst |   1 +
>  drivers/net/phy/lxt.c                        |   2 +
>  drivers/net/phy/marvell.c                    |   9 ++
>  drivers/net/phy/marvell10g.c                 |   2 +
>  drivers/net/phy/phy.c                        |  44 ++++++++
>  drivers/net/phy/phy_device.c                 | 101 +++++++++++++++++--
>  include/linux/ethtool.h                      |   8 ++
>  include/linux/phy.h                          |  42 ++++++++
>  include/uapi/linux/ethtool_netlink.h         |   2 +
>  net/ethtool/netlink.c                        |   8 ++
>  net/ethtool/netlink.h                        |   1 +
>  net/ethtool/phy.c                            |  68 +++++++++++++
>  13 files changed, 297 insertions(+), 6 deletions(-)
> 
> -- 
> 2.46.1
> 
> 

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

Re: [PATCH net-next v2 0/9] Allow isolating PHY devices

[Maxime Chevallier]

Hello Russell

On Fri, 4 Oct 2024 18:02:25 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> I'm going to ask a very basic question concerning this.
> 
> Isolation was present in PHYs early on when speeds were low, and thus
> electrical reflections weren't too much of a problem, and thus star
> topologies didn't have too much of an effect. A star topology is
> multi-drop. Even if the PCB tracks go from MAC to PHY1 and then onto
> PHY2, if PHY2 is isolated, there are two paths that the signal will
> take, one to MAC and the other to PHY2. If there's no impediance match
> at PHY2 (e.g. because it's in high-impedance mode) then that
> transmission line is unterminated, and thus will reflect back towards
> the MAC.
> 
> As speeds get faster, then reflections from unterminated ends become
> more of an issue.
> 
> I suspect the reason why e.g. 88x3310, 88E1111 etc do not support
> isolate mode is because of this - especially when being used in
> serdes mode, the topology is essentially point-to-point and any
> side branches can end up causing data corruption.

I suspect indeed that this won't work on serdes interfaces. I didn't
find any reliable information on that, but so far the few PHYs I've
seen seem to work that way.

The 88e1512 supports that, but I was testing in RGMII.

> 
> So my questions would be, is adding support for isolation mode in
> PHYs given todays network speeds something that is realistic, and
> do we have actual hardware out there where there is more than one
> PHY in the bus. If there is, it may be useful to include details
> of that (such as PHY interface type) in the patch series description.

I do have some hardware with this configuration (I'd like to support
that upstream, the topology work was preliminary work for that, and the
next move would be to send an RFC for these topolopgies exactly).

I am working with 3 different HW platforms with this layout :

      /--- PHY
      |
MAC  -|  phy_interface_mode == MII so, 100Mbps Max.
      |
      \--- PHY

and another that is similar but with RMII. I finally have one last case
with MII interface, same layout, but the PHYs can't isolate so we need
to make sure all but one PHYs are powered-down at any given time.

I will include that in the cover.

Could we consider limiting the isolation to non-serdes interfaces ?
that would be :

 - MII
 - RMII
 - GMII
 - RGMII and its -[TX|RX] ID flavours
 - TBI and RTBI ?? (I'm not sure about these)

Trying to isolate a PHY that doesn't have any of the interfaces above
would result in -EOPNOTSUPP ?

Thanks,

Maxime

Re: [PATCH net-next v2 0/9] Allow isolating PHY devices

[Russell King (Oracle)]

On Mon, Oct 07, 2024 at 12:25:13PM +0200, Maxime Chevallier wrote:
> Hello Russell
> 
> On Fri, 4 Oct 2024 18:02:25 +0100
> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> 
> > I'm going to ask a very basic question concerning this.
> > 
> > Isolation was present in PHYs early on when speeds were low, and thus
> > electrical reflections weren't too much of a problem, and thus star
> > topologies didn't have too much of an effect. A star topology is
> > multi-drop. Even if the PCB tracks go from MAC to PHY1 and then onto
> > PHY2, if PHY2 is isolated, there are two paths that the signal will
> > take, one to MAC and the other to PHY2. If there's no impediance match
> > at PHY2 (e.g. because it's in high-impedance mode) then that
> > transmission line is unterminated, and thus will reflect back towards
> > the MAC.
> > 
> > As speeds get faster, then reflections from unterminated ends become
> > more of an issue.
> > 
> > I suspect the reason why e.g. 88x3310, 88E1111 etc do not support
> > isolate mode is because of this - especially when being used in
> > serdes mode, the topology is essentially point-to-point and any
> > side branches can end up causing data corruption.
> 
> I suspect indeed that this won't work on serdes interfaces. I didn't
> find any reliable information on that, but so far the few PHYs I've
> seen seem to work that way.
> 
> The 88e1512 supports that, but I was testing in RGMII.

Looking at 802.3, there is no support for isolation in the clause 45
register set - the isolate bit only appears in the clause 22 BMCR.
Clause 22 registers are optional for clause 45 PHYs.

My reading of this is that 802.3 has phased out isolation support on
the MII side of the PHY on more modern PHYs, so this seems to be a
legacy feature.

Andrew has already suggested that we should default to isolate not
being supported - given that it's legacy, I agree with that.

> > So my questions would be, is adding support for isolation mode in
> > PHYs given todays network speeds something that is realistic, and
> > do we have actual hardware out there where there is more than one
> > PHY in the bus. If there is, it may be useful to include details
> > of that (such as PHY interface type) in the patch series description.
> 
> I do have some hardware with this configuration (I'd like to support
> that upstream, the topology work was preliminary work for that, and the
> next move would be to send an RFC for these topolopgies exactly).
> 
> I am working with 3 different HW platforms with this layout :
> 
>       /--- PHY
>       |
> MAC  -|  phy_interface_mode == MII so, 100Mbps Max.
>       |
>       \--- PHY
> 
> and another that is similar but with RMII. I finally have one last case
> with MII interface, same layout, but the PHYs can't isolate so we need
> to make sure all but one PHYs are powered-down at any given time.

You have given further details in other response to Andrew. I'll
comment further there.

> I will include that in the cover.

Yes, it would be good to include all these details in the cover message
so that it isn't spread out over numerous replies.

> Could we consider limiting the isolation to non-serdes interfaces ?
> that would be :
> 
>  - MII
>  - RMII
>  - GMII
>  - RGMII and its -[TX|RX] ID flavours
>  - TBI and RTBI ?? (I'm not sure about these)
> 
> Trying to isolate a PHY that doesn't have any of the interfaces above
> would result in -EOPNOTSUPP ?

I think the question should be: which MII interfaces can electrically
support multi-drop setups.

22.2.4.1.6 describes the Clause 22 Isolate bit, which only suggests
at one use case - for a PHY connected via an 802.3 defined connector
which shall power up in isolated state "to avoid the possibility of
having multiple MII output drivers actively driving the same signal
path simultaneously". This connector only supports four data signals
in each direction, which precludes GMII over this defined connector.

However, it talks about isolating the MII and GMII signals in this
section.

Putting that all together, 802.3 suggests that it is possible to
have multiple PHYs on a MII or GMII (which in an explanatory note
elsewhere, MII means 100Mb/s, GMII for 1Gb/s.) However, it is
vague.

Now... I want to say more, but this thread is fragmented and the
next bit of the reply needs to go elsewhere in this thread,
which is going to make reviewing this discussion later on rather
difficult... but we're being drip-fed the technical details.


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

Re: [PATCH net-next v2 0/9] Allow isolating PHY devices

[Andrew Lunn]

> Looking at 802.3, there is no support for isolation in the clause 45
> register set - the isolate bit only appears in the clause 22 BMCR.
> Clause 22 registers are optional for clause 45 PHYs.

That was also an observation i had, the code goes straight to C22
operations, without even considering if the PHY has phydev->is_c45 is
set.  I think we need an op for this, which will default to NULL. The
driver can then opt-in by using the genphy function, or its own driver
method.

But lets get the big picture understood first, before we focus on the
details.

	Andrew

Re: [PATCH net-next v2 0/9] Allow isolating PHY devices

[Maxime Chevallier]

Hi Russell,

On Mon, 7 Oct 2024 16:53:32 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Mon, Oct 07, 2024 at 12:25:13PM +0200, Maxime Chevallier wrote:
> > Hello Russell
> > 
> > On Fri, 4 Oct 2024 18:02:25 +0100
> > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> >   
> > > I'm going to ask a very basic question concerning this.
> > > 
> > > Isolation was present in PHYs early on when speeds were low, and thus
> > > electrical reflections weren't too much of a problem, and thus star
> > > topologies didn't have too much of an effect. A star topology is
> > > multi-drop. Even if the PCB tracks go from MAC to PHY1 and then onto
> > > PHY2, if PHY2 is isolated, there are two paths that the signal will
> > > take, one to MAC and the other to PHY2. If there's no impediance match
> > > at PHY2 (e.g. because it's in high-impedance mode) then that
> > > transmission line is unterminated, and thus will reflect back towards
> > > the MAC.
> > > 
> > > As speeds get faster, then reflections from unterminated ends become
> > > more of an issue.
> > > 
> > > I suspect the reason why e.g. 88x3310, 88E1111 etc do not support
> > > isolate mode is because of this - especially when being used in
> > > serdes mode, the topology is essentially point-to-point and any
> > > side branches can end up causing data corruption.  
> > 
> > I suspect indeed that this won't work on serdes interfaces. I didn't
> > find any reliable information on that, but so far the few PHYs I've
> > seen seem to work that way.
> > 
> > The 88e1512 supports that, but I was testing in RGMII.  
> 
> Looking at 802.3, there is no support for isolation in the clause 45
> register set - the isolate bit only appears in the clause 22 BMCR.
> Clause 22 registers are optional for clause 45 PHYs.
> 
> My reading of this is that 802.3 has phased out isolation support on
> the MII side of the PHY on more modern PHYs, so this seems to be a
> legacy feature.
> 
> Andrew has already suggested that we should default to isolate not
> being supported - given that it's legacy, I agree with that.
> 
> > > So my questions would be, is adding support for isolation mode in
> > > PHYs given todays network speeds something that is realistic, and
> > > do we have actual hardware out there where there is more than one
> > > PHY in the bus. If there is, it may be useful to include details
> > > of that (such as PHY interface type) in the patch series description.  
> > 
> > I do have some hardware with this configuration (I'd like to support
> > that upstream, the topology work was preliminary work for that, and the
> > next move would be to send an RFC for these topolopgies exactly).
> > 
> > I am working with 3 different HW platforms with this layout :
> > 
> >       /--- PHY
> >       |
> > MAC  -|  phy_interface_mode == MII so, 100Mbps Max.
> >       |
> >       \--- PHY
> > 
> > and another that is similar but with RMII. I finally have one last case
> > with MII interface, same layout, but the PHYs can't isolate so we need
> > to make sure all but one PHYs are powered-down at any given time.  
> 
> You have given further details in other response to Andrew. I'll
> comment further there.
> 
> > I will include that in the cover.  
> 
> Yes, it would be good to include all these details in the cover message
> so that it isn't spread out over numerous replies.
> 
> > Could we consider limiting the isolation to non-serdes interfaces ?
> > that would be :
> > 
> >  - MII
> >  - RMII
> >  - GMII
> >  - RGMII and its -[TX|RX] ID flavours
> >  - TBI and RTBI ?? (I'm not sure about these)
> > 
> > Trying to isolate a PHY that doesn't have any of the interfaces above
> > would result in -EOPNOTSUPP ?  
> 
> I think the question should be: which MII interfaces can electrically
> support multi-drop setups.
> 
> 22.2.4.1.6 describes the Clause 22 Isolate bit, which only suggests
> at one use case - for a PHY connected via an 802.3 defined connector
> which shall power up in isolated state "to avoid the possibility of
> having multiple MII output drivers actively driving the same signal
> path simultaneously". This connector only supports four data signals
> in each direction, which precludes GMII over this defined connector.
> 
> However, it talks about isolating the MII and GMII signals in this
> section.
> 
> Putting that all together, 802.3 suggests that it is possible to
> have multiple PHYs on a MII or GMII (which in an explanatory note
> elsewhere, MII means 100Mb/s, GMII for 1Gb/s.) However, it is
> vague.

Yes it's vague, as as testing showed, vendors are pretty liberal with
how/if they implement this feature :(

> Now... I want to say more, but this thread is fragmented and the
> next bit of the reply needs to go elsewhere in this thread,
> which is going to make reviewing this discussion later on rather
> difficult... but we're being drip-fed the technical details.

TBH I wasn't expecting this series on isolation to be the place to
discuss the multiplexing use-cases, hence why I didn't include a full
descriptin of every setup I have in the cover.

Given what Andrew replied, this whole series on controling isolation
from userspace isn't relevant.

Let me start a proper discussion thread and summarize what has been
said so far.

Maxime