[RFC PATCH 0/5] PTP: add support for Intel's TGPIO controller

[RFC PATCH 0/5] PTP: add support for Intel's TGPIO controller

[Felipe Balbi]

TGPIO is a new IP which allows for time synchronization between systems
without any other means of synchronization such as PTP or NTP. The
driver is implemented as part of the PTP framework since its features
covered most of what this controller can do.

There are a few things that made me send this as a RFC, however:

(1) This version of the controller lacks an interrupt line. Currently I
	put a kthread that starts polling the controller whenever its
	pin is configured as input. Any better ideas for allowing
	userspace control the polling rate? Perhaps tap into ptp_poll()?

(2) ACPI IDs can't be shared at this moment, unfortunately.

(3) The change in arch/x86/kernel/tsc.c needs to be reviewed at length
	before going in.

Let me know what you guys think,
Cheers

Felipe Balbi (5):
  x86: tsc: add tsc to art helpers
  PTP: add a callback for counting timestamp events
  PTP: implement PTP_EVENT_COUNT_TSTAMP ioctl
  PTP: Add flag for non-periodic output
  PTP: Add support for Intel PMC Timed GPIO Controller

 arch/x86/include/asm/tsc.h        |   2 +
 arch/x86/kernel/tsc.c             |  32 +++
 drivers/ptp/Kconfig               |   8 +
 drivers/ptp/Makefile              |   1 +
 drivers/ptp/ptp-intel-pmc-tgpio.c | 378 ++++++++++++++++++++++++++++++
 drivers/ptp/ptp_chardev.c         |  15 ++
 include/linux/ptp_clock_kernel.h  |  12 +
 include/uapi/linux/ptp_clock.h    |   6 +-
 8 files changed, 453 insertions(+), 1 deletion(-)
 create mode 100644 drivers/ptp/ptp-intel-pmc-tgpio.c

-- 
2.22.0

[RFC PATCH 1/5] x86: tsc: add tsc to art helpers

[Felipe Balbi]

Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---
 arch/x86/include/asm/tsc.h |  2 ++
 arch/x86/kernel/tsc.c      | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/arch/x86/include/asm/tsc.h b/arch/x86/include/asm/tsc.h
index 8a0c25c6bf09..b7a9f4385a82 100644
--- a/arch/x86/include/asm/tsc.h
+++ b/arch/x86/include/asm/tsc.h
@@ -32,6 +32,8 @@ 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 void get_tsc_ns(struct system_counterval_t *tsc_counterval, u64 *tsc_ns);
+extern u64 get_art_ns_now(void);
 
 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 0b29e58f288e..333fffc1db7c 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -1215,6 +1215,38 @@ struct system_counterval_t convert_art_to_tsc(u64 art)
 }
 EXPORT_SYMBOL(convert_art_to_tsc);
 
+void get_tsc_ns(struct system_counterval_t *tsc_counterval, u64 *tsc_ns)
+{
+	u64 tmp, res, rem;
+	u64 cycles;
+
+	tsc_counterval->cycles = clocksource_tsc.read(NULL);
+	cycles = tsc_counterval->cycles;
+	tsc_counterval->cs = art_related_clocksource;
+
+	rem = do_div(cycles, tsc_khz);
+
+	res = cycles * USEC_PER_SEC;
+	tmp = rem * USEC_PER_SEC;
+
+	do_div(tmp, tsc_khz);
+	res += tmp;
+
+	*tsc_ns = res;
+}
+EXPORT_SYMBOL(get_tsc_ns);
+
+u64 get_art_ns_now(void)
+{
+	struct system_counterval_t tsc_cycles;
+	u64 tsc_ns;
+
+	get_tsc_ns(&tsc_cycles, &tsc_ns);
+
+	return tsc_ns;
+}
+EXPORT_SYMBOL(get_art_ns_now);
+
 /**
  * convert_art_ns_to_tsc() - Convert ART in nanoseconds to TSC.
  * @art_ns: ART (Always Running Timer) in unit of nanoseconds
-- 
2.22.0

[RFC PATCH 2/5] PTP: add a callback for counting timestamp events

[Felipe Balbi]

This will be used for frequency discipline adjustments.

Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---
 include/linux/ptp_clock_kernel.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
index 28eb9c792522..1a4e3f916128 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -35,6 +35,16 @@ struct ptp_system_timestamp {
 	struct timespec64 post_ts;
 };
 
+/**
+ * struct ptp_event_count_tstamp - device time vs event count for frequency discipline
+ */
+struct ptp_event_count_tstamp {
+	unsigned int index;
+
+	struct ptp_clock_time device_time;
+	u64 event_count;
+};
+
 /**
  * struct ptp_clock_info - decribes a PTP hardware clock
  *
@@ -134,6 +144,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);
-- 
2.22.0

[RFC PATCH 3/5] PTP: implement PTP_EVENT_COUNT_TSTAMP ioctl

[Felipe Balbi]

With this, we can request the underlying driver to count the number of
events that have been captured.

Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---
 drivers/ptp/ptp_chardev.c      | 15 +++++++++++++++
 include/uapi/linux/ptp_clock.h |  2 ++
 2 files changed, 17 insertions(+)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index 18ffe449efdf..a3e163a6acdc 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -114,6 +114,7 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 	struct system_device_crosststamp xtstamp;
 	struct ptp_clock_info *ops = ptp->info;
 	struct ptp_sys_offset *sysoff = NULL;
+	struct ptp_event_count_tstamp counttstamp;
 	struct ptp_system_timestamp sts;
 	struct ptp_clock_request req;
 	struct ptp_clock_caps caps;
@@ -301,6 +302,20 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 		mutex_unlock(&ptp->pincfg_mux);
 		break;
 
+	case PTP_EVENT_COUNT_TSTAMP:
+		if (!ops->counttstamp)
+			return -ENOTSUPP;
+		if (copy_from_user(&req.perout, (void __user *)arg,
+				   sizeof(counttstamp))) {
+			err = -EFAULT;
+			break;
+		}
+		err = ops->counttstamp(ops, &counttstamp);
+		if (!err && copy_to_user((void __user *)arg, &counttstamp,
+						sizeof(counttstamp)))
+			err = -EFAULT;
+		break;
+
 	default:
 		err = -ENOTTY;
 		break;
diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
index 1bc794ad957a..674db7de64f3 100644
--- a/include/uapi/linux/ptp_clock.h
+++ b/include/uapi/linux/ptp_clock.h
@@ -148,6 +148,8 @@ struct ptp_pin_desc {
 	_IOWR(PTP_CLK_MAGIC, 8, struct ptp_sys_offset_precise)
 #define PTP_SYS_OFFSET_EXTENDED \
 	_IOWR(PTP_CLK_MAGIC, 9, struct ptp_sys_offset_extended)
+#define PTP_EVENT_COUNT_TSTAMP \
+	_IOWR(PTP_CLK_MAGIC, 6, struct ptp_event_count_tstamp)
 
 struct ptp_extts_event {
 	struct ptp_clock_time t; /* Time event occured. */
-- 
2.22.0

[RFC PATCH 4/5] PTP: Add flag for non-periodic output

[Felipe Balbi]

When this new flag is set, we can use single-shot output.

Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---
 include/uapi/linux/ptp_clock.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
index 674db7de64f3..439cbdfc3d9b 100644
--- a/include/uapi/linux/ptp_clock.h
+++ b/include/uapi/linux/ptp_clock.h
@@ -67,7 +67,9 @@ struct ptp_perout_request {
 	struct ptp_clock_time start;  /* Absolute start time. */
 	struct ptp_clock_time period; /* Desired period, zero means disable. */
 	unsigned int index;           /* Which channel to configure. */
-	unsigned int flags;           /* Reserved for future use. */
+
+#define PTP_PEROUT_ONE_SHOT BIT(0)
+	unsigned int flags;           /* Bit 0 -> oneshot output. */
 	unsigned int rsv[4];          /* Reserved for future use. */
 };
 
-- 
2.22.0

[RFC PATCH 5/5] PTP: Add support for Intel PMC Timed GPIO Controller

[Felipe Balbi]

Add a driver supporting Intel Timed GPIO controller available as part
of some Intel PMCs.

Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---
 drivers/ptp/Kconfig               |   8 +
 drivers/ptp/Makefile              |   1 +
 drivers/ptp/ptp-intel-pmc-tgpio.c | 378 ++++++++++++++++++++++++++++++
 3 files changed, 387 insertions(+)
 create mode 100644 drivers/ptp/ptp-intel-pmc-tgpio.c

diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
index 9b8fee5178e8..bb0fce70a783 100644
--- a/drivers/ptp/Kconfig
+++ b/drivers/ptp/Kconfig
@@ -107,6 +107,14 @@ 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
+	imply PTP_1588_CLOCK
+	help
+	  This driver adds support for Intel PMC Timed GPIO Controller
+
 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 677d1d178a3e..ff89c90ace82 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..880ece34868a
