diff mbox series

[net-next,09/11] ptp: ocp: Add debugfs entry for timecard

Message ID 20210830235236.309993-10-jonathan.lemon@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series ocp timecard updates | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers success CCed 3 of 3 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch warning CHECK: Macro argument 'bit' may be better as '(bit)' to avoid precedence issues CHECK: Macro argument 'gpio' may be better as '(gpio)' to avoid precedence issues CHECK: braces {} should be used on all arms of this statement
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link

Commit Message

Jonathan Lemon Aug. 30, 2021, 11:52 p.m. UTC
Provide a view into the timecard internals for debugging.

Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
 drivers/ptp/ptp_ocp.c | 238 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 237 insertions(+), 1 deletion(-)

Comments

Jakub Kicinski Sept. 2, 2021, 12:06 a.m. UTC | #1
On Mon, 30 Aug 2021 16:52:34 -0700 Jonathan Lemon wrote:
> Provide a view into the timecard internals for debugging.
> 
> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>

> +#ifdef CONFIG_DEBUG_FS
> +#define gpio_map(gpio, bit, pri, sec, def) ({			\
> +	char *_ans;						\
> +	if (gpio & (1 << bit))					\
> +		_ans = pri;					\
> +	else if (gpio & (1 << (bit + 16)))			\
> +		_ans = sec;					\
> +	else							\
> +		_ans = def;					\
> +	_ans;							\
> +})
> +
> +#define gpio_multi_map(buf, gpio, bit, pri, sec, def) ({	\
> +		char *_ans;					\
> +		_ans = buf;					\
> +		strcpy(buf, def);				\
> +		if (gpio & (1 << (bit + 16)))			\
> +			_ans += sprintf(_ans, "%s ", pri);	\
> +		if (gpio & (1 << bit))				\
> +			_ans += sprintf(_ans, "%s ", sec);	\
> +})

These can't be static inlines?

> +static int
> +ptp_ocp_summary_show(struct seq_file *s, void *data)
> +{
> +	struct device *dev = s->private;
> +	struct ts_reg __iomem *ts_reg;
> +	u32 sma_in, sma_out, val;
> +	struct timespec64 ts;
> +	struct ptp_ocp *bp;
> +	char *buf, *src;
> +	bool on;
> +
> +	buf = (char *)__get_free_page(GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	sma1_out_show(dev, NULL, buf);
> +	seq_printf(s, "   sma1: out from %s", buf);
> +
> +	sma2_out_show(dev, NULL, buf);
> +	seq_printf(s, "   sma2: out from %s", buf);
> +
> +	sma3_in_show(dev, NULL, buf);
> +	seq_printf(s, "   sma3: input to %s", buf);
> +
> +	sma4_in_show(dev, NULL, buf);
> +	seq_printf(s, "   sma4: input to %s", buf);

Why duplicate the data already available via sysfs?

> +static int
> +ptp_ocp_debugfs_add_device(struct ptp_ocp *bp)
> +{
> +	struct dentry *d;
> +
> +	d = debugfs_create_dir(dev_name(&bp->dev), ptp_ocp_debugfs_root);
> +	if (IS_ERR(d))
> +		return PTR_ERR(d);

Driver's are not supposed to depend on debugfs, you should be able to
carry on and all debugfs functions you pass an error pointer as a
parent will just return the same error right back.

> +	bp->debug_root = d;
> +
> +	d = debugfs_create_file("summary", 0444, bp->debug_root,
> +				&bp->dev, &ptp_ocp_summary_fops);
> +	if (IS_ERR(d))
> +		goto fail;
> +
> +	return 0;
> +
> +fail:
> +	debugfs_remove_recursive(bp->debug_root);
> +	bp->debug_root = NULL;
> +
> +	return PTR_ERR(d);
> +}
> +
> +static void
> +ptp_ocp_debugfs_remove_device(struct ptp_ocp *bp)
> +{
> +	debugfs_remove_recursive(bp->debug_root);
> +}
> +
> +static int
> +ptp_ocp_debugfs_init(void)
> +{
> +	struct dentry *d;
> +
> +	d = debugfs_create_dir("timecard", NULL);
> +	if (IS_ERR(d))
> +		return PTR_ERR(d);
> +
> +	ptp_ocp_debugfs_root = d;
> +
> +	return 0;
> +}
> +
> +static void
> +ptp_ocp_debugfs_fini(void)
> +{
> +	debugfs_remove_recursive(ptp_ocp_debugfs_root);
> +}
> +#else
> +#define ptp_ocp_debugfs_init()			0
> +#define ptp_ocp_debugfs_fini()
> +#define ptp_ocp_debugfs_add_device(bp)		0
> +#define ptp_ocp_debugfs_remove_device(bp)
> +#endif

This should not be necessary. Compiler should remove all those
functions as dead code when debugfs is not compiled in.

>  static void
>  ptp_ocp_dev_release(struct device *dev)
>  {
Jonathan Lemon Sept. 2, 2021, 5 p.m. UTC | #2
On Wed, Sep 01, 2021 at 05:06:41PM -0700, Jakub Kicinski wrote:
> On Mon, 30 Aug 2021 16:52:34 -0700 Jonathan Lemon wrote:
> > Provide a view into the timecard internals for debugging.
> > 
> > Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
> 
> > +#ifdef CONFIG_DEBUG_FS
> > +#define gpio_map(gpio, bit, pri, sec, def) ({			\
> > +	char *_ans;						\
> > +	if (gpio & (1 << bit))					\
> > +		_ans = pri;					\
> > +	else if (gpio & (1 << (bit + 16)))			\
> > +		_ans = sec;					\
> > +	else							\
> > +		_ans = def;					\
> > +	_ans;							\
> > +})
> > +
> > +#define gpio_multi_map(buf, gpio, bit, pri, sec, def) ({	\
> > +		char *_ans;					\
> > +		_ans = buf;					\
> > +		strcpy(buf, def);				\
> > +		if (gpio & (1 << (bit + 16)))			\
> > +			_ans += sprintf(_ans, "%s ", pri);	\
> > +		if (gpio & (1 << bit))				\
> > +			_ans += sprintf(_ans, "%s ", sec);	\
> > +})
> 
> These can't be static inlines?

Fixed - old habit of writing macros.


