[PATCH net-next 0/3] ptp: add pulse signal loopback support for debugging

[PATCH net-next 0/3] ptp: add pulse signal loopback support for debugging

[Wei Fang]

Some PTP devices support looping back the periodic pulse signal for
debugging, so add the generic debugfs interfaces to the ptp_clock
driver. The first two patch are separated from the previous patch
set [1]. The third patch is new added.

[1]: https://lore.kernel.org/imx/20250827063332.1217664-1-wei.fang@nxp.com/ #patch 3 and 9

Wei Fang (3):
  ptp: add debugfs interfaces to loop back the periodic output signal
  ptp: netc: add the periodic output signal loopback support
  ptp: qoriq: covert to use generic interfaces to set loopback mode

 MAINTAINERS                      |   1 -
 drivers/ptp/Kconfig              |   2 +-
 drivers/ptp/Makefile             |   4 +-
 drivers/ptp/ptp_clock.c          |  66 ++++++++++++++++++++
 drivers/ptp/ptp_netc.c           |  25 ++++++++
 drivers/ptp/ptp_qoriq.c          |  24 +++++++-
 drivers/ptp/ptp_qoriq_debugfs.c  | 101 -------------------------------
 include/linux/fsl/ptp_qoriq.h    |  10 ---
 include/linux/ptp_clock_kernel.h |  10 +++
 9 files changed, 125 insertions(+), 118 deletions(-)
 delete mode 100644 drivers/ptp/ptp_qoriq_debugfs.c

-- 
2.34.1

[PATCH net-next 1/3] ptp: add debugfs interfaces to loop back the periodic output signal

[Wei Fang]

For some PTP devices, they have the capability to loop back the periodic
output signal for debugging, such as the ptp_qoriq device. So add the
generic interfaces to set the periodic output signal loopback, rather
than each vendor having a different implementation.

Show how many channels support the periodic output signal loopback:
$ cat /sys/kernel/debug/ptp<N>/n_perout_loopback

Enable the loopback of the periodic output signal of channel X:
$ echo <X> 1 > /sys/kernel/debug/ptp<N>/perout_loopback

Disable the loopback of the periodic output signal of channel X:
$ echo <X> 0 > /sys/kernel/debug/ptp<N>/perout_loopback

Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 drivers/ptp/ptp_clock.c          | 66 ++++++++++++++++++++++++++++++++
 include/linux/ptp_clock_kernel.h | 10 +++++
 2 files changed, 76 insertions(+)

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index 5739a57958c7..e4a5658e82b3 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -248,6 +248,66 @@ static void ptp_aux_kworker(struct kthread_work *work)
 		kthread_queue_delayed_work(ptp->kworker, &ptp->aux_work, delay);
 }
 
+static ssize_t ptp_n_perout_loopback_read(struct file *filep,
+					  char __user *buffer,
+					  size_t count, loff_t *pos)
+{
+	struct ptp_clock *ptp = filep->private_data;
+	char buf[12] = {};
+
+	snprintf(buf, sizeof(buf), "%d\n", ptp->info->n_per_lp);
+
+	return simple_read_from_buffer(buffer, count, pos, buf, strlen(buf));
+}
+
+static const struct file_operations ptp_n_perout_loopback_fops = {
+	.owner	= THIS_MODULE,
+	.open	= simple_open,
+	.read	= ptp_n_perout_loopback_read,
+};
+
+static ssize_t ptp_perout_loopback_write(struct file *filep,
+					 const char __user *buffer,
+					 size_t count, loff_t *ppos)
+{
+	struct ptp_clock *ptp = filep->private_data;
+	struct ptp_clock_info *ops = ptp->info;
+	int len, cnt, enable, err;
+	unsigned int index;
+	char buf[32] = {};
+
+	if (*ppos || !count)
+		return -EINVAL;
+
+	if (count >= sizeof(buf))
+		return -ENOSPC;
+
+	len = simple_write_to_buffer(buf, sizeof(buf) - 1,
+				     ppos, buffer, count);
+	if (len < 0)
+		return len;
+
+	buf[len] = '\0';
+	cnt = sscanf(buf, "%u %d", &index, &enable);
+	if (cnt != 2)
+		return -EINVAL;
+
+	if (index >= ops->n_per_lp)
+		return -EINVAL;
+
+	err = ops->perout_loopback(ops, index, enable ? 1 : 0);
+	if (err)
+		return err;
+
+	return count;
+}
+
+static const struct file_operations ptp_perout_loopback_ops = {
+	.owner   = THIS_MODULE,
+	.open    = simple_open,
+	.write	 = ptp_perout_loopback_write,
+};
+
 /* public interface */
 
 struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
