[Intel PMC TGPIO Driver 0/5] Add support for Intel PMC Time GPIO Driver with PHC interface changes to support additional H/W Features

[Intel PMC TGPIO Driver 0/5] Add support for Intel PMC Time GPIO Driver with PHC interface changes to support additional H/W Features

[christopher.s.hall]

From: Christopher Hall <christopher.s.hall@intel.com>

Upcoming Intel platforms will have Time-Aware GPIO (TGPIO) hardware.
The TGPIO logic is driven by the Always Running Timer (ART) that's
related to TSC using CPUID[15H] (See Intel SDM Invariant
Time-Keeping).

The ART frequency is not adjustable. In order, to implement output
adjustments an additional edge-timestamp API is added, as well, as
a periodic output frequency adjustment API. Togther, these implement
equivalent functionality to the existing SYS_OFFSET_* and frequency
adjustment APIs.

The TGPIO hardware doesn't implement interrupts. For TGPIO input, the
output edge-timestamp API is re-used to implement a user-space polling
interface. For periodic input (e.g. PPS) this is fairly efficient,
requiring only a marginally faster poll rate than the input event
frequency.

Acknowledgment: Portions of the driver code were authored by Felipe
Balbi <balbi@kernel.org>

=======================================================================

Christopher Hall (5):
  drivers/ptp: Add Enhanced handling of reserve fields
  drivers/ptp: Add PEROUT2 ioctl frequency adjustment interface
  drivers/ptp: Add user-space input polling interface
  x86/tsc: Add TSC support functions to support ART driven Time-Aware
    GPIO
  drivers/ptp: Add PMC Time-Aware GPIO Driver

 arch/x86/include/asm/tsc.h        |   6 +
 arch/x86/kernel/tsc.c             | 116 +++-
 drivers/ptp/Kconfig               |  13 +
 drivers/ptp/Makefile              |   1 +
 drivers/ptp/ptp-intel-pmc-tgpio.c | 867 ++++++++++++++++++++++++++++++
 drivers/ptp/ptp_chardev.c         |  86 ++-
 drivers/ptp/ptp_clock.c           |  13 +
 include/linux/ptp_clock_kernel.h  |   2 +
 include/uapi/linux/ptp_clock.h    |  26 +-
 9 files changed, 1099 insertions(+), 31 deletions(-)
 create mode 100644 drivers/ptp/ptp-intel-pmc-tgpio.c

-- 
2.21.0

[Intel PMC TGPIO Driver 1/5] drivers/ptp: Add Enhanced handling of reserve fields

[christopher.s.hall]

From: Christopher Hall <christopher.s.hall@intel.com>

Add functions that parameterize checking and zeroing of reserve fields in
ioctl arguments. Eliminates need to change this code when repurposing
reserve fields.

Signed-off-by: Christopher Hall <christopher.s.hall@intel.com>
---
 drivers/ptp/ptp_chardev.c | 60 +++++++++++++++++++++------------------
 1 file changed, 33 insertions(+), 27 deletions(-)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index 9d72ab593f13..f9ad6df57fa5 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -12,6 +12,7 @@
 #include <linux/timekeeping.h>
 
 #include <linux/nospec.h>
+#include <linux/string.h>
 
 #include "ptp_private.h"
 
@@ -106,6 +107,28 @@ int ptp_open(struct posix_clock *pc, fmode_t fmode)
 	return 0;
 }
 
+/* Returns -1 if any reserved fields are non-zero */
+static inline int _check_rsv_field(unsigned int *field, size_t size)
+{
+	unsigned int *iter;
+	int ret = 0;
+
+	for (iter = field; iter < field+size && ret == 0; ++iter)
+		ret = *iter == 0 ? 0 : -1;
+
+	return ret;
+}
+#define check_rsv_field(field) _check_rsv_field(field, ARRAY_SIZE(field))
+
+static inline void _zero_rsv_field(unsigned int *field, size_t size)
+{
+	unsigned int *iter;
+
+	for (iter = field; iter < field+size; ++iter)
+		*iter = 0;
+}
+#define zero_rsv_field(field) _zero_rsv_field(field, ARRAY_SIZE(field))
+
 long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 {
 	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
@@ -154,7 +177,7 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 			req.extts.flags |= PTP_STRICT_FLAGS;
 			/* Make sure no reserved bit is set. */
 			if ((req.extts.flags & ~PTP_EXTTS_VALID_FLAGS) ||
-			    req.extts.rsv[0] || req.extts.rsv[1]) {
+			    check_rsv_field(req.extts.rsv)) {
 				err = -EINVAL;
 				break;
 			}
@@ -166,8 +189,7 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 			}
 		} else if (cmd == PTP_EXTTS_REQUEST) {
 			req.extts.flags &= PTP_EXTTS_V1_VALID_FLAGS;
-			req.extts.rsv[0] = 0;
-			req.extts.rsv[1] = 0;
+			zero_rsv_field(req.extts.rsv);
 		}
 		if (req.extts.index >= ops->n_ext_ts) {
 			err = -EINVAL;
@@ -188,17 +210,13 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 			break;
 		}
 		if (((req.perout.flags & ~PTP_PEROUT_VALID_FLAGS) ||
-			req.perout.rsv[0] || req.perout.rsv[1] ||
-			req.perout.rsv[2] || req.perout.rsv[3]) &&
-			cmd == PTP_PEROUT_REQUEST2) {
+		     check_rsv_field(req.perout.rsv)) &&
+		    cmd == PTP_PEROUT_REQUEST2) {
 			err = -EINVAL;
 			break;
 		} else if (cmd == PTP_PEROUT_REQUEST) {
 			req.perout.flags &= PTP_PEROUT_V1_VALID_FLAGS;
-			req.perout.rsv[0] = 0;
-			req.perout.rsv[1] = 0;
-			req.perout.rsv[2] = 0;
-			req.perout.rsv[3] = 0;
+			zero_rsv_field(req.perout.rsv);
 		}
 		if (req.perout.index >= ops->n_per_out) {
 			err = -EINVAL;
@@ -258,7 +276,7 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 			break;
 		}
 		if (extoff->n_samples > PTP_MAX_SAMPLES
-		    || extoff->rsv[0] || extoff->rsv[1] || extoff->rsv[2]) {
+		    || check_rsv_field(extoff->rsv)) {
 			err = -EINVAL;
 			break;
 		}
@@ -318,17 +336,11 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 			err = -EFAULT;
 			break;
 		}