> > +static int
> > +ptp_ocp_summary_show(struct seq_file *s, void *data)
> > +{
> > +	struct device *dev = s->private;
> > +	struct ts_reg __iomem *ts_reg;
> > +	u32 sma_in, sma_out, val;
> > +	struct timespec64 ts;
> > +	struct ptp_ocp *bp;
> > +	char *buf, *src;
> > +	bool on;
> > +
> > +	buf = (char *)__get_free_page(GFP_KERNEL);
> > +	if (!buf)
> > +		return -ENOMEM;
> > +
> > +	sma1_out_show(dev, NULL, buf);
> > +	seq_printf(s, "   sma1: out from %s", buf);
> > +
> > +	sma2_out_show(dev, NULL, buf);
> > +	seq_printf(s, "   sma2: out from %s", buf);
> > +
> > +	sma3_in_show(dev, NULL, buf);
> > +	seq_printf(s, "   sma3: input to %s", buf);
> > +
> > +	sma4_in_show(dev, NULL, buf);
> > +	seq_printf(s, "   sma4: input to %s", buf);
> 
> Why duplicate the data already available via sysfs?

It is convenient to have all the information on 
one page when trying to understand where the signals
are being steered.


[ snip ]

> Driver's are not supposed to depend on debugfs, you should be able to
> carry on and all debugfs functions you pass an error pointer as a
> parent will just return the same error right back.

Ack.

> This should not be necessary. Compiler should remove all those
> functions as dead code when debugfs is not compiled in.

Removed #ifdef - now the _summary function is left in 
and the compiler removes it in the dead code pass.
diff mbox series

Patch

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 093385c6fed0..daa95f48c39f 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -4,6 +4,7 @@ 
 #include <linux/err.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/debugfs.h>
 #include <linux/init.h>
 #include <linux/pci.h>
 #include <linux/serial_8250.h>
@@ -208,6 +209,7 @@  struct ptp_ocp {
 	struct tod_reg __iomem	*tod;
 	struct pps_reg __iomem	*pps_to_ext;
 	struct pps_reg __iomem	*pps_to_clk;
+	struct gpio_reg __iomem	*pps_select;
 	struct gpio_reg __iomem	*sma;
 	struct irig_master_reg	__iomem *irig_out;
 	struct irig_slave_reg	__iomem *irig_in;
@@ -224,6 +226,7 @@  struct ptp_ocp {
 	struct platform_device	*spi_flash;
 	struct clk_hw		*i2c_clk;
 	struct timer_list	watchdog;
+	struct dentry		*debug_root;
 	time64_t		gnss_lost;
 	int			id;
 	int			n_irqs;
@@ -354,6 +357,10 @@  static struct ocp_resource ocp_fb_resource[] = {
 		OCP_MEM_RESOURCE(image),
 		.offset = 0x00020000, .size = 0x1000,
 	},
+	{
+		OCP_MEM_RESOURCE(pps_select),
+		.offset = 0x00130000, .size = 0x1000,
+	},
 	{
 		OCP_MEM_RESOURCE(sma),
 		.offset = 0x00140000, .size = 0x1000,
@@ -1684,6 +1691,223 @@  static struct attribute *timecard_attrs[] = {
 };
 ATTRIBUTE_GROUPS(timecard);
 
+#ifdef CONFIG_DEBUG_FS
+#define gpio_map(gpio, bit, pri, sec, def) ({			\
+	char *_ans;						\
+	if (gpio & (1 << bit))					\
+		_ans = pri;					\
+	else if (gpio & (1 << (bit + 16)))			\
+		_ans = sec;					\
+	else							\
+		_ans = def;					\
+	_ans;							\
+})
+
+#define gpio_multi_map(buf, gpio, bit, pri, sec, def) ({	\
+		char *_ans;					\
+		_ans = buf;					\
+		strcpy(buf, def);				\
+		if (gpio & (1 << (bit + 16)))			\
+			_ans += sprintf(_ans, "%s ", pri);	\
+		if (gpio & (1 << bit))				\
+			_ans += sprintf(_ans, "%s ", sec);	\
+})
+
+static int
+ptp_ocp_summary_show(struct seq_file *s, void *data)
+{
+	struct device *dev = s->private;
+	struct ts_reg __iomem *ts_reg;
+	u32 sma_in, sma_out, val;
+	struct timespec64 ts;
+	struct ptp_ocp *bp;
+	char *buf, *src;
+	bool on;
+
+	buf = (char *)__get_free_page(GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	sma1_out_show(dev, NULL, buf);
+	seq_printf(s, "   sma1: out from %s", buf);
+
+	sma2_out_show(dev, NULL, buf);
+	seq_printf(s, "   sma2: out from %s", buf);
+
+	sma3_in_show(dev, NULL, buf);
+	seq_printf(s, "   sma3: input to %s", buf);
+
+	sma4_in_show(dev, NULL, buf);
+	seq_printf(s, "   sma4: input to %s", buf);
+
+	bp = dev_get_drvdata(dev);
+	sma_in = ioread32(&bp->sma->gpio1);
+	sma_out = ioread32(&bp->sma->gpio2);
+
+	if (bp->ts0) {
+		ts_reg = bp->ts0->mem;
+		on = ioread32(&ts_reg->enable);
+		src = "GNSS";
+		seq_printf(s, "%7s: %s, src: %s\n", "TS0",
+			   on ? " ON" : "OFF", src);
+	}
+
+	if (bp->ts1) {
+		ts_reg = bp->ts1->mem;
+		on = ioread32(&ts_reg->enable);
+		src = gpio_map(sma_in, 2, "sma4", "sma3", "----");
+		seq_printf(s, "%7s: %s, src: %s\n", "TS1",
+			   on ? " ON" : "OFF", src);
+	}
+
+	if (bp->ts2) {
+		ts_reg = bp->ts2->mem;
+		on = ioread32(&ts_reg->enable);
+		src = gpio_map(sma_in, 3, "sma4", "sma3", "----");
+		seq_printf(s, "%7s: %s, src: %s\n", "TS2",
+			   on ? " ON" : "OFF", src);
+	}
+
+	if (bp->irig_out) {
+		on = ioread32(&bp->irig_out->ctrl) & IRIG_M_CTRL_ENABLE;
+		val = ioread32(&bp->irig_out->status);
+		gpio_multi_map(buf, sma_out, 4, "sma1", "sma2", "----");
+		seq_printf(s, "%7s: %s, error: %d, out: %s\n", "IRIG",
+			   on ? " ON" : "OFF", val, buf);
+	}
+
+	if (bp->irig_in) {
+		on = ioread32(&bp->irig_in->ctrl) & IRIG_S_CTRL_ENABLE;
+		val = ioread32(&bp->irig_in->status);
+		src = gpio_map(sma_in, 4, "sma4", "sma3", "----");
+		seq_printf(s, "%7s: %s, error: %d, src: %s\n", "IRIG in",
+			   on ? " ON" : "OFF", val, src);
+	}
+
+	if (bp->dcf_out) {
+		on = ioread32(&bp->dcf_out->ctrl) & DCF_M_CTRL_ENABLE;
+		val = ioread32(&bp->dcf_out->status);
+		gpio_multi_map(buf, sma_out, 5, "sma1", "sma2", "----");
+		seq_printf(s, "%7s: %s, error: %d, out: %s\n", "DCF",
+			   on ? " ON" : "OFF", val, buf);
+	}
+
+	if (bp->dcf_in) {
+		on = ioread32(&bp->dcf_in->ctrl) & DCF_S_CTRL_ENABLE;
+		val = ioread32(&bp->dcf_in->status);
+		src = gpio_map(sma_in, 5, "sma4", "sma3", "----");
+		seq_printf(s, "%7s: %s, error: %d, src: %s\n", "DCF in",
+			   on ? " ON" : "OFF", val, src);
+	}
+
+	src = gpio_map(sma_in, 2, "sma4", "sma3", "GNSS");
+	sprintf(buf, "%s via PPS2", src);
+	seq_printf(s, " MAC PPS src: %s\n", buf);
+
+	/* assumes automatic switchover/selection */
+	val = ioread32(&bp->reg->select);
+	switch (val >> 16) {
+	case 0:
+		sprintf(buf, "----");
+		break;
+	case 2:
+		sprintf(buf, "IRIG");
+		break;
+	case 3:
+		if (bp->pps_select) {
+			val = ioread32(&bp->pps_select->gpio1);
+			if (val & 0x01) {
+				src = gpio_map(sma_in, 1,
+					       "sma4", "sma3", "----");
+			} else if (val & 0x02)
+				src = "MAC";
+			else if (val & 0x04)
+				src = "GNSS";
+			else
+				src = "----";
+			sprintf(buf, "%s via PPS1", src);
+		} else {
+			strcpy(buf, "PPS1");
+		}
+		break;
+	case 6:
+		sprintf(buf, "DCF");
+		break;
+	default:
+		strcpy(buf, "unknown");
+		break;
+	}
+	val = ioread32(&bp->reg->status);
+	seq_printf(s, "%7s: %s, state: %s\n", "PHC src", buf,
+		   val & OCP_STATUS_IN_SYNC ? "sync" : "unsynced");
+
+	if (!ptp_ocp_gettimex(&bp->ptp_info, &ts, NULL))
+		seq_printf(s, "%7s: %lld.%ld == %ptT\n", "PHC",
+			   ts.tv_sec, ts.tv_nsec, &ts);
+
+	free_page((unsigned long)buf);
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(ptp_ocp_summary);
+
+static struct dentry *ptp_ocp_debugfs_root;
+
+static int
+ptp_ocp_debugfs_add_device(struct ptp_ocp *bp)
+{
+	struct dentry *d;
+
+	d = debugfs_create_dir(dev_name(&bp->dev), ptp_ocp_debugfs_root);
+	if (IS_ERR(d))
+		return PTR_ERR(d);
+	bp->debug_root = d;
+
+	d = debugfs_create_file("summary", 0444, bp->debug_root,
+				&bp->dev, &ptp_ocp_summary_fops);
+	if (IS_ERR(d))
+		goto fail;
+
+	return 0;
+
+fail:
+	debugfs_remove_recursive(bp->debug_root);
+	bp->debug_root = NULL;
+
+	return PTR_ERR(d);
+}
+
+static void
+ptp_ocp_debugfs_remove_device(struct ptp_ocp *bp)
+{
+	debugfs_remove_recursive(bp->debug_root);
+}
+
+static int
+ptp_ocp_debugfs_init(void)
+{
+	struct dentry *d;
+
+	d = debugfs_create_dir("timecard", NULL);
+	if (IS_ERR(d))
+		return PTR_ERR(d);
+
+	ptp_ocp_debugfs_root = d;
+
+	return 0;
+}
+
+static void
+ptp_ocp_debugfs_fini(void)
+{
+	debugfs_remove_recursive(ptp_ocp_debugfs_root);
+}
+#else
+#define ptp_ocp_debugfs_init()			0
+#define ptp_ocp_debugfs_fini()
+#define ptp_ocp_debugfs_add_device(bp)		0
+#define ptp_ocp_debugfs_remove_device(bp)
+#endif
+
 static void
 ptp_ocp_dev_release(struct device *dev)
 {
@@ -1787,6 +2011,9 @@  ptp_ocp_complete(struct ptp_ocp *bp)
 	if (device_add_groups(&bp->dev, timecard_groups))
 		pr_err("device add groups failed\n");
 
+	if (ptp_ocp_debugfs_add_device(bp))
+		pr_err("debugfs add device failed\n");
+
 	return 0;
 }
 
@@ -1827,6 +2054,7 @@  ptp_ocp_detach_sysfs(struct ptp_ocp *bp)
 static void
 ptp_ocp_detach(struct ptp_ocp *bp)
 {
+	ptp_ocp_debugfs_remove_device(bp);
 	ptp_ocp_detach_sysfs(bp);
 	if (timer_pending(&bp->watchdog))
 		del_timer_sync(&bp->watchdog);
@@ -1997,10 +2225,15 @@  ptp_ocp_init(void)
 	const char *what;
 	int err;
 
+	what = "debugfs entry";
+	err = ptp_ocp_debugfs_init();
+	if (err)
+		goto out;
+
 	what = "timecard class";
 	err = class_register(&timecard_class);
 	if (err)
-		goto out;
+		goto out_debug;
 
 	what = "i2c notifier";
 	err = bus_register_notifier(&i2c_bus_type, &ptp_ocp_i2c_notifier);
@@ -2018,6 +2251,8 @@  ptp_ocp_init(void)
 	bus_unregister_notifier(&i2c_bus_type, &ptp_ocp_i2c_notifier);
 out_notifier:
 	class_unregister(&timecard_class);
+out_debug:
+	ptp_ocp_debugfs_fini();
 out:
 	pr_err(KBUILD_MODNAME ": failed to register %s: %d\n", what, err);
 	return err;
@@ -2029,6 +2264,7 @@  ptp_ocp_fini(void)
 	bus_unregister_notifier(&i2c_bus_type, &ptp_ocp_i2c_notifier);
 	pci_unregister_driver(&ptp_ocp_driver);
 	class_unregister(&timecard_class);
+	ptp_ocp_debugfs_fini();
 }
 
 module_init(ptp_ocp_init);