--- /dev/null
+++ b/drivers/ptp/ptp-intel-pmc-tgpio.c
@@ -0,0 +1,378 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel Timed GPIO Controller Driver
+ *
+ * Copyright (C) 2018 Intel Corporation
+ * Author: Felipe Balbi <felipe.balbi@linux.intel.com>
+ */
+
+#include <linux/acpi.h>
+#include <linux/bitops.h>
+#include <linux/gpio.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>
+
+#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 NSECS_PER_SEC		1000000000
+#define TGPIO_MAX_ADJ_TIME	999999900
+
+struct intel_pmc_tgpio {
+	struct ptp_clock_info	info;
+	struct ptp_clock	*clock;
+
+	struct mutex		lock;
+	struct device		*dev;
+	void __iomem		*base;
+
+	struct task_struct	*event_thread;
+	bool			input;
+};
+#define to_intel_pmc_tgpio(i)	(container_of((i), struct intel_pmc_tgpio, info))
+
+static inline u64 to_intel_pmc_tgpio_time(struct ptp_clock_time *t)
+{
+	return t->sec * NSECS_PER_SEC + t->nsec;
+}
+
+static inline u64 intel_pmc_tgpio_readq(void __iomem *base, u32 offset)
+{
+	return lo_hi_readq(base + offset);
+}
+
+static inline void intel_pmc_tgpio_writeq(void __iomem *base, u32 offset, u64 v)
+{
+	return lo_hi_writeq(v, base + offset);
+}
+
+static inline u32 intel_pmc_tgpio_readl(void __iomem *base, u32 offset)
+{
+	return readl(base + offset);
+}
+
+static inline void intel_pmc_tgpio_writel(void __iomem *base, u32 offset, u32 value)
+{
+	writel(value, base + offset);
+}
+
+static struct ptp_pin_desc intel_pmc_tgpio_pin_config[] = {
+	{					\
+		.name	= "pin0",		\
+		.index	= 0,			\
+		.func	= PTP_PF_NONE,		\
+		.chan	= 0,			\
+	}
+};
+
+static int intel_pmc_tgpio_gettime64(struct ptp_clock_info *info,
+		struct timespec64 *ts)
+{
+	struct intel_pmc_tgpio	*tgpio = to_intel_pmc_tgpio(info);
+	u64 now;
+
+	mutex_lock(&tgpio->lock);
+	now = get_art_ns_now();
+	*ts = ns_to_timespec64(now);
+	mutex_unlock(&tgpio->lock);
+
+	return 0;
+}
+
+static int intel_pmc_tgpio_settime64(struct ptp_clock_info *info,
+		const struct timespec64 *ts)
+{
+	return -EOPNOTSUPP;
+}
+
+static int intel_pmc_tgpio_event_thread(void *_tgpio)
+{
+	struct intel_pmc_tgpio	*tgpio = _tgpio;
+	u64 reg;
+
+	while (!kthread_should_stop()) {
+		bool input;
+		int i;
+
+		mutex_lock(&tgpio->lock);
+		input = tgpio->input;
+		mutex_unlock(&tgpio->lock);
+
+		if (!input)
+			schedule();
+
+		reg = intel_pmc_tgpio_readq(tgpio->base, TGPIOEC31_0);
+
+		for (i = 0; i < reg; i++) {
+			struct ptp_clock_event event;
+
+			event.type = PTP_CLOCK_EXTTS;
+			event.index = 0;
+			event.timestamp = intel_pmc_tgpio_readq(tgpio->base,
+					TGPIOTCV31_0);
+
+			ptp_clock_event(tgpio->clock, &event);
+		}
+		schedule_timeout_interruptible(10);
+	}
+
+	return 0;
+}
+
+static int intel_pmc_tgpio_config_input(struct intel_pmc_tgpio *tgpio,
+		struct ptp_extts_request *extts, int on)
+{
+	u32			ctrl;
+	bool			input;
+
+	ctrl = intel_pmc_tgpio_readl(tgpio->base, TGPIOCTL);
+	ctrl &= ~TGPIOCTL_EN;
+	intel_pmc_tgpio_writel(tgpio->base, TGPIOCTL, ctrl);
+
+	if (on) {
+		ctrl |= TGPIOCTL_DIR;
+
+		if (extts->flags & PTP_RISING_EDGE &&
+				extts->flags & PTP_FALLING_EDGE)
+			ctrl |= TGPIOCTL_EP_TOGGLE_EDGE;
+		else if (extts->flags & PTP_RISING_EDGE)
+			ctrl |= TGPIOCTL_EP_RISING_EDGE;
+		else if (extts->flags & PTP_FALLING_EDGE)
+			ctrl |= TGPIOCTL_EP_FALLING_EDGE;
+
+		/* gotta program all other bits before EN bit is set */
+		intel_pmc_tgpio_writel(tgpio->base, TGPIOCTL, ctrl);
+		ctrl |= TGPIOCTL_EN;
+		input = true;
+	} else {
+		ctrl &= ~(TGPIOCTL_DIR | TGPIOCTL_EN);
+		input = false;
+	}
+
+	intel_pmc_tgpio_writel(tgpio->base, TGPIOCTL, ctrl);
+	tgpio->input = input;
+
+	if (input)
+		wake_up_process(tgpio->event_thread);
+
+	return 0;
+}
+
+static int intel_pmc_tgpio_config_output(struct intel_pmc_tgpio *tgpio,
+		struct ptp_perout_request *perout, int on)
+{
+	u32			ctrl;
+
+	ctrl = intel_pmc_tgpio_readl(tgpio->base, TGPIOCTL);
+	if (on) {
+		struct ptp_clock_time *period = &perout->period;
+		struct ptp_clock_time *start = &perout->start;
+
+		if (ctrl & TGPIOCTL_EN)
+			return 0;
+
+		intel_pmc_tgpio_writeq(tgpio->base, TGPIOCOMPV31_0,
+				to_intel_pmc_tgpio_time(start));
+
+		intel_pmc_tgpio_writeq(tgpio->base, TGPIOPIV31_0,
+				to_intel_pmc_tgpio_time(period));
+
+		ctrl &= ~TGPIOCTL_DIR;
+		if (perout->flags & PTP_PEROUT_ONE_SHOT)
+			ctrl &= ~TGPIOCTL_PM;
+		else
+			ctrl |= TGPIOCTL_PM;
+
+		/* gotta program all other bits before EN bit is set */
+		intel_pmc_tgpio_writel(tgpio->base, TGPIOCTL, ctrl);
+
+		ctrl |= TGPIOCTL_EN;
+		intel_pmc_tgpio_writel(tgpio->base, TGPIOCTL, ctrl);
+	} else {
+		if (!(ctrl & ~TGPIOCTL_EN))
+			return 0;
+
+		ctrl &= ~(TGPIOCTL_EN | TGPIOCTL_PM);
+		intel_pmc_tgpio_writel(tgpio->base, TGPIOCTL, ctrl);
+	}
+
+	return 0;
+}
+
+static int intel_pmc_tgpio_enable(struct ptp_clock_info *info,
+		struct ptp_clock_request *req, int on)
+{
+	struct intel_pmc_tgpio	*tgpio = to_intel_pmc_tgpio(info);
+	int			ret = -EOPNOTSUPP;
+
+	mutex_lock(&tgpio->lock);
+	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:
+		break;
+	}
+	mutex_unlock(&tgpio->lock);
+
+	return ret;
+}
+
+static int intel_pmc_tgpio_get_time_fn(ktime_t *device_time,
+		struct system_counterval_t *system_counter, void *_tgpio)
+{
+	get_tsc_ns(system_counter, device_time);
+	return 0;
+}
+
+static int intel_pmc_tgpio_getcrosststamp(struct ptp_clock_info *info,
+		struct system_device_crosststamp *cts)
+{
+	struct intel_pmc_tgpio	*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	*tgpio = to_intel_pmc_tgpio(info);
+	u32 dt_hi_tmp;
+	u32 dt_hi;
+	u32 dt_lo;
+
+	dt_hi_tmp = intel_pmc_tgpio_readl(tgpio->base, TGPIOTCV63_32);
+	dt_lo = intel_pmc_tgpio_readl(tgpio->base, TGPIOTCV31_0);
+
+	count->event_count = intel_pmc_tgpio_readl(tgpio->base, TGPIOECCV63_32);
+	count->event_count <<= 32;
+	count->event_count |= intel_pmc_tgpio_readl(tgpio->base, TGPIOECCV31_0);
+
+	dt_hi = intel_pmc_tgpio_readl(tgpio->base, TGPIOTCV63_32);
+
+	if (dt_hi_tmp != dt_hi && dt_lo & 0x80000000)
+		count->device_time.sec = dt_hi_tmp;
+	else
+		count->device_time.sec = dt_hi;
+
+	count->device_time.nsec = dt_lo;
+
+	return 0;
+}
+
+static int intel_pmc_tgpio_verify(struct ptp_clock_info *ptp, unsigned int pin,
+		enum ptp_pin_function func, unsigned int chan)
+{
+	return 0;
+}
+
+static const struct ptp_clock_info intel_pmc_tgpio_info = {
+	.owner		= THIS_MODULE,
+	.name		= "Intel PMC TGPIO",
+	.max_adj	= 50000000,
+	.n_pins		= 1,
+	.n_ext_ts	= 1,
+	.n_per_out	= 1,
+	.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 intel_pmc_tgpio_probe(struct platform_device *pdev)
+{
+	struct intel_pmc_tgpio	*tgpio;
+	struct device		*dev;
+	struct resource		*res;
+
+	dev = &pdev->dev;
+	tgpio = devm_kzalloc(dev, sizeof(*tgpio), GFP_KERNEL);
+	if (!tgpio)
+		return -ENOMEM;
+
+	tgpio->dev = dev;
+	tgpio->info = intel_pmc_tgpio_info;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	tgpio->base = devm_ioremap_resource(dev, res);
+	if (!tgpio->base)
+		return -ENOMEM;
+
+	mutex_init(&tgpio->lock);
+	platform_set_drvdata(pdev, tgpio);
+
+	tgpio->event_thread = kthread_create(intel_pmc_tgpio_event_thread,
+			tgpio, dev_name(tgpio->dev));
+	if (IS_ERR(tgpio->event_thread))
+		return PTR_ERR(tgpio->event_thread);
+
+	tgpio->clock = ptp_clock_register(&tgpio->info, &pdev->dev);
+	if (IS_ERR(tgpio->clock))
+		return PTR_ERR(tgpio->clock);
+
+	wake_up_process(tgpio->event_thread);
+
+	return 0;
+}
+
+static int intel_pmc_tgpio_remove(struct platform_device *pdev)
+{
+	struct intel_pmc_tgpio	*tgpio = platform_get_drvdata(pdev);
+
+	ptp_clock_unregister(tgpio->clock);
+
+	return 0;
+}
+
+static const struct acpi_device_id intel_pmc_acpi_match[] = {
+	/* TODO */
+
+	{  },
+};
+
+/* MODULE_ALIAS("acpi*:TODO:*"); */
+
+static struct platform_driver intel_pmc_tgpio_driver = {
+	.probe		= intel_pmc_tgpio_probe,
+	.remove		= intel_pmc_tgpio_remove,
+	.driver		= {
+		.name	= "intel-pmc-tgpio",
+		.acpi_match_table = ACPI_PTR(intel_pmc_acpi_match),
+	},
+};
+
+module_platform_driver(intel_pmc_tgpio_driver);
+
+MODULE_AUTHOR("Felipe Balbi <felipe.balbi@linux.intel.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Intel PMC Timed GPIO Controller Driver");
-- 
2.22.0

Re: [RFC PATCH 1/5] x86: tsc: add tsc to art helpers

[Thomas Gleixner]

Felipe,

On Tue, 16 Jul 2019, Felipe Balbi wrote:

-ENOCHANGELOG

As you said in the cover letter:

>  (3) The change in arch/x86/kernel/tsc.c needs to be reviewed at length
>      before going in.

So some information what those interfaces are used for and why they are
needed would be really helpful.

> +void get_tsc_ns(struct system_counterval_t *tsc_counterval, u64 *tsc_ns)
> +{
> +	u64 tmp, res, rem;
> +	u64 cycles;
> +
> +	tsc_counterval->cycles = clocksource_tsc.read(NULL);
> +	cycles = tsc_counterval->cycles;
> +	tsc_counterval->cs = art_related_clocksource;
> +
> +	rem = do_div(cycles, tsc_khz);
> +
> +	res = cycles * USEC_PER_SEC;
> +	tmp = rem * USEC_PER_SEC;
> +
> +	do_div(tmp, tsc_khz);
> +	res += tmp;
> +
> +	*tsc_ns = res;
> +}
> +EXPORT_SYMBOL(get_tsc_ns);
> +
> +u64 get_art_ns_now(void)
> +{
> +	struct system_counterval_t tsc_cycles;
> +	u64 tsc_ns;
> +
> +	get_tsc_ns(&tsc_cycles, &tsc_ns);
> +
> +	return tsc_ns;
> +}
> +EXPORT_SYMBOL(get_art_ns_now);

While the changes look innocuous I'm missing the big picture why this needs
to emulate ART instead of simply using TSC directly.

Thanks,

	tglx

Re: [RFC PATCH 4/5] PTP: Add flag for non-periodic output

[Richard Cochran]

On Tue, Jul 16, 2019 at 10:20:37AM +0300, Felipe Balbi wrote:
> When this new flag is set, we can use single-shot output.
> 
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> ---
>  include/uapi/linux/ptp_clock.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
> index 674db7de64f3..439cbdfc3d9b 100644
> --- a/include/uapi/linux/ptp_clock.h
> +++ b/include/uapi/linux/ptp_clock.h
> @@ -67,7 +67,9 @@ struct ptp_perout_request {
>  	struct ptp_clock_time start;  /* Absolute start time. */
>  	struct ptp_clock_time period; /* Desired period, zero means disable. */
>  	unsigned int index;           /* Which channel to configure. */
> -	unsigned int flags;           /* Reserved for future use. */
> +
> +#define PTP_PEROUT_ONE_SHOT BIT(0)
> +	unsigned int flags;           /* Bit 0 -> oneshot output. */
>  	unsigned int rsv[4];          /* Reserved for future use. */

Unfortunately, the code never checked that .flags and .rsv are zero,
and so the de-facto ABI makes extending these fields impossible.  That
was my mistake from the beginning.

In order to actually support extensions, you will first have to
introduce a new ioctl.

Sorry,
Richard

>  };
>  
> -- 
> 2.22.0
> 

Re: [RFC PATCH 0/5] PTP: add support for Intel's TGPIO controller

[Richard Cochran]

On Tue, Jul 16, 2019 at 10:20:33AM +0300, Felipe Balbi wrote:
> TGPIO is a new IP which allows for time synchronization between systems
> without any other means of synchronization such as PTP or NTP. The
> driver is implemented as part of the PTP framework since its features
> covered most of what this controller can do.

Can you provide some background on this new HW?  Is the interface
copper wires between chips?  Or is it perhaps coax between hosts?

Thanks,
Richard

Re: [RFC PATCH 5/5] PTP: Add support for Intel PMC Timed GPIO Controller

[Shannon Nelson]

On 7/16/19 12:20 AM, Felipe Balbi wrote:
> Add a driver supporting Intel Timed GPIO controller available as part
> of some Intel PMCs.
>
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>

Hi Felipe, just a couple of quick comments:

There are several places where a line is continued on the next line, but 
should be indented to match the opening parenthesis on a function call 
or 'if' expression.

Shouldn't there be a kthread_stop() in intel_pmc_tgpio_remove(), or did 
I miss that somewhere?

Cheers,
sln


> ---
>   drivers/ptp/Kconfig               |   8 +
>   drivers/ptp/Makefile              |   1 +
>   drivers/ptp/ptp-intel-pmc-tgpio.c | 378 ++++++++++++++++++++++++++++++
>   3 files changed, 387 insertions(+)
>   create mode 100644 drivers/ptp/ptp-intel-pmc-tgpio.c
>
> diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
> index 9b8fee5178e8..bb0fce70a783 100644
> --- a/drivers/ptp/Kconfig
> +++ b/drivers/ptp/Kconfig
> @@ -107,6 +107,14 @@ 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
> +	imply PTP_1588_CLOCK
> +	help
> +	  This driver adds support for Intel PMC Timed GPIO Controller
> +
>   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 677d1d178a3e..ff89c90ace82 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..880ece34868a
> --- /dev/null
> +++ b/drivers/ptp/ptp-intel-pmc-tgpio.c
> @@ -0,0 +1,378 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Intel Timed GPIO Controller Driver
> + *
> + * Copyright (C) 2018 Intel Corporation
> + * Author: Felipe Balbi <felipe.balbi@linux.intel.com>
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/bitops.h>
> +#include <linux/gpio.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>
> +
> +#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 NSECS_PER_SEC		1000000000
> +#define TGPIO_MAX_ADJ_TIME	999999900
> +
> +struct intel_pmc_tgpio {
> +	struct ptp_clock_info	info;
> +	struct ptp_clock	*clock;
> +
> +	struct mutex		lock;
> +	struct device		*dev;
> +	void __iomem		*base;
> +
> +	struct task_struct	*event_thread;
> +	bool			input;
> +};
> +#define to_intel_pmc_tgpio(i)	(container_of((i), struct intel_pmc_tgpio, info))
> +
> +static inline u64 to_intel_pmc_tgpio_time(struct ptp_clock_time *t)
> +{
> +	return t->sec * NSECS_PER_SEC + t->nsec;
> +}
> +
> +static inline u64 intel_pmc_tgpio_readq(void __iomem *base, u32 offset)
> +{
> +	return lo_hi_readq(base + offset);
> +}
> +
> +static inline void intel_pmc_tgpio_writeq(void __iomem *base, u32 offset, u64 v)
> +{
> +	return lo_hi_writeq(v, base + offset);
> +}
> +
> +static inline u32 intel_pmc_tgpio_readl(void __iomem *base, u32 offset)
> +{
> +	return readl(base + offset);
> +}
> +
> +static inline void intel_pmc_tgpio_writel(void __iomem *base, u32 offset, u32 value)
> +{
> +	writel(value, base + offset);
> +}
> +
> +static struct ptp_pin_desc intel_pmc_tgpio_pin_config[] = {
> +	{					\
> +		.name	= "pin0",		\
> +		.index	= 0,			\
> +		.func	= PTP_PF_NONE,		\
> +		.chan	= 0,			\
> +	}
> +};
> +
> +static int intel_pmc_tgpio_gettime64(struct ptp_clock_info *info,
> +		struct timespec64 *ts)
> +{
> +	struct intel_pmc_tgpio	*tgpio = to_intel_pmc_tgpio(info);
> +	u64 now;
> +
> +	mutex_lock(&tgpio->lock);
> +	now = get_art_ns_now();
> +	*ts = ns_to_timespec64(now);
> +	mutex_unlock(&tgpio->lock);
> +
> +	return 0;
> +}
> +
> +static int intel_pmc_tgpio_settime64(struct ptp_clock_info *info,
> +		const struct timespec64 *ts)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static int intel_pmc_tgpio_event_thread(void *_tgpio)
> +{
> +	struct intel_pmc_tgpio	*tgpio = _tgpio;
> +	u64 reg;
> +
> +	while (!kthread_should_stop()) {
> +		bool input;
> +		int i;
> +
> +		mutex_lock(&tgpio->lock);
> +		input = tgpio->input;
> +		mutex_unlock(&tgpio->lock);
> +
> +		if (!input)
> +			schedule();
> +
> +		reg = intel_pmc_tgpio_readq(tgpio->base, TGPIOEC31_0);
> +
> +		for (i = 0; i < reg; i++) {
> +			struct ptp_clock_event event;
> +
> +			event.type = PTP_CLOCK_EXTTS;
> +			event.index = 0;
> +			event.timestamp = intel_pmc_tgpio_readq(tgpio->base,
> +					TGPIOTCV31_0);
> +
> +			ptp_clock_event(tgpio->clock, &event);
> +		}
> +		schedule_timeout_interruptible(10);
> +	}
> +
> +	return 0;
> +}
> +
> +static int intel_pmc_tgpio_config_input(struct intel_pmc_tgpio *tgpio,
> +		struct ptp_extts_request *extts, int on)
> +{
> +	u32			ctrl;
> +	bool			input;
> +
> +	ctrl = intel_pmc_tgpio_readl(tgpio->base, TGPIOCTL);
> +	ctrl &= ~TGPIOCTL_EN;
> +	intel_pmc_tgpio_writel(tgpio->base, TGPIOCTL, ctrl);
> +
> +	if (on) {
> +		ctrl |= TGPIOCTL_DIR;
> +
> +		if (extts->flags & PTP_RISING_EDGE &&
> +				extts->flags & PTP_FALLING_EDGE)
> +			ctrl |= TGPIOCTL_EP_TOGGLE_EDGE;
> +		else if (extts->flags & PTP_RISING_EDGE)
> +			ctrl |= TGPIOCTL_EP_RISING_EDGE;
> +		else if (extts->flags & PTP_FALLING_EDGE)
> +			ctrl |= TGPIOCTL_EP_FALLING_EDGE;
> +
> +		/* gotta program all other bits before EN bit is set */
> +		intel_pmc_tgpio_writel(tgpio->base, TGPIOCTL, ctrl);
> +		ctrl |= TGPIOCTL_EN;
> +		input = true;
> +	} else {
> +		ctrl &= ~(TGPIOCTL_DIR | TGPIOCTL_EN);
> +		input = false;
> +	}
> +
> +	intel_pmc_tgpio_writel(tgpio->base, TGPIOCTL, ctrl);
> +	tgpio->input = input;
> +
> +	if (input)
> +		wake_up_process(tgpio->event_thread);
> +
> +	return 0;
> +}
> +
> +static int intel_pmc_tgpio_config_output(struct intel_pmc_tgpio *tgpio,
> +		struct ptp_perout_request *perout, int on)
> +{
> +	u32			ctrl;
> +
> +	ctrl = intel_pmc_tgpio_readl(tgpio->base, TGPIOCTL);
> +	if (on) {
> +		struct ptp_clock_time *period = &perout->period;
> +		struct ptp_clock_time *start = &perout->start;
> +
> +		if (ctrl & TGPIOCTL_EN)
> +			return 0;
> +
> +		intel_pmc_tgpio_writeq(tgpio->base, TGPIOCOMPV31_0,
> +				to_intel_pmc_tgpio_time(start));
> +
> +		intel_pmc_tgpio_writeq(tgpio->base, TGPIOPIV31_0,
> +				to_intel_pmc_tgpio_time(period));
> +
> +		ctrl &= ~TGPIOCTL_DIR;
> +		if (perout->flags & PTP_PEROUT_ONE_SHOT)
> +			ctrl &= ~TGPIOCTL_PM;
> +		else
> +			ctrl |= TGPIOCTL_PM;
> +
> +		/* gotta program all other bits before EN bit is set */
> +		intel_pmc_tgpio_writel(tgpio->base, TGPIOCTL, ctrl);
> +
> +		ctrl |= TGPIOCTL_EN;
> +		intel_pmc_tgpio_writel(tgpio->base, TGPIOCTL, ctrl);
> +	} else {
> +		if (!(ctrl & ~TGPIOCTL_EN))
> +			return 0;
> +
> +		ctrl &= ~(TGPIOCTL_EN | TGPIOCTL_PM);
> +		intel_pmc_tgpio_writel(tgpio->base, TGPIOCTL, ctrl);
> +	}
> +
> +	return 0;
> +}
> +
> +static int intel_pmc_tgpio_enable(struct ptp_clock_info *info,
> +		struct ptp_clock_request *req, int on)
> +{
> +	struct intel_pmc_tgpio	*tgpio = to_intel_pmc_tgpio(info);
> +	int			ret = -EOPNOTSUPP;
> +
> +	mutex_lock(&tgpio->lock);
> +	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:
> +		break;
> +	}
> +	mutex_unlock(&tgpio->lock);
> +
> +	return ret;
> +}
> +
> +static int intel_pmc_tgpio_get_time_fn(ktime_t *device_time,
> +		struct system_counterval_t *system_counter, void *_tgpio)
> +{
> +	get_tsc_ns(system_counter, device_time);
> +	return 0;
> +}
> +
> +static int intel_pmc_tgpio_getcrosststamp(struct ptp_clock_info *info,
> +		struct system_device_crosststamp *cts)
> +{
> +	struct intel_pmc_tgpio	*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	*tgpio = to_intel_pmc_tgpio(info);
> +	u32 dt_hi_tmp;
> +	u32 dt_hi;
> +	u32 dt_lo;
> +
> +	dt_hi_tmp = intel_pmc_tgpio_readl(tgpio->base, TGPIOTCV63_32);
> +	dt_lo = intel_pmc_tgpio_readl(tgpio->base, TGPIOTCV31_0);
> +
> +	count->event_count = intel_pmc_tgpio_readl(tgpio->base, TGPIOECCV63_32);
> +	count->event_count <<= 32;
> +	count->event_count |= intel_pmc_tgpio_readl(tgpio->base, TGPIOECCV31_0);
> +
> +	dt_hi = intel_pmc_tgpio_readl(tgpio->base, TGPIOTCV63_32);
> +
> +	if (dt_hi_tmp != dt_hi && dt_lo & 0x80000000)
> +		count->device_time.sec = dt_hi_tmp;
> +	else
> +		count->device_time.sec = dt_hi;
> +
> +	count->device_time.nsec = dt_lo;
> +
> +	return 0;
> +}
> +
> +static int intel_pmc_tgpio_verify(struct ptp_clock_info *ptp, unsigned int pin,
> +		enum ptp_pin_function func, unsigned int chan)
> +{
> +	return 0;
> +}
> +
> +static const struct ptp_clock_info intel_pmc_tgpio_info = {
> +	.owner		= THIS_MODULE,
> +	.name		= "Intel PMC TGPIO",
> +	.max_adj	= 50000000,
> +	.n_pins		= 1,
> +	.n_ext_ts	= 1,
> +	.n_per_out	= 1,
> +	.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 intel_pmc_tgpio_probe(struct platform_device *pdev)
> +{
> +	struct intel_pmc_tgpio	*tgpio;
> +	struct device		*dev;
> +	struct resource		*res;
> +
> +	dev = &pdev->dev;
> +	tgpio = devm_kzalloc(dev, sizeof(*tgpio), GFP_KERNEL);
> +	if (!tgpio)
> +		return -ENOMEM;
> +
> +	tgpio->dev = dev;
> +	tgpio->info = intel_pmc_tgpio_info;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	tgpio->base = devm_ioremap_resource(dev, res);
> +	if (!tgpio->base)
> +		return -ENOMEM;
> +
> +	mutex_init(&tgpio->lock);
> +	platform_set_drvdata(pdev, tgpio);
> +
> +	tgpio->event_thread = kthread_create(intel_pmc_tgpio_event_thread,
> +			tgpio, dev_name(tgpio->dev));
> +	if (IS_ERR(tgpio->event_thread))
> +		return PTR_ERR(tgpio->event_thread);
> +
> +	tgpio->clock = ptp_clock_register(&tgpio->info, &pdev->dev);
> +	if (IS_ERR(tgpio->clock))
> +		return PTR_ERR(tgpio->clock);
> +
> +	wake_up_process(tgpio->event_thread);
> +
> +	return 0;
> +}
> +
> +static int intel_pmc_tgpio_remove(struct platform_device *pdev)
> +{
> +	struct intel_pmc_tgpio	*tgpio = platform_get_drvdata(pdev);
> +
> +	ptp_clock_unregister(tgpio->clock);
> +
> +	return 0;
> +}
> +
> +static const struct acpi_device_id intel_pmc_acpi_match[] = {
> +	/* TODO */
> +
> +	{  },
> +};
> +
> +/* MODULE_ALIAS("acpi*:TODO:*"); */
> +
> +static struct platform_driver intel_pmc_tgpio_driver = {
> +	.probe		= intel_pmc_tgpio_probe,
> +	.remove		= intel_pmc_tgpio_remove,
> +	.driver		= {
> +		.name	= "intel-pmc-tgpio",
> +		.acpi_match_table = ACPI_PTR(intel_pmc_acpi_match),
> +	},
> +};
> +
> +module_platform_driver(intel_pmc_tgpio_driver);
> +
> +MODULE_AUTHOR("Felipe Balbi <felipe.balbi@linux.intel.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Intel PMC Timed GPIO Controller Driver");

Re: [RFC PATCH 4/5] PTP: Add flag for non-periodic output

[Felipe Balbi]

Hi Richard,

Richard Cochran <richardcochran@gmail.com> writes:

> On Tue, Jul 16, 2019 at 10:20:37AM +0300, Felipe Balbi wrote:
>> When this new flag is set, we can use single-shot output.
>> 
>> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
>> ---
>>  include/uapi/linux/ptp_clock.h | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
>> index 674db7de64f3..439cbdfc3d9b 100644
>> --- a/include/uapi/linux/ptp_clock.h
>> +++ b/include/uapi/linux/ptp_clock.h
>> @@ -67,7 +67,9 @@ struct ptp_perout_request {
>>  	struct ptp_clock_time start;  /* Absolute start time. */
>>  	struct ptp_clock_time period; /* Desired period, zero means disable. */
>>  	unsigned int index;           /* Which channel to configure. */
>> -	unsigned int flags;           /* Reserved for future use. */
>> +
>> +#define PTP_PEROUT_ONE_SHOT BIT(0)
>> +	unsigned int flags;           /* Bit 0 -> oneshot output. */
>>  	unsigned int rsv[4];          /* Reserved for future use. */
>
> Unfortunately, the code never checked that .flags and .rsv are zero,
> and so the de-facto ABI makes extending these fields impossible.  That
> was my mistake from the beginning.
>
> In order to actually support extensions, you will first have to
> introduce a new ioctl.

No worries, I'll work on this after vacations (I'll off for 2 weeks
starting next week). I thought about adding a new IOCTL until I saw that
rsv field. Oh well :-)