-		if ((pd.rsv[0] || pd.rsv[1] || pd.rsv[2]
-				|| pd.rsv[3] || pd.rsv[4])
-			&& cmd == PTP_PIN_GETFUNC2) {
+		if (check_rsv_field(pd.rsv) && cmd == PTP_PIN_GETFUNC2) {
 			err = -EINVAL;
 			break;
 		} else if (cmd == PTP_PIN_GETFUNC) {
-			pd.rsv[0] = 0;
-			pd.rsv[1] = 0;
-			pd.rsv[2] = 0;
-			pd.rsv[3] = 0;
-			pd.rsv[4] = 0;
+			zero_rsv_field(pd.rsv);
 		}
 		pin_index = pd.index;
 		if (pin_index >= ops->n_pins) {
@@ -350,17 +362,11 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 			err = -EFAULT;
 			break;
 		}
-		if ((pd.rsv[0] || pd.rsv[1] || pd.rsv[2]
-				|| pd.rsv[3] || pd.rsv[4])
-			&& cmd == PTP_PIN_SETFUNC2) {
+		if (check_rsv_field(pd.rsv) && cmd == PTP_PIN_SETFUNC2) {
 			err = -EINVAL;
 			break;
 		} else if (cmd == PTP_PIN_SETFUNC) {
-			pd.rsv[0] = 0;
-			pd.rsv[1] = 0;
-			pd.rsv[2] = 0;
-			pd.rsv[3] = 0;
-			pd.rsv[4] = 0;
+			zero_rsv_field(pd.rsv);
 		}
 		pin_index = pd.index;
 		if (pin_index >= ops->n_pins) {
-- 
2.21.0

[Intel PMC TGPIO Driver 2/5] drivers/ptp: Add PEROUT2 ioctl frequency adjustment interface

[christopher.s.hall]

From: Christopher Hall <christopher.s.hall@intel.com>

The Intel PMC TGPIO controller logic is driven by ART which is not adjust-
able. Rather than adjusting the clock frequency, an output frequency
adjustment method is added that doesn't require restarting or resetting
output.

Output frequency adjustment is achieved by adding a flag to be passed in
the PEROUT2 ioctl argument. In this case, the driver should disregard the
'start' time field and adjust the hardware output frequency if periodic
output is already "running".

For devices with an adjustable clock, the OFFSET_PRECISE* ioctl is used to
compute the device clock offset with respect to the system clock. For
non-adjustable clocks, the EVENT_COUNT_TSTAMP2 ioctl adds an analogous
relation between output edges and elapsed time device time. This tuple is
captured simultaneously in hardware and is used to precisely compute the
*actual* average cumulative output frequency relative to the device clock.
The actual average is used to adjust the output period to achieve the
*desired* cumulative output frequency.

Signed-off-by: Christopher Hall <christopher.s.hall@intel.com>
---
 drivers/ptp/ptp_chardev.c        | 26 ++++++++++++++++++++++++++
 include/linux/ptp_clock_kernel.h |  2 ++
 include/uapi/linux/ptp_clock.h   | 25 +++++++++++++++++++++++--
 3 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index f9ad6df57fa5..04c51878723d 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -134,6 +134,7 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
 	struct ptp_sys_offset_extended *extoff = NULL;
 	struct ptp_sys_offset_precise precise_offset;
+	struct ptp_event_count_tstamp counttstamp;
 	struct system_device_crosststamp xtstamp;
 	struct ptp_clock_info *ops = ptp->info;
 	struct ptp_sys_offset *sysoff = NULL;
@@ -218,6 +219,13 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 			req.perout.flags &= PTP_PEROUT_V1_VALID_FLAGS;
 			zero_rsv_field(req.perout.rsv);
 		}
+		/* These flags don't make sense together */
+		if (cmd == PTP_PEROUT_REQUEST2 &&
+		    req.perout.flags & PTP_PEROUT_FREQ_ADJ &&
+		    req.perout.flags & PTP_PEROUT_ONE_SHOT) {
+			err = -EINVAL;
+			break;
+		}
 		if (req.perout.index >= ops->n_per_out) {
 			err = -EINVAL;
 			break;
@@ -227,6 +235,24 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 		err = ops->enable(ops, &req, enable);
 		break;
 
+	case PTP_EVENT_COUNT_TSTAMP2:
+		if (!ops->counttstamp)
+			return -ENOTSUPP;
+		if (copy_from_user(&counttstamp, (void __user *)arg,
+				   sizeof(counttstamp))) {
+			err = -EFAULT;
+			break;
+		}
+		if (check_rsv_field(counttstamp.rsv)) {
+			err = -EINVAL;
+			break;
+		}
+		err = ops->counttstamp(ops, &counttstamp);
+		if (!err && copy_to_user((void __user *)arg, &counttstamp,
+						sizeof(counttstamp)))
+			err = -EFAULT;
+		break;
+
 	case PTP_ENABLE_PPS:
 	case PTP_ENABLE_PPS2:
 		memset(&req, 0, sizeof(req));
diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
index 93cc4f1d444a..8223f6f656dd 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -134,6 +134,8 @@ struct ptp_clock_info {
 			  struct ptp_system_timestamp *sts);
 	int (*getcrosststamp)(struct ptp_clock_info *ptp,
 			      struct system_device_crosststamp *cts);
+	int (*counttstamp)(struct ptp_clock_info *ptp,
+			   struct ptp_event_count_tstamp *count);
 	int (*settime64)(struct ptp_clock_info *p, const struct timespec64 *ts);
 	int (*enable)(struct ptp_clock_info *ptp,
 		      struct ptp_clock_request *request, int on);
diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
index 9dc9d0079e98..ecb4c4e49205 100644
--- a/include/uapi/linux/ptp_clock.h
+++ b/include/uapi/linux/ptp_clock.h
@@ -34,6 +34,11 @@
 #define PTP_STRICT_FLAGS   (1<<3)
 #define PTP_EXTTS_EDGES    (PTP_RISING_EDGE | PTP_FALLING_EDGE)
 
+/*
+ * Bits of the ptp_pin_desc.flags field:
+ */
+#define PTP_PINDESC_EVTCNTVALID	(1<<0)
+
 /*
  * flag fields valid for the new PTP_EXTTS_REQUEST2 ioctl.
  */
@@ -54,11 +59,13 @@
  * Bits of the ptp_perout_request.flags field:
  */
 #define PTP_PEROUT_ONE_SHOT (1<<0)
+#define PTP_PEROUT_FREQ_ADJ (1<<1)
 
 /*
  * flag fields valid for the new PTP_PEROUT_REQUEST2 ioctl.
  */
-#define PTP_PEROUT_VALID_FLAGS	(PTP_PEROUT_ONE_SHOT)
+#define PTP_PEROUT_VALID_FLAGS	(PTP_PEROUT_ONE_SHOT |	\
+				 PTP_PEROUT_FREQ_ADJ)
 
 /*
  * No flags are valid for the original PTP_PEROUT_REQUEST ioctl
@@ -106,6 +113,14 @@ struct ptp_perout_request {
 	unsigned int rsv[4];          /* Reserved for future use. */
 };
 
+struct ptp_event_count_tstamp {
+	struct ptp_clock_time device_time;
+	unsigned long long event_count;
+	unsigned int index;
+	unsigned int flags;
+	unsigned int rsv[4];          /* Reserved for future use. */
+};
+
 #define PTP_MAX_SAMPLES 25 /* Maximum allowed offset measurement samples. */
 
 struct ptp_sys_offset {
@@ -164,10 +179,14 @@ struct ptp_pin_desc {
 	 * PTP_EXTTS_REQUEST and PTP_PEROUT_REQUEST ioctls.
 	 */
 	unsigned int chan;
+	/*
+	 * Per pin capability flag
+	 */
+	unsigned int flags;
 	/*
 	 * Reserved for future use.
 	 */
-	unsigned int rsv[5];
+	unsigned int rsv[4];
 };
 
 #define PTP_CLK_MAGIC '='
@@ -195,6 +214,8 @@ struct ptp_pin_desc {
 	_IOWR(PTP_CLK_MAGIC, 17, struct ptp_sys_offset_precise)
 #define PTP_SYS_OFFSET_EXTENDED2 \
 	_IOWR(PTP_CLK_MAGIC, 18, struct ptp_sys_offset_extended)
+#define PTP_EVENT_COUNT_TSTAMP2 \
+	_IOWR(PTP_CLK_MAGIC, 19, struct ptp_event_count_tstamp)
 
 struct ptp_extts_event {
 	struct ptp_clock_time t; /* Time event occured. */
-- 
2.21.0

[Intel PMC TGPIO Driver 3/5] drivers/ptp: Add user-space input polling interface

[christopher.s.hall]

From: Christopher Hall <christopher.s.hall@intel.com>

The Intel PMC Time-Aware GPIO controller doesn't implement interrupts to
notify software that an input event has occurred. To solve this problem,
implement a user-space polling interface allowing the application to check
for input events. The API returns an event count and time. This interface
(EVENT_COUNT_TSTAMP2) is *reused* from the output frequency adjustment API.
The event count delta indicates that one or more events have occurred and
how many events may have been missed.

For periodic input use cases, polling is fairly efficient. Using PPS input,
as an example, polling 3x per second is more than sufficient to capture
rising and falling edge timestamps. Generalizing, the poll period should
be:

	poll_period = (nominal_input_period / (1 + freq_offset)) / 2

	Where freq_offset = <remote:local clock ratio> - 1;

A flag that indicates a pin isn't capable of generating interrupts is
added. Software should not expect notification through the read() interface
on pins marked as such. If all pins are marked as poll only, the read
interface is marked 'defunct' and immediately returns an error because no
interrupt would ever occur to exit the wait state.

Signed-off-by: Christopher Hall <christopher.s.hall@intel.com>
---
 drivers/ptp/ptp_clock.c        | 13 +++++++++++++
 include/uapi/linux/ptp_clock.h |  1 +
 2 files changed, 14 insertions(+)

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index b84f16bbd6f2..518dc911ec40 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -190,6 +190,17 @@ static void ptp_aux_kworker(struct kthread_work *work)
 		kthread_queue_delayed_work(ptp->kworker, &ptp->aux_work, delay);
 }
 
+static bool check_for_readability(struct ptp_pin_desc *pin_desc, size_t size)
+{
+	int i;
+	unsigned int flags = PTP_PINDESC_INPUTPOLL;
+
+	for (i = 0; i < size && flags != 0; ++i)
+		flags &= pin_desc[i].flags;
+
+	return flags == 0;
+}
+
 /* public interface */
 
 struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
@@ -213,6 +224,8 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 		goto no_slot;
 	}
 
+	ptp->defunct = !check_for_readability(info->pin_config, info->n_pins);
+
 	ptp->clock.ops = ptp_clock_ops;
 	ptp->info = info;
 	ptp->devid = MKDEV(major, index);
diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
index ecb4c4e49205..d5bd6504480c 100644
--- a/include/uapi/linux/ptp_clock.h
+++ b/include/uapi/linux/ptp_clock.h
@@ -38,6 +38,7 @@
  * Bits of the ptp_pin_desc.flags field:
  */
 #define PTP_PINDESC_EVTCNTVALID	(1<<0)
+#define PTP_PINDESC_INPUTPOLL	(1<<1)
 
 /*
  * flag fields valid for the new PTP_EXTTS_REQUEST2 ioctl.
-- 
2.21.0

[Intel PMC TGPIO Driver 4/5] x86/tsc: Add TSC support functions to support ART driven Time-Aware GPIO

[christopher.s.hall]

From: Christopher Hall <christopher.s.hall@intel.com>

The PMC Time-Aware GPIO (TGPIO) logic is driven by ART. Several support
functions are required to translate ART to/from nanoseconds units which
is required by the PHC clock interface. A function is also added to get
the current value of ART/TSC using get_cycles() (RDTSC). This avoids
multiple MMIO reads required to retrieve ART.

Note that rather than ART nanoseconds, TSC nanoseconds are used. These are
related by TSC = ART * CPUID[15H].EBX/CPUID[15H].EAX + K. The value of
K is the distinction between between the ART and TSC nanoseconds.

The advantage of using TSC nanoseconds to represent the device clock
because it can easily be calculated in user-space using only CPUID[15H].
The value of ART in general can't be since K isn't necessarily known to
the application.

Signed-off-by: Christopher Hall <christopher.s.hall@intel.com>
---
 arch/x86/include/asm/tsc.h |   6 ++
 arch/x86/kernel/tsc.c      | 116 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 120 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/tsc.h b/arch/x86/include/asm/tsc.h
index 8a0c25c6bf09..f44c92fe3cd5 100644
--- a/arch/x86/include/asm/tsc.h
+++ b/arch/x86/include/asm/tsc.h
@@ -32,6 +32,12 @@ static inline cycles_t get_cycles(void)
 
 extern struct system_counterval_t convert_art_to_tsc(u64 art);
 extern struct system_counterval_t convert_art_ns_to_tsc(u64 art_ns);
+extern struct timespec64 get_tsc_ns_now(struct system_counterval_t
+					*system_counter);
+extern u64 convert_tsc_ns_to_art(struct timespec64 *tsc_ns);
+extern u64 convert_tsc_ns_to_art_duration(struct timespec64 *tsc_ns);
+extern struct timespec64 convert_art_to_tsc_ns(u64 art);
+extern struct timespec64 convert_art_to_tsc_ns_duration(u64 art);
 
 extern void tsc_early_init(void);
 extern void tsc_init(void);
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 7e322e2daaf5..fb0dc3169e6b 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -1215,7 +1215,7 @@ int unsynchronized_tsc(void)
 /*
  * Convert ART to TSC given numerator/denominator found in detect_art()
  */
-struct system_counterval_t convert_art_to_tsc(u64 art)
+static struct system_counterval_t _convert_art_to_tsc(u64 art, bool dur)
 {
 	u64 tmp, res, rem;
 
@@ -1225,13 +1225,125 @@ struct system_counterval_t convert_art_to_tsc(u64 art)
 	tmp = rem * art_to_tsc_numerator;
 
 	do_div(tmp, art_to_tsc_denominator);
-	res += tmp + art_to_tsc_offset;
+	res += tmp;
+	if (!dur)
+		res += art_to_tsc_offset;
 
 	return (struct system_counterval_t) {.cs = art_related_clocksource,
 			.cycles = res};
 }
+
+struct system_counterval_t convert_art_to_tsc(u64 art)
+{
+	return _convert_art_to_tsc(art, false);
+}
 EXPORT_SYMBOL(convert_art_to_tsc);
 
+static
+struct timespec64 get_tsc_ns(struct system_counterval_t *system_counter)
+{
+	u64 tmp, res, rem;
+	u64 cycles;
+
+	cycles = system_counter->cycles;
+	rem = do_div(cycles, tsc_khz);
+
+	res = cycles * USEC_PER_SEC;
+	tmp = rem * USEC_PER_SEC;
+
+	rem = do_div(tmp, tsc_khz);
+	tmp += rem > tsc_khz >> 2 ? 1 : 0;
+	res += tmp;
+
+	rem = do_div(res, NSEC_PER_SEC);
+
+	return (struct timespec64) {.tv_sec = res, .tv_nsec = rem};
+}
+
+struct timespec64 get_tsc_ns_now(struct system_counterval_t *system_counter)
+{
+	struct system_counterval_t counterval;
+
+	if (system_counter == NULL)
+		system_counter = &counterval;
+
+	system_counter->cycles = clocksource_tsc.read(NULL);
+	system_counter->cs = art_related_clocksource;
+
+	return get_tsc_ns(system_counter);
+}
+EXPORT_SYMBOL(get_tsc_ns_now);
+
+struct timespec64 convert_art_to_tsc_ns(u64 art)
+{
+	struct system_counterval_t counterval;
+
+	counterval = _convert_art_to_tsc(art, false);
+
+	return get_tsc_ns(&counterval);
+}
+EXPORT_SYMBOL(convert_art_to_tsc_ns);
+
+struct timespec64 convert_art_to_tsc_ns_duration(u64 art)
+{
+	struct system_counterval_t counterval;
+
+	counterval = _convert_art_to_tsc(art, true);
+
+	return get_tsc_ns(&counterval);
+}
+EXPORT_SYMBOL(convert_art_to_tsc_ns_duration);
+
+static u64 convert_tsc_ns_to_tsc(struct timespec64 *tsc_ns)
+{
+	u64 tmp, res, rem;
+	u64 cycles;
+
+	cycles = timespec64_to_ns(tsc_ns);
+
+	rem = do_div(cycles, USEC_PER_SEC);
+
+	res = cycles * tsc_khz;
+	tmp = rem * tsc_khz;
+
+	do_div(tmp, USEC_PER_SEC);
+
+	return res + tmp;
+}
+
+
+static u64 _convert_tsc_ns_to_art(struct timespec64 *tsc_ns, bool dur)
+{
+	u64 tmp, res, rem;
+	u64 cycles;
+
+	cycles = convert_tsc_ns_to_tsc(tsc_ns);
+	if (!dur)
+		cycles -= art_to_tsc_offset;
+
+	rem = do_div(cycles, art_to_tsc_numerator);
+
+	res = cycles * art_to_tsc_denominator;
+	tmp = rem * art_to_tsc_denominator;
+
+	rem = do_div(tmp, art_to_tsc_numerator);
+	tmp += rem > art_to_tsc_numerator >> 2 ? 1 : 0;
+
+	return res + tmp;
+}
+
+u64 convert_tsc_ns_to_art(struct timespec64 *tsc_ns)
+{
+	return _convert_tsc_ns_to_art(tsc_ns, false);
+}
+EXPORT_SYMBOL(convert_tsc_ns_to_art);
+
+u64 convert_tsc_ns_to_art_duration(struct timespec64 *tsc_ns)
+{
+	return _convert_tsc_ns_to_art(tsc_ns, true);
+}
+EXPORT_SYMBOL(convert_tsc_ns_to_art_duration);
+
 /**
  * convert_art_ns_to_tsc() - Convert ART in nanoseconds to TSC.
  * @art_ns: ART (Always Running Timer) in unit of nanoseconds
-- 
2.21.0

[Intel PMC TGPIO Driver 5/5] drivers/ptp: Add PMC Time-Aware GPIO Driver

[christopher.s.hall]

From: Christopher Hall <christopher.s.hall@intel.com>

Add support for PMC Time-Aware GPIO (TGPIO) hardware that is present on
upcoming Intel platforms. The hardware logic is driven by the ART clock.
The current hardware has two GPIO pins. Input interrupts are not
implemented in hardware.

The driver implements to the expanded PHC interface. Input requires use of
the user-polling interface. Also, since the ART clock can't be adjusted,
modulating the output frequency uses the edge timestamp interface
(EVENT_COUNT_TSTAMP2) and the PEROUT2 ioctl output frequency adjustment
interface.

Acknowledgment: Portions of the driver code were authored by Felipe
Balbi <balbi@kernel.org>

Signed-off-by: Christopher Hall <christopher.s.hall@intel.com>
---
 drivers/ptp/Kconfig               |  13 +
 drivers/ptp/Makefile              |   1 +
 drivers/ptp/ptp-intel-pmc-tgpio.c | 867 ++++++++++++++++++++++++++++++
 3 files changed, 881 insertions(+)
 create mode 100644 drivers/ptp/ptp-intel-pmc-tgpio.c

diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
index b0d1b8d264fa..974964d24947 100644
--- a/drivers/ptp/Kconfig
+++ b/drivers/ptp/Kconfig
@@ -107,6 +107,19 @@ config PTP_1588_CLOCK_PCH
 	  To compile this driver as a module, choose M here: the module
 	  will be called ptp_pch.
 
+config PTP_INTEL_PMC_TGPIO
+	tristate "Intel PMC Timed GPIO"
+	depends on X86
+	depends on ACPI
+	depends on PTP_1588_CLOCK
+	help
+	  This driver adds support for Intel PMC Timed-Aware GPIO (TGPIO)
+	  Controller. The device clock used to drive TGPIO logic is the
+	  Always Running Timer (ART).
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called ptp-intel-pmc-tgpio.
+
 config PTP_1588_CLOCK_KVM
 	tristate "KVM virtual PTP clock"
 	depends on PTP_1588_CLOCK
diff --git a/drivers/ptp/Makefile b/drivers/ptp/Makefile
index 69a06f86a450..3733ae240d4f 100644
--- a/drivers/ptp/Makefile
+++ b/drivers/ptp/Makefile
@@ -7,6 +7,7 @@ ptp-y					:= ptp_clock.o ptp_chardev.o ptp_sysfs.o
 obj-$(CONFIG_PTP_1588_CLOCK)		+= ptp.o
 obj-$(CONFIG_PTP_1588_CLOCK_DTE)	+= ptp_dte.o
 obj-$(CONFIG_PTP_1588_CLOCK_IXP46X)	+= ptp_ixp46x.o
+obj-$(CONFIG_PTP_INTEL_PMC_TGPIO)	+= ptp-intel-pmc-tgpio.o
 obj-$(CONFIG_PTP_1588_CLOCK_PCH)	+= ptp_pch.o
 obj-$(CONFIG_PTP_1588_CLOCK_KVM)	+= ptp_kvm.o
 obj-$(CONFIG_PTP_1588_CLOCK_QORIQ)	+= ptp-qoriq.o
diff --git a/drivers/ptp/ptp-intel-pmc-tgpio.c b/drivers/ptp/ptp-intel-pmc-tgpio.c
new file mode 100644
index 000000000000..2a2947b0e896
--- /dev/null
+++ b/drivers/ptp/ptp-intel-pmc-tgpio.c
@@ -0,0 +1,867 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel Timed GPIO Controller Driver
+ *
+ * Copyright (C) 2018 Intel Corporation
+ * Author: Christopher Hall <christopher.s.hall@intel.com>
+ */
+
+#include <linux/acpi.h>
+#include <linux/bitops.h>
+#include <linux/gpio.h>
+#include <linux/debugfs.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/kthread.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/ptp_clock_kernel.h>
+#include <asm/tsc.h>
+#include <linux/delay.h>
+
+#define TGPIOCTL		0x00
+#define TGPIOCOMPV31_0		0x10
+#define TGPIOCOMPV63_32		0x14
+#define TGPIOPIV31_0		0x18
+#define TGPIOPIV63_32		0x1c
+#define TGPIOTCV31_0		0x20
+#define TGPIOTCV63_32		0x24
+#define TGPIOECCV31_0		0x28
+#define TGPIOECCV63_32		0x2c
+#define TGPIOEC31_0		0x30
+#define TGPIOEC63_32		0x34
+
+/* Control Register */
+#define TGPIOCTL_EN			BIT(0)
+#define TGPIOCTL_DIR			BIT(1)
+#define TGPIOCTL_EP			GENMASK(3, 2)
+#define TGPIOCTL_EP_RISING_EDGE		(0 << 2)
+#define TGPIOCTL_EP_FALLING_EDGE	(1 << 2)
+#define TGPIOCTL_EP_TOGGLE_EDGE		(2 << 2)
+#define TGPIOCTL_PM			BIT(4)
+
+#define DRIVER_NAME		"intel-pmc-tgpio"
+#define MAX_PINS_PER_PLAT	(2)
+
+typedef char *plat_acpi_resource[MAX_PINS_PER_PLAT];
+
+/* Each row represent a platform that supports PMC TGPIO */
+static const plat_acpi_resource acpi_plat_map[] = {
+	{ "INTC1021", "INTC1022" },
+	{ "INTC1023", "INTC1024" },
+};
+
+#define PLATFORM_COUNT ARRAY_SIZE(acpi_plat_map)
+
+struct intel_pmc_tgpio_t {
+	struct mutex			 lock;
+	struct ptp_clock_info		 info;
+	struct ptp_clock		*clock;
+	const plat_acpi_resource	*plat_res;
+
+	struct intel_pmc_tgpio_pin {
+		void __iomem		*base;
+		bool			 busy;
+		struct completion	 transact_comp;
+
+		ktime_t			 curr_ns;
+		u64			 curr_art;
+
+		struct dentry		*root;
+		struct debugfs_regset32	*regset;
+		struct platform_device	*pdev;
+	} pin[MAX_PINS_PER_PLAT];
+};
+
+static struct intel_pmc_tgpio_t *intel_pmc_tgpio;
+
+#define to_intel_pmc_tgpio(i) \
+	(container_of((i), struct intel_pmc_tgpio_t, info))
+
+#define ts64_to_ptp_clock_time(x) ((struct ptp_clock_time){.sec = (x).tv_sec, \
+			.nsec = (x).tv_nsec})
+
+#define ptp_clock_time_to_ts64(x) ((struct timespec64){.tv_sec = (x).sec, \
+						       .tv_nsec = (x).nsec})
+
+static inline u32 intel_pmc_tgpio_readl
+(struct intel_pmc_tgpio_t *tgpio, u32 offset, unsigned int index)
+{
+	return readl(tgpio->pin[index].base + offset);
+}
+
+static inline void intel_pmc_tgpio_writel
+(struct intel_pmc_tgpio_t *tgpio, u32 offset, unsigned int index, u32 value)
+{
+	writel(value, tgpio->pin[index].base + offset);
+}
+
+#define INTEL_PMC_TGPIO_RD_REG(offset, index)			\
+	(intel_pmc_tgpio_readl((tgpio), (offset), (index)))
+#define INTEL_PMC_TGPIO_WR_REG(offset, index, value)		\
+	(intel_pmc_tgpio_writel((tgpio), (offset), (index), (value)))
+
+static const struct debugfs_reg32 intel_pmc_tgpio_regs[] = {
+	{
+		.name = "TGPIOCTL",
+		.offset = TGPIOCTL
+	},
+	{
+		.name = "TGPIOCOMPV31_0",
+		.offset = TGPIOCOMPV31_0
+	},
+	{
+		.name = "TGPIOCOMPV63_32",
+		.offset = TGPIOCOMPV63_32
+	},
+	{
+		.name = "TGPIOPIV31_0",
+		.offset = TGPIOPIV31_0
+	},
+	{
+		.name = "TGPIOPIV63_32",
+		.offset = TGPIOPIV63_32
+	},
+	{
+		.name = "TGPIOECCV31_0",
+		.offset = TGPIOECCV31_0
+	},
+	{
+		.name = "TGPIOECCV63_32",
+		.offset = TGPIOECCV63_32
+	},
+	{
+		.name = "TGPIOEC31_0",
+		.offset = TGPIOEC31_0
+	},
+	{
+		.name = "TGPIOEC63_32",
+		.offset = TGPIOEC63_32
+	},
+};
+
+static int intel_pmc_tgpio_gettime64(struct ptp_clock_info *info,
+		struct timespec64 *ts)
+{
+	*ts = get_tsc_ns_now(NULL);
+
+	return 0;
+}
+
+static int intel_pmc_tgpio_settime64(struct ptp_clock_info *info,
+		const struct timespec64 *ts)
+{
+	return -EOPNOTSUPP;
+}
+
+static void intel_pmc_tgpio_pre_config(
+struct intel_pmc_tgpio_t *tgpio, unsigned int index, unsigned int ctrl,
+unsigned int ctrl_new, int *enable_toggle)
+{
+	if (ctrl_new != ctrl) {
+		INTEL_PMC_TGPIO_WR_REG(TGPIOCTL, index, ctrl);
+		INTEL_PMC_TGPIO_WR_REG(TGPIOCTL, index, ctrl_new);
+		*enable_toggle = *enable_toggle > 0 ? 2 : -1;
+	}
+}
+
+static void intel_pmc_tgpio_post_config(
+struct intel_pmc_tgpio_t *tgpio, unsigned int index, unsigned int ctrl,
+int enable_toggle)
+{
+	if (enable_toggle > 0)
+		ctrl |= TGPIOCTL_EN;
+	if (enable_toggle > 1 || enable_toggle < -1)
+		INTEL_PMC_TGPIO_WR_REG(TGPIOCTL, index, ctrl);
+}
+
+static int intel_pmc_tgpio_config_input
+(struct intel_pmc_tgpio_t *tgpio, struct ptp_extts_request *extts, int on)
+{
+	u32			ctrl, ctrl_new;
+	int			enable_toggle;
+
+	/*
+	 * enable_toggle meaning:
+	 *
+	 *	-2: Change from enabled state to disabled state
+	 *	-1: Keep disabled state
+	 *	 1: Keep enabled state
+	 *	 2: Change from disabled state to enabled state
+	 */
+
+	ctrl = INTEL_PMC_TGPIO_RD_REG(TGPIOCTL, extts->index);
+	enable_toggle = ctrl & TGPIOCTL_EN ? 1 : -1;
+	ctrl &= ~TGPIOCTL_EN;
+	ctrl_new = ctrl;
+
+	if (on) {
+		int                      rising_cap, falling_cap;
+
+		ctrl_new &= ~TGPIOCTL_EP;
+
+		rising_cap = extts->flags & PTP_RISING_EDGE;
+		falling_cap = extts->flags & PTP_FALLING_EDGE;
+
+		/* By default capture rising and falling edges */
+		if (rising_cap && !falling_cap)
+			ctrl_new |= TGPIOCTL_EP_RISING_EDGE;
+		else if (!rising_cap && falling_cap)
+			ctrl_new |= TGPIOCTL_EP_FALLING_EDGE;
+		else
+			ctrl_new |= TGPIOCTL_EP_TOGGLE_EDGE;
+
+		ctrl_new |= TGPIOCTL_DIR;
+		enable_toggle = enable_toggle < 0 ? 2 : 1;
+	} else {
+		enable_toggle = enable_toggle > 0 ? -2 : -1;
+	}
+
+	intel_pmc_tgpio_pre_config
+		(tgpio, extts->index, ctrl, ctrl_new, &enable_toggle);
+	intel_pmc_tgpio_post_config
+		(tgpio, extts->index, ctrl_new, enable_toggle);
+
+	return 0;
+}
+
+#define MAX_ATOMIC_PERIOD		((u32)-1/*cycles*/)
+#define MIN_OUTPUT_PERIOD		(3/*cycles*/)
+#define FREQ_CHANGE_BLACKOUT_THRESH	((NSEC_PER_SEC/1000)*4/*ms*/)
+#define FREQ_CHANGE_SLEEP_MIN		(20/*us*/)
+#define FREQ_CHANGE_SLEEP_WINDOW	(FREQ_CHANGE_SLEEP_MIN*5)
+
+static int _intel_pmc_tgpio_config_output(
+struct intel_pmc_tgpio_t *tgpio, unsigned int index,
+struct timespec64 new_start_ns, struct timespec64 new_period_ns,
+unsigned int flags, int on)
+{
+	u32			ctrl, ctrl_new;
+	u64			new_period;
+	int			enable_toggle;
+	bool			enable_preempt = false;
+
+	/*
+	 * enable_toggle meaning:
+	 *
+	 *	-2: Change from enabled state to disabled state
+	 *	-1: Keep disabled state
+	 *	 1: Keep enabled state
+	 *	 2: Change from disabled state to enabled state
+	 */
+
+	ctrl = INTEL_PMC_TGPIO_RD_REG(TGPIOCTL, index);
+	enable_toggle = ctrl & TGPIOCTL_EN ? 1 : -1;
+	ctrl &= ~TGPIOCTL_EN;
+	ctrl_new = ctrl;
+
+	/* Frequency adjustment flag is only valid if we're already running */
+	if (flags & PTP_PEROUT_FREQ_ADJ &&
+	   (enable_toggle != 1 || !(ctrl & TGPIOCTL_PM)))
+		return -EINVAL;
+
+	new_period = convert_tsc_ns_to_art_duration(&new_period_ns);
+
+	/* Don't use a period less than the minimum */
+	if (!(flags & PTP_PEROUT_ONE_SHOT) && new_period < MIN_OUTPUT_PERIOD)
+		return -EINVAL;
+
+	/*
+	 * In the unlikely case of an adjustment from a small period (< 4ms)
+	 * to a large period (>>4 sec) change to a "transitory" period first
+	 */
+	if (flags & PTP_PEROUT_FREQ_ADJ &&
+	    tgpio->pin[index].curr_ns < FREQ_CHANGE_BLACKOUT_THRESH &&
+	    new_period >> 32) {
+		struct timespec64 transit_period_ns;
+		int err;
+
+		transit_period_ns =
+			convert_art_to_tsc_ns_duration(MAX_ATOMIC_PERIOD);
+		/* If transit period is too small we recurse infinitely */
+		if (timespec64_to_ktime(transit_period_ns) <
+		    FREQ_CHANGE_BLACKOUT_THRESH)
+			return -EINVAL;
+		err = _intel_pmc_tgpio_config_output
+			(tgpio, index, (struct timespec64){0, 0},
+			 transit_period_ns, flags, on);
+		if (err)
+			return err;
+	}
+
+	/* Don't make a change to/from 64 bit period across an output edge */
+	if (flags & PTP_PEROUT_FREQ_ADJ &&
+	    tgpio->pin[index].curr_ns >= FREQ_CHANGE_BLACKOUT_THRESH &&
+	    (new_period >> 32 || tgpio->pin[index].curr_art >> 32)) {
+		u64			next_edge;
+		u32			next_edge_lo1, next_edge_lo2;
+		struct timespec64	next_edge_tsc;
+		struct timespec64	tsc_now, tsc_tmp;
+		ktime_t			delta;
+
+		preempt_disable();
+		enable_preempt = true;
+
+		/* Calculate time delta until next edge */
+		tsc_tmp = get_tsc_ns_now(NULL);
+		next_edge_lo2 =
+			INTEL_PMC_TGPIO_RD_REG(TGPIOCOMPV31_0, index);
+		do {
+			tsc_now = tsc_tmp;
+			next_edge_lo1 = next_edge_lo2;
+			next_edge =
+				INTEL_PMC_TGPIO_RD_REG(TGPIOCOMPV63_32, index);
+			tsc_tmp = get_tsc_ns_now(NULL);
+			next_edge_lo2 =
+				INTEL_PMC_TGPIO_RD_REG(TGPIOCOMPV31_0, index);
+		} while (next_edge_lo1 != next_edge_lo2);
+		next_edge <<= 32;
+		next_edge |= next_edge_lo1;
+
+		next_edge_tsc = convert_art_to_tsc_ns(next_edge);
+		delta = timespec64_to_ktime
+			(timespec64_sub(next_edge_tsc, tsc_now));
+
+		/* If there's a chance our write will get stepped on, wait */
+		if (delta < FREQ_CHANGE_BLACKOUT_THRESH) {
+			unsigned long		min, max;
+
+			min = delta;
+			min /= 1000;
+			max = min + FREQ_CHANGE_SLEEP_WINDOW;
+			min = min < FREQ_CHANGE_SLEEP_MIN ?
+				FREQ_CHANGE_SLEEP_MIN : min;
+			usleep_range(min, max);
+		}
+	}
+
+	if (on || flags & PTP_PEROUT_ONE_SHOT) {
+		/* We only use toggle edge */
+		ctrl_new &= ~TGPIOCTL_EP;
+		ctrl_new |= TGPIOCTL_EP_TOGGLE_EDGE;
+		ctrl_new &= ~TGPIOCTL_DIR;
+
+		if (flags & PTP_PEROUT_ONE_SHOT)
+			ctrl_new &= ~TGPIOCTL_PM;
+		else
+			ctrl_new |= TGPIOCTL_PM;
+
+		enable_toggle = enable_toggle < 0 ?  2 :  1;
+	} else {
+		enable_toggle = enable_toggle > 0 ? -2 : -1;
+	}
+
+	intel_pmc_tgpio_pre_config
+		(tgpio, index, ctrl, ctrl_new, &enable_toggle);
+
+	if (enable_toggle >= 0 && !(flags & PTP_PEROUT_FREQ_ADJ)) {
+		u64 new_start = convert_tsc_ns_to_art(&new_start_ns);
+
+		INTEL_PMC_TGPIO_WR_REG
+			(TGPIOCOMPV63_32, index, new_start >> 32);
+		INTEL_PMC_TGPIO_WR_REG
+			(TGPIOCOMPV31_0, index, new_start & 0xFFFFFFFF);
+	}
+
+	if (enable_toggle >= 0 && !(flags & PTP_PEROUT_ONE_SHOT)) {
+		INTEL_PMC_TGPIO_WR_REG
+			(TGPIOPIV31_0, index, new_period & 0xFFFFFFFF);
+		INTEL_PMC_TGPIO_WR_REG
+			(TGPIOPIV63_32, index, new_period >> 32);
+		if (enable_preempt)
+			preempt_enable();
+		tgpio->pin[index].curr_ns = timespec64_to_ktime(new_period_ns);
+		tgpio->pin[index].curr_art = new_period;
+	}
+
+	intel_pmc_tgpio_post_config(tgpio, index, ctrl_new, enable_toggle);
+
+	return 0;
+}
+
+static int intel_pmc_tgpio_config_output
+(struct intel_pmc_tgpio_t *tgpio, struct ptp_perout_request *perout, int on)
+{
+	struct timespec64	new_start_ns;
+	struct timespec64	new_period_ns;
+
+	if (on || perout->flags & PTP_PEROUT_ONE_SHOT) {
+		new_start_ns = ptp_clock_time_to_ts64(perout->start);
+		new_period_ns = ptp_clock_time_to_ts64(perout->period);
+		new_period_ns = ktime_to_timespec64
+		  (ktime_divns(timespec64_to_ktime(new_period_ns), 2));
+	}
+
+	return _intel_pmc_tgpio_config_output
+		(tgpio, perout->index, new_start_ns, new_period_ns,
+		 perout->flags, on);
+}
+
+static int intel_pmc_tgpio_enable
+(struct ptp_clock_info *info, struct ptp_clock_request *req, int on)
+{
+	struct intel_pmc_tgpio_t	*tgpio = to_intel_pmc_tgpio(info);
+	int				 ret = -EOPNOTSUPP;
+	unsigned int			 index;
+
+	switch (req->type) {
+	case PTP_CLK_REQ_EXTTS:
+		index = req->extts.index;
+		break;
+	case PTP_CLK_REQ_PEROUT:
+		index = req->perout.index;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	mutex_lock(&intel_pmc_tgpio->lock);
+	while (intel_pmc_tgpio->pin[index].busy) {
+		mutex_unlock(&intel_pmc_tgpio->lock);
+		wait_for_completion
+			(&intel_pmc_tgpio->pin[index].transact_comp);
+		mutex_lock(&intel_pmc_tgpio->lock);
+	}
+	intel_pmc_tgpio->pin[index].busy = true;
+	mutex_unlock(&intel_pmc_tgpio->lock);
+
+	if (intel_pmc_tgpio->pin[index].base != NULL) {
+		switch (req->type) {
+		case PTP_CLK_REQ_EXTTS:
+			ret = intel_pmc_tgpio_config_input
+				(tgpio, &req->extts, on);
+			break;
+		case PTP_CLK_REQ_PEROUT:
+			ret = intel_pmc_tgpio_config_output
+				(tgpio, &req->perout, on);
+			break;
+		default:
+			ret = -EINVAL;
+			break;
+		}
+	} else {
+		ret = -ENODEV;
+	}
+
+	mutex_lock(&intel_pmc_tgpio->lock);
+	intel_pmc_tgpio->pin[index].busy = false;
+	complete(&intel_pmc_tgpio->pin[index].transact_comp);
+	mutex_unlock(&intel_pmc_tgpio->lock);
+
+	return ret;
+}
+
+
+static int intel_pmc_tgpio_get_time_fn(ktime_t *device_time,
+		struct system_counterval_t *system_counter, void *_tgpio)
+{
+	struct timespec64 now_ns;
+
+	now_ns = get_tsc_ns_now(system_counter);
+	*device_time = timespec64_to_ktime(now_ns);
+	return 0;
+}
+
+static int intel_pmc_tgpio_getcrosststamp
+(struct ptp_clock_info *info, struct system_device_crosststamp *cts)
+{
+	struct intel_pmc_tgpio_t	*tgpio = to_intel_pmc_tgpio(info);
+
+	return get_device_system_crosststamp
+		(intel_pmc_tgpio_get_time_fn, tgpio, NULL, cts);
+}
+
+static int intel_pmc_tgpio_counttstamp
+(struct ptp_clock_info *info, struct ptp_event_count_tstamp *count)
+{
+	struct intel_pmc_tgpio_t	*tgpio = to_intel_pmc_tgpio(info);
+	u32                              dt_hi_s;
+	u32                              dt_hi_e;
+	u32				 dt_lo;
+	struct timespec64		 dt_ts;
+	struct timespec64		 tsc_now;
+
+	mutex_lock(&intel_pmc_tgpio->lock);
+	while (intel_pmc_tgpio->pin[count->index].busy) {
+		mutex_unlock(&intel_pmc_tgpio->lock);
+		wait_for_completion
+			(&intel_pmc_tgpio->pin[count->index].transact_comp);
+		mutex_lock(&intel_pmc_tgpio->lock);
+	}
+	intel_pmc_tgpio->pin[count->index].busy = true;
+	mutex_unlock(&intel_pmc_tgpio->lock);
+
+	tsc_now = get_tsc_ns_now(NULL);
+	dt_hi_s = convert_tsc_ns_to_art(&tsc_now) >> 32;
+
+	/* Reading lower 32-bit word of Time Capture Value (TCV) loads */
+	/* the event time and event count capture */
+	dt_lo = INTEL_PMC_TGPIO_RD_REG(TGPIOTCV31_0, count->index);
+	count->event_count =
+		INTEL_PMC_TGPIO_RD_REG(TGPIOECCV63_32, count->index);
+	count->event_count <<= 32;
+	count->event_count |=
+		INTEL_PMC_TGPIO_RD_REG(TGPIOECCV31_0, count->index);
+	dt_hi_e = INTEL_PMC_TGPIO_RD_REG(TGPIOTCV63_32, count->index);
+
+	if (dt_hi_e != dt_hi_s && dt_lo >> 31)
+		dt_ts = convert_art_to_tsc_ns(((u64)dt_hi_s << 32) | dt_lo);
+	else
+		dt_ts = convert_art_to_tsc_ns(((u64)dt_hi_e << 32) | dt_lo);
+
+	count->device_time = ts64_to_ptp_clock_time(dt_ts);
+
+	mutex_lock(&intel_pmc_tgpio->lock);
+	intel_pmc_tgpio->pin[count->index].busy = false;
+	complete(&intel_pmc_tgpio->pin[count->index].transact_comp);
+	mutex_unlock(&intel_pmc_tgpio->lock);
+
+	return 0;
+}
+
+static int intel_pmc_tgpio_verify(
+struct ptp_clock_info *ptp, unsigned int pin, enum ptp_pin_function func,
+unsigned int chan)
+{
+	struct intel_pmc_tgpio_t	*tgpio  = to_intel_pmc_tgpio(ptp);
+	int				 ret    = 0;
+
+	if (func == PTP_PF_PHYSYNC)
+		return -1;
+
+	/* pin/channel is fixed for TGPIO hardware */
+	if (ptp->pin_config[pin].chan != chan)
+		return -1;
+
+	mutex_lock(&tgpio->lock);
+	/* The pin has been removed, but the PTP interface is still here */
+	if (intel_pmc_tgpio->pin[pin].base == NULL)
+		ret = -1;
+	mutex_unlock(&tgpio->lock);
+
+	return ret;
+}
+
+static struct ptp_pin_desc intel_pmc_tgpio_pin_config[MAX_PINS_PER_PLAT];
+
+static const struct ptp_clock_info intel_pmc_tgpio_info = {
+	.owner		= THIS_MODULE,
+	.name		= "Intel PMC TGPIO",
+	.max_adj	= 0,
+	.n_pins		= 0,
+	.n_ext_ts	= 0,
+	.n_per_out	= 0,
+	.pin_config	= intel_pmc_tgpio_pin_config,
+	.gettime64	= intel_pmc_tgpio_gettime64,
+	.settime64	= intel_pmc_tgpio_settime64,
+	.enable		= intel_pmc_tgpio_enable,
+	.getcrosststamp	= intel_pmc_tgpio_getcrosststamp,
+	.counttstamp	= intel_pmc_tgpio_counttstamp,
+	.verify		= intel_pmc_tgpio_verify,
+};
+
+static int ptp_device_register(struct platform_device *pdev)
+{
+	intel_pmc_tgpio->clock = ptp_clock_register
+		(&intel_pmc_tgpio->info, NULL);
+	if (IS_ERR(intel_pmc_tgpio->clock))
+		return PTR_ERR(intel_pmc_tgpio->clock);
+
+	return 0;
+}
+
+static const plat_acpi_resource *find_plat_acpi_resource
+(struct platform_device *pdev, int *n_pins)
+{
+	unsigned int			 index;
+	unsigned int			 pin_index;
+	bool				 found		= false;
+	const plat_acpi_resource	*ret;
+
+	for (pin_index = 0; pin_index < MAX_PINS_PER_PLAT; ++pin_index) {
+		for (index = 0; index < PLATFORM_COUNT; ++index) {
+			char *res_name = acpi_plat_map[index][pin_index];
+
+			if (res_name == NULL)
+				continue;
+			if (strncmp(pdev->name, res_name, strlen(res_name))
+			    == 0) {
+				found = true;
+				break;
+			}
+		}
+		if (found)
+			break;
+	}
+	ret = acpi_plat_map+index;
+	if (pin_index == MAX_PINS_PER_PLAT)
+		return ERR_PTR(-ENODEV);
+
+	/* Count the number of pins */
+	*n_pins = 0;
+	for (pin_index = 0; pin_index < MAX_PINS_PER_PLAT; ++pin_index) {
+		if ((*ret)[pin_index] == NULL)
+			break;
+		*n_pins += 1;
+	}
+
+	return ret;
+}
+
+static int intel_pmc_tgpio_probe(struct platform_device *pdev)
+{
+	struct intel_pmc_tgpio_t	*tgpio		= intel_pmc_tgpio;
+	unsigned int			 pin_index;
+	unsigned int			 pin_count;
+	struct intel_pmc_tgpio_pin	*tgpio_pin;
+	struct ptp_pin_desc		*tgpio_pin_desc;
+	struct resource			*res;
+	char				 pinname_tmpl[]	= DRIVER_NAME"-pin//";
+	int				 ret = 0;
+
+	mutex_lock(&tgpio->lock);
+
+	/* Find and cache the list of ACPI resources for this platform */
+	if (tgpio->plat_res == NULL) {
+		tgpio->plat_res = find_plat_acpi_resource
+			(pdev, &tgpio->info.n_pins);
+		tgpio->info.n_ext_ts = tgpio->info.n_per_out
+			= tgpio->info.n_pins;
+	}
+	if (IS_ERR(tgpio->plat_res)) {
+		ret = PTR_ERR(tgpio->plat_res);
+		goto unlock;
+	}
+
+	/* Find an available pin */
+	for (pin_index = 0; pin_index < MAX_PINS_PER_PLAT; ++pin_index) {
+		if (tgpio->pin[pin_index].base == NULL)
+			break;
+	}
+	if (pin_index == MAX_PINS_PER_PLAT) {
+		ret = -ENODEV;
+		goto unlock;
+	}
+
+	tgpio_pin = tgpio->pin+pin_index;
+	tgpio_pin_desc = tgpio->info.pin_config+pin_index;
+	tgpio_pin->pdev = pdev;
+	platform_set_drvdata(pdev, tgpio);
+	pin_count = pin_index + 1;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	tgpio_pin->base = devm_ioremap_resource(&pdev->dev, res);
+	if (!tgpio_pin->base) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+
+	/* Find the pin corresponding to the ACPI ID */
+	for (pin_index = 0; pin_index < MAX_PINS_PER_PLAT; ++pin_index) {
+		const char *cmpr = (const char *)(*tgpio->plat_res)[pin_index];
+
+		if (strncmp(cmpr, pdev->name, strlen(cmpr)) == 0)
+			break;
+	}
+	if (pin_index == MAX_PINS_PER_PLAT) {
+		ret = -ENODEV;
+		goto unlock;
+	}
+
+	tgpio_pin->regset = devm_kzalloc
+		(&pdev->dev, sizeof(*tgpio_pin->regset), GFP_KERNEL);
+	if (!tgpio_pin->regset) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+
+	tgpio_pin->regset->regs = intel_pmc_tgpio_regs;
+	tgpio_pin->regset->nregs = ARRAY_SIZE(intel_pmc_tgpio_regs);
+	tgpio_pin->regset->base = tgpio_pin->base;
+
+	/* Pin names are 1 indexed (+1) because hardware works like that */
+	snprintf(strchr(pinname_tmpl, '/'), 3, "%02u", pin_index+1);
+	tgpio_pin->root = debugfs_create_dir(pinname_tmpl, NULL);
+	debugfs_create_regset32("regdump", 0444, tgpio_pin->root,
+				tgpio_pin->regset);
+	strncpy(tgpio_pin_desc->name, pinname_tmpl,
+		sizeof(tgpio_pin_desc->name));
+
+	if (tgpio->info.n_pins == pin_count)
+		ret = ptp_device_register(pdev);
+
+unlock:
+	mutex_unlock(&tgpio->lock);
+
+	return ret;
+}
+
+static int intel_pmc_tgpio_remove(struct platform_device *pdev)
+{
+	struct intel_pmc_tgpio_t	*tgpio = platform_get_drvdata(pdev);
+	unsigned int			 pin_index;
+	struct intel_pmc_tgpio_pin	*tgpio_pin;
+	int ret = 0;
+
+	mutex_lock(&tgpio->lock);
+
+	for (pin_index = 0; pin_index < MAX_PINS_PER_PLAT; ++pin_index) {
+		tgpio_pin = tgpio->pin+pin_index;
+		if (tgpio_pin->pdev == pdev || tgpio_pin->pdev == NULL)
+			break;
+	}
+	if (pin_index == MAX_PINS_PER_PLAT || tgpio_pin->pdev == NULL) {
+		ret = -ENODEV;
+		goto unlock;
+	}
+
+	while (tgpio_pin->busy) {
+		mutex_unlock(&tgpio->lock);
+		wait_for_completion(&tgpio_pin->transact_comp);
+		mutex_lock(&tgpio->lock);
+	}
+	tgpio_pin->busy = true;
+	mutex_unlock(&tgpio->lock);
+
+	tgpio_pin->base = NULL;
+	debugfs_remove_recursive(tgpio_pin->root);
+
+	mutex_lock(&tgpio->lock);
+	tgpio_pin->busy = false;
+	complete(&tgpio_pin->transact_comp);
+
+unlock:
+	mutex_unlock(&tgpio->lock);
+	return ret;
+}
+
+static const struct acpi_device_id intel_pmc_acpi_match[] = {
+	{ "INTC1021", 0 }, /* EHL */
+	{ "INTC1022", 0 }, /* EHL */
+	{ "INTC1023", 0 }, /* TGL */
+	{ "INTC1024", 0 }, /* TGL */
+	{  },
+};
+
+MODULE_ALIAS("acpi*:INTC1021:*");
+MODULE_ALIAS("acpi*:INTC1022:*");
+MODULE_ALIAS("acpi*:INTC1023:*");
+MODULE_ALIAS("acpi*:INTC1024:*");
+
+static struct platform_driver intel_pmc_tgpio_driver = {
+	.probe		= intel_pmc_tgpio_probe,
+	.remove		= intel_pmc_tgpio_remove,
+	.driver		= {
+		.name	= DRIVER_NAME,
+	},
+};
+
+static struct acpi_device_id *construct_acpi_match_table(void)
+{
+	int			 acpi_id_count	= 0;
+	int			 plat_iter;
+	int			 pin_iter;
+	struct acpi_device_id	*table_iter;
+	struct acpi_device_id	*ret;
+
+	/* Walk through all ACPI IDs constructing driver table */
+	for (plat_iter = 0; plat_iter < PLATFORM_COUNT; ++plat_iter) {
+		for (pin_iter = 0; pin_iter < MAX_PINS_PER_PLAT; ++pin_iter) {
+			if (acpi_plat_map[plat_iter][pin_iter])
+				++acpi_id_count;
+			else
+				break;
+		}
+	}
+	/* Leave NULL entry at the end */
+	ret = kzalloc(sizeof(*ret)*(acpi_id_count+1), GFP_KERNEL);
+	if (ret == NULL)
+		ret = ERR_PTR(-ENOMEM);
+
+	table_iter = ret;
+	for (plat_iter = 0; plat_iter < PLATFORM_COUNT; ++plat_iter) {
+		for (pin_iter = 0; pin_iter < MAX_PINS_PER_PLAT; ++pin_iter) {
+			if (acpi_plat_map[plat_iter][pin_iter]) {
+				strncpy(table_iter->id,
+					acpi_plat_map[plat_iter][pin_iter],
+					sizeof(table_iter->id));
+				++table_iter;
+			} else {
+				break;
+			}
+		}
+	}
+
+	return ret;
+}
+
+static int intel_pmc_tgpio_init(void)
+{
+	unsigned int	iter;
+	int		ret;
+
+	if (!boot_cpu_has(X86_FEATURE_TSC_KNOWN_FREQ))
+		return -ENXIO;
+
+	intel_pmc_tgpio_driver.driver.acpi_match_table =
+		ACPI_PTR(construct_acpi_match_table());
+	if (IS_ERR(intel_pmc_tgpio_driver.driver.acpi_match_table)) {
+		ret = PTR_ERR(intel_pmc_tgpio_driver.driver.acpi_match_table);
+		goto alloc_acpi_table;
+	}
+
+	intel_pmc_tgpio =
+		kmalloc(sizeof(struct intel_pmc_tgpio_t), GFP_KERNEL);
+	if (intel_pmc_tgpio == NULL) {
+		ret = -ENOMEM;
+		goto alloc_tgpio;
+	}
+
+	mutex_init(&intel_pmc_tgpio->lock);
+	intel_pmc_tgpio->info = intel_pmc_tgpio_info;
+	intel_pmc_tgpio->plat_res = NULL;
+	for (iter = 0; iter < MAX_PINS_PER_PLAT; ++iter) {
+		intel_pmc_tgpio->info.pin_config[iter].name[0] = '\0';
+		intel_pmc_tgpio->info.pin_config[iter].index   = iter;
+		intel_pmc_tgpio->info.pin_config[iter].chan    = iter;
+		intel_pmc_tgpio->info.pin_config[iter].func    = PTP_PF_NONE;
+		intel_pmc_tgpio->info.pin_config[iter].flags   =
+			PTP_PINDESC_INPUTPOLL | PTP_PINDESC_EVTCNTVALID;
+		intel_pmc_tgpio->pin[iter].busy = false;
+		init_completion(&intel_pmc_tgpio->pin[iter].transact_comp);
+		intel_pmc_tgpio->pin[iter].root = NULL;
+		intel_pmc_tgpio->pin[iter].base = NULL;
+	}
+
+	ret = platform_driver_register(&intel_pmc_tgpio_driver);
+	if (ret)
+		goto driver_register;
+
+	return 0;
+
+driver_register:
+	kfree(intel_pmc_tgpio);
+
+alloc_tgpio:
+	kfree(intel_pmc_tgpio_driver.driver.acpi_match_table);
+
+alloc_acpi_table:
+	return ret;
+}
+
+static void intel_pmc_tgpio_exit(void)
+{
+	ptp_clock_unregister(intel_pmc_tgpio->clock);
+	platform_driver_unregister(&intel_pmc_tgpio_driver);
+	mutex_destroy(&intel_pmc_tgpio->lock);
+	kfree(intel_pmc_tgpio);
+	kfree(intel_pmc_tgpio_driver.driver.acpi_match_table);
+}
+
+module_init(intel_pmc_tgpio_init);
+module_exit(intel_pmc_tgpio_exit);
+
+
+MODULE_AUTHOR("Christopher Hall <christopher.s.hall@intel.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Intel PMC Timed GPIO Controller Driver");
-- 
2.21.0

Re: [Intel PMC TGPIO Driver 0/5] Add support for Intel PMC Time GPIO Driver with PHC interface changes to support additional H/W Features

[Jakub Kicinski]

Some process notes here:

On Wed, 11 Dec 2019 13:48:47 -0800, christopher.s.hall@intel.com wrote:

Please fix the date on your system / patches.

> Acknowledgment: Portions of the driver code were authored by Felipe
> Balbi <balbi@kernel.org>

Strangely none of the patches carry his sign-off tho, neither have you
CCed him?

Presumably this is going via the networking tree, so please tag the
patches with [PATCH net-next] rather than the name of the driver.
Subject should sufficiently serve as an indication of what the patches
do.

The net-next networking tree is now closed and will reopen shortly
after the merge window is over:

http://vger.kernel.org/~davem/net-next.html

If you post a next version before it's open to get reviews - please
post it as [RFC net-next].

(also don't use "static inline" in C files, compiler will know)

Re: [Intel PMC TGPIO Driver 1/5] drivers/ptp: Add Enhanced handling of reserve fields

[Jacob Keller]

On 12/11/2019 1:48 PM, christopher.s.hall@intel.com wrote:
> From: Christopher Hall <christopher.s.hall@intel.com>
> 
> Add functions that parameterize checking and zeroing of reserve fields in
> ioctl arguments. Eliminates need to change this code when repurposing
> reserve fields.
> 

Nice!

> Signed-off-by: Christopher Hall <christopher.s.hall@intel.com>
> ---
>  drivers/ptp/ptp_chardev.c | 60 +++++++++++++++++++++------------------
>  1 file changed, 33 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
> index 9d72ab593f13..f9ad6df57fa5 100644
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -12,6 +12,7 @@
>  #include <linux/timekeeping.h>
>  
>  #include <linux/nospec.h>
> +#include <linux/string.h>
>  
>  #include "ptp_private.h"
>  
> @@ -106,6 +107,28 @@ int ptp_open(struct posix_clock *pc, fmode_t fmode)
>  	return 0;
>  }
>  
> +/* Returns -1 if any reserved fields are non-zero */
> +static inline int _check_rsv_field(unsigned int *field, size_t size)
> +{
> +	unsigned int *iter;
> +	int ret = 0;
> +
> +	for (iter = field; iter < field+size && ret == 0; ++iter)
> +		ret = *iter == 0 ? 0 : -1;
> +
> +	return ret;
> +}
> +#define check_rsv_field(field) _check_rsv_field(field, ARRAY_SIZE(field))
> +

This assumes that reserved fields will always be arrays. Seems like a
reasonable restriction to me.

Are the reserved fields always integers? Seems so. Ok.

> +static inline void _zero_rsv_field(unsigned int *field, size_t size)
> +{
> +	unsigned int *iter;
> +
> +	for (iter = field; iter < field+size; ++iter)
> +		*iter = 0;
> +}
> +#define zero_rsv_field(field) _zero_rsv_field(field, ARRAY_SIZE(field))
> +
>  long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  {
>  	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
> @@ -154,7 +177,7 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  			req.extts.flags |= PTP_STRICT_FLAGS;
>  			/* Make sure no reserved bit is set. */
>  			if ((req.extts.flags & ~PTP_EXTTS_VALID_FLAGS) ||
> -			    req.extts.rsv[0] || req.extts.rsv[1]) {
> +			    check_rsv_field(req.extts.rsv)) {
>  				err = -EINVAL;
>  				break;
>  			}
> @@ -166,8 +189,7 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  			}
>  		} else if (cmd == PTP_EXTTS_REQUEST) {
>  			req.extts.flags &= PTP_EXTTS_V1_VALID_FLAGS;
> -			req.extts.rsv[0] = 0;
> -			req.extts.rsv[1] = 0;
> +			zero_rsv_field(req.extts.rsv);
>  		}
>  		if (req.extts.index >= ops->n_ext_ts) {
>  			err = -EINVAL;
> @@ -188,17 +210,13 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  			break;
>  		}
>  		if (((req.perout.flags & ~PTP_PEROUT_VALID_FLAGS) ||
> -			req.perout.rsv[0] || req.perout.rsv[1] ||
> -			req.perout.rsv[2] || req.perout.rsv[3]) &&
> -			cmd == PTP_PEROUT_REQUEST2) {
> +		     check_rsv_field(req.perout.rsv)) &&
> +		    cmd == PTP_PEROUT_REQUEST2) {
>  			err = -EINVAL;
>  			break;
>  		} else if (cmd == PTP_PEROUT_REQUEST) {
>  			req.perout.flags &= PTP_PEROUT_V1_VALID_FLAGS;
> -			req.perout.rsv[0] = 0;
> -			req.perout.rsv[1] = 0;
> -			req.perout.rsv[2] = 0;
> -			req.perout.rsv[3] = 0;
> +			zero_rsv_field(req.perout.rsv);
>  		}
>  		if (req.perout.index >= ops->n_per_out) {
>  			err = -EINVAL;
> @@ -258,7 +276,7 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  			break;
>  		}
>  		if (extoff->n_samples > PTP_MAX_SAMPLES
> -		    || extoff->rsv[0] || extoff->rsv[1] || extoff->rsv[2]) {
> +		    || check_rsv_field(extoff->rsv)) {
>  			err = -EINVAL;
>  			break;
>  		}
> @@ -318,17 +336,11 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  			err = -EFAULT;
>  			break;
>  		}
> -		if ((pd.rsv[0] || pd.rsv[1] || pd.rsv[2]
> -				|| pd.rsv[3] || pd.rsv[4])
> -			&& cmd == PTP_PIN_GETFUNC2) {
> +		if (check_rsv_field(pd.rsv) && cmd == PTP_PIN_GETFUNC2) {

Not that it's a big deal, but I think this might read more clearly if
this was "cmd == PTP_PIN_GETFUNC2 && check_rsv_field(pd.rsv)"

Thanks,
Jake

Re: [Intel PMC TGPIO Driver 1/5] drivers/ptp: Add Enhanced handling of reserve fields

[Jacob Keller]

On 12/11/2019 1:48 PM, christopher.s.hall@intel.com wrote:
> From: Christopher Hall <christopher.s.hall@intel.com>
> 
> Add functions that parameterize checking and zeroing of reserve fields in
> ioctl arguments. Eliminates need to change this code when repurposing
> reserve fields.
> 

One thing to note that may be problematic with this handling: a later
usage of a reserved field must still be reported as zero for callers of
the old ioctl variants. Reserved fields were only handled properly in
the "2" variants of the ioctls.

This change might make it harder to notice and remember to ensure that
previously unused fields get zero'd for the old ioctls.

Richard, thoughts?

Thanks,
Jake

Re: [Intel PMC TGPIO Driver 0/5] Add support for Intel PMC Time GPIO Driver with PHC interface changes to support additional H/W Features

[Thomas Gleixner]

christopher.s.hall@intel.com writes:
> From: Christopher Hall <christopher.s.hall@intel.com>
>
> Upcoming Intel platforms will have Time-Aware GPIO (TGPIO) hardware.
> The TGPIO logic is driven by the Always Running Timer (ART) that's
> related to TSC using CPUID[15H] (See Intel SDM Invariant
> Time-Keeping).
>
> The ART frequency is not adjustable. In order, to implement output
> adjustments an additional edge-timestamp API is added, as well, as
> a periodic output frequency adjustment API. Togther, these implement
> equivalent functionality to the existing SYS_OFFSET_* and frequency
> adjustment APIs.
>
> The TGPIO hardware doesn't implement interrupts. For TGPIO input, the
> output edge-timestamp API is re-used to implement a user-space polling
> interface. For periodic input (e.g. PPS) this is fairly efficient,
> requiring only a marginally faster poll rate than the input event
> frequency.

I really have a hard time to understand why this is implemented as part
of PTP while you talk about PPS at the same time.

Proper information about why this approach was chosen and what that
magic device is used for would be really helpful.

Thanks,

        tglx

Re: [Intel PMC TGPIO Driver 1/5] drivers/ptp: Add Enhanced handling of reserve fields

[Richard Cochran]

On Wed, Dec 11, 2019 at 01:48:48PM -0800, christopher.s.hall@intel.com wrote:
> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
> index 9d72ab593f13..f9ad6df57fa5 100644
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -12,6 +12,7 @@
>  #include <linux/timekeeping.h>
>  
>  #include <linux/nospec.h>
> +#include <linux/string.h>

Please group these two includes with the others, above, in
alphabetical order.

>  #include "ptp_private.h"
>  
> @@ -106,6 +107,28 @@ int ptp_open(struct posix_clock *pc, fmode_t fmode)
>  	return 0;
>  }
>  
> +/* Returns -1 if any reserved fields are non-zero */
> +static inline int _check_rsv_field(unsigned int *field, size_t size)

How about _check_reserved_field() instead?

> +{
> +	unsigned int *iter;

Ugh, 'ptr' please.

> +	int ret = 0;
> +
> +	for (iter = field; iter < field+size && ret == 0; ++iter)
> +		ret = *iter == 0 ? 0 : -1;

Please use the "early out" pattern:

	for (ptr = field; ptr < field + size; ptr++) {
		if (*ptr) {
			return -1;
		}
	}
	return 0;

Note:  field + size
Note:  ptr++

> +
> +	return ret;
> +}
> +#define check_rsv_field(field) _check_rsv_field(field, ARRAY_SIZE(field))

And check_reserved_field() here.  No need to abbreviate.

Thanks,
Richard

Re: [Intel PMC TGPIO Driver 1/5] drivers/ptp: Add Enhanced handling of reserve fields

[Richard Cochran]

On Fri, Jan 31, 2020 at 08:54:19AM -0800, Jacob Keller wrote:
> 
> Not that it's a big deal, but I think this might read more clearly if
> this was "cmd == PTP_PIN_GETFUNC2 && check_rsv_field(pd.rsv)"

+1

Re: [Intel PMC TGPIO Driver 2/5] drivers/ptp: Add PEROUT2 ioctl frequency adjustment interface

[Richard Cochran]

On Wed, Dec 11, 2019 at 01:48:49PM -0800, christopher.s.hall@intel.com wrote:
> diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
> index 93cc4f1d444a..8223f6f656dd 100644
> --- a/include/linux/ptp_clock_kernel.h
> +++ b/include/linux/ptp_clock_kernel.h
> @@ -134,6 +134,8 @@ struct ptp_clock_info {
>  			  struct ptp_system_timestamp *sts);
>  	int (*getcrosststamp)(struct ptp_clock_info *ptp,
>  			      struct system_device_crosststamp *cts);
> +	int (*counttstamp)(struct ptp_clock_info *ptp,
> +			   struct ptp_event_count_tstamp *count);

KernelDoc missing.

As tglx said, it is hard to guess what this will be used for.  I would
appreciate a fuller explanation of the new callback in the commit log
message.

In general, please introduce a specific new API with an example of how
it is used.  In this series you have three new APIs,

   [Intel PMC TGPIO Driver 2/5] drivers/ptp: Add PEROUT2 ioctl frequency adjustment interface
   [Intel PMC TGPIO Driver 3/5] drivers/ptp: Add user-space input polling interface
   [Intel PMC TGPIO Driver 4/5] x86/tsc: Add TSC support functions to support ART driven Time-Aware GPIO

and then a largish driver using them all.

   [Intel PMC TGPIO Driver 5/5] drivers/ptp: Add PMC Time-Aware GPIO Driver

May I suggest an ordering more like:

[1/5] x86/tsc: Add TSC support functions to support ART...	(with forward explanation of the use case)
[2/5] drivers/ptp: Add PMC Time-Aware GPIO Driver		(without new bits)
[3/5] drivers/ptp: Add Enhanced handling of reserve fields	(okay as is)
[4/5] drivers/ptp: Add PEROUT2 ioctl frequency adjustment interface
[5/5] implement ^^^ in the driver
[6/5] drivers/ptp: Add user-space input polling interface
[7/5] implement ^^^ in the driver

> +/*
> + * Bits of the ptp_pin_desc.flags field:
> + */
> +#define PTP_PINDESC_EVTCNTVALID	(1<<0)

Is this somehow connected to ...

>  #define PTP_PEROUT_ONE_SHOT (1<<0)
> +#define PTP_PEROUT_FREQ_ADJ (1<<1)

... this?  If not, then they each deserve their own patch.

> @@ -164,10 +179,14 @@ struct ptp_pin_desc {
>  	 * PTP_EXTTS_REQUEST and PTP_PEROUT_REQUEST ioctls.
>  	 */
>  	unsigned int chan;
> +	/*
> +	 * Per pin capability flag
> +	 */
> +	unsigned int flags;

Please use 'capabilities' instead of 'flags'.

> +#define PTP_EVENT_COUNT_TSTAMP2 \
> +	_IOWR(PTP_CLK_MAGIC, 19, struct ptp_event_count_tstamp)

What is the connection between this, PTP_PINDESC_EVTCNTVALID, and
PTP_PEROUT_FREQ?

Thanks,
Richard

Re: [Intel PMC TGPIO Driver 3/5] drivers/ptp: Add user-space input polling interface

[Richard Cochran]

On Wed, Dec 11, 2019 at 01:48:50PM -0800, christopher.s.hall@intel.com wrote:
> From: Christopher Hall <christopher.s.hall@intel.com>
> 
> The Intel PMC Time-Aware GPIO controller doesn't implement interrupts to
> notify software that an input event has occurred. To solve this problem,
> implement a user-space polling interface allowing the application to check
> for input events. The API returns an event count and time. This interface
> (EVENT_COUNT_TSTAMP2) is *reused* from the output frequency adjustment API.
> The event count delta indicates that one or more events have occurred and
> how many events may have been missed.

So I think this interface is truly horrible.  

The ptp_pin_desc describes a pin's configuration WRT the PTP_PF_xxx
and the specific EXTTS/PEROUT_REQUEST channel.  I don't know exactly
what you are trying to accomplish, but there has got to be a better
way.  Re-using the ptp_pin_desc for a polling interface is surely not
the way forward.

Thanks,
Richard

Re: [Intel PMC TGPIO Driver 5/5] drivers/ptp: Add PMC Time-Aware GPIO Driver

[Richard Cochran]

On Wed, Dec 11, 2019 at 01:48:52PM -0800, christopher.s.hall@intel.com wrote:
> +static inline u32 intel_pmc_tgpio_readl
> +(struct intel_pmc_tgpio_t *tgpio, u32 offset, unsigned int index)

Coding style is off the deep end, here and in many following
functions.

Thanks,
Richard

Re: [Intel PMC TGPIO Driver 0/5] Add support for Intel PMC Time GPIO Driver with PHC interface changes to support additional H/W Features

[Richard Cochran]

On Wed, Dec 11, 2019 at 01:48:47PM -0800, christopher.s.hall@intel.com wrote:
> The ART frequency is not adjustable. In order, to implement output
> adjustments an additional edge-timestamp API is added, as well, as
> a periodic output frequency adjustment API. Togther, these implement
> equivalent functionality to the existing SYS_OFFSET_* and frequency
> adjustment APIs.

I don't see a reason for a custom, new API just for this device.

The TGPIO input clock, the ART, is a free running counter, but you
want to support frequency adjustments.  Use a timecounter cyclecounter
pair.

Let the user dial a periodic output signal in the normal way.

Let the user change the frequency in the normal way, and during this
call, adjust the counter values accordingly.

Thanks,
Richard

Re: [Intel PMC TGPIO Driver 0/5] Add support for Intel PMC Time GPIO Driver with PHC interface changes to support additional H/W Features

[Jacob Keller]

On 2/2/2020 8:08 PM, Richard Cochran wrote:
> On Wed, Dec 11, 2019 at 01:48:47PM -0800, christopher.s.hall@intel.com wrote:
>> The ART frequency is not adjustable. In order, to implement output
>> adjustments an additional edge-timestamp API is added, as well, as
>> a periodic output frequency adjustment API. Togther, these implement
>> equivalent functionality to the existing SYS_OFFSET_* and frequency
>> adjustment APIs.
> 
> I don't see a reason for a custom, new API just for this device.
> 
> The TGPIO input clock, the ART, is a free running counter, but you
> want to support frequency adjustments.  Use a timecounter cyclecounter
> pair.
> 
> Let the user dial a periodic output signal in the normal way.
> 
> Let the user change the frequency in the normal way, and during this
> call, adjust the counter values accordingly.
> 

To add:

in order to program the pin output correctly, you may need to reverse
the timecounter/cyclecounter calculations. I recall doing something
similar in ixgbe for programming the correct output signal to match.

It may actually be worth adding helper functions to the timecounter
system for doing those kind of reverse calculations for converting from
a time value back into cycles.

> Thanks,
> Richard
> 

Re: [Intel PMC TGPIO Driver 5/5] drivers/ptp: Add PMC Time-Aware GPIO Driver

[Linus Walleij]

Hi Christopher,

thanks for your patch!

On Fri, Jan 31, 2020 at 7:41 AM <christopher.s.hall@intel.com> wrote:

> From: Christopher Hall <christopher.s.hall@intel.com>
>
> Add support for PMC Time-Aware GPIO (TGPIO) hardware that is present on
> upcoming Intel platforms. The hardware logic is driven by the ART clock.
> The current hardware has two GPIO pins. Input interrupts are not
> implemented in hardware.
>
> The driver implements to the expanded PHC interface. Input requires use of
> the user-polling interface. Also, since the ART clock can't be adjusted,
> modulating the output frequency uses the edge timestamp interface
> (EVENT_COUNT_TSTAMP2) and the PEROUT2 ioctl output frequency adjustment
> interface.
>
> Acknowledgment: Portions of the driver code were authored by Felipe
> Balbi <balbi@kernel.org>
>
> Signed-off-by: Christopher Hall <christopher.s.hall@intel.com>

This driver becomes a big confusion for the GPIO maintainer...

> +config PTP_INTEL_PMC_TGPIO
> +       tristate "Intel PMC Timed GPIO"
> +       depends on X86
> +       depends on ACPI
> +       depends on PTP_1588_CLOCK
(...)
> +#include <linux/gpio.h>

Don't use this header in new code, use <linux/gpio/driver.h>

But it looks like you should just drop it because there is no GPIO
of that generic type going on at all?

> +/* Control Register */
> +#define TGPIOCTL_EN                    BIT(0)
> +#define TGPIOCTL_DIR                   BIT(1)
> +#define TGPIOCTL_EP                    GENMASK(3, 2)
> +#define TGPIOCTL_EP_RISING_EDGE                (0 << 2)
> +#define TGPIOCTL_EP_FALLING_EDGE       (1 << 2)
> +#define TGPIOCTL_EP_TOGGLE_EDGE                (2 << 2)
> +#define TGPIOCTL_PM                    BIT(4)

OK this looks like some GPIO registers...

Then there is a bunch of PTP stuff I don't understand I suppose
related to the precision time protocol.

Could you explain to a simple soul like me what is going on?
Should I bother myself with this or is this "some other GPIO,
not what you work on" or could it be that it's something I should
review?

I get the impression that this so-called "general purpose I/O"
isn't very general purpose at all, it seems to be very PTP-purpose
rather, so this confusion needs to be explained in the commit
message and possibly in the code as well.

What is it for really?

Yours,
Linus Walleij

Re: [Intel PMC TGPIO Driver 0/5] Add support for Intel PMC Time GPIO Driver with PHC interface changes to support additional H/W Features

[Linus Walleij]

On Fri, Jan 31, 2020 at 7:40 AM <christopher.s.hall@intel.com> wrote:

> The TGPIO hardware doesn't implement interrupts. For TGPIO input, the
> output edge-timestamp API is re-used to implement a user-space polling
> interface.

It you modeled it reusing the GPIO subsystem (which I don't know if
you can) you would get access to the gpiochip character device
/dev/gpiochipN and be able to read timestamped events like
the tool in tools/gpio/gpio-event-mon.c does.

That said I am still confused about what this driver does or what the
purpose is.

GPIO pins in or out? Network coming in or going out using PTP?

What is the use case?

Yours,
Linus Walleij

Re: [Intel PMC TGPIO Driver 5/5] drivers/ptp: Add PMC Time-Aware GPIO Driver

[Andrew Lunn]

On Fri, Feb 07, 2020 at 06:10:46PM +0100, Linus Walleij wrote:
> OK this looks like some GPIO registers...
> 
> Then there is a bunch of PTP stuff I don't understand I suppose
> related to the precision time protocol.

Hi Linus

I understand your confusion. The first time this was posted to netdev,
i asked it to be renamed because it has very little to do with GPIO

https://lore.kernel.org/netdev/20190719132021.GC24930@lunn.ch/

	Andrew

Re: [Intel PMC TGPIO Driver 5/5] drivers/ptp: Add PMC Time-Aware GPIO Driver

[Andy Shevchenko]

On Fri, Feb 07, 2020 at 06:28:44PM +0100, Andrew Lunn wrote:
> On Fri, Feb 07, 2020 at 06:10:46PM +0100, Linus Walleij wrote:
> > OK this looks like some GPIO registers...
> > 
> > Then there is a bunch of PTP stuff I don't understand I suppose
> > related to the precision time protocol.
> 
> Hi Linus
> 
> I understand your confusion. The first time this was posted to netdev,
> i asked it to be renamed because it has very little to do with GPIO
> 
> https://lore.kernel.org/netdev/20190719132021.GC24930@lunn.ch/

And besides that I didn't see it in internal review list, so, it needs to be
very carefully reviewed. I already saw some not good formatted and questionable
code.

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel PMC TGPIO Driver 5/5] drivers/ptp: Add PMC Time-Aware GPIO Driver

[Andy Shevchenko]

On Fri, Feb 07, 2020 at 09:49:51PM +0200, Andy Shevchenko wrote:
> On Fri, Feb 07, 2020 at 06:28:44PM +0100, Andrew Lunn wrote:
> > On Fri, Feb 07, 2020 at 06:10:46PM +0100, Linus Walleij wrote:
> > > OK this looks like some GPIO registers...
> > > 
> > > Then there is a bunch of PTP stuff I don't understand I suppose
> > > related to the precision time protocol.
> > 
> > Hi Linus
> > 
> > I understand your confusion. The first time this was posted to netdev,
> > i asked it to be renamed because it has very little to do with GPIO
> > 
> > https://lore.kernel.org/netdev/20190719132021.GC24930@lunn.ch/
> 
> And besides that I didn't see it in internal review list, so, it needs to be
> very carefully reviewed. I already saw some not good formatted and questionable
> code.

Just to have some evidences.

The entire function
  static const plat_acpi_resource *find_plat_acpi_resource (struct platform_device *pdev, int *n_pins)
brings a lot of questions.

  MODULE_ALIAS("acpi*:INTC1021:*");
What is this?!

And so on...

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel PMC TGPIO Driver 0/5] Add support for Intel PMC Time GPIO Driver with PHC interface changes to support additional H/W Features

