[RFC PATCH 0/3] PPS Set event clock source

[RFC PATCH 0/3] PPS Set event clock source

[Gerik Kubiak]

Hello all,

This patch series adds support for userspace to specify the clock source
used to collect pps events. The current behavior timestamps events in
the REALTIME clock and this series adds support for the BOOTTIME and
MONOTONIC_RAW clocks via a new ioctl on a pps fd.

This is a minimal proof of concept RFC to understand if there is
interest in this feature and if the current approach to the userspace 
API is appropriate.

My usecase is using the pps subsystem to synchronize sensor data
to an external clock. The sensor data is timestamped by the BOOTTIME
clock in the kernel and I would like to use the pps subsystem to
correlate these timestamps to an external clock via a pps-gpio device.
sensor data, which is timestamped in the BOOTTIME clock to the external
The pps subsystem would work well for this if it could return timestamps
in the BOOTTIME clock. Otherwise I would need to correlate the clocks
in userspace, which is non-trivial and may be constantly changing.

This series adds support for this behavior across three patches.
1. Create a new userspace API for setting the requested clock using
   an ioctl
2. Update the pps_get_ts function to capture the event in the clocks
   of interest
3. Update the pps subsystem to track the requested clock and return the
   stored timestamp corresponding the requested clock

I have a few outstanding issues with this series that need
clarifications before merging
1. The current implementation collects timestamps for all relevant
   clocks in pps_get_ts. This seems like overkill, but it prevents
   issues when the clock source changes. A slimmer implementation would
   collect timestamps with the selected clock and drop events if
   userspace changes the clock source after an event has been collected.
2. This implements pps sources for REALTIME, MONOTONIC_RAW and BOOTTIME
   clocks. This was done to keep the single call to ktime_get_snapshot.
   It would make sense for this API to support MONOTONIC as well, but I
   believe that will require changes to ktime_get_snapshot to include
   the MONOTONIC clock in the system_time_snapshot struct.
3. I created a new ioctl on the pps fd to prevent changing the existing
   pps API calls. Is this the preferred way to implement this new
   behavior? It seems cleaner to add an additional flag and field to the
   existing pps_kparams struct, but it wasn't clear to me if this could
   be done without breaking the existing userspace contract.

Thank you,
Gerik


Gerik Kubiak (3):
  pps: Add PPS_SETCLOCK ioctl
  pps: Capture PPS timestamps in multiple clocks
  pps: Timestamp pps events per PPS_SETCLOCK clock

 drivers/pps/kapi.c         | 27 ++++++++++++++++++++-------
 drivers/pps/pps.c          | 22 ++++++++++++++++++++++
 include/linux/pps_kernel.h | 10 ++++------
 include/uapi/linux/pps.h   |  6 ++++++
 4 files changed, 52 insertions(+), 13 deletions(-)

-- 
2.54.0

[RFC PATCH 1/3] pps: Add PPS_SETCLOCK ioctl

[Gerik Kubiak]

Add the PPS_SETCLOCK ioctl to the pps driver and an associated
pps_kclock_source structure used as an arugment in the ioctl. This ioctl
allows userspace to specify the clock used in timestamping PPS events.

Adds a pps_kclock_source structure for this ioctl. It contains one value,
clock_id, which holds the requested clock source for subsequent PPS events.

Signed-off-by: Gerik Kubiak <gerikkub@gmail.com>
---
 include/uapi/linux/pps.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/uapi/linux/pps.h b/include/uapi/linux/pps.h
index 009ebcd8ced5..db233511c6ef 100644
--- a/include/uapi/linux/pps.h
+++ b/include/uapi/linux/pps.h
@@ -87,6 +87,10 @@ struct pps_kparams {
 	struct pps_ktime clear_off_tu;	/* offset compensation for clear */
 };
 
+struct pps_kclock_source {
+	__u32 clock_id;  /* System clock id */
+};
+
 /*
  * 3.3 Mode bit definitions
  */