-- 
balbi

Re: [RFC PATCH 5/5] PTP: Add support for Intel PMC Timed GPIO Controller

[Felipe Balbi]

Hi,

Shannon Nelson <snelson@pensando.io> writes:

> On 7/16/19 12:20 AM, Felipe Balbi wrote:
>> Add a driver supporting Intel Timed GPIO controller available as part
>> of some Intel PMCs.
>>
>> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
>
> Hi Felipe, just a couple of quick comments:
>
> There are several places where a line is continued on the next line, but 
> should be indented to match the opening parenthesis on a function call 
> or 'if' expression.
>
> Shouldn't there be a kthread_stop() in intel_pmc_tgpio_remove(), or did 
> I miss that somewhere?

Oops :-p

I could've sworn I had added it when disabling the pin. I'll review
that, sure.

-- 
balbi

Re: [RFC PATCH 0/5] PTP: add support for Intel's TGPIO controller

[Felipe Balbi]

Hi,

Richard Cochran <richardcochran@gmail.com> writes:

> On Tue, Jul 16, 2019 at 10:20:33AM +0300, Felipe Balbi wrote:
>> TGPIO is a new IP which allows for time synchronization between systems
>> without any other means of synchronization such as PTP or NTP. The
>> driver is implemented as part of the PTP framework since its features
>> covered most of what this controller can do.
>
> Can you provide some background on this new HW?  Is the interface
> copper wires between chips?  Or is it perhaps coax between hosts?

