[PATCH RFC/RFT 0/1] add simple logic analyzer using polling

[PATCH RFC/RFT 0/1] add simple logic analyzer using polling

[Wolfram Sang]

Okay, this one is maybe a bit brave, let's see if it is suitable for
upstream. This is an in-kernel logic analyzer based on GPIO polling with
local irqs disabled. Besides the driver, there is a script which
isolates a CPU to get towards the best possible result. I am aware of
the latency limitations. However, the intention is only for debugging.
Especially for remote debugging and to get a first impression, this has
already been useful. So, I wonder if we want to provide this for others,
too, and have it in Linus' tree. Documentation is within the patch, to
get a better idea what this is all about.

A branch with preparation for the Renesas Salvator-XS boards is here:
git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/topic/gpio-logic-analyzer

The documentation is also available online on the elinux wiki:
https://elinux.org/Kernel_GPIO_Logic_analyzer

Looking forward to comments. If somebody has a pointer how to detect if
a task was requested to be killed (while irqs and preemption are
disabled), I'd appreciate that to avoid the currently unkillable
sub-process.

All the best,

   Wolfram


Wolfram Sang (1):
  misc: add simple logic analyzer using polling

 .../dev-tools/gpio-logic-analyzer.rst         |  63 ++++
 Documentation/dev-tools/index.rst             |   1 +
 .../bindings/misc/gpio-logic-analyzer.yaml    |  40 ++
 drivers/misc/Kconfig                          |  12 +
 drivers/misc/Makefile                         |   1 +
 drivers/misc/gpio-logic-analyzer.c            | 355 ++++++++++++++++++
 tools/debugging/gpio-logic-analyzer           | 156 ++++++++
 7 files changed, 628 insertions(+)
 create mode 100644 Documentation/dev-tools/gpio-logic-analyzer.rst
 create mode 100644 Documentation/devicetree/bindings/misc/gpio-logic-analyzer.yaml
 create mode 100644 drivers/misc/gpio-logic-analyzer.c
 create mode 100755 tools/debugging/gpio-logic-analyzer

-- 
2.30.0

[PATCH RFC/RFT 1/1] misc: add simple logic analyzer using polling

[Wolfram Sang]

This is a simple logic analyzer using GPIO polling. It comes with a
script to isolate a CPU for polling. While this is definately not a
production level analyzer, it can be a helpful first view when remote
debugging. Read the documentation for details.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 .../dev-tools/gpio-logic-analyzer.rst         |  63 ++++
 Documentation/dev-tools/index.rst             |   1 +
 .../bindings/misc/gpio-logic-analyzer.yaml    |  40 ++
 drivers/misc/Kconfig                          |  12 +
 drivers/misc/Makefile                         |   1 +
 drivers/misc/gpio-logic-analyzer.c            | 355 ++++++++++++++++++
 tools/debugging/gpio-logic-analyzer           | 156 ++++++++
 7 files changed, 628 insertions(+)
 create mode 100644 Documentation/dev-tools/gpio-logic-analyzer.rst
 create mode 100644 Documentation/devicetree/bindings/misc/gpio-logic-analyzer.yaml
 create mode 100644 drivers/misc/gpio-logic-analyzer.c
 create mode 100755 tools/debugging/gpio-logic-analyzer

diff --git a/Documentation/dev-tools/gpio-logic-analyzer.rst b/Documentation/dev-tools/gpio-logic-analyzer.rst
new file mode 100644
index 000000000000..2847260736d4
--- /dev/null
+++ b/Documentation/dev-tools/gpio-logic-analyzer.rst
@@ -0,0 +1,63 @@
+Linux Kernel GPIO based logic analyzer
+======================================
+
+:Author: Wolfram Sang
+
+Introduction
+------------
+
+This document briefly describes how to run the software based in-kernel logic
+analyzer.
+
+Note that this is still a last resort analyzer which can be affected by
+latencies and non-determinant code paths. However, for e.g. remote development,
+it may be useful to get a first view and aid further debugging.
+
+Setup
+-----
+
+Tell the kernel which GPIOs are used as probes. For a DT based system:
+
+    i2c-analyzer {
+            compatible = "gpio-logic-analyzer";
+            probe-gpios = <&gpio6 21 GPIO_OPEN_DRAIN>, <&gpio6 4 GPIO_OPEN_DRAIN>;
+            probe-names = "SCL", "SDA";
+    };
+
+The binding documentation is in the ``misc`` folder of the Kernel binding
+documentation.
+
+Usage
+-----
+
+The logic analyzer is configurable via files in debugfs. However, it is
+strongly recommended to not use them directly, but to to use the
+``gpio-logic-analyzer`` script in the ``tools/debugging`` directory. Besides
+checking parameters more extensively, it will isolate a CPU core for you, so
+you will have least disturbance while measuring.
+
+The script has a help option explaining the parameters. For the above DT
+snipplet which analyzes an I2C bus at 400KHz on a Renesas Salvator-XS board,
+the following settings are used: The isolated CPU shall be CPU1 because it is a
+big core in a big.LITTLE setup. Because CPU1 is the default, we don't need a
+parameter. The bus speed is 400kHz. So, the sampling theorem says we need to
+sample at least at 800kHz. However, falling of both, SDA and SCL, in a start
+condition is faster, so we need a higher sampling frequency, e.g. ``-s
+1500000`` for 1.5MHz. Also, we don't want to sample right away but wait for a
+start condition on an idle bus. So, we need to set a trigger to a falling edge
+on SDA, i.e. ``-t "2F"``. Last is the duration, let us assume 15ms here which
+results in the parameter ``-d 15000``. So, altogether:
+
+    gpio-logic-analyzer -s 1500000 -t "2F" -d 15000
+
+Note that the process will return you back to the prompt but a sub-process is
+still sampling in the background. Unless this finished, you will not find a
+result file in the current or specified directory. Please also note that
+currently this sub-process is not killable! For the above example, we will then
+need to trigger I2C communication:
+
+    i2cdetect -y -r <your bus number>
+
+Result is a .sr file to be consumed with PulseView from the free Sigrok project. It is
+a zip file which also contains the binary sample data which may be consumed by others.
+The filename is the logic analyzer instance name plus a since-epoch timestamp.
diff --git a/Documentation/dev-tools/index.rst b/Documentation/dev-tools/index.rst
index 1b1cf4f5c9d9..9e0168bd3698 100644
--- a/Documentation/dev-tools/index.rst
+++ b/Documentation/dev-tools/index.rst
@@ -27,6 +27,7 @@ whole; patches welcome!
    kgdb
    kselftest
    kunit/index
