[PATCH net-next 0/3] net: ethernet: ti: am65-cpts: add PTP_SYS_OFFSET_PRECISE support

[PATCH net-next 0/3] net: ethernet: ti: am65-cpts: add PTP_SYS_OFFSET_PRECISE support

[James Clark]

This series adds PTP_SYS_OFFSET_PRECISE to the CPTS driver for TI K3
SoCs. This brings a similar hardware-supported level of precision to the
transfer of time from the PHC to the system clock as already exists for
the transfer of time into the PHC, whether over the network using
hardware timestamping of packets, or from a GNSS receiver using hardware
timestamping of its PPS signal.

The system counter of these SoCs, the Global Timebase Counter (GTC), can
export one of its bits on a push event output. The Time Sync Router can
route that output to a CPTS hardware timestamp input. The CPTS then
timestamps edges that fall at known counter values, so each timestamp
gives an exact pair of PHC time and system counter value. This is
similar to the PCIe PTM integration on these SoCs, where a selected bit
of the PTM timer drives a push input of the PCIe CPTS.

Patch 1 adds a syscon compatible for the GTC register frame that selects
the bit. Patch 2 adds a ti,gtc-push property to the CPTS binding, which
names that syscon and the CPTS input. Patch 3 implements getcrosststamp.

The series does not set up the Time Sync Router. Mainline has no binding
for it on the J722S, so for now a board overlay sets the route, as it
does for ti,pps. With Rahul Sharma's proposed binding for the router
[1], the CPTS node could select the route itself. ti,gtc-push would stay
the same.

Tested on a BeagleY-AI (AM67A / J722S), a BeagleBone AI-64 (TDA4VM /
J721E) and a BeaglePlay (AM625).

With this series, phc2sys steering the system clock from a free-running
PHC sees an offset that never varies by more than 1ns, whether idle or
under memory load. Since the PHC and the system counter run from the
same oscillator, this is the noise of PTP_SYS_OFFSET_PRECISE itself.
Without this series, phc2sys falls back to PTP_SYS_OFFSET_EXTENDED and
the offset has a standard deviation of 2.5 to 10ns when idle and 22 to
31ns under load, with excursions of up to 200ns.

When the PHC is disciplined (by ptp4l or ts2phc, to within 4 to 7ns rms
of its reference), the offset seen by phc2sys with this series has a
standard deviation of 2 to 5ns, whether idle or under memory load.
Without this series, it has a standard deviation of 5 to 12ns when idle
and 22 to 29ns under load, again with excursions of up to 200ns.

PTP_SYS_OFFSET_PRECISE has a fixed bias, the capture delay of the push
event through the Time Sync Router and the CPTS input (about 11ns, plus
an undocumented delay in the GTC), which did not measurably change under
any load or CPU clock. This series does not correct for this bias. The
driver similarly does not correct for timestamping delay on extts
inputs, nor for ingress/egress latency in hardware timestamping of
packets. It turns out that the bias in PTP_SYS_OFFSET_PRECISE and the
delay in extts timestamping largely cancel each other out. The offset of
PTP_SYS_OFFSET_EXTENDED relative to PTP_SYS_OFFSET_PRECISE is about
-50ns when idle, and moves by up to 40ns with memory load and by up to
60ns with CPU clock frequency.

The implementation was also stress tested by using ntpperf to load the
CPTS (-D 0 -M at 15,000/s and -N 0 at 5,000/s). No errors or wrong pairs
were seen.

To try it on a BeagleY-AI, apply this overlay:

	// SPDX-License-Identifier: GPL-2.0-only OR MIT
	/dts-v1/;
	/plugin/;

	&{/} {
		#address-cells = <2>;
		#size-cells = <2>;

		wkup_gtc: syscon@a80000 {
			compatible = "ti,am654-gtc", "syscon";
			reg = <0x0 0xa80000 0x0 0x400>;
		};
	};

	&cbass_main {
		#address-cells = <2>;
		#size-cells = <2>;

		timesync_router: pinctrl@a40000 {
			compatible = "pinctrl-single";
			reg = <0x0 0xa40000 0x0 0x400>;
			#pinctrl-cells = <1>;
			pinctrl-single,register-width = <32>;
			pinctrl-single,function-mask = <0x000107ff>;
			pinctrl-names = "default";
			pinctrl-0 = <&gtc_push_route>;

			gtc_push_route: gtc-push-route {
				/* input 11 (GTC push event) -> output 14 (CPTS HW5_TS_PUSH) */
				pinctrl-single,pins = <0x3c 0x1000b>;
			};
		};
	};

	&{/bus@f0000/ethernet@8000000/cpts@3d000} {
		ti,gtc-push = <&wkup_gtc 4>;
	};

I used an LLM coding assistant to help prepare this series.

[1] https://lore.kernel.org/all/20260828100615.1700223-1-r-sharma3@ti.com/

