[PATCH v12 0/3] Add support for Intel PPS Generator

[PATCH v12 0/3] Add support for Intel PPS Generator

[subramanian.mohan]

From: Subramanian Mohan <subramanian.mohan@intel.com>

The goal of the PPS (Pulse Per Second) hardware/software is to generate a
signal from the system on a wire so that some third-party hardware can
observe that signal and judge how close the system's time is to another
system or piece of hardware.

Existing methods (like parallel ports) require software to flip a bit at
just the right time to create a PPS signal. Many things can prevent
software from doing this precisely. This (Timed I/O) method is better
because software only "arms" the hardware in advance and then depends on
the hardware to "fire" and flip the signal at just the right time.

To generate a PPS signal with this new hardware, the kernel wakes up
twice a second, once for 1->0 edge and other for the 0->1 edge. It does
this shortly (~10ms) before the actual change in the signal needs to be
made. It computes the TSC value at which edge will happen, convert to a
value hardware understands and program this value to Timed I/O hardware.
The actual edge transition happens without any further action from the
kernel.

The result here is a signal coming out of the system that is roughly
1,000 times more accurate than the old methods. If the system is heavily
loaded, the difference in accuracy is larger in old methods.

Application Interface:
The API to use Timed I/O is very simple. It is enabled and disabled by
writing a '1' or '0' value to the sysfs enable attribute associated with
the Timed I/O PPS device. Each Timed I/O pin is represented by a PPS
device. When enabled, a pulse-per-second (PPS) synchronized with the
system clock is continuously produced on the Timed I/O pin, otherwise it
is pulled low.

The Timed I/O signal on the motherboard is enabled in the BIOS setup.
Intel Advanced Menu -> PCH IO Configuration -> Timed I/O <Enable>

References:
https://en.wikipedia.org/wiki/Pulse-per-second_signal
https://drive.google.com/file/d/1vkBRRDuELmY8I3FlfOZaEBp-DxLW6t_V/view
https://youtu.be/JLUTT-lrDqw

Patch 1 adds the pps(pulse per second) generator tio driver to the pps
subsystem.
Patch 2 documentation and usage of the pps tio generator module.
Patch 3 includes documentation for sysfs interface.

These patches are based on the timers/core branch:
[1] https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/?h=timers/core
These changes are dependent on patches that are merged in [1].

Please help to review the changes.

Thanks in advance,
Subramanian Mohan

Changes from v2:
 - Split patch 1 to remove the functions in later stages.
 - Include required headers in pps_gen_tio.

Changes from v3:
 - Corrections in Documentation.
 - Introducing non-RFC version of the patch series.

Changes from v4:
 - Setting id in ice_ptp
 - Modified conversion logic in convert_base_to_cs.
 - Included the usage of the APIs in the commit message of 2nd patch.

Changes from v5:
 - Change nsecs variable to use_nsecs.
 - Change order of 1&2 patches and modify the commit message.
 - Add sysfs abi file entry in MAINTAINERS file.
 - Add check to find if any event is missed and disable hardware
   accordingly.

Changes from v6:
 - Split patch 1 into 1&2 patches.
 - Add check for overflow in convert_ns_to_cs().
 - Refine commit messages.

Changes from v7:
 - Split the if condition and return error if current time exceeds
   expire time.
 - Update kernel version and month in ABI file.

Changes from v8:
 - Add function to enable Timed I/O.
 - Changed the updating of tio->enabled to a centralized place in
   disable and enable functions.

Changes from v9:
 - use tio->enabled instead of reading ctrl register.
 - change error code in enable_store to -ENODEV.

Changes from v10:
 - Rebased to latest codebase.

Changes from v11:
 - Rebased to latest codebase.

Subramanian Mohan (3):
  pps: generators: Add PPS Generator TIO Driver
  Documentation: driver-api: pps: Add Intel Timed I/O PPS generator
  ABI: pps: Add ABI documentation for Intel TIO

 .../ABI/testing/sysfs-platform-pps-tio        |   8 +
 Documentation/driver-api/pps.rst              |  24 ++
 MAINTAINERS                                   |   1 +
 drivers/pps/generators/Kconfig                |  16 ++
 drivers/pps/generators/Makefile               |   1 +
 drivers/pps/generators/pps_gen_tio.c          | 262 ++++++++++++++++++
 6 files changed, 312 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-pps-tio
 create mode 100644 drivers/pps/generators/pps_gen_tio.c

-- 
2.35.3

[PATCH v12 1/3] pps: generators: Add PPS Generator TIO Driver

[subramanian.mohan]

From: Subramanian Mohan <subramanian.mohan@intel.com>

The Intel Timed IO PPS generator driver outputs a PPS signal using
dedicated hardware that is more accurate than software actuated PPS.
The Timed IO hardware generates output events using the ART timer.
The ART timer period varies based on platform type, but is less than 100
nanoseconds for all current platforms. Timed IO output accuracy is
within 1 ART period.

PPS output is enabled by writing '1' the 'enable' sysfs attribute. The
driver uses hrtimers to schedule a wake-up 10 ms before each event
(edge) target time. At wakeup, the driver converts the target time in
terms of CLOCK_REALTIME to ART trigger time and writes this to the Timed
IO hardware. The Timed IO hardware generates an event precisely at the
requested system time without software involvement.

