| Message ID | 20240212173307.1124120-2-maxime.chevallier@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introduce support for USGMII Inband Extensions | expand |
On Mon, 12 Feb 2024 18:33:04 +0100 Maxime Chevallier wrote: > +/** > + * 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) Spurious use of /**, it seems. You either need to use full kdoc format, or the normal /* comment start.
Hello Jakub, On Mon, 12 Feb 2024 17:25:11 -0800 Jakub Kicinski <kuba@kernel.org> wrote: > On Mon, 12 Feb 2024 18:33:04 +0100 Maxime Chevallier wrote: > > +/** > > + * 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) > > > Spurious use of /**, it seems. You either need to use full kdoc format, > or the normal /* comment start. Ah sorry about that, I'll address it. Thanks for taking a look ! Maxime
> +Inband Extensions > +================= > + > +The USGMII Standard allows the possibility to re-use the full-length 7-bytes > +frame preamble to convey meaningful data. This is already partly used by modes > +like QSGMII, which passes the port number in the preamble. > + > +In USGMII, we have a standardized approach to allow the MAC and PHY to pass > +such data in the preamble, which looks like this : > + > +| 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | Frame data > +| SoP | | Extension | CRC | > +| / \_______________ | | > +| / \ | | > +| | type | subport | ext type | | | > + > +The preamble in that case uses the Packet Control Header (PCH) format, where > +the byte 1 is used as a control field with : > + > +type - 2 bits : > + - 00 : Packet with PCH > + - 01 : Packet without PCH > + - 10 : Idle Packet, without data > + - 11 : Reserved > + > +subport - 4 bits : The subport identifier. For QUSGMII, this field ranges from > + 0 to 3, and for OUSGMII, it ranges from 0 to 7. > + > +ext type - 2 bits : Indicated the type of data conveyed in the extension > + - 00 : Ignore extension > + - 01 : 8 bits reserved + 32 timestamp > + - 10 : Reserved > + - 11 : Reserved Somewhat crystal ball... Those two reserved values could be used in the future to indicate other extensions. So we could have three in operation at once, but only one selected per frame. > +A PHY driver can register available modes with:: > + > + int phy_inband_ext_set_available(struct phy_device *phydev, enum phy_inband_ext ext); > + int phy_inband_ext_set_unavailable(struct phy_device *phydev, enum phy_inband_ext ext); enum phy_inband_ext is just an well defined, but arbitrary number? 0 is this time stamp value mode, 1 could be used MACSEC, 2 could be a QoS indicator when doing rate adaptation? 3 could be .... > +It's then up to the MAC driver to enable/disable the extension in the PHY as > +needed. This was designed to fit the timestamping configuration model, as it > +is the only mode supported so far. > + > +Enabling/Disabling an extension is done from the MAC driver through:: > + > + int phy_inband_ext_enable(struct phy_device *phydev, enum phy_inband_ext ext); So maybe this should return the 2 bit ext type value? The MAC can request QoS marking, and the PHY replies it expects the bits to be 3 ? I'm just trying to ensure we have an API which is extensible in the future to make use of those two reserved values. > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > index 3b9531143be1..4b6cf94f51d5 100644 > --- a/drivers/net/phy/phy.c > +++ b/drivers/net/phy/phy.c > @@ -1760,3 +1760,89 @@ int phy_ethtool_nway_reset(struct net_device *ndev) > return ret; > } > 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) No inline functions in .c file please. Let the compiler decide. > +bool phy_inband_ext_available(struct phy_device *phydev, enum phy_inband_ext ext) > +{ > + return !!(phydev->inband_ext.available & ext); should this be BIT(ext) ? > +} > +EXPORT_SYMBOL(phy_inband_ext_available); If you don't mind, i would prefer EXPORT_SYMBOL_GPL(). > +static int phy_set_inband_ext(struct phy_device *phydev, > + enum phy_inband_ext ext, > + bool enable) > +{ > + int ret; > + > + if (!phy_interface_has_inband_ext(phydev->interface)) > + return -EOPNOTSUPP; > + > + if (!phydev->drv->set_inband_ext) > + return -EOPNOTSUPP; That is a driver bug. It should not set phydev->inband_ext.available and then not have drv->set_inband_ext. So we should probably test this earlier. Maybe define that phydev->inband_ext.available has to be set during probe, and the core can validate this after probe and reject the device if it is inconsistent? > + > + mutex_lock(&phydev->lock); > + ret = phydev->drv->set_inband_ext(phydev, ext, enable); > + mutex_unlock(&phydev->lock); > + if (ret) > + return ret; > + > + if (enable) > + phydev->inband_ext.enabled |= BIT(ext); > + else > + phydev->inband_ext.enabled &= ~BIT(ext); Should these be also protected by the mutex? Andrew
Hello Andrew, On Tue, 13 Feb 2024 15:03:01 +0100 Andrew Lunn <andrew@lunn.ch> wrote: > > +Inband Extensions > > +================= > > + > > +The USGMII Standard allows the possibility to re-use the full-length 7-bytes > > +frame preamble to convey meaningful data. This is already partly used by modes > > +like QSGMII, which passes the port number in the preamble. > > + > > +In USGMII, we have a standardized approach to allow the MAC and PHY to pass > > +such data in the preamble, which looks like this : > > + > > +| 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | Frame data > > +| SoP | | Extension | CRC | > > +| / \_______________ | | > > +| / \ | | > > +| | type | subport | ext type | | | > > + > > +The preamble in that case uses the Packet Control Header (PCH) format, where > > +the byte 1 is used as a control field with : > > + > > +type - 2 bits : > > + - 00 : Packet with PCH > > + - 01 : Packet without PCH > > + - 10 : Idle Packet, without data > > + - 11 : Reserved > > + > > +subport - 4 bits : The subport identifier. For QUSGMII, this field ranges from > > + 0 to 3, and for OUSGMII, it ranges from 0 to 7. > > + > > +ext type - 2 bits : Indicated the type of data conveyed in the extension > > + - 00 : Ignore extension > > + - 01 : 8 bits reserved + 32 timestamp > > + - 10 : Reserved > > + - 11 : Reserved > > Somewhat crystal ball... > > Those two reserved values could be used in the future to indicate > other extensions. So we could have three in operation at once, but > only one selected per frame. > > > +A PHY driver can register available modes with:: > > + > > + int phy_inband_ext_set_available(struct phy_device *phydev, enum phy_inband_ext ext); > > + int phy_inband_ext_set_unavailable(struct phy_device *phydev, enum phy_inband_ext ext); > > enum phy_inband_ext is just an well defined, but arbitrary number? 0 > is this time stamp value mode, 1 could be used MACSEC, 2 could be a > QoS indicator when doing rate adaptation? 3 could be .... > > > +It's then up to the MAC driver to enable/disable the extension in the PHY as > > +needed. This was designed to fit the timestamping configuration model, as it > > +is the only mode supported so far. > > + > > +Enabling/Disabling an extension is done from the MAC driver through:: > > + > > + int phy_inband_ext_enable(struct phy_device *phydev, enum phy_inband_ext ext); > > So maybe this should return the 2 bit ext type value? The MAC can > request QoS marking, and the PHY replies it expects the bits to be 3 ? > > I'm just trying to ensure we have an API which is extensible in the > future to make use of those two reserved values. You are right, that's a much better idea ! > > > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > > index 3b9531143be1..4b6cf94f51d5 100644 > > --- a/drivers/net/phy/phy.c > > +++ b/drivers/net/phy/phy.c > > @@ -1760,3 +1760,89 @@ int phy_ethtool_nway_reset(struct net_device *ndev) > > return ret; > > } > > 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) > > No inline functions in .c file please. Let the compiler decide. My bad this one slept through the cracks... > > > +bool phy_inband_ext_available(struct phy_device *phydev, enum phy_inband_ext ext) > > +{ > > + return !!(phydev->inband_ext.available & ext); > > should this be BIT(ext) ? Correct indeed > > > +} > > +EXPORT_SYMBOL(phy_inband_ext_available); > > If you don't mind, i would prefer EXPORT_SYMBOL_GPL(). I don't mind, I'll fix that > > > +static int phy_set_inband_ext(struct phy_device *phydev, > > + enum phy_inband_ext ext, > > + bool enable) > > +{ > > + int ret; > > + > > + if (!phy_interface_has_inband_ext(phydev->interface)) > > + return -EOPNOTSUPP; > > + > > + if (!phydev->drv->set_inband_ext) > > + return -EOPNOTSUPP; > > That is a driver bug. It should not set phydev->inband_ext.available > and then not have drv->set_inband_ext. So we should probably test this > earlier. Maybe define that phydev->inband_ext.available has to be set > during probe, and the core can validate this after probe and reject > the device if it is inconsistent? Good point, I'll add that ! > > > + > > + mutex_lock(&phydev->lock); > > + ret = phydev->drv->set_inband_ext(phydev, ext, enable); > > + mutex_unlock(&phydev->lock); > > + if (ret) > > + return ret; > > + > > + if (enable) > > + phydev->inband_ext.enabled |= BIT(ext); > > + else > > + phydev->inband_ext.enabled &= ~BIT(ext); > > Should these be also protected by the mutex? I think you are right, it would be better making sure we serialize accesses to these indeed. Thanks for the review, Maxime
diff --git a/Documentation/networking/phy.rst b/Documentation/networking/phy.rst index 1283240d7620..f10a45ac7053 100644 --- a/Documentation/networking/phy.rst +++ b/Documentation/networking/phy.rst @@ -538,6 +538,76 @@ Call one of following function before unloading module:: int phy_unregister_fixup_for_uid(u32 phy_uid, u32 phy_uid_mask); int phy_register_fixup_for_id(const char *phy_id); +Inband Extensions +================= + +The USGMII Standard allows the possibility to re-use the full-length 7-bytes +frame preamble to convey meaningful data. This is already partly used by modes +like QSGMII, which passes the port number in the preamble. + +In USGMII, we have a standardized approach to allow the MAC and PHY to pass +such data in the preamble, which looks like this : + +| 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | Frame data +| SoP | | Extension | CRC | +| / \_______________ | | +| / \ | | +| | type | subport | ext type | | | + +The preamble in that case uses the Packet Control Header (PCH) format, where +the byte 1 is used as a control field with : + +type - 2 bits : + - 00 : Packet with PCH + - 01 : Packet without PCH + - 10 : Idle Packet, without data + - 11 : Reserved + +subport - 4 bits : The subport identifier. For QUSGMII, this field ranges from + 0 to 3, and for OUSGMII, it ranges from 0 to 7. + +ext type - 2 bits : Indicated the type of data conveyed in the extension + - 00 : Ignore extension + - 01 : 8 bits reserved + 32 timestamp + - 10 : Reserved + - 11 : Reserved + +It is possible for vendors to use the extensions mechanism without relying on +the PCH formatting. + +In order to leverage such extensions, both the MAC and the PHY have to agree on +which extension to use. The current model has the PHY expose the possible +available extensions with :: + + bool phy_inband_ext_available(struct phy_device *phydev, enum phy_inband_ext ext); + +The PHY driver decides which extensions are available to use at any given time, +as they can only be used if :: + - A compatible PHY mode is used, such as USXGMII or QUSGMII + - The PHY can use the required mode at that moment + +A PHY driver can register available modes with:: + + int phy_inband_ext_set_available(struct phy_device *phydev, enum phy_inband_ext ext); + int phy_inband_ext_set_unavailable(struct phy_device *phydev, enum phy_inband_ext ext); + +It's then up to the MAC driver to enable/disable the extension in the PHY as +needed. This was designed to fit the timestamping configuration model, as it +is the only mode supported so far. + +Enabling/Disabling an extension is done from the MAC driver through:: + + int phy_inband_ext_enable(struct phy_device *phydev, enum phy_inband_ext ext); + int phy_inband_ext_disable(struct phy_device *phydev, enum phy_inband_ext ext); + +These functions will cause the relevant callback to be called in the PHY driver:: + + int (*set_inband_ext)(struct phy_device *dev, enum phy_inband_ext ext, bool enable); + +The state of currently enabled extensions can be queried with:: + + bool phy_inband_ext_enabled(struct phy_device *phydev, enum phy_inband_ext ext); + Standards ========= diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 3b9531143be1..4b6cf94f51d5 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -1760,3 +1760,89 @@ int phy_ethtool_nway_reset(struct net_device *ndev) return ret; } 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, enum phy_inband_ext ext) +{ + return !!(phydev->inband_ext.available & ext); +} +EXPORT_SYMBOL(phy_inband_ext_available); + +bool phy_inband_ext_enabled(struct phy_device *phydev, enum phy_inband_ext ext) +{ + return !!(phydev->inband_ext.enabled & ext); +} +EXPORT_SYMBOL(phy_inband_ext_enabled); + +static int phy_set_inband_ext(struct phy_device *phydev, + enum phy_inband_ext ext, + bool enable) +{ + int ret; + + if (!phy_interface_has_inband_ext(phydev->interface)) + return -EOPNOTSUPP; + + if (!phydev->drv->set_inband_ext) + return -EOPNOTSUPP; + + mutex_lock(&phydev->lock); + ret = phydev->drv->set_inband_ext(phydev, ext, enable); + mutex_unlock(&phydev->lock); + if (ret) + return ret; + + if (enable) + phydev->inband_ext.enabled |= BIT(ext); + else + phydev->inband_ext.enabled &= ~BIT(ext); + + return 0; +} + +int phy_inband_ext_enable(struct phy_device *phydev, enum phy_inband_ext ext) +{ + if (!phy_inband_ext_available(phydev, ext)) + return -EOPNOTSUPP; + + return phy_set_inband_ext(phydev, ext, true); +} +EXPORT_SYMBOL(phy_inband_ext_enable); + +int phy_inband_ext_disable(struct phy_device *phydev, enum phy_inband_ext ext) +{ + return phy_set_inband_ext(phydev, ext, false); +} +EXPORT_SYMBOL(phy_inband_ext_disable); + +int phy_inband_ext_set_available(struct phy_device *phydev, enum phy_inband_ext ext) +{ + if (!(BIT(ext) & phydev->drv->inband_ext)) + return -EOPNOTSUPP; + + phydev->inband_ext.available |= BIT(ext); + + return 0; +} +EXPORT_SYMBOL(phy_inband_ext_set_available); + +int phy_inband_ext_set_unavailable(struct phy_device *phydev, enum phy_inband_ext ext) +{ + if (!(BIT(ext) & phydev->drv->inband_ext)) + return -EOPNOTSUPP; + + phydev->inband_ext.available &= ~BIT(ext); + + return 0; +} +EXPORT_SYMBOL(phy_inband_ext_set_unavailable); diff --git a/include/linux/phy.h b/include/linux/phy.h index a66f07d3f5f4..b358a96f71e7 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -202,6 +202,25 @@ static inline void phy_interface_set_rgmii(unsigned long *intf) __set_bit(PHY_INTERFACE_MODE_RGMII_TXID, intf); } +/** + * enum phy_inband_ext - Inband extensions + * + * @PHY_INBAND_EXT_PCH_TIMESTAMP: Transmit the nanoseconds part of a timestamp, + * Using the PCH format. + * + * Describes the inband extensions that can be conveyed in the ethernet preamble + */ +enum phy_inband_ext { + PHY_INBAND_EXT_PCH_TIMESTAMP = 0, +}; + +int phy_inband_ext_enable(struct phy_device *phydev, enum phy_inband_ext ext); +int phy_inband_ext_disable(struct phy_device *phydev, enum phy_inband_ext ext); +int phy_inband_ext_set_available(struct phy_device *phydev, enum phy_inband_ext ext); +int phy_inband_ext_set_unavailable(struct phy_device *phydev, enum phy_inband_ext ext); +bool phy_inband_ext_available(struct phy_device *phydev, enum phy_inband_ext ext); +bool phy_inband_ext_enabled(struct phy_device *phydev, enum phy_inband_ext ext); + /* * phy_supported_speeds - return all speeds currently supported by a PHY device */ @@ -678,6 +697,11 @@ struct phy_device { phy_interface_t interface; DECLARE_PHY_INTERFACE_MASK(possible_interfaces); + struct { + u32 available; + u32 enabled; + } inband_ext; + /* * forced speed & duplex (no autoneg) * partner speed & duplex & pause (autoneg) @@ -908,6 +932,7 @@ struct phy_driver { u32 phy_id_mask; const unsigned long * const features; u32 flags; + u32 inband_ext; const void *driver_data; /** @@ -1167,6 +1192,9 @@ struct phy_driver { */ int (*led_polarity_set)(struct phy_device *dev, int index, unsigned long modes); + /** @set_inband_ext: Enable or disable a given extension*/ + int (*set_inband_ext)(struct phy_device *dev, enum phy_inband_ext ext, + bool enable); }; #define to_phy_driver(d) container_of(to_mdio_common_driver(d), \ struct phy_driver, mdiodrv)
The USGMII 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 USGMII 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> --- Documentation/networking/phy.rst | 70 ++++++++++++++++++++++++++ drivers/net/phy/phy.c | 86 ++++++++++++++++++++++++++++++++ include/linux/phy.h | 28 +++++++++++ 3 files changed, 184 insertions(+)