+   gpio-logic-analyzer
 
 
 .. only::  subproject and html
diff --git a/Documentation/devicetree/bindings/misc/gpio-logic-analyzer.yaml b/Documentation/devicetree/bindings/misc/gpio-logic-analyzer.yaml
new file mode 100644
index 000000000000..e664cec85a72
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/gpio-logic-analyzer.yaml
@@ -0,0 +1,40 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/misc/gpio-logic-analyzer.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Bindings for a GPIO based logic analyzer
+
+maintainers:
+  - Wolfram Sang <wsa@sang-engineering.com>
+
+properties:
+  compatible:
+    items:
+      - const: gpio-logic-analyzer
+
+  probe-gpios:
+    description:
+      gpios used as probes for the logic analyzer
+
+  probe-names:
+    description:
+      names used to distinguish the probes
+
+  required:
+    - compatible
+    - probe-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c-analyzer {
+            compatible = "gpio-logic-analyzer";
+
+            probe-gpios = <&gpio6 21 GPIO_OPEN_DRAIN>, <&gpio6 4 GPIO_OPEN_DRAIN>;
+            probe-names = "SCL", "SDA";
+    };
+
+...
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index f532c59bb59b..6b1c1c951d74 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -445,6 +445,18 @@ config HISI_HIKEY_USB
 	  switching between the dual-role USB-C port and the USB-A host ports
 	  using only one USB controller.
 
+config GPIO_LOGIC_ANALYZER
+	tristate "Simple GPIO logic analyzer"
+	depends on GPIOLIB || COMPILE_TEST
+	help
+	  This option enables support for a simple logic analyzer using polled
+	  GPIOs. Use the 'tools/debugging/gpio-logic-analyzer' script with this
+	  driver. The script will make using it easier and can also isolate a
+	  CPU for the polling task. Note that this is still a last resort
+	  analyzer which can be affected by latencies and non-determinant code
+	  paths. However, for e.g. remote development, it may be useful to get
+	  a first view and aid further debugging.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 99b6f15a3c70..cff9c0a2a2fb 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -56,3 +56,4 @@ obj-$(CONFIG_HABANA_AI)		+= habanalabs/
 obj-$(CONFIG_UACCE)		+= uacce/
 obj-$(CONFIG_XILINX_SDFEC)	+= xilinx_sdfec.o
 obj-$(CONFIG_HISI_HIKEY_USB)	+= hisi_hikey_usb.o