[Christopher S. Hall]

Thanks for reviewing.

On Fri, Jan 31, 2020 at 07:14:49PM +0100, Thomas Gleixner wrote:
> christopher.s.hall@intel.com writes:
> > From: Christopher Hall <christopher.s.hall@intel.com>
> >
> > The TGPIO hardware doesn't implement interrupts. For TGPIO input, the
> > output edge-timestamp API is re-used to implement a user-space polling
> > interface. For periodic input (e.g. PPS) this is fairly efficient,
> > requiring only a marginally faster poll rate than the input event
> > frequency.
> 
> I really have a hard time to understand why this is implemented as part
> of PTP while you talk about PPS at the same time.

We primarily need support for periodic input and output uses cases.
Apologies for omitting the periodic output use case from the cover
letter. While TGPIO isn't associated with a PTP timestamp clock, the PHC
pin/clock interface fits the usage otherwise.

The PHC periodic output API is the closest fit for periodic output without
creating a new API. The PHC interface can also register as a PPS source. I
am, however, concerned in general about implementing PPS input in the
driver because the hardware doesn't implement interrupts - requiring
polling.

> Proper information about why this approach was chosen and what that
> magic device is used for would be really helpful.

The customer requested usages are 1 kHz and 1 Hz for both input and
output. Some higher level use cases are:
- using a GPS PPS signal to sync the system clock
- auditing timesync precision for financial services, especially high
	frequency trading (e.g. MiFID).

