| Message ID | 20241004161601.2932901-4-maxime.chevallier@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Allow isolating PHY devices | expand |
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
> +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
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
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
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,
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(+)