Re: [PATCH RFC/RFT 1/1] misc: add simple logic analyzer using polling
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 signature.asc Description: PGP signature
Re: [PATCH RFC/RFT 1/1] misc: add simple logic analyzer using polling
On Thu, Apr 1, 2021 at 3:08 PM Linus Walleij wrote:
>
> On Tue, Mar 30, 2021 at 10:58 AM Wolfram Sang
> 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
>
> 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 = < 21 GPIO_OPEN_DRAIN>, < 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
On Tue, Mar 30, 2021 at 10:58 AM Wolfram Sang
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
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 = < 21 GPIO_OPEN_DRAIN>, < 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
> > +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! signature.asc Description: PGP signature
Re: [PATCH RFC/RFT 1/1] misc: add simple logic analyzer using polling
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 ..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 = < 21 GPIO_OPEN_DRAIN>, < 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
> +150`` 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 150 -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
> +
> +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
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
>
> 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
signature.asc
Description: PGP signature
Re: [PATCH RFC/RFT 1/1] misc: add simple logic analyzer using polling
On Tue, Mar 30, 2021 at 11:58 AM Wolfram Sang
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
...
> +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
...
> +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
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(>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, );
> +
> + 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, );
> + 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
[PATCH RFC/RFT 1/1] misc: add simple logic analyzer using polling
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
---
.../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 ..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 = < 21 GPIO_OPEN_DRAIN>, < 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
+150`` 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 150 -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
+
+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 ..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
+
+properties:
+ compatible:
+items:
+ - const: gpio-logic-analyzer
+
+ probe-gpios:
+description:
+ gpios used