James Clark (3):
  dt-bindings: mfd: syscon: add ti,am654-gtc
  dt-bindings: net: ti,k3-am654-cpts: add ti,gtc-push
  net: ethernet: ti: am65-cpts: implement PTP_SYS_OFFSET_PRECISE

 .../devicetree/bindings/mfd/syscon.yaml       |   1 +
 .../bindings/net/ti,k3-am654-cpts.yaml        |  17 ++
 drivers/net/ethernet/ti/am65-cpts.c           | 289 ++++++++++++++++++
 3 files changed, 307 insertions(+)


base-commit: fa5196392691051963726787944db872420dfa75
-- 
2.56.0

[PATCH net-next 1/3] dt-bindings: mfd: syscon: add ti,am654-gtc

[James Clark]

Add a compatible for the CFG0 register frame of the Global Timebase
Counter (GTC) on TI K3 SoCs. The GTC is the system counter that the Arm
cores read as the architected counter. CFG0 holds GTC_CFG0_PUSHEVT,
which selects the counter bit that the GTC exports as its push event
output; the CPTS driver writes to that register to provide hardware
cross-timestamps.

Signed-off-by: James Clark <jjc@jclark.com>
---
 Documentation/devicetree/bindings/mfd/syscon.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/mfd/syscon.yaml b/Documentation/devicetree/bindings/mfd/syscon.yaml
index 80ff0e1caca2..b1030f11b2db 100644
--- a/Documentation/devicetree/bindings/mfd/syscon.yaml
+++ b/Documentation/devicetree/bindings/mfd/syscon.yaml
@@ -131,6 +131,7 @@ properties:
               - ti,am625-dss-oldi-io-ctrl
               - ti,am62p-cpsw-mac-efuse
               - ti,am654-dss-oldi-io-ctrl
+              - ti,am654-gtc
               - ti,j784s4-acspcie-proxy-ctrl
               - ti,j784s4-pcie-ctrl
               - ti,keystone-pllctrl
-- 
2.56.0

[PATCH net-next 2/3] dt-bindings: net: ti,k3-am654-cpts: add ti,gtc-push

[James Clark]

The Global Timebase Counter (GTC) of TI K3 SoCs is the system counter,
the one the Arm cores read as the architected counter. It can export one
of its counter bits, selected by GTC_CFG0_PUSHEVT, as a push event
output, and the Time Sync Router can route that output to one of the
CPTS hardware timestamp push inputs (HWx_TS_PUSH). The CPTS then
timestamps edges that fall at known counter values, which gives pairs of
CPTS and system counter values.

Add an optional ti,gtc-push property giving the GTC CFG0 syscon and the
HWx_TS_PUSH input the push event is routed to. The CPTS driver uses this
to write GTC_CFG0_PUSHEVT. The router itself is configured separately, as
it is for ti,pps.

Signed-off-by: James Clark <jjc@jclark.com>
---
 .../bindings/net/ti,k3-am654-cpts.yaml          | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ti,k3-am654-cpts.yaml b/Documentation/devicetree/bindings/net/ti,k3-am654-cpts.yaml
index 3572749147fb..7bf5bedc2b9c 100644
--- a/Documentation/devicetree/bindings/net/ti,k3-am654-cpts.yaml
+++ b/Documentation/devicetree/bindings/net/ti,k3-am654-cpts.yaml
@@ -101,6 +101,23 @@ properties:
       The pair of HWx_TS_PUSH input and TS_GENFy output indexes used for
       PPS events generation. Platform/board specific.
 
+  ti,gtc-push:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    items:
+      - items:
+          - description: phandle to the Global Timebase Counter (GTC) CFG0
+              syscon
+          - description: HWx_TS_PUSH input index (0-based) the GTC push event
+              is routed to
+            maximum: 7
+    description: |
+      The GTC is the SoC's system counter (the Arm architected counter). Its
+      push event output, one counter bit selected by GTC_CFG0_PUSHEVT, reaches
+      a CPTS hardware timestamp push input through the Time Sync Router. With
+      this property the CPTS timestamps that signal to provide hardware
+      cross-timestamps of the CPTS and the system counter. The router is
+      configured separately. Platform/board specific.
+
   refclk-mux:
     type: object
     additionalProperties: false
-- 
2.56.0

[PATCH net-next 3/3] net: ethernet: ti: am65-cpts: implement PTP_SYS_OFFSET_PRECISE

[James Clark]

Add hardware-supported PTP_SYS_OFFSET_PRECISE to the CPTS driver for
TI K3 SoCs.

The system counter of these SoCs, the Global Timebase Counter (GTC),
can export one of its bits on a push event output, selected by the
PUSHEVT_EXPBIT_SEL field of GTC_CFG0_PUSHEVT. The Time Sync Router can
route that output to a CPTS hardware timestamp input. The CPTS then
timestamps rising edges that fall at known counter values (with bit b
selected, at k * 2^(b + 1) + 2^b), so each timestamp gives an exact
pair of PHC time and system counter value.

