diff mbox series

[net-next,4/6] net: phy: Add support for inband extensions

Message ID 20220519135647.465653-5-maxime.chevallier@bootlin.com (mailing list archive)
State New, archived
Headers show
Series net: Introduce Ethernet Inband Extensions | expand

Commit Message

Maxime Chevallier May 19, 2022, 1:56 p.m. UTC
The USXGMII Standard by Cisco introduces the notion of extensions used
in the preamble. The standard proposes a "PCH" extension, which allows
passing timestamps in the preamble. However, other alternatives are
possible, like Microchip's "MCH" mode, that allows passing indication to
a PHY telling whether or not the PHY should timestamp an outgoing packet,
therefore removing the need for the PHY to have an internal classifier.

This commit allows reporting the various extensions a PHY supports,
without tying them to the actual PHY mode. This is done 1) because there
are multiple variants of the USXGMII mode, like QUSGMII and OUSGMII, and
2) because other non-cisco standards might one day propose a similar
mechanism.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy.c | 68 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/phy.h   | 25 +++++++++++++++-
 2 files changed, 92 insertions(+), 1 deletion(-)

Comments

Andrew Lunn May 19, 2022, 2:10 p.m. UTC | #1
> +static int phy_set_inband_ext(struct phy_device *phydev, u32 mask, u32 ext)
> +{
> +	int ret;
> +
> +	if (!phy_interface_has_inband_ext(phydev->interface))
> +		return -EOPNOTSUPP;
> +
> +	if (!phydev->drv->inband_ext_config)
> +		return -EOPNOTSUPP;
> +
> +	ret = phydev->drv->inband_ext_config(phydev, mask, ext);
> +	if (ret)
> +		return ret;
> +
> +	phydev->inband_ext.enabled &= ~mask;
> +	phydev->inband_ext.enabled |= (mask & ext);

You appear to be missing locking in this patchset.

> +int phy_inband_ext_enable(struct phy_device *phydev, u32 ext)
> +{
> +	return phy_set_inband_ext(phydev, ext, ext);

There should be an -EOPNOTSUPP here is requested to enable an
extension which is not available.

> +}
> +EXPORT_SYMBOL(phy_inband_ext_enable);
> +
> +int phy_inband_ext_disable(struct phy_device *phydev, u32 ext)
> +{
> +	return phy_set_inband_ext(phydev, ext, 0);

And the same here.

> +}
> +EXPORT_SYMBOL(phy_inband_ext_disable);
> +
> +int phy_inband_ext_set_available(struct phy_device *phydev, u32 mask, u32 ext)
> +{
> +	if (!(mask & phydev->drv->inband_ext))
> +		return -EOPNOTSUPP;
> +
> +	phydev->inband_ext.available &= ~mask;
> +	phydev->inband_ext.available |= (mask & ext);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(phy_inband_ext_set_available);
> +
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 4a2731c78590..6b08f49bce5b 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -190,6 +190,21 @@ static inline void phy_interface_set_rgmii(unsigned long *intf)
>  	__set_bit(PHY_INTERFACE_MODE_RGMII_TXID, intf);
>  }
>  
> +/*
> + * TODO : Doc
> + */
> +enum {
> +	__PHY_INBAND_EXT_PCH = 0,
> +};
> +
> +#define PHY_INBAND_EXT_PCH	BIT(__PHY_INBAND_EXT_PCH)

the documentation is important here, since it makes it clear if these
are values directly taken from the specification, or if these are
linux specific, and the driver needs to map from linux to whatever the
spec calls them.

     Andrew
Andrew Lunn May 19, 2022, 2:28 p.m. UTC | #2
> +static int phy_set_inband_ext(struct phy_device *phydev, u32 mask, u32 ext)
> +{

> +/*
> + * TODO : Doc
> + */
> +enum {
> +	__PHY_INBAND_EXT_PCH = 0,
> +};

I'm not so happy with this API passing masks and values, when you are
actually dealing with a feature which is a boolean, exists, does not
exist.

> +int phy_inband_ext_enable(struct phy_device *phydev, u32 ext);
> +int phy_inband_ext_disable(struct phy_device *phydev, u32 ext);

I would prefer enum phy_inband_ext ext;

phy_inband_ext_set_available(struct phy_device *phydev, enum phy_inband_ext ext);

and add

phy_inband_ext_set_unavailable(struct phy_device *phydev, enum phy_inband_ext ext);

Internally you can then turn these into operations on a u32.

	   Andrew
diff mbox series

Patch

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index beb2b66da132..bbd3d7620609 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1475,3 +1475,71 @@  int phy_ethtool_nway_reset(struct net_device *ndev)
 	return phy_restart_aneg(phydev);
 }
 EXPORT_SYMBOL(phy_ethtool_nway_reset);
+
+/**
+ * PHY modes in the USXGMII family can have extensions, with data transmitted
+ * in the frame preamble.
+ * For now, only QUSGMII is supported, but other variants like USGMII and
+ * OUSGMII can be added in the future.
+ */
+static inline bool phy_interface_has_inband_ext(phy_interface_t interface)
+{
+	return interface == PHY_INTERFACE_MODE_QUSGMII;
+}
+
+bool phy_inband_ext_available(struct phy_device *phydev, u32 ext)
+{
+	return !!(phydev->inband_ext.available & ext);
+}
+EXPORT_SYMBOL(phy_inband_ext_available);
+
+bool phy_inband_ext_enabled(struct phy_device *phydev, u32 ext)
+{
+	return !!(phydev->inband_ext.enabled & ext);
+}
+EXPORT_SYMBOL(phy_inband_ext_enabled);
+
+static int phy_set_inband_ext(struct phy_device *phydev, u32 mask, u32 ext)
+{
+	int ret;
+
+	if (!phy_interface_has_inband_ext(phydev->interface))
+		return -EOPNOTSUPP;
+
+	if (!phydev->drv->inband_ext_config)
+		return -EOPNOTSUPP;
+
+	ret = phydev->drv->inband_ext_config(phydev, mask, ext);
+	if (ret)
+		return ret;
+
+	phydev->inband_ext.enabled &= ~mask;
+	phydev->inband_ext.enabled |= (mask & ext);
+
+	return 0;
+}
+
+int phy_inband_ext_enable(struct phy_device *phydev, u32 ext)
+{
+	return phy_set_inband_ext(phydev, ext, ext);
+}
+EXPORT_SYMBOL(phy_inband_ext_enable);
+
+int phy_inband_ext_disable(struct phy_device *phydev, u32 ext)
+{
+	return phy_set_inband_ext(phydev, ext, 0);
+}
+EXPORT_SYMBOL(phy_inband_ext_disable);
+
+int phy_inband_ext_set_available(struct phy_device *phydev, u32 mask, u32 ext)
+{
+	if (!(mask & phydev->drv->inband_ext))
+		return -EOPNOTSUPP;
+
+	phydev->inband_ext.available &= ~mask;
+	phydev->inband_ext.available |= (mask & ext);
+
+	return 0;
+}
+EXPORT_SYMBOL(phy_inband_ext_set_available);
+
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 4a2731c78590..6b08f49bce5b 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -190,6 +190,21 @@  static inline void phy_interface_set_rgmii(unsigned long *intf)
 	__set_bit(PHY_INTERFACE_MODE_RGMII_TXID, intf);
 }
 
+/*
+ * TODO : Doc
+ */
+enum {
+	__PHY_INBAND_EXT_PCH = 0,
+};
+
+#define PHY_INBAND_EXT_PCH	BIT(__PHY_INBAND_EXT_PCH)
+
+int phy_inband_ext_enable(struct phy_device *phydev, u32 ext);
+int phy_inband_ext_disable(struct phy_device *phydev, u32 ext);
+int phy_inband_ext_set_available(struct phy_device *phydev, u32 mask, u32 ext);
+bool phy_inband_ext_available(struct phy_device *phydev, u32 ext);
+bool phy_inband_ext_enabled(struct phy_device *phydev, u32 ext);
+
 /*
  * phy_supported_speeds - return all speeds currently supported by a PHY device
  */
@@ -275,7 +290,6 @@  static inline const char *phy_modes(phy_interface_t interface)
 	}
 }
 
-
 #define PHY_INIT_TIMEOUT	100000
 #define PHY_FORCE_TIMEOUT	10
 
@@ -635,6 +649,11 @@  struct phy_device {
 
 	phy_interface_t interface;
 
+	struct {
+		u32 available;
+		u32 enabled;
+	} inband_ext;
+
 	/*
 	 * forced speed & duplex (no autoneg)
 	 * partner speed & duplex & pause (autoneg)
@@ -766,6 +785,7 @@  struct phy_driver {
 	u32 phy_id_mask;
 	const unsigned long * const features;
 	u32 flags;
+	u32 inband_ext;
 	const void *driver_data;
 
 	/**
@@ -935,6 +955,9 @@  struct phy_driver {
 	int (*get_sqi)(struct phy_device *dev);
 	/** @get_sqi_max: Get the maximum signal quality indication */
 	int (*get_sqi_max)(struct phy_device *dev);
+
+	int (*inband_ext_config)(struct phy_device *dev, u32 features,
+				 u32 mask);
 };
 #define to_phy_driver(d) container_of(to_mdio_common_driver(d),		\
 				      struct phy_driver, mdiodrv)