[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 <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

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
> +}

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

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.

> > +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!

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

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

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