@@ -140,6 +144,7 @@ struct pps_bind_args {
 	int consumer;	/* selected kernel consumer */
 };
 
+
 #include <linux/ioctl.h>
 
 #define PPS_GETPARAMS		_IOR('p', 0xa1, struct pps_kparams *)
@@ -147,5 +152,6 @@ struct pps_bind_args {
 #define PPS_GETCAP		_IOR('p', 0xa3, int *)
 #define PPS_FETCH		_IOWR('p', 0xa4, struct pps_fdata *)
 #define PPS_KC_BIND		_IOW('p', 0xa5, struct pps_bind_args *)
+#define PPS_SETCLOCK		_IOW('p', 0xa6, struct pps_clock_source *)
 
 #endif /* _PPS_H_ */
-- 
2.54.0

[RFC PATCH 2/3] pps: Capture PPS timestamps in multiple clocks

[Gerik Kubiak]

Capture the REALTIME, MONOTONIC and BOOTTIME timestamps of all pps
events.

Removes the conditional compilation of ts_raw with the CONFIG_NTP_PPS
option, since now that clock is captured unconditionally.

Signed-off-by: Gerik Kubiak <gerikkub@gmail.com>
---
 include/linux/pps_kernel.h | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
index aab0aebb529e..b13cac7fa6b5 100644
--- a/include/linux/pps_kernel.h
+++ b/include/linux/pps_kernel.h
@@ -33,10 +33,9 @@ struct pps_source_info {
 };
 
 struct pps_event_time {
-#ifdef CONFIG_NTP_PPS
 	struct timespec64 ts_raw;
-#endif /* CONFIG_NTP_PPS */
 	struct timespec64 ts_real;
+	struct timespec64 ts_boot;
 };
 
 /* The main struct */
@@ -103,18 +102,16 @@ static inline void pps_get_ts(struct pps_event_time *ts)
 
 	ktime_get_snapshot(&snap);
 	ts->ts_real = ktime_to_timespec64(snap.real);
-#ifdef CONFIG_NTP_PPS
 	ts->ts_raw = ktime_to_timespec64(snap.raw);
-#endif
+	ts->ts_boot = ktime_to_timespec64(snap.boot);
 }
 
 /* Subtract known time delay from PPS event time(s) */
 static inline void pps_sub_ts(struct pps_event_time *ts, struct timespec64 delta)
 {
 	ts->ts_real = timespec64_sub(ts->ts_real, delta);
-#ifdef CONFIG_NTP_PPS
 	ts->ts_raw = timespec64_sub(ts->ts_raw, delta);
-#endif
+	ts->ts_boot = timespec64_sub(ts->ts_boot, delta);
 }
 
 #endif /* LINUX_PPS_KERNEL_H */
-- 
2.54.0

[RFC PATCH 3/3] pps: Timestamp pps events per PPS_SETCLOCK clock

[Gerik Kubiak]

Implements the PPS_SETCLOCK ioctl to allow userspace to set the clock
used to timestamp PPS event.

Userspace passes the clock in a pps_kclock_source structure through the
PPS_SETCLOCK ioctl. The PPS subsystem will store this clock and future
pps events will be timestamped with this clock.

The clock source is initialiaized to REALTIME to keep the existing PPS
behavior if this ioctl is not used.

Signed-off-by: Gerik Kubiak <gerikkub@gmail.com>
---
 drivers/pps/kapi.c         | 27 ++++++++++++++++++++-------
 drivers/pps/pps.c          | 22 ++++++++++++++++++++++
 include/linux/pps_kernel.h |  1 +
 3 files changed, 43 insertions(+), 7 deletions(-)

diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index 1bf0335a1b41..c1af445d3e57 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -94,6 +94,7 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
 	 */
 	pps->params.api_version = PPS_API_VERS;
 	pps->params.mode = default_params;
+	pps->clock_source.clock_id = CLOCK_REALTIME;
 	pps->info = *info;
 
 	/* check for default echo function */