Implement getcrosststamp with these timestamps when the CPTS node has a
ti,gtc-push property. The property names the GTC CFG0 syscon and the
push input to which the router delivers the push event. The router is
set up separately, as it is for ti,pps. If the input is one of the extts
channels, that channel can no longer be enabled.

A push event timestamp contains just the PHC time, so the driver needs
to work out which GTC edge it corresponds to. The push input is
normally disabled. For each getcrosststamp request, the driver reads
the GTC, enables the input, reads the GTC again, waits for a push
event and then disables the input. The timestamp in that event must
correspond to the first edge after the earlier reading, provided that
edge is also after the later reading. If not, the driver retries. The
driver also has to detect a push event lost to an overflow of the CPTS
event FIFO. So it reads the GTC a third time when it takes the event
from the FIFO, and retries if that reading is a period or more after
the expected edge. After three failed attempts, the request fails with
-EBUSY.

Usually drivers capture the pair in the callback to
get_device_system_crosststamp(), but this works only when the capture
is fast, because the core calls the callback inside a timekeeper
seqcount loop and repeats it whenever the timekeeper is updated. So
this driver captures the pair before the call, and the callback just
returns it.

The choice of counter bit is a tradeoff: each attempt waits for half a
period on average; a longer period makes retries rarer but makes each
attempt longer. The driver uses bit 15, which at 200 MHz gives a period
of 328 us, chosen after testing a range of possible values. This is
sufficiently long to ensure each attempt fails rarely (0.2% to 0.9%
depending on CPU and memory load); failure of all three attempts
required extreme conditions (50 kHz extts flood with memory load).

The driver reads the GTC through the clocksource, which assumes that
the clocksource counter value equals the GTC count, that is a CNTVOFF
of 0. This holds on the tested boards. The cross-timestamp in
dwmac-socfpga makes the same assumption.

The timestamps are not corrected for the delay from the push output to
the CPTS latching the edge, estimated at about 11ns on a BeagleY-AI; the
driver similarly does not correct extts timestamps for their input
delay.

Tested on a BeagleY-AI (TI AM67A / J722S), on the MCU CPSW of a
BeagleBone AI-64 (TI TDA4VM / J721E) and on a BeaglePlay (TI AM625).

Signed-off-by: James Clark <jjc@jclark.com>
---
 drivers/net/ethernet/ti/am65-cpts.c | 289 ++++++++++++++++++++++++++++
 1 file changed, 289 insertions(+)

diff --git a/drivers/net/ethernet/ti/am65-cpts.c b/drivers/net/ethernet/ti/am65-cpts.c
index 0d71147f3c29..70b0d43bfe78 100644
--- a/drivers/net/ethernet/ti/am65-cpts.c
+++ b/drivers/net/ethernet/ti/am65-cpts.c
@@ -5,11 +5,13 @@
  *
  */
 
+#include <linux/bitfield.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/err.h>
 #include <linux/if_vlan.h>
 #include <linux/interrupt.h>
+#include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/netdevice.h>
 #include <linux/net_tstamp.h>
@@ -19,6 +21,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/ptp_classify.h>
 #include <linux/ptp_clock_kernel.h>
+#include <linux/regmap.h>
 
 #include "am65-cpts.h"
 