It's just a pin, like a GPIO. So it would be a PCB trace, flat flex,
copper wire... Anything, really.

I think most of the usecases will involve devices somehow on the same
PCB, so a trace or flat flex would be more common. Perhaps Chris has a
better idea in mind? :-)

-- 
balbi

Re: [RFC PATCH 4/5] PTP: Add flag for non-periodic output

[Richard Cochran]

On Wed, Jul 17, 2019 at 09:49:13AM +0300, Felipe Balbi wrote:
> No worries, I'll work on this after vacations (I'll off for 2 weeks
> starting next week). I thought about adding a new IOCTL until I saw that
> rsv field. Oh well :-)

It would be great if you could fix up the PTP ioctls as a preface to
your series.

Thanks,
Richard

Re: [RFC PATCH 0/5] PTP: add support for Intel's TGPIO controller

[Richard Cochran]

On Wed, Jul 17, 2019 at 09:52:55AM +0300, Felipe Balbi wrote:
> 
> It's just a pin, like a GPIO. So it would be a PCB trace, flat flex,
> copper wire... Anything, really.

Cool.  Are there any Intel CPUs available that have this feature?

Thanks,
Richard

Re: [RFC PATCH 0/5] PTP: add support for Intel's TGPIO controller

[Felipe Balbi]