Apart from clock import/export applications, timestamping single I/O
events are potentially valuable for industrial control applications
(e.g. motor position sensing vs. time). As time sync precision
requirements for these applications are tightened, standard GPIO
timing precision will not be good enough.

> Thanks,
> 
>         tglx

Thanks,
Christopher

Re: [Intel PMC TGPIO Driver 5/5] drivers/ptp: Add PMC Time-Aware GPIO Driver

[Christopher S. Hall]

Hi Linus,

Thanks for the review.

On Fri, Feb 07, 2020 at 06:10:46PM +0100, Linus Walleij wrote:
> Hi Christopher,
> 
> thanks for your patch!
> 
> On Fri, Jan 31, 2020 at 7:41 AM <christopher.s.hall@intel.com> wrote:
> 
> > From: Christopher Hall <christopher.s.hall@intel.com>
> >

> > The driver implements to the expanded PHC interface. Input requires use of
> > the user-polling interface. Also, since the ART clock can't be adjusted,
> > modulating the output frequency uses the edge timestamp interface
> > (EVENT_COUNT_TSTAMP2) and the PEROUT2 ioctl output frequency adjustment
> > interface.
> >
> > Acknowledgment: Portions of the driver code were authored by Felipe
> > Balbi <balbi@kernel.org>
> >
> > Signed-off-by: Christopher Hall <christopher.s.hall@intel.com>