@@ -128,6 +131,10 @@ struct am65_cpts_regs {
 #define AM65_CPTS_EVENT_2_REG_DOMAIN_MASK	(0xFF)
 #define AM65_CPTS_EVENT_2_REG_DOMAIN_SHIFT	(0)
 
+/* GTC_CFG0_PUSHEVT_REG */
+#define AM65_CPTS_GTC_PUSHEVT			0x8
+#define AM65_CPTS_GTC_PUSHEVT_EXPBIT_SEL	GENMASK(5, 0)
+
 enum {
 	AM65_CPTS_EV_PUSH,	/* Time Stamp Push Event */
 	AM65_CPTS_EV_ROLL,	/* Time Stamp Rollover Event */
@@ -152,6 +159,11 @@ struct am65_cpts_event {
 #define AM65_CPTS_EVENT_RX_TX_TIMEOUT	(20) /* ms */
 #define AM65_CPTS_SKB_TX_WORK_TIMEOUT	1 /* jiffies */
 #define AM65_CPTS_MIN_PPM		0x400
+#define AM65_CPTS_GTC_PUSH_BIT		15 /* 328 us period at 200 MHz */
+#define AM65_CPTS_GTC_PUSH_PERIOD	BIT_ULL(AM65_CPTS_GTC_PUSH_BIT + 1)
+#define AM65_CPTS_GTC_PUSH_TIMEOUT	(20) /* ms */
+#define AM65_CPTS_GTC_PUSH_TRIES	3
+#define AM65_CPTS_GTC_PUSH_DRAIN_PASSES	8
 
 struct am65_cpts {
 	struct device *dev;
@@ -186,6 +198,13 @@ struct am65_cpts {
 	bool pps_present;
 	u32 pps_hw_ts_idx;
 	u32 pps_genf_idx;
+	struct regmap *gtc;
+	u32 gtc_push_hw_idx;
+	u64 gtc_push_ts;
+	u64 gtc_push_cycles; /* the counter when the event was read */
+	bool gtc_push_armed;
+	wait_queue_head_t gtc_push_wq;
+	struct mutex gtc_push_lock; /* serialises crosststamp requests */
 	/* context save/restore */
 	u64 sr_cpts_ns;
 	u64 sr_ktime_ns;
@@ -290,6 +309,21 @@ static bool am65_cpts_fifo_pop_event(struct am65_cpts *cpts,
 	return true;
 }
 
+static void am65_cpts_gtc_push_event(struct am65_cpts *cpts,
+				     struct am65_cpts_event *event)
+{
+	struct system_time_snapshot snap;
+
+	/* keep the first event of a request; later edges are not used */
+	if (!cpts->gtc_push_armed)
+		return;
+	ktime_get_snapshot_id(CLOCK_REALTIME, &snap);
+	cpts->gtc_push_ts = event->timestamp;
+	cpts->gtc_push_cycles = snap.cycles;
+	WRITE_ONCE(cpts->gtc_push_armed, false);
+	wake_up(&cpts->gtc_push_wq);
+}
+
 static int __am65_cpts_fifo_read(struct am65_cpts *cpts)
 {
 	struct ptp_clock_event pevent;
@@ -345,6 +379,11 @@ static int __am65_cpts_fifo_read(struct am65_cpts *cpts)
 			break;
 		case AM65_CPTS_EV_HW:
 			pevent.index = am65_cpts_event_get_port(event) - 1;
+			if (cpts->gtc &&
+			    pevent.index == cpts->gtc_push_hw_idx) {
+				am65_cpts_gtc_push_event(cpts, event);
+				break;
+			}
 			pevent.timestamp = event->timestamp;
 			if (cpts->pps_enabled && pevent.index == cpts->pps_hw_ts_idx) {
 				pevent.type = PTP_CLOCK_PPSUSR;
@@ -620,6 +659,9 @@ static int am65_cpts_extts_enable(struct am65_cpts *cpts, u32 index, int on)
 	if (cpts->pps_present && index == cpts->pps_hw_ts_idx)
 		return -EINVAL;
 
+	if (cpts->gtc && index == cpts->gtc_push_hw_idx)
+		return -EINVAL;
+
 	if (((cpts->hw_ts_enable & BIT(index)) >> index) == on)
 		return 0;
 
@@ -797,6 +839,226 @@ static int am65_cpts_ptp_enable(struct ptp_clock_info *ptp,
 	return -EOPNOTSUPP;
 }
 
+struct am65_cpts_gtc_push {
+	u64 ts;
+	u64 cycles;
+};
+
+/* the first rising edge of the exported bit after cycles */
+static u64 am65_cpts_gtc_push_next_edge(u64 cycles)
+{
+	u64 half = AM65_CPTS_GTC_PUSH_PERIOD / 2;
+
+	return round_down(cycles - half, AM65_CPTS_GTC_PUSH_PERIOD) + half +
+	       AM65_CPTS_GTC_PUSH_PERIOD;
+}
+
+static int am65_cpts_gtc_push_read(ktime_t *device,
+				   struct system_counterval_t *system,
+				   void *ctx)
+{
+	struct am65_cpts_gtc_push *push = ctx;
+
+	*device = ns_to_ktime(push->ts);
+	system->cycles = push->cycles;
+	system->cs_id = CSID_ARM_ARCH_COUNTER;
+	system->use_nsecs = false;
+
+	return 0;
+}
+
+/* select the counter bit the GTC exports on its push event output */
+static int am65_cpts_gtc_push_select(struct am65_cpts *cpts)
+{
+	u32 val;
+	int ret;
+
+	ret = regmap_update_bits(cpts->gtc, AM65_CPTS_GTC_PUSHEVT,
+				 AM65_CPTS_GTC_PUSHEVT_EXPBIT_SEL,
+				 FIELD_PREP(AM65_CPTS_GTC_PUSHEVT_EXPBIT_SEL,
+					    AM65_CPTS_GTC_PUSH_BIT));
+	if (ret)
+		return ret;
+	/* flush the posted write before the input is enabled */
+	return regmap_read(cpts->gtc, AM65_CPTS_GTC_PUSHEVT, &val);
+}
+
+/*
+ * Read the FIFO until it is empty, then accept the next push event. One
+ * __am65_cpts_fifo_read() takes at most AM65_CPTS_FIFO_DEPTH events, and after
+ * a burst of push edges (a request that wakes up late leaves the input enabled
+ * for many periods) more can be waiting. A push event left behind would be
+ * taken for the next attempt's. With the push input disabled no new push
+ * events arrive, so after enough passes every old one has been read even if
+ * packet events keep the FIFO busy. The lock is dropped between passes; a push
+ * event read meanwhile by the IRQ thread is ignored, as nothing is armed.
+ */
+static int am65_cpts_gtc_push_arm(struct am65_cpts *cpts)
+{
+	unsigned long flags;
+	int i;
+
+	for (i = 0; i < AM65_CPTS_GTC_PUSH_DRAIN_PASSES; i++) {
+		spin_lock_irqsave(&cpts->lock, flags);
+		__am65_cpts_fifo_read(cpts);
+		if (!(am65_cpts_read32(cpts, intstat_raw) &
+		      AM65_CPTS_INTSTAT_RAW_TS_PEND)) {
+			cpts->gtc_push_armed = true;
+			spin_unlock_irqrestore(&cpts->lock, flags);
+			return 0;
+		}
+		spin_unlock_irqrestore(&cpts->lock, flags);
+	}
+	return -EBUSY;
+}
+
+static void am65_cpts_gtc_push_disarm(struct am65_cpts *cpts)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&cpts->lock, flags);
+	cpts->gtc_push_armed = false;
+	spin_unlock_irqrestore(&cpts->lock, flags);
+}
+
+/*
+ * Enable or disable the push input and read CONTROL back. After a disable,
+ * the read-back means no event of an edge captured before the disable
+ * reaches the FIFO after the next drain.
+ */
+static void am65_cpts_gtc_push_enable(struct am65_cpts *cpts, bool on)
+{
+	mutex_lock(&cpts->ptp_clk_lock);
+	am65_cpts_extts_enable_hw(cpts, cpts->gtc_push_hw_idx, on);
+	am65_cpts_read32(cpts, control);
+	mutex_unlock(&cpts->ptp_clk_lock);
+}
+
+/*
+ * Wait for the event of the edge at counter value edge. Returns -EAGAIN if no
+ * event arrives in time or the event may not be of that edge.
+ */
+static int am65_cpts_gtc_push_wait(struct am65_cpts *cpts, u64 edge,
+				   struct am65_cpts_gtc_push *push)
+{
+	unsigned long tmo = msecs_to_jiffies(AM65_CPTS_GTC_PUSH_TIMEOUT);
+	unsigned long flags;
+	u64 popped;
+
+	if (!wait_event_timeout(cpts->gtc_push_wq,
+				!READ_ONCE(cpts->gtc_push_armed), tmo)) {
+		dev_dbg(cpts->dev, "%s: no event for edge %llu\n",
+			__func__, edge);
+		return -EAGAIN;
+	}
+
+	/*
+	 * The event handler read the counter when it took the event from the
+	 * FIFO. The event of the edge cannot be taken before the edge. If it
+	 * was taken a period or more after the edge, the event may be of a
+	 * later edge: when the FIFO overruns, the CPTS loses events.
+	 */
+	spin_lock_irqsave(&cpts->lock, flags);
+	push->ts = cpts->gtc_push_ts;
+	popped = cpts->gtc_push_cycles;
+	spin_unlock_irqrestore(&cpts->lock, flags);
+	if ((s64)(popped - edge) < 0 ||
+	    (s64)(popped - edge) >= (s64)AM65_CPTS_GTC_PUSH_PERIOD) {
+		dev_dbg(cpts->dev, "%s: event read at %llu, edge %llu\n",
+			__func__, popped, edge);
+		return -EAGAIN;
+	}
+
+	push->cycles = edge;
+	return 0;
+}
+
+/*
+ * Make one attempt to capture a push event. Whatever the outcome, the input
+ * is left disabled and the request disarmed.
+ */
+static int am65_cpts_gtc_push_try(struct am65_cpts *cpts, clockid_t clock_id,
+				  struct system_time_snapshot *before,
+				  struct am65_cpts_gtc_push *push)
+{
+	struct system_time_snapshot after;
+	u64 edge;
+	int ret;
+
+	ret = am65_cpts_gtc_push_arm(cpts);
+	if (ret)
+		return ret;
+
+	/*
+	 * Read the counter before and after enabling the input. An edge at or
+	 * before the first read reaches the CPTS long before the enable does,
+	 * which needs a CONTROL read first. After the read-back of CONTROL the
+	 * input is enabled, so an edge after the second read is captured. If
+	 * no edge lies between the two reads, the first event is of the first
+	 * edge after the first read.
+	 */
+	ktime_get_snapshot_id(clock_id, before);
+	am65_cpts_gtc_push_enable(cpts, true);
+	ktime_get_snapshot_id(clock_id, &after);
+
+	if (!before->valid || !after.valid ||
+	    before->cs_id != CSID_ARM_ARCH_COUNTER ||
+	    after.cs_id != CSID_ARM_ARCH_COUNTER) {
+		ret = -ENODEV;
+		goto disable;
+	}
+
+	edge = am65_cpts_gtc_push_next_edge(before->cycles);
+	if (edge <= after.cycles) {
+		dev_dbg(cpts->dev, "%s: edge %llu during enable\n",
+			__func__, edge);
+		ret = -EAGAIN;
+		goto disable;
+	}
+
+	ret = am65_cpts_gtc_push_wait(cpts, edge, push);
+
+disable:
+	am65_cpts_gtc_push_enable(cpts, false);
+	am65_cpts_gtc_push_disarm(cpts);
+
+	return ret;
+}
+
+static int am65_cpts_ptp_getcrosststamp(struct ptp_clock_info *ptp,
+					struct system_device_crosststamp *cts)
+{
+	struct am65_cpts *cpts = container_of(ptp, struct am65_cpts, ptp_info);
+	struct system_time_snapshot before;
+	struct am65_cpts_gtc_push push;
+	int i, ret;
+
+	mutex_lock(&cpts->gtc_push_lock);
+
+	ret = am65_cpts_gtc_push_select(cpts);
+	if (ret)
+		goto out;
+
+	for (i = 0; i < AM65_CPTS_GTC_PUSH_TRIES; i++) {
+		ret = am65_cpts_gtc_push_try(cpts, cts->clock_id, &before,
+					     &push);
+		if (ret != -EAGAIN)
+			break;
+	}
+	/* as other drivers do when a cross-timestamp cannot be had now */
+	if (ret == -EAGAIN)
+		ret = -EBUSY;
+	if (ret)
+		goto out;
+
+	/* before can serve as history_begin: the edge is after it */
+	ret = get_device_system_crosststamp(am65_cpts_gtc_push_read, &push,
+					    &before, cts);
+out:
+	mutex_unlock(&cpts->gtc_push_lock);
+	return ret;
+}
+
 static long am65_cpts_ts_work(struct ptp_clock_info *ptp);
 
 static struct ptp_clock_info am65_ptp_info = {
@@ -1135,6 +1397,7 @@ static int cpts_of_mux_clk_setup(struct am65_cpts *cpts,
 
 static int am65_cpts_of_parse(struct am65_cpts *cpts, struct device_node *node)
 {
+	struct regmap *gtc;
 	u32 prop[2];
 
 	if (!of_property_read_u32(node, "ti,cpts-ext-ts-inputs", &prop[0]))
@@ -1160,6 +1423,24 @@ static int am65_cpts_of_parse(struct am65_cpts *cpts, struct device_node *node)
 		}
 	}
 
+	if (of_property_present(node, "ti,gtc-push")) {
+		gtc = syscon_regmap_lookup_by_phandle_args(node, "ti,gtc-push",
+							   1, &prop[0]);
+		if (IS_ERR(gtc))
+			return dev_err_probe(cpts->dev, PTR_ERR(gtc),
+					     "Failed to get GTC syscon\n");
+
+		if (prop[0] > 7 ||
+		    (cpts->pps_present && prop[0] == cpts->pps_hw_ts_idx)) {
+			dev_err(cpts->dev,
+				"invalid GTC push HWx_TS_PUSH index: %u\n",
+				prop[0]);
+		} else {
+			cpts->gtc = gtc;
+			cpts->gtc_push_hw_idx = prop[0];
+		}
+	}
+
 	return cpts_of_mux_clk_setup(cpts, node);
 }
 
@@ -1196,11 +1477,13 @@ struct am65_cpts *am65_cpts_create(struct device *dev, void __iomem *regs,
 		return ERR_PTR(ret);
 
 	mutex_init(&cpts->ptp_clk_lock);
+	mutex_init(&cpts->gtc_push_lock);
 	INIT_LIST_HEAD(&cpts->events_tx);
 	INIT_LIST_HEAD(&cpts->events_rx);
 	INIT_LIST_HEAD(&cpts->pool);
 	spin_lock_init(&cpts->lock);
 	skb_queue_head_init(&cpts->txq);
+	init_waitqueue_head(&cpts->gtc_push_wq);
 
 	for (i = 0; i < AM65_CPTS_MAX_EVENTS; i++)
 		list_add(&cpts->pool_data[i].list, &cpts->pool);
@@ -1229,6 +1512,8 @@ struct am65_cpts *am65_cpts_create(struct device *dev, void __iomem *regs,
 		cpts->ptp_info.n_per_out = cpts->genf_num;
 	if (cpts->pps_present)
 		cpts->ptp_info.pps = 1;
+	if (cpts->gtc)
+		cpts->ptp_info.getcrosststamp = am65_cpts_ptp_getcrosststamp;
 
 	am65_cpts_set_add_val(cpts);
 
@@ -1276,6 +1561,10 @@ void am65_cpts_suspend(struct am65_cpts *cpts)
 {
 	/* save state and disable CPTS */
 	cpts->sr_control = am65_cpts_read32(cpts, control);
+	/* the GTC push input is enabled only during a request */
+	if (cpts->gtc)
+		cpts->sr_control &= ~BIT(AM65_CPTS_CONTROL_HW1_TS_PUSH_OFFSET +
+					 cpts->gtc_push_hw_idx);
 	cpts->sr_int_enable = am65_cpts_read32(cpts, int_enable);
 	cpts->sr_rftclk_sel = am65_cpts_read32(cpts, rftclk_sel);
 	cpts->sr_ts_ppm_hi = am65_cpts_read32(cpts, ts_ppm_hi);
-- 
2.56.0

Re: [PATCH net-next 1/3] dt-bindings: mfd: syscon: add ti,am654-gtc

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The new compatible "ti,am654-gtc"
  (Documentation/devicetree/bindings/mfd/syscon.yaml, line 134) is named…
- [Low] The subject line "dt-bindings: mfd: syscon: add ti,am654-gtc"
  starts the description after the prefix with a lowercase "add".

--

dt-bindings: mfd: syscon: add ti,am654-gtc

This adds a ti,am654-gtc compatible to the generic syscon binding. It is
for the CFG0 register frame of the TI K3 Global Timebase Counter. The CPTS
driver writes GTC_CFG0_PUSHEVT in that frame to pick the counter bit the
GTC exports as its push event, which provides hardware cross-timestamps.

> dt-bindings: mfd: syscon: add ti,am654-gtc

[Severity: Low]
This isn't a bug, but MFD subjects normally capitalize the description
after the prefix. Recent commits to syscon.yaml do this, for example:

  dt-bindings: mfd: syscon: Add ESWIN EIC7700 compatible

Should this be "dt-bindings: mfd: syscon: Add ti,am654-gtc"?

> diff --git a/Documentation/devicetree/bindings/mfd/syscon.yaml b/Documentation/devicetree/bindings/mfd/syscon.yaml
> index 80ff0e1caca22..b1030f11b2dbf 100644
> --- a/Documentation/devicetree/bindings/mfd/syscon.yaml
> +++ b/Documentation/devicetree/bindings/mfd/syscon.yaml
> @@ -131,6 +131,7 @@ properties:
>                - ti,am625-dss-oldi-io-ctrl
>                - ti,am62p-cpsw-mac-efuse
>                - ti,am654-dss-oldi-io-ctrl
> +              - ti,am654-gtc

[Severity: Low]
Is ti,am654-gtc the right name for this node? The commit message says it
describes only "the CFG0 register frame" of the GTC. The compatible,
though, is named after the whole GTC IP.

The GTC has more than one register frame. k3-am62l.dtsi maps a
0x34000-byte window for it:

arch/arm64/boot/dts/ti/k3-am62l.dtsi:
    ranges = <0x00 0x00a80000 0x00 0x00a80000 0x00 0x00034000>, /* GTC */

The generic syscon schema allows only one register region and no extra
properties:

  reg:
    maxItems: 1
  [ ... ]
unevaluatedProperties: false

So a node with this compatible can never describe the other GTC frames
or resources.

The syscon.yaml description also says syscon is for registers that are
"not cohesive enough to represent as any specific type of device". The
GTC is a specific counter IP, and PUSHEVT is one of its own functions.

Compatibles become DT ABI once merged. A full GTC binding added later
would then need a different name for the same IP.

Would a frame-specific name such as ti,am654-gtc-cfg0, or a dedicated GTC
binding, describe the hardware more accurately?

The ti,gtc-push property in ti,k3-am654-cpts.yaml, added later in the
series, also describes its target as only the GTC CFG0 syscon. No later
patch in the series changes syscon.yaml, so the naming is the same at the
end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1794284248.git.jjc%40jclark.com

Re: [PATCH net-next 3/3] net: ethernet: ti: am65-cpts: implement PTP_SYS_OFFSET_PRECISE

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 3 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The optional ti,gtc-push property fails in two different ways
  in am65_cpts_of_parse() (drivers/net/ethernet/ti/am65-cpts.c).
- [Medium] am65_cpts_gtc_push_select() in
  drivers/net/ethernet/ti/am65-cpts.c writes the EXPBIT_SEL field of…
- [Medium] am65_cpts_gtc_push_select() in
  drivers/net/ethernet/ti/am65-cpts.c reads GTC_CFG0_PUSHEVT back only…

--

net: ethernet: ti: am65-cpts: implement PTP_SYS_OFFSET_PRECISE

This commit adds getcrosststamp support to the TI K3 CPTS driver. It uses
the Global Timebase Counter push event output, routed to a CPTS hardware
timestamp input. For each request the driver enables the push input,
waits for an event and pairs the PHC timestamp with the computed GTC
edge, retrying up to three times. The feature is enabled when the CPTS
node has a ti,gtc-push property naming the GTC CFG0 syscon and the push
input.

> diff --git a/drivers/net/ethernet/ti/am65-cpts.c b/drivers/net/ethernet/ti/am65-cpts.c
> index 0d71147f3c291..70b0d43bfe788 100644
> --- a/drivers/net/ethernet/ti/am65-cpts.c
> +++ b/drivers/net/ethernet/ti/am65-cpts.c

[ ... ]

> @@ -797,6 +839,226 @@ static int am65_cpts_ptp_enable(struct ptp_clock_info *ptp,

[ ... ]

> +/* select the counter bit the GTC exports on its push event output */
> +static int am65_cpts_gtc_push_select(struct am65_cpts *cpts)
> +{
> +	u32 val;
> +	int ret;
> +
> +	ret = regmap_update_bits(cpts->gtc, AM65_CPTS_GTC_PUSHEVT,
> +				 AM65_CPTS_GTC_PUSHEVT_EXPBIT_SEL,
> +				 FIELD_PREP(AM65_CPTS_GTC_PUSHEVT_EXPBIT_SEL,
> +					    AM65_CPTS_GTC_PUSH_BIT));

[Severity: Medium]
Is it safe for this driver to own the EXPBIT_SEL field of GTC_CFG0_PUSHEVT?

The register belongs to the SoC-wide GTC, not to the CPTS.
am65_cpts_ptp_getcrosststamp() rewrites it on every PTP_SYS_OFFSET_PRECISE
request. The previous value is never saved or restored, on success or on
error.

The GTC push event output goes into the Time Sync Router, which can send
it to other timesync capture inputs, SYNC_OUT pins, or software on other
cores. Suppose one of those consumers expects a different exported bit.
Won't its signal frequency change after the first request from Linux, and
then be forced back to bit 15 on every later request?

The ti,gtc-push binding added in this series describes the GTC as the
SoC's system counter. It doesn't give this CPTS exclusive ownership of the
push output.

> +	if (ret)
> +		return ret;
> +	/* flush the posted write before the input is enabled */
> +	return regmap_read(cpts->gtc, AM65_CPTS_GTC_PUSHEVT, &val);

[Severity: Medium]
Should val be checked here, to confirm that EXPBIT_SEL now reads back as
AM65_CPTS_GTC_PUSH_BIT?

am65_cpts_gtc_push_next_edge() and the window check in
am65_cpts_gtc_push_wait() both assume rising edges at k * 2^16 + 2^15:

	if ((s64)(popped - edge) < 0 ||
	    (s64)(popped - edge) >= (s64)AM65_CPTS_GTC_PUSH_PERIOD) {
		...
		return -EAGAIN;
	}

	push->cycles = edge;

The write might not take effect. For example, the CFG0 region could be
firewalled or read-only in some security configuration, or another agent
could rewrite the field. Then the exported bit b' is not 15.

With b' < 15, the first event comes from an edge before the expected
bit-15 edge. If IRQ thread latency puts popped past edge, the attempt is
accepted with push->cycles = edge. The result is off by up to that
latency.

With b' = 16, every bit-16 edge falls 2^15 cycles after a bit-15 edge,
which is inside the window. About half of attempts would pass while being
about 164 us off at 200 MHz.

In both cases, wouldn't PTP_SYS_OFFSET_PRECISE return a wrong
cross-timestamp to userspace with no error? The syscon regmap is uncached,
so a FIELD_GET() on val would catch the mismatch.

[ ... ]

> @@ -1160,6 +1423,24 @@ static int am65_cpts_of_parse(struct am65_cpts *cpts, struct device_node *node)
>  		}
>  	}
>  
> +	if (of_property_present(node, "ti,gtc-push")) {
> +		gtc = syscon_regmap_lookup_by_phandle_args(node, "ti,gtc-push",
> +							   1, &prop[0]);
> +		if (IS_ERR(gtc))
> +			return dev_err_probe(cpts->dev, PTR_ERR(gtc),
> +					     "Failed to get GTC syscon\n");

[Severity: Medium]
Should a failed syscon lookup be fatal here? A bad index just below only
logs an error and leaves the feature disabled.

Returning here makes am65_cpts_create() return ERR_PTR(ret).
am65_cpsw_init_cpts() in am65-cpsw-nuss.c then fails the whole CPSW probe:

	if (IS_ERR(cpts)) {
		int ret = PTR_ERR(cpts);

		of_node_put(node);
		dev_err(dev, "cpts create err %d\n", ret);
		return ret;
	}

So a problem with the optional cross-timestamp property takes down the
Ethernet interface.

A correct DT can also hit this. TI_K3_AM65_CPTS does not select or depend
on MFD_SYSCON, and ARCH_K3 does not select it either:

config TI_K3_AM65_CPTS
	tristate "TI K3 AM65x CPTS"
	depends on ARCH_K3 && OF
	depends on PTP_1588_CLOCK

With CONFIG_MFD_SYSCON=n, the syscon stub returns ERR_PTR(-ENOTSUPP). Any
DT with ti,gtc-push would then stop Ethernet from probing. The same
happens if the phandle points at a node without the "syscon" compatible.

The existing ti,pps handling and the new index check both treat errors as
non-fatal. Could this path do the same, or should a Kconfig dependency on
MFD_SYSCON be added?

> +
> +		if (prop[0] > 7 ||
> +		    (cpts->pps_present && prop[0] == cpts->pps_hw_ts_idx)) {
> +			dev_err(cpts->dev,
> +				"invalid GTC push HWx_TS_PUSH index: %u\n",
> +				prop[0]);
> +		} else {
> +			cpts->gtc = gtc;
> +			cpts->gtc_push_hw_idx = prop[0];
> +		}
> +	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1794284248.git.jjc%40jclark.com