Co-developed-by: Christopher Hall <christopher.s.hall@intel.com>
Signed-off-by: Christopher Hall <christopher.s.hall@intel.com>
Co-developed-by: Pandith N <pandith.n@intel.com>
Signed-off-by: Pandith N <pandith.n@intel.com>
Co-developed-by: Thejesh Reddy T R <thejesh.reddy.t.r@intel.com>
Signed-off-by: Thejesh Reddy T R <thejesh.reddy.t.r@intel.com>
Signed-off-by: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>
Reviewed-by: Eddie Dong <eddie.dong@intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Rodolfo Giometti <giometti@enneenne.com>
Signed-off-by: Subramanian Mohan <subramanian.mohan@intel.com>
---
 drivers/pps/generators/Kconfig       |  16 ++
 drivers/pps/generators/Makefile      |   1 +
 drivers/pps/generators/pps_gen_tio.c | 262 +++++++++++++++++++++++++++
 3 files changed, 279 insertions(+)
 create mode 100644 drivers/pps/generators/pps_gen_tio.c

diff --git a/drivers/pps/generators/Kconfig b/drivers/pps/generators/Kconfig
index d615e640fcad..0f090932336f 100644
--- a/drivers/pps/generators/Kconfig
+++ b/drivers/pps/generators/Kconfig
@@ -12,3 +12,19 @@ config PPS_GENERATOR_PARPORT
 	  If you say yes here you get support for a PPS signal generator which
 	  utilizes STROBE pin of a parallel port to send PPS signals. It uses
 	  parport abstraction layer and hrtimers to precisely control the signal.
+
+config PPS_GENERATOR_TIO
+	tristate "TIO PPS signal generator"
+	depends on X86 && CPU_SUP_INTEL
+	help
+	  If you say yes here you get support for a PPS TIO signal generator
+	  which generates a pulse at a prescribed time based on the system clock.
+	  It uses time translation and hrtimers to precisely generate a pulse.
+	  This hardware is present on 2019 and newer Intel CPUs. However, this
+	  driver is not useful without adding highly specialized hardware outside
+	  the Linux system to observe these pulses.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pps_gen_tio.
+
+	  If unsure, say N.
diff --git a/drivers/pps/generators/Makefile b/drivers/pps/generators/Makefile
index 2589fd0f2481..714e847ae193 100644
--- a/drivers/pps/generators/Makefile
+++ b/drivers/pps/generators/Makefile
@@ -4,5 +4,6 @@
 #
 
 obj-$(CONFIG_PPS_GENERATOR_PARPORT) += pps_gen_parport.o
+obj-$(CONFIG_PPS_GENERATOR_TIO) += pps_gen_tio.o
 
 ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG
diff --git a/drivers/pps/generators/pps_gen_tio.c b/drivers/pps/generators/pps_gen_tio.c
new file mode 100644
index 000000000000..ae5d351be479
--- /dev/null
+++ b/drivers/pps/generators/pps_gen_tio.c
@@ -0,0 +1,262 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Intel PPS signal Generator Driver
+ *
+ * Copyright (C) 2024 Intel Corporation
+ */
+
+#include <linux/bits.h>
+#include <linux/bitfield.h>
+#include <linux/cleanup.h>
+#include <linux/container_of.h>
+#include <linux/cpu.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/hrtimer.h>
+#include <linux/io-64-nonatomic-hi-lo.h>
+#include <linux/kstrtox.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/sysfs.h>
+#include <linux/timekeeping.h>
+#include <linux/types.h>
+
+#include <asm/cpu_device_id.h>
+
+#define TIOCTL			0x00
+#define TIOCOMPV		0x10
+#define TIOEC			0x30
+
+/* Control Register */
+#define TIOCTL_EN			BIT(0)
+#define TIOCTL_DIR			BIT(1)
+#define TIOCTL_EP			GENMASK(3, 2)
+#define TIOCTL_EP_RISING_EDGE		FIELD_PREP(TIOCTL_EP, 0)
+#define TIOCTL_EP_FALLING_EDGE		FIELD_PREP(TIOCTL_EP, 1)
+#define TIOCTL_EP_TOGGLE_EDGE		FIELD_PREP(TIOCTL_EP, 2)
+
+#define SAFE_TIME_NS			(10 * NSEC_PER_MSEC) /* Safety time to set hrtimer early */
+#define MAGIC_CONST			(NSEC_PER_SEC - SAFE_TIME_NS)
+#define ART_HW_DELAY_CYCLES		2
+
+struct pps_tio {
+	struct hrtimer timer;
+	struct device *dev;
+	spinlock_t lock;
+	struct attribute_group attrs;
+	void __iomem *base;
+	bool enabled;
+	u32 prev_count;
+};
+
+static inline u32 pps_tio_read(struct pps_tio *tio, u32 offset)
+{
+	return readl(tio->base + offset);
+}
+
+static inline void pps_ctl_write(struct pps_tio *tio, u32 value)
+{
+	writel(value, tio->base + TIOCTL);
+}
+
+/* For COMPV register, It's safer to write higher 32-bit followed by lower 32-bit */
+static inline void pps_compv_write(struct pps_tio *tio, u64 value)
+{
+	hi_lo_writeq(value, tio->base + TIOCOMPV);
+}
+
+static inline ktime_t first_event(struct pps_tio *tio)
+{
+	return ktime_set(ktime_get_real_seconds() + 1, MAGIC_CONST);
+}
+
+static u32 pps_tio_disable(struct pps_tio *tio)
+{
+	u32 ctrl;
+
+	ctrl = pps_tio_read(tio, TIOCTL);
+	pps_compv_write(tio, 0);
+
+	ctrl &= ~TIOCTL_EN;
+	pps_ctl_write(tio, ctrl);
+	tio->enabled = false;
+	tio->prev_count = 0;
+
+	return ctrl;
+}
+
+static void pps_tio_enable(struct pps_tio *tio)
+{
+	u32 ctrl;
+
+	ctrl = pps_tio_read(tio, TIOCTL);
+	ctrl |= TIOCTL_EN;
+	pps_ctl_write(tio, ctrl);
+	tio->enabled = true;
+}
+
+static void pps_tio_direction_output(struct pps_tio *tio)
+{
+	u32 ctrl;
+
+	ctrl = pps_tio_disable(tio);
+
+	/* We enable the device, be sure that the 'compare' value is invalid */
+	pps_compv_write(tio, 0);
+
+	ctrl &= ~(TIOCTL_DIR | TIOCTL_EP);
+	ctrl |= TIOCTL_EP_TOGGLE_EDGE;
+	pps_ctl_write(tio, ctrl);
+	pps_tio_enable(tio);
+}
+
+static bool pps_generate_next_pulse(struct pps_tio *tio, ktime_t expires)
+{
+	u64 art;
+
+	if (!ktime_real_to_base_clock(expires, CSID_X86_ART, &art)) {
+		pps_tio_disable(tio);
+		return false;
+	}
+
+	pps_compv_write(tio, art - ART_HW_DELAY_CYCLES);
+	return true;
+}
+
+static enum hrtimer_restart hrtimer_callback(struct hrtimer *timer)
+{
+	struct pps_tio *tio = container_of(timer, struct pps_tio, timer);
+	ktime_t expires, now;
+	u32 event_count;
+
+	guard(spinlock)(&tio->lock);
+
+	/* Check if any event is missed. If an event is missed, TIO will be disabled*/
+	event_count = pps_tio_read(tio, TIOEC);
+	if (tio->prev_count && tio->prev_count == event_count)
+		goto err;
+	tio->prev_count = event_count;
+	expires = hrtimer_get_expires(timer);
+	now = ktime_get_real();
+
+	if (now - expires >= SAFE_TIME_NS)
+		goto err;
+
+	tio->enabled = pps_generate_next_pulse(tio, expires + SAFE_TIME_NS);
+	if (!tio->enabled)
+		return HRTIMER_NORESTART;
+
+	hrtimer_forward(timer, now, NSEC_PER_SEC / 2);
+	return HRTIMER_RESTART;
+err:
+	dev_err(tio->dev, "Event missed, Disabling Timed I/O");
+	pps_tio_disable(tio);
+	return HRTIMER_NORESTART;
+}
+
+static ssize_t enable_store(struct device *dev, struct device_attribute *attr, const char *buf,
+			    size_t count)
+{
+	struct pps_tio *tio = dev_get_drvdata(dev);
+	bool enable;
+	int err;
+
+	if (!timekeeping_clocksource_has_base(CSID_X86_ART)) {
+		dev_err_once(dev, "PPS cannot be used as clock is not related to ART");
+		return -ENODEV;
+	}
+
+	err = kstrtobool(buf, &enable);
+	if (err)
+		return err;
+
+	guard(spinlock_irqsave)(&tio->lock);
+	if (enable && !tio->enabled) {
+		pps_tio_direction_output(tio);
+		hrtimer_start(&tio->timer, first_event(tio), HRTIMER_MODE_ABS);
+	} else if (!enable && tio->enabled) {
+		hrtimer_cancel(&tio->timer);
+		pps_tio_disable(tio);
+	}
+	return count;
+}
+
+static ssize_t enable_show(struct device *dev, struct device_attribute *devattr, char *buf)
+{
+	struct pps_tio *tio = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%u\n", tio->enabled);
+}
+static DEVICE_ATTR_RW(enable);
+
+static struct attribute *pps_tio_attrs[] = {
+	&dev_attr_enable.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(pps_tio);
+
+static int pps_gen_tio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct pps_tio *tio;
+
+	if (!(cpu_feature_enabled(X86_FEATURE_TSC_KNOWN_FREQ) &&
+	      cpu_feature_enabled(X86_FEATURE_ART))) {
+		dev_warn(dev, "TSC/ART is not enabled");
+		return -ENODEV;
+	}
+
+	tio = devm_kzalloc(dev, sizeof(*tio), GFP_KERNEL);
+	if (!tio)
+		return -ENOMEM;
+
+	tio->dev = dev;
+	tio->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(tio->base))
+		return PTR_ERR(tio->base);
+
+	pps_tio_disable(tio);
+	hrtimer_init(&tio->timer, CLOCK_REALTIME, HRTIMER_MODE_ABS);
+	tio->timer.function = hrtimer_callback;
+	spin_lock_init(&tio->lock);
+	platform_set_drvdata(pdev, tio);
+
+	return 0;
+}
+
+static void pps_gen_tio_remove(struct platform_device *pdev)
+{
+	struct pps_tio *tio = platform_get_drvdata(pdev);
+
+	hrtimer_cancel(&tio->timer);
+	pps_tio_disable(tio);
+}
+
+static const struct acpi_device_id intel_pmc_tio_acpi_match[] = {
+	{ "INTC1021" },
+	{ "INTC1022" },
+	{ "INTC1023" },
+	{ "INTC1024" },
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, intel_pmc_tio_acpi_match);
+
+static struct platform_driver pps_gen_tio_driver = {
+	.probe          = pps_gen_tio_probe,
+	.remove         = pps_gen_tio_remove,
+	.driver         = {
+		.name                   = "intel-pps-gen-tio",
+		.acpi_match_table       = intel_pmc_tio_acpi_match,
+		.dev_groups             = pps_tio_groups,
+	},
+};
+module_platform_driver(pps_gen_tio_driver);
+
+MODULE_AUTHOR("Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>");
+MODULE_AUTHOR("Christopher Hall <christopher.s.hall@intel.com>");
+MODULE_AUTHOR("Pandith N <pandith.n@intel.com>");
+MODULE_AUTHOR("Thejesh Reddy T R <thejesh.reddy.t.r@intel.com>");
+MODULE_DESCRIPTION("Intel PMC Time-Aware IO Generator Driver");
+MODULE_LICENSE("GPL");
-- 
2.35.3