> This driver becomes a big confusion for the GPIO maintainer...

I see your concern. TGPIO is Intel's internal name for the device, but
there's no reason we can't use some other terminology in the context of
the Linux kernel. How about removing the GP? We could refer to the device
as "timed I/O". I think that is still fairly descriptive, but removes the
confusion. Does this help the problem?

> > +config PTP_INTEL_PMC_TGPIO
> > +       tristate "Intel PMC Timed GPIO"
> > +       depends on X86
> > +       depends on ACPI
> > +       depends on PTP_1588_CLOCK
> (...)
> > +#include <linux/gpio.h>
> 
> Don't use this header in new code, use <linux/gpio/driver.h>
> 
> But it looks like you should just drop it because there is no GPIO
> of that generic type going on at all?

Yes. You're correct. Removed.

> > +/* Control Register */
> > +#define TGPIOCTL_EN                    BIT(0)
> > +#define TGPIOCTL_DIR                   BIT(1)
> > +#define TGPIOCTL_EP                    GENMASK(3, 2)
> > +#define TGPIOCTL_EP_RISING_EDGE                (0 << 2)
> > +#define TGPIOCTL_EP_FALLING_EDGE       (1 << 2)
> > +#define TGPIOCTL_EP_TOGGLE_EDGE                (2 << 2)
> > +#define TGPIOCTL_PM                    BIT(4)
> 
> OK this looks like some GPIO registers...
> 
> Then there is a bunch of PTP stuff I don't understand I suppose
> related to the precision time protocol.
> 
> Could you explain to a simple soul like me what is going on?
> Should I bother myself with this or is this "some other GPIO,
> not what you work on" or could it be that it's something I should
> review?

The Timed GPIO device has some GPIO-like features, but is mostly used to
import/export a clock signal. It doesn't implement PWM or some other "GP"
features like reading/setting pin state. I think you can safely ignore
the feature.

> I get the impression that this so-called "general purpose I/O"
> isn't very general purpose at all, it seems to be very PTP-purpose

Yes. It is missing many of general purpose features.

> rather, so this confusion needs to be explained in the commit
> message and possibly in the code as well.
> 
> What is it for really?

For import/export system clock, primarily.

> Yours,
> Linus Walleij

Thanks,
Christopher

Re: [Intel PMC TGPIO Driver 1/5] drivers/ptp: Add Enhanced handling of reserve fields

[Christopher S. Hall]

Hi Richard,

Thanks for the detailed review.

On Sun, Feb 02, 2020 at 05:27:00PM -0800, Richard Cochran wrote:
> On Wed, Dec 11, 2019 at 01:48:48PM -0800, christopher.s.hall@intel.com wrote:
> > diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
> > index 9d72ab593f13..f9ad6df57fa5 100644
> > --- a/drivers/ptp/ptp_chardev.c
> > +++ b/drivers/ptp/ptp_chardev.c
> > @@ -12,6 +12,7 @@
> >  #include <linux/timekeeping.h>
> >  
> >  #include <linux/nospec.h>
> > +#include <linux/string.h>
> 
> Please group these two includes with the others, above, in
> alphabetical order.

