On Fri, Mar 10, 2023 at 06:44:51PM +0200, Vladimir Oltean wrote:

On Fri, Mar 10, 2023 at 02:15:29PM +0100, Horatiu Vultur wrote:

quoted

I was thinking about another scenario (I am sorry if this was already
discussed).
Currently when setting up to do the timestamp, the MAC will check if the
PHY has timestamping support if that is the case the PHY will do the
timestamping. So in case the switch was supposed to be a TC then we had
to make sure that the HW was setting up some rules not to forward PTP
frames by HW but to copy these frames to CPU.
With this new implementation, this would not be possible anymore as the
MAC will not be notified when doing the timestamping in the PHY.
Does it mean that now the switch should allocate these rules at start
time?

I would say no (to the allocation of trapping rules at startup time).
It was argued before by people present in this thread that it should be
possible (and default behavior) for switches to forward PTP frames as if
they were PTP-unaware:
https://patchwork.ozlabs.org/project/netdev/patch/20190813025214.18601-5-yangbo.lu@nxp.com/

But it raises a really good point about how much care a switch driver
needs to take, such that with PTP timestamping, it must trap but not
timestamp the PTP frames.

There is a huge amount of variability here today.

The ocelot driver would be broken with PHY timestamping, since it would
flood the PTP messages (it installs the traps only if it is responsible
for taking the timestamps too).

The lan966x driver is very fine-tuned to call lan966x_ptp_setup_traps()
regardless of what phy_has_hwtstamp() says.

The sparx5 driver doesn't even seem to install traps at all (unclear if
they are predefined in hardware or not).

I guess that we want something like lan966x to keep working, since it
sounds like it's making the sanest decision about what to do.

But, as you point out, with Köry's/Richard's proposal, the MAC driver
will be bypassed when the selected timestamping layer is the PHY, and
that's a problem currently.

May I suggest the following? There was another RFC which proposed the
introduction of a netdev notifier when timestamping is turned on/off:
https://lore.kernel.org/netdev/20220317225035.3475538-1-vladimir.oltean@nxp.com/ (local)

It didn't go beyond RFC status, because I started doing what Jakub
suggested (converting the raw ioctls handlers to NDOs) but quickly got
absolutely swamped into the whole mess.

If we have a notifier, then we can make switch drivers do things
differently. They can activate timestamping per se in the timestamping
NDO (which is only called when the MAC is the active timestamping layer),
and they can activate PTP traps in the netdev notifier (which is called
any time a timestamping status change takes place - the notifier info
should contain details about which net_device and timestamping layer
this is, for example).

It's just a proposal of how to create an alternative notification path
that doesn't disturb the goals of this patch set.

To make things even more complicated - I  have a project where the DSA master
should be used for time stamping. Due to board specific limitations, we
are forced to use FEC PHC instead of dsa KSZ switch PHC. So, it is not
a choice between MAC and PHY, it is more the MAC before MAC and PHY.
PTP sync in this case will have more jitter, but it still good enough
for this project.
Currently I use quick hack to do so, but mainlinamble solution working for
most use cases will be nice.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |