[PATCH net-next 0/5] PTP support for the SJA1105 DSA driver

[PATCH net-next 0/5] PTP support for the SJA1105 DSA driver

[Vladimir Oltean]

This patchset adds the following:

 - A timecounter/cyclecounter based PHC for the free-running
   timestamping clock of this switch.

 - A state machine implemented in the DSA tagger for SJA1105, which
   keeps track of metadata follow-up Ethernet frames (the switch's way
   of transmitting RX timestamps).

 - Some common-sense on whether or not frames should be timestamped was
   taken out of the mv88e6xxx driver (the only other DSA driver with PTP
   support) and moved to the generic framework.  An option was also
   added for drivers to override these common-sense decisions, and
   timestamp some more frames.  This was the path of least resistance
   after implementing the aforementioned state machine - metadata
   follow-up frames need to be tracked anyway even if only to discard
   them and not pass them up the network stack.  And since the switch
   can't just be told to timestamp only what the kernel wants (PTP
   frames), simply use all the timestamps it provides.

 - A generic helper in the timecounter/cyclecounter code for
   reconstructing partial PTP timestamps, such as those generated by the
   SJA1105.

Not all is rosy, though.

PTP timestamping will only work when the ports are bridged. Otherwise,
the metadata follow-up frames holding RX timestamps won't be received
because they will be blocked by the master port's MAC filter. Linuxptp
tries to put the net device in ALLMULTI/PROMISC mode, but DSA doesn't
pass this on to the master port, which does the actual reception.
The master port is put in promiscous mode when the slave ports are
enslaved to a bridge.

Also, even with software-corrected timestamps, one can observe a
negative path delay reported by linuxptp:

ptp4l[55.600]: master offset          8 s2 freq  +83677 path delay     -2390
ptp4l[56.600]: master offset         17 s2 freq  +83688 path delay     -2391
ptp4l[57.601]: master offset          6 s2 freq  +83682 path delay     -2391
ptp4l[58.601]: master offset         -1 s2 freq  +83677 path delay     -2391

Without investigating too deeply, this appears to be introduced by the
correction applied by linuxptp to t4 (t4c: corrected master rxtstamp)
during the path delay estimation process (removing the correction makes
the path delay positive).  This does not appear to have an obvious
negative effect upon the synchronization.

Lastly, clock manipulations on the actual hardware PTP clock will have
to be implemented anyway, for the TTEthernet block and the time-based
ingress policer.

Vladimir Oltean (5):
  timecounter: Add helper for reconstructing partial timestamps
  net: dsa: sja1105: Add support for the PTP clock
  net: dsa: mv88e6xxx: Let taggers specify a can_timestamp function
  net: dsa: sja1105: Add support for PTP timestamping
  net: dsa: sja1105: Increase priority of CPU-trapped frames

 drivers/net/dsa/mv88e6xxx/hwtstamp.c          |  25 +-
 drivers/net/dsa/mv88e6xxx/hwtstamp.h          |   4 +-
 drivers/net/dsa/sja1105/Kconfig               |   7 +
 drivers/net/dsa/sja1105/Makefile              |   1 +
 drivers/net/dsa/sja1105/sja1105.h             |  30 ++
 .../net/dsa/sja1105/sja1105_dynamic_config.c  |   2 +
 drivers/net/dsa/sja1105/sja1105_main.c        | 272 ++++++++++++-
 drivers/net/dsa/sja1105/sja1105_ptp.c         | 357 ++++++++++++++++++
 drivers/net/dsa/sja1105/sja1105_ptp.h         |  48 +++
 drivers/net/dsa/sja1105/sja1105_spi.c         |  28 ++
 .../net/dsa/sja1105/sja1105_static_config.c   |  59 +++
 .../net/dsa/sja1105/sja1105_static_config.h   |  10 +
 include/linux/dsa/sja1105.h                   |  15 +
 include/linux/timecounter.h                   |   7 +
 include/net/dsa.h                             |   6 +-
 kernel/time/timecounter.c                     |  33 ++
 net/dsa/dsa.c                                 |  25 +-
 net/dsa/slave.c                               |  20 +-
 net/dsa/tag_sja1105.c                         | 135 ++++++-
 19 files changed, 1043 insertions(+), 41 deletions(-)
 create mode 100644 drivers/net/dsa/sja1105/sja1105_ptp.c
 create mode 100644 drivers/net/dsa/sja1105/sja1105_ptp.h

-- 
2.17.1

[PATCH net-next 1/5] timecounter: Add helper for reconstructing partial timestamps

[Vladimir Oltean]

Some PTP hardware offers a 64-bit free-running counter whose snapshots
are used for timestamping, but only makes part of that snapshot
available as timestamps (low-order bits).

In that case, timecounter/cyclecounter users must bring the cyclecounter
and timestamps to the same bit width, and they currently have two
options of doing so:

- Trim the higher bits of the timecounter itself to the number of bits
  of the timestamps.  This might work for some setups, but if the
  wraparound of the timecounter in this case becomes high (~10 times per
  second) then this causes additional strain on the system, which must
  read the clock that often just to avoid missing the wraparounds.

- Reconstruct the timestamp by racing to read the PTP time within one
  wraparound cycle since the timestamp was generated.  This is
  preferable when the wraparound time is small (do a time-critical
  readout once vs doing it periodically), and it has no drawback even
  when the wraparound is comfortably sized.

Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 include/linux/timecounter.h |  7 +++++++
 kernel/time/timecounter.c   | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/include/linux/timecounter.h b/include/linux/timecounter.h
index 2496ad4cfc99..03eab1f3bb9c 100644
--- a/include/linux/timecounter.h
+++ b/include/linux/timecounter.h
@@ -30,6 +30,9 @@
  *	by the implementor and user of specific instances of this API.
  *
  * @read:		returns the current cycle value
+ * @partial_tstamp_mask:bitmask in case the hardware emits timestamps
+ *			which only capture low-order bits of the full
+ *			counter, and should be reconstructed.
  * @mask:		bitmask for two's complement
  *			subtraction of non 64 bit counters,
  *			see CYCLECOUNTER_MASK() helper macro
@@ -38,6 +41,7 @@
  */
 struct cyclecounter {
 	u64 (*read)(const struct cyclecounter *cc);
+	u64 partial_tstamp_mask;
 	u64 mask;
 	u32 mult;
 	u32 shift;
@@ -136,4 +140,7 @@ extern u64 timecounter_read(struct timecounter *tc);
 extern u64 timecounter_cyc2time(struct timecounter *tc,
 				u64 cycle_tstamp);
 
+extern u64 cyclecounter_reconstruct(const struct cyclecounter *cc,
+				    u64 ts_partial);
+
 #endif
diff --git a/kernel/time/timecounter.c b/kernel/time/timecounter.c
index 85b98e727306..d4657d64e38d 100644
--- a/kernel/time/timecounter.c
+++ b/kernel/time/timecounter.c
@@ -97,3 +97,36 @@ u64 timecounter_cyc2time(struct timecounter *tc,
 	return nsec;
 }
 EXPORT_SYMBOL_GPL(timecounter_cyc2time);
+
+/**
+ * cyclecounter_reconstruct - reconstructs @ts_partial
+ * @cc:		Pointer to cycle counter.
+ * @ts_partial:	Typically RX or TX NIC timestamp, provided by hardware as
+ *		the lower @partial_tstamp_mask bits of the cycle counter,
+ *		sampled at the time the timestamp was collected.
+ *		To reconstruct into a full @mask bit-wide timestamp, the
+ *		cycle counter is read and the high-order bits (up to @mask) are
+ *		filled in.
+ *		Must be called within one wraparound of @partial_tstamp_mask
+ *		bits of the cycle counter.
+ */
+u64 cyclecounter_reconstruct(const struct cyclecounter *cc, u64 ts_partial)
+{
+	u64 ts_reconstructed;
+	u64 cycle_now;
+
+	cycle_now = cc->read(cc);
+
+	ts_reconstructed = (cycle_now & ~cc->partial_tstamp_mask) |
+			    ts_partial;
+
+	/* Check lower bits of current cycle counter against the timestamp.
+	 * If the current cycle counter is lower than the partial timestamp,
+	 * then wraparound surely occurred and must be accounted for.
+	 */
+	if ((cycle_now & cc->partial_tstamp_mask) <= ts_partial)
+		ts_reconstructed -= (cc->partial_tstamp_mask + 1);
+
+	return ts_reconstructed;
+}
+EXPORT_SYMBOL_GPL(cyclecounter_reconstruct);
-- 
2.17.1

Re: [PATCH net-next 1/5] timecounter: Add helper for reconstructing partial timestamps

[John Stultz]

On Tue, May 28, 2019 at 4:58 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> Some PTP hardware offers a 64-bit free-running counter whose snapshots
> are used for timestamping, but only makes part of that snapshot
> available as timestamps (low-order bits).
>
> In that case, timecounter/cyclecounter users must bring the cyclecounter
> and timestamps to the same bit width, and they currently have two
> options of doing so:
>
> - Trim the higher bits of the timecounter itself to the number of bits
>   of the timestamps.  This might work for some setups, but if the
>   wraparound of the timecounter in this case becomes high (~10 times per
>   second) then this causes additional strain on the system, which must
>   read the clock that often just to avoid missing the wraparounds.
>
> - Reconstruct the timestamp by racing to read the PTP time within one
>   wraparound cycle since the timestamp was generated.  This is
>   preferable when the wraparound time is small (do a time-critical
>   readout once vs doing it periodically), and it has no drawback even
>   when the wraparound is comfortably sized.
>
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> ---
>  include/linux/timecounter.h |  7 +++++++
>  kernel/time/timecounter.c   | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 40 insertions(+)
>
> diff --git a/include/linux/timecounter.h b/include/linux/timecounter.h
> index 2496ad4cfc99..03eab1f3bb9c 100644
> --- a/include/linux/timecounter.h
> +++ b/include/linux/timecounter.h
> @@ -30,6 +30,9 @@
>   *     by the implementor and user of specific instances of this API.
>   *
>   * @read:              returns the current cycle value
> + * @partial_tstamp_mask:bitmask in case the hardware emits timestamps
> + *                     which only capture low-order bits of the full
> + *                     counter, and should be reconstructed.
>   * @mask:              bitmask for two's complement
>   *                     subtraction of non 64 bit counters,
>   *                     see CYCLECOUNTER_MASK() helper macro
> @@ -38,6 +41,7 @@
>   */
>  struct cyclecounter {
>         u64 (*read)(const struct cyclecounter *cc);
> +       u64 partial_tstamp_mask;
>         u64 mask;
>         u32 mult;
>         u32 shift;
> @@ -136,4 +140,7 @@ extern u64 timecounter_read(struct timecounter *tc);
>  extern u64 timecounter_cyc2time(struct timecounter *tc,
>                                 u64 cycle_tstamp);
>
> +extern u64 cyclecounter_reconstruct(const struct cyclecounter *cc,
> +                                   u64 ts_partial);
> +
>  #endif
> diff --git a/kernel/time/timecounter.c b/kernel/time/timecounter.c
> index 85b98e727306..d4657d64e38d 100644
> --- a/kernel/time/timecounter.c
> +++ b/kernel/time/timecounter.c
> @@ -97,3 +97,36 @@ u64 timecounter_cyc2time(struct timecounter *tc,
>         return nsec;
>  }
>  EXPORT_SYMBOL_GPL(timecounter_cyc2time);
> +
> +/**
> + * cyclecounter_reconstruct - reconstructs @ts_partial
> + * @cc:                Pointer to cycle counter.
> + * @ts_partial:        Typically RX or TX NIC timestamp, provided by hardware as
> + *             the lower @partial_tstamp_mask bits of the cycle counter,
> + *             sampled at the time the timestamp was collected.
> + *             To reconstruct into a full @mask bit-wide timestamp, the
> + *             cycle counter is read and the high-order bits (up to @mask) are
> + *             filled in.
> + *             Must be called within one wraparound of @partial_tstamp_mask
> + *             bits of the cycle counter.
> + */
> +u64 cyclecounter_reconstruct(const struct cyclecounter *cc, u64 ts_partial)
> +{
> +       u64 ts_reconstructed;
> +       u64 cycle_now;
> +
> +       cycle_now = cc->read(cc);
> +
> +       ts_reconstructed = (cycle_now & ~cc->partial_tstamp_mask) |
> +                           ts_partial;
> +
> +       /* Check lower bits of current cycle counter against the timestamp.
> +        * If the current cycle counter is lower than the partial timestamp,
> +        * then wraparound surely occurred and must be accounted for.
> +        */
> +       if ((cycle_now & cc->partial_tstamp_mask) <= ts_partial)
> +               ts_reconstructed -= (cc->partial_tstamp_mask + 1);
> +
> +       return ts_reconstructed;
> +}
> +EXPORT_SYMBOL_GPL(cyclecounter_reconstruct);

Hrm. Is this actually generic? Would it make more sense to have the
specific implementations with this quirk implement this in their
read() handler? If not, why?

thanks
-john

Re: [PATCH net-next 1/5] timecounter: Add helper for reconstructing partial timestamps

[Richard Cochran]

On Tue, May 28, 2019 at 07:14:22PM -0700, John Stultz wrote:
> Hrm. Is this actually generic? Would it make more sense to have the
> specific implementations with this quirk implement this in their
> read() handler? If not, why?

Strongly agree that this workaround should stay in the driver.  After
all, we do not want to encourage HW designers to continue in this way.

Thanks,
Richard

Re: [PATCH net-next 1/5] timecounter: Add helper for reconstructing partial timestamps

[Vladimir Oltean]

On Wed, 29 May 2019 at 05:14, John Stultz <john.stultz@linaro.org> wrote:
>
> On Tue, May 28, 2019 at 4:58 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> >
> > Some PTP hardware offers a 64-bit free-running counter whose snapshots
> > are used for timestamping, but only makes part of that snapshot
> > available as timestamps (low-order bits).
> >
> > In that case, timecounter/cyclecounter users must bring the cyclecounter
> > and timestamps to the same bit width, and they currently have two
> > options of doing so:
> >
> > - Trim the higher bits of the timecounter itself to the number of bits
> >   of the timestamps.  This might work for some setups, but if the
> >   wraparound of the timecounter in this case becomes high (~10 times per
> >   second) then this causes additional strain on the system, which must
> >   read the clock that often just to avoid missing the wraparounds.
> >
> > - Reconstruct the timestamp by racing to read the PTP time within one
> >   wraparound cycle since the timestamp was generated.  This is
> >   preferable when the wraparound time is small (do a time-critical
> >   readout once vs doing it periodically), and it has no drawback even
> >   when the wraparound is comfortably sized.
> >
> > Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> > ---
> >  include/linux/timecounter.h |  7 +++++++
> >  kernel/time/timecounter.c   | 33 +++++++++++++++++++++++++++++++++
> >  2 files changed, 40 insertions(+)
> >
> > diff --git a/include/linux/timecounter.h b/include/linux/timecounter.h
> > index 2496ad4cfc99..03eab1f3bb9c 100644
> > --- a/include/linux/timecounter.h
> > +++ b/include/linux/timecounter.h
> > @@ -30,6 +30,9 @@
> >   *     by the implementor and user of specific instances of this API.
> >   *
> >   * @read:              returns the current cycle value
> > + * @partial_tstamp_mask:bitmask in case the hardware emits timestamps
> > + *                     which only capture low-order bits of the full
> > + *                     counter, and should be reconstructed.
> >   * @mask:              bitmask for two's complement
> >   *                     subtraction of non 64 bit counters,
> >   *                     see CYCLECOUNTER_MASK() helper macro
> > @@ -38,6 +41,7 @@
> >   */
> >  struct cyclecounter {
> >         u64 (*read)(const struct cyclecounter *cc);
> > +       u64 partial_tstamp_mask;
> >         u64 mask;
> >         u32 mult;
> >         u32 shift;
> > @@ -136,4 +140,7 @@ extern u64 timecounter_read(struct timecounter *tc);
> >  extern u64 timecounter_cyc2time(struct timecounter *tc,
> >                                 u64 cycle_tstamp);
> >
> > +extern u64 cyclecounter_reconstruct(const struct cyclecounter *cc,
> > +                                   u64 ts_partial);
> > +
> >  #endif
> > diff --git a/kernel/time/timecounter.c b/kernel/time/timecounter.c
> > index 85b98e727306..d4657d64e38d 100644
> > --- a/kernel/time/timecounter.c
> > +++ b/kernel/time/timecounter.c
> > @@ -97,3 +97,36 @@ u64 timecounter_cyc2time(struct timecounter *tc,
> >         return nsec;
> >  }
> >  EXPORT_SYMBOL_GPL(timecounter_cyc2time);
> > +
> > +/**
> > + * cyclecounter_reconstruct - reconstructs @ts_partial
> > + * @cc:                Pointer to cycle counter.
> > + * @ts_partial:        Typically RX or TX NIC timestamp, provided by hardware as
> > + *             the lower @partial_tstamp_mask bits of the cycle counter,
> > + *             sampled at the time the timestamp was collected.
> > + *             To reconstruct into a full @mask bit-wide timestamp, the
> > + *             cycle counter is read and the high-order bits (up to @mask) are
> > + *             filled in.
> > + *             Must be called within one wraparound of @partial_tstamp_mask
> > + *             bits of the cycle counter.
> > + */
> > +u64 cyclecounter_reconstruct(const struct cyclecounter *cc, u64 ts_partial)
> > +{
> > +       u64 ts_reconstructed;
> > +       u64 cycle_now;
> > +
> > +       cycle_now = cc->read(cc);
> > +
> > +       ts_reconstructed = (cycle_now & ~cc->partial_tstamp_mask) |
> > +                           ts_partial;
> > +
> > +       /* Check lower bits of current cycle counter against the timestamp.
> > +        * If the current cycle counter is lower than the partial timestamp,
> > +        * then wraparound surely occurred and must be accounted for.
> > +        */
> > +       if ((cycle_now & cc->partial_tstamp_mask) <= ts_partial)
> > +               ts_reconstructed -= (cc->partial_tstamp_mask + 1);
> > +
> > +       return ts_reconstructed;
> > +}
> > +EXPORT_SYMBOL_GPL(cyclecounter_reconstruct);
>
> Hrm. Is this actually generic? Would it make more sense to have the
> specific implementations with this quirk implement this in their
> read() handler? If not, why?

Hi John, Richard,

It's not the cycle counter that needs reconstruction, but hardware
timestamps based on it.  Hence not possible to add a workaround in the
read() handler.
If it's not desirable to have this helper function in the cyclecounter
I'll move it to the driver in v2.

Thanks,
-Vladimir

>
> thanks
> -john

[PATCH net-next 2/5] net: dsa: sja1105: Add support for the PTP clock

[Vladimir Oltean]

The design of this PHC driver is influenced by the switch's behavior
w.r.t. timestamping.  It exposes two PTP counters, one free-running
(PTPTSCLK) and the other offset- and frequency-corrected in hardware
through PTPCLKVAL, PTPCLKADD and PTPCLKRATE.  The MACs can sample either
of these for frame timestamps.

However, the user manual warns that taking timestamps based on the
corrected clock is less than useful, as the switch can deliver corrupted
timestamps in a variety of circumstances.

Therefore, this PHC uses the free-running PTPTSCLK together with a
timecounter/cyclecounter structure that translates it into a software
time domain.  Thus, the settime/adjtime and adjfine callbacks are
hardware no-ops.

The timestamps (introduced in a further patch) will also be translated
to the correct time domain before being handed over to the userspace PTP
stack.

The introduction of a second set of PHC operations that operate on the
hardware PTPCLKVAL/PTPCLKADD/PTPCLKRATE in the future is somewhat
unavoidable, as the TTEthernet core uses the corrected PTP time domain.
However, the free-running counter + timecounter structure combination
will suffice for now, as the resulting timestamps yield a sub-50 ns
synchronization offset in steady state using linuxptp.

For this patch, in absence of frame timestamping, the operations of the
switch PHC were tested by syncing it to the system time as a local slave
clock with:

phc2sys -s CLOCK_REALTIME -c swp2 -O 0 -m -S 0.01

Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/net/dsa/sja1105/Kconfig        |   7 +
 drivers/net/dsa/sja1105/Makefile       |   1 +
 drivers/net/dsa/sja1105/sja1105.h      |  19 ++
 drivers/net/dsa/sja1105/sja1105_main.c |   7 +
 drivers/net/dsa/sja1105/sja1105_ptp.c  | 293 +++++++++++++++++++++++++
 drivers/net/dsa/sja1105/sja1105_ptp.h  |  40 ++++
 drivers/net/dsa/sja1105/sja1105_spi.c  |  16 ++
 7 files changed, 383 insertions(+)
 create mode 100644 drivers/net/dsa/sja1105/sja1105_ptp.c
 create mode 100644 drivers/net/dsa/sja1105/sja1105_ptp.h

diff --git a/drivers/net/dsa/sja1105/Kconfig b/drivers/net/dsa/sja1105/Kconfig
index 1144fc5f61a8..049cea8240e4 100644
--- a/drivers/net/dsa/sja1105/Kconfig
+++ b/drivers/net/dsa/sja1105/Kconfig
@@ -16,3 +16,10 @@ tristate "NXP SJA1105 Ethernet switch family support"
 	    - SJA1105Q (Gen. 2, No SGMII, TT-Ethernet)
 	    - SJA1105R (Gen. 2, SGMII, No TT-Ethernet)
 	    - SJA1105S (Gen. 2, SGMII, TT-Ethernet)
+
+config NET_DSA_SJA1105_PTP
+tristate "Support for the PTP clock on the NXP SJA1105 Ethernet switch"
+	depends on NET_DSA_SJA1105
+	help
+	  This enables support for timestamping and PTP clock manipulations in
+	  the SJA1105 DSA driver.
diff --git a/drivers/net/dsa/sja1105/Makefile b/drivers/net/dsa/sja1105/Makefile
index 941848de8b46..946eea7d8480 100644
--- a/drivers/net/dsa/sja1105/Makefile
+++ b/drivers/net/dsa/sja1105/Makefile
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_NET_DSA_SJA1105) += sja1105.o
+obj-$(CONFIG_NET_DSA_SJA1105_PTP) += sja1105_ptp.o
 
 sja1105-objs := \
     sja1105_spi.o \
diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h
index b043bfc408f2..e0252e6f65ed 100644
--- a/drivers/net/dsa/sja1105/sja1105.h
+++ b/drivers/net/dsa/sja1105/sja1105.h
@@ -5,6 +5,8 @@
 #ifndef _SJA1105_H
 #define _SJA1105_H
 
+#include <linux/ptp_clock_kernel.h>
+#include <linux/timecounter.h>
 #include <linux/dsa/sja1105.h>
 #include <net/dsa.h>
 #include <linux/mutex.h>
@@ -27,6 +29,11 @@ struct sja1105_regs {
 	u64 rgu;
 	u64 config;
 	u64 rmii_pll1;
+	u64 ptp_control;
+	u64 ptpclk;
+	u64 ptpclkrate;
+	u64 ptptsclk;
+	u64 ptpegr_ts;
 	u64 pad_mii_tx[SJA1105_NUM_PORTS];
 	u64 cgu_idiv[SJA1105_NUM_PORTS];
 	u64 rgmii_pad_mii_tx[SJA1105_NUM_PORTS];
@@ -53,6 +60,7 @@ struct sja1105_info {
 	const struct sja1105_dynamic_table_ops *dyn_ops;
 	const struct sja1105_table_ops *static_ops;
 	const struct sja1105_regs *regs;
+	int (*ptp_cmd)(const void *ctx, const void *data);
 	int (*reset_cmd)(const void *ctx, const void *data);
 	int (*setup_rgmii_delay)(const void *ctx, int port);
 	const char *name;
@@ -67,6 +75,16 @@ struct sja1105_private {
 	struct spi_device *spidev;
 	struct dsa_switch *ds;
 	struct sja1105_port ports[SJA1105_NUM_PORTS];
+	struct ptp_clock_info ptp_caps;
+	struct ptp_clock *clock;
+	/* The cycle counter translates the PTP timestamps (based on
+	 * a free-running counter) into a software time domain.
+	 */
+	struct cyclecounter tstamp_cc;
+	struct timecounter tstamp_tc;
+	struct delayed_work refresh_work;
+	/* Serializes all operations on the cycle counter */
+	struct mutex ptp_lock;
 	/* Serializes transmission of management frames so that
 	 * the switch doesn't confuse them with one another.
 	 */
@@ -74,6 +92,7 @@ struct sja1105_private {
 };
 
 #include "sja1105_dynamic_config.h"
+#include "sja1105_ptp.h"
 
 struct sja1105_spi_message {
 	u64 access;
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 0663b78a2f6c..8bce1af13d53 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -1384,6 +1384,11 @@ static int sja1105_setup(struct dsa_switch *ds)
 		dev_err(ds->dev, "Failed to configure MII clocking: %d\n", rc);
 		return rc;
 	}
+	rc = sja1105_ptp_clock_register(priv);
+	if (rc < 0) {
+		dev_err(ds->dev, "Failed to register PTP clock: %d\n", rc);
+		return rc;
+	}
 	/* On SJA1105, VLAN filtering per se is always enabled in hardware.
 	 * The only thing we can do to disable it is lie about what the 802.1Q
 	 * EtherType is.
@@ -1521,6 +1526,7 @@ static const struct dsa_switch_ops sja1105_switch_ops = {
 	.get_strings		= sja1105_get_strings,
 	.get_ethtool_stats	= sja1105_get_ethtool_stats,
 	.get_sset_count		= sja1105_get_sset_count,
+	.get_ts_info		= sja1105_get_ts_info,
 	.port_fdb_dump		= sja1105_fdb_dump,
 	.port_fdb_add		= sja1105_fdb_add,
 	.port_fdb_del		= sja1105_fdb_del,
@@ -1645,6 +1651,7 @@ static int sja1105_remove(struct spi_device *spi)
 {
 	struct sja1105_private *priv = spi_get_drvdata(spi);
 
+	sja1105_ptp_clock_unregister(priv);
 	dsa_unregister_switch(priv->ds);
 	sja1105_static_config_free(&priv->static_config);
 	return 0;
diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.c b/drivers/net/dsa/sja1105/sja1105_ptp.c
new file mode 100644
index 000000000000..9c7f5d6a7e3c
--- /dev/null
+++ b/drivers/net/dsa/sja1105/sja1105_ptp.c
@@ -0,0 +1,293 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2019, Vladimir Oltean <olteanv@gmail.com>
+ */
+#include "sja1105.h"
+
+/* The adjfine API clamps ppb between [-32,768,000, 32,768,000], and
+ * therefore scaled_ppm between [-2,147,483,648, 2,147,483,647].
+ * Set the maximum supported ppb to a round value smaller than the maximum.
+ *
+ * Percentually speaking, this is a +/- 0.032x adjustment of the
+ * free-running counter (0.968x to 1.032x).
+ */
+#define SJA1105_MAX_ADJ_PPB		32000000
+#define SJA1105_SIZE_PTP_CMD		4
+
+/* Timestamps are in units of 8 ns clock ticks (equivalent to a fixed
+ * 125 MHz clock) so the scale factor (MULT / SHIFT) needs to be 8.
+ * Furthermore, wisely pick SHIFT as 28 bits, which translates
+ * MULT into 2^31 (0x80000000).  This is the same value around which
+ * the hardware PTPCLKRATE is centered, so the same ppb conversion
+ * arithmetic can be reused.
+ */
+#define SJA1105_CC_SHIFT		28
+#define SJA1105_CC_MULT			(8 << SJA1105_CC_SHIFT)
+
+/* Having 33 bits of cycle counter left until a 64-bit overflow during delta
+ * conversion, we multiply this by the 8 ns counter resolution and arrive at
+ * a comfortable 68.71 second refresh interval until the delta would cause
+ * an integer overflow, in absence of any other readout.
+ * Approximate to 1 minute.
+ */
+#define SJA1105_REFRESH_INTERVAL	(HZ * 60)
+
+/*            This range is actually +/- SJA1105_MAX_ADJ_PPB
+ *            divided by 1000 (ppb -> ppm) and with a 16-bit
+ *            "fractional" part (actually fixed point).
+ *                                    |
+ *                                    v
+ * Convert scaled_ppm from the +/- ((10^6) << 16) range
+ * into the +/- (1 << 31) range.
+ *
+ * This forgoes a "ppb" numeric representation (up to NSEC_PER_SEC)
+ * and defines the scaling factor between scaled_ppm and the actual
+ * frequency adjustments (both cycle counter and hardware).
+ *
+ *   ptpclkrate = scaled_ppm * 2^31 / (10^6 * 2^16)
+ *   simplifies to
+ *   ptpclkrate = scaled_ppm * 2^9 / 5^6
+ */
+#define SJA1105_CC_MULT_NUM			(1 << 9)
+#define SJA1105_CC_MULT_DEM			15625
+
+#define ptp_to_sja1105(d) container_of((d), struct sja1105_private, ptp_caps)
+#define cc_to_sja1105(d) container_of((d), struct sja1105_private, tstamp_cc)
+#define dw_to_sja1105(d) container_of((d), struct sja1105_private, refresh_work)
+
+struct sja1105_ptp_cmd {
+	u64 resptp;       /* reset */
+};
+
+int sja1105_get_ts_info(struct dsa_switch *ds, int port,
+			struct ethtool_ts_info *info)
+{
+	struct sja1105_private *priv = ds->priv;
+
+	/* Called during cleanup */
+	if (!priv->clock)
+		return -ENODEV;
+
+	info->so_timestamping = SOF_TIMESTAMPING_TX_HARDWARE |
+				SOF_TIMESTAMPING_RX_HARDWARE |
+				SOF_TIMESTAMPING_RAW_HARDWARE;
+	info->tx_types = (1 << HWTSTAMP_TX_OFF);
+	info->rx_filters = (1 << HWTSTAMP_FILTER_NONE);
+	info->phc_index = ptp_clock_index(priv->clock);
+	return 0;
+}
+
+int sja1105et_ptp_cmd(const void *ctx, const void *data)
+{
+	const struct sja1105_ptp_cmd *cmd = data;
+	const struct sja1105_private *priv = ctx;
+	const struct sja1105_regs *regs = priv->info->regs;
+	const int size = SJA1105_SIZE_PTP_CMD;
+	u8 buf[SJA1105_SIZE_PTP_CMD] = {0};
+	/* No need to keep this as part of the structure */
+	u64 valid = 1;
+
+	sja1105_pack(buf, &valid,           31, 31, size);
+	sja1105_pack(buf, &cmd->resptp,      2,  2, size);
+
+	return sja1105_spi_send_packed_buf(priv, SPI_WRITE, regs->ptp_control,
+					   buf, SJA1105_SIZE_PTP_CMD);
+}
+
+int sja1105pqrs_ptp_cmd(const void *ctx, const void *data)
+{
+	const struct sja1105_ptp_cmd *cmd = data;
+	const struct sja1105_private *priv = ctx;
+	const struct sja1105_regs *regs = priv->info->regs;
+	const int size = SJA1105_SIZE_PTP_CMD;
+	u8 buf[SJA1105_SIZE_PTP_CMD] = {0};
+	/* No need to keep this as part of the structure */
+	u64 valid = 1;
+
+	sja1105_pack(buf, &valid,           31, 31, size);
+	sja1105_pack(buf, &cmd->resptp,      3,  3, size);
+
+	return sja1105_spi_send_packed_buf(priv, SPI_WRITE, regs->ptp_control,
+					   buf, SJA1105_SIZE_PTP_CMD);
+}
+
+static int sja1105_ptp_reset(struct sja1105_private *priv)
+{
+	struct dsa_switch *ds = priv->ds;
+	struct sja1105_ptp_cmd cmd = {0};
+
+	cmd.resptp = 1;
+	dev_dbg(ds->dev, "Resetting PTP clock\n");
+	return priv->info->ptp_cmd(priv, &cmd);
+}
+
+static int sja1105_ptp_gettime(struct ptp_clock_info *ptp,
+			       struct timespec64 *ts)
+{
+	struct sja1105_private *priv = ptp_to_sja1105(ptp);
+	u64 ns;
+
+	mutex_lock(&priv->ptp_lock);
+	ns = timecounter_read(&priv->tstamp_tc);
+	mutex_unlock(&priv->ptp_lock);
+
+	*ts = ns_to_timespec64(ns);
+
+	return 0;
+}
+
+static int sja1105_ptp_settime(struct ptp_clock_info *ptp,
+			       const struct timespec64 *ts)
+{
+	struct sja1105_private *priv = ptp_to_sja1105(ptp);
+	u64 ns = timespec64_to_ns(ts);
+
+	mutex_lock(&priv->ptp_lock);
+	timecounter_init(&priv->tstamp_tc, &priv->tstamp_cc, ns);
+	mutex_unlock(&priv->ptp_lock);
+
+	return 0;
+}
+
+static int sja1105_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
+{
+	struct sja1105_private *priv = ptp_to_sja1105(ptp);
+	s64 clkrate;
+
+	clkrate = (s64)scaled_ppm * SJA1105_CC_MULT_NUM;
+	clkrate = div_s64(clkrate, SJA1105_CC_MULT_DEM);
+
+	mutex_lock(&priv->ptp_lock);
+
+	/* Force a readout to update the timer *before* changing its frequency.
+	 *
+	 * This way, its corrected time curve can at all times be modeled
+	 * as a linear "A * x + B" function, where:
+	 *
+	 * - B are past frequency adjustments and offset shifts, all
+	 *   accumulated into the cycle_last variable.
+	 *
+	 * - A is the new frequency adjustments we're just about to set.
+	 *
+	 * Reading now makes B accumulate the correct amount of time,
+	 * corrected at the old rate, before changing it.
+	 *
+	 * Hardware timestamps then become simple points on the curve and
+	 * are approximated using the above function.  This is still better
+	 * than letting the switch take the timestamps using the hardware
+	 * rate-corrected clock (PTPCLKVAL) - the comparison in this case would
+	 * be that we're shifting the ruler at the same time as we're taking
+	 * measurements with it.
+	 *
+	 * The disadvantage is that it's possible to receive timestamps when
+	 * a frequency adjustment took place in the near past.
+	 * In this case they will be approximated using the new ppb value
+	 * instead of a compound function made of two segments (one at the old
+	 * and the other at the new rate) - introducing some inaccuracy.
+	 */
+	timecounter_read(&priv->tstamp_tc);
+
+	priv->tstamp_cc.mult = SJA1105_CC_MULT + clkrate;
+
+	mutex_unlock(&priv->ptp_lock);
+
+	return 0;
+}
+
+static int sja1105_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
+{
+	struct sja1105_private *priv = ptp_to_sja1105(ptp);
+
+	mutex_lock(&priv->ptp_lock);
+	timecounter_adjtime(&priv->tstamp_tc, delta);
+	mutex_unlock(&priv->ptp_lock);
+
+	return 0;
+}
+
+static u64 sja1105_ptptsclk_read(const struct cyclecounter *cc)
+{
+	struct sja1105_private *priv = cc_to_sja1105(cc);
+	const struct sja1105_regs *regs = priv->info->regs;
+	u64 ptptsclk = 0;
+	int rc;
+
+	rc = sja1105_spi_send_int(priv, SPI_READ, regs->ptptsclk,
+				  &ptptsclk, 8);
+	if (rc < 0)
+		dev_err_ratelimited(priv->ds->dev,
+				    "failed to read ptp cycle counter: %d\n",
+				    rc);
+	return ptptsclk;
+}
+
+static void sja1105_ptp_overflow_check(struct work_struct *work)
+{
+	struct delayed_work *dw = to_delayed_work(work);
+	struct sja1105_private *priv = dw_to_sja1105(dw);
+	struct timespec64 ts;
+
+	sja1105_ptp_gettime(&priv->ptp_caps, &ts);
+
+	schedule_delayed_work(&priv->refresh_work, SJA1105_REFRESH_INTERVAL);
+}
+
+static const struct ptp_clock_info sja1105_ptp_caps = {
+	.owner		= THIS_MODULE,
+	.name		= "SJA1105 PHC",
+	.adjfine	= sja1105_ptp_adjfine,
+	.adjtime	= sja1105_ptp_adjtime,
+	.gettime64	= sja1105_ptp_gettime,
+	.settime64	= sja1105_ptp_settime,
+	.max_adj	= SJA1105_MAX_ADJ_PPB,
+};
+
+static int sja1105_ptp_init(struct sja1105_private *priv)
+{
+	/* SJA1105 E/T partial timestamps wrap around in
+	 * 0.135 seconds, P/Q/R/S in 34.35 seconds
+	 */
+	u64 partial_tstamp_mask = CYCLECOUNTER_MASK(priv->info->ptp_ts_bits);
+
+	/* Set up the cycle counter */
+	priv->tstamp_cc = (struct cyclecounter) {
+		.read = sja1105_ptptsclk_read,
+		.mask = CYCLECOUNTER_MASK(64),
+		.shift = SJA1105_CC_SHIFT,
+		.mult = SJA1105_CC_MULT,
+	};
+	timecounter_init(&priv->tstamp_tc, &priv->tstamp_cc,
+			 ktime_to_ns(ktime_get_real()));
+	mutex_init(&priv->ptp_lock);
+	INIT_DELAYED_WORK(&priv->refresh_work, sja1105_ptp_overflow_check);
+
+	schedule_delayed_work(&priv->refresh_work, SJA1105_REFRESH_INTERVAL);
+
+	return 0;
+}
+
+int sja1105_ptp_clock_register(struct sja1105_private *priv)
+{
+	struct dsa_switch *ds = priv->ds;
+	int rc;
+
+	priv->ptp_caps = sja1105_ptp_caps;
+
+	priv->clock = ptp_clock_register(&priv->ptp_caps, ds->dev);
+	if (IS_ERR_OR_NULL(priv->clock))
+		return PTR_ERR(priv->clock);
+
+	rc = sja1105_ptp_reset(priv);
+	if (rc < 0)
+		return -EIO;
+
+	return sja1105_ptp_init(priv);
+}
+
+void sja1105_ptp_clock_unregister(struct sja1105_private *priv)
+{
+	if (IS_ERR_OR_NULL(priv->clock))
+		return;
+
+	ptp_clock_unregister(priv->clock);
+	priv->clock = NULL;
+}
diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.h b/drivers/net/dsa/sja1105/sja1105_ptp.h
new file mode 100644
index 000000000000..d2c6cf273500
--- /dev/null
+++ b/drivers/net/dsa/sja1105/sja1105_ptp.h
@@ -0,0 +1,40 @@
+/* SPDX-License-Identifier: GPL-2.0
+ * Copyright (c) 2019, Vladimir Oltean <olteanv@gmail.com>
+ */
+#ifndef _SJA1105_PTP_H
+#define _SJA1105_PTP_H
+
+#if IS_ENABLED(CONFIG_NET_DSA_SJA1105_PTP)
+
+int sja1105_ptp_clock_register(struct sja1105_private *priv);
+
+void sja1105_ptp_clock_unregister(struct sja1105_private *priv);
+
+int sja1105et_ptp_cmd(const void *ctx, const void *data);
+
+int sja1105pqrs_ptp_cmd(const void *ctx, const void *data);
+
+int sja1105_get_ts_info(struct dsa_switch *ds, int port,
+			struct ethtool_ts_info *ts);
+
+#else
+
+static inline int sja1105_ptp_clock_register(struct sja1105_private *priv)
+{
+	return 0;
+}
+
+static inline void sja1105_ptp_clock_unregister(struct sja1105_private *priv)
+{
+	return;
+}
+
+#define sja1105et_ptp_cmd NULL
+
+#define sja1105pqrs_ptp_cmd NULL
+
+#define sja1105_get_ts_info NULL
+
+#endif /* IS_ENABLED(CONFIG_NET_DSA_SJA1105_PTP) */
+
+#endif /* _SJA1105_PTP_H */
diff --git a/drivers/net/dsa/sja1105/sja1105_spi.c b/drivers/net/dsa/sja1105/sja1105_spi.c
index 2eb70b8acfc3..f30ac8326c98 100644
--- a/drivers/net/dsa/sja1105/sja1105_spi.c
+++ b/drivers/net/dsa/sja1105/sja1105_spi.c
@@ -507,6 +507,11 @@ static struct sja1105_regs sja1105et_regs = {
 	.rgmii_tx_clk = {0x100016, 0x10001D, 0x100024, 0x10002B, 0x100032},
 	.rmii_ref_clk = {0x100015, 0x10001C, 0x100023, 0x10002A, 0x100031},
 	.rmii_ext_tx_clk = {0x100018, 0x10001F, 0x100026, 0x10002D, 0x100034},
+	.ptpegr_ts = 0xC0,
+	.ptp_control = 0x17,
+	.ptpclk = 0x18, /* Spans 0x18 to 0x19 */
+	.ptpclkrate = 0x1A,
+	.ptptsclk = 0x1B, /* Spans 0x1B to 0x1C */
 };
 
 static struct sja1105_regs sja1105pqrs_regs = {
@@ -533,6 +538,11 @@ static struct sja1105_regs sja1105pqrs_regs = {
 	.rmii_ref_clk = {0x100015, 0x10001B, 0x100021, 0x100027, 0x10002D},
 	.rmii_ext_tx_clk = {0x100017, 0x10001D, 0x100023, 0x100029, 0x10002F},
 	.qlevel = {0x604, 0x614, 0x624, 0x634, 0x644},
+	.ptpegr_ts = 0xC0,
+	.ptp_control = 0x18,
+	.ptpclk = 0x19,
+	.ptpclkrate = 0x1B,
+	.ptptsclk = 0x1C,
 };
 
 struct sja1105_info sja1105e_info = {
@@ -541,6 +551,7 @@ struct sja1105_info sja1105e_info = {
 	.static_ops		= sja1105e_table_ops,
 	.dyn_ops		= sja1105et_dyn_ops,
 	.reset_cmd		= sja1105et_reset_cmd,
+	.ptp_cmd		= sja1105et_ptp_cmd,
 	.regs			= &sja1105et_regs,
 	.name			= "SJA1105E",
 };
@@ -550,6 +561,7 @@ struct sja1105_info sja1105t_info = {
 	.static_ops		= sja1105t_table_ops,
 	.dyn_ops		= sja1105et_dyn_ops,
 	.reset_cmd		= sja1105et_reset_cmd,
+	.ptp_cmd		= sja1105et_ptp_cmd,
 	.regs			= &sja1105et_regs,
 	.name			= "SJA1105T",
 };
@@ -559,6 +571,7 @@ struct sja1105_info sja1105p_info = {
 	.static_ops		= sja1105p_table_ops,
 	.dyn_ops		= sja1105pqrs_dyn_ops,
 	.reset_cmd		= sja1105pqrs_reset_cmd,
+	.ptp_cmd		= sja1105pqrs_ptp_cmd,
 	.regs			= &sja1105pqrs_regs,
 	.name			= "SJA1105P",
 };
@@ -568,6 +581,7 @@ struct sja1105_info sja1105q_info = {
 	.static_ops		= sja1105q_table_ops,
 	.dyn_ops		= sja1105pqrs_dyn_ops,
 	.reset_cmd		= sja1105pqrs_reset_cmd,
+	.ptp_cmd		= sja1105pqrs_ptp_cmd,
 	.regs			= &sja1105pqrs_regs,
 	.name			= "SJA1105Q",
 };
@@ -577,6 +591,7 @@ struct sja1105_info sja1105r_info = {
 	.static_ops		= sja1105r_table_ops,
 	.dyn_ops		= sja1105pqrs_dyn_ops,
 	.reset_cmd		= sja1105pqrs_reset_cmd,
+	.ptp_cmd		= sja1105pqrs_ptp_cmd,
 	.regs			= &sja1105pqrs_regs,
 	.name			= "SJA1105R",
 };
@@ -587,5 +602,6 @@ struct sja1105_info sja1105s_info = {
 	.dyn_ops		= sja1105pqrs_dyn_ops,
 	.regs			= &sja1105pqrs_regs,
 	.reset_cmd		= sja1105pqrs_reset_cmd,
+	.ptp_cmd		= sja1105pqrs_ptp_cmd,
 	.name			= "SJA1105S",
 };
-- 
2.17.1

[PATCH net-next 3/5] net: dsa: mv88e6xxx: Let taggers specify a can_timestamp function

[Vladimir Oltean]

The newly introduced function is called on both the RX and TX paths.

The boolean returned by port_txtstamp should only return false if the
driver tried to timestamp the skb but failed.

Currently there is some logic in the mv88e6xxx driver that determines
whether it should timestamp frames or not.

This is wasteful, because if the decision is to not timestamp them, then
DSA will have cloned an skb and freed it immediately afterwards.

Additionally other drivers (sja1105) may have other hardware criteria
for timestamping frames on RX, and the default conditions for
timestamping a frame are too restrictive.  When RX timestamping is
enabled, the sja1105 hardware emits a follow-up frame containing the
timestamp for every trapped link-local frame.  Then the link-local frame
is queued up inside the port_rxtstamp callback where it waits for its
follow-up meta frame to come.  But only a subset of the link-local
frames will pass through DSA's default filter for port_rxtstamp, so the
rest of the link-local traffic would still receive a meta frame but
would not get timestamped.

Since the state machine of waiting for meta frames is implemented in the
tagger rcv function for sja1105, it is difficult to know which frames
will pass through DSA's later filter and which won't.  And since
timestamping more frames than just PTP does no harm, just implement a
callback for sja1105 that will say that all link-local traffic will be
timestamped on RX.

PTP classification on the skb is still performed.  But now it is saved
to the DSA_SKB_CB, so drivers can reuse it without calling it again.

The mv88e6xxx driver was also modified to use the new generic
DSA_SKB_CB(skb)->ptp_type instead of its own, custom SKB_PTP_TYPE(skb).

Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/hwtstamp.c | 25 +++++++++----------------
 drivers/net/dsa/mv88e6xxx/hwtstamp.h |  4 ++--
 include/net/dsa.h                    |  6 ++++--
 net/dsa/dsa.c                        | 25 +++++++++++++++----------
 net/dsa/slave.c                      | 20 ++++++++++++++------
 5 files changed, 44 insertions(+), 36 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.c b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
index a17c16a2ab78..3295ad10818f 100644
--- a/drivers/net/dsa/mv88e6xxx/hwtstamp.c
+++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
@@ -20,8 +20,6 @@
 #include "ptp.h"
 #include <linux/ptp_classify.h>
 
-#define SKB_PTP_TYPE(__skb) (*(unsigned int *)((__skb)->cb))
-
 static int mv88e6xxx_port_ptp_read(struct mv88e6xxx_chip *chip, int port,
 				   int addr, u16 *data, int len)
 {
@@ -216,8 +214,9 @@ int mv88e6xxx_port_hwtstamp_get(struct dsa_switch *ds, int port,
 }
 
 /* Get the start of the PTP header in this skb */
-static u8 *parse_ptp_header(struct sk_buff *skb, unsigned int type)
+static u8 *parse_ptp_header(struct sk_buff *skb)
 {
+	unsigned int type = DSA_SKB_CB(skb)->ptp_type;
 	u8 *data = skb_mac_header(skb);
 	unsigned int offset = 0;
 
@@ -249,7 +248,7 @@ static u8 *parse_ptp_header(struct sk_buff *skb, unsigned int type)
  * or NULL if the caller should not.
  */
 static u8 *mv88e6xxx_should_tstamp(struct mv88e6xxx_chip *chip, int port,
-				   struct sk_buff *skb, unsigned int type)
+				   struct sk_buff *skb)
 {
 	struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[port];
 	u8 *hdr;
@@ -257,7 +256,7 @@ static u8 *mv88e6xxx_should_tstamp(struct mv88e6xxx_chip *chip, int port,
 	if (!chip->info->ptp_support)
 		return NULL;
 
-	hdr = parse_ptp_header(skb, type);
+	hdr = parse_ptp_header(skb);
 	if (!hdr)
 		return NULL;
 
@@ -278,8 +277,7 @@ static int mv88e6xxx_ts_valid(u16 status)
 
 static int seq_match(struct sk_buff *skb, u16 ts_seqid)
 {
-	unsigned int type = SKB_PTP_TYPE(skb);
-	u8 *hdr = parse_ptp_header(skb, type);
+	u8 *hdr = parse_ptp_header(skb);
 	__be16 *seqid;
 
 	seqid = (__be16 *)(hdr + OFF_PTP_SEQUENCE_ID);
@@ -367,7 +365,7 @@ static int is_pdelay_resp(u8 *msgtype)
 }
 
 bool mv88e6xxx_port_rxtstamp(struct dsa_switch *ds, int port,
-			     struct sk_buff *skb, unsigned int type)
+			     struct sk_buff *skb)
 {
 	struct mv88e6xxx_port_hwtstamp *ps;
 	struct mv88e6xxx_chip *chip;
@@ -379,12 +377,10 @@ bool mv88e6xxx_port_rxtstamp(struct dsa_switch *ds, int port,
 	if (ps->tstamp_config.rx_filter != HWTSTAMP_FILTER_PTP_V2_EVENT)
 		return false;
 
-	hdr = mv88e6xxx_should_tstamp(chip, port, skb, type);
+	hdr = mv88e6xxx_should_tstamp(chip, port, skb);
 	if (!hdr)
 		return false;
 
-	SKB_PTP_TYPE(skb) = type;
-
 	if (is_pdelay_resp(hdr))
 		skb_queue_tail(&ps->rx_queue2, skb);
 	else
@@ -503,17 +499,14 @@ long mv88e6xxx_hwtstamp_work(struct ptp_clock_info *ptp)
 }
 
 bool mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
-			     struct sk_buff *clone, unsigned int type)
+			     struct sk_buff *clone)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
 	struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[port];
 	__be16 *seq_ptr;
 	u8 *hdr;
 
-	if (!(skb_shinfo(clone)->tx_flags & SKBTX_HW_TSTAMP))
-		return false;
-
-	hdr = mv88e6xxx_should_tstamp(chip, port, clone, type);
+	hdr = mv88e6xxx_should_tstamp(chip, port, clone);
 	if (!hdr)
 		return false;
 
diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.h b/drivers/net/dsa/mv88e6xxx/hwtstamp.h
index b9a72661bcc4..17caf374332b 100644
--- a/drivers/net/dsa/mv88e6xxx/hwtstamp.h
+++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.h
@@ -120,9 +120,9 @@ int mv88e6xxx_port_hwtstamp_get(struct dsa_switch *ds, int port,
 				struct ifreq *ifr);
 
 bool mv88e6xxx_port_rxtstamp(struct dsa_switch *ds, int port,
-			     struct sk_buff *clone, unsigned int type);
+			     struct sk_buff *clone);
 bool mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
-			     struct sk_buff *clone, unsigned int type);
+			     struct sk_buff *clone);
 
 int mv88e6xxx_get_ts_info(struct dsa_switch *ds, int port,
 			  struct ethtool_ts_info *info);
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 685294817712..2fd844688f83 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -76,6 +76,7 @@ struct dsa_device_ops {
 	 * as regular on the master net device.
 	 */
 	bool (*filter)(const struct sk_buff *skb, struct net_device *dev);
+	bool (*can_tstamp)(const struct sk_buff *skb, struct net_device *dev);
 	unsigned int overhead;
 	const char *name;
 	enum dsa_tag_protocol proto;
@@ -87,6 +88,7 @@ struct dsa_device_ops {
 
 struct dsa_skb_cb {
 	struct sk_buff *clone;
+	unsigned int ptp_type;
 	bool deferred_xmit;
 };
 
@@ -534,9 +536,9 @@ struct dsa_switch_ops {
 	int	(*port_hwtstamp_set)(struct dsa_switch *ds, int port,
 				     struct ifreq *ifr);
 	bool	(*port_txtstamp)(struct dsa_switch *ds, int port,
-				 struct sk_buff *clone, unsigned int type);
+				 struct sk_buff *clone);
 	bool	(*port_rxtstamp)(struct dsa_switch *ds, int port,
-				 struct sk_buff *skb, unsigned int type);
+				 struct sk_buff *skb);
 
 	/*
 	 * Deferred frame Tx
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 1fc782fab393..ee5606412e2e 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -181,27 +181,32 @@ EXPORT_SYMBOL_GPL(dsa_dev_to_net_device);
  * delivered is never notified unless we do so here.
  */
 static bool dsa_skb_defer_rx_timestamp(struct dsa_slave_priv *p,
-				       struct sk_buff *skb)
+				       struct sk_buff *skb,
+				       struct net_device *dev)
 {
 	struct dsa_switch *ds = p->dp->ds;
-	unsigned int type;
+
+	if (!ds->ops->port_rxtstamp)
+		return false;
 
 	if (skb_headroom(skb) < ETH_HLEN)
 		return false;
 
 	__skb_push(skb, ETH_HLEN);
 
-	type = ptp_classify_raw(skb);
+	DSA_SKB_CB(skb)->ptp_type = ptp_classify_raw(skb);
 
 	__skb_pull(skb, ETH_HLEN);
 
-	if (type == PTP_CLASS_NONE)
-		return false;
-
-	if (likely(ds->ops->port_rxtstamp))
-		return ds->ops->port_rxtstamp(ds, p->dp->index, skb, type);
+	if (p->dp->cpu_dp->tag_ops->can_tstamp) {
+		if (!p->dp->cpu_dp->tag_ops->can_tstamp(skb, dev))
+			return false;
+	} else {
+		if (DSA_SKB_CB(skb)->ptp_type == PTP_CLASS_NONE)
+			return false;
+	}
 
-	return false;
+	return ds->ops->port_rxtstamp(ds, p->dp->index, skb);
 }
 
 static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
@@ -239,7 +244,7 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
 	s->rx_bytes += skb->len;
 	u64_stats_update_end(&s->syncp);
 
-	if (dsa_skb_defer_rx_timestamp(p, skb))
+	if (dsa_skb_defer_rx_timestamp(p, skb, dev))
 		return 0;
 
 	netif_receive_skb(skb);
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 9892ca1f6859..ebe7944a2d0f 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -410,24 +410,32 @@ static inline netdev_tx_t dsa_slave_netpoll_send_skb(struct net_device *dev,
 }
 
 static void dsa_skb_tx_timestamp(struct dsa_slave_priv *p,
-				 struct sk_buff *skb)
+				 struct sk_buff *skb, struct net_device *dev)
 {
 	struct dsa_switch *ds = p->dp->ds;
 	struct sk_buff *clone;
-	unsigned int type;
 
-	type = ptp_classify_raw(skb);
-	if (type == PTP_CLASS_NONE)
+	if (!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
 		return;
 
 	if (!ds->ops->port_txtstamp)
 		return;
 
+	DSA_SKB_CB(skb)->ptp_type = ptp_classify_raw(skb);
+
+	if (p->dp->cpu_dp->tag_ops->can_tstamp) {
+		if (!p->dp->cpu_dp->tag_ops->can_tstamp(skb, dev))
+			return;
+	} else {
+		if (DSA_SKB_CB(skb)->ptp_type == PTP_CLASS_NONE)
+			return;
+	}
+
 	clone = skb_clone_sk(skb);
 	if (!clone)
 		return;
 
-	if (ds->ops->port_txtstamp(ds, p->dp->index, clone, type))
+	if (ds->ops->port_txtstamp(ds, p->dp->index, clone))
 		return;
 
 	kfree_skb(clone);
@@ -468,7 +476,7 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
 	/* Identify PTP protocol packets, clone them, and pass them to the
 	 * switch driver
 	 */
-	dsa_skb_tx_timestamp(p, skb);
+	dsa_skb_tx_timestamp(p, skb, dev);
 
 	/* Transmit function may have to reallocate the original SKB,
 	 * in which case it must have freed it. Only free it here on error.
-- 
2.17.1

Re: [PATCH net-next 3/5] net: dsa: mv88e6xxx: Let taggers specify a can_timestamp function

[Richard Cochran]

On Wed, May 29, 2019 at 02:56:25AM +0300, Vladimir Oltean wrote:
> The newly introduced function is called on both the RX and TX paths.

NAK on this patch.
 
> The boolean returned by port_txtstamp should only return false if the
> driver tried to timestamp the skb but failed.

So you say.
 
> Currently there is some logic in the mv88e6xxx driver that determines
> whether it should timestamp frames or not.
> 
> This is wasteful, because if the decision is to not timestamp them, then
> DSA will have cloned an skb and freed it immediately afterwards.

No, it isn't wasteful.  Look at the tests in that driver to see why.
 
> Additionally other drivers (sja1105) may have other hardware criteria
> for timestamping frames on RX, and the default conditions for
> timestamping a frame are too restrictive.

I'm sorry, but we won't change the frame just for one device that has
design issues.

Please put device specific workarounds into its driver.

Thanks,
Richard

Re: [PATCH net-next 3/5] net: dsa: mv88e6xxx: Let taggers specify a can_timestamp function

[Vladimir Oltean]

On Wed, 29 May 2019 at 07:49, Richard Cochran <richardcochran@gmail.com> wrote:
>
> On Wed, May 29, 2019 at 02:56:25AM +0300, Vladimir Oltean wrote:
> > The newly introduced function is called on both the RX and TX paths.
>
> NAK on this patch.
>
> > The boolean returned by port_txtstamp should only return false if the
> > driver tried to timestamp the skb but failed.
>
> So you say.
>
> > Currently there is some logic in the mv88e6xxx driver that determines
> > whether it should timestamp frames or not.
> >
> > This is wasteful, because if the decision is to not timestamp them, then
> > DSA will have cloned an skb and freed it immediately afterwards.
>
> No, it isn't wasteful.  Look at the tests in that driver to see why.
>
> > Additionally other drivers (sja1105) may have other hardware criteria
> > for timestamping frames on RX, and the default conditions for
> > timestamping a frame are too restrictive.
>
> I'm sorry, but we won't change the frame just for one device that has
> design issues.
>
> Please put device specific workarounds into its driver.
>
> Thanks,
> Richard

Hi Richard,

I removed this patch and my RX timestamping path still works, apparently.
It's just that now I'm not holding up in the RX timestamping queue
anything else except what the PTP classifier requested me to.
What I'm concerned about is that I'm using the skb->cb as a
communication space between the tagger waiting for the meta frame, and
the RX timestamping queue waiting to be notified that the meta frame
arrived.
If I'm not holding up all frames that I know will have a follow-up
come after them, then I'm letting them free up the stack, and who
knows whose skb->cb the tagger will overwrite.
By asking me to remove this patch you're basically asking the state
machine inside the tagger to guess whether the previous frame is one
that DSA cares about w.r.t. RX timestamping, and based on that info
write to its skb->cb or not.
I would like to avoid keeping meta frames in their own RX queue,
because then I'm complicating (or rather put, making impossible) the
association between a meta frame and the frame it holds a timestamp
of.

Regards,
-Vladimir

Re: [PATCH net-next 3/5] net: dsa: mv88e6xxx: Let taggers specify a can_timestamp function

[Richard Cochran]

On Wed, May 29, 2019 at 11:33:31PM +0300, Vladimir Oltean wrote:
> I would like to avoid keeping meta frames in their own RX queue,
> because then I'm complicating (or rather put, making impossible) the
> association between a meta frame and the frame it holds a timestamp
> of.

We have an example of how a driver can match meta time stamp packets
with received packets.  See drivers/net/phy/dp83640.c to see how it
can be done completely within the driver.

Thanks,
Richard

Re: [PATCH net-next 3/5] net: dsa: mv88e6xxx: Let taggers specify a can_timestamp function

[Vladimir Oltean]

On Thu, 30 May 2019 at 06:51, Richard Cochran <richardcochran@gmail.com> wrote:
>
> On Wed, May 29, 2019 at 11:33:31PM +0300, Vladimir Oltean wrote:
> > I would like to avoid keeping meta frames in their own RX queue,
> > because then I'm complicating (or rather put, making impossible) the
> > association between a meta frame and the frame it holds a timestamp
> > of.
>
> We have an example of how a driver can match meta time stamp packets
> with received packets.  See drivers/net/phy/dp83640.c to see how it
> can be done completely within the driver.
>
> Thanks,
> Richard

The meta frames generated by the SJA1105 do not contain any seqid.
They contain:
* A globally programmable DMAC
* A globally programmable SMAC
* The 0x8 EtherType
* A partial (24-bit or 32-bit) RX timestamp
* Two bytes from the initial (pre follow-up) frame's DMAC, before the
switch mangled those with the source port and switch id. The driver is
supposed to patch these bytes from the follow-up back into the initial
frame before passing them up the stack.
* The source port that generated the meta frame
* The switch id that generated the meta frame

Regards,
-Vladimir

Re: [PATCH net-next 3/5] net: dsa: mv88e6xxx: Let taggers specify a can_timestamp function

[Richard Cochran]

On Thu, May 30, 2019 at 10:42:41AM +0300, Vladimir Oltean wrote:
> The meta frames generated by the SJA1105 do not contain any seqid.

So this cannot ever work...

> They contain:
> * A globally programmable DMAC
> * A globally programmable SMAC

Don't know what these are, but doesn't sound like they uniquely
identify the original frame.

> * The 0x8 EtherType
> * A partial (24-bit or 32-bit) RX timestamp
> * Two bytes from the initial (pre follow-up) frame's DMAC, before the
> switch mangled those with the source port and switch id. The driver is
> supposed to patch these bytes from the follow-up back into the initial
> frame before passing them up the stack.
> * The source port that generated the meta frame
> * The switch id that generated the meta frame

None of these match to the original frame uniquely.  Looks like this
is a dead end.

I recommend forgetting about these meta frames.  Instead, read out the
time stamps over MDIO.

Thanks,
Richard

Re: [PATCH net-next 3/5] net: dsa: mv88e6xxx: Let taggers specify a can_timestamp function

[Richard Cochran]

On Thu, May 30, 2019 at 07:23:56AM -0700, Richard Cochran wrote:
> I recommend forgetting about these meta frames.  Instead, read out the
> time stamps over MDIO.

Or SPI.  It appears you use that for Tx time stamps already.
 
Thanks,
Richard

Re: [PATCH net-next 3/5] net: dsa: mv88e6xxx: Let taggers specify a can_timestamp function

[Vladimir Oltean]

On Thu, 30 May 2019 at 17:24, Richard Cochran <richardcochran@gmail.com> wrote:
>
> On Thu, May 30, 2019 at 10:42:41AM +0300, Vladimir Oltean wrote:
> > The meta frames generated by the SJA1105 do not contain any seqid.
>
> So this cannot ever work...
>
> > They contain:
> > * A globally programmable DMAC
> > * A globally programmable SMAC
>
> Don't know what these are, but doesn't sound like they uniquely
> identify the original frame.
>
> > * The 0x8 EtherType
> > * A partial (24-bit or 32-bit) RX timestamp
> > * Two bytes from the initial (pre follow-up) frame's DMAC, before the
> > switch mangled those with the source port and switch id. The driver is
> > supposed to patch these bytes from the follow-up back into the initial
> > frame before passing them up the stack.
> > * The source port that generated the meta frame
> > * The switch id that generated the meta frame
>
> None of these match to the original frame uniquely.  Looks like this
> is a dead end.
>

Yes, they don't identify the original frame.
The hardware's line of thinking seems to be "The meta frame is sent
immediately after the trapped frame that triggered the action." (quote
from https://www.nxp.com/docs/en/user-guide/UM10944.pdf).

> I recommend forgetting about these meta frames.  Instead, read out the
> time stamps over MDIO.
>

If there was any other way to retrieve RX timestamps I would have done
it already.

> Thanks,
> Richard

Re: [PATCH net-next 3/5] net: dsa: mv88e6xxx: Let taggers specify a can_timestamp function

[Richard Cochran]

On Thu, May 30, 2019 at 05:47:55PM +0300, Vladimir Oltean wrote:
> Yes, they don't identify the original frame.
> The hardware's line of thinking seems to be "The meta frame is sent
> immediately after the trapped frame that triggered the action." (quote
> from https://www.nxp.com/docs/en/user-guide/UM10944.pdf).

Hm.

> If there was any other way to retrieve RX timestamps I would have done
> it already.

So it cannot even possibly work.
 
Sorry,
Richard

Design issue in DSA RX timestamping (Was "Re: [PATCH net-next 3/5] net: dsa: mv88e6xxx: Let taggers specify a can_timestamp function")

[Vladimir Oltean]

On Wed, 29 May 2019 at 02:58, Vladimir Oltean <olteanv@gmail.com> wrote:
>
> The newly introduced function is called on both the RX and TX paths.
>
> The boolean returned by port_txtstamp should only return false if the
> driver tried to timestamp the skb but failed.
>
> Currently there is some logic in the mv88e6xxx driver that determines
> whether it should timestamp frames or not.
>
> This is wasteful, because if the decision is to not timestamp them, then
> DSA will have cloned an skb and freed it immediately afterwards.
>
> Additionally other drivers (sja1105) may have other hardware criteria
> for timestamping frames on RX, and the default conditions for
> timestamping a frame are too restrictive.  When RX timestamping is
> enabled, the sja1105 hardware emits a follow-up frame containing the
> timestamp for every trapped link-local frame.  Then the link-local frame
> is queued up inside the port_rxtstamp callback where it waits for its
> follow-up meta frame to come.  But only a subset of the link-local
> frames will pass through DSA's default filter for port_rxtstamp, so the
> rest of the link-local traffic would still receive a meta frame but
> would not get timestamped.
>
> Since the state machine of waiting for meta frames is implemented in the
> tagger rcv function for sja1105, it is difficult to know which frames
> will pass through DSA's later filter and which won't.  And since
> timestamping more frames than just PTP does no harm, just implement a
> callback for sja1105 that will say that all link-local traffic will be
> timestamped on RX.
>
> PTP classification on the skb is still performed.  But now it is saved
> to the DSA_SKB_CB, so drivers can reuse it without calling it again.
>
> The mv88e6xxx driver was also modified to use the new generic
> DSA_SKB_CB(skb)->ptp_type instead of its own, custom SKB_PTP_TYPE(skb).
>
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> ---

Hi Richard,

I've been battling some issues with sja1105 PTP after you suggested me
to do this rework and eliminate the .can_tstamp callback.

Forget my reasoning above, I've done more digging and I think I have
better arguments now. Let me try to explain a catch-22:

- DSA doesn't let drivers to select which frames should be
timestamped. It knows better.

- To be precise, it runs the PTP BPF classifier and filters only the
SYNC (but not FOLLOW-UP) messages. In principle I agree, the FOLLOW-UP
frames are general messages and don't need to be timestamped for the
PTP protocol to work. But by that logic, the HWTSTAMP_FILTER_ALL
rx_filter shouldn't exist? See this commit message:

commit f75159e9936143177b442afc78150b7a7ad8aa07
Author: Richard Cochran <richardcochran@gmail.com>
Date:   Tue Sep 20 01:25:41 2011 +0000

    ptp: fix L2 event message recognition

    The IEEE 1588 standard defines two kinds of messages, event and general
    messages. Event messages require time stamping, and general do not. When
    using UDP transport, two separate ports are used for the two message
    types.

    The BPF designed to recognize event messages incorrectly classifies L2
    general messages as event messages. This commit fixes the issue by
    extending the filter to check the message type field for L2 PTP packets.
    Event messages are be distinguished from general messages by testing
    the "general" bit.

    Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
    Cc: <stable@kernel.org>
    Signed-off-by: David S. Miller <davem@davemloft.net>

- Because it treats SYNC and FOLLOW-UP frames differently on the RX
path, it gives them a chance to get reordered. It doesn't give the
driver a chance to avoid the reordering. Both DSA drivers that support
PTP at the moment (sja1105 and mv88e6xxx) schedule a workqueue in the
.port_rxtstamp callback to collect the RX timestamp of the SYNC frame.
So the SYNC frame will get delayed, but the FOLLOW-UP will return as
false from dsa_skb_defer_rx_timestamp and be delivered immediately up
the stack.

- Without fully understanding what happens, I tried to propose better
logic for recovering reordered SYNC/FOLLOW-UP pairs at the application
level in linuxptp [1]. Needless to say, the consensus was to fix the
kernel. So here we are.

[1] https://sourceforge.net/p/linuxptp/mailman/linuxptp-devel/thread/20190928123414.9422-1-olteanv%40gmail.com/#msg36773629

>  drivers/net/dsa/mv88e6xxx/hwtstamp.c | 25 +++++++++----------------
>  drivers/net/dsa/mv88e6xxx/hwtstamp.h |  4 ++--
>  include/net/dsa.h                    |  6 ++++--
>  net/dsa/dsa.c                        | 25 +++++++++++++++----------
>  net/dsa/slave.c                      | 20 ++++++++++++++------
>  5 files changed, 44 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.c b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
> index a17c16a2ab78..3295ad10818f 100644
> --- a/drivers/net/dsa/mv88e6xxx/hwtstamp.c
> +++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
> @@ -20,8 +20,6 @@
>  #include "ptp.h"
>  #include <linux/ptp_classify.h>
>
> -#define SKB_PTP_TYPE(__skb) (*(unsigned int *)((__skb)->cb))
> -
>  static int mv88e6xxx_port_ptp_read(struct mv88e6xxx_chip *chip, int port,
>                                    int addr, u16 *data, int len)
>  {
> @@ -216,8 +214,9 @@ int mv88e6xxx_port_hwtstamp_get(struct dsa_switch *ds, int port,
>  }
>
>  /* Get the start of the PTP header in this skb */
> -static u8 *parse_ptp_header(struct sk_buff *skb, unsigned int type)
> +static u8 *parse_ptp_header(struct sk_buff *skb)
>  {
> +       unsigned int type = DSA_SKB_CB(skb)->ptp_type;
>         u8 *data = skb_mac_header(skb);
>         unsigned int offset = 0;
>
> @@ -249,7 +248,7 @@ static u8 *parse_ptp_header(struct sk_buff *skb, unsigned int type)
>   * or NULL if the caller should not.
>   */
>  static u8 *mv88e6xxx_should_tstamp(struct mv88e6xxx_chip *chip, int port,
> -                                  struct sk_buff *skb, unsigned int type)
> +                                  struct sk_buff *skb)
>  {
>         struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[port];
>         u8 *hdr;
> @@ -257,7 +256,7 @@ static u8 *mv88e6xxx_should_tstamp(struct mv88e6xxx_chip *chip, int port,
>         if (!chip->info->ptp_support)
>                 return NULL;
>
> -       hdr = parse_ptp_header(skb, type);
> +       hdr = parse_ptp_header(skb);
>         if (!hdr)
>                 return NULL;
>
> @@ -278,8 +277,7 @@ static int mv88e6xxx_ts_valid(u16 status)
>
>  static int seq_match(struct sk_buff *skb, u16 ts_seqid)
>  {
> -       unsigned int type = SKB_PTP_TYPE(skb);
> -       u8 *hdr = parse_ptp_header(skb, type);
> +       u8 *hdr = parse_ptp_header(skb);
>         __be16 *seqid;
>
>         seqid = (__be16 *)(hdr + OFF_PTP_SEQUENCE_ID);
> @@ -367,7 +365,7 @@ static int is_pdelay_resp(u8 *msgtype)
>  }
>
>  bool mv88e6xxx_port_rxtstamp(struct dsa_switch *ds, int port,
> -                            struct sk_buff *skb, unsigned int type)
> +                            struct sk_buff *skb)
>  {
>         struct mv88e6xxx_port_hwtstamp *ps;
>         struct mv88e6xxx_chip *chip;
> @@ -379,12 +377,10 @@ bool mv88e6xxx_port_rxtstamp(struct dsa_switch *ds, int port,
>         if (ps->tstamp_config.rx_filter != HWTSTAMP_FILTER_PTP_V2_EVENT)
>                 return false;
>
> -       hdr = mv88e6xxx_should_tstamp(chip, port, skb, type);
> +       hdr = mv88e6xxx_should_tstamp(chip, port, skb);
>         if (!hdr)
>                 return false;
>
> -       SKB_PTP_TYPE(skb) = type;
> -
>         if (is_pdelay_resp(hdr))
>                 skb_queue_tail(&ps->rx_queue2, skb);
>         else
> @@ -503,17 +499,14 @@ long mv88e6xxx_hwtstamp_work(struct ptp_clock_info *ptp)
>  }
>
>  bool mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
> -                            struct sk_buff *clone, unsigned int type)
> +                            struct sk_buff *clone)
>  {
>         struct mv88e6xxx_chip *chip = ds->priv;
>         struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[port];
>         __be16 *seq_ptr;
>         u8 *hdr;
>
> -       if (!(skb_shinfo(clone)->tx_flags & SKBTX_HW_TSTAMP))
> -               return false;
> -
> -       hdr = mv88e6xxx_should_tstamp(chip, port, clone, type);
> +       hdr = mv88e6xxx_should_tstamp(chip, port, clone);
>         if (!hdr)
>                 return false;
>
> diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.h b/drivers/net/dsa/mv88e6xxx/hwtstamp.h
> index b9a72661bcc4..17caf374332b 100644
> --- a/drivers/net/dsa/mv88e6xxx/hwtstamp.h
> +++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.h
> @@ -120,9 +120,9 @@ int mv88e6xxx_port_hwtstamp_get(struct dsa_switch *ds, int port,
>                                 struct ifreq *ifr);
>
>  bool mv88e6xxx_port_rxtstamp(struct dsa_switch *ds, int port,
> -                            struct sk_buff *clone, unsigned int type);
> +                            struct sk_buff *clone);
>  bool mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
> -                            struct sk_buff *clone, unsigned int type);
> +                            struct sk_buff *clone);
>
>  int mv88e6xxx_get_ts_info(struct dsa_switch *ds, int port,
>                           struct ethtool_ts_info *info);
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 685294817712..2fd844688f83 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -76,6 +76,7 @@ struct dsa_device_ops {
>          * as regular on the master net device.
>          */
>         bool (*filter)(const struct sk_buff *skb, struct net_device *dev);
> +       bool (*can_tstamp)(const struct sk_buff *skb, struct net_device *dev);
>         unsigned int overhead;
>         const char *name;
>         enum dsa_tag_protocol proto;
> @@ -87,6 +88,7 @@ struct dsa_device_ops {
>
>  struct dsa_skb_cb {
>         struct sk_buff *clone;
> +       unsigned int ptp_type;
>         bool deferred_xmit;
>  };
>
> @@ -534,9 +536,9 @@ struct dsa_switch_ops {
>         int     (*port_hwtstamp_set)(struct dsa_switch *ds, int port,
>                                      struct ifreq *ifr);
>         bool    (*port_txtstamp)(struct dsa_switch *ds, int port,
> -                                struct sk_buff *clone, unsigned int type);
> +                                struct sk_buff *clone);
>         bool    (*port_rxtstamp)(struct dsa_switch *ds, int port,
> -                                struct sk_buff *skb, unsigned int type);
> +                                struct sk_buff *skb);
>
>         /*
>          * Deferred frame Tx
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 1fc782fab393..ee5606412e2e 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -181,27 +181,32 @@ EXPORT_SYMBOL_GPL(dsa_dev_to_net_device);
>   * delivered is never notified unless we do so here.
>   */
>  static bool dsa_skb_defer_rx_timestamp(struct dsa_slave_priv *p,
> -                                      struct sk_buff *skb)
> +                                      struct sk_buff *skb,
> +                                      struct net_device *dev)
>  {
>         struct dsa_switch *ds = p->dp->ds;
> -       unsigned int type;
> +
> +       if (!ds->ops->port_rxtstamp)
> +               return false;
>
>         if (skb_headroom(skb) < ETH_HLEN)
>                 return false;
>
>         __skb_push(skb, ETH_HLEN);
>
> -       type = ptp_classify_raw(skb);
> +       DSA_SKB_CB(skb)->ptp_type = ptp_classify_raw(skb);
>
>         __skb_pull(skb, ETH_HLEN);
>
> -       if (type == PTP_CLASS_NONE)
> -               return false;
> -
> -       if (likely(ds->ops->port_rxtstamp))
> -               return ds->ops->port_rxtstamp(ds, p->dp->index, skb, type);
> +       if (p->dp->cpu_dp->tag_ops->can_tstamp) {
> +               if (!p->dp->cpu_dp->tag_ops->can_tstamp(skb, dev))
> +                       return false;
> +       } else {
> +               if (DSA_SKB_CB(skb)->ptp_type == PTP_CLASS_NONE)
> +                       return false;
> +       }
>
> -       return false;
> +       return ds->ops->port_rxtstamp(ds, p->dp->index, skb);
>  }
>
>  static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
> @@ -239,7 +244,7 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
>         s->rx_bytes += skb->len;
>         u64_stats_update_end(&s->syncp);
>
> -       if (dsa_skb_defer_rx_timestamp(p, skb))
> +       if (dsa_skb_defer_rx_timestamp(p, skb, dev))
>                 return 0;
>
>         netif_receive_skb(skb);
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 9892ca1f6859..ebe7944a2d0f 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -410,24 +410,32 @@ static inline netdev_tx_t dsa_slave_netpoll_send_skb(struct net_device *dev,
>  }
>
>  static void dsa_skb_tx_timestamp(struct dsa_slave_priv *p,
> -                                struct sk_buff *skb)
> +                                struct sk_buff *skb, struct net_device *dev)
>  {
>         struct dsa_switch *ds = p->dp->ds;
>         struct sk_buff *clone;
> -       unsigned int type;
>
> -       type = ptp_classify_raw(skb);
> -       if (type == PTP_CLASS_NONE)
> +       if (!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
>                 return;
>
>         if (!ds->ops->port_txtstamp)
>                 return;
>
> +       DSA_SKB_CB(skb)->ptp_type = ptp_classify_raw(skb);
> +
> +       if (p->dp->cpu_dp->tag_ops->can_tstamp) {
> +               if (!p->dp->cpu_dp->tag_ops->can_tstamp(skb, dev))
> +                       return;
> +       } else {
> +               if (DSA_SKB_CB(skb)->ptp_type == PTP_CLASS_NONE)
> +                       return;
> +       }
> +
>         clone = skb_clone_sk(skb);
>         if (!clone)
>                 return;
>
> -       if (ds->ops->port_txtstamp(ds, p->dp->index, clone, type))
> +       if (ds->ops->port_txtstamp(ds, p->dp->index, clone))
>                 return;
>
>         kfree_skb(clone);
> @@ -468,7 +476,7 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
>         /* Identify PTP protocol packets, clone them, and pass them to the
>          * switch driver
>          */
> -       dsa_skb_tx_timestamp(p, skb);
> +       dsa_skb_tx_timestamp(p, skb, dev);
>
>         /* Transmit function may have to reallocate the original SKB,
>          * in which case it must have freed it. Only free it here on error.
> --
> 2.17.1
>

Comments appreciated.

Regards,
-Vladimir

Re: Design issue in DSA RX timestamping (Was "Re: [PATCH net-next 3/5] net: dsa: mv88e6xxx: Let taggers specify a can_timestamp function")

[Richard Cochran]

On Tue, Nov 19, 2019 at 01:35:14PM +0200, Vladimir Oltean wrote:
> - DSA doesn't let drivers to select which frames should be
> timestamped. It knows better.

It time stamps the event frames.
 
> - To be precise, it runs the PTP BPF classifier and filters only the
> SYNC (but not FOLLOW-UP) messages. In principle I agree, the FOLLOW-UP
> frames are general messages and don't need to be timestamped for the
> PTP protocol to work. But by that logic, the HWTSTAMP_FILTER_ALL
> rx_filter shouldn't exist?

FILTER_ALL means all event frames on all transports.  It has nothing
to do with event messages.

> - Because it treats SYNC and FOLLOW-UP frames differently on the RX
> path, it gives them a chance to get reordered. It doesn't give the
> driver a chance to avoid the reordering.

The re-ordering will occur no matter what.  Think about the UDP/IPv4
transport.  The event and general messages are on different ports!
The user space PTP must deal with message re-ordering.


> - Without fully understanding what happens, I tried to propose better
> logic for recovering reordered SYNC/FOLLOW-UP pairs at the application
> level in linuxptp [1]. Needless to say, the consensus was to fix the
> kernel. So here we are.
> 
> [1] https://sourceforge.net/p/linuxptp/mailman/linuxptp-devel/thread/20190928123414.9422-1-olteanv%40gmail.com/#msg36773629

There was no consensus to fix the kernel.  The kernel is fine.  Your
hardware is too slow to keep up with the very high message rate of the
telecom profiles.  Here are a few quotes from that discussion:

   ---
   Are you saying in a local network it's expected that a PTP slave
   receives the follow-up message one or more sync intervals after the
   corresponding sync message, e.g. a delay of 1 second with the default
   sync interval?
   
   I'd not consider such network to be suitable for PTP.
   ---
   I think it's expected that two messages sent within few microseconds or
   milliseconds can be received in a wrong order and PTP slaves need to
   handle that. But it's not expected that a message will be delayed so
   much that it will be received after a message that was sent one or
   more sync intervals later.
   ---
   I'd not expect to receive a follow-up message 30 milliseconds
   after the corresponding sync message. If the difference was getting to
   the millisecond range, I'd be worried that the network is so
   overloaded or misconfigured that it might drop some PTP messages.
   ---

Thanks,
Richard

[PATCH net-next 4/5] net: dsa: sja1105: Add support for PTP timestamping

[Vladimir Oltean]

On RX, timestamping is done by programming the switch to send "meta"
follow-up Ethernet frames after each link-local frame that was trapped
to the CPU port through MAC filtering. This includes PTP frames. These
meta frames contain partial timestamps that are processed in a worker
thread and then the link-local traffic is released back on its path up
the network stack once the timestamps are in place.

Meta frame reception relies on the hardware keeping its promise that it
will send no other traffic towards the CPU port between a link-local
frame and a meta frame. The receive function is made stateful while
expecting for the meta frame to arrive, and a last_stampable_skb pointer
is kept. Otherwise there is no other way to associate the meta frame
with the link-local frame it's holding a timestamp of.

On TX, timestamping is performed synchronously from the port_deferred_xmit
worker thread. In management routes, the switch is requested to take
egress timestamps (again partial), which are reconstructed and appended
to a clone of the skb that was just sent. The cloning is done by DSA and
we retrieve the pointer from the structure that DSA keeps in skb->cb.
Then these clones are enqueued to the socket's error queue for
application-level processing.

Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/net/dsa/sja1105/sja1105.h             |  13 +-
 .../net/dsa/sja1105/sja1105_dynamic_config.c  |   2 +
 drivers/net/dsa/sja1105/sja1105_main.c        | 263 +++++++++++++++++-
 drivers/net/dsa/sja1105/sja1105_ptp.c         |  68 ++++-
 drivers/net/dsa/sja1105/sja1105_ptp.h         |   8 +
 drivers/net/dsa/sja1105/sja1105_spi.c         |  16 +-
 .../net/dsa/sja1105/sja1105_static_config.c   |  59 ++++
 .../net/dsa/sja1105/sja1105_static_config.h   |  10 +
 include/linux/dsa/sja1105.h                   |  15 +
 net/dsa/tag_sja1105.c                         | 135 ++++++++-
 10 files changed, 580 insertions(+), 9 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h
index e0252e6f65ed..f03432bd0e36 100644
--- a/drivers/net/dsa/sja1105/sja1105.h
+++ b/drivers/net/dsa/sja1105/sja1105.h
@@ -33,7 +33,7 @@ struct sja1105_regs {
 	u64 ptpclk;
 	u64 ptpclkrate;
 	u64 ptptsclk;
-	u64 ptpegr_ts;
+	u64 ptpegr_ts[SJA1105_NUM_PORTS];
 	u64 pad_mii_tx[SJA1105_NUM_PORTS];
 	u64 cgu_idiv[SJA1105_NUM_PORTS];
 	u64 rgmii_pad_mii_tx[SJA1105_NUM_PORTS];
@@ -57,6 +57,15 @@ struct sja1105_info {
 	 * switch core and device_id)
 	 */
 	u64 part_no;
+	/* E/T and P/Q/R/S have partial timestamps of different sizes.
+	 * They must be reconstructed on both families anyway to get the full
+	 * 64-bit values back.
+	 */
+	int ptp_ts_bits;
+	/* Also SPI commands are of different sizes to retrieve
+	 * the egress timestamps.
+	 */
+	int ptpegr_ts_bytes;
 	const struct sja1105_dynamic_table_ops *dyn_ops;
 	const struct sja1105_table_ops *static_ops;
 	const struct sja1105_regs *regs;
@@ -89,6 +98,8 @@ struct sja1105_private {
 	 * the switch doesn't confuse them with one another.
 	 */
 	struct mutex mgmt_lock;
+	/* RX timestamping flag is global */
+	bool hwts_rx_en;
 };
 
 #include "sja1105_dynamic_config.h"
diff --git a/drivers/net/dsa/sja1105/sja1105_dynamic_config.c b/drivers/net/dsa/sja1105/sja1105_dynamic_config.c
index e73ab28bf632..6f0e9d180f3e 100644
--- a/drivers/net/dsa/sja1105/sja1105_dynamic_config.c
+++ b/drivers/net/dsa/sja1105/sja1105_dynamic_config.c
@@ -293,6 +293,7 @@ struct sja1105_dynamic_table_ops sja1105et_dyn_ops[BLK_IDX_MAX_DYN] = {
 		.addr = 0x38,
 	},
 	[BLK_IDX_L2_FORWARDING_PARAMS] = {0},
+	[BLK_IDX_AVB_PARAMS] = {0},
 	[BLK_IDX_GENERAL_PARAMS] = {
 		.entry_packing = sja1105et_general_params_entry_packing,
 		.cmd_packing = sja1105et_general_params_cmd_packing,
@@ -348,6 +349,7 @@ struct sja1105_dynamic_table_ops sja1105pqrs_dyn_ops[BLK_IDX_MAX_DYN] = {
 		.addr = 0x38,
 	},
 	[BLK_IDX_L2_FORWARDING_PARAMS] = {0},
+	[BLK_IDX_AVB_PARAMS] = {0},
 	[BLK_IDX_GENERAL_PARAMS] = {
 		.entry_packing = sja1105et_general_params_entry_packing,
 		.cmd_packing = sja1105et_general_params_cmd_packing,
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 8bce1af13d53..ce516615536d 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -499,6 +499,39 @@ static int sja1105_init_l2_policing(struct sja1105_private *priv)
 	return 0;
 }
 
+static int sja1105_init_avb_params(struct sja1105_private *priv,
+				   bool on)
+{
+	struct sja1105_avb_params_entry *avb;
+	struct sja1105_table *table;
+
+	table = &priv->static_config.tables[BLK_IDX_AVB_PARAMS];
+
+	/* Discard previous AVB Parameters Table */
+	if (table->entry_count) {
+		kfree(table->entries);
+		table->entry_count = 0;
+	}
+
+	/* Configure the reception of meta frames only if requested */
+	if (!on)
+		return 0;
+
+	table->entries = kcalloc(SJA1105_MAX_AVB_PARAMS_COUNT,
+				 table->ops->unpacked_entry_size, GFP_KERNEL);
+	if (!table->entries)
+		return -ENOMEM;
+
+	table->entry_count = SJA1105_MAX_AVB_PARAMS_COUNT;
+
+	avb = table->entries;
+
+	avb->destmeta = SJA1105_META_DMAC;
+	avb->srcmeta  = SJA1105_META_SMAC;
+
+	return 0;
+}
+
 static int sja1105_static_config_load(struct sja1105_private *priv,
 				      struct sja1105_dt_port *ports)
 {
@@ -537,6 +570,9 @@ static int sja1105_static_config_load(struct sja1105_private *priv,
 	if (rc < 0)
 		return rc;
 	rc = sja1105_init_general_params(priv);
+	if (rc < 0)
+		return rc;
+	rc = sja1105_init_avb_params(priv, false);
 	if (rc < 0)
 		return rc;
 
@@ -1407,7 +1443,7 @@ static int sja1105_setup(struct dsa_switch *ds)
 }
 
 static int sja1105_mgmt_xmit(struct dsa_switch *ds, int port, int slot,
-			     struct sk_buff *skb)
+			     struct sk_buff *skb, bool takets)
 {
 	struct sja1105_mgmt_entry mgmt_route = {0};
 	struct sja1105_private *priv = ds->priv;
@@ -1420,6 +1456,8 @@ static int sja1105_mgmt_xmit(struct dsa_switch *ds, int port, int slot,
 	mgmt_route.macaddr = ether_addr_to_u64(hdr->h_dest);
 	mgmt_route.destports = BIT(port);
 	mgmt_route.enfport = 1;
+	mgmt_route.tsreg = 0;
+	mgmt_route.takets = takets;
 
 	rc = sja1105_dynamic_config_write(priv, BLK_IDX_MGMT_ROUTE,
 					  slot, &mgmt_route, true);
@@ -1469,7 +1507,12 @@ static netdev_tx_t sja1105_port_deferred_xmit(struct dsa_switch *ds, int port,
 {
 	struct sja1105_private *priv = ds->priv;
 	struct sja1105_port *sp = &priv->ports[port];
+	struct skb_shared_hwtstamps shwt = {0};
 	int slot = sp->mgmt_slot;
+	struct sk_buff *clone;
+	int timeout = 10;
+	int rc;
+	u64 ts;
 
 	/* The tragic fact about the switch having 4x2 slots for installing
 	 * management routes is that all of them except one are actually
@@ -1487,8 +1530,39 @@ static netdev_tx_t sja1105_port_deferred_xmit(struct dsa_switch *ds, int port,
 	 */
 	mutex_lock(&priv->mgmt_lock);
 
-	sja1105_mgmt_xmit(ds, port, slot, skb);
+	/* The clone, if there, was made by dsa_skb_tx_timestamp */
+	clone = DSA_SKB_CB(skb)->clone;
+
+	sja1105_mgmt_xmit(ds, port, slot, skb, !!clone);
+
+	if (!clone)
+		goto out;
+
+	skb_shinfo(clone)->tx_flags |= SKBTX_IN_PROGRESS;
 
+	mutex_lock(&priv->ptp_lock);
+
+	do {
+		rc = sja1105_ptpegr_ts_poll(priv, slot, &ts);
+		if (rc < 0 && rc != -EAGAIN) {
+			dev_err(ds->dev, "xmit: failed to poll for tstamp\n");
+			kfree_skb(clone);
+			goto out_unlock_ptp;
+		}
+		usleep_range(10, 50);
+	} while (rc == -EAGAIN && --timeout);
+
+	if (!timeout)
+		dev_err(priv->ds->dev, "xmit: timed out polling for ts\n");
+
+	ts = timecounter_cyc2time(&priv->tstamp_tc, ts);
+
+	shwt.hwtstamp = ns_to_ktime(ts);
+	skb_complete_tx_timestamp(clone, &shwt);
+
+out_unlock_ptp:
+	mutex_unlock(&priv->ptp_lock);
+out:
 	mutex_unlock(&priv->mgmt_lock);
 	return NETDEV_TX_OK;
 }
@@ -1517,6 +1591,184 @@ static int sja1105_set_ageing_time(struct dsa_switch *ds,
 	return sja1105_static_config_reload(priv);
 }
 
+static int sja1105_change_rxtstamping(struct sja1105_private *priv,
+				      bool on)
+{
+	struct sja1105_general_params_entry *general_params;
+	struct sja1105_table *table;
+	int rc;
+
+	table = &priv->static_config.tables[BLK_IDX_GENERAL_PARAMS];
+	general_params = table->entries;
+	general_params->send_meta1 = on;
+	general_params->send_meta0 = on;
+
+	rc = sja1105_init_avb_params(priv, on);
+	if (rc < 0)
+		return rc;
+
+	return sja1105_static_config_reload(priv);
+}
+
+static int sja1105_hwtstamp_set(struct dsa_switch *ds, int port,
+				struct ifreq *ifr)
+{
+	struct sja1105_private *priv = ds->priv;
+	struct hwtstamp_config config;
+	bool rx_on;
+	int rc, i;
+
+	if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
+		return -EFAULT;
+
+	switch (config.tx_type) {
+	case HWTSTAMP_TX_OFF:
+		priv->ports[port].hwts_tx_en = false;
+		break;
+	case HWTSTAMP_TX_ON:
+		priv->ports[port].hwts_tx_en = true;
+		break;
+	default:
+		return -ERANGE;
+	}
+
+	switch (config.rx_filter) {
+	case HWTSTAMP_FILTER_NONE:
+		rx_on = false;
+		break;
+	default:
+		rx_on = true;
+		break;
+	}
+
+	if (rx_on != priv->hwts_rx_en) {
+		rc = sja1105_change_rxtstamping(priv, rx_on);
+		if (rc < 0) {
+			dev_err(ds->dev,
+				"Failed to change RX timestamping: %d\n", rc);
+			return -EFAULT;
+		}
+		priv->hwts_rx_en = rx_on;
+		/* Duplicate the setting to struct sja1105_port for
+		 * access from the tagger code
+		 */
+		for (i = 0; i < SJA1105_NUM_PORTS; i++)
+			priv->ports[i].hwts_rx_en = rx_on;
+	}
+
+	if (copy_to_user(ifr->ifr_data, &config, sizeof(config)))
+		return -EFAULT;
+	return 0;
+}
+
+static int sja1105_hwtstamp_get(struct dsa_switch *ds, int port,
+				struct ifreq *ifr)
+{
+	struct sja1105_private *priv = ds->priv;
+	struct hwtstamp_config config;
+
+	config.flags = 0;
+	if (priv->ports[port].hwts_tx_en)
+		config.tx_type = HWTSTAMP_TX_ON;
+	else
+		config.tx_type = HWTSTAMP_TX_OFF;
+	if (priv->hwts_rx_en)
+		config.rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
+	else
+		config.rx_filter = HWTSTAMP_FILTER_NONE;
+
+	return copy_to_user(ifr->ifr_data, &config, sizeof(config)) ?
+		-EFAULT : 0;
+}
+
+static void sja1105_port_rxtstamp_work(struct work_struct *work)
+{
+	struct sja1105_port *sp = container_of(work, struct sja1105_port,
+					       rxtstamp_work);
+	struct dsa_switch *ds = sp->dp->ds;
+	struct sja1105_private *priv = ds->priv;
+	struct sk_buff *skb;
+
+	while ((skb = skb_dequeue(&sp->skb_rxtstamp_queue)) != NULL) {
+		struct skb_shared_hwtstamps *shwt = skb_hwtstamps(skb);
+		struct sja1105_skb_cb *cb = SJA1105_SKB_CB(skb);
+		int timeout = 10;
+		u64 ts;
+
+		*shwt = (struct skb_shared_hwtstamps) {0};
+
+		do {
+			if (test_bit(SJA1105_STATE_META_ARRIVED, &cb->state))
+				break;
+			usleep_range(10, 50);
+		} while (--timeout);
+
+		if (!timeout) {
+			dev_err_ratelimited(ds->dev,
+					    "timed out waiting for meta frame\n");
+			goto rcv_anyway;
+		}
+
+		mutex_lock(&priv->ptp_lock);
+
+		ts = cyclecounter_reconstruct(&priv->tstamp_cc,
+					      cb->meta_tstamp);
+		ts = timecounter_cyc2time(&priv->tstamp_tc, ts);
+
+		mutex_unlock(&priv->ptp_lock);
+
+		shwt->hwtstamp = ns_to_ktime(ts);
+rcv_anyway:
+		netif_rx_ni(skb);
+	}
+}
+
+/* Called from dsa_skb_defer_rx_timestamp */
+bool sja1105_port_rxtstamp(struct dsa_switch *ds, int port,
+			   struct sk_buff *skb)
+{
+	struct sja1105_private *priv = ds->priv;
+	struct sja1105_port *sp = &priv->ports[port];
+
+	if (!priv->hwts_rx_en)
+		return false;
+
+	/* We need to read the full PTP clock to reconstruct the Rx
+	 * timestamp. For that we need a sleepable context.
+	 * Furthermore, we should enqueue the skb for timestamp
+	 * reconstruction only once its meta frame came.
+	 * A reference to this skb is also held in sp->last_stampable_skb.
+	 */
+	skb_queue_tail(&sp->skb_rxtstamp_queue, skb);
+	schedule_work(&sp->rxtstamp_work);
+	return true;
+}
+
+/* Called from dsa_skb_tx_timestamp. This callback is just to make DSA clone
+ * the skb and have it available in DSA_SKB_CB in the .port_deferred_xmit
+ * callback, where we will timestamp it synchronously.
+ */
+bool sja1105_port_txtstamp(struct dsa_switch *ds, int port,
+			   struct sk_buff *skb)
+{
+	struct sja1105_private *priv = ds->priv;
+	struct sja1105_port *sp = &priv->ports[port];
+
+	if (!sp->hwts_tx_en)
+		return false;
+
+	return true;
+}
+
+static void sja1105_port_disable(struct dsa_switch *ds, int port)
+{
+	struct sja1105_private *priv = ds->priv;
+	struct sja1105_port *sp = &priv->ports[port];
+
+	cancel_work_sync(&sp->rxtstamp_work);
+	skb_queue_purge(&sp->skb_rxtstamp_queue);
+}
+
 static const struct dsa_switch_ops sja1105_switch_ops = {
 	.get_tag_protocol	= sja1105_get_tag_protocol,
 	.setup			= sja1105_setup,
@@ -1527,6 +1779,7 @@ static const struct dsa_switch_ops sja1105_switch_ops = {
 	.get_ethtool_stats	= sja1105_get_ethtool_stats,
 	.get_sset_count		= sja1105_get_sset_count,
 	.get_ts_info		= sja1105_get_ts_info,
+	.port_disable		= sja1105_port_disable,
 	.port_fdb_dump		= sja1105_fdb_dump,
 	.port_fdb_add		= sja1105_fdb_add,
 	.port_fdb_del		= sja1105_fdb_del,
@@ -1541,6 +1794,10 @@ static const struct dsa_switch_ops sja1105_switch_ops = {
 	.port_mdb_add		= sja1105_mdb_add,
 	.port_mdb_del		= sja1105_mdb_del,
 	.port_deferred_xmit	= sja1105_port_deferred_xmit,
+	.port_hwtstamp_get	= sja1105_hwtstamp_get,
+	.port_hwtstamp_set	= sja1105_hwtstamp_set,
+	.port_rxtstamp		= sja1105_port_rxtstamp,
+	.port_txtstamp		= sja1105_port_txtstamp,
 };
 
 static int sja1105_check_device_id(struct sja1105_private *priv)
@@ -1641,6 +1898,8 @@ static int sja1105_probe(struct spi_device *spi)
 
 		ds->ports[i].priv = sp;
 		sp->dp = &ds->ports[i];
+		skb_queue_head_init(&sp->skb_rxtstamp_queue);
+		INIT_WORK(&sp->rxtstamp_work, sja1105_port_rxtstamp_work);
 	}
 	mutex_init(&priv->mgmt_lock);
 
diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.c b/drivers/net/dsa/sja1105/sja1105_ptp.c
index 9c7f5d6a7e3c..ae3cad7d1ba1 100644
--- a/drivers/net/dsa/sja1105/sja1105_ptp.c
+++ b/drivers/net/dsa/sja1105/sja1105_ptp.c
@@ -70,8 +70,10 @@ int sja1105_get_ts_info(struct dsa_switch *ds, int port,
 	info->so_timestamping = SOF_TIMESTAMPING_TX_HARDWARE |
 				SOF_TIMESTAMPING_RX_HARDWARE |
 				SOF_TIMESTAMPING_RAW_HARDWARE;
-	info->tx_types = (1 << HWTSTAMP_TX_OFF);
-	info->rx_filters = (1 << HWTSTAMP_FILTER_NONE);
+	info->tx_types = (1 << HWTSTAMP_TX_OFF) |
+			 (1 << HWTSTAMP_TX_ON);
+	info->rx_filters = (1 << HWTSTAMP_FILTER_NONE) |
+			   (1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT);
 	info->phc_index = ptp_clock_index(priv->clock);
 	return 0;
 }
@@ -110,6 +112,67 @@ int sja1105pqrs_ptp_cmd(const void *ctx, const void *data)
 					   buf, SJA1105_SIZE_PTP_CMD);
 }
 
+int sja1105_ptpegr_ts_poll(struct sja1105_private *priv, int port, u64 *ts)
+{
+	const struct sja1105_regs *regs = priv->info->regs;
+	int tstamp_bit_start, tstamp_bit_end;
+	u8 packed_buf[8];
+	u64 ts_partial;
+	u64 update;
+	int rc;
+
+	rc = sja1105_spi_send_packed_buf(priv, SPI_READ, regs->ptpegr_ts[port],
+					 packed_buf,
+					 priv->info->ptpegr_ts_bytes);
+	if (rc < 0)
+		return rc;
+
+	/* SJA1105 E/T layout of the 4-byte SPI payload:
+	 *
+	 * 31    23    15    7     0
+	 * |     |     |     |     |
+	 * +-----+-----+-----+     ^
+	 *          ^              |
+	 *          |              |
+	 *  24-bit timestamp   Update bit
+	 *
+	 *
+	 * SJA1105 P/Q/R/S layout of the 8-byte SPI payload:
+	 *
+	 * 31    23    15    7     0     63    55    47    39    32
+	 * |     |     |     |     |     |     |     |     |     |
+	 *                         ^     +-----+-----+-----+-----+
+	 *                         |                 ^
+	 *                         |                 |
+	 *                    Update bit    32-bit timestamp
+	 *
+	 * Notice that the update bit is in the same place.
+	 * To have common code for E/T and P/Q/R/S for reading the timestamp,
+	 * we need to juggle with the offset and the bit indices.
+	 */
+	sja1105_unpack(packed_buf, &update, 0, 0, priv->info->ptpegr_ts_bytes);
+	if (!update)
+		/* No update. Keep trying, you'll make it someday. */
+		return -EAGAIN;
+
+	/* Point the end bit to the second 32-bit word on P/Q/R/S,
+	 * no-op on E/T.
+	 */
+	tstamp_bit_end = (priv->info->ptpegr_ts_bytes - 4) * 8;
+	/* Shift the 24-bit timestamp on E/T to be collected from 31:8.
+	 * No-op on P/Q/R/S.
+	 */
+	tstamp_bit_end += 32 - priv->info->ptp_ts_bits;
+	tstamp_bit_start = tstamp_bit_end + priv->info->ptp_ts_bits - 1;
+
+	sja1105_unpack(packed_buf, &ts_partial, tstamp_bit_start,
+		       tstamp_bit_end, priv->info->ptpegr_ts_bytes);
+
+	*ts = cyclecounter_reconstruct(&priv->tstamp_cc, ts_partial);
+
+	return 0;
+}
+
 static int sja1105_ptp_reset(struct sja1105_private *priv)
 {
 	struct dsa_switch *ds = priv->ds;
@@ -251,6 +314,7 @@ static int sja1105_ptp_init(struct sja1105_private *priv)
 	/* Set up the cycle counter */
 	priv->tstamp_cc = (struct cyclecounter) {
 		.read = sja1105_ptptsclk_read,
+		.partial_tstamp_mask = partial_tstamp_mask,
 		.mask = CYCLECOUNTER_MASK(64),
 		.shift = SJA1105_CC_SHIFT,
 		.mult = SJA1105_CC_MULT,
diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.h b/drivers/net/dsa/sja1105/sja1105_ptp.h
index d2c6cf273500..626292962cb2 100644
--- a/drivers/net/dsa/sja1105/sja1105_ptp.h
+++ b/drivers/net/dsa/sja1105/sja1105_ptp.h
@@ -10,6 +10,8 @@ int sja1105_ptp_clock_register(struct sja1105_private *priv);
 
 void sja1105_ptp_clock_unregister(struct sja1105_private *priv);
 
+int sja1105_ptpegr_ts_poll(struct sja1105_private *priv, int port, u64 *ts);
+
 int sja1105et_ptp_cmd(const void *ctx, const void *data);
 
 int sja1105pqrs_ptp_cmd(const void *ctx, const void *data);
@@ -29,6 +31,12 @@ static inline void sja1105_ptp_clock_unregister(struct sja1105_private *priv)
 	return;
 }
 
+static inline int
+sja1105_ptpegr_ts_poll(struct sja1105_private *priv, int port, u64 *ts)
+{
+	return 0;
+}
+
 #define sja1105et_ptp_cmd NULL
 
 #define sja1105pqrs_ptp_cmd NULL
diff --git a/drivers/net/dsa/sja1105/sja1105_spi.c b/drivers/net/dsa/sja1105/sja1105_spi.c
index f30ac8326c98..0c51548f90fa 100644
--- a/drivers/net/dsa/sja1105/sja1105_spi.c
+++ b/drivers/net/dsa/sja1105/sja1105_spi.c
@@ -507,7 +507,7 @@ static struct sja1105_regs sja1105et_regs = {
 	.rgmii_tx_clk = {0x100016, 0x10001D, 0x100024, 0x10002B, 0x100032},
 	.rmii_ref_clk = {0x100015, 0x10001C, 0x100023, 0x10002A, 0x100031},
 	.rmii_ext_tx_clk = {0x100018, 0x10001F, 0x100026, 0x10002D, 0x100034},
-	.ptpegr_ts = 0xC0,
+	.ptpegr_ts = {0xC0, 0xC2, 0xC4, 0xC6, 0xC8},
 	.ptp_control = 0x17,
 	.ptpclk = 0x18, /* Spans 0x18 to 0x19 */
 	.ptpclkrate = 0x1A,
@@ -538,7 +538,7 @@ static struct sja1105_regs sja1105pqrs_regs = {
 	.rmii_ref_clk = {0x100015, 0x10001B, 0x100021, 0x100027, 0x10002D},
 	.rmii_ext_tx_clk = {0x100017, 0x10001D, 0x100023, 0x100029, 0x10002F},
 	.qlevel = {0x604, 0x614, 0x624, 0x634, 0x644},
-	.ptpegr_ts = 0xC0,
+	.ptpegr_ts = {0xC0, 0xC4, 0xC8, 0xCC, 0xD0},
 	.ptp_control = 0x18,
 	.ptpclk = 0x19,
 	.ptpclkrate = 0x1B,
@@ -550,6 +550,8 @@ struct sja1105_info sja1105e_info = {
 	.part_no		= SJA1105ET_PART_NO,
 	.static_ops		= sja1105e_table_ops,
 	.dyn_ops		= sja1105et_dyn_ops,
+	.ptp_ts_bits		= 24,
+	.ptpegr_ts_bytes	= 4,
 	.reset_cmd		= sja1105et_reset_cmd,
 	.ptp_cmd		= sja1105et_ptp_cmd,
 	.regs			= &sja1105et_regs,
@@ -560,6 +562,8 @@ struct sja1105_info sja1105t_info = {
 	.part_no		= SJA1105ET_PART_NO,
 	.static_ops		= sja1105t_table_ops,
 	.dyn_ops		= sja1105et_dyn_ops,
+	.ptp_ts_bits		= 24,
+	.ptpegr_ts_bytes	= 4,
 	.reset_cmd		= sja1105et_reset_cmd,
 	.ptp_cmd		= sja1105et_ptp_cmd,
 	.regs			= &sja1105et_regs,
@@ -570,6 +574,8 @@ struct sja1105_info sja1105p_info = {
 	.part_no		= SJA1105P_PART_NO,
 	.static_ops		= sja1105p_table_ops,
 	.dyn_ops		= sja1105pqrs_dyn_ops,
+	.ptp_ts_bits		= 32,
+	.ptpegr_ts_bytes	= 8,
 	.reset_cmd		= sja1105pqrs_reset_cmd,
 	.ptp_cmd		= sja1105pqrs_ptp_cmd,
 	.regs			= &sja1105pqrs_regs,
@@ -580,6 +586,8 @@ struct sja1105_info sja1105q_info = {
 	.part_no		= SJA1105Q_PART_NO,
 	.static_ops		= sja1105q_table_ops,
 	.dyn_ops		= sja1105pqrs_dyn_ops,
+	.ptp_ts_bits		= 32,
+	.ptpegr_ts_bytes	= 8,
 	.reset_cmd		= sja1105pqrs_reset_cmd,
 	.ptp_cmd		= sja1105pqrs_ptp_cmd,
 	.regs			= &sja1105pqrs_regs,
@@ -590,6 +598,8 @@ struct sja1105_info sja1105r_info = {
 	.part_no		= SJA1105R_PART_NO,
 	.static_ops		= sja1105r_table_ops,
 	.dyn_ops		= sja1105pqrs_dyn_ops,
+	.ptp_ts_bits		= 32,
+	.ptpegr_ts_bytes	= 8,
 	.reset_cmd		= sja1105pqrs_reset_cmd,
 	.ptp_cmd		= sja1105pqrs_ptp_cmd,
 	.regs			= &sja1105pqrs_regs,
@@ -601,6 +611,8 @@ struct sja1105_info sja1105s_info = {
 	.static_ops		= sja1105s_table_ops,
 	.dyn_ops		= sja1105pqrs_dyn_ops,
 	.regs			= &sja1105pqrs_regs,
+	.ptp_ts_bits		= 32,
+	.ptpegr_ts_bytes	= 8,
 	.reset_cmd		= sja1105pqrs_reset_cmd,
 	.ptp_cmd		= sja1105pqrs_ptp_cmd,
 	.name			= "SJA1105S",
diff --git a/drivers/net/dsa/sja1105/sja1105_static_config.c b/drivers/net/dsa/sja1105/sja1105_static_config.c
index b3c992b0abb0..63ea0471badf 100644
--- a/drivers/net/dsa/sja1105/sja1105_static_config.c
+++ b/drivers/net/dsa/sja1105/sja1105_static_config.c
@@ -91,6 +91,28 @@ u32 sja1105_crc32(const void *buf, size_t len)
 	return ~crc;
 }
 
+static size_t sja1105et_avb_params_entry_packing(void *buf, void *entry_ptr,
+						 enum packing_op op)
+{
+	const size_t size = SJA1105ET_SIZE_AVB_PARAMS_ENTRY;
+	struct sja1105_avb_params_entry *entry = entry_ptr;
+
+	sja1105_packing(buf, &entry->destmeta, 95, 48, size, op);
+	sja1105_packing(buf, &entry->srcmeta,  47,  0, size, op);
+	return size;
+}
+
+static size_t sja1105pqrs_avb_params_entry_packing(void *buf, void *entry_ptr,
+						   enum packing_op op)
+{
+	const size_t size = SJA1105PQRS_SIZE_AVB_PARAMS_ENTRY;
+	struct sja1105_avb_params_entry *entry = entry_ptr;
+
+	sja1105_packing(buf, &entry->destmeta,   125,  78, size, op);
+	sja1105_packing(buf, &entry->srcmeta,     77,  30, size, op);
+	return size;
+}
+
 static size_t sja1105et_general_params_entry_packing(void *buf, void *entry_ptr,
 						     enum packing_op op)
 {
@@ -413,6 +435,7 @@ static u64 blk_id_map[BLK_IDX_MAX] = {
 	[BLK_IDX_MAC_CONFIG] = BLKID_MAC_CONFIG,
 	[BLK_IDX_L2_LOOKUP_PARAMS] = BLKID_L2_LOOKUP_PARAMS,
 	[BLK_IDX_L2_FORWARDING_PARAMS] = BLKID_L2_FORWARDING_PARAMS,
+	[BLK_IDX_AVB_PARAMS] = BLKID_AVB_PARAMS,
 	[BLK_IDX_GENERAL_PARAMS] = BLKID_GENERAL_PARAMS,
 	[BLK_IDX_XMII_PARAMS] = BLKID_XMII_PARAMS,
 };
@@ -614,6 +637,12 @@ struct sja1105_table_ops sja1105e_table_ops[BLK_IDX_MAX] = {
 		.packed_entry_size = SJA1105_SIZE_L2_FORWARDING_PARAMS_ENTRY,
 		.max_entry_count = SJA1105_MAX_L2_FORWARDING_PARAMS_COUNT,
 	},
+	[BLK_IDX_AVB_PARAMS] = {
+		.packing = sja1105et_avb_params_entry_packing,
+		.unpacked_entry_size = sizeof(struct sja1105_avb_params_entry),
+		.packed_entry_size = SJA1105ET_SIZE_AVB_PARAMS_ENTRY,
+		.max_entry_count = SJA1105_MAX_AVB_PARAMS_COUNT,
+	},
 	[BLK_IDX_GENERAL_PARAMS] = {
 		.packing = sja1105et_general_params_entry_packing,
 		.unpacked_entry_size = sizeof(struct sja1105_general_params_entry),
@@ -672,6 +701,12 @@ struct sja1105_table_ops sja1105t_table_ops[BLK_IDX_MAX] = {
 		.packed_entry_size = SJA1105_SIZE_L2_FORWARDING_PARAMS_ENTRY,
 		.max_entry_count = SJA1105_MAX_L2_FORWARDING_PARAMS_COUNT,
 	},
+	[BLK_IDX_AVB_PARAMS] = {
+		.packing = sja1105et_avb_params_entry_packing,
+		.unpacked_entry_size = sizeof(struct sja1105_avb_params_entry),
+		.packed_entry_size = SJA1105ET_SIZE_AVB_PARAMS_ENTRY,
+		.max_entry_count = SJA1105_MAX_AVB_PARAMS_COUNT,
+	},
 	[BLK_IDX_GENERAL_PARAMS] = {
 		.packing = sja1105et_general_params_entry_packing,
 		.unpacked_entry_size = sizeof(struct sja1105_general_params_entry),
@@ -730,6 +765,12 @@ struct sja1105_table_ops sja1105p_table_ops[BLK_IDX_MAX] = {
 		.packed_entry_size = SJA1105_SIZE_L2_FORWARDING_PARAMS_ENTRY,
 		.max_entry_count = SJA1105_MAX_L2_FORWARDING_PARAMS_COUNT,
 	},
+	[BLK_IDX_AVB_PARAMS] = {
+		.packing = sja1105pqrs_avb_params_entry_packing,
+		.unpacked_entry_size = sizeof(struct sja1105_avb_params_entry),
+		.packed_entry_size = SJA1105PQRS_SIZE_AVB_PARAMS_ENTRY,
+		.max_entry_count = SJA1105_MAX_AVB_PARAMS_COUNT,
+	},
 	[BLK_IDX_GENERAL_PARAMS] = {
 		.packing = sja1105pqrs_general_params_entry_packing,
 		.unpacked_entry_size = sizeof(struct sja1105_general_params_entry),
@@ -788,6 +829,12 @@ struct sja1105_table_ops sja1105q_table_ops[BLK_IDX_MAX] = {
 		.packed_entry_size = SJA1105_SIZE_L2_FORWARDING_PARAMS_ENTRY,
 		.max_entry_count = SJA1105_MAX_L2_FORWARDING_PARAMS_COUNT,
 	},
+	[BLK_IDX_AVB_PARAMS] = {
+		.packing = sja1105pqrs_avb_params_entry_packing,
+		.unpacked_entry_size = sizeof(struct sja1105_avb_params_entry),
+		.packed_entry_size = SJA1105PQRS_SIZE_AVB_PARAMS_ENTRY,
+		.max_entry_count = SJA1105_MAX_AVB_PARAMS_COUNT,
+	},
 	[BLK_IDX_GENERAL_PARAMS] = {
 		.packing = sja1105pqrs_general_params_entry_packing,
 		.unpacked_entry_size = sizeof(struct sja1105_general_params_entry),
@@ -846,6 +893,12 @@ struct sja1105_table_ops sja1105r_table_ops[BLK_IDX_MAX] = {
 		.packed_entry_size = SJA1105_SIZE_L2_FORWARDING_PARAMS_ENTRY,
 		.max_entry_count = SJA1105_MAX_L2_FORWARDING_PARAMS_COUNT,
 	},
+	[BLK_IDX_AVB_PARAMS] = {
+		.packing = sja1105pqrs_avb_params_entry_packing,
+		.unpacked_entry_size = sizeof(struct sja1105_avb_params_entry),
+		.packed_entry_size = SJA1105PQRS_SIZE_AVB_PARAMS_ENTRY,
+		.max_entry_count = SJA1105_MAX_AVB_PARAMS_COUNT,
+	},
 	[BLK_IDX_GENERAL_PARAMS] = {
 		.packing = sja1105pqrs_general_params_entry_packing,
 		.unpacked_entry_size = sizeof(struct sja1105_general_params_entry),
@@ -904,6 +957,12 @@ struct sja1105_table_ops sja1105s_table_ops[BLK_IDX_MAX] = {
 		.packed_entry_size = SJA1105_SIZE_L2_FORWARDING_PARAMS_ENTRY,
 		.max_entry_count = SJA1105_MAX_L2_FORWARDING_PARAMS_COUNT,
 	},
+	[BLK_IDX_AVB_PARAMS] = {
+		.packing = sja1105pqrs_avb_params_entry_packing,
+		.unpacked_entry_size = sizeof(struct sja1105_avb_params_entry),
+		.packed_entry_size = SJA1105PQRS_SIZE_AVB_PARAMS_ENTRY,
+		.max_entry_count = SJA1105_MAX_AVB_PARAMS_COUNT,
+	},
 	[BLK_IDX_GENERAL_PARAMS] = {
 		.packing = sja1105pqrs_general_params_entry_packing,
 		.unpacked_entry_size = sizeof(struct sja1105_general_params_entry),
diff --git a/drivers/net/dsa/sja1105/sja1105_static_config.h b/drivers/net/dsa/sja1105/sja1105_static_config.h
index 069ca8fd059c..2c1d7d04e22e 100644
--- a/drivers/net/dsa/sja1105/sja1105_static_config.h
+++ b/drivers/net/dsa/sja1105/sja1105_static_config.h
@@ -20,10 +20,12 @@
 #define SJA1105ET_SIZE_MAC_CONFIG_ENTRY			28
 #define SJA1105ET_SIZE_L2_LOOKUP_PARAMS_ENTRY		4
 #define SJA1105ET_SIZE_GENERAL_PARAMS_ENTRY		40
+#define SJA1105ET_SIZE_AVB_PARAMS_ENTRY			12
 #define SJA1105PQRS_SIZE_L2_LOOKUP_ENTRY		20
 #define SJA1105PQRS_SIZE_MAC_CONFIG_ENTRY		32
 #define SJA1105PQRS_SIZE_L2_LOOKUP_PARAMS_ENTRY		16
 #define SJA1105PQRS_SIZE_GENERAL_PARAMS_ENTRY		44
+#define SJA1105PQRS_SIZE_AVB_PARAMS_ENTRY		16
 
 /* UM10944.pdf Page 11, Table 2. Configuration Blocks */
 enum {
@@ -34,6 +36,7 @@ enum {
 	BLKID_MAC_CONFIG				= 0x09,
 	BLKID_L2_LOOKUP_PARAMS				= 0x0D,
 	BLKID_L2_FORWARDING_PARAMS			= 0x0E,
+	BLKID_AVB_PARAMS				= 0x10,
 	BLKID_GENERAL_PARAMS				= 0x11,
 	BLKID_XMII_PARAMS				= 0x4E,
 };
@@ -46,6 +49,7 @@ enum sja1105_blk_idx {
 	BLK_IDX_MAC_CONFIG,
 	BLK_IDX_L2_LOOKUP_PARAMS,
 	BLK_IDX_L2_FORWARDING_PARAMS,
+	BLK_IDX_AVB_PARAMS,
 	BLK_IDX_GENERAL_PARAMS,
 	BLK_IDX_XMII_PARAMS,
 	BLK_IDX_MAX,
@@ -64,6 +68,7 @@ enum sja1105_blk_idx {
 #define SJA1105_MAX_L2_FORWARDING_PARAMS_COUNT		1
 #define SJA1105_MAX_GENERAL_PARAMS_COUNT		1
 #define SJA1105_MAX_XMII_PARAMS_COUNT			1
+#define SJA1105_MAX_AVB_PARAMS_COUNT			1
 
 #define SJA1105_MAX_FRAME_MEMORY			929
 
@@ -153,6 +158,11 @@ struct sja1105_l2_policing_entry {
 	u64 partition;
 };
 
+struct sja1105_avb_params_entry {
+	u64 destmeta;
+	u64 srcmeta;
+};
+
 struct sja1105_mac_config_entry {
 	u64 top[8];
 	u64 base[8];
diff --git a/include/linux/dsa/sja1105.h b/include/linux/dsa/sja1105.h
index 603a02e5a8cb..b306529636b4 100644
--- a/include/linux/dsa/sja1105.h
+++ b/include/linux/dsa/sja1105.h
@@ -20,13 +20,22 @@
 #define SJA1105_LINKLOCAL_FILTER_B		0x011B19000000ull
 #define SJA1105_LINKLOCAL_FILTER_B_MASK		0xFFFFFF000000ull
 
+/* Source and Destination MAC of follow-up meta frames */
+#define SJA1105_META_SMAC			0x222222222222ull
+#define SJA1105_META_DMAC			0x222222222222ull
+
+#define SJA1105_STATE_META_ARRIVED		0
+
 enum sja1105_frame_type {
 	SJA1105_FRAME_TYPE_NORMAL = 0,
 	SJA1105_FRAME_TYPE_LINK_LOCAL,
+	SJA1105_FRAME_TYPE_META,
 };
 
 struct sja1105_skb_cb {
 	enum sja1105_frame_type type;
+	unsigned long state;
+	u32 meta_tstamp;
 };
 
 #define SJA1105_SKB_CB(skb) \
@@ -35,6 +44,12 @@ struct sja1105_skb_cb {
 struct sja1105_port {
 	struct dsa_port *dp;
 	int mgmt_slot;
+	bool hwts_tx_en;
+	bool hwts_rx_en;
+	bool expect_meta;
+	struct work_struct rxtstamp_work;
+	struct sk_buff *last_stampable_skb;
+	struct sk_buff_head skb_rxtstamp_queue;
 };
 
 #endif /* _NET_DSA_SJA1105_H */
diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c
index 969402c7dbf1..7c2ff5381dd0 100644
--- a/net/dsa/tag_sja1105.c
+++ b/net/dsa/tag_sja1105.c
@@ -22,12 +22,31 @@ static inline bool sja1105_is_link_local(const struct sk_buff *skb)
 	return false;
 }
 
+static inline bool sja1105_is_meta_frame(const struct sk_buff *skb)
+{
+	const struct ethhdr *hdr = eth_hdr(skb);
+	u64 smac = ether_addr_to_u64(hdr->h_source);
+	u64 dmac = ether_addr_to_u64(hdr->h_dest);
+
+	if (smac != SJA1105_META_SMAC)
+		return false;
+	if (dmac != SJA1105_META_DMAC)
+		return false;
+	if (hdr->h_proto != ETH_P_IP)
+		return false;
+	return true;
+}
+
 /* This is the first time the tagger sees the frame on RX.
  * Figure out if we can decode it, and if we can, annotate skb->cb with how we
  * plan to do that, so we don't need to check again in the rcv function.
  */
 static bool sja1105_filter(const struct sk_buff *skb, struct net_device *dev)
 {
+	if (sja1105_is_meta_frame(skb)) {
+		SJA1105_SKB_CB(skb)->type = SJA1105_FRAME_TYPE_META;
+		return true;
+	}
 	if (sja1105_is_link_local(skb)) {
 		SJA1105_SKB_CB(skb)->type = SJA1105_FRAME_TYPE_LINK_LOCAL;
 		return true;
@@ -39,6 +58,16 @@ static bool sja1105_filter(const struct sk_buff *skb, struct net_device *dev)
 	return false;
 }
 
+/* Since this function is called both on ingress and on egress, we can't rely
+ * on the SJA1105_SKB_CB(skb)->type which we only populate on ingress. So we
+ * have to perform the link-local check again.
+ */
+static bool sja1105_can_tstamp(const struct sk_buff *skb,
+			       struct net_device *dev)
+{
+	return sja1105_is_link_local(skb);
+}
+
 static struct sk_buff *sja1105_xmit(struct sk_buff *skb,
 				    struct net_device *netdev)
 {
@@ -66,6 +95,85 @@ static struct sk_buff *sja1105_xmit(struct sk_buff *skb,
 			     ((pcp << VLAN_PRIO_SHIFT) | tx_vid));
 }
 
+/* This is a simple state machine (the state is kept in the private structure
+ * of struct dsa_port) which follows the hardware mechanism of generating RX
+ * timestamps: after each timestampable skb (all traffic for which send_meta1
+ * and send_meta0 is true, therefore all MAC-filtered link-local traffic) a
+ * meta frame containing a partial timestamp is immediately generated by the
+ * switch and sent as a follow-up to the link-local frame on the CPU port.
+ * This function pairs the link-local frame with its partial timestamp from the
+ * meta follow-up frame. The full timestamp will be reconstructed in a delayed
+ * work context.
+ */
+static struct sk_buff *sja1105_rcv_meta_state_machine(struct sk_buff *skb)
+{
+	struct sja1105_port *sp;
+	struct dsa_port *dp;
+
+	dp = dsa_slave_to_port(skb->dev);
+	sp = dp->priv;
+
+	if (SJA1105_SKB_CB(skb)->type == SJA1105_FRAME_TYPE_META) {
+		struct sja1105_skb_cb *cb;
+
+		/* Was this a meta frame instead of the link-local
+		 * that we were expecting?
+		 */
+		if (!sp->expect_meta) {
+			/* If timestamping is not enabled on this port and
+			 * we're still receiving meta frames, it must have been
+			 * requested on other ports. But since the send_meta
+			 * setting is global for the switch, receiving a meta
+			 * frame at this point is not really unexpected, but
+			 * nonetheless useless. So just discard it and don't
+			 * print anything to the user in that case.
+			 */
+			if (sp->hwts_rx_en)
+				dev_err_ratelimited(dp->ds->dev,
+						    "Unexpected meta frame\n");
+			return NULL;
+		}
+
+		sp->expect_meta = false;
+		/* Copy the timestamp from the meta frame
+		 * into the cached skb.
+		 */
+		cb = SJA1105_SKB_CB(sp->last_stampable_skb);
+		cb->meta_tstamp = SJA1105_SKB_CB(skb)->meta_tstamp;
+		set_bit(SJA1105_STATE_META_ARRIVED, &cb->state);
+		skb_unref(sp->last_stampable_skb);
+		sp->last_stampable_skb = NULL;
+		/* Drop the meta frame from further processing up the stack */
+		return NULL;
+	} else if ((SJA1105_SKB_CB(skb)->type ==
+		    SJA1105_FRAME_TYPE_LINK_LOCAL) && sp->hwts_rx_en) {
+		/* Was this a link-local frame instead of the meta
+		 * that we were expecting?
+		 */
+		if (sp->expect_meta) {
+			dev_err_ratelimited(dp->ds->dev,
+					    "Expected meta frame\n");
+			skb_unref(sp->last_stampable_skb);
+			sp->last_stampable_skb = NULL;
+		}
+
+		sp->expect_meta = true;
+		/* Hold a reference so that we avoid the
+		 * sja1105_port_rxtstamp_work freeing it from under our feet.
+		 */
+		sp->last_stampable_skb = skb_get(skb);
+		/* Let this frame be forwarded on the switch port, queued by
+		 * dsa_skb_defer_rx_timestamp and reinjected in the stack by
+		 * sja1105_port_rxtstamp_work once we also receive the meta
+		 * frame.
+		 */
+		clear_bit(SJA1105_STATE_META_ARRIVED,
+			 &SJA1105_SKB_CB(skb)->state);
+	}
+
+	return skb;
+}
+
 static struct sk_buff *sja1105_rcv(struct sk_buff *skb,
 				   struct net_device *netdev,
 				   struct packet_type *pt)
@@ -75,6 +183,7 @@ static struct sk_buff *sja1105_rcv(struct sk_buff *skb,
 	struct sk_buff *nskb;
 	u16 tpid, vid, tci;
 	bool is_tagged;
+	u64 tstamp;
 
 	nskb = dsa_8021q_rcv(skb, netdev, pt, &tpid, &tci);
 	is_tagged = (nskb && tpid == ETH_P_SJA1105);
@@ -84,7 +193,28 @@ static struct sk_buff *sja1105_rcv(struct sk_buff *skb,
 
 	skb->offload_fwd_mark = 1;
 
-	if (SJA1105_SKB_CB(skb)->type == SJA1105_FRAME_TYPE_LINK_LOCAL) {
+	if (SJA1105_SKB_CB(skb)->type == SJA1105_FRAME_TYPE_META) {
+		u8 *meta = skb_mac_header(skb) + ETH_HLEN;
+
+		/* UM10944.pdf section 4.2.17 AVB Parameters:
+		 * Structure of the meta-data follow-up frame.
+		 * It is in network byte order, so there are no quirks
+		 * while unpacking the meta frame.
+		 *
+		 * Also SJA1105 E/T only populates bits 23:0 of the timestamp
+		 * whereas P/Q/R/S does 32 bits. Since the structure is the
+		 * same and the E/T puts zeroes in the high-order byte, use
+		 * a unified unpacking command for both device series.
+		 */
+		packing(meta,     &tstamp,     31, 0, 4, UNPACK, 0);
+		packing(meta + 6, &source_port, 7, 0, 1, UNPACK, 0);
+		packing(meta + 7, &switch_id,   7, 0, 1, UNPACK, 0);
+		if (is_tagged)
+			dump_stack();
+
+		SJA1105_SKB_CB(skb)->meta_tstamp = tstamp;
+
+	} else if (SJA1105_SKB_CB(skb)->type == SJA1105_FRAME_TYPE_LINK_LOCAL) {
 		/* Management traffic path. Switch embeds the switch ID and
 		 * port ID into bytes of the destination MAC, courtesy of
 		 * the incl_srcpt options.
@@ -113,7 +243,7 @@ static struct sk_buff *sja1105_rcv(struct sk_buff *skb,
 		memmove(skb->data - ETH_HLEN, skb->data - ETH_HLEN - VLAN_HLEN,
 			ETH_HLEN - VLAN_HLEN);
 
-	return skb;
+	return sja1105_rcv_meta_state_machine(skb);
 }
 
 static struct dsa_device_ops sja1105_netdev_ops = {
@@ -122,6 +252,7 @@ static struct dsa_device_ops sja1105_netdev_ops = {
 	.xmit = sja1105_xmit,
 	.rcv = sja1105_rcv,
 	.filter = sja1105_filter,
+	.can_tstamp = sja1105_can_tstamp,
 	.overhead = VLAN_HLEN,
 };
 
-- 
2.17.1

[PATCH net-next 5/5] net: dsa: sja1105: Increase priority of CPU-trapped frames

[Vladimir Oltean]

Without noticing any particular issue, this patch ensures that
management traffic is treated with the maximum priority on RX by the
switch.  This is generally desirable, as the driver keeps a state
machine that waits for metadata follow-up frames as soon as a management
frame is received.  Increasing the priority helps expedite the reception
(and further reconstruction) of the RX timestamp to the driver after the
MAC has generated it.

Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/net/dsa/sja1105/sja1105_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index ce516615536d..3bd250e4e070 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -380,7 +380,7 @@ static int sja1105_init_general_params(struct sja1105_private *priv)
 		.mirr_ptacu = 0,
 		.switchid = priv->ds->index,
 		/* Priority queue for link-local frames trapped to CPU */
-		.hostprio = 0,
+		.hostprio = 7,
 		.mac_fltres1 = SJA1105_LINKLOCAL_FILTER_A,
 		.mac_flt1    = SJA1105_LINKLOCAL_FILTER_A_MASK,
 		.incl_srcpt1 = true,
-- 
2.17.1

Re: [PATCH net-next 0/5] PTP support for the SJA1105 DSA driver

[Richard Cochran]

On Wed, May 29, 2019 at 02:56:22AM +0300, Vladimir Oltean wrote:
> Not all is rosy, though.

You can sure say that again!
 
> PTP timestamping will only work when the ports are bridged. Otherwise,
> the metadata follow-up frames holding RX timestamps won't be received
> because they will be blocked by the master port's MAC filter. Linuxptp
> tries to put the net device in ALLMULTI/PROMISC mode,

Untrue.

> but DSA doesn't
> pass this on to the master port, which does the actual reception.
> The master port is put in promiscous mode when the slave ports are
> enslaved to a bridge.
> 
> Also, even with software-corrected timestamps, one can observe a
> negative path delay reported by linuxptp:
> 
> ptp4l[55.600]: master offset          8 s2 freq  +83677 path delay     -2390
> ptp4l[56.600]: master offset         17 s2 freq  +83688 path delay     -2391
> ptp4l[57.601]: master offset          6 s2 freq  +83682 path delay     -2391
> ptp4l[58.601]: master offset         -1 s2 freq  +83677 path delay     -2391
> 
> Without investigating too deeply, this appears to be introduced by the
> correction applied by linuxptp to t4 (t4c: corrected master rxtstamp)
> during the path delay estimation process (removing the correction makes
> the path delay positive).

No.  The root cause is the time stamps delivered by the hardware or
your driver.  That needs to be addressed before going forward.

Thanks,
Richard

Re: [PATCH net-next 0/5] PTP support for the SJA1105 DSA driver

[Vladimir Oltean]

On 5/29/19 7:52 AM, Richard Cochran wrote:
> On Wed, May 29, 2019 at 02:56:22AM +0300, Vladimir Oltean wrote:
>> Not all is rosy, though.
> 
> You can sure say that again!
>   
>> PTP timestamping will only work when the ports are bridged. Otherwise,
>> the metadata follow-up frames holding RX timestamps won't be received
>> because they will be blocked by the master port's MAC filter. Linuxptp
>> tries to put the net device in ALLMULTI/PROMISC mode,
> 
> Untrue.
> 

I'm sorry, then what does this code from raw.c do?

> 	mreq.mr_ifindex = index;
> 	mreq.mr_type = PACKET_MR_ALLMULTI;
> 	mreq.mr_alen = 0;
> 	if (!setsockopt(fd, SOL_PACKET, option, &mreq, sizeof(mreq))) {
> 		return 0;
> 	}
> 	pr_warning("setsockopt PACKET_MR_ALLMULTI failed: %m");
> 
> 	mreq.mr_ifindex = index;
> 	mreq.mr_type = PACKET_MR_PROMISC;
> 	mreq.mr_alen = 0;
> 	if (!setsockopt(fd, SOL_PACKET, option, &mreq, sizeof(mreq))) {
> 		return 0;
> 	}
> 	pr_warning("setsockopt PACKET_MR_PROMISC failed: %m");


>> but DSA doesn't
>> pass this on to the master port, which does the actual reception.
>> The master port is put in promiscous mode when the slave ports are
>> enslaved to a bridge.
>>
>> Also, even with software-corrected timestamps, one can observe a
>> negative path delay reported by linuxptp:
>>
>> ptp4l[55.600]: master offset          8 s2 freq  +83677 path delay     -2390
>> ptp4l[56.600]: master offset         17 s2 freq  +83688 path delay     -2391
>> ptp4l[57.601]: master offset          6 s2 freq  +83682 path delay     -2391
>> ptp4l[58.601]: master offset         -1 s2 freq  +83677 path delay     -2391
>>
>> Without investigating too deeply, this appears to be introduced by the
>> correction applied by linuxptp to t4 (t4c: corrected master rxtstamp)
>> during the path delay estimation process (removing the correction makes
>> the path delay positive).
> 
> No.  The root cause is the time stamps delivered by the hardware or
> your driver.  That needs to be addressed before going forward.
> 

How can I check that the timestamps are valid?

Regards,
-Vladimir

> Thanks,
> Richard
> 

Re: [PATCH net-next 0/5] PTP support for the SJA1105 DSA driver

[Richard Cochran]

On Wed, May 29, 2019 at 11:41:22PM +0300, Vladimir Oltean wrote:
> I'm sorry, then what does this code from raw.c do?

It is a fallback for HW that doesn't support multicast filtering.

Care to look a few lines above?  If you did, you would have seen this:

	memset(&mreq, 0, sizeof(mreq));
	mreq.mr_ifindex = index;
	mreq.mr_type = PACKET_MR_MULTICAST;
	mreq.mr_alen = MAC_LEN;
	memcpy(mreq.mr_address, addr1, MAC_LEN);

	err1 = setsockopt(fd, SOL_PACKET, option, &mreq, sizeof(mreq));

> > No.  The root cause is the time stamps delivered by the hardware or
> > your driver.  That needs to be addressed before going forward.
> > 
> 
> How can I check that the timestamps are valid?

Well, you can see that there is something wrong.  Perhaps you are not
matching the meta frames to the received packets.  That is one
possible explanation, but you'll have to figure out what is happening.

Thanks,
Richard

Re: [PATCH net-next 0/5] PTP support for the SJA1105 DSA driver

[Vladimir Oltean]

On Thu, 30 May 2019 at 06:45, Richard Cochran <richardcochran@gmail.com> wrote:
>
> On Wed, May 29, 2019 at 11:41:22PM +0300, Vladimir Oltean wrote:
> > I'm sorry, then what does this code from raw.c do?
>
> It is a fallback for HW that doesn't support multicast filtering.
>
> Care to look a few lines above?  If you did, you would have seen this:
>
>         memset(&mreq, 0, sizeof(mreq));
>         mreq.mr_ifindex = index;
>         mreq.mr_type = PACKET_MR_MULTICAST;
>         mreq.mr_alen = MAC_LEN;
>         memcpy(mreq.mr_address, addr1, MAC_LEN);
>
>         err1 = setsockopt(fd, SOL_PACKET, option, &mreq, sizeof(mreq));
>

You're right.
In fact that's why it doesn't work: because linuxptp adds ptp_dst_mac
(01-1B-19-00-00-00) and (01-80-C2-00-00-0E) to the MAC's multicast
filter, but the switch in its great wisdom mangles bytes
01-1B-19-xx-xx-00 of the DMAC to place the switch id and source port
there (a rudimentary tagging mechanism). So the frames are no longer
accepted by this multicast MAC filter on the DSA master port unless
it's put in ALLMULTI or PROMISC.

> > > No.  The root cause is the time stamps delivered by the hardware or
> > > your driver.  That needs to be addressed before going forward.
> > >
> >
> > How can I check that the timestamps are valid?
>
> Well, you can see that there is something wrong.  Perhaps you are not
> matching the meta frames to the received packets.  That is one
> possible explanation, but you'll have to figure out what is happening.
>

If the meta frames weren't associated with the correct link-local
frame, then the whole expect_meta -> SJA1105_STATE_META_ARRIVED
mechanism would go haywire, but it doesn't.
I was actually thinking it has something to do with the fact that I
shouldn't apply frequency corrections on timestamps of PTP delay
messages. Does that make any sense?

Regards,
-Vladimir

> Thanks,
> Richard

Re: [PATCH net-next 0/5] PTP support for the SJA1105 DSA driver

[Richard Cochran]

On Thu, May 30, 2019 at 12:01:23PM +0300, Vladimir Oltean wrote:
> In fact that's why it doesn't work: because linuxptp adds ptp_dst_mac
> (01-1B-19-00-00-00) and (01-80-C2-00-00-0E) to the MAC's multicast
> filter, but the switch in its great wisdom mangles bytes
> 01-1B-19-xx-xx-00 of the DMAC to place the switch id and source port
> there (a rudimentary tagging mechanism). So the frames are no longer
> accepted by this multicast MAC filter on the DSA master port unless
> it's put in ALLMULTI or PROMISC.

IOW, it is not linuxptp's choice to use these modes, but rather this
is caused by a limitation of your device.
 
> If the meta frames weren't associated with the correct link-local
> frame, then the whole expect_meta -> SJA1105_STATE_META_ARRIVED
> mechanism would go haywire, but it doesn't.

Not necessarily.  If two frames that arrive at nearly the same time
get their timestamps mixed up, that would be enough to break the time
values but without breaking your state machine.

> I was actually thinking it has something to do with the fact that I
> shouldn't apply frequency corrections on timestamps of PTP delay
> messages. Does that make any sense?

What do you mean by that?  Is the driver altering PTP message fields?

Thanks,
Richard

Re: [PATCH net-next 0/5] PTP support for the SJA1105 DSA driver

[Vladimir Oltean]

On Thu, 30 May 2019 at 17:30, Richard Cochran <richardcochran@gmail.com> wrote:
>
> On Thu, May 30, 2019 at 12:01:23PM +0300, Vladimir Oltean wrote:
> > In fact that's why it doesn't work: because linuxptp adds ptp_dst_mac
> > (01-1B-19-00-00-00) and (01-80-C2-00-00-0E) to the MAC's multicast
> > filter, but the switch in its great wisdom mangles bytes
> > 01-1B-19-xx-xx-00 of the DMAC to place the switch id and source port
> > there (a rudimentary tagging mechanism). So the frames are no longer
> > accepted by this multicast MAC filter on the DSA master port unless
> > it's put in ALLMULTI or PROMISC.
>
> IOW, it is not linuxptp's choice to use these modes, but rather this
> is caused by a limitation of your device.
>

Didn't want to suggest otherwise. I'll see how I'm going to address that.

> > If the meta frames weren't associated with the correct link-local
> > frame, then the whole expect_meta -> SJA1105_STATE_META_ARRIVED
> > mechanism would go haywire, but it doesn't.
>
> Not necessarily.  If two frames that arrive at nearly the same time
> get their timestamps mixed up, that would be enough to break the time
> values but without breaking your state machine.
>

This doesn't exactly sound like the type of thing I can check for.
The RX and TX timestamps *are* monotonically increasing with time for
all frames when I'm printing them in the {rx,tx}tstamp callbacks.

> > I was actually thinking it has something to do with the fact that I
> > shouldn't apply frequency corrections on timestamps of PTP delay
> > messages. Does that make any sense?
>
> What do you mean by that?  Is the driver altering PTP message fields?

No.
The driver returns free-running timestamps altered with a timecounter
frequency set by adjfine and offset set by adjtime.
I was wondering out loud if there's any value in identifying delay
messages in order to not apply this frequency adjustment for their
timestamps.

-Vladimir

>
> Thanks,
> Richard

Re: [PATCH net-next 0/5] PTP support for the SJA1105 DSA driver

[Richard Cochran]

On Thu, May 30, 2019 at 05:57:30PM +0300, Vladimir Oltean wrote:
> On Thu, 30 May 2019 at 17:30, Richard Cochran <richardcochran@gmail.com> wrote:
> >
> > Not necessarily.  If two frames that arrive at nearly the same time
> > get their timestamps mixed up, that would be enough to break the time
> > values but without breaking your state machine.
> >
> 
> This doesn't exactly sound like the type of thing I can check for.

And that is why it cannot work.

> The RX and TX timestamps *are* monotonically increasing with time for
> all frames when I'm printing them in the {rx,tx}tstamp callbacks.

But are the frames received in the same order?  What happens your MAC
drops a frame?
 
> The driver returns free-running timestamps altered with a timecounter
> frequency set by adjfine and offset set by adjtime.

That should be correct.

Thanks,
Richard

Re: [PATCH net-next 0/5] PTP support for the SJA1105 DSA driver

[Vladimir Oltean]

On Thu, 30 May 2019 at 18:06, Richard Cochran <richardcochran@gmail.com> wrote:
>
> On Thu, May 30, 2019 at 05:57:30PM +0300, Vladimir Oltean wrote:
> > On Thu, 30 May 2019 at 17:30, Richard Cochran <richardcochran@gmail.com> wrote:
> > >
> > > Not necessarily.  If two frames that arrive at nearly the same time
> > > get their timestamps mixed up, that would be enough to break the time
> > > values but without breaking your state machine.
> > >
> >
> > This doesn't exactly sound like the type of thing I can check for.
>
> And that is why it cannot work.
>
> > The RX and TX timestamps *are* monotonically increasing with time for
> > all frames when I'm printing them in the {rx,tx}tstamp callbacks.
>
> But are the frames received in the same order?  What happens your MAC
> drops a frame?
>

If it drops a normal frame, it carries on.
If it drops a meta frame, it prints "Expected meta frame", resets the
state machine and carries on.
If it drops a timestampable frame, it prints "Unexpected meta frame",
resets the state machine and carries on.
This doesn't happen under correct runtime conditions though.

-Vladimir

> > The driver returns free-running timestamps altered with a timecounter
> > frequency set by adjfine and offset set by adjtime.
>
> That should be correct.
>
> Thanks,
> Richard

Re: [PATCH net-next 0/5] PTP support for the SJA1105 DSA driver

[Richard Cochran]

On Thu, May 30, 2019 at 06:23:09PM +0300, Vladimir Oltean wrote:
> On Thu, 30 May 2019 at 18:06, Richard Cochran <richardcochran@gmail.com> wrote:
> >
> > But are the frames received in the same order?  What happens your MAC
> > drops a frame?
> >
> 
> If it drops a normal frame, it carries on.
> If it drops a meta frame, it prints "Expected meta frame", resets the
> state machine and carries on.
> If it drops a timestampable frame, it prints "Unexpected meta frame",
> resets the state machine and carries on.

What I meant was, consider how dropped frames in the MAC will spoil
any chance that the driver has to correctly match time stamps with
frames.

Thanks,
Richard

Re: [PATCH net-next 0/5] PTP support for the SJA1105 DSA driver

[Vladimir Oltean]

On Fri, 31 May 2019 at 07:34, Richard Cochran <richardcochran@gmail.com> wrote:
>
> On Thu, May 30, 2019 at 06:23:09PM +0300, Vladimir Oltean wrote:
> > On Thu, 30 May 2019 at 18:06, Richard Cochran <richardcochran@gmail.com> wrote:
> > >
> > > But are the frames received in the same order?  What happens your MAC
> > > drops a frame?
> > >
> >
> > If it drops a normal frame, it carries on.
> > If it drops a meta frame, it prints "Expected meta frame", resets the
> > state machine and carries on.
> > If it drops a timestampable frame, it prints "Unexpected meta frame",
> > resets the state machine and carries on.
>
> What I meant was, consider how dropped frames in the MAC will spoil
> any chance that the driver has to correctly match time stamps with
> frames.
>
> Thanks,
> Richard

I think you are still looking at this through the perspective of
sequence numbers.
I am *not* proposing to add sequence numbers for link-local and for
meta in software, and then try to match them, as that would lead to
complete chaos as you're suggesting.
The switch has internal logic to not send any other frame to the CPU
between a link-local and a meta frame.
Hence, if the MAC of the DSA master drops some of these frames, it
does not "spoil any chance" except if, out of the sequence LL n ->
META n -> LL n+1 -> META n+1, it persistently drops only META n and LL
n+1. If it drops less than 2 frames, the system recovers. And if it
drops more, oh well, there are more pressing issues to deal with..
So I'd like to re-state the problem towards what should be done to
prevent LL and META frames getting reordered in the DSA master driver
on multi-queue/multi-core systems. At the most basic level, there
should exist a rule that makes only a single core process these
frames.

Regards,
-Vladimir

Re: [PATCH net-next 0/5] PTP support for the SJA1105 DSA driver

[Richard Cochran]

On Fri, May 31, 2019 at 04:23:24PM +0300, Vladimir Oltean wrote:
> The switch has internal logic to not send any other frame to the CPU
> between a link-local and a meta frame.

So this is guarantied by the switch?  What happens when multiple PTP
frames arrive at the same time on different ports?  Does the switch
buffer them and ensure strict ordering at the CPU port?

In any case, the switch's guarantee is an important fact to state
clearly in your series!

> Hence, if the MAC of the DSA master drops some of these frames, it
> does not "spoil any chance" except if, out of the sequence LL n ->
> META n -> LL n+1 -> META n+1, it persistently drops only META n and LL
> n+1.

LL = link layer?

> So I'd like to re-state the problem towards what should be done to
> prevent LL and META frames getting reordered in the DSA master driver
> on multi-queue/multi-core systems.

Ok.

> At the most basic level, there
> should exist a rule that makes only a single core process these
> frames.

This can be done simply using a data structure in the driver with an
appropriate locking mechanism.  Then you don't have to worry which
core the driver code runs on.

Thanks,
Richard

Re: [PATCH net-next 0/5] PTP support for the SJA1105 DSA driver

[Vladimir Oltean]

On Fri, 31 May 2019 at 17:08, Richard Cochran <richardcochran@gmail.com> wrote:
>
> On Fri, May 31, 2019 at 04:23:24PM +0300, Vladimir Oltean wrote:
> > The switch has internal logic to not send any other frame to the CPU
> > between a link-local and a meta frame.
>
> So this is guarantied by the switch?  What happens when multiple PTP
> frames arrive at the same time on different ports?  Does the switch
> buffer them and ensure strict ordering at the CPU port?
>
> In any case, the switch's guarantee is an important fact to state
> clearly in your series!
>

Yes, ports with lower index take priority.

> > Hence, if the MAC of the DSA master drops some of these frames, it
> > does not "spoil any chance" except if, out of the sequence LL n ->
> > META n -> LL n+1 -> META n+1, it persistently drops only META n and LL
> > n+1.
>
> LL = link layer?
>

Yes, link-local in this case means trapped frames in the
01-80-C2-xx-xx-xx or 01:1B:C9:xx:xx:xx space.

> > So I'd like to re-state the problem towards what should be done to
> > prevent LL and META frames getting reordered in the DSA master driver
> > on multi-queue/multi-core systems.
>
> Ok.
>
> > At the most basic level, there
> > should exist a rule that makes only a single core process these
> > frames.
>
> This can be done simply using a data structure in the driver with an
> appropriate locking mechanism.  Then you don't have to worry which
> core the driver code runs on.
>

Actually you do. DSA is special because it is not the first net device
in the RX path that processes the frames. Something needs to be done
on the master port.

> Thanks,
> Richard

Re: [PATCH net-next 0/5] PTP support for the SJA1105 DSA driver

[Richard Cochran]

On Fri, May 31, 2019 at 05:27:15PM +0300, Vladimir Oltean wrote:
> On Fri, 31 May 2019 at 17:08, Richard Cochran <richardcochran@gmail.com> wrote:
> > This can be done simply using a data structure in the driver with an
> > appropriate locking mechanism.  Then you don't have to worry which
> > core the driver code runs on.
> >
> 
> Actually you do. DSA is special because it is not the first net device
> in the RX path that processes the frames. Something needs to be done
> on the master port.

Before you said,

	the switch in its great wisdom mangles bytes 01-1B-19-xx-xx-00
	of the DMAC to place the switch id and source port there (a
	rudimentary tagging mechanism).

So why not simply save each frame in a per-switch/port data structure?

Now I'm starting to understand your series.  I think it can be done in
simpler way...

sja1105_rcv_meta_state_machine - can and should be at the driver level
and not at the port level.

sja1105_port_rxtstamp_work - isn't needed at all.

How about this?

1. When the driver receives a deferred PTP frame, save it into a
   per-switch,port slot at the driver (not port) level.

2. When the driver receives a META frame, match it to the
   per-switch,port slot.  If there is a PTP frame in that slot, then
   deliver it with the time stamp from the META frame.

Thanks,
Richard

Re: [PATCH net-next 0/5] PTP support for the SJA1105 DSA driver

[Richard Cochran]

On Fri, May 31, 2019 at 08:11:51AM -0700, Richard Cochran wrote:
> 
> 1. When the driver receives a deferred PTP frame, save it into a
>    per-switch,port slot at the driver (not port) level.
> 
> 2. When the driver receives a META frame, match it to the
>    per-switch,port slot.  If there is a PTP frame in that slot, then
>    deliver it with the time stamp from the META frame.

Actually, since the switch guarantees strict ordering, you don't need
multiple slots.  You only need to save one Rx'd PTP frame with its
switch-id and port-id.

Thanks,
Richard

Re: [PATCH net-next 0/5] PTP support for the SJA1105 DSA driver

[Vladimir Oltean]

On Fri, 31 May 2019 at 18:11, Richard Cochran <richardcochran@gmail.com> wrote:
>
> On Fri, May 31, 2019 at 05:27:15PM +0300, Vladimir Oltean wrote:
> > On Fri, 31 May 2019 at 17:08, Richard Cochran <richardcochran@gmail.com> wrote:
> > > This can be done simply using a data structure in the driver with an
> > > appropriate locking mechanism.  Then you don't have to worry which
> > > core the driver code runs on.
> > >
> >
> > Actually you do. DSA is special because it is not the first net device
> > in the RX path that processes the frames. Something needs to be done
> > on the master port.
>
> Before you said,
>
>         the switch in its great wisdom mangles bytes 01-1B-19-xx-xx-00
>         of the DMAC to place the switch id and source port there (a
>         rudimentary tagging mechanism).
>
> So why not simply save each frame in a per-switch/port data structure?
>

You mean to queue it and subvert DSA's own RX timestamping callback?
Why would I do that? Just so as not to introduce my .can_timestamp
callback?

> Now I'm starting to understand your series.  I think it can be done in
> simpler way...
>
> sja1105_rcv_meta_state_machine - can and should be at the driver level
> and not at the port level.
>

Can: yes. Should: why?

> sja1105_port_rxtstamp_work - isn't needed at all.
>
> How about this?
>
> 1. When the driver receives a deferred PTP frame, save it into a
>    per-switch,port slot at the driver (not port) level.
>
> 2. When the driver receives a META frame, match it to the
>    per-switch,port slot.  If there is a PTP frame in that slot, then
>    deliver it with the time stamp from the META frame.
>

One important aspect makes this need be a little bit more complicated:
reconstructing these RX timestamps.
You see, there is a mutex on the SPI bus, so in practice I do need the
sja1105_port_rxtstamp_work for exactly this purpose - to read the
timestamping clock over SPI.

> Thanks,
> Richard

Re: [PATCH net-next 0/5] PTP support for the SJA1105 DSA driver

[Richard Cochran]

On Fri, May 31, 2019 at 06:23:34PM +0300, Vladimir Oltean wrote:
> You mean to queue it and subvert DSA's own RX timestamping callback?

No, use the callback.

> Why would I do that? Just so as not to introduce my .can_timestamp
> callback?

Right, the .can_timestamp is unneeded, AFAICT.
 
> > Now I'm starting to understand your series.  I think it can be done in
> > simpler way...
> >
> > sja1105_rcv_meta_state_machine - can and should be at the driver level
> > and not at the port level.
> >
> 
> Can: yes. Should: why?

To keep it simple and robust.
 
> One important aspect makes this need be a little bit more complicated:
> reconstructing these RX timestamps.
> You see, there is a mutex on the SPI bus, so in practice I do need the
> sja1105_port_rxtstamp_work for exactly this purpose - to read the
> timestamping clock over SPI.

Sure.  But you schedule the work after a META frame.  And no busy
waiting is needed.
 
Thanks,
Richard

Re: [PATCH net-next 0/5] PTP support for the SJA1105 DSA driver

[Vladimir Oltean]

On Fri, 31 May 2019 at 19:09, Richard Cochran <richardcochran@gmail.com> wrote:
>
> On Fri, May 31, 2019 at 06:23:34PM +0300, Vladimir Oltean wrote:
> > You mean to queue it and subvert DSA's own RX timestamping callback?
>
> No, use the callback.
>
> > Why would I do that? Just so as not to introduce my .can_timestamp
> > callback?
>
> Right, the .can_timestamp is unneeded, AFAICT.
>
> > > Now I'm starting to understand your series.  I think it can be done in
> > > simpler way...
> > >
> > > sja1105_rcv_meta_state_machine - can and should be at the driver level
> > > and not at the port level.
> > >
> >
> > Can: yes. Should: why?
>
> To keep it simple and robust.
>
> > One important aspect makes this need be a little bit more complicated:
> > reconstructing these RX timestamps.
> > You see, there is a mutex on the SPI bus, so in practice I do need the
> > sja1105_port_rxtstamp_work for exactly this purpose - to read the
> > timestamping clock over SPI.
>
> Sure.  But you schedule the work after a META frame.  And no busy
> waiting is needed.

Ok, I suppose this could work.
But now comes the question on what to do on error cases - the meta
frame didn't arrive. Should I just drop the skb waiting for it? Right
now I "goto rcv_anyway" - which linuxptp doesn't like btw.

>
> Thanks,
> Richard

Re: [PATCH net-next 0/5] PTP support for the SJA1105 DSA driver

[Vladimir Oltean]

On Fri, 31 May 2019 at 19:16, Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Fri, 31 May 2019 at 19:09, Richard Cochran <richardcochran@gmail.com> wrote:
> >
> > On Fri, May 31, 2019 at 06:23:34PM +0300, Vladimir Oltean wrote:
> > > You mean to queue it and subvert DSA's own RX timestamping callback?
> >
> > No, use the callback.
> >
> > > Why would I do that? Just so as not to introduce my .can_timestamp
> > > callback?
> >
> > Right, the .can_timestamp is unneeded, AFAICT.
> >
> > > > Now I'm starting to understand your series.  I think it can be done in
> > > > simpler way...
> > > >
> > > > sja1105_rcv_meta_state_machine - can and should be at the driver level
> > > > and not at the port level.
> > > >
> > >
> > > Can: yes. Should: why?
> >
> > To keep it simple and robust.
> >
> > > One important aspect makes this need be a little bit more complicated:
> > > reconstructing these RX timestamps.
> > > You see, there is a mutex on the SPI bus, so in practice I do need the
> > > sja1105_port_rxtstamp_work for exactly this purpose - to read the
> > > timestamping clock over SPI.
> >
> > Sure.  But you schedule the work after a META frame.  And no busy
> > waiting is needed.
>
> Ok, I suppose this could work.
> But now comes the question on what to do on error cases - the meta
> frame didn't arrive. Should I just drop the skb waiting for it? Right
> now I "goto rcv_anyway" - which linuxptp doesn't like btw.
>

Actually I've been there before, just forgot it.
It won't work unless I make changes to dsa_switch_rcv.
Right now taggers can only return a pointer to the skb, or NULL, case
in which DSA will free it.
I'd need a mechanism to signal DSA that I'm holding up the skb for a
little bit more time, then re-engage the dsa_switch_rcv path once the
meta frame arrived.

> >
> > Thanks,
> > Richard

Re: [PATCH net-next 0/5] PTP support for the SJA1105 DSA driver

[Richard Cochran]

On Fri, May 31, 2019 at 09:12:03PM +0300, Vladimir Oltean wrote:
> It won't work unless I make changes to dsa_switch_rcv.

Or to the tagging code.

> Right now taggers can only return a pointer to the skb, or NULL, case
> in which DSA will free it.

The tagger can re-write the skb.  Why not reform it into a PTP frame?
This clever trick is what the phyter does in hardware.  See dp83640.c.

Thanks,
Richard

Re: [PATCH net-next 0/5] PTP support for the SJA1105 DSA driver

[Vladimir Oltean]

On Sat, 1 Jun 2019 at 08:07, Richard Cochran <richardcochran@gmail.com> wrote:
>
> On Fri, May 31, 2019 at 09:12:03PM +0300, Vladimir Oltean wrote:
> > It won't work unless I make changes to dsa_switch_rcv.
>
> Or to the tagging code.
>
> > Right now taggers can only return a pointer to the skb, or NULL, case
> > in which DSA will free it.
>
> The tagger can re-write the skb.  Why not reform it into a PTP frame?
> This clever trick is what the phyter does in hardware.  See dp83640.c.
>

I think you're missing the point here.
If I dress the meta frame into a PTP frame (btw is there any
preferable event message for this purpose?) then sure, I'll make
dsa_skb_defer_rx_timestamp call my .port_rxtstamp and I can e.g. move
my state machine there.
The problem is that in the current DSA structure, I'll still have less
timestampable frames waiting for a meta frame than meta frames
themselves. This is because not all frames that the switch takes an RX
timestamp for will make it to my .port_rxtstamp (in fact that is what
my .can_timestamp patch changes). I can put the timestampable frame in
a 1-entry wait queue which I'll deplete upon arrival of the first meta
frame, but when I get meta frames and the wait queue is empty, it can
mean multiple things: either DSA didn't care about this timestamp
(ok), or the timestampable frame got reordered or dropped by the MAC,
or what have you (not ok). So I can't exclude the possibility that the
meta frame was holding a relevant timestamp.
Sure, I can dress the meta frame into whatever the previous
MAC-trapped frame was (PTP or not) and then I'll have .port_rxtstamp
function see a 1-to-1 correspondence with meta frames in case
everything works fine. But then I'll have non-PTP meta frames leaking
up the stack...

Regards,
-Vladimir

> Thanks,
> Richard

Re: [PATCH net-next 0/5] PTP support for the SJA1105 DSA driver

[Vladimir Oltean]

On Sat, 1 Jun 2019 at 13:31, Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Sat, 1 Jun 2019 at 08:07, Richard Cochran <richardcochran@gmail.com> wrote:
> >
> > On Fri, May 31, 2019 at 09:12:03PM +0300, Vladimir Oltean wrote:
> > > It won't work unless I make changes to dsa_switch_rcv.
> >
> > Or to the tagging code.
> >
> > > Right now taggers can only return a pointer to the skb, or NULL, case
> > > in which DSA will free it.
> >
> > The tagger can re-write the skb.  Why not reform it into a PTP frame?
> > This clever trick is what the phyter does in hardware.  See dp83640.c.
> >
>
> I think you're missing the point here.
> If I dress the meta frame into a PTP frame (btw is there any
> preferable event message for this purpose?) then sure, I'll make
> dsa_skb_defer_rx_timestamp call my .port_rxtstamp and I can e.g. move
> my state machine there.
> The problem is that in the current DSA structure, I'll still have less
> timestampable frames waiting for a meta frame than meta frames
> themselves. This is because not all frames that the switch takes an RX
> timestamp for will make it to my .port_rxtstamp (in fact that is what
> my .can_timestamp patch changes). I can put the timestampable frame in
> a 1-entry wait queue which I'll deplete upon arrival of the first meta
> frame, but when I get meta frames and the wait queue is empty, it can
> mean multiple things: either DSA didn't care about this timestamp
> (ok), or the timestampable frame got reordered or dropped by the MAC,
> or what have you (not ok). So I can't exclude the possibility that the
> meta frame was holding a relevant timestamp.
> Sure, I can dress the meta frame into whatever the previous
> MAC-trapped frame was (PTP or not) and then I'll have .port_rxtstamp
> function see a 1-to-1 correspondence with meta frames in case
> everything works fine. But then I'll have non-PTP meta frames leaking
> up the stack...
>

Actually maybe this is exactly what you meant and I didn't think it through.
If RX timestamping is enabled, then I can just copy all MAC-trapped
frames to a private skb in a per-driver data structure, and have DSA
drop them.
Then when their meta frame arrives, I can just morph them into what
the previous frame was, just that now I'm also holding the partial
timestamp in skb->cb.
PTP frames will reconstruct the full timestamp without waiting for any
meta (they are the meta), while other MAC-trapped frames (STP etc)
will just carry a meaningless skb->cb when passed up the stack.
In retrospect, it would have been amazing if the switch gave me the
meta frames *before* the actual link-local frames that needed the
timestamp.

Thanks!
-Vladimir

> Regards,
> -Vladimir
>
> > Thanks,
> > Richard

Re: [PATCH net-next 0/5] PTP support for the SJA1105 DSA driver

[Richard Cochran]

On Sat, Jun 01, 2019 at 03:06:59PM +0300, Vladimir Oltean wrote:
> PTP frames will reconstruct the full timestamp without waiting for any
> meta (they are the meta), while other MAC-trapped frames (STP etc)
> will just carry a meaningless skb->cb when passed up the stack.
> In retrospect, it would have been amazing if the switch gave me the
> meta frames *before* the actual link-local frames that needed the
> timestamp.

I didn't follow everything you wrote, but it sounds like you are on
the right track!

Thanks,
Richard

Re: [PATCH net-next 0/5] PTP support for the SJA1105 DSA driver

[Richard Cochran]

On Sat, Jun 01, 2019 at 01:31:34PM +0300, Vladimir Oltean wrote:
> If I dress the meta frame into a PTP frame (btw is there any
> preferable event message for this purpose?)

I would just make a L2 PTP event message from a specific source
address, just like the phyter does.

Use Ethertype ETH_P_1588 (0x88f7), and make sure the "general" bit
(0x8) of the messageType field (the first payload byte, at offset 14)
is clear.

dp83640.c uses a magic source address to identify a time stamping
status frame:

static u8 status_frame_src[6] = { 0x08, 0x00, 0x17, 0x0B, 0x6B, 0x0F };

HTH,
Richard

Re: [PATCH net-next 0/5] PTP support for the SJA1105 DSA driver

[Richard Cochran]

On Fri, May 31, 2019 at 07:16:17PM +0300, Vladimir Oltean wrote:
> But now comes the question on what to do on error cases - the meta
> frame didn't arrive. Should I just drop the skb waiting for it?

Yes, that is what other drivers do.

> Right now I "goto rcv_anyway" - which linuxptp doesn't like btw.

The application sees the missing time stamp and prints a warning
message.  This is IHMO the right thing to do, so that the user is made
aware of the degradation of the synchronization.

Thanks,
Richard