Hi,

Richard Cochran <richardcochran@gmail.com> writes:

> On Wed, Jul 17, 2019 at 09:52:55AM +0300, Felipe Balbi wrote:
>> 
>> It's just a pin, like a GPIO. So it would be a PCB trace, flat flex,
>> copper wire... Anything, really.
>
> Cool.  Are there any Intel CPUs available that have this feature?

At least canon lake has it, but its BIOS doesn't enable it. This is
something that will be more important on future CPUs.

-- 
balbi

Re: [RFC PATCH 4/5] PTP: Add flag for non-periodic output

[Felipe Balbi]

Hi,

Richard Cochran <richardcochran@gmail.com> writes:

> On Wed, Jul 17, 2019 at 09:49:13AM +0300, Felipe Balbi wrote:
>> No worries, I'll work on this after vacations (I'll off for 2 weeks
>> starting next week). I thought about adding a new IOCTL until I saw that
>> rsv field. Oh well :-)
>
> It would be great if you could fix up the PTP ioctls as a preface to
> your series.

no problem, anything in particular in mind? Just create new versions of
all the IOCTLs so we can actually use the reserved fields in the future?

cheers

-- 
balbi

Re: [RFC PATCH 4/5] PTP: Add flag for non-periodic output

[Richard Cochran]

On Thu, Jul 18, 2019 at 11:59:10AM +0300, Felipe Balbi wrote:
> no problem, anything in particular in mind? Just create new versions of
> all the IOCTLs so we can actually use the reserved fields in the future?

Yes, please!

Thanks,
Richard

Re: [RFC PATCH 0/5] PTP: add support for Intel's TGPIO controller

[Andrew Lunn]

On Tue, Jul 16, 2019 at 10:20:33AM +0300, Felipe Balbi wrote:
> TGPIO is a new IP which allows for time synchronization between systems
> without any other means of synchronization such as PTP or NTP. The
> driver is implemented as part of the PTP framework since its features
> covered most of what this controller can do.

Hi Felipe

Given the name TGPIO, can it also be used for plain old boring GPIO?
Does there need to be some sort of mux between GPIO and TGPIO? And an
interface into the generic GPIO core?

Also, is this always embedded into a SoC? Or could it actually be in a
discrete NIC?

Thanks
	Andrew

Re: [RFC PATCH 0/5] PTP: add support for Intel's TGPIO controller

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1019 bytes --]


Hi,

Andrew Lunn <andrew@lunn.ch> writes:
> On Tue, Jul 16, 2019 at 10:20:33AM +0300, Felipe Balbi wrote:
>> TGPIO is a new IP which allows for time synchronization between systems
>> without any other means of synchronization such as PTP or NTP. The
>> driver is implemented as part of the PTP framework since its features
>> covered most of what this controller can do.
>
> Hi Felipe
>
> Given the name TGPIO, can it also be used for plain old boring GPIO?

not really, no. This is a misnomer, IMHO :-) We can only assert output
pulses at specified intervals or capture a timestamp of an external
signal.

> Does there need to be some sort of mux between GPIO and TGPIO? And an
> interface into the generic GPIO core?

no

> Also, is this always embedded into a SoC? Or could it actually be in a
> discrete NIC?

Technically, this could be done as a discrete, but it isn't. In any
case, why does that matter? From a linux-point of view, we have a device
driver either way.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [RFC PATCH 0/5] PTP: add support for Intel's TGPIO controller

[Andrew Lunn]

On Fri, Jul 19, 2019 at 10:35:14AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Andrew Lunn <andrew@lunn.ch> writes:
> > On Tue, Jul 16, 2019 at 10:20:33AM +0300, Felipe Balbi wrote:
> >> TGPIO is a new IP which allows for time synchronization between systems
> >> without any other means of synchronization such as PTP or NTP. The
> >> driver is implemented as part of the PTP framework since its features
> >> covered most of what this controller can do.
> >
> > Hi Felipe
> >
> > Given the name TGPIO, can it also be used for plain old boring GPIO?
> 
> not really, no. This is a misnomer, IMHO :-) We can only assert output
> pulses at specified intervals or capture a timestamp of an external
> signal.

Hi Felipe

So i guess Intel Marketing wants to call it a GPIO, but between
engineers can we give it a better name?

> > Also, is this always embedded into a SoC? Or could it actually be in a
> > discrete NIC?
> 
> Technically, this could be done as a discrete, but it isn't. In any
> case, why does that matter? From a linux-point of view, we have a device
> driver either way.

I've seen a lot of i210 used with ARM SoCs. How necessary is the tsc
patch? Is there an architecture independent alternative?

       Andrew

Re: [RFC PATCH 0/5] PTP: add support for Intel's TGPIO controller

[Felipe Balbi]

Hi,

Andrew Lunn <andrew@lunn.ch> writes:
>> Andrew Lunn <andrew@lunn.ch> writes:
>> > On Tue, Jul 16, 2019 at 10:20:33AM +0300, Felipe Balbi wrote:
>> >> TGPIO is a new IP which allows for time synchronization between systems
>> >> without any other means of synchronization such as PTP or NTP. The
>> >> driver is implemented as part of the PTP framework since its features
>> >> covered most of what this controller can do.
>> >
>> > Hi Felipe
>> >
>> > Given the name TGPIO, can it also be used for plain old boring GPIO?
>> 
>> not really, no. This is a misnomer, IMHO :-) We can only assert output
>> pulses at specified intervals or capture a timestamp of an external
>> signal.
>
> Hi Felipe
>
> So i guess Intel Marketing wants to call it a GPIO, but between
> engineers can we give it a better name?

If we do that we make it difficult for those reading specification and
trying to find the matching driver.

>> > Also, is this always embedded into a SoC? Or could it actually be in a
>> > discrete NIC?
>> 
>> Technically, this could be done as a discrete, but it isn't. In any
>> case, why does that matter? From a linux-point of view, we have a device
>> driver either way.
>
> I've seen a lot of i210 used with ARM SoCs. How necessary is the tsc
> patch? Is there an architecture independent alternative?

Without the TSC patch, we don't get the timestamp we need. One can argue
that $this driver could call get_tsc_ns() directly instead of providing
a wrapper for it. But that's something else entirely.

-- 
balbi

Re: [RFC PATCH 4/5] PTP: Add flag for non-periodic output

[Felipe Balbi]

Hi,

Richard Cochran <richardcochran@gmail.com> writes:

> On Thu, Jul 18, 2019 at 11:59:10AM +0300, Felipe Balbi wrote:
>> no problem, anything in particular in mind? Just create new versions of
>> all the IOCTLs so we can actually use the reserved fields in the future?
>
> Yes, please!

before I send a new series built on top of this change, I thought I'd
check with you if I'm on the right path. Below you can find my current
take at the new IOCTLs. I maintained the same exact structures so that
there's no maintenance burden. Also introduce a new IOCTL for every
single one of the previously existing ones even though not all of them
needed changes. The reason for that was just to make it easier for
libary authors to update their library by a simple sed script adding '2'
to the end of the IOCTL macro.

Let me know if you want anything to be changed or had a different idea
about any of this. Also, if you prefer that I finish the entire series
before you review, no worries either ;-)

Cheers, patch follows:

From bc2aa511d4c2e2228590fb29604c6c33b56527ad Mon Sep 17 00:00:00 2001
From: Felipe Balbi <felipe.balbi@linux.intel.com>
Date: Tue, 13 Aug 2019 10:32:35 +0300
Subject: [PATCH] PTP: introduce new versions of IOCTLs