Re: [PATCH v12 1/3] pps: generators: Add PPS Generator TIO Driver

[Andy Shevchenko]

On Fri, Aug 23, 2024 at 12:31:06PM +0530, subramanian.mohan@intel.com wrote:
> From: Subramanian Mohan <subramanian.mohan@intel.com>
> 
> The Intel Timed IO PPS generator driver outputs a PPS signal using
> dedicated hardware that is more accurate than software actuated PPS.
> The Timed IO hardware generates output events using the ART timer.
> The ART timer period varies based on platform type, but is less than 100
> nanoseconds for all current platforms. Timed IO output accuracy is
> within 1 ART period.
> 
> PPS output is enabled by writing '1' the 'enable' sysfs attribute. The
> driver uses hrtimers to schedule a wake-up 10 ms before each event
> (edge) target time. At wakeup, the driver converts the target time in
> terms of CLOCK_REALTIME to ART trigger time and writes this to the Timed
> IO hardware. The Timed IO hardware generates an event precisely at the
> requested system time without software involvement.

...

> +#include <linux/bits.h>
> +#include <linux/bitfield.h>

These two should be swapped in ordering

> +#include <linux/cleanup.h>
> +#include <linux/container_of.h>
> +#include <linux/cpu.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/hrtimer.h>
> +#include <linux/io-64-nonatomic-hi-lo.h>
> +#include <linux/kstrtox.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include <linux/sysfs.h>
> +#include <linux/timekeeping.h>
> +#include <linux/types.h>

...

> +#define SAFE_TIME_NS			(10 * NSEC_PER_MSEC) /* Safety time to set hrtimer early */

It's better to have

/* ...comment... */
...definition...

...

> +static enum hrtimer_restart hrtimer_callback(struct hrtimer *timer)
> +{
> +	struct pps_tio *tio = container_of(timer, struct pps_tio, timer);
> +	ktime_t expires, now;
> +	u32 event_count;
> +
> +	guard(spinlock)(&tio->lock);
> +
> +	/* Check if any event is missed. If an event is missed, TIO will be disabled*/

/*
 * Missing space at the of the comment. But since it's two-sentences comment,
 * make it multi-line like in this example.
 */

> +	event_count = pps_tio_read(tio, TIOEC);
> +	if (tio->prev_count && tio->prev_count == event_count)
> +		goto err;
> +	tio->prev_count = event_count;

+ Blank line.

> +	expires = hrtimer_get_expires(timer);

(1)

> +	now = ktime_get_real();
> +

The location of this blank line seems incorrect and should be moved to (1) above.

> +	if (now - expires >= SAFE_TIME_NS)
> +		goto err;
> +
> +	tio->enabled = pps_generate_next_pulse(tio, expires + SAFE_TIME_NS);
> +	if (!tio->enabled)
> +		return HRTIMER_NORESTART;
> +
> +	hrtimer_forward(timer, now, NSEC_PER_SEC / 2);
> +	return HRTIMER_RESTART;

+ Blank line.

> +err:
> +	dev_err(tio->dev, "Event missed, Disabling Timed I/O");
> +	pps_tio_disable(tio);
> +	return HRTIMER_NORESTART;
> +}

...

Note, the above are nit-picks and may be applied if you ever need a v13 to be
sent. For now let's wait for the more serious comments against the series.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v12 2/3] Documentation: driver-api: pps: Add Intel Timed I/O PPS generator

[subramanian.mohan]

From: Subramanian Mohan <subramanian.mohan@intel.com>

Add Intel Timed I/O PPS usage instructions.

Co-developed-by: Pandith N <pandith.n@intel.com>
Signed-off-by: Pandith N <pandith.n@intel.com>
Signed-off-by: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Rodolfo Giometti <giometti@enneenne.com>
Signed-off-by: Subramanian Mohan <subramanian.mohan@intel.com>
---
 Documentation/driver-api/pps.rst | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/Documentation/driver-api/pps.rst b/Documentation/driver-api/pps.rst
