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
Signed-off-by: Felipe Balbi <[email protected]>
---
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
Add a driver supporting Intel Timed GPIO controller available as part
of some Intel PMCs.
Signed-off-by: Felipe Balbi <[email protected]>
---
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 <[email protected]>
+ */
+
+#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 <[email protected]>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Intel PMC Timed GPIO Controller Driver");
--
2.22.0
This will be used for frequency discipline adjustments.
Signed-off-by: Felipe Balbi <[email protected]>
---
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
With this, we can request the underlying driver to count the number of
events that have been captured.
Signed-off-by: Felipe Balbi <[email protected]>
---
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
When this new flag is set, we can use single-shot output.
Signed-off-by: Felipe Balbi <[email protected]>
---
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
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
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 <[email protected]>
> ---
> 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
>
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
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 <[email protected]>
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 <[email protected]>
> + */
> +
> +#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 <[email protected]>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Intel PMC Timed GPIO Controller Driver");
Hi Richard,
Richard Cochran <[email protected]> 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 <[email protected]>
>> ---
>> 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
Hi,
Shannon Nelson <[email protected]> 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 <[email protected]>
>
> 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
Hi,
Richard Cochran <[email protected]> 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
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
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
Hi,
Richard Cochran <[email protected]> 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
Hi,
Richard Cochran <[email protected]> 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
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
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
Hi,
Andrew Lunn <[email protected]> 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
On Fri, Jul 19, 2019 at 10:35:14AM +0300, Felipe Balbi wrote:
>
> Hi,
>
> Andrew Lunn <[email protected]> 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
Hi,
Andrew Lunn <[email protected]> writes:
>> Andrew Lunn <[email protected]> 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
Hi,
Richard Cochran <[email protected]> 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 <[email protected]>
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 <[email protected]>
---
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
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
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
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 <[email protected]>
> 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 <[email protected]>
> ---
> 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
Hi,
Richard Cochran <[email protected]> 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
Hi,
Thomas Gleixner <[email protected]> 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
Felipe,
On Thu, 15 Aug 2019, Felipe Balbi wrote:
> Thomas Gleixner <[email protected]> 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
Hi,
(sorry for the long delay, got caught up in other tasks)
Thomas Gleixner <[email protected]> writes:
> On Thu, 15 Aug 2019, Felipe Balbi wrote:
>> Thomas Gleixner <[email protected]> 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
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 <[email protected]> writes:
> > On Thu, 15 Aug 2019, Felipe Balbi wrote:
> >> Thomas Gleixner <[email protected]> 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
Hi,
Thomas Gleixner <[email protected]> writes:
>> Thomas Gleixner <[email protected]> writes:
>> > On Thu, 15 Aug 2019, Felipe Balbi wrote:
>> >> Thomas Gleixner <[email protected]> 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