@@ -389,6 +449,12 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 	/* Debugfs initialization */
 	snprintf(debugfsname, sizeof(debugfsname), "ptp%d", ptp->index);
 	ptp->debugfs_root = debugfs_create_dir(debugfsname, NULL);
+	if (info->n_per_lp > 0 && info->perout_loopback) {
+		debugfs_create_file("n_perout_loopback", 0400, ptp->debugfs_root,
+				    ptp, &ptp_n_perout_loopback_fops);
+		debugfs_create_file("perout_loopback", 0200, ptp->debugfs_root,
+				    ptp, &ptp_perout_loopback_ops);
+	}
 
 	return ptp;
 
diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
index 7dd7951b23d5..884364596dd3 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -67,6 +67,8 @@ struct ptp_system_timestamp {
  * @n_ext_ts:  The number of external time stamp channels.
  * @n_per_out: The number of programmable periodic signals.
  * @n_pins:    The number of programmable pins.
+ * @n_per_lp:  The number of channels that support loopback the periodic
+ *             output signal.
  * @pps:       Indicates whether the clock supports a PPS callback.
  *
  * @supported_perout_flags:  The set of flags the driver supports for the
@@ -175,6 +177,11 @@ struct ptp_system_timestamp {
  *                scheduling time (>=0) or negative value in case further
  *                scheduling is not required.
  *
+ * @perout_loopback: Request driver to enable or disable the periodic output
+ *                   signal loopback.
+ *                   parameter index: index of the periodic output signal channel.
+ *                   parameter on: caller passes one to enable or zero to disable.
+ *
  * Drivers should embed their ptp_clock_info within a private
  * structure, obtaining a reference to it using container_of().
  *
@@ -189,6 +196,7 @@ struct ptp_clock_info {
 	int n_ext_ts;
 	int n_per_out;
 	int n_pins;
+	int n_per_lp;
 	int pps;
 	unsigned int supported_perout_flags;
 	unsigned int supported_extts_flags;
@@ -213,6 +221,8 @@ struct ptp_clock_info {
 	int (*verify)(struct ptp_clock_info *ptp, unsigned int pin,
 		      enum ptp_pin_function func, unsigned int chan);
 	long (*do_aux_work)(struct ptp_clock_info *ptp);
+	int (*perout_loopback)(struct ptp_clock_info *ptp, unsigned int index,
+			       int on);
 };
 
 struct ptp_clock;
-- 
2.34.1

[PATCH net-next 2/3] ptp: netc: add the periodic output signal loopback support

[Wei Fang]

The NETC Timer supports looping back the output pulse signal of Fiper-n
into Trigger-n input, so that users can leverage this feature to validate
some other features without external hardware support. For example, users
can use it to test external trigger stamp (EXTTS). And users can combine
EXTTS with loopback mode to check whether the generation time of PPS is
aligned with an integral second of PHC, or the periodic output signal
(PTP_CLK_REQ_PEROUT) whether is generated at the specified time.

Since ptp_clock_info::perout_loopback() has been added to the ptp_clock
driver as a generic interface to enable or disable the periodic output
signal loopback, therefore, netc_timer_perout_loopback() is added as a
callback of ptp_clock_info::perout_loopback().

Test the generation time of PPS event:

$ echo 0 1 > /sys/kernel/debug/ptp0/perout_loopback
$ echo 1 > /sys/class/ptp/ptp0/pps_enable
$ testptp -d /dev/ptp0 -e 3
external time stamp request okay
event index 0 at 63.000000017
event index 0 at 64.000000017
event index 0 at 65.000000017

Test the generation time of the periodic output signal:

$ echo 0 1 > /sys/kernel/debug/ptp0/perout_loopback
$ echo 0 150 0 1 500000000 > /sys/class/ptp/ptp0/period
$ testptp -d /dev/ptp0 -e 3
external time stamp request okay
event index 0 at 150.000000014
event index 0 at 151.500000015
event index 0 at 153.000000014

Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 drivers/ptp/ptp_netc.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/ptp/ptp_netc.c b/drivers/ptp/ptp_netc.c
index 8c5fea1f43fa..75594f47807d 100644
--- a/drivers/ptp/ptp_netc.c
+++ b/drivers/ptp/ptp_netc.c
@@ -21,6 +21,7 @@
 #define  TMR_ETEP(i)			BIT(8 + (i))
 #define  TMR_COMP_MODE			BIT(15)
 #define  TMR_CTRL_TCLK_PERIOD		GENMASK(25, 16)
+#define  TMR_CTRL_PPL(i)		BIT(27 - (i))
 #define  TMR_CTRL_FS			BIT(28)
 
 #define NETC_TMR_TEVENT			0x0084
@@ -609,6 +610,28 @@ static int netc_timer_enable(struct ptp_clock_info *ptp,
 	}
 }
 
+static int netc_timer_perout_loopback(struct ptp_clock_info *ptp,
+				      unsigned int index, int on)
+{
+	struct netc_timer *priv = ptp_to_netc_timer(ptp);
+	unsigned long flags;
+	u32 tmr_ctrl;
+
+	spin_lock_irqsave(&priv->lock, flags);
+
+	tmr_ctrl = netc_timer_rd(priv, NETC_TMR_CTRL);
+	if (on)
+		tmr_ctrl |= TMR_CTRL_PPL(index);
+	else
+		tmr_ctrl &= ~TMR_CTRL_PPL(index);
+
+	netc_timer_wr(priv, NETC_TMR_CTRL, tmr_ctrl);
+
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	return 0;
+}
+
 static void netc_timer_adjust_period(struct netc_timer *priv, u64 period)
 {
 	u32 fractional_period = lower_32_bits(period);
@@ -717,6 +740,7 @@ static const struct ptp_clock_info netc_timer_ptp_caps = {
 	.pps		= 1,
 	.n_per_out	= 3,
 	.n_ext_ts	= 2,
+	.n_per_lp	= 2,
 	.supported_extts_flags = PTP_RISING_EDGE | PTP_FALLING_EDGE |
 				 PTP_STRICT_FLAGS,
 	.adjfine	= netc_timer_adjfine,
@@ -724,6 +748,7 @@ static const struct ptp_clock_info netc_timer_ptp_caps = {
 	.gettimex64	= netc_timer_gettimex64,
 	.settime64	= netc_timer_settime64,
 	.enable		= netc_timer_enable,
+	.perout_loopback = netc_timer_perout_loopback,
 };
 
 static void netc_timer_init(struct netc_timer *priv)
-- 
2.34.1

[PATCH net-next 3/3] ptp: qoriq: covert to use generic interfaces to set loopback mode

[Wei Fang]

Since the generic debugfs interfaces for setting the periodic pulse
signal loopback have been added to the ptp_clock driver, so covert the
vendor-defined debugfs interfaces to the generic interfaces.

Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 MAINTAINERS                     |   1 -
 drivers/ptp/Kconfig             |   2 +-
 drivers/ptp/Makefile            |   4 +-
 drivers/ptp/ptp_qoriq.c         |  24 +++++++-
 drivers/ptp/ptp_qoriq_debugfs.c | 101 --------------------------------
 include/linux/fsl/ptp_qoriq.h   |  10 ----
 6 files changed, 24 insertions(+), 118 deletions(-)
 delete mode 100644 drivers/ptp/ptp_qoriq_debugfs.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 6cad6225381a..4140fdd6ccf3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9816,7 +9816,6 @@ F:	drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp*
 F:	drivers/net/ethernet/freescale/dpaa2/dprtc*
 F:	drivers/net/ethernet/freescale/enetc/enetc_ptp.c
 F:	drivers/ptp/ptp_qoriq.c
-F:	drivers/ptp/ptp_qoriq_debugfs.c
 F:	include/linux/fsl/ptp_qoriq.h
 
 FREESCALE QUAD SPI DRIVER
diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
index 9256bf2e8ad4..5f8ea34d11d6 100644
--- a/drivers/ptp/Kconfig
+++ b/drivers/ptp/Kconfig
@@ -67,7 +67,7 @@ config PTP_1588_CLOCK_QORIQ
 	  packets using the SO_TIMESTAMPING API.
 
 	  To compile this driver as a module, choose M here: the module
-	  will be called ptp-qoriq.
+	  will be called ptp_qoriq.
 
 comment "Enable PHYLIB and NETWORK_PHY_TIMESTAMPING to see the additional clocks."
 	depends on PHYLIB=n || NETWORK_PHY_TIMESTAMPING=n
diff --git a/drivers/ptp/Makefile b/drivers/ptp/Makefile
index 8985d723d29c..bdc47e284f14 100644
--- a/drivers/ptp/Makefile
+++ b/drivers/ptp/Makefile
@@ -12,9 +12,7 @@ obj-$(CONFIG_PTP_1588_CLOCK_INES)	+= ptp_ines.o
 obj-$(CONFIG_PTP_1588_CLOCK_PCH)	+= ptp_pch.o
 obj-$(CONFIG_PTP_1588_CLOCK_KVM)	+= ptp_kvm.o
 obj-$(CONFIG_PTP_1588_CLOCK_VMCLOCK)	+= ptp_vmclock.o
-obj-$(CONFIG_PTP_1588_CLOCK_QORIQ)	+= ptp-qoriq.o
-ptp-qoriq-y				+= ptp_qoriq.o
-ptp-qoriq-$(CONFIG_DEBUG_FS)		+= ptp_qoriq_debugfs.o
+obj-$(CONFIG_PTP_1588_CLOCK_QORIQ)	+= ptp_qoriq.o
 obj-$(CONFIG_PTP_1588_CLOCK_IDTCM)	+= ptp_clockmatrix.o
 obj-$(CONFIG_PTP_1588_CLOCK_FC3W)	+= ptp_fc3.o
 obj-$(CONFIG_PTP_1588_CLOCK_IDT82P33)	+= ptp_idt82p33.o
diff --git a/drivers/ptp/ptp_qoriq.c b/drivers/ptp/ptp_qoriq.c
index 4d488c1f1941..8da995e36aeb 100644
--- a/drivers/ptp/ptp_qoriq.c
+++ b/drivers/ptp/ptp_qoriq.c
@@ -465,6 +465,25 @@ static int ptp_qoriq_auto_config(struct ptp_qoriq *ptp_qoriq,
 	return 0;
 }
 
+static int ptp_qoriq_perout_loopback(struct ptp_clock_info *ptp,
+				     unsigned int index, int on)
+{
+	struct ptp_qoriq *ptp_qoriq = container_of(ptp, struct ptp_qoriq, caps);
+	struct ptp_qoriq_registers *regs = &ptp_qoriq->regs;
+	u32 loopback_bit = index ? PP2L : PP1L;
+	u32 tmr_ctrl;
+
+	tmr_ctrl = ptp_qoriq->read(&regs->ctrl_regs->tmr_ctrl);
+	if (on)
+		tmr_ctrl |= loopback_bit;
+	else
+		tmr_ctrl &= ~loopback_bit;
+
+	ptp_qoriq->write(&regs->ctrl_regs->tmr_ctrl, tmr_ctrl);
+
+	return 0;
+}
+
 int ptp_qoriq_init(struct ptp_qoriq *ptp_qoriq, void __iomem *base,
 		   const struct ptp_clock_info *caps)
 {
@@ -479,6 +498,8 @@ int ptp_qoriq_init(struct ptp_qoriq *ptp_qoriq, void __iomem *base,
 
 	ptp_qoriq->base = base;
 	ptp_qoriq->caps = *caps;
+	ptp_qoriq->caps.n_per_lp = 2;
+	ptp_qoriq->caps.perout_loopback = ptp_qoriq_perout_loopback;
 
 	if (of_property_read_u32(node, "fsl,cksel", &ptp_qoriq->cksel))
 		ptp_qoriq->cksel = DEFAULT_CKSEL;
@@ -568,7 +589,7 @@ int ptp_qoriq_init(struct ptp_qoriq *ptp_qoriq, void __iomem *base,
 		return PTR_ERR(ptp_qoriq->clock);
 
 	ptp_qoriq->phc_index = ptp_clock_index(ptp_qoriq->clock);
-	ptp_qoriq_create_debugfs(ptp_qoriq);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(ptp_qoriq_init);
@@ -580,7 +601,6 @@ void ptp_qoriq_free(struct ptp_qoriq *ptp_qoriq)
 	ptp_qoriq->write(&regs->ctrl_regs->tmr_temask, 0);
 	ptp_qoriq->write(&regs->ctrl_regs->tmr_ctrl,   0);
 
-	ptp_qoriq_remove_debugfs(ptp_qoriq);
 	ptp_clock_unregister(ptp_qoriq->clock);
 	iounmap(ptp_qoriq->base);
 	free_irq(ptp_qoriq->irq, ptp_qoriq);
diff --git a/drivers/ptp/ptp_qoriq_debugfs.c b/drivers/ptp/ptp_qoriq_debugfs.c
deleted file mode 100644
index e8dddcedf288..000000000000
--- a/drivers/ptp/ptp_qoriq_debugfs.c
+++ /dev/null
@@ -1,101 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-/* Copyright 2019 NXP
- */
-#include <linux/device.h>
-#include <linux/debugfs.h>
-#include <linux/fsl/ptp_qoriq.h>
-
-static int ptp_qoriq_fiper1_lpbk_get(void *data, u64 *val)
-{
-	struct ptp_qoriq *ptp_qoriq = data;
-	struct ptp_qoriq_registers *regs = &ptp_qoriq->regs;
-	u32 ctrl;
-
-	ctrl = ptp_qoriq->read(&regs->ctrl_regs->tmr_ctrl);
-	*val = ctrl & PP1L ? 1 : 0;
-
-	return 0;
-}
-
-static int ptp_qoriq_fiper1_lpbk_set(void *data, u64 val)
-{
-	struct ptp_qoriq *ptp_qoriq = data;
-	struct ptp_qoriq_registers *regs = &ptp_qoriq->regs;
-	u32 ctrl;
-
-	ctrl = ptp_qoriq->read(&regs->ctrl_regs->tmr_ctrl);
-	if (val == 0)
-		ctrl &= ~PP1L;
-	else
-		ctrl |= PP1L;
-
-	ptp_qoriq->write(&regs->ctrl_regs->tmr_ctrl, ctrl);
-	return 0;
-}
-
-DEFINE_DEBUGFS_ATTRIBUTE(ptp_qoriq_fiper1_fops, ptp_qoriq_fiper1_lpbk_get,
-			 ptp_qoriq_fiper1_lpbk_set, "%llu\n");
-
-static int ptp_qoriq_fiper2_lpbk_get(void *data, u64 *val)
-{
-	struct ptp_qoriq *ptp_qoriq = data;
-	struct ptp_qoriq_registers *regs = &ptp_qoriq->regs;
-	u32 ctrl;
-
-	ctrl = ptp_qoriq->read(&regs->ctrl_regs->tmr_ctrl);
-	*val = ctrl & PP2L ? 1 : 0;
-
-	return 0;
-}
-
-static int ptp_qoriq_fiper2_lpbk_set(void *data, u64 val)
-{
-	struct ptp_qoriq *ptp_qoriq = data;
-	struct ptp_qoriq_registers *regs = &ptp_qoriq->regs;
-	u32 ctrl;
-
-	ctrl = ptp_qoriq->read(&regs->ctrl_regs->tmr_ctrl);
-	if (val == 0)
-		ctrl &= ~PP2L;
-	else
-		ctrl |= PP2L;
-
-	ptp_qoriq->write(&regs->ctrl_regs->tmr_ctrl, ctrl);
-	return 0;
-}
-
-DEFINE_DEBUGFS_ATTRIBUTE(ptp_qoriq_fiper2_fops, ptp_qoriq_fiper2_lpbk_get,
-			 ptp_qoriq_fiper2_lpbk_set, "%llu\n");
-
-void ptp_qoriq_create_debugfs(struct ptp_qoriq *ptp_qoriq)
-{
-	struct dentry *root;
-
-	root = debugfs_create_dir(dev_name(ptp_qoriq->dev), NULL);
-	if (IS_ERR(root))
-		return;
-	if (!root)
-		goto err_root;
-
-	ptp_qoriq->debugfs_root = root;
-
-	if (!debugfs_create_file_unsafe("fiper1-loopback", 0600, root,
-					ptp_qoriq, &ptp_qoriq_fiper1_fops))
-		goto err_node;
-	if (!debugfs_create_file_unsafe("fiper2-loopback", 0600, root,
-					ptp_qoriq, &ptp_qoriq_fiper2_fops))
-		goto err_node;
-	return;
-
-err_node:
-	debugfs_remove_recursive(root);
-	ptp_qoriq->debugfs_root = NULL;
-err_root:
-	dev_err(ptp_qoriq->dev, "failed to initialize debugfs\n");
-}
-
-void ptp_qoriq_remove_debugfs(struct ptp_qoriq *ptp_qoriq)
-{
-	debugfs_remove_recursive(ptp_qoriq->debugfs_root);
-	ptp_qoriq->debugfs_root = NULL;
-}
diff --git a/include/linux/fsl/ptp_qoriq.h b/include/linux/fsl/ptp_qoriq.h
index b301bf7199d3..3601e25779ba 100644
--- a/include/linux/fsl/ptp_qoriq.h
+++ b/include/linux/fsl/ptp_qoriq.h
@@ -145,7 +145,6 @@ struct ptp_qoriq {
 	struct ptp_clock *clock;
 	struct ptp_clock_info caps;
 	struct resource *rsrc;
-	struct dentry *debugfs_root;
 	struct device *dev;
 	bool extts_fifo_support;
 	bool fiper3_support;
@@ -195,14 +194,5 @@ int ptp_qoriq_settime(struct ptp_clock_info *ptp,
 int ptp_qoriq_enable(struct ptp_clock_info *ptp,
 		     struct ptp_clock_request *rq, int on);
 int extts_clean_up(struct ptp_qoriq *ptp_qoriq, int index, bool update_event);
-#ifdef CONFIG_DEBUG_FS
-void ptp_qoriq_create_debugfs(struct ptp_qoriq *ptp_qoriq);
-void ptp_qoriq_remove_debugfs(struct ptp_qoriq *ptp_qoriq);
-#else
-static inline void ptp_qoriq_create_debugfs(struct ptp_qoriq *ptp_qoriq)
-{ }
-static inline void ptp_qoriq_remove_debugfs(struct ptp_qoriq *ptp_qoriq)
-{ }
-#endif
 
 #endif
-- 
2.34.1

Re: [PATCH net-next 0/3] ptp: add pulse signal loopback support for debugging

[Richard Cochran]

On Wed, Sep 03, 2025 at 04:37:46PM +0800, Wei Fang wrote:
> Some PTP devices support looping back the periodic pulse signal for
> debugging,

What kinds of debugs can be resolved by this loopback feature?

It seems pointless to me...

Thanks,
Richard

Re: [PATCH net-next 0/3] ptp: add pulse signal loopback support for debugging

[Vladimir Oltean]

On Wed, Sep 03, 2025 at 06:41:30AM -0700, Richard Cochran wrote:
> On Wed, Sep 03, 2025 at 04:37:46PM +0800, Wei Fang wrote:
> > Some PTP devices support looping back the periodic pulse signal for
> > debugging,
> 
> What kinds of debugs can be resolved by this loopback feature?

The commit message of patch 2/3 shows that you can record extts events
for the periodic output signal emitted by the device, without this
signal ever being required to be routed in any particular way on the PCB,
through the SoC's pins.

So you can make sure that the pulse intervals and their phase alignment
are correct from the perspective of the emitting PHC's time base.

Or you can use it as a built-in extts event generator when you have no
external equipment which does that, as I did a few years ago to validate
the extts functionality on ptp_qoriq.

> It seems pointless to me...

Well, it's pointless in the same sense that a mirror is pointless.

RE: [PATCH net-next 0/3] ptp: add pulse signal loopback support for debugging

[Wei Fang]

> On Wed, Sep 03, 2025 at 04:37:46PM +0800, Wei Fang wrote:
> > Some PTP devices support looping back the periodic pulse signal for
> > debugging,
> 
> What kinds of debugs can be resolved by this loopback feature?
> 
> It seems pointless to me...


Vladimir helped explain its purpose in the thread, do you still think
it is pointless? If so, then I can only add a custom debugfs interface
to the ptp_netc driver, at least this will be a better debugging feature
for the NETC Timer. :)

Re: [PATCH net-next 0/3] ptp: add pulse signal loopback support for debugging

[Richard Cochran]

On Thu, Sep 04, 2025 at 01:55:43AM +0000, Wei Fang wrote:
> Vladimir helped explain its purpose in the thread, do you still think
> it is pointless?

Vladimir gave practical examples for the use case, so no objection
from my side.  I just wanted to understand how this is useful.

Next time, it would be helpful to have that info in the cover letter.

Thanks,
Richard

Re: [PATCH net-next 1/3] ptp: add debugfs interfaces to loop back the periodic output signal

[Vladimir Oltean]

On Wed, Sep 03, 2025 at 04:37:47PM +0800, Wei Fang wrote:
> +	buf[len] = '\0';
> +	cnt = sscanf(buf, "%u %d", &index, &enable);
> +	if (cnt != 2)
> +		return -EINVAL;
> +
> +	if (index >= ops->n_per_lp)
> +		return -EINVAL;
> +
> +	err = ops->perout_loopback(ops, index, enable ? 1 : 0);

Why not just reject other 'enable' values than 1 or 0? You make it
impossible for other values like '2' to be used in the future, if they
are currently treated the same as '1'.

Also, signed 'enable' doesn't make much sense.

> +	if (err)
> +		return err;
> +
> +	return count;
> +}

Re: [PATCH net-next 2/3] ptp: netc: add the periodic output signal loopback support

[Vladimir Oltean]

On Wed, Sep 03, 2025 at 04:37:48PM +0800, Wei Fang wrote:
> The NETC Timer supports looping back the output pulse signal of Fiper-n
> into Trigger-n input, so that users can leverage this feature to validate
> some other features without external hardware support. For example, users
> can use it to test external trigger stamp (EXTTS). And users can combine
> EXTTS with loopback mode to check whether the generation time of PPS is
> aligned with an integral second of PHC, or the periodic output signal
> (PTP_CLK_REQ_PEROUT) whether is generated at the specified time.
> 
> Since ptp_clock_info::perout_loopback() has been added to the ptp_clock
> driver as a generic interface to enable or disable the periodic output
> signal loopback, therefore, netc_timer_perout_loopback() is added as a
> callback of ptp_clock_info::perout_loopback().
> 
> Test the generation time of PPS event:
> 
> $ echo 0 1 > /sys/kernel/debug/ptp0/perout_loopback
> $ echo 1 > /sys/class/ptp/ptp0/pps_enable
> $ testptp -d /dev/ptp0 -e 3
> external time stamp request okay
> event index 0 at 63.000000017
> event index 0 at 64.000000017
> event index 0 at 65.000000017
> 
> Test the generation time of the periodic output signal:
> 
> $ echo 0 1 > /sys/kernel/debug/ptp0/perout_loopback
> $ echo 0 150 0 1 500000000 > /sys/class/ptp/ptp0/period
> $ testptp -d /dev/ptp0 -e 3
> external time stamp request okay
> event index 0 at 150.000000014
> event index 0 at 151.500000015
> event index 0 at 153.000000014
> 
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Re: [PATCH net-next 3/3] ptp: qoriq: covert to use generic interfaces to set loopback mode

[Vladimir Oltean]

On Wed, Sep 03, 2025 at 04:37:49PM +0800, Wei Fang wrote:
> Since the generic debugfs interfaces for setting the periodic pulse
> signal loopback have been added to the ptp_clock driver, so covert the

s/covert/convert/

Also, 'since' doesn't go along with 'so'. You can remove either word.

> vendor-defined debugfs interfaces to the generic interfaces.
> 
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Re: [PATCH net-next 0/3] ptp: add pulse signal loopback support for debugging

[Jakub Kicinski]

On Thu, 4 Sep 2025 06:05:02 -0700 Richard Cochran wrote:
> On Thu, Sep 04, 2025 at 01:55:43AM +0000, Wei Fang wrote:
> > Vladimir helped explain its purpose in the thread, do you still think
> > it is pointless?  
> 
> Vladimir gave practical examples for the use case, so no objection
> from my side.  I just wanted to understand how this is useful.
> 
> Next time, it would be helpful to have that info in the cover letter.

+1, let's get it reposted with updated cover letter.

I'm tempted to ask for a description under Documentation/, tho, 
I'm not 100% clear whether authors expect users or driver developers
to exercise the loop. Maybe such distinction doesn't even makes sense
for embedded devices..