diff mbox series

[net-next,1/7] net: phy: allow isolating PHY devices

Message ID 20240911212713.2178943-2-maxime.chevallier@bootlin.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series Allow controlling PHY loopback and isolate modes | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; GEN HAS DIFF 2 files changed, 121 insertions(+);
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 16 this patch: 16
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 29 this patch: 29
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 401 this patch: 401
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 163 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 70 this patch: 70
netdev/source_inline success Was 0 now: 0

Commit Message

Maxime Chevallier Sept. 11, 2024, 9:27 p.m. UTC 
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>
---
 drivers/net/phy/phy_device.c | 76 +++++++++++++++++++++++++++++++++---
 include/linux/phy.h          |  4 ++
 2 files changed, 74 insertions(+), 6 deletions(-)

Comments

Florian Fainelli Sept. 12, 2024, 6:30 p.m. UTC | #1 
On 9/11/24 14:27, Maxime Chevallier wrote:
> 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>

Not sure where that comment belongs so I will put it here, one thing 
that concerns me is if you have hardware that is not strapped to be 
isolated by default, and the PHY retains the state configured by Linux, 
such that the PHY is in isolation mode. A boot loader that is not 
properly taking the PHY out of isolation mode would be unavailable to 
use it and that would be a bug that Linux would likely be on the hook to 
fix.

Would recommend adding a phy_shutdown() method which is called upon 
reboot/kexec and which, based upon a quirk/flag can ensure that the 
isolation bit is cleared.
Maxime Chevallier Sept. 13, 2024, 7:43 a.m. UTC | #2 
Hello Florian,

On Thu, 12 Sep 2024 11:30:26 -0700
Florian Fainelli <f.fainelli@gmail.com> wrote:

> On 9/11/24 14:27, Maxime Chevallier wrote:
> > 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>  
> 
> Not sure where that comment belongs so I will put it here, one thing 
> that concerns me is if you have hardware that is not strapped to be 
> isolated by default, and the PHY retains the state configured by Linux, 
> such that the PHY is in isolation mode. A boot loader that is not 
> properly taking the PHY out of isolation mode would be unavailable to 
> use it and that would be a bug that Linux would likely be on the hook to 
> fix.
> 
> Would recommend adding a phy_shutdown() method which is called upon 
> reboot/kexec and which, based upon a quirk/flag can ensure that the 
> isolation bit is cleared.

Very good point. I can see the same problem occuring with loopback then
(if we use the 802.3 C22 PHY loopback bit).

I have such a patch ready actually, for the 2-PHYs-on-the-same-MAC
scenario, I will include it in the next iteration.

Thanks for your feedback,

Maxime
diff mbox series

Patch

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