index 78dded03e5d8..75f7b094f963 100644
--- a/Documentation/driver-api/pps.rst
+++ b/Documentation/driver-api/pps.rst
@@ -246,3 +246,27 @@ delay between assert and clear edge as small as possible to reduce system
 latencies. But if it is too small slave won't be able to capture clear edge
 transition. The default of 30us should be good enough in most situations.
 The delay can be selected using 'delay' pps_gen_parport module parameter.
+
+
+Intel Timed I/O PPS signal generator
+------------------------------------
+
+Intel Timed I/O is a high precision device, present on 2019 and newer Intel
+CPUs, that can generate PPS signals.
+
+Timed I/O and system time are both driven by same hardware clock. The signal
+is generated with a precision of ~20 nanoseconds. The generated PPS signal
+is used to synchronize an external device with system clock. For example,
+it can be used to share your clock with a device that receives PPS signal,
+generated by Timed I/O device. There are dedicated Timed I/O pins to deliver
+the PPS signal to an external device.
+
+Usage of Intel Timed I/O as PPS generator:
+
+Start generating PPS signal::
+
+        $echo 1 > /sys/devices/platform/INTCxxxx\:00/enable
+
+Stop generating PPS signal::
+
+        $echo 0 > /sys/devices/platform/INTCxxxx\:00/enable
-- 
2.35.3

Re: [PATCH v12 2/3] Documentation: driver-api: pps: Add Intel Timed I/O PPS generator

[Greg KH]

On Fri, Aug 23, 2024 at 12:31:07PM +0530, subramanian.mohan@intel.com wrote:
> From: Subramanian Mohan <subramanian.mohan@intel.com>
> 
> Add Intel Timed I/O PPS usage instructions.
> 
> Co-developed-by: Pandith N <pandith.n@intel.com>
> Signed-off-by: Pandith N <pandith.n@intel.com>
> Signed-off-by: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Acked-by: Rodolfo Giometti <giometti@enneenne.com>
> Signed-off-by: Subramanian Mohan <subramanian.mohan@intel.com>
> ---
>  Documentation/driver-api/pps.rst | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/Documentation/driver-api/pps.rst b/Documentation/driver-api/pps.rst
> index 78dded03e5d8..75f7b094f963 100644
> --- a/Documentation/driver-api/pps.rst
> +++ b/Documentation/driver-api/pps.rst
> @@ -246,3 +246,27 @@ delay between assert and clear edge as small as possible to reduce system
>  latencies. But if it is too small slave won't be able to capture clear edge
>  transition. The default of 30us should be good enough in most situations.
>  The delay can be selected using 'delay' pps_gen_parport module parameter.
> +
> +
> +Intel Timed I/O PPS signal generator
> +------------------------------------
> +
> +Intel Timed I/O is a high precision device, present on 2019 and newer Intel
> +CPUs, that can generate PPS signals.
> +
> +Timed I/O and system time are both driven by same hardware clock. The signal
> +is generated with a precision of ~20 nanoseconds. The generated PPS signal
> +is used to synchronize an external device with system clock. For example,
> +it can be used to share your clock with a device that receives PPS signal,
> +generated by Timed I/O device. There are dedicated Timed I/O pins to deliver
> +the PPS signal to an external device.
> +
> +Usage of Intel Timed I/O as PPS generator:
> +
> +Start generating PPS signal::
> +
> +        $echo 1 > /sys/devices/platform/INTCxxxx\:00/enable
> +
> +Stop generating PPS signal::
> +
> +        $echo 0 > /sys/devices/platform/INTCxxxx\:00/enable

As I mentioned on the sysfs documentation, why isn't this just a generic
pps class attribute instead?  Why did you make it
only-this-one-special-driver type of thing?

thanks,

greg k-h

Re: [PATCH v12 2/3] Documentation: driver-api: pps: Add Intel Timed I/O PPS generator

[Rodolfo Giometti]

On 24/08/24 04:21, Greg KH wrote:
> On Fri, Aug 23, 2024 at 12:31:07PM +0530, subramanian.mohan@intel.com wrote:
>> From: Subramanian Mohan <subramanian.mohan@intel.com>
>>
>> Add Intel Timed I/O PPS usage instructions.
>>
>> Co-developed-by: Pandith N <pandith.n@intel.com>
>> Signed-off-by: Pandith N <pandith.n@intel.com>
>> Signed-off-by: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Acked-by: Rodolfo Giometti <giometti@enneenne.com>
>> Signed-off-by: Subramanian Mohan <subramanian.mohan@intel.com>
>> ---
>>   Documentation/driver-api/pps.rst | 24 ++++++++++++++++++++++++
>>   1 file changed, 24 insertions(+)
>>
>> diff --git a/Documentation/driver-api/pps.rst b/Documentation/driver-api/pps.rst
>> index 78dded03e5d8..75f7b094f963 100644
>> --- a/Documentation/driver-api/pps.rst
>> +++ b/Documentation/driver-api/pps.rst
>> @@ -246,3 +246,27 @@ delay between assert and clear edge as small as possible to reduce system
>>   latencies. But if it is too small slave won't be able to capture clear edge
>>   transition. The default of 30us should be good enough in most situations.
>>   The delay can be selected using 'delay' pps_gen_parport module parameter.
>> +
>> +
>> +Intel Timed I/O PPS signal generator
>> +------------------------------------
>> +
>> +Intel Timed I/O is a high precision device, present on 2019 and newer Intel
>> +CPUs, that can generate PPS signals.
>> +
>> +Timed I/O and system time are both driven by same hardware clock. The signal
>> +is generated with a precision of ~20 nanoseconds. The generated PPS signal
>> +is used to synchronize an external device with system clock. For example,
>> +it can be used to share your clock with a device that receives PPS signal,
>> +generated by Timed I/O device. There are dedicated Timed I/O pins to deliver
>> +the PPS signal to an external device.
>> +
>> +Usage of Intel Timed I/O as PPS generator:
>> +
>> +Start generating PPS signal::
>> +
>> +        $echo 1 > /sys/devices/platform/INTCxxxx\:00/enable
>> +
>> +Stop generating PPS signal::
>> +
>> +        $echo 0 > /sys/devices/platform/INTCxxxx\:00/enable
> 
> As I mentioned on the sysfs documentation, why isn't this just a generic
> pps class attribute instead?  Why did you make it
> only-this-one-special-driver type of thing?