OK. Done.

> >  #include "ptp_private.h"
> >  
> > @@ -106,6 +107,28 @@ int ptp_open(struct posix_clock *pc, fmode_t fmode)
> >  	return 0;
> >  }
> >  
> > +/* Returns -1 if any reserved fields are non-zero */
> > +static inline int _check_rsv_field(unsigned int *field, size_t size)
> 
> How about _check_reserved_field() instead?

No problem. Sounds good.

> > +{
> > +	unsigned int *iter;
> 
> Ugh, 'ptr' please.
> 
> > +	int ret = 0;
> > +
> > +	for (iter = field; iter < field+size && ret == 0; ++iter)
> > +		ret = *iter == 0 ? 0 : -1;
> 
> Please use the "early out" pattern:
> 
> 	for (ptr = field; ptr < field + size; ptr++) {
> 		if (*ptr) {
> 			return -1;
> 		}
> 	}
> 	return 0;
> 
> Note:  field + size
> Note:  ptr++

OK for both of these.

> > +
> > +	return ret;
> > +}
> > +#define check_rsv_field(field) _check_rsv_field(field, ARRAY_SIZE(field))
> 
> And check_reserved_field() here.  No need to abbreviate.

OK. Done.

> Thanks,
> Richard

Thanks,
Chris

Re: [Intel PMC TGPIO Driver 1/5] drivers/ptp: Add Enhanced handling of reserve fields

[Christopher S. Hall]

On Sun, Feb 02, 2020 at 05:45:44PM -0800, Richard Cochran wrote:
> On Fri, Jan 31, 2020 at 08:54:19AM -0800, Jacob Keller wrote:
> > 
> > Not that it's a big deal, but I think this might read more clearly if
> > this was "cmd == PTP_PIN_GETFUNC2 && check_rsv_field(pd.rsv)"
> 
> +1

Yes. Good point.

Re: [Intel PMC TGPIO Driver 0/5] Add support for Intel PMC Time GPIO Driver with PHC interface changes to support additional H/W Features

[Christopher S. Hall]

Hi Richard,

Thanks for reviewing.

On Sun, Feb 02, 2020 at 08:08:38PM -0800, Richard Cochran wrote:
> On Wed, Dec 11, 2019 at 01:48:47PM -0800, christopher.s.hall@intel.com wrote:
> > The ART frequency is not adjustable. In order, to implement output
> > adjustments an additional edge-timestamp API is added, as well, as
> > a periodic output frequency adjustment API. Togther, these implement
> > equivalent functionality to the existing SYS_OFFSET_* and frequency
> > adjustment APIs.
> 
> I don't see a reason for a custom, new API just for this device.
> 
> The TGPIO input clock, the ART, is a free running counter, but you
> want to support frequency adjustments.  Use a timecounter cyclecounter
> pair.

I'm concerned about the complexity that the timecounter adds to
the driver. Specifically, the complexity of dealing with any rate mismatches
between the timecounter and the periodic output signal. The phase
error between the output and timecounter needs to be zero.

My counter-proposal would be to use the real-time clock as the basis of the
device clock. This is fairly simple because the relation between ART and the
realtime clock is known. When output is enabled any phase error between
the realtime clock and the periodic output signal is accumulated in the
SYS_OFFSET result.

This leaves the PHC API behavior as it is currently and uses the frequency
adjust API to adjust the output rate.

> Let the user dial a periodic output signal in the normal way.
> 
> Let the user change the frequency in the normal way, and during this
> call, adjust the counter values accordingly.

Yes to both of the above.

> Thanks,
> Richard

Thanks,
Christopher

Re: [Intel PMC TGPIO Driver 2/5] drivers/ptp: Add PEROUT2 ioctl frequency adjustment interface

[Christopher S. Hall]

Hi Richard,

On Sun, Feb 02, 2020 at 06:14:29PM -0800, Richard Cochran wrote:
> On Wed, Dec 11, 2019 at 01:48:49PM -0800, christopher.s.hall@intel.com wrote:
> > diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h

> > +	int (*counttstamp)(struct ptp_clock_info *ptp,
> > +			   struct ptp_event_count_tstamp *count);
> 
> KernelDoc missing.

> As tglx said, it is hard to guess what this will be used for.  I would
> appreciate a fuller explanation of the new callback in the commit log
> message.

Yes, to both of these above. More incremental patches as you point out
below also helps here. I have replied to tglx and you in another thread.

> In general, please introduce a specific new API with an example of how
> it is used.  In this series you have three new APIs,
> 
>    [Intel PMC TGPIO Driver 2/5] drivers/ptp: Add PEROUT2 ioctl frequency adjustment interface
>    [Intel PMC TGPIO Driver 3/5] drivers/ptp: Add user-space input polling interface
>    [Intel PMC TGPIO Driver 4/5] x86/tsc: Add TSC support functions to support ART driven Time-Aware GPIO
> 
> and then a largish driver using them all.
> 
>    [Intel PMC TGPIO Driver 5/5] drivers/ptp: Add PMC Time-Aware GPIO Driver
> 
> May I suggest an ordering more like:
> 
> [1/5] x86/tsc: Add TSC support functions to support ART...	(with forward explanation of the use case)
> [2/5] drivers/ptp: Add PMC Time-Aware GPIO Driver		(without new bits)
> [3/5] drivers/ptp: Add Enhanced handling of reserve fields	(okay as is)
> [4/5] drivers/ptp: Add PEROUT2 ioctl frequency adjustment interface
> [5/5] implement ^^^ in the driver
> [6/5] drivers/ptp: Add user-space input polling interface
> [7/5] implement ^^^ in the driver

This makes sense. Thanks for the detail here.

> > @@ -164,10 +179,14 @@ struct ptp_pin_desc {
> >  	 * PTP_EXTTS_REQUEST and PTP_PEROUT_REQUEST ioctls.
> >  	 */
> >  	unsigned int chan;
> > +	/*
> > +	 * Per pin capability flag
> > +	 */
> > +	unsigned int flags;
> 
> Please use 'capabilities' instead of 'flags'.

Yes. Makes sense.

> Thanks,
> Richard

Thanks,
Christopher

Re: [Intel PMC TGPIO Driver 0/5] Add support for Intel PMC Time GPIO Driver with PHC interface changes to support additional H/W Features

[Richard Cochran]

On Tue, Feb 25, 2020 at 03:37:07PM -0800, Christopher S. Hall wrote:
> On Sun, Feb 02, 2020 at 08:08:38PM -0800, Richard Cochran wrote:
> > The TGPIO input clock, the ART, is a free running counter, but you
> > want to support frequency adjustments.  Use a timecounter cyclecounter
> > pair.
> 
> I'm concerned about the complexity that the timecounter adds to
> the driver. Specifically, the complexity of dealing with any rate mismatches
> between the timecounter and the periodic output signal. The phase
> error between the output and timecounter needs to be zero.

If I understood correctly, the device's outputs are generated from a
non-adjustable counter.  So, no matter what, you will have the problem
of changing the pulse period in concert with the user changing the
desired frequency.

> My counter-proposal would be to use the real-time clock as the basis of the
> device clock. This is fairly simple because the relation between ART and the
> realtime clock is known. When output is enabled any phase error between
> the realtime clock and the periodic output signal is accumulated in the
> SYS_OFFSET result.

I don't understand how you intend to do this...
 
> This leaves the PHC API behavior as it is currently and uses the frequency
> adjust API to adjust the output rate.
> 
> > Let the user dial a periodic output signal in the normal way.
> > 
> > Let the user change the frequency in the normal way, and during this
> > call, adjust the counter values accordingly.
> 
> Yes to both of the above.

So, why then do you need this?

+#define PTP_EVENT_COUNT_TSTAMP2 \
+       _IOWR(PTP_CLK_MAGIC, 19, struct ptp_event_count_tstamp)

If you can make the device work with the existing user space API,

	ioctl(fd, PTP_PEROUT_REQUEST2, ...);
	while (1) {
		clock_adjtimex(FD_TO_CLOCKID(fd), ...);
	}

that would be ideal.  But I will push back on anything like the
following.

	ioctl(fd, PTP_PEROUT_REQUEST2, ...);
	while (1) {
		clock_adjtimex(FD_TO_CLOCKID(fd), ...);
		ioctl(fd, PTP_EVENT_COUNT_TSTAMP, ...);
	}

But maybe I misunderstood?

Thanks,
Richard

Re: [Intel PMC TGPIO Driver 0/5] Add support for Intel PMC Time GPIO Driver with PHC interface changes to support additional H/W Features

[Thomas Gleixner]

Christopher,

"Christopher S. Hall" <christopher.s.hall@intel.com> writes:
> On Fri, Jan 31, 2020 at 07:14:49PM +0100, Thomas Gleixner wrote:
>> christopher.s.hall@intel.com writes:
>> >
>> > The TGPIO hardware doesn't implement interrupts. For TGPIO input, the
>> > output edge-timestamp API is re-used to implement a user-space polling
>> > interface. For periodic input (e.g. PPS) this is fairly efficient,
>> > requiring only a marginally faster poll rate than the input event
>> > frequency.
>> 
>> I really have a hard time to understand why this is implemented as part
>> of PTP while you talk about PPS at the same time.
>
> We primarily need support for periodic input and output uses cases.
> Apologies for omitting the periodic output use case from the cover
> letter. While TGPIO isn't associated with a PTP timestamp clock, the PHC
> pin/clock interface fits the usage otherwise.

Which usage? PTP like usage? I really have a hard time to make the
connection. PTP is as the name says a protocol to synchronize time
across a network.

What you're having is a GPIO which has some magic timestamp clock which
can be correlated back to ART/TSC, right?

So squeezing this into PTP makes as much sense as squeezing a
camera/video/audio interface which provides or consumes timestamps into
it.

> The PHC periodic output API is the closest fit for periodic output without
> creating a new API.

Well, you avoid creating a new API or extending one which makes sense by
abusing the PTP interface for something which is not even remotely
connected to PTP.

> The PHC interface can also register as a PPS source. I am, however,
> concerned in general about implementing PPS input in the driver
> because the hardware doesn't implement interrupts - requiring polling.

Really useful.

>> Proper information about why this approach was chosen and what that
>> magic device is used for would be really helpful.
>
> The customer requested usages are 1 kHz and 1 Hz for both input and
> output. Some higher level use cases are:
> - using a GPS PPS signal to sync the system clock

That makes at least some sense. See below.

> - auditing timesync precision for financial services, especially high
> 	frequency trading (e.g. MiFID).

A good reason to not support it at all. Aside of that I have no idea how
that auditing is supposed to work. Just throwing a few buzzwords around
is not giving much technical context.

> Apart from clock import/export applications, timestamping single I/O
> events are potentially valuable for industrial control applications
> (e.g. motor position sensing vs. time). As time sync precision
> requirements for these applications are tightened, standard GPIO
> timing precision will not be good enough.

Well, coming from that industry I really doubt that you can do anything
useful with it, but hey it's been 25 years since I stopped working on
motor and motion controllers :)

Anyway, the device we are talking about is a GPIO device with inputs and
outputs plus bells and whistels attached to it.

On the input side this provides a timestamp taken by the hardware when
the input level changes, i.e. hardware based time stamping instead of
software based interrupt arrival timestamping. Looks like an obvious
extension to the GPIO subsystem.

How that timestamp is processed/converted and what an application can
actually do with it is a secondary problem:

  - PPS mode:

    This can be implemented as an actual PPS driver which consumes the
    GPIO, does timer based polling and feeds the timestamp into the PPS
    subsystem. Might be not the most accurate solution, so I can see why
    you want to use the PTP interface for it, which provides the raw
    clocksource (ART/TSC) and the correlated monotonic/realtime
    timestamps. But then again this wants to be a PTP driver consuming
    the GPIO and the timestamp via timer based polling.

  - GPIO sampling
  
    That's totally disconnected from PPS/PTP and just provides a
    correlated clock monotonic timestamp to the application.

    That covers your motor example :)

  - Timesync validation:

    -Enocluehowthatshouldworkatall

And of course you can use the GPIO input just as input without bells and
whistels :)

Now you have the output side which again is a GPIO in the first
place. But then it also has a secondary function which allows to create
a periodic output with a magic correlation to the ART and some way to
actually adjust the frequency. Neither of those two functions are in
anyway relatable to PTP AFAICT.

The periodic, programmable and adjustable output is pretty much a PWM of
some form and what you want to tell it is: Output a pulse at a given
frequency. Due to the fact that the input clock of that thing is ART you
can do the magic transformation from ART frequency to frequency adjusted
clock monotonic in order to tweak the parameters so they actually end up
generating your precise output frequency.  Tell the driver which
frequency you want and it retrieves the correlation information from the
kernel and uses it to achieve a precise output frequency. Doesn't sound
like rocket science and does not required new magic ioctls.

I might be missing something, but you surely can fill the blanks then.

Thanks,

        tglx

Re: [Intel PMC TGPIO Driver 0/5] Add support for Intel PMC Time GPIO Driver with PHC interface changes to support additional H/W Features

[Christopher S. Hall]

Hi Thomas,

Thank you for your suggestions.

On Thu, Feb 27, 2020 at 12:06:01AM +0100, Thomas Gleixner wrote:
> Christopher,
> 
> "Christopher S. Hall" <christopher.s.hall@intel.com> writes:
> > On Fri, Jan 31, 2020 at 07:14:49PM +0100, Thomas Gleixner wrote:
> >> christopher.s.hall@intel.com writes:
> >> >
> >> > The TGPIO hardware doesn't implement interrupts. For TGPIO input, the
> >> > output edge-timestamp API is re-used to implement a user-space polling
> >> > interface. For periodic input (e.g. PPS) this is fairly efficient,
> >> > requiring only a marginally faster poll rate than the input event
> >> > frequency.
> >> 
> >> I really have a hard time to understand why this is implemented as part
> >> of PTP while you talk about PPS at the same time.
> >
> > We primarily need support for periodic input and output uses cases.
> > Apologies for omitting the periodic output use case from the cover
> > letter. While TGPIO isn't associated with a PTP timestamp clock, the PHC
> > pin/clock interface fits the usage otherwise.
> 
> Which usage? PTP like usage? I really have a hard time to make the
> connection. PTP is as the name says a protocol to synchronize time
> across a network.
> 
> What you're having is a GPIO which has some magic timestamp clock which
> can be correlated back to ART/TSC, right?

Right.

