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 Fri, 13 Oct 2023 18:11:19 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > > 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.  
> > 
> > Netdev vs phylib is an implementation detail of Linux.
> > I'm also surprised that you changed this.
> >   
> > > > > + */
> > > > > +enum {
> > > > > +	NO_TIMESTAMPING = 0,
> > > > > +	NETDEV_TIMESTAMPING = (1 << 0),
> > > > > +	PHYLIB_TIMESTAMPING = (1 << 1),
> > > > > +	SOFTWARE_TIMESTAMPING = (1 << 2) | (1 << 0),      
> 
> Just emphasising Jakubs point here. phylib is an implementation
> detail, in that the MAC driver might be using firmware to drive its
> PHY, and that firmware can do a timestamp in the PHY. The API being
> defined here should be independent of the implementation details. So
> it probably should be MAC_TIMESTAMPING and PHY_TIMESTAMPING, and leave
> it to the driver to decide if its PHYLIB doing the actual work, or
> firmware.

That is one reason why I moved to NETDEV_TIMESTAMPING, we don't know if it will
really be the MAC that does the timestamping, as the firmware could ask the PHY
to does it, but it surely goes though the netdev driver.

> Netdev vs phylib is an implementation detail of Linux.
> I'm also surprised that you changed this.

This is the main reason I changed this. This is Linux implementation purpose to
know whether it should go through netdev or phylib, and then each of these
drivers could use other timestamps which are hardware related.

As I have answered to Florian maybe you prefer to separate the Linux
implementation detail and the hardware timestamping like this:

> 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,
> };


> > The gist of what I'm proposing is for the core ethtool netlink message
> > handler to get just the phc_index as an attribute. No other information
> > as to what it represents. Not that it's netdev, DMA, phylib PHY or whatnot.
> > 
> > The ethtool kernel code would iterate through the stuff registered in
> > the system for the netdev, calling get_ts_info() or phy_ts_info() on it,
> > until it finds something which populates struct ethtool_ts_info ::
> > phc_index with the phc_index retrieved from netlink.
> > 
> > Then, ethtool just talks with the timestamper that matched that phc_index.
> > 
> > Same idea would be applied for the command that lists all timestamping
> > layers for a netdev. Check get_ts_info(), phy_ts_info(dev->phydev), and
> > can be extended in the future.  
> 
> I see, that could work. The user would then dig around sysfs to figure
> out which PHC has what characteristics?

I am not an expert but there are net drivers that enable
SOF_TIMESTAMPING_TX/RX/RAW_HARDWARE without phc. In that case we won't ever be
able to enter the get_ts_info() with you proposition.
Still I am wondering why hardware timestamping capabilities can be enabled
without phc.