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