> > The customer requested usages are 1 kHz and 1 Hz for both input and
> > output. Some higher level use cases are:
> > - using a GPS PPS signal to sync the system clock
> 
> That makes at least some sense. See below.
> 
> > - auditing timesync precision for financial services, especially high
> > 	frequency trading (e.g. MiFID).
> 
> A good reason to not support it at all. Aside of that I have no idea how
> that auditing is supposed to work. Just throwing a few buzzwords around
> is not giving much technical context.
> 
> > Apart from clock import/export applications, timestamping single I/O
> > events are potentially valuable for industrial control applications
> > (e.g. motor position sensing vs. time). As time sync precision
> > requirements for these applications are tightened, standard GPIO
> > timing precision will not be good enough.
> 
> Well, coming from that industry I really doubt that you can do anything
> useful with it, but hey it's been 25 years since I stopped working on
> motor and motion controllers :)
> 
> Anyway, the device we are talking about is a GPIO device with inputs and
> outputs plus bells and whistels attached to it.
> 
> On the input side this provides a timestamp taken by the hardware when
> the input level changes, i.e. hardware based time stamping instead of
> software based interrupt arrival timestamping. Looks like an obvious
> extension to the GPIO subsystem.
> 
> How that timestamp is processed/converted and what an application can
> actually do with it is a secondary problem:
> 
>   - PPS mode:
> 
>     This can be implemented as an actual PPS driver which consumes the
>     GPIO, does timer based polling and feeds the timestamp into the PPS
>     subsystem. Might be not the most accurate solution, so I can see why
>     you want to use the PTP interface for it, which provides the raw
>     clocksource (ART/TSC) and the correlated monotonic/realtime
>     timestamps. But then again this wants to be a PTP driver consuming
>     the GPIO and the timestamp via timer based polling.
> 
>   - GPIO sampling
>   
>     That's totally disconnected from PPS/PTP and just provides a
>     correlated clock monotonic timestamp to the application.
> 
>     That covers your motor example :)
> 
>   - Timesync validation:
> 
>     -Enocluehowthatshouldworkatall
> 
> And of course you can use the GPIO input just as input without bells and
> whistels :)
> 
> Now you have the output side which again is a GPIO in the first
> place. But then it also has a secondary function which allows to create
> a periodic output with a magic correlation to the ART and some way to
> actually adjust the frequency. Neither of those two functions are in
> anyway relatable to PTP AFAICT.
> 
> The periodic, programmable and adjustable output is pretty much a PWM of
> some form and what you want to tell it is: Output a pulse at a given
> frequency. Due to the fact that the input clock of that thing is ART you
> can do the magic transformation from ART frequency to frequency adjusted
> clock monotonic in order to tweak the parameters so they actually end up
> generating your precise output frequency.  Tell the driver which
> frequency you want and it retrieves the correlation information from the
> kernel and uses it to achieve a precise output frequency. Doesn't sound
> like rocket science and does not required new magic ioctls.

This will have a few touch points in the kernel - PWM, GPIO, PPS. I'll
work on an RFC patchset.

> I might be missing something, but you surely can fill the blanks then.
> 
> Thanks,
> 
>         tglx

Thanks,
Christopher

Re: [Intel PMC TGPIO Driver 0/5] Add support for Intel PMC Time GPIO Driver with PHC interface changes to support additional H/W Features

[Christopher S. Hall]

Hi Richard,

On Tue, Feb 25, 2020 at 06:47:07PM -0800, Richard Cochran wrote:
> On Tue, Feb 25, 2020 at 03:37:07PM -0800, Christopher S. Hall wrote:
> > On Sun, Feb 02, 2020 at 08:08:38PM -0800, Richard Cochran wrote:
> > > The TGPIO input clock, the ART, is a free running counter, but you
> > > want to support frequency adjustments.  Use a timecounter cyclecounter
> > > pair.
> > 
> > I'm concerned about the complexity that the timecounter adds to
> > the driver. Specifically, the complexity of dealing with any rate mismatches
> > between the timecounter and the periodic output signal. The phase
> > error between the output and timecounter needs to be zero.
> 
> If I understood correctly, the device's outputs are generated from a
> non-adjustable counter.  So, no matter what, you will have the problem
> of changing the pulse period in concert with the user changing the
> desired frequency.
> 

> > This leaves the PHC API behavior as it is currently and uses the frequency
> > adjust API to adjust the output rate.
> > 
> > > Let the user dial a periodic output signal in the normal way.
> > > 
> > > Let the user change the frequency in the normal way, and during this
> > > call, adjust the counter values accordingly.
> > 
> > Yes to both of the above.
> 
> So, why then do you need this?
> 
> +#define PTP_EVENT_COUNT_TSTAMP2 \
> +       _IOWR(PTP_CLK_MAGIC, 19, struct ptp_event_count_tstamp)
> 
> If you can make the device work with the existing user space API,
> 
> 	ioctl(fd, PTP_PEROUT_REQUEST2, ...);
> 	while (1) {
> 		clock_adjtimex(FD_TO_CLOCKID(fd), ...);
> 	}
> 
> that would be ideal.  But I will push back on anything like the
> following.
> 
> 	ioctl(fd, PTP_PEROUT_REQUEST2, ...);
> 	while (1) {
> 		clock_adjtimex(FD_TO_CLOCKID(fd), ...);
> 		ioctl(fd, PTP_EVENT_COUNT_TSTAMP, ...);
> 	}
> 
> But maybe I misunderstood?

Thank you for the feedback, but Thomas wants to see this as
an extension of GPIO. I'll work on an RFC patch for that instead.

> Thanks,
> Richard

Thanks,
Christopher

Re: [Intel PMC TGPIO Driver 0/5] Add support for Intel PMC Time GPIO Driver with PHC interface changes to support additional H/W Features

[Linus Walleij]

On Thu, Feb 27, 2020 at 12:06 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> "Christopher S. Hall" <christopher.s.hall@intel.com> writes:

> > Apart from clock import/export applications, timestamping single I/O
> > events are potentially valuable for industrial control applications
> > (e.g. motor position sensing vs. time). As time sync precision
> > requirements for these applications are tightened, standard GPIO
> > timing precision will not be good enough.

If you are using (from userspace) the GPIO character device
and open the events using e.g. tools/gpio/gpio-event-mon.c
you get GPIO events to userspace.

This uses a threaded interrupt with an top half (fastpath)
that timestamps it as the IRQ comes in using
ktime_get_ns(). It's as good as we can get it with just
software and IRQs (I think).

This uses a KFIFO to userspace, same approach as the IIO
subsystem.

> Anyway, the device we are talking about is a GPIO device with inputs and
> outputs plus bells and whistles attached to it.
>
> On the input side this provides a timestamp taken by the hardware when
> the input level changes, i.e. hardware based time stamping instead of
> software based interrupt arrival timestamping. Looks like an obvious
> extension to the GPIO subsystem.

That looks like something I/we would want to support all the way
to userspace so people can do their funny industrial stuff in some
standard manner.

IIO has a config file in sysfs that lets them select the source of the
timestamp like so (drivers/iio/industrialio-core.c):

s64 iio_get_time_ns(const struct iio_dev *indio_dev)
{
        struct timespec64 tp;

        switch (iio_device_get_clock(indio_dev)) {
        case CLOCK_REALTIME:
                return ktime_get_real_ns();
        case CLOCK_MONOTONIC:
                return ktime_get_ns();
        case CLOCK_MONOTONIC_RAW:
                return ktime_get_raw_ns();
        case CLOCK_REALTIME_COARSE:
                return ktime_to_ns(ktime_get_coarse_real());
        case CLOCK_MONOTONIC_COARSE:
                ktime_get_coarse_ts64(&tp);
                return timespec64_to_ns(&tp);
        case CLOCK_BOOTTIME:
                return ktime_get_boottime_ns();
        case CLOCK_TAI:
                return ktime_get_clocktai_ns();
        default:
                BUG();
        }
}

After discussion with Arnd we concluded the only timestamp that
makes sense is ktime_get_ns(). So in GPIO we just use that, all the
userspace I can think of certainly prefers monotonic time.
(If tglx does not agree with that I stand corrected to whatever
he says, I suppose.)

Anyway in GPIO we could also make it configurable for users who
know what they are doing.

HW timestamps would be something more elaborate and
nice CLOCK_HW_SPECIFIC or so. Some of the IIO sensors also
have that, we just don't expose it as of now.

Yours,
Linus Walleij

Re: [Intel PMC TGPIO Driver 0/5] Add support for Intel PMC Time GPIO Driver with PHC interface changes to support additional H/W Features

[Richard Cochran]

On Tue, Mar 03, 2020 at 02:00:48PM +0100, Linus Walleij wrote:
> 
> That looks like something I/we would want to support all the way
> to userspace so people can do their funny industrial stuff in some
> standard manner.

...

> HW timestamps would be something more elaborate and
> nice CLOCK_HW_SPECIFIC or so. Some of the IIO sensors also
> have that, we just don't expose it as of now.

It is worth considering whether it makes sense to somehow unify gpio,
iio, and the phc pin subsystems.  In my view, a big chunk of work
would be to have something like the "clock tree" for gpios and clock
lines.  This tree would describe the HW connectivity between (at
least) MAC/PHY clocks and IOs, gpio controllers, audio/video codecs,
and so on.

Also, there is that comedi thing in staging.  If it has a chance ever
to leave staging, then I think it would also benefit from integration
into gpio/iio world.

Thanks,
Richard

Re: [Intel PMC TGPIO Driver 0/5] Add support for Intel PMC Time GPIO Driver with PHC interface changes to support additional H/W Features

[Thomas Gleixner]

Linus Walleij <linus.walleij@linaro.org> writes:
> On Thu, Feb 27, 2020 at 12:06 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>> "Christopher S. Hall" <christopher.s.hall@intel.com> writes:
> IIO has a config file in sysfs that lets them select the source of the
> timestamp like so (drivers/iio/industrialio-core.c):
>
> s64 iio_get_time_ns(const struct iio_dev *indio_dev)
> {
>         struct timespec64 tp;
>
>         switch (iio_device_get_clock(indio_dev)) {
>         case CLOCK_REALTIME:
>                 return ktime_get_real_ns();
>         case CLOCK_MONOTONIC:
>                 return ktime_get_ns();
>         case CLOCK_MONOTONIC_RAW:
>                 return ktime_get_raw_ns();
>         case CLOCK_REALTIME_COARSE:
>                 return ktime_to_ns(ktime_get_coarse_real());
>         case CLOCK_MONOTONIC_COARSE:
>                 ktime_get_coarse_ts64(&tp);
>                 return timespec64_to_ns(&tp);
>         case CLOCK_BOOTTIME:
>                 return ktime_get_boottime_ns();
>         case CLOCK_TAI:
>                 return ktime_get_clocktai_ns();
>         default:
>                 BUG();
>         }
> }

That's a nice example of overengineering :)

> After discussion with Arnd we concluded the only timestamp that
> makes sense is ktime_get_ns(). So in GPIO we just use that, all the
> userspace I can think of certainly prefers monotonic time.
> (If tglx does not agree with that I stand corrected to whatever
> he says, I suppose.)

In general, CLOCK_MONOTONIC is what makes most sense.

The only other interesting clock which makes sense from an application
POV is CLOCK_TAI which is becoming more popular in terms of network wide
time coordination and TSN.

CLOCK_REALTIME is a pain to deal with due to leap seconds, daylight
savings etc.

> Anyway in GPIO we could also make it configurable for users who
> know what they are doing.
>
> HW timestamps would be something more elaborate and
> nice CLOCK_HW_SPECIFIC or so. Some of the IIO sensors also
> have that, we just don't expose it as of now.

HW timestamps are just more accurate than the software timestamps which
we have now and from a portability and interface POV they should just be
converted converted / mapped to clock MONOTONIC or clock TAI. So your
existing interface (maybe extended to TAI in the future) is just
working, but more accurate.

Exposing the HW timestamp itself based on some random and potentially
unknown clock might still be useful for some specialized applications,
but that want's to be through a distinct interface so there is no chance
to confuse it with something generally useful.

Thanks,

        tglx

Re: [Intel PMC TGPIO Driver 0/5] Add support for Intel PMC Time GPIO Driver with PHC interface changes to support additional H/W Features

[Jonathan Cameron]

On Tue, 03 Mar 2020 16:24:03 +0100
Thomas Gleixner <tglx@linutronix.de> wrote:

> Linus Walleij <linus.walleij@linaro.org> writes:
> > On Thu, Feb 27, 2020 at 12:06 AM Thomas Gleixner <tglx@linutronix.de> wrote:  
> >> "Christopher S. Hall" <christopher.s.hall@intel.com> writes:  
> > IIO has a config file in sysfs that lets them select the source of the
> > timestamp like so (drivers/iio/industrialio-core.c):
> >
> > s64 iio_get_time_ns(const struct iio_dev *indio_dev)
> > {
> >         struct timespec64 tp;
> >
> >         switch (iio_device_get_clock(indio_dev)) {
> >         case CLOCK_REALTIME:
> >                 return ktime_get_real_ns();
> >         case CLOCK_MONOTONIC:
> >                 return ktime_get_ns();
> >         case CLOCK_MONOTONIC_RAW:
> >                 return ktime_get_raw_ns();
> >         case CLOCK_REALTIME_COARSE:
> >                 return ktime_to_ns(ktime_get_coarse_real());
> >         case CLOCK_MONOTONIC_COARSE:
> >                 ktime_get_coarse_ts64(&tp);
> >                 return timespec64_to_ns(&tp);
> >         case CLOCK_BOOTTIME:
> >                 return ktime_get_boottime_ns();
> >         case CLOCK_TAI:
> >                 return ktime_get_clocktai_ns();
> >         default:
> >                 BUG();
> >         }
> > }  
> 
> That's a nice example of overengineering :)

Yeah.  There was some ugly history behind that including some 'ancient'
stupidity from me :(  I certainly don't recommend anyone copies it.

We may have overcompensated for having an odd default by allowing
lots of other odd choices.

> 
> > After discussion with Arnd we concluded the only timestamp that
> > makes sense is ktime_get_ns(). So in GPIO we just use that, all the
> > userspace I can think of certainly prefers monotonic time.
> > (If tglx does not agree with that I stand corrected to whatever
> > he says, I suppose.)  
> 
> In general, CLOCK_MONOTONIC is what makes most sense.
> 
> The only other interesting clock which makes sense from an application
> POV is CLOCK_TAI which is becoming more popular in terms of network wide
> time coordination and TSN.
> 
> CLOCK_REALTIME is a pain to deal with due to leap seconds, daylight
> savings etc.
> 
> > Anyway in GPIO we could also make it configurable for users who
> > know what they are doing.
> >
> > HW timestamps would be something more elaborate and
> > nice CLOCK_HW_SPECIFIC or so. Some of the IIO sensors also
> > have that, we just don't expose it as of now.  
> 
> HW timestamps are just more accurate than the software timestamps which
> we have now and from a portability and interface POV they should just be
> converted converted / mapped to clock MONOTONIC or clock TAI. So your
> existing interface (maybe extended to TAI in the future) is just
> working, but more accurate.
> 
> Exposing the HW timestamp itself based on some random and potentially
> unknown clock might still be useful for some specialized applications,
> but that want's to be through a distinct interface so there is no chance
> to confuse it with something generally useful.

Agreed, though it would be nice to actually have some hardware
that supports sane synchronization between a hardware timestamp and
sensible clocks in the system.   In IIO we have some nasty filtering in
some drivers to attempt to align hardware timestamps and deal with
jitter in interrupt handling.

Good luck (or maybe you do have a rare sane system!)

Jonathan

> 
> Thanks,
> 
>         tglx