The current version of the IOCTL have a small problem which prevents
us from extending the API by making use of reserved fields. In these
new IOCTLs, we are now making sure that flags and rsv fields are zero
which will allow us to extend the API in the future.

Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---
 drivers/ptp/ptp_chardev.c      | 105 +++++++++++++++++++++++++++++++++
 include/uapi/linux/ptp_clock.h |  12 ++++
 2 files changed, 117 insertions(+)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index 18ffe449efdf..94775073527b 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -126,6 +126,7 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 	switch (cmd) {
 
 	case PTP_CLOCK_GETCAPS:
+	case PTP_CLOCK_GETCAPS2:
 		memset(&caps, 0, sizeof(caps));
 		caps.max_adj = ptp->info->max_adj;
 		caps.n_alarm = ptp->info->n_alarm;
@@ -153,6 +154,28 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 		err = ops->enable(ops, &req, enable);
 		break;
 
+	case PTP_EXTTS_REQUEST2:
+		memset(&req, 0, sizeof(req));
+		if (copy_from_user(&req.extts, (void __user *)arg,
+				   sizeof(req.extts))) {
+			err = -EFAULT;
+			break;
+		}
+		if (req.extts.flags || req.extts.rsv[0]
+				|| req.extts.rsv[1]) {
+			err = -EINVAL;
+			break;
+		}
+			
+		if (req.extts.index >= ops->n_ext_ts) {
+			err = -EINVAL;
+			break;
+		}
+		req.type = PTP_CLK_REQ_EXTTS;
+		enable = req.extts.flags & PTP_ENABLE_FEATURE ? 1 : 0;
+		err = ops->enable(ops, &req, enable);
+		break;
+
 	case PTP_PEROUT_REQUEST:
 		if (copy_from_user(&req.perout, (void __user *)arg,
 				   sizeof(req.perout))) {
@@ -168,6 +191,28 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 		err = ops->enable(ops, &req, enable);
 		break;
 
+	case PTP_PEROUT_REQUEST2:
+		memset(&req, 0, sizeof(req));
+		if (copy_from_user(&req.perout, (void __user *)arg,
+				   sizeof(req.perout))) {
+			err = -EFAULT;
+			break;
+		}
+		if (req.perout.flags || req.perout.rsv[0]
+				|| req.perout.rsv[1] || req.perout.rsv[2]
+				|| req.perout.rsv[3]) {
+			err = -EINVAL;
+			break;
+		}
+		if (req.perout.index >= ops->n_per_out) {
+			err = -EINVAL;
+			break;
+		}
+		req.type = PTP_CLK_REQ_PEROUT;
+		enable = req.perout.period.sec || req.perout.period.nsec;
+		err = ops->enable(ops, &req, enable);
+		break;
+
 	case PTP_ENABLE_PPS:
 		if (!capable(CAP_SYS_TIME))
 			return -EPERM;
@@ -176,7 +221,17 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 		err = ops->enable(ops, &req, enable);
 		break;
 
+	case PTP_ENABLE_PPS2:
+		if (!capable(CAP_SYS_TIME))
+			return -EPERM;
+		memset(&req, 0, sizeof(req));
+		req.type = PTP_CLK_REQ_PPS;
+		enable = arg ? 1 : 0;
+		err = ops->enable(ops, &req, enable);
+		break;
+
 	case PTP_SYS_OFFSET_PRECISE:
+	case PTP_SYS_OFFSET_PRECISE2:
 		if (!ptp->info->getcrosststamp) {
 			err = -EOPNOTSUPP;
 			break;
@@ -201,6 +256,7 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 		break;
 
 	case PTP_SYS_OFFSET_EXTENDED:
+	case PTP_SYS_OFFSET_EXTENDED2:
 		if (!ptp->info->gettimex64) {
 			err = -EOPNOTSUPP;
 			break;
@@ -232,6 +288,7 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 		break;
 
 	case PTP_SYS_OFFSET:
+	case PTP_SYS_OFFSET2:
 		sysoff = memdup_user((void __user *)arg, sizeof(*sysoff));
 		if (IS_ERR(sysoff)) {
 			err = PTR_ERR(sysoff);
@@ -284,6 +341,31 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 			err = -EFAULT;
 		break;
 
+	case PTP_PIN_GETFUNC2:
+		memset(&pd, 0, sizeof(pd));
+		if (copy_from_user(&pd, (void __user *)arg, sizeof(pd))) {
+			err = -EFAULT;
+			break;
+		}
+		if (pd.rsv[0] || pd.rsv[1] || pd.rsv[2]
+				|| pd.rsv[3] || pd.rsv[4]) {
+			err = -EINVAL;
+			break;
+		}
+		pin_index = pd.index;
+		if (pin_index >= ops->n_pins) {
+			err = -EINVAL;
+			break;
+		}
+		pin_index = array_index_nospec(pin_index, ops->n_pins);
+		if (mutex_lock_interruptible(&ptp->pincfg_mux))
+			return -ERESTARTSYS;
+		pd = ops->pin_config[pin_index];
+		mutex_unlock(&ptp->pincfg_mux);
+		if (!err && copy_to_user((void __user *)arg, &pd, sizeof(pd)))
+			err = -EFAULT;
+		break;
+
 	case PTP_PIN_SETFUNC:
 		if (copy_from_user(&pd, (void __user *)arg, sizeof(pd))) {
 			err = -EFAULT;
@@ -301,6 +383,29 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 		mutex_unlock(&ptp->pincfg_mux);
 		break;
 
+	case PTP_PIN_SETFUNC2:
+		memset(&pd, 0, sizeof(pd));
+		if (copy_from_user(&pd, (void __user *)arg, sizeof(pd))) {
+			err = -EFAULT;
+			break;
+		}
+		if (pd.rsv[0] || pd.rsv[1] || pd.rsv[2]
+				|| pd.rsv[3] || pd.rsv[4]) {
+			err = -EINVAL;
+			break;
+		}
+		pin_index = pd.index;
+		if (pin_index >= ops->n_pins) {
+			err = -EINVAL;
+			break;
+		}
+		pin_index = array_index_nospec(pin_index, ops->n_pins);
+		if (mutex_lock_interruptible(&ptp->pincfg_mux))
+			return -ERESTARTSYS;
+		err = ptp_set_pinfunc(ptp, pin_index, pd.func, pd.chan);
+		mutex_unlock(&ptp->pincfg_mux);
+		break;
+
 	default:
 		err = -ENOTTY;
 		break;
diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
index 1bc794ad957a..039cd62ec706 100644
--- a/include/uapi/linux/ptp_clock.h
+++ b/include/uapi/linux/ptp_clock.h
@@ -149,6 +149,18 @@ struct ptp_pin_desc {
 #define PTP_SYS_OFFSET_EXTENDED \
 	_IOWR(PTP_CLK_MAGIC, 9, struct ptp_sys_offset_extended)
 
+#define PTP_CLOCK_GETCAPS2  _IOR(PTP_CLK_MAGIC, 10, struct ptp_clock_caps)
+#define PTP_EXTTS_REQUEST2  _IOW(PTP_CLK_MAGIC, 11, struct ptp_extts_request)
+#define PTP_PEROUT_REQUEST2 _IOW(PTP_CLK_MAGIC, 12, struct ptp_perout_request)
+#define PTP_ENABLE_PPS2     _IOW(PTP_CLK_MAGIC, 13, int)
+#define PTP_SYS_OFFSET2     _IOW(PTP_CLK_MAGIC, 14, struct ptp_sys_offset)
+#define PTP_PIN_GETFUNC2    _IOWR(PTP_CLK_MAGIC, 15, struct ptp_pin_desc)
+#define PTP_PIN_SETFUNC2    _IOW(PTP_CLK_MAGIC, 16, struct ptp_pin_desc)
+#define PTP_SYS_OFFSET_PRECISE2 \
+	_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)
+
 struct ptp_extts_event {
 	struct ptp_clock_time t; /* Time event occured. */
 	unsigned int index;      /* Which channel produced the event. */
-- 
2.22.0



-- 
balbi

Re: [RFC PATCH 4/5] PTP: Add flag for non-periodic output

[Richard Cochran]

On Tue, Aug 13, 2019 at 10:53:53AM +0300, Felipe Balbi wrote:
> before I send a new series built on top of this change, I thought I'd
> check with you if I'm on the right path. Below you can find my current
> take at the new IOCTLs. I maintained the same exact structures so that
> there's no maintenance burden. Also introduce a new IOCTL for every
> single one of the previously existing ones even though not all of them
> needed changes. The reason for that was just to make it easier for
> libary authors to update their library by a simple sed script adding '2'
> to the end of the IOCTL macro.

Sounds good.  I have a few comments, below...
 
> Let me know if you want anything to be changed or had a different idea
> about any of this. Also, if you prefer that I finish the entire series
> before you review, no worries either ;-)
> 
> Cheers, patch follows:
> 
> From bc2aa511d4c2e2228590fb29604c6c33b56527ad Mon Sep 17 00:00:00 2001
> From: Felipe Balbi <felipe.balbi@linux.intel.com>
> Date: Tue, 13 Aug 2019 10:32:35 +0300
> Subject: [PATCH] PTP: introduce new versions of IOCTLs
> 
> The current version of the IOCTL have a small problem which prevents
> us from extending the API by making use of reserved fields. In these
> new IOCTLs, we are now making sure that flags and rsv fields are zero
> which will allow us to extend the API in the future.
> 
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> ---
>  drivers/ptp/ptp_chardev.c      | 105 +++++++++++++++++++++++++++++++++
>  include/uapi/linux/ptp_clock.h |  12 ++++
>  2 files changed, 117 insertions(+)
> 
> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
> index 18ffe449efdf..94775073527b 100644
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -126,6 +126,7 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  	switch (cmd) {
>  
>  	case PTP_CLOCK_GETCAPS:
> +	case PTP_CLOCK_GETCAPS2:
>  		memset(&caps, 0, sizeof(caps));
>  		caps.max_adj = ptp->info->max_adj;
>  		caps.n_alarm = ptp->info->n_alarm;
> @@ -153,6 +154,28 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  		err = ops->enable(ops, &req, enable);
>  		break;
>  
> +	case PTP_EXTTS_REQUEST2:
> +		memset(&req, 0, sizeof(req));

This memset not needed, AFAICT.  Oh wait, you want to keep drivers
from seeing stack data in the unused parts of the union.  That is
fine, but please just do that unconditionally at the top of the
function.

> +		if (copy_from_user(&req.extts, (void __user *)arg,
> +				   sizeof(req.extts))) {
> +			err = -EFAULT;
> +			break;
> +		}
> +		if (req.extts.flags || req.extts.rsv[0]
> +				|| req.extts.rsv[1]) {
> +			err = -EINVAL;

Since the code is mostly the same as in the PTP_EXTTS_REQUEST case,
maybe just double up the case statements (like in the other) and add
an extra test for (cmd == PTP_EXTTS_REQUEST2) for this if-block.

> +			break;
> +		}
> +			
> +		if (req.extts.index >= ops->n_ext_ts) {
> +			err = -EINVAL;
> +			break;
> +		}
> +		req.type = PTP_CLK_REQ_EXTTS;
> +		enable = req.extts.flags & PTP_ENABLE_FEATURE ? 1 : 0;
> +		err = ops->enable(ops, &req, enable);
> +		break;
> +
>  	case PTP_PEROUT_REQUEST:
>  		if (copy_from_user(&req.perout, (void __user *)arg,
>  				   sizeof(req.perout))) {
> @@ -168,6 +191,28 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  		err = ops->enable(ops, &req, enable);
>  		break;
>  
> +	case PTP_PEROUT_REQUEST2:
> +		memset(&req, 0, sizeof(req));
> +		if (copy_from_user(&req.perout, (void __user *)arg,
> +				   sizeof(req.perout))) {
> +			err = -EFAULT;
> +			break;
> +		}
> +		if (req.perout.flags || req.perout.rsv[0]
> +				|| req.perout.rsv[1] || req.perout.rsv[2]
> +				|| req.perout.rsv[3]) {
> +			err = -EINVAL;
> +			break;
> +		}

Also this could share code with the legacy ioctl.

> +		if (req.perout.index >= ops->n_per_out) {
> +			err = -EINVAL;
> +			break;
> +		}
> +		req.type = PTP_CLK_REQ_PEROUT;
> +		enable = req.perout.period.sec || req.perout.period.nsec;
> +		err = ops->enable(ops, &req, enable);
> +		break;
> +
>  	case PTP_ENABLE_PPS:
>  		if (!capable(CAP_SYS_TIME))
>  			return -EPERM;
> @@ -176,7 +221,17 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  		err = ops->enable(ops, &req, enable);
>  		break;
>  
> +	case PTP_ENABLE_PPS2:
> +		if (!capable(CAP_SYS_TIME))
> +			return -EPERM;
> +		memset(&req, 0, sizeof(req));

Clearing 'req' unconditionally will make this case the same as the
legacy case.

> +		req.type = PTP_CLK_REQ_PPS;
> +		enable = arg ? 1 : 0;
> +		err = ops->enable(ops, &req, enable);
> +		break;
> +
>  	case PTP_SYS_OFFSET_PRECISE:
> +	case PTP_SYS_OFFSET_PRECISE2:
>  		if (!ptp->info->getcrosststamp) {
>  			err = -EOPNOTSUPP;
>  			break;
> @@ -201,6 +256,7 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  		break;
>  
>  	case PTP_SYS_OFFSET_EXTENDED:
> +	case PTP_SYS_OFFSET_EXTENDED2:
>  		if (!ptp->info->gettimex64) {
>  			err = -EOPNOTSUPP;
>  			break;
> @@ -232,6 +288,7 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  		break;
>  
>  	case PTP_SYS_OFFSET:
> +	case PTP_SYS_OFFSET2:
>  		sysoff = memdup_user((void __user *)arg, sizeof(*sysoff));
>  		if (IS_ERR(sysoff)) {
>  			err = PTR_ERR(sysoff);
> @@ -284,6 +341,31 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  			err = -EFAULT;
>  		break;
>  
> +	case PTP_PIN_GETFUNC2:
> +		memset(&pd, 0, sizeof(pd));

This memset is pointless because of the following copy_from_user().

> +		if (copy_from_user(&pd, (void __user *)arg, sizeof(pd))) {
> +			err = -EFAULT;
> +			break;
> +		}
> +		if (pd.rsv[0] || pd.rsv[1] || pd.rsv[2]
> +				|| pd.rsv[3] || pd.rsv[4]) {
> +			err = -EINVAL;
> +			break;
> +		}

Again maybe share the code?

> +		pin_index = pd.index;
> +		if (pin_index >= ops->n_pins) {
> +			err = -EINVAL;
> +			break;
> +		}
> +		pin_index = array_index_nospec(pin_index, ops->n_pins);
> +		if (mutex_lock_interruptible(&ptp->pincfg_mux))
> +			return -ERESTARTSYS;
> +		pd = ops->pin_config[pin_index];
> +		mutex_unlock(&ptp->pincfg_mux);
> +		if (!err && copy_to_user((void __user *)arg, &pd, sizeof(pd)))
> +			err = -EFAULT;
> +		break;
> +
>  	case PTP_PIN_SETFUNC:
>  		if (copy_from_user(&pd, (void __user *)arg, sizeof(pd))) {
>  			err = -EFAULT;
> @@ -301,6 +383,29 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  		mutex_unlock(&ptp->pincfg_mux);
>  		break;
>  
> +	case PTP_PIN_SETFUNC2:
> +		memset(&pd, 0, sizeof(pd));

memset not needed here either.

> +		if (copy_from_user(&pd, (void __user *)arg, sizeof(pd))) {
> +			err = -EFAULT;
> +			break;
> +		}
> +		if (pd.rsv[0] || pd.rsv[1] || pd.rsv[2]
> +				|| pd.rsv[3] || pd.rsv[4]) {
> +			err = -EINVAL;
> +			break;
> +		}

also shareable.

Thanks,
Richard

> +		pin_index = pd.index;
> +		if (pin_index >= ops->n_pins) {
> +			err = -EINVAL;
> +			break;
> +		}
> +		pin_index = array_index_nospec(pin_index, ops->n_pins);
> +		if (mutex_lock_interruptible(&ptp->pincfg_mux))
> +			return -ERESTARTSYS;
> +		err = ptp_set_pinfunc(ptp, pin_index, pd.func, pd.chan);
> +		mutex_unlock(&ptp->pincfg_mux);
> +		break;
> +
>  	default:
>  		err = -ENOTTY;
>  		break;
> diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
> index 1bc794ad957a..039cd62ec706 100644
> --- a/include/uapi/linux/ptp_clock.h
> +++ b/include/uapi/linux/ptp_clock.h
> @@ -149,6 +149,18 @@ struct ptp_pin_desc {
>  #define PTP_SYS_OFFSET_EXTENDED \
>  	_IOWR(PTP_CLK_MAGIC, 9, struct ptp_sys_offset_extended)
>  
> +#define PTP_CLOCK_GETCAPS2  _IOR(PTP_CLK_MAGIC, 10, struct ptp_clock_caps)
> +#define PTP_EXTTS_REQUEST2  _IOW(PTP_CLK_MAGIC, 11, struct ptp_extts_request)
> +#define PTP_PEROUT_REQUEST2 _IOW(PTP_CLK_MAGIC, 12, struct ptp_perout_request)
> +#define PTP_ENABLE_PPS2     _IOW(PTP_CLK_MAGIC, 13, int)
> +#define PTP_SYS_OFFSET2     _IOW(PTP_CLK_MAGIC, 14, struct ptp_sys_offset)
> +#define PTP_PIN_GETFUNC2    _IOWR(PTP_CLK_MAGIC, 15, struct ptp_pin_desc)
> +#define PTP_PIN_SETFUNC2    _IOW(PTP_CLK_MAGIC, 16, struct ptp_pin_desc)
> +#define PTP_SYS_OFFSET_PRECISE2 \
> +	_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)
> +
>  struct ptp_extts_event {
>  	struct ptp_clock_time t; /* Time event occured. */
>  	unsigned int index;      /* Which channel produced the event. */
> -- 
> 2.22.0
> 
> 
> 
> -- 
> balbi

Re: [RFC PATCH 0/5] PTP: add support for Intel's TGPIO controller

[Richard Cochran]

On Tue, Aug 13, 2019 at 10:50:06AM +0300, Felipe Balbi wrote:
> If we do that we make it difficult for those reading specification and
> trying to find the matching driver.

+1

Thanks,
Richard

Re: [RFC PATCH 4/5] PTP: Add flag for non-periodic output

[Richard Cochran]

On Tue, Aug 13, 2019 at 10:48:21AM -0700, Richard Cochran wrote:
> > +		if (copy_from_user(&req.extts, (void __user *)arg,
> > +				   sizeof(req.extts))) {
> > +			err = -EFAULT;
> > +			break;
> > +		}
> > +		if (req.extts.flags || req.extts.rsv[0]
> > +				|| req.extts.rsv[1]) {
> > +			err = -EINVAL;
> 
> Since the code is mostly the same as in the PTP_EXTTS_REQUEST case,
> maybe just double up the case statements (like in the other) and add
> an extra test for (cmd == PTP_EXTTS_REQUEST2) for this if-block.

Thinking about the drivers, in the case of the legacy ioctls, let's
also be sure to clear the flags and reserved fields before passing
them to the drivers.

Thanks,
Richard

Re: [RFC PATCH 4/5] PTP: Add flag for non-periodic output

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 904 bytes --]


Hi,

Richard Cochran <richardcochran@gmail.com> writes:

> On Tue, Aug 13, 2019 at 10:48:21AM -0700, Richard Cochran wrote:
>> > +		if (copy_from_user(&req.extts, (void __user *)arg,
>> > +				   sizeof(req.extts))) {
>> > +			err = -EFAULT;
>> > +			break;
>> > +		}
>> > +		if (req.extts.flags || req.extts.rsv[0]
>> > +				|| req.extts.rsv[1]) {
>> > +			err = -EINVAL;
>> 
>> Since the code is mostly the same as in the PTP_EXTTS_REQUEST case,
>> maybe just double up the case statements (like in the other) and add
>> an extra test for (cmd == PTP_EXTTS_REQUEST2) for this if-block.
>
> Thinking about the drivers, in the case of the legacy ioctls, let's
> also be sure to clear the flags and reserved fields before passing
> them to the drivers.

makes sense to me. I'll update per your requests and send only this
patch officially. Thanks for the pointers.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [RFC PATCH 1/5] x86: tsc: add tsc to art helpers

[Felipe Balbi]

Hi,


Thomas Gleixner <tglx@linutronix.de> writes:

> Felipe,
>
> On Tue, 16 Jul 2019, Felipe Balbi wrote:
>
> -ENOCHANGELOG
>
> As you said in the cover letter:
>
>>  (3) The change in arch/x86/kernel/tsc.c needs to be reviewed at length
>>      before going in.
>
> So some information what those interfaces are used for and why they are
> needed would be really helpful.

Okay, I have some more details about this. The TGPIO device itself uses
ART since TSC is not directly available to anything other than the
CPU. The 'problem' here is that reading ART incurs extra latency which
we would like to avoid. Therefore, we use TSC and scale it to
nanoseconds which, would be the same as ART to ns.

>> +void get_tsc_ns(struct system_counterval_t *tsc_counterval, u64 *tsc_ns)
>> +{
>> +	u64 tmp, res, rem;
>> +	u64 cycles;
>> +
>> +	tsc_counterval->cycles = clocksource_tsc.read(NULL);
>> +	cycles = tsc_counterval->cycles;
>> +	tsc_counterval->cs = art_related_clocksource;
>> +
>> +	rem = do_div(cycles, tsc_khz);
>> +
>> +	res = cycles * USEC_PER_SEC;
>> +	tmp = rem * USEC_PER_SEC;
>> +
>> +	do_div(tmp, tsc_khz);
>> +	res += tmp;
>> +
>> +	*tsc_ns = res;
>> +}
>> +EXPORT_SYMBOL(get_tsc_ns);
>> +
>> +u64 get_art_ns_now(void)
>> +{
>> +	struct system_counterval_t tsc_cycles;
>> +	u64 tsc_ns;
>> +
>> +	get_tsc_ns(&tsc_cycles, &tsc_ns);
>> +
>> +	return tsc_ns;
>> +}
>> +EXPORT_SYMBOL(get_art_ns_now);
>
> While the changes look innocuous I'm missing the big picture why this needs
> to emulate ART instead of simply using TSC directly.

i don't think we're emulating ART here (other than the name in the
function). We're just reading TSC and converting to nanoseconds, right?

Cheers

-- 
balbi

Re: [RFC PATCH 1/5] x86: tsc: add tsc to art helpers

[Thomas Gleixner]

Felipe,

On Thu, 15 Aug 2019, Felipe Balbi wrote:
> Thomas Gleixner <tglx@linutronix.de> writes:
> > On Tue, 16 Jul 2019, Felipe Balbi wrote:
> >
> > So some information what those interfaces are used for and why they are
> > needed would be really helpful.
> 
> Okay, I have some more details about this. The TGPIO device itself uses
> ART since TSC is not directly available to anything other than the
> CPU. The 'problem' here is that reading ART incurs extra latency which
> we would like to avoid. Therefore, we use TSC and scale it to
> nanoseconds which, would be the same as ART to ns.

Fine. But that's not really correct:

      TSC = art_to_tsc_offset + ART * scale;
 
> >> +void get_tsc_ns(struct system_counterval_t *tsc_counterval, u64 *tsc_ns)

Why is this not returning the result instead of having that pointer
indirection?

> >> +{
> >> +	u64 tmp, res, rem;
> >> +	u64 cycles;
> >> +
> >> +	tsc_counterval->cycles = clocksource_tsc.read(NULL);
> >> +	cycles = tsc_counterval->cycles;
> >> +	tsc_counterval->cs = art_related_clocksource;

So this does more than returning the TSC time converted to nanoseconds. The
function name should reflect this. Plus both functions want kernel-doc
explaining what they do.

> >> +	rem = do_div(cycles, tsc_khz);
> >> +
> >> +	res = cycles * USEC_PER_SEC;
> >> +	tmp = rem * USEC_PER_SEC;
> >> +
> >> +	do_div(tmp, tsc_khz);
> >> +	res += tmp;
> >> +
> >> +	*tsc_ns = res;
> >> +}
> >> +EXPORT_SYMBOL(get_tsc_ns);
> >> +
> >> +u64 get_art_ns_now(void)
> >> +{
> >> +	struct system_counterval_t tsc_cycles;
> >> +	u64 tsc_ns;
> >> +
> >> +	get_tsc_ns(&tsc_cycles, &tsc_ns);
> >> +
> >> +	return tsc_ns;
> >> +}
> >> +EXPORT_SYMBOL(get_art_ns_now);
> >
> > While the changes look innocuous I'm missing the big picture why this needs
> > to emulate ART instead of simply using TSC directly.
> 
> i don't think we're emulating ART here (other than the name in the
> function). We're just reading TSC and converting to nanoseconds, right?

Well, the function name says clearly: get_art_ns_now(). But you are not
using ART, you use the TSC and derive the ART value from it (incorrectly).

Thanks,

	tglx

Re: [RFC PATCH 1/5] x86: tsc: add tsc to art helpers

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2080 bytes --]


Hi,

(sorry for the long delay, got caught up in other tasks)

Thomas Gleixner <tglx@linutronix.de> writes:
> On Thu, 15 Aug 2019, Felipe Balbi wrote:
>> Thomas Gleixner <tglx@linutronix.de> writes:
>> > On Tue, 16 Jul 2019, Felipe Balbi wrote:
>> >
>> > So some information what those interfaces are used for and why they are
>> > needed would be really helpful.
>> 
>> Okay, I have some more details about this. The TGPIO device itself uses
>> ART since TSC is not directly available to anything other than the
>> CPU. The 'problem' here is that reading ART incurs extra latency which
>> we would like to avoid. Therefore, we use TSC and scale it to
>> nanoseconds which, would be the same as ART to ns.
>
> Fine. But that's not really correct:
>
>       TSC = art_to_tsc_offset + ART * scale;

From silicon folks I got the equation:

ART = ECX * EBX / EAX;

If I'm reading this correctly, that's basically what
native_calibrate_tsc() does (together with some error checking the safe
defaults). Couldn't we, instead, just have a single function like below?

u64 convert_tsc_to_art_ns()
{
	return x86_platform.calibrate_tsc();
}

Another way would be extract the important parts from
native_calibrate_tsc() into a separate helper. This would safe another
call to cpuid(0x15,...);

>> >> +void get_tsc_ns(struct system_counterval_t *tsc_counterval, u64 *tsc_ns)
>
> Why is this not returning the result instead of having that pointer
> indirection?

That can be changed easily, no worries.

>> >> +{
>> >> +	u64 tmp, res, rem;
>> >> +	u64 cycles;
>> >> +
>> >> +	tsc_counterval->cycles = clocksource_tsc.read(NULL);
>> >> +	cycles = tsc_counterval->cycles;
>> >> +	tsc_counterval->cs = art_related_clocksource;
>
> So this does more than returning the TSC time converted to nanoseconds. The
> function name should reflect this. Plus both functions want kernel-doc
> explaining what they do.

convert_tsc_to_art_ns()? That would be analogous to convert_art_to_tsc()
and convert_art_ns_to_tsc().

cheers

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [RFC PATCH 1/5] x86: tsc: add tsc to art helpers

[Thomas Gleixner]

Hi,

On Tue, 1 Oct 2019, Felipe Balbi wrote:
> (sorry for the long delay, got caught up in other tasks)

Delayed by vacation :)

> Thomas Gleixner <tglx@linutronix.de> writes:
> > On Thu, 15 Aug 2019, Felipe Balbi wrote:
> >> Thomas Gleixner <tglx@linutronix.de> writes:
> >> > On Tue, 16 Jul 2019, Felipe Balbi wrote:
> >> >
> >> > So some information what those interfaces are used for and why they are
> >> > needed would be really helpful.
> >> 
> >> Okay, I have some more details about this. The TGPIO device itself uses
> >> ART since TSC is not directly available to anything other than the
> >> CPU. The 'problem' here is that reading ART incurs extra latency which
> >> we would like to avoid. Therefore, we use TSC and scale it to
> >> nanoseconds which, would be the same as ART to ns.
> >
> > Fine. But that's not really correct:
> >
> >       TSC = art_to_tsc_offset + ART * scale;
> 
> From silicon folks I got the equation:
> 
> ART = ECX * EBX / EAX;

What is the content of ECX/EBX/EAX and where is it coming from?
 
> If I'm reading this correctly, that's basically what
> native_calibrate_tsc() does (together with some error checking the safe
> defaults). Couldn't we, instead, just have a single function like below?
> 
> u64 convert_tsc_to_art_ns()
> {
> 	return x86_platform.calibrate_tsc();
> }

Huch? How is that supposed to work? calibrate_tsc() returns the TSC
frequency.

> Another way would be extract the important parts from
> native_calibrate_tsc() into a separate helper. This would safe another
> call to cpuid(0x15,...);

What for?

The relation between TSC and ART is already established via detect_art()
which reads all relevant data out of CPUID(ART_CPUID_LEAF).

We use exactly that information for convert_art_to_tsc() so the obvious
solution for calculating ART from TSC is to do the reverse operation.

convert_art_to_tsc()
{
        rem = do_div(art, art_to_tsc_denominator);

        res = art * art_to_tsc_numerator;
        tmp = rem * art_to_tsc_numerator;

        do_div(tmp, art_to_tsc_denominator);
        res += tmp + art_to_tsc_offset;
}

which is translated into math:

      TSC = ART * SCALE + OFFSET

where

      SCALE = N / D

and

      N = CPUID(ART_CPUID_LEAF).EAX
      D = CPUID(ART_CPUID_LEAF).EBX

So the obvious reverse operation is:

     ART = (TSC - OFFSET) / SCALE;

Translating that into code should not be rocket science.

Thanks,

	tglx

Re: [RFC PATCH 1/5] x86: tsc: add tsc to art helpers

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3238 bytes --]


Hi,

Thomas Gleixner <tglx@linutronix.de> writes:
>> Thomas Gleixner <tglx@linutronix.de> writes:
>> > On Thu, 15 Aug 2019, Felipe Balbi wrote:
>> >> Thomas Gleixner <tglx@linutronix.de> writes:
>> >> > On Tue, 16 Jul 2019, Felipe Balbi wrote:
>> >> >
>> >> > So some information what those interfaces are used for and why they are
>> >> > needed would be really helpful.
>> >> 
>> >> Okay, I have some more details about this. The TGPIO device itself uses
>> >> ART since TSC is not directly available to anything other than the
>> >> CPU. The 'problem' here is that reading ART incurs extra latency which
>> >> we would like to avoid. Therefore, we use TSC and scale it to
>> >> nanoseconds which, would be the same as ART to ns.
>> >
>> > Fine. But that's not really correct:
>> >
>> >       TSC = art_to_tsc_offset + ART * scale;
>> 
>> From silicon folks I got the equation:
>> 
>> ART = ECX * EBX / EAX;
>
> What is the content of ECX/EBX/EAX and where is it coming from?

Since last email, I got a bit of extra information about how all of
these should work.

ECX contains crystal rate of TSC, EBX and EAX contain constants for
converting between TSC and ART.

So, ART = tsc_cycles * EBX/EAX, this will give me ART cycles. Also, the
time gpio IP needs ART cycles to be written to its comparator
registers, but the PTP framework wants time in nanoseconds.

Therefore we need two new conversion functions:
convert_tsc_to_art_cycles() and something which gives us current TSC in
nanoseconds.

>> If I'm reading this correctly, that's basically what
>> native_calibrate_tsc() does (together with some error checking the safe
>> defaults). Couldn't we, instead, just have a single function like below?
>> 
>> u64 convert_tsc_to_art_ns()
>> {
>> 	return x86_platform.calibrate_tsc();
>> }
>
> Huch? How is that supposed to work? calibrate_tsc() returns the TSC
> frequency.

Yup, that was a total brain fart.

>> Another way would be extract the important parts from
>> native_calibrate_tsc() into a separate helper. This would safe another
>> call to cpuid(0x15,...);
>
> What for?
>
> The relation between TSC and ART is already established via detect_art()
> which reads all relevant data out of CPUID(ART_CPUID_LEAF).
>
> We use exactly that information for convert_art_to_tsc() so the obvious
> solution for calculating ART from TSC is to do the reverse operation.
>
> convert_art_to_tsc()
> {
>         rem = do_div(art, art_to_tsc_denominator);
>
>         res = art * art_to_tsc_numerator;
>         tmp = rem * art_to_tsc_numerator;
>
>         do_div(tmp, art_to_tsc_denominator);
>         res += tmp + art_to_tsc_offset;
> }
>
> which is translated into math:
>
>       TSC = ART * SCALE + OFFSET
>
> where
>
>       SCALE = N / D
>
> and
>
>       N = CPUID(ART_CPUID_LEAF).EAX
>       D = CPUID(ART_CPUID_LEAF).EBX
>
> So the obvious reverse operation is:
>
>      ART = (TSC - OFFSET) / SCALE;
>
> Translating that into code should not be rocket science.

Right, that's where we got after talking to other folks.

Do you have any preferences for the function names? Or does
convert_tsc_to_art() sound ok?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]