This is an Original Sin when PPS generators were introduced. :-(

In 2011 a patch from Alexander Gordeev <lasaine@lvk.cs.msu.su> (which introduced 
the "parallel port PPS signal generator") was committed in the main kernel.

At the time it was something exotic and doing a PPS generator interface for it 
was not considered (since it actually has no controlling inputs), but now 
several Ethernet cards have such PPS generator functionalities, and they are 
enabled in a per-driver way or via the PTP API.

This code is a pure PPS generator and it cannot use any other way to enable such 
functionality than the one above since the PPS layer misses a proper 
implementation for PPS generator.

If you are willing to stop the inclusion due this fact maybe its time to add 
such PPS generators interface... on the other hand, if you agree for inclusion 
we can do this job as soon as the code has been included, in order to fix this 
anomalous status.

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming

Re: [PATCH v12 2/3] Documentation: driver-api: pps: Add Intel Timed I/O PPS generator

[Greg KH]

On Tue, Aug 27, 2024 at 03:09:15PM +0200, Rodolfo Giometti wrote:
> On 24/08/24 04:21, Greg KH wrote:
> > On Fri, Aug 23, 2024 at 12:31:07PM +0530, subramanian.mohan@intel.com wrote:
> > > From: Subramanian Mohan <subramanian.mohan@intel.com>
> > > 
> > > Add Intel Timed I/O PPS usage instructions.
> > > 
> > > Co-developed-by: Pandith N <pandith.n@intel.com>
> > > Signed-off-by: Pandith N <pandith.n@intel.com>
> > > Signed-off-by: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>
> > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Acked-by: Rodolfo Giometti <giometti@enneenne.com>
> > > Signed-off-by: Subramanian Mohan <subramanian.mohan@intel.com>
> > > ---
> > >   Documentation/driver-api/pps.rst | 24 ++++++++++++++++++++++++
> > >   1 file changed, 24 insertions(+)
> > > 
> > > diff --git a/Documentation/driver-api/pps.rst b/Documentation/driver-api/pps.rst
> > > index 78dded03e5d8..75f7b094f963 100644
> > > --- a/Documentation/driver-api/pps.rst
> > > +++ b/Documentation/driver-api/pps.rst
> > > @@ -246,3 +246,27 @@ delay between assert and clear edge as small as possible to reduce system
> > >   latencies. But if it is too small slave won't be able to capture clear edge
> > >   transition. The default of 30us should be good enough in most situations.
> > >   The delay can be selected using 'delay' pps_gen_parport module parameter.
> > > +
> > > +
> > > +Intel Timed I/O PPS signal generator
> > > +------------------------------------
> > > +
> > > +Intel Timed I/O is a high precision device, present on 2019 and newer Intel
> > > +CPUs, that can generate PPS signals.
> > > +
> > > +Timed I/O and system time are both driven by same hardware clock. The signal
> > > +is generated with a precision of ~20 nanoseconds. The generated PPS signal
> > > +is used to synchronize an external device with system clock. For example,
> > > +it can be used to share your clock with a device that receives PPS signal,
> > > +generated by Timed I/O device. There are dedicated Timed I/O pins to deliver
> > > +the PPS signal to an external device.
> > > +
> > > +Usage of Intel Timed I/O as PPS generator:
> > > +
> > > +Start generating PPS signal::
> > > +
> > > +        $echo 1 > /sys/devices/platform/INTCxxxx\:00/enable
> > > +
> > > +Stop generating PPS signal::
> > > +
> > > +        $echo 0 > /sys/devices/platform/INTCxxxx\:00/enable
> > 
> > As I mentioned on the sysfs documentation, why isn't this just a generic
> > pps class attribute instead?  Why did you make it
> > only-this-one-special-driver type of thing?
> 
> This is an Original Sin when PPS generators were introduced. :-(
> 
> In 2011 a patch from Alexander Gordeev <lasaine@lvk.cs.msu.su> (which
> introduced the "parallel port PPS signal generator") was committed in the
> main kernel.
> 
> At the time it was something exotic and doing a PPS generator interface for
> it was not considered (since it actually has no controlling inputs), but now
> several Ethernet cards have such PPS generator functionalities, and they are
> enabled in a per-driver way or via the PTP API.
> 
> This code is a pure PPS generator and it cannot use any other way to enable
> such functionality than the one above since the PPS layer misses a proper
> implementation for PPS generator.
> 
> If you are willing to stop the inclusion due this fact maybe its time to add
> such PPS generators interface... on the other hand, if you agree for
> inclusion we can do this job as soon as the code has been included, in order
> to fix this anomalous status.

Please make a generic pps subsystem for this, it would make it simpler
for everyone.

thanks,

greg k-h

RE: [PATCH v12 2/3] Documentation: driver-api: pps: Add Intel Timed I/O PPS generator

[Hall, Christopher S]

Hi Rodolfo,

> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Tuesday, September 03, 2024 3:25 AM
> To: Rodolfo Giometti <giometti@enneenne.com>

> Subject: Re: [PATCH v12 2/3] Documentation: driver-api: pps: Add Intel Timed
> I/O PPS generator
> 

> > If you are willing to stop the inclusion due this fact maybe its time to add
> > such PPS generators interface... on the other hand, if you agree for
> > inclusion we can do this job as soon as the code has been included, in order
> > to fix this anomalous status.
> 
> Please make a generic pps subsystem for this, it would make it simpler
> for everyone.

What is required to move this? We can certainly test the code and do some
of this work, but I would look to you to define the interface.

Thanks,
Chris

Re: [PATCH v12 2/3] Documentation: driver-api: pps: Add Intel Timed I/O PPS generator

[Rodolfo Giometti]

On 25/09/24 23:55, Hall, Christopher S wrote:
> Hi Rodolfo,

Hello.

>> From: Greg KH <gregkh@linuxfoundation.org>
>> Sent: Tuesday, September 03, 2024 3:25 AM
>> To: Rodolfo Giometti <giometti@enneenne.com>
> 
>> Subject: Re: [PATCH v12 2/3] Documentation: driver-api: pps: Add Intel Timed
>> I/O PPS generator
>>
> 
>>> If you are willing to stop the inclusion due this fact maybe its time to add
>>> such PPS generators interface... on the other hand, if you agree for
>>> inclusion we can do this job as soon as the code has been included, in order
>>> to fix this anomalous status.
>>
>> Please make a generic pps subsystem for this, it would make it simpler
>> for everyone.
> 
> What is required to move this? We can certainly test the code and do some
> of this work, but I would look to you to define the interface.

The problem is that a pps-generator has no related device, then no sysfs entries.

I think the right-thing(TM) to do is adding a new class named "pps-generator", 
so we will get the directory /sys/class/pps-generator with several devices as 
pps-generator0, pps-generator1, etc. For each device we should add at least 
these sysfs entries:

- system       : This file return "1" if the generator takes the timing from
                  the system clock, while it returns "0" if not (i.e. from a
                  peripheral device clock).

- time         : This file contains the current time stored into the generator
                  clock as two integers representing the current time seconds and
                  nanoseconds.

- enable       : This write-only file enables or disables generation of the
                  PPS signal.

- period       : This file defines the period for the generator signal. When
                  read, by default, it returns "1 0" which represent the period
                  second and nanoseconds (i.e. a PPS signal). When written, it
                  sets the period accordingly or ENOTSUPP if not supported.

- start_time   : This file defines the starting time for the the generator
                  signal. When read, by default, it returns "0 0" which means
                  "now". When written, it sets the starting time accordingly or
                  ENOTSUPP if not supported.

Please, let me know what do you think about this proposal. I'm going to provide 
a draft in the next weeks since I'm stuck on another project right now. :(

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming

Re: [PATCH v12 2/3] Documentation: driver-api: pps: Add Intel Timed I/O PPS generator

[Greg KH]

On Thu, Sep 26, 2024 at 10:46:54AM +0200, Rodolfo Giometti wrote:
> On 25/09/24 23:55, Hall, Christopher S wrote:
> > Hi Rodolfo,
> 
> Hello.
> 
> > > From: Greg KH <gregkh@linuxfoundation.org>
> > > Sent: Tuesday, September 03, 2024 3:25 AM
> > > To: Rodolfo Giometti <giometti@enneenne.com>
> > 
> > > Subject: Re: [PATCH v12 2/3] Documentation: driver-api: pps: Add Intel Timed
> > > I/O PPS generator
> > > 
> > 
> > > > If you are willing to stop the inclusion due this fact maybe its time to add
> > > > such PPS generators interface... on the other hand, if you agree for
> > > > inclusion we can do this job as soon as the code has been included, in order
> > > > to fix this anomalous status.
> > > 
> > > Please make a generic pps subsystem for this, it would make it simpler
> > > for everyone.
> > 
> > What is required to move this? We can certainly test the code and do some
> > of this work, but I would look to you to define the interface.
> 
> The problem is that a pps-generator has no related device, then no sysfs entries.
> 
> I think the right-thing(TM) to do is adding a new class named
> "pps-generator", so we will get the directory /sys/class/pps-generator with
> several devices as pps-generator0, pps-generator1, etc. For each device we
> should add at least these sysfs entries:
> 
> - system       : This file return "1" if the generator takes the timing from
>                  the system clock, while it returns "0" if not (i.e. from a
>                  peripheral device clock).
> 
> - time         : This file contains the current time stored into the generator
>                  clock as two integers representing the current time seconds and
>                  nanoseconds.
> 
> - enable       : This write-only file enables or disables generation of the
>                  PPS signal.
> 
> - period       : This file defines the period for the generator signal. When
>                  read, by default, it returns "1 0" which represent the period
>                  second and nanoseconds (i.e. a PPS signal). When written, it
>                  sets the period accordingly or ENOTSUPP if not supported.
> 
> - start_time   : This file defines the starting time for the the generator
>                  signal. When read, by default, it returns "0 0" which means
>                  "now". When written, it sets the starting time accordingly or
>                  ENOTSUPP if not supported.

This seems sane to me, thanks for writing it up!

greg k-h

[PATCH v12 3/3] ABI: pps: Add ABI documentation for Intel TIO

[subramanian.mohan]

From: Subramanian Mohan <subramanian.mohan@intel.com>

Document sysfs interface for Intel Timed I/O PPS driver.

Signed-off-by: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>
Acked-by: Christopher Hall <christopher.s.hall@intel.com>
Signed-off-by: Subramanian Mohan <subramanian.mohan@intel.com>
---
 Documentation/ABI/testing/sysfs-platform-pps-tio | 8 ++++++++
 MAINTAINERS                                      | 1 +
 2 files changed, 9 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-pps-tio

diff --git a/Documentation/ABI/testing/sysfs-platform-pps-tio b/Documentation/ABI/testing/sysfs-platform-pps-tio
new file mode 100644
index 000000000000..8face1dc8010
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-pps-tio
@@ -0,0 +1,8 @@
+What:		/sys/devices/platform/INTCxxxx/enable
+Date:		September 2024
+KernelVersion:	6.12
+Contact:	Christopher Hall <christopher.s.hall@intel.com>
+		Subramanian Mohan<subramanian.mohan@intel.com>
+Description:
+		(RW) Enable or disable PPS TIO generator output, read to
+		see the status of hardware (Enabled/Disabled).
diff --git a/MAINTAINERS b/MAINTAINERS
index f328373463b0..242d9784ee5d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18263,6 +18263,7 @@ M:	Rodolfo Giometti <giometti@enneenne.com>
 L:	linuxpps@ml.enneenne.com (subscribers-only)
 S:	Maintained
 W:	http://wiki.enneenne.com/index.php/LinuxPPS_support
+F:	Documentation/ABI/testing/sysfs-platform-pps-tio
 F:	Documentation/ABI/testing/sysfs-pps
 F:	Documentation/devicetree/bindings/pps/pps-gpio.yaml
 F:	Documentation/driver-api/pps.rst
-- 
2.35.3

Re: [PATCH v12 3/3] ABI: pps: Add ABI documentation for Intel TIO

[Andy Shevchenko]

On Fri, Aug 23, 2024 at 12:31:08PM +0530, subramanian.mohan@intel.com wrote:
> From: Subramanian Mohan <subramanian.mohan@intel.com>
> 
> Document sysfs interface for Intel Timed I/O PPS driver.

...

> +Contact:	Christopher Hall <christopher.s.hall@intel.com>
> +		Subramanian Mohan<subramanian.mohan@intel.com>

Missing space. This seems like a warrant for v13, but again, let's wait a bit
for others to comment.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v12 3/3] ABI: pps: Add ABI documentation for Intel TIO

[Greg KH]

On Fri, Aug 23, 2024 at 12:31:08PM +0530, subramanian.mohan@intel.com wrote:
> From: Subramanian Mohan <subramanian.mohan@intel.com>
> 
> Document sysfs interface for Intel Timed I/O PPS driver.
> 
> Signed-off-by: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>
> Acked-by: Christopher Hall <christopher.s.hall@intel.com>
> Signed-off-by: Subramanian Mohan <subramanian.mohan@intel.com>
> ---
>  Documentation/ABI/testing/sysfs-platform-pps-tio | 8 ++++++++
>  MAINTAINERS                                      | 1 +
>  2 files changed, 9 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-platform-pps-tio
> 
> diff --git a/Documentation/ABI/testing/sysfs-platform-pps-tio b/Documentation/ABI/testing/sysfs-platform-pps-tio
> new file mode 100644
> index 000000000000..8face1dc8010
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-platform-pps-tio
> @@ -0,0 +1,8 @@
> +What:		/sys/devices/platform/INTCxxxx/enable
> +Date:		September 2024
> +KernelVersion:	6.12
> +Contact:	Christopher Hall <christopher.s.hall@intel.com>

Why are you adding a random person here who did not sign off on this
commit as being responsible for it?

> +		Subramanian Mohan<subramanian.mohan@intel.com>

Do you really need 2 people for a sysfs attribute?

> +Description:
> +		(RW) Enable or disable PPS TIO generator output, read to
> +		see the status of hardware (Enabled/Disabled).
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f328373463b0..242d9784ee5d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18263,6 +18263,7 @@ M:	Rodolfo Giometti <giometti@enneenne.com>
>  L:	linuxpps@ml.enneenne.com (subscribers-only)
>  S:	Maintained
>  W:	http://wiki.enneenne.com/index.php/LinuxPPS_support
> +F:	Documentation/ABI/testing/sysfs-platform-pps-tio

Why are you saying that Rodolfo is now responsible for this file?

>  F:	Documentation/ABI/testing/sysfs-pps

Wait, there already is default pps sysfs attributes?  Why aren't you
adding your attribute to them instead?  Doing a "driver-specific"
attribute for a generic class device is not a good idea as userspace
tools will never know to look for it.

thanks,

greg k-h