+obj-$(CONFIG_GPIO_LOGIC_ANALYZER)	+= gpio-logic-analyzer.o
diff --git a/drivers/misc/gpio-logic-analyzer.c b/drivers/misc/gpio-logic-analyzer.c
new file mode 100644
index 000000000000..3c8d5e1f8489
--- /dev/null
+++ b/drivers/misc/gpio-logic-analyzer.c
@@ -0,0 +1,355 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Simple logic analyzer using GPIOs (to be run on an isolated CPU)
+ *
+ * Use the 'gpio-logic-analyzer' script in the 'tools/debugging' folder for
+ * easier usage and further documentation. Note that this is still a last resort
+ * analyzer which can be affected by latencies and non-determinant code paths.
+ * However, for e.g. remote development, it may be useful to get a first view
+ * and aid further debugging.
+ *
+ * Copyright (C) Wolfram Sang <wsa@sang-engineering.com>
+ * Copyright (C) Renesas Electronics Corporation
+ */
+
+#include <linux/ctype.h>
+#include <linux/debugfs.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/init.h>
+#include <linux/ktime.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/sizes.h>
+#include <linux/timekeeping.h>
+#include <linux/vmalloc.h>
+
+#define GPIO_LA_NAME "gpio-logic-analyzer"
+#define GPIO_LA_DEFAULT_BUF_SIZE SZ_256K
+/* can be increased if needed */
+#define GPIO_LA_MAX_PROBES 8
+#define GPIO_LA_PROBES_MASK 7
+
+struct gpio_la_poll_priv {
+	unsigned long ndelay;
+	u32 buf_idx;
+	struct mutex lock;
+	struct debugfs_blob_wrapper blob;
+	struct gpio_descs *descs;
+	struct dentry *debug_dir, *blob_dent;
+	struct debugfs_blob_wrapper meta;
+	unsigned long gpio_delay;
+	unsigned int trigger_len;
+	u8 trigger_data[PAGE_SIZE];
+};
+
+static struct dentry *gpio_la_poll_debug_dir;
+
+static int fops_capture_set(void *data, u64 val)
+{
+	struct gpio_la_poll_priv *priv = data;
+	u8 *la_buf = priv->blob.data;
+	unsigned long state = 0;
+	int i, ret;
+
+	if (!la_buf)
+		return -ENOMEM;
+
+	if (val) {
+		mutex_lock(&priv->lock);
+		if (priv->blob_dent) {
+			debugfs_remove(priv->blob_dent);
+			priv->blob_dent = NULL;
+		}
+
+		priv->buf_idx = 0;
+
+		local_irq_disable();
+		preempt_disable_notrace();
+
+		for (i = 0; i < priv->trigger_len; i++) {
+			u8 data = priv->trigger_data[i];
+
+			do {
+				ret = gpiod_get_array_value(priv->descs->ndescs, priv->descs->desc,
+							    priv->descs->info, &state);
+
+				if (ret)
+					goto gpio_err;
+			} while (!!(state & BIT(data & GPIO_LA_PROBES_MASK)) != !!(data & 0x80));
+		}
+
+		if (priv->trigger_len) {
+			la_buf[priv->buf_idx++] = state;
+			ndelay(priv->ndelay);
+		}
+
+		while (priv->buf_idx < priv->blob.size && ret == 0) {
+			ret = gpiod_get_array_value(priv->descs->ndescs, priv->descs->desc,
+					      priv->descs->info, &state);
+			la_buf[priv->buf_idx++] = state;
+			ndelay(priv->ndelay);
+		}
+gpio_err:
+		preempt_enable_notrace();
+		local_irq_enable();
+		if (ret)
+			pr_err("%s: couldn't read GPIOs: %d\n", __func__, ret);
+
+		priv->blob_dent = debugfs_create_blob("sample_data", 0400, priv->debug_dir, &priv->blob);
+		mutex_unlock(&priv->lock);
+	}
+
+	return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(fops_capture, NULL, fops_capture_set, "%llu\n");
+
+static int fops_buf_size_get(void *data, u64 *val)
+{
+	struct gpio_la_poll_priv *priv = data;
+
+	*val = priv->blob.size;
+
+	return 0;
+}
+
+static int fops_buf_size_set(void *data, u64 val)
+{
+	struct gpio_la_poll_priv *priv = data;
+	int ret = 0;
+	void *p;
+
+	if (!val)
+		return -EINVAL;
+
+	mutex_lock(&priv->lock);
+
+	vfree(priv->blob.data);
+	p = vzalloc(val);
+	if (!p) {
+		/* Try the old value again */
+		val = priv->blob.size;
+		p = vzalloc(val);
+		if (!p) {
+			val = 0;
+			ret = -ENOMEM;
+		}
+	}
+
+	priv->blob.data = p;
+	priv->blob.size = val;
+
+	mutex_unlock(&priv->lock);
+	return ret;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(fops_buf_size, fops_buf_size_get, fops_buf_size_set, "%llu\n");
+
+static int trigger_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, NULL, inode->i_private);
+}
+
+static ssize_t trigger_write(struct file *file, const char __user *ubuf,
+			     size_t count, loff_t *offset)
+{
+	struct seq_file *m = file->private_data;
+	struct gpio_la_poll_priv *priv = m->private;
+	char *buf;
+	int i, trigger_len = 0;
+
+	priv->trigger_len = 0;
+
+	if (count & 1)
+	    return -EINVAL;
+
+	buf = memdup_user(ubuf, count);
+	if (IS_ERR(buf))
+		return PTR_ERR(buf);
+
+	for (i = 0; i < count; i += 2) {
+		u8 val;
+
+		if (buf[i] < '1' || buf[i] > '0' + GPIO_LA_MAX_PROBES)
+			goto bail_out;
+
+		val = buf[i] - '1';
+
+		switch (toupper(buf[i + 1])) {
+		case 'L':
+			priv->trigger_data[trigger_len] = val;
+			trigger_len++;
+			break;
+		case 'H':
+			priv->trigger_data[trigger_len] = val | 0x80;
+			trigger_len++;
+			break;
+		case 'R':
+			priv->trigger_data[trigger_len] = val;
+			priv->trigger_data[trigger_len + 1] = val | 0x80;
+			trigger_len += 2;
+			break;
+		case 'F':
+			priv->trigger_data[trigger_len] = val | 0x80;
+			priv->trigger_data[trigger_len + 1] = val;
+			trigger_len += 2;
+			break;
+		default:
+			goto bail_out;
+		}
+
+		if (trigger_len > PAGE_SIZE)	/* should never happen */
+			goto bail_out;
+
+	}
+
+	priv->trigger_len = trigger_len;
+
+bail_out:
+	kfree(buf);
+	return priv->trigger_len ? count : -EINVAL;
+}
+
+static const struct file_operations fops_trigger = {
+	.owner = THIS_MODULE,
+	.open = trigger_open,
+	.write = trigger_write,
+	.llseek = no_llseek,
+	.release = single_release,
+};
+
+static int gpio_la_poll_probe(struct platform_device *pdev)
+{
+	struct gpio_la_poll_priv *priv;
+	struct device *dev = &pdev->dev;
+	char *meta = NULL;
+	unsigned long state;
+	ktime_t start_time, end_time;
+	int ret, i;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	mutex_init(&priv->lock);
+
+	fops_buf_size_set(priv, GPIO_LA_DEFAULT_BUF_SIZE);
+
+	priv->descs = devm_gpiod_get_array(dev, "probe", GPIOD_IN);
+	if (IS_ERR(priv->descs))
+		return PTR_ERR(priv->descs);
+
+	/* artificial limit to keep 1 byte per sample for now */
+	if (priv->descs->ndescs > GPIO_LA_MAX_PROBES)
+		return -ERANGE;
+
+	for (i = 0; i < priv->descs->ndescs; i++) {
+		const char *str, *old_meta;
+		char *def_name = "ProbeX\0";
+
+		if (gpiod_cansleep(priv->descs->desc[i]))
+			return -EREMOTE;
+
+		ret = of_property_read_string_index(pdev->dev.of_node, "probe-names",
+						    i, &str);
+
+		/* Hacky way of providing a fallback name if none provided */
+		if (ret) {
+			def_name[5] = i + '1'; /* assumes GPIO_LA_MAX_PROBES = 8 */
+			str = def_name;
+		}
+
+		gpiod_set_consumer_name(priv->descs->desc[i], str);
+
+		old_meta = meta;
+		meta = devm_kasprintf(dev, GFP_KERNEL, "%sprobe%d=%s\n",
+				      old_meta ?: "", i + 1, str);
+		if (!meta)
+			return -ENOMEM;
+
+		devm_kfree(dev, old_meta);
+	}
+
+	platform_set_drvdata(pdev, priv);
+
+	/* Measure delay of reading GPIOs */
+	local_irq_disable();
+	preempt_disable_notrace();
+	start_time = ktime_get();
+	for (i = 0, ret = 0; i < 1024 && ret == 0; i++)
+		ret = gpiod_get_array_value(priv->descs->ndescs, priv->descs->desc,
+				      priv->descs->info, &state);
+	end_time = ktime_get();
+	preempt_enable_notrace();
+	local_irq_enable();
+	if (ret) {
+		dev_err(dev, "couldn't read GPIOs: %d\n", ret);
+		return ret;
+	}
+
+	priv->gpio_delay = ktime_sub(end_time, start_time) / 1024;
+
+	priv->debug_dir = debugfs_create_dir(dev_name(dev), gpio_la_poll_debug_dir);
+	if (IS_ERR(priv->debug_dir))
+		return PTR_ERR(priv->debug_dir);
+
+	priv->meta.data = meta;
+	priv->meta.size = strlen(meta);
+	debugfs_create_blob("meta_data", 0400, priv->debug_dir, &priv->meta);
+	debugfs_create_ulong("delay_ns_acquisition", 0400, priv->debug_dir, &priv->gpio_delay);
+	debugfs_create_ulong("delay_ns_user", 0600, priv->debug_dir, &priv->ndelay);
+	debugfs_create_file_unsafe("buf_size", 0600, priv->debug_dir, priv, &fops_buf_size);
+	debugfs_create_file_unsafe("capture", 0200, priv->debug_dir, priv, &fops_capture);
+	debugfs_create_file_unsafe("trigger", 0200, priv->debug_dir, priv, &fops_trigger);
+
+	return 0;
+}
+
+static int gpio_la_poll_remove(struct platform_device *pdev)
+{
+	struct gpio_la_poll_priv *priv = platform_get_drvdata(pdev);
+
+	mutex_lock(&priv->lock);
+	debugfs_remove_recursive(priv->debug_dir);
+	mutex_unlock(&priv->lock);
+
+	return 0;
+}
+
+static const struct of_device_id gpio_la_poll_of_match[] = {
+	{ .compatible = GPIO_LA_NAME, },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, gpio_la_poll_of_match);
+
+static struct platform_driver gpio_la_poll_device_driver = {
+	.probe = gpio_la_poll_probe,
+	.remove = gpio_la_poll_remove,
+	.driver = {
+		.name = GPIO_LA_NAME,
+		.of_match_table = gpio_la_poll_of_match,
+	}
+};
+
+static int __init gpio_la_poll_init(void)
+{
+	gpio_la_poll_debug_dir = debugfs_create_dir(GPIO_LA_NAME, NULL);
+	if (IS_ERR(gpio_la_poll_debug_dir))
+		return PTR_ERR(gpio_la_poll_debug_dir);
+
+	return platform_driver_register(&gpio_la_poll_device_driver);
+}
+late_initcall(gpio_la_poll_init);
+
+static void __exit gpio_la_poll_exit(void)
+{
+	platform_driver_unregister(&gpio_la_poll_device_driver);
+	debugfs_remove_recursive(gpio_la_poll_debug_dir);
+}
+module_exit(gpio_la_poll_exit);
+
+MODULE_AUTHOR("Wolfram Sang <wsa@sang-engineering.com>");
+MODULE_DESCRIPTION("Simple logic analyzer using GPIOs");
+MODULE_LICENSE("GPL v2");
diff --git a/tools/debugging/gpio-logic-analyzer b/tools/debugging/gpio-logic-analyzer
new file mode 100755
index 000000000000..4506f67c18bc
--- /dev/null
+++ b/tools/debugging/gpio-logic-analyzer
@@ -0,0 +1,156 @@
+#! /bin/sh
+
+INITCPU=
+SAMPLEFREQ=1000000
+NUMSAMPLES=250000
+LASYSFSDIR=
+CPUSETDIR='/dev/cpuset'
+LACPUSETDIR="$CPUSETDIR/gpio-logic-analyzer"
+SYSFSDIR='/sys/kernel/debug/gpio-logic-analyzer/'
+OUTPUTDIR="$PWD"
+TRIGGERDAT=
+NEEDEDCMDS='taskset zip'
+
+print_help()
+{
+	cat <<EOF
+$0 - helper script for the Linux Kernel Simple GPIO Logic Analyzer
+Available options:
+	-d|--duration-us <n>: number of microseconds to sample. Overrides -n, no default value.
+	-h|--help: print this help
+	-i|--init <n>: which CPU to isolate for sampling. Only needed once. Default <1>.
+		       Remember that a more powerful CPU gives you higher sample speeds.
+		       Also CPU0 is not recommended as it usually does extra bookkeeping.
+	-n|--num_samples <n>: number of samples to acquire. Default <$NUMSAMPLES>
+	-o|--output-dir <str>: directory to put the result files. Default: current dir
+	-p|--path <str>: path to Logic Analyzer dir in case you have multiple instances.
+			 Default to first instance found.
+	-s|--sample_freq <n>: desired sample frequency. Might be capped if too large. Default: 1MHz.
+	-t|--trigger <str>: pattern to use as trigger. <str> consists of n two-char pairs. First
+			    char is channel number starting at "1". Second char is trigger level:
+			    "L" - low; "H" - high; "R" - rising; "F" - falling
+Examples:
+Samples $NUMSAMPLES at 1MHz with already prepared CPU or automatically prepare CPU1 if needed
+	'$0'
+Samples 50us at 2MHz waiting for falling edge on channel 2. CPU usage as above.
+	'$0 -d 50 -s 2000000 -t "2F"'
+
+Note that the process exits after checking all parameters but a sub-process still works in
+the background. The result is only available once the sub-process finished. As the time of
+writing, the sub-process is not killable, so be extra careful that your triggers work.
+
+Result is a .sr file to be consumed with PulseView from the free Sigrok project. It is
+a zip file which also contains the binary sample data which may be consumed by others.
+The filename is the logic analyzer instance name plus a since-epoch timestamp.
+EOF
+}
+
+set_newmask()
+{
+	local f
+	for f in $(find $1 -iname "$2"); do echo $NEWMASK > $f 2>/dev/null; done
+}
+
+init_cpu()
+{
+	CPU="$1"
+
+	[ ! -d $CPUSETDIR ] && mkdir $CPUSETDIR
+	mount | grep -q $CPUSETDIR || mount -t cpuset cpuset $CPUSETDIR
+	[ ! -d $LACPUSETDIR ] && mkdir $LACPUSETDIR
+
+	echo $CPU > $LACPUSETDIR/cpus
+	echo 1 > $LACPUSETDIR/cpu_exclusive
+	echo 0 > $LACPUSETDIR/mems
+
+	OLDMASK=$(cat /proc/irq/default_smp_affinity)
+	let val="0x$OLDMASK & ~(1 << $CPU)"
+	NEWMASK=$(printf "%x" $val)
+
+	set_newmask '/proc/irq' '*smp_affinity'
+	set_newmask '/sys/devices/virtual/workqueue/' 'cpumask'
+
+	# Move tasks away from isolated CPU
+	for p in $(ps -o pid | tail -n +2); do
+		MASK=$(taskset -p $p)
+		[ "${MASK##*: }" != "$OLDMASK" ] && continue
+		taskset -p $NEWMASK $p
+	done 2>/dev/null >/dev/null
+
+	echo 1 > /sys/module/rcupdate/parameters/rcu_cpu_stall_suppress
+}
+
+do_capture()
+{
+	taskset $1 echo 1 > $LASYSFSDIR/capture
+
+	SRTMP=$(mktemp -d)
+	echo 1 > $SRTMP/version
+	cp $LASYSFSDIR/sample_data $SRTMP/logic-1-1
+	cat > $SRTMP/metadata <<EOF
+[global]
+sigrok version=0.2.0
+
+[device 1]
+capturefile=logic-1
+total probes=$(cat $LASYSFSDIR/meta_data | wc -l)
+samplerate=${SAMPLEFREQ}Hz
+unitsize=1
+EOF
+	cat $LASYSFSDIR/meta_data >> $SRTMP/metadata
+
+	ZIPNAME="$OUTPUTDIR/${LASYSFSDIR##*/}-$(date +%s).sr"
+	zip -jq $ZIPNAME $SRTMP/*
+	rm -rf $SRTMP
+}
+
+REP=$(getopt -a -l path:,init:,sample_freq:,num_samples:,duration-us:,trigger:,output-dir:,help -o i:s:n:d:t:o:h -- "$@") || exit 1
+eval set -- "$REP"
+while true; do
+	case "$1" in
+	-d|--duration-us) DURATION="$2"; shift 2;;
+	-h|--help) print_help; exit 0;;
+	-i|--init) INITCPU="$2"; shift 2;;
+	-n|--num_samples) NUMSAMPLES="$2"; shift 2;;
+	-o|--output-dir) OUTPUTDIR="$2"; shift 2;;
+	-p|--path) LASYSFSDIR="$2"; shift 2;;
+	-s|--sample_freq) SAMPLEFREQ="$2"; shift 2;;
+	-t|--trigger) TRIGGERDAT="$2"; shift 2;;
+	--)	shift; break;;
+	*)	echo "error parsing commandline: $@"; exit 1;;
+	esac
+done
+
+for f in $NEEDEDCMDS; do
+	command -v $f >/dev/null || { echo "Command '$f' not found"; exit 1; }
+done
+
+[ $SAMPLEFREQ -eq 0 ] && echo "Invalid sample frequency" && exit 1
+
+[ -z "$LASYSFSDIR" ] && LASYSFSDIR="$SYSFSDIR/$(ls -1 $SYSFSDIR | head -n1)"
+[ ! -d "$LASYSFSDIR" ] && echo "LA directory '$LASYSFSDIR' not found!" && exit 1
+
+[ -n "$INITCPU" ] && init_cpu $INITCPU
+[ ! -d "$LACPUSETDIR" ] && echo "Auto-Isolating CPU1" && init_cpu 1
+
+let NDELAY=1000000000/$SAMPLEFREQ
+NDELAY_ACQ=$(cat $LASYSFSDIR/delay_ns_acquisition)
+[ $NDELAY_ACQ -eq 0 ] && echo "Invalid acquisition delay received" && exit 1
+let NDELAY_USER=$NDELAY-$NDELAY_ACQ
+let MAXFREQ=1000000000/$NDELAY_ACQ
+
+[ $NDELAY_USER -lt 0 ] && NDELAY_USER=0 && SAMPLEFREQ=$MAXFREQ && echo "Capping sample_freq to $MAXFREQ"
+echo $NDELAY_USER > $LASYSFSDIR/delay_ns_user
+
+[ -n "$DURATION" ] && let NUMSAMPLES=$SAMPLEFREQ*$DURATION/1000000
+echo $NUMSAMPLES > $LASYSFSDIR/buf_size
+
+if [ -n "$TRIGGERDAT" ]; then
+	echo -n "$TRIGGERDAT" > $LASYSFSDIR/trigger 2>/dev/null
+	[ $? -gt 0 ] && echo "Trigger data '$TRIGGERDAT' rejected" && exit 1
+fi
+
+CPU=$(cat $LACPUSETDIR/effective_cpus)
+[ -z "$CPU" ] && echo "No isolated CPU found" && exit 1
+let CPUMASK="1 << $CPU"
+do_capture $CPUMASK &
-- 
2.30.0

Re: [PATCH RFC/RFT 1/1] misc: add simple logic analyzer using polling

[Andy Shevchenko]

On Tue, Mar 30, 2021 at 11:58 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> This is a simple logic analyzer using GPIO polling. It comes with a
> script to isolate a CPU for polling. While this is definately not a

definitely

> production level analyzer, it can be a helpful first view when remote
> debugging. Read the documentation for details.

I would like to look at it closer, but don't have time right now. So,
some kind of a shallow review.

But the idea is, let's say, interesting.

> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

...

> +The binding documentation is in the ``misc`` folder of the Kernel binding
> +documentation.

Can't you give a reference in terms of reST format?

...

> +Note that the process will return you back to the prompt but a sub-process is
> +still sampling in the background. Unless this finished, you will not find a

this is finished

> +result file in the current or specified directory. Please also note that
> +currently this sub-process is not killable! For the above example, we will then
> +need to trigger I2C communication:

Shouldn't you use :: instead of : to mark sections as code excerpts?

> +    i2cdetect -y -r <your bus number>

...

> +config GPIO_LOGIC_ANALYZER
> +       tristate "Simple GPIO logic analyzer"
> +       depends on GPIOLIB || COMPILE_TEST
> +       help
> +         This option enables support for a simple logic analyzer using polled
> +         GPIOs. Use the 'tools/debugging/gpio-logic-analyzer' script with this
> +         driver. The script will make using it easier and can also isolate a
> +         CPU for the polling task. Note that this is still a last resort
> +         analyzer which can be affected by latencies and non-determinant code
> +         paths. However, for e.g. remote development, it may be useful to get
> +         a first view and aid further debugging.

Module name?

...

> +#include <linux/of.h>

Can you switch to use device property API?

...

> +#define GPIO_LA_NAME "gpio-logic-analyzer"
> +#define GPIO_LA_DEFAULT_BUF_SIZE SZ_256K

> +/* can be increased if needed */
> +#define GPIO_LA_MAX_PROBES 8
> +#define GPIO_LA_PROBES_MASK 7

Does this assume the power-of-two number of probes?
Perhaps using BIT(x) and (BIT(x) - 1) will clarify that.

...

> +struct gpio_la_poll_priv {
> +       unsigned long ndelay;
> +       u32 buf_idx;
> +       struct mutex lock;
> +       struct debugfs_blob_wrapper blob;
> +       struct gpio_descs *descs;
> +       struct dentry *debug_dir, *blob_dent;
> +       struct debugfs_blob_wrapper meta;
> +       unsigned long gpio_delay;
> +       unsigned int trigger_len;

> +       u8 trigger_data[PAGE_SIZE];

This is not good for fragmentation (basically you make your struct to
occupy 2 pages, one of which will be almost wasted). Better to have a
pointer here and allocate one page by get_zero_page() or so.

> +};
> +
> +static struct dentry *gpio_la_poll_debug_dir;
> +
> +static int fops_capture_set(void *data, u64 val)
> +{
> +       struct gpio_la_poll_priv *priv = data;
> +       u8 *la_buf = priv->blob.data;
> +       unsigned long state = 0;
> +       int i, ret;
> +
> +       if (!la_buf)
> +               return -ENOMEM;

> +       if (val) {

if (!val)
  return 0;

makes your life easier.

> +               mutex_lock(&priv->lock);
> +               if (priv->blob_dent) {
> +                       debugfs_remove(priv->blob_dent);
> +                       priv->blob_dent = NULL;
> +               }
> +
> +               priv->buf_idx = 0;
> +
> +               local_irq_disable();
> +               preempt_disable_notrace();
> +
> +               for (i = 0; i < priv->trigger_len; i++) {
> +                       u8 data = priv->trigger_data[i];
> +                       do {
> +                               ret = gpiod_get_array_value(priv->descs->ndescs, priv->descs->desc,
> +                                                           priv->descs->info, &state);
> +
> +                               if (ret)
> +                                       goto gpio_err;
> +                       } while (!!(state & BIT(data & GPIO_LA_PROBES_MASK)) != !!(data & 0x80));
> +               }
> +
> +               if (priv->trigger_len) {
> +                       la_buf[priv->buf_idx++] = state;
> +                       ndelay(priv->ndelay);
> +               }
> +
> +               while (priv->buf_idx < priv->blob.size && ret == 0) {
> +                       ret = gpiod_get_array_value(priv->descs->ndescs, priv->descs->desc,
> +                                             priv->descs->info, &state);
> +                       la_buf[priv->buf_idx++] = state;
> +                       ndelay(priv->ndelay);
> +               }
> +gpio_err:
> +               preempt_enable_notrace();
> +               local_irq_enable();
> +               if (ret)

> +                       pr_err("%s: couldn't read GPIOs: %d\n", __func__, ret);

Haven't noticed if you are using pr_fmt(). It may be better than using __func__.

Btw, it seems you have a struct device for that or so. Why don't you
use dev_err()?

> +               priv->blob_dent = debugfs_create_blob("sample_data", 0400, priv->debug_dir, &priv->blob);
> +               mutex_unlock(&priv->lock);
> +       }
> +
> +       return 0;
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(fops_capture, NULL, fops_capture_set, "%llu\n");


> +static ssize_t trigger_write(struct file *file, const char __user *ubuf,
> +                            size_t count, loff_t *offset)
> +{
> +       struct seq_file *m = file->private_data;
> +       struct gpio_la_poll_priv *priv = m->private;
> +       char *buf;
> +       int i, trigger_len = 0;
> +
> +       priv->trigger_len = 0;
> +
> +       if (count & 1)
> +           return -EINVAL;
> +
> +       buf = memdup_user(ubuf, count);
> +       if (IS_ERR(buf))
> +               return PTR_ERR(buf);
> +
> +       for (i = 0; i < count; i += 2) {
> +               u8 val;
> +
> +               if (buf[i] < '1' || buf[i] > '0' + GPIO_LA_MAX_PROBES)

So, you can't increase the amount of probes without breaking this
entire parser (it will go somewhere to symbols and letters...).

> +                       goto bail_out;
> +
> +               val = buf[i] - '1';
> +
> +               switch (toupper(buf[i + 1])) {
> +               case 'L':
> +                       priv->trigger_data[trigger_len] = val;
> +                       trigger_len++;
> +                       break;
> +               case 'H':
> +                       priv->trigger_data[trigger_len] = val | 0x80;
> +                       trigger_len++;
> +                       break;
> +               case 'R':
> +                       priv->trigger_data[trigger_len] = val;
> +                       priv->trigger_data[trigger_len + 1] = val | 0x80;
> +                       trigger_len += 2;
> +                       break;
> +               case 'F':
> +                       priv->trigger_data[trigger_len] = val | 0x80;
> +                       priv->trigger_data[trigger_len + 1] = val;
> +                       trigger_len += 2;
> +                       break;
> +               default:
> +                       goto bail_out;
> +               }
> +
> +               if (trigger_len > PAGE_SIZE)    /* should never happen */

Shouldn't you return OVERFLOW here or something like that?

> +                       goto bail_out;
> +
> +       }
> +
> +       priv->trigger_len = trigger_len;
> +
> +bail_out:
> +       kfree(buf);
> +       return priv->trigger_len ? count : -EINVAL;
> +}

I'm not a fan of yet another parser in the kernel. Can you provide a
bit of description of the format?

...

> +       priv->debug_dir = debugfs_create_dir(dev_name(dev), gpio_la_poll_debug_dir);

> +       if (IS_ERR(priv->debug_dir))
> +               return PTR_ERR(priv->debug_dir);

Shouldn't be checked AFAIU.

...

> +static const struct of_device_id gpio_la_poll_of_match[] = {
> +       { .compatible = GPIO_LA_NAME, },

> +       { },

No comma needed.

> +};
> +MODULE_DEVICE_TABLE(of, gpio_la_poll_of_match);

...

> +       -t|--trigger <str>: pattern to use as trigger. <str> consists of n two-char pairs. First
> +                           char is channel number starting at "1". Second char is trigger level:
> +                           "L" - low; "H" - high; "R" - rising; "F" - falling
> +Examples:
> +Samples $NUMSAMPLES at 1MHz with already prepared CPU or automatically prepare CPU1 if needed

with an already
prepares

> +       '$0'
> +Samples 50us at 2MHz waiting for falling edge on channel 2. CPU usage as above.
> +       '$0 -d 50 -s 2000000 -t "2F"'
> +
> +Note that the process exits after checking all parameters but a sub-process still works in
> +the background. The result is only available once the sub-process finished. As the time of

finishes

> +writing, the sub-process is not killable, so be extra careful that your triggers work.
> +
> +Result is a .sr file to be consumed with PulseView from the free Sigrok project. It is
> +a zip file which also contains the binary sample data which may be consumed by others.
> +The filename is the logic analyzer instance name plus a since-epoch timestamp.
> +EOF
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC/RFT 1/1] misc: add simple logic analyzer using polling

[Wolfram Sang]

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

Hi Andy,

> I would like to look at it closer, but don't have time right now. So,
> some kind of a shallow review.

Still, thanks for that!

> But the idea is, let's say, interesting.

:)

