Re: [PATCH net-next v5 08/16] net: ethtool: Add a command to expose current time stamping layer

["Köry Maincent" <kory.maincent@bootlin.com>]

On Mon, 9 Oct 2023 14:20:02 -0700
Florian Fainelli <florian.fainelli@broadcom.com> wrote:

Hello Florian,
Thanks for your review!

> > +/*
> > + * Hardware layer of the TIMESTAMPING provider
> > + * New description layer should have the NETDEV_TIMESTAMPING or
> > + * PHYLIB_TIMESTAMPING bit set to know which API to use for timestamping.  
> 
> If we are talking about hardware layers, then we shall use either 
> PHY_TIMESTAMPING or MAC_TIMESTAMPING. PHYLIB is the sub-subsystem to 
> deal with Ethernet PHYs, and netdev is the object through which we 
> represent network devices, so they are not even quite describing similar 
> things. If you go with the {PHY,MAC}_TIMESTAMPING suggestion, then I 
> could see how we could somewhat easily add PCS_TIMESTAMPING for instance.

I am indeed talking about hardware layers but I updated the name to use NETDEV
and PHYLIB timestamping for a reason. It is indeed only PHY or MAC timestamping
for now but it may be expanded in the future to theoretically to 7 layers of
timestamps possible. Also there may be several possible timestamp within a MAC
device precision vs volume.
See the thread of my last version that talk about it:
https://lore.kernel.org/netdev/20230511203646.ihljeknxni77uu5j@skbuf/

All these possibles timestamps go through exclusively the netdev API or the
phylib API. Even the software timestamping is done in the netdev driver,
therefore it goes through the netdev API and then should have the
NETDEV_TIMESTAMPING bit set.

> > + */
> > +enum {
> > +	NO_TIMESTAMPING = 0,
> > +	NETDEV_TIMESTAMPING = (1 << 0),
> > +	PHYLIB_TIMESTAMPING = (1 << 1),
> > +	SOFTWARE_TIMESTAMPING = (1 << 2) | (1 << 0),  
> 
> Why do we have to set NETDEV_TIMESTAMPING here, or is this a round-about 
> way of enumerating 0, 1, 2 and 3?

I answered you above the software timestamping should have the
NETDEV_TIMESTAMPING bit set as it is done from the net device driver.

What I was thinking is that all the new timestamping should have
NETDEV_TIMESTAMPING or PHYLIB_TIMESTAMPING set to know which API to pass
through.
Like we could add these in the future:
MAC_DMA_TIMESTAMPING = (2 << 2) | (1 >> 0),
MAC_PRECISION_TIMESTAMPING = (3 << 2) | (1 >> 0),
...
PHY_SFP_TIMESTAMPING = (2 << 2) | (1 << 1),
...


Or maybe do you prefer to use defines like this:
# define NETDEV_TIMESTAMPING (1 << 0)
# define PHYLIB_TIMESTAMPING (1 << 1)

enum {
	NO_TIMESTAMPING = 0,
	MAC_TIMESTAMPING = NETDEV_TIMESTAMPING,
	PHY_TIMESTAMPING = PHYLIB_TIMESTAMPING,
	SOFTWARE_TIMESTAMPING = (1 << 2) | NETDEV_TIMESTAMPING,
	...
	MAC_DMA_TIMESTAMPING = (2 << 2) | NETDEV_TIMESTAMPING,
	MAC_PRECISION_TIMESTAMPING = (3 << 2) | NETDEV_TIMESTAMPING,

or other idea?

Regards,