[net-next,0/6] net: Introduce Ethernet Inband Extensions

Hello everyone,

This series introduces support for Ethernet in-band extensions, a
mechanism proposed by Cisco as part of the USXGMII spec.

The idea is to leverage the 7 bytes preamble to convey meaningful data,
in what's called an "extension".

This series adds the QUSGMII mode, which is a quad variant of the
USXGMII standard, and adds its support in the lan966x driver. In
QUSGMII, extensions can be used.

The only extension support thus far is the PCH mode, a way to convey
part of a timestamp into the ethernet preamble. That's a pretty
straightfoward extension, documented in the Cisco spec. Other extensions
can exist, each being identified by a 2 bits code in the preamble,
parsed by the hardware.

We therefore need an API to synchronise which mode is supported by a
given PHY, then a way to enable it in the PHY, from the MAC's control.

This is done through a new phy_driver callback, .inband_ext_config(),
that the MAC driver will call to ask a PHY driver to enable a given
extension.

The PCH mode that is added in this series is used to offload a bit
the MDIO bus when doing PHY-side timestamping, by conveying the nanoseconds
part of the timestamp into the preamble. The MAC driver then extracts
the timestamp (using lan966x's IFH mechanism), puts the nanosecond part
in the SKB. The RX deferred timestamping then asks the PHY for the rest
of the timestamp.

Other modes exists, such as Microchip's MCH mode, but this series only
include PCH since it's simple enough and keeps the code reviewable.

Thanks,

Maxime

Maxime Chevallier (6):
  net: phy: Introduce QUSGMII PHY mode
  dt-bindings: net: ethernet-controller: add QUSGMII mode
  net: lan966x: Add QUSGMII support for lan966x
  net: phy: Add support for inband extensions
  net: lan966x: Allow using PCH extension for PTP
  net: phy: micrel: Add QUSGMII support and PCH extension

 .../bindings/net/ethernet-controller.yaml     |   1 +
 Documentation/networking/phy.rst              |   9 ++
 .../ethernet/microchip/lan966x/lan966x_main.c |  14 +--
 .../ethernet/microchip/lan966x/lan966x_main.h |   6 ++
 .../microchip/lan966x/lan966x_phylink.c       |   9 +-
 .../ethernet/microchip/lan966x/lan966x_port.c |  33 ++++--
 .../ethernet/microchip/lan966x/lan966x_ptp.c  |  93 +++++++++++++++-
 .../ethernet/microchip/lan966x/lan966x_regs.h |  72 +++++++++++++
 drivers/net/phy/micrel.c                      | 102 ++++++++++++++++--
 drivers/net/phy/phy.c                         |  68 ++++++++++++
 drivers/net/phy/phylink.c                     |   3 +
 include/linux/phy.h                           |  28 ++++-
 12 files changed, 413 insertions(+), 25 deletions(-)

> +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

Hi,

On Thu, May 19, 2022 at 03:56:44PM +0200, Maxime Chevallier wrote:
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> index e6642083ab9e..304c784f48f6 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> @@ -452,4 +452,10 @@ static inline void lan_rmw(u32 val, u32 mask, struct lan966x *lan966x,
>  			      gcnt, gwidth, raddr, rinst, rcnt, rwidth));
>  }
>  
> +static inline bool lan966x_is_qsgmii(phy_interface_t mode)
> +{
> +	return (mode == PHY_INTERFACE_MODE_QSGMII) ||
> +	       (mode == PHY_INTERFACE_MODE_QUSGMII);
> +}

Maybe linux/phy.h should provide a helper, something like:

	phy_interface_serdes_lanes()

that returns how many serdes lanes the interface mode uses?

> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_phylink.c b/drivers/net/ethernet/microchip/lan966x/lan966x_phylink.c
> index 38a7e95d69b4..96708352f53e 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_phylink.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_phylink.c
> @@ -28,11 +28,18 @@ static int lan966x_phylink_mac_prepare(struct phylink_config *config,
>  				       phy_interface_t iface)
>  {
>  	struct lan966x_port *port = netdev_priv(to_net_dev(config->dev));
> +	phy_interface_t serdes_mode = iface;
>  	int err;
>  
>  	if (port->serdes) {
> +		/* As far as the SerDes is concerned, QUSGMII is the same as
> +		 * QSGMII.
> +		 */
> +		if (lan966x_is_qsgmii(iface))
> +			serdes_mode = PHY_INTERFACE_MODE_QSGMII;
> +
>  		err = phy_set_mode_ext(port->serdes, PHY_MODE_ETHERNET,
> -				       iface);
> +				       serdes_mode);

I don't think that the ethernet MAC driver should be changing the
interface mode before passing it down to the generic PHY layer -
phy_set_mode_ext() is defined to take the phy interface mode, and any
aliasing of modes should really be up to the generic PHY driver not
the ethernet MAC driver.

Thanks.

> +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

Hello Russell,

On Thu, 19 May 2022 15:26:23 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> Hi,
> 
> On Thu, May 19, 2022 at 03:56:44PM +0200, Maxime Chevallier wrote:
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h index
> > e6642083ab9e..304c784f48f6 100644 ---
> > a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h +++
> > b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h @@ -452,4
> > +452,10 @@ static inline void lan_rmw(u32 val, u32 mask, struct
> > lan966x *lan966x, gcnt, gwidth, raddr, rinst, rcnt, rwidth)); }
> >  
> > +static inline bool lan966x_is_qsgmii(phy_interface_t mode)
> > +{
> > +	return (mode == PHY_INTERFACE_MODE_QSGMII) ||
> > +	       (mode == PHY_INTERFACE_MODE_QUSGMII);
> > +}  
>
> Maybe linux/phy.h should provide a helper, something like:
> 
> 	phy_interface_serdes_lanes()
> 
> that returns how many serdes lanes the interface mode uses?

Sorry about the delayed answer, I was resuming the work on this, and
realised that although a helper would be indeed great, especially for
generic PHY drivers, it won't help much in this case since
QSGMII/QUSGMII both use 1 serdes lane, as SGMII and such. If I'm not
mistaken, QSGMII is SGMII clocked at 5Gbps with a specific preamble
allowing to identify the src/dst port.

We could however imagine a helper identifying the number of links, or
lanes (or another terminology) that is carried by a given mode. I know
that besides QSGMII for 4 ports, there exists PSGMII for 5 ports, and
OSGMII for 8 ports, so this would definitely prove useful in the
future.

Sorry if this ends-up being a misunderstanding on the terminology,
we're probably already talking about the same thing, but I think that
"serdes lane" would better describe the number of physical differential
pairs that creates the link (like, 1 for SGMII, 2 for RXAUI, 4 for XAUI
and so on).

maybe something like

	phy_interface_lines() or
	phy_interface_num_ports() or simply
	phy_interface_lanes()

> > diff --git
> > a/drivers/net/ethernet/microchip/lan966x/lan966x_phylink.c
> > b/drivers/net/ethernet/microchip/lan966x/lan966x_phylink.c index
> > 38a7e95d69b4..96708352f53e 100644 ---
> > a/drivers/net/ethernet/microchip/lan966x/lan966x_phylink.c +++
> > b/drivers/net/ethernet/microchip/lan966x/lan966x_phylink.c @@
> > -28,11 +28,18 @@ static int lan966x_phylink_mac_prepare(struct
> > phylink_config *config, phy_interface_t iface) { struct
> > lan966x_port *port = netdev_priv(to_net_dev(config->dev));
> > +	phy_interface_t serdes_mode = iface;
> >  	int err;
> >  
> >  	if (port->serdes) {
> > +		/* As far as the SerDes is concerned, QUSGMII is
> > the same as
> > +		 * QSGMII.
> > +		 */
> > +		if (lan966x_is_qsgmii(iface))
> > +			serdes_mode = PHY_INTERFACE_MODE_QSGMII;
> > +
> >  		err = phy_set_mode_ext(port->serdes,
> > PHY_MODE_ETHERNET,
> > -				       iface);
> > +				       serdes_mode);  
> 
> I don't think that the ethernet MAC driver should be changing the
> interface mode before passing it down to the generic PHY layer -
> phy_set_mode_ext() is defined to take the phy interface mode, and any
> aliasing of modes should really be up to the generic PHY driver not
> the ethernet MAC driver.

Indeed, I'll split the series so that we first add support for the new
mode, and then send separate series for the generic PHY driver on one
side, and inband extensions on the other one.

Thanks,

Maxime

> Thanks.
>