> > +The binding documentation is in the ``misc`` folder of the Kernel binding
> > +documentation.
> 
> Can't you give a reference in terms of reST format?

Sure. Still need to practice reST.

> > +config GPIO_LOGIC_ANALYZER
> > +       tristate "Simple GPIO logic analyzer"
> > +       depends on GPIOLIB || COMPILE_TEST
> > +       help
> > +         This option enables support for a simple logic analyzer using polled
> > +         GPIOs. Use the 'tools/debugging/gpio-logic-analyzer' script with this
> > +         driver. The script will make using it easier and can also isolate a
> > +         CPU for the polling task. Note that this is still a last resort
> > +         analyzer which can be affected by latencies and non-determinant code
> > +         paths. However, for e.g. remote development, it may be useful to get
> > +         a first view and aid further debugging.
> 
> Module name?

Yup, willl add.

> > +#include <linux/of.h>
> 
> Can you switch to use device property API?

IIRC I checked that and I couldn't find a replacement for
of_property_read_string_index().

> > +/* can be increased if needed */
> > +#define GPIO_LA_MAX_PROBES 8
> > +#define GPIO_LA_PROBES_MASK 7
> 
> Does this assume the power-of-two number of probes?
> Perhaps using BIT(x) and (BIT(x) - 1) will clarify that.