@@ -158,14 +159,26 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
 {
 	unsigned long flags;
 	int captured = 0;
-	struct pps_ktime ts_real = { .sec = 0, .nsec = 0, .flags = 0 };
+	struct pps_ktime ts_ev = { .sec = 0, .nsec = 0, .flags = 0 };
 
 	/* check event type */
 	BUG_ON((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0);
 
-	dev_dbg(&pps->dev, "PPS event at %ptSp\n", &ts->ts_real);
+	switch (pps->clock_source.clock_id) {
+	case CLOCK_REALTIME:
+		timespec_to_pps_ktime(&ts_ev, ts->ts_real);
+		break;
+	case CLOCK_MONOTONIC_RAW:
+		timespec_to_pps_ktime(&ts_ev, ts->ts_raw);
+		break;
+	case CLOCK_BOOTTIME:
+		timespec_to_pps_ktime(&ts_ev, ts->ts_boot);
+		break;
+	default:
+		WARN_ON_ONCE(true);
+	}
 
-	timespec_to_pps_ktime(&ts_real, ts->ts_real);
+	dev_dbg(&pps->dev, "PPS event at %ptSp\n", &ts_ev);
 
 	spin_lock_irqsave(&pps->lock, flags);
 
@@ -178,11 +191,11 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
 	if (event & pps->params.mode & PPS_CAPTUREASSERT) {
 		/* We have to add an offset? */
 		if (pps->params.mode & PPS_OFFSETASSERT)
-			pps_add_offset(&ts_real,
+			pps_add_offset(&ts_ev,
 					&pps->params.assert_off_tu);
 
 		/* Save the time stamp */
-		pps->assert_tu = ts_real;
+		pps->assert_tu = ts_ev;
 		pps->assert_sequence++;
 		dev_dbg(&pps->dev, "capture assert seq #%u\n",
 			pps->assert_sequence);
@@ -192,11 +205,11 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
 	if (event & pps->params.mode & PPS_CAPTURECLEAR) {
 		/* We have to add an offset? */
 		if (pps->params.mode & PPS_OFFSETCLEAR)
-			pps_add_offset(&ts_real,
+			pps_add_offset(&ts_ev,
 					&pps->params.clear_off_tu);
 
 		/* Save the time stamp */
-		pps->clear_tu = ts_real;
+		pps->clear_tu = ts_ev;
 		pps->clear_sequence++;
 		dev_dbg(&pps->dev, "capture clear seq #%u\n",
 			pps->clear_sequence);
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index de1122bb69ea..2349aebeb65e 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -246,6 +246,28 @@ static long pps_cdev_ioctl(struct file *file,
 
 		break;
 	}
+	case PPS_SETCLOCK: {
+		struct pps_kclock_source source_args;
+
+		dev_dbg(&pps->dev, "PPS_SETCLOCK\n");
+
+		if (copy_from_user(&source_args, uarg,
+					sizeof(struct pps_kclock_source)))
+			return -EFAULT;
+
+		switch (source_args.clock_id) {
+		case CLOCK_REALTIME:
+		case CLOCK_MONOTONIC_RAW:
+		case CLOCK_BOOTTIME:
+			break;
+		default:
+			return -EINVAL;
+		}
+
+		pps->clock_source = source_args;
+
+		break;
+	}
 	default:
 		return -ENOTTY;
 	}
diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
index b13cac7fa6b5..2c8367de12e3 100644
--- a/include/linux/pps_kernel.h
+++ b/include/linux/pps_kernel.h
@@ -43,6 +43,7 @@ struct pps_device {
 	struct pps_source_info info;		/* PSS source info */
 
 	struct pps_kparams params;		/* PPS current params */
+	struct pps_kclock_source clock_source;	/* Capture clock id */
 
 	__u32 assert_sequence;			/* PPS assert event seq # */
 	__u32 clear_sequence;			/* PPS clear event seq # */
-- 
2.54.0