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

So far, these parameters include the loopback state of the PHY
(host-side loopback) and 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>
---
 drivers/net/phy/phy.c        | 59 ++++++++++++++++++++++++++++++++++++
 drivers/net/phy/phy_device.c |  2 ++
 include/linux/ethtool.h      |  8 +++++
 include/linux/phy.h          | 21 +++++++++++++
 4 files changed, 90 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 4f3e742907cb..0cdb0fc30727 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1811,3 +1811,62 @@ 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;
+	phy_cfg->loopback = phydev->loopback_enabled;
+	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 loopback_change, isolate_change;
+	int ret;
+
+	/* As the phydev's loopback and isolation state are protected by the
+	 * phy lock, check first if we'll need to perform the action,
+	 * then do them as a second step.
+	 */
+	mutex_lock(&phydev->lock);
+	isolate_change = (phy_cfg->isolate != phydev->isolated);
+	loopback_change = (phy_cfg->loopback != phydev->loopback_enabled);
+	mutex_unlock(&phydev->lock);
+
+	if (isolate_change) {
+		ret = phy_isolate(phydev, phy_cfg->isolate);
+		if (ret) {
+			NL_SET_ERR_MSG(extack, "Error while configuring PHY isolation");
+			return ret;
+		}
+	}
+
+	if (loopback_change) {
+		ret = phy_loopback(phydev, phy_cfg->loopback);
+		if (ret) {
+			NL_SET_ERR_MSG(extack, "Error while configuring PHY loopback");
+			return ret;
+		}
+	}
+
+	return 0;
+}
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 2a3db1043626..0714a2b83d18 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3845,6 +3845,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 f0a8a5459fbe..ee0364d2afc3 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -887,6 +887,22 @@ 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
+ * @loopback: The loopback state of the PHY
+ */
+struct phy_device_config {
+	bool isolate;
+	bool loopback;
+};
+
 /**
  * struct phy_led: An LED driven by the PHY
  *
@@ -2067,6 +2083,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.0

Hi Maxime,

On Wed, Sep 11, 2024 at 11:27:09PM +0200, Maxime Chevallier wrote:
...
>  
> +/**
> + * 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
> + * @loopback: The loopback state of the PHY
> + */
> +struct phy_device_config {
> +	bool isolate;
> +	bool loopback;
> +};
 
I would recommend to have loopback enum. There are different levels of
loopback:
https://www.ti.com/document-viewer/DP83TD510E/datasheet#GUID-50834313-DEF1-42FB-BA00-9B0902B2D7E4/TITLE-SNLS656SNLS5055224

I imagine something like this:

/*
 * enum phy_loopback_mode - PHY loopback modes
 * These modes represent different loopback configurations to
 * facilitate in-circuit testing of the PHY's digital and analog paths.
 */
enum phy_loopback_mode {
	PHY_LOOPBACK_NONE = 0,		/* No loopback mode enabled */
	PHY_LOOPBACK_MII,		/* MII Loopback: MAC to PHY internal loopback */
	PHY_LOOPBACK_PCS,		/* PCS Loopback: PCS layer loopback, no signal processing */
	PHY_LOOPBACK_DIGITAL,		/* Digital Loopback: Loops back entire digital TX/RX path */
	PHY_LOOPBACK_ANALOG,		/* Analog Loopback: Loops back after analog front-end */
	PHY_LOOPBACK_FAR_END		/* Far-End (Reverse) Loopback: Receiver to MAC interface loopback */
};

At same time, one interface will have multiple loopback providers, except of
multiple PHYs, MAC will provide it too.

I assume, we need a bit field per component to reflect supported loopback modes.

If you have time, please take a look at net/core/selftests.c this will be
one of consumers which should walk over different loopback levels to find the
location of potential problem.

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 |

Hello Oleksij,

On Thu, 12 Sep 2024 06:46:29 +0200
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> Hi Maxime,
> 
> On Wed, Sep 11, 2024 at 11:27:09PM +0200, Maxime Chevallier wrote:
> ...
> >  
> > +/**
> > + * 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
> > + * @loopback: The loopback state of the PHY
> > + */
> > +struct phy_device_config {
> > +	bool isolate;
> > +	bool loopback;
> > +};  
>  
> I would recommend to have loopback enum. There are different levels of
> loopback:
> https://www.ti.com/document-viewer/DP83TD510E/datasheet#GUID-50834313-DEF1-42FB-BA00-9B0902B2D7E4/TITLE-SNLS656SNLS5055224
> 
> I imagine something like this:
> 
> /*
>  * enum phy_loopback_mode - PHY loopback modes
>  * These modes represent different loopback configurations to
>  * facilitate in-circuit testing of the PHY's digital and analog paths.
>  */
> enum phy_loopback_mode {
> 	PHY_LOOPBACK_NONE = 0,		/* No loopback mode enabled */
> 	PHY_LOOPBACK_MII,		/* MII Loopback: MAC to PHY internal loopback */
> 	PHY_LOOPBACK_PCS,		/* PCS Loopback: PCS layer loopback, no signal processing */
> 	PHY_LOOPBACK_DIGITAL,		/* Digital Loopback: Loops back entire digital TX/RX path */
> 	PHY_LOOPBACK_ANALOG,		/* Analog Loopback: Loops back after analog front-end */
> 	PHY_LOOPBACK_FAR_END		/* Far-End (Reverse) Loopback: Receiver to MAC interface loopback */
> };

I agree with you on that, having the ability to fine-tune where the
loopback happens is really useful for debug. The main problem I would
see is to come-up with a set of modes that are somewhat generic, as
vendors implement a wide variety of loopback modes.

For example, on BaseT4 PHYs the Analog loopback doesn't exist and is
more akin to using a loopback stub, whereas the Digital loopback would
be a loopback at the PMD level (I don't know if that even exists).

That being said, the list of possible places to setup the loopback
within a PHY is finite and it's conceivable to come-up with a set of
loopback modes.

Thanks a lot for the feedback,

Maxime