The arbitrary limit of 8 probes is solely to get this out now for
initial review, to check if this is upstreamable at all. If this is
considered acceptable, I can also update this to 64 probes and can get
rid of some more hackish code (e.g. fallback names of probes), too.

> > +struct gpio_la_poll_priv {
> > +       unsigned long ndelay;
> > +       u32 buf_idx;
> > +       struct mutex lock;
> > +       struct debugfs_blob_wrapper blob;
> > +       struct gpio_descs *descs;
> > +       struct dentry *debug_dir, *blob_dent;
> > +       struct debugfs_blob_wrapper meta;
> > +       unsigned long gpio_delay;
> > +       unsigned int trigger_len;
> 
> > +       u8 trigger_data[PAGE_SIZE];
> 
> This is not good for fragmentation (basically you make your struct to
> occupy 2 pages, one of which will be almost wasted). Better to have a
> pointer here and allocate one page by get_zero_page() or so.

Point taken. I will have a look.

> > +       if (val) {
> 
> if (!val)
>   return 0;
> 
> makes your life easier.

Yeah, it is cruft from an earlier version

> > +               if (ret)
> 
> > +                       pr_err("%s: couldn't read GPIOs: %d\n", __func__, ret);
> 
> Haven't noticed if you are using pr_fmt(). It may be better than using __func__.
> 
> Btw, it seems you have a struct device for that or so. Why don't you
> use dev_err()?

Will check.

> > +               if (buf[i] < '1' || buf[i] > '0' + GPIO_LA_MAX_PROBES)
> 
> So, you can't increase the amount of probes without breaking this
> entire parser (it will go somewhere to symbols and letters...).

Yeah. This is why I put GPIO_LA_MAX_PROBES there. When I upgrade the
number of probes, I need to have a look at all place using this define.
This code is ugly, I know.

> Shouldn't you return OVERFLOW here or something like that?

I could. But 4K of trigger data is also invalid. It is an academic
discussion, though. 

> I'm not a fan of yet another parser in the kernel. Can you provide a
> bit of description of the format?

It is in the help of the script. I could maybe add it to the docs, too.

> > +       if (IS_ERR(priv->debug_dir))
> > +               return PTR_ERR(priv->debug_dir);
> 
> Shouldn't be checked AFAIU.

Oh, really? Will check.

> > +static const struct of_device_id gpio_la_poll_of_match[] = {
> > +       { .compatible = GPIO_LA_NAME, },
> 
> > +       { },
> 
> No comma needed.

OK.

Thanks for your time!

   Wolfram


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

Re: [PATCH RFC/RFT 1/1] misc: add simple logic analyzer using polling

[Randy Dunlap]

Hi--

On 3/30/21 1:56 AM, Wolfram Sang wrote:
> diff --git a/Documentation/dev-tools/gpio-logic-analyzer.rst b/Documentation/dev-tools/gpio-logic-analyzer.rst
> new file mode 100644
> index 000000000000..2847260736d4
> --- /dev/null
> +++ b/Documentation/dev-tools/gpio-logic-analyzer.rst
> @@ -0,0 +1,63 @@
> +Linux Kernel GPIO based logic analyzer
> +======================================
> +
> +:Author: Wolfram Sang
> +
> +Introduction
> +------------
> +
> +This document briefly describes how to run the software based in-kernel logic
> +analyzer.
> +
> +Note that this is still a last resort analyzer which can be affected by
> +latencies and non-determinant code paths. However, for e.g. remote development,
> +it may be useful to get a first view and aid further debugging.
> +
> +Setup
> +-----
> +
> +Tell the kernel which GPIOs are used as probes. For a DT based system:
> +
> +    i2c-analyzer {
> +            compatible = "gpio-logic-analyzer";
> +            probe-gpios = <&gpio6 21 GPIO_OPEN_DRAIN>, <&gpio6 4 GPIO_OPEN_DRAIN>;
> +            probe-names = "SCL", "SDA";
> +    };
> +
> +The binding documentation is in the ``misc`` folder of the Kernel binding
> +documentation.
> +
> +Usage
> +-----
> +
> +The logic analyzer is configurable via files in debugfs. However, it is
> +strongly recommended to not use them directly, but to to use the
> +``gpio-logic-analyzer`` script in the ``tools/debugging`` directory. Besides
> +checking parameters more extensively, it will isolate a CPU core for you, so
> +you will have least disturbance while measuring.
> +
> +The script has a help option explaining the parameters. For the above DT
> +snipplet which analyzes an I2C bus at 400KHz on a Renesas Salvator-XS board,

   snippet

> +the following settings are used: The isolated CPU shall be CPU1 because it is a
> +big core in a big.LITTLE setup. Because CPU1 is the default, we don't need a
> +parameter. The bus speed is 400kHz. So, the sampling theorem says we need to
> +sample at least at 800kHz. However, falling of both, SDA and SCL, in a start

Is "falling" like a falling edge of a signal?
If not, then I think "failing" would make more sense.
Even "failing both".

> +condition is faster, so we need a higher sampling frequency, e.g. ``-s
> +1500000`` for 1.5MHz. Also, we don't want to sample right away but wait for a
> +start condition on an idle bus. So, we need to set a trigger to a falling edge
> +on SDA, i.e. ``-t "2F"``. Last is the duration, let us assume 15ms here which
> +results in the parameter ``-d 15000``. So, altogether:
> +
> +    gpio-logic-analyzer -s 1500000 -t "2F" -d 15000
> +
> +Note that the process will return you back to the prompt but a sub-process is
> +still sampling in the background. Unless this finished, you will not find a
> +result file in the current or specified directory. Please also note that
> +currently this sub-process is not killable! For the above example, we will then
> +need to trigger I2C communication:
> +
> +    i2cdetect -y -r <your bus number>
> +
> +Result is a .sr file to be consumed with PulseView from the free Sigrok project. It is
> +a zip file which also contains the binary sample data which may be consumed by others.
> +The filename is the logic analyzer instance name plus a since-epoch timestamp.


thanks.
-- 
~Randy

Re: [PATCH RFC/RFT 1/1] misc: add simple logic analyzer using polling

[Wolfram Sang]

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


> > +snipplet which analyzes an I2C bus at 400KHz on a Renesas Salvator-XS board,
> 
>    snippet

Thanks!

> 
> > +the following settings are used: The isolated CPU shall be CPU1 because it is a
> > +big core in a big.LITTLE setup. Because CPU1 is the default, we don't need a
> > +parameter. The bus speed is 400kHz. So, the sampling theorem says we need to
> > +sample at least at 800kHz. However, falling of both, SDA and SCL, in a start
> 
> Is "falling" like a falling edge of a signal?

Yes. It seems I should make it more clear, then.

Thanks for the review, Randy!


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

Re: [PATCH RFC/RFT 1/1] misc: add simple logic analyzer using polling

[Linus Walleij]

On Tue, Mar 30, 2021 at 10:58 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:

> This is a simple logic analyzer using GPIO polling. It comes with a
> script to isolate a CPU for polling. While this is definately not a
> production level analyzer, it can be a helpful first view when remote
> debugging. Read the documentation for details.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

I am a great supporter of this idea.

When we created gpiod_get_array_value() and friends, the idea
was exactly to be able to do things like this. It's a good way to
utilize the fact that several GPIO lines can often be read from a single
register read.

> +    i2c-analyzer {
> +            compatible = "gpio-logic-analyzer";
> +            probe-gpios = <&gpio6 21 GPIO_OPEN_DRAIN>, <&gpio6 4 GPIO_OPEN_DRAIN>;
> +            probe-names = "SCL", "SDA";
> +    };
> +
> +The binding documentation is in the ``misc`` folder of the Kernel binding
> +documentation.
(...)
> +++ b/Documentation/devicetree/bindings/misc/gpio-logic-analyzer.yaml

When other debugging tools for GPIO got DT bindings it was concluded that
it is possible to create bindings like this for debugging without even
specifying
any formal bindings. They are just for debugging after all.

Personally I like the bindings anyway.

> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig

I would consider housing this tool under drivers/gpio actually.
We have other funky things like gpio-sim and gpio-aggregator
so why not.

I would create a Kconfig menu with "GPIO hardware hacking tools".

But Bartosz would need to agree on that idea.

> +config GPIO_LOGIC_ANALYZER
> +       tristate "Simple GPIO logic analyzer"
> +       depends on GPIOLIB || COMPILE_TEST
> +       help

depends on EXPERT

I would say. Definitely not something for the average user.

Yours,
Linus Walleij

Re: [PATCH RFC/RFT 1/1] misc: add simple logic analyzer using polling

[Bartosz Golaszewski]

On Thu, Apr 1, 2021 at 3:08 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Tue, Mar 30, 2021 at 10:58 AM Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
>
> > This is a simple logic analyzer using GPIO polling. It comes with a
> > script to isolate a CPU for polling. While this is definately not a
> > production level analyzer, it can be a helpful first view when remote
> > debugging. Read the documentation for details.
> >
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> I am a great supporter of this idea.
>
> When we created gpiod_get_array_value() and friends, the idea
> was exactly to be able to do things like this. It's a good way to
> utilize the fact that several GPIO lines can often be read from a single
> register read.
>
> > +    i2c-analyzer {
> > +            compatible = "gpio-logic-analyzer";
> > +            probe-gpios = <&gpio6 21 GPIO_OPEN_DRAIN>, <&gpio6 4 GPIO_OPEN_DRAIN>;
> > +            probe-names = "SCL", "SDA";
> > +    };
> > +
> > +The binding documentation is in the ``misc`` folder of the Kernel binding
> > +documentation.
> (...)
> > +++ b/Documentation/devicetree/bindings/misc/gpio-logic-analyzer.yaml
>
> When other debugging tools for GPIO got DT bindings it was concluded that
> it is possible to create bindings like this for debugging without even
> specifying
> any formal bindings. They are just for debugging after all.
>
> Personally I like the bindings anyway.
>
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>
> I would consider housing this tool under drivers/gpio actually.
> We have other funky things like gpio-sim and gpio-aggregator
> so why not.
>

We have actually created a sub-menu for "Virtual GPIO drivers".

> I would create a Kconfig menu with "GPIO hardware hacking tools".
>
> But Bartosz would need to agree on that idea.
>

It's perfect! If we ever get something like a generic bitbanging
driver or something, it could go in there too.

Bart

> > +config GPIO_LOGIC_ANALYZER
> > +       tristate "Simple GPIO logic analyzer"
> > +       depends on GPIOLIB || COMPILE_TEST
> > +       help
>
> depends on EXPERT
>
> I would say. Definitely not something for the average user.
>
> Yours,
> Linus Walleij

Re: [PATCH RFC/RFT 1/1] misc: add simple logic analyzer using polling

[Wolfram Sang]

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

Hi Linus,

> I am a great supporter of this idea.

Great, thanks!

> When other debugging tools for GPIO got DT bindings it was concluded that
> it is possible to create bindings like this for debugging without even
> specifying
> any formal bindings. They are just for debugging after all.

So, I remove the yaml file and add the bindings to the documenation,
then? Makes sense to me because it is for debugging and not really
official.

> I would consider housing this tool under drivers/gpio actually.
> We have other funky things like gpio-sim and gpio-aggregator
> so why not.

Heh, my first draft was placed in drivers/gpio. I'd be happy to have it
there.

> I would create a Kconfig menu with "GPIO hardware hacking tools".
> 
> But Bartosz would need to agree on that idea.

Since he agreed, I'll update this in v2.

> > +config GPIO_LOGIC_ANALYZER
> > +       tristate "Simple GPIO logic analyzer"
> > +       depends on GPIOLIB || COMPILE_TEST
> > +       help
> 
> depends on EXPERT

Yes, good point!

